linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] hwmon: (pmbus) Add support for 2nd Gen Renesas digital multiphase
@ 2020-02-28 21:23 Grant Peltier
  2020-02-28 22:58 ` Guenter Roeck
  0 siblings, 1 reply; 6+ messages in thread
From: Grant Peltier @ 2020-02-28 21:23 UTC (permalink / raw)
  To: linux, linux-hwmon, linux-kernel, openbmc; +Cc: grantpeltier93, adam.vaughn.xh

Add a driver to support 2nd generation Renesas digital multiphase power
regulators. The driver is meant to support a large family of part
numbers spanning isl682xx, isl692xx, and some raa228/9 part designations.

Signed-off-by: Grant Peltier <grantpeltier93@gmail.com>
---
 drivers/hwmon/pmbus/Kconfig    |   9 +
 drivers/hwmon/pmbus/Makefile   |   1 +
 drivers/hwmon/pmbus/isl692xx.c | 352 +++++++++++++++++++++++++++++++++
 3 files changed, 362 insertions(+)
 create mode 100644 drivers/hwmon/pmbus/isl692xx.c

diff --git a/drivers/hwmon/pmbus/Kconfig b/drivers/hwmon/pmbus/Kconfig
index a9ea06204767..fbe7bbc8b37c 100644
--- a/drivers/hwmon/pmbus/Kconfig
+++ b/drivers/hwmon/pmbus/Kconfig
@@ -100,6 +100,15 @@ config SENSORS_ISL68137
 	  This driver can also be built as a module. If so, the module will
 	  be called isl68137.
 
+config SENSORS_ISL692XX
+        tristate "Renesas 2nd Gen Digital Multiphase"
+        help
+          If you say yes here you get hardware monitoring support for Renesas
+          2nd Generation Digital Multiphase power regulators.
+
+          This driver can also be built as a module. If so, the module will
+          be called isl692xx.
+
 config SENSORS_LM25066
 	tristate "National Semiconductor LM25066 and compatibles"
 	help
diff --git a/drivers/hwmon/pmbus/Makefile b/drivers/hwmon/pmbus/Makefile
index 5feb45806123..bf1ea99a6120 100644
--- a/drivers/hwmon/pmbus/Makefile
+++ b/drivers/hwmon/pmbus/Makefile
@@ -13,6 +13,7 @@ obj-$(CONFIG_SENSORS_IR35221)	+= ir35221.o
 obj-$(CONFIG_SENSORS_IR38064)	+= ir38064.o
 obj-$(CONFIG_SENSORS_IRPS5401)	+= irps5401.o
 obj-$(CONFIG_SENSORS_ISL68137)	+= isl68137.o
+obj-$(CONFIG_SENSORS_ISL692XX)	+= isl692xx.o
 obj-$(CONFIG_SENSORS_LM25066)	+= lm25066.o
 obj-$(CONFIG_SENSORS_LTC2978)	+= ltc2978.o
 obj-$(CONFIG_SENSORS_LTC3815)	+= ltc3815.o
diff --git a/drivers/hwmon/pmbus/isl692xx.c b/drivers/hwmon/pmbus/isl692xx.c
new file mode 100644
index 000000000000..26f3d90a7ddc
--- /dev/null
+++ b/drivers/hwmon/pmbus/isl692xx.c
@@ -0,0 +1,352 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Hardware monitoring driver for Renesas Gen 2 Digital Multiphase Devices
+ *
+ * Copyright (c) 2020 Renesas Electronics America
+ */
+
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/init.h>
+#include <linux/err.h>
+#include <linux/i2c.h>
+
+#include "pmbus.h"
+
+#define ISL692XX_READ_VMON        0xc8
+
+enum parts {
+        isl68220,
+        isl68221,
+        isl68222,
+        isl68223,
+        isl68224,
+        isl68225,
+        isl68226,
+        isl68227,
+        isl68229,
+        isl68233,
+        isl68239,
+        isl69222,
+        isl69223,
+        isl69224,
+        isl69225,
+        isl69227,
+        isl69228,
+        isl69234,
+        isl69236,
+        isl69239,
+        isl69242,
+        isl69243,
+        isl69247,
+        isl69248,
+        isl69254,
+        isl69255,
+        isl69256,
+        isl69259,
+        isl69260,
+        isl69268,
+        isl69269,
+        isl69298,
+        raa228000,
+        raa228004,
+        raa228006,
+        raa228228,
+        raa229001,
+        raa229004,
+};
+
+enum rail_configs { high_voltage, one_rail, two_rail, three_rail };
+
+static const struct  i2c_device_id isl692xx_id[] = {
+        { "isl68220", isl68220 },
+        { "isl68221", isl68221 },
+        { "isl68222", isl68222 },
+        { "isl68223", isl68223 },
+        { "isl68224", isl68224 },
+        { "isl68225", isl68225 },
+        { "isl68226", isl68226 },
+        { "isl68227", isl68227 },
+        { "isl68229", isl68229 },
+        { "isl68233", isl68233 },
+        { "isl68239", isl68239 },
+        { "isl69222", isl69222 },
+        { "isl69223", isl69223 },
+        { "isl69224", isl69224 },
+        { "isl69225", isl69225 },
+        { "isl69227", isl69227 },
+        { "isl69228", isl69228 },
+        { "isl69234", isl69234 },
+        { "isl69236", isl69236 },
+        { "isl69239", isl69239 },
+        { "isl69242", isl69242 },
+        { "isl69243", isl69243 },
+        { "isl69247", isl69247 },
+        { "isl69248", isl69248 },
+        { "isl69254", isl69254 },
+        { "isl69255", isl69255 },
+        { "isl69256", isl69256 },
+        { "isl69259", isl69259 },
+        { "isl69260", isl69260 },
+        { "isl69268", isl69268 },
+        { "isl69269", isl69269 },
+        { "isl69298", isl69298 },
+        { "raa228000", raa228000 },
+        { "raa228004", raa228004 },
+        { "raa228006", raa228006 },
+        { "raa228228", raa228228 },
+        { "raa229001", raa229001 },
+        { "raa229004", raa229004 },
+        { },
+};
+
+MODULE_DEVICE_TABLE(i2c, isl692xx_id);
+
+static int isl692xx_read_word_data(struct i2c_client *client, int page, int reg)
+{
+        int ret;
+
+        switch (reg) {
+        case PMBUS_VIRT_READ_VMON:
+                ret = pmbus_read_word_data(client, page, ISL692XX_READ_VMON);
+                break;
+        default:
+                ret = -ENODATA;
+                break;
+        }
+        
+        return ret;
+}
+
+static struct pmbus_driver_info isl692xx_info[] = {
+        [high_voltage] = {
+                .pages = 1,
+                .format[PSC_VOLTAGE_IN] = direct,
+                .format[PSC_VOLTAGE_OUT] = direct,
+                .format[PSC_CURRENT_IN] = direct,
+                .format[PSC_CURRENT_OUT] = direct,
+                .format[PSC_POWER] = direct,
+                .format[PSC_TEMPERATURE] = direct,
+                .m[PSC_VOLTAGE_IN] = 1,
+                .b[PSC_VOLTAGE_IN] = 0,
+                .R[PSC_VOLTAGE_IN] = 1,
+                .m[PSC_VOLTAGE_OUT] = 2,
+                .b[PSC_VOLTAGE_OUT] = 0,
+                .R[PSC_VOLTAGE_OUT] = 2,
+                .m[PSC_CURRENT_IN] = 2,
+                .b[PSC_CURRENT_IN] = 0,
+                .R[PSC_CURRENT_IN] = 2,
+                .m[PSC_CURRENT_OUT] = 1,
+                .b[PSC_CURRENT_OUT] = 0,
+                .R[PSC_CURRENT_OUT] = 1,
+                .m[PSC_POWER] = 2,
+                .b[PSC_POWER] = 0,
+                .R[PSC_POWER] = -1,
+                .m[PSC_TEMPERATURE] = 1,
+                .b[PSC_TEMPERATURE] = 0,
+                .R[PSC_TEMPERATURE] = 0,
+                .func[0] = PMBUS_HAVE_VIN | PMBUS_HAVE_VOUT | PMBUS_HAVE_IIN |
+                        PMBUS_HAVE_IOUT | PMBUS_HAVE_PIN | PMBUS_HAVE_POUT |
+                        PMBUS_HAVE_TEMP | PMBUS_HAVE_TEMP2 | PMBUS_HAVE_TEMP3 |
+                        PMBUS_HAVE_STATUS_VOUT | PMBUS_HAVE_STATUS_IOUT |
+                        PMBUS_HAVE_STATUS_INPUT | PMBUS_HAVE_STATUS_TEMP |
+                        PMBUS_HAVE_VMON,
+                .read_word_data = isl692xx_read_word_data,
+        },
+        [one_rail] = {
+                .pages = 1,
+                .format[PSC_VOLTAGE_IN] = direct,
+                .format[PSC_VOLTAGE_OUT] = direct,
+                .format[PSC_CURRENT_IN] = direct,
+                .format[PSC_CURRENT_OUT] = direct,
+                .format[PSC_POWER] = direct,
+                .format[PSC_TEMPERATURE] = direct,
+                .m[PSC_VOLTAGE_IN] = 1,
+                .b[PSC_VOLTAGE_IN] = 0,
+                .R[PSC_VOLTAGE_IN] = 2,
+                .m[PSC_VOLTAGE_OUT] = 1,
+                .b[PSC_VOLTAGE_OUT] = 0,
+                .R[PSC_VOLTAGE_OUT] = 3,
+                .m[PSC_CURRENT_IN] = 1,
+                .b[PSC_CURRENT_IN] = 0,
+                .R[PSC_CURRENT_IN] = 2,
+                .m[PSC_CURRENT_OUT] = 1,
+                .b[PSC_CURRENT_OUT] = 0,
+                .R[PSC_CURRENT_OUT] = 1,
+                .m[PSC_POWER] = 1,
+                .b[PSC_POWER] = 0,
+                .R[PSC_POWER] = 0,
+                .m[PSC_TEMPERATURE] = 1,
+                .b[PSC_TEMPERATURE] = 0,
+                .R[PSC_TEMPERATURE] = 0,
+                .func[0] = PMBUS_HAVE_VIN | PMBUS_HAVE_VOUT | PMBUS_HAVE_IIN |
+                        PMBUS_HAVE_IOUT | PMBUS_HAVE_PIN | PMBUS_HAVE_POUT |
+                        PMBUS_HAVE_TEMP | PMBUS_HAVE_TEMP2 | PMBUS_HAVE_TEMP3 |
+                        PMBUS_HAVE_STATUS_VOUT | PMBUS_HAVE_STATUS_IOUT |
+                        PMBUS_HAVE_STATUS_INPUT | PMBUS_HAVE_STATUS_TEMP |
+                        PMBUS_HAVE_VMON,
+                .read_word_data = isl692xx_read_word_data,
+        },
+        [two_rail] = {
+                .pages = 2,
+                .format[PSC_VOLTAGE_IN] = direct,
+                .format[PSC_VOLTAGE_OUT] = direct,
+                .format[PSC_CURRENT_IN] = direct,
+                .format[PSC_CURRENT_OUT] = direct,
+                .format[PSC_POWER] = direct,
+                .format[PSC_TEMPERATURE] = direct,
+                .m[PSC_VOLTAGE_IN] = 1,
+                .b[PSC_VOLTAGE_IN] = 0,
+                .R[PSC_VOLTAGE_IN] = 2,
+                .m[PSC_VOLTAGE_OUT] = 1,
+                .b[PSC_VOLTAGE_OUT] = 0,
+                .R[PSC_VOLTAGE_OUT] = 3,
+                .m[PSC_CURRENT_IN] = 1,
+                .b[PSC_CURRENT_IN] = 0,
+                .R[PSC_CURRENT_IN] = 2,
+                .m[PSC_CURRENT_OUT] = 1,
+                .b[PSC_CURRENT_OUT] = 0,
+                .R[PSC_CURRENT_OUT] = 1,
+                .m[PSC_POWER] = 1,
+                .b[PSC_POWER] = 0,
+                .R[PSC_POWER] = 0,
+                .m[PSC_TEMPERATURE] = 1,
+                .b[PSC_TEMPERATURE] = 0,
+                .R[PSC_TEMPERATURE] = 0,
+                .func[0] = PMBUS_HAVE_VIN | PMBUS_HAVE_VOUT | PMBUS_HAVE_IIN |
+                        PMBUS_HAVE_IOUT | PMBUS_HAVE_PIN | PMBUS_HAVE_POUT |
+                        PMBUS_HAVE_TEMP | PMBUS_HAVE_TEMP2 | PMBUS_HAVE_TEMP3 |
+                        PMBUS_HAVE_STATUS_VOUT | PMBUS_HAVE_STATUS_IOUT |
+                        PMBUS_HAVE_STATUS_INPUT | PMBUS_HAVE_STATUS_TEMP |
+                        PMBUS_HAVE_VMON,
+                .func[1] = PMBUS_HAVE_VIN | PMBUS_HAVE_VOUT | PMBUS_HAVE_IIN |
+                        PMBUS_HAVE_IOUT | PMBUS_HAVE_PIN | PMBUS_HAVE_POUT |
+                        PMBUS_HAVE_TEMP | PMBUS_HAVE_TEMP2 | PMBUS_HAVE_TEMP3 |
+                        PMBUS_HAVE_STATUS_VOUT | PMBUS_HAVE_STATUS_IOUT |
+                        PMBUS_HAVE_STATUS_INPUT | PMBUS_HAVE_STATUS_TEMP |
+                        PMBUS_HAVE_VMON,
+                .read_word_data = isl692xx_read_word_data,
+        },
+        [three_rail] = {
+                .pages = 3,
+                .format[PSC_VOLTAGE_IN] = direct,
+                .format[PSC_VOLTAGE_OUT] = direct,
+                .format[PSC_CURRENT_IN] = direct,
+                .format[PSC_CURRENT_OUT] = direct,
+                .format[PSC_POWER] = direct,
+                .format[PSC_TEMPERATURE] = direct,
+                .m[PSC_VOLTAGE_IN] = 1,
+                .b[PSC_VOLTAGE_IN] = 0,
+                .R[PSC_VOLTAGE_IN] = 2,
+                .m[PSC_VOLTAGE_OUT] = 1,
+                .b[PSC_VOLTAGE_OUT] = 0,
+                .R[PSC_VOLTAGE_OUT] = 3,
+                .m[PSC_CURRENT_IN] = 1,
+                .b[PSC_CURRENT_IN] = 0,
+                .R[PSC_CURRENT_IN] = 2,
+                .m[PSC_CURRENT_OUT] = 1,
+                .b[PSC_CURRENT_OUT] = 0,
+                .R[PSC_CURRENT_OUT] = 1,
+                .m[PSC_POWER] = 1,
+                .b[PSC_POWER] = 0,
+                .R[PSC_POWER] = 0,
+                .m[PSC_TEMPERATURE] = 1,
+                .b[PSC_TEMPERATURE] = 0,
+                .R[PSC_TEMPERATURE] = 0,
+                .func[0] = PMBUS_HAVE_VIN | PMBUS_HAVE_VOUT | PMBUS_HAVE_IIN |
+                        PMBUS_HAVE_IOUT | PMBUS_HAVE_PIN | PMBUS_HAVE_POUT |
+                        PMBUS_HAVE_TEMP | PMBUS_HAVE_TEMP2 | PMBUS_HAVE_TEMP3 |
+                        PMBUS_HAVE_STATUS_VOUT | PMBUS_HAVE_STATUS_IOUT |
+                        PMBUS_HAVE_STATUS_INPUT | PMBUS_HAVE_STATUS_TEMP |
+                        PMBUS_HAVE_VMON,
+                .func[1] = PMBUS_HAVE_VIN | PMBUS_HAVE_VOUT | PMBUS_HAVE_IIN |
+                        PMBUS_HAVE_IOUT | PMBUS_HAVE_PIN | PMBUS_HAVE_POUT |
+                        PMBUS_HAVE_TEMP | PMBUS_HAVE_TEMP2 | PMBUS_HAVE_TEMP3 |
+                        PMBUS_HAVE_STATUS_VOUT | PMBUS_HAVE_STATUS_IOUT |
+                        PMBUS_HAVE_STATUS_INPUT | PMBUS_HAVE_STATUS_TEMP |
+                        PMBUS_HAVE_VMON,
+                .func[2] = PMBUS_HAVE_VIN | PMBUS_HAVE_VOUT | PMBUS_HAVE_IIN |
+                        PMBUS_HAVE_IOUT | PMBUS_HAVE_PIN | PMBUS_HAVE_POUT |
+                        PMBUS_HAVE_TEMP | PMBUS_HAVE_TEMP2 | PMBUS_HAVE_TEMP3 |
+                        PMBUS_HAVE_STATUS_VOUT | PMBUS_HAVE_STATUS_IOUT |
+                        PMBUS_HAVE_STATUS_INPUT | PMBUS_HAVE_STATUS_TEMP |
+                        PMBUS_HAVE_VMON,
+                .read_word_data = isl692xx_read_word_data,
+        },
+};
+
+static int isl692xx_probe(struct i2c_client *client,
+                          const struct i2c_device_id *id)
+{
+        int ret;
+
+        switch (id->driver_data) {
+        case raa228000:
+        case raa228004:
+        case raa228006:
+                ret = pmbus_do_probe(client, id, &isl692xx_info[high_voltage]);
+                break;
+        case isl68227:
+        case isl69243:
+                ret = pmbus_do_probe(client, id, &isl692xx_info[one_rail]);
+                break;
+        case isl68220:
+        case isl68222:
+        case isl68223:
+        case isl68225:
+        case isl68233:
+        case isl69222:
+        case isl69224:
+        case isl69225:
+        case isl69234:
+        case isl69236:
+        case isl69242:
+        case isl69247:
+        case isl69248:
+        case isl69254:
+        case isl69255:
+        case isl69256:
+        case isl69259:
+        case isl69260:
+        case isl69268:
+        case isl69298:
+        case raa228228:
+        case raa229001:
+        case raa229004:
+                ret = pmbus_do_probe(client, id, &isl692xx_info[two_rail]);
+                break;
+        case isl68221:
+        case isl68224:
+        case isl68226:
+        case isl68229:
+        case isl68239:
+        case isl69223:
+        case isl69227:
+        case isl69228:
+        case isl69239:
+        case isl69269:
+                ret = pmbus_do_probe(client, id, &isl692xx_info[three_rail]);
+                break;
+        default:
+                ret = -ENODEV;
+        }
+
+        return ret;
+}
+
+static struct i2c_driver isl692xx_driver = {
+        .driver = {
+                .name = "isl692xx",
+        },
+        .probe = isl692xx_probe,
+        .remove = pmbus_do_remove,
+        .id_table = isl692xx_id,
+};
+
+module_i2c_driver(isl692xx_driver);
+
+MODULE_AUTHOR("Grant Peltier");
+MODULE_DESCRIPTION("PMBus driver for 2nd Gen Renesas digital multiphase "
+                   "devices");
+MODULE_LICENSE("GPL");
+
-- 
2.20.1


^ permalink raw reply related	[flat|nested] 6+ messages in thread

* Re: [PATCH] hwmon: (pmbus) Add support for 2nd Gen Renesas digital multiphase
  2020-02-28 21:23 [PATCH] hwmon: (pmbus) Add support for 2nd Gen Renesas digital multiphase Grant Peltier
@ 2020-02-28 22:58 ` Guenter Roeck
  2020-02-28 23:52   ` Grant Peltier
  0 siblings, 1 reply; 6+ messages in thread
From: Guenter Roeck @ 2020-02-28 22:58 UTC (permalink / raw)
  To: Grant Peltier; +Cc: linux-hwmon, linux-kernel, openbmc, adam.vaughn.xh, zaitsev

On Fri, Feb 28, 2020 at 03:23:49PM -0600, Grant Peltier wrote:
> Add a driver to support 2nd generation Renesas digital multiphase power
> regulators. The driver is meant to support a large family of part
> numbers spanning isl682xx, isl692xx, and some raa228/9 part designations.
> 
> Signed-off-by: Grant Peltier <grantpeltier93@gmail.com>
> ---
>  drivers/hwmon/pmbus/Kconfig    |   9 +
>  drivers/hwmon/pmbus/Makefile   |   1 +
>  drivers/hwmon/pmbus/isl692xx.c | 352 +++++++++++++++++++++++++++++++++

One of the supported chips should be selected as driver name. There is no
guarantee that there will never be any ISL692XX chips with different
functionality. Besides, the name is misleading anyway since the driver
already supports other chips besides those named isl692xx.

Please provide Documentation/hwmon/isl692xx (or whatever the final name is).
This needs to briefly explain what each of those chips actually is.

There is no public information available for several of the chips listed
in the driver. Without datasheets I can only accept support for chips
which are by some means confirmed to actually exist.

In this context, I have to admit that I _really_ dislike the secrecy
around those chips. I have seen several instances where I accepted
a driver which turned out to be buggy, simply because I did not have
access to a datasheet. In some cases, even if I know that there
may be a problem or missing support for something I can't talk
about it it because, say, I have seen an internal driver which
does things a bit differently and my employer didn't get permission
to publish the driver. Yes, I understand that this is becoming
more and more common in the industry, but that doesn't make it
better and ultimately just hurts everyone.

More comments inline.

Guenter

>  3 files changed, 362 insertions(+)
>  create mode 100644 drivers/hwmon/pmbus/isl692xx.c
> 
> diff --git a/drivers/hwmon/pmbus/Kconfig b/drivers/hwmon/pmbus/Kconfig
> index a9ea06204767..fbe7bbc8b37c 100644
> --- a/drivers/hwmon/pmbus/Kconfig
> +++ b/drivers/hwmon/pmbus/Kconfig
> @@ -100,6 +100,15 @@ config SENSORS_ISL68137
>  	  This driver can also be built as a module. If so, the module will
>  	  be called isl68137.
>  
> +config SENSORS_ISL692XX
> +        tristate "Renesas 2nd Gen Digital Multiphase"
						... Power Regulators
> +        help
> +          If you say yes here you get hardware monitoring support for Renesas
> +          2nd Generation Digital Multiphase power regulators.
> +
> +          This driver can also be built as a module. If so, the module will
> +          be called isl692xx.
> +
>  config SENSORS_LM25066
>  	tristate "National Semiconductor LM25066 and compatibles"
>  	help
> diff --git a/drivers/hwmon/pmbus/Makefile b/drivers/hwmon/pmbus/Makefile
> index 5feb45806123..bf1ea99a6120 100644
> --- a/drivers/hwmon/pmbus/Makefile
> +++ b/drivers/hwmon/pmbus/Makefile
> @@ -13,6 +13,7 @@ obj-$(CONFIG_SENSORS_IR35221)	+= ir35221.o
>  obj-$(CONFIG_SENSORS_IR38064)	+= ir38064.o
>  obj-$(CONFIG_SENSORS_IRPS5401)	+= irps5401.o
>  obj-$(CONFIG_SENSORS_ISL68137)	+= isl68137.o
> +obj-$(CONFIG_SENSORS_ISL692XX)	+= isl692xx.o
>  obj-$(CONFIG_SENSORS_LM25066)	+= lm25066.o
>  obj-$(CONFIG_SENSORS_LTC2978)	+= ltc2978.o
>  obj-$(CONFIG_SENSORS_LTC3815)	+= ltc3815.o
> diff --git a/drivers/hwmon/pmbus/isl692xx.c b/drivers/hwmon/pmbus/isl692xx.c
> new file mode 100644
> index 000000000000..26f3d90a7ddc
> --- /dev/null
> +++ b/drivers/hwmon/pmbus/isl692xx.c
> @@ -0,0 +1,352 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * Hardware monitoring driver for Renesas Gen 2 Digital Multiphase Devices
> + *
> + * Copyright (c) 2020 Renesas Electronics America
> + */
> +
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/init.h>
> +#include <linux/err.h>
> +#include <linux/i2c.h>
> +
Alphabetic order please

> +#include "pmbus.h"
> +
> +#define ISL692XX_READ_VMON        0xc8
> +
> +enum parts {
> +        isl68220,
> +        isl68221,
> +        isl68222,
> +        isl68223,
> +        isl68224,
> +        isl68225,
> +        isl68226,
> +        isl68227,
> +        isl68229,
> +        isl68233,
> +        isl68239,
> +        isl69222,
> +        isl69223,
> +        isl69224,
> +        isl69225,
> +        isl69227,
> +        isl69228,
> +        isl69234,
> +        isl69236,
> +        isl69239,
> +        isl69242,
> +        isl69243,
> +        isl69247,
> +        isl69248,
> +        isl69254,
> +        isl69255,
> +        isl69256,
> +        isl69259,
> +        isl69260,
> +        isl69268,
> +        isl69269,
> +        isl69298,
> +        raa228000,
> +        raa228004,
> +        raa228006,
> +        raa228228,
> +        raa229001,
> +        raa229004,
> +};
> +
> +enum rail_configs { high_voltage, one_rail, two_rail, three_rail };
> +
> +static const struct  i2c_device_id isl692xx_id[] = {
> +        { "isl68220", isl68220 },
> +        { "isl68221", isl68221 },
> +        { "isl68222", isl68222 },
> +        { "isl68223", isl68223 },
> +        { "isl68224", isl68224 },
> +        { "isl68225", isl68225 },
> +        { "isl68226", isl68226 },
> +        { "isl68227", isl68227 },
> +        { "isl68229", isl68229 },
> +        { "isl68233", isl68233 },
> +        { "isl68239", isl68239 },
> +        { "isl69222", isl69222 },
> +        { "isl69223", isl69223 },
> +        { "isl69224", isl69224 },
> +        { "isl69225", isl69225 },
> +        { "isl69227", isl69227 },
> +        { "isl69228", isl69228 },
> +        { "isl69234", isl69234 },
> +        { "isl69236", isl69236 },
> +        { "isl69239", isl69239 },
> +        { "isl69242", isl69242 },
> +        { "isl69243", isl69243 },
> +        { "isl69247", isl69247 },
> +        { "isl69248", isl69248 },
> +        { "isl69254", isl69254 },
> +        { "isl69255", isl69255 },
> +        { "isl69256", isl69256 },
> +        { "isl69259", isl69259 },
> +        { "isl69260", isl69260 },
> +        { "isl69268", isl69268 },
> +        { "isl69269", isl69269 },
> +        { "isl69298", isl69298 },
> +        { "raa228000", raa228000 },
> +        { "raa228004", raa228004 },
> +        { "raa228006", raa228006 },
> +        { "raa228228", raa228228 },
> +        { "raa229001", raa229001 },
> +        { "raa229004", raa229004 },

It would be much simpler to just specify high_voltage, one_rail, two_rail,
and three_rail as parameters. I don't see value in the code translating
chip types to enum rail_configs (nor in enum parts).

> +        { },
> +};
> +
> +MODULE_DEVICE_TABLE(i2c, isl692xx_id);
> +
> +static int isl692xx_read_word_data(struct i2c_client *client, int page, int reg)
> +{
> +        int ret;
> +
> +        switch (reg) {
> +        case PMBUS_VIRT_READ_VMON:
> +                ret = pmbus_read_word_data(client, page, ISL692XX_READ_VMON);
> +                break;
> +        default:
> +                ret = -ENODATA;
> +                break;
> +        }
> +        
> +        return ret;
> +}
> +
> +static struct pmbus_driver_info isl692xx_info[] = {
> +        [high_voltage] = {
> +                .pages = 1,
> +                .format[PSC_VOLTAGE_IN] = direct,
> +                .format[PSC_VOLTAGE_OUT] = direct,
> +                .format[PSC_CURRENT_IN] = direct,
> +                .format[PSC_CURRENT_OUT] = direct,
> +                .format[PSC_POWER] = direct,
> +                .format[PSC_TEMPERATURE] = direct,
> +                .m[PSC_VOLTAGE_IN] = 1,
> +                .b[PSC_VOLTAGE_IN] = 0,
> +                .R[PSC_VOLTAGE_IN] = 1,
> +                .m[PSC_VOLTAGE_OUT] = 2,
> +                .b[PSC_VOLTAGE_OUT] = 0,
> +                .R[PSC_VOLTAGE_OUT] = 2,
> +                .m[PSC_CURRENT_IN] = 2,
> +                .b[PSC_CURRENT_IN] = 0,
> +                .R[PSC_CURRENT_IN] = 2,
> +                .m[PSC_CURRENT_OUT] = 1,
> +                .b[PSC_CURRENT_OUT] = 0,
> +                .R[PSC_CURRENT_OUT] = 1,
> +                .m[PSC_POWER] = 2,
> +                .b[PSC_POWER] = 0,
> +                .R[PSC_POWER] = -1,
> +                .m[PSC_TEMPERATURE] = 1,
> +                .b[PSC_TEMPERATURE] = 0,
> +                .R[PSC_TEMPERATURE] = 0,
> +                .func[0] = PMBUS_HAVE_VIN | PMBUS_HAVE_VOUT | PMBUS_HAVE_IIN |
> +                        PMBUS_HAVE_IOUT | PMBUS_HAVE_PIN | PMBUS_HAVE_POUT |
> +                        PMBUS_HAVE_TEMP | PMBUS_HAVE_TEMP2 | PMBUS_HAVE_TEMP3 |
> +                        PMBUS_HAVE_STATUS_VOUT | PMBUS_HAVE_STATUS_IOUT |
> +                        PMBUS_HAVE_STATUS_INPUT | PMBUS_HAVE_STATUS_TEMP |
> +                        PMBUS_HAVE_VMON,
> +                .read_word_data = isl692xx_read_word_data,
> +        },
> +        [one_rail] = {
> +                .pages = 1,
> +                .format[PSC_VOLTAGE_IN] = direct,
> +                .format[PSC_VOLTAGE_OUT] = direct,
> +                .format[PSC_CURRENT_IN] = direct,
> +                .format[PSC_CURRENT_OUT] = direct,
> +                .format[PSC_POWER] = direct,
> +                .format[PSC_TEMPERATURE] = direct,
> +                .m[PSC_VOLTAGE_IN] = 1,
> +                .b[PSC_VOLTAGE_IN] = 0,
> +                .R[PSC_VOLTAGE_IN] = 2,
> +                .m[PSC_VOLTAGE_OUT] = 1,
> +                .b[PSC_VOLTAGE_OUT] = 0,
> +                .R[PSC_VOLTAGE_OUT] = 3,
> +                .m[PSC_CURRENT_IN] = 1,
> +                .b[PSC_CURRENT_IN] = 0,
> +                .R[PSC_CURRENT_IN] = 2,
> +                .m[PSC_CURRENT_OUT] = 1,
> +                .b[PSC_CURRENT_OUT] = 0,
> +                .R[PSC_CURRENT_OUT] = 1,
> +                .m[PSC_POWER] = 1,
> +                .b[PSC_POWER] = 0,
> +                .R[PSC_POWER] = 0,
> +                .m[PSC_TEMPERATURE] = 1,
> +                .b[PSC_TEMPERATURE] = 0,
> +                .R[PSC_TEMPERATURE] = 0,
> +                .func[0] = PMBUS_HAVE_VIN | PMBUS_HAVE_VOUT | PMBUS_HAVE_IIN |
> +                        PMBUS_HAVE_IOUT | PMBUS_HAVE_PIN | PMBUS_HAVE_POUT |
> +                        PMBUS_HAVE_TEMP | PMBUS_HAVE_TEMP2 | PMBUS_HAVE_TEMP3 |
> +                        PMBUS_HAVE_STATUS_VOUT | PMBUS_HAVE_STATUS_IOUT |
> +                        PMBUS_HAVE_STATUS_INPUT | PMBUS_HAVE_STATUS_TEMP |
> +                        PMBUS_HAVE_VMON,
> +                .read_word_data = isl692xx_read_word_data,
> +        },
> +        [two_rail] = {
> +                .pages = 2,
> +                .format[PSC_VOLTAGE_IN] = direct,
> +                .format[PSC_VOLTAGE_OUT] = direct,
> +                .format[PSC_CURRENT_IN] = direct,
> +                .format[PSC_CURRENT_OUT] = direct,
> +                .format[PSC_POWER] = direct,
> +                .format[PSC_TEMPERATURE] = direct,
> +                .m[PSC_VOLTAGE_IN] = 1,
> +                .b[PSC_VOLTAGE_IN] = 0,
> +                .R[PSC_VOLTAGE_IN] = 2,
> +                .m[PSC_VOLTAGE_OUT] = 1,
> +                .b[PSC_VOLTAGE_OUT] = 0,
> +                .R[PSC_VOLTAGE_OUT] = 3,
> +                .m[PSC_CURRENT_IN] = 1,
> +                .b[PSC_CURRENT_IN] = 0,
> +                .R[PSC_CURRENT_IN] = 2,
> +                .m[PSC_CURRENT_OUT] = 1,
> +                .b[PSC_CURRENT_OUT] = 0,
> +                .R[PSC_CURRENT_OUT] = 1,
> +                .m[PSC_POWER] = 1,
> +                .b[PSC_POWER] = 0,
> +                .R[PSC_POWER] = 0,
> +                .m[PSC_TEMPERATURE] = 1,
> +                .b[PSC_TEMPERATURE] = 0,
> +                .R[PSC_TEMPERATURE] = 0,
> +                .func[0] = PMBUS_HAVE_VIN | PMBUS_HAVE_VOUT | PMBUS_HAVE_IIN |
> +                        PMBUS_HAVE_IOUT | PMBUS_HAVE_PIN | PMBUS_HAVE_POUT |
> +                        PMBUS_HAVE_TEMP | PMBUS_HAVE_TEMP2 | PMBUS_HAVE_TEMP3 |
> +                        PMBUS_HAVE_STATUS_VOUT | PMBUS_HAVE_STATUS_IOUT |
> +                        PMBUS_HAVE_STATUS_INPUT | PMBUS_HAVE_STATUS_TEMP |
> +                        PMBUS_HAVE_VMON,
> +                .func[1] = PMBUS_HAVE_VIN | PMBUS_HAVE_VOUT | PMBUS_HAVE_IIN |
> +                        PMBUS_HAVE_IOUT | PMBUS_HAVE_PIN | PMBUS_HAVE_POUT |
> +                        PMBUS_HAVE_TEMP | PMBUS_HAVE_TEMP2 | PMBUS_HAVE_TEMP3 |
> +                        PMBUS_HAVE_STATUS_VOUT | PMBUS_HAVE_STATUS_IOUT |
> +                        PMBUS_HAVE_STATUS_INPUT | PMBUS_HAVE_STATUS_TEMP |
> +                        PMBUS_HAVE_VMON,
> +                .read_word_data = isl692xx_read_word_data,
> +        },
> +        [three_rail] = {
> +                .pages = 3,
> +                .format[PSC_VOLTAGE_IN] = direct,
> +                .format[PSC_VOLTAGE_OUT] = direct,
> +                .format[PSC_CURRENT_IN] = direct,
> +                .format[PSC_CURRENT_OUT] = direct,
> +                .format[PSC_POWER] = direct,
> +                .format[PSC_TEMPERATURE] = direct,
> +                .m[PSC_VOLTAGE_IN] = 1,
> +                .b[PSC_VOLTAGE_IN] = 0,
> +                .R[PSC_VOLTAGE_IN] = 2,
> +                .m[PSC_VOLTAGE_OUT] = 1,
> +                .b[PSC_VOLTAGE_OUT] = 0,
> +                .R[PSC_VOLTAGE_OUT] = 3,
> +                .m[PSC_CURRENT_IN] = 1,
> +                .b[PSC_CURRENT_IN] = 0,
> +                .R[PSC_CURRENT_IN] = 2,
> +                .m[PSC_CURRENT_OUT] = 1,
> +                .b[PSC_CURRENT_OUT] = 0,
> +                .R[PSC_CURRENT_OUT] = 1,
> +                .m[PSC_POWER] = 1,
> +                .b[PSC_POWER] = 0,
> +                .R[PSC_POWER] = 0,
> +                .m[PSC_TEMPERATURE] = 1,
> +                .b[PSC_TEMPERATURE] = 0,
> +                .R[PSC_TEMPERATURE] = 0,
> +                .func[0] = PMBUS_HAVE_VIN | PMBUS_HAVE_VOUT | PMBUS_HAVE_IIN |
> +                        PMBUS_HAVE_IOUT | PMBUS_HAVE_PIN | PMBUS_HAVE_POUT |
> +                        PMBUS_HAVE_TEMP | PMBUS_HAVE_TEMP2 | PMBUS_HAVE_TEMP3 |
> +                        PMBUS_HAVE_STATUS_VOUT | PMBUS_HAVE_STATUS_IOUT |
> +                        PMBUS_HAVE_STATUS_INPUT | PMBUS_HAVE_STATUS_TEMP |
> +                        PMBUS_HAVE_VMON,
> +                .func[1] = PMBUS_HAVE_VIN | PMBUS_HAVE_VOUT | PMBUS_HAVE_IIN |
> +                        PMBUS_HAVE_IOUT | PMBUS_HAVE_PIN | PMBUS_HAVE_POUT |
> +                        PMBUS_HAVE_TEMP | PMBUS_HAVE_TEMP2 | PMBUS_HAVE_TEMP3 |
> +                        PMBUS_HAVE_STATUS_VOUT | PMBUS_HAVE_STATUS_IOUT |
> +                        PMBUS_HAVE_STATUS_INPUT | PMBUS_HAVE_STATUS_TEMP |
> +                        PMBUS_HAVE_VMON,
> +                .func[2] = PMBUS_HAVE_VIN | PMBUS_HAVE_VOUT | PMBUS_HAVE_IIN |
> +                        PMBUS_HAVE_IOUT | PMBUS_HAVE_PIN | PMBUS_HAVE_POUT |
> +                        PMBUS_HAVE_TEMP | PMBUS_HAVE_TEMP2 | PMBUS_HAVE_TEMP3 |
> +                        PMBUS_HAVE_STATUS_VOUT | PMBUS_HAVE_STATUS_IOUT |
> +                        PMBUS_HAVE_STATUS_INPUT | PMBUS_HAVE_STATUS_TEMP |
> +                        PMBUS_HAVE_VMON,
> +                .read_word_data = isl692xx_read_word_data,
> +        },
> +};
> +
> +static int isl692xx_probe(struct i2c_client *client,
> +                          const struct i2c_device_id *id)
> +{
> +        int ret;
> +
> +        switch (id->driver_data) {
> +        case raa228000:
> +        case raa228004:
> +        case raa228006:
> +                ret = pmbus_do_probe(client, id, &isl692xx_info[high_voltage]);
> +                break;
> +        case isl68227:
> +        case isl69243:
> +                ret = pmbus_do_probe(client, id, &isl692xx_info[one_rail]);
> +                break;
> +        case isl68220:
> +        case isl68222:
> +        case isl68223:
> +        case isl68225:
> +        case isl68233:
> +        case isl69222:
> +        case isl69224:
> +        case isl69225:
> +        case isl69234:
> +        case isl69236:
> +        case isl69242:
> +        case isl69247:
> +        case isl69248:
> +        case isl69254:
> +        case isl69255:
> +        case isl69256:
> +        case isl69259:
> +        case isl69260:
> +        case isl69268:
> +        case isl69298:
> +        case raa228228:
> +        case raa229001:
> +        case raa229004:
> +                ret = pmbus_do_probe(client, id, &isl692xx_info[two_rail]);
> +                break;
> +        case isl68221:
> +        case isl68224:
> +        case isl68226:
> +        case isl68229:
> +        case isl68239:
> +        case isl69223:
> +        case isl69227:
> +        case isl69228:
> +        case isl69239:
> +        case isl69269:
> +                ret = pmbus_do_probe(client, id, &isl692xx_info[three_rail]);
> +                break;
> +        default:
> +                ret = -ENODEV;
> +        }
> +
> +        return ret;
> +}
> +
> +static struct i2c_driver isl692xx_driver = {
> +        .driver = {
> +                .name = "isl692xx",
> +        },
> +        .probe = isl692xx_probe,
> +        .remove = pmbus_do_remove,
> +        .id_table = isl692xx_id,
> +};
> +
> +module_i2c_driver(isl692xx_driver);
> +
> +MODULE_AUTHOR("Grant Peltier");
> +MODULE_DESCRIPTION("PMBus driver for 2nd Gen Renesas digital multiphase "
> +                   "devices");
> +MODULE_LICENSE("GPL");
> +
> -- 
> 2.20.1
> 

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] hwmon: (pmbus) Add support for 2nd Gen Renesas digital multiphase
  2020-02-28 22:58 ` Guenter Roeck
