Message-ID: <20240803145836.4e372899@jic23-huawei>
Date: Sat, 3 Aug 2024 14:58:36 +0100
From: Jonathan Cameron <jic23@...nel.org>
To: Abhash Jha <abhashkumarjha123@...il.com>
Cc: linux-iio@...r.kernel.org, lars@...afoo.de, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 1/1] iio: light: apds9960: Add proximity and gesture
 offset calibration

On Thu,  1 Aug 2024 20:08:57 +0530
Abhash Jha <abhashkumarjha123@...il.com> wrote:

> Proximity and gesture offset registers perform offset correction to
> improve cross-talk performance. Added `calibbias` to the proximity
> and gesture channels.
> Provided facility to set calibbias based on the channel number.
> 
> Signed-off-by: Abhash Jha <abhashkumarjha123@...il.com>

I nearly applied this with tweaks, but I think there are enough improvements
that can be made to make it worth bouncing back to you one more time

Thanks,

Jonathan
> ---
>  drivers/iio/light/apds9960.c | 69 +++++++++++++++++++++++++++++++++++-
>  1 file changed, 68 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/iio/light/apds9960.c b/drivers/iio/light/apds9960.c
> index 1065a340b..e7f2e129c 100644
> --- a/drivers/iio/light/apds9960.c
> +++ b/drivers/iio/light/apds9960.c
> @@ -146,6 +146,25 @@ struct apds9960_data {
>  
>  	/* gesture buffer */
>  	u8 buffer[4]; /* 4 8-bit channels */
> +
> +	/* calibration value buffer */
> +	int calibbias[5];
> +};
> +
> +enum {
> +	APDS9960_CHAN_PROXIMITY,
> +	APDS9960_CHAN_GESTURE_UP,
> +	APDS9960_CHAN_GESTURE_DOWN,
> +	APDS9960_CHAN_GESTURE_LEFT,
> +	APDS9960_CHAN_GESTURE_RIGHT,
> +};
> +
> +static const unsigned int apds9960_offset_regs[] = {
> +	[APDS9960_CHAN_PROXIMITY] = APDS9960_REG_POFFSET_UR,
As below - I don't think this needs to be looked up here as it's the only proximity
channel + we have two registers to over.

Either, make this a 2D array and include both registers + some code below
to only write a register if non zero, or drop the first entry. Fine to leave
it as 0 with a comment saying why.

Advantage of a 2D register with 1 or 2 addresses per channel is that you can
have just one code path below.

> +	[APDS9960_CHAN_GESTURE_UP] = APDS9960_REG_GOFFSET_U,
> +	[APDS9960_CHAN_GESTURE_DOWN] = APDS9960_REG_GOFFSET_D,
> +	[APDS9960_CHAN_GESTURE_LEFT] = APDS9960_REG_GOFFSET_L,
> +	[APDS9960_CHAN_GESTURE_RIGHT] = APDS9960_REG_GOFFSET_R,
>  };
>  
>  static const struct reg_default apds9960_reg_defaults[] = {
> @@ -255,6 +274,7 @@ static const struct iio_event_spec apds9960_als_event_spec[] = {
>  
>  #define APDS9960_GESTURE_CHANNEL(_dir, _si) { \
>  	.type = IIO_PROXIMITY, \
> +	.info_mask_separate = BIT(IIO_CHAN_INFO_CALIBBIAS), \
>  	.channel = _si + 1, \
>  	.scan_index = _si, \
>  	.indexed = 1, \
> @@ -282,7 +302,8 @@ static const struct iio_chan_spec apds9960_channels[] = {
>  	{
>  		.type = IIO_PROXIMITY,
>  		.address = APDS9960_REG_PDATA,
> -		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
> +		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
> +			BIT(IIO_CHAN_INFO_CALIBBIAS),
>  		.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE),
>  		.channel = 0,
>  		.indexed = 0,
> @@ -316,6 +337,42 @@ static const struct iio_chan_spec apds9960_channels[] = {
>  	APDS9960_INTENSITY_CHANNEL(BLUE),
>  };
>  
> +static int apds9960_set_calibbias(struct apds9960_data *data,
> +		struct iio_chan_spec const *chan, int calibbias)
> +{
> +	unsigned int reg;
> +	int ret;
> +
> +	if (calibbias < S8_MIN || calibbias > S8_MAX)
> +		return -EINVAL;
> +
> +	guard(mutex)(&data->lock);
> +	switch (chan->channel) {
> +	case APDS9960_CHAN_PROXIMITY:
> +		/* We will be writing the calibbias value to both the */
> +		/* POFFSET_UR and POFFSET_DL registers */
Use multiline comment syntax.

		/*
		 * Write calibbias to both POFFSET_UR adn POFFSET_DL
		 * registers.
		 */

> +		reg = apds9960_offset_regs[chan->channel];
> +		ret = regmap_write(data->regmap, reg, calibbias);

I can't see an advantage in having the local variable reg.

Easier to just put the array inline.
		ret = regmap_write(data->regmap,
				   apds9960_offset_regs[chan->channel,
				   calibbias);

Mind you, this always hits the same register I think. So just
hardcode the value here.
> +		if (ret < 0)
> +			return ret;
> +
> +		ret = regmap_write(data->regmap, APDS9960_REG_POFFSET_DL, calibbias);
> +		if (ret < 0)
> +			return ret;
> +		break;
> +	default:
Use an explicit type here as will make it more readable.
Throw in a default with an error return just to make it clear there are
no other valid values.

As above, I'd suggest you can combine these two paths easily anyway 
by making the second write optional based on whether offset_regs[chan->channel][1] == 0
or not.

> +		/* For GOFFSET_x we will be writing to the */
> +		/* respective offset register */
Fix this comment as well.  Is it trying to say that unlike the above,
only only register is relevant?  I'm not sure that's worth saying
as it's pretty obvious from the code.

> +		reg = apds9960_offset_regs[chan->channel];
> +		ret = regmap_write(data->regmap, reg, calibbias);
Similar here.

> +		if (ret < 0)
> +			return ret;
> +	}
> +
> +	data->calibbias[chan->channel] = calibbias;
Blank line here.

> +	return 0;
> +}
> +
>  /* integration time in us */
>  static const int apds9960_int_time[][2] = {
>  	{ 28000, 246},
> @@ -531,6 +588,12 @@ static int apds9960_read_raw(struct iio_dev *indio_dev,
>  		}
>  		mutex_unlock(&data->lock);
>  		break;
> +	case IIO_CHAN_INFO_CALIBBIAS:
> +		mutex_lock(&data->lock);
> +		*val = data->calibbias[chan->channel];
> +		ret = IIO_VAL_INT;

This driver would benefit a lot from use of guard() and scoped_guard()
but that isn't related to this patch beyond this function making
me take a look at read_raw.


> +		mutex_unlock(&data->lock);
> +		break;
>  	}
>  
>  	return ret;
> @@ -564,6 +627,10 @@ static int apds9960_write_raw(struct iio_dev *indio_dev,
>  		default:
>  			return -EINVAL;
>  		}
> +	case IIO_CHAN_INFO_CALIBBIAS:
> +		if (val2 != 0)
> +			return -EINVAL;
> +		return apds9960_set_calibbias(data, chan, val);
>  	default:
>  		return -EINVAL;
>  	}

Powered by blists - more mailing lists