Message-ID: <SG2PR04MB5543FEAFC1777ADE29239AC9A1A0A@SG2PR04MB5543.apcprd04.prod.outlook.com>
Date:   Tue, 31 Oct 2023 06:46:59 +0000
From:   Delphine_CC_Chiu/WYHQ/Wiwynn <Delphine_CC_Chiu@...ynn.com>
To:     Guenter Roeck <linux@...ck-us.net>,
        Delphine_CC_Chiu/WYHQ/Wiwynn <Delphine_CC_Chiu@...ynn.com>,
        "patrick@...cx.xyz" <patrick@...cx.xyz>,
        Jean Delvare <jdelvare@...e.com>,
        Jonathan Corbet <corbet@....net>
CC:     Rob Herring <robh+dt@...nel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
        Conor Dooley <conor+dt@...nel.org>,
        "linux-i2c@...r.kernel.org" <linux-i2c@...r.kernel.org>,
        "linux-hwmon@...r.kernel.org" <linux-hwmon@...r.kernel.org>,
        "devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "linux-doc@...r.kernel.org" <linux-doc@...r.kernel.org>
Subject: RE: [PATCH v2 2/2] hwmon: pmbus: Add ltc4286 driver

> -----Original Message-----
> From: Guenter Roeck <groeck7@...il.com> On Behalf Of Guenter Roeck
> Sent: Friday, October 27, 2023 12:28 AM
> To: Delphine_CC_Chiu/WYHQ/Wiwynn <Delphine_CC_Chiu@...ynn.com>;
> patrick@...cx.xyz; Jean Delvare <jdelvare@...e.com>; Jonathan Corbet
> <corbet@....net>
> Cc: Rob Herring <robh+dt@...nel.org>; Krzysztof Kozlowski
> <krzysztof.kozlowski+dt@...aro.org>; Conor Dooley <conor+dt@...nel.org>;
> linux-i2c@...r.kernel.org; linux-hwmon@...r.kernel.org;
> devicetree@...r.kernel.org; linux-kernel@...r.kernel.org;
> linux-doc@...r.kernel.org
> Subject: Re: [PATCH v2 2/2] hwmon: pmbus: Add ltc4286 driver
> 
>   Security Reminder: Please be aware that this email is sent by an external
> sender.
> 
> On 10/26/23 01:15, Delphine CC Chiu wrote:
> > Add a driver to support ltc4286 chip
> >
> > Signed-off-by: Delphine CC Chiu <Delphine_CC_Chiu@...ynn.com>
> >
> > Changelog:
> >    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/ltc4286.rst |  79 ++++++++++++++++
> >   drivers/hwmon/pmbus/Kconfig     |   9 ++
> >   drivers/hwmon/pmbus/Makefile    |   1 +
> >   drivers/hwmon/pmbus/ltc4286.c   | 160
> ++++++++++++++++++++++++++++++++
> >   4 files changed, 249 insertions(+)
> >   create mode 100644 Documentation/hwmon/ltc4286.rst
> >   create mode 100644 drivers/hwmon/pmbus/ltc4286.c
> >
> > diff --git a/Documentation/hwmon/ltc4286.rst
> > b/Documentation/hwmon/ltc4286.rst new file mode 100644 index
> > 000000000000..9cae50b7478d
> > --- /dev/null
> > +++ b/Documentation/hwmon/ltc4286.rst
> > @@ -0,0 +1,79 @@
> > +.. SPDX-License-Identifier: GPL-2.0-or-later
> > +
> > +Kernel driver ltc4286
> > +=====================
> > +
> > +Supported chips:
> > +
> > +  * Analog Devices LTC4286
> > +
> > +    Prefix: 'ltc4286'
> > +
> > +    Addresses scanned: -
> > +
> > +    Datasheet:
> > + https://apc01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fww
> > +
> w.analog.com%2Fmedia%2Fen%2Ftechnical-documentation%2Fdata-sheets%2
> F
> > +
> ltc4286.pdf&data=05%7C01%7CWayne_SC_Liu%40wiwynn.com%7Cb749e252b
> fb84
> > +
> 24531ac08dbd640774e%7Cda6e0628fc834caf9dd273061cbab167%7C0%7C0%
> 7C638
> > +
> 339344747629221%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiL
> CJQIjoi
> > +
> V2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=7s
> HwFye
> > + so39ED13H3KaxosVOMJT1Kswhwj38arystWQ%3D&reserved=0
> > +
> > +  * Analog Devices LTC4287
> > +
> > +    Prefix: 'ltc4287'
> > +
> > +    Addresses scanned: -
> > +
> > +    Datasheet:
> > + https://apc01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fww
> > +
> w.analog.com%2Fmedia%2Fen%2Ftechnical-documentation%2Fdata-sheets%2
> F
> > +
> ltc4287.pdf&data=05%7C01%7CWayne_SC_Liu%40wiwynn.com%7Cb749e252b
> fb84
> > +
> 24531ac08dbd640774e%7Cda6e0628fc834caf9dd273061cbab167%7C0%7C0%
> 7C638
> > +
> 339344747629221%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiL
> CJQIjoi
> > +
> V2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=31
> 402MZ
> > + 9ONvkV8BsRRPxmTZNMU1yj1u%2F3NFVEpThFPI%3D&reserved=0
> > +
> > +Author: Delphine CC Chiu <Delphine_CC_Chiu@...ynn.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.
> > +
> > +=======================
> =======================================================
> > +inX_label               "vin1" or "vout1" depending on chip variant and
> > +                        configuration.
> 
> Is that a cut-and-paste ? I don't see it handled in the driver, and I don't
> immediately see it in the datasheet. From the datasheet, it seems to me that
> both are always present.
We will revise to the correct description

