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

[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.


> > > +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.

Jonathan