[PATCH v3 1/9] iio: light: rpr0521 disable sensor -bugfix

[PATCH v3 1/9] iio: light: rpr0521 disable sensor -bugfix

[Mikko Koivunen]

Sensor was marked enabled on each call even if the call was for disabling
sensor.

Signed-off-by: Mikko Koivunen <mikko.koivunen@fi.rohmeurope.com>
---
Tested on LeMaker HiKey with AOSP7.1 kernel 4.4.
Patch v2->v3 changes:
whitespace change.

 drivers/iio/light/rpr0521.c |   10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/drivers/iio/light/rpr0521.c b/drivers/iio/light/rpr0521.c
index 7de0f39..03504f6 100644
--- a/drivers/iio/light/rpr0521.c
+++ b/drivers/iio/light/rpr0521.c
@@ -197,7 +197,10 @@ static int rpr0521_als_enable(struct rpr0521_data *data, u8 status)
 	if (ret < 0)
 		return ret;
 
-	data->als_dev_en = true;
+	if (status & RPR0521_MODE_ALS_MASK)
+		data->als_dev_en = true;
+	else
+		data->als_dev_en = false;
 
 	return 0;
 }
@@ -212,7 +215,10 @@ static int rpr0521_pxs_enable(struct rpr0521_data *data, u8 status)
 	if (ret < 0)
 		return ret;
 
-	data->pxs_dev_en = true;
+	if (status & RPR0521_MODE_PXS_MASK)
+		data->pxs_dev_en = true;
+	else
+		data->pxs_dev_en = false;
 
 	return 0;
 }
-- 
1.7.9.5

[PATCH v3 2/9] iio: light: rpr0521 poweroff for probe fails

[Mikko Koivunen]

Set sensor measurement off after probe fail in pm_runtime_set_active() or
iio_device_register(). Without this change sensor measurement stays on
even though probe fails on these calls.

This is maybe rare case, but causes constant power drain without any
benefits when it happens. Power drain is 20-500uA, typically 180uA.

Signed-off-by: Mikko Koivunen <mikko.koivunen@fi.rohmeurope.com>
---
Sensor measurement is turned on rpr0521_init() so in probe it should be
turned off in error cases after that. With CONFIG_PM it's probably
unnecessary for iio_device_register()-fail, but without CONFIG_PM it is
needed. Writing power off twice when CONFIG_PM enabled is ok.

 drivers/iio/light/rpr0521.c |   17 +++++++++++++++--
 1 file changed, 15 insertions(+), 2 deletions(-)

diff --git a/drivers/iio/light/rpr0521.c b/drivers/iio/light/rpr0521.c
index 03504f6..5d077f4 100644
--- a/drivers/iio/light/rpr0521.c
+++ b/drivers/iio/light/rpr0521.c
@@ -516,13 +516,26 @@ static int rpr0521_probe(struct i2c_client *client,
 
 	ret = pm_runtime_set_active(&client->dev);
 	if (ret < 0)
-		return ret;
+		goto err_poweroff;
 
 	pm_runtime_enable(&client->dev);
 	pm_runtime_set_autosuspend_delay(&client->dev, RPR0521_SLEEP_DELAY_MS);
 	pm_runtime_use_autosuspend(&client->dev);
 
-	return iio_device_register(indio_dev);
+	ret = iio_device_register(indio_dev);
+	if (ret)
+		goto err_pm_disable;
+
+	return 0;
+
+err_pm_disable:
+	pm_runtime_disable(&client->dev);
+	pm_runtime_set_suspended(&client->dev);
+	pm_runtime_put_noidle(&client->dev);
+err_poweroff:
+	(void) rpr0521_poweroff(data);
+
+	return ret;
 }
 
 static int rpr0521_remove(struct i2c_client *client)
-- 
1.7.9.5

Re: [PATCH v3 2/9] iio: light: rpr0521 poweroff for probe fails

[Jonathan Cameron]

On 05/05/17 12:19, Mikko Koivunen wrote:
> Set sensor measurement off after probe fail in pm_runtime_set_active() or
> iio_device_register(). Without this change sensor measurement stays on
> even though probe fails on these calls.
> 
> This is maybe rare case, but causes constant power drain without any
> benefits when it happens. Power drain is 20-500uA, typically 180uA.
> 
> Signed-off-by: Mikko Koivunen <mikko.koivunen@fi.rohmeurope.com>
Looks good to me, but I will be looking for Acks from Daniel on this
set.  Daniel let me know if you want to pass on responsibility for the
driver (perhaps Mikko if willing?)

one tiny point inline.
> ---
> Sensor measurement is turned on rpr0521_init() so in probe it should be
> turned off in error cases after that. With CONFIG_PM it's probably
> unnecessary for iio_device_register()-fail, but without CONFIG_PM it is
> needed. Writing power off twice when CONFIG_PM enabled is ok.
> 
>  drivers/iio/light/rpr0521.c |   17 +++++++++++++++--
>  1 file changed, 15 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/iio/light/rpr0521.c b/drivers/iio/light/rpr0521.c
> index 03504f6..5d077f4 100644
> --- a/drivers/iio/light/rpr0521.c
> +++ b/drivers/iio/light/rpr0521.c
> @@ -516,13 +516,26 @@ static int rpr0521_probe(struct i2c_client *client,
>  
>  	ret = pm_runtime_set_active(&client->dev);
>  	if (ret < 0)
> -		return ret;
> +		goto err_poweroff;
>  
>  	pm_runtime_enable(&client->dev);
>  	pm_runtime_set_autosuspend_delay(&client->dev, RPR0521_SLEEP_DELAY_MS);
>  	pm_runtime_use_autosuspend(&client->dev);
>  
> -	return iio_device_register(indio_dev);
> +	ret = iio_device_register(indio_dev);
> +	if (ret)
> +		goto err_pm_disable;
> +
> +	return 0;
> +
> +err_pm_disable:
> +	pm_runtime_disable(&client->dev);
> +	pm_runtime_set_suspended(&client->dev);
> +	pm_runtime_put_noidle(&client->dev);
> +err_poweroff:
> +	(void) rpr0521_poweroff(data);
shouldn't need the void cast.
> +
> +	return ret;
>  }
>  
>  static int rpr0521_remove(struct i2c_client *client)
> 

Re: [PATCH v3 2/9] iio: light: rpr0521 poweroff for probe fails

[Koivunen, Mikko]

On 7.5.2017 14:42, Jonathan Cameron wrote:
> On 05/05/17 12:19, Mikko Koivunen wrote:
>> Set sensor measurement off after probe fail in pm_runtime_set_active() or
>> iio_device_register(). Without this change sensor measurement stays on
>> even though probe fails on these calls.
>>
>> This is maybe rare case, but causes constant power drain without any
>> benefits when it happens. Power drain is 20-500uA, typically 180uA.
>>
>> Signed-off-by: Mikko Koivunen <mikko.koivunen@fi.rohmeurope.com>
> Looks good to me, but I will be looking for Acks from Daniel on this
> set.  Daniel let me know if you want to pass on responsibility for the
> driver (perhaps Mikko if willing?)
>
> one tiny point inline.
>> ---
>> Sensor measurement is turned on rpr0521_init() so in probe it should be
>> turned off in error cases after that. With CONFIG_PM it's probably
>> unnecessary for iio_device_register()-fail, but without CONFIG_PM it is
>> needed. Writing power off twice when CONFIG_PM enabled is ok.
>>
>>  drivers/iio/light/rpr0521.c |   17 +++++++++++++++--
>>  1 file changed, 15 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/iio/light/rpr0521.c b/drivers/iio/light/rpr0521.c
>> index 03504f6..5d077f4 100644
>> --- a/drivers/iio/light/rpr0521.c
>> +++ b/drivers/iio/light/rpr0521.c
>> @@ -516,13 +516,26 @@ static int rpr0521_probe(struct i2c_client *client,
>>  
>>  	ret = pm_runtime_set_active(&client->dev);
>>  	if (ret < 0)
>> -		return ret;
>> +		goto err_poweroff;
>>  
>>  	pm_runtime_enable(&client->dev);
>>  	pm_runtime_set_autosuspend_delay(&client->dev, RPR0521_SLEEP_DELAY_MS);
>>  	pm_runtime_use_autosuspend(&client->dev);
>>  
>> -	return iio_device_register(indio_dev);
>> +	ret = iio_device_register(indio_dev);
>> +	if (ret)
>> +		goto err_pm_disable;
>> +
>> +	return 0;
>> +
>> +err_pm_disable:
>> +	pm_runtime_disable(&client->dev);
>> +	pm_runtime_set_suspended(&client->dev);
>> +	pm_runtime_put_noidle(&client->dev);
>> +err_poweroff:
>> +	(void) rpr0521_poweroff(data);
> shouldn't need the void cast.

It is there because discarding the return value is done on purpose, not
by accident. Should I still remove it?

>> +
>> +	return ret;
>>  }
>>  
>>  static int rpr0521_remove(struct i2c_client *client)
>>
>

Re: [PATCH v3 2/9] iio: light: rpr0521 poweroff for probe fails

[Jonathan Cameron]

On 15/05/17 11:32, Koivunen, Mikko wrote:
> On 7.5.2017 14:42, Jonathan Cameron wrote:
>> On 05/05/17 12:19, Mikko Koivunen wrote:
>>> Set sensor measurement off after probe fail in pm_runtime_set_active() or
>>> iio_device_register(). Without this change sensor measurement stays on
>>> even though probe fails on these calls.
>>>
>>> This is maybe rare case, but causes constant power drain without any
>>> benefits when it happens. Power drain is 20-500uA, typically 180uA.
>>>
>>> Signed-off-by: Mikko Koivunen <mikko.koivunen@fi.rohmeurope.com>
>> Looks good to me, but I will be looking for Acks from Daniel on this
>> set.  Daniel let me know if you want to pass on responsibility for the
>> driver (perhaps Mikko if willing?)
>>
>> one tiny point inline.
>>> ---
>>> Sensor measurement is turned on rpr0521_init() so in probe it should be
>>> turned off in error cases after that. With CONFIG_PM it's probably
>>> unnecessary for iio_device_register()-fail, but without CONFIG_PM it is
>>> needed. Writing power off twice when CONFIG_PM enabled is ok.
>>>
>>>   drivers/iio/light/rpr0521.c |   17 +++++++++++++++--
>>>   1 file changed, 15 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/iio/light/rpr0521.c b/drivers/iio/light/rpr0521.c
>>> index 03504f6..5d077f4 100644
>>> --- a/drivers/iio/light/rpr0521.c
>>> +++ b/drivers/iio/light/rpr0521.c
>>> @@ -516,13 +516,26 @@ static int rpr0521_probe(struct i2c_client *client,
>>>   
>>>   	ret = pm_runtime_set_active(&client->dev);
>>>   	if (ret < 0)
>>> -		return ret;
>>> +		goto err_poweroff;
>>>   
>>>   	pm_runtime_enable(&client->dev);
>>>   	pm_runtime_set_autosuspend_delay(&client->dev, RPR0521_SLEEP_DELAY_MS);
>>>   	pm_runtime_use_autosuspend(&client->dev);
>>>   
>>> -	return iio_device_register(indio_dev);
>>> +	ret = iio_device_register(indio_dev);
>>> +	if (ret)
>>> +		goto err_pm_disable;
>>> +
>>> +	return 0;
>>> +
>>> +err_pm_disable:
>>> +	pm_runtime_disable(&client->dev);
>>> +	pm_runtime_set_suspended(&client->dev);
>>> +	pm_runtime_put_noidle(&client->dev);
>>> +err_poweroff:
>>> +	(void) rpr0521_poweroff(data);
>> shouldn't need the void cast.
> 
> It is there because discarding the return value is done on purpose, not
> by accident. Should I still remove it?
> 
In an error path it's normal not to check so I don't think it needs
to be explicit...
>>> +
>>> +	return ret;
>>>   }
>>>   
>>>   static int rpr0521_remove(struct i2c_client *client)
>>>
>>
> 
> --
> 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
> 

Re: [PATCH v3 2/9] iio: light: rpr0521 poweroff for probe fails

[Koivunen, Mikko]

On 16.5.2017 21:07, Jonathan Cameron wrote:
> On 15/05/17 11:32, Koivunen, Mikko wrote:
>> On 7.5.2017 14:42, Jonathan Cameron wrote:
>>> On 05/05/17 12:19, Mikko Koivunen wrote:
>>>> Set sensor measurement off after probe fail in pm_runtime_set_active() or
>>>> iio_device_register(). Without this change sensor measurement stays on
>>>> even though probe fails on these calls.
>>>>
>>>> This is maybe rare case, but causes constant power drain without any
>>>> benefits when it happens. Power drain is 20-500uA, typically 180uA.
>>>>
>>>> Signed-off-by: Mikko Koivunen <mikko.koivunen@fi.rohmeurope.com>
>>> Looks good to me, but I will be looking for Acks from Daniel on this
>>> set.  Daniel let me know if you want to pass on responsibility for the
>>> driver (perhaps Mikko if willing?)
>>>
>>> one tiny point inline.
>>>> ---
>>>> Sensor measurement is turned on rpr0521_init() so in probe it should be
>>>> turned off in error cases after that. With CONFIG_PM it's probably
>>>> unnecessary for iio_device_register()-fail, but without CONFIG_PM it is
>>>> needed. Writing power off twice when CONFIG_PM enabled is ok.
>>>>
>>>>   drivers/iio/light/rpr0521.c |   17 +++++++++++++++--
>>>>   1 file changed, 15 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/drivers/iio/light/rpr0521.c b/drivers/iio/light/rpr0521.c
>>>> index 03504f6..5d077f4 100644
>>>> --- a/drivers/iio/light/rpr0521.c
>>>> +++ b/drivers/iio/light/rpr0521.c
>>>> @@ -516,13 +516,26 @@ static int rpr0521_probe(struct i2c_client *client,
>>>>   
>>>>   	ret = pm_runtime_set_active(&client->dev);
>>>>   	if (ret < 0)
>>>> -		return ret;
>>>> +		goto err_poweroff;
>>>>   
>>>>   	pm_runtime_enable(&client->dev);
>>>>   	pm_runtime_set_autosuspend_delay(&client->dev, RPR0521_SLEEP_DELAY_MS);
>>>>   	pm_runtime_use_autosuspend(&client->dev);
>>>>   
>>>> -	return iio_device_register(indio_dev);
>>>> +	ret = iio_device_register(indio_dev);
>>>> +	if (ret)
>>>> +		goto err_pm_disable;
>>>> +
>>>> +	return 0;
>>>> +
>>>> +err_pm_disable:
>>>> +	pm_runtime_disable(&client->dev);
>>>> +	pm_runtime_set_suspended(&client->dev);
>>>> +	pm_runtime_put_noidle(&client->dev);
>>>> +err_poweroff:
>>>> +	(void) rpr0521_poweroff(data);
>>> shouldn't need the void cast.
>> It is there because discarding the return value is done on purpose, not
>> by accident. Should I still remove it?
>>
> In an error path it's normal not to check so I don't think it needs
> to be explicit...

Ack.

>>>> +
>>>> +	return ret;
>>>>   }
>>>>   
>>>>   static int rpr0521_remove(struct i2c_client *client)
>>>>
>> --
>> 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
>>
>

Re: [PATCH v3 2/9] iio: light: rpr0521 poweroff for probe fails

[Daniel Baluta]

On Sun, May 7, 2017 at 2:42 PM, Jonathan Cameron <jic23@kernel.org> wrote:
> On 05/05/17 12:19, Mikko Koivunen wrote:
>> Set sensor measurement off after probe fail in pm_runtime_set_active() or
>> iio_device_register(). Without this change sensor measurement stays on
>> even though probe fails on these calls.
>>
>> This is maybe rare case, but causes constant power drain without any
>> benefits when it happens. Power drain is 20-500uA, typically 180uA.
>>
>> Signed-off-by: Mikko Koivunen <mikko.koivunen@fi.rohmeurope.com>
> Looks good to me, but I will be looking for Acks from Daniel on this
> set.  Daniel let me know if you want to pass on responsibility for the
> driver (perhaps Mikko if willing?)

This patchset looks good to me. Of course, if Mikko wants to take care of this
in the future :).

You can add my:

Acked-by: Daniel Baluta <daniel.baluta@nxp.com>

[PATCH v3 3/9] iio: light: rpr0521 on-off sequence change for CONFIG_PM

[Mikko Koivunen]

Refactor _set_power_state(), _resume() and _suspend().
Enable measurement only when needed, not in _init(). System can suspend
during measurement and measurement is continued on resume.
Pm turns off measurement when both ps and als measurements are disabled for
2 seconds. During off-time the power save is 20-500mA, typically 180mA.

Signed-off-by: Mikko Koivunen <mikko.koivunen@fi.rohmeurope.com>
---
Don't enable measurement on _init() when using CONFIG_PM. Enable only
when needed, otherwise it messes up the pm suspend-resume. Polling
enables/disables the measurement anyway.

Save ALS/PS enabled status on _suspend() when called during measurement,
so that measurement can be re-enabled on _resume().

checkpatch.pl OK
Tested on LeMaker HiKey with AOSP7.1 kernel 4.4.
Builds ok with torvalds/linux feb 27.

 drivers/iio/light/rpr0521.c |   70 ++++++++++++++++++++++++++++---------------
 1 file changed, 46 insertions(+), 24 deletions(-)

diff --git a/drivers/iio/light/rpr0521.c b/drivers/iio/light/rpr0521.c
index 5d077f4..02ce635 100644
--- a/drivers/iio/light/rpr0521.c
+++ b/drivers/iio/light/rpr0521.c
@@ -9,7 +9,7 @@
  *
  * IIO driver for RPR-0521RS (7-bit I2C slave address 0x38).
  *
- * TODO: illuminance channel, PM support, buffer
+ * TODO: illuminance channel, buffer
  */
 
 #include <linux/module.h>
@@ -142,9 +142,11 @@ struct rpr0521_data {
 	bool als_dev_en;
 	bool pxs_dev_en;
 
-	/* optimize runtime pm ops - enable device only if needed */
+	/* optimize runtime pm ops - enable/disable device only if needed */
 	bool als_ps_need_en;
 	bool pxs_ps_need_en;
+	bool als_need_dis;
+	bool pxs_need_dis;
 
 	struct regmap *regmap;
 };
@@ -230,40 +232,32 @@ static int rpr0521_pxs_enable(struct rpr0521_data *data, u8 status)
  * @on: state to be set for devices in @device_mask
  * @device_mask: bitmask specifying for which device we need to update @on state
  *
- * We rely on rpr0521_runtime_resume to enable our @device_mask devices, but
- * if (for example) PXS was enabled (pxs_dev_en = true) by a previous call to
- * rpr0521_runtime_resume and we want to enable ALS we MUST set ALS enable
- * bit of RPR0521_REG_MODE_CTRL here because rpr0521_runtime_resume will not
- * be called twice.
+ * Calls for this function must be balanced so that each ON should have matching
+ * OFF. Otherwise pm usage_count gets out of sync.
  */
 static int rpr0521_set_power_state(struct rpr0521_data *data, bool on,
 				   u8 device_mask)
 {
 #ifdef CONFIG_PM
 	int ret;
-	u8 update_mask = 0;
 
 	if (device_mask & RPR0521_MODE_ALS_MASK) {
-		if (on && !data->als_ps_need_en && data->pxs_dev_en)
-			update_mask |= RPR0521_MODE_ALS_MASK;
-		else
-			data->als_ps_need_en = on;
+		data->als_ps_need_en = on;
+		data->als_need_dis = !on;
 	}
 
 	if (device_mask & RPR0521_MODE_PXS_MASK) {
-		if (on && !data->pxs_ps_need_en && data->als_dev_en)
-			update_mask |= RPR0521_MODE_PXS_MASK;
-		else
-			data->pxs_ps_need_en = on;
-	}
-
-	if (update_mask) {
-		ret = regmap_update_bits(data->regmap, RPR0521_REG_MODE_CTRL,
-					 update_mask, update_mask);
-		if (ret < 0)
-			return ret;
+		data->pxs_ps_need_en = on;
+		data->pxs_need_dis = !on;
 	}
 
+	/*
+	 * On: _resume() is called only when we are suspended
+	 * Off: _suspend() is called after delay if _resume() is not
+	 * called before that.
+	 * Note: If either measurement is re-enabled before _suspend(),
+	 * both stay enabled until _suspend().
+	 */
 	if (on) {
 		ret = pm_runtime_get_sync(&data->client->dev);
 	} else {
@@ -279,6 +273,23 @@ static int rpr0521_set_power_state(struct rpr0521_data *data, bool on,
 
 		return ret;
 	}
+
+	if (on) {
+		/* If _resume() was not called, enable measurement now. */
+		if (data->als_ps_need_en) {
+			ret = rpr0521_als_enable(data, RPR0521_MODE_ALS_ENABLE);
+			if (ret)
+				return ret;
+			data->als_ps_need_en = false;
+		}
+
+		if (data->pxs_ps_need_en) {
+			ret = rpr0521_pxs_enable(data, RPR0521_MODE_PXS_ENABLE);
+			if (ret)
+				return ret;
+			data->pxs_ps_need_en = false;
+		}
+	}
 #endif
 	return 0;
 }
@@ -425,12 +436,14 @@ static int rpr0521_init(struct rpr0521_data *data)
 		return ret;
 	}
 
+#ifndef CONFIG_PM
 	ret = rpr0521_als_enable(data, RPR0521_MODE_ALS_ENABLE);
 	if (ret < 0)
 		return ret;
 	ret = rpr0521_pxs_enable(data, RPR0521_MODE_PXS_ENABLE);
 	if (ret < 0)
 		return ret;
