[PATCH v2 0/3] net: dp83867: add new output-impedance DT binding

[PATCH v2 0/3] net: dp83867: add new output-impedance DT binding

[Steffen Trumtrar]

Currently the dp83867 supports setting the io impedance to minimum,
maximum, default or a value from a nvmem cell. In situations where there
is no backend for a nvmem cell, the value can not be set to anything but
minimum, maximum or default.

Add support for a ti,output-impedance-ohms binding, allowing to specify
the impedance via the devicetree. The existing boolean bindings for
minimum and maximum take precedence over this new binding. The nvmem
consumer still takes lowest precedence.

Signed-off-by: Steffen Trumtrar <s.trumtrar@pengutronix.de>
---
Changes in v2:
- change 'u8 *val' to 'u32 *val'
- fix logic in driver
- update description of new binding
- rename ti,output-impedance -> ti,output-impedance-ohms
- make mutual exclusivity explicit in binding
- Link to v1: https://patch.msgid.link/20260804-v7-2-topic-dp83867-v1-0-1ca79eb54eef@pengutronix.de

To: Andrew Lunn <andrew+netdev@lunn.ch>
To: "David S. Miller" <davem@davemloft.net>
To: Eric Dumazet <edumazet@google.com>
To: Jakub Kicinski <kuba@kernel.org>
To: Paolo Abeni <pabeni@redhat.com>
To: Rob Herring <robh@kernel.org>
To: Krzysztof Kozlowski <krzk+dt@kernel.org>
To: Conor Dooley <conor+dt@kernel.org>
To: Andrew Davis <afd@ti.com>
To: Andrew Lunn <andrew@lunn.ch>
To: Heiner Kallweit <hkallweit1@gmail.com>
To: Russell King <linux@armlinux.org.uk>
Cc: netdev@vger.kernel.org
Cc: devicetree@vger.kernel.org
Cc: linux-kernel@vger.kernel.org

---
Steffen Trumtrar (3):
      dt-bindings: net: dp83867: make exclusivity explicit
      dt-bindings: dp83867: add binding for output-impedance
      net: phy: dp83867: support setting output-impedance

 .../devicetree/bindings/net/ti,dp83867.yaml        | 26 +++++++++++++++++++---
 drivers/net/phy/dp83867.c                          | 18 +++++++++++----
 2 files changed, 37 insertions(+), 7 deletions(-)
---
base-commit: 495561a136eaf585a305d36fc3affdba94aaf28e
change-id: 20260804-v7-2-topic-dp83867-e73a84c37016

Best regards,
--  
Steffen Trumtrar <s.trumtrar@pengutronix.de>

[PATCH v2 1/3] dt-bindings: net: dp83867: make exclusivity explicit

[Steffen Trumtrar]

The properties nvmem-cells and ti,{min,max}-output-impedance are
mutually exclusive as already noted in the ti,max-output-impedance
description.

As this is only stated in the description and not made explicit in the
schema, the binding checker will not catch if this condition is ignored.

Make it explicit in the schema, that nmvem-cells can not be used with
ti,min-output-impedance or ti,max-output-impedance.

Signed-off-by: Steffen Trumtrar <s.trumtrar@pengutronix.de>
---
 Documentation/devicetree/bindings/net/ti,dp83867.yaml | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/Documentation/devicetree/bindings/net/ti,dp83867.yaml b/Documentation/devicetree/bindings/net/ti,dp83867.yaml
index 4bc1f98fd9fe5..7a6fe516bab27 100644
--- a/Documentation/devicetree/bindings/net/ti,dp83867.yaml
+++ b/Documentation/devicetree/bindings/net/ti,dp83867.yaml
@@ -7,9 +7,6 @@ $schema: http://devicetree.org/meta-schemas/core.yaml#
 
 title: TI DP83867 ethernet PHY
 
-allOf:
-  - $ref: ethernet-controller.yaml#
-
 maintainers:
   - Andrew Davis <afd@ti.com>
 
