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

[Jonathan Cameron <jic23@kernel.org>]

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.


>  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.

> +}
> +
>  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.  
> +	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

	*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.

> +}
> +
>  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.

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