[PATCH 1/2] hid: add suspend/resume hooks for hid drivers

[PATCH 1/2] hid: add suspend/resume hooks for hid drivers

[Bruno Prémont]

Add suspend/resume hooks for HID drivers so these can do some
additional state adjustment when device gets suspended/resumed.

v3:
  - Pass full message to hid driver's suspend hook
v2:
  - Adds auto_suspend parameter to suspend hook
  - Only calls HID driver's resume hooks when previous code
    did succeed

Signed-off-by: Bruno Prémont <bonbons@linux-vserver.org>
---

Note, I've not added PM parts to bluetooth as I'm not sure how/where
to catch the PM events generated by bluetooth.
It looks like PM on bluetooth side does just some bluetooth housekeeping
and sends an event.
Marcel, do you have some pointers for wiring up these HID suspend hooks
in the bluetooth/hidp code?

 drivers/hid/usbhid/hid-core.c |   24 +++++++++++++++++++++++-
 include/linux/hid.h           |    8 ++++++++
 2 files changed, 31 insertions(+), 1 deletions(-)

diff --git a/drivers/hid/usbhid/hid-core.c b/drivers/hid/usbhid/hid-core.c
index 3e7909b..17eb3ec 100644
--- a/drivers/hid/usbhid/hid-core.c
+++ b/drivers/hid/usbhid/hid-core.c
@@ -1291,6 +1291,11 @@ static int hid_suspend(struct usb_interface *intf, pm_message_t message)
 		{
 			set_bit(HID_REPORTED_IDLE, &usbhid->iofl);
 			spin_unlock_irq(&usbhid->lock);
+			if (hid->driver && hid->driver->suspend) {
+				status = hid->driver->suspend(hid, message);
+				if (status < 0)
+					return status;
+			}
 		} else {
 			usbhid_mark_busy(usbhid);
 			spin_unlock_irq(&usbhid->lock);
@@ -1298,6 +1303,11 @@ static int hid_suspend(struct usb_interface *intf, pm_message_t message)
 		}
 
 	} else {
+		if (hid->driver && hid->driver->suspend) {
+			status = hid->driver->suspend(hid, message);
+			if (status < 0)
+				return status;
+		}
 		spin_lock_irq(&usbhid->lock);
 		set_bit(HID_REPORTED_IDLE, &usbhid->iofl);
 		spin_unlock_irq(&usbhid->lock);
@@ -1352,6 +1362,11 @@ static int hid_resume(struct usb_interface *intf)
 		hid_io_error(hid);
 	usbhid_restart_queues(usbhid);
 
+	if (status >= 0 && hid->driver && hid->driver->resume) {
+		int ret = hid->driver->resume(hid);
+		if (ret < 0)
+			status = ret;
+	}
 	dev_dbg(&intf->dev, "resume status %d\n", status);
 	return 0;
 }
@@ -1360,9 +1375,16 @@ static int hid_reset_resume(struct usb_interface *intf)
 {
 	struct hid_device *hid = usb_get_intfdata(intf);
 	struct usbhid_device *usbhid = hid->driver_data;
+	int status;
 
 	clear_bit(HID_REPORTED_IDLE, &usbhid->iofl);
-	return hid_post_reset(intf);
+	status = hid_post_reset(intf);
+	if (status >= 0 && hid->driver && hid->driver->reset_resume) {
+		int ret = hid->driver->reset_resume(hid);
+		if (ret < 0)
+			status = ret;
+	}
+	return status;
 }
 
 #endif /* CONFIG_PM */
