Re: [PATCH v1 2/2] hwmon: pmbus: Add ltc4286 driver

[Guenter Roeck]

On Mon, Apr 24, 2023 at 06:13:50PM +0800, Delphine CC Chiu wrote:
> Add support for ltc4286 driver

The patch does not add support for a driver, it adds support for a chip.

> 
> Signed-off-by: Delphine CC Chiu <Delphine_CC_Chiu@xxxxxxxxxx>
> ---
>  drivers/hwmon/pmbus/Kconfig   |   9 +++
>  drivers/hwmon/pmbus/Makefile  |   1 +
>  drivers/hwmon/pmbus/ltc4286.c | 142 ++++++++++++++++++++++++++++++++++

Documentation is missing.

>  3 files changed, 152 insertions(+)
>  create mode 100644 drivers/hwmon/pmbus/ltc4286.c
> 
> diff --git a/drivers/hwmon/pmbus/Kconfig b/drivers/hwmon/pmbus/Kconfig
> index 59d9a7430499..1230d910d681 100644
> --- a/drivers/hwmon/pmbus/Kconfig
> +++ b/drivers/hwmon/pmbus/Kconfig
> @@ -218,6 +218,15 @@ config SENSORS_LTC3815
>  	  This driver can also be built as a module. If so, the module will
>  	  be called ltc3815.
>  
> +config SENSORS_LTC4286
> +	bool "Linear Technologies LTC4286"

Analog Devices ?

> +	help
> +	  If you say yes here you get hardware monitoring support for Linear
> +	  Technology LTC4286.
> +
> +	  This driver can also be built as a module. If so, the module will
> +	  be called ltc4286.
> +
>  config SENSORS_MAX15301
>  	tristate "Maxim MAX15301"
>  	help
> diff --git a/drivers/hwmon/pmbus/Makefile b/drivers/hwmon/pmbus/Makefile
> index 3ae019916267..540265539580 100644
> --- a/drivers/hwmon/pmbus/Makefile
> +++ b/drivers/hwmon/pmbus/Makefile
> @@ -23,6 +23,7 @@ obj-$(CONFIG_SENSORS_LM25066)	+= lm25066.o
>  obj-$(CONFIG_SENSORS_LT7182S)	+= lt7182s.o
>  obj-$(CONFIG_SENSORS_LTC2978)	+= ltc2978.o
>  obj-$(CONFIG_SENSORS_LTC3815)	+= ltc3815.o
> +obj-$(CONFIG_SENSORS_LTC4286)	+= ltc4286.o
>  obj-$(CONFIG_SENSORS_MAX15301)	+= max15301.o
>  obj-$(CONFIG_SENSORS_MAX16064)	+= max16064.o
>  obj-$(CONFIG_SENSORS_MAX16601)	+= max16601.o
> diff --git a/drivers/hwmon/pmbus/ltc4286.c b/drivers/hwmon/pmbus/ltc4286.c
> new file mode 100644
> index 000000000000..474f4ec9107c
> --- /dev/null
> +++ b/drivers/hwmon/pmbus/ltc4286.c
> @@ -0,0 +1,142 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * Hardware monitoring driver for LTC4286 Hot-Swap Controller
> + *
> + * Copyright (c) 2023 Linear Technology

Really ?

> + */
> +
> +#include <linux/err.h>
> +#include <linux/i2c.h>
> +#include <linux/init.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/pmbus.h>
> +#include "pmbus.h"
> +
> +// LTC4286 register

Please no C++ comments in the code.

> +#define LTC4286_MFR_CONFIG1 (0xF2)

Unnecessary ( )

> +
> +// LTC4286 configuration
> +#define VRANGE_SELECT (1 << 1)

#define<space>DEFINE<tab>value, please. Also, please use BIT() macros
where appropriate.
> +
> +#define LTC4286_MFR_ID_SIZE 3
> +
> +enum chips { ltc4286, ltc4287 };

There is no LTC4287 according to information available in public.
It has not even be announced.

Besides, the above enum is not really used anywhere and therefore
has zero value. Maybe the LTC4287 is not yet released. Even then,
there is no value listing it here because its parameters seem
to be identical to LTC4286.

> +
> +static struct pmbus_driver_info ltc4286_info = {
> +	.pages = 1,
> +	.format[PSC_VOLTAGE_IN] = direct,
> +	.format[PSC_VOLTAGE_OUT] = direct,
> +	.format[PSC_CURRENT_OUT] = direct,
> +	.format[PSC_POWER] = direct,
> +	.format[PSC_TEMPERATURE] = direct,
> +	.m[PSC_VOLTAGE_IN] = 32,
> +	.b[PSC_VOLTAGE_IN] = 0,
> +	.R[PSC_VOLTAGE_IN] = 1,
> +	.m[PSC_VOLTAGE_OUT] = 32,
> +	.b[PSC_VOLTAGE_OUT] = 0,
> +	.R[PSC_VOLTAGE_OUT] = 1,
> +	.m[PSC_CURRENT_OUT] = 1024,
> +	.b[PSC_CURRENT_OUT] = 0,
> +	.R[PSC_CURRENT_OUT] = 3 - 6,

This needs explanation.

> +	.m[PSC_POWER] = 1,
> +	.b[PSC_POWER] = 0,
> +	.R[PSC_POWER] = 4 - 6,

This needs explanation.

> +	.m[PSC_TEMPERATURE] = 1,
> +	.b[PSC_TEMPERATURE] = 273.15,

Assigning a float to an int doesn't make much sense.

> +	.R[PSC_TEMPERATURE] = 0,
> +	.func[0] = PMBUS_HAVE_VIN | PMBUS_HAVE_VOUT | PMBUS_HAVE_IOUT |
> +		   PMBUS_HAVE_PIN | PMBUS_HAVE_TEMP,

The chip does have a number of status registers.

> +};
> +
> +static int ltc4286_probe(struct i2c_client *client,
> +			 const struct i2c_device_id *id)
> +{
> +	int ret;
> +	u8 block_buffer[I2C_SMBUS_BLOCK_MAX + 1];
> +	struct device *dev = &client->dev;
> +	struct pmbus_driver_info *info;
> +	u32 rsense;
> +
> +	ret = i2c_smbus_read_block_data(client, PMBUS_MFR_ID, block_buffer);
> +	if (ret < 0) {
> +		dev_err(&client->dev, "failed to read manufacturer id\n");

Kind of pointless to declare a local 'dev' variable and not use it.

> +		return ret;
> +	}
> +
> +	/* Refer to ltc4286 datasheet page 20
> +	 * the default manufacturer id is LTC

Why "default" ?

> +	 */

Please use standard multi-line comments.

> +	if (ret != LTC4286_MFR_ID_SIZE ||
> +	    strncmp(block_buffer, "LTC", LTC4286_MFR_ID_SIZE)) {
> +		dev_err(&client->dev, "unsupported manufacturer id\n");
> +		return -ENODEV;
> +	}
> +
> +	ret = i2c_smbus_read_block_data(client, PMBUS_MFR_MODEL, block_buffer);
> +	if (ret < 0) {
> +		dev_err(&client->dev, "failed to read manufacturer model\n");
> +		return ret;
> +	}

Why read the model if you don't do anything with it ?

> +
> +	ret = of_property_read_u32(client->dev.of_node, "rsense-micro-ohms",
> +				   &rsense);
> +	if (ret < 0)
> +		return ret;
> +
> +	if (rsense == 0)
> +		return -EINVAL;
> +

There should be a default for systems not supporting devicetree.

> +	info = &ltc4286_info;
> +
> +	/* Default of VRANGE_SELECT = 1 */
> +	if (device_property_read_bool(dev, "vrange_select_25p6")) {
> +		/* Setup MFR1 CONFIG register bit 1 VRANGE_SELECT */
> +		ret = i2c_smbus_read_word_data(client, LTC4286_MFR_CONFIG1);
> +		if (ret < 0) {
> +			dev_err(&client->dev,
> +				"failed to read manufacturer configuration one\n");
> +			return ret;
> +		}
> +
> +		ret &= ~VRANGE_SELECT; /* VRANGE_SELECT = 0, 25.6V */
> +		i2c_smbus_write_word_data(client, LTC4286_MFR_CONFIG1, ret);

Error check missing.

> +
> +		info->m[PSC_VOLTAGE_IN] = 128;
> +		info->m[PSC_VOLTAGE_OUT] = 128;
> +		info->m[PSC_POWER] = 4 * rsense;
> +	} else {
> +		info->m[PSC_POWER] = rsense;

This just takes the current configuration, and assumes it is the default.
That may not be correct. The chip may have been configured by the BIOS,
or manually.

> +	}
> +

The code assumes that there is only a single chip in the system, or that
the configuration of all chips is identical. This is not necessarily
correct.

> +	info->m[PSC_CURRENT_OUT] = 1024 * rsense;
> +
> +	return pmbus_do_probe(client, info);
> +}
> +
> +static const struct i2c_device_id ltc4286_id[] = { { "ltc4286", ltc4286 },
> +						   { "ltc4287", ltc4287 },

Even if LTC4287 existed, assigning the ID here ...

> +						   {} };
> +MODULE_DEVICE_TABLE(i2c, ltc4286_id);
> +
> +static const struct of_device_id ltc4286_of_match[] = {
> +	{ .compatible = "lltc,ltc4286" },
> +	{ .compatible = "lltc,ltc4287" },

... but not here defeats having it in the first place.

> +	{}
> +};

MODULE_DEVOCE_TABLE() missing.

> +
> +/* This is the driver that will be inserted */

Useless comment

> +static struct i2c_driver ltc4286_driver = {
> +	.driver = {
> +		.name = "ltc4286",
> +		.of_match_table = ltc4286_of_match,
> +	},
> +	.probe = ltc4286_probe,
> +	.id_table = ltc4286_id,
> +};
> +
> +module_i2c_driver(ltc4286_driver);
> +
> +MODULE_AUTHOR("Delphine CC Chiu <Delphine_CC_Chiu@xxxxxxxxxx>");
> +MODULE_DESCRIPTION("PMBUS driver for LTC4286 and compatibles");
> +MODULE_LICENSE("GPL");
> -- 
> 2.17.1
>