Message ID | 20240204103710.19212-1-dima.fedrau@gmail.com (mailing list archive) |
---|---|
State | Changes Requested |
Headers | show |
Series | [v2] iio: humidity: hdc3020: add threshold events support | expand |
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
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
On Sun, 4 Feb 2024 11:37:10 +0100 Dimitri Fedrau <dima.fedrau@gmail.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. 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@gmail.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. > +#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. As I mention below, I'm unconvinced that it makes sense to handle these as pairs. > +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. > + } > + } 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. > + 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). > + 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. > + 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); > + > + 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. > + 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. > + 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 > + default: > + return -EOPNOTSUPP; > + } > + > + return IIO_VAL_FRACTIONAL; Drop this as will have returned above. > +} > + > +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. > + > + 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. > + > + 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). > + * 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 > + 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.... > + /* 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... > + */ > + 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. > + 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. > + 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,
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@gmail.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@gmail.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, >
> > > 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. > > > +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. Jonathan
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
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 +#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 }; +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); + } + } 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; +} + +static int _hdc3020_read_thresh(struct hdc3020_data *data, + 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) + 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); + + 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); + 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; + 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; + default: + return -EOPNOTSUPP; + } + + return IIO_VAL_FRACTIONAL; +} + +static int hdc3020_clear_status(struct hdc3020_data *data) +{ + return hdc3020_write_bytes(data, HDC3020_S_STATUS, 2); +} + +static int hdc3020_read_status(struct hdc3020_data *data, u16 *stat) +{ + u8 crc, buf[3]; + int ret; + + ret = hdc3020_read_bytes(data, HDC3020_R_STATUS, buf, 3); + if (ret < 0) + return ret; + + /* CRC check of the status */ + crc = crc8(hdc3020_crc8_table, buf, 2, CRC8_INIT_VALUE); + if (crc != buf[2]) + return -EINVAL; + + *stat = get_unaligned_be16(buf); + + return 0; +} + +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; + + 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; + + 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 (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 + * 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); + 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) { + /* The alert output is activated by default upon power up, hardware + * reset, and soft reset. Clear the status register before enabling + * the interrupt. + */ + 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 | + IRQF_ONESHOT, + dev_id->name, indio_dev); + 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,
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 Signed-off-by: Dimitri Fedrau <dima.fedrau@gmail.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(+)