Re: [PATCH 2/3] lm25066: export sysfs attribute for SAMPLES_FOR_AVG

["Adamski, Krzysztof (Nokia - PL/Wroclaw)" <krzysztof.adamski@nokia.com>]

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