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
> > >
> > > > 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.
Not that I can think of. Maybe how they are represented on the
dataheet? Often people just copy that stuff without thinking
about it (I know I've been guilty of this ;)
Jonathan
Am Sat, Feb 10, 2024 at 04:11:17PM +0000 schrieb Jonathan Cameron:
> > > > > [...]
> > > > #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.
>
> Not that I can think of. Maybe how they are represented on the
> dataheet? Often people just copy that stuff without thinking
> about it (I know I've been guilty of this ;)
>
Ok, just wanted to make sure I didn't miss anything. Yes, probably the
command table drives one into that direction.
Dimitri