@ 2020-02-28 23:52   ` Grant Peltier
  2020-02-29  1:55     ` Guenter Roeck
  0 siblings, 1 reply; 6+ messages in thread
From: Grant Peltier @ 2020-02-28 23:52 UTC (permalink / raw)
  To: Guenter Roeck; +Cc: linux-hwmon, linux-kernel, openbmc, adam.vaughn.xh, zaitsev

Hi Guenter,

Thank you for your expedient review. I will need to consult with my
coworkers to determine a more appropriate driver name. In the meantime I
will make the desired changes and I will also create a document for the
driver, which I will submit as a linked but separate patch.

With regard to the part numbers, this family of parts is currently in
the process of being released and we have not yet published all of the
corresponding datasheets. However, I have been assured that all of the
parts listed are slated to have a datasheet published publicly in the near
future.

Thank you,
Grant

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] hwmon: (pmbus) Add support for 2nd Gen Renesas digital multiphase
  2020-02-28 23:52   ` Grant Peltier
@ 2020-02-29  1:55     ` Guenter Roeck
  2020-02-29 15:48       ` Grant Peltier
  0 siblings, 1 reply; 6+ messages in thread
From: Guenter Roeck @ 2020-02-29  1:55 UTC (permalink / raw)
  To: Grant Peltier; +Cc: linux-hwmon, linux-kernel, openbmc, adam.vaughn.xh, zaitsev

On 2/28/20 3:52 PM, Grant Peltier wrote:
> Hi Guenter,
> 
> Thank you for your expedient review. I will need to consult with my
> coworkers to determine a more appropriate driver name. In the meantime I
> will make the desired changes and I will also create a document for the
> driver, which I will submit as a linked but separate patch.
> 
> With regard to the part numbers, this family of parts is currently in
> the process of being released and we have not yet published all of the
> corresponding datasheets. However, I have been assured that all of the
> parts listed are slated to have a datasheet published publicly in the near
> future.
> 
That would be great.

As for the driver name, I had a look into drivers/hwmon/pmbus/isl68137.c,
and I don't immediately see why the new chips would warrant a new driver.
The only differences seem to be that VMON is a new command, and of course
only the ISL68137 supports AVL. But then there is, for example, ISL68127,
which is again quite similar. The only other difference as far as I can
see is input voltage scaling, but that doesn't warrant a separate driver
(and, of course, I have no means to validate if input voltage scaling
is indeed different for all the new chips).

Overall I would suggest to extend the isl68137 driver. I would also
suggest to not add separate tables for each of the rail configurations
but use the three-phase entry as starting point, copy it, and adjust its
values as needed.

For the multi-phase chips, I question if reporting the input voltage
for each phase make sense. Is it really a different voltage ? For IIN
and PIN, the question is if the registers are indeed paged, since they
are not paged in the older chips.

Guenter

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] hwmon: (pmbus) Add support for 2nd Gen Renesas digital multiphase
  2020-02-29  1:55     ` Guenter Roeck
