Re: [PATCH v2 2/5] dt-bindings: media: Add bindings for ARM mali-c55

From: Conor Dooley <conor@kernel.org>
To: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Cc: Daniel Scally <dan.scally@ideasonboard.com>,
	linux-media@vger.kernel.org, devicetree@vger.kernel.org,
	linux-arm-kernel@lists.infradead.org,
	jacopo.mondi@ideasonboard.com, nayden.kanchev@arm.com,
	robh+dt@kernel.org, mchehab@kernel.org,
	krzysztof.kozlowski+dt@linaro.org, conor+dt@kernel.org,
	jerome.forissier@linaro.org, kieran.bingham@ideasonboard.com
Subject: Re: [PATCH v2 2/5] dt-bindings: media: Add bindings for ARM mali-c55
Date: Wed, 14 Feb 2024 17:37:17 +0000	[thread overview]
Message-ID: <20240214-velcro-pushy-0cbd18b23361@spud> (raw)
In-Reply-To: <20240214142825.GA7873@pendragon.ideasonboard.com>

[-- Attachment #1.1: Type: text/plain, Size: 2615 bytes --]

On Wed, Feb 14, 2024 at 04:28:25PM +0200, Laurent Pinchart wrote:
> Hi Dan,
> 
> Thank you for the patch.
> 
> On Wed, Feb 14, 2024 at 02:19:03PM +0000, Daniel Scally wrote:
> > Add the yaml binding for ARM's Mali-C55 Image Signal Processor.
> > 
> > Acked-by: Nayden Kanchev <nayden.kanchev@arm.com>
> > Signed-off-by: Daniel Scally <dan.scally@ideasonboard.com>
> > ---
> > Changes in v2:
> > 
> > 	- Added clocks information
> > 	- Fixed the warnings raised by Rob
> > 
> >  .../bindings/media/arm,mali-c55.yaml          | 77 +++++++++++++++++++
> >  1 file changed, 77 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/media/arm,mali-c55.yaml
> > 
> > diff --git a/Documentation/devicetree/bindings/media/arm,mali-c55.yaml b/Documentation/devicetree/bindings/media/arm,mali-c55.yaml
> > new file mode 100644
> > index 000000000000..30038cfec3a4
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/media/arm,mali-c55.yaml
> > @@ -0,0 +1,77 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/media/arm,mali-c55.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: ARM Mali-C55 Image Signal Processor
> > +
> > +maintainers:
> > +  - Daniel Scally <dan.scally@ideasonboard.com>
> > +  - Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> > +
> > +properties:
> > +  compatible:
> > +    const: arm,mali-c55
> > +
> > +  reg:
> > +    maxItems: 1
> > +
> > +  interrupts:
> > +    maxItems: 1
> > +
> > +  clocks:
> > +    items:
> > +      - description: ISP video clock
> 
> I wonder if we need this clock. Granted, it's an input clock to the ISP,
> but it's part of the input video bus. I don't expect anyone would ever
> need to control it manually, it should be provided by the video source
> automatically.

I'd say that if there's a clock controller providing this clock, even if
it is implicit in the video feed it's good to have here. Being able to
increment the refcount on that clock would be good, even if you don't
actually control it manually?

> 
> > +      - description: ISP AXI clock
> > +      - description: ISP AHB-lite clock
> 
> These two other clocks look good to me.
> 
> > +
> > +  clock-names:
> > +    items:
> > +      - const: vclk
> > +      - const: aclk
> > +      - const: hclk

Why not "video" "axi" "ahb-lite"? There's 3 useful letters between the
tree clock names you've provided - they're all clocks, so having "clk"
in them is just noise :)

Cheers,
Conor.

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

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel