| Message ID | 20210811041738.15061-3-chris.packham@alliedtelesis.co.nz (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | hwmon: (pmbus/bpa-rs600) cleanup and workaround | expand |
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 >
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 >>
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 > >>
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 >>>>
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 >>>> >
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);
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(-)