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

["Niklas Söderlund" <niklas.soderlund+renesas@ragnatech.se>]

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

-- 
Kind Regards,
Niklas Söderlund