[PATCH 0/1] Add ST STPDDC60 pmbus driver

[PATCH 0/1] Add ST STPDDC60 pmbus driver

[Erik Rosen]

This patch adds a pmbus client driver for the ST STPDDC60 Universal
Digital Multicell Controller chip. The technical specification is
not made publicly available, but the pmbus parts are described in the
specification for the BMR481 converter.

The patch has been tested with a Flex BMR481 converter module.


Erik Rosen (1):
  Add ST STPDDC60 pmbus driver

 Documentation/hwmon/index.rst    |   1 +
 Documentation/hwmon/stpddc60.rst |  79 +++++++++++++
 MAINTAINERS                      |   7 ++
 drivers/hwmon/pmbus/Kconfig      |  10 ++
 drivers/hwmon/pmbus/Makefile     |   2 +
 drivers/hwmon/pmbus/stpddc60.c   | 188 +++++++++++++++++++++++++++++++
 6 files changed, 287 insertions(+)
 create mode 100644 Documentation/hwmon/stpddc60.rst
 create mode 100644 drivers/hwmon/pmbus/stpddc60.c

-- 
2.20.1

[PATCH 1/1] Add ST STPDDC60 pmbus driver

[Erik Rosen]

Add ST STPDDC60 pmbus client driver.

Signed-off-by: Erik Rosen <erik.rosen@metormote.com>
---
 Documentation/hwmon/index.rst    |   1 +
 Documentation/hwmon/stpddc60.rst |  79 +++++++++++++
 MAINTAINERS                      |   7 ++
 drivers/hwmon/pmbus/Kconfig      |  10 ++
 drivers/hwmon/pmbus/Makefile     |   2 +
 drivers/hwmon/pmbus/stpddc60.c   | 188 +++++++++++++++++++++++++++++++
 6 files changed, 287 insertions(+)
 create mode 100644 Documentation/hwmon/stpddc60.rst
 create mode 100644 drivers/hwmon/pmbus/stpddc60.c

diff --git a/Documentation/hwmon/index.rst b/Documentation/hwmon/index.rst
index fcb870ce6286..94b4fcf182cd 100644
--- a/Documentation/hwmon/index.rst
+++ b/Documentation/hwmon/index.rst
@@ -169,6 +169,7 @@ Hardware Monitoring Kernel Drivers
    smsc47m192
    smsc47m1
    sparx5-temp
+   stpddc60
    tc654
    tc74
    thmc50
diff --git a/Documentation/hwmon/stpddc60.rst b/Documentation/hwmon/stpddc60.rst
new file mode 100644
index 000000000000..11d5a9977e80
--- /dev/null
+++ b/Documentation/hwmon/stpddc60.rst
@@ -0,0 +1,79 @@
+.. SPDX-License-Identifier: GPL-2.0
+
+Kernel driver stpddc60
+======================
+
+Supported chips:
+
+  * ST STPDDC60
+
+    Prefix: 'stpddc60', 'bmr481'
+
+    Addresses scanned: -
+
+    Datasheet: https://flexpowermodules.com/documents/fpm-techspec-bmr481
+
+Author: Erik Rosen <erik.rosen@metormote.com>
+
+
+Description
+-----------
+
+This driver supports hardware monitoring for ST STPDDC60 controller chip and
+compatible modules.
+
+The driver is a client driver to the core PMBus driver. Please see
+Documentation/hwmon/pmbus.rst and Documentation.hwmon/pmbus-core for details
+on PMBus client drivers.
+
+
+Usage Notes
+-----------
+
+This driver does not auto-detect devices. You will have to instantiate the
+devices explicitly. Please see Documentation/i2c/instantiating-devices.rst for
+details.
+
+The vout under- and over-voltage limits are read-only for this chip.
+
+
+Platform data support
+---------------------
+
+The driver supports standard PMBus driver platform data.
+
+
+Sysfs entries
+-------------
+
+The following attributes are supported. Vin, iout and temp limits
+are read-write; all other attributes are read-only.
+
+======================= ========================================================
+in1_label		"vin"
+in1_input		Measured input voltage.
+in1_lcrit		Critical minimum input voltage.
+in1_crit		Critical maximum input voltage.
+in1_lcrit_alarm		Input voltage critical low alarm.
+in1_crit_alarm		Input voltage critical high alarm.
+
+in2_label		"vout1"
+in2_input		Measured output voltage.
+in2_lcrit		Critical minimum output Voltage.
+in2_crit		Critical maximum output voltage.
+in2_lcrit_alarm		Critical output voltage critical low alarm.
+in2_crit_alarm		Critical output voltage critical high alarm.
+
+curr1_label		"iout1"
+curr1_input		Measured output current.
+curr1_max		Maximum output current.
+curr1_max_alarm		Output current high alarm.
+curr1_crit		Critical maximum output current.
+curr1_crit_alarm	Output current critical high alarm.
+
+temp1_input		Measured maximum temperature of all phases.
+temp1_max		Maximum temperature limit.
+temp1_max_alarm		High temperature alarm.
+temp1_crit		Critical maximum temperature limit.
+temp1_crit_alarm	Critical maximum temperature alarm.
+======================= ========================================================
diff --git a/MAINTAINERS b/MAINTAINERS
index 992fe3b0900a..e4c696f8eabe 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -16819,6 +16819,13 @@ L:	linux-i2c@vger.kernel.org
 S:	Maintained
 F:	drivers/i2c/busses/i2c-stm32*
 
+ST STPDDC60 DRIVER
+M:	Daniel Nilsson <daniel.nilsson@flex.com>
+L:	linux-hwmon@vger.kernel.org
+S:	Maintained
+F:	Documentation/hwmon/stpddc60.rst
+F:	drivers/hwmon/pmbus/stpddc60.c
+
 ST VL53L0X ToF RANGER(I2C) IIO DRIVER
 M:	Song Qiang <songqiang1304521@gmail.com>
 L:	linux-iio@vger.kernel.org
diff --git a/drivers/hwmon/pmbus/Kconfig b/drivers/hwmon/pmbus/Kconfig
index 03606d4298a4..b2becdd84b11 100644
--- a/drivers/hwmon/pmbus/Kconfig
+++ b/drivers/hwmon/pmbus/Kconfig
@@ -247,6 +247,16 @@ config SENSORS_Q54SJ108A2
 	  This driver can also be built as a module. If so, the module will
 	  be called q54sj108a2.
 
+config SENSORS_STPDDC60
+	tristate "ST STPDDC60"
+	help
+	  If you say yes here you get hardware monitoring support for ST
+	  STPDDC60 Universal Digital Multicell Controller, as well as for
+	  Flex BMR481.
+
+	  This driver can also be built as a module. If so, the module will
+	  be called stpddc60.
+
 config SENSORS_TPS40422
 	tristate "TI TPS40422"
 	help
