Re: [RFC PATCH v3 1/2] dt-bindings: media: i2c: Add onsemi AR0234 image sensor binding

[Conor Dooley <conor@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 5999 bytes --]

On Tue, May 05, 2026 at 07:37:13PM +0300, Laurent Pinchart wrote:
> On Tue, May 05, 2026 at 05:09:18PM +0300, Alexander Shiyan wrote:
> > > On Fri, Mar 06, 2026 at 01:36:13PM +0300, Alexander Shiyan wrote:
> > > > Add devicetree binding for the onsemi AR0234 CMOS image sensor.
> > > >
> > > > Signed-off-by: Alexander Shiyan <eagle.alexander923@gmail.com>
> > > > ---
> > > >  .../bindings/media/i2c/onnn,ar0234.yaml       | 109 ++++++++++++++++++
> > > >  1 file changed, 109 insertions(+)
> > > >  create mode 100644 Documentation/devicetree/bindings/media/i2c/onnn,ar0234.yaml
> > > >
> > > > diff --git a/Documentation/devicetree/bindings/media/i2c/onnn,ar0234.yaml b/Documentation/devicetree/bindings/media/i2c/onnn,ar0234.yaml
> > > > new file mode 100644
> > > > index 000000000000..d93fa99e6535
> > > > --- /dev/null
> > > > +++ b/Documentation/devicetree/bindings/media/i2c/onnn,ar0234.yaml
> > > > @@ -0,0 +1,109 @@
> > > > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > > > +%YAML 1.2
> > > > +---
> > > > +$id: http://devicetree.org/schemas/media/i2c/onnn,ar0234.yaml#
> > > > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > > > +
> > > > +title: ON Semiconductor AR0234 1/2.6-inch CMOS Digital Image Sensor
> > > > +
> > > > +description:
> > > > +  The AR0234 is a 1/2.6-inch CMOS digital image sensor with a pixel
> > > > +  array of 1940x1220 pixels, capable of 1920x1200 resolution at up
> > > > +  to 120 fps. It supports MIPI CSI-2 output with 1, 2, or 4 data lanes,
> > > > +  and raw Bayer (8/10-bit) or monochrome output.
> > > > +
> > > > +properties:
> > > > +  compatible:
> > > > +    const: onnn,ar0234cs
> > >
> > > Should we define separate compatible strings for the mono and colour
> > > variants ? I know you identify the variant at runtime in the driver, but
> > > avoid I2C communication at boot time can be beneficial (to reduce boot
> > > time, and also to avoid flashing the privacy LED on systems that have
> > > one, albeit the latter is probably less applicable to the AR0234).
> > 
> > We could do it like this — Color: ar0234cssc, Mono: ar0234cssm.
> > But the current approach is more universal...
> > Could we add two compatible strings and keep the base one for auto-detection?
> > For detection, it would still be good to check the identifier anyway...
> > Or just add two compatible strings but detect connected variant in any case?
> 
> I see multiple use cases:
> 
> 1. You know at build time that you have an AR0234CS, but the model (mono
>    or colour) is only known at runtime (e.g. a product exists in mono and
>    colour options, with the options being otherwise identical).
> 
>    This can be implemented with a single compatible string
>    "onnn,ar0234cs" and a runtime check of the model in the driver, *or*
>    with two compatible strings for the two models and a runtime check in
>    the boot loader that will set the correct compatible string.

You can kinda have both of these, runtime detection on a common fallback
compatible, and the bootloader selecting the right dtb/overlay etc with
specific ones.

> 
> 2. You know at build time what exact camera module you have, and you
>    want to avoid powering the sensor up at boot time (e.g. boot time
>    optimization, avoiding privacy LED flashing, ...).
> 
>    This requires two separate compatible strings for the two models.
> 
> 3. You know at build time what exact camera module you have, and you
>    want a runtime sanity check.
> 
>    This requires two separate compatible strings for the two models.

Maybe in media land this kind of thing is okay, because the likelihood
of compatibility between models is low, but overall I dislike this kind
of thing as it causes problems when using fallbacks. Perhaps here that
doesn't matter at all because you're only doing colour/mono
differentiation, not validating that this is an ar0234cs?

> In order to cover all those use cases, we could use
> 
> properties:
>   compatible:
>     enum:
>       - onnn,ar0234cssc
>       - onnn,ar0234cssm
>       - onnn,ar0234cs
> 
> The first two compatible strings would cover use case 1 with the boot
> loader detection, use case 2, and use case 3. The last compatible string
> would cover use case 1 without the boot loader detection. This is waht
> the sony,imx296.yaml binding does. The sony,imx290.yaml binding, on the
> other hand, has deprecated the generic compatible string.
> 
> We could also use
> 
> properties:
>   compatible:
>     oneOf:
>       - const: onnn,ar0234cs
>       - items:
>           - enum:
>               - onnn,ar0234cssc
>               - onnn,ar0234cssm
>           - const: onnn,ar0234cs
> 
> if we want a fallback compatible string, which could allow systems that
> only case about use case 1 without boot loader detection to only match
> on "onnn,ar0234cs" in their driver. I don't think we have any such
> bindings for image sensors.
> 
> I don't think we've decided on a recommended practice.

I definitely don't like the first suggestion here, the second is more
acceptable. With the first, it definitely seems just like abusing the
compatible property to have different behaviour for the exact same
hardware.

| properties:
|   compatible:
|     items: 
|       - enum:
|           - onnn,ar0234cssc
|           - onnn,ar0234cssm
|       - const: onnn,ar0234cs

Can also do this, but have all three devices in the driver, to promote
completely describing a device while also supporting lazy/cheap vendors
that want to reuse a dtb rather than implement two images or a selection
method. Obviously it'd produce dtbs_check warnings, but it is not as if
those sorts of vendors care about dtbs_check.
I mention vendors specifically here, since an end user can probably
update their dtb once they figure out which device they actually have,
which I think shouldn't be very difficult!


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]