[RFC PATCH 0/1] Enabling/disabling separate gpio-keys buttons

[RFC PATCH 0/1] Enabling/disabling separate gpio-keys buttons

[Mika Westerberg]

Some embedded devices use gpio lines as keys/buttons through gpio-keys.
Current implementation doesn't allow one to enable/disable these buttons
separately.

Example use-case would be that when phone is locked in our pocket,
we don't want it to woken up when camera key gets accidentally pressed.

Following patch exports gpio-keys through sysfs to userland and allows
userland programs to control whether gpio line (button/key) is disabled
or enabled.

Thanks,
MW


Mika Westerberg (1):
  Input: gpio-keys: export gpio key information through sysfs

 drivers/input/keyboard/gpio_keys.c |  162 ++++++++++++++++++++++++++++++++++++
 1 files changed, 162 insertions(+), 0 deletions(-)

[RFC PATCH 1/1] Input: gpio-keys: export gpio key information through sysfs

[Mika Westerberg]

From: Mika Westerberg <ext-mika.1.westerberg@nokia.com>

In some embedded devices gpio lines are used as keys/buttons
through input layer and gpio-keys.  It is, however, impossible
to disable gpio lines separately from waking up the cpu.  For
example when device is locked we don't want accidental camera
button press to cause the device to wakeup just to notice that
it should continue sleeping.

This patch exports gpio-keys through sysfs and allows userland
to control whether single gpio line should wakeup the cpu or not.

Sysfs interface is accessible via:

	/sys/class/input/gpio-keys/input/input0/gpio-key.N/

Following attributes are exported per gpio key:

	/code    ... input event code (ro)
	/type    ... input event type (ro)
	/desc    ... description of the button (ro)
	/disable ... enable/disable gpio line (rw)

Userspace should be able to find out what key to disable/enable
by investigating {code, type, desc} tuple.

Signed-off-by: Mika Westerberg <ext-mika.1.westerberg@nokia.com>
---
 drivers/input/keyboard/gpio_keys.c |  162 ++++++++++++++++++++++++++++++++++++
 1 files changed, 162 insertions(+), 0 deletions(-)

diff --git a/drivers/input/keyboard/gpio_keys.c b/drivers/input/keyboard/gpio_keys.c
index a88aff3..76e7c5c 100644
--- a/drivers/input/keyboard/gpio_keys.c
+++ b/drivers/input/keyboard/gpio_keys.c
@@ -31,6 +31,8 @@ struct gpio_button_data {
 	struct input_dev *input;
 	struct timer_list timer;
 	struct work_struct work;
+	struct device *dev;	/* device used to export button to sysfs */
+	bool disable;		/* is this gpio button disabled */
 };
 
 struct gpio_keys_drvdata {
@@ -38,6 +40,159 @@ struct gpio_keys_drvdata {
 	struct gpio_button_data data[0];
 };
 
+/*
+ * gpio-keys sysfs interface
+ *
+ * Following interface is export to userspace through
+ * sysfs.  This interface can be used to enable/disable
+ * single GPIO lines from generating events.
+ *
+ * /sys/class/input/gpio-keys/input/input0/gpio-key.N/
+ * 	/code	 ... input event code (ro)
+ *	/type	 ... input event type (ro)
+ *	/desc	 ... description of the button (ro)
+ *	/disable ... enable/disable gpio line from
+ *	             generating events (rw)
+ *
+ * Userspace program can enumerate these keys and based
+ * on {code,type,desc} tuple disable or enable the line
+ * depending on the system state.
+ */
+
+static ssize_t gpio_keys_code_show(struct device *dev,
+	struct device_attribute *attr, char *buf)
+{
+	const struct gpio_button_data *bdata = dev_get_drvdata(dev);
+
+	return snprintf(buf, PAGE_SIZE, "%d\n", bdata->button->code);
+}
+
+static DEVICE_ATTR(code, S_IRUGO, gpio_keys_code_show, NULL);
+
+static ssize_t gpio_keys_type_show(struct device *dev,
+	struct device_attribute *attr, char *buf)
+{
+	const struct gpio_button_data *bdata = dev_get_drvdata(dev);
+
+	return snprintf(buf, PAGE_SIZE, "%d\n", bdata->button->type);
+}
+
+static DEVICE_ATTR(type, S_IRUGO, gpio_keys_type_show, NULL);
+
+static ssize_t gpio_keys_desc_show(struct device *dev,
+	struct device_attribute *attr, char *buf)
+{
+	const struct gpio_button_data *bdata = dev_get_drvdata(dev);
+
+	return snprintf(buf, PAGE_SIZE, "%s\n", bdata->button->desc);
+}
+
+static DEVICE_ATTR(desc, S_IRUGO, gpio_keys_desc_show, NULL);
+
+static ssize_t gpio_keys_disable_show(struct device *dev,
+	struct device_attribute *attr, char *buf)
+{
+	const struct gpio_button_data *bdata = dev_get_drvdata(dev);
+
+	return snprintf(buf, PAGE_SIZE, "%d\n", (int)bdata->disable);
+}
+
+static ssize_t gpio_keys_disable_store(struct device *dev,
+	struct device_attribute *attr, const char *buf, size_t size)
+{
+	struct gpio_button_data *bdata = dev_get_drvdata(dev);
+	int val;
+
+	if (sscanf(buf, "%d", &val) != 1)
+		return -EINVAL;
+
+	val = !!val;
+	if (bdata->disable == val)
+		return size;
+
+	if (val) {
+		disable_irq(gpio_to_irq(bdata->button->gpio));
+	} else {
+		enable_irq(gpio_to_irq(bdata->button->gpio));
+	}
+
+	bdata->disable = val;
+	return size;
+}
+
+static DEVICE_ATTR(disable, S_IWUSR | S_IRUGO,
+	gpio_keys_disable_show, gpio_keys_disable_store);
+
+static struct attribute *gpio_keys_attrs[] = {
+	&dev_attr_code.attr,
+	&dev_attr_type.attr,
+	&dev_attr_desc.attr,
+	&dev_attr_disable.attr,
+	NULL,
+};
+
+static const struct attribute_group gpio_keys_attr_group = {
+	.attrs = gpio_keys_attrs,
+};
+
+static void gpio_keys_unexport(struct platform_device *pdev)
+{
+	struct gpio_keys_platform_data *pdata = pdev->dev.platform_data;
+	struct gpio_keys_drvdata *ddata = platform_get_drvdata(pdev);
+	int i;
+
+	for (i = 0; i < pdata->nbuttons; i++) {
+		struct gpio_button_data *bdata = &ddata->data[i];
+
+		if (bdata->dev != NULL) {
+			sysfs_remove_group(&bdata->dev->kobj,
+				&gpio_keys_attr_group);
+			device_unregister(bdata->dev);
+			bdata->dev = NULL;
+		}
+	}
+}
+
+static int gpio_keys_export(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, error = 0;
+
+	for (i = 0; i < pdata->nbuttons; i++) {
+		struct gpio_button_data *bdata = &ddata->data[i];
+		struct device *dev;
+
+		/*
+		 * We create one "device" per gpio line that is used
+		 * as input gpio key device.
+		 */
+		dev = device_create(input->dev.class, &input->dev,
+			MKDEV(0, 0), bdata, "gpio-key.%d", i);
+		if (dev == NULL) {
+			error = -ENOMEM;
+			break;
+		}
+
+		error = sysfs_create_group(&dev->kobj, &gpio_keys_attr_group);
+		if (error != 0) {
+			device_unregister(dev);
+			break;
+		}
+
+		bdata->disable = false;
+		bdata->dev = dev;
+	}
+
+	if (error != 0) {
+		/* something failed, clean up all entries */
+		gpio_keys_unexport(pdev);
+	}
+
+	return error;
+}
+
 static void gpio_keys_report_event(struct work_struct *work)
 {
 	struct gpio_button_data *bdata =
@@ -170,6 +325,12 @@ static int __devinit gpio_keys_probe(struct platform_device *pdev)
 		goto fail2;
 	}
 
+	error = gpio_keys_export(pdev);
+	if (error) {
+		pr_warning("gpio-keys: Unable to export gpio-keys to sysfs, "
+			" error %d", error);
+	}
+
 	device_init_wakeup(&pdev->dev, wakeup);
 
 	return 0;
@@ -199,6 +360,7 @@ static int __devexit gpio_keys_remove(struct platform_device *pdev)
 	int i;
 
 	device_init_wakeup(&pdev->dev, 0);
+	gpio_keys_unexport(pdev);
 
 	for (i = 0; i < pdata->nbuttons; i++) {
 		int irq = gpio_to_irq(pdata->buttons[i].gpio);
-- 
1.5.6.5

Re: [RFC PATCH 1/1] Input: gpio-keys: export gpio key information through sysfs

[Dmitry Torokhov]

Hi Mika,

On Fri, Oct 23, 2009 at 03:15:46PM +0300, Mika Westerberg wrote:
> From: Mika Westerberg <ext-mika.1.westerberg@nokia.com>
> 
> In some embedded devices gpio lines are used as keys/buttons
> through input layer and gpio-keys.  It is, however, impossible
> to disable gpio lines separately from waking up the cpu.  For
> example when device is locked we don't want accidental camera
> button press to cause the device to wakeup just to notice that
> it should continue sleeping.
> 
> This patch exports gpio-keys through sysfs and allows userland
> to control whether single gpio line should wakeup the cpu or not.
> 
> Sysfs interface is accessible via:
> 
> 	/sys/class/input/gpio-keys/input/input0/gpio-key.N/
> 
> Following attributes are exported per gpio key:
> 
> 	/code    ... input event code (ro)
> 	/type    ... input event type (ro)
> 	/desc    ... description of the button (ro)
> 	/disable ... enable/disable gpio line (rw)
> 
> Userspace should be able to find out what key to disable/enable
> by investigating {code, type, desc} tuple.
> 

I think registering a full-blown device for every key is way too much,
given that most consumers of gpio-keys driver are embedded... Besides, I
don't think this should be driven from userspace. Board (platform) code
should know what GPIO make sense as wake up sources for the particular
device and should set up platform data accordingly.

-- 
Dmitry

Re: [RFC PATCH 1/1] Input: gpio-keys: export gpio key information through sysfs

[Mika Westerberg]

Hi,

On Wed, Oct 28, 2009 at 06:43:17AM +0100, ext Dmitry Torokhov wrote:
> 
> On Fri, Oct 23, 2009 at 03:15:46PM +0300, Mika Westerberg wrote:
> > From: Mika Westerberg <ext-mika.1.westerberg@nokia.com>
> > 
> > In some embedded devices gpio lines are used as keys/buttons
> > through input layer and gpio-keys.  It is, however, impossible
> > to disable gpio lines separately from waking up the cpu.  For
> > example when device is locked we don't want accidental camera
> > button press to cause the device to wakeup just to notice that
> > it should continue sleeping.
> > 
> > This patch exports gpio-keys through sysfs and allows userland
> > to control whether single gpio line should wakeup the cpu or not.
> > 
> > Sysfs interface is accessible via:
> > 
> > 	/sys/class/input/gpio-keys/input/input0/gpio-key.N/
> > 
> > Following attributes are exported per gpio key:
> > 
> > 	/code    ... input event code (ro)
> > 	/type    ... input event type (ro)
> > 	/desc    ... description of the button (ro)
> > 	/disable ... enable/disable gpio line (rw)
> > 
> > Userspace should be able to find out what key to disable/enable
> > by investigating {code, type, desc} tuple.
> > 
> 
> I think registering a full-blown device for every key is way too much,
> given that most consumers of gpio-keys driver are embedded... Besides, I
> don't think this should be driven from userspace. Board (platform) code
> should know what GPIO make sense as wake up sources for the particular
> device and should set up platform data accordingly.

Thanks for the comments.

This was done because setting these from board files is not enough
as this is something that should be dynamically configured.  For example
when we lock the device, there are some buttons (gpio lines) that will
wakeup the device but not all of them.  When device goes to sleep (but
is not locked) then there are additional buttons that can be used to
wake it up.

We have daemon that does this stuff depending on system state so we
need some way to let userspace to control these.

Do you have any suggestions or alternative ways of doing this?  Would
it be beneficial for other consumers to dynamically configure these
lines (maybe not through sysfs but for example via ioctl)?

Thanks,
MW

Re: [RFC PATCH 1/1] Input: gpio-keys: export gpio key information through sysfs

[Mika Westerberg]

On Wed, Oct 28, 2009 at 12:50:01PM +0200, Mika Westerberg wrote:
> Hi,
> 
> On Wed, Oct 28, 2009 at 06:43:17AM +0100, ext Dmitry Torokhov wrote:
> > 
> > On Fri, Oct 23, 2009 at 03:15:46PM +0300, Mika Westerberg wrote:
> > > From: Mika Westerberg <ext-mika.1.westerberg@nokia.com>
> > > 
> > > In some embedded devices gpio lines are used as keys/buttons
> > > through input layer and gpio-keys.  It is, however, impossible
> > > to disable gpio lines separately from waking up the cpu.  For
> > > example when device is locked we don't want accidental camera
> > > button press to cause the device to wakeup just to notice that
> > > it should continue sleeping.
> > > 
> > > This patch exports gpio-keys through sysfs and allows userland
> > > to control whether single gpio line should wakeup the cpu or not.
> > > 
> > > Sysfs interface is accessible via:
> > > 
> > > 	/sys/class/input/gpio-keys/input/input0/gpio-key.N/
> > > 
> > > Following attributes are exported per gpio key:
> > > 
> > > 	/code    ... input event code (ro)
> > > 	/type    ... input event type (ro)
> > > 	/desc    ... description of the button (ro)
> > > 	/disable ... enable/disable gpio line (rw)
> > > 
> > > Userspace should be able to find out what key to disable/enable
> > > by investigating {code, type, desc} tuple.
> > > 
> > 
> > I think registering a full-blown device for every key is way too much,
> > given that most consumers of gpio-keys driver are embedded... Besides, I
> > don't think this should be driven from userspace. Board (platform) code
> > should know what GPIO make sense as wake up sources for the particular
> > device and should set up platform data accordingly.
> 
> Thanks for the comments.
> 
> This was done because setting these from board files is not enough
> as this is something that should be dynamically configured.  For example
> when we lock the device, there are some buttons (gpio lines) that will
> wakeup the device but not all of them.  When device goes to sleep (but
> is not locked) then there are additional buttons that can be used to
> wake it up.
> 
> We have daemon that does this stuff depending on system state so we
> need some way to let userspace to control these.
> 
> Do you have any suggestions or alternative ways of doing this?  Would
> it be beneficial for other consumers to dynamically configure these
> lines (maybe not through sysfs but for example via ioctl)?

Hi Dmitry,

Sorry to bother you again.  Do you have any comments on this?  How about
adding few ioctl()s to disable/enable the keys on the input device?

Thanks,
MW

Re: [RFC PATCH 1/1] Input: gpio-keys: export gpio key information through sysfs

[Artem Bityutskiy]

On Wed, 2009-11-04 at 11:06 +0200, Mika Westerberg wrote:
> On Wed, Oct 28, 2009 at 12:50:01PM +0200, Mika Westerberg wrote:
> > Hi,
> > 
> > On Wed, Oct 28, 2009 at 06:43:17AM +0100, ext Dmitry Torokhov wrote:
> > > 
> > > On Fri, Oct 23, 2009 at 03:15:46PM +0300, Mika Westerberg wrote:
> > > > From: Mika Westerberg <ext-mika.1.westerberg@nokia.com>
> > > > 
> > > > In some embedded devices gpio lines are used as keys/buttons
> > > > through input layer and gpio-keys.  It is, however, impossible
> > > > to disable gpio lines separately from waking up the cpu.  For
> > > > example when device is locked we don't want accidental camera
> > > > button press to cause the device to wakeup just to notice that
> > > > it should continue sleeping.
> > > > 
> > > > This patch exports gpio-keys through sysfs and allows userland
> > > > to control whether single gpio line should wakeup the cpu or not.
> > > > 
> > > > Sysfs interface is accessible via:
> > > > 
> > > > 	/sys/class/input/gpio-keys/input/input0/gpio-key.N/
> > > > 
> > > > Following attributes are exported per gpio key:
> > > > 
> > > > 	/code    ... input event code (ro)
> > > > 	/type    ... input event type (ro)
> > > > 	/desc    ... description of the button (ro)
> > > > 	/disable ... enable/disable gpio line (rw)
> > > > 
> > > > Userspace should be able to find out what key to disable/enable
> > > > by investigating {code, type, desc} tuple.
> > > > 
> > > 
> > > I think registering a full-blown device for every key is way too much,
> > > given that most consumers of gpio-keys driver are embedded... Besides, I
> > > don't think this should be driven from userspace. Board (platform) code
> > > should know what GPIO make sense as wake up sources for the particular
> > > device and should set up platform data accordingly.

For example, on N900 we want to disable the lens cover / proximity / etc
gpios when the phones' screen is locked. Simply because we do not want
the lines to generate interrupts, wakes up the CPU and waste our battery
energy. But we do not want to disable the screen lock gpio, and some
incoming call related gpios.

So we really do want this to be userspace-driven.

-- 
Best Regards,
Artem Bityutskiy (Артём Битюцкий)

--
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: [RFC PATCH 1/1] Input: gpio-keys: export gpio key information through sysfs

[Dmitry Torokhov]

On Wed, Nov 04, 2009 at 11:25:59AM +0200, Artem Bityutskiy wrote:
> On Wed, 2009-11-04 at 11:06 +0200, Mika Westerberg wrote:
> > On Wed, Oct 28, 2009 at 12:50:01PM +0200, Mika Westerberg wrote:
> > > Hi,
> > > 
> > > On Wed, Oct 28, 2009 at 06:43:17AM +0100, ext Dmitry Torokhov wrote:
> > > > 
> > > > On Fri, Oct 23, 2009 at 03:15:46PM +0300, Mika Westerberg wrote:
> > > > > From: Mika Westerberg <ext-mika.1.westerberg@nokia.com>
> > > > > 
> > > > > In some embedded devices gpio lines are used as keys/buttons
> > > > > through input layer and gpio-keys.  It is, however, impossible
> > > > > to disable gpio lines separately from waking up the cpu.  For
> > > > > example when device is locked we don't want accidental camera
> > > > > button press to cause the device to wakeup just to notice that
> > > > > it should continue sleeping.
> > > > > 
> > > > > This patch exports gpio-keys through sysfs and allows userland
> > > > > to control whether single gpio line should wakeup the cpu or not.
> > > > > 
> > > > > Sysfs interface is accessible via:
> > > > > 
> > > > > 	/sys/class/input/gpio-keys/input/input0/gpio-key.N/
> > > > > 
> > > > > Following attributes are exported per gpio key:
> > > > > 
> > > > > 	/code    ... input event code (ro)
> > > > > 	/type    ... input event type (ro)
> > > > > 	/desc    ... description of the button (ro)
> > > > > 	/disable ... enable/disable gpio line (rw)
> > > > > 
> > > > > Userspace should be able to find out what key to disable/enable
> > > > > by investigating {code, type, desc} tuple.
> > > > > 
> > > > 
> > > > I think registering a full-blown device for every key is way too much,
> > > > given that most consumers of gpio-keys driver are embedded... Besides, I
> > > > don't think this should be driven from userspace. Board (platform) code
> > > > should know what GPIO make sense as wake up sources for the particular
> > > > device and should set up platform data accordingly.
> 
> For example, on N900 we want to disable the lens cover / proximity / etc
> gpios when the phones' screen is locked. Simply because we do not want
> the lines to generate interrupts, wakes up the CPU and waste our battery
> energy. But we do not want to disable the screen lock gpio, and some
> incoming call related gpios.
> 
> So we really do want this to be userspace-driven.
> 

OK, then maybe we should use 2 attributes, one showing gpios that can
be used for wakeup and one showing gpios that are currently set up as
wakeup sources. These can be built on using bitmap_scnlistprintf() and
bitmap_parselist() and can work with either GPIO numbers or keycodes.

-- 
Dmitry

Re: [RFC PATCH 1/1] Input: gpio-keys: export gpio key information through sysfs

[Artem Bityutskiy]

On Thu, 2009-11-05 at 23:52 -0800, Dmitry Torokhov wrote:
> On Wed, Nov 04, 2009 at 11:25:59AM +0200, Artem Bityutskiy wrote:
> > > > > I think registering a full-blown device for every key is way too much,
> > > > > given that most consumers of gpio-keys driver are embedded... Besides, I
> > > > > don't think this should be driven from userspace. Board (platform) code
> > > > > should know what GPIO make sense as wake up sources for the particular
> > > > > device and should set up platform data accordingly.
> > 
> > For example, on N900 we want to disable the lens cover / proximity / etc
> > gpios when the phones' screen is locked. Simply because we do not want
> > the lines to generate interrupts, wakes up the CPU and waste our battery
> > energy. But we do not want to disable the screen lock gpio, and some
> > incoming call related gpios.
> > 
> > So we really do want this to be userspace-driven.
> > 
> 
> OK, then maybe we should use 2 attributes, one showing gpios that can
> be used for wakeup and one showing gpios that are currently set up as
> wakeup sources. These can be built on using bitmap_scnlistprintf() and
> bitmap_parselist() and can work with either GPIO numbers or keycodes.

Hi Dmitry,

could you please elaborate some more?

* are you talking about per-input device sysfs files?
* could you please give an example of how would I switch off, say, the
  SW_CAMERA_LENS_COVER gpio by the keycode.
* why this should be sysfs and not a new ioctl?
* should this be something specific to gpio-keys or common for all
  devices?

Thanks.

-- 
Best Regards,
Artem Bityutskiy (Артём Битюцкий)

--
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: [RFC PATCH 1/1] Input: gpio-keys: export gpio key information through sysfs

[Dmitry Torokhov]

Hi Artem,

On Mon, Nov 09, 2009 at 05:09:04PM +0200, Artem Bityutskiy wrote:
> On Thu, 2009-11-05 at 23:52 -0800, Dmitry Torokhov wrote:
> > On Wed, Nov 04, 2009 at 11:25:59AM +0200, Artem Bityutskiy wrote:
> > > > > > I think registering a full-blown device for every key is way too much,
> > > > > > given that most consumers of gpio-keys driver are embedded... Besides, I
> > > > > > don't think this should be driven from userspace. Board (platform) code
> > > > > > should know what GPIO make sense as wake up sources for the particular
> > > > > > device and should set up platform data accordingly.
> > > 
> > > For example, on N900 we want to disable the lens cover / proximity / etc
> > > gpios when the phones' screen is locked. Simply because we do not want
> > > the lines to generate interrupts, wakes up the CPU and waste our battery
> > > energy. But we do not want to disable the screen lock gpio, and some
> > > incoming call related gpios.
> > > 
> > > So we really do want this to be userspace-driven.
> > > 
> > 
> > OK, then maybe we should use 2 attributes, one showing gpios that can
> > be used for wakeup and one showing gpios that are currently set up as
> > wakeup sources. These can be built on using bitmap_scnlistprintf() and
> > bitmap_parselist() and can work with either GPIO numbers or keycodes.
> 
> Hi Dmitry,
> 
> could you please elaborate some more?
> 
> * are you talking about per-input device sysfs files?

I am not sure at what level these attributes must be created. While the
easiest way is indeed per-input device attribute (for that particular
driver) the functionality does not really have anything to do with
input... I'd probably prefer having this attribute elsewhere, maybe
we should extend /sys/class/gpio/gpioN for these purposes?

> * could you please give an example of how would I switch off, say, the
>   SW_CAMERA_LENS_COVER gpio by the keycode.

If the code lies in your driver then it is easy (the driver knows the
mapping); otherwsie you'll have to go with GPIO number.

> * why this should be sysfs and not a new ioctl?

Because it makes sense onfy for 1 or 2 devices.

> * should this be something specific to gpio-keys or common for all
>   devices?

I don't see how you can make it common for all input devices... But
maybe there other subsystems (PM?) that would make better sense for such
facility (control of wakeup sources). In fact, there is already power/wakeup
attribute, can you plug in there?

Thanks.

-- 
Dmitry

Re: [RFC PATCH 1/1] Input: gpio-keys: export gpio key information through sysfs

[Artem Bityutskiy]

On Mon, 2009-11-09 at 09:18 -0800, Dmitry Torokhov wrote:
> Hi Artem,
> 
> On Mon, Nov 09, 2009 at 05:09:04PM +0200, Artem Bityutskiy wrote:
> > On Thu, 2009-11-05 at 23:52 -0800, Dmitry Torokhov wrote:
> > > On Wed, Nov 04, 2009 at 11:25:59AM +0200, Artem Bityutskiy wrote:
> > > > > > > I think registering a full-blown device for every key is way too much,
> > > > > > > given that most consumers of gpio-keys driver are embedded... Besides, I
> > > > > > > don't think this should be driven from userspace. Board (platform) code
> > > > > > > should know what GPIO make sense as wake up sources for the particular
> > > > > > > device and should set up platform data accordingly.
> > > > 
> > > > For example, on N900 we want to disable the lens cover / proximity / etc
> > > > gpios when the phones' screen is locked. Simply because we do not want
> > > > the lines to generate interrupts, wakes up the CPU and waste our battery
> > > > energy. But we do not want to disable the screen lock gpio, and some
> > > > incoming call related gpios.
> > > > 
> > > > So we really do want this to be userspace-driven.
> > > > 
> > > 
> > > OK, then maybe we should use 2 attributes, one showing gpios that can
> > > be used for wakeup and one showing gpios that are currently set up as
> > > wakeup sources. These can be built on using bitmap_scnlistprintf() and
> > > bitmap_parselist() and can work with either GPIO numbers or keycodes.
> > 
> > Hi Dmitry,
> > 
> > could you please elaborate some more?
> > 
> > * are you talking about per-input device sysfs files?
> 
> I am not sure at what level these attributes must be created. While the
> easiest way is indeed per-input device attribute (for that particular
> driver) the functionality does not really have anything to do with
> input... I'd probably prefer having this attribute elsewhere, maybe
> we should extend /sys/class/gpio/gpioN for these purposes?
> 
> > * could you please give an example of how would I switch off, say, the
> >   SW_CAMERA_LENS_COVER gpio by the keycode.
> 
> If the code lies in your driver then it is easy (the driver knows the
> mapping); otherwsie you'll have to go with GPIO number.

GPIO numbers are bad for us. We want to work with keycodes, and this is
input subsystem after all. We do not want to do any perversions like
making our user-space figuring out gpio numbers by the keycodes. This is
simply wrong and bad interface.

We want to have a simple way for the application to disable any key by
key code. This should be as simple as calling one single
"enable/disable" ioctl for /dev/input/event2. I do not see how this
could be done nicely with sysfs, still.

But we will try to come up with some solution.

-- 
Best Regards,
Artem Bityutskiy (Артём Битюцкий)

--
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: [RFC PATCH 1/1] Input: gpio-keys: export gpio key information through sysfs

[Dmitry Torokhov]

On Tue, Nov 10, 2009 at 01:04:19PM +0200, Artem Bityutskiy wrote:
> On Mon, 2009-11-09 at 09:18 -0800, Dmitry Torokhov wrote:
> > 
> > I am not sure at what level these attributes must be created. While the
> > easiest way is indeed per-input device attribute (for that particular
> > driver) the functionality does not really have anything to do with
> > input... I'd probably prefer having this attribute elsewhere, maybe
> > we should extend /sys/class/gpio/gpioN for these purposes?
> > 
> > > * could you please give an example of how would I switch off, say, the
> > >   SW_CAMERA_LENS_COVER gpio by the keycode.
> > 
> > If the code lies in your driver then it is easy (the driver knows the
> > mapping); otherwsie you'll have to go with GPIO number.
> 
> GPIO numbers are bad for us. We want to work with keycodes, and this is
> input subsystem after all. We do not want to do any perversions like
> making our user-space figuring out gpio numbers by the keycodes. This is
> simply wrong and bad interface.
> 
> We want to have a simple way for the application to disable any key by
> key code. This should be as simple as calling one single
> "enable/disable" ioctl for /dev/input/event2. I do not see how this
> could be done nicely with sysfs, still.
> 

I understand the appeal of working with the keycodes but what you are
asking is not simply ignoring certain keypresses (that is easy, just
ignore corresponding events in userspace), you want to prevent system
from waking up. In other words you want to "control PM layer through
input layer" and I believe this is wrong.

-- 
Dmitry

Re: [RFC PATCH 1/1] Input: gpio-keys: export gpio key information through sysfs

[Artem Bityutskiy]

On Tue, 2009-11-10 at 09:19 -0800, Dmitry Torokhov wrote:
> On Tue, Nov 10, 2009 at 01:04:19PM +0200, Artem Bityutskiy wrote:
> > On Mon, 2009-11-09 at 09:18 -0800, Dmitry Torokhov wrote:
> > > 
> > > I am not sure at what level these attributes must be created. While the
> > > easiest way is indeed per-input device attribute (for that particular
> > > driver) the functionality does not really have anything to do with
> > > input... I'd probably prefer having this attribute elsewhere, maybe
> > > we should extend /sys/class/gpio/gpioN for these purposes?
> > > 
> > > > * could you please give an example of how would I switch off, say, the
> > > >   SW_CAMERA_LENS_COVER gpio by the keycode.
> > > 
> > > If the code lies in your driver then it is easy (the driver knows the
> > > mapping); otherwsie you'll have to go with GPIO number.
> > 
> > GPIO numbers are bad for us. We want to work with keycodes, and this is
> > input subsystem after all. We do not want to do any perversions like
> > making our user-space figuring out gpio numbers by the keycodes. This is
> > simply wrong and bad interface.
> > 
> > We want to have a simple way for the application to disable any key by
> > key code. This should be as simple as calling one single
> > "enable/disable" ioctl for /dev/input/event2. I do not see how this
> > could be done nicely with sysfs, still.
> > 
> 
> I understand the appeal of working with the keycodes but what you are
> asking is not simply ignoring certain keypresses (that is easy, just
> ignore corresponding events in userspace), you want to prevent system
> from waking up. In other words you want to "control PM layer through
> input layer" and I believe this is wrong.

Err, this is really about disabling keys. If we think about this in an
abstract way, you have an input device with many keys. And the device's
HW allows you to disable any of it's keys. When you disable the key, the
input device ignores it and does not generate any interrupt for it.

So this is really about ignoring keycodes but on the driver level, not
at the evdev level. We just want to make evdev pass the ioctl down to
the device. If the device does not support it, it'll return an error.
Otherwise the device will just mask out the key on the HW level.

So it is not about playing evil games with PM, it is just about
disabling separate keys on the HW level. We really can look at this that
way.

For me this looks like a good enough abstraction. And probably this may
be useful for other input devices.

I think Mika had some patch which implements this, right?

-- 
Best Regards,
Artem Bityutskiy (Артём Битюцкий)

--
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: [RFC PATCH 1/1] Input: gpio-keys: export gpio key information through sysfs

[Dmitry Torokhov]

On Wed, Nov 11, 2009 at 08:50:11AM +0200, Artem Bityutskiy wrote:
> On Tue, 2009-11-10 at 09:19 -0800, Dmitry Torokhov wrote:
> > On Tue, Nov 10, 2009 at 01:04:19PM +0200, Artem Bityutskiy wrote:
> > > On Mon, 2009-11-09 at 09:18 -0800, Dmitry Torokhov wrote:
> > > > 
> > > > I am not sure at what level these attributes must be created. While the
> > > > easiest way is indeed per-input device attribute (for that particular
> > > > driver) the functionality does not really have anything to do with
> > > > input... I'd probably prefer having this attribute elsewhere, maybe
> > > > we should extend /sys/class/gpio/gpioN for these purposes?
> > > > 
> > > > > * could you please give an example of how would I switch off, say, the
> > > > >   SW_CAMERA_LENS_COVER gpio by the keycode.
> > > > 
> > > > If the code lies in your driver then it is easy (the driver knows the
> > > > mapping); otherwsie you'll have to go with GPIO number.
> > > 
> > > GPIO numbers are bad for us. We want to work with keycodes, and this is
> > > input subsystem after all. We do not want to do any perversions like
> > > making our user-space figuring out gpio numbers by the keycodes. This is
> > > simply wrong and bad interface.
> > > 
> > > We want to have a simple way for the application to disable any key by
> > > key code. This should be as simple as calling one single
> > > "enable/disable" ioctl for /dev/input/event2. I do not see how this
> > > could be done nicely with sysfs, still.
> > > 
> > 
> > I understand the appeal of working with the keycodes but what you are
> > asking is not simply ignoring certain keypresses (that is easy, just
> > ignore corresponding events in userspace), you want to prevent system
> > from waking up. In other words you want to "control PM layer through
> > input layer" and I believe this is wrong.
> 
> Err, this is really about disabling keys. 

If you want to stop delivery of certain keycodes to userspace then OK,
an option to subscrbe to certain events only instead of getting all
events from the device. But again, it will only work for that particular
user only, not everyone.

> If we think about this in an
> abstract way, you have an input device with many keys. And the device's
> HW allows you to disable any of it's keys. When you disable the key, the
> input device ignores it and does not generate any interrupt for it.

And the reason you want to stop interrupts?

-- 
Dmitry

Re: [RFC PATCH 1/1] Input: gpio-keys: export gpio key information through sysfs

[Artem Bityutskiy]

Sorry Dmitry, I'm new to the input subsystem, may be this is why I still
do not see any problem with introducing a nice and useful ioctl?

On Wed, 2009-11-11 at 00:19 -0800, Dmitry Torokhov wrote:
> > > I understand the appeal of working with the keycodes but what you are
> > > asking is not simply ignoring certain keypresses (that is easy, just
> > > ignore corresponding events in userspace), you want to prevent system
> > > from waking up. In other words you want to "control PM layer through
> > > input layer" and I believe this is wrong.
> > 
> > Err, this is really about disabling keys. 
> 
> If you want to stop delivery of certain keycodes to userspace then OK,
> an option to subscrbe to certain events only instead of getting all
> events from the device. But again, it will only work for that particular
> user only, not everyone.

Yes, this will work for some particular input devices only, those which
support the keys masking. What is wrong with this? We have the '-ENOTTY'
error code, which for ioctl's means "ioctl is not supported".

But no one prevents us from making this work for _every_ input device by
implementing a default ioctl handler in evdev.c, if needed. This handler
can mask certain keys in SW.

IOW:
   1. if my HW supports HW key masking, the evdev ioctl calls my
      handler, which is implemented in my driver.
   2. if my HW does not support this, the evdev ioctl masks the keys in
      SW.

We are interested in 1. And if later someone needs 2., he/she can very
easily implement that. But for now we can just return -ENOTTY (or
-EINVAL, which seems to be used in the input subsystem).

> > If we think about this in an
> > abstract way, you have an input device with many keys. And the device's
> > HW allows you to disable any of it's keys. When you disable the key, the
> > input device ignores it and does not generate any interrupt for it.
> 
> And the reason you want to stop interrupts?

The reason is that I want simply mute certain keys. This is the only
reason.

Now the question is _how_ I want to mute the key. My HW supports muting,
so I want to mute it in HW level, just because it is much better and
more efficient. Just because in this case I will not have any interrupt
and my device will wake up less.

-- 
Best Regards,
Artem Bityutskiy (Артём Битюцкий)

--
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: [RFC PATCH 1/1] Input: gpio-keys: export gpio key information through sysfs

[Dmitry Torokhov]

On Wed, Nov 11, 2009 at 10:52:53AM +0200, Artem Bityutskiy wrote:
> Sorry Dmitry, I'm new to the input subsystem, may be this is why I still
> do not see any problem with introducing a nice and useful ioctl?

There are several reasons:

- ioctls are not useful from scripts
- 32/64 bit comapt issues
- currently there islimited number of ioctls in input core, they are all
  concentrated in joydev and evdev, almost all of them work on all
  devices, verifying locking is easy

> 
> On Wed, 2009-11-11 at 00:19 -0800, Dmitry Torokhov wrote:
> > > > I understand the appeal of working with the keycodes but what you are
> > > > asking is not simply ignoring certain keypresses (that is easy, just
> > > > ignore corresponding events in userspace), you want to prevent system
> > > > from waking up. In other words you want to "control PM layer through
> > > > input layer" and I believe this is wrong.
> > > 
> > > Err, this is really about disabling keys. 
> > 
> > If you want to stop delivery of certain keycodes to userspace then OK,
> > an option to subscrbe to certain events only instead of getting all
> > events from the device. But again, it will only work for that particular
> > user only, not everyone.
> 
> Yes, this will work for some particular input devices only, those which
> support the keys masking. What is wrong with this? We have the '-ENOTTY'
> error code, which for ioctl's means "ioctl is not supported".
> 
> But no one prevents us from making this work for _every_ input device by
> implementing a default ioctl handler in evdev.c, if needed. This handler
> can mask certain keys in SW.
> 
> IOW:
>    1. if my HW supports HW key masking, the evdev ioctl calls my
>       handler, which is implemented in my driver.
>    2. if my HW does not support this, the evdev ioctl masks the keys in
>       SW.
> 
> We are interested in 1. And if later someone needs 2., he/she can very
> easily implement that. But for now we can just return -ENOTTY (or
> -EINVAL, which seems to be used in the input subsystem).

And #2 is what I think input layer should provide. Something that
works for all devices and is isolated (one client does not affect
others).

> 
> > > If we think about this in an
> > > abstract way, you have an input device with many keys. And the device's
> > > HW allows you to disable any of it's keys. When you disable the key, the
> > > input device ignores it and does not generate any interrupt for it.
> > 
> > And the reason you want to stop interrupts?
> 
> The reason is that I want simply mute certain keys. This is the only
> reason.
> 
> Now the question is _how_ I want to mute the key. My HW supports muting,
> so I want to mute it in HW level, just because it is much better and
> more efficient. Just because in this case I will not have any interrupt
> and my device will wake up less.

And the reason you care about number of wake ups is not saving power by
any chance?

-- 
Dmitry

Re: [RFC PATCH 1/1] Input: gpio-keys: export gpio key information through sysfs

[Artem Bityutskiy]

On Wed, 2009-11-11 at 01:59 -0800, Dmitry Torokhov wrote:
> On Wed, Nov 11, 2009 at 10:52:53AM +0200, Artem Bityutskiy wrote:
> > Sorry Dmitry, I'm new to the input subsystem, may be this is why I still
> > do not see any problem with introducing a nice and useful ioctl?

I do not insist on ioctl, if you can show a nice way to use sysfs. So
far you did suggest this way, and I also do not see any nice way.

Really, we are happy to use sysfs, just show us how.

> There are several reasons:
> 
> - ioctls are not useful from scripts

Not valid point. There are tons of ioctls all over the place. And no one
prevents us from creating a userspace program which the scripts can use.

And your sysfs proposal is unusable for both the scripts and the C
programs. It is not any better.

> - 32/64 bit comapt issues

Invalid point. Real compat issues exist only for ioctls which pass
pointers. We are not going to do this at all.

> - currently there islimited number of ioctls in input core, they are all
>   concentrated in joydev and evdev, almost all of them work on all
>   devices, verifying locking is easy

OK, but this does not look like a blocker for introducing one more
ioctl.

> > On Wed, 2009-11-11 at 00:19 -0800, Dmitry Torokhov wrote:
> > > > > I understand the appeal of working with the keycodes but what you are
> > > > > asking is not simply ignoring certain keypresses (that is easy, just
> > > > > ignore corresponding events in userspace), you want to prevent system
> > > > > from waking up. In other words you want to "control PM layer through
> > > > > input layer" and I believe this is wrong.
> > > > 
> > > > Err, this is really about disabling keys. 
> > > 
> > > If you want to stop delivery of certain keycodes to userspace then OK,
> > > an option to subscrbe to certain events only instead of getting all
> > > events from the device. But again, it will only work for that particular
> > > user only, not everyone.
> > 
> > Yes, this will work for some particular input devices only, those which
> > support the keys masking. What is wrong with this? We have the '-ENOTTY'
> > error code, which for ioctl's means "ioctl is not supported".
> > 
> > But no one prevents us from making this work for _every_ input device by
> > implementing a default ioctl handler in evdev.c, if needed. This handler
> > can mask certain keys in SW.
> > 
> > IOW:
> >    1. if my HW supports HW key masking, the evdev ioctl calls my
> >       handler, which is implemented in my driver.
> >    2. if my HW does not support this, the evdev ioctl masks the keys in
> >       SW.
> > 
> > We are interested in 1. And if later someone needs 2., he/she can very
> > easily implement that. But for now we can just return -ENOTTY (or
> > -EINVAL, which seems to be used in the input subsystem).
> 
> And #2 is what I think input layer should provide. Something that
> works for all devices and is isolated (one client does not affect
> others).

We can do this, if you believe it normal if we implement code we do not
use and thus, we cannot really test. I do not think it is normal, but if
you insist, we can try doing.

> > > > If we think about this in an
> > > > abstract way, you have an input device with many keys. And the device's
> > > > HW allows you to disable any of it's keys. When you disable the key, the
> > > > input device ignores it and does not generate any interrupt for it.
> > > 
> > > And the reason you want to stop interrupts?
> > 
> > The reason is that I want simply mute certain keys. This is the only
> > reason.
> > 
> > Now the question is _how_ I want to mute the key. My HW supports muting,
> > so I want to mute it in HW level, just because it is much better and
> > more efficient. Just because in this case I will not have any interrupt
> > and my device will wake up less.
> 
> And the reason you care about number of wake ups is not saving power by
> any chance?

Yes it is. But there could be many other reasons as well. E.g., we may
want to achieve smaller latency by lessening amount of interrupts.

Really, you can look at this as we simply want to disable specific keys.
For exampe, for power savings. Or for smaller latency because of less
interrupts. Whatever.

-- 
Best Regards,
Artem Bityutskiy (Артём Битюцкий)

--
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: [RFC PATCH 1/1] Input: gpio-keys: export gpio key information through sysfs

[Artem Bityutskiy]

On Wed, 2009-11-11 at 12:26 +0200, Artem Bityutskiy wrote:
> On Wed, 2009-11-11 at 01:59 -0800, Dmitry Torokhov wrote:
> > On Wed, Nov 11, 2009 at 10:52:53AM +0200, Artem Bityutskiy wrote:
> > > Sorry Dmitry, I'm new to the input subsystem, may be this is why I still
> > > do not see any problem with introducing a nice and useful ioctl?
> 
> I do not insist on ioctl, if you can show a nice way to use sysfs. So
> far you did suggest this way, and I also do not see any nice way.

Sorry, this is unparsable. I wanted to say that so far you did not show
any nice way to go with sysfs.

-- 
Best Regards,
Artem Bityutskiy (Артём Битюцкий)

--
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: [RFC PATCH 1/1] Input: gpio-keys: export gpio key information through sysfs

[Dmitry Torokhov]

On Wed, Nov 11, 2009 at 12:26:32PM +0200, Artem Bityutskiy wrote:
> On Wed, 2009-11-11 at 01:59 -0800, Dmitry Torokhov wrote:
> > On Wed, Nov 11, 2009 at 10:52:53AM +0200, Artem Bityutskiy wrote:
> > > Sorry Dmitry, I'm new to the input subsystem, may be this is why I still
> > > do not see any problem with introducing a nice and useful ioctl?
> 
> I do not insist on ioctl, if you can show a nice way to use sysfs. So
> far you did suggest this way, and I also do not see any nice way.
> 
> Really, we are happy to use sysfs, just show us how.
> 
> > There are several reasons:
> > 
> > - ioctls are not useful from scripts
> 
> Not valid point. There are tons of ioctls all over the place. And no one
> prevents us from creating a userspace program which the scripts can use.
>

Which needs to be packaged and distributed... The nice thing about sysfs
is that echo and cat are always available.

> And your sysfs proposal is unusable for both the scripts and the C
> programs. It is not any better.

It is hard to say at the moment, since we don't have a concrete
proposal for sysfs interface ;)

> 
> > - 32/64 bit comapt issues
> 
> Invalid point. Real compat issues exist only for ioctls which pass
> pointers. We are not going to do this at all.

And longs... and other types that based on longs...

> 
> > - currently there islimited number of ioctls in input core, they are all
> >   concentrated in joydev and evdev, almost all of them work on all
> >   devices, verifying locking is easy
> 
> OK, but this does not look like a blocker for introducing one more
> ioctl.
> 

The dfifference in your ioctl that it leaves implementationto drivers
thus vastly expanding the scope.

> > > On Wed, 2009-11-11 at 00:19 -0800, Dmitry Torokhov wrote:
> > > > > > I understand the appeal of working with the keycodes but what you are
> > > > > > asking is not simply ignoring certain keypresses (that is easy, just
> > > > > > ignore corresponding events in userspace), you want to prevent system
> > > > > > from waking up. In other words you want to "control PM layer through
> > > > > > input layer" and I believe this is wrong.
> > > > > 
> > > > > Err, this is really about disabling keys. 
> > > > 
> > > > If you want to stop delivery of certain keycodes to userspace then OK,
> > > > an option to subscrbe to certain events only instead of getting all
> > > > events from the device. But again, it will only work for that particular
> > > > user only, not everyone.
> > > 
> > > Yes, this will work for some particular input devices only, those which
> > > support the keys masking. What is wrong with this? We have the '-ENOTTY'
> > > error code, which for ioctl's means "ioctl is not supported".
> > > 
> > > But no one prevents us from making this work for _every_ input device by
> > > implementing a default ioctl handler in evdev.c, if needed. This handler
> > > can mask certain keys in SW.
> > > 
> > > IOW:
> > >    1. if my HW supports HW key masking, the evdev ioctl calls my
> > >       handler, which is implemented in my driver.
> > >    2. if my HW does not support this, the evdev ioctl masks the keys in
> > >       SW.
> > > 
> > > We are interested in 1. And if later someone needs 2., he/she can very
> > > easily implement that. But for now we can just return -ENOTTY (or
> > > -EINVAL, which seems to be used in the input subsystem).
> > 
> > And #2 is what I think input layer should provide. Something that
> > works for all devices and is isolated (one client does not affect
> > others).
> 
> We can do this, if you believe it normal if we implement code we do not
> use and thus, we cannot really test. I do not think it is normal, but if
> you insist, we can try doing.

I certainly do not insist that you write code that you are not be using,
far from it. I am just saying that from input layer POV #2 is what makes
most sense.

OK, I can see that limiting everything at evdev layer is not enough for
you and you would prefer disabling interrupt completely. This kind of
fine-grained control is possible I think only in gpio-keys and
gpio-mouse (and maaaaybe matrix_keypad) so I do not think that plumbing
it all the way through evdev with ioctl makes much sense. Let's see if I
can scribble something sysfs-based in a couple of days or if you beat me
to it.

The basic idea is that you create a new 'disabled_keys' and
'disabled_switches' attributes for the gpio-keys platfrom device. They,
for example, may take input in form of list of keys whose interrupts
should be disabled:

133-139,143,147,369-370

See bitmap_scnlistprintf() and bitmap_parselist() for parsing to bitmap
and then you can go through all keys and disable according to said
bitmap. You would need to extend the platform data forst to allow driver
specify whether it wants to sharee interrupt or it is greedy since we
can't disable shared interrupts.

What do you think about it?

-- 
Dmitry

Re: [RFC PATCH 1/1] Input: gpio-keys: export gpio key information through sysfs

[Artem Bityutskiy]

On Wed, 2009-11-11 at 09:40 -0800, Dmitry Torokhov wrote:
> On Wed, Nov 11, 2009 at 12:26:32PM +0200, Artem Bityutskiy wrote:
> > On Wed, 2009-11-11 at 01:59 -0800, Dmitry Torokhov wrote:
> > > On Wed, Nov 11, 2009 at 10:52:53AM +0200, Artem Bityutskiy wrote:
> > > > Sorry Dmitry, I'm new to the input subsystem, may be this is why I still
> > > > do not see any problem with introducing a nice and useful ioctl?
> > 
> > I do not insist on ioctl, if you can show a nice way to use sysfs. So
> > far you did suggest this way, and I also do not see any nice way.
> > 
> > Really, we are happy to use sysfs, just show us how.
> > 
> > > There are several reasons:
> > > 
> > > - ioctls are not useful from scripts
> > 
> > Not valid point. There are tons of ioctls all over the place. And no one
> > prevents us from creating a userspace program which the scripts can use.
> >
> 
> Which needs to be packaged and distributed... The nice thing about sysfs
> is that echo and cat are always available.

The bad thing about sysfs is that it is PITA when it comes to C
programs. Sysfs stuff is slower and more wasteful - e.g., each 'struct
inode' takes kilobytes.

Anyway, I do not think we really want to have a ioctl vs sysfs
discussion, there pluses and minuses, and lkml wars showed that both are
perfectly OK in the linux kernel.

> > And your sysfs proposal is unusable for both the scripts and the C
> > programs. It is not any better.
> 
> It is hard to say at the moment, since we don't have a concrete
> proposal for sysfs interface ;)

Err, OK. I was actually referring to your proposal to use gpio numbers
instead of keycodes.

> > 
> > > - 32/64 bit comapt issues
> > 
> > Invalid point. Real compat issues exist only for ioctls which pass
> > pointers. We are not going to do this at all.
> 
> And longs... and other types that based on longs...

But no one uses longs and pointers in ioctl's nowadays, at least not for
simple things. This is mostly about legacy stuff.

> > > - currently there islimited number of ioctls in input core, they are all
> > >   concentrated in joydev and evdev, almost all of them work on all
> > >   devices, verifying locking is easy
> > 
> > OK, but this does not look like a blocker for introducing one more
> > ioctl.
> > 
> 
> The dfifference in your ioctl that it leaves implementationto drivers
> thus vastly expanding the scope.

Yes, I thought it is just nice general concept - input device can mute
some keys. If it can - you have ioctl, if it cannot - you have -ENOTTY.

> > > > On Wed, 2009-11-11 at 00:19 -0800, Dmitry Torokhov wrote:
> > > > > > > I understand the appeal of working with the keycodes but what you are
> > > > > > > asking is not simply ignoring certain keypresses (that is easy, just
> > > > > > > ignore corresponding events in userspace), you want to prevent system
> > > > > > > from waking up. In other words you want to "control PM layer through
> > > > > > > input layer" and I believe this is wrong.
> > > > > > 
> > > > > > Err, this is really about disabling keys. 
> > > > > 
> > > > > If you want to stop delivery of certain keycodes to userspace then OK,
> > > > > an option to subscrbe to certain events only instead of getting all
> > > > > events from the device. But again, it will only work for that particular
> > > > > user only, not everyone.
> > > > 
> > > > Yes, this will work for some particular input devices only, those which
> > > > support the keys masking. What is wrong with this? We have the '-ENOTTY'
> > > > error code, which for ioctl's means "ioctl is not supported".
> > > > 
> > > > But no one prevents us from making this work for _every_ input device by
> > > > implementing a default ioctl handler in evdev.c, if needed. This handler
> > > > can mask certain keys in SW.
> > > > 
> > > > IOW:
> > > >    1. if my HW supports HW key masking, the evdev ioctl calls my
> > > >       handler, which is implemented in my driver.
> > > >    2. if my HW does not support this, the evdev ioctl masks the keys in
> > > >       SW.
> > > > 
> > > > We are interested in 1. And if later someone needs 2., he/she can very
> > > > easily implement that. But for now we can just return -ENOTTY (or
> > > > -EINVAL, which seems to be used in the input subsystem).
> > > 
> > > And #2 is what I think input layer should provide. Something that
> > > works for all devices and is isolated (one client does not affect
> > > others).
> > 
> > We can do this, if you believe it normal if we implement code we do not
> > use and thus, we cannot really test. I do not think it is normal, but if
> > you insist, we can try doing.
> 
> I certainly do not insist that you write code that you are not be using,
> far from it. I am just saying that from input layer POV #2 is what makes
> most sense.
> 
> OK, I can see that limiting everything at evdev layer is not enough for
> you and you would prefer disabling interrupt completely. This kind of
> fine-grained control is possible I think only in gpio-keys and
> gpio-mouse (and maaaaybe matrix_keypad) so I do not think that plumbing
> it all the way through evdev with ioctl makes much sense. Let's see if I
> can scribble something sysfs-based in a couple of days or if you beat me
> to it.

The list of input drivers may be longer once a new ioctl is there, and
people realize they can mute keys.

Disabling separate key is such a nice and simple concept, that plumbing
it to evdev makes perfect sense for me. And device-specific handlers is
also so standard concept for me.

I feel like we need opinions of other people.

> The basic idea is that you create a new 'disabled_keys' and
> 'disabled_switches' attributes for the gpio-keys platfrom device. They,
> for example, may take input in form of list of keys whose interrupts
> should be disabled:
> 
> 133-139,143,147,369-370
> 
> See bitmap_scnlistprintf() and bitmap_parselist() for parsing to bitmap
> and then you can go through all keys and disable according to said
> bitmap. You would need to extend the platform data forst to allow driver
> specify whether it wants to sharee interrupt or it is greedy since we
> can't disable shared interrupts.
> 
> What do you think about it?

Looks unnecessarily complicated. We can jut try this, and see.

Thanks!

-- 
Best Regards,
Artem Bityutskiy (Артём Битюцкий)

--
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

[PATCH 0/2] Input: gpio-keys: support for disabling GPIOs

[Mika Westerberg]

Hi Dmitry,

This is a third proposal for disabling individual GPIO keys. Now I
implemented this as you suggested. There are now 4 new attributes
under sys/devices/platform/gpio-keys/:
	keys - keys that can be disabled (by keycode)
	switches - switches that can be disabled (by switch code)
	disabled_keys - keys that are currently disabled
	disabled_switches - switches that are currently disabled

I added those 2 attributes (keys and switches) so that userland
can find out what keys/switches are available. Hope this is OK
for you.

These are exported as bitmaps as you suggested (using
bitmap_scnlistprintf() and bitmap_parselist()).

Patches were made against branch 'next'. If these patches get accepted
I'll prepare patch for RX-51 (probably send it to linux-omap ml) where
GPIO buttons don't allow sharing the IRQ.

I tested this with RX-51.

Thanks,
MW

Mika Westerberg (2):
  Input: gpio-keys: allow drivers to specify whether IRQ can be shared
  Input: gpio-keys: added support for disabling gpios through sysfs

 drivers/input/keyboard/gpio_keys.c |  372 +++++++++++++++++++++++++++++++++++-
 include/linux/gpio_keys.h          |    1 +
 2 files changed, 367 insertions(+), 6 deletions(-)

[PATCH 1/2] Input: gpio-keys: allow drivers to specify whether IRQ can be shared

[Mika Westerberg]

Added new field to struct gpio_keys_button: exclusive_irq which can
be used to specify whether driver wants to have exclusive access to
IRQ or can it be shared. By default IRQs for gpio-keys are shareable.

Signed-off-by: Mika Westerberg <ext-mika.1.westerberg@nokia.com>
---
 drivers/input/keyboard/gpio_keys.c |   14 ++++++++++----
 include/linux/gpio_keys.h          |    1 +
 2 files changed, 11 insertions(+), 4 deletions(-)

diff --git a/drivers/input/keyboard/gpio_keys.c b/drivers/input/keyboard/gpio_keys.c
index 8941a8b..113d187 100644
--- a/drivers/input/keyboard/gpio_keys.c
+++ b/drivers/input/keyboard/gpio_keys.c
@@ -78,6 +78,7 @@ static int __devinit gpio_keys_setup_key(struct device *dev,
 					 struct gpio_keys_button *button)
 {
 	char *desc = button->desc ? button->desc : "gpio_keys";
+	unsigned long irqflags;
 	int irq, error;
 
 	setup_timer(&bdata->timer, gpio_keys_timer, (unsigned long)bdata);
@@ -106,10 +107,15 @@ static int __devinit gpio_keys_setup_key(struct device *dev,
 		goto fail3;
 	}
 
-	error = request_irq(irq, gpio_keys_isr,
-			    IRQF_SHARED |
-			    IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING,
-			    desc, bdata);
+	irqflags = IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING;
+	/*
+	 * If not explicitly specified in platform data, we allow
+	 * IRQs to be shared with some other device.
+	 */
+	if (!button->exclusive_irq)
+		irqflags |= IRQF_SHARED;
+
+	error = request_irq(irq, gpio_keys_isr, irqflags, desc, bdata);
 	if (error) {
 		dev_err(dev, "Unable to claim irq %d; error %d\n",
 			irq, error);
diff --git a/include/linux/gpio_keys.h b/include/linux/gpio_keys.h
index 1289fa7..82639c7 100644
--- a/include/linux/gpio_keys.h
+++ b/include/linux/gpio_keys.h
@@ -10,6 +10,7 @@ struct gpio_keys_button {
 	int type;		/* input event type (EV_KEY, EV_SW) */
 	int wakeup;		/* configure the button as a wake-up source */
 	int debounce_interval;	/* debounce ticks interval in msecs */
+	int exclusive_irq;	/* don't share the irq */
 };
 
 struct gpio_keys_platform_data {
-- 
1.5.6.5

[PATCH 2/2] Input: gpio-keys: added support for disabling gpios through sysfs

[Mika Westerberg]

Now gpio-keys input driver exports 4 new attributes to userland through
sysfs:
	/sys/devices/platform/gpio-keys/keys [ro]
	/sys/devices/platform/gpio-keys/switches [ro]
	/sys/devices/platform/gpio-keys/disabled_keys [rw]
	/sys/devices/platform/gpio-keys/disables_switches [rw]

With these attributes, userland program can read which keys and
switches can be disabled and then disable/enable them as needed.
Keys and switches are exported as stringified bitmap of codes
(keycodes or switch codes). For example keys 15, 89, 100, 101,
102 are exported as: '15,89,100-102'.

Description of the attributes:
	keys - bitmap of keys which can be disabled
	switches - bitmap of switches which can be disabled
	disabled_keys - bitmap of currently disabled keys
	                (bit 1 means disabled, 0 enabled)
	disabled_switches - bitmap of currently disabled switches
	                (bit 1 means disabled, 0 enabled)

Signed-off-by: Mika Westerberg <ext-mika.1.westerberg@nokia.com>
---
 drivers/input/keyboard/gpio_keys.c |  358 +++++++++++++++++++++++++++++++++++-
 1 files changed, 356 insertions(+), 2 deletions(-)

diff --git a/drivers/input/keyboard/gpio_keys.c b/drivers/input/keyboard/gpio_keys.c
index 113d187..41dcc9e 100644
--- a/drivers/input/keyboard/gpio_keys.c
+++ b/drivers/input/keyboard/gpio_keys.c
@@ -14,6 +14,7 @@
 #include <linux/fs.h>
 #include <linux/interrupt.h>
 #include <linux/irq.h>
+#include <linux/list.h>
 #include <linux/sched.h>
 #include <linux/pm.h>
 #include <linux/sysctl.h>
@@ -30,13 +31,336 @@ struct gpio_button_data {
 	struct input_dev *input;
 	struct timer_list timer;
 	struct work_struct work;
+	struct list_head node;
+	/* protects button from concurrent disables/enables */
+	struct mutex disable_lock;
+	bool disabled;
 };
 
 struct gpio_keys_drvdata {
 	struct input_dev *input;
+	/*
+	 * NOTE: currently these lists are filled in when module is
+	 *       initialized and never changed after that. This means
+	 *       that no locking is needed when accessing these. If
+	 *       this changes then we obviously need to protect these
+	 *       from concurrent accesses.
+	 */
+	struct list_head keys;     /* list of keys that can be disabled */
+	struct list_head switches; /* list of switches that can be disabled */
 	struct gpio_button_data data[0];
 };
 
+/*
+ * SYSFS interface for enabling/disabling keys and switches:
+ *
+ * There are 4 attributes under /sys/devices/platform/gpio-keys/
+ * 	keys [ro]              - bitmap of keys (EV_KEY) which can be
+ *	                         disabled
+ *	switches [ro]          - bitmap of switches (EV_SW) which can be
+ *	                         disabled
+ *	disabled_keys [rw]     - bitmap of keys currently disabled
+ *	disabled_switches [rw] - bitmap of switches currently disabled
+ *
+ * Userland can change these values and hence disable event generation
+ * for each key (or switch). Disabling a key means its interrupt line
+ * is disabled.
+ *
+ * For example, if we have following switches set up as gpio-keys:
+ *	SW_DOCK = 5
+ *	SW_CAMERA_LENS_COVER = 9
+ *	SW_KEYPAD_SLIDE = 10
+ * 	SW_FRONT_PROXIMITY = 11
+ * This is read from switches:
+ *	11-9,5
+ * Next we want to disable proximity (11) and dock (5), we write:
+ *	11,5
+ * to file disabled_switches. Now proximity and dock IRQs are disabled.
+ * This can be verified by reading the file disabled_switches:
+ *	11,5
+ * If we now want to enable proximity (11) switch we write:
+ *	5
+ * to disabled_switches.
+ *
+ * Note that we allow user to specify other bits that are allowed (as
+ * long as she doesn't try to use bits higher than KEY_CNT or SW_CNT).
+ * User can disable bunch of keys for example like this:
+ *	echo 0-600 > disabled_keys
+ * and we only disable those keys that can be disabled.
+ *
+ * We can disable only those keys who have specified 'exclusive_irq = 1'
+ * in their platform data (e.g shared IRQs cannot be disabled).
+ */
+
+/**
+ * get_nkeys_by_type() - returns maximum number of keys per @type
+ * @type: type of button (%EV_KEY, %EV_SW)
+ *
+ * Return value of this function can be used to allocate bitmap
+ * large enough to hold all bits for given type.
+ */
+static inline int get_nkeys_by_type(int type)
+{
+	BUG_ON(type != EV_SW && type != EV_KEY);
+
+	return (type == EV_KEY) ? KEY_CNT : SW_CNT;
+}
+
+/**
+ * get_button_list_by_type() - returns correct button list per @type
+ * @ddata: driver data containing the lists
+ * @type: type of button which list we want (%EV_KEY, %EV_SW)
+ */
+static inline struct list_head *get_button_list_by_type(
+	struct gpio_keys_drvdata *ddata, int type)
+{
+	BUG_ON(type != EV_SW && type != EV_KEY);
+
+	return (type == EV_KEY) ? &ddata->keys : &ddata->switches;
+}
+
+/**
+ * gpio_keys_disable_button() - disables given GPIO button
+ * @bdata: button data for button to be disabled
+ *
+ * Disables button pointed by @bdata. This is done by masking
+ * IRQ line. After this function is called, button won't generate
+ * input events anymore. Note that one can only disable buttons
+ * that don't share IRQs.
+ *
+ * Make sure that @bdata->disable_lock is locked when entering
+ * this function to avoid races when concurrent threads are
+ * disabling buttons at the same time.
+ */
+static void gpio_keys_disable_button(struct gpio_button_data *bdata)
+{
+	BUG_ON(bdata->button->exclusive_irq == 0);
+
+	if (bdata->disabled)
+		return;
+
+	/*
+	 * Disable IRQ and possible debouncing timer.
+	 */
+	disable_irq(gpio_to_irq(bdata->button->gpio));
+	if (bdata->button->debounce_interval)
+		(void) del_timer_sync(&bdata->timer);
+
+	bdata->disabled = true;
+}
+
+/**
+ * gpio_keys_enable_button() - enables given GPIO button
+ * @bdata: button data for button to be disabled
+ *
+ * Enables given button pointed by @bdata.
+ *
+ * Make sure that @bdata->disable_lock is locked when entering
+ * this function to avoid races with concurrent threads trying
+ * to enable the same button at the same time.
+ */
+static void gpio_keys_enable_button(struct gpio_button_data *bdata)
+{
+	if (!bdata->disabled)
+		return;
+
+	enable_irq(gpio_to_irq(bdata->button->gpio));
+	bdata->disabled = false;
+}
+
+/**
+ * gpio_keys_attr_show_helper() - fill in stringified bitmap of buttons
+ * @ddata: pointer to drvdata
+ * @buf: buffer where stringified bitmap is written
+ * @type: button type (%EV_KEY, %EV_SW)
+ * @only_disabled: does caller want only those buttons that are
+ *                 currently disabled or all buttons that can be
+ *                 disabled
+ *
+ * This function writes buttons that can be disabled to @buf. If
+ * @only_disabled is true, then @buf contains only those buttons
+ * that are currently disabled. Returns 0 on success or negative
+ * errno on failure.
+ */
+static ssize_t gpio_keys_attr_show_helper(struct gpio_keys_drvdata *ddata,
+					  char *buf, int type,
+					  bool only_disabled)
+{
+	struct list_head *button_list, *pbtn;
+	unsigned long *bits;
+	ssize_t ret;
+	int nkeys;
+
+	nkeys = get_nkeys_by_type(type);
+	bits = kzalloc(sizeof(*bits) * BITS_TO_LONGS(nkeys), GFP_KERNEL);
+	if (!bits)
+		return -ENOMEM;
+
+	button_list = get_button_list_by_type(ddata, type);
+	list_for_each(pbtn, button_list) {
+		struct gpio_button_data *bdata;
+
+		bdata = list_entry(pbtn, struct gpio_button_data, node);
+		if (only_disabled && !bdata->disabled)
+			continue;
+		set_bit(bdata->button->code, bits);
+	}
+
+	ret = bitmap_scnlistprintf(buf, PAGE_SIZE - 1, bits, nkeys);
+	buf[ret++] = '\n';
+	kfree(bits);
+
+	return ret;
+}
+
+/**
+ * gpio_keys_attr_store_helper() - enable/disable buttons based on given bitmap
+ * @ddata: pointer to drvdata
+ * @buf: buffer from userspace that contains stringified bitmap
+ * @type: button type (%EV_KEY, %EV_SW)
+ *
+ * This function parses stringified bitmap from @buf and disables/enables
+ * GPIO buttons accordinly. Returns 0 on success and negative error
+ * on failure.
+ */
+static ssize_t gpio_keys_attr_store_helper(struct gpio_keys_drvdata *ddata,
+					   const char *buf, int type)
+{
+	struct list_head *button_list, *pbtn;
+	unsigned long *bits;
+	ssize_t error;
+	int nkeys;
+
+	nkeys = get_nkeys_by_type(type);
+	bits = kzalloc(sizeof(*bits) * BITS_TO_LONGS(nkeys), GFP_KERNEL);
+	if (!bits)
+		return -ENOMEM;
+
+	error = bitmap_parselist(buf, bits, nkeys);
+	if (error)
+		goto out;
+
+	button_list = get_button_list_by_type(ddata, type);
+	list_for_each(pbtn, button_list) {
+		struct gpio_button_data *bdata;
+		int bit;
+
+		bdata = list_entry(pbtn, struct gpio_button_data, node);
+		bit = test_bit(bdata->button->code, bits);
+
+		mutex_lock(&bdata->disable_lock);
+
+		if (bdata->disabled && !bit)
+			gpio_keys_enable_button(bdata);
+		else if (!bdata->disabled && bit)
+			gpio_keys_disable_button(bdata);
+
+		mutex_unlock(&bdata->disable_lock);
+	}
+
+out:
+	kfree(bits);
+	return error;
+}
+
+#define ATTR_SHOW_FN(name, type, only_disabled)				\
+static ssize_t gpio_keys_show_##name(struct device *dev,		\
+			      struct device_attribute *attr,		\
+			      char *buf)				\
+{									\
+	struct platform_device *pdev = to_platform_device(dev);		\
+	struct gpio_keys_drvdata *ddata = platform_get_drvdata(pdev);	\
+									\
+	(void)attr;							\
+	return gpio_keys_attr_show_helper(ddata, buf, (type),		\
+					  (only_disabled));		\
+}
+ATTR_SHOW_FN(keys, EV_KEY, false);
+ATTR_SHOW_FN(switches, EV_SW, false);
+ATTR_SHOW_FN(disabled_keys, EV_KEY, true);
+ATTR_SHOW_FN(disabled_switches, EV_SW, true);
+
+/*
+ * ATTRIBUTES:
+ *
+ * /sys/devices/platform/gpio-keys/keys [ro]
+ * /sys/devices/platform/gpio-keys/switches [ro]
+ */
+static DEVICE_ATTR(keys, S_IRUGO, gpio_keys_show_keys, NULL);
+static DEVICE_ATTR(switches, S_IRUGO, gpio_keys_show_switches, NULL);
+
+#define ATTR_STORE_FN(name, type)					\
+static ssize_t gpio_keys_store_##name(struct device *dev,		\
+				      struct device_attribute *attr,	\
+				      const char *buf,			\
+				      size_t count)			\
+{									\
+	struct platform_device *pdev = to_platform_device(dev);		\
+	struct gpio_keys_drvdata *ddata = platform_get_drvdata(pdev);	\
+	ssize_t ret;							\
+									\
+	(void)attr;							\
+	ret = gpio_keys_attr_store_helper(ddata, buf, (type));		\
+	if (ret)							\
+		return ret;						\
+	return count;							\
+}
+ATTR_STORE_FN(disabled_keys, EV_KEY);
+ATTR_STORE_FN(disabled_switches, EV_SW);
+
+/*
+ * ATTRIBUTES:
+ *
+ * /sys/devices/platform/gpio-keys/disabled_keys [rw]
+ * /sys/devices/platform/gpio-keys/disables_switches [rw]
+ */
+static DEVICE_ATTR(disabled_keys, S_IWUSR | S_IRUGO,
+		   gpio_keys_show_disabled_keys,
+		   gpio_keys_store_disabled_keys);
+static DEVICE_ATTR(disabled_switches, S_IWUSR | S_IRUGO,
+		   gpio_keys_show_disabled_switches,
+		   gpio_keys_store_disabled_switches);
+
+static struct attribute *gpio_keys_attrs[] = {
+	&dev_attr_keys.attr,
+	&dev_attr_switches.attr,
+	&dev_attr_disabled_keys.attr,
+	&dev_attr_disabled_switches.attr,
+	NULL,
+};
+
+static struct attribute_group gpio_keys_attr_group = {
+	.attrs = gpio_keys_attrs,
+};
+
+/**
+ * gpio_keys_export - export gpio key/switch information through sysfs
+ * @pdev: platform_device whose keys/switches are exported
+ *
+ * This function creates 4 new attributes under gpio-keys sysfs
+ * entry: keys, switches, disabled_keys and disabled_switches.
+ * Userland can use these files to enable/disable gpios by
+ * specifying keycode or switchcode.
+ *
+ * Returns 0 on success and negative error on failure.
+ */
+static int gpio_keys_export(struct platform_device *pdev)
+{
+	return sysfs_create_group(&pdev->dev.kobj, &gpio_keys_attr_group);
+}
+
+/**
+ * gpio_keys_unexport - unexports gpio key/switch information
+ * @pdev: platform_device whose keys/switches are unexported
+ *
+ * This function removes exported gpio keys sysfs attributes from
+ * sysfs.
+ */
+static void gpio_keys_unexport(struct platform_device *pdev)
+{
+	sysfs_remove_group(&pdev->dev.kobj, &gpio_keys_attr_group);
+}
+
 static void gpio_keys_report_event(struct work_struct *work)
 {
 	struct gpio_button_data *bdata =
@@ -73,16 +397,19 @@ static irqreturn_t gpio_keys_isr(int irq, void *dev_id)
 	return IRQ_HANDLED;
 }
 
-static int __devinit gpio_keys_setup_key(struct device *dev,
+static int __devinit gpio_keys_setup_key(struct platform_device *pdev,
 					 struct gpio_button_data *bdata,
 					 struct gpio_keys_button *button)
 {
+	struct gpio_keys_drvdata *ddata = platform_get_drvdata(pdev);
 	char *desc = button->desc ? button->desc : "gpio_keys";
+	struct device *dev = &pdev->dev;
 	unsigned long irqflags;
 	int irq, error;
 
 	setup_timer(&bdata->timer, gpio_keys_timer, (unsigned long)bdata);
 	INIT_WORK(&bdata->work, gpio_keys_report_event);
+	mutex_init(&bdata->disable_lock);
 
 	error = gpio_request(button->gpio, desc);
 	if (error < 0) {
@@ -114,6 +441,21 @@ static int __devinit gpio_keys_setup_key(struct device *dev,
 	 */
 	if (!button->exclusive_irq)
 		irqflags |= IRQF_SHARED;
+	else {
+		/*
+		 * GPIOs which don't share their IRQ can be disabled
+		 * so we add them to appropriate lists depending
+		 * on type.
+		 */
+		switch (button->type) {
+		case EV_KEY:
+			list_add_tail(&bdata->node, &ddata->keys);
+			break;
+		case EV_SW:
+			list_add_tail(&bdata->node, &ddata->switches);
+			break;
+		}
+	}
 
 	error = request_irq(irq, gpio_keys_isr, irqflags, desc, bdata);
 	if (error) {
@@ -149,6 +491,9 @@ static int __devinit gpio_keys_probe(struct platform_device *pdev)
 		goto fail1;
 	}
 
+	INIT_LIST_HEAD(&ddata->keys);
+	INIT_LIST_HEAD(&ddata->switches);
+
 	platform_set_drvdata(pdev, ddata);
 
 	input->name = pdev->name;
@@ -174,7 +519,7 @@ static int __devinit gpio_keys_probe(struct platform_device *pdev)
 		bdata->input = input;
 		bdata->button = button;
 
-		error = gpio_keys_setup_key(dev, bdata, button);
+		error = gpio_keys_setup_key(pdev, bdata, button);
 		if (error)
 			goto fail2;
 
@@ -191,6 +536,13 @@ static int __devinit gpio_keys_probe(struct platform_device *pdev)
 		goto fail2;
 	}
 
+	error = gpio_keys_export(pdev);
+	if (error) {
+		pr_err("gpio-keys: Unable to export keys/switches, "
+			"error: %d\n", error);
+		goto fail2;
+	}
+
 	device_init_wakeup(&pdev->dev, wakeup);
 
 	return 0;
@@ -205,6 +557,7 @@ static int __devinit gpio_keys_probe(struct platform_device *pdev)
 	}
 
 	platform_set_drvdata(pdev, NULL);
+	gpio_keys_unexport(pdev);
  fail1:
 	input_free_device(input);
 	kfree(ddata);
@@ -219,6 +572,7 @@ static int __devexit gpio_keys_remove(struct platform_device *pdev)
 	struct input_dev *input = ddata->input;
 	int i;
 
+	gpio_keys_unexport(pdev);
 	device_init_wakeup(&pdev->dev, 0);
 
 	for (i = 0; i < pdata->nbuttons; i++) {
-- 
1.5.6.5

Re: [PATCH 2/2] Input: gpio-keys: added support for disabling gpios through sysfs

[Dmitry Torokhov]

Hi Mika,

On Thu, Nov 19, 2009 at 09:23:46AM +0200, Mika Westerberg wrote:
>  
>  struct gpio_keys_drvdata {
>  	struct input_dev *input;
> +	/*
> +	 * NOTE: currently these lists are filled in when module is
> +	 *       initialized and never changed after that. This means
> +	 *       that no locking is needed when accessing these. If
> +	 *       this changes then we obviously need to protect these
> +	 *       from concurrent accesses.
> +	 */
> +	struct list_head keys;     /* list of keys that can be disabled */
> +	struct list_head switches; /* list of switches that can be disabled */
>  	struct gpio_button_data data[0];
>  };
>  

Why do you need the lists? Does the version of patch below still work
for you?

-- 
Dmitry


Input: gpio-keys - added support for disabling gpios through sysfs

From: Mika Westerberg <ext-mika.1.westerberg@nokia.com>

Now gpio-keys input driver exports 4 new attributes to userland through
sysfs:
	/sys/devices/platform/gpio-keys/keys [ro]
	/sys/devices/platform/gpio-keys/switches [ro]
	/sys/devices/platform/gpio-keys/disabled_keys [rw]
	/sys/devices/platform/gpio-keys/disables_switches [rw]

With these attributes, userland program can read which keys and
switches can be disabled and then disable/enable them as needed.
Keys and switches are exported as stringified bitmap of codes
(keycodes or switch codes). For example keys 15, 89, 100, 101,
102 are exported as: '15,89,100-102'.

Description of the attributes:
	keys - bitmap of keys which can be disabled
	switches - bitmap of switches which can be disabled
	disabled_keys - bitmap of currently disabled keys
	                (bit 1 means disabled, 0 enabled)
	disabled_switches - bitmap of currently disabled switches
	                (bit 1 means disabled, 0 enabled)

Signed-off-by: Mika Westerberg <ext-mika.1.westerberg@nokia.com>
Signed-off-by: Dmitry Torokhov <dtor@mail.ru>
---

 drivers/input/keyboard/gpio_keys.c |  318 +++++++++++++++++++++++++++++++++++-
 1 files changed, 311 insertions(+), 7 deletions(-)


diff --git a/drivers/input/keyboard/gpio_keys.c b/drivers/input/keyboard/gpio_keys.c
index 113d187..3e534db 100644
--- a/drivers/input/keyboard/gpio_keys.c
+++ b/drivers/input/keyboard/gpio_keys.c
@@ -14,6 +14,7 @@
 #include <linux/fs.h>
 #include <linux/interrupt.h>
 #include <linux/irq.h>
+#include <linux/list.h>
 #include <linux/sched.h>
 #include <linux/pm.h>
 #include <linux/sysctl.h>
@@ -30,13 +31,299 @@ struct gpio_button_data {
 	struct input_dev *input;
 	struct timer_list timer;
 	struct work_struct work;
+	bool can_disable;
+	bool disabled;
 };
 
 struct gpio_keys_drvdata {
 	struct input_dev *input;
+	struct mutex disable_lock;
+	unsigned int n_buttons;
 	struct gpio_button_data data[0];
 };
 
+/*
+ * SYSFS interface for enabling/disabling keys and switches:
+ *
+ * There are 4 attributes under /sys/devices/platform/gpio-keys/
+ *	keys [ro]              - bitmap of keys (EV_KEY) which can be
+ *	                         disabled
+ *	switches [ro]          - bitmap of switches (EV_SW) which can be
+ *	                         disabled
+ *	disabled_keys [rw]     - bitmap of keys currently disabled
+ *	disabled_switches [rw] - bitmap of switches currently disabled
+ *
+ * Userland can change these values and hence disable event generation
+ * for each key (or switch). Disabling a key means its interrupt line
+ * is disabled.
+ *
+ * For example, if we have following switches set up as gpio-keys:
+ *	SW_DOCK = 5
+ *	SW_CAMERA_LENS_COVER = 9
+ *	SW_KEYPAD_SLIDE = 10
+ *	SW_FRONT_PROXIMITY = 11
+ * This is read from switches:
+ *	11-9,5
+ * Next we want to disable proximity (11) and dock (5), we write:
+ *	11,5
+ * to file disabled_switches. Now proximity and dock IRQs are disabled.
+ * This can be verified by reading the file disabled_switches:
+ *	11,5
+ * If we now want to enable proximity (11) switch we write:
+ *	5
+ * to disabled_switches.
+ *
+ * Note that we allow user to specify other bits that are allowed (as
+ * long as she doesn't try to use bits higher than KEY_CNT or SW_CNT).
+ * User can disable bunch of keys for example like this:
+ *	echo 0-600 > disabled_keys
+ * and we only disable those keys that can be disabled.
+ *
+ * We can disable only those keys who have specified 'exclusive_irq = 1'
+ * in their platform data (e.g shared IRQs cannot be disabled).
+ */
+
+/**
+ * get_n_events_by_type() - returns maximum number of events per @type
+ * @type: type of button (%EV_KEY, %EV_SW)
+ *
+ * Return value of this function can be used to allocate bitmap
+ * large enough to hold all bits for given type.
+ */
+static inline int get_n_events_by_type(int type)
+{
+	BUG_ON(type != EV_SW && type != EV_KEY);
+
+	return (type == EV_KEY) ? KEY_CNT : SW_CNT;
+}
+
+/**
+ * gpio_keys_disable_button() - disables given GPIO button
+ * @bdata: button data for button to be disabled
+ *
+ * Disables button pointed by @bdata. This is done by masking
+ * IRQ line. After this function is called, button won't generate
+ * input events anymore. Note that one can only disable buttons
+ * that don't share IRQs.
+ *
+ * Make sure that @bdata->disable_lock is locked when entering
+ * this function to avoid races when concurrent threads are
+ * disabling buttons at the same time.
+ */
+static void gpio_keys_disable_button(struct gpio_button_data *bdata)
+{
+	BUG_ON(bdata->button->exclusive_irq == 0);
+
+	if (!bdata->disabled) {
+		/*
+		 * Disable IRQ and possible debouncing timer.
+		 */
+		disable_irq(gpio_to_irq(bdata->button->gpio));
+		if (bdata->button->debounce_interval)
+			del_timer_sync(&bdata->timer);
+
+		bdata->disabled = true;
+	}
+}
+
+/**
+ * gpio_keys_enable_button() - enables given GPIO button
+ * @bdata: button data for button to be disabled
+ *
+ * Enables given button pointed by @bdata.
+ *
+ * Make sure that @bdata->disable_lock is locked when entering
+ * this function to avoid races with concurrent threads trying
+ * to enable the same button at the same time.
+ */
+static void gpio_keys_enable_button(struct gpio_button_data *bdata)
+{
+	if (bdata->disabled) {
+		enable_irq(gpio_to_irq(bdata->button->gpio));
+		bdata->disabled = false;
+	}
+}
+
+/**
+ * gpio_keys_attr_show_helper() - fill in stringified bitmap of buttons
+ * @ddata: pointer to drvdata
+ * @buf: buffer where stringified bitmap is written
+ * @type: button type (%EV_KEY, %EV_SW)
+ * @only_disabled: does caller want only those buttons that are
+ *                 currently disabled or all buttons that can be
+ *                 disabled
+ *
+ * This function writes buttons that can be disabled to @buf. If
+ * @only_disabled is true, then @buf contains only those buttons
+ * that are currently disabled. Returns 0 on success or negative
+ * errno on failure.
+ */
+static ssize_t gpio_keys_attr_show_helper(struct gpio_keys_drvdata *ddata,
+					  char *buf, unsigned int type,
+					  bool only_disabled)
+{
+	int n_events = get_n_events_by_type(type);
+	unsigned long *bits;
+	ssize_t ret;
+	int i;
+
+	bits = kcalloc(BITS_TO_LONGS(n_events), sizeof(*bits), GFP_KERNEL);
+	if (!bits)
+		return -ENOMEM;
+
+	for (i = 0; i < ddata->n_buttons; i++) {
+		struct gpio_button_data *bdata = &ddata->data[i];
+
+		if (bdata->button->type != type)
+			continue;
+
+		if (only_disabled && !bdata->disabled)
+			continue;
+
+		__set_bit(bdata->button->code, bits);
+	}
+
+	ret = bitmap_scnlistprintf(buf, PAGE_SIZE - 2, bits, n_events);
+	buf[ret++] = '\n';
+	buf[ret] = '\0';
+
+	kfree(bits);
+
+	return ret;
+}
+
+/**
+ * gpio_keys_attr_store_helper() - enable/disable buttons based on given bitmap
+ * @ddata: pointer to drvdata
+ * @buf: buffer from userspace that contains stringified bitmap
+ * @type: button type (%EV_KEY, %EV_SW)
+ *
+ * This function parses stringified bitmap from @buf and disables/enables
+ * GPIO buttons accordinly. Returns 0 on success and negative error
+ * on failure.
+ */
+static ssize_t gpio_keys_attr_store_helper(struct gpio_keys_drvdata *ddata,
+					   const char *buf, unsigned int type)
+{
+	int n_events = get_n_events_by_type(type);
+	unsigned long *bits;
+	ssize_t error;
+	int i;
+
+	bits = kcalloc(BITS_TO_LONGS(n_events), sizeof(*bits), GFP_KERNEL);
+	if (!bits)
+		return -ENOMEM;
+
+	error = bitmap_parselist(buf, bits, n_events);
+	if (error)
+		goto out;
+
+	/* First validate */
+	for (i = 0; i < ddata->n_buttons; i++) {
+		struct gpio_button_data *bdata = &ddata->data[i];
+
+		if (bdata->button->type != type)
+			continue;
+
+		if (test_bit(bdata->button->code, bits) &&
+		    !bdata->can_disable) {
+			error = -EINVAL;
+			goto out;
+		}
+	}
+
+	mutex_lock(&ddata->disable_lock);
+
+	for (i = 0; i < ddata->n_buttons; i++) {
+		struct gpio_button_data *bdata = &ddata->data[i];
+
+		if (bdata->button->type != type)
+			continue;
+
+		if (test_bit(bdata->button->code, bits))
+			gpio_keys_disable_button(bdata);
+		else
+			gpio_keys_enable_button(bdata);
+	}
+
+	mutex_unlock(&ddata->disable_lock);
+
+out:
+	kfree(bits);
+	return error;
+}
+
+#define ATTR_SHOW_FN(name, type, only_disabled)				\
+static ssize_t gpio_keys_show_##name(struct device *dev,		\
+				     struct device_attribute *attr,	\
+				     char *buf)				\
+{									\
+	struct platform_device *pdev = to_platform_device(dev);		\
+	struct gpio_keys_drvdata *ddata = platform_get_drvdata(pdev);	\
+									\
+	return gpio_keys_attr_show_helper(ddata, buf,			\
+					  type, only_disabled);		\
+}
+
+ATTR_SHOW_FN(keys, EV_KEY, false);
+ATTR_SHOW_FN(switches, EV_SW, false);
+ATTR_SHOW_FN(disabled_keys, EV_KEY, true);
+ATTR_SHOW_FN(disabled_switches, EV_SW, true);
+
+/*
+ * ATTRIBUTES:
+ *
+ * /sys/devices/platform/gpio-keys/keys [ro]
+ * /sys/devices/platform/gpio-keys/switches [ro]
+ */
+static DEVICE_ATTR(keys, S_IRUGO, gpio_keys_show_keys, NULL);
+static DEVICE_ATTR(switches, S_IRUGO, gpio_keys_show_switches, NULL);
+
+#define ATTR_STORE_FN(name, type)					\
+static ssize_t gpio_keys_store_##name(struct device *dev,		\
+				      struct device_attribute *attr,	\
+				      const char *buf,			\
+				      size_t count)			\
+{									\
+	struct platform_device *pdev = to_platform_device(dev);		\
+	struct gpio_keys_drvdata *ddata = platform_get_drvdata(pdev);	\
+	ssize_t error;							\
+									\
+	error = gpio_keys_attr_store_helper(ddata, buf, type);		\
+	if (error)							\
+		return error;						\
+									\
+	return count;							\
+}
+
+ATTR_STORE_FN(disabled_keys, EV_KEY);
+ATTR_STORE_FN(disabled_switches, EV_SW);
+
+/*
+ * ATTRIBUTES:
+ *
+ * /sys/devices/platform/gpio-keys/disabled_keys [rw]
+ * /sys/devices/platform/gpio-keys/disables_switches [rw]
+ */
+static DEVICE_ATTR(disabled_keys, S_IWUSR | S_IRUGO,
+		   gpio_keys_show_disabled_keys,
+		   gpio_keys_store_disabled_keys);
+static DEVICE_ATTR(disabled_switches, S_IWUSR | S_IRUGO,
+		   gpio_keys_show_disabled_switches,
+		   gpio_keys_store_disabled_switches);
+
+static struct attribute *gpio_keys_attrs[] = {
+	&dev_attr_keys.attr,
+	&dev_attr_switches.attr,
+	&dev_attr_disabled_keys.attr,
+	&dev_attr_disabled_switches.attr,
+	NULL,
+};
+
+static struct attribute_group gpio_keys_attr_group = {
+	.attrs = gpio_keys_attrs,
+};
+
 static void gpio_keys_report_event(struct work_struct *work)
 {
 	struct gpio_button_data *bdata =
@@ -73,11 +360,12 @@ static irqreturn_t gpio_keys_isr(int irq, void *dev_id)
 	return IRQ_HANDLED;
 }
 
-static int __devinit gpio_keys_setup_key(struct device *dev,
+static int __devinit gpio_keys_setup_key(struct platform_device *pdev,
 					 struct gpio_button_data *bdata,
 					 struct gpio_keys_button *button)
 {
 	char *desc = button->desc ? button->desc : "gpio_keys";
+	struct device *dev = &pdev->dev;
 	unsigned long irqflags;
 	int irq, error;
 
@@ -114,6 +402,8 @@ static int __devinit gpio_keys_setup_key(struct device *dev,
 	 */
 	if (!button->exclusive_irq)
 		irqflags |= IRQF_SHARED;
+	else
+		bdata->can_disable = true;
 
 	error = request_irq(irq, gpio_keys_isr, irqflags, desc, bdata);
 	if (error) {
@@ -149,6 +439,10 @@ static int __devinit gpio_keys_probe(struct platform_device *pdev)
 		goto fail1;
 	}
 
+	ddata->input = input;
+	ddata->n_buttons = pdata->nbuttons;
+	mutex_init(&ddata->disable_lock);
+
 	platform_set_drvdata(pdev, ddata);
 
 	input->name = pdev->name;
@@ -164,8 +458,6 @@ static int __devinit gpio_keys_probe(struct platform_device *pdev)
 	if (pdata->rep)
 		__set_bit(EV_REP, input->evbit);
 
-	ddata->input = input;
-
 	for (i = 0; i < pdata->nbuttons; i++) {
 		struct gpio_keys_button *button = &pdata->buttons[i];
 		struct gpio_button_data *bdata = &ddata->data[i];
@@ -174,7 +466,7 @@ static int __devinit gpio_keys_probe(struct platform_device *pdev)
 		bdata->input = input;
 		bdata->button = button;
 
-		error = gpio_keys_setup_key(dev, bdata, button);
+		error = gpio_keys_setup_key(pdev, bdata, button);
 		if (error)
 			goto fail2;
 
@@ -184,17 +476,26 @@ static int __devinit gpio_keys_probe(struct platform_device *pdev)
 		input_set_capability(input, type, button->code);
 	}
 
-	error = input_register_device(input);
+	error = sysfs_create_group(&pdev->dev.kobj, &gpio_keys_attr_group);
 	if (error) {
-		dev_err(dev, "Unable to register input device, "
-			"error: %d\n", error);
+		dev_err(dev, "Unable to export keys/switches, error: %d\n",
+			error);
 		goto fail2;
 	}
 
+	error = input_register_device(input);
+	if (error) {
+		dev_err(dev, "Unable to register input device, error: %d\n",
+			error);
+		goto fail3;
+	}
+
 	device_init_wakeup(&pdev->dev, wakeup);
 
 	return 0;
 
+ fail3:
+	sysfs_remove_group(&pdev->dev.kobj, &gpio_keys_attr_group);
  fail2:
 	while (--i >= 0) {
 		free_irq(gpio_to_irq(pdata->buttons[i].gpio), &ddata->data[i]);
@@ -219,10 +520,13 @@ static int __devexit gpio_keys_remove(struct platform_device *pdev)
 	struct input_dev *input = ddata->input;
 	int i;
 
+	sysfs_remove_group(&pdev->dev.kobj, &gpio_keys_attr_group);
+
 	device_init_wakeup(&pdev->dev, 0);
 
 	for (i = 0; i < pdata->nbuttons; i++) {
 		int irq = gpio_to_irq(pdata->buttons[i].gpio);
+
 		free_irq(irq, &ddata->data[i]);
 		if (pdata->buttons[i].debounce_interval)
 			del_timer_sync(&ddata->data[i].timer);

Re: [PATCH 2/2] Input: gpio-keys: added support for disabling gpios through sysfs

[Mika Westerberg]

On Fri, Nov 20, 2009 at 09:40:55AM +0100, ext Dmitry Torokhov wrote:
> Hi Mika,
> 
> On Thu, Nov 19, 2009 at 09:23:46AM +0200, Mika Westerberg wrote:
> >
> >  struct gpio_keys_drvdata {
> >       struct input_dev *input;
> > +     /*
> > +      * NOTE: currently these lists are filled in when module is
> > +      *       initialized and never changed after that. This means
> > +      *       that no locking is needed when accessing these. If
> > +      *       this changes then we obviously need to protect these
> > +      *       from concurrent accesses.
> > +      */
> > +     struct list_head keys;     /* list of keys that can be disabled */
> > +     struct list_head switches; /* list of switches that can be disabled */
> >       struct gpio_button_data data[0];
> >  };
> >
> 
> Why do you need the lists? Does the version of patch below still work
> for you?

Indeed. Your version looks much cleaner and simpler :)

I tested it on RX-51 and it works.

Do you want me to create new version of patch 1 where 'exclusive_irq'
is changed to something like 'irqflags'?  Or do we go with the current
one?

Thanks a lot,
MW

> 
> --
> Dmitry
> 
> 
> Input: gpio-keys - added support for disabling gpios through sysfs
> 
> From: Mika Westerberg <ext-mika.1.westerberg@nokia.com>
> 
> Now gpio-keys input driver exports 4 new attributes to userland through
> sysfs:
>         /sys/devices/platform/gpio-keys/keys [ro]
>         /sys/devices/platform/gpio-keys/switches [ro]
>         /sys/devices/platform/gpio-keys/disabled_keys [rw]
>         /sys/devices/platform/gpio-keys/disables_switches [rw]
> 
> With these attributes, userland program can read which keys and
> switches can be disabled and then disable/enable them as needed.
> Keys and switches are exported as stringified bitmap of codes
> (keycodes or switch codes). For example keys 15, 89, 100, 101,
> 102 are exported as: '15,89,100-102'.
> 
> Description of the attributes:
>         keys - bitmap of keys which can be disabled
>         switches - bitmap of switches which can be disabled
>         disabled_keys - bitmap of currently disabled keys
>                         (bit 1 means disabled, 0 enabled)
>         disabled_switches - bitmap of currently disabled switches
>                         (bit 1 means disabled, 0 enabled)
> 
> Signed-off-by: Mika Westerberg <ext-mika.1.westerberg@nokia.com>
> Signed-off-by: Dmitry Torokhov <dtor@mail.ru>
> ---
> 
>  drivers/input/keyboard/gpio_keys.c |  318 +++++++++++++++++++++++++++++++++++-
>  1 files changed, 311 insertions(+), 7 deletions(-)
> 
> 
> diff --git a/drivers/input/keyboard/gpio_keys.c b/drivers/input/keyboard/gpio_keys.c
> index 113d187..3e534db 100644
> --- a/drivers/input/keyboard/gpio_keys.c
> +++ b/drivers/input/keyboard/gpio_keys.c
> @@ -14,6 +14,7 @@
>  #include <linux/fs.h>
>  #include <linux/interrupt.h>
>  #include <linux/irq.h>
> +#include <linux/list.h>
>  #include <linux/sched.h>
>  #include <linux/pm.h>
>  #include <linux/sysctl.h>
> @@ -30,13 +31,299 @@ struct gpio_button_data {
>         struct input_dev *input;
>         struct timer_list timer;
>         struct work_struct work;
> +       bool can_disable;
> +       bool disabled;
>  };
> 
>  struct gpio_keys_drvdata {
>         struct input_dev *input;
> +       struct mutex disable_lock;
> +       unsigned int n_buttons;
>         struct gpio_button_data data[0];
>  };
> 
> +/*
> + * SYSFS interface for enabling/disabling keys and switches:
> + *
> + * There are 4 attributes under /sys/devices/platform/gpio-keys/
> + *     keys [ro]              - bitmap of keys (EV_KEY) which can be
> + *                              disabled
> + *     switches [ro]          - bitmap of switches (EV_SW) which can be
> + *                              disabled
> + *     disabled_keys [rw]     - bitmap of keys currently disabled
> + *     disabled_switches [rw] - bitmap of switches currently disabled
> + *
> + * Userland can change these values and hence disable event generation
> + * for each key (or switch). Disabling a key means its interrupt line
> + * is disabled.
> + *
> + * For example, if we have following switches set up as gpio-keys:
> + *     SW_DOCK = 5
> + *     SW_CAMERA_LENS_COVER = 9
> + *     SW_KEYPAD_SLIDE = 10
> + *     SW_FRONT_PROXIMITY = 11
> + * This is read from switches:
> + *     11-9,5
> + * Next we want to disable proximity (11) and dock (5), we write:
> + *     11,5
> + * to file disabled_switches. Now proximity and dock IRQs are disabled.
> + * This can be verified by reading the file disabled_switches:
> + *     11,5
> + * If we now want to enable proximity (11) switch we write:
> + *     5
> + * to disabled_switches.
> + *
> + * Note that we allow user to specify other bits that are allowed (as
> + * long as she doesn't try to use bits higher than KEY_CNT or SW_CNT).
> + * User can disable bunch of keys for example like this:
> + *     echo 0-600 > disabled_keys
> + * and we only disable those keys that can be disabled.
> + *
> + * We can disable only those keys who have specified 'exclusive_irq = 1'
> + * in their platform data (e.g shared IRQs cannot be disabled).
> + */
> +
> +/**
> + * get_n_events_by_type() - returns maximum number of events per @type
> + * @type: type of button (%EV_KEY, %EV_SW)
> + *
> + * Return value of this function can be used to allocate bitmap
> + * large enough to hold all bits for given type.
> + */
> +static inline int get_n_events_by_type(int type)
> +{
> +       BUG_ON(type != EV_SW && type != EV_KEY);
> +
> +       return (type == EV_KEY) ? KEY_CNT : SW_CNT;
> +}
> +
> +/**
> + * gpio_keys_disable_button() - disables given GPIO button
> + * @bdata: button data for button to be disabled
> + *
> + * Disables button pointed by @bdata. This is done by masking
> + * IRQ line. After this function is called, button won't generate
> + * input events anymore. Note that one can only disable buttons
> + * that don't share IRQs.
> + *
> + * Make sure that @bdata->disable_lock is locked when entering
> + * this function to avoid races when concurrent threads are
> + * disabling buttons at the same time.
> + */
> +static void gpio_keys_disable_button(struct gpio_button_data *bdata)
> +{
> +       BUG_ON(bdata->button->exclusive_irq == 0);
> +
> +       if (!bdata->disabled) {
> +               /*
> +                * Disable IRQ and possible debouncing timer.
> +                */
> +               disable_irq(gpio_to_irq(bdata->button->gpio));
> +               if (bdata->button->debounce_interval)
> +                       del_timer_sync(&bdata->timer);
> +
> +               bdata->disabled = true;
> +       }
> +}
> +
> +/**
> + * gpio_keys_enable_button() - enables given GPIO button
> + * @bdata: button data for button to be disabled
> + *
> + * Enables given button pointed by @bdata.
> + *
> + * Make sure that @bdata->disable_lock is locked when entering
> + * this function to avoid races with concurrent threads trying
> + * to enable the same button at the same time.
> + */
> +static void gpio_keys_enable_button(struct gpio_button_data *bdata)
> +{
> +       if (bdata->disabled) {
> +               enable_irq(gpio_to_irq(bdata->button->gpio));
> +               bdata->disabled = false;
> +       }
> +}
> +
> +/**
> + * gpio_keys_attr_show_helper() - fill in stringified bitmap of buttons
> + * @ddata: pointer to drvdata
> + * @buf: buffer where stringified bitmap is written
> + * @type: button type (%EV_KEY, %EV_SW)
> + * @only_disabled: does caller want only those buttons that are
> + *                 currently disabled or all buttons that can be
> + *                 disabled
> + *
> + * This function writes buttons that can be disabled to @buf. If
> + * @only_disabled is true, then @buf contains only those buttons
> + * that are currently disabled. Returns 0 on success or negative
> + * errno on failure.
> + */
> +static ssize_t gpio_keys_attr_show_helper(struct gpio_keys_drvdata *ddata,
> +                                         char *buf, unsigned int type,
> +                                         bool only_disabled)
> +{
> +       int n_events = get_n_events_by_type(type);
> +       unsigned long *bits;
> +       ssize_t ret;
> +       int i;
> +
> +       bits = kcalloc(BITS_TO_LONGS(n_events), sizeof(*bits), GFP_KERNEL);
> +       if (!bits)
> +               return -ENOMEM;
> +
> +       for (i = 0; i < ddata->n_buttons; i++) {
> +               struct gpio_button_data *bdata = &ddata->data[i];
> +
> +               if (bdata->button->type != type)
> +                       continue;
> +
> +               if (only_disabled && !bdata->disabled)
> +                       continue;
> +
> +               __set_bit(bdata->button->code, bits);
> +       }
> +
> +       ret = bitmap_scnlistprintf(buf, PAGE_SIZE - 2, bits, n_events);
> +       buf[ret++] = '\n';
> +       buf[ret] = '\0';
> +
> +       kfree(bits);
> +
> +       return ret;
> +}
> +
> +/**
> + * gpio_keys_attr_store_helper() - enable/disable buttons based on given bitmap
> + * @ddata: pointer to drvdata
> + * @buf: buffer from userspace that contains stringified bitmap
> + * @type: button type (%EV_KEY, %EV_SW)
> + *
> + * This function parses stringified bitmap from @buf and disables/enables
> + * GPIO buttons accordinly. Returns 0 on success and negative error
> + * on failure.
> + */
> +static ssize_t gpio_keys_attr_store_helper(struct gpio_keys_drvdata *ddata,
> +                                          const char *buf, unsigned int type)
> +{
> +       int n_events = get_n_events_by_type(type);
> +       unsigned long *bits;
> +       ssize_t error;
> +       int i;
> +
> +       bits = kcalloc(BITS_TO_LONGS(n_events), sizeof(*bits), GFP_KERNEL);
> +       if (!bits)
> +               return -ENOMEM;
> +
> +       error = bitmap_parselist(buf, bits, n_events);
> +       if (error)
> +               goto out;
> +
> +       /* First validate */
> +       for (i = 0; i < ddata->n_buttons; i++) {
> +               struct gpio_button_data *bdata = &ddata->data[i];
> +
> +               if (bdata->button->type != type)
> +                       continue;
> +
> +               if (test_bit(bdata->button->code, bits) &&
> +                   !bdata->can_disable) {
> +                       error = -EINVAL;
> +                       goto out;
> +               }
> +       }
> +
> +       mutex_lock(&ddata->disable_lock);
> +
> +       for (i = 0; i < ddata->n_buttons; i++) {
> +               struct gpio_button_data *bdata = &ddata->data[i];
> +
> +               if (bdata->button->type != type)
> +                       continue;
> +
> +               if (test_bit(bdata->button->code, bits))
> +                       gpio_keys_disable_button(bdata);
> +               else
> +                       gpio_keys_enable_button(bdata);
> +       }
> +
> +       mutex_unlock(&ddata->disable_lock);
> +
> +out:
> +       kfree(bits);
> +       return error;
> +}
> +
> +#define ATTR_SHOW_FN(name, type, only_disabled)                                \
> +static ssize_t gpio_keys_show_##name(struct device *dev,               \
> +                                    struct device_attribute *attr,     \
> +                                    char *buf)                         \
> +{                                                                      \
> +       struct platform_device *pdev = to_platform_device(dev);         \
> +       struct gpio_keys_drvdata *ddata = platform_get_drvdata(pdev);   \
> +                                                                       \
> +       return gpio_keys_attr_show_helper(ddata, buf,                   \
> +                                         type, only_disabled);         \
> +}
> +
> +ATTR_SHOW_FN(keys, EV_KEY, false);
> +ATTR_SHOW_FN(switches, EV_SW, false);
> +ATTR_SHOW_FN(disabled_keys, EV_KEY, true);
> +ATTR_SHOW_FN(disabled_switches, EV_SW, true);
> +
> +/*
> + * ATTRIBUTES:
> + *
> + * /sys/devices/platform/gpio-keys/keys [ro]
> + * /sys/devices/platform/gpio-keys/switches [ro]
> + */
> +static DEVICE_ATTR(keys, S_IRUGO, gpio_keys_show_keys, NULL);
> +static DEVICE_ATTR(switches, S_IRUGO, gpio_keys_show_switches, NULL);
> +
> +#define ATTR_STORE_FN(name, type)                                      \
> +static ssize_t gpio_keys_store_##name(struct device *dev,              \
> +                                     struct device_attribute *attr,    \
> +                                     const char *buf,                  \
> +                                     size_t count)                     \
> +{                                                                      \
> +       struct platform_device *pdev = to_platform_device(dev);         \
> +       struct gpio_keys_drvdata *ddata = platform_get_drvdata(pdev);   \
> +       ssize_t error;                                                  \
> +                                                                       \
> +       error = gpio_keys_attr_store_helper(ddata, buf, type);          \
> +       if (error)                                                      \
> +               return error;                                           \
> +                                                                       \
> +       return count;                                                   \
> +}
> +
> +ATTR_STORE_FN(disabled_keys, EV_KEY);
> +ATTR_STORE_FN(disabled_switches, EV_SW);
> +
> +/*
> + * ATTRIBUTES:
> + *
> + * /sys/devices/platform/gpio-keys/disabled_keys [rw]
> + * /sys/devices/platform/gpio-keys/disables_switches [rw]
> + */
> +static DEVICE_ATTR(disabled_keys, S_IWUSR | S_IRUGO,
> +                  gpio_keys_show_disabled_keys,
> +                  gpio_keys_store_disabled_keys);
> +static DEVICE_ATTR(disabled_switches, S_IWUSR | S_IRUGO,
> +                  gpio_keys_show_disabled_switches,
> +                  gpio_keys_store_disabled_switches);
> +
> +static struct attribute *gpio_keys_attrs[] = {
> +       &dev_attr_keys.attr,
> +       &dev_attr_switches.attr,
> +       &dev_attr_disabled_keys.attr,
> +       &dev_attr_disabled_switches.attr,
> +       NULL,
> +};
> +
> +static struct attribute_group gpio_keys_attr_group = {
> +       .attrs = gpio_keys_attrs,
> +};
> +
>  static void gpio_keys_report_event(struct work_struct *work)
>  {
>         struct gpio_button_data *bdata =
> @@ -73,11 +360,12 @@ static irqreturn_t gpio_keys_isr(int irq, void *dev_id)
>         return IRQ_HANDLED;
>  }
> 
> -static int __devinit gpio_keys_setup_key(struct device *dev,
> +static int __devinit gpio_keys_setup_key(struct platform_device *pdev,
>                                          struct gpio_button_data *bdata,
>                                          struct gpio_keys_button *button)
>  {
>         char *desc = button->desc ? button->desc : "gpio_keys";
> +       struct device *dev = &pdev->dev;
>         unsigned long irqflags;
>         int irq, error;
> 
> @@ -114,6 +402,8 @@ static int __devinit gpio_keys_setup_key(struct device *dev,
>          */
>         if (!button->exclusive_irq)
>                 irqflags |= IRQF_SHARED;
> +       else
> +               bdata->can_disable = true;
> 
>         error = request_irq(irq, gpio_keys_isr, irqflags, desc, bdata);
>         if (error) {
> @@ -149,6 +439,10 @@ static int __devinit gpio_keys_probe(struct platform_device *pdev)
>                 goto fail1;
>         }
> 
> +       ddata->input = input;
> +       ddata->n_buttons = pdata->nbuttons;
> +       mutex_init(&ddata->disable_lock);
> +
>         platform_set_drvdata(pdev, ddata);
> 
>         input->name = pdev->name;
> @@ -164,8 +458,6 @@ static int __devinit gpio_keys_probe(struct platform_device *pdev)
>         if (pdata->rep)
>                 __set_bit(EV_REP, input->evbit);
> 
> -       ddata->input = input;
> -
>         for (i = 0; i < pdata->nbuttons; i++) {
>                 struct gpio_keys_button *button = &pdata->buttons[i];
>                 struct gpio_button_data *bdata = &ddata->data[i];
> @@ -174,7 +466,7 @@ static int __devinit gpio_keys_probe(struct platform_device *pdev)
>                 bdata->input = input;
>                 bdata->button = button;
> 
> -               error = gpio_keys_setup_key(dev, bdata, button);
> +               error = gpio_keys_setup_key(pdev, bdata, button);
>                 if (error)
>                         goto fail2;
> 
> @@ -184,17 +476,26 @@ static int __devinit gpio_keys_probe(struct platform_device *pdev)
>                 input_set_capability(input, type, button->code);
>         }
> 
> -       error = input_register_device(input);
> +       error = sysfs_create_group(&pdev->dev.kobj, &gpio_keys_attr_group);
>         if (error) {
> -               dev_err(dev, "Unable to register input device, "
> -                       "error: %d\n", error);
> +               dev_err(dev, "Unable to export keys/switches, error: %d\n",
> +                       error);
>                 goto fail2;
>         }
> 
> +       error = input_register_device(input);
> +       if (error) {
> +               dev_err(dev, "Unable to register input device, error: %d\n",
> +                       error);
> +               goto fail3;
> +       }
> +
>         device_init_wakeup(&pdev->dev, wakeup);
> 
>         return 0;
> 
> + fail3:
> +       sysfs_remove_group(&pdev->dev.kobj, &gpio_keys_attr_group);
>   fail2:
>         while (--i >= 0) {
>                 free_irq(gpio_to_irq(pdata->buttons[i].gpio), &ddata->data[i]);
> @@ -219,10 +520,13 @@ static int __devexit gpio_keys_remove(struct platform_device *pdev)
>         struct input_dev *input = ddata->input;
>         int i;
> 
> +       sysfs_remove_group(&pdev->dev.kobj, &gpio_keys_attr_group);
> +
>         device_init_wakeup(&pdev->dev, 0);
> 
>         for (i = 0; i < pdata->nbuttons; i++) {
>                 int irq = gpio_to_irq(pdata->buttons[i].gpio);
> +
>                 free_irq(irq, &ddata->data[i]);
>                 if (pdata->buttons[i].debounce_interval)
>                         del_timer_sync(&ddata->data[i].timer);

[PATCH v2 0/2] Input: gpio-keys: support for disabling GPIOs

[Mika Westerberg]

Hi Dmitry,

This is second version of the patches. I'm not sure whether I was
supposed to send them again but here they are :)

The first patch now uses new field 'irqflags' which allows platform
to specify exact irq flags it wants.

Second patch is basically the one you provided (the cleaner and
simpler one) but I changed it to use this new field. Also added
some ABI documentation for the sysfs files.

I tested this with RX-51.

Thanks,
MW

Mika Westerberg (2):
  Input: gpio-keys - allow platform to specify exact irq flags
  Input: gpio-keys - added support for disabling gpios through sysfs

 Documentation/ABI/testing/sysfs-platform-gpio-keys |   44 +++
 drivers/input/keyboard/gpio_keys.c                 |  325 +++++++++++++++++++-
 include/linux/gpio_keys.h                          |    7 +
 3 files changed, 365 insertions(+), 11 deletions(-)
 create mode 100644 Documentation/ABI/testing/sysfs-platform-gpio-keys

[PATCH v2 1/2] Input: gpio-keys - allow platform to specify exact irq flags

[Mika Westerberg]

Added new field to struct gpio_keys_button: irqflags which can
be used to specify exact irqflags the platform wants. If not specified,
it defaults to: IRQF_SHARED | IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING.

Signed-off-by: Mika Westerberg <ext-mika.1.westerberg@nokia.com>
---
 drivers/input/keyboard/gpio_keys.c |   13 +++++++++----
 include/linux/gpio_keys.h          |    7 +++++++
 2 files changed, 16 insertions(+), 4 deletions(-)

diff --git a/drivers/input/keyboard/gpio_keys.c b/drivers/input/keyboard/gpio_keys.c
index 8941a8b..6ed5a18 100644
--- a/drivers/input/keyboard/gpio_keys.c
+++ b/drivers/input/keyboard/gpio_keys.c
@@ -78,6 +78,7 @@ static int __devinit gpio_keys_setup_key(struct device *dev,
 					 struct gpio_keys_button *button)
 {
 	char *desc = button->desc ? button->desc : "gpio_keys";
+	unsigned long irqflags;
 	int irq, error;
 
 	setup_timer(&bdata->timer, gpio_keys_timer, (unsigned long)bdata);
@@ -106,10 +107,14 @@ static int __devinit gpio_keys_setup_key(struct device *dev,
 		goto fail3;
 	}
 
-	error = request_irq(irq, gpio_keys_isr,
-			    IRQF_SHARED |
-			    IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING,
-			    desc, bdata);
+	if (button->irqflags == 0) {
+		irqflags = IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING |
+			   IRQF_SHARED;
+	} else {
+		irqflags = button->irqflags;
+	}
+
+	error = request_irq(irq, gpio_keys_isr, irqflags, desc, bdata);
 	if (error) {
 		dev_err(dev, "Unable to claim irq %d; error %d\n",
 			irq, error);
diff --git a/include/linux/gpio_keys.h b/include/linux/gpio_keys.h
index 1289fa7..387b980 100644
--- a/include/linux/gpio_keys.h
+++ b/include/linux/gpio_keys.h
@@ -10,6 +10,13 @@ struct gpio_keys_button {
 	int type;		/* input event type (EV_KEY, EV_SW) */
 	int wakeup;		/* configure the button as a wake-up source */
 	int debounce_interval;	/* debounce ticks interval in msecs */
+	/*
+	 * Exact irqflags this button wants.
+	 *
+	 * If left unset, defaults to:
+	 * IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING | IRQF_SHARED
+	 */
+	unsigned long irqflags;
 };
 
 struct gpio_keys_platform_data {
-- 
1.5.6.5

[PATCH v2 2/2] Input: gpio-keys - added support for disabling gpios through sysfs

[Mika Westerberg]

Now gpio-keys input driver exports 4 new attributes to userland through
sysfs:
        /sys/devices/platform/gpio-keys/keys [ro]
        /sys/devices/platform/gpio-keys/switches [ro]
        /sys/devices/platform/gpio-keys/disabled_keys [rw]
        /sys/devices/platform/gpio-keys/disables_switches [rw]

With these attributes, userland program can read which keys and
switches can be disabled and then disable/enable them as needed.
Keys and switches are exported as stringified bitmap of codes
(keycodes or switch codes). For example keys 15, 89, 100, 101,
102 are exported as: '15,89,100-102'.

Description of the attributes:
        keys - bitmap of keys which can be disabled
        switches - bitmap of switches which can be disabled
        disabled_keys - bitmap of currently disabled keys
                        (bit 1 means disabled, 0 enabled)
        disabled_switches - bitmap of currently disabled switches
                        (bit 1 means disabled, 0 enabled)

Signed-off-by: Mika Westerberg <ext-mika.1.westerberg@nokia.com>
Signed-off-by: Dmitry Torokhov <dtor@mail.ru>
---
 Documentation/ABI/testing/sysfs-platform-gpio-keys |   44 +++
 drivers/input/keyboard/gpio_keys.c                 |  312 +++++++++++++++++++-
 2 files changed, 349 insertions(+), 7 deletions(-)
 create mode 100644 Documentation/ABI/testing/sysfs-platform-gpio-keys

diff --git a/Documentation/ABI/testing/sysfs-platform-gpio-keys b/Documentation/ABI/testing/sysfs-platform-gpio-keys
new file mode 100644
index 0000000..4477323
--- /dev/null
+++ b/Documentation/ABI/testing/sysfs-platform-gpio-keys
@@ -0,0 +1,44 @@
+What:		/sys/devices/platform/gpio-keys/keys
+Date:		November 2009
+KernelVersion:	2.6.33
+Contact:	Mika Westerberg <ext-mika.1.westerberg@nokia.com>
+Description:
+		This file lists gpio keys (EV_KEY) by key code
+		which can be disabled. These are formatted as
+		ASCII encoded bitmap. For example if we have keys
+		152, 212, 528 that can be disabled, the file
+		contains:
+			152,212,528
+		Consecutive bits are shown as hyphen separated
+		numbers. For example keys 9, 10, 11 are shown
+		as:
+			9-11
+
+What:		/sys/devices/platform/gpio-keys/switches
+Date:		November 2009
+KernelVersion:	2.6.33
+Contact:	Mika Westerberg <ext-mika.1.westerberg@nokia.com>
+Description:
+		This file lists gpio switches (EV_SW) by switch
+		code which can be disabled. These are formatted
+		in similar manner than keys.
+
+What:		/sys/devices/platform/gpio-keys/disabled_keys
+Date:		November 2009
+KernelVersion:	2.6.33
+Contact:	Mika Westerberg <ext-mika.1.westerberg@nokia.com>
+Description:
+		This file lists gpio keys (EV_KEY) by key code
+		which are currently disabled and cannot generate
+		interrupts. Format is similar than in previous
+		files. Writing to this file disables given keys.
+
+What:		/sys/devices/platform/gpio-keys/disabled_switches
+Date:		November 2009
+KernelVersion:	2.6.33
+Contact:	Mika Westerberg <ext-mika.1.westerberg@nokia.com>
+Description:
+		This file lists gpio switches (EV_SW) by switch code
+		which are currently disabled and cannot generate
+		interrupts. Format is similar than in previous
+		files. Writing to this file disables given switches.
diff --git a/drivers/input/keyboard/gpio_keys.c b/drivers/input/keyboard/gpio_keys.c
index 6ed5a18..bf15652 100644
--- a/drivers/input/keyboard/gpio_keys.c
+++ b/drivers/input/keyboard/gpio_keys.c
@@ -30,13 +30,290 @@ struct gpio_button_data {
 	struct input_dev *input;
 	struct timer_list timer;
 	struct work_struct work;
+	bool can_disable;
+	bool disabled;
 };
 
 struct gpio_keys_drvdata {
 	struct input_dev *input;
+	struct mutex disable_lock;
+	unsigned int n_buttons;
 	struct gpio_button_data data[0];
 };
 
+/*
+ * SYSFS interface for enabling/disabling keys and switches:
+ *
+ * There are 4 attributes under /sys/devices/platform/gpio-keys/
+ *	keys [ro]              - bitmap of keys (EV_KEY) which can be
+ *	                         disabled
+ *	switches [ro]          - bitmap of switches (EV_SW) which can be
+ *	                         disabled
+ *	disabled_keys [rw]     - bitmap of keys currently disabled
+ *	disabled_switches [rw] - bitmap of switches currently disabled
+ *
+ * Userland can change these values and hence disable event generation
+ * for each key (or switch). Disabling a key means its interrupt line
+ * is disabled.
+ *
+ * For example, if we have following switches set up as gpio-keys:
+ *	SW_DOCK = 5
+ *	SW_CAMERA_LENS_COVER = 9
+ *	SW_KEYPAD_SLIDE = 10
+ *	SW_FRONT_PROXIMITY = 11
+ * This is read from switches:
+ *	11-9,5
+ * Next we want to disable proximity (11) and dock (5), we write:
+ *	11,5
+ * to file disabled_switches. Now proximity and dock IRQs are disabled.
+ * This can be verified by reading the file disabled_switches:
+ *	11,5
+ * If we now want to enable proximity (11) switch we write:
+ *	5
+ * to disabled_switches.
+ *
+ * We can disable only those keys which don't allow sharing the irq.
+ */
+
+/**
+ * get_n_events_by_type() - returns maximum number of events per @type
+ * @type: type of button (%EV_KEY, %EV_SW)
+ *
+ * Return value of this function can be used to allocate bitmap
+ * large enough to hold all bits for given type.
+ */
+static inline int get_n_events_by_type(int type)
+{
+	BUG_ON(type != EV_SW && type != EV_KEY);
+
+	return (type == EV_KEY) ? KEY_CNT : SW_CNT;
+}
+
+/**
+ * gpio_keys_disable_button() - disables given GPIO button
+ * @bdata: button data for button to be disabled
+ *
+ * Disables button pointed by @bdata. This is done by masking
+ * IRQ line. After this function is called, button won't generate
+ * input events anymore. Note that one can only disable buttons
+ * that don't share IRQs.
+ *
+ * Make sure that @bdata->disable_lock is locked when entering
+ * this function to avoid races when concurrent threads are
+ * disabling buttons at the same time.
+ */
+static void gpio_keys_disable_button(struct gpio_button_data *bdata)
+{
+	if (!bdata->disabled) {
+		/*
+		 * Disable IRQ and possible debouncing timer.
+		 */
+		disable_irq(gpio_to_irq(bdata->button->gpio));
+		if (bdata->button->debounce_interval)
+			del_timer_sync(&bdata->timer);
+
+		bdata->disabled = true;
+	}
+}
+
+/**
+ * gpio_keys_enable_button() - enables given GPIO button
+ * @bdata: button data for button to be disabled
+ *
+ * Enables given button pointed by @bdata.
+ *
+ * Make sure that @bdata->disable_lock is locked when entering
+ * this function to avoid races with concurrent threads trying
+ * to enable the same button at the same time.
+ */
+static void gpio_keys_enable_button(struct gpio_button_data *bdata)
+{
+	if (bdata->disabled) {
+		enable_irq(gpio_to_irq(bdata->button->gpio));
+		bdata->disabled = false;
+	}
+}
+
+/**
+ * gpio_keys_attr_show_helper() - fill in stringified bitmap of buttons
+ * @ddata: pointer to drvdata
+ * @buf: buffer where stringified bitmap is written
+ * @type: button type (%EV_KEY, %EV_SW)
+ * @only_disabled: does caller want only those buttons that are
+ *                 currently disabled or all buttons that can be
+ *                 disabled
+ *
+ * This function writes buttons that can be disabled to @buf. If
+ * @only_disabled is true, then @buf contains only those buttons
+ * that are currently disabled. Returns 0 on success or negative
+ * errno on failure.
+ */
+static ssize_t gpio_keys_attr_show_helper(struct gpio_keys_drvdata *ddata,
+					  char *buf, unsigned int type,
+					  bool only_disabled)
+{
+	int n_events = get_n_events_by_type(type);
+	unsigned long *bits;
+	ssize_t ret;
+	int i;
+
+	bits = kcalloc(BITS_TO_LONGS(n_events), sizeof(*bits), GFP_KERNEL);
+	if (!bits)
+		return -ENOMEM;
+
+	for (i = 0; i < ddata->n_buttons; i++) {
+		struct gpio_button_data *bdata = &ddata->data[i];
+
+		if (bdata->button->type != type)
+			continue;
+
+		if (only_disabled && !bdata->disabled)
+			continue;
+
+		__set_bit(bdata->button->code, bits);
+	}
+
+	ret = bitmap_scnlistprintf(buf, PAGE_SIZE - 2, bits, n_events);
+	buf[ret++] = '\n';
+	buf[ret] = '\0';
+
+	kfree(bits);
+
+	return ret;
+}
+
+/**
+ * gpio_keys_attr_store_helper() - enable/disable buttons based on given bitmap
+ * @ddata: pointer to drvdata
+ * @buf: buffer from userspace that contains stringified bitmap
+ * @type: button type (%EV_KEY, %EV_SW)
+ *
+ * This function parses stringified bitmap from @buf and disables/enables
+ * GPIO buttons accordinly. Returns 0 on success and negative error
+ * on failure.
+ */
+static ssize_t gpio_keys_attr_store_helper(struct gpio_keys_drvdata *ddata,
+					   const char *buf, unsigned int type)
+{
+	int n_events = get_n_events_by_type(type);
+	unsigned long *bits;
+	ssize_t error;
+	int i;
+
+	bits = kcalloc(BITS_TO_LONGS(n_events), sizeof(*bits), GFP_KERNEL);
+	if (!bits)
+		return -ENOMEM;
+
+	error = bitmap_parselist(buf, bits, n_events);
+	if (error)
+		goto out;
+
+	/* First validate */
+	for (i = 0; i < ddata->n_buttons; i++) {
+		struct gpio_button_data *bdata = &ddata->data[i];
+
+		if (bdata->button->type != type)
+			continue;
+
+		if (test_bit(bdata->button->code, bits) &&
+		    !bdata->can_disable) {
+			error = -EINVAL;
+			goto out;
+		}
+	}
+
+	mutex_lock(&ddata->disable_lock);
+
+	for (i = 0; i < ddata->n_buttons; i++) {
+		struct gpio_button_data *bdata = &ddata->data[i];
+
+		if (bdata->button->type != type)
+			continue;
+
+		if (test_bit(bdata->button->code, bits))
+			gpio_keys_disable_button(bdata);
+		else
+			gpio_keys_enable_button(bdata);
+	}
+
+	mutex_unlock(&ddata->disable_lock);
+
+out:
+	kfree(bits);
+	return error;
+}
+
+#define ATTR_SHOW_FN(name, type, only_disabled)				\
+static ssize_t gpio_keys_show_##name(struct device *dev,		\
+				     struct device_attribute *attr,	\
+				     char *buf)				\
+{									\
+	struct platform_device *pdev = to_platform_device(dev);		\
+	struct gpio_keys_drvdata *ddata = platform_get_drvdata(pdev);	\
+									\
+	return gpio_keys_attr_show_helper(ddata, buf,			\
+					  type, only_disabled);		\
+}
+
+ATTR_SHOW_FN(keys, EV_KEY, false);
+ATTR_SHOW_FN(switches, EV_SW, false);
+ATTR_SHOW_FN(disabled_keys, EV_KEY, true);
+ATTR_SHOW_FN(disabled_switches, EV_SW, true);
+
+/*
+ * ATTRIBUTES:
+ *
+ * /sys/devices/platform/gpio-keys/keys [ro]
+ * /sys/devices/platform/gpio-keys/switches [ro]
+ */
+static DEVICE_ATTR(keys, S_IRUGO, gpio_keys_show_keys, NULL);
+static DEVICE_ATTR(switches, S_IRUGO, gpio_keys_show_switches, NULL);
+
+#define ATTR_STORE_FN(name, type)					\
+static ssize_t gpio_keys_store_##name(struct device *dev,		\
+				      struct device_attribute *attr,	\
+				      const char *buf,			\
+				      size_t count)			\
+{									\
+	struct platform_device *pdev = to_platform_device(dev);		\
+	struct gpio_keys_drvdata *ddata = platform_get_drvdata(pdev);	\
+	ssize_t error;							\
+									\
+	error = gpio_keys_attr_store_helper(ddata, buf, type);		\
+	if (error)							\
+		return error;						\
+									\
+	return count;							\
+}
+
+ATTR_STORE_FN(disabled_keys, EV_KEY);
+ATTR_STORE_FN(disabled_switches, EV_SW);
+
+/*
+ * ATTRIBUTES:
+ *
+ * /sys/devices/platform/gpio-keys/disabled_keys [rw]
+ * /sys/devices/platform/gpio-keys/disables_switches [rw]
+ */
+static DEVICE_ATTR(disabled_keys, S_IWUSR | S_IRUGO,
+		   gpio_keys_show_disabled_keys,
+		   gpio_keys_store_disabled_keys);
+static DEVICE_ATTR(disabled_switches, S_IWUSR | S_IRUGO,
+		   gpio_keys_show_disabled_switches,
+		   gpio_keys_store_disabled_switches);
+
+static struct attribute *gpio_keys_attrs[] = {
+	&dev_attr_keys.attr,
+	&dev_attr_switches.attr,
+	&dev_attr_disabled_keys.attr,
+	&dev_attr_disabled_switches.attr,
+	NULL,
+};
+
+static struct attribute_group gpio_keys_attr_group = {
+	.attrs = gpio_keys_attrs,
+};
+
 static void gpio_keys_report_event(struct work_struct *work)
 {
 	struct gpio_button_data *bdata =
@@ -73,11 +350,12 @@ static irqreturn_t gpio_keys_isr(int irq, void *dev_id)
 	return IRQ_HANDLED;
 }
 
-static int __devinit gpio_keys_setup_key(struct device *dev,
+static int __devinit gpio_keys_setup_key(struct platform_device *pdev,
 					 struct gpio_button_data *bdata,
 					 struct gpio_keys_button *button)
 {
 	char *desc = button->desc ? button->desc : "gpio_keys";
+	struct device *dev = &pdev->dev;
 	unsigned long irqflags;
 	int irq, error;
 
@@ -112,6 +390,12 @@ static int __devinit gpio_keys_setup_key(struct device *dev,
 			   IRQF_SHARED;
 	} else {
 		irqflags = button->irqflags;
+		/*
+		 * If platform doesn't allow sharing the IRQ we
+		 * can then disable the button through sysfs.
+		 */
+		if ((irqflags & IRQF_SHARED) == 0)
+			bdata->can_disable = true;
 	}
 
 	error = request_irq(irq, gpio_keys_isr, irqflags, desc, bdata);
@@ -148,6 +432,10 @@ static int __devinit gpio_keys_probe(struct platform_device *pdev)
 		goto fail1;
 	}
 
+	ddata->input = input;
+	ddata->n_buttons = pdata->nbuttons;
+	mutex_init(&ddata->disable_lock);
+
 	platform_set_drvdata(pdev, ddata);
 
 	input->name = pdev->name;
@@ -163,8 +451,6 @@ static int __devinit gpio_keys_probe(struct platform_device *pdev)
 	if (pdata->rep)
 		__set_bit(EV_REP, input->evbit);
 
-	ddata->input = input;
-
 	for (i = 0; i < pdata->nbuttons; i++) {
 		struct gpio_keys_button *button = &pdata->buttons[i];
 		struct gpio_button_data *bdata = &ddata->data[i];
@@ -173,7 +459,7 @@ static int __devinit gpio_keys_probe(struct platform_device *pdev)
 		bdata->input = input;
 		bdata->button = button;
 
-		error = gpio_keys_setup_key(dev, bdata, button);
+		error = gpio_keys_setup_key(pdev, bdata, button);
 		if (error)
 			goto fail2;
 
@@ -183,17 +469,26 @@ static int __devinit gpio_keys_probe(struct platform_device *pdev)
 		input_set_capability(input, type, button->code);
 	}
 
-	error = input_register_device(input);
+	error = sysfs_create_group(&pdev->dev.kobj, &gpio_keys_attr_group);
 	if (error) {
-		dev_err(dev, "Unable to register input device, "
-			"error: %d\n", error);
+		dev_err(dev, "Unable to export keys/switches, error: %d\n",
+			error);
 		goto fail2;
 	}
 
+	error = input_register_device(input);
+	if (error) {
+		dev_err(dev, "Unable to register input device, error: %d\n",
+			error);
+		goto fail3;
+	}
+
 	device_init_wakeup(&pdev->dev, wakeup);
 
 	return 0;
 
+ fail3:
+	sysfs_remove_group(&pdev->dev.kobj, &gpio_keys_attr_group);
  fail2:
 	while (--i >= 0) {
 		free_irq(gpio_to_irq(pdata->buttons[i].gpio), &ddata->data[i]);
@@ -218,10 +513,13 @@ static int __devexit gpio_keys_remove(struct platform_device *pdev)
 	struct input_dev *input = ddata->input;
 	int i;
 
+	sysfs_remove_group(&pdev->dev.kobj, &gpio_keys_attr_group);
+
 	device_init_wakeup(&pdev->dev, 0);
 
 	for (i = 0; i < pdata->nbuttons; i++) {
 		int irq = gpio_to_irq(pdata->buttons[i].gpio);
+
 		free_irq(irq, &ddata->data[i]);
 		if (pdata->buttons[i].debounce_interval)
 			del_timer_sync(&ddata->data[i].timer);
-- 
1.5.6.5

Re: [PATCH v2 1/2] Input: gpio-keys - allow platform to specify exact irq flags

[Ferenc Wagner]

Mika Westerberg <ext-mika.1.westerberg@nokia.com> writes:

> Added new field to struct gpio_keys_button: irqflags which can
> be used to specify exact irqflags the platform wants. If not specified,
> it defaults to: IRQF_SHARED | IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING.
>
> --- a/drivers/input/keyboard/gpio_keys.c
> +++ b/drivers/input/keyboard/gpio_keys.c
> @@ -106,10 +107,14 @@ static int __devinit gpio_keys_setup_key(struct device *dev,
>  		goto fail3;
>  	}
>  
> -	error = request_irq(irq, gpio_keys_isr,
> -			    IRQF_SHARED |
> -			    IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING,
> -			    desc, bdata);
> +	if (button->irqflags == 0) {
> +		irqflags = IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING |
> +			   IRQF_SHARED;
> +	} else {
> +		irqflags = button->irqflags;
> +	}
> +
> +	error = request_irq(irq, gpio_keys_isr, irqflags, desc, bdata);
>  	if (error) {
>  		dev_err(dev, "Unable to claim irq %d; error %d\n",
>  			irq, error);

Hi Mika,

linux/interrupt.h says:

    When requesting an interrupt without specifying a IRQF_TRIGGER, the
    setting should be assumed to be "as already configured", which may
    be as per machine or firmware initialisation. 

So I'm not sure it's a good idea to exclude 0 from the possible irqflag
values.
-- 
Regards,
Feri.

Re: [PATCH v2 1/2] Input: gpio-keys - allow platform to specify exact irq flags

[Dmitry Torokhov]

On Mon, Nov 23, 2009 at 05:42:08PM +0100, Ferenc Wagner wrote:
> Mika Westerberg <ext-mika.1.westerberg@nokia.com> writes:
> 
> > Added new field to struct gpio_keys_button: irqflags which can
> > be used to specify exact irqflags the platform wants. If not specified,
> > it defaults to: IRQF_SHARED | IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING.
> >
> > --- a/drivers/input/keyboard/gpio_keys.c
> > +++ b/drivers/input/keyboard/gpio_keys.c
> > @@ -106,10 +107,14 @@ static int __devinit gpio_keys_setup_key(struct device *dev,
> >  		goto fail3;
> >  	}
> >  
> > -	error = request_irq(irq, gpio_keys_isr,
> > -			    IRQF_SHARED |
> > -			    IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING,
> > -			    desc, bdata);
> > +	if (button->irqflags == 0) {
> > +		irqflags = IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING |
> > +			   IRQF_SHARED;
> > +	} else {
> > +		irqflags = button->irqflags;
> > +	}
> > +
> > +	error = request_irq(irq, gpio_keys_isr, irqflags, desc, bdata);
> >  	if (error) {
> >  		dev_err(dev, "Unable to claim irq %d; error %d\n",
> >  			irq, error);
> 
> Hi Mika,
> 
> linux/interrupt.h says:
> 
>     When requesting an interrupt without specifying a IRQF_TRIGGER, the
>     setting should be assumed to be "as already configured", which may
>     be as per machine or firmware initialisation. 
> 
> So I'm not sure it's a good idea to exclude 0 from the possible irqflag
> values.

Hmm... OTOH we can't just require the new irq flags to be always
specified, that would break all current users. Do we need a flag
indicating that the button will be using custom IRQ flags?

-- 
Dmitry

Re: [PATCH v2 1/2] Input: gpio-keys - allow platform to specify exact irq flags

[Ferenc Wagner]

Dmitry Torokhov <dmitry.torokhov@gmail.com> writes:

> On Mon, Nov 23, 2009 at 05:42:08PM +0100, Ferenc Wagner wrote:
>> Mika Westerberg <ext-mika.1.westerberg@nokia.com> writes:
>> 
>>> Added new field to struct gpio_keys_button: irqflags which can
>>> be used to specify exact irqflags the platform wants. If not specified,
>>> it defaults to: IRQF_SHARED | IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING.
>>>
>>> --- a/drivers/input/keyboard/gpio_keys.c
>>> +++ b/drivers/input/keyboard/gpio_keys.c
>>> @@ -106,10 +107,14 @@ static int __devinit gpio_keys_setup_key(struct device *dev,
>>>  		goto fail3;
>>>  	}
>>>  
>>> -	error = request_irq(irq, gpio_keys_isr,
>>> -			    IRQF_SHARED |
>>> -			    IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING,
>>> -			    desc, bdata);
>>> +	if (button->irqflags == 0) {
>>> +		irqflags = IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING |
>>> +			   IRQF_SHARED;
>>> +	} else {
>>> +		irqflags = button->irqflags;
>>> +	}
>>> +
>>> +	error = request_irq(irq, gpio_keys_isr, irqflags, desc, bdata);
>>>  	if (error) {
>>>  		dev_err(dev, "Unable to claim irq %d; error %d\n",
>>>  			irq, error);
>> 
>> linux/interrupt.h says:
>> 
>>     When requesting an interrupt without specifying a IRQF_TRIGGER, the
>>     setting should be assumed to be "as already configured", which may
>>     be as per machine or firmware initialisation. 
>> 
>> So I'm not sure it's a good idea to exclude 0 from the possible irqflag
>> values.
>
> Hmm... OTOH we can't just require the new irq flags to be always
> specified, that would break all current users. Do we need a flag
> indicating that the button will be using custom IRQ flags?

Or let for example -1 mean 0, which is ugly, to say the least.

Anyway, I'd much prefer an implementation which doesn't work against
having multiple buttons on a single IRQ line, which is the case for
example in arch/mips/bcm47xx/gpio.c.  Then the IRQ could still be
disabled if all its buttons are disabled, unless it's shared with
another driver (and IRQF_SHARED would control this aspect sharing).
-- 
Regards,
Feri.

Re: [PATCH v2 1/2] Input: gpio-keys - allow platform to specify exact irq flags

[Mika Westerberg]

On Mon, Nov 23, 2009 at 07:50:20PM +0100, ext Ferenc Wagner wrote:
> Dmitry Torokhov <dmitry.torokhov@gmail.com> writes:
> 
> > On Mon, Nov 23, 2009 at 05:42:08PM +0100, Ferenc Wagner wrote:
> >> Mika Westerberg <ext-mika.1.westerberg@nokia.com> writes:
> >> 
> >>> Added new field to struct gpio_keys_button: irqflags which can
> >>> be used to specify exact irqflags the platform wants. If not specified,
> >>> it defaults to: IRQF_SHARED | IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING.
> >>>
> >>> --- a/drivers/input/keyboard/gpio_keys.c
> >>> +++ b/drivers/input/keyboard/gpio_keys.c
> >>> @@ -106,10 +107,14 @@ static int __devinit gpio_keys_setup_key(struct device *dev,
> >>>  		goto fail3;
> >>>  	}
> >>>  
> >>> -	error = request_irq(irq, gpio_keys_isr,
> >>> -			    IRQF_SHARED |
> >>> -			    IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING,
> >>> -			    desc, bdata);
> >>> +	if (button->irqflags == 0) {
> >>> +		irqflags = IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING |
> >>> +			   IRQF_SHARED;
> >>> +	} else {
> >>> +		irqflags = button->irqflags;
> >>> +	}
> >>> +
> >>> +	error = request_irq(irq, gpio_keys_isr, irqflags, desc, bdata);
> >>>  	if (error) {
> >>>  		dev_err(dev, "Unable to claim irq %d; error %d\n",
> >>>  			irq, error);
> >> 
> >> linux/interrupt.h says:
> >> 
> >>     When requesting an interrupt without specifying a IRQF_TRIGGER, the
> >>     setting should be assumed to be "as already configured", which may
> >>     be as per machine or firmware initialisation. 
> >> 
> >> So I'm not sure it's a good idea to exclude 0 from the possible irqflag
> >> values.
> >
> > Hmm... OTOH we can't just require the new irq flags to be always
> > specified, that would break all current users. Do we need a flag
> > indicating that the button will be using custom IRQ flags?
> 
> Or let for example -1 mean 0, which is ugly, to say the least.

How about adding (again) new field: bool custom_irqflags?

> Anyway, I'd much prefer an implementation which doesn't work against
> having multiple buttons on a single IRQ line, which is the case for
> example in arch/mips/bcm47xx/gpio.c.  Then the IRQ could still be
> disabled if all its buttons are disabled, unless it's shared with
> another driver (and IRQF_SHARED would control this aspect sharing).

Do you mean something like:

	if (button->custom_irqflags) {
		if (button->irqflags & IRQF_SHARED) {
			/* cannot mute this button */
		} else {
			...
	}

then keeping some count of buttons that share single GPIO line and
after all buttons are disabled, only then call disable_irq() for the line?

In our case, we only have 1:1 mapping between GPIO lines and buttons so
current version works but if there are any (possible) users for this kind
of setup I can implement it.

Dmitry, what do you think?

Thanks,
MW

Re: [PATCH v2 1/2] Input: gpio-keys - allow platform to specify exact irq flags

[Ferenc Wagner]

Mika Westerberg <ext-mika.1.westerberg@nokia.com> writes:

> On Mon, Nov 23, 2009 at 07:50:20PM +0100, ext Ferenc Wagner wrote:
>> Dmitry Torokhov <dmitry.torokhov@gmail.com> writes:
>> 
>>> On Mon, Nov 23, 2009 at 05:42:08PM +0100, Ferenc Wagner wrote:
>>>> Mika Westerberg <ext-mika.1.westerberg@nokia.com> writes:
>>>> 
>>>>> Added new field to struct gpio_keys_button: irqflags which can
>>>>> be used to specify exact irqflags the platform wants. If not specified,
>>>>> it defaults to: IRQF_SHARED | IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING.
>>>>>
>>>>> --- a/drivers/input/keyboard/gpio_keys.c
>>>>> +++ b/drivers/input/keyboard/gpio_keys.c
>>>>> @@ -106,10 +107,14 @@ static int __devinit gpio_keys_setup_key(struct device *dev,
>>>>>  		goto fail3;
>>>>>  	}
>>>>>  
>>>>> -	error = request_irq(irq, gpio_keys_isr,
>>>>> -			    IRQF_SHARED |
>>>>> -			    IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING,
>>>>> -			    desc, bdata);
>>>>> +	if (button->irqflags == 0) {
>>>>> +		irqflags = IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING |
>>>>> +			   IRQF_SHARED;
>>>>> +	} else {
>>>>> +		irqflags = button->irqflags;
>>>>> +	}
>>>>> +
>>>>> +	error = request_irq(irq, gpio_keys_isr, irqflags, desc, bdata);
>>>>>  	if (error) {
>>>>>  		dev_err(dev, "Unable to claim irq %d; error %d\n",
>>>>>  			irq, error);
>>>> 
>>>> linux/interrupt.h says:
>>>> 
>>>>     When requesting an interrupt without specifying a IRQF_TRIGGER, the
>>>>     setting should be assumed to be "as already configured", which may
>>>>     be as per machine or firmware initialisation. 
>>>> 
>>>> So I'm not sure it's a good idea to exclude 0 from the possible irqflag
>>>> values.
>>>
>>> Hmm... OTOH we can't just require the new irq flags to be always
>>> specified, that would break all current users. Do we need a flag
>>> indicating that the button will be using custom IRQ flags?
>> 
>> Or let for example -1 mean 0, which is ugly, to say the least.
>
> How about adding (again) new field: bool custom_irqflags?

Thinking about it, since multiple buttons can share a single IRQ, this
can't be a per-button field after all.  Perhaps a better solution would
be adding a new mapping to the platform data:

struct gpio_keys_irq {
        int irq;
        int irqflags;
        int wakeup;                 /* replace the per-button field */
}

struct gpio_keys_platform_data {
        struct gpio_keys_button *buttons;
        int nbuttons;
        unsigned int rep:1;
        struct gpio_keys_irq *irqs; /* new member */
        int nirqs;                  /* new member */
};

>> Anyway, I'd much prefer an implementation which doesn't work against
>> having multiple buttons on a single IRQ line, which is the case for
>> example in arch/mips/bcm47xx/gpio.c.  Then the IRQ could still be
>> disabled if all its buttons are disabled, unless it's shared with
>> another driver (and IRQF_SHARED would control this aspect sharing).
>
> Do you mean something like:
>
> 	if (button->custom_irqflags) {
> 		if (button->irqflags & IRQF_SHARED) {
> 			/* cannot mute this button */
> 		} else {
> 			...
> 	}

Not quite, because the default irqflags contain IRQF_SHARED.  More like:

disable_button (this);
if (button->custom_irqflags && !(button->irqflags & IRQF_SHARED)
    && (all buttons on this IRQ are disabled)) {
        disable_irq (gpio_to_irq (this));
}

> In our case, we only have 1:1 mapping between GPIO lines and buttons so
> current version works but if there are any (possible) users for this kind
> of setup I can implement it.

I quoted (and own) a platform with such arrangement.  I'm somewhat short
on time to work on this hobby project, but of course I don't expect you
to do the heavy lifting.  Unfortunately I haven't yet managed to get a
clear picture about how to properly attack it (the problem is GPIO
access from the IRQ handler, if you didn't read the thread), so I
couldn't produce acceptable code (even though in my case gpio_get_value
can't sleep, so I've got working code).  Meanwhile, I'm trying to do my
best to not let things go in a direction which makes this change harder
that it currently is.  I'm really sorry this hurts your efforts.

> Dmitry, what do you think?

Yep.
-- 
Thanks,
Feri.

Re: [PATCH v2 1/2] Input: gpio-keys - allow platform to specify exact irq flags

[Mika Westerberg]

On Tue, Nov 24, 2009 at 12:05:57PM +0100, ext Ferenc Wagner wrote:
[snip]
> >> Or let for example -1 mean 0, which is ugly, to say the least.
> >
> > How about adding (again) new field: bool custom_irqflags?
> 
> Thinking about it, since multiple buttons can share a single IRQ, this
> can't be a per-button field after all.  Perhaps a better solution would
> be adding a new mapping to the platform data:
> 
> struct gpio_keys_irq {
>         int irq;
>         int irqflags;
>         int wakeup;                 /* replace the per-button field */
> }
> 
> struct gpio_keys_platform_data {
>         struct gpio_keys_button *buttons;
>         int nbuttons;
>         unsigned int rep:1;
>         struct gpio_keys_irq *irqs; /* new member */
>         int nirqs;                  /* new member */
> };

OK. This seems reasonable.

> >> Anyway, I'd much prefer an implementation which doesn't work against
> >> having multiple buttons on a single IRQ line, which is the case for
> >> example in arch/mips/bcm47xx/gpio.c.  Then the IRQ could still be
> >> disabled if all its buttons are disabled, unless it's shared with
> >> another driver (and IRQF_SHARED would control this aspect sharing).
> >
> > Do you mean something like:
> >
> > 	if (button->custom_irqflags) {
> > 		if (button->irqflags & IRQF_SHARED) {
> > 			/* cannot mute this button */
> > 		} else {
> > 			...
> > 	}
> 
> Not quite, because the default irqflags contain IRQF_SHARED.  More like:
> 
> disable_button (this);
> if (button->custom_irqflags && !(button->irqflags & IRQF_SHARED)
>     && (all buttons on this IRQ are disabled)) {
>         disable_irq (gpio_to_irq (this));
> }

I actually meant this code snippet to be in the function gpio_keys_setup_key()
where it only checks whether button can be disabled. Sorry for forgotting
to mention the context.

> > In our case, we only have 1:1 mapping between GPIO lines and buttons so
> > current version works but if there are any (possible) users for this kind
> > of setup I can implement it.
> 
> I quoted (and own) a platform with such arrangement.  I'm somewhat short
> on time to work on this hobby project, but of course I don't expect you
> to do the heavy lifting.  Unfortunately I haven't yet managed to get a
> clear picture about how to properly attack it (the problem is GPIO
> access from the IRQ handler, if you didn't read the thread), so I
> couldn't produce acceptable code (even though in my case gpio_get_value
> can't sleep, so I've got working code).  Meanwhile, I'm trying to do my
> best to not let things go in a direction which makes this change harder
> that it currently is.  I'm really sorry this hurts your efforts.

Thanks for clarifying things. It really don't hurt my efforts; I get
paid for doing this :)

So I'll try to implement this gpio-keys muting so that it allows
also multiple buttons to share single IRQ line.

Thanks,
MW

Re: [PATCH v2 1/2] Input: gpio-keys - allow platform to specify exact irq flags

[Ferenc Wagner]

Mika Westerberg <ext-mika.1.westerberg@nokia.com> writes:

> On Tue, Nov 24, 2009 at 12:05:57PM +0100, ext Ferenc Wagner wrote:
>
>>>> Or let for example -1 mean 0, which is ugly, to say the least.
>>>
>>> How about adding (again) new field: bool custom_irqflags?
>> 
>> Thinking about it, since multiple buttons can share a single IRQ, this
>> can't be a per-button field after all.  Perhaps a better solution would
>> be adding a new mapping to the platform data:
>> 
>> struct gpio_keys_irq {
>>         int irq;
>>         int irqflags;
>>         int wakeup;                 /* replace the per-button field */
>> }
>> 
>> struct gpio_keys_platform_data {
>>         struct gpio_keys_button *buttons;
>>         int nbuttons;
>>         unsigned int rep:1;
>>         struct gpio_keys_irq *irqs; /* new member */
>>         int nirqs;                  /* new member */
>> };
>
> OK. This seems reasonable.

But mind the comment below.

>>> In our case, we only have 1:1 mapping between GPIO lines and buttons
>>> so current version works but if there are any (possible) users for
>>> this kind of setup I can implement it.
>> 
>> I quoted (and own) a platform with such arrangement.  I'm somewhat
>> short on time to work on this hobby project, but of course I don't
>> expect you to do the heavy lifting.  Unfortunately I haven't yet
>> managed to get a clear picture about how to properly attack it (the
>> problem is GPIO access from the IRQ handler, if you didn't read the
>> thread), so I couldn't produce acceptable code (even though in my
>> case gpio_get_value can't sleep, so I've got working code).
>> Meanwhile, I'm trying to do my best to not let things go in a
>> direction which makes this change harder that it currently is.  I'm
>> really sorry this hurts your efforts.
>
> Thanks for clarifying things. It really don't hurt my efforts; I get
> paid for doing this :)
>
> So I'll try to implement this gpio-keys muting so that it allows
> also multiple buttons to share single IRQ line.

Thanks.  But you'd better wait for Dmitry's response, because I'm by no
means authoritative in this business.
-- 
Regards,
Feri.

Re: [PATCH v2 1/2] Input: gpio-keys - allow platform to specify exact irq flags

[Dmitry Torokhov]

On Tue, Nov 24, 2009 at 07:39:28PM +0100, Ferenc Wagner wrote:
> Mika Westerberg <ext-mika.1.westerberg@nokia.com> writes:
> 
> >
> > So I'll try to implement this gpio-keys muting so that it allows
> > also multiple buttons to share single IRQ line.
> 
> Thanks.  But you'd better wait for Dmitry's response, because I'm by no
> means authoritative in this business.

I would not refuse a reasonable patch as long as there is a use case for
it. The only think is that I would prefer to keep compatibility with the
current format of the platform data, otherwise we will screw up embedded
guys that are currently using the driver. But if compatibility results
in too ugly code then I reserve the right to invoke "out of tree drivers
- don't care" card ;)

-- 
Dmitry

Re: [PATCH v2 1/2] Input: gpio-keys - allow platform to specify exact irq flags

[Mika Westerberg]

On Thu, Nov 26, 2009 at 07:35:07AM +0100, ext Dmitry Torokhov wrote:
> On Tue, Nov 24, 2009 at 07:39:28PM +0100, Ferenc Wagner wrote:
> > Mika Westerberg <ext-mika.1.westerberg@nokia.com> writes:
> > 
> > >
> > > So I'll try to implement this gpio-keys muting so that it allows
> > > also multiple buttons to share single IRQ line.
> > 
> > Thanks.  But you'd better wait for Dmitry's response, because I'm by no
> > means authoritative in this business.
> 
> I would not refuse a reasonable patch as long as there is a use case for
> it. The only think is that I would prefer to keep compatibility with the
> current format of the platform data, otherwise we will screw up embedded
> guys that are currently using the driver. But if compatibility results
> in too ugly code then I reserve the right to invoke "out of tree drivers
> - don't care" card ;)

How about if we just add one field to struct gpio_keys_button?
	...
	bool can_disable;

Then gpio_keys uses this field when it decides what irqflags it
is going to pass to request_irq() (we don't allow platform data
to pass exact flags). So in case of can_disable is set, we don't
pass IRQF_SHARED to request_irq().

When support for sharing single IRQ between multiple buttons arrives
this field is still valid and we just need to take care of disable IRQ
line only when all buttons that share it, are disabled.

This won't break existing users of gpio_keys and allows future
extension to support case of multiple buttons sharing single
IRQ line, right?

Thanks,
MW

Re: [PATCH v2 1/2] Input: gpio-keys - allow platform to specify exact irq flags

[Ferenc Wagner]

Mika Westerberg <ext-mika.1.westerberg@nokia.com> writes:

> On Thu, Nov 26, 2009 at 07:35:07AM +0100, ext Dmitry Torokhov wrote:
>> On Tue, Nov 24, 2009 at 07:39:28PM +0100, Ferenc Wagner wrote:
>>> Mika Westerberg <ext-mika.1.westerberg@nokia.com> writes:
>>> 
>>>> So I'll try to implement this gpio-keys muting so that it allows
>>>> also multiple buttons to share single IRQ line.
>>> 
>>> Thanks.  But you'd better wait for Dmitry's response, because I'm by no
>>> means authoritative in this business.
>> 
>> I would not refuse a reasonable patch as long as there is a use case for
>> it. The only think is that I would prefer to keep compatibility with the
>> current format of the platform data, otherwise we will screw up embedded
>> guys that are currently using the driver. But if compatibility results
>> in too ugly code then I reserve the right to invoke "out of tree drivers
>> - don't care" card ;)
>
> How about if we just add one field to struct gpio_keys_button?
> 	...
> 	bool can_disable;
>
> Then gpio_keys uses this field when it decides what irqflags it
> is going to pass to request_irq() (we don't allow platform data
> to pass exact flags). So in case of can_disable is set, we don't
> pass IRQF_SHARED to request_irq().
>
> When support for sharing single IRQ between multiple buttons arrives
> this field is still valid and we just need to take care of disable IRQ
> line only when all buttons that share it, are disabled.
>
> This won't break existing users of gpio_keys and allows future
> extension to support case of multiple buttons sharing single
> IRQ line, right?

Please don't shoot me, but I've got a different idea: what if we leave
this whole business to the generic kernel interrupt handling routines?
It would be as easy as requesting the IRQ on device open (I guess the
input layer can relay the open event down here) and freeing it on close.
The IRQ could stat shared all the time, and the kernel would
automatically disable it when all handlers are unregistered.  All this
means no need to extend the platform data, no need for a separate sysfs
interface, and still the application would have complete control of its
wakeup sources by opening/closing them as needed.  What do you think?
-- 
Regards,
Feri.

Re: [PATCH v2 1/2] Input: gpio-keys - allow platform to specify exact irq flags

[Mika Westerberg]

On Sat, Nov 28, 2009 at 01:16:48PM +0100, Ferenc Wagner wrote:
> Mika Westerberg <ext-mika.1.westerberg@nokia.com> writes:
> 
> > On Thu, Nov 26, 2009 at 07:35:07AM +0100, ext Dmitry Torokhov wrote:
> >> On Tue, Nov 24, 2009 at 07:39:28PM +0100, Ferenc Wagner wrote:
> >>> Mika Westerberg <ext-mika.1.westerberg@nokia.com> writes:
> >>> 
> >>>> So I'll try to implement this gpio-keys muting so that it allows
> >>>> also multiple buttons to share single IRQ line.
> >>> 
> >>> Thanks.  But you'd better wait for Dmitry's response, because I'm by no
> >>> means authoritative in this business.
> >> 
> >> I would not refuse a reasonable patch as long as there is a use case for
> >> it. The only think is that I would prefer to keep compatibility with the
> >> current format of the platform data, otherwise we will screw up embedded
> >> guys that are currently using the driver. But if compatibility results
> >> in too ugly code then I reserve the right to invoke "out of tree drivers
> >> - don't care" card ;)
> >
> > How about if we just add one field to struct gpio_keys_button?
> > 	...
> > 	bool can_disable;
> >
> > Then gpio_keys uses this field when it decides what irqflags it
> > is going to pass to request_irq() (we don't allow platform data
> > to pass exact flags). So in case of can_disable is set, we don't
> > pass IRQF_SHARED to request_irq().
> >
> > When support for sharing single IRQ between multiple buttons arrives
> > this field is still valid and we just need to take care of disable IRQ
> > line only when all buttons that share it, are disabled.
> >
> > This won't break existing users of gpio_keys and allows future
> > extension to support case of multiple buttons sharing single
> > IRQ line, right?
> 
> Please don't shoot me, but I've got a different idea: what if we leave
> this whole business to the generic kernel interrupt handling routines?
> It would be as easy as requesting the IRQ on device open (I guess the
> input layer can relay the open event down here) and freeing it on close.
> The IRQ could stat shared all the time, and the kernel would
> automatically disable it when all handlers are unregistered.  All this
> means no need to extend the platform data, no need for a separate sysfs
> interface, and still the application would have complete control of its
> wakeup sources by opening/closing them as needed.  What do you think?

Yeah, that would be nice. But it won't work for us :(

This is because the actual input device might be open for several
applications. Then we have single process which controls state of
the device. Now if, for example the device is locked by user, this
process just disables those buttons which are not allowed to wake
up the device and blanks the screen.

So the input device is still open but we just prevent GPIO lines
from generating any interrupts while the device is locked. There
are other use-cases also where different buttons are
disabled/enabled.

Thanks,
MW

Re: [PATCH v2 1/2] Input: gpio-keys - allow platform to specify exact irq flags

[Ferenc Wagner]

Mika Westerberg <mika.westerberg@iki.fi> writes:

> On Sat, Nov 28, 2009 at 01:16:48PM +0100, Ferenc Wagner wrote:
>
>> Mika Westerberg <ext-mika.1.westerberg@nokia.com> writes:
>> 
>>> How about if we just add one field to struct gpio_keys_button?
>>> 	...
>>> 	bool can_disable;
>>>
>>> Then gpio_keys uses this field when it decides what irqflags it
>>> is going to pass to request_irq() (we don't allow platform data
>>> to pass exact flags). So in case of can_disable is set, we don't
>>> pass IRQF_SHARED to request_irq().
>>>
>>> When support for sharing single IRQ between multiple buttons arrives
>>> this field is still valid and we just need to take care of disable IRQ
>>> line only when all buttons that share it, are disabled.
>>>
>>> This won't break existing users of gpio_keys and allows future
>>> extension to support case of multiple buttons sharing single
>>> IRQ line, right?
>> 
>> Please don't shoot me, but I've got a different idea: what if we leave
>> this whole business to the generic kernel interrupt handling routines?
>> It would be as easy as requesting the IRQ on device open (I guess the
>> input layer can relay the open event down here) and freeing it on close.
>> The IRQ could stat shared all the time, and the kernel would
>> automatically disable it when all handlers are unregistered.  All this
>> means no need to extend the platform data, no need for a separate sysfs
>> interface, and still the application would have complete control of its
>> wakeup sources by opening/closing them as needed.  What do you think?
>
> Yeah, that would be nice. But it won't work for us :(
>
> This is because the actual input device might be open for several
> applications. Then we have single process which controls state of
> the device. Now if, for example the device is locked by user, this
> process just disables those buttons which are not allowed to wake
> up the device and blanks the screen.
>
> So the input device is still open but we just prevent GPIO lines
> from generating any interrupts while the device is locked. There
> are other use-cases also where different buttons are
> disabled/enabled.

Hi,

I thought we'd better ask our friends over at linux-pm, if they've got
some interface for this task.  To summarize: an embedded application
wants to go into a "locked" state, where some input devices (gpio keys
in this case) are "muted", ie. they don't even generate interrupts to
minimize power consumption.  This could be solved by adding a new
interface to gpio-keys, but the problem seems more general, so I wonder
if something like the USB selective runtime suspend is already available
(or preferable to develop) for such tasks.

(I hope I got the summary right; Mika will provide the missing bits if I
didn't.) 
-- 
Thanks,
Feri.

Re: [linux-pm] [PATCH v2 1/2] Input: gpio-keys - allow platform to specify exact irq flags

[Alan Stern]

On Sun, 29 Nov 2009, Ferenc Wagner wrote:

> Hi,
> 
> I thought we'd better ask our friends over at linux-pm, if they've got
> some interface for this task.  To summarize: an embedded application
> wants to go into a "locked" state, where some input devices (gpio keys
> in this case) are "muted", ie. they don't even generate interrupts to
> minimize power consumption.  This could be solved by adding a new
> interface to gpio-keys, but the problem seems more general, so I wonder
> if something like the USB selective runtime suspend is already available
> (or preferable to develop) for such tasks.
> 
> (I hope I got the summary right; Mika will provide the missing bits if I
> didn't.) 

See Documentation/power/runtime_pm.txt.  I don't know to what extent 
the input layer has implemented any runtime PM.

Alan Stern

Re: [linux-pm] [PATCH v2 1/2] Input: gpio-keys - allow platform to specify exact irq flags

[Ferenc Wagner]

Alan Stern <stern@rowland.harvard.edu> writes:

> On Sun, 29 Nov 2009, Ferenc Wagner wrote:
>
>> I thought we'd better ask our friends over at linux-pm, if they've got
>> some interface for this task.  To summarize: an embedded application
>> wants to go into a "locked" state, where some input devices (gpio keys
>> in this case) are "muted", ie. they don't even generate interrupts to
>> minimize power consumption.  This could be solved by adding a new
>> interface to gpio-keys, but the problem seems more general, so I wonder
>> if something like the USB selective runtime suspend is already available
>> (or preferable to develop) for such tasks.
>
> See Documentation/power/runtime_pm.txt.  I don't know to what extent 
> the input layer has implemented any runtime PM.

Hi Alan,

Looks like it wasn't a bright idea from me to mention runtime suspend
above.  We are looking from something else: some way to forcefully put a
device to "sleep" even thogh it's open (used), because we don't want to
receive input from it but want to save power instead.
-- 
Regards,
Feri.

Re: [linux-pm] [PATCH v2 1/2] Input: gpio-keys - allow platform to specify exact irq flags

[Mika Westerberg]

On Sun, Nov 29, 2009 at 11:58:44PM +0100, Ferenc Wagner wrote:
> Alan Stern <stern@rowland.harvard.edu> writes:
> 
> > On Sun, 29 Nov 2009, Ferenc Wagner wrote:
> >
> >> I thought we'd better ask our friends over at linux-pm, if they've got
> >> some interface for this task.  To summarize: an embedded application
> >> wants to go into a "locked" state, where some input devices (gpio keys
> >> in this case) are "muted", ie. they don't even generate interrupts to
> >> minimize power consumption.  This could be solved by adding a new
> >> interface to gpio-keys, but the problem seems more general, so I wonder
> >> if something like the USB selective runtime suspend is already available
> >> (or preferable to develop) for such tasks.
> >
> > See Documentation/power/runtime_pm.txt.  I don't know to what extent 
> > the input layer has implemented any runtime PM.
> 
> Hi Alan,
> 
> Looks like it wasn't a bright idea from me to mention runtime suspend
> above.  We are looking from something else: some way to forcefully put a
> device to "sleep" even thogh it's open (used), because we don't want to
> receive input from it but want to save power instead.

Hi Feri,

Also in our case, the device is not always in sleep when something needs
to be disabled. For example we might want to disable proximity "button"
when the phone application is not running.

So this is really about "muting" the gpio lines. The ultimate reason behind
this is, obviously, to save battery as much as possible. But these decisions
of muting the buttons come from the user-space.

If it suits you, I would go with the "can_disable" -field in the struct
gpio_keys_button. This way it should be possible to extend gpio-keys in
future to support multiple buttons sharing the single IRQ and it also
works for us. So If it is OK for you, I can prepare patch that does this
and send it to linux-input ml.

BTW: maybe this sharing the IRQ line between multiple buttons could be
implemented with threaded irq handler? Then it can go to sleep if, for
some reason, gpio_get_value() sleeps. Unfortunately I don't have such
hardware so I can't test it.

Thanks,
MW

Re: [linux-pm] [PATCH v2 1/2] Input: gpio-keys - allow platform to specify exact irq flags

[Dmitry Torokhov]

On Mon, Nov 30, 2009 at 10:27:37AM +0200, Mika Westerberg wrote:
> 
> BTW: maybe this sharing the IRQ line between multiple buttons could be
> implemented with threaded irq handler? Then it can go to sleep if, for
> some reason, gpio_get_value() sleeps. Unfortunately I don't have such
> hardware so I can't test it.
> 

We still need debounce support there so you still need a timer and a
workqueue. In the current form theraded IRQ will not buy us anything.

-- 
Dmitry

Re: [linux-pm] [PATCH v2 1/2] Input: gpio-keys - allow platform to specify exact irq flags

[Mika Westerberg]

On Mon, Nov 30, 2009 at 01:14:20AM -0800, Dmitry Torokhov wrote:
> On Mon, Nov 30, 2009 at 10:27:37AM +0200, Mika Westerberg wrote:
> > 
> > BTW: maybe this sharing the IRQ line between multiple buttons could be
> > implemented with threaded irq handler? Then it can go to sleep if, for
> > some reason, gpio_get_value() sleeps. Unfortunately I don't have such
> > hardware so I can't test it.
> > 
> 
> We still need debounce support there so you still need a timer and a
> workqueue. In the current form theraded IRQ will not buy us anything.

Yes, but with threaded irq handler it should be possible to find
out which gpio line caused the irq (via gpio_get_value()) so we can
schedule the correct workqueue handler, right?

MW

Re: [linux-pm] [PATCH v2 1/2] Input: gpio-keys - allow platform to specify exact irq flags

[Ferenc Wagner]

Mika Westerberg <mika.westerberg@iki.fi> writes:

> On Mon, Nov 30, 2009 at 01:14:20AM -0800, Dmitry Torokhov wrote:
>
>> On Mon, Nov 30, 2009 at 10:27:37AM +0200, Mika Westerberg wrote:
>> 
>>> BTW: maybe this sharing the IRQ line between multiple buttons could be
>>> implemented with threaded irq handler? Then it can go to sleep if, for
>>> some reason, gpio_get_value() sleeps. Unfortunately I don't have such
>>> hardware so I can't test it.
>> 
>> We still need debounce support there so you still need a timer and a
>> workqueue. In the current form theraded IRQ will not buy us anything.
>
> Yes, but with threaded irq handler it should be possible to find
> out which gpio line caused the irq (via gpio_get_value()) so we can
> schedule the correct workqueue handler, right?

The principal problem is not sharing an IRQ between multiple GPIO
buttons, but between GPIO buttons and other devices.  Even if you use
threaded IRQs, you have to decide in atomic context whether the handler
thread should be woken up, and that already involves gpio_get_value()
calls.  Of course you can wake up the handler thread unconditionally,
but then

 1. you're running your handlers with IRQs enabled (IRQF_ONESHOT is not
    an option, because it isn't allowed to be shared), which could mean
    retriggering the IRQ before you could acknowledge it, and

 2. you're potentially wasting resources by waking up the handler thread
    needlessly (think about sharing the IRQ with a 115 kbaud serial
    line).

Maybe a good middle ground would be switching to threaded interrupts
only if some of the given GPIOs can sleep, and hope that in this case
the IRQ is not shared with anything speedy (or disallow it, even).  But
I'm pretty much a rookie on this field, so I won't jump on this until
the plan is acked by some authority (Dmitry? :).  This hasn't happened
yet, but critique hasn't been raised either.
-- 
Cheers,
Feri.

Re: [linux-pm] [PATCH v2 1/2] Input: gpio-keys - allow platform to specify exact irq flags

[Ferenc Wagner]

Mika Westerberg <mika.westerberg@iki.fi> writes:

> If it suits you, I would go with the "can_disable" -field in the
> struct gpio_keys_button. This way it should be possible to extend
> gpio-keys in future to support multiple buttons sharing the single IRQ
> and it also works for us.

Why not simply release the corresponding IRQ?  No new fields are needed
in the platform data, you don't even have to change the IRQ flags.  Or
do I miss something again?  Could well be, I'm starting to lose ground
in this discussion... this runtime PM stuff is rather confusing. :]
-- 
Cheers,
Feri.

Re: [linux-pm] [PATCH v2 1/2] Input: gpio-keys - allow platform to specify exact irq flags

[Dmitry Torokhov]

On Mon, Nov 30, 2009 at 09:59:06PM +0100, Ferenc Wagner wrote:
> Mika Westerberg <mika.westerberg@iki.fi> writes:
> 
> > If it suits you, I would go with the "can_disable" -field in the
> > struct gpio_keys_button. This way it should be possible to extend
> > gpio-keys in future to support multiple buttons sharing the single IRQ
> > and it also works for us.
> 
> Why not simply release the corresponding IRQ?  No new fields are needed
> in the platform data, you don't even have to change the IRQ flags.  Or
> do I miss something again?

It would work but with one unpleasant possibility - of you release IRQ
some other device might "steal" it. I don't think it is a good style for
a device to fail due to resources conflict if it was working to begin
with.

-- 
Dmitry

Re: [linux-pm] [PATCH v2 1/2] Input: gpio-keys - allow platform to specify exact irq flags

[Ferenc Wagner]

Dmitry Torokhov <dmitry.torokhov@gmail.com> writes:

> On Mon, Nov 30, 2009 at 09:59:06PM +0100, Ferenc Wagner wrote:
>> Mika Westerberg <mika.westerberg@iki.fi> writes:
>> 
>>> If it suits you, I would go with the "can_disable" -field in the
>>> struct gpio_keys_button. This way it should be possible to extend
>>> gpio-keys in future to support multiple buttons sharing the single IRQ
>>> and it also works for us.
>> 
>> Why not simply release the corresponding IRQ?  No new fields are needed
>> in the platform data, you don't even have to change the IRQ flags.  Or
>> do I miss something again?
>
> It would work but with one unpleasant possibility - of you release IRQ
> some other device might "steal" it. I don't think it is a good style for
> a device to fail due to resources conflict if it was working to begin
> with.

Well, the IRQ is shared, so if somebody steals it, you have a resource
conflict anyway, only with another loser this time.  Unmuting can fail
with -EBUSY.  The Linux Device Drivers book (the only reference I could
find) strongly suggests acquiring resources (especially IRQ in ch.10) on
open() and releasing them on close() -- it wasn't my idea. :)  What's
more, that would in itself make a pure user space solution possible, if
inconvenient, and also be a natural extension of your suggestion to
support multiple buttons on one IRQ by registering the shared handler
separately for each button.  Anyway, it's your call, I just wonder...
-- 
Cheers,
Feri.

Re: [linux-pm] [PATCH v2 1/2] Input: gpio-keys - allow platform to specify exact irq flags

[Dmitry Torokhov]

On Sun, Nov 29, 2009 at 11:04:57AM -0500, Alan Stern wrote:
> On Sun, 29 Nov 2009, Ferenc Wagner wrote:
> 
> > Hi,
> > 
> > I thought we'd better ask our friends over at linux-pm, if they've got
> > some interface for this task.  To summarize: an embedded application
> > wants to go into a "locked" state, where some input devices (gpio keys
> > in this case) are "muted", ie. they don't even generate interrupts to
> > minimize power consumption.  This could be solved by adding a new
> > interface to gpio-keys, but the problem seems more general, so I wonder
> > if something like the USB selective runtime suspend is already available
> > (or preferable to develop) for such tasks.
> > 
> > (I hope I got the summary right; Mika will provide the missing bits if I
> > didn't.) 
> 
> See Documentation/power/runtime_pm.txt.  I don't know to what extent 
> the input layer has implemented any runtime PM.

Input layer is not implementing any PM since it consists of virtual, not
physical devices, and leaves PM for the appropriate bus drivers (USB,
serio, platform, etc.).

Also, it is not really runtime PM but rather user-initiated action of
putting device into lower poer state we are talking about here.

-- 
Dmitry

Re: [linux-pm] [PATCH v2 1/2] Input: gpio-keys - allow platform to specify exact irq flags

[Alan Stern]

On Mon, 30 Nov 2009, Dmitry Torokhov wrote:

> On Sun, Nov 29, 2009 at 11:04:57AM -0500, Alan Stern wrote:
> > On Sun, 29 Nov 2009, Ferenc Wagner wrote:
> > 
> > > Hi,
> > > 
> > > I thought we'd better ask our friends over at linux-pm, if they've got
> > > some interface for this task.  To summarize: an embedded application
> > > wants to go into a "locked" state, where some input devices (gpio keys
> > > in this case) are "muted", ie. they don't even generate interrupts to
> > > minimize power consumption.  This could be solved by adding a new
> > > interface to gpio-keys, but the problem seems more general, so I wonder
> > > if something like the USB selective runtime suspend is already available
> > > (or preferable to develop) for such tasks.
> > > 
> > > (I hope I got the summary right; Mika will provide the missing bits if I
> > > didn't.) 
> > 
> > See Documentation/power/runtime_pm.txt.  I don't know to what extent 
> > the input layer has implemented any runtime PM.
> 
> Input layer is not implementing any PM since it consists of virtual, not
> physical devices, and leaves PM for the appropriate bus drivers (USB,
> serio, platform, etc.).
> 
> Also, it is not really runtime PM but rather user-initiated action of
> putting device into lower poer state we are talking about here.

This is in fact the definition of runtime PM -- except for the
"user-initiated" restriction (runtime PM can be initiated by anything
or anybody).  So it really _is_ runtime PM.

But if you prefer to implement it without using the runtime PM 
framework, that's fine.

Alan Stern

Re: [linux-pm] [PATCH v2 1/2] Input: gpio-keys - allow platform to specify exact irq flags

[Ferenc Wagner]

Alan Stern <stern@rowland.harvard.edu> writes:

> On Mon, 30 Nov 2009, Dmitry Torokhov wrote:
>
>> On Sun, Nov 29, 2009 at 11:04:57AM -0500, Alan Stern wrote:
>>
>>> On Sun, 29 Nov 2009, Ferenc Wagner wrote:
>>> 
>>>> I thought we'd better ask our friends over at linux-pm, if they've got
>>>> some interface for this task.  To summarize: an embedded application
>>>> wants to go into a "locked" state, where some input devices (gpio keys
>>>> in this case) are "muted", ie. they don't even generate interrupts to
>>>> minimize power consumption.  This could be solved by adding a new
>>>> interface to gpio-keys, but the problem seems more general, so I wonder
>>>> if something like the USB selective runtime suspend is already available
>>>> (or preferable to develop) for such tasks.
>>> 
>>> See Documentation/power/runtime_pm.txt.  I don't know to what extent 
>>> the input layer has implemented any runtime PM.
>> 
>> Input layer is not implementing any PM since it consists of virtual, not
>> physical devices, and leaves PM for the appropriate bus drivers (USB,
>> serio, platform, etc.).
>> 
>> Also, it is not really runtime PM but rather user-initiated action of
>> putting device into lower power state we are talking about here.
>
> This is in fact the definition of runtime PM -- except for the
> "user-initiated" restriction (runtime PM can be initiated by anything
> or anybody).  So it really _is_ runtime PM.

But runtime-pm.txt says for example:

    Generally, remote wake-up should be enabled for all input devices
    put into a low power state at run time.

But in this case the requirement is to suppress input events from a
given device, effectively muting and putting it into low power state,
even though it's still open by some other parties.  Runtime PM, on the
other hand tries not to interfere with the normal usage of the device.

Later:

(3) ->runtime_idle() and ->runtime_suspend() can only be executed for a
    device the usage counter of which is equal to zero _and_ [...]

which underlines the difference again: the usage counter (defined by
common sense) won't be zero in our case, because the device is
constantly kept open, while we want to mute it, putting it into a low
power state.  Probably, the rules could be bent so that the platform bus
could suspend these devices and achieve our aim, but I'd consider that
an abuse of the runtime PM infrastructure.  Don't you?

Actually, this could be implemented by the various users cooperating in
closing the device, letting it go to sleep automatically.  But this
requires strictly cooperating parties and is more complicated that
flipping some master switch of the device.  We're looking for this
master switch, before needlessly building our own.

> But if you prefer to implement it without using the runtime PM 
> framework, that's fine.

We're also looking for a framework to implement it in.  Now I feel like
the runtime PM framework is not a good fit for what we want, but it's
the first time I'm reading its documentation, so please correct my
misunderstandings.  That's why I cross posted linux-pm, after all. :)
-- 
Thanks,
Feri.

Re: [linux-pm] [PATCH v2 1/2] Input: gpio-keys - allow platform to specify exact irq flags

[Alan Stern]

On Mon, 30 Nov 2009, Ferenc Wagner wrote:

> >> Also, it is not really runtime PM but rather user-initiated action of
> >> putting device into lower power state we are talking about here.
> >
> > This is in fact the definition of runtime PM -- except for the
> > "user-initiated" restriction (runtime PM can be initiated by anything
> > or anybody).  So it really _is_ runtime PM.
> 
> But runtime-pm.txt says for example:
> 
>     Generally, remote wake-up should be enabled for all input devices
>     put into a low power state at run time.
> 
> But in this case the requirement is to suppress input events from a
> given device, effectively muting and putting it into low power state,
> even though it's still open by some other parties.  Runtime PM, on the
> other hand tries not to interfere with the normal usage of the device.

That's why the text says "Generally".  You're free to do it a different 
way if you want.

> Later:
> 
> (3) ->runtime_idle() and ->runtime_suspend() can only be executed for a
>     device the usage counter of which is equal to zero _and_ [...]
> 
> which underlines the difference again: the usage counter (defined by
> common sense) won't be zero in our case, because the device is
> constantly kept open, while we want to mute it, putting it into a low
> power state.  Probably, the rules could be bent so that the platform bus
> could suspend these devices and achieve our aim, but I'd consider that
> an abuse of the runtime PM infrastructure.  Don't you?

The usage counter can be set whenever you want; it doesn't have to be 
incremented during open and decremented during close.  You could leave 
it equal to 0 permanently.  Or you could make it nonzero precisely 
during the times when you don't want the input to be muted.

> Actually, this could be implemented by the various users cooperating in
> closing the device, letting it go to sleep automatically.  But this
> requires strictly cooperating parties and is more complicated that
> flipping some master switch of the device.  We're looking for this
> master switch, before needlessly building our own.

Just out of curiosity, how do you decide when the input should be 
muted and unmuted?

> > But if you prefer to implement it without using the runtime PM 
> > framework, that's fine.
> 
> We're also looking for a framework to implement it in.  Now I feel like
> the runtime PM framework is not a good fit for what we want, but it's
> the first time I'm reading its documentation, so please correct my
> misunderstandings.  That's why I cross posted linux-pm, after all. :)

The framework was intended to be pretty flexible.  You should think it 
over a little more before deciding against it.

Alan Stern

Re: [linux-pm] [PATCH v2 1/2] Input: gpio-keys - allow platform to specify exact irq flags

[Ferenc Wagner]

Alan Stern <stern@rowland.harvard.edu> writes:

> On Mon, 30 Nov 2009, Ferenc Wagner wrote:
>
>>>> Also, it is not really runtime PM but rather user-initiated action of
>>>> putting device into lower power state we are talking about here.
>>>
>>> This is in fact the definition of runtime PM -- except for the
>>> "user-initiated" restriction (runtime PM can be initiated by anything
>>> or anybody).  So it really _is_ runtime PM.
>> 
>> But runtime-pm.txt says for example:
>> 
>>     Generally, remote wake-up should be enabled for all input devices
>>     put into a low power state at run time.
>> 
>> But in this case the requirement is to suppress input events from a
>> given device, effectively muting and putting it into low power state,
>> even though it's still open by some other parties.  Runtime PM, on the
>> other hand tries not to interfere with the normal usage of the device.
>
> That's why the text says "Generally".  You're free to do it a different 
> way if you want.
>
>> Later:
>> 
>> (3) ->runtime_idle() and ->runtime_suspend() can only be executed for a
>>     device the usage counter of which is equal to zero _and_ [...]
>> 
>> which underlines the difference again: the usage counter (defined by
>> common sense) won't be zero in our case, because the device is
>> constantly kept open, while we want to mute it, putting it into a low
>> power state.  Probably, the rules could be bent so that the platform
>> bus could suspend these devices and achieve our aim, but I'd consider
>> that an abuse of the runtime PM infrastructure.  Wouldn't you?
>
> The usage counter can be set whenever you want; it doesn't have to be
> incremented during open and decremented during close.  You could leave
> it equal to 0 permanently.  Or you could make it nonzero precisely
> during the times when you don't want the input to be muted.

And what would be the user space interface for controlling the power
state of the device?  The documentation doesn't talk about this, and if
we need to make up new ioctls or sysfs files, what do we gain by using
the runtime PM framework?  Especially if there's no uniform (bus- and
system-wide) semantics?

>> Actually, this could be implemented by the various users cooperating
>> in closing the device, letting it go to sleep automatically.  But
>> this requires strictly cooperating parties and is more complicated
>> that flipping some master switch of the device.  We're looking for
>> this master switch, before needlessly building our own.
>
> Just out of curiosity, how do you decide when the input should be 
> muted and unmuted?

That's application logic.  For example, the user touches the "Lock"
button, then slips the gadget into his pocket.  Then most other buttons
(like volume control) are muted until the device is unlocked again by
some very specific action.  My mobile phone doesn't work like this, it
reacts to each button even when it's locked (though of course does
nothing but says how to unlock it).

>>> But if you prefer to implement it without using the runtime PM 
>>> framework, that's fine.
>> 
>> We're also looking for a framework to implement it in.  Now I feel like
>> the runtime PM framework is not a good fit for what we want, but it's
>> the first time I'm reading its documentation, so please correct my
>> misunderstandings.  That's why I cross posted linux-pm, after all. :)
>
> The framework was intended to be pretty flexible.  You should think it 
> over a little more before deciding against it.

Well, I thought runtime PM is intended to have some "common sense"
semantics, so it can be automatic.  After all, it's perfectly reasonable
for somebody else to want the "normal" runtime PM for gpio-keys: to
autosuspend when not in use.  If we stole the callbacks for our purpose,
the "expected" behaviour couldn't be implemented later.  In other words,
the wanted behaviour is not device specific, but usage specific.  And
the runtime suspend behaviour of a device can't be usage specific, it
should match common expectations (to the extent it's possible to talk
about common expectations wrt. runtime PM -- but you may still want to
build one up).  Are these fears unfounded?  Is runtime PM not what I
think it is?
-- 
Thanks,
Feri.

Re: [linux-pm] [PATCH v2 1/2] Input: gpio-keys - allow platform to specify exact irq flags

[Alan Stern]

On Mon, 30 Nov 2009, Ferenc Wagner wrote:

> > Just out of curiosity, how do you decide when the input should be 
> > muted and unmuted?
> 
> That's application logic.  For example, the user touches the "Lock"
> button, then slips the gadget into his pocket.  Then most other buttons
> (like volume control) are muted until the device is unlocked again by
> some very specific action.  My mobile phone doesn't work like this, it
> reacts to each button even when it's locked (though of course does
> nothing but says how to unlock it).

Okay.  This wasn't clear to me before.

> Well, I thought runtime PM is intended to have some "common sense"
> semantics, so it can be automatic.  After all, it's perfectly reasonable
> for somebody else to want the "normal" runtime PM for gpio-keys: to
> autosuspend when not in use.  If we stole the callbacks for our purpose,
> the "expected" behaviour couldn't be implemented later.  In other words,
> the wanted behaviour is not device specific, but usage specific.  And
> the runtime suspend behaviour of a device can't be usage specific, it
> should match common expectations (to the extent it's possible to talk
> about common expectations wrt. runtime PM -- but you may still want to
> build one up).  Are these fears unfounded?  Is runtime PM not what I
> think it is?

No, you are right.  Runtime PM is not well suited for this purpose.

Alan Stern

Re: [linux-pm] [PATCH v2 1/2] Input: gpio-keys - allow platform to specify exact irq flags

[Ferenc Wagner]

Alan Stern <stern@rowland.harvard.edu> writes:

> Runtime PM is not well suited for this purpose.

So the question is: is there some other PM framework which would be a
good fit for this "forced manual selective suspend" use case?
-- 
Thanks,
Feri.

Re: [linux-pm] [PATCH v2 1/2] Input: gpio-keys - allow platform to specify exact irq flags

[Alan Stern]

On Tue, 1 Dec 2009, Ferenc Wagner wrote:

> Alan Stern <stern@rowland.harvard.edu> writes:
> 
> > Runtime PM is not well suited for this purpose.
> 
> So the question is: is there some other PM framework which would be a
> good fit for this "forced manual selective suspend" use case?

None that I know of.

Alan Stern

Re: [linux-pm] [PATCH v2 1/2] Input: gpio-keys - allow platform to specify exact irq flags

[Pavel Machek]

Hi!


> But runtime-pm.txt says for example:
> 
>     Generally, remote wake-up should be enabled for all input devices
>     put into a low power state at run time.
> 
> But in this case the requirement is to suppress input events from a
> given device, effectively muting and putting it into low power state,
> even though it's still open by some other parties.  Runtime PM, on the
> other hand tries not to interfere with the normal usage of the device.
> 
> Later:
> 
> (3) ->runtime_idle() and ->runtime_suspend() can only be executed for a
>     device the usage counter of which is equal to zero _and_ [...]
> 
> which underlines the difference again: the usage counter (defined by
> common sense) won't be zero in our case, because the device is
> constantly kept open, while we want to mute it, putting it into a low
> power state. 
...
> Actually, this could be implemented by the various users cooperating in
> closing the device, letting it go to sleep automatically.  But this
> requires strictly cooperating parties and is more complicated that
> flipping some master switch of the device.  We're looking for this
> master switch, before needlessly building our own.

Please just close the device properly. I do not think we want 100
different 'please mute keys A and G', 'please mute middle mouse button',
... interfaces anywhere near mainline.

Or just do it as local patch.

									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [linux-pm] [PATCH v2 1/2] Input: gpio-keys - allow platform to specify exact irq flags

[Dmitry Torokhov]

On Sun, Dec 06, 2009 at 09:47:04AM +0100, Pavel Machek wrote:
> Hi!
> 
> 
> > But runtime-pm.txt says for example:
> > 
> >     Generally, remote wake-up should be enabled for all input devices
> >     put into a low power state at run time.
> > 
> > But in this case the requirement is to suppress input events from a
> > given device, effectively muting and putting it into low power state,
> > even though it's still open by some other parties.  Runtime PM, on the
> > other hand tries not to interfere with the normal usage of the device.
> > 
> > Later:
> > 
> > (3) ->runtime_idle() and ->runtime_suspend() can only be executed for a
> >     device the usage counter of which is equal to zero _and_ [...]
> > 
> > which underlines the difference again: the usage counter (defined by
> > common sense) won't be zero in our case, because the device is
> > constantly kept open, while we want to mute it, putting it into a low
> > power state. 
> ...
> > Actually, this could be implemented by the various users cooperating in
> > closing the device, letting it go to sleep automatically.  But this
> > requires strictly cooperating parties and is more complicated that
> > flipping some master switch of the device.  We're looking for this
> > master switch, before needlessly building our own.
> 
> Please just close the device properly. I do not think we want 100
> different 'please mute keys A and G', 'please mute middle mouse button',
> ... interfaces anywhere near mainline.
> 

I do not think it is practical to simply close the device, given that
there may be several applications that have it open. I constantly see
embedded guys adding custom knobs to the devices allowing them to shut
off the device when not in use. Kind of runtame PM but user-initiated.

I would really love to have it implemented in the driver core so the
interface is the same for all drivers (that support this future).

> Or just do it as local patch.

I also see that gpio-keys is quite different in the sence that it can
shut off buttons selectively. I fact, at the moment every button can be
considered a separate device... But that would be too much overhead.

They could probably split the keys into 2 groups (critical that should
be always active) and not critical, that could be shut off, but I think
they want teh flexibility of controlling this at runtime instead of
doing it in board data.

-- 
Dmitry

Re: [linux-pm] [PATCH v2 1/2] Input: gpio-keys - allow platform to specify exact irq flags

[Artem Bityutskiy]

On Mon, 2009-12-07 at 20:22 -0800, Dmitry Torokhov wrote:
> On Sun, Dec 06, 2009 at 09:47:04AM +0100, Pavel Machek wrote:
> > Hi!
> > 
> > 
> > > But runtime-pm.txt says for example:
> > > 
> > >     Generally, remote wake-up should be enabled for all input devices
> > >     put into a low power state at run time.
> > > 
> > > But in this case the requirement is to suppress input events from a
> > > given device, effectively muting and putting it into low power state,
> > > even though it's still open by some other parties.  Runtime PM, on the
> > > other hand tries not to interfere with the normal usage of the device.
> > > 
> > > Later:
> > > 
> > > (3) ->runtime_idle() and ->runtime_suspend() can only be executed for a
> > >     device the usage counter of which is equal to zero _and_ [...]
> > > 
> > > which underlines the difference again: the usage counter (defined by
> > > common sense) won't be zero in our case, because the device is
> > > constantly kept open, while we want to mute it, putting it into a low
> > > power state. 
> > ...
> > > Actually, this could be implemented by the various users cooperating in
> > > closing the device, letting it go to sleep automatically.  But this
> > > requires strictly cooperating parties and is more complicated that
> > > flipping some master switch of the device.  We're looking for this
> > > master switch, before needlessly building our own.
> > 
> > Please just close the device properly. I do not think we want 100
> > different 'please mute keys A and G', 'please mute middle mouse button',
> > ... interfaces anywhere near mainline.
> > 
> 
> I do not think it is practical to simply close the device, given that
> there may be several applications that have it open. I constantly see
> embedded guys adding custom knobs to the devices allowing them to shut
> off the device when not in use. Kind of runtame PM but user-initiated.
> 
> I would really love to have it implemented in the driver core so the
> interface is the same for all drivers (that support this future).
> 
> > Or just do it as local patch.
> 
> I also see that gpio-keys is quite different in the sence that it can
> shut off buttons selectively. I fact, at the moment every button can be
> considered a separate device... But that would be too much overhead.
> 
> They could probably split the keys into 2 groups (critical that should
> be always active) and not critical, that could be shut off, but I think
> they want teh flexibility of controlling this at runtime instead of
> doing it in board data.

I suggested including this into the "abstract input device" model, but
you refuse this. But I still think it is a good idea.

Indeed, if we look at an input device as at something abstract which has
many keys, why we cannot assume that separate keys can be
enabled/disabled? Just imagine you have a very advanced keybord :-) And
we simply implement an ioctl which enables/disables a specific key. The
generic layers just pass this ioctl down to the lower lever drivers. If
the specific input device or driver support it - fine, if not - it
returns -EINVAL or something like that.

-- 
Best Regards,
Artem Bityutskiy (Артём Битюцкий)

--
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: [linux-pm] [PATCH v2 1/2] Input: gpio-keys - allow platform to specify exact irq flags

[Dmitry Torokhov]

On Tue, Dec 08, 2009 at 03:03:30PM +0200, Artem Bityutskiy wrote:
> On Mon, 2009-12-07 at 20:22 -0800, Dmitry Torokhov wrote:
> > On Sun, Dec 06, 2009 at 09:47:04AM +0100, Pavel Machek wrote:
> > > Hi!
> > > 
> > > 
> > > > But runtime-pm.txt says for example:
> > > > 
> > > >     Generally, remote wake-up should be enabled for all input devices
> > > >     put into a low power state at run time.
> > > > 
> > > > But in this case the requirement is to suppress input events from a
> > > > given device, effectively muting and putting it into low power state,
> > > > even though it's still open by some other parties.  Runtime PM, on the
> > > > other hand tries not to interfere with the normal usage of the device.
> > > > 
> > > > Later:
> > > > 
> > > > (3) ->runtime_idle() and ->runtime_suspend() can only be executed for a
> > > >     device the usage counter of which is equal to zero _and_ [...]
> > > > 
> > > > which underlines the difference again: the usage counter (defined by
> > > > common sense) won't be zero in our case, because the device is
> > > > constantly kept open, while we want to mute it, putting it into a low
> > > > power state. 
> > > ...
> > > > Actually, this could be implemented by the various users cooperating in
> > > > closing the device, letting it go to sleep automatically.  But this
> > > > requires strictly cooperating parties and is more complicated that
> > > > flipping some master switch of the device.  We're looking for this
> > > > master switch, before needlessly building our own.
> > > 
> > > Please just close the device properly. I do not think we want 100
> > > different 'please mute keys A and G', 'please mute middle mouse button',
> > > ... interfaces anywhere near mainline.
> > > 
> > 
> > I do not think it is practical to simply close the device, given that
> > there may be several applications that have it open. I constantly see
> > embedded guys adding custom knobs to the devices allowing them to shut
> > off the device when not in use. Kind of runtame PM but user-initiated.
> > 
> > I would really love to have it implemented in the driver core so the
> > interface is the same for all drivers (that support this future).
> > 
> > > Or just do it as local patch.
> > 
> > I also see that gpio-keys is quite different in the sence that it can
> > shut off buttons selectively. I fact, at the moment every button can be
> > considered a separate device... But that would be too much overhead.
> > 
> > They could probably split the keys into 2 groups (critical that should
> > be always active) and not critical, that could be shut off, but I think
> > they want teh flexibility of controlling this at runtime instead of
> > doing it in board data.
> 
> I suggested including this into the "abstract input device" model, but
> you refuse this. But I still think it is a good idea.
> 
> Indeed, if we look at an input device as at something abstract which has
> many keys, why we cannot assume that separate keys can be
> enabled/disabled? Just imagine you have a very advanced keybord :-) And
> we simply implement an ioctl which enables/disables a specific key. The
> generic layers just pass this ioctl down to the lower lever drivers. If
> the specific input device or driver support it - fine, if not - it
> returns -EINVAL or something like that.

I refuse it because it will be supported by exactly 1 driver in the
kernel - gpio-keys. It is the only driver that allows shut half of the
"device" (because in reality it is a group of disjoint devices). It is
the only case when "muting" a button means that IRQ is shut off abnd
thus CPU can continue to sleep if that button is pressed. For all other
devices that have 1 inettrupt per device, you still have to wake up,
because you don't know whether the button that generated event is
"important" or not.

Now, there is a issue of waking up userspace task, additional scheduling
and keeping CPU running longer than necessary for "uninteresting" keys.
This can be solved by implementing a subscription model which allows
filtering uninteresing events on a per-client basis at evdev level.
This, if implemented properly, would work for _all_ input devices out
there. You were not interested into looking into it (because for your
particular and only device the otehr approach promises bigger savings)
but I think we'll get there eventually.

And the third topic - shutting (or putting into low power) entire device
upon request from userspace. This again has much wider auditory than
gpio-keys, or input devices layer for that matter. We may want to do so
for other types of devices as well. That is why the question when to
general PM list.

-- 
Dmitry

Re: [linux-pm] [PATCH v2 1/2] Input: gpio-keys - allow platform to specify exact irq flags

[Artem Bityutskiy]

On Tue, 2009-12-08 at 09:42 -0800, Dmitry Torokhov wrote:
> > > I also see that gpio-keys is quite different in the sence that it can
> > > shut off buttons selectively. I fact, at the moment every button can be
> > > considered a separate device... But that would be too much overhead.
> > > 
> > > They could probably split the keys into 2 groups (critical that should
> > > be always active) and not critical, that could be shut off, but I think
> > > they want teh flexibility of controlling this at runtime instead of
> > > doing it in board data.
> > 
> > I suggested including this into the "abstract input device" model, but
> > you refuse this. But I still think it is a good idea.
> > 
> > Indeed, if we look at an input device as at something abstract which has
> > many keys, why we cannot assume that separate keys can be
> > enabled/disabled? Just imagine you have a very advanced keybord :-) And
> > we simply implement an ioctl which enables/disables a specific key. The
> > generic layers just pass this ioctl down to the lower lever drivers. If
> > the specific input device or driver support it - fine, if not - it
> > returns -EINVAL or something like that.
> 
> I refuse it because it will be supported by exactly 1 driver in the
> kernel - gpio-keys. It is the only driver that allows shut half of the
> "device" (because in reality it is a group of disjoint devices). It is
> the only case when "muting" a button means that IRQ is shut off abnd
> thus CPU can continue to sleep if that button is pressed. For all other
> devices that have 1 inettrupt per device, you still have to wake up,
> because you don't know whether the button that generated event is
> "important" or not.

Fair enough.

> Now, there is a issue of waking up userspace task, additional scheduling
> and keeping CPU running longer than necessary for "uninteresting" keys.
> This can be solved by implementing a subscription model which allows
> filtering uninteresing events on a per-client basis at evdev level.

Right. And for gpio_keys, this would be dine on the driver level.

> This, if implemented properly, would work for _all_ input devices out
> there. You were not interested into looking into it (because for your
> particular and only device the otehr approach promises bigger savings)
> but I think we'll get there eventually.

Well, we can (and have to, if this approach is taken) look into this in
a sense of implementing our particular task in a way that it could be
extended with this generic stuff. We might as well implement something
generic, but may be not too comprehensive, feature-full, and well-tested
(simply because we do not need it, so cannot prove usefulness on real
applications).

> And the third topic - shutting (or putting into low power) entire device
> upon request from userspace. This again has much wider auditory than
> gpio-keys, or input devices layer for that matter. We may want to do so
> for other types of devices as well. That is why the question when to
> general PM list.

Yeah.

-- 
Best Regards,
Artem Bityutskiy (Артём Битюцкий)

--
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: [linux-pm] [PATCH v2 1/2] Input: gpio-keys - allow platform to specify exact irq flags

[Dmitry Torokhov]

On Wed, Dec 09, 2009 at 09:31:00AM +0200, Artem Bityutskiy wrote:
> On Tue, 2009-12-08 at 09:42 -0800, Dmitry Torokhov wrote:
> > > > I also see that gpio-keys is quite different in the sence that it can
> > > > shut off buttons selectively. I fact, at the moment every button can be
> > > > considered a separate device... But that would be too much overhead.
> > > > 
> > > > They could probably split the keys into 2 groups (critical that should
> > > > be always active) and not critical, that could be shut off, but I think
> > > > they want teh flexibility of controlling this at runtime instead of
> > > > doing it in board data.
> > > 
> > > I suggested including this into the "abstract input device" model, but
> > > you refuse this. But I still think it is a good idea.
> > > 
> > > Indeed, if we look at an input device as at something abstract which has
> > > many keys, why we cannot assume that separate keys can be
> > > enabled/disabled? Just imagine you have a very advanced keybord :-) And
> > > we simply implement an ioctl which enables/disables a specific key. The
> > > generic layers just pass this ioctl down to the lower lever drivers. If
> > > the specific input device or driver support it - fine, if not - it
> > > returns -EINVAL or something like that.
> > 
> > I refuse it because it will be supported by exactly 1 driver in the
> > kernel - gpio-keys. It is the only driver that allows shut half of the
> > "device" (because in reality it is a group of disjoint devices). It is
> > the only case when "muting" a button means that IRQ is shut off abnd
> > thus CPU can continue to sleep if that button is pressed. For all other
> > devices that have 1 inettrupt per device, you still have to wake up,
> > because you don't know whether the button that generated event is
> > "important" or not.
> 
> Fair enough.
> 
> > Now, there is a issue of waking up userspace task, additional scheduling
> > and keeping CPU running longer than necessary for "uninteresting" keys.
> > This can be solved by implementing a subscription model which allows
> > filtering uninteresing events on a per-client basis at evdev level.
> 
> Right. And for gpio_keys, this would be dine on the driver level.

But the semantics are different - if done on driver level you'd be
affecting _all_  consumers of the device; what I want to be done only
affects owner of the file descriptor.

-- 
Dmitry

Re: [linux-pm] [PATCH v2 1/2] Input: gpio-keys - allow platform to specify exact irq flags

[Pavel Machek]

> > > I refuse it because it will be supported by exactly 1 driver in the
> > > kernel - gpio-keys. It is the only driver that allows shut half of the
> > > "device" (because in reality it is a group of disjoint devices). It is
> > > the only case when "muting" a button means that IRQ is shut off abnd
> > > thus CPU can continue to sleep if that button is pressed. For all other
> > > devices that have 1 inettrupt per device, you still have to wake up,
> > > because you don't know whether the button that generated event is
> > > "important" or not.
> > 
> > Fair enough.
> > 
> > > Now, there is a issue of waking up userspace task, additional scheduling
> > > and keeping CPU running longer than necessary for "uninteresting" keys.
> > > This can be solved by implementing a subscription model which allows
> > > filtering uninteresing events on a per-client basis at evdev level.
> > 
> > Right. And for gpio_keys, this would be dine on the driver level.
> 
> But the semantics are different - if done on driver level you'd be
> affecting _all_  consumers of the device; what I want to be done only
> affects owner of the file descriptor.

Well, if _all_ consumers decide to ignore some key, we should be able
to ignore it at driver level.

And actually it may make some sense -- I do not think disabling irq
during runtime is worth the effort, but disabling wakeup source and
getting rid of unneccessary wakeup when system is suspended is
probably worth it.

									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [linux-pm] [PATCH v2 1/2] Input: gpio-keys - allow platform to specify exact irq flags

[Dmitry Torokhov]

On Wed, Dec 09, 2009 at 10:08:16PM +0100, Pavel Machek wrote:
> > > > I refuse it because it will be supported by exactly 1 driver in the
> > > > kernel - gpio-keys. It is the only driver that allows shut half of the
> > > > "device" (because in reality it is a group of disjoint devices). It is
> > > > the only case when "muting" a button means that IRQ is shut off abnd
> > > > thus CPU can continue to sleep if that button is pressed. For all other
> > > > devices that have 1 inettrupt per device, you still have to wake up,
> > > > because you don't know whether the button that generated event is
> > > > "important" or not.
> > > 
> > > Fair enough.
> > > 
> > > > Now, there is a issue of waking up userspace task, additional scheduling
> > > > and keeping CPU running longer than necessary for "uninteresting" keys.
> > > > This can be solved by implementing a subscription model which allows
> > > > filtering uninteresing events on a per-client basis at evdev level.
> > > 
> > > Right. And for gpio_keys, this would be dine on the driver level.
> > 
> > But the semantics are different - if done on driver level you'd be
> > affecting _all_  consumers of the device; what I want to be done only
> > affects owner of the file descriptor.
> 
> Well, if _all_ consumers decide to ignore some key, we should be able
> to ignore it at driver level.

The intesection of drivers allowing shut off individual buttons with
all consumers agreeing on not using a key would be pretty miniscule.

> 
> And actually it may make some sense -- I do not think disabling irq
> during runtime is worth the effort, but disabling wakeup source and
> getting rid of unneccessary wakeup when system is suspended is
> probably worth it.
> 

I don't believe Mika's patches touched wakeup in any way. So it has
been strictly about wakig up processor to service that interrupt so far.

-- 
Dmitry

Re: [linux-pm] [PATCH v2 1/2] Input: gpio-keys - allow platform to specify exact irq flags

[Pavel Machek]

Hi!

> > > > > Now, there is a issue of waking up userspace task, additional scheduling
> > > > > and keeping CPU running longer than necessary for "uninteresting" keys.
> > > > > This can be solved by implementing a subscription model which allows
> > > > > filtering uninteresing events on a per-client basis at evdev level.
> > > > 
> > > > Right. And for gpio_keys, this would be dine on the driver level.
> > > 
> > > But the semantics are different - if done on driver level you'd be
> > > affecting _all_  consumers of the device; what I want to be done only
> > > affects owner of the file descriptor.
> > 
> > Well, if _all_ consumers decide to ignore some key, we should be able
> > to ignore it at driver level.
> 
> The intesection of drivers allowing shut off individual buttons with
> all consumers agreeing on not using a key would be pretty miniscule.

Well; people want to do that on specialized hw/embedded sw. It could
work there. 

> > And actually it may make some sense -- I do not think disabling irq
> > during runtime is worth the effort, but disabling wakeup source and
> > getting rid of unneccessary wakeup when system is suspended is
> > probably worth it.
> 
> I don't believe Mika's patches touched wakeup in any way. So it has
> been strictly about wakig up processor to service that interrupt so far.

I think it makes sense for wakeups; I doubt it makes sense for runtime
- the power savings are just not big enough.

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [linux-pm] [PATCH v2 1/2] Input: gpio-keys - allow platform to specify exact irq flags

[Artem Bityutskiy]

On Wed, 2009-12-09 at 10:03 -0800, Dmitry Torokhov wrote:
> On Wed, Dec 09, 2009 at 09:31:00AM +0200, Artem Bityutskiy wrote:
> > On Tue, 2009-12-08 at 09:42 -0800, Dmitry Torokhov wrote:
> > > > > I also see that gpio-keys is quite different in the sence that it can
> > > > > shut off buttons selectively. I fact, at the moment every button can be
> > > > > considered a separate device... But that would be too much overhead.
> > > > > 
> > > > > They could probably split the keys into 2 groups (critical that should
> > > > > be always active) and not critical, that could be shut off, but I think
> > > > > they want teh flexibility of controlling this at runtime instead of
> > > > > doing it in board data.
> > > > 
> > > > I suggested including this into the "abstract input device" model, but
> > > > you refuse this. But I still think it is a good idea.
> > > > 
> > > > Indeed, if we look at an input device as at something abstract which has
> > > > many keys, why we cannot assume that separate keys can be
> > > > enabled/disabled? Just imagine you have a very advanced keybord :-) And
> > > > we simply implement an ioctl which enables/disables a specific key. The
> > > > generic layers just pass this ioctl down to the lower lever drivers. If
> > > > the specific input device or driver support it - fine, if not - it
> > > > returns -EINVAL or something like that.
> > > 
> > > I refuse it because it will be supported by exactly 1 driver in the
> > > kernel - gpio-keys. It is the only driver that allows shut half of the
> > > "device" (because in reality it is a group of disjoint devices). It is
> > > the only case when "muting" a button means that IRQ is shut off abnd
> > > thus CPU can continue to sleep if that button is pressed. For all other
> > > devices that have 1 inettrupt per device, you still have to wake up,
> > > because you don't know whether the button that generated event is
> > > "important" or not.
> > 
> > Fair enough.
> > 
> > > Now, there is a issue of waking up userspace task, additional scheduling
> > > and keeping CPU running longer than necessary for "uninteresting" keys.
> > > This can be solved by implementing a subscription model which allows
> > > filtering uninteresing events on a per-client basis at evdev level.
> > 
> > Right. And for gpio_keys, this would be dine on the driver level.
> 
> But the semantics are different - if done on driver level you'd be
> affecting _all_  consumers of the device; what I want to be done only
> affects owner of the file descriptor.

OK, makes sense. I'm convinced, thanks!

-- 
Best Regards,
Artem Bityutskiy (Артём Битюцкий)

--
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: [linux-pm] [PATCH v2 1/2] Input: gpio-keys - allow platform to specify exact irq flags

[Pavel Machek]

Hi!

> > Yeah, that would be nice. But it won't work for us :(
> >
> > This is because the actual input device might be open for several
> > applications. Then we have single process which controls state of
> > the device. Now if, for example the device is locked by user, this
> > process just disables those buttons which are not allowed to wake
> > up the device and blanks the screen.
> >
> > So the input device is still open but we just prevent GPIO lines
> > from generating any interrupts while the device is locked. There
> > are other use-cases also where different buttons are
> > disabled/enabled.
> 
> Hi,
> 
> I thought we'd better ask our friends over at linux-pm, if they've got
> some interface for this task.  To summarize: an embedded application
> wants to go into a "locked" state, where some input devices (gpio keys
> in this case) are "muted", ie. they don't even generate interrupts to
> minimize power consumption.  This could be solved by adding a new
> interface to gpio-keys, but the problem seems more general, so I wonder

Is it so bright idea? How much power does it save?
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [PATCH 1/2] Input: gpio-keys: allow drivers to specify whether IRQ can be shared

[Dmitry Torokhov]

Hi Mika,

On Thu, Nov 19, 2009 at 09:23:45AM +0200, Mika Westerberg wrote:
> Added new field to struct gpio_keys_button: exclusive_irq which can
> be used to specify whether driver wants to have exclusive access to
> IRQ or can it be shared. By default IRQs for gpio-keys are shareable.
> 

I think the idea makes sense with one exception - why limit just to
sharing and not let platform specify the exact IRQ flags it needs,
defaulting to IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING | IRQF_SHARED?

-- 
Dmitry

Re: [PATCH 1/2] Input: gpio-keys: allow drivers to specify whether IRQ can be shared

[Ferenc Wagner]

Dmitry Torokhov <dmitry.torokhov@gmail.com> writes:

> On Thu, Nov 19, 2009 at 09:23:45AM +0200, Mika Westerberg wrote:
>
>> Added new field to struct gpio_keys_button: exclusive_irq which can
>> be used to specify whether driver wants to have exclusive access to
>> IRQ or can it be shared. By default IRQs for gpio-keys are shareable.
>
> I think the idea makes sense with one exception - why limit just to
> sharing and not let platform specify the exact IRQ flags it needs,
> defaulting to IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING | IRQF_SHARED?

Though I'm all for level triggered GPIO buttons, that would hardly fly
without changes in the handler logic.  And for the necessary change of
polarity one needs GPIO access, and we're back to Arve's as-yet-
unanswered question: "If gpio_get_value may sleep, then what is
gpio_get_value_cansleep for?"  There was also talk about using the
nested interrupt infrastructure, but I didn't really get you, and the
thread died off without conclusion:
http://thread.gmane.org/gmane.linux.kernel.input/8775/focus=9153
I'd appreciate some explanation, if you can afford the time.
-- 
Regards,
Feri.

[RFC PATCH 0/2] Input: adding new ioctl()s for enabling/disabling events

[Mika Westerberg]

Hi Dmitry,

This patchset is second proposal to get selected gpio lines
enabled/disabled with gpio-keys. Basically we provide 2 new ioctl()s
that can be used by userland.  Lower-level input drivers (e.g gpio-keys)
can choose to implement these when needed.

Thanks,
MW

Mika Westerberg (2):
  Input: added 2 new ioctl()s for setting/getting event state
  Input: gpio-keys: implemented support for enabling/disabling gpios

 drivers/input/evdev.c              |   19 ++++++
 drivers/input/input.c              |  115 ++++++++++++++++++++++++++++++++++++
 drivers/input/keyboard/gpio_keys.c |   75 +++++++++++++++++++++++
 include/linux/input.h              |   44 ++++++++++++++
 4 files changed, 253 insertions(+), 0 deletions(-)

[RFC PATCH 1/2] Input: added 2 new ioctl()s for setting/getting event state

[Mika Westerberg]

From: Mika Westerberg <ext-mika.1.westerberg@nokia.com>

Added 2 new ioctl()s for the input layer:

	EVIOCGSTATE - gets input event state from the driver
	EVIOCSSTATE - sets input event state

These ioctls(), when implemented in lower-level driver, can be used
to change state of generated events (for example to disable/enable
those).

Signed-off-by: Mika Westerberg <ext-mika.1.westerberg@nokia.com>
---
 drivers/input/evdev.c |   19 ++++++++
 drivers/input/input.c |  115 +++++++++++++++++++++++++++++++++++++++++++++++++
 include/linux/input.h |   44 +++++++++++++++++++
 3 files changed, 178 insertions(+), 0 deletions(-)

diff --git a/drivers/input/evdev.c b/drivers/input/evdev.c
index dee6706..7abaf16 100644
--- a/drivers/input/evdev.c
+++ b/drivers/input/evdev.c
@@ -510,6 +510,7 @@ static long evdev_do_ioctl(struct file *file, unsigned int cmd,
 	struct evdev *evdev = client->evdev;
 	struct input_dev *dev = evdev->handle.dev;
 	struct input_absinfo abs;
+	struct input_event_state st;
 	struct ff_effect effect;
 	int __user *ip = (int __user *)p;
 	int i, t, u, v;
@@ -566,6 +567,24 @@ static long evdev_do_ioctl(struct file *file, unsigned int cmd,
 
 		return input_set_keycode(dev, t, v);
 
+	case EVIOCGSTATE:
+		if (copy_from_user(&st, p, sizeof(st)))
+			return -EFAULT;
+
+		error = input_get_event_state(dev, &st);
+		if (error)
+			return error;
+
+		if (copy_to_user(p, &st, sizeof(st)))
+			return -EFAULT;
+
+		return 0;
+
+	case EVIOCSSTATE:
+		if (copy_from_user(&st, p, sizeof(st)))
+			return -EFAULT;
+		return input_set_event_state(dev, &st);
+
 	case EVIOCRMFF:
 		return input_ff_erase(dev, (int)(unsigned long) p, file);
 
diff --git a/drivers/input/input.c b/drivers/input/input.c
index cc763c9..01db55e 100644
--- a/drivers/input/input.c
+++ b/drivers/input/input.c
@@ -690,6 +690,121 @@ int input_set_keycode(struct input_dev *dev, int scancode, int keycode)
 }
 EXPORT_SYMBOL(input_set_keycode);
 
+/**
+ * input_validate_event_state - validates event state
+ * @dev: input device whose event state is being validated
+ * @st: input event state which is to be validated
+ *
+ * This function checks whether contents of given event state
+ * structure (@st) is valid for @dev (i.e it is supported).
+ * Returns 0 when @st is valid, negative error otherwise.
+ *
+ * Function accesses internals of @dev so make sure that
+ * @dev->event_lock is locked.
+ */
+static int input_validate_event_state(struct input_dev *dev,
+				      const struct input_event_state *st)
+{
+	unsigned long *bits;
+	unsigned int max;
+
+	BUG_ON(st == NULL);
+
+	/* for now, we support only EV_KEY and EV_SW */
+	switch (st->type) {
+	case EV_KEY:
+		bits = dev->keybit;
+		max = KEY_MAX;
+		break;
+	case EV_SW:
+		bits = dev->swbit;
+		max = SW_MAX;
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	if (!is_event_supported(st->code, bits, max))
+		return -EINVAL;
+
+	return 0;
+}
+
+/**
+ * input_get_event_state - gets current state of given input event
+ * @dev: device whose event state is being get
+ * @st: buffer where event state is stored
+ *
+ * This function is used to get state of given event.  Event is identified
+ * by {@st->code, @st->type} tuple.  On success function returns 0, negative
+ * error in case of failure.
+ */
+int input_get_event_state(struct input_dev *dev,
+			  struct input_event_state *st)
+{
+	unsigned long flags;
+	int error;
+
+	if (dev->get_event_state == NULL)
+		return -ENOTTY;
+
+	spin_lock_irqsave(&dev->event_lock, flags);
+
+	error = input_validate_event_state(dev, st);
+	if (error)
+		goto out;
+
+	error = dev->get_event_state(dev, st);
+out:
+	spin_unlock_irqrestore(&dev->event_lock, flags);
+
+	return error;
+
+}
+EXPORT_SYMBOL(input_get_event_state);
+
+/**
+ * input_set_event_state - sets state of given input event
+ * @dev: device which event is being set
+ * @st: new event state
+ *
+ * Input event state is controlled by this function.  Caller must pass valid
+ * @st to the function (@st->code, @st->type and @st->state should be correct).
+ * For example one can disable input event (if this is supported by underlying
+ * driver by doing following:
+ *      new_state->state = EVENT_STATE_DISABLE;
+ * 	error = input_set_event_state(dev, new_state);
+ * 	if (error)
+ *		...
+ * Function calls the actual implementation function provided by the driver
+ * with @dev->event_lock locked so it cannot sleep.
+ *
+ * If function fails, returns negative error number or 0 on success.
+ */
+int input_set_event_state(struct input_dev *dev,
+			  const struct input_event_state *st)
+{
+	unsigned long flags;
+	int error;
+
+	if (dev->set_event_state == NULL)
+		return -ENOTTY;
+
+	spin_lock_irqsave(&dev->event_lock, flags);
+
+	error = input_validate_event_state(dev, st);
+	if (error)
+		goto out;
+
+	error = dev->set_event_state(dev, st);
+out:
+	spin_unlock_irqrestore(&dev->event_lock, flags);
+
+	return error;
+
+}
+EXPORT_SYMBOL(input_set_event_state);
+
 #define MATCH_BIT(bit, max) \
 		for (i = 0; i < BITS_TO_LONGS(max); i++) \
 			if ((id->bit[i] & dev->bit[i]) != id->bit[i]) \
diff --git a/include/linux/input.h b/include/linux/input.h
index 9a04e26..0230c29 100644
--- a/include/linux/input.h
+++ b/include/linux/input.h
@@ -56,6 +56,37 @@ struct input_absinfo {
 	__s32 resolution;
 };
 
+/**
+ * struct input_event_state - struct used to set/get input event state
+ * @type: input event type (EV_KEY, EV_SW, ..) [user supplied]
+ * @code: input event code [user supplied]
+ * @state: state of the input event (see EVENT_STATE_* below)
+ * @pad: zero padding
+ *
+ * Generated events can have state if underlying event driver supports it.
+ * This way it is possible for example to ask hardware to disable given
+ * event from generating interrupts.  In future there might be some other
+ * states event can have (currently only enable/disable is supported).
+ *
+ * This structure is passed to lower-level input event driver to change
+ * @state of given input event.  Event is identified by {@type, @code} tuple.
+ *
+ * See functions:
+ *	input_get_event_state()
+ *	input_set_event_state()
+ *
+ * for more information.
+ */
+struct input_event_state {
+	__u16	type;
+	__u16	code;
+	__u8	state;
+	__u8	pad[3];
+};
+
+#define EVENT_STATE_DISABLE	0	/* event is disabled */
+#define EVENT_STATE_ENABLE	1	/* event is enabled */
+
 #define EVIOCGVERSION		_IOR('E', 0x01, int)			/* get driver version */
 #define EVIOCGID		_IOR('E', 0x02, struct input_id)	/* get device ID */
 #define EVIOCGREP		_IOR('E', 0x03, int[2])			/* get repeat settings */
@@ -67,6 +98,9 @@ struct input_absinfo {
 #define EVIOCGPHYS(len)		_IOC(_IOC_READ, 'E', 0x07, len)		/* get physical location */
 #define EVIOCGUNIQ(len)		_IOC(_IOC_READ, 'E', 0x08, len)		/* get unique identifier */
 
+#define EVIOCGSTATE		_IOR('E', 0x10, struct input_event_state) /* get event state */
+#define EVIOCSSTATE		_IOW('E', 0x11, struct input_event_state) /* set event state */
+
 #define EVIOCGKEY(len)		_IOC(_IOC_READ, 'E', 0x18, len)		/* get global keystate */
 #define EVIOCGLED(len)		_IOC(_IOC_READ, 'E', 0x19, len)		/* get all LEDs */
 #define EVIOCGSND(len)		_IOC(_IOC_READ, 'E', 0x1a, len)		/* get all sounds status */
@@ -1024,6 +1058,8 @@ struct ff_effect {
  *	sparse keymaps. If not supplied default mechanism will be used
  * @getkeycode: optional method to retrieve current keymap. If not supplied
  *	default mechanism will be used
+ * @get_event_state: optional method to retrieve state of single event
+ * @set_event_state: optional method to alter state of single event
  * @ff: force feedback structure associated with the device if device
  *	supports force feedback effects
  * @repeat_key: stores key code of the last key pressed; used to implement
@@ -1095,6 +1131,10 @@ struct input_dev {
 	void *keycode;
 	int (*setkeycode)(struct input_dev *dev, int scancode, int keycode);
 	int (*getkeycode)(struct input_dev *dev, int scancode, int *keycode);
+	int (*get_event_state)(struct input_dev *dev,
+			       struct input_event_state *st);
+	int (*set_event_state)(struct input_dev *dev,
+			       const struct input_event_state *st);
 
 	struct ff_device *ff;
 
@@ -1357,6 +1397,10 @@ static inline void input_set_abs_params(struct input_dev *dev, int axis, int min
 
 int input_get_keycode(struct input_dev *dev, int scancode, int *keycode);
 int input_set_keycode(struct input_dev *dev, int scancode, int keycode);
+int input_get_event_state(struct input_dev *dev,
+			  struct input_event_state *st);
+int input_set_event_state(struct input_dev *dev,
+			  const struct input_event_state *st);
 
 extern struct class input_class;
 
-- 
1.5.6.5

[RFC PATCH 2/2] Input: gpio-keys: implemented support for enabling/disabling gpios

[Mika Westerberg]

From: Mika Westerberg <ext-mika.1.westerberg@nokia.com>

Add support for disabling and enabling gpio lines separately using
ioctl().

This patch is build using 2 new input event ioctl()s: EVIOCGSTATE
and EVIOCSSTATE so that patch is needed before applying this.

Signed-off-by: Mika Westerberg <ext-mika.1.westerberg@nokia.com>
---
 drivers/input/keyboard/gpio_keys.c |   75 ++++++++++++++++++++++++++++++++++++
 1 files changed, 75 insertions(+), 0 deletions(-)

diff --git a/drivers/input/keyboard/gpio_keys.c b/drivers/input/keyboard/gpio_keys.c
index 8941a8b..d9b492e 100644
--- a/drivers/input/keyboard/gpio_keys.c
+++ b/drivers/input/keyboard/gpio_keys.c
@@ -30,10 +30,12 @@ struct gpio_button_data {
 	struct input_dev *input;
 	struct timer_list timer;
 	struct work_struct work;
+	u8 state;	/* event state */
 };
 
 struct gpio_keys_drvdata {
 	struct input_dev *input;
+	struct gpio_keys_platform_data *pdata;
 	struct gpio_button_data data[0];
 };
 
@@ -73,6 +75,69 @@ static irqreturn_t gpio_keys_isr(int irq, void *dev_id)
 	return IRQ_HANDLED;
 }
 
+static int gpio_keys_get_event_state(struct input_dev *dev,
+				     struct input_event_state *st)
+{
+	const struct gpio_keys_drvdata *ddata = input_get_drvdata(dev);
+	const struct gpio_keys_platform_data *pdata = ddata->pdata;
+	int i;
+
+	for (i = 0; i < pdata->nbuttons; i++) {
+		const struct gpio_keys_button *button = &pdata->buttons[i];
+		const struct gpio_button_data *bdata = &ddata->data[i];
+
+		if (button->code == st->code && button->type == st->type) {
+			st->state = bdata->state;
+			return 0;
+		}
+	}
+	return -EINVAL;
+}
+
+/*
+ * This function is used to enable/disable hardware interrupts from
+ * GPIO lines that are aggregated into single gpio-keys input device.
+ */
+static int gpio_keys_set_event_state(struct input_dev *dev,
+				     const struct input_event_state *st)
+{
+	struct gpio_keys_drvdata *ddata = input_get_drvdata(dev);
+	struct gpio_keys_platform_data *pdata = ddata->pdata;
+	int i;
+
+	for (i = 0; i < pdata->nbuttons; i++) {
+		struct gpio_keys_button *button = &pdata->buttons[i];
+		struct gpio_button_data *bdata = &ddata->data[i];
+
+		if (button->code == st->code && button->type == st->type) {
+			switch (st->state) {
+			case EVENT_STATE_DISABLE:
+				if (bdata->state == EVENT_STATE_ENABLE) {
+					bdata->state = EVENT_STATE_DISABLE;
+					/*
+					 * Disable physical irq line.  This is
+					 * enough also for keeping device from
+					 * waking up during sleep so no need
+					 * to change wakeup flags for this irq.
+					 */
+					disable_irq(gpio_to_irq(button->gpio));
+				}
+				break;
+			case EVENT_STATE_ENABLE:
+				if (bdata->state == EVENT_STATE_DISABLE) {
+					bdata->state = EVENT_STATE_ENABLE;
+					enable_irq(gpio_to_irq(button->gpio));
+				}
+				break;
+			default:
+				return -EINVAL;
+			}
+			return 0;
+		}
+	}
+	return -EINVAL;
+}
+
 static int __devinit gpio_keys_setup_key(struct device *dev,
 					 struct gpio_button_data *bdata,
 					 struct gpio_keys_button *button)
@@ -154,11 +219,19 @@ static int __devinit gpio_keys_probe(struct platform_device *pdev)
 	input->id.product = 0x0001;
 	input->id.version = 0x0100;
 
+	/*
+	 * We support setting/getting event state ioctl.
+	 */
+	input->get_event_state = gpio_keys_get_event_state;
+	input->set_event_state = gpio_keys_set_event_state;
+	input_set_drvdata(input, ddata);
+
 	/* Enable auto repeat feature of Linux input subsystem */
 	if (pdata->rep)
 		__set_bit(EV_REP, input->evbit);
 
 	ddata->input = input;
+	ddata->pdata = pdata;
 
 	for (i = 0; i < pdata->nbuttons; i++) {
 		struct gpio_keys_button *button = &pdata->buttons[i];
@@ -167,6 +240,7 @@ static int __devinit gpio_keys_probe(struct platform_device *pdev)
 
 		bdata->input = input;
 		bdata->button = button;
+		bdata->state = EVENT_STATE_ENABLE;	/* enabled by default */
 
 		error = gpio_keys_setup_key(dev, bdata, button);
 		if (error)
@@ -200,6 +274,7 @@ static int __devinit gpio_keys_probe(struct platform_device *pdev)
 
 	platform_set_drvdata(pdev, NULL);
  fail1:
+	input_set_drvdata(input, NULL);
 	input_free_device(input);
 	kfree(ddata);
 
-- 
1.5.6.5

Re: [RFC PATCH 2/2] Input: gpio-keys: implemented support for enabling/disabling gpios

[Ferenc Wagner]

Mika Westerberg <ext-mika.1.westerberg@nokia.com> writes:

> +	for (i = 0; i < pdata->nbuttons; i++) {
> +		struct gpio_keys_button *button = &pdata->buttons[i];
> +		struct gpio_button_data *bdata = &ddata->data[i];
> +
> +		if (button->code == st->code && button->type == st->type) {
> +			switch (st->state) {
> +			case EVENT_STATE_DISABLE:
> +				if (bdata->state == EVENT_STATE_ENABLE) {
> +					bdata->state = EVENT_STATE_DISABLE;
> +					/*
> +					 * Disable physical irq line.  This is
> +					 * enough also for keeping device from
> +					 * waking up during sleep so no need
> +					 * to change wakeup flags for this irq.
> +					 */
> +					disable_irq(gpio_to_irq(button->gpio));

Hi Mika,

Did you consider what would happen when several GPIO buttons share a
single IRQ?  The current code does not support this (it uses IRQF_SHARED
for a different reason), but this is very much possible: see for example
http://thread.gmane.org/gmane.linux.kernel.input/8775 for a related
thread which unfortunately died off without conclusion (but the patch
went in).
-- 
Thanks,
Feri.

Re: [RFC PATCH 2/2] Input: gpio-keys: implemented support for enabling/disabling gpios

[Mika Westerberg]

On Wed, Nov 11, 2009 at 03:37:38PM +0100, ext Ferenc Wagner wrote:
> Mika Westerberg <ext-mika.1.westerberg@nokia.com> writes:
> 
> > +	for (i = 0; i < pdata->nbuttons; i++) {
> > +		struct gpio_keys_button *button = &pdata->buttons[i];
> > +		struct gpio_button_data *bdata = &ddata->data[i];
> > +
> > +		if (button->code == st->code && button->type == st->type) {
> > +			switch (st->state) {
> > +			case EVENT_STATE_DISABLE:
> > +				if (bdata->state == EVENT_STATE_ENABLE) {
> > +					bdata->state = EVENT_STATE_DISABLE;
> > +					/*
> > +					 * Disable physical irq line.  This is
> > +					 * enough also for keeping device from
> > +					 * waking up during sleep so no need
> > +					 * to change wakeup flags for this irq.
> > +					 */
> > +					disable_irq(gpio_to_irq(button->gpio));
> 
> Hi Mika,
> 
> Did you consider what would happen when several GPIO buttons share a
> single IRQ?  The current code does not support this (it uses IRQF_SHARED
> for a different reason), but this is very much possible: see for example
> http://thread.gmane.org/gmane.linux.kernel.input/8775 for a related
> thread which unfortunately died off without conclusion (but the patch
> went in).

Good point!  I didn't consider that at all. 

Maybe we disable irq only when it is not shared with anything else (even
with other GPIO button) and otherwise return -EINVAL or something like
that?

Thanks,
MW

Re: [RFC PATCH 2/2] Input: gpio-keys: implemented support for enabling/disabling gpios

[Dmitry Torokhov]

On Wed, Nov 11, 2009 at 04:52:06PM +0200, Mika Westerberg wrote:
> On Wed, Nov 11, 2009 at 03:37:38PM +0100, ext Ferenc Wagner wrote:
> > Mika Westerberg <ext-mika.1.westerberg@nokia.com> writes:
> > 
> > > +	for (i = 0; i < pdata->nbuttons; i++) {
> > > +		struct gpio_keys_button *button = &pdata->buttons[i];
> > > +		struct gpio_button_data *bdata = &ddata->data[i];
> > > +
> > > +		if (button->code == st->code && button->type == st->type) {
> > > +			switch (st->state) {
> > > +			case EVENT_STATE_DISABLE:
> > > +				if (bdata->state == EVENT_STATE_ENABLE) {
> > > +					bdata->state = EVENT_STATE_DISABLE;
> > > +					/*
> > > +					 * Disable physical irq line.  This is
> > > +					 * enough also for keeping device from
> > > +					 * waking up during sleep so no need
> > > +					 * to change wakeup flags for this irq.
> > > +					 */
> > > +					disable_irq(gpio_to_irq(button->gpio));
> > 
> > Hi Mika,
> > 
> > Did you consider what would happen when several GPIO buttons share a
> > single IRQ?  The current code does not support this (it uses IRQF_SHARED
> > for a different reason), but this is very much possible: see for example
> > http://thread.gmane.org/gmane.linux.kernel.input/8775 for a related
> > thread which unfortunately died off without conclusion (but the patch
> > went in).
> 
> Good point!  I didn't consider that at all. 
> 
> Maybe we disable irq only when it is not shared with anything else (even
> with other GPIO button) and otherwise return -EINVAL or something like
> that?
> 

How could a driver know if it is [going to] share irq with another
driver. Dmitry's (the other one ;) ) patch was needed because he wanted
to share IRQ with another device entirely.

-- 
Dmitry

Re: [RFC PATCH 2/2] Input: gpio-keys: implemented support for enabling/disabling gpios

[Mika Westerberg]

On Wed, Nov 11, 2009 at 06:08:49PM +0100, ext Dmitry Torokhov wrote:
> On Wed, Nov 11, 2009 at 04:52:06PM +0200, Mika Westerberg wrote:
> > On Wed, Nov 11, 2009 at 03:37:38PM +0100, ext Ferenc Wagner wrote:
> > > Mika Westerberg <ext-mika.1.westerberg@nokia.com> writes:
> > > 
> > > > +	for (i = 0; i < pdata->nbuttons; i++) {
> > > > +		struct gpio_keys_button *button = &pdata->buttons[i];
> > > > +		struct gpio_button_data *bdata = &ddata->data[i];
> > > > +
> > > > +		if (button->code == st->code && button->type == st->type) {
> > > > +			switch (st->state) {
> > > > +			case EVENT_STATE_DISABLE:
> > > > +				if (bdata->state == EVENT_STATE_ENABLE) {
> > > > +					bdata->state = EVENT_STATE_DISABLE;
> > > > +					/*
> > > > +					 * Disable physical irq line.  This is
> > > > +					 * enough also for keeping device from
> > > > +					 * waking up during sleep so no need
> > > > +					 * to change wakeup flags for this irq.
> > > > +					 */
> > > > +					disable_irq(gpio_to_irq(button->gpio));
> > > 
> > > Hi Mika,
> > > 
> > > Did you consider what would happen when several GPIO buttons share a
> > > single IRQ?  The current code does not support this (it uses IRQF_SHARED
> > > for a different reason), but this is very much possible: see for example
> > > http://thread.gmane.org/gmane.linux.kernel.input/8775 for a related
> > > thread which unfortunately died off without conclusion (but the patch
> > > went in).
> > 
> > Good point!  I didn't consider that at all. 
> > 
> > Maybe we disable irq only when it is not shared with anything else (even
> > with other GPIO button) and otherwise return -EINVAL or something like
> > that?
> > 
> 
> How could a driver know if it is [going to] share irq with another
> driver. Dmitry's (the other one ;) ) patch was needed because he wanted
> to share IRQ with another device entirely.

Ah, it seems that there is no such API in the kernel :(

How about specifying whether some GPIO button allows sharing of the IRQ
in board files (e.g amend gpio_keys_button struct)? Then we can disable
only those GPIO buttons where IRQ line is not shared.

Thanks,
MW