Re: [PATCH] [PATCH] hwmon/pmbus/Q54SJ108A2: new driver for Delta modules Q54SJ108A2

[Guenter Roeck]

On 8/18/20 7:21 PM, xiao.mx.ma wrote:
> The driver is used for Q54SJ108A2 series.
> 
> Signed-off-by: xiao.mx.ma <734056705@xxxxxx>

there is a gazillion of unnecessary empty lines. Please drop all those.
checkpatch --strict might help.

Other comments inline.

> ---
>  drivers/hwmon/pmbus/Kconfig      |   9 +
>  drivers/hwmon/pmbus/Makefile     |   1 +
>  drivers/hwmon/pmbus/Q54SJ108A2.c | 404 +++++++++++++++++++++++++++++++
>  3 files changed, 414 insertions(+)
>  create mode 100644 drivers/hwmon/pmbus/Q54SJ108A2.c
> 
> diff --git a/drivers/hwmon/pmbus/Kconfig b/drivers/hwmon/pmbus/Kconfig
> index e35db489b76f..b4bd6ac491c8 100644
> --- a/drivers/hwmon/pmbus/Kconfig
> +++ b/drivers/hwmon/pmbus/Kconfig
> @@ -209,6 +209,15 @@ config SENSORS_PXE1610
>  	  This driver can also be built as a module. If so, the module will
>  	  be called pxe1610.
>  
> +config SENSORS_Q54SJ108A2
> +	tristate "Delta Q54SJ108A2"
> +	help
> +	  If you say yes here you get hardware monitoring support for Delta modules
> +	  Q54SJ108A2.
> +
> +	  This driver can also be built as a module. If so, the module will
> +	  be called Q54SJ108A2.
> +
>  config SENSORS_TPS40422
>  	tristate "TI TPS40422"
>  	help
> diff --git a/drivers/hwmon/pmbus/Makefile b/drivers/hwmon/pmbus/Makefile
> index c4b15db996ad..4536c57ef1a4 100644
> --- a/drivers/hwmon/pmbus/Makefile
> +++ b/drivers/hwmon/pmbus/Makefile
> @@ -30,3 +30,4 @@ obj-$(CONFIG_SENSORS_UCD9000)	+= ucd9000.o
>  obj-$(CONFIG_SENSORS_UCD9200)	+= ucd9200.o
>  obj-$(CONFIG_SENSORS_XDPE122)	+= xdpe12284.o
>  obj-$(CONFIG_SENSORS_ZL6100)	+= zl6100.o
> +obj-$(CONFIG_SENSORS_Q54SJ108A2)	+= Q54SJ108A2.o
> diff --git a/drivers/hwmon/pmbus/Q54SJ108A2.c b/drivers/hwmon/pmbus/Q54SJ108A2.c
> new file mode 100644
> index 000000000000..67fac5506763
> --- /dev/null
> +++ b/drivers/hwmon/pmbus/Q54SJ108A2.c

This is way too cryptic. Please select a human readable file name (delta.c would do).

