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;
> + }
> +}
