[PATCH 1/3] dt-bindings: net: dp83826: add ti,cfg-dac-minus binding

[PATCH 1/3] dt-bindings: net: dp83826: add ti,cfg-dac-minus binding

[Catalin Popescu]

Add property ti,cfg-dac-minus to allow for voltage tuning
of logical level -1 of the MLT-3 encoded data.

Signed-off-by: Catalin Popescu <catalin.popescu@leica-geosystems.com>
---
 Documentation/devicetree/bindings/net/ti,dp83822.yaml | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/Documentation/devicetree/bindings/net/ti,dp83822.yaml b/Documentation/devicetree/bindings/net/ti,dp83822.yaml
index db74474207ed..2f010333be49 100644
--- a/Documentation/devicetree/bindings/net/ti,dp83822.yaml
+++ b/Documentation/devicetree/bindings/net/ti,dp83822.yaml
@@ -62,6 +62,15 @@ properties:
        for the PHY.  The internal delay for the PHY is fixed to 3.5ns relative
        to transmit data.
 
+  ti,cfg-dac-minus:
+    description: |
+       DP83826 PHY only.
+       Sets the voltage ratio of the logical level -1 for the MLT-3 encoded data.
+       0 = 50%, 1 = 56.25%, 2 = 62.50%, 3 = 68.75%, 4 = 75%, 5 = 81.25%, 6 = 87.50%,
+       7 = 93.75%, 8 = 100%, 9 = 106.25%, 10 = 112.50%, 11 = 118.75%, 12 = 125%,
+       13 = 131.25%, 14 = 137.50%, 15 = 143.75%, 16 = 150%.
+    enum: [0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16]
+
 required:
   - reg
 
-- 
2.34.1

[PATCH 2/3] dt-bindings: net: dp83826: add ti,cfg-dac-plus binding

[Catalin Popescu]

Add property ti,cfg-dac-plus to allow for voltage tuning
of logical level +1 of the MLT-3 encoded data.

Signed-off-by: Catalin Popescu <catalin.popescu@leica-geosystems.com>
---
 Documentation/devicetree/bindings/net/ti,dp83822.yaml | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/Documentation/devicetree/bindings/net/ti,dp83822.yaml b/Documentation/devicetree/bindings/net/ti,dp83822.yaml
index 2f010333be49..f9b68bde57a4 100644
--- a/Documentation/devicetree/bindings/net/ti,dp83822.yaml
+++ b/Documentation/devicetree/bindings/net/ti,dp83822.yaml
@@ -71,6 +71,15 @@ properties:
        13 = 131.25%, 14 = 137.50%, 15 = 143.75%, 16 = 150%.
     enum: [0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16]
 
+  ti,cfg-dac-plus:
+    description: |
+       DP83826 PHY only.
+       Sets the voltage ratio of the logical level +1 for the MLT-3 encoded data.
+       0 = 50%, 1 = 56.25%, 2 = 62.50%, 3 = 68.75%, 4 = 75%, 5 = 81.25%, 6 = 87.50%,
+       7 = 93.75%, 8 = 100%, 9 = 106.25%, 10 = 112.50%, 11 = 118.75%, 12 = 125%,
+       13 = 131.25%, 14 = 137.50%, 15 = 143.75%, 16 = 150%.
+    enum: [0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16]
+
 required:
   - reg
 
-- 
2.34.1

[PATCH 3/3] net: phy: dp83826: add support for voltage tuning of logical levels

[Catalin Popescu]

DP83826 offers the possibility to tune the voltage of logical levels
of the MLT-3 encoded data. This is especially interesting when the
data path is lossy and we want to increase the voltage levels to
compensate the loss.

Signed-off-by: Catalin Popescu <catalin.popescu@leica-geosystems.com>
---
 drivers/net/phy/dp83822.c | 127 ++++++++++++++++++++++++++++++++++++--
 1 file changed, 123 insertions(+), 4 deletions(-)

diff --git a/drivers/net/phy/dp83822.c b/drivers/net/phy/dp83822.c
index b7cb71817780..e26aaacc7ea5 100644
--- a/drivers/net/phy/dp83822.c
+++ b/drivers/net/phy/dp83822.c
@@ -12,6 +12,7 @@
 #include <linux/of.h>
 #include <linux/phy.h>
 #include <linux/netdevice.h>
+#include <linux/bitfield.h>
 
 #define DP83822_PHY_ID	        0x2000a240
 #define DP83825S_PHY_ID		0x2000a140
@@ -34,6 +35,10 @@
 #define MII_DP83822_GENCFG	0x465
 #define MII_DP83822_SOR1	0x467
 
+/* DP83826 specific registers */
+#define MII_DP83826_VOD_CFG1	0x30b
+#define MII_DP83826_VOD_CFG2	0x30c
+
 /* GENCFG */
 #define DP83822_SIG_DET_LOW	BIT(0)
 
@@ -110,6 +115,16 @@
 #define DP83822_RX_ER_STR_MASK	GENMASK(9, 8)
 #define DP83822_RX_ER_SHIFT	8
 
+/* DP83826: VOD_CFG1 & VOD_CFG2 */
+#define DP83826_VOD_CFG1_MINUS_MDIX_MASK	GENMASK(13, 12)
+#define DP83826_VOD_CFG1_MINUS_MDI_MASK		GENMASK(11, 6)
+#define DP83826_VOD_CFG2_MINUS_MDIX_MASK	GENMASK(15, 12)
+#define DP83826_VOD_CFG2_PLUS_MDIX_MASK		GENMASK(11, 6)
+#define DP83826_VOD_CFG2_PLUS_MDI_MASK		GENMASK(5, 0)
+#define DP83826_CFG_DAC_MINUS_MDIX_5_TO_4	GENMASK(5, 4)
+#define DP83826_CFG_DAC_MINUS_MDIX_3_TO_0	GENMASK(3, 0)
+#define DP83826_CFG_DAC_RAW_VALUES_MAX		17
+
 #define MII_DP83822_FIBER_ADVERTISE    (ADVERTISED_TP | ADVERTISED_MII | \
 					ADVERTISED_FIBRE | \
 					ADVERTISED_Pause | ADVERTISED_Asym_Pause)
@@ -118,6 +133,8 @@ struct dp83822_private {
 	bool fx_signal_det_low;
 	int fx_enabled;
 	u16 fx_sd_enable;
+	u8 cfg_dac_minus;
+	u8 cfg_dac_plus;
 };
 
 static int dp83822_set_wol(struct phy_device *phydev,
@@ -233,7 +250,7 @@ static int dp83822_config_intr(struct phy_device *phydev)
 				DP83822_ENERGY_DET_INT_EN |
 				DP83822_LINK_QUAL_INT_EN);
 
-		/* Private data pointer is NULL on DP83825/26 */
+		/* Private data pointer is NULL on DP83825 */
 		if (!dp83822 || !dp83822->fx_enabled)
 			misr_status |= DP83822_ANEG_COMPLETE_INT_EN |
 				       DP83822_DUP_MODE_CHANGE_INT_EN |
@@ -254,7 +271,7 @@ static int dp83822_config_intr(struct phy_device *phydev)
 				DP83822_PAGE_RX_INT_EN |
 				DP83822_EEE_ERROR_CHANGE_INT_EN);
 
-		/* Private data pointer is NULL on DP83825/26 */
+		/* Private data pointer is NULL on DP83825 */
 		if (!dp83822 || !dp83822->fx_enabled)
 			misr_status |= DP83822_ANEG_ERR_INT_EN |
 				       DP83822_WOL_PKT_INT_EN;
@@ -474,6 +491,46 @@ static int dp83822_config_init(struct phy_device *phydev)
 	return dp8382x_disable_wol(phydev);
 }
 
+static int dp83826_config_init(struct phy_device *phydev)
+{
+	struct dp83822_private *dp83822 = phydev->priv;
+	u16 vod_cfg1_val = 0, vod_cfg1_msk = 0, vod_cfg2_val = 0, vod_cfg2_msk = 0;
+	int ret;
+
+	if (dp83822->cfg_dac_plus) {
+		vod_cfg2_val = FIELD_PREP(DP83826_VOD_CFG2_PLUS_MDIX_MASK, dp83822->cfg_dac_plus) |
+			       FIELD_PREP(DP83826_VOD_CFG2_PLUS_MDI_MASK, dp83822->cfg_dac_plus);
+		vod_cfg2_msk = DP83826_VOD_CFG2_PLUS_MDIX_MASK | DP83826_VOD_CFG2_PLUS_MDI_MASK;
+	}
+
+	if (dp83822->cfg_dac_minus) {
+		vod_cfg2_val |= FIELD_PREP(DP83826_VOD_CFG2_MINUS_MDIX_MASK,
+				FIELD_GET(DP83826_CFG_DAC_MINUS_MDIX_3_TO_0,
+					  dp83822->cfg_dac_minus));
+		vod_cfg2_msk |= DP83826_VOD_CFG2_MINUS_MDIX_MASK;
+		vod_cfg1_val = FIELD_PREP(DP83826_VOD_CFG1_MINUS_MDI_MASK, dp83822->cfg_dac_minus) |
+			       FIELD_PREP(DP83826_VOD_CFG1_MINUS_MDIX_MASK,
+					  FIELD_GET(DP83826_CFG_DAC_MINUS_MDIX_5_TO_4,
+						    dp83822->cfg_dac_minus));
+		vod_cfg1_msk = DP83826_VOD_CFG1_MINUS_MDIX_MASK | DP83826_VOD_CFG1_MINUS_MDI_MASK;
+	}
+
+	if (vod_cfg1_msk) {
+		ret = phy_modify_mmd(phydev, DP83822_DEVADDR, MII_DP83826_VOD_CFG1,
+				     vod_cfg1_msk, vod_cfg1_val);
+		if (ret)
+			return ret;
+	}
+	if (vod_cfg2_msk) {
+		ret = phy_modify_mmd(phydev, DP83822_DEVADDR, MII_DP83826_VOD_CFG2,
+				     vod_cfg2_msk, vod_cfg2_val);
+		if (ret)
+			return ret;
+	}
+
+	return 0;
+}
+
 static int dp8382x_config_init(struct phy_device *phydev)
 {
 	return dp8382x_disable_wol(phydev);
@@ -509,11 +566,41 @@ static int dp83822_of_init(struct phy_device *phydev)
 
 	return 0;
 }
+
+u8 dp83826_cfg_dac_minus_raw[DP83826_CFG_DAC_RAW_VALUES_MAX] = {0x38, 0x37, 0x36, 0x35, 0x34, 0x33,
+								0x32, 0x31, 0x30, 0x2f, 0x2e, 0x2d,
+								0x2c, 0x2b, 0x2a, 0x29, 0x28};
+u8 dp83826_cfg_dac_plus_raw[DP83826_CFG_DAC_RAW_VALUES_MAX] = {0x08, 0x09, 0x0a, 0x0b, 0x0c, 0x0d,
+							       0x0e, 0x0f, 0x10, 0x11, 0x12, 0x13,
+							       0x14, 0x15, 0x16, 0x17, 0x18};
+
+static int dp83826_of_init(struct phy_device *phydev)
+{
+	struct dp83822_private *dp83822 = phydev->priv;
+	struct device *dev = &phydev->mdio.dev;
+	u32 val;
+	int ret;
+
+	ret = device_property_read_u32(dev, "ti,cfg-dac-minus", &val);
+	if (!ret && val < DP83826_CFG_DAC_RAW_VALUES_MAX)
+		dp83822->cfg_dac_minus = dp83826_cfg_dac_minus_raw[val];
+
+	ret = device_property_read_u32(dev, "ti,cfg-dac-plus", &val);
+	if (!ret && val < DP83826_CFG_DAC_RAW_VALUES_MAX)
+		dp83822->cfg_dac_plus = dp83826_cfg_dac_plus_raw[val];
+
+	return 0;
+}
 #else
 static int dp83822_of_init(struct phy_device *phydev)
 {
 	return 0;
 }
+
+static int dp83826_of_init(struct phy_device *phydev)
+{
+	return 0;
+}
 #endif /* CONFIG_OF_MDIO */
 
 static int dp83822_read_straps(struct phy_device *phydev)
@@ -567,6 +654,22 @@ static int dp83822_probe(struct phy_device *phydev)
 	return 0;
 }
 
+static int dp83826_probe(struct phy_device *phydev)
+{
+	struct dp83822_private *dp83822;
+
+	dp83822 = devm_kzalloc(&phydev->mdio.dev, sizeof(*dp83822),
+			       GFP_KERNEL);
+	if (!dp83822)
+		return -ENOMEM;
+
+	phydev->priv = dp83822;
+
+	dp83826_of_init(phydev);
+
+	return 0;
+}
+
 static int dp83822_suspend(struct phy_device *phydev)
 {
 	int value;
@@ -610,6 +713,22 @@ static int dp83822_resume(struct phy_device *phydev)
 		.resume = dp83822_resume,			\
 	}
 