> @@ -0,0 +1,404 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +
> +/*
> + * Driver for Q54SJ108A2, Q50SN12050, and Q50SN12072 Integrated, Step-Down
> + * Switching Regulators
> + *
> + * Copyright 2020 Delta LLC.
> + */
> +
> +
> +#include <linux/bits.h>
> +
> +#include <linux/err.h>
> +
> +#include <linux/i2c.h>
> +
> +#include <linux/init.h>
> +
> +#include <linux/kernel.h>
> +
> +#include <linux/module.h>
> +
> +#include <linux/mutex.h>
> +
> +#include <linux/of_device.h>
> +
> +#include <linux/pmbus.h>
> +
> +#include <linux/util_macros.h>
> +
> +#include "pmbus.h"
> +
> +enum chips {
> +
> +	Q54SJ108A2,
> +	Q50SN12050,
> +	Q50SN12072
> +};
> +
> +static int delta_read_word_data(struct i2c_client *client, int page, int phase, int reg)
> +{
> +	int ret = 0;
> +	u16 temp;
> +
> +	temp = pmbus_read_word_data(client, page, phase, reg);
> +
> +	switch (reg) {
> +	case PMBUS_STATUS_WORD:
> +		ret = temp;
> +		break;
> +	default:
> +		ret = -ENODATA;
> +		break;
> +	}
> +	return ret;

This doesn't even make sense. It returns -ENODATA for pretty much all sensors,
meaning the core will read the value again. Why read the value twice except for
PMBUS_STATUS_WORD ?

> +
> +}
> +
> +static int delta_write_word_data(struct i2c_client *client, int page, int reg, u16 word)
> +{
> +	u8 value;
> +
> +	switch (reg) {
> +	case PMBUS_OPERATION:

Pointless. This is a byte command anyway.

> +	case PMBUS_WRITE_PROTECT:
> +	case PMBUS_VOUT_OV_FAULT_RESPONSE:
> +	case PMBUS_IOUT_OC_FAULT_RESPONSE:

Those three are also pointless. Never called.

> +		value = (u8)word;
> +		return pmbus_write_byte_data(client, page, reg, value);
> +
> +	default:
> +		return -ENODATA;
> +	}
> +
> +}
> +
> +static int delta_read_byte_data(struct i2c_client *client, int page, int reg)
> +{
> +	int ret = 0;
> +	u16 temp;
> +
> +	temp = pmbus_read_byte_data(client, page, reg);
> +
> +	switch (reg) {
> +	case PMBUS_OPERATION:
> +	case PMBUS_WRITE_PROTECT:
> +	case PMBUS_VOUT_MODE:
> +	case PMBUS_VOUT_OV_FAULT_RESPONSE:
> +	case PMBUS_IOUT_OC_FAULT_RESPONSE:
> +	case PMBUS_STATUS_VOUT:
> +	case PMBUS_STATUS_IOUT:
> +	case PMBUS_STATUS_INPUT:
> +	case PMBUS_STATUS_TEMPERATURE:
> +	case PMBUS_STATUS_CML:
> +	case PMBUS_REVISION:
> +		ret = temp;
> +		break;
> +
> +	default:
> +		ret = -ENODATA;
> +		break;
> +	}
> +	return ret;
> +

What exactly is the point of this ? Return the value read for some commands,
-ENODATA for others, which means the PMBus core will read them again.

> +}
> +
> +static int delta_write_byte(struct i2c_client *client, int page, u8 value)
> +{
> +	switch (value) {
> +	case PMBUS_CLEAR_FAULTS:
> +		ret = pmbus_write_byte(client, page, PMBUS_CLEAR_FAULTS);
> +		break;
> +
> +	default:
> +		ret = -ENODATA;
> +		break;
> +	}

Pointless again. Write PMBUS_CLEAR_FAULTS here, the rest in the PMBus core.

What is this all about ? Sorry, I am completely lost. All those driver
specific functions are useless, the rest of the code has severe formatting
problems. Is this driver even needed, or would it be sufficient to add the
chip IDs to pmbus.c ? I see nothing in the datasheets that would suggest
otherwise.

Guenter

> +	return ret;
> +}
> +
> +static const struct pmbus_driver_info delta_info[] = {
> +
> +[Q54SJ108A2] = {
> +
> +.pages = 1,
> +
> +.read_word_data = delta_read_word_data,
> +
> +.write_word_data = delta_write_word_data,
> +
> +.read_byte_data = delta_read_byte_data,
> +
> +.write_byte = delta_write_byte,
> +
> +
> +/* Source : Delta Q54SJ108A2 */
> +
> +.format[PSC_TEMPERATURE] = linear,
> +
> +.format[PSC_VOLTAGE_IN] = linear,
> +
> +.format[PSC_CURRENT_OUT] = linear,
> +
> +
> +.func[0] = PMBUS_HAVE_VIN |
> +
> +PMBUS_HAVE_VOUT | PMBUS_HAVE_STATUS_VOUT |
> +
> +PMBUS_HAVE_IOUT | PMBUS_HAVE_STATUS_IOUT |
> +
> +PMBUS_HAVE_TEMP | PMBUS_HAVE_STATUS_TEMP |
> +
> +PMBUS_HAVE_STATUS_INPUT,
> +
> +},
> +
> +[Q50SN12050] = {
> +
> +.pages = 1,
> +
> +.read_word_data = delta_read_word_data,
> +
> +.write_word_data = delta_write_word_data,
> +
> +.read_byte_data = delta_read_byte_data,
> +
> +.write_byte = delta_write_byte,
> +
> +
> +/* Source : Delta Q50SN12050 */
> +
> +.format[PSC_TEMPERATURE] = linear,
> +
> +
> +.format[PSC_VOLTAGE_IN] = linear,
> +
> +
> +.format[PSC_CURRENT_OUT] = linear,
> +
> +
> +.func[0] = PMBUS_HAVE_VIN |
> +
> +PMBUS_HAVE_VOUT | PMBUS_HAVE_STATUS_VOUT |
> +
> +PMBUS_HAVE_IOUT | PMBUS_HAVE_STATUS_IOUT |
> +
> +PMBUS_HAVE_TEMP | PMBUS_HAVE_STATUS_TEMP |
> +
> +PMBUS_HAVE_STATUS_INPUT,
> +},
> +
> +[Q50SN12072] = {
> +
> +.pages = 1,
> +
> +.read_word_data = delta_read_word_data,
> +
> +.write_word_data = delta_write_word_data,
> +
> +.read_byte_data = delta_read_byte_data,
> +
> +.write_byte = delta_write_byte,
> +
> +
> +/* Source : Delta Q50SN12072 */
> +
> +.format[PSC_TEMPERATURE] = linear,
> +
> +
> +.format[PSC_VOLTAGE_IN] = linear,
> +
> +
> +.format[PSC_CURRENT_OUT] = linear,
> +
> +
> +.func[0] = PMBUS_HAVE_VIN |
> +
> +PMBUS_HAVE_VOUT | PMBUS_HAVE_STATUS_VOUT |
> +
> +PMBUS_HAVE_IOUT | PMBUS_HAVE_STATUS_IOUT |
> +
> +PMBUS_HAVE_TEMP | PMBUS_HAVE_STATUS_TEMP |
> +
> +PMBUS_HAVE_STATUS_INPUT,
> +},
> +
> +};
> +
> +
> +
> +static int delta_probe(struct i2c_client *client, const struct i2c_device_id *id)
> +{
> +
> +	struct device *dev = &client->dev;
> +
> +	u8 buf[I2C_SMBUS_BLOCK_MAX + 1];
> +
> +	struct pmbus_driver_info *info;
> +
> +	enum chips chip_id;
> +
> +	int ret;
> +
> +	if (!i2c_check_functionality(client->adapter,
> +
> +		I2C_FUNC_SMBUS_BYTE_DATA |
> +
> +		I2C_FUNC_SMBUS_WORD_DATA |
> +
> +		I2C_FUNC_SMBUS_BLOCK_DATA))
> +
> +		return -ENODEV;
> +
> +	if (client->dev.of_node)
> +
> +		chip_id = (enum chips)of_device_get_match_data(dev);
> +
> +	else
> +
> +		chip_id = id->driver_data;
> +
> +	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(dev, "Unsupported Manufacturer ID '%s'\n", buf);
> +
> +		return -ENODEV;
> +
> +	}
> +
> +/*
> + * The chips support reading PMBUS_MFR_MODEL.
> + */
> +
> +	ret = i2c_smbus_read_block_data(client, PMBUS_MFR_MODEL, buf);
> +
> +	if (ret < 0) {
> +
> +		dev_err(dev, "Failed to read Manufacturer Model\n");
> +
> +		return ret;
> +
> +	}
> +
> +	if (ret != 14 || strncmp(buf, "Q54SJ108A2", 10)) {
> +
> +		buf[ret] = '\0';
> +
> +		dev_err(dev, "Unsupported Manufacturer Model '%s'\n", buf);
> +
> +		return -ENODEV;
> +
> +	}
> +
> +	ret = i2c_smbus_read_block_data(client, PMBUS_MFR_REVISION, buf);
> +
> +	if (ret < 0) {
> +
> +		dev_err(dev, "Failed to read Manufacturer Revision\n");
> +
> +		return ret;
> +
> +	}
> +
> +	if (ret != 4 || buf[0] != 'S') {
> +
> +		buf[ret] = '\0';
> +
> +		dev_err(dev, "Unsupported Manufacturer Revision '%s'\n", buf);
> +
> +		return -ENODEV;
> +
> +	}
> +
> +	info = devm_kzalloc(dev, sizeof(*info), GFP_KERNEL);
> +
> +	if (!info)
> +
> +		return -ENOMEM;
> +
> +	memcpy(info, &delta_info[chip_id], sizeof(*info));
> +
> +	return pmbus_do_probe(client, id, info);
> +
> +}
> +
> +
> +
> +static const struct i2c_device_id delta_id[] = {
> +
> +{ "Q54SJ108A2", Q54SJ108A2 },
> +
> +{ "Q50SN12050", Q50SN12050 },
> +
> +{ "Q50SN12072", Q50SN12072 },
> +
> +{ },
> +
> +};
> +
> +
> +
> +MODULE_DEVICE_TABLE(i2c, delta_id);
> +
> +
> +
> +static const struct of_device_id delta_of_match[] = {
> +
> +{ .compatible = "delta,Q54SJ108A2", .data = (void *)Q54SJ108A2 },
> +
> +{ .compatible = "delta,Q50SN12050", .data = (void *)Q50SN12050 },
> +
> +{ .compatible = "delta,Q50SN12072", .data = (void *)Q50SN12072 },
> +
> +{ },
> +
> +};
> +
> +
> +
> +MODULE_DEVICE_TABLE(of, delta_of_match);
> +
> +
> +
> +static struct i2c_driver delta_driver = {
> +
> +.driver = {
> +
> +.name = "Q54SJ108A2",
> +
> +.of_match_table = delta_of_match,
> +
> +},
> +
> +.probe = delta_probe,
> +
> +.remove = pmbus_do_remove,
> +
> +.id_table = delta_id,
> +
> +};
> +
> +
> +
> +module_i2c_driver(delta_driver);
> +
> +
> +
> +MODULE_AUTHOR("Delta <xiao.mx.ma@xxxxxxxxxxx>");
> +
> +MODULE_DESCRIPTION("PMBus driver for Delta Q54SJ108A2 / Q50SN12050 / Q50SN12072");
> +
> +MODULE_LICENSE("GPL");
>