[RFC,4/5] regulator: add 88pm88x regulators driver
Commit Message
From: Karel Balej <balejk@matfyz.cz>
Support the LDO and buck regulators of the Marvell 88PM886 PMIC. Support
for 88PM880 is not included but should be easy to implement being just a
matter of defining the additional LDOs and all bucks and modifying the
88PM88X MFD driver appropriately.
Signed-off-by: Karel Balej <balejk@matfyz.cz>
---
drivers/mfd/88pm88x.c | 15 ++
drivers/regulator/88pm88x-regulator.c | 214 ++++++++++++++++++++++++++
drivers/regulator/Kconfig | 6 +
drivers/regulator/Makefile | 1 +
include/linux/mfd/88pm88x.h | 11 ++
5 files changed, 247 insertions(+)
create mode 100644 drivers/regulator/88pm88x-regulator.c
Comments
On Thu, Dec 28, 2023 at 10:39:13AM +0100, Karel Balej wrote:
> @@ -68,6 +68,21 @@ static struct mfd_cell pm886_devs[] = {
> .num_resources = ARRAY_SIZE(pm88x_onkey_resources),
> .resources = pm88x_onkey_resources,
> },
> + {
> + .name = "88pm88x-regulator",
> + .id = PM88X_REGULATOR_ID_LDO2,
> + .of_compatible = "marvell,88pm88x-regulator",
> + },
Why are we adding an of_compatible here? It's redundant, the MFD split
is a feature of Linux internals not of the hardware, and the existing
88pm8xx MFD doesn't use them.
Mark,
On Fri Jan 5, 2024 at 4:18 PM CET, Mark Brown wrote:
> On Thu, Dec 28, 2023 at 10:39:13AM +0100, Karel Balej wrote:
>
> > @@ -68,6 +68,21 @@ static struct mfd_cell pm886_devs[] = {
> > .num_resources = ARRAY_SIZE(pm88x_onkey_resources),
> > .resources = pm88x_onkey_resources,
> > },
> > + {
> > + .name = "88pm88x-regulator",
> > + .id = PM88X_REGULATOR_ID_LDO2,
> > + .of_compatible = "marvell,88pm88x-regulator",
> > + },
>
> Why are we adding an of_compatible here? It's redundant, the MFD split
> is a feature of Linux internals not of the hardware, and the existing
> 88pm8xx MFD doesn't use them.
in a feedback to my MFD series, Rob Herring pointed out that there is no
need to have a devicetree node for a subdevice if it only contains
"compatible" as the MFD driver can instantiate subdevices itself. I
understood that this is what he was referring to, but now I suspect that
it is sufficient for the mfd_cell.name to be set to the subdevice driver
name for this - is that correct?
Thank you,
K. B.
On 07/01/2024 10:49, Karel Balej wrote:
> Mark,
>
> On Fri Jan 5, 2024 at 4:18 PM CET, Mark Brown wrote:
>> On Thu, Dec 28, 2023 at 10:39:13AM +0100, Karel Balej wrote:
>>
>>> @@ -68,6 +68,21 @@ static struct mfd_cell pm886_devs[] = {
>>> .num_resources = ARRAY_SIZE(pm88x_onkey_resources),
>>> .resources = pm88x_onkey_resources,
>>> },
>>> + {
>>> + .name = "88pm88x-regulator",
>>> + .id = PM88X_REGULATOR_ID_LDO2,
>>> + .of_compatible = "marvell,88pm88x-regulator",
>>> + },
>>
>> Why are we adding an of_compatible here? It's redundant, the MFD split
>> is a feature of Linux internals not of the hardware, and the existing
>> 88pm8xx MFD doesn't use them.
>
> in a feedback to my MFD series, Rob Herring pointed out that there is no
> need to have a devicetree node for a subdevice if it only contains
> "compatible" as the MFD driver can instantiate subdevices itself. I
> understood that this is what he was referring to, but now I suspect that
> it is sufficient for the mfd_cell.name to be set to the subdevice driver
> name for this - is that correct?
I think Rob was only referring to "no need to have a devicetree node".
But you added here a devicetree node, plus probably undocumented compatible.
Does it even pass the checkpatch?
Best regards,
Krzysztof
On 28/12/2023 10:39, Karel Balej wrote:
> diff --git a/drivers/mfd/88pm88x.c b/drivers/mfd/88pm88x.c
> index 69a8e39d43b3..999d0539b720 100644
> --- a/drivers/mfd/88pm88x.c
> +++ b/drivers/mfd/88pm88x.c
> @@ -68,6 +68,21 @@ static struct mfd_cell pm886_devs[] = {
> .num_resources = ARRAY_SIZE(pm88x_onkey_resources),
> .resources = pm88x_onkey_resources,
> },
> + {
> + .name = "88pm88x-regulator",
> + .id = PM88X_REGULATOR_ID_LDO2,
> + .of_compatible = "marvell,88pm88x-regulator",
> + },
> + {
> + .name = "88pm88x-regulator",
> + .id = PM88X_REGULATOR_ID_LDO15,
> + .of_compatible = "marvell,88pm88x-regulator",
> + },
> + {
> + .name = "88pm88x-regulator",
> + .id = PM886_REGULATOR_ID_BUCK2,
> + .of_compatible = "marvell,88pm88x-regulator",
Same compatible per each regulator looks suspicious, if not even wrong.
What are these?
Best regards,
Krzysztof
Krzysztof,
On Sun Jan 7, 2024 at 11:34 AM CET, Krzysztof Kozlowski wrote:
> On 07/01/2024 10:49, Karel Balej wrote:
> > Mark,
> >
> > On Fri Jan 5, 2024 at 4:18 PM CET, Mark Brown wrote:
> >> On Thu, Dec 28, 2023 at 10:39:13AM +0100, Karel Balej wrote:
> >>
> >>> @@ -68,6 +68,21 @@ static struct mfd_cell pm886_devs[] = {
> >>> .num_resources = ARRAY_SIZE(pm88x_onkey_resources),
> >>> .resources = pm88x_onkey_resources,
> >>> },
> >>> + {
> >>> + .name = "88pm88x-regulator",
> >>> + .id = PM88X_REGULATOR_ID_LDO2,
> >>> + .of_compatible = "marvell,88pm88x-regulator",
> >>> + },
> >>
> >> Why are we adding an of_compatible here? It's redundant, the MFD split
> >> is a feature of Linux internals not of the hardware, and the existing
> >> 88pm8xx MFD doesn't use them.
> >
> > in a feedback to my MFD series, Rob Herring pointed out that there is no
> > need to have a devicetree node for a subdevice if it only contains
> > "compatible" as the MFD driver can instantiate subdevices itself. I
> > understood that this is what he was referring to, but now I suspect that
> > it is sufficient for the mfd_cell.name to be set to the subdevice driver
> > name for this - is that correct?
>
> I think Rob was only referring to "no need to have a devicetree node".
yes, but I thought the presence of the compatible in the node is what
triggers instantiation of the driver and that adding it here instead was
necessary for that to happen if the node was to be removed. But like I
said, now I think only the .name property is relevant for that.
> But you added here a devicetree node, plus probably undocumented compatible.
>
> Does it even pass the checkpatch?
It does, but you were correct in your previous messages that I have not
run `make dt_binding_check` for this (or I assume that was what you
meant when you said that I did not test this) because I was not aware of
it when sending the MFD series and because this one would fail with the
same problems as Rob pointed out for that one, which is the main reason
why I only asked for feedback on the new parts. Sorry about that, next
time I will be sure to first fix all already known problems before
building on something.
>
> Best regards,
> Krzysztof
Thank you,
K. B.
On Sun Jan 7, 2024 at 11:35 AM CET, Krzysztof Kozlowski wrote:
> On 28/12/2023 10:39, Karel Balej wrote:
> > diff --git a/drivers/mfd/88pm88x.c b/drivers/mfd/88pm88x.c
> > index 69a8e39d43b3..999d0539b720 100644
> > --- a/drivers/mfd/88pm88x.c
> > +++ b/drivers/mfd/88pm88x.c
> > @@ -68,6 +68,21 @@ static struct mfd_cell pm886_devs[] = {
> > .num_resources = ARRAY_SIZE(pm88x_onkey_resources),
> > .resources = pm88x_onkey_resources,
> > },
> > + {
> > + .name = "88pm88x-regulator",
> > + .id = PM88X_REGULATOR_ID_LDO2,
> > + .of_compatible = "marvell,88pm88x-regulator",
> > + },
> > + {
> > + .name = "88pm88x-regulator",
> > + .id = PM88X_REGULATOR_ID_LDO15,
> > + .of_compatible = "marvell,88pm88x-regulator",
> > + },
> > + {
> > + .name = "88pm88x-regulator",
> > + .id = PM886_REGULATOR_ID_BUCK2,
> > + .of_compatible = "marvell,88pm88x-regulator",
>
> Same compatible per each regulator looks suspicious, if not even wrong.
> What are these?
The original attempt for upstreaming this MFD had a different compatible
for each regulator which was not correct according to the reviewers at
the time. I have thus used the same compatible for all regulators and
make the distinction in the regulator driver (using the .id property).
But I think that the problem here is again that I have confused the
purpose of .name and .of_compatible properties of struct mfd_cell - if a
driver is probed due to the .name property then I indeed should not need
compatible for the regulator driver at all.
>
> Best regards,
> Krzysztof
Best regards,
K. B.
On Sun, Jan 07, 2024 at 10:49:20AM +0100, Karel Balej wrote:
> On Fri Jan 5, 2024 at 4:18 PM CET, Mark Brown wrote:
> > Why are we adding an of_compatible here? It's redundant, the MFD split
> > is a feature of Linux internals not of the hardware, and the existing
> > 88pm8xx MFD doesn't use them.
> in a feedback to my MFD series, Rob Herring pointed out that there is no
> need to have a devicetree node for a subdevice if it only contains
> "compatible" as the MFD driver can instantiate subdevices itself. I
> understood that this is what he was referring to, but now I suspect that
> it is sufficient for the mfd_cell.name to be set to the subdevice driver
> name for this - is that correct?
That's what I'd expect, yes.
@@ -68,6 +68,21 @@ static struct mfd_cell pm886_devs[] = {
.num_resources = ARRAY_SIZE(pm88x_onkey_resources),
.resources = pm88x_onkey_resources,
},
+ {
+ .name = "88pm88x-regulator",
+ .id = PM88X_REGULATOR_ID_LDO2,
+ .of_compatible = "marvell,88pm88x-regulator",
+ },
+ {
+ .name = "88pm88x-regulator",
+ .id = PM88X_REGULATOR_ID_LDO15,
+ .of_compatible = "marvell,88pm88x-regulator",
+ },
+ {
+ .name = "88pm88x-regulator",
+ .id = PM886_REGULATOR_ID_BUCK2,
+ .of_compatible = "marvell,88pm88x-regulator",
+ },
};
static struct pm88x_data pm886_a1_data = {
new file mode 100644
@@ -0,0 +1,214 @@
+// SPDX-License-Identifier: GPL-2.0-only
+#include <linux/container_of.h>
+#include <linux/kernel.h>
+#include <linux/linear_range.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/regmap.h>
+#include <linux/regulator/driver.h>
+#include <linux/regulator/of_regulator.h>
+
+#include <linux/mfd/88pm88x.h>
+
+#define PM88X_REG_LDO_EN1 0x09
+#define PM88X_REG_LDO_EN2 0x0a
+
+#define PM88X_REG_BUCK_EN 0x08
+
+#define PM88X_REG_LDO1_VOUT 0x20
+#define PM88X_REG_LDO2_VOUT 0x26
+#define PM88X_REG_LDO3_VOUT 0x2c
+#define PM88X_REG_LDO4_VOUT 0x32
+#define PM88X_REG_LDO5_VOUT 0x38
+#define PM88X_REG_LDO6_VOUT 0x3e
+#define PM88X_REG_LDO7_VOUT 0x44
+#define PM88X_REG_LDO8_VOUT 0x4a
+#define PM88X_REG_LDO9_VOUT 0x50
+#define PM88X_REG_LDO10_VOUT 0x56
+#define PM88X_REG_LDO11_VOUT 0x5c
+#define PM88X_REG_LDO12_VOUT 0x62
+#define PM88X_REG_LDO13_VOUT 0x68
+#define PM88X_REG_LDO14_VOUT 0x6e
+#define PM88X_REG_LDO15_VOUT 0x74
+#define PM88X_REG_LDO16_VOUT 0x7a
+
+#define PM886_REG_BUCK1_VOUT 0xa5
+#define PM886_REG_BUCK2_VOUT 0xb3
+#define PM886_REG_BUCK3_VOUT 0xc1
+#define PM886_REG_BUCK4_VOUT 0xcf
+#define PM886_REG_BUCK5_VOUT 0xdd
+
+#define PM88X_LDO_VSEL_MASK 0x0f
+#define PM88X_BUCK_VSEL_MASK 0x7f
+
+struct pm88x_regulator {
+ struct regulator_desc desc;
+ int max_uA;
+};
+
+static int pm88x_regulator_get_ilim(struct regulator_dev *rdev)
+{
+ struct pm88x_regulator *data = rdev_get_drvdata(rdev);
+
+ if (!data) {
+ dev_err(&rdev->dev, "Failed to get regulator data\n");
+ return -EINVAL;
+ }
+ return data->max_uA;
+}
+
+static const struct regulator_ops pm88x_ldo_ops = {
+ .list_voltage = regulator_list_voltage_table,
+ .map_voltage = regulator_map_voltage_iterate,
+ .set_voltage_sel = regulator_set_voltage_sel_regmap,
+ .get_voltage_sel = regulator_get_voltage_sel_regmap,
+ .enable = regulator_enable_regmap,
+ .disable = regulator_disable_regmap,
+ .is_enabled = regulator_is_enabled_regmap,
+ .get_current_limit = pm88x_regulator_get_ilim,
+};
+
+static const struct regulator_ops pm88x_buck_ops = {
+ .list_voltage = regulator_list_voltage_linear_range,
+ .map_voltage = regulator_map_voltage_linear_range,
+ .set_voltage_sel = regulator_set_voltage_sel_regmap,
+ .get_voltage_sel = regulator_get_voltage_sel_regmap,
+ .enable = regulator_enable_regmap,
+ .disable = regulator_disable_regmap,
+ .is_enabled = regulator_is_enabled_regmap,
+ .get_current_limit = pm88x_regulator_get_ilim,
+};
+
+static const unsigned int pm88x_ldo_volt_table1[] = {
+ 1700000, 1800000, 1900000, 2500000, 2800000, 2900000, 3100000, 3300000,
+};
+
+static const unsigned int pm88x_ldo_volt_table2[] = {
+ 1200000, 1250000, 1700000, 1800000, 1850000, 1900000, 2500000, 2600000,
+ 2700000, 2750000, 2800000, 2850000, 2900000, 3000000, 3100000, 3300000,
+};
+
+static const unsigned int pm88x_ldo_volt_table3[] = {
+ 1700000, 1800000, 1900000, 2000000, 2100000, 2500000, 2700000, 2800000,
+};
+
+static const struct linear_range pm88x_buck_volt_ranges1[] = {
+ REGULATOR_LINEAR_RANGE(600000, 0, 79, 12500),
+ REGULATOR_LINEAR_RANGE(1600000, 80, 84, 50000),
+};
+
+static const struct linear_range pm88x_buck_volt_ranges2[] = {
+ REGULATOR_LINEAR_RANGE(600000, 0, 79, 12500),
+ REGULATOR_LINEAR_RANGE(1600000, 80, 114, 50000),
+};
+
+static struct pm88x_regulator pm88x_ldo2 = {
+ .desc = {
+ .name = "LDO2",
+ .id = PM88X_REGULATOR_ID_LDO2,
+ .regulators_node = "regulators",
+ .of_match = "ldo2",
+ .ops = &pm88x_ldo_ops,
+ .type = REGULATOR_VOLTAGE,
+ .enable_reg = PM88X_REG_LDO_EN1,
+ .enable_mask = BIT(1),
+ .volt_table = pm88x_ldo_volt_table1,
+ .n_voltages = ARRAY_SIZE(pm88x_ldo_volt_table1),
+ .vsel_reg = PM88X_REG_LDO2_VOUT,
+ .vsel_mask = PM88X_LDO_VSEL_MASK,
+ },
+ .max_uA = 100000,
+};
+
+static struct pm88x_regulator pm88x_ldo15 = {
+ .desc = {
+ .name = "LDO15",
+ .id = PM88X_REGULATOR_ID_LDO15,
+ .regulators_node = "regulators",
+ .of_match = "ldo15",
+ .ops = &pm88x_ldo_ops,
+ .type = REGULATOR_VOLTAGE,
+ .enable_reg = PM88X_REG_LDO_EN2,
+ .enable_mask = BIT(6),
+ .volt_table = pm88x_ldo_volt_table2,
+ .n_voltages = ARRAY_SIZE(pm88x_ldo_volt_table2),
+ .vsel_reg = PM88X_REG_LDO15_VOUT,
+ .vsel_mask = PM88X_LDO_VSEL_MASK,
+ },
+ .max_uA = 200000,
+};
+
+static struct pm88x_regulator pm886_buck2 = {
+ .desc = {
+ .name = "buck2",
+ .id = PM886_REGULATOR_ID_BUCK2,
+ .regulators_node = "regulators",
+ .of_match = "buck2",
+ .ops = &pm88x_buck_ops,
+ .type = REGULATOR_VOLTAGE,
+ .n_voltages = 115,
+ .linear_ranges = pm88x_buck_volt_ranges2,
+ .n_linear_ranges = ARRAY_SIZE(pm88x_buck_volt_ranges2),
+ .vsel_reg = PM886_REG_BUCK2_VOUT,
+ .vsel_mask = PM88X_BUCK_VSEL_MASK,
+ .enable_reg = PM88X_REG_BUCK_EN,
+ .enable_mask = BIT(1),
+ },
+ .max_uA = 1200000,
+};
+
+static struct pm88x_regulator *pm88x_regulators[] = {
+ [PM88X_REGULATOR_ID_LDO2] = &pm88x_ldo2,
+ [PM88X_REGULATOR_ID_LDO15] = &pm88x_ldo15,
+ [PM886_REGULATOR_ID_BUCK2] = &pm886_buck2,
+};
+
+static int pm88x_regulator_probe(struct platform_device *pdev)
+{
+ struct pm88x_chip *chip = dev_get_drvdata(pdev->dev.parent);
+ struct regulator_config rcfg = { };
+ struct pm88x_regulator *regulator;
+ struct regulator_desc *rdesc;
+ struct regulator_dev *rdev;
+ int ret;
+
+ if (pdev->id < 0 || pdev->id == PM88X_REGULATOR_ID_BUCKS
+ || pdev->id >= PM88X_REGULATOR_ID_SENTINEL) {
+ dev_err(&pdev->dev, "Invalid regulator ID: %d\n", pdev->id);
+ return -EINVAL;
+ }
+
+ rcfg.dev = pdev->dev.parent;
+ regulator = pm88x_regulators[pdev->id];
+ rdesc = ®ulator->desc;
+ rcfg.driver_data = regulator;
+ rcfg.regmap = chip->regmaps[rdesc->id > PM88X_REGULATOR_ID_BUCKS ?
+ PM88X_REGMAP_BUCK : PM88X_REGMAP_LDO];
+ rdev = devm_regulator_register(&pdev->dev, rdesc, &rcfg);
+ if (IS_ERR(rdev)) {
+ ret = PTR_ERR(rdev);
+ dev_err(&pdev->dev, "Failed to register %s: %d",
+ rdesc->name, ret);
+ return ret;
+ }
+
+ return 0;
+}
+
+const struct of_device_id pm88x_regulator_of_match[] = {
+ { .compatible = "marvell,88pm88x-regulator", },
+ { },
+};
+
+static struct platform_driver pm88x_regulator_driver = {
+ .driver = {
+ .name = "88pm88x-regulator",
+ .of_match_table = of_match_ptr(pm88x_regulator_of_match),
+ },
+ .probe = pm88x_regulator_probe,
+};
+module_platform_driver(pm88x_regulator_driver);
+
+MODULE_DESCRIPTION("Marvell 88PM88X PMIC regulator driver");
+MODULE_AUTHOR("Karel Balej <balejk@matfyz.cz>");
+MODULE_LICENSE("GPL");
@@ -81,6 +81,12 @@ config REGULATOR_88PM8607
help
This driver supports 88PM8607 voltage regulator chips.
+config REGULATOR_88PM88X
+ tristate "Marvell 88PM88X voltage regulators"
+ depends on MFD_88PM88X
+ help
+ This driver implements support for Marvell 88PM88X voltage regulators.
+
config REGULATOR_ACT8865
tristate "Active-semi act8865 voltage regulator"
depends on I2C
@@ -13,6 +13,7 @@ obj-$(CONFIG_REGULATOR_USERSPACE_CONSUMER) += userspace-consumer.o
obj-$(CONFIG_REGULATOR_88PG86X) += 88pg86x.o
obj-$(CONFIG_REGULATOR_88PM800) += 88pm800-regulator.o
obj-$(CONFIG_REGULATOR_88PM8607) += 88pm8607.o
+obj-$(CONFIG_REGULATOR_88PM88X) += 88pm88x-regulator.o
obj-$(CONFIG_REGULATOR_CROS_EC) += cros-ec-regulator.o
obj-$(CONFIG_REGULATOR_CPCAP) += cpcap-regulator.o
obj-$(CONFIG_REGULATOR_AAT2870) += aat2870-regulator.o
@@ -41,6 +41,17 @@
#define PM88X_PAGE_OFFSET_LDO 1
+enum pm88x_regulator_id {
+ PM88X_REGULATOR_ID_LDO2,
+ PM88X_REGULATOR_ID_LDO15,
+
+ PM88X_REGULATOR_ID_BUCKS,
+
+ PM886_REGULATOR_ID_BUCK2,
+
+ PM88X_REGULATOR_ID_SENTINEL
+};
+
enum pm88x_regmap_index {
PM88X_REGMAP_BASE,
PM88X_REGMAP_LDO,