@@ -118,6 +115,13 @@ properties:
       Transmitt FIFO depth- see dt-bindings/net/ti-dp83867.h for applicable
       values.
 
+allOf:
+  - $ref: ethernet-controller.yaml#
+  - not:
+      required: [nvmem-cells, "ti,min-output-impedance"]
+  - not:
+      required: [nvmem-cells, "ti,max-output-impedance"]
+
 required:
   - reg
 

-- 
2.54.0

Re: [PATCH v2 1/3] dt-bindings: net: dp83867: make exclusivity explicit

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Low] Spelling error in the commit message ('nmvem-cells').
--

commit aca94911b8a130018e97c8fe36235a2d9d984b2d
Author: Steffen Trumtrar <s.trumtrar@pengutronix.de>

dt-bindings: net: dp83867: make exclusivity explicit

This commit updates the TI DP83867 PHY binding schema to explicitly enforce
the mutual exclusivity between the nvmem-cells property and the output
impedance properties.

> Make it explicit in the schema, that nmvem-cells can not be used with
> ti,min-output-impedance or ti,max-output-impedance.

[Severity: Low]
This isn't a bug, but there is a typo (nmvem-cells) in the commit message.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260826-v7-2-topic-dp83867-v2-0-a97b4cec6e93@pengutronix.de?part=1

[PATCH v2 2/3] dt-bindings: dp83867: add binding for output-impedance

[Steffen Trumtrar]

Currently, the output-impedance can only be set to default, minimum or
maximum or via a nvmem cell.
In cases where there is no backend for a nvmem cell, a value can not be
set.

Add a DT binding, 'ti,output-impedance-ohms', for specifying the
output-impedance via a fixed value. The existing boolean values for
minimum and maximum still take precedence over the new value.

Signed-off-by: Steffen Trumtrar <s.trumtrar@pengutronix.de>
---
 Documentation/devicetree/bindings/net/ti,dp83867.yaml | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/Documentation/devicetree/bindings/net/ti,dp83867.yaml b/Documentation/devicetree/bindings/net/ti,dp83867.yaml
index 7a6fe516bab27..49fafe3ce6f47 100644
--- a/Documentation/devicetree/bindings/net/ti,dp83867.yaml
+++ b/Documentation/devicetree/bindings/net/ti,dp83867.yaml
@@ -38,6 +38,16 @@ properties:
     items:
       - const: io_impedance_ctrl
 
+  ti,output-impedance-ohms:
+    description: |
+      MAC Interface Impedance control to set the programmable output impedance.
+      Valid range is 0x00 to 0x1f (32 steps).
+      - 0x1f = 35 ohms (minimum impedance)
+      - 0x00 = 70 ohms (maximum impedance)
+      - Step size varies with process.
+    minimum: 0
+    maximum: 31
+
   ti,min-output-impedance:
     type: boolean
     description: |
@@ -121,6 +131,12 @@ allOf:
       required: [nvmem-cells, "ti,min-output-impedance"]
   - not:
       required: [nvmem-cells, "ti,max-output-impedance"]
+  - not:
+      required: [nvmem-cells, "ti,output-impedance-ohms"]
+  - not:
+      required: ["ti,output-impedance-ohms", "ti,min-output-impedance"]
+  - not:
+      required: ["ti,output-impedance-ohms", "ti,max-output-impedance"]
 
 required:
   - reg

-- 
2.54.0

Re: [PATCH v2 2/3] dt-bindings: dp83867: add binding for output-impedance

[Conor Dooley]

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

