All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v2 1/3] hwmon: (pmbus) Add driver for Delta DPS-920AB PSU
@ 2021-05-28 10:38 Robert Marko
  2021-05-28 10:38 ` [PATCH v2 2/3] dt-bindings: trivial-devices: Add Delta DPS920AB Robert Marko
                   ` (2 more replies)
  0 siblings, 3 replies; 6+ messages in thread
From: Robert Marko @ 2021-05-28 10:38 UTC (permalink / raw)
  To: robh+dt, jdelvare, linux, corbet, linux-hwmon, linux-doc,
	linux-kernel, trivial, Jonathan.Cameron, alexandre.belloni, sst,
	krzk, alexandru.ardelean, devicetree
  Cc: luka.perkov, jmp, pmenzel, buczek, Robert Marko

This adds support for the Delta DPS-920AB PSU.

Only missing feature is fan control which the PSU supports.

Signed-off-by: Robert Marko <robert.marko@sartura.hr>
---
Changes in v2:
* Check for Manufacturer and Model
* Restrict word read/write to supported only
* Update documentation to reflect driver changes
* Add basic debugfs entries

 Documentation/hwmon/dps920ab.rst |  73 ++++++++++
 Documentation/hwmon/index.rst    |   1 +
 drivers/hwmon/pmbus/Kconfig      |   9 ++
 drivers/hwmon/pmbus/Makefile     |   1 +
 drivers/hwmon/pmbus/dps920ab.c   | 229 +++++++++++++++++++++++++++++++
 5 files changed, 313 insertions(+)
 create mode 100644 Documentation/hwmon/dps920ab.rst
 create mode 100644 drivers/hwmon/pmbus/dps920ab.c

diff --git a/Documentation/hwmon/dps920ab.rst b/Documentation/hwmon/dps920ab.rst
new file mode 100644
index 000000000000..c33b4cdc0a60
--- /dev/null
+++ b/Documentation/hwmon/dps920ab.rst
@@ -0,0 +1,73 @@
+.. SPDX-License-Identifier: GPL-2.0-or-later
+
+Kernel driver dps920ab
+========================
+
+Supported chips:
+
+  * Delta DPS920AB
+
+    Prefix: 'dps920ab'
+
+    Addresses scanned: -
+
+Authors:
+    Robert Marko <robert.marko@sartura.hr>
+
+
+Description
+-----------
+
+This driver implements support for Delta DPS920AB 920W 54V DC single output
+power supply with PMBus support.
+
+The driver is a client driver to the core PMBus driver.
+Please see Documentation/hwmon/pmbus.rst 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.
+
+
+Sysfs entries
+-------------
+
+======================= ======================================================
+curr1_label		"iin"
+curr1_input		Measured input current
+curr1_alarm		Input current high alarm
+
+curr2_label		"iout1"
+curr2_input		Measured output current
+curr2_max		Maximum output current
+curr2_rated_max		Maximum rated output current
+
+in1_label		"vin"
+in1_input		Measured input voltage
+in1_alarm		Input voltage alarm
+
+in2_label		"vout1"
+in2_input		Measured output voltage
+in2_rated_min		Minimum rated output voltage
+in2_rated_max		Maximum rated output voltage
+in2_alarm		Output voltage alarm
+
+power1_label		"pin"
+power1_input		Measured input power
+power1_alarm		Input power high alarm
+
+power2_label		"pout1"
+power2_input		Measured output power
+power2_rated_max	Maximum rated output power
+
+temp[1-3]_input		Measured temperature
+temp[1-3]_alarm		Temperature alarm
+
+fan1_alarm		Fan 1 warning.
+fan1_fault		Fan 1 fault.
+fan1_input		Fan 1 speed in RPM.
+======================= ======================================================
diff --git a/Documentation/hwmon/index.rst b/Documentation/hwmon/index.rst
index 8d5a2df1ecb6..b24436f22052 100644
--- a/Documentation/hwmon/index.rst
+++ b/Documentation/hwmon/index.rst
@@ -54,6 +54,7 @@ Hardware Monitoring Kernel Drivers
    dell-smm-hwmon
    dme1737
    drivetemp
+   dps920ab
    ds1621
    ds620
    emc1403
diff --git a/drivers/hwmon/pmbus/Kconfig b/drivers/hwmon/pmbus/Kconfig
index 32d2fc850621..865ade0aa205 100644
--- a/drivers/hwmon/pmbus/Kconfig
+++ b/drivers/hwmon/pmbus/Kconfig
@@ -66,6 +66,15 @@ config SENSORS_IBM_CFFPS
 	  This driver can also be built as a module. If so, the module will
 	  be called ibm-cffps.
 
+config SENSORS_DPS920AB
+	tristate "Delta DPS920AB Power Supply"
+	help
+	  If you say yes here you get hardware monitoring support for Delta
+	  DPS920AB Power Supplies.
+
+	  This driver can also be built as a module. If so, the module will
+	  be called dps920ab.
+
 config SENSORS_INSPUR_IPSPS
 	tristate "INSPUR Power System Power Supply"
 	help
diff --git a/drivers/hwmon/pmbus/Makefile b/drivers/hwmon/pmbus/Makefile
index 6a4ba0fdc1db..f59ba0123d68 100644
--- a/drivers/hwmon/pmbus/Makefile
+++ b/drivers/hwmon/pmbus/Makefile
@@ -9,6 +9,7 @@ obj-$(CONFIG_SENSORS_ADM1266)	+= adm1266.o
 obj-$(CONFIG_SENSORS_ADM1275)	+= adm1275.o
 obj-$(CONFIG_SENSORS_BEL_PFE)	+= bel-pfe.o
 obj-$(CONFIG_SENSORS_IBM_CFFPS)	+= ibm-cffps.o
+obj-$(CONFIG_SENSORS_DPS920AB)	+= dps920ab.o
 obj-$(CONFIG_SENSORS_INSPUR_IPSPS) += inspur-ipsps.o
 obj-$(CONFIG_SENSORS_IR35221)	+= ir35221.o
 obj-$(CONFIG_SENSORS_IR38064)	+= ir38064.o
