On Mon, 2012-05-07 at 10:20 +0200, Manohar Vanga wrote:
> Hey Samuel,
>
> Just a couple of quick comments on this patch :)
>
> > +TPCI-200
> > +--------
> > +
> > +* It receives the name of the mezzanine plugged in each slot by
SYSFS.
> > + No autodetection supported yet, because the mezzanine driver
could not be
> > + loaded at the time that the tpci200 driver loads.
> > +
> > +* It has a linked list with the tpci200 devices it is managing. Get
rid of it
> > + and use driver_for_each_device() instead.
> > +
> > Ipack
> > -----
> >
> > @@ -20,4 +30,3 @@ Ipack
> > remove_device() to notify the carrier driver, or the opposite
with the call to
> > the ipack_driver_ops' remove() function could be improved.
> >
> > -
>
> Is this whitespace change required?
>
>
It is not. I will rearrange the previous patch to avoid this
uncomfortable change.
> +#include <linux/module.h>
> > +#include "tpci200.h"
> > +
> > +#define MODULE_NAME "tpci200"
>
> Here you can just use the KBUILD_MODNAME variable
>
> > +#define PFX MODULE_NAME ": "
>
> You can also add this before all your includes:
> #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
>
>
Thanks a lot. I didn't know about it.
> +static int tpci200_install(struct tpci200_board *tpci200)
> > +{
> > + int res = 0;
> > +
> > + tpci200->slots = kzalloc(TPCI200_NB_SLOT * sizeof(struct
tpci200_slot), GFP_KERNEL);
>
> Did you run checkpatch.pl on the patches? Are you ignoring the >80
char
> recommendation? In that case ignore this :)
>
Yes, I ran it. I ignore the warning to facilitate the reading of the
sentences. However, I will re-check all the warnings to minimize the
number of them.
> > +static struct pci_device_id tpci200_idtable[2]; /* last must be
zero */
> > +
> > +static struct pci_driver tpci200_pci_drv = {
> > + .name = "tpci200",
> > + .id_table = tpci200_idtable,
> > + .probe = tpci200_pciprobe,
> > + .remove = __devexit_p(tpci200_pci_remove),
> > +};
> > +
> > +static int __init tpci200_drvr_init_module(void)
> > +{
> > + tpci200_idtable[0].vendor = TPCI200_VENDOR_ID;
> > + tpci200_idtable[0].device = TPCI200_DEVICE_ID;
> > + tpci200_idtable[0].subvendor = TPCI200_SUBVENDOR_ID;
> > + tpci200_idtable[0].subdevice = TPCI200_SUBDEVICE_ID;
> > + return pci_register_driver(&tpci200_pci_drv);
> > +}
>
> Can't tpci200_idtable be statically declared instead of inside the
init function?
>
>
Yes, it can. I will fix it.
> +static void __exit tpci200_drvr_exit_module(void)
> > +{
> > + struct tpci200_board *tpci200;
> > + struct list_head *element, *next;
> > +
> > + list_for_each_safe(element, next, &tpci200_list) {
> > + tpci200 = list_entry(element, struct tpci200_board, list);
> > + __tpci200_pci_remove(tpci200);
> > + }
>
> You can use list_for_each_entry_safe instead of list_for_each_safe +
list_entry.
> I think you've used this everywhere so this would apply to the whole
patch :)
>
>
Thanks for the tips,
Sam
_______________________________________________
devel mailing list
devel@xxxxxxxxxxxxxxxxxxxxxx
http://driverdev.linuxdriverproject.org/mailman/listinfo/devel
