| Message ID | 20240218054826.2881-1-subhajit.ghosh@tweaklogic.com |
|---|---|
| Headers | show |
| Series | Support for Avago APDS9306 Ambient Light Sensor | expand |
On Sun, 18 Feb 2024 16:18:26 +1030 Subhajit Ghosh <subhajit.ghosh@tweaklogic.com> 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@tweaklogic.com> 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; > + } > +}
On 25/2/24 01:43, Jonathan Cameron wrote: > On Sun, 18 Feb 2024 16:18:26 +1030 > Subhajit Ghosh <subhajit.ghosh@tweaklogic.com> 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@tweaklogic.com> > 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 Appreciate that you tried to fix all the issues for me. > ... > ... > >> + >> +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). Yes, I will check which interrupt channel is enabled then push appropriate event. Earlier versions wrongly had both channels as IIO_LIGHT which were fixed in later revisions. I forgot to change the event part! > >> + >> + /* >> + * 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. This unique name was suggested in a previous review: https://lore.kernel.org/all/20240121152332.6b15666a@jic23-huawei/ I will change it next version. int_src is logical. > >> + 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 am using gcc 12.2.0 for cross compiling. I definitely do not want to send patches with warnings in them. Can you please let me know the gcc version or flags using which you got the above warning? Should I always use the latest released version? > > 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); I will implement this. Thank you for taking time to review the code in detail and also appreciate your suggestions. Regards, Subhajit Ghosh
> > > >> + > >> + /* > >> + * 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. > This unique name was suggested in a previous review: > https://lore.kernel.org/all/20240121152332.6b15666a@jic23-huawei/ > I will change it next version. int_src is logical. Ah, I failed to register that it was a multi bit field (which it isn't really but we should track what the datasheet says). If it had been a single bit then it would have made sense to name it as a boolean flag. Not so when in theory it could take 4 values (even if only 2 are defined). > > > >> + 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 am using gcc 12.2.0 for cross compiling. I definitely do not want to send > patches with warnings in them. Can you please let me know the gcc version > or flags using which you got the above warning? Should I always use the > latest released version? Version shouldn't matter that much but this was x86 build with gcc 13.2.1 W=1 is maybe what is showing this up as it enables a bunch more warnings. IIO is almost clean with W=1 though a few minor things have gotten in recently that I need to tidy up. > > > > 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); > I will implement this. > > > Thank you for taking time to review the code in detail and also appreciate > your suggestions. > > Regards, > Subhajit Ghosh
On Sun, Feb 18, 2024 at 04:18:26PM +1030, Subhajit Ghosh 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. ... > +/* > + * Available scales with gain 1x - 18x, timings 3.125, 25, 50, 100, 200, 400 mS "mS" --> "ms." > + * Time impacts to gain: 1x, 8x, 16x, 32x, 64x, 128x > + */ ... > + /* > + * 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); > + Redundant blank line > + if (ret) > + return ret; ... > +static int apds9306_init_iio_gts(struct apds9306_data *data) > +{ > + int i, ret, part_id; > + > + ret = regmap_read(data->regmap, APDS9306_PART_ID_REG, &part_id); > + if (ret) > + return ret; > + > + for (i = 0; i < ARRAY_SIZE(apds9306_gts_mul); i++) > + if (part_id == apds9306_gts_mul[i].part_id) > + break; > + > + if (i == ARRAY_SIZE(apds9306_gts_mul)) > + return -ENXIO; Strange choice of the error code, why not (one of) -ENOENT / -ENODATA ? > + return devm_iio_init_iio_gts(data->dev, > + apds9306_gts_mul[i].max_scale_int, > + apds9306_gts_mul[i].max_scale_nano, > + apds9306_gains, ARRAY_SIZE(apds9306_gains), > + apds9306_itimes, ARRAY_SIZE(apds9306_itimes), > + &data->gts); > + > + return -ENXIO; Dead code. > +} ... Jonathan, are you going to apply this and addressing comments at the same time? Or should it be another version?
On Mon, 26 Feb 2024 16:12:23 +0200 Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > On Sun, Feb 18, 2024 at 04:18:26PM +1030, Subhajit Ghosh 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. > > ... > > > +/* > > + * Available scales with gain 1x - 18x, timings 3.125, 25, 50, 100, 200, 400 mS > > "mS" --> "ms." > > > + * Time impacts to gain: 1x, 8x, 16x, 32x, 64x, 128x > > + */ > > ... > > > + /* > > + * 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); > > > + > > Redundant blank line > > > + if (ret) > > + return ret; > > ... > > > +static int apds9306_init_iio_gts(struct apds9306_data *data) > > +{ > > + int i, ret, part_id; > > + > > + ret = regmap_read(data->regmap, APDS9306_PART_ID_REG, &part_id); > > + if (ret) > > + return ret; > > + > > + for (i = 0; i < ARRAY_SIZE(apds9306_gts_mul); i++) > > + if (part_id == apds9306_gts_mul[i].part_id) > > + break; > > + > > + if (i == ARRAY_SIZE(apds9306_gts_mul)) > > + return -ENXIO; > > Strange choice of the error code, why not (one of) -ENOENT / -ENODATA ? > > > + return devm_iio_init_iio_gts(data->dev, > > + apds9306_gts_mul[i].max_scale_int, > > + apds9306_gts_mul[i].max_scale_nano, > > + apds9306_gains, ARRAY_SIZE(apds9306_gains), > > + apds9306_itimes, ARRAY_SIZE(apds9306_itimes), > > + &data->gts); > > > + > > + return -ENXIO; > > Dead code. > > > +} > > ... > > Jonathan, are you going to apply this and addressing comments at the same time? > Or should it be another version? > The multibit field pretending to be a boolean was too complex for to want to modify whilst applying. So yes, v8 with that tidied up and your comments sorted out Jonathan
On 25/2/24 01:43, Jonathan Cameron wrote: > On Sun, 18 Feb 2024 16:18:26 +1030 > Subhajit Ghosh <subhajit.ghosh@tweaklogic.com> 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@tweaklogic.com> > 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 > >> +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. Hi Jonathan, For the above fix I am supplying the second parameter to IIO_UNMOD_EVENT_CODE() as "0" which gives me the below output from userspace: ./iio_event_monitor /dev/iio:device0 Event: time: xx, type: illuminance, channel: 0, evtype: thresh, direction: either Event: time: yy, type: intensity, channel: 0, evtype: thresh, direction: either As I do not have indexed channels, I have used zero for both Light and Intensity channel numbers. Should I make the intensity type as channel one for the output to look like this? Event: time: xx, type: illuminance, channel: 0, evtype: thresh, direction: either Event: time: yy, type: intensity, channel: 1, evtype: thresh, direction: either What do you think? Regards, Subhajit Ghosh > > >> + 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; >> +} >> + > > ...
On Tue, 27 Feb 2024 23:42:48 +1030 Subhajit Ghosh <subhajit.ghosh@tweaklogic.com> wrote: > On 25/2/24 01:43, Jonathan Cameron wrote: > > On Sun, 18 Feb 2024 16:18:26 +1030 > > Subhajit Ghosh <subhajit.ghosh@tweaklogic.com> 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@tweaklogic.com> > > 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 > > > > >> +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. > Hi Jonathan, > > For the above fix I am supplying the second parameter to IIO_UNMOD_EVENT_CODE() > as "0" which gives me the below output from userspace: > ./iio_event_monitor /dev/iio:device0 > Event: time: xx, type: illuminance, channel: 0, evtype: thresh, direction: either > Event: time: yy, type: intensity, channel: 0, evtype: thresh, direction: either > > As I do not have indexed channels, I have used zero for both Light and Intensity > channel numbers. Should I make the intensity type as channel one for the output > to look like this? > Event: time: xx, type: illuminance, channel: 0, evtype: thresh, direction: either > Event: time: yy, type: intensity, channel: 1, evtype: thresh, direction: either > No need. It's not an ABI bug if you did have that mix of channels, but you'd need to make them indexed in the chan_spec to match. Don't bother. You should however us a modified event for the intensity channel seeing as it is .modified = 1, IIO_MOD_LIGHT_CLEAR So IIO_MOD_EVENT_CODE would be appropriate. > What do you think? > > Regards, > Subhajit Ghosh > > > > > >> + 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; > >> +} > >> + > > > > ... > >