[PATCH 0/3] net: pse-pd: Add TPS23881B support

[PATCH 0/3] net: pse-pd: Add TPS23881B support

[Thomas Wismer]

This patch series aims at adding support for the TI TPS23881B PoE
PSE controller.

---
Thomas Wismer (3):
  net: pse-pd: tps23881: Fix current measurement scaling
  net: pse-pd: tps23881: Add support for TPS23881B
  dt-bindings: pse-pd: ti,tps23881: Add TPS23881B

 .../bindings/net/pse-pd/ti,tps23881.yaml      |  1 +
 drivers/net/pse-pd/tps23881.c                 | 67 ++++++++++++++-----
 2 files changed, 53 insertions(+), 15 deletions(-)

-- 
2.43.0

[PATCH 1/3] net: pse-pd: tps23881: Fix current measurement scaling

[Thomas Wismer]

From: Thomas Wismer <thomas.wismer@scs.ch>

The TPS23881 improves on the TPS23880 with current sense resistors reduced
from 255 mOhm to 200 mOhm. This has a direct impact on the scaling of the
current measurement. However, the latest TPS23881 data sheet from May 2023
still shows the scaling of the TPS23880 model.

Signed-off-by: Thomas Wismer <thomas.wismer@scs.ch>
---
 drivers/net/pse-pd/tps23881.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/pse-pd/tps23881.c b/drivers/net/pse-pd/tps23881.c
index 63f8f43062bc..b724b222ab44 100644
--- a/drivers/net/pse-pd/tps23881.c
+++ b/drivers/net/pse-pd/tps23881.c
@@ -62,7 +62,7 @@
 #define TPS23881_REG_SRAM_DATA	0x61
 
 #define TPS23881_UV_STEP	3662
-#define TPS23881_NA_STEP	70190
+#define TPS23881_NA_STEP	89500
 #define TPS23881_MW_STEP	500
 #define TPS23881_MIN_PI_PW_LIMIT_MW	2000
 
-- 
2.43.0

[PATCH 2/3] net: pse-pd: tps23881: Add support for TPS23881B

[Thomas Wismer]

From: Thomas Wismer <thomas.wismer@scs.ch>

The TPS23881B device requires different firmware, but has a more recent ROM
firmware. Since no updated firmware has been released yet, the firmware
loading step must be skipped. The device runs from its ROM firmware.

Signed-off-by: Thomas Wismer <thomas.wismer@scs.ch>
---
 drivers/net/pse-pd/tps23881.c | 65 +++++++++++++++++++++++++++--------
 1 file changed, 51 insertions(+), 14 deletions(-)

diff --git a/drivers/net/pse-pd/tps23881.c b/drivers/net/pse-pd/tps23881.c
index b724b222ab44..f45c08759082 100644
--- a/drivers/net/pse-pd/tps23881.c
+++ b/drivers/net/pse-pd/tps23881.c
@@ -55,8 +55,6 @@
 #define TPS23881_REG_TPON	BIT(0)
 #define TPS23881_REG_FWREV	0x41
 #define TPS23881_REG_DEVID	0x43
-#define TPS23881_REG_DEVID_MASK	0xF0
-#define TPS23881_DEVICE_ID	0x02
 #define TPS23881_REG_CHAN1_CLASS	0x4c
 #define TPS23881_REG_SRAM_CTRL	0x60
 #define TPS23881_REG_SRAM_DATA	0x61
@@ -1012,8 +1010,28 @@ static const struct pse_controller_ops tps23881_ops = {
 	.pi_get_pw_req = tps23881_pi_get_pw_req,
 };
 
