Re: [PATCH net-next v3 12/12] dt-bindings: net: add Microchip's LAN865X 10BASE-T1S MACPHY

[<Parthiban.Veerasooran@microchip.com>]

Hi Conor,

On 21/03/24 9:04 pm, Conor Dooley wrote:
> On Thu, Mar 21, 2024 at 12:00:56PM +0000,Parthiban.Veerasooran@microchip.com  wrote:
>> Hi Krzysztof,
>>
>> On 21/03/24 2:10 pm, Krzysztof Kozlowski wrote:
>>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>>>
>>> On 21/03/2024 09:38,Parthiban.Veerasooran@microchip.com  wrote:
>>>> Hi Krzysztof,
>>>>
>>>> On 20/03/24 3:23 pm, Krzysztof Kozlowski wrote:
>>>>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>>>>>
>>>>> On 20/03/2024 09:40,Parthiban.Veerasooran@microchip.com  wrote:
>>>>>> Hi Conor & Andrew,
>>>>>>
>>>>>> Please find my reply below by consolidating other two emails comments
>>>>>> related to this.
>>>>>>
>>>>>> On 07/03/24 12:31 am, Conor Dooley wrote:
>>>>>>> On Wed, Mar 06, 2024 at 07:48:57PM +0100, Andrew Lunn wrote:
>>>>>>>>>> +description:
>>>>>>>>>> +  The LAN8650/1 combines a Media Access Controller (MAC) and an Ethernet
>>>>>>>>>> +  PHY to enable 10BASE‑T1S networks. The Ethernet Media Access Controller
>>>>>>>>>> +  (MAC) module implements a 10 Mbps half duplex Ethernet MAC, compatible
>>>>>>>>>> +  with the IEEE 802.3 standard and a 10BASE-T1S physical layer transceiver
>>>>>>>>>> +  integrated into the LAN8650/1. The communication between the Host and
>>>>>>>>>> +  the MAC-PHY is specified in the OPEN Alliance 10BASE-T1x MACPHY Serial
>>>>>>>>>> +  Interface (TC6).
>>>>>>>>>> +
>>>>>>>>>> +allOf:
>>>>>>>>>> +  - $ref: ethernet-controller.yaml#
>>>>>>>>>> +
>>>>>>>>>> +properties:
>>>>>>>>>> +  compatible:
>>>>>>>>>> +    oneOf:
>>>>>>>>>> +      - items:
>>>>>>>>>> +          - const: microchip,lan8650
>>>>>>>>>> +          - const: microchip,lan8651
>>>>>>>>> The order here is wrong, lan8561 needs to come before the fallback of
>>>>>>>>> lan8650.
>>>>>>>> I don't think it is a fallback. There are two devices, and hence two
>>>>>>>> different compatibles. So i suspect the -items: is wrong here?
>>>>>>> It'd just be a two entry enum then, but I did take a quick look at the
>>>>>>> driver earlier and saw:
>>>>>>> +static const struct of_device_id lan865x_dt_ids[] = {
>>>>>>> +    { .compatible = "microchip,lan8650" },
>>>>>>> +    { .compatible = "microchip,lan8651" },
>>>>>>> +    { /* Sentinel */ }
>>>>>>> +};
>>>>>>>
>>>>>>> That, along with no other of_device_is_compatible() type operations
>>>>>>> made me think that having a fallback actually was suitable.
>>>>>>>
>>>>>>> You cropped it out, but the patch had:
>>>>>>>> +  compatible:
>>>>>>>> +    oneOf:
>>>>>>>> +      - items:
>>>>>>>> +          - const: microchip,lan8650
>>>>>>>> +          - const: microchip,lan8651
>>>>>>>> +      - enum:
>>>>>>>> +          - microchip,lan8650
>>>>>>> So it doesn't appear to be an accidental items in place of an enum,
>>>>>>> since the other compatible is in another enum.
>>>>>> As per Andrew's comment in another email, both LAN8650 and LAN8651 are
>>>>>> two different variants but they both share almost all characteristics
>>>>>> except one thing that is LAN8651 has "Single 3.3V supply with integrated
>>>>>> 1.8V regulator" which doesn't have anything to do with driver. That's
>>>>> So why this is not reflected in your driver? Why didn't you address that
>>>>> part, but ignored?
>>>> No, it is not ignored. This difference is specific to hardware and there
>>>> is no configuration/setting to be done from driver.
>>>>>> why I have kept them as fallback as Conor said in this email. Hope you
>>>>>> all OK with this.
>>>>> Did you read the feedback? Your response is not solving here anything.
>>>>> How 8650 can be used twice? Please point me to DTS showing both usages.
>>>> May be I have a misunderstanding here. Let's clarify it.
>>>>
>>>> LAN8650 and LAN8651 both are two different variants but both implements
>>>> same functionality. The only difference is LAN8651 has "Single 3.3V
>>>> supply with integrated" where LAN8650 doesn't have this. This is
>>>> hardware specific difference and there is no configuration/setting to be
>>>> done in the driver specific to this difference in the LAN8651. So
>>>> basically the driver can support for both variants without any
>>>> additional settings.
>>>>
>>>> LAN8650:https://www.microchip.com/en-us/product/lan8650
>>>> LAN8651:https://www.microchip.com/en-us/product/lan8651
>>>>
>>>> The below link shows the difference between them,
>>>> https://www.microchip.com/en-us/product-comparison.lan8650.lan8651
>>>>
>>>> With the above details, I would change the microchip,lan865x.yaml with
>>>> the below details.
>>>>
>>>> compatible:
>>>>      enum:
>>>>        - microchip,lan8650
>>>>        - microchip,lan8651
>>>>
>>>> And in the lan865x.c, I would remove the below line because
>>>> .compatible = "microchip,lan8650" already supports for LAN8651 as well.
>>>>
>>>> .compatible = "microchip,lan8651"
>>>>
>>>> Let me know your opinion on this proposal? or do you have any
>>>> misunderstanding here?
>>> It's still wrong. Upstream your DTS and then test it. You will
>>> immediately see that it does not work. So first make it working, then
>>> send code to review.
>> Sorry for the inconvenience. I did the below changes in my
>> microchip,lan865x.yaml file and executed dt_binding_check. It
>> successfully created the microchip,lan865x.example.dts without any
>> errors. Herewith I have attached the updated microchip,lan865x.yaml file
>> and the generated microchip,lan865x.example.dts file for your reference.
>>
>> properties:
>>     compatible:
>>       oneOf:
>>         - items:
>>             - const: microchip,lan8651
>>             - const: microchip,lan8650
> No, this is not right either. You need to also allow the lan8650 on its
> own. All you had to do with the original items list was flip the order
> of the lan8650 and lan8651.
Ah ok, now I understand this. Then it is supposed to be like below,

properties: 

   compatible: 

     oneOf: 

       - const: microchip,lan8650 

       - items: 

           - const: microchip,lan8651 

           - const: microchip,lan8650

Executed dt_binding_check with the above update and it was successful. 
Hope this is OK?

Best regards,
Parthiban V