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

[Dan Scally]

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 at arm.com>
>>>> Co-developed-by: Jacopo Mondi <jacopo.mondi at ideasonboard.com>
>>>> Signed-off-by: Jacopo Mondi <jacopo.mondi at ideasonboard.com>
>>>> Signed-off-by: Daniel Scally <dan.scally at ideasonboard.com>
>>>> ---
>>>> 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 at linaro.org>
Thanks
>
> Best regards,
> Krzysztof
>