diff --git a/drivers/hwmon/pmbus/dps920ab.c b/drivers/hwmon/pmbus/dps920ab.c
new file mode 100644
index 000000000000..de084a42dec3
--- /dev/null
+++ b/drivers/hwmon/pmbus/dps920ab.c
@@ -0,0 +1,229 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Driver for Delta DPS920AB PSU
+ *
+ * Copyright (C) 2021 Delta Networks, Inc.
+ * Copyright (C) 2021 Sartura Ltd.
+ */
+
+#include <linux/debugfs.h>
+#include <linux/i2c.h>
+#include <linux/module.h>
+#include <linux/of_device.h>
+#include "pmbus.h"
+
+struct dps920ab_data {
+	struct i2c_client *client;
+	struct dentry *debugfs_dir;
+};
+
+static int dps920ab_read_word_data(struct i2c_client *client, int page, int phase, int reg)
+{
+	int ret;
+
+	/*
+	 * This masks commands which are not supported.
+	 * PSU advertises that all features are supported,
+	 * in reality that unfortunately is not true.
+	 * So enable only those that the datasheet confirms.
+	 */
+	switch (reg) {
+	case PMBUS_FAN_COMMAND_1:
+	case PMBUS_IOUT_OC_WARN_LIMIT:
+	case PMBUS_STATUS_WORD:
+	case PMBUS_READ_VIN:
+	case PMBUS_READ_IIN:
+	case PMBUS_READ_VOUT:
+	case PMBUS_READ_IOUT:
+	case PMBUS_READ_TEMPERATURE_1:
+	case PMBUS_READ_TEMPERATURE_2:
+	case PMBUS_READ_TEMPERATURE_3:
+	case PMBUS_READ_FAN_SPEED_1:
+	case PMBUS_READ_POUT:
+	case PMBUS_READ_PIN:
+	case PMBUS_MFR_VOUT_MIN:
+	case PMBUS_MFR_VOUT_MAX:
+	case PMBUS_MFR_IOUT_MAX:
+	case PMBUS_MFR_POUT_MAX:
+		break;
+	default:
+		return -ENXIO;
+	}
+
+	ret = pmbus_set_page(client, page, phase);
+	if (ret < 0)
+		return ret;
+
+	return i2c_smbus_read_word_data(client, reg);
+}
+
+static int dps920ab_write_word_data(struct i2c_client *client, int page, int reg,
+				    u16 word)
+{
+	int ret;
+
+	/*
+	 * This masks commands which are not supported.
+	 * PSU only has one R/W register and that is
+	 * for the fan.
+	 */
+	switch (reg) {
+	case PMBUS_FAN_COMMAND_1:
+		break;
+	default:
+		ret = -ENODATA;
+	}
+
+	ret = pmbus_set_page(client, page, 0xff);
+	if (ret < 0)
+		return ret;
+
+	return i2c_smbus_write_word_data(client, reg, word);
+}
+
+static struct pmbus_driver_info dps920ab_info = {
+	.pages = 1,
+
+	.format[PSC_VOLTAGE_IN] = linear,
+	.format[PSC_VOLTAGE_OUT] = linear,
+	.format[PSC_CURRENT_IN] = linear,
+	.format[PSC_CURRENT_OUT] = linear,
+	.format[PSC_POWER] = linear,
+	.format[PSC_FAN] = linear,
+	.format[PSC_TEMPERATURE] = linear,
+
+	.func[0] =
+		PMBUS_HAVE_VIN | PMBUS_HAVE_IIN | PMBUS_HAVE_PIN |
+		PMBUS_HAVE_VOUT | PMBUS_HAVE_IOUT | PMBUS_HAVE_POUT |
+		PMBUS_HAVE_TEMP  | PMBUS_HAVE_TEMP2 | PMBUS_HAVE_TEMP3 |
+		PMBUS_HAVE_FAN12 | PMBUS_HAVE_STATUS_FAN12 |
+		PMBUS_HAVE_STATUS_VOUT | PMBUS_HAVE_STATUS_IOUT |
+		PMBUS_HAVE_STATUS_INPUT | PMBUS_HAVE_STATUS_TEMP,
+	.read_word_data = dps920ab_read_word_data,
+	.write_word_data = dps920ab_write_word_data,
+};
+
+static int dps920ab_mfr_id_show(struct seq_file *s, void *data)
+{
+	struct dps920ab_data *priv = s->private;
+	u8 buf[I2C_SMBUS_BLOCK_MAX + 1];
+	int ret;
+
+	ret = i2c_smbus_read_block_data(priv->client, PMBUS_MFR_ID, buf);
+	if (ret < 0)
+		return ret;
+
+	seq_printf(s, "%s\n", buf);
+
+	return 0;
+}
+
+DEFINE_SHOW_ATTRIBUTE(dps920ab_mfr_id);
+
+static int dps920ab_mfr_model_show(struct seq_file *s, void *data)
+{
+	struct dps920ab_data *priv = s->private;
+	u8 buf[I2C_SMBUS_BLOCK_MAX + 1];
+	int ret;
+
+	ret = i2c_smbus_read_block_data(priv->client, PMBUS_MFR_MODEL, buf);
+	if (ret < 0)
+		return ret;
+
+	buf[ret] = '\0';
+
+	seq_printf(s, "%s\n", buf);
+
+	return 0;
+}
+
+DEFINE_SHOW_ATTRIBUTE(dps920ab_mfr_model);
+
+static void dps920ab_init_debugfs(struct dps920ab_data *data)
+{
+	struct dentry *root;
+
+	root = pmbus_get_debugfs_dir(data->client);
+	if (!root)
+		return;
+
+	data->debugfs_dir = debugfs_create_dir(data->client->name, root);
+	if (!data->debugfs_dir)
+		return;
+
+	debugfs_create_file("mfr_id",
+			    0400,
+			    data->debugfs_dir,
+			    data,
+			    &dps920ab_mfr_id_fops);
+
+	debugfs_create_file("mfr_model",
+			    0400,
+			    data->debugfs_dir,
+			    data,
+			    &dps920ab_mfr_model_fops);
+}
+
+static int dps920ab_probe(struct i2c_client *client)
+{
+	u8 buf[I2C_SMBUS_BLOCK_MAX + 1];
+	struct dps920ab_data *data;
+	int ret;
+
+	ret = i2c_smbus_read_block_data(client, PMBUS_MFR_ID, buf);
+	if (ret < 0) {
+		dev_err(&client->dev, "Failed to read Manufacturer ID\n");
+		return ret;
+	}
+	if (ret != 5 || strncmp(buf, "DELTA", 5)) {
+		buf[ret] = '\0';
+		dev_err(&client->dev, "Unsupported Manufacturer ID '%s'\n", buf);
+		return -ENODEV;
+	}
+
+	ret = i2c_smbus_read_block_data(client, PMBUS_MFR_MODEL, buf);
+	if (ret < 0) {
+		dev_err(&client->dev, "Failed to read Manufacturer Model\n");
+		return ret;
+	}
+	if (ret != 11 || strncmp(buf, "DPS-920AB", 9)) {
+		buf[ret] = '\0';
+		dev_err(&client->dev, "Unsupported Manufacturer Model '%s'\n", buf);
+		return -ENODEV;
+	}
+
+	ret = pmbus_do_probe(client, &dps920ab_info);
+	if (ret)
+		return ret;
+
+	data = devm_kzalloc(&client->dev, sizeof(*data), GFP_KERNEL);
+	if (!data)
+		return -ENOMEM;
+
+	data->client = client;
+
+	dps920ab_init_debugfs(data);
+
+	return 0;
+}
+
+static const struct of_device_id __maybe_unused dps920ab_of_match[] = {
+	{ .compatible = "delta,dps920ab", },
+	{}
+};
+
+MODULE_DEVICE_TABLE(of, dps920ab_of_match);
+
+static struct i2c_driver dps920ab_driver = {
+	.driver = {
+		   .name = "dps920ab",
+		   .of_match_table = of_match_ptr(dps920ab_of_match),
+	},
+	.probe_new = dps920ab_probe,
+};
+
+module_i2c_driver(dps920ab_driver);
+
+MODULE_AUTHOR("Robert Marko <robert.marko@sartura.hr>");
+MODULE_DESCRIPTION("PMBus driver for Delta DPS920AB PSU");
+MODULE_LICENSE("GPL");
-- 
2.31.1


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