diff --git a/drivers/hwmon/pmbus/Makefile b/drivers/hwmon/pmbus/Makefile
index 6a4ba0fdc1db..ef468806238c 100644
--- a/drivers/hwmon/pmbus/Makefile
+++ b/drivers/hwmon/pmbus/Makefile
@@ -28,9 +28,11 @@ obj-$(CONFIG_SENSORS_MP2975)	+= mp2975.o
 obj-$(CONFIG_SENSORS_PM6764TR)	+= pm6764tr.o
 obj-$(CONFIG_SENSORS_PXE1610)	+= pxe1610.o
 obj-$(CONFIG_SENSORS_Q54SJ108A2)	+= q54sj108a2.o
+obj-$(CONFIG_SENSORS_STPDDC60)	+= stpddc60.o
 obj-$(CONFIG_SENSORS_TPS40422)	+= tps40422.o
 obj-$(CONFIG_SENSORS_TPS53679)	+= tps53679.o
 obj-$(CONFIG_SENSORS_UCD9000)	+= ucd9000.o
 obj-$(CONFIG_SENSORS_UCD9200)	+= ucd9200.o
 obj-$(CONFIG_SENSORS_XDPE122)	+= xdpe12284.o
 obj-$(CONFIG_SENSORS_ZL6100)	+= zl6100.o
+
diff --git a/drivers/hwmon/pmbus/stpddc60.c b/drivers/hwmon/pmbus/stpddc60.c
new file mode 100644
index 000000000000..4e3ab260a3c2
--- /dev/null
+++ b/drivers/hwmon/pmbus/stpddc60.c
@@ -0,0 +1,188 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Hardware monitoring driver for the STPDDC60 controller
+ *
+ * Copyright (c) 2021 Flextronics International Sweden AB.
+ */
+
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/init.h>
+#include <linux/err.h>
+#include <linux/i2c.h>
+#include <linux/pmbus.h>
+#include "pmbus.h"
+
+enum chips { stpddc60 };
+
+static const struct i2c_device_id stpddc60_id[] = {
+	{"stpddc60", stpddc60},
+	{"bmr481", stpddc60},
+	{}
+};
+MODULE_DEVICE_TABLE(i2c, stpddc60_id);
+
+static struct pmbus_driver_info stpddc60_info = {
+	.pages = 1,
+	.func[0] = PMBUS_HAVE_VOUT | PMBUS_HAVE_STATUS_VOUT
+		| PMBUS_HAVE_VIN | PMBUS_HAVE_STATUS_INPUT
+		| PMBUS_HAVE_TEMP | PMBUS_HAVE_STATUS_TEMP
+		| PMBUS_HAVE_IOUT | PMBUS_HAVE_STATUS_IOUT
+		| PMBUS_HAVE_POUT,
+};
+
+/*
+ * Convert VID value to milli-volt
+ */
+static long stpddc60_vid2mv(int val)
+{
+	long rv = 0;
+
+	if (val >= 0x01)
+		rv = 250 + (val - 1) * 5;
+
+	return rv;
+}
+
+/*
+ * Convert milli-volt to linear
+ */
+static int stpddc60_mv2l(long mv)
+{
+	int rv;
+
+	rv = (mv << 8) / 1000;
+
+	return rv;
+}
+
+/*
+ * The VOUT_COMMAND register uses the VID format but the vout alarm limit
+ * registers use the linear format so we override VOUT_MODE here to force
+ * linear format for all registers.
+ */
+static int stpddc60_read_byte_data(struct i2c_client *client, int page, int reg)
+{
+	int ret;
+
+	if (page > 0)
+		return -ENXIO;
+
+	switch (reg) {
+	case PMBUS_VOUT_MODE:
+		ret = 0x18;
+		break;
+	default:
+		ret = -ENODATA;
+		break;
+	}
+
+	return ret;
+}
+
+/*
+ * Do the necessary conversions between VID and linear data format.
+ */
+static int stpddc60_read_word_data(struct i2c_client *client, int page,
+				   int phase, int reg)
+{
+	int ret;
+
+	if (page > 0)
+		return -ENXIO;
+
+	switch (reg) {
+	case PMBUS_READ_VOUT:
+		ret = pmbus_read_word_data(client, page, phase, reg);
+		if (ret < 0)
+			return ret;
+		ret = stpddc60_mv2l(stpddc60_vid2mv(ret));
+		break;
+	case PMBUS_VOUT_OV_FAULT_LIMIT:
+	case PMBUS_VOUT_UV_FAULT_LIMIT:
+		ret = pmbus_read_word_data(client, page, phase, reg);
+		if (ret < 0)
+			return ret;
+		ret &= 0x07ff;
+		break;
+	default:
+		ret = -ENODATA;
+		break;
+	}
+
+	return ret;
+}
+
+/*
+ * The vout under- and over-voltage limits are readonly for this chip.
+ */
+static int stpddc60_write_word_data(struct i2c_client *client, int page,
+				    int reg, u16 word)
+{
+	int ret;
+
+	if (page > 0)
+		return -ENXIO;
+
+	switch (reg) {
+	case PMBUS_VOUT_OV_FAULT_LIMIT:
+		ret = -EACCES;
+		break;
+	case PMBUS_VOUT_UV_FAULT_LIMIT:
+		ret = -EACCES;
+		break;
+	default:
+		ret = -ENODATA;
+		break;
+	}
+
+	return ret;
+}
+
+static int stpddc60_probe(struct i2c_client *client,
+			  const struct i2c_device_id *id)
+{
+	int status;
+	u8 device_id[I2C_SMBUS_BLOCK_MAX + 1];
+	const struct i2c_device_id *mid;
+	struct pmbus_driver_info *info = &stpddc60_info;
+
+	if (!i2c_check_functionality(client->adapter,
+				     I2C_FUNC_SMBUS_READ_BYTE_DATA
+				     | I2C_FUNC_SMBUS_BLOCK_DATA))
+		return -ENODEV;
+
+	status = i2c_smbus_read_block_data(client, PMBUS_MFR_MODEL, device_id);
+	if (status < 0) {
+		dev_err(&client->dev, "Failed to read Manufacturer Model\n");
+		return status;
+	}
+	for (mid = stpddc60_id; mid->name[0]; mid++) {
+		if (!strncasecmp(mid->name, device_id, strlen(mid->name)))
+			break;
+	}
+	if (!mid->name[0]) {
+		dev_err(&client->dev, "Unsupported device\n");
+		return -ENODEV;
+	}
+
+	info->read_byte_data = stpddc60_read_byte_data;
+	info->read_word_data = stpddc60_read_word_data;
+	info->write_word_data = stpddc60_write_word_data;
+
+	return pmbus_do_probe(client, info);
+}
+
+static struct i2c_driver stpddc60_driver = {
+	.driver = {
+		   .name = "stpddc60",
+		   },
+	.probe = stpddc60_probe,
+	.id_table = stpddc60_id,
+};
+
+module_i2c_driver(stpddc60_driver);
+
+MODULE_AUTHOR("Erik Rosen <erik.rosen@metormote.com>");
+MODULE_DESCRIPTION("PMBus driver for ST STPDDC60");
+MODULE_LICENSE("GPL");
-- 
2.20.1

