* [PATCH v2 0/6] hwmon: (pmbus/pim4328) Add pim4328 PMBus driver
@ 2021-05-22 10:55 Erik Rosen
2021-05-22 10:55 ` [PATCH v2 1/6] hwmon: (pmbus/pim4328) Add new pmbus flag NO_WRITE_PROTECT Erik Rosen
` (5 more replies)
0 siblings, 6 replies; 10+ messages in thread
From: Erik Rosen @ 2021-05-22 10:55 UTC (permalink / raw)
To: Jean Delvare, Guenter Roeck, Jonathan Corbet, linux-hwmon,
linux-doc, linux-kernel
Cc: Erik Rosen
Add hardware monitoring support for the Flex power interface modules
PIM4006, PIM4328 and PIM4820.
The modules are equipped with dual feed input and has support for
hotswap, holdup and various circuit protection functionality.
[PATCH 1/6]
The modules have no CAPABILITY or WRITE_PROTECT commands. If these
commands are read, the modules return invalid data (0xFF),
so in addition to the NO_CAPABILITY flag we need a NO_WRITE_PROTECT
flag to tell the pmbus_core driver to not access this register.
[PATCH 2/6]
PIM4328 and PIM4820 use the direct mode data format so a new function
is added to the pmbus_core driver to be able to read and decode
the COEFFICIENTS command.
[PATCH 3/6]
This is a tentative implementation of core driver support for reading
and decoding direct format coefficients. If the new flag
PMBUS_USE_COEFFICIENTS_CMD is set, the driver will use the
attribute information in the pmbus_sensor_attr structs together
with the COEFFICIENTS command to read and set the relevant
direct mode coefficients.
Please have a look and comment.
[PATCH 4/6]
The two inputs are modelled using virtual phases but there
is a limitation in the pmbus_core that disallows monitoring
of phase functions if there is no corresponding function on
the page level.
In this specific case the PIM4006 module allows
monitoring of current on each input separately,
but there is no corresponding command on the page level.
Is there a specific reason for this limitation?
Otherwise we suggest relaxing this criteria.
[PATCH 5/6]
All modules use manufacturer specific registers (mfr) for
status data and only supports the CML bit in the PMBus
STATUS register. The driver overrides reading the STATUS
register and maps the bits in the mfr registers to the STATUS
register alarm bits.
PATCH 6/6]
Add driver documentation
This patch has been tested with PIM4406, PIM4280 and PIM4328
modules.
v2
-Remove the for_reading parameter from the pmbus_read_coefficients
function.
-Use the correct namespace macro for the pmbus_read_coefficients
function.
-Fix alphabetic ordering of includes
-Remove override of STATUS_WORD since it will never get called by
the core driver.
-Add new patch with tentative implementation of core driver support
for reading direct mode coefficients using the COEFFICIENTS command.
Erik Rosen (6):
Add new pmbus flag NO_WRITE_PROTECT
Add function for reading direct mode coefficients
Add support for reading and decoding direct format coefficients
Allow phase function even if it does not exist not on the associated
page
Add PMBus driver for PIM4006, PIM4328 and PIM4820
Add documentation for the pim4328 PMBus driver
Documentation/hwmon/index.rst | 1 +
Documentation/hwmon/pim4328.rst | 105 ++++++++++++++
MAINTAINERS | 7 +
drivers/hwmon/pmbus/Kconfig | 9 ++
drivers/hwmon/pmbus/Makefile | 1 +
drivers/hwmon/pmbus/pim4328.c | 240 +++++++++++++++++++++++++++++++
drivers/hwmon/pmbus/pmbus.h | 4 +
drivers/hwmon/pmbus/pmbus_core.c | 156 ++++++++++++++++++--
include/linux/pmbus.h | 17 +++
9 files changed, 529 insertions(+), 11 deletions(-)
create mode 100644 Documentation/hwmon/pim4328.rst
create mode 100644 drivers/hwmon/pmbus/pim4328.c
base-commit: 6efb943b8616ec53a5e444193dccf1af9ad627b5
--
2.20.1
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH v2 1/6] hwmon: (pmbus/pim4328) Add new pmbus flag NO_WRITE_PROTECT
2021-05-22 10:55 [PATCH v2 0/6] hwmon: (pmbus/pim4328) Add pim4328 PMBus driver Erik Rosen
@ 2021-05-22 10:55 ` Erik Rosen
2021-05-22 10:55 ` [PATCH v2 2/6] hwmon: (pmbus/pim4328) Add function for reading direct mode coefficients Erik Rosen
` (4 subsequent siblings)
5 siblings, 0 replies; 10+ messages in thread
From: Erik Rosen @ 2021-05-22 10:55 UTC (permalink / raw)
To: Jean Delvare, Guenter Roeck, Jonathan Corbet, linux-hwmon,
linux-doc, linux-kernel
Cc: Erik Rosen
Some PMBus chips respond with invalid data when reading the WRITE_PROTECT
register. For such chips, this flag should be set so that the PMBus core
driver doesn't use the WRITE_PROTECT command to determine it's behavior.
Signed-off-by: Erik Rosen <erik.rosen@metormote.com>
---
drivers/hwmon/pmbus/pmbus_core.c | 9 ++++++---
include/linux/pmbus.h | 9 +++++++++
2 files changed, 15 insertions(+), 3 deletions(-)
diff --git a/drivers/hwmon/pmbus/pmbus_core.c b/drivers/hwmon/pmbus/pmbus_core.c
index bbd745178147..cb885efc4fba 100644
--- a/drivers/hwmon/pmbus/pmbus_core.c
+++ b/drivers/hwmon/pmbus/pmbus_core.c
@@ -2226,9 +2226,12 @@ static int pmbus_init_common(struct i2c_client *client, struct pmbus_data *data,
* faults, and we should not try it. Also, in that case, writes into
* limit registers need to be disabled.
*/
- ret = i2c_smbus_read_byte_data(client, PMBUS_WRITE_PROTECT);
- if (ret > 0 && (ret & PB_WP_ANY))
- data->flags |= PMBUS_WRITE_PROTECTED | PMBUS_SKIP_STATUS_CHECK;
+ if (!(data->flags & PMBUS_NO_WRITE_PROTECT)) {
+ ret = i2c_smbus_read_byte_data(client, PMBUS_WRITE_PROTECT);
+ if (ret > 0 && (ret & PB_WP_ANY))
+ data->flags |= PMBUS_WRITE_PROTECTED
+ | PMBUS_SKIP_STATUS_CHECK;
+ }
if (data->info->pages)
pmbus_clear_faults(client);
diff --git a/include/linux/pmbus.h b/include/linux/pmbus.h
index 12cbbf305969..f720470b1bab 100644
--- a/include/linux/pmbus.h
+++ b/include/linux/pmbus.h
@@ -43,6 +43,15 @@
*/
#define PMBUS_NO_CAPABILITY BIT(2)
+/*
+ * PMBUS_NO_WRITE_PROTECT
+ *
+ * Some PMBus chips respond with invalid data when reading the WRITE_PROTECT
+ * register. For such chips, this flag should be set so that the PMBus core
+ * driver doesn't use the WRITE_PROTECT command to determine it's behavior.
+ */
+#define PMBUS_NO_WRITE_PROTECT BIT(4)
+
struct pmbus_platform_data {
u32 flags; /* Device specific flags */
--
2.20.1
^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH v2 2/6] hwmon: (pmbus/pim4328) Add function for reading direct mode coefficients
2021-05-22 10:55 [PATCH v2 0/6] hwmon: (pmbus/pim4328) Add pim4328 PMBus driver Erik Rosen
2021-05-22 10:55 ` [PATCH v2 1/6] hwmon: (pmbus/pim4328) Add new pmbus flag NO_WRITE_PROTECT Erik Rosen
@ 2021-05-22 10:55 ` Erik Rosen
2021-05-22 10:55 ` [PATCH v2 3/6] hwmon: (pmbus/pim4328) Add support for reading direct format coefficients Erik Rosen
` (3 subsequent siblings)
5 siblings, 0 replies; 10+ messages in thread
From: Erik Rosen @ 2021-05-22 10:55 UTC (permalink / raw)
To: Jean Delvare, Guenter Roeck, Jonathan Corbet, linux-hwmon,
linux-doc, linux-kernel
Cc: Erik Rosen
Add the function pmbus_read_coefficients to pmbus_core to be able to
read and decode the coefficients for the direct format for a certain
command and read/write direction.
Signed-off-by: Erik Rosen <erik.rosen@metormote.com>
---
drivers/hwmon/pmbus/pmbus.h | 4 ++++
drivers/hwmon/pmbus/pmbus_core.c | 38 ++++++++++++++++++++++++++++++++
2 files changed, 42 insertions(+)
diff --git a/drivers/hwmon/pmbus/pmbus.h b/drivers/hwmon/pmbus/pmbus.h
index 3968924f8533..d9ed9736b480 100644
--- a/drivers/hwmon/pmbus/pmbus.h
+++ b/drivers/hwmon/pmbus/pmbus.h
@@ -499,6 +499,10 @@ int pmbus_get_fan_rate_cached(struct i2c_client *client, int page, int id,
enum pmbus_fan_mode mode);
int pmbus_update_fan(struct i2c_client *client, int page, int id,
u8 config, u8 mask, u16 command);
+int pmbus_read_coefficients(struct i2c_client *client,
+ struct pmbus_driver_info *info,
+ enum pmbus_sensor_classes sensor_class,
+ u8 command);
struct dentry *pmbus_get_debugfs_dir(struct i2c_client *client);
#endif /* PMBUS_H */
diff --git a/drivers/hwmon/pmbus/pmbus_core.c b/drivers/hwmon/pmbus/pmbus_core.c
index cb885efc4fba..460cbfd716e4 100644
--- a/drivers/hwmon/pmbus/pmbus_core.c
+++ b/drivers/hwmon/pmbus/pmbus_core.c
@@ -301,6 +301,44 @@ int pmbus_update_fan(struct i2c_client *client, int page, int id,
}
EXPORT_SYMBOL_NS_GPL(pmbus_update_fan, PMBUS);
+/*
+ * Read the coefficients for direct mode.
+ */
+int pmbus_read_coefficients(struct i2c_client *client,
+ struct pmbus_driver_info *info,
+ enum pmbus_sensor_classes sensor_class,
+ u8 command)
+{
+ int rv;
+ union i2c_smbus_data data;
+ s8 R;
+ s16 m, b;
+
+ data.block[0] = 2;
+ data.block[1] = command;
+ data.block[2] = 0x01;
+
+ rv = i2c_smbus_xfer(client->adapter, client->addr, client->flags,
+ I2C_SMBUS_WRITE, PMBUS_COEFFICIENTS,
+ I2C_SMBUS_BLOCK_PROC_CALL, &data);
+
+ if (rv < 0)
+ return rv;
+
+ if (data.block[0] != 5)
+ return -EIO;
+
+ m = data.block[1] | (data.block[2] << 8);
+ b = data.block[3] | (data.block[4] << 8);
+ R = data.block[5];
+ info->m[sensor_class] = m;
+ info->b[sensor_class] = b;
+ info->R[sensor_class] = R;
+
+ return rv;
+}
+EXPORT_SYMBOL_NS_GPL(pmbus_read_coefficients, PMBUS);
+
int pmbus_read_word_data(struct i2c_client *client, int page, int phase, u8 reg)
{
int rv;
--
2.20.1
^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH v2 3/6] hwmon: (pmbus/pim4328) Add support for reading direct format coefficients
2021-05-22 10:55 [PATCH v2 0/6] hwmon: (pmbus/pim4328) Add pim4328 PMBus driver Erik Rosen
2021-05-22 10:55 ` [PATCH v2 1/6] hwmon: (pmbus/pim4328) Add new pmbus flag NO_WRITE_PROTECT Erik Rosen
2021-05-22 10:55 ` [PATCH v2 2/6] hwmon: (pmbus/pim4328) Add function for reading direct mode coefficients Erik Rosen
@ 2021-05-22 10:55 ` Erik Rosen
2021-05-22 13:41 ` Guenter Roeck
2021-05-22 10:55 ` [PATCH v2 4/6] hwmon: (pmbus/pim4328) Allow phase function even if it's not on page Erik Rosen
` (2 subsequent siblings)
5 siblings, 1 reply; 10+ messages in thread
From: Erik Rosen @ 2021-05-22 10:55 UTC (permalink / raw)
To: Jean Delvare, Guenter Roeck, Jonathan Corbet, linux-hwmon,
linux-doc, linux-kernel
Cc: Erik Rosen
Add support for reading and decoding direct format coefficients to
the PMBus core driver. If the new flag PMBUS_USE_COEFFICIENTS_CMD
is set, the driver will use the COEFFICIENTS register together with
the information in the pmbus_sensor_attr structs to initialize
relevant coefficients for the direct mode format.
Signed-off-by: Erik Rosen <erik.rosen@metormote.com>
---
drivers/hwmon/pmbus/pmbus_core.c | 93 ++++++++++++++++++++++++++++++++
include/linux/pmbus.h | 8 +++
2 files changed, 101 insertions(+)
diff --git a/drivers/hwmon/pmbus/pmbus_core.c b/drivers/hwmon/pmbus/pmbus_core.c
index 460cbfd716e4..03c169bf5633 100644
--- a/drivers/hwmon/pmbus/pmbus_core.c
+++ b/drivers/hwmon/pmbus/pmbus_core.c
@@ -2177,6 +2177,38 @@ static int pmbus_find_attributes(struct i2c_client *client,
return ret;
}
+static int pmbus_init_coefficients(struct i2c_client *client,
+ struct pmbus_data *data, int page,
+ enum pmbus_sensor_classes sensor_class,
+ const struct pmbus_sensor_attr *attrs,
+ int nattrs)
+{
+ int i, status;
+
+ for (i = 0; i < nattrs; i++) {
+ if (attrs->class == sensor_class &&
+ (attrs->func & data->info->func[page])) {
+ status = pmbus_read_coefficients(client,
+ (struct pmbus_driver_info *)data->info,
+ sensor_class,
+ attrs->reg);
+ if (status < 0) {
+ dev_err(&client->dev,
+ "Failed to read coefficients for register: %x\n",
+ attrs->reg);
+ return status;
+ }
+ return 0;
+ }
+ attrs++;
+ }
+
+ dev_err(&client->dev, "No coefficients found for register: %x\n",
+ attrs->reg);
+
+ return -ENODEV;
+}
+
/*
* Identify chip parameters.
* This function is called for all chips.
@@ -2185,6 +2217,7 @@ static int pmbus_identify_common(struct i2c_client *client,
struct pmbus_data *data, int page)
{
int vout_mode = -1;
+ int ret;
if (pmbus_check_byte_register(client, page, PMBUS_VOUT_MODE))
vout_mode = _pmbus_read_byte_data(client, page,
@@ -2214,6 +2247,66 @@ static int pmbus_identify_common(struct i2c_client *client,
}
}
+ if (data->flags & PMBUS_USE_COEFFICIENTS_CMD) {
+ if (!i2c_check_functionality(client->adapter,
+ I2C_FUNC_SMBUS_BLOCK_PROC_CALL))
+ return -ENODEV;
+
+ if (data->info->format[PSC_VOLTAGE_IN] == direct) {
+ ret = pmbus_init_coefficients(client, data, page,
+ PSC_VOLTAGE_IN,
+ voltage_attributes,
+ ARRAY_SIZE(voltage_attributes));
+ if (ret)
+ return ret;
+ }
+
+ if (data->info->format[PSC_VOLTAGE_OUT] == direct) {
+ ret = pmbus_init_coefficients(client, data, page,
+ PSC_VOLTAGE_OUT,
+ voltage_attributes,
+ ARRAY_SIZE(voltage_attributes));
+ if (ret)
+ return ret;
+ }
+
+ if (data->info->format[PSC_CURRENT_IN] == direct) {
+ ret = pmbus_init_coefficients(client, data, page,
+ PSC_CURRENT_IN,
+ current_attributes,
+ ARRAY_SIZE(current_attributes));
+ if (ret)
+ return ret;
+ }
+
+ if (data->info->format[PSC_CURRENT_OUT] == direct) {
+ ret = pmbus_init_coefficients(client, data, page,
+ PSC_CURRENT_OUT,
+ current_attributes,
+ ARRAY_SIZE(current_attributes));
+ if (ret)
+ return ret;
+ }
+
+ if (data->info->format[PSC_POWER] == direct) {
+ ret = pmbus_init_coefficients(client, data, page,
+ PSC_POWER,
+ power_attributes,
+ ARRAY_SIZE(power_attributes));
+ if (ret)
+ return ret;
+ }
+
+ if (data->info->format[PSC_TEMPERATURE] == direct) {
+ ret = pmbus_init_coefficients(client, data, page,
+ PSC_TEMPERATURE,
+ temp_attributes,
+ ARRAY_SIZE(temp_attributes));
+ if (ret)
+ return ret;
+ }
+ }
+
pmbus_clear_fault_page(client, page);
return 0;
}
diff --git a/include/linux/pmbus.h b/include/linux/pmbus.h
index f720470b1bab..7fdc282dab5a 100644
--- a/include/linux/pmbus.h
+++ b/include/linux/pmbus.h
@@ -52,6 +52,14 @@
*/
#define PMBUS_NO_WRITE_PROTECT BIT(4)
+/*
+ * PMBUS_USE_COEFFICIENTS_CMD
+ *
+ * When this flag is set the PMBus core driver will use the COEFFICIENTS
+ * register to initialize the coefficients for the direct mode format.
+ */
+#define PMBUS_USE_COEFFICIENTS_CMD BIT(5)
+
struct pmbus_platform_data {
u32 flags; /* Device specific flags */
--
2.20.1
^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH v2 4/6] hwmon: (pmbus/pim4328) Allow phase function even if it's not on page
2021-05-22 10:55 [PATCH v2 0/6] hwmon: (pmbus/pim4328) Add pim4328 PMBus driver Erik Rosen
` (2 preceding siblings ...)
2021-05-22 10:55 ` [PATCH v2 3/6] hwmon: (pmbus/pim4328) Add support for reading direct format coefficients Erik Rosen
@ 2021-05-22 10:55 ` Erik Rosen
2021-05-22 10:55 ` [PATCH v2 5/6] hwmon: (pmbus/pim4328) Add PMBus driver for PIM4006, PIM4328 and PIM4820 Erik Rosen
2021-05-22 10:55 ` [PATCH v2 6/6] hwmon: (pmbus/pim4328) Add documentation for the pim4328 PMBus driver Erik Rosen
5 siblings, 0 replies; 10+ messages in thread
From: Erik Rosen @ 2021-05-22 10:55 UTC (permalink / raw)
To: Jean Delvare, Guenter Roeck, Jonathan Corbet, linux-hwmon,
linux-doc, linux-kernel
Cc: Erik Rosen
Allow the use of a phase function even if it does not exist not on
the associated page.
Signed-off-by: Erik Rosen <erik.rosen@metormote.com>
---
drivers/hwmon/pmbus/pmbus_core.c | 16 ++++++++--------
1 file changed, 8 insertions(+), 8 deletions(-)
diff --git a/drivers/hwmon/pmbus/pmbus_core.c b/drivers/hwmon/pmbus/pmbus_core.c
index 3b3f1d83a1ef..73f0972be991 100644
--- a/drivers/hwmon/pmbus/pmbus_core.c
+++ b/drivers/hwmon/pmbus/pmbus_core.c
@@ -1365,14 +1365,14 @@ static int pmbus_add_sensor_attrs(struct i2c_client *client,
pages = paged ? info->pages : 1;
for (page = 0; page < pages; page++) {
- if (!(info->func[page] & attrs->func))
- continue;
- ret = pmbus_add_sensor_attrs_one(client, data, info,
- name, index, page,
- 0xff, attrs, paged);
- if (ret)
- return ret;
- index++;
+ if (info->func[page] & attrs->func) {
+ ret = pmbus_add_sensor_attrs_one(client, data, info,
+ name, index, page,
+ 0xff, attrs, paged);
+ if (ret)
+ return ret;
+ index++;
+ }
if (info->phases[page]) {
int phase;
--
2.20.1
^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH v2 5/6] hwmon: (pmbus/pim4328) Add PMBus driver for PIM4006, PIM4328 and PIM4820
2021-05-22 10:55 [PATCH v2 0/6] hwmon: (pmbus/pim4328) Add pim4328 PMBus driver Erik Rosen
` (3 preceding siblings ...)
2021-05-22 10:55 ` [PATCH v2 4/6] hwmon: (pmbus/pim4328) Allow phase function even if it's not on page Erik Rosen
@ 2021-05-22 10:55 ` Erik Rosen
2021-05-22 10:55 ` [PATCH v2 6/6] hwmon: (pmbus/pim4328) Add documentation for the pim4328 PMBus driver Erik Rosen
5 siblings, 0 replies; 10+ messages in thread
From: Erik Rosen @ 2021-05-22 10:55 UTC (permalink / raw)
To: Jean Delvare, Guenter Roeck, Jonathan Corbet, linux-hwmon,
linux-doc, linux-kernel
Cc: Erik Rosen
Add hardware monitoring support for Flex power interface modules PIM4006,
PIM4328 and PIM4820.
Signed-off-by: Erik Rosen <erik.rosen@metormote.com>
---
drivers/hwmon/pmbus/Kconfig | 9 ++
drivers/hwmon/pmbus/Makefile | 1 +
drivers/hwmon/pmbus/pim4328.c | 240 ++++++++++++++++++++++++++++++++++
3 files changed, 250 insertions(+)
create mode 100644 drivers/hwmon/pmbus/pim4328.c
diff --git a/drivers/hwmon/pmbus/Kconfig b/drivers/hwmon/pmbus/Kconfig
index 37a5c39784fa..001527c71269 100644
--- a/drivers/hwmon/pmbus/Kconfig
+++ b/drivers/hwmon/pmbus/Kconfig
@@ -257,6 +257,15 @@ config SENSORS_MP2975
This driver can also be built as a module. If so, the module will
be called mp2975.
+config SENSORS_PIM4328
+ tristate "Flex PIM4328 and compatibles"
+ help
+ If you say yes here you get hardware monitoring support for Flex
+ PIM4328, PIM4820 and PIM4006 Power Interface Modules.
+
+ This driver can also be built as a module. If so, the module will
+ be called pim4328.
+
config SENSORS_PM6764TR
tristate "ST PM6764TR"
help
diff --git a/drivers/hwmon/pmbus/Makefile b/drivers/hwmon/pmbus/Makefile
index f8dcc27cd56a..2a12397535ba 100644
--- a/drivers/hwmon/pmbus/Makefile
+++ b/drivers/hwmon/pmbus/Makefile
@@ -39,3 +39,4 @@ obj-$(CONFIG_SENSORS_UCD9000) += ucd9000.o
obj-$(CONFIG_SENSORS_UCD9200) += ucd9200.o
obj-$(CONFIG_SENSORS_XDPE122) += xdpe12284.o
obj-$(CONFIG_SENSORS_ZL6100) += zl6100.o
+obj-$(CONFIG_SENSORS_PIM4328) += pim4328.o
diff --git a/drivers/hwmon/pmbus/pim4328.c b/drivers/hwmon/pmbus/pim4328.c
new file mode 100644
index 000000000000..ead7419479ef
--- /dev/null
+++ b/drivers/hwmon/pmbus/pim4328.c
@@ -0,0 +1,240 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Hardware monitoring driver for PIM4006, PIM4328 and PIM4820
+ *
+ * Copyright (c) 2021 Flextronics International Sweden AB
+ */
+
+#include <linux/err.h>
+#include <linux/i2c.h>
+#include <linux/init.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/pmbus.h>
+#include <linux/slab.h>
+#include "pmbus.h"
+
+enum chips { pim4006, pim4328, pim4820 };
+
+struct pim4328_data {
+ enum chips id;
+ struct pmbus_driver_info info;
+};
+
+#define to_pim4328_data(x) container_of(x, struct pim4328_data, info)
+
+/* PIM4006 and PIM4328 */
+#define PIM4328_MFR_READ_VINA 0xd3
+#define PIM4328_MFR_READ_VINB 0xd4
+
+/* PIM4006 */
+#define PIM4328_MFR_READ_IINA 0xd6
+#define PIM4328_MFR_READ_IINB 0xd7
+#define PIM4328_MFR_FET_CHECKSTATUS 0xd9
+
+/* PIM4328 */
+#define PIM4328_MFR_STATUS_BITS 0xd5
+
+/* PIM4820 */
+#define PIM4328_MFR_READ_STATUS 0xd0
+
+static const struct i2c_device_id pim4328_id[] = {
+ {"bmr455", pim4328},
+ {"pim4006", pim4006},
+ {"pim4106", pim4006},
+ {"pim4206", pim4006},
+ {"pim4306", pim4006},
+ {"pim4328", pim4328},
+ {"pim4406", pim4006},
+ {"pim4820", pim4820},
+ {}
+};
+MODULE_DEVICE_TABLE(i2c, pim4328_id);
+
+static int pim4328_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_VIN:
+ if (phase != 0xff) {
+ ret = pmbus_read_word_data(client, page, phase,
+ phase == 0 ? PIM4328_MFR_READ_VINA
+ : PIM4328_MFR_READ_VINB);
+ } else {
+ ret = -ENODATA;
+ }
+ break;
+ case PMBUS_READ_IIN:
+ if (phase != 0xff) {
+ ret = pmbus_read_word_data(client, page, phase,
+ phase == 0 ? PIM4328_MFR_READ_IINA
+ : PIM4328_MFR_READ_IINB);
+ } else {
+ ret = -ENODATA;
+ }
+ break;
+ default:
+ ret = -ENODATA;
+ }
+
+ return ret;
+}
+
+static int pim4328_read_byte_data(struct i2c_client *client, int page, int reg)
+{
+ const struct pmbus_driver_info *info = pmbus_get_driver_info(client);
+ struct pim4328_data *data = to_pim4328_data(info);
+ int ret, status;
+
+ if (page > 0)
+ return -ENXIO;
+
+ switch (reg) {
+ case PMBUS_STATUS_BYTE:
+ ret = pmbus_read_byte_data(client, page, PMBUS_STATUS_BYTE);
+ if (ret >= 0) {
+ if (data->id == pim4006) {
+ status = pmbus_read_word_data(client, page, 0xff,
+ PIM4328_MFR_FET_CHECKSTATUS);
+ if (status > 0) {
+ if (status & 0x0630) /* Input UV */
+ ret |= 0x08;
+ }
+ } else if (data->id == pim4328) {
+ status = pmbus_read_byte_data(client, page,
+ PIM4328_MFR_STATUS_BITS);
+ if (status > 0) {
+ if (status & 0x04) /* Input UV */
+ ret |= 0x08;
+ if (status & 0x40) /* Output UV */
+ ret |= 0x80;
+ }
+ } else if (data->id == pim4820) {
+ status = pmbus_read_byte_data(client, page,
+ PIM4328_MFR_READ_STATUS);
+ if (status > 0) {
+ if (status & 0x05) /* Input OV or OC */
+ ret |= 0x01;
+ if (status & 0x1a) /* Input UV */
+ ret |= 0x08;
+ if (status & 0x40) /* OT */
+ ret |= 0x04;
+ }
+ }
+ }
+ break;
+ default:
+ ret = -ENODATA;
+ }
+
+ return ret;
+}
+
+static int pim4328_probe(struct i2c_client *client)
+{
+ int status;
+ u8 device_id[I2C_SMBUS_BLOCK_MAX + 1];
+ const struct i2c_device_id *mid;
+ struct pim4328_data *data;
+ struct pmbus_driver_info *info;
+ struct pmbus_platform_data *pdata;
+ struct device *dev = &client->dev;
+
+ if (!i2c_check_functionality(client->adapter,
+ I2C_FUNC_SMBUS_READ_BYTE_DATA
+ | I2C_FUNC_SMBUS_BLOCK_DATA))
+ return -ENODEV;
+
+ data = devm_kzalloc(&client->dev, sizeof(struct pim4328_data),
+ GFP_KERNEL);
+ if (!data)
+ return -ENOMEM;
+
+ 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 = pim4328_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;
+ }
+
+ if (strcmp(client->name, mid->name) != 0)
+ dev_notice(&client->dev,
+ "Device mismatch: Configured %s, detected %s\n",
+ client->name, mid->name);
+
+ data->id = mid->driver_data;
+ info = &data->info;
+ info->pages = 1;
+ info->read_byte_data = pim4328_read_byte_data;
+ info->read_word_data = pim4328_read_word_data;
+
+ switch (data->id) {
+ case pim4006:
+ info->phases[0] = 2;
+ info->func[0] = PMBUS_PHASE_VIRTUAL | PMBUS_HAVE_VIN
+ | PMBUS_HAVE_TEMP | PMBUS_HAVE_IOUT;
+ info->pfunc[0] = PMBUS_HAVE_VIN | PMBUS_HAVE_IIN;
+ info->pfunc[1] = PMBUS_HAVE_VIN | PMBUS_HAVE_IIN;
+ break;
+ case pim4328:
+ info->phases[0] = 2;
+ info->func[0] = PMBUS_PHASE_VIRTUAL
+ | PMBUS_HAVE_VCAP | PMBUS_HAVE_VIN
+ | PMBUS_HAVE_TEMP | PMBUS_HAVE_IOUT;
+ info->pfunc[0] = PMBUS_HAVE_VIN;
+ info->pfunc[1] = PMBUS_HAVE_VIN;
+ info->format[PSC_VOLTAGE_IN] = direct;
+ info->format[PSC_TEMPERATURE] = direct;
+ info->format[PSC_CURRENT_OUT] = direct;
+ break;
+ case pim4820:
+ info->func[0] = PMBUS_HAVE_VIN | PMBUS_HAVE_TEMP
+ | PMBUS_HAVE_IIN;
+ info->format[PSC_VOLTAGE_IN] = direct;
+ info->format[PSC_TEMPERATURE] = direct;
+ info->format[PSC_CURRENT_IN] = direct;
+ break;
+ default:
+ return -ENODEV;
+ }
+
+ pdata = devm_kzalloc(dev, sizeof(struct pmbus_platform_data),
+ GFP_KERNEL);
+ if (!pdata)
+ return -ENOMEM;
+
+ pdata->flags = PMBUS_NO_CAPABILITY | PMBUS_NO_WRITE_PROTECT;
+ if (data->id == pim4328 || data->id == pim4820)
+ pdata->flags |= PMBUS_USE_COEFFICIENTS_CMD;
+
+ dev->platform_data = pdata;
+
+ return pmbus_do_probe(client, info);
+}
+
+static struct i2c_driver pim4328_driver = {
+ .driver = {
+ .name = "pim4328",
+ },
+ .probe_new = pim4328_probe,
+ .id_table = pim4328_id,
+};
+
+module_i2c_driver(pim4328_driver);
+
+MODULE_AUTHOR("Erik Rosen <erik.rosen@metormote.com>");
+MODULE_DESCRIPTION("PMBus driver for PIM4006, PIM4328, PIM4820 power interface modules");
+MODULE_LICENSE("GPL");
+MODULE_IMPORT_NS(PMBUS);
--
2.20.1
^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH v2 6/6] hwmon: (pmbus/pim4328) Add documentation for the pim4328 PMBus driver
2021-05-22 10:55 [PATCH v2 0/6] hwmon: (pmbus/pim4328) Add pim4328 PMBus driver Erik Rosen
` (4 preceding siblings ...)
2021-05-22 10:55 ` [PATCH v2 5/6] hwmon: (pmbus/pim4328) Add PMBus driver for PIM4006, PIM4328 and PIM4820 Erik Rosen
@ 2021-05-22 10:55 ` Erik Rosen
5 siblings, 0 replies; 10+ messages in thread
From: Erik Rosen @ 2021-05-22 10:55 UTC (permalink / raw)
To: Jean Delvare, Guenter Roeck, Jonathan Corbet, linux-hwmon,
linux-doc, linux-kernel
Cc: Erik Rosen
Add documentation and index link for pim4328 PMBus driver.
Signed-off-by: Erik Rosen <erik.rosen@metormote.com>
---
Documentation/hwmon/index.rst | 1 +
Documentation/hwmon/pim4328.rst | 105 ++++++++++++++++++++++++++++++++
MAINTAINERS | 7 +++
3 files changed, 113 insertions(+)
create mode 100644 Documentation/hwmon/pim4328.rst
diff --git a/Documentation/hwmon/index.rst b/Documentation/hwmon/index.rst
index 9ed60fa84cbe..719625f8f755 100644
--- a/Documentation/hwmon/index.rst
+++ b/Documentation/hwmon/index.rst
@@ -150,6 +150,7 @@ Hardware Monitoring Kernel Drivers
pc87360
pc87427
pcf8591
+ pim4328
pm6764tr
pmbus
powr1220
diff --git a/Documentation/hwmon/pim4328.rst b/Documentation/hwmon/pim4328.rst
new file mode 100644
index 000000000000..70c9e7a6882c
--- /dev/null
+++ b/Documentation/hwmon/pim4328.rst
@@ -0,0 +1,105 @@
+.. SPDX-License-Identifier: GPL-2.0
+
+Kernel driver pim4328
+=====================
+
+Supported chips:
+
+ * Flex PIM4328
+
+ Prefix: 'pim4328', 'bmr455'
+
+ Addresses scanned: -
+
+ Datasheet:
+
+https://flexpowermodules.com/resources/fpm-techspec-pim4328
+
+ * Flex PIM4820
+
+ Prefixes: 'pim4820'
+
+ Addresses scanned: -
+
+ Datasheet: https://flexpowermodules.com/resources/fpm-techspec-pim4820
+
+ * Flex PIM4006, PIM4106, PIM4206, PIM4306, PIM4406
+
+ Prefixes: 'pim4006', 'pim4106', 'pim4206', 'pim4306', 'pim4406'
+
+ Addresses scanned: -
+
+ Datasheet: https://flexpowermodules.com/resources/fpm-techspec-pim4006
+
+Author: Erik Rosen <erik.rosen@metormote.com>
+
+
+Description
+-----------
+
+This driver supports hardware monitoring for Flex PIM4328 and
+compatible digital power interface 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.
+
+
+Platform data support
+---------------------
+
+The driver supports standard PMBus driver platform data.
+
+
+Sysfs entries
+-------------
+
+The following attributes are supported. All attributes are read-only.
+
+======================= ========================================================
+in1_label "vin"
+in1_input Measured input voltage.
+in1_alarm Input voltage alarm.
+
+in2_label "vin.0"
+in2_input Measured input voltage on input A.
+
+ PIM4328 and PIM4X06
+
+in3_label "vin.1"
+in3_input Measured input voltage on input B.
+
+ PIM4328 and PIM4X06
+
+in4_label "vcap"
+in4_input Measured voltage on holdup capacitor.
+
+ PIM4328
+
+curr1_label "iin.0"
+curr1_input Measured input current on input A.
+
+ PIM4X06
+
+curr2_label "iin.1"
+curr2_input Measured input current on input B.
+
+ PIM4X06
+
+currX_label "iout1"
+currX_input Measured output current.
+currX_alarm Output current alarm.
+
+ X is 1 for PIM4820, 3 otherwise.
+
+temp1_input Measured temperature.
+temp1_alarm High temperature alarm.
+======================= ========================================================
diff --git a/MAINTAINERS b/MAINTAINERS
index bd7aff0c120f..378a121d80f6 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -14416,6 +14416,13 @@ K: (?i)pidfd
K: (?i)clone3
K: \b(clone_args|kernel_clone_args)\b
+PIM4328 DRIVER
+M: Daniel Nilsson <daniel.nilsson@flex.com>
+L: linux-hwmon@vger.kernel.org
+S: Maintained
+F: Documentation/hwmon/pim4328.rst
+F: drivers/hwmon/pmbus/pim4328.c
+
PIN CONTROL SUBSYSTEM
M: Linus Walleij <linus.walleij@linaro.org>
L: linux-gpio@vger.kernel.org
--
2.20.1
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH v2 3/6] hwmon: (pmbus/pim4328) Add support for reading direct format coefficients
2021-05-22 10:55 ` [PATCH v2 3/6] hwmon: (pmbus/pim4328) Add support for reading direct format coefficients Erik Rosen
@ 2021-05-22 13:41 ` Guenter Roeck
2021-05-23 17:52 ` Erik Rosen
0 siblings, 1 reply; 10+ messages in thread
From: Guenter Roeck @ 2021-05-22 13:41 UTC (permalink / raw)
To: Erik Rosen, Jean Delvare, Jonathan Corbet, linux-hwmon,
linux-doc, linux-kernel
On 5/22/21 3:55 AM, Erik Rosen wrote:
> Add support for reading and decoding direct format coefficients to
> the PMBus core driver. If the new flag PMBUS_USE_COEFFICIENTS_CMD
> is set, the driver will use the COEFFICIENTS register together with
> the information in the pmbus_sensor_attr structs to initialize
> relevant coefficients for the direct mode format.
>
> Signed-off-by: Erik Rosen <erik.rosen@metormote.com>
> ---
> drivers/hwmon/pmbus/pmbus_core.c | 93 ++++++++++++++++++++++++++++++++
> include/linux/pmbus.h | 8 +++
> 2 files changed, 101 insertions(+)
>
> diff --git a/drivers/hwmon/pmbus/pmbus_core.c b/drivers/hwmon/pmbus/pmbus_core.c
> index 460cbfd716e4..03c169bf5633 100644
> --- a/drivers/hwmon/pmbus/pmbus_core.c
> +++ b/drivers/hwmon/pmbus/pmbus_core.c
> @@ -2177,6 +2177,38 @@ static int pmbus_find_attributes(struct i2c_client *client,
> return ret;
> }
>
> +static int pmbus_init_coefficients(struct i2c_client *client,
> + struct pmbus_data *data, int page,
This seems wrong. Coefficients are not maintained per page but per class,
and (re-)reading them for each supported page doesn't really add value or
even make sense.
> + enum pmbus_sensor_classes sensor_class,
> + const struct pmbus_sensor_attr *attrs,
> + int nattrs)
> +{
> + int i, status;
> +
> + for (i = 0; i < nattrs; i++) {
> + if (attrs->class == sensor_class &&
> + (attrs->func & data->info->func[page])) {
> + status = pmbus_read_coefficients(client,
> + (struct pmbus_driver_info *)data->info,
> + sensor_class,
> + attrs->reg);
> + if (status < 0) {
> + dev_err(&client->dev,
> + "Failed to read coefficients for register: %x\n",
> + attrs->reg);
> + return status;
> + }
> + return 0;
> + }
> + attrs++;
> + }
> +
> + dev_err(&client->dev, "No coefficients found for register: %x\n",
> + attrs->reg);
> +
attrs points beyond the array size here, so attrs->reg does not point
to a valid array element. The problem would also not be the register
this happens to point to, but the class (ie the chip does not support
a sensor of the requested class).
Not sure if this should trigger a message or error in the first place.
It won't matter since the chip will never need those coefficients.
If anything, this would be a misconfiguration (the driver should
not set direct format for this sensor class), and the return value
should be -EINVAL.
Either case, I wonder if this can be handled with less complex code,
ie without having to check data->info->func[] for all pages. How
about just walking through attrs and try all class matches until
one is found that works (ie not return on error but keep trying) ?
> + return -ENODEV;
> +}
> +
> /*
> * Identify chip parameters.
> * This function is called for all chips.
> @@ -2185,6 +2217,7 @@ static int pmbus_identify_common(struct i2c_client *client,
> struct pmbus_data *data, int page)
> {
> int vout_mode = -1;
> + int ret;
>
> if (pmbus_check_byte_register(client, page, PMBUS_VOUT_MODE))
> vout_mode = _pmbus_read_byte_data(client, page,
> @@ -2214,6 +2247,66 @@ static int pmbus_identify_common(struct i2c_client *client,
> }
> }
>
> + if (data->flags & PMBUS_USE_COEFFICIENTS_CMD) {
I think there should be a separate function to handle that,
to be called only once, not once per page.
> + if (!i2c_check_functionality(client->adapter,
> + I2C_FUNC_SMBUS_BLOCK_PROC_CALL))
> + return -ENODEV;
> +
> + if (data->info->format[PSC_VOLTAGE_IN] == direct) {
> + ret = pmbus_init_coefficients(client, data, page,
> + PSC_VOLTAGE_IN,
> + voltage_attributes,
> + ARRAY_SIZE(voltage_attributes));
> + if (ret)
> + return ret;
> + }
It might be useful to have a little structure with {class, attribute list pointer,
attribute list size} and walk through that in a loop instead of repeating essentially
the same code multiple times.
> +
> + if (data->info->format[PSC_VOLTAGE_OUT] == direct) {
> + ret = pmbus_init_coefficients(client, data, page,
> + PSC_VOLTAGE_OUT,
> + voltage_attributes,
> + ARRAY_SIZE(voltage_attributes));
> + if (ret)
> + return ret;
> + }
> +
> + if (data->info->format[PSC_CURRENT_IN] == direct) {
> + ret = pmbus_init_coefficients(client, data, page,
> + PSC_CURRENT_IN,
> + current_attributes,
> + ARRAY_SIZE(current_attributes));
> + if (ret)
> + return ret;
> + }
> +
> + if (data->info->format[PSC_CURRENT_OUT] == direct) {
> + ret = pmbus_init_coefficients(client, data, page,
> + PSC_CURRENT_OUT,
> + current_attributes,
> + ARRAY_SIZE(current_attributes));
> + if (ret)
> + return ret;
> + }
> +
> + if (data->info->format[PSC_POWER] == direct) {
> + ret = pmbus_init_coefficients(client, data, page,
> + PSC_POWER,
> + power_attributes,
> + ARRAY_SIZE(power_attributes));
> + if (ret)
> + return ret;
> + }
> +
> + if (data->info->format[PSC_TEMPERATURE] == direct) {
> + ret = pmbus_init_coefficients(client, data, page,
> + PSC_TEMPERATURE,
> + temp_attributes,
> + ARRAY_SIZE(temp_attributes));
> + if (ret)
> + return ret;
> + }
> + }
> +
> pmbus_clear_fault_page(client, page);
> return 0;
> }
> diff --git a/include/linux/pmbus.h b/include/linux/pmbus.h
> index f720470b1bab..7fdc282dab5a 100644
> --- a/include/linux/pmbus.h
> +++ b/include/linux/pmbus.h
> @@ -52,6 +52,14 @@
> */
> #define PMBUS_NO_WRITE_PROTECT BIT(4)
>
> +/*
> + * PMBUS_USE_COEFFICIENTS_CMD
> + *
> + * When this flag is set the PMBus core driver will use the COEFFICIENTS
> + * register to initialize the coefficients for the direct mode format.
> + */
> +#define PMBUS_USE_COEFFICIENTS_CMD BIT(5)
> +
> struct pmbus_platform_data {
> u32 flags; /* Device specific flags */
>
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2 3/6] hwmon: (pmbus/pim4328) Add support for reading direct format coefficients
2021-05-22 13:41 ` Guenter Roeck
@ 2021-05-23 17:52 ` Erik Rosen
2021-05-23 19:47 ` Guenter Roeck
0 siblings, 1 reply; 10+ messages in thread
From: Erik Rosen @ 2021-05-23 17:52 UTC (permalink / raw)
To: Guenter Roeck
Cc: Jean Delvare, Jonathan Corbet, linux-hwmon, linux-doc, linux-kernel
On Sat, May 22, 2021 at 3:41 PM Guenter Roeck <linux@roeck-us.net> wrote:
>
> On 5/22/21 3:55 AM, Erik Rosen wrote:
> > Add support for reading and decoding direct format coefficients to
> > the PMBus core driver. If the new flag PMBUS_USE_COEFFICIENTS_CMD
> > is set, the driver will use the COEFFICIENTS register together with
> > the information in the pmbus_sensor_attr structs to initialize
> > relevant coefficients for the direct mode format.
> >
> > Signed-off-by: Erik Rosen <erik.rosen@metormote.com>
> > ---
> > drivers/hwmon/pmbus/pmbus_core.c | 93 ++++++++++++++++++++++++++++++++
> > include/linux/pmbus.h | 8 +++
> > 2 files changed, 101 insertions(+)
> >
> > diff --git a/drivers/hwmon/pmbus/pmbus_core.c b/drivers/hwmon/pmbus/pmbus_core.c
> > index 460cbfd716e4..03c169bf5633 100644
> > --- a/drivers/hwmon/pmbus/pmbus_core.c
> > +++ b/drivers/hwmon/pmbus/pmbus_core.c
> > @@ -2177,6 +2177,38 @@ static int pmbus_find_attributes(struct i2c_client *client,
> > return ret;
> > }
> >
> > +static int pmbus_init_coefficients(struct i2c_client *client,
> > + struct pmbus_data *data, int page,
>
> This seems wrong. Coefficients are not maintained per page but per class,
> and (re-)reading them for each supported page doesn't really add value or
> even make sense.
>
> > + enum pmbus_sensor_classes sensor_class,
> > + const struct pmbus_sensor_attr *attrs,
> > + int nattrs)
> > +{
> > + int i, status;
> > +
> > + for (i = 0; i < nattrs; i++) {
> > + if (attrs->class == sensor_class &&
> > + (attrs->func & data->info->func[page])) {
> > + status = pmbus_read_coefficients(client,
> > + (struct pmbus_driver_info *)data->info,
> > + sensor_class,
> > + attrs->reg);
> > + if (status < 0) {
> > + dev_err(&client->dev,
> > + "Failed to read coefficients for register: %x\n",
> > + attrs->reg);
> > + return status;
> > + }
> > + return 0;
> > + }
> > + attrs++;
> > + }
> > +
> > + dev_err(&client->dev, "No coefficients found for register: %x\n",
> > + attrs->reg);
> > +
>
> attrs points beyond the array size here, so attrs->reg does not point
> to a valid array element. The problem would also not be the register
> this happens to point to, but the class (ie the chip does not support
> a sensor of the requested class).
>
> Not sure if this should trigger a message or error in the first place.
> It won't matter since the chip will never need those coefficients.
> If anything, this would be a misconfiguration (the driver should
> not set direct format for this sensor class), and the return value
> should be -EINVAL.
>
> Either case, I wonder if this can be handled with less complex code,
> ie without having to check data->info->func[] for all pages. How
> about just walking through attrs and try all class matches until
> one is found that works (ie not return on error but keep trying) ?
Ok, I'll send a new version based on your comments.
I'm not entirely comfortable with just silently ignoring any failure to
retrieve the coefficients for a sensor class. I mean it could be due to any
reason; a bus error for instance. I'll return a -EINVAL for now if you don't
disagree.
/Erik
>
> > + return -ENODEV;
> > +}
> > +
> > /*
> > * Identify chip parameters.
> > * This function is called for all chips.
> > @@ -2185,6 +2217,7 @@ static int pmbus_identify_common(struct i2c_client *client,
> > struct pmbus_data *data, int page)
> > {
> > int vout_mode = -1;
> > + int ret;
> >
> > if (pmbus_check_byte_register(client, page, PMBUS_VOUT_MODE))
> > vout_mode = _pmbus_read_byte_data(client, page,
> > @@ -2214,6 +2247,66 @@ static int pmbus_identify_common(struct i2c_client *client,
> > }
> > }
> >
> > + if (data->flags & PMBUS_USE_COEFFICIENTS_CMD) {
>
> I think there should be a separate function to handle that,
> to be called only once, not once per page.
>
> > + if (!i2c_check_functionality(client->adapter,
> > + I2C_FUNC_SMBUS_BLOCK_PROC_CALL))
> > + return -ENODEV;
> > +
> > + if (data->info->format[PSC_VOLTAGE_IN] == direct) {
> > + ret = pmbus_init_coefficients(client, data, page,
> > + PSC_VOLTAGE_IN,
> > + voltage_attributes,
> > + ARRAY_SIZE(voltage_attributes));
> > + if (ret)
> > + return ret;
> > + }
>
> It might be useful to have a little structure with {class, attribute list pointer,
> attribute list size} and walk through that in a loop instead of repeating essentially
> the same code multiple times.
>
> > +
> > + if (data->info->format[PSC_VOLTAGE_OUT] == direct) {
> > + ret = pmbus_init_coefficients(client, data, page,
> > + PSC_VOLTAGE_OUT,
> > + voltage_attributes,
> > + ARRAY_SIZE(voltage_attributes));
> > + if (ret)
> > + return ret;
> > + }
> > +
> > + if (data->info->format[PSC_CURRENT_IN] == direct) {
> > + ret = pmbus_init_coefficients(client, data, page,
> > + PSC_CURRENT_IN,
> > + current_attributes,
> > + ARRAY_SIZE(current_attributes));
> > + if (ret)
> > + return ret;
> > + }
> > +
> > + if (data->info->format[PSC_CURRENT_OUT] == direct) {
> > + ret = pmbus_init_coefficients(client, data, page,
> > + PSC_CURRENT_OUT,
> > + current_attributes,
> > + ARRAY_SIZE(current_attributes));
> > + if (ret)
> > + return ret;
> > + }
> > +
> > + if (data->info->format[PSC_POWER] == direct) {
> > + ret = pmbus_init_coefficients(client, data, page,
> > + PSC_POWER,
> > + power_attributes,
> > + ARRAY_SIZE(power_attributes));
> > + if (ret)
> > + return ret;
> > + }
> > +
> > + if (data->info->format[PSC_TEMPERATURE] == direct) {
> > + ret = pmbus_init_coefficients(client, data, page,
> > + PSC_TEMPERATURE,
> > + temp_attributes,
> > + ARRAY_SIZE(temp_attributes));
> > + if (ret)
> > + return ret;
> > + }
> > + }
> > +
> > pmbus_clear_fault_page(client, page);
> > return 0;
> > }
> > diff --git a/include/linux/pmbus.h b/include/linux/pmbus.h
> > index f720470b1bab..7fdc282dab5a 100644
> > --- a/include/linux/pmbus.h
> > +++ b/include/linux/pmbus.h
> > @@ -52,6 +52,14 @@
> > */
> > #define PMBUS_NO_WRITE_PROTECT BIT(4)
> >
> > +/*
> > + * PMBUS_USE_COEFFICIENTS_CMD
> > + *
> > + * When this flag is set the PMBus core driver will use the COEFFICIENTS
> > + * register to initialize the coefficients for the direct mode format.
> > + */
> > +#define PMBUS_USE_COEFFICIENTS_CMD BIT(5)
> > +
> > struct pmbus_platform_data {
> > u32 flags; /* Device specific flags */
> >
> >
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2 3/6] hwmon: (pmbus/pim4328) Add support for reading direct format coefficients
2021-05-23 17:52 ` Erik Rosen
@ 2021-05-23 19:47 ` Guenter Roeck
0 siblings, 0 replies; 10+ messages in thread
From: Guenter Roeck @ 2021-05-23 19:47 UTC (permalink / raw)
To: Erik Rosen
Cc: Jean Delvare, Jonathan Corbet, linux-hwmon, linux-doc, linux-kernel
On 5/23/21 10:52 AM, Erik Rosen wrote:
> On Sat, May 22, 2021 at 3:41 PM Guenter Roeck <linux@roeck-us.net> wrote:
>>
>> On 5/22/21 3:55 AM, Erik Rosen wrote:
>>> Add support for reading and decoding direct format coefficients to
>>> the PMBus core driver. If the new flag PMBUS_USE_COEFFICIENTS_CMD
>>> is set, the driver will use the COEFFICIENTS register together with
>>> the information in the pmbus_sensor_attr structs to initialize
>>> relevant coefficients for the direct mode format.
>>>
>>> Signed-off-by: Erik Rosen <erik.rosen@metormote.com>
>>> ---
>>> drivers/hwmon/pmbus/pmbus_core.c | 93 ++++++++++++++++++++++++++++++++
>>> include/linux/pmbus.h | 8 +++
>>> 2 files changed, 101 insertions(+)
>>>
>>> diff --git a/drivers/hwmon/pmbus/pmbus_core.c b/drivers/hwmon/pmbus/pmbus_core.c
>>> index 460cbfd716e4..03c169bf5633 100644
>>> --- a/drivers/hwmon/pmbus/pmbus_core.c
>>> +++ b/drivers/hwmon/pmbus/pmbus_core.c
>>> @@ -2177,6 +2177,38 @@ static int pmbus_find_attributes(struct i2c_client *client,
>>> return ret;
>>> }
>>>
>>> +static int pmbus_init_coefficients(struct i2c_client *client,
>>> + struct pmbus_data *data, int page,
>>
>> This seems wrong. Coefficients are not maintained per page but per class,
>> and (re-)reading them for each supported page doesn't really add value or
>> even make sense.
>>
>>> + enum pmbus_sensor_classes sensor_class,
>>> + const struct pmbus_sensor_attr *attrs,
>>> + int nattrs)
>>> +{
>>> + int i, status;
>>> +
>>> + for (i = 0; i < nattrs; i++) {
>>> + if (attrs->class == sensor_class &&
>>> + (attrs->func & data->info->func[page])) {
>>> + status = pmbus_read_coefficients(client,
>>> + (struct pmbus_driver_info *)data->info,
>>> + sensor_class,
>>> + attrs->reg);
>>> + if (status < 0) {
>>> + dev_err(&client->dev,
>>> + "Failed to read coefficients for register: %x\n",
>>> + attrs->reg);
>>> + return status;
>>> + }
>>> + return 0;
>>> + }
>>> + attrs++;
>>> + }
>>> +
>>> + dev_err(&client->dev, "No coefficients found for register: %x\n",
>>> + attrs->reg);
>>> +
>>
>> attrs points beyond the array size here, so attrs->reg does not point
>> to a valid array element. The problem would also not be the register
>> this happens to point to, but the class (ie the chip does not support
>> a sensor of the requested class).
>>
>> Not sure if this should trigger a message or error in the first place.
>> It won't matter since the chip will never need those coefficients.
>> If anything, this would be a misconfiguration (the driver should
>> not set direct format for this sensor class), and the return value
>> should be -EINVAL.
>>
>> Either case, I wonder if this can be handled with less complex code,
>> ie without having to check data->info->func[] for all pages. How
>> about just walking through attrs and try all class matches until
>> one is found that works (ie not return on error but keep trying) ?
>
> Ok, I'll send a new version based on your comments.
> I'm not entirely comfortable with just silently ignoring any failure to
> retrieve the coefficients for a sensor class. I mean it could be due to any
> reason; a bus error for instance. I'll return a -EINVAL for now if you don't
> disagree.
>
Ok. After all, it does suggest a misconfiguration.
Thanks,
Guenter
> /Erik
>
>>
>>> + return -ENODEV;
>>> +}
>>> +
>>> /*
>>> * Identify chip parameters.
>>> * This function is called for all chips.
>>> @@ -2185,6 +2217,7 @@ static int pmbus_identify_common(struct i2c_client *client,
>>> struct pmbus_data *data, int page)
>>> {
>>> int vout_mode = -1;
>>> + int ret;
>>>
>>> if (pmbus_check_byte_register(client, page, PMBUS_VOUT_MODE))
>>> vout_mode = _pmbus_read_byte_data(client, page,
>>> @@ -2214,6 +2247,66 @@ static int pmbus_identify_common(struct i2c_client *client,
>>> }
>>> }
>>>
>>> + if (data->flags & PMBUS_USE_COEFFICIENTS_CMD) {
>>
>> I think there should be a separate function to handle that,
>> to be called only once, not once per page.
>>
>>> + if (!i2c_check_functionality(client->adapter,
>>> + I2C_FUNC_SMBUS_BLOCK_PROC_CALL))
>>> + return -ENODEV;
>>> +
>>> + if (data->info->format[PSC_VOLTAGE_IN] == direct) {
>>> + ret = pmbus_init_coefficients(client, data, page,
>>> + PSC_VOLTAGE_IN,
>>> + voltage_attributes,
>>> + ARRAY_SIZE(voltage_attributes));
>>> + if (ret)
>>> + return ret;
>>> + }
>>
>> It might be useful to have a little structure with {class, attribute list pointer,
>> attribute list size} and walk through that in a loop instead of repeating essentially
>> the same code multiple times.
>>
>>> +
>>> + if (data->info->format[PSC_VOLTAGE_OUT] == direct) {
>>> + ret = pmbus_init_coefficients(client, data, page,
>>> + PSC_VOLTAGE_OUT,
>>> + voltage_attributes,
>>> + ARRAY_SIZE(voltage_attributes));
>>> + if (ret)
>>> + return ret;
>>> + }
>>> +
>>> + if (data->info->format[PSC_CURRENT_IN] == direct) {
>>> + ret = pmbus_init_coefficients(client, data, page,
>>> + PSC_CURRENT_IN,
>>> + current_attributes,
>>> + ARRAY_SIZE(current_attributes));
>>> + if (ret)
>>> + return ret;
>>> + }
>>> +
>>> + if (data->info->format[PSC_CURRENT_OUT] == direct) {
>>> + ret = pmbus_init_coefficients(client, data, page,
>>> + PSC_CURRENT_OUT,
>>> + current_attributes,
>>> + ARRAY_SIZE(current_attributes));
>>> + if (ret)
>>> + return ret;
>>> + }
>>> +
>>> + if (data->info->format[PSC_POWER] == direct) {
>>> + ret = pmbus_init_coefficients(client, data, page,
>>> + PSC_POWER,
>>> + power_attributes,
>>> + ARRAY_SIZE(power_attributes));
>>> + if (ret)
>>> + return ret;
>>> + }
>>> +
>>> + if (data->info->format[PSC_TEMPERATURE] == direct) {
>>> + ret = pmbus_init_coefficients(client, data, page,
>>> + PSC_TEMPERATURE,
>>> + temp_attributes,
>>> + ARRAY_SIZE(temp_attributes));
>>> + if (ret)
>>> + return ret;
>>> + }
>>> + }
>>> +
>>> pmbus_clear_fault_page(client, page);
>>> return 0;
>>> }
>>> diff --git a/include/linux/pmbus.h b/include/linux/pmbus.h
>>> index f720470b1bab..7fdc282dab5a 100644
>>> --- a/include/linux/pmbus.h
>>> +++ b/include/linux/pmbus.h
>>> @@ -52,6 +52,14 @@
>>> */
>>> #define PMBUS_NO_WRITE_PROTECT BIT(4)
>>>
>>> +/*
>>> + * PMBUS_USE_COEFFICIENTS_CMD
>>> + *
>>> + * When this flag is set the PMBus core driver will use the COEFFICIENTS
>>> + * register to initialize the coefficients for the direct mode format.
>>> + */
>>> +#define PMBUS_USE_COEFFICIENTS_CMD BIT(5)
>>> +
>>> struct pmbus_platform_data {
>>> u32 flags; /* Device specific flags */
>>>
>>>
>>
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2021-05-23 19:47 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-05-22 10:55 [PATCH v2 0/6] hwmon: (pmbus/pim4328) Add pim4328 PMBus driver Erik Rosen
2021-05-22 10:55 ` [PATCH v2 1/6] hwmon: (pmbus/pim4328) Add new pmbus flag NO_WRITE_PROTECT Erik Rosen
2021-05-22 10:55 ` [PATCH v2 2/6] hwmon: (pmbus/pim4328) Add function for reading direct mode coefficients Erik Rosen
2021-05-22 10:55 ` [PATCH v2 3/6] hwmon: (pmbus/pim4328) Add support for reading direct format coefficients Erik Rosen
2021-05-22 13:41 ` Guenter Roeck
2021-05-23 17:52 ` Erik Rosen
2021-05-23 19:47 ` Guenter Roeck
2021-05-22 10:55 ` [PATCH v2 4/6] hwmon: (pmbus/pim4328) Allow phase function even if it's not on page Erik Rosen
2021-05-22 10:55 ` [PATCH v2 5/6] hwmon: (pmbus/pim4328) Add PMBus driver for PIM4006, PIM4328 and PIM4820 Erik Rosen
2021-05-22 10:55 ` [PATCH v2 6/6] hwmon: (pmbus/pim4328) Add documentation for the pim4328 PMBus driver Erik Rosen
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.