Hello Philippe,

On Fri, Jul 08, 2011 at 04:45:00PM +0200, Philippe Rétornaz wrote:

quoted

quoted

+config INPUT_MC13783_ON
+	tristate "MC13783 ON buttons"
+	depends on MFD_MC13783
+	help
+	  Support the ON buttons of MC13783 PMIC as an input device
+	  reporting power button status.
+
+	  To compile this driver as a module, choose M here: the module
+	  will be called mc13783-pwrbutton.

Why not use INPUT_MC13783_PWRBUTTON to match the source file (and so
module) name.

Ok.

quoted

quoted

+
+

one empty line should be enough

Ok.

quoted

quoted

diff --git a/drivers/input/misc/mc13783-pwrbutton.c

b/drivers/input/misc/mc13783-pwrbutton.c new file mode 100644
index 0000000..6aee01c

--- /dev/null
+++ b/drivers/input/misc/mc13783-pwrbutton.c

@@ -0,0 +1,288 @@
+/**
+ * mc13783-pwrbutton.c - MC13783 Power Button Input Driver
+ *
+ * Copyright (C) 2011 Philippe Rétornaz
+ *
+ * Based on twl4030-pwrbutton driver by:
+ *     Peter De Schrijver <peter.de-schrijver@nokia.com>
+ *     Felipe Balbi <felipe.balbi@nokia.com>
+ *
+ * This file is subject to the terms and conditions of the GNU General
+ * Public License. See the file "COPYING" in the main directory of this
+ * archive for more details.

This implies GPL v2 only? So you either need to clearify here or you
need to adapt the MODULE_LICENSE line below to read

	MODULE_LICENSE("GPL v2");

Well, I took exactly the same license than the twl4030-pwrbutton. 
This driver is: MODULE_LICENSE("GPL"), why should I change it ?

So this driver is wrong, too. According to include/linux/module.h, "GPL"
means "GNU Public License v2 or later" and "GPL v2" means "GNU Public
License v2".

quoted

quoted

--- a/drivers/mfd/mc13xxx-core.c
+++ b/drivers/mfd/mc13xxx-core.c

@@ -778,6 +778,10 @@ err_revision:
 		mc13xxx_add_subdevice_pdata(mc13xxx, "%s-led",
 		
 				pdata->leds, sizeof(*pdata->leds));

+	if (pdata->flags & MC13XXX_USE_BUTTON)

Why not use

	if (pdata->buttons)

to detect if the pwrbutton device should be registered?

To keep consistancy as the others sub-device are registered the same way 
(regulator, led). I can do it as you propose for the button but IMHO it's a 
bit strange to have a MC13XXX_USE_* for all the subdevice except the buttons. 
So should I change ?

It would be a third patch for your commit count to convert regulator and
led, too ;-)

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
--
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