Re: [PATCH] leds: leds-pca963x: add power management support

From: Matt Ranostay
Date: Fri Sep 30 2016 - 18:58:38 EST


On Fri, Sep 30, 2016 at 2:23 PM, Jacek Anaszewski
<jacek.anaszewski@xxxxxxxxx> wrote:
> Hi Matt,
>
> Thanks for the patch.
>
>
> On 09/30/2016 09:27 PM, Matt Ranostay wrote:
>>
>> Turn off LEDS on suspend and put device in low power state, and restore
>> settings on resume.
>>
>> Cc: Tony Lindgren <tony@xxxxxxxxxxx>
>> Cc: Richard Purdie <rpurdie@xxxxxxxxx>
>> Cc: Jacek Anaszewski <j.anaszewski@xxxxxxxxxxx>
>> Signed-off-by: Matt Ranostay <matt@xxxxxxxxxxxxxxxxxxx>
>> ---
>> drivers/leds/leds-pca963x.c | 31 +++++++++++++++++++++++++++++++
>> 1 file changed, 31 insertions(+)
>>
>> diff --git a/drivers/leds/leds-pca963x.c b/drivers/leds/leds-pca963x.c
>> index 407eba11e187..4840bdaa1b48 100644
>> --- a/drivers/leds/leds-pca963x.c
>> +++ b/drivers/leds/leds-pca963x.c
>> @@ -382,6 +382,7 @@ static int pca963x_probe(struct i2c_client *client,
>>
>> pca963x[i].led_cdev.name = pca963x[i].name;
>> pca963x[i].led_cdev.brightness_set_blocking =
>> pca963x_led_set;
>> + pca963x[i].led_cdev.flags = LED_CORE_SUSPENDRESUME;
>>
>> if (pdata && pdata->blink_type == PCA963X_HW_BLINK)
>> pca963x[i].led_cdev.blink_set = pca963x_blink_set;
>> @@ -422,10 +423,40 @@ static int pca963x_remove(struct i2c_client *client)
>> return 0;
>> }
>>
>> +static int pca963x_set_power(struct i2c_client *client, bool state)
>> +{
>> + return i2c_smbus_write_byte_data(client, PCA963X_MODE1,
>> + state ? 0 : BIT(4));
>> +}
>> +
>> +static int pca963x_suspend(struct device *dev)
>> +{
>> + struct i2c_client *client = to_i2c_client(dev);
>> +
>> + return pca963x_set_power(client, false);
>> +}
>> +
>> +static int pca963x_resume(struct device *dev)
>> +{
>> + struct i2c_client *client = to_i2c_client(dev);
>> + int ret;
>> +
>> + ret = pca963x_set_power(client, true);
>> +
>> + /* wait for oscillator enabling */
>> + if (!ret)
>> + usleep_range(500, 1000);
>> +
>> + return ret;
>> +}
>> +
>> +static SIMPLE_DEV_PM_OPS(pca963x_pm, pca963x_suspend, pca963x_resume);
>> +
>> static struct i2c_driver pca963x_driver = {
>> .driver = {
>> .name = "leds-pca963x",
>> .of_match_table = of_match_ptr(of_pca963x_match),
>> + .pm = &pca963x_pm,
>
>
> pm ops are initialized in led-class.c. If LED_CORE_SUSPENDRESUME
> flag is set by a LED class driver, then the brightness will be
> set to 0 on suspend and brought back on resume.

Understood.

>
> LED class driver's responsibility is to enter power down mode if
> all LED class devices it exposes have their brightness set to 0.
>
> Effectively, you should avoid defining pm ops, but instead
> just track how many LEDs are on, and set the device in the power
> down mode if brightness of the last active LED is set to LED_OFF.
> Similarly the device should be powered up if any of LEDs brightness
> is set to a value greater than 0.

Ok in this case it would make sense to use runtime PM.. which
shouldn't be too hard to do. Will fix in v2

>
>> },
>> .probe = pca963x_probe,
>> .remove = pca963x_remove,
>>
>
> --
> Best regards,
> Jacek Anaszewski