On Wed, Aug 26, 2026 at 02:48:40PM +0200, Steffen Trumtrar wrote:
> Currently, the output-impedance can only be set to default, minimum or
> maximum or via a nvmem cell.
> In cases where there is no backend for a nvmem cell, a value can not be
> set.
> 
> Add a DT binding, 'ti,output-impedance-ohms', for specifying the
> output-impedance via a fixed value. The existing boolean values for
> minimum and maximum still take precedence over the new value.
> 
> Signed-off-by: Steffen Trumtrar <s.trumtrar@pengutronix.de>
> ---
>  Documentation/devicetree/bindings/net/ti,dp83867.yaml | 16 ++++++++++++++++
>  1 file changed, 16 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/net/ti,dp83867.yaml b/Documentation/devicetree/bindings/net/ti,dp83867.yaml
> index 7a6fe516bab27..49fafe3ce6f47 100644
> --- a/Documentation/devicetree/bindings/net/ti,dp83867.yaml
> +++ b/Documentation/devicetree/bindings/net/ti,dp83867.yaml
> @@ -38,6 +38,16 @@ properties:
>      items:
>        - const: io_impedance_ctrl
>  
> +  ti,output-impedance-ohms:
> +    description: |
> +      MAC Interface Impedance control to set the programmable output impedance.
> +      Valid range is 0x00 to 0x1f (32 steps).
> +      - 0x1f = 35 ohms (minimum impedance)
> +      - 0x00 = 70 ohms (maximum impedance)
> +      - Step size varies with process.
> +    minimum: 0
> +    maximum: 31

Your property is named in ohms, but not implemented in ohms.
pw-bot: changes-requested

Can't you just achieve the same result using the existing max property?
I don't get the use case where you specify a max, but then don't want to
use that max. Just set the max to what you actually want?

Thanks,
Conor.

> +
>    ti,min-output-impedance:
>      type: boolean
>      description: |
> @@ -121,6 +131,12 @@ allOf:
>        required: [nvmem-cells, "ti,min-output-impedance"]
>    - not:
>        required: [nvmem-cells, "ti,max-output-impedance"]
> +  - not:
> +      required: [nvmem-cells, "ti,output-impedance-ohms"]
> +  - not:
> +      required: ["ti,output-impedance-ohms", "ti,min-output-impedance"]
> +  - not:
> +      required: ["ti,output-impedance-ohms", "ti,max-output-impedance"]
>  
>  required:
>    - reg
> 
> -- 
> 2.54.0
> 

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

Re: [PATCH v2 2/3] dt-bindings: dp83867: add binding for output-impedance

[Steffen Trumtrar]

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

On 2026-08-26 at 17:01 +01, Conor Dooley <conor@kernel.org> wrote:

> On Wed, Aug 26, 2026 at 02:48:40PM +0200, Steffen Trumtrar wrote:
> > Currently, the output-impedance can only be set to default, minimum or
> > maximum or via a nvmem cell.
> > In cases where there is no backend for a nvmem cell, a value can not be
> > set.
> > 
> > Add a DT binding, 'ti,output-impedance-ohms', for specifying the
> > output-impedance via a fixed value. The existing boolean values for
> > minimum and maximum still take precedence over the new value.
> > 
> > Signed-off-by: Steffen Trumtrar <s.trumtrar@pengutronix.de>
> > ---
> >  Documentation/devicetree/bindings/net/ti,dp83867.yaml | 16 ++++++++++++++++
> >  1 file changed, 16 insertions(+)
> > 
> > diff --git a/Documentation/devicetree/bindings/net/ti,dp83867.yaml b/Documentation/devicetree/bindings/net/ti,dp83867.yaml
> > index 7a6fe516bab27..49fafe3ce6f47 100644
> > --- a/Documentation/devicetree/bindings/net/ti,dp83867.yaml
> > +++ b/Documentation/devicetree/bindings/net/ti,dp83867.yaml
> > @@ -38,6 +38,16 @@ properties:
> >      items:
> >        - const: io_impedance_ctrl
> >  
> > +  ti,output-impedance-ohms:
> > +    description: |
> > +      MAC Interface Impedance control to set the programmable output impedance.
> > +      Valid range is 0x00 to 0x1f (32 steps).
> > +      - 0x1f = 35 ohms (minimum impedance)
> > +      - 0x00 = 70 ohms (maximum impedance)
> > +      - Step size varies with process.
> > +    minimum: 0
> > +    maximum: 31
> 
> Your property is named in ohms, but not implemented in ohms.

