On Thu, Apr 8, 2010 at 12:50 AM, Yong Wang <yong.y.wang@xxxxxxxxxxxxxxx> wrote: > On Wed, Apr 07, 2010 at 05:05:41PM +0200, Corentin Chary wrote: >> On Wed, Apr 7, 2010 at 4:17 PM, Yong Wang <yong.y.wang@xxxxxxxxxxxxxxx> wrote: >> > Add backlight support for WMI based Eee PC laptops. In addition, start >> > to use a platform device to manage all functional devices as more >> > features will be implemented later. >> > >> > +static struct platform_device *eeepc_wmi_platform_device; >> > ?static struct input_dev *eeepc_wmi_input_dev; >> > +struct backlight_device *eeepc_wmi_backlight_device; >> >> Instead of using static variables, you should really use a struct >> eeepc_wmi, store stuff inside it, >> and make all these functions reentrant. Alan did it for eeepc-laptop, >> and I did it for asus-laptop, >> see http://git.iksaif.net/?p=acpi4asus.git;a=commit;h=854c78363f37f03e30e2856ef17d7eefc62e0d06 >> . >> The driver would really be cleaner and easier to review with that. And >> it would be more coherent with >> eeepc-laptop's code. >> > > Thanks for your review, Corentin. Could you please explain a bit more > what you mean by "make all these functions reentrant"? > > Thanks > -Yong > > Well, it means that you should'nt use a static variable for platform, backlight, input and only use function parameters to get the job done. In your case, it's not really mandatory, but as Alan explain in the changelog I linked, it's really cleaner. And also make the code a lot easier to review. Also, if you do that, could you split up the patch in (for example): - add a eeepc_wmi context structure, and put input_device into it - add a platform device - add backlight support Thanks ! -- Corentin Chary http://xf.iksaif.net -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
