Message-ID: <479725a2-7d85-1489-4228-ddff16b52287@roeck-us.net>
Date:   Fri, 28 May 2021 06:28:02 -0700
From:   Guenter Roeck <linux@...ck-us.net>
To:     Robert Marko <robert.marko@...tura.hr>, robh+dt@...nel.org,
        jdelvare@...e.com, corbet@....net, linux-hwmon@...r.kernel.org,
        linux-doc@...r.kernel.org, linux-kernel@...r.kernel.org,
        trivial@...nel.org, Jonathan.Cameron@...wei.com,
        alexandre.belloni@...tlin.com, sst@...zta.fm, krzk@...nel.org,
        alexandru.ardelean@...log.com, devicetree@...r.kernel.org
Cc:     luka.perkov@...tura.hr, jmp@...phyte.org, pmenzel@...gen.mpg.de,
        buczek@...gen.mpg.de
Subject: Re: [PATCH v2 1/3] hwmon: (pmbus) Add driver for Delta DPS-920AB PSU

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@...tura.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@...tura.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@...tura.hr>");
> +MODULE_DESCRIPTION("PMBus driver for Delta DPS920AB PSU");
> +MODULE_LICENSE("GPL");
> 

Powered by blists - more mailing lists