Re: [PATCH 1/1] Add ST STPDDC60 pmbus driver

[Guenter Roeck]

On Wed, Jan 27, 2021 at 09:41:40AM +0100, Erik Rosen wrote:
> Add ST STPDDC60 pmbus client driver.
> 
> Signed-off-by: Erik Rosen <erik.rosen@metormote.com>
> ---
>  Documentation/hwmon/index.rst    |   1 +
>  Documentation/hwmon/stpddc60.rst |  79 +++++++++++++
>  MAINTAINERS                      |   7 ++
>  drivers/hwmon/pmbus/Kconfig      |  10 ++
>  drivers/hwmon/pmbus/Makefile     |   2 +
>  drivers/hwmon/pmbus/stpddc60.c   | 188 +++++++++++++++++++++++++++++++
>  6 files changed, 287 insertions(+)
>  create mode 100644 Documentation/hwmon/stpddc60.rst
>  create mode 100644 drivers/hwmon/pmbus/stpddc60.c
> 
> diff --git a/Documentation/hwmon/index.rst b/Documentation/hwmon/index.rst
> index fcb870ce6286..94b4fcf182cd 100644
> --- a/Documentation/hwmon/index.rst
> +++ b/Documentation/hwmon/index.rst
> @@ -169,6 +169,7 @@ Hardware Monitoring Kernel Drivers
>     smsc47m192
>     smsc47m1
>     sparx5-temp
> +   stpddc60
>     tc654
>     tc74
>     thmc50
> diff --git a/Documentation/hwmon/stpddc60.rst b/Documentation/hwmon/stpddc60.rst
> new file mode 100644
> index 000000000000..11d5a9977e80
> --- /dev/null
> +++ b/Documentation/hwmon/stpddc60.rst
> @@ -0,0 +1,79 @@
> +.. SPDX-License-Identifier: GPL-2.0
> +
> +Kernel driver stpddc60
> +======================
> +
> +Supported chips:
> +
> +  * ST STPDDC60
> +
> +    Prefix: 'stpddc60', 'bmr481'
> +
> +    Addresses scanned: -
> +
> +    Datasheet: https://flexpowermodules.com/documents/fpm-techspec-bmr481
> +
> +Author: Erik Rosen <erik.rosen@metormote.com>
> +
> +
> +Description
> +-----------
> +
> +This driver supports hardware monitoring for ST STPDDC60 controller chip and
> +compatible modules.
> +
> +The driver is a client driver to the core PMBus driver. Please see
> +Documentation/hwmon/pmbus.rst and Documentation.hwmon/pmbus-core for details
> +on PMBus client drivers.
> +
> +
> +Usage Notes
> +-----------
> +
> +This driver does not auto-detect devices. You will have to instantiate the
> +devices explicitly. Please see Documentation/i2c/instantiating-devices.rst for
> +details.
> +
> +The vout under- and over-voltage limits are read-only for this chip.
> +
> +
> +Platform data support
> +---------------------
> +
> +The driver supports standard PMBus driver platform data.
> +
> +
> +Sysfs entries
> +-------------
> +
> +The following attributes are supported. Vin, iout and temp limits
> +are read-write; all other attributes are read-only.
> +
> +======================= ========================================================
> +in1_label		"vin"
> +in1_input		Measured input voltage.
> +in1_lcrit		Critical minimum input voltage.
> +in1_crit		Critical maximum input voltage.
> +in1_lcrit_alarm		Input voltage critical low alarm.
> +in1_crit_alarm		Input voltage critical high alarm.
> +
> +in2_label		"vout1"
> +in2_input		Measured output voltage.
> +in2_lcrit		Critical minimum output Voltage.
> +in2_crit		Critical maximum output voltage.
> +in2_lcrit_alarm		Critical output voltage critical low alarm.
> +in2_crit_alarm		Critical output voltage critical high alarm.
> +
> +curr1_label		"iout1"
> +curr1_input		Measured output current.
> +curr1_max		Maximum output current.
> +curr1_max_alarm		Output current high alarm.
> +curr1_crit		Critical maximum output current.
> +curr1_crit_alarm	Output current critical high alarm.
> +
> +temp1_input		Measured maximum temperature of all phases.
> +temp1_max		Maximum temperature limit.
> +temp1_max_alarm		High temperature alarm.
> +temp1_crit		Critical maximum temperature limit.
> +temp1_crit_alarm	Critical maximum temperature alarm.
> +======================= ========================================================
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 992fe3b0900a..e4c696f8eabe 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -16819,6 +16819,13 @@ L:	linux-i2c@vger.kernel.org
>  S:	Maintained
>  F:	drivers/i2c/busses/i2c-stm32*
>  
> +ST STPDDC60 DRIVER
> +M:	Daniel Nilsson <daniel.nilsson@flex.com>
> +L:	linux-hwmon@vger.kernel.org
> +S:	Maintained
> +F:	Documentation/hwmon/stpddc60.rst
> +F:	drivers/hwmon/pmbus/stpddc60.c
> +
>  ST VL53L0X ToF RANGER(I2C) IIO DRIVER
>  M:	Song Qiang <songqiang1304521@gmail.com>
>  L:	linux-iio@vger.kernel.org
> diff --git a/drivers/hwmon/pmbus/Kconfig b/drivers/hwmon/pmbus/Kconfig
> index 03606d4298a4..b2becdd84b11 100644
> --- a/drivers/hwmon/pmbus/Kconfig
> +++ b/drivers/hwmon/pmbus/Kconfig
> @@ -247,6 +247,16 @@ config SENSORS_Q54SJ108A2
>  	  This driver can also be built as a module. If so, the module will
>  	  be called q54sj108a2.
>  
> +config SENSORS_STPDDC60
> +	tristate "ST STPDDC60"
> +	help
> +	  If you say yes here you get hardware monitoring support for ST
> +	  STPDDC60 Universal Digital Multicell Controller, as well as for
> +	  Flex BMR481.
> +
> +	  This driver can also be built as a module. If so, the module will
> +	  be called stpddc60.
> +
>  config SENSORS_TPS40422
>  	tristate "TI TPS40422"
>  	help
> diff --git a/drivers/hwmon/pmbus/Makefile b/drivers/hwmon/pmbus/Makefile
> index 6a4ba0fdc1db..ef468806238c 100644
> --- a/drivers/hwmon/pmbus/Makefile
> +++ b/drivers/hwmon/pmbus/Makefile
> @@ -28,9 +28,11 @@ obj-$(CONFIG_SENSORS_MP2975)	+= mp2975.o
>  obj-$(CONFIG_SENSORS_PM6764TR)	+= pm6764tr.o
>  obj-$(CONFIG_SENSORS_PXE1610)	+= pxe1610.o
>  obj-$(CONFIG_SENSORS_Q54SJ108A2)	+= q54sj108a2.o
> +obj-$(CONFIG_SENSORS_STPDDC60)	+= stpddc60.o
>  obj-$(CONFIG_SENSORS_TPS40422)	+= tps40422.o
>  obj-$(CONFIG_SENSORS_TPS53679)	+= tps53679.o
>  obj-$(CONFIG_SENSORS_UCD9000)	+= ucd9000.o
>  obj-$(CONFIG_SENSORS_UCD9200)	+= ucd9200.o
>  obj-$(CONFIG_SENSORS_XDPE122)	+= xdpe12284.o
>  obj-$(CONFIG_SENSORS_ZL6100)	+= zl6100.o
> +
> diff --git a/drivers/hwmon/pmbus/stpddc60.c b/drivers/hwmon/pmbus/stpddc60.c
> new file mode 100644
> index 000000000000..4e3ab260a3c2
> --- /dev/null
> +++ b/drivers/hwmon/pmbus/stpddc60.c
> @@ -0,0 +1,188 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * Hardware monitoring driver for the STPDDC60 controller
> + *
> + * Copyright (c) 2021 Flextronics International Sweden AB.
> + */
> +
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/init.h>
> +#include <linux/err.h>
> +#include <linux/i2c.h>
> +#include <linux/pmbus.h>
> +#include "pmbus.h"
> +
> +enum chips { stpddc60 };