@ 2020-02-29 15:48       ` Grant Peltier
  2020-02-29 16:30         ` Guenter Roeck
  0 siblings, 1 reply; 6+ messages in thread
From: Grant Peltier @ 2020-02-29 15:48 UTC (permalink / raw)
  To: Guenter Roeck; +Cc: linux-hwmon, linux-kernel, openbmc, adam.vaughn.xh, zaitsev

On Fri, Feb 28, 2020 at 05:55:44PM -0800, Guenter Roeck wrote:
> On 2/28/20 3:52 PM, Grant Peltier wrote:
> > Hi Guenter,
> > 
> > Thank you for your expedient review. I will need to consult with my
> > coworkers to determine a more appropriate driver name. In the meantime I
> > will make the desired changes and I will also create a document for the
> > driver, which I will submit as a linked but separate patch.
> > 
> > With regard to the part numbers, this family of parts is currently in
> > the process of being released and we have not yet published all of the
> > corresponding datasheets. However, I have been assured that all of the
> > parts listed are slated to have a datasheet published publicly in the near
> > future.
> > 
> That would be great.
> 
> As for the driver name, I had a look into drivers/hwmon/pmbus/isl68137.c,
> and I don't immediately see why the new chips would warrant a new driver.
> The only differences seem to be that VMON is a new command, and of course
> only the ISL68137 supports AVL. But then there is, for example, ISL68127,
> which is again quite similar. The only other difference as far as I can
> see is input voltage scaling, but that doesn't warrant a separate driver
> (and, of course, I have no means to validate if input voltage scaling
> is indeed different for all the new chips).
> 
> Overall I would suggest to extend the isl68137 driver. I would also
> suggest to not add separate tables for each of the rail configurations
> but use the three-phase entry as starting point, copy it, and adjust its
> values as needed.
> 
> For the multi-phase chips, I question if reporting the input voltage
> for each phase make sense. Is it really a different voltage ? For IIN
> and PIN, the question is if the registers are indeed paged, since they
> are not paged in the older chips.
> 
> Guenter

