Message-ID: <20240205070421.GA2264419@debian>
Date: Mon, 5 Feb 2024 08:04:21 +0100
From: Dimitri Fedrau <dima.fedrau@...il.com>
To: Jonathan Cameron <jic23@...nel.org>
Cc: Javier Carrasco <javier.carrasco.cruz@...il.com>,
	Li peiyu <579lpy@...il.com>, Lars-Peter Clausen <lars@...afoo.de>,
	linux-iio@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2] iio: humidity: hdc3020: add threshold events support

Am Sun, Feb 04, 2024 at 02:43:47PM +0000 schrieb Jonathan Cameron:
> On Sun,  4 Feb 2024 11:37:10 +0100
> Dimitri Fedrau <dima.fedrau@...il.com> wrote:
> 
> > Add threshold events support for temperature and relative humidity. To
> > enable them the higher and lower threshold registers must be programmed
> > and the higher threshold must be greater then or equal to the lower
> > threshold. Otherwise the event is disabled. Invalid hysteresis values
> > are ignored by the device. There is no further configuration possible.
> > 
> > Tested by setting thresholds/hysteresis and turning the heater on/off.
> > Used iio_event_monitor in tools/iio to catch events while constantly
> > displaying temperature and humidity values.
> > Threshold and hysteresis values are cached in the driver, used i2c-tools
> > to read the threshold and hysteresis values from the device and make
> > sure cached values are consistent to values written to the device.
> > 
> > Based on Fix:
> > a69eeaad093d "iio: humidity: hdc3020: fix temperature offset" in branch
> > fixes-togreg
> Move this below the ---
> as we don't want to keep that info in the log long term.
> 
Ok.

> It's good to have it for now though as helps me know when I can apply the patch.
> 
> > 
> > Signed-off-by: Dimitri Fedrau <dima.fedrau@...il.com>
> > ---
> > Changes in V2:
> >   - Fix alphabetical order of includes(Christophe)
> >   - Fix typo: change varibale name "HDC3020_R_R_RH_THRESH_LOW_CLR" to
> >     HDC3020_R_T_RH_THRESH_LOW_CLR to match variable name pattern
> >     (Christophe)
> >   - Add constants HDC3020_MIN_TEMP and HDC3020_MAX_TEMP for min/max
> >     threshold inputs (Christophe)
> >   - Change HDC3020_MIN_TEMP to -40, as stated in the datasheet(Javier)
> >   - Fix shadowing of global variable "hdc3020_id" in probe function,
> >     rename variable in probe function to "dev_id"(Javier)
> > ---
> >  drivers/iio/humidity/hdc3020.c | 342 +++++++++++++++++++++++++++++++++
> >  1 file changed, 342 insertions(+)
> > 
> > diff --git a/drivers/iio/humidity/hdc3020.c b/drivers/iio/humidity/hdc3020.c
> > index ed70415512f6..80cfb343c92d 100644
> > --- a/drivers/iio/humidity/hdc3020.c
> > +++ b/drivers/iio/humidity/hdc3020.c
> > @@ -14,11 +14,13 @@
> >  #include <linux/delay.h>
> >  #include <linux/i2c.h>
> >  #include <linux/init.h>
> > +#include <linux/interrupt.h>
> >  #include <linux/module.h>
> >  #include <linux/mutex.h>
> >  
> >  #include <asm/unaligned.h>
> >  
> > +#include <linux/iio/events.h>
> >  #include <linux/iio/iio.h>
> >  
> >  #define HDC3020_HEATER_CMD_MSB		0x30 /* shared by all heater commands */
> > @@ -26,21 +28,47 @@
> >  #define HDC3020_HEATER_DISABLE		0x66
> >  #define HDC3020_HEATER_CONFIG		0x6E
> >  
> > +#define HDC3020_THRESH_TEMP_MASK	GENMASK(8, 0)
> > +#define HDC3020_THRESH_TEMP_SHIFT	7
> 
> This is odd.  Normally a mask and shift pair like this is about
> a register field.  Here that's not true.  So don't use the common
> naming and instead use something like TRUNCATED_BITS.
> Define that for both fields, then use
> FIELD_PREP() to set them, even though that will meant shifting
> the humidity values down and up again by the same amount.
>
Could have done better with the naming. Will fix this.