Seems pointless since it is not used.

> +
> +static const struct i2c_device_id stpddc60_id[] = {
> +	{"stpddc60", stpddc60},
> +	{"bmr481", stpddc60},
> +	{}
> +};
> +MODULE_DEVICE_TABLE(i2c, stpddc60_id);
> +
> +static struct pmbus_driver_info stpddc60_info = {
> +	.pages = 1,
> +	.func[0] = PMBUS_HAVE_VOUT | PMBUS_HAVE_STATUS_VOUT
> +		| PMBUS_HAVE_VIN | PMBUS_HAVE_STATUS_INPUT
> +		| PMBUS_HAVE_TEMP | PMBUS_HAVE_STATUS_TEMP
> +		| PMBUS_HAVE_IOUT | PMBUS_HAVE_STATUS_IOUT
> +		| PMBUS_HAVE_POUT,
> +};
> +
> +/*
> + * Convert VID value to milli-volt
> + */
> +static long stpddc60_vid2mv(int val)
> +{
> +	long rv = 0;
> +
> +	if (val >= 0x01)
> +		rv = 250 + (val - 1) * 5;
> +
> +	return rv;
> +}
> +
> +/*
> + * Convert milli-volt to linear
> + */
> +static int stpddc60_mv2l(long mv)
> +{
> +	int rv;
> +
> +	rv = (mv << 8) / 1000;

DIV_ROUND_CLOSEST(), maybe ?

> +
> +	return rv;
> +}

Pleae combine those two functions into one to directly convert VID to
linear format. Having two functions is confusing and adds unnecessary
complexity.

> +
> +/*
> + * The VOUT_COMMAND register uses the VID format but the vout alarm limit
> + * registers use the linear format so we override VOUT_MODE here to force
> + * linear format for all registers.
> + */
> +static int stpddc60_read_byte_data(struct i2c_client *client, int page, int reg)
> +{
> +	int ret;
> +
> +	if (page > 0)
> +		return -ENXIO;
> +
> +	switch (reg) {
> +	case PMBUS_VOUT_MODE:
> +		ret = 0x18;
> +		break;
> +	default:
> +		ret = -ENODATA;
> +		break;
> +	}
> +
> +	return ret;
> +}
> +
> +/*
> + * Do the necessary conversions between VID and linear data format.
> + */
> +static int stpddc60_read_word_data(struct i2c_client *client, int page,
> +				   int phase, int reg)
> +{
> +	int ret;
> +
> +	if (page > 0)
> +		return -ENXIO;
> +
> +	switch (reg) {
> +	case PMBUS_READ_VOUT:
> +		ret = pmbus_read_word_data(client, page, phase, reg);
> +		if (ret < 0)
> +			return ret;
> +		ret = stpddc60_mv2l(stpddc60_vid2mv(ret));

It might possibly be simpler to just return the value reported
by MFR_READ_VOUT. 

> +		break;
> +	case PMBUS_VOUT_OV_FAULT_LIMIT:
> +	case PMBUS_VOUT_UV_FAULT_LIMIT:
> +		ret = pmbus_read_word_data(client, page, phase, reg);
> +		if (ret < 0)
> +			return ret;
> +		ret &= 0x07ff;

This needs explanation. The BMR481 datasheet does not suggest that only
11 bits are valid.

> +		break;
> +	default:
> +		ret = -ENODATA;
> +		break;
> +	}
> +
> +	return ret;
> +}
> +
> +/*
> + * The vout under- and over-voltage limits are readonly for this chip.
> + */

Not really. The BMR481 datasheet suggests that the value can be changed
by writing to MFR_OV_LIMIT_OFFSET and MFR_UV_LIMIT_OFFSET.
I am not saying that you should implement that if you don't want to,
but I would suggest a better (more accurate) explanation.

> +static int stpddc60_write_word_data(struct i2c_client *client, int page,
> +				    int reg, u16 word)
> +{
> +	int ret;
> +
> +	if (page > 0)
> +		return -ENXIO;
> +
> +	switch (reg) {
> +	case PMBUS_VOUT_OV_FAULT_LIMIT:
> +		ret = -EACCES;
> +		break;
> +	case PMBUS_VOUT_UV_FAULT_LIMIT:
> +		ret = -EACCES;
> +		break;
> +	default:
> +		ret = -ENODATA;
> +		break;
> +	}
> +
> +	return ret;
> +}
> +
> +static int stpddc60_probe(struct i2c_client *client,
> +			  const struct i2c_device_id *id)
> +{
> +	int status;
> +	u8 device_id[I2C_SMBUS_BLOCK_MAX + 1];
> +	const struct i2c_device_id *mid;
> +	struct pmbus_driver_info *info = &stpddc60_info;
> +
> +	if (!i2c_check_functionality(client->adapter,
> +				     I2C_FUNC_SMBUS_READ_BYTE_DATA
> +				     | I2C_FUNC_SMBUS_BLOCK_DATA))
> +		return -ENODEV;
> +
> +	status = i2c_smbus_read_block_data(client, PMBUS_MFR_MODEL, device_id);
> +	if (status < 0) {
> +		dev_err(&client->dev, "Failed to read Manufacturer Model\n");
> +		return status;
> +	}
> +	for (mid = stpddc60_id; mid->name[0]; mid++) {
> +		if (!strncasecmp(mid->name, device_id, strlen(mid->name)))
> +			break;
> +	}
> +	if (!mid->name[0]) {
> +		dev_err(&client->dev, "Unsupported device\n");
> +		return -ENODEV;
> +	}
> +
> +	info->read_byte_data = stpddc60_read_byte_data;
> +	info->read_word_data = stpddc60_read_word_data;
> +	info->write_word_data = stpddc60_write_word_data;
> +
> +	return pmbus_do_probe(client, info);
> +}
> +
> +static struct i2c_driver stpddc60_driver = {
> +	.driver = {
> +		   .name = "stpddc60",
> +		   },
> +	.probe = stpddc60_probe,

Please use the probe_new function callback (no id parameter).

> +	.id_table = stpddc60_id,
> +};
> +
> +module_i2c_driver(stpddc60_driver);
> +
> +MODULE_AUTHOR("Erik Rosen <erik.rosen@metormote.com>");
> +MODULE_DESCRIPTION("PMBus driver for ST STPDDC60");
> +MODULE_LICENSE("GPL");
> -- 
> 2.20.1
> 

