linux-input.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] input: bitmap update for sh_keysc V2
@ 2010-02-09  9:16 Magnus Damm
  2010-02-09  9:39 ` Dmitry Torokhov
  0 siblings, 1 reply; 6+ messages in thread
From: Magnus Damm @ 2010-02-09  9:16 UTC (permalink / raw)
  To: linux-input; +Cc: lethal, Magnus Damm, dmitry.torokhov, linux-sh

From: Magnus Damm <damm@opensource.se>

Update the sh_keysc driver with proper bitmap support V2.
This instead of using a fixed 32-bit integer to keep track
for the key states. With this change in place the driver
supports key pads with more than 32 keys.

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

 Changes since V1:
 - ditched the wrapping macros
 - use __set_bit() and __clear_bit()

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

--- 0010/drivers/input/keyboard/sh_keysc.c
+++ work/drivers/input/keyboard/sh_keysc.c	2010-02-09 18:01:32.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,85 @@ static void sh_keysc_level_mode(struct s
 		udelay(pdata->kycr2_delay);
 }
 
+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;
+	bitmap_fill(keys1.b, SH_KEYSC_MAXKEYS);
+	bitmap_zero(keys0.b, SH_KEYSC_MAXKEYS);
 
 	do {
-		keys = 0;
+		bitmap_zero(keys.b, SH_KEYSC_MAXKEYS);
 		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))
+					__set_bit(n + k, keys.b);
+			}
+
+			/* 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;
+		bitmap_complement(keys.b, keys.b, SH_KEYSC_MAXKEYS);
+		bitmap_and(keys1.b, keys1.b, keys.b, SH_KEYSC_MAXKEYS);
+		bitmap_or(keys0.b, keys0.b, keys.b, SH_KEYSC_MAXKEYS);
 
-		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 (test_bit(i, keys0.b) == test_bit(i, priv->last_keys.b))
 			continue;
 
-		if ((keys1 | keys0) & mask) {
+		if (test_bit(i, keys1.b) || test_bit(i, keys0.b)) {
 			input_event(priv->input, EV_KEY, k, 1);
-			priv->last_keys |= mask;
+			__set_bit(i, priv->last_keys.b);
 		}
 
-		if (!(keys1 & mask)) {
+		if (!test_bit(i, keys1.b)) {
 			input_event(priv->input, EV_KEY, k, 0);
-			priv->last_keys &= ~mask;
+			__clear_bit(i, priv->last_keys.b);
 		}
 
 	}

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

* Re: [PATCH] input: bitmap update for sh_keysc V2
  2010-02-09  9:16 [PATCH] input: bitmap update for sh_keysc V2 Magnus Damm
@ 2010-02-09  9:39 ` Dmitry Torokhov
  2010-02-10  4:13   ` Magnus Damm
  0 siblings, 1 reply; 6+ messages in thread
From: Dmitry Torokhov @ 2010-02-09  9:39 UTC (permalink / raw)
  To: Magnus Damm; +Cc: linux-input, lethal, linux-sh

On Tue, Feb 09, 2010 at 06:16:09PM +0900, Magnus Damm wrote:
> From: Magnus Damm <damm@opensource.se>
> 
> Update the sh_keysc driver with proper bitmap support V2.
> This instead of using a fixed 32-bit integer to keep track
> for the key states. With this change in place the driver
> supports key pads with more than 32 keys.
> 
> Signed-off-by: Magnus Damm <damm@opensource.se>
> ---
> 
>  Changes since V1:
>  - ditched the wrapping macros
>  - use __set_bit() and __clear_bit()

Thank you for making the change. It indeed is easier to read than the
original since one does not have to go and look up what wrappers are
doing.

-- 
Dmitry

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

* Re: [PATCH] input: bitmap update for sh_keysc V2
  2010-02-09  9:39 ` Dmitry Torokhov
@ 2010-02-10  4:13   ` Magnus Damm
  2010-02-10  9:26     ` Dmitry Torokhov
  0 siblings, 1 reply; 6+ messages in thread
From: Magnus Damm @ 2010-02-10  4:13 UTC (permalink / raw)
  To: Dmitry Torokhov; +Cc: linux-input, lethal, linux-sh

On Tue, Feb 9, 2010 at 6:39 PM, Dmitry Torokhov
<dmitry.torokhov@gmail.com> wrote:
> On Tue, Feb 09, 2010 at 06:16:09PM +0900, Magnus Damm wrote:
>> From: Magnus Damm <damm@opensource.se>
>>
>> Update the sh_keysc driver with proper bitmap support V2.
>> This instead of using a fixed 32-bit integer to keep track
>> for the key states. With this change in place the driver
>> supports key pads with more than 32 keys.
>>
>> Signed-off-by: Magnus Damm <damm@opensource.se>
>> ---
>>
>>  Changes since V1:
>>  - ditched the wrapping macros
>>  - use __set_bit() and __clear_bit()
>
> Thank you for making the change. It indeed is easier to read than the
> original since one does not have to go and look up what wrappers are
> doing.

Yeah, I initially disliked the SH_KEYSC_MAXKEYS that now are sprinkled
all over the place, but I have to admit that not using wrappers made
me realize that I should use __set_bit() and __clear_bit() instead of
the atomic ones. So all good.

Thanks!

/ magnus
--
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] 6+ messages in thread

* Re: [PATCH] input: bitmap update for sh_keysc V2
  2010-02-10  4:13   ` Magnus Damm
@ 2010-02-10  9:26     ` Dmitry Torokhov
  2010-02-10 11:13       ` Magnus Damm
  0 siblings, 1 reply; 6+ messages in thread
From: Dmitry Torokhov @ 2010-02-10  9:26 UTC (permalink / raw)
  To: Magnus Damm; +Cc: linux-input, lethal, linux-sh

On Wed, Feb 10, 2010 at 01:13:37PM +0900, Magnus Damm wrote:
> On Tue, Feb 9, 2010 at 6:39 PM, Dmitry Torokhov
> <dmitry.torokhov@gmail.com> wrote:
> > On Tue, Feb 09, 2010 at 06:16:09PM +0900, Magnus Damm wrote:
> >> From: Magnus Damm <damm@opensource.se>
> >>
> >> Update the sh_keysc driver with proper bitmap support V2.
> >> This instead of using a fixed 32-bit integer to keep track
> >> for the key states. With this change in place the driver
> >> supports key pads with more than 32 keys.
> >>
> >> Signed-off-by: Magnus Damm <damm@opensource.se>
> >> ---
> >>
> >>  Changes since V1:
> >>  - ditched the wrapping macros
> >>  - use __set_bit() and __clear_bit()
> >
> > Thank you for making the change. It indeed is easier to read than the
> > original since one does not have to go and look up what wrappers are
> > doing.
> 
> Yeah, I initially disliked the SH_KEYSC_MAXKEYS that now are sprinkled
> all over the place, but I have to admit that not using wrappers made
> me realize that I should use __set_bit() and __clear_bit() instead of
> the atomic ones. So all good.
> 

BTW, that sh_keysc_map seems to be a holdover from the V1 of the patch
and is not needed anymore. Does the patch below still work for you?

Thanks!

-- 
Dmitry


Input: sh_keysc - switch to using bitmaps

From: Magnus Damm <damm@opensource.se>

Use bitmaps instead of using 32-bit integers to keep track of the key
states. With this change in place the driver supports key pads with
more than 32 keys.

Signed-off-by: Magnus Damm <damm@opensource.se>
Signed-off-by: Dmitry Torokhov <dtor@mail.ru>
---

 drivers/input/keyboard/sh_keysc.c |   69 ++++++++++++++++++++++++-------------
 1 files changed, 44 insertions(+), 25 deletions(-)


diff --git a/drivers/input/keyboard/sh_keysc.c b/drivers/input/keyboard/sh_keysc.c
index 6218b2f..c2fc977 100644
--- a/drivers/input/keyboard/sh_keysc.c
+++ b/drivers/input/keyboard/sh_keysc.c
@@ -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>
 
@@ -35,7 +36,7 @@ static const struct {
 struct sh_keysc_priv {
 	void __iomem *iomem_base;
 	struct clk *clk;
-	unsigned long last_keys;
+	DECLARE_BITMAP(last_keys, SH_KEYSC_MAXKEYS);
 	struct input_dev *input;
 	struct sh_keysc_info pdata;
 };
@@ -71,69 +72,87 @@ static void sh_keysc_level_mode(struct sh_keysc_priv *p,
 		udelay(pdata->kycr2_delay);
 }
 
+static void sh_keysc_map_dbg(struct device *dev, unsigned long *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[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;
+	DECLARE_BITMAP(keys, SH_KEYSC_MAXKEYS);
+	DECLARE_BITMAP(keys0, SH_KEYSC_MAXKEYS);
+	DECLARE_BITMAP(keys1, SH_KEYSC_MAXKEYS);
 	unsigned char keyin_set, tmp;
-	int i, k;
+	int i, k, n;
 
 	dev_dbg(&pdev->dev, "isr!\n");
 
-	keys1 = ~0;
-	keys0 = 0;
+	bitmap_fill(keys1, SH_KEYSC_MAXKEYS);
+	bitmap_zero(keys0, SH_KEYSC_MAXKEYS);
 
 	do {
-		keys = 0;
+		bitmap_zero(keys, SH_KEYSC_MAXKEYS);
 		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))
+					__set_bit(n + k, keys);
+			}
+
+			/* 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;
+		bitmap_complement(keys, keys, SH_KEYSC_MAXKEYS);
+		bitmap_and(keys1, keys1, keys, SH_KEYSC_MAXKEYS);
+		bitmap_or(keys0, keys0, keys, SH_KEYSC_MAXKEYS);
 
-		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 (test_bit(i, keys0) == test_bit(i, priv->last_keys))
 			continue;
 
-		if ((keys1 | keys0) & mask) {
+		if (test_bit(i, keys1) || test_bit(i, keys0)) {
 			input_event(priv->input, EV_KEY, k, 1);
-			priv->last_keys |= mask;
+			__set_bit(i, priv->last_keys);
 		}
 
-		if (!(keys1 & mask)) {
+		if (!test_bit(i, keys1)) {
 			input_event(priv->input, EV_KEY, k, 0);
-			priv->last_keys &= ~mask;
+			__clear_bit(i, priv->last_keys);
 		}
 
 	}
--
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 related	[flat|nested] 6+ messages in thread

* Re: [PATCH] input: bitmap update for sh_keysc V2
  2010-02-10  9:26     ` Dmitry Torokhov
@ 2010-02-10 11:13       ` Magnus Damm
  2010-02-10 17:59         ` Dmitry Torokhov
  0 siblings, 1 reply; 6+ messages in thread
From: Magnus Damm @ 2010-02-10 11:13 UTC (permalink / raw)
  To: Dmitry Torokhov; +Cc: linux-input, lethal, linux-sh

On Wed, Feb 10, 2010 at 6:26 PM, Dmitry Torokhov
<dmitry.torokhov@gmail.com> wrote:
> On Wed, Feb 10, 2010 at 01:13:37PM +0900, Magnus Damm wrote:
>> On Tue, Feb 9, 2010 at 6:39 PM, Dmitry Torokhov
>> <dmitry.torokhov@gmail.com> wrote:
>> > On Tue, Feb 09, 2010 at 06:16:09PM +0900, Magnus Damm wrote:
>> >> From: Magnus Damm <damm@opensource.se>
>> >>
>> >> Update the sh_keysc driver with proper bitmap support V2.
>> >> This instead of using a fixed 32-bit integer to keep track
>> >> for the key states. With this change in place the driver
>> >> supports key pads with more than 32 keys.
>> >>
>> >> Signed-off-by: Magnus Damm <damm@opensource.se>
>> >> ---
>> >>
>> >>  Changes since V1:
>> >>  - ditched the wrapping macros
>> >>  - use __set_bit() and __clear_bit()
>> >
>> > Thank you for making the change. It indeed is easier to read than the
>> > original since one does not have to go and look up what wrappers are
>> > doing.
>>
>> Yeah, I initially disliked the SH_KEYSC_MAXKEYS that now are sprinkled
>> all over the place, but I have to admit that not using wrappers made
>> me realize that I should use __set_bit() and __clear_bit() instead of
>> the atomic ones. So all good.
>>
>
> BTW, that sh_keysc_map seems to be a holdover from the V1 of the patch
> and is not needed anymore. Does the patch below still work for you?

Thanks for updating the patch. It still works as expected. Please mark
the new patch as V3 if possible so people can
tell the difference between the patches.

Cheers,

/ magnus
--
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] 6+ messages in thread

* Re: [PATCH] input: bitmap update for sh_keysc V2
  2010-02-10 11:13       ` Magnus Damm
@ 2010-02-10 17:59         ` Dmitry Torokhov
  0 siblings, 0 replies; 6+ messages in thread
From: Dmitry Torokhov @ 2010-02-10 17:59 UTC (permalink / raw)
  To: Magnus Damm; +Cc: linux-input, lethal, linux-sh

On Wednesday 10 February 2010 03:13:38 am Magnus Damm wrote:
> On Wed, Feb 10, 2010 at 6:26 PM, Dmitry Torokhov
> 
> <dmitry.torokhov@gmail.com> wrote:
> > On Wed, Feb 10, 2010 at 01:13:37PM +0900, Magnus Damm wrote:
> >> On Tue, Feb 9, 2010 at 6:39 PM, Dmitry Torokhov
> >>
> >> <dmitry.torokhov@gmail.com> wrote:
> >> > On Tue, Feb 09, 2010 at 06:16:09PM +0900, Magnus Damm wrote:
> >> >> From: Magnus Damm <damm@opensource.se>
> >> >>
> >> >> Update the sh_keysc driver with proper bitmap support V2.
> >> >> This instead of using a fixed 32-bit integer to keep track
> >> >> for the key states. With this change in place the driver
> >> >> supports key pads with more than 32 keys.
> >> >>
> >> >> Signed-off-by: Magnus Damm <damm@opensource.se>
> >> >> ---
> >> >>
> >> >>  Changes since V1:
> >> >>  - ditched the wrapping macros
> >> >>  - use __set_bit() and __clear_bit()
> >> >
> >> > Thank you for making the change. It indeed is easier to read than the
> >> > original since one does not have to go and look up what wrappers are
> >> > doing.
> >>
> >> Yeah, I initially disliked the SH_KEYSC_MAXKEYS that now are sprinkled
> >> all over the place, but I have to admit that not using wrappers made
> >> me realize that I should use __set_bit() and __clear_bit() instead of
> >> the atomic ones. So all good.
> >
> > BTW, that sh_keysc_map seems to be a holdover from the V1 of the patch
> > and is not needed anymore. Does the patch below still work for you?
> 
> Thanks for updating the patch. It still works as expected.

Great, thank you for testing it.

> Please mark
> the new patch as V3 if possible so people can
> tell the difference between the patches.
> 

Umm, I usually remove all versioning from the changelogs  when I apply the 
patches to my tree.

-- 
Dmitry

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

end of thread, other threads:[~2010-02-10 17:59 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2010-02-09  9:16 [PATCH] input: bitmap update for sh_keysc V2 Magnus Damm
2010-02-09  9:39 ` Dmitry Torokhov
2010-02-10  4:13   ` Magnus Damm
2010-02-10  9:26     ` Dmitry Torokhov
2010-02-10 11:13       ` Magnus Damm
2010-02-10 17:59         ` Dmitry Torokhov

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).