Message ID | 20250323-s2mpg10-v1-0-d08943702707@linaro.org |
---|---|
Headers | show |
Series | Samsung S2MPG10 PMIC MFD-based drivers | expand |
Hi Rob, Thanks for your review! On Mon, 2025-03-24 at 11:55 -0500, Rob Herring wrote: > On Sun, Mar 23, 2025 at 10:39:17PM +0000, André Draszik wrote: > > The Samsung S2MPG10 PMIC is similar to the existing PMICs supported by > > this binding. > > > > It is a Power Management IC for mobile applications with buck > > converters, various LDOs, power meters, RTC, clock outputs, and > > additional GPIOs interfaces. > > > > Unlike other Samsung PMICs, communication is not via I2C, but via the > > Samsung ACPM firmware, it therefore doesn't need a 'reg' property but a > > handle to the ACPM firmware node instead. > > Can it be a child node of the ACPM node instead? That should work, I'll do that instead so. > > > > S2MPG10 can also act as a system power controller allowing > > implementation of a true cold-reset of the system. > > > > Support for the other components will be added in subsequent future > > patches. > > > > Signed-off-by: André Draszik <andre.draszik@linaro.org> > > --- > > .../devicetree/bindings/mfd/samsung,s2mps11.yaml | 34 ++++++++++++++++++++-- > > 1 file changed, 32 insertions(+), 2 deletions(-) > > > > diff --git a/Documentation/devicetree/bindings/mfd/samsung,s2mps11.yaml b/Documentation/devicetree/bindings/mfd/samsung,s2mps11.yaml > > index ac5d0c149796b6a4034b5d4245bfa8be0433cfab..ae8adb80b3af7ec3722c2a5718ad8fddf0a5df34 100644 > > --- a/Documentation/devicetree/bindings/mfd/samsung,s2mps11.yaml > > +++ b/Documentation/devicetree/bindings/mfd/samsung,s2mps11.yaml > > @@ -20,6 +20,7 @@ description: | > > properties: > > compatible: > > enum: > > + - samsung,s2mpg10-pmic > > - samsung,s2mps11-pmic > > - samsung,s2mps13-pmic > > - samsung,s2mps14-pmic > > @@ -43,6 +44,12 @@ properties: > > description: > > List of child nodes that specify the regulators. > > > > + exynos,acpm-ipc: > > + $ref: /schemas/types.yaml#/definitions/phandle > > + description: | > > Don't need '|' if no formatting to preserve. Oops, yes, sorry. Cheers, Andre' > > > + Phandle to the ACPM node for when ACPM is used to communicate with the > > + PMIC, rather than I2C. > > + > > samsung,s2mps11-acokb-ground: > > description: | > > Indicates that ACOKB pin of S2MPS11 PMIC is connected to the ground so > > @@ -58,16 +65,39 @@ properties: > > reset (setting buck voltages to default values). > > type: boolean > > > > + system-power-controller: true > > + > > wakeup-source: true > > > > required: > > - compatible > > - - reg > > - - regulators > > > > additionalProperties: false > > > > allOf: > > + - if: > > + properties: > > + compatible: > > + contains: > > + const: samsung,s2mpg10-pmic > > + then: > > + properties: > > + regulators: false > > + samsung,s2mps11-acokb-ground: false > > + samsung,s2mps11-wrstbi-ground: false > > + > > + required: > > + - exynos,acpm-ipc > > + > > + else: > > + properties: > > + exynos,acpm-ipc: false > > + system-power-controller: false > > + > > + required: > > + - reg > > + - regulators > > + > > - if: > > properties: > > compatible: > > > > -- > > 2.49.0.395.g12beb8f557-goog > >
Quoting André Draszik (2025-03-23 15:39:42) > Add support for Samsung's S2MPG10 PMIC clock, which is similar to the > existing PMIC clocks supported by this driver. > > S2MPG10 has three clock outputs @ 32kHz: AP, peri1 and peri2. > > Signed-off-by: André Draszik <andre.draszik@linaro.org> > --- Acked-by: Stephen Boyd <sboyd@kernel.org>
On 23/03/2025 23:39, André Draszik wrote: > The upcoming Samsung S2MPG10 PMIC driver will need this symbol to > communicate with the IC. > > Export it. > > Signed-off-by: André Draszik <andre.draszik@linaro.org> > --- > drivers/firmware/samsung/exynos-acpm.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/drivers/firmware/samsung/exynos-acpm.c b/drivers/firmware/samsung/exynos-acpm.c > index a85b2dbdd9f0d7b1f327f54a0a283e4f32587a98..7525bee4c6715edb964fc770ac9d8b3dd2be2172 100644 > --- a/drivers/firmware/samsung/exynos-acpm.c > +++ b/drivers/firmware/samsung/exynos-acpm.c > @@ -741,6 +741,7 @@ const struct acpm_handle *devm_acpm_get_by_phandle(struct device *dev, > > return handle; > } > +EXPORT_SYMBOL_GPL(devm_acpm_get_by_phandle); If binding changes to parent-child relationship, this might not be needed. Best regards, Krzysztof
On 23/03/2025 23:39, André Draszik wrote: > As a preparation for adding support for Samsung's S2MPG10, which is > connected via SPEEDY / ACPM rather than I2C, we're going to split out > (move) all I2C-specific driver code into its own kernel module, and > create a (common) core transport-agnostic kernel module. > > That move of code would highlight some unexpected alignment which > checkpatch would complain about. To avoid that, address the error now, > before the split, to keep the amount of unrelated changes to a minimum > when actually doing the split. > > Signed-off-by: André Draszik <andre.draszik@linaro.org> > --- > drivers/mfd/sec-core.c | 10 ++++++---- > 1 file changed, 6 insertions(+), 4 deletions(-) > > diff --git a/drivers/mfd/sec-core.c b/drivers/mfd/sec-core.c > index 83693686567df61b5e09f7129dc6b01d69156ff3..b931f66f366571d93ce59c301265fe1c9550b37d 100644 > --- a/drivers/mfd/sec-core.c > +++ b/drivers/mfd/sec-core.c > @@ -276,10 +276,12 @@ sec_pmic_i2c_parse_dt_pdata(struct device *dev) > if (!pd) > return ERR_PTR(-ENOMEM); > > - pd->manual_poweroff = of_property_read_bool(dev->of_node, > - "samsung,s2mps11-acokb-ground"); > - pd->disable_wrstbi = of_property_read_bool(dev->of_node, > - "samsung,s2mps11-wrstbi-ground"); > + pd->manual_poweroff = > + of_property_read_bool(dev->of_node, > + "samsung,s2mps11-acokb-ground"); I don't think this code more readable. The continued line should be re-aligned. Best regards, Krzysztof
On 23/03/2025 23:39, André Draszik wrote: > As a preparation for adding support for Samsung's S2MPG10, which is > connected via SPEEDY / ACPM rather than I2C, we're going to split out > (move) all I2C-specific driver code into its own kernel module, and > create a (common) core transport-agnostic kernel module. > > Transport drivers will have to do device tree parsing, and the core > driver will allocate its own additional memory as needed. > > In preparation for that change, separate out runtime platform data > allocation from device tree parsing. > > Having this change will create less churn in the upcoming split of the > transport-specific parts. > > Signed-off-by: André Draszik <andre.draszik@linaro.org> Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> Best regards, Krzysztof
On 23/03/2025 23:39, André Draszik wrote: > There is no reason to have these two kernel modules separate. Having > them merged into one kernel module also slightly reduces memory > consumption and module load times a little. > > mapped size (lsmod): > before: after: > sec_core 20480 sec_core 24576 > sec_irq 16384 > ---------------- > total 36864 > > Section sizes (size -A): > before: after: > sec_core 6780 sec_core 13239 > sec_irq 8046 > ---------------- > Total 14826 > > Signed-off-by: André Draszik <andre.draszik@linaro.org> > > --- Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> Best regards, Krzysztof
On 23/03/2025 23:39, André Draszik wrote: > Sort struct of_device_id entries and the device type switch in _probe() > alphabetically, which makes it easier to find the right place where to > insert new entries in the future. > > Signed-off-by: André Draszik <andre.draszik@linaro.org> > --- > drivers/mfd/sec-i2c.c | 18 +++++++++--------- Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> Best regards, Krzysztof
On 23/03/2025 23:39, André Draszik wrote: > When support for PMICs without compatibles was removed in > commit f736d2c0caa8 ("mfd: sec: Remove PMICs without compatibles"), > sec_regmap_config effectively became an orphan, because S5M8763X was > the only user left of it before removal, using the default: case of the > switch statement. > > Subsequently, the accidental new users have been updated, so > sec_regmap_config is an orphan again, and can and should be removed > from the code. Doing so will also ensure future additions to support > new devices in this driver don't forget to add a regmap config. > > Drop this fallback regmap config. > > Signed-off-by: André Draszik <andre.draszik@linaro.org> > --- Please squash it with previous patch. Logically you want to get rid of default regmap config - that's the change you are making. Your previous patch - adding regmap_configs for these variants - makes no sense on its own, because they were using the same regmap config already - the default one. Best regards, Krzysztof
On Wed, 2025-03-26 at 08:03 +0100, Krzysztof Kozlowski wrote: > On 23/03/2025 23:39, André Draszik wrote: > > The upcoming Samsung S2MPG10 PMIC driver will need this symbol to > > communicate with the IC. > > > > Export it. > > > > Signed-off-by: André Draszik <andre.draszik@linaro.org> > > --- > > drivers/firmware/samsung/exynos-acpm.c | 1 + > > 1 file changed, 1 insertion(+) > > > > diff --git a/drivers/firmware/samsung/exynos-acpm.c b/drivers/firmware/samsung/exynos-acpm.c > > index a85b2dbdd9f0d7b1f327f54a0a283e4f32587a98..7525bee4c6715edb964fc770ac9d8b3dd2be2172 100644 > > --- a/drivers/firmware/samsung/exynos-acpm.c > > +++ b/drivers/firmware/samsung/exynos-acpm.c > > @@ -741,6 +741,7 @@ const struct acpm_handle *devm_acpm_get_by_phandle(struct device *dev, > > > > return handle; > > } > > +EXPORT_SYMBOL_GPL(devm_acpm_get_by_phandle); > > If binding changes to parent-child relationship, this might not be needed. Indeed, I've instead added devm_acpm_get_by_node(), and clients are meant to pass the parent node pointer into it. Cheers, Andre'
On Wed, 2025-03-26 at 08:06 +0100, Krzysztof Kozlowski wrote: > On 23/03/2025 23:39, André Draszik wrote: > > As a preparation for adding support for Samsung's S2MPG10, which is > > connected via SPEEDY / ACPM rather than I2C, we're going to split out > > (move) all I2C-specific driver code into its own kernel module, and > > create a (common) core transport-agnostic kernel module. > > > > That move of code would highlight some unexpected alignment which > > checkpatch would complain about. To avoid that, address the error now, > > before the split, to keep the amount of unrelated changes to a minimum > > when actually doing the split. > > > > Signed-off-by: André Draszik <andre.draszik@linaro.org> > > --- > > drivers/mfd/sec-core.c | 10 ++++++---- > > 1 file changed, 6 insertions(+), 4 deletions(-) > > > > diff --git a/drivers/mfd/sec-core.c b/drivers/mfd/sec-core.c > > index 83693686567df61b5e09f7129dc6b01d69156ff3..b931f66f366571d93ce59c301265fe1c9550b37d 100644 > > --- a/drivers/mfd/sec-core.c > > +++ b/drivers/mfd/sec-core.c > > @@ -276,10 +276,12 @@ sec_pmic_i2c_parse_dt_pdata(struct device *dev) > > if (!pd) > > return ERR_PTR(-ENOMEM); > > > > - pd->manual_poweroff = of_property_read_bool(dev->of_node, > > - "samsung,s2mps11-acokb-ground"); > > - pd->disable_wrstbi = of_property_read_bool(dev->of_node, > > - "samsung,s2mps11-wrstbi-ground"); > > + pd->manual_poweroff = > > + of_property_read_bool(dev->of_node, > > + "samsung,s2mps11-acokb-ground"); > > I don't think this code more readable. The continued line should be > re-aligned. Agree, but I've tried to stay below 80 columns. I'll just move the string to the right in the next version so it is aligned with the '(' (but becomes a longer line). Cheers, Andre'
On 26/03/2025 10:21, André Draszik wrote: > On Wed, 2025-03-26 at 08:06 +0100, Krzysztof Kozlowski wrote: >> On 23/03/2025 23:39, André Draszik wrote: >>> As a preparation for adding support for Samsung's S2MPG10, which is >>> connected via SPEEDY / ACPM rather than I2C, we're going to split out >>> (move) all I2C-specific driver code into its own kernel module, and >>> create a (common) core transport-agnostic kernel module. >>> >>> That move of code would highlight some unexpected alignment which >>> checkpatch would complain about. To avoid that, address the error now, >>> before the split, to keep the amount of unrelated changes to a minimum >>> when actually doing the split. >>> >>> Signed-off-by: André Draszik <andre.draszik@linaro.org> >>> --- >>> drivers/mfd/sec-core.c | 10 ++++++---- >>> 1 file changed, 6 insertions(+), 4 deletions(-) >>> >>> diff --git a/drivers/mfd/sec-core.c b/drivers/mfd/sec-core.c >>> index 83693686567df61b5e09f7129dc6b01d69156ff3..b931f66f366571d93ce59c301265fe1c9550b37d 100644 >>> --- a/drivers/mfd/sec-core.c >>> +++ b/drivers/mfd/sec-core.c >>> @@ -276,10 +276,12 @@ sec_pmic_i2c_parse_dt_pdata(struct device *dev) >>> if (!pd) >>> return ERR_PTR(-ENOMEM); >>> >>> - pd->manual_poweroff = of_property_read_bool(dev->of_node, >>> - "samsung,s2mps11-acokb-ground"); >>> - pd->disable_wrstbi = of_property_read_bool(dev->of_node, >>> - "samsung,s2mps11-wrstbi-ground"); >>> + pd->manual_poweroff = >>> + of_property_read_bool(dev->of_node, >>> + "samsung,s2mps11-acokb-ground"); >> >> I don't think this code more readable. The continued line should be >> re-aligned. > > Agree, but I've tried to stay below 80 columns. I'll just move the string to > the right in the next version so it is aligned with the '(' (but becomes a > longer line). Lee expressed in the past that he is happy with 100. Coding style accepts longer lines. Best regards, Krzysztof
On Sun, 23 Mar 2025, André Draszik wrote: > This series adds initial support for the Samsung S2MPG10 PMIC using the > MFD framework. This is a PMIC for mobile applications and is used on > the Google Pixel 6 and 6 Pro (oriole / raven). > > *** dependency note *** > > This depends on the Samsung ACPM driver in Linux next, and runtime > depends on some fixes for it: > https://lore.kernel.org/all/20250321-acpm-atomic-v1-0-fb887bde7e61@linaro.org/ > https://lore.kernel.org/all/20250319-acpm-fixes-v2-0-ac2c1bcf322b@linaro.org/ FTR, I have no intention of reviewing this until Krzysztof's review comments have been satisfied.
This series adds initial support for the Samsung S2MPG10 PMIC using the MFD framework. This is a PMIC for mobile applications and is used on the Google Pixel 6 and 6 Pro (oriole / raven). *** dependency note *** This depends on the Samsung ACPM driver in Linux next, and runtime depends on some fixes for it: https://lore.kernel.org/all/20250321-acpm-atomic-v1-0-fb887bde7e61@linaro.org/ https://lore.kernel.org/all/20250319-acpm-fixes-v2-0-ac2c1bcf322b@linaro.org/ *** dependency note end *** +++ Kconfig update +++ There is a Kconfig symbol update in this series, because the existing Samsung S2M driver has been split into core and transport (I2C & ACPM) parts. CONFIG_MFD_SEC_CORE is now truly a core driver, and the I2C code that was part of it is now enabled via CONFIG_MFD_SEC_I2C. This was necessary because unlike the other S2M PMICs, S2MPG10 doesn't talk via I2C, but via the Samsung ACPM firmware. +++ Kconfig update end +++ This series must be applied in-order, due to interdependencies of some of the patches. There are also various cleanup patches to the S2M drivers. I've kept them ordered as: * DT bindings (patches 1 ... 2) * EXPORT_SYMBOL ACPM's devm_acpm_get_by_phandle() (patch 3) * S2M MFD prep for adding S2MPG10 to MFD core (patches 4 ... 11) * S2MPG10 core driver (patch 12) * S2M MFD cleanup patches (patches 14 ... 25) * S2MPG10 clock driver (patch 26) * S2M RTC prep for adding S2MPG10 (patch 27 ... 28) * S2MPG10 RTC driver (patch 29) * S2M RTC cleanup patches (patches 30 ... 33) I realise these are many, but since some prep-work was required to be able to add S2MPG anyway, I wanted to get the cleanup patches in as well :-) Let me know if I should postpone them to a later date instead. The S2MPG10 includes buck converters, various LDOs, power meters, RTC, clock outputs, and additional GPIOs interfaces. This series adds support in the top-level device driver, and for the RTC and clock. Importantly, having the RTC driver allows to do a proper reset of the system. Drivers or driver updates for the other components will be added in future patches. This will need a DT update for Oriole / Raven to enable this device. I will send that out separately. Cheers, Andre' Signed-off-by: André Draszik <andre.draszik@linaro.org> --- André Draszik (34): dt-bindings: mfd: samsung,s2mps11: add s2mpg10 dt-bindings: clock: samsung,s2mps11: add s2mpg10 firmware: exynos-acpm: export devm_acpm_get_by_phandle() mfd: sec: drop non-existing forward declarations mfd: sec: sort includes alphabetically mfd: sec: update includes to add missing and remove superfluous ones mfd: sec: move private internal API to internal header mfd: sec: fix open parenthesis alignment (of_property_read_bool) mfd: sec: slightly rework runtime platform data allocation mfd: sec: split into core and transport (i2c) drivers defconfigs: rename CONFIG_MFD_SEC_CORE to CONFIG_MFD_SEC_I2C mfd: sec: add support for S2MPG10 PMIC mfd: sec: merge separate core and irq modules mfd: sec: sort struct of_device_id entries and the device type switch mfd: sec: use dev_err_probe() where appropriate mfd: sec: s2dos05/s2mpu05: use explicit regmap config mfd: sec: drop generic regmap config mfd: sec: s2dos05: doesn't support interrupts (it seems) mfd: sec: don't ignore errors from sec_irq_init() mfd: sec: rework platform data and regmap instantiating mfd: sec: change device_type to int mfd: sec: don't compare against NULL / 0 for errors, use ! mfd: sec: use sizeof(*var), not sizeof(struct type_of_var) mfd: sec: convert to using MFD_CELL macros mfd: sec: convert to using REGMAP_IRQ_REG() macros clk: s2mps11: add support for S2MPG10 PMIC clock rtc: s5m: cache value of platform_get_device_id() during probe rtc: s5m: prepare for external regmap rtc: s5m: add support for S2MPG10 RTC rtc: s5m: fix a typo: peding -> pending rtc: s5m: switch to devm_device_init_wakeup rtc: s5m: replace regmap_update_bits with regmap_clear/set_bits rtc: s5m: replace open-coded read/modify/write registers with regmap helpers MAINTAINERS: add myself as reviewer for Samsung S2M MFD .../devicetree/bindings/clock/samsung,s2mps11.yaml | 1 + .../devicetree/bindings/mfd/samsung,s2mps11.yaml | 34 +- MAINTAINERS | 3 +- arch/arm/configs/exynos_defconfig | 2 +- arch/arm/configs/multi_v7_defconfig | 2 +- arch/arm/configs/pxa_defconfig | 2 +- arch/arm64/configs/defconfig | 2 +- drivers/clk/clk-s2mps11.c | 8 + drivers/firmware/samsung/exynos-acpm.c | 1 + drivers/mfd/Kconfig | 35 +- drivers/mfd/Makefile | 5 +- drivers/mfd/sec-acpm.c | 471 ++++++++++++++++++++ drivers/mfd/sec-common.c | 284 ++++++++++++ drivers/mfd/sec-core.c | 481 --------------------- drivers/mfd/sec-core.h | 32 ++ drivers/mfd/sec-i2c.c | 259 +++++++++++ drivers/mfd/sec-irq.c | 461 +++++++------------- drivers/rtc/rtc-s5m.c | 197 ++++++--- include/linux/mfd/samsung/core.h | 7 +- include/linux/mfd/samsung/irq.h | 103 +++++ include/linux/mfd/samsung/rtc.h | 37 ++ include/linux/mfd/samsung/s2mpg10.h | 310 +++++++++++++ 22 files changed, 1872 insertions(+), 865 deletions(-) --- base-commit: c4d4884b67802c41fd67399747165d65c770621a change-id: 20250321-s2mpg10-ef5d1ebd3043 Best regards,