On Sat, Oct 15, 2011 at 10:54:43PM +0200, Piotr Chmura wrote:
> staging as102: cleanup - formatting code
>
> Cleanup code: change double spaces into single, put tabs instead of spaces
> where they should be.
>
> Signed-off-by: Piotr Chmura<[email protected]>
> Cc: Devin Heitmueller<[email protected]>
> Cc: Greg HK<[email protected]>
Just a few hints from my side. Most of my comments apply to multiple other parts
of the code, but I did not want to quote everything and you should be able to
find the other parts I did not mention explicitely as well.
I don't have much knowledge of kernel code style, but wanted to point out a few
things that seem to be obviously wrong or uncommon, and stuff I wouldn't do.
There
may be a few false positives and some things missing.
[And yes, I actually only wanted to comment on the two-space thing, but I
somehow
ended up reading the complete patch or the first half of it].
>
> diff -Nur linux.as102.03-typedefs/drivers/staging/as102/as102_drv.c
> linux.as102.04-tabs/drivers/staging/as102/as102_drv.c
> --- linux.as102.03-typedefs/drivers/staging/as102/as102_drv.c 2011-10-14
> 17:55:02.000000000 +0200
> +++ linux.as102.04-tabs/drivers/staging/as102/as102_drv.c 2011-10-14
> 23:20:05.000000000 +0200
> @@ -10,7 +10,7 @@
> *
> * This program is distributed in the hope that it will be useful,
> * but WITHOUT ANY WARRANTY; without even the implied warranty of
> - * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> * GNU General Public License for more details.
> *
> * You should have received a copy of the GNU General Public License
For reference, the standard variant uses two spaces after the period (aka
English Spacing).
> -failed:
> + failed:
> LEAVE();
> /* FIXME: free dvb_XXX */
> return ret;
It's more common to have no indentation before labels (about
7 times more common).
> @@ -332,7 +332,7 @@
>
> /**
> * \brief as102 driver exit point. This function is called when device has
> - * to be removed.
> + * to be removed.
> */
Does it make sense to reindent that? If you intent to keep API documentation
comments, you want to convert them to kernel-doc style anyway.
> dprintk(debug, "min_delay_ms = %d -> %d\n", settings->min_delay_ms,
> - 1000);
> + 1000);
> #endif
Seems to be more indentation than really required.
> @@ -201,7 +201,7 @@
> break;
> case TUNE_STATUS_STREAM_TUNED:
> *status = FE_HAS_SIGNAL | FE_HAS_CARRIER | FE_HAS_SYNC |
> - FE_HAS_LOCK;
> + FE_HAS_LOCK;
> break;
I think it looks better with indentation here. Otherwise you might not
read the | at the end of the previous line and think FE_HAS_LOCK is
a strange macro evaluating to a function call. Moving the operator
into the second line would also make this even more clear.
> #else
> - .release = as102_fe_release,
> - .init = as102_fe_init,
> + .release = as102_fe_release,
> + .init = as102_fe_init,
> #endif
> };
Is there a reason why struct members are indented using
two tabs (here and elsewhere)?
>
> @@ -393,7 +393,7 @@
> }
>
> int as102_dvb_register_fe(struct as102_dev_t *as102_dev,
> - struct dvb_frontend *dvb_fe)
> + struct dvb_frontend *dvb_fe)
> {
If there's still space to align further to the right, why not
use it? It makes the code look better if parameters are aligned
below each other (or at least nearly).
> int errno;
> struct dvb_adapter *dvb_adap;
> @@ -407,7 +407,7 @@
> /* init frontend callback ops */
> memcpy(&dvb_fe->ops,&as102_fe_ops, sizeof(struct dvb_frontend_ops));
> strncpy(dvb_fe->ops.info.name, as102_dev->name,
> - sizeof(dvb_fe->ops.info.name));
> + sizeof(dvb_fe->ops.info.name));
>
It does not seem helpful to align like this, it certainly looks
much uglier than the old one which had sizeof aligned with dvb.
> /* set frequency */
> @@ -642,32 +642,32 @@
> * if HP/LP are both set to FEC_NONE, HP will be selected.
> */
> if ((tune_args->hierarchy != HIER_NONE)&&
Misses a space before the &&
> dprintk(debug, "\thierarchy: 0x%02x "
> "selected: %s code_rate_%s: 0x%02x\n",
> - tune_args->hierarchy,
> - tune_args->hier_select == HIER_HIGH_PRIORITY ?
> - "HP" : "LP",
> - tune_args->hier_select == HIER_HIGH_PRIORITY ?
> - "HP" : "LP",
> - tune_args->code_rate);
> + tune_args->hierarchy,
> + tune_args->hier_select == HIER_HIGH_PRIORITY ?
> + "HP" : "LP",
> + tune_args->hier_select ==
> HIER_HIGH_PRIORITY ?
> + "HP" : "LP",
> +
> tune_args->code_rate);
You indented the second argument one level further than the
first one. And the third argument even more.
> errno = bus_adap->ops->upload_fw_pkt(bus_adap,
> - (uint8_t *)
> - &fw_pkt, 2, 0);
> + (uint8_t *)
> + &fw_pkt, 2, 0);
Splitting after the "&fw_pkt," seems more sensible, as you have the
cast and its operand on the same line then.
>
> static int as102_usb_start_stream(struct as102_dev_t *dev);
> static void as102_usb_stop_stream(struct as102_dev_t *dev);
> @@ -38,59 +38,59 @@
> static int as102_release(struct inode *inode, struct file *file);
>
> static struct usb_device_id as102_usb_id_table[] = {
> - { USB_DEVICE(AS102_USB_DEVICE_VENDOR_ID, AS102_USB_DEVICE_PID_0001) },
> - { USB_DEVICE(PCTV_74E_USB_VID, PCTV_74E_USB_PID) },
> - { USB_DEVICE(ELGATO_EYETV_DTT_USB_VID, ELGATO_EYETV_DTT_USB_PID) },
> - { USB_DEVICE(NBOX_DVBT_DONGLE_USB_VID, NBOX_DVBT_DONGLE_USB_PID) },
> - { } /* Terminating entry */
> + { USB_DEVICE(AS102_USB_DEVICE_VENDOR_ID,
> AS102_USB_DEVICE_PID_0001) },
> + { USB_DEVICE(PCTV_74E_USB_VID, PCTV_74E_USB_PID) },
> + { USB_DEVICE(ELGATO_EYETV_DTT_USB_VID,
> ELGATO_EYETV_DTT_USB_PID) },
> + { USB_DEVICE(NBOX_DVBT_DONGLE_USB_VID,
> NBOX_DVBT_DONGLE_USB_PID) },
> + { } /* Terminating entry */
> };
Again, two levels of indentation do not add anything valuable.
>
> /* Note that this table must always have the same number of entries as the
> - as102_usb_id_table struct */
> + as102_usb_id_table struct */
> static const char *as102_device_names[] = {
> - AS102_REFERENCE_DESIGN,
> - AS102_PCTV_74E,
> - AS102_ELGATO_EYETV_DTT_NAME,
> - AS102_NBOX_DVBT_DONGLE_NAME,
> - NULL /* Terminating entry */
> + AS102_REFERENCE_DESIGN,
> + AS102_PCTV_74E,
> + AS102_ELGATO_EYETV_DTT_NAME,
> + AS102_NBOX_DVBT_DONGLE_NAME,
> + NULL /* Terminating entry */
> };
Same here and at a lot of other locations.
[I stopped here]
--
Julian Andres Klode - Debian Developer, Ubuntu Member
See http://wiki.debian.org/JulianAndresKlode and http://jak-linux.org/.
_______________________________________________
devel mailing list
[email protected]
http://driverdev.linuxdriverproject.org/mailman/listinfo/devel
