diff mbox series

[v10,3/3] regulator: axp20x: Add support for AXP313a variant

Message ID 20230401001850.4988-4-andre.przywara@arm.com
State New
Headers
Series regulator: Add X-Powers AXP313a PMIC support |

Commit Message

Andre Przywara April 1, 2023, 12:18 a.m. UTC 
  From: Martin Botka <martin.botka@somainline.org>

The AXP313a is your typical I2C controlled PMIC, although in a lighter
fashion compared to the other X-Powers PMICs: it has only three DCDC
rails, three LDOs, and no battery charging support.

The AXP313a datasheet does not describe a register to change the DCDC
switching frequency, and talks of it being fixed at 3 MHz. Check that
the property allowing to change that frequency is absent from the DT,
and bail out otherwise.

The third LDO, RTCLDO, is fixed, and cannot even be turned on or off,
programmatically. On top of that, its voltage is customisable (either
1.8V or 3.3V), which we cannot describe easily using the existing
regulator wrapper functions. This should be fixed properly, using
regulator-{min,max}-microvolt in the DT, but this requires more changes
to the code. As some other PMICs (AXP2xx, AXP803) seem to paper over the
same problem as well, we follow suit here and pretend it's a fixed 1.8V
regulator. A proper fix can follow later. The BSP code seems to ignore
this regulator altogether.

Describe the AXP313A's voltage settings and switch registers, how the
voltages are encoded, and connect this to the MFD device via its
regulator ID.

Signed-off-by: Martin Botka <martin.botka@somainline.org>
Signed-off-by: Andre Przywara <andre.przywara@arm.com>
---
 drivers/regulator/axp20x-regulator.c | 60 ++++++++++++++++++++++++++++
 1 file changed, 60 insertions(+)

Comments

Chen-Yu Tsai April 2, 2023, 6:13 a.m. UTC | #1 
On Sat, Apr 1, 2023 at 8:19 AM Andre Przywara <andre.przywara@arm.com> wrote:
>
> From: Martin Botka <martin.botka@somainline.org>
>
> The AXP313a is your typical I2C controlled PMIC, although in a lighter
> fashion compared to the other X-Powers PMICs: it has only three DCDC
> rails, three LDOs, and no battery charging support.
>
> The AXP313a datasheet does not describe a register to change the DCDC
> switching frequency, and talks of it being fixed at 3 MHz. Check that
> the property allowing to change that frequency is absent from the DT,
> and bail out otherwise.
>
> The third LDO, RTCLDO, is fixed, and cannot even be turned on or off,
> programmatically. On top of that, its voltage is customisable (either
> 1.8V or 3.3V), which we cannot describe easily using the existing
> regulator wrapper functions. This should be fixed properly, using
> regulator-{min,max}-microvolt in the DT, but this requires more changes
> to the code. As some other PMICs (AXP2xx, AXP803) seem to paper over the
> same problem as well, we follow suit here and pretend it's a fixed 1.8V
> regulator. A proper fix can follow later. The BSP code seems to ignore
> this regulator altogether.
>
> Describe the AXP313A's voltage settings and switch registers, how the
> voltages are encoded, and connect this to the MFD device via its
> regulator ID.
>
> Signed-off-by: Martin Botka <martin.botka@somainline.org>
> Signed-off-by: Andre Przywara <andre.przywara@arm.com>

Reviewed-by: Chen-Yu Tsai <wens@csie.org>
Lee Jones April 5, 2023, 2:21 p.m. UTC | #2 
Mark,

On Sat, 01 Apr 2023, Andre Przywara wrote:

> From: Martin Botka <martin.botka@somainline.org>
>
> The AXP313a is your typical I2C controlled PMIC, although in a lighter
> fashion compared to the other X-Powers PMICs: it has only three DCDC
> rails, three LDOs, and no battery charging support.
>
> The AXP313a datasheet does not describe a register to change the DCDC
> switching frequency, and talks of it being fixed at 3 MHz. Check that
> the property allowing to change that frequency is absent from the DT,
> and bail out otherwise.
>
> The third LDO, RTCLDO, is fixed, and cannot even be turned on or off,
> programmatically. On top of that, its voltage is customisable (either
> 1.8V or 3.3V), which we cannot describe easily using the existing
> regulator wrapper functions. This should be fixed properly, using
> regulator-{min,max}-microvolt in the DT, but this requires more changes
> to the code. As some other PMICs (AXP2xx, AXP803) seem to paper over the
> same problem as well, we follow suit here and pretend it's a fixed 1.8V
> regulator. A proper fix can follow later. The BSP code seems to ignore
> this regulator altogether.
>
> Describe the AXP313A's voltage settings and switch registers, how the
> voltages are encoded, and connect this to the MFD device via its
> regulator ID.
>
> Signed-off-by: Martin Botka <martin.botka@somainline.org>
> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> ---
>  drivers/regulator/axp20x-regulator.c | 60 ++++++++++++++++++++++++++++
>  1 file changed, 60 insertions(+)

Is this on your radar?

Can I take the other two patches without causing issues?

