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 <g...@apitzsch.eu>
> >
> > Calculate PLL parameters based on clock frequency and link
> > frequency.
> >
> > Acked-by: Ricardo Ribalda <riba...@chromium.org>
> > Signed-off-by: André Apitzsch <g...@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);
