Hi,
On 25.11.23 15:52, Jonathan Cameron wrote:
>> +
>> +static const struct iio_chan_spec hdc3020_channels[] = {
>> + {
>> + .type = IIO_TEMP,
>
> There is only one temp channel so I'd like to see the peaks added to this
> one as well. Can be done if we add a new bit of ABI for the min value
> seen.
>
> Whilst naming .index = 0, .channel = 0 is different from this case
> the ABI and all userspace software should treat them the same hence this
> is an ambiguous channel specification.
>
>> + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
>> + BIT(IIO_CHAN_INFO_SCALE),
>> + },
>> + {
>> + /* For minimum value during measurement */
>
> Please add some docs for this - preferably in patch description
> or cover letter if it is too long for there. You are using the ABI in a fashion
> not previously considered.
>
> I don't think it is a good solution. Perhaps keeping IIO_CHAN_INFO_PEAK
> as assumed to be maximum, we could add a new IIO_CHAN_INFO_TROUGH
> perhaps? Hopefully the scale applies to both peak and trough so we
> don't need separate attributes.
>
If only IIO_CHAN_INFO_TROUGH is added without an additional _SCALE, in
this particular case you end up having the following sysfs entries:
in_humidityrelative_peak_raw
in_humidityrelative_peak_scale
in_temp_peak_raw
in_temp_peak_scale
in_humidityrelative_trough_raw
in_temp_trough_raw
I just would like to know if documenting the trough attribute in a way
that it is clear that the peak_scale applies for it as well is better
than adding a TROUGH_SCALE. We would save the additional attribute, but
at first sight it is not that obvious (it makes sense that the scale is
the same for both peaks, but the names are not so consistent anymore).
I suppose that often the raw and peak scales are also the same, but
there are indeed two separate attributes. On the other hand I don't know
if the additional attribute would imply bigger issues (maintenance,
documentation, etc) than just adding the line, so I leave the question open.
Thank you and best regards,
Javier Carrasco
