Received-SPF: pass (google.com: domain of linux-kernel+bounces-51639-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Date: Sun, 4 Feb 2024 13:38:27 +0100
From: Dimitri Fedrau <dima.fedrau@gmail.com>
To: Javier Carrasco <javier.carrasco.cruz@gmail.com>
Cc: Li peiyu <579lpy@gmail.com>, Jonathan Cameron <jic23@kernel.org>,
	Lars-Peter Clausen <lars@metafoo.de>, linux-iio@vger.kernel.org,
	linux-kernel@vger.kernel.org
Subject: Re: [PATCH v2] iio: humidity: hdc3020: add threshold events support
Message-ID: <20240204123827.GA19525@debian>
References: <20240204103710.19212-1-dima.fedrau@gmail.com>
 <194ed4ce-bb2e-404e-a716-ec2e9876c740@gmail.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <194ed4ce-bb2e-404e-a716-ec2e9876c740@gmail.com>

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