* [PATCH 0/2] hwmon: (pmbus/bpa-rs600) cleanup and workaround
@ 2021-08-11 4:17 Chris Packham
2021-08-11 4:17 ` [PATCH 1/2] hwmon: (pmbus/bpa-rs600) Remove duplicate defininitions Chris Packham
` (2 more replies)
0 siblings, 3 replies; 12+ messages in thread
From: Chris Packham @ 2021-08-11 4:17 UTC (permalink / raw)
To: linux, jdelvare; +Cc: linux-hwmon, linux-kernel, Chris Packham
This series builds on top of the BPD-RS600 support[1] which is in Guenter's
tree by hasn't made it to Linus' yet. They might actually cleanly apply without
it since they touch different parts of the file.
[1] - https://lore.kernel.org/linux-hwmon/20210708220618.23576-1-chris.packham@alliedtelesis.co.nz/
Chris Packham (2):
hwmon: (pmbus/bpa-rs600) Remove duplicate defininitions
hwmon: (pmbus/bpa-rs600) Add workaround for incorrect Pin max
drivers/hwmon/pmbus/bpa-rs600.c | 44 ++++++++++++++++++++-------------
1 file changed, 27 insertions(+), 17 deletions(-)
--
2.32.0
^ permalink raw reply [flat|nested] 12+ messages in thread* [PATCH 1/2] hwmon: (pmbus/bpa-rs600) Remove duplicate defininitions 2021-08-11 4:17 [PATCH 0/2] hwmon: (pmbus/bpa-rs600) cleanup and workaround Chris Packham @ 2021-08-11 4:17 ` Chris Packham 2021-08-11 19:53 ` Guenter Roeck 2021-08-11 4:17 ` [PATCH 2/2] hwmon: (pmbus/bpa-rs600) Add workaround for incorrect Pin max Chris Packham 2021-08-12 3:15 ` [PATCH 0/2] hwmon: (pmbus/bpa-rs600) cleanup and workaround Chris Packham 2 siblings, 1 reply; 12+ messages in thread From: Chris Packham @ 2021-08-11 4:17 UTC (permalink / raw) To: linux, jdelvare; +Cc: linux-hwmon, linux-kernel, Chris Packham Commit 787c095edaa9 ("hwmon: (pmbus/core) Add support for rated attributes") added definitions for MFR_VIN_MIN etc so we can remove the local definitions of these from the bpa-rs600 driver. Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz> --- drivers/hwmon/pmbus/bpa-rs600.c | 25 ++++++++----------------- 1 file changed, 8 insertions(+), 17 deletions(-) diff --git a/drivers/hwmon/pmbus/bpa-rs600.c b/drivers/hwmon/pmbus/bpa-rs600.c index d205b41540ce..d495faa89799 100644 --- a/drivers/hwmon/pmbus/bpa-rs600.c +++ b/drivers/hwmon/pmbus/bpa-rs600.c @@ -12,15 +12,6 @@ #include <linux/pmbus.h> #include "pmbus.h" -#define BPARS600_MFR_VIN_MIN 0xa0 -#define BPARS600_MFR_VIN_MAX 0xa1 -#define BPARS600_MFR_IIN_MAX 0xa2 -#define BPARS600_MFR_PIN_MAX 0xa3 -#define BPARS600_MFR_VOUT_MIN 0xa4 -#define BPARS600_MFR_VOUT_MAX 0xa5 -#define BPARS600_MFR_IOUT_MAX 0xa6 -#define BPARS600_MFR_POUT_MAX 0xa7 - enum chips { bpa_rs600, bpd_rs600 }; static int bpa_rs600_read_byte_data(struct i2c_client *client, int page, int reg) @@ -83,28 +74,28 @@ static int bpa_rs600_read_word_data(struct i2c_client *client, int page, int pha switch (reg) { case PMBUS_VIN_UV_WARN_LIMIT: - ret = pmbus_read_word_data(client, 0, 0xff, BPARS600_MFR_VIN_MIN); + ret = pmbus_read_word_data(client, 0, 0xff, PMBUS_MFR_VIN_MIN); break; case PMBUS_VIN_OV_WARN_LIMIT: - ret = pmbus_read_word_data(client, 0, 0xff, BPARS600_MFR_VIN_MAX); + ret = pmbus_read_word_data(client, 0, 0xff, PMBUS_MFR_VIN_MAX); break; case PMBUS_VOUT_UV_WARN_LIMIT: - ret = pmbus_read_word_data(client, 0, 0xff, BPARS600_MFR_VOUT_MIN); + ret = pmbus_read_word_data(client, 0, 0xff, PMBUS_MFR_VOUT_MIN); break; case PMBUS_VOUT_OV_WARN_LIMIT: - ret = pmbus_read_word_data(client, 0, 0xff, BPARS600_MFR_VOUT_MAX); + ret = pmbus_read_word_data(client, 0, 0xff, PMBUS_MFR_VOUT_MAX); break; case PMBUS_IIN_OC_WARN_LIMIT: - ret = pmbus_read_word_data(client, 0, 0xff, BPARS600_MFR_IIN_MAX); + ret = pmbus_read_word_data(client, 0, 0xff, PMBUS_MFR_IIN_MAX); break; case PMBUS_IOUT_OC_WARN_LIMIT: - ret = pmbus_read_word_data(client, 0, 0xff, BPARS600_MFR_IOUT_MAX); + ret = pmbus_read_word_data(client, 0, 0xff, PMBUS_MFR_IOUT_MAX); break; case PMBUS_PIN_OP_WARN_LIMIT: - ret = pmbus_read_word_data(client, 0, 0xff, BPARS600_MFR_PIN_MAX); + ret = pmbus_read_word_data(client, 0, 0xff, PMBUS_MFR_PIN_MAX); break; case PMBUS_POUT_OP_WARN_LIMIT: - ret = pmbus_read_word_data(client, 0, 0xff, BPARS600_MFR_POUT_MAX); + ret = pmbus_read_word_data(client, 0, 0xff, PMBUS_MFR_POUT_MAX); break; case PMBUS_VIN_UV_FAULT_LIMIT: case PMBUS_VIN_OV_FAULT_LIMIT: -- 2.32.0 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH 1/2] hwmon: (pmbus/bpa-rs600) Remove duplicate defininitions 2021-08-11 4:17 ` [PATCH 1/2] hwmon: (pmbus/bpa-rs600) Remove duplicate defininitions Chris Packham @ 2021-08-11 19:53 ` Guenter Roeck 2021-08-11 21:58 ` Chris Packham 0 siblings, 1 reply; 12+ messages in thread From: Guenter Roeck @ 2021-08-11 19:53 UTC (permalink / raw) To: Chris Packham; +Cc: jdelvare, linux-hwmon, linux-kernel On Wed, Aug 11, 2021 at 04:17:37PM +1200, Chris Packham wrote: > Commit 787c095edaa9 ("hwmon: (pmbus/core) Add support for rated > attributes") added definitions for MFR_VIN_MIN etc so we can remove the > local definitions of these from the bpa-rs600 driver. > > Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz> > --- > drivers/hwmon/pmbus/bpa-rs600.c | 25 ++++++++----------------- > 1 file changed, 8 insertions(+), 17 deletions(-) > > diff --git a/drivers/hwmon/pmbus/bpa-rs600.c b/drivers/hwmon/pmbus/bpa-rs600.c > index d205b41540ce..d495faa89799 100644 > --- a/drivers/hwmon/pmbus/bpa-rs600.c > +++ b/drivers/hwmon/pmbus/bpa-rs600.c > @@ -12,15 +12,6 @@ > #include <linux/pmbus.h> > #include "pmbus.h" > > -#define BPARS600_MFR_VIN_MIN 0xa0 > -#define BPARS600_MFR_VIN_MAX 0xa1 > -#define BPARS600_MFR_IIN_MAX 0xa2 > -#define BPARS600_MFR_PIN_MAX 0xa3 > -#define BPARS600_MFR_VOUT_MIN 0xa4 > -#define BPARS600_MFR_VOUT_MAX 0xa5 > -#define BPARS600_MFR_IOUT_MAX 0xa6 > -#define BPARS600_MFR_POUT_MAX 0xa7 > - > enum chips { bpa_rs600, bpd_rs600 }; > > static int bpa_rs600_read_byte_data(struct i2c_client *client, int page, int reg) > @@ -83,28 +74,28 @@ static int bpa_rs600_read_word_data(struct i2c_client *client, int page, int pha > > switch (reg) { > case PMBUS_VIN_UV_WARN_LIMIT: > - ret = pmbus_read_word_data(client, 0, 0xff, BPARS600_MFR_VIN_MIN); > + ret = pmbus_read_word_data(client, 0, 0xff, PMBUS_MFR_VIN_MIN); > break; > case PMBUS_VIN_OV_WARN_LIMIT: > - ret = pmbus_read_word_data(client, 0, 0xff, BPARS600_MFR_VIN_MAX); > + ret = pmbus_read_word_data(client, 0, 0xff, PMBUS_MFR_VIN_MAX); > break; > case PMBUS_VOUT_UV_WARN_LIMIT: > - ret = pmbus_read_word_data(client, 0, 0xff, BPARS600_MFR_VOUT_MIN); > + ret = pmbus_read_word_data(client, 0, 0xff, PMBUS_MFR_VOUT_MIN); > break; > case PMBUS_VOUT_OV_WARN_LIMIT: > - ret = pmbus_read_word_data(client, 0, 0xff, BPARS600_MFR_VOUT_MAX); > + ret = pmbus_read_word_data(client, 0, 0xff, PMBUS_MFR_VOUT_MAX); > break; > case PMBUS_IIN_OC_WARN_LIMIT: > - ret = pmbus_read_word_data(client, 0, 0xff, BPARS600_MFR_IIN_MAX); > + ret = pmbus_read_word_data(client, 0, 0xff, PMBUS_MFR_IIN_MAX); > break; > case PMBUS_IOUT_OC_WARN_LIMIT: > - ret = pmbus_read_word_data(client, 0, 0xff, BPARS600_MFR_IOUT_MAX); > + ret = pmbus_read_word_data(client, 0, 0xff, PMBUS_MFR_IOUT_MAX); > break; > case PMBUS_PIN_OP_WARN_LIMIT: > - ret = pmbus_read_word_data(client, 0, 0xff, BPARS600_MFR_PIN_MAX); > + ret = pmbus_read_word_data(client, 0, 0xff, PMBUS_MFR_PIN_MAX); > break; > case PMBUS_POUT_OP_WARN_LIMIT: > - ret = pmbus_read_word_data(client, 0, 0xff, BPARS600_MFR_POUT_MAX); > + ret = pmbus_read_word_data(client, 0, 0xff, PMBUS_MFR_POUT_MAX); If the above is correct, the driver reports the wrong attributes. For example, PMBUS_MFR_PIN_MAX is supposed to report the rated limit, not the warning limit. What does the datasheet say ? Guenter > break; > case PMBUS_VIN_UV_FAULT_LIMIT: > case PMBUS_VIN_OV_FAULT_LIMIT: > -- > 2.32.0 > ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 1/2] hwmon: (pmbus/bpa-rs600) Remove duplicate defininitions 2021-08-11 19:53 ` Guenter Roeck @ 2021-08-11 21:58 ` Chris Packham 0 siblings, 0 replies; 12+ messages in thread From: Chris Packham @ 2021-08-11 21:58 UTC (permalink / raw) To: Guenter Roeck; +Cc: jdelvare, linux-hwmon, linux-kernel On 12/08/21 7:53 am, Guenter Roeck wrote: > On Wed, Aug 11, 2021 at 04:17:37PM +1200, Chris Packham wrote: >> Commit 787c095edaa9 ("hwmon: (pmbus/core) Add support for rated >> attributes") added definitions for MFR_VIN_MIN etc so we can remove the >> local definitions of these from the bpa-rs600 driver. >> >> Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz> >> --- >> drivers/hwmon/pmbus/bpa-rs600.c | 25 ++++++++----------------- >> 1 file changed, 8 insertions(+), 17 deletions(-) >> >> diff --git a/drivers/hwmon/pmbus/bpa-rs600.c b/drivers/hwmon/pmbus/bpa-rs600.c >> index d205b41540ce..d495faa89799 100644 >> --- a/drivers/hwmon/pmbus/bpa-rs600.c >> +++ b/drivers/hwmon/pmbus/bpa-rs600.c >> @@ -12,15 +12,6 @@ >> #include <linux/pmbus.h> >> #include "pmbus.h" >> >> -#define BPARS600_MFR_VIN_MIN 0xa0 >> -#define BPARS600_MFR_VIN_MAX 0xa1 >> -#define BPARS600_MFR_IIN_MAX 0xa2 >> -#define BPARS600_MFR_PIN_MAX 0xa3 >> -#define BPARS600_MFR_VOUT_MIN 0xa4 >> -#define BPARS600_MFR_VOUT_MAX 0xa5 >> -#define BPARS600_MFR_IOUT_MAX 0xa6 >> -#define BPARS600_MFR_POUT_MAX 0xa7 >> - >> enum chips { bpa_rs600, bpd_rs600 }; >> >> static int bpa_rs600_read_byte_data(struct i2c_client *client, int page, int reg) >> @@ -83,28 +74,28 @@ static int bpa_rs600_read_word_data(struct i2c_client *client, int page, int pha >> >> switch (reg) { >> case PMBUS_VIN_UV_WARN_LIMIT: >> - ret = pmbus_read_word_data(client, 0, 0xff, BPARS600_MFR_VIN_MIN); >> + ret = pmbus_read_word_data(client, 0, 0xff, PMBUS_MFR_VIN_MIN); >> break; >> case PMBUS_VIN_OV_WARN_LIMIT: >> - ret = pmbus_read_word_data(client, 0, 0xff, BPARS600_MFR_VIN_MAX); >> + ret = pmbus_read_word_data(client, 0, 0xff, PMBUS_MFR_VIN_MAX); >> break; >> case PMBUS_VOUT_UV_WARN_LIMIT: >> - ret = pmbus_read_word_data(client, 0, 0xff, BPARS600_MFR_VOUT_MIN); >> + ret = pmbus_read_word_data(client, 0, 0xff, PMBUS_MFR_VOUT_MIN); >> break; >> case PMBUS_VOUT_OV_WARN_LIMIT: >> - ret = pmbus_read_word_data(client, 0, 0xff, BPARS600_MFR_VOUT_MAX); >> + ret = pmbus_read_word_data(client, 0, 0xff, PMBUS_MFR_VOUT_MAX); >> break; >> case PMBUS_IIN_OC_WARN_LIMIT: >> - ret = pmbus_read_word_data(client, 0, 0xff, BPARS600_MFR_IIN_MAX); >> + ret = pmbus_read_word_data(client, 0, 0xff, PMBUS_MFR_IIN_MAX); >> break; >> case PMBUS_IOUT_OC_WARN_LIMIT: >> - ret = pmbus_read_word_data(client, 0, 0xff, BPARS600_MFR_IOUT_MAX); >> + ret = pmbus_read_word_data(client, 0, 0xff, PMBUS_MFR_IOUT_MAX); >> break; >> case PMBUS_PIN_OP_WARN_LIMIT: >> - ret = pmbus_read_word_data(client, 0, 0xff, BPARS600_MFR_PIN_MAX); >> + ret = pmbus_read_word_data(client, 0, 0xff, PMBUS_MFR_PIN_MAX); >> break; >> case PMBUS_POUT_OP_WARN_LIMIT: >> - ret = pmbus_read_word_data(client, 0, 0xff, BPARS600_MFR_POUT_MAX); >> + ret = pmbus_read_word_data(client, 0, 0xff, PMBUS_MFR_POUT_MAX); > If the above is correct, the driver reports the wrong attributes. For example, > PMBUS_MFR_PIN_MAX is supposed to report the rated limit, not the warning limit. > What does the datasheet say ? The datasheet doesn't list PMBUS_VIN_UV_WARN_LIMIT etc at all. It does say that MFR_VIN_xxx is the "rated" value but in my testing this also appears that this is the same threshold at which the ALERT pin is asserted. When I did the initial implementation I decided to map the WARN_LIMITs to what I thought were reasonable manufacturer specific equivalents. This also means that the thresholds can be displayed by existing userspace tools that consume the sysfs ABI. > Guenter > >> break; >> case PMBUS_VIN_UV_FAULT_LIMIT: >> case PMBUS_VIN_OV_FAULT_LIMIT: >> -- >> 2.32.0 >> ^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH 2/2] hwmon: (pmbus/bpa-rs600) Add workaround for incorrect Pin max 2021-08-11 4:17 [PATCH 0/2] hwmon: (pmbus/bpa-rs600) cleanup and workaround Chris Packham 2021-08-11 4:17 ` [PATCH 1/2] hwmon: (pmbus/bpa-rs600) Remove duplicate defininitions Chris Packham @ 2021-08-11 4:17 ` Chris Packham 2021-08-11 19:53 ` Guenter Roeck 2021-08-12 3:15 ` [PATCH 0/2] hwmon: (pmbus/bpa-rs600) cleanup and workaround Chris Packham 2 siblings, 1 reply; 12+ messages in thread From: Chris Packham @ 2021-08-11 4:17 UTC (permalink / raw) To: linux, jdelvare; +Cc: linux-hwmon, linux-kernel, Chris Packham BPD-RS600 modules running firmware v5.70 misreport the MFR_PIN_MAX. The indicate a maximum of 1640W instead of 700W. Detect the invalid reading and return a sensible value instead. Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz> --- drivers/hwmon/pmbus/bpa-rs600.c | 21 ++++++++++++++++++++- 1 file changed, 20 insertions(+), 1 deletion(-) diff --git a/drivers/hwmon/pmbus/bpa-rs600.c b/drivers/hwmon/pmbus/bpa-rs600.c index d495faa89799..f4baed9ce8a4 100644 --- a/drivers/hwmon/pmbus/bpa-rs600.c +++ b/drivers/hwmon/pmbus/bpa-rs600.c @@ -65,6 +65,24 @@ static int bpa_rs600_read_vin(struct i2c_client *client) return ret; } +/* + * The firmware on some BPD-RS600 models incorrectly reports 1640W + * for MFR_PIN_MAX. Deal with this by returning a sensible value. + */ +static int bpa_rs600_read_pin_max(struct i2c_client *client) +{ + int ret; + + ret = pmbus_read_word_data(client, 0, 0xff, PMBUS_MFR_PIN_MAX); + if (ret < 0) + return ret; + + if (ret == 0x0b34) + return 0x095e; + + return ret; +} + static int bpa_rs600_read_word_data(struct i2c_client *client, int page, int phase, int reg) { int ret; @@ -92,7 +110,8 @@ static int bpa_rs600_read_word_data(struct i2c_client *client, int page, int pha ret = pmbus_read_word_data(client, 0, 0xff, PMBUS_MFR_IOUT_MAX); break; case PMBUS_PIN_OP_WARN_LIMIT: - ret = pmbus_read_word_data(client, 0, 0xff, PMBUS_MFR_PIN_MAX); + case PMBUS_MFR_PIN_MAX: + ret = bpa_rs600_read_pin_max(client); break; case PMBUS_POUT_OP_WARN_LIMIT: ret = pmbus_read_word_data(client, 0, 0xff, PMBUS_MFR_POUT_MAX); -- 2.32.0 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH 2/2] hwmon: (pmbus/bpa-rs600) Add workaround for incorrect Pin max 2021-08-11 4:17 ` [PATCH 2/2] hwmon: (pmbus/bpa-rs600) Add workaround for incorrect Pin max Chris Packham @ 2021-08-11 19:53 ` Guenter Roeck 2021-08-11 22:19 ` Chris Packham 0 siblings, 1 reply; 12+ messages in thread From: Guenter Roeck @ 2021-08-11 19:53 UTC (permalink / raw) To: Chris Packham; +Cc: jdelvare, linux-hwmon, linux-kernel On Wed, Aug 11, 2021 at 04:17:38PM +1200, Chris Packham wrote: > BPD-RS600 modules running firmware v5.70 misreport the MFR_PIN_MAX. > The indicate a maximum of 1640W instead of 700W. Detect the invalid > reading and return a sensible value instead. > > Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz> > --- > drivers/hwmon/pmbus/bpa-rs600.c | 21 ++++++++++++++++++++- > 1 file changed, 20 insertions(+), 1 deletion(-) > > diff --git a/drivers/hwmon/pmbus/bpa-rs600.c b/drivers/hwmon/pmbus/bpa-rs600.c > index d495faa89799..f4baed9ce8a4 100644 > --- a/drivers/hwmon/pmbus/bpa-rs600.c > +++ b/drivers/hwmon/pmbus/bpa-rs600.c > @@ -65,6 +65,24 @@ static int bpa_rs600_read_vin(struct i2c_client *client) > return ret; > } > > +/* > + * The firmware on some BPD-RS600 models incorrectly reports 1640W > + * for MFR_PIN_MAX. Deal with this by returning a sensible value. > + */ > +static int bpa_rs600_read_pin_max(struct i2c_client *client) > +{ > + int ret; > + > + ret = pmbus_read_word_data(client, 0, 0xff, PMBUS_MFR_PIN_MAX); > + if (ret < 0) > + return ret; > + > + if (ret == 0x0b34) > + return 0x095e; The comments from the descriotion need to be here. Thanks, Guenter > + > + return ret; > +} > + > static int bpa_rs600_read_word_data(struct i2c_client *client, int page, int phase, int reg) > { > int ret; > @@ -92,7 +110,8 @@ static int bpa_rs600_read_word_data(struct i2c_client *client, int page, int pha > ret = pmbus_read_word_data(client, 0, 0xff, PMBUS_MFR_IOUT_MAX); > break; > case PMBUS_PIN_OP_WARN_LIMIT: > - ret = pmbus_read_word_data(client, 0, 0xff, PMBUS_MFR_PIN_MAX); > + case PMBUS_MFR_PIN_MAX: > + ret = bpa_rs600_read_pin_max(client); So the idea is to return the same value for PMBUS_PIN_OP_WARN_LIMIT (max_alarm) and PMBUS_MFR_PIN_MAX (rated_max) ? That doesn't really make sense. The meaning of those limits is distinctly different. Guenter > break; > case PMBUS_POUT_OP_WARN_LIMIT: > ret = pmbus_read_word_data(client, 0, 0xff, PMBUS_MFR_POUT_MAX); > -- > 2.32.0 > ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 2/2] hwmon: (pmbus/bpa-rs600) Add workaround for incorrect Pin max 2021-08-11 19:53 ` Guenter Roeck @ 2021-08-11 22:19 ` Chris Packham 2021-08-11 23:18 ` Guenter Roeck 0 siblings, 1 reply; 12+ messages in thread From: Chris Packham @ 2021-08-11 22:19 UTC (permalink / raw) To: Guenter Roeck; +Cc: jdelvare, linux-hwmon, linux-kernel On 12/08/21 7:53 am, Guenter Roeck wrote: > On Wed, Aug 11, 2021 at 04:17:38PM +1200, Chris Packham wrote: >> BPD-RS600 modules running firmware v5.70 misreport the MFR_PIN_MAX. >> The indicate a maximum of 1640W instead of 700W. Detect the invalid >> reading and return a sensible value instead. >> >> Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz> >> --- >> drivers/hwmon/pmbus/bpa-rs600.c | 21 ++++++++++++++++++++- >> 1 file changed, 20 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/hwmon/pmbus/bpa-rs600.c b/drivers/hwmon/pmbus/bpa-rs600.c >> index d495faa89799..f4baed9ce8a4 100644 >> --- a/drivers/hwmon/pmbus/bpa-rs600.c >> +++ b/drivers/hwmon/pmbus/bpa-rs600.c >> @@ -65,6 +65,24 @@ static int bpa_rs600_read_vin(struct i2c_client *client) >> return ret; >> } >> >> +/* >> + * The firmware on some BPD-RS600 models incorrectly reports 1640W >> + * for MFR_PIN_MAX. Deal with this by returning a sensible value. >> + */ >> +static int bpa_rs600_read_pin_max(struct i2c_client *client) >> +{ >> + int ret; >> + >> + ret = pmbus_read_word_data(client, 0, 0xff, PMBUS_MFR_PIN_MAX); >> + if (ret < 0) >> + return ret; >> + >> + if (ret == 0x0b34) >> + return 0x095e; > The comments from the descriotion need to be here. will update > Thanks, > Guenter > >> + >> + return ret; >> +} >> + >> static int bpa_rs600_read_word_data(struct i2c_client *client, int page, int phase, int reg) >> { >> int ret; >> @@ -92,7 +110,8 @@ static int bpa_rs600_read_word_data(struct i2c_client *client, int page, int pha >> ret = pmbus_read_word_data(client, 0, 0xff, PMBUS_MFR_IOUT_MAX); >> break; >> case PMBUS_PIN_OP_WARN_LIMIT: >> - ret = pmbus_read_word_data(client, 0, 0xff, PMBUS_MFR_PIN_MAX); >> + case PMBUS_MFR_PIN_MAX: >> + ret = bpa_rs600_read_pin_max(client); > So the idea is to return the same value for PMBUS_PIN_OP_WARN_LIMIT > (max_alarm) and PMBUS_MFR_PIN_MAX (rated_max) ? That doesn't really > make sense. The meaning of those limits is distinctly different. For the BPA-RS600/BPD-RS600 these appear to be treated the same. > > Guenter > >> break; >> case PMBUS_POUT_OP_WARN_LIMIT: >> ret = pmbus_read_word_data(client, 0, 0xff, PMBUS_MFR_POUT_MAX); >> -- >> 2.32.0 >> ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 2/2] hwmon: (pmbus/bpa-rs600) Add workaround for incorrect Pin max 2021-08-11 22:19 ` Chris Packham @ 2021-08-11 23:18 ` Guenter Roeck 2021-08-11 23:25 ` Chris Packham 0 siblings, 1 reply; 12+ messages in thread From: Guenter Roeck @ 2021-08-11 23:18 UTC (permalink / raw) To: Chris Packham; +Cc: jdelvare, linux-hwmon, linux-kernel On Wed, Aug 11, 2021 at 10:19:44PM +0000, Chris Packham wrote: > > On 12/08/21 7:53 am, Guenter Roeck wrote: > > On Wed, Aug 11, 2021 at 04:17:38PM +1200, Chris Packham wrote: > >> BPD-RS600 modules running firmware v5.70 misreport the MFR_PIN_MAX. > >> The indicate a maximum of 1640W instead of 700W. Detect the invalid > >> reading and return a sensible value instead. > >> > >> Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz> > >> --- > >> drivers/hwmon/pmbus/bpa-rs600.c | 21 ++++++++++++++++++++- > >> 1 file changed, 20 insertions(+), 1 deletion(-) > >> > >> diff --git a/drivers/hwmon/pmbus/bpa-rs600.c b/drivers/hwmon/pmbus/bpa-rs600.c > >> index d495faa89799..f4baed9ce8a4 100644 > >> --- a/drivers/hwmon/pmbus/bpa-rs600.c > >> +++ b/drivers/hwmon/pmbus/bpa-rs600.c > >> @@ -65,6 +65,24 @@ static int bpa_rs600_read_vin(struct i2c_client *client) > >> return ret; > >> } > >> > >> +/* > >> + * The firmware on some BPD-RS600 models incorrectly reports 1640W > >> + * for MFR_PIN_MAX. Deal with this by returning a sensible value. > >> + */ > >> +static int bpa_rs600_read_pin_max(struct i2c_client *client) > >> +{ > >> + int ret; > >> + > >> + ret = pmbus_read_word_data(client, 0, 0xff, PMBUS_MFR_PIN_MAX); > >> + if (ret < 0) > >> + return ret; > >> + > >> + if (ret == 0x0b34) > >> + return 0x095e; > > The comments from the descriotion need to be here. > will update > > Thanks, > > Guenter > > > >> + > >> + return ret; > >> +} > >> + > >> static int bpa_rs600_read_word_data(struct i2c_client *client, int page, int phase, int reg) > >> { > >> int ret; > >> @@ -92,7 +110,8 @@ static int bpa_rs600_read_word_data(struct i2c_client *client, int page, int pha > >> ret = pmbus_read_word_data(client, 0, 0xff, PMBUS_MFR_IOUT_MAX); > >> break; > >> case PMBUS_PIN_OP_WARN_LIMIT: > >> - ret = pmbus_read_word_data(client, 0, 0xff, PMBUS_MFR_PIN_MAX); > >> + case PMBUS_MFR_PIN_MAX: > >> + ret = bpa_rs600_read_pin_max(client); > > So the idea is to return the same value for PMBUS_PIN_OP_WARN_LIMIT > > (max_alarm) and PMBUS_MFR_PIN_MAX (rated_max) ? That doesn't really > > make sense. The meaning of those limits is distinctly different. > For the BPA-RS600/BPD-RS600 these appear to be treated the same. What a mess. This needs to be documented in the driver, including the behavior if any of those attributes is written into. Guenter > > > > Guenter > > > >> break; > >> case PMBUS_POUT_OP_WARN_LIMIT: > >> ret = pmbus_read_word_data(client, 0, 0xff, PMBUS_MFR_POUT_MAX); > >> -- > >> 2.32.0 > >> ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 2/2] hwmon: (pmbus/bpa-rs600) Add workaround for incorrect Pin max 2021-08-11 23:18 ` Guenter Roeck @ 2021-08-11 23:25 ` Chris Packham 2021-08-12 0:41 ` Guenter Roeck 0 siblings, 1 reply; 12+ messages in thread From: Chris Packham @ 2021-08-11 23:25 UTC (permalink / raw) To: Guenter Roeck; +Cc: jdelvare, linux-hwmon, linux-kernel On 12/08/21 11:18 am, Guenter Roeck wrote: > On Wed, Aug 11, 2021 at 10:19:44PM +0000, Chris Packham wrote: >> On 12/08/21 7:53 am, Guenter Roeck wrote: >>> On Wed, Aug 11, 2021 at 04:17:38PM +1200, Chris Packham wrote: >>>> BPD-RS600 modules running firmware v5.70 misreport the MFR_PIN_MAX. >>>> The indicate a maximum of 1640W instead of 700W. Detect the invalid >>>> reading and return a sensible value instead. >>>> >>>> Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz> >>>> --- >>>> drivers/hwmon/pmbus/bpa-rs600.c | 21 ++++++++++++++++++++- >>>> 1 file changed, 20 insertions(+), 1 deletion(-) >>>> >>>> diff --git a/drivers/hwmon/pmbus/bpa-rs600.c b/drivers/hwmon/pmbus/bpa-rs600.c >>>> index d495faa89799..f4baed9ce8a4 100644 >>>> --- a/drivers/hwmon/pmbus/bpa-rs600.c >>>> +++ b/drivers/hwmon/pmbus/bpa-rs600.c >>>> @@ -65,6 +65,24 @@ static int bpa_rs600_read_vin(struct i2c_client *client) >>>> return ret; >>>> } >>>> >>>> +/* >>>> + * The firmware on some BPD-RS600 models incorrectly reports 1640W >>>> + * for MFR_PIN_MAX. Deal with this by returning a sensible value. >>>> + */ >>>> +static int bpa_rs600_read_pin_max(struct i2c_client *client) >>>> +{ >>>> + int ret; >>>> + >>>> + ret = pmbus_read_word_data(client, 0, 0xff, PMBUS_MFR_PIN_MAX); >>>> + if (ret < 0) >>>> + return ret; >>>> + >>>> + if (ret == 0x0b34) >>>> + return 0x095e; >>> The comments from the descriotion need to be here. >> will update >>> Thanks, >>> Guenter >>> >>>> + >>>> + return ret; >>>> +} >>>> + >>>> static int bpa_rs600_read_word_data(struct i2c_client *client, int page, int phase, int reg) >>>> { >>>> int ret; >>>> @@ -92,7 +110,8 @@ static int bpa_rs600_read_word_data(struct i2c_client *client, int page, int pha >>>> ret = pmbus_read_word_data(client, 0, 0xff, PMBUS_MFR_IOUT_MAX); >>>> break; >>>> case PMBUS_PIN_OP_WARN_LIMIT: >>>> - ret = pmbus_read_word_data(client, 0, 0xff, PMBUS_MFR_PIN_MAX); >>>> + case PMBUS_MFR_PIN_MAX: >>>> + ret = bpa_rs600_read_pin_max(client); >>> So the idea is to return the same value for PMBUS_PIN_OP_WARN_LIMIT >>> (max_alarm) and PMBUS_MFR_PIN_MAX (rated_max) ? That doesn't really >>> make sense. The meaning of those limits is distinctly different. >> For the BPA-RS600/BPD-RS600 these appear to be treated the same. > What a mess. *sigh* I know. I've also got another 2 BluTek supplies I haven't got round to dealing with yet. > This needs to be documented in the driver, including the > behavior if any of those attributes is written into. Mercifully these attributes are all read-only. So at least we don't have to deal with that. It's probably not too late to return -ENXIO for the WARN_LIMITs and have lm-sensors display the rated_max (we also have a custom consumer of the sysfs API that I'd need to sort out). > > Guenter > >>> Guenter >>> >>>> break; >>>> case PMBUS_POUT_OP_WARN_LIMIT: >>>> ret = pmbus_read_word_data(client, 0, 0xff, PMBUS_MFR_POUT_MAX); >>>> -- >>>> 2.32.0 >>>> ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 2/2] hwmon: (pmbus/bpa-rs600) Add workaround for incorrect Pin max 2021-08-11 23:25 ` Chris Packham @ 2021-08-12 0:41 ` Guenter Roeck 0 siblings, 0 replies; 12+ messages in thread From: Guenter Roeck @ 2021-08-12 0:41 UTC (permalink / raw) To: Chris Packham; +Cc: jdelvare, linux-hwmon, linux-kernel On 8/11/21 4:25 PM, Chris Packham wrote: > > On 12/08/21 11:18 am, Guenter Roeck wrote: >> On Wed, Aug 11, 2021 at 10:19:44PM +0000, Chris Packham wrote: >>> On 12/08/21 7:53 am, Guenter Roeck wrote: >>>> On Wed, Aug 11, 2021 at 04:17:38PM +1200, Chris Packham wrote: >>>>> BPD-RS600 modules running firmware v5.70 misreport the MFR_PIN_MAX. >>>>> The indicate a maximum of 1640W instead of 700W. Detect the invalid >>>>> reading and return a sensible value instead. >>>>> >>>>> Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz> >>>>> --- >>>>> drivers/hwmon/pmbus/bpa-rs600.c | 21 ++++++++++++++++++++- >>>>> 1 file changed, 20 insertions(+), 1 deletion(-) >>>>> >>>>> diff --git a/drivers/hwmon/pmbus/bpa-rs600.c b/drivers/hwmon/pmbus/bpa-rs600.c >>>>> index d495faa89799..f4baed9ce8a4 100644 >>>>> --- a/drivers/hwmon/pmbus/bpa-rs600.c >>>>> +++ b/drivers/hwmon/pmbus/bpa-rs600.c >>>>> @@ -65,6 +65,24 @@ static int bpa_rs600_read_vin(struct i2c_client *client) >>>>> return ret; >>>>> } >>>>> >>>>> +/* >>>>> + * The firmware on some BPD-RS600 models incorrectly reports 1640W >>>>> + * for MFR_PIN_MAX. Deal with this by returning a sensible value. >>>>> + */ >>>>> +static int bpa_rs600_read_pin_max(struct i2c_client *client) >>>>> +{ >>>>> + int ret; >>>>> + >>>>> + ret = pmbus_read_word_data(client, 0, 0xff, PMBUS_MFR_PIN_MAX); >>>>> + if (ret < 0) >>>>> + return ret; >>>>> + >>>>> + if (ret == 0x0b34) >>>>> + return 0x095e; >>>> The comments from the descriotion need to be here. >>> will update >>>> Thanks, >>>> Guenter >>>> >>>>> + >>>>> + return ret; >>>>> +} >>>>> + >>>>> static int bpa_rs600_read_word_data(struct i2c_client *client, int page, int phase, int reg) >>>>> { >>>>> int ret; >>>>> @@ -92,7 +110,8 @@ static int bpa_rs600_read_word_data(struct i2c_client *client, int page, int pha >>>>> ret = pmbus_read_word_data(client, 0, 0xff, PMBUS_MFR_IOUT_MAX); >>>>> break; >>>>> case PMBUS_PIN_OP_WARN_LIMIT: >>>>> - ret = pmbus_read_word_data(client, 0, 0xff, PMBUS_MFR_PIN_MAX); >>>>> + case PMBUS_MFR_PIN_MAX: >>>>> + ret = bpa_rs600_read_pin_max(client); >>>> So the idea is to return the same value for PMBUS_PIN_OP_WARN_LIMIT >>>> (max_alarm) and PMBUS_MFR_PIN_MAX (rated_max) ? That doesn't really >>>> make sense. The meaning of those limits is distinctly different. >>> For the BPA-RS600/BPD-RS600 these appear to be treated the same. >> What a mess. > *sigh* I know. I've also got another 2 BluTek supplies I haven't got > round to dealing with yet. >> This needs to be documented in the driver, including the >> behavior if any of those attributes is written into. > > Mercifully these attributes are all read-only. So at least we don't have > to deal with that. > Ok. > It's probably not too late to return -ENXIO for the WARN_LIMITs and have > lm-sensors display the rated_max (we also have a custom consumer of the > sysfs API that I'd need to sort out). > That would indeed be much better if it works for you. Thanks, Guenter >> >> Guenter >> >>>> Guenter >>>> >>>>> break; >>>>> case PMBUS_POUT_OP_WARN_LIMIT: >>>>> ret = pmbus_read_word_data(client, 0, 0xff, PMBUS_MFR_POUT_MAX); >>>>> -- >>>>> 2.32.0 >>>> > ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 0/2] hwmon: (pmbus/bpa-rs600) cleanup and workaround 2021-08-11 4:17 [PATCH 0/2] hwmon: (pmbus/bpa-rs600) cleanup and workaround Chris Packham 2021-08-11 4:17 ` [PATCH 1/2] hwmon: (pmbus/bpa-rs600) Remove duplicate defininitions Chris Packham 2021-08-11 4:17 ` [PATCH 2/2] hwmon: (pmbus/bpa-rs600) Add workaround for incorrect Pin max Chris Packham @ 2021-08-12 3:15 ` Chris Packham 2021-08-12 4:29 ` Guenter Roeck 2 siblings, 1 reply; 12+ messages in thread From: Chris Packham @ 2021-08-12 3:15 UTC (permalink / raw) To: linux, jdelvare; +Cc: linux-hwmon, linux-kernel On 11/08/21 4:17 pm, Chris Packham wrote: > This series builds on top of the BPD-RS600 support[1] which is in Guenter's > tree by hasn't made it to Linus' yet. They might actually cleanly apply without > it since they touch different parts of the file. > > [1] - https://lore.kernel.org/linux-hwmon/20210708220618.23576-1-chris.packham@alliedtelesis.co.nz/ > > Chris Packham (2): > hwmon: (pmbus/bpa-rs600) Remove duplicate defininitions > hwmon: (pmbus/bpa-rs600) Add workaround for incorrect Pin max > > drivers/hwmon/pmbus/bpa-rs600.c | 44 ++++++++++++++++++++------------- > 1 file changed, 27 insertions(+), 17 deletions(-) I've also sent a PR for adding display of the rated values to lm-sensors https://github.com/lm-sensors/lm-sensors/pull/358 ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 0/2] hwmon: (pmbus/bpa-rs600) cleanup and workaround 2021-08-12 3:15 ` [PATCH 0/2] hwmon: (pmbus/bpa-rs600) cleanup and workaround Chris Packham @ 2021-08-12 4:29 ` Guenter Roeck 0 siblings, 0 replies; 12+ messages in thread From: Guenter Roeck @ 2021-08-12 4:29 UTC (permalink / raw) To: Chris Packham, jdelvare; +Cc: linux-hwmon, linux-kernel On 8/11/21 8:15 PM, Chris Packham wrote: > > On 11/08/21 4:17 pm, Chris Packham wrote: >> This series builds on top of the BPD-RS600 support[1] which is in Guenter's >> tree by hasn't made it to Linus' yet. They might actually cleanly apply without >> it since they touch different parts of the file. >> >> [1] - https://lore.kernel.org/linux-hwmon/20210708220618.23576-1-chris.packham@alliedtelesis.co.nz/ >> >> Chris Packham (2): >> hwmon: (pmbus/bpa-rs600) Remove duplicate defininitions >> hwmon: (pmbus/bpa-rs600) Add workaround for incorrect Pin max >> >> drivers/hwmon/pmbus/bpa-rs600.c | 44 ++++++++++++++++++++------------- >> 1 file changed, 27 insertions(+), 17 deletions(-) > > I've also sent a PR for adding display of the rated values to lm-sensors > > https://github.com/lm-sensors/lm-sensors/pull/358 > Excellent, thanks! Guenter ^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2021-08-12 4:29 UTC | newest] Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-08-11 4:17 [PATCH 0/2] hwmon: (pmbus/bpa-rs600) cleanup and workaround Chris Packham 2021-08-11 4:17 ` [PATCH 1/2] hwmon: (pmbus/bpa-rs600) Remove duplicate defininitions Chris Packham 2021-08-11 19:53 ` Guenter Roeck 2021-08-11 21:58 ` Chris Packham 2021-08-11 4:17 ` [PATCH 2/2] hwmon: (pmbus/bpa-rs600) Add workaround for incorrect Pin max Chris Packham 2021-08-11 19:53 ` Guenter Roeck 2021-08-11 22:19 ` Chris Packham 2021-08-11 23:18 ` Guenter Roeck 2021-08-11 23:25 ` Chris Packham 2021-08-12 0:41 ` Guenter Roeck 2021-08-12 3:15 ` [PATCH 0/2] hwmon: (pmbus/bpa-rs600) cleanup and workaround Chris Packham 2021-08-12 4:29 ` Guenter Roeck
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).