Hi Krzysztof,
Thank you for reviewing the patch.
On 10/09/23 4:25 pm, Krzysztof Kozlowski wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>
> On 08/09/2023 16:29, Parthiban Veerasooran wrote:
>> Add device-tree support for Microchip's LAN865X MACPHY for configuring
>> the OPEN Alliance 10BASE-T1x MACPHY Serial Interface parameters.
>
> Please use subject prefixes matching the subsystem. You can get them for
> example with `git log --oneline -- DIRECTORY_OR_FILE` on the directory
> your patch is touching.
Ok sure, so it will become like,
dt-bindings: net: add device-tree support for Microchip's LAN865X MACPHY
I will correct it in the next revision.
>
>>
>> Signed-off-by: Parthiban Veerasooran <[email protected]>
>> ---
>> .../bindings/net/microchip,lan865x.yaml | 54 +++++++++++++++++++
>> MAINTAINERS | 1 +
>> 2 files changed, 55 insertions(+)
>> create mode 100644 Documentation/devicetree/bindings/net/microchip,lan865x.yaml
>>
>> diff --git a/Documentation/devicetree/bindings/net/microchip,lan865x.yaml b/Documentation/devicetree/bindings/net/microchip,lan865x.yaml
>> new file mode 100644
>> index 000000000000..3465b2c97690
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/net/microchip,lan865x.yaml
>> @@ -0,0 +1,54 @@
>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>> +%YAML 1.2
>> +---
>> +$id: http://devicetree.org/schemas/net/microchip,lan865x.yaml#
>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: Microchip LAN8650/1 10BASE-T1S MACPHY Ethernet Controllers
>> +
>> +maintainers:
>> + - Parthiban Veerasooran <[email protected]>
>> +
>> +description: |
>> + Device tree properties for LAN8650/1 10BASE-T1S MACPHY Ethernet
>
> Drop "Device tree properties for" and instead describe the hardware.
sure, will do it.
>
>> + controller.
>> +
>> +allOf:
>> + - $ref: ethernet-controller.yaml#
>> +
>> +properties:
>> + compatible:
>> + items:
>
> No need for items. Just enum.
Ok noted.
>
>
>> + - enum:
>> + - microchip,lan865x
>
> No wildcards in compatibles.
Yes then we don't need enum also isn't it?
>
> Missing blank line.
Ok will add it.
>
>
>
>> + reg:
>> + maxItems: 1
>> +
>> + local-mac-address: true
>> + oa-chunk-size: true
>> + oa-tx-cut-through: true
>> + oa-rx-cut-through: true
>> + oa-protected: true
>
> What are all these? Where are they defined that you skip description,
> type and vendor prefix?
Ok missed it. Will do it in the next revision.
>
>> +
>> +required:
>> + - compatible
>> + - reg
>> +
>> +additionalProperties: false
>> +
>> +examples:
>> + - |
>> + spi {
>> + #address-cells = <1>;
>> + #size-cells = <0>;
>> +
>> + ethernet@1{
>
> Missing space
Ok will add it.
>
>> + compatible = "microchip,lan865x";
>> + reg = <1>; /* CE0 */
>
> CE0? chip-select? What does this comment mean in this context?
Yes it is chip-select. Will add proper comment.
Best Regards,
Parthiban V
>
>> + local-mac-address = [04 05 06 01 02 03];
>> + oa-chunk-size = <64>;
>> + oa-tx-cut-through;
>> + oa-rx-cut-through;
>> + oa-protected;
>
>
>
> Best regards,
> Krzysztof
>
On 12/09/2023 14:15, [email protected] wrote:
> Hi Krzysztof,
>
> Thank you for reviewing the patch.
>
> On 10/09/23 4:25 pm, Krzysztof Kozlowski wrote:
>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>>
>> On 08/09/2023 16:29, Parthiban Veerasooran wrote:
>>> Add device-tree support for Microchip's LAN865X MACPHY for configuring
>>> the OPEN Alliance 10BASE-T1x MACPHY Serial Interface parameters.
>>
>> Please use subject prefixes matching the subsystem. You can get them for
>> example with `git log --oneline -- DIRECTORY_OR_FILE` on the directory
>> your patch is touching.
> Ok sure, so it will become like,
>
> dt-bindings: net: add device-tree support for Microchip's LAN865X MACPHY
>
> I will correct it in the next revision.
"device-tree support for " is redundant, drop
>>
>>>
>>> Signed-off-by: Parthiban Veerasooran <[email protected]>
>>> ---
>>> .../bindings/net/microchip,lan865x.yaml | 54 +++++++++++++++++++
>>> MAINTAINERS | 1 +
>>> 2 files changed, 55 insertions(+)
>>> create mode 100644 Documentation/devicetree/bindings/net/microchip,lan865x.yaml
>>>
>>> diff --git a/Documentation/devicetree/bindings/net/microchip,lan865x.yaml b/Documentation/devicetree/bindings/net/microchip,lan865x.yaml
>>> new file mode 100644
>>> index 000000000000..3465b2c97690
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/net/microchip,lan865x.yaml
>>> @@ -0,0 +1,54 @@
>>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>>> +%YAML 1.2
>>> +---
>>> +$id: http://devicetree.org/schemas/net/microchip,lan865x.yaml#
>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>>> +
>>> +title: Microchip LAN8650/1 10BASE-T1S MACPHY Ethernet Controllers
>>> +
>>> +maintainers:
>>> + - Parthiban Veerasooran <[email protected]>
>>> +
>>> +description: |
>>> + Device tree properties for LAN8650/1 10BASE-T1S MACPHY Ethernet
>>
>> Drop "Device tree properties for" and instead describe the hardware.
> sure, will do it.
>>
>>> + controller.
>>> +
>>> +allOf:
>>> + - $ref: ethernet-controller.yaml#
>>> +
>>> +properties:
>>> + compatible:
>>> + items:
>>
>> No need for items. Just enum.
> Ok noted.
>>
>>
>>> + - enum:
>>> + - microchip,lan865x
>>
>> No wildcards in compatibles.
> Yes then we don't need enum also isn't it?
I don't see correlation between these two. Please read the writing
bindings guidelines.
>>
>> Missing blank line.
> Ok will add it.
>>
>>
>>
>>> + reg:
>>> + maxItems: 1
>>> +
>>> + local-mac-address: true
>>> + oa-chunk-size: true
>>> + oa-tx-cut-through: true
>>> + oa-rx-cut-through: true
>>> + oa-protected: true
>>
>> What are all these? Where are they defined that you skip description,
>> type and vendor prefix?
> Ok missed it. Will do it in the next revision.
No, drop them or explain why they are hardware properties.
>>
>>> +
>>> +required:
>>> + - compatible
>>> + - reg
>>> +
>>> +additionalProperties: false
>>> +
>>> +examples:
>>> + - |
>>> + spi {
>>> + #address-cells = <1>;
>>> + #size-cells = <0>;
>>> +
>>> + ethernet@1{
>>
>> Missing space
> Ok will add it.
>>
>>> + compatible = "microchip,lan865x";
>>> + reg = <1>; /* CE0 */
>>
>> CE0? chip-select? What does this comment mean in this context?
> Yes it is chip-select. Will add proper comment.
Why? isn't reg obvious?
Best regards,
Krzysztof
Hi Krzysztof,
On 12/09/23 6:47 pm, Krzysztof Kozlowski wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>
> On 12/09/2023 14:15, [email protected] wrote:
>> Hi Krzysztof,
>>
>> Thank you for reviewing the patch.
>>
>> On 10/09/23 4:25 pm, Krzysztof Kozlowski wrote:
>>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>>>
>>> On 08/09/2023 16:29, Parthiban Veerasooran wrote:
>>>> Add device-tree support for Microchip's LAN865X MACPHY for configuring
>>>> the OPEN Alliance 10BASE-T1x MACPHY Serial Interface parameters.
>>>
>>> Please use subject prefixes matching the subsystem. You can get them for
>>> example with `git log --oneline -- DIRECTORY_OR_FILE` on the directory
>>> your patch is touching.
>> Ok sure, so it will become like,
>>
>> dt-bindings: net: add device-tree support for Microchip's LAN865X MACPHY
>>
>> I will correct it in the next revision.
>
> "device-tree support for " is redundant, drop
Ah ok will do that.
>
>>>
>>>>
>>>> Signed-off-by: Parthiban Veerasooran <[email protected]>
>>>> ---
>>>> .../bindings/net/microchip,lan865x.yaml | 54 +++++++++++++++++++
>>>> MAINTAINERS | 1 +
>>>> 2 files changed, 55 insertions(+)
>>>> create mode 100644 Documentation/devicetree/bindings/net/microchip,lan865x.yaml
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/net/microchip,lan865x.yaml b/Documentation/devicetree/bindings/net/microchip,lan865x.yaml
>>>> new file mode 100644
>>>> index 000000000000..3465b2c97690
>>>> --- /dev/null
>>>> +++ b/Documentation/devicetree/bindings/net/microchip,lan865x.yaml
>>>> @@ -0,0 +1,54 @@
>>>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>>>> +%YAML 1.2
>>>> +---
>>>> +$id: http://devicetree.org/schemas/net/microchip,lan865x.yaml#
>>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>>>> +
>>>> +title: Microchip LAN8650/1 10BASE-T1S MACPHY Ethernet Controllers
>>>> +
>>>> +maintainers:
>>>> + - Parthiban Veerasooran <[email protected]>
>>>> +
>>>> +description: |
>>>> + Device tree properties for LAN8650/1 10BASE-T1S MACPHY Ethernet
>>>
>>> Drop "Device tree properties for" and instead describe the hardware.
>> sure, will do it.
>>>
>>>> + controller.
>>>> +
>>>> +allOf:
>>>> + - $ref: ethernet-controller.yaml#
>>>> +
>>>> +properties:
>>>> + compatible:
>>>> + items:
>>>
>>> No need for items. Just enum.
>> Ok noted.
>>>
>>>
>>>> + - enum:
>>>> + - microchip,lan865x
>>>
>>> No wildcards in compatibles.
>> Yes then we don't need enum also isn't it?
>
> I don't see correlation between these two. Please read the writing
> bindings guidelines.
Ok, will check it out.
>
>
>>>
>>> Missing blank line.
>> Ok will add it.
>>>
>>>
>>>
>>>> + reg:
>>>> + maxItems: 1
>>>> +
>>>> + local-mac-address: true
>>>> + oa-chunk-size: true
>>>> + oa-tx-cut-through: true
>>>> + oa-rx-cut-through: true
>>>> + oa-protected: true
>>>
>>> What are all these? Where are they defined that you skip description,
>>> type and vendor prefix?
>> Ok missed it. Will do it in the next revision.
>
> No, drop them or explain why they are hardware properties.
Will separate hardware specific and OA specific properties.
>
>>>
>>>> +
>>>> +required:
>>>> + - compatible
>>>> + - reg
>>>> +
>>>> +additionalProperties: false
>>>> +
>>>> +examples:
>>>> + - |
>>>> + spi {
>>>> + #address-cells = <1>;
>>>> + #size-cells = <0>;
>>>> +
>>>> + ethernet@1{
>>>
>>> Missing space
>> Ok will add it.
>>>
>>>> + compatible = "microchip,lan865x";
>>>> + reg = <1>; /* CE0 */
>>>
>>> CE0? chip-select? What does this comment mean in this context?
>> Yes it is chip-select. Will add proper comment.
>
> Why? isn't reg obvious?
Sorry, yes it is reg. The comment is wrong. Will remove it.
Best Regards,
Parthiban V
>
> Best regards,
> Krzysztof
>
>