Message-ID: <0a1f45f3221f74fdde0f388b3693e51c771bb307.camel@linaro.org>
Date: Mon, 07 Apr 2025 09:55:45 +0100
From: André Draszik <andre.draszik@...aro.org>
To: Lee Jones <lee@...nel.org>
Cc: Krzysztof Kozlowski <krzk@...nel.org>, Rob Herring <robh@...nel.org>, 
 Conor Dooley <conor+dt@...nel.org>, Sylwester Nawrocki
 <s.nawrocki@...sung.com>, Chanwoo Choi	 <cw00.choi@...sung.com>, Alim
 Akhtar <alim.akhtar@...sung.com>, Michael Turquette
 <mturquette@...libre.com>, Stephen Boyd <sboyd@...nel.org>, Russell King	
 <linux@...linux.org.uk>, Catalin Marinas <catalin.marinas@....com>, Will
 Deacon	 <will@...nel.org>, Alexandre Belloni
 <alexandre.belloni@...tlin.com>, Peter Griffin <peter.griffin@...aro.org>,
 Tudor Ambarus <tudor.ambarus@...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-clk@...r.kernel.org, 
	linux-arm-kernel@...ts.infradead.org, linux-rtc@...r.kernel.org
Subject: Re: [PATCH v3 09/32] mfd: sec: add support for S2MPG10 PMIC

Hi,

Thanks Lee for your review!

On Fri, 2025-04-04 at 10:18 +0100, Lee Jones wrote:
> On Thu, 03 Apr 2025, André Draszik wrote:

[...]

