[v2] power: supply: Add enable the primary charger interface

From: Chen Yongzhi <Yongzhi.Chen@unisoc.com>

In the case of charging multiple charging ICs,the primary
charging IC often needs to be turned off in the fast
charging stage, and only using the charger pump to charge,
need to add a new power_supply_property attribute.

Signed-off-by: Chen Yongzhi <Yongzhi.Chen@unisoc.com>
Reported-by: kernel test robot <lkp@intel.com>
Link: https://lore.kernel.org/all/202204230206.9TgyhSb1-lkp@intel.com
Signed-off-by: Cixi Geng <cixi.geng1@unisoc.com>
---
 drivers/power/supply/sc2731_charger.c | 52 +++++++++++++++++++++++++--
 include/linux/power_supply.h          |  1 +
 2 files changed, 50 insertions(+), 3 deletions(-)

On Thu, Apr 28, 2022 at 8:56 PM Cixi Geng <gengcixi@gmail.com> wrote:
>
> From: Chen Yongzhi <Yongzhi.Chen@unisoc.com>
>
> In the case of charging multiple charging ICs,the primary
> charging IC often needs to be turned off in the fast
> charging stage, and only using the charger pump to charge,
> need to add a new power_supply_property attribute.

I'm still confused why introducing a new
POWER_SUPPLY_PROP_CHARGE_ENABLED property to control the charging, but
you already controlled the charging by POWER_SUPPLY_PROP_STATUS?

>
> Signed-off-by: Chen Yongzhi <Yongzhi.Chen@unisoc.com>
> Reported-by: kernel test robot <lkp@intel.com>
> Link: https://lore.kernel.org/all/202204230206.9TgyhSb1-lkp@intel.com
> Signed-off-by: Cixi Geng <cixi.geng1@unisoc.com>
> ---
>  drivers/power/supply/sc2731_charger.c | 52 +++++++++++++++++++++++++--
>  include/linux/power_supply.h          |  1 +
>  2 files changed, 50 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/power/supply/sc2731_charger.c b/drivers/power/supply/sc2731_charger.c
> index 9ac17cf7a126..c15f9b75e6a8 100644
> --- a/drivers/power/supply/sc2731_charger.c
> +++ b/drivers/power/supply/sc2731_charger.c
> @@ -1,5 +1,5 @@
>  // SPDX-License-Identifier: GPL-2.0
> -// Copyright (C) 2018 Spreadtrum Communications Inc.
> +// Copyright (C) 2022 Spreadtrum Communications Inc.

Do not add unrelated changes.