The ISL68137 is part of the first generation of our digital multiphase
parts which are all exclusively 2-rail (2-page) devices. There are a
couple of reasons that we are opting for a new driver for the new
generation of devices:

1) Gen 2 has multiple rail configurations (1, 2, or 3) with different scaling
parameters than Gen 1
2) We are planning to support some of the non-generic PMBus functions of
the Gen 2 devices using the debugfs interface.

I am currently working on point 2 and those features are not
quite ready to be included in a patch set but we wanted to move forward
with the hwmon functionality for now as that is useful on it's own.

Fair point on the global vs paged commands. I will modify the page
functions so that global commands are only read from page 0.

Thank you,
Grant

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] hwmon: (pmbus) Add support for 2nd Gen Renesas digital multiphase
  2020-02-29 15:48       ` Grant Peltier
@ 2020-02-29 16:30         ` Guenter Roeck
  0 siblings, 0 replies; 6+ messages in thread
From: Guenter Roeck @ 2020-02-29 16:30 UTC (permalink / raw)
  To: Grant Peltier; +Cc: linux-hwmon, linux-kernel, openbmc, adam.vaughn.xh, zaitsev

On 2/29/20 7:48 AM, Grant Peltier wrote:
> On Fri, Feb 28, 2020 at 05:55:44PM -0800, Guenter Roeck wrote:
>> On 2/28/20 3:52 PM, Grant Peltier wrote:
>>> Hi Guenter,
>>>
>>> Thank you for your expedient review. I will need to consult with my
>>> coworkers to determine a more appropriate driver name. In the meantime I
>>> will make the desired changes and I will also create a document for the
>>> driver, which I will submit as a linked but separate patch.
>>>
>>> With regard to the part numbers, this family of parts is currently in
>>> the process of being released and we have not yet published all of the
>>> corresponding datasheets. However, I have been assured that all of the
>>> parts listed are slated to have a datasheet published publicly in the near
>>> future.
>>>
>> That would be great.
>>
>> As for the driver name, I had a look into drivers/hwmon/pmbus/isl68137.c,
>> and I don't immediately see why the new chips would warrant a new driver.
>> The only differences seem to be that VMON is a new command, and of course
>> only the ISL68137 supports AVL. But then there is, for example, ISL68127,
>> which is again quite similar. The only other difference as far as I can
>> see is input voltage scaling, but that doesn't warrant a separate driver
>> (and, of course, I have no means to validate if input voltage scaling
>> is indeed different for all the new chips).
>>
>> Overall I would suggest to extend the isl68137 driver. I would also
>> suggest to not add separate tables for each of the rail configurations
>> but use the three-phase entry as starting point, copy it, and adjust its
>> values as needed.
>>
>> For the multi-phase chips, I question if reporting the input voltage
>> for each phase make sense. Is it really a different voltage ? For IIN
>> and PIN, the question is if the registers are indeed paged, since they
>> are not paged in the older chips.
>>
>> Guenter
> 
> The ISL68137 is part of the first generation of our digital multiphase
> parts which are all exclusively 2-rail (2-page) devices. There are a
> couple of reasons that we are opting for a new driver for the new
> generation of devices:
> 
> 1) Gen 2 has multiple rail configurations (1, 2, or 3) with different scaling
> parameters than Gen 1