diff --git a/include/linux/hid.h b/include/linux/hid.h
index b1344ec..069e587 100644
--- a/include/linux/hid.h
+++ b/include/linux/hid.h
@@ -589,6 +589,9 @@ struct hid_usage_id {
  * @report_fixup: called before report descriptor parsing (NULL means nop)
  * @input_mapping: invoked on input registering before mapping an usage
  * @input_mapped: invoked on input registering after mapping an usage
+ * @suspend: invoked on suspend (NULL means nop)
+ * @resume: invoked on resume if device was not reset (NULL means nop)
+ * @reset_resume: invoked on resume if device was reset (NULL means nop)
  *
  * raw_event and event should return 0 on no action performed, 1 when no
  * further processing should be done and negative on error
@@ -629,6 +632,11 @@ struct hid_driver {
 	int (*input_mapped)(struct hid_device *hdev,
 			struct hid_input *hidinput, struct hid_field *field,
 			struct hid_usage *usage, unsigned long **bit, int *max);
+#ifdef CONFIG_PM
+	int (*suspend)(struct hid_device *hdev, pm_message_t message);
+	int (*resume)(struct hid_device *hdev);
+	int (*reset_resume)(struct hid_device *hdev);
+#endif
 /* private: */
 	struct device_driver driver;
 };
-- 
1.6.4.4

[PATCH 2/2] hid: add PM support to PicoLCD device

[Bruno Prémont]

Add PM support in order to turn off backlight on suspend, restore
it on resume and especially restore complete state on reset-resume.

Signed-off-by: Bruno Prémont <bonbons@linux-vserver.org>
---
 drivers/hid/hid-picolcd.c |   61 +++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 61 insertions(+), 0 deletions(-)

diff --git a/drivers/hid/hid-picolcd.c b/drivers/hid/hid-picolcd.c
index aa6f2e1..c652390 100644
--- a/drivers/hid/hid-picolcd.c
+++ b/drivers/hid/hid-picolcd.c
@@ -852,6 +852,20 @@ static inline int picolcd_resume_backlight(struct picolcd_data *data)
 	return picolcd_set_brightness(data->backlight);
 }
 
+#ifdef CONFIG_PM
+static void picolcd_suspend_backlight(struct picolcd_data *data)
+{
+	int bl_power = data->lcd_power;
+	if (!data->backlight)
+		return;
+
+	data->backlight->props.power = FB_BLANK_POWERDOWN;
+	picolcd_set_brightness(data->backlight);
+	data->lcd_power = data->backlight->props.power = bl_power;
+}
+#else
+#define picolcd_suspend_backlight(a)
+#endif /* CONFIG_PM */
 #else
 static inline int picolcd_init_backlight(struct picolcd_data *data,
 		struct hid_report *report)
@@ -865,6 +879,7 @@ static inline int picolcd_resume_backlight(struct picolcd_data *data)
 {
 	return 0;
 }
+#define picolcd_suspend_backlight(a)
 #endif /* CONFIG_HID_PICOLCD_BACKLIGHT */
 
 #ifdef CONFIG_HID_PICOLCD_LCD
@@ -2259,6 +2274,46 @@ static int picolcd_raw_event(struct hid_device *hdev,
 	return 1;
 }
 
+#ifdef CONFIG_PM
+static int picolcd_suspend(struct hid_device *hdev, pm_message_t message)
+{
+	if (message.event & PM_EVENT_AUTO)
+		return 0;
+
+	picolcd_suspend_backlight(hid_get_drvdata(hdev));
+	dbg_hid(PICOLCD_NAME " device ready for suspend\n");
+	return 0;
+}
+
+static int picolcd_resume(struct hid_device *hdev)
+{
+	int ret;
+	ret = picolcd_resume_backlight(hid_get_drvdata(hdev));
+	if (ret)
+		dbg_hid(PICOLCD_NAME " restoring backlight failed: %d\n", ret);
+	return 0;
+}
+
+static int picolcd_reset_resume(struct hid_device *hdev)
+{
+	int ret;
+	ret = picolcd_reset(hdev);
+	if (ret)
+		dbg_hid(PICOLCD_NAME " resetting our device failed: %d\n", ret);
+	ret = picolcd_fb_reset(hid_get_drvdata(hdev), 0);
+	if (ret)
+		dbg_hid(PICOLCD_NAME " restoring framebuffer content failed: %d\n", ret);
+	ret = picolcd_resume_lcd(hid_get_drvdata(hdev));
+	if (ret)
+		dbg_hid(PICOLCD_NAME " restoring lcd failed: %d\n", ret);
+	ret = picolcd_resume_backlight(hid_get_drvdata(hdev));
+	if (ret)
+		dbg_hid(PICOLCD_NAME " restoring backlight failed: %d\n", ret);
+	picolcd_leds_set(hid_get_drvdata(hdev));
+	return 0;
+}
+#endif
+
 /* initialize keypad input device */
 static int picolcd_init_keys(struct picolcd_data *data,
 		struct hid_report *report)
