From: Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com>
To: Linus Walleij <linus.walleij@linaro.org>
Cc: Baolin Wang <baolin.wang@linaro.org>,
Matti Vaittinen <mazziesaccount@gmail.com>,
heikki.haikola@fi.rohmeurope.com,
mikko.mutanen@fi.rohmeurope.com, Lee Jones <lee.jones@linaro.org>,
Rob Herring <robh+dt@kernel.org>,
Mark Rutland <mark.rutland@arm.com>,
Mark Brown <broonie@kernel.org>,
Greg KH <gregkh@linuxfoundation.org>,
"Rafael J. Wysocki" <rafael@kernel.org>,
Michael Turquette <mturquette@baylibre.com>,
Stephen Boyd <sboyd@kernel.org>,
Bartosz Golaszewski <bgolaszewski@baylibre.com>,
Sebastian Reichel <sre@kernel.org>,
Liam Girdwood <lgirdwood@gmail.com>,
Alessandro Zummo <a.zummo@towertech.it>,
Alexandre Belloni <alexandre.belloni@bootlin.com>,
Wim Van Sebroeck <wim@linux-watchdog.org>,
Guenter Roeck <linux@roeck-us.net>,
"open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS"
<devicetree@vger.kernel.org>,
"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
linux-clk <linux-clk@vger.kernel.org>,
"open list:GPIO SUBSYSTEM" <linux-gpio@vger.kernel.org>,
Linux PM list <linux-pm@vger.kernel.org>,
linux-rtc@vger.kernel.org,
LINUXWATCHDOG <linux-watchdog@vger.kernel.org>
Subject: Re: [RFC PATCH v2 09/10] power: supply: Initial support for ROHM BD70528 PMIC charger block
Date: Mon, 28 Jan 2019 15:53:54 +0200 [thread overview]
Message-ID: <20190128135354.GA4156@localhost.localdomain> (raw)
In-Reply-To: <CACRpkdYTCgXo8FeitEfRujeWdshnsR3Kn57cKUZsA3CsZ5Cnrw@mail.gmail.com>
Hello Linus,
Big Thanks for the proper review at this early satge!
On Mon, Jan 28, 2019 at 01:49:04PM +0100, Linus Walleij wrote:
> Hi Matti!
>
> Thanks for your patch.
>
> We are going to have a problem with the power subsystem.
>
> These charging drivers are growing wild. This is starting to get out
> of hand, we need some more framework for properly handling charging
> state machines the kernel. Not specifically your problem, but
> when working on the driver try to keep generic support in mind.
I for sure can try - but as the power subsystem is quite new to me - any
specific items you would like me to really pay attention?
> On Fri, Jan 25, 2019 at 12:06 PM Matti Vaittinen
> <matti.vaittinen@fi.rohmeurope.com> wrote:
>
> > +#define CHG_STAT_SUSPEND 0x0
> > +#define CHG_STAT_TRICKLE 0x1
> > +#define CHG_STAT_FAST 0x3
> > +#define CHG_STAT_TOPOFF 0xe
> > +#define CHG_STAT_DONE 0xf
> > +#define CHG_STAT_OTP_TRICKLE 0x10
> > +#define CHG_STAT_OTP_FAST 0x11
> > +#define CHG_STAT_OTP_DONE 0x12
> > +#define CHG_STAT_TSD_TRICKLE 0x20
> > +#define CHG_STAT_TSD_FAST 0x21
> > +#define CHG_STAT_TSD_TOPOFF 0x22
> > +#define CHG_STAT_BAT_ERR 0x7f
>
> So what I am seeing is that these states are starting to turn up in more
> and more drivers, so we really need to think about a central management
> component for charging state machines. I do not think they are all
> that different after all.
Any suggestions how I should take this into account with bd70528?
> > +BD_ERR_IRQ_HND(BAT_OV_DET, "Battery overvoltage detected\n");
> > +BD_ERR_IRQ_HND(DBAT_DET, "Dead battery detected\n");
> > +BD_ERR_IRQ_HND(COLD_DET, "Battery cold\n");
> > +BD_ERR_IRQ_HND(HOT_DET, "Battery hot\n");
> > +BD_ERR_IRQ_HND(CHG_TSD, "Charger thermal shutdown\n");
> > +BD_ERR_IRQ_HND(DCIN2_OV_DET, "DCIN2 overvoltage detected\n");
> > +
> > +BD_INFO_IRQ_HND(BAT_OV_RES, "Battery voltage back to normal\n");
> > +BD_INFO_IRQ_HND(COLD_RES, "Battery temperature back to normal\n");
> > +BD_INFO_IRQ_HND(HOT_RES, "Battery temperature back to normal\n");
> > +BD_INFO_IRQ_HND(BAT_RMV, "Battery removed\n");
> > +BD_INFO_IRQ_HND(BAT_DET, "Battery detected\n");
> > +BD_INFO_IRQ_HND(DCIN2_OV_RES, "DCIN2 voltage back to normal\n");
> > +BD_INFO_IRQ_HND(DCIN2_RMV, "DCIN2 removed\n");
> > +BD_INFO_IRQ_HND(DCIN2_DET, "DCIN2 detected\n");
> > +BD_INFO_IRQ_HND(DCIN1_RMV, "DCIN1 removed\n");
> > +BD_INFO_IRQ_HND(DCIN1_DET, "DCIN1 detected\n");
>
> So we have states and events, and these events form edges
> between the states, right?
Right. State change causes an irq.
> I am certain you must have a graphical picture of this state
> machine somewhere, it seems to be how charging hardware people
> do their thinking.
I don't have any document I could link to yet. I can ask around if we
can have some public doc for this :/ And as a last resort I can do some
ASCII art in commenets - if this is seen helpfull.
> > +/*
> > + * For BD70528 voltage/current limits we happily accept any value which
> > + * belongs the range. We could check if value matching the selector is
> > + * desired by computing the range min + (sel - sel_low) * range step - but
> > + * I guess it is enough if we use voltage/current which is closest (below)
> > + * the requested?
> > + */
> > +static int find_selector_for_value_low(struct linear_range *r, int selectors,
> > + unsigned int val, unsigned int *sel,
> > + bool *found)
> > +{
> > + int i;
> > + int ret = -EINVAL;
> > +
> > + *found = false;
> > + for (i = 0; i < selectors; i++) {
> > + if (r[i].min <= val) {
> > + if (r[i].min + r[i].step * r[i].vals >= val) {
> > + *found = true;
> > + *sel = r[i].low_sel + (val - r[i].min) /
> > + r[i].step;
> > + ret = 0;
> > + break;
> > + }
> > + /*
> > + * If the range max is smaller than requested
> > + * we can set the max supported value from range
> > + */
> > + *sel = r[i].low_sel + r[i].vals;
> > + ret = 0;
> > + }
> > + }
> > + return ret;
> > +}
>
> If I'm not mistaken this is yet another instance of linear interpolation
> from a table?
"linear interpolation from a table" is really not part of my
vocabulary :] But I guess you know the REGULATOR_LINEAR_VOLTAGE - macro?
I borrowed the idea from there...
>
> We really need to think about abstracting this. Last time this
> duplication appeared I suggested adding linear interpolation
> primitives to:
> include/linux/fixp-arith.h
... I really think a generic helper for this would be usefull.
It will take some time untill I can send a proper (non RFC patch) for
the charger block as I currently lack of HW I could use for testing the
charger properly. Do you think it is better to drop the charger part
from the series untill then and submit it only later? As I mentioned in
cover-letter, the charger part is currently submitted more to give an
overview of the chip than to be applied as 'finalized' version of
driver.
Br,
Matti Vaittinen
next prev parent reply other threads:[~2019-01-28 13:54 UTC|newest]
Thread overview: 4+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-01-25 11:06 [RFC PATCH v2 09/10] power: supply: Initial support for ROHM BD70528 PMIC charger block Matti Vaittinen
2019-01-28 12:49 ` Linus Walleij
2019-01-28 13:53 ` Matti Vaittinen [this message]
2019-01-28 14:46 ` Linus Walleij
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=20190128135354.GA4156@localhost.localdomain \
--to=matti.vaittinen@fi.rohmeurope.com \
--cc=a.zummo@towertech.it \
--cc=alexandre.belloni@bootlin.com \
--cc=baolin.wang@linaro.org \
--cc=bgolaszewski@baylibre.com \
--cc=broonie@kernel.org \
--cc=devicetree@vger.kernel.org \
--cc=gregkh@linuxfoundation.org \
--cc=heikki.haikola@fi.rohmeurope.com \
--cc=lee.jones@linaro.org \
--cc=lgirdwood@gmail.com \
--cc=linus.walleij@linaro.org \
--cc=linux-clk@vger.kernel.org \
--cc=linux-gpio@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-pm@vger.kernel.org \
--cc=linux-rtc@vger.kernel.org \
--cc=linux-watchdog@vger.kernel.org \
--cc=linux@roeck-us.net \
--cc=mark.rutland@arm.com \
--cc=mazziesaccount@gmail.com \
--cc=mikko.mutanen@fi.rohmeurope.com \
--cc=mturquette@baylibre.com \
--cc=rafael@kernel.org \
--cc=robh+dt@kernel.org \
--cc=sboyd@kernel.org \
--cc=sre@kernel.org \
--cc=wim@linux-watchdog.org \
/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 a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).