Re: [PATCH v3 09/20] mfd: sec: Add support for S2MPG11 PMIC via ACPM

["André Draszik" <andre.draszik@linaro.org>]

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@linaro.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'