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

[Jai Luthra]

Hi Jacopo,

Quoting Jacopo Mondi (2026-07-23 14:46:07)
> Hi Jai
> 
> On Thu, Jul 23, 2026 at 01:40:46PM +0530, Jai Luthra wrote:
> > Quoting Jacopo Mondi (2026-07-23 12:51:48)
> > > Hi Jai,
> > >    thanks for the review
> > >
> > > On Wed, Jul 22, 2026 at 06:07:56PM +0530, Jai Luthra wrote:
> > > > Hi Jacopo,
> > > >
> > > > Thank you for the patch.
> > > >
> > > > Quoting Jacopo Mondi (2026-07-22 16:25:17)
> > > > > 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>
> >
> > [...]
> >
> > > > > +
> > > > > +/* Horizontal flip */
> > > > > +#define MIRA220_HFLIP_REG CCI_REG8(0x209C)
> > > > > +#define MIRA220_HFLIP_ENABLE_MIRROR 1
> > > > > +#define MIRA220_HFLIP_DISABLE_MIRROR 0
> > > > > +
> > > > > +/* Vertical flip */
> > > > > +#define MIRA220_VFLIP_REG CCI_REG8(0x1095)
> > > > > +#define MIRA220_VFLIP_ENABLE_FLIP 1
> > > > > +#define MIRA220_VFLIP_DISABLE_FLIP 0
> > > >
> > > > I don't see the above macros being used when updating the flip registers,
> > >
> > > We use ctrl->val indeed as it can be 1 or 0 only
> > >
> > > > and also HFLIP value is swapped from the control, which does not match the
> > > > macro.
> > >
> > > Why I don't see it ?
> > >
> > >        case V4L2_CID_HFLIP:
> > >                ret = cci_write(mira220->regmap, MIRA220_HFLIP_REG,
> > >                                mira220->hflip->val, NULL);
> > >
> > >         mira220->hflip = v4l2_ctrl_new_std(ctrl_hdlr, &mira220_ctrl_ops,
> > >                                            V4L2_CID_HFLIP, 0, 1, 1, 0);
> > >
> >
> > Oops, my bad I confused the code picking the bayer order from the table
> > with the one that writes to the registers. Please ignore this comment.
> >
> 
> There is instead an inverted handling of hflip when adjusting the
> image format
> 
> i = (i & ~3) | (mira220->vflip->val ? 2 : 0) | (mira220->hflip->val ? 0 : 1);
> 
> This is because the wrong sorting of mira220_mbus_color_formats[].
> 
> I've resorted it to match the ordering reported in the comment above
> it
> 
>  * - no flip
>  * - h flip
>  * - v flip
>  * - h&v flips
> 
> And adjusted the index calculation to use hflip->val ? 1 : 0
> 
> (Sashiko had also pointed this out, but thought this was going to
> registers, while flip handling used to work fine as it was "wrong" in
> two places in the driver).
> 

Aah good find.

> > >
> > > >
> > > > Could you please use the macros to keep things easy to read? And maybe a
> > > > mask macro instead of `(i & ~3)` too?
> > > >
> > >
> > > I could
> > >
> > > > > +
> > > > > +#define MIRA220_VSIZE_REG CCI_REG16_LE(0x1087)
> > > > > +#define MIRA220_VSTART_REG CCI_REG16_LE(0x107d)
> > > > > +#define MIRA220_HSIZE_REG CCI_REG16_LE(0x2008)
> > > > > +#define MIRA220_HSIZE_MASK GENMASK(9, 0)
> > > > > +#define MIRA220_HSTART_REG CCI_REG16_LE(0x200a)
> > > > > +#define MIRA220_HSTART_MASK GENMASK(9, 0)
> > > >
> > > > Same, drop the mask if unused.
> > > >
> > >
> > > Or maybe I should have used the masks!
> > >
> > > > > +#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.

> >
> > If you end up reading the DT property to report an error, might as well try
> > setting the bit too? ;-)
> >
> 
> I don't think I'm reading it, am I ?
> 

I meant that if you do end up accepting my suggestion and read the DT to
add a dev_warn() somewhere, you might as well set the bit because that's
easier.

[...]

> > > > > +/* Test Pattern */
> > > > > +#define MIRA220_REG_TEST_PATTERN CCI_REG8(0x2091)
> > > > > +#define MIRA220_TEST_PATTERN_DISABLE 0x00
> > > > > +#define MIRA220_TEST_PATTERN_VERTICAL_GRADIENT 0x01
> > > > > +
> > > > > +/* Module variant identifiers */
> > > > > +#define MIRA220_MONO_PLAIN_PF          0x1e62a032
> > > > > +#define MIRA220_MONO_PLAIN_NO_PF       0x1e62a045
> > > > > +#define MIRA220_MONO_AR_PF             0x1e62a036
> > > > > +#define MIRA220_RGB_PLAIN_PF           0x1e62a03b
> > > > > +#define MIRA220_RGB_PLAIN_NO_PF                0x1e62a046
> > > > > +#define MIRA220_RGB_AR_PF              0x1e62a03d
> > > >
> > > > Ah, I think this might be relevant to userspace, but not a problem to solve
> > > > in this patch.
> > > >
> > >
> > > How so ?
> > >
> >
> > I assumed AR coating and protective film might effect the tuning needed for
> > the ISP, but I honestly don't know enough about that process.
> >
> 
> One place where it is required to know the sensor variant is the
> tuning file selection in libcamera.
> 
> We currently don't have a standardized way to do that. The only
> pipeline that automatically adjust for _mono variants is RPi, and it
> does that using the support image formats.
> 
> I presume other pipelines should do the same ?
> 

I would guess so. I don't think that's easily solvable so I'm okay with the
current driver.

> > > > > +
> > > > > +enum mira220_variants {
> > > > > +       MIRA220_VARIANT_MONO,
> > > > > +       MIRA220_VARIANT_RGB,
> > > > > +};
> > > > > +
[...]
> > [...]
> >
> > > > > +static const struct of_device_id mira220_dt_ids[] = {
> > > > > +       { .compatible = "ams,mira220" },
> > > >
> > > > I see AMS-OSRAM using the following naming scheme on the product page [1]:
> > > >     Mira220-2QC: Color
> > > >     Mira220-2QM: Mono
> > > >     Mira220-2QI: RGB-IR
> > > >
> > > > Should we use separate compatibles for the three variants? This is a
> > > > practice we have followed for other sensors too, and especially relevant
> > > > here because the driver currently doesn't work for RGB-IR variants.
> > > >
> > > > [1]: https://ams-osram.com/products/sensor-solutions/cmos-image-sensors/ams-mira220
> > > >
> > >
> > > I don't know. As we can perform runtime identification what would an
> > > additional compatible give us here ?
> > >
> >
> > Some usecases might want to avoid powering on (and potentially flashing
> > privacy LED) and doing I2C transactions if the type of sensor can be known
> > at boot-time already.
> >
> > There was a similar discussion here:
> > https://lore.kernel.org/linux-media/20260521-dangling-utensil-610af5e07d79@spud/
> >
> > I don't know enough about this particular sensor, and if reading the OTP
> > values and programming registers is mandatory for all potential usecases
> > anyway, which would mean there's no benefit for avoiding the runtime
> > detection.
> 
> A bit out of my territory here, but can we start with a single
> compatible and add more if needed (and use the initial one as a
> fallback?)
> 

That's fine with me.

Thanks,
Jai