+#endif
 
 	return 0;
 }
@@ -560,9 +573,16 @@ static int rpr0521_runtime_suspend(struct device *dev)
 	struct rpr0521_data *data = iio_priv(indio_dev);
 	int ret;
 
-	/* disable channels and sets {als,pxs}_dev_en to false */
 	mutex_lock(&data->lock);
+	/* If measurements are enabled, enable them on resume */
+	if (!data->als_need_dis)
+		data->als_ps_need_en = data->als_dev_en;
+	if (!data->pxs_need_dis)
+		data->pxs_ps_need_en = data->pxs_dev_en;
+
+	/* disable channels and sets {als,pxs}_dev_en to false */
 	ret = rpr0521_poweroff(data);
+	regcache_mark_dirty(data->regmap);
 	mutex_unlock(&data->lock);
 
 	return ret;
@@ -574,6 +594,7 @@ static int rpr0521_runtime_resume(struct device *dev)
 	struct rpr0521_data *data = iio_priv(indio_dev);
 	int ret;
 
+	regcache_sync(data->regmap);
 	if (data->als_ps_need_en) {
 		ret = rpr0521_als_enable(data, RPR0521_MODE_ALS_ENABLE);
 		if (ret < 0)
@@ -587,6 +608,7 @@ static int rpr0521_runtime_resume(struct device *dev)
 			return ret;
 		data->pxs_ps_need_en = false;
 	}
+	msleep(100);	//wait for first measurement result
 
 	return 0;
 }
-- 
1.7.9.5

Re: [PATCH v3 3/9] iio: light: rpr0521 on-off sequence change for CONFIG_PM

[Jonathan Cameron]

On 05/05/17 12:19, Mikko Koivunen wrote:
> Refactor _set_power_state(), _resume() and _suspend().
> Enable measurement only when needed, not in _init(). System can suspend
> during measurement and measurement is continued on resume.
> Pm turns off measurement when both ps and als measurements are disabled for
> 2 seconds. During off-time the power save is 20-500mA, typically 180mA.
> 
> Signed-off-by: Mikko Koivunen <mikko.koivunen@fi.rohmeurope.com>
This looks good to me if a little convoluted.  I do wonder how many
people are actually building without CONFIG_PM any more... Maybe we
should just make that a dependency in new drivers where this sort
of thing is needed.  Obviously trickier in drivers once they are in
as there may be platforms relying on it working without PM support.

Anyhow, Ack from Daniel ideally!

Jonathan
> ---
> Don't enable measurement on _init() when using CONFIG_PM. Enable only
> when needed, otherwise it messes up the pm suspend-resume. Polling
> enables/disables the measurement anyway.
> 
> Save ALS/PS enabled status on _suspend() when called during measurement,
> so that measurement can be re-enabled on _resume().
> 
> checkpatch.pl OK
> Tested on LeMaker HiKey with AOSP7.1 kernel 4.4.
> Builds ok with torvalds/linux feb 27.
> 
>  drivers/iio/light/rpr0521.c |   70 ++++++++++++++++++++++++++++---------------
>  1 file changed, 46 insertions(+), 24 deletions(-)
> 
> diff --git a/drivers/iio/light/rpr0521.c b/drivers/iio/light/rpr0521.c
> index 5d077f4..02ce635 100644
> --- a/drivers/iio/light/rpr0521.c
> +++ b/drivers/iio/light/rpr0521.c
> @@ -9,7 +9,7 @@
>   *
>   * IIO driver for RPR-0521RS (7-bit I2C slave address 0x38).
>   *
> - * TODO: illuminance channel, PM support, buffer
> + * TODO: illuminance channel, buffer
>   */
>  
>  #include <linux/module.h>
> @@ -142,9 +142,11 @@ struct rpr0521_data {
>  	bool als_dev_en;
>  	bool pxs_dev_en;
>  
> -	/* optimize runtime pm ops - enable device only if needed */
> +	/* optimize runtime pm ops - enable/disable device only if needed */
>  	bool als_ps_need_en;
>  	bool pxs_ps_need_en;
> +	bool als_need_dis;
> +	bool pxs_need_dis;
>  
>  	struct regmap *regmap;
>  };
> @@ -230,40 +232,32 @@ static int rpr0521_pxs_enable(struct rpr0521_data *data, u8 status)
>   * @on: state to be set for devices in @device_mask
>   * @device_mask: bitmask specifying for which device we need to update @on state
>   *
> - * We rely on rpr0521_runtime_resume to enable our @device_mask devices, but
> - * if (for example) PXS was enabled (pxs_dev_en = true) by a previous call to
> - * rpr0521_runtime_resume and we want to enable ALS we MUST set ALS enable
> - * bit of RPR0521_REG_MODE_CTRL here because rpr0521_runtime_resume will not
> - * be called twice.
> + * Calls for this function must be balanced so that each ON should have matching
> + * OFF. Otherwise pm usage_count gets out of sync.
>   */
>  static int rpr0521_set_power_state(struct rpr0521_data *data, bool on,
>  				   u8 device_mask)
>  {
>  #ifdef CONFIG_PM
>  	int ret;
> -	u8 update_mask = 0;
>  
>  	if (device_mask & RPR0521_MODE_ALS_MASK) {
> -		if (on && !data->als_ps_need_en && data->pxs_dev_en)
> -			update_mask |= RPR0521_MODE_ALS_MASK;
> -		else
> -			data->als_ps_need_en = on;
> +		data->als_ps_need_en = on;
> +		data->als_need_dis = !on;
>  	}
>  
>  	if (device_mask & RPR0521_MODE_PXS_MASK) {
> -		if (on && !data->pxs_ps_need_en && data->als_dev_en)
> -			update_mask |= RPR0521_MODE_PXS_MASK;
> -		else
> -			data->pxs_ps_need_en = on;
> -	}
> -
> -	if (update_mask) {
> -		ret = regmap_update_bits(data->regmap, RPR0521_REG_MODE_CTRL,
> -					 update_mask, update_mask);
> -		if (ret < 0)
> -			return ret;
> +		data->pxs_ps_need_en = on;
> +		data->pxs_need_dis = !on;
>  	}
>  
> +	/*
> +	 * On: _resume() is called only when we are suspended
> +	 * Off: _suspend() is called after delay if _resume() is not
> +	 * called before that.
> +	 * Note: If either measurement is re-enabled before _suspend(),
> +	 * both stay enabled until _suspend().
> +	 */
>  	if (on) {
>  		ret = pm_runtime_get_sync(&data->client->dev);
>  	} else {
> @@ -279,6 +273,23 @@ static int rpr0521_set_power_state(struct rpr0521_data *data, bool on,
>  
>  		return ret;
>  	}
> +
> +	if (on) {
> +		/* If _resume() was not called, enable measurement now. */
> +		if (data->als_ps_need_en) {
> +			ret = rpr0521_als_enable(data, RPR0521_MODE_ALS_ENABLE);
> +			if (ret)
> +				return ret;
> +			data->als_ps_need_en = false;
> +		}
> +
> +		if (data->pxs_ps_need_en) {
> +			ret = rpr0521_pxs_enable(data, RPR0521_MODE_PXS_ENABLE);
> +			if (ret)
> +				return ret;
> +			data->pxs_ps_need_en = false;
> +		}
> +	}
>  #endif
>  	return 0;
>  }
> @@ -425,12 +436,14 @@ static int rpr0521_init(struct rpr0521_data *data)
>  		return ret;
>  	}
>  
> +#ifndef CONFIG_PM
>  	ret = rpr0521_als_enable(data, RPR0521_MODE_ALS_ENABLE);
>  	if (ret < 0)
>  		return ret;
>  	ret = rpr0521_pxs_enable(data, RPR0521_MODE_PXS_ENABLE);
>  	if (ret < 0)
>  		return ret;
> +#endif
>  
>  	return 0;
>  }
> @@ -560,9 +573,16 @@ static int rpr0521_runtime_suspend(struct device *dev)
>  	struct rpr0521_data *data = iio_priv(indio_dev);
>  	int ret;
>  
> -	/* disable channels and sets {als,pxs}_dev_en to false */
>  	mutex_lock(&data->lock);
> +	/* If measurements are enabled, enable them on resume */
> +	if (!data->als_need_dis)
> +		data->als_ps_need_en = data->als_dev_en;
> +	if (!data->pxs_need_dis)
> +		data->pxs_ps_need_en = data->pxs_dev_en;
> +
> +	/* disable channels and sets {als,pxs}_dev_en to false */
>  	ret = rpr0521_poweroff(data);
> +	regcache_mark_dirty(data->regmap);
>  	mutex_unlock(&data->lock);
>  
>  	return ret;
> @@ -574,6 +594,7 @@ static int rpr0521_runtime_resume(struct device *dev)
>  	struct rpr0521_data *data = iio_priv(indio_dev);
>  	int ret;
>  
> +	regcache_sync(data->regmap);
>  	if (data->als_ps_need_en) {
>  		ret = rpr0521_als_enable(data, RPR0521_MODE_ALS_ENABLE);
>  		if (ret < 0)
> @@ -587,6 +608,7 @@ static int rpr0521_runtime_resume(struct device *dev)
>  			return ret;
>  		data->pxs_ps_need_en = false;
>  	}
> +	msleep(100);	//wait for first measurement result
This one rather looks like a bug fix. Is it required even without the
pm changes? 
>  
>  	return 0;
>  }
> 

Re: [PATCH v3 3/9] iio: light: rpr0521 on-off sequence change for CONFIG_PM

[Koivunen, Mikko]

On 7.5.2017 14:48, Jonathan Cameron wrote:
> On 05/05/17 12:19, Mikko Koivunen wrote:
>> Refactor _set_power_state(), _resume() and _suspend().
>> Enable measurement only when needed, not in _init(). System can suspend
>> during measurement and measurement is continued on resume.
>> Pm turns off measurement when both ps and als measurements are disabled for
>> 2 seconds. During off-time the power save is 20-500mA, typically 180mA.
>>
>> Signed-off-by: Mikko Koivunen <mikko.koivunen@fi.rohmeurope.com>
> This looks good to me if a little convoluted.  I do wonder how many
> people are actually building without CONFIG_PM any more... Maybe we
> should just make that a dependency in new drivers where this sort
> of thing is needed.  Obviously trickier in drivers once they are in
> as there may be platforms relying on it working without PM support.
>
> Anyhow, Ack from Daniel ideally!
>
> Jonathan
>> ---
>> Don't enable measurement on _init() when using CONFIG_PM. Enable only
>> when needed, otherwise it messes up the pm suspend-resume. Polling
>> enables/disables the measurement anyway.
>>
>> Save ALS/PS enabled status on _suspend() when called during measurement,
>> so that measurement can be re-enabled on _resume().
>>
>> checkpatch.pl OK
>> Tested on LeMaker HiKey with AOSP7.1 kernel 4.4.
>> Builds ok with torvalds/linux feb 27.
>>
>>  drivers/iio/light/rpr0521.c |   70 ++++++++++++++++++++++++++++---------------
>>  1 file changed, 46 insertions(+), 24 deletions(-)
>>
>> diff --git a/drivers/iio/light/rpr0521.c b/drivers/iio/light/rpr0521.c
>> index 5d077f4..02ce635 100644
>> --- a/drivers/iio/light/rpr0521.c
>> +++ b/drivers/iio/light/rpr0521.c
>> @@ -9,7 +9,7 @@
>>   *
>>   * IIO driver for RPR-0521RS (7-bit I2C slave address 0x38).
>>   *
>> - * TODO: illuminance channel, PM support, buffer
>> + * TODO: illuminance channel, buffer
>>   */
>>  
>>  #include <linux/module.h>
>> @@ -142,9 +142,11 @@ struct rpr0521_data {
>>  	bool als_dev_en;
>>  	bool pxs_dev_en;
>>  
>> -	/* optimize runtime pm ops - enable device only if needed */
>> +	/* optimize runtime pm ops - enable/disable device only if needed */
>>  	bool als_ps_need_en;
>>  	bool pxs_ps_need_en;
>> +	bool als_need_dis;
>> +	bool pxs_need_dis;
>>  
>>  	struct regmap *regmap;
>>  };
>> @@ -230,40 +232,32 @@ static int rpr0521_pxs_enable(struct rpr0521_data *data, u8 status)
>>   * @on: state to be set for devices in @device_mask
>>   * @device_mask: bitmask specifying for which device we need to update @on state
>>   *
>> - * We rely on rpr0521_runtime_resume to enable our @device_mask devices, but
>> - * if (for example) PXS was enabled (pxs_dev_en = true) by a previous call to
>> - * rpr0521_runtime_resume and we want to enable ALS we MUST set ALS enable
>> - * bit of RPR0521_REG_MODE_CTRL here because rpr0521_runtime_resume will not
>> - * be called twice.
>> + * Calls for this function must be balanced so that each ON should have matching
>> + * OFF. Otherwise pm usage_count gets out of sync.
>>   */
>>  static int rpr0521_set_power_state(struct rpr0521_data *data, bool on,
>>  				   u8 device_mask)
>>  {
>>  #ifdef CONFIG_PM
>>  	int ret;
>> -	u8 update_mask = 0;
>>  
>>  	if (device_mask & RPR0521_MODE_ALS_MASK) {
>> -		if (on && !data->als_ps_need_en && data->pxs_dev_en)
>> -			update_mask |= RPR0521_MODE_ALS_MASK;
>> -		else
>> -			data->als_ps_need_en = on;
>> +		data->als_ps_need_en = on;
>> +		data->als_need_dis = !on;
>>  	}
>>  
>>  	if (device_mask & RPR0521_MODE_PXS_MASK) {
>> -		if (on && !data->pxs_ps_need_en && data->als_dev_en)
>> -			update_mask |= RPR0521_MODE_PXS_MASK;
>> -		else
>> -			data->pxs_ps_need_en = on;
>> -	}
>> -
>> -	if (update_mask) {
>> -		ret = regmap_update_bits(data->regmap, RPR0521_REG_MODE_CTRL,
>> -					 update_mask, update_mask);
>> -		if (ret < 0)
>> -			return ret;
>> +		data->pxs_ps_need_en = on;
>> +		data->pxs_need_dis = !on;
>>  	}
>>  
>> +	/*
>> +	 * On: _resume() is called only when we are suspended
>> +	 * Off: _suspend() is called after delay if _resume() is not
>> +	 * called before that.
>> +	 * Note: If either measurement is re-enabled before _suspend(),
>> +	 * both stay enabled until _suspend().
>> +	 */
>>  	if (on) {
>>  		ret = pm_runtime_get_sync(&data->client->dev);
>>  	} else {
>> @@ -279,6 +273,23 @@ static int rpr0521_set_power_state(struct rpr0521_data *data, bool on,
>>  
>>  		return ret;
>>  	}
>> +
>> +	if (on) {
>> +		/* If _resume() was not called, enable measurement now. */
>> +		if (data->als_ps_need_en) {
>> +			ret = rpr0521_als_enable(data, RPR0521_MODE_ALS_ENABLE);
>> +			if (ret)
>> +				return ret;
>> +			data->als_ps_need_en = false;
>> +		}
>> +
>> +		if (data->pxs_ps_need_en) {
>> +			ret = rpr0521_pxs_enable(data, RPR0521_MODE_PXS_ENABLE);
>> +			if (ret)
>> +				return ret;
>> +			data->pxs_ps_need_en = false;
>> +		}
>> +	}
>>  #endif
>>  	return 0;
>>  }
>> @@ -425,12 +436,14 @@ static int rpr0521_init(struct rpr0521_data *data)
>>  		return ret;
>>  	}
>>  
>> +#ifndef CONFIG_PM
>>  	ret = rpr0521_als_enable(data, RPR0521_MODE_ALS_ENABLE);
>>  	if (ret < 0)
>>  		return ret;
>>  	ret = rpr0521_pxs_enable(data, RPR0521_MODE_PXS_ENABLE);
>>  	if (ret < 0)
>>  		return ret;
>> +#endif
>>  
>>  	return 0;
>>  }
>> @@ -560,9 +573,16 @@ static int rpr0521_runtime_suspend(struct device *dev)
>>  	struct rpr0521_data *data = iio_priv(indio_dev);
>>  	int ret;
>>  
>> -	/* disable channels and sets {als,pxs}_dev_en to false */
>>  	mutex_lock(&data->lock);
>> +	/* If measurements are enabled, enable them on resume */
>> +	if (!data->als_need_dis)
>> +		data->als_ps_need_en = data->als_dev_en;
>> +	if (!data->pxs_need_dis)
>> +		data->pxs_ps_need_en = data->pxs_dev_en;
>> +
>> +	/* disable channels and sets {als,pxs}_dev_en to false */
>>  	ret = rpr0521_poweroff(data);
>> +	regcache_mark_dirty(data->regmap);
>>  	mutex_unlock(&data->lock);
>>  
>>  	return ret;
>> @@ -574,6 +594,7 @@ static int rpr0521_runtime_resume(struct device *dev)
>>  	struct rpr0521_data *data = iio_priv(indio_dev);
>>  	int ret;
>>  
>> +	regcache_sync(data->regmap);
>>  	if (data->als_ps_need_en) {
>>  		ret = rpr0521_als_enable(data, RPR0521_MODE_ALS_ENABLE);
>>  		if (ret < 0)
>> @@ -587,6 +608,7 @@ static int rpr0521_runtime_resume(struct device *dev)
>>  			return ret;
>>  		data->pxs_ps_need_en = false;
>>  	}
>> +	msleep(100);	//wait for first measurement result
> This one rather looks like a bug fix. Is it required even without the
> pm changes? 

Nope, not bug fix. It's from the real world where measurement actually
takes 100ms from "measurement on"-command :).
You could read before that too, but value will be 0 on startup and
otherwise the previous measured value. With this msleep() the first
value read will be actual correct value.

>>  
>>  	return 0;
>>  }
>>
>

Re: [PATCH v3 3/9] iio: light: rpr0521 on-off sequence change for CONFIG_PM

[Jonathan Cameron]

On 15/05/17 11:40, Koivunen, Mikko wrote:
> On 7.5.2017 14:48, Jonathan Cameron wrote:
>> On 05/05/17 12:19, Mikko Koivunen wrote:
>>> Refactor _set_power_state(), _resume() and _suspend().
>>> Enable measurement only when needed, not in _init(). System can suspend
>>> during measurement and measurement is continued on resume.
>>> Pm turns off measurement when both ps and als measurements are disabled for
>>> 2 seconds. During off-time the power save is 20-500mA, typically 180mA.
>>>
>>> Signed-off-by: Mikko Koivunen <mikko.koivunen@fi.rohmeurope.com>
>> This looks good to me if a little convoluted.  I do wonder how many
>> people are actually building without CONFIG_PM any more... Maybe we
>> should just make that a dependency in new drivers where this sort
>> of thing is needed.  Obviously trickier in drivers once they are in
>> as there may be platforms relying on it working without PM support.
>>
>> Anyhow, Ack from Daniel ideally!
>>
>> Jonathan
>>> ---
>>> Don't enable measurement on _init() when using CONFIG_PM. Enable only
>>> when needed, otherwise it messes up the pm suspend-resume. Polling
>>> enables/disables the measurement anyway.
>>>
>>> Save ALS/PS enabled status on _suspend() when called during measurement,
>>> so that measurement can be re-enabled on _resume().
>>>
>>> checkpatch.pl OK
>>> Tested on LeMaker HiKey with AOSP7.1 kernel 4.4.
>>> Builds ok with torvalds/linux feb 27.
>>>
>>>   drivers/iio/light/rpr0521.c |   70 ++++++++++++++++++++++++++++---------------
>>>   1 file changed, 46 insertions(+), 24 deletions(-)
>>>
>>> diff --git a/drivers/iio/light/rpr0521.c b/drivers/iio/light/rpr0521.c
>>> index 5d077f4..02ce635 100644
>>> --- a/drivers/iio/light/rpr0521.c
>>> +++ b/drivers/iio/light/rpr0521.c
>>> @@ -9,7 +9,7 @@
>>>    *
>>>    * IIO driver for RPR-0521RS (7-bit I2C slave address 0x38).
>>>    *
>>> - * TODO: illuminance channel, PM support, buffer
>>> + * TODO: illuminance channel, buffer
>>>    */
>>>   
>>>   #include <linux/module.h>
>>> @@ -142,9 +142,11 @@ struct rpr0521_data {
>>>   	bool als_dev_en;
>>>   	bool pxs_dev_en;
>>>   
>>> -	/* optimize runtime pm ops - enable device only if needed */
>>> +	/* optimize runtime pm ops - enable/disable device only if needed */
>>>   	bool als_ps_need_en;
>>>   	bool pxs_ps_need_en;
>>> +	bool als_need_dis;
>>> +	bool pxs_need_dis;
>>>   
>>>   	struct regmap *regmap;
>>>   };
>>> @@ -230,40 +232,32 @@ static int rpr0521_pxs_enable(struct rpr0521_data *data, u8 status)
>>>    * @on: state to be set for devices in @device_mask
>>>    * @device_mask: bitmask specifying for which device we need to update @on state
>>>    *
>>> - * We rely on rpr0521_runtime_resume to enable our @device_mask devices, but
>>> - * if (for example) PXS was enabled (pxs_dev_en = true) by a previous call to
>>> - * rpr0521_runtime_resume and we want to enable ALS we MUST set ALS enable
>>> - * bit of RPR0521_REG_MODE_CTRL here because rpr0521_runtime_resume will not
>>> - * be called twice.
>>> + * Calls for this function must be balanced so that each ON should have matching
>>> + * OFF. Otherwise pm usage_count gets out of sync.
>>>    */
>>>   static int rpr0521_set_power_state(struct rpr0521_data *data, bool on,
>>>   				   u8 device_mask)
>>>   {
>>>   #ifdef CONFIG_PM
>>>   	int ret;
>>> -	u8 update_mask = 0;
>>>   
>>>   	if (device_mask & RPR0521_MODE_ALS_MASK) {
>>> -		if (on && !data->als_ps_need_en && data->pxs_dev_en)
>>> -			update_mask |= RPR0521_MODE_ALS_MASK;
>>> -		else
>>> -			data->als_ps_need_en = on;
>>> +		data->als_ps_need_en = on;
>>> +		data->als_need_dis = !on;
>>>   	}
>>>   
>>>   	if (device_mask & RPR0521_MODE_PXS_MASK) {
>>> -		if (on && !data->pxs_ps_need_en && data->als_dev_en)
>>> -			update_mask |= RPR0521_MODE_PXS_MASK;
>>> -		else
>>> -			data->pxs_ps_need_en = on;
>>> -	}
>>> -
>>> -	if (update_mask) {
>>> -		ret = regmap_update_bits(data->regmap, RPR0521_REG_MODE_CTRL,
>>> -					 update_mask, update_mask);
>>> -		if (ret < 0)
>>> -			return ret;
>>> +		data->pxs_ps_need_en = on;
>>> +		data->pxs_need_dis = !on;
>>>   	}
>>>   
>>> +	/*
>>> +	 * On: _resume() is called only when we are suspended
>>> +	 * Off: _suspend() is called after delay if _resume() is not
>>> +	 * called before that.
>>> +	 * Note: If either measurement is re-enabled before _suspend(),
>>> +	 * both stay enabled until _suspend().
>>> +	 */
>>>   	if (on) {
>>>   		ret = pm_runtime_get_sync(&data->client->dev);
>>>   	} else {
>>> @@ -279,6 +273,23 @@ static int rpr0521_set_power_state(struct rpr0521_data *data, bool on,
>>>   
>>>   		return ret;
>>>   	}
>>> +
>>> +	if (on) {
>>> +		/* If _resume() was not called, enable measurement now. */
>>> +		if (data->als_ps_need_en) {
>>> +			ret = rpr0521_als_enable(data, RPR0521_MODE_ALS_ENABLE);
>>> +			if (ret)
>>> +				return ret;
>>> +			data->als_ps_need_en = false;
>>> +		}
>>> +
>>> +		if (data->pxs_ps_need_en) {
>>> +			ret = rpr0521_pxs_enable(data, RPR0521_MODE_PXS_ENABLE);
>>> +			if (ret)
>>> +				return ret;
>>> +			data->pxs_ps_need_en = false;
>>> +		}
>>> +	}
>>>   #endif
>>>   	return 0;
>>>   }
>>> @@ -425,12 +436,14 @@ static int rpr0521_init(struct rpr0521_data *data)
>>>   		return ret;
>>>   	}
>>>   
>>> +#ifndef CONFIG_PM
>>>   	ret = rpr0521_als_enable(data, RPR0521_MODE_ALS_ENABLE);
>>>   	if (ret < 0)
>>>   		return ret;
>>>   	ret = rpr0521_pxs_enable(data, RPR0521_MODE_PXS_ENABLE);
>>>   	if (ret < 0)
>>>   		return ret;
>>> +#endif
>>>   
>>>   	return 0;
>>>   }
>>> @@ -560,9 +573,16 @@ static int rpr0521_runtime_suspend(struct device *dev)
>>>   	struct rpr0521_data *data = iio_priv(indio_dev);
>>>   	int ret;
>>>   
>>> -	/* disable channels and sets {als,pxs}_dev_en to false */
>>>   	mutex_lock(&data->lock);
>>> +	/* If measurements are enabled, enable them on resume */
>>> +	if (!data->als_need_dis)
>>> +		data->als_ps_need_en = data->als_dev_en;
>>> +	if (!data->pxs_need_dis)
>>> +		data->pxs_ps_need_en = data->pxs_dev_en;
>>> +
>>> +	/* disable channels and sets {als,pxs}_dev_en to false */
>>>   	ret = rpr0521_poweroff(data);
>>> +	regcache_mark_dirty(data->regmap);
>>>   	mutex_unlock(&data->lock);
>>>   
>>>   	return ret;
>>> @@ -574,6 +594,7 @@ static int rpr0521_runtime_resume(struct device *dev)
>>>   	struct rpr0521_data *data = iio_priv(indio_dev);
>>>   	int ret;
>>>   
>>> +	regcache_sync(data->regmap);
>>>   	if (data->als_ps_need_en) {
>>>   		ret = rpr0521_als_enable(data, RPR0521_MODE_ALS_ENABLE);
>>>   		if (ret < 0)
>>> @@ -587,6 +608,7 @@ static int rpr0521_runtime_resume(struct device *dev)
>>>   			return ret;
>>>   		data->pxs_ps_need_en = false;
>>>   	}
>>> +	msleep(100);	//wait for first measurement result
>> This one rather looks like a bug fix. Is it required even without the
>> pm changes?
> 
> Nope, not bug fix. It's from the real world where measurement actually
> takes 100ms from "measurement on"-command :).
> You could read before that too, but value will be 0 on startup and
> otherwise the previous measured value. With this msleep() the first
> value read will be actual correct value.
Kind of a logic bug fix.  Be nice to push that one back to older
trees perhaps?

Thanks for the explanation.

Jonathan
> 
>>>   
>>>   	return 0;
>>>   }
>>>
>>
> 
> --
> 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
> 

Re: [PATCH v3 3/9] iio: light: rpr0521 on-off sequence change for CONFIG_PM

[Koivunen, Mikko]

On 16.5.2017 21:08, Jonathan Cameron wrote:
> On 15/05/17 11:40, Koivunen, Mikko wrote:
>> On 7.5.2017 14:48, Jonathan Cameron wrote:
>>> On 05/05/17 12:19, Mikko Koivunen wrote:
>>>> Refactor _set_power_state(), _resume() and _suspend().
>>>> Enable measurement only when needed, not in _init(). System can suspend
>>>> during measurement and measurement is continued on resume.
>>>> Pm turns off measurement when both ps and als measurements are disabled for
>>>> 2 seconds. During off-time the power save is 20-500mA, typically 180mA.
>>>>
>>>> Signed-off-by: Mikko Koivunen <mikko.koivunen@fi.rohmeurope.com>
>>> This looks good to me if a little convoluted.  I do wonder how many
>>> people are actually building without CONFIG_PM any more... Maybe we
>>> should just make that a dependency in new drivers where this sort
>>> of thing is needed.  Obviously trickier in drivers once they are in
>>> as there may be platforms relying on it working without PM support.
>>>
>>> Anyhow, Ack from Daniel ideally!
>>>
>>> Jonathan
>>>> ---
>>>> Don't enable measurement on _init() when using CONFIG_PM. Enable only
>>>> when needed, otherwise it messes up the pm suspend-resume. Polling
>>>> enables/disables the measurement anyway.
>>>>
>>>> Save ALS/PS enabled status on _suspend() when called during measurement,
>>>> so that measurement can be re-enabled on _resume().
>>>>
>>>> checkpatch.pl OK
>>>> Tested on LeMaker HiKey with AOSP7.1 kernel 4.4.
>>>> Builds ok with torvalds/linux feb 27.
>>>>
>>>>   drivers/iio/light/rpr0521.c |   70 ++++++++++++++++++++++++++++---------------
>>>>   1 file changed, 46 insertions(+), 24 deletions(-)
>>>>
>>>> diff --git a/drivers/iio/light/rpr0521.c b/drivers/iio/light/rpr0521.c
>>>> index 5d077f4..02ce635 100644
>>>> --- a/drivers/iio/light/rpr0521.c
>>>> +++ b/drivers/iio/light/rpr0521.c
>>>> @@ -9,7 +9,7 @@
>>>>    *
>>>>    * IIO driver for RPR-0521RS (7-bit I2C slave address 0x38).
>>>>    *
>>>> - * TODO: illuminance channel, PM support, buffer
>>>> + * TODO: illuminance channel, buffer
>>>>    */
>>>>   
>>>>   #include <linux/module.h>
>>>> @@ -142,9 +142,11 @@ struct rpr0521_data {
>>>>   	bool als_dev_en;
>>>>   	bool pxs_dev_en;
>>>>   
>>>> -	/* optimize runtime pm ops - enable device only if needed */
>>>> +	/* optimize runtime pm ops - enable/disable device only if needed */
>>>>   	bool als_ps_need_en;
>>>>   	bool pxs_ps_need_en;
>>>> +	bool als_need_dis;
>>>> +	bool pxs_need_dis;
>>>>   
>>>>   	struct regmap *regmap;
>>>>   };
>>>> @@ -230,40 +232,32 @@ static int rpr0521_pxs_enable(struct rpr0521_data *data, u8 status)
>>>>    * @on: state to be set for devices in @device_mask
>>>>    * @device_mask: bitmask specifying for which device we need to update @on state
>>>>    *
>>>> - * We rely on rpr0521_runtime_resume to enable our @device_mask devices, but
>>>> - * if (for example) PXS was enabled (pxs_dev_en = true) by a previous call to
>>>> - * rpr0521_runtime_resume and we want to enable ALS we MUST set ALS enable
>>>> - * bit of RPR0521_REG_MODE_CTRL here because rpr0521_runtime_resume will not
>>>> - * be called twice.
>>>> + * Calls for this function must be balanced so that each ON should have matching
>>>> + * OFF. Otherwise pm usage_count gets out of sync.
>>>>    */
>>>>   static int rpr0521_set_power_state(struct rpr0521_data *data, bool on,
>>>>   				   u8 device_mask)
>>>>   {
>>>>   #ifdef CONFIG_PM
>>>>   	int ret;
>>>> -	u8 update_mask = 0;
>>>>   
>>>>   	if (device_mask & RPR0521_MODE_ALS_MASK) {
>>>> -		if (on && !data->als_ps_need_en && data->pxs_dev_en)
>>>> -			update_mask |= RPR0521_MODE_ALS_MASK;
>>>> -		else
>>>> -			data->als_ps_need_en = on;
>>>> +		data->als_ps_need_en = on;
>>>> +		data->als_need_dis = !on;
>>>>   	}
>>>>   
>>>>   	if (device_mask & RPR0521_MODE_PXS_MASK) {
>>>> -		if (on && !data->pxs_ps_need_en && data->als_dev_en)
>>>> -			update_mask |= RPR0521_MODE_PXS_MASK;
>>>> -		else
>>>> -			data->pxs_ps_need_en = on;
>>>> -	}
>>>> -
>>>> -	if (update_mask) {
>>>> -		ret = regmap_update_bits(data->regmap, RPR0521_REG_MODE_CTRL,
>>>> -					 update_mask, update_mask);
>>>> -		if (ret < 0)
>>>> -			return ret;
>>>> +		data->pxs_ps_need_en = on;
>>>> +		data->pxs_need_dis = !on;
>>>>   	}
>>>>   
>>>> +	/*
>>>> +	 * On: _resume() is called only when we are suspended
>>>> +	 * Off: _suspend() is called after delay if _resume() is not
>>>> +	 * called before that.
>>>> +	 * Note: If either measurement is re-enabled before _suspend(),
>>>> +	 * both stay enabled until _suspend().
>>>> +	 */
>>>>   	if (on) {
>>>>   		ret = pm_runtime_get_sync(&data->client->dev);
>>>>   	} else {
>>>> @@ -279,6 +273,23 @@ static int rpr0521_set_power_state(struct rpr0521_data *data, bool on,
>>>>   
>>>>   		return ret;
>>>>   	}
>>>> +
>>>> +	if (on) {
>>>> +		/* If _resume() was not called, enable measurement now. */
>>>> +		if (data->als_ps_need_en) {
>>>> +			ret = rpr0521_als_enable(data, RPR0521_MODE_ALS_ENABLE);
>>>> +			if (ret)
>>>> +				return ret;
>>>> +			data->als_ps_need_en = false;
>>>> +		}
>>>> +
>>>> +		if (data->pxs_ps_need_en) {
>>>> +			ret = rpr0521_pxs_enable(data, RPR0521_MODE_PXS_ENABLE);
>>>> +			if (ret)
>>>> +				return ret;
>>>> +			data->pxs_ps_need_en = false;
>>>> +		}
>>>> +	}
>>>>   #endif
>>>>   	return 0;
>>>>   }
>>>> @@ -425,12 +436,14 @@ static int rpr0521_init(struct rpr0521_data *data)
>>>>   		return ret;
>>>>   	}
>>>>   
>>>> +#ifndef CONFIG_PM
>>>>   	ret = rpr0521_als_enable(data, RPR0521_MODE_ALS_ENABLE);
>>>>   	if (ret < 0)
>>>>   		return ret;
>>>>   	ret = rpr0521_pxs_enable(data, RPR0521_MODE_PXS_ENABLE);
>>>>   	if (ret < 0)
>>>>   		return ret;

msleep here?

>>>> +#endif
>>>>   
>>>>   	return 0;
>>>>   }
>>>> @@ -560,9 +573,16 @@ static int rpr0521_runtime_suspend(struct device *dev)
>>>>   	struct rpr0521_data *data = iio_priv(indio_dev);
>>>>   	int ret;
>>>>   
>>>> -	/* disable channels and sets {als,pxs}_dev_en to false */
>>>>   	mutex_lock(&data->lock);
>>>> +	/* If measurements are enabled, enable them on resume */
>>>> +	if (!data->als_need_dis)
>>>> +		data->als_ps_need_en = data->als_dev_en;
>>>> +	if (!data->pxs_need_dis)
>>>> +		data->pxs_ps_need_en = data->pxs_dev_en;
>>>> +
>>>> +	/* disable channels and sets {als,pxs}_dev_en to false */
>>>>   	ret = rpr0521_poweroff(data);
>>>> +	regcache_mark_dirty(data->regmap);
>>>>   	mutex_unlock(&data->lock);
>>>>   
>>>>   	return ret;
>>>> @@ -574,6 +594,7 @@ static int rpr0521_runtime_resume(struct device *dev)
>>>>   	struct rpr0521_data *data = iio_priv(indio_dev);
>>>>   	int ret;
>>>>   
>>>> +	regcache_sync(data->regmap);
>>>>   	if (data->als_ps_need_en) {
>>>>   		ret = rpr0521_als_enable(data, RPR0521_MODE_ALS_ENABLE);
>>>>   		if (ret < 0)
>>>> @@ -587,6 +608,7 @@ static int rpr0521_runtime_resume(struct device *dev)
>>>>   			return ret;
>>>>   		data->pxs_ps_need_en = false;
>>>>   	}
>>>> +	msleep(100);	//wait for first measurement result
>>> This one rather looks like a bug fix. Is it required even without the
>>> pm changes?
>> Nope, not bug fix. It's from the real world where measurement actually
>> takes 100ms from "measurement on"-command :).
>> You could read before that too, but value will be 0 on startup and
>> otherwise the previous measured value. With this msleep() the first
>> value read will be actual correct value.
> Kind of a logic bug fix.  Be nice to push that one back to older
> trees perhaps?
>
> Thanks for the explanation.
>
> Jonathan

Hmm.. On a second thought, yes it is.
It could be added to init -function too. That would fix first 0-value
that is read within 100ms of probe start, regardless of CONFIG_PM.
Initially I thought it's not worth fixing, but now I don't know :).

>>>>   
>>>>   	return 0;
>>>>   }
>>>>
>> --
>> 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
>>
>

[PATCH v3 4/9] iio: light: rpr0521 magic number to sizeof() on value read

[Mikko Koivunen]

Changed magic number to sizeof() on value read.

Signed-off-by: Mikko Koivunen <mikko.koivunen@fi.rohmeurope.com>
---
 drivers/iio/light/rpr0521.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/iio/light/rpr0521.c b/drivers/iio/light/rpr0521.c
index 02ce635..d4d2d8c 100644
--- a/drivers/iio/light/rpr0521.c
+++ b/drivers/iio/light/rpr0521.c
@@ -356,7 +356,7 @@ static int rpr0521_read_raw(struct iio_dev *indio_dev,
 
 		ret = regmap_bulk_read(data->regmap,
 				       rpr0521_data_reg[chan->address].address,
-				       &raw_data, 2);
+				       &raw_data, sizeof(raw_data));
 		if (ret < 0) {
 			rpr0521_set_power_state(data, false, device_mask);
 			mutex_unlock(&data->lock);
-- 
1.7.9.5

[PATCH v3 5/9] iio: light: rpr0521 whitespace fixes

[Mikko Koivunen]

Just whitespace change, no functional changes.

Signed-off-by: Mikko Koivunen <mikko.koivunen@fi.rohmeurope.com>
---
 drivers/iio/light/rpr0521.c |    3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/iio/light/rpr0521.c b/drivers/iio/light/rpr0521.c
index d4d2d8c..da4b374 100644
--- a/drivers/iio/light/rpr0521.c
+++ b/drivers/iio/light/rpr0521.c
@@ -371,6 +371,7 @@ static int rpr0521_read_raw(struct iio_dev *indio_dev,
 		*val = le16_to_cpu(raw_data);
 
 		return IIO_VAL_INT;
+
 	case IIO_CHAN_INFO_SCALE:
 		mutex_lock(&data->lock);
 		ret = rpr0521_get_gain(data, chan->address, val, val2);
@@ -379,6 +380,7 @@ static int rpr0521_read_raw(struct iio_dev *indio_dev,
 			return ret;
 
 		return IIO_VAL_INT_PLUS_MICRO;
+
 	default:
 		return -EINVAL;
 	}
@@ -398,6 +400,7 @@ static int rpr0521_write_raw(struct iio_dev *indio_dev,
 		mutex_unlock(&data->lock);
 
 		return ret;
+
 	default:
 		return -EINVAL;
 	}
-- 
1.7.9.5

[PATCH v3 6/9] iio: light: rpr0521 sample_frequency read/write

[Mikko Koivunen]

Add sysfs read/write sample frequency.

Signed-off-by: Mikko Koivunen <mikko.koivunen@fi.rohmeurope.com>
---
Patch v1->v2 changes:
multiline comments fixed
Patch v2->v3 changes:
multiline comments fixed
Empty line fixes on switch-case.
Changed if-elseif-else to switch-case.

checkpatch.pl OK
Tested on LeMaker HiKey with AOSP7.1 kernel 4.4.
Builds ok with torvalds/linux feb 27.

 drivers/iio/light/rpr0521.c |  118 +++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 118 insertions(+)

diff --git a/drivers/iio/light/rpr0521.c b/drivers/iio/light/rpr0521.c
index da4b374..94a0d9d 100644
--- a/drivers/iio/light/rpr0521.c
+++ b/drivers/iio/light/rpr0521.c
@@ -132,6 +132,30 @@ static const struct rpr0521_gain_info {
 	},
 };
 
+struct rpr0521_samp_freq {
+	int	als_hz;
+	int	als_uhz;
+	int	pxs_hz;
+	int	pxs_uhz;
+};
+
+static const struct rpr0521_samp_freq rpr0521_samp_freq_i[13] = {
+/*	{ALS, PXS},		   W==currently writable option */
+	{0, 0, 0, 0},		/* W0000, 0=standby */
+	{0, 0, 100, 0},		/*  0001 */
+	{0, 0, 25, 0},		/*  0010 */
+	{0, 0, 10, 0},		/*  0011 */
+	{0, 0, 2, 500000},	/*  0100 */
+	{10, 0, 20, 0},		/*  0101 */
+	{10, 0, 10, 0},		/* W0110 */
+	{10, 0, 2, 500000},	/*  0111 */
+	{2, 500000, 20, 0},	/*  1000, measurement 100ms, sleep 300ms */
+	{2, 500000, 10, 0},	/*  1001, measurement 100ms, sleep 300ms */
+	{2, 500000, 0, 0},	/*  1010, high sensitivity mode */
+	{2, 500000, 2, 500000},	/* W1011, high sensitivity mode */
+	{20, 0, 20, 0}	/* 1100, ALS_data x 0.5, see specification P.18 */
+};
+
 struct rpr0521_data {
 	struct i2c_client *client;
 
@@ -154,9 +178,16 @@ struct rpr0521_data {
 static IIO_CONST_ATTR(in_intensity_scale_available, RPR0521_ALS_SCALE_AVAIL);
 static IIO_CONST_ATTR(in_proximity_scale_available, RPR0521_PXS_SCALE_AVAIL);
 
+/*
+ * Start with easy freq first, whole table of freq combinations is more
+ * complicated.
+ */
+static IIO_CONST_ATTR_SAMP_FREQ_AVAIL("2.5 10");
+
 static struct attribute *rpr0521_attributes[] = {
 	&iio_const_attr_in_intensity_scale_available.dev_attr.attr,
 	&iio_const_attr_in_proximity_scale_available.dev_attr.attr,
+	&iio_const_attr_sampling_frequency_available.dev_attr.attr,
 	NULL,
 };
 
@@ -172,6 +203,7 @@ static const struct iio_chan_spec rpr0521_channels[] = {
 		.channel2 = IIO_MOD_LIGHT_BOTH,
 		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
 			BIT(IIO_CHAN_INFO_SCALE),
+		.info_mask_shared_by_all = BIT(IIO_CHAN_INFO_SAMP_FREQ),
 	},
 	{
 		.type = IIO_INTENSITY,
@@ -180,12 +212,14 @@ static const struct iio_chan_spec rpr0521_channels[] = {
 		.channel2 = IIO_MOD_LIGHT_IR,
 		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
 			BIT(IIO_CHAN_INFO_SCALE),
+		.info_mask_shared_by_all = BIT(IIO_CHAN_INFO_SAMP_FREQ),
 	},
 	{
 		.type = IIO_PROXIMITY,
 		.address = RPR0521_CHAN_PXS,
 		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
 			BIT(IIO_CHAN_INFO_SCALE),
+		.info_mask_shared_by_all = BIT(IIO_CHAN_INFO_SAMP_FREQ),
 	}
 };
 
@@ -331,6 +365,72 @@ static int rpr0521_set_gain(struct rpr0521_data *data, int chan,
 				  idx << rpr0521_gain[chan].shift);
 }
 
+static int rpr0521_read_samp_freq(struct rpr0521_data *data,
+				enum iio_chan_type chan_type,
+			    int *val, int *val2)
+{
+	int reg, ret;
+
+	ret = regmap_read(data->regmap, RPR0521_REG_MODE_CTRL, &reg);
+	if (ret < 0)
+		return ret;
+
+	reg &= RPR0521_MODE_MEAS_TIME_MASK;
+	if (reg >= ARRAY_SIZE(rpr0521_samp_freq_i))
+		return -EINVAL;
+
+	switch (chan_type) {
+	case IIO_INTENSITY:
+		*val = rpr0521_samp_freq_i[reg].als_hz;
+		*val2 = rpr0521_samp_freq_i[reg].als_uhz;
+		return 0;
+
+	case IIO_PROXIMITY:
+		*val = rpr0521_samp_freq_i[reg].pxs_hz;
+		*val2 = rpr0521_samp_freq_i[reg].pxs_uhz;
+		return 0;
+
+	default:
+		return -EINVAL;
+	}
+}
+
+static int rpr0521_write_samp_freq_common(struct rpr0521_data *data,
+				enum iio_chan_type chan_type,
+				int val, int val2)
+{
+	int i;
+
+	/*
+	 * Ignore channel
+	 * both pxs and als are setup only to same freq because of simplicity
+	 */
+	switch (val) {
+	case 0:
+		i = 0;
+		break;
+
+	case 2:
+		if (val2 != 500000)
+			return -EINVAL;
+
+		i = 11;
+		break;
+
+	case 10:
+		i = 6;
+		break;
+
+	default:
+		return -EINVAL;
+	}
+
+	return regmap_update_bits(data->regmap,
+		RPR0521_REG_MODE_CTRL,
+		RPR0521_MODE_MEAS_TIME_MASK,
+		i);
+}
+
 static int rpr0521_read_raw(struct iio_dev *indio_dev,
 			    struct iio_chan_spec const *chan, int *val,
 			    int *val2, long mask)
@@ -381,6 +481,15 @@ static int rpr0521_read_raw(struct iio_dev *indio_dev,
 
 		return IIO_VAL_INT_PLUS_MICRO;
 
+	case IIO_CHAN_INFO_SAMP_FREQ:
+		mutex_lock(&data->lock);
+		ret = rpr0521_read_samp_freq(data, chan->type, val, val2);
+		mutex_unlock(&data->lock);
+		if (ret < 0)
+			return ret;
+
+		return IIO_VAL_INT_PLUS_MICRO;
+
 	default:
 		return -EINVAL;
 	}
@@ -401,6 +510,15 @@ static int rpr0521_write_raw(struct iio_dev *indio_dev,
 
 		return ret;
 
+	case IIO_CHAN_INFO_SAMP_FREQ:
+		mutex_lock(&data->lock);
+		ret = rpr0521_write_samp_freq_common(data,
+			chan->type,
+			val, val2);
+		mutex_unlock(&data->lock);
+
+		return ret;
+
 	default:
 		return -EINVAL;
 	}
-- 
1.7.9.5

Re: [PATCH v3 6/9] iio: light: rpr0521 sample_frequency read/write

[Jonathan Cameron]

On 05/05/17 12:19, Mikko Koivunen wrote:
> Add sysfs read/write sample frequency.
> 
> Signed-off-by: Mikko Koivunen <mikko.koivunen@fi.rohmeurope.com>
Trivial bit of formatting that could be improved
(mainly because it's a standard target for checkpatch lovers so
I'll only get a 'fix' a few days after this goes live if we don't fix
it now ;)

Jonathan
> ---
> Patch v1->v2 changes:
> multiline comments fixed
> Patch v2->v3 changes:
> multiline comments fixed
> Empty line fixes on switch-case.
> Changed if-elseif-else to switch-case.
> 
> checkpatch.pl OK
> Tested on LeMaker HiKey with AOSP7.1 kernel 4.4.
> Builds ok with torvalds/linux feb 27.
> 
>  drivers/iio/light/rpr0521.c |  118 +++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 118 insertions(+)
> 
> diff --git a/drivers/iio/light/rpr0521.c b/drivers/iio/light/rpr0521.c
> index da4b374..94a0d9d 100644
> --- a/drivers/iio/light/rpr0521.c
> +++ b/drivers/iio/light/rpr0521.c
> @@ -132,6 +132,30 @@ static const struct rpr0521_gain_info {
>  	},
>  };
>  
> +struct rpr0521_samp_freq {
> +	int	als_hz;
> +	int	als_uhz;
> +	int	pxs_hz;
> +	int	pxs_uhz;
> +};
> +
> +static const struct rpr0521_samp_freq rpr0521_samp_freq_i[13] = {
> +/*	{ALS, PXS},		   W==currently writable option */
> +	{0, 0, 0, 0},		/* W0000, 0=standby */
> +	{0, 0, 100, 0},		/*  0001 */
> +	{0, 0, 25, 0},		/*  0010 */
> +	{0, 0, 10, 0},		/*  0011 */
> +	{0, 0, 2, 500000},	/*  0100 */
> +	{10, 0, 20, 0},		/*  0101 */
> +	{10, 0, 10, 0},		/* W0110 */
> +	{10, 0, 2, 500000},	/*  0111 */
> +	{2, 500000, 20, 0},	/*  1000, measurement 100ms, sleep 300ms */
> +	{2, 500000, 10, 0},	/*  1001, measurement 100ms, sleep 300ms */
> +	{2, 500000, 0, 0},	/*  1010, high sensitivity mode */
> +	{2, 500000, 2, 500000},	/* W1011, high sensitivity mode */
> +	{20, 0, 20, 0}	/* 1100, ALS_data x 0.5, see specification P.18 */
> +};
> +
>  struct rpr0521_data {
>  	struct i2c_client *client;
>  
> @@ -154,9 +178,16 @@ struct rpr0521_data {
>  static IIO_CONST_ATTR(in_intensity_scale_available, RPR0521_ALS_SCALE_AVAIL);
>  static IIO_CONST_ATTR(in_proximity_scale_available, RPR0521_PXS_SCALE_AVAIL);
>  
> +/*
> + * Start with easy freq first, whole table of freq combinations is more
> + * complicated.
> + */
> +static IIO_CONST_ATTR_SAMP_FREQ_AVAIL("2.5 10");
> +
>  static struct attribute *rpr0521_attributes[] = {
>  	&iio_const_attr_in_intensity_scale_available.dev_attr.attr,
>  	&iio_const_attr_in_proximity_scale_available.dev_attr.attr,
> +	&iio_const_attr_sampling_frequency_available.dev_attr.attr,
>  	NULL,
>  };
>  
> @@ -172,6 +203,7 @@ static const struct iio_chan_spec rpr0521_channels[] = {
>  		.channel2 = IIO_MOD_LIGHT_BOTH,
>  		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
>  			BIT(IIO_CHAN_INFO_SCALE),
> +		.info_mask_shared_by_all = BIT(IIO_CHAN_INFO_SAMP_FREQ),
>  	},
>  	{
>  		.type = IIO_INTENSITY,
> @@ -180,12 +212,14 @@ static const struct iio_chan_spec rpr0521_channels[] = {
>  		.channel2 = IIO_MOD_LIGHT_IR,
>  		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
>  			BIT(IIO_CHAN_INFO_SCALE),
> +		.info_mask_shared_by_all = BIT(IIO_CHAN_INFO_SAMP_FREQ),
>  	},
>  	{
>  		.type = IIO_PROXIMITY,
>  		.address = RPR0521_CHAN_PXS,
>  		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
>  			BIT(IIO_CHAN_INFO_SCALE),
> +		.info_mask_shared_by_all = BIT(IIO_CHAN_INFO_SAMP_FREQ),
>  	}
>  };
>  
> @@ -331,6 +365,72 @@ static int rpr0521_set_gain(struct rpr0521_data *data, int chan,
>  				  idx << rpr0521_gain[chan].shift);
>  }
>  
> +static int rpr0521_read_samp_freq(struct rpr0521_data *data,
> +				enum iio_chan_type chan_type,
> +			    int *val, int *val2)
> +{
> +	int reg, ret;
> +
> +	ret = regmap_read(data->regmap, RPR0521_REG_MODE_CTRL, &reg);
> +	if (ret < 0)
> +		return ret;
> +
> +	reg &= RPR0521_MODE_MEAS_TIME_MASK;
> +	if (reg >= ARRAY_SIZE(rpr0521_samp_freq_i))
> +		return -EINVAL;
> +
> +	switch (chan_type) {
> +	case IIO_INTENSITY:
> +		*val = rpr0521_samp_freq_i[reg].als_hz;
> +		*val2 = rpr0521_samp_freq_i[reg].als_uhz;
> +		return 0;
> +
> +	case IIO_PROXIMITY:
> +		*val = rpr0521_samp_freq_i[reg].pxs_hz;
> +		*val2 = rpr0521_samp_freq_i[reg].pxs_uhz;
> +		return 0;
> +
> +	default:
> +		return -EINVAL;
> +	}
> +}
> +
> +static int rpr0521_write_samp_freq_common(struct rpr0521_data *data,
> +				enum iio_chan_type chan_type,
> +				int val, int val2)
> +{
> +	int i;
> +
> +	/*
> +	 * Ignore channel
> +	 * both pxs and als are setup only to same freq because of simplicity
> +	 */
> +	switch (val) {
> +	case 0:
> +		i = 0;
> +		break;
> +
> +	case 2:
> +		if (val2 != 500000)
> +			return -EINVAL;
> +
> +		i = 11;
> +		break;
> +
> +	case 10:
> +		i = 6;
> +		break;
> +
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	return regmap_update_bits(data->regmap,
> +		RPR0521_REG_MODE_CTRL,
> +		RPR0521_MODE_MEAS_TIME_MASK,
> +		i);
> +}
> +
>  static int rpr0521_read_raw(struct iio_dev *indio_dev,
>  			    struct iio_chan_spec const *chan, int *val,
>  			    int *val2, long mask)
> @@ -381,6 +481,15 @@ static int rpr0521_read_raw(struct iio_dev *indio_dev,
>  
>  		return IIO_VAL_INT_PLUS_MICRO;
>  
> +	case IIO_CHAN_INFO_SAMP_FREQ:
> +		mutex_lock(&data->lock);
> +		ret = rpr0521_read_samp_freq(data, chan->type, val, val2);
> +		mutex_unlock(&data->lock);
> +		if (ret < 0)
> +			return ret;
> +
> +		return IIO_VAL_INT_PLUS_MICRO;
> +
>  	default:
>  		return -EINVAL;
>  	}
> @@ -401,6 +510,15 @@ static int rpr0521_write_raw(struct iio_dev *indio_dev,
>  
>  		return ret;
>  
> +	case IIO_CHAN_INFO_SAMP_FREQ:
> +		mutex_lock(&data->lock);
> +		ret = rpr0521_write_samp_freq_common(data,
> +			chan->type,
> +			val, val2);
It's trivial but please align arguments to open bracket where possible.
> +		mutex_unlock(&data->lock);
> +
> +		return ret;
> +
>  	default:
>  		return -EINVAL;
>  	}
> 

Re: [PATCH v3 6/9] iio: light: rpr0521 sample_frequency read/write

[Koivunen, Mikko]

On 7.5.2017 14:51, Jonathan Cameron wrote:
> On 05/05/17 12:19, Mikko Koivunen wrote:
>> Add sysfs read/write sample frequency.
>>
>> Signed-off-by: Mikko Koivunen <mikko.koivunen@fi.rohmeurope.com>
> Trivial bit of formatting that could be improved
> (mainly because it's a standard target for checkpatch lovers so
> I'll only get a 'fix' a few days after this goes live if we don't fix
> it now ;)
>
> Jonathan

Ack.
Checkpatch didn't mark it for me.

>> ---
>> Patch v1->v2 changes:
>> multiline comments fixed
>> Patch v2->v3 changes:
>> multiline comments fixed
>> Empty line fixes on switch-case.
>> Changed if-elseif-else to switch-case.
>>
>> checkpatch.pl OK
>> Tested on LeMaker HiKey with AOSP7.1 kernel 4.4.
>> Builds ok with torvalds/linux feb 27.
>>
>>  drivers/iio/light/rpr0521.c |  118 +++++++++++++++++++++++++++++++++++++++++++
>>  1 file changed, 118 insertions(+)
>>
>> diff --git a/drivers/iio/light/rpr0521.c b/drivers/iio/light/rpr0521.c
>> index da4b374..94a0d9d 100644
>> --- a/drivers/iio/light/rpr0521.c
>> +++ b/drivers/iio/light/rpr0521.c
>> @@ -132,6 +132,30 @@ static const struct rpr0521_gain_info {
>>  	},
>>  };
>>  
>> +struct rpr0521_samp_freq {
>> +	int	als_hz;
>> +	int	als_uhz;
>> +	int	pxs_hz;
>> +	int	pxs_uhz;
>> +};
>> +
>> +static const struct rpr0521_samp_freq rpr0521_samp_freq_i[13] = {
>> +/*	{ALS, PXS},		   W==currently writable option */
>> +	{0, 0, 0, 0},		/* W0000, 0=standby */
>> +	{0, 0, 100, 0},		/*  0001 */
>> +	{0, 0, 25, 0},		/*  0010 */
>> +	{0, 0, 10, 0},		/*  0011 */
>> +	{0, 0, 2, 500000},	/*  0100 */
>> +	{10, 0, 20, 0},		/*  0101 */
>> +	{10, 0, 10, 0},		/* W0110 */
>> +	{10, 0, 2, 500000},	/*  0111 */
>> +	{2, 500000, 20, 0},	/*  1000, measurement 100ms, sleep 300ms */
>> +	{2, 500000, 10, 0},	/*  1001, measurement 100ms, sleep 300ms */
>> +	{2, 500000, 0, 0},	/*  1010, high sensitivity mode */
>> +	{2, 500000, 2, 500000},	/* W1011, high sensitivity mode */
>> +	{20, 0, 20, 0}	/* 1100, ALS_data x 0.5, see specification P.18 */
>> +};
>> +
>>  struct rpr0521_data {
>>  	struct i2c_client *client;
>>  
>> @@ -154,9 +178,16 @@ struct rpr0521_data {
>>  static IIO_CONST_ATTR(in_intensity_scale_available, RPR0521_ALS_SCALE_AVAIL);
>>  static IIO_CONST_ATTR(in_proximity_scale_available, RPR0521_PXS_SCALE_AVAIL);
>>  
>> +/*
>> + * Start with easy freq first, whole table of freq combinations is more
>> + * complicated.
>> + */
>> +static IIO_CONST_ATTR_SAMP_FREQ_AVAIL("2.5 10");
>> +
>>  static struct attribute *rpr0521_attributes[] = {
>>  	&iio_const_attr_in_intensity_scale_available.dev_attr.attr,
>>  	&iio_const_attr_in_proximity_scale_available.dev_attr.attr,
>> +	&iio_const_attr_sampling_frequency_available.dev_attr.attr,
>>  	NULL,
>>  };
>>  
>> @@ -172,6 +203,7 @@ static const struct iio_chan_spec rpr0521_channels[] = {
>>  		.channel2 = IIO_MOD_LIGHT_BOTH,
>>  		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
>>  			BIT(IIO_CHAN_INFO_SCALE),
>> +		.info_mask_shared_by_all = BIT(IIO_CHAN_INFO_SAMP_FREQ),
>>  	},
>>  	{
>>  		.type = IIO_INTENSITY,
>> @@ -180,12 +212,14 @@ static const struct iio_chan_spec rpr0521_channels[] = {
>>  		.channel2 = IIO_MOD_LIGHT_IR,
>>  		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
>>  			BIT(IIO_CHAN_INFO_SCALE),
>> +		.info_mask_shared_by_all = BIT(IIO_CHAN_INFO_SAMP_FREQ),
>>  	},
>>  	{
>>  		.type = IIO_PROXIMITY,
>>  		.address = RPR0521_CHAN_PXS,
>>  		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
>>  			BIT(IIO_CHAN_INFO_SCALE),
>> +		.info_mask_shared_by_all = BIT(IIO_CHAN_INFO_SAMP_FREQ),
>>  	}
>>  };
>>  
>> @@ -331,6 +365,72 @@ static int rpr0521_set_gain(struct rpr0521_data *data, int chan,
>>  				  idx << rpr0521_gain[chan].shift);
>>  }
>>  
>> +static int rpr0521_read_samp_freq(struct rpr0521_data *data,
>> +				enum iio_chan_type chan_type,
>> +			    int *val, int *val2)
>> +{
>> +	int reg, ret;
>> +
>> +	ret = regmap_read(data->regmap, RPR0521_REG_MODE_CTRL, &reg);
>> +	if (ret < 0)
>> +		return ret;
>> +
>> +	reg &= RPR0521_MODE_MEAS_TIME_MASK;
>> +	if (reg >= ARRAY_SIZE(rpr0521_samp_freq_i))
>> +		return -EINVAL;
>> +
>> +	switch (chan_type) {
>> +	case IIO_INTENSITY:
>> +		*val = rpr0521_samp_freq_i[reg].als_hz;
>> +		*val2 = rpr0521_samp_freq_i[reg].als_uhz;
>> +		return 0;
>> +
>> +	case IIO_PROXIMITY:
>> +		*val = rpr0521_samp_freq_i[reg].pxs_hz;
>> +		*val2 = rpr0521_samp_freq_i[reg].pxs_uhz;
>> +		return 0;
>> +
>> +	default:
>> +		return -EINVAL;
>> +	}
>> +}
>> +
>> +static int rpr0521_write_samp_freq_common(struct rpr0521_data *data,
>> +				enum iio_chan_type chan_type,
>> +				int val, int val2)
>> +{
>> +	int i;
>> +
>> +	/*
>> +	 * Ignore channel
>> +	 * both pxs and als are setup only to same freq because of simplicity
>> +	 */
>> +	switch (val) {
>> +	case 0:
>> +		i = 0;
>> +		break;
>> +
>> +	case 2:
>> +		if (val2 != 500000)
>> +			return -EINVAL;
>> +
>> +		i = 11;
>> +		break;
>> +
>> +	case 10:
>> +		i = 6;
>> +		break;
>> +
>> +	default:
>> +		return -EINVAL;
>> +	}
>> +
>> +	return regmap_update_bits(data->regmap,
>> +		RPR0521_REG_MODE_CTRL,
>> +		RPR0521_MODE_MEAS_TIME_MASK,
>> +		i);
>> +}
>> +
>>  static int rpr0521_read_raw(struct iio_dev *indio_dev,
>>  			    struct iio_chan_spec const *chan, int *val,
>>  			    int *val2, long mask)
>> @@ -381,6 +481,15 @@ static int rpr0521_read_raw(struct iio_dev *indio_dev,
>>  
>>  		return IIO_VAL_INT_PLUS_MICRO;
>>  
>> +	case IIO_CHAN_INFO_SAMP_FREQ:
>> +		mutex_lock(&data->lock);
>> +		ret = rpr0521_read_samp_freq(data, chan->type, val, val2);
>> +		mutex_unlock(&data->lock);
>> +		if (ret < 0)
>> +			return ret;
>> +
>> +		return IIO_VAL_INT_PLUS_MICRO;
>> +
>>  	default:
>>  		return -EINVAL;
>>  	}
>> @@ -401,6 +510,15 @@ static int rpr0521_write_raw(struct iio_dev *indio_dev,
>>  
>>  		return ret;
>>  
>> +	case IIO_CHAN_INFO_SAMP_FREQ:
>> +		mutex_lock(&data->lock);
>> +		ret = rpr0521_write_samp_freq_common(data,
>> +			chan->type,
>> +			val, val2);
> It's trivial but please align arguments to open bracket where possible.
>> +		mutex_unlock(&data->lock);
>> +
>> +		return ret;
>> +
>>  	default:
>>  		return -EINVAL;
>>  	}
>>
>

Re: [PATCH v3 6/9] iio: light: rpr0521 sample_frequency read/write

[Jonathan Cameron]

On 15/05/17 11:42, Koivunen, Mikko wrote:
> On 7.5.2017 14:51, Jonathan Cameron wrote:
>> On 05/05/17 12:19, Mikko Koivunen wrote:
>>> Add sysfs read/write sample frequency.
>>>
>>> Signed-off-by: Mikko Koivunen <mikko.koivunen@fi.rohmeurope.com>
>> Trivial bit of formatting that could be improved
>> (mainly because it's a standard target for checkpatch lovers so
>> I'll only get a 'fix' a few days after this goes live if we don't fix
>> it now ;)
>>
>> Jonathan
> 
> Ack.
> Checkpatch didn't mark it for me.
> 
Strange.  I wonder why it isn't?

Ah well - fragile script perhaps or one of the heuristics to get
rid of false positives is tripping...

Jonathan
>>> ---
>>> Patch v1->v2 changes:
>>> multiline comments fixed
>>> Patch v2->v3 changes:
>>> multiline comments fixed
>>> Empty line fixes on switch-case.
>>> Changed if-elseif-else to switch-case.
>>>
>>> checkpatch.pl OK
>>> Tested on LeMaker HiKey with AOSP7.1 kernel 4.4.
>>> Builds ok with torvalds/linux feb 27.
>>>
>>>   drivers/iio/light/rpr0521.c |  118 +++++++++++++++++++++++++++++++++++++++++++
>>>   1 file changed, 118 insertions(+)
>>>
>>> diff --git a/drivers/iio/light/rpr0521.c b/drivers/iio/light/rpr0521.c
>>> index da4b374..94a0d9d 100644
>>> --- a/drivers/iio/light/rpr0521.c
>>> +++ b/drivers/iio/light/rpr0521.c
>>> @@ -132,6 +132,30 @@ static const struct rpr0521_gain_info {
>>>   	},
>>>   };
>>>   
>>> +struct rpr0521_samp_freq {
>>> +	int	als_hz;
>>> +	int	als_uhz;
>>> +	int	pxs_hz;
>>> +	int	pxs_uhz;
>>> +};
>>> +
>>> +static const struct rpr0521_samp_freq rpr0521_samp_freq_i[13] = {
>>> +/*	{ALS, PXS},		   W==currently writable option */
>>> +	{0, 0, 0, 0},		/* W0000, 0=standby */
>>> +	{0, 0, 100, 0},		/*  0001 */
>>> +	{0, 0, 25, 0},		/*  0010 */
>>> +	{0, 0, 10, 0},		/*  0011 */
>>> +	{0, 0, 2, 500000},	/*  0100 */
>>> +	{10, 0, 20, 0},		/*  0101 */
>>> +	{10, 0, 10, 0},		/* W0110 */
>>> +	{10, 0, 2, 500000},	/*  0111 */
>>> +	{2, 500000, 20, 0},	/*  1000, measurement 100ms, sleep 300ms */
>>> +	{2, 500000, 10, 0},	/*  1001, measurement 100ms, sleep 300ms */
>>> +	{2, 500000, 0, 0},	/*  1010, high sensitivity mode */
>>> +	{2, 500000, 2, 500000},	/* W1011, high sensitivity mode */
>>> +	{20, 0, 20, 0}	/* 1100, ALS_data x 0.5, see specification P.18 */
>>> +};
>>> +
>>>   struct rpr0521_data {
>>>   	struct i2c_client *client;
>>>   
>>> @@ -154,9 +178,16 @@ struct rpr0521_data {
>>>   static IIO_CONST_ATTR(in_intensity_scale_available, RPR0521_ALS_SCALE_AVAIL);
>>>   static IIO_CONST_ATTR(in_proximity_scale_available, RPR0521_PXS_SCALE_AVAIL);
>>>   
>>> +/*
>>> + * Start with easy freq first, whole table of freq combinations is more
>>> + * complicated.
>>> + */
>>> +static IIO_CONST_ATTR_SAMP_FREQ_AVAIL("2.5 10");
>>> +
>>>   static struct attribute *rpr0521_attributes[] = {
>>>   	&iio_const_attr_in_intensity_scale_available.dev_attr.attr,
>>>   	&iio_const_attr_in_proximity_scale_available.dev_attr.attr,
>>> +	&iio_const_attr_sampling_frequency_available.dev_attr.attr,
>>>   	NULL,
>>>   };
>>>   
>>> @@ -172,6 +203,7 @@ static const struct iio_chan_spec rpr0521_channels[] = {
>>>   		.channel2 = IIO_MOD_LIGHT_BOTH,
>>>   		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
>>>   			BIT(IIO_CHAN_INFO_SCALE),
>>> +		.info_mask_shared_by_all = BIT(IIO_CHAN_INFO_SAMP_FREQ),
>>>   	},
>>>   	{
>>>   		.type = IIO_INTENSITY,
>>> @@ -180,12 +212,14 @@ static const struct iio_chan_spec rpr0521_channels[] = {
>>>   		.channel2 = IIO_MOD_LIGHT_IR,
>>>   		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
>>>   			BIT(IIO_CHAN_INFO_SCALE),
>>> +		.info_mask_shared_by_all = BIT(IIO_CHAN_INFO_SAMP_FREQ),
>>>   	},
>>>   	{
>>>   		.type = IIO_PROXIMITY,
>>>   		.address = RPR0521_CHAN_PXS,
>>>   		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
>>>   			BIT(IIO_CHAN_INFO_SCALE),
>>> +		.info_mask_shared_by_all = BIT(IIO_CHAN_INFO_SAMP_FREQ),
>>>   	}
>>>   };
>>>   
>>> @@ -331,6 +365,72 @@ static int rpr0521_set_gain(struct rpr0521_data *data, int chan,
>>>   				  idx << rpr0521_gain[chan].shift);
>>>   }
>>>   
>>> +static int rpr0521_read_samp_freq(struct rpr0521_data *data,
>>> +				enum iio_chan_type chan_type,
>>> +			    int *val, int *val2)
>>> +{
>>> +	int reg, ret;
>>> +
>>> +	ret = regmap_read(data->regmap, RPR0521_REG_MODE_CTRL, &reg);
>>> +	if (ret < 0)
>>> +		return ret;
>>> +
>>> +	reg &= RPR0521_MODE_MEAS_TIME_MASK;
>>> +	if (reg >= ARRAY_SIZE(rpr0521_samp_freq_i))
>>> +		return -EINVAL;
>>> +
>>> +	switch (chan_type) {
>>> +	case IIO_INTENSITY:
>>> +		*val = rpr0521_samp_freq_i[reg].als_hz;
>>> +		*val2 = rpr0521_samp_freq_i[reg].als_uhz;
>>> +		return 0;
>>> +
>>> +	case IIO_PROXIMITY:
>>> +		*val = rpr0521_samp_freq_i[reg].pxs_hz;
>>> +		*val2 = rpr0521_samp_freq_i[reg].pxs_uhz;
>>> +		return 0;
>>> +
>>> +	default:
>>> +		return -EINVAL;
>>> +	}
>>> +}
>>> +
>>> +static int rpr0521_write_samp_freq_common(struct rpr0521_data *data,
>>> +				enum iio_chan_type chan_type,
>>> +				int val, int val2)
>>> +{
>>> +	int i;
>>> +
>>> +	/*
>>> +	 * Ignore channel
>>> +	 * both pxs and als are setup only to same freq because of simplicity
>>> +	 */
>>> +	switch (val) {
>>> +	case 0:
>>> +		i = 0;
>>> +		break;
>>> +
>>> +	case 2:
>>> +		if (val2 != 500000)
>>> +			return -EINVAL;
>>> +
>>> +		i = 11;
>>> +		break;
>>> +
>>> +	case 10:
>>> +		i = 6;
>>> +		break;
>>> +
>>> +	default:
>>> +		return -EINVAL;
>>> +	}
>>> +
>>> +	return regmap_update_bits(data->regmap,
>>> +		RPR0521_REG_MODE_CTRL,
>>> +		RPR0521_MODE_MEAS_TIME_MASK,
>>> +		i);
>>> +}
>>> +
>>>   static int rpr0521_read_raw(struct iio_dev *indio_dev,
>>>   			    struct iio_chan_spec const *chan, int *val,
>>>   			    int *val2, long mask)
>>> @@ -381,6 +481,15 @@ static int rpr0521_read_raw(struct iio_dev *indio_dev,
>>>   
>>>   		return IIO_VAL_INT_PLUS_MICRO;
>>>   
>>> +	case IIO_CHAN_INFO_SAMP_FREQ:
>>> +		mutex_lock(&data->lock);
>>> +		ret = rpr0521_read_samp_freq(data, chan->type, val, val2);
>>> +		mutex_unlock(&data->lock);
>>> +		if (ret < 0)
>>> +			return ret;
>>> +
>>> +		return IIO_VAL_INT_PLUS_MICRO;
>>> +
>>>   	default:
>>>   		return -EINVAL;
>>>   	}
>>> @@ -401,6 +510,15 @@ static int rpr0521_write_raw(struct iio_dev *indio_dev,
>>>   
>>>   		return ret;
>>>   
>>> +	case IIO_CHAN_INFO_SAMP_FREQ:
>>> +		mutex_lock(&data->lock);
>>> +		ret = rpr0521_write_samp_freq_common(data,
>>> +			chan->type,
>>> +			val, val2);
>> It's trivial but please align arguments to open bracket where possible.
>>> +		mutex_unlock(&data->lock);
>>> +
>>> +		return ret;
>>> +
>>>   	default:
>>>   		return -EINVAL;
>>>   	}
>>>
>>
> 
> --
> 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
> 

[PATCH v3 7/9] iio: light: rpr0521 proximity offset read/write

[Mikko Koivunen]

Add sysfs read/write proximity offset feature. Offset is read/write from
sensor registers. Values are proximity raw 10-bit values. After applying
offset value, output values will be (measured_raw - offset_value). Output
values are unsigned so offset value doesn't make output negative.

Signed-off-by: Mikko Koivunen <mikko.koivunen@fi.rohmeurope.com>
---
Patch v1->v2 changes:
read/write_ps_offset() rewritten with:
 -  static, __le16, cpu_to_le16(), sizeof()
Patch v2->v3 changes:
RPR0521_*REG_*PS_OFFSET_LSB
removed RPR0521_REG_PS_OFFSET_MSB
Whitespace changes

checkpatch.pl OK
Tested on LeMaker HiKey with AOSP7.1 kernel 4.4.
Builds ok with torvalds/linux feb 27.

 drivers/iio/light/rpr0521.c |   52 +++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 52 insertions(+)

diff --git a/drivers/iio/light/rpr0521.c b/drivers/iio/light/rpr0521.c
index 94a0d9d..41b3023 100644
--- a/drivers/iio/light/rpr0521.c
+++ b/drivers/iio/light/rpr0521.c
@@ -30,6 +30,7 @@
 #define RPR0521_REG_PXS_DATA		0x44 /* 16-bit, little endian */
 #define RPR0521_REG_ALS_DATA0		0x46 /* 16-bit, little endian */
 #define RPR0521_REG_ALS_DATA1		0x48 /* 16-bit, little endian */
+#define RPR0521_REG_PS_OFFSET_LSB	0x53
 #define RPR0521_REG_ID			0x92
 
 #define RPR0521_MODE_ALS_MASK		BIT(7)
@@ -218,6 +219,7 @@ static const struct iio_chan_spec rpr0521_channels[] = {
 		.type = IIO_PROXIMITY,
 		.address = RPR0521_CHAN_PXS,
 		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
+			BIT(IIO_CHAN_INFO_OFFSET) |
 			BIT(IIO_CHAN_INFO_SCALE),
 		.info_mask_shared_by_all = BIT(IIO_CHAN_INFO_SAMP_FREQ),
 	}
@@ -431,6 +433,40 @@ static int rpr0521_write_samp_freq_common(struct rpr0521_data *data,
 		i);
 }
 
+static int rpr0521_read_ps_offset(struct rpr0521_data *data, int *offset)
+{
+	int ret;
+	__le16 buffer;
+
+	ret = regmap_bulk_read(data->regmap,
+		RPR0521_REG_PS_OFFSET_LSB, &buffer, sizeof(buffer));
+
+	if (ret < 0) {
+		dev_err(&data->client->dev, "Failed to read PS OFFSET register\n");
+		return ret;
+	}
+	*offset = le16_to_cpu(buffer);
+
+	return ret;
+}
+
+static int rpr0521_write_ps_offset(struct rpr0521_data *data, int offset)
+{
+	int ret;
+	__le16 buffer;
+
+	buffer = cpu_to_le16(offset & 0x3ff);
+	ret = regmap_raw_write(data->regmap,
+		RPR0521_REG_PS_OFFSET_LSB, &buffer, sizeof(buffer));
+
+	if (ret < 0) {
+		dev_err(&data->client->dev, "Failed to write PS OFFSET register\n");
+		return ret;
+	}
+
+	return ret;
+}
+
 static int rpr0521_read_raw(struct iio_dev *indio_dev,
 			    struct iio_chan_spec const *chan, int *val,
 			    int *val2, long mask)
@@ -490,6 +526,15 @@ static int rpr0521_read_raw(struct iio_dev *indio_dev,
 
 		return IIO_VAL_INT_PLUS_MICRO;
 
+	case IIO_CHAN_INFO_OFFSET:
+		mutex_lock(&data->lock);
+		ret = rpr0521_read_ps_offset(data, val);
+		mutex_unlock(&data->lock);
+		if (ret < 0)
+			return ret;
+
+		return IIO_VAL_INT;
+
 	default:
 		return -EINVAL;
 	}
@@ -519,6 +564,13 @@ static int rpr0521_write_raw(struct iio_dev *indio_dev,
 
 		return ret;
 
+	case IIO_CHAN_INFO_OFFSET:
+		mutex_lock(&data->lock);
+		ret = rpr0521_write_ps_offset(data, val);
+		mutex_unlock(&data->lock);
+
+		return ret;
+
 	default:
 		return -EINVAL;
 	}
-- 
1.7.9.5

[PATCH v3 8/9] iio: light: rpr0521 channel numbers reordered

[Mikko Koivunen]

Move proximity channel from last to first in structs to avoid confusion
later with buffered triggers. Proximity data output is first in rpr0521
register map.

Signed-off-by: Mikko Koivunen <mikko.koivunen@fi.rohmeurope.com>
---
 drivers/iio/light/rpr0521.c |   40 ++++++++++++++++++++--------------------
 1 file changed, 20 insertions(+), 20 deletions(-)

diff --git a/drivers/iio/light/rpr0521.c b/drivers/iio/light/rpr0521.c
index 41b3023..2dc5e38 100644
--- a/drivers/iio/light/rpr0521.c
+++ b/drivers/iio/light/rpr0521.c
@@ -78,9 +78,9 @@ static const struct rpr0521_gain rpr0521_pxs_gain[3] = {
 };
 
 enum rpr0521_channel {
+	RPR0521_CHAN_PXS,
 	RPR0521_CHAN_ALS_DATA0,
 	RPR0521_CHAN_ALS_DATA1,
-	RPR0521_CHAN_PXS,
 };
 
 struct rpr0521_reg_desc {
@@ -89,6 +89,10 @@ struct rpr0521_reg_desc {
 };
 
 static const struct rpr0521_reg_desc rpr0521_data_reg[] = {
+	[RPR0521_CHAN_PXS]	= {
+		.address	= RPR0521_REG_PXS_DATA,
+		.device_mask	= RPR0521_MODE_PXS_MASK,
+	},
 	[RPR0521_CHAN_ALS_DATA0] = {
 		.address	= RPR0521_REG_ALS_DATA0,
 		.device_mask	= RPR0521_MODE_ALS_MASK,
@@ -97,10 +101,6 @@ static const struct rpr0521_reg_desc rpr0521_data_reg[] = {
 		.address	= RPR0521_REG_ALS_DATA1,
 		.device_mask	= RPR0521_MODE_ALS_MASK,
 	},
-	[RPR0521_CHAN_PXS]	= {
-		.address	= RPR0521_REG_PXS_DATA,
-		.device_mask	= RPR0521_MODE_PXS_MASK,
-	},
 };
 
 static const struct rpr0521_gain_info {
@@ -110,6 +110,13 @@ static const struct rpr0521_gain_info {
 	const struct rpr0521_gain *gain;
 	int size;
 } rpr0521_gain[] = {
+	[RPR0521_CHAN_PXS] = {
+		.reg	= RPR0521_REG_PXS_CTRL,
+		.mask	= RPR0521_PXS_GAIN_MASK,
+		.shift	= RPR0521_PXS_GAIN_SHIFT,
+		.gain	= rpr0521_pxs_gain,
+		.size	= ARRAY_SIZE(rpr0521_pxs_gain),
+	},
 	[RPR0521_CHAN_ALS_DATA0] = {
 		.reg	= RPR0521_REG_ALS_CTRL,
 		.mask	= RPR0521_ALS_DATA0_GAIN_MASK,
@@ -124,13 +131,6 @@ static const struct rpr0521_gain_info {
 		.gain	= rpr0521_als_gain,
 		.size	= ARRAY_SIZE(rpr0521_als_gain),
 	},
-	[RPR0521_CHAN_PXS] = {
-		.reg	= RPR0521_REG_PXS_CTRL,
-		.mask	= RPR0521_PXS_GAIN_MASK,
-		.shift	= RPR0521_PXS_GAIN_SHIFT,
-		.gain	= rpr0521_pxs_gain,
-		.size	= ARRAY_SIZE(rpr0521_pxs_gain),
-	},
 };
 
 struct rpr0521_samp_freq {
@@ -198,6 +198,14 @@ static const struct attribute_group rpr0521_attribute_group = {
 
 static const struct iio_chan_spec rpr0521_channels[] = {
 	{
+		.type = IIO_PROXIMITY,
+		.address = RPR0521_CHAN_PXS,
+		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
+			BIT(IIO_CHAN_INFO_OFFSET) |
+			BIT(IIO_CHAN_INFO_SCALE),
+		.info_mask_shared_by_all = BIT(IIO_CHAN_INFO_SAMP_FREQ),
+	},
+	{
 		.type = IIO_INTENSITY,
 		.modified = 1,
 		.address = RPR0521_CHAN_ALS_DATA0,
@@ -215,14 +223,6 @@ static const struct iio_chan_spec rpr0521_channels[] = {
 			BIT(IIO_CHAN_INFO_SCALE),
 		.info_mask_shared_by_all = BIT(IIO_CHAN_INFO_SAMP_FREQ),
 	},
-	{
-		.type = IIO_PROXIMITY,
-		.address = RPR0521_CHAN_PXS,
-		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
-			BIT(IIO_CHAN_INFO_OFFSET) |
-			BIT(IIO_CHAN_INFO_SCALE),
-		.info_mask_shared_by_all = BIT(IIO_CHAN_INFO_SAMP_FREQ),
-	}
 };
 
 static int rpr0521_als_enable(struct rpr0521_data *data, u8 status)
-- 
1.7.9.5

Re: [PATCH v3 8/9] iio: light: rpr0521 channel numbers reordered

[Jonathan Cameron]

On 05/05/17 12:19, Mikko Koivunen wrote:
> Move proximity channel from last to first in structs to avoid confusion
> later with buffered triggers. Proximity data output is first in rpr0521
> register map.
> 
> Signed-off-by: Mikko Koivunen <mikko.koivunen@fi.rohmeurope.com>
> ---
>  drivers/iio/light/rpr0521.c |   40 ++++++++++++++++++++--------------------
>  1 file changed, 20 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/iio/light/rpr0521.c b/drivers/iio/light/rpr0521.c
> index 41b3023..2dc5e38 100644
> --- a/drivers/iio/light/rpr0521.c
> +++ b/drivers/iio/light/rpr0521.c
> @@ -78,9 +78,9 @@ static const struct rpr0521_gain rpr0521_pxs_gain[3] = {
>  };
>  
>  enum rpr0521_channel {
> +	RPR0521_CHAN_PXS,
>  	RPR0521_CHAN_ALS_DATA0,
>  	RPR0521_CHAN_ALS_DATA1,
> -	RPR0521_CHAN_PXS,
>  };
>  
>  struct rpr0521_reg_desc {
> @@ -89,6 +89,10 @@ struct rpr0521_reg_desc {
>  };
>  
>  static const struct rpr0521_reg_desc rpr0521_data_reg[] = {
> +	[RPR0521_CHAN_PXS]	= {
> +		.address	= RPR0521_REG_PXS_DATA,
> +		.device_mask	= RPR0521_MODE_PXS_MASK,
> +	},
>  	[RPR0521_CHAN_ALS_DATA0] = {
>  		.address	= RPR0521_REG_ALS_DATA0,
>  		.device_mask	= RPR0521_MODE_ALS_MASK,
> @@ -97,10 +101,6 @@ static const struct rpr0521_reg_desc rpr0521_data_reg[] = {
>  		.address	= RPR0521_REG_ALS_DATA1,
>  		.device_mask	= RPR0521_MODE_ALS_MASK,
>  	},
> -	[RPR0521_CHAN_PXS]	= {
> -		.address	= RPR0521_REG_PXS_DATA,
> -		.device_mask	= RPR0521_MODE_PXS_MASK,
> -	},
>  };
>  
>  static const struct rpr0521_gain_info {
> @@ -110,6 +110,13 @@ static const struct rpr0521_gain_info {
>  	const struct rpr0521_gain *gain;
>  	int size;
>  } rpr0521_gain[] = {
> +	[RPR0521_CHAN_PXS] = {
> +		.reg	= RPR0521_REG_PXS_CTRL,
> +		.mask	= RPR0521_PXS_GAIN_MASK,
> +		.shift	= RPR0521_PXS_GAIN_SHIFT,
> +		.gain	= rpr0521_pxs_gain,
> +		.size	= ARRAY_SIZE(rpr0521_pxs_gain),
> +	},
>  	[RPR0521_CHAN_ALS_DATA0] = {
>  		.reg	= RPR0521_REG_ALS_CTRL,
>  		.mask	= RPR0521_ALS_DATA0_GAIN_MASK,
> @@ -124,13 +131,6 @@ static const struct rpr0521_gain_info {
>  		.gain	= rpr0521_als_gain,
>  		.size	= ARRAY_SIZE(rpr0521_als_gain),
>  	},
> -	[RPR0521_CHAN_PXS] = {
> -		.reg	= RPR0521_REG_PXS_CTRL,
> -		.mask	= RPR0521_PXS_GAIN_MASK,
> -		.shift	= RPR0521_PXS_GAIN_SHIFT,
> -		.gain	= rpr0521_pxs_gain,
> -		.size	= ARRAY_SIZE(rpr0521_pxs_gain),
> -	},
>  };
>  
>  struct rpr0521_samp_freq {
> @@ -198,6 +198,14 @@ static const struct attribute_group rpr0521_attribute_group = {
>  
>  static const struct iio_chan_spec rpr0521_channels[] = {
Hmm. ordering here matters not in the slightest.  Still I guess
it makes sense to change it for consistency if nothing else so
fair enough.
>  	{
> +		.type = IIO_PROXIMITY,
> +		.address = RPR0521_CHAN_PXS,
> +		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
> +			BIT(IIO_CHAN_INFO_OFFSET) |
> +			BIT(IIO_CHAN_INFO_SCALE),
> +		.info_mask_shared_by_all = BIT(IIO_CHAN_INFO_SAMP_FREQ),
> +	},
> +	{
>  		.type = IIO_INTENSITY,
>  		.modified = 1,
>  		.address = RPR0521_CHAN_ALS_DATA0,
> @@ -215,14 +223,6 @@ static const struct iio_chan_spec rpr0521_channels[] = {
>  			BIT(IIO_CHAN_INFO_SCALE),
>  		.info_mask_shared_by_all = BIT(IIO_CHAN_INFO_SAMP_FREQ),
>  	},
> -	{
> -		.type = IIO_PROXIMITY,
> -		.address = RPR0521_CHAN_PXS,
> -		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
> -			BIT(IIO_CHAN_INFO_OFFSET) |
> -			BIT(IIO_CHAN_INFO_SCALE),
> -		.info_mask_shared_by_all = BIT(IIO_CHAN_INFO_SAMP_FREQ),
> -	}
>  };
>  
>  static int rpr0521_als_enable(struct rpr0521_data *data, u8 status)
> 

[PATCH v3 9/9] iio: light: rpr0521 triggered buffer

[Mikko Koivunen]

Set up and use triggered buffer if there is irq defined for device in
device tree. Trigger producer triggers from rpr0521 drdy interrupt line.
Trigger consumer reads rpr0521 data to scan buffer.
Depends on previous commits of _scale and _offset.

Signed-off-by: Mikko Koivunen <mikko.koivunen@fi.rohmeurope.com>
---
Patch v2->v3 changes:
.shift = 0 removed
reordered functions to remove forward declarations
whitespace changes
refactored some update_bits->write, no "err = err || *"-pattern
refactored trigger_consumer_handler
reordered _probe() and _remove()
added int clear on poweroff()
checkpatch.pl OK
Tested on LeMaker HiKey with AOSP7.1 kernel 4.4.
Builds ok with torvalds/linux feb 27.

 drivers/iio/light/rpr0521.c |  274 ++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 271 insertions(+), 3 deletions(-)

diff --git a/drivers/iio/light/rpr0521.c b/drivers/iio/light/rpr0521.c
index 2dc5e38..6feaa78 100644
--- a/drivers/iio/light/rpr0521.c
+++ b/drivers/iio/light/rpr0521.c
@@ -9,7 +9,7 @@
  *
  * IIO driver for RPR-0521RS (7-bit I2C slave address 0x38).
  *
- * TODO: illuminance channel, buffer
+ * TODO: illuminance channel
  */
 
 #include <linux/module.h>
@@ -20,6 +20,10 @@
 #include <linux/acpi.h>
 
 #include <linux/iio/iio.h>
+#include <linux/iio/buffer.h>
+#include <linux/iio/trigger.h>
+#include <linux/iio/trigger_consumer.h>
+#include <linux/iio/triggered_buffer.h>
 #include <linux/iio/sysfs.h>
 #include <linux/pm_runtime.h>
 
@@ -30,6 +34,7 @@
 #define RPR0521_REG_PXS_DATA		0x44 /* 16-bit, little endian */
 #define RPR0521_REG_ALS_DATA0		0x46 /* 16-bit, little endian */
 #define RPR0521_REG_ALS_DATA1		0x48 /* 16-bit, little endian */
+#define RPR0521_REG_INTERRUPT		0x4A
 #define RPR0521_REG_PS_OFFSET_LSB	0x53
 #define RPR0521_REG_ID			0x92
 
@@ -42,16 +47,29 @@
 #define RPR0521_ALS_DATA1_GAIN_SHIFT	2
 #define RPR0521_PXS_GAIN_MASK		GENMASK(5, 4)
 #define RPR0521_PXS_GAIN_SHIFT		4
+#define RPR0521_PXS_PERSISTENCE_MASK	GENMASK(3, 0)
+#define RPR0521_INTERRUPT_INT_TRIG_PS_MASK	BIT(0)
+#define RPR0521_INTERRUPT_INT_TRIG_ALS_MASK	BIT(1)
+#define RPR0521_INTERRUPT_INT_REASSERT_MASK	BIT(3)
 
 #define RPR0521_MODE_ALS_ENABLE		BIT(7)
 #define RPR0521_MODE_ALS_DISABLE	0x00
 #define RPR0521_MODE_PXS_ENABLE		BIT(6)
 #define RPR0521_MODE_PXS_DISABLE	0x00
+#define RPR0521_PXS_PERSISTENCE_DRDY	0x00
+
+#define RPR0521_INTERRUPT_INT_TRIG_PS_ENABLE	BIT(0)
+#define RPR0521_INTERRUPT_INT_TRIG_PS_DISABLE	0x00
+#define RPR0521_INTERRUPT_INT_TRIG_ALS_ENABLE	BIT(1)
+#define RPR0521_INTERRUPT_INT_TRIG_ALS_DISABLE	0x00
+#define RPR0521_INTERRUPT_INT_REASSERT_ENABLE	BIT(3)
+#define RPR0521_INTERRUPT_INT_REASSERT_DISABLE	0x00
 
 #define RPR0521_MANUFACT_ID		0xE0
 #define RPR0521_DEFAULT_MEAS_TIME	0x06 /* ALS - 100ms, PXS - 100ms */
 
 #define RPR0521_DRV_NAME		"RPR0521"
+#define RPR0521_IRQ_NAME		"rpr0521_event"
 #define RPR0521_REGMAP_NAME		"rpr0521_regmap"
 
 #define RPR0521_SLEEP_DELAY_MS	2000
@@ -167,6 +185,9 @@ struct rpr0521_data {
 	bool als_dev_en;
 	bool pxs_dev_en;
 
+	struct iio_trigger *drdy_trigger0;
+	bool drdy_trigger_on;
+
 	/* optimize runtime pm ops - enable/disable device only if needed */
 	bool als_ps_need_en;
 	bool pxs_ps_need_en;
@@ -196,6 +217,13 @@ static const struct attribute_group rpr0521_attribute_group = {
 	.attrs = rpr0521_attributes,
 };
 
+/* Order of the channel data in buffer */
+enum rpr0521_scan_index_order {
+	RPR0521_CHAN_INDEX_PXS,
+	RPR0521_CHAN_INDEX_BOTH,
+	RPR0521_CHAN_INDEX_IR,
+};
+
 static const struct iio_chan_spec rpr0521_channels[] = {
 	{
 		.type = IIO_PROXIMITY,
@@ -204,6 +232,13 @@ static const struct iio_chan_spec rpr0521_channels[] = {
 			BIT(IIO_CHAN_INFO_OFFSET) |
 			BIT(IIO_CHAN_INFO_SCALE),
 		.info_mask_shared_by_all = BIT(IIO_CHAN_INFO_SAMP_FREQ),
+		.scan_index = RPR0521_CHAN_INDEX_PXS,
+		.scan_type = {
+			.sign = 'u',
+			.realbits = 16,
+			.storagebits = 16,
+			.endianness = IIO_LE,
+		},
 	},
 	{
 		.type = IIO_INTENSITY,
@@ -213,6 +248,13 @@ static const struct iio_chan_spec rpr0521_channels[] = {
 		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
 			BIT(IIO_CHAN_INFO_SCALE),
 		.info_mask_shared_by_all = BIT(IIO_CHAN_INFO_SAMP_FREQ),
+		.scan_index = RPR0521_CHAN_INDEX_BOTH,
+		.scan_type = {
+			.sign = 'u',
+			.realbits = 16,
+			.storagebits = 16,
+			.endianness = IIO_LE,
+		},
 	},
 	{
 		.type = IIO_INTENSITY,
@@ -222,6 +264,13 @@ static const struct iio_chan_spec rpr0521_channels[] = {
 		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
 			BIT(IIO_CHAN_INFO_SCALE),
 		.info_mask_shared_by_all = BIT(IIO_CHAN_INFO_SAMP_FREQ),
+		.scan_index = RPR0521_CHAN_INDEX_IR,
+		.scan_type = {
+			.sign = 'u',
+			.realbits = 16,
+			.storagebits = 16,
+			.endianness = IIO_LE,
+		},
 	},
 };
 
@@ -330,6 +379,137 @@ static int rpr0521_set_power_state(struct rpr0521_data *data, bool on,
 	return 0;
 }
 
+/* IRQ to trigger handler */
+static irqreturn_t rpr0521_drdy_irq_handler(int irq, void *private)
+{
+	struct iio_dev *indio_dev = private;
+	struct rpr0521_data *data = iio_priv(indio_dev);
+
+	/*
+	 * Sometimes static on floating (hi-z) interrupt line causes interrupt.
+	 * Notify trigger0 consumers/subscribers only if trigger has been
+	 * enabled. This is to prevent i2c writes to sensor which is actually
+	 * powered off.
+	 */
+	if (data->drdy_trigger_on)
+		iio_trigger_poll(data->drdy_trigger0);
+
+	return IRQ_HANDLED;
+}
+
+static irqreturn_t rpr0521_trigger_consumer_handler(int irq, void *p)
+{
+	struct iio_poll_func *pf = p;
+	struct iio_dev *indio_dev = pf->indio_dev;
+	struct rpr0521_data *data = iio_priv(indio_dev);
+	int err;
+
+	u8 buffer[16]; /* 3 16-bit channels + padding + ts */
+
+	err = regmap_bulk_read(data->regmap, RPR0521_REG_PXS_DATA,
+		&buffer,
+		(3 * 2) + 1);	/* 3 * 16-bit + (discarded) int clear reg. */
+	if (!err)
+		iio_push_to_buffers_with_timestamp(indio_dev,
+			buffer, pf->timestamp);
+	else
+		dev_err(&data->client->dev,
+			"Trigger consumer can't read from sensor.\n");
+	/*
+	 * Note when using CONFIG_PM: the sensor is powered on in trigger
+	 * producer _set_state(). If using different trigger as current_trigger
+	 * the sensor must be powered on and measurement enabled using
+	 * _set_power_state().
+	 */
+
+	iio_trigger_notify_done(indio_dev->trig);
+
+	return IRQ_HANDLED;
+}
+
+static int rpr0521_write_int_enable(struct rpr0521_data *data)
+{
+	int err;
+
+	/* Interrupt after each measurement */
+	err = regmap_update_bits(data->regmap, RPR0521_REG_PXS_CTRL,
+		RPR0521_PXS_PERSISTENCE_MASK,
+		RPR0521_PXS_PERSISTENCE_DRDY);
+	if (err) {
+		dev_err(&data->client->dev, "PS control reg write fail.\n");
+		return -EBUSY;
+		}
+
+	/* Ignore latch and mode because of drdy */
+	err = regmap_write(data->regmap, RPR0521_REG_INTERRUPT,
+		RPR0521_INTERRUPT_INT_REASSERT_DISABLE |
+		RPR0521_INTERRUPT_INT_TRIG_ALS_DISABLE |
+		RPR0521_INTERRUPT_INT_TRIG_PS_ENABLE
+		);
+	if (err) {
+		dev_err(&data->client->dev, "Interrupt setup write fail.\n");
+		return -EBUSY;
+		}
+
+	return 0;
+}
+
+static int rpr0521_write_int_disable(struct rpr0521_data *data)
+{
+	/* Don't care of clearing mode, assert and latch. */
+	return regmap_write(data->regmap, RPR0521_REG_INTERRUPT,
+				RPR0521_INTERRUPT_INT_TRIG_ALS_DISABLE |
+				RPR0521_INTERRUPT_INT_TRIG_PS_DISABLE
+				);
+}
+
+/* Trigger producer enable / disable */
+static int rpr0521_pxs_drdy_set_state(struct iio_trigger *trigger,
+	bool enable_drdy)
+{
+	struct iio_dev *indio_dev = iio_trigger_get_drvdata(trigger);
+	struct rpr0521_data *data = iio_priv(indio_dev);
+	int err;
+
+	if (enable_drdy) {
+		/* ENABLE DRDY */
+		mutex_lock(&data->lock);
+		err = rpr0521_set_power_state(data, true,
+			(RPR0521_MODE_PXS_MASK | RPR0521_MODE_ALS_MASK));
+		mutex_unlock(&data->lock);
+		if (err)
+			goto rpr0521_set_state_err;
+
+		err = rpr0521_write_int_enable(data);
+		if (err)
+			goto rpr0521_set_state_err;
+	} else {
+		/* DISABLE DRDY */
+		mutex_lock(&data->lock);
+		err = rpr0521_set_power_state(data, false,
+			(RPR0521_MODE_PXS_MASK | RPR0521_MODE_ALS_MASK));
+		mutex_unlock(&data->lock);
+		if (err)
+			goto rpr0521_set_state_err;
+
+		err = rpr0521_write_int_disable(data);
+		if (err)
+			goto rpr0521_set_state_err;
+	}
+	data->drdy_trigger_on = enable_drdy;
+
+	return 0;
+
+rpr0521_set_state_err:
+	dev_err(&data->client->dev, "rpr0521_pxs_drdy_set_state failed\n");
+	return err;
+}
+
+static const struct iio_trigger_ops rpr0521_trigger_ops = {
+	.set_trigger_state = rpr0521_pxs_drdy_set_state,
+	.owner = THIS_MODULE,
+	};
+
 static int rpr0521_get_gain(struct rpr0521_data *data, int chan,
 			    int *val, int *val2)
 {
@@ -481,6 +661,9 @@ static int rpr0521_read_raw(struct iio_dev *indio_dev,
 		if (chan->type != IIO_INTENSITY && chan->type != IIO_PROXIMITY)
 			return -EINVAL;
 
+		if (iio_buffer_enabled(indio_dev))
+			return -EBUSY;
+
 		device_mask = rpr0521_data_reg[chan->address].device_mask;
 
 		mutex_lock(&data->lock);
@@ -624,6 +807,7 @@ static int rpr0521_init(struct rpr0521_data *data)
 static int rpr0521_poweroff(struct rpr0521_data *data)
 {
 	int ret;
+	int tmp;
 
 	ret = regmap_update_bits(data->regmap, RPR0521_REG_MODE_CTRL,
 				 RPR0521_MODE_ALS_MASK |
@@ -636,6 +820,16 @@ static int rpr0521_poweroff(struct rpr0521_data *data)
 	data->als_dev_en = false;
 	data->pxs_dev_en = false;
 
+	/*
+	 * Int pin keeps state after power off. Set pin to high impedance
+	 * mode to prevent power drain.
+	 */
+	ret = regmap_read(data->regmap, RPR0521_REG_INTERRUPT, &tmp);
+	if (ret) {
+		dev_err(&data->client->dev, "Failed to reset int pin.\n");
+		return ret;
+		}
+
 	return 0;
 }
 
@@ -708,12 +902,77 @@ static int rpr0521_probe(struct i2c_client *client,
 	pm_runtime_set_autosuspend_delay(&client->dev, RPR0521_SLEEP_DELAY_MS);
 	pm_runtime_use_autosuspend(&client->dev);
 
+	/*
+	 * If sensor write/read  is needed in _probe after _use_autosuspend,
+	 * sensor needs to be _resumed first using rpr0521_set_power_state().
+	 */
+
+	/* IRQ to trigger setup */
+	if (client->irq) {
+		/* Trigger0 producer setup */
+		data->drdy_trigger0 = devm_iio_trigger_alloc(
+			indio_dev->dev.parent,
+			"%s-dev%d", indio_dev->name, indio_dev->id);
+		if (!data->drdy_trigger0) {
+			ret = -ENOMEM;
+			goto err_pm_disable;
+		}
+		data->drdy_trigger0->dev.parent = indio_dev->dev.parent;
+		data->drdy_trigger0->ops = &rpr0521_trigger_ops;
+		iio_trigger_set_drvdata(data->drdy_trigger0, indio_dev);
+
+		/* Ties irq to trigger producer handler. */
+		ret = devm_request_threaded_irq(&client->dev, client->irq,
+			rpr0521_drdy_irq_handler,
+			NULL,
+			IRQF_TRIGGER_FALLING | IRQF_ONESHOT,
+			RPR0521_IRQ_NAME,
+			indio_dev);
+		if (ret < 0) {
+			dev_err(&client->dev, "request irq %d for trigger0 failed\n",
+				client->irq);
+			goto err_pm_disable;
+			}
+
+		ret = iio_trigger_register(data->drdy_trigger0);
+		if (ret) {
+			dev_err(&client->dev, "iio trigger register failed\n");
+			goto err_pm_disable;
+		}
+
+		/*
+		 * Now whole pipe from physical interrupt (irq defined by
+		 * devicetree to device) to trigger0 output is set up.
+		 */
+
+		/* Trigger consumer setup */
+		ret = iio_triggered_buffer_setup(indio_dev,
+			&iio_pollfunc_store_time,
+			rpr0521_trigger_consumer_handler,
+			NULL);		//Use default _ops
+		if (ret < 0) {
+			dev_err(&client->dev, "iio triggered buffer setup failed\n");
+			/* Count on devm to free_irq */
+			goto err_iio_trigger_unregister;
+		}
+	}
+
 	ret = iio_device_register(indio_dev);
 	if (ret)
-		goto err_pm_disable;
+		goto err_iio_triggered_buffer_cleanup;
+
+	if (client->irq) {
+		/* Set trigger0 as current trigger (and inc refcount) */
+		indio_dev->trig = iio_trigger_get(data->drdy_trigger0);
+	}
 
 	return 0;
 
+err_iio_triggered_buffer_cleanup:
+	iio_triggered_buffer_cleanup(indio_dev);
+err_iio_trigger_unregister:
+	if (data->drdy_trigger0)
+		iio_trigger_unregister(data->drdy_trigger0);
 err_pm_disable:
 	pm_runtime_disable(&client->dev);
 	pm_runtime_set_suspended(&client->dev);
@@ -727,14 +986,23 @@ err_poweroff:
 static int rpr0521_remove(struct i2c_client *client)
 {
 	struct iio_dev *indio_dev = i2c_get_clientdata(client);
+	struct rpr0521_data *data = iio_priv(indio_dev);
+
+	if (indio_dev->trig)
+		iio_trigger_put(indio_dev->trig);
 
 	iio_device_unregister(indio_dev);
 
+	iio_triggered_buffer_cleanup(indio_dev);
+
+	if (data->drdy_trigger0)
+		iio_trigger_unregister(data->drdy_trigger0);
+
 	pm_runtime_disable(&client->dev);
 	pm_runtime_set_suspended(&client->dev);
 	pm_runtime_put_noidle(&client->dev);
 
-	rpr0521_poweroff(iio_priv(indio_dev));
+	rpr0521_poweroff(data);
 
 	return 0;
 }
-- 
1.7.9.5

Re: [PATCH v3 9/9] iio: light: rpr0521 triggered buffer

[Jonathan Cameron]

On 05/05/17 12:19, Mikko Koivunen wrote:
> Set up and use triggered buffer if there is irq defined for device in
> device tree. Trigger producer triggers from rpr0521 drdy interrupt line.
> Trigger consumer reads rpr0521 data to scan buffer.
> Depends on previous commits of _scale and _offset.
> 
> Signed-off-by: Mikko Koivunen <mikko.koivunen@fi.rohmeurope.com>A few elements inline.

Also looking for Daniel input on this one in particular as it's
a little fiddly to say the least!

I wonder if the balance between what goes on the trigger enable
vs the buffer enable is correct.  Seems there are some issues around
runtime pm that need resolving.  It may be that you need a bit of
an on off dance to ensure the power is enabled when needed for
trigger setup and for buffers being enabled.

Jonathan
> ---
> Patch v2->v3 changes:
> .shift = 0 removed
> reordered functions to remove forward declarations
> whitespace changes
> refactored some update_bits->write, no "err = err || *"-pattern
> refactored trigger_consumer_handler
> reordered _probe() and _remove()
> added int clear on poweroff()
> checkpatch.pl OK
> Tested on LeMaker HiKey with AOSP7.1 kernel 4.4.
> Builds ok with torvalds/linux feb 27.
> 
>  drivers/iio/light/rpr0521.c |  274 ++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 271 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/iio/light/rpr0521.c b/drivers/iio/light/rpr0521.c
> index 2dc5e38..6feaa78 100644
> --- a/drivers/iio/light/rpr0521.c
> +++ b/drivers/iio/light/rpr0521.c
> @@ -9,7 +9,7 @@
>   *
>   * IIO driver for RPR-0521RS (7-bit I2C slave address 0x38).
>   *
> - * TODO: illuminance channel, buffer
> + * TODO: illuminance channel
>   */
>  
>  #include <linux/module.h>
> @@ -20,6 +20,10 @@
>  #include <linux/acpi.h>
>  
>  #include <linux/iio/iio.h>
> +#include <linux/iio/buffer.h>
> +#include <linux/iio/trigger.h>
> +#include <linux/iio/trigger_consumer.h>
> +#include <linux/iio/triggered_buffer.h>
>  #include <linux/iio/sysfs.h>
>  #include <linux/pm_runtime.h>
>  
> @@ -30,6 +34,7 @@
>  #define RPR0521_REG_PXS_DATA		0x44 /* 16-bit, little endian */
>  #define RPR0521_REG_ALS_DATA0		0x46 /* 16-bit, little endian */
>  #define RPR0521_REG_ALS_DATA1		0x48 /* 16-bit, little endian */
> +#define RPR0521_REG_INTERRUPT		0x4A
>  #define RPR0521_REG_PS_OFFSET_LSB	0x53
>  #define RPR0521_REG_ID			0x92
>  
> @@ -42,16 +47,29 @@
>  #define RPR0521_ALS_DATA1_GAIN_SHIFT	2
>  #define RPR0521_PXS_GAIN_MASK		GENMASK(5, 4)
>  #define RPR0521_PXS_GAIN_SHIFT		4
> +#define RPR0521_PXS_PERSISTENCE_MASK	GENMASK(3, 0)
> +#define RPR0521_INTERRUPT_INT_TRIG_PS_MASK	BIT(0)
> +#define RPR0521_INTERRUPT_INT_TRIG_ALS_MASK	BIT(1)
> +#define RPR0521_INTERRUPT_INT_REASSERT_MASK	BIT(3)
>  
>  #define RPR0521_MODE_ALS_ENABLE		BIT(7)
>  #define RPR0521_MODE_ALS_DISABLE	0x00
>  #define RPR0521_MODE_PXS_ENABLE		BIT(6)
>  #define RPR0521_MODE_PXS_DISABLE	0x00
> +#define RPR0521_PXS_PERSISTENCE_DRDY	0x00
> +
> +#define RPR0521_INTERRUPT_INT_TRIG_PS_ENABLE	BIT(0)
> +#define RPR0521_INTERRUPT_INT_TRIG_PS_DISABLE	0x00
> +#define RPR0521_INTERRUPT_INT_TRIG_ALS_ENABLE	BIT(1)
> +#define RPR0521_INTERRUPT_INT_TRIG_ALS_DISABLE	0x00
> +#define RPR0521_INTERRUPT_INT_REASSERT_ENABLE	BIT(3)
> +#define RPR0521_INTERRUPT_INT_REASSERT_DISABLE	0x00
>  
>  #define RPR0521_MANUFACT_ID		0xE0
>  #define RPR0521_DEFAULT_MEAS_TIME	0x06 /* ALS - 100ms, PXS - 100ms */
>  
>  #define RPR0521_DRV_NAME		"RPR0521"
> +#define RPR0521_IRQ_NAME		"rpr0521_event"
>  #define RPR0521_REGMAP_NAME		"rpr0521_regmap"
>  
>  #define RPR0521_SLEEP_DELAY_MS	2000
> @@ -167,6 +185,9 @@ struct rpr0521_data {
>  	bool als_dev_en;
>  	bool pxs_dev_en;
>  
> +	struct iio_trigger *drdy_trigger0;
> +	bool drdy_trigger_on;
> +
>  	/* optimize runtime pm ops - enable/disable device only if needed */
>  	bool als_ps_need_en;
>  	bool pxs_ps_need_en;
> @@ -196,6 +217,13 @@ static const struct attribute_group rpr0521_attribute_group = {
>  	.attrs = rpr0521_attributes,
>  };
>  
> +/* Order of the channel data in buffer */
> +enum rpr0521_scan_index_order {
> +	RPR0521_CHAN_INDEX_PXS,
> +	RPR0521_CHAN_INDEX_BOTH,
> +	RPR0521_CHAN_INDEX_IR,
> +};
> +
>  static const struct iio_chan_spec rpr0521_channels[] = {
>  	{
>  		.type = IIO_PROXIMITY,
> @@ -204,6 +232,13 @@ static const struct iio_chan_spec rpr0521_channels[] = {
>  			BIT(IIO_CHAN_INFO_OFFSET) |
>  			BIT(IIO_CHAN_INFO_SCALE),
>  		.info_mask_shared_by_all = BIT(IIO_CHAN_INFO_SAMP_FREQ),
> +		.scan_index = RPR0521_CHAN_INDEX_PXS,
> +		.scan_type = {
> +			.sign = 'u',
> +			.realbits = 16,
> +			.storagebits = 16,
> +			.endianness = IIO_LE,
> +		},
>  	},
>  	{
>  		.type = IIO_INTENSITY,
> @@ -213,6 +248,13 @@ static const struct iio_chan_spec rpr0521_channels[] = {
>  		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
>  			BIT(IIO_CHAN_INFO_SCALE),
>  		.info_mask_shared_by_all = BIT(IIO_CHAN_INFO_SAMP_FREQ),
> +		.scan_index = RPR0521_CHAN_INDEX_BOTH,
> +		.scan_type = {
> +			.sign = 'u',
> +			.realbits = 16,
> +			.storagebits = 16,
> +			.endianness = IIO_LE,
> +		},
>  	},
>  	{
>  		.type = IIO_INTENSITY,
> @@ -222,6 +264,13 @@ static const struct iio_chan_spec rpr0521_channels[] = {
>  		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
>  			BIT(IIO_CHAN_INFO_SCALE),
>  		.info_mask_shared_by_all = BIT(IIO_CHAN_INFO_SAMP_FREQ),
> +		.scan_index = RPR0521_CHAN_INDEX_IR,
> +		.scan_type = {
> +			.sign = 'u',
> +			.realbits = 16,
> +			.storagebits = 16,
> +			.endianness = IIO_LE,
> +		},
>  	},
>  };
>  
> @@ -330,6 +379,137 @@ static int rpr0521_set_power_state(struct rpr0521_data *data, bool on,
>  	return 0;
>  }
>  
> +/* IRQ to trigger handler */
> +static irqreturn_t rpr0521_drdy_irq_handler(int irq, void *private)
> +{
> +	struct iio_dev *indio_dev = private;
> +	struct rpr0521_data *data = iio_priv(indio_dev);
> +
> +	/*
> +	 * Sometimes static on floating (hi-z) interrupt line causes interrupt.
> +	 * Notify trigger0 consumers/subscribers only if trigger has been
> +	 * enabled. This is to prevent i2c writes to sensor which is actually
> +	 * powered off.
> +	 */
> +	if (data->drdy_trigger_on)
> +		iio_trigger_poll(data->drdy_trigger0);
> +
> +	return IRQ_HANDLED;
> +}
> +
> +static irqreturn_t rpr0521_trigger_consumer_handler(int irq, void *p)
> +{
> +	struct iio_poll_func *pf = p;
> +	struct iio_dev *indio_dev = pf->indio_dev;
> +	struct rpr0521_data *data = iio_priv(indio_dev);
> +	int err;
> +
> +	u8 buffer[16]; /* 3 16-bit channels + padding + ts */
> +
> +	err = regmap_bulk_read(data->regmap, RPR0521_REG_PXS_DATA,
> +		&buffer,
> +		(3 * 2) + 1);	/* 3 * 16-bit + (discarded) int clear reg. */
> +	if (!err)
> +		iio_push_to_buffers_with_timestamp(indio_dev,
> +			buffer, pf->timestamp);
> +	else
> +		dev_err(&data->client->dev,
> +			"Trigger consumer can't read from sensor.\n");
> +	/*
> +	 * Note when using CONFIG_PM: the sensor is powered on in trigger
> +	 * producer _set_state(). If using different trigger as current_trigger
> +	 * the sensor must be powered on and measurement enabled using
> +	 * _set_power_state().
So it won't work with a different trigger?  If so you want to do
a validate trigger to stop it trying to use anything else.

You could move the power enable to the buffer preenable instead
if I understand your comment correctly.  That would make it work either
way.
> +	 */
> +
> +	iio_trigger_notify_done(indio_dev->trig);
> +
> +	return IRQ_HANDLED;
> +}
> +
> +static int rpr0521_write_int_enable(struct rpr0521_data *data)
> +{
> +	int err;
> +
> +	/* Interrupt after each measurement */
> +	err = regmap_update_bits(data->regmap, RPR0521_REG_PXS_CTRL,
> +		RPR0521_PXS_PERSISTENCE_MASK,
> +		RPR0521_PXS_PERSISTENCE_DRDY);
> +	if (err) {
> +		dev_err(&data->client->dev, "PS control reg write fail.\n");
> +		return -EBUSY;
> +		}
> +
> +	/* Ignore latch and mode because of drdy */
> +	err = regmap_write(data->regmap, RPR0521_REG_INTERRUPT,
> +		RPR0521_INTERRUPT_INT_REASSERT_DISABLE |
> +		RPR0521_INTERRUPT_INT_TRIG_ALS_DISABLE |
> +		RPR0521_INTERRUPT_INT_TRIG_PS_ENABLE
> +		);
> +	if (err) {
> +		dev_err(&data->client->dev, "Interrupt setup write fail.\n");
> +		return -EBUSY;
> +		}
> +
> +	return 0;
> +}
> +
> +static int rpr0521_write_int_disable(struct rpr0521_data *data)
> +{
> +	/* Don't care of clearing mode, assert and latch. */
> +	return regmap_write(data->regmap, RPR0521_REG_INTERRUPT,
> +				RPR0521_INTERRUPT_INT_TRIG_ALS_DISABLE |
> +				RPR0521_INTERRUPT_INT_TRIG_PS_DISABLE
> +				);
> +}
> +
> +/* Trigger producer enable / disable */
> +static int rpr0521_pxs_drdy_set_state(struct iio_trigger *trigger,
> +	bool enable_drdy)
> +{
> +	struct iio_dev *indio_dev = iio_trigger_get_drvdata(trigger);
> +	struct rpr0521_data *data = iio_priv(indio_dev);
> +	int err;
> +
> +	if (enable_drdy) {
> +		/* ENABLE DRDY */
> +		mutex_lock(&data->lock);
> +		err = rpr0521_set_power_state(data, true,
> +			(RPR0521_MODE_PXS_MASK | RPR0521_MODE_ALS_MASK));
So it will always capture all the channels?  Fair enough but you will
need to specify available_scan_masks to let the core know it it needs
to demux the channels to provide only those requested by userspace.
> +		mutex_unlock(&data->lock);
> +		if (err)
> +			goto rpr0521_set_state_err;
> +
> +		err = rpr0521_write_int_enable(data);
> +		if (err)
> +			goto rpr0521_set_state_err;
> +	} else {
> +		/* DISABLE DRDY */
> +		mutex_lock(&data->lock);
> +		err = rpr0521_set_power_state(data, false,
> +			(RPR0521_MODE_PXS_MASK | RPR0521_MODE_ALS_MASK));
> +		mutex_unlock(&data->lock);
> +		if (err)
> +			goto rpr0521_set_state_err;
> +
> +		err = rpr0521_write_int_disable(data);
> +		if (err)
> +			goto rpr0521_set_state_err;
> +	}
> +	data->drdy_trigger_on = enable_drdy;
> +
> +	return 0;
> +
> +rpr0521_set_state_err:
> +	dev_err(&data->client->dev, "rpr0521_pxs_drdy_set_state failed\n");
> +	return err;
> +}
> +
> +static const struct iio_trigger_ops rpr0521_trigger_ops = {
> +	.set_trigger_state = rpr0521_pxs_drdy_set_state,
> +	.owner = THIS_MODULE,
> +	};
> +
>  static int rpr0521_get_gain(struct rpr0521_data *data, int chan,
>  			    int *val, int *val2)
>  {
> @@ -481,6 +661,9 @@ static int rpr0521_read_raw(struct iio_dev *indio_dev,
>  		if (chan->type != IIO_INTENSITY && chan->type != IIO_PROXIMITY)
>  			return -EINVAL;
>  
> +		if (iio_buffer_enabled(indio_dev))
> +			return -EBUSY;
> +
>  		device_mask = rpr0521_data_reg[chan->address].device_mask;
>  
>  		mutex_lock(&data->lock);
> @@ -624,6 +807,7 @@ static int rpr0521_init(struct rpr0521_data *data)
>  static int rpr0521_poweroff(struct rpr0521_data *data)
>  {
>  	int ret;
> +	int tmp;
>  
>  	ret = regmap_update_bits(data->regmap, RPR0521_REG_MODE_CTRL,
>  				 RPR0521_MODE_ALS_MASK |
> @@ -636,6 +820,16 @@ static int rpr0521_poweroff(struct rpr0521_data *data)
>  	data->als_dev_en = false;
>  	data->pxs_dev_en = false;
>  
> +	/*
> +	 * Int pin keeps state after power off. Set pin to high impedance
> +	 * mode to prevent power drain.
> +	 */
> +	ret = regmap_read(data->regmap, RPR0521_REG_INTERRUPT, &tmp);
> +	if (ret) {
> +		dev_err(&data->client->dev, "Failed to reset int pin.\n");
> +		return ret;
> +		}
Could be an email client weirdness but that bracket indent looks wrong...
> +
>  	return 0;
>  }
>  
> @@ -708,12 +902,77 @@ static int rpr0521_probe(struct i2c_client *client,
>  	pm_runtime_set_autosuspend_delay(&client->dev, RPR0521_SLEEP_DELAY_MS);
>  	pm_runtime_use_autosuspend(&client->dev);
>  
> +	/*
> +	 * If sensor write/read  is needed in _probe after _use_autosuspend,
> +	 * sensor needs to be _resumed first using rpr0521_set_power_state().
> +	 */
> +
> +	/* IRQ to trigger setup */
> +	if (client->irq) {
> +		/* Trigger0 producer setup */
> +		data->drdy_trigger0 = devm_iio_trigger_alloc(
> +			indio_dev->dev.parent,
> +			"%s-dev%d", indio_dev->name, indio_dev->id);
> +		if (!data->drdy_trigger0) {
> +			ret = -ENOMEM;
> +			goto err_pm_disable;
> +		}
> +		data->drdy_trigger0->dev.parent = indio_dev->dev.parent;
> +		data->drdy_trigger0->ops = &rpr0521_trigger_ops;
> +		iio_trigger_set_drvdata(data->drdy_trigger0, indio_dev);
> +
> +		/* Ties irq to trigger producer handler. */
> +		ret = devm_request_threaded_irq(&client->dev, client->irq,
> +			rpr0521_drdy_irq_handler,
> +			NULL,
> +			IRQF_TRIGGER_FALLING | IRQF_ONESHOT,
> +			RPR0521_IRQ_NAME,
> +			indio_dev);
> +		if (ret < 0) {
> +			dev_err(&client->dev, "request irq %d for trigger0 failed\n",
> +				client->irq);
> +			goto err_pm_disable;
> +			}
> +
> +		ret = iio_trigger_register(data->drdy_trigger0);
> +		if (ret) {
> +			dev_err(&client->dev, "iio trigger register failed\n");
> +			goto err_pm_disable;
> +		}
> +
> +		/*
> +		 * Now whole pipe from physical interrupt (irq defined by
> +		 * devicetree to device) to trigger0 output is set up.
> +		 */
> +
> +		/* Trigger consumer setup */
> +		ret = iio_triggered_buffer_setup(indio_dev,
> +			&iio_pollfunc_store_time,
> +			rpr0521_trigger_consumer_handler,
> +			NULL);		//Use default _ops
> +		if (ret < 0) {
> +			dev_err(&client->dev, "iio triggered buffer setup failed\n");
> +			/* Count on devm to free_irq */
> +			goto err_iio_trigger_unregister;
> +		}
> +	}
> +
>  	ret = iio_device_register(indio_dev);
>  	if (ret)
> -		goto err_pm_disable;
> +		goto err_iio_triggered_buffer_cleanup;
> +
> +	if (client->irq) {
> +		/* Set trigger0 as current trigger (and inc refcount) */
> +		indio_dev->trig = iio_trigger_get(data->drdy_trigger0);
> +	}
>  
>  	return 0;
>  
> +err_iio_triggered_buffer_cleanup:
> +	iio_triggered_buffer_cleanup(indio_dev);
> +err_iio_trigger_unregister:
> +	if (data->drdy_trigger0)
> +		iio_trigger_unregister(data->drdy_trigger0);
>  err_pm_disable:
>  	pm_runtime_disable(&client->dev);
>  	pm_runtime_set_suspended(&client->dev);
> @@ -727,14 +986,23 @@ err_poweroff:
>  static int rpr0521_remove(struct i2c_client *client)
>  {
>  	struct iio_dev *indio_dev = i2c_get_clientdata(client);
> +	struct rpr0521_data *data = iio_priv(indio_dev);
> +
> +	if (indio_dev->trig)
> +		iio_trigger_put(indio_dev->trig);
>  
>  	iio_device_unregister(indio_dev);
>  
> +	iio_triggered_buffer_cleanup(indio_dev);
> +
> +	if (data->drdy_trigger0)
> +		iio_trigger_unregister(data->drdy_trigger0);
> +
>  	pm_runtime_disable(&client->dev);
>  	pm_runtime_set_suspended(&client->dev);
>  	pm_runtime_put_noidle(&client->dev);
>  
> -	rpr0521_poweroff(iio_priv(indio_dev));
> +	rpr0521_poweroff(data);
>  
>  	return 0;
>  }
> 

Re: [PATCH v3 9/9] iio: light: rpr0521 triggered buffer

[Koivunen, Mikko]

On 7.5.2017 15:09, Jonathan Cameron wrote:
> On 05/05/17 12:19, Mikko Koivunen wrote:
>> Set up and use triggered buffer if there is irq defined for device in
>> device tree. Trigger producer triggers from rpr0521 drdy interrupt line.
>> Trigger consumer reads rpr0521 data to scan buffer.
>> Depends on previous commits of _scale and _offset.
>>
>> Signed-off-by: Mikko Koivunen <mikko.koivunen@fi.rohmeurope.com>A few elements inline.
> Also looking for Daniel input on this one in particular as it's
> a little fiddly to say the least!
>
> I wonder if the balance between what goes on the trigger enable
> vs the buffer enable is correct.  Seems there are some issues around
> runtime pm that need resolving.  It may be that you need a bit of
> an on off dance to ensure the power is enabled when needed for
> trigger setup and for buffers being enabled.
>
> Jonathan

Thanks for pointers.  I didn't think of buffer preenable before. Looking
into it now.

>> ---
>> Patch v2->v3 changes:
>> .shift = 0 removed
>> reordered functions to remove forward declarations
>> whitespace changes
>> refactored some update_bits->write, no "err = err || *"-pattern
>> refactored trigger_consumer_handler
>> reordered _probe() and _remove()
>> added int clear on poweroff()
>> checkpatch.pl OK
>> Tested on LeMaker HiKey with AOSP7.1 kernel 4.4.
>> Builds ok with torvalds/linux feb 27.
>>
>>  drivers/iio/light/rpr0521.c |  274 ++++++++++++++++++++++++++++++++++++++++++-
>>  1 file changed, 271 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/iio/light/rpr0521.c b/drivers/iio/light/rpr0521.c
>> index 2dc5e38..6feaa78 100644
>> --- a/drivers/iio/light/rpr0521.c
>> +++ b/drivers/iio/light/rpr0521.c
>> @@ -9,7 +9,7 @@
>>   *
>>   * IIO driver for RPR-0521RS (7-bit I2C slave address 0x38).
>>   *
>> - * TODO: illuminance channel, buffer
>> + * TODO: illuminance channel
>>   */
>>  
>>  #include <linux/module.h>
>> @@ -20,6 +20,10 @@
>>  #include <linux/acpi.h>
>>  
>>  #include <linux/iio/iio.h>
>> +#include <linux/iio/buffer.h>
>> +#include <linux/iio/trigger.h>
>> +#include <linux/iio/trigger_consumer.h>
>> +#include <linux/iio/triggered_buffer.h>
>>  #include <linux/iio/sysfs.h>
>>  #include <linux/pm_runtime.h>
>>  
>> @@ -30,6 +34,7 @@
>>  #define RPR0521_REG_PXS_DATA		0x44 /* 16-bit, little endian */
>>  #define RPR0521_REG_ALS_DATA0		0x46 /* 16-bit, little endian */
>>  #define RPR0521_REG_ALS_DATA1		0x48 /* 16-bit, little endian */
>> +#define RPR0521_REG_INTERRUPT		0x4A
>>  #define RPR0521_REG_PS_OFFSET_LSB	0x53
>>  #define RPR0521_REG_ID			0x92
>>  
>> @@ -42,16 +47,29 @@
>>  #define RPR0521_ALS_DATA1_GAIN_SHIFT	2
>>  #define RPR0521_PXS_GAIN_MASK		GENMASK(5, 4)
>>  #define RPR0521_PXS_GAIN_SHIFT		4
>> +#define RPR0521_PXS_PERSISTENCE_MASK	GENMASK(3, 0)
>> +#define RPR0521_INTERRUPT_INT_TRIG_PS_MASK	BIT(0)
>> +#define RPR0521_INTERRUPT_INT_TRIG_ALS_MASK	BIT(1)
>> +#define RPR0521_INTERRUPT_INT_REASSERT_MASK	BIT(3)
>>  
>>  #define RPR0521_MODE_ALS_ENABLE		BIT(7)
>>  #define RPR0521_MODE_ALS_DISABLE	0x00
>>  #define RPR0521_MODE_PXS_ENABLE		BIT(6)
>>  #define RPR0521_MODE_PXS_DISABLE	0x00
>> +#define RPR0521_PXS_PERSISTENCE_DRDY	0x00
>> +
>> +#define RPR0521_INTERRUPT_INT_TRIG_PS_ENABLE	BIT(0)
>> +#define RPR0521_INTERRUPT_INT_TRIG_PS_DISABLE	0x00
>> +#define RPR0521_INTERRUPT_INT_TRIG_ALS_ENABLE	BIT(1)
>> +#define RPR0521_INTERRUPT_INT_TRIG_ALS_DISABLE	0x00
>> +#define RPR0521_INTERRUPT_INT_REASSERT_ENABLE	BIT(3)
>> +#define RPR0521_INTERRUPT_INT_REASSERT_DISABLE	0x00
>>  
>>  #define RPR0521_MANUFACT_ID		0xE0
>>  #define RPR0521_DEFAULT_MEAS_TIME	0x06 /* ALS - 100ms, PXS - 100ms */
>>  
>>  #define RPR0521_DRV_NAME		"RPR0521"
>> +#define RPR0521_IRQ_NAME		"rpr0521_event"
>>  #define RPR0521_REGMAP_NAME		"rpr0521_regmap"
>>  
>>  #define RPR0521_SLEEP_DELAY_MS	2000
>> @@ -167,6 +185,9 @@ struct rpr0521_data {
>>  	bool als_dev_en;
>>  	bool pxs_dev_en;
>>  
>> +	struct iio_trigger *drdy_trigger0;
>> +	bool drdy_trigger_on;
>> +
>>  	/* optimize runtime pm ops - enable/disable device only if needed */
>>  	bool als_ps_need_en;
>>  	bool pxs_ps_need_en;
>> @@ -196,6 +217,13 @@ static const struct attribute_group rpr0521_attribute_group = {
>>  	.attrs = rpr0521_attributes,
>>  };
>>  
>> +/* Order of the channel data in buffer */
>> +enum rpr0521_scan_index_order {
>> +	RPR0521_CHAN_INDEX_PXS,
>> +	RPR0521_CHAN_INDEX_BOTH,
>> +	RPR0521_CHAN_INDEX_IR,
>> +};
>> +
>>  static const struct iio_chan_spec rpr0521_channels[] = {
>>  	{
>>  		.type = IIO_PROXIMITY,
>> @@ -204,6 +232,13 @@ static const struct iio_chan_spec rpr0521_channels[] = {
>>  			BIT(IIO_CHAN_INFO_OFFSET) |
>>  			BIT(IIO_CHAN_INFO_SCALE),
>>  		.info_mask_shared_by_all = BIT(IIO_CHAN_INFO_SAMP_FREQ),
>> +		.scan_index = RPR0521_CHAN_INDEX_PXS,
>> +		.scan_type = {
>> +			.sign = 'u',
>> +			.realbits = 16,
>> +			.storagebits = 16,
>> +			.endianness = IIO_LE,
>> +		},
>>  	},
>>  	{
>>  		.type = IIO_INTENSITY,
>> @@ -213,6 +248,13 @@ static const struct iio_chan_spec rpr0521_channels[] = {
>>  		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
>>  			BIT(IIO_CHAN_INFO_SCALE),
>>  		.info_mask_shared_by_all = BIT(IIO_CHAN_INFO_SAMP_FREQ),
>> +		.scan_index = RPR0521_CHAN_INDEX_BOTH,
>> +		.scan_type = {
>> +			.sign = 'u',
>> +			.realbits = 16,
>> +			.storagebits = 16,
>> +			.endianness = IIO_LE,
>> +		},
>>  	},
>>  	{
>>  		.type = IIO_INTENSITY,
>> @@ -222,6 +264,13 @@ static const struct iio_chan_spec rpr0521_channels[] = {
>>  		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
>>  			BIT(IIO_CHAN_INFO_SCALE),
>>  		.info_mask_shared_by_all = BIT(IIO_CHAN_INFO_SAMP_FREQ),
>> +		.scan_index = RPR0521_CHAN_INDEX_IR,
>> +		.scan_type = {
>> +			.sign = 'u',
>> +			.realbits = 16,
>> +			.storagebits = 16,
>> +			.endianness = IIO_LE,
>> +		},
>>  	},
>>  };
>>  
>> @@ -330,6 +379,137 @@ static int rpr0521_set_power_state(struct rpr0521_data *data, bool on,
>>  	return 0;
>>  }
>>  
>> +/* IRQ to trigger handler */
>> +static irqreturn_t rpr0521_drdy_irq_handler(int irq, void *private)
>> +{
>> +	struct iio_dev *indio_dev = private;
>> +	struct rpr0521_data *data = iio_priv(indio_dev);
>> +
>> +	/*
>> +	 * Sometimes static on floating (hi-z) interrupt line causes interrupt.
>> +	 * Notify trigger0 consumers/subscribers only if trigger has been
>> +	 * enabled. This is to prevent i2c writes to sensor which is actually
>> +	 * powered off.
>> +	 */
>> +	if (data->drdy_trigger_on)
>> +		iio_trigger_poll(data->drdy_trigger0);
>> +
>> +	return IRQ_HANDLED;
>> +}
>> +
>> +static irqreturn_t rpr0521_trigger_consumer_handler(int irq, void *p)
>> +{
>> +	struct iio_poll_func *pf = p;
>> +	struct iio_dev *indio_dev = pf->indio_dev;
>> +	struct rpr0521_data *data = iio_priv(indio_dev);
>> +	int err;
>> +
>> +	u8 buffer[16]; /* 3 16-bit channels + padding + ts */
>> +
>> +	err = regmap_bulk_read(data->regmap, RPR0521_REG_PXS_DATA,
>> +		&buffer,
>> +		(3 * 2) + 1);	/* 3 * 16-bit + (discarded) int clear reg. */
>> +	if (!err)
>> +		iio_push_to_buffers_with_timestamp(indio_dev,
>> +			buffer, pf->timestamp);
>> +	else
>> +		dev_err(&data->client->dev,
>> +			"Trigger consumer can't read from sensor.\n");
>> +	/*
>> +	 * Note when using CONFIG_PM: the sensor is powered on in trigger
>> +	 * producer _set_state(). If using different trigger as current_trigger
>> +	 * the sensor must be powered on and measurement enabled using
>> +	 * _set_power_state().
> So it won't work with a different trigger?  If so you want to do
> a validate trigger to stop it trying to use anything else.
>
> You could move the power enable to the buffer preenable instead
> if I understand your comment correctly.  That would make it work either
> way.

Ok, valid point. Looking into that.

>> +	 */
>> +
>> +	iio_trigger_notify_done(indio_dev->trig);
>> +
>> +	return IRQ_HANDLED;
>> +}
>> +
>> +static int rpr0521_write_int_enable(struct rpr0521_data *data)
>> +{
>> +	int err;
>> +
>> +	/* Interrupt after each measurement */
>> +	err = regmap_update_bits(data->regmap, RPR0521_REG_PXS_CTRL,
>> +		RPR0521_PXS_PERSISTENCE_MASK,
>> +		RPR0521_PXS_PERSISTENCE_DRDY);
>> +	if (err) {
>> +		dev_err(&data->client->dev, "PS control reg write fail.\n");
>> +		return -EBUSY;
>> +		}
>> +
>> +	/* Ignore latch and mode because of drdy */
>> +	err = regmap_write(data->regmap, RPR0521_REG_INTERRUPT,
>> +		RPR0521_INTERRUPT_INT_REASSERT_DISABLE |
>> +		RPR0521_INTERRUPT_INT_TRIG_ALS_DISABLE |
>> +		RPR0521_INTERRUPT_INT_TRIG_PS_ENABLE
>> +		);
>> +	if (err) {
>> +		dev_err(&data->client->dev, "Interrupt setup write fail.\n");
>> +		return -EBUSY;
>> +		}
>> +
>> +	return 0;
>> +}
>> +
>> +static int rpr0521_write_int_disable(struct rpr0521_data *data)
>> +{
>> +	/* Don't care of clearing mode, assert and latch. */
>> +	return regmap_write(data->regmap, RPR0521_REG_INTERRUPT,
>> +				RPR0521_INTERRUPT_INT_TRIG_ALS_DISABLE |
>> +				RPR0521_INTERRUPT_INT_TRIG_PS_DISABLE
>> +				);
>> +}
>> +
>> +/* Trigger producer enable / disable */
>> +static int rpr0521_pxs_drdy_set_state(struct iio_trigger *trigger,
>> +	bool enable_drdy)
>> +{
>> +	struct iio_dev *indio_dev = iio_trigger_get_drvdata(trigger);
>> +	struct rpr0521_data *data = iio_priv(indio_dev);
>> +	int err;
>> +
>> +	if (enable_drdy) {
>> +		/* ENABLE DRDY */
>> +		mutex_lock(&data->lock);
>> +		err = rpr0521_set_power_state(data, true,
>> +			(RPR0521_MODE_PXS_MASK | RPR0521_MODE_ALS_MASK));
> So it will always capture all the channels?  Fair enough but you will
> need to specify available_scan_masks to let the core know it it needs
> to demux the channels to provide only those requested by userspace.

Ack. Not 100% sure if I got it right for v4 though.

>> +		mutex_unlock(&data->lock);
>> +		if (err)
>> +			goto rpr0521_set_state_err;
>> +
>> +		err = rpr0521_write_int_enable(data);
>> +		if (err)
>> +			goto rpr0521_set_state_err;
>> +	} else {
>> +		/* DISABLE DRDY */
>> +		mutex_lock(&data->lock);
>> +		err = rpr0521_set_power_state(data, false,
>> +			(RPR0521_MODE_PXS_MASK | RPR0521_MODE_ALS_MASK));
>> +		mutex_unlock(&data->lock);
>> +		if (err)
>> +			goto rpr0521_set_state_err;
>> +
>> +		err = rpr0521_write_int_disable(data);
>> +		if (err)
>> +			goto rpr0521_set_state_err;
>> +	}
>> +	data->drdy_trigger_on = enable_drdy;
>> +
>> +	return 0;
>> +
>> +rpr0521_set_state_err:
>> +	dev_err(&data->client->dev, "rpr0521_pxs_drdy_set_state failed\n");
>> +	return err;
>> +}
>> +
>> +static const struct iio_trigger_ops rpr0521_trigger_ops = {
>> +	.set_trigger_state = rpr0521_pxs_drdy_set_state,
>> +	.owner = THIS_MODULE,
>> +	};
>> +
>>  static int rpr0521_get_gain(struct rpr0521_data *data, int chan,
>>  			    int *val, int *val2)
>>  {
>> @@ -481,6 +661,9 @@ static int rpr0521_read_raw(struct iio_dev *indio_dev,
>>  		if (chan->type != IIO_INTENSITY && chan->type != IIO_PROXIMITY)
>>  			return -EINVAL;
>>  
>> +		if (iio_buffer_enabled(indio_dev))
>> +			return -EBUSY;
>> +
>>  		device_mask = rpr0521_data_reg[chan->address].device_mask;
>>  
>>  		mutex_lock(&data->lock);
>> @@ -624,6 +807,7 @@ static int rpr0521_init(struct rpr0521_data *data)
>>  static int rpr0521_poweroff(struct rpr0521_data *data)
>>  {
>>  	int ret;
>> +	int tmp;
>>  
>>  	ret = regmap_update_bits(data->regmap, RPR0521_REG_MODE_CTRL,
>>  				 RPR0521_MODE_ALS_MASK |
>> @@ -636,6 +820,16 @@ static int rpr0521_poweroff(struct rpr0521_data *data)
>>  	data->als_dev_en = false;
>>  	data->pxs_dev_en = false;
>>  
>> +	/*
>> +	 * Int pin keeps state after power off. Set pin to high impedance
>> +	 * mode to prevent power drain.
>> +	 */
>> +	ret = regmap_read(data->regmap, RPR0521_REG_INTERRUPT, &tmp);
>> +	if (ret) {
>> +		dev_err(&data->client->dev, "Failed to reset int pin.\n");
>> +		return ret;
>> +		}
> Could be an email client weirdness but that bracket indent looks wrong...

It is, ack.

>> +
>>  	return 0;
>>  }
>>  
>> @@ -708,12 +902,77 @@ static int rpr0521_probe(struct i2c_client *client,
>>  	pm_runtime_set_autosuspend_delay(&client->dev, RPR0521_SLEEP_DELAY_MS);
>>  	pm_runtime_use_autosuspend(&client->dev);
>>  
>> +	/*
>> +	 * If sensor write/read  is needed in _probe after _use_autosuspend,
>> +	 * sensor needs to be _resumed first using rpr0521_set_power_state().
>> +	 */
>> +
>> +	/* IRQ to trigger setup */
>> +	if (client->irq) {
>> +		/* Trigger0 producer setup */
>> +		data->drdy_trigger0 = devm_iio_trigger_alloc(
>> +			indio_dev->dev.parent,
>> +			"%s-dev%d", indio_dev->name, indio_dev->id);
>> +		if (!data->drdy_trigger0) {
>> +			ret = -ENOMEM;
>> +			goto err_pm_disable;
>> +		}
>> +		data->drdy_trigger0->dev.parent = indio_dev->dev.parent;
>> +		data->drdy_trigger0->ops = &rpr0521_trigger_ops;
>> +		iio_trigger_set_drvdata(data->drdy_trigger0, indio_dev);
>> +
>> +		/* Ties irq to trigger producer handler. */
>> +		ret = devm_request_threaded_irq(&client->dev, client->irq,
>> +			rpr0521_drdy_irq_handler,
>> +			NULL,
>> +			IRQF_TRIGGER_FALLING | IRQF_ONESHOT,
>> +			RPR0521_IRQ_NAME,
>> +			indio_dev);
>> +		if (ret < 0) {
>> +			dev_err(&client->dev, "request irq %d for trigger0 failed\n",
>> +				client->irq);
>> +			goto err_pm_disable;
>> +			}
>> +
>> +		ret = iio_trigger_register(data->drdy_trigger0);
>> +		if (ret) {
>> +			dev_err(&client->dev, "iio trigger register failed\n");
>> +			goto err_pm_disable;
>> +		}
>> +
>> +		/*
>> +		 * Now whole pipe from physical interrupt (irq defined by
>> +		 * devicetree to device) to trigger0 output is set up.
>> +		 */
>> +
>> +		/* Trigger consumer setup */
>> +		ret = iio_triggered_buffer_setup(indio_dev,
>> +			&iio_pollfunc_store_time,
>> +			rpr0521_trigger_consumer_handler,
>> +			NULL);		//Use default _ops
>> +		if (ret < 0) {
>> +			dev_err(&client->dev, "iio triggered buffer setup failed\n");
>> +			/* Count on devm to free_irq */
>> +			goto err_iio_trigger_unregister;
>> +		}
>> +	}
>> +
>>  	ret = iio_device_register(indio_dev);
>>  	if (ret)
>> -		goto err_pm_disable;
>> +		goto err_iio_triggered_buffer_cleanup;
>> +
>> +	if (client->irq) {
>> +		/* Set trigger0 as current trigger (and inc refcount) */
>> +		indio_dev->trig = iio_trigger_get(data->drdy_trigger0);
>> +	}
>>  
>>  	return 0;
>>  
>> +err_iio_triggered_buffer_cleanup:
>> +	iio_triggered_buffer_cleanup(indio_dev);
>> +err_iio_trigger_unregister:
>> +	if (data->drdy_trigger0)
>> +		iio_trigger_unregister(data->drdy_trigger0);
>>  err_pm_disable:
>>  	pm_runtime_disable(&client->dev);
>>  	pm_runtime_set_suspended(&client->dev);
>> @@ -727,14 +986,23 @@ err_poweroff:
>>  static int rpr0521_remove(struct i2c_client *client)
>>  {
>>  	struct iio_dev *indio_dev = i2c_get_clientdata(client);
>> +	struct rpr0521_data *data = iio_priv(indio_dev);
>> +
>> +	if (indio_dev->trig)
>> +		iio_trigger_put(indio_dev->trig);
>>  
>>  	iio_device_unregister(indio_dev);
>>  
>> +	iio_triggered_buffer_cleanup(indio_dev);
>> +
>> +	if (data->drdy_trigger0)
>> +		iio_trigger_unregister(data->drdy_trigger0);
>> +
>>  	pm_runtime_disable(&client->dev);
>>  	pm_runtime_set_suspended(&client->dev);
>>  	pm_runtime_put_noidle(&client->dev);
>>  
>> -	rpr0521_poweroff(iio_priv(indio_dev));
>> +	rpr0521_poweroff(data);
>>  
>>  	return 0;
>>  }
>>
>

Re: [PATCH v3 1/9] iio: light: rpr0521 disable sensor -bugfix

[Daniel Baluta]

On Fri, May 5, 2017 at 2:19 PM, Mikko Koivunen
<mikko.koivunen@fi.rohmeurope.com> wrote:
> Sensor was marked enabled on each call even if the call was for disabling
> sensor.
>
> Signed-off-by: Mikko Koivunen <mikko.koivunen@fi.rohmeurope.com>
> ---
> Tested on LeMaker HiKey with AOSP7.1 kernel 4.4.
> Patch v2->v3 changes:
> whitespace change.
>
>  drivers/iio/light/rpr0521.c |   10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/iio/light/rpr0521.c b/drivers/iio/light/rpr0521.c
> index 7de0f39..03504f6 100644
> --- a/drivers/iio/light/rpr0521.c
> +++ b/drivers/iio/light/rpr0521.c
> @@ -197,7 +197,10 @@ static int rpr0521_als_enable(struct rpr0521_data *data, u8 status)
>         if (ret < 0)
>                 return ret;
>
> -       data->als_dev_en = true;

How about:

data->als_dev_en = !!(status & RPR0521_MODE_ALS_MASK);

Thanks for fixing this!

Re: [PATCH v3 1/9] iio: light: rpr0521 disable sensor -bugfix

[Koivunen, Mikko]

On 5.5.2017 14:24, Daniel Baluta wrote:=0A=
> On Fri, May 5, 2017 at 2:19 PM, Mikko Koivunen=0A=
> <mikko.koivunen@fi.rohmeurope.com> wrote:=0A=
>> Sensor was marked enabled on each call even if the call was for disablin=
g=0A=
>> sensor.=0A=
>>=0A=
>> Signed-off-by: Mikko Koivunen <mikko.koivunen@fi.rohmeurope.com>=0A=
>> ---=0A=
>> Tested on LeMaker HiKey with AOSP7.1 kernel 4.4.=0A=
>> Patch v2->v3 changes:=0A=
>> whitespace change.=0A=
>>=0A=
>>  drivers/iio/light/rpr0521.c |   10 ++++++++--=0A=
>>  1 file changed, 8 insertions(+), 2 deletions(-)=0A=
>>=0A=
>> diff --git a/drivers/iio/light/rpr0521.c b/drivers/iio/light/rpr0521.c=
=0A=
>> index 7de0f39..03504f6 100644=0A=
>> --- a/drivers/iio/light/rpr0521.c=0A=
>> +++ b/drivers/iio/light/rpr0521.c=0A=
>> @@ -197,7 +197,10 @@ static int rpr0521_als_enable(struct rpr0521_data *=
data, u8 status)=0A=
>>         if (ret < 0)=0A=
>>                 return ret;=0A=
>>=0A=
>> -       data->als_dev_en =3D true;=0A=
> How about:=0A=
>=0A=
> data->als_dev_en =3D !!(status & RPR0521_MODE_ALS_MASK);=0A=
>=0A=
> Thanks for fixing this!=0A=
>=0A=
I got the impression previously that we should prefer verbosity instead=0A=
of shortness when it helps understanding the code. Otherwise yes, that=0A=
could work too.=0A=
=0A=
-Mikko=0A=
=0A=

Re: [PATCH v3 1/9] iio: light: rpr0521 disable sensor -bugfix

[Daniel Baluta]

On Mon, May 15, 2017 at 1:25 PM, Koivunen, Mikko
<Mikko.Koivunen@fi.rohmeurope.com> wrote:
> On 5.5.2017 14:24, Daniel Baluta wrote:
>> On Fri, May 5, 2017 at 2:19 PM, Mikko Koivunen
>> <mikko.koivunen@fi.rohmeurope.com> wrote:
>>> Sensor was marked enabled on each call even if the call was for disabling
>>> sensor.
>>>
>>> Signed-off-by: Mikko Koivunen <mikko.koivunen@fi.rohmeurope.com>
>>> ---
>>> Tested on LeMaker HiKey with AOSP7.1 kernel 4.4.
>>> Patch v2->v3 changes:
>>> whitespace change.
>>>
>>>  drivers/iio/light/rpr0521.c |   10 ++++++++--
>>>  1 file changed, 8 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/iio/light/rpr0521.c b/drivers/iio/light/rpr0521.c
>>> index 7de0f39..03504f6 100644
>>> --- a/drivers/iio/light/rpr0521.c
>>> +++ b/drivers/iio/light/rpr0521.c
>>> @@ -197,7 +197,10 @@ static int rpr0521_als_enable(struct rpr0521_data *data, u8 status)
>>>         if (ret < 0)
>>>                 return ret;
>>>
>>> -       data->als_dev_en = true;
>> How about:
>>
>> data->als_dev_en = !!(status & RPR0521_MODE_ALS_MASK);
>>
>> Thanks for fixing this!
>>
> I got the impression previously that we should prefer verbosity instead
> of shortness when it helps understanding the code. Otherwise yes, that
> could work too.

It depends on the mood :). But you are right, lets go with your variant.

Daniel.