From: Lee Jones <lee@kernel.org>
To: Jerome Brunet <jbrunet@baylibre.com>
Cc: Andre Przywara <andre.przywara@arm.com>,
Rob Herring <robh@kernel.org>,
Krzysztof Kozlowski <krzk+dt@kernel.org>,
Conor Dooley <conor+dt@kernel.org>,
Chen-Yu Tsai <wens@kernel.org>,
Liam Girdwood <lgirdwood@gmail.com>,
Mark Brown <broonie@kernel.org>,
devicetree@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH v4 2/5] mfd: axp20x: Add support for AXP318W PMIC
Date: Thu, 6 Aug 2026 13:09:52 +0100 [thread overview]
Message-ID: <20260806120952.GA2869284@google.com> (raw)
In-Reply-To: <20260717-axp318-regulator-v4-2-50aff97084dd@baylibre.com>
/* Sashiko Automation: Issues Found (2 Findings) */
Please explain or fix these Sashiko reviews.
On Fri, 17 Jul 2026, Jerome Brunet wrote:
> From: Andre Przywara <andre.przywara@arm.com>
>
> The AXP318W is a PMIC chip produced by X-Powers, it can be connected to
> an I2C bus.
>
> It has a large number of regulators: 9(!) DCDC buck converters, and 28
> LDOs, also some ADCs, interrupts, and a power key.
>
> Describe the regmap and the MFD bits, along with the registers exposed
> via I2C only. This covers the regulators and interrupts for now.
> Advertise the device using the new compatible string.
>
> We use just "318" for the internal identifiers, for easier typing and
> less churn, but use "318W" for anything externally visible. If something
> else other than the "AXP318W" shows up, that's an easy change then.
>
> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> Signed-off-by: Jerome Brunet <jbrunet@baylibre.com>
> ---
> drivers/mfd/axp20x-i2c.c | 2 ++
> drivers/mfd/axp20x.c | 78 +++++++++++++++++++++++++++++++++++++++++
> include/linux/mfd/axp20x.h | 86 ++++++++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 166 insertions(+)
>
> diff --git a/drivers/mfd/axp20x-i2c.c b/drivers/mfd/axp20x-i2c.c
> index 5c93136f977e..4e4ebfc78525 100644
> --- a/drivers/mfd/axp20x-i2c.c
> +++ b/drivers/mfd/axp20x-i2c.c
> @@ -65,6 +65,7 @@ static const struct of_device_id axp20x_i2c_of_match[] = {
> { .compatible = "x-powers,axp221", .data = (void *)AXP221_ID },
> { .compatible = "x-powers,axp223", .data = (void *)AXP223_ID },
> { .compatible = "x-powers,axp313a", .data = (void *)AXP313A_ID },
> + { .compatible = "x-powers,axp318w", .data = (void *)AXP318_ID },
> { .compatible = "x-powers,axp323", .data = (void *)AXP323_ID },
> { .compatible = "x-powers,axp717", .data = (void *)AXP717_ID },
> { .compatible = "x-powers,axp803", .data = (void *)AXP803_ID },
> @@ -83,6 +84,7 @@ static const struct i2c_device_id axp20x_i2c_id[] = {
> { "axp221" },
> { "axp223" },
> { "axp313a" },
> + { "axp318w" },
> { "axp717" },
> { "axp803" },
> { "axp806" },
> diff --git a/drivers/mfd/axp20x.c b/drivers/mfd/axp20x.c
> index 679364189ea5..c8aeebd01bbc 100644
> --- a/drivers/mfd/axp20x.c
> +++ b/drivers/mfd/axp20x.c
> @@ -42,6 +42,7 @@ static const char * const axp20x_model_names[] = {
> [AXP223_ID] = "AXP223",
> [AXP288_ID] = "AXP288",
> [AXP313A_ID] = "AXP313a",
> + [AXP318_ID] = "AXP318W",
> [AXP323_ID] = "AXP323",
> [AXP717_ID] = "AXP717",
> [AXP803_ID] = "AXP803",
> @@ -218,6 +219,31 @@ static const struct regmap_access_table axp313a_volatile_table = {
> .n_yes_ranges = ARRAY_SIZE(axp313a_volatile_ranges),
> };
>
> +static const struct regmap_range axp318_writeable_ranges[] = {
> + regmap_reg_range(AXP318_DCDC_OUTPUT_CONTROL1, AXP318_IRQ_STATE4),
> + regmap_reg_range(AXP318_SHUTDOWN_CTRL, AXP318_TEMP_ADC_H_EN),
> + regmap_reg_range(AXP318_DIE_TEMP_ADC_H_EN, AXP318_DIE_TEMP_ADC_H_EN),
> + regmap_reg_range(AXP318_GPADC_H_EN, AXP318_GPADC_H_EN),
> + regmap_reg_range(AXP318_GPIO_CTRL, AXP318_WDOG_CTRL),
> +};
> +
> +static const struct regmap_range axp318_volatile_ranges[] = {
> + regmap_reg_range(AXP318_IRQ_EN1, AXP318_IRQ_STATE4),
> + regmap_reg_range(AXP318_POWER_REASON, AXP318_SHUTDOWN_REASON),
> + regmap_reg_range(AXP318_TEMP_ADC_H_EN, AXP318_GPADC_L),
> + regmap_reg_range(AXP318_GPIO_INPUT, AXP318_GPIO_INPUT),
> +};
> +
> +static const struct regmap_access_table axp318_writeable_table = {
> + .yes_ranges = axp318_writeable_ranges,
> + .n_yes_ranges = ARRAY_SIZE(axp318_writeable_ranges),
> +};
> +
> +static const struct regmap_access_table axp318_volatile_table = {
> + .yes_ranges = axp318_volatile_ranges,
> + .n_yes_ranges = ARRAY_SIZE(axp318_volatile_ranges),
> +};
> +
> static const struct regmap_range axp717_writeable_ranges[] = {
> regmap_reg_range(AXP717_PMU_FAULT, AXP717_MODULE_EN_CONTROL_1),
> regmap_reg_range(AXP717_MIN_SYS_V_CONTROL, AXP717_BOOST_CONTROL),
> @@ -447,6 +473,15 @@ static const struct regmap_config axp313a_regmap_config = {
> .cache_type = REGCACHE_MAPLE,
> };
>
> +static const struct regmap_config axp318_regmap_config = {
> + .reg_bits = 8,
> + .val_bits = 8,
> + .wr_table = &axp318_writeable_table,
> + .volatile_table = &axp318_volatile_table,
> + .max_register = AXP318_WDOG_CTRL,
> + .cache_type = REGCACHE_MAPLE,
> +};
> +
> static const struct regmap_config axp323_regmap_config = {
> .reg_bits = 8,
> .val_bits = 8,
> @@ -663,6 +698,28 @@ static const struct regmap_irq axp313a_regmap_irqs[] = {
> INIT_REGMAP_IRQ(AXP313A, DIE_TEMP_HIGH, 0, 0),
> };
>
> +static const struct regmap_irq axp318_regmap_irqs[] = {
> + INIT_REGMAP_IRQ(AXP318, DCDC8_V_LOW, 0, 7),
> + INIT_REGMAP_IRQ(AXP318, DCDC7_V_LOW, 0, 6),
> + INIT_REGMAP_IRQ(AXP318, DCDC6_V_LOW, 0, 5),
> + INIT_REGMAP_IRQ(AXP318, DCDC5_V_LOW, 0, 4),
> + INIT_REGMAP_IRQ(AXP318, DCDC4_V_LOW, 0, 3),
> + INIT_REGMAP_IRQ(AXP318, DCDC3_V_LOW, 0, 2),
> + INIT_REGMAP_IRQ(AXP318, DCDC2_V_LOW, 0, 1),
> + INIT_REGMAP_IRQ(AXP318, DCDC1_V_LOW, 0, 0),
> + INIT_REGMAP_IRQ(AXP318, PEK_RIS_EDGE, 1, 6),
> + INIT_REGMAP_IRQ(AXP318, PEK_FAL_EDGE, 1, 5),
> + INIT_REGMAP_IRQ(AXP318, PEK_LONG, 1, 4),
> + INIT_REGMAP_IRQ(AXP318, PEK_SHORT, 1, 3),
> + INIT_REGMAP_IRQ(AXP318, DIE_TEMP_HIGH_LV2, 1, 2),
> + INIT_REGMAP_IRQ(AXP318, DIE_TEMP_HIGH_LV1, 1, 1),
> + INIT_REGMAP_IRQ(AXP318, DCDC9_V_LOW, 1, 0),
> + INIT_REGMAP_IRQ(AXP318, GPIO3_INPUT, 2, 6),
> + INIT_REGMAP_IRQ(AXP318, GPIO2_INPUT, 2, 5),
> + INIT_REGMAP_IRQ(AXP318, GPIO1_INPUT, 2, 4),
> + INIT_REGMAP_IRQ(AXP318, WDOG_EXPIRE, 3, 0),
> +};
> +
> static const struct regmap_irq axp717_regmap_irqs[] = {
> INIT_REGMAP_IRQ(AXP717, SOC_DROP_LVL2, 0, 7),
> INIT_REGMAP_IRQ(AXP717, SOC_DROP_LVL1, 0, 6),
> @@ -884,6 +941,17 @@ static const struct regmap_irq_chip axp313a_regmap_irq_chip = {
> .num_regs = 1,
> };
>
> +static const struct regmap_irq_chip axp318_regmap_irq_chip = {
> + .name = "axp318w_irq_chip",
> + .status_base = AXP318_IRQ_STATE1,
> + .ack_base = AXP318_IRQ_STATE1,
> + .unmask_base = AXP318_IRQ_EN1,
> + .init_ack_masked = true,
> + .irqs = axp318_regmap_irqs,
> + .num_irqs = ARRAY_SIZE(axp318_regmap_irqs),
> + .num_regs = 4,
> +};
> +
> static const struct regmap_irq_chip axp717_regmap_irq_chip = {
> .name = "axp717_irq_chip",
> .status_base = AXP717_IRQ0_STATE,
> @@ -1061,6 +1129,10 @@ static struct mfd_cell axp313a_cells[] = {
> MFD_CELL_RES("axp313a-pek", axp313a_pek_resources),
> };
>
> +static const struct mfd_cell axp318_cells[] = {
> + MFD_CELL_BASIC("axp20x-regulator", NULL, NULL, 0, 1),
[Severity: Medium]
Does this hardcoded cell ID of 1 violate the MFD subsystem guidelines?
If multiple AXP devices with hardcoded IDs of 1 (such as the AXP313a and
AXP318W) are instantiated on the same system, could they both attempt to
register a platform device named axp20x-regulator.1, resulting in a fatal
-EEXIST failure?
Should this use PLATFORM_DEVID_AUTO instead?
> +};
> +
> static struct mfd_cell axp717_cells[] = {
> MFD_CELL_NAME("axp20x-regulator"),
> MFD_CELL_RES("axp20x-pek", axp717_pek_resources),
> @@ -1313,6 +1385,12 @@ int axp20x_match_device(struct axp20x_dev *axp20x)
> axp20x->regmap_cfg = &axp313a_regmap_config;
> axp20x->regmap_irq_chip = &axp313a_regmap_irq_chip;
> break;
> + case AXP318_ID:
> + axp20x->nr_cells = ARRAY_SIZE(axp318_cells);
> + axp20x->cells = axp318_cells;
> + axp20x->regmap_cfg = &axp318_regmap_config;
> + axp20x->regmap_irq_chip = &axp318_regmap_irq_chip;
> + break;
[Severity: High]
Is AXP318_ID missing from the switch statement in axp20x_power_off()?
When the system attempts to power off, axp20x_power_off() is called. If
AXP318_ID is not handled in the switch statement there, will it fall through
to the default case and select AXP20X_OFF_CTRL (0x32) instead of the correct
AXP318_SHUTDOWN_CTRL (0x52)?
Could this failure to shut down the PMIC corrupt the state of the CLDO4
regulator by writing the power-off command to AXP318_CLDO4_CONTROL (which
is also 0x32)?
> case AXP323_ID:
> axp20x->nr_cells = ARRAY_SIZE(axp313a_cells);
> axp20x->cells = axp313a_cells;
> diff --git a/include/linux/mfd/axp20x.h b/include/linux/mfd/axp20x.h
> index b352661d99a1..c1f9dc06387a 100644
> --- a/include/linux/mfd/axp20x.h
> +++ b/include/linux/mfd/axp20x.h
> @@ -19,6 +19,7 @@ enum axp20x_variants {
> AXP223_ID,
> AXP288_ID,
> AXP313A_ID,
> + AXP318_ID,
> AXP323_ID,
> AXP717_ID,
> AXP803_ID,
> @@ -116,6 +117,69 @@ enum axp20x_variants {
> #define AXP313A_IRQ_STATE 0x21
> #define AXP323_DCDC_MODE_CTRL2 0x22
>
> +#define AXP318_DCDC_OUTPUT_CONTROL1 0x10
> +#define AXP318_DCDC_OUTPUT_CONTROL2 0x11
> +#define AXP318_DCDC1_CONTROL 0x12
> +#define AXP318_DCDC2_CONTROL 0x13
> +#define AXP318_DCDC3_CONTROL 0x14
> +#define AXP318_DCDC4_CONTROL 0x15
> +#define AXP318_DCDC5_CONTROL 0x16
> +#define AXP318_DCDC6_CONTROL 0x17
> +#define AXP318_DCDC7_CONTROL 0x18
> +#define AXP318_DCDC8_CONTROL 0x19
> +#define AXP318_DCDC9_CONTROL 0x1a
> +#define AXP318_LDO_OUTPUT_CONTROL1 0x20
> +#define AXP318_LDO_OUTPUT_CONTROL2 0x21
> +#define AXP318_LDO_OUTPUT_CONTROL3 0x22
> +#define AXP318_LDO_OUTPUT_CONTROL4 0x23
> +#define AXP318_ALDO1_CONTROL 0x24
> +#define AXP318_ALDO2_CONTROL 0x25
> +#define AXP318_ALDO3_CONTROL 0x26
> +#define AXP318_ALDO4_CONTROL 0x27
> +#define AXP318_ALDO5_CONTROL 0x28
> +#define AXP318_ALDO6_CONTROL 0x29
> +#define AXP318_BLDO1_CONTROL 0x2a
> +#define AXP318_BLDO2_CONTROL 0x2b
> +#define AXP318_BLDO3_CONTROL 0x2c
> +#define AXP318_BLDO4_CONTROL 0x2d
> +#define AXP318_BLDO5_CONTROL 0x2e
> +#define AXP318_CLDO1_CONTROL 0x2f
> +#define AXP318_CLDO2_CONTROL 0x30
> +#define AXP318_CLDO3_CONTROL 0x31
> +#define AXP318_CLDO4_CONTROL 0x32
> +#define AXP318_CLDO5_CONTROL 0x33
> +#define AXP318_DLDO1_CONTROL 0x34
> +#define AXP318_DLDO2_CONTROL 0x35
> +#define AXP318_DLDO3_CONTROL 0x36
> +#define AXP318_DLDO4_CONTROL 0x37
> +#define AXP318_DLDO5_CONTROL 0x38
> +#define AXP318_DLDO6_CONTROL 0x39
> +#define AXP318_ELDO1_CONTROL 0x3a
> +#define AXP318_ELDO2_CONTROL 0x3b
> +#define AXP318_ELDO3_CONTROL 0x3c
> +#define AXP318_ELDO4_CONTROL 0x3d
> +#define AXP318_ELDO5_CONTROL 0x3e
> +#define AXP318_ELDO6_CONTROL 0x3f
> +#define AXP318_IRQ_EN1 0x40
> +#define AXP318_IRQ_EN2 0x41
> +#define AXP318_IRQ_EN3 0x42
> +#define AXP318_IRQ_EN4 0x43
> +#define AXP318_IRQ_STATE1 0x48
> +#define AXP318_IRQ_STATE2 0x49
> +#define AXP318_IRQ_STATE3 0x4a
> +#define AXP318_IRQ_STATE4 0x4b
> +#define AXP318_POWER_REASON 0x50
> +#define AXP318_SHUTDOWN_REASON 0x51
> +#define AXP318_SHUTDOWN_CTRL 0x52
> +#define AXP318_TEMP_ADC_H_EN 0x65
> +#define AXP318_TEMP_ADC_L 0x66
> +#define AXP318_DIE_TEMP_ADC_H_EN 0x67
> +#define AXP318_GPADC_H_EN 0x69
> +#define AXP318_GPADC_L 0x6a
> +#define AXP318_GPIO_CTRL 0x70
> +#define AXP318_GPIO_INPUT 0x71
> +#define AXP318_WDOG_CTRL 0x77
> +
> #define AXP717_ON_INDICATE 0x00
> #define AXP717_PMU_STATUS_2 0x01
> #define AXP717_BC_DETECT 0x05
> @@ -819,6 +883,28 @@ enum axp313a_irqs {
> AXP313A_IRQ_PEK_RIS_EDGE,
> };
>
> +enum axp318_irqs {
> + AXP318_IRQ_DCDC1_V_LOW,
> + AXP318_IRQ_DCDC2_V_LOW,
> + AXP318_IRQ_DCDC3_V_LOW,
> + AXP318_IRQ_DCDC4_V_LOW,
> + AXP318_IRQ_DCDC5_V_LOW,
> + AXP318_IRQ_DCDC6_V_LOW,
> + AXP318_IRQ_DCDC7_V_LOW,
> + AXP318_IRQ_DCDC8_V_LOW,
> + AXP318_IRQ_DCDC9_V_LOW,
> + AXP318_IRQ_DIE_TEMP_HIGH_LV1,
> + AXP318_IRQ_DIE_TEMP_HIGH_LV2,
> + AXP318_IRQ_PEK_SHORT,
> + AXP318_IRQ_PEK_LONG,
> + AXP318_IRQ_PEK_FAL_EDGE,
> + AXP318_IRQ_PEK_RIS_EDGE,
> + AXP318_IRQ_GPIO1_INPUT = 20,
> + AXP318_IRQ_GPIO2_INPUT,
> + AXP318_IRQ_GPIO3_INPUT,
> + AXP318_IRQ_WDOG_EXPIRE = 24,
> +};
> +
> enum axp717_irqs {
> AXP717_IRQ_VBUS_FAULT,
> AXP717_IRQ_VBUS_OVER_V,
>
> --
> 2.47.3
>
--
Lee Jones
next prev parent reply other threads:[~2026-08-06 12:09 UTC|newest]
Thread overview: 10+ messages / expand[flat|nested] mbox.gz Atom feed top
2026-07-17 14:31 [PATCH v4 0/5] regulator: Add X-Powers AXP318W PMIC support Jerome Brunet
2026-07-17 14:31 ` [PATCH v4 1/5] dt-bindings: mfd: x-powers,axp152: Document AXP318W Jerome Brunet
2026-07-17 14:31 ` [PATCH v4 2/5] mfd: axp20x: Add support for AXP318W PMIC Jerome Brunet
2026-08-06 12:09 ` Lee Jones [this message]
2026-08-06 12:24 ` Lee Jones
2026-08-06 13:36 ` Jerome Brunet
2026-08-06 13:48 ` Jerome Brunet
2026-07-17 14:31 ` [PATCH v4 3/5] mfd: axp20x: Constify axp313 and axp717 cells Jerome Brunet
2026-07-17 14:31 ` [PATCH v4 4/5] mfd: axp20x-i2c: Fix i2c instantiation Jerome Brunet
2026-07-17 14:31 ` [PATCH v4 5/5] regulator: axp20x: add support for the AXP318W Jerome Brunet
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20260806120952.GA2869284@google.com \
--to=lee@kernel.org \
--cc=andre.przywara@arm.com \
--cc=broonie@kernel.org \
--cc=conor+dt@kernel.org \
--cc=devicetree@vger.kernel.org \
--cc=jbrunet@baylibre.com \
--cc=krzk+dt@kernel.org \
--cc=lgirdwood@gmail.com \
--cc=linux-kernel@vger.kernel.org \
--cc=robh@kernel.org \
--cc=wens@kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox