[PATCH net-next v2 1/7] net: phy: mscc: factorize code for LEDs mode

[PATCH net-next v2 1/7] net: phy: mscc: factorize code for LEDs mode

[Quentin Schulz]

LEDs modes are set the same way, except they are offset by 4 times the
index of the LED.

Let's factorize all the code so that it's easier to add support for the
4 LEDs of the VSC8584 PHY.

Signed-off-by: Quentin Schulz <quentin.schulz@bootlin.com>
---
 drivers/net/phy/mscc.c | 75 ++++++++++++++++++++++--------------------
 1 file changed, 39 insertions(+), 36 deletions(-)

diff --git a/drivers/net/phy/mscc.c b/drivers/net/phy/mscc.c
index 84ca9ff40ae0..af433f226ef4 100644
--- a/drivers/net/phy/mscc.c
+++ b/drivers/net/phy/mscc.c
@@ -54,9 +54,9 @@ enum rgmii_rx_clock_delay {
 #define HP_AUTO_MDIX_X_OVER_IND_MASK	  0x2000
 
 #define MSCC_PHY_LED_MODE_SEL		  29
-#define LED_1_MODE_SEL_MASK		  0x00F0
-#define LED_0_MODE_SEL_MASK		  0x000F
-#define LED_1_MODE_SEL_POS		  4
+#define LED_MODE_SEL_POS(x)		  ((x) * 4)
+#define LED_MODE_SEL_MASK(x)		  (GENMASK(3, 0) << LED_MODE_SEL_POS(x))
+#define LED_MODE_SEL(x, mode)		  (((mode) << LED_MODE_SEL_POS(x)) & LED_MODE_SEL_MASK(x))
 
 #define MSCC_EXT_PAGE_ACCESS		  31
 #define MSCC_PHY_PAGE_STANDARD		  0x0000 /* Standard registers */
@@ -103,10 +103,11 @@ enum rgmii_rx_clock_delay {
 
 #define DOWNSHIFT_COUNT_MAX		  5
 
+#define MAX_LEDS			  4
 struct vsc8531_private {
 	int rate_magic;
-	u8 led_0_mode;
-	u8 led_1_mode;
+	u8 leds_mode[MAX_LEDS];
+	u8 nleds;
 };
 
 #ifdef CONFIG_OF_MDIO
@@ -140,14 +141,8 @@ static int vsc85xx_led_cntl_set(struct phy_device *phydev,
 
 	mutex_lock(&phydev->lock);
 	reg_val = phy_read(phydev, MSCC_PHY_LED_MODE_SEL);
-	if (led_num) {
-		reg_val &= ~LED_1_MODE_SEL_MASK;
-		reg_val |= (((u16)mode << LED_1_MODE_SEL_POS) &
-			    LED_1_MODE_SEL_MASK);
-	} else {
-		reg_val &= ~LED_0_MODE_SEL_MASK;
-		reg_val |= ((u16)mode & LED_0_MODE_SEL_MASK);
-	}
+	reg_val &= ~LED_MODE_SEL_MASK(led_num);
+	reg_val |= LED_MODE_SEL(led_num, (u16)mode);
 	rc = phy_write(phydev, MSCC_PHY_LED_MODE_SEL, reg_val);
 	mutex_unlock(&phydev->lock);
 
@@ -438,6 +433,27 @@ static int vsc85xx_dt_led_mode_get(struct phy_device *phydev,
 }
 #endif /* CONFIG_OF_MDIO */
 
+static int vsc85xx_dt_led_modes_get(struct phy_device *phydev, u8 *default_mode)
+{
+	struct vsc8531_private *priv = phydev->priv;
+	char led_dt_prop[19];
+	int i, ret;
+
+	for (i = 0; i < priv->nleds; i++) {
+		ret = sprintf(led_dt_prop, "vsc8531,led-%d-mode", i);
+		if (ret < 0)
+			return ret;
+
+		ret = vsc85xx_dt_led_mode_get(phydev, led_dt_prop,
+					      default_mode[i]);
+		if (ret < 0)
+			return ret;
+		priv->leds_mode[i] = ret;
+	}
+
+	return 0;
+}
+
 static int vsc85xx_edge_rate_cntl_set(struct phy_device *phydev, u8 edge_rate)
 {
 	int rc;
@@ -545,7 +561,7 @@ static int vsc85xx_set_tunable(struct phy_device *phydev,
 
 static int vsc85xx_config_init(struct phy_device *phydev)
 {
-	int rc;
+	int rc, i;
 	struct vsc8531_private *vsc8531 = phydev->priv;
 
 	rc = vsc85xx_default_config(phydev);
@@ -560,13 +576,11 @@ static int vsc85xx_config_init(struct phy_device *phydev)
 	if (rc)
 		return rc;
 
-	rc = vsc85xx_led_cntl_set(phydev, 1, vsc8531->led_1_mode);
-	if (rc)
-		return rc;
-
-	rc = vsc85xx_led_cntl_set(phydev, 0, vsc8531->led_0_mode);
-	if (rc)
-		return rc;
+	for (i = 0; i < vsc8531->nleds; i++) {
+		rc = vsc85xx_led_cntl_set(phydev, i, vsc8531->leds_mode[i]);
+		if (rc)
+			return rc;
+	}
 
 	rc = genphy_config_init(phydev);
 
@@ -626,7 +640,8 @@ static int vsc85xx_probe(struct phy_device *phydev)
 {
 	struct vsc8531_private *vsc8531;
 	int rate_magic;
-	int led_mode;
+	u8 default_mode[2] = {VSC8531_LINK_1000_ACTIVITY,
+	   VSC8531_LINK_100_ACTIVITY};
 
 	rate_magic = vsc85xx_edge_rate_magic_get(phydev);
 	if (rate_magic < 0)
@@ -639,21 +654,9 @@ static int vsc85xx_probe(struct phy_device *phydev)
 	phydev->priv = vsc8531;
 
 	vsc8531->rate_magic = rate_magic;
+	vsc8531->nleds = 2;
 
-	/* LED[0] and LED[1] mode */
-	led_mode = vsc85xx_dt_led_mode_get(phydev, "vsc8531,led-0-mode",
-					   VSC8531_LINK_1000_ACTIVITY);
-	if (led_mode < 0)
-		return led_mode;
-	vsc8531->led_0_mode = led_mode;
-
-	led_mode = vsc85xx_dt_led_mode_get(phydev, "vsc8531,led-1-mode",
-					   VSC8531_LINK_100_ACTIVITY);
-	if (led_mode < 0)
-		return led_mode;
-	vsc8531->led_1_mode = led_mode;
-
-	return 0;
+	return vsc85xx_dt_led_modes_get(phydev, default_mode);
 }
 
 /* Microsemi VSC85xx PHYs */
-- 
2.17.1

[PATCH net-next v2 2/7] net: phy: mscc: factorize function for getting LED mode from DT

[Quentin Schulz]

Microsemi PHYs support different LED modes depending on the variant, so
let's factorize the code so we just have to give the supported modes
while the logic behind getting the mode remains identical.

Signed-off-by: Quentin Schulz <quentin.schulz@bootlin.com>
---

added in v2

 drivers/net/phy/mscc.c | 20 +++++++++++++++++++-
 1 file changed, 19 insertions(+), 1 deletion(-)

diff --git a/drivers/net/phy/mscc.c b/drivers/net/phy/mscc.c
index af433f226ef4..aa37e8547cd0 100644
--- a/drivers/net/phy/mscc.c
+++ b/drivers/net/phy/mscc.c
@@ -104,8 +104,24 @@ enum rgmii_rx_clock_delay {
 #define DOWNSHIFT_COUNT_MAX		  5
 
 #define MAX_LEDS			  4
+#define VSC85XX_SUPP_LED_MODES (BIT(VSC8531_LINK_ACTIVITY) | \
+				BIT(VSC8531_LINK_1000_ACTIVITY) | \
+				BIT(VSC8531_LINK_100_ACTIVITY) | \
+				BIT(VSC8531_LINK_10_ACTIVITY) | \
+				BIT(VSC8531_LINK_100_1000_ACTIVITY) | \
+				BIT(VSC8531_LINK_10_1000_ACTIVITY) | \
+				BIT(VSC8531_LINK_10_100_ACTIVITY) | \
+				BIT(VSC8531_DUPLEX_COLLISION) | \
+				BIT(VSC8531_COLLISION) | \
+				BIT(VSC8531_ACTIVITY) | \
+				BIT(VSC8531_AUTONEG_FAULT) | \
+				BIT(VSC8531_SERIAL_MODE) | \
+				BIT(VSC8531_FORCE_LED_OFF) | \
+				BIT(VSC8531_FORCE_LED_ON))
+
 struct vsc8531_private {
 	int rate_magic;
+	u16 supp_led_modes;
 	u8 leds_mode[MAX_LEDS];
 	u8 nleds;
 };
@@ -401,6 +417,7 @@ static int vsc85xx_dt_led_mode_get(struct phy_device *phydev,
 				   char *led,
 				   u8 default_mode)
 {
+	struct vsc8531_private *priv = phydev->priv;
 	struct device *dev = &phydev->mdio.dev;
 	struct device_node *of_node = dev->of_node;
 	u8 led_mode;
@@ -411,7 +428,7 @@ static int vsc85xx_dt_led_mode_get(struct phy_device *phydev,
 
 	led_mode = default_mode;
 	err = of_property_read_u8(of_node, led, &led_mode);
-	if (!err && (led_mode > 15 || led_mode == 7 || led_mode == 11)) {
+	if (!err && !(BIT(led_mode) & priv->supp_led_modes)) {
 		phydev_err(phydev, "DT %s invalid\n", led);
 		return -EINVAL;
 	}
@@ -655,6 +672,7 @@ static int vsc85xx_probe(struct phy_device *phydev)
 
 	vsc8531->rate_magic = rate_magic;
 	vsc8531->nleds = 2;
+	vsc8531->supp_led_modes = VSC85XX_SUPP_LED_MODES;
 
 	return vsc85xx_dt_led_modes_get(phydev, default_mode);
 }
-- 
2.17.1

[PATCH net-next v2 3/7] net: phy: mscc: read 'vsc8531,vddmac' as an u32

[Quentin Schulz]

In the DT binding, it is specified nowhere that 'vsc8531,vddmac' is an
u16, even though it's read as an u16 in the driver.

Let's update the driver to take into consideration that the
'vsc8531,vddmac' property is of the default type u32.

Signed-off-by: Quentin Schulz <quentin.schulz@bootlin.com>
---

added in v2

 drivers/net/phy/mscc.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/net/phy/mscc.c b/drivers/net/phy/mscc.c
index aa37e8547cd0..49dc23117732 100644
--- a/drivers/net/phy/mscc.c
+++ b/drivers/net/phy/mscc.c
@@ -128,7 +128,7 @@ struct vsc8531_private {
 
 #ifdef CONFIG_OF_MDIO
 struct vsc8531_edge_rate_table {
-	u16 vddmac;
+	u32 vddmac;
 	u8 slowdown[8];
 };
 
@@ -387,7 +387,7 @@ static void vsc85xx_wol_get(struct phy_device *phydev,
 static int vsc85xx_edge_rate_magic_get(struct phy_device *phydev)
 {
 	u8 sd;
-	u16 vdd;
+	u32 vdd;
 	int rc, i, j;
 	struct device *dev = &phydev->mdio.dev;
 	struct device_node *of_node = dev->of_node;
@@ -396,7 +396,7 @@ static int vsc85xx_edge_rate_magic_get(struct phy_device *phydev)
 	if (!of_node)
 		return -ENODEV;
 
-	rc = of_property_read_u16(of_node, "vsc8531,vddmac", &vdd);
+	rc = of_property_read_u32(of_node, "vsc8531,vddmac", &vdd);
 	if (rc != 0)
 		vdd = MSCC_VDDMAC_3300;
 
-- 
2.17.1

[PATCH net-next v2 4/7] net: phy: mscc: read 'vsc8531,edge-slowdown' as an u32

[Quentin Schulz]

In the DT binding, it is specified nowhere that 'vsc8531,edge-slowdown'
is an u8, even though it's read as an u8 in the driver.

Let's update the driver to take into consideration that the
'vsc8531,edge-slowdown' property is of the default type u32.

Signed-off-by: Quentin Schulz <quentin.schulz@bootlin.com>
---

added in v2

 drivers/net/phy/mscc.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/drivers/net/phy/mscc.c b/drivers/net/phy/mscc.c
index 49dc23117732..3c7b02bb5c38 100644
--- a/drivers/net/phy/mscc.c
+++ b/drivers/net/phy/mscc.c
@@ -129,7 +129,7 @@ struct vsc8531_private {
 #ifdef CONFIG_OF_MDIO
 struct vsc8531_edge_rate_table {
 	u32 vddmac;
-	u8 slowdown[8];
+	u32 slowdown[8];
 };
 
 static const struct vsc8531_edge_rate_table edge_table[] = {
@@ -386,8 +386,7 @@ static void vsc85xx_wol_get(struct phy_device *phydev,
 #ifdef CONFIG_OF_MDIO
 static int vsc85xx_edge_rate_magic_get(struct phy_device *phydev)
 {
-	u8 sd;
-	u32 vdd;
+	u32 vdd, sd;
 	int rc, i, j;
 	struct device *dev = &phydev->mdio.dev;
 	struct device_node *of_node = dev->of_node;
@@ -400,7 +399,7 @@ static int vsc85xx_edge_rate_magic_get(struct phy_device *phydev)
 	if (rc != 0)
 		vdd = MSCC_VDDMAC_3300;
 
-	rc = of_property_read_u8(of_node, "vsc8531,edge-slowdown", &sd);
+	rc = of_property_read_u32(of_node, "vsc8531,edge-slowdown", &sd);
 	if (rc != 0)
 		sd = 0;
 
-- 
2.17.1

Re: [PATCH net-next v2 4/7] net: phy: mscc: read 'vsc8531,edge-slowdown' as an u32

[Andrew Lunn]

On Mon, Sep 03, 2018 at 10:48:50AM +0200, Quentin Schulz wrote:
> In the DT binding, it is specified nowhere that 'vsc8531,edge-slowdown'
> is an u8, even though it's read as an u8 in the driver.

Hi Quentin

of_property_read_u8() will perform bounds checking. Is that important
here?

	Thanks
		Andrew

Re: [PATCH net-next v2 4/7] net: phy: mscc: read 'vsc8531,edge-slowdown' as an u32

[Quentin Schulz]

[-- Attachment #1: Type: text/plain, Size: 831 bytes --]

Hi Andrew,

On Mon, Sep 03, 2018 at 03:27:56PM +0200, Andrew Lunn wrote:
> On Mon, Sep 03, 2018 at 10:48:50AM +0200, Quentin Schulz wrote:
> > In the DT binding, it is specified nowhere that 'vsc8531,edge-slowdown'
> > is an u8, even though it's read as an u8 in the driver.
> 
> Hi Quentin
> 
> of_property_read_u8() will perform bounds checking. Is that important
> here?
> 

Just to be sure, we're talking here about making sure the value stored
in the DT is not bigger than the specified value (here an u8)? If so,
that isn't the reason why I'm suggesting those two patches.

Without /bits 8/ in the DT property, whatever were the values I put in
the property, I'd always get a 0. So I need to fix it either in the DT
(but Rob does not really like it) or in the driver.

Hope that makes sense.

Quentin

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH net-next v2 4/7] net: phy: mscc: read 'vsc8531,edge-slowdown' as an u32

[Andrew Lunn]

> Just to be sure, we're talking here about making sure the value stored
> in the DT is not bigger than the specified value (here an u8)? If so,
> that isn't the reason why I'm suggesting those two patches.
> 
> Without /bits 8/ in the DT property, whatever were the values I put in
> the property, I'd always get a 0. So I need to fix it either in the DT
> (but Rob does not really like it) or in the driver.

Hi Quentin

Ah, you are fixing endian issues. That was not clear to me from the
commit message.

I don't know enough about how DT stores values in the blob. Is there
type info? Can the DT core tell if a value in the blob is a u8 or a
u32?  It would be nice if it warned about reading a u8 from a u32
blob.

Anyway, this change still removes some bounds checking. Are they
important? Do they need to be added back?

Thanks
	Andrew

Re: [PATCH net-next v2 4/7] net: phy: mscc: read 'vsc8531,edge-slowdown' as an u32

[Quentin Schulz]

[-- Attachment #1: Type: text/plain, Size: 2269 bytes --]

Hi Andrew,

On Mon, Sep 03, 2018 at 10:05:54PM +0200, Andrew Lunn wrote:
> > Just to be sure, we're talking here about making sure the value stored
> > in the DT is not bigger than the specified value (here an u8)? If so,
> > that isn't the reason why I'm suggesting those two patches.
> > 
> > Without /bits 8/ in the DT property, whatever were the values I put in
> > the property, I'd always get a 0. So I need to fix it either in the DT
> > (but Rob does not really like it) or in the driver.
> 
> Hi Quentin
> 
> Ah, you are fixing endian issues. That was not clear to me from the
> commit message.
> 
> I don't know enough about how DT stores values in the blob. Is there
> type info? Can the DT core tell if a value in the blob is a u8 or a
> u32?  It would be nice if it warned about reading a u8 from a u32
> blob.
> 

From my quick research, the lower bound checking is performed by
of_property_read_u* functions but not the higher bound checking (the
internal function of_find_property_value_of_size allows higher bound
checking but it seems it's never used by those functions (see 0 in
sz_max of of_property_read_variable_u*_array)).

sz_max is used by of_property_read_variable_u*_array to copy at a
maximum of sz_max values in the output buffer. If sz_max is 0, it takes
sz_min so it's an array of definite size.
So since sz_max is 0 for all calls to of_property_read_variable_u*_array
by of_property_read_u*_array, we basically know we'll get a buffer of
sz_min values but we don't actually make use of the higher bound
checking of of_find_property_value_of_size.

We could enforce this higher bound check by, instead of setting sz_max
to 0, setting sz_max to sz_min in calls to of_property_read_u*_array.

But I guess there is a reason for sz_max being 0. Rob, Richard (commit
signer of this code) do you know why? Could you explain?

> Anyway, this change still removes some bounds checking. Are they
> important? Do they need to be added back?
> 

The edge-slowdown and the vddmac values are compared against a const
array so we´re fine with those ones.

For the led-X-mode, I added a constant for supported modes that gets
checked when retrieving the DT property. So we´re fine here too.

Quentin

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH net-next v2 4/7] net: phy: mscc: read 'vsc8531,edge-slowdown' as an u32 [UNSCANNED]

[Richard Fitzgerald]

On 04/09/18 08:26, Quentin Schulz wrote:
> Hi Andrew,
> 
> On Mon, Sep 03, 2018 at 10:05:54PM +0200, Andrew Lunn wrote:
>>> Just to be sure, we're talking here about making sure the value stored
>>> in the DT is not bigger than the specified value (here an u8)? If so,
>>> that isn't the reason why I'm suggesting those two patches.
>>>
>>> Without /bits 8/ in the DT property, whatever were the values I put in
>>> the property, I'd always get a 0. So I need to fix it either in the DT
>>> (but Rob does not really like it) or in the driver.
>>
>> Hi Quentin
>>
>> Ah, you are fixing endian issues. That was not clear to me from the
>> commit message.
>>
>> I don't know enough about how DT stores values in the blob. Is there
>> type info? Can the DT core tell if a value in the blob is a u8 or a
>> u32?  It would be nice if it warned about reading a u8 from a u32
>> blob.
>>
> 
>  From my quick research, the lower bound checking is performed by
> of_property_read_u* functions but not the higher bound checking (the
> internal function of_find_property_value_of_size allows higher bound
> checking but it seems it's never used by those functions (see 0 in
> sz_max of of_property_read_variable_u*_array)).
> 
> sz_max is used by of_property_read_variable_u*_array to copy at a
> maximum of sz_max values in the output buffer. If sz_max is 0, it takes
> sz_min so it's an array of definite size.
> So since sz_max is 0 for all calls to of_property_read_variable_u*_array
> by of_property_read_u*_array, we basically know we'll get a buffer of
> sz_min values but we don't actually make use of the higher bound
> checking of of_find_property_value_of_size.
> 

This was the original behaviour of the of_property_read_u*_array functions.
If you look back at the of_property_read_u*_array implementations
before my patch they passed max=0 to of_find_property_value_of_size.

To avoid duplicating code I reimplemented the of_property_read_u*_array
to use the new of_property_read_variable_u*_array hence they pass
sz_max=0 to preserve the original behaviour that max=0 to
of_find_property_value_of_size, so that I didn't break any code that might
depend on that.

> We could enforce this higher bound check by, instead of setting sz_max
> to 0, setting sz_max to sz_min in calls to of_property_read_u*_array.
> 
> But I guess there is a reason for sz_max being 0. Rob, Richard (commit
> signer of this code) do you know why? Could you explain?

> 
>> Anyway, this change still removes some bounds checking. Are they
>> important? Do they need to be added back?
>>
> 
> The edge-slowdown and the vddmac values are compared against a const
> array so we´re fine with those ones.
> 
> For the led-X-mode, I added a constant for supported modes that gets
> checked when retrieving the DT property. So we´re fine here too.
> 
> Quentin
> 

Re: [PATCH net-next v2 4/7] net: phy: mscc: read 'vsc8531,edge-slowdown' as an u32 [UNSCANNED]

[Richard Fitzgerald]

On 04/09/18 08:26, Quentin Schulz wrote:
> Hi Andrew,
> 
> On Mon, Sep 03, 2018 at 10:05:54PM +0200, Andrew Lunn wrote:
>>> Just to be sure, we're talking here about making sure the value stored
>>> in the DT is not bigger than the specified value (here an u8)? If so,
>>> that isn't the reason why I'm suggesting those two patches.
>>>
>>> Without /bits 8/ in the DT property, whatever were the values I put in
>>> the property, I'd always get a 0. So I need to fix it either in the DT
>>> (but Rob does not really like it) or in the driver.
>>
>> Hi Quentin
>>
>> Ah, you are fixing endian issues. That was not clear to me from the
>> commit message.
>>
>> I don't know enough about how DT stores values in the blob. Is there
>> type info? Can the DT core tell if a value in the blob is a u8 or a
>> u32?  It would be nice if it warned about reading a u8 from a u32
>> blob.
>>
> 
>  From my quick research, the lower bound checking is performed by
> of_property_read_u* functions but not the higher bound checking (the
> internal function of_find_property_value_of_size allows higher bound
> checking but it seems it's never used by those functions (see 0 in
> sz_max of of_property_read_variable_u*_array)).
> 
> sz_max is used by of_property_read_variable_u*_array to copy at a
> maximum of sz_max values in the output buffer. If sz_max is 0, it takes
> sz_min so it's an array of definite size.
> So since sz_max is 0 for all calls to of_property_read_variable_u*_array
> by of_property_read_u*_array, we basically know we'll get a buffer of
> sz_min values but we don't actually make use of the higher bound
> checking of of_find_property_value_of_size.
> 

This was the original behaviour of the of_property_read_u*_array functions.
If you look back at the of_property_read_u*_array implementations
before my patch they passed max=0 to of_find_property_value_of_size.

To avoid duplicating code I reimplemented the of_property_read_u*_array
to use the new of_property_read_variable_u*_array hence they pass
sz_max=0 to preserve the original behaviour that max=0 to
of_find_property_value_of_size, so that I didn't break any code that might
depend on that.

> We could enforce this higher bound check by, instead of setting sz_max
> to 0, setting sz_max to sz_min in calls to of_property_read_u*_array.
> 
> But I guess there is a reason for sz_max being 0. Rob, Richard (commit
> signer of this code) do you know why? Could you explain?

> 
>> Anyway, this change still removes some bounds checking. Are they
>> important? Do they need to be added back?
>>
> 
> The edge-slowdown and the vddmac values are compared against a const
> array so we´re fine with those ones.
> 
> For the led-X-mode, I added a constant for supported modes that gets
> checked when retrieving the DT property. So we´re fine here too.
> 
> Quentin
> 

[PATCH net-next v2 5/7] net: phy: mscc: read 'vsc8531,led-%d-mode' as an u32

[Quentin Schulz]

In the DT binding, it is specified nowhere that 'vsc8531,led-%d-mode' is
an u8, even though it's read as an u8 in the driver.

Let's update the driver to take into consideration that the
'vsc8531,led-%d-mode' property is of the default type u32.

Signed-off-by: Quentin Schulz <quentin.schulz@bootlin.com>
---

added in v2 in lieu of the DT binding patch for adding /bits 8/ to the
vsc8531,led-N-mode property

 drivers/net/phy/mscc.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/drivers/net/phy/mscc.c b/drivers/net/phy/mscc.c
index 3c7b02bb5c38..2d9676d78d3f 100644
--- a/drivers/net/phy/mscc.c
+++ b/drivers/net/phy/mscc.c
@@ -122,7 +122,7 @@ enum rgmii_rx_clock_delay {
 struct vsc8531_private {
 	int rate_magic;
 	u16 supp_led_modes;
-	u8 leds_mode[MAX_LEDS];
+	u32 leds_mode[MAX_LEDS];
 	u8 nleds;
 };
 
@@ -414,19 +414,19 @@ static int vsc85xx_edge_rate_magic_get(struct phy_device *phydev)
 
 static int vsc85xx_dt_led_mode_get(struct phy_device *phydev,
 				   char *led,
-				   u8 default_mode)
+				   u32 default_mode)
 {
 	struct vsc8531_private *priv = phydev->priv;
 	struct device *dev = &phydev->mdio.dev;
 	struct device_node *of_node = dev->of_node;
-	u8 led_mode;
+	u32 led_mode;
 	int err;
 
 	if (!of_node)
 		return -ENODEV;
 
 	led_mode = default_mode;
-	err = of_property_read_u8(of_node, led, &led_mode);
+	err = of_property_read_u32(of_node, led, &led_mode);
 	if (!err && !(BIT(led_mode) & priv->supp_led_modes)) {
 		phydev_err(phydev, "DT %s invalid\n", led);
 		return -EINVAL;
@@ -449,7 +449,8 @@ static int vsc85xx_dt_led_mode_get(struct phy_device *phydev,
 }
 #endif /* CONFIG_OF_MDIO */
 
-static int vsc85xx_dt_led_modes_get(struct phy_device *phydev, u8 *default_mode)
+static int vsc85xx_dt_led_modes_get(struct phy_device *phydev,
+				    u32 *default_mode)
 {
 	struct vsc8531_private *priv = phydev->priv;
 	char led_dt_prop[19];
@@ -656,7 +657,7 @@ static int vsc85xx_probe(struct phy_device *phydev)
 {
 	struct vsc8531_private *vsc8531;
 	int rate_magic;
-	u8 default_mode[2] = {VSC8531_LINK_1000_ACTIVITY,
+	u32 default_mode[2] = {VSC8531_LINK_1000_ACTIVITY,
 	   VSC8531_LINK_100_ACTIVITY};
 
 	rate_magic = vsc85xx_edge_rate_magic_get(phydev);
-- 
2.17.1

[PATCH net-next v2 6/7] dt-bindings: net: phy: mscc: vsc8531: remove compatible from required properties

[Quentin Schulz]

Compatible isn't a required property for PHYs so let's remove it from
the binding DT of the VSC8531 PHYs.

Acked-by: Rob Herring <robh@kernel.org>
Reviewed-by: Andrew Lunn <andrew@lunn.ch>
Signed-off-by: Quentin Schulz <quentin.schulz@bootlin.com>
---
 Documentation/devicetree/bindings/net/mscc-phy-vsc8531.txt | 5 -----
 1 file changed, 5 deletions(-)

diff --git a/Documentation/devicetree/bindings/net/mscc-phy-vsc8531.txt b/Documentation/devicetree/bindings/net/mscc-phy-vsc8531.txt
index 0eedabe22cc3..664d9d0543fc 100644
--- a/Documentation/devicetree/bindings/net/mscc-phy-vsc8531.txt
+++ b/Documentation/devicetree/bindings/net/mscc-phy-vsc8531.txt
@@ -1,10 +1,5 @@
 * Microsemi - vsc8531 Giga bit ethernet phy
 
-Required properties:
-- compatible	: Should contain phy id as "ethernet-phy-idAAAA.BBBB"
-		  The PHY device uses the binding described in
-		  Documentation/devicetree/bindings/net/phy.txt
-
 Optional properties:
 - vsc8531,vddmac	: The vddmac in mV. Allowed values is listed
 			  in the first row of Table 1 (below).
-- 
2.17.1

Re: [PATCH net-next v2 6/7] dt-bindings: net: phy: mscc: vsc8531: remove compatible from required properties

[Andrew Lunn]

On Mon, Sep 03, 2018 at 10:48:52AM +0200, Quentin Schulz wrote:
> Compatible isn't a required property for PHYs so let's remove it from
> the binding DT of the VSC8531 PHYs.

Hi Quentin

It is actually a optional property. So you could move it into that
section of the binding documentation. But removing it is also O.K.

	Andrew

Re: [PATCH net-next v2 6/7] dt-bindings: net: phy: mscc: vsc8531: remove compatible from required properties

[Quentin Schulz]

[-- Attachment #1: Type: text/plain, Size: 815 bytes --]

Hi Andrew,

On Mon, Sep 03, 2018 at 03:30:20PM +0200, Andrew Lunn wrote:
> On Mon, Sep 03, 2018 at 10:48:52AM +0200, Quentin Schulz wrote:
> > Compatible isn't a required property for PHYs so let's remove it from
> > the binding DT of the VSC8531 PHYs.
> 
> Hi Quentin
> 
> It is actually a optional property. So you could move it into that
> section of the binding documentation. But removing it is also O.K.
> 

It's a property that is already defined in
Documentation/devicetree/bindings/net/phy.txt

I don't think it's actually useful to redefine it there (on the same
principle that we don't redefine/re-explain the reg property).

What do we do? Do I put it in optional properties and refer to the
aforementioned doc or do I just remove it as it's done in this patch?

Thanks,
Quentin

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH net-next v2 6/7] dt-bindings: net: phy: mscc: vsc8531: remove compatible from required properties

[Andrew Lunn]

> What do we do? Do I put it in optional properties and refer to the
> aforementioned doc or do I just remove it as it's done in this patch?

Just remove it.

     Andrew

[PATCH net-next v2 7/7] dt-bindings: net: phy: mscc: vsc8531: factorize vsc8531,led-N-mode

[Quentin Schulz]

VSC8584 supports 4 LEDs while VSC8531 only supports 2. Let's factorize
the documentation for LED mode properties and give the 4 default values
(the first two being shared between VSC8531 and VSC8584).

Reviewed-by: Rob Herring <robh@kernel.org>
Signed-off-by: Quentin Schulz <quentin.schulz@bootlin.com>
---
 .../devicetree/bindings/net/mscc-phy-vsc8531.txt | 16 +++++++++-------
 1 file changed, 9 insertions(+), 7 deletions(-)

diff --git a/Documentation/devicetree/bindings/net/mscc-phy-vsc8531.txt b/Documentation/devicetree/bindings/net/mscc-phy-vsc8531.txt
index 664d9d0543fc..5ff37c68c941 100644
--- a/Documentation/devicetree/bindings/net/mscc-phy-vsc8531.txt
+++ b/Documentation/devicetree/bindings/net/mscc-phy-vsc8531.txt
@@ -22,14 +22,16 @@ Optional properties:
 			  'vddmac'.
 			  Default value is 0%.
 			  Ref: Table:1 - Edge rate change (below).
-- vsc8531,led-0-mode	: LED mode. Specify how the LED[0] should behave.
-			  Allowed values are define in
+- vsc8531,led-[N]-mode	: LED mode. Specify how the LED[N] should behave.
+			  N depends on the number of LEDs supported by a
+			  PHY.
+			  Allowed values are defined in
 			  "include/dt-bindings/net/mscc-phy-vsc8531.h".
-			  Default value is VSC8531_LINK_1000_ACTIVITY (1).
-- vsc8531,led-1-mode	: LED mode. Specify how the LED[1] should behave.
-			  Allowed values are define in
-			  "include/dt-bindings/net/mscc-phy-vsc8531.h".
-			  Default value is VSC8531_LINK_100_ACTIVITY (2).
+			  Default values are VSC8531_LINK_1000_ACTIVITY (1),
+			  VSC8531_LINK_100_ACTIVITY (2),
+			  VSC8531_LINK_ACTIVITY (0) and
+			  VSC8531_DUPLEX_COLLISION (8).
+
 
 Table: 1 - Edge rate change
 ----------------------------------------------------------------|
-- 
2.17.1

Re: [PATCH net-next v2 1/7] net: phy: mscc: factorize code for LEDs mode

[David Miller]

From: Quentin Schulz <quentin.schulz@bootlin.com>
Date: Mon,  3 Sep 2018 10:48:47 +0200

> LEDs modes are set the same way, except they are offset by 4 times the
> index of the LED.
> 
> Let's factorize all the code so that it's easier to add support for the
> 4 LEDs of the VSC8584 PHY.
> 
> Signed-off-by: Quentin Schulz <quentin.schulz@bootlin.com>

Entire series applied to net-next.

Please provide a proper "0/N ..." cover letter next time.

Thank you.

Re: [PATCH net-next v2 1/7] net: phy: mscc: factorize code for LEDs mode

[Quentin Schulz]

[-- Attachment #1: Type: text/plain, Size: 611 bytes --]

Hi David,

On Tue, Sep 04, 2018 at 10:48:27AM -0700, David Miller wrote:
> From: Quentin Schulz <quentin.schulz@bootlin.com>
> Date: Mon,  3 Sep 2018 10:48:47 +0200
> 
> > LEDs modes are set the same way, except they are offset by 4 times the
> > index of the LED.
> > 
> > Let's factorize all the code so that it's easier to add support for the
> > 4 LEDs of the VSC8584 PHY.
> > 
> > Signed-off-by: Quentin Schulz <quentin.schulz@bootlin.com>
> 
> Entire series applied to net-next.
> 

Thanks!

> Please provide a proper "0/N ..." cover letter next time.
> 

Sure, will do.

Quentin

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]