-static const char fw_parity_name[] = "ti/tps23881/tps23881-parity-14.bin";
-static const char fw_sram_name[] = "ti/tps23881/tps23881-sram-14.bin";
+struct tps23881_info {
+	u8 dev_id;	/* device ID and silicon revision */
+	const char *fw_parity_name;	/* parity code firmware file name */
+	const char *fw_sram_name;	/* SRAM code firmware file name */
+};
+
+enum tps23881_model {
+	TPS23881,
+	TPS23881B,
+};
+
+static const struct tps23881_info tps23881_info[] = {
+	[TPS23881] = {
+		.dev_id = 0x22,
+		.fw_parity_name = "ti/tps23881/tps23881-parity-14.bin",
+		.fw_sram_name = "ti/tps23881/tps23881-sram-14.bin",
+	},
+	[TPS23881B] = {
+		.dev_id = 0x24,
+		/* skip SRAM load, ROM firmware already IEEE802.3bt compliant */
+	},
+};
 
 struct tps23881_fw_conf {
 	u8 reg;
@@ -1085,16 +1103,17 @@ static int tps23881_flash_sram_fw_part(struct i2c_client *client,
 	return ret;
 }
 
-static int tps23881_flash_sram_fw(struct i2c_client *client)
+static int tps23881_flash_sram_fw(struct i2c_client *client,
+				  const struct tps23881_info *info)
 {
 	int ret;
 
-	ret = tps23881_flash_sram_fw_part(client, fw_parity_name,
+	ret = tps23881_flash_sram_fw_part(client, info->fw_parity_name,
 					  tps23881_fw_parity_conf);
 	if (ret)
 		return ret;
 
-	ret = tps23881_flash_sram_fw_part(client, fw_sram_name,
+	ret = tps23881_flash_sram_fw_part(client, info->fw_sram_name,
 					  tps23881_fw_sram_conf);
 	if (ret)
 		return ret;
@@ -1412,6 +1431,7 @@ static int tps23881_setup_irq(struct tps23881_priv *priv, int irq)
 static int tps23881_i2c_probe(struct i2c_client *client)
 {
 	struct device *dev = &client->dev;
+	const struct tps23881_info *info;
 	struct tps23881_priv *priv;
 	struct gpio_desc *reset;
 	int ret;
@@ -1422,6 +1442,10 @@ static int tps23881_i2c_probe(struct i2c_client *client)
 		return -ENXIO;
 	}
 
+	info = device_get_match_data(dev);
+	if (!info)
+		return -EINVAL;
+
 	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
 	if (!priv)
 		return -ENOMEM;
@@ -1440,7 +1464,7 @@ static int tps23881_i2c_probe(struct i2c_client *client)
 		 * to Load TPS2388x SRAM and Parity Code over I2C" (Rev E))
 		 * indicates we should delay that programming by at least 50ms. So
 		 * we'll wait the entire 50ms here to ensure we're safe to go to the
-		 * SRAM loading proceedure.
+		 * SRAM loading procedure.
 		 */
 		msleep(50);
 	}
@@ -1449,20 +1473,26 @@ static int tps23881_i2c_probe(struct i2c_client *client)
 	if (ret < 0)
 		return ret;
 
-	if (FIELD_GET(TPS23881_REG_DEVID_MASK, ret) != TPS23881_DEVICE_ID) {
+	if (ret != info->dev_id) {
 		dev_err(dev, "Wrong device ID\n");
 		return -ENXIO;
 	}
 
-	ret = tps23881_flash_sram_fw(client);
-	if (ret < 0)
-		return ret;
+	if (info->fw_sram_name) {
+		ret = tps23881_flash_sram_fw(client, info);
+		if (ret < 0)
+			return ret;
+	}
 
 	ret = i2c_smbus_read_byte_data(client, TPS23881_REG_FWREV);
 	if (ret < 0)
 		return ret;
 
-	dev_info(&client->dev, "Firmware revision 0x%x\n", ret);
+	if (ret == 0xFF)
+		dev_warn(&client->dev, "Device entered safe mode\n");
+	else
+		dev_info(&client->dev, "Firmware revision 0x%x%s\n", ret,
+			 ret == 0x00 ? " (ROM firmware)" : "");
 
 	/* Set configuration B, 16 bit access on a single device address */
 	ret = i2c_smbus_read_byte_data(client, TPS23881_REG_GEN_MASK);
@@ -1504,7 +1534,14 @@ static const struct i2c_device_id tps23881_id[] = {
 MODULE_DEVICE_TABLE(i2c, tps23881_id);
 
 static const struct of_device_id tps23881_of_match[] = {
-	{ .compatible = "ti,tps23881", },
+	{
+		.compatible = "ti,tps23881",
+		.data = &tps23881_info[TPS23881]
+	},
+	{
+		.compatible = "ti,tps23881b",
+		.data = &tps23881_info[TPS23881B]
+	},
 	{ },
 };
 MODULE_DEVICE_TABLE(of, tps23881_of_match);
-- 
2.43.0

[PATCH 3/3] dt-bindings: pse-pd: ti,tps23881: Add TPS23881B

[Thomas Wismer]

From: Thomas Wismer <thomas.wismer@scs.ch>

Add the TPS23881B I2C power sourcing equipment controller to the list of
supported devices.

Signed-off-by: Thomas Wismer <thomas.wismer@scs.ch>
---
 Documentation/devicetree/bindings/net/pse-pd/ti,tps23881.yaml | 1 +
 1 file changed, 1 insertion(+)

diff --git a/Documentation/devicetree/bindings/net/pse-pd/ti,tps23881.yaml b/Documentation/devicetree/bindings/net/pse-pd/ti,tps23881.yaml
index bb1ee3398655..0b3803f647b7 100644
--- a/Documentation/devicetree/bindings/net/pse-pd/ti,tps23881.yaml
+++ b/Documentation/devicetree/bindings/net/pse-pd/ti,tps23881.yaml
@@ -16,6 +16,7 @@ properties:
   compatible:
     enum:
       - ti,tps23881
+      - ti,tps23881b
 
   reg:
     maxItems: 1
-- 
2.43.0

Re: [PATCH 1/3] net: pse-pd: tps23881: Fix current measurement scaling

[Kory Maincent]

On Sat,  4 Oct 2025 20:03:49 +0200
Thomas Wismer <thomas@wismer.xyz> wrote:

> From: Thomas Wismer <thomas.wismer@scs.ch>
> 
> The TPS23881 improves on the TPS23880 with current sense resistors reduced
> from 255 mOhm to 200 mOhm. This has a direct impact on the scaling of the
> current measurement. However, the latest TPS23881 data sheet from May 2023
> still shows the scaling of the TPS23880 model.

Didn't know that. Where did you get that new current step value if it's not
from the datasheet?

Also as the value reported was wrong maybe we need a fix tag here and send it
to net instead of net-next.
 
> Signed-off-by: Thomas Wismer <thomas.wismer@scs.ch>
> ---
>  drivers/net/pse-pd/tps23881.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/net/pse-pd/tps23881.c b/drivers/net/pse-pd/tps23881.c
> index 63f8f43062bc..b724b222ab44 100644
> --- a/drivers/net/pse-pd/tps23881.c
> +++ b/drivers/net/pse-pd/tps23881.c
> @@ -62,7 +62,7 @@
>  #define TPS23881_REG_SRAM_DATA	0x61
>  
>  #define TPS23881_UV_STEP	3662
> -#define TPS23881_NA_STEP	70190
> +#define TPS23881_NA_STEP	89500
>  #define TPS23881_MW_STEP	500
>  #define TPS23881_MIN_PI_PW_LIMIT_MW	2000
>  



-- 
Köry Maincent, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com

Re: [PATCH 0/3] net: pse-pd: Add TPS23881B support

[Kory Maincent]

On Sat,  4 Oct 2025 20:03:47 +0200
Thomas Wismer <thomas@wismer.xyz> wrote:

> This patch series aims at adding support for the TI TPS23881B PoE
> PSE controller.

First you are missing net-next prefix in the patches subject. Like that:
[PATCH net-next 2/3] net: pse-pd: tps23881: Add support for TPS23881B

See:
https://elixir.bootlin.com/linux/v6.17.1/source/Documentation/process/maintainer-netdev.rst#L9

The merge window for v6.18 has begun and therefore net-next is closed
for new drivers, features, code refactoring and optimizations. We are
currently accepting bug fixes only.

Please repost when net-next reopens after October 12th.

Only patch 1 which is a fix can be posted during the merge window.
 
> ---
> Thomas Wismer (3):
>   net: pse-pd: tps23881: Fix current measurement scaling
>   net: pse-pd: tps23881: Add support for TPS23881B
>   dt-bindings: pse-pd: ti,tps23881: Add TPS23881B
> 
>  .../bindings/net/pse-pd/ti,tps23881.yaml      |  1 +
>  drivers/net/pse-pd/tps23881.c                 | 67 ++++++++++++++-----
>  2 files changed, 53 insertions(+), 15 deletions(-)
> 



-- 
Köry Maincent, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com

Re: [PATCH 2/3] net: pse-pd: tps23881: Add support for TPS23881B

[Kory Maincent]

On Sat,  4 Oct 2025 20:03:51 +0200
Thomas Wismer <thomas@wismer.xyz> wrote:

> From: Thomas Wismer <thomas.wismer@scs.ch>
> 
> The TPS23881B device requires different firmware, but has a more recent ROM
> firmware. Since no updated firmware has been released yet, the firmware
> loading step must be skipped. The device runs from its ROM firmware.
> 
> Signed-off-by: Thomas Wismer <thomas.wismer@scs.ch>
> ---
>  drivers/net/pse-pd/tps23881.c | 65 +++++++++++++++++++++++++++--------
>  1 file changed, 51 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/net/pse-pd/tps23881.c b/drivers/net/pse-pd/tps23881.c
> index b724b222ab44..f45c08759082 100644
> --- a/drivers/net/pse-pd/tps23881.c
> +++ b/drivers/net/pse-pd/tps23881.c
> @@ -55,8 +55,6 @@
>  #define TPS23881_REG_TPON	BIT(0)
>  #define TPS23881_REG_FWREV	0x41
>  #define TPS23881_REG_DEVID	0x43
> -#define TPS23881_REG_DEVID_MASK	0xF0
> -#define TPS23881_DEVICE_ID	0x02
>  #define TPS23881_REG_CHAN1_CLASS	0x4c
>  #define TPS23881_REG_SRAM_CTRL	0x60
>  #define TPS23881_REG_SRAM_DATA	0x61
> @@ -1012,8 +1010,28 @@ static const struct pse_controller_ops tps23881_ops = {
>  	.pi_get_pw_req = tps23881_pi_get_pw_req,
>  };
>  
> -static const char fw_parity_name[] = "ti/tps23881/tps23881-parity-14.bin";
> -static const char fw_sram_name[] = "ti/tps23881/tps23881-sram-14.bin";
> +struct tps23881_info {
> +	u8 dev_id;	/* device ID and silicon revision */
> +	const char *fw_parity_name;	/* parity code firmware file name
> */
> +	const char *fw_sram_name;	/* SRAM code firmware file name */
> +};
> +
> +enum tps23881_model {
> +	TPS23881,
> +	TPS23881B,
> +};
> +
> +static const struct tps23881_info tps23881_info[] = {
> +	[TPS23881] = {
> +		.dev_id = 0x22,
> +		.fw_parity_name = "ti/tps23881/tps23881-parity-14.bin",
> +		.fw_sram_name = "ti/tps23881/tps23881-sram-14.bin",
> +	},
> +	[TPS23881B] = {
> +		.dev_id = 0x24,
> +		/* skip SRAM load, ROM firmware already IEEE802.3bt
> compliant */
> +	},

You are breaking Kyle's patch:
https://patchwork.kernel.org/project/netdevbpf/patch/20240731154152.4020668-1-kyle.swenson@est.tech/

You should check only the device id and not the silicon id.

Regards,
-- 
Köry Maincent, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com

Re: [PATCH 1/3] net: pse-pd: tps23881: Fix current measurement scaling

[Thomas Wismer]

Am Mon, 6 Oct 2025 14:50:29 +0200
schrieb Kory Maincent <kory.maincent@bootlin.com>:

> On Sat,  4 Oct 2025 20:03:49 +0200
> Thomas Wismer <thomas@wismer.xyz> wrote:
> 
> > From: Thomas Wismer <thomas.wismer@scs.ch>
> > 
> > The TPS23881 improves on the TPS23880 with current sense resistors
> > reduced from 255 mOhm to 200 mOhm. This has a direct impact on the
> > scaling of the current measurement. However, the latest TPS23881
> > data sheet from May 2023 still shows the scaling of the TPS23880
> > model.  
> 
> Didn't know that. Where did you get that new current step value if
> it's not from the datasheet?

The correct current measurement scaling can either be found in the
TPS23881B datasheet revised in May 2025 or computed from the legacy sense
resistor value (255 mOhm / 200 mOhm * 70.19 uA = 89.50 uA). I was able to
verify the corrected scaling using power measurement instruments.

> Also as the value reported was wrong maybe we need a fix tag here and
> send it to net instead of net-next.

Right. I'll resubmit this patch to net.

> > Signed-off-by: Thomas Wismer <thomas.wismer@scs.ch>
> > ---
> >  drivers/net/pse-pd/tps23881.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/net/pse-pd/tps23881.c
> > b/drivers/net/pse-pd/tps23881.c index 63f8f43062bc..b724b222ab44
> > 100644 --- a/drivers/net/pse-pd/tps23881.c
> > +++ b/drivers/net/pse-pd/tps23881.c
> > @@ -62,7 +62,7 @@
> >  #define TPS23881_REG_SRAM_DATA	0x61
> >  
> >  #define TPS23881_UV_STEP	3662
> > -#define TPS23881_NA_STEP	70190
> > +#define TPS23881_NA_STEP	89500
> >  #define TPS23881_MW_STEP	500
> >  #define TPS23881_MIN_PI_PW_LIMIT_MW	2000
> >    
> 
> 
> 

Re: [PATCH 2/3] net: pse-pd: tps23881: Add support for TPS23881B

[Thomas Wismer]

Am Mon, 6 Oct 2025 15:05:05 +0200
schrieb Kory Maincent <kory.maincent@bootlin.com>:

> On Sat,  4 Oct 2025 20:03:51 +0200
> Thomas Wismer <thomas@wismer.xyz> wrote:
> 
> > From: Thomas Wismer <thomas.wismer@scs.ch>
> > 
> > The TPS23881B device requires different firmware, but has a more
> > recent ROM firmware. Since no updated firmware has been released
> > yet, the firmware loading step must be skipped. The device runs
> > from its ROM firmware.
> > 
> > Signed-off-by: Thomas Wismer <thomas.wismer@scs.ch>
> > ---
> >  drivers/net/pse-pd/tps23881.c | 65
> > +++++++++++++++++++++++++++-------- 1 file changed, 51
> > insertions(+), 14 deletions(-)
> > 
> > diff --git a/drivers/net/pse-pd/tps23881.c
> > b/drivers/net/pse-pd/tps23881.c index b724b222ab44..f45c08759082
> > 100644 --- a/drivers/net/pse-pd/tps23881.c
> > +++ b/drivers/net/pse-pd/tps23881.c
> > @@ -55,8 +55,6 @@
> >  #define TPS23881_REG_TPON	BIT(0)
> >  #define TPS23881_REG_FWREV	0x41
> >  #define TPS23881_REG_DEVID	0x43
> > -#define TPS23881_REG_DEVID_MASK	0xF0
> > -#define TPS23881_DEVICE_ID	0x02
> >  #define TPS23881_REG_CHAN1_CLASS	0x4c
> >  #define TPS23881_REG_SRAM_CTRL	0x60
> >  #define TPS23881_REG_SRAM_DATA	0x61
> > @@ -1012,8 +1010,28 @@ static const struct pse_controller_ops
> > tps23881_ops = { .pi_get_pw_req = tps23881_pi_get_pw_req,
> >  };
> >  
> > -static const char fw_parity_name[] =
> > "ti/tps23881/tps23881-parity-14.bin"; -static const char
> > fw_sram_name[] = "ti/tps23881/tps23881-sram-14.bin"; +struct
> > tps23881_info {
> > +	u8 dev_id;	/* device ID and silicon revision */
> > +	const char *fw_parity_name;	/* parity code firmware
> > file name */
> > +	const char *fw_sram_name;	/* SRAM code firmware
> > file name */ +};
> > +
> > +enum tps23881_model {
> > +	TPS23881,
> > +	TPS23881B,
> > +};
> > +
> > +static const struct tps23881_info tps23881_info[] = {
> > +	[TPS23881] = {
> > +		.dev_id = 0x22,
> > +		.fw_parity_name =
> > "ti/tps23881/tps23881-parity-14.bin",
> > +		.fw_sram_name = "ti/tps23881/tps23881-sram-14.bin",
> > +	},
> > +	[TPS23881B] = {
> > +		.dev_id = 0x24,
> > +		/* skip SRAM load, ROM firmware already IEEE802.3bt
> > compliant */
> > +	},  
> 
> You are breaking Kyle's patch:
> https://patchwork.kernel.org/project/netdevbpf/patch/20240731154152.4020668-1-kyle.swenson@est.tech/
> 
> You should check only the device id and not the silicon id.

On the TPS23881, the register "DEVICE ID" reads as 0x22 (Device ID number
DID = 0010b, silicon revision number SR = 0010b). On the TPS23881B, 0x24
(DID = 0010b, SR = 0100b) is returned. Both devices report the same
device ID number DID and can only be distinguished by their silicon
revision number SR.

Unfortunately, Kyle's assumption that the driver should work fine with
any silicon revision proved to be wrong. The TPS23881 firmware is not
compatible with the TPS23881B and must not be attempted to be loaded. As
of today, the TPS23881B must be operated using the ROM firmware.

> Regards,

Re: [PATCH 2/3] net: pse-pd: tps23881: Add support for TPS23881B

[Kory Maincent]

On Mon, 6 Oct 2025 23:23:18 +0200
Thomas Wismer <thomas@wismer.xyz> wrote:

> Am Mon, 6 Oct 2025 15:05:05 +0200
> schrieb Kory Maincent <kory.maincent@bootlin.com>:
> 
> > On Sat,  4 Oct 2025 20:03:51 +0200
> > Thomas Wismer <thomas@wismer.xyz> wrote:
> >   
> > > From: Thomas Wismer <thomas.wismer@scs.ch>
> > > 
> > > The TPS23881B device requires different firmware, but has a more
> > > recent ROM firmware. Since no updated firmware has been released
> > > yet, the firmware loading step must be skipped. The device runs
> > > from its ROM firmware.
> > > 
> > > Signed-off-by: Thomas Wismer <thomas.wismer@scs.ch>
> > > ---
> > >  drivers/net/pse-pd/tps23881.c | 65
> > > +++++++++++++++++++++++++++-------- 1 file changed, 51
> > > insertions(+), 14 deletions(-)
> > > 
> > > diff --git a/drivers/net/pse-pd/tps23881.c
> > > b/drivers/net/pse-pd/tps23881.c index b724b222ab44..f45c08759082
> > > 100644 --- a/drivers/net/pse-pd/tps23881.c
> > > +++ b/drivers/net/pse-pd/tps23881.c
> > > @@ -55,8 +55,6 @@
> > >  #define TPS23881_REG_TPON	BIT(0)
> > >  #define TPS23881_REG_FWREV	0x41
> > >  #define TPS23881_REG_DEVID	0x43
> > > -#define TPS23881_REG_DEVID_MASK	0xF0
> > > -#define TPS23881_DEVICE_ID	0x02
> > >  #define TPS23881_REG_CHAN1_CLASS	0x4c
> > >  #define TPS23881_REG_SRAM_CTRL	0x60
> > >  #define TPS23881_REG_SRAM_DATA	0x61
> > > @@ -1012,8 +1010,28 @@ static const struct pse_controller_ops
> > > tps23881_ops = { .pi_get_pw_req = tps23881_pi_get_pw_req,
> > >  };
> > >  
> > > -static const char fw_parity_name[] =
> > > "ti/tps23881/tps23881-parity-14.bin"; -static const char
> > > fw_sram_name[] = "ti/tps23881/tps23881-sram-14.bin"; +struct
> > > tps23881_info {
> > > +	u8 dev_id;	/* device ID and silicon revision */
> > > +	const char *fw_parity_name;	/* parity code firmware
> > > file name */
> > > +	const char *fw_sram_name;	/* SRAM code firmware
> > > file name */ +};
> > > +
> > > +enum tps23881_model {
> > > +	TPS23881,
> > > +	TPS23881B,
> > > +};
> > > +
> > > +static const struct tps23881_info tps23881_info[] = {
> > > +	[TPS23881] = {
> > > +		.dev_id = 0x22,
> > > +		.fw_parity_name =
> > > "ti/tps23881/tps23881-parity-14.bin",
> > > +		.fw_sram_name = "ti/tps23881/tps23881-sram-14.bin",
> > > +	},
> > > +	[TPS23881B] = {
> > > +		.dev_id = 0x24,
> > > +		/* skip SRAM load, ROM firmware already IEEE802.3bt
> > > compliant */
> > > +	},    
> > 
> > You are breaking Kyle's patch:
> > https://patchwork.kernel.org/project/netdevbpf/patch/20240731154152.4020668-1-kyle.swenson@est.tech/
> > 
> > You should check only the device id and not the silicon id.  
> 
> On the TPS23881, the register "DEVICE ID" reads as 0x22 (Device ID number
> DID = 0010b, silicon revision number SR = 0010b). On the TPS23881B, 0x24
> (DID = 0010b, SR = 0100b) is returned. Both devices report the same
> device ID number DID and can only be distinguished by their silicon
> revision number SR.
> 
> Unfortunately, Kyle's assumption that the driver should work fine with
> any silicon revision proved to be wrong. The TPS23881 firmware is not
> compatible with the TPS23881B and must not be attempted to be loaded. As
> of today, the TPS23881B must be operated using the ROM firmware.

Indeed you are right, I misread the datasheet, on my head I thought it was the
device ID which changes between the TPS23881 and the TPS23881B.

Regards,
-- 
Köry Maincent, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com

Re: [PATCH 3/3] dt-bindings: pse-pd: ti,tps23881: Add TPS23881B

[Conor Dooley]

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

On Sat, Oct 04, 2025 at 08:03:53PM +0200, Thomas Wismer wrote:
> From: Thomas Wismer <thomas.wismer@scs.ch>
> 
> Add the TPS23881B I2C power sourcing equipment controller to the list of
> supported devices.

Missing an explanation for why a fallback compatible is not suitable
here. Seems like it is, if the only difference is that the firmware is
not required to be refreshed, provided that loading the non-B firmware
on a B device would not be problematic.

> 
> Signed-off-by: Thomas Wismer <thomas.wismer@scs.ch>
> ---
>  Documentation/devicetree/bindings/net/pse-pd/ti,tps23881.yaml | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/Documentation/devicetree/bindings/net/pse-pd/ti,tps23881.yaml b/Documentation/devicetree/bindings/net/pse-pd/ti,tps23881.yaml
> index bb1ee3398655..0b3803f647b7 100644
> --- a/Documentation/devicetree/bindings/net/pse-pd/ti,tps23881.yaml
> +++ b/Documentation/devicetree/bindings/net/pse-pd/ti,tps23881.yaml
> @@ -16,6 +16,7 @@ properties:
>    compatible:
>      enum:
>        - ti,tps23881
> +      - ti,tps23881b
>  
>    reg:
>      maxItems: 1
> -- 
> 2.43.0
> 

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

Re: [PATCH 3/3] dt-bindings: pse-pd: ti,tps23881: Add TPS23881B

[Thomas Wismer]

Am Tue, 7 Oct 2025 21:40:03 +0100
schrieb Conor Dooley <conor@kernel.org>:

> On Sat, Oct 04, 2025 at 08:03:53PM +0200, Thomas Wismer wrote:
> > From: Thomas Wismer <thomas.wismer@scs.ch>
> > 
> > Add the TPS23881B I2C power sourcing equipment controller to the
> > list of supported devices.  
> 
> Missing an explanation for why a fallback compatible is not suitable
> here. Seems like it is, if the only difference is that the firmware is
> not required to be refreshed, provided that loading the non-B firmware
> on a B device would not be problematic.

Loading the non-B firmware on a B device is indeed problematic. I'll
append the following paragraph to the patch when reposting it after
the current merge window has closed.

Falling back to the TPS23881 predecessor device is not suitable as firmware
loading needs to handled differently by the driver. The TPS23881 and
TPS23881B devices require different firmware. Trying to load the TPS23881
firmware on a TPS23881B device fails and must therefore be omitted.

> > 
> > Signed-off-by: Thomas Wismer <thomas.wismer@scs.ch>
> > ---
> >  Documentation/devicetree/bindings/net/pse-pd/ti,tps23881.yaml | 1 +
> >  1 file changed, 1 insertion(+)
> > 
> > diff --git
> > a/Documentation/devicetree/bindings/net/pse-pd/ti,tps23881.yaml
> > b/Documentation/devicetree/bindings/net/pse-pd/ti,tps23881.yaml
> > index bb1ee3398655..0b3803f647b7 100644 ---
> > a/Documentation/devicetree/bindings/net/pse-pd/ti,tps23881.yaml +++
> > b/Documentation/devicetree/bindings/net/pse-pd/ti,tps23881.yaml @@
> > -16,6 +16,7 @@ properties: compatible: enum:
> >        - ti,tps23881
> > +      - ti,tps23881b
> >  
> >    reg:
> >      maxItems: 1
> > -- 
> > 2.43.0
> >   

Re: [PATCH 3/3] dt-bindings: pse-pd: ti,tps23881: Add TPS23881B

[Andrew Lunn]

On Wed, Oct 08, 2025 at 01:52:43PM +0200, Thomas Wismer wrote:
> Am Tue, 7 Oct 2025 21:40:03 +0100
> schrieb Conor Dooley <conor@kernel.org>:
> 
> > On Sat, Oct 04, 2025 at 08:03:53PM +0200, Thomas Wismer wrote:
> > > From: Thomas Wismer <thomas.wismer@scs.ch>
> > > 
> > > Add the TPS23881B I2C power sourcing equipment controller to the
> > > list of supported devices.  
> > 
> > Missing an explanation for why a fallback compatible is not suitable
> > here. Seems like it is, if the only difference is that the firmware is
> > not required to be refreshed, provided that loading the non-B firmware
> > on a B device would not be problematic.
> 
> Loading the non-B firmware on a B device is indeed problematic. I'll
> append the following paragraph to the patch when reposting it after
> the current merge window has closed.

Is it possible to ask the device what it is?

If you can, maybe you don't even need a new compatible, just load the
appropriate firmware depending on what the device says it is.

	Andrew

Re: [PATCH 3/3] dt-bindings: pse-pd: ti,tps23881: Add TPS23881B

[Thomas Wismer]

Am Wed, 8 Oct 2025 14:38:52 +0200
schrieb Andrew Lunn <andrew@lunn.ch>:

> On Wed, Oct 08, 2025 at 01:52:43PM +0200, Thomas Wismer wrote:
> > Am Tue, 7 Oct 2025 21:40:03 +0100
> > schrieb Conor Dooley <conor@kernel.org>:
> >   
> > > On Sat, Oct 04, 2025 at 08:03:53PM +0200, Thomas Wismer wrote:  
> > > > From: Thomas Wismer <thomas.wismer@scs.ch>
> > > > 
> > > > Add the TPS23881B I2C power sourcing equipment controller to the
> > > > list of supported devices.    
> > > 
> > > Missing an explanation for why a fallback compatible is not
> > > suitable here. Seems like it is, if the only difference is that
> > > the firmware is not required to be refreshed, provided that
> > > loading the non-B firmware on a B device would not be
> > > problematic.  
> > 
> > Loading the non-B firmware on a B device is indeed problematic. I'll
> > append the following paragraph to the patch when reposting it after
> > the current merge window has closed.  
> 
> Is it possible to ask the device what it is?

Yes, the devices allow the silicon revision to be read, which would
enable a driver to correctly handle the case distinctions.

> If you can, maybe you don't even need a new compatible, just load the
> appropriate firmware depending on what the device says it is.

When adapting the driver, I also considered an auto-detection mechanism.
However, it felt safer to rely on the devicetree information than reading
a silicon revision register, which has a totally different meaning on
some other device. I have therefore decided to make the driver behaviour
solely dependent on the devicetree information and to use the silicon
revision only as a sanity check (as already implemented in the driver).
Is there any best practice when to use auto-detection with I2C devices?

Regardless of whether the driver queries the silicon revision, the B
device declaration would look somehow strange to me with a driver having
one single compatible, i.e. compatible = "ti,tps23881b", "ti,tps23881".
The first one specifically names the hardware, the fallback is actually
the name of its predecessor, which is strictly speaking not 100%
compatible but required to have the driver loaded.

Re: [PATCH 3/3] dt-bindings: pse-pd: ti,tps23881: Add TPS23881B

[Andrew Lunn]

> When adapting the driver, I also considered an auto-detection mechanism.
> However, it felt safer to rely on the devicetree information than reading
> a silicon revision register, which has a totally different meaning on
> some other device. I have therefore decided to make the driver behaviour
> solely dependent on the devicetree information and to use the silicon
> revision only as a sanity check (as already implemented in the driver).

So if the silicon and the DT disagree, you get -ENODEV or similar?
That is what i would recommend, so that broken DT blobs get found by
the developer.

> Is there any best practice when to use auto-detection with I2C devices?

Not really. There are devices/drivers where the compatible is just
used to indicate where to find the ID register in the hardware,
nothing else. The ID register is then used by the driver to do the
right thing, we trust the silicon to describe itself. But things like
PHY devices have the ID in a well known location, so we actually don't
require a compatible, but if one is given, we use that instead of the
ID found in the silicon. So the exact opposite.

> Regardless of whether the driver queries the silicon revision, the B
> device declaration would look somehow strange to me with a driver having
> one single compatible, i.e. compatible = "ti,tps23881b", "ti,tps23881".
> The first one specifically names the hardware, the fallback is actually
> the name of its predecessor, which is strictly speaking not 100%
> compatible but required to have the driver loaded.

If it is not compatible, a fallback will not actually work, don't list
a fallback.

	Andrew

Re: [PATCH 3/3] dt-bindings: pse-pd: ti,tps23881: Add TPS23881B

[Conor Dooley]

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

On Thu, Oct 09, 2025 at 11:43:04PM +0200, Andrew Lunn wrote:
> > When adapting the driver, I also considered an auto-detection mechanism.
> > However, it felt safer to rely on the devicetree information than reading
> > a silicon revision register, which has a totally different meaning on
> > some other device. I have therefore decided to make the driver behaviour
> > solely dependent on the devicetree information and to use the silicon
> > revision only as a sanity check (as already implemented in the driver).
> 
> So if the silicon and the DT disagree, you get -ENODEV or similar?
> That is what i would recommend, so that broken DT blobs get found by
> the developer.

I'm personally not a big fan of this kind of thing, as it prevents using
fallbacks for new devices when done strictly. I only really like it
being done this way if the driver does not produce errors for unknown
part numbers, only if (using this case as an example) a b device is
labeled as a non-b, or vice-versa. IOW, if the driver doesn't recognise
the ID, believe what's in DT.

> > Is there any best practice when to use auto-detection with I2C devices?
> 
> Not really. There are devices/drivers where the compatible is just
> used to indicate where to find the ID register in the hardware,
> nothing else. The ID register is then used by the driver to do the
> right thing, we trust the silicon to describe itself. But things like
> PHY devices have the ID in a well known location, so we actually don't
> require a compatible, but if one is given, we use that instead of the
> ID found in the silicon. So the exact opposite.
> 
> > Regardless of whether the driver queries the silicon revision, the B
> > device declaration would look somehow strange to me with a driver having
> > one single compatible, i.e. compatible = "ti,tps23881b", "ti,tps23881".
> > The first one specifically names the hardware, the fallback is actually
> > the name of its predecessor, which is strictly speaking not 100%
> > compatible but required to have the driver loaded.
> 
> If it is not compatible, a fallback will not actually work, don't list
> a fallback.

Yeah, seconded. I think my original mail about this was maybe a bit
confusingly worded, where I was envisaging a world where a driver that
encountered a b device could load the firmware for the non-b device, and
it would just be a redundant operation. A fallback would be suitable,
but obviously not ideal then. Since that isn't permitted, using a
fallback here does not make sense.

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

Re: [PATCH 3/3] dt-bindings: pse-pd: ti,tps23881: Add TPS23881B

[Conor Dooley]

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

On Sat, Oct 04, 2025 at 08:03:53PM +0200, Thomas Wismer wrote:
> From: Thomas Wismer <thomas.wismer@scs.ch>
> 
> Add the TPS23881B I2C power sourcing equipment controller to the list of
> supported devices.
> 
> Signed-off-by: Thomas Wismer <thomas.wismer@scs.ch>

Acked-by: Conor Dooley <conor.dooley@microchip.com>

> ---
>  Documentation/devicetree/bindings/net/pse-pd/ti,tps23881.yaml | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/Documentation/devicetree/bindings/net/pse-pd/ti,tps23881.yaml b/Documentation/devicetree/bindings/net/pse-pd/ti,tps23881.yaml
> index bb1ee3398655..0b3803f647b7 100644
> --- a/Documentation/devicetree/bindings/net/pse-pd/ti,tps23881.yaml
> +++ b/Documentation/devicetree/bindings/net/pse-pd/ti,tps23881.yaml
> @@ -16,6 +16,7 @@ properties:
>    compatible:
>      enum:
>        - ti,tps23881
> +      - ti,tps23881b
>  
>    reg:
>      maxItems: 1
> -- 
> 2.43.0
> 

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

Re: [PATCH 3/3] dt-bindings: pse-pd: ti,tps23881: Add TPS23881B

[Andrew Lunn]

> > So if the silicon and the DT disagree, you get -ENODEV or similar?
> > That is what i would recommend, so that broken DT blobs get found by
> > the developer.
> 
> I'm personally not a big fan of this kind of thing, as it prevents using
> fallbacks for new devices when done strictly. I only really like it
> being done this way if the driver does not produce errors for unknown
> part numbers, only if (using this case as an example) a b device is
> labeled as a non-b, or vice-versa. IOW, if the driver doesn't recognise
> the ID, believe what's in DT.

The issue we have seen in the past when not strictly checking the
compatible against the hardware, is that a number of DT blob ship with
the wrong compatible. Then sometime later you find you need to key
some feature off the compatible, but you cannot without breaking all
those devices which have the wrong compatible.

In order to be able to use the compatible, it has to be correct.

   Andrew