Re: [PATCH V2 14/15] power: supply: axp20x_battery: add support for AXP717

[Chris Morgan <macroalpha82@gmail.com>]

On Sat, Aug 03, 2024 at 12:10:44PM +0100, Jonathan Cameron wrote:
> On Fri,  2 Aug 2024 14:20:25 -0500
> Chris Morgan <macroalpha82@gmail.com> wrote:
> 
> > From: Chris Morgan <macromorgan@hotmail.com>
> > 
> > Add support for the AXP717 PMIC battery charger. The AXP717 differs
> > greatly from existing AXP battery chargers in that it cannot measure
> > the discharge current. The datasheet does not document the current
> > value's offset or scale, so the POWER_SUPPLY_PROP_CURRENT_NOW is left
> > unscaled.
> > 
> > Tested-by: Philippe Simons <simons.philippe@gmail.com>
> > Signed-off-by: Chris Morgan <macromorgan@hotmail.com>
> Hi.
> 
> A few drive by comments,
> 
> Jonathan
> 
> > ---
> >  drivers/power/supply/axp20x_battery.c | 444 ++++++++++++++++++++++++++
> >  1 file changed, 444 insertions(+)
> > 
> > diff --git a/drivers/power/supply/axp20x_battery.c b/drivers/power/supply/axp20x_battery.c
> > index c903c588b361..53af4ad0549d 100644
> > --- a/drivers/power/supply/axp20x_battery.c
> > +++ b/drivers/power/supply/axp20x_battery.c
> > @@ -32,9 +32,19 @@
> >  #include <linux/mfd/axp20x.h>
> >  
> >  #define AXP20X_PWR_STATUS_BAT_CHARGING	BIT(2)
> > +#define AXP717_PWR_STATUS_MASK		GENMASK(6, 5)
> > +#define AXP717_PWR_STATUS_BAT_STANDBY	(0 << 5)
> > +#define AXP717_PWR_STATUS_BAT_CHRG	(1 << 5)
> > +#define AXP717_PWR_STATUS_BAT_DISCHRG	(2 << 5)
> 
> Fine to match local style in this patch, but just thought I'd
> comment that this driver would probably be more readable with
> use of FIELD_PREP and changing convention to not shift the defined
> values for contents of each field.
> 
> To change to that it would either need to be before this patch,
> or done as a follow up.

I'll take your other comments and apply them, but if it's okay with
you I'll opt to not use FIELD_PREP/FIELD_GET for the moment, so the
style remains the same. I will make sure to use those macros for
other drivers I'm working on though as they seem handy.

