diff mbox series

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

Message ID 20240802192026.446344-15-macroalpha82@gmail.com (mailing list archive)
State New, archived
Headers show
Series Add Battery and USB Supply for AXP717 | expand

Commit Message

Chris Morgan Aug. 2, 2024, 7:20 p.m. UTC
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>
---
 drivers/power/supply/axp20x_battery.c | 444 ++++++++++++++++++++++++++
 1 file changed, 444 insertions(+)

Comments

Jonathan Cameron Aug. 3, 2024, 11:10 a.m. UTC | #1
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;
> +	}
> +}
> +
Chris Morgan Aug. 14, 2024, 9:13 p.m. UTC | #2
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
Jonathan Cameron Aug. 17, 2024, 9:59 a.m. UTC | #3
On Wed, 14 Aug 2024 16:13:01 -0500
Chris Morgan <macroalpha82@gmail.com> wrote:

> 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.
That's fine, though if you are in a position to test, it would be good to follow
up with a tidy up of this driver as that style is much more compact and
helps with maintenance longer term.

Jonathan
diff mbox series

Patch

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)
 
 #define AXP20X_PWR_OP_BATT_PRESENT	BIT(5)
 #define AXP20X_PWR_OP_BATT_ACTIVATED	BIT(3)
+#define AXP717_PWR_OP_BATT_PRESENT	BIT(3)
+
+#define AXP717_BATT_PMU_FAULT_MASK	GENMASK(2, 0)
+#define AXP717_BATT_UVLO_2_5V		(1 << 2)
+#define AXP717_BATT_OVER_TEMP		(1 << 1)
+#define AXP717_BATT_UNDER_TEMP		(1 << 0)
 
 #define AXP209_FG_PERCENT		GENMASK(6, 0)
 #define AXP22X_FG_VALID			BIT(7)
@@ -49,11 +59,34 @@ 
 #define AXP22X_CHRG_CTRL1_TGT_4_22V	(1 << 5)
 #define AXP22X_CHRG_CTRL1_TGT_4_24V	(3 << 5)
 
+#define AXP717_CHRG_ENABLE		BIT(1)
+#define AXP717_CHRG_CV_VOLT_MASK	GENMASK(2, 0)
+#define AXP717_CHRG_CV_4_0V		0
+#define AXP717_CHRG_CV_4_1V		1
+#define AXP717_CHRG_CV_4_2V		2
+#define AXP717_CHRG_CV_4_35V		3
+#define AXP717_CHRG_CV_4_4V		4
+/* Values 5 and 6 reserved. */
+#define AXP717_CHRG_CV_5_0V		7
+
 #define AXP813_CHRG_CTRL1_TGT_4_35V	(3 << 5)
 
 #define AXP20X_CHRG_CTRL1_TGT_CURR	GENMASK(3, 0)
+#define AXP717_ICC_CHARGER_LIM_MASK	GENMASK(5, 0)
+
+#define AXP717_ITERM_CHG_LIM_MASK	GENMASK(3, 0)
+#define AXP717_ITERM_CC_STEP		64000
 
 #define AXP20X_V_OFF_MASK		GENMASK(2, 0)
+#define AXP717_V_OFF_MASK		GENMASK(6, 4)
+
+#define AXP717_BAT_VMIN_MIN_UV		2600000
+#define AXP717_BAT_VMIN_MAX_UV		3300000
+#define AXP717_BAT_VMIN_STEP		100000
+#define AXP717_BAT_CV_MIN_UV		4000000
+#define AXP717_BAT_CV_MAX_UV		5000000
+#define AXP717_BAT_CC_MIN_UA		0
+#define AXP717_BAT_CC_MAX_UA		3008000
 
 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;
+}
+
 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);
+	if (ret)
+		return ret;
+
+	*val &= AXP717_ICC_CHARGER_LIM_MASK;
+
+	*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);
+		break;
+
+	case POWER_SUPPLY_PROP_STATUS:
+		ret = regmap_read(axp20x_batt->regmap, AXP717_PMU_STATUS_2,
+				  &reg);
+		if (ret)
+			return ret;
+
+		switch (reg & AXP717_PWR_STATUS_MASK) {
+		case AXP717_PWR_STATUS_BAT_STANDBY:
+			val->intval = POWER_SUPPLY_STATUS_NOT_CHARGING;
+			break;
+
+		case AXP717_PWR_STATUS_BAT_CHRG:
+			val->intval = POWER_SUPPLY_STATUS_CHARGING;
+			break;
+
+		case AXP717_PWR_STATUS_BAT_DISCHRG:
+			val->intval = POWER_SUPPLY_STATUS_DISCHARGING;
+			break;
+
+		default:
+			val->intval = POWER_SUPPLY_STATUS_UNKNOWN;
+		}
+
+		break;
+
+	/*
+	 * If a fault is detected it must also be cleared; if the
+	 * condition persists it should reappear (This is an
+	 * assumption, it's actually not documented). A restart was
+	 * not sufficient to clear the bit in testing despite the
+	 * register listed as POR.
+	 */
+	case POWER_SUPPLY_PROP_HEALTH:
+		ret = regmap_read(axp20x_batt->regmap, AXP717_PMU_FAULT,
+				  &reg);
+		if (ret)
+			return ret;
+
+		switch (reg & AXP717_BATT_PMU_FAULT_MASK) {
+		case AXP717_BATT_UVLO_2_5V:
+			val->intval = POWER_SUPPLY_HEALTH_DEAD;
+			regmap_update_bits(axp20x_batt->regmap,
+					   AXP717_PMU_FAULT,
+					   AXP717_BATT_UVLO_2_5V,
+					   AXP717_BATT_UVLO_2_5V);
+			break;
+
+		case AXP717_BATT_OVER_TEMP:
+			val->intval = POWER_SUPPLY_HEALTH_HOT;
+			regmap_update_bits(axp20x_batt->regmap,
+					   AXP717_PMU_FAULT,
+					   AXP717_BATT_OVER_TEMP,
+					   AXP717_BATT_OVER_TEMP);
+			break;
+
+		case AXP717_BATT_UNDER_TEMP:
+			val->intval = POWER_SUPPLY_HEALTH_COLD;
+			regmap_update_bits(axp20x_batt->regmap,
+					   AXP717_PMU_FAULT,
+					   AXP717_BATT_UNDER_TEMP,
+					   AXP717_BATT_UNDER_TEMP);
+			break;
+
+		default:
+			val->intval = POWER_SUPPLY_HEALTH_GOOD;
+		}
+
+		break;
+
+	case POWER_SUPPLY_PROP_CONSTANT_CHARGE_CURRENT_MAX:
+		ret = axp717_get_constant_charge_current(axp20x_batt,
+							 &val->intval);
+		if (ret)
+			return ret;
+		break;
+
+	case POWER_SUPPLY_PROP_CURRENT_NOW:
+		/*
+		 * The offset of this value is currently unknown and is
+		 * not documented in the datasheet. Based on
+		 * observation it's assumed to be somewhere around
+		 * 450ma. I will leave the value raw for now.
+		 */
+		ret = iio_read_channel_processed(axp20x_batt->batt_chrg_i, &val->intval);
+		if (ret)
+			return ret;
+		/* IIO framework gives mA but Power Supply framework gives uA */
+		val->intval *= 1000;
+		break;
+
+	case POWER_SUPPLY_PROP_CAPACITY:
+		ret = regmap_read(axp20x_batt->regmap, AXP717_ON_INDICATE,
+				  &reg);
+		if (ret)
+			return ret;
+
+		if (!(reg & AXP717_PWR_OP_BATT_PRESENT))
+			return -ENODEV;
+
+		ret = regmap_read(axp20x_batt->regmap,
+				  AXP717_BATT_PERCENT_DATA, &reg);
+		if (ret)
+			return ret;
+
+		/*
+		 * Fuel Gauge data takes 7 bits but the stored value seems to be
+		 * directly the raw percentage without any scaling to 7 bits.
+		 */
+		val->intval = reg & AXP209_FG_PERCENT;
+		break;
+
+	case POWER_SUPPLY_PROP_VOLTAGE_MAX:
+		return axp20x_batt->data->get_max_voltage(axp20x_batt,
+							  &val->intval);
+
+	case POWER_SUPPLY_PROP_VOLTAGE_MIN:
+		ret = regmap_read(axp20x_batt->regmap,
+				  AXP717_VSYS_V_POWEROFF, &reg);
+		if (ret)
+			return ret;
+
+		val->intval = AXP717_BAT_VMIN_MIN_UV + AXP717_BAT_VMIN_STEP *
+			(reg & AXP717_V_OFF_MASK);
+		break;
+
+	case POWER_SUPPLY_PROP_VOLTAGE_NOW:
+		ret = iio_read_channel_processed(axp20x_batt->batt_v,
+						 &val->intval);
+		if (ret)
+			return ret;
+
+		/* IIO framework gives mV but Power Supply framework gives uV */
+		val->intval *= 1000;
+		break;
+
+	case POWER_SUPPLY_PROP_CHARGE_TERM_CURRENT:
+		ret = regmap_read(axp20x_batt->regmap,
+				  AXP717_ITERM_CHG_SET, &reg);
+		if (ret)
+			return ret;
+
+		val->intval = (reg & AXP717_ITERM_CHG_LIM_MASK) * AXP717_ITERM_CC_STEP;
+		break;
+
+	default:
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
 static int axp22x_battery_set_max_voltage(struct axp20x_batt_ps *axp20x_batt,
 					  int val)
 {
@@ -396,6 +649,35 @@  static int axp20x_battery_set_max_voltage(struct axp20x_batt_ps *axp20x_batt,
 				  AXP20X_CHRG_CTRL1_TGT_VOLT, val);
 }
 
