All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] input/keyboard: new driver for ADP5520 MFD PMICs
@ 2009-09-17 18:24 Mike Frysinger
  2009-09-20  6:30   ` Andrew Morton
                   ` (2 more replies)
  0 siblings, 3 replies; 20+ messages in thread
From: Mike Frysinger @ 2009-09-17 18:24 UTC (permalink / raw)
  To: linux-input, Dmitry Torokhov
  Cc: linux-kernel, uclinux-dist-devel, Michael Hennerich, Bryan Wu

From: Michael Hennerich <michael.hennerich@analog.com>

Signed-off-by: Michael Hennerich <michael.hennerich@analog.com>
Signed-off-by: Bryan Wu <cooloney@kernel.org>
Signed-off-by: Mike Frysinger <vapier@gentoo.org>
---
 drivers/input/keyboard/Kconfig        |   10 ++
 drivers/input/keyboard/Makefile       |    1 +
 drivers/input/keyboard/adp5520-keys.c |  219 +++++++++++++++++++++++++++++++++
 3 files changed, 230 insertions(+), 0 deletions(-)
 create mode 100644 drivers/input/keyboard/adp5520-keys.c

diff --git a/drivers/input/keyboard/Kconfig b/drivers/input/keyboard/Kconfig
index e0bfcd7..53c9561 100644
--- a/drivers/input/keyboard/Kconfig
+++ b/drivers/input/keyboard/Kconfig
@@ -392,4 +392,14 @@ config KEYBOARD_ADP5588
 	  To compile this driver as a module, choose M here: the
 	  module will be called adp5588-keys.
 
+config KEYBOARD_ADP5520
+	tristate "Keypad Support for ADP5520 PMIC"
+	depends on PMIC_ADP5520
+	help
+	  This option enables support for the keypad scan matrix
+	  on Analog Devices ADP5520 PMICs.
+
+	  To compile this driver as a module, choose M here: the module will
+	  be called adp5520-keys.
+
 endif
diff --git a/drivers/input/keyboard/Makefile b/drivers/input/keyboard/Makefile
index 72082a4..c541489 100644
--- a/drivers/input/keyboard/Makefile
+++ b/drivers/input/keyboard/Makefile
@@ -5,6 +5,7 @@
 # Each configuration option enables a list of files.
 
 obj-$(CONFIG_KEYBOARD_AAED2000)		+= aaed2000_kbd.o
+obj-$(CONFIG_KEYBOARD_ADP5520)		+= adp5520-keys.o
 obj-$(CONFIG_KEYBOARD_ADP5588)		+= adp5588-keys.o
 obj-$(CONFIG_KEYBOARD_AMIGA)		+= amikbd.o
 obj-$(CONFIG_KEYBOARD_ATARI)		+= atakbd.o
diff --git a/drivers/input/keyboard/adp5520-keys.c b/drivers/input/keyboard/adp5520-keys.c
new file mode 100644
index 0000000..cc2e1ed
--- /dev/null
+++ b/drivers/input/keyboard/adp5520-keys.c
@@ -0,0 +1,219 @@
+/*
+ * Keypad driver for Analog Devices ADP5520 MFD PMICs
+ *
+ * Copyright 2009 Analog Devices Inc.
+ *
+ * Licensed under the GPL-2 or later.
+ */
+
+#include <linux/module.h>
+#include <linux/kernel.h>
+#include <linux/init.h>
+#include <linux/platform_device.h>
+#include <linux/input.h>
+#include <linux/mfd/adp5520.h>
+
+struct adp5520_keys {
+	struct input_dev *input;
+	struct notifier_block notifier;
+	struct device *master;
+	unsigned short keycode[ADP5520_KEYMAPSIZE];
+};
+
+static void adp5520_keys_report_event(struct adp5520_keys *dev,
+					unsigned short keymask, int value)
+{
+	int i;
+
+	for (i = 0; i < ADP5520_MAXKEYS; i++)
+		if (keymask & (1 << i))
+			input_report_key(dev->input, dev->keycode[i], value);
+
+	input_sync(dev->input);
+}
+
+static int adp5520_keys_notifier(struct notifier_block *nb,
+				 unsigned long event, void *data)
+{
+	struct adp5520_keys *dev;
+	uint8_t reg_val_low, reg_val_high;
+	unsigned short keymask;
+
+	dev = container_of(nb, struct adp5520_keys, notifier);
+
+	if (event & KP_INT) {
+		adp5520_read(dev->master, KP_INT_STAT_1, &reg_val_low);
+		adp5520_read(dev->master, KP_INT_STAT_2, &reg_val_high);
+
+		keymask = (reg_val_high << 8) | reg_val_low;
+		/* Read twice to clear */
+		adp5520_read(dev->master, KP_INT_STAT_1, &reg_val_low);
+		adp5520_read(dev->master, KP_INT_STAT_2, &reg_val_high);
+		keymask |= (reg_val_high << 8) | reg_val_low;
+		adp5520_keys_report_event(dev, keymask, 1);
+	}
+
+	if (event & KR_INT) {
+		adp5520_read(dev->master, KR_INT_STAT_1, &reg_val_low);
+		adp5520_read(dev->master, KR_INT_STAT_2, &reg_val_high);
+
+		keymask = (reg_val_high << 8) | reg_val_low;
+		/* Read twice to clear */
+		adp5520_read(dev->master, KR_INT_STAT_1, &reg_val_low);
+		adp5520_read(dev->master, KR_INT_STAT_2, &reg_val_high);
+		keymask |= (reg_val_high << 8) | reg_val_low;
+		adp5520_keys_report_event(dev, keymask, 0);
+	}
+
+	return 0;
+}
+
+static int __devinit adp5520_keys_probe(struct platform_device *pdev)
+{
+	struct adp5520_keys_platfrom_data *pdata = pdev->dev.platform_data;
+	struct input_dev *input;
+	struct adp5520_keys *dev;
+	int ret, i;
+	unsigned char en_mask, ctl_mask = 0;
+
+	if (pdev->id != ID_ADP5520) {
+		dev_err(&pdev->dev, "only ADP5520 supports Keypad\n");
+		return -EFAULT;
+	}
+
+	if (pdata == NULL) {
+		dev_err(&pdev->dev, "missing platform data\n");
+		return -ENODEV;
+	}
+
+	if (!(pdata->rows_en_mask && pdata->cols_en_mask))
+		return -ENODEV;
+
+	dev = kzalloc(sizeof(*dev), GFP_KERNEL);
+	if (dev == NULL) {
+		dev_err(&pdev->dev, "failed to alloc memory\n");
+		return -ENOMEM;
+	}
+
+	input = input_allocate_device();
+	if (!input) {
+		ret = -ENOMEM;
+		goto err;
+	}
+
+	dev->master = pdev->dev.parent;
+	dev->input = input;
+
+	input->name = pdev->name;
+	input->phys = "adp5520-keys/input0";
+	input->dev.parent = &pdev->dev;
+
+	input_set_drvdata(input, dev);
+
+	input->id.bustype = BUS_I2C;
+	input->id.vendor = 0x0001;
+	input->id.product = 0x5520;
+	input->id.version = 0x0001;
+
+	input->keycodesize = sizeof(unsigned short);
+	input->keycodemax = pdata->keymapsize;
+	input->keycode = dev->keycode;
+
+	memcpy(dev->keycode, pdata->keymap,
+		pdata->keymapsize * input->keycodesize);
+
+	/* setup input device */
+	__set_bit(EV_KEY, input->evbit);
+
+	if (pdata->repeat)
+		__set_bit(EV_REP, input->evbit);
+
+	for (i = 0; i < input->keycodemax; i++)
+		__set_bit(dev->keycode[i] & KEY_MAX, input->keybit);
+	__clear_bit(KEY_RESERVED, input->keybit);
+
+	ret = input_register_device(input);
+	if (ret) {
+		dev_err(&pdev->dev, "unable to register input device\n");
+		input_free_device(input);
+		goto err;
+	}
+
+	en_mask = pdata->rows_en_mask | pdata->cols_en_mask;
+
+	ret = adp5520_set_bits(dev->master, GPIO_CFG_1, en_mask);
+
+	if (en_mask & COL_C3)
+		ctl_mask |= C3_MODE;
+
+	if (en_mask & ROW_R3)
+		ctl_mask |= R3_MODE;
+
+	if (ctl_mask)
+		ret |= adp5520_set_bits(dev->master, LED_CONTROL,
+			ctl_mask);
+
+	ret |= adp5520_set_bits(dev->master, GPIO_PULLUP,
+		pdata->rows_en_mask);
+
+	if (ret) {
+		dev_err(&pdev->dev, "failed to write\n");
+		goto err1;
+	}
+
+	dev->notifier.notifier_call = adp5520_keys_notifier;
+	ret = adp5520_register_notifier(dev->master, &dev->notifier,
+			KP_IEN | KR_IEN);
+	if (ret) {
+		dev_err(&pdev->dev, "failed to register notifier\n");
+		goto err1;
+	}
+
+	platform_set_drvdata(pdev, dev);
+	return 0;
+
+err1:
+	input_unregister_device(input);
+err:
+	kfree(dev);
+	return ret;
+}
+
+static int __devexit adp5520_keys_remove(struct platform_device *pdev)
+{
+	struct adp5520_keys *dev;
+	dev = platform_get_drvdata(pdev);
+
+	adp5520_unregister_notifier(dev->master, &dev->notifier,
+				KP_IEN | KR_IEN);
+
+	input_unregister_device(dev->input);
+	kfree(dev);
+	return 0;
+}
+
+static struct platform_driver adp5520_keys_driver = {
+	.driver	= {
+		.name	= "adp5520-keys",
+		.owner	= THIS_MODULE,
+	},
+	.probe		= adp5520_keys_probe,
+	.remove		= __devexit_p(adp5520_keys_remove),
+};
+
+static int __init adp5520_keys_init(void)
+{
+	return platform_driver_register(&adp5520_keys_driver);
+}
+module_init(adp5520_keys_init);
+
+static void __exit adp5520_keys_exit(void)
+{
+	platform_driver_unregister(&adp5520_keys_driver);
+}
+module_exit(adp5520_keys_exit);
+
+MODULE_AUTHOR("Michael Hennerich <hennerich@blackfin.uclinux.org>");
+MODULE_DESCRIPTION("Keys ADP5520 Driver");
+MODULE_LICENSE("GPL");
+MODULE_ALIAS("platform:adp5520-keys");
-- 
1.6.5.rc1


^ permalink raw reply related	[flat|nested] 20+ messages in thread

* Re: [PATCH] input/keyboard: new driver for ADP5520 MFD PMICs
@ 2009-09-20  6:30   ` Andrew Morton
  0 siblings, 0 replies; 20+ messages in thread
From: Andrew Morton @ 2009-09-20  6:30 UTC (permalink / raw)
  To: Mike Frysinger
  Cc: linux-input, Dmitry Torokhov, linux-kernel, uclinux-dist-devel,
	Michael Hennerich, Bryan Wu

On Thu, 17 Sep 2009 14:24:10 -0400 Mike Frysinger <vapier@gentoo.org> wrote:

> ...
> +struct adp5520_keys {
> +	struct input_dev *input;
> +	struct notifier_block notifier;
> +	struct device *master;
> +	unsigned short keycode[ADP5520_KEYMAPSIZE];

Where is ADP5520_KEYMAPSIZE defined?

I can find a single instance in the tree:

./arch/blackfin/mach-bf537/boards/stamp.c:static const unsigned short adp5520_keymap[ADP5520_KEYMAPSIZE] = {
	
is it runtime-generated or something?

> +};
> +
> +static void adp5520_keys_report_event(struct adp5520_keys *dev,
> +					unsigned short keymask, int value)
> +{
> +	int i;
> +
> +	for (i = 0; i < ADP5520_MAXKEYS; i++)

Where is ADP5520_MAXKEYS defined?

> +		if (keymask & (1 << i))
> +			input_report_key(dev->input, dev->keycode[i], value);

It seems odd that keycode is dimensioned with ADP5520_KEYMAPSIZE but
here we iterate up to ADP5520_MAXKEYS.

> +
> +	input_sync(dev->input);
> +}
> +
> ...

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH] input/keyboard: new driver for ADP5520 MFD PMICs
@ 2009-09-20  6:30   ` Andrew Morton
  0 siblings, 0 replies; 20+ messages in thread
From: Andrew Morton @ 2009-09-20  6:30 UTC (permalink / raw)
  To: Mike Frysinger
  Cc: Michael Hennerich, Dmitry Torokhov,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA,
	linux-input-u79uwXL29TY76Z2rM5mHXA,
	uclinux-dist-devel-ZG0+EudsQA8dtHy/vicBwGD2FQJk+8+b

On Thu, 17 Sep 2009 14:24:10 -0400 Mike Frysinger <vapier-aBrp7R+bbdUdnm+yROfE0A@public.gmane.org> wrote:

> ...
> +struct adp5520_keys {
> +	struct input_dev *input;
> +	struct notifier_block notifier;
> +	struct device *master;
> +	unsigned short keycode[ADP5520_KEYMAPSIZE];

Where is ADP5520_KEYMAPSIZE defined?

I can find a single instance in the tree:

./arch/blackfin/mach-bf537/boards/stamp.c:static const unsigned short adp5520_keymap[ADP5520_KEYMAPSIZE] = {
	
is it runtime-generated or something?

> +};
> +
> +static void adp5520_keys_report_event(struct adp5520_keys *dev,
> +					unsigned short keymask, int value)
> +{
> +	int i;
> +
> +	for (i = 0; i < ADP5520_MAXKEYS; i++)

Where is ADP5520_MAXKEYS defined?

> +		if (keymask & (1 << i))
> +			input_report_key(dev->input, dev->keycode[i], value);

It seems odd that keycode is dimensioned with ADP5520_KEYMAPSIZE but
here we iterate up to ADP5520_MAXKEYS.

> +
> +	input_sync(dev->input);
> +}
> +
> ...

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [Uclinux-dist-devel] [PATCH] input/keyboard: new driver for  ADP5520 MFD PMICs
  2009-09-20  6:30   ` Andrew Morton
@ 2009-09-20 22:40     ` Mike Frysinger
  -1 siblings, 0 replies; 20+ messages in thread
From: Mike Frysinger @ 2009-09-20 22:40 UTC (permalink / raw)
  To: Andrew Morton
  Cc: Michael Hennerich, Dmitry Torokhov, linux-kernel, linux-input,
	uclinux-dist-devel

On Sun, Sep 20, 2009 at 02:30, Andrew Morton wrote:
> On Thu, 17 Sep 2009 14:24:10 -0400 Mike Frysinger wrote:
>> +struct adp5520_keys {
>> +     struct input_dev *input;
>> +     struct notifier_block notifier;
>> +     struct device *master;
>> +     unsigned short keycode[ADP5520_KEYMAPSIZE];
>
> Where is ADP5520_KEYMAPSIZE defined?

all these things are in the common mfd adp5520 patch.  it introduces a
common adp5520 header.

>> +     for (i = 0; i < ADP5520_MAXKEYS; i++)
>> +             if (keymask & (1 << i))
>> +                     input_report_key(dev->input, dev->keycode[i], value);
>
> It seems odd that keycode is dimensioned with ADP5520_KEYMAPSIZE but
> here we iterate up to ADP5520_MAXKEYS.

ARRAY_SIZE() probably makes more sense
-mike

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [Uclinux-dist-devel] [PATCH] input/keyboard: new driver for ADP5520 MFD PMICs
@ 2009-09-20 22:40     ` Mike Frysinger
  0 siblings, 0 replies; 20+ messages in thread
From: Mike Frysinger @ 2009-09-20 22:40 UTC (permalink / raw)
  To: Andrew Morton
  Cc: Michael Hennerich, Dmitry Torokhov, linux-kernel, linux-input,
	uclinux-dist-devel

On Sun, Sep 20, 2009 at 02:30, Andrew Morton wrote:
> On Thu, 17 Sep 2009 14:24:10 -0400 Mike Frysinger wrote:
>> +struct adp5520_keys {
>> +     struct input_dev *input;
>> +     struct notifier_block notifier;
>> +     struct device *master;
>> +     unsigned short keycode[ADP5520_KEYMAPSIZE];
>
> Where is ADP5520_KEYMAPSIZE defined?

all these things are in the common mfd adp5520 patch.  it introduces a
common adp5520 header.

>> +     for (i = 0; i < ADP5520_MAXKEYS; i++)
>> +             if (keymask & (1 << i))
>> +                     input_report_key(dev->input, dev->keycode[i], value);
>
> It seems odd that keycode is dimensioned with ADP5520_KEYMAPSIZE but
> here we iterate up to ADP5520_MAXKEYS.

ARRAY_SIZE() probably makes more sense
-mike
--
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

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [Uclinux-dist-devel] [PATCH] input/keyboard: new driver for  ADP5520 MFD PMICs
  2009-09-20 22:40     ` Mike Frysinger
@ 2009-09-21  4:04       ` Andrew Morton
  -1 siblings, 0 replies; 20+ messages in thread
From: Andrew Morton @ 2009-09-21  4:04 UTC (permalink / raw)
  To: Mike Frysinger
  Cc: Michael Hennerich, Dmitry Torokhov, linux-kernel, linux-input,
	uclinux-dist-devel

On Sun, 20 Sep 2009 18:40:41 -0400 Mike Frysinger <vapier.adi@gmail.com> wrote:

> On Sun, Sep 20, 2009 at 02:30, Andrew Morton wrote:
> > On Thu, 17 Sep 2009 14:24:10 -0400 Mike Frysinger wrote:
> >> +struct adp5520_keys {
> >> + __ __ struct input_dev *input;
> >> + __ __ struct notifier_block notifier;
> >> + __ __ struct device *master;
> >> + __ __ unsigned short keycode[ADP5520_KEYMAPSIZE];
> >
> > Where is ADP5520_KEYMAPSIZE defined?
> 
> all these things are in the common mfd adp5520 patch.  it introduces a
> common adp5520 header.

err, what common MFD patch?  There was nothing in either the changelog
nor the patch title which indicated that this patch had a dependency on
other patches.  This makes things harder for mergers and reviewers,
especially when the patchset hits multiple subsystems, maintained by
multiple developers.

Probably the best way to handle this sort of thing is to present it as a
sequence-numbered patch series with well-chosen cc's and an overall
description of the dependencies and perhaps the merge plan in [patch
0/n].


^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [Uclinux-dist-devel] [PATCH] input/keyboard: new driver for ADP5520 MFD PMICs
@ 2009-09-21  4:04       ` Andrew Morton
  0 siblings, 0 replies; 20+ messages in thread
From: Andrew Morton @ 2009-09-21  4:04 UTC (permalink / raw)
  To: Mike Frysinger
  Cc: Michael Hennerich, Dmitry Torokhov, linux-kernel, linux-input,
	uclinux-dist-devel

On Sun, 20 Sep 2009 18:40:41 -0400 Mike Frysinger <vapier.adi@gmail.com> wrote:

> On Sun, Sep 20, 2009 at 02:30, Andrew Morton wrote:
> > On Thu, 17 Sep 2009 14:24:10 -0400 Mike Frysinger wrote:
> >> +struct adp5520_keys {
> >> + __ __ struct input_dev *input;
> >> + __ __ struct notifier_block notifier;
> >> + __ __ struct device *master;
> >> + __ __ unsigned short keycode[ADP5520_KEYMAPSIZE];
> >
> > Where is ADP5520_KEYMAPSIZE defined?
> 
> all these things are in the common mfd adp5520 patch.  it introduces a
> common adp5520 header.

err, what common MFD patch?  There was nothing in either the changelog
nor the patch title which indicated that this patch had a dependency on
other patches.  This makes things harder for mergers and reviewers,
especially when the patchset hits multiple subsystems, maintained by
multiple developers.

Probably the best way to handle this sort of thing is to present it as a
sequence-numbered patch series with well-chosen cc's and an overall
description of the dependencies and perhaps the merge plan in [patch
0/n].


^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [Uclinux-dist-devel] [PATCH] input/keyboard: new driver for  ADP5520 MFD PMICs
@ 2009-09-21 13:40         ` Mike Frysinger
  0 siblings, 0 replies; 20+ messages in thread
From: Mike Frysinger @ 2009-09-21 13:40 UTC (permalink / raw)
  To: Andrew Morton
  Cc: Michael Hennerich, Dmitry Torokhov, linux-kernel, linux-input,
	uclinux-dist-devel

On Mon, Sep 21, 2009 at 00:04, Andrew Morton wrote:
> On Sun, 20 Sep 2009 18:40:41 -0400 Mike Frysinger wrote:
>> On Sun, Sep 20, 2009 at 02:30, Andrew Morton wrote:
>> > On Thu, 17 Sep 2009 14:24:10 -0400 Mike Frysinger wrote:
>> >> +struct adp5520_keys {
>> >> + __ __ struct input_dev *input;
>> >> + __ __ struct notifier_block notifier;
>> >> + __ __ struct device *master;
>> >> + __ __ unsigned short keycode[ADP5520_KEYMAPSIZE];
>> >
>> > Where is ADP5520_KEYMAPSIZE defined?
>>
>> all these things are in the common mfd adp5520 patch.  it introduces a
>> common adp5520 header.
>
> err, what common MFD patch?  There was nothing in either the changelog
> nor the patch title which indicated that this patch had a dependency on
> other patches.  This makes things harder for mergers and reviewers,
> especially when the patchset hits multiple subsystems, maintained by
> multiple developers.
>
> Probably the best way to handle this sort of thing is to present it as a
> sequence-numbered patch series with well-chosen cc's and an overall
> description of the dependencies and perhaps the merge plan in [patch
> 0/n].

the MFD patch has been sent to the MFD guy already for review.  being
an MFD means it touches like 4 or 5 different subsystems.  but the
only dependency is on the MFD core.
-mike

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH] input/keyboard: new driver for ADP5520 MFD PMICs
@ 2009-09-21 13:40         ` Mike Frysinger
  0 siblings, 0 replies; 20+ messages in thread
From: Mike Frysinger @ 2009-09-21 13:40 UTC (permalink / raw)
  To: Andrew Morton
  Cc: uclinux-dist-devel-ZG0+EudsQA8dtHy/vicBwGD2FQJk+8+b,
	Dmitry Torokhov, linux-kernel-u79uwXL29TY76Z2rM5mHXA,
	Michael Hennerich, linux-input-u79uwXL29TY76Z2rM5mHXA

On Mon, Sep 21, 2009 at 00:04, Andrew Morton wrote:
> On Sun, 20 Sep 2009 18:40:41 -0400 Mike Frysinger wrote:
>> On Sun, Sep 20, 2009 at 02:30, Andrew Morton wrote:
>> > On Thu, 17 Sep 2009 14:24:10 -0400 Mike Frysinger wrote:
>> >> +struct adp5520_keys {
>> >> + __ __ struct input_dev *input;
>> >> + __ __ struct notifier_block notifier;
>> >> + __ __ struct device *master;
>> >> + __ __ unsigned short keycode[ADP5520_KEYMAPSIZE];
>> >
>> > Where is ADP5520_KEYMAPSIZE defined?
>>
>> all these things are in the common mfd adp5520 patch.  it introduces a
>> common adp5520 header.
>
> err, what common MFD patch?  There was nothing in either the changelog
> nor the patch title which indicated that this patch had a dependency on
> other patches.  This makes things harder for mergers and reviewers,
> especially when the patchset hits multiple subsystems, maintained by
> multiple developers.
>
> Probably the best way to handle this sort of thing is to present it as a
> sequence-numbered patch series with well-chosen cc's and an overall
> description of the dependencies and perhaps the merge plan in [patch
> 0/n].

the MFD patch has been sent to the MFD guy already for review.  being
an MFD means it touches like 4 or 5 different subsystems.  but the
only dependency is on the MFD core.
-mike
_______________________________________________
Uclinux-dist-devel mailing list
Uclinux-dist-devel@blackfin.uclinux.org
https://blackfin.uclinux.org/mailman/listinfo/uclinux-dist-devel

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [Uclinux-dist-devel] [PATCH] input/keyboard: new driver for  ADP5520 MFD PMICs
  2009-09-21 13:40         ` Mike Frysinger
@ 2009-09-21 18:23           ` Andrew Morton
  -1 siblings, 0 replies; 20+ messages in thread
From: Andrew Morton @ 2009-09-21 18:23 UTC (permalink / raw)
  To: Mike Frysinger
  Cc: Michael Hennerich, Dmitry Torokhov, linux-kernel, linux-input,
	uclinux-dist-devel

On Mon, 21 Sep 2009 09:40:46 -0400 Mike Frysinger <vapier.adi@gmail.com> wrote:

> On Mon, Sep 21, 2009 at 00:04, Andrew Morton wrote:
> > On Sun, 20 Sep 2009 18:40:41 -0400 Mike Frysinger wrote:
> >> On Sun, Sep 20, 2009 at 02:30, Andrew Morton wrote:
> >> > On Thu, 17 Sep 2009 14:24:10 -0400 Mike Frysinger wrote:
> >> >> +struct adp5520_keys {
> >> >> + __ __ struct input_dev *input;
> >> >> + __ __ struct notifier_block notifier;
> >> >> + __ __ struct device *master;
> >> >> + __ __ unsigned short keycode[ADP5520_KEYMAPSIZE];
> >> >
> >> > Where is ADP5520_KEYMAPSIZE defined?
> >>
> >> all these things are in the common mfd adp5520 patch. __it introduces a
> >> common adp5520 header.
> >
> > err, what common MFD patch? __There was nothing in either the changelog
> > nor the patch title which indicated that this patch had a dependency on
> > other patches. __This makes things harder for mergers and reviewers,
> > especially when the patchset hits multiple subsystems, maintained by
> > multiple developers.
> >
> > Probably the best way to handle this sort of thing is to present it as a
> > sequence-numbered patch series with well-chosen cc's and an overall
> > description of the dependencies and perhaps the merge plan in [patch
> > 0/n].
> 
> the MFD patch has been sent to the MFD guy already for review.  being
> an MFD means it touches like 4 or 5 different subsystems.  but the
> only dependency is on the MFD core.

What you said has nothing to do with what I said.

As the patch was presented, Dmitry or I could well have merged it only
to find out that it doesn't compile.


^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [Uclinux-dist-devel] [PATCH] input/keyboard: new driver for ADP5520 MFD PMICs
@ 2009-09-21 18:23           ` Andrew Morton
  0 siblings, 0 replies; 20+ messages in thread
From: Andrew Morton @ 2009-09-21 18:23 UTC (permalink / raw)
  To: Mike Frysinger
  Cc: Michael Hennerich, Dmitry Torokhov, linux-kernel, linux-input,
	uclinux-dist-devel

On Mon, 21 Sep 2009 09:40:46 -0400 Mike Frysinger <vapier.adi@gmail.com> wrote:

> On Mon, Sep 21, 2009 at 00:04, Andrew Morton wrote:
> > On Sun, 20 Sep 2009 18:40:41 -0400 Mike Frysinger wrote:
> >> On Sun, Sep 20, 2009 at 02:30, Andrew Morton wrote:
> >> > On Thu, 17 Sep 2009 14:24:10 -0400 Mike Frysinger wrote:
> >> >> +struct adp5520_keys {
> >> >> + __ __ struct input_dev *input;
> >> >> + __ __ struct notifier_block notifier;
> >> >> + __ __ struct device *master;
> >> >> + __ __ unsigned short keycode[ADP5520_KEYMAPSIZE];
> >> >
> >> > Where is ADP5520_KEYMAPSIZE defined?
> >>
> >> all these things are in the common mfd adp5520 patch. __it introduces a
> >> common adp5520 header.
> >
> > err, what common MFD patch? __There was nothing in either the changelog
> > nor the patch title which indicated that this patch had a dependency on
> > other patches. __This makes things harder for mergers and reviewers,
> > especially when the patchset hits multiple subsystems, maintained by
> > multiple developers.
> >
> > Probably the best way to handle this sort of thing is to present it as a
> > sequence-numbered patch series with well-chosen cc's and an overall
> > description of the dependencies and perhaps the merge plan in [patch
> > 0/n].
> 
> the MFD patch has been sent to the MFD guy already for review.  being
> an MFD means it touches like 4 or 5 different subsystems.  but the
> only dependency is on the MFD core.

What you said has nothing to do with what I said.

As the patch was presented, Dmitry or I could well have merged it only
to find out that it doesn't compile.

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH] input/keyboard: new driver for ADP5520 MFD PMICs
  2009-09-17 18:24 [PATCH] input/keyboard: new driver for ADP5520 MFD PMICs Mike Frysinger
  2009-09-20  6:30   ` Andrew Morton
@ 2009-09-22  5:59 ` Dmitry Torokhov
  2009-09-22  7:33   ` Jiri Kosina
  2009-09-23  5:15 ` [PATCH v2] " Mike Frysinger
  2 siblings, 1 reply; 20+ messages in thread
From: Dmitry Torokhov @ 2009-09-22  5:59 UTC (permalink / raw)
  To: Mike Frysinger
  Cc: linux-input, linux-kernel, uclinux-dist-devel, Michael Hennerich,
	Bryan Wu

Hi Mike,

On Thu, Sep 17, 2009 at 02:24:10PM -0400, Mike Frysinger wrote:
> From: Michael Hennerich <michael.hennerich@analog.com>
> 
> Signed-off-by: Michael Hennerich <michael.hennerich@analog.com>
> Signed-off-by: Bryan Wu <cooloney@kernel.org>
> Signed-off-by: Mike Frysinger <vapier@gentoo.org>
> ---
>  drivers/input/keyboard/Kconfig        |   10 ++
>  drivers/input/keyboard/Makefile       |    1 +
>  drivers/input/keyboard/adp5520-keys.c |  219 +++++++++++++++++++++++++++++++++
>  3 files changed, 230 insertions(+), 0 deletions(-)
>  create mode 100644 drivers/input/keyboard/adp5520-keys.c
> 
> diff --git a/drivers/input/keyboard/Kconfig b/drivers/input/keyboard/Kconfig
> index e0bfcd7..53c9561 100644
> --- a/drivers/input/keyboard/Kconfig
> +++ b/drivers/input/keyboard/Kconfig
> @@ -392,4 +392,14 @@ config KEYBOARD_ADP5588
>  	  To compile this driver as a module, choose M here: the
>  	  module will be called adp5588-keys.
>  
> +config KEYBOARD_ADP5520
> +	tristate "Keypad Support for ADP5520 PMIC"
> +	depends on PMIC_ADP5520
> +	help
> +	  This option enables support for the keypad scan matrix
> +	  on Analog Devices ADP5520 PMICs.
> +
> +	  To compile this driver as a module, choose M here: the module will
> +	  be called adp5520-keys.
> +
>  endif
> diff --git a/drivers/input/keyboard/Makefile b/drivers/input/keyboard/Makefile
> index 72082a4..c541489 100644
> --- a/drivers/input/keyboard/Makefile
> +++ b/drivers/input/keyboard/Makefile
> @@ -5,6 +5,7 @@
>  # Each configuration option enables a list of files.
>  
>  obj-$(CONFIG_KEYBOARD_AAED2000)		+= aaed2000_kbd.o
> +obj-$(CONFIG_KEYBOARD_ADP5520)		+= adp5520-keys.o
>  obj-$(CONFIG_KEYBOARD_ADP5588)		+= adp5588-keys.o
>  obj-$(CONFIG_KEYBOARD_AMIGA)		+= amikbd.o
>  obj-$(CONFIG_KEYBOARD_ATARI)		+= atakbd.o
> diff --git a/drivers/input/keyboard/adp5520-keys.c b/drivers/input/keyboard/adp5520-keys.c
> new file mode 100644
> index 0000000..cc2e1ed
> --- /dev/null
> +++ b/drivers/input/keyboard/adp5520-keys.c
> @@ -0,0 +1,219 @@
> +/*
> + * Keypad driver for Analog Devices ADP5520 MFD PMICs
> + *
> + * Copyright 2009 Analog Devices Inc.
> + *
> + * Licensed under the GPL-2 or later.
> + */
> +
> +#include <linux/module.h>
> +#include <linux/kernel.h>
> +#include <linux/init.h>
> +#include <linux/platform_device.h>
> +#include <linux/input.h>
> +#include <linux/mfd/adp5520.h>
> +
> +struct adp5520_keys {
> +	struct input_dev *input;
> +	struct notifier_block notifier;
> +	struct device *master;
> +	unsigned short keycode[ADP5520_KEYMAPSIZE];
> +};
> +
> +static void adp5520_keys_report_event(struct adp5520_keys *dev,
> +					unsigned short keymask, int value)
> +{
> +	int i;
> +
> +	for (i = 0; i < ADP5520_MAXKEYS; i++)
> +		if (keymask & (1 << i))
> +			input_report_key(dev->input, dev->keycode[i], value);
> +
> +	input_sync(dev->input);
> +}
> +
> +static int adp5520_keys_notifier(struct notifier_block *nb,
> +				 unsigned long event, void *data)
> +{
> +	struct adp5520_keys *dev;
> +	uint8_t reg_val_low, reg_val_high;
> +	unsigned short keymask;
> +
> +	dev = container_of(nb, struct adp5520_keys, notifier);
> +
> +	if (event & KP_INT) {
> +		adp5520_read(dev->master, KP_INT_STAT_1, &reg_val_low);
> +		adp5520_read(dev->master, KP_INT_STAT_2, &reg_val_high);
> +
> +		keymask = (reg_val_high << 8) | reg_val_low;
> +		/* Read twice to clear */
> +		adp5520_read(dev->master, KP_INT_STAT_1, &reg_val_low);
> +		adp5520_read(dev->master, KP_INT_STAT_2, &reg_val_high);
> +		keymask |= (reg_val_high << 8) | reg_val_low;
> +		adp5520_keys_report_event(dev, keymask, 1);
> +	}
> +
> +	if (event & KR_INT) {

Why do you check the same condition twice?

> +		adp5520_read(dev->master, KR_INT_STAT_1, &reg_val_low);
> +		adp5520_read(dev->master, KR_INT_STAT_2, &reg_val_high);
> +
> +		keymask = (reg_val_high << 8) | reg_val_low;
> +		/* Read twice to clear */
> +		adp5520_read(dev->master, KR_INT_STAT_1, &reg_val_low);
> +		adp5520_read(dev->master, KR_INT_STAT_2, &reg_val_high);
> +		keymask |= (reg_val_high << 8) | reg_val_low;
> +		adp5520_keys_report_event(dev, keymask, 0);
> +	}
> +
> +	return 0;
> +}
> +
> +static int __devinit adp5520_keys_probe(struct platform_device *pdev)
> +{
> +	struct adp5520_keys_platfrom_data *pdata = pdev->dev.platform_data;
> +	struct input_dev *input;
> +	struct adp5520_keys *dev;
> +	int ret, i;
> +	unsigned char en_mask, ctl_mask = 0;
> +
> +	if (pdev->id != ID_ADP5520) {
> +		dev_err(&pdev->dev, "only ADP5520 supports Keypad\n");
> +		return -EFAULT;

-ENODEV

> +	}
> +
> +	if (pdata == NULL) {
> +		dev_err(&pdev->dev, "missing platform data\n");
> +		return -ENODEV;

-EINVAL

> +	}
> +
> +	if (!(pdata->rows_en_mask && pdata->cols_en_mask))
> +		return -ENODEV;
> +

-EINVAL

> +	dev = kzalloc(sizeof(*dev), GFP_KERNEL);
> +	if (dev == NULL) {
> +		dev_err(&pdev->dev, "failed to alloc memory\n");
> +		return -ENOMEM;
> +	}
> +
> +	input = input_allocate_device();
> +	if (!input) {
> +		ret = -ENOMEM;
> +		goto err;
> +	}
> +
> +	dev->master = pdev->dev.parent;
> +	dev->input = input;
> +
> +	input->name = pdev->name;
> +	input->phys = "adp5520-keys/input0";
> +	input->dev.parent = &pdev->dev;
> +
> +	input_set_drvdata(input, dev);
> +
> +	input->id.bustype = BUS_I2C;
> +	input->id.vendor = 0x0001;
> +	input->id.product = 0x5520;
> +	input->id.version = 0x0001;
> +
> +	input->keycodesize = sizeof(unsigned short);

sizeof(dev->keycode[0]) is safer.

> +	input->keycodemax = pdata->keymapsize;
> +	input->keycode = dev->keycode;
> +
> +	memcpy(dev->keycode, pdata->keymap,
> +		pdata->keymapsize * input->keycodesize);
> +
> +	/* setup input device */
> +	__set_bit(EV_KEY, input->evbit);
> +
> +	if (pdata->repeat)
> +		__set_bit(EV_REP, input->evbit);
> +
> +	for (i = 0; i < input->keycodemax; i++)
> +		__set_bit(dev->keycode[i] & KEY_MAX, input->keybit);

Not sure if we need "& KEY_MAX"...

> +	__clear_bit(KEY_RESERVED, input->keybit);
> +
> +	ret = input_register_device(input);
> +	if (ret) {
> +		dev_err(&pdev->dev, "unable to register input device\n");
> +		input_free_device(input);

Please either out-of-line error handling or inline, but not both.

> +		goto err;
> +	}
> +
> +	en_mask = pdata->rows_en_mask | pdata->cols_en_mask;
> +
> +	ret = adp5520_set_bits(dev->master, GPIO_CFG_1, en_mask);
> +
> +	if (en_mask & COL_C3)
> +		ctl_mask |= C3_MODE;
> +
> +	if (en_mask & ROW_R3)
> +		ctl_mask |= R3_MODE;
> +
> +	if (ctl_mask)
> +		ret |= adp5520_set_bits(dev->master, LED_CONTROL,
> +			ctl_mask);
> +
> +	ret |= adp5520_set_bits(dev->master, GPIO_PULLUP,
> +		pdata->rows_en_mask);
> +
> +	if (ret) {
> +		dev_err(&pdev->dev, "failed to write\n");
> +		goto err1;

What are the chances that ret hs proper errno value here?

> +	}
> +
> +	dev->notifier.notifier_call = adp5520_keys_notifier;
> +	ret = adp5520_register_notifier(dev->master, &dev->notifier,
> +			KP_IEN | KR_IEN);
> +	if (ret) {
> +		dev_err(&pdev->dev, "failed to register notifier\n");
> +		goto err1;
> +	}
> +
> +	platform_set_drvdata(pdev, dev);
> +	return 0;
> +
> +err1:
> +	input_unregister_device(input);
> +err:
> +	kfree(dev);
> +	return ret;
> +}
> +
> +static int __devexit adp5520_keys_remove(struct platform_device *pdev)
> +{
> +	struct adp5520_keys *dev;
> +	dev = platform_get_drvdata(pdev);

Combine in one line?

> +
> +	adp5520_unregister_notifier(dev->master, &dev->notifier,
> +				KP_IEN | KR_IEN);
> +
> +	input_unregister_device(dev->input);
> +	kfree(dev);
> +	return 0;
> +}
> +
> +static struct platform_driver adp5520_keys_driver = {
> +	.driver	= {
> +		.name	= "adp5520-keys",
> +		.owner	= THIS_MODULE,
> +	},
> +	.probe		= adp5520_keys_probe,
> +	.remove		= __devexit_p(adp5520_keys_remove),
> +};
> +
> +static int __init adp5520_keys_init(void)
> +{
> +	return platform_driver_register(&adp5520_keys_driver);
> +}
> +module_init(adp5520_keys_init);
> +
> +static void __exit adp5520_keys_exit(void)
> +{
> +	platform_driver_unregister(&adp5520_keys_driver);
> +}
> +module_exit(adp5520_keys_exit);
> +
> +MODULE_AUTHOR("Michael Hennerich <hennerich@blackfin.uclinux.org>");
> +MODULE_DESCRIPTION("Keys ADP5520 Driver");
> +MODULE_LICENSE("GPL");
> +MODULE_ALIAS("platform:adp5520-keys");
> -- 
> 1.6.5.rc1
> 

Thanks.

-- 
Dmitry

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH] input/keyboard: new driver for ADP5520 MFD PMICs
  2009-09-22  5:59 ` Dmitry Torokhov
@ 2009-09-22  7:33   ` Jiri Kosina
  2009-09-22  7:53       ` Hennerich, Michael
  0 siblings, 1 reply; 20+ messages in thread
From: Jiri Kosina @ 2009-09-22  7:33 UTC (permalink / raw)
  To: Dmitry Torokhov
  Cc: Mike Frysinger, linux-input, linux-kernel, uclinux-dist-devel,
	Michael Hennerich, Bryan Wu

On Mon, 21 Sep 2009, Dmitry Torokhov wrote:

> > +	if (event & KP_INT) {
> > +		adp5520_read(dev->master, KP_INT_STAT_1, &reg_val_low);
> > +		adp5520_read(dev->master, KP_INT_STAT_2, &reg_val_high);
> > +
> > +		keymask = (reg_val_high << 8) | reg_val_low;
> > +		/* Read twice to clear */
> > +		adp5520_read(dev->master, KP_INT_STAT_1, &reg_val_low);
> > +		adp5520_read(dev->master, KP_INT_STAT_2, &reg_val_high);
> > +		keymask |= (reg_val_high << 8) | reg_val_low;
> > +		adp5520_keys_report_event(dev, keymask, 1);
> > +	}
> > +
> > +	if (event & KR_INT) {
> 
> Why do you check the same condition twice?

It actually doesn't seem to be the same condition (KP_INT vs. KR_INT), but 
it's difficult to say, as these constants are apparently added in some 
other patch I have no idea about.
And the same for other constants used in the code ... the patch seems to 
be quite incomplete.

-- 
Jiri Kosina
SUSE Labs, Novell Inc.


^ permalink raw reply	[flat|nested] 20+ messages in thread

* RE: [PATCH] input/keyboard: new driver for ADP5520 MFD PMICs
  2009-09-22  7:33   ` Jiri Kosina
@ 2009-09-22  7:53       ` Hennerich, Michael
  0 siblings, 0 replies; 20+ messages in thread
From: Hennerich, Michael @ 2009-09-22  7:53 UTC (permalink / raw)
  To: Jiri Kosina, Dmitry Torokhov
  Cc: Mike Frysinger, linux-input, linux-kernel, uclinux-dist-devel, Bryan Wu


>From: Jiri Kosina
>Sent: Tuesday, September 22, 2009 9:33 AM
>
>On Mon, 21 Sep 2009, Dmitry Torokhov wrote:
>
>> > +	if (event & KP_INT) {
>> > +		adp5520_read(dev->master, KP_INT_STAT_1, &reg_val_low);
>> > +		adp5520_read(dev->master, KP_INT_STAT_2, &reg_val_high);
>> > +
>> > +		keymask = (reg_val_high << 8) | reg_val_low;
>> > +		/* Read twice to clear */
>> > +		adp5520_read(dev->master, KP_INT_STAT_1, &reg_val_low);
>> > +		adp5520_read(dev->master, KP_INT_STAT_2, &reg_val_high);
>> > +		keymask |= (reg_val_high << 8) | reg_val_low;
>> > +		adp5520_keys_report_event(dev, keymask, 1);
>> > +	}
>> > +
>> > +	if (event & KR_INT) {
>>
>> Why do you check the same condition twice?
>
>It actually doesn't seem to be the same condition (KP_INT vs. KR_INT), but
>it's difficult to say, as these constants are apparently added in some
>other patch I have no idea about.

KP Key-Press versus Key-Release 

>And the same for other constants used in the code ... the patch seems to
>be quite incomplete.

The missing constants are in include/linux/mfd/adp5520.h

This file was sent in a different patch, adding support for the Multi Function Device Core 
(drivers/mfd/adp5520.c)

-Michael

------------------------------------------------------------------
********* Analog Devices GmbH
**  ***** Systems Engineering
**     ** Wilhelm-Wagenfeld-Strasse 6 
**  ***** D-80807 Munich
********* Germany 
Registergericht München HRB 40368,  Geschäftsführer: Thomas Wessel, William A. Martin, Margaret K. Seif

>
>--
>Jiri Kosina
>SUSE Labs, Novell Inc.


^ permalink raw reply	[flat|nested] 20+ messages in thread

* RE: [PATCH] input/keyboard: new driver for ADP5520 MFD PMICs
@ 2009-09-22  7:53       ` Hennerich, Michael
  0 siblings, 0 replies; 20+ messages in thread
From: Hennerich, Michael @ 2009-09-22  7:53 UTC (permalink / raw)
  To: Jiri Kosina, Dmitry Torokhov
  Cc: Mike Frysinger, linux-input, linux-kernel, uclinux-dist-devel, Bryan Wu


>From: Jiri Kosina
>Sent: Tuesday, September 22, 2009 9:33 AM
>
>On Mon, 21 Sep 2009, Dmitry Torokhov wrote:
>
>> > +	if (event & KP_INT) {
>> > +		adp5520_read(dev->master, KP_INT_STAT_1, &reg_val_low);
>> > +		adp5520_read(dev->master, KP_INT_STAT_2, &reg_val_high);
>> > +
>> > +		keymask = (reg_val_high << 8) | reg_val_low;
>> > +		/* Read twice to clear */
>> > +		adp5520_read(dev->master, KP_INT_STAT_1, &reg_val_low);
>> > +		adp5520_read(dev->master, KP_INT_STAT_2, &reg_val_high);
>> > +		keymask |= (reg_val_high << 8) | reg_val_low;
>> > +		adp5520_keys_report_event(dev, keymask, 1);
>> > +	}
>> > +
>> > +	if (event & KR_INT) {
>>
>> Why do you check the same condition twice?
>
>It actually doesn't seem to be the same condition (KP_INT vs. KR_INT), but
>it's difficult to say, as these constants are apparently added in some
>other patch I have no idea about.

KP Key-Press versus Key-Release 

>And the same for other constants used in the code ... the patch seems to
>be quite incomplete.

The missing constants are in include/linux/mfd/adp5520.h

This file was sent in a different patch, adding support for the Multi Function Device Core 
(drivers/mfd/adp5520.c)

-Michael

------------------------------------------------------------------
********* Analog Devices GmbH
**  ***** Systems Engineering
**     ** Wilhelm-Wagenfeld-Strasse 6 
**  ***** D-80807 Munich
********* Germany 
Registergericht München HRB 40368,  Geschäftsführer: Thomas Wessel, William A. Martin, Margaret K. Seif

>
>--
>Jiri Kosina
>SUSE Labs, Novell Inc.

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

^ permalink raw reply	[flat|nested] 20+ messages in thread

* RE: [PATCH] input/keyboard: new driver for ADP5520 MFD PMICs
  2009-09-22  7:53       ` Hennerich, Michael
  (?)
@ 2009-09-22  7:56       ` Jiri Kosina
  2009-09-22 14:21         ` Mike Frysinger
  -1 siblings, 1 reply; 20+ messages in thread
From: Jiri Kosina @ 2009-09-22  7:56 UTC (permalink / raw)
  To: Hennerich, Michael
  Cc: Dmitry Torokhov, Mike Frysinger, linux-input, linux-kernel,
	uclinux-dist-devel, Bryan Wu

On Tue, 22 Sep 2009, Hennerich, Michael wrote:

> >It actually doesn't seem to be the same condition (KP_INT vs. KR_INT), 
> >but it's difficult to say, as these constants are apparently added in 
> >some other patch I have no idea about.
> 
> KP Key-Press versus Key-Release 
> 
> >And the same for other constants used in the code ... the patch seems to
> >be quite incomplete.
> 
> The missing constants are in include/linux/mfd/adp5520.h
> 
> This file was sent in a different patch, adding support for the Multi Function Device Core 
> (drivers/mfd/adp5520.c)

The dependency on another patch should be either at least expressed in the 
patch Changelog, or even better send the patches as series with proper 
numbering, so that the dependency is obvious.

Thanks,

-- 
Jiri Kosina
SUSE Labs, Novell Inc.

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH] input/keyboard: new driver for ADP5520 MFD PMICs
  2009-09-22  7:56       ` Jiri Kosina
@ 2009-09-22 14:21         ` Mike Frysinger
  0 siblings, 0 replies; 20+ messages in thread
From: Mike Frysinger @ 2009-09-22 14:21 UTC (permalink / raw)
  To: Jiri Kosina
  Cc: Hennerich, Michael, Dmitry Torokhov, linux-input, linux-kernel,
	uclinux-dist-devel, Bryan Wu

On Tue, Sep 22, 2009 at 03:56, Jiri Kosina wrote:
> On Tue, 22 Sep 2009, Hennerich, Michael wrote:
>
>> >It actually doesn't seem to be the same condition (KP_INT vs. KR_INT),
>> >but it's difficult to say, as these constants are apparently added in
>> >some other patch I have no idea about.
>>
>> KP Key-Press versus Key-Release
>>
>> >And the same for other constants used in the code ... the patch seems to
>> >be quite incomplete.
>>
>> The missing constants are in include/linux/mfd/adp5520.h
>>
>> This file was sent in a different patch, adding support for the Multi Function Device Core
>> (drivers/mfd/adp5520.c)
>
> The dependency on another patch should be either at least expressed in the
> patch Changelog, or even better send the patches as series with proper
> numbering, so that the dependency is obvious.

we've already been over this.  read the whole thread and/or search
lkml for the mfd patch.  the followup will contain the dependency
info, but until then, the exact define values really arent relevant to
review of this code.
-mike

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH] input/keyboard: new driver for ADP5520 MFD PMICs
  2009-09-22  7:53       ` Hennerich, Michael
  (?)
  (?)
@ 2009-09-22 15:06       ` Dmitry Torokhov
  -1 siblings, 0 replies; 20+ messages in thread
From: Dmitry Torokhov @ 2009-09-22 15:06 UTC (permalink / raw)
  To: Hennerich, Michael
  Cc: Jiri Kosina, Mike Frysinger, linux-input, linux-kernel,
	uclinux-dist-devel, Bryan Wu

On Tue, Sep 22, 2009 at 08:53:19AM +0100, Hennerich, Michael wrote:
> 
> >From: Jiri Kosina
> >Sent: Tuesday, September 22, 2009 9:33 AM
> >
> >On Mon, 21 Sep 2009, Dmitry Torokhov wrote:
> >
> >> > +	if (event & KP_INT) {
> >> > +		adp5520_read(dev->master, KP_INT_STAT_1, &reg_val_low);
> >> > +		adp5520_read(dev->master, KP_INT_STAT_2, &reg_val_high);
> >> > +
> >> > +		keymask = (reg_val_high << 8) | reg_val_low;
> >> > +		/* Read twice to clear */
> >> > +		adp5520_read(dev->master, KP_INT_STAT_1, &reg_val_low);
> >> > +		adp5520_read(dev->master, KP_INT_STAT_2, &reg_val_high);
> >> > +		keymask |= (reg_val_high << 8) | reg_val_low;
> >> > +		adp5520_keys_report_event(dev, keymask, 1);
> >> > +	}
> >> > +
> >> > +	if (event & KR_INT) {
> >>
> >> Why do you check the same condition twice?
> >
> >It actually doesn't seem to be the same condition (KP_INT vs. KR_INT), but
> >it's difficult to say, as these constants are apparently added in some
> >other patch I have no idea about.
> 
> KP Key-Press versus Key-Release 
> 

Yeah, my bad... However the constants could be made more distinct.

-- 
Dmitry

^ permalink raw reply	[flat|nested] 20+ messages in thread

* [PATCH v2] input/keyboard: new driver for ADP5520 MFD PMICs
  2009-09-17 18:24 [PATCH] input/keyboard: new driver for ADP5520 MFD PMICs Mike Frysinger
  2009-09-20  6:30   ` Andrew Morton
  2009-09-22  5:59 ` Dmitry Torokhov
@ 2009-09-23  5:15 ` Mike Frysinger
  2009-09-23  6:28   ` Dmitry Torokhov
  2 siblings, 1 reply; 20+ messages in thread
From: Mike Frysinger @ 2009-09-23  5:15 UTC (permalink / raw)
  To: linux-input, Dmitry Torokhov
  Cc: linux-kernel, uclinux-dist-devel, Michael Hennerich, Bryan Wu

From: Michael Hennerich <michael.hennerich@analog.com>

Signed-off-by: Michael Hennerich <michael.hennerich@analog.com>
Signed-off-by: Bryan Wu <cooloney@kernel.org>
Signed-off-by: Mike Frysinger <vapier@gentoo.org>
---
this patch requires:
	[PATCH] mfd: ADP5520 Multifunction LCD Backlight and Keypad Input Device Driver

v2
	- address feedback from Dmitry

 drivers/input/keyboard/Kconfig        |   10 ++
 drivers/input/keyboard/Makefile       |    1 +
 drivers/input/keyboard/adp5520-keys.c |  220 +++++++++++++++++++++++++++++++++
 3 files changed, 231 insertions(+), 0 deletions(-)
 create mode 100644 drivers/input/keyboard/adp5520-keys.c

diff --git a/drivers/input/keyboard/Kconfig b/drivers/input/keyboard/Kconfig
index e0bfcd7..53c9561 100644
--- a/drivers/input/keyboard/Kconfig
+++ b/drivers/input/keyboard/Kconfig
@@ -392,4 +392,14 @@ config KEYBOARD_ADP5588
 	  To compile this driver as a module, choose M here: the
 	  module will be called adp5588-keys.
 
+config KEYBOARD_ADP5520
+	tristate "Keypad Support for ADP5520 PMIC"
+	depends on PMIC_ADP5520
+	help
+	  This option enables support for the keypad scan matrix
+	  on Analog Devices ADP5520 PMICs.
+
+	  To compile this driver as a module, choose M here: the module will
+	  be called adp5520-keys.
+
 endif
diff --git a/drivers/input/keyboard/Makefile b/drivers/input/keyboard/Makefile
index 72082a4..c541489 100644
--- a/drivers/input/keyboard/Makefile
+++ b/drivers/input/keyboard/Makefile
@@ -5,6 +5,7 @@
 # Each configuration option enables a list of files.
 
 obj-$(CONFIG_KEYBOARD_AAED2000)		+= aaed2000_kbd.o
+obj-$(CONFIG_KEYBOARD_ADP5520)		+= adp5520-keys.o
 obj-$(CONFIG_KEYBOARD_ADP5588)		+= adp5588-keys.o
 obj-$(CONFIG_KEYBOARD_AMIGA)		+= amikbd.o
 obj-$(CONFIG_KEYBOARD_ATARI)		+= atakbd.o
diff --git a/drivers/input/keyboard/adp5520-keys.c b/drivers/input/keyboard/adp5520-keys.c
new file mode 100644
index 0000000..1860d30
--- /dev/null
+++ b/drivers/input/keyboard/adp5520-keys.c
@@ -0,0 +1,220 @@
+/*
+ * Keypad driver for Analog Devices ADP5520 MFD PMICs
+ *
+ * Copyright 2009 Analog Devices Inc.
+ *
+ * Licensed under the GPL-2 or later.
+ */
+
+#include <linux/module.h>
+#include <linux/kernel.h>
+#include <linux/init.h>
+#include <linux/platform_device.h>
+#include <linux/input.h>
+#include <linux/mfd/adp5520.h>
+
+struct adp5520_keys {
+	struct input_dev *input;
+	struct notifier_block notifier;
+	struct device *master;
+	unsigned short keycode[ADP5520_KEYMAPSIZE];
+};
+
+static void adp5520_keys_report_event(struct adp5520_keys *dev,
+					unsigned short keymask, int value)
+{
+	int i;
+
+	for (i = 0; i < ADP5520_MAXKEYS; i++)
+		if (keymask & (1 << i))
+			input_report_key(dev->input, dev->keycode[i], value);
+
+	input_sync(dev->input);
+}
+
+static int adp5520_keys_notifier(struct notifier_block *nb,
+				 unsigned long event, void *data)
+{
+	struct adp5520_keys *dev;
+	uint8_t reg_val_low, reg_val_high;
+	unsigned short keymask;
+
+	dev = container_of(nb, struct adp5520_keys, notifier);
+
+	if (event & KP_INT) {
+		adp5520_read(dev->master, KP_INT_STAT_1, &reg_val_low);
+		adp5520_read(dev->master, KP_INT_STAT_2, &reg_val_high);
+
+		keymask = (reg_val_high << 8) | reg_val_low;
+		/* Read twice to clear */
+		adp5520_read(dev->master, KP_INT_STAT_1, &reg_val_low);
+		adp5520_read(dev->master, KP_INT_STAT_2, &reg_val_high);
+		keymask |= (reg_val_high << 8) | reg_val_low;
+		adp5520_keys_report_event(dev, keymask, 1);
+	}
+
+	if (event & KR_INT) {
+		adp5520_read(dev->master, KR_INT_STAT_1, &reg_val_low);
+		adp5520_read(dev->master, KR_INT_STAT_2, &reg_val_high);
+
+		keymask = (reg_val_high << 8) | reg_val_low;
+		/* Read twice to clear */
+		adp5520_read(dev->master, KR_INT_STAT_1, &reg_val_low);
+		adp5520_read(dev->master, KR_INT_STAT_2, &reg_val_high);
+		keymask |= (reg_val_high << 8) | reg_val_low;
+		adp5520_keys_report_event(dev, keymask, 0);
+	}
+
+	return 0;
+}
+
+static int __devinit adp5520_keys_probe(struct platform_device *pdev)
+{
+	struct adp5520_keys_platfrom_data *pdata = pdev->dev.platform_data;
+	struct input_dev *input;
+	struct adp5520_keys *dev;
+	int ret, i;
+	unsigned char en_mask, ctl_mask = 0;
+
+	if (pdev->id != ID_ADP5520) {
+		dev_err(&pdev->dev, "only ADP5520 supports Keypad\n");
+		return -EINVAL;
+	}
+
+	if (pdata == NULL) {
+		dev_err(&pdev->dev, "missing platform data\n");
+		return -EINVAL;
+	}
+
+	if (!(pdata->rows_en_mask && pdata->cols_en_mask))
+		return -EINVAL;
+
+	dev = kzalloc(sizeof(*dev), GFP_KERNEL);
+	if (dev == NULL) {
+		dev_err(&pdev->dev, "failed to alloc memory\n");
+		return -ENOMEM;
+	}
+
+	input = input_allocate_device();
+	if (!input) {
+		ret = -ENOMEM;
+		goto err;
+	}
+
+	dev->master = pdev->dev.parent;
+	dev->input = input;
+
+	input->name = pdev->name;
+	input->phys = "adp5520-keys/input0";
+	input->dev.parent = &pdev->dev;
+
+	input_set_drvdata(input, dev);
+
+	input->id.bustype = BUS_I2C;
+	input->id.vendor = 0x0001;
+	input->id.product = 0x5520;
+	input->id.version = 0x0001;
+
+	input->keycodesize = sizeof(dev->keycode[0]);
+	input->keycodemax = pdata->keymapsize;
+	input->keycode = dev->keycode;
+
+	memcpy(dev->keycode, pdata->keymap,
+		pdata->keymapsize * input->keycodesize);
+
+	/* setup input device */
+	__set_bit(EV_KEY, input->evbit);
+
+	if (pdata->repeat)
+		__set_bit(EV_REP, input->evbit);
+
+	for (i = 0; i < input->keycodemax; i++)
+		__set_bit(dev->keycode[i], input->keybit);
+	__clear_bit(KEY_RESERVED, input->keybit);
+
+	ret = input_register_device(input);
+	if (ret) {
+		dev_err(&pdev->dev, "unable to register input device\n");
+		goto err;
+	}
+
+	en_mask = pdata->rows_en_mask | pdata->cols_en_mask;
+
+	ret = adp5520_set_bits(dev->master, GPIO_CFG_1, en_mask);
+
+	if (en_mask & COL_C3)
+		ctl_mask |= C3_MODE;
+
+	if (en_mask & ROW_R3)
+		ctl_mask |= R3_MODE;
+
+	if (ctl_mask)
+		ret |= adp5520_set_bits(dev->master, LED_CONTROL,
+			ctl_mask);
+
+	ret |= adp5520_set_bits(dev->master, GPIO_PULLUP,
+		pdata->rows_en_mask);
+
+	if (ret) {
+		dev_err(&pdev->dev, "failed to write\n");
+		ret = -EIO;
+		goto err1;
+	}
+
+	dev->notifier.notifier_call = adp5520_keys_notifier;
+	ret = adp5520_register_notifier(dev->master, &dev->notifier,
+			KP_IEN | KR_IEN);
+	if (ret) {
+		dev_err(&pdev->dev, "failed to register notifier\n");
+		goto err1;
+	}
+
+	platform_set_drvdata(pdev, dev);
+	return 0;
+
+err1:
+	input_unregister_device(input);
+	input = NULL;
+err:
+	input_free_device(input);
+	kfree(dev);
+	return ret;
+}
+
+static int __devexit adp5520_keys_remove(struct platform_device *pdev)
+{
+	struct adp5520_keys *dev = platform_get_drvdata(pdev);
+
+	adp5520_unregister_notifier(dev->master, &dev->notifier,
+				KP_IEN | KR_IEN);
+
+	input_unregister_device(dev->input);
+	kfree(dev);
+	return 0;
+}
+
+static struct platform_driver adp5520_keys_driver = {
+	.driver	= {
+		.name	= "adp5520-keys",
+		.owner	= THIS_MODULE,
+	},
+	.probe		= adp5520_keys_probe,
+	.remove		= __devexit_p(adp5520_keys_remove),
+};
+
+static int __init adp5520_keys_init(void)
+{
+	return platform_driver_register(&adp5520_keys_driver);
+}
+module_init(adp5520_keys_init);
+
+static void __exit adp5520_keys_exit(void)
+{
+	platform_driver_unregister(&adp5520_keys_driver);
+}
+module_exit(adp5520_keys_exit);
+
+MODULE_AUTHOR("Michael Hennerich <hennerich@blackfin.uclinux.org>");
+MODULE_DESCRIPTION("Keys ADP5520 Driver");
+MODULE_LICENSE("GPL");
+MODULE_ALIAS("platform:adp5520-keys");
-- 
1.6.5.rc1


^ permalink raw reply related	[flat|nested] 20+ messages in thread

* Re: [PATCH v2] input/keyboard: new driver for ADP5520 MFD PMICs
  2009-09-23  5:15 ` [PATCH v2] " Mike Frysinger
@ 2009-09-23  6:28   ` Dmitry Torokhov
  0 siblings, 0 replies; 20+ messages in thread
From: Dmitry Torokhov @ 2009-09-23  6:28 UTC (permalink / raw)
  To: Mike Frysinger
  Cc: linux-input, linux-kernel, uclinux-dist-devel, Michael Hennerich,
	Bryan Wu

On Wed, Sep 23, 2009 at 01:15:16AM -0400, Mike Frysinger wrote:
> From: Michael Hennerich <michael.hennerich@analog.com>
> 
> Signed-off-by: Michael Hennerich <michael.hennerich@analog.com>
> Signed-off-by: Bryan Wu <cooloney@kernel.org>
> Signed-off-by: Mike Frysinger <vapier@gentoo.org>
> ---
> this patch requires:
> 	[PATCH] mfd: ADP5520 Multifunction LCD Backlight and Keypad Input Device Driver
> 
> v2
> 	- address feedback from Dmitry
> 

Looks good. I assume you want it to be merged with the rest of ADP5520
patches, so:

Acked-by: Dmitry Torokhov <dtor@mail.ru>

-- 
Dmitry

^ permalink raw reply	[flat|nested] 20+ messages in thread

end of thread, other threads:[~2009-09-23  6:28 UTC | newest]

Thread overview: 20+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2009-09-17 18:24 [PATCH] input/keyboard: new driver for ADP5520 MFD PMICs Mike Frysinger
2009-09-20  6:30 ` Andrew Morton
2009-09-20  6:30   ` Andrew Morton
2009-09-20 22:40   ` [Uclinux-dist-devel] " Mike Frysinger
2009-09-20 22:40     ` Mike Frysinger
2009-09-21  4:04     ` Andrew Morton
2009-09-21  4:04       ` Andrew Morton
2009-09-21 13:40       ` Mike Frysinger
2009-09-21 13:40         ` Mike Frysinger
2009-09-21 18:23         ` [Uclinux-dist-devel] " Andrew Morton
2009-09-21 18:23           ` Andrew Morton
2009-09-22  5:59 ` Dmitry Torokhov
2009-09-22  7:33   ` Jiri Kosina
2009-09-22  7:53     ` Hennerich, Michael
2009-09-22  7:53       ` Hennerich, Michael
2009-09-22  7:56       ` Jiri Kosina
2009-09-22 14:21         ` Mike Frysinger
2009-09-22 15:06       ` Dmitry Torokhov
2009-09-23  5:15 ` [PATCH v2] " Mike Frysinger
2009-09-23  6:28   ` Dmitry Torokhov

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.