by Jonathan Cameron

[permalink] [raw]

Subject: Re: [PATCH v7 5/5] iio: light: Add support for APDS9306 Light Sensor

On Tue, 27 Feb 2024 23:42:48 +1030
Subhajit Ghosh <[email protected]> wrote:

> On 25/2/24 01:43, Jonathan Cameron wrote:
> > On Sun, 18 Feb 2024 16:18:26 +1030
> > Subhajit Ghosh <[email protected]> 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 <[email protected]>
> > 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;
> >> +}
> >> +
> >
> > ...
>
>