Re: [PATCH v3 1/2] dt-bindings: media: renesas,vin: Add binding for V4M

[Conor Dooley <conor.dooley@microchip.com>]

[-- Attachment #1: Type: text/plain, Size: 5175 bytes --]

On Mon, Jun 24, 2024 at 02:50:51PM +0200, Niklas Söderlund wrote:
> On 2024-06-24 11:36:40 +0100, Conor Dooley wrote:
> > On Mon, Jun 24, 2024 at 11:20:29AM +0200, Niklas Söderlund wrote:
> > > Hi Conor,
> > > 
> > > On 2024-06-21 09:21:24 +0200, Geert Uytterhoeven wrote:
> > > > Hi Niklas,
> > > > 
> > > > On Thu, Jun 20, 2024 at 7:22 PM Niklas Söderlund
> > > > <niklas.soderlund+renesas@ragnatech.se> wrote:
> > > > > On 2024-06-20 17:27:00 +0100, Conor Dooley wrote:
> > > > > > > +      - items:
> > > > > > > +          - enum:
> > > > > > >                - renesas,vin-r8a779g0 # R-Car V4H
> > > > > > > +              - renesas,vin-r8a779h0 # R-Car V4M
> > > > > > > +          - const: renesas,rcar-gen4-vin # Generic R-Car Gen4
> > > > > > >
> > > > > > > If so I can see that working as I could still fix any issues that come
> > > > > > > from differences between V4H and V4M if needed. If so do you think it
> > > > > > > best to add this in two different patches? One to add the
> > > > > > > renesas,rcar-gen4-vin fallback (which will also need DTS updates to fix
> > > > > > > warnings from exciting users of V4H not listing the gen4 fallback) and
> > > > > > > one to add V4M?
> > > > > >
> > > > > >
> > > > > > I would just do:
> > > > > > diff --git a/Documentation/devicetree/bindings/media/renesas,vin.yaml b/Documentation/devicetree/bindings/media/renesas,vin.yaml
> > > > > > index 5539d0f8e74d..22bbad42fc03 100644
> > > > > > --- a/Documentation/devicetree/bindings/media/renesas,vin.yaml
> > > > > > +++ b/Documentation/devicetree/bindings/media/renesas,vin.yaml
> > > > > > @@ -54,6 +54,9 @@ properties:
> > > > > >                - renesas,vin-r8a77995 # R-Car D3
> > > > > >                - renesas,vin-r8a779a0 # R-Car V3U
> > > > > >                - renesas,vin-r8a779g0 # R-Car V4H
> > > > > > +      - items:
> > > > > > +          - const: renesas,vin-r8a779h0 # R-Car V4L2
> > > > > > +          - const: renesas,vin-r8a779g0 # R-Car V4H
> > > > >
> > > > > @Geert: What do you think about this? This would be a first use-case for
> > > > > compatibles crossing SoC DTS files that I know of. I'm a bit uneasy
> > > > > going down this road.
> > > > 
> > > > Me too ;-)
> > > > 
> > > > > Would this not also effect the existing users of renesas,vin-r8a779g0
> > > > > which would now need something similar to what you propose below with a
> > > > > list of SoC compatibles and a fallback.
> > > > >
> > > > > >
> > > > > >    reg:
> > > > > >      maxItems: 1
> > > > > >
> > > > > > Which requires no driver or dts changes. That could become:
> > > > > >       - items:
> > > > > >           - enum:
> > > > > >               - renesas,vin-r8a779h0 # R-Car V4L2
> > > > > >               - renesas,vin-r8a779i0 # R-Car R4P17
> > > > > >           - const: renesas,vin-r8a779g0 # R-Car V4H
> > > > >
> > > > > FWIW, on Gen2 where fallback es where useful compared to Gen3 we did
> > > > > this with "renesas,rcar-gen2-vin".
> > > > 
> > > > We do know there are differences (albeit probably small) among the R-Car
> > > > Gen4 VIN implementations, so I am reluctant to add a family-specific
> > > > compatible value.  Typically we only use a family-specific compatible
> > > > value if the IP cores are known (or better, assumed ;-) to be identical.
> > > > 
> > > > And sometimes our assumptions turn out to be wrong...
> > > > See slides 25-33 (last two for the numbers) of my talk at ER2019
> > > > https://embedded-recipes.org/2019/talks/herd-your-socs-become-a-matchmaker/
> > > 
> > > Do Geert's slides help to explain the R-Car perspective on why a 
> > > family-specific fallback compatible might not be desirable, and why the 
> > > SoC specific one is proposed? 
> > 
> > IIRC, it was you that wanted to use a "family-specific" fallback, I
> > don't understand what you want from me. If you look back at even the
> > context in this email, you can see you suggesting one and my counter
> > point.
> 
> Sorry that I'm spreading my confusion around and taking up your time.

I don't care if non-native speakers of English say confusing things,
don't worry about that.

> I'm trying to understand if Geert's reply helped outline why a single 
> SoC specific compatible is being used here, if so I was hoping a revised 
> commit message would make this solution acceptable.  
> 
> If not I will try to summaries the issue and the different proposals so 
> we can find a design that works and address some of the confusion before 
> sending a new version.

These devices look, for all intents and purposes, to be compatible. If
they're not, *say* what is not compatible about them. Don't just say
"ohh there might be, but they're small", say exactly what - because your
driver makes them look compatible. It looks compatible with the a0 as
well... The vibe that comes across here is of being "afraid" of having
fallback compatibles that reference other SoCs - which is totally normal
for other vendors, not that there are any differences in programming
model between the VIN instances on these devices.

Thanks,
Conor.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]