Date: Sun, 25 Feb 2024 20:30:40 +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
Appreciate that you tried to fix all the issues for me.
> 
..
> ...
> 
>> +
>> +static irqreturn_t apds9306_irq_handler(int irq, void *priv)
>> +{
>> +	struct iio_dev *indio_dev = priv;
>> +	struct apds9306_data *data = iio_priv(indio_dev);
>> +	struct apds9306_regfields *rf = &data->rf;
>> +	int ret, status, int_ch;
>> +
>> +	/*
>> +	 * The interrupt line is released and the interrupt flag is
>> +	 * cleared as a result of reading the status register. All the
>> +	 * status flags are cleared as a result of this read.
>> +	 */
>> +	ret = regmap_read(data->regmap, APDS9306_MAIN_STATUS_REG, &status);
>> +	if (ret < 0) {
>> +		dev_err_ratelimited(data->dev, "status reg read failed\n");
>> +		return IRQ_HANDLED;
>> +	}
>> +
>> +	ret = regmap_field_read(rf->int_src, &int_ch);
>> +	if (ret)
>> +		return ret;
>> +
>> +	if ((status & APDS9306_ALS_INT_STAT_MASK))
>> +		iio_push_event(indio_dev, IIO_UNMOD_EVENT_CODE(IIO_LIGHT,
>> +			       int_ch, IIO_EV_TYPE_THRESH, IIO_EV_DIR_EITHER),
>> +			       iio_get_time_ns(indio_dev));
> 
> As commented on elsewhere I'm not seeing the relationship between the event
> pushed here and the channels this device provides (one of which is modified
> for starters).
Yes, I will check which interrupt channel is enabled then push appropriate
event. Earlier versions wrongly had both channels as IIO_LIGHT which were fixed in
later revisions. I forgot to change the event part!
> 
>> +
>> +	/*
>> +	 * If a one-shot read through sysfs is underway at the same time
>> +	 * as this interrupt handler is executing and a read data available
>> +	 * flag was set, this flag is set to inform read_poll_timeout()
>> +	 * to exit.
>> +	 */
>> +	if ((status & APDS9306_ALS_DATA_STAT_MASK))
>> +		data->read_data_available = 1;
>> +
>> +	return IRQ_HANDLED;
>> +}
> 
> ...
> 
>> +static int apds9306_read_event_config(struct iio_dev *indio_dev,
>> +				      const struct iio_chan_spec *chan,
>> +				      enum iio_event_type type,
>> +				      enum iio_event_direction dir)
>> +{
>> +	struct apds9306_data *data = iio_priv(indio_dev);
>> +	struct apds9306_regfields *rf = &data->rf;
>> +	int int_en, event_ch_is_light, ret;
>> +
>> +	switch (type) {
>> +	case IIO_EV_TYPE_THRESH:
>> +		guard(mutex)(&data->mutex);
>> +
>> +		ret = regmap_field_read(rf->int_src, &event_ch_is_light);
> 
> Call the local value int_src - it's not obvious to a reviewer what
> relationship between that and int_src is. I had to go read the datasheet
> to find out.
This unique name was suggested in a previous review:
https://lore.kernel.org/all/20240121152332.6b15666a@jic23-huawei/
I will change it next version. int_src is logical.
> 
>> +		if (ret)
>> +			return ret;
>> +
>> +		ret = regmap_field_read(rf->int_en, &int_en);
>> +		if (ret)
>> +			return ret;
>> +
>> +		if (chan->type == IIO_LIGHT)
>> +			return int_en & event_ch_is_light;
>> +
>> +		if (chan->type == IIO_INTENSITY)
>> +			return int_en & !event_ch_is_light;
> This is the specific line the compiler doesn't like
> drivers/iio/light/apds9306.c:1036:39: warning: dubious: x & !y
I am using gcc 12.2.0 for cross compiling. I definitely do not want to send
patches with warnings in them. Can you please let me know the gcc version
or flags using which you got the above warning? Should I always use the
latest released version?
> 
> I would match int_src against specific values rather than using tricks
> based on what those values happen to be.
> 
> 			return int_en && (int_src == APDS9306_INT_SRC_CLEAR);
I will implement this.


Thank you for taking time to review the code in detail and also appreciate
your suggestions.

Regards,
Subhajit Ghosh

Powered by blists - more mailing lists