Re: [PATCH v5 5/5] i2c: qcom-cci: Enforce the required CCI clock rate

[Loic Poulain]

Hi Andi,


On Tue, Sep 29, 2026 at 1:28 AM Andi Shyti <andi.shyti@xxxxxxxxxx> wrote:
>
> Hi Loic,
>
> ...
>
> > @@ -318,7 +319,7 @@ static const struct hw_params *cci_get_hw_params(struct cci *cci, int mode)
> >       unsigned long rate = clk_get_rate(cci->cci_clk);
> >       int ri = cci_clk_rate_idx(rate);
> >
> > -     if (ri >= 0 && mode <= cci->data->max_mode && cci_hw_params[ri][mode].thigh)
> > +     if (ri >= 0 && mode <= cci->data->max_mode)
>
> why are you removing the .thigh check here?

Because it is now redundant, the mode content is already validated
during core clock selection in probe. At this point, we only ensure
that the mode value is a valid index.
This follows Vlad's comment:
https://lore.kernel.org/all/CAFEp6-1GCa2t3wBznoF-HnmtNCUSsshnWnrv8O6R2_06KqgKQA@xxxxxxxxxxxxxx/

>
> >               return &cci_hw_params[ri][mode];
> >
> >       return NULL;
>
> ...
>
> > +static int cci_set_core_rate(struct cci *cci)
> > +{
> > +     struct device *dev = cci->dev;
> > +     unsigned long rate;
> > +     int ret;
> > +
> > +     ret = cci_get_required_rate(cci, &rate);
> > +     if (ret) {
> > +             dev_err(dev, "no CCI clock rate satisfies all masters\n");
> > +             return ret;
> > +     }
> > +
> > +     ret = dev_pm_opp_set_rate(dev, rate);
> > +     if (ret) {
> > +             dev_warn(dev, "CCI clock could not be set to %lu Hz\n", rate);
> > +             return ret;
> > +     }
> > +
> > +     /*
> > +      * Sanity: The hw_params timings are only valid at the exact
> > +      * expected rate, verify what landed on the hardware.
> > +      */
> > +     if (clk_get_rate(cci->cci_clk) != rate)
> > +             dev_warn(dev, "CCI clock is not at expected %lu Hz\n", rate);
> > +
> > +     return 0;
> > +}
> > +
> >  static int __maybe_unused cci_suspend_runtime(struct device *dev)
> >  {
> >       struct cci *cci = dev_get_drvdata(dev);
> > @@ -678,6 +739,19 @@ static int cci_probe(struct platform_device *pdev)
> >               return dev_err_probe(dev, PTR_ERR(cci->cci_clk),
> >                                    "failed to get CCI clock\n");
> >
> > +     ret = devm_pm_opp_set_clkname(dev, "cci");
> > +     if (ret)
> > +             return dev_err_probe(dev, ret, "failed to set CCI OPP clk\n");
> > +
> > +     /* OPP table is optional */
> > +     ret = devm_pm_opp_of_add_table(dev);
> > +     if (ret && ret != -ENODEV)
> > +             return dev_err_probe(dev, ret, "failed to add OPP table\n");
> > +
> > +     ret = cci_set_core_rate(cci);
> > +     if (ret)
> > +             return dev_err_probe(dev, ret, "failed to set CCI clock rate\n");
>
> The error logging is a bit messed up here: you either print here
> or in cci_set_core_rate(). Besides why is a warning there and an
> error here? I would just print error in cci_set_core_rate() and
> return here.

Right, will fix that.

Thanks,
Loic

>
> Andi
>
> > +