Messages in this thread | | | From | Andy Shevchenko <> | Date | Tue, 7 Feb 2017 13:08:21 +0200 | Subject | Re: [PATCH] power: supply: Add driver for TI BQ2416X battery charger |
| |
On Tue, Feb 7, 2017 at 3:09 AM, Wojciech Ziemba <wo.ziemba@gmail.com> 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
| |