On Fri, Dec 23, 2022 at 05:00:54PM +0100, Jonathan Cameron wrote:
> On Tue, 20 Dec 2022 22:49:59 +0100
> Mårten Lindahl <marten.lindahl@xxxxxxxx> wrote:
>
> > Add support to configure proximity sensor interrupts and threshold
> > limits for vcnl4040. If an interrupt is detected an event will be
> > pushed to the event interface.
> >
> > Signed-off-by: Mårten Lindahl <marten.lindahl@xxxxxxxx>
> Hi,
>
> Code looks good in general. A few readability related suggestions inline.
>
> Thanks,
>
> Jonathan
Hi Jonathan!
Thank you. Please see my reflections below.
>
> > ---
> > drivers/iio/light/vcnl4000.c | 163 +++++++++++++++++++++++++++++++++++
> > 1 file changed, 163 insertions(+)
> >
> > diff --git a/drivers/iio/light/vcnl4000.c b/drivers/iio/light/vcnl4000.c
> > index 142d1760f65d..61d18c404ea6 100644
> > --- a/drivers/iio/light/vcnl4000.c
> > +++ b/drivers/iio/light/vcnl4000.c
> > @@ -60,8 +60,11 @@
>
> ...
>
> > /* Bit masks for interrupt registers. */
> > #define VCNL4010_INT_THR_SEL BIT(0) /* Select threshold interrupt source */
> > @@ -138,6 +144,7 @@ struct vcnl4000_data {
> > enum vcnl4000_device_ids id;
> > int rev;
> > int al_scale;
> > + int ps_int;
>
> Bit big for 2 bits ;) Maybe size it same as register size.
>
> Also, probably benefit from a comment as ps_int isn't a particularly obviously name.
Ok, I'll do so.
>
> > const struct vcnl4000_chip_spec *chip_spec;
> > struct mutex vcnl4000_lock;
> > struct vcnl4200_channel vcnl4200_al;
>
>
> ...
>
> >
> > +static int vcnl4040_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 vcnl4000_data *data = iio_priv(indio_dev);
> > +
> > + return (dir == IIO_EV_DIR_RISING) ?
> > + (data->ps_int & 0x01) : (data->ps_int & 0x02) >> 1;
>
> Add some field definitions and FIELD_GET() to extract them.
I will do that.
>
> > +}
> > +
> > +static int vcnl4040_write_event_config(struct iio_dev *indio_dev,
> > + const struct iio_chan_spec *chan,
> > + enum iio_event_type type,
> > + enum iio_event_direction dir, int state)
> > +{
> > + int ret;
> > + u16 val;
> > + struct vcnl4000_data *data = iio_priv(indio_dev);
> > +
> > + mutex_lock(&data->vcnl4000_lock);
> > +
> > + ret = i2c_smbus_read_word_data(data->client, VCNL4200_PS_CONF1);
> > + if (ret < 0)
> > + goto out;
> > +
> > + val = FIELD_GET(VCNL4040_PS_CONF2_PS_INT, ret);
> > +
> > + if (dir == IIO_EV_DIR_RISING)
> > + val = state ? (val | 0x1) : (val & 0x2);
>
> Whilst I'm sure this is correct, it's not easy to follow. Perhaps
> val = state ? (val | 0x1) : (val & ~0x1);
> to make it clear you are turning on an off one bit?
> Also as above, some field definitions may make this easier to follow.
I will do that to make it more clear.
>
> > + else
> > + val = state ? (val | 0x2) : (val & 0x1);
> > +
> > + data->ps_int = val;
> > + val = (ret & ~VCNL4040_PS_CONF2_PS_INT) |
>
> It's been quite a few lines. Probably better to put that ret into
> a reg_val or similarly named field to make it slightly easier to see it
> is retained from above.
>
> > + FIELD_PREP(VCNL4040_PS_CONF2_PS_INT, val);
> > + ret = i2c_smbus_write_word_data(data->client, VCNL4200_PS_CONF1, val);
> > +
> > +out:
> > + mutex_unlock(&data->vcnl4000_lock);
> > + data->chip_spec->set_power_state(data, (bool)data->ps_int);
> the bool cast is a little nasty. Perhaps != 0 is clearer?
I will rework these lines using field definitions. I'll send v2 shortly.
Kind regards
Mårten
>
> > +
> > + return ret;
> > +}
> > +
>
>
