Received-SPF: pass (google.com: domain of linux-kernel+bounces-32084-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Precedence: bulk
MIME-Version: 1.0
References: <20240121014057.1042466-1-aren@peacevolution.org> <20240121014057.1042466-3-aren@peacevolution.org>
In-Reply-To: <20240121014057.1042466-3-aren@peacevolution.org>
Reply-To: wens@csie.org
From: Chen-Yu Tsai <wens@csie.org>
Date: Sun, 21 Jan 2024 23:25:04 +0800
Message-ID: <CAGb2v65QcP1bcT++51n7Ada6QvFmrQzqiA-b3tbzfAr3XC6XJQ@mail.gmail.com>
Subject: Re: [PATCH 1/3] power: supply: axp20x_usb_power: add input current limit
To: Aren Moynihan <aren@peacevolution.org>
Cc: linux-pm@vger.kernel.org, linux-kernel@vger.kernel.org, 
	Aidan MacDonald <aidanmacdonald.0x0@gmail.com>, =?UTF-8?Q?Ond=C5=99ej_Jirman?= <megi@xff.cz>, 
	Sebastian Reichel <sre@kernel.org>, Hans de Goede <j.w.r.degoede@gmail.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

Hi,

+CC Hans de Goede since the AXP288 is similar to the later AXPs in the USB
power section.

On Sun, Jan 21, 2024 at 9:54=E2=80=AFAM Aren Moynihan <aren@peacevolution.o=
rg> wrote:
>
> Add properties for setting the maximum current that will be drawn from
> the usb connection.
>
> These changes don't apply to all axp20x chips, so we need to add new
> power_desc and power supply property objects for axp813 specifically.
> These are copied from the axp22x variants that were used before, with
> extra fields added.
>
> Also add a dev field to the axp20x_usb_power struct, so we can use
> dev_dbg and dev_err in more places.

I think this patch highlights some errors in the driver.

1. We are likely misusing POWER_SUPPLY_PROP_CURRENT_MAX. Based on the
   ABI docs, this is supposed to be a read only property. The correct
   way would be to use POWER_SUPPLY_PROP_INPUT_CURRENT_LIMIT as you did.

2. For AXP288 and AXP803/AXP813/AXP818, we are setting the current limit
   for when BC detection finishes in register 0x30, not the actual active
   limit in register 0x35. This should be fixed.

3. BC detection race condition

Could you maybe respin the patches to address them separately instead?


Thanks
ChenYu


> Signed-off-by: Aren Moynihan <aren@peacevolution.org>
> ---
>
>  drivers/power/supply/axp20x_usb_power.c | 127 +++++++++++++++++++++++-
>  1 file changed, 125 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/power/supply/axp20x_usb_power.c b/drivers/power/supp=
ly/axp20x_usb_power.c
> index e23308ad4cc7..8c0c2c25565f 100644
> --- a/drivers/power/supply/axp20x_usb_power.c
> +++ b/drivers/power/supply/axp20x_usb_power.c
> @@ -50,7 +50,10 @@ struct axp_data {
>         const char * const              *irq_names;
>         unsigned int                    num_irq_names;
>         const int                       *curr_lim_table;
> +       int                             input_curr_lim_table_size;
> +       const int                       *input_curr_lim_table;
>         struct reg_field                curr_lim_fld;
> +       struct reg_field                input_curr_lim_fld;
>         struct reg_field                vbus_valid_bit;
>         struct reg_field                vbus_mon_bit;
>         struct reg_field                usb_bc_en_bit;
> @@ -59,7 +62,9 @@ struct axp_data {
>  };
>
>  struct axp20x_usb_power {
> +       struct device *dev;
>         struct regmap *regmap;
> +       struct regmap_field *input_curr_lim_fld;
>         struct regmap_field *curr_lim_fld;
>         struct regmap_field *vbus_valid_bit;
>         struct regmap_field *vbus_mon_bit;
> @@ -115,6 +120,15 @@ static void axp20x_usb_power_poll_vbus(struct work_s=
truct *work)
>         if (val !=3D power->old_status)
>                 power_supply_changed(power->supply);
>
> +       if (power->usb_bc_en_bit && (val & AXP20X_PWR_STATUS_VBUS_PRESENT=
) !=3D
> +               (power->old_status & AXP20X_PWR_STATUS_VBUS_PRESENT)) {
> +               dev_dbg(power->dev, "Cable status changed, re-enabling US=
B BC");
> +               ret =3D regmap_field_write(power->usb_bc_en_bit, 1);
> +               if (ret)
> +                       dev_err(power->dev, "failed to enable USB BC: err=
no %d",
> +                               ret);
> +       }
> +
>         power->old_status =3D val;
>         power->online =3D val & AXP20X_PWR_STATUS_VBUS_USED;
>
> @@ -123,6 +137,66 @@ static void axp20x_usb_power_poll_vbus(struct work_s=
truct *work)
>                 mod_delayed_work(system_power_efficient_wq, &power->vbus_=
detect, DEBOUNCE_TIME);
>  }
>
> +static int
> +axp20x_usb_power_set_input_current_limit(struct axp20x_usb_power *power,
> +                                        int limit)
> +{
> +       int ret;
> +       unsigned int reg;
> +
> +       if (!power->axp_data->input_curr_lim_table)
> +               return -EINVAL;
> +
> +       if (limit < power->axp_data->input_curr_lim_table[0])
> +               return -EINVAL;
> +
> +       /*
> +        * BC1.2 detection can cause a race condition if we try to set a =
current
> +        * limit while it's in progress. When it finishes it will overwri=
te the
> +        * current limit we just set.
> +        */
> +       if (power->usb_bc_en_bit) {
> +               dev_dbg(power->dev,
> +                       "disabling BC1.2 detection because current limit =
was set");
> +               ret =3D regmap_field_write(power->usb_bc_en_bit, 0);
> +               if (ret)
> +                       return ret;
> +       }
> +
> +       for (reg =3D power->axp_data->input_curr_lim_table_size - 1; reg =
> 0; reg--) {
> +               if (limit >=3D power->axp_data->input_curr_lim_table[reg]=
)
> +                       break;
> +       }
> +
> +       dev_dbg(power->dev, "setting input current limit reg to %d (%d uA=
), requested %d uA",
> +               reg, power->axp_data->input_curr_lim_table[reg], limit);
> +
> +       return regmap_field_write(power->input_curr_lim_fld, reg);
> +}
> +
> +static int
> +axp20x_usb_power_get_input_current_limit(struct axp20x_usb_power *power,
> +                                        int *intval)
> +{
> +       int ret;
> +       unsigned int v;
> +
> +       if (!power->axp_data->input_curr_lim_table)
> +               return -EINVAL;
> +
> +       ret =3D regmap_field_read(power->input_curr_lim_fld, &v);
> +       if (ret)
> +               return ret;
> +
> +       if (v < power->axp_data->input_curr_lim_table_size)
> +               *intval =3D power->axp_data->input_curr_lim_table[v];
> +       else
> +               *intval =3D power->axp_data->input_curr_lim_table[
> +                         power->axp_data->input_curr_lim_table_size - 1]=
;
> +
> +       return 0;
> +}
> +
>  static int axp20x_usb_power_get_property(struct power_supply *psy,
>         enum power_supply_property psp, union power_supply_propval *val)
>  {
> @@ -189,6 +263,9 @@ static int axp20x_usb_power_get_property(struct power=
_supply *psy,
>
>                 val->intval =3D ret * 375; /* 1 step =3D 0.375 mA */
>                 return 0;
> +       case POWER_SUPPLY_PROP_INPUT_CURRENT_LIMIT:
> +               return axp20x_usb_power_get_input_current_limit(power,
> +                                                               &val->int=
val);
>         default:
>                 break;
>         }
> @@ -290,6 +367,10 @@ static int axp20x_usb_power_set_property(struct powe=
r_supply *psy,
>         case POWER_SUPPLY_PROP_CURRENT_MAX:
>                 return axp20x_usb_power_set_current_max(power, val->intva=
l);
>
> +       case POWER_SUPPLY_PROP_INPUT_CURRENT_LIMIT:
> +               return axp20x_usb_power_set_input_current_limit(power,
> +                                                               val->intv=
al);
> +
>         default:
>                 return -EINVAL;
>         }
> @@ -313,7 +394,8 @@ static int axp20x_usb_power_prop_writeable(struct pow=
er_supply *psy,
>                 return power->vbus_disable_bit !=3D NULL;
>
>         return psp =3D=3D POWER_SUPPLY_PROP_VOLTAGE_MIN ||
> -              psp =3D=3D POWER_SUPPLY_PROP_CURRENT_MAX;
> +              psp =3D=3D POWER_SUPPLY_PROP_CURRENT_MAX ||
> +              psp =3D=3D POWER_SUPPLY_PROP_INPUT_CURRENT_LIMIT;
>  }
>
>  static enum power_supply_property axp20x_usb_power_properties[] =3D {
> @@ -334,6 +416,15 @@ static enum power_supply_property axp22x_usb_power_p=
roperties[] =3D {
>         POWER_SUPPLY_PROP_CURRENT_MAX,
>  };
>
> +static enum power_supply_property axp813_usb_power_properties[] =3D {
> +       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 =3D {
>         .name =3D "axp20x-usb",
>         .type =3D POWER_SUPPLY_TYPE_USB,
> @@ -354,6 +445,16 @@ static const struct power_supply_desc axp22x_usb_pow=
er_desc =3D {
>         .set_property =3D axp20x_usb_power_set_property,
>  };
>
> +static const struct power_supply_desc axp813_usb_power_desc =3D {
> +       .name =3D "axp20x-usb",
> +       .type =3D POWER_SUPPLY_TYPE_USB,
> +       .properties =3D axp813_usb_power_properties,
> +       .num_properties =3D ARRAY_SIZE(axp813_usb_power_properties),
> +       .property_is_writeable =3D axp20x_usb_power_prop_writeable,
> +       .get_property =3D axp20x_usb_power_get_property,
> +       .set_property =3D axp20x_usb_power_set_property,
> +};
> +
>  static const char * const axp20x_irq_names[] =3D {
>         "VBUS_PLUGIN",
>         "VBUS_REMOVAL",
> @@ -394,6 +495,18 @@ static int axp813_usb_curr_lim_table[] =3D {
>         2500000,
>  };
>
> +static int axp_813_usb_input_curr_lim_table[] =3D {
> +       100000,
> +       500000,
> +       900000,
> +       1500000,
> +       2000000,
> +       2500000,
> +       3000000,
> +       3500000,
> +       4000000,
> +};
> +
>  static const struct axp_data axp192_data =3D {
>         .power_desc     =3D &axp20x_usb_power_desc,
>         .irq_names      =3D axp20x_irq_names,
> @@ -433,11 +546,14 @@ static const struct axp_data axp223_data =3D {
>  };
>
>  static const struct axp_data axp813_data =3D {
> -       .power_desc     =3D &axp22x_usb_power_desc,
> +       .power_desc     =3D &axp813_usb_power_desc,
>         .irq_names      =3D axp22x_irq_names,
>         .num_irq_names  =3D ARRAY_SIZE(axp22x_irq_names),
>         .curr_lim_table =3D axp813_usb_curr_lim_table,
>         .curr_lim_fld   =3D REG_FIELD(AXP20X_VBUS_IPSOUT_MGMT, 0, 1),
> +       .input_curr_lim_table_size =3D ARRAY_SIZE(axp_813_usb_input_curr_=
lim_table),
> +       .input_curr_lim_table =3D axp_813_usb_input_curr_lim_table,
> +       .input_curr_lim_fld =3D REG_FIELD(AXP22X_CHRG_CTRL3, 4, 7),
>         .usb_bc_en_bit  =3D REG_FIELD(AXP288_BC_GLOBAL, 0, 0),
>         .vbus_disable_bit =3D REG_FIELD(AXP20X_VBUS_IPSOUT_MGMT, 7, 7),
>         .vbus_needs_polling =3D true,
> @@ -558,6 +674,7 @@ static int axp20x_usb_power_probe(struct platform_dev=
ice *pdev)
>
>         platform_set_drvdata(pdev, power);
>
> +       power->dev =3D &pdev->dev;
>         power->axp_data =3D axp_data;
>         power->regmap =3D axp20x->regmap;
>         power->num_irqs =3D axp_data->num_irq_names;
> @@ -567,6 +684,12 @@ static int axp20x_usb_power_probe(struct platform_de=
vice *pdev)
>         if (IS_ERR(power->curr_lim_fld))
>                 return PTR_ERR(power->curr_lim_fld);
>
> +       ret =3D axp20x_regmap_field_alloc_optional(&pdev->dev, power->reg=
map,
> +                                                axp_data->input_curr_lim=
_fld,
> +                                                &power->input_curr_lim_f=
ld);
> +       if (ret)
> +               return ret;
> +
>         ret =3D axp20x_regmap_field_alloc_optional(&pdev->dev, power->reg=
map,
>                                                  axp_data->vbus_valid_bit=
,
>                                                  &power->vbus_valid_bit);
> --
> 2.43.0
>