[PATCH 0/2] hwmon: (pmbus/bpa-rs600) cleanup and workaround

[PATCH 0/2] hwmon: (pmbus/bpa-rs600) cleanup and workaround

[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

[PATCH 1/2] hwmon: (pmbus/bpa-rs600) Remove duplicate defininitions

[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

Re: [PATCH 1/2] hwmon: (pmbus/bpa-rs600) Remove duplicate defininitions

[Guenter Roeck]

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
> 

Re: [PATCH 1/2] hwmon: (pmbus/bpa-rs600) Remove duplicate defininitions

[Chris Packham]

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
>>

[PATCH 2/2] hwmon: (pmbus/bpa-rs600) Add workaround for incorrect Pin max

[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

Re: [PATCH 2/2] hwmon: (pmbus/bpa-rs600) Add workaround for incorrect Pin max

[Guenter Roeck]

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
> 

Re: [PATCH 2/2] hwmon: (pmbus/bpa-rs600) Add workaround for incorrect Pin max

[Chris Packham]

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
>>

Re: [PATCH 2/2] hwmon: (pmbus/bpa-rs600) Add workaround for incorrect Pin max

[Guenter Roeck]

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
> >>

Re: [PATCH 2/2] hwmon: (pmbus/bpa-rs600) Add workaround for incorrect Pin max

[Chris Packham]

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
>>>>

Re: [PATCH 2/2] hwmon: (pmbus/bpa-rs600) Add workaround for incorrect Pin max

[Guenter Roeck]

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
>>>> >

Re: [PATCH 0/2] hwmon: (pmbus/bpa-rs600) cleanup and workaround

[Chris Packham]

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

Re: [PATCH 0/2] hwmon: (pmbus/bpa-rs600) cleanup and workaround

[Guenter Roeck]

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