All of lore.kernel.org
 help / color / mirror / Atom feed
From: Andy Shevchenko <andy.shevchenko@gmail.com>
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>
Subject: Re: [PATCH] power: supply: Add driver for TI BQ2416X battery charger
Date: Tue, 7 Feb 2017 13:08:21 +0200	[thread overview]
Message-ID: <CAHp75VfWNd8p7eg9TU2SNC1dqBMf0fmkbEVmkWOWcDqHrDgOLQ@mail.gmail.com> (raw)
In-Reply-To: <1486429749-12973-1-git-send-email-wojciech.ziemba@verifone.com>

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

WARNING: multiple messages have this Message-ID (diff)
From: Andy Shevchenko <andy.shevchenko-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
To: Wojciech Ziemba <wo.ziemba-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
Cc: Sebastian Reichel <sre-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>,
	Rob Herring <robh+dt-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>,
	Mark Rutland <mark.rutland-5wv7dgnIgG8@public.gmane.org>,
	wojciech.ziemba-VFDYytZz4I1Wk0Htik3J/w@public.gmane.org,
	"linux-pm-u79uwXL29TY76Z2rM5mHXA@public.gmane.org"
	<linux-pm-u79uwXL29TY76Z2rM5mHXA@public.gmane.org>,
	devicetree <devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org>,
	"linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org"
	<linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org>
Subject: Re: [PATCH] power: supply: Add driver for TI BQ2416X battery charger
Date: Tue, 7 Feb 2017 13:08:21 +0200	[thread overview]
Message-ID: <CAHp75VfWNd8p7eg9TU2SNC1dqBMf0fmkbEVmkWOWcDqHrDgOLQ@mail.gmail.com> (raw)
In-Reply-To: <1486429749-12973-1-git-send-email-wojciech.ziemba-VFDYytZz4I1Wk0Htik3J/w@public.gmane.org>

On Tue, Feb 7, 2017 at 3:09 AM, Wojciech Ziemba <wo.ziemba-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> 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
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

  parent reply	other threads:[~2017-02-07 11:08 UTC|newest]

Thread overview: 9+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2017-02-07  1:09 [PATCH] power: supply: Add driver for TI BQ2416X battery charger Wojciech Ziemba
2017-02-07  3:00 ` Liam Breck
2017-02-07  3:00   ` Liam Breck
2017-02-07 11:08 ` Andy Shevchenko [this message]
2017-02-07 11:08   ` Andy Shevchenko
2017-03-20  5:58 ` Sebastian Reichel
2017-03-20  5:58   ` Sebastian Reichel
2017-03-22 13:53   ` Wojciech Ziemba
2017-03-23 13:54     ` Sebastian Reichel

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=CAHp75VfWNd8p7eg9TU2SNC1dqBMf0fmkbEVmkWOWcDqHrDgOLQ@mail.gmail.com \
    --to=andy.shevchenko@gmail.com \
    --cc=devicetree@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-pm@vger.kernel.org \
    --cc=mark.rutland@arm.com \
    --cc=robh+dt@kernel.org \
    --cc=sre@kernel.org \
    --cc=wo.ziemba@gmail.com \
    --cc=wojciech.ziemba@verifone.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body. 
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.