* [PATCH v2 2/3] dt-bindings: trivial-devices: Add Delta DPS920AB
  2021-05-28 10:38 [PATCH v2 1/3] hwmon: (pmbus) Add driver for Delta DPS-920AB PSU Robert Marko
@ 2021-05-28 10:38 ` Robert Marko
  2021-05-28 10:38 ` [PATCH v2 3/3] MAINTAINERS: Add Delta DPS920AB PSU driver Robert Marko
  2021-05-28 13:28 ` [PATCH v2 1/3] hwmon: (pmbus) Add driver for Delta DPS-920AB PSU Guenter Roeck
  2 siblings, 0 replies; 6+ messages in thread
From: Robert Marko @ 2021-05-28 10:38 UTC (permalink / raw)
  To: robh+dt, jdelvare, linux, corbet, linux-hwmon, linux-doc,
	linux-kernel, trivial, Jonathan.Cameron, alexandre.belloni, sst,
	krzk, alexandru.ardelean, devicetree
  Cc: luka.perkov, jmp, pmenzel, buczek, Robert Marko

Add trivial device entry for Delta DPS920AB PSU.

Signed-off-by: Robert Marko <robert.marko@sartura.hr>
---
Changes in v2:
* Drop custom bindings file and use trivial-devices

 Documentation/devicetree/bindings/trivial-devices.yaml | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/Documentation/devicetree/bindings/trivial-devices.yaml b/Documentation/devicetree/bindings/trivial-devices.yaml
index a327130d1faa..1cb6c1fc0903 100644
--- a/Documentation/devicetree/bindings/trivial-devices.yaml
+++ b/Documentation/devicetree/bindings/trivial-devices.yaml
@@ -68,6 +68,8 @@ properties:
           - dallas,ds4510
             # Digital Thermometer and Thermostat
           - dallas,ds75
+          # Delta Electronics DPS920AB 920W 54V Power Supply
+          - delta,dps920ab
             # 1/4 Brick DC/DC Regulated Power Module
           - delta,q54sj108a2
             # Devantech SRF02 ultrasonic ranger in I2C mode
-- 
2.31.1


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

* [PATCH v2 3/3] MAINTAINERS: Add Delta DPS920AB PSU driver
  2021-05-28 10:38 [PATCH v2 1/3] hwmon: (pmbus) Add driver for Delta DPS-920AB PSU Robert Marko
  2021-05-28 10:38 ` [PATCH v2 2/3] dt-bindings: trivial-devices: Add Delta DPS920AB Robert Marko
@ 2021-05-28 10:38 ` Robert Marko
  2021-05-28 13:28 ` [PATCH v2 1/3] hwmon: (pmbus) Add driver for Delta DPS-920AB PSU Guenter Roeck
  2 siblings, 0 replies; 6+ messages in thread
From: Robert Marko @ 2021-05-28 10:38 UTC (permalink / raw)
  To: robh+dt, jdelvare, linux, corbet, linux-hwmon, linux-doc,
	linux-kernel, trivial, Jonathan.Cameron, alexandre.belloni, sst,
	krzk, alexandru.ardelean, devicetree
  Cc: luka.perkov, jmp, pmenzel, buczek, Robert Marko

Add maintainers entry for the Delta DPS920AB PSU driver.

Signed-off-by: Robert Marko <robert.marko@sartura.hr>
---
Changes in v2:
* Drop YAML bindings

 MAINTAINERS | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index 3c3e2177349d..d12cac63689e 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -5103,6 +5103,13 @@ F:	Documentation/devicetree/bindings/gpio/delta,tn48m-gpio.yaml
 F:	Documentation/devicetree/bindings/mfd/delta,tn48m-cpld.yaml
 F:	drivers/gpio/gpio-tn48m.c
 
+DELTA DPS920AB PSU DRIVER
+M:	Robert Marko <robert.marko@sartura.hr>
+L:	linux-hwmon@vger.kernel.org
+S:	Maintained
+F:	Documentation/hwmon/dps920ab.rst
+F:	drivers/hwmon/pmbus/dps920ab.c
+
 DENALI NAND DRIVER
 L:	linux-mtd@lists.infradead.org
 S:	Orphan
-- 
2.31.1


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

* Re: [PATCH v2 1/3] hwmon: (pmbus) Add driver for Delta DPS-920AB PSU
  2021-05-28 10:38 [PATCH v2 1/3] hwmon: (pmbus) Add driver for Delta DPS-920AB PSU Robert Marko
  2021-05-28 10:38 ` [PATCH v2 2/3] dt-bindings: trivial-devices: Add Delta DPS920AB Robert Marko
  2021-05-28 10:38 ` [PATCH v2 3/3] MAINTAINERS: Add Delta DPS920AB PSU driver Robert Marko
@ 2021-05-28 13:28 ` Guenter Roeck
  2021-06-04 12:16   ` Robert Marko
  2 siblings, 1 reply; 6+ messages in thread
From: Guenter Roeck @ 2021-05-28 13:28 UTC (permalink / raw)
  To: Robert Marko, robh+dt, jdelvare, corbet, linux-hwmon, linux-doc,
	linux-kernel, trivial, Jonathan.Cameron, alexandre.belloni, sst,
	krzk, alexandru.ardelean, devicetree
  Cc: luka.perkov, jmp, pmenzel, buczek

