Re: [PATCH v2 2/3] lm25066: support SAMPLES_FOR_AVG register

[Guenter Roeck <linux@roeck-us.net>]

On 4/14/19 12:00 PM, Adamski, Krzysztof (Nokia - PL/Wroclaw) wrote:
> On Sun, Apr 14, 2019 at 07:37:35AM -0700, Guenter Roeck wrote:
>> On 4/13/19 9:03 AM, Adamski, Krzysztof (Nokia - PL/Wroclaw) wrote:
>>> Manufacturer specific SAMPLES_FOR_AVG register allows setting the number
>>> of samples used in computing the average values (PMBUS_VIRT_READ_*_AVG).
>>> The number we write is an exponent of base 2 of the number of samples so
>>> for example writing 3 will result in 8 samples average.
>>>
>>> Signed-off-by: Krzysztof Adamski <krzysztof.adamski@nokia.com>
>>> ---
>>>   drivers/hwmon/pmbus/lm25066.c | 15 ++++++++++++++-
>>>   1 file changed, 14 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/hwmon/pmbus/lm25066.c b/drivers/hwmon/pmbus/lm25066.c
>>> index 53db78753a0d..b560d33db459 100644
>>> --- a/drivers/hwmon/pmbus/lm25066.c
>>> +++ b/drivers/hwmon/pmbus/lm25066.c
>>> @@ -26,6 +26,7 @@
>>>   #include <linux/err.h>
>>>   #include <linux/slab.h>
>>>   #include <linux/i2c.h>
>>> +#include <linux/log2.h>
>>>   #include "pmbus.h"
>>>   enum chips { lm25056, lm25066, lm5064, lm5066, lm5066i };
>>> @@ -39,12 +40,15 @@ enum chips { lm25056, lm25066, lm5064, lm5066, lm5066i };
>>>   #define LM25066_CLEAR_PIN_PEAK		0xd6
>>>   #define LM25066_DEVICE_SETUP		0xd9
>>>   #define LM25066_READ_AVG_VIN		0xdc
>>> +#define LM25066_SAMPLES_FOR_AVG		0xdb
>>>   #define LM25066_READ_AVG_VOUT		0xdd
>>>   #define LM25066_READ_AVG_IIN		0xde
>>>   #define LM25066_READ_AVG_PIN		0xdf
>>>   #define LM25066_DEV_SETUP_CL		BIT(4)	/* Current limit */
>>> +#define LM25066_SAMPLES_FOR_AVG_MAX	4096
>>> +
>>>   /* LM25056 only */
>>>   #define LM25056_VAUX_OV_WARN_LIMIT	0xe3
>>> @@ -284,6 +288,10 @@ static int lm25066_read_word_data(struct i2c_client *client, int page, int reg)
>>>   	case PMBUS_VIRT_RESET_PIN_HISTORY:
>>>   		ret = 0;
>>>   		break;
>>> +	case PMBUS_VIRT_SAMPLES:
>>> +		ret = pmbus_read_byte_data(client, 0, LM25066_SAMPLES_FOR_AVG);
>>
>> 		if (ret < 0)
>> 			return ret;
>>
> 
> Good point. However the convention for this switch is to break instead
> of returning early. I would stick with that if you don't mind.
> 

I don't mind if you want to use
		if (ret < 0)
			break;

instead, but I do mind if you want to convert an error code into an undefined value.

>>> +		ret = 1 << ret;
>>> +		break;
>>>   	default:
>>>   		ret = -ENODATA;
>>>   		break;
>>> @@ -398,6 +406,11 @@ static int lm25066_write_word_data(struct i2c_client *client, int page, int reg,
>>>   	case PMBUS_VIRT_RESET_PIN_HISTORY:
>>>   		ret = pmbus_write_byte(client, 0, LM25066_CLEAR_PIN_PEAK);
>>>   		break;
>>> +	case PMBUS_VIRT_SAMPLES:
>>> +		word = clamp_val(word, 0, LM25066_SAMPLES_FOR_AVG_MAX);
>>
>> 0 is not valid, and ilog2(0) returns -1 (or is undefined).
> 
> True, I'll clamp with minimum 1 to fix that. I don't think it's worth
> returning an error for 0. Do you?
> 

The whole point of using clamp() is to avoid returning an error. Returning an error
for values <= 0 but clamping large positive values to 4096 seems inconsistent.

Guenter

>>
>>> +		ret = pmbus_write_byte_data(client, 0, LM25066_SAMPLES_FOR_AVG,
>>> +					    ilog2(word));
>>> +		break;
>>>   	default:
>>>   		ret = -ENODATA;
>>>   		break;
>>> @@ -438,7 +451,7 @@ static int lm25066_probe(struct i2c_client *client,
>>>   	info->func[0] = PMBUS_HAVE_VIN | PMBUS_HAVE_VMON
>>>   	  | PMBUS_HAVE_PIN | PMBUS_HAVE_IIN | PMBUS_HAVE_STATUS_INPUT
>>> -	  | PMBUS_HAVE_TEMP | PMBUS_HAVE_STATUS_TEMP;
>>> +	  | PMBUS_HAVE_TEMP | PMBUS_HAVE_STATUS_TEMP | PMBUS_HAVE_SAMPLES;
>>>   	if (data->id == lm25056) {
>>>   		info->func[0] |= PMBUS_HAVE_STATUS_VMON;
>>>
>>
> 
> Krzysztof
>