Re: [PATCH v3 1/2] dt-bindings: media: i2c: Add Sony IMX678

[Jai Luthra]

Hi Conor,

Thank you for the review.

Quoting Conor Dooley (2026-05-20 17:56:29)
> On Wed, May 20, 2026 at 05:17:25PM +0200, Jai Luthra wrote:
> > Sony IMX678 is an 8.4 Megapixel (3856x2180) CMOS sensor, that can output
> > pixels over MIPI CSI-2 bus. Add bindings for it.
> > 
> > Signed-off-by: Jai Luthra <jai.luthra@xxxxxxxxxxxxxxxx>
> > ---
> > Changes in v3:
> > - Use `reset-gpios`, mentioning the sensor XCLR acts like RESETN, instead of `xclr-gpios`
> > Changes in v2:
> > - Add per-variant compatibles for mono and colour, alongside the
> >   generic fallback, so the variant can be declared without powering
> >   the sensor at probe.
> > - Rename reset GPIO to xclr as that's what it's called in the
> >   datasheet, and how it behaves
> > - Reference the generic video interface devices schema and switch to
> >   unevaluatedProperties.
> > - Drop "link-frequencies: true"
> > - Drop the T: entry for media.git from MAINTAINERS.
> > ---
> >  .../devicetree/bindings/media/i2c/sony,imx678.yaml | 129 +++++++++++++++++++++
> >  MAINTAINERS                                        |   6 +
> >  2 files changed, 135 insertions(+)
> > 
> > diff --git a/Documentation/devicetree/bindings/media/i2c/sony,imx678.yaml b/Documentation/devicetree/bindings/media/i2c/sony,imx678.yaml
> > new file mode 100644
> > index 000000000000..d85745ddbefd
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/media/i2c/sony,imx678.yaml
> > @@ -0,0 +1,129 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > +# Copyright (C) 2026 Ideas on Board Oy
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/media/i2c/sony,imx678.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Sony IMX678 Sensor
> > +
> > +maintainers:
> > +  - Jai Luthra <jai.luthra@xxxxxxxxxxxxxxxx>
> > +
> > +description:
> > +  Sony IMX678 diagonal 8.86 mm (Type 1/1.8) CMOS active pixel type solid-state
> > +  image sensor with a square pixel array and 8.40M (3856x2180) effective pixels.
> > +
> > +allOf:
> > +  - $ref: /schemas/media/video-interface-devices.yaml#
> > +
> > +properties:
> > +  compatible:
> > +    enum:
> > +      - sony,imx678
> > +      - sony,imx678-aamr
> > +      - sony,imx678-aaqr
> > +    description:
> > +      The IMX678 sensor exists in a colour variant (IMX678-AAQR) and a mono
> > +      variant (IMX678-AAMR). An internal register can also help detect this at
> > +      runtime.
> 
> I don't understand the compatibles here. If aaqr is tge colour variant,
> and aamr is mono, what does the suffix-less compatible represent?

Sorry, I had seen Laurent's comment on this area in v2 but forgot to update
it in this revision.

The suffix-less compatible is for the cases where a product comes in two
variants with the sensor being either mono or color. It allows sharing DT
blobs amongst the two variants, where the driver powers the sensor on and
reads the register to figure out if it is mono or color.

> 
> Your commit message says:
> > - Add per-variant compatibles for mono and colour, alongside the
> >   generic fallback, so the variant can be declared without powering
> >   the sensor at probe.
> But that's not what you have permitted in the binding, you've described
> 3 different variants and using the one with no suffix as a fallback will
> produce validation errors.
> 

"fallback" was a wrong choice of word, I'll update the description in v4.

> I think this probably is
> pw-bot: changes-requested
> 
> Thanks,
> Conor.

Thanks,
    Jai