Re: [PATCH] power: supply: Add driver for TI BQ2416X battery charger

[Andy Shevchenko]

On Tue, Feb 7, 2017 at 3:09 AM, Wojciech Ziemba <wo.ziemba@xxxxxxxxx> wrote:
> There is interest in adding a Linux driver for TI BQ2416X battery
> charger. The driver supports BQ24160 chip, thus can be easily extended
> for other BQ2416X family chargers.
> Device exposes 'POWER_SUPPLY_PROP_*' properties and a number of knobs
> for controlling the charging process as well as sends power supply change
> notification via power-supply subsystem.

Some style related comments

0. Less lines of code -> better! (but not be too eager)
1. Use GENMASK()
2.
int ret = -EINVAL;

if (a)
 ret = x;
else if (b)
 ret = y;

>> else
>> ret = -EINVAL;

Those are redundant.

if (ret)
 return ret;

3. #ifdef:s are ugly.
For CONFIG_PM_SLEEP functions just use
__maybe_unused attribute.

4. Try to avoid #ifdef CONFIG_OF (this will limit driver for OF case
when it might be used elsewhere, e.g. ACPI case)

5. Check headers and library for existing helpers. I believe some of
your bit operations and such already have nice helpers in kernel.

-- 
With Best Regards,
Andy Shevchenko