This is v2 of the fixes to the AXP717 PMIC support series. Lee put the
original patches in an immutable branch already, so these here go on top.
Patch 1 is new in v2, and adds the IRQ status and acknowledge registers
to the writable range. Thanks to Chris for pointing this out.
Patch 2 contains fixes to the regulator descriptions: the LDOs had the
wrong supply source, and two numbers were wrong. The datasheet describes
the voltage ranges and register values differently from what our macros
expect, in a way that literally begs for off-by-ones, so here you go.
Also there is an actual wrong number in the datasheet, add a comment to
document this.
I don't know if that's still feasible, but those two patches would be a
good candidate to squash into the patches that they fix.
The other three patches add the "boost" regulator, which is meant to
provide the 5V USB VBUS power when operating from the battery. It's the
usual trinity of binding/mfd/regulator patches.
Again this could be squashed into the respective patches from the
original series, if people agree.
Please have a look and test!
Based on mfd/ib-mfd-regulator-6.10, as detailed below.
Cheers,
Andre
Changelog v1 .. v2:
- add tags
- add patch to add missing IRQ ack register range
- add comment to document bug in datasheet
Andre Przywara (5):
mfd: axp20x: AXP717: Fix missing IRQ status registers range
regulator: axp20x: AXP717: fix LDO supply rails and off-by-ones
dt-bindings: mfd: x-powers,axp152: add boost regulator
mfd: axp20x: AXP717: Add support for boost regulator
regulator: axp20x: AXP717: Add boost regulator
.../bindings/mfd/x-powers,axp152.yaml | 2 +-
drivers/mfd/axp20x.c | 3 ++
drivers/regulator/axp20x-regulator.c | 37 ++++++++++++-------
include/linux/mfd/axp20x.h | 3 ++
4 files changed, 30 insertions(+), 15 deletions(-)
base-commit: 4cece764965020c22cff7665b18a012006359095
prerequisite-patch-id: 2b5fb10f68e0994071fc4c7dce73db7047c23220
prerequisite-patch-id: 5d0735de888d155b2c1cdb814e852a5852a17ec7
prerequisite-patch-id: 29c30894b4bf0b9e1e71de065cabbd842505e248
prerequisite-patch-id: 0ab87cbf7362b6dc2d577d2264eb9574be47b5f6
--
2.35.8
While we list the "IRQ status *and acknowledge*" registers as volatile,
they are missing from the writable range array, so acknowledging any
interrupts was met with an -EIO error.
Add the five registers that hold those bits to the writable array.
Fixes: b5bfc8ab2484 ("mfd: axp20x: Add support for AXP717 PMIC")
Reported-by: Chris Morgan <[email protected]>
Signed-off-by: Andre Przywara <[email protected]>
---
drivers/mfd/axp20x.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/mfd/axp20x.c b/drivers/mfd/axp20x.c
index 48ce6ea693cea..d8ad4e120d379 100644
--- a/drivers/mfd/axp20x.c
+++ b/drivers/mfd/axp20x.c
@@ -210,6 +210,7 @@ static const struct regmap_access_table axp313a_volatile_table = {
static const struct regmap_range axp717_writeable_ranges[] = {
regmap_reg_range(AXP717_IRQ0_EN, AXP717_IRQ4_EN),
+ regmap_reg_range(AXP717_IRQ0_STATE, AXP717_IRQ4_STATE),
regmap_reg_range(AXP717_DCDC_OUTPUT_CONTROL, AXP717_CPUSLDO_CONTROL),
};
--
2.35.8
The X-Powers AXP717 PMIC has separate input supply pins for each group
of LDOs, so they are not all using the same DCDC1 input, as described
currently.
Replace the "supply" member of each LDO description with the respective
group supply name, so that the supply dependencies can be correctly
described in the devicetree.
Also fix two off-by-ones in the regulator macros, after some double
checking the numbers against the datasheet. This uncovered a bug in the
datasheet: add a comment to document this.
Fixes: d2ac3df75c3a ("regulator: axp20x: add support for the AXP717")
Signed-off-by: Andre Przywara <[email protected]>
---
drivers/regulator/axp20x-regulator.c | 33 ++++++++++++++++------------
1 file changed, 19 insertions(+), 14 deletions(-)
diff --git a/drivers/regulator/axp20x-regulator.c b/drivers/regulator/axp20x-regulator.c
index 34fcdd82b2eaa..f3c447ecdc3bf 100644
--- a/drivers/regulator/axp20x-regulator.c
+++ b/drivers/regulator/axp20x-regulator.c
@@ -140,7 +140,7 @@
#define AXP717_DCDC1_NUM_VOLTAGES 88
#define AXP717_DCDC2_NUM_VOLTAGES 107
-#define AXP717_DCDC3_NUM_VOLTAGES 104
+#define AXP717_DCDC3_NUM_VOLTAGES 103
#define AXP717_DCDC_V_OUT_MASK GENMASK(6, 0)
#define AXP717_LDO_V_OUT_MASK GENMASK(4, 0)
@@ -763,10 +763,15 @@ static const struct linear_range axp717_dcdc1_ranges[] = {
REGULATOR_LINEAR_RANGE(1220000, 71, 87, 20000),
};
+/*
+ * The manual says that the last voltage is 3.4V, encoded as 0b1101011 (107),
+ * but every other method proves that this is wrong, so it's really 106 that
+ * programs the final 3.4V.
+ */
static const struct linear_range axp717_dcdc2_ranges[] = {
REGULATOR_LINEAR_RANGE(500000, 0, 70, 10000),
REGULATOR_LINEAR_RANGE(1220000, 71, 87, 20000),
- REGULATOR_LINEAR_RANGE(1600000, 88, 107, 100000),
+ REGULATOR_LINEAR_RANGE(1600000, 88, 106, 100000),
};
static const struct linear_range axp717_dcdc3_ranges[] = {
@@ -790,40 +795,40 @@ static const struct regulator_desc axp717_regulators[] = {
AXP_DESC(AXP717, DCDC4, "dcdc4", "vin4", 1000, 3700, 100,
AXP717_DCDC4_CONTROL, AXP717_DCDC_V_OUT_MASK,
AXP717_DCDC_OUTPUT_CONTROL, BIT(3)),
- AXP_DESC(AXP717, ALDO1, "aldo1", "vin1", 500, 3500, 100,
+ AXP_DESC(AXP717, ALDO1, "aldo1", "aldoin", 500, 3500, 100,
AXP717_ALDO1_CONTROL, AXP717_LDO_V_OUT_MASK,
AXP717_LDO0_OUTPUT_CONTROL, BIT(0)),
- AXP_DESC(AXP717, ALDO2, "aldo2", "vin1", 500, 3500, 100,
+ AXP_DESC(AXP717, ALDO2, "aldo2", "aldoin", 500, 3500, 100,
AXP717_ALDO2_CONTROL, AXP717_LDO_V_OUT_MASK,
AXP717_LDO0_OUTPUT_CONTROL, BIT(1)),
- AXP_DESC(AXP717, ALDO3, "aldo3", "vin1", 500, 3500, 100,
+ AXP_DESC(AXP717, ALDO3, "aldo3", "aldoin", 500, 3500, 100,
AXP717_ALDO3_CONTROL, AXP717_LDO_V_OUT_MASK,
AXP717_LDO0_OUTPUT_CONTROL, BIT(2)),
- AXP_DESC(AXP717, ALDO4, "aldo4", "vin1", 500, 3500, 100,
+ AXP_DESC(AXP717, ALDO4, "aldo4", "aldoin", 500, 3500, 100,
AXP717_ALDO4_CONTROL, AXP717_LDO_V_OUT_MASK,
AXP717_LDO0_OUTPUT_CONTROL, BIT(3)),
- AXP_DESC(AXP717, BLDO1, "bldo1", "vin1", 500, 3500, 100,
+ AXP_DESC(AXP717, BLDO1, "bldo1", "bldoin", 500, 3500, 100,
AXP717_BLDO1_CONTROL, AXP717_LDO_V_OUT_MASK,
AXP717_LDO0_OUTPUT_CONTROL, BIT(4)),
- AXP_DESC(AXP717, BLDO2, "bldo2", "vin1", 500, 3500, 100,
+ AXP_DESC(AXP717, BLDO2, "bldo2", "bldoin", 500, 3500, 100,
AXP717_BLDO2_CONTROL, AXP717_LDO_V_OUT_MASK,
AXP717_LDO0_OUTPUT_CONTROL, BIT(5)),
- AXP_DESC(AXP717, BLDO3, "bldo3", "vin1", 500, 3500, 100,
+ AXP_DESC(AXP717, BLDO3, "bldo3", "bldoin", 500, 3500, 100,
AXP717_BLDO3_CONTROL, AXP717_LDO_V_OUT_MASK,
AXP717_LDO0_OUTPUT_CONTROL, BIT(6)),
- AXP_DESC(AXP717, BLDO4, "bldo4", "vin1", 500, 3500, 100,
+ AXP_DESC(AXP717, BLDO4, "bldo4", "bldoin", 500, 3500, 100,
AXP717_BLDO4_CONTROL, AXP717_LDO_V_OUT_MASK,
AXP717_LDO0_OUTPUT_CONTROL, BIT(7)),
- AXP_DESC(AXP717, CLDO1, "cldo1", "vin1", 500, 3500, 100,
+ AXP_DESC(AXP717, CLDO1, "cldo1", "cldoin", 500, 3500, 100,
AXP717_CLDO1_CONTROL, AXP717_LDO_V_OUT_MASK,
AXP717_LDO1_OUTPUT_CONTROL, BIT(0)),
- AXP_DESC(AXP717, CLDO2, "cldo2", "vin1", 500, 3500, 100,
+ AXP_DESC(AXP717, CLDO2, "cldo2", "cldoin", 500, 3500, 100,
AXP717_CLDO2_CONTROL, AXP717_LDO_V_OUT_MASK,
AXP717_LDO1_OUTPUT_CONTROL, BIT(1)),
- AXP_DESC(AXP717, CLDO3, "cldo3", "vin1", 500, 3500, 100,
+ AXP_DESC(AXP717, CLDO3, "cldo3", "cldoin", 500, 3500, 100,
AXP717_CLDO3_CONTROL, AXP717_LDO_V_OUT_MASK,
AXP717_LDO1_OUTPUT_CONTROL, BIT(2)),
- AXP_DESC(AXP717, CLDO4, "cldo4", "vin1", 500, 3500, 100,
+ AXP_DESC(AXP717, CLDO4, "cldo4", "cldoin", 500, 3500, 100,
AXP717_CLDO4_CONTROL, AXP717_LDO_V_OUT_MASK,
AXP717_LDO1_OUTPUT_CONTROL, BIT(3)),
AXP_DESC(AXP717, CPUSLDO, "cpusldo", "vin1", 500, 1400, 50,
--
2.35.8
The X-Powers AXP717 contains a boost regulator, that it meant to provide
the 5V USB VBUS voltage when the devices operates on battery.
Add the name "boost" to the regexp describing the allowed node names,
to allow the regulator to be described in the devicetree.
Signed-off-by: Andre Przywara <[email protected]>
Acked-by: Krzysztof Kozlowski <[email protected]>
---
Documentation/devicetree/bindings/mfd/x-powers,axp152.yaml | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/Documentation/devicetree/bindings/mfd/x-powers,axp152.yaml b/Documentation/devicetree/bindings/mfd/x-powers,axp152.yaml
index b8e8db0d58e9c..14ab367fc8871 100644
--- a/Documentation/devicetree/bindings/mfd/x-powers,axp152.yaml
+++ b/Documentation/devicetree/bindings/mfd/x-powers,axp152.yaml
@@ -274,7 +274,7 @@ properties:
Defines the work frequency of DC-DC in kHz.
patternProperties:
- "^(([a-f])?ldo[0-9]|dcdc[0-7a-e]|ldo(_|-)io(0|1)|(dc1)?sw|rtc(_|-)ldo|cpusldo|drivevbus|dc5ldo)$":
+ "^(([a-f])?ldo[0-9]|dcdc[0-7a-e]|ldo(_|-)io(0|1)|(dc1)?sw|rtc(_|-)ldo|cpusldo|drivevbus|dc5ldo|boost)$":
$ref: /schemas/regulator/regulator.yaml#
type: object
unevaluatedProperties: false
--
2.35.8
The AXP717 also contains a boost regulator, to provide the 5V USB VBUS
rail when running on battery.
Add the registers to the MFD description to be able to use them from the
regulator driver.
Signed-off-by: Andre Przywara <[email protected]>
Reviewed-by: John Watts <[email protected]>
---
drivers/mfd/axp20x.c | 2 ++
include/linux/mfd/axp20x.h | 2 ++
2 files changed, 4 insertions(+)
diff --git a/drivers/mfd/axp20x.c b/drivers/mfd/axp20x.c
index d8ad4e120d379..02513b1eff2e8 100644
--- a/drivers/mfd/axp20x.c
+++ b/drivers/mfd/axp20x.c
@@ -209,6 +209,8 @@ static const struct regmap_access_table axp313a_volatile_table = {
};
static const struct regmap_range axp717_writeable_ranges[] = {
+ regmap_reg_range(AXP717_MODULE_EN_CONTROL, AXP717_MODULE_EN_CONTROL),
+ regmap_reg_range(AXP717_BOOST_CONTROL, AXP717_BOOST_CONTROL),
regmap_reg_range(AXP717_IRQ0_EN, AXP717_IRQ4_EN),
regmap_reg_range(AXP717_IRQ0_STATE, AXP717_IRQ4_STATE),
regmap_reg_range(AXP717_DCDC_OUTPUT_CONTROL, AXP717_CPUSLDO_CONTROL),
diff --git a/include/linux/mfd/axp20x.h b/include/linux/mfd/axp20x.h
index 8c0a33a2e9ce2..4dad54fdf67ee 100644
--- a/include/linux/mfd/axp20x.h
+++ b/include/linux/mfd/axp20x.h
@@ -115,6 +115,8 @@ enum axp20x_variants {
#define AXP313A_IRQ_STATE 0x21
#define AXP717_ON_INDICATE 0x00
+#define AXP717_MODULE_EN_CONTROL 0x19
+#define AXP717_BOOST_CONTROL 0x1e
#define AXP717_IRQ0_EN 0x40
#define AXP717_IRQ1_EN 0x41
#define AXP717_IRQ2_EN 0x42
--
2.35.8
The AXP717 also contains an adjustable boost regulator, to provide the
5V USB VBUS rail when running on battery.
Add the regulator description that states the voltage range this
regulator can cover.
Signed-off-by: Andre Przywara <[email protected]>
Reviewed-by: John Watts <[email protected]>
---
drivers/regulator/axp20x-regulator.c | 4 ++++
include/linux/mfd/axp20x.h | 1 +
2 files changed, 5 insertions(+)
diff --git a/drivers/regulator/axp20x-regulator.c b/drivers/regulator/axp20x-regulator.c
index f3c447ecdc3bf..20bef3971feca 100644
--- a/drivers/regulator/axp20x-regulator.c
+++ b/drivers/regulator/axp20x-regulator.c
@@ -143,6 +143,7 @@
#define AXP717_DCDC3_NUM_VOLTAGES 103
#define AXP717_DCDC_V_OUT_MASK GENMASK(6, 0)
#define AXP717_LDO_V_OUT_MASK GENMASK(4, 0)
+#define AXP717_BOOST_V_OUT_MASK GENMASK(7, 4)
#define AXP803_PWR_OUT_DCDC1_MASK BIT_MASK(0)
#define AXP803_PWR_OUT_DCDC2_MASK BIT_MASK(1)
@@ -834,6 +835,9 @@ static const struct regulator_desc axp717_regulators[] = {
AXP_DESC(AXP717, CPUSLDO, "cpusldo", "vin1", 500, 1400, 50,
AXP717_CPUSLDO_CONTROL, AXP717_LDO_V_OUT_MASK,
AXP717_LDO1_OUTPUT_CONTROL, BIT(4)),
+ AXP_DESC(AXP717, BOOST, "boost", "vin1", 4550, 5510, 64,
+ AXP717_BOOST_CONTROL, AXP717_BOOST_V_OUT_MASK,
+ AXP717_MODULE_EN_CONTROL, BIT(4)),
};
/* DCDC ranges shared with AXP813 */
diff --git a/include/linux/mfd/axp20x.h b/include/linux/mfd/axp20x.h
index 4dad54fdf67ee..5e86b976c4caa 100644
--- a/include/linux/mfd/axp20x.h
+++ b/include/linux/mfd/axp20x.h
@@ -486,6 +486,7 @@ enum {
AXP717_CLDO3,
AXP717_CLDO4,
AXP717_CPUSLDO,
+ AXP717_BOOST,
AXP717_REG_ID_MAX,
};
--
2.35.8
On Thu, Apr 18, 2024 at 01:07:33AM +0100, Andre Przywara wrote:
> The X-Powers AXP717 PMIC has separate input supply pins for each group
> of LDOs, so they are not all using the same DCDC1 input, as described
> currently.
>
> Replace the "supply" member of each LDO description with the respective
> group supply name, so that the supply dependencies can be correctly
> described in the devicetree.
>
> Also fix two off-by-ones in the regulator macros, after some double
> checking the numbers against the datasheet. This uncovered a bug in the
> datasheet: add a comment to document this.
Hi,
This looks a lot better with the comment.
John.
Reviewed-by: John Watts <[email protected]>
>
> Fixes: d2ac3df75c3a ("regulator: axp20x: add support for the AXP717")
> Signed-off-by: Andre Przywara <[email protected]>
> ---
> drivers/regulator/axp20x-regulator.c | 33 ++++++++++++++++------------
> 1 file changed, 19 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/regulator/axp20x-regulator.c b/drivers/regulator/axp20x-regulator.c
> index 34fcdd82b2eaa..f3c447ecdc3bf 100644
> --- a/drivers/regulator/axp20x-regulator.c
> +++ b/drivers/regulator/axp20x-regulator.c
> @@ -140,7 +140,7 @@
>
> #define AXP717_DCDC1_NUM_VOLTAGES 88
> #define AXP717_DCDC2_NUM_VOLTAGES 107
> -#define AXP717_DCDC3_NUM_VOLTAGES 104
> +#define AXP717_DCDC3_NUM_VOLTAGES 103
> #define AXP717_DCDC_V_OUT_MASK GENMASK(6, 0)
> #define AXP717_LDO_V_OUT_MASK GENMASK(4, 0)
>
> @@ -763,10 +763,15 @@ static const struct linear_range axp717_dcdc1_ranges[] = {
> REGULATOR_LINEAR_RANGE(1220000, 71, 87, 20000),
> };
>
> +/*
> + * The manual says that the last voltage is 3.4V, encoded as 0b1101011 (107),
> + * but every other method proves that this is wrong, so it's really 106 that
> + * programs the final 3.4V.
> + */
> static const struct linear_range axp717_dcdc2_ranges[] = {
> REGULATOR_LINEAR_RANGE(500000, 0, 70, 10000),
> REGULATOR_LINEAR_RANGE(1220000, 71, 87, 20000),
> - REGULATOR_LINEAR_RANGE(1600000, 88, 107, 100000),
> + REGULATOR_LINEAR_RANGE(1600000, 88, 106, 100000),
> };
>
> static const struct linear_range axp717_dcdc3_ranges[] = {
> @@ -790,40 +795,40 @@ static const struct regulator_desc axp717_regulators[] = {
> AXP_DESC(AXP717, DCDC4, "dcdc4", "vin4", 1000, 3700, 100,
> AXP717_DCDC4_CONTROL, AXP717_DCDC_V_OUT_MASK,
> AXP717_DCDC_OUTPUT_CONTROL, BIT(3)),
> - AXP_DESC(AXP717, ALDO1, "aldo1", "vin1", 500, 3500, 100,
> + AXP_DESC(AXP717, ALDO1, "aldo1", "aldoin", 500, 3500, 100,
> AXP717_ALDO1_CONTROL, AXP717_LDO_V_OUT_MASK,
> AXP717_LDO0_OUTPUT_CONTROL, BIT(0)),
> - AXP_DESC(AXP717, ALDO2, "aldo2", "vin1", 500, 3500, 100,
> + AXP_DESC(AXP717, ALDO2, "aldo2", "aldoin", 500, 3500, 100,
> AXP717_ALDO2_CONTROL, AXP717_LDO_V_OUT_MASK,
> AXP717_LDO0_OUTPUT_CONTROL, BIT(1)),
> - AXP_DESC(AXP717, ALDO3, "aldo3", "vin1", 500, 3500, 100,
> + AXP_DESC(AXP717, ALDO3, "aldo3", "aldoin", 500, 3500, 100,
> AXP717_ALDO3_CONTROL, AXP717_LDO_V_OUT_MASK,
> AXP717_LDO0_OUTPUT_CONTROL, BIT(2)),
> - AXP_DESC(AXP717, ALDO4, "aldo4", "vin1", 500, 3500, 100,
> + AXP_DESC(AXP717, ALDO4, "aldo4", "aldoin", 500, 3500, 100,
> AXP717_ALDO4_CONTROL, AXP717_LDO_V_OUT_MASK,
> AXP717_LDO0_OUTPUT_CONTROL, BIT(3)),
> - AXP_DESC(AXP717, BLDO1, "bldo1", "vin1", 500, 3500, 100,
> + AXP_DESC(AXP717, BLDO1, "bldo1", "bldoin", 500, 3500, 100,
> AXP717_BLDO1_CONTROL, AXP717_LDO_V_OUT_MASK,
> AXP717_LDO0_OUTPUT_CONTROL, BIT(4)),
> - AXP_DESC(AXP717, BLDO2, "bldo2", "vin1", 500, 3500, 100,
> + AXP_DESC(AXP717, BLDO2, "bldo2", "bldoin", 500, 3500, 100,
> AXP717_BLDO2_CONTROL, AXP717_LDO_V_OUT_MASK,
> AXP717_LDO0_OUTPUT_CONTROL, BIT(5)),
> - AXP_DESC(AXP717, BLDO3, "bldo3", "vin1", 500, 3500, 100,
> + AXP_DESC(AXP717, BLDO3, "bldo3", "bldoin", 500, 3500, 100,
> AXP717_BLDO3_CONTROL, AXP717_LDO_V_OUT_MASK,
> AXP717_LDO0_OUTPUT_CONTROL, BIT(6)),
> - AXP_DESC(AXP717, BLDO4, "bldo4", "vin1", 500, 3500, 100,
> + AXP_DESC(AXP717, BLDO4, "bldo4", "bldoin", 500, 3500, 100,
> AXP717_BLDO4_CONTROL, AXP717_LDO_V_OUT_MASK,
> AXP717_LDO0_OUTPUT_CONTROL, BIT(7)),
> - AXP_DESC(AXP717, CLDO1, "cldo1", "vin1", 500, 3500, 100,
> + AXP_DESC(AXP717, CLDO1, "cldo1", "cldoin", 500, 3500, 100,
> AXP717_CLDO1_CONTROL, AXP717_LDO_V_OUT_MASK,
> AXP717_LDO1_OUTPUT_CONTROL, BIT(0)),
> - AXP_DESC(AXP717, CLDO2, "cldo2", "vin1", 500, 3500, 100,
> + AXP_DESC(AXP717, CLDO2, "cldo2", "cldoin", 500, 3500, 100,
> AXP717_CLDO2_CONTROL, AXP717_LDO_V_OUT_MASK,
> AXP717_LDO1_OUTPUT_CONTROL, BIT(1)),
> - AXP_DESC(AXP717, CLDO3, "cldo3", "vin1", 500, 3500, 100,
> + AXP_DESC(AXP717, CLDO3, "cldo3", "cldoin", 500, 3500, 100,
> AXP717_CLDO3_CONTROL, AXP717_LDO_V_OUT_MASK,
> AXP717_LDO1_OUTPUT_CONTROL, BIT(2)),
> - AXP_DESC(AXP717, CLDO4, "cldo4", "vin1", 500, 3500, 100,
> + AXP_DESC(AXP717, CLDO4, "cldo4", "cldoin", 500, 3500, 100,
> AXP717_CLDO4_CONTROL, AXP717_LDO_V_OUT_MASK,
> AXP717_LDO1_OUTPUT_CONTROL, BIT(3)),
> AXP_DESC(AXP717, CPUSLDO, "cpusldo", "vin1", 500, 1400, 50,
> --
> 2.35.8
>
On Thu, Apr 18, 2024 at 01:07:32AM +0100, Andre Przywara wrote:
> While we list the "IRQ status *and acknowledge*" registers as volatile,
> they are missing from the writable range array, so acknowledging any
> interrupts was met with an -EIO error.
>
> Add the five registers that hold those bits to the writable array.
Hi,
This change looks good to me. Checked against the datasheet.
John.
Reviewed-by: John Watts <[email protected]>
>
> Fixes: b5bfc8ab2484 ("mfd: axp20x: Add support for AXP717 PMIC")
> Reported-by: Chris Morgan <[email protected]>
> Signed-off-by: Andre Przywara <[email protected]>
> ---
> drivers/mfd/axp20x.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/drivers/mfd/axp20x.c b/drivers/mfd/axp20x.c
> index 48ce6ea693cea..d8ad4e120d379 100644
> --- a/drivers/mfd/axp20x.c
> +++ b/drivers/mfd/axp20x.c
> @@ -210,6 +210,7 @@ static const struct regmap_access_table axp313a_volatile_table = {
>
> static const struct regmap_range axp717_writeable_ranges[] = {
> regmap_reg_range(AXP717_IRQ0_EN, AXP717_IRQ4_EN),
> + regmap_reg_range(AXP717_IRQ0_STATE, AXP717_IRQ4_STATE),
> regmap_reg_range(AXP717_DCDC_OUTPUT_CONTROL, AXP717_CPUSLDO_CONTROL),
> };
>
> --
> 2.35.8
>
On Thu, Apr 18, 2024 at 01:07:31AM +0100, Andre Przywara wrote:
> This is v2 of the fixes to the AXP717 PMIC support series. Lee put the
> original patches in an immutable branch already, so these here go on top.
> Patch 1 is new in v2, and adds the IRQ status and acknowledge registers
Lee, this looks like more of a regulator series than a MFD one (it's got
a couple of small updates to the MFD but it's mainly focused on the
regualtor functionality) so it probably makes sense to merge via the
regulator tree?
On Thu, 18 Apr 2024, at 12:07 PM, Andre Przywara wrote:
> This is v2 of the fixes to the AXP717 PMIC support series.
<snip>
> I don't know if that's still feasible, but those two patches would be a
> good candidate to squash into the patches that they fix.
>
> The other three patches add the "boost" regulator, which is meant to
> provide the 5V USB VBUS power when operating from the battery. It's the
> usual trinity of binding/mfd/regulator patches.
> Again this could be squashed into the respective patches from the
> original series, if people agree.
>
> Please have a look and test!
>
> Based on mfd/ib-mfd-regulator-6.10, as detailed below.
Looks good here, RSB communication, regulator and 5v boost support configured via DT working well on my H700 board, established by a combination of successful device bringup and kernel reporting. Concur with the request to be squashed into the mfd-next tree for 6.10 if possible, thanks!
Ryan
Tested-by: Ryan Walklin <ryan@testtoast,com>
On Tue, 23 Apr 2024, Mark Brown wrote:
> On Thu, Apr 18, 2024 at 01:07:31AM +0100, Andre Przywara wrote:
> > This is v2 of the fixes to the AXP717 PMIC support series. Lee put the
> > original patches in an immutable branch already, so these here go on top.
> > Patch 1 is new in v2, and adds the IRQ status and acknowledge registers
>
> Lee, this looks like more of a regulator series than a MFD one (it's got
> a couple of small updates to the MFD but it's mainly focused on the
> regualtor functionality) so it probably makes sense to merge via the
> regulator tree?
I'm fine with it.
Please ensure a succinct immutable branch is available in case of future conflicts.
--
Lee Jones [李琼斯]
On Sun, 28 Apr 2024, Ryan Walklin wrote:
> On Thu, 18 Apr 2024, at 12:07 PM, Andre Przywara wrote:
> > This is v2 of the fixes to the AXP717 PMIC support series.
> <snip>
> > I don't know if that's still feasible, but those two patches would be a
> > good candidate to squash into the patches that they fix.
> >
> > The other three patches add the "boost" regulator, which is meant to
> > provide the 5V USB VBUS power when operating from the battery. It's the
> > usual trinity of binding/mfd/regulator patches.
> > Again this could be squashed into the respective patches from the
> > original series, if people agree.
> >
> > Please have a look and test!
> >
> > Based on mfd/ib-mfd-regulator-6.10, as detailed below.
>
> Looks good here, RSB communication, regulator and 5v boost support configured via DT working well on my H700 board, established by a combination of successful device bringup and kernel reporting. Concur with the request to be squashed into the mfd-next tree for 6.10 if possible, thanks!
>
> Ryan
>
> Tested-by: Ryan Walklin <ryan@testtoast,com>
This doesn't look like a valid email address.
Did you manually type it out? I suggest against doing that.
--
Lee Jones [李琼斯]
On Thu, 18 Apr 2024, Andre Przywara wrote:
> While we list the "IRQ status *and acknowledge*" registers as volatile,
> they are missing from the writable range array, so acknowledging any
> interrupts was met with an -EIO error.
>
> Add the five registers that hold those bits to the writable array.
>
> Fixes: b5bfc8ab2484 ("mfd: axp20x: Add support for AXP717 PMIC")
> Reported-by: Chris Morgan <[email protected]>
> Signed-off-by: Andre Przywara <[email protected]>
> ---
> drivers/mfd/axp20x.c | 1 +
> 1 file changed, 1 insertion(+)
Acked-by: Lee Jones <[email protected]>
> diff --git a/drivers/mfd/axp20x.c b/drivers/mfd/axp20x.c
> index 48ce6ea693cea..d8ad4e120d379 100644
> --- a/drivers/mfd/axp20x.c
> +++ b/drivers/mfd/axp20x.c
> @@ -210,6 +210,7 @@ static const struct regmap_access_table axp313a_volatile_table = {
>
> static const struct regmap_range axp717_writeable_ranges[] = {
> regmap_reg_range(AXP717_IRQ0_EN, AXP717_IRQ4_EN),
> + regmap_reg_range(AXP717_IRQ0_STATE, AXP717_IRQ4_STATE),
> regmap_reg_range(AXP717_DCDC_OUTPUT_CONTROL, AXP717_CPUSLDO_CONTROL),
> };
>
> --
> 2.35.8
>
--
Lee Jones [李琼斯]
On Thu, 18 Apr 2024, Andre Przywara wrote:
> The AXP717 also contains a boost regulator, to provide the 5V USB VBUS
> rail when running on battery.
>
> Add the registers to the MFD description to be able to use them from the
> regulator driver.
>
> Signed-off-by: Andre Przywara <[email protected]>
> Reviewed-by: John Watts <[email protected]>
> ---
> drivers/mfd/axp20x.c | 2 ++
> include/linux/mfd/axp20x.h | 2 ++
> 2 files changed, 4 insertions(+)
Acked-by: Lee Jones <[email protected]>
> diff --git a/drivers/mfd/axp20x.c b/drivers/mfd/axp20x.c
> index d8ad4e120d379..02513b1eff2e8 100644
> --- a/drivers/mfd/axp20x.c
> +++ b/drivers/mfd/axp20x.c
> @@ -209,6 +209,8 @@ static const struct regmap_access_table axp313a_volatile_table = {
> };
>
> static const struct regmap_range axp717_writeable_ranges[] = {
> + regmap_reg_range(AXP717_MODULE_EN_CONTROL, AXP717_MODULE_EN_CONTROL),
> + regmap_reg_range(AXP717_BOOST_CONTROL, AXP717_BOOST_CONTROL),
> regmap_reg_range(AXP717_IRQ0_EN, AXP717_IRQ4_EN),
> regmap_reg_range(AXP717_IRQ0_STATE, AXP717_IRQ4_STATE),
> regmap_reg_range(AXP717_DCDC_OUTPUT_CONTROL, AXP717_CPUSLDO_CONTROL),
> diff --git a/include/linux/mfd/axp20x.h b/include/linux/mfd/axp20x.h
> index 8c0a33a2e9ce2..4dad54fdf67ee 100644
> --- a/include/linux/mfd/axp20x.h
> +++ b/include/linux/mfd/axp20x.h
> @@ -115,6 +115,8 @@ enum axp20x_variants {
> #define AXP313A_IRQ_STATE 0x21
>
> #define AXP717_ON_INDICATE 0x00
> +#define AXP717_MODULE_EN_CONTROL 0x19
> +#define AXP717_BOOST_CONTROL 0x1e
> #define AXP717_IRQ0_EN 0x40
> #define AXP717_IRQ1_EN 0x41
> #define AXP717_IRQ2_EN 0x42
> --
> 2.35.8
>
--
Lee Jones [李琼斯]
On Thu, 18 Apr 2024, Andre Przywara wrote:
> The X-Powers AXP717 contains a boost regulator, that it meant to provide
> the 5V USB VBUS voltage when the devices operates on battery.
>
> Add the name "boost" to the regexp describing the allowed node names,
> to allow the regulator to be described in the devicetree.
>
> Signed-off-by: Andre Przywara <[email protected]>
> Acked-by: Krzysztof Kozlowski <[email protected]>
> ---
> Documentation/devicetree/bindings/mfd/x-powers,axp152.yaml | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
Acked-by: Lee Jones <[email protected]>
> diff --git a/Documentation/devicetree/bindings/mfd/x-powers,axp152.yaml b/Documentation/devicetree/bindings/mfd/x-powers,axp152.yaml
> index b8e8db0d58e9c..14ab367fc8871 100644
> --- a/Documentation/devicetree/bindings/mfd/x-powers,axp152.yaml
> +++ b/Documentation/devicetree/bindings/mfd/x-powers,axp152.yaml
> @@ -274,7 +274,7 @@ properties:
> Defines the work frequency of DC-DC in kHz.
>
> patternProperties:
> - "^(([a-f])?ldo[0-9]|dcdc[0-7a-e]|ldo(_|-)io(0|1)|(dc1)?sw|rtc(_|-)ldo|cpusldo|drivevbus|dc5ldo)$":
> + "^(([a-f])?ldo[0-9]|dcdc[0-7a-e]|ldo(_|-)io(0|1)|(dc1)?sw|rtc(_|-)ldo|cpusldo|drivevbus|dc5ldo|boost)$":
> $ref: /schemas/regulator/regulator.yaml#
> type: object
> unevaluatedProperties: false
> --
> 2.35.8
>
--
Lee Jones [李琼斯]
On Thu, Apr 18, 2024 at 01:07:31AM +0100, Andre Przywara wrote:
> Based on mfd/ib-mfd-regulator-6.10, as detailed below.
Please provide references like this in a form that's directly usable,
ideally the git URL plus tag format used for git pull requests. The
only references lore has to an 'ib-mfd-regulator-6.10' are to this
thread...
> prerequisite-patch-id: 2b5fb10f68e0994071fc4c7dce73db7047c23220
> prerequisite-patch-id: 5d0735de888d155b2c1cdb814e852a5852a17ec7
> prerequisite-patch-id: 29c30894b4bf0b9e1e71de065cabbd842505e248
> prerequisite-patch-id: 0ab87cbf7362b6dc2d577d2264eb9574be47b5f6
Patch IDs aren't terribly useful for reverse engineering what you're
talking about here...
On Thu, Apr 18, 2024 at 01:07:31AM +0100, Andre Przywara wrote:
> This is v2 of the fixes to the AXP717 PMIC support series. Lee put the
> original patches in an immutable branch already, so these here go on top.
Reviewed-by: Mark Brown <[email protected]>
On Thu, May 02, 2024 at 10:38:36AM +0100, Lee Jones wrote:
> > Lee, this looks like more of a regulator series than a MFD one (it's got
> > a couple of small updates to the MFD but it's mainly focused on the
> > regualtor functionality) so it probably makes sense to merge via the
> > regulator tree?
> I'm fine with it.
> Please ensure a succinct immutable branch is available in case of future conflicts.
Actually now I look again it's fixes to stuff that's only in your tree
anyway so it probably makes more sense for it to go via MFD, sorry.
On Thu, Apr 18, 2024 at 01:07:35AM +0100, Andre Przywara wrote:
> The AXP717 also contains a boost regulator, to provide the 5V USB VBUS
> rail when running on battery.
>
> Add the registers to the MFD description to be able to use them from the
> regulator driver.
>
> Signed-off-by: Andre Przywara <[email protected]>
> Reviewed-by: John Watts <[email protected]>
> ---
> drivers/mfd/axp20x.c | 2 ++
> include/linux/mfd/axp20x.h | 2 ++
> 2 files changed, 4 insertions(+)
>
> diff --git a/drivers/mfd/axp20x.c b/drivers/mfd/axp20x.c
> index d8ad4e120d379..02513b1eff2e8 100644
> --- a/drivers/mfd/axp20x.c
> +++ b/drivers/mfd/axp20x.c
> @@ -209,6 +209,8 @@ static const struct regmap_access_table axp313a_volatile_table = {
> };
>
> static const struct regmap_range axp717_writeable_ranges[] = {
> + regmap_reg_range(AXP717_MODULE_EN_CONTROL, AXP717_MODULE_EN_CONTROL),
> + regmap_reg_range(AXP717_BOOST_CONTROL, AXP717_BOOST_CONTROL),
> regmap_reg_range(AXP717_IRQ0_EN, AXP717_IRQ4_EN),
> regmap_reg_range(AXP717_IRQ0_STATE, AXP717_IRQ4_STATE),
> regmap_reg_range(AXP717_DCDC_OUTPUT_CONTROL, AXP717_CPUSLDO_CONTROL),
> diff --git a/include/linux/mfd/axp20x.h b/include/linux/mfd/axp20x.h
> index 8c0a33a2e9ce2..4dad54fdf67ee 100644
> --- a/include/linux/mfd/axp20x.h
> +++ b/include/linux/mfd/axp20x.h
> @@ -115,6 +115,8 @@ enum axp20x_variants {
> #define AXP313A_IRQ_STATE 0x21
>
> #define AXP717_ON_INDICATE 0x00
> +#define AXP717_MODULE_EN_CONTROL 0x19
Could we name this register something like "AXP717_MODULE_EN_CONTROL_2"?
We're going to need register 0x0b later for usb and battery, and that's
also marked as a control register (control_1 per the datasheet).
Thank you,
Chris
> +#define AXP717_BOOST_CONTROL 0x1e
> #define AXP717_IRQ0_EN 0x40
> #define AXP717_IRQ1_EN 0x41
> #define AXP717_IRQ2_EN 0x42
> --
> 2.35.8
>
Hi Broonie,
On 18/04/2024 01:07, Andre Przywara wrote:
> The X-Powers AXP717 PMIC has separate input supply pins for each group
> of LDOs, so they are not all using the same DCDC1 input, as described
> currently.
>
> Replace the "supply" member of each LDO description with the respective
> group supply name, so that the supply dependencies can be correctly
> described in the devicetree.
>
> Also fix two off-by-ones in the regulator macros, after some double
> checking the numbers against the datasheet. This uncovered a bug in the
> datasheet: add a comment to document this.
>
> Fixes: d2ac3df75c3a ("regulator: axp20x: add support for the AXP717")
> Signed-off-by: Andre Przywara <[email protected]>
Can you please take (at least) this one patch as a fix for 6.10? Applies
cleanly on top of v6.10-rc3. The changes are not super-critical, but
worth fixing nevertheless.
Thanks,
Andre
> ---
> drivers/regulator/axp20x-regulator.c | 33 ++++++++++++++++------------
> 1 file changed, 19 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/regulator/axp20x-regulator.c b/drivers/regulator/axp20x-regulator.c
> index 34fcdd82b2eaa..f3c447ecdc3bf 100644
> --- a/drivers/regulator/axp20x-regulator.c
> +++ b/drivers/regulator/axp20x-regulator.c
> @@ -140,7 +140,7 @@
>
> #define AXP717_DCDC1_NUM_VOLTAGES 88
> #define AXP717_DCDC2_NUM_VOLTAGES 107
> -#define AXP717_DCDC3_NUM_VOLTAGES 104
> +#define AXP717_DCDC3_NUM_VOLTAGES 103
> #define AXP717_DCDC_V_OUT_MASK GENMASK(6, 0)
> #define AXP717_LDO_V_OUT_MASK GENMASK(4, 0)
>
> @@ -763,10 +763,15 @@ static const struct linear_range axp717_dcdc1_ranges[] = {
> REGULATOR_LINEAR_RANGE(1220000, 71, 87, 20000),
> };
>
> +/*
> + * The manual says that the last voltage is 3.4V, encoded as 0b1101011 (107),
> + * but every other method proves that this is wrong, so it's really 106 that
> + * programs the final 3.4V.
> + */
> static const struct linear_range axp717_dcdc2_ranges[] = {
> REGULATOR_LINEAR_RANGE(500000, 0, 70, 10000),
> REGULATOR_LINEAR_RANGE(1220000, 71, 87, 20000),
> - REGULATOR_LINEAR_RANGE(1600000, 88, 107, 100000),
> + REGULATOR_LINEAR_RANGE(1600000, 88, 106, 100000),
> };
>
> static const struct linear_range axp717_dcdc3_ranges[] = {
> @@ -790,40 +795,40 @@ static const struct regulator_desc axp717_regulators[] = {
> AXP_DESC(AXP717, DCDC4, "dcdc4", "vin4", 1000, 3700, 100,
> AXP717_DCDC4_CONTROL, AXP717_DCDC_V_OUT_MASK,
> AXP717_DCDC_OUTPUT_CONTROL, BIT(3)),
> - AXP_DESC(AXP717, ALDO1, "aldo1", "vin1", 500, 3500, 100,
> + AXP_DESC(AXP717, ALDO1, "aldo1", "aldoin", 500, 3500, 100,
> AXP717_ALDO1_CONTROL, AXP717_LDO_V_OUT_MASK,
> AXP717_LDO0_OUTPUT_CONTROL, BIT(0)),
> - AXP_DESC(AXP717, ALDO2, "aldo2", "vin1", 500, 3500, 100,
> + AXP_DESC(AXP717, ALDO2, "aldo2", "aldoin", 500, 3500, 100,
> AXP717_ALDO2_CONTROL, AXP717_LDO_V_OUT_MASK,
> AXP717_LDO0_OUTPUT_CONTROL, BIT(1)),
> - AXP_DESC(AXP717, ALDO3, "aldo3", "vin1", 500, 3500, 100,
> + AXP_DESC(AXP717, ALDO3, "aldo3", "aldoin", 500, 3500, 100,
> AXP717_ALDO3_CONTROL, AXP717_LDO_V_OUT_MASK,
> AXP717_LDO0_OUTPUT_CONTROL, BIT(2)),
> - AXP_DESC(AXP717, ALDO4, "aldo4", "vin1", 500, 3500, 100,
> + AXP_DESC(AXP717, ALDO4, "aldo4", "aldoin", 500, 3500, 100,
> AXP717_ALDO4_CONTROL, AXP717_LDO_V_OUT_MASK,
> AXP717_LDO0_OUTPUT_CONTROL, BIT(3)),
> - AXP_DESC(AXP717, BLDO1, "bldo1", "vin1", 500, 3500, 100,
> + AXP_DESC(AXP717, BLDO1, "bldo1", "bldoin", 500, 3500, 100,
> AXP717_BLDO1_CONTROL, AXP717_LDO_V_OUT_MASK,
> AXP717_LDO0_OUTPUT_CONTROL, BIT(4)),
> - AXP_DESC(AXP717, BLDO2, "bldo2", "vin1", 500, 3500, 100,
> + AXP_DESC(AXP717, BLDO2, "bldo2", "bldoin", 500, 3500, 100,
> AXP717_BLDO2_CONTROL, AXP717_LDO_V_OUT_MASK,
> AXP717_LDO0_OUTPUT_CONTROL, BIT(5)),
> - AXP_DESC(AXP717, BLDO3, "bldo3", "vin1", 500, 3500, 100,
> + AXP_DESC(AXP717, BLDO3, "bldo3", "bldoin", 500, 3500, 100,
> AXP717_BLDO3_CONTROL, AXP717_LDO_V_OUT_MASK,
> AXP717_LDO0_OUTPUT_CONTROL, BIT(6)),
> - AXP_DESC(AXP717, BLDO4, "bldo4", "vin1", 500, 3500, 100,
> + AXP_DESC(AXP717, BLDO4, "bldo4", "bldoin", 500, 3500, 100,
> AXP717_BLDO4_CONTROL, AXP717_LDO_V_OUT_MASK,
> AXP717_LDO0_OUTPUT_CONTROL, BIT(7)),
> - AXP_DESC(AXP717, CLDO1, "cldo1", "vin1", 500, 3500, 100,
> + AXP_DESC(AXP717, CLDO1, "cldo1", "cldoin", 500, 3500, 100,
> AXP717_CLDO1_CONTROL, AXP717_LDO_V_OUT_MASK,
> AXP717_LDO1_OUTPUT_CONTROL, BIT(0)),
> - AXP_DESC(AXP717, CLDO2, "cldo2", "vin1", 500, 3500, 100,
> + AXP_DESC(AXP717, CLDO2, "cldo2", "cldoin", 500, 3500, 100,
> AXP717_CLDO2_CONTROL, AXP717_LDO_V_OUT_MASK,
> AXP717_LDO1_OUTPUT_CONTROL, BIT(1)),
> - AXP_DESC(AXP717, CLDO3, "cldo3", "vin1", 500, 3500, 100,
> + AXP_DESC(AXP717, CLDO3, "cldo3", "cldoin", 500, 3500, 100,
> AXP717_CLDO3_CONTROL, AXP717_LDO_V_OUT_MASK,
> AXP717_LDO1_OUTPUT_CONTROL, BIT(2)),
> - AXP_DESC(AXP717, CLDO4, "cldo4", "vin1", 500, 3500, 100,
> + AXP_DESC(AXP717, CLDO4, "cldo4", "cldoin", 500, 3500, 100,
> AXP717_CLDO4_CONTROL, AXP717_LDO_V_OUT_MASK,
> AXP717_LDO1_OUTPUT_CONTROL, BIT(3)),
> AXP_DESC(AXP717, CPUSLDO, "cpusldo", "vin1", 500, 1400, 50,
Hi Lee,
On 02/05/2024 10:39, Lee Jones wrote:
> On Thu, 18 Apr 2024, Andre Przywara wrote:
>
>> While we list the "IRQ status *and acknowledge*" registers as volatile,
>> they are missing from the writable range array, so acknowledging any
>> interrupts was met with an -EIO error.
>>
>> Add the five registers that hold those bits to the writable array.
>>
>> Fixes: b5bfc8ab2484 ("mfd: axp20x: Add support for AXP717 PMIC")
>> Reported-by: Chris Morgan <[email protected]>
>> Signed-off-by: Andre Przywara <[email protected]>
>> ---
>> drivers/mfd/axp20x.c | 1 +
>> 1 file changed, 1 insertion(+)
>
> Acked-by: Lee Jones <[email protected]>
Can you please take just this patch as a fix for 6.10? This fixes the
power key operation.
This applies cleanly on top of v6.10-rc3, so there is no need for any
extra immutable branch or coordination with regulator.
(The same is true independently for patch 2/5, on the regulator side).
Cheers,
Andre
>
>> diff --git a/drivers/mfd/axp20x.c b/drivers/mfd/axp20x.c
>> index 48ce6ea693cea..d8ad4e120d379 100644
>> --- a/drivers/mfd/axp20x.c
>> +++ b/drivers/mfd/axp20x.c
>> @@ -210,6 +210,7 @@ static const struct regmap_access_table axp313a_volatile_table = {
>>
>> static const struct regmap_range axp717_writeable_ranges[] = {
>> regmap_reg_range(AXP717_IRQ0_EN, AXP717_IRQ4_EN),
>> + regmap_reg_range(AXP717_IRQ0_STATE, AXP717_IRQ4_STATE),
>> regmap_reg_range(AXP717_DCDC_OUTPUT_CONTROL, AXP717_CPUSLDO_CONTROL),
>> };
>>
>> --
>> 2.35.8
>>
>
On Wed, 12 Jun 2024, Andre Przywara wrote:
> Hi Lee,
>
> On 02/05/2024 10:39, Lee Jones wrote:
> > On Thu, 18 Apr 2024, Andre Przywara wrote:
> >
> > > While we list the "IRQ status *and acknowledge*" registers as volatile,
> > > they are missing from the writable range array, so acknowledging any
> > > interrupts was met with an -EIO error.
> > >
> > > Add the five registers that hold those bits to the writable array.
> > >
> > > Fixes: b5bfc8ab2484 ("mfd: axp20x: Add support for AXP717 PMIC")
> > > Reported-by: Chris Morgan <[email protected]>
> > > Signed-off-by: Andre Przywara <[email protected]>
> > > ---
> > > drivers/mfd/axp20x.c | 1 +
> > > 1 file changed, 1 insertion(+)
> >
> > Acked-by: Lee Jones <[email protected]>
>
> Can you please take just this patch as a fix for 6.10? This fixes the power
> key operation.
> This applies cleanly on top of v6.10-rc3, so there is no need for any extra
> immutable branch or coordination with regulator.
> (The same is true independently for patch 2/5, on the regulator side).
What does the Fixes: commit break?
Or is it the case that it never worked properly?
--
Lee Jones [李琼斯]
Hi,
On 12/06/2024 16:25, Lee Jones wrote:
> On Wed, 12 Jun 2024, Andre Przywara wrote:
>
>> Hi Lee,
>>
>> On 02/05/2024 10:39, Lee Jones wrote:
>>> On Thu, 18 Apr 2024, Andre Przywara wrote:
>>>
>>>> While we list the "IRQ status *and acknowledge*" registers as volatile,
>>>> they are missing from the writable range array, so acknowledging any
>>>> interrupts was met with an -EIO error.
>>>>
>>>> Add the five registers that hold those bits to the writable array.
>>>>
>>>> Fixes: b5bfc8ab2484 ("mfd: axp20x: Add support for AXP717 PMIC")
>>>> Reported-by: Chris Morgan <[email protected]>
>>>> Signed-off-by: Andre Przywara <[email protected]>
>>>> ---
>>>> drivers/mfd/axp20x.c | 1 +
>>>> 1 file changed, 1 insertion(+)
>>>
>>> Acked-by: Lee Jones <[email protected]>
>>
>> Can you please take just this patch as a fix for 6.10? This fixes the power
>> key operation.
>> This applies cleanly on top of v6.10-rc3, so there is no need for any extra
>> immutable branch or coordination with regulator.
>> (The same is true independently for patch 2/5, on the regulator side).
>
> What does the Fixes: commit break?
>
> Or is it the case that it never worked properly?
The interrupt part never worked properly, but so far that's only needed
for the power key operation. Unfortunately that part wasn't tested
properly initially, so the patches were merged into your tree before that.
Cheers,
Andre
On Wed, 12 Jun 2024, Andre Przywara wrote:
> Hi,
>
> On 12/06/2024 16:25, Lee Jones wrote:
> > On Wed, 12 Jun 2024, Andre Przywara wrote:
> >
> > > Hi Lee,
> > >
> > > On 02/05/2024 10:39, Lee Jones wrote:
> > > > On Thu, 18 Apr 2024, Andre Przywara wrote:
> > > >
> > > > > While we list the "IRQ status *and acknowledge*" registers as volatile,
> > > > > they are missing from the writable range array, so acknowledging any
> > > > > interrupts was met with an -EIO error.
> > > > >
> > > > > Add the five registers that hold those bits to the writable array.
> > > > >
> > > > > Fixes: b5bfc8ab2484 ("mfd: axp20x: Add support for AXP717 PMIC")
> > > > > Reported-by: Chris Morgan <[email protected]>
> > > > > Signed-off-by: Andre Przywara <[email protected]>
> > > > > ---
> > > > > drivers/mfd/axp20x.c | 1 +
> > > > > 1 file changed, 1 insertion(+)
> > > >
> > > > Acked-by: Lee Jones <[email protected]>
> > >
> > > Can you please take just this patch as a fix for 6.10? This fixes the power
> > > key operation.
> > > This applies cleanly on top of v6.10-rc3, so there is no need for any extra
> > > immutable branch or coordination with regulator.
> > > (The same is true independently for patch 2/5, on the regulator side).
> >
> > What does the Fixes: commit break?
> >
> > Or is it the case that it never worked properly?
>
> The interrupt part never worked properly, but so far that's only needed for
> the power key operation. Unfortunately that part wasn't tested properly
> initially, so the patches were merged into your tree before that.
This doesn't sounds like a -fixes candidate. I'll mark the set for v6.11.
--
Lee Jones [李琼斯]
Hi Lee,
On 12/06/2024 16:48, Lee Jones wrote:
> On Wed, 12 Jun 2024, Andre Przywara wrote:
>
>> Hi,
>>
>> On 12/06/2024 16:25, Lee Jones wrote:
>>> On Wed, 12 Jun 2024, Andre Przywara wrote:
>>>
>>>> Hi Lee,
>>>>
>>>> On 02/05/2024 10:39, Lee Jones wrote:
>>>>> On Thu, 18 Apr 2024, Andre Przywara wrote:
>>>>>
>>>>>> While we list the "IRQ status *and acknowledge*" registers as volatile,
>>>>>> they are missing from the writable range array, so acknowledging any
>>>>>> interrupts was met with an -EIO error.
>>>>>>
>>>>>> Add the five registers that hold those bits to the writable array.
>>>>>>
>>>>>> Fixes: b5bfc8ab2484 ("mfd: axp20x: Add support for AXP717 PMIC")
>>>>>> Reported-by: Chris Morgan <[email protected]>
>>>>>> Signed-off-by: Andre Przywara <[email protected]>
>>>>>> ---
>>>>>> drivers/mfd/axp20x.c | 1 +
>>>>>> 1 file changed, 1 insertion(+)
>>>>>
>>>>> Acked-by: Lee Jones <[email protected]>
>>>>
>>>> Can you please take just this patch as a fix for 6.10? This fixes the power
>>>> key operation.
>>>> This applies cleanly on top of v6.10-rc3, so there is no need for any extra
>>>> immutable branch or coordination with regulator.
>>>> (The same is true independently for patch 2/5, on the regulator side).
>>>
>>> What does the Fixes: commit break?
>>>
>>> Or is it the case that it never worked properly?
>>
>> The interrupt part never worked properly, but so far that's only needed for
>> the power key operation. Unfortunately that part wasn't tested properly
>> initially, so the patches were merged into your tree before that.
>
> This doesn't sounds like a -fixes candidate. I'll mark the set for v6.11.
Sorry, correction, this patch missing is actually fatal now, since we
have an interrupt connected in the DT (which wasn't there initially).
The code tries to clear all IRQs upon driver probe, which fails due to
regmap error-ing out. This makes the whole driver fail probing, and
since the AXP supplies basically every peripheral, the system is dead in
the water:
[ 1.173014] sunxi-rsb 7083000.rsb: RSB running at 3000000 Hz
[ 1.174996] axp20x-rsb sunxi-rsb-3a3: AXP20x variant AXP717 found
[ 1.198931] axp20x-rsb sunxi-rsb-3a3: Failed to ack 0x49: -5
[ 1.220878] axp20x-rsb sunxi-rsb-3a3: failed to add irq chip: -5
[ 1.235760] axp20x-rsb sunxi-rsb-3a3:
(Thanks to loki666@IRC for providing the log!)
This was discovered early, long before the merge window, and I was
actually hoping to have this patch squashed into the original series
still, but there was this immutable branch already.
So can you please take this as a fix for 6.10?
Cheers,
Andre
On Thu, 13 Jun 2024, Andre Przywara wrote:
> Hi Lee,
>
> On 12/06/2024 16:48, Lee Jones wrote:
> > On Wed, 12 Jun 2024, Andre Przywara wrote:
> >
> > > Hi,
> > >
> > > On 12/06/2024 16:25, Lee Jones wrote:
> > > > On Wed, 12 Jun 2024, Andre Przywara wrote:
> > > >
> > > > > Hi Lee,
> > > > >
> > > > > On 02/05/2024 10:39, Lee Jones wrote:
> > > > > > On Thu, 18 Apr 2024, Andre Przywara wrote:
> > > > > >
> > > > > > > While we list the "IRQ status *and acknowledge*" registers as volatile,
> > > > > > > they are missing from the writable range array, so acknowledging any
> > > > > > > interrupts was met with an -EIO error.
> > > > > > >
> > > > > > > Add the five registers that hold those bits to the writable array.
> > > > > > >
> > > > > > > Fixes: b5bfc8ab2484 ("mfd: axp20x: Add support for AXP717 PMIC")
> > > > > > > Reported-by: Chris Morgan <[email protected]>
> > > > > > > Signed-off-by: Andre Przywara <[email protected]>
> > > > > > > ---
> > > > > > > drivers/mfd/axp20x.c | 1 +
> > > > > > > 1 file changed, 1 insertion(+)
> > > > > >
> > > > > > Acked-by: Lee Jones <[email protected]>
> > > > >
> > > > > Can you please take just this patch as a fix for 6.10? This fixes the power
> > > > > key operation.
> > > > > This applies cleanly on top of v6.10-rc3, so there is no need for any extra
> > > > > immutable branch or coordination with regulator.
> > > > > (The same is true independently for patch 2/5, on the regulator side).
> > > >
> > > > What does the Fixes: commit break?
> > > >
> > > > Or is it the case that it never worked properly?
> > >
> > > The interrupt part never worked properly, but so far that's only needed for
> > > the power key operation. Unfortunately that part wasn't tested properly
> > > initially, so the patches were merged into your tree before that.
> >
> > This doesn't sounds like a -fixes candidate. I'll mark the set for v6.11.
>
> Sorry, correction, this patch missing is actually fatal now, since we have
> an interrupt connected in the DT (which wasn't there initially). The code
> tries to clear all IRQs upon driver probe, which fails due to regmap
> error-ing out. This makes the whole driver fail probing, and since the AXP
> supplies basically every peripheral, the system is dead in the water:
>
> [ 1.173014] sunxi-rsb 7083000.rsb: RSB running at 3000000 Hz
> [ 1.174996] axp20x-rsb sunxi-rsb-3a3: AXP20x variant AXP717 found
> [ 1.198931] axp20x-rsb sunxi-rsb-3a3: Failed to ack 0x49: -5
> [ 1.220878] axp20x-rsb sunxi-rsb-3a3: failed to add irq chip: -5
> [ 1.235760] axp20x-rsb sunxi-rsb-3a3:
>
> (Thanks to loki666@IRC for providing the log!)
>
> This was discovered early, long before the merge window, and I was actually
> hoping to have this patch squashed into the original series still, but there
> was this immutable branch already.
>
> So can you please take this as a fix for 6.10?
Please draft a new patch (the diff is likely to be the same) with an
updated commit message describing the new problem and why it's required
for -fixes. I'll then submit it to Linus.
--
Lee Jones [李琼斯]
On Wed, 12 Jun 2024, Andre Przywara wrote:
> Hi Broonie,
>
> On 18/04/2024 01:07, Andre Przywara wrote:
> > The X-Powers AXP717 PMIC has separate input supply pins for each group
> > of LDOs, so they are not all using the same DCDC1 input, as described
> > currently.
> >
> > Replace the "supply" member of each LDO description with the respective
> > group supply name, so that the supply dependencies can be correctly
> > described in the devicetree.
> >
> > Also fix two off-by-ones in the regulator macros, after some double
> > checking the numbers against the datasheet. This uncovered a bug in the
> > datasheet: add a comment to document this.
> >
> > Fixes: d2ac3df75c3a ("regulator: axp20x: add support for the AXP717")
> > Signed-off-by: Andre Przywara <[email protected]>
>
> Can you please take (at least) this one patch as a fix for 6.10? Applies
> cleanly on top of v6.10-rc3. The changes are not super-critical, but worth
> fixing nevertheless.
FWIW, I have no idea what's going on with this set now.
Once all of the necessary patches have been expedited, please gather
together what's left and I'll take them via MFD.
--
Lee Jones [李琼斯]
On Thu, 18 Apr 2024 01:07:31 +0100, Andre Przywara wrote:
> This is v2 of the fixes to the AXP717 PMIC support series. Lee put the
> original patches in an immutable branch already, so these here go on top.
> Patch 1 is new in v2, and adds the IRQ status and acknowledge registers
> to the writable range. Thanks to Chris for pointing this out.
> Patch 2 contains fixes to the regulator descriptions: the LDOs had the
> wrong supply source, and two numbers were wrong. The datasheet describes
> the voltage ranges and register values differently from what our macros
> expect, in a way that literally begs for off-by-ones, so here you go.
> Also there is an actual wrong number in the datasheet, add a comment to
> document this.
> I don't know if that's still feasible, but those two patches would be a
> good candidate to squash into the patches that they fix.
>
> [...]
Applied to
https://git.kernel.org/pub/scm/linux/kernel/git/broonie/regulator.git for-next
Thanks!
[2/5] regulator: axp20x: AXP717: fix LDO supply rails and off-by-ones
commit: 0057222c45140830a7bf55e92fb67f84a2814f67
All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.
You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.
If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.
Please add any relevant lists and maintainers to the CCs when replying
to this mail.
Thanks,
Mark
On Fri, 14 Jun 2024 09:12:24 +0100
Lee Jones <[email protected]> wrote:
Hi Lee,
> On Wed, 12 Jun 2024, Andre Przywara wrote:
>
> > Hi Broonie,
> >
> > On 18/04/2024 01:07, Andre Przywara wrote:
> > > The X-Powers AXP717 PMIC has separate input supply pins for each group
> > > of LDOs, so they are not all using the same DCDC1 input, as described
> > > currently.
> > >
> > > Replace the "supply" member of each LDO description with the respective
> > > group supply name, so that the supply dependencies can be correctly
> > > described in the devicetree.
> > >
> > > Also fix two off-by-ones in the regulator macros, after some double
> > > checking the numbers against the datasheet. This uncovered a bug in the
> > > datasheet: add a comment to document this.
> > >
> > > Fixes: d2ac3df75c3a ("regulator: axp20x: add support for the AXP717")
> > > Signed-off-by: Andre Przywara <[email protected]>
> >
> > Can you please take (at least) this one patch as a fix for 6.10? Applies
> > cleanly on top of v6.10-rc3. The changes are not super-critical, but worth
> > fixing nevertheless.
>
> FWIW, I have no idea what's going on with this set now.
>
> Once all of the necessary patches have been expedited, please gather
> together what's left and I'll take them via MFD.
Yes, we dropped the ball on this one a bit, apologies.
Broonie just took 2/5, so I will resend 3/5-5/5 as a new series, targeting
6.11.
Cheers,
Andre.