Staging: panel: TODO fixes
cover Staging: panel: TODO fixes
This patchset is based on patch sent by Bijosh Thykkoottathil. Here I tried to address all suggestions made by Dan and Willy.
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(-)
Signature · 1 line
1.9.1
PATCH 1/5 Staging: panel: Use u8 type
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.
--- drivers/staging/panel/panel.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)
drivers/staging/panel/panel.c +3 -3
@@ -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 ;
Signature · 1 line
1.9.1
PATCH 2/5 Staging: panel: Remove typedef pmask_t
In reply to Ksenija StanojevicUse __u64 instead of pmask_t and remove pmask_t since is useless.
--- drivers/staging/panel/panel.c | 24 +++++++++++------------- 1 file changed, 11 insertions(+), 13 deletions(-)
drivers/staging/panel/panel.c +11 -13
@@ -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 ;
Signature · 1 line
1.9.1
PATCH 3/5 Staging: panel: Remove ULL
In reply to Ksenija StanojevicRemove ULL since it's useless.
--- drivers/staging/panel/panel.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-)
drivers/staging/panel/panel.c +6 -6
@@ -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;
Signature · 1 line
1.9.1
Re: [PATCH 3/5] Staging: panel: Remove ULL
Quoted 27 lines · Ksenija Stanojevic
Remove ULL since it's useless.
Signed-off-by Ksenija Stanojevic <[email protected]>--- drivers/staging/panel/panel.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-)drivers/staging/panel/panel.c +4
-4@@ -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;
drivers/staging/panel/panel.c static u8 input_name2mask(const char *name, __u64 *mask, __u64 *value, · line 2079
Quoted 5 lines
drivers/staging/panel/panel.c +1
-1@@ -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...
Quoted 14 lines
drivers/staging/panel/panel.c +1
-2if (!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 [email protected]
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
Quoted 30 lines · Ilia Mirkin
Quoted 28 lines · Ksenija Stanojevic
Remove ULL since it's useless.
Signed-off-by Ksenija Stanojevic <[email protected]>--- drivers/staging/panel/panel.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-)drivers/staging/panel/panel.c +4
-4@@ -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,
Quoted 7 lines
drivers/staging/panel/panel.c static u8 input_name2mask(const char *name, __u64 *mask, __u64 *value, · line 2079
Quoted 4 lines
drivers/staging/panel/panel.c +1
-1bit = (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
In reply to Ksenija Stanojevicout is 0-9 so it's too much for om, therefore reduce value range for *name from '0'-'9' to '0'-'7'.
--- drivers/staging/panel/panel.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/staging/panel/panel.c +1 -1
@@ -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 =='-' ) {
Signature · 1 line
1.9.1
PATCH 5/5 Staging: panel: Make statement more readable
In reply to Ksenija StanojevicBroke statement into 3 different lines to make it more readable.
--- drivers/staging/panel/panel.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
drivers/staging/panel/panel.c +2 -2
@@ -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))
Signature · 1 line
1.9.1
Re: [PATCH 5/5] Staging: panel: Make statement more readable
Hi Ksenija,
several points, see below.
Quoted 3 lines · Ksenija Stanojevic
Broke statement into 3 different lines to make it more readable.
Signeded-off-by Ksenija Stanojevic <[email protected]>
^^
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".
Quoted 5 lines
drivers/staging/panel/panel.c +0
-0
drivers/staging/panel/panel.c static u8 input_name2mask(const char *name, __u64 *mask, __u64 *value, · line 2061
Quoted 10 lines
drivers/staging/panel/panel.c +2
-2@@ -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