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

[Conor Dooley]

On Thu, May 21, 2026 at 09:30:56AM +0200, Jai Luthra wrote:
> Quoting Conor Dooley (2026-05-21 00:04:16)
> > On Wed, May 20, 2026 at 07:19:36PM +0200, Jai Luthra wrote:
> > > 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".

Ah, I definitely misunderstood what you meant by "product". It was not
clear to me that "product" in this context did not mean the camera that
we were discussing a binding for.

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

I get too much email to read every single thread. If something was
reviewed by Krzysztof or Rob I don't open it, unless something piques my
interest. I think I average 700 binding related emails per day with all
the new sashiko-bot mails included, I wouldn't be able to have a life if
I read them all! If I review a patch where I didn't review earlier
revisions do I look at the binding patch on lore, but I don't generally
care about the driver patches. Laurent's proposal was a reply to something
long deleted from my mailbox, so I highly doubt I opened it.

I personally don't expect contributors to have any wider awareness, but
every maintainer differs in their expectations of contributors, across
all subsystems.

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

To be the devil's advocate, nothing stops someone from shipping a
devicetree for a "product" like this with the specific compatibles
omitted!
That said, if someone is building two different products, with two
different SKUs and two different BoMs, I don't really see why they can't
ship different images (or the same image, with a way for the bootloader
to figure out which dtb to load), so this can be achieved without having
to permit a binding like this.

> > 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??
> 
> What we do in the linux driver today should be irrelevant to it.

The reason I brought up the points about the driver is that I thought
that either you had forgotten to implement the things you were using as
justification, or you were inventing justifications on the fly to avoid
making changes. To me, what the driver does (or doesn't do) is very
relevant when you're trying to justify something we would ordinarily
not permit.
Remember, having distinct multiple compatibles for the same hardware so
that you can induce different software behaviour is not something we
typically permit. While we do want to support all use cases, it isn't
always the case that devicetree is the right place to do it.

Additionally, it saves me having to send another email in response to
the driver!

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

This is a valid justification for keeping the specific compatibles.
Although this is effectively the same as "avoid powering on the device
to check", providing reasons why this has value makes it a much stronger
argument.

Hope that helps,
Conor.

Attachment: signature.asc
Description: PGP signature