Re: Advantech iManager2 driver for Linux Kernel

Re: Advantech iManager2 driver for Linux Kernel

[Lee Jones]

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