| Message ID | cover.156212c9c5df493e098556cad9793a07dcab5e32.1543245984.git-series.plaes@plaes.org |
|---|---|
| Headers | show |
| Series | regulator: axp20x: Stop AXP209 from crashing when enabling LDO3 | expand |
On Mon, Nov 26, 2018 at 05:27:50PM +0200, Priit Laes wrote: > In the defense of LDO3, LDO3 is the regulator that feeds port bank E, > which has no other purpose then a CSI/TS interface, however the case > may still be, that the connected IO may be just as well be 3.3 volts. > The big misnomer is however, that the schematic names GPIO-2 pin4 > LDO3_2.8V, rather then VDD-CSI0 or similar. In general you want to run regulators at the lowest voltage you can, this tends to reduce power consumption. > Ideally, we want to set a supply voltage for each port bank, but the > monolithic nature of the sunxi pinctroller currently prevents this and > as such, the board should at least configure the LDO4 with the proper > ranges. > ®_ldo4 { > - regulator-min-microvolt = <2800000>; > - regulator-max-microvolt = <2800000>; > - regulator-name = "vddio-csi1"; > + regulator-always-on; > + regulator-min-microvolt = <1250000>; > + regulator-max-microvolt = <3300000>; > + regulator-name = "vdd-io-pg"; > }; This is obviously broken even according to your analysis above - if you have consumers for which 2.8V is too low allowing other consumers to set even lower voltages is not going to help as soon as they start doing that.
On Mon, Nov 26, 2018 at 05:27:47PM +0200, Priit Laes wrote: > From: Olliver Schinagl <oliver@schinagl.nl> > > In the past, there have been words on various lists that if LDO3 is > disabled in u-boot, but enabled in the DTS, the axp209 driver would > fail to continue/hang. Several enable/disable patches have been > issues to devicetree's in both the kernel and u-boot to address > this issue. > > What really happened however, was that the AXP209 shuts down without > a notice and without setting an interrupt. This is caused when LDO3 > gets overloaded, for example with large capacitors on the LDO3 output. > > Normally, we would expect that AXP209 would source 200 mA as per > datasheet and set and trigger an interrupt when being overloaded. > For some reason however, this does not happen. > > As a work-around, we use the soft-start constraint of the regulator > node to first bring up the LDO3 to the lowest possible voltage and > then enable the LDO. After that, we can set the requested voltage > as usual. > > Combining this setting with the regulator-ramp-delay allows LDO3 to > enable voltage slowly and staggered, potentially reducing overall > inrush current. > > Signed-off-by: Olliver Schinagl <oliver@schinagl.nl> > Signed-off-by: Priit Laes <plaes@plaes.org> > --- > drivers/regulator/axp20x-regulator.c | 57 ++++++++++++++++++++++++++++- > 1 file changed, 56 insertions(+), 1 deletion(-) > > diff --git a/drivers/regulator/axp20x-regulator.c b/drivers/regulator/axp20x-regulator.c > index 1d9fa62..e8a895b 100644 > --- a/drivers/regulator/axp20x-regulator.c > +++ b/drivers/regulator/axp20x-regulator.c > @@ -14,6 +14,7 @@ > */ > > #include <linux/bitops.h> > +#include <linux/delay.h> > #include <linux/err.h> > #include <linux/init.h> > #include <linux/mfd/axp20x.h> > @@ -23,6 +24,7 @@ > #include <linux/platform_device.h> > #include <linux/regmap.h> > #include <linux/regulator/driver.h> > +#include <linux/regulator/machine.h> > #include <linux/regulator/of_regulator.h> > > #define AXP20X_GPIO0_FUNC_MASK GENMASK(3, 0) > @@ -430,6 +432,59 @@ static int axp20x_set_ramp_delay(struct regulator_dev *rdev, int ramp) > return regmap_update_bits(axp20x->regmap, reg, mask, cfg); > } > > +static int axp20x_regulator_enable_regmap(struct regulator_dev *rdev) > +{ > + struct axp20x_dev *axp20x = rdev_get_drvdata(rdev); > + const struct regulator_desc *desc = rdev->desc; > + > + if (!rdev) > + return -EINVAL; > + > + switch (axp20x->variant) { > + case AXP209_ID: > + if ((desc->id == AXP20X_LDO3) && > + rdev->constraints && rdev->constraints->soft_start) { > + int v_out; > + int ret; > + > + /* > + * On some boards, the LDO3 can be overloaded when > + * turning on, causing the entire PMIC to shutdown > + * without warning. Turning it on at the minimal voltage > + * and then setting the voltage to the requested value > + * works reliably. > + */ > + if (regulator_is_enabled_regmap(rdev)) > + break; > + > + v_out = regulator_get_voltage_sel_regmap(rdev); > + if (v_out < 0) > + return v_out; > + > + if (v_out == 0) > + break; > + > + ret = regulator_set_voltage_sel_regmap(rdev, 0x00); > + /* > + * A small pause is needed between > + * setting the voltage and enabling the LDO to give the > + * internal state machine time to process the request. > + */ > + usleep_range(1000, 5000); > + ret |= regulator_enable_regmap(rdev); > + ret |= regulator_set_voltage_sel_regmap(rdev, v_out); > + > + return ret; > + } > + break; > + default: > + /* No quirks */ > + break; > + } > + > + return regulator_enable_regmap(rdev); > +}; > + This is some pretty generic code, and could be useful to some other users. I guess a generic function would be better for this. Maxime
On Mon, Nov 26, 2018 at 05:27:51PM +0200, Priit Laes wrote: > From: Olliver Schinagl <oliver@schinagl.nl> > > The lradc's analog reference voltage is set to 3.0 volt in the > hardware. This is more or less set in copper for at least lradc0. Set the > property in the dts to ensure the lradc is referenced properly. > > Signed-off-by: Olliver Schinagl <oliver@schinagl.nl> > Signed-off-by: Priit Laes <plaes@plaes.org> I'm not sure why that patch is part of this series, but I applied it. Thanks! Maxime
On Mon, Nov 26, 2018 at 05:27:50PM +0200, Priit Laes wrote: > From: Olliver Schinagl <oliver@schinagl.nl> > > With commit b43776d65a33b46092 ("ARM: dts: sunxi: Use axp209.dtsi for > Olinuxino Lime2") we force them an arbitrary 2.8 volts. Granted, for > LDO3 this may be less arbitrary, but for LDO4 this is just wrong. > > In the defense of LDO3, LDO3 is the regulator that feeds port bank E, > which has no other purpose then a CSI/TS interface, however the case > may still be, that the connected IO may be just as well be 3.3 volts. > The big misnomer is however, that the schematic names GPIO-2 pin4 > LDO3_2.8V, rather then VDD-CSI0 or similar. > > This is much worse for LDO4 however, which is not referenced on any > pin, is now set to 2.8 volts, but port bank G can also support various > other peripherals such as UARTS etc. > > By having 2.8 volts however for LDO4, we thus now have peripherals that > no longer function properly all of the time. > > Ideally, we want to set a supply voltage for each port bank, but the > monolithic nature of the sunxi pinctroller currently prevents this and > as such, the board should at least configure the LDO4 with the proper > ranges. > > Until we can set the consumer at the port bank level, a child > device-tree has to do something like: > > ®_ldo4 { > regulator-min-microvolt = <3300000>; > regulator-max-microvolt = <3300000>; > }; > > While doing this the same way results in the same solution currently, > we force the hack into the final devicetree rather then having it wrong > at the board level. > > Signed-off-by: Olliver Schinagl <oliver@schinagl.nl> > Signed-off-by: Priit Laes <plaes@plaes.org> > --- > arch/arm/boot/dts/sun7i-a20-olinuxino-lime2.dts | 7 ++++--- > 1 file changed, 4 insertions(+), 3 deletions(-) > > diff --git a/arch/arm/boot/dts/sun7i-a20-olinuxino-lime2.dts b/arch/arm/boot/dts/sun7i-a20-olinuxino-lime2.dts > index ffafe97..1b9867f 100644 > --- a/arch/arm/boot/dts/sun7i-a20-olinuxino-lime2.dts > +++ b/arch/arm/boot/dts/sun7i-a20-olinuxino-lime2.dts > @@ -250,9 +250,10 @@ > }; > > ®_ldo4 { > - regulator-min-microvolt = <2800000>; > - regulator-max-microvolt = <2800000>; > - regulator-name = "vddio-csi1"; > + regulator-always-on; > + regulator-min-microvolt = <1250000>; > + regulator-max-microvolt = <3300000>; > + regulator-name = "vdd-io-pg"; As we discussed on the U-Boot ML already, this shouldn't be made always-on but tied to the consumer device (the pinctrl one) instead. Maxime
On Tue, Nov 27, 2018 at 5:37 PM Maxime Ripard <maxime.ripard@bootlin.com> wrote: > > On Mon, Nov 26, 2018 at 05:27:51PM +0200, Priit Laes wrote: > > From: Olliver Schinagl <oliver@schinagl.nl> > > > > The lradc's analog reference voltage is set to 3.0 volt in the > > hardware. This is more or less set in copper for at least lradc0. Set the > > property in the dts to ensure the lradc is referenced properly. > > > > Signed-off-by: Olliver Schinagl <oliver@schinagl.nl> > > Signed-off-by: Priit Laes <plaes@plaes.org> > > I'm not sure why that patch is part of this series, but I applied > it. Thanks! Please help fix up the subject. Should read "ARM: dts: sun7i: set proper lradc vref on OLinuXino Lime2" or "ARM: dts: sun7i: olinoxino-lime2: set proper lradc vref" ChenYu
On Tue, Nov 27, 2018 at 05:41:10PM +0800, Chen-Yu Tsai wrote: > On Tue, Nov 27, 2018 at 5:37 PM Maxime Ripard <maxime.ripard@bootlin.com> wrote: > > > > On Mon, Nov 26, 2018 at 05:27:51PM +0200, Priit Laes wrote: > > > From: Olliver Schinagl <oliver@schinagl.nl> > > > > > > The lradc's analog reference voltage is set to 3.0 volt in the > > > hardware. This is more or less set in copper for at least lradc0. Set the > > > property in the dts to ensure the lradc is referenced properly. > > > > > > Signed-off-by: Olliver Schinagl <oliver@schinagl.nl> > > > Signed-off-by: Priit Laes <plaes@plaes.org> > > > > I'm not sure why that patch is part of this series, but I applied > > it. Thanks! > > Please help fix up the subject. Should read > > "ARM: dts: sun7i: set proper lradc vref on OLinuXino Lime2" > > or > > "ARM: dts: sun7i: olinoxino-lime2: set proper lradc vref" Yes, I've noticed it while applying, and this is already fixed :) Thanks for the reminder! Maxime
On Mon, 26 Nov 2018, Priit Laes wrote: > From: Olliver Schinagl <oliver@schinagl.nl> > > The current axp20x names the ramping register 'scal' which probably > means scaling. Since the register really has nothing to do with > scaling, but really is the voltage ramp we rename it appropriately. > > Signed-off-by: Olliver Schinagl <oliver@schinagl.nl> > Signed-off-by: Priit Laes <plaes@plaes.org> > --- > include/linux/mfd/axp20x.h | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) This is clearly not a regulator patch. Please change the subject line to "mfd", Once changed you can add my: For my own reference: Acked-for-MFD-by: Lee Jones <lee.jones@linaro.org>
On Mon, 26 Nov 2018, Priit Laes wrote: > From: Olliver Schinagl <oliver@schinagl.nl> > > Add the bitops.h header as we need it, alphabetize header order. > > Signed-off-by: Olliver Schinagl <oliver@schinagl.nl> > Signed-off-by: Priit Laes <plaes@plaes.org> > --- > drivers/mfd/axp20x.c | 11 ++++++----- > 1 file changed, 6 insertions(+), 5 deletions(-) Nit: You are making 2 separate functional changes here. In future please submit functional changes in separate patches. For my own reference: Acked-for-MFD-by: Lee Jones <lee.jones@linaro.org>
On Mon, 26 Nov 2018, Priit Laes wrote: > From: Olliver Schinagl <oliver@schinagl.nl> > > The AXP20X_OFF define is an actual specific bit, define it as such. > > Signed-off-by: Olliver Schinagl <oliver@schinagl.nl> > Signed-off-by: Priit Laes <plaes@plaes.org> > --- > drivers/mfd/axp20x.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) For my own reference: Acked-for-MFD-by: Lee Jones <lee.jones@linaro.org>
On Tue, Nov 27, 2018 at 10:38:52AM +0100, Maxime Ripard wrote: > On Mon, Nov 26, 2018 at 05:27:50PM +0200, Priit Laes wrote: > > From: Olliver Schinagl <oliver@schinagl.nl> > > > > With commit b43776d65a33b46092 ("ARM: dts: sunxi: Use axp209.dtsi for > > Olinuxino Lime2") we force them an arbitrary 2.8 volts. Granted, for > > LDO3 this may be less arbitrary, but for LDO4 this is just wrong. > > > > In the defense of LDO3, LDO3 is the regulator that feeds port bank E, > > which has no other purpose then a CSI/TS interface, however the case > > may still be, that the connected IO may be just as well be 3.3 volts. > > The big misnomer is however, that the schematic names GPIO-2 pin4 > > LDO3_2.8V, rather then VDD-CSI0 or similar. > > > > This is much worse for LDO4 however, which is not referenced on any > > pin, is now set to 2.8 volts, but port bank G can also support various > > other peripherals such as UARTS etc. > > > > By having 2.8 volts however for LDO4, we thus now have peripherals that > > no longer function properly all of the time. > > > > Ideally, we want to set a supply voltage for each port bank, but the > > monolithic nature of the sunxi pinctroller currently prevents this and > > as such, the board should at least configure the LDO4 with the proper > > ranges. > > > > Until we can set the consumer at the port bank level, a child > > device-tree has to do something like: > > > > ®_ldo4 { > > regulator-min-microvolt = <3300000>; > > regulator-max-microvolt = <3300000>; > > }; > > > > While doing this the same way results in the same solution currently, > > we force the hack into the final devicetree rather then having it wrong > > at the board level. > > > > Signed-off-by: Olliver Schinagl <oliver@schinagl.nl> > > Signed-off-by: Priit Laes <plaes@plaes.org> > > --- > > arch/arm/boot/dts/sun7i-a20-olinuxino-lime2.dts | 7 ++++--- > > 1 file changed, 4 insertions(+), 3 deletions(-) > > > > diff --git a/arch/arm/boot/dts/sun7i-a20-olinuxino-lime2.dts b/arch/arm/boot/dts/sun7i-a20-olinuxino-lime2.dts > > index ffafe97..1b9867f 100644 > > --- a/arch/arm/boot/dts/sun7i-a20-olinuxino-lime2.dts > > +++ b/arch/arm/boot/dts/sun7i-a20-olinuxino-lime2.dts > > @@ -250,9 +250,10 @@ > > }; > > > > ®_ldo4 { > > - regulator-min-microvolt = <2800000>; > > - regulator-max-microvolt = <2800000>; > > - regulator-name = "vddio-csi1"; > > + regulator-always-on; > > + regulator-min-microvolt = <1250000>; > > + regulator-max-microvolt = <3300000>; > > + regulator-name = "vdd-io-pg"; > > As we discussed on the U-Boot ML already, this shouldn't be made > always-on but tied to the consumer device (the pinctrl one) instead. Can you test that patch (not tested): http://code.bulix.org/h02vha-514284?raw Thanks! Maxime
On Tue, Nov 27, 2018 at 10:36:19AM +0100, Maxime Ripard wrote: > On Mon, Nov 26, 2018 at 05:27:47PM +0200, Priit Laes wrote: > > From: Olliver Schinagl <oliver@schinagl.nl> > > > > In the past, there have been words on various lists that if LDO3 is > > disabled in u-boot, but enabled in the DTS, the axp209 driver would > > fail to continue/hang. Several enable/disable patches have been > > issues to devicetree's in both the kernel and u-boot to address > > this issue. > > > > What really happened however, was that the AXP209 shuts down without > > a notice and without setting an interrupt. This is caused when LDO3 > > gets overloaded, for example with large capacitors on the LDO3 output. > > > > Normally, we would expect that AXP209 would source 200 mA as per > > datasheet and set and trigger an interrupt when being overloaded. > > For some reason however, this does not happen. > > > > As a work-around, we use the soft-start constraint of the regulator > > node to first bring up the LDO3 to the lowest possible voltage and > > then enable the LDO. After that, we can set the requested voltage > > as usual. > > > > Combining this setting with the regulator-ramp-delay allows LDO3 to > > enable voltage slowly and staggered, potentially reducing overall > > inrush current. > > > > Signed-off-by: Olliver Schinagl <oliver@schinagl.nl> > > Signed-off-by: Priit Laes <plaes@plaes.org> > > --- > > drivers/regulator/axp20x-regulator.c | 57 ++++++++++++++++++++++++++++- > > 1 file changed, 56 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/regulator/axp20x-regulator.c b/drivers/regulator/axp20x-regulator.c > > index 1d9fa62..e8a895b 100644 > > --- a/drivers/regulator/axp20x-regulator.c > > +++ b/drivers/regulator/axp20x-regulator.c > > @@ -14,6 +14,7 @@ > > */ > > > > #include <linux/bitops.h> > > +#include <linux/delay.h> > > #include <linux/err.h> > > #include <linux/init.h> > > #include <linux/mfd/axp20x.h> > > @@ -23,6 +24,7 @@ > > #include <linux/platform_device.h> > > #include <linux/regmap.h> > > #include <linux/regulator/driver.h> > > +#include <linux/regulator/machine.h> > > #include <linux/regulator/of_regulator.h> > > > > #define AXP20X_GPIO0_FUNC_MASK GENMASK(3, 0) > > @@ -430,6 +432,59 @@ static int axp20x_set_ramp_delay(struct regulator_dev *rdev, int ramp) > > return regmap_update_bits(axp20x->regmap, reg, mask, cfg); > > } > > > > +static int axp20x_regulator_enable_regmap(struct regulator_dev *rdev) > > +{ > > + struct axp20x_dev *axp20x = rdev_get_drvdata(rdev); > > + const struct regulator_desc *desc = rdev->desc; > > + > > + if (!rdev) > > + return -EINVAL; > > + > > + switch (axp20x->variant) { > > + case AXP209_ID: > > + if ((desc->id == AXP20X_LDO3) && > > + rdev->constraints && rdev->constraints->soft_start) { > > + int v_out; > > + int ret; > > + > > + /* > > + * On some boards, the LDO3 can be overloaded when > > + * turning on, causing the entire PMIC to shutdown > > + * without warning. Turning it on at the minimal voltage > > + * and then setting the voltage to the requested value > > + * works reliably. > > + */ > > + if (regulator_is_enabled_regmap(rdev)) > > + break; > > + > > + v_out = regulator_get_voltage_sel_regmap(rdev); > > + if (v_out < 0) > > + return v_out; > > + > > + if (v_out == 0) > > + break; > > + > > + ret = regulator_set_voltage_sel_regmap(rdev, 0x00); > > + /* > > + * A small pause is needed between > > + * setting the voltage and enabling the LDO to give the > > + * internal state machine time to process the request. > > + */ > > + usleep_range(1000, 5000); > > + ret |= regulator_enable_regmap(rdev); > > + ret |= regulator_set_voltage_sel_regmap(rdev, v_out); > > + > > + return ret; > > + } > > + break; > > + default: > > + /* No quirks */ > > + break; > > + } > > + > > + return regulator_enable_regmap(rdev); > > +}; > > + > > This is some pretty generic code, and could be useful to some other > users. I guess a generic function would be better for this. Yes, makes sense. Although, should we then also distinguish between regulators which support soft-start in hardware and devices which emulate it by delay, like in this case? Päikest, Priit
On Wed, Nov 28, 2018 at 10:56:39AM +0100, Maxime Ripard wrote: > On Tue, Nov 27, 2018 at 10:38:52AM +0100, Maxime Ripard wrote: > > On Mon, Nov 26, 2018 at 05:27:50PM +0200, Priit Laes wrote: > > > From: Olliver Schinagl <oliver@schinagl.nl> > > > > > > With commit b43776d65a33b46092 ("ARM: dts: sunxi: Use axp209.dtsi for > > > Olinuxino Lime2") we force them an arbitrary 2.8 volts. Granted, for > > > LDO3 this may be less arbitrary, but for LDO4 this is just wrong. > > > [ ... ] > > As we discussed on the U-Boot ML already, this shouldn't be made > > always-on but tied to the consumer device (the pinctrl one) instead. > > Can you test that patch (not tested): > http://code.bulix.org/h02vha-514284?raw Quick test (after applying this: http://code.bulix.org/w01xab-518044?raw ) results in failure: [snip] dmesg -t |egrep "(regu|seria)" sun4i-pinctrl 1c20800.pinctrl: 1c20800.pinctrl supply vcc-pb not found, using dummy regulator sun4i-pinctrl 1c20800.pinctrl: Linked as a consumer to regulator.0 sun4i-pinctrl 1c20800.pinctrl: 1c20800.pinctrl supply vcc-pb not found, using dummy regulator 1c28000.serial: ttyS0 at MMIO 0x1c28000 (irq = 47, base_baud = 1500000) is a U6_16550A sun4i-pinctrl 1c20800.pinctrl: Couldn't get bank PG regulator sun4i-pinctrl 1c20800.pinctrl: pin-198 (1c28c00.serial) status -22 dw-apb-uart 1c28c00.serial: Error applying setting, reverse things back dw-apb-uart: probe of 1c28c00.serial failed with error -22 sun4i-pinctrl 1c20800.pinctrl: Couldn't get bank PG regulator sun4i-pinctrl 1c20800.pinctrl: pin-202 (1c29000.serial) status -22 dw-apb-uart 1c29000.serial: Error applying setting, reverse things back dw-apb-uart: probe of 1c29000.serial failed with error -22 sun4i-pinctrl 1c20800.pinctrl: 1c20800.pinctrl supply vcc-pa not found, using dummy regulator sun4i-pinctrl 1c20800.pinctrl: 1c20800.pinctrl supply vcc-pa not found, using dummy regulator sun4i-pinctrl 1c20800.pinctrl: 1c20800.pinctrl supply vcc-pa not found, using dummy regulator sun4i-pinctrl 1c20800.pinctrl: 1c20800.pinctrl supply vcc-pa not found, using dummy regulator sun4i-pinctrl 1c20800.pinctrl: 1c20800.pinctrl supply vcc-pa not found, using dummy regulator sun4i-pinctrl 1c20800.pinctrl: 1c20800.pinctrl supply vcc-pa not found, using dummy regulator sun4i-pinctrl 1c20800.pinctrl: 1c20800.pinctrl supply vcc-pa not found, using dummy regulator sun4i-pinctrl 1c20800.pinctrl: 1c20800.pinctrl supply vcc-pa not found, using dummy regulator sun4i-pinctrl 1c20800.pinctrl: 1c20800.pinctrl supply vcc-pa not found, using dummy regulator sun4i-pinctrl 1c20800.pinctrl: 1c20800.pinctrl supply vcc-pa not found, using dummy regulator sun4i-pinctrl 1c20800.pinctrl: 1c20800.pinctrl supply vcc-pa not found, using dummy regulator sun4i-pinctrl 1c20800.pinctrl: 1c20800.pinctrl supply vcc-pa not found, using dummy regulator sun4i-pinctrl 1c20800.pinctrl: 1c20800.pinctrl supply vcc-pa not found, using dummy regulator ... [/snip] And I cannot actually see the link between pinctrl and ldo4. There's currently only this link: sun4i-pinctrl 1c20800.pinctrl: Linked as a consumer to regulator.0 $ cat /sys/class/regulator/regulator.0/name regulator-dummy > > Thanks! > Maxime > > -- > Maxime Ripard, Bootlin > Embedded Linux and Kernel engineering > https://bootlin.com
On Tue, Dec 04, 2018 at 01:31:44PM +0000, Priit Laes wrote: > Yes, makes sense. Although, should we then also distinguish between > regulators which support soft-start in hardware and devices which emulate > it by delay, like in this case? Drivers should not be emulating soft start, if they need a ramp delay they should set that in their descriptor and let the framework do it.
Hi, On Mon, Nov 26, 2018 at 05:27:54PM +0200, Priit Laes wrote: > From: Olliver Schinagl <oliver@schinagl.nl> > > The axp20x_usb_power driver uses BIT() operations but lacks the include > for it. Include the bitops.h header file. > > Signed-off-by: Olliver Schinagl <oliver@schinagl.nl> > Signed-off-by: Priit Laes <plaes@plaes.org> > --- > drivers/power/supply/axp20x_usb_power.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/drivers/power/supply/axp20x_usb_power.c b/drivers/power/supply/axp20x_usb_power.c > index 42001df..f52fe77 100644 > --- a/drivers/power/supply/axp20x_usb_power.c > +++ b/drivers/power/supply/axp20x_usb_power.c > @@ -10,6 +10,7 @@ > * option) any later version. > */ > > +#include <linux/bitops.h> > #include <linux/device.h> > #include <linux/init.h> > #include <linux/interrupt.h> > -- > git-series 0.9.1 Thanks, queued to power-supply-next. -- Sebastian
Hi, On Mon, Nov 26, 2018 at 05:27:55PM +0200, Priit Laes wrote: > From: Olliver Schinagl <oliver@schinagl.nl> > > Make use of the recommended BIT() macro for bit defines. > > Signed-off-by: Olliver Schinagl <oliver@schinagl.nl> > Signed-off-by: Priit Laes <plaes@plaes.org> > --- Thanks, queued to power-supply-next. -- Sebastian > drivers/power/supply/axp288_charger.c | 35 ++++++++++++++-------------- > 1 file changed, 18 insertions(+), 17 deletions(-) > > diff --git a/drivers/power/supply/axp288_charger.c b/drivers/power/supply/axp288_charger.c > index 735658e..f8c6da9 100644 > --- a/drivers/power/supply/axp288_charger.c > +++ b/drivers/power/supply/axp288_charger.c > @@ -16,6 +16,7 @@ > */ > > #include <linux/acpi.h> > +#include <linux/bitops.h> > #include <linux/module.h> > #include <linux/device.h> > #include <linux/regmap.h> > @@ -29,17 +30,17 @@ > #include <linux/mfd/axp20x.h> > #include <linux/extcon.h> > > -#define PS_STAT_VBUS_TRIGGER (1 << 0) > -#define PS_STAT_BAT_CHRG_DIR (1 << 2) > -#define PS_STAT_VBAT_ABOVE_VHOLD (1 << 3) > -#define PS_STAT_VBUS_VALID (1 << 4) > -#define PS_STAT_VBUS_PRESENT (1 << 5) > +#define PS_STAT_VBUS_TRIGGER BIT(0) > +#define PS_STAT_BAT_CHRG_DIR BIT(2) > +#define PS_STAT_VBAT_ABOVE_VHOLD BIT(3) > +#define PS_STAT_VBUS_VALID BIT(4) > +#define PS_STAT_VBUS_PRESENT BIT(5) > > -#define CHRG_STAT_BAT_SAFE_MODE (1 << 3) > -#define CHRG_STAT_BAT_VALID (1 << 4) > -#define CHRG_STAT_BAT_PRESENT (1 << 5) > -#define CHRG_STAT_CHARGING (1 << 6) > -#define CHRG_STAT_PMIC_OTP (1 << 7) > +#define CHRG_STAT_BAT_SAFE_MODE BIT(3) > +#define CHRG_STAT_BAT_VALID BIT(4) > +#define CHRG_STAT_BAT_PRESENT BIT(5) > +#define CHRG_STAT_CHARGING BIT(6) > +#define CHRG_STAT_PMIC_OTP BIT(7) > > #define VBUS_ISPOUT_CUR_LIM_MASK 0x03 > #define VBUS_ISPOUT_CUR_LIM_BIT_POS 0 > @@ -52,33 +53,33 @@ > #define VBUS_ISPOUT_VHOLD_SET_OFFSET 4000 /* 4000mV */ > #define VBUS_ISPOUT_VHOLD_SET_LSB_RES 100 /* 100mV */ > #define VBUS_ISPOUT_VHOLD_SET_4300MV 0x3 /* 4300mV */ > -#define VBUS_ISPOUT_VBUS_PATH_DIS (1 << 7) > +#define VBUS_ISPOUT_VBUS_PATH_DIS BIT(7) > > #define CHRG_CCCV_CC_MASK 0xf /* 4 bits */ > #define CHRG_CCCV_CC_BIT_POS 0 > #define CHRG_CCCV_CC_OFFSET 200 /* 200mA */ > #define CHRG_CCCV_CC_LSB_RES 200 /* 200mA */ > -#define CHRG_CCCV_ITERM_20P (1 << 4) /* 20% of CC */ > +#define CHRG_CCCV_ITERM_20P BIT(4) /* 20% of CC */ > #define CHRG_CCCV_CV_MASK 0x60 /* 2 bits */ > #define CHRG_CCCV_CV_BIT_POS 5 > #define CHRG_CCCV_CV_4100MV 0x0 /* 4.10V */ > #define CHRG_CCCV_CV_4150MV 0x1 /* 4.15V */ > #define CHRG_CCCV_CV_4200MV 0x2 /* 4.20V */ > #define CHRG_CCCV_CV_4350MV 0x3 /* 4.35V */ > -#define CHRG_CCCV_CHG_EN (1 << 7) > +#define CHRG_CCCV_CHG_EN BIT(7) > > #define CNTL2_CC_TIMEOUT_MASK 0x3 /* 2 bits */ > #define CNTL2_CC_TIMEOUT_OFFSET 6 /* 6 Hrs */ > #define CNTL2_CC_TIMEOUT_LSB_RES 2 /* 2 Hrs */ > #define CNTL2_CC_TIMEOUT_12HRS 0x3 /* 12 Hrs */ > -#define CNTL2_CHGLED_TYPEB (1 << 4) > -#define CNTL2_CHG_OUT_TURNON (1 << 5) > +#define CNTL2_CHGLED_TYPEB BIT(4) > +#define CNTL2_CHG_OUT_TURNON BIT(5) > #define CNTL2_PC_TIMEOUT_MASK 0xC0 > #define CNTL2_PC_TIMEOUT_OFFSET 40 /* 40 mins */ > #define CNTL2_PC_TIMEOUT_LSB_RES 10 /* 10 mins */ > #define CNTL2_PC_TIMEOUT_70MINS 0x3 > > -#define CHRG_ILIM_TEMP_LOOP_EN (1 << 3) > +#define CHRG_ILIM_TEMP_LOOP_EN BIT(3) > #define CHRG_VBUS_ILIM_MASK 0xf0 > #define CHRG_VBUS_ILIM_BIT_POS 4 > #define CHRG_VBUS_ILIM_100MA 0x0 /* 100mA */ > @@ -94,7 +95,7 @@ > #define CHRG_VLTFC_0C 0xA5 /* 0 DegC */ > #define CHRG_VHTFC_45C 0x1F /* 45 DegC */ > > -#define FG_CNTL_OCV_ADJ_EN (1 << 3) > +#define FG_CNTL_OCV_ADJ_EN BIT(3) > > #define CV_4100MV 4100 /* 4100mV */ > #define CV_4150MV 4150 /* 4150mV */ > -- > git-series 0.9.1