On 5/28/21 3:38 AM, Robert Marko wrote:
> This adds support for the Delta DPS-920AB PSU.
> 
> Only missing feature is fan control which the PSU supports.
> 
> Signed-off-by: Robert Marko <robert.marko@sartura.hr>
> ---
> Changes in v2:
> * Check for Manufacturer and Model
> * Restrict word read/write to supported only
> * Update documentation to reflect driver changes
> * Add basic debugfs entries
> 
>   Documentation/hwmon/dps920ab.rst |  73 ++++++++++
>   Documentation/hwmon/index.rst    |   1 +
>   drivers/hwmon/pmbus/Kconfig      |   9 ++
>   drivers/hwmon/pmbus/Makefile     |   1 +
>   drivers/hwmon/pmbus/dps920ab.c   | 229 +++++++++++++++++++++++++++++++
>   5 files changed, 313 insertions(+)
>   create mode 100644 Documentation/hwmon/dps920ab.rst
>   create mode 100644 drivers/hwmon/pmbus/dps920ab.c
> 
> diff --git a/Documentation/hwmon/dps920ab.rst b/Documentation/hwmon/dps920ab.rst
> new file mode 100644
> index 000000000000..c33b4cdc0a60
> --- /dev/null
> +++ b/Documentation/hwmon/dps920ab.rst
> @@ -0,0 +1,73 @@
> +.. SPDX-License-Identifier: GPL-2.0-or-later
> +
> +Kernel driver dps920ab
> +========================
> +
> +Supported chips:
> +
> +  * Delta DPS920AB
> +
> +    Prefix: 'dps920ab'
> +
> +    Addresses scanned: -
> +
> +Authors:
> +    Robert Marko <robert.marko@sartura.hr>
> +
> +
> +Description
> +-----------
> +
> +This driver implements support for Delta DPS920AB 920W 54V DC single output
> +power supply with PMBus support.
> +
> +The driver is a client driver to the core PMBus driver.
> +Please see Documentation/hwmon/pmbus.rst 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.
> +
> +
> +Sysfs entries
> +-------------
> +
> +======================= ======================================================
> +curr1_label		"iin"
> +curr1_input		Measured input current
> +curr1_alarm		Input current high alarm
> +
> +curr2_label		"iout1"
> +curr2_input		Measured output current
> +curr2_max		Maximum output current
> +curr2_rated_max		Maximum rated output current
> +
> +in1_label		"vin"
> +in1_input		Measured input voltage
> +in1_alarm		Input voltage alarm
> +
> +in2_label		"vout1"
> +in2_input		Measured output voltage
> +in2_rated_min		Minimum rated output voltage
> +in2_rated_max		Maximum rated output voltage
> +in2_alarm		Output voltage alarm
> +
> +power1_label		"pin"
> +power1_input		Measured input power
> +power1_alarm		Input power high alarm
> +
> +power2_label		"pout1"
> +power2_input		Measured output power
> +power2_rated_max	Maximum rated output power
> +
> +temp[1-3]_input		Measured temperature
> +temp[1-3]_alarm		Temperature alarm
> +
> +fan1_alarm		Fan 1 warning.
> +fan1_fault		Fan 1 fault.
> +fan1_input		Fan 1 speed in RPM.
> +======================= ======================================================
> diff --git a/Documentation/hwmon/index.rst b/Documentation/hwmon/index.rst
> index 8d5a2df1ecb6..b24436f22052 100644
> --- a/Documentation/hwmon/index.rst
> +++ b/Documentation/hwmon/index.rst
> @@ -54,6 +54,7 @@ Hardware Monitoring Kernel Drivers
>      dell-smm-hwmon
>      dme1737
>      drivetemp
> +   dps920ab
>      ds1621
>      ds620
>      emc1403
> diff --git a/drivers/hwmon/pmbus/Kconfig b/drivers/hwmon/pmbus/Kconfig
> index 32d2fc850621..865ade0aa205 100644
> --- a/drivers/hwmon/pmbus/Kconfig
> +++ b/drivers/hwmon/pmbus/Kconfig
> @@ -66,6 +66,15 @@ config SENSORS_IBM_CFFPS
>   	  This driver can also be built as a module. If so, the module will
>   	  be called ibm-cffps.
>   
> +config SENSORS_DPS920AB
> +	tristate "Delta DPS920AB Power Supply"
> +	help
> +	  If you say yes here you get hardware monitoring support for Delta
> +	  DPS920AB Power Supplies.
> +
> +	  This driver can also be built as a module. If so, the module will
> +	  be called dps920ab.
> +
>   config SENSORS_INSPUR_IPSPS
>   	tristate "INSPUR Power System Power Supply"
>   	help
> diff --git a/drivers/hwmon/pmbus/Makefile b/drivers/hwmon/pmbus/Makefile
> index 6a4ba0fdc1db..f59ba0123d68 100644
> --- a/drivers/hwmon/pmbus/Makefile
> +++ b/drivers/hwmon/pmbus/Makefile
> @@ -9,6 +9,7 @@ obj-$(CONFIG_SENSORS_ADM1266)	+= adm1266.o
>   obj-$(CONFIG_SENSORS_ADM1275)	+= adm1275.o
>   obj-$(CONFIG_SENSORS_BEL_PFE)	+= bel-pfe.o
>   obj-$(CONFIG_SENSORS_IBM_CFFPS)	+= ibm-cffps.o
> +obj-$(CONFIG_SENSORS_DPS920AB)	+= dps920ab.o
>   obj-$(CONFIG_SENSORS_INSPUR_IPSPS) += inspur-ipsps.o
>   obj-$(CONFIG_SENSORS_IR35221)	+= ir35221.o
>   obj-$(CONFIG_SENSORS_IR38064)	+= ir38064.o
> diff --git a/drivers/hwmon/pmbus/dps920ab.c b/drivers/hwmon/pmbus/dps920ab.c
> new file mode 100644
> index 000000000000..de084a42dec3
> --- /dev/null
> +++ b/drivers/hwmon/pmbus/dps920ab.c
> @@ -0,0 +1,229 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * Driver for Delta DPS920AB PSU
> + *
> + * Copyright (C) 2021 Delta Networks, Inc.
> + * Copyright (C) 2021 Sartura Ltd.
> + */
> +
> +#include <linux/debugfs.h>
> +#include <linux/i2c.h>
> +#include <linux/module.h>
> +#include <linux/of_device.h>
> +#include "pmbus.h"
> +
> +struct dps920ab_data {
> +	struct i2c_client *client;
> +	struct dentry *debugfs_dir;
> +};
> +
> +static int dps920ab_read_word_data(struct i2c_client *client, int page, int phase, int reg)
> +{
> +	int ret;
> +
> +	/*
> +	 * This masks commands which are not supported.
> +	 * PSU advertises that all features are supported,
> +	 * in reality that unfortunately is not true.
> +	 * So enable only those that the datasheet confirms.
> +	 */
> +	switch (reg) {
> +	case PMBUS_FAN_COMMAND_1:
> +	case PMBUS_IOUT_OC_WARN_LIMIT:
> +	case PMBUS_STATUS_WORD:
> +	case PMBUS_READ_VIN:
> +	case PMBUS_READ_IIN:
> +	case PMBUS_READ_VOUT:
> +	case PMBUS_READ_IOUT:
> +	case PMBUS_READ_TEMPERATURE_1:
> +	case PMBUS_READ_TEMPERATURE_2:
> +	case PMBUS_READ_TEMPERATURE_3:
> +	case PMBUS_READ_FAN_SPEED_1:
> +	case PMBUS_READ_POUT:
> +	case PMBUS_READ_PIN:
> +	case PMBUS_MFR_VOUT_MIN:
> +	case PMBUS_MFR_VOUT_MAX:
> +	case PMBUS_MFR_IOUT_MAX:
> +	case PMBUS_MFR_POUT_MAX:
> +		break;
> +	default:
> +		return -ENXIO;
> +	}
> +
> +	ret = pmbus_set_page(client, page, phase);
> +	if (ret < 0)
> +		return ret;
> +
> +	return i2c_smbus_read_word_data(client, reg);

I find the code a bit confusing. I think it would be better to move the action code
into the case statement.

> +}
> +
> +static int dps920ab_write_word_data(struct i2c_client *client, int page, int reg,
> +				    u16 word)
> +{
> +	int ret;
> +
> +	/*
> +	 * This masks commands which are not supported.
> +	 * PSU only has one R/W register and that is
> +	 * for the fan.
> +	 */
> +	switch (reg) {
> +	case PMBUS_FAN_COMMAND_1:
> +		break;
> +	default:
> +		ret = -ENODATA;
> +	}
> +

Does this work ? It is the wrong error message for a failed write;
it should probably return -EACCES.

> +	ret = pmbus_set_page(client, page, 0xff);
> +	if (ret < 0)
> +		return ret;
> +
There is only one page, so what would this be for ?

> +	return i2c_smbus_write_word_data(client, reg, word);

Same as avove; I think it would be better to handle the
access code in the case statement.

> +}
> +
> +static struct pmbus_driver_info dps920ab_info = {
> +	.pages = 1,
> +
> +	.format[PSC_VOLTAGE_IN] = linear,
> +	.format[PSC_VOLTAGE_OUT] = linear,
> +	.format[PSC_CURRENT_IN] = linear,
> +	.format[PSC_CURRENT_OUT] = linear,
> +	.format[PSC_POWER] = linear,
> +	.format[PSC_FAN] = linear,
> +	.format[PSC_TEMPERATURE] = linear,
> +
> +	.func[0] =
> +		PMBUS_HAVE_VIN | PMBUS_HAVE_IIN | PMBUS_HAVE_PIN |
> +		PMBUS_HAVE_VOUT | PMBUS_HAVE_IOUT | PMBUS_HAVE_POUT |
> +		PMBUS_HAVE_TEMP  | PMBUS_HAVE_TEMP2 | PMBUS_HAVE_TEMP3 |
> +		PMBUS_HAVE_FAN12 | PMBUS_HAVE_STATUS_FAN12 |
> +		PMBUS_HAVE_STATUS_VOUT | PMBUS_HAVE_STATUS_IOUT |
> +		PMBUS_HAVE_STATUS_INPUT | PMBUS_HAVE_STATUS_TEMP,
> +	.read_word_data = dps920ab_read_word_data,
> +	.write_word_data = dps920ab_write_word_data,
> +};
> +
> +static int dps920ab_mfr_id_show(struct seq_file *s, void *data)
> +{
> +	struct dps920ab_data *priv = s->private;
> +	u8 buf[I2C_SMBUS_BLOCK_MAX + 1];
> +	int ret;
> +
> +	ret = i2c_smbus_read_block_data(priv->client, PMBUS_MFR_ID, buf);
> +	if (ret < 0)
> +		return ret;
> +
	buf[ret] = '\0';

missing here. With that, the two functions are effectively the same
and can be merged into one with the register as additional parameter.
Also, you might consider storing this information in struct dps920ab_data.
After all, it is unlikely to change, so reading it over and over again
has no real value.

> +	seq_printf(s, "%s\n", buf);
> +
> +	return 0;
> +}
> +
> +DEFINE_SHOW_ATTRIBUTE(dps920ab_mfr_id);
> +
> +static int dps920ab_mfr_model_show(struct seq_file *s, void *data)
> +{
> +	struct dps920ab_data *priv = s->private;
> +	u8 buf[I2C_SMBUS_BLOCK_MAX + 1];
> +	int ret;
> +
> +	ret = i2c_smbus_read_block_data(priv->client, PMBUS_MFR_MODEL, buf);
> +	if (ret < 0)
> +		return ret;
> +
> +	buf[ret] = '\0';
> +
> +	seq_printf(s, "%s\n", buf);
> +
> +	return 0;
> +}
> +
> +DEFINE_SHOW_ATTRIBUTE(dps920ab_mfr_model);
> +
> +static void dps920ab_init_debugfs(struct dps920ab_data *data)
> +{
> +	struct dentry *root;
> +
> +	root = pmbus_get_debugfs_dir(data->client);
> +	if (!root)
> +		return;
> +
> +	data->debugfs_dir = debugfs_create_dir(data->client->name, root);
> +	if (!data->debugfs_dir)
> +		return;
> +
> +	debugfs_create_file("mfr_id",
> +			    0400,
> +			    data->debugfs_dir,
> +			    data,
> +			    &dps920ab_mfr_id_fops);
> +
> +	debugfs_create_file("mfr_model",
> +			    0400,
> +			    data->debugfs_dir,
> +			    data,
> +			    &dps920ab_mfr_model_fops);
> +}
> +
> +static int dps920ab_probe(struct i2c_client *client)
> +{
> +	u8 buf[I2C_SMBUS_BLOCK_MAX + 1];
> +	struct dps920ab_data *data;
> +	int ret;
> +
> +	ret = i2c_smbus_read_block_data(client, PMBUS_MFR_ID, buf);
> +	if (ret < 0) {
> +		dev_err(&client->dev, "Failed to read Manufacturer ID\n");
> +		return ret;
> +	}
> +	if (ret != 5 || strncmp(buf, "DELTA", 5)) {

Does this return 5 or 6 ? Just asking, because other supplies from Delta
return 6.

> +		buf[ret] = '\0';
> +		dev_err(&client->dev, "Unsupported Manufacturer ID '%s'\n", buf);
> +		return -ENODEV;
> +	}
> +
> +	ret = i2c_smbus_read_block_data(client, PMBUS_MFR_MODEL, buf);
> +	if (ret < 0) {
> +		dev_err(&client->dev, "Failed to read Manufacturer Model\n");
> +		return ret;
> +	}
> +	if (ret != 11 || strncmp(buf, "DPS-920AB", 9)) {
> +		buf[ret] = '\0';
> +		dev_err(&client->dev, "Unsupported Manufacturer Model '%s'\n", buf);
> +		return -ENODEV;
> +	}
> +
> +	ret = pmbus_do_probe(client, &dps920ab_info);
> +	if (ret)
> +		return ret;
> +
> +	data = devm_kzalloc(&client->dev, sizeof(*data), GFP_KERNEL);
> +	if (!data)
> +		return -ENOMEM;
> +
> +	data->client = client;
> +
> +	dps920ab_init_debugfs(data);
> +
> +	return 0;
> +}
> +
> +static const struct of_device_id __maybe_unused dps920ab_of_match[] = {
> +	{ .compatible = "delta,dps920ab", },
> +	{}
> +};
> +
> +MODULE_DEVICE_TABLE(of, dps920ab_of_match);
> +
> +static struct i2c_driver dps920ab_driver = {
> +	.driver = {
> +		   .name = "dps920ab",
> +		   .of_match_table = of_match_ptr(dps920ab_of_match),
> +	},
> +	.probe_new = dps920ab_probe,
> +};
> +
> +module_i2c_driver(dps920ab_driver);
> +
> +MODULE_AUTHOR("Robert Marko <robert.marko@sartura.hr>");
> +MODULE_DESCRIPTION("PMBus driver for Delta DPS920AB PSU");
> +MODULE_LICENSE("GPL");
> 


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