Re: [PATCH 1/1] Add ST STPDDC60 pmbus driver

[Erik Rosen]

Hi Guenter


On Fri, Jan 29, 2021 at 4:50 PM Guenter Roeck <linux@roeck-us.net> wrote:
>
> Hi Erik,
>
> On 1/29/21 5:07 AM, Erik Rosen wrote:
> [ ... ]
> >>
> >>> +             break;
> >>> +     case PMBUS_VOUT_OV_FAULT_LIMIT:
> >>> +     case PMBUS_VOUT_UV_FAULT_LIMIT:
> >>> +             ret = pmbus_read_word_data(client, page, phase, reg);
> >>> +             if (ret < 0)
> >>> +                     return ret;
> >>> +             ret &= 0x07ff;
> >>
> >> This needs explanation. The BMR481 datasheet does not suggest that only
> >> 11 bits are valid.
> >
> > Ok, I will add a clarification. These registers use LINEAR11 whereas
> > LINEAR16 is expected for vout-related registers.
> >
> Is that the correct fix, then ? LINEAR11 means that the exponent is flexible,
> while it is fixed for LINEAR16. Just assuming that the exponents always match
> seems risky. Also, bit 11 in LINEAR11 is the sign bit, meaning negative limits
> (which seem at least theoretically be possible) would be reported as large
> positive values.

The chip actually uses fixed exponents for representing all linear values and
the specification explicitly states the values of the LSB for all registers.
It also states that the limits can be handled as linear format when
writing  _if_
the exponent is the fixed value used for that limit. This means I'll have to
convert all limit writes to use the expected exponent.
Given this, I think it's safe to assume that the exponents are
constant, but I'll
add a check to handle potential negative values.

>
> >>
> >>> +             break;
> >>> +     default:
> >>> +             ret = -ENODATA;
> >>> +             break;
> >>> +     }
> >>> +
> >>> +     return ret;
> >>> +}
> >>> +
> >>> +/*
> >>> + * The vout under- and over-voltage limits are readonly for this chip.
> >>> + */
> >>
> >> Not really. The BMR481 datasheet suggests that the value can be changed
> >> by writing to MFR_OV_LIMIT_OFFSET and MFR_UV_LIMIT_OFFSET.
> >> I am not saying that you should implement that if you don't want to,
> >> but I would suggest a better (more accurate) explanation.
> >
> > I have looked into this a bit more and it's a bit more to it than I expected.
> > The limits are actually dynamic values that will follow both commanded
> > output voltage via the PMBus or SVID/AVSBus as well as current
> > load (droop). To account for this I propose a mechanism to set the
>
> Yes, I noticed.
>
> > 'update' flag on selected sensors after auto-detection of limit attributes.
> >
> > Maybe add a function like this to pmbus_core that can be called
> > after the probing is done?
> >
> > int pmbus_set_update(struct i2c_client *client, u8 reg)
> > {
> >         struct pmbus_data *data = i2c_get_clientdata(client);
> >         struct pmbus_sensor *sensor;
> >
> >         for (sensor = data->sensors; sensor; sensor = sensor->next) {
> >                 if (sensor->reg == reg) {
> >                         sensor->update = true;
> >                         return 0;
> >                 }
> >         }
> >         return -ENXIO;
> > }
> >
>
> Add a boolean 'update' parameter (to be able to disable updates),
> and make the function void. Also, remember that 'reg' may be repeated
> on a chip with multiple pages, so you can't return after the first match.
> Otherwise looks ok.

Ok

>
> > I did also implemented writes to the limit registers via the offset
> > registers but it might be
> > a bit confusing to the user since the limits are set in relation to
> > the current commanded
> > output voltage and will change together with this. In the worst case,
> > the voltage might
> > change at the same time as the limit is written to, which will give
> > unexpected results.
>
> Agreed, that can be tricky.
>
> > The alternative would be to just set these limits read-only. What is
> > your opinion?
> >
>
> Your call. Just add a note explaining why it is made read-only to explain
> the reasoning for future readers.

Ok, I'll keep the functionality and add a note in the docs.


>
> > Also, I found a problem in how pmbus_set_sensor() works together with
> > pmbus_clear_cache()
> > as used in the zl6100 and lm25066 drivers. The new value is written to
> > the sensor struct _after_
> > the _pmbus_write_word_data() has returned and thus after
> > pmbus_clear_cache() is called.
> > The effect is that all values are cleared, except the one just written
> > to, which I believe defeats
> > the purpose of clearing the cache in the first place.> One solution would be to write the new value to sensor->data before
> > the _pmbus_write_word_data
> > is called and to restore the old value in case of error.
> > I can make a separate patch for this if it seems like a reasonable solution?
> >
>
> Good catch.
>
> An alternate and more generic solution might be to set sensor->data to
> -ENODATA after updates. After all, it seems risky to assume that the chip
> returns the value that was written (I have seen it often enough in other
> drivers that this is not necessarily the case). That would also mean that
> it would no longer be necessary to call pmbus_clear_cache() in the zl6100
> and lm25066 drivers. Impact would be that a limit read after a write would
> always be sent to the chip for all drivers, but that seems minimal compared
> to the gain (and presumably limit registers are not frequently updated).

I agree that this is a more robust solution. I'll send a separate patch for this
update. The zl6100 in fact still needs to clear the cache since a write to the
fault limit also changes the value of the warning limit and vice versa. As far
as I can see the usage in lm25066 can be removed however.

>
> Thanks,
> Guenter

Re: [PATCH 1/1] Add ST STPDDC60 pmbus driver

[Guenter Roeck]

On 2/1/21 8:48 AM, Erik Rosen wrote:
> Hi Guenter
> 
> 
> On Fri, Jan 29, 2021 at 4:50 PM Guenter Roeck <linux@roeck-us.net> wrote:
>>
>> Hi Erik,
>>
>> On 1/29/21 5:07 AM, Erik Rosen wrote:
>> [ ... ]
>>>>
>>>>> +             break;
>>>>> +     case PMBUS_VOUT_OV_FAULT_LIMIT:
>>>>> +     case PMBUS_VOUT_UV_FAULT_LIMIT:
>>>>> +             ret = pmbus_read_word_data(client, page, phase, reg);
>>>>> +             if (ret < 0)
>>>>> +                     return ret;
>>>>> +             ret &= 0x07ff;
>>>>
>>>> This needs explanation. The BMR481 datasheet does not suggest that only
>>>> 11 bits are valid.
>>>
>>> Ok, I will add a clarification. These registers use LINEAR11 whereas
>>> LINEAR16 is expected for vout-related registers.
>>>
>> Is that the correct fix, then ? LINEAR11 means that the exponent is flexible,
>> while it is fixed for LINEAR16. Just assuming that the exponents always match
>> seems risky. Also, bit 11 in LINEAR11 is the sign bit, meaning negative limits
>> (which seem at least theoretically be possible) would be reported as large
>> positive values.
> 
> The chip actually uses fixed exponents for representing all linear values and
> the specification explicitly states the values of the LSB for all registers.
> It also states that the limits can be handled as linear format when
> writing  _if_
> the exponent is the fixed value used for that limit. This means I'll have to
> convert all limit writes to use the expected exponent.

