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

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

Hi André,

On Sun, Jun 22, 2025 at 05:34:56PM +0200, André Apitzsch wrote:
> Am Samstag, dem 21.06.2025 um 21:17 +0300 schrieb Laurent Pinchart:
> > 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.

When I said "on top", I didn't mean in a new version of this series. We
can merge this first, and then lift this restriction. I don't have an
imx214-based device so I can't do it myself and test it :-/

> > > - 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.

The current code accepts both IMX214_DEFAULT_LINK_FREQ (600 MHz) and
IMX214_DEFAULT_LINK_FREQ_LEGACY (400 MHz), and programs the PLL with (as
far as I understand) a 600 MHz clock frequency in either case. To avoid
a change in behaviour, I think overriding the 400 MHz frequency with 600
MHz in this patch would be best. We could then drop that in a later
patch, possibly by patching the clock frequency in a platform-specific
driver instead of the imx214 driver.

> >  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.

Ricardo, do you have any information about this ?

> > >   }
> > >  
> > > - 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);

-- 
Regards,

Laurent Pinchart