| Message ID | e231af1feac9cbe8b72923a13587035220b76ad8.1554934898.git.krzysztof.adamski@nokia.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | pmbus: extend configurability via sysfs | expand |
On 4/10/19 3:39 PM, Adamski, Krzysztof (Nokia - PL/Wroclaw) wrote: > Register custom sysfs attribute to be registered by pmbus allowing > read/write access to the manufacturer specific SAMPLES_FOR_AVG register. > This 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 | 45 +++++++++++++++++++++++++++++++++++ > 1 file changed, 45 insertions(+) > > diff --git a/drivers/hwmon/pmbus/lm25066.c b/drivers/hwmon/pmbus/lm25066.c > index 53db78753a0d..c78af0a7e5ff 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 }; > @@ -38,6 +39,7 @@ enum chips { lm25056, lm25066, lm5064, lm5066, lm5066i }; > #define LM25066_READ_PIN_PEAK 0xd5 > #define LM25066_CLEAR_PIN_PEAK 0xd6 > #define LM25066_DEVICE_SETUP 0xd9 > +#define LM25066_SAMPLES_FOR_AVG 0xdb > #define LM25066_READ_AVG_VIN 0xdc > #define LM25066_READ_AVG_VOUT 0xdd > #define LM25066_READ_AVG_IIN 0xde > @@ -405,6 +407,47 @@ static int lm25066_write_word_data(struct i2c_client *client, int page, int reg, > return ret; > } > > +static ssize_t samples_for_avg_show(struct device *dev, > + struct device_attribute *attr, char *buf) > +{ > + struct i2c_client *client = to_i2c_client(dev->parent); > + int ret; > + > + ret = pmbus_read_byte_data(client, 0, LM25066_SAMPLES_FOR_AVG); > + if (ret < 0) > + return ret; > + > + return sprintf(buf, "%d\n", 1 << ret); > +} > + > +static ssize_t samples_for_avg_store(struct device *dev, > + struct device_attribute *attr, > + const char *buf, size_t count) > +{ > + struct i2c_client *client = to_i2c_client(dev->parent); > + int ret, val; > + > + ret = kstrtoint(buf, 0, &val); > + if (ret < 0) > + return ret; > + > + ret = pmbus_write_byte_data(client, 0, LM25066_SAMPLES_FOR_AVG, > + ilog2(val)); > + if (ret < 0) > + return ret; > + > + return count; > +} > + > +static DEVICE_ATTR_RW(samples_for_avg); > + > +static struct attribute *lm25056_attrs[] = { > + &dev_attr_samples_for_avg.attr, > + NULL, > +}; > +ATTRIBUTE_GROUPS(lm25056); // should we set a name of this group to put all > + // those attributes in subdirectory? Like "custom" ? > + We don't add subdirectories for other chips, and we won't start it here. I don't mind the attribute itself, but I do mind its name. We'll have to find something more generic, such as 'num_samples' or just 'samples'. I am open to suggestions. We'll also have to decide if the attribute(s) should be limited to one per chip, or if there can be multiple attributes. For example, MAX34462 has separate values for iout_samples and adc_average. Do we want samples, {curr,in,power,...}_samples, or both ? Or even currX_samples ? Either case, this is needed for more than one pmbus chip (and I know it is needed for some non-PMBus chips). I am inclined to add it as generic attribute into the pmbus core instead (and possibly add it to the ABI). Thanks, Guenter > static int lm25066_probe(struct i2c_client *client, > const struct i2c_device_id *id) > { > @@ -476,6 +519,8 @@ static int lm25066_probe(struct i2c_client *client, > info->b[PSC_POWER] = coeff[PSC_POWER].b; > } > > + info->groups = lm25056_groups; > + > return pmbus_do_probe(client, id, info); > } > >
On Wed, Apr 10, 2019 at 05:55:19PM -0700, Guenter Roeck wrote: > > +static ssize_t samples_for_avg_show(struct device *dev, > > + struct device_attribute *attr, char *buf) > > +{ > > + struct i2c_client *client = to_i2c_client(dev->parent); > > + int ret; > > + > > + ret = pmbus_read_byte_data(client, 0, LM25066_SAMPLES_FOR_AVG); > > + if (ret < 0) > > + return ret; > > + > > + return sprintf(buf, "%d\n", 1 << ret); > > +} > > + > > +static ssize_t samples_for_avg_store(struct device *dev, > > + struct device_attribute *attr, > > + const char *buf, size_t count) > > +{ > > + struct i2c_client *client = to_i2c_client(dev->parent); > > + int ret, val; > > + > > + ret = kstrtoint(buf, 0, &val); > > + if (ret < 0) > > + return ret; > > + > > + ret = pmbus_write_byte_data(client, 0, LM25066_SAMPLES_FOR_AVG, > > + ilog2(val)); > > + if (ret < 0) > > + return ret; > > + > > + return count; > > +} > > + > > +static DEVICE_ATTR_RW(samples_for_avg); > > + > > +static struct attribute *lm25056_attrs[] = { > > + &dev_attr_samples_for_avg.attr, > > + NULL, > > +}; > > +ATTRIBUTE_GROUPS(lm25056); // should we set a name of this group to put all > > + // those attributes in subdirectory? Like "custom" ? > > + > We don't add subdirectories for other chips, and we won't start it here. > > I don't mind the attribute itself, but I do mind its name. We'll have > to find something more generic, such as 'num_samples' or just 'samples'. > I am open to suggestions. We'll also have to decide if the attribute(s) > should be limited to one per chip, or if there can be multiple attributes. > For example, MAX34462 has separate values for iout_samples and adc_average. > Do we want samples, {curr,in,power,...}_samples, or both ? Or even > currX_samples ? For my use case -- TI's INA chips, there is only one "samples" configuration being used for all currX_inputs and inX_inputs. So having a "samples" node would certainly fit better than an in0_samples. So I vote for having both. Thank you Nicolin
On Wed, Apr 10, 2019 at 09:24:29PM -0700, Nicolin Chen wrote: >On Wed, Apr 10, 2019 at 05:55:19PM -0700, Guenter Roeck wrote: > >> > +static ssize_t samples_for_avg_show(struct device *dev, >> > + struct device_attribute *attr, char *buf) >> > +{ >> > + struct i2c_client *client = to_i2c_client(dev->parent); >> > + int ret; >> > + >> > + ret = pmbus_read_byte_data(client, 0, LM25066_SAMPLES_FOR_AVG); >> > + if (ret < 0) >> > + return ret; >> > + >> > + return sprintf(buf, "%d\n", 1 << ret); >> > +} >> > + >> > +static ssize_t samples_for_avg_store(struct device *dev, >> > + struct device_attribute *attr, >> > + const char *buf, size_t count) >> > +{ >> > + struct i2c_client *client = to_i2c_client(dev->parent); >> > + int ret, val; >> > + >> > + ret = kstrtoint(buf, 0, &val); >> > + if (ret < 0) >> > + return ret; >> > + >> > + ret = pmbus_write_byte_data(client, 0, LM25066_SAMPLES_FOR_AVG, >> > + ilog2(val)); >> > + if (ret < 0) >> > + return ret; >> > + >> > + return count; >> > +} >> > + >> > +static DEVICE_ATTR_RW(samples_for_avg); >> > + >> > +static struct attribute *lm25056_attrs[] = { >> > + &dev_attr_samples_for_avg.attr, >> > + NULL, >> > +}; >> > +ATTRIBUTE_GROUPS(lm25056); // should we set a name of this group to put all >> > + // those attributes in subdirectory? Like "custom" ? >> > + >> We don't add subdirectories for other chips, and we won't start it here. >> >> I don't mind the attribute itself, but I do mind its name. We'll have >> to find something more generic, such as 'num_samples' or just 'samples'. >> I am open to suggestions. We'll also have to decide if the attribute(s) >> should be limited to one per chip, or if there can be multiple attributes. >> For example, MAX34462 has separate values for iout_samples and adc_average. >> Do we want samples, {curr,in,power,...}_samples, or both ? Or even >> currX_samples ? > >For my use case -- TI's INA chips, there is only one "samples" >configuration being used for all currX_inputs and inX_inputs. >So having a "samples" node would certainly fit better than an >in0_samples. So I vote for having both. The name is family specific. The data sheet calls this register exactly like that so I used this name. But I agree that we could standardise on some common name, even if the actual implementation will be device-specific. The LM5064 has one value for all readings, ADM1293 would have one setting for PIN and separate one for VIN/VAUX/IOUT. So maybe it makes sense to allow for some device specific naming (with prefixes) where it makes sense but default to "samples" in simple case of shared value? In this case, if there is specific value like "curr_samples", user knows exactly which readings are influenced but when using "samples" it needs to have device specific knowledge to understand this. I'm just not sure what would be the best way to express situation for adm1293 for example - the PIN case is simple but what to do with "shared" settings - expose both curr_samples/in_samples and writing to one would change the other as well? Or just default to "samples" for this case and have separate "power_samples" just for PIN? Krzysztof
On 4/11/19 1:09 AM, Adamski, Krzysztof (Nokia - PL/Wroclaw) wrote: > On Wed, Apr 10, 2019 at 09:24:29PM -0700, Nicolin Chen wrote: >> On Wed, Apr 10, 2019 at 05:55:19PM -0700, Guenter Roeck wrote: >> >>>> +static ssize_t samples_for_avg_show(struct device *dev, >>>> + struct device_attribute *attr, char *buf) >>>> +{ >>>> + struct i2c_client *client = to_i2c_client(dev->parent); >>>> + int ret; >>>> + >>>> + ret = pmbus_read_byte_data(client, 0, LM25066_SAMPLES_FOR_AVG); >>>> + if (ret < 0) >>>> + return ret; >>>> + >>>> + return sprintf(buf, "%d\n", 1 << ret); >>>> +} >>>> + >>>> +static ssize_t samples_for_avg_store(struct device *dev, >>>> + struct device_attribute *attr, >>>> + const char *buf, size_t count) >>>> +{ >>>> + struct i2c_client *client = to_i2c_client(dev->parent); >>>> + int ret, val; >>>> + >>>> + ret = kstrtoint(buf, 0, &val); >>>> + if (ret < 0) >>>> + return ret; >>>> + >>>> + ret = pmbus_write_byte_data(client, 0, LM25066_SAMPLES_FOR_AVG, >>>> + ilog2(val)); >>>> + if (ret < 0) >>>> + return ret; >>>> + >>>> + return count; >>>> +} >>>> + >>>> +static DEVICE_ATTR_RW(samples_for_avg); >>>> + >>>> +static struct attribute *lm25056_attrs[] = { >>>> + &dev_attr_samples_for_avg.attr, >>>> + NULL, >>>> +}; >>>> +ATTRIBUTE_GROUPS(lm25056); // should we set a name of this group to put all >>>> + // those attributes in subdirectory? Like "custom" ? >>>> + >>> We don't add subdirectories for other chips, and we won't start it here. >>> >>> I don't mind the attribute itself, but I do mind its name. We'll have >>> to find something more generic, such as 'num_samples' or just 'samples'. >>> I am open to suggestions. We'll also have to decide if the attribute(s) >>> should be limited to one per chip, or if there can be multiple attributes. >>> For example, MAX34462 has separate values for iout_samples and adc_average. >>> Do we want samples, {curr,in,power,...}_samples, or both ? Or even >>> currX_samples ? >> >> For my use case -- TI's INA chips, there is only one "samples" >> configuration being used for all currX_inputs and inX_inputs. >> So having a "samples" node would certainly fit better than an >> in0_samples. So I vote for having both. > > The name is family specific. The data sheet calls this register exactly > like that so I used this name. But I agree that we could standardise on Well, yes, but the whole point of an ABI is to make it chip independent. > some common name, even if the actual implementation will be > device-specific. > > The LM5064 has one value for all readings, ADM1293 would have one > setting for PIN and separate one for VIN/VAUX/IOUT. > > So maybe it makes sense to allow for some device specific naming (with > prefixes) where it makes sense but default to "samples" in simple case > of shared value? In this case, if there is specific value like > "curr_samples", user knows exactly which readings are influenced but > when using "samples" it needs to have device specific knowledge to > understand this. > Let's go for "samples" and {in,curr,power,temp,...}_samples. "samples" should be used if the attribute applies to all sensors. > I'm just not sure what would be the best way to express situation for > adm1293 for example - the PIN case is simple but what to do with > "shared" settings - expose both curr_samples/in_samples and writing to > one would change the other as well? Or just default to "samples" for > this case and have separate "power_samples" just for PIN? > Both "samples" and "power_samples" at the same time would be confusing. Common implementation in situations like this is to have both curr_samples and in_samples, and changing one would also change the other (or only one would be writable, but that is just an implementation detail). So what we need is virtual registers (PMBUS_VIRT_SAMPLES, PMBUS_VIRT_IN_SAMPLES, and so on), plus the necessary code in pmbus_core.c and the implementation in the chip driver. We'll also need to document new ABI attributes (samples, in_samples, temp_samples, ...). Any takers ? Nicolin, I think with that you can move forward with the TI INA chip implementation. I agree that 'samples' would be most appropriate for this chip. Thanks, Guenter
On Thu, Apr 11, 2019 at 06:07:47AM -0700, Guenter Roeck wrote: >On 4/11/19 1:09 AM, Adamski, Krzysztof (Nokia - PL/Wroclaw) wrote: >>On Wed, Apr 10, 2019 at 09:24:29PM -0700, Nicolin Chen wrote: >>>On Wed, Apr 10, 2019 at 05:55:19PM -0700, Guenter Roeck wrote: >>> >>>>>+static ssize_t samples_for_avg_show(struct device *dev, >>>>>+ struct device_attribute *attr, char *buf) >>>>>+{ >>>>>+ struct i2c_client *client = to_i2c_client(dev->parent); >>>>>+ int ret; >>>>>+ >>>>>+ ret = pmbus_read_byte_data(client, 0, LM25066_SAMPLES_FOR_AVG); >>>>>+ if (ret < 0) >>>>>+ return ret; >>>>>+ >>>>>+ return sprintf(buf, "%d\n", 1 << ret); >>>>>+} >>>>>+ >>>>>+static ssize_t samples_for_avg_store(struct device *dev, >>>>>+ struct device_attribute *attr, >>>>>+ const char *buf, size_t count) >>>>>+{ >>>>>+ struct i2c_client *client = to_i2c_client(dev->parent); >>>>>+ int ret, val; >>>>>+ >>>>>+ ret = kstrtoint(buf, 0, &val); >>>>>+ if (ret < 0) >>>>>+ return ret; >>>>>+ >>>>>+ ret = pmbus_write_byte_data(client, 0, LM25066_SAMPLES_FOR_AVG, >>>>>+ ilog2(val)); >>>>>+ if (ret < 0) >>>>>+ return ret; >>>>>+ >>>>>+ return count; >>>>>+} >>>>>+ >>>>>+static DEVICE_ATTR_RW(samples_for_avg); >>>>>+ >>>>>+static struct attribute *lm25056_attrs[] = { >>>>>+ &dev_attr_samples_for_avg.attr, >>>>>+ NULL, >>>>>+}; >>>>>+ATTRIBUTE_GROUPS(lm25056); // should we set a name of this group to put all >>>>>+ // those attributes in subdirectory? Like "custom" ? >>>>>+ >>>>We don't add subdirectories for other chips, and we won't start it here. >>>> >>>>I don't mind the attribute itself, but I do mind its name. We'll have >>>>to find something more generic, such as 'num_samples' or just 'samples'. >>>>I am open to suggestions. We'll also have to decide if the attribute(s) >>>>should be limited to one per chip, or if there can be multiple attributes. >>>>For example, MAX34462 has separate values for iout_samples and adc_average. >>>>Do we want samples, {curr,in,power,...}_samples, or both ? Or even >>>>currX_samples ? >>> >>>For my use case -- TI's INA chips, there is only one "samples" >>>configuration being used for all currX_inputs and inX_inputs. >>>So having a "samples" node would certainly fit better than an >>>in0_samples. So I vote for having both. >> >>The name is family specific. The data sheet calls this register exactly >>like that so I used this name. But I agree that we could standardise on > >Well, yes, but the whole point of an ABI is to make it chip independent. > >>some common name, even if the actual implementation will be >>device-specific. >> >>The LM5064 has one value for all readings, ADM1293 would have one >>setting for PIN and separate one for VIN/VAUX/IOUT. >> >>So maybe it makes sense to allow for some device specific naming (with >>prefixes) where it makes sense but default to "samples" in simple case >>of shared value? In this case, if there is specific value like >>"curr_samples", user knows exactly which readings are influenced but >>when using "samples" it needs to have device specific knowledge to >>understand this. >> >Let's go for "samples" and {in,curr,power,temp,...}_samples. "samples" >should be used if the attribute applies to all sensors. > >>I'm just not sure what would be the best way to express situation for >>adm1293 for example - the PIN case is simple but what to do with >>"shared" settings - expose both curr_samples/in_samples and writing to >>one would change the other as well? Or just default to "samples" for >>this case and have separate "power_samples" just for PIN? >> >Both "samples" and "power_samples" at the same time would be confusing. >Common implementation in situations like this is to have both curr_samples >and in_samples, and changing one would also change the other (or only one >would be writable, but that is just an implementation detail). > >So what we need is virtual registers (PMBUS_VIRT_SAMPLES, PMBUS_VIRT_IN_SAMPLES, >and so on), plus the necessary code in pmbus_core.c and the implementation >in the chip driver. We'll also need to document new ABI attributes (samples, >in_samples, temp_samples, ...). > >Any takers ? > >Nicolin, I think with that you can move forward with the TI INA chip >implementation. I agree that 'samples' would be most appropriate for >this chip. I will try implementing this the way you suggested and resubmit this soon. Krzysztof
diff --git a/drivers/hwmon/pmbus/lm25066.c b/drivers/hwmon/pmbus/lm25066.c index 53db78753a0d..c78af0a7e5ff 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 }; @@ -38,6 +39,7 @@ enum chips { lm25056, lm25066, lm5064, lm5066, lm5066i }; #define LM25066_READ_PIN_PEAK 0xd5 #define LM25066_CLEAR_PIN_PEAK 0xd6 #define LM25066_DEVICE_SETUP 0xd9 +#define LM25066_SAMPLES_FOR_AVG 0xdb #define LM25066_READ_AVG_VIN 0xdc #define LM25066_READ_AVG_VOUT 0xdd #define LM25066_READ_AVG_IIN 0xde @@ -405,6 +407,47 @@ static int lm25066_write_word_data(struct i2c_client *client, int page, int reg, return ret; } +static ssize_t samples_for_avg_show(struct device *dev, + struct device_attribute *attr, char *buf) +{ + struct i2c_client *client = to_i2c_client(dev->parent); + int ret; + + ret = pmbus_read_byte_data(client, 0, LM25066_SAMPLES_FOR_AVG); + if (ret < 0) + return ret; + + return sprintf(buf, "%d\n", 1 << ret); +} + +static ssize_t samples_for_avg_store(struct device *dev, + struct device_attribute *attr, + const char *buf, size_t count) +{ + struct i2c_client *client = to_i2c_client(dev->parent); + int ret, val; + + ret = kstrtoint(buf, 0, &val); + if (ret < 0) + return ret; + + ret = pmbus_write_byte_data(client, 0, LM25066_SAMPLES_FOR_AVG, + ilog2(val)); + if (ret < 0) + return ret; + + return count; +} + +static DEVICE_ATTR_RW(samples_for_avg); + +static struct attribute *lm25056_attrs[] = { + &dev_attr_samples_for_avg.attr, + NULL, +}; +ATTRIBUTE_GROUPS(lm25056); // should we set a name of this group to put all + // those attributes in subdirectory? Like "custom" ? + static int lm25066_probe(struct i2c_client *client, const struct i2c_device_id *id) { @@ -476,6 +519,8 @@ static int lm25066_probe(struct i2c_client *client, info->b[PSC_POWER] = coeff[PSC_POWER].b; } + info->groups = lm25056_groups; + return pmbus_do_probe(client, id, info); }
Register custom sysfs attribute to be registered by pmbus allowing read/write access to the manufacturer specific SAMPLES_FOR_AVG register. This 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 | 45 +++++++++++++++++++++++++++++++++++ 1 file changed, 45 insertions(+)