Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20210421090354.GF19953@lemon.iwr.uni-heidelberg.de>
 <CAGb2v64U3vMew8LUU776Mx7jYj3eVb4FXQdXMZ0aJNBPUh2D2A@mail.gmail.com> <20210505112902.GC5302@lemon.iwr.uni-heidelberg.de>
In-Reply-To: <20210505112902.GC5302@lemon.iwr.uni-heidelberg.de>
Reply-To: wens@csie.org
From:   Chen-Yu Tsai <wens@csie.org>
Date:   Sun, 9 May 2021 23:05:56 +0800
Message-ID: <CAGb2v64UN6=26QiQLqSWmNJPo49bPOQ3Q-Oz=LsbZz3JcszU0Q@mail.gmail.com>
Subject: Re: [PATCH] power: supply: axp20x_battery: implement writeable status
 to enable/disable battery charging
To:     Hermann.Lauer@uni-heidelberg.de
Cc:     Sebastian Reichel <sre@kernel.org>,
        "open list:THERMAL" <linux-pm@vger.kernel.org>,
        linux-kernel <linux-kernel@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

Hi,

On Wed, May 5, 2021 at 7:29 PM <Hermann.Lauer@uni-heidelberg.de> wrote:
>
> Dear Maintainers,
>
> this patch allows enabling/disabling charging for the axp209 PMIC through a
> writeable status property.

  ^ writable

>
> This works on the axp209 of Banana {Pi M1+,Pro}. I have no access to
> the other chips the driver deals with.

This should work on all the AXP chips, as it's the same bit that
controls the charger on all of them.

> Thanks to ChenYu for the idea and greetings
>   Hermann
>
> Signed-off-by: Hermann.Lauer@uni-heidelberg.de

The patch itself looks good, but your commit message needs a bit of
cleanup. Your commit message should only include details about the
patch, especially why the patch was done, which is kind of missing.
"What" was done is already obvious when looking at the body of the
patch.

Any pleasantries and other comments addressed to maintainers should
be included after the "---" so as not to be included in git history.


Thanks
ChenYu


> ---
>  drivers/power/supply/axp20x_battery.c | 16 ++++++++++++++--
>  1 file changed, 14 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/power/supply/axp20x_battery.c b/drivers/power/supply/axp20x_battery.c
> --- a/drivers/power/supply/axp20x_battery.c
> +++ b/drivers/power/supply/axp20x_battery.c
> @@ -40,6 +40,7 @@
>  #define AXP209_FG_PERCENT              GENMASK(6, 0)
>  #define AXP22X_FG_VALID                        BIT(7)
>
> +#define AXP20X_CHRG_CTRL1_ENABLE       BIT(7)
>  #define AXP20X_CHRG_CTRL1_TGT_VOLT     GENMASK(6, 5)
>  #define AXP20X_CHRG_CTRL1_TGT_4_1V     (0 << 5)
>  #define AXP20X_CHRG_CTRL1_TGT_4_15V    (1 << 5)
> @@ -468,7 +469,17 @@
>         case POWER_SUPPLY_PROP_CONSTANT_CHARGE_CURRENT_MAX:
>                 return axp20x_set_max_constant_charge_current(axp20x_batt,
>                                                               val->intval);
> +       case POWER_SUPPLY_PROP_STATUS:
> +               switch (val->intval) {
> +               case POWER_SUPPLY_STATUS_CHARGING:
> +                       return regmap_update_bits(axp20x_batt->regmap, AXP20X_CHRG_CTRL1,
> +                               AXP20X_CHRG_CTRL1_ENABLE, AXP20X_CHRG_CTRL1_ENABLE);
>
> +               case POWER_SUPPLY_STATUS_DISCHARGING:
> +               case POWER_SUPPLY_STATUS_NOT_CHARGING:
> +                       return regmap_update_bits(axp20x_batt->regmap, AXP20X_CHRG_CTRL1,
> +                               AXP20X_CHRG_CTRL1_ENABLE, 0);
> +       }
>         default:
>                 return -EINVAL;
>         }
> @@ -491,7 +502,8 @@
>  static int axp20x_battery_prop_writeable(struct power_supply *psy,
>                                          enum power_supply_property psp)
>  {
> -       return psp == POWER_SUPPLY_PROP_VOLTAGE_MIN_DESIGN ||
> +       return psp == POWER_SUPPLY_PROP_STATUS ||
> +              psp == POWER_SUPPLY_PROP_VOLTAGE_MIN_DESIGN ||
>                psp == POWER_SUPPLY_PROP_VOLTAGE_MAX_DESIGN ||
>                psp == POWER_SUPPLY_PROP_CONSTANT_CHARGE_CURRENT ||
>                psp == POWER_SUPPLY_PROP_CONSTANT_CHARGE_CURRENT_MAX;