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 - thanks for reviewing

On 06/11/2024 14:29, Krzysztof Kozlowski wrote:
On 06/11/2024 14:57, Laurent Pinchart wrote:
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...

Fair enough, I just didn't want to be presumptuous


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.


Thanks for the recommendation - I'll take a look when I get time, but either way I can add a link to the previous versions next time.



...

+  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.
If these are from manual then it is fine, although sometimes the names
are really pointless in manual...

Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@xxxxxxxxxx>
Thanks

Best regards,
Krzysztof





[Index of Archives]     [Linux Input]     [Video for Linux]     [Gstreamer Embedded]     [Mplayer Users]     [Linux USB Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [Yosemite Backpacking]

  Powered by Linux