Re: [PATCH 4/5] ARM: dts: microchip: Rename the usb node

[Krzysztof Kozlowski <krzk@kernel.org>]

On 05/09/2024 15:51, Andrei.Simion@microchip.com wrote:
> Hi,
> 
> On 14.08.2024 16:22, Krzysztof Kozlowski wrote:
>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>>
>> On 14/08/2024 14:26, Andrei Simion wrote:
>>> Rename the usb node according to devicetree
>>> specification and update the label according
>>> with the node-specific standard as: ohci, ehci
>>> or gadget.
>>
>> Please wrap commit message according to Linux coding style / submission
>> process (neither too early nor over the limit):
>> https://elixir.bootlin.com/linux/v6.4-rc1/source/Documentation/process/submitting-patches.rst#L597
>>
>> ...
>>
> 
> In V2 I will wrap the commit message according to Linux coding style / submission process
> 
>>
>>>
>>> diff --git a/arch/arm/boot/dts/microchip/aks-cdu.dts b/arch/arm/boot/dts/microchip/aks-cdu.dts
>>> index 742fcf525e1b..52e166c8a365 100644
>>> --- a/arch/arm/boot/dts/microchip/aks-cdu.dts
>>> +++ b/arch/arm/boot/dts/microchip/aks-cdu.dts
>>> @@ -50,13 +50,13 @@ macb0: ethernet@fffc4000 {
>>>                               status = "okay";
>>>                       };
>>>
>>> -                     usb1: gadget@fffa4000 {
>>> +                     gadget: usb@fffa4000 {
>>>                               atmel,vbus-gpio = <&pioC 15 GPIO_ACTIVE_HIGH>;
>>>                               status = "okay";
>>>                       };
>>>               };
>>>
>>> -             usb0: ohci@500000 {
>>> +             ohci: usb@500000 {
>>
>> I don't think that these label renames are correct.
>>
> 
> I checked in other dts dtsi (other silicon vendors) the USB node 
> and it uses ohci, ehci, gadget, etc as a label.  Also, I verified the
> address in the datasheet and they are correctly labeled (meant that 
> they were: usb gadget, usb ohci, usb ehci)
> 
> From what point of view is it not correct?
> I would like to know so I can do it right

Because:
1. You drop existing information about interface number. That's USB0.
2. You did not provide answer why.


> 
> 
>>>                       num-ports = <2>;
>>>                       status = "okay";
>>>               };
>>> diff --git a/arch/arm/boot/dts/microchip/animeo_ip.dts b/arch/arm/boot/dts/microchip/animeo_ip.dts
>>> index 29936bfbeeb7..911c8d9ee013 100644
>>> --- a/arch/arm/boot/dts/microchip/animeo_ip.dts
>>> +++ b/arch/arm/boot/dts/microchip/animeo_ip.dts
>>> @@ -136,7 +136,7 @@ ubi@60000 {
>>>                       };
>>>               };
>>>
>>> -             usb0: ohci@500000 {
>>> +             ohci: usb@500000 {
>>>                       num-ports = <2>;
>>>                       atmel,vbus-gpio = <&pioB 15 GPIO_ACTIVE_LOW>;
>>>                       status = "okay";
>>> diff --git a/arch/arm/boot/dts/microchip/at91-ariag25.dts b/arch/arm/boot/dts/microchip/at91-ariag25.dts
>>> index 713d18f80356..fedcf30a924e 100644
>>> --- a/arch/arm/boot/dts/microchip/at91-ariag25.dts
>>> +++ b/arch/arm/boot/dts/microchip/at91-ariag25.dts
>>> @@ -173,11 +173,11 @@ &usart3 {
>>>       status = "okay";
>>>  };
>>>
>>> -&usb0 {
>>> +&ohci {
>>>       status = "okay";
>>>       num-ports = <3>;
>>>  };
>>>
>>> -&usb1 {
>>> +&ehci {
>>>       status = "okay";
>>>  };
>>
>> And how now the sorting works? I don't get the point of it. What is
>> exactly wrong in the label that justifies the code reshuffling.
>>
> 
> The point is to be easy to determine what kind of standard 
> represents/implements the usb node.

For one, number of interface is important, e.g. because it matches
schematics or datasheet. For other, maybe the standard. You did not
provide any rationale for this.

Plus you ignored comment about sorting.


Best regards,
Krzysztof