Received-SPF: pass (google.com: domain of linux-kernel+bounces-32031-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Date: Sun, 21 Jan 2024 12:38:54 +0100
From: =?utf-8?Q?Ond=C5=99ej?= Jirman <megi@xff.cz>
To: Aren Moynihan <aren@peacevolution.org>
Cc: linux-pm@vger.kernel.org, linux-kernel@vger.kernel.org, 
	Aidan MacDonald <aidanmacdonald.0x0@gmail.com>, Chen-Yu Tsai <wens@csie.org>, Sebastian Reichel <sre@kernel.org>
Subject: Re: [PATCH 1/3] power: supply: axp20x_usb_power: add input current
 limit
Message-ID: <lz3sm6yxu3d5pgr2ffs4m3ive54lbiis6abmrph3u5wrav7lqu@qh2kivwzwd6z>
Mail-Followup-To: =?utf-8?Q?Ond=C5=99ej?= Jirman <megi@xff.cz>, 
	Aren Moynihan <aren@peacevolution.org>, linux-pm@vger.kernel.org, linux-kernel@vger.kernel.org, 
	Aidan MacDonald <aidanmacdonald.0x0@gmail.com>, Chen-Yu Tsai <wens@csie.org>, Sebastian Reichel <sre@kernel.org>
References: <20240121014057.1042466-1-aren@peacevolution.org>
 <20240121014057.1042466-3-aren@peacevolution.org>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20240121014057.1042466-3-aren@peacevolution.org>

Hello Aren,

On Sat, Jan 20, 2024 at 08:40:00PM -0500, Aren Moynihan 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.
> 
> 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/supply/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_struct *work)
>  	if (val != power->old_status)
>  		power_supply_changed(power->supply);
>  
> +	if (power->usb_bc_en_bit && (val & AXP20X_PWR_STATUS_VBUS_PRESENT) !=
> +		(power->old_status & AXP20X_PWR_STATUS_VBUS_PRESENT)) {
> +		dev_dbg(power->dev, "Cable status changed, re-enabling USB BC");
> +		ret = regmap_field_write(power->usb_bc_en_bit, 1);
> +		if (ret)
> +			dev_err(power->dev, "failed to enable USB BC: errno %d",
> +				ret);
> +	}
> +
>  	power->old_status = val;
>  	power->online = val & AXP20X_PWR_STATUS_VBUS_USED;
>  
> @@ -123,6 +137,66 @@ static void axp20x_usb_power_poll_vbus(struct work_struct *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;

I think that you should just set the lowest possible limit. A caller (calling
driver or userspace or user) has no way to identify what is the lowest possible
limit on arbitrary PMIC and I kinda like having an option to
echo 0 > .../axp20x-usb/input_current_limit as a way to limit power consumption from
USB to a minimum without having to look up what actual minimum is in various
PMICs dataheets.

I looked through most of the uses of POWER_SUPPLY_PROP_INPUT_CURRENT_LIMIT in
the upstream drivers and both approaches are used. Erroring out when out of range
is less common.

Otherwise,

Reviewed-By: Ondrej Jirman <megi@xff.cz>

Thank you,
	Ondrej