Message-ID: <SG2PR04MB55433BDC32F46C5874366C7EA114A@SG2PR04MB5543.apcprd04.prod.outlook.com>
Date:   Tue, 15 Aug 2023 09:08:55 +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>
CC:     "patrick@...cx.xyz" <patrick@...cx.xyz>,
        Jean Delvare <jdelvare@...e.com>,
        Rob Herring <robh+dt@...nel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.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>
Subject: RE: [PATCH v1 2/2] hwmon: pmbus: Add ltc4286 driver

> -----Original Message-----
> From: Guenter Roeck <groeck7@...il.com> On Behalf Of Guenter Roeck
> Sent: Monday, July 24, 2023 2:57 PM
> To: Delphine_CC_Chiu/WYHQ/Wiwynn <Delphine_CC_Chiu@...ynn.com>
> Cc: patrick@...cx.xyz; Jean Delvare <jdelvare@...e.com>; Rob Herring
> <robh+dt@...nel.org>; Krzysztof Kozlowski
> <krzysztof.kozlowski+dt@...aro.org>; linux-i2c@...r.kernel.org;
> linux-hwmon@...r.kernel.org; devicetree@...r.kernel.org;
> linux-kernel@...r.kernel.org
> Subject: Re: [PATCH v1 2/2] hwmon: pmbus: Add ltc4286 driver
> 
>   Security Reminder: Please be aware that this email is sent by an external
> sender.
> 
> On 7/23/23 23:03, Delphine_CC_Chiu/WYHQ/Wiwynn wrote:
> >> -----Original Message-----
> >> From: Guenter Roeck <groeck7@...il.com> On Behalf Of Guenter Roeck
> >> Sent: Monday, April 24, 2023 10:14 PM
> >> To: Delphine_CC_Chiu/WYHQ/Wiwynn <Delphine_CC_Chiu@...ynn.com>
> >> Cc: patrick@...cx.xyz; Jean Delvare <jdelvare@...e.com>; Rob Herring
> >> <robh+dt@...nel.org>; Krzysztof Kozlowski
> >> <krzysztof.kozlowski+dt@...aro.org>; linux-i2c@...r.kernel.org;
> >> linux-hwmon@...r.kernel.org; devicetree@...r.kernel.org;
> >> linux-kernel@...r.kernel.org
> >> Subject: Re: [PATCH v1 2/2] hwmon: pmbus: Add ltc4286 driver
> >>
> >>    Security Reminder: Please be aware that this email is sent by an
> >> external sender.
> >>
> >> 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.
> > Add a driver to support ltc4286 chip
> >
> >>
> >>>
> >>> Signed-off-by: Delphine CC Chiu <Delphine_CC_Chiu@...ynn.com>
> >>> ---
> >>>   drivers/hwmon/pmbus/Kconfig   |   9 +++
> >>>   drivers/hwmon/pmbus/Makefile  |   1 +
> >>>   drivers/hwmon/pmbus/ltc4286.c | 142
> >>> ++++++++++++++++++++++++++++++++++
> >>
> >> Documentation is missing.
> > Documentation is in patch one ([PATCH v1 1/2] dt-bindings: hwmon: Add
> > lltc ltc4286 driver bindings)
> >
> 
> Documentation/hwmon/ltc4286.rst is expected in this patch and not as part of
> the bindings. The bindings do not document the driver, they document the chip
> bindings.

We will add ltc4286.rst

> 
> >>
> >>>   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 ?
> > We will revise to "Analog Devices LTC4286"
> >
> >>
> >>> +     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 ?
> > We will remove the copyright declaration as it was false reference
> > from other drivers Chip vendor doesn't support this driver, so we implement
> it for our own use and contribute it as Wiwynn is a system manufacturer, our
> server products use this chip.
> >
> >>
> >>> + */
> >>> +
> >>> +#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.
> > We will revise to /* LTC4286 register */
> >
> >>
> >>> +#define LTC4286_MFR_CONFIG1 (0xF2)
> >>
> >> Unnecessary ( )
> > We will revise to #define LTC4286_MFR_CONFIG1 0xF2
> >
> >>
> >>> +
> >>> +// LTC4286 configuration
> >>> +#define VRANGE_SELECT (1 << 1)
> >>
> >> #define<space>DEFINE<tab>value, please. Also, please use BIT() macros
> >> where appropriate.
> > We will revise to below.
> > #define VRANGE_SELECT_BIT     BIT(1)
> >
> >>> +
> >>> +#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.
> > It has been announced on Analog Devices website.
> 
> No, it has not.

We have sent e-mail to query about the release date for LTC4287 chip.
Analog Device reply that they will release this chip in last week of Sep, 2023.
Please refer to the attachment to review their reply.

> 
> > Please refer to this link:
> > https://apc01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fwww.
> >
> analog.com%2Fen%2Fproducts%2Fltc2487.html%23product-overview&data=05
> %7
> >
> C01%7CWayne_SC_Liu%40wiwynn.com%7Ca1151d861c5545c3407508db8c13
> 2746%7Cd
> >
> a6e0628fc834caf9dd273061cbab167%7C0%7C0%7C638257786161934465%7C
> Unknown
> >
> %7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haW
> wiLCJ
> >
> XVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=MuIiftfChWZwCRBtt%2B1%2BoPj
> ZGr1mE2Zt
> > fqqztvhjz6I%3D&reserved=0
> > enum chips { ltc4286 = 0, ltc4287 };
> > Use in v1 line 118 to list chip index instead of hardcoding
> >
> 
> ltc2487 is _not_ ltc4287.
>     ^^               ^^
> 
> Sure, a Google search for LTC4287 returns a link to LTC2487 (!) as first response,
> but LTC2487 is an ADC and has nothing but the manufacturer in common with
> LTC4286.
> 
> LTC4286 High Power Positive Hot-Swap Controller with Power Monitor via
> PMBus
> LTC2487 16-Bit 2-/4-Channel ∆Σ ADC with PGA, Easy Drive and I2C Interface
> 
> I _really_ do not understand what you are trying to do here or why.

