| Message ID | 20240619153559.1647957-2-niklas.soderlund+renesas@ragnatech.se (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Kieran Bingham |
| Headers | show |
| Series | rcar-vin: Add support for R-Car V4M | expand |
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 >
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 > >
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
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
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
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
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?
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.
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.
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 --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