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

[Jai Luthra]

Quoting Conor Dooley (2026-05-21 00:04:16)
> 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@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.
> 
> 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's very myopic to say bindings should only care about the camera sensor
on its own and not how or where it is integrated in, which is usually a
"product".

Maybe my response won't sound "weird" (?) if you see Dave's comment on v1:
https://lore.kernel.org/all/CAPY8ntBi88-dd2HxxftErf8h5-ERRPcGy5KJ-+oF7jawNOJpuA@xxxxxxxxxxxxxx/
and Laurent's recent proposal(s) for handling this for a different sensor:
https://lore.kernel.org/linux-media/20260505163713.GE1547435@xxxxxxxxxxxxxxxxxxxxxxxxxx/

Both of those threads have you in CC, so I assumed you were aware. That's a
higher bar for "being aware of something" than DT maintainers often have
for contributors, like Krzysztof's comment with unnecessarily snappy tone
about the reset-gpio on v2.

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

Yes, that's the other proposal. The downside being having a certainty of
the variant at integration time, and not allowing sharing binary blobs
as-is between two "products" that differ only in which of the two sensor
variants it ships with.

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

I thought the bindings are for the hardware and all its potential usecases,
and not tied to a specific driver or OS? Has that changed??

The specific compatibles are useful for the potential usecase of not waking
up the sensor to save boot-time or avoiding privacy LED flash at multiple
stages of the boot process.

What we do in the linux driver today should be irrelevant to it.

--
Jai

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