Re: Advantech iManager2 driver for Linux Kernel

From: Lee Jones
Date: Fri Aug 30 2013 - 09:02:20 EST


CC'ing the list (you must always do that when submitting patches).

> > Thanks for the patchset. For Lee or/and myself to properly review (and
> > potentially merge it upstream) it, you have to split it into smaller
> > chunks, and generate the patches with git (e.g. git-format-patch).
> > The patches should be as independent as possible, e.g. you could split
> > that one into an MFD one, and i2c one and a hwmon one.
> >
> > Now, about the code itself, from a very quick first glance:
> >
> > - No cross-compilation stuff, this is going to be a Linux driver.
> > - Put the i2c and hwmon part away from drivers/mfd/, they're not MFD
> > drivers. They could have build time dependencies against the MFD
> > driver (for the I/O API for example).
> > - No one liners wrappers (imanager2_ec_lock for example).
> > - Try to use managed resources (devm_*) if possible.
> > - Where is the sio_inb() API coming from ?
> > - ARRAY_SIZE is your friend.
>
> Just to add a few things of my own:
>
> - Read: Documentation/[SubmittingPatches|email-clients.txt|CodingStyle]
> and pay particular attention to code formatting and comments.
> - The print messages are too many and the formatting is pretty
> ugly. You want to be striving for really simple, easy to ready code.
> There is a time and a place for obfuscated C, this isn't it. :) If
> it's impossible to, or makes sound sense not to make a particular
> piece of code simple then you need to add a suitable comment.
> - You use (!variable) and (variable == NULL), just standardise to the
> former.
> - Don't return -1, use proper Linux error codes [1].
> - The DRVNAME and DRV_NAME name stuff is pretty ugly.
> - Magic numbers are never acceptable, swap them out for #defines.
> - Don't call probe() from init(), set them up as platform devices.
> - You need to support Device Tree.
> - Hmmm... there doesn't appear to be any platform side code in this
> driver at all. I don't think that's right. Does this driver run
> on multiple platforms?
> - Using sizeof() to allocate the size of an array is ugly.
> - You've defined your own version of sprintf() and it's not even
> static! Why can't you use the one in the kernel? Same goes for
> udelay().
> - Variable names need to be descriptive, things like 'v' and 'f'
> won't do.
> - No need to comment the end of switch(), if(), for() etc.
> - You can clean up set_temp_min() et. al by setting 'count' to
> -EINVAL just once at the top instead of for all error conditions
> throughout the function.
>
> I think that's enough for now. :)
>
> Good luck!
>
> [1] http://www-numi.fnal.gov/offline_software/srt_public_context/WebDocs/Errors/unix_system_errors.html
>
> --
> Lee Jones
> Linaro STMicroelectronics Landing Team Lead
> Linaro.org â Open source software for ARM SoCs
> Follow Linaro: Facebook | Twitter | Blog

--
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org â Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/