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

From: Jonathan Cameron <jic23@kernel.org>
To: Subhajit Ghosh <subhajit.ghosh@tweaklogic.com>
Cc: Lars-Peter Clausen <lars@metafoo.de>,
	Rob Herring <robh+dt@kernel.org>,
	Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
	Conor Dooley <conor+dt@kernel.org>,
	Matti Vaittinen <mazziesaccount@gmail.com>,
	Andy Shevchenko <andriy.shevchenko@linux.intel.com>,
	Marek Vasut <marex@denx.de>, Anshul Dalal <anshulusr@gmail.com>,
	Javier Carrasco <javier.carrasco.cruz@gmail.com>,
	Matt Ranostay <matt@ranostay.sg>,
	Stefan Windfeldt-Prytz <stefan.windfeldt-prytz@axis.com>,
	linux-iio@vger.kernel.org, devicetree@vger.kernel.org,
	linux-kernel@vger.kernel.org
Subject: Re: [PATCH v7 5/5] iio: light: Add support for APDS9306 Light Sensor
Date: Tue, 27 Feb 2024 19:20:32 +0000	[thread overview]
Message-ID: <20240227192032.12def64a@jic23-huawei> (raw)
In-Reply-To: <a94b86fe-0896-47ba-a597-0cd59a0665a2@tweaklogic.com>

On Tue, 27 Feb 2024 23:42:48 +1030
Subhajit Ghosh <subhajit.ghosh@tweaklogic.com> wrote:

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