Em Sat, 19 Nov 2016 19:18:39 -0200
Mauro Carvalho Chehab <mchehab@xxxxxxxxxxxxxxxx> escreveu:
> Em Sat, 19 Nov 2016 19:58:50 +0000
> Andrey Utkin <andrey_utkin@xxxxxxxxxxxx> escreveu:
>
> > This is from checkpatch run on cx88 source files with "-f", not your
> > patch files, right? I guess it might produce less changes if run on
> > patches.
>
> Yes, I know.
>
> > On Sat, Nov 19, 2016 at 10:14:05AM -0200, Mauro Carvalho Chehab wrote:
> > > Usually, I don't like fixing coding style issues on non-staging
> > > drivers, as it could be a mess pretty easy, and could become like
> > > a snow ball. That's the case of recent changes on two changesets:
> > > they disalign some statements.
> >
> > In my understanding, commits dedicated to style fixes on non-staging are
> > discouraged because they clutter git log and "git blame" view. But new
> > commits are encouraged to be style-perfect.
>
> Yes, that's the usual policy. The main reason is not due to git log:
> when you do lots of changes on a file, the maintainer's live become
> very hard, as he needs to fix conflicts of patches affecting the file,
> or ask everybody to rebase their patches.
>
> However, when we do large reformats for some other reason, though,
> it sometimes makes sense to take the opportunity and fix the style
> on the file, as bisect, blame and patch handling will be affected
> anyway.
>
> >
> > And in case of discussed alignment breakage, I expected that you make
> > this your fixup (the current patch) really a git-ish fixup and just
> > merge it into 09/35 patch.
>
> Too late for that, as your review came after applying the patch
> reached media upstream. I don't rebase the main media development
> tree (except when something really bad happened at the tree, and
> if I notice a few minutes after pushing something).
>
> > As I see it's published in media tree master
> > already and you are not going to force-push there; maybe a bit of
> > latency in pushing patches to media tree after publishing them for
> > review would prevent this sort of inconvenience.
>
> I usually wait for some time before applying upstream. The time
> I wait is based on my own good sense if either people will comment
> the patch or silently ignore.
>
> >
> > P. S. (Running away from rotten tomatoes) another way to avoid such
> > painful alignment issues is to legalize "one-more-tab" indentation for
> > trailing parts of lines, alignment on opening brace is brittle IMHO.
> >
> >
> > > --- a/drivers/media/pci/cx88/cx88-blackbird.c
> > > +++ b/drivers/media/pci/cx88/cx88-blackbird.c
> >
> > > @@ -1061,7 +1092,8 @@ static int cx8802_blackbird_advise_acquire(struct cx8802_driver *drv)
> > >
> > > switch (core->boardnr) {
> > > case CX88_BOARD_HAUPPAUGE_HVR1300:
> > > - /* By default, core setup will leave the cx22702 out of reset, on the bus.
> > > + /* By default, core setup will leave the cx22702 out of reset,
> > > + * on the bus.
> > > * We left the hardware on power up with the cx22702 active.
> > > * We're being given access to re-arrange the GPIOs.
> > > * Take the bus off the cx22702 and put the cx23416 on it.
> >
> > Let first line with "/*" be empty :)
> >
> > > --- a/drivers/media/pci/cx88/cx88-core.c
> > > +++ b/drivers/media/pci/cx88/cx88-core.c
> >
> > > @@ -102,28 +104,29 @@ static __le32 *cx88_risc_field(__le32 *rp, struct scatterlist *sglist,
> > > sol = RISC_SOL | RISC_IRQ1 | RISC_CNT_INC;
> > > else
> > > sol = RISC_SOL;
> > > - if (bpl <= sg_dma_len(sg)-offset) {
> > > + if (bpl <= sg_dma_len(sg) - offset) {
> > > /* fits into current chunk */
> > > - *(rp++) = cpu_to_le32(RISC_WRITE|sol|RISC_EOL|bpl);
> > > - *(rp++) = cpu_to_le32(sg_dma_address(sg)+offset);
> > > + *(rp++) = cpu_to_le32(RISC_WRITE | sol |
> > > + RISC_EOL | bpl);
> > > + *(rp++) = cpu_to_le32(sg_dma_address(sg) + offset);
> > > offset += bpl;
> > > } else {
> > > /* scanline needs to be split */
> > > todo = bpl;
> > > - *(rp++) = cpu_to_le32(RISC_WRITE|sol|
> > > - (sg_dma_len(sg)-offset));
> > > - *(rp++) = cpu_to_le32(sg_dma_address(sg)+offset);
> > > - todo -= (sg_dma_len(sg)-offset);
> > > + *(rp++) = cpu_to_le32(RISC_WRITE | sol |
> > > + (sg_dma_len(sg) - offset));
> >
> > This is strange, but checkpatch --strict is really happy on this,
> > however there is a misalignment in added lines. Going to look into this
> > later.
> >
> > > --- a/drivers/media/pci/cx88/cx88-input.c
> > > +++ b/drivers/media/pci/cx88/cx88-input.c
> > > @@ -62,11 +62,15 @@ static int ir_debug;
> > > module_param(ir_debug, int, 0644); /* debug level [IR] */
> > > MODULE_PARM_DESC(ir_debug, "enable debug messages [IR]");
> > >
> > > -#define ir_dprintk(fmt, arg...) if (ir_debug) \
> > > - printk(KERN_DEBUG "%s IR: " fmt, ir->core->name, ##arg)
> > > +#define ir_dprintk(fmt, arg...) do { \
> >
> > Backslash stands out.
>
> Sorry, but I didn't understand what you're meaning here.
>
> ...
> > Everything else seems fine.
>
> The other comments are OK. I'm sending a version 2 of this patch.
> To make easier to review, I'm enclosing the diff against version 1
> here.
I actually added two more hunks, removing FSF address and converting
the initial comment to the new format.
Thanks,
Mauro
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