So without the -ohms?

> pw-bot: changes-requested
> 
> Can't you just achieve the same result using the existing max property?

The max property is a boolean which just says "use 70ohms".

> I don't get the use case where you specify a max, but then don't want to
> use that max. Just set the max to what you actually want?

That's why I added the exclusion in the binding, because having nvmem-cells and/or any of the other output-impedance bindings doesn't make any sense.

Best regards,
Steffen

-- 
Pengutronix e.K.                | Dipl.-Inform. Steffen Trumtrar |
Steuerwalder Str. 21            | https://www.pengutronix.de/    |
31137 Hildesheim, Germany       | Phone: +49-5121-206917-0       |
Amtsgericht Hildesheim, HRA 2686| Fax:   +49-5121-206917-5555    |

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

Re: [PATCH v2 2/3] dt-bindings: dp83867: add binding for output-impedance

[Andrew Lunn]

On Thu, Aug 27, 2026 at 10:04:07AM +0200, Steffen Trumtrar wrote:
> On 2026-08-26 at 17:01 +01, Conor Dooley <conor@kernel.org> wrote:
> 
> > On Wed, Aug 26, 2026 at 02:48:40PM +0200, Steffen Trumtrar wrote:
> > > Currently, the output-impedance can only be set to default, minimum or
> > > maximum or via a nvmem cell.
> > > In cases where there is no backend for a nvmem cell, a value can not be
> > > set.
> > > > Add a DT binding, 'ti,output-impedance-ohms', for specifying the
> > > output-impedance via a fixed value. The existing boolean values for
> > > minimum and maximum still take precedence over the new value.
> > > > Signed-off-by: Steffen Trumtrar <s.trumtrar@pengutronix.de>
> > > ---
> > >  Documentation/devicetree/bindings/net/ti,dp83867.yaml | 16 ++++++++++++++++
> > >  1 file changed, 16 insertions(+)
> > > > diff --git a/Documentation/devicetree/bindings/net/ti,dp83867.yaml
> > b/Documentation/devicetree/bindings/net/ti,dp83867.yaml
> > > index 7a6fe516bab27..49fafe3ce6f47 100644
> > > --- a/Documentation/devicetree/bindings/net/ti,dp83867.yaml
> > > +++ b/Documentation/devicetree/bindings/net/ti,dp83867.yaml
> > > @@ -38,6 +38,16 @@ properties:
> > >      items:
> > >        - const: io_impedance_ctrl
> > >  > +  ti,output-impedance-ohms:
> > > +    description: |
> > > +      MAC Interface Impedance control to set the programmable output impedance.
> > > +      Valid range is 0x00 to 0x1f (32 steps).
> > > +      - 0x1f = 35 ohms (minimum impedance)
> > > +      - 0x00 = 70 ohms (maximum impedance)
> > > +      - Step size varies with process.
> > > +    minimum: 0
> > > +    maximum: 31
> > 
> > Your property is named in ohms, but not implemented in ohms.
> 
> So without the -ohms?

No. DT uses real unit, Ohms, Meters, Volts, Degrees Centigrade, Seconds
etc.

Have the driver do the conversion from Ohms to the magical register
value.

	Andrew

Re: [PATCH v2 2/3] dt-bindings: dp83867: add binding for output-impedance

[Steffen Trumtrar]

On 2026-08-27 at 14:47 +02, Andrew Lunn <andrew@lunn.ch> wrote:

