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

[Subhajit Ghosh]

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