I understand the datasheet a bit differently. You are correct, the exponent for
VOUT limits is always the same (LSB=0.00390625V, exponent -8), as it should be.
But it also seems to me that the data format is LINEAR16, not LINEAR11.
The datasheet says that the vout limit values are in bit 0..15.

For other sensors, the datasheet is a bit ambiguous. I can read it as always
using a fixed exponent when reading, or that it expects a fixed exponent when
writing. It might make sense to check this with an actual chip instead of
guessing. In this context, it is a bit odd that the datasheet keeps talking
about a "System Register IOUT_EXP" without actually specifying it.

In this context, you might want to watch out for register MFR_SVID_SLOW_SR_SELECTOR.
Its value seems to impact the exponents used for IOUT and POUT.

Also, I am not sure about the scale of other registers. It almost seems as if
limit registers are LINEAR11, but actual READ_xxx registers are LINEAR16.
For example, READ_IOUT is supposed to be bit 15:0 with the LSB determined
by IOUT_EXP, but IOUT_OC_WARN_LIMIT is in bit 9:0. This would be a problem
if READ_IOUT does not include the exponent, since it is interpreted as
LINEAR11 and expects the exponent in bit 11..15. With an expected exponent
of -1, the reported value would always be wrong. The same might apply
to pretty much all READ_xxx registers (what a mess).

Thanks,
Guenter

> Given this, I think it's safe to assume that the exponents are
> constant, but I'll
> add a check to handle potential negative values.
> 
>>
>>>>
>>>>> +             break;
>>>>> +     default:
>>>>> +             ret = -ENODATA;
>>>>> +             break;
>>>>> +     }
>>>>> +
>>>>> +     return ret;
>>>>> +}
>>>>> +
>>>>> +/*
>>>>> + * The vout under- and over-voltage limits are readonly for this chip.
>>>>> + */
>>>>
>>>> Not really. The BMR481 datasheet suggests that the value can be changed
>>>> by writing to MFR_OV_LIMIT_OFFSET and MFR_UV_LIMIT_OFFSET.
>>>> I am not saying that you should implement that if you don't want to,
>>>> but I would suggest a better (more accurate) explanation.
>>>
>>> I have looked into this a bit more and it's a bit more to it than I expected.
>>> The limits are actually dynamic values that will follow both commanded
>>> output voltage via the PMBus or SVID/AVSBus as well as current
>>> load (droop). To account for this I propose a mechanism to set the
>>
>> Yes, I noticed.
>>
>>> 'update' flag on selected sensors after auto-detection of limit attributes.
>>>
>>> Maybe add a function like this to pmbus_core that can be called
>>> after the probing is done?
>>>
>>> int pmbus_set_update(struct i2c_client *client, u8 reg)
>>> {
>>>         struct pmbus_data *data = i2c_get_clientdata(client);
>>>         struct pmbus_sensor *sensor;
>>>
>>>         for (sensor = data->sensors; sensor; sensor = sensor->next) {
>>>                 if (sensor->reg == reg) {
>>>                         sensor->update = true;
>>>                         return 0;
>>>                 }
>>>         }
>>>         return -ENXIO;
>>> }
>>>
>>
>> Add a boolean 'update' parameter (to be able to disable updates),
>> and make the function void. Also, remember that 'reg' may be repeated
>> on a chip with multiple pages, so you can't return after the first match.
>> Otherwise looks ok.
> 
> Ok
> 
>>
>>> I did also implemented writes to the limit registers via the offset
>>> registers but it might be
>>> a bit confusing to the user since the limits are set in relation to
>>> the current commanded
>>> output voltage and will change together with this. In the worst case,
>>> the voltage might
>>> change at the same time as the limit is written to, which will give
>>> unexpected results.
>>
>> Agreed, that can be tricky.
>>
>>> The alternative would be to just set these limits read-only. What is
>>> your opinion?
>>>
>>
>> Your call. Just add a note explaining why it is made read-only to explain
>> the reasoning for future readers.
> 
> Ok, I'll keep the functionality and add a note in the docs.
> 
> 
>>
>>> Also, I found a problem in how pmbus_set_sensor() works together with
>>> pmbus_clear_cache()
>>> as used in the zl6100 and lm25066 drivers. The new value is written to
>>> the sensor struct _after_
>>> the _pmbus_write_word_data() has returned and thus after
>>> pmbus_clear_cache() is called.
>>> The effect is that all values are cleared, except the one just written
>>> to, which I believe defeats
>>> the purpose of clearing the cache in the first place.> One solution would be to write the new value to sensor->data before
>>> the _pmbus_write_word_data
>>> is called and to restore the old value in case of error.
>>> I can make a separate patch for this if it seems like a reasonable solution?
>>>
>>
>> Good catch.
>>
>> An alternate and more generic solution might be to set sensor->data to
>> -ENODATA after updates. After all, it seems risky to assume that the chip
>> returns the value that was written (I have seen it often enough in other
>> drivers that this is not necessarily the case). That would also mean that
>> it would no longer be necessary to call pmbus_clear_cache() in the zl6100
>> and lm25066 drivers. Impact would be that a limit read after a write would
>> always be sent to the chip for all drivers, but that seems minimal compared
>> to the gain (and presumably limit registers are not frequently updated).
> 
> I agree that this is a more robust solution. I'll send a separate patch for this
> update. The zl6100 in fact still needs to clear the cache since a write to the
> fault limit also changes the value of the warning limit and vice versa. As far
> as I can see the usage in lm25066 can be removed however.
> 
>>
>> Thanks,
>> Guenter

Re: [PATCH 1/1] Add ST STPDDC60 pmbus driver

[Erik Rosen]

