Re: [PATCH] adm1275: support PMBUS_VIRT_*_SAMPLES

From: Adamski, Krzysztof (Nokia - PL/Wroclaw)
Date: Wed May 29 2019 - 03:14:35 EST

On Tue, May 28, 2019 at 12:46:52PM -0700, Guenter Roeck wrote:
>On Fri, May 24, 2019 at 12:49:13PM +0000, Adamski, Krzysztof (Nokia - PL/Wroclaw) wrote:
>> The device supports setting the number of samples for averaging the
>> measurements. There are two separate settings - PWR_AVG for averaging
>> PIN and VI_AVG for averaging VIN/VAUX/IOUT, both being part of
>> PMON_CONFIG register. The values are stored as exponent of base 2 of the
>> actual number of samples that will be taken.
>>
>> Signed-off-by: Krzysztof Adamski <krzysztof.adamski@xxxxxxxxx>
>> ---
>> drivers/hwmon/pmbus/adm1275.c | 68 ++++++++++++++++++++++++++++++++++-
>> 1 file changed, 67 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/hwmon/pmbus/adm1275.c b/drivers/hwmon/pmbus/adm1275.c
>> index f569372c9204..4efe1a9df563 100644
>> --- a/drivers/hwmon/pmbus/adm1275.c
>> +++ b/drivers/hwmon/pmbus/adm1275.c
>> @@ -23,6 +23,8 @@
>> #include <linux/slab.h>
>> #include <linux/i2c.h>
>> #include <linux/bitops.h>
>> +#include <linux/bitfield.h>
>> +#include <linux/log2.h>
>> #include "pmbus.h"
>>
>> enum chips { adm1075, adm1272, adm1275, adm1276, adm1278, adm1293, adm1294 };
>> @@ -78,6 +80,10 @@ enum chips { adm1075, adm1272, adm1275, adm1276, adm1278, adm1293, adm1294 };
>> #define ADM1075_VAUX_OV_WARN BIT(7)
>> #define ADM1075_VAUX_UV_WARN BIT(6)
>>
>> +#define ADM1275_PWR_AVG_MASK GENMASK(13, 11)
>> +#define ADM1275_VI_AVG_MASK GENMASK(10, 8)
>> +#define ADM1275_SAMPLES_AVG_MAX 128
>> +
>> struct adm1275_data {
>> int id;
>> bool have_oc_fault;
>> @@ -90,6 +96,7 @@ struct adm1275_data {
>> bool have_pin_max;
>> bool have_temp_max;
>> struct pmbus_driver_info info;
>> + struct mutex lock;
>> };
>>
>> #define to_adm1275_data(x) container_of(x, struct adm1275_data, info)
>> @@ -164,6 +171,38 @@ static const struct coefficients adm1293_coefficients[] = {
>> [18] = { 7658, 0, -3 }, /* power, 21V, irange200 */
>> };
>>
>> +static inline int adm1275_read_pmon_config(struct i2c_client *client, u64 mask)
>
>Why is the mask passed through as u64 ?

Good point. I used u64 as this is the type used by bitfield machinery
under the hood but I agree it doesn't make sense and is even confusing
to have this in the function prototype as we are using this to mask 16
bit word anyways. I will fix that in v2. I am gonna have to cast the ret
to u16 when passing to FIELD_GET() to make sure the __BF_FIELD_CHECK is
not complaining (since it is signed right now), though.

>
>> +{
>> + int ret;
>> +
>> + ret = i2c_smbus_read_word_data(client, ADM1275_PMON_CONFIG);
>> + if (ret < 0)
>> + return ret;
>> +
>> + return FIELD_GET(mask, ret);
>> +}
>> +
>> +static inline int adm1275_write_pmon_config(struct i2c_client *client, u64 mask,
>> + u16 word)
>> +{
>> + const struct pmbus_driver_info *info = pmbus_get_driver_info(client);
>> + struct adm1275_data *data = to_adm1275_data(info);
>> + int ret;
>> +
>> + mutex_lock(&data->lock);
>
>Why is another lock on top of the lock provided by the pmbus core required ?
>

Good point, I was considering if I should instead add mutex_lock on
update_lock in the pmbus_set_samples() function inside of pmbus_core.c
instead (as this function is missing it) but figured that not all
devices will need that (lm25066 didn't) so it might be a waste in most
cases. But this may be cleaner approach indeed.

Is this what you mean or there is some other lock I missed?

>> + ret = i2c_smbus_read_word_data(client, ADM1275_PMON_CONFIG);
>> + if (ret < 0) {
>> + mutex_unlock(&data->lock);
>> + return ret;
>> + }
>> +
>> + word = FIELD_PREP(mask, word) | (ret & ~mask);
>> + ret = i2c_smbus_write_word_data(client, ADM1275_PMON_CONFIG, word);
>> + mutex_unlock(&data->lock);
>> +
>> + return ret;
>> +}
>> +
>> static int adm1275_read_word_data(struct i2c_client *client, int page, int reg)
>> {
>> const struct pmbus_driver_info *info = pmbus_get_driver_info(client);
>> @@ -242,6 +281,19 @@ static int adm1275_read_word_data(struct i2c_client *client, int page, int reg)
>> if (!data->have_temp_max)
>> return -ENXIO;
>> break;
>> + case PMBUS_VIRT_POWER_SAMPLES:
>> + ret = adm1275_read_pmon_config(client, ADM1275_PWR_AVG_MASK);
>> + if (ret < 0)
>> + break;
>> + ret = 1 << ret;
>
> ret = BIT(ret);
>

I intentionally used the "raw" left shift to make it more obvious this
is pow2 arithmetic operation and an direct inverse to the ilog2() used
on write counterpart. This is also consistent with what I used in
lm25066.c driver not long time ago.

I don't have strong preference but this is my reasoning. So do you still
think it is better to use BIT() macro instead?

>> + break;
>> + case PMBUS_VIRT_IN_SAMPLES:
>> + case PMBUS_VIRT_CURR_SAMPLES:
>> + ret = adm1275_read_pmon_config(client, ADM1275_VI_AVG_MASK);
>> + if (ret < 0)
>> + break;
>> + ret = 1 << ret;
>
> ret = BIT(ret);
>
>> + break;
>> default:
>> ret = -ENODATA;
>> break;
>> @@ -286,6 +338,17 @@ static int adm1275_write_word_data(struct i2c_client *client, int page, int reg,
>> case PMBUS_VIRT_RESET_TEMP_HISTORY:
>> ret = pmbus_write_word_data(client, 0, ADM1278_PEAK_TEMP, 0);
>> break;
>> + case PMBUS_VIRT_POWER_SAMPLES:
>> + word = clamp_val(word, 1, ADM1275_SAMPLES_AVG_MAX);
>> + ret = adm1275_write_pmon_config(client, ADM1275_PWR_AVG_MASK,
>> + ilog2(word));
>> + break;
>> + case PMBUS_VIRT_IN_SAMPLES:
>> + case PMBUS_VIRT_CURR_SAMPLES:
>> + word = clamp_val(word, 1, ADM1275_SAMPLES_AVG_MAX);
>> + ret = adm1275_write_pmon_config(client, ADM1275_VI_AVG_MASK,
>> + ilog2(word));
>> + break;
>> default:
>> ret = -ENODATA;
>> break;
>> @@ -422,6 +485,8 @@ static int adm1275_probe(struct i2c_client *client,
>> if (!data)
>> return -ENOMEM;
>>
>> + mutex_init(&data->lock);
>> +
>> if (of_property_read_u32(client->dev.of_node,
>> "shunt-resistor-micro-ohms", &shunt))
>> shunt = 1000; /* 1 mOhm if not set via DT */
>> @@ -439,7 +504,8 @@ static int adm1275_probe(struct i2c_client *client,
>> info->format[PSC_CURRENT_OUT] = direct;
>> info->format[PSC_POWER] = direct;
>> info->format[PSC_TEMPERATURE] = direct;
>> - info->func[0] = PMBUS_HAVE_IOUT | PMBUS_HAVE_STATUS_IOUT;
>> + info->func[0] = PMBUS_HAVE_IOUT | PMBUS_HAVE_STATUS_IOUT |
>> + PMBUS_HAVE_SAMPLES;
>>
>> info->read_word_data = adm1275_read_word_data;
>> info->read_byte_data = adm1275_read_byte_data;
>> --
>> 2.20.1
>>