[PATCH v5 2/9] cpufreq: mediatek: Add opp notification support

[Rex-BC Chen]

On Thu, 2022-05-05 at 14:13 +0530, Viresh Kumar wrote:
> On 04-05-22, 21:05, Rex-BC Chen wrote:
> > From: "Andrew-sh.Cheng" <andrew-sh.cheng at mediatek.com>
> > 
> > > From this opp notifier, cpufreq should listen to opp notification
> > > and do
> 
> What happened with the extra ">" here ?
> 

Hello Viresh,

Sorry for this. I don't know why it appear this ">".
In my patch, there is no ">" here.
I will check this..

> >  static int mtk_cpu_dvfs_info_init(struct mtk_cpu_dvfs_info *info,
> > int cpu)
> >  {
> >  	struct device *cpu_dev;
> > @@ -396,6 +458,17 @@ static int mtk_cpu_dvfs_info_init(struct
> > mtk_cpu_dvfs_info *info, int cpu)
> >  	info->intermediate_voltage = dev_pm_opp_get_voltage(opp);
> >  	dev_pm_opp_put(opp);
> >  
> > +	info->opp_cpu = cpu;
> > +	info->opp_nb.notifier_call = mtk_cpufreq_opp_notifier;
> > +	ret = dev_pm_opp_register_notifier(cpu_dev, &info->opp_nb);
> > +	if (ret) {
> > +		dev_err(cpu_dev, "cpu%d: failed to register opp
> > notifier\n", cpu);
> > +		goto out_disable_inter_clock;
> > +	}
> > +
> > +	mutex_init(&info->reg_lock);
> 
> You should always initialize a resource before its users. The
> notifier
> callback, which can get called right after
> dev_pm_opp_register_notifier() returns, will use this mutex.
> 

ok, I will move mutex_init(&info->reg_lock) before
dev_pm_opp_register_notifier().

> > +	info->opp_freq = clk_get_rate(info->cpu_clk);
> > +
> >  	/*
> >  	 * If SRAM regulator is present, software "voltage tracking" is
> > needed
> >  	 * for this CPU power domain.
> > @@ -451,6 +524,9 @@ static void mtk_cpu_dvfs_info_release(struct
> > mtk_cpu_dvfs_info *info)
> >  	}
> >  
> >  	dev_pm_opp_of_cpumask_remove_table(&info->cpus);
> > +
> > +	if (!IS_ERR_OR_NULL(info->cpu_dev))
> 
> cpu_dev can never be error here.

ok, I will drop this.

> 
> > +		dev_pm_opp_unregister_notifier(info->cpu_dev, &info-
> > >opp_nb);
> >  }
> >  
> >  static int mtk_cpufreq_init(struct cpufreq_policy *policy)
> 
> I also asked you last time to stack things in a order so they are
> easier for me to apply. Bugfixes, followed by simple cleanups, which
> don't make behavioral changes, followed by real patches.
> 
> Now you have sent this patch at an early stage, which blocks me from
> applying anything after this.
> 
> I can see the earlier comments weren't all considered, and it doesn't
> look nice.
> 

Sorry for inconvenience.
I will move "cpufreq: mediatek: Move voltage limits to platform data"
before this patch.

The order will be:
//cleanup and bug fix
new patch for add "platform_device_unregister(cpufreq_pdev);"
cpufreq: mediatek: Move voltage limits to platform data

//driver refinement
cpufreq: mediatek: Refine mtk_cpufreq_voltage_tracking()

//new feature
cpufreq: mediatek: Add opp notification support
cpufreq: mediatek: Link CCI device to CPU

//support new soc
cpufreq: mediatek: Add support for MT8186

//dts
arm64: dts: mediatek: Add opp table and clock property for MT8183
cpufreq
arm64: dts: mediatek: Add MediaTek CCI node for MT8183
arm64: dts: mediatek: Add mediatek, cci property for MT8183 cpufreq

BRs,
Rex