Re: [PATCH v4 10/16] media: dt-bindings: sony,imx678: Add other Starvis 2 family members

[Dave Stevenson]

Hi Sakari

Thanks for the review

On Tue, 20 Oct 2026 at 11:07, Sakari Ailus <sakari.ailus@xxxxxxxxxxxxxxx> wrote:
>
> Hi Dave,
>
> On Thu, Oct 15, 2026 at 04:57:36PM +0100, Dave Stevenson wrote:
> > There are multiple members of the Sony Starvis 2 image sensor family
> > that have the same hardware requirements as imx678.
> > Add them as options to the imx678 binding, rather than duplicating it.
> >
> > Signed-off-by: Dave Stevenson <dave.stevenson@xxxxxxxxxxxxxxx>
> > Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@xxxxxxxxxxxxxxxx>
> > ---
> >  .../devicetree/bindings/media/i2c/sony,imx678.yaml | 38 ++++++++++++++++++----
> >  1 file changed, 32 insertions(+), 6 deletions(-)
> >
> > diff --git a/Documentation/devicetree/bindings/media/i2c/sony,imx678.yaml b/Documentation/devicetree/bindings/media/i2c/sony,imx678.yaml
> > index 098f72544216..027872a134f2 100644
> > --- a/Documentation/devicetree/bindings/media/i2c/sony,imx678.yaml
> > +++ b/Documentation/devicetree/bindings/media/i2c/sony,imx678.yaml
> > @@ -5,7 +5,7 @@
> >  $id: http://devicetree.org/schemas/media/i2c/sony,imx678.yaml#
> >  $schema: http://devicetree.org/meta-schemas/core.yaml#
> >
> > -title: Sony IMX678 Sensor
> > +title: Sony IMX678 and Starvis 2 Sensors
> >
> >  maintainers:
> >    - Jai Luthra <jai.luthra@xxxxxxxxxxxxxxxx>
> > @@ -13,21 +13,47 @@ maintainers:
> >  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.
>
> You could rewrap this while at it.
>
> > +  It is part of the Starvis 2 family of sensors.
> > +  Other members of the Starvis 2 have the same hardware requirements, and
> > +  therefore share this binding.
>
> This paragraph could be rewrapped, too -- the limit is 80 characters per
> line.

OK, I'll rephrase it so that it makes sense as one reflowed paragraph.

> >
> >  allOf:
> >    - $ref: /schemas/media/video-interface-devices.yaml#
> >
> >  properties:
> >    compatible:
> > -    items:
> > -      - enum:
> > -          - sony,imx678-aamr
> > -          - sony,imx678-aaqr
> > -      - const: sony,imx678
> > +    oneOf:
> > +      - items:
> > +          - enum:
> > +              - sony,imx585-aamj
> > +              - sony,imx585-aaqj
> > +          - const: sony,imx585
> > +      - items:
> > +          - enum:
> > +              - sony,imx662-aamr
> > +              - sony,imx662-aaqr
> > +          - const: sony,imx662
> > +      - items:
> > +          - enum:
> > +              - sony,imx675-aamr
> > +              - sony,imx675-aaqr
> > +          - const: sony,imx675
> > +      - items:
> > +          - enum:
> > +              - sony,imx678-aamr
> > +              - sony,imx678-aaqr
> > +          - const: sony,imx678
> > +      - items:
> > +          - enum:
> > +              - sony,imx832-amr
> > +              - sony,imx832-aqr
> > +          - const: sony,imx832
>
> Should we also have a sony,starvis2 compatible? Just wondering.

I deliberately didn't based on the discussions Jai had with DT
maintainers when upstreaming the original binding [1], particularly
Conor's comment:
"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."

Sorry, but when time is limited, IMHO debating dtbindings just isn't
worth it. I've therefore taken the simple option.

I do have patches that implement that compatible and fully autodetect
the sensor[2], but I'll keep them downstream.
Doing so means that we have one common overlay for connecting any of
the supported Starvis2 sensors to a Pi.

[1] https://patchwork.linuxtv.org/project/linux-media/patch/20260520-imx678-v3-1-8b5f9676486e@xxxxxxxxxxxxxxxx/
[2] Currently the top 2 commits on
https://github.com/6by9/linux/tree/mainline_starvis2_v2, but that
branch will get rebased/reworked.

> The driver isn't generic still as it requires sensor specific information
> to be passed to it. CCS solves this with CCS static data (via firmware
> loading mechanism) so there are no driver changes even if the capability
> and limit information can't be obtained from device's registers or the
> device requires MSRs.

I'm assuming you're referring to the patchset as a whole as I don't
believe I've used the term generic in this patch. I'm happy to swap to
"common Starvis 2 driver" where appropriate within the patchset
instead.

IMHO Pushing the config to firmware files doesn't help, particularly
as these sensors don't have a common ID register and so identification
isn't trivial. It also makes life more involved for a distribution
maintainer to ensure they're picking up the relevant files, and that's
assuming that someone is curating them.

My aim with the series was to reuse as much common code as possible,
rather than submitting 5 separate drivers which are identical other
than constants and arrays of registers. Jai now has IMX676 working
with this driver, so that would make it 6 variants. Refactoring to use
a firmware file adds complexity for minimal gain, and you'd have to
special case IMX678 so it worked without a firmware file to avoid
regressions.

I guess the common core could be refactored to be a library, so that
you could minimise module size by having the sensor specific config in
a module that depends on the library. You're talking of saving a few
kB uncompressed though, so that would be quite extreme. I'd be more
tempted to have individual Kconfig entries and use #if
IS_ENABLED(CONFIG_IMXxxx) to conditionally compile in the config to a
common module.

  Dave

> > +
> >      description:
> >        The IMX678 sensor exists in a colour variant (IMX678-AAQR) and a mono
> >        variant (IMX678-AAMR). The variant-specific compatible describes the
> >        exact sensor variant, with the generic IMX678 compatible as a fallback.
> > +      The same applies for the other sensors in the family.
> >
> >    reg:
> >      maxItems: 1
> >
>
> --
> Regards,
>
> Sakari Ailus