Re: [RFC PATCH v2 09/10] power: supply: Initial support for ROHM BD70528 PMIC charger block

[Matti Vaittinen <matti.vaittinen@xxxxxxxxxxxxxxxxx>]

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@xxxxxxxxxxxxxxxxx> 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