[PATCH v2 0/1] Added calibbias to perform offset correction

[PATCH v2 0/1] Added calibbias to perform offset correction

[Abhash Jha]

Hello,
This patch adds support for doing internal offset correction for the
proximity and gesture circuits. The correction value is written to
the respective POFFSET_x or GOFFSET_x register depending on which
direction we want to perform this calibration.
For the proximity channel we apply the offset to both the DL and UR
directions. And for the gesture channel we apply the offset to the
respective U, D, L, R direction.

Changes in v2:
- Used IIO_CHAN_INFO_CALIBBIAS instead of exposing custom sysfs attributes
- Used enum to choose between correct the offset registers
- Corrected the formatting and style errors
- Link to v1 : https://lore.kernel.org/linux-iio/20240707171357.709d9e35@jic23-huawei/

Regards,
Abhash

Abhash Jha (1):
  iio: light: apds9960: Add proximity and gesture offset calibration

 drivers/iio/light/apds9960.c | 69 +++++++++++++++++++++++++++++++++++-
 1 file changed, 68 insertions(+), 1 deletion(-)

-- 
2.43.0

[PATCH v2 1/1] iio: light: apds9960: Add proximity and gesture offset calibration

[Abhash Jha]

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@gmail.com>
---
 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,
+	[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 */
+		reg = apds9960_offset_regs[chan->channel];
+		ret = regmap_write(data->regmap, reg, calibbias);
+		if (ret < 0)
+			return ret;
+
+		ret = regmap_write(data->regmap, APDS9960_REG_POFFSET_DL, calibbias);
+		if (ret < 0)
+			return ret;
+		break;
+	default:
+		/* For GOFFSET_x we will be writing to the */
+		/* respective offset register */
+		reg = apds9960_offset_regs[chan->channel];
+		ret = regmap_write(data->regmap, reg, calibbias);
+		if (ret < 0)
+			return ret;
+	}
+
+	data->calibbias[chan->channel] = calibbias;
+	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;
+		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;
 	}
-- 
2.43.0

Re: [PATCH v2 1/1] iio: light: apds9960: Add proximity and gesture offset calibration

[Jonathan Cameron]

On Thu,  1 Aug 2024 20:08:57 +0530
Abhash Jha <abhashkumarjha123@gmail.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@gmail.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;
>  	}

Re: [PATCH v2 1/1] iio: light: apds9960: Add proximity and gesture offset calibration

[Abhash jha]

On Sat, Aug 3, 2024 at 7:28 PM Jonathan Cameron <jic23@kernel.org> wrote:
>
> On Thu,  1 Aug 2024 20:08:57 +0530
> Abhash Jha <abhashkumarjha123@gmail.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@gmail.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

Hi Jonathan,
I hope I understood what  you were trying to say correctly, I made a 2D array
for the registers and then modified the set_calibbias function accordingly.
Here is the link to the patch v3 :
https://lore.kernel.org/linux-iio/20240804134212.51682-1-abhashkumarjha123@gmail.com/T/#t
Do tell me if there is something else that needs to be changed.

Thanks,
Abhash