[PATCH v3 0/1] Offset correction for apds9960

[PATCH v3 0/1] Offset correction for apds9960

[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 v3:
- Some code refactoring based on comments in v2
- Link to v2 : https://lore.kernel.org/linux-iio/20240803145836.4e372899@jic23-huawei/T/#u

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/


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

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

-- 
2.43.0

[PATCH v3 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 | 55 +++++++++++++++++++++++++++++++++++-
 1 file changed, 54 insertions(+), 1 deletion(-)

diff --git a/drivers/iio/light/apds9960.c b/drivers/iio/light/apds9960.c
index 1065a340b..bc966b096 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[][2] = {
+	[APDS9960_CHAN_PROXIMITY] = {APDS9960_REG_POFFSET_UR, APDS9960_REG_POFFSET_DL},
+	[APDS9960_CHAN_GESTURE_UP] = {APDS9960_REG_GOFFSET_U, 0},
+	[APDS9960_CHAN_GESTURE_DOWN] = {APDS9960_REG_GOFFSET_D, 0},
+	[APDS9960_CHAN_GESTURE_LEFT] = {APDS9960_REG_GOFFSET_L, 0},
+	[APDS9960_CHAN_GESTURE_RIGHT] = {APDS9960_REG_GOFFSET_R, 0},
 };
 
 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,28 @@ 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)
+{
+	int ret, i;
+
+	if (calibbias < S8_MIN || calibbias > S8_MAX)
+		return -EINVAL;
+
+	guard(mutex)(&data->lock);
+	for (i = 0; i < 2; i++) {
+		if (apds9960_offset_regs[chan->channel][i] == 0)
+			break;
+
+		ret = regmap_write(data->regmap, apds9960_offset_regs[chan->channel][i], 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 +574,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 +613,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 v3 1/1] iio: light: apds9960: Add proximity and gesture offset calibration

[Jonathan Cameron]

On Sun,  4 Aug 2024 19:12:12 +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>
Hi Abhash,

LGTM. Applied to the togreg branch of iio.git and pushed out as testing
for 0-day (build bot) to take a look at it and see if it can find anything
we missed.

Thanks,

Jonathan

> ---
>  drivers/iio/light/apds9960.c | 55 +++++++++++++++++++++++++++++++++++-
>  1 file changed, 54 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/iio/light/apds9960.c b/drivers/iio/light/apds9960.c
> index 1065a340b..bc966b096 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[][2] = {
> +	[APDS9960_CHAN_PROXIMITY] = {APDS9960_REG_POFFSET_UR, APDS9960_REG_POFFSET_DL},
> +	[APDS9960_CHAN_GESTURE_UP] = {APDS9960_REG_GOFFSET_U, 0},
> +	[APDS9960_CHAN_GESTURE_DOWN] = {APDS9960_REG_GOFFSET_D, 0},
> +	[APDS9960_CHAN_GESTURE_LEFT] = {APDS9960_REG_GOFFSET_L, 0},
> +	[APDS9960_CHAN_GESTURE_RIGHT] = {APDS9960_REG_GOFFSET_R, 0},
>  };
>  
>  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,28 @@ 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)
> +{
> +	int ret, i;
> +
> +	if (calibbias < S8_MIN || calibbias > S8_MAX)
> +		return -EINVAL;
> +
> +	guard(mutex)(&data->lock);
> +	for (i = 0; i < 2; i++) {
> +		if (apds9960_offset_regs[chan->channel][i] == 0)
> +			break;
> +
> +		ret = regmap_write(data->regmap, apds9960_offset_regs[chan->channel][i], 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 +574,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 +613,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;
>  	}