Re: [PATCH v5 3/3] iio: light: Add support for APDS9306 Light Sensor

[Subhajit Ghosh <subhajit.ghosh@tweaklogic.com>]

On 21/1/24 19:52, Christophe JAILLET wrote:
> Le 21/01/2024 à 06:17, Subhajit Ghosh a écrit :
>> 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-ojZBjWEdjYKukZHgTAicrQ@public.gmane.org>
>> ---
> 
> Hi,
> 
> a few nits and a few real comment/question below.
> 
> Just my 2c.
> 
> CJ
> ...

>> +
>> +static int apds9306_read_event(struct iio_dev *indio_dev,
>> +                   const struct iio_chan_spec *chan,
>> +                   enum iio_event_type type,
>> +                   enum iio_event_direction dir,
>> +                   enum iio_event_info info,
>> +                   int *val, int *val2)
>> +{
>> +    struct apds9306_data *data = iio_priv(indio_dev);
>> +    int ret;
> 
> Other functions below that look really similar have a:
>     guard(mutex)(&data->mutex);
> 
> Is it needed here?
You are right, don't think so. Regmap lock is being used.
I will review the locking mechanism. Acknowledging all other comments.

Thanks for the review.

Regards,
Subhajit Ghosh