2024-02-04 11:27:19

by Javier Carrasco

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

Hi Dimitri,

On 04.02.24 11:37, Dimitri Fedrau wrote:
>
> +static int hdc3020_write_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;
> + u8 buf[5];
> + int ret;
> +
I tested your patch right now and I noticed that you are only writing
integer values, which means that for example 19.9999 turns into 18.9169.
It seems that you are not using val2 (the decimal part).

Is there any reason for that? The displayed thresholds are decimal, though.
> + /* Supported temperature range is from –40 to 125 degree celsius */
> + if (val < HDC3020_MIN_TEMP || val > HDC3020_MAX_TEMP)
> + return -EINVAL;
> +
> + /* Select threshold and associated register */
> + if (info == IIO_EV_INFO_VALUE) {
> + if (dir == IIO_EV_DIR_RISING) {
> + thresh = &data->t_rh_thresh_high;
> + memcpy(buf, HDC3020_S_T_RH_THRESH_HIGH, 2);
> + } else {
> + thresh = &data->t_rh_thresh_low;
> + memcpy(buf, HDC3020_S_T_RH_THRESH_LOW, 2);
> + }
> + } else {
> + if (dir == IIO_EV_DIR_RISING) {
> + thresh = &data->t_rh_thresh_high_clr;
> + memcpy(buf, HDC3020_S_T_RH_THRESH_HIGH_CLR, 2);
> + } else {
> + thresh = &data->t_rh_thresh_low_clr;
> + memcpy(buf, HDC3020_S_T_RH_THRESH_LOW_CLR, 2);
> + }
> + }
> +
> + guard(mutex)(&data->lock);
> + switch (chan->type) {
> + case IIO_TEMP:
> + /*
> + * Store truncated temperature threshold into 9 LSBs while
> + * keeping the old humidity threshold in the 7 MSBs.
> + */
> + val = (((val + 45) * 65535 / 175) >> HDC3020_THRESH_TEMP_SHIFT);
> + val &= HDC3020_THRESH_TEMP_MASK;
> + val |= (*thresh & HDC3020_THRESH_HUM_MASK);
> + break;
> + case IIO_HUMIDITYRELATIVE:
> + /*
> + * Store truncated humidity threshold into 7 MSBs while
> + * keeping the old temperature threshold in the 9 LSBs.
> + */
> + val = ((val * 65535 / 100) & HDC3020_THRESH_HUM_MASK);
> + val |= (*thresh & HDC3020_THRESH_TEMP_MASK);
> + break;
> + default:
> + return -EOPNOTSUPP;
> + }
> +
> + put_unaligned_be16(val, &buf[2]);
> + buf[4] = crc8(hdc3020_crc8_table, buf + 2, 2, CRC8_INIT_VALUE);
> + ret = hdc3020_write_bytes(data, buf, 5);
> + if (ret)
> + return ret;
> +
> + /* Update threshold */
> + *thresh = val;
> +
> + return 0;
> +}

Best regards,
Javier Carrasco

2024-02-04 12:38:43

by Dimitri Fedrau

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

Am Sun, Feb 04, 2024 at 12:26:53PM +0100 schrieb Javier Carrasco:
> Hi Dimitri,
>
Hi Javier,
> On 04.02.24 11:37, Dimitri Fedrau wrote:
> >
> > +static int hdc3020_write_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;
> > + u8 buf[5];
> > + int ret;
> > +
> I tested your patch right now and I noticed that you are only writing
> integer values, which means that for example 19.9999 turns into 18.9169.
> It seems that you are not using val2 (the decimal part).
>
> Is there any reason for that? The displayed thresholds are decimal, though.
thanks for testing. I wanted to allow only integers because of the
truncated threshold values.(Still there is the resolution loss) On the other
side I missed to return integers. I think you are right, have to evaluate
val2. It's better to be as accurate as possible. I will fix this.

> > + /* Supported temperature range is from –40 to 125 degree celsius */
> > + if (val < HDC3020_MIN_TEMP || val > HDC3020_MAX_TEMP)
> > + return -EINVAL;
> > +
> > + /* Select threshold and associated register */
> > + if (info == IIO_EV_INFO_VALUE) {
> > + if (dir == IIO_EV_DIR_RISING) {
> > + thresh = &data->t_rh_thresh_high;
> > + memcpy(buf, HDC3020_S_T_RH_THRESH_HIGH, 2);
> > + } else {
> > + thresh = &data->t_rh_thresh_low;
> > + memcpy(buf, HDC3020_S_T_RH_THRESH_LOW, 2);
> > + }
> > + } else {
> > + if (dir == IIO_EV_DIR_RISING) {
> > + thresh = &data->t_rh_thresh_high_clr;
> > + memcpy(buf, HDC3020_S_T_RH_THRESH_HIGH_CLR, 2);
> > + } else {
> > + thresh = &data->t_rh_thresh_low_clr;
> > + memcpy(buf, HDC3020_S_T_RH_THRESH_LOW_CLR, 2);
> > + }
> > + }
> > +
> > + guard(mutex)(&data->lock);
> > + switch (chan->type) {
> > + case IIO_TEMP:
> > + /*
> > + * Store truncated temperature threshold into 9 LSBs while
> > + * keeping the old humidity threshold in the 7 MSBs.
> > + */
> > + val = (((val + 45) * 65535 / 175) >> HDC3020_THRESH_TEMP_SHIFT);
> > + val &= HDC3020_THRESH_TEMP_MASK;
> > + val |= (*thresh & HDC3020_THRESH_HUM_MASK);
> > + break;
> > + case IIO_HUMIDITYRELATIVE:
> > + /*
> > + * Store truncated humidity threshold into 7 MSBs while
> > + * keeping the old temperature threshold in the 9 LSBs.
> > + */
> > + val = ((val * 65535 / 100) & HDC3020_THRESH_HUM_MASK);
> > + val |= (*thresh & HDC3020_THRESH_TEMP_MASK);
> > + break;
> > + default:
> > + return -EOPNOTSUPP;
> > + }
> > +
> > + put_unaligned_be16(val, &buf[2]);
> > + buf[4] = crc8(hdc3020_crc8_table, buf + 2, 2, CRC8_INIT_VALUE);
> > + ret = hdc3020_write_bytes(data, buf, 5);
> > + if (ret)
> > + return ret;
> > +
> > + /* Update threshold */
> > + *thresh = val;
> > +
> > + return 0;
> > +}
>
> Best regards,
> Javier Carrasco

Best regards,
Dimitri Fedrau