Re: [PATCH v2] iio: humidity: hdc3020: add threshold events support

[Dimitri Fedrau]

Am Mon, Feb 05, 2024 at 09:33:49AM +0000 schrieb Jonathan Cameron:
> > > >  static const u8 HDC3020_S_AUTO_10HZ_MOD0[2] = { 0x27, 0x37 };
> > > >  
> > > > +static const u8 HDC3020_S_STATUS[2] = { 0x30, 0x41 };
> > > > +
> > > >  static const u8 HDC3020_EXIT_AUTO[2] = { 0x30, 0x93 };
> > > >  
> > > > +static const u8 HDC3020_S_T_RH_THRESH_LOW[2] = { 0x61, 0x00 };  
> > > 
> > > Ah. missed this in original driver, but this use of capitals for
> > > non #defines is really confusing and we should aim to clean that
> > > up.
> > >  
> > Could use small letters instead.
> 
> That would avoid any confusion.
> 
> > 
> > > As I mention below, I'm unconvinced that it makes sense to handle
> > > these as pairs.
> > >  
> > For the threshold I could convert it as it is for the heater registers:
> > 
> > #define HDC3020_S_T_RH_THRESH_MSB	0x61
> > #define HDC3020_S_T_RH_THRESH_LOW	0x00
> > #define HDC3020_S_T_RH_THRESH_LOW_CLR	0x0B
> > #define HDC3020_S_T_RH_THRESH_HIGH_CLR	0x16
> > #define HDC3020_S_T_RH_THRESH_HIGH	0x1D
> > 
> > #define HDC3020_R_T_RH_THRESH_MSB	0xE1
> > #define HDC3020_R_T_RH_THRESH_LOW	0x02
> > #define HDC3020_R_T_RH_THRESH_LOW_CLR	0x09
> > #define HDC3020_R_T_RH_THRESH_HIGH_CLR	0x14
> > #define HDC3020_R_T_RH_THRESH_HIGH	0x1F
> > 
> > or:
> > 
> > #define HDC3020_S_T_RH_THRESH_LOW       0x6100
> > #define HDC3020_S_T_RH_THRESH_LOW_CLR   0x610B
> > #define HDC3020_S_T_RH_THRESH_HIGH_CLR  0x6116
> > #define HDC3020_S_T_RH_THRESH_HIGH      0x611D
> > 
> > #define HDC3020_R_T_RH_THRESH_LOW       0x6102
> > #define HDC3020_R_T_RH_THRESH_LOW_CLR   0x6109
> > #define HDC3020_R_T_RH_THRESH_HIGH_CLR  0x6114
> > #define HDC3020_R_T_RH_THRESH_HIGH      0x611F
> > 
> > I don't know if it's a good idea, as we would need to make sure it is
> > big endian in the buffer. Probably with a function that handles this.
> I think this is the best plan with a
> put_unaligned_be16() to deal with the endianness.
> The compiler should be able to optimize that heavily.
>
I think that would require some refactoring. I would add patches that
are fixing this. Have there been reasons for using the pairs ? I'm just
curious.
> 
> > > > +static int hdc3020_read_thresh(struct iio_dev *indio_dev,
> > > > +			       const struct iio_chan_spec *chan,
> > > > +			       enum iio_event_type type,
> > > > +			       enum iio_event_direction dir,
> > > > +			       enum iio_event_info info,
> > > > +			       int *val, int *val2)
> > > > +{
> > > > +	struct hdc3020_data *data = iio_priv(indio_dev);
> > > > +	u16 *thresh;
> > > > +
> > > > +	/* Select threshold */
> > > > +	if (info == IIO_EV_INFO_VALUE) {
> > > > +		if (dir == IIO_EV_DIR_RISING)
> > > > +			thresh = &data->t_rh_thresh_high;
> > > > +		else
> > > > +			thresh = &data->t_rh_thresh_low;
> > > > +	} else {
> > > > +		if (dir == IIO_EV_DIR_RISING)
> > > > +			thresh = &data->t_rh_thresh_high_clr;
> > > > +		else
> > > > +			thresh = &data->t_rh_thresh_low_clr;
> > > > +	}
> > > > +
> > > > +	guard(mutex)(&data->lock);  
> > > 
> > > Why take the lock here?
> > > 
> > > you are relying on a single value that is already cached.
> > >  
> > A single threshold value is used for humidity and temperature values. I
> > didn't see a lock in "iio_ev_value_show", so there might be some
> > concurrent access triggered by "in_temp_thresh_rising_value" and
> > "in_humidityrelative_thresh_rising_value" sysfs files which is not
> > secured by a mutex or similiar.
> 
> Unless you going to get value tearing (very unlikely and lots of the
> kernel assumes that won't happen - more of a theoretical possibility
> that we don't want compilers to do!) this just protects against a race
> where you read one and write the other.  That doesn't really help us
> as it just moves the race to which one gets the lock first.
> 
Yes, it's very unlikely to happen. Anyway, I'm dropping the support for
the caching and with it this function.

Dimitri