+static int axp717_battery_set_max_voltage(struct axp20x_batt_ps *axp20x_batt,
+					  int val)
+{
+	switch (val) {
+	case 4000000:
+		val = AXP717_CHRG_CV_4_0V;
+		break;
+
+	case 4100000:
+		val = AXP717_CHRG_CV_4_1V;
+		break;
+
+	case 4200000:
+		val = AXP717_CHRG_CV_4_2V;
+		break;
+
+	default:
+		/*
+		 * AXP717 can go up to 4.35, 4.4, and 5.0 volts which
+		 * seem too high for lithium batteries, so do not allow.
+		 */
+		return -EINVAL;
+	}
+
+	return regmap_update_bits(axp20x_batt->regmap,
+				  AXP717_CV_CHG_SET,
+				  AXP717_CHRG_CV_VOLT_MASK, val);
+}
+
 static int axp20x_set_constant_charge_current(struct axp20x_batt_ps *axp_batt,
 					      int charge_current)
 {
@@ -412,6 +694,24 @@  static int axp20x_set_constant_charge_current(struct axp20x_batt_ps *axp_batt,
 				  AXP20X_CHRG_CTRL1_TGT_CURR, charge_current);
 }
 
+static int axp717_set_constant_charge_current(struct axp20x_batt_ps *axp,
+					      int charge_current)
+{
+	int val;
+
+	if (charge_current > axp->max_ccc)
+		return -EINVAL;
+
+	if (charge_current > AXP717_BAT_CC_MAX_UA || charge_current < 0)
+		return -EINVAL;
+
+	val = (charge_current - axp->data->ccc_offset) /
+		axp->data->ccc_scale;
+
+	return regmap_update_bits(axp->regmap, AXP717_ICC_CHG_SET,
+				  AXP717_ICC_CHARGER_LIM_MASK, val);
+}
+
 static int axp20x_set_max_constant_charge_current(struct axp20x_batt_ps *axp,
 						  int charge_current)
 {
@@ -456,6 +756,19 @@  static int axp20x_set_voltage_min_design(struct axp20x_batt_ps *axp_batt,
 				  AXP20X_V_OFF_MASK, val1);
 }
 
