Re: On Lindent shortcomings and massive style fixing
On Tue, Dec 29, 2015 at 9:32 AM, Mauro Carvalho Chehab wrote: > IMHO, there are two problems by letting indent breaking long > lines: > > 1) indent would break strings on printks. This is something that we don't > want to break strings on multiple lines in the Kernel; Yeah, GNU indent does its work badly (although I believe it could be taught to respect long literals), this makes me want to have a better tool for clean "relayouting" C code. I believe that'd require at last a proper syntax parser. With such tool, it would be straightforward to rewrite source code automatically to please any demanding reviewer of code style, except for issues of higher level of thought (like naming or nesting limitations). > 2) It doesn't actually solve the problem of having too complex loops, > with is why the 80 columns warning is meant to warn. Worse than that, > if a piece of code is inside more than 4 or 5 indentation levels, the > resulting code of using indent for 80-cols line break is a total crap. Then I'd propose to enforce nesting limitation explicitly, because Documentation/CodingStyle appreciates low nesting, just doesn't give exact numbers. It's better this way, because the programmer could pay attention to N places of excessive nesting and fix it manually, and then carelessly reformat NNN places of "80 chars" issues automatically, comparing to reviewing all NNN places, to figure out if there's excessive nesting, or not. (CCed checkpatch.pl maintainers.) > That's said, on a quick look at the driver, it seems that the 80-cols > violations are mostly (if not all) on the comments, like: > > int i_poc_lsb = (frame_seqno_in_gop << 1); /* why multiplied by two? > TODO try without multiplication */ > > and > > #define TW5864_UNDEF_REG_0x0224 0x0224 /* Undeclared in spec (or not yet > added to tw5864-reg.h) but used */ > #define TW5864_UNDEF_REG_0x4014 0x4014 /* Undeclared in spec (or not yet > added to tw5864-reg.h) but used */ > #define TW5864_UNDEF_REG_0xA800 0xA800 /* Undeclared in spec (or not yet > added to tw5864-reg.h) but used */ > > Btw, the content of tw5864-reg-undefined.h is weird... Why not just > add the stuff there at tw5864-reg.h and remove the comments for all > defines there? tw5864-reg-undefined.h will be edited for sure (maybe dropped), of course it won't stay as it is now. It was generated by script during reverse-engineering that bastard chip from hell. > Also, Lindent already did some crappy 80-cols like breaks, like: > > static int pci_i2c_multi_read(struct tw5864_dev *dev, u8 devid, u8 devfn, u8 > *buf, >u32 count) > > (count is misaligned with the open parenthesis) I just added "static " after indenting. Actually, Documentation/CodingStyle says nothing about alignment of function declaration tail on open parenthesis. Also I'd like to mention that I hate such alignment, because it requires intermixing of tabs and spaces. I am not aware if this is K&R thing or not. If it is, then please don't kill me. Thanks for kind replies, gentlemen. ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: On Lindent shortcomings and massive style fixing
On Tue, 29 Dec 2015, Andrey Utkin wrote: > On Tue, Dec 29, 2015 at 9:32 AM, Mauro Carvalho Chehab > wrote: > > IMHO, there are two problems by letting indent breaking long > > lines: > > > > 1) indent would break strings on printks. This is something that we don't > > want to break strings on multiple lines in the Kernel; > > Yeah, GNU indent does its work badly (although I believe it could be > taught to respect long literals), this makes me want to have a better > tool for clean "relayouting" C code. > I believe that'd require at last a proper syntax parser. With such > tool, it would be straightforward to rewrite source code automatically > to please any demanding reviewer of code style, except for issues of > higher level of thought (like naming or nesting limitations). > > > 2) It doesn't actually solve the problem of having too complex loops, > > with is why the 80 columns warning is meant to warn. Worse than that, > > if a piece of code is inside more than 4 or 5 indentation levels, the > > resulting code of using indent for 80-cols line break is a total crap. > > Then I'd propose to enforce nesting limitation explicitly, because > Documentation/CodingStyle appreciates low nesting, just doesn't give > exact numbers. > It's better this way, because the programmer could pay attention to N > places of excessive nesting and fix it manually, and then carelessly > reformat NNN places of "80 chars" issues automatically, comparing to > reviewing all NNN places, to figure out if there's excessive nesting, > or not. > (CCed checkpatch.pl maintainers.) Personally, I prefer to see only 80 characters per line, as long as it doesn't require contorting the code in some other way. So perhaps not everyone would agree that the issue is only the amount of nesting. julia > > That's said, on a quick look at the driver, it seems that the 80-cols > > violations are mostly (if not all) on the comments, like: > > > > int i_poc_lsb = (frame_seqno_in_gop << 1); /* why multiplied by > > two? TODO try without multiplication */ > > > > and > > > > #define TW5864_UNDEF_REG_0x0224 0x0224 /* Undeclared in spec (or not yet > > added to tw5864-reg.h) but used */ > > #define TW5864_UNDEF_REG_0x4014 0x4014 /* Undeclared in spec (or not yet > > added to tw5864-reg.h) but used */ > > #define TW5864_UNDEF_REG_0xA800 0xA800 /* Undeclared in spec (or not yet > > added to tw5864-reg.h) but used */ > > > > Btw, the content of tw5864-reg-undefined.h is weird... Why not just > > add the stuff there at tw5864-reg.h and remove the comments for all > > defines there? > > tw5864-reg-undefined.h will be edited for sure (maybe dropped), of > course it won't stay as it is now. > It was generated by script during reverse-engineering that bastard > chip from hell. > > > Also, Lindent already did some crappy 80-cols like breaks, like: > > > > static int pci_i2c_multi_read(struct tw5864_dev *dev, u8 devid, u8 devfn, > > u8 *buf, > >u32 count) > > > > (count is misaligned with the open parenthesis) > > I just added "static " after indenting. > Actually, Documentation/CodingStyle says nothing about alignment of > function declaration tail on open parenthesis. > Also I'd like to mention that I hate such alignment, because it > requires intermixing of tabs and spaces. I am not aware if this is K&R > thing or not. If it is, then please don't kill me. > > Thanks for kind replies, gentlemen. > -- > To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in > the body of a message to majord...@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH 0/5] 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. Signed-off-by: Ksenija Stanojevic 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 ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[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.
Signed-off-by: Ksenija Stanojevic
---
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
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH 2/5] Staging: panel: Remove typedef pmask_t
Use __u64 instead of pmask_t and remove pmask_t since is useless.
Signed-off-by: Ksenija Stanojevic
---
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 :
* 000BAPSEBAPSEBAPSEBAPSEBAPSEBAPSEBAPSEBAPSEBAPSE
* <-unused-->
*/
/* 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
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH 3/5] Staging: panel: Remove ULL
Remove ULL since it's useless.
Signed-off-by: Ksenija Stanojevic
---
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
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH 4/5] Staging: panel: Reduce value range for *name
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
---
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
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH 5/5] Staging: panel: Make statement more readable
Broke statement into 3 different lines to make it more readable.
Signeded-off-by: Ksenija Stanojevic
---
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
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 3/5] Staging: panel: Remove ULL
On Tue, Dec 29, 2015 at 3:08 PM, Ksenija Stanojevic
wrote:
> Remove ULL since it's useless.
>
> Signed-off-by: Ksenija Stanojevic
> ---
> 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 majord...@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 3/5] Staging: panel: Remove ULL
On Tue, Dec 29, 2015 at 03:59:26PM -0500, Ilia Mirkin wrote:
> On Tue, Dec 29, 2015 at 3:08 PM, Ksenija Stanojevic
> wrote:
> > Remove ULL since it's useless.
> >
> > Signed-off-by: Ksenija Stanojevic
> > ---
> > 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
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 1/5] staging: media: lirc: replace NULL comparisons with !var
On Fri, Dec 18, 2015 at 06:35:25PM +0530, Sudip Mukherjee wrote: > A NULL comparison can be written as if (var) or if (!var). > Reported by checkpatch. > > Signed-off-by: Sudip Mukherjee > --- Hi Mauro, A gentle ping. Can this series be considered for 4.5? regards sudip ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 1/6] Staging: gdm72xx: Add space around that "+", "&" and "|"
On Sun, Dec 27, 2015 at 02:44:40AM +0530, shraddha.6...@gmail.com wrote: > From: Shraddha Barke Why are you adding this extra From: line here? You can fix your gitconfig file so that the From: header of the email will contain this information. regards sudip ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 2/6] Staging: gdm72xx: Add space around that " "
On Sun, Dec 27, 2015 at 02:44:41AM +0530, shraddha.6...@gmail.com wrote:
> From: Shraddha Barke
>
> Add missing space around that " " to follow linux kernel coding
> style. Warning detected by checkpatch
>
> Signed-off-by: Shraddha Barke
> ---
This will not apply due to:
2bc29a1abc5c ("staging: gdm72xx: Replace timeval with ktime_t")
regards
sudip
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 3/6] Staging: gdm72xx: Add appropriate comment for spinlock_t definition
On Sun, Dec 27, 2015 at 02:44:42AM +0530, shraddha.6...@gmail.com wrote:
> From: Shraddha Barke
>
> Fix checkpatch issue: "CHECK: spinlock_t definition without comment".
>
> Signed-off-by: Shraddha Barke
> ---
This will also not apply because of:
2bc29a1abc5c ("staging: gdm72xx: Replace timeval with ktime_t")
regards
sudip
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] mm: fix noisy sparse warning in LIBCFS_ALLOC_PRE()
On Sat, Dec 26, 2015 at 09:12:42PM -0800, Joshua Clayton wrote: > running sparse on drivers/staging/lustre results in dozens of warnings: > include/linux/gfp.h:281:41: warning: > odd constant _Bool cast (40 becomes 1) > > Use "!!" to explicitly convert the result to bool range. > --- Signed-off-by missing. regards sudip ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] mm: fix noisy sparse warning in LIBCFS_ALLOC_PRE()
On Wednesday, December 30, 2015 10:17:50 AM Sudip Mukherjee wrote: > On Sat, Dec 26, 2015 at 09:12:42PM -0800, Joshua Clayton wrote: > > running sparse on drivers/staging/lustre results in dozens of warnings: > > include/linux/gfp.h:281:41: warning: > > odd constant _Bool cast (40 becomes 1) > > > > Use "!!" to explicitly convert the result to bool range. > > --- > > Signed-off-by missing. > > regards > sudip Hmm. I must have forgotten the "-s" in git send-email I'm thinking a patch this size wouldn't qualify for copyright even if an Oracle Lawyer claimed it. Nevertheless, all is well. The SOB made it into v2. ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 1/2] Staging: panel: Fix line over 80 characters
On Sun, Dec 27, 2015 at 12:16:15PM +0100, Ksenija Stanojevic wrote:
> Split comment in order to fit into 80 characters per line.
> Found by checkpatch.pl
>
> Signed-off-by: Ksenija Stanojevic
> ---
Instead of multiline maybe this is better:
diff --git a/drivers/staging/panel/panel.c b/drivers/staging/panel/panel.c
index 79ac192..3525919 100644
--- a/drivers/staging/panel/panel.c
+++ b/drivers/staging/panel/panel.c
@@ -947,7 +947,8 @@ static void lcd_clear_fast_s(void)
lcd_send_serial(0x5F); /* R/W=W, RS=1 */
lcd_send_serial(' ' & 0x0F);
lcd_send_serial((' ' >> 4) & 0x0F);
- usleep_range(40, 100); /* the shortest data takes at least 40
us */
+ /* the shortest data takes at least 40 us */
+ usleep_range(40, 100);
}
spin_unlock_irq(&pprt_lock);
regards
sudip
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