> On Thu, Aug 27, 2026 at 10:04:07AM +0200, Steffen Trumtrar wrote:
> > On 2026-08-26 at 17:01 +01, Conor Dooley <conor@kernel.org> wrote:
> > 
> > > On Wed, Aug 26, 2026 at 02:48:40PM +0200, Steffen Trumtrar wrote:
> > > > Currently, the output-impedance can only be set to default, minimum or
> > > > maximum or via a nvmem cell.
> > > > In cases where there is no backend for a nvmem cell, a value can not be
> > > > set.
> > > > > Add a DT binding, 'ti,output-impedance-ohms', for specifying the
> > > > output-impedance via a fixed value. The existing boolean values for
> > > > minimum and maximum still take precedence over the new value.
> > > > > Signed-off-by: Steffen Trumtrar <s.trumtrar@pengutronix.de>
> > > > ---
> > > >  Documentation/devicetree/bindings/net/ti,dp83867.yaml | 16 ++++++++++++++++
> > > >  1 file changed, 16 insertions(+)
> > > > > diff --git a/Documentation/devicetree/bindings/net/ti,dp83867.yaml
> > > b/Documentation/devicetree/bindings/net/ti,dp83867.yaml
> > > > index 7a6fe516bab27..49fafe3ce6f47 100644
> > > > --- a/Documentation/devicetree/bindings/net/ti,dp83867.yaml
> > > > +++ b/Documentation/devicetree/bindings/net/ti,dp83867.yaml
> > > > @@ -38,6 +38,16 @@ properties:
> > > >      items:
> > > >        - const: io_impedance_ctrl
> > > >  > +  ti,output-impedance-ohms:
> > > > +    description: |
> > > > +      MAC Interface Impedance control to set the programmable output impedance.
> > > > +      Valid range is 0x00 to 0x1f (32 steps).
> > > > +      - 0x1f = 35 ohms (minimum impedance)
> > > > +      - 0x00 = 70 ohms (maximum impedance)
> > > > +      - Step size varies with process.
> > > > +    minimum: 0
> > > > +    maximum: 31
> > > 
> > > Your property is named in ohms, but not implemented in ohms.
> > 
> > So without the -ohms?
> 
> No. DT uses real unit, Ohms, Meters, Volts, Degrees Centigrade, Seconds
> etc.
> 
> Have the driver do the conversion from Ohms to the magical register
> value.

Fair enough and I agree, but the datasheet says:

Output impedance approximate range from 35-70 Ohm in 32 steps. Lowest being 11111 and highest being 00000. Range and step size will vary with process.

I mean, the difference will be miniscule, but the binding can't specify the exact ohm value, because it is not guaranteed to be correct. There are more values than steps and even the range is only an approximate range.

So, just allow values between 35 and 70 in the binding and then convert that value to something near the truth in the driver?


Best regards,
Steffen

-- 
Pengutronix e.K.                | Dipl.-Inform. Steffen Trumtrar |
Steuerwalder Str. 21            | https://www.pengutronix.de/    |
31137 Hildesheim, Germany       | Phone: +49-5121-206917-0       |
Amtsgericht Hildesheim, HRA 2686| Fax:   +49-5121-206917-5555    |

Re: [PATCH v2 2/3] dt-bindings: dp83867: add binding for output-impedance

[Andrew Lunn]

> So, just allow values between 35 and 70 in the binding and then
> convert that value to something near the truth in the driver?

Yes. And you can add a comment that the values are not exact, due to
process issues.

	Andrew

Re: [PATCH v2 2/3] dt-bindings: dp83867: add binding for output-impedance

[Conor Dooley]

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