> 
> 
> > +#define HDC3020_THRESH_HUM_MASK		GENMASK(15, 9)
> > +
> > +#define HDC3020_STATUS_T_LOW_ALERT	BIT(6)
> > +#define HDC3020_STATUS_T_HIGH_ALERT	BIT(7)
> > +#define HDC3020_STATUS_RH_LOW_ALERT	BIT(8)
> > +#define HDC3020_STATUS_RH_HIGH_ALERT	BIT(9)
> > +
> >  #define HDC3020_READ_RETRY_TIMES	10
> >  #define HDC3020_BUSY_DELAY_MS		10
> >  
> >  #define HDC3020_CRC8_POLYNOMIAL		0x31
> >  
> > +#define HDC3020_MIN_TEMP		-40
> > +#define HDC3020_MAX_TEMP		125
> > +
> >  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.

> 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.
> 
> > +static const u8 HDC3020_S_T_RH_THRESH_LOW_CLR[2] = { 0x61, 0x0B };
> > +static const u8 HDC3020_S_T_RH_THRESH_HIGH_CLR[2] = { 0x61, 0x16 };
> > +static const u8 HDC3020_S_T_RH_THRESH_HIGH[2] = { 0x61, 0x1D };
> > +
> >  static const u8 HDC3020_R_T_RH_AUTO[2] = { 0xE0, 0x00 };
> >  static const u8 HDC3020_R_T_LOW_AUTO[2] = { 0xE0, 0x02 };
> >  static const u8 HDC3020_R_T_HIGH_AUTO[2] = { 0xE0, 0x03 };
> >  static const u8 HDC3020_R_RH_LOW_AUTO[2] = { 0xE0, 0x04 };
> >  static const u8 HDC3020_R_RH_HIGH_AUTO[2] = { 0xE0, 0x05 };
> >  
> > +static const u8 HDC3020_R_T_RH_THRESH_LOW[2] = { 0xE1, 0x02 };
> > +static const u8 HDC3020_R_T_RH_THRESH_LOW_CLR[2] = { 0xE1, 0x09 };
> > +static const u8 HDC3020_R_T_RH_THRESH_HIGH_CLR[2] = { 0xE1, 0x14 };
> > +static const u8 HDC3020_R_T_RH_THRESH_HIGH[2] = { 0xE1, 0x1F };
> > +
> > +static const u8 HDC3020_R_STATUS[2] = { 0xF3, 0x2D };
> > +
> >  struct hdc3020_data {
> >  	struct i2c_client *client;
> >  	/*
> > @@ -50,22 +78,54 @@ struct hdc3020_data {
> >  	 * if the device does not respond).
> >  	 */
> >  	struct mutex lock;
> > +	/*
> > +	 * Temperature and humidity thresholds are packed together into a single
> > +	 * 16 bit value. Each threshold is represented by a truncated 16 bit
> > +	 * value. The 7 MSBs of a relative humidity threshold are concatenated
> > +	 * with the 9 MSBs of a temperature threshold, where the temperature
> > +	 * threshold resides in the 7 LSBs. Due to the truncated representation,
> > +	 * there is a resolution loss of 0.5 degree celsius in temperature and a
> > +	 * 1% resolution loss in relative humidity.
> > +	 */
> > +	u16 t_rh_thresh_low;
> > +	u16 t_rh_thresh_high;
> > +	u16 t_rh_thresh_low_clr;
> > +	u16 t_rh_thresh_high_clr;
> >  };
> >  
> >  static const int hdc3020_heater_vals[] = {0, 1, 0x3FFF};
> >  
> > +static const struct iio_event_spec hdc3020_t_rh_event[] = {
> > +	{
> > +		.type = IIO_EV_TYPE_THRESH,
> > +		.dir = IIO_EV_DIR_RISING,
> > +		.mask_separate = BIT(IIO_EV_INFO_VALUE) |
> > +		BIT(IIO_EV_INFO_HYSTERESIS),
> > +	},
> > +	{
> > +		.type = IIO_EV_TYPE_THRESH,
> > +		.dir = IIO_EV_DIR_FALLING,
> > +		.mask_separate = BIT(IIO_EV_INFO_VALUE) |
> > +		BIT(IIO_EV_INFO_HYSTERESIS),
> > +	},
> > +};
> > +
> >  static const struct iio_chan_spec hdc3020_channels[] = {
> >  	{
> >  		.type = IIO_TEMP,
> >  		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
> >  		BIT(IIO_CHAN_INFO_SCALE) | BIT(IIO_CHAN_INFO_PEAK) |
> >  		BIT(IIO_CHAN_INFO_TROUGH) | BIT(IIO_CHAN_INFO_OFFSET),
> > +		.event_spec = hdc3020_t_rh_event,
> > +		.num_event_specs = ARRAY_SIZE(hdc3020_t_rh_event),
> >  	},
> >  	{
> >  		.type = IIO_HUMIDITYRELATIVE,
> >  		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
> >  		BIT(IIO_CHAN_INFO_SCALE) | BIT(IIO_CHAN_INFO_PEAK) |
> >  		BIT(IIO_CHAN_INFO_TROUGH),
> > +		.event_spec = hdc3020_t_rh_event,
> > +		.num_event_specs = ARRAY_SIZE(hdc3020_t_rh_event),
> >  	},
> >  	{
> >  		/*
> > @@ -389,10 +449,241 @@ static int hdc3020_write_raw(struct iio_dev *indio_dev,
> >  	return -EINVAL;
> >  }
> >  
> > +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;
> > +
> > +	/* 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);
> First value of buf is always 0x61 - so just set that above
> u8 buf[5] = { 0x61, };
> and here just write buf[1] with appropriate value.
> 
Will fix it.
> > +		}
> > +	} 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);
> 
> This almost looks like FIELD_PREP() but is really a division then a store?
> Perhaps rename TEMP_SHIFT TEMP_TRUNCATED_BITS or something like that to avoid
> the currently confusing naming.
> 
The comment is misleading. Calculation of the temperature threshold goes first
and then the value is truncated. Will fix this.

> > +		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;
> > +}
> > +
> > +static int _hdc3020_read_thresh(struct hdc3020_data *data,
> 
> Relationship of this function to the following one not clear from
> naming.  Rename it to make the two different cases easier to follow.
> Mind you, threshold checking isn't usually a fast path - so
> it's unusual to cache the thresholds in the driver explicitly
> (as opposed to getting them cached for free via regmap which doesn't
> add driver complexity).
>
It is left from a previous version which I didn't submit. Will fix the
naming if I need the function in the next version. I probably remove the
caching, as you already explained it adds complexity and isn't in the
fast path.

> 
> > +				enum iio_event_info info,
> > +				enum iio_event_direction dir, u16 *thresh)
> > +{
> > +	u8 crc, buf[3];
> > +	const u8 *cmd;
> > +	int ret;
> > +
> > +	if (info == IIO_EV_INFO_VALUE) {
> > +		if (dir == IIO_EV_DIR_RISING)
> 
> As you only use these in the initial readback, maybe just pass the
> cmd directly into each call.  No need to use general interfaces.
> 
Ok.
> > +			cmd = HDC3020_R_T_RH_THRESH_HIGH;
> > +		else
> > +			cmd = HDC3020_R_T_RH_THRESH_LOW;
> > +	} else {
> > +		if (dir == IIO_EV_DIR_RISING)
> > +			cmd = HDC3020_R_T_RH_THRESH_HIGH_CLR;
> > +		else
> > +			cmd = HDC3020_R_T_RH_THRESH_LOW_CLR;
> > +	}
> > +
> > +	ret = hdc3020_read_bytes(data, cmd, buf, 3);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	/* CRC check of the threshold */
> > +	crc = crc8(hdc3020_crc8_table, buf, 2, CRC8_INIT_VALUE);
> > +	if (crc != buf[2])
> > +		return -EINVAL;
> 
> > +
> > +	*thresh = get_unaligned_be16(buf);
> 
> This 3 byte read, crc check and be16 extraction is common in this diver
> maybe - just add a helper function for this rather than adding
> yet another copy of the same code?
> 
> int hdc3020_read_be16_reg(struct iio_dev *indio_dev, u8 cmd)
> {...
> 
> 	return get_unaligned_be16(buf);
> 
You are right. Will also fix this for the existing code.

> > +
> > +	return 0;
> > +}
> > +
> > +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.

> 
> > +	switch (chan->type) {
> > +	case IIO_TEMP:
> > +		/* Get the truncated temperature threshold from 9 LSBs,
> > +		 * shift them and calculate the threshold according to the
> > +		 * formula in the datasheet.
> > +		 */
> > +		*val = ((*thresh) & HDC3020_THRESH_TEMP_MASK) <<
> > +			HDC3020_THRESH_TEMP_SHIFT;
> > +		*val = -2949075 + (175 * (*val));
> > +		*val2 = 65535;
> > +		break;
> 		return here.  Gives easier to review code as no need for
> a reader to check if anything else happens in this path.
> 
Ok.

> > +	case IIO_HUMIDITYRELATIVE:
> > +		/* Get the truncated humidity threshold from 7 MSBs, and
> > +		 * calculate the threshold according to the formula in the
> > +		 * datasheet.
> > +		 */
> > +		*val = 100 * ((*thresh) & HDC3020_THRESH_HUM_MASK);
> > +		*val2 = 65535;
> > +		break;
> return here
Ok.

> > +	default:
> > +		return -EOPNOTSUPP;
> > +	}
> > +
> > +	return IIO_VAL_FRACTIONAL;
> Drop this as will have returned above.
Ok.

> > +}
> 
> > +
> > +static irqreturn_t hdc3020_interrupt_handler(int irq, void *private)
> > +{
> > +	struct iio_dev *indio_dev = private;
> > +	struct hdc3020_data *data;
> > +	u16 stat;
> > +	int ret;
> > +
> > +	data = iio_priv(indio_dev);
> > +	ret = hdc3020_read_status(data, &stat);
> > +	if (ret)
> > +		return IRQ_NONE;
> Hmm. In cases where we get a read back failure on an interrupt it is always
> messy to decide if it's spurious or not.  If this actually happens you may
> find you want to return IRQ_HANDLED; even though it wasn't.
Will fix this, thanks.

> > +
> > +	if (!(stat & (HDC3020_STATUS_T_HIGH_ALERT | HDC3020_STATUS_T_LOW_ALERT |
> > +		HDC3020_STATUS_RH_HIGH_ALERT | HDC3020_STATUS_RH_LOW_ALERT)))
> > +		return IRQ_NONE;
> 
> This one is fine as you know it is spurious.
> 
> > +
> > +	if (stat & HDC3020_STATUS_T_HIGH_ALERT)
> > +		iio_push_event(indio_dev,
> > +			       IIO_MOD_EVENT_CODE(IIO_TEMP, 0,
> > +						  IIO_NO_MOD,
> > +						  IIO_EV_TYPE_THRESH,
> > +						  IIO_EV_DIR_RISING),
> > +						  iio_get_time_ns(indio_dev));
> If you happen to get more than one, you probably want the timestamp to match.
> I'd take the timestamp first into a local variable then use it in each of these.
> 
Will fix this.

> > +
> > +	if (stat & HDC3020_STATUS_T_LOW_ALERT)
> > +		iio_push_event(indio_dev,
> > +			       IIO_MOD_EVENT_CODE(IIO_TEMP, 0,
> > +						  IIO_NO_MOD,
> > +						  IIO_EV_TYPE_THRESH,
> > +						  IIO_EV_DIR_FALLING),
> > +						  iio_get_time_ns(indio_dev));
> > +
> > +	if (stat & HDC3020_STATUS_RH_HIGH_ALERT)
> > +		iio_push_event(indio_dev,
> > +			       IIO_MOD_EVENT_CODE(IIO_HUMIDITYRELATIVE, 0,
> > +						  IIO_NO_MOD,
> > +						  IIO_EV_TYPE_THRESH,
> > +						  IIO_EV_DIR_RISING),
> > +						  iio_get_time_ns(indio_dev));
> > +
> > +	if (stat & HDC3020_STATUS_RH_LOW_ALERT)
> > +		iio_push_event(indio_dev,
> > +			       IIO_MOD_EVENT_CODE(IIO_HUMIDITYRELATIVE, 0,
> > +						  IIO_NO_MOD,
> > +						  IIO_EV_TYPE_THRESH,
> > +						  IIO_EV_DIR_FALLING),
> > +						  iio_get_time_ns(indio_dev));
> > +
> > +	return IRQ_HANDLED;
> > +}
> > +
> >  static const struct iio_info hdc3020_info = {
> >  	.read_raw = hdc3020_read_raw,
> >  	.write_raw = hdc3020_write_raw,
> >  	.read_avail = hdc3020_read_available,
> > +	.read_event_value = hdc3020_read_thresh,
> > +	.write_event_value = hdc3020_write_thresh,
> >  };
> >  
> >  static void hdc3020_stop(void *data)
> > @@ -402,6 +693,7 @@ static void hdc3020_stop(void *data)
> >  
> >  static int hdc3020_probe(struct i2c_client *client)
> >  {
> > +	const struct i2c_device_id *dev_id;
> >  	struct iio_dev *indio_dev;
> >  	struct hdc3020_data *data;
> >  	int ret;
> > @@ -413,6 +705,8 @@ static int hdc3020_probe(struct i2c_client *client)
> >  	if (!indio_dev)
> >  		return -ENOMEM;
> >  
> > +	dev_id = i2c_client_get_device_id(client);
> > +
> >  	data = iio_priv(indio_dev);
> >  	data->client = client;
> >  	mutex_init(&data->lock);
> > @@ -425,6 +719,54 @@ static int hdc3020_probe(struct i2c_client *client)
> >  	indio_dev->channels = hdc3020_channels;
> >  	indio_dev->num_channels = ARRAY_SIZE(hdc3020_channels);
> >  
> > +	/* Read out upper and lower thresholds and hysteresis, which can be the
> 
> As below - comment syntax wrong for IIO drivers (only net and a few other
> corners of the kernel prefer this one for historical reasons).
>
Will fix this.

> > +	 * default values or values programmed into non-volatile memory.
> > +	 */
> > +	ret = _hdc3020_read_thresh(data, IIO_EV_INFO_VALUE, IIO_EV_DIR_FALLING,
> > +				   &data->t_rh_thresh_low);
> As above, it feels to me like you can just use the registers directly into
> a be16 readback function.
> 
> 	ret = hdc3020_read_be16_reg(indio_dev, HDC3020_R_T_RH_THRESH_LOW)
> 	if (ret < 0)
> 		return ...
> 
> 	data->t_rh_thresh_low = ret;
> etc
> 
Will fix this.

> > +	if (ret)
> > +		return dev_err_probe(&client->dev, ret,
> > +				     "Unable to get low thresholds\n");
> > +
> > +	ret = _hdc3020_read_thresh(data, IIO_EV_INFO_VALUE, IIO_EV_DIR_RISING,
> > +				   &data->t_rh_thresh_high);
> > +	if (ret)
> > +		return dev_err_probe(&client->dev, ret,
> > +				     "Unable to get high thresholds\n");
> > +
> > +	ret = _hdc3020_read_thresh(data, IIO_EV_INFO_HYSTERESIS,
> > +				   IIO_EV_DIR_FALLING,
> > +				   &data->t_rh_thresh_low_clr);
> > +	if (ret)
> > +		return dev_err_probe(&client->dev, ret,
> > +				     "Unable to get low hysteresis\n");
> > +
> > +	ret = _hdc3020_read_thresh(data, IIO_EV_INFO_HYSTERESIS,
> > +				   IIO_EV_DIR_RISING,
> > +				   &data->t_rh_thresh_high_clr);
> > +	if (ret)
> > +		return dev_err_probe(&client->dev, ret,
> > +				     "Unable to get high hysteresis\n");
> > +
> > +	if (client->irq) {
> Comment syntax in IIO (and most of the kernel) is
> 		/*
> 		 * The alert....
> 
Will fix this.

> > +		/* The alert output is activated by default upon power up, hardware
> > +		 * reset, and soft reset. Clear the status register before enabling
> > +		 * the interrupt.
> That's a bit nasty. Ah well.  Ordering not critical though as you are registering
> a rising trigger.  However...
Will fix this. It makes more sense to clear the interrupt after registering the
interrupt handler.

> > +		 */
> > +		ret = hdc3020_clear_status(data);
> > +		if (ret)
> > +			return ret;
> > +
> > +		ret = devm_request_threaded_irq(&client->dev, client->irq,
> > +						NULL, hdc3020_interrupt_handler,
> > +						IRQF_TRIGGER_RISING |
> 
> These days (we got this wrong a lot in the past) we tend to leave the interrupt
> polarity to the firmware to configure (DT or similar) as people have an annoying
> habit of using not gates in interrupt wiring.   So Just pass IRQF_ONESHOT but
> make sure the DT binding example sets this right.
> 
Will fix this, thanks.

> > +						IRQF_ONESHOT,
> > +						dev_id->name, indio_dev);
> 
> dev_id->name is annoyingly unstable depending on the probe path and whether
> the various firmware match tables align perfectly with the i2c_device_id
> table. I'd just use a fixed value here for the driver in question and
> not worry about it too much.  hdc3020 is fine.  If you really care about
> it add a device specific structure and put a string for the name in there.
> That can then be referenced from all the firmware tables (safely) and
> i2c_get_match_data() used to get it from which ever one is available.
> 
Will stick to the fixed value "hdc3020". Thanks for the explanation,
didn't know it. :)
> > +		if (ret)
> > +			return dev_err_probe(&client->dev, ret,
> > +					     "Failed to request IRQ\n");
> > +	}
> > +
> >  	ret = hdc3020_write_bytes(data, HDC3020_S_AUTO_10HZ_MOD0, 2);
> >  	if (ret)
> >  		return dev_err_probe(&client->dev, ret,
> 

Powered by blists - more mailing lists