On Sun, 18 Feb 2024 16:18:26 +1030 Subhajit Ghosh <subhajit.ghosh@xxxxxxxxxxxxxx> wrote: > Driver support for Avago (Broadcom) APDS9306 Ambient Light Sensor. > It has two channels - ALS and CLEAR. The ALS (Ambient Light Sensor) > channel approximates the response of the human-eye providing direct > read out where the output count is proportional to ambient light levels. > It is internally temperature compensated and rejects 50Hz and 60Hz flicker > caused by artificial light sources. Hardware interrupt configuration is > optional. It is a low power device with 20 bit resolution and has > configurable adaptive interrupt mode and interrupt persistence mode. > The device also features inbuilt hardware gain, multiple integration time > selection options and sampling frequency selection options. > > This driver also uses the IIO GTS (Gain Time Scale) Helpers Namespace for > Scales, Gains and Integration time implementation. > > Signed-off-by: Subhajit Ghosh <subhajit.ghosh@xxxxxxxxxxxxxx> I applied this but then got some build warnings that made me look more closely at the int_src handling. This is confusing because of the less than helpful datasheet defintion of a 2 bit register that takes values 0 and 1 only. I thought about trying to fix this up whilst applying but the event code issue is too significant to do without a means to test it. Jonathan > diff --git a/drivers/iio/light/apds9306.c b/drivers/iio/light/apds9306.c > new file mode 100644 > index 000000000000..c18e0d48556b > --- /dev/null > +++ b/drivers/iio/light/apds9306.c > @@ -0,0 +1,1336 @@ > .... > + > +static struct iio_event_spec apds9306_event_spec_als[] = { > + { > + .type = IIO_EV_TYPE_THRESH, > + .dir = IIO_EV_DIR_RISING, > + .mask_shared_by_all = BIT(IIO_EV_INFO_VALUE), > + }, { > + .type = IIO_EV_TYPE_THRESH, > + .dir = IIO_EV_DIR_FALLING, > + .mask_shared_by_all = BIT(IIO_EV_INFO_VALUE), > + }, { > + .type = IIO_EV_TYPE_THRESH, > + .dir = IIO_EV_DIR_EITHER, > + .mask_shared_by_all = BIT(IIO_EV_INFO_PERIOD), > + .mask_separate = BIT(IIO_EV_INFO_ENABLE), > + }, { > + .type = IIO_EV_TYPE_THRESH_ADAPTIVE, > + .mask_shared_by_all = BIT(IIO_EV_INFO_VALUE) | > + BIT(IIO_EV_INFO_ENABLE), > + }, > +}; > + > +static struct iio_event_spec apds9306_event_spec_clear[] = { > + { > + .type = IIO_EV_TYPE_THRESH, Specify dir. > + .mask_separate = BIT(IIO_EV_INFO_ENABLE), > + }, > +}; > + > +#define APDS9306_CHANNEL(_type) \ I think the code would be easier to read without this macro. It will be a few lines longer but simpler > + .type = _type, \ > + .info_mask_shared_by_all = BIT(IIO_CHAN_INFO_INT_TIME) | \ > + BIT(IIO_CHAN_INFO_SAMP_FREQ), \ > + .info_mask_shared_by_all_available = BIT(IIO_CHAN_INFO_INT_TIME) | \ > + BIT(IIO_CHAN_INFO_SAMP_FREQ), \ > + > +static struct iio_chan_spec apds9306_channels_with_events[] = { > + { > + APDS9306_CHANNEL(IIO_LIGHT) > + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | > + BIT(IIO_CHAN_INFO_SCALE), > + .info_mask_separate_available = BIT(IIO_CHAN_INFO_SCALE), > + .event_spec = apds9306_event_spec_als, > + .num_event_specs = ARRAY_SIZE(apds9306_event_spec_als), > + }, { > + APDS9306_CHANNEL(IIO_INTENSITY) > + .channel2 = IIO_MOD_LIGHT_CLEAR, > + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW), > + .modified = 1, > + .event_spec = apds9306_event_spec_clear, > + .num_event_specs = ARRAY_SIZE(apds9306_event_spec_clear), > + }, > +}; > + > +static struct iio_chan_spec apds9306_channels_without_events[] = { > + { > + APDS9306_CHANNEL(IIO_LIGHT) > + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | > + BIT(IIO_CHAN_INFO_SCALE), > + .info_mask_separate_available = BIT(IIO_CHAN_INFO_SCALE), > + }, { > + APDS9306_CHANNEL(IIO_INTENSITY) > + .channel2 = IIO_MOD_LIGHT_CLEAR, > + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW), > + .modified = 1, > + }, > +}; > + > +/* INT_PERSISTENCE available */ > +IIO_CONST_ATTR(thresh_either_period_available, "[0 1 15]"); > + > +/* ALS_THRESH_VAR available */ > +IIO_CONST_ATTR(thresh_adaptive_either_values_available, "[0 1 7]"); These need to be static > + > +static struct attribute *apds9306_event_attributes[] = { > + &iio_const_attr_thresh_either_period_available.dev_attr.attr, > + &iio_const_attr_thresh_adaptive_either_values_available.dev_attr.attr, > + NULL > +}; > +static int apds9306_read_data(struct apds9306_data *data, int *val, int reg) > +{ > + struct device *dev = data->dev; > + struct iio_dev *indio_dev = dev_to_iio_dev(dev); > + struct apds9306_regfields *rf = &data->rf; > + int ret, delay, intg_time, intg_time_idx, repeat_rate_idx, int_src; > + int status = 0; > + u8 buff[3]; > + > + ret = pm_runtime_resume_and_get(data->dev); > + if (ret) > + return ret; > + > + ret = regmap_field_read(rf->intg_time, &intg_time_idx); > + if (ret) > + return ret; > + > + ret = regmap_field_read(rf->repeat_rate, &repeat_rate_idx); > + if (ret) > + return ret; > + > + ret = regmap_field_read(rf->int_src, &int_src); > + if (ret) > + return ret; > + > + intg_time = iio_gts_find_int_time_by_sel(&data->gts, intg_time_idx); > + if (intg_time < 0) > + return intg_time; > + > + /* Whichever is greater - integration time period or sampling period. */ > + delay = max(intg_time, apds9306_repeat_rate_period[repeat_rate_idx]); > + > + /* > + * Clear stale data flag that might have been set by the interrupt > + * handler if it got data available flag set in the status reg. > + */ > + data->read_data_available = 0; > + > + /* > + * If this function runs parallel with the interrupt handler, either > + * this reads and clears the status registers or the interrupt handler > + * does. The interrupt handler sets a flag for read data available > + * in our private structure which we read here. > + */ > + ret = regmap_read_poll_timeout(data->regmap, APDS9306_MAIN_STATUS_REG, > + status, data->read_data_available || > + (status & (APDS9306_ALS_DATA_STAT_MASK | > + APDS9306_ALS_INT_STAT_MASK)), > + APDS9306_ALS_READ_DATA_DELAY_US, delay * 2); > + > + if (ret) > + return ret; > + > + /* If we reach here before the interrupt handler we push an event */ > + if ((status & APDS9306_ALS_INT_STAT_MASK)) > + iio_push_event(indio_dev, IIO_UNMOD_EVENT_CODE(IIO_LIGHT, > + int_src, IIO_EV_TYPE_THRESH, IIO_EV_DIR_EITHER), You are pushing an event on channel 0 or 1 (which is non obvious as that int_src is a 2 bit value again). However you don't use indexed channels so this is wrong. It's also pushing IIO_LIGHT for both channels which makes no sense as you only have one IIO_LIGHT channel. > + iio_get_time_ns(indio_dev)); > + > + ret = regmap_bulk_read(data->regmap, reg, buff, sizeof(buff)); > + if (ret) { > + dev_err_ratelimited(dev, "read data failed\n"); > + return ret; > + } > + > + *val = get_unaligned_le24(&buff); > + > + pm_runtime_mark_last_busy(data->dev); > + pm_runtime_put_autosuspend(data->dev); > + > + return 0; > +} > + ... > + > +static irqreturn_t apds9306_irq_handler(int irq, void *priv) > +{ > + struct iio_dev *indio_dev = priv; > + struct apds9306_data *data = iio_priv(indio_dev); > + struct apds9306_regfields *rf = &data->rf; > + int ret, status, int_ch; > + > + /* > + * The interrupt line is released and the interrupt flag is > + * cleared as a result of reading the status register. All the > + * status flags are cleared as a result of this read. > + */ > + ret = regmap_read(data->regmap, APDS9306_MAIN_STATUS_REG, &status); > + if (ret < 0) { > + dev_err_ratelimited(data->dev, "status reg read failed\n"); > + return IRQ_HANDLED; > + } > + > + ret = regmap_field_read(rf->int_src, &int_ch); > + if (ret) > + return ret; > + > + if ((status & APDS9306_ALS_INT_STAT_MASK)) > + iio_push_event(indio_dev, IIO_UNMOD_EVENT_CODE(IIO_LIGHT, > + int_ch, IIO_EV_TYPE_THRESH, IIO_EV_DIR_EITHER), > + iio_get_time_ns(indio_dev)); As commented on elsewhere I'm not seeing the relationship between the event pushed here and the channels this device provides (one of which is modified for starters). > + > + /* > + * If a one-shot read through sysfs is underway at the same time > + * as this interrupt handler is executing and a read data available > + * flag was set, this flag is set to inform read_poll_timeout() > + * to exit. > + */ > + if ((status & APDS9306_ALS_DATA_STAT_MASK)) > + data->read_data_available = 1; > + > + return IRQ_HANDLED; > +} ... > +static int apds9306_read_event_config(struct iio_dev *indio_dev, > + const struct iio_chan_spec *chan, > + enum iio_event_type type, > + enum iio_event_direction dir) > +{ > + struct apds9306_data *data = iio_priv(indio_dev); > + struct apds9306_regfields *rf = &data->rf; > + int int_en, event_ch_is_light, ret; > + > + switch (type) { > + case IIO_EV_TYPE_THRESH: > + guard(mutex)(&data->mutex); > + > + ret = regmap_field_read(rf->int_src, &event_ch_is_light); Call the local value int_src - it's not obvious to a reviewer what relationship between that and int_src is. I had to go read the datasheet to find out. > + if (ret) > + return ret; > + > + ret = regmap_field_read(rf->int_en, &int_en); > + if (ret) > + return ret; > + > + if (chan->type == IIO_LIGHT) > + return int_en & event_ch_is_light; > + > + if (chan->type == IIO_INTENSITY) > + return int_en & !event_ch_is_light; This is the specific line the compiler doesn't like drivers/iio/light/apds9306.c:1036:39: warning: dubious: x & !y I would match int_src against specific values rather than using tricks based on what those values happen to be. return int_en && (int_src == APDS9306_INT_SRC_CLEAR); > + > + return -EINVAL; > + case IIO_EV_TYPE_THRESH_ADAPTIVE: > + ret = regmap_field_read(rf->int_thresh_var_en, &int_en); > + if (ret) > + return ret; > + > + return int_en; > + default: > + return -EINVAL; > + } > +} > + > +static int apds9306_write_event_config(struct iio_dev *indio_dev, > + const struct iio_chan_spec *chan, > + enum iio_event_type type, > + enum iio_event_direction dir, > + int state) > +{ > + struct apds9306_data *data = iio_priv(indio_dev); > + struct apds9306_regfields *rf = &data->rf; > + int ret, val; > + > + state = !!state; > + > + switch (type) { > + case IIO_EV_TYPE_THRESH: > + guard(mutex)(&data->mutex); Better to add explicit scope if you are going to use a guard. case IIO_EV_TYPE_THRESH: { guard(mutex)(&data->mutex); so we can see where the scope ends more clearly. > + > + /* > + * If interrupt is enabled, the channel is set before enabling > + * the interrupt. In case of disable, no need to switch > + * channels. In case of different channel is selected while > + * interrupt in on, just change the channel. > + */ > + if (state) { > + if (chan->type == IIO_LIGHT) > + val = 1; > + else if (chan->type == IIO_INTENSITY) > + val = 0; > + else > + return -EINVAL; > + > + ret = regmap_field_write(rf->int_src, val); > + if (ret) > + return ret; > + } > + > + ret = regmap_field_read(rf->int_en, &val); > + if (ret) > + return ret; > + > + if (val == state) > + return 0; > + > + ret = regmap_field_write(rf->int_en, state); > + if (ret) > + return ret; > + > + if (state) > + return pm_runtime_resume_and_get(data->dev); > + > + pm_runtime_mark_last_busy(data->dev); > + pm_runtime_put_autosuspend(data->dev); > + > + return 0; > + case IIO_EV_TYPE_THRESH_ADAPTIVE: > + return regmap_field_write(rf->int_thresh_var_en, state); > + default: > + return -EINVAL; > + } > +}