On Thu, Aug 27, 2026 at 10:04:07AM +0200, Steffen Trumtrar wrote:
> On 2026-08-26 at 17:01 +01, Conor Dooley <conor@kernel.org> wrote:
> 
> > On Wed, Aug 26, 2026 at 02:48:40PM +0200, Steffen Trumtrar wrote:
> > > Currently, the output-impedance can only be set to default, minimum or
> > > maximum or via a nvmem cell.
> > > In cases where there is no backend for a nvmem cell, a value can not be
> > > set.
> > > > Add a DT binding, 'ti,output-impedance-ohms', for specifying the
> > > output-impedance via a fixed value. The existing boolean values for
> > > minimum and maximum still take precedence over the new value.
> > > > Signed-off-by: Steffen Trumtrar <s.trumtrar@pengutronix.de>
> > > ---
> > >  Documentation/devicetree/bindings/net/ti,dp83867.yaml | 16 ++++++++++++++++
> > >  1 file changed, 16 insertions(+)
> > > > diff --git a/Documentation/devicetree/bindings/net/ti,dp83867.yaml
> > b/Documentation/devicetree/bindings/net/ti,dp83867.yaml
> > > index 7a6fe516bab27..49fafe3ce6f47 100644
> > > --- a/Documentation/devicetree/bindings/net/ti,dp83867.yaml
> > > +++ b/Documentation/devicetree/bindings/net/ti,dp83867.yaml
> > > @@ -38,6 +38,16 @@ properties:
> > >      items:
> > >        - const: io_impedance_ctrl
> > >  > +  ti,output-impedance-ohms:
> > > +    description: |
> > > +      MAC Interface Impedance control to set the programmable output impedance.
> > > +      Valid range is 0x00 to 0x1f (32 steps).
> > > +      - 0x1f = 35 ohms (minimum impedance)
> > > +      - 0x00 = 70 ohms (maximum impedance)
> > > +      - Step size varies with process.
> > > +    minimum: 0
> > > +    maximum: 31
> > 
> > Your property is named in ohms, but not implemented in ohms.
> 
> So without the -ohms?
> 
> > pw-bot: changes-requested
> > 
> > Can't you just achieve the same result using the existing max property?
> 
> The max property is a boolean which just says "use 70ohms".

Ah, I somehow missed that. I thought it was a uint32...

> 
> > I don't get the use case where you specify a max, but then don't want to
> > use that max. Just set the max to what you actually want?

And this question was based on that assumption, so while your answer
makes no sense that's because the question didn't. Sorry bout that.

> 
> That's why I added the exclusion in the binding, because having nvmem-cells
> and/or any of the other output-impedance bindings doesn't make any sense.

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

Re: [PATCH v2 2/3] dt-bindings: dp83867: add binding for output-impedance

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] The property `ti,output-impedance-ohms` expects a register value (0-31) instead of an actual resistance in ohms, violating standard devicetree unit suffix rules.
--

commit baf7480d85f53a0dc882f36f231a4b891961b193
Author: Steffen Trumtrar <s.trumtrar@pengutronix.de>

dt-bindings: dp83867: add binding for output-impedance

This commit adds a new devicetree binding, ti,output-impedance-ohms, to allow
specifying the output impedance for the DP83867 PHY via a fixed value. The
existing boolean values for minimum and maximum still take precedence over the
new value.

> diff --git a/Documentation/devicetree/bindings/net/ti,dp83867.yaml b/Documentation/devicetree/bindings/net/ti,dp83867.yaml
> --- a/Documentation/devicetree/bindings/net/ti,dp83867.yaml
> +++ b/Documentation/devicetree/bindings/net/ti,dp83867.yaml
> @@ -38,6 +38,16 @@ properties:
>      items:
>        - const: io_impedance_ctrl
>  
> +  ti,output-impedance-ohms:
> +    description: |
> +      MAC Interface Impedance control to set the programmable output impedance.
> +      Valid range is 0x00 to 0x1f (32 steps).
> +      - 0x1f = 35 ohms (minimum impedance)
> +      - 0x00 = 70 ohms (maximum impedance)
> +      - Step size varies with process.
> +    minimum: 0
> +    maximum: 31

[Severity: Medium]
Does the property name ti,output-impedance-ohms violate devicetree unit suffix
conventions by accepting raw register values (0-31) instead of actual ohms?

According to standard devicetree property rules, the -ohms suffix dictates
that the value must be specified in Ohms. However, the schema restricts this
property to raw register step values (0 to 31, where 31 configures 35 ohms
and 0 configures 70 ohms).

This mismatch could actively mislead devicetree authors. A developer might
provide a value like 35 which will be rejected by the schema as it exceeds
the maximum of 31, or dangerously configure 31 believing it sets 31 ohms,
but actually configuring 35 ohms.

