[PATCH net-next v3 0/3] net: phy: dp83822: Add support for changing the transmit amplitude voltage

[PATCH net-next v3 0/3] net: phy: dp83822: Add support for changing the transmit amplitude voltage

[Dimitri Fedrau via B4 Relay]

Add support for changing the transmit amplitude voltage in 100BASE-TX mode.
Add support for configuration via DT.

Signed-off-by: Dimitri Fedrau <dimitri.fedrau@liebherr.com>
---
Changes in v3:
- Switch to tx-amplitude-100base-tx-percent in bindings
- Link to v2: https://lore.kernel.org/r/20250120-dp83822-tx-swing-v2-0-07c99dc42627@liebherr.com

Changes in v2:
- Remove binding ti,tx-amplitude-100base-tx-millivolt from ti,dp83822.yaml
- Add binding tx-amplitude-100base-tx-gain-milli to ethernet-phy.yaml
- Add helper to get tx amplitude gain from DT
- Link to v1: https://lore.kernel.org/r/20250113-dp83822-tx-swing-v1-0-7ed5a9d80010@liebherr.com

---
Dimitri Fedrau (3):
      dt-bindings: net: ethernet-phy: add property tx-amplitude-100base-tx-percent
      net: phy: Add helper for getting tx amplitude gain
      net: phy: dp83822: Add support for changing the transmit amplitude voltage

 .../devicetree/bindings/net/ethernet-phy.yaml      |  7 ++++
 drivers/net/phy/dp83822.c                          | 37 ++++++++++++++++++++++
 drivers/net/phy/phy_device.c                       | 20 +++++++++---
 include/linux/phy.h                                |  3 ++
 4 files changed, 63 insertions(+), 4 deletions(-)
---
base-commit: c2933b2befe25309f4c5cfbea0ca80909735fd76
change-id: 20241213-dp83822-tx-swing-5ba6c1e9b065

Best regards,
-- 
Dimitri Fedrau <dimitri.fedrau@liebherr.com>

[PATCH net-next v3 1/3] dt-bindings: net: ethernet-phy: add property tx-amplitude-100base-tx-percent

[Dimitri Fedrau via B4 Relay]

From: Dimitri Fedrau <dimitri.fedrau@liebherr.com>

Add property tx-amplitude-100base-tx-percent in the device tree bindings
for configuring the tx amplitude of 100BASE-TX PHYs. Modifying it can be
necessary to compensate losses on the PCB and connector, so the voltages
measured on the RJ45 pins are conforming.

Signed-off-by: Dimitri Fedrau <dimitri.fedrau@liebherr.com>
---
 Documentation/devicetree/bindings/net/ethernet-phy.yaml | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/Documentation/devicetree/bindings/net/ethernet-phy.yaml b/Documentation/devicetree/bindings/net/ethernet-phy.yaml
index 2c71454ae8e362e7032e44712949e12da6826070..04f42961035f273990fdf4368ad1352397fc3774 100644
--- a/Documentation/devicetree/bindings/net/ethernet-phy.yaml
+++ b/Documentation/devicetree/bindings/net/ethernet-phy.yaml
@@ -232,6 +232,13 @@ properties:
       PHY's that have configurable TX internal delays. If this property is
       present then the PHY applies the TX delay.
 
+  tx-amplitude-100base-tx-percent:
+    description: |
+      Transmit amplitude gain applied for 100BASE-TX. When omitted, the PHYs
+      default will be left as is.
+    $ref: /schemas/types.yaml#/definitions/uint32
+    default: 100
+
   leds:
     type: object
 

-- 
2.39.5

[PATCH net-next v3 2/3] net: phy: Add helper for getting tx amplitude gain

[Dimitri Fedrau via B4 Relay]

From: Dimitri Fedrau <dimitri.fedrau@liebherr.com>

Add helper which returns the tx amplitude gain defined in device tree.
Modifying it can be necessary to compensate losses on the PCB and
connector, so the voltages measured on the RJ45 pins are conforming.

Signed-off-by: Dimitri Fedrau <dimitri.fedrau@liebherr.com>
---
 drivers/net/phy/phy_device.c | 20 ++++++++++++++++----
 include/linux/phy.h          |  3 +++
 2 files changed, 19 insertions(+), 4 deletions(-)

diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index 46713d27412b76077d2e51e29b8d84f4f8f0a86d..c42e3e22eba6f7508cefa3568ccb4629cedce6b2 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -3096,7 +3096,7 @@ void phy_get_pause(struct phy_device *phydev, bool *tx_pause, bool *rx_pause)
 EXPORT_SYMBOL(phy_get_pause);
 
 #if IS_ENABLED(CONFIG_OF_MDIO)
-static int phy_get_int_delay_property(struct device *dev, const char *name)
+static int phy_get_u32_property(struct device *dev, const char *name)
 {
 	s32 int_delay;
 	int ret;
@@ -3108,7 +3108,7 @@ static int phy_get_int_delay_property(struct device *dev, const char *name)
 	return int_delay;
 }
 #else
-static int phy_get_int_delay_property(struct device *dev, const char *name)
+static int phy_get_u32_property(struct device *dev, const char *name)
 {
 	return -EINVAL;
 }
@@ -3137,7 +3137,7 @@ s32 phy_get_internal_delay(struct phy_device *phydev, struct device *dev,
 	int i;
 
 	if (is_rx) {
-		delay = phy_get_int_delay_property(dev, "rx-internal-delay-ps");
+		delay = phy_get_u32_property(dev, "rx-internal-delay-ps");
 		if (delay < 0 && size == 0) {
 			if (phydev->interface == PHY_INTERFACE_MODE_RGMII_ID ||
 			    phydev->interface == PHY_INTERFACE_MODE_RGMII_RXID)
@@ -3147,7 +3147,7 @@ s32 phy_get_internal_delay(struct phy_device *phydev, struct device *dev,
 		}
 
 	} else {
-		delay = phy_get_int_delay_property(dev, "tx-internal-delay-ps");
+		delay = phy_get_u32_property(dev, "tx-internal-delay-ps");
 		if (delay < 0 && size == 0) {
 			if (phydev->interface == PHY_INTERFACE_MODE_RGMII_ID ||
 			    phydev->interface == PHY_INTERFACE_MODE_RGMII_TXID)
@@ -3193,6 +3193,18 @@ s32 phy_get_internal_delay(struct phy_device *phydev, struct device *dev,
 }
 EXPORT_SYMBOL(phy_get_internal_delay);
 
+s32 phy_get_tx_amplitude_gain(struct phy_device *phydev, struct device *dev,
+			      enum ethtool_link_mode_bit_indices linkmode)
+{
+	switch (linkmode) {
+	case ETHTOOL_LINK_MODE_100baseT_Full_BIT:
+		return phy_get_u32_property(dev, "tx-amplitude-100base-tx-percent");
+	default:
+		return -EINVAL;
+	}
+}
+EXPORT_SYMBOL(phy_get_tx_amplitude_gain);
+
 static int phy_led_set_brightness(struct led_classdev *led_cdev,
 				  enum led_brightness value)
 {
diff --git a/include/linux/phy.h b/include/linux/phy.h
index 19f076a71f9462cd37588a5da240a1d54df0fe0f..abdd768b7acece3db9ffb0f9de7d20cf86e72bc7 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -2114,6 +2114,9 @@ void phy_get_pause(struct phy_device *phydev, bool *tx_pause, bool *rx_pause);
 s32 phy_get_internal_delay(struct phy_device *phydev, struct device *dev,
 			   const int *delay_values, int size, bool is_rx);
 
+s32 phy_get_tx_amplitude_gain(struct phy_device *phydev, struct device *dev,
+			      enum ethtool_link_mode_bit_indices linkmode);
+
 void phy_resolve_pause(unsigned long *local_adv, unsigned long *partner_adv,
 		       bool *tx_pause, bool *rx_pause);
 

-- 
2.39.5

[PATCH net-next v3 3/3] net: phy: dp83822: Add support for changing the transmit amplitude voltage

[Dimitri Fedrau via B4 Relay]

From: Dimitri Fedrau <dimitri.fedrau@liebherr.com>

Add support for changing the transmit amplitude voltage in 100BASE-TX mode.
Modifying it can be necessary to compensate losses on the PCB and
connector, so the voltages measured on the RJ45 pins are conforming.

Signed-off-by: Dimitri Fedrau <dimitri.fedrau@liebherr.com>
---
 drivers/net/phy/dp83822.c | 37 +++++++++++++++++++++++++++++++++++++
 1 file changed, 37 insertions(+)

diff --git a/drivers/net/phy/dp83822.c b/drivers/net/phy/dp83822.c
index 6599feca1967d705331d6e354205a2485ea962f2..80b49e7744d6d4c9571606455fe50534d2dee977 100644
--- a/drivers/net/phy/dp83822.c
+++ b/drivers/net/phy/dp83822.c
@@ -31,6 +31,7 @@
 #define MII_DP83822_RCSR	0x17
 #define MII_DP83822_RESET_CTRL	0x1f
 #define MII_DP83822_MLEDCR	0x25
+#define MII_DP83822_LDCTRL	0x403
 #define MII_DP83822_LEDCFG1	0x460
 #define MII_DP83822_IOCTRL1	0x462
 #define MII_DP83822_IOCTRL2	0x463
@@ -123,6 +124,9 @@
 #define DP83822_IOCTRL1_GPIO1_CTRL		GENMASK(2, 0)
 #define DP83822_IOCTRL1_GPIO1_CTRL_LED_1	BIT(0)
 
+/* LDCTRL bits */
+#define DP83822_100BASE_TX_LINE_DRIVER_SWING	GENMASK(7, 4)
+
 /* IOCTRL2 bits */
 #define DP83822_IOCTRL2_GPIO2_CLK_SRC		GENMASK(6, 4)
 #define DP83822_IOCTRL2_GPIO2_CTRL		GENMASK(2, 0)
@@ -197,6 +201,7 @@ struct dp83822_private {
 	bool set_gpio2_clk_out;
 	u32 gpio2_clk_out;
 	bool led_pin_enable[DP83822_MAX_LED_PINS];
+	int tx_amplitude_100base_tx_index;
 };
 
 static int dp83822_config_wol(struct phy_device *phydev,
@@ -522,6 +527,12 @@ static int dp83822_config_init(struct phy_device *phydev)
 			       FIELD_PREP(DP83822_IOCTRL2_GPIO2_CLK_SRC,
 					  dp83822->gpio2_clk_out));
 
+	if (dp83822->tx_amplitude_100base_tx_index >= 0)
+		phy_modify_mmd(phydev, MDIO_MMD_VEND2, MII_DP83822_LDCTRL,
+			       DP83822_100BASE_TX_LINE_DRIVER_SWING,
+			       FIELD_PREP(DP83822_100BASE_TX_LINE_DRIVER_SWING,
+					  dp83822->tx_amplitude_100base_tx_index));
+
 	err = dp83822_config_init_leds(phydev);
 	if (err)
 		return err;
@@ -720,6 +731,11 @@ static int dp83822_phy_reset(struct phy_device *phydev)
 }
 
 #ifdef CONFIG_OF_MDIO
+static const u32 tx_amplitude_100base_tx_gain[] = {
+	80, 82, 83, 85, 87, 88, 90, 92,
+	93, 95, 97, 98, 100, 102, 103, 105,
+};
+
 static int dp83822_of_init_leds(struct phy_device *phydev)
 {
 	struct device_node *node = phydev->mdio.dev.of_node;
@@ -780,6 +796,8 @@ static int dp83822_of_init(struct phy_device *phydev)
 	struct dp83822_private *dp83822 = phydev->priv;
 	struct device *dev = &phydev->mdio.dev;
 	const char *of_val;
+	s32 val;
+	int i;
 
 	/* Signal detection for the PHY is only enabled if the FX_EN and the
 	 * SD_EN pins are strapped. Signal detection can only enabled if FX_EN
@@ -815,6 +833,25 @@ static int dp83822_of_init(struct phy_device *phydev)
 		dp83822->set_gpio2_clk_out = true;
 	}
 
+	dp83822->tx_amplitude_100base_tx_index = -1;
+	val = phy_get_tx_amplitude_gain(phydev, dev,
+					ETHTOOL_LINK_MODE_100baseT_Full_BIT);
+	if (val > 0) {
+		for (i = 0; i < ARRAY_SIZE(tx_amplitude_100base_tx_gain); i++) {
+			if (tx_amplitude_100base_tx_gain[i] == val) {
+				dp83822->tx_amplitude_100base_tx_index = i;
+				break;
+			}
+		}
+
+		if (dp83822->tx_amplitude_100base_tx_index < 0) {
+			phydev_err(phydev,
+				   "Invalid value for tx-amplitude-100base-tx-percent property (%u)\n",
+				   val);
+			return -EINVAL;
+		}
+	}
+
 	return dp83822_of_init_leds(phydev);
 }
 

-- 
2.39.5

Re: [PATCH net-next v3 1/3] dt-bindings: net: ethernet-phy: add property tx-amplitude-100base-tx-percent

[Rob Herring (Arm)]

On Tue, 04 Feb 2025 14:09:15 +0100, Dimitri Fedrau wrote:
> Add property tx-amplitude-100base-tx-percent in the device tree bindings
> for configuring the tx amplitude of 100BASE-TX PHYs. Modifying it can be
> necessary to compensate losses on the PCB and connector, so the voltages
> measured on the RJ45 pins are conforming.
> 
> Signed-off-by: Dimitri Fedrau <dimitri.fedrau@liebherr.com>
> ---
>  Documentation/devicetree/bindings/net/ethernet-phy.yaml | 7 +++++++
>  1 file changed, 7 insertions(+)
> 

My bot found errors running 'make dt_binding_check' on your patch:

yamllint warnings/errors:

dtschema/dtc warnings/errors:
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/net/ethernet-phy.yaml: properties:tx-amplitude-100base-tx-percent: '$ref' should not be valid under {'const': '$ref'}
	hint: Standard unit suffix properties don't need a type $ref
	from schema $id: http://devicetree.org/meta-schemas/core.yaml#

doc reference errors (make refcheckdocs):

See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20250204-dp83822-tx-swing-v3-1-9798e96500d9@liebherr.com

The base for the series is generally the latest rc1. A different dependency
should be noted in *this* patch.

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:

pip3 install dtschema --upgrade

Please check and re-submit after running the above command yourself. Note
that DT_SCHEMA_FILES can be set to your schema file to speed up checking
your schema. However, it must be unset to test all examples with your schema.

Re: [PATCH net-next v3 1/3] dt-bindings: net: ethernet-phy: add property tx-amplitude-100base-tx-percent

[Rob Herring]

On Tue, Feb 04, 2025 at 02:09:15PM +0100, Dimitri Fedrau wrote:
> Add property tx-amplitude-100base-tx-percent in the device tree bindings
> for configuring the tx amplitude of 100BASE-TX PHYs. Modifying it can be
> necessary to compensate losses on the PCB and connector, so the voltages
> measured on the RJ45 pins are conforming.
> 
> Signed-off-by: Dimitri Fedrau <dimitri.fedrau@liebherr.com>
> ---
>  Documentation/devicetree/bindings/net/ethernet-phy.yaml | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/net/ethernet-phy.yaml b/Documentation/devicetree/bindings/net/ethernet-phy.yaml
> index 2c71454ae8e362e7032e44712949e12da6826070..04f42961035f273990fdf4368ad1352397fc3774 100644
> --- a/Documentation/devicetree/bindings/net/ethernet-phy.yaml
> +++ b/Documentation/devicetree/bindings/net/ethernet-phy.yaml
> @@ -232,6 +232,13 @@ properties:
>        PHY's that have configurable TX internal delays. If this property is
>        present then the PHY applies the TX delay.
>  
> +  tx-amplitude-100base-tx-percent:
> +    description: |

Don't need '|' if no formatting to preserve.

> +      Transmit amplitude gain applied for 100BASE-TX. When omitted, the PHYs
> +      default will be left as is.
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    default: 100
> +
>    leds:
>      type: object
>  
> 
> -- 
> 2.39.5
> 

Re: [PATCH net-next v3 2/3] net: phy: Add helper for getting tx amplitude gain

[Russell King (Oracle)]

On Tue, Feb 04, 2025 at 02:09:16PM +0100, Dimitri Fedrau via B4 Relay wrote:
>  #if IS_ENABLED(CONFIG_OF_MDIO)
> -static int phy_get_int_delay_property(struct device *dev, const char *name)
> +static int phy_get_u32_property(struct device *dev, const char *name)
>  {
>  	s32 int_delay;
>  	int ret;
> @@ -3108,7 +3108,7 @@ static int phy_get_int_delay_property(struct device *dev, const char *name)
>  	return int_delay;

Hmm. You're changing the name of this function from "int" to "u32", yet
it still returns "int".

What range of values are you expecting to be returned by this function?
If it's the full range of u32 values, then that overlaps with the error
range returned by device_property_read_u32().

I'm wondering whether it would be better to follow the example set by
these device_* functions, and pass a pointer for the value to them, and
just have the return value indicating success/failure.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH net-next v3 1/3] dt-bindings: net: ethernet-phy: add property tx-amplitude-100base-tx-percent

[Dimitri Fedrau]

Am Tue, Feb 04, 2025 at 08:28:09AM -0600 schrieb Rob Herring (Arm):
> 
> On Tue, 04 Feb 2025 14:09:15 +0100, Dimitri Fedrau wrote:
> > Add property tx-amplitude-100base-tx-percent in the device tree bindings
> > for configuring the tx amplitude of 100BASE-TX PHYs. Modifying it can be
> > necessary to compensate losses on the PCB and connector, so the voltages
> > measured on the RJ45 pins are conforming.
> > 
> > Signed-off-by: Dimitri Fedrau <dimitri.fedrau@liebherr.com>
> > ---
> >  Documentation/devicetree/bindings/net/ethernet-phy.yaml | 7 +++++++
> >  1 file changed, 7 insertions(+)
> > 
> 
> My bot found errors running 'make dt_binding_check' on your patch:
> 
> yamllint warnings/errors:
> 
> dtschema/dtc warnings/errors:
> /builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/net/ethernet-phy.yaml: properties:tx-amplitude-100base-tx-percent: '$ref' should not be valid under {'const': '$ref'}
> 	hint: Standard unit suffix properties don't need a type $ref
> 	from schema $id: http://devicetree.org/meta-schemas/core.yaml#
> 
> doc reference errors (make refcheckdocs):
> 
> See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20250204-dp83822-tx-swing-v3-1-9798e96500d9@liebherr.com
> 
> The base for the series is generally the latest rc1. A different dependency
> should be noted in *this* patch.
> 
> If you already ran 'make dt_binding_check' and didn't see the above
> error(s), then make sure 'yamllint' is installed and dt-schema is up to
> date:
> 
> pip3 install dtschema --upgrade
> 
> Please check and re-submit after running the above command yourself. Note
> that DT_SCHEMA_FILES can be set to your schema file to speed up checking
> your schema. However, it must be unset to test all examples with your schema.
>
Thanks, missed to check. Will fix it.

Best regards,
Dimitri Fedrau

Re: [PATCH net-next v3 1/3] dt-bindings: net: ethernet-phy: add property tx-amplitude-100base-tx-percent

[Dimitri Fedrau]

Am Tue, Feb 04, 2025 at 09:34:09AM -0600 schrieb Rob Herring:
> On Tue, Feb 04, 2025 at 02:09:15PM +0100, Dimitri Fedrau wrote:
> > Add property tx-amplitude-100base-tx-percent in the device tree bindings
> > for configuring the tx amplitude of 100BASE-TX PHYs. Modifying it can be
> > necessary to compensate losses on the PCB and connector, so the voltages
> > measured on the RJ45 pins are conforming.
> > 
> > Signed-off-by: Dimitri Fedrau <dimitri.fedrau@liebherr.com>
> > ---
> >  Documentation/devicetree/bindings/net/ethernet-phy.yaml | 7 +++++++
> >  1 file changed, 7 insertions(+)
> > 
> > diff --git a/Documentation/devicetree/bindings/net/ethernet-phy.yaml b/Documentation/devicetree/bindings/net/ethernet-phy.yaml
> > index 2c71454ae8e362e7032e44712949e12da6826070..04f42961035f273990fdf4368ad1352397fc3774 100644
> > --- a/Documentation/devicetree/bindings/net/ethernet-phy.yaml
> > +++ b/Documentation/devicetree/bindings/net/ethernet-phy.yaml
> > @@ -232,6 +232,13 @@ properties:
> >        PHY's that have configurable TX internal delays. If this property is
> >        present then the PHY applies the TX delay.
> >  
> > +  tx-amplitude-100base-tx-percent:
> > +    description: |
> 
> Don't need '|' if no formatting to preserve.
>
Wasn't sure about the '|', because some properties use
it(rx-internal-delay-ps or tx-internal-delay-ps) others not. Is it wrong
there ? I don't see any reason to preserve formatting there. Anyway will remove
the '|' as you proposed. Thanks for explaining, didn't know.

Best regards,
Dimitri Fedrau

[...]

Re: [PATCH net-next v3 2/3] net: phy: Add helper for getting tx amplitude gain

[Dimitri Fedrau]

Am Tue, Feb 04, 2025 at 05:54:53PM +0000 schrieb Russell King (Oracle):
> On Tue, Feb 04, 2025 at 02:09:16PM +0100, Dimitri Fedrau via B4 Relay wrote:
> >  #if IS_ENABLED(CONFIG_OF_MDIO)
> > -static int phy_get_int_delay_property(struct device *dev, const char *name)
> > +static int phy_get_u32_property(struct device *dev, const char *name)
> >  {
> >  	s32 int_delay;
> >  	int ret;
> > @@ -3108,7 +3108,7 @@ static int phy_get_int_delay_property(struct device *dev, const char *name)
> >  	return int_delay;
> 
> Hmm. You're changing the name of this function from "int" to "u32", yet
> it still returns "int".
>

I just wanted to reuse code for retrieving the u32, I found
phy_get_int_delay_property and renamed it. But the renaming from "int"
to "u32" is wrong as you outlined.

> What range of values are you expecting to be returned by this function?
> If it's the full range of u32 values, then that overlaps with the error
> range returned by device_property_read_u32().
>

Values are in percent, u8 would already be enough, so it wouldn't
overlap with the error range.

> I'm wondering whether it would be better to follow the example set by
> these device_* functions, and pass a pointer for the value to them, and
> just have the return value indicating success/failure.
>

I would prefer this, but this would mean changes in phy_get_internal_delay
if we don't want to duplicate code, as phy_get_internal_delay relies on
phy_get_int_delay_property and we change function parameters of
phy_get_int_delay_property as you described. I would switch from
static int phy_get_int_delay_property(struct device *dev, const char *name)
to
static int phy_get_u32_property(struct device *dev, const char *name, u32 *val)

Do you agree ?

Best regards,
Dimitri Fedrau

Re: [PATCH net-next v3 2/3] net: phy: Add helper for getting tx amplitude gain

[Andrew Lunn]

On Wed, Feb 05, 2025 at 06:22:18AM +0100, Dimitri Fedrau wrote:
> Am Tue, Feb 04, 2025 at 05:54:53PM +0000 schrieb Russell King (Oracle):
> > On Tue, Feb 04, 2025 at 02:09:16PM +0100, Dimitri Fedrau via B4 Relay wrote:
> > >  #if IS_ENABLED(CONFIG_OF_MDIO)
> > > -static int phy_get_int_delay_property(struct device *dev, const char *name)
> > > +static int phy_get_u32_property(struct device *dev, const char *name)
> > >  {
> > >  	s32 int_delay;
> > >  	int ret;
> > > @@ -3108,7 +3108,7 @@ static int phy_get_int_delay_property(struct device *dev, const char *name)
> > >  	return int_delay;
> > 
> > Hmm. You're changing the name of this function from "int" to "u32", yet
> > it still returns "int".
> >
> 
> I just wanted to reuse code for retrieving the u32, I found
> phy_get_int_delay_property and renamed it. But the renaming from "int"
> to "u32" is wrong as you outlined.
> 
> > What range of values are you expecting to be returned by this function?
> > If it's the full range of u32 values, then that overlaps with the error
> > range returned by device_property_read_u32().
> >
> 
> Values are in percent, u8 would already be enough, so it wouldn't
> overlap with the error range.
> 
> > I'm wondering whether it would be better to follow the example set by
> > these device_* functions, and pass a pointer for the value to them, and
> > just have the return value indicating success/failure.
> >
> 
> I would prefer this, but this would mean changes in phy_get_internal_delay
> if we don't want to duplicate code, as phy_get_internal_delay relies on
> phy_get_int_delay_property and we change function parameters of
> phy_get_int_delay_property as you described. I would switch from
> static int phy_get_int_delay_property(struct device *dev, const char *name)
> to
> static int phy_get_u32_property(struct device *dev, const char *name, u32 *val)
> 
> Do you agree ?

This looks O.K. You should also rename the local variable int_delay.

Humm, that function has other issues.

static int phy_get_int_delay_property(struct device *dev, const char *name)
{
	s32 int_delay;
	int ret;

	ret = device_property_read_u32(dev, name, &int_delay);
	if (ret)
		return ret;

	return int_delay;
}

int_delay should really be a u32. if ret is not an error, there should
be a range check to ensure int_long actually fits in an s32, otherwise
-EINVAL, or maybe -ERANGE.

For delays, we never expect too much more than 2000ps, so no valid DT
blob should trigger issues here.

     Andrew

Re: [PATCH net-next v3 2/3] net: phy: Add helper for getting tx amplitude gain

[Dimitri Fedrau]

Am Wed, Feb 05, 2025 at 06:08:47PM +0100 schrieb Andrew Lunn:
> On Wed, Feb 05, 2025 at 06:22:18AM +0100, Dimitri Fedrau wrote:
> > Am Tue, Feb 04, 2025 at 05:54:53PM +0000 schrieb Russell King (Oracle):
> > > On Tue, Feb 04, 2025 at 02:09:16PM +0100, Dimitri Fedrau via B4 Relay wrote:
> > > >  #if IS_ENABLED(CONFIG_OF_MDIO)
> > > > -static int phy_get_int_delay_property(struct device *dev, const char *name)
> > > > +static int phy_get_u32_property(struct device *dev, const char *name)
> > > >  {
> > > >  	s32 int_delay;
> > > >  	int ret;
> > > > @@ -3108,7 +3108,7 @@ static int phy_get_int_delay_property(struct device *dev, const char *name)
> > > >  	return int_delay;
> > > 
> > > Hmm. You're changing the name of this function from "int" to "u32", yet
> > > it still returns "int".
> > >
> > 
> > I just wanted to reuse code for retrieving the u32, I found
> > phy_get_int_delay_property and renamed it. But the renaming from "int"
> > to "u32" is wrong as you outlined.
> > 
> > > What range of values are you expecting to be returned by this function?
> > > If it's the full range of u32 values, then that overlaps with the error
> > > range returned by device_property_read_u32().
> > >
> > 
> > Values are in percent, u8 would already be enough, so it wouldn't
> > overlap with the error range.
> > 
> > > I'm wondering whether it would be better to follow the example set by
> > > these device_* functions, and pass a pointer for the value to them, and
> > > just have the return value indicating success/failure.
> > >
> > 
> > I would prefer this, but this would mean changes in phy_get_internal_delay
> > if we don't want to duplicate code, as phy_get_internal_delay relies on
> > phy_get_int_delay_property and we change function parameters of
> > phy_get_int_delay_property as you described. I would switch from
> > static int phy_get_int_delay_property(struct device *dev, const char *name)
> > to
> > static int phy_get_u32_property(struct device *dev, const char *name, u32 *val)
> > 
> > Do you agree ?
> 
> This looks O.K. You should also rename the local variable int_delay.
> 
> Humm, that function has other issues.
> 
> static int phy_get_int_delay_property(struct device *dev, const char *name)
> {
> 	s32 int_delay;
> 	int ret;
> 
> 	ret = device_property_read_u32(dev, name, &int_delay);
> 	if (ret)
> 		return ret;
> 
> 	return int_delay;
> }
> 
> int_delay should really be a u32. if ret is not an error, there should
> be a range check to ensure int_long actually fits in an s32, otherwise
> -EINVAL, or maybe -ERANGE.
> 
> For delays, we never expect too much more than 2000ps, so no valid DT
> blob should trigger issues here.
> 
I think you mention this because you want to avoid changes in
phy_get_internal_delay because this would lead to changes in other
drivers too. Is it worth fixing this ? Then we didn't have to workaround by
checking if int_long actually fits in an s32.

Best regards,
Dimitri Fedrau