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 <redacted>
---
drivers/net/phy/mscc.c | 75 ++++++++++++++++++++++--------------------
1 file changed, 39 insertions(+), 36 deletions(-)
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 <redacted>
---
added in v2
drivers/net/phy/mscc.c | 7 +++----
1 file changed, 3 insertions(+), 4 deletions(-)
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 <redacted>
---
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(-)
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 <redacted>
---
Documentation/devicetree/bindings/net/mscc-phy-vsc8531.txt | 5 -----
1 file changed, 5 deletions(-)
@@ -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).
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 <redacted>
---
.../devicetree/bindings/net/mscc-phy-vsc8531.txt | 16 +++++++++-------
1 file changed, 9 insertions(+), 7 deletions(-)
@@ -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 ----------------------------------------------------------------|
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 <redacted>
---
added in v2
drivers/net/phy/mscc.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
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 <redacted>
---
added in v2
drivers/net/phy/mscc.c | 20 +++++++++++++++++++-
1 file changed, 19 insertions(+), 1 deletion(-)
From: Andrew Lunn <andrew@lunn.ch> Date: 2018-09-03 13:30:29
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
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:
quoted
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
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:
quoted
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
From: Andrew Lunn <andrew@lunn.ch> Date: 2018-09-03 20:06:08
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
Hi Andrew,
On Mon, Sep 03, 2018 at 10:05:54PM +0200, Andrew Lunn wrote:
quoted
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
From: Richard Fitzgerald <rf@opensource.cirrus.com> Date: 2018-09-04 09:22:15
On 04/09/18 08:26, Quentin Schulz wrote:
Hi Andrew,
On Mon, Sep 03, 2018 at 10:05:54PM +0200, Andrew Lunn wrote:
quoted
quoted
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?
quoted
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
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 <redacted>
Entire series applied to net-next.
Please provide a proper "0/N ..." cover letter next time.
Thank you.
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 <redacted>
Entire series applied to net-next.
Thanks!
Please provide a proper "0/N ..." cover letter next time.