Could this property be renamed to omit the -ohms suffix, or could the driver
and binding be updated to accept the actual resistance in ohms?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260826-v7-2-topic-dp83867-v2-0-a97b4cec6e93@pengutronix.de?part=2

[PATCH v2 3/3] net: phy: dp83867: support setting output-impedance

[Steffen Trumtrar]

Currently the dp83867 supports setting the io impedance to minimum,
maximum, default or a value from a nvmem cell. In situations where there
is no backend for a nvmem cell, the value can not be set to anything but
minimum, maximum or default.

Add support for the ti,output-impedance-ohms binding, allowing to
specify the impedance via the devicetree. The existing boolean bindings
for minimum and maximum take precedence over this new binding. The nvmem
consumer still takes lowest precedence.

Signed-off-by: Steffen Trumtrar <s.trumtrar@pengutronix.de>
---
 drivers/net/phy/dp83867.c | 18 ++++++++++++++----
 1 file changed, 14 insertions(+), 4 deletions(-)

diff --git a/drivers/net/phy/dp83867.c b/drivers/net/phy/dp83867.c
index 88255e92b4cdb..973b70f11c45a 100644
--- a/drivers/net/phy/dp83867.c
+++ b/drivers/net/phy/dp83867.c
@@ -531,7 +531,8 @@ static int dp83867_of_init_io_impedance(struct phy_device *phydev)
 	struct device *dev = &phydev->mdio.dev;
 	struct device_node *of_node = dev->of_node;
 	struct nvmem_cell *cell;
-	u8 *buf, val;
+	u8 *buf;
+	u32 val;
 	int ret;
 
 	cell = of_nvmem_cell_get(of_node, "io_impedance_ctrl");
@@ -542,12 +543,21 @@ static int dp83867_of_init_io_impedance(struct phy_device *phydev)
 						"failed to get nvmem cell io_impedance_ctrl\n");
 
 		/* If no nvmem cell, check for the boolean properties. */
-		if (of_property_read_bool(of_node, "ti,max-output-impedance"))
+		if (of_property_read_bool(of_node, "ti,max-output-impedance")) {
 			dp83867->io_impedance = DP83867_IO_MUX_CFG_IO_IMPEDANCE_MAX;
-		else if (of_property_read_bool(of_node, "ti,min-output-impedance"))
+		} else if (of_property_read_bool(of_node, "ti,min-output-impedance")) {
 			dp83867->io_impedance = DP83867_IO_MUX_CFG_IO_IMPEDANCE_MIN;
-		else
+		} else if (of_property_read_u32(of_node, "ti,output-impedance-ohms", &val) == 0) {
+			val = val & DP83867_IO_MUX_CFG_IO_IMPEDANCE_MASK;
+			if (val > DP83867_IO_MUX_CFG_IO_IMPEDANCE_MIN ||
+			    val < DP83867_IO_MUX_CFG_IO_IMPEDANCE_MAX) {
+				phydev_err(phydev, "'ti,output-impedance-ohms' contents out of range\n");
+				return -ERANGE;
+			}
+			dp83867->io_impedance = val;
+		} else {
 			dp83867->io_impedance = -1; /* leave at default */
+		}
 
 		return 0;
 	}

-- 
2.54.0

Re: [PATCH v2 3/3] net: phy: dp83867: support setting output-impedance

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Medium] The bounds check for `ti,output-impedance-ohms` is dead code and fails to validate user input due to premature masking, leading to silent hardware misconfiguration.
- [Medium] Implementation contradicts the commit message regarding the precedence of nvmem and DT bindings.
- [Medium] Semantic mismatch between the `-ohms` property suffix and the raw hardware values expected by the driver.
--

