On Fri, 2022-04-08 at 13:54 -0700, Kevin Hilman wrote: > Rex-BC Chen <rex-bc.chen@xxxxxxxxxxxx> writes: > > > From: Jia-Wei Chang <jia-wei.chang@xxxxxxxxxxxx> > > > > In some MediaTek SoCs, like MT8183, CPU and CCI share the same > > power > > supplies. Cpufreq needs to check if CCI devfreq exists and wait > > until > > CCI devfreq ready before scaling frequency. > > > > - Add is_ccifreq_ready() to link CCI device to CPI, and CPU will > > start > > DVFS when CCI is ready. > > - Add platform data for MT8183. > > > > Signed-off-by: Jia-Wei Chang <jia-wei.chang@xxxxxxxxxxxx> > > The checks here are not enough, and will lead to unexpected behavior. > IIUC, before doing DVFS, you're checking: > > 1) if the "cci" DT node is present and > 2) if the driver for that device is bound > > If both those conditions are not met, you don't actually fail, you > just > silently do nothing in ->set_target(). As Angelo pointed out also, > this > is not a good idea, and will be rather confusing to users. > > The same thing would happen if the cci DT node was present, but the > CCI > devfreq driver was disabled. Silent failure would also be quite > unexpected behavior. Similarily, if the cci DT node is not present > at > all (like it is in current upstream DT), this CPUfreq driver will > silently do nothing. Not good. > > So, this patch needs to handle several scenarios: > > 1) CCI DT node not present > > In this case, the driver should still operate normally. With no CCI > node, or driver there's no conflict. > > 2) CCI DT present/enabled but not yet bound > > In this case, you could return -EAGAIN as suggested by Angelo, or > maybe > better, it should do a deferred probe. > > 3) CCI DT present, but driver disabled > > This case is similar to (1), this driver should continue to work. > > Kevin Hello Kevin, Thanks for your review. I will review these three situations and do some modification. BRs, Rex
