Re: [EXT] Re: [PATCH v2 1/2] dt-bindings: i3c: Add NXP P3H2x4x i3c-hub support

[Krzysztof Kozlowski <krzk@kernel.org>]

On 12/05/2025 13:45, Aman Kumar Pandey wrote:
>>>
>>> Signed-off-by: Aman Kumar Pandey <aman.kumarpandey@nxp.com>
>>> Signed-off-by: Vikash Bansal <vikash.bansal@nxp.com>
>>> ---
>>> V1 -> V2: Cleaned up coding style and addressed review comments
>>> ---
>>>  .../bindings/i3c/p3h2840-i3c-hub.yaml         | 332 ++++++++++++++++++
>>
>> Not much improved. I have doubts that you really looked at other bindings.
>>
>> Filename matching compatible.
>>
>>
> 
> Thanks for reviewing the patch.
> There is no existing entry like this one. It is the first device tree entry for any i3c target device being added to Linux upstream. 
> For I3C target device, compatible is not required as per i3c.yml. I am adding compatible for backward compatibility with i2c binding ( i3c target device have backward compatibility). 
> Should I use compatible matching filename in this case ?

Do you have compatible here? Yes, you have, so I do not get what is
unclear in my comment. Rename the binding to match the compatible, which
you have here.


> 
>>>  MAINTAINERS                                   |   8 +
>>>  2 files changed, 340 insertions(+)
>  
>  
>>> +
>>> +properties:
>>> +  $nodename:
>>> +    pattern: "^hub@[@.*]+,[0-9a-f]+$"
>>
>> Drop
>>
> 
> Sorry, did not understand your point. Could you please give some clarification ?

Drop this nodename. Remove it.

> 
>>> +
> 
>>> +
>>> +  cp0-ldo-enable:
>>> +    type: boolean
>>> +    description:
>>> +      Enables the on-die LDO for controller Port 0.
>>> +
>>> +  cp1-ldo-enable:
>>> +    type: boolean
>>> +    description:
>>> +      Enables the on-die LDO for controller Port 1.
>>> +
>>> +  tp0145-ldo-enable:
>>> +    type: boolean
>>> +    description:
>>> +      Enables the on-die LDO for target ports 0/1/4/5.
>>> +
>>> +  tp2367-ldo-enable:
>>> +    type: boolean
>>> +    description:
>>> +      Enables the on-die LDO for target ports 2/3/6/7.
>>> +
>>
>> NAK for all above properties.
>>
> 
> These are not vender specific properties. all these properties are as per i3c hub specification.
>  What should I change for above properties ?


What do they represent? Regulators? Why would you add custom properties
for controlling regulators? Why would you enable LDOs for ports which
have nothing connected? Why would you not enable LDOs for ports which
have something there? Why these are not properties of the ports?

So many questions.


Best regards,
Krzysztof

-- 
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c