Re: [PATCH] dt-bindings: add device tree bindings for mxl-8611x PHY

[Nate Drude <nate.d@variscite.com>]

Hi Krzysztof,

On 7/13/23 2:06 PM, Krzysztof Kozlowski wrote:
> On 13/07/2023 15:23, Nate Drude wrote:
>> The MXL8611X driver has custom bindings for configuring the LEDs
>> and RGMII internal delays. This patch adds the documentation and
>> defines necessary to configure it from the device tree.
>>
>> Signed-off-by: Nate Drude <nate.d@variscite.com>
>> ---
>>    .../net/phy/mxl-8611x.txt                     | 37 +++++++++++++++++++
>>    include/dt-bindings/net/mxl-8611x.h           | 27 ++++++++++++++
>>    2 files changed, 64 insertions(+)
>>    create mode 100644 doc/device-tree-bindings/net/phy/mxl-8611x.txt
> 
> You did not CC DT maintainers, so kind of funny way to ask them to
> review something.
> 
> In general I will not provide full review for projects other than Linux.
> Submit bindings to the Linux, following proper Linux kernel process, if
> you wish them to be fully reviewed.

I appreciate your review, and sorry for the CC mistake. I added the 
other maintainers.

> 
> 
>>    create mode 100644 include/dt-bindings/net/mxl-8611x.h
>>
>> diff --git a/doc/device-tree-bindings/net/phy/mxl-8611x.txt
>> b/doc/device-tree-bindings/net/phy/mxl-8611x.txt
>> new file mode 100644
>> index 00000000000..462fdf61666
>> --- /dev/null
>> +++ b/doc/device-tree-bindings/net/phy/mxl-8611x.txt
>> @@ -0,0 +1,37 @@
>> +* MaxLinear MXL8611x PHY Device Tree binding
>> +
>> +Required properties:
>> +- reg: PHY address
>> +
>> +Optional properties:
>> +- mxl-8611x,ledN_cfg: Register configuration for COM_EXT_LED0_CFG,
> 
> That's not correct vendor name. Neither property name - underscores are
> not allowed. The property itself does not describe any feature or
> hardware. We do not program registers in DT.

Thanks, I'll need to reconsider how to abstract this from register 
values. Forget about this for now, I will split the LED support into a 
separate effort.

> 
>> +	COM_EXT_LED1_CFG, and COM_EXT_LED2_CFG
>> +- mxl-8611x,rx-internal-delay-ps: RGMII RX Clock Delay used only when
>> PHY operates
>> +	in RGMII mode with internal delay (phy-mode is 'rgmii-id' or
>> +	'rgmii-rxid') in pico-seconds.
>> +- mxl-8611x,tx-internal-delay-ps-100m: RGMII TX Clock Delay used only
> 
> Use correct unit suffixes.
> 
>> when PHY operates
>> +	in 10/100M RGMII mode with internal delay (phy-mode is 'rgmii-id' or
>> +	'rgmii-txid') in pico-seconds.
>> +- mxl-8611x,tx-internal-delay-ps-1g: RGMII TX Clock Delay used only
>> when PHY operates
>> +	in 1G RGMII mode with internal delay (phy-mode is 'rgmii-id' or
>> +	'rgmii-txid') in pico-seconds.
> 
> Same problem.

Can you please provide feedback on these updated bindings?

- maxlinear,rx-internal-delay-ps: RGMII RX Clock Delay used only when 
PHY operates
	in RGMII mode with internal delay (phy-mode is 'rgmii-id' or
	'rgmii-rxid') in pico-seconds.
- maxlinear,tx-internal-delay-100m-ps: RGMII TX Clock Delay used only 
when PHY operates
	in 10/100M RGMII mode with internal delay (phy-mode is 'rgmii-id' or
	'rgmii-txid') in pico-seconds.
- maxlinear,tx-internal-delay-1g-ps: RGMII TX Clock Delay used only when 
PHY operates
	in 1G RGMII mode with internal delay (phy-mode is 'rgmii-id' or
	'rgmii-txid') in pico-seconds.

> 
>> +
>> +Example:
>> +
>> +	ethernet-phy@0 {
>> +		reg = <0>;
>> +
>> +		mxl-8611x,led0_cfg = <(
>> +			MXL8611X_LEDX_CFG_LINK_UP_RX_ACT_ON |
>> +			MXL8611X_LEDX_CFG_LINK_UP_TX_ACT_ON |
>> +			MXL8611X_LEDX_CFG_TRAFFIC_ACT_BLINK_IND
>> +		)>;
>> +		mxl-8611x,led1_cfg = <(
>> +			MXL8611X_LEDX_CFG_LINK_UP_10MB_ON |
>> +			MXL8611X_LEDX_CFG_LINK_UP_100MB_ON |
>> +			MXL8611X_LEDX_CFG_LINK_UP_1GB_ON
>> +		)>;
>> +		mxl-8611x,rx-internal-delay-ps = <0>;
>> +		mxl-8611x,tx-internal-delay-ps-100m = <2250>;
>> +		mxl-8611x,tx-internal-delay-ps-1g = <150>;
>> +	};
>> diff --git a/include/dt-bindings/net/mxl-8611x.h
>> b/include/dt-bindings/net/mxl-8611x.h
>> new file mode 100644
>> index 00000000000..cb0ec0f8bd0
>> --- /dev/null
>> +++ b/include/dt-bindings/net/mxl-8611x.h
>> @@ -0,0 +1,27 @@
>> +/* SPDX-License-Identifier: (GPL-2.0+ OR MIT) */
>> +/*
>> + * Device Tree constants for MaxLinear MXL8611x PHYs
>> + *
>> + * Copyright 2023 Variscite Ltd.
>> + * Copyright 2023 MaxLinear Inc.
>> + */
>> +
>> +#ifndef _DT_BINDINGS_MXL_8611X_H
>> +#define _DT_BINDINGS_MXL_8611X_H
>> +
>> +#define MXL8611X_LEDX_CFG_TRAFFIC_ACT_BLINK_IND		(1 << 13)
> 
> Register values are not bindings.
> 
> 
> Best regards,
> Krzysztof
> 

Once again, thanks for your review. Hopefully when the driver patches 
are sent for Linux this will have been helpful.

Sincerely,
Nate