> 
> 
> >  struct axp20x_batt_ps;
> >  
> > @@ -143,6 +176,41 @@ static int axp22x_battery_get_max_voltage(struct axp20x_batt_ps *axp20x_batt,
> >  	return 0;
> >  }
> >  
> > +static int axp717_battery_get_max_voltage(struct axp20x_batt_ps *axp20x_batt,
> > +					  int *val)
> > +{
> > +	int ret, reg;
> > +
> > +	ret = regmap_read(axp20x_batt->regmap, AXP717_CV_CHG_SET, &reg);
> > +	if (ret)
> > +		return ret;
> > +
> > +	switch (reg & AXP717_CHRG_CV_VOLT_MASK) {
> > +	case AXP717_CHRG_CV_4_0V:
> > +		*val = 4000000;
> > +		break;
> > +	case AXP717_CHRG_CV_4_1V:
> > +		*val = 4100000;
> > +		break;
> > +	case AXP717_CHRG_CV_4_2V:
> > +		*val = 4200000;
> > +		break;
> > +	case AXP717_CHRG_CV_4_35V:
> > +		*val = 4350000;
> > +		break;
> > +	case AXP717_CHRG_CV_4_4V:
> > +		*val = 4400000;
> > +		break;
> > +	case AXP717_CHRG_CV_5_0V:
> > +		*val = 5000000;
> > +		break;
> > +	default:
> > +		return -EINVAL;
> > +	}
> > +
> > +	return 0;
> Could just return instead of breaking an save reader having to look to see
> if anything else happens after the switch finishes.

Acknowledged.

> 
> > +}
> > +
> >  static int axp813_battery_get_max_voltage(struct axp20x_batt_ps *axp20x_batt,
> >  					  int *val)
> >  {
> > @@ -188,6 +256,22 @@ static int axp20x_get_constant_charge_current(struct axp20x_batt_ps *axp,
> >  	return 0;
> >  }
> >  
> > +static int axp717_get_constant_charge_current(struct axp20x_batt_ps *axp,
> > +					      int *val)
> > +{
> > +	int ret;
> > +
> > +	ret = regmap_read(axp->regmap, AXP717_ICC_CHG_SET, val);
> Trivial but I'd use a separate local variable for the register value.  

Will do.

> > +	if (ret)
> > +		return ret;
> > +
> > +	*val &= AXP717_ICC_CHARGER_LIM_MASK;
> 
> FIELD_GET() would be much more readable here as we'd not need to go
> check if LIM_MASK included bit 0 and it could be used directly inline
> with the below as
> 

Nack, if that's okay (as mentioned above). If it's not let me know and
I'll go back and redo this driver.

> 	*val = FIELD_GET(AXP717_IC_CHARGER_LIM_MASK, val) * axp->data->ccc_scale;
> 
> > +
> > +	*val = *val * axp->data->ccc_scale;
> > +
> > +	return 0;
> > +}
> > +
> >  static int axp20x_battery_get_prop(struct power_supply *psy,
> >  				   enum power_supply_property psp,
> >  				   union power_supply_propval *val)
> > @@ -340,6 +424,175 @@ static int axp20x_battery_get_prop(struct power_supply *psy,
> >  	return 0;
> >  }
> >  
> > +static int axp717_battery_get_prop(struct power_supply *psy,
> > +				   enum power_supply_property psp,
> > +				   union power_supply_propval *val)
> > +{
> > +	struct axp20x_batt_ps *axp20x_batt = power_supply_get_drvdata(psy);
> > +	int ret = 0, reg;
> > +
> > +	switch (psp) {
> > +	case POWER_SUPPLY_PROP_PRESENT:
> > +	case POWER_SUPPLY_PROP_ONLINE:
> > +		ret = regmap_read(axp20x_batt->regmap, AXP717_ON_INDICATE,
> > +				  &reg);
> > +		if (ret)
> > +			return ret;
> > +
> > +		val->intval = !!(reg & AXP717_PWR_OP_BATT_PRESENT);
> 
> FIELD_GET() here would be cleaner.
> 
> > +		break;
> > +
> >;
> > +	}
> > +
> > +	return 0;
> 
> As nothing to do down here, I think early returns would make things more redabel.
> 

Will do.

> > +}
> > +
> >  static int axp20x_battery_set_prop(struct power_supply *psy,
> >  				   enum power_supply_property psp,
> >  				   const union power_supply_propval *val)
> > @@ -492,6 +805,42 @@ static int axp20x_battery_set_prop(struct power_supply *psy,
> >  	}
> >  }
> >  
> > +static int axp717_battery_set_prop(struct power_supply *psy,
> > +				   enum power_supply_property psp,
> > +				   const union power_supply_propval *val)
> > +{
> > +	struct axp20x_batt_ps *axp20x_batt = power_supply_get_drvdata(psy);
> > +
> > +	switch (psp) {
> > +	case POWER_SUPPLY_PROP_VOLTAGE_MIN:
> > +		return axp717_set_voltage_min_design(axp20x_batt, val->intval);
> > +
> > +	case POWER_SUPPLY_PROP_VOLTAGE_MAX:
> > +		return axp20x_batt->data->set_max_voltage(axp20x_batt, val->intval);
> > +
> > +	case POWER_SUPPLY_PROP_CONSTANT_CHARGE_CURRENT_MAX:
> > +		return axp717_set_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,
> > +						  AXP717_MODULE_EN_CONTROL_2,
> > +						  AXP717_CHRG_ENABLE,
> > +						  AXP717_CHRG_ENABLE);
> > +
> > +		case POWER_SUPPLY_STATUS_DISCHARGING:
> > +		case POWER_SUPPLY_STATUS_NOT_CHARGING:
> > +			return regmap_update_bits(axp20x_batt->regmap,
> > +						  AXP717_MODULE_EN_CONTROL_2,
> > +						  AXP717_CHRG_ENABLE, 0);
> > +		}
> > +		fallthrough;
> Why bother? Just return -EINVAL here.
> 

Will do.

> > +	default:
> > +		return -EINVAL;
> > +	}
> > +}
> > +

Thank you for your review,
Chris