On Mon, Jan 25, 2021 at 02:51:57PM -0800, Alex Qiu wrote:
> Tested: No obvious hwmon directory entry or value reading changes
> after applying the patch 2 on MAX16601. Had to work around the
> pmbus_do_probe() in order to apply to the 5.1 kernel on my hand
> though.
>
> Thx!
>
Thanks a lot for testing!
Guenter
> - Alex Qiu
>
> On Mon, Jan 25, 2021 at 10:53 AM Guenter Roeck <linux@xxxxxxxxxxxx> wrote:
> >
> > MAX16508 is quite similar to MAX16601, except that it does not support
> > the DEFAULT_NUM_POP register and we thus can not dynamically determine
> > the number of populated phases.
> >
> > Cc: Alex Qiu <xqiu@xxxxxxxxxx>
> > Cc: Ugur Usug <Ugur.Usug@xxxxxxxxxxxxxxxxxxx>
> > Signed-off-by: Guenter Roeck <linux@xxxxxxxxxxxx>
> Reviewed-by: Alex Qiu <xqiu@xxxxxxxxxx>
> Tested-by: Alex Qiu <xqiu@xxxxxxxxxx>
> > ---
> > Tested with MAX16601 to ensure that it doesn't break existing support,
> > but untested on MAX16508.
> >
> > The most likely cause of failure is the chip detection string: The
> > datasheet suggests that it is "MAX16500" for all chips in the MAX165xx
> > series, but without hardware that is all but impossible to confirm.
> >
> > It should also be confirmed that REG_DEFAULT_NUM_POP (the expected
> > number of populated phases) is indeed not supported on MAX16508.
> >
> > Documentation/hwmon/max16601.rst | 12 +++++-
> > drivers/hwmon/pmbus/Kconfig | 4 +-
> > drivers/hwmon/pmbus/max16601.c | 74 +++++++++++++++++++++++---------
> > 3 files changed, 66 insertions(+), 24 deletions(-)
> >
> > diff --git a/Documentation/hwmon/max16601.rst b/Documentation/hwmon/max16601.rst
> > index 93d25dfa028e..d16792be7533 100644
> > --- a/Documentation/hwmon/max16601.rst
> > +++ b/Documentation/hwmon/max16601.rst
> > @@ -5,6 +5,14 @@ Kernel driver max16601
> >
> > Supported chips:
> >
> > + * Maxim MAX16508
> > +
> > + Prefix: 'max16508'
> > +
> > + Addresses scanned: -
> > +
> > + Datasheet: Not published
> > +
> > * Maxim MAX16601
> >
> > Prefix: 'max16601'
> > @@ -19,8 +27,8 @@ Author: Guenter Roeck <linux@xxxxxxxxxxxx>
> > Description
> > -----------
> >
> > -This driver supports the MAX16601 VR13.HC Dual-Output Voltage Regulator
> > -Chipset.
> > +This driver supports the MAX16508 VR13 Dual-Output Voltage Regulator
> > +as well as the MAX16601 VR13.HC Dual-Output Voltage Regulator chipsets.
> >
> > The driver is a client driver to the core PMBus driver.
> > Please see Documentation/hwmon/pmbus.rst for details on PMBus client drivers.
> > diff --git a/drivers/hwmon/pmbus/Kconfig b/drivers/hwmon/pmbus/Kconfig
> > index 03606d4298a4..32d2fc850621 100644
> > --- a/drivers/hwmon/pmbus/Kconfig
> > +++ b/drivers/hwmon/pmbus/Kconfig
> > @@ -158,10 +158,10 @@ config SENSORS_MAX16064
> > be called max16064.
> >
> > config SENSORS_MAX16601
> > - tristate "Maxim MAX16601"
> > + tristate "Maxim MAX16508, MAX16601"
> > help
> > If you say yes here you get hardware monitoring support for Maxim
> > - MAX16601.
> > + MAX16508 and MAX16601.
> >
> > This driver can also be built as a module. If so, the module will
> > be called max16601.
> > diff --git a/drivers/hwmon/pmbus/max16601.c b/drivers/hwmon/pmbus/max16601.c
> > index efe6da3bc8d0..0d1204c2dd54 100644
> > --- a/drivers/hwmon/pmbus/max16601.c
> > +++ b/drivers/hwmon/pmbus/max16601.c
> > @@ -1,11 +1,11 @@
> > // SPDX-License-Identifier: GPL-2.0
> > /*
> > - * Hardware monitoring driver for Maxim MAX16601
> > + * Hardware monitoring driver for Maxim MAX16508 and MAX16601.
> > *
> > * Implementation notes:
> > *
> > - * Ths chip supports two rails, VCORE and VSA. Telemetry information for the
> > - * two rails is reported in two subsequent I2C addresses. The driver
> > + * This chip series supports two rails, VCORE and VSA. Telemetry information
> > + * for the two rails is reported in two subsequent I2C addresses. The driver
> > * instantiates a dummy I2C client at the second I2C address to report
> > * information for the VSA rail in a single instance of the driver.
> > * Telemetry for the VSA rail is reported to the PMBus core in PMBus page 2.
> > @@ -31,6 +31,8 @@
> >
> > #include "pmbus.h"
> >
> > +enum chips { max16508, max16601 };
> > +
> > #define REG_DEFAULT_NUM_POP 0xc4
> > #define REG_SETPT_DVID 0xd1
> > #define DAC_10MV_MODE BIT(4)
> > @@ -44,6 +46,7 @@
> > #define MAX16601_NUM_PHASES 8
> >
> > struct max16601_data {
> > + enum chips id;
> > struct pmbus_driver_info info;
> > struct i2c_client *vsa;
> > int iout_avg_pkg;
> > @@ -188,6 +191,7 @@ static int max16601_write_word(struct i2c_client *client, int page, int reg,
> > static int max16601_identify(struct i2c_client *client,
> > struct pmbus_driver_info *info)
> > {
> > + struct max16601_data *data = to_max16601_data(info);
> > int reg;
> >
> > reg = i2c_smbus_read_byte_data(client, REG_SETPT_DVID);
> > @@ -198,6 +202,9 @@ static int max16601_identify(struct i2c_client *client,
> > else
> > info->vrm_version[0] = vr12;
> >
> > + if (data->id != max16601)
> > + return 0;
> > +
> > reg = i2c_smbus_read_byte_data(client, REG_DEFAULT_NUM_POP);
> > if (reg < 0)
> > return reg;
> > @@ -254,28 +261,61 @@ static void max16601_remove(void *_data)
> > i2c_unregister_device(data->vsa);
> > }
> >
> > -static int max16601_probe(struct i2c_client *client)
> > +static const struct i2c_device_id max16601_id[] = {
> > + {"max16508", max16508},
> > + {"max16601", max16601},
> > + {}
> > +};
> > +MODULE_DEVICE_TABLE(i2c, max16601_id);
> > +
> > +static int max16601_get_id(struct i2c_client *client)
> > {
> > struct device *dev = &client->dev;
> > u8 buf[I2C_SMBUS_BLOCK_MAX + 1];
> > - struct max16601_data *data;
> > + enum chips id;
> > int ret;
> >
> > - if (!i2c_check_functionality(client->adapter,
> > - I2C_FUNC_SMBUS_READ_BYTE_DATA |
> > - I2C_FUNC_SMBUS_READ_BLOCK_DATA))
> > - return -ENODEV;
> > -
> > ret = i2c_smbus_read_block_data(client, PMBUS_IC_DEVICE_ID, buf);
> > - if (ret < 0)
> > + if (ret < 0 || ret < 11)
> > return -ENODEV;
> >
> > - /* PMBUS_IC_DEVICE_ID is expected to return "MAX16601y.xx" */
> > - if (ret < 11 || strncmp(buf, "MAX16601", 8)) {
> > + /*
> > + * PMBUS_IC_DEVICE_ID is expected to return "MAX16601y.xx"
> > + * or "MAX16500y.xx".
> > + */
> > + if (!strncmp(buf, "MAX16500", 8)) {
> > + id = max16508;
> > + } else if (!strncmp(buf, "MAX16601", 8)) {
> > + id = max16601;
> > + } else {
> > buf[ret] = '\0';
> > dev_err(dev, "Unsupported chip '%s'\n", buf);
> > return -ENODEV;
> > }
> > + return id;
> > +}
> > +
> > +static int max16601_probe(struct i2c_client *client)
> > +{
> > + struct device *dev = &client->dev;
> > + const struct i2c_device_id *id;
> > + struct max16601_data *data;
> > + int ret, chip_id;
> > +
> > + if (!i2c_check_functionality(client->adapter,
> > + I2C_FUNC_SMBUS_READ_BYTE_DATA |
> > + I2C_FUNC_SMBUS_READ_BLOCK_DATA))
> > + return -ENODEV;
> > +
> > + chip_id = max16601_get_id(client);
> > + if (chip_id < 0)
> > + return chip_id;
> > +
> > + id = i2c_match_id(max16601_id, client);
> > + if (chip_id != id->driver_data)
> > + dev_warn(&client->dev,
> > + "Device mismatch: Configured %s (%d), detected %d\n",
> > + id->name, (int) id->driver_data, chip_id);
> >
> > ret = i2c_smbus_read_byte_data(client, REG_PHASE_ID);
> > if (ret < 0)
> > @@ -290,6 +330,7 @@ static int max16601_probe(struct i2c_client *client)
> > if (!data)
> > return -ENOMEM;
> >
> > + data->id = chip_id;
> > data->iout_avg_pkg = 0xfc00;
> > data->vsa = i2c_new_dummy_device(client->adapter, client->addr + 1);
> > if (IS_ERR(data->vsa)) {
> > @@ -305,13 +346,6 @@ static int max16601_probe(struct i2c_client *client)
> > return pmbus_do_probe(client, &data->info);
> > }
> >
> > -static const struct i2c_device_id max16601_id[] = {
> > - {"max16601", 0},
> > - {}
> > -};
> > -
> > -MODULE_DEVICE_TABLE(i2c, max16601_id);
> > -
> > static struct i2c_driver max16601_driver = {
> > .driver = {
> > .name = "max16601",
> > --
> > 2.17.1
> >
