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

[Conor Dooley <conor@kernel.org>]

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

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

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