| Message ID | 20200105012416.23296-8-samuel@sholland.org (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
| Series | X-Powers Power Supply Improvements | expand |
On Sun, Jan 5, 2020 at 9:24 AM Samuel Holland <samuel@sholland.org> 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. As for disabling charging, wouldn't it make more sense to disable the charger? 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@sholland.org> > --- > 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. > 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@csie.org>
On 1/5/20 4:40 AM, Chen-Yu Tsai wrote: > On Sun, Jan 5, 2020 at 9:24 AM Samuel Holland <samuel@sholland.org> 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(). > 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? > 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@sholland.org> >> --- >> 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. >> 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@csie.org> >
On Mon, Jan 6, 2020 at 1:47 AM Samuel Holland <samuel@sholland.org> wrote: > > On 1/5/20 4:40 AM, Chen-Yu Tsai wrote: > > On Sun, Jan 5, 2020 at 9:24 AM Samuel Holland <samuel@sholland.org> 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@sholland.org> > >> --- > >> 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@csie.org> > > > > -- > 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@googlegroups.com. > To view this discussion on the web, visit https://groups.google.com/d/msgid/linux-sunxi/f0c5e260-dcc3-2744-21cd-305e4534f2be%40sholland.org.
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); + 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; }
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. 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@sholland.org> --- drivers/power/supply/axp20x_usb_power.c | 27 +++++++++++++++++++++++++ 1 file changed, 27 insertions(+)