Re: [PATCH v8 05/17] dt-bindings: media: Add bindings for ARM mali-c55

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Krzysztof,

On Wed, Nov 06, 2024 at 01:15:23PM +0100, Krzysztof Kozlowski wrote:
> On Wed, Nov 06, 2024 at 10:05:22AM +0000, Daniel Scally wrote:
> > Add the yaml binding for ARM's Mali-C55 Image Signal Processor.
> > 
> > Acked-by: Nayden Kanchev <nayden.kanchev@xxxxxxx>
> > Co-developed-by: Jacopo Mondi <jacopo.mondi@xxxxxxxxxxxxxxxx>
> > Signed-off-by: Jacopo Mondi <jacopo.mondi@xxxxxxxxxxxxxxxx>
> > Signed-off-by: Daniel Scally <dan.scally@xxxxxxxxxxxxxxxx>
> > ---
> > Changes in v8:
> > 
> > 	- Added the video clock back in. Now that we have actual hardware it's
> > 	  clear that it's necessary.
> > 	- Added reset lines 
> > 	- Dropped R-bs
> 
> These are trivial, so I wish you kept the review... but since you ask,
> then comment further
> 
> I recommend using b4, so your cover letter changelog comes with nice
> links to previous versions. I scrolled through entire cover letter for
> this (for me that's almost the only point of cover letter) and could
> not find them. Anyway, just a remark.
> 
> 
> ...
> 
> > +  resets:
> > +    items:
> > +      - description: vclk domain reset
> > +      - description: aclk domain reset
> > +      - description: hclk domain reset
> > +
> > +  reset-names:
> > +    items:
> > +      - const: vresetn
> 
> drop "reset", it's redundant and rather come here with logical name. I
> wonder what "n" means as well. It's not a GPIO to be "inverted"...

The aresetn and hresetn names come directly from a hardware manual
(vresetn seems to be called rstn in that document though). As far as I
understand, they are the names of the external signals of the IP core.
I tend to pick the hardware names for clock and reset names. That makes
it easier for integrators, and from a driver point of view it doesn't
change much as DT names are just a convention anyway.

That being said, if there's a good reason to do otherwise (such as
standardizing property names to make handling through common code
possible), that's fine too.

> I wonder about reset domains for clocks as well... is this just gate
> clock misrepresented?

No, those are real reset signals. There can be clock gates external to
the IP, and those are handled by the clock providers. The IP has three
domains of logical that are synchronous to three different clocks, and
they have one reset signal each, hence the description mentioning "clock
domain".

-- 
Regards,

Laurent Pinchart




[Index of Archives]     [Device Tree Compilter]     [Device Tree Spec]     [Linux Driver Backports]     [Video for Linux]     [Linux USB Devel]     [Linux PCI Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Yosemite Backpacking]


  Powered by Linux