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

[Krzysztof Kozlowski <krzk@kernel.org>]

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.


>   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.

> +	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.

> +
> +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