@@ -2544,6 +2599,11 @@ static struct hid_driver picolcd_driver = {
 	.probe =         picolcd_probe,
 	.remove =        picolcd_remove,
 	.raw_event =     picolcd_raw_event,
+#ifdef CONFIG_PM
+	.suspend =       picolcd_suspend,
+	.resume =        picolcd_resume,
+	.reset_resume =  picolcd_reset_resume,
+#endif
 };
 
 static int __init picolcd_init(void)
-- 
1.6.4.4

Re: [PATCH 1/2] hid: add suspend/resume hooks for hid drivers

[Jiri Kosina]

On Sun, 25 Apr 2010, Bruno Prémont wrote:

> Add suspend/resume hooks for HID drivers so these can do some
> additional state adjustment when device gets suspended/resumed.
> 
> v3:
>   - Pass full message to hid driver's suspend hook
> v2:
>   - Adds auto_suspend parameter to suspend hook
>   - Only calls HID driver's resume hooks when previous code
>     did succeed
> 

I have applied this to 'hid-suspend' branch of my tree for now. Thanks,

-- 
Jiri Kosina
SUSE Labs, Novell Inc.
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/2] hid: add PM support to PicoLCD device

[Jiri Kosina]

On Sun, 25 Apr 2010, Bruno Prémont wrote:

> Add PM support in order to turn off backlight on suspend, restore
> it on resume and especially restore complete state on reset-resume.
> 
> Signed-off-by: Bruno Prémont <bonbons@linux-vserver.org>
> ---
>  drivers/hid/hid-picolcd.c |   61 +++++++++++++++++++++++++++++++++++++++++++++
>  1 files changed, 61 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/hid/hid-picolcd.c b/drivers/hid/hid-picolcd.c
> index aa6f2e1..c652390 100644
> --- a/drivers/hid/hid-picolcd.c
> +++ b/drivers/hid/hid-picolcd.c
> @@ -852,6 +852,20 @@ static inline int picolcd_resume_backlight(struct picolcd_data *data)
>  	return picolcd_set_brightness(data->backlight);
>  }
>  
> +#ifdef CONFIG_PM
> +static void picolcd_suspend_backlight(struct picolcd_data *data)
> +{
> +	int bl_power = data->lcd_power;
> +	if (!data->backlight)
> +		return;
> +
> +	data->backlight->props.power = FB_BLANK_POWERDOWN;
> +	picolcd_set_brightness(data->backlight);
> +	data->lcd_power = data->backlight->props.power = bl_power;
> +}
> +#else
> +#define picolcd_suspend_backlight(a)
> +#endif /* CONFIG_PM */

Stylistic thing -- it would be nice if this was actually 

static inline void picolcd_suspend_backlight(struct picolcd_data *data)
{
	return 0;
}

But why do you need to have it defined in !CONFIG_PM situation anyway? 
It's not used otherwise at all.

>  #else
>  static inline int picolcd_init_backlight(struct picolcd_data *data,
>  		struct hid_report *report)
> @@ -865,6 +879,7 @@ static inline int picolcd_resume_backlight(struct picolcd_data *data)
>  {
>  	return 0;
>  }
> +#define picolcd_suspend_backlight(a)

