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 |
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.
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 <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,