> -----Original Message-----
> From: Guenter Roeck <[email protected]> On Behalf Of Guenter Roeck
> Sent: Friday, November 10, 2023 1:28 AM
> To: Delphine_CC_Chiu/WYHQ/Wiwynn <[email protected]>;
> [email protected]; Jean Delvare <[email protected]>; Jonathan Corbet
> <[email protected]>
> Cc: Rob Herring <[email protected]>; Krzysztof Kozlowski
> <[email protected]>; Conor Dooley <[email protected]>;
> [email protected]; [email protected];
> [email protected]; [email protected];
> [email protected]
> Subject: Re: [PATCH v4 2/2] hwmon: pmbus: Add ltc4286 driver
>
> Security Reminder: Please be aware that this email is sent by an external
> sender.
>
> On 11/8/23 17:49, Delphine CC Chiu wrote:
> > Add a driver to support ltc4286 chip
> >
> > Signed-off-by: Delphine CC Chiu <[email protected]>
> >
> > Changelog:
> > 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 | 177
> ++++++++++++++++++++++++++++++++
> > 5 files changed, 284 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 <[email protected]>
> > +
> > +
> > +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..e6690b38349a
> > --- /dev/null
> > +++ b/drivers/hwmon/pmbus/ltc4286.c
> > @@ -0,0 +1,177 @@
> > +// 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_25P6 0
> > +
> > +/*
> > + * 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 },
> > + {} };
>
> Please fix that formatting and use the more common style.
>
> static const struct i2c_device_id ltc4286_id[] = {
> { "ltc4286", 0 },
> { "ltc4287", 1 },
> {}
> };
We will revise to
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;
> > + int temp_setting;
> > + 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");
> > +
> > + ret = of_property_read_u32(client->dev.of_node,
> > + "shunt-resistor-micro-ohms",
> &rsense);
> > + if (ret < 0)
> > + return ret;
> > +
> > + if (rsense == 0)
> > + return -EINVAL;
> > +
> > + if (rsense > INT_MAX)
> > + return -ERANGE;
> > +
>
> This is not "Math result not representable". See below.
We will revise as below
/* Check for the latter MBR value won't overflow */
if (rsense > (INT_MAX / 1024))
return -EINVAL;
>
> > + info = devm_kmemdup(&client->dev, <c4286_info, sizeof(*info),
> > + GFP_KERNEL);
> > + if (!info)
> > + return -ENOMEM;
> > +
> > + /* Default of VRANGE_SELECT = 1, 102.4V */
> > + if (device_property_read_bool(&client->dev, "adi,vrange-low-enable"))
> {
> > + /* Setup MFR1 CONFIG register bit 1 VRANGE_SELECT */
> > + 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");
> > +
> > + ret &= ~VRANGE_SELECT_BIT; /* VRANGE_SELECT = 0,
> 25.6V */
> > + ret = i2c_smbus_write_word_data(client,
> LTC4286_MFR_CONFIG1,
> > + ret);
> > + if (ret < 0)
> > + return dev_err_probe(&client->dev, ret,
> > + "Failed to set
> vrange\n");
> > +
> > + info->m[PSC_VOLTAGE_IN] = 128;
> > + info->m[PSC_VOLTAGE_OUT] = 128;
> > +
> > + temp_setting = 4 * rsense;
> > + if (temp_setting > INT_MAX)
>
> This will still overflow for values of rsense larger than INT_MAX / 4.
> temp_setting is an int, and its value will never be > INT_MAX.
>
> > + return dev_err_probe(&client->dev, -ERANGE,
> > + "Power coefficient
> > + overflow\n");
>
> ERANGE is "Math result not representable", which is not the case here.
> This (and the above above) error needs to be -EINVAL.
>
>
> > + info->m[PSC_POWER] = temp_setting;
> > + } else {
> > + info->m[PSC_POWER] = rsense;
>
> I told you before, the default range needs to be set. The range may have been
> changed by the BIOS/ROMMON, or someone could have changed it manually
> with i2cset or some other application, or some other operating system was
> loaded earlier which did its own setting.
After consulting with our hardware engineer, we will set default rsense value as 300.
>
> I do understand by now that you don't want to do that, but I won't accept the
> driver without it, sorry.
>
> > + }
> > +
> > + temp_setting = 1024 * rsense;
> > + if (temp_setting > INT_MAX)
> > + return dev_err_probe(&client->dev, -ERANGE,
> > + "Current coefficient overflow\n");
>
> Same comments as above for both overflow detection and error code. This
> also means that rsense must _always_ be <= INT_MAX / 1024. I would suggest
> to check that once after reading rsense instead of having three different checks
> in different places.
>
> Also, having an error message here but not with the initial check against
> 0 and INT_MAX is inconsistent. I'd suggest no error message, but if you want to
> have error messages please make it consistent.
OK, we will check the rsense right after reading it.
>
> > + info->m[PSC_CURRENT_OUT] = temp_setting;
> > +
> > + return pmbus_do_probe(client, info); }
> > +
> > +static const struct of_device_id ltc4286_of_match[] = {
> > + { .compatible = "lltc,ltc4286" },
> > + { .compatible = "lltc,ltc4287" },
> > + {}
> > +};
> > +
> > +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
> <[email protected]>");
> > +MODULE_DESCRIPTION("PMBUS driver for LTC4286 and compatibles");
> > +MODULE_LICENSE("GPL");