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