[PATCH 00/03] input: mode 6 patches for sh_keysc

[PATCH 00/03] input: mode 6 patches for sh_keysc

[Magnus Damm]

input: mode 6 patches for sh_keysc

[PATCH 01/03] input: break out hw access functions in sh_keysc 
[PATCH 02/03] input: bitmap update for sh_keysc
[PATCH 03/03] input: update sh_keysc driver with mode 6

These patches improve the sh_keysc driver with mode 6 support.

The earlier submitted patch for mode 4 and mode 5 extended the
driver with up to 6 * 7 keys. Mode 6 support included in this
patch set allows up to 7 * 7 keys.

The sh_keysc driver has so far only been used with hardware
where the number of keys can fit in a 32-bit bitmap, but from
mode 4 and up these 32 bits are no longer enough. To address
this proper bitmaps are made use of instead 32-bit integers.

Signed-off-by: Magnus Damm <damm@opensource.se>
---

 drivers/input/keyboard/sh_keysc.c |  155 ++++++++++++++++++++++++-------------
 include/linux/input/sh_keysc.h    |    6 -
 2 files changed, 106 insertions(+), 55 deletions(-)

[PATCH 01/03] input: break out hw access functions in sh_keysc

[Magnus Damm]

From: Magnus Damm <damm@opensource.se>

Update the sh_keysc driver to break out the register
access functions sh_keysc_read(), sh_keysc_write()
together with sh_keysc_level_mode(). This makes the
code a bit easier to follow.

Signed-off-by: Magnus Damm <damm@opensource.se>
---

 drivers/input/keyboard/sh_keysc.c |   69 +++++++++++++++++++++++--------------
 1 file changed, 43 insertions(+), 26 deletions(-)

--- 0004/drivers/input/keyboard/sh_keysc.c
+++ work/drivers/input/keyboard/sh_keysc.c	2010-02-05 13:02:22.000000000 +0900
@@ -22,14 +22,6 @@
 #include <linux/clk.h>
 #include <linux/io.h>
 
-#define KYCR1_OFFS   0x00
-#define KYCR2_OFFS   0x04
-#define KYINDR_OFFS  0x08
-#define KYOUTDR_OFFS 0x0c
-
-#define KYCR2_IRQ_LEVEL    0x10
-#define KYCR2_IRQ_DISABLED 0x00
-
 static const struct {
 	unsigned char kymd, keyout, keyin;
 } sh_keysc_mode[] = {
@@ -48,6 +40,37 @@ struct sh_keysc_priv {
 	struct sh_keysc_info pdata;
 };
 
+#define KYCR1 0
+#define KYCR2 1
+#define KYINDR 2
+#define KYOUTDR 3
+
+#define KYCR2_IRQ_LEVEL    0x10
+#define KYCR2_IRQ_DISABLED 0x00
+
+static unsigned long sh_keysc_read(struct sh_keysc_priv *p, int reg_nr)
+{
+	return ioread16(p->iomem_base + (reg_nr << 2));
+}
+
+static void sh_keysc_write(struct sh_keysc_priv *p, int reg_nr,
+			   unsigned long value)
+{
+	iowrite16(value, p->iomem_base + (reg_nr << 2));
+}
+
+static void sh_keysc_level_mode(struct sh_keysc_priv *p,
+				unsigned long keys_set)
+{
+	struct sh_keysc_info *pdata = &p->pdata;
+
+	sh_keysc_write(p, KYOUTDR, 0);
+	sh_keysc_write(p, KYCR2, KYCR2_IRQ_LEVEL | (keys_set << 8));
+
+	if (pdata->kycr2_delay)
+		udelay(pdata->kycr2_delay);
+}
+
 static irqreturn_t sh_keysc_isr(int irq, void *dev_id)
 {
 	struct platform_device *pdev = dev_id;
@@ -66,24 +89,19 @@ static irqreturn_t sh_keysc_isr(int irq,
 		keys = 0;
 		keyin_set = 0;
 
-		iowrite16(KYCR2_IRQ_DISABLED, priv->iomem_base + KYCR2_OFFS);
+		sh_keysc_write(priv, KYCR2, KYCR2_IRQ_DISABLED);
 
 		for (i = 0; i < sh_keysc_mode[pdata->mode].keyout; i++) {
-			iowrite16(0xfff ^ (3 << (i * 2)),
-				  priv->iomem_base + KYOUTDR_OFFS);
+			sh_keysc_write(priv, KYOUTDR, 0xfff ^ (3 << (i * 2)));
 			udelay(pdata->delay);
-			tmp = ioread16(priv->iomem_base + KYINDR_OFFS);
+			tmp = sh_keysc_read(priv, KYINDR);
+
 			keys |= tmp << (sh_keysc_mode[pdata->mode].keyin * i);
 			tmp ^= (1 << sh_keysc_mode[pdata->mode].keyin) - 1;
 			keyin_set |= tmp;
 		}
 
-		iowrite16(0, priv->iomem_base + KYOUTDR_OFFS);
-		iowrite16(KYCR2_IRQ_LEVEL | (keyin_set << 8),
-			  priv->iomem_base + KYCR2_OFFS);
-
-		if (pdata->kycr2_delay)
-			udelay(pdata->kycr2_delay);
+		sh_keysc_level_mode(priv, keyin_set);
 
 		keys ^= ~0;
 		keys &= (1 << (sh_keysc_mode[pdata->mode].keyin *
@@ -93,7 +111,7 @@ static irqreturn_t sh_keysc_isr(int irq,
 
 		dev_dbg(&pdev->dev, "keys 0x%08lx\n", keys);
 
-	} while (ioread16(priv->iomem_base + KYCR2_OFFS) & 0x01);
+	} while (sh_keysc_read(priv, KYCR2) & 0x01);
 
 	dev_dbg(&pdev->dev, "last_keys 0x%08lx keys0 0x%08lx keys1 0x%08lx\n",
 		priv->last_keys, keys0, keys1);
@@ -220,10 +238,9 @@ static int __devinit sh_keysc_probe(stru
 
 	clk_enable(priv->clk);
 
-	iowrite16((sh_keysc_mode[pdata->mode].kymd << 8) |
-		  pdata->scan_timing, priv->iomem_base + KYCR1_OFFS);
-	iowrite16(0, priv->iomem_base + KYOUTDR_OFFS);
-	iowrite16(KYCR2_IRQ_LEVEL, priv->iomem_base + KYCR2_OFFS);
+	sh_keysc_write(priv, KYCR1, (sh_keysc_mode[pdata->mode].kymd << 8) |
+		       pdata->scan_timing);
+	sh_keysc_level_mode(priv, 0);
 
 	device_init_wakeup(&pdev->dev, 1);
 
@@ -248,7 +265,7 @@ static int __devexit sh_keysc_remove(str
 {
 	struct sh_keysc_priv *priv = platform_get_drvdata(pdev);
 
-	iowrite16(KYCR2_IRQ_DISABLED, priv->iomem_base + KYCR2_OFFS);
+	sh_keysc_write(priv, KYCR2, KYCR2_IRQ_DISABLED);
 
 	input_unregister_device(priv->input);
 	free_irq(platform_get_irq(pdev, 0), pdev);
@@ -270,7 +287,7 @@ static int sh_keysc_suspend(struct devic
 	int irq = platform_get_irq(pdev, 0);
 	unsigned short value;
 
-	value = ioread16(priv->iomem_base + KYCR1_OFFS);
+	value = sh_keysc_read(priv, KYCR1);
 
 	if (device_may_wakeup(dev)) {
 		value |= 0x80;
@@ -279,7 +296,7 @@ static int sh_keysc_suspend(struct devic
 		value &= ~0x80;
 	}
 
-	iowrite16(value, priv->iomem_base + KYCR1_OFFS);
+	sh_keysc_write(priv, KYCR1, value);
 
 	return 0;
 }

[PATCH 02/03] input: bitmap update for sh_keysc

[Magnus Damm]

From: Magnus Damm <damm@opensource.se>

Update the sh_keysc driver with proper bitmap support
instead of using a fixed 32-bit integer to keep track
for the key states. This allows the driver to support
key pads with more than 32 keys.

Signed-off-by: Magnus Damm <damm@opensource.se>
---

 drivers/input/keyboard/sh_keysc.c |   83 +++++++++++++++++++++++++------------
 1 file changed, 58 insertions(+), 25 deletions(-)

--- 0005/drivers/input/keyboard/sh_keysc.c
+++ work/drivers/input/keyboard/sh_keysc.c	2010-02-05 13:13:24.000000000 +0900
@@ -19,6 +19,7 @@
 #include <linux/platform_device.h>
 #include <linux/input.h>
 #include <linux/input/sh_keysc.h>
+#include <linux/bitmap.h>
 #include <linux/clk.h>
 #include <linux/io.h>
 
@@ -32,10 +33,14 @@ static const struct {
 	[SH_KEYSC_MODE_5] = { 4, 6, 7 },
 };
 
+struct sh_keysc_map {
+	DECLARE_BITMAP(b, SH_KEYSC_MAXKEYS);
+};
+
 struct sh_keysc_priv {
 	void __iomem *iomem_base;
 	struct clk *clk;
-	unsigned long last_keys;
+	struct sh_keysc_map last_keys;
 	struct input_dev *input;
 	struct sh_keysc_info pdata;
 };
@@ -71,69 +76,97 @@ static void sh_keysc_level_mode(struct s
 		udelay(pdata->kycr2_delay);
 }
 
+#define WRAP(fn, m...) bitmap_##fn(m, SH_KEYSC_MAXKEYS)
+#define sh_keysc_map_zero(m) WRAP(zero, (m)->b)
+#define sh_keysc_map_fill(m) WRAP(fill, (m)->b)
+#define sh_keysc_map_and(m, m2) WRAP(and, (m)->b, (m)->b, (m2)->b)
+#define sh_keysc_map_or(m, m2) WRAP(or, (m)->b, (m)->b, (m2)->b)
+#define sh_keysc_map_complement(m) WRAP(complement, (m)->b, (m)->b)
+#define sh_keysc_map_set(m, n) set_bit((n), (m)->b)
+#define sh_keysc_map_clear(m, n) clear_bit((n), (m)->b)
+#define sh_keysc_map_test(m, n) test_bit((n), (m)->b)
+
+static void sh_keysc_map_dbg(struct device *dev, struct sh_keysc_map *map,
+			     const char *str)
+{
+	int k;
+
+	for (k = 0; k < BITS_TO_LONGS(SH_KEYSC_MAXKEYS); k++)
+		dev_dbg(dev, "%s[%d] 0x%lx\n", str, k, map->b[k]);
+}
+
 static irqreturn_t sh_keysc_isr(int irq, void *dev_id)
 {
 	struct platform_device *pdev = dev_id;
 	struct sh_keysc_priv *priv = platform_get_drvdata(pdev);
 	struct sh_keysc_info *pdata = &priv->pdata;
-	unsigned long keys, keys1, keys0, mask;
+	int keyout_nr = sh_keysc_mode[pdata->mode].keyout;
+	int keyin_nr = sh_keysc_mode[pdata->mode].keyin;
+	struct sh_keysc_map keys, keys1, keys0;
 	unsigned char keyin_set, tmp;
-	int i, k;
+	int i, k, n;
 
 	dev_dbg(&pdev->dev, "isr!\n");
 
-	keys1 = ~0;
-	keys0 = 0;
+	sh_keysc_map_fill(&keys1);
+	sh_keysc_map_zero(&keys0);
 
 	do {
-		keys = 0;
+		sh_keysc_map_zero(&keys);
 		keyin_set = 0;
 
 		sh_keysc_write(priv, KYCR2, KYCR2_IRQ_DISABLED);
 
-		for (i = 0; i < sh_keysc_mode[pdata->mode].keyout; i++) {
+		for (i = 0; i < keyout_nr; i++) {
+			n = keyin_nr * i;
+
+			/* drive one KEYOUT pin low, read KEYIN pins */
 			sh_keysc_write(priv, KYOUTDR, 0xfff ^ (3 << (i * 2)));
 			udelay(pdata->delay);
 			tmp = sh_keysc_read(priv, KYINDR);
 
-			keys |= tmp << (sh_keysc_mode[pdata->mode].keyin * i);
-			tmp ^= (1 << sh_keysc_mode[pdata->mode].keyin) - 1;
-			keyin_set |= tmp;
+			/* set bit if key press has been detected */
+			for (k = 0; k < keyin_nr; k++) {
+				if (tmp & (1 << k))
+					sh_keysc_map_set(&keys, n + k);
+			}
+
+			/* keep track of which KEYIN bits that have been set */
+			keyin_set |= tmp ^ ((1 << keyin_nr) - 1);
 		}
 
 		sh_keysc_level_mode(priv, keyin_set);
 
-		keys ^= ~0;
-		keys &= (1 << (sh_keysc_mode[pdata->mode].keyin *
-			       sh_keysc_mode[pdata->mode].keyout)) - 1;
-		keys1 &= keys;
-		keys0 |= keys;
+		sh_keysc_map_complement(&keys);
+		sh_keysc_map_and(&keys1, &keys);
+		sh_keysc_map_or(&keys0, &keys);
 
-		dev_dbg(&pdev->dev, "keys 0x%08lx\n", keys);
+		sh_keysc_map_dbg(&pdev->dev, &keys, "keys");
 
 	} while (sh_keysc_read(priv, KYCR2) & 0x01);
 
-	dev_dbg(&pdev->dev, "last_keys 0x%08lx keys0 0x%08lx keys1 0x%08lx\n",
-		priv->last_keys, keys0, keys1);
+	sh_keysc_map_dbg(&pdev->dev, &priv->last_keys, "last_keys");
+	sh_keysc_map_dbg(&pdev->dev, &keys0, "keys0");
+	sh_keysc_map_dbg(&pdev->dev, &keys1, "keys1");
 
 	for (i = 0; i < SH_KEYSC_MAXKEYS; i++) {
 		k = pdata->keycodes[i];
 		if (!k)
 			continue;
 
-		mask = 1 << i;
-
-		if (!((priv->last_keys ^ keys0) & mask))
+		if (sh_keysc_map_test(&keys0, i) ==
+		    sh_keysc_map_test(&priv->last_keys, i))
 			continue;
 
-		if ((keys1 | keys0) & mask) {
+		if (sh_keysc_map_test(&keys1, i) ||
+		    sh_keysc_map_test(&keys0, i)) {
 			input_event(priv->input, EV_KEY, k, 1);
-			priv->last_keys |= mask;
+			sh_keysc_map_set(&priv->last_keys, i);
 		}
 
-		if (!(keys1 & mask)) {
+		if (!sh_keysc_map_test(&keys1, i)) {
 			input_event(priv->input, EV_KEY, k, 0);
-			priv->last_keys &= ~mask;
+			sh_keysc_map_clear(&priv->last_keys, i);
 		}
 
 	}

[PATCH 03/03] input: update sh_keysc driver with mode 6

[Magnus Damm]

From: Magnus Damm <damm@opensource.se>

Add mode 6 support to the sh_keysc driver. Also update
the KYOUTDR mask value to include all 16 register bits.

Signed-off-by: Magnus Damm <damm@opensource.se>
---

 drivers/input/keyboard/sh_keysc.c |    3 ++-
 include/linux/input/sh_keysc.h    |    6 +++---
 2 files changed, 5 insertions(+), 4 deletions(-)

--- 0006/drivers/input/keyboard/sh_keysc.c
+++ work/drivers/input/keyboard/sh_keysc.c	2010-02-05 13:19:52.000000000 +0900
@@ -31,6 +31,7 @@ static const struct {
 	[SH_KEYSC_MODE_3] = { 2, 4, 7 },
 	[SH_KEYSC_MODE_4] = { 3, 6, 6 },
 	[SH_KEYSC_MODE_5] = { 4, 6, 7 },
+	[SH_KEYSC_MODE_6] = { 5, 7, 7 },
 };
 
 struct sh_keysc_map {
@@ -121,7 +122,7 @@ static irqreturn_t sh_keysc_isr(int irq,
 			n = keyin_nr * i;
 
 			/* drive one KEYOUT pin low, read KEYIN pins */
-			sh_keysc_write(priv, KYOUTDR, 0xfff ^ (3 << (i * 2)));
+			sh_keysc_write(priv, KYOUTDR, 0xffff ^ (3 << (i * 2)));
 			udelay(pdata->delay);
 			tmp = sh_keysc_read(priv, KYINDR);
 
--- 0004/include/linux/input/sh_keysc.h
+++ work/include/linux/input/sh_keysc.h	2010-02-05 13:19:52.000000000 +0900
@@ -1,15 +1,15 @@
 #ifndef __SH_KEYSC_H__
 #define __SH_KEYSC_H__
 
-#define SH_KEYSC_MAXKEYS 42
+#define SH_KEYSC_MAXKEYS 49
 
 struct sh_keysc_info {
 	enum { SH_KEYSC_MODE_1, SH_KEYSC_MODE_2, SH_KEYSC_MODE_3,
-	       SH_KEYSC_MODE_4, SH_KEYSC_MODE_5 } mode;
+	       SH_KEYSC_MODE_4, SH_KEYSC_MODE_5, SH_KEYSC_MODE_6 } mode;
 	int scan_timing; /* 0 -> 7, see KYCR1, SCN[2:0] */
 	int delay;
 	int kycr2_delay;
-	int keycodes[SH_KEYSC_MAXKEYS];
+	int keycodes[SH_KEYSC_MAXKEYS]; /* KEYIN * KEYOUT */
 };
 
 #endif /* __SH_KEYSC_H__ */

Re: [PATCH 02/03] input: bitmap update for sh_keysc

[Dmitry Torokhov]

Hi Magnus,

On Mon, Feb 08, 2010 at 03:32:34PM +0900, Magnus Damm wrote:
>  
> +#define WRAP(fn, m...) bitmap_##fn(m, SH_KEYSC_MAXKEYS)
> +#define sh_keysc_map_zero(m) WRAP(zero, (m)->b)
> +#define sh_keysc_map_fill(m) WRAP(fill, (m)->b)
> +#define sh_keysc_map_and(m, m2) WRAP(and, (m)->b, (m)->b, (m2)->b)
> +#define sh_keysc_map_or(m, m2) WRAP(or, (m)->b, (m)->b, (m2)->b)
> +#define sh_keysc_map_complement(m) WRAP(complement, (m)->b, (m)->b)
> +#define sh_keysc_map_set(m, n) set_bit((n), (m)->b)
> +#define sh_keysc_map_clear(m, n) clear_bit((n), (m)->b)
> +#define sh_keysc_map_test(m, n) test_bit((n), (m)->b)
> +

Why do you need these wrappers? For me they simply create a distraction,
later when I read the code I will have to go and look up what
sh_keysc_map_set() means but if I see __set_bit() I'd know right away.

Thanks.

-- 
Dmitry

Re: [PATCH 02/03] input: bitmap update for sh_keysc

[Magnus Damm]

Hi Dmitry,

On Mon, Feb 8, 2010 at 4:18 PM, Dmitry Torokhov
<dmitry.torokhov@gmail.com> wrote:
> On Mon, Feb 08, 2010 at 03:32:34PM +0900, Magnus Damm wrote:
>>
>> +#define WRAP(fn, m...) bitmap_##fn(m, SH_KEYSC_MAXKEYS)
>> +#define sh_keysc_map_zero(m) WRAP(zero, (m)->b)
>> +#define sh_keysc_map_fill(m) WRAP(fill, (m)->b)
>> +#define sh_keysc_map_and(m, m2) WRAP(and, (m)->b, (m)->b, (m2)->b)
>> +#define sh_keysc_map_or(m, m2) WRAP(or, (m)->b, (m)->b, (m2)->b)
>> +#define sh_keysc_map_complement(m) WRAP(complement, (m)->b, (m)->b)
>> +#define sh_keysc_map_set(m, n) set_bit((n), (m)->b)
>> +#define sh_keysc_map_clear(m, n) clear_bit((n), (m)->b)
>> +#define sh_keysc_map_test(m, n) test_bit((n), (m)->b)
>> +
>
> Why do you need these wrappers? For me they simply create a distraction,
> later when I read the code I will have to go and look up what
> sh_keysc_map_set() means but if I see __set_bit() I'd know right away.

To avoid duplicating SH_KEYSC_MAXKEYS all over the place I started out
by wrapping bitmap_zero/fill/and/or/complement(). To be consistent I
decided to wrap the set/clear/test_bit() functions as well, but it may
be better to leave them alone.

Are you ok with wrapping the bitmap_...() functions to avoid
duplicating SH_KEYSC_MAXKEYS?

Cheers,

/ magnus

Re: [PATCH 02/03] input: bitmap update for sh_keysc

[Paul Mundt]

On Mon, Feb 08, 2010 at 04:41:16PM +0900, Magnus Damm wrote:
> Hi Dmitry,
> 
> On Mon, Feb 8, 2010 at 4:18 PM, Dmitry Torokhov
> <dmitry.torokhov@gmail.com> wrote:
> > On Mon, Feb 08, 2010 at 03:32:34PM +0900, Magnus Damm wrote:
> >>
> >> +#define WRAP(fn, m...) bitmap_##fn(m, SH_KEYSC_MAXKEYS)
> >> +#define sh_keysc_map_zero(m) WRAP(zero, (m)->b)
> >> +#define sh_keysc_map_fill(m) WRAP(fill, (m)->b)
> >> +#define sh_keysc_map_and(m, m2) WRAP(and, (m)->b, (m)->b, (m2)->b)
> >> +#define sh_keysc_map_or(m, m2) WRAP(or, (m)->b, (m)->b, (m2)->b)
> >> +#define sh_keysc_map_complement(m) WRAP(complement, (m)->b, (m)->b)
> >> +#define sh_keysc_map_set(m, n) set_bit((n), (m)->b)
> >> +#define sh_keysc_map_clear(m, n) clear_bit((n), (m)->b)
> >> +#define sh_keysc_map_test(m, n) test_bit((n), (m)->b)
> >> +
> >
> > Why do you need these wrappers? For me they simply create a distraction,
> > later when I read the code I will have to go and look up what
> > sh_keysc_map_set() means but if I see __set_bit() I'd know right away.
> 
> To avoid duplicating SH_KEYSC_MAXKEYS all over the place I started out
> by wrapping bitmap_zero/fill/and/or/complement(). To be consistent I
> decided to wrap the set/clear/test_bit() functions as well, but it may
> be better to leave them alone.
> 
> Are you ok with wrapping the bitmap_...() functions to avoid
> duplicating SH_KEYSC_MAXKEYS?
> 
I don't see the point, either. The bitmap routines take the maximum size
of the bitmap as an argument, and so code that uses the API heavily and
has the size defined in a macro is going to have it sprinkled around, who
cares?

Wrapping up the bitmap API in your own wrappers to hide this fact only
makes it more confusing and really doesn't buy you anything. While things
like sh_keysc_map_set/clear() might seem obvious enough, it's also
impossible to tell whether these employ __set_bit()/__clear_bit() or
set_bit()/clear_bit() semantics without having to scroll back and look
them up. The added obfuscation to save yourself from having to sprinkle
SH_KEYSC_MAXKEYS around simply isn't worth it.

Any time you start out wrapping an existing API for private driver use
outside of things like I/O accessors, you're almost certainly going to
run in to trouble.