+static int axp717_set_voltage_min_design(struct axp20x_batt_ps *axp_batt,
+					 int min_voltage)
+{
+	int val1 = (min_voltage - AXP717_BAT_VMIN_MIN_UV) / AXP717_BAT_VMIN_STEP;
+
+	if (val1 < 0 || val1 > AXP717_V_OFF_MASK)
+		return -EINVAL;
+
+	return regmap_update_bits(axp_batt->regmap,
+				  AXP717_VSYS_V_POWEROFF,
+				  AXP717_V_OFF_MASK, val1);
+}
+
 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;
+	default:
+		return -EINVAL;
+	}
+}
+
 static enum power_supply_property axp20x_battery_props[] = {
 	POWER_SUPPLY_PROP_PRESENT,
 	POWER_SUPPLY_PROP_ONLINE,
@@ -506,6 +855,20 @@  static enum power_supply_property axp20x_battery_props[] = {
 	POWER_SUPPLY_PROP_CAPACITY,
 };
 
+static enum power_supply_property axp717_battery_props[] = {
+	POWER_SUPPLY_PROP_PRESENT,
+	POWER_SUPPLY_PROP_ONLINE,
+	POWER_SUPPLY_PROP_STATUS,
+	POWER_SUPPLY_PROP_VOLTAGE_NOW,
+	POWER_SUPPLY_PROP_CURRENT_NOW,
+	POWER_SUPPLY_PROP_CONSTANT_CHARGE_CURRENT_MAX,
+	POWER_SUPPLY_PROP_HEALTH,
+	POWER_SUPPLY_PROP_VOLTAGE_MAX,
+	POWER_SUPPLY_PROP_VOLTAGE_MIN,
+	POWER_SUPPLY_PROP_CAPACITY,
+	POWER_SUPPLY_PROP_CHARGE_TERM_CURRENT,
+};
+
 static int axp20x_battery_prop_writeable(struct power_supply *psy,
 					 enum power_supply_property psp)
 {
@@ -516,6 +879,15 @@  static int axp20x_battery_prop_writeable(struct power_supply *psy,
 	       psp == POWER_SUPPLY_PROP_CONSTANT_CHARGE_CURRENT_MAX;
 }
 
+static int axp717_battery_prop_writeable(struct power_supply *psy,
+					 enum power_supply_property psp)
+{
+	return psp == POWER_SUPPLY_PROP_STATUS ||
+	       psp == POWER_SUPPLY_PROP_VOLTAGE_MIN ||
+	       psp == POWER_SUPPLY_PROP_VOLTAGE_MAX ||
+	       psp == POWER_SUPPLY_PROP_CONSTANT_CHARGE_CURRENT_MAX;
+}
+
 static const struct power_supply_desc axp209_batt_ps_desc = {
 	.name = "axp20x-battery",
 	.type = POWER_SUPPLY_TYPE_BATTERY,
@@ -526,6 +898,16 @@  static const struct power_supply_desc axp209_batt_ps_desc = {
 	.set_property = axp20x_battery_set_prop,
 };
 
+static const struct power_supply_desc axp717_batt_ps_desc = {
+	.name = "axp20x-battery",
+	.type = POWER_SUPPLY_TYPE_BATTERY,
+	.properties = axp717_battery_props,
+	.num_properties = ARRAY_SIZE(axp717_battery_props),
+	.property_is_writeable = axp717_battery_prop_writeable,
+	.get_property = axp717_battery_get_prop,
+	.set_property = axp717_battery_set_prop,
+};
+
 static int axp209_bat_cfg_iio_channels(struct platform_device *pdev,
 				       struct axp20x_batt_ps *axp_batt)
 {
@@ -555,6 +937,27 @@  static int axp209_bat_cfg_iio_channels(struct platform_device *pdev,
 	return 0;
 }
 
+static int axp717_bat_cfg_iio_channels(struct platform_device *pdev,
+				       struct axp20x_batt_ps *axp_batt)
+{
+	axp_batt->batt_v = devm_iio_channel_get(&pdev->dev, "batt_v");
+	if (IS_ERR(axp_batt->batt_v)) {
+		if (PTR_ERR(axp_batt->batt_v) == -ENODEV)
+			return -EPROBE_DEFER;
+		return PTR_ERR(axp_batt->batt_v);
+	}
+
+	axp_batt->batt_chrg_i = devm_iio_channel_get(&pdev->dev,
+							"batt_chrg_i");
+	if (IS_ERR(axp_batt->batt_chrg_i)) {
+		if (PTR_ERR(axp_batt->batt_chrg_i) == -ENODEV)
+			return -EPROBE_DEFER;
+		return PTR_ERR(axp_batt->batt_chrg_i);
+	}
+
+	return 0;
+}
+
 static void axp209_set_battery_info(struct platform_device *pdev,
 				    struct axp20x_batt_ps *axp_batt,
 				    struct power_supply_battery_info *info)
@@ -578,6 +981,32 @@  static void axp209_set_battery_info(struct platform_device *pdev,
 	}
 }
 
+static void axp717_set_battery_info(struct platform_device *pdev,
+				    struct axp20x_batt_ps *axp_batt,
+				    struct power_supply_battery_info *info)
+{
+	int vmin = info->voltage_min_design_uv;
+	int vmax = info->voltage_max_design_uv;
+	int ccc = info->constant_charge_current_max_ua;
+	int val;
+
+	if (vmin > 0 && axp717_set_voltage_min_design(axp_batt, vmin))
+		dev_err(&pdev->dev,
+			"couldn't set voltage_min_design\n");
+
+	if (vmax > 0 && axp717_battery_set_max_voltage(axp_batt, vmax))
+		dev_err(&pdev->dev,
+			"couldn't set voltage_max_design\n");
+
+	axp717_get_constant_charge_current(axp_batt, &val);
+	axp_batt->max_ccc = ccc;
+	if (ccc <= 0 || axp717_set_constant_charge_current(axp_batt, ccc)) {
+		dev_err(&pdev->dev,
+			"couldn't set ccc from DT: current ccc is %d\n",
+			val);
+	}
+}
+
 static const struct axp_data axp209_data = {
 	.ccc_scale = 100000,
 	.ccc_offset = 300000,
@@ -603,6 +1032,18 @@  static const struct axp_data axp221_data = {
 	.set_bat_info = axp209_set_battery_info,
 };
 
+static const struct axp_data axp717_data = {
+	.ccc_scale = 64000,
+	.ccc_offset = 0,
+	.ccc_reg = AXP717_ICC_CHG_SET,
+	.ccc_mask = AXP717_ICC_CHARGER_LIM_MASK,
+	.bat_ps_desc = &axp717_batt_ps_desc,
+	.get_max_voltage = axp717_battery_get_max_voltage,
+	.set_max_voltage = axp717_battery_set_max_voltage,
+	.cfg_iio_chan = axp717_bat_cfg_iio_channels,
+	.set_bat_info = axp717_set_battery_info,
+};
+
 static const struct axp_data axp813_data = {
 	.ccc_scale = 200000,
 	.ccc_offset = 200000,
@@ -623,6 +1064,9 @@  static const struct of_device_id axp20x_battery_ps_id[] = {
 	}, {
 		.compatible = "x-powers,axp221-battery-power-supply",
 		.data = (void *)&axp221_data,
+	}, {
+		.compatible = "x-powers,axp717-battery-power-supply",
+		.data = (void *)&axp717_data,
 	}, {
 		.compatible = "x-powers,axp813-battery-power-supply",
 		.data = (void *)&axp813_data,