Date: Tue, 27 Feb 2024 23:42:48 +1030
From: Subhajit Ghosh <subhajit.ghosh@...aklogic.com>
To: Jonathan Cameron <jic23@...nel.org>
Cc: Lars-Peter Clausen <lars@...afoo.de>, Rob Herring <robh+dt@...nel.org>,
 Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
 Conor Dooley <conor+dt@...nel.org>,
 Matti Vaittinen <mazziesaccount@...il.com>,
 Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
 Marek Vasut <marex@...x.de>, Anshul Dalal <anshulusr@...il.com>,
 Javier Carrasco <javier.carrasco.cruz@...il.com>,
 Matt Ranostay <matt@...ostay.sg>,
 Stefan Windfeldt-Prytz <stefan.windfeldt-prytz@...s.com>,
 linux-iio@...r.kernel.org, devicetree@...r.kernel.org,
 linux-kernel@...r.kernel.org
Subject: Re: [PATCH v7 5/5] iio: light: Add support for APDS9306 Light Sensor

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

Powered by blists - more mailing lists