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

[Jacopo Mondi <jacopo.mondi@ideasonboard.com>]

Hi Jai

On Mon, Jul 27, 2026 at 01:13:24PM +0530, Jai Luthra wrote:
> 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.

Note that the driver registers V4L2_CID_LINK_FREQUENCY regardless of
the presence of the dts property.

I can make it required if it's simpler.

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