From: Karel Balej <[email protected]>
Hello,
the following adds the regulators driver for Marvell 88PM88X PMICs
implementing only the 88PM886 specific parts - however extension for
88PM880 should be trivial. The series adding MFD driver for these PMICs
is available here [1]. Please note that this series depends on that
one.
The motivation and testing platform for this is the
samsung,coreprimevelte smartphone for which the initial support efforts
are ongoing here [2]. This PMIC is also found in at least two other
devices with the PXA1908 SoC, such as samsung,xcover3lte and
samsung,grandprimevelte.
As the only reference for this driver served the smartphone's downstream
kernel tree which is available here [3].
Please note that the first patch of this series is just a joining step
with respect to series [1] and will be amalgated with future versions of
it and dropped here. Also please note that that this series has the same
defects as the MFD one and thus please only review the new parts.
Lastly, as I would like to get some feedback on whether the approach I
have taken here is OK, I have only defined descriptions for three
regulators so far, the remaining eighteen will be defined in the same
style and will of course be added when this series leaves the RFC state
at the latest.
[1] https://lore.kernel.org/all/[email protected]/
[2] https://lore.kernel.org/all/[email protected]/
[3] https://github.com/CoderCharmander/g361f-kernel
Thank you,
K. B.
Karel Balej (5):
mfd: 88pm88x: differences with respect to the PMIC RFC series
mfd: 88pm88x: initialize the regulators regmaps
dt-bindings: regulator: add documentation entry for 88pm88x-regulator
regulator: add 88pm88x regulators driver
MAINTAINERS: add entries for the 88pm88x regulators driver
.../bindings/mfd/marvell,88pm88x.yaml | 17 ++
.../regulator/marvell,88pm88x-regulator.yaml | 28 +++
MAINTAINERS | 2 +
drivers/mfd/88pm88x.c | 62 ++++-
drivers/regulator/88pm88x-regulator.c | 214 ++++++++++++++++++
drivers/regulator/Kconfig | 6 +
drivers/regulator/Makefile | 1 +
include/linux/mfd/88pm88x.h | 17 ++
8 files changed, 341 insertions(+), 6 deletions(-)
create mode 100644 Documentation/devicetree/bindings/regulator/marvell,88pm88x-regulator.yaml
create mode 100644 drivers/regulator/88pm88x-regulator.c
--
2.43.0
From: Karel Balej <[email protected]>
Signed-off-by: Karel Balej <[email protected]>
---
drivers/mfd/88pm88x.c | 14 ++++++++------
include/linux/mfd/88pm88x.h | 2 ++
2 files changed, 10 insertions(+), 6 deletions(-)
diff --git a/drivers/mfd/88pm88x.c b/drivers/mfd/88pm88x.c
index 5db6c65b667d..3424d88a58f6 100644
--- a/drivers/mfd/88pm88x.c
+++ b/drivers/mfd/88pm88x.c
@@ -57,16 +57,16 @@ static struct reg_sequence pm886_presets[] = {
REG_SEQ0(PM88X_REG_BK_OSC_CTRL3, 0xc0),
};
-static struct resource onkey_resources[] = {
+static struct resource pm88x_onkey_resources[] = {
DEFINE_RES_IRQ_NAMED(PM88X_IRQ_ONKEY, "88pm88x-onkey"),
};
-static struct mfd_cell pm88x_devs[] = {
+static struct mfd_cell pm886_devs[] = {
{
.name = "88pm88x-onkey",
- .num_resources = ARRAY_SIZE(onkey_resources),
- .resources = onkey_resources,
- .id = -1,
+ .of_compatible = "marvell,88pm88x-onkey",
+ .num_resources = ARRAY_SIZE(pm88x_onkey_resources),
+ .resources = pm88x_onkey_resources,
},
};
@@ -74,6 +74,8 @@ static struct pm88x_data pm886_a1_data = {
.whoami = PM886_A1_WHOAMI,
.presets = pm886_presets,
.num_presets = ARRAY_SIZE(pm886_presets),
+ .devs = pm886_devs,
+ .num_devs = ARRAY_SIZE(pm886_devs),
};
static const struct regmap_config pm88x_i2c_regmap = {
@@ -157,7 +159,7 @@ static int pm88x_probe(struct i2c_client *client)
if (ret)
return ret;
- ret = devm_mfd_add_devices(&client->dev, 0, pm88x_devs, ARRAY_SIZE(pm88x_devs),
+ ret = devm_mfd_add_devices(&client->dev, 0, chip->data->devs, chip->data->num_devs,
NULL, 0, regmap_irq_get_domain(chip->irq_data));
if (ret) {
dev_err(&client->dev, "Failed to add devices: %d\n", ret);
diff --git a/include/linux/mfd/88pm88x.h b/include/linux/mfd/88pm88x.h
index a34c57447827..9a335f6b9c07 100644
--- a/include/linux/mfd/88pm88x.h
+++ b/include/linux/mfd/88pm88x.h
@@ -49,6 +49,8 @@ struct pm88x_data {
unsigned int whoami;
struct reg_sequence *presets;
unsigned int num_presets;
+ struct mfd_cell *devs;
+ unsigned int num_devs;
};
struct pm88x_chip {
--
2.43.0
From: Karel Balej <[email protected]>
The Marvell 88PM88X PMICs provide regulators among other things.
Document how to use them.
Signed-off-by: Karel Balej <[email protected]>
---
.../bindings/mfd/marvell,88pm88x.yaml | 17 +++++++++++
.../regulator/marvell,88pm88x-regulator.yaml | 28 +++++++++++++++++++
2 files changed, 45 insertions(+)
create mode 100644 Documentation/devicetree/bindings/regulator/marvell,88pm88x-regulator.yaml
diff --git a/Documentation/devicetree/bindings/mfd/marvell,88pm88x.yaml b/Documentation/devicetree/bindings/mfd/marvell,88pm88x.yaml
index 115b41c9f22c..e6944369fc5c 100644
--- a/Documentation/devicetree/bindings/mfd/marvell,88pm88x.yaml
+++ b/Documentation/devicetree/bindings/mfd/marvell,88pm88x.yaml
@@ -54,6 +54,23 @@ examples:
onkey {
compatible = "marvell,88pm88x-onkey";
};
+
+ regulators {
+ ldo2: ldo2 {
+ regulator-min-microvolt = <3100000>;
+ regulator-max-microvolt = <3300000>;
+ };
+
+ ldo15: ldo15 {
+ regulator-min-microvolt = <3300000>;
+ regulator-max-microvolt = <3300000>;
+ };
+
+ buck2: buck2 {
+ regulator-min-microvolt = <1800000>;
+ regulator-max-microvolt = <1800000>;
+ };
+ };
};
};
...
diff --git a/Documentation/devicetree/bindings/regulator/marvell,88pm88x-regulator.yaml b/Documentation/devicetree/bindings/regulator/marvell,88pm88x-regulator.yaml
new file mode 100644
index 000000000000..c6ac17b113e7
--- /dev/null
+++ b/Documentation/devicetree/bindings/regulator/marvell,88pm88x-regulator.yaml
@@ -0,0 +1,28 @@
+# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/regulator/marvell,88pm88x-regulator.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Marvell 88PM88X PMICs regulators
+
+maintainers:
+ - Karel Balej <[email protected]>
+
+description: |
+ This module is part of the Marvell 88PM88X MFD device. For more details
+ see Documentation/devicetree/bindings/mfd/marvell,88pm88x.yaml.
+
+ The regulator controller is represented as a sub-node of the PMIC node
+ in the device tree.
+
+ The valid names for 88PM886 regulator nodes are ldo[1-9], ldo1[0-6], buck[1-5].
+
+patternProperties:
+ "^(ldo|buck)[0-9]+$":
+ type: object
+ description:
+ Properties for single regulator.
+ $ref: regulator.yaml#
+
+additionalProperties: false
--
2.43.0
From: Karel Balej <[email protected]>
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 <[email protected]>
---
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
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 = ®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 <[email protected]>");
+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,
--
2.43.0
From: Karel Balej <[email protected]>
List the related files under the Marvell 88PM88X PMICs entry.
Signed-off-by: Karel Balej <[email protected]>
---
MAINTAINERS | 2 ++
1 file changed, 2 insertions(+)
diff --git a/MAINTAINERS b/MAINTAINERS
index f35ec0f186a9..f9676aec7397 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -12743,8 +12743,10 @@ M: Karel Balej <[email protected]>
S: Maintained
F: Documentation/devicetree/bindings/input/marvell,88pm88x-onkey.yaml
F: Documentation/devicetree/bindings/mfd/marvell,88pm88x.yaml
+F: Documentation/devicetree/bindings/regulator/marvell,88pm88x-regulator.yaml
F: drivers/input/misc/88pm88x-onkey.c
F: drivers/mfd/88pm88x.c
+F: drivers/regulators/88pm88x-regulator.c
F: include/linux/mfd/88pm88x.h
MARVELL ARMADA 3700 PHY DRIVERS
--
2.43.0
From: Karel Balej <[email protected]>
The regulators registers are accessed via a different I2C address than
the already implemented functionality. Initialize the new regmap for the
regulator driver to use. For 88PM886 the buck regmap is the same as LDO
regmap, however this is not the case for 88PM880.
Signed-off-by: Karel Balej <[email protected]>
---
drivers/mfd/88pm88x.c | 33 +++++++++++++++++++++++++++++++++
include/linux/mfd/88pm88x.h | 4 ++++
2 files changed, 37 insertions(+)
diff --git a/drivers/mfd/88pm88x.c b/drivers/mfd/88pm88x.c
index 3424d88a58f6..69a8e39d43b3 100644
--- a/drivers/mfd/88pm88x.c
+++ b/drivers/mfd/88pm88x.c
@@ -98,6 +98,35 @@ static int pm88x_power_off_handler(struct sys_off_data *data)
return NOTIFY_DONE;
}
+static int pm88x_initialize_subregmaps(struct pm88x_chip *chip)
+{
+ struct i2c_client *page;
+ struct regmap *regmap;
+ int ret;
+
+ /* LDO page */
+ page = devm_i2c_new_dummy_device(&chip->client->dev, chip->client->adapter,
+ chip->client->addr + PM88X_PAGE_OFFSET_LDO);
+ if (IS_ERR(page)) {
+ ret = PTR_ERR(page);
+ dev_err(&chip->client->dev, "Failed to initialize LDO client: %d\n",
+ ret);
+ return ret;
+ }
+ regmap = devm_regmap_init_i2c(page, &pm88x_i2c_regmap);
+ if (IS_ERR(regmap)) {
+ ret = PTR_ERR(regmap);
+ dev_err(&chip->client->dev, "Failed to initialize LDO regmap: %d\n",
+ ret);
+ return ret;
+ }
+ chip->regmaps[PM88X_REGMAP_LDO] = regmap;
+ /* buck regmap is the same as LDO */
+ chip->regmaps[PM88X_REGMAP_BUCK] = regmap;
+
+ return 0;
+}
+
static int pm88x_setup_irq(struct pm88x_chip *chip)
{
int ret;
@@ -155,6 +184,10 @@ static int pm88x_probe(struct i2c_client *client)
return -EINVAL;
}
+ ret = pm88x_initialize_subregmaps(chip);
+ if (ret)
+ return ret;
+
ret = pm88x_setup_irq(chip);
if (ret)
return ret;
diff --git a/include/linux/mfd/88pm88x.h b/include/linux/mfd/88pm88x.h
index 9a335f6b9c07..703e6104c1d8 100644
--- a/include/linux/mfd/88pm88x.h
+++ b/include/linux/mfd/88pm88x.h
@@ -39,8 +39,12 @@
#define PM88X_REG_AON_CTRL2 0xe2
+#define PM88X_PAGE_OFFSET_LDO 1
+
enum pm88x_regmap_index {
PM88X_REGMAP_BASE,
+ PM88X_REGMAP_LDO,
+ PM88X_REGMAP_BUCK,
PM88X_REGMAP_NR
};
--
2.43.0
On 28/12/2023 10:39, Karel Balej wrote:
> From: Karel Balej <[email protected]>
>
A nit, subject: drop second/last, redundant "documentation entry". The
"dt-bindings" prefix is already stating that these are bindings.
> The Marvell 88PM88X PMICs provide regulators among other things.
> Document how to use them.
You did not document them in the bindings. You just added example to
make it complete. Where is the binding change?
What's more, I have doubts that you tested it.
>
> Signed-off-by: Karel Balej <[email protected]>
> ---
> .../bindings/mfd/marvell,88pm88x.yaml | 17 +++++++++++
> .../regulator/marvell,88pm88x-regulator.yaml | 28 +++++++++++++++++++
> 2 files changed, 45 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/regulator/marvell,88pm88x-regulator.yaml
>
> diff --git a/Documentation/devicetree/bindings/mfd/marvell,88pm88x.yaml b/Documentation/devicetree/bindings/mfd/marvell,88pm88x.yaml
> index 115b41c9f22c..e6944369fc5c 100644
> --- a/Documentation/devicetree/bindings/mfd/marvell,88pm88x.yaml
> +++ b/Documentation/devicetree/bindings/mfd/marvell,88pm88x.yaml
> @@ -54,6 +54,23 @@ examples:
> onkey {
> compatible = "marvell,88pm88x-onkey";
> };
> +
> + regulators {
> + ldo2: ldo2 {
> + regulator-min-microvolt = <3100000>;
> + regulator-max-microvolt = <3300000>;
> + };
> +
> + ldo15: ldo15 {
> + regulator-min-microvolt = <3300000>;
> + regulator-max-microvolt = <3300000>;
> + };
> +
> + buck2: buck2 {
> + regulator-min-microvolt = <1800000>;
> + regulator-max-microvolt = <1800000>;
> + };
> + };
> };
> };
> ...
> diff --git a/Documentation/devicetree/bindings/regulator/marvell,88pm88x-regulator.yaml b/Documentation/devicetree/bindings/regulator/marvell,88pm88x-regulator.yaml
> new file mode 100644
> index 000000000000..c6ac17b113e7
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/regulator/marvell,88pm88x-regulator.yaml
> @@ -0,0 +1,28 @@
> +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/regulator/marvell,88pm88x-regulator.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Marvell 88PM88X PMICs regulators
> +
> +maintainers:
> + - Karel Balej <[email protected]>
> +
> +description: |
> + This module is part of the Marvell 88PM88X MFD device. For more details
> + see Documentation/devicetree/bindings/mfd/marvell,88pm88x.yaml.
> +
> + The regulator controller is represented as a sub-node of the PMIC node
> + in the device tree.
> +
> + The valid names for 88PM886 regulator nodes are ldo[1-9], ldo1[0-6], buck[1-5].
Don't repeat constraints in free form text.
> +
> +patternProperties:
> + "^(ldo|buck)[0-9]+$":
You need to fix the pattern to be narrow. buck0 and buck6 are not correct.
> + type: object
> + description:
> + Properties for single regulator.
> + $ref: regulator.yaml#
Not many benefits of this being in its own schema.
> +
> +additionalProperties: false
Best regards,
Krzysztof
On 04/01/2024 10:25, Krzysztof Kozlowski wrote:
> On 28/12/2023 10:39, Karel Balej wrote:
>> From: Karel Balej <[email protected]>
>>
>
> A nit, subject: drop second/last, redundant "documentation entry". The
> "dt-bindings" prefix is already stating that these are bindings.
>
>> The Marvell 88PM88X PMICs provide regulators among other things.
>> Document how to use them.
>
>
> You did not document them in the bindings. You just added example to
> make it complete. Where is the binding change?
>
> What's more, I have doubts that you tested it.
I found your other patchset - now I am 100% sure you did not test it.
Best regards,
Krzysztof
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.
The subject needs work. Please tell us what the patches is doing.
On Thu, 28 Dec 2023, Karel Balej wrote:
> From: Karel Balej <[email protected]>
A full an complete commit message is a must.
> Signed-off-by: Karel Balej <[email protected]>
> ---
> drivers/mfd/88pm88x.c | 14 ++++++++------
> include/linux/mfd/88pm88x.h | 2 ++
> 2 files changed, 10 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/mfd/88pm88x.c b/drivers/mfd/88pm88x.c
> index 5db6c65b667d..3424d88a58f6 100644
> --- a/drivers/mfd/88pm88x.c
> +++ b/drivers/mfd/88pm88x.c
> @@ -57,16 +57,16 @@ static struct reg_sequence pm886_presets[] = {
> REG_SEQ0(PM88X_REG_BK_OSC_CTRL3, 0xc0),
> };
>
> -static struct resource onkey_resources[] = {
> +static struct resource pm88x_onkey_resources[] = {
> DEFINE_RES_IRQ_NAMED(PM88X_IRQ_ONKEY, "88pm88x-onkey"),
> };
>
> -static struct mfd_cell pm88x_devs[] = {
> +static struct mfd_cell pm886_devs[] = {
> {
> .name = "88pm88x-onkey",
> - .num_resources = ARRAY_SIZE(onkey_resources),
> - .resources = onkey_resources,
> - .id = -1,
> + .of_compatible = "marvell,88pm88x-onkey",
> + .num_resources = ARRAY_SIZE(pm88x_onkey_resources),
> + .resources = pm88x_onkey_resources,
> },
> };
>
> @@ -74,6 +74,8 @@ static struct pm88x_data pm886_a1_data = {
> .whoami = PM886_A1_WHOAMI,
> .presets = pm886_presets,
> .num_presets = ARRAY_SIZE(pm886_presets),
> + .devs = pm886_devs,
> + .num_devs = ARRAY_SIZE(pm886_devs),
> };
>
> static const struct regmap_config pm88x_i2c_regmap = {
> @@ -157,7 +159,7 @@ static int pm88x_probe(struct i2c_client *client)
> if (ret)
> return ret;
>
> - ret = devm_mfd_add_devices(&client->dev, 0, pm88x_devs, ARRAY_SIZE(pm88x_devs),
> + ret = devm_mfd_add_devices(&client->dev, 0, chip->data->devs, chip->data->num_devs,
> NULL, 0, regmap_irq_get_domain(chip->irq_data));
> if (ret) {
> dev_err(&client->dev, "Failed to add devices: %d\n", ret);
> diff --git a/include/linux/mfd/88pm88x.h b/include/linux/mfd/88pm88x.h
> index a34c57447827..9a335f6b9c07 100644
> --- a/include/linux/mfd/88pm88x.h
> +++ b/include/linux/mfd/88pm88x.h
> @@ -49,6 +49,8 @@ struct pm88x_data {
> unsigned int whoami;
> struct reg_sequence *presets;
> unsigned int num_presets;
> + struct mfd_cell *devs;
> + unsigned int num_devs;
Why are you adding extra abstraction?
> };
>
> struct pm88x_chip {
> --
> 2.43.0
>
--
Lee Jones [李琼斯]
Lee,
On Thu Jan 11, 2024 at 11:54 AM CET, Lee Jones wrote:
> The subject needs work. Please tell us what the patches is doing.
>
> On Thu, 28 Dec 2023, Karel Balej wrote:
>
> > From: Karel Balej <[email protected]>
>
> A full an complete commit message is a must.
I have not provided a detailed description here because as I have noted
in the cover letter, this patch will be squashed into the MFD series. I
sent it only as a bridge between the two series, sorry for the
confusion.
> > diff --git a/include/linux/mfd/88pm88x.h b/include/linux/mfd/88pm88x.h
> > index a34c57447827..9a335f6b9c07 100644
> > --- a/include/linux/mfd/88pm88x.h
> > +++ b/include/linux/mfd/88pm88x.h
> > @@ -49,6 +49,8 @@ struct pm88x_data {
> > unsigned int whoami;
> > struct reg_sequence *presets;
> > unsigned int num_presets;
> > + struct mfd_cell *devs;
> > + unsigned int num_devs;
>
> Why are you adding extra abstraction?
Right, this is probably not necessary now since I'm only implementing
support for one of the chips - it's just that I keep thinking about it
as a driver for both of them and thus tend to write it a bit more
abstractly. Shall I then drop this and also the `presets` member which
is also chip-specific?
Thank you, best regards,
K. B.
On Thu, 11 Jan 2024, Karel Balej wrote:
> Lee,
>
> On Thu Jan 11, 2024 at 11:54 AM CET, Lee Jones wrote:
> > The subject needs work. Please tell us what the patches is doing.
> >
> > On Thu, 28 Dec 2023, Karel Balej wrote:
> >
> > > From: Karel Balej <[email protected]>
> >
> > A full an complete commit message is a must.
>
> I have not provided a detailed description here because as I have noted
> in the cover letter, this patch will be squashed into the MFD series. I
> sent it only as a bridge between the two series, sorry for the
> confusion.
>
> > > diff --git a/include/linux/mfd/88pm88x.h b/include/linux/mfd/88pm88x.h
> > > index a34c57447827..9a335f6b9c07 100644
> > > --- a/include/linux/mfd/88pm88x.h
> > > +++ b/include/linux/mfd/88pm88x.h
> > > @@ -49,6 +49,8 @@ struct pm88x_data {
> > > unsigned int whoami;
> > > struct reg_sequence *presets;
> > > unsigned int num_presets;
> > > + struct mfd_cell *devs;
> > > + unsigned int num_devs;
> >
> > Why are you adding extra abstraction?
>
> Right, this is probably not necessary now since I'm only implementing
> support for one of the chips - it's just that I keep thinking about it
> as a driver for both of them and thus tend to write it a bit more
> abstractly. Shall I then drop this and also the `presets` member which
> is also chip-specific?
Even if you were to support multiple devices, this strategy is unusual
and isn't likely to be accepted.
With respect to the other variables, you are in a better position to
know if they are still required. By the sounds of it, I'd suggest it
might be better to remove them.
--
Lee Jones [李琼斯]
On Thu Jan 11, 2024 at 4:25 PM CET, Lee Jones wrote:
[...]
> > > > diff --git a/include/linux/mfd/88pm88x.h b/include/linux/mfd/88pm88x.h
> > > > index a34c57447827..9a335f6b9c07 100644
> > > > --- a/include/linux/mfd/88pm88x.h
> > > > +++ b/include/linux/mfd/88pm88x.h
> > > > @@ -49,6 +49,8 @@ struct pm88x_data {
> > > > unsigned int whoami;
> > > > struct reg_sequence *presets;
> > > > unsigned int num_presets;
> > > > + struct mfd_cell *devs;
> > > > + unsigned int num_devs;
> > >
> > > Why are you adding extra abstraction?
> >
> > Right, this is probably not necessary now since I'm only implementing
> > support for one of the chips - it's just that I keep thinking about it
> > as a driver for both of them and thus tend to write it a bit more
> > abstractly. Shall I then drop this and also the `presets` member which
> > is also chip-specific?
>
> Even if you were to support multiple devices, this strategy is unusual
> and isn't likely to be accepted.
May I please ask what the recommended strategy is then? `switch`ing on
the chip ID? I have taken this approach because it seemed to produce a
cleaner/more straightforward code in comparison to that. Or are you only
talking about the chip cells/subdevices in particular?
Thank you,
K. B.
On Thu, 11 Jan 2024, Karel Balej wrote:
> On Thu Jan 11, 2024 at 4:25 PM CET, Lee Jones wrote:
>
> [...]
>
> > > > > diff --git a/include/linux/mfd/88pm88x.h b/include/linux/mfd/88pm88x.h
> > > > > index a34c57447827..9a335f6b9c07 100644
> > > > > --- a/include/linux/mfd/88pm88x.h
> > > > > +++ b/include/linux/mfd/88pm88x.h
> > > > > @@ -49,6 +49,8 @@ struct pm88x_data {
> > > > > unsigned int whoami;
> > > > > struct reg_sequence *presets;
> > > > > unsigned int num_presets;
> > > > > + struct mfd_cell *devs;
> > > > > + unsigned int num_devs;
> > > >
> > > > Why are you adding extra abstraction?
> > >
> > > Right, this is probably not necessary now since I'm only implementing
> > > support for one of the chips - it's just that I keep thinking about it
> > > as a driver for both of them and thus tend to write it a bit more
> > > abstractly. Shall I then drop this and also the `presets` member which
> > > is also chip-specific?
> >
> > Even if you were to support multiple devices, this strategy is unusual
> > and isn't likely to be accepted.
>
> May I please ask what the recommended strategy is then? `switch`ing on
> the chip ID? I have taken this approach because it seemed to produce a
> cleaner/more straightforward code in comparison to that. Or are you only
> talking about the chip cells/subdevices in particular?
I'd have to see the implementation, but the general exception I'm taking
here is storing the use-once MFD cell data inside a data structure. If
you were to match on device ID it's *sometimes* acceptable to store the
pointers in local variables.
--
Lee Jones [李琼斯]