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

[Conor Dooley <conor@kernel.org>]

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

On Wed, May 20, 2026 at 07:19:36PM +0200, Jai Luthra wrote:
> 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@ideasonboard.com>
> > > ---
> > > 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@ideasonboard.com>
> > > +
> > > +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.

This response is very weird. It's worded in a really generic way that
barely seems to be a response to my mail. We aren't talking about
"a product" here, we are specifically talking about the imx678, and we
know it comes it these variants. There are no "cases" involved.

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

To be honest, I don't really get why the driver uses the specific
compatibles at all, if it can just determine if it is colour or mono at
runtime. Seems to me like this should be
compatible:
  items:
    - enum:
       - imx678-aamr
       - imx678-aaqr
    - const: imx678

Or just entirely drop the suffixed compatibles from the binding, since
you can detect mono v colour at runtime. The justification for the
aamr/aaqr compatibles seems to be that it avoids powering on the device
to check, but it looks like you unconditionally power it on and check
which variant it is, so that argument holds no water.

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

Why would you want to share the dtb anyway? That makes no sense to
me as a usecase in the first place. If the sensor isn't part of the
board, you should be using an overlay or something similar to apply it,
because if you can swap the sensor you can also have no sensor!

Also doesn't your driver print a warning if you did this anyway?

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

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