On Mon, Feb 1, 2021 at 10:38 PM Guenter Roeck <linux@roeck-us.net> wrote:
>
> On 2/1/21 8:48 AM, Erik Rosen wrote:
> > Hi Guenter
> >
> >
> > On Fri, Jan 29, 2021 at 4:50 PM Guenter Roeck <linux@roeck-us.net> wrote:
> >>
> >> Hi Erik,
> >>
> >> On 1/29/21 5:07 AM, Erik Rosen wrote:
> >> [ ... ]
> >>>>
> >>>>> +             break;
> >>>>> +     case PMBUS_VOUT_OV_FAULT_LIMIT:
> >>>>> +     case PMBUS_VOUT_UV_FAULT_LIMIT:
> >>>>> +             ret = pmbus_read_word_data(client, page, phase, reg);
> >>>>> +             if (ret < 0)
> >>>>> +                     return ret;
> >>>>> +             ret &= 0x07ff;
> >>>>
> >>>> This needs explanation. The BMR481 datasheet does not suggest that only
> >>>> 11 bits are valid.
> >>>
> >>> Ok, I will add a clarification. These registers use LINEAR11 whereas
> >>> LINEAR16 is expected for vout-related registers.
> >>>
> >> Is that the correct fix, then ? LINEAR11 means that the exponent is flexible,
> >> while it is fixed for LINEAR16. Just assuming that the exponents always match
> >> seems risky. Also, bit 11 in LINEAR11 is the sign bit, meaning negative limits
> >> (which seem at least theoretically be possible) would be reported as large
> >> positive values.
> >
> > The chip actually uses fixed exponents for representing all linear values and
> > the specification explicitly states the values of the LSB for all registers.
> > It also states that the limits can be handled as linear format when
> > writing  _if_
> > the exponent is the fixed value used for that limit. This means I'll have to
> > convert all limit writes to use the expected exponent.
>
> I understand the datasheet a bit differently. You are correct, the exponent for
> VOUT limits is always the same (LSB=0.00390625V, exponent -8), as it should be.
> But it also seems to me that the data format is LINEAR16, not LINEAR11.
> The datasheet says that the vout limit values are in bit 0..15.
>
> For other sensors, the datasheet is a bit ambiguous. I can read it as always
> using a fixed exponent when reading, or that it expects a fixed exponent when
> writing. It might make sense to check this with an actual chip instead of
> guessing. In this context, it is a bit odd that the datasheet keeps talking
> about a "System Register IOUT_EXP" without actually specifying it.
>
> In this context, you might want to watch out for register MFR_SVID_SLOW_SR_SELECTOR.
> Its value seems to impact the exponents used for IOUT and POUT.
>
> Also, I am not sure about the scale of other registers. It almost seems as if
> limit registers are LINEAR11, but actual READ_xxx registers are LINEAR16.
> For example, READ_IOUT is supposed to be bit 15:0 with the LSB determined
> by IOUT_EXP, but IOUT_OC_WARN_LIMIT is in bit 9:0. This would be a problem
> if READ_IOUT does not include the exponent, since it is interpreted as
> LINEAR11 and expects the exponent in bit 11..15. With an expected exponent
> of -1, the reported value would always be wrong. The same might apply
> to pretty much all READ_xxx registers (what a mess).

I have tested all commands on a real chip and the behavior is as follows:

All linear (both limits and READ_XXX) registers return the exponent i.e. use
the LINEAR11 format (even the vout related ones that really should use LINEAR16
according to the pmbus standard), so reading registers is quite straightforward.

I got hold of an excel-document from ST that describes the pmbus
commands and system registers of the chip in greater detail. Unfortunately
it has got 'STMicroelectronics Confidential' stamped all over it so I
can't really
share it publicly.

Iout and pout data and limits are described as:
[15:11]: Encoded/Decoded with N programmed by HC_SUPPORT bit or IOUT_EXP
[10]: 0b0
[9:0]: Y mantissa

Other data and limits (including vout) are described as:
[15:11]: Encoded/Decoded with N=X, LSB=XX
[10]: Don't care (returns 0 in read access)
[9:0]: Y; max XXX

where X... are fixed values. The sign bit is always zero.

You are correct in that it is possible to change the fixed exponents used for
iout and pout by manipulating the system registers accessible via
 extended commands.

However, in a footnote on the bottom of the page it says:
(***) Number format - When Linear, data need to be formatted with N specified.
Data sent with N different from what reported, will be decoded as if N
is the one
reported.

I can only interpret this as that the chip expects data written to a
linear register
to be encoded with the same exponent as it uses when the same register is read.

So the simplest way of ensuring that the correct exponent is used when
writing to
the chip seems to be to first read the value from the chip, extract the exponent
and then adjust the value to use this exponent before writing it.

/Erik
>
> Thanks,
> Guenter
>
> > Given this, I think it's safe to assume that the exponents are
> > constant, but I'll
> > add a check to handle potential negative values.
> >
> >>
> >>>>
> >>>>> +             break;
> >>>>> +     default:
> >>>>> +             ret = -ENODATA;
> >>>>> +             break;
> >>>>> +     }
> >>>>> +
> >>>>> +     return ret;
> >>>>> +}
> >>>>> +
> >>>>> +/*
> >>>>> + * The vout under- and over-voltage limits are readonly for this chip.
> >>>>> + */
> >>>>
> >>>> Not really. The BMR481 datasheet suggests that the value can be changed
> >>>> by writing to MFR_OV_LIMIT_OFFSET and MFR_UV_LIMIT_OFFSET.
> >>>> I am not saying that you should implement that if you don't want to,
> >>>> but I would suggest a better (more accurate) explanation.
> >>>
> >>> I have looked into this a bit more and it's a bit more to it than I expected.
> >>> The limits are actually dynamic values that will follow both commanded
> >>> output voltage via the PMBus or SVID/AVSBus as well as current
> >>> load (droop). To account for this I propose a mechanism to set the
> >>
> >> Yes, I noticed.
> >>
> >>> 'update' flag on selected sensors after auto-detection of limit attributes.
> >>>
> >>> Maybe add a function like this to pmbus_core that can be called
> >>> after the probing is done?
> >>>
> >>> int pmbus_set_update(struct i2c_client *client, u8 reg)
> >>> {
> >>>         struct pmbus_data *data = i2c_get_clientdata(client);
> >>>         struct pmbus_sensor *sensor;
> >>>
> >>>         for (sensor = data->sensors; sensor; sensor = sensor->next) {
> >>>                 if (sensor->reg == reg) {
> >>>                         sensor->update = true;
> >>>                         return 0;
> >>>                 }
> >>>         }
> >>>         return -ENXIO;
> >>> }
> >>>
> >>
> >> Add a boolean 'update' parameter (to be able to disable updates),
> >> and make the function void. Also, remember that 'reg' may be repeated
> >> on a chip with multiple pages, so you can't return after the first match.
> >> Otherwise looks ok.
> >
> > Ok
> >
> >>
> >>> I did also implemented writes to the limit registers via the offset
> >>> registers but it might be
> >>> a bit confusing to the user since the limits are set in relation to
> >>> the current commanded
> >>> output voltage and will change together with this. In the worst case,
> >>> the voltage might
> >>> change at the same time as the limit is written to, which will give
> >>> unexpected results.
> >>
> >> Agreed, that can be tricky.
> >>
> >>> The alternative would be to just set these limits read-only. What is
> >>> your opinion?
> >>>
> >>
> >> Your call. Just add a note explaining why it is made read-only to explain
> >> the reasoning for future readers.
> >
> > Ok, I'll keep the functionality and add a note in the docs.
> >
> >
> >>
> >>> Also, I found a problem in how pmbus_set_sensor() works together with
> >>> pmbus_clear_cache()
> >>> as used in the zl6100 and lm25066 drivers. The new value is written to
> >>> the sensor struct _after_
> >>> the _pmbus_write_word_data() has returned and thus after
> >>> pmbus_clear_cache() is called.
> >>> The effect is that all values are cleared, except the one just written
> >>> to, which I believe defeats
> >>> the purpose of clearing the cache in the first place.> One solution would be to write the new value to sensor->data before
> >>> the _pmbus_write_word_data
> >>> is called and to restore the old value in case of error.
> >>> I can make a separate patch for this if it seems like a reasonable solution?
> >>>
> >>
> >> Good catch.
> >>
> >> An alternate and more generic solution might be to set sensor->data to
> >> -ENODATA after updates. After all, it seems risky to assume that the chip
> >> returns the value that was written (I have seen it often enough in other
> >> drivers that this is not necessarily the case). That would also mean that
> >> it would no longer be necessary to call pmbus_clear_cache() in the zl6100
> >> and lm25066 drivers. Impact would be that a limit read after a write would
> >> always be sent to the chip for all drivers, but that seems minimal compared
> >> to the gain (and presumably limit registers are not frequently updated).
> >
> > I agree that this is a more robust solution. I'll send a separate patch for this
> > update. The zl6100 in fact still needs to clear the cache since a write to the
> > fault limit also changes the value of the warning limit and vice versa. As far
> > as I can see the usage in lm25066 can be removed however.
> >
> >>
> >> Thanks,
> >> Guenter
>

