Re: [PATCH v8 2/2] media: i2c: Add driver for AMS-OSRAM Mira220

[Jacopo Mondi]

Hi Jai

On Sat, Jul 25, 2026 at 12:27:25AM +0530, Jai Luthra wrote:
> Hi Jacopo,
>
> Thank you for the fixes!
>
> Quoting Jacopo Mondi (2026-07-24 21:07:01)
> > From: Philippe Baetens <philippebaetens@xxxxxxxxx>
> >
> > Add a V4L2 subdev driver for driver for the AMS-OSRAM Mira220 image
> > sensor.
> >
> > Mira220 is a global shutter image sensor with a resolution of 1600x1400
> > pixels.
> >
> > The driver implements support for mono and RGB 12, 10 and 8 bits
> > formats. The output data-rate per lane is 1500Mbit/s, with a maximum
> > frame rate up to 90 fps.
> >
> > Signed-off-by: Philippe Baetens <philippebaetens@xxxxxxxxx>
> > Signed-off-by: Jacopo Mondi <jacopo.mondi@xxxxxxxxxxxxxxxx>
> >
>
> [...]
>
> > +static int mira220_parse_endpoint(struct device *dev, struct mira220 *mira220)
> > +{
> > +       struct fwnode_handle *endpoint;
> > +       struct v4l2_fwnode_endpoint ep_cfg = {
> > +               .bus_type = V4L2_MBUS_CSI2_DPHY
> > +       };
> > +       int ret = 0;
> > +
> > +       endpoint = fwnode_graph_get_endpoint_by_id(dev_fwnode(dev), 0, 0, 0);
> > +       if (!endpoint) {
> > +               dev_err(dev, "Endpoint node not found\n");
> > +               return -EINVAL;
> > +       }
> > +
> > +       if (v4l2_fwnode_endpoint_alloc_parse(endpoint, &ep_cfg)) {
> > +               ret = -EINVAL;
> > +               dev_err(dev, "Failed to parse endpoint\n");
> > +               goto error_out;
> > +       }
> > +
> > +       /* Non-continuous mode not implemented. */
> > +       if (ep_cfg.bus.mipi_csi2.flags & V4L2_MBUS_CSI2_NONCONTINUOUS_CLOCK) {
> > +               dev_warn(dev, "clock non-continuous mode not supported\n");
> > +               ret = -EINVAL;
> > +               goto error_out;
> > +       }
> > +
> > +       if (ep_cfg.nr_of_link_frequencies > ARRAY_SIZE(mira220_link_freqs)) {
> > +               ret = -EINVAL;
> > +               dev_err(dev, "Unsupported number of link_frequencies: %u\n",
> > +                       ep_cfg.nr_of_link_frequencies);
> > +               goto error_out;
> > +       }
> > +
> > +       if (ep_cfg.link_frequencies[0] != MIRA220_LINK_FREQ_750M) {
>
> The LLM-bot says link_frequencies[0] might be invalid if DT omitted the
> link-frequencies property, and I think that's a valid concern.

Yeah the bot is right, my bad

I thought the property was required by fw, but it's not.

>
> Any particular reason for not using the v4l2_link_freq_to_bitmap() helper I

I think I missed it from your previous comment, sorry :)

Now that I look at it, the function wants link_frequencies from fw

drivers/media/v4l2-core/v4l2-common.c-  if (!num_of_fw_link_freqs) {
drivers/media/v4l2-core/v4l2-common.c-          dev_err(dev, "no link frequencies in firmware\n");
drivers/media/v4l2-core/v4l2-common.c-          return -ENODATA;
drivers/media/v4l2-core/v4l2-common.c-  }

Should I:

- make link_frequencies required in bindings (something that doesn't
  seem that useful considering a single freq is supported)

- only call v4l2_link_freq_to_bitmap() if the property is specified ?

?

> had suggested? It can handles all of these cases and error msgs too.
>
> With the edge case fixed (with or without the helper):
>
> Reviewed-by: Jai Luthra <jai.luthra@xxxxxxxxxxxxxxxx>

Thanks
  j

>
> Thanks,
>     Jai
> > +               ret = -EINVAL;
> > +               dev_err(dev, "Unsupported link_frequency: %llu\n",
> > +                       ep_cfg.link_frequencies[0]);
> > +               goto error_out;
> > +       }
> > +
> > +       /* Check the number of MIPI CSI2 data lanes */
> > +       if (ep_cfg.bus.mipi_csi2.num_data_lanes != 1 &&
> > +           ep_cfg.bus.mipi_csi2.num_data_lanes != 2) {
> > +               ret = -EINVAL;
> > +               dev_err(dev, "%u data lanes are not supported\n",
> > +                       ep_cfg.bus.mipi_csi2.num_data_lanes);
> > +               goto error_out;
> > +       }
> > +
> > +       mira220->lanes = ep_cfg.bus.mipi_csi2.num_data_lanes;
> > +       mira220->row_length = MIRA220_ROW_LENGTH_MIN * (2 / mira220->lanes);
> > +
> > +error_out:
> > +       v4l2_fwnode_endpoint_free(&ep_cfg);
> > +       fwnode_handle_put(endpoint);
> > +
> > +       return ret;
> > +}
> > +