diff mbox series

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

Message ID 20240619153559.1647957-2-niklas.soderlund+renesas@ragnatech.se (mailing list archive)
State New
Headers show
Series rcar-vin: Add support for R-Car V4M | expand

Commit Message

Niklas Söderlund June 19, 2024, 3:35 p.m. UTC 
Document support for the VIN module in the Renesas V4M (r8a779h0) SoC.

Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
 Documentation/devicetree/bindings/media/renesas,vin.yaml | 1 +
 1 file changed, 1 insertion(+)

Comments

Conor Dooley June 19, 2024, 5:33 p.m. UTC | #1 
On Wed, Jun 19, 2024 at 05:35:58PM +0200, Niklas Söderlund wrote:
> Document support for the VIN module in the Renesas V4M (r8a779h0) SoC.
> 
> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>

Didn't we just have a conversation about this, yet nothing has changed?
NAK. Either you need a fallback or to explain why a fallback is not
suitable _in this patch_.

Thanks,
Conor.

> ---
>  Documentation/devicetree/bindings/media/renesas,vin.yaml | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/Documentation/devicetree/bindings/media/renesas,vin.yaml b/Documentation/devicetree/bindings/media/renesas,vin.yaml
> index 5539d0f8e74d..168cb02f8abe 100644
> --- a/Documentation/devicetree/bindings/media/renesas,vin.yaml
> +++ b/Documentation/devicetree/bindings/media/renesas,vin.yaml
> @@ -54,6 +54,7 @@ properties:
>                - renesas,vin-r8a77995 # R-Car D3
>                - renesas,vin-r8a779a0 # R-Car V3U
>                - renesas,vin-r8a779g0 # R-Car V4H
> +              - renesas,vin-r8a779h0 # R-Car V4M
>  
>    reg:
>      maxItems: 1
> -- 
> 2.45.2
>
Niklas Söderlund June 19, 2024, 6:56 p.m. UTC | #2 
Hi Conor,

On 2024-06-19 18:33:37 +0100, Conor Dooley wrote:
> On Wed, Jun 19, 2024 at 05:35:58PM +0200, Niklas Söderlund wrote:
> > Document support for the VIN module in the Renesas V4M (r8a779h0) SoC.
> > 
> > Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> > Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
> 
> Didn't we just have a conversation about this, yet nothing has changed?
> NAK. Either you need a fallback or to explain why a fallback is not
> suitable _in this patch_.

Sorry, I'm confused from the conclusion of our conversation in v2. I did 
add an explanation to why not fallback is used, but I added it to patch 
2/2 which adds the compatible to the driver.

It was my understanding that a SoC specific compatible was needed in 
either case so, at lest to me, made more sens to explain why in the 
driver patch the reason go into detail about the register differences 
between the two. Sorry if I misunderstood. I can add the same 
explanation to both patches, would this help explain why only a SoC 
specific value is added?

  The datasheet for the two SoCs have small nuances around the Pre-Clip
  registers ELPrC and EPPrC in three use-cases, interlaced images,
  embedded data and RAW8 input. On V4H the values written to the registers
  are based on odd numbers while on V4M they are even numbers, based on
  the input image size.

  No board that uses these SoCs which also have the external peripherals
  to test these nuances exists. Most likely this is an issue in the
  datasheet, but to make this easy to address in the future do not add a
  common Gen4 fallback compatible. Instead uses SoC specific compatibles
  for both SoCs. This is what was done for Gen3 SoCs, which also had
  similar nuances in the register documentation.

> 
> Thanks,
> Conor.
> 
> > ---
> >  Documentation/devicetree/bindings/media/renesas,vin.yaml | 1 +
> >  1 file changed, 1 insertion(+)
> > 
> > diff --git a/Documentation/devicetree/bindings/media/renesas,vin.yaml b/Documentation/devicetree/bindings/media/renesas,vin.yaml
> > index 5539d0f8e74d..168cb02f8abe 100644
> > --- a/Documentation/devicetree/bindings/media/renesas,vin.yaml
> > +++ b/Documentation/devicetree/bindings/media/renesas,vin.yaml
> > @@ -54,6 +54,7 @@ properties:
> >                - renesas,vin-r8a77995 # R-Car D3
> >                - renesas,vin-r8a779a0 # R-Car V3U
> >                - renesas,vin-r8a779g0 # R-Car V4H
> > +              - renesas,vin-r8a779h0 # R-Car V4M
> >  
> >    reg:
> >      maxItems: 1
> > -- 
> > 2.45.2
> >
Niklas Söderlund June 19, 2024, 8:43 p.m. UTC | #3 
Hello again.

