Re: [patch 01/37] V4L/DVB (7473): PATCH for various Dibcom based devices

["Michael Krufky" <mkrufky@linuxtv.org>]

On Tue, May 13, 2008 at 10:03 PM, Greg KH <gregkh@suse.de> wrote:
> On Tue, May 13, 2008 at 09:27:30PM -0400, Michael Krufky wrote:
>  > On Tue, May 13, 2008 at 4:11 PM, Greg KH <gregkh@suse.de> wrote:
>  > > 2.6.25-stable review patch.  If anyone has any objections, please let us know.
>  > >
>  > >  ------------------
>  > >
>  > >  From: Albert Comerma <albert.comerma@gmail.com>
>  > >
>  > >  patch 6ca8f0b97473dcef3a754bab5239dcfcdd00b244 upstream
>  > >
>  > >  This patch introduces support for dvb-t for the following DiBcom based cards:
>  > >
>  > >  - Terratec Cinergy HT USB XE (USB-ID: 0ccd:0058)
>  > >  - Terratec Cinergy HT Express (USB-ID: 0ccd:0060)
>  > >  - Pinnacle 320CX (USB-ID: 2304:022e)
>  > >  - Pinnacle PCTV72e (USB-ID: 2304:0236)
>  > >  - Pinnacle PCTV73e (USB-ID: 2304:0237)
>  > >  - Yuan EC372S (USB-ID: 1164:1edc)
>  > >
>  > >  Signed-off-by: Hans-Frieder Vogt <hfvogt@gmx.net>
>  > >  Signed-off-by: Felix Apitzsch <F.Apitzsch@soz.uni-frankfurt.de>
>  > >  Signed-off-by: Antti Palosaari <crope@iki.fi>
>  > >  Signed-off-by: Albert Comerma <albert.comerma@gmail.com>
>  > >  Signed-off-by: Patrick Boettcher <pb@linuxtv.org>
>  > >  Signed-off-by: Mauro Carvalho Chehab <mchehab@infradead.org>
>  > >  Cc: Michel Morisot <mmorisot.abonnement@belcenter.com>
>  > >  Signed-off-by: Greg Kroah-Hartman <gregkh@suse.de>
>  > >
>  > >  ---
>  > >   drivers/media/dvb/dvb-usb/dib0700_devices.c |  259 ++++++++++++++++++++++++----
>  > >   drivers/media/dvb/dvb-usb/dvb-usb-ids.h     |    9
>  > >   2 files changed, 238 insertions(+), 30 deletions(-)
>  >
>  >
>  > This patch is entirely inappropriate for -stable
>
>  Why do you say that?  It adds new device ids for devices to a driver,
>  which is find for stable.

It is much larger than a 100 line patch.  I know that this is not a
_strict_ requirement, but upon closer inspection, I see that the
majority of this is codingstyle cleanup.  The codingstyle cleanups
should have been removed from this patch before backporting to
-stable, in my opinion.

>  > I usually send in the v4l-dvb backports for -stable, and this was
>  > never in my queue, not to mention that it doesn't qualify, based on
>  > the -stable rules.
>
>  I think it does qualify and I've been asking for feedback about this for
>  the past week to everyone on the signed-off-lines above with no real
>  objections :)
>
>  openSUSE had a user that requested this patch be added as they had
>  hardware that needed this patch to work properly on 2.6.25.  As it only
>  added device ids, I didn't see the problem with it.
>
>  Does it cause any problems that you can see?

The patch is fine, and there is nothing wrong with it.

The only problem is that all of the cosmetic cleanups are unnecessary,
and they have caused the patch to appear like a larger change than it
actually is.

I thought it was important for patches going in to -stable to be
"obviously correct".  All of the cleanups remove the "obvious
correctness" from this patch, and introduce the chance of a typo
somewhere.

This is all the result of checkpatch.pl -- now it is run before any
commit occurs to the v4l-dvb mercurial repository, causing people to
merge codingstyle cleanups into actual changesets.  This makes review
more difficult.

I always believe that separate changes should appear in separate
patches.  Had this patch been the simple ID addition & config struct
additions, this discussion would never have occurred.

I withdraw my complaint, but I recommend that the "codingstyle
cleanup" hunks from the patch should be dropped.

Of the 30 deletions, only two of them are valid:

-		.num_device_descs = 1,
+		.num_device_descs = 2,

and

-		.num_device_descs = 6,
+		.num_device_descs = 8,

Regards,

Mike