Re: [PATCH v4 1/6] hid: new driver for PicoLCD device

Bruno Prémont
2010-03-29T10:16:16Z

On Mon, 29 Mar 2010 11:47:27 +0200 Jiri Kosina <jkosina@suse.cz> wrote:
> On Sat, 27 Mar 2010, Bruno Prémont wrote:
>
> > +#ifdef CONFIG_PM
> > +static int picolcd_suspend(struct hid_device *hdev)
> > +{
> > + dbg_hid(PICOLCD_NAME " device ready for suspend\n");
> > + return 0;
> > +}
> > +
> > +static int picolcd_resume(struct hid_device *hdev)
> > +{
> > + 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);
> > + return 0;
> > +}
> > +#endif
> [ ... ]
> > +static struct hid_driver picolcd_driver = {
> > + .name = "hid-picolcd",
> > + .id_table = picolcd_devices,
> > + .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
> > +};
>
> struct hid_driver doesn't provide power-management related callbacks.
> So I guess that you have either not tested this feature at all, or
> you have some extra patch somewhere which adds such callbacks to HID
> core, but you haven't sent it out for review?
>
> In any cases, this will very likely cause compilation failure with
> CONFIG_PM turned on.

That's the patch I referred to (under series introduction mail) when
saying:
> The series depends on my previous patch adding HID suspend support
> (I've not yet looked at improving it).

Link to the patch:
http://lkml.org/lkml/2010/2/24/233

Especially the reset-resume part is important as the device has to be
reprogrammed with framebuffer content, brightness/contrast in that case.

As stated, Oliver didn't like the implementation of the addition of
those hooks too much and I have yet to look at improving it.

Thanks,
Bruno