On 2024-06-19 20:56:11 +0200, Niklas Söderlund wrote:
> Hi Conor,
> 
> On 2024-06-19 18:33:37 +0100, Conor Dooley wrote:
> > On Wed, Jun 19, 2024 at 05:35:58PM +0200, Niklas Söderlund wrote:
> > > Document support for the VIN module in the Renesas V4M (r8a779h0) SoC.
> > > 
> > > Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> > > Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
> > 
> > Didn't we just have a conversation about this, yet nothing has changed?
> > NAK. Either you need a fallback or to explain why a fallback is not
> > suitable _in this patch_.
> 
> Sorry, I'm confused from the conclusion of our conversation in v2. I did 
> add an explanation to why not fallback is used, but I added it to patch 
> 2/2 which adds the compatible to the driver.
> 
> It was my understanding that a SoC specific compatible was needed in 
> either case so, at lest to me, made more sens to explain why in the 
> driver patch the reason go into detail about the register differences 
> between the two. Sorry if I misunderstood. I can add the same 
> explanation to both patches, would this help explain why only a SoC 
> specific value is added?
> 
>   The datasheet for the two SoCs have small nuances around the Pre-Clip
>   registers ELPrC and EPPrC in three use-cases, interlaced images,
>   embedded data and RAW8 input. On V4H the values written to the registers
>   are based on odd numbers while on V4M they are even numbers, based on
>   the input image size.
> 
>   No board that uses these SoCs which also have the external peripherals
>   to test these nuances exists. Most likely this is an issue in the
>   datasheet, but to make this easy to address in the future do not add a
>   common Gen4 fallback compatible. Instead uses SoC specific compatibles
>   for both SoCs. This is what was done for Gen3 SoCs, which also had
>   similar nuances in the register documentation.

After have read thru v1 and v2 comments a few more times I think I might 
have spotted what I got wrong. If so I apologies for wasting your time 
reviewing this. I'm really trying to understand what I got wrong and 
address the review feedback.

Is what you are asking for with a fallback something like this?

--- a/Documentation/devicetree/bindings/media/renesas,vin.yaml
+++ b/Documentation/devicetree/bindings/media/renesas,vin.yaml
@@ -53,7 +53,11 @@ properties:
               - renesas,vin-r8a77990 # R-Car E3
               - renesas,vin-r8a77995 # R-Car D3
               - renesas,vin-r8a779a0 # R-Car V3U
+      - 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?

Apologies again for the confusion.

> 
> > 
> > Thanks,
> > Conor.
> > 
> > > ---
> > >  Documentation/devicetree/bindings/media/renesas,vin.yaml | 1 +
> > >  1 file changed, 1 insertion(+)
> > > 
> > > diff --git a/Documentation/devicetree/bindings/media/renesas,vin.yaml b/Documentation/devicetree/bindings/media/renesas,vin.yaml
> > > index 5539d0f8e74d..168cb02f8abe 100644
> > > --- a/Documentation/devicetree/bindings/media/renesas,vin.yaml
> > > +++ b/Documentation/devicetree/bindings/media/renesas,vin.yaml
> > > @@ -54,6 +54,7 @@ properties:
> > >                - renesas,vin-r8a77995 # R-Car D3
> > >                - renesas,vin-r8a779a0 # R-Car V3U
> > >                - renesas,vin-r8a779g0 # R-Car V4H
> > > +              - renesas,vin-r8a779h0 # R-Car V4M
> > >  
> > >    reg:
> > >      maxItems: 1
> > > -- 
> > > 2.45.2
> > > 
> 
> 
> 
> -- 
> Kind Regards,
> Niklas Söderlund
Conor Dooley June 20, 2024, 4:27 p.m. UTC | #4 
On Wed, Jun 19, 2024 at 10:43:21PM +0200, Niklas Söderlund wrote:
> Hello again.
> 
> On 2024-06-19 20:56:11 +0200, Niklas Söderlund wrote:
> > Hi Conor,
> > 
> > On 2024-06-19 18:33:37 +0100, Conor Dooley wrote:
> > > On Wed, Jun 19, 2024 at 05:35:58PM +0200, Niklas Söderlund wrote:
> > > > Document support for the VIN module in the Renesas V4M (r8a779h0) SoC.
> > > > 
> > > > Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> > > > Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
> > > 
> > > Didn't we just have a conversation about this, yet nothing has changed?
> > > NAK. Either you need a fallback or to explain why a fallback is not
> > > suitable _in this patch_.
> > 
> > Sorry, I'm confused from the conclusion of our conversation in v2. I did 
> > add an explanation to why not fallback is used, but I added it to patch 
> > 2/2 which adds the compatible to the driver.