That would only mean a single additional entry for the Gen1 devices.
This is not a valid argument for a separate driver, especially since
the difference in scaling only affects a single parameter as far as I
can see.

> 2) We are planning to support some of the non-generic PMBus functions of
> the Gen 2 devices using the debugfs interface.
> 
This is not a valid argument either. We have, for example, a single driver
for all Linear chips, even though they have quite some difference across
individual chips. The above can simply be solved by not instantiating
debugfs for Gen1 devices, and marking VMON as supported for Gen2 devices
only. If the Gen1 devices are all pretty much the same, you'd only need
a single additional table entry for those in the driver (if you insist
using a table).

For the debugfs functions, please keep in mind that those will have to be
documented in a way that lets people without access to datasheets
understand what they are for. We can't have cryptic debugfs functions
which, if misused, blow up the chip.

> I am currently working on point 2 and those features are not
> quite ready to be included in a patch set but we wanted to move forward
> with the hwmon functionality for now as that is useful on it's own.
> 
> Fair point on the global vs paged commands. I will modify the page
> functions so that global commands are only read from page 0.
> 

Also a fair point showing that not having the datasheet hurts. I see the same
problem with recent TI devices. In some cases I _know_ that a driver is buggy
or much less than perfect, because I have access to a datasheet through
my employer, but I can't talk about it because what I know is under NDA.
Then I end up spending a lot of time trying to find leaks that let me comment.
I am _not_ happy about this situation, not at all.

Please understand that I won't accept a driver adding support for a chip
if there is no public information available that the chip exists in the
first place. Imagine a situation where you are requesting to add support
for a chip, and that chip isn't mentioned anywhere, not even in a datasheet
I (hypothetically) may have access to through my employer. How would _you_
handle such a situation if you were in my place ?

You could of course send me all the datasheets under NDA to my work e-mail,
but that wouldn't really be much better since I still would not be able to
comment in public. On the other side, I'd have at least some confirmation
that those magic chips do indeed exist, so you might possibly want to
consider that.

Thanks,
Guenter

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2020-02-29 16:30 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-02-28 21:23 [PATCH] hwmon: (pmbus) Add support for 2nd Gen Renesas digital multiphase Grant Peltier
2020-02-28 22:58 ` Guenter Roeck
2020-02-28 23:52   ` Grant Peltier
2020-02-29  1:55     ` Guenter Roeck
2020-02-29 15:48       ` Grant Peltier
2020-02-29 16:30         ` Guenter Roeck

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).