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

Subhajit Ghosh
2024-02-25T10:00:50Z

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
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