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