If you're unsure at all just ask, better that than send a new version.

> > 
> > It was my understanding that a SoC specific compatible was needed in 
> > either case so, at lest to me, made more sens to explain why in the 
> > driver patch the reason go into detail about the register differences 
> > between the two. Sorry if I misunderstood. I can add the same 
> > explanation to both patches, would this help explain why only a SoC 
> > specific value is added?
> > 
> >   The datasheet for the two SoCs have small nuances around the Pre-Clip
> >   registers ELPrC and EPPrC in three use-cases, interlaced images,
> >   embedded data and RAW8 input. On V4H the values written to the registers
> >   are based on odd numbers while on V4M they are even numbers, based on
> >   the input image size.
> > 
> >   No board that uses these SoCs which also have the external peripherals
> >   to test these nuances exists. Most likely this is an issue in the
> >   datasheet, but to make this easy to address in the future do not add a
> >   common Gen4 fallback compatible. Instead uses SoC specific compatibles
> >   for both SoCs. This is what was done for Gen3 SoCs, which also had
> >   similar nuances in the register documentation.
> 
> After have read thru v1 and v2 comments a few more times I think I might 
> have spotted what I got wrong. If so I apologies for wasting your time 
> reviewing this. I'm really trying to understand what I got wrong and 
> address the review feedback.
> 
> Is what you are asking for with a fallback something like this?
> 
> --- a/Documentation/devicetree/bindings/media/renesas,vin.yaml
> +++ b/Documentation/devicetree/bindings/media/renesas,vin.yaml
> @@ -53,7 +53,11 @@ properties:
>                - renesas,vin-r8a77990 # R-Car E3
>                - renesas,vin-r8a77995 # R-Car D3
>                - renesas,vin-r8a779a0 # R-Car V3U
> +      - 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
 
   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

if there's another compatible device in the future.

> Apologies again for the confusion.

dw about it
Niklas Söderlund June 20, 2024, 5:22 p.m. UTC | #5 
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.

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

> 
> if there's another compatible device in the future.
> 
> > Apologies again for the confusion.
> 
> dw about it
Geert Uytterhoeven June 21, 2024, 7:21 a.m. UTC | #6 
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/

Gr{oetje,eeting}s,

                        Geert
Niklas Söderlund June 24, 2024, 9:20 a.m. UTC | #7 
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?
Conor Dooley June 24, 2024, 10:36 a.m. UTC | #8 
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.

Confused,
Conor.
Niklas Söderlund June 24, 2024, 12:50 p.m. UTC | #9 
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'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.
Conor Dooley June 24, 2024, 1:17 p.m. UTC | #10 
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.
diff mbox series

Patch

diff --git a/Documentation/devicetree/bindings/media/renesas,vin.yaml b/Documentation/devicetree/bindings/media/renesas,vin.yaml
index 5539d0f8e74d..168cb02f8abe 100644
--- a/Documentation/devicetree/bindings/media/renesas,vin.yaml
+++ b/Documentation/devicetree/bindings/media/renesas,vin.yaml
@@ -54,6 +54,7 @@  properties:
               - renesas,vin-r8a77995 # R-Car D3
               - renesas,vin-r8a779a0 # R-Car V3U
               - renesas,vin-r8a779g0 # R-Car V4H
+              - renesas,vin-r8a779h0 # R-Car V4M
 
   reg:
     maxItems: 1