Re: [PATCH RESEND v4 3/5] media: i2c: imx214: Make use of CCS PLL calculator

["André Apitzsch" <git@apitzsch.eu>]

Hi Laurent,

thanks for the review. Some comments below.


Am Samstag, dem 21.06.2025 um 21:17 +0300 schrieb Laurent Pinchart:
> Hi André,
> 
> Thank you for the patch.
> 
> On Sat, Jun 21, 2025 at 11:37:27AM +0200, André Apitzsch via B4 Relay
> wrote:
> > From: André Apitzsch <git@apitzsch.eu>
> > 
> > Calculate PLL parameters based on clock frequency and link
> > frequency.
> > 
> > Acked-by: Ricardo Ribalda <ribalda@chromium.org>
> > Signed-off-by: André Apitzsch <git@apitzsch.eu>
> > ---
> >  drivers/media/i2c/Kconfig  |   1 +
> >  drivers/media/i2c/imx214.c | 213
> > ++++++++++++++++++++++++++++++++++++---------
> >  2 files changed, 175 insertions(+), 39 deletions(-)
> > 
> > diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig
> > index
> > e68202954a8fd4711d108cf295d5771246fbc406..08db8abeea218080b0bf5bfe6
> > cf82f1c0b100c4a 100644
> > --- a/drivers/media/i2c/Kconfig
> > +++ b/drivers/media/i2c/Kconfig
> > [..]
> > @@ -1224,42 +1336,52 @@ static int imx214_parse_fwnode(struct
> > device *dev)
> >   if (!endpoint)
> >   return dev_err_probe(dev, -EINVAL, "endpoint node not found\n");
> >  
> > - ret = v4l2_fwnode_endpoint_alloc_parse(endpoint, &bus_cfg);
> > + bus_cfg->bus_type = V4L2_MBUS_CSI2_DPHY;
> > + ret = v4l2_fwnode_endpoint_alloc_parse(endpoint, bus_cfg);
> > + fwnode_handle_put(endpoint);
> 
> ... drop this. Up to you.
> 
> >   if (ret) {
> >   dev_err_probe(dev, ret, "parsing endpoint node failed\n");
> > - goto done;
> > + goto error;
> 
> You can return ret here.
> 
> >   }
> >  
> >   /* Check the number of MIPI CSI2 data lanes */
> > - if (bus_cfg.bus.mipi_csi2.num_data_lanes != 4) {
> > + if (bus_cfg->bus.mipi_csi2.num_data_lanes != 4) {
> >   ret = dev_err_probe(dev, -EINVAL,
> >       "only 4 data lanes are currently supported\n");
> > - goto done;
> > + goto error;
> >   }
> >  
> > - if (bus_cfg.nr_of_link_frequencies != 1)
> > + if (bus_cfg->nr_of_link_frequencies != 1)
> >   dev_warn(dev, "Only one link-frequency supported, please review
> > your DT. Continuing anyway\n");
> 
> Now that the driver can calculate PLL parameters dynamically, it
> would be nice to lift this restriction and make the link frequency
> control writable, in a separate patch on top of this series.
> 
Maybe this could be postponed, as I don't have any use for it at the
moment and I don't want to further delay this series.

> >  
> > - for (i = 0; i < bus_cfg.nr_of_link_frequencies; i++) {
> > - if (bus_cfg.link_frequencies[i] == IMX214_DEFAULT_LINK_FREQ)
> > + for (i = 0; i < bus_cfg->nr_of_link_frequencies; i++) {
> > + u64 freq = bus_cfg->link_frequencies[i];
> > + struct ccs_pll pll;
> > +
> > + if (!imx214_pll_calculate(imx214, &pll, freq))
> >   break;
> > - if (bus_cfg.link_frequencies[i] ==
> > -     IMX214_DEFAULT_LINK_FREQ_LEGACY) {
> > + if (freq == IMX214_DEFAULT_LINK_FREQ_LEGACY) {
> >   dev_warn(dev,
> >   "link-frequencies %d not supported, please review your DT.
> > Continuing anyway\n",
> >   IMX214_DEFAULT_LINK_FREQ);
> > + freq = IMX214_DEFAULT_LINK_FREQ;
> > + if (imx214_pll_calculate(imx214, &pll, freq))
> > + continue;
> > + bus_cfg->link_frequencies[i] = freq;
> >   break;
> >   }
> 
> How about separating the IMX214_DEFAULT_LINK_FREQ_LEGACY check from
> the PLL calculation ? Something like
> 
>  u64 freq = bus_cfg->link_frequencies[i];
>  struct ccs_pll pll;
> 
>  if (freq == IMX214_DEFAULT_LINK_FREQ_LEGACY) {
>  dev_warn(dev,
>  "link-frequencies %d not supported, please review your DT.
> Continuing anyway\n",
>  IMX214_DEFAULT_LINK_FREQ);
>  freq = IMX214_DEFAULT_LINK_FREQ;
>  bus_cfg->link_frequencies[i] = freq;
>  }

With PLL calculation, 480000000 (=IMX214_DEFAULT_LINK_FREQ_LEGACY)
might be a valid link frequency explicitly set by the user. I'm not
sure whether it is a good idea to overwrite the link frequency, before
trying the PLL calculation. That's why I would keep the code the way it
is.

> 
>  if (!imx214_pll_calculate(imx214, &pll, freq))
>  break;
> 
> It will then become easier to drop this legacy support from the
> driver. What platform(s) are know to specify an incorrect link
> frequency ?

I don't know.

Best regards,
André

> 
> >   }
> >  
> > - if (i == bus_cfg.nr_of_link_frequencies)
> > + if (i == bus_cfg->nr_of_link_frequencies)
> >   ret = dev_err_probe(dev, -EINVAL,
> > -     "link-frequencies %d not supported, please review your DT\n",
> > -     IMX214_DEFAULT_LINK_FREQ);
> > +     "link-frequencies %lld not supported, please review your
> > DT\n",
> > +     bus_cfg->nr_of_link_frequencies ?
> > +     bus_cfg->link_frequencies[0] : 0);
> >  
> > -done:
> > - v4l2_fwnode_endpoint_free(&bus_cfg);
> > - fwnode_handle_put(endpoint);
> > + return 0;
> > +
> > +error:
> > + v4l2_fwnode_endpoint_free(&imx214->bus_cfg);
> >   return ret;
> >  }
> >  
> > @@ -1299,7 +1421,7 @@ static int imx214_probe(struct i2c_client
> > *client)
> >   return dev_err_probe(dev, PTR_ERR(imx214->regmap),
> >        "failed to initialize CCI\n");
> >  
> > - ret = imx214_parse_fwnode(dev);
> > + ret = imx214_parse_fwnode(dev, imx214);
> >   if (ret)
> >   return ret;
> >  
> > @@ -1310,7 +1432,9 @@ static int imx214_probe(struct i2c_client
> > *client)
> >   * Enable power initially, to avoid warnings
> >   * from clk_disable on power_off
> >   */
> > - imx214_power_on(imx214->dev);
> > + ret = imx214_power_on(imx214->dev);
> > + if (ret < 0)
> > + goto error_fwnode;
> 
> This change seems to belong to a separate patch.
> 
> >  
> >   ret = imx214_identify_module(imx214);
> >   if (ret)
> > @@ -1341,6 +1465,12 @@ static int imx214_probe(struct i2c_client
> > *client)
> >   pm_runtime_set_active(imx214->dev);
> >   pm_runtime_enable(imx214->dev);
> >  
> > + ret = imx214_pll_update(imx214);
> > + if (ret < 0) {
> > + dev_err_probe(dev, ret, "failed to update PLL\n");
> > + goto error_subdev_cleanup;
> > + }
> 
> I would move this to imx214_ctrls_init().
> 
> > +
> >   ret = v4l2_async_register_subdev_sensor(&imx214->sd);
> >   if (ret < 0) {
> >   dev_err_probe(dev, ret,
> > @@ -1366,6 +1496,9 @@ static int imx214_probe(struct i2c_client
> > *client)
> >  error_power_off:
> >   imx214_power_off(imx214->dev);
> >  
> > +error_fwnode:
> > + v4l2_fwnode_endpoint_free(&imx214->bus_cfg);
> > +
> >   return ret;
> >  }
> >  
> > @@ -1378,6 +1511,8 @@ static void imx214_remove(struct i2c_client
> > *client)
> >   v4l2_subdev_cleanup(sd);
> >   media_entity_cleanup(&imx214->sd.entity);
> >   v4l2_ctrl_handler_free(&imx214->ctrls);
> > + v4l2_fwnode_endpoint_free(&imx214->bus_cfg);
> > +
> >   pm_runtime_disable(&client->dev);
> >   if (!pm_runtime_status_suspended(&client->dev)) {
> >   imx214_power_off(imx214->dev);