> diff --git a/drivers/regulator/axp20x-regulator.c b/drivers/regulator/axp20x-regulator.c
> index d260c442b788d..81f339009df9b 100644
> --- a/drivers/regulator/axp20x-regulator.c
> +++ b/drivers/regulator/axp20x-regulator.c
> @@ -134,6 +134,11 @@
>  #define AXP22X_PWR_OUT_DLDO4_MASK	BIT_MASK(6)
>  #define AXP22X_PWR_OUT_ALDO3_MASK	BIT_MASK(7)
>
> +#define AXP313A_DCDC1_NUM_VOLTAGES	107
> +#define AXP313A_DCDC23_NUM_VOLTAGES	88
> +#define AXP313A_DCDC_V_OUT_MASK		GENMASK(6, 0)
> +#define AXP313A_LDO_V_OUT_MASK		GENMASK(4, 0)
> +
>  #define AXP803_PWR_OUT_DCDC1_MASK	BIT_MASK(0)
>  #define AXP803_PWR_OUT_DCDC2_MASK	BIT_MASK(1)
>  #define AXP803_PWR_OUT_DCDC3_MASK	BIT_MASK(2)
> @@ -638,6 +643,48 @@ static const struct regulator_desc axp22x_drivevbus_regulator = {
>  	.ops		= &axp20x_ops_sw,
>  };
>
> +static const struct linear_range axp313a_dcdc1_ranges[] = {
> +	REGULATOR_LINEAR_RANGE(500000,   0,  70,  10000),
> +	REGULATOR_LINEAR_RANGE(1220000, 71,  87,  20000),
> +	REGULATOR_LINEAR_RANGE(1600000, 88, 106, 100000),
> +};
> +
> +static const struct linear_range axp313a_dcdc2_ranges[] = {
> +	REGULATOR_LINEAR_RANGE(500000,   0, 70, 10000),
> +	REGULATOR_LINEAR_RANGE(1220000, 71, 87, 20000),
> +};
> +
> +/*
> + * This is deviating from the datasheet. The values here are taken from the
> + * BSP driver and have been confirmed by measurements.
> + */
> +static const struct linear_range axp313a_dcdc3_ranges[] = {
> +	REGULATOR_LINEAR_RANGE(500000,   0,  70, 10000),
> +	REGULATOR_LINEAR_RANGE(1220000, 71, 102, 20000),
> +};
> +
> +static const struct regulator_desc axp313a_regulators[] = {
> +	AXP_DESC_RANGES(AXP313A, DCDC1, "dcdc1", "vin1",
> +			axp313a_dcdc1_ranges, AXP313A_DCDC1_NUM_VOLTAGES,
> +			AXP313A_DCDC1_CONRTOL, AXP313A_DCDC_V_OUT_MASK,
> +			AXP313A_OUTPUT_CONTROL, BIT(0)),
> +	AXP_DESC_RANGES(AXP313A, DCDC2, "dcdc2", "vin2",
> +			axp313a_dcdc2_ranges, AXP313A_DCDC23_NUM_VOLTAGES,
> +			AXP313A_DCDC2_CONRTOL, AXP313A_DCDC_V_OUT_MASK,
> +			AXP313A_OUTPUT_CONTROL, BIT(1)),
> +	AXP_DESC_RANGES(AXP313A, DCDC3, "dcdc3", "vin3",
> +			axp313a_dcdc3_ranges, AXP313A_DCDC23_NUM_VOLTAGES,
> +			AXP313A_DCDC3_CONRTOL, AXP313A_DCDC_V_OUT_MASK,
> +			AXP313A_OUTPUT_CONTROL, BIT(2)),
> +	AXP_DESC(AXP313A, ALDO1, "aldo1", "vin1", 500, 3500, 100,
> +		 AXP313A_ALDO1_CONRTOL, AXP313A_LDO_V_OUT_MASK,
> +		 AXP313A_OUTPUT_CONTROL, BIT(3)),
> +	AXP_DESC(AXP313A, DLDO1, "dldo1", "vin1", 500, 3500, 100,
> +		 AXP313A_DLDO1_CONRTOL, AXP313A_LDO_V_OUT_MASK,
> +		 AXP313A_OUTPUT_CONTROL, BIT(4)),
> +	AXP_DESC_FIXED(AXP313A, RTC_LDO, "rtc-ldo", "vin1", 1800),
> +};
> +
>  /* DCDC ranges shared with AXP813 */
>  static const struct linear_range axp803_dcdc234_ranges[] = {
>  	REGULATOR_LINEAR_RANGE(500000,
> @@ -1040,6 +1087,15 @@ static int axp20x_set_dcdc_freq(struct platform_device *pdev, u32 dcdcfreq)
>  		def = 3000;
>  		step = 150;
>  		break;
> +	case AXP313A_ID:
> +		/* The DCDC PWM frequency seems to be fixed to 3 MHz. */
> +		if (dcdcfreq != 0) {
> +			dev_err(&pdev->dev,
> +				"DCDC frequency on AXP313a is fixed to 3 MHz.\n");
> +			return -EINVAL;
> +		}
> +
> +		return 0;
>  	default:
>  		dev_err(&pdev->dev,
>  			"Setting DCDC frequency for unsupported AXP variant\n");
> @@ -1232,6 +1288,10 @@ static int axp20x_regulator_probe(struct platform_device *pdev)
>  		drivevbus = of_property_read_bool(pdev->dev.parent->of_node,
>  						  "x-powers,drive-vbus-en");
>  		break;
> +	case AXP313A_ID:
> +		regulators = axp313a_regulators;
> +		nregulators = AXP313A_REG_ID_MAX;
> +		break;
>  	case AXP803_ID:
>  		regulators = axp803_regulators;
>  		nregulators = AXP803_REG_ID_MAX;
> --
> 2.35.7
>

--
Lee Jones [李琼斯]
Mark Brown April 5, 2023, 3:07 p.m. UTC | #3 
On Wed, Apr 05, 2023 at 03:21:03PM +0100, Lee Jones wrote:

> Is this on your radar?

> Can I take the other two patches without causing issues?

I'm waiting for the MFD.
Lee Jones April 5, 2023, 3:36 p.m. UTC | #4 
On Wed, 05 Apr 2023, Mark Brown wrote:

> On Wed, Apr 05, 2023 at 03:21:03PM +0100, Lee Jones wrote:
>
> > Is this on your radar?
>
> > Can I take the other two patches without causing issues?
>
> I'm waiting for the MFD.

To do what?  #deadlock

--
Lee Jones [李琼斯]
Mark Brown April 5, 2023, 4:15 p.m. UTC | #5 
On Wed, Apr 05, 2023 at 04:36:51PM +0100, Lee Jones wrote:
> On Wed, 05 Apr 2023, Mark Brown wrote:

> > I'm waiting for the MFD.

> To do what?  #deadlock

Whatever it is you need to do to be happy with and apply the shared bit
of the series.  We're somehow on v10 here for what seems like it should
be a very simple change, I've not followed the ins and outs of how that
happened.
Lee Jones April 5, 2023, 4:39 p.m. UTC | #6 
On Wed, 05 Apr 2023, Mark Brown wrote:

> On Wed, Apr 05, 2023 at 04:36:51PM +0100, Lee Jones wrote:
> > On Wed, 05 Apr 2023, Mark Brown wrote:
>
> > > I'm waiting for the MFD.
>
> > To do what?  #deadlock
>
> Whatever it is you need to do to be happy with and apply the shared bit
> of the series.  We're somehow on v10 here for what seems like it should
> be a very simple change, I've not followed the ins and outs of how that
> happened.

From an MFD perspective, reviews happened followed by an approval in v9.

I can't do anything without an Ack from you or some indication that you
want me to apply the first 2 patches and share an IB.

--
Lee Jones [李琼斯]
Mark Brown April 5, 2023, 5:07 p.m. UTC | #7 
On Wed, Apr 05, 2023 at 05:39:06PM +0100, Lee Jones wrote:
> On Wed, 05 Apr 2023, Mark Brown wrote:

> > Whatever it is you need to do to be happy with and apply the shared bit
> > of the series.  We're somehow on v10 here for what seems like it should
> > be a very simple change, I've not followed the ins and outs of how that
> > happened.

> From an MFD perspective, reviews happened followed by an approval in v9.

> I can't do anything without an Ack from you or some indication that you
> want me to apply the first 2 patches and share an IB.

Yes, please apply and share a branch like you usually do.
Lee Jones April 5, 2023, 6:03 p.m. UTC | #8 
On Wed, 05 Apr 2023, Mark Brown wrote:

> On Wed, Apr 05, 2023 at 05:39:06PM +0100, Lee Jones wrote:
> > On Wed, 05 Apr 2023, Mark Brown wrote:
>
> > > Whatever it is you need to do to be happy with and apply the shared bit
> > > of the series.  We're somehow on v10 here for what seems like it should
> > > be a very simple change, I've not followed the ins and outs of how that
> > > happened.
>
> > From an MFD perspective, reviews happened followed by an approval in v9.
>
> > I can't do anything without an Ack from you or some indication that you
> > want me to apply the first 2 patches and share an IB.
>
> Yes, please apply and share a branch like you usually do.

I usually encompass the other subsystem's patches in the IB too.

To enable me to do that, I need an Ack from you.

--
Lee Jones [李琼斯]
Mark Brown April 5, 2023, 6:38 p.m. UTC | #9 
On Wed, Apr 05, 2023 at 07:03:40PM +0100, Lee Jones wrote:
> On Wed, 05 Apr 2023, Mark Brown wrote:
> > On Wed, Apr 05, 2023 at 05:39:06PM +0100, Lee Jones wrote:

> > > I can't do anything without an Ack from you or some indication that you
> > > want me to apply the first 2 patches and share an IB.

> > Yes, please apply and share a branch like you usually do.

> I usually encompass the other subsystem's patches in the IB too.

> To enable me to do that, I need an Ack from you.

So not apply the first two patches and share a branch like you said
above...  TBH these serieses would probably be a bit more legible if
the branch were created with just the MFD patches, that'd also mean
smaller cross merges.
Mark Brown April 6, 2023, 1:49 p.m. UTC | #10 
On Sat, Apr 01, 2023 at 01:18:50AM +0100, Andre Przywara wrote:
> From: Martin Botka <martin.botka@somainline.org>
> 
> The AXP313a is your typical I2C controlled PMIC, although in a lighter
> fashion compared to the other X-Powers PMICs: it has only three DCDC
> rails, three LDOs, and no battery charging support.

Reviewed-by: Mark Brown <broonie@kernel.org>
Andre Przywara April 14, 2023, 10:32 a.m. UTC | #11 
On Thu, 6 Apr 2023 14:49:44 +0100
Mark Brown <broonie@kernel.org> wrote:

Hi Mark, Lee,

> On Sat, Apr 01, 2023 at 01:18:50AM +0100, Andre Przywara wrote:
> > From: Martin Botka <martin.botka@somainline.org>
> > 
> > The AXP313a is your typical I2C controlled PMIC, although in a lighter
> > fashion compared to the other X-Powers PMICs: it has only three DCDC
> > rails, three LDOs, and no battery charging support.  
> 
> Reviewed-by: Mark Brown <broonie@kernel.org>

so is there anything Martin and I can do to help this move forward? I
guess broonie's tag above means that the regulator part is good, so the MFC
parts can go ahead now?

Cheers,
Andre
Lee Jones April 19, 2023, 3:49 p.m. UTC | #12 
On Fri, 14 Apr 2023, Andre Przywara wrote:

> On Thu, 6 Apr 2023 14:49:44 +0100
> Mark Brown <broonie@kernel.org> wrote:
> 
> Hi Mark, Lee,
> 
> > On Sat, Apr 01, 2023 at 01:18:50AM +0100, Andre Przywara wrote:
> > > From: Martin Botka <martin.botka@somainline.org>
> > > 
> > > The AXP313a is your typical I2C controlled PMIC, although in a lighter
> > > fashion compared to the other X-Powers PMICs: it has only three DCDC
> > > rails, three LDOs, and no battery charging support.  
> > 
> > Reviewed-by: Mark Brown <broonie@kernel.org>
> 
> so is there anything Martin and I can do to help this move forward? I
> guess broonie's tag above means that the regulator part is good, so the MFC
> parts can go ahead now?

It's on my list.  Busy time.  Please bear with.
Andre Przywara April 19, 2023, 4:39 p.m. UTC | #13 
On Wed, 19 Apr 2023 16:49:24 +0100
Lee Jones <lee@kernel.org> wrote:

Hi Lee,

> On Fri, 14 Apr 2023, Andre Przywara wrote:
> 
> > On Thu, 6 Apr 2023 14:49:44 +0100
> > Mark Brown <broonie@kernel.org> wrote:
> > 
> > Hi Mark, Lee,
> > 
> > > On Sat, Apr 01, 2023 at 01:18:50AM +0100, Andre Przywara wrote:
> > > > From: Martin Botka <martin.botka@somainline.org>
> > > > 
> > > > The AXP313a is your typical I2C controlled PMIC, although in a lighter
> > > > fashion compared to the other X-Powers PMICs: it has only three DCDC
> > > > rails, three LDOs, and no battery charging support.  
> > > 
> > > Reviewed-by: Mark Brown <broonie@kernel.org>
> > 
> > so is there anything Martin and I can do to help this move forward? I
> > guess broonie's tag above means that the regulator part is good, so the MFC
> > parts can go ahead now?
> 
> It's on my list.  Busy time.  Please bear with.

Thanks, and no worries, just wanted to avoid it falling through the cracks!

Cheers,
Andre
diff mbox series

Patch

diff --git a/drivers/regulator/axp20x-regulator.c b/drivers/regulator/axp20x-regulator.c
index d260c442b788d..81f339009df9b 100644
--- a/drivers/regulator/axp20x-regulator.c
+++ b/drivers/regulator/axp20x-regulator.c
@@ -134,6 +134,11 @@ 
 #define AXP22X_PWR_OUT_DLDO4_MASK	BIT_MASK(6)
 #define AXP22X_PWR_OUT_ALDO3_MASK	BIT_MASK(7)
 
+#define AXP313A_DCDC1_NUM_VOLTAGES	107
+#define AXP313A_DCDC23_NUM_VOLTAGES	88
+#define AXP313A_DCDC_V_OUT_MASK		GENMASK(6, 0)
+#define AXP313A_LDO_V_OUT_MASK		GENMASK(4, 0)
+
 #define AXP803_PWR_OUT_DCDC1_MASK	BIT_MASK(0)
 #define AXP803_PWR_OUT_DCDC2_MASK	BIT_MASK(1)
 #define AXP803_PWR_OUT_DCDC3_MASK	BIT_MASK(2)
@@ -638,6 +643,48 @@  static const struct regulator_desc axp22x_drivevbus_regulator = {
 	.ops		= &axp20x_ops_sw,
 };
 
+static const struct linear_range axp313a_dcdc1_ranges[] = {
+	REGULATOR_LINEAR_RANGE(500000,   0,  70,  10000),
+	REGULATOR_LINEAR_RANGE(1220000, 71,  87,  20000),
+	REGULATOR_LINEAR_RANGE(1600000, 88, 106, 100000),
+};
+
+static const struct linear_range axp313a_dcdc2_ranges[] = {
+	REGULATOR_LINEAR_RANGE(500000,   0, 70, 10000),
+	REGULATOR_LINEAR_RANGE(1220000, 71, 87, 20000),
+};
+
+/*
+ * This is deviating from the datasheet. The values here are taken from the
+ * BSP driver and have been confirmed by measurements.
+ */
+static const struct linear_range axp313a_dcdc3_ranges[] = {
+	REGULATOR_LINEAR_RANGE(500000,   0,  70, 10000),
+	REGULATOR_LINEAR_RANGE(1220000, 71, 102, 20000),
+};
+
+static const struct regulator_desc axp313a_regulators[] = {
+	AXP_DESC_RANGES(AXP313A, DCDC1, "dcdc1", "vin1",
+			axp313a_dcdc1_ranges, AXP313A_DCDC1_NUM_VOLTAGES,
+			AXP313A_DCDC1_CONRTOL, AXP313A_DCDC_V_OUT_MASK,
+			AXP313A_OUTPUT_CONTROL, BIT(0)),
+	AXP_DESC_RANGES(AXP313A, DCDC2, "dcdc2", "vin2",
+			axp313a_dcdc2_ranges, AXP313A_DCDC23_NUM_VOLTAGES,
+			AXP313A_DCDC2_CONRTOL, AXP313A_DCDC_V_OUT_MASK,
+			AXP313A_OUTPUT_CONTROL, BIT(1)),
+	AXP_DESC_RANGES(AXP313A, DCDC3, "dcdc3", "vin3",
+			axp313a_dcdc3_ranges, AXP313A_DCDC23_NUM_VOLTAGES,
+			AXP313A_DCDC3_CONRTOL, AXP313A_DCDC_V_OUT_MASK,
+			AXP313A_OUTPUT_CONTROL, BIT(2)),
+	AXP_DESC(AXP313A, ALDO1, "aldo1", "vin1", 500, 3500, 100,
+		 AXP313A_ALDO1_CONRTOL, AXP313A_LDO_V_OUT_MASK,
+		 AXP313A_OUTPUT_CONTROL, BIT(3)),
+	AXP_DESC(AXP313A, DLDO1, "dldo1", "vin1", 500, 3500, 100,
+		 AXP313A_DLDO1_CONRTOL, AXP313A_LDO_V_OUT_MASK,
+		 AXP313A_OUTPUT_CONTROL, BIT(4)),
+	AXP_DESC_FIXED(AXP313A, RTC_LDO, "rtc-ldo", "vin1", 1800),
+};
+
 /* DCDC ranges shared with AXP813 */
 static const struct linear_range axp803_dcdc234_ranges[] = {
 	REGULATOR_LINEAR_RANGE(500000,
@@ -1040,6 +1087,15 @@  static int axp20x_set_dcdc_freq(struct platform_device *pdev, u32 dcdcfreq)
 		def = 3000;
 		step = 150;
 		break;
+	case AXP313A_ID:
+		/* The DCDC PWM frequency seems to be fixed to 3 MHz. */
+		if (dcdcfreq != 0) {
+			dev_err(&pdev->dev,
+				"DCDC frequency on AXP313a is fixed to 3 MHz.\n");
+			return -EINVAL;
+		}
+
+		return 0;
 	default:
 		dev_err(&pdev->dev,
 			"Setting DCDC frequency for unsupported AXP variant\n");
@@ -1232,6 +1288,10 @@  static int axp20x_regulator_probe(struct platform_device *pdev)
 		drivevbus = of_property_read_bool(pdev->dev.parent->of_node,
 						  "x-powers,drive-vbus-en");
 		break;
+	case AXP313A_ID:
+		regulators = axp313a_regulators;
+		nregulators = AXP313A_REG_ID_MAX;
+		break;
 	case AXP803_ID:
 		regulators = axp803_regulators;
 		nregulators = AXP803_REG_ID_MAX;