Messages in this thread | | | Date | Tue, 27 Feb 2024 23:42:48 +1030 | Subject | Re: [PATCH v7 5/5] iio: light: Add support for APDS9306 Light Sensor | From | Subhajit Ghosh <> |
| |
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; >> +} >> + > > ...
| |