commit 2371852a6bc6ccb808abdfecb87e3c77ba5c83df
Author: Steffen Trumtrar <s.trumtrar@pengutronix.de>

    net: phy: dp83867: support setting output-impedance
    
    Currently the dp83867 supports setting the io impedance to minimum,
    maximum, default or a value from a nvmem cell. In situations where there
    is no backend for a nvmem cell, the value can not be set to anything but
    minimum, maximum or default.
    
    Add support for the ti,output-impedance-ohms binding, allowing to
    specify the impedance via the devicetree. The existing boolean bindings
    for minimum and maximum take precedence over this new binding. The nvmem
    consumer still takes lowest precedence.

[Severity: Medium]
Does the implementation contradict the commit message regarding precedence?

The commit message states "The nvmem consumer still takes lowest precedence."
However, if an nvmem cell is successfully found:

dp83867_of_init_io_impedance() {
	cell = of_nvmem_cell_get(of_node, "io_impedance_ctrl");
	if (IS_ERR(cell)) {
		/* If no nvmem cell, check for the boolean properties. */
		...
	}
}

The code skips the error path entirely, meaning the device tree properties
are never evaluated when the nvmem cell exists. This makes the nvmem consumer
take the highest precedence, rather than the lowest.

> diff --git a/drivers/net/phy/dp83867.c b/drivers/net/phy/dp83867.c
> index 88255e92b4cdb..973b70f11c45a 100644
> --- a/drivers/net/phy/dp83867.c
> +++ b/drivers/net/phy/dp83867.c

[ ... ]

> @@ -542,12 +543,21 @@ static int dp83867_of_init_io_impedance(struct phy_device *phydev)
>  						"failed to get nvmem cell io_impedance_ctrl\n");
>  
>  		/* If no nvmem cell, check for the boolean properties. */
> -		if (of_property_read_bool(of_node, "ti,max-output-impedance"))
> +		if (of_property_read_bool(of_node, "ti,max-output-impedance")) {
>  			dp83867->io_impedance = DP83867_IO_MUX_CFG_IO_IMPEDANCE_MAX;
> -		else if (of_property_read_bool(of_node, "ti,min-output-impedance"))
> +		} else if (of_property_read_bool(of_node, "ti,min-output-impedance")) {
>  			dp83867->io_impedance = DP83867_IO_MUX_CFG_IO_IMPEDANCE_MIN;
> -		else
> +		} else if (of_property_read_u32(of_node, "ti,output-impedance-ohms", &val) == 0) {
> +			val = val & DP83867_IO_MUX_CFG_IO_IMPEDANCE_MASK;

[Severity: Medium]
Is there a semantic mismatch between the property name and the expected value?

The property name suffix -ohms implies that the value should be specified in
standard electrical units (Ohms). However, the driver reads the value and uses
it directly as a raw 5-bit register tuning step. Should this property parse
a value in Ohms and translate it to the correct register setting?

> +			if (val > DP83867_IO_MUX_CFG_IO_IMPEDANCE_MIN ||
> +			    val < DP83867_IO_MUX_CFG_IO_IMPEDANCE_MAX) {
> +				phydev_err(phydev, "'ti,output-impedance-ohms' contents out of range\n");
> +				return -ERANGE;
> +			}

[Severity: Medium]
Is this bounds check dead code?

Because val is first masked with DP83867_IO_MUX_CFG_IO_IMPEDANCE_MASK (0x1f),
it can never be greater than DP83867_IO_MUX_CFG_IO_IMPEDANCE_MIN (which is 0x1f).

Additionally, since val is a u32, it can never be less than 0
(DP83867_IO_MUX_CFG_IO_IMPEDANCE_MAX).

If a device tree specifies an out-of-range value like <50>, it will be
silently bitwise-truncated to 18 (50 & 0x1f), bypassing this validation
entirely and potentially misconfiguring the hardware.

> +			dp83867->io_impedance = val;
> +		} else {
>  			dp83867->io_impedance = -1; /* leave at default */
> +		}
>  
>  		return 0;
>  	}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260826-v7-2-topic-dp83867-v2-0-a97b4cec6e93@pengutronix.de?part=3