Re: [PATCH RFC v3 1/5] dt-bindings: phy: hisi-inno-usb2: convert to YAML

[Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>]

On 20/02/2024 12:41, Krzysztof Kozlowski wrote:
> On 20/02/2024 11:40, Yang Xiwen wrote:
>> On 2/20/2024 4:16 PM, Krzysztof Kozlowski wrote:
>>> On 19/02/2024 22:49, Yang Xiwen wrote:
>>>> On 2/20/2024 5:37 AM, Krzysztof Kozlowski wrote:
>>>>> On 19/02/2024 22:35, Yang Xiwen wrote:
>>>>>> On 2/20/2024 5:32 AM, Krzysztof Kozlowski wrote:
>>>>>>> On 19/02/2024 22:27, Yang Xiwen via B4 Relay wrote:
>>>>>>>> From: Yang Xiwen <forbidden405@outlook.com>
>>>>>>>>
>>>>>>>> Add missing compatible "hisilicon,hi3798mv100-usb2-phy" to compatible
>>>>>>>> list due to prior driver change.
>>>>>>>>
>>>>>>>> Also rename to hisilicon,inno-usb2-phy.yaml and add this name to
>>>>>>>> compatible lists.
>>>>>>>>
>>>>>>>> Fixes: 3940ffc65492 ("phy: hisilicon: Add inno-usb2-phy driver for Hi3798MV100")
>>>>>>>> Signed-off-by: Yang Xiwen <forbidden405@outlook.com>
>>>>>>>> ---
>>>>>>>>     .../bindings/phy/hisilicon,inno-usb2-phy.yaml      | 95 ++++++++++++++++++++++
>>>>>>>>     .../devicetree/bindings/phy/phy-hisi-inno-usb2.txt | 71 ----------------
>>>>>>>>     2 files changed, 95 insertions(+), 71 deletions(-)
>>>>>>>>
>>>>>>>> diff --git a/Documentation/devicetree/bindings/phy/hisilicon,inno-usb2-phy.yaml b/Documentation/devicetree/bindings/phy/hisilicon,inno-usb2-phy.yaml
>>>>>>>> new file mode 100644
>>>>>>>> index 000000000000..1b57e0396209
>>>>>>>> --- /dev/null
>>>>>>>> +++ b/Documentation/devicetree/bindings/phy/hisilicon,inno-usb2-phy.yaml
>>>>>>>> @@ -0,0 +1,95 @@
>>>>>>>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>>>>>>>> +%YAML 1.2
>>>>>>>> +---
>>>>>>>> +$id: http://devicetree.org/schemas/phy/hisilicon,inno-usb2-phy.yaml#
>>>>>>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>>>>>>>> +
>>>>>>>> +title: HiSilicon HiSTB SoCs INNO USB2 PHY device
>>>>>>>> +
>>>>>>>> +maintainers:
>>>>>>>> +  - Yang Xiwen <forbidden405@outlook.com>
>>>>>>>> +
>>>>>>>> +properties:
>>>>>>>> +  compatible:
>>>>>>>> +    items:
>>>>>>>> +      - enum:
>>>>>>>> +          - hisilicon,hi3798cv200-usb2-phy
>>>>>>>> +          - hisilicon,hi3798mv100-usb2-phy
>>>>>>>> +      - const: hisilicon,inno-usb2-phy
>>>>>>> According to your driver hisilicon,hi3798mv100-usb2-phy and
>>>>>>> hisilicon,inno-usb2-phy are not compatible.
>>>>>> Ah, i didn't pay too much attention to that. I should remove the entry
>>>>>> for hisilicon,inno-usb2-phy in the driver. Sorry for that.
>>>>> We don't talk here about driver, although I used the driver as proof or
>>>>> argument, because I don't have access to hardware datasheet (and no
>>>>> intention to look there).
>>>>>
>>>>> What I claim is these are not compatible, so respond to this argument,
>>>>> not some other one.
>>>> Why not? Of course they are compatible. All 3 SoCs are using
>>> Why? Because...
>>>
>>>> inno-usb2-phy. The only difference here is the method to access the
>>> ... here! Different programming interface means not compatible.
>>>
>>> Please provide instead any argument that they are compatible, in the
>>> meaning of Devicetree of course. You are claiming inno-usb2-phy  can be
>>> used for hi3798mv100 and it will work fine?
>>>
>>>> registers. They are all enabled by `writing BIT(2) to address 0x6`. In
>>>> the cover letter, I said the driver is actually doing things wrong.
>>> Cover letter does not matter, I don't even read them. Your commits matter.
>>>
>>>> Especially the commit adding PHY_TYPE enums, the name is confusing and
>>>> conveys the wrong info. It's not PHY which are not compatible, it's the
>>>> bus. I'll fix the driver, but still the PHY hardwares are compatible
>>>> between these 3 SoCs.
>>> Provide any argument.
>>
>> Just take a look at the driver. hisi_inno_phy_write_reg() is the 
>> function that differs between different models. But for all of them, 
>> hisi_inno_phy_setup() is the same.
>>
>>
>> hisi_inno_phy_write_reg() should be moved to a separate bus driver. It's 
>> bus-related, not phy. PHY driver should not care how to access the bus, 
> 
> So drivers are compatible or hardware? We talk about hardware, not
> drivers...
> 
>> but the bus driver should. The PHY driver only needs to use regmap_* 
>> APIs to "write BIT(2) to addr 6".
> 
> Different programming interface, so not compatible.

Although maybe I jumped to conclusions too fast. Do you claim that all
registers are the same? All the values, offsets, fields and masks?

Best regards,
Krzysztof


-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy