Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
MIME-Version: 1.0
References: <20191002112545.58481-1-icenowy@aosc.io> <20191002112545.58481-3-icenowy@aosc.io>
In-Reply-To: <20191002112545.58481-3-icenowy@aosc.io>
From:   Chen-Yu Tsai <wens@csie.org>
Date:   Tue, 8 Oct 2019 00:07:05 +0800
Message-ID: <CAGb2v65MPG=zbKtQk1oXq+TYP=0fPBXEj0fcGA=6mCD2+Smmpg@mail.gmail.com>
Subject: Re: [linux-sunxi] [PATCH 2/2] power: supply: axp20x_usb_power: add
 applied max Vbus support for AXP813
To:     Icenowy Zheng <icenowy@aosc.io>, Sebastian Reichel <sre@kernel.org>
Cc:     "open list:THERMAL" <linux-pm@vger.kernel.org>,
        linux-kernel <linux-kernel@vger.kernel.org>,
        linux-sunxi <linux-sunxi@googlegroups.com>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

Hi,

On Wed, Oct 2, 2019 at 7:27 PM Icenowy Zheng <icenowy@aosc.io> 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@aosc.io>
> ---
>  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.

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@googlegroups.com.
> To view this discussion on the web, visit https://groups.google.com/d/msgid/linux-sunxi/20191002112545.58481-3-icenowy%40aosc.io.