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@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;
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@xxxxxxxxxxxxxxx
>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>>
>
--
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