Any why is it defined once more here?

Thanks,

-- 
Jiri Kosina
SUSE Labs, Novell Inc.

Re: [PATCH 2/2] hid: add PM support to PicoLCD device

[Bruno Prémont]

On Tue, 27 April 2010 Jiri Kosina <jkosina@suse.cz> wrote:
> On Sun, 25 Apr 2010, Bruno Prémont wrote:
> 
> > Add PM support in order to turn off backlight on suspend, restore
> > it on resume and especially restore complete state on reset-resume.
> > 
> > Signed-off-by: Bruno Prémont <bonbons@linux-vserver.org>
> > ---
> >  drivers/hid/hid-picolcd.c |   61 +++++++++++++++++++++++++++++++++++++++++++++
> >  1 files changed, 61 insertions(+), 0 deletions(-)
> > 
> > diff --git a/drivers/hid/hid-picolcd.c b/drivers/hid/hid-picolcd.c
> > index aa6f2e1..c652390 100644
> > --- a/drivers/hid/hid-picolcd.c
> > +++ b/drivers/hid/hid-picolcd.c
> > @@ -852,6 +852,20 @@ static inline int picolcd_resume_backlight(struct picolcd_data *data)
> >  	return picolcd_set_brightness(data->backlight);
> >  }
> >  
> > +#ifdef CONFIG_PM
> > +static void picolcd_suspend_backlight(struct picolcd_data *data)
> > +{
> > +	int bl_power = data->lcd_power;
> > +	if (!data->backlight)
> > +		return;
> > +
> > +	data->backlight->props.power = FB_BLANK_POWERDOWN;
> > +	picolcd_set_brightness(data->backlight);
> > +	data->lcd_power = data->backlight->props.power = bl_power;
> > +}
> > +#else
> > +#define picolcd_suspend_backlight(a)
> > +#endif /* CONFIG_PM */
> 
> Stylistic thing -- it would be nice if this was actually 
> 
> static inline void picolcd_suspend_backlight(struct picolcd_data *data)
> {
> 	return 0;
> }
> 
> But why do you need to have it defined in !CONFIG_PM situation anyway? 
> It's not used otherwise at all.

Yeah, I could drop the #else branch (of #ifconfig CONFIG_PM).

Is gcc quiet about defined but unused static inline functions?
It's in order to avoid those warnings that I used #define bla().
(same in other occasions of stubs returning void.

Might be I forgot the inline keyword for the case I got the warning
about defined but unused function. Will check tomorrow.

> >  #else
> >  static inline int picolcd_init_backlight(struct picolcd_data *data,
> >  		struct hid_report *report)
> > @@ -865,6 +879,7 @@ static inline int picolcd_resume_backlight(struct picolcd_data *data)
> >  {
> >  	return 0;
> >  }
> > +#define picolcd_suspend_backlight(a)
> 
> Any why is it defined once more here?

Thats in the case where CONFIG_HID_PICOLCD_BACKLIGHT is not defined.
c.f. the #else at the beginning of this quote.


Thanks,
Bruno
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/2] hid: add PM support to PicoLCD device

[Jiri Kosina]

On Tue, 27 Apr 2010, Bruno Prémont wrote:

> > > +#ifdef CONFIG_PM
> > > +static void picolcd_suspend_backlight(struct picolcd_data *data)
> > > +{
> > > +	int bl_power = data->lcd_power;
> > > +	if (!data->backlight)
> > > +		return;
> > > +
> > > +	data->backlight->props.power = FB_BLANK_POWERDOWN;
> > > +	picolcd_set_brightness(data->backlight);
> > > +	data->lcd_power = data->backlight->props.power = bl_power;
> > > +}
> > > +#else
> > > +#define picolcd_suspend_backlight(a)
> > > +#endif /* CONFIG_PM */
> > 
> > Stylistic thing -- it would be nice if this was actually 
> > 
> > static inline void picolcd_suspend_backlight(struct picolcd_data *data)
> > {
> > 	return 0;
> > }
> > 
> > But why do you need to have it defined in !CONFIG_PM situation anyway? 
> > It's not used otherwise at all.
> 
> Yeah, I could drop the #else branch (of #ifconfig CONFIG_PM).

