Re: [PATCH v3 1/2] iio: humidity: Add driver for ti HDC302x humidity sensors

[Jonathan Cameron <Jonathan.Cameron@Huawei.com>]

On Thu, 30 Nov 2023 19:59:03 +0100
Javier Carrasco <javier.carrasco.cruz@gmail.com> 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
>