| Message ID | 20240801143857.16438-2-abhashkumarjha123@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | Added calibbias to perform offset correction | expand |
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; > }
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
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; }
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(-)