Re: [PATCH 1/1] Add ST STPDDC60 pmbus driver

[Guenter Roeck]

On 2/2/21 6:34 AM, Erik Rosen wrote:
> On Mon, Feb 1, 2021 at 10:38 PM Guenter Roeck <linux@roeck-us.net> wrote:
>>
>> On 2/1/21 8:48 AM, Erik Rosen wrote:
>>> Hi Guenter
>>>
>>>
>>> On Fri, Jan 29, 2021 at 4:50 PM Guenter Roeck <linux@roeck-us.net> wrote:
>>>>
>>>> Hi Erik,
>>>>
>>>> On 1/29/21 5:07 AM, Erik Rosen wrote:
>>>> [ ... ]
>>>>>>
>>>>>>> +             break;
>>>>>>> +     case PMBUS_VOUT_OV_FAULT_LIMIT:
>>>>>>> +     case PMBUS_VOUT_UV_FAULT_LIMIT:
>>>>>>> +             ret = pmbus_read_word_data(client, page, phase, reg);
>>>>>>> +             if (ret < 0)
>>>>>>> +                     return ret;
>>>>>>> +             ret &= 0x07ff;
>>>>>>
>>>>>> This needs explanation. The BMR481 datasheet does not suggest that only
>>>>>> 11 bits are valid.
>>>>>
>>>>> Ok, I will add a clarification. These registers use LINEAR11 whereas
>>>>> LINEAR16 is expected for vout-related registers.
>>>>>
>>>> Is that the correct fix, then ? LINEAR11 means that the exponent is flexible,
>>>> while it is fixed for LINEAR16. Just assuming that the exponents always match
>>>> seems risky. Also, bit 11 in LINEAR11 is the sign bit, meaning negative limits
>>>> (which seem at least theoretically be possible) would be reported as large
>>>> positive values.
>>>
>>> The chip actually uses fixed exponents for representing all linear values and
>>> the specification explicitly states the values of the LSB for all registers.
>>> It also states that the limits can be handled as linear format when
>>> writing  _if_
>>> the exponent is the fixed value used for that limit. This means I'll have to
>>> convert all limit writes to use the expected exponent.
>>
>> I understand the datasheet a bit differently. You are correct, the exponent for
>> VOUT limits is always the same (LSB=0.00390625V, exponent -8), as it should be.
>> But it also seems to me that the data format is LINEAR16, not LINEAR11.
>> The datasheet says that the vout limit values are in bit 0..15.
>>
>> For other sensors, the datasheet is a bit ambiguous. I can read it as always
>> using a fixed exponent when reading, or that it expects a fixed exponent when
>> writing. It might make sense to check this with an actual chip instead of
>> guessing. In this context, it is a bit odd that the datasheet keeps talking
>> about a "System Register IOUT_EXP" without actually specifying it.
>>
>> In this context, you might want to watch out for register MFR_SVID_SLOW_SR_SELECTOR.
>> Its value seems to impact the exponents used for IOUT and POUT.
>>
>> Also, I am not sure about the scale of other registers. It almost seems as if
>> limit registers are LINEAR11, but actual READ_xxx registers are LINEAR16.
>> For example, READ_IOUT is supposed to be bit 15:0 with the LSB determined
>> by IOUT_EXP, but IOUT_OC_WARN_LIMIT is in bit 9:0. This would be a problem
>> if READ_IOUT does not include the exponent, since it is interpreted as
>> LINEAR11 and expects the exponent in bit 11..15. With an expected exponent
>> of -1, the reported value would always be wrong. The same might apply
>> to pretty much all READ_xxx registers (what a mess).
> 
> I have tested all commands on a real chip and the behavior is as follows:
> 
> All linear (both limits and READ_XXX) registers return the exponent i.e. use
> the LINEAR11 format (even the vout related ones that really should use LINEAR16
> according to the pmbus standard), so reading registers is quite straightforward.
> 
> I got hold of an excel-document from ST that describes the pmbus
> commands and system registers of the chip in greater detail. Unfortunately
> it has got 'STMicroelectronics Confidential' stamped all over it so I
> can't really
> share it publicly.
> 
> Iout and pout data and limits are described as:
> [15:11]: Encoded/Decoded with N programmed by HC_SUPPORT bit or IOUT_EXP
> [10]: 0b0
> [9:0]: Y mantissa
> 
> Other data and limits (including vout) are described as:
> [15:11]: Encoded/Decoded with N=X, LSB=XX
> [10]: Don't care (returns 0 in read access)
> [9:0]: Y; max XXX
> 
> where X... are fixed values. The sign bit is always zero.
> 
> You are correct in that it is possible to change the fixed exponents used for
> iout and pout by manipulating the system registers accessible via
>  extended commands.
> 
> However, in a footnote on the bottom of the page it says:
> (***) Number format - When Linear, data need to be formatted with N specified.
> Data sent with N different from what reported, will be decoded as if N
> is the one
> reported.
> 

Some chip designers are really quite inventive.

> I can only interpret this as that the chip expects data written to a
> linear register
> to be encoded with the same exponent as it uses when the same register is read.
> 
> So the simplest way of ensuring that the correct exponent is used when
> writing to
> the chip seems to be to first read the value from the chip, extract the exponent
> and then adjust the value to use this exponent before writing it.
> 

Guess so, and the value written needs to be clamped to [0, 0x3ff].

Thanks,
Guenter