Yup, please do so.

> Is gcc quiet about defined but unused static inline functions?

Yup, -Wunused-function doesn't warn about unused static inline function.

-- 
Jiri Kosina
SUSE Labs, Novell Inc.
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/2] hid: add PM support to PicoLCD device

[Bruno Prémont]

On Tue, 27 April 2010 Jiri Kosina <jkosina@suse.cz> wrote:
> > Yeah, I could drop the #else branch (of #ifconfig CONFIG_PM).
> 
> Yup, please do so.
> 
> > Is gcc quiet about defined but unused static inline functions?
> 
> Yup, -Wunused-function doesn't warn about unused static inline function.

Ok, below is an updated patch, which also adds missing 'inline' keyword
for a few stubs and replaces #define f(a,b) to nothing with equivalent
stubs.
Is it ok to have all of this in a single patch or is it better to split
it into two (one for PM and one for cleaning up)?

Thanks,
Bruno


---
Add PM support in order to turn off backlight on suspend, restore
it on resume and especially restore complete state on reset-resume.

v2:
  Don't define stub to nothing but use static inline stubs
  Don't define picolcd_suspend_backlight() at all in !CONFIG_PM case

Signed-off-by: Bruno Prémont <bonbons@linux-vserver.org>
---
 drivers/hid/hid-picolcd.c |   79 ++++++++++++++++++++++++++++++++++++++++++---
 1 files changed, 74 insertions(+), 5 deletions(-)

diff --git a/drivers/hid/hid-picolcd.c b/drivers/hid/hid-picolcd.c
index aa6f2e1..95253b3 100644
--- a/drivers/hid/hid-picolcd.c
+++ b/drivers/hid/hid-picolcd.c
@@ -763,7 +763,7 @@ static inline int picolcd_init_framebuffer(struct picolcd_data *data)
 {
 	return 0;
 }
-static void picolcd_exit_framebuffer(struct picolcd_data *data)
+static inline void picolcd_exit_framebuffer(struct picolcd_data *data)
 {
 }
 #define picolcd_fbinfo(d) NULL
@@ -852,6 +852,18 @@ static inline int picolcd_resume_backlight(struct picolcd_data *data)
 	return picolcd_set_brightness(data->backlight);
 }
 
+#ifdef CONFIG_PM
+static void picolcd_suspend_backlight(struct picolcd_data *data)
+{
+	int bl_power = data->lcd_power;
+	if (!data->backlight)
+		return;
+
+	data->backlight->props.power = FB_BLANK_POWERDOWN;
+	picolcd_set_brightness(data->backlight);
+	data->lcd_power = data->backlight->props.power = bl_power;
+}
+#endif /* CONFIG_PM */
 #else
 static inline int picolcd_init_backlight(struct picolcd_data *data,
 		struct hid_report *report)
@@ -865,6 +877,9 @@ static inline int picolcd_resume_backlight(struct picolcd_data *data)
 {
 	return 0;
 }
+static inline void picolcd_suspend_backlight(struct picolcd_data *data)
+{
+}
 #endif /* CONFIG_HID_PICOLCD_BACKLIGHT */
 
 #ifdef CONFIG_HID_PICOLCD_LCD
@@ -1098,7 +1113,7 @@ static inline int picolcd_init_leds(struct picolcd_data *data,
 {
 	return 0;
 }
-static void picolcd_exit_leds(struct picolcd_data *data)
+static inline void picolcd_exit_leds(struct picolcd_data *data)
 {
 }
 static inline int picolcd_leds_set(struct picolcd_data *data)
@@ -2214,9 +2229,18 @@ static void picolcd_exit_devfs(struct picolcd_data *data)
 	mutex_destroy(&data->mutex_flash);
 }
 #else
