On Thu, 30 Nov 2023 19:59:03 +0100
Javier Carrasco <javier.carrasco.cruz@xxxxxxxxx> wrote:
> 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).
Agreed this isn't that intuitive.
>
> 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.
I wonder if we should have the ABI state that peak_scale is only applicable
it it overrides the _scale value. Here I think they are the same anyway
thus not providing peak_scale would leave us with a single attribute reflecting
scale of _raw, _peak_raw and _trough_raw
I think this is already the case in reality. We have two users of the peak interface
and only one of them provides peak_scale. Hopefully hdc2010 is
assuming _scale applies to it.
So maybe this is just a documentation update and drop peak_scale from this
driver.
Jonathan
>
> Thank you and best regards,
> Javier Carrasco
>