+#define DP83826_PHY_DRIVER(_id, _name)				\
+	{							\
+		PHY_ID_MATCH_MODEL(_id),			\
+		.name		= (_name),			\
+		/* PHY_BASIC_FEATURES */			\
+		.probe          = dp83826_probe,		\
+		.soft_reset	= dp83822_phy_reset,		\
+		.config_init	= dp83826_config_init,		\
+		.get_wol = dp83822_get_wol,			\
+		.set_wol = dp83822_set_wol,			\
+		.config_intr = dp83822_config_intr,		\
+		.handle_interrupt = dp83822_handle_interrupt,	\
+		.suspend = dp83822_suspend,			\
+		.resume = dp83822_resume,			\
+	}
+
 #define DP8382X_PHY_DRIVER(_id, _name)				\
 	{							\
 		PHY_ID_MATCH_MODEL(_id),			\
@@ -628,8 +747,8 @@ static int dp83822_resume(struct phy_device *phydev)
 static struct phy_driver dp83822_driver[] = {
 	DP83822_PHY_DRIVER(DP83822_PHY_ID, "TI DP83822"),
 	DP8382X_PHY_DRIVER(DP83825I_PHY_ID, "TI DP83825I"),
-	DP8382X_PHY_DRIVER(DP83826C_PHY_ID, "TI DP83826C"),
-	DP8382X_PHY_DRIVER(DP83826NC_PHY_ID, "TI DP83826NC"),
+	DP83826_PHY_DRIVER(DP83826C_PHY_ID, "TI DP83826C"),
+	DP83826_PHY_DRIVER(DP83826NC_PHY_ID, "TI DP83826NC"),
 	DP8382X_PHY_DRIVER(DP83825S_PHY_ID, "TI DP83825S"),
 	DP8382X_PHY_DRIVER(DP83825CM_PHY_ID, "TI DP83825M"),
 	DP8382X_PHY_DRIVER(DP83825CS_PHY_ID, "TI DP83825CS"),
-- 
2.34.1

Re: [PATCH 1/3] dt-bindings: net: dp83826: add ti,cfg-dac-minus binding

[Andrew Lunn]

On Thu, Jan 11, 2024 at 05:19:25PM +0100, Catalin Popescu wrote:
> Add property ti,cfg-dac-minus to allow for voltage tuning
> of logical level -1 of the MLT-3 encoded data.
> 
> Signed-off-by: Catalin Popescu <catalin.popescu@leica-geosystems.com>
> ---
>  Documentation/devicetree/bindings/net/ti,dp83822.yaml | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/net/ti,dp83822.yaml b/Documentation/devicetree/bindings/net/ti,dp83822.yaml
> index db74474207ed..2f010333be49 100644
> --- a/Documentation/devicetree/bindings/net/ti,dp83822.yaml
> +++ b/Documentation/devicetree/bindings/net/ti,dp83822.yaml
> @@ -62,6 +62,15 @@ properties:
>         for the PHY.  The internal delay for the PHY is fixed to 3.5ns relative
>         to transmit data.
>  
> +  ti,cfg-dac-minus:
> +    description: |
> +       DP83826 PHY only.
> +       Sets the voltage ratio of the logical level -1 for the MLT-3 encoded data.
> +       0 = 50%, 1 = 56.25%, 2 = 62.50%, 3 = 68.75%, 4 = 75%, 5 = 81.25%, 6 = 87.50%,
> +       7 = 93.75%, 8 = 100%, 9 = 106.25%, 10 = 112.50%, 11 = 118.75%, 12 = 125%,
> +       13 = 131.25%, 14 = 137.50%, 15 = 143.75%, 16 = 150%.
> +    enum: [0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16]

We try to avoid register values in DT. We use real units. This is a
voltage you are configuring, so can you change the unit to millivolts?
Have the driver do the conversion of volts to register value.

Is it possible to configure any of the other logical levels?

    Andrew

---
pw-bot: cr

Re: [PATCH 1/3] dt-bindings: net: dp83826: add ti,cfg-dac-minus binding

[POPESCU Catalin]

On 11.01.24 17:35, Andrew Lunn wrote:
> [You don't often get email from andrew@lunn.ch. Learn why this is important at https://aka.ms/LearnAboutSenderIdentification ]
>
> This email is not from Hexagon’s Office 365 instance. Please be careful while clicking links, opening attachments, or replying to this email.
>
>
> On Thu, Jan 11, 2024 at 05:19:25PM +0100, Catalin Popescu wrote:
>> Add property ti,cfg-dac-minus to allow for voltage tuning
>> of logical level -1 of the MLT-3 encoded data.
>>
>> Signed-off-by: Catalin Popescu <catalin.popescu@leica-geosystems.com>
>> ---
>>   Documentation/devicetree/bindings/net/ti,dp83822.yaml | 9 +++++++++
>>   1 file changed, 9 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/net/ti,dp83822.yaml b/Documentation/devicetree/bindings/net/ti,dp83822.yaml
>> index db74474207ed..2f010333be49 100644
>> --- a/Documentation/devicetree/bindings/net/ti,dp83822.yaml
>> +++ b/Documentation/devicetree/bindings/net/ti,dp83822.yaml
>> @@ -62,6 +62,15 @@ properties:
>>          for the PHY.  The internal delay for the PHY is fixed to 3.5ns relative
>>          to transmit data.
>>
>> +  ti,cfg-dac-minus:
>> +    description: |
>> +       DP83826 PHY only.
>> +       Sets the voltage ratio of the logical level -1 for the MLT-3 encoded data.
>> +       0 = 50%, 1 = 56.25%, 2 = 62.50%, 3 = 68.75%, 4 = 75%, 5 = 81.25%, 6 = 87.50%,
>> +       7 = 93.75%, 8 = 100%, 9 = 106.25%, 10 = 112.50%, 11 = 118.75%, 12 = 125%,
>> +       13 = 131.25%, 14 = 137.50%, 15 = 143.75%, 16 = 150%.
>> +    enum: [0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16]
> We try to avoid register values in DT. We use real units. This is a
> voltage you are configuring, so can you change the unit to millivolts?
> Have the driver do the conversion of volts to register value.
>
> Is it possible to configure any of the other logical levels?

Hi Andrew,
These are not raw register values and these are not voltage values but 
voltage ratios. I'm mapping the voltage ratios to enum values [0-16] 
which are converted to register raw values by the driver. I don't see a 
better way to do this.

>      Andrew
>
> ---
> pw-bot: cr

Re: [PATCH 3/3] net: phy: dp83826: add support for voltage tuning of logical levels

[Andrew Lunn]

> +u8 dp83826_cfg_dac_minus_raw[DP83826_CFG_DAC_RAW_VALUES_MAX] = {0x38, 0x37, 0x36, 0x35, 0x34, 0x33,
> +								0x32, 0x31, 0x30, 0x2f, 0x2e, 0x2d,
> +								0x2c, 0x2b, 0x2a, 0x29, 0x28};
> +u8 dp83826_cfg_dac_plus_raw[DP83826_CFG_DAC_RAW_VALUES_MAX] = {0x08, 0x09, 0x0a, 0x0b, 0x0c, 0x0d,
> +							       0x0e, 0x0f, 0x10, 0x11, 0x12, 0x13,
> +							       0x14, 0x15, 0x16, 0x17, 0x18};

Both of these should be static const.

However, they appear pointless. Plus is just a shift. minus is some
simple arithmetic and a shift.

       Andrew

Re: [PATCH 2/3] dt-bindings: net: dp83826: add ti,cfg-dac-plus binding

[POPESCU Catalin]

Adding more people in CC.

On 11.01.24 17:19, Catalin Popescu wrote:
> Add property ti,cfg-dac-plus to allow for voltage tuning
> of logical level +1 of the MLT-3 encoded data.
>
> Signed-off-by: Catalin Popescu <catalin.popescu@leica-geosystems.com>
> ---
>   Documentation/devicetree/bindings/net/ti,dp83822.yaml | 9 +++++++++
>   1 file changed, 9 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/net/ti,dp83822.yaml b/Documentation/devicetree/bindings/net/ti,dp83822.yaml
> index 2f010333be49..f9b68bde57a4 100644
> --- a/Documentation/devicetree/bindings/net/ti,dp83822.yaml
> +++ b/Documentation/devicetree/bindings/net/ti,dp83822.yaml
> @@ -71,6 +71,15 @@ properties:
>          13 = 131.25%, 14 = 137.50%, 15 = 143.75%, 16 = 150%.
>       enum: [0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16]
>   
> +  ti,cfg-dac-plus:
> +    description: |
> +       DP83826 PHY only.
> +       Sets the voltage ratio of the logical level +1 for the MLT-3 encoded data.
> +       0 = 50%, 1 = 56.25%, 2 = 62.50%, 3 = 68.75%, 4 = 75%, 5 = 81.25%, 6 = 87.50%,
> +       7 = 93.75%, 8 = 100%, 9 = 106.25%, 10 = 112.50%, 11 = 118.75%, 12 = 125%,
> +       13 = 131.25%, 14 = 137.50%, 15 = 143.75%, 16 = 150%.
> +    enum: [0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16]
> +
>   required:
>     - reg
>   

Re: [PATCH 1/3] dt-bindings: net: dp83826: add ti,cfg-dac-minus binding

[Andrew Lunn]

> These are not raw register values and these are not voltage values but 
> voltage ratios. I'm mapping the voltage ratios to enum values [0-16] 
> which are converted to register raw values by the driver. I don't see a 
> better way to do this.

I assume 802.3 states what the actual voltage should be? So you can
calculate what 50% of that is?

	  Andrew

Re: [PATCH 1/3] dt-bindings: net: dp83826: add ti,cfg-dac-minus binding

[Russell King (Oracle)]

On Thu, Jan 11, 2024 at 04:45:26PM +0000, POPESCU Catalin wrote:
> On 11.01.24 17:35, Andrew Lunn wrote:
> > [You don't often get email from andrew@lunn.ch. Learn why this is important at https://aka.ms/LearnAboutSenderIdentification ]
> >
> > This email is not from Hexagon’s Office 365 instance. Please be careful while clicking links, opening attachments, or replying to this email.
> >
> >
> > On Thu, Jan 11, 2024 at 05:19:25PM +0100, Catalin Popescu wrote:
> >> Add property ti,cfg-dac-minus to allow for voltage tuning
> >> of logical level -1 of the MLT-3 encoded data.
> >>
> >> Signed-off-by: Catalin Popescu <catalin.popescu@leica-geosystems.com>
> >> ---
> >>   Documentation/devicetree/bindings/net/ti,dp83822.yaml | 9 +++++++++
> >>   1 file changed, 9 insertions(+)
> >>
> >> diff --git a/Documentation/devicetree/bindings/net/ti,dp83822.yaml b/Documentation/devicetree/bindings/net/ti,dp83822.yaml
> >> index db74474207ed..2f010333be49 100644
> >> --- a/Documentation/devicetree/bindings/net/ti,dp83822.yaml
> >> +++ b/Documentation/devicetree/bindings/net/ti,dp83822.yaml
> >> @@ -62,6 +62,15 @@ properties:
> >>          for the PHY.  The internal delay for the PHY is fixed to 3.5ns relative
> >>          to transmit data.
> >>
> >> +  ti,cfg-dac-minus:
> >> +    description: |
> >> +       DP83826 PHY only.
> >> +       Sets the voltage ratio of the logical level -1 for the MLT-3 encoded data.
> >> +       0 = 50%, 1 = 56.25%, 2 = 62.50%, 3 = 68.75%, 4 = 75%, 5 = 81.25%, 6 = 87.50%,
> >> +       7 = 93.75%, 8 = 100%, 9 = 106.25%, 10 = 112.50%, 11 = 118.75%, 12 = 125%,
> >> +       13 = 131.25%, 14 = 137.50%, 15 = 143.75%, 16 = 150%.
> >> +    enum: [0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16]
> > We try to avoid register values in DT. We use real units. This is a
> > voltage you are configuring, so can you change the unit to millivolts?
> > Have the driver do the conversion of volts to register value.
> >
> > Is it possible to configure any of the other logical levels?
> 
> Hi Andrew,
> These are not raw register values and these are not voltage values but 
> voltage ratios. I'm mapping the voltage ratios to enum values [0-16] 
> which are converted to register raw values by the driver. I don't see a 
> better way to do this.

	enum: [ 5000, 5625, 6250, 6875, 7500, 8125, 8750, 9375, 10000,
		10625, 11250, 11875, 12500 13125, 13750, 14375, 15000 ]

?

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

Re: [PATCH 3/3] net: phy: dp83826: add support for voltage tuning of logical levels

[POPESCU Catalin]

On 11.01.24 17:45, Andrew Lunn wrote:
> [You don't often get email from andrew@lunn.ch. Learn why this is important at https://aka.ms/LearnAboutSenderIdentification ]
>
> This email is not from Hexagon’s Office 365 instance. Please be careful while clicking links, opening attachments, or replying to this email.
>
>
>> +u8 dp83826_cfg_dac_minus_raw[DP83826_CFG_DAC_RAW_VALUES_MAX] = {0x38, 0x37, 0x36, 0x35, 0x34, 0x33,
>> +                                                             0x32, 0x31, 0x30, 0x2f, 0x2e, 0x2d,
>> +                                                             0x2c, 0x2b, 0x2a, 0x29, 0x28};
>> +u8 dp83826_cfg_dac_plus_raw[DP83826_CFG_DAC_RAW_VALUES_MAX] = {0x08, 0x09, 0x0a, 0x0b, 0x0c, 0x0d,
>> +                                                            0x0e, 0x0f, 0x10, 0x11, 0x12, 0x13,
>> +                                                            0x14, 0x15, 0x16, 0x17, 0x18};
> Both of these should be static const.
Indeed.
>
> However, they appear pointless. Plus is just a shift. minus is some
> simple arithmetic and a shift.
Well, I found it more clear to use a bit of memory than adding 
instructions to compute some raw values from other raw values used as 
reference.
>
>         Andrew

Re: [PATCH 1/3] dt-bindings: net: dp83826: add ti,cfg-dac-minus binding

[POPESCU Catalin]

On 11.01.24 17:52, Russell King (Oracle) wrote:
> This email is not from Hexagon’s Office 365 instance. Please be careful while clicking links, opening attachments, or replying to this email.
>
>
> On Thu, Jan 11, 2024 at 04:45:26PM +0000, POPESCU Catalin wrote:
>> On 11.01.24 17:35, Andrew Lunn wrote:
>>> [You don't often get email from andrew@lunn.ch. Learn why this is important at https://aka.ms/LearnAboutSenderIdentification ]
>>>
>>> This email is not from Hexagon’s Office 365 instance. Please be careful while clicking links, opening attachments, or replying to this email.
>>>
>>>
>>> On Thu, Jan 11, 2024 at 05:19:25PM +0100, Catalin Popescu wrote:
>>>> Add property ti,cfg-dac-minus to allow for voltage tuning
>>>> of logical level -1 of the MLT-3 encoded data.
>>>>
>>>> Signed-off-by: Catalin Popescu <catalin.popescu@leica-geosystems.com>
>>>> ---
>>>>    Documentation/devicetree/bindings/net/ti,dp83822.yaml | 9 +++++++++
>>>>    1 file changed, 9 insertions(+)
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/net/ti,dp83822.yaml b/Documentation/devicetree/bindings/net/ti,dp83822.yaml
>>>> index db74474207ed..2f010333be49 100644
>>>> --- a/Documentation/devicetree/bindings/net/ti,dp83822.yaml
>>>> +++ b/Documentation/devicetree/bindings/net/ti,dp83822.yaml
>>>> @@ -62,6 +62,15 @@ properties:
>>>>           for the PHY.  The internal delay for the PHY is fixed to 3.5ns relative
>>>>           to transmit data.
>>>>
>>>> +  ti,cfg-dac-minus:
>>>> +    description: |
>>>> +       DP83826 PHY only.
>>>> +       Sets the voltage ratio of the logical level -1 for the MLT-3 encoded data.
>>>> +       0 = 50%, 1 = 56.25%, 2 = 62.50%, 3 = 68.75%, 4 = 75%, 5 = 81.25%, 6 = 87.50%,
>>>> +       7 = 93.75%, 8 = 100%, 9 = 106.25%, 10 = 112.50%, 11 = 118.75%, 12 = 125%,
>>>> +       13 = 131.25%, 14 = 137.50%, 15 = 143.75%, 16 = 150%.
>>>> +    enum: [0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16]
>>> We try to avoid register values in DT. We use real units. This is a
>>> voltage you are configuring, so can you change the unit to millivolts?
>>> Have the driver do the conversion of volts to register value.
>>>
>>> Is it possible to configure any of the other logical levels?
>> Hi Andrew,
>> These are not raw register values and these are not voltage values but
>> voltage ratios. I'm mapping the voltage ratios to enum values [0-16]
>> which are converted to register raw values by the driver. I don't see a
>> better way to do this.
>          enum: [ 5000, 5625, 6250, 6875, 7500, 8125, 8750, 9375, 10000,
>                  10625, 11250, 11875, 12500 13125, 13750, 14375, 15000 ]
>
> ?
I'm okay with that approach if there's no better one. I would need to
remove the register raw values tables from the driver and use a switch
statement to map those values to raw values.
>
> --
> RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
> FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH 1/3] dt-bindings: net: dp83826: add ti,cfg-dac-minus binding

[Krzysztof Kozlowski]

On 11/01/2024 17:19, Catalin Popescu wrote:
> Add property ti,cfg-dac-minus to allow for voltage tuning
> of logical level -1 of the MLT-3 encoded data.

A nit, subject: drop second/last, redundant "binding". The "dt-bindings"
prefix is already stating that these are bindings.
See also:
https://elixir.bootlin.com/linux/v6.7-rc8/source/Documentation/devicetree/bindings/submitting-patches.rst#L18
> 
> Signed-off-by: Catalin Popescu <catalin.popescu@leica-geosystems.com>
> ---
>  Documentation/devicetree/bindings/net/ti,dp83822.yaml | 9 +++++++++
>  1 file changed, 9 insertions(+)

Don't add properties one by one. Especially if they are related.

> 
> diff --git a/Documentation/devicetree/bindings/net/ti,dp83822.yaml b/Documentation/devicetree/bindings/net/ti,dp83822.yaml
> index db74474207ed..2f010333be49 100644
> --- a/Documentation/devicetree/bindings/net/ti,dp83822.yaml
> +++ b/Documentation/devicetree/bindings/net/ti,dp83822.yaml
> @@ -62,6 +62,15 @@ properties:
>         for the PHY.  The internal delay for the PHY is fixed to 3.5ns relative
>         to transmit data.
>  
> +  ti,cfg-dac-minus:
> +    description: |
> +       DP83826 PHY only.
> +       Sets the voltage ratio of the logical level -1 for the MLT-3 encoded data.

ratio between what and what?

> +       0 = 50%, 1 = 56.25%, 2 = 62.50%, 3 = 68.75%, 4 = 75%, 5 = 81.25%, 6 = 87.50%,
> +       7 = 93.75%, 8 = 100%, 9 = 106.25%, 10 = 112.50%, 11 = 118.75%, 12 = 125%,
> +       13 = 131.25%, 14 = 137.50%, 15 = 143.75%, 16 = 150%.
> +    enum: [0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16]

Missing: "default:"


>  

Best regards,
Krzysztof

Re: [PATCH 1/3] dt-bindings: net: dp83826: add ti,cfg-dac-minus binding

[Krzysztof Kozlowski]

On 11/01/2024 17:59, POPESCU Catalin wrote:
> On 11.01.24 17:52, Russell King (Oracle) wrote:
>> This email is not from Hexagon’s Office 365 instance. Please be careful while clicking links, opening attachments, or replying to this email.
>>
>>
>> On Thu, Jan 11, 2024 at 04:45:26PM +0000, POPESCU Catalin wrote:
>>> On 11.01.24 17:35, Andrew Lunn wrote:
>>>> [You don't often get email from andrew@lunn.ch. Learn why this is important at https://aka.ms/LearnAboutSenderIdentification ]
>>>>
>>>> This email is not from Hexagon’s Office 365 instance. Please be careful while clicking links, opening attachments, or replying to this email.
>>>>
>>>>
>>>> On Thu, Jan 11, 2024 at 05:19:25PM +0100, Catalin Popescu wrote:
>>>>> Add property ti,cfg-dac-minus to allow for voltage tuning
>>>>> of logical level -1 of the MLT-3 encoded data.
>>>>>
>>>>> Signed-off-by: Catalin Popescu <catalin.popescu@leica-geosystems.com>
>>>>> ---
>>>>>    Documentation/devicetree/bindings/net/ti,dp83822.yaml | 9 +++++++++
>>>>>    1 file changed, 9 insertions(+)
>>>>>
>>>>> diff --git a/Documentation/devicetree/bindings/net/ti,dp83822.yaml b/Documentation/devicetree/bindings/net/ti,dp83822.yaml
>>>>> index db74474207ed..2f010333be49 100644
>>>>> --- a/Documentation/devicetree/bindings/net/ti,dp83822.yaml
>>>>> +++ b/Documentation/devicetree/bindings/net/ti,dp83822.yaml
>>>>> @@ -62,6 +62,15 @@ properties:
>>>>>           for the PHY.  The internal delay for the PHY is fixed to 3.5ns relative
>>>>>           to transmit data.
>>>>>
>>>>> +  ti,cfg-dac-minus:
>>>>> +    description: |
>>>>> +       DP83826 PHY only.
>>>>> +       Sets the voltage ratio of the logical level -1 for the MLT-3 encoded data.
>>>>> +       0 = 50%, 1 = 56.25%, 2 = 62.50%, 3 = 68.75%, 4 = 75%, 5 = 81.25%, 6 = 87.50%,
>>>>> +       7 = 93.75%, 8 = 100%, 9 = 106.25%, 10 = 112.50%, 11 = 118.75%, 12 = 125%,
>>>>> +       13 = 131.25%, 14 = 137.50%, 15 = 143.75%, 16 = 150%.
>>>>> +    enum: [0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16]
>>>> We try to avoid register values in DT. We use real units. This is a
>>>> voltage you are configuring, so can you change the unit to millivolts?
>>>> Have the driver do the conversion of volts to register value.
>>>>
>>>> Is it possible to configure any of the other logical levels?
>>> Hi Andrew,
>>> These are not raw register values and these are not voltage values but
>>> voltage ratios. I'm mapping the voltage ratios to enum values [0-16]
>>> which are converted to register raw values by the driver. I don't see a
>>> better way to do this.
>>          enum: [ 5000, 5625, 6250, 6875, 7500, 8125, 8750, 9375, 10000,
>>                  10625, 11250, 11875, 12500 13125, 13750, 14375, 15000 ]
>>
>> ?
> I'm okay with that approach if there's no better one. I would need to
> remove the register raw values tables from the driver and use a switch
> statement to map those values to raw values.

You can also use -bp or -percent:
https://github.com/devicetree-org/dt-schema/blob/main/dtschema/schemas/property-units.yaml

Best regards,
Krzysztof

Re: [PATCH 2/3] dt-bindings: net: dp83826: add ti,cfg-dac-plus binding

[Krzysztof Kozlowski]

On 11/01/2024 17:19, Catalin Popescu wrote:
> Add property ti,cfg-dac-plus to allow for voltage tuning
> of logical level +1 of the MLT-3 encoded data.
> 

Please squash it with previous patch.

Best regards,
Krzysztof

Re: [PATCH 1/3] dt-bindings: net: dp83826: add ti,cfg-dac-minus binding

[Rob Herring]

On Thu, 11 Jan 2024 17:19:25 +0100, Catalin Popescu wrote:
> Add property ti,cfg-dac-minus to allow for voltage tuning
> of logical level -1 of the MLT-3 encoded data.
> 
> Signed-off-by: Catalin Popescu <catalin.popescu@leica-geosystems.com>
> ---
>  Documentation/devicetree/bindings/net/ti,dp83822.yaml | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 

My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
on your patch (DT_CHECKER_FLAGS is new in v5.13):

yamllint warnings/errors:

dtschema/dtc warnings/errors:
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/net/ti,dp83822.yaml: ti,cfg-dac-minus: missing type definition

doc reference errors (make refcheckdocs):

See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20240111161927.3689084-1-catalin.popescu@leica-geosystems.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 2/3] dt-bindings: net: dp83826: add ti,cfg-dac-plus binding

[Rob Herring]

On Thu, 11 Jan 2024 17:19:26 +0100, Catalin Popescu wrote:
> Add property ti,cfg-dac-plus to allow for voltage tuning
> of logical level +1 of the MLT-3 encoded data.
> 
> Signed-off-by: Catalin Popescu <catalin.popescu@leica-geosystems.com>
> ---
>  Documentation/devicetree/bindings/net/ti,dp83822.yaml | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 

My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
on your patch (DT_CHECKER_FLAGS is new in v5.13):

yamllint warnings/errors:

dtschema/dtc warnings/errors:
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/net/ti,dp83822.yaml: ti,cfg-dac-plus: missing type definition

doc reference errors (make refcheckdocs):

See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20240111161927.3689084-2-catalin.popescu@leica-geosystems.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 1/3] dt-bindings: net: dp83826: add ti,cfg-dac-minus binding

[POPESCU Catalin]

On 11.01.24 17:47, Andrew Lunn wrote:
> This email is not from Hexagon’s Office 365 instance. Please be careful while clicking links, opening attachments, or replying to this email.
>
>
>> These are not raw register values and these are not voltage values but
>> voltage ratios. I'm mapping the voltage ratios to enum values [0-16]
>> which are converted to register raw values by the driver. I don't see a
>> better way to do this.
> I assume 802.3 states what the actual voltage should be? So you can
> calculate what 50% of that is?
>
>            Andrew
The actual voltage depends on the mode (I know that for 100BASE-TX and 
MLT-3 it's +/-1V) : it makes no sense to try to code the actual voltage, 
we need to keep voltage ratio here.

Re: [PATCH 2/3] dt-bindings: net: dp83826: add ti,cfg-dac-plus binding

[POPESCU Catalin]

On 11.01.24 18:21, Krzysztof Kozlowski wrote:
> [You don't often get email from krzysztof.kozlowski@linaro.org. Learn why this is important at https://aka.ms/LearnAboutSenderIdentification ]
>
> This email is not from Hexagon’s Office 365 instance. Please be careful while clicking links, opening attachments, or replying to this email.
>
>
> On 11/01/2024 17:19, Catalin Popescu wrote:
>> Add property ti,cfg-dac-plus to allow for voltage tuning
>> of logical level +1 of the MLT-3 encoded data.
>>
> Please squash it with previous patch.
OK
>
> Best regards,
> Krzysztof
>

Re: [PATCH 1/3] dt-bindings: net: dp83826: add ti,cfg-dac-minus binding

[POPESCU Catalin]

On 11.01.24 18:20, Krzysztof Kozlowski wrote:
> [You don't often get email from krzysztof.kozlowski@linaro.org. Learn why this is important at https://aka.ms/LearnAboutSenderIdentification ]
>
> This email is not from Hexagon’s Office 365 instance. Please be careful while clicking links, opening attachments, or replying to this email.
>
>
> On 11/01/2024 17:19, Catalin Popescu wrote:
>> Add property ti,cfg-dac-minus to allow for voltage tuning
>> of logical level -1 of the MLT-3 encoded data.
> A nit, subject: drop second/last, redundant "binding". The "dt-bindings"
> prefix is already stating that these are bindings.
> See also:
> https://elixir.bootlin.com/linux/v6.7-rc8/source/Documentation/devicetree/bindings/submitting-patches.rst#L18
>> Signed-off-by: Catalin Popescu <catalin.popescu@leica-geosystems.com>
>> ---
>>   Documentation/devicetree/bindings/net/ti,dp83822.yaml | 9 +++++++++
>>   1 file changed, 9 insertions(+)
> Don't add properties one by one. Especially if they are related.
OK
>
>> diff --git a/Documentation/devicetree/bindings/net/ti,dp83822.yaml b/Documentation/devicetree/bindings/net/ti,dp83822.yaml
>> index db74474207ed..2f010333be49 100644
>> --- a/Documentation/devicetree/bindings/net/ti,dp83822.yaml
>> +++ b/Documentation/devicetree/bindings/net/ti,dp83822.yaml
>> @@ -62,6 +62,15 @@ properties:
>>          for the PHY.  The internal delay for the PHY is fixed to 3.5ns relative
>>          to transmit data.
>>
>> +  ti,cfg-dac-minus:
>> +    description: |
>> +       DP83826 PHY only.
>> +       Sets the voltage ratio of the logical level -1 for the MLT-3 encoded data.
> ratio between what and what?
It's the ration b/w the actual output voltage of AOI and the spec
reference (the datasheet is not very clear).
>
>> +       0 = 50%, 1 = 56.25%, 2 = 62.50%, 3 = 68.75%, 4 = 75%, 5 = 81.25%, 6 = 87.50%,
>> +       7 = 93.75%, 8 = 100%, 9 = 106.25%, 10 = 112.50%, 11 = 118.75%, 12 = 125%,
>> +       13 = 131.25%, 14 = 137.50%, 15 = 143.75%, 16 = 150%.
>> +    enum: [0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16]
> Missing: "default:"
>
>
> Best regards,
> Krzysztof
>

Re: [PATCH 1/3] dt-bindings: net: dp83826: add ti,cfg-dac-minus binding

[POPESCU Catalin]

On 11.01.24 18:21, Krzysztof Kozlowski wrote:
> [You don't often get email from krzysztof.kozlowski@linaro.org. Learn why this is important at https://aka.ms/LearnAboutSenderIdentification ]
>
> This email is not from Hexagon’s Office 365 instance. Please be careful while clicking links, opening attachments, or replying to this email.
>
>
> On 11/01/2024 17:59, POPESCU Catalin wrote:
>> On 11.01.24 17:52, Russell King (Oracle) wrote:
>>> This email is not from Hexagon’s Office 365 instance. Please be careful while clicking links, opening attachments, or replying to this email.
>>>
>>>
>>> On Thu, Jan 11, 2024 at 04:45:26PM +0000, POPESCU Catalin wrote:
>>>> On 11.01.24 17:35, Andrew Lunn wrote:
>>>>> [You don't often get email from andrew@lunn.ch. Learn why this is important at https://aka.ms/LearnAboutSenderIdentification ]
>>>>>
>>>>> This email is not from Hexagon’s Office 365 instance. Please be careful while clicking links, opening attachments, or replying to this email.
>>>>>
>>>>>
>>>>> On Thu, Jan 11, 2024 at 05:19:25PM +0100, Catalin Popescu wrote:
>>>>>> Add property ti,cfg-dac-minus to allow for voltage tuning
>>>>>> of logical level -1 of the MLT-3 encoded data.
>>>>>>
>>>>>> Signed-off-by: Catalin Popescu <catalin.popescu@leica-geosystems.com>
>>>>>> ---
>>>>>>     Documentation/devicetree/bindings/net/ti,dp83822.yaml | 9 +++++++++
>>>>>>     1 file changed, 9 insertions(+)
>>>>>>
>>>>>> diff --git a/Documentation/devicetree/bindings/net/ti,dp83822.yaml b/Documentation/devicetree/bindings/net/ti,dp83822.yaml
>>>>>> index db74474207ed..2f010333be49 100644
>>>>>> --- a/Documentation/devicetree/bindings/net/ti,dp83822.yaml
>>>>>> +++ b/Documentation/devicetree/bindings/net/ti,dp83822.yaml
>>>>>> @@ -62,6 +62,15 @@ properties:
>>>>>>            for the PHY.  The internal delay for the PHY is fixed to 3.5ns relative
>>>>>>            to transmit data.
>>>>>>
>>>>>> +  ti,cfg-dac-minus:
>>>>>> +    description: |
>>>>>> +       DP83826 PHY only.
>>>>>> +       Sets the voltage ratio of the logical level -1 for the MLT-3 encoded data.
>>>>>> +       0 = 50%, 1 = 56.25%, 2 = 62.50%, 3 = 68.75%, 4 = 75%, 5 = 81.25%, 6 = 87.50%,
>>>>>> +       7 = 93.75%, 8 = 100%, 9 = 106.25%, 10 = 112.50%, 11 = 118.75%, 12 = 125%,
>>>>>> +       13 = 131.25%, 14 = 137.50%, 15 = 143.75%, 16 = 150%.
>>>>>> +    enum: [0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16]
>>>>> We try to avoid register values in DT. We use real units. This is a
>>>>> voltage you are configuring, so can you change the unit to millivolts?
>>>>> Have the driver do the conversion of volts to register value.
>>>>>
>>>>> Is it possible to configure any of the other logical levels?
>>>> Hi Andrew,
>>>> These are not raw register values and these are not voltage values but
>>>> voltage ratios. I'm mapping the voltage ratios to enum values [0-16]
>>>> which are converted to register raw values by the driver. I don't see a
>>>> better way to do this.
>>>           enum: [ 5000, 5625, 6250, 6875, 7500, 8125, 8750, 9375, 10000,
>>>                   10625, 11250, 11875, 12500 13125, 13750, 14375, 15000 ]
>>>
>>> ?
>> I'm okay with that approach if there's no better one. I would need to
>> remove the register raw values tables from the driver and use a switch
>> statement to map those values to raw values.
> You can also use -bp or -percent:
> https://github.com/devicetree-org/dt-schema/blob/main/dtschema/schemas/property-units.yaml

   ti,cfg-dac-minus-percent:
     description: |
        DP83826 PHY only.
        Sets the voltage ratio of the logical level -1 relative to the
nominal level for the MLT-3 encoded TX data.
     enum: [50, 56, 62, 68, 75, 81, 87, 93, 100, 106, 112, 118, 125,
131, 137, 143, 150]
     default: 100


would this be acceptable ??

> Best regards,
> Krzysztof
>

Re: [PATCH 1/3] dt-bindings: net: dp83826: add ti,cfg-dac-minus binding

[Krzysztof Kozlowski]

On 12/01/2024 14:41, POPESCU Catalin wrote:
> On 11.01.24 18:21, Krzysztof Kozlowski wrote:
>> [You don't often get email from krzysztof.kozlowski@linaro.org. Learn why this is important at https://aka.ms/LearnAboutSenderIdentification ]
>>
>> This email is not from Hexagon’s Office 365 instance. Please be careful while clicking links, opening attachments, or replying to this email.
>>
>>
>> On 11/01/2024 17:59, POPESCU Catalin wrote:
>>> On 11.01.24 17:52, Russell King (Oracle) wrote:
>>>> This email is not from Hexagon’s Office 365 instance. Please be careful while clicking links, opening attachments, or replying to this email.
>>>>
>>>>
>>>> On Thu, Jan 11, 2024 at 04:45:26PM +0000, POPESCU Catalin wrote:
>>>>> On 11.01.24 17:35, Andrew Lunn wrote:
>>>>>> [You don't often get email from andrew@lunn.ch. Learn why this is important at https://aka.ms/LearnAboutSenderIdentification ]
>>>>>>
>>>>>> This email is not from Hexagon’s Office 365 instance. Please be careful while clicking links, opening attachments, or replying to this email.
>>>>>>
>>>>>>
>>>>>> On Thu, Jan 11, 2024 at 05:19:25PM +0100, Catalin Popescu wrote:
>>>>>>> Add property ti,cfg-dac-minus to allow for voltage tuning
>>>>>>> of logical level -1 of the MLT-3 encoded data.
>>>>>>>
>>>>>>> Signed-off-by: Catalin Popescu <catalin.popescu@leica-geosystems.com>
>>>>>>> ---
>>>>>>>     Documentation/devicetree/bindings/net/ti,dp83822.yaml | 9 +++++++++
>>>>>>>     1 file changed, 9 insertions(+)
>>>>>>>
>>>>>>> diff --git a/Documentation/devicetree/bindings/net/ti,dp83822.yaml b/Documentation/devicetree/bindings/net/ti,dp83822.yaml
>>>>>>> index db74474207ed..2f010333be49 100644
>>>>>>> --- a/Documentation/devicetree/bindings/net/ti,dp83822.yaml
>>>>>>> +++ b/Documentation/devicetree/bindings/net/ti,dp83822.yaml
>>>>>>> @@ -62,6 +62,15 @@ properties:
>>>>>>>            for the PHY.  The internal delay for the PHY is fixed to 3.5ns relative
>>>>>>>            to transmit data.
>>>>>>>
>>>>>>> +  ti,cfg-dac-minus:
>>>>>>> +    description: |
>>>>>>> +       DP83826 PHY only.
>>>>>>> +       Sets the voltage ratio of the logical level -1 for the MLT-3 encoded data.
>>>>>>> +       0 = 50%, 1 = 56.25%, 2 = 62.50%, 3 = 68.75%, 4 = 75%, 5 = 81.25%, 6 = 87.50%,
>>>>>>> +       7 = 93.75%, 8 = 100%, 9 = 106.25%, 10 = 112.50%, 11 = 118.75%, 12 = 125%,
>>>>>>> +       13 = 131.25%, 14 = 137.50%, 15 = 143.75%, 16 = 150%.
>>>>>>> +    enum: [0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16]
>>>>>> We try to avoid register values in DT. We use real units. This is a
>>>>>> voltage you are configuring, so can you change the unit to millivolts?
>>>>>> Have the driver do the conversion of volts to register value.
>>>>>>
>>>>>> Is it possible to configure any of the other logical levels?
>>>>> Hi Andrew,
>>>>> These are not raw register values and these are not voltage values but
>>>>> voltage ratios. I'm mapping the voltage ratios to enum values [0-16]
>>>>> which are converted to register raw values by the driver. I don't see a
>>>>> better way to do this.
>>>>           enum: [ 5000, 5625, 6250, 6875, 7500, 8125, 8750, 9375, 10000,
>>>>                   10625, 11250, 11875, 12500 13125, 13750, 14375, 15000 ]
>>>>
>>>> ?
>>> I'm okay with that approach if there's no better one. I would need to
>>> remove the register raw values tables from the driver and use a switch
>>> statement to map those values to raw values.
>> You can also use -bp or -percent:
>> https://github.com/devicetree-org/dt-schema/blob/main/dtschema/schemas/property-units.yaml
> 
>    ti,cfg-dac-minus-percent:
>      description: |
>         DP83826 PHY only.
>         Sets the voltage ratio of the logical level -1 relative to the
> nominal level for the MLT-3 encoded TX data.
>      enum: [50, 56, 62, 68, 75, 81, 87, 93, 100, 106, 112, 118, 125,
> 131, 137, 143, 150]
>      default: 100

Yes, which would also solve your problem of binding errors. But does not
remove the need of testing it before sending to the lists.

Best regards,
Krzysztof

Re: [PATCH 3/3] net: phy: dp83826: add support for voltage tuning of logical levels

[kernel test robot]

Hi Catalin,

kernel test robot noticed the following build warnings:

[auto build test WARNING on robh/for-next]
[also build test WARNING on net-next/main net/main linus/master v6.7 next-20240112]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Catalin-Popescu/dt-bindings-net-dp83826-add-ti-cfg-dac-plus-binding/20240112-002701
base:   https://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git for-next
patch link:    https://lore.kernel.org/r/20240111161927.3689084-3-catalin.popescu%40leica-geosystems.com
patch subject: [PATCH 3/3] net: phy: dp83826: add support for voltage tuning of logical levels
config: x86_64-randconfig-121-20240112 (https://download.01.org/0day-ci/archive/20240113/202401131120.vAXFTd3t-lkp@intel.com/config)
compiler: gcc-9 (Debian 9.3.0-22) 9.3.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240113/202401131120.vAXFTd3t-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202401131120.vAXFTd3t-lkp@intel.com/

sparse warnings: (new ones prefixed by >>)
>> drivers/net/phy/dp83822.c:570:4: sparse: sparse: symbol 'dp83826_cfg_dac_minus_raw' was not declared. Should it be static?
>> drivers/net/phy/dp83822.c:573:4: sparse: sparse: symbol 'dp83826_cfg_dac_plus_raw' was not declared. Should it be static?

vim +/dp83826_cfg_dac_minus_raw +570 drivers/net/phy/dp83822.c

   569	
 > 570	u8 dp83826_cfg_dac_minus_raw[DP83826_CFG_DAC_RAW_VALUES_MAX] = {0x38, 0x37, 0x36, 0x35, 0x34, 0x33,
   571									0x32, 0x31, 0x30, 0x2f, 0x2e, 0x2d,
   572									0x2c, 0x2b, 0x2a, 0x29, 0x28};
 > 573	u8 dp83826_cfg_dac_plus_raw[DP83826_CFG_DAC_RAW_VALUES_MAX] = {0x08, 0x09, 0x0a, 0x0b, 0x0c, 0x0d,
   574								       0x0e, 0x0f, 0x10, 0x11, 0x12, 0x13,
   575								       0x14, 0x15, 0x16, 0x17, 0x18};
   576	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH 1/3] dt-bindings: net: dp83826: add ti,cfg-dac-minus binding

[kernel test robot]

Hi Catalin,

kernel test robot noticed the following build warnings:

[auto build test WARNING on robh/for-next]
[also build test WARNING on net-next/main net/main linus/master v6.7 next-20240112]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Catalin-Popescu/dt-bindings-net-dp83826-add-ti-cfg-dac-plus-binding/20240112-002701
base:   https://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git for-next
patch link:    https://lore.kernel.org/r/20240111161927.3689084-1-catalin.popescu%40leica-geosystems.com
patch subject: [PATCH 1/3] dt-bindings: net: dp83826: add ti,cfg-dac-minus binding
compiler: loongarch64-linux-gcc (GCC) 13.2.0
reproduce: (https://download.01.org/0day-ci/archive/20240113/202401131320.WhWHSzeD-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202401131320.WhWHSzeD-lkp@intel.com/

dtcheck warnings: (new ones prefixed by >>)
>> Documentation/devicetree/bindings/net/ti,dp83822.yaml: ti,cfg-dac-minus: missing type definition
   Documentation/devicetree/bindings/net/snps,dwmac.yaml: mac-mode: missing type definition

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH 2/3] dt-bindings: net: dp83826: add ti,cfg-dac-plus binding

[kernel test robot]

Hi Catalin,

kernel test robot noticed the following build warnings:

[auto build test WARNING on robh/for-next]
[also build test WARNING on net-next/main net/main linus/master v6.7 next-20240112]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Catalin-Popescu/dt-bindings-net-dp83826-add-ti-cfg-dac-plus-binding/20240112-002701
base:   https://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git for-next
patch link:    https://lore.kernel.org/r/20240111161927.3689084-2-catalin.popescu%40leica-geosystems.com
patch subject: [PATCH 2/3] dt-bindings: net: dp83826: add ti,cfg-dac-plus binding
compiler: loongarch64-linux-gcc (GCC) 13.2.0
reproduce: (https://download.01.org/0day-ci/archive/20240114/202401140357.ZT1pEydN-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202401140357.ZT1pEydN-lkp@intel.com/

dtcheck warnings: (new ones prefixed by >>)
   Documentation/devicetree/bindings/net/ti,dp83822.yaml: ti,cfg-dac-minus: missing type definition
>> Documentation/devicetree/bindings/net/ti,dp83822.yaml: ti,cfg-dac-plus: missing type definition
   Documentation/devicetree/bindings/net/snps,dwmac.yaml: mac-mode: missing type definition

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki