[PATCH 0/5] Staging: panel: TODO fixes

[PATCH 0/5] Staging: panel: TODO fixes

[Ksenija Stanojevic]

This patchset is based on patch sent by Bijosh Thykkoottathil.
Here I tried to address all suggestions made by Dan and Willy.

Signed-off-by: Ksenija Stanojevic <ksenija.stanojevic@gmail.com>

Ksenija Stanojevic (5):
  Staging: panel: Use u8 type
  Staging: panel: Remove typedef pmask_t
  Staging: panel: Remove ULL
  Staging: panel: Reduce value range for *name
  Staging: panel: Make statement more readable

 drivers/staging/panel/panel.c | 46 +++++++++++++++++++++----------------------
 1 file changed, 22 insertions(+), 24 deletions(-)

-- 
1.9.1

[PATCH 1/5] Staging: panel: Use u8 type

[Ksenija Stanojevic]

Declare om, im, omask and imask as u8 to remove any confusion if
that describes the 8 bits of the data bus on the parallel port.
Also change return type of lcd_write_data() to u8.

Signed-off-by: Ksenija Stanojevic <ksenija.stanojevic@gmail.com>
---
 drivers/staging/panel/panel.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/staging/panel/panel.c b/drivers/staging/panel/panel.c
index 04d86f3..8bc604d 100644
--- a/drivers/staging/panel/panel.c
+++ b/drivers/staging/panel/panel.c
@@ -2042,11 +2042,11 @@ static void init_scan_timer(void)
  * corresponding to out and in bits respectively.
  * returns 1 if ok, 0 if error (in which case, nothing is written).
  */
-static int input_name2mask(const char *name, pmask_t *mask, pmask_t *value,
-			   char *imask, char *omask)
+static u8 input_name2mask(const char *name, pmask_t *mask, pmask_t *value,
+			  u8 *imask, u8 *omask)
 {
 	static char sigtab[10] = "EeSsPpAaBb";
-	char im, om;
+	u8 im, om;
 	pmask_t m, v;
 
 	om = 0ULL;
-- 
1.9.1

[PATCH 2/5] Staging: panel: Remove typedef pmask_t

[Ksenija Stanojevic]

Use __u64 instead of pmask_t and remove pmask_t since is useless.

Signed-off-by: Ksenija Stanojevic <ksenija.stanojevic@gmail.com>
---
 drivers/staging/panel/panel.c | 24 +++++++++++-------------
 1 file changed, 11 insertions(+), 13 deletions(-)

diff --git a/drivers/staging/panel/panel.c b/drivers/staging/panel/panel.c
index 8bc604d..7138ee7 100644
--- a/drivers/staging/panel/panel.c
+++ b/drivers/staging/panel/panel.c
@@ -172,8 +172,6 @@ static __u8 scan_mask_o;
 /* logical or of the input bits involved in the scan matrix */
 static __u8 scan_mask_i;
 
-typedef __u64 pmask_t;
-
 enum input_type {
 	INPUT_TYPE_STD,
 	INPUT_TYPE_KBD,
@@ -188,8 +186,8 @@ enum input_state {
 
 struct logical_input {
 	struct list_head list;
-	pmask_t mask;
-	pmask_t value;
+	__u64 mask;
+	__u64 value;
 	enum input_type type;
 	enum input_state state;
 	__u8 rise_time, fall_time;
@@ -219,19 +217,19 @@ static LIST_HEAD(logical_inputs);	/* list of all defined logical inputs */
  * corresponds to the ground.
  * Within each group, bits are stored in the same order as read on the port :
  * BAPSE (busy=4, ack=3, paper empty=2, select=1, error=0).
- * So, each __u64 (or pmask_t) is represented like this :
+ * So, each __u64 is represented like this :
  * 0000000000000000000BAPSEBAPSEBAPSEBAPSEBAPSEBAPSEBAPSEBAPSEBAPSE
  * <-----unused------><gnd><d07><d06><d05><d04><d03><d02><d01><d00>
  */
 
 /* what has just been read from the I/O ports */
-static pmask_t phys_read;
+static __u64 phys_read;
 /* previous phys_read */
-static pmask_t phys_read_prev;
+static __u64 phys_read_prev;
 /* stabilized phys_read (phys_read|phys_read_prev) */
-static pmask_t phys_curr;
+static __u64 phys_curr;
 /* previous phys_curr */
-static pmask_t phys_prev;
+static __u64 phys_prev;
 /* 0 means that at least one logical signal needs be computed */
 static char inputs_stable;
 
@@ -1789,7 +1787,7 @@ static void phys_scan_contacts(void)
 	gndmask = PNL_PINPUT(r_str(pprt)) & scan_mask_i;
 
 	/* grounded inputs are signals 40-44 */
-	phys_read |= (pmask_t)gndmask << 40;
+	phys_read |= (__u64)gndmask << 40;
 
 	if (bitmask != gndmask) {
 		/*
@@ -1805,7 +1803,7 @@ static void phys_scan_contacts(void)
 
 			w_dtr(pprt, oldval & ~bitval);	/* enable this output */
 			bitmask = PNL_PINPUT(r_str(pprt)) & ~gndmask;
-			phys_read |= (pmask_t)bitmask << (5 * bit);
+			phys_read |= (__u64)bitmask << (5 * bit);
 		}
 		w_dtr(pprt, oldval);	/* disable all outputs */
 	}
@@ -2042,12 +2040,12 @@ static void init_scan_timer(void)
  * corresponding to out and in bits respectively.
  * returns 1 if ok, 0 if error (in which case, nothing is written).
  */
-static u8 input_name2mask(const char *name, pmask_t *mask, pmask_t *value,
+static u8 input_name2mask(const char *name, __u64 *mask, __u64 *value,
 			  u8 *imask, u8 *omask)
 {
 	static char sigtab[10] = "EeSsPpAaBb";
 	u8 im, om;
-	pmask_t m, v;
+	__u64 m, v;
 
 	om = 0ULL;
 	im = 0ULL;
-- 
1.9.1

[PATCH 3/5] Staging: panel: Remove ULL

[Ksenija Stanojevic]

Remove ULL since it's useless.

Signed-off-by: Ksenija Stanojevic <ksenija.stanojevic@gmail.com>
---
 drivers/staging/panel/panel.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/staging/panel/panel.c b/drivers/staging/panel/panel.c
index 7138ee7..6dc3efb 100644
--- a/drivers/staging/panel/panel.c
+++ b/drivers/staging/panel/panel.c
@@ -2047,10 +2047,10 @@ static u8 input_name2mask(const char *name, __u64 *mask, __u64 *value,
 	u8 im, om;
 	__u64 m, v;
 
-	om = 0ULL;
-	im = 0ULL;
-	m = 0ULL;
-	v = 0ULL;
+	om = 0;
+	im = 0;
+	m = 0;
+	v = 0;
 	while (*name) {
 		int in, out, bit, neg;
 
@@ -2076,9 +2076,9 @@ static u8 input_name2mask(const char *name, __u64 *mask, __u64 *value,
 
 		bit = (out * 5) + in;
 
-		m |= 1ULL << bit;
+		m |= 1 << bit;
 		if (!neg)
-			v |= 1ULL << bit;
+			v |= 1 << bit;
 		name++;
 	}
 	*mask = m;
-- 
1.9.1

Re: [PATCH 3/5] Staging: panel: Remove ULL

[Ilia Mirkin]

On Tue, Dec 29, 2015 at 3:08 PM, Ksenija Stanojevic
<ksenija.stanojevic@gmail.com> wrote:
> Remove ULL since it's useless.
>
> Signed-off-by: Ksenija Stanojevic <ksenija.stanojevic@gmail.com>
> ---
>  drivers/staging/panel/panel.c | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/staging/panel/panel.c b/drivers/staging/panel/panel.c
> index 7138ee7..6dc3efb 100644
> --- a/drivers/staging/panel/panel.c
> +++ b/drivers/staging/panel/panel.c
> @@ -2047,10 +2047,10 @@ static u8 input_name2mask(const char *name, __u64 *mask, __u64 *value,
>         u8 im, om;
>         __u64 m, v;
>
> -       om = 0ULL;
> -       im = 0ULL;
> -       m = 0ULL;
> -       v = 0ULL;
> +       om = 0;
> +       im = 0;
> +       m = 0;
> +       v = 0;
>         while (*name) {
>                 int in, out, bit, neg;
>
> @@ -2076,9 +2076,9 @@ static u8 input_name2mask(const char *name, __u64 *mask, __u64 *value,
>
>                 bit = (out * 5) + in;
>
> -               m |= 1ULL << bit;
> +               m |= 1 << bit;

m and v are 64-bit, if bit can be >= 32, then you definitely do need
the ULL here...

>                 if (!neg)
> -                       v |= 1ULL << bit;
> +                       v |= 1 << bit;
>                 name++;
>         }
>         *mask = m;
> --
> 1.9.1
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 3/5] Staging: panel: Remove ULL

[Willy Tarreau]

On Tue, Dec 29, 2015 at 03:59:26PM -0500, Ilia Mirkin wrote:
> On Tue, Dec 29, 2015 at 3:08 PM, Ksenija Stanojevic
> <ksenija.stanojevic@gmail.com> wrote:
> > Remove ULL since it's useless.
> >
> > Signed-off-by: Ksenija Stanojevic <ksenija.stanojevic@gmail.com>
> > ---
> >  drivers/staging/panel/panel.c | 12 ++++++------
> >  1 file changed, 6 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/staging/panel/panel.c b/drivers/staging/panel/panel.c
> > index 7138ee7..6dc3efb 100644
> > --- a/drivers/staging/panel/panel.c
> > +++ b/drivers/staging/panel/panel.c
> > @@ -2047,10 +2047,10 @@ static u8 input_name2mask(const char *name, __u64 *mask, __u64 *value,
> >         u8 im, om;
> >         __u64 m, v;
> >
> > -       om = 0ULL;
> > -       im = 0ULL;
> > -       m = 0ULL;
> > -       v = 0ULL;
> > +       om = 0;
> > +       im = 0;
> > +       m = 0;
> > +       v = 0;
> >         while (*name) {
> >                 int in, out, bit, neg;
> >
> > @@ -2076,9 +2076,9 @@ static u8 input_name2mask(const char *name, __u64 *mask, __u64 *value,
> >
> >                 bit = (out * 5) + in;
> >
> > -               m |= 1ULL << bit;
> > +               m |= 1 << bit;
> 
> m and v are 64-bit, if bit can be >= 32, then you definitely do need
> the ULL here...

If that's it, indeed. I thought they were 8-bit from a previous patch.
I'm sorry for having suggested this cleanup, I've not put my nose in
this code for quite a while I must confess :-/

Regards,
Willy

[PATCH 4/5] Staging: panel: Reduce value range for *name

[Ksenija Stanojevic]

out is 0-9 so it's too much for om, therefore reduce value range for
*name from '0'-'9' to '0'-'7'.

Signed-off-by: Ksenija Stanojevic <ksenija.stanojevic@gmail.com>
---
 drivers/staging/panel/panel.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/staging/panel/panel.c b/drivers/staging/panel/panel.c
index 6dc3efb..70cb9f3 100644
--- a/drivers/staging/panel/panel.c
+++ b/drivers/staging/panel/panel.c
@@ -2065,7 +2065,7 @@ static u8 input_name2mask(const char *name, __u64 *mask, __u64 *value,
 		im |= BIT(in);
 
 		name++;
-		if (isdigit(*name)) {
+		if (*name >= '0' && *name <= '7') {
 			out = *name - '0';
 			om |= BIT(out);
 		} else if (*name == '-') {
-- 
1.9.1

[PATCH 5/5] Staging: panel: Make statement more readable

[Ksenija Stanojevic]

Broke statement into 3 different lines to make it more readable.

Signeded-off-by: Ksenija Stanojevic <ksenija.stanojevic@gmail.com>
---
 drivers/staging/panel/panel.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/staging/panel/panel.c b/drivers/staging/panel/panel.c
index 70cb9f3..207d8d5 100644
--- a/drivers/staging/panel/panel.c
+++ b/drivers/staging/panel/panel.c
@@ -2054,8 +2054,8 @@ static u8 input_name2mask(const char *name, __u64 *mask, __u64 *value,
 	while (*name) {
 		int in, out, bit, neg;
 
-		for (in = 0; (in < sizeof(sigtab)) && (sigtab[in] != *name);
-		     in++)
+		for (in = 0;
+		    (in < sizeof(sigtab)) && (sigtab[in] != *name); in++)
 			;
 
 		if (in >= sizeof(sigtab))
-- 
1.9.1

Re: [PATCH 5/5] Staging: panel: Make statement more readable

[Willy Tarreau]

Hi Ksenija,

several points, see below.

On Tue, Dec 29, 2015 at 09:11:03PM +0100, Ksenija Stanojevic wrote:
> Broke statement into 3 different lines to make it more readable.
> 
> Signeded-off-by: Ksenija Stanojevic <ksenija.stanojevic@gmail.com>
        ^^
Extra "ed". Don't waste your time typing it by hand, several git commands
(including git-commit and git-format-patch) will do it for you when you
pass "-s".

> diff --git a/drivers/staging/panel/panel.c b/drivers/staging/panel/panel.c
> index 70cb9f3..207d8d5 100644
> --- a/drivers/staging/panel/panel.c
> +++ b/drivers/staging/panel/panel.c
> @@ -2054,8 +2054,8 @@ static u8 input_name2mask(const char *name, __u64 *mask, __u64 *value,
>  	while (*name) {
>  		int in, out, bit, neg;
>  
> -		for (in = 0; (in < sizeof(sigtab)) && (sigtab[in] != *name);
> -		     in++)
> +		for (in = 0;
> +		    (in < sizeof(sigtab)) && (sigtab[in] != *name); in++)
>  			;
>  
>  		if (in >= sizeof(sigtab))

well, this one is still ugly in my opinion. I've just looked at the code
and it was crap originally :
  - sigtab[] is declared as static and without a trailing zero
  - the for loop uses extra parenthesis and basically only reimplements
    strchr()
  - the end condition is tested again after the for loop

=> I'd rather use strchr(), and clean up this part, approx like this, but
   do it as you want :

-       static char sigtab[10] = "EeSsPpAaBb";
+       const char sigtab[] = "EeSsPpAaBb";
...
 	while (*name) {
 		int in, out, bit, neg;
+		const char *idx;
 
-		for (in = 0; (in < sizeof(sigtab)) && (sigtab[in] != *name);
-		     in++)
-
-		if (in >= sizeof(sigtab))
-			return 0;
+		idx = strchr(sigtab, *name);
+		if (!idx)
+			return 0;
+
+		in = idx - sigtab;
 
I think it's more readable this way.

Regards,
Willy