Re: [PATCH 2/3] devfreq: mediatek: add mt8183 cci devfreq driver

From: Johnson Wang <johnson.wang@mediatek.com>
To: Kevin Hilman <khilman@kernel.org>,
	Hsin-Yi Wang <hsinyi@google.com>,
	"Tim Chang" <jia-wei.chang@mediatek.com>
Cc: MyungJoo Ham <myungjoo.ham@samsung.com>,
	Kyungmin Park <kyungmin.park@samsung.com>,
	Chanwoo Choi <cw00.choi@samsung.com>,
	"Rob Herring" <robh+dt@kernel.org>,
	Matthias Brugger <matthias.bgg@gmail.com>,
	Liam Girdwood <lgirdwood@gmail.com>,
	Mark Brown <broonie@kernel.org>, <linux-pm@vger.kernel.org>,
	<devicetree@vger.kernel.org>,
	<linux-arm-kernel@lists.infradead.org>,
	<linux-mediatek@lists.infradead.org>,
	<linux-kernel@vger.kernel.org>, <fan.chen@mediatek.com>,
	<louis.yu@mediatek.com>, <roger.lu@mediatek.com>,
	<Allen-yy.Lin@mediatek.com>,
	<Project_Global_Chrome_Upstream_Group@mediatek.com>,
	Jia-Wei Chang <jia-wei.chang@mediatek.corp-partner.google.com>
Subject: Re: [PATCH 2/3] devfreq: mediatek: add mt8183 cci devfreq driver
Date: Fri, 8 Apr 2022 10:53:11 +0800	[thread overview]
Message-ID: <79b785cada5bdf65673a272255ba22bdb4b55df9.camel@mediatek.com> (raw)
In-Reply-To: <7h7d808sti.fsf@baylibre.com>

On Thu, 2022-04-07 at 14:52 -0700, Kevin Hilman wrote:
> Hsin-Yi Wang <hsinyi@google.com> writes:
> 
> > On Mon, Mar 7, 2022 at 8:32 PM Tim Chang <
> > jia-wei.chang@mediatek.com> wrote:
> > > 
> > > This adds a devfreq driver for the Cache Coherent Interconnect
> > > (CCI) of
> > > the Mediatek MT8183.
> > > 
> > > On mt8183 the cci is supplied by the same regulator as the little
> > > cores.
> > > The driver is notified when the regulator voltage changes (driven
> > > by
> > > cpufreq) and adjusts the cci frequency to the maximum possible
> > > value.
> > > 
> > > Add need_voltage_tracking variable to platforma data. if true, it
> > > indicates soc is required to realize the voltage tracking between
> > > voltage of sram and voltage of cci by software approach.
> > > otherwise, the
> > > voltage tracking is realized by hardware appraoch.
> > > 
> > > Add the notifier to cci so that it could react after svs driver
> > > changes
> > > opp table of cci.
> > > 
> > > Signed-off-by: Jia-Wei Chang <
> > > jia-wei.chang@mediatek.corp-partner.google.com>
> 
> [...]
> 
> > > +
> > > +       passive_data = devm_kzalloc(cci_dev, sizeof(struct
> > > devfreq_passive_data), GFP_KERNEL);
> > > +       if (!passive_data) {
> > > +               ret = -ENOMEM;
> > > +               goto out_remove_opp_table;
> > > +       }
> > > +
> > > +       passive_data->parent_type = CPUFREQ_PARENT_DEV;
> > 
> > It's better to add a note below commit message to state that this
> > series depends on
> > 
https://urldefense.com/v3/__https://git.kernel.org/pub/scm/linux/kernel/git/chanwoo/linux.git/log/?h=devfreq-testing__;!!CTRNKA9wMg0ARbw!3dQn_JiiD_cRiN-TmoNWXX7mD2cmF4M5elG4WabiS392Mg9S8wlsagK1o-n9beULMei4AAj9zsoPBElt9R4-UnAd1g$
> >  
> 
> Not only is this series dependent the previous series from Chanwoo,
> in
> also fails to compile if CONFIG_DEVFREQ_GOV_PASSIVE is not enabled,
> because CPUFREQ_PARENT_DEV defined inside of an #ifdef.
> 
> Please compile test this with and without CONFIG_DEVFREQ_GOV_PASSIVE
> enabled.
> 
> Kevin

Hi Kevin,

Thank you for review.
I will fix it in next verison.

BRs,
Johnson Wang