* [RFC PATCH 0/5] regulator: support for Marvell 88PM886 LDOs and bucks @ 2023-12-28 9:39 Karel Balej 2023-12-28 9:39 ` [RFC PATCH 1/5] mfd: 88pm88x: differences with respect to the PMIC RFC series Karel Balej ` (4 more replies) 0 siblings, 5 replies; 20+ messages in thread From: Karel Balej @ 2023-12-28 9:39 UTC (permalink / raw) To: Karel Balej, Lee Jones, Rob Herring, Krzysztof Kozlowski, Conor Dooley, Liam Girdwood, Mark Brown, devicetree, linux-kernel Cc: Duje Mihanović, ~postmarketos/upstreaming, phone-devel From: Karel Balej <balejk@matfyz.cz> 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/20231217131838.7569-1-karelb@gimli.ms.mff.cuni.cz/ [2] https://lore.kernel.org/all/20231102-pxa1908-lkml-v7-0-cabb1a0cb52b@skole.hr/ [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 ^ permalink raw reply [flat|nested] 20+ messages in thread
* [RFC PATCH 1/5] mfd: 88pm88x: differences with respect to the PMIC RFC series 2023-12-28 9:39 [RFC PATCH 0/5] regulator: support for Marvell 88PM886 LDOs and bucks Karel Balej @ 2023-12-28 9:39 ` Karel Balej 2024-01-11 10:54 ` Lee Jones 2023-12-28 9:39 ` [RFC PATCH 2/5] mfd: 88pm88x: initialize the regulators regmaps Karel Balej ` (3 subsequent siblings) 4 siblings, 1 reply; 20+ messages in thread From: Karel Balej @ 2023-12-28 9:39 UTC (permalink / raw) To: Karel Balej, Lee Jones, Rob Herring, Krzysztof Kozlowski, Conor Dooley, Liam Girdwood, Mark Brown, devicetree, linux-kernel Cc: Duje Mihanović, ~postmarketos/upstreaming, phone-devel From: Karel Balej <balejk@matfyz.cz> Signed-off-by: Karel Balej <balejk@matfyz.cz> --- 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 ^ permalink raw reply related [flat|nested] 20+ messages in thread
* Re: [RFC PATCH 1/5] mfd: 88pm88x: differences with respect to the PMIC RFC series 2023-12-28 9:39 ` [RFC PATCH 1/5] mfd: 88pm88x: differences with respect to the PMIC RFC series Karel Balej @ 2024-01-11 10:54 ` Lee Jones 2024-01-11 15:06 ` Karel Balej 0 siblings, 1 reply; 20+ messages in thread From: Lee Jones @ 2024-01-11 10:54 UTC (permalink / raw) To: Karel Balej Cc: Karel Balej, Rob Herring, Krzysztof Kozlowski, Conor Dooley, Liam Girdwood, Mark Brown, devicetree, linux-kernel, Duje Mihanović, ~postmarketos/upstreaming, phone-devel The subject needs work. Please tell us what the patches is doing. On Thu, 28 Dec 2023, Karel Balej wrote: > From: Karel Balej <balejk@matfyz.cz> A full an complete commit message is a must. > Signed-off-by: Karel Balej <balejk@matfyz.cz> > --- > 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 [李琼斯] ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [RFC PATCH 1/5] mfd: 88pm88x: differences with respect to the PMIC RFC series 2024-01-11 10:54 ` Lee Jones @ 2024-01-11 15:06 ` Karel Balej 2024-01-11 15:25 ` Lee Jones 0 siblings, 1 reply; 20+ messages in thread From: Karel Balej @ 2024-01-11 15:06 UTC (permalink / raw) To: Lee Jones Cc: Karel Balej, Rob Herring, Krzysztof Kozlowski, Conor Dooley, Liam Girdwood, Mark Brown, devicetree, linux-kernel, Duje Mihanović, ~postmarketos/upstreaming, phone-devel 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 <balejk@matfyz.cz> > > 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. ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [RFC PATCH 1/5] mfd: 88pm88x: differences with respect to the PMIC RFC series 2024-01-11 15:06 ` Karel Balej @ 2024-01-11 15:25 ` Lee Jones 2024-01-11 15:36 ` Karel Balej 0 siblings, 1 reply; 20+ messages in thread From: Lee Jones @ 2024-01-11 15:25 UTC (permalink / raw) To: Karel Balej Cc: Karel Balej, Rob Herring, Krzysztof Kozlowski, Conor Dooley, Liam Girdwood, Mark Brown, devicetree, linux-kernel, Duje Mihanović, ~postmarketos/upstreaming, phone-devel 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 <balejk@matfyz.cz> > > > > 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 [李琼斯] ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [RFC PATCH 1/5] mfd: 88pm88x: differences with respect to the PMIC RFC series 2024-01-11 15:25 ` Lee Jones @ 2024-01-11 15:36 ` Karel Balej 2024-01-12 7:58 ` Lee Jones 0 siblings, 1 reply; 20+ messages in thread From: Karel Balej @ 2024-01-11 15:36 UTC (permalink / raw) To: Lee Jones Cc: Karel Balej, Rob Herring, Krzysztof Kozlowski, Conor Dooley, Liam Girdwood, Mark Brown, devicetree, linux-kernel, Duje Mihanović, ~postmarketos/upstreaming, phone-devel 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. ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [RFC PATCH 1/5] mfd: 88pm88x: differences with respect to the PMIC RFC series 2024-01-11 15:36 ` Karel Balej @ 2024-01-12 7:58 ` Lee Jones 0 siblings, 0 replies; 20+ messages in thread From: Lee Jones @ 2024-01-12 7:58 UTC (permalink / raw) To: Karel Balej Cc: Karel Balej, Rob Herring, Krzysztof Kozlowski, Conor Dooley, Liam Girdwood, Mark Brown, devicetree, linux-kernel, Duje Mihanović, ~postmarketos/upstreaming, phone-devel 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 [李琼斯] ^ permalink raw reply [flat|nested] 20+ messages in thread
* [RFC PATCH 2/5] mfd: 88pm88x: initialize the regulators regmaps 2023-12-28 9:39 [RFC PATCH 0/5] regulator: support for Marvell 88PM886 LDOs and bucks Karel Balej 2023-12-28 9:39 ` [RFC PATCH 1/5] mfd: 88pm88x: differences with respect to the PMIC RFC series Karel Balej @ 2023-12-28 9:39 ` Karel Balej 2023-12-28 9:39 ` [RFC PATCH 3/5] dt-bindings: regulator: add documentation entry for 88pm88x-regulator Karel Balej ` (2 subsequent siblings) 4 siblings, 0 replies; 20+ messages in thread From: Karel Balej @ 2023-12-28 9:39 UTC (permalink / raw) To: Karel Balej, Lee Jones, Rob Herring, Krzysztof Kozlowski, Conor Dooley, Liam Girdwood, Mark Brown, devicetree, linux-kernel Cc: Duje Mihanović, ~postmarketos/upstreaming, phone-devel From: Karel Balej <balejk@matfyz.cz> 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 <balejk@matfyz.cz> --- 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 ^ permalink raw reply related [flat|nested] 20+ messages in thread
* [RFC PATCH 3/5] dt-bindings: regulator: add documentation entry for 88pm88x-regulator 2023-12-28 9:39 [RFC PATCH 0/5] regulator: support for Marvell 88PM886 LDOs and bucks Karel Balej 2023-12-28 9:39 ` [RFC PATCH 1/5] mfd: 88pm88x: differences with respect to the PMIC RFC series Karel Balej 2023-12-28 9:39 ` [RFC PATCH 2/5] mfd: 88pm88x: initialize the regulators regmaps Karel Balej @ 2023-12-28 9:39 ` Karel Balej 2024-01-04 9:25 ` Krzysztof Kozlowski 2023-12-28 9:39 ` [RFC PATCH 4/5] regulator: add 88pm88x regulators driver Karel Balej 2023-12-28 9:39 ` [RFC PATCH 5/5] MAINTAINERS: add entries for the " Karel Balej 4 siblings, 1 reply; 20+ messages in thread From: Karel Balej @ 2023-12-28 9:39 UTC (permalink / raw) To: Karel Balej, Lee Jones, Rob Herring, Krzysztof Kozlowski, Conor Dooley, Liam Girdwood, Mark Brown, devicetree, linux-kernel Cc: Duje Mihanović, ~postmarketos/upstreaming, phone-devel From: Karel Balej <balejk@matfyz.cz> The Marvell 88PM88X PMICs provide regulators among other things. Document how to use them. Signed-off-by: Karel Balej <balejk@matfyz.cz> --- .../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 <balejk@matfyz.cz> + +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 ^ permalink raw reply related [flat|nested] 20+ messages in thread
* Re: [RFC PATCH 3/5] dt-bindings: regulator: add documentation entry for 88pm88x-regulator 2023-12-28 9:39 ` [RFC PATCH 3/5] dt-bindings: regulator: add documentation entry for 88pm88x-regulator Karel Balej @ 2024-01-04 9:25 ` Krzysztof Kozlowski 2024-01-04 9:26 ` Krzysztof Kozlowski 0 siblings, 1 reply; 20+ messages in thread From: Krzysztof Kozlowski @ 2024-01-04 9:25 UTC (permalink / raw) To: Karel Balej, Karel Balej, Lee Jones, Rob Herring, Krzysztof Kozlowski, Conor Dooley, Liam Girdwood, Mark Brown, devicetree, linux-kernel Cc: Duje Mihanović, ~postmarketos/upstreaming, phone-devel On 28/12/2023 10:39, Karel Balej wrote: > From: Karel Balej <balejk@matfyz.cz> > 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 <balejk@matfyz.cz> > --- > .../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 <balejk@matfyz.cz> > + > +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 ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [RFC PATCH 3/5] dt-bindings: regulator: add documentation entry for 88pm88x-regulator 2024-01-04 9:25 ` Krzysztof Kozlowski @ 2024-01-04 9:26 ` Krzysztof Kozlowski 0 siblings, 0 replies; 20+ messages in thread From: Krzysztof Kozlowski @ 2024-01-04 9:26 UTC (permalink / raw) To: Karel Balej, Karel Balej, Lee Jones, Rob Herring, Krzysztof Kozlowski, Conor Dooley, Liam Girdwood, Mark Brown, devicetree, linux-kernel Cc: Duje Mihanović, ~postmarketos/upstreaming, phone-devel On 04/01/2024 10:25, Krzysztof Kozlowski wrote: > On 28/12/2023 10:39, Karel Balej wrote: >> From: Karel Balej <balejk@matfyz.cz> >> > > 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 ^ permalink raw reply [flat|nested] 20+ messages in thread
* [RFC PATCH 4/5] regulator: add 88pm88x regulators driver 2023-12-28 9:39 [RFC PATCH 0/5] regulator: support for Marvell 88PM886 LDOs and bucks Karel Balej ` (2 preceding siblings ...) 2023-12-28 9:39 ` [RFC PATCH 3/5] dt-bindings: regulator: add documentation entry for 88pm88x-regulator Karel Balej @ 2023-12-28 9:39 ` Karel Balej 2024-01-05 15:18 ` Mark Brown 2024-01-07 10:35 ` Krzysztof Kozlowski 2023-12-28 9:39 ` [RFC PATCH 5/5] MAINTAINERS: add entries for the " Karel Balej 4 siblings, 2 replies; 20+ messages in thread From: Karel Balej @ 2023-12-28 9:39 UTC (permalink / raw) To: Karel Balej, Lee Jones, Rob Herring, Krzysztof Kozlowski, Conor Dooley, Liam Girdwood, Mark Brown, devicetree, linux-kernel Cc: Duje Mihanović, ~postmarketos/upstreaming, phone-devel 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 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, -- 2.43.0 ^ permalink raw reply related [flat|nested] 20+ messages in thread
* Re: [RFC PATCH 4/5] regulator: add 88pm88x regulators driver 2023-12-28 9:39 ` [RFC PATCH 4/5] regulator: add 88pm88x regulators driver Karel Balej @ 2024-01-05 15:18 ` Mark Brown 2024-01-07 9:49 ` Karel Balej 2024-01-07 10:35 ` Krzysztof Kozlowski 1 sibling, 1 reply; 20+ messages in thread From: Mark Brown @ 2024-01-05 15:18 UTC (permalink / raw) To: Karel Balej Cc: Karel Balej, Lee Jones, Rob Herring, Krzysztof Kozlowski, Conor Dooley, Liam Girdwood, devicetree, linux-kernel, Duje Mihanović, ~postmarketos/upstreaming, phone-devel [-- Attachment #1: Type: text/plain, Size: 535 bytes --] 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. [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 488 bytes --] ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [RFC PATCH 4/5] regulator: add 88pm88x regulators driver 2024-01-05 15:18 ` Mark Brown @ 2024-01-07 9:49 ` Karel Balej 2024-01-07 10:34 ` Krzysztof Kozlowski 2024-01-07 15:26 ` Mark Brown 0 siblings, 2 replies; 20+ messages in thread From: Karel Balej @ 2024-01-07 9:49 UTC (permalink / raw) To: Mark Brown Cc: Karel Balej, Lee Jones, Rob Herring, Krzysztof Kozlowski, Conor Dooley, Liam Girdwood, devicetree, linux-kernel, Duje Mihanović, ~postmarketos/upstreaming, phone-devel 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. ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [RFC PATCH 4/5] regulator: add 88pm88x regulators driver 2024-01-07 9:49 ` Karel Balej @ 2024-01-07 10:34 ` Krzysztof Kozlowski 2024-01-07 12:46 ` Karel Balej 2024-01-07 15:26 ` Mark Brown 1 sibling, 1 reply; 20+ messages in thread From: Krzysztof Kozlowski @ 2024-01-07 10:34 UTC (permalink / raw) To: Karel Balej, Mark Brown Cc: Karel Balej, Lee Jones, Rob Herring, Krzysztof Kozlowski, Conor Dooley, Liam Girdwood, devicetree, linux-kernel, Duje Mihanović, ~postmarketos/upstreaming, phone-devel 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 ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [RFC PATCH 4/5] regulator: add 88pm88x regulators driver 2024-01-07 10:34 ` Krzysztof Kozlowski @ 2024-01-07 12:46 ` Karel Balej 0 siblings, 0 replies; 20+ messages in thread From: Karel Balej @ 2024-01-07 12:46 UTC (permalink / raw) To: Krzysztof Kozlowski, Mark Brown Cc: Karel Balej, Lee Jones, Rob Herring, Krzysztof Kozlowski, Conor Dooley, Liam Girdwood, devicetree, linux-kernel, Duje Mihanović, ~postmarketos/upstreaming, phone-devel 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. ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [RFC PATCH 4/5] regulator: add 88pm88x regulators driver 2024-01-07 9:49 ` Karel Balej 2024-01-07 10:34 ` Krzysztof Kozlowski @ 2024-01-07 15:26 ` Mark Brown 1 sibling, 0 replies; 20+ messages in thread From: Mark Brown @ 2024-01-07 15:26 UTC (permalink / raw) To: Karel Balej Cc: Karel Balej, Lee Jones, Rob Herring, Krzysztof Kozlowski, Conor Dooley, Liam Girdwood, devicetree, linux-kernel, Duje Mihanović, ~postmarketos/upstreaming, phone-devel [-- Attachment #1: Type: text/plain, Size: 731 bytes --] 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. [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 488 bytes --] ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [RFC PATCH 4/5] regulator: add 88pm88x regulators driver 2023-12-28 9:39 ` [RFC PATCH 4/5] regulator: add 88pm88x regulators driver Karel Balej 2024-01-05 15:18 ` Mark Brown @ 2024-01-07 10:35 ` Krzysztof Kozlowski 2024-01-07 13:02 ` Karel Balej 1 sibling, 1 reply; 20+ messages in thread From: Krzysztof Kozlowski @ 2024-01-07 10:35 UTC (permalink / raw) To: Karel Balej, Karel Balej, Lee Jones, Rob Herring, Krzysztof Kozlowski, Conor Dooley, Liam Girdwood, Mark Brown, devicetree, linux-kernel Cc: Duje Mihanović, ~postmarketos/upstreaming, phone-devel 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 ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [RFC PATCH 4/5] regulator: add 88pm88x regulators driver 2024-01-07 10:35 ` Krzysztof Kozlowski @ 2024-01-07 13:02 ` Karel Balej 0 siblings, 0 replies; 20+ messages in thread From: Karel Balej @ 2024-01-07 13:02 UTC (permalink / raw) To: Krzysztof Kozlowski, Karel Balej, Lee Jones, Rob Herring, Krzysztof Kozlowski, Conor Dooley, Liam Girdwood, Mark Brown, devicetree, linux-kernel Cc: Duje Mihanović, ~postmarketos/upstreaming, phone-devel 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. ^ permalink raw reply [flat|nested] 20+ messages in thread
* [RFC PATCH 5/5] MAINTAINERS: add entries for the 88pm88x regulators driver 2023-12-28 9:39 [RFC PATCH 0/5] regulator: support for Marvell 88PM886 LDOs and bucks Karel Balej ` (3 preceding siblings ...) 2023-12-28 9:39 ` [RFC PATCH 4/5] regulator: add 88pm88x regulators driver Karel Balej @ 2023-12-28 9:39 ` Karel Balej 4 siblings, 0 replies; 20+ messages in thread From: Karel Balej @ 2023-12-28 9:39 UTC (permalink / raw) To: Karel Balej, Lee Jones, Rob Herring, Krzysztof Kozlowski, Conor Dooley, Liam Girdwood, Mark Brown, devicetree, linux-kernel Cc: Duje Mihanović, ~postmarketos/upstreaming, phone-devel From: Karel Balej <balejk@matfyz.cz> List the related files under the Marvell 88PM88X PMICs entry. Signed-off-by: Karel Balej <balejk@matfyz.cz> --- 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 <balejk@matfyz.cz> 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 ^ permalink raw reply related [flat|nested] 20+ messages in thread
end of thread, other threads:[~2024-01-12 7:58 UTC | newest] Thread overview: 20+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2023-12-28 9:39 [RFC PATCH 0/5] regulator: support for Marvell 88PM886 LDOs and bucks Karel Balej 2023-12-28 9:39 ` [RFC PATCH 1/5] mfd: 88pm88x: differences with respect to the PMIC RFC series Karel Balej 2024-01-11 10:54 ` Lee Jones 2024-01-11 15:06 ` Karel Balej 2024-01-11 15:25 ` Lee Jones 2024-01-11 15:36 ` Karel Balej 2024-01-12 7:58 ` Lee Jones 2023-12-28 9:39 ` [RFC PATCH 2/5] mfd: 88pm88x: initialize the regulators regmaps Karel Balej 2023-12-28 9:39 ` [RFC PATCH 3/5] dt-bindings: regulator: add documentation entry for 88pm88x-regulator Karel Balej 2024-01-04 9:25 ` Krzysztof Kozlowski 2024-01-04 9:26 ` Krzysztof Kozlowski 2023-12-28 9:39 ` [RFC PATCH 4/5] regulator: add 88pm88x regulators driver Karel Balej 2024-01-05 15:18 ` Mark Brown 2024-01-07 9:49 ` Karel Balej 2024-01-07 10:34 ` Krzysztof Kozlowski 2024-01-07 12:46 ` Karel Balej 2024-01-07 15:26 ` Mark Brown 2024-01-07 10:35 ` Krzysztof Kozlowski 2024-01-07 13:02 ` Karel Balej 2023-12-28 9:39 ` [RFC PATCH 5/5] MAINTAINERS: add entries for the " Karel Balej
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).