* Re: [PATCH v2 1/3] hwmon: (pmbus) Add driver for Delta DPS-920AB PSU
  2021-05-28 13:28 ` [PATCH v2 1/3] hwmon: (pmbus) Add driver for Delta DPS-920AB PSU Guenter Roeck
@ 2021-06-04 12:16   ` Robert Marko
  2021-06-04 12:38     ` Guenter Roeck
  0 siblings, 1 reply; 6+ messages in thread
From: Robert Marko @ 2021-06-04 12:16 UTC (permalink / raw)
  To: Guenter Roeck
  Cc: robh+dt, jdelvare, corbet, linux-hwmon, linux-doc, linux-kernel,
	trivial, Jonathan.Cameron, alexandre.belloni, sst, krzk,
	alexandru.ardelean, devicetree, Luka Perkov, jmp, Paul Menzel,
	Donald Buczek

On Fri, May 28, 2021 at 3:28 PM Guenter Roeck <linux@roeck-us.net> wrote:
>
> On 5/28/21 3:38 AM, Robert Marko wrote:
> > This adds support for the Delta DPS-920AB PSU.
> >
> > Only missing feature is fan control which the PSU supports.
> >
> > Signed-off-by: Robert Marko <robert.marko@sartura.hr>
> > ---
> > Changes in v2:
> > * Check for Manufacturer and Model
> > * Restrict word read/write to supported only
> > * Update documentation to reflect driver changes
> > * Add basic debugfs entries
> >
> >   Documentation/hwmon/dps920ab.rst |  73 ++++++++++
> >   Documentation/hwmon/index.rst    |   1 +
> >   drivers/hwmon/pmbus/Kconfig      |   9 ++
> >   drivers/hwmon/pmbus/Makefile     |   1 +
> >   drivers/hwmon/pmbus/dps920ab.c   | 229 +++++++++++++++++++++++++++++++
> >   5 files changed, 313 insertions(+)
> >   create mode 100644 Documentation/hwmon/dps920ab.rst
> >   create mode 100644 drivers/hwmon/pmbus/dps920ab.c
> >
> > diff --git a/Documentation/hwmon/dps920ab.rst b/Documentation/hwmon/dps920ab.rst
> > new file mode 100644
> > index 000000000000..c33b4cdc0a60
> > --- /dev/null
> > +++ b/Documentation/hwmon/dps920ab.rst
> > @@ -0,0 +1,73 @@
> > +.. SPDX-License-Identifier: GPL-2.0-or-later
> > +
> > +Kernel driver dps920ab
> > +========================
> > +
> > +Supported chips:
> > +
> > +  * Delta DPS920AB
> > +
> > +    Prefix: 'dps920ab'
> > +
> > +    Addresses scanned: -
> > +
> > +Authors:
> > +    Robert Marko <robert.marko@sartura.hr>
> > +
> > +
> > +Description
> > +-----------
> > +
> > +This driver implements support for Delta DPS920AB 920W 54V DC single output
> > +power supply with PMBus support.
> > +
> > +The driver is a client driver to the core PMBus driver.
> > +Please see Documentation/hwmon/pmbus.rst 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.
> > +
> > +
> > +Sysfs entries
> > +-------------
> > +
> > +======================= ======================================================
> > +curr1_label          "iin"
> > +curr1_input          Measured input current
> > +curr1_alarm          Input current high alarm
> > +
> > +curr2_label          "iout1"
> > +curr2_input          Measured output current
> > +curr2_max            Maximum output current
> > +curr2_rated_max              Maximum rated output current
> > +
> > +in1_label            "vin"
> > +in1_input            Measured input voltage
> > +in1_alarm            Input voltage alarm
> > +
> > +in2_label            "vout1"
> > +in2_input            Measured output voltage
> > +in2_rated_min                Minimum rated output voltage
> > +in2_rated_max                Maximum rated output voltage
> > +in2_alarm            Output voltage alarm
> > +
> > +power1_label         "pin"
> > +power1_input         Measured input power
> > +power1_alarm         Input power high alarm
> > +
> > +power2_label         "pout1"
> > +power2_input         Measured output power
> > +power2_rated_max     Maximum rated output power
> > +
> > +temp[1-3]_input              Measured temperature
> > +temp[1-3]_alarm              Temperature alarm
> > +
> > +fan1_alarm           Fan 1 warning.
> > +fan1_fault           Fan 1 fault.
> > +fan1_input           Fan 1 speed in RPM.
> > +======================= ======================================================
> > diff --git a/Documentation/hwmon/index.rst b/Documentation/hwmon/index.rst
> > index 8d5a2df1ecb6..b24436f22052 100644
> > --- a/Documentation/hwmon/index.rst
> > +++ b/Documentation/hwmon/index.rst
> > @@ -54,6 +54,7 @@ Hardware Monitoring Kernel Drivers
> >      dell-smm-hwmon
> >      dme1737
> >      drivetemp
> > +   dps920ab
> >      ds1621
> >      ds620
> >      emc1403
> > diff --git a/drivers/hwmon/pmbus/Kconfig b/drivers/hwmon/pmbus/Kconfig
> > index 32d2fc850621..865ade0aa205 100644
> > --- a/drivers/hwmon/pmbus/Kconfig
> > +++ b/drivers/hwmon/pmbus/Kconfig
> > @@ -66,6 +66,15 @@ config SENSORS_IBM_CFFPS
> >         This driver can also be built as a module. If so, the module will
> >         be called ibm-cffps.
> >
> > +config SENSORS_DPS920AB
> > +     tristate "Delta DPS920AB Power Supply"
> > +     help
> > +       If you say yes here you get hardware monitoring support for Delta
> > +       DPS920AB Power Supplies.
> > +
> > +       This driver can also be built as a module. If so, the module will
> > +       be called dps920ab.
> > +
> >   config SENSORS_INSPUR_IPSPS
> >       tristate "INSPUR Power System Power Supply"
> >       help
> > diff --git a/drivers/hwmon/pmbus/Makefile b/drivers/hwmon/pmbus/Makefile
> > index 6a4ba0fdc1db..f59ba0123d68 100644
> > --- a/drivers/hwmon/pmbus/Makefile
> > +++ b/drivers/hwmon/pmbus/Makefile
> > @@ -9,6 +9,7 @@ obj-$(CONFIG_SENSORS_ADM1266) += adm1266.o
> >   obj-$(CONFIG_SENSORS_ADM1275)       += adm1275.o
> >   obj-$(CONFIG_SENSORS_BEL_PFE)       += bel-pfe.o
> >   obj-$(CONFIG_SENSORS_IBM_CFFPS)     += ibm-cffps.o
> > +obj-$(CONFIG_SENSORS_DPS920AB)       += dps920ab.o
> >   obj-$(CONFIG_SENSORS_INSPUR_IPSPS) += inspur-ipsps.o
> >   obj-$(CONFIG_SENSORS_IR35221)       += ir35221.o
> >   obj-$(CONFIG_SENSORS_IR38064)       += ir38064.o
> > diff --git a/drivers/hwmon/pmbus/dps920ab.c b/drivers/hwmon/pmbus/dps920ab.c
> > new file mode 100644
> > index 000000000000..de084a42dec3
> > --- /dev/null
> > +++ b/drivers/hwmon/pmbus/dps920ab.c
> > @@ -0,0 +1,229 @@
> > +// SPDX-License-Identifier: GPL-2.0-or-later
> > +/*
> > + * Driver for Delta DPS920AB PSU
> > + *
> > + * Copyright (C) 2021 Delta Networks, Inc.
> > + * Copyright (C) 2021 Sartura Ltd.
> > + */
> > +
> > +#include <linux/debugfs.h>
> > +#include <linux/i2c.h>
> > +#include <linux/module.h>
> > +#include <linux/of_device.h>
> > +#include "pmbus.h"
> > +
> > +struct dps920ab_data {
> > +     struct i2c_client *client;
> > +     struct dentry *debugfs_dir;
> > +};
> > +
> > +static int dps920ab_read_word_data(struct i2c_client *client, int page, int phase, int reg)
> > +{
> > +     int ret;
> > +
> > +     /*
> > +      * This masks commands which are not supported.
> > +      * PSU advertises that all features are supported,
> > +      * in reality that unfortunately is not true.
> > +      * So enable only those that the datasheet confirms.
> > +      */
> > +     switch (reg) {
> > +     case PMBUS_FAN_COMMAND_1:
> > +     case PMBUS_IOUT_OC_WARN_LIMIT:
> > +     case PMBUS_STATUS_WORD:
> > +     case PMBUS_READ_VIN:
> > +     case PMBUS_READ_IIN:
> > +     case PMBUS_READ_VOUT:
> > +     case PMBUS_READ_IOUT:
> > +     case PMBUS_READ_TEMPERATURE_1:
> > +     case PMBUS_READ_TEMPERATURE_2:
> > +     case PMBUS_READ_TEMPERATURE_3:
> > +     case PMBUS_READ_FAN_SPEED_1:
> > +     case PMBUS_READ_POUT:
> > +     case PMBUS_READ_PIN:
> > +     case PMBUS_MFR_VOUT_MIN:
> > +     case PMBUS_MFR_VOUT_MAX:
> > +     case PMBUS_MFR_IOUT_MAX:
> > +     case PMBUS_MFR_POUT_MAX:
> > +             break;
> > +     default:
> > +             return -ENXIO;
> > +     }
> > +
> > +     ret = pmbus_set_page(client, page, phase);
> > +     if (ret < 0)
> > +             return ret;
> > +
> > +     return i2c_smbus_read_word_data(client, reg);
>
> I find the code a bit confusing. I think it would be better to move the action code
> into the case statement.

Agree, makes sense.
>
> > +}
> > +
> > +static int dps920ab_write_word_data(struct i2c_client *client, int page, int reg,
> > +                                 u16 word)
> > +{
> > +     int ret;
> > +
> > +     /*
> > +      * This masks commands which are not supported.
> > +      * PSU only has one R/W register and that is
> > +      * for the fan.
> > +      */
> > +     switch (reg) {
> > +     case PMBUS_FAN_COMMAND_1:
> > +             break;
> > +     default:
> > +             ret = -ENODATA;
> > +     }
> > +
>
> Does this work ? It is the wrong error message for a failed write;
> it should probably return -EACCES.

It appears to work fine, I was looking at other drivers, and all of them
use -ENODATA for unsupported word registers.
>
> > +     ret = pmbus_set_page(client, page, 0xff);
> > +     if (ret < 0)
> > +             return ret;
> > +
> There is only one page, so what would this be for ?
>
> > +     return i2c_smbus_write_word_data(client, reg, word);
>
> Same as avove; I think it would be better to handle the
> access code in the case statement.

Agreed, will do.
>
> > +}
> > +
> > +static struct pmbus_driver_info dps920ab_info = {
> > +     .pages = 1,
> > +
> > +     .format[PSC_VOLTAGE_IN] = linear,
> > +     .format[PSC_VOLTAGE_OUT] = linear,
> > +     .format[PSC_CURRENT_IN] = linear,
> > +     .format[PSC_CURRENT_OUT] = linear,
> > +     .format[PSC_POWER] = linear,
> > +     .format[PSC_FAN] = linear,
> > +     .format[PSC_TEMPERATURE] = linear,
> > +
> > +     .func[0] =
> > +             PMBUS_HAVE_VIN | PMBUS_HAVE_IIN | PMBUS_HAVE_PIN |
> > +             PMBUS_HAVE_VOUT | PMBUS_HAVE_IOUT | PMBUS_HAVE_POUT |
> > +             PMBUS_HAVE_TEMP  | PMBUS_HAVE_TEMP2 | PMBUS_HAVE_TEMP3 |
> > +             PMBUS_HAVE_FAN12 | PMBUS_HAVE_STATUS_FAN12 |
> > +             PMBUS_HAVE_STATUS_VOUT | PMBUS_HAVE_STATUS_IOUT |
> > +             PMBUS_HAVE_STATUS_INPUT | PMBUS_HAVE_STATUS_TEMP,
> > +     .read_word_data = dps920ab_read_word_data,
> > +     .write_word_data = dps920ab_write_word_data,
> > +};
> > +
> > +static int dps920ab_mfr_id_show(struct seq_file *s, void *data)
> > +{
> > +     struct dps920ab_data *priv = s->private;
> > +     u8 buf[I2C_SMBUS_BLOCK_MAX + 1];
> > +     int ret;
> > +
> > +     ret = i2c_smbus_read_block_data(priv->client, PMBUS_MFR_ID, buf);
> > +     if (ret < 0)
> > +             return ret;
> > +
>         buf[ret] = '\0';
>
> missing here. With that, the two functions are effectively the same
> and can be merged into one with the register as additional parameter.
> Also, you might consider storing this information in struct dps920ab_data.
> After all, it is unlikely to change, so reading it over and over again
> has no real value.

Ok, makes sense.

>
> > +     seq_printf(s, "%s\n", buf);
> > +
> > +     return 0;
> > +}
> > +
> > +DEFINE_SHOW_ATTRIBUTE(dps920ab_mfr_id);
> > +
> > +static int dps920ab_mfr_model_show(struct seq_file *s, void *data)
> > +{
> > +     struct dps920ab_data *priv = s->private;
> > +     u8 buf[I2C_SMBUS_BLOCK_MAX + 1];
> > +     int ret;
> > +
> > +     ret = i2c_smbus_read_block_data(priv->client, PMBUS_MFR_MODEL, buf);
> > +     if (ret < 0)
> > +             return ret;
> > +
> > +     buf[ret] = '\0';
> > +
> > +     seq_printf(s, "%s\n", buf);
> > +
> > +     return 0;
> > +}
> > +
> > +DEFINE_SHOW_ATTRIBUTE(dps920ab_mfr_model);
> > +
> > +static void dps920ab_init_debugfs(struct dps920ab_data *data)
> > +{
> > +     struct dentry *root;
> > +
> > +     root = pmbus_get_debugfs_dir(data->client);
> > +     if (!root)
> > +             return;
> > +
> > +     data->debugfs_dir = debugfs_create_dir(data->client->name, root);
> > +     if (!data->debugfs_dir)
> > +             return;
> > +
> > +     debugfs_create_file("mfr_id",
> > +                         0400,
> > +                         data->debugfs_dir,
> > +                         data,
> > +                         &dps920ab_mfr_id_fops);
> > +
> > +     debugfs_create_file("mfr_model",
> > +                         0400,
> > +                         data->debugfs_dir,
> > +                         data,
> > +                         &dps920ab_mfr_model_fops);
> > +}
> > +
> > +static int dps920ab_probe(struct i2c_client *client)
> > +{
> > +     u8 buf[I2C_SMBUS_BLOCK_MAX + 1];
> > +     struct dps920ab_data *data;
> > +     int ret;
> > +
> > +     ret = i2c_smbus_read_block_data(client, PMBUS_MFR_ID, buf);
> > +     if (ret < 0) {
> > +             dev_err(&client->dev, "Failed to read Manufacturer ID\n");
> > +             return ret;
> > +     }
> > +     if (ret != 5 || strncmp(buf, "DELTA", 5)) {
>
> Does this return 5 or 6 ? Just asking, because other supplies from Delta
> return 6.

This was weird for me as well, but it actually returns 5, I think only
the brick q54sj108a2
does that as well.

I will send out a v3 today.

Regards,
Robert
>
> > +             buf[ret] = '\0';
> > +             dev_err(&client->dev, "Unsupported Manufacturer ID '%s'\n", buf);
> > +             return -ENODEV;
> > +     }
> > +
> > +     ret = i2c_smbus_read_block_data(client, PMBUS_MFR_MODEL, buf);
> > +     if (ret < 0) {
> > +             dev_err(&client->dev, "Failed to read Manufacturer Model\n");
> > +             return ret;
> > +     }
> > +     if (ret != 11 || strncmp(buf, "DPS-920AB", 9)) {
> > +             buf[ret] = '\0';
> > +             dev_err(&client->dev, "Unsupported Manufacturer Model '%s'\n", buf);
> > +             return -ENODEV;
> > +     }
> > +
> > +     ret = pmbus_do_probe(client, &dps920ab_info);
> > +     if (ret)
> > +             return ret;
> > +
> > +     data = devm_kzalloc(&client->dev, sizeof(*data), GFP_KERNEL);
> > +     if (!data)
> > +             return -ENOMEM;
> > +
> > +     data->client = client;
> > +
> > +     dps920ab_init_debugfs(data);
> > +
> > +     return 0;
> > +}
> > +
> > +static const struct of_device_id __maybe_unused dps920ab_of_match[] = {
> > +     { .compatible = "delta,dps920ab", },
> > +     {}
> > +};
> > +
> > +MODULE_DEVICE_TABLE(of, dps920ab_of_match);
> > +
> > +static struct i2c_driver dps920ab_driver = {
> > +     .driver = {
> > +                .name = "dps920ab",
> > +                .of_match_table = of_match_ptr(dps920ab_of_match),
> > +     },
> > +     .probe_new = dps920ab_probe,
> > +};
> > +
> > +module_i2c_driver(dps920ab_driver);
> > +
> > +MODULE_AUTHOR("Robert Marko <robert.marko@sartura.hr>");
> > +MODULE_DESCRIPTION("PMBus driver for Delta DPS920AB PSU");
> > +MODULE_LICENSE("GPL");
> >
>


-- 
Robert Marko
Staff Embedded Linux Engineer
Sartura Ltd.
Lendavska ulica 16a
10000 Zagreb, Croatia
Email: robert.marko@sartura.hr
Web: www.sartura.hr

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

* Re: [PATCH v2 1/3] hwmon: (pmbus) Add driver for Delta DPS-920AB PSU
  2021-06-04 12:16   ` Robert Marko
