From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <linux-kernel-owner@vger.kernel.org>
Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand
        id S1753866AbdBGLIY (ORCPT <rfc822;w@1wt.eu>);
        Tue, 7 Feb 2017 06:08:24 -0500
Received: from mail-qk0-f195.google.com ([209.85.220.195]:34546 "EHLO
        mail-qk0-f195.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org
        with ESMTP id S1753268AbdBGLIX (ORCPT
        <rfc822;linux-kernel@vger.kernel.org>);
        Tue, 7 Feb 2017 06:08:23 -0500
MIME-Version: 1.0
In-Reply-To: <1486429749-12973-1-git-send-email-wojciech.ziemba@verifone.com>
References: <1486429749-12973-1-git-send-email-wojciech.ziemba@verifone.com>
From: Andy Shevchenko <andy.shevchenko@gmail.com>
Date: Tue, 7 Feb 2017 13:08:21 +0200
Message-ID: <CAHp75VfWNd8p7eg9TU2SNC1dqBMf0fmkbEVmkWOWcDqHrDgOLQ@mail.gmail.com>
Subject: Re: [PATCH] power: supply: Add driver for TI BQ2416X battery charger
To: Wojciech Ziemba <wo.ziemba@gmail.com>
Cc: Sebastian Reichel <sre@kernel.org>, Rob Herring <robh+dt@kernel.org>,
        Mark Rutland <mark.rutland@arm.com>, wojciech.ziemba@verifone.com,
        "linux-pm@vger.kernel.org" <linux-pm@vger.kernel.org>,
        devicetree <devicetree@vger.kernel.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Content-Type: text/plain; charset=UTF-8
Sender: linux-kernel-owner@vger.kernel.org
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

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