Re: [linux-sunxi] [PATCH 2/2] power: supply: axp20x_usb_power: add applied max Vbus support for AXP813

[Chen-Yu Tsai]

On Tue, Oct 8, 2019 at 11:09 AM Icenowy Zheng <icenowy@xxxxxxx> wrote:
> ä 2019å10æ8æ GMT+08:00 äå12:07:05, Chen-Yu Tsai <wens@xxxxxxxx> åå:
> >Hi,
> >
> >On Wed, Oct 2, 2019 at 7:27 PM Icenowy Zheng <icenowy@xxxxxxx> wrote:
> >>
> >> AXP813 PMIC has two Vbus maximum value settings -- one is the default
> >> value, which is currently the only supported one; the other is the
> >> really applied value, which is set according to the default value if
> >the
> >> BC detection module detected a charging port, or 500mA if no charging
> >> port is detected.
> >>
> >> Add support for reading and writing of the really applied Vbus
> >maxmium
> >> value. Interestingly it has a larger range than the default value.
> >>
> >> Signed-off-by: Icenowy Zheng <icenowy@xxxxxxx>
> >> ---
> >>  drivers/power/supply/axp20x_usb_power.c | 132
> >+++++++++++++++++++++++-
> >>  1 file changed, 129 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/drivers/power/supply/axp20x_usb_power.c
> >b/drivers/power/supply/axp20x_usb_power.c
> >> index 5f0a5722b19e..905668a2727f 100644
> >> --- a/drivers/power/supply/axp20x_usb_power.c
> >> +++ b/drivers/power/supply/axp20x_usb_power.c
> >
> >[...]
> >
> >> @@ -354,6 +451,9 @@ static int axp20x_usb_power_set_property(struct
> >power_supply *psy,
> >>
> >val->intval);
> >>                 return axp20x_usb_power_set_current_max(power,
> >val->intval);
> >>
> >> +       case POWER_SUPPLY_PROP_INPUT_CURRENT_LIMIT:
> >> +               return
> >axp20x_usb_power_set_input_current_limit(power, val->intval);
> >> +
> >
> >So I think there are two things that should be adjusted.
> >
> >First, we should be using POWER_SUPPLY_PROP_INPUT_CURRENT_LIMIT for all
> >PMICs.
> >As far as the sysfs documents go, CURRENT_MAX is read-only, and should
> >refer to
> >the hard limit the hardware can support, i.e. maximum power ratings.
> >INPUT_CURRENT_LIMIT and INPUT_VOLTAGE_LIMIT are for configurable upper
> >and lower
> >limits respectively.
> >
> >Sebastian, is my understanding of this correct?
> >
> >We already use INPUT_CURRENT_LIMIT for the AXP813 in the axp20x-ac
> >driver, and
> >it would be nice to have both drivers expose the same attributes.
> >
> >Second, since the value set in register 0x35 is the one that actually
> >has an
> >effect, as opposed to just being a default, we should just use that
> >one.
>
> However, that default value is also important, otherwise users will
> get dropped back to 500mAh each time they re-insert USB jack.
>
> Is there a property to export the default value?

Not that I know of. I suppose you could piggy back on INPUT_CURRENT_LIMIT
and just set both values at the same time. Of course the default value
has a smaller range, so you would end up setting the highest value for
actual values above its range.

> BTW, if possible, apply patch 1 first, because it can raise current to 1.5A
> in the default situation.

Agreed.

ChenYu

> >Could you restructure the series based on what I described, with a new
> >patch 1
> >switching from CURRENT_MAX to INPUT_CURRENT_LIMIT, and then this patch
> >as patch 2?
> >And both patches should have Fixes tags and possibly CC stable so they
> >get backported
> >for people that are using stable kernels? And then the original patch
> >2 as patch 3.
> >
> >ChenYu
> >
> >>         default:
> >>                 return -EINVAL;
> >>         }
> >> @@ -365,7 +465,8 @@ static int axp20x_usb_power_prop_writeable(struct
> >power_supply *psy,
> >>                                            enum power_supply_property
> >psp)
> >>  {
> >>         return psp == POWER_SUPPLY_PROP_VOLTAGE_MIN ||
> >> -              psp == POWER_SUPPLY_PROP_CURRENT_MAX;
> >> +              psp == POWER_SUPPLY_PROP_CURRENT_MAX ||
> >> +              psp == POWER_SUPPLY_PROP_INPUT_CURRENT_LIMIT;
> >>  }
> >>
> >>  static enum power_supply_property axp20x_usb_power_properties[] = {
> >> @@ -386,6 +487,15 @@ static enum power_supply_property
> >axp22x_usb_power_properties[] = {
> >>         POWER_SUPPLY_PROP_CURRENT_MAX,
> >>  };
> >>
> >> +static enum power_supply_property axp813_usb_power_properties[] = {
> >> +       POWER_SUPPLY_PROP_HEALTH,
> >> +       POWER_SUPPLY_PROP_PRESENT,
> >> +       POWER_SUPPLY_PROP_ONLINE,
> >> +       POWER_SUPPLY_PROP_VOLTAGE_MIN,
> >> +       POWER_SUPPLY_PROP_CURRENT_MAX,
> >> +       POWER_SUPPLY_PROP_INPUT_CURRENT_LIMIT,
> >> +};
> >> +
> >>  static const struct power_supply_desc axp20x_usb_power_desc = {
> >>         .name = "axp20x-usb",
> >>         .type = POWER_SUPPLY_TYPE_USB,
> >> @@ -406,6 +516,16 @@ static const struct power_supply_desc
> >axp22x_usb_power_desc = {
> >>         .set_property = axp20x_usb_power_set_property,
> >>  };
> >>
> >> +static const struct power_supply_desc axp813_usb_power_desc = {
> >> +       .name = "axp20x-usb",
> >> +       .type = POWER_SUPPLY_TYPE_USB,
> >> +       .properties = axp813_usb_power_properties,
> >> +       .num_properties = ARRAY_SIZE(axp813_usb_power_properties),
> >> +       .property_is_writeable = axp20x_usb_power_prop_writeable,
> >> +       .get_property = axp20x_usb_power_get_property,
> >> +       .set_property = axp20x_usb_power_set_property,
> >> +};
> >> +
> >>  static int configure_iio_channels(struct platform_device *pdev,
> >>                                   struct axp20x_usb_power *power)
> >>  {
> >> @@ -487,10 +607,16 @@ static int axp20x_usb_power_probe(struct
> >platform_device *pdev)
> >>                 usb_power_desc = &axp20x_usb_power_desc;
> >>                 irq_names = axp20x_irq_names;
> >>         } else if (power->axp20x_id == AXP221_ID ||
> >> -                  power->axp20x_id == AXP223_ID ||
> >> -                  power->axp20x_id == AXP813_ID) {
> >> +                  power->axp20x_id == AXP223_ID) {
> >>                 usb_power_desc = &axp22x_usb_power_desc;
> >>                 irq_names = axp22x_irq_names;
> >> +       } else if (power->axp20x_id == AXP813_ID) {
> >> +               usb_power_desc = &axp813_usb_power_desc;
> >> +               irq_names = axp22x_irq_names;
> >> +
> >> +               /* Enable USB Battery Charging specification
> >detection */
> >> +               regmap_update_bits(axp20x->regmap, AXP288_BC_GLOBAL,
> >> +                                  AXP813_BC_EN, AXP813_BC_EN);
> >
> >This seems like a duplicate of
> >
> >>         } else {
> >>                 dev_err(&pdev->dev, "Unsupported AXP variant: %ld\n",
> >>                         axp20x->variant);
> >> --
> >> 2.21.0
> >>
> >> --
> >> 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/20191002112545.58481-3-icenowy%40aosc.io.
>
> --
> äç K-9 Mail åéèæçAndroidèåã
>
> --
> 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/A6B1EF32-5CDE-4AAE-A7DA-F9A636BE7BF3%40aosc.io.