Re: [linux-sunxi] [PATCH v2 7/9] power: supply: axp20x_usb_power: Allow offlining

[Chen-Yu Tsai]

On Mon, Jan 6, 2020 at 1:47 AM Samuel Holland <samuel@xxxxxxxxxxxx> wrote:
>
> On 1/5/20 4:40 AM, Chen-Yu Tsai wrote:
> > On Sun, Jan 5, 2020 at 9:24 AM Samuel Holland <samuel@xxxxxxxxxxxx> wrote:
> >>
> >> AXP803/AXP813 have a flag that enables/disables the USB power supply
> >> input. Allow control of this flag via the ONLINE property on those
> >> variants.
> >>
> >> It may be necessary to offline the USB power supply input when using
> >> the USB port in OTG mode, or to allow userspace to disable charging.
> >
> > Any idea how the former would be implemented? AFAIK this isn't allowed
> > right now.
>
> Pinephone currently has AXP N_VBUSEN/DRIVEVBUS floating, so the hardware doesn't
> automatically disable the VBUS path when enabling the boost regulator driving
> it. This doubles the current draw from the battery.
>
> The USB PHY driver would need to call:
>
>     union power_supply_propval val = { .intval = false };
>     power_supply_set_property(data->vbus_power_supply,
>                               POWER_SUPPLY_PROP_ONLINE, &val);
>
> or similar to set VBUS offline in sun4i_usb_phy_power_on(), and set it back
> online in sun4i_usb_phy_power_off().

Ah, OK. That's a valid use case. I had something else in mind, the OTG host
mode with charger one.

> > As for disabling charging, wouldn't it make more sense to disable the
> > charger?
>
> Yes, I see now that there's a bit at 33H[7] for this. I don't see an obvious
> property to hook it up to, though. Maybe POWER_SUPPLY_PROP_CHARGE_TYPE ==
> POWER_SUPPLY_CHARGE_TYPE_NONE?

Maybe? I suppose the sysfs ABI docs would have some clues.

> > Either way, these are not directly related to the changes. I'm just curious.
> >
> >> When the USB VBUS input is disabled via the PATH_SEL bit, the VBUS_USED
> >> bit in PWR_INPUT_STATUS is cleared, so there is no change needed when
> >> getting the property.
> >>
> >> Signed-off-by: Samuel Holland <samuel@xxxxxxxxxxxx>
> >> ---
> >>  drivers/power/supply/axp20x_usb_power.c | 27 +++++++++++++++++++++++++
> >>  1 file changed, 27 insertions(+)
> >>
> >> diff --git a/drivers/power/supply/axp20x_usb_power.c b/drivers/power/supply/axp20x_usb_power.c
> >> index 2d7272e19a87..68443f264dff 100644
> >> --- a/drivers/power/supply/axp20x_usb_power.c
> >> +++ b/drivers/power/supply/axp20x_usb_power.c
> >> @@ -29,6 +29,9 @@
> >>
> >>  #define AXP20X_USB_STATUS_VBUS_VALID   BIT(2)
> >>
> >> +#define AXP20X_VBUS_PATH_SEL           BIT(7)
> >> +#define AXP20X_VBUS_PATH_SEL_OFFSET    7
> >> +
> >>  #define AXP20X_VBUS_VHOLD_uV(b)                (4000000 + (((b) >> 3) & 7) * 100000)
> >>  #define AXP20X_VBUS_VHOLD_MASK         GENMASK(5, 3)
> >>  #define AXP20X_VBUS_VHOLD_OFFSET       3
> >> @@ -263,6 +266,16 @@ static int axp20x_usb_power_get_property(struct power_supply *psy,
> >>         return 0;
> >>  }
> >>
> >> +static int axp813_usb_power_set_online(struct axp20x_usb_power *power,
> >> +                                      int intval)
> >> +{
> >> +       int val = !intval << AXP20X_VBUS_PATH_SEL_OFFSET;
> >> +
> >> +       return regmap_update_bits(power->regmap,
> >> +                                 AXP20X_VBUS_IPSOUT_MGMT,
> >> +                                 AXP20X_VBUS_PATH_SEL, val);
> >> +}
> >> +
> >>  static int axp20x_usb_power_set_voltage_min(struct axp20x_usb_power *power,
> >>                                             int intval)
> >>  {
> >> @@ -344,6 +357,9 @@ static int axp20x_usb_power_set_property(struct power_supply *psy,
> >>         struct axp20x_usb_power *power = power_supply_get_drvdata(psy);
> >>
> >>         switch (psp) {
> >> +       case POWER_SUPPLY_PROP_ONLINE:
> >> +               return axp813_usb_power_set_online(power, val->intval);
> >> +
> >
> > I would add a comment here pointing to the next change as to why there's
> > only an axp813-specific callback used here.
>
> I'll add this for v3.

Thanks
ChenYu

> >>         case POWER_SUPPLY_PROP_VOLTAGE_MIN:
> >>                 return axp20x_usb_power_set_voltage_min(power, val->intval);
> >>
> >> @@ -363,6 +379,17 @@ static int axp20x_usb_power_set_property(struct power_supply *psy,
> >>  static int axp20x_usb_power_prop_writeable(struct power_supply *psy,
> >>                                            enum power_supply_property psp)
> >>  {
> >> +       struct axp20x_usb_power *power = power_supply_get_drvdata(psy);
> >> +
> >> +       /*
> >> +        * Both AXP2xx and AXP8xx have a VBUS path select flag.
> >> +        * On AXP2xx, setting the flag enables VBUS (ignoring N_VBUSEN).
> >> +        * On AXP8xx, setting the flag disables VBUS (ignoring N_VBUSEN).
> >> +        * So we only expose the control on AXP8xx where it is meaningful.
> >> +        */
> >> +       if (psp == POWER_SUPPLY_PROP_ONLINE)
> >> +               return power->axp20x_id == AXP813_ID;
> >> +
> >>         return psp == POWER_SUPPLY_PROP_VOLTAGE_MIN ||
> >>                psp == POWER_SUPPLY_PROP_CURRENT_MAX;
> >>  }
> >> --
> >
> > Otherwise,
> >
> > Reviewed-by: Chen-Yu Tsai <wens@xxxxxxxx>
> >
>
> --
> You received this message because you are subscribed to the Google Groups "linux-sunxi" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to linux-sunxi+unsubscribe@xxxxxxxxxxxxxxxxx
> To view this discussion on the web, visit https://groups.google.com/d/msgid/linux-sunxi/f0c5e260-dcc3-2744-21cd-305e4534f2be%40sholland.org.