| Message ID | 20200105012416.23296-3-samuel@sholland.org (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
| Series | X-Powers Power Supply Improvements | expand |
On Sun, Jan 5, 2020 at 9:24 AM Samuel Holland <samuel@sholland.org> wrote: > > AXP803/AXP813 have a flag that enables/disables the AC power supply > input. This flag does not affect the status bits in PWR_INPUT_STATUS. > Its effect can be verified by checking the battery charge/discharge > state (bit 2 of PWR_INPUT_STATUS), or by examining the current draw on > the AC input. > > Take this flag into account when getting the ONLINE property of the AC > input, on PMICs where this flag is present. > > Fixes: 7693b5643fd2 ("power: supply: add AC power supply driver for AXP813") > Cc: stable@vger.kernel.org > Signed-off-by: Samuel Holland <samuel@sholland.org> Reviewed-by: Chen-Yu Tsai <wens@csie.org>
Hi Samuel, On Sun, Jan 5, 2020 at 12:24 PM Samuel Holland <samuel@sholland.org> wrote: > > AXP803/AXP813 have a flag that enables/disables the AC power supply > input. This flag does not affect the status bits in PWR_INPUT_STATUS. > Its effect can be verified by checking the battery charge/discharge > state (bit 2 of PWR_INPUT_STATUS), or by examining the current draw on > the AC input. > > Take this flag into account when getting the ONLINE property of the AC > input, on PMICs where this flag is present. > > Fixes: 7693b5643fd2 ("power: supply: add AC power supply driver for AXP813") > Cc: stable@vger.kernel.org > Signed-off-by: Samuel Holland <samuel@sholland.org> > --- > drivers/power/supply/axp20x_ac_power.c | 31 +++++++++++++++++++++----- > 1 file changed, 25 insertions(+), 6 deletions(-) > > diff --git a/drivers/power/supply/axp20x_ac_power.c b/drivers/power/supply/axp20x_ac_power.c > index 0d34a932b6d5..ca0a28f72a27 100644 > --- a/drivers/power/supply/axp20x_ac_power.c > +++ b/drivers/power/supply/axp20x_ac_power.c > @@ -23,6 +23,8 @@ > #define AXP20X_PWR_STATUS_ACIN_PRESENT BIT(7) > #define AXP20X_PWR_STATUS_ACIN_AVAIL BIT(6) > > +#define AXP813_ACIN_PATH_SEL BIT(7) > + > #define AXP813_VHOLD_MASK GENMASK(5, 3) > #define AXP813_VHOLD_UV_TO_BIT(x) ((((x) / 100000) - 40) << 3) > #define AXP813_VHOLD_REG_TO_UV(x) \ > @@ -40,6 +42,7 @@ struct axp20x_ac_power { > struct power_supply *supply; > struct iio_channel *acin_v; > struct iio_channel *acin_i; > + bool has_acin_path_sel; > }; > > static irqreturn_t axp20x_ac_power_irq(int irq, void *devid) > @@ -86,6 +89,17 @@ static int axp20x_ac_power_get_property(struct power_supply *psy, > return ret; > > val->intval = !!(reg & AXP20X_PWR_STATUS_ACIN_AVAIL); > + > + /* ACIN_PATH_SEL disables ACIN even if ACIN_AVAIL is set. */ > + if (power->has_acin_path_sel) { Do we need to check this bit if ACIN_AVAIL is not set? > + ret = regmap_read(power->regmap, AXP813_ACIN_PATH_CTRL, > + ®); > + if (ret) > + return ret; > + > + val->intval &= !!(reg & AXP813_ACIN_PATH_SEL); If we only check this bit if ACIN_AVAIL is set, then we don't need the "&" in the "&=". (I'm assuming that val->intval is an int, not a bool, otherwise this is the wrong operator) Thanks,
Hi Julian, On 1/5/20 7:00 AM, Julian Calaby wrote: > On Sun, Jan 5, 2020 at 12:24 PM Samuel Holland <samuel@sholland.org> wrote: >> >> AXP803/AXP813 have a flag that enables/disables the AC power supply >> input. This flag does not affect the status bits in PWR_INPUT_STATUS. >> Its effect can be verified by checking the battery charge/discharge >> state (bit 2 of PWR_INPUT_STATUS), or by examining the current draw on >> the AC input. >> >> Take this flag into account when getting the ONLINE property of the AC >> input, on PMICs where this flag is present. >> >> Fixes: 7693b5643fd2 ("power: supply: add AC power supply driver for AXP813") >> Cc: stable@vger.kernel.org >> Signed-off-by: Samuel Holland <samuel@sholland.org> >> --- >> drivers/power/supply/axp20x_ac_power.c | 31 +++++++++++++++++++++----- >> 1 file changed, 25 insertions(+), 6 deletions(-) >> >> diff --git a/drivers/power/supply/axp20x_ac_power.c b/drivers/power/supply/axp20x_ac_power.c >> index 0d34a932b6d5..ca0a28f72a27 100644 >> --- a/drivers/power/supply/axp20x_ac_power.c >> +++ b/drivers/power/supply/axp20x_ac_power.c >> @@ -23,6 +23,8 @@ >> #define AXP20X_PWR_STATUS_ACIN_PRESENT BIT(7) >> #define AXP20X_PWR_STATUS_ACIN_AVAIL BIT(6) >> >> +#define AXP813_ACIN_PATH_SEL BIT(7) >> + >> #define AXP813_VHOLD_MASK GENMASK(5, 3) >> #define AXP813_VHOLD_UV_TO_BIT(x) ((((x) / 100000) - 40) << 3) >> #define AXP813_VHOLD_REG_TO_UV(x) \ >> @@ -40,6 +42,7 @@ struct axp20x_ac_power { >> struct power_supply *supply; >> struct iio_channel *acin_v; >> struct iio_channel *acin_i; >> + bool has_acin_path_sel; >> }; >> >> static irqreturn_t axp20x_ac_power_irq(int irq, void *devid) >> @@ -86,6 +89,17 @@ static int axp20x_ac_power_get_property(struct power_supply *psy, >> return ret; >> >> val->intval = !!(reg & AXP20X_PWR_STATUS_ACIN_AVAIL); >> + >> + /* ACIN_PATH_SEL disables ACIN even if ACIN_AVAIL is set. */ >> + if (power->has_acin_path_sel) { > > Do we need to check this bit if ACIN_AVAIL is not set? No, we don't. However due to regcache this won't actually cause another read from the device. If I send a v3, I'll move the && to the if statement. >> + ret = regmap_read(power->regmap, AXP813_ACIN_PATH_CTRL, >> + ®); >> + if (ret) >> + return ret; >> + >> + val->intval &= !!(reg & AXP813_ACIN_PATH_SEL); > > If we only check this bit if ACIN_AVAIL is set, then we don't need the > "&" in the "&=". (I'm assuming that val->intval is an int, not a bool, > otherwise this is the wrong operator) val->intval is an int, but it only ever takes the values 0 or 1. The !! expression coerces an integer to the range of a boolean. So the two ways of deriving the value ("&=" here vs "&& val->intval" in the if statement) are equivalent. > Thanks, Thanks! Samuel
diff --git a/drivers/power/supply/axp20x_ac_power.c b/drivers/power/supply/axp20x_ac_power.c index 0d34a932b6d5..ca0a28f72a27 100644 --- a/drivers/power/supply/axp20x_ac_power.c +++ b/drivers/power/supply/axp20x_ac_power.c @@ -23,6 +23,8 @@ #define AXP20X_PWR_STATUS_ACIN_PRESENT BIT(7) #define AXP20X_PWR_STATUS_ACIN_AVAIL BIT(6) +#define AXP813_ACIN_PATH_SEL BIT(7) + #define AXP813_VHOLD_MASK GENMASK(5, 3) #define AXP813_VHOLD_UV_TO_BIT(x) ((((x) / 100000) - 40) << 3) #define AXP813_VHOLD_REG_TO_UV(x) \ @@ -40,6 +42,7 @@ struct axp20x_ac_power { struct power_supply *supply; struct iio_channel *acin_v; struct iio_channel *acin_i; + bool has_acin_path_sel; }; static irqreturn_t axp20x_ac_power_irq(int irq, void *devid) @@ -86,6 +89,17 @@ static int axp20x_ac_power_get_property(struct power_supply *psy, return ret; val->intval = !!(reg & AXP20X_PWR_STATUS_ACIN_AVAIL); + + /* ACIN_PATH_SEL disables ACIN even if ACIN_AVAIL is set. */ + if (power->has_acin_path_sel) { + ret = regmap_read(power->regmap, AXP813_ACIN_PATH_CTRL, + ®); + if (ret) + return ret; + + val->intval &= !!(reg & AXP813_ACIN_PATH_SEL); + } + return 0; case POWER_SUPPLY_PROP_VOLTAGE_NOW: @@ -224,21 +238,25 @@ static const struct power_supply_desc axp813_ac_power_desc = { struct axp_data { const struct power_supply_desc *power_desc; bool acin_adc; + bool acin_path_sel; }; static const struct axp_data axp20x_data = { - .power_desc = &axp20x_ac_power_desc, - .acin_adc = true, + .power_desc = &axp20x_ac_power_desc, + .acin_adc = true, + .acin_path_sel = false, }; static const struct axp_data axp22x_data = { - .power_desc = &axp22x_ac_power_desc, - .acin_adc = false, + .power_desc = &axp22x_ac_power_desc, + .acin_adc = false, + .acin_path_sel = false, }; static const struct axp_data axp813_data = { - .power_desc = &axp813_ac_power_desc, - .acin_adc = false, + .power_desc = &axp813_ac_power_desc, + .acin_adc = false, + .acin_path_sel = true, }; static int axp20x_ac_power_probe(struct platform_device *pdev) @@ -282,6 +300,7 @@ static int axp20x_ac_power_probe(struct platform_device *pdev) } power->regmap = dev_get_regmap(pdev->dev.parent, NULL); + power->has_acin_path_sel = axp_data->acin_path_sel; platform_set_drvdata(pdev, power);
AXP803/AXP813 have a flag that enables/disables the AC power supply input. This flag does not affect the status bits in PWR_INPUT_STATUS. Its effect can be verified by checking the battery charge/discharge state (bit 2 of PWR_INPUT_STATUS), or by examining the current draw on the AC input. Take this flag into account when getting the ONLINE property of the AC input, on PMICs where this flag is present. Fixes: 7693b5643fd2 ("power: supply: add AC power supply driver for AXP813") Cc: stable@vger.kernel.org Signed-off-by: Samuel Holland <samuel@sholland.org> --- drivers/power/supply/axp20x_ac_power.c | 31 +++++++++++++++++++++----- 1 file changed, 25 insertions(+), 6 deletions(-)