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

[Dimitri Fedrau]

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