[PATCH] iio: light: STK3310: un-invert proximity values

[PATCH] iio: light: STK3310: un-invert proximity values

[Tiberiu Breana]

In accordance with the recent ABI changes, STK3310's
proximity readings should be un-inversed in order to
return low values for far-away objects and high values
for close ones.

Signed-off-by: Tiberiu Breana <tiberiu.a.breana@intel.com>
---
 drivers/iio/light/stk3310.c | 53 +++++++++++++++------------------------------
 1 file changed, 17 insertions(+), 36 deletions(-)

diff --git a/drivers/iio/light/stk3310.c b/drivers/iio/light/stk3310.c
index fee4297..c1a2182 100644
--- a/drivers/iio/light/stk3310.c
+++ b/drivers/iio/light/stk3310.c
@@ -43,7 +43,6 @@
 #define STK3311_CHIP_ID_VAL			0x1D
 #define STK3310_PSINT_EN			0x01
 #define STK3310_PS_MAX_VAL			0xFFFF
-#define STK3310_THRESH_MAX			0xFFFF
 
 #define STK3310_DRIVER_NAME			"stk3310"
 #define STK3310_REGMAP_NAME			"stk3310_regmap"
@@ -84,15 +83,13 @@ static const struct reg_field stk3310_reg_field_flag_psint =
 				REG_FIELD(STK3310_REG_FLAG, 4, 4);
 static const struct reg_field stk3310_reg_field_flag_nf =
 				REG_FIELD(STK3310_REG_FLAG, 0, 0);
