* Re: [PATCH v4 1/1] cap11xx: add runtime PM support
2015-08-23 21:17 ` [PATCH v4 1/1] " Matt Ranostay
@ 2015-09-17 6:39 ` Matt Ranostay
2015-10-02 18:06 ` Dmitry Torokhov
1 sibling, 0 replies; 4+ messages in thread
From: Matt Ranostay @ 2015-09-17 6:39 UTC (permalink / raw)
To: dmitry.torokhov@gmail.com; +Cc: linux-input@vger.kernel.org
Poke for comments on this :)
> On Aug 23, 2015, at 14:17, Matt Ranostay <mranostay@gmail.com> wrote:
>
> Puts device into DEEP SLEEP when no LEDs are in in the on-state, and no
> input_handlers are in use. Also uses *_autosuspend to prevent a LED
> trigger from constantly suspending and resuming device.
>
> Signed-off-by: Matt Ranostay <mranostay@gmail.com>
> ---
> drivers/input/keyboard/cap11xx.c | 84 +++++++++++++++++++++++++++++++---------
> 1 file changed, 65 insertions(+), 19 deletions(-)
>
> diff --git a/drivers/input/keyboard/cap11xx.c b/drivers/input/keyboard/cap11xx.c
> index 378db10..13fae03 100644
> --- a/drivers/input/keyboard/cap11xx.c
> +++ b/drivers/input/keyboard/cap11xx.c
> @@ -14,6 +14,7 @@
> #include <linux/input.h>
> #include <linux/leds.h>
> #include <linux/of_irq.h>
> +#include <linux/pm_runtime.h>
> #include <linux/regmap.h>
> #include <linux/i2c.h>
> #include <linux/gpio/consumer.h>
> @@ -84,9 +85,9 @@ struct cap11xx_led {
> struct cap11xx_priv {
> struct regmap *regmap;
> struct input_dev *idev;
> + struct i2c_client *i2c_client;
>
> struct cap11xx_led *leds;
> - int num_leds;
>
> /* config */
> u32 keycodes[];
> @@ -207,12 +208,6 @@ out:
>
> static int cap11xx_set_sleep(struct cap11xx_priv *priv, bool sleep)
> {
> - /*
> - * DLSEEP mode will turn off all LEDS, prevent this
> - */
> - if (IS_ENABLED(CONFIG_LEDS_CLASS) && priv->num_leds)
> - return 0;
> -
> return regmap_update_bits(priv->regmap, CAP11XX_REG_MAIN_CONTROL,
> CAP11XX_REG_MAIN_CONTROL_DLSEEP,
> sleep ? CAP11XX_REG_MAIN_CONTROL_DLSEEP : 0);
> @@ -222,14 +217,16 @@ static int cap11xx_input_open(struct input_dev *idev)
> {
> struct cap11xx_priv *priv = input_get_drvdata(idev);
>
> - return cap11xx_set_sleep(priv, false);
> + pm_runtime_get(&priv->i2c_client->dev);
> +
> + return 0;
> }
>
> static void cap11xx_input_close(struct input_dev *idev)
> {
> struct cap11xx_priv *priv = input_get_drvdata(idev);
>
> - cap11xx_set_sleep(priv, true);
> + pm_runtime_put_autosuspend(&priv->i2c_client->dev);
> }
>
> #ifdef CONFIG_LEDS_CLASS
> @@ -251,10 +248,16 @@ static void cap11xx_led_set(struct led_classdev *cdev,
> enum led_brightness value)
> {
> struct cap11xx_led *led = container_of(cdev, struct cap11xx_led, cdev);
> + struct cap11xx_priv *priv = led->priv;
>
> if (led->new_brightness == value)
> return;
>
> + if (value)
> + pm_runtime_get(&priv->i2c_client->dev);
> + else
> + pm_runtime_put_autosuspend(&priv->i2c_client->dev);
> +
> led->new_brightness = value;
> schedule_work(&led->work);
> }
> @@ -315,8 +318,6 @@ static int cap11xx_init_leds(struct device *dev,
> error = devm_led_classdev_register(dev, &led->cdev);
> if (error)
> return error;
> -
> - priv->num_leds++;
> led++;
> }
>
> @@ -330,6 +331,16 @@ static int cap11xx_init_leds(struct device *dev,
> }
> #endif
>
> +static void cap11xx_shutdown_act(void *data)
> +{
> + struct i2c_client *i2c_client = data;
> + struct cap11xx_priv *priv = i2c_get_clientdata(i2c_client);
> +
> + cap11xx_set_sleep(priv, true);
> + pm_runtime_disable(&i2c_client->dev);
> + pm_runtime_set_suspended(&i2c_client->dev);
> +}
> +
> static int cap11xx_i2c_probe(struct i2c_client *i2c_client,
> const struct i2c_device_id *id)
> {
> @@ -363,6 +374,14 @@ static int cap11xx_i2c_probe(struct i2c_client *i2c_client,
> if (IS_ERR(priv->regmap))
> return PTR_ERR(priv->regmap);
>
> + error = devm_add_action(dev, cap11xx_shutdown_act, i2c_client);
> + if (error) {
> + dev_err(dev, "failed to register remove action\n");
> + return error;
> + }
> + i2c_set_clientdata(i2c_client, priv);
> + priv->i2c_client = i2c_client;
> +
> error = regmap_read(priv->regmap, CAP11XX_REG_PRODUCT_ID, &val);
> if (error)
> return error;
> @@ -388,7 +407,6 @@ static int cap11xx_i2c_probe(struct i2c_client *i2c_client,
> return error;
>
> dev_info(dev, "CAP11XX detected, revision 0x%02x\n", rev);
> - i2c_set_clientdata(i2c_client, priv);
> node = dev->of_node;
>
> if (!of_property_read_u32(node, "microchip,sensor-gain", &gain32)) {
> @@ -450,22 +468,28 @@ static int cap11xx_i2c_probe(struct i2c_client *i2c_client,
> priv->idev->open = cap11xx_input_open;
> priv->idev->close = cap11xx_input_close;
>
> - error = cap11xx_init_leds(dev, priv, cap->num_leds);
> + error = cap11xx_set_sleep(priv, true);
> if (error)
> return error;
>
> - input_set_drvdata(priv->idev, priv);
> + error = pm_runtime_set_active(dev);
> + if (error)
> + return error;
>
> - /*
> - * Put the device in deep sleep mode for now.
> - * ->open() will bring it back once the it is actually needed.
> - */
> - cap11xx_set_sleep(priv, true);
> + pm_runtime_enable(dev);
> + pm_runtime_set_autosuspend_delay(dev, 1000);
> + pm_runtime_use_autosuspend(dev);
> +
> + input_set_drvdata(priv->idev, priv);
>
> error = input_register_device(priv->idev);
> if (error)
> return error;
>
> + error = cap11xx_init_leds(dev, priv, cap->num_leds);
> + if (error)
> + return error;
> +
> irq = irq_of_parse_and_map(node, 0);
> if (!irq) {
> dev_err(dev, "Unable to parse or map IRQ\n");
> @@ -496,10 +520,32 @@ static const struct i2c_device_id cap11xx_i2c_ids[] = {
> };
> MODULE_DEVICE_TABLE(i2c, cap11xx_i2c_ids);
>
> +#ifdef CONFIG_PM
> +static int cap11xx_runtime_suspend(struct device *dev)
> +{
> + struct cap11xx_priv *priv = i2c_get_clientdata(to_i2c_client(dev));
> +
> + return cap11xx_set_sleep(priv, true);
> +}
> +
> +static int cap11xx_runtime_resume(struct device *dev)
> +{
> + struct cap11xx_priv *priv = i2c_get_clientdata(to_i2c_client(dev));
> +
> + return cap11xx_set_sleep(priv, false);
> +}
> +#endif
> +
> +static const struct dev_pm_ops cap11xx_pm_ops = {
> + SET_RUNTIME_PM_OPS(cap11xx_runtime_suspend,
> + cap11xx_runtime_resume, NULL)
> +};
> +
> static struct i2c_driver cap11xx_i2c_driver = {
> .driver = {
> .name = "cap11xx",
> .of_match_table = cap11xx_dt_ids,
> + .pm = &cap11xx_pm_ops,
> },
> .id_table = cap11xx_i2c_ids,
> .probe = cap11xx_i2c_probe,
> --
> 1.9.1
>
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH v4 1/1] cap11xx: add runtime PM support
2015-08-23 21:17 ` [PATCH v4 1/1] " Matt Ranostay
2015-09-17 6:39 ` Matt Ranostay
@ 2015-10-02 18:06 ` Dmitry Torokhov
1 sibling, 0 replies; 4+ messages in thread
From: Dmitry Torokhov @ 2015-10-02 18:06 UTC (permalink / raw)
To: Matt Ranostay; +Cc: linux-input
Hi Matt,
On Sun, Aug 23, 2015 at 02:17:57PM -0700, Matt Ranostay wrote:
> Puts device into DEEP SLEEP when no LEDs are in in the on-state, and no
> input_handlers are in use. Also uses *_autosuspend to prevent a LED
> trigger from constantly suspending and resuming device.
>
> Signed-off-by: Matt Ranostay <mranostay@gmail.com>
> ---
> drivers/input/keyboard/cap11xx.c | 84 +++++++++++++++++++++++++++++++---------
> 1 file changed, 65 insertions(+), 19 deletions(-)
>
> diff --git a/drivers/input/keyboard/cap11xx.c b/drivers/input/keyboard/cap11xx.c
> index 378db10..13fae03 100644
> --- a/drivers/input/keyboard/cap11xx.c
> +++ b/drivers/input/keyboard/cap11xx.c
> @@ -14,6 +14,7 @@
> #include <linux/input.h>
> #include <linux/leds.h>
> #include <linux/of_irq.h>
> +#include <linux/pm_runtime.h>
> #include <linux/regmap.h>
> #include <linux/i2c.h>
> #include <linux/gpio/consumer.h>
> @@ -84,9 +85,9 @@ struct cap11xx_led {
> struct cap11xx_priv {
> struct regmap *regmap;
> struct input_dev *idev;
> + struct i2c_client *i2c_client;
>
> struct cap11xx_led *leds;
> - int num_leds;
>
> /* config */
> u32 keycodes[];
> @@ -207,12 +208,6 @@ out:
>
> static int cap11xx_set_sleep(struct cap11xx_priv *priv, bool sleep)
> {
> - /*
> - * DLSEEP mode will turn off all LEDS, prevent this
> - */
> - if (IS_ENABLED(CONFIG_LEDS_CLASS) && priv->num_leds)
> - return 0;
> -
> return regmap_update_bits(priv->regmap, CAP11XX_REG_MAIN_CONTROL,
> CAP11XX_REG_MAIN_CONTROL_DLSEEP,
> sleep ? CAP11XX_REG_MAIN_CONTROL_DLSEEP : 0);
> @@ -222,14 +217,16 @@ static int cap11xx_input_open(struct input_dev *idev)
> {
> struct cap11xx_priv *priv = input_get_drvdata(idev);
>
> - return cap11xx_set_sleep(priv, false);
> + pm_runtime_get(&priv->i2c_client->dev);
> +
> + return 0;
> }
>
> static void cap11xx_input_close(struct input_dev *idev)
> {
> struct cap11xx_priv *priv = input_get_drvdata(idev);
>
> - cap11xx_set_sleep(priv, true);
> + pm_runtime_put_autosuspend(&priv->i2c_client->dev);
> }
>
> #ifdef CONFIG_LEDS_CLASS
> @@ -251,10 +248,16 @@ static void cap11xx_led_set(struct led_classdev *cdev,
> enum led_brightness value)
> {
> struct cap11xx_led *led = container_of(cdev, struct cap11xx_led, cdev);
> + struct cap11xx_priv *priv = led->priv;
>
> if (led->new_brightness == value)
> return;
>
> + if (value)
> + pm_runtime_get(&priv->i2c_client->dev);
> + else
> + pm_runtime_put_autosuspend(&priv->i2c_client->dev);
> +
IIRC cap11xx_led_set() can not sleep (that is why we use workqueue to
actually toggle the led) so I do not think that you can call
pm_runtime_get() here, as it is likely to involve sleep as well.
I also wonder if there is a race between PM state that you set here and
work item trying to access device state.
I'd look into moving PM handling into work function.
> led->new_brightness = value;
> schedule_work(&led->work);
> }
> @@ -315,8 +318,6 @@ static int cap11xx_init_leds(struct device *dev,
> error = devm_led_classdev_register(dev, &led->cdev);
> if (error)
> return error;
> -
> - priv->num_leds++;
> led++;
> }
>
> @@ -330,6 +331,16 @@ static int cap11xx_init_leds(struct device *dev,
> }
> #endif
>
> +static void cap11xx_shutdown_act(void *data)
> +{
> + struct i2c_client *i2c_client = data;
> + struct cap11xx_priv *priv = i2c_get_clientdata(i2c_client);
> +
> + cap11xx_set_sleep(priv, true);
> + pm_runtime_disable(&i2c_client->dev);
> + pm_runtime_set_suspended(&i2c_client->dev);
> +}
> +
> static int cap11xx_i2c_probe(struct i2c_client *i2c_client,
> const struct i2c_device_id *id)
> {
> @@ -363,6 +374,14 @@ static int cap11xx_i2c_probe(struct i2c_client *i2c_client,
> if (IS_ERR(priv->regmap))
> return PTR_ERR(priv->regmap);
>
> + error = devm_add_action(dev, cap11xx_shutdown_act, i2c_client);
> + if (error) {
> + dev_err(dev, "failed to register remove action\n");
> + return error;
> + }
> + i2c_set_clientdata(i2c_client, priv);
> + priv->i2c_client = i2c_client;
> +
> error = regmap_read(priv->regmap, CAP11XX_REG_PRODUCT_ID, &val);
> if (error)
> return error;
> @@ -388,7 +407,6 @@ static int cap11xx_i2c_probe(struct i2c_client *i2c_client,
> return error;
>
> dev_info(dev, "CAP11XX detected, revision 0x%02x\n", rev);
> - i2c_set_clientdata(i2c_client, priv);
> node = dev->of_node;
>
> if (!of_property_read_u32(node, "microchip,sensor-gain", &gain32)) {
> @@ -450,22 +468,28 @@ static int cap11xx_i2c_probe(struct i2c_client *i2c_client,
> priv->idev->open = cap11xx_input_open;
> priv->idev->close = cap11xx_input_close;
>
> - error = cap11xx_init_leds(dev, priv, cap->num_leds);
> + error = cap11xx_set_sleep(priv, true);
> if (error)
> return error;
>
> - input_set_drvdata(priv->idev, priv);
> + error = pm_runtime_set_active(dev);
I am confused - we put device to sleep above and then declare it active?
> + if (error)
> + return error;
>
> - /*
> - * Put the device in deep sleep mode for now.
> - * ->open() will bring it back once the it is actually needed.
> - */
> - cap11xx_set_sleep(priv, true);
> + pm_runtime_enable(dev);
> + pm_runtime_set_autosuspend_delay(dev, 1000);
> + pm_runtime_use_autosuspend(dev);
> +
> + input_set_drvdata(priv->idev, priv);
>
> error = input_register_device(priv->idev);
> if (error)
> return error;
>
> + error = cap11xx_init_leds(dev, priv, cap->num_leds);
> + if (error)
> + return error;
> +
> irq = irq_of_parse_and_map(node, 0);
> if (!irq) {
> dev_err(dev, "Unable to parse or map IRQ\n");
> @@ -496,10 +520,32 @@ static const struct i2c_device_id cap11xx_i2c_ids[] = {
> };
> MODULE_DEVICE_TABLE(i2c, cap11xx_i2c_ids);
>
> +#ifdef CONFIG_PM
> +static int cap11xx_runtime_suspend(struct device *dev)
__maybe_unused instead of #ifdef please.
> +{
> + struct cap11xx_priv *priv = i2c_get_clientdata(to_i2c_client(dev));
> +
> + return cap11xx_set_sleep(priv, true);
> +}
> +
> +static int cap11xx_runtime_resume(struct device *dev)
> +{
> + struct cap11xx_priv *priv = i2c_get_clientdata(to_i2c_client(dev));
> +
> + return cap11xx_set_sleep(priv, false);
> +}
> +#endif
> +
> +static const struct dev_pm_ops cap11xx_pm_ops = {
> + SET_RUNTIME_PM_OPS(cap11xx_runtime_suspend,
> + cap11xx_runtime_resume, NULL)
> +};
> +
> static struct i2c_driver cap11xx_i2c_driver = {
> .driver = {
> .name = "cap11xx",
> .of_match_table = cap11xx_dt_ids,
> + .pm = &cap11xx_pm_ops,
> },
> .id_table = cap11xx_i2c_ids,
> .probe = cap11xx_i2c_probe,
> --
> 1.9.1
>
Thanks.
--
Dmitry
^ permalink raw reply [flat|nested] 4+ messages in thread