> -----Original Message-----
> From: Guenter Roeck <groeck7@xxxxxxxxx> On Behalf Of Guenter Roeck
> Sent: Friday, April 4, 2025 12:33 AM
> To: Torreno, Alexis Czezar <AlexisCzezar.Torreno@xxxxxxxxxx>
> Cc: Jean Delvare <jdelvare@xxxxxxxx>; Jonathan Corbet <corbet@xxxxxxx>;
> Delphine CC Chiu <Delphine_CC_Chiu@xxxxxxxxxx>; linux-
> hwmon@xxxxxxxxxxxxxxx; linux-doc@xxxxxxxxxxxxxxx; linux-
> kernel@xxxxxxxxxxxxxxx; linux-i2c@xxxxxxxxxxxxxxx
> Subject: Re: [PATCH v2 1/2] hwmon: (pmbus/max34440): Fix support for
> max34451
>
> [External]
>
> On Thu, Apr 03, 2025 at 01:16:18PM +0800, Alexis Czezar Torreno wrote:
> > The max344** family has an issue with some PMBUS address being switched.
> > This includes max34451 however version MAX34451-NA6 and later has this
> > issue fixed and this commit supports that update.
> >
> > Signed-off-by: Alexis Czezar Torreno <alexisczezar.torreno@xxxxxxxxxx>
> > ---
> > drivers/hwmon/pmbus/max34440.c | 55
> > +++++++++++++++++++++++++++++++++++++++---
> > 1 file changed, 51 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/hwmon/pmbus/max34440.c
> > b/drivers/hwmon/pmbus/max34440.c index
> >
> c9dda33831ff24e7b5e2fd1956a65e6bd2bfcbb9..585746806663409bc970426
> 47f6c
> > 0aba4c6f520a 100644
> > --- a/drivers/hwmon/pmbus/max34440.c
> > +++ b/drivers/hwmon/pmbus/max34440.c
> > @@ -34,16 +34,22 @@ enum chips { max34440, max34441, max34446,
> > max34451, max34460, max34461 };
> > /*
> > * The whole max344* family have IOUT_OC_WARN_LIMIT and
> IOUT_OC_FAULT_LIMIT
> > * swapped from the standard pmbus spec addresses.
> > + * For max34451, version MAX34451ETNA6+ and later has this issue fixed.
> > */
> > #define MAX34440_IOUT_OC_WARN_LIMIT 0x46
> > #define MAX34440_IOUT_OC_FAULT_LIMIT 0x4A
> >
> > +#define MAX34451ETNA6_MFR_REV 0x0012
> > +
> > #define MAX34451_MFR_CHANNEL_CONFIG 0xe4
> > #define MAX34451_MFR_CHANNEL_CONFIG_SEL_MASK 0x3f
> >
> > struct max34440_data {
> > int id;
> > struct pmbus_driver_info info;
> > + bool pmbus_addr_fixed;
>
> Unnecessary. See below.
>
> > + u32 iout_oc_warn_limit;
> > + u32 iout_oc_fault_limit;
>
> u8 would be sufficient.
Will change
>
> > };
> >
> > #define to_max34440_data(x) container_of(x, struct max34440_data,
> > info) @@ -60,11 +66,11 @@ static int max34440_read_word_data(struct
> i2c_client *client, int page,
> > switch (reg) {
> > case PMBUS_IOUT_OC_FAULT_LIMIT:
> > ret = pmbus_read_word_data(client, page, phase,
> > -
> MAX34440_IOUT_OC_FAULT_LIMIT);
> > + data->iout_oc_fault_limit);
> > break;
> > case PMBUS_IOUT_OC_WARN_LIMIT:
> > ret = pmbus_read_word_data(client, page, phase,
> > -
> MAX34440_IOUT_OC_WARN_LIMIT);
> > + data->iout_oc_warn_limit);
> > break;
> > case PMBUS_VIRT_READ_VOUT_MIN:
> > ret = pmbus_read_word_data(client, page, phase, @@ -133,11
> +139,11
> > @@ static int max34440_write_word_data(struct i2c_client *client, int
> > page,
> >
> > switch (reg) {
> > case PMBUS_IOUT_OC_FAULT_LIMIT:
> > - ret = pmbus_write_word_data(client, page,
> MAX34440_IOUT_OC_FAULT_LIMIT,
> > + ret = pmbus_write_word_data(client, page,
> > +data->iout_oc_fault_limit,
> > word);
> > break;
> > case PMBUS_IOUT_OC_WARN_LIMIT:
> > - ret = pmbus_write_word_data(client, page,
> MAX34440_IOUT_OC_WARN_LIMIT,
> > + ret = pmbus_write_word_data(client, page, data-
> >iout_oc_warn_limit,
> > word);
> > break;
> > case PMBUS_VIRT_RESET_POUT_HISTORY:
> > @@ -235,6 +241,24 @@ static int max34451_set_supported_funcs(struct
> i2c_client *client,
> > */
> >
> > int page, rv;
> > + bool max34451_na6 = false;
> > +
> > + rv = i2c_smbus_read_word_data(client, PMBUS_MFR_REVISION);
> > + if (rv < 0)
> > + return rv;
> > +
> > + if (rv == MAX34451ETNA6_MFR_REV) {
>
> Sure that this is only one revision ?
> Would it be better to use ">=" instead of "==" ?
Currently yes this is the only revision and the latest
It is nice to future proof this just in case so will change to >=
>
> > + max34451_na6 = true;
> > + data->pmbus_addr_fixed = true;
> > + data->info.format[PSC_VOLTAGE_IN] = direct;
> > + data->info.format[PSC_CURRENT_IN] = direct;
> > + data->info.m[PSC_VOLTAGE_IN] = 1;
> > + data->info.b[PSC_VOLTAGE_IN] = 0;
> > + data->info.R[PSC_VOLTAGE_IN] = 3;
> > + data->info.m[PSC_CURRENT_IN] = 1;
> > + data->info.b[PSC_CURRENT_IN] = 0;
> > + data->info.R[PSC_CURRENT_IN] = 2;
>
> Assign register addresses directly here.
Ah I see, will move.
Thanks!
>
> data->iout_oc_fault_limit = PMBUS_IOUT_OC_FAULT_LIMIT;
> data->iout_oc_warn_limit = PMBUS_IOUT_OC_WARN_LIMIT;
> } else {
> data->iout_oc_fault_limit =
> MAX34440_IOUT_OC_FAULT_LIMIT;
> data->iout_oc_warn_limit =
> MAX34440_IOUT_OC_WARN_LIMIT;
>
> > + }
>
> Thanks,
> Guenter