-#define picolcd_debug_raw_event(data, hdev, report, raw_data, size)
-#define picolcd_init_devfs(data, eeprom_r, eeprom_w, flash_r, flash_w, reset)
-static void picolcd_exit_devfs(struct picolcd_data *data)
+static inline void picolcd_debug_raw_event(struct picolcd_data *data,
+		struct hid_device *hdev, struct hid_report *report,
+		u8 *raw_data, int size)
+{
+}
+static inline void picolcd_init_devfs(struct picolcd_data *data,
+		struct hid_report *eeprom_r, struct hid_report *eeprom_w,
+		struct hid_report *flash_r, struct hid_report *flash_w,
+		struct hid_report *reset)
+{
+}
+static inline void picolcd_exit_devfs(struct picolcd_data *data)
 {
 }
 #endif /* CONFIG_DEBUG_FS */
@@ -2259,6 +2283,46 @@ static int picolcd_raw_event(struct hid_device *hdev,
 	return 1;
 }
 
+#ifdef CONFIG_PM
+static int picolcd_suspend(struct hid_device *hdev, pm_message_t message)
+{
+	if (message.event & PM_EVENT_AUTO)
+		return 0;
+
+	picolcd_suspend_backlight(hid_get_drvdata(hdev));
+	dbg_hid(PICOLCD_NAME " device ready for suspend\n");
+	return 0;
+}
+
+static int picolcd_resume(struct hid_device *hdev)
+{
+	int ret;
+	ret = picolcd_resume_backlight(hid_get_drvdata(hdev));
+	if (ret)
+		dbg_hid(PICOLCD_NAME " restoring backlight failed: %d\n", ret);
+	return 0;
+}
+
+static int picolcd_reset_resume(struct hid_device *hdev)
+{
+	int ret;
+	ret = picolcd_reset(hdev);
+	if (ret)
+		dbg_hid(PICOLCD_NAME " resetting our device failed: %d\n", ret);
+	ret = picolcd_fb_reset(hid_get_drvdata(hdev), 0);
+	if (ret)
+		dbg_hid(PICOLCD_NAME " restoring framebuffer content failed: %d\n", ret);
+	ret = picolcd_resume_lcd(hid_get_drvdata(hdev));
+	if (ret)
+		dbg_hid(PICOLCD_NAME " restoring lcd failed: %d\n", ret);
+	ret = picolcd_resume_backlight(hid_get_drvdata(hdev));
+	if (ret)
+		dbg_hid(PICOLCD_NAME " restoring backlight failed: %d\n", ret);
+	picolcd_leds_set(hid_get_drvdata(hdev));
+	return 0;
+}
+#endif
+
 /* initialize keypad input device */
 static int picolcd_init_keys(struct picolcd_data *data,
 		struct hid_report *report)
@@ -2544,6 +2608,11 @@ static struct hid_driver picolcd_driver = {
 	.probe =         picolcd_probe,
 	.remove =        picolcd_remove,
 	.raw_event =     picolcd_raw_event,
+#ifdef CONFIG_PM
+	.suspend =       picolcd_suspend,
+	.resume =        picolcd_resume,
+	.reset_resume =  picolcd_reset_resume,
+#endif
 };
 
 static int __init picolcd_init(void)
-- 
1.6.4.4

--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/2] hid: add PM support to PicoLCD device

[Jiri Kosina]

On Sun, 2 May 2010, Bruno Prémont wrote:

> Ok, below is an updated patch, which also adds missing 'inline' keyword
> for a few stubs and replaces #define f(a,b) to nothing with equivalent
> stubs.

Looks good to me, I have applied the patch. Thanks Bruno.

-- 
Jiri Kosina
SUSE Labs, Novell Inc.
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html