Hi Dan,
Thanks for your review, I'm adding a few extra comments below.
On Mon, Dec 28, 2015 at 06:27:36PM +0300, Dan Carpenter wrote:
> On Mon, Dec 28, 2015 at 12:32:39PM +0100, Ksenija Stanojevi?? wrote:
> > Hi Willy,
> >
> > I'm helping Greg do a bit of cleanup in the staging tree, I noticed that
> > panel driver is maybe ready to be moved to drivers/misc. Are there any
> > TODO tasks left to do? I already sent checkpatch clean-up patches.
> >
>
> I feel like lcd_write_data() should take a u8 instead of an int. Or
> possibly a char, I suppose.
Looks like so indeed.
> Could we tighten the checking in input_name2mask() a bit?
>
> drivers/staging/panel/panel.c
> 2044 static int input_name2mask(const char *name, pmask_t *mask, pmask_t *value,
> 2045 char *imask, char *omask)
> 2046 {
> 2047 static char sigtab[10] = "EeSsPpAaBb";
> 2048 char im, om;
>
> Om is 8 bits (signed or not depending on the arch).
>
> 2049 pmask_t m, v;
I don't know what pmask_t is but I think it should be an opportunity to get
rid of it if it's a scalar.
> 2050
> 2051 om = 0ULL;
> 2052 im = 0ULL;
> 2053 m = 0ULL;
> 2054 v = 0ULL;
ULL is useless here BTW.
> 2055 while (*name) {
> 2056 int in, out, bit, neg;
> 2057
> 2058 for (in = 0; (in < sizeof(sigtab)) && (sigtab[in] != *name);
> 2059 in++)
> 2060 ;
The 80-chars limit makes this statement even more confusing than needed.
Possibly it should be broken into 3 different lines.
> 2061
> 2062 if (in >= sizeof(sigtab))
> 2063 return 0; /* input name not found */
> 2064 neg = (in & 1); /* odd (lower) names are negated */
> 2065 in >>= 1;
> 2066 im |= BIT(in);
> 2067
> 2068 name++;
> 2069 if (isdigit(*name)) {
> 2070 out = *name - '0';
> 2071 om |= BIT(out);
> ^^^^^^^^^^^^^^
> out is 0-9 so it's too much for "om". I don't know if this causes a
> problem, but let's remove the question by adding a check for illegal
> values.
> if (*name >= '0' && *name <= '7') {
It's very old memories for me now but looking at this code I guess these
are the digits corresponding to the data bits of the parallel port. Thus
indeed such a control is needed to remove any doubt.
> 2072 } else if (*name == '-') {
> 2073 out = 8;
> 2074 } else {
> 2075 return 0; /* unknown bit name */
> 2076 }
> 2077
> 2078 bit = (out * 5) + in;
> 2079
> 2080 m |= 1ULL << bit;
> 2081 if (!neg)
> 2082 v |= 1ULL << bit;
We can remove ULL here and there as well I guess.
> 2083 name++;
> 2084 }
> 2085 *mask = m;
> 2086 *value = v;
> 2087 if (imask)
> 2088 *imask |= im;
> 2089 if (imask)
> 2090 *imask |= im;
> 2091 if (omask)
> 2092 *omask |= om;
>
> It's too much for omask also.
Yes, let's have om and friends be declared u8 to remove any confusion if
that describes the 8 bits of the data bus on the parallel port. It will
be much saner.
Ksenija, are you interested in trying to address this ?
Thanks!
Willy
_______________________________________________
devel mailing list
devel@xxxxxxxxxxxxxxxxxxxxxx
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