We have sent e-mail to query about the release date for LTC4287 chip.
Analog Device reply that they will release this chip in last week of Sep, 2023.
Please refer to the attachment to review their reply.

> 
> >>
> >>> +
> >>> +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.
> > We will add comments as below
> > /* Initialize the MBR as default settings which is referred to LTC4286
> > datasheet(March 22, 2022 version) table 3 page 16 */
> >
> 
> That does not explain "3 - 6".

We will add explanation like below
/*
 * 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.
 */

> 
> >>
> >>> +     .m[PSC_POWER] = 1,
> >>> +     .b[PSC_POWER] = 0,
> >>> +     .R[PSC_POWER] = 4 - 6,
> >>
> >> This needs explanation.
> > We will add comments as below
> > /* To support small shunt resistor value */
> >
> 
> That does not explain "4 - 6".

We will add explanation like below
/*
 * 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.
 */

> 
> 
> >>
> >>> +     .m[PSC_TEMPERATURE] = 1,
> >>> +     .b[PSC_TEMPERATURE] = 273.15,
> >>
> >> Assigning a float to an int doesn't make much sense.
> > We will revise to .b[PSC_TEMPERATURE] = 273, However the value for
> > this MBR is 273.15 in datasheet We use 273 due to pmbus code
> > limitation
> >
> >>
> >>> +     .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.
> > We will add status registers here
> >
> >>
> >>> +};
> >>> +
> >>> +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.
> > We will drop it
> >
> >>
> >>> +             return ret;
> >>> +     }
> >>> +
> >>> +     /* Refer to ltc4286 datasheet page 20
> >>> +      * the default manufacturer id is LTC
> >>
> >> Why "default" ?
> > We will revise to
> > /*
> >   * Refer to ltc4286 datasheet page 20
> >   * the manufacturer id is LTC
> >   */
> >
> >>
> >>> +      */
> >>
> >> Please use standard multi-line comments.
> > We will revise to
> > /*
> >   * 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)) {
> >>> +             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 ?
> > We will add comaprision here.
> > for (mid = ltc4286_id; mid->name[0]; mid++) {
> >       if (!strncasecmp(mid->name, block_buffer, strlen(mid->name)))
> >               break;
> > }
> >
> >>
> >>> +
> >>> +     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.
> > After confirming with vendor, they said there is no default rsesne for
> > this chip The value for rsense depends on hardware engineer in each
> > manufacturer
> >
> 
> The _driver_ needs a default.

Here we provide two options
1. Do not set default rsense
As mentioned before, vendor said there is no default rsesne for this chip.
The value for rsense depends on hardware engineer in each manufacturer.
Thus, if user forgets to input rsesne, it will probe fail because user doesn't get the correct rsense value from hardware engineer.
Moreover, if user uses the default rsense, the sensor reading would be weird in his or her systems.
2. Set rsense as 300 micro ohm
We have queried our hardware engineer and they said that the min and max value are 500 micro ohm and 250 micro ohm in their experience.
Which one do you prefer?

> 
> >>
> >>> +     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.
> > ret = i2c_smbus_write_word_data(client, LTC4286_MFR_CONFIG1, ret); 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;
> >>> +     } 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 MBR values are based on hardware design, so it must be set in
> > initial stage
> >
> >>
> >>> +     }
> >>> +
> >>
> >> 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.
> > If there are more than one LTC4286 or LTC4287 chips, user can add the
> > configuration for different chips in dts file
> >
> 
> "info" is a pointer to a static variable.
> 
>         info = &ltc4286_info;
> 
> In this context, what you are saying above does not make sense. The second
> chip's configuration will override the first chip's configuration.
> 
>         info->m[PSC_POWER] = 4 * rsense;
>         ...
> 
> 
> No, sorry, this still does not make sense.

In our opinion, if there are multiple chips in one system, the LTC4286 driver would probe many times and kernel allocate these chips for different memory space.
Thus, the configuration wonn't be overrided

> 
> 
> >>
> >>> +     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 ...
> > So do you recommend us to put this enum (enum chips { ltc4286, ltc4287 };)
> 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.
> > So do you recommend us to not put this enum (enum chips { ltc4286,
> ltc4287 };) here?
> >
> 
> Please read my comment. I am saying that it does not make sense to provide
> the chip ID to i2c_device_id but not in of_device_id. Either add it do both if
> needed or not at all. I am not telling which one, I am just asking for
> consistency.
> 
> However, as mentioned earlier, according to your code the chips supposedly
> have identical functionality. Separate chip IDs (ltc4286 and ltc4287) therefore
> seem unnecessary (not even counting the fact that ltc4287 still does not seem
> to exist).

We will revise to below and remove the enum
static const struct i2c_device_id ltc4286_id[] = { { "ltc4286", 0 }, {} };

static const struct of_device_id ltc4286_of_match[] = { { .compatible = "lltc,ltc4286" }, {} };

> 
> 
> >>
> >>> +     {}
> >>> +};
> >>
> >> MODULE_DEVOCE_TABLE() missing.
> > In v1 line 120
> >
> >>
> >>> +
> >>> +/* This is the driver that will be inserted */
> >>
> >> Useless comment
> > We will drop it.
> >
> >>
> >>> +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");
> >>> --
> >>> 2.17.1
> >>>


Content of type "message/rfc822" skipped

Powered by blists - more mailing lists