2022-09-05 14:35:55

by Alexandru Gagniuc

Subject: [PATCH 1/3] hwmon: (tps23861) fix byte order in resistance register

The tps23861 registers are little-endian, and regmap_read_bulk() does
not do byte order conversion. On BE machines, the bytes were swapped,
and the interpretation of the resistance value was incorrect.

To make it work on both big and little-endian machines, use
le16_to_cpu() to convert the resitance register to host byte order.

Signed-off-by: Alexandru Gagniuc <[email protected]>
---
drivers/hwmon/tps23861.c | 10 ++++++----
1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/drivers/hwmon/tps23861.c b/drivers/hwmon/tps23861.c
index 42762e87b014..f7c59ff7ae8e 100644
--- a/drivers/hwmon/tps23861.c
+++ b/drivers/hwmon/tps23861.c
@@ -493,18 +493,20 @@ static char *tps23861_port_poe_plus_status(struct tps23861_data *data, int port)

static int tps23861_port_resistance(struct tps23861_data *data, int port)
{
- u16 regval;
+ unsigned int raw_val;
+ __le16 regval;

regmap_bulk_read(data->regmap,
PORT_1_RESISTANCE_LSB + PORT_N_RESISTANCE_LSB_OFFSET * (port - 1),
&regval,
2);

- switch (FIELD_GET(PORT_RESISTANCE_RSN_MASK, regval)) {
+ raw_val = le16_to_cpu(regval);
+ switch (FIELD_GET(PORT_RESISTANCE_RSN_MASK, raw_val)) {
case PORT_RESISTANCE_RSN_OTHER:
- return (FIELD_GET(PORT_RESISTANCE_MASK, regval) * RESISTANCE_LSB) / 10000;
+ return (FIELD_GET(PORT_RESISTANCE_MASK, raw_val) * RESISTANCE_LSB) / 10000;
case PORT_RESISTANCE_RSN_LOW:
- return (FIELD_GET(PORT_RESISTANCE_MASK, regval) * RESISTANCE_LSB_LOW) / 10000;
+ return (FIELD_GET(PORT_RESISTANCE_MASK, raw_val) * RESISTANCE_LSB_LOW) / 10000;
case PORT_RESISTANCE_RSN_SHORT:
case PORT_RESISTANCE_RSN_OPEN:
default:
--
2.34.3

2022-09-05 14:35:57

by Alexandru Gagniuc

Subject: [PATCH 2/3] hwmon: (tps23861) reduce count of i2c transactions for port_status

When reading the 'port_status' debugfs entry, some I2C registers were
read more than once. This looks inefficient in an I2C trace.

To reduce I2C traffic, update tps23861_port_status_show() to only read
each register once. Indexing the port number from 0 instead of 1 also
allows simplifying things a bit, resulting in a negative line count.

Signed-off-by: Alexandru Gagniuc <[email protected]>
---
drivers/hwmon/tps23861.c | 75 ++++++++++++----------------------------
1 file changed, 22 insertions(+), 53 deletions(-)

diff --git a/drivers/hwmon/tps23861.c b/drivers/hwmon/tps23861.c
index f7c59ff7ae8e..0dbd12060b50 100644
--- a/drivers/hwmon/tps23861.c
+++ b/drivers/hwmon/tps23861.c
@@ -372,29 +372,12 @@ static const struct hwmon_chip_info tps23861_chip_info = {
.info = tps23861_info,
};

-static char *tps23861_port_operating_mode(struct tps23861_data *data, int port)
+static char *port_operating_mode_string(uint8_t mode_reg, unsigned int port)
{
- unsigned int regval;
- int mode;
-
- regmap_read(data->regmap, OPERATING_MODE, &regval);
+ unsigned int mode = ~0;

- switch (port) {
- case 1:
- mode = FIELD_GET(OPERATING_MODE_PORT_1_MASK, regval);
- break;
- case 2:
- mode = FIELD_GET(OPERATING_MODE_PORT_2_MASK, regval);
- break;
- case 3:
- mode = FIELD_GET(OPERATING_MODE_PORT_3_MASK, regval);
- break;
- case 4:
- mode = FIELD_GET(OPERATING_MODE_PORT_4_MASK, regval);
- break;
- default:
- mode = -EINVAL;
- }
+ if (port < TPS23861_NUM_PORTS)
+ mode = (mode_reg >> (2 * port)) & OPERATING_MODE_PORT_1_MASK;

switch (mode) {
case OPERATING_MODE_OFF:
@@ -410,15 +393,9 @@ static char *tps23861_port_operating_mode(struct tps23861_data *data, int port)
}
}

-static char *tps23861_port_detect_status(struct tps23861_data *data, int port)
+static char *port_detect_status_string(uint8_t status_reg)
{
- unsigned int regval;
-
- regmap_read(data->regmap,
- PORT_1_STATUS + (port - 1),
- &regval);
-
- switch (FIELD_GET(PORT_STATUS_DETECT_MASK, regval)) {
+ switch (FIELD_GET(PORT_STATUS_DETECT_MASK, status_reg)) {
case PORT_DETECT_UNKNOWN:
return "Unknown device";
case PORT_DETECT_SHORT:
@@ -448,15 +425,9 @@ static char *tps23861_port_detect_status(struct tps23861_data *data, int port)
}
}

-static char *tps23861_port_class_status(struct tps23861_data *data, int port)
+static char *port_class_status_string(uint8_t status_reg)
{
- unsigned int regval;
-
- regmap_read(data->regmap,
- PORT_1_STATUS + (port - 1),
- &regval);
-
- switch (FIELD_GET(PORT_STATUS_CLASS_MASK, regval)) {
+ switch (FIELD_GET(PORT_STATUS_CLASS_MASK, status_reg)) {
case PORT_CLASS_UNKNOWN:
return "Unknown";
case PORT_CLASS_RESERVED:
@@ -479,16 +450,9 @@ static char *tps23861_port_class_status(struct tps23861_data *data, int port)
}
}

-static char *tps23861_port_poe_plus_status(struct tps23861_data *data, int port)
+static char *port_poe_plus_status_string(uint8_t poe_plus, unsigned int port)
{
- unsigned int regval;
-
- regmap_read(data->regmap, POE_PLUS, &regval);
-
- if (BIT(port + 3) & regval)
- return "Yes";
- else
- return "No";
+ return (BIT(port + 4) & poe_plus) ? "Yes" : "No";
}

static int tps23861_port_resistance(struct tps23861_data *data, int port)
@@ -497,7 +461,7 @@ static int tps23861_port_resistance(struct tps23861_data *data, int port)
__le16 regval;

regmap_bulk_read(data->regmap,
- PORT_1_RESISTANCE_LSB + PORT_N_RESISTANCE_LSB_OFFSET * (port - 1),
+ PORT_1_RESISTANCE_LSB + PORT_N_RESISTANCE_LSB_OFFSET * port,
&regval,
2);

@@ -517,14 +481,19 @@ static int tps23861_port_resistance(struct tps23861_data *data, int port)
static int tps23861_port_status_show(struct seq_file *s, void *data)
{
struct tps23861_data *priv = s->private;
- int i;
+ unsigned int i, mode, poe_plus, status;
+
+ regmap_read(priv->regmap, OPERATING_MODE, &mode);
+ regmap_read(priv->regmap, POE_PLUS, &poe_plus);
+
+ for (i = 0; i < TPS23861_NUM_PORTS; i++) {
+ regmap_read(priv->regmap, PORT_1_STATUS + i, &status);

- for (i = 1; i < TPS23861_NUM_PORTS + 1; i++) {
seq_printf(s, "Port: \t\t%d\n", i);
- seq_printf(s, "Operating mode: %s\n", tps23861_port_operating_mode(priv, i));
- seq_printf(s, "Detected: \t%s\n", tps23861_port_detect_status(priv, i));
- seq_printf(s, "Class: \t\t%s\n", tps23861_port_class_status(priv, i));
- seq_printf(s, "PoE Plus: \t%s\n", tps23861_port_poe_plus_status(priv, i));
+ seq_printf(s, "Operating mode: %s\n", port_operating_mode_string(mode, i));
+ seq_printf(s, "Detected: \t%s\n", port_detect_status_string(status));
+ seq_printf(s, "Class: \t\t%s\n", port_class_status_string(status));
+ seq_printf(s, "PoE Plus: \t%s\n", port_poe_plus_status_string(poe_plus, i));
seq_printf(s, "Resistance: \t%d\n", tps23861_port_resistance(priv, i));
seq_putc(s, '\n');
}
--
2.34.3

2022-09-05 16:00:52

by Alexandru Gagniuc

Subject: [PATCH 3/3] hwmon: (tps23861) create unique debugfs directory per device

On systems with more than one tps23861, creating the debugfs directory
for additional devices fails with

debugfs: Directory 'tps23861' with parent '/' already present!

To resolve this, include the hwmon device name in the directory name.
Since the name is unique, this guarantees that the debugfs directory
is unique.

Signed-off-by: Alexandru Gagniuc <[email protected]>
---
drivers/hwmon/tps23861.c | 14 +++++++++++---
1 file changed, 11 insertions(+), 3 deletions(-)

diff --git a/drivers/hwmon/tps23861.c b/drivers/hwmon/tps23861.c
index 0dbd12060b50..3e9df72702c0 100644
--- a/drivers/hwmon/tps23861.c
+++ b/drivers/hwmon/tps23861.c
@@ -503,9 +503,17 @@ static int tps23861_port_status_show(struct seq_file *s, void *data)

DEFINE_SHOW_ATTRIBUTE(tps23861_port_status);

-static void tps23861_init_debugfs(struct tps23861_data *data)
+static void tps23861_init_debugfs(struct tps23861_data *data,
+ struct device *hwmon_dev)
{
- data->debugfs_dir = debugfs_create_dir(data->client->name, NULL);
+ const char *debugfs_name;
+
+ debugfs_name = devm_kasprintf(&data->client->dev, GFP_KERNEL, "%s-%s",
+ data->client->name, dev_name(hwmon_dev));
+ if (!debugfs_name)
+ return;
+
+ data->debugfs_dir = debugfs_create_dir(debugfs_name, NULL);

debugfs_create_file("port_status",
0400,
@@ -554,7 +562,7 @@ static int tps23861_probe(struct i2c_client *client)
if (IS_ERR(hwmon_dev))
return PTR_ERR(hwmon_dev);

- tps23861_init_debugfs(data);
+ tps23861_init_debugfs(data, hwmon_dev);

return 0;
}
--
2.34.3

2022-09-05 22:22:23

by Guenter Roeck

Subject: Re: [PATCH 1/3] hwmon: (tps23861) fix byte order in resistance register

On Mon, Sep 05, 2022 at 09:28:04AM -0500, Alexandru Gagniuc wrote:
> The tps23861 registers are little-endian, and regmap_read_bulk() does
> not do byte order conversion. On BE machines, the bytes were swapped,
> and the interpretation of the resistance value was incorrect.
>
> To make it work on both big and little-endian machines, use
> le16_to_cpu() to convert the resitance register to host byte order.
>
> Signed-off-by: Alexandru Gagniuc <[email protected]>

Applied.

Thanks,
Guenter

> ---
> drivers/hwmon/tps23861.c | 10 ++++++----
> 1 file changed, 6 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/hwmon/tps23861.c b/drivers/hwmon/tps23861.c
> index 42762e87b014..f7c59ff7ae8e 100644
> --- a/drivers/hwmon/tps23861.c
> +++ b/drivers/hwmon/tps23861.c
> @@ -493,18 +493,20 @@ static char *tps23861_port_poe_plus_status(struct tps23861_data *data, int port)
>
> static int tps23861_port_resistance(struct tps23861_data *data, int port)
> {
> - u16 regval;
> + unsigned int raw_val;
> + __le16 regval;
>
> regmap_bulk_read(data->regmap,
> PORT_1_RESISTANCE_LSB + PORT_N_RESISTANCE_LSB_OFFSET * (port - 1),
> &regval,
> 2);
>
> - switch (FIELD_GET(PORT_RESISTANCE_RSN_MASK, regval)) {
> + raw_val = le16_to_cpu(regval);
> + switch (FIELD_GET(PORT_RESISTANCE_RSN_MASK, raw_val)) {
> case PORT_RESISTANCE_RSN_OTHER:
> - return (FIELD_GET(PORT_RESISTANCE_MASK, regval) * RESISTANCE_LSB) / 10000;
> + return (FIELD_GET(PORT_RESISTANCE_MASK, raw_val) * RESISTANCE_LSB) / 10000;
> case PORT_RESISTANCE_RSN_LOW:
> - return (FIELD_GET(PORT_RESISTANCE_MASK, regval) * RESISTANCE_LSB_LOW) / 10000;
> + return (FIELD_GET(PORT_RESISTANCE_MASK, raw_val) * RESISTANCE_LSB_LOW) / 10000;
> case PORT_RESISTANCE_RSN_SHORT:
> case PORT_RESISTANCE_RSN_OPEN:
> default:

2022-09-05 22:44:46

by Guenter Roeck

Subject: Re: [PATCH 3/3] hwmon: (tps23861) create unique debugfs directory per device

On Mon, Sep 05, 2022 at 09:28:06AM -0500, Alexandru Gagniuc wrote:
> On systems with more than one tps23861, creating the debugfs directory
> for additional devices fails with
>
> debugfs: Directory 'tps23861' with parent '/' already present!
>
> To resolve this, include the hwmon device name in the directory name.
> Since the name is unique, this guarantees that the debugfs directory
> is unique.
>
> Signed-off-by: Alexandru Gagniuc <[email protected]>

For my reference:

Reviewed-by: Guenter Roeck <[email protected]>

> ---
> drivers/hwmon/tps23861.c | 14 +++++++++++---
> 1 file changed, 11 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/hwmon/tps23861.c b/drivers/hwmon/tps23861.c
> index 0dbd12060b50..3e9df72702c0 100644
> --- a/drivers/hwmon/tps23861.c
> +++ b/drivers/hwmon/tps23861.c
> @@ -503,9 +503,17 @@ static int tps23861_port_status_show(struct seq_file *s, void *data)
>
> DEFINE_SHOW_ATTRIBUTE(tps23861_port_status);
>
> -static void tps23861_init_debugfs(struct tps23861_data *data)
> +static void tps23861_init_debugfs(struct tps23861_data *data,
> + struct device *hwmon_dev)
> {
> - data->debugfs_dir = debugfs_create_dir(data->client->name, NULL);
> + const char *debugfs_name;
> +
> + debugfs_name = devm_kasprintf(&data->client->dev, GFP_KERNEL, "%s-%s",
> + data->client->name, dev_name(hwmon_dev));
> + if (!debugfs_name)
> + return;
> +
> + data->debugfs_dir = debugfs_create_dir(debugfs_name, NULL);
>
> debugfs_create_file("port_status",
> 0400,
> @@ -554,7 +562,7 @@ static int tps23861_probe(struct i2c_client *client)
> if (IS_ERR(hwmon_dev))
> return PTR_ERR(hwmon_dev);
>
> - tps23861_init_debugfs(data);
> + tps23861_init_debugfs(data, hwmon_dev);
>
> return 0;
> }

2022-09-05 22:48:11

by Guenter Roeck

Subject: Re: [PATCH 2/3] hwmon: (tps23861) reduce count of i2c transactions for port_status

On Mon, Sep 05, 2022 at 09:28:05AM -0500, Alexandru Gagniuc wrote:
> When reading the 'port_status' debugfs entry, some I2C registers were
> read more than once. This looks inefficient in an I2C trace.
>
> To reduce I2C traffic, update tps23861_port_status_show() to only read
> each register once. Indexing the port number from 0 instead of 1 also
> allows simplifying things a bit, resulting in a negative line count.

"resulting in a negative line count"

That took me a while to understand. Please rephrase or drop.

>
> Signed-off-by: Alexandru Gagniuc <[email protected]>
> ---
> drivers/hwmon/tps23861.c | 75 ++++++++++++----------------------------
> 1 file changed, 22 insertions(+), 53 deletions(-)
>
> diff --git a/drivers/hwmon/tps23861.c b/drivers/hwmon/tps23861.c
> index f7c59ff7ae8e..0dbd12060b50 100644
> --- a/drivers/hwmon/tps23861.c
> +++ b/drivers/hwmon/tps23861.c
> @@ -372,29 +372,12 @@ static const struct hwmon_chip_info tps23861_chip_info = {
> .info = tps23861_info,
> };
>
> -static char *tps23861_port_operating_mode(struct tps23861_data *data, int port)
> +static char *port_operating_mode_string(uint8_t mode_reg, unsigned int port)
> {
> - unsigned int regval;
> - int mode;
> -
> - regmap_read(data->regmap, OPERATING_MODE, &regval);
> + unsigned int mode = ~0;
>
> - switch (port) {
> - case 1:
> - mode = FIELD_GET(OPERATING_MODE_PORT_1_MASK, regval);
> - break;
> - case 2:
> - mode = FIELD_GET(OPERATING_MODE_PORT_2_MASK, regval);
> - break;
> - case 3:
> - mode = FIELD_GET(OPERATING_MODE_PORT_3_MASK, regval);
> - break;
> - case 4:
> - mode = FIELD_GET(OPERATING_MODE_PORT_4_MASK, regval);
> - break;
> - default:
> - mode = -EINVAL;
> - }
> + if (port < TPS23861_NUM_PORTS)
> + mode = (mode_reg >> (2 * port)) & OPERATING_MODE_PORT_1_MASK;
>
> switch (mode) {
> case OPERATING_MODE_OFF:
> @@ -410,15 +393,9 @@ static char *tps23861_port_operating_mode(struct tps23861_data *data, int port)
> }
> }
>
> -static char *tps23861_port_detect_status(struct tps23861_data *data, int port)
> +static char *port_detect_status_string(uint8_t status_reg)
> {
> - unsigned int regval;
> -
> - regmap_read(data->regmap,
> - PORT_1_STATUS + (port - 1),
> - &regval);
> -
> - switch (FIELD_GET(PORT_STATUS_DETECT_MASK, regval)) {
> + switch (FIELD_GET(PORT_STATUS_DETECT_MASK, status_reg)) {
> case PORT_DETECT_UNKNOWN:
> return "Unknown device";
> case PORT_DETECT_SHORT:
> @@ -448,15 +425,9 @@ static char *tps23861_port_detect_status(struct tps23861_data *data, int port)
> }
> }
>
> -static char *tps23861_port_class_status(struct tps23861_data *data, int port)
> +static char *port_class_status_string(uint8_t status_reg)
> {
> - unsigned int regval;
> -
> - regmap_read(data->regmap,
> - PORT_1_STATUS + (port - 1),
> - &regval);
> -
> - switch (FIELD_GET(PORT_STATUS_CLASS_MASK, regval)) {
> + switch (FIELD_GET(PORT_STATUS_CLASS_MASK, status_reg)) {
> case PORT_CLASS_UNKNOWN:
> return "Unknown";
> case PORT_CLASS_RESERVED:
> @@ -479,16 +450,9 @@ static char *tps23861_port_class_status(struct tps23861_data *data, int port)
> }
> }
>
> -static char *tps23861_port_poe_plus_status(struct tps23861_data *data, int port)
> +static char *port_poe_plus_status_string(uint8_t poe_plus, unsigned int port)
> {
> - unsigned int regval;
> -
> - regmap_read(data->regmap, POE_PLUS, &regval);
> -
> - if (BIT(port + 3) & regval)
> - return "Yes";
> - else
> - return "No";
> + return (BIT(port + 4) & poe_plus) ? "Yes" : "No";
> }
>
> static int tps23861_port_resistance(struct tps23861_data *data, int port)
> @@ -497,7 +461,7 @@ static int tps23861_port_resistance(struct tps23861_data *data, int port)
> __le16 regval;
>
> regmap_bulk_read(data->regmap,
> - PORT_1_RESISTANCE_LSB + PORT_N_RESISTANCE_LSB_OFFSET * (port - 1),
> + PORT_1_RESISTANCE_LSB + PORT_N_RESISTANCE_LSB_OFFSET * port,
> &regval,
> 2);
>
> @@ -517,14 +481,19 @@ static int tps23861_port_resistance(struct tps23861_data *data, int port)
> static int tps23861_port_status_show(struct seq_file *s, void *data)
> {
> struct tps23861_data *priv = s->private;
> - int i;
> + unsigned int i, mode, poe_plus, status;
> +
> + regmap_read(priv->regmap, OPERATING_MODE, &mode);
> + regmap_read(priv->regmap, POE_PLUS, &poe_plus);
> +
> + for (i = 0; i < TPS23861_NUM_PORTS; i++) {
> + regmap_read(priv->regmap, PORT_1_STATUS + i, &status);
>
> - for (i = 1; i < TPS23861_NUM_PORTS + 1; i++) {
> seq_printf(s, "Port: \t\t%d\n", i);

This changes the port number output from 1..4 to 0..3.
Why ? I can understand the code change, but that doesn't mean
the output has to change as well.

Thanks,
Guenter

> - seq_printf(s, "Operating mode: %s\n", tps23861_port_operating_mode(priv, i));
> - seq_printf(s, "Detected: \t%s\n", tps23861_port_detect_status(priv, i));
> - seq_printf(s, "Class: \t\t%s\n", tps23861_port_class_status(priv, i));
> - seq_printf(s, "PoE Plus: \t%s\n", tps23861_port_poe_plus_status(priv, i));
> + seq_printf(s, "Operating mode: %s\n", port_operating_mode_string(mode, i));
> + seq_printf(s, "Detected: \t%s\n", port_detect_status_string(status));
> + seq_printf(s, "Class: \t\t%s\n", port_class_status_string(status));
> + seq_printf(s, "PoE Plus: \t%s\n", port_poe_plus_status_string(poe_plus, i));
> seq_printf(s, "Resistance: \t%d\n", tps23861_port_resistance(priv, i));
> seq_putc(s, '\n');
> }