[PATCH 0/2] add device tree probe support for gpio_keys

[PATCH 0/2] add device tree probe support for gpio_keys

[Shawn Guo]

The first patch gets the driver stop referencing platform_data after
.probe exits.  And the second patch adds the actual device tree probe
support for the driver.

Shawn Guo (2):
      Input: gpio-keys: do not reference platform_data after .probe exits
      Input: gpio-keys: add device tree probe support

 Documentation/devicetree/bindings/gpio/key.txt |   57 +++++++++++
 drivers/input/keyboard/gpio_keys.c             |  128 +++++++++++++++++++-----
 2 files changed, 161 insertions(+), 24 deletions(-)

[PATCH 1/2] Input: gpio-keys: do not reference platform_data after .probe exits

[Shawn Guo]

The patch makes a copy of platform data into driver data, so that any
reference to platform_data after .probe exits can be avoided.

Signed-off-by: Shawn Guo <shawn.guo@linaro.org>
Cc: Phil Blundell <pb@handhelds.org>
Cc: Dmitry Torokhov <dtor@mail.ru>
---
 drivers/input/keyboard/gpio_keys.c |   45 +++++++++++++++++------------------
 1 files changed, 22 insertions(+), 23 deletions(-)

diff --git a/drivers/input/keyboard/gpio_keys.c b/drivers/input/keyboard/gpio_keys.c
index 97bada4..85b5685 100644
--- a/drivers/input/keyboard/gpio_keys.c
+++ b/drivers/input/keyboard/gpio_keys.c
@@ -27,7 +27,7 @@
 #include <linux/gpio.h>
 
 struct gpio_button_data {
-	struct gpio_keys_button *button;
+	struct gpio_keys_button button;
 	struct input_dev *input;
 	struct timer_list timer;
 	struct work_struct work;
@@ -111,7 +111,7 @@ static void gpio_keys_disable_button(struct gpio_button_data *bdata)
 		/*
 		 * Disable IRQ and possible debouncing timer.
 		 */
-		disable_irq(gpio_to_irq(bdata->button->gpio));
+		disable_irq(gpio_to_irq(bdata->button.gpio));
 		if (bdata->timer_debounce)
 			del_timer_sync(&bdata->timer);
 
@@ -132,7 +132,7 @@ static void gpio_keys_disable_button(struct gpio_button_data *bdata)
 static void gpio_keys_enable_button(struct gpio_button_data *bdata)
 {
 	if (bdata->disabled) {
-		enable_irq(gpio_to_irq(bdata->button->gpio));
+		enable_irq(gpio_to_irq(bdata->button.gpio));
 		bdata->disabled = false;
 	}
 }
@@ -167,13 +167,13 @@ static ssize_t gpio_keys_attr_show_helper(struct gpio_keys_drvdata *ddata,
 	for (i = 0; i < ddata->n_buttons; i++) {
 		struct gpio_button_data *bdata = &ddata->data[i];
 
-		if (bdata->button->type != type)
+		if (bdata->button.type != type)
 			continue;
 
 		if (only_disabled && !bdata->disabled)
 			continue;
 
-		__set_bit(bdata->button->code, bits);
+		__set_bit(bdata->button.code, bits);
 	}
 
 	ret = bitmap_scnlistprintf(buf, PAGE_SIZE - 2, bits, n_events);
@@ -215,11 +215,11 @@ static ssize_t gpio_keys_attr_store_helper(struct gpio_keys_drvdata *ddata,
 	for (i = 0; i < ddata->n_buttons; i++) {
 		struct gpio_button_data *bdata = &ddata->data[i];
 
-		if (bdata->button->type != type)
+		if (bdata->button.type != type)
 			continue;
 
-		if (test_bit(bdata->button->code, bits) &&
-		    !bdata->button->can_disable) {
+		if (test_bit(bdata->button.code, bits) &&
+		    !bdata->button.can_disable) {
 			error = -EINVAL;
 			goto out;
 		}
@@ -230,10 +230,10 @@ static ssize_t gpio_keys_attr_store_helper(struct gpio_keys_drvdata *ddata,
 	for (i = 0; i < ddata->n_buttons; i++) {
 		struct gpio_button_data *bdata = &ddata->data[i];
 
-		if (bdata->button->type != type)
+		if (bdata->button.type != type)
 			continue;
 
-		if (test_bit(bdata->button->code, bits))
+		if (test_bit(bdata->button.code, bits))
 			gpio_keys_disable_button(bdata);
 		else
 			gpio_keys_enable_button(bdata);
@@ -319,7 +319,7 @@ static struct attribute_group gpio_keys_attr_group = {
 
 static void gpio_keys_report_event(struct gpio_button_data *bdata)
 {
-	struct gpio_keys_button *button = bdata->button;
+	struct gpio_keys_button *button = &bdata->button;
 	struct input_dev *input = bdata->input;
 	unsigned int type = button->type ?: EV_KEY;
 	int state = (gpio_get_value_cansleep(button->gpio) ? 1 : 0) ^ button->active_low;
@@ -351,7 +351,7 @@ static void gpio_keys_timer(unsigned long _data)
 static irqreturn_t gpio_keys_isr(int irq, void *dev_id)
 {
 	struct gpio_button_data *bdata = dev_id;
-	struct gpio_keys_button *button = bdata->button;
+	struct gpio_keys_button *button = &bdata->button;
 
 	BUG_ON(irq != gpio_to_irq(button->gpio));
 
@@ -494,7 +494,7 @@ static int __devinit gpio_keys_probe(struct platform_device *pdev)
 		unsigned int type = button->type ?: EV_KEY;
 
 		bdata->input = input;
-		bdata->button = button;
+		bdata->button = *button;
 
 		error = gpio_keys_setup_key(pdev, bdata, button);
 		if (error)
@@ -550,7 +550,6 @@ static int __devinit gpio_keys_probe(struct platform_device *pdev)
 
 static int __devexit gpio_keys_remove(struct platform_device *pdev)
 {
-	struct gpio_keys_platform_data *pdata = pdev->dev.platform_data;
 	struct gpio_keys_drvdata *ddata = platform_get_drvdata(pdev);
 	struct input_dev *input = ddata->input;
 	int i;
@@ -559,13 +558,13 @@ static int __devexit gpio_keys_remove(struct platform_device *pdev)
 
 	device_init_wakeup(&pdev->dev, 0);
 
-	for (i = 0; i < pdata->nbuttons; i++) {
-		int irq = gpio_to_irq(pdata->buttons[i].gpio);
+	for (i = 0; i < ddata->n_buttons; i++) {
+		int irq = gpio_to_irq(ddata->data[i].button.gpio);
 		free_irq(irq, &ddata->data[i]);
 		if (ddata->data[i].timer_debounce)
 			del_timer_sync(&ddata->data[i].timer);
 		cancel_work_sync(&ddata->data[i].work);
-		gpio_free(pdata->buttons[i].gpio);
+		gpio_free(ddata->data[i].button.gpio);
 	}
 
 	input_unregister_device(input);
@@ -579,12 +578,13 @@ static int __devexit gpio_keys_remove(struct platform_device *pdev)
 static int gpio_keys_suspend(struct device *dev)
 {
 	struct platform_device *pdev = to_platform_device(dev);
-	struct gpio_keys_platform_data *pdata = pdev->dev.platform_data;
+	struct gpio_keys_drvdata *ddata = platform_get_drvdata(pdev);
 	int i;
 
 	if (device_may_wakeup(&pdev->dev)) {
-		for (i = 0; i < pdata->nbuttons; i++) {
-			struct gpio_keys_button *button = &pdata->buttons[i];
+		for (i = 0; i < ddata->n_buttons; i++) {
+			struct gpio_keys_button *button =
+						&ddata->data[i].button;
 			if (button->wakeup) {
 				int irq = gpio_to_irq(button->gpio);
 				enable_irq_wake(irq);
@@ -599,12 +599,11 @@ static int gpio_keys_resume(struct device *dev)
 {
 	struct platform_device *pdev = to_platform_device(dev);
 	struct gpio_keys_drvdata *ddata = platform_get_drvdata(pdev);
-	struct gpio_keys_platform_data *pdata = pdev->dev.platform_data;
 	int i;
 
-	for (i = 0; i < pdata->nbuttons; i++) {
+	for (i = 0; i < ddata->n_buttons; i++) {
 
-		struct gpio_keys_button *button = &pdata->buttons[i];
+		struct gpio_keys_button *button = &ddata->data[i].button;
 		if (button->wakeup && device_may_wakeup(&pdev->dev)) {
 			int irq = gpio_to_irq(button->gpio);
 			disable_irq_wake(irq);
-- 
1.7.4.1

Re: [PATCH 1/2] Input: gpio-keys: do not reference platform_data after .probe exits

[Dmitry Torokhov]

On Monday, July 18, 2011 09:45:07 AM Shawn Guo wrote:
> The patch makes a copy of platform data into driver data, so that any
> reference to platform_data after .probe exits can be avoided.

And why is this beneficial? I am of the opinion that platform data should
stay on (and be accessed through a const pointer to ensure that the driver
will not alter it).

Thanks.

-- 
Dmitry

Re: [PATCH 1/2] Input: gpio-keys: do not reference platform_data after .probe exits

[Grant Likely]

On Mon, Jul 18, 2011 at 11:02 AM, Dmitry Torokhov
<dmitry.torokhov@gmail.com> wrote:
> On Monday, July 18, 2011 09:45:07 AM Shawn Guo wrote:
>> The patch makes a copy of platform data into driver data, so that any
>> reference to platform_data after .probe exits can be avoided.
>
> And why is this beneficial? I am of the opinion that platform data should
> stay on (and be accessed through a const pointer to ensure that the driver
> will not alter it).

Because when using the device tree, there is no platform_data.

g.

Re: [PATCH 1/2] Input: gpio-keys: do not reference platform_data after .probe exits

[Dmitry Torokhov]

On Monday, July 18, 2011 10:15:27 AM Grant Likely wrote:
> On Mon, Jul 18, 2011 at 11:02 AM, Dmitry Torokhov
> 
> <dmitry.torokhov@gmail.com> wrote:
> > On Monday, July 18, 2011 09:45:07 AM Shawn Guo wrote:
> >> The patch makes a copy of platform data into driver data, so that any
> >> reference to platform_data after .probe exits can be avoided.
> > 
> > And why is this beneficial? I am of the opinion that platform data
> > should stay on (and be accessed through a const pointer to ensure
> > that the driver will not alter it).
> 
> Because when using the device tree, there is no platform_data.
>

So allocate it... That's what Davids patch does. BTW, you never gave
ACK for the final version and I'd prefer to have it for the DT bindings.

Thanks.

-- 
Dmitry

Re: [PATCH 1/2] Input: gpio-keys: do not reference platform_data after .probe exits

[Shawn Guo]

On Mon, Jul 18, 2011 at 10:02:44AM -0700, Dmitry Torokhov wrote:
> On Monday, July 18, 2011 09:45:07 AM Shawn Guo wrote:
> > The patch makes a copy of platform data into driver data, so that any
> > reference to platform_data after .probe exits can be avoided.
> 
> And why is this beneficial? I am of the opinion that platform data should
> stay on (and be accessed through a const pointer to ensure that the driver
> will not alter it).
> 
To me, it's a common sense that platform data should not be referenced
after .probe exits, so that any platform code providing the data can
claim the data as __initconst.  When we build multiple platforms which
all have their own platform data for gpio_keys in the kernel, there is
only one of them will be used by gpio_keys driver, others should be
freed after init, no?

-- 
Regards,
Shawn

Re: [PATCH 1/2] Input: gpio-keys: do not reference platform_data after .probe exits

[Russell King - ARM Linux]

On Tue, Jul 19, 2011 at 09:17:26AM +0800, Shawn Guo wrote:
> On Mon, Jul 18, 2011 at 10:02:44AM -0700, Dmitry Torokhov wrote:
> > On Monday, July 18, 2011 09:45:07 AM Shawn Guo wrote:
> > > The patch makes a copy of platform data into driver data, so that any
> > > reference to platform_data after .probe exits can be avoided.
> > 
> > And why is this beneficial? I am of the opinion that platform data should
> > stay on (and be accessed through a const pointer to ensure that the driver
> > will not alter it).
> > 
> To me, it's a common sense that platform data should not be referenced
> after .probe exits, so that any platform code providing the data can
> claim the data as __initconst.

That's totally buggered, and that's putting it kindly.

Consider a driver built as a module, vs built-in.  If you build it as a
module, your driver data is stale by the time you insert the module.
It's not much better with it built-in - if you have hotplug enabled, you
can unbind and rebind the driver, which means that the .probe function
can be called long after the .init sections have been discarded.

So no, this is no justification for the patch.

Don't *ever* make any platform devices or any data pointed to by a
platform device discardable after init time.  It's an oops waiting to
happen.

Re: [PATCH 1/2] Input: gpio-keys: do not reference platform_data after .probe exits

[Shawn Guo]

On Tue, Jul 19, 2011 at 08:48:41AM +0100, Russell King - ARM Linux wrote:
> On Tue, Jul 19, 2011 at 09:17:26AM +0800, Shawn Guo wrote:
> > On Mon, Jul 18, 2011 at 10:02:44AM -0700, Dmitry Torokhov wrote:
> > > On Monday, July 18, 2011 09:45:07 AM Shawn Guo wrote:
> > > > The patch makes a copy of platform data into driver data, so that any
> > > > reference to platform_data after .probe exits can be avoided.
> > > 
> > > And why is this beneficial? I am of the opinion that platform data should
> > > stay on (and be accessed through a const pointer to ensure that the driver
> > > will not alter it).
> > > 
> > To me, it's a common sense that platform data should not be referenced
> > after .probe exits, so that any platform code providing the data can
> > claim the data as __initconst.
> 
> That's totally buggered, and that's putting it kindly.
> 
Well, you can tell I'm an idiot, but I was not trying to do what you
say here.

> Consider a driver built as a module, vs built-in.  If you build it as a
> module, your driver data is stale by the time you insert the module.
> It's not much better with it built-in - if you have hotplug enabled, you
> can unbind and rebind the driver, which means that the .probe function
> can be called long after the .init sections have been discarded.
> 
> So no, this is no justification for the patch.
> 
> Don't *ever* make any platform devices or any data pointed to by a
> platform device discardable after init time.  It's an oops waiting to
> happen.
> 
Something learnt.  Thanks, Russell.

-- 
Regards,
Shawn

[PATCH 2/2] Input: gpio-keys: add device tree probe support

[Shawn Guo]

It adds device tree probe support for gpio-keys driver.

One thing to note is that .enable/.disable hooks becomes unsupported
when the driver gets probed from device tree.  The reason why the
patch does not address that is primarily because there are only 2
out of over 120 boards using the hooks for the gpio-keys device.

board-4430sdp pulls up/down a gpio, while board-mop500 enables/disables
a regulator in that pair of hooks.  There is no common pattern at all,
so the patch leaves the hooks outside the device tree support.

Signed-off-by: Shawn Guo <shawn.guo@linaro.org>
Cc: Grant Likely <grant.likely@secretlab.ca>
Cc: Phil Blundell <pb@handhelds.org>
Cc: Dmitry Torokhov <dtor@mail.ru>
---
 Documentation/devicetree/bindings/gpio/key.txt |   57 ++++++++++++++++
 drivers/input/keyboard/gpio_keys.c             |   83 +++++++++++++++++++++++-
 2 files changed, 139 insertions(+), 1 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/gpio/key.txt

diff --git a/Documentation/devicetree/bindings/gpio/key.txt b/Documentation/devicetree/bindings/gpio/key.txt
new file mode 100644
index 0000000..36296c1b
--- /dev/null
+++ b/Documentation/devicetree/bindings/gpio/key.txt
@@ -0,0 +1,57 @@
+* Keys connected to GPIO lines
+
+Required properties:
+- compatible : Should be "gpio-keys"
+
+Optional properties:
+- keys-auto-repeat : Indicates the support of Auto-Repeat
+
+Each key is represented as a sub-node of the gpio-keys device.  Each
+node's name represents the name of the corresponding key.
+
+Key sub-node properties:
+
+Required properties:
+- gpios : Should specify the key's GPIO.  Active low key should be
+  indicated using flags in the GPIO specifier.
+- linux,key-code : Should specify the key code defined by linux in
+  include/linux/input.h
+
+Optional properties:
+- label : Contains the label for this key
+- key-debounce-internal : Specifies debounce interval in milliseconds
+- key-axis-value : Specifies axis value for an absolute change event
+- key-is-switch : Indicates the key is used as a switch event
+- key-is-active-low : Indicates the key is active low
+- key-can-wakeup-system : Indicates the key is a wake-up source
+- key-can-be-disabled : Indicates the key can be disabled
+
+Examples:
+
+gpio-keys {
+	compatible = "gpio-keys";
+
+	power {
+		label = "Power Button";
+		gpios = <&gpio0 8 0>;
+		linux,key-code = <116>; /* KEY_POWER */
+		key-is-active-low;
+		key-can-wakeup-system;
+	};
+
+	volume-up {
+		label = "Volume Up";
+		gpios = <&gpio1 14 0>;
+		linux,key-code = <115>; /* KEY_VOLUMEUP */
+		key-is-active-low;
+		key-can-wakeup-system;
+	};
+
+	volume-down {
+		label = "Volume Down";
+		gpios = <&gpio1 15 0>;
+		linux,key-code = <114>; /* KEY_VOLUMEDOWN */
+		key-is-active-low;
+		key-can-wakeup-system;
+	};
+};
diff --git a/drivers/input/keyboard/gpio_keys.c b/drivers/input/keyboard/gpio_keys.c
index 85b5685..2a79d40 100644
--- a/drivers/input/keyboard/gpio_keys.c
+++ b/drivers/input/keyboard/gpio_keys.c
@@ -25,6 +25,8 @@
 #include <linux/gpio_keys.h>
 #include <linux/workqueue.h>
 #include <linux/gpio.h>
+#include <linux/of.h>
+#include <linux/of_gpio.h>
 
 struct gpio_button_data {
 	struct gpio_keys_button button;
@@ -445,15 +447,93 @@ static void gpio_keys_close(struct input_dev *input)
 		ddata->disable(input->dev.parent);
 }
 
+#ifdef CONFIG_OF_GPIO
+static const struct of_device_id gpio_keys_dt_ids[] = {
+	{ .compatible = "gpio-keys", },
+	{ /* sentinel */ }
+};
+
+static struct gpio_keys_platform_data * __devinit gpio_keys_probe_pdata_dt(
+						struct platform_device *pdev)
+{
+	struct device_node *np = pdev->dev.of_node, *child;
+	struct gpio_keys_platform_data *pdata;
+	struct gpio_keys_button *button;
+	int count = 0;
+
+	if (!np)
+		return ERR_PTR(-ENODEV);
+
+	/* count keys in this device, so we know how much to allocate */
+	for_each_child_of_node(np, child)
+		count++;
+	if (!count)
+		return ERR_PTR(-ENODEV);
+
+	pdata = devm_kzalloc(&pdev->dev, sizeof(*pdata) +
+			     sizeof(*button) * count, GFP_KERNEL);
+	if (!pdata)
+		return ERR_PTR(-ENOMEM);
+
+	pdata->buttons = (struct gpio_keys_button *) (pdata + 1);
+	pdata->nbuttons = count;
+
+	if (of_get_property(np, "keys-auto-repeat", NULL))
+		pdata->rep = 1;
+
+	button = pdata->buttons;
+	for_each_child_of_node(np, child) {
+		enum of_gpio_flags flags;
+
+		button->gpio = of_get_gpio_flags(child, 0, &flags);
+		of_property_read_string(child, "label", &button->desc);
+		of_property_read_u32(child, "linux,key-code", &button->code);
+		of_property_read_u32(child, "key-debounce-internal",
+				     &button->debounce_interval);
+		if (!of_property_read_u32(child, "key-axis-value",
+					  &button->value))
+			button->type = EV_ABS;
+		if (of_get_property(np, "key-is-switch", NULL))
+			button->type = EV_SW;
+		if (of_get_property(np, "key-is-active-low", NULL))
+			button->active_low = 1;
+		if (of_get_property(np, "key-can-wakeup-system", NULL))
+			button->wakeup = 1;
+		if (of_get_property(np, "key-can-be-disabled", NULL))
+			button->can_disable = 1;
+
+		button++;
+	}
+
+	return pdata;
+}
+#else /* CONFIG_OF_GPIO */
+#define of_gpio_keys_match NULL
+static inline struct gpio_keys_platform_data *gpio_keys_probe_pdata_dt(
+						struct platform_device *pdev)
+{
+	return ERR_PTR(-ENODEV);
+}
+#endif /* CONFIG_OF_GPIO */
+
 static int __devinit gpio_keys_probe(struct platform_device *pdev)
 {
-	struct gpio_keys_platform_data *pdata = pdev->dev.platform_data;
+	struct gpio_keys_platform_data *pdata;
 	struct gpio_keys_drvdata *ddata;
 	struct device *dev = &pdev->dev;
 	struct input_dev *input;
 	int i, error;
 	int wakeup = 0;
 
+	pdata = gpio_keys_probe_pdata_dt(pdev);
+	if (IS_ERR(pdata))
+		pdata = pdev->dev.platform_data;
+
+	if (IS_ERR_OR_NULL(pdata)) {
+		dev_err(dev, "failed to get platform data\n");
+		return PTR_ERR(pdata);
+	}
+
 	ddata = kzalloc(sizeof(struct gpio_keys_drvdata) +
 			pdata->nbuttons * sizeof(struct gpio_button_data),
 			GFP_KERNEL);
@@ -631,6 +711,7 @@ static struct platform_driver gpio_keys_device_driver = {
 #ifdef CONFIG_PM
 		.pm	= &gpio_keys_pm_ops,
 #endif
+		.of_match_table = gpio_keys_dt_ids,
 	}
 };
 
-- 
1.7.4.1

Re: [PATCH 2/2] Input: gpio-keys: add device tree probe support

[Dmitry Torokhov]

Hi Shawn,

On Monday, July 18, 2011 09:45:08 AM Shawn Guo wrote:
> It adds device tree probe support for gpio-keys driver.

There is already a patch by David Jander adding device tree support to
the driver; I intend to apply it.

Thanks.

-- 
Dmitry

Re: [PATCH 2/2] Input: gpio-keys: add device tree probe support

[Shawn Guo]

On Mon, Jul 18, 2011 at 10:05:52AM -0700, Dmitry Torokhov wrote:
> Hi Shawn,
> 
> On Monday, July 18, 2011 09:45:08 AM Shawn Guo wrote:
> > It adds device tree probe support for gpio-keys driver.
> 
> There is already a patch by David Jander adding device tree support to
> the driver; I intend to apply it.
> 
Sorry, I was not aware of David's patch.  No problem, you can pick
whichever you want.  All I want to see is gpio_keys gets device tree
support, because I need to convert several board files to device tree
and the gpio_keys seems to be the only one missing device tree support
there.

-- 
Regards,
Shawn

Re: [PATCH 2/2] Input: gpio-keys: add device tree probe support

[Shawn Guo]

On Mon, Jul 18, 2011 at 10:05:52AM -0700, Dmitry Torokhov wrote:
> Hi Shawn,
> 
> On Monday, July 18, 2011 09:45:08 AM Shawn Guo wrote:
> > It adds device tree probe support for gpio-keys driver.
> 
> There is already a patch by David Jander adding device tree support to
> the driver; I intend to apply it.
> 
I just fetched it from linux-next.  Though it seems working for me,
I'm wondering why there are no bindings for following configurations.

struct gpio_keys_button {
	...
        int active_low;
	...
        bool can_disable;
        int value;              /* axis value for EV_ABS */
};

I'm seeing some platforms using them in their gpio_keys platform data.
Does that mean the dt version of the driver will be broken on these
platforms?

BTW, if I have fetched linux-next a little earlier or David Cc list
devicetree-discuss when he posted his patch, I do not have to waste
one day effort :)

-- 
Regards,
Shawn