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@xxxxxxxxx> wrote:
>
> > From: Chris Morgan <macromorgan@xxxxxxxxxxx>
> >
> > 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@xxxxxxxxx>
> > Signed-off-by: Chris Morgan <macromorgan@xxxxxxxxxxx>
> 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, ®);
> > + 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,
> > + ®);
> > + 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
