diff mbox series

[RFC,4/5] regulator: add 88pm88x regulators driver

Message ID 20231228100208.2932-5-karelb@gimli.ms.mff.cuni.cz
State New
Headers show
Series regulator: support for Marvell 88PM886 LDOs and bucks | expand

Commit Message

Karel Balej Dec. 28, 2023, 9:39 a.m. UTC
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

Mark Brown Jan. 5, 2024, 3:18 p.m. UTC | #1
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.
Karel Balej Jan. 7, 2024, 9:49 a.m. UTC | #2
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.
Krzysztof Kozlowski Jan. 7, 2024, 10:34 a.m. UTC | #3
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
Krzysztof Kozlowski Jan. 7, 2024, 10:35 a.m. UTC | #4
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
Karel Balej Jan. 7, 2024, 12:46 p.m. UTC | #5
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.
Karel Balej Jan. 7, 2024, 1:02 p.m. UTC | #6
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.
Mark Brown Jan. 7, 2024, 3:26 p.m. UTC | #7
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.
diff mbox series

Patch

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",
+	},
 };
 
 static struct pm88x_data pm886_a1_data = {
diff --git a/drivers/regulator/88pm88x-regulator.c b/drivers/regulator/88pm88x-regulator.c
new file mode 100644
index 000000000000..8b55e1365387
--- /dev/null
+++ b/drivers/regulator/88pm88x-regulator.c
@@ -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 = &regulator->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");
diff --git a/drivers/regulator/Kconfig b/drivers/regulator/Kconfig
index f3ec24691378..e3fffae60b4c 100644
--- a/drivers/regulator/Kconfig
+++ b/drivers/regulator/Kconfig
@@ -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
diff --git a/drivers/regulator/Makefile b/drivers/regulator/Makefile
index b2b059b5ee56..9c8a85b21699 100644
--- a/drivers/regulator/Makefile
+++ b/drivers/regulator/Makefile
@@ -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
diff --git a/include/linux/mfd/88pm88x.h b/include/linux/mfd/88pm88x.h
index 703e6104c1d8..edb871cc1d47 100644
--- a/include/linux/mfd/88pm88x.h
+++ b/include/linux/mfd/88pm88x.h
@@ -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,