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

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

On Wed, Feb 14, 2024 at 05:37:17PM +0000, Conor Dooley wrote:
> On Wed, Feb 14, 2024 at 04:28:25PM +0200, Laurent Pinchart wrote:
> > 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?

I don't expect there would be a clock controller to directly reference
in most cases. It depends a bit on where the clock domain crossing
between the source (often a CSI-2 receiver) and the ISP is. If it's
implemented in glue logic bundled with the ISP, which wouldn't be
described in DT as a separate node, then there's a higher chance we'll
have a clock controller for vclk. If it's implemented in the source,
vclk will just come from the source, which won't be listed as a clock
controller.

One option would be to make this clock optional, by moving it to the end
of the clocks list, and setting

	minItems: 2
	maxItems: 3

> > > +      - 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 :)

As far as I understand, the names proposed by Dan come directly from the
IP core documentation.

-- 
Regards,

Laurent Pinchart

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