Re: Re: [PATCH 1/3] power: supply: axp20x_usb_power: add input current limit

[Aren]

On Sun, Jan 21, 2024 at 12:38:54PM +0100, Ondřej Jirman wrote:
> 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@xxxxxxxxxxxxxxxxx>
> > ---
> > 
> >  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.

That should be pretty easy, I love a good reason to delete code :)

Thanks
 - Aren

> Otherwise,
> 
> Reviewed-By: Ondrej Jirman <megi@xxxxxx>
> 
> Thank you,
> 	Ondrej