On Tue, May 30, 2023 at 06:48:44PM +0100, Dave Stevenson wrote:
> Thanks for the incredibly speedy review.

Just happened to change mailboxes right as it arrived ;)

> On Tue, 30 May 2023 at 18:39, Conor Dooley <conor@kernel.org> wrote:
> >
> > Hey Dave,
> >
> > On Tue, May 30, 2023 at 06:29:58PM +0100, Dave Stevenson wrote:
> > > There are a number of variants of the imx258 modules that can not
> > > be differentiated at runtime, so add compatible strings for them.
> > >
> > > Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
> > > ---
> > >  .../devicetree/bindings/media/i2c/sony,imx258.yaml         | 7 ++++++-
> > >  1 file changed, 6 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/Documentation/devicetree/bindings/media/i2c/sony,imx258.yaml b/Documentation/devicetree/bindings/media/i2c/sony,imx258.yaml
> > > index bee61a443b23..3415b26b5991 100644
> > > --- a/Documentation/devicetree/bindings/media/i2c/sony,imx258.yaml
> > > +++ b/Documentation/devicetree/bindings/media/i2c/sony,imx258.yaml
> > > @@ -14,10 +14,15 @@ description: |-
> > >    type stacked image sensor with a square pixel array of size 4208 x 3120. It
> > >    is programmable through I2C interface.  Image data is sent through MIPI
> > >    CSI-2.
> > > +  There are a number of variants of the sensor which cannot be detected at
> > > +  runtime, so multiple compatible strings are required to differentiate these.
> >
> > This is implied by having several compatibles.
> 
> I'm happy to drop it, just that I've seen a number of media bindings
> that had debate on why extra compatible strings were required.

If there were no non-detectable differences, then there should be
a fallback compatible i.e.
compatible = "sony,imx666-foo", "sony,imx666";
Maybe Laurent will come in here and scream at me, but I don't think the
pattern should be propagated.

> > >  properties:
> > >    compatible:
> > > -    const: sony,imx258
> > > +    oneOf:
> > > +      - enum:
> > > +          - sony,imx258
> > > +          - sony,imx258-pdaf
> >
> > Why not just
> > properties:
> >   compatible:
> >     enum:
> > ?
> > I don't see other patches anding more complex compatibles (or they've
> > not arrived yet) so it doesn't appear to be avoiding churn.
> 
> I'll freely admit that DT bindings are a black art to me, so I was
> following sony,imx290.yaml [1].
> properties:
>   compatible:
>     oneOf:
>       - enum:
>           - sony,imx290lqr # Colour
>           - sony,imx290llr # Monochrome
>           - sony,imx327lqr # Colour
>       - const: sony,imx290
>         deprecated: true
> 
> Looking again at that case, I assume the oneOf is selecting between
> the enum and the const?

Bingo!

> Seeing as we don't have the const, I guess we
> can drop the "oneOf:"

Cheers,
Conor.