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

[Jai Luthra <jai.luthra@ideasonboard.com>]

Hi Jacopo,

Quoting Jacopo Mondi (2026-07-27 12:16:53)
> 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@gmail.com>
> > >
> > > 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@gmail.com>
> > > Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> > >
> >
> > [...]
> >
> > > +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-  }
> 

Ah thanks for checking, I missed that requirement for using the helper.

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

I have a slight preference towards this option.

Many receiver drivers use v4l2_get_link_freq() to configure the PHY, so
even a single freq in the endpoint is useful. Plus it would keep the driver
logic simple too.

Thanks,
    Jai

> - 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@ideasonboard.com>
> 
> Thanks
>   j
> 
> >
> > Thanks,
> >     Jai