> > diff --git a/drivers/mfd/sec-acpm.c b/drivers/mfd/sec-acpm.c
> > new file mode 100644
> > index 0000000000000000000000000000000000000000..39dbb968086ac835b96ed3e4efa68868fda63429
> > --- /dev/null
> > +++ b/drivers/mfd/sec-acpm.c
> > @@ -0,0 +1,465 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +/*
> > + * Copyright 2020 Google Inc
> > + * Copyright 2025 Linaro Ltd.
> > + *
> > + * Samsung S2MPG1x ACPM driver
> > + */
> > +
> > +#include <linux/array_size.h>
> > +#include <linux/device.h>
> > +#include <linux/firmware/samsung/exynos-acpm-protocol.h>
> > +#include <linux/mfd/samsung/core.h>
> > +#include <linux/mfd/samsung/rtc.h>
> > +#include <linux/mfd/samsung/s2mpg10.h>
> > +#include <linux/mod_devicetable.h>
> > +#include <linux/module.h>
> > +#include <linux/of.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/pm.h>
> > +#include <linux/property.h>
> > +#include <linux/regmap.h>
> > +#include "sec-core.h"
> > +
> > +#define ACPM_MAX_BULK_DATA   8
> > +
> > +struct sec_pmic_acpm_platform_data {
> 
> This isn't platform data.  It's driver data.
> 
> Platform data is passed in, driver data is derived.

This is the match data from of_device_id::data, it is passed in
via device_get_match_data().

> 
> See how the other drivers do this:
> 
>   $ git grep ddata -- drivers/mfd

I had followed the example of drivers/mfd/rk8xx-i2c.c

I can rename it to struct sec_pmic_acpm_chip_data if you prefer or
something like that, but the rk8xx driver also calls this platform
data.

ddata in drivers/mfd/ generally seems used for dynamically allocated
runtime driver data. That's not the case here.

> > +	int device_type;
> > +
> > +	unsigned int acpm_chan_id;
> > +	u8 speedy_channel;
> > +
> > +	const struct regmap_config *regmap_cfg_common;
> > +	const struct regmap_config *regmap_cfg_pmic;
> > +	const struct regmap_config *regmap_cfg_rtc;
> > +	const struct regmap_config *regmap_cfg_meter;
> > +};
> > +
> > +static const struct regmap_range s2mpg10_common_registers[] = {
> > +	regmap_reg_range(0x00, 0x02), /* CHIP_ID_M, INT, INT_MASK */
> > +	regmap_reg_range(0x0a, 0x0c), /* speedy control */
> > +	regmap_reg_range(0x1a, 0x2a), /* debug */
> 
> Nit: I like comments to start with an upper-case char.

OK

> 
> > +};
> > +
> > +static const struct regmap_range s2mpg10_common_ro_registers[] = {
> > +	regmap_reg_range(0x00, 0x01),
> > +	regmap_reg_range(0x28, 0x2a),
> 
> Why describe some, but not all ranges?

They're all covered above. I'll duplicate them here and elsewhere.

> > +struct sec_pmic_acpm_shared_bus_context {
> > +	const struct acpm_handle *acpm;
> > +	unsigned int acpm_chan_id;
> > +	u8 speedy_channel;
> > +};
> > +
> > +enum sec_pmic_acpm_accesstype {
> > +	SEC_PMIC_ACPM_ACCESSTYPE_COMMON = 0x00,
> > +	SEC_PMIC_ACPM_ACCESSTYPE_PMIC = 0x01,
> > +	SEC_PMIC_ACPM_ACCESSTYPE_RTC = 0x02,
> > +	SEC_PMIC_ACPM_ACCESSTYPE_METER = 0x0a,
> > +	SEC_PMIC_ACPM_ACCESSTYPE_WLWP = 0x0b,
> > +	SEC_PMIC_ACPM_ACCESSTYPE_TRIM = 0x0f,
> > +};
> > +
> > +struct sec_pmic_acpm_bus_context {
> > +	struct sec_pmic_acpm_shared_bus_context *shared;
> > +	enum sec_pmic_acpm_accesstype type;
> > +};
> > +
> > +static int sec_pmic_acpm_bus_write(void *context, const void *data,
> > +				   size_t count)
> 
> Nit: You can tidy this, and similar line-feeds, up by using 100-chars here.

Will do.

> > +{
> > +	struct sec_pmic_acpm_bus_context *ctx = context;
> > +	const struct acpm_handle *acpm = ctx->shared->acpm;
> > +	const struct acpm_pmic_ops *pmic_ops = &acpm->ops.pmic_ops;
> > +	u8 reg;
> > +
> > +	if (count < 2 || count > (ACPM_MAX_BULK_DATA + 1))
> 
> 2 because?  Either comment or define magic numbers please.
> 
> > +		return -EINVAL;
> > +
> > +	reg = *(u8 *)data;
> 
> No API to conduct this raw read for you?  readl(), *_to_cpu() or similar?

This is just regmap, passing a buffer. First byte(s) contains the reg
address, depending on the regmap_config used during creation, and remainder
the values starting from that address. This is not an I/O read as such, it's
only extracting the register address. See e.g. regmap_parse_8().

I'll reflow it a little.

> 
> > +	++data;
> > +	--count;
> > +
> > +	return pmic_ops->bulk_write(acpm, ctx->shared->acpm_chan_id,
> > +				    ctx->type, reg,
> > +				    ctx->shared->speedy_channel, count, data);
> > +}
> > +
> > +static int sec_pmic_acpm_bus_read(void *context, const void *reg_buf,
> > +				  size_t reg_size, void *val_buf,
> > +				  size_t val_size)
> > +{
> > +	struct sec_pmic_acpm_bus_context *ctx = context;
> > +	const struct acpm_handle *acpm = ctx->shared->acpm;
> > +	const struct acpm_pmic_ops *pmic_ops = &acpm->ops.pmic_ops;
> > +	u8 reg;
> > +
> > +	if (reg_size != 1 || !val_size || val_size > ACPM_MAX_BULK_DATA)
> > +		return -EINVAL;
> > +
> > +	reg = *(u8 *)reg_buf;
> > +
> > +	return pmic_ops->bulk_read(acpm, ctx->shared->acpm_chan_id,
> > +				   ctx->type, reg,
> > +				   ctx->shared->speedy_channel,
> > +				   val_size, val_buf);
> > +}
> > +
> > +static int sec_pmic_acpm_bus_reg_update_bits(void *context, unsigned int reg,
> > +					     unsigned int mask,
> > +					     unsigned int val)
> > +{
> > +	struct sec_pmic_acpm_bus_context *ctx = context;
> > +	const struct acpm_handle *acpm = ctx->shared->acpm;
> > +	const struct acpm_pmic_ops *pmic_ops = &acpm->ops.pmic_ops;
> > +
> > +	return pmic_ops->update_reg(acpm, ctx->shared->acpm_chan_id,
> > +				    ctx->type, reg & 0xff,
> > +				    ctx->shared->speedy_channel, val, mask);
> > +}
> > +
> > +static const struct regmap_bus sec_pmic_acpm_regmap_bus = {
> > +	.write = sec_pmic_acpm_bus_write,
> > +	.read = sec_pmic_acpm_bus_read,
> > +	.reg_update_bits = sec_pmic_acpm_bus_reg_update_bits,
> > +	.max_raw_read = ACPM_MAX_BULK_DATA,
> > +	.max_raw_write = ACPM_MAX_BULK_DATA,
> > +};
> > +
> > +static struct regmap *
> > +sec_pmic_acpm_regmap_init(struct device *dev,
> 
> Place both of these on the same line please.
> 
> > +			  struct sec_pmic_acpm_shared_bus_context *shared_ctx,
> > +			  enum sec_pmic_acpm_accesstype type,
> > +			  const struct regmap_config *cfg, bool do_attach)
> > +{
> > +	struct sec_pmic_acpm_bus_context *ctx;
> > +	struct regmap *regmap;
> > +
> > +	ctx = devm_kzalloc(dev, sizeof(*ctx), GFP_KERNEL);
> > +	if (!ctx)
> > +		return ERR_PTR(-ENOMEM);
> > +
> > +	ctx->shared = shared_ctx;
> > +	ctx->type = type;
> > +
> > +	regmap = devm_regmap_init(dev, &sec_pmic_acpm_regmap_bus, ctx, cfg);
> > +	if (IS_ERR(regmap))
> > +		return ERR_PTR(dev_err_probe(dev, PTR_ERR(regmap),
> 
> dev_err_cast_probe()
> 
> > +					     "regmap init (%s) failed\n",
> > +					     cfg->name));
> > +
> > +	if (do_attach) {
> > +		int ret;
> > +
> > +		ret = regmap_attach_dev(dev, regmap, cfg);
> > +		if (ret)
> > +			return ERR_PTR(dev_err_probe(dev, ret,
> 
> dev_err_ptr_probe()

Thanks! I had forgotten about those two.

> > +						     "regmap attach (%s) failed\n",
> > +						     cfg->name));
> > +	}
> > +
> > +	return regmap;
> > +}
> > +
> > +static void sec_pmic_acpm_mask_common_irqs(void *regmap_common)
> > +{
> > +	regmap_write(regmap_common, S2MPG10_COMMON_INT_MASK,
> > +		     S2MPG10_COMMON_INT_SRC);
> 
> Single line.  And others like it.
> 
> > +}
> > +
> > +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;
> > +	const struct acpm_handle *acpm;
> > +	struct device *dev;
> > +	int ret, irq;
> > +
> > +	dev = &pdev->dev;
> 
> You can do this during the declaration.
> 
> > +	pdata = device_get_match_data(dev);
> > +	if (!pdata)
> > +		return dev_err_probe(dev, -ENODEV,
> > +				     "unsupported device type\n");
> > +
> > +	acpm = devm_acpm_get_by_node(dev, pdev->dev.parent->of_node);
> 
> You have 'dev' now.  Please use it.
> 
> > +	if (IS_ERR(acpm))
> > +		return dev_err_probe(dev, PTR_ERR(acpm),
> > +				     "failed to get acpm (2)\n");
> > +
> > +	irq = platform_get_irq(pdev, 0);
> > +	if (irq < 0)
> > +		return irq;
> > +
> > +	shared_ctx = devm_kzalloc(dev, sizeof(*shared_ctx), GFP_KERNEL);
> > +	if (!shared_ctx)
> > +		return -ENOMEM;
> > +
> > +	shared_ctx->acpm = acpm;
> > +	shared_ctx->acpm_chan_id = pdata->acpm_chan_id;
> > +	shared_ctx->speedy_channel = pdata->speedy_channel;
> > +
> > +	regmap_common = sec_pmic_acpm_regmap_init(dev, shared_ctx,
> > +						  SEC_PMIC_ACPM_ACCESSTYPE_COMMON,
> > +						  pdata->regmap_cfg_common, false);
> > +	if (IS_ERR(regmap_common))
> > +		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);
> > +	if (ret)
> > +		return dev_err_probe(dev, ret,
> > +				     "failed to mask common block interrupts\n");
> > +
> > +	regmap_pmic = sec_pmic_acpm_regmap_init(dev, shared_ctx,
> > +						SEC_PMIC_ACPM_ACCESSTYPE_PMIC,
> > +						pdata->regmap_cfg_pmic, false);
> > +	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);
> > +
> > +	regmap = sec_pmic_acpm_regmap_init(dev, shared_ctx,
> > +					   SEC_PMIC_ACPM_ACCESSTYPE_METER,
> > +					   pdata->regmap_cfg_meter, true);
> > +	if (IS_ERR(regmap))
> > +		return PTR_ERR(regmap);
> > +
> > +	ret = sec_pmic_probe(dev, pdata->device_type, irq, regmap_pmic, NULL);
> > +	if (ret)
> > +		return ret;
> > +
> > +	if (device_property_read_bool(dev, "wakeup-source"))
> > +		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);
> > +	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);
> > +	if (ret)
> > +		return ret;
> > +
> > +	return 0;
> > +}
> > +
> > +static void sec_pmic_acpm_shutdown(struct platform_device *pdev)
> > +{
> > +	sec_pmic_shutdown(&pdev->dev);
> 
> sec_pmic_shutdown() takes a pointer to i2c_client (unless you changed it
> somewhere else).  If the later is true, then why not make it take a
> pointer to platform_device and omit this abstraction?

I changed it earlier indeed to support both I2C and ACPM transports, similar
to drivers/mfd/rk*. The I2C driver doesn't have a struct platform_device,
but it has struct i2c_client::dev, hence I'm passing struct device to the
common code, like in the rk8xx example.

[...]

> 
> > diff --git a/drivers/mfd/sec-irq.c b/drivers/mfd/sec-irq.c
> > index 4d49bb42bd0d109263f485c8b58e88cdd8d598d9..bf86281401ac6ff05c90c2d71c84744709ed79cb 100644
> > --- a/drivers/mfd/sec-irq.c
> > +++ b/drivers/mfd/sec-irq.c
> > @@ -11,6 +11,7 @@
> >  #include <linux/irq.h>
> >  #include <linux/mfd/samsung/core.h>
> >  #include <linux/mfd/samsung/irq.h>
> > +#include <linux/mfd/samsung/s2mpg10.h>
> >  #include <linux/mfd/samsung/s2mps11.h>
> >  #include <linux/mfd/samsung/s2mps14.h>
> >  #include <linux/mfd/samsung/s2mpu02.h>
> > @@ -20,6 +21,60 @@
> >  #include <linux/regmap.h>
> >  #include "sec-core.h"
> >  
> > +static const struct regmap_irq s2mpg10_irqs[] = {
> > +	REGMAP_IRQ_REG(S2MPG10_IRQ_PWRONF, 0, S2MPG10_IRQ_PWRONF_MASK),
> > +	REGMAP_IRQ_REG(S2MPG10_IRQ_PWRONR, 0, S2MPG10_IRQ_PWRONR_MASK),
> > +	REGMAP_IRQ_REG(S2MPG10_IRQ_JIGONBF, 0, S2MPG10_IRQ_JIGONBF_MASK),
> > +	REGMAP_IRQ_REG(S2MPG10_IRQ_JIGONBR, 0, S2MPG10_IRQ_JIGONBR_MASK),
> > +	REGMAP_IRQ_REG(S2MPG10_IRQ_ACOKBF, 0, S2MPG10_IRQ_ACOKBF_MASK),
> > +	REGMAP_IRQ_REG(S2MPG10_IRQ_ACOKBR, 0, S2MPG10_IRQ_ACOKBR_MASK),
> > +	REGMAP_IRQ_REG(S2MPG10_IRQ_PWRON1S, 0, S2MPG10_IRQ_PWRON1S_MASK),
> > +	REGMAP_IRQ_REG(S2MPG10_IRQ_MRB, 0, S2MPG10_IRQ_MRB_MASK),
> > +
> > +	REGMAP_IRQ_REG(S2MPG10_IRQ_RTC60S, 1, S2MPG10_IRQ_RTC60S_MASK),
> > +	REGMAP_IRQ_REG(S2MPG10_IRQ_RTCA1, 1, S2MPG10_IRQ_RTCA1_MASK),
> > +	REGMAP_IRQ_REG(S2MPG10_IRQ_RTCA0, 1, S2MPG10_IRQ_RTCA0_MASK),
> > +	REGMAP_IRQ_REG(S2MPG10_IRQ_RTC1S, 1, S2MPG10_IRQ_RTC1S_MASK),
> > +	REGMAP_IRQ_REG(S2MPG10_IRQ_WTSR_COLDRST, 1, S2MPG10_IRQ_WTSR_COLDRST_MASK),
> > +	REGMAP_IRQ_REG(S2MPG10_IRQ_WTSR, 1, S2MPG10_IRQ_WTSR_MASK),
> > +	REGMAP_IRQ_REG(S2MPG10_IRQ_WRST, 1, S2MPG10_IRQ_WRST_MASK),
> > +	REGMAP_IRQ_REG(S2MPG10_IRQ_SMPL, 1, S2MPG10_IRQ_SMPL_MASK),
> > +
> > +	REGMAP_IRQ_REG(S2MPG10_IRQ_120C, 2, S2MPG10_IRQ_INT120C_MASK),
> > +	REGMAP_IRQ_REG(S2MPG10_IRQ_140C, 2, S2MPG10_IRQ_INT140C_MASK),
> > +	REGMAP_IRQ_REG(S2MPG10_IRQ_TSD, 2, S2MPG10_IRQ_TSD_MASK),
> > +	REGMAP_IRQ_REG(S2MPG10_IRQ_PIF_TIMEOUT1, 2, S2MPG10_IRQ_PIF_TIMEOUT1_MASK),
> > +	REGMAP_IRQ_REG(S2MPG10_IRQ_PIF_TIMEOUT2, 2, S2MPG10_IRQ_PIF_TIMEOUT2_MASK),
> > +	REGMAP_IRQ_REG(S2MPG10_IRQ_SPD_PARITY_ERR, 2, S2MPG10_IRQ_SPD_PARITY_ERR_MASK),
> > +	REGMAP_IRQ_REG(S2MPG10_IRQ_SPD_ABNORMAL_STOP, 2, S2MPG10_IRQ_SPD_ABNORMAL_STOP_MASK),
> > +	REGMAP_IRQ_REG(S2MPG10_IRQ_PMETER_OVERF, 2, S2MPG10_IRQ_PMETER_OVERF_MASK),
> > +
> > +	REGMAP_IRQ_REG(S2MPG10_IRQ_OCP_B1M, 3, S2MPG10_IRQ_OCP_B1M_MASK),
> > +	REGMAP_IRQ_REG(S2MPG10_IRQ_OCP_B2M, 3, S2MPG10_IRQ_OCP_B2M_MASK),
> > +	REGMAP_IRQ_REG(S2MPG10_IRQ_OCP_B3M, 3, S2MPG10_IRQ_OCP_B3M_MASK),
> > +	REGMAP_IRQ_REG(S2MPG10_IRQ_OCP_B4M, 3, S2MPG10_IRQ_OCP_B4M_MASK),
> > +	REGMAP_IRQ_REG(S2MPG10_IRQ_OCP_B5M, 3, S2MPG10_IRQ_OCP_B5M_MASK),
> > +	REGMAP_IRQ_REG(S2MPG10_IRQ_OCP_B6M, 3, S2MPG10_IRQ_OCP_B6M_MASK),
> > +	REGMAP_IRQ_REG(S2MPG10_IRQ_OCP_B7M, 3, S2MPG10_IRQ_OCP_B7M_MASK),
> > +	REGMAP_IRQ_REG(S2MPG10_IRQ_OCP_B8M, 3, S2MPG10_IRQ_OCP_B8M_MASK),
> > +
> > +	REGMAP_IRQ_REG(S2MPG10_IRQ_OCP_B9M, 4, S2MPG10_IRQ_OCP_B9M_MASK),
> > +	REGMAP_IRQ_REG(S2MPG10_IRQ_OCP_B10M, 4, S2MPG10_IRQ_OCP_B10M_MASK),
> > +	REGMAP_IRQ_REG(S2MPG10_IRQ_WLWP_ACC, 4, S2MPG10_IRQ_WLWP_ACC_MASK),
> > +	REGMAP_IRQ_REG(S2MPG10_IRQ_SMPL_TIMEOUT, 4, S2MPG10_IRQ_SMPL_TIMEOUT_MASK),
> > +	REGMAP_IRQ_REG(S2MPG10_IRQ_WTSR_TIMEOUT, 4, S2MPG10_IRQ_WTSR_TIMEOUT_MASK),
> > +	REGMAP_IRQ_REG(S2MPG10_IRQ_SPD_SRP_PKT_RST, 4, S2MPG10_IRQ_SPD_SRP_PKT_RST_MASK),
> > +
> > +	REGMAP_IRQ_REG(S2MPG10_IRQ_PWR_WARN_CH0, 5, S2MPG10_IRQ_PWR_WARN_CH0_MASK),
> > +	REGMAP_IRQ_REG(S2MPG10_IRQ_PWR_WARN_CH1, 5, S2MPG10_IRQ_PWR_WARN_CH1_MASK),
> > +	REGMAP_IRQ_REG(S2MPG10_IRQ_PWR_WARN_CH2, 5, S2MPG10_IRQ_PWR_WARN_CH2_MASK),
> > +	REGMAP_IRQ_REG(S2MPG10_IRQ_PWR_WARN_CH3, 5, S2MPG10_IRQ_PWR_WARN_CH3_MASK),
> > +	REGMAP_IRQ_REG(S2MPG10_IRQ_PWR_WARN_CH4, 5, S2MPG10_IRQ_PWR_WARN_CH4_MASK),
> > +	REGMAP_IRQ_REG(S2MPG10_IRQ_PWR_WARN_CH5, 5, S2MPG10_IRQ_PWR_WARN_CH5_MASK),
> > +	REGMAP_IRQ_REG(S2MPG10_IRQ_PWR_WARN_CH6, 5, S2MPG10_IRQ_PWR_WARN_CH6_MASK),
> > +	REGMAP_IRQ_REG(S2MPG10_IRQ_PWR_WARN_CH7, 5, S2MPG10_IRQ_PWR_WARN_CH7_MASK),
> > +};
> > +
> >  static const struct regmap_irq s2mps11_irqs[] = {
> >  	[S2MPS11_IRQ_PWRONF] = {
> >  		.reg_offset = 0,
> > @@ -320,6 +375,16 @@ static const struct regmap_irq s5m8767_irqs[] = {
> >  	},
> >  };
> >  
> > +static const struct regmap_irq_chip s2mpg10_irq_chip = {
> > +	.name = "s2mpg10",
> > +	.irqs = s2mpg10_irqs,
> > +	.num_irqs = ARRAY_SIZE(s2mpg10_irqs),
> > +	.num_regs = 6,
> > +	.status_base = S2MPG10_PMIC_INT1,
> > +	.mask_base = S2MPG10_PMIC_INT1M,
> > +	/* all interrupt sources are read-to-clear */
> 
> TOUPPER(a);
> 
> Comments usually go on-top of the thing they're commenting on.

This comment is where .ack_base would usually be specified, but I'll move it.

[...]

> > diff --git a/include/linux/mfd/samsung/s2mpg10.h b/include/linux/mfd/samsung/s2mpg10.h
> > new file mode 100644
> > index 0000000000000000000000000000000000000000..778ff16ef6668ded514e8dc7242f369cb9c2d0e6
> > --- /dev/null
> > +++ b/include/linux/mfd/samsung/s2mpg10.h
> > @@ -0,0 +1,454 @@
> > +/* SPDX-License-Identifier: GPL-2.0+ */
> > +/*
> > + * Copyright 2015 Samsung Electronics
> > + * Copyright 2020 Google Inc
> > + * Copyright 2025 Linaro Ltd.
> > + */
> > +
> > +#ifndef __LINUX_MFD_S2MPG10_H
> > +#define __LINUX_MFD_S2MPG10_H
> > +
> > +/* Common registers (type 0x000) */
> > +enum s2mpg10_common_reg {
> > +	S2MPG10_COMMON_CHIPID,
> > +	S2MPG10_COMMON_INT,
> > +	S2MPG10_COMMON_INT_MASK,
> > +	S2MPG10_COMMON_SPD_CTRL1 = 0x0a,
> > +	S2MPG10_COMMON_SPD_CTRL2,
> > +	S2MPG10_COMMON_SPD_CTRL3,
> > +	S2MPG10_COMMON_MON1SEL = 0x1a,
> > +	S2MPG10_COMMON_MON2SEL,
> > +	S2MPG10_COMMON_MONR,
> > +	S2MPG10_COMMON_DEBUG_CTRL1,
> > +	S2MPG10_COMMON_DEBUG_CTRL2,
> > +	S2MPG10_COMMON_DEBUG_CTRL3,
> > +	S2MPG10_COMMON_DEBUG_CTRL4,
> > +	S2MPG10_COMMON_DEBUG_CTRL5,
> > +	S2MPG10_COMMON_DEBUG_CTRL6,
> > +	S2MPG10_COMMON_DEBUG_CTRL7,
> > +	S2MPG10_COMMON_DEBUG_CTRL8,
> > +	S2MPG10_COMMON_TEST_MODE1,
> > +	S2MPG10_COMMON_TEST_MODE2,
> > +	S2MPG10_COMMON_SPD_DEBUG1,
> > +	S2MPG10_COMMON_SPD_DEBUG2,
> > +	S2MPG10_COMMON_SPD_DEBUG3,
> > +	S2MPG10_COMMON_SPD_DEBUG4,
> > +};
> > +
> > +/* for S2MPG10_COMMON_INT and S2MPG10_COMMON_INT_MASK */
> 
> TOUPPER(f), etc.

Still getting used to this, sorry I missed them

Thanks Lee!

Andre'

Powered by blists - more mailing lists