On Mon, May 19, 2025 at 03:57:52PM GMT, Daniel Scally wrote: > The RZ/V2H SoC has a block called the Input Video Control block which > feeds image data into the Image Signal Processor. Add dt bindings to > describe the IVC. A nit, subject: drop second/last, redundant "bindings for". The "dt-bindings" prefix is already stating that these are bindings. See also: https://elixir.bootlin.com/linux/v6.7-rc8/source/Documentation/devicetree/bindings/submitting-patches.rst#L18 > > Signed-off-by: Daniel Scally <dan.scally@xxxxxxxxxxxxxxxx> > --- > .../bindings/media/renesas,rzv2h-ivc.yaml | 99 +++++++++++++++++++ This fails testing - expect Rob's bot report (or check DT patchwork) - thus limited review. > 1 file changed, 99 insertions(+) > create mode 100644 Documentation/devicetree/bindings/media/renesas,rzv2h-ivc.yaml Filename matching compatible. You can also explain exception in the commit msg. > > diff --git a/Documentation/devicetree/bindings/media/renesas,rzv2h-ivc.yaml b/Documentation/devicetree/bindings/media/renesas,rzv2h-ivc.yaml > new file mode 100644 > index 000000000000..29d522f7d365 > ... > + - compatible > + - reg > + - interrupts > + - clocks > + - clock-names > + - resets > + - reset-names > + - port > + > +additionalProperties: false > + > +examples: > + - | > + #include <dt-bindings/clock/renesas,r9a09g057-cpg.h> > + #include <dt-bindings/interrupt-controller/arm-gic.h> > + > + ivc: ivc@16040000 { Node names should be generic. See also an explanation and list of examples (not exhaustive) in DT specification: https://devicetree-specification.readthedocs.io/en/latest/chapter2-devicetree-basics.html#generic-names-recommendation video-codec? video-encoder? video-engine? Or anything reasonable you find. Drop unused label > + compatible = "renesas,r9a09g057-ivc"; > + reg = <0x16040000 0x230>; > + > + clocks = <&cpg CPG_MOD R9A09G057_ISP0_PCLK>, > + <&cpg CPG_MOD R9A09G057_ISP0_VIN_ACLK>, > + <&cpg CPG_MOD R9A09G057_ISP0_SCLK>; Misaligned, should be aligned to < (see DTS coding style). > + clock-names = "pclk", "vin_aclk", "sclk"; > + > + resets = <&cpg R9A09G057_ISP_0_PRESETN>, > + <&cpg R9A09G057_ISP_0_VIN_ARESETN>, Same here > + <&cpg R9A09G057_ISP_0_ISP_SRESETN>; > + reset-names = "presetn", "vin_aresetn", "sresetn"; > + > + interrupts = <GIC_SPI 861 IRQ_TYPE_EDGE_RISING>; > + > + status = "okay"; Drop These are nits, so normally would qualify for review, but maybe I missed something since it fails testing. Best regards, Krzysztof