> 
> > +inX_input               Measured voltage.
> > +
> > +curr1_label             "iout1"
> > +curr1_input             Measured current.
> > +
> > +power1_label            "pin1"
> > +power1_input            Input power.
> > +
> > +temp1_input             Chip temperature.
> > +=======================
> > +=======================================================
> > diff --git a/drivers/hwmon/pmbus/Kconfig b/drivers/hwmon/pmbus/Kconfig
> > index b4e93bd5835e..f2b53e8abc3c 100644
> > --- a/drivers/hwmon/pmbus/Kconfig
> > +++ b/drivers/hwmon/pmbus/Kconfig
> > @@ -226,6 +226,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 "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"
> > 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..e1d72fe9587c
> > --- /dev/null
> > +++ b/drivers/hwmon/pmbus/ltc4286.c
> > @@ -0,0 +1,160 @@
> > +// 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
> > +
> > +enum chips { ltc4286, ltc4287 };
> > +
> > +/*
> > + * 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 mirco 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 mirco 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, };
> > +
> 
> The datasheet for LTC4286  (in the PMBus description talks about LSB 21.3
> mV/RSENSE
> for IOUT_OC_WARN_LIMIT and 2.8/RSENSE for PIN_OP_WARN_LIMIT. This
> contradicts
> data elsewhere in the datasheet which uses above coefficients for both
> LTC4286
> and LTC4287. I don't know if the datasheet is wrong, but this needs to be
> clarified.
> If the datasheet is wrong, that needs to be mentioned above. If the limit
> registers
> use different coefficients, local code will be needed to adjust values when
> reading /
> writing the registers to have it match coefficients.
We have sent an e-mail about this question.

> 
> > +static const struct i2c_device_id ltc4286_id[] = { { "ltc4286", ltc4286 },
> > +                                                { "ltc4287",
> ltc4287 },
> > +                                                {} };
> > +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) {
> > +             dev_err(&client->dev, "failed to read manufacturer id\n");
> 
> Why not use dev_err_probe() here ?
We will use dev_err_probe() instead of dev_err()

> 
> > +             return ret;
> > +     }
> > +
> > +     /*
> > +      * 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,
> > +                                  "failed to read manufacturer
> id\n");
> 
> This is misleading. It didn't _fail_ to read the manufacturer ID.
We will revise to "Manufacturer id mismatch"

> 
> > +     }
> > +
> > +     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");
> 
> Why not use dev_err_probe() here ?
We will use dev_err_probe() instead of dev_err()

> 
> > +             return ret;
> > +     }
> > +
> > +     for (mid = ltc4286_id; mid->name[0]; mid++) {
> > +             if (!strncasecmp(mid->name, block_buffer,
> strlen(mid->name)))
> > +                     break;
> > +     }
> 
> This is pointless code. If the ID is not found, mid will point after
> the end of the array, and then what ?
> 
> The purpose of such code is to validate if the chip is actually the one
> referenced in the match function and at least warn if that is not the case.
> It should never accept a chip which is _known_ to not be supported.
We will revise as below
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;
> > +
> > +     info = &ltc4286_info;
> > +
> > +     /* Default of VRANGE_SELECT = 1, 102.4V */
> > +     if (device_property_read_bool(&client->dev,
> "adi,vrange-select-25p6")) {
> 
> What if the adi,vrange-select-25p6 property is not provided, but the chip
> is programmed for this range ?
The binding document tells programmers how to fill the dts.
Thus, programmers must fill this property if their system is 25.6 volts voltage range.

> 
> > +             /* 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_BIT; /* VRANGE_SELECT = 0,
> 25.6V */
> > +             ret = i2c_smbus_write_word_data(client,
> LTC4286_MFR_CONFIG1,
> > +                                             ret);
> 
> This should only be written if it actually changed.
We will revise as below
if ((ret & VRANGE_SELECT_BIT) != VRANGE_25P6) {
	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");
}
Moreover, we will check the behavior about this register with vendor.

> 
> > +             if (ret < 0) {
> > +                     dev_err(&client->dev, "failed to set vrange\n");
> > +                     return ret;
> > +             }
> > +
> > +             info->m[PSC_VOLTAGE_IN] = 128;
> > +             info->m[PSC_VOLTAGE_OUT] = 128;
> > +             info->m[PSC_POWER] = 4 * rsense;
> 
> You can not overwrite ltc4286_info because there might be several chips
> in the system with different sense resistor values and range
> configurations.
We will add below to replace the line info = &ltc4286_info;
info = devm_kzalloc(&client->dev, sizeof(*info), GFP_KERNEL);
if (!info)
	return -ENOMEM;
memcpy(info, &ltc4286_info, sizeof(*info));

> 
> Also, the above (and the calculation in the code below) will overflow
> with too-large sense register values.
We will add below to check if the 4*rsense overflow or not.
if (info->m[PSC_POWER] > INT_MAX)
	return dev_err_probe(&client->dev, -ERANGE,
                      "Power coefficient overflow\n");

> 
> > +     } else
> > +             info->m[PSC_POWER] = rsense;
> 
> Please run checkpatch --strict on your patches.
We will run checkpatch --strict to check our patch.

> 
> > +
> > +     info->m[PSC_CURRENT_OUT] = 1024 * rsense;
> 
> Any rsense value larger than MAXINT / 1024 will overflow.
if (info->m[PSC_CURRENT_OUT] > INT_MAX)
	return dev_err_probe(&client->dev, -ERANGE,
                      "Current coefficient overflow\n");

> 
> > +
> > +     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
> <Delphine_CC_Chiu@...ynn.com>");
> > +MODULE_DESCRIPTION("PMBUS driver for LTC4286 and compatibles");
> > +MODULE_LICENSE("GPL");

Powered by blists - more mailing lists