-/*
- * Maximum PS values with regard to scale. Used to export the 'inverse'
- * PS value (high values for far objects, low values for near objects).
- */
+
+/* Estimate maximum proximity values with regard to measurement scale. */
 static const int stk3310_ps_max[4] = {
-	STK3310_PS_MAX_VAL / 64,
-	STK3310_PS_MAX_VAL / 16,
-	STK3310_PS_MAX_VAL /  4,
-	STK3310_PS_MAX_VAL,
+	STK3310_PS_MAX_VAL / 640,
+	STK3310_PS_MAX_VAL / 160,
+	STK3310_PS_MAX_VAL /  40,
+	STK3310_PS_MAX_VAL /  10
 };
 
 static const int stk3310_scale_table[][2] = {
@@ -128,14 +125,14 @@ static const struct iio_event_spec stk3310_events[] = {
 	/* Proximity event */
 	{
 		.type = IIO_EV_TYPE_THRESH,
-		.dir = IIO_EV_DIR_FALLING,
+		.dir = IIO_EV_DIR_RISING,
 		.mask_separate = BIT(IIO_EV_INFO_VALUE) |
 				 BIT(IIO_EV_INFO_ENABLE),
 	},
 	/* Out-of-proximity event */
 	{
 		.type = IIO_EV_TYPE_THRESH,
-		.dir = IIO_EV_DIR_RISING,
+		.dir = IIO_EV_DIR_FALLING,
 		.mask_separate = BIT(IIO_EV_INFO_VALUE) |
 				 BIT(IIO_EV_INFO_ENABLE),
 	},
@@ -205,23 +202,16 @@ static int stk3310_read_event(struct iio_dev *indio_dev,
 	u8 reg;
 	u16 buf;
 	int ret;
-	unsigned int index;
 	struct stk3310_data *data = iio_priv(indio_dev);
 
 	if (info != IIO_EV_INFO_VALUE)
 		return -EINVAL;
 
-	/*
-	 * Only proximity interrupts are implemented at the moment.
-	 * Since we're inverting proximity values, the sensor's 'high'
-	 * threshold will become our 'low' threshold, associated with
-	 * 'near' events. Similarly, the sensor's 'low' threshold will
-	 * be our 'high' threshold, associated with 'far' events.
-	 */
+	/* Only proximity interrupts are implemented at the moment. */
 	if (dir == IIO_EV_DIR_RISING)
-		reg = STK3310_REG_THDL_PS;
-	else if (dir == IIO_EV_DIR_FALLING)
 		reg = STK3310_REG_THDH_PS;
+	else if (dir == IIO_EV_DIR_FALLING)
+		reg = STK3310_REG_THDL_PS;
 	else
 		return -EINVAL;
 
@@ -232,8 +222,7 @@ static int stk3310_read_event(struct iio_dev *indio_dev,
 		dev_err(&data->client->dev, "register read failed\n");
 		return ret;
 	}
-	regmap_field_read(data->reg_ps_gain, &index);
-	*val = swab16(stk3310_ps_max[index] - buf);
+	*val = swab16(buf);
 
 	return IIO_VAL_INT;
 }
@@ -257,13 +246,13 @@ static int stk3310_write_event(struct iio_dev *indio_dev,
 		return -EINVAL;
 
 	if (dir == IIO_EV_DIR_RISING)
-		reg = STK3310_REG_THDL_PS;
-	else if (dir == IIO_EV_DIR_FALLING)
 		reg = STK3310_REG_THDH_PS;
+	else if (dir == IIO_EV_DIR_FALLING)
+		reg = STK3310_REG_THDL_PS;
 	else
 		return -EINVAL;
 
-	buf = swab16(stk3310_ps_max[index] - val);
+	buf = swab16(val);
 	ret = regmap_bulk_write(data->regmap, reg, &buf, 2);
 	if (ret < 0)
 		dev_err(&client->dev, "failed to set PS threshold!\n");
@@ -334,14 +323,6 @@ static int stk3310_read_raw(struct iio_dev *indio_dev,
 			return ret;
 		}
 		*val = swab16(buf);
-		if (chan->type == IIO_PROXIMITY) {
-			/*
-			 * Invert the proximity data so we return low values
-			 * for close objects and high values for far ones.
-			 */
-			regmap_field_read(data->reg_ps_gain, &index);
-			*val = stk3310_ps_max[index] - *val;
-		}
 		mutex_unlock(&data->lock);
 		return IIO_VAL_INT;
 	case IIO_CHAN_INFO_INT_TIME:
@@ -581,8 +562,8 @@ static irqreturn_t stk3310_irq_event_handler(int irq, void *private)
 	}
 	event = IIO_UNMOD_EVENT_CODE(IIO_PROXIMITY, 1,
 				     IIO_EV_TYPE_THRESH,
-				     (dir ? IIO_EV_DIR_RISING :
-					    IIO_EV_DIR_FALLING));
+				     (dir ? IIO_EV_DIR_FALLING :
+					    IIO_EV_DIR_RISING));
 	iio_push_event(indio_dev, event, data->timestamp);
 
 	/* Reset the interrupt flag */
-- 
1.9.1

Re: [PATCH] iio: light: STK3310: un-invert proximity values

[Peter Meerwald]

> In accordance with the recent ABI changes, STK3310's
> proximity readings should be un-inversed in order to
> return low values for far-away objects and high values
> for close ones.

the patch does a lot more than it advertises in the text above;
I think it needs to be split up

thanks, p.

> Signed-off-by: Tiberiu Breana <tiberiu.a.breana@intel.com>
> ---
>  drivers/iio/light/stk3310.c | 53 +++++++++++++++------------------------------
>  1 file changed, 17 insertions(+), 36 deletions(-)
> 
> diff --git a/drivers/iio/light/stk3310.c b/drivers/iio/light/stk3310.c
> index fee4297..c1a2182 100644
> --- a/drivers/iio/light/stk3310.c
> +++ b/drivers/iio/light/stk3310.c
> @@ -43,7 +43,6 @@
>  #define STK3311_CHIP_ID_VAL			0x1D
>  #define STK3310_PSINT_EN			0x01
>  #define STK3310_PS_MAX_VAL			0xFFFF
> -#define STK3310_THRESH_MAX			0xFFFF
>  
>  #define STK3310_DRIVER_NAME			"stk3310"
>  #define STK3310_REGMAP_NAME			"stk3310_regmap"
> @@ -84,15 +83,13 @@ static const struct reg_field stk3310_reg_field_flag_psint =
>  				REG_FIELD(STK3310_REG_FLAG, 4, 4);
>  static const struct reg_field stk3310_reg_field_flag_nf =
>  				REG_FIELD(STK3310_REG_FLAG, 0, 0);
> -/*
> - * Maximum PS values with regard to scale. Used to export the 'inverse'
> - * PS value (high values for far objects, low values for near objects).
> - */
> +
> +/* Estimate maximum proximity values with regard to measurement scale. */
>  static const int stk3310_ps_max[4] = {
> -	STK3310_PS_MAX_VAL / 64,
> -	STK3310_PS_MAX_VAL / 16,
> -	STK3310_PS_MAX_VAL /  4,
> -	STK3310_PS_MAX_VAL,
> +	STK3310_PS_MAX_VAL / 640,
> +	STK3310_PS_MAX_VAL / 160,
> +	STK3310_PS_MAX_VAL /  40,
> +	STK3310_PS_MAX_VAL /  10
>  };
>  
>  static const int stk3310_scale_table[][2] = {
> @@ -128,14 +125,14 @@ static const struct iio_event_spec stk3310_events[] = {
>  	/* Proximity event */
>  	{
>  		.type = IIO_EV_TYPE_THRESH,
> -		.dir = IIO_EV_DIR_FALLING,
> +		.dir = IIO_EV_DIR_RISING,
>  		.mask_separate = BIT(IIO_EV_INFO_VALUE) |
>  				 BIT(IIO_EV_INFO_ENABLE),
>  	},
>  	/* Out-of-proximity event */
>  	{
>  		.type = IIO_EV_TYPE_THRESH,
> -		.dir = IIO_EV_DIR_RISING,
> +		.dir = IIO_EV_DIR_FALLING,
>  		.mask_separate = BIT(IIO_EV_INFO_VALUE) |
>  				 BIT(IIO_EV_INFO_ENABLE),
>  	},
> @@ -205,23 +202,16 @@ static int stk3310_read_event(struct iio_dev *indio_dev,
>  	u8 reg;
>  	u16 buf;
>  	int ret;
> -	unsigned int index;
>  	struct stk3310_data *data = iio_priv(indio_dev);
>  
>  	if (info != IIO_EV_INFO_VALUE)
>  		return -EINVAL;
>  
> -	/*
> -	 * Only proximity interrupts are implemented at the moment.
> -	 * Since we're inverting proximity values, the sensor's 'high'
> -	 * threshold will become our 'low' threshold, associated with
> -	 * 'near' events. Similarly, the sensor's 'low' threshold will
> -	 * be our 'high' threshold, associated with 'far' events.
> -	 */
> +	/* Only proximity interrupts are implemented at the moment. */
>  	if (dir == IIO_EV_DIR_RISING)
> -		reg = STK3310_REG_THDL_PS;
> -	else if (dir == IIO_EV_DIR_FALLING)
>  		reg = STK3310_REG_THDH_PS;
> +	else if (dir == IIO_EV_DIR_FALLING)
> +		reg = STK3310_REG_THDL_PS;
>  	else
>  		return -EINVAL;
>  
> @@ -232,8 +222,7 @@ static int stk3310_read_event(struct iio_dev *indio_dev,
>  		dev_err(&data->client->dev, "register read failed\n");
>  		return ret;
>  	}
> -	regmap_field_read(data->reg_ps_gain, &index);
> -	*val = swab16(stk3310_ps_max[index] - buf);
> +	*val = swab16(buf);
>  
>  	return IIO_VAL_INT;
>  }
> @@ -257,13 +246,13 @@ static int stk3310_write_event(struct iio_dev *indio_dev,
>  		return -EINVAL;
>  
>  	if (dir == IIO_EV_DIR_RISING)
> -		reg = STK3310_REG_THDL_PS;
> -	else if (dir == IIO_EV_DIR_FALLING)
>  		reg = STK3310_REG_THDH_PS;
> +	else if (dir == IIO_EV_DIR_FALLING)
> +		reg = STK3310_REG_THDL_PS;
>  	else
>  		return -EINVAL;
>  
> -	buf = swab16(stk3310_ps_max[index] - val);
> +	buf = swab16(val);
>  	ret = regmap_bulk_write(data->regmap, reg, &buf, 2);
>  	if (ret < 0)
>  		dev_err(&client->dev, "failed to set PS threshold!\n");
> @@ -334,14 +323,6 @@ static int stk3310_read_raw(struct iio_dev *indio_dev,
>  			return ret;
>  		}
>  		*val = swab16(buf);
> -		if (chan->type == IIO_PROXIMITY) {
> -			/*
> -			 * Invert the proximity data so we return low values
> -			 * for close objects and high values for far ones.
> -			 */
> -			regmap_field_read(data->reg_ps_gain, &index);
> -			*val = stk3310_ps_max[index] - *val;
> -		}
>  		mutex_unlock(&data->lock);
>  		return IIO_VAL_INT;
>  	case IIO_CHAN_INFO_INT_TIME:
> @@ -581,8 +562,8 @@ static irqreturn_t stk3310_irq_event_handler(int irq, void *private)
>  	}
>  	event = IIO_UNMOD_EVENT_CODE(IIO_PROXIMITY, 1,
>  				     IIO_EV_TYPE_THRESH,
> -				     (dir ? IIO_EV_DIR_RISING :
> -					    IIO_EV_DIR_FALLING));
> +				     (dir ? IIO_EV_DIR_FALLING :
> +					    IIO_EV_DIR_RISING));
>  	iio_push_event(indio_dev, event, data->timestamp);
>  
>  	/* Reset the interrupt flag */
> 

-- 

Peter Meerwald
+43-664-2444418 (mobile)

RE: [PATCH] iio: light: STK3310: un-invert proximity values

[Breana, Tiberiu A]

> -----Original Message-----
> From: Peter Meerwald [mailto:pmeerw@pmeerw.net]
> Sent: Wednesday, June 17, 2015 4:18 PM
> To: Breana, Tiberiu A
> Cc: linux-iio@vger.kernel.org
> Subject: Re: [PATCH] iio: light: STK3310: un-invert proximity values
> 
> 
> > In accordance with the recent ABI changes, STK3310's proximity
> > readings should be un-inversed in order to return low values for
> > far-away objects and high values for close ones.
> 
> the patch does a lot more than it advertises in the text above; I think it needs
> to be split up
> 
> thanks, p.
> 

Peter, how would you see this patch split? A patch that deals with the raw readings and max values and another that switches the event types?
Or should I rather just write a more detailed commit message?

Thanks,
Tiberiu

> > Signed-off-by: Tiberiu Breana <tiberiu.a.breana@intel.com>
> > ---
> >  drivers/iio/light/stk3310.c | 53
> > +++++++++++++++------------------------------
> >  1 file changed, 17 insertions(+), 36 deletions(-)
> >
> > diff --git a/drivers/iio/light/stk3310.c b/drivers/iio/light/stk3310.c
> > index fee4297..c1a2182 100644
> > --- a/drivers/iio/light/stk3310.c
> > +++ b/drivers/iio/light/stk3310.c
> > @@ -43,7 +43,6 @@
> >  #define STK3311_CHIP_ID_VAL			0x1D
> >  #define STK3310_PSINT_EN			0x01
> >  #define STK3310_PS_MAX_VAL			0xFFFF
> > -#define STK3310_THRESH_MAX			0xFFFF
> >
> >  #define STK3310_DRIVER_NAME			"stk3310"
> >  #define STK3310_REGMAP_NAME			"stk3310_regmap"
> > @@ -84,15 +83,13 @@ static const struct reg_field
> stk3310_reg_field_flag_psint =
> >  				REG_FIELD(STK3310_REG_FLAG, 4, 4);  static
> const struct reg_field
> > stk3310_reg_field_flag_nf =
> >  				REG_FIELD(STK3310_REG_FLAG, 0, 0);
> > -/*
> > - * Maximum PS values with regard to scale. Used to export the 'inverse'
> > - * PS value (high values for far objects, low values for near objects).
> > - */
> > +
> > +/* Estimate maximum proximity values with regard to measurement
> > +scale. */
> >  static const int stk3310_ps_max[4] = {
> > -	STK3310_PS_MAX_VAL / 64,
> > -	STK3310_PS_MAX_VAL / 16,
> > -	STK3310_PS_MAX_VAL /  4,
> > -	STK3310_PS_MAX_VAL,
> > +	STK3310_PS_MAX_VAL / 640,
> > +	STK3310_PS_MAX_VAL / 160,
> > +	STK3310_PS_MAX_VAL /  40,
> > +	STK3310_PS_MAX_VAL /  10
> >  };
> >
> >  static const int stk3310_scale_table[][2] = { @@ -128,14 +125,14 @@
> > static const struct iio_event_spec stk3310_events[] = {
> >  	/* Proximity event */
> >  	{
> >  		.type = IIO_EV_TYPE_THRESH,
> > -		.dir = IIO_EV_DIR_FALLING,
> > +		.dir = IIO_EV_DIR_RISING,
> >  		.mask_separate = BIT(IIO_EV_INFO_VALUE) |
> >  				 BIT(IIO_EV_INFO_ENABLE),
> >  	},
> >  	/* Out-of-proximity event */
> >  	{
> >  		.type = IIO_EV_TYPE_THRESH,
> > -		.dir = IIO_EV_DIR_RISING,
> > +		.dir = IIO_EV_DIR_FALLING,
> >  		.mask_separate = BIT(IIO_EV_INFO_VALUE) |
> >  				 BIT(IIO_EV_INFO_ENABLE),
> >  	},
> > @@ -205,23 +202,16 @@ static int stk3310_read_event(struct iio_dev
> *indio_dev,
> >  	u8 reg;
> >  	u16 buf;
> >  	int ret;
> > -	unsigned int index;
> >  	struct stk3310_data *data = iio_priv(indio_dev);
> >
> >  	if (info != IIO_EV_INFO_VALUE)
> >  		return -EINVAL;
> >
> > -	/*
> > -	 * Only proximity interrupts are implemented at the moment.
> > -	 * Since we're inverting proximity values, the sensor's 'high'
> > -	 * threshold will become our 'low' threshold, associated with
> > -	 * 'near' events. Similarly, the sensor's 'low' threshold will
> > -	 * be our 'high' threshold, associated with 'far' events.
> > -	 */
> > +	/* Only proximity interrupts are implemented at the moment. */
> >  	if (dir == IIO_EV_DIR_RISING)
> > -		reg = STK3310_REG_THDL_PS;
> > -	else if (dir == IIO_EV_DIR_FALLING)
> >  		reg = STK3310_REG_THDH_PS;
> > +	else if (dir == IIO_EV_DIR_FALLING)
> > +		reg = STK3310_REG_THDL_PS;
> >  	else
> >  		return -EINVAL;
> >
> > @@ -232,8 +222,7 @@ static int stk3310_read_event(struct iio_dev
> *indio_dev,
> >  		dev_err(&data->client->dev, "register read failed\n");
> >  		return ret;
> >  	}
> > -	regmap_field_read(data->reg_ps_gain, &index);
> > -	*val = swab16(stk3310_ps_max[index] - buf);
> > +	*val = swab16(buf);
> >
> >  	return IIO_VAL_INT;
> >  }
> > @@ -257,13 +246,13 @@ static int stk3310_write_event(struct iio_dev
> *indio_dev,
> >  		return -EINVAL;
> >
> >  	if (dir == IIO_EV_DIR_RISING)
> > -		reg = STK3310_REG_THDL_PS;
> > -	else if (dir == IIO_EV_DIR_FALLING)
> >  		reg = STK3310_REG_THDH_PS;
> > +	else if (dir == IIO_EV_DIR_FALLING)
> > +		reg = STK3310_REG_THDL_PS;
> >  	else
> >  		return -EINVAL;
> >
> > -	buf = swab16(stk3310_ps_max[index] - val);
> > +	buf = swab16(val);
> >  	ret = regmap_bulk_write(data->regmap, reg, &buf, 2);
> >  	if (ret < 0)
> >  		dev_err(&client->dev, "failed to set PS threshold!\n"); @@ -
> 334,14
> > +323,6 @@ static int stk3310_read_raw(struct iio_dev *indio_dev,
> >  			return ret;
> >  		}
> >  		*val = swab16(buf);
> > -		if (chan->type == IIO_PROXIMITY) {
> > -			/*
> > -			 * Invert the proximity data so we return low values
> > -			 * for close objects and high values for far ones.
> > -			 */
> > -			regmap_field_read(data->reg_ps_gain, &index);
> > -			*val = stk3310_ps_max[index] - *val;
> > -		}
> >  		mutex_unlock(&data->lock);
> >  		return IIO_VAL_INT;
> >  	case IIO_CHAN_INFO_INT_TIME:
> > @@ -581,8 +562,8 @@ static irqreturn_t stk3310_irq_event_handler(int
> irq, void *private)
> >  	}
> >  	event = IIO_UNMOD_EVENT_CODE(IIO_PROXIMITY, 1,
> >  				     IIO_EV_TYPE_THRESH,
> > -				     (dir ? IIO_EV_DIR_RISING :
> > -					    IIO_EV_DIR_FALLING));
> > +				     (dir ? IIO_EV_DIR_FALLING :
> > +					    IIO_EV_DIR_RISING));
> >  	iio_push_event(indio_dev, event, data->timestamp);
> >
> >  	/* Reset the interrupt flag */
> >
> 
> --
> 
> Peter Meerwald
> +43-664-2444418 (mobile)

Re: [PATCH] iio: light: STK3310: un-invert proximity values

[Jonathan Cameron]

On 17/06/15 12:09, Tiberiu Breana wrote:
> In accordance with the recent ABI changes, STK3310's
> proximity readings should be un-inversed in order to
> return low values for far-away objects and high values
> for close ones.
> 
> Signed-off-by: Tiberiu Breana <tiberiu.a.breana@intel.com>
> ---
>  drivers/iio/light/stk3310.c | 53 +++++++++++++++------------------------------
>  1 file changed, 17 insertions(+), 36 deletions(-)
> 
> diff --git a/drivers/iio/light/stk3310.c b/drivers/iio/light/stk3310.c
> index fee4297..c1a2182 100644
> --- a/drivers/iio/light/stk3310.c
> +++ b/drivers/iio/light/stk3310.c
> @@ -43,7 +43,6 @@
>  #define STK3311_CHIP_ID_VAL			0x1D
>  #define STK3310_PSINT_EN			0x01
>  #define STK3310_PS_MAX_VAL			0xFFFF
> -#define STK3310_THRESH_MAX			0xFFFF
>  
>  #define STK3310_DRIVER_NAME			"stk3310"
>  #define STK3310_REGMAP_NAME			"stk3310_regmap"
> @@ -84,15 +83,13 @@ static const struct reg_field stk3310_reg_field_flag_psint =
>  				REG_FIELD(STK3310_REG_FLAG, 4, 4);
>  static const struct reg_field stk3310_reg_field_flag_nf =
>  				REG_FIELD(STK3310_REG_FLAG, 0, 0);
> -/*
> - * Maximum PS values with regard to scale. Used to export the 'inverse'
> - * PS value (high values for far objects, low values for near objects).
> - */
> +
> +/* Estimate maximum proximity values with regard to measurement scale. */
>  static const int stk3310_ps_max[4] = {
> -	STK3310_PS_MAX_VAL / 64,
> -	STK3310_PS_MAX_VAL / 16,
> -	STK3310_PS_MAX_VAL /  4,
> -	STK3310_PS_MAX_VAL,
> +	STK3310_PS_MAX_VAL / 640,
> +	STK3310_PS_MAX_VAL / 160,
> +	STK3310_PS_MAX_VAL /  40,
> +	STK3310_PS_MAX_VAL /  10
>  };
What's this change about?  All uses of this array as present in this patch
have been removed.  It is used elsewhere for parameter sanity checking, but
I can't follow why it now needs to be divided by 10.
>  
>  static const int stk3310_scale_table[][2] = {
> @@ -128,14 +125,14 @@ static const struct iio_event_spec stk3310_events[] = {
>  	/* Proximity event */
>  	{
>  		.type = IIO_EV_TYPE_THRESH,
> -		.dir = IIO_EV_DIR_FALLING,
> +		.dir = IIO_EV_DIR_RISING,
>  		.mask_separate = BIT(IIO_EV_INFO_VALUE) |
>  				 BIT(IIO_EV_INFO_ENABLE),
>  	},
>  	/* Out-of-proximity event */
>  	{
>  		.type = IIO_EV_TYPE_THRESH,
> -		.dir = IIO_EV_DIR_RISING,
> +		.dir = IIO_EV_DIR_FALLING,
>  		.mask_separate = BIT(IIO_EV_INFO_VALUE) |
>  				 BIT(IIO_EV_INFO_ENABLE),
>  	},
> @@ -205,23 +202,16 @@ static int stk3310_read_event(struct iio_dev *indio_dev,
>  	u8 reg;
>  	u16 buf;
>  	int ret;
> -	unsigned int index;
>  	struct stk3310_data *data = iio_priv(indio_dev);
>  
>  	if (info != IIO_EV_INFO_VALUE)
>  		return -EINVAL;
>  
> -	/*
> -	 * Only proximity interrupts are implemented at the moment.
> -	 * Since we're inverting proximity values, the sensor's 'high'
> -	 * threshold will become our 'low' threshold, associated with
> -	 * 'near' events. Similarly, the sensor's 'low' threshold will
> -	 * be our 'high' threshold, associated with 'far' events.
> -	 */
> +	/* Only proximity interrupts are implemented at the moment. */
>  	if (dir == IIO_EV_DIR_RISING)
> -		reg = STK3310_REG_THDL_PS;
> -	else if (dir == IIO_EV_DIR_FALLING)
>  		reg = STK3310_REG_THDH_PS;
> +	else if (dir == IIO_EV_DIR_FALLING)
> +		reg = STK3310_REG_THDL_PS;
>  	else
>  		return -EINVAL;
>  
> @@ -232,8 +222,7 @@ static int stk3310_read_event(struct iio_dev *indio_dev,
>  		dev_err(&data->client->dev, "register read failed\n");
>  		return ret;
>  	}
> -	regmap_field_read(data->reg_ps_gain, &index);
> -	*val = swab16(stk3310_ps_max[index] - buf);
> +	*val = swab16(buf);
>  
>  	return IIO_VAL_INT;
>  }
> @@ -257,13 +246,13 @@ static int stk3310_write_event(struct iio_dev *indio_dev,
>  		return -EINVAL;
>  
>  	if (dir == IIO_EV_DIR_RISING)
> -		reg = STK3310_REG_THDL_PS;
> -	else if (dir == IIO_EV_DIR_FALLING)
>  		reg = STK3310_REG_THDH_PS;
> +	else if (dir == IIO_EV_DIR_FALLING)
> +		reg = STK3310_REG_THDL_PS;
>  	else
>  		return -EINVAL;
>  
> -	buf = swab16(stk3310_ps_max[index] - val);
> +	buf = swab16(val);
>  	ret = regmap_bulk_write(data->regmap, reg, &buf, 2);
>  	if (ret < 0)
>  		dev_err(&client->dev, "failed to set PS threshold!\n");
> @@ -334,14 +323,6 @@ static int stk3310_read_raw(struct iio_dev *indio_dev,
>  			return ret;
>  		}
>  		*val = swab16(buf);
> -		if (chan->type == IIO_PROXIMITY) {
> -			/*
> -			 * Invert the proximity data so we return low values
> -			 * for close objects and high values for far ones.
> -			 */
> -			regmap_field_read(data->reg_ps_gain, &index);
> -			*val = stk3310_ps_max[index] - *val;
> -		}
>  		mutex_unlock(&data->lock);
>  		return IIO_VAL_INT;
>  	case IIO_CHAN_INFO_INT_TIME:
> @@ -581,8 +562,8 @@ static irqreturn_t stk3310_irq_event_handler(int irq, void *private)
>  	}
>  	event = IIO_UNMOD_EVENT_CODE(IIO_PROXIMITY, 1,
>  				     IIO_EV_TYPE_THRESH,
> -				     (dir ? IIO_EV_DIR_RISING :
> -					    IIO_EV_DIR_FALLING));
> +				     (dir ? IIO_EV_DIR_FALLING :
> +					    IIO_EV_DIR_RISING));
>  	iio_push_event(indio_dev, event, data->timestamp);
>  
>  	/* Reset the interrupt flag */
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-iio" in

Re: [PATCH] iio: light: STK3310: un-invert proximity values

[Jonathan Cameron]

On 18/06/15 09:09, Breana, Tiberiu A wrote:
>> -----Original Message-----
>> From: Peter Meerwald [mailto:pmeerw@pmeerw.net]
>> Sent: Wednesday, June 17, 2015 4:18 PM
>> To: Breana, Tiberiu A
>> Cc: linux-iio@vger.kernel.org
>> Subject: Re: [PATCH] iio: light: STK3310: un-invert proximity values
>>
>>
>>> In accordance with the recent ABI changes, STK3310's proximity
>>> readings should be un-inversed in order to return low values for
>>> far-away objects and high values for close ones.
>>
>> the patch does a lot more than it advertises in the text above; I think it needs
>> to be split up
>>
>> thanks, p.
>>
> 
> Peter, how would you see this patch split? A patch that deals with the raw readings and max values and another that switches the event types?
> Or should I rather just write a more detailed commit message?
I think a small change to the commit message to detail that both the event directions
and the actual returned values need to be flipped should do the job.
> 
> Thanks,
> Tiberiu
> 
>>> Signed-off-by: Tiberiu Breana <tiberiu.a.breana@intel.com>
>>> ---
>>>  drivers/iio/light/stk3310.c | 53
>>> +++++++++++++++------------------------------
>>>  1 file changed, 17 insertions(+), 36 deletions(-)
>>>
>>> diff --git a/drivers/iio/light/stk3310.c b/drivers/iio/light/stk3310.c
>>> index fee4297..c1a2182 100644
>>> --- a/drivers/iio/light/stk3310.c
>>> +++ b/drivers/iio/light/stk3310.c
>>> @@ -43,7 +43,6 @@
>>>  #define STK3311_CHIP_ID_VAL			0x1D
>>>  #define STK3310_PSINT_EN			0x01
>>>  #define STK3310_PS_MAX_VAL			0xFFFF
>>> -#define STK3310_THRESH_MAX			0xFFFF
>>>
>>>  #define STK3310_DRIVER_NAME			"stk3310"
>>>  #define STK3310_REGMAP_NAME			"stk3310_regmap"
>>> @@ -84,15 +83,13 @@ static const struct reg_field
>> stk3310_reg_field_flag_psint =
>>>  				REG_FIELD(STK3310_REG_FLAG, 4, 4);  static
>> const struct reg_field
>>> stk3310_reg_field_flag_nf =
>>>  				REG_FIELD(STK3310_REG_FLAG, 0, 0);
>>> -/*
>>> - * Maximum PS values with regard to scale. Used to export the 'inverse'
>>> - * PS value (high values for far objects, low values for near objects).
>>> - */
>>> +
>>> +/* Estimate maximum proximity values with regard to measurement
>>> +scale. */
>>>  static const int stk3310_ps_max[4] = {
>>> -	STK3310_PS_MAX_VAL / 64,
>>> -	STK3310_PS_MAX_VAL / 16,
>>> -	STK3310_PS_MAX_VAL /  4,
>>> -	STK3310_PS_MAX_VAL,
>>> +	STK3310_PS_MAX_VAL / 640,
>>> +	STK3310_PS_MAX_VAL / 160,
>>> +	STK3310_PS_MAX_VAL /  40,
>>> +	STK3310_PS_MAX_VAL /  10
>>>  };
>>>
>>>  static const int stk3310_scale_table[][2] = { @@ -128,14 +125,14 @@
>>> static const struct iio_event_spec stk3310_events[] = {
>>>  	/* Proximity event */
>>>  	{
>>>  		.type = IIO_EV_TYPE_THRESH,
>>> -		.dir = IIO_EV_DIR_FALLING,
>>> +		.dir = IIO_EV_DIR_RISING,
>>>  		.mask_separate = BIT(IIO_EV_INFO_VALUE) |
>>>  				 BIT(IIO_EV_INFO_ENABLE),
>>>  	},
>>>  	/* Out-of-proximity event */
>>>  	{
>>>  		.type = IIO_EV_TYPE_THRESH,
>>> -		.dir = IIO_EV_DIR_RISING,
>>> +		.dir = IIO_EV_DIR_FALLING,
>>>  		.mask_separate = BIT(IIO_EV_INFO_VALUE) |
>>>  				 BIT(IIO_EV_INFO_ENABLE),
>>>  	},
>>> @@ -205,23 +202,16 @@ static int stk3310_read_event(struct iio_dev
>> *indio_dev,
>>>  	u8 reg;
>>>  	u16 buf;
>>>  	int ret;
>>> -	unsigned int index;
>>>  	struct stk3310_data *data = iio_priv(indio_dev);
>>>
>>>  	if (info != IIO_EV_INFO_VALUE)
>>>  		return -EINVAL;
>>>
>>> -	/*
>>> -	 * Only proximity interrupts are implemented at the moment.
>>> -	 * Since we're inverting proximity values, the sensor's 'high'
>>> -	 * threshold will become our 'low' threshold, associated with
>>> -	 * 'near' events. Similarly, the sensor's 'low' threshold will
>>> -	 * be our 'high' threshold, associated with 'far' events.
>>> -	 */
>>> +	/* Only proximity interrupts are implemented at the moment. */
>>>  	if (dir == IIO_EV_DIR_RISING)
>>> -		reg = STK3310_REG_THDL_PS;
>>> -	else if (dir == IIO_EV_DIR_FALLING)
>>>  		reg = STK3310_REG_THDH_PS;
>>> +	else if (dir == IIO_EV_DIR_FALLING)
>>> +		reg = STK3310_REG_THDL_PS;
>>>  	else
>>>  		return -EINVAL;
>>>
>>> @@ -232,8 +222,7 @@ static int stk3310_read_event(struct iio_dev
>> *indio_dev,
>>>  		dev_err(&data->client->dev, "register read failed\n");
>>>  		return ret;
>>>  	}
>>> -	regmap_field_read(data->reg_ps_gain, &index);
>>> -	*val = swab16(stk3310_ps_max[index] - buf);
>>> +	*val = swab16(buf);
>>>
>>>  	return IIO_VAL_INT;
>>>  }
>>> @@ -257,13 +246,13 @@ static int stk3310_write_event(struct iio_dev
>> *indio_dev,
>>>  		return -EINVAL;
>>>
>>>  	if (dir == IIO_EV_DIR_RISING)
>>> -		reg = STK3310_REG_THDL_PS;
>>> -	else if (dir == IIO_EV_DIR_FALLING)
>>>  		reg = STK3310_REG_THDH_PS;
>>> +	else if (dir == IIO_EV_DIR_FALLING)
>>> +		reg = STK3310_REG_THDL_PS;
>>>  	else
>>>  		return -EINVAL;
>>>
>>> -	buf = swab16(stk3310_ps_max[index] - val);
>>> +	buf = swab16(val);
>>>  	ret = regmap_bulk_write(data->regmap, reg, &buf, 2);
>>>  	if (ret < 0)
>>>  		dev_err(&client->dev, "failed to set PS threshold!\n"); @@ -
>> 334,14
>>> +323,6 @@ static int stk3310_read_raw(struct iio_dev *indio_dev,
>>>  			return ret;
>>>  		}
>>>  		*val = swab16(buf);
>>> -		if (chan->type == IIO_PROXIMITY) {
>>> -			/*
>>> -			 * Invert the proximity data so we return low values
>>> -			 * for close objects and high values for far ones.
>>> -			 */
>>> -			regmap_field_read(data->reg_ps_gain, &index);
>>> -			*val = stk3310_ps_max[index] - *val;
>>> -		}
>>>  		mutex_unlock(&data->lock);
>>>  		return IIO_VAL_INT;
>>>  	case IIO_CHAN_INFO_INT_TIME:
>>> @@ -581,8 +562,8 @@ static irqreturn_t stk3310_irq_event_handler(int
>> irq, void *private)
>>>  	}
>>>  	event = IIO_UNMOD_EVENT_CODE(IIO_PROXIMITY, 1,
>>>  				     IIO_EV_TYPE_THRESH,
>>> -				     (dir ? IIO_EV_DIR_RISING :
>>> -					    IIO_EV_DIR_FALLING));
>>> +				     (dir ? IIO_EV_DIR_FALLING :
>>> +					    IIO_EV_DIR_RISING));
>>>  	iio_push_event(indio_dev, event, data->timestamp);
>>>
>>>  	/* Reset the interrupt flag */
>>>
>>
>> --
>>
>> Peter Meerwald
>> +43-664-2444418 (mobile)
> --
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-iio" in

RE: [PATCH] iio: light: STK3310: un-invert proximity values

[Breana, Tiberiu A]

> -----Original Message-----=0A=
> From: Jonathan Cameron [mailto:jic23@kernel.org]=0A=
> Sent: Sunday, June 21, 2015 1:06 PM=0A=
> To: Breana, Tiberiu A; linux-iio@vger.kernel.org=0A=
> Subject: Re: [PATCH] iio: light: STK3310: un-invert proximity values=0A=
> =0A=
> On 17/06/15 12:09, Tiberiu Breana wrote:=0A=
> > In accordance with the recent ABI changes, STK3310's proximity=0A=
> > readings should be un-inversed in order to return low values for=0A=
> > far-away objects and high values for close ones.=0A=
> >=0A=
=0A=
<snip>=0A=
=0A=
> > -/*=0A=
> > - * Maximum PS values with regard to scale. Used to export the 'inverse=
'=0A=
> > - * PS value (high values for far objects, low values for near objects)=
.=0A=
> > - */=0A=
> > +=0A=
> > +/* Estimate maximum proximity values with regard to measurement=0A=
> > +scale. */=0A=
> >  static const int stk3310_ps_max[4] =3D {=0A=
> > -	STK3310_PS_MAX_VAL / 64,=0A=
> > -	STK3310_PS_MAX_VAL / 16,=0A=
> > -	STK3310_PS_MAX_VAL /  4,=0A=
> > -	STK3310_PS_MAX_VAL,=0A=
> > +	STK3310_PS_MAX_VAL / 640,=0A=
> > +	STK3310_PS_MAX_VAL / 160,=0A=
> > +	STK3310_PS_MAX_VAL /  40,=0A=
> > +	STK3310_PS_MAX_VAL /  10=0A=
> >  };=0A=
> What's this change about?  All uses of this array as present in this patc=
h have=0A=
> been removed.  It is used elsewhere for parameter sanity checking, but I=
=0A=
> can't follow why it now needs to be divided by 10.=0A=
=0A=
Indeed, these values are still used in _write_event for setting proximity i=
nterrupt=0A=
threshold values. Truth is, they should have been divided by 10 from the st=
art,=0A=
but since all values were relative to the pre-defined maximum values, it di=
dn't=0A=
really matter. I used the higher max values because the readings would=0A=
sometimes go over the expected range (when I press down on the sensor with=
=0A=
my thumb), so the proximity readings became negative. But that shouldn't ha=
ppen=0A=
in the case of a real device, as the sensor is covered by at least a screen=
 layer.=0A=
=0A=
For the default scale of 0.1 real proximity readings max out at around 6500=
,=0A=
roughly a tenth of the maximum theoretical value, as expected. Now that we'=
re=0A=
using un-inverted measurements, we can't use the same reference values as=
=0A=
before - e.g. if the real maximum value is 6500, we can't allow setting a t=
hreshold=0A=
of 30000 - it will never be reached.=0A=
If you think this change warrants a separate patch, I'll split it.=0A=
=0A=
Regards,=0A=
Tiberiu=

Re: [PATCH] iio: light: STK3310: un-invert proximity values

[Jonathan Cameron]

On 24/06/15 10:41, Breana, Tiberiu A wrote:
>> -----Original Message-----
>> From: Jonathan Cameron [mailto:jic23@kernel.org]
>> Sent: Sunday, June 21, 2015 1:06 PM
>> To: Breana, Tiberiu A; linux-iio@vger.kernel.org
>> Subject: Re: [PATCH] iio: light: STK3310: un-invert proximity values
>>
>> On 17/06/15 12:09, Tiberiu Breana wrote:
>>> In accordance with the recent ABI changes, STK3310's proximity
>>> readings should be un-inversed in order to return low values for
>>> far-away objects and high values for close ones.
>>>
> 
> <snip>
> 
>>> -/*
>>> - * Maximum PS values with regard to scale. Used to export the 'inverse'
>>> - * PS value (high values for far objects, low values for near objects).
>>> - */
>>> +
>>> +/* Estimate maximum proximity values with regard to measurement
>>> +scale. */
>>>  static const int stk3310_ps_max[4] = {
>>> -	STK3310_PS_MAX_VAL / 64,
>>> -	STK3310_PS_MAX_VAL / 16,
>>> -	STK3310_PS_MAX_VAL /  4,
>>> -	STK3310_PS_MAX_VAL,
>>> +	STK3310_PS_MAX_VAL / 640,
>>> +	STK3310_PS_MAX_VAL / 160,
>>> +	STK3310_PS_MAX_VAL /  40,
>>> +	STK3310_PS_MAX_VAL /  10
>>>  };
>> What's this change about?  All uses of this array as present in this patch have
>> been removed.  It is used elsewhere for parameter sanity checking, but I
>> can't follow why it now needs to be divided by 10.
> 
> Indeed, these values are still used in _write_event for setting proximity interrupt
> threshold values. Truth is, they should have been divided by 10 from the start,
> but since all values were relative to the pre-defined maximum values, it didn't
> really matter. I used the higher max values because the readings would
> sometimes go over the expected range (when I press down on the sensor with
> my thumb), so the proximity readings became negative. But that shouldn't happen
> in the case of a real device, as the sensor is covered by at least a screen layer.
> 
> For the default scale of 0.1 real proximity readings max out at around 6500,
> roughly a tenth of the maximum theoretical value, as expected. Now that we're
> using un-inverted measurements, we can't use the same reference values as
> before - e.g. if the real maximum value is 6500, we can't allow setting a threshold
> of 30000 - it will never be reached.
> If you think this change warrants a separate patch, I'll split it.
> 
Perhaps just warrants a mention in the patch description!