Hi Rhyland,



Following the naming used for other options in this menu, this would be


Please remove, this doesn't add much value and is likely to get






This structure doesn't seem terribly useful. You typically need the
client to get to this information... And storing the device pointer as

-EINVAL doesn't look like the right error code. -ENODEV or -EIO would
seem more appropriate. Same issue several times, please address them






You should pass down the exact error value returned by the function,












-- 
Jean Delvare
--

[ message continues ]

Thanks for the review, fixed up what you suggested, I did leave one place as -EINVAL as it seemed the most appropriate for the spot. Will repost soon.

-----Original Message-----
From: Jean Delvare [mailto:khali@linux-fr.org]
Sent: Tuesday, August 31, 2010 1:24 AM
To: Rhyland Klein
Cc: cbouatmailru@gmail.com; linux-i2c@vger.kernel.org; linux-kernel@vger.kernel.org; olof@lixom.net; Andrew Chew
Subject: Re: [PATCH 1/1] power supply: add driver for TI BQ20Z75 gas gauge IC

Hi Rhyland,



Following the naming used for other options in this menu, this would be
BATTERY_BQ20Z75.


Not all capitals, please.


Please remove, this doesn't add much value and is likely to get
outdated at some point.


I don't think you need this one?


You don't use these anywhere?


This could be const.


You don't need the backslash at the end of the line.


Please be consistent with case in hexadecimal numbers.


This structure doesn't seem terribly useful. You typically need the
client to get to this information... And storing the device pointer as
a global is evil anyway, please don't do that.



-EINVAL doesn't look like the right error code. -ENODEV or -EIO would
seem more appropriate. Same issue several times, please address them
all.


As far as I can see, this masking is a no-op.



... here, with a else.


Can also be written min(ret, 100).


For loops like int better.


You should pass down the exact error value returned by the function,
instead of hardcoding it. Same below.


Wrong indentation.


Should be turned into dev_dbg().


Wrong indentation, and useless code, as kzalloc did it for you already.


What you did here is register a device, not a driver.



(typo: manufacturer)


... just to get the client from the data. Terribly efficient ;)


"-battery" is redundant, just use "bq20z75".


Needless comma.


This is essentially needless. The kernel will log the failure already.



--
Jean Delvare
[ message continues ]

Rather than using REG_MAX it'd seem safer to use ARRAY_SIZE() to make


Remove unconditional logging like this, make it dev_dbg() if you want to



The standard way to do this is with the suspend/resume functions -

This only makes sense if it's an actual person (or other contact
address); there's already copyright statements on the file, the author
information is more there to help find someone who might know something
about the driver.
--

[ message continues ]

Thanks for the review, fixed up what you suggested. Will repost soon.

-----Original Message-----
From: Mark Brown [mailto:broonie@opensource.wolfsonmicro.com] 
Sent: Tuesday, August 31, 2010 6:13 AM
To: Rhyland Klein
Cc: cbouatmailru@gmail.com; linux-i2c@vger.kernel.org; linux-kernel@vger.kernel.org; olof@lixom.net; Andrew Chew
Subject: Re: [PATCH 1/1] power supply: add driver for TI BQ20Z75 gas gauge IC


Rather than using REG_MAX it'd seem safer to use ARRAY_SIZE() to make
sure you're within the array.


Indentation is messed up here.


Remove unconditional logging like this, make it dev_dbg() if you want to
keep it in so it's only visible when explicitly requested.


Indentation again.


No need for null functions like this.


The standard way to do this is with the suspend/resume functions -
#define them to NULL which turns their assignments into noops.


This only makes sense if it's an actual person (or other contact
address); there's already copyright statements on the file, the author
information is more there to help find someone who might know something
about the driver.

-Done
--

[ message continues ]