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