@ 2021-06-04 12:38     ` Guenter Roeck
  0 siblings, 0 replies; 6+ messages in thread
From: Guenter Roeck @ 2021-06-04 12:38 UTC (permalink / raw)
  To: Robert Marko
  Cc: robh+dt, jdelvare, corbet, linux-hwmon, linux-doc, linux-kernel,
	trivial, Jonathan.Cameron, alexandre.belloni, sst, krzk,
	alexandru.ardelean, devicetree, Luka Perkov, jmp, Paul Menzel,
	Donald Buczek

On Fri, Jun 04, 2021 at 02:16:02PM +0200, Robert Marko wrote:
[ ... ]
> >
> > > +}
> > > +
> > > +static int dps920ab_write_word_data(struct i2c_client *client, int page, int reg,
> > > +                                 u16 word)
> > > +{
> > > +     int ret;
> > > +
> > > +     /*
> > > +      * This masks commands which are not supported.
> > > +      * PSU only has one R/W register and that is
> > > +      * for the fan.
> > > +      */
> > > +     switch (reg) {
> > > +     case PMBUS_FAN_COMMAND_1:
> > > +             break;
> > > +     default:
> > > +             ret = -ENODATA;
> > > +     }
> > > +
> >
> > Does this work ? It is the wrong error message for a failed write;
> > it should probably return -EACCES.
> 
> It appears to work fine, I was looking at other drivers, and all of them
> use -ENODATA for unsupported word registers.

No. They return -ENODATA to tell the PMBus core to use standard PMBus
access functions, ie to handle the access in the core. Please see
_pmbus_write_word_data().

Guenter

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

end of thread, other threads:[~2021-06-04 12:39 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-05-28 10:38 [PATCH v2 1/3] hwmon: (pmbus) Add driver for Delta DPS-920AB PSU Robert Marko
2021-05-28 10:38 ` [PATCH v2 2/3] dt-bindings: trivial-devices: Add Delta DPS920AB Robert Marko
2021-05-28 10:38 ` [PATCH v2 3/3] MAINTAINERS: Add Delta DPS920AB PSU driver Robert Marko
2021-05-28 13:28 ` [PATCH v2 1/3] hwmon: (pmbus) Add driver for Delta DPS-920AB PSU Guenter Roeck
2021-06-04 12:16   ` Robert Marko
2021-06-04 12:38     ` Guenter Roeck

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.