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@xxxxxxxxxxxxxxxxx>
> 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;
>> }
>>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-iio" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
