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@xxxxxxxxxxxx>
> > > > Reviewed-by: Geert Uytterhoeven <geert+renesas@xxxxxxxxx>
> > >
> > > 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:
signature.asc
Description: PGP signature
