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. The BSP
driver hints at a register being able to change that, but we haven't
verified that, so leave that one out. It can be added later, if needed
and/or required.
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 <[email protected]>
Signed-off-by: Andre Przywara <[email protected]>
---
drivers/regulator/axp20x-regulator.c | 60 ++++++++++++++++++++++++++++
1 file changed, 60 insertions(+)
diff --git a/drivers/regulator/axp20x-regulator.c b/drivers/regulator/axp20x-regulator.c
index d260c442b788..3087bc98694f 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, LDO1, "ldo1", "vin1", 500, 3500, 100,
+ AXP313A_ALDO1_CONRTOL, AXP313A_LDO_V_OUT_MASK,
+ AXP313A_OUTPUT_CONTROL, BIT(3)),
+ AXP_DESC(AXP313A, LDO2, "ldo2", "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 != 3000000 && 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.39.0
On Sat, Jan 21, 2023 at 2:45 AM Martin Botka
<[email protected]> wrote:
>
> 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. The BSP
> driver hints at a register being able to change that, but we haven't
> verified that, so leave that one out. It can be added later, if needed
> and/or required.
The datasheet released by MangoPi says this isn't configurable. The
thing that is configurable is spread-spectrum operation, and mode
switching between fixed PWM and hybrid PFM/PWM. So just drop the
DCDC frequency stuff and use the default code path.
> 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 <[email protected]>
> Signed-off-by: Andre Przywara <[email protected]>
> ---
> drivers/regulator/axp20x-regulator.c | 60 ++++++++++++++++++++++++++++
> 1 file changed, 60 insertions(+)
>
> diff --git a/drivers/regulator/axp20x-regulator.c b/drivers/regulator/axp20x-regulator.c
> index d260c442b788..3087bc98694f 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, LDO1, "ldo1", "vin1", 500, 3500, 100,
> + AXP313A_ALDO1_CONRTOL, AXP313A_LDO_V_OUT_MASK,
> + AXP313A_OUTPUT_CONTROL, BIT(3)),
The datasheet says this one is called ALDO1 ...
> + AXP_DESC(AXP313A, LDO2, "ldo2", "vin1", 500, 3500, 100,
> + AXP313A_DLDO1_CONRTOL, AXP313A_LDO_V_OUT_MASK,
> + AXP313A_OUTPUT_CONTROL, BIT(4)),
... and this one DLDO1.
> + 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 != 3000000 && dcdcfreq != 0) {
> + dev_err(&pdev->dev,
> + "DCDC frequency on AXP313a is fixed to 3 MHz.\n");
> + return -EINVAL;
> + }
> +
> + return 0;
As mentioned above, please drop this.
Besides the bits mentioned above, this looks OK.
> 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.39.0
>
Hi Martin,
> On Sat, Jan 21, 2023 at 2:45 AM Martin Botka
> <[email protected]> wrote:
>> 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. The BSP
>> driver hints at a register being able to change that, but we haven't
>> verified that, so leave that one out. It can be added later, if needed
>> and/or required.
> The datasheet released by MangoPi says this isn't configurable. The
> thing that is configurable is spread-spectrum operation, and mode
> switching between fixed PWM and hybrid PFM/PWM. So just drop the
> DCDC frequency stuff and use the default code path.
You could get full datasheet of AXP313A here:
https://github.com/YuzukiHD/YuzukiChameleon/blob/master/Datasheet/AXP313A_Datasheet_V1.0_cn.pdf
Btw I'm working on AXP15060 support mostly based on your series.
Best regards,
Shengyu
Hi Shengyu,
On Sat, Feb 18 2023 at 06:08:06 PM +08:00:00, Shengyu Qu
<[email protected]> wrote:
> Hi Martin,
>
>> On Sat, Jan 21, 2023 at 2:45 AM Martin Botka
>> <[email protected]> wrote:
>>> 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. The BSP
>>> driver hints at a register being able to change that, but we haven't
>>> verified that, so leave that one out. It can be added later, if
>>> needed
>>> and/or required.
>> The datasheet released by MangoPi says this isn't configurable. The
>> thing that is configurable is spread-spectrum operation, and mode
>> switching between fixed PWM and hybrid PFM/PWM. So just drop the
>> DCDC frequency stuff and use the default code path.
>
> You could get full datasheet of AXP313A here:
>
> https://github.com/YuzukiHD/YuzukiChameleon/blob/master/Datasheet/AXP313A_Datasheet_V1.0_cn.pdf
I do have the datasheet but maybe this one is more up to date somehow.
Will have to check.
>
> Btw I'm working on AXP15060 support mostly based on your series.
Lovely to hear. So sorry for the very very late reply. New semester
began 3 weeks ago and been quite the ride.
Would love to get the series more up to date in the upcoming weeks :) I
will see what time allows :)
>
>
> Best regards,
>
> Shengyu
Best regards,
Martin
On Sat, 28 Jan 2023 01:24:18 +0800
Chen-Yu Tsai <[email protected]> wrote:
Hi,
> On Sat, Jan 21, 2023 at 2:45 AM Martin Botka
> <[email protected]> wrote:
> >
> > 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. The BSP
> > driver hints at a register being able to change that, but we haven't
> > verified that, so leave that one out. It can be added later, if needed
> > and/or required.
>
> The datasheet released by MangoPi says this isn't configurable. The
> thing that is configurable is spread-spectrum operation, and mode
> switching between fixed PWM and hybrid PFM/PWM. So just drop the
> DCDC frequency stuff and use the default code path.
The default code path is fatal to the driver, so we can't really do this.
axp20x_set_dcdc_freq is *always* called, even when the property is missing,
in this case the frequency will just be 0.
If we don't specify the variant ID in the switch/case, we get an error and
the driver bails out with -EINVAL.
So the minimal implementation would be:
case AXP313A_ID:
return 0;
To be a bit more robust and catch cases where people try to specify some
DCDC frequency, I added this extra check for 3MHz or 0 (no property).
> > 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 <[email protected]>
> > Signed-off-by: Andre Przywara <[email protected]>
> > ---
> > drivers/regulator/axp20x-regulator.c | 60 ++++++++++++++++++++++++++++
> > 1 file changed, 60 insertions(+)
> >
> > diff --git a/drivers/regulator/axp20x-regulator.c b/drivers/regulator/axp20x-regulator.c
> > index d260c442b788..3087bc98694f 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, LDO1, "ldo1", "vin1", 500, 3500, 100,
> > + AXP313A_ALDO1_CONRTOL, AXP313A_LDO_V_OUT_MASK,
> > + AXP313A_OUTPUT_CONTROL, BIT(3)),
>
> The datasheet says this one is called ALDO1 ...
>
> > + AXP_DESC(AXP313A, LDO2, "ldo2", "vin1", 500, 3500, 100,
> > + AXP313A_DLDO1_CONRTOL, AXP313A_LDO_V_OUT_MASK,
> > + AXP313A_OUTPUT_CONTROL, BIT(4)),
>
> ... and this one DLDO1.
Fixed.
> > + 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 != 3000000 && dcdcfreq != 0) {
> > + dev_err(&pdev->dev,
> > + "DCDC frequency on AXP313a is fixed to 3 MHz.\n");
> > + return -EINVAL;
> > + }
> > +
> > + return 0;
>
> As mentioned above, please drop this.
As mentioned above, we need at least the variant ID and a "return 0;". Do
you want me to drop the extra checks as well? Doesn't really hurt, and
provides extra info in case people try something stupid.
> Besides the bits mentioned above, this looks OK.
Thanks!
Andre
>
> > 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.39.0
> >
On Thu, Mar 23, 2023 at 9:59 PM Andre Przywara <[email protected]> wrote:
>
> On Sat, 28 Jan 2023 01:24:18 +0800
> Chen-Yu Tsai <[email protected]> wrote:
>
> Hi,
>
> > On Sat, Jan 21, 2023 at 2:45 AM Martin Botka
> > <[email protected]> wrote:
> > >
> > > 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. The BSP
> > > driver hints at a register being able to change that, but we haven't
> > > verified that, so leave that one out. It can be added later, if needed
> > > and/or required.
> >
> > The datasheet released by MangoPi says this isn't configurable. The
> > thing that is configurable is spread-spectrum operation, and mode
> > switching between fixed PWM and hybrid PFM/PWM. So just drop the
> > DCDC frequency stuff and use the default code path.
>
> The default code path is fatal to the driver, so we can't really do this.
> axp20x_set_dcdc_freq is *always* called, even when the property is missing,
> in this case the frequency will just be 0.
> If we don't specify the variant ID in the switch/case, we get an error and
> the driver bails out with -EINVAL.
> So the minimal implementation would be:
> case AXP313A_ID:
> return 0;
> To be a bit more robust and catch cases where people try to specify some
> DCDC frequency, I added this extra check for 3MHz or 0 (no property).
Given that it's fixed, it really shouldn't be specified as a property.
> > > 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 <[email protected]>
> > > Signed-off-by: Andre Przywara <[email protected]>
> > > ---
> > > drivers/regulator/axp20x-regulator.c | 60 ++++++++++++++++++++++++++++
> > > 1 file changed, 60 insertions(+)
> > >
> > > diff --git a/drivers/regulator/axp20x-regulator.c b/drivers/regulator/axp20x-regulator.c
> > > index d260c442b788..3087bc98694f 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, LDO1, "ldo1", "vin1", 500, 3500, 100,
> > > + AXP313A_ALDO1_CONRTOL, AXP313A_LDO_V_OUT_MASK,
> > > + AXP313A_OUTPUT_CONTROL, BIT(3)),
> >
> > The datasheet says this one is called ALDO1 ...
> >
> > > + AXP_DESC(AXP313A, LDO2, "ldo2", "vin1", 500, 3500, 100,
> > > + AXP313A_DLDO1_CONRTOL, AXP313A_LDO_V_OUT_MASK,
> > > + AXP313A_OUTPUT_CONTROL, BIT(4)),
> >
> > ... and this one DLDO1.
>
> Fixed.
>
>
> > > + 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 != 3000000 && dcdcfreq != 0) {
> > > + dev_err(&pdev->dev,
> > > + "DCDC frequency on AXP313a is fixed to 3 MHz.\n");
> > > + return -EINVAL;
> > > + }
> > > +
> > > + return 0;
> >
> > As mentioned above, please drop this.
>
> As mentioned above, we need at least the variant ID and a "return 0;". Do
> you want me to drop the extra checks as well? Doesn't really hurt, and
> provides extra info in case people try something stupid.
Ah, OK. In that case let's allow the "no property" case, and error out
for all any other value set.
ChenYu
> > Besides the bits mentioned above, this looks OK.
>
> Thanks!
> Andre
>
> >
> > > 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.39.0
> > >
>
On Thu, 23 Mar 2023 22:21:44 +0800
Chen-Yu Tsai <[email protected]> wrote:
Hi,
> On Thu, Mar 23, 2023 at 9:59 PM Andre Przywara <[email protected]> wrote:
> >
> > On Sat, 28 Jan 2023 01:24:18 +0800
> > Chen-Yu Tsai <[email protected]> wrote:
> >
> > Hi,
> >
> > > On Sat, Jan 21, 2023 at 2:45 AM Martin Botka
> > > <[email protected]> wrote:
> > > >
> > > > 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. The BSP
> > > > driver hints at a register being able to change that, but we haven't
> > > > verified that, so leave that one out. It can be added later, if needed
> > > > and/or required.
> > >
> > > The datasheet released by MangoPi says this isn't configurable. The
> > > thing that is configurable is spread-spectrum operation, and mode
> > > switching between fixed PWM and hybrid PFM/PWM. So just drop the
> > > DCDC frequency stuff and use the default code path.
> >
> > The default code path is fatal to the driver, so we can't really do this.
> > axp20x_set_dcdc_freq is *always* called, even when the property is missing,
> > in this case the frequency will just be 0.
> > If we don't specify the variant ID in the switch/case, we get an error and
> > the driver bails out with -EINVAL.
> > So the minimal implementation would be:
> > case AXP313A_ID:
> > return 0;
> > To be a bit more robust and catch cases where people try to specify some
> > DCDC frequency, I added this extra check for 3MHz or 0 (no property).
>
> Given that it's fixed, it really shouldn't be specified as a property.
I agree, I guess this means we need to disallow it in the DT binding then?
>
> > > > 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 <[email protected]>
> > > > Signed-off-by: Andre Przywara <[email protected]>
> > > > ---
> > > > drivers/regulator/axp20x-regulator.c | 60 ++++++++++++++++++++++++++++
> > > > 1 file changed, 60 insertions(+)
> > > >
> > > > diff --git a/drivers/regulator/axp20x-regulator.c b/drivers/regulator/axp20x-regulator.c
> > > > index d260c442b788..3087bc98694f 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, LDO1, "ldo1", "vin1", 500, 3500, 100,
> > > > + AXP313A_ALDO1_CONRTOL, AXP313A_LDO_V_OUT_MASK,
> > > > + AXP313A_OUTPUT_CONTROL, BIT(3)),
> > >
> > > The datasheet says this one is called ALDO1 ...
> > >
> > > > + AXP_DESC(AXP313A, LDO2, "ldo2", "vin1", 500, 3500, 100,
> > > > + AXP313A_DLDO1_CONRTOL, AXP313A_LDO_V_OUT_MASK,
> > > > + AXP313A_OUTPUT_CONTROL, BIT(4)),
> > >
> > > ... and this one DLDO1.
> >
> > Fixed.
> >
> >
> > > > + 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 != 3000000 && dcdcfreq != 0) {
> > > > + dev_err(&pdev->dev,
> > > > + "DCDC frequency on AXP313a is fixed to 3 MHz.\n");
> > > > + return -EINVAL;
> > > > + }
> > > > +
> > > > + return 0;
> > >
> > > As mentioned above, please drop this.
> >
> > As mentioned above, we need at least the variant ID and a "return 0;". Do
> > you want me to drop the extra checks as well? Doesn't really hurt, and
> > provides extra info in case people try something stupid.
>
> Ah, OK. In that case let's allow the "no property" case, and error out
> for all any other value set.
OK, will do.
Thanks,
Andre
>
> ChenYu
>
> > > Besides the bits mentioned above, this looks OK.
> >
> > Thanks!
> > Andre
> >
> > >
> > > > 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.39.0
> > > >
> >
On Fri, Mar 24, 2023 at 12:07 AM Andre Przywara <[email protected]> wrote:
>
> On Thu, 23 Mar 2023 22:21:44 +0800
> Chen-Yu Tsai <[email protected]> wrote:
>
> Hi,
>
> > On Thu, Mar 23, 2023 at 9:59 PM Andre Przywara <[email protected]> wrote:
> > >
> > > On Sat, 28 Jan 2023 01:24:18 +0800
> > > Chen-Yu Tsai <[email protected]> wrote:
> > >
> > > Hi,
> > >
> > > > On Sat, Jan 21, 2023 at 2:45 AM Martin Botka
> > > > <[email protected]> wrote:
> > > > >
> > > > > 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. The BSP
> > > > > driver hints at a register being able to change that, but we haven't
> > > > > verified that, so leave that one out. It can be added later, if needed
> > > > > and/or required.
> > > >
> > > > The datasheet released by MangoPi says this isn't configurable. The
> > > > thing that is configurable is spread-spectrum operation, and mode
> > > > switching between fixed PWM and hybrid PFM/PWM. So just drop the
> > > > DCDC frequency stuff and use the default code path.
> > >
> > > The default code path is fatal to the driver, so we can't really do this.
> > > axp20x_set_dcdc_freq is *always* called, even when the property is missing,
> > > in this case the frequency will just be 0.
> > > If we don't specify the variant ID in the switch/case, we get an error and
> > > the driver bails out with -EINVAL.
> > > So the minimal implementation would be:
> > > case AXP313A_ID:
> > > return 0;
> > > To be a bit more robust and catch cases where people try to specify some
> > > DCDC frequency, I added this extra check for 3MHz or 0 (no property).
> >
> > Given that it's fixed, it really shouldn't be specified as a property.
>
> I agree, I guess this means we need to disallow it in the DT binding then?
That's right.
> >
> > > > > 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 <[email protected]>
> > > > > Signed-off-by: Andre Przywara <[email protected]>
> > > > > ---
> > > > > drivers/regulator/axp20x-regulator.c | 60 ++++++++++++++++++++++++++++
> > > > > 1 file changed, 60 insertions(+)
> > > > >
> > > > > diff --git a/drivers/regulator/axp20x-regulator.c b/drivers/regulator/axp20x-regulator.c
> > > > > index d260c442b788..3087bc98694f 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, LDO1, "ldo1", "vin1", 500, 3500, 100,
> > > > > + AXP313A_ALDO1_CONRTOL, AXP313A_LDO_V_OUT_MASK,
> > > > > + AXP313A_OUTPUT_CONTROL, BIT(3)),
> > > >
> > > > The datasheet says this one is called ALDO1 ...
> > > >
> > > > > + AXP_DESC(AXP313A, LDO2, "ldo2", "vin1", 500, 3500, 100,
> > > > > + AXP313A_DLDO1_CONRTOL, AXP313A_LDO_V_OUT_MASK,
> > > > > + AXP313A_OUTPUT_CONTROL, BIT(4)),
> > > >
> > > > ... and this one DLDO1.
> > >
> > > Fixed.
> > >
> > >
> > > > > + 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 != 3000000 && dcdcfreq != 0) {
> > > > > + dev_err(&pdev->dev,
> > > > > + "DCDC frequency on AXP313a is fixed to 3 MHz.\n");
> > > > > + return -EINVAL;
> > > > > + }
> > > > > +
> > > > > + return 0;
> > > >
> > > > As mentioned above, please drop this.
> > >
> > > As mentioned above, we need at least the variant ID and a "return 0;". Do
> > > you want me to drop the extra checks as well? Doesn't really hurt, and
> > > provides extra info in case people try something stupid.
> >
> > Ah, OK. In that case let's allow the "no property" case, and error out
> > for all any other value set.
>
> OK, will do.
>
> Thanks,
> Andre
>
> >
> > ChenYu
> >
> > > > Besides the bits mentioned above, this looks OK.
> > >
> > > Thanks!
> > > Andre
> > >
> > > >
> > > > > 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.39.0
> > > > >
> > >
>