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

Subhajit Ghosh
2024-02-27T13:12:58Z

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