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

[Linus Walleij <linus.walleij@xxxxxxxxxx>]

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.

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.

> +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?

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.

> +/*
> + * 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?

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 don't know what others say though?

Yours,
Linus Walleij