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

[Jai Luthra]

Quoting Jacopo Mondi (2026-07-23 16:47:09)
> Hi Jai
> 
> On Thu, Jul 23, 2026 at 03:20:21PM +0530, Jai Luthra wrote:
> > Hi Jacopo,
> >
> > Quoting Jacopo Mondi (2026-07-23 14:46:07)
> > > Hi Jai
> > >
> 
> [snip]
> 
> > > > > > > +#define MIRA220_MIPI_HSIZE_REG CCI_REG16_LE(0x207d)
> > > > > > > +
> > > > > > > +/* MIPI bus control */
> > > > > > > +#define MIRA220_MIPI_LANES_REG CCI_REG8(0x6012)
> > > > > > > +
> > > > > > > +#define MIRA220_MIPI_CLK_MODE_REG CCI_REG8(0x6013)
> > > > > > > +#define MIRA220_MIPI_CLK_CONTINUOUS 0x00
> > > > > > > +#define MIRA220_MIPI_CLK_NON_CONTINUOUS 0x01
> > > > > >
> > > > > > Does DT allow clock-continuous property? Do we handle it?
> > > > > >
> > > > >
> > > > > The bindings for this sensor allows all properties from
> > > > > video-interfaces.yaml, including clock-noncontinuous.
> > > > >
> > > > > The driver doesn't support it at the moment and unconditionally use
> > > > > continuous clock, but I guess it's nice to provide macros for the
> > > > > non-continuous configuration as well
> > > >
> > > > Ah, that's not too good if the DT allows it but the driver silently ignores
> > > > it.
> > >
> > > Why ?
> > >
> > > We allow all properties from video-interfaces.yaml (something which I
> > > think it's wrong, but that's a long time ago discussion with dt
> > > maintainers, so I'm not contesting that) and we certainly don't handle
> > > them in drivers (in facts, what drivers do is not relevant to which
> > > properties are allowed for a device).
> > >
> >
> > I am not saying the bindings shouldn't support that property, bindings are
> > for hardware. And yes you are free to not implement all features in the
> > driver.
> >
> > But given the bindings and the hardware both support toggling between
> > continuous or non-continous MIPI clock, from a hardware engineer or
> > integrators perspective it would be quite confusing that setting the field
> > in DT will do nothing, with *no* warning to the user either during the DT
> > validation or the driver probe that it was ignored.
> >
> 
> True that.
> 
> But does this mean I should be doing the same for all the properties
> that apply to the hardware ?
> 
> clock-noncontinuous:

Yes, to me it seems like a low-effort win.

> link-frequencies:

Supporting different rates is hard, but at least driver should print a
warning if the DT supplied frequency is not supported today. Most driver
use v4l2_link_freq_to_bitmap() to make life easy.

> lane-polarities:

Does the sensor hardware support flipping polarities? I've seen some
bridges supporting that, in which case they should handle it instead.

But let's not "perfect be the enemy of good" :-)

> ?
> 

Thanks,
    Jai