| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <45ce203c03ec34631a0170baa7e4cf26c98b9cd3.camel@linaro.org>
Date: Thu, 13 Nov 2025 21:43:29 +0000
From: André Draszik <andre.draszik@...aro.org>
To: Lee Jones <lee@...nel.org>
Cc: Tudor Ambarus <tudor.ambarus@...aro.org>, Rob Herring <robh@...nel.org>,
Conor Dooley <conor+dt@...nel.org>, Krzysztof Kozlowski <krzk@...nel.org>,
Liam Girdwood <lgirdwood@...il.com>, Mark Brown <broonie@...nel.org>,
Linus Walleij <linus.walleij@...aro.org>, Bartosz Golaszewski
<brgl@...ev.pl>, Krzysztof Kozlowski <krzk+dt@...nel.org>, Peter Griffin
<peter.griffin@...aro.org>, Will McVicker <willmcvicker@...gle.com>,
kernel-team@...roid.com, linux-kernel@...r.kernel.org,
linux-samsung-soc@...r.kernel.org, devicetree@...r.kernel.org,
linux-gpio@...r.kernel.org
Subject: Re: [PATCH v3 09/20] mfd: sec: Add support for S2MPG11 PMIC via ACPM
Hi Lee,
Thanks for your review.
On Thu, 2025-11-13 at 16:25 +0000, Lee Jones wrote:
> Mark, something for you below.
>
> > Add support for Samsung's S2MPG11 PMIC, which is a Power Management IC
> > for mobile applications with buck converters, various LDOs, power
> > meters, NTC thermistor inputs, and additional GPIO interfaces. It
> > typically complements an S2MPG10 PMIC in a main/sub configuration as
> > the sub-PMIC.
> >
> > Like S2MPG10, communication is not via I2C, but via the Samsung ACPM
> > firmware.
> >
> > Also like S2MPG10, the regulator rails will need to be instantiated
> > individually to allow probe to succeed due to rails being used as
> > supplies for S2MPG10, and to avoid supply rails from being disabled
> > unexpectedly due to probe deferral.
> >
> > Note: The firmware uses the ACPM channel ID and the Speedy channel ID
> > to select the PMIC address. Since these are firmware properties, they
> > can not be retrieved from DT, but instead are deducted from the
> > compatible for now.
> >
> > Signed-off-by: André Draszik <andre.draszik@...aro.org>
> >
> > ---
> > Note: checkpatch suggests to update MAINTAINERS, but the new file is
> > covered already due to using a wildcard.
> >
> > v3:
> > - mention NTC thermistor inputs in commit message
> > - one instance per actual rail, not per rail type (LDO or buck)
> >
> > v2:
> > - mention GPIOs in commit message
> > ---
> > drivers/mfd/sec-acpm.c | 213 +++++++++++++++++-
> > drivers/mfd/sec-common.c | 45 +++-
> > drivers/mfd/sec-irq.c | 67 +++++-
> > include/linux/mfd/samsung/core.h | 1 +
> > include/linux/mfd/samsung/irq.h | 99 ++++++++
> > include/linux/mfd/samsung/s2mpg11.h | 434 ++++++++++++++++++++++++++++++++++++
> > 6 files changed, 848 insertions(+), 11 deletions(-)
> >
> > diff --git a/drivers/mfd/sec-acpm.c b/drivers/mfd/sec-acpm.c
> > index 8b31c816d65b86c54a108fa994384abfac0e7da4..b44af6f8b1cdfcb75cf9d4c55c9d973a88fd510c 100644
> > --- a/drivers/mfd/sec-acpm.c
> > +++ b/drivers/mfd/sec-acpm.c
> > @@ -13,6 +13,7 @@
> > #include <linux/mfd/samsung/core.h>
> > #include <linux/mfd/samsung/rtc.h>
> > #include <linux/mfd/samsung/s2mpg10.h>
> > +#include <linux/mfd/samsung/s2mpg11.h>
> > #include <linux/mod_devicetable.h>
> > #include <linux/module.h>
> > #include <linux/of.h>
> > @@ -216,6 +217,155 @@ static const struct regmap_config s2mpg10_regmap_config_meter = {
> > .cache_type = REGCACHE_FLAT,
> > };
> >
> > +static const struct regmap_range s2mpg11_common_registers[] = {
> > + regmap_reg_range(0x00, 0x02), /* CHIP_ID_S, INT, INT_MASK */
> > + regmap_reg_range(0x0a, 0x0c), /* Speedy control */
> > + regmap_reg_range(0x1a, 0x27), /* Debug */
>
> These numbers are usually defined, and rightfully so.
We already had this discussion when S2MPG10 core support was added, so
I used the same approach again for consistency.
>
> > +};
> > +
> > +static const struct regmap_range s2mpg11_common_ro_registers[] = {
> > + regmap_reg_range(0x00, 0x01), /* CHIP_ID_S, INT */
> > + regmap_reg_range(0x25, 0x27), /* Debug */
> > +};
> > +
> > +static const struct regmap_range s2mpg11_common_nonvolatile_registers[] = {
> > + regmap_reg_range(0x00, 0x00), /* CHIP_ID_S */
> > + regmap_reg_range(0x02, 0x02), /* INT_MASK */
> > + regmap_reg_range(0x0a, 0x0c), /* Speedy control */
> > +};
> > +
> > +static const struct regmap_range s2mpg11_common_precious_registers[] = {
> > + regmap_reg_range(0x01, 0x01), /* INT */
> > +};
> > +
> > +static const struct regmap_access_table s2mpg11_common_wr_table = {
> > + .yes_ranges = s2mpg11_common_registers,
> > + .n_yes_ranges = ARRAY_SIZE(s2mpg11_common_registers),
> > + .no_ranges = s2mpg11_common_ro_registers,
> > + .n_no_ranges = ARRAY_SIZE(s2mpg11_common_ro_registers),
> > +};
> > +
> > +static const struct regmap_access_table s2mpg11_common_rd_table = {
> > + .yes_ranges = s2mpg11_common_registers,
> > + .n_yes_ranges = ARRAY_SIZE(s2mpg11_common_registers),
> > +};
> > +
> > +static const struct regmap_access_table s2mpg11_common_volatile_table = {
> > + .no_ranges = s2mpg11_common_nonvolatile_registers,
> > + .n_no_ranges = ARRAY_SIZE(s2mpg11_common_nonvolatile_registers),
> > +};
> > +
> > +static const struct regmap_access_table s2mpg11_common_precious_table = {
> > + .yes_ranges = s2mpg11_common_precious_registers,
> > + .n_yes_ranges = ARRAY_SIZE(s2mpg11_common_precious_registers),
> > +};
> > +
> > +static const struct regmap_config s2mpg11_regmap_config_common = {
> > + .name = "common",
> > + .reg_bits = ACPM_ADDR_BITS,
> > + .val_bits = 8,
> > + .max_register = S2MPG11_COMMON_SPD_DEBUG4,
> > + .wr_table = &s2mpg11_common_wr_table,
> > + .rd_table = &s2mpg11_common_rd_table,
> > + .volatile_table = &s2mpg11_common_volatile_table,
> > + .precious_table = &s2mpg11_common_precious_table,
> > + .num_reg_defaults_raw = S2MPG11_COMMON_SPD_DEBUG4 + 1,
> > + .cache_type = REGCACHE_FLAT,
> > +};
> > +
> > +static const struct regmap_range s2mpg11_pmic_registers[] = {
> > + regmap_reg_range(0x00, 0x5a), /* All PMIC registers */
> > + regmap_reg_range(0x5c, 0xb7), /* All PMIC registers */
> > +};
> > +
> > +static const struct regmap_range s2mpg11_pmic_ro_registers[] = {
> > + regmap_reg_range(0x00, 0x05), /* INTx */
> > + regmap_reg_range(0x0c, 0x0d), /* STATUS OFFSRC */
> > + regmap_reg_range(0x98, 0x98), /* GPIO input */
> > +};
> > +
> > +static const struct regmap_range s2mpg11_pmic_nonvolatile_registers[] = {
> > + regmap_reg_range(0x06, 0x0b), /* INTxM */
> > +};
> > +
> > +static const struct regmap_range s2mpg11_pmic_precious_registers[] = {
> > + regmap_reg_range(0x00, 0x05), /* INTx */
> > +};
> > +
> > +static const struct regmap_access_table s2mpg11_pmic_wr_table = {
> > + .yes_ranges = s2mpg11_pmic_registers,
> > + .n_yes_ranges = ARRAY_SIZE(s2mpg11_pmic_registers),
> > + .no_ranges = s2mpg11_pmic_ro_registers,
> > + .n_no_ranges = ARRAY_SIZE(s2mpg11_pmic_ro_registers),
> > +};
> > +
> > +static const struct regmap_access_table s2mpg11_pmic_rd_table = {
> > + .yes_ranges = s2mpg11_pmic_registers,
> > + .n_yes_ranges = ARRAY_SIZE(s2mpg11_pmic_registers),
> > +};
> > +
> > +static const struct regmap_access_table s2mpg11_pmic_volatile_table = {
> > + .no_ranges = s2mpg11_pmic_nonvolatile_registers,
> > + .n_no_ranges = ARRAY_SIZE(s2mpg11_pmic_nonvolatile_registers),
> > +};
> > +
> > +static const struct regmap_access_table s2mpg11_pmic_precious_table = {
> > + .yes_ranges = s2mpg11_pmic_precious_registers,
> > + .n_yes_ranges = ARRAY_SIZE(s2mpg11_pmic_precious_registers),
> > +};
> > +
> > +static const struct regmap_config s2mpg11_regmap_config_pmic = {
> > + .name = "pmic",
> > + .reg_bits = ACPM_ADDR_BITS,
> > + .val_bits = 8,
> > + .max_register = S2MPG11_PMIC_LDO_SENSE2,
> > + .wr_table = &s2mpg11_pmic_wr_table,
> > + .rd_table = &s2mpg11_pmic_rd_table,
> > + .volatile_table = &s2mpg11_pmic_volatile_table,
> > + .precious_table = &s2mpg11_pmic_precious_table,
> > + .num_reg_defaults_raw = S2MPG11_PMIC_LDO_SENSE2 + 1,
> > + .cache_type = REGCACHE_FLAT,
> > +};
> > +
> > +static const struct regmap_range s2mpg11_meter_registers[] = {
> > + regmap_reg_range(0x00, 0x3e), /* Meter config */
> > + regmap_reg_range(0x40, 0x8a), /* Meter data */
> > + regmap_reg_range(0x8d, 0x9c), /* Meter data */
> > +};
> > +
> > +static const struct regmap_range s2mpg11_meter_ro_registers[] = {
> > + regmap_reg_range(0x40, 0x9c), /* Meter data */
> > +};
> > +
> > +static const struct regmap_access_table s2mpg11_meter_wr_table = {
> > + .yes_ranges = s2mpg11_meter_registers,
> > + .n_yes_ranges = ARRAY_SIZE(s2mpg11_meter_registers),
> > + .no_ranges = s2mpg11_meter_ro_registers,
> > + .n_no_ranges = ARRAY_SIZE(s2mpg11_meter_ro_registers),
> > +};
> > +
> > +static const struct regmap_access_table s2mpg11_meter_rd_table = {
> > + .yes_ranges = s2mpg11_meter_registers,
> > + .n_yes_ranges = ARRAY_SIZE(s2mpg11_meter_registers),
> > +};
> > +
> > +static const struct regmap_access_table s2mpg11_meter_volatile_table = {
> > + .yes_ranges = s2mpg11_meter_ro_registers,
> > + .n_yes_ranges = ARRAY_SIZE(s2mpg11_meter_ro_registers),
> > +};
> > +
> > +static const struct regmap_config s2mpg11_regmap_config_meter = {
> > + .name = "meter",
> > + .reg_bits = ACPM_ADDR_BITS,
> > + .val_bits = 8,
> > + .max_register = S2MPG11_METER_LPF_DATA_NTC7_2,
> > + .wr_table = &s2mpg11_meter_wr_table,
> > + .rd_table = &s2mpg11_meter_rd_table,
> > + .volatile_table = &s2mpg11_meter_volatile_table,
> > + .num_reg_defaults_raw = S2MPG11_METER_LPF_DATA_NTC7_2 + 1,
> > + .cache_type = REGCACHE_FLAT,
> > +};
> > +
> > struct sec_pmic_acpm_shared_bus_context {
> > const struct acpm_handle *acpm;
> > unsigned int acpm_chan_id;
> > @@ -325,16 +475,22 @@ static struct regmap *sec_pmic_acpm_regmap_init(struct device *dev,
> > return regmap;
> > }
> >
> > -static void sec_pmic_acpm_mask_common_irqs(void *regmap_common)
> > +static void sec_pmic_acpm_mask_common_s2mpg10_irqs(void *regmap_common)
> > {
> > regmap_write(regmap_common, S2MPG10_COMMON_INT_MASK, S2MPG10_COMMON_INT_SRC);
> > }
> >
> > +static void sec_pmic_acpm_mask_common_s2mpg11_irqs(void *regmap_common)
> > +{
> > + regmap_write(regmap_common, S2MPG11_COMMON_INT_MASK, S2MPG11_COMMON_INT_SRC);
> > +}
> > +
> > static int sec_pmic_acpm_probe(struct platform_device *pdev)
> > {
> > struct regmap *regmap_common, *regmap_pmic, *regmap;
> > const struct sec_pmic_acpm_platform_data *pdata;
> > struct sec_pmic_acpm_shared_bus_context *shared_ctx;
> > + void (*masq_irqs_handler)(void *data);
> > const struct acpm_handle *acpm;
> > struct device *dev = &pdev->dev;
> > int ret, irq;
> > @@ -365,7 +521,19 @@ static int sec_pmic_acpm_probe(struct platform_device *pdev)
> > return PTR_ERR(regmap_common);
> >
> > /* Mask all interrupts from 'common' block, until successful init */
> > - ret = regmap_write(regmap_common, S2MPG10_COMMON_INT_MASK, S2MPG10_COMMON_INT_SRC);
> > + switch (pdata->device_type) {
> > + case S2MPG10:
> > + ret = regmap_write(regmap_common, S2MPG10_COMMON_INT_MASK, S2MPG10_COMMON_INT_SRC);
> > + break;
> > +
> > + case S2MPG11:
> > + ret = regmap_write(regmap_common, S2MPG11_COMMON_INT_MASK, S2MPG11_COMMON_INT_SRC);
> > + break;
> > +
> > + default:
> > + return dev_err_probe(dev, -EINVAL, "Unsupported device type %d\n",
> > + pdata->device_type);
> > + }
> > if (ret)
> > return dev_err_probe(dev, ret, "failed to mask common block interrupts\n");
> >
> > @@ -374,10 +542,12 @@ static int sec_pmic_acpm_probe(struct platform_device *pdev)
> > if (IS_ERR(regmap_pmic))
> > return PTR_ERR(regmap_pmic);
> >
> > - regmap = sec_pmic_acpm_regmap_init(dev, shared_ctx, SEC_PMIC_ACPM_ACCESSTYPE_RTC,
> > - pdata->regmap_cfg_rtc, true);
> > - if (IS_ERR(regmap))
> > - return PTR_ERR(regmap);
> > + if (pdata->regmap_cfg_rtc) {
> > + regmap = sec_pmic_acpm_regmap_init(dev, shared_ctx, SEC_PMIC_ACPM_ACCESSTYPE_RTC,
> > + pdata->regmap_cfg_rtc, true);
> > + if (IS_ERR(regmap))
> > + return PTR_ERR(regmap);
> > + }
> >
> > regmap = sec_pmic_acpm_regmap_init(dev, shared_ctx, SEC_PMIC_ACPM_ACCESSTYPE_METER,
> > pdata->regmap_cfg_meter, true);
> > @@ -392,13 +562,28 @@ static int sec_pmic_acpm_probe(struct platform_device *pdev)
> > devm_device_init_wakeup(dev);
> >
> > /* Unmask PMIC interrupt from 'common' block, now that everything is in place. */
> > - ret = regmap_clear_bits(regmap_common, S2MPG10_COMMON_INT_MASK,
> > - S2MPG10_COMMON_INT_SRC_PMIC);
> > + switch (pdata->device_type) {
> > + case S2MPG10:
> > + ret = regmap_clear_bits(regmap_common, S2MPG10_COMMON_INT_MASK,
> > + S2MPG10_COMMON_INT_SRC_PMIC);
> > + masq_irqs_handler = sec_pmic_acpm_mask_common_s2mpg10_irqs;
> > + break;
> > +
> > + case S2MPG11:
> > + ret = regmap_clear_bits(regmap_common, S2MPG11_COMMON_INT_MASK,
> > + S2MPG11_COMMON_INT_SRC_PMIC);
> > + masq_irqs_handler = sec_pmic_acpm_mask_common_s2mpg11_irqs;
> > + break;
> > +
> > + default:
> > + return dev_err_probe(dev, -EINVAL, "Unsupported device type %d\n",
> > + pdata->device_type);
> > + }
> > if (ret)
> > return dev_err_probe(dev, ret, "failed to unmask PMIC interrupt\n");
> >
> > /* Mask all interrupts from 'common' block on shutdown */
> > - ret = devm_add_action_or_reset(dev, sec_pmic_acpm_mask_common_irqs, regmap_common);
> > + ret = devm_add_action_or_reset(dev, masq_irqs_handler, regmap_common);
> > if (ret)
> > return ret;
> >
> > @@ -420,8 +605,18 @@ static const struct sec_pmic_acpm_platform_data s2mpg10_data = {
> > .regmap_cfg_meter = &s2mpg10_regmap_config_meter,
> > };
> >
> > +static const struct sec_pmic_acpm_platform_data s2mpg11_data = {
> > + .device_type = S2MPG11,
> > + .acpm_chan_id = 2,
> > + .speedy_channel = 1,
> > + .regmap_cfg_common = &s2mpg11_regmap_config_common,
> > + .regmap_cfg_pmic = &s2mpg11_regmap_config_pmic,
> > + .regmap_cfg_meter = &s2mpg11_regmap_config_meter,
> > +};
> > +
> > static const struct of_device_id sec_pmic_acpm_of_match[] = {
> > { .compatible = "samsung,s2mpg10-pmic", .data = &s2mpg10_data, },
> > + { .compatible = "samsung,s2mpg11-pmic", .data = &s2mpg11_data, },
> > { },
> > };
> > MODULE_DEVICE_TABLE(of, sec_pmic_acpm_of_match);
> > diff --git a/drivers/mfd/sec-common.c b/drivers/mfd/sec-common.c
> > index b722481594801e545d24014af6afd5e1e39d7522..4daa0ece91dc783560dfad499f11193b689c2fd1 100644
> > --- a/drivers/mfd/sec-common.c
> > +++ b/drivers/mfd/sec-common.c
> > @@ -15,6 +15,7 @@
> > #include <linux/mfd/samsung/core.h>
> > #include <linux/mfd/samsung/irq.h>
> > #include <linux/mfd/samsung/s2mpg10.h>
> > +#include <linux/mfd/samsung/s2mpg11.h>
> > #include <linux/mfd/samsung/s2mps11.h>
> > #include <linux/mfd/samsung/s2mps13.h>
> > #include <linux/module.h>
> > @@ -82,6 +83,39 @@ static const struct mfd_cell s2mpg10_devs[] = {
> > MFD_CELL_OF("s2mpg10-gpio", NULL, NULL, 0, 0, "samsung,s2mpg10-gpio"),
> > };
> >
> > +static const struct mfd_cell s2mpg11_devs[] = {
> > + MFD_CELL_NAME("s2mpg11-meter"),
> > + MFD_CELL_BASIC("s2mpg11-regulator", NULL, NULL, 0, S2MPG11_BUCKBOOST),
>
> This is highly irregular - in that, we've never done this before.
>
> We're going to need to have Mark look at this.
I did see this in at least one other driver, ah yes at least
drivers/mfd/88pm860x-core.c is doing something similar, maybe others, too
(I stopped there).
Cheers,
Andre'
Powered by blists - more mailing lists