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

[Sakari Ailus <sakari.ailus@linux.intel.com>]

Hi Dave,

On Tue, Oct 20, 2026 at 03:39:52PM +0100, Dave Stevenson wrote:
> Hi Sakari
> 
> Thanks for the review
> 
> On Tue, 20 Oct 2026 at 11:07, Sakari Ailus <sakari.ailus@linux.intel.com> 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@raspberrypi.com>
> > > Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@oss.qualcomm.com>
> > > ---
> > >  .../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@ideasonboard.com>
> > > @@ -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.

The DT bindings are important to get right from the beginning, that's why I
asked. But I see this has been considered so I'm fine with it.

> 
> 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@ideasonboard.com/
> [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.

Fair points. CCS was made to scale and obviously the number of Starvis 2
(or 3) sensors will be somewhat limited in any case. The identification
information could also be put to DT, that's something we should do for CCS
as some devices are simply missing the information.

While we can proceed with this approach I guess this is something to
consider in the future.

-- 
Regards,

Sakari Ailus