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

Guenter Roeck
2023-11-21T16:13:45Z

On 11/20/23 21:07, Delphine CC Chiu wrote:
> Add a driver to support ltc4286 chip
>
> Signed-off-by: Delphine CC Chiu <Delphine_CC_Chiu@Wiwynn.com>
> ---
> Changelog:
> v6 - Check VRANGE_SELECT before driver loading
> v5 - Check the overflow when reading rsense
> - Set default rsense value
> v4 - Add empty line before "config SENSORS_LTC4286" in Kconfig
> - Add ltc4286 to Documentation/hwmon/index.rst
> - Revise comment typo
> - Use devm_kmemdup instead of memcpy
> - Check MBR value before writting into
> v3 - Use dev_err_probe() instead of dev_err()
> - The VRANGE_SELECT bit only be written if it actually changed
> - Avoid the info pointer being overwritten
> - Check the MBR value range to avoid overflow
> - Revise ltc4286.rst to corrcet description
> v2 - Revise Linear Technologies LTC4286 to
> Analog Devices LTC4286 in Kconfig
> - Add more description for this driver in Kconfig
> - Add some comments for MBR setting in ltc4286.c
> - Add ltc4286.rst
> ---
> Documentation/hwmon/index.rst | 1 +
> Documentation/hwmon/ltc4286.rst | 95 +++++++++++++++++
> drivers/hwmon/pmbus/Kconfig | 10 ++
> drivers/hwmon/pmbus/Makefile | 1 +
> drivers/hwmon/pmbus/ltc4286.c | 183 ++++++++++++++++++++++++++++++++
> 5 files changed, 290 insertions(+)
> create mode 100644 Documentation/hwmon/ltc4286.rst
> create mode 100644 drivers/hwmon/pmbus/ltc4286.c
>
> diff --git a/Documentation/hwmon/index.rst b/Documentation/hwmon/index.rst
> index 72f4e6065bae..080827cc4c34 100644
> --- a/Documentation/hwmon/index.rst
> +++ b/Documentation/hwmon/index.rst
> @@ -128,6 +128,7 @@ Hardware Monitoring Kernel Drivers
> ltc4245
> ltc4260
> ltc4261
> + ltc4286
> max127
> max15301
> max16064
> diff --git a/Documentation/hwmon/ltc4286.rst b/Documentation/hwmon/ltc4286.rst
> new file mode 100644
> index 000000000000..2cd149676d86
> --- /dev/null
> +++ b/Documentation/hwmon/ltc4286.rst
> @@ -0,0 +1,95 @@
> +.. SPDX-License-Identifier: GPL-2.0-or-later
> +
> +Kernel driver ltc4286
> +=====================
> +
> +Supported chips:
> +
> + * Analog Devices LTC4286
> +
> + Prefix: 'ltc4286'
> +
> + Addresses scanned: -
> +
> + Datasheet: https://www.analog.com/media/en/technical-documentation/data-sheets/ltc4286.pdf
> +
> + * Analog Devices LTC4287
> +
> + Prefix: 'ltc4287'
> +
> + Addresses scanned: -
> +
> + Datasheet: https://www.analog.com/media/en/technical-documentation/data-sheets/ltc4287.pdf
> +
> +Author: Delphine CC Chiu <Delphine_CC_Chiu@Wiwynn.com>
> +
> +
> +Description
> +-----------
> +
> +This driver supports hardware monitoring for Analog Devices LTC4286
> +and LTC4287 Hot-Swap Controller and Digital Power Monitors.
> +
> +LTC4286 and LTC4287 are hot-swap controllers that allow a circuit board
> +to be removed from or inserted into a live backplane. They also feature
> +current and voltage readback via an integrated 12 bit analog-to-digital
> +converter (ADC), accessed using a PMBus interface.
> +
> +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.
> +
> +The shunt value in micro-ohms can be set via device tree at compile-time. Please
> +refer to the Documentation/devicetree/bindings/hwmon/lltc,ltc4286.yaml for bindings
> +if the device tree is used.
> +
> +
> +Platform data support
> +---------------------
> +
> +The driver supports standard PMBus driver platform data. Please see
> +Documentation/hwmon/pmbus.rst for details.
> +
> +
> +Sysfs entries
> +-------------
> +
> +The following attributes are supported. Limits are read-write, history reset
> +attributes are write-only, all other attributes are read-only.
> +
> +======================= =======================================================
> +in1_label "vin"
> +in1_input Measured voltage.
> +in1_alarm Input voltage alarm.
> +in1_min Minimum input voltage.
> +in1_max Maximum input voltage.
> +
> +in2_label "vout1"
> +in2_input Measured voltage.
> +in2_alarm Output voltage alarm.
> +in2_min Minimum output voltage.
> +in2_max Maximum output voltage.
> +
> +curr1_label "iout1"
> +curr1_input Measured current.
> +curr1_alarm Output current alarm.
> +curr1_max Maximum current.
> +
> +power1_label "pin"
> +power1_input Input power.
> +power1_alarm Input power alarm.
> +power1_max Maximum poewr.
> +
> +temp1_input Chip temperature.
> +temp1_min Minimum chip temperature.
> +temp1_max Maximum chip temperature.
> +temp1_crit Critical chip temperature.
> +temp1_alarm Chip temperature alarm.
> +======================= =======================================================
> diff --git a/drivers/hwmon/pmbus/Kconfig b/drivers/hwmon/pmbus/Kconfig
> index b4e93bd5835e..2d4f972e5a65 100644
> --- a/drivers/hwmon/pmbus/Kconfig
> +++ b/drivers/hwmon/pmbus/Kconfig
> @@ -227,6 +227,16 @@ config SENSORS_LTC3815
> This driver can also be built as a module. If so, the module will
> be called ltc3815.
>
> +config SENSORS_LTC4286
> + bool "Analog Devices LTC4286"
> + help
> + LTC4286 is an integrated solution for hot swap applications that
> + allows a board to be safely inserted and removed from a live
> + backplane.
> + This chip could be used to monitor voltage, current, ...etc.
> + If you say yes here you get hardware monitoring support for Analog
> + Devices LTC4286.
> +
> config SENSORS_MAX15301
> tristate "Maxim MAX15301"
> help
> diff --git a/drivers/hwmon/pmbus/Makefile b/drivers/hwmon/pmbus/Makefile
> index 84ee960a6c2d..94e28f6d6a61 100644
> --- a/drivers/hwmon/pmbus/Makefile
> +++ b/drivers/hwmon/pmbus/Makefile
> @@ -24,6 +24,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..e43e05579be1
> --- /dev/null
> +++ b/drivers/hwmon/pmbus/ltc4286.c
> @@ -0,0 +1,183 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +
> +#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 */
> +#define LTC4286_MFR_CONFIG1 0xF2
> +
> +/* LTC4286 configuration */
> +#define VRANGE_SELECT_BIT BIT(1)
> +
> +#define LTC4286_MFR_ID_SIZE 3
> +#define VRANGE_102P4 1
> +
> +/*
> + * Initialize the MBR as default settings which is referred to LTC4286 datasheet
> + * (March 22, 2022 version) table 3 page 16
> + */
> +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,
> + /*
> + * The rsense value used in MBR formula in LTC4286 datasheet should be ohm unit.
> + * However, the rsense value that user input is micro ohm.
> + * Thus, the MBR setting which involves rsense should be shifted by 6 digits.
> + */
> + .R[PSC_CURRENT_OUT] = 3 - 6,
> + .m[PSC_POWER] = 1,
> + .b[PSC_POWER] = 0,
> + /*
> + * The rsense value used in MBR formula in LTC4286 datasheet should be ohm unit.
> + * However, the rsense value that user input is micro ohm.
> + * Thus, the MBR setting which involves rsense should be shifted by 6 digits.
> + */
> + .R[PSC_POWER] = 4 - 6,
> + .m[PSC_TEMPERATURE] = 1,
> + .b[PSC_TEMPERATURE] = 273,
> + .R[PSC_TEMPERATURE] = 0,
> + .func[0] = PMBUS_HAVE_VIN | PMBUS_HAVE_VOUT | PMBUS_HAVE_IOUT |
> + PMBUS_HAVE_PIN | PMBUS_HAVE_TEMP | PMBUS_HAVE_STATUS_VOUT |
> + PMBUS_HAVE_STATUS_IOUT | PMBUS_HAVE_STATUS_TEMP,
> +};
> +
> +static const struct i2c_device_id ltc4286_id[] = {
> + { "ltc4286", 0 },
> + { "ltc4287", 1 },
> + {}
> +};
> +MODULE_DEVICE_TABLE(i2c, ltc4286_id);
> +
> +static int ltc4286_probe(struct i2c_client *client)
> +{
> + int ret;
> + const struct i2c_device_id *mid;
> + u8 block_buffer[I2C_SMBUS_BLOCK_MAX + 1];
> + struct pmbus_driver_info *info;
> + u32 rsense;
> +
> + ret = i2c_smbus_read_block_data(client, PMBUS_MFR_ID, block_buffer);
> + if (ret < 0) {
> + return dev_err_probe(&client->dev, ret,
> + "Failed to read manufacturer id\n");
> + }
> +
> + /*
> + * Refer to ltc4286 datasheet page 20
> + * the manufacturer id is LTC
> + */
> + if (ret != LTC4286_MFR_ID_SIZE ||
> + strncmp(block_buffer, "LTC", LTC4286_MFR_ID_SIZE)) {
> + return dev_err_probe(&client->dev, ret,
> + "Manufacturer id mismatch\n");
> + }
> +
> + ret = i2c_smbus_read_block_data(client, PMBUS_MFR_MODEL, block_buffer);
> + if (ret < 0) {
> + return dev_err_probe(&client->dev, ret,
> + "Failed to read manufacturer model\n");
> + }
> +
> + for (mid = ltc4286_id; mid->name[0]; mid++) {
> + if (!strncasecmp(mid->name, block_buffer, strlen(mid->name)))
> + break;
> + }
> + if (!mid->name[0])
> + return dev_err_probe(&client->dev, -ENODEV,
> + "Unsupported device\n");
> +
> + if (of_property_read_u32(client->dev.of_node,
> + "shunt-resistor-micro-ohms", &rsense))
> + rsense = 300; /* 0.3 mOhm if not set via DT */
> +
> + if (rsense == 0)
> + return -EINVAL;
> +
> + /* Check for the latter MBR value won't overflow */
> + if (rsense > (INT_MAX / 1024))
> + return -EINVAL;
> +
> + info = devm_kmemdup(&client->dev, &ltc4286_info, sizeof(*info),
> + GFP_KERNEL);
> + if (!info)
> + return -ENOMEM;
> +
> + /* Check MFR1 CONFIG register bit 1 VRANGE_SELECT before driver loading */
> + ret = i2c_smbus_read_word_data(client, LTC4286_MFR_CONFIG1);
> + if (ret < 0)
> + return dev_err_probe(
> + &client->dev, ret,
> + "Failed to read manufacturer configuration one\n");
> +
> + if (device_property_read_bool(&client->dev, "adi,vrange-low-enable")) {
> + /* The voltage range is 102.4 volts now */
> + if (ret == VRANGE_102P4) {

This has to check VRANGE_SELECT_BIT, not the entire register.
Other register bit values are unknown.

if (ret & VRANGE_SELECT_BIT)

Alternatively, something like
int new, orig;
...
orig = i2c_smbus_read_word_data(client, LTC4286_MFR_CONFIG1);
if (orig < 0)
return dev_err_probe(...);
new = orig;
if (device_property_read_bool(&client->dev, "adi,vrange-low-enable")) {
new &= ~VRANGE_SELECT_BIT;
info->m[PSC_VOLTAGE_IN] = 128;
info->m[PSC_VOLTAGE_OUT] = 128;
info->m[PSC_POWER] = 4 * rsense;
} else {
new |= VRANGE_SELECT_BIT;
info->m[PSC_POWER] = rsense;
}
if (new != orig) {
ret = i2c_smbus_write_word_data(client, LTC4286_MFR_CONFIG1, new);
if (ret)
return dev_err_probe(...);
}

seems cleaner to me, but that is your call.

Guenter