>
>  #include <linux/module.h>
>  #include <linux/platform_device.h>
> @@ -146,6 +146,24 @@ static int sc2731_charger_get_status(struct sc2731_charger_info *info)
>         return POWER_SUPPLY_STATUS_CHARGING;
>  }
>
> +static int sc2731_charger_set_status(struct sc2731_charger_info *info, int val)
> +{
> +       int ret = 0;
> +
> +       if (!val && info->charging) {
> +               sc2731_charger_stop_charge(info);
> +               info->charging = false;
> +       } else if (val && !info->charging) {
> +               ret = sc2731_charger_start_charge(info);
> +               if (ret)
> +                       dev_err(info->dev, "start charge failed\n");

Duplicate error information, since you already print errors in
sc2731_charger_usb_set_property()

> +               else
> +                       info->charging = true;
> +       }
> +
> +       return ret;
> +}
> +
>  static int sc2731_charger_get_current(struct sc2731_charger_info *info,
>                                       u32 *cur)
>  {
> @@ -204,7 +222,7 @@ sc2731_charger_usb_set_property(struct power_supply *psy,
>                                 const union power_supply_propval *val)
>  {
>         struct sc2731_charger_info *info = power_supply_get_drvdata(psy);
> -       int ret;
> +       int ret = 0;
>
>         mutex_lock(&info->lock);
>
> @@ -214,6 +232,12 @@ sc2731_charger_usb_set_property(struct power_supply *psy,
>         }
>
>         switch (psp) {
> +       case POWER_SUPPLY_PROP_STATUS:
> +               ret = sc2731_charger_set_status(info, val->intval);
> +               if (ret < 0)
> +                       dev_err(info->dev, "set charge status failed\n");
> +               break;
> +
>         case POWER_SUPPLY_PROP_CONSTANT_CHARGE_CURRENT:
>                 ret = sc2731_charger_set_current(info, val->intval / 1000);
>                 if (ret < 0)
> @@ -227,6 +251,15 @@ sc2731_charger_usb_set_property(struct power_supply *psy,
>                         dev_err(info->dev, "set input current limit failed\n");
>                 break;
>
> +       case POWER_SUPPLY_PROP_CHARGE_ENABLED:
> +               if (val->intval == true) {
> +                       ret = sc2731_charger_start_charge(info);
> +                       if (ret)
> +                               dev_err(info->dev, "start charge failed\n");
> +               } else if (val->intval == false) {
> +                       sc2731_charger_stop_charge(info);
> +               }
> +               break;
>         default:
>                 ret = -EINVAL;
>         }
> @@ -241,7 +274,7 @@ static int sc2731_charger_usb_get_property(struct power_supply *psy,
>  {
>         struct sc2731_charger_info *info = power_supply_get_drvdata(psy);
>         int ret = 0;
> -       u32 cur;
> +       u32 cur, enabled;
>
>         mutex_lock(&info->lock);
>
> @@ -277,6 +310,16 @@ static int sc2731_charger_usb_get_property(struct power_supply *psy,
>                 }
>                 break;
>
> +       case POWER_SUPPLY_PROP_CHARGE_ENABLED:
> +               ret = regmap_read(info->regmap, info->base + SC2731_CHG_CFG0, &enabled);
> +               if (ret) {
> +                       dev_err(info->dev, "get sc2731 charge enabled failed\n");
> +                       goto out;
> +               }
> +
> +               val->intval = enabled & SC2731_CHARGER_PD;
> +
> +               break;
>         default:
>                 ret = -EINVAL;
>         }
> @@ -292,8 +335,10 @@ static int sc2731_charger_property_is_writeable(struct power_supply *psy,
>         int ret;
>
>         switch (psp) {
> +       case POWER_SUPPLY_PROP_STATUS:
>         case POWER_SUPPLY_PROP_CONSTANT_CHARGE_CURRENT:
>         case POWER_SUPPLY_PROP_INPUT_CURRENT_LIMIT:
> +       case POWER_SUPPLY_PROP_CHARGE_ENABLED:
>                 ret = 1;
>                 break;
>
> @@ -308,6 +353,7 @@ static enum power_supply_property sc2731_usb_props[] = {
>         POWER_SUPPLY_PROP_STATUS,
>         POWER_SUPPLY_PROP_CONSTANT_CHARGE_CURRENT,
>         POWER_SUPPLY_PROP_INPUT_CURRENT_LIMIT,
> +       POWER_SUPPLY_PROP_CHARGE_ENABLED,
>  };
>
>  static const struct power_supply_desc sc2731_charger_desc = {
> diff --git a/include/linux/power_supply.h b/include/linux/power_supply.h
> index cb380c1d9459..1dfe194d8a5e 100644
> --- a/include/linux/power_supply.h
> +++ b/include/linux/power_supply.h
> @@ -167,6 +167,7 @@ enum power_supply_property {
>         POWER_SUPPLY_PROP_PRECHARGE_CURRENT,
>         POWER_SUPPLY_PROP_CHARGE_TERM_CURRENT,
>         POWER_SUPPLY_PROP_CALIBRATE,
> +       POWER_SUPPLY_PROP_CHARGE_ENABLED,
>         POWER_SUPPLY_PROP_MANUFACTURE_YEAR,
>         POWER_SUPPLY_PROP_MANUFACTURE_MONTH,
>         POWER_SUPPLY_PROP_MANUFACTURE_DAY,
> --
> 2.25.1
>

Baolin Wang <baolin.wang7@gmail.com> 于2022年5月3日周二 12:53写道：
>
> On Thu, Apr 28, 2022 at 8:56 PM Cixi Geng <gengcixi@gmail.com> wrote:
> >
> > From: Chen Yongzhi <Yongzhi.Chen@unisoc.com>
> >
> > In the case of charging multiple charging ICs,the primary
> > charging IC often needs to be turned off in the fast
> > charging stage, and only using the charger pump to charge,
> > need to add a new power_supply_property attribute.
>
> I'm still confused why introducing a new
> POWER_SUPPLY_PROP_CHARGE_ENABLED property to control the charging, but
> you already controlled the charging by POWER_SUPPLY_PROP_STATUS?
>
Our purpose is to achieve two different stop charging states:
POWER_SUPPLY_PROP_STATUS: The software status stops charging, and the
hardware also stops charging；
POWER_SUPPLY_PROP_CHARGE_ENABLED: The hardware is stopped charging,
the software is still charging；
  Our  don't want to change the charge_status switch due to the
switching of charging and discharging of the charging IC in the
charging scenario of multiple charging ICs, and let the upper layer
perceive this switching
> >
> > Signed-off-by: Chen Yongzhi <Yongzhi.Chen@unisoc.com>
> > Reported-by: kernel test robot <lkp@intel.com>
> > Link: https://lore.kernel.org/all/202204230206.9TgyhSb1-lkp@intel.com
> > Signed-off-by: Cixi Geng <cixi.geng1@unisoc.com>
> > ---
> >  drivers/power/supply/sc2731_charger.c | 52 +++++++++++++++++++++++++--
> >  include/linux/power_supply.h          |  1 +
> >  2 files changed, 50 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/power/supply/sc2731_charger.c b/drivers/power/supply/sc2731_charger.c
> > index 9ac17cf7a126..c15f9b75e6a8 100644
> > --- a/drivers/power/supply/sc2731_charger.c
> > +++ b/drivers/power/supply/sc2731_charger.c
> > @@ -1,5 +1,5 @@
> >  // SPDX-License-Identifier: GPL-2.0
> > -// Copyright (C) 2018 Spreadtrum Communications Inc.
> > +// Copyright (C) 2022 Spreadtrum Communications Inc.
>
> Do not add unrelated changes.
>
  ok, do not change
> >
> >  #include <linux/module.h>
> >  #include <linux/platform_device.h>
> > @@ -146,6 +146,24 @@ static int sc2731_charger_get_status(struct sc2731_charger_info *info)
> >         return POWER_SUPPLY_STATUS_CHARGING;
> >  }
> >
> > +static int sc2731_charger_set_status(struct sc2731_charger_info *info, int val)
> > +{
> > +       int ret = 0;
> > +
> > +       if (!val && info->charging) {
> > +               sc2731_charger_stop_charge(info);
> > +               info->charging = false;
> > +       } else if (val && !info->charging) {
> > +               ret = sc2731_charger_start_charge(info);
> > +               if (ret)
> > +                       dev_err(info->dev, "start charge failed\n");
>
> Duplicate error information, since you already print errors in
   fix it
> sc2731_charger_usb_set_property()
>
> > +               else
> > +                       info->charging = true;
> > +       }
> > +
> > +       return ret;
> > +}
> > +
> >  static int sc2731_charger_get_current(struct sc2731_charger_info *info,
> >                                       u32 *cur)
> >  {
> > @@ -204,7 +222,7 @@ sc2731_charger_usb_set_property(struct power_supply *psy,
> >                                 const union power_supply_propval *val)
> >  {
> >         struct sc2731_charger_info *info = power_supply_get_drvdata(psy);
> > -       int ret;
> > +       int ret = 0;
> >
> >         mutex_lock(&info->lock);
> >
> > @@ -214,6 +232,12 @@ sc2731_charger_usb_set_property(struct power_supply *psy,
> >         }
> >
> >         switch (psp) {
> > +       case POWER_SUPPLY_PROP_STATUS:
> > +               ret = sc2731_charger_set_status(info, val->intval);
> > +               if (ret < 0)
> > +                       dev_err(info->dev, "set charge status failed\n");
> > +               break;
> > +
> >         case POWER_SUPPLY_PROP_CONSTANT_CHARGE_CURRENT:
> >                 ret = sc2731_charger_set_current(info, val->intval / 1000);
> >                 if (ret < 0)
> > @@ -227,6 +251,15 @@ sc2731_charger_usb_set_property(struct power_supply *psy,
> >                         dev_err(info->dev, "set input current limit failed\n");
> >                 break;
> >
> > +       case POWER_SUPPLY_PROP_CHARGE_ENABLED:
> > +               if (val->intval == true) {
> > +                       ret = sc2731_charger_start_charge(info);
> > +                       if (ret)
> > +                               dev_err(info->dev, "start charge failed\n");
> > +               } else if (val->intval == false) {
> > +                       sc2731_charger_stop_charge(info);
> > +               }
> > +               break;
> >         default:
> >                 ret = -EINVAL;
> >         }
> > @@ -241,7 +274,7 @@ static int sc2731_charger_usb_get_property(struct power_supply *psy,
> >  {
> >         struct sc2731_charger_info *info = power_supply_get_drvdata(psy);
> >         int ret = 0;
> > -       u32 cur;
> > +       u32 cur, enabled;
> >
> >         mutex_lock(&info->lock);
> >
> > @@ -277,6 +310,16 @@ static int sc2731_charger_usb_get_property(struct power_supply *psy,
> >                 }
> >                 break;
> >
> > +       case POWER_SUPPLY_PROP_CHARGE_ENABLED:
> > +               ret = regmap_read(info->regmap, info->base + SC2731_CHG_CFG0, &enabled);
> > +               if (ret) {
> > +                       dev_err(info->dev, "get sc2731 charge enabled failed\n");
> > +                       goto out;
> > +               }
> > +
> > +               val->intval = enabled & SC2731_CHARGER_PD;
> > +
> > +               break;
> >         default:
> >                 ret = -EINVAL;
> >         }
> > @@ -292,8 +335,10 @@ static int sc2731_charger_property_is_writeable(struct power_supply *psy,
> >         int ret;
> >
> >         switch (psp) {
> > +       case POWER_SUPPLY_PROP_STATUS:
> >         case POWER_SUPPLY_PROP_CONSTANT_CHARGE_CURRENT:
> >         case POWER_SUPPLY_PROP_INPUT_CURRENT_LIMIT:
> > +       case POWER_SUPPLY_PROP_CHARGE_ENABLED:
> >                 ret = 1;
> >                 break;
> >
> > @@ -308,6 +353,7 @@ static enum power_supply_property sc2731_usb_props[] = {
> >         POWER_SUPPLY_PROP_STATUS,
> >         POWER_SUPPLY_PROP_CONSTANT_CHARGE_CURRENT,
> >         POWER_SUPPLY_PROP_INPUT_CURRENT_LIMIT,
> > +       POWER_SUPPLY_PROP_CHARGE_ENABLED,
> >  };
> >
> >  static const struct power_supply_desc sc2731_charger_desc = {
> > diff --git a/include/linux/power_supply.h b/include/linux/power_supply.h
> > index cb380c1d9459..1dfe194d8a5e 100644
> > --- a/include/linux/power_supply.h
> > +++ b/include/linux/power_supply.h
> > @@ -167,6 +167,7 @@ enum power_supply_property {
> >         POWER_SUPPLY_PROP_PRECHARGE_CURRENT,
> >         POWER_SUPPLY_PROP_CHARGE_TERM_CURRENT,
> >         POWER_SUPPLY_PROP_CALIBRATE,
> > +       POWER_SUPPLY_PROP_CHARGE_ENABLED,
> >         POWER_SUPPLY_PROP_MANUFACTURE_YEAR,
> >         POWER_SUPPLY_PROP_MANUFACTURE_MONTH,
> >         POWER_SUPPLY_PROP_MANUFACTURE_DAY,
> > --
> > 2.25.1
> >
>
>
> --
> Baolin Wang

Hi,

On Thu, May 12, 2022 at 11:22 AM 陈永志 <chenyongzhi811@gmail.com> wrote:
>
> Baolin Wang <baolin.wang7@gmail.com> 于2022年5月3日周二 12:53写道：
> >
> > On Thu, Apr 28, 2022 at 8:56 PM Cixi Geng <gengcixi@gmail.com> wrote:
> > >
> > > From: Chen Yongzhi <Yongzhi.Chen@unisoc.com>
> > >
> > > In the case of charging multiple charging ICs,the primary
> > > charging IC often needs to be turned off in the fast
> > > charging stage, and only using the charger pump to charge,
> > > need to add a new power_supply_property attribute.
> >
> > I'm still confused why introducing a new
> > POWER_SUPPLY_PROP_CHARGE_ENABLED property to control the charging, but
> > you already controlled the charging by POWER_SUPPLY_PROP_STATUS?
> >
> Our purpose is to achieve two different stop charging states:
> POWER_SUPPLY_PROP_STATUS: The software status stops charging, and the
> hardware also stops charging；
> POWER_SUPPLY_PROP_CHARGE_ENABLED: The hardware is stopped charging,
> the software is still charging；

Please separate it into two patches, one patch adds charging control
with POWER_SUPPLY_PROP_STATUS attribute, and describe the use case in
detail.

The second patch introduces the new POWER_SUPPLY_PROP_CHARGE_ENABLED
attribute with explicit description why you want a new attribute.
Cause I'm still confused about what you want, and your commit message
is useless. If the hardware stopped charging, why does the software
still need to charge?

>   Our  don't want to change the charge_status switch due to the
> switching of charging and discharging of the charging IC in the
> charging scenario of multiple charging ICs, and let the upper layer
> perceive this switching
> > >

snip.