* [PATCH 1/3] dt-bindings: usb: tps6598x: Add sn201202x/ACE3
2026-07-25 16:20 [PATCH 0/3] usb: typec: tipd: Add sn201202x (ACE3) support Sasha Finkelstein
@ 2026-07-25 16:20 ` Sasha Finkelstein
2026-07-25 16:30 ` sashiko-bot
2026-07-25 16:20 ` [PATCH 2/3] usb: typec: tipd: Factor out i2c specifics Sasha Finkelstein
2026-07-25 16:20 ` [PATCH 3/3] usb: typec: tipd: Add sn201202x support Sasha Finkelstein
2 siblings, 1 reply; 7+ messages in thread
From: Sasha Finkelstein @ 2026-07-25 16:20 UTC (permalink / raw)
To: Sven Peter, Janne Grunau, Neal Gompa, Greg Kroah-Hartman,
Rob Herring, Krzysztof Kozlowski, Conor Dooley, Heikki Krogerus
Cc: asahi, linux-arm-kernel, linux-usb, devicetree, linux-kernel,
Sasha Finkelstein
A variant of tps6598x that is attached to a SPMI bus and is found on
Apple devices starting from the M3 series
Signed-off-by: Sasha Finkelstein <k@chaosmail.tech>
---
Documentation/devicetree/bindings/usb/apple,sn201202x.yaml | 78 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
MAINTAINERS | 1 +
2 files changed, 79 insertions(+)
diff --git a/Documentation/devicetree/bindings/usb/apple,sn201202x.yaml b/Documentation/devicetree/bindings/usb/apple,sn201202x.yaml
new file mode 100644
index 000000000000..84d230ccccc7
--- /dev/null
+++ b/Documentation/devicetree/bindings/usb/apple,sn201202x.yaml
@@ -0,0 +1,78 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/usb/apple,sn201202x.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Texas Instruments sn201202x Type-C Power Delivery controller
+
+maintainers:
+ - Sasha Finkelstein <k@chaosmail.tech>
+
+description: |
+ Texas Instruments sn201202x Type-C Power Delivery controller
+
+ A variant of tps6598x controller also known as Apple ACE3
+ present on hardware with Apple SoCs starting from the M3.
+
+properties:
+ compatible:
+ enum:
+ - apple,sn201202x
+
+ reg:
+ minItems: 1
+
+ interrupts:
+ items:
+ - description: Primary irq used for tps6598x events
+ - description: Logical register selection completed
+ - description: Standby command completed
+ - description: Wakeup command completed
+
+ interrupt-names:
+ items:
+ - const: irq
+ - const: select
+ - const: sleep
+ - const: wake
+
+ connector:
+ $ref: /schemas/connector/usb-connector.yaml#
+
+required:
+ - compatible
+ - reg
+ - interrupts
+ - interrupt-names
+
+additionalProperties: false
+
+examples:
+ - |
+ #include <dt-bindings/interrupt-controller/irq.h>
+ #include <dt-bindings/spmi/spmi.h>
+ spmi {
+ #address-cells = <2>;
+ #size-cells = <0>;
+ usb-pd@c {
+ compatible = "apple,sn201202x";
+ reg = <0xc SPMI_USID>;
+ interrupts = <11 IRQ_TYPE_EDGE_RISING>,
+ <13 IRQ_TYPE_EDGE_RISING>,
+ <17 IRQ_TYPE_EDGE_RISING>,
+ <19 IRQ_TYPE_EDGE_RISING>;
+ interrupt-names = "irq", "select", "sleep", "wake";
+
+ connector {
+ compatible = "usb-c-connector";
+ label = "USB-C";
+
+ port {
+ typec_ep: endpoint {
+ remote-endpoint = <&otg_ep>;
+ };
+ };
+ };
+ };
+ };
diff --git a/MAINTAINERS b/MAINTAINERS
index ff08a65d942a..b2819ca62e0c 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -2610,6 +2610,7 @@ F: Documentation/devicetree/bindings/rtc/apple,smc-rtc.yaml
F: Documentation/devicetree/bindings/spi/apple,spi.yaml
F: Documentation/devicetree/bindings/spmi/apple,spmi.yaml
F: Documentation/devicetree/bindings/usb/apple,dwc3.yaml
+F: Documentation/devicetree/bindings/usb/apple,sn201202x.yaml
F: Documentation/devicetree/bindings/watchdog/apple,wdt.yaml
F: Documentation/hwmon/macsmc-hwmon.rst
F: arch/arm64/boot/dts/apple/
--
2.55.0
^ permalink raw reply related [flat|nested] 7+ messages in thread* Re: [PATCH 1/3] dt-bindings: usb: tps6598x: Add sn201202x/ACE3
2026-07-25 16:20 ` [PATCH 1/3] dt-bindings: usb: tps6598x: Add sn201202x/ACE3 Sasha Finkelstein
@ 2026-07-25 16:30 ` sashiko-bot
0 siblings, 0 replies; 7+ messages in thread
From: sashiko-bot @ 2026-07-25 16:30 UTC (permalink / raw)
To: Sasha Finkelstein; +Cc: robh, devicetree, conor+dt
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] The `wakeup-source` property is missing from the binding schema despite the hardware supporting wake-up.
- [Low] The `reg` property lacks a `maxItems` boundary constraint.
--
--- Patch [1]: [PATCH 1/3] dt-bindings: usb: tps6598x: Add sn201202x/ACE3 ---
commit 9a24f811e68db136afd48ea26f05ded70940e705
Author: Sasha Finkelstein <k@chaosmail.tech>
dt-bindings: usb: tps6598x: Add sn201202x/ACE3
This commit adds a binding for the sn201202x, a variant of the tps6598x
Type-C Power Delivery controller attached to an SPMI bus on Apple M3 devices.
> diff --git a/Documentation/devicetree/bindings/usb/apple,sn201202x.yaml b/Documentation/devicetree/bindings/usb/apple,sn201202x.yaml
> new file mode 100644
> index 0000000000000..84d230ccccc7f
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/usb/apple,sn201202x.yaml
> @@ -0,0 +1,78 @@
[ ... ]
> +properties:
> + compatible:
> + enum:
> + - apple,sn201202x
> +
> + reg:
> + minItems: 1
[Severity: Low]
Is a maxItems constraint also needed here?
Without a maxItems limit, the schema might silently accept device trees with
multiple register region entries, even if only one is expected on the SPMI
bus.
> +
> + interrupts:
> + items:
> + - description: Primary irq used for tps6598x events
> + - description: Logical register selection completed
> + - description: Standby command completed
> + - description: Wakeup command completed
> +
> + interrupt-names:
> + items:
> + - const: irq
> + - const: select
> + - const: sleep
> + - const: wake
> +
> + connector:
> + $ref: /schemas/connector/usb-connector.yaml#
[Severity: Medium]
Should a wakeup-source property be declared in this properties block?
Because the hardware supports a dedicated wake interrupt, the tps6598x driver
will likely query the wakeup-source property. Since additionalProperties is
set to false below, any board device tree that properly defines wakeup-source
will trigger validation errors during dt_binding_check.
[ ... ]
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260725-tipd-ace3-v1-0-73202d36138a@chaosmail.tech?part=1
^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH 2/3] usb: typec: tipd: Factor out i2c specifics
2026-07-25 16:20 [PATCH 0/3] usb: typec: tipd: Add sn201202x (ACE3) support Sasha Finkelstein
2026-07-25 16:20 ` [PATCH 1/3] dt-bindings: usb: tps6598x: Add sn201202x/ACE3 Sasha Finkelstein
@ 2026-07-25 16:20 ` Sasha Finkelstein
2026-07-25 16:36 ` sashiko-bot
2026-07-25 16:20 ` [PATCH 3/3] usb: typec: tipd: Add sn201202x support Sasha Finkelstein
2 siblings, 1 reply; 7+ messages in thread
From: Sasha Finkelstein @ 2026-07-25 16:20 UTC (permalink / raw)
To: Sven Peter, Janne Grunau, Neal Gompa, Greg Kroah-Hartman,
Rob Herring, Krzysztof Kozlowski, Conor Dooley, Heikki Krogerus
Cc: asahi, linux-arm-kernel, linux-usb, devicetree, linux-kernel,
Sasha Finkelstein, Alyssa Milburn
From: Alyssa Milburn <amilburn@zall.org>
Make the core driver more bus-agnostic to prepare for SPMI variants of
the tipd chip
Signed-off-by: Alyssa Milburn <amilburn@zall.org>
Signed-off-by: Sasha Finkelstein <k@chaosmail.tech>
---
drivers/usb/typec/tipd/Makefile | 2 +-
drivers/usb/typec/tipd/{core.c => core.h} | 109 ++++++++++++++++++++++++++++---------------------------------------------------------------------------------
drivers/usb/typec/tipd/i2c.c | 86 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
3 files changed, 115 insertions(+), 82 deletions(-)
diff --git a/drivers/usb/typec/tipd/Makefile b/drivers/usb/typec/tipd/Makefile
index aa439f80a889..ba38daee9e72 100644
--- a/drivers/usb/typec/tipd/Makefile
+++ b/drivers/usb/typec/tipd/Makefile
@@ -2,5 +2,5 @@
CFLAGS_trace.o := -I$(src)
obj-$(CONFIG_TYPEC_TPS6598X) += tps6598x.o
-tps6598x-y := core.o
+tps6598x-y := i2c.o
tps6598x-$(CONFIG_TRACING) += trace.o
diff --git a/drivers/usb/typec/tipd/core.c b/drivers/usb/typec/tipd/core.h
similarity index 95%
rename from drivers/usb/typec/tipd/core.c
rename to drivers/usb/typec/tipd/core.h
index d5ee0af9058b..1ba29e439e01 100644
--- a/drivers/usb/typec/tipd/core.c
+++ b/drivers/usb/typec/tipd/core.h
@@ -9,7 +9,6 @@
#include <linux/i2c.h>
#include <linux/acpi.h>
#include <linux/gpio/consumer.h>
-#include <linux/module.h>
#include <linux/of.h>
#include <linux/power_supply.h>
#include <linux/regmap.h>
@@ -167,6 +166,7 @@ struct tps6598x {
struct device *dev;
struct regmap *regmap;
struct mutex lock; /* device lock */
+ int irq;
u8 i2c_protocol:1;
struct gpio_desc *reset;
@@ -231,6 +231,13 @@ static const char *tps6598x_psy_name_prefix = "tps6598x-source-psy-";
*/
#define TPS_MAX_LEN 64
+static struct tps6598x *tps6598x_from_device(struct device *dev)
+{
+ struct i2c_client *client = i2c_verify_client(dev);
+ struct tps6598x *tps = i2c_get_clientdata(client);
+ return tps;
+}
+
static int
tps6598x_block_read(struct tps6598x *tps, u8 reg, void *val, size_t len)
{
@@ -1738,27 +1745,13 @@ static void cd321x_remove(struct tps6598x *tps)
cancel_delayed_work_sync(&cd321x->update_work);
}
-static int tps6598x_probe(struct i2c_client *client)
+static int tps6598x_probe(struct tps6598x *tps)
{
- const struct tipd_data *data;
- struct tps6598x *tps;
struct fwnode_handle *fwnode;
u32 status;
u32 vid;
int ret;
- data = i2c_get_match_data(client);
- if (!data)
- return -EINVAL;
-
- tps = devm_kzalloc(&client->dev, data->tps_struct_size, GFP_KERNEL);
- if (!tps)
- return -ENOMEM;
-
- mutex_init(&tps->lock);
- tps->dev = &client->dev;
- tps->data = data;
-
tps->reset = devm_gpiod_get_optional(tps->dev, "reset", GPIOD_OUT_LOW);
if (IS_ERR(tps->reset))
return dev_err_probe(tps->dev, PTR_ERR(tps->reset),
@@ -1766,23 +1759,12 @@ static int tps6598x_probe(struct i2c_client *client)
if (tps->reset)
msleep(TPS_SETUP_MS);
- tps->regmap = devm_regmap_init_i2c(client, &tps6598x_regmap_config);
- if (IS_ERR(tps->regmap))
- return PTR_ERR(tps->regmap);
-
if (!device_is_compatible(tps->dev, "ti,tps25750")) {
ret = tps6598x_read32(tps, TPS_REG_VID, &vid);
if (ret < 0 || !vid)
return -ENODEV;
}
- /*
- * Checking can the adapter handle SMBus protocol. If it can not, the
- * driver needs to take care of block reads separately.
- */
- if (i2c_check_functionality(client->adapter, I2C_FUNC_I2C))
- tps->i2c_protocol = true;
-
if (tps->data->switch_power_state) {
ret = tps->data->switch_power_state(tps, TPS_SYSTEM_POWER_STATE_S0);
if (ret)
@@ -1816,7 +1798,7 @@ static int tps6598x_probe(struct i2c_client *client)
* with existing DT files, we work around this by deleting any
* fwnode_links to/from this fwnode.
*/
- fwnode = device_get_named_child_node(&client->dev, "connector");
+ fwnode = device_get_named_child_node(tps->dev, "connector");
if (fwnode)
fw_devlink_purge_absent_suppliers(fwnode);
@@ -1842,14 +1824,14 @@ static int tps6598x_probe(struct i2c_client *client)
goto err_unregister_port;
ret = tps->data->connect(tps, status);
if (ret)
- dev_err(&client->dev, "failed to register partner\n");
+ dev_err(tps->dev, "failed to register partner\n");
}
- if (client->irq) {
- ret = devm_request_threaded_irq(&client->dev, client->irq, NULL,
+ if (tps->irq) {
+ ret = devm_request_threaded_irq(tps->dev, tps->irq, NULL,
tps->data->irq_handler,
IRQF_SHARED | IRQF_ONESHOT,
- dev_name(&client->dev), tps);
+ dev_name(tps->dev), tps);
} else {
dev_warn(tps->dev, "Unable to find the interrupt, switching to polling\n");
INIT_DELAYED_WORK(&tps->wq_poll, tps6598x_poll_work);
@@ -1860,13 +1842,12 @@ static int tps6598x_probe(struct i2c_client *client)
if (ret)
goto err_disconnect;
- i2c_set_clientdata(client, tps);
fwnode_handle_put(fwnode);
tps->wakeup = device_property_read_bool(tps->dev, "wakeup-source");
- if (tps->wakeup && client->irq) {
- devm_device_init_wakeup(&client->dev);
- enable_irq_wake(client->irq);
+ if (tps->wakeup && tps->irq) {
+ devm_device_init_wakeup(tps->dev);
+ enable_irq_wake(tps->irq);
}
return 0;
@@ -1888,14 +1869,12 @@ static int tps6598x_probe(struct i2c_client *client)
return ret;
}
-static void tps6598x_remove(struct i2c_client *client)
+static void tps6598x_remove(struct tps6598x *tps)
{
- struct tps6598x *tps = i2c_get_clientdata(client);
-
- if (!client->irq)
+ if (!tps->irq)
cancel_delayed_work_sync(&tps->wq_poll);
else
- devm_free_irq(tps->dev, client->irq, tps);
+ devm_free_irq(tps->dev, tps->irq, tps);
if (tps->data->remove)
tps->data->remove(tps);
@@ -1913,17 +1892,16 @@ static void tps6598x_remove(struct i2c_client *client)
static int __maybe_unused tps6598x_suspend(struct device *dev)
{
- struct i2c_client *client = to_i2c_client(dev);
- struct tps6598x *tps = i2c_get_clientdata(client);
+ struct tps6598x *tps = tps6598x_from_device(dev);
if (tps->wakeup) {
- disable_irq(client->irq);
- enable_irq_wake(client->irq);
+ disable_irq(tps->irq);
+ enable_irq_wake(tps->irq);
} else if (tps->reset) {
gpiod_set_value_cansleep(tps->reset, 1);
}
- if (!client->irq)
+ if (!tps->irq)
cancel_delayed_work_sync(&tps->wq_poll);
return 0;
@@ -1931,8 +1909,7 @@ static int __maybe_unused tps6598x_suspend(struct device *dev)
static int __maybe_unused tps6598x_resume(struct device *dev)
{
- struct i2c_client *client = to_i2c_client(dev);
- struct tps6598x *tps = i2c_get_clientdata(client);
+ struct tps6598x *tps = tps6598x_from_device(dev);
int ret;
ret = tps6598x_check_mode(tps);
@@ -1946,14 +1923,14 @@ static int __maybe_unused tps6598x_resume(struct device *dev)
}
if (tps->wakeup) {
- disable_irq_wake(client->irq);
- enable_irq(client->irq);
+ disable_irq_wake(tps->irq);
+ enable_irq(tps->irq);
} else if (tps->reset) {
gpiod_set_value_cansleep(tps->reset, 0);
msleep(TPS_SETUP_MS);
}
- if (!client->irq)
+ if (!tps->irq)
queue_delayed_work(system_power_efficient_wq, &tps->wq_poll,
msecs_to_jiffies(POLL_INTERVAL));
@@ -2018,33 +1995,3 @@ static const struct tipd_data tps25750_data = {
.reset = tps25750_reset,
.connect = tps6598x_connect,
};
-
-static const struct of_device_id tps6598x_of_match[] = {
- { .compatible = "ti,tps6598x", &tps6598x_data},
- { .compatible = "apple,cd321x", &cd321x_data},
- { .compatible = "ti,tps25750", &tps25750_data},
- {}
-};
-MODULE_DEVICE_TABLE(of, tps6598x_of_match);
-
-static const struct i2c_device_id tps6598x_id[] = {
- { .name = "tps6598x", .driver_data = (kernel_ulong_t)&tps6598x_data },
- { }
-};
-MODULE_DEVICE_TABLE(i2c, tps6598x_id);
-
-static struct i2c_driver tps6598x_i2c_driver = {
- .driver = {
- .name = "tps6598x",
- .pm = &tps6598x_pm_ops,
- .of_match_table = tps6598x_of_match,
- },
- .probe = tps6598x_probe,
- .remove = tps6598x_remove,
- .id_table = tps6598x_id,
-};
-module_i2c_driver(tps6598x_i2c_driver);
-
-MODULE_AUTHOR("Heikki Krogerus <heikki.krogerus@linux.intel.com>");
-MODULE_LICENSE("GPL v2");
-MODULE_DESCRIPTION("TI TPS6598x USB Power Delivery Controller Driver");
diff --git a/drivers/usb/typec/tipd/i2c.c b/drivers/usb/typec/tipd/i2c.c
new file mode 100644
index 000000000000..0233e19290ea
--- /dev/null
+++ b/drivers/usb/typec/tipd/i2c.c
@@ -0,0 +1,86 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Driver for TI TPS6598x USB Power Delivery controller family
+ *
+ * Copyright (C) 2017, Intel Corporation
+ * Author: Heikki Krogerus <heikki.krogerus@linux.intel.com>
+ */
+
+#include <linux/module.h>
+
+#include "core.h"
+
+static int tps6598x_probe_i2c(struct i2c_client *client)
+{
+ const struct tipd_data *data;
+ struct tps6598x *tps;
+ int ret;
+
+ data = i2c_get_match_data(client);
+ if (!data)
+ return -EINVAL;
+
+ tps = devm_kzalloc(&client->dev, data->tps_struct_size, GFP_KERNEL);
+ if (!tps)
+ return -ENOMEM;
+
+ mutex_init(&tps->lock);
+ tps->dev = &client->dev;
+ tps->data = data;
+ tps->irq = client->irq;
+
+ tps->regmap = devm_regmap_init_i2c(client, &tps6598x_regmap_config);
+ if (IS_ERR(tps->regmap))
+ return PTR_ERR(tps->regmap);
+
+ /*
+ * Checking can the adapter handle SMBus protocol. If it can not, the
+ * driver needs to take care of block reads separately.
+ */
+ if (i2c_check_functionality(client->adapter, I2C_FUNC_I2C))
+ tps->i2c_protocol = true;
+
+ ret = tps6598x_probe(tps);
+
+ if (ret == 0)
+ i2c_set_clientdata(client, tps);
+
+ return ret;
+}
+
+static void tps6598x_remove_i2c(struct i2c_client *client)
+{
+ struct tps6598x *tps = i2c_get_clientdata(client);
+
+ tps6598x_remove(tps);
+}
+
+static const struct of_device_id tps6598x_of_match[] = {
+ { .compatible = "ti,tps6598x", &tps6598x_data},
+ { .compatible = "apple,cd321x", &cd321x_data},
+ { .compatible = "ti,tps25750", &tps25750_data},
+ {}
+};
+MODULE_DEVICE_TABLE(of, tps6598x_of_match);
+
+static const struct i2c_device_id tps6598x_id[] = {
+ { .name = "tps6598x", .driver_data = (kernel_ulong_t)&tps6598x_data },
+ { }
+};
+MODULE_DEVICE_TABLE(i2c, tps6598x_id);
+
+static struct i2c_driver tps6598x_i2c_driver = {
+ .driver = {
+ .name = "tps6598x",
+ .pm = &tps6598x_pm_ops,
+ .of_match_table = tps6598x_of_match,
+ },
+ .probe = tps6598x_probe_i2c,
+ .remove = tps6598x_remove_i2c,
+ .id_table = tps6598x_id,
+};
+module_i2c_driver(tps6598x_i2c_driver);
+
+MODULE_AUTHOR("Heikki Krogerus <heikki.krogerus@linux.intel.com>");
+MODULE_LICENSE("GPL");
+MODULE_DESCRIPTION("TI TPS6598x USB Power Delivery Controller Driver");
--
2.55.0
^ permalink raw reply related [flat|nested] 7+ messages in thread* Re: [PATCH 2/3] usb: typec: tipd: Factor out i2c specifics
2026-07-25 16:20 ` [PATCH 2/3] usb: typec: tipd: Factor out i2c specifics Sasha Finkelstein
@ 2026-07-25 16:36 ` sashiko-bot
0 siblings, 0 replies; 7+ messages in thread
From: sashiko-bot @ 2026-07-25 16:36 UTC (permalink / raw)
To: Sasha Finkelstein; +Cc: conor+dt, robh, devicetree
Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
New issues:
- [Medium] Moving the entire driver implementation into a header file (`core.h`) and including it directly causes code bloat and violates C best practices.
- [Medium] The module license string was changed from strict 'GPL v2' to 'GPL', broadening the license terms and conflicting with the SPDX tag.
Pre-existing issues:
- [High] Delayed works (`wq_poll` and `update_work`) are not cancelled on the probe error path, leading to a use-after-free.
- [High] Suspend/resume functions blindly modify IRQ 0 if `wakeup-source` is set but no interrupt is provided, and the IRQ wake count is unbalanced.
- [High] The device is accessed via I2C before its reset line is deasserted during resume.
--
--- Patch [2]: [PATCH 2/3] usb: typec: tipd: Factor out i2c specifics ---
commit 6db23dbbea249ebf73709095210ec56e8a1e7da9
Author: Alyssa Milburn <amilburn@zall.org>
usb: typec: tipd: Factor out i2c specifics
Make the core driver more bus-agnostic to prepare for SPMI variants of
the tipd chip
> diff --git a/drivers/usb/typec/tipd/core.c b/drivers/usb/typec/tipd/core.h
> similarity index 95%
> rename from drivers/usb/typec/tipd/core.c
> rename to drivers/usb/typec/tipd/core.h
> index d5ee0af9058b9..1ba29e439e01c 100644
> --- a/drivers/usb/typec/tipd/core.c
> +++ b/drivers/usb/typec/tipd/core.h
[Severity: Medium]
Is it intentional to move the entire core driver implementation into a header
file?
If this file is included from multiple bus variant C files, won't it
result in duplicate copies of all these static functions in every object file,
causing significant kernel binary bloat?
Would it be better to leave the core logic in a separate C file and export
the necessary functions?
[ ... ]
> @@ -1860,13 +1842,12 @@ static int tps6598x_probe(struct i2c_client *client)
> if (ret)
> goto err_disconnect;
[Severity: High]
This is a pre-existing issue, but does this error path cause a use-after-free?
In tps6598x_probe(), if a plug is present, tps->data->connect() may queue
cd321x->update_work, or if tps->irq == 0, wq_poll is queued.
If a subsequent step fails (like devm_request_threaded_irq) or connect()
itself fails when there is no interrupt, the code jumps to err_disconnect:
err_disconnect:
tps6598x_disconnect(tps, 0);
err_unregister_port:
...
This error path does not appear to cancel wq_poll or update_work, meaning
they could execute after the tps structure is freed by devres.
[ ... ]
> @@ -1913,17 +1892,16 @@ static void tps6598x_remove(struct i2c_client *client)
>
> static int __maybe_unused tps6598x_suspend(struct device *dev)
> {
> - struct i2c_client *client = to_i2c_client(dev);
> - struct tps6598x *tps = i2c_get_clientdata(client);
> + struct tps6598x *tps = tps6598x_from_device(dev);
>
> if (tps->wakeup) {
> - disable_irq(client->irq);
> - enable_irq_wake(client->irq);
[Severity: High]
This isn't a bug introduced by this patch, but what happens here if a device
tree specifies "wakeup-source" but does not provide an interrupt line?
During tps6598x_probe(), tps->wakeup would be set to true. Then in
tps6598x_suspend(), this check unconditionally calls disable_irq() and
enable_irq_wake() even if tps->irq is 0.
Could modifying irq 0 disable critical system timers and hang the system
during suspend?
Also, enable_irq_wake() is called during probe when tps->wakeup and
tps->irq are set, but there appears to be no corresponding disable_irq_wake()
in tps6598x_remove(), which might leak the wake count.
> } else if (tps->reset) {
> gpiod_set_value_cansleep(tps->reset, 1);
> }
[ ... ]
> @@ -1931,8 +1909,7 @@ static int __maybe_unused tps6598x_suspend(struct device *dev)
>
> static int __maybe_unused tps6598x_resume(struct device *dev)
> {
> - struct i2c_client *client = to_i2c_client(dev);
> - struct tps6598x *tps = i2c_get_clientdata(client);
> + struct tps6598x *tps = tps6598x_from_device(dev);
> int ret;
>
> ret = tps6598x_check_mode(tps);
[Severity: High]
This is a pre-existing issue, but does this i2c transaction happen too early?
If this is a non-wakeup device with a configured reset gpio, system suspend
asserts the hardware reset by setting the gpio to 1.
During system resume, tps6598x_resume() attempts to communicate with the
device via tps6598x_check_mode() here, before the reset gpio is deasserted
later in the function.
Will this cause the i2c read to fail and leave the device unusable?
> if (ret < 0)
> return ret;
[ ... ]
> diff --git a/drivers/usb/typec/tipd/i2c.c b/drivers/usb/typec/tipd/i2c.c
> new file mode 100644
> index 0000000000000..0233e19290ea2
> --- /dev/null
> +++ b/drivers/usb/typec/tipd/i2c.c
> @@ -0,0 +1,86 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Driver for TI TPS6598x USB Power Delivery controller family
> + *
> + * Copyright (C) 2017, Intel Corporation
> + * Author: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> + */
> +
> +#include <linux/module.h>
> +
> +#include "core.h"
[Severity: Medium]
By including "core.h" directly instead of linking object files, aren't we
embedding the entire core driver implementation directly into this
compilation unit?
[ ... ]
> +MODULE_AUTHOR("Heikki Krogerus <heikki.krogerus@linux.intel.com>");
> +MODULE_LICENSE("GPL");
[Severity: Medium]
Is it intentional to change the module license string from "GPL v2" to "GPL"?
In the kernel, "GPL" implies "GPL v2 or later", which broadens the license
terms and seems to conflict with the SPDX tag at the top of the file.
> +MODULE_DESCRIPTION("TI TPS6598x USB Power Delivery Controller Driver");
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260725-tipd-ace3-v1-0-73202d36138a@chaosmail.tech?part=2
^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH 3/3] usb: typec: tipd: Add sn201202x support
2026-07-25 16:20 [PATCH 0/3] usb: typec: tipd: Add sn201202x (ACE3) support Sasha Finkelstein
2026-07-25 16:20 ` [PATCH 1/3] dt-bindings: usb: tps6598x: Add sn201202x/ACE3 Sasha Finkelstein
2026-07-25 16:20 ` [PATCH 2/3] usb: typec: tipd: Factor out i2c specifics Sasha Finkelstein
@ 2026-07-25 16:20 ` Sasha Finkelstein
2026-07-25 16:36 ` sashiko-bot
2 siblings, 1 reply; 7+ messages in thread
From: Sasha Finkelstein @ 2026-07-25 16:20 UTC (permalink / raw)
To: Sven Peter, Janne Grunau, Neal Gompa, Greg Kroah-Hartman,
Rob Herring, Krzysztof Kozlowski, Conor Dooley, Heikki Krogerus
Cc: asahi, linux-arm-kernel, linux-usb, devicetree, linux-kernel,
Sasha Finkelstein, Alyssa Milburn
Add support for sn201202x (aka ACE3), a tipd variant that uses a very
similar register map, that is exposed over a "logical register"
interface on the SPMI bus.
Co-developed-by: Alyssa Milburn <amilburn@zall.org>
Signed-off-by: Alyssa Milburn <amilburn@zall.org>
Signed-off-by: Sasha Finkelstein <k@chaosmail.tech>
---
MAINTAINERS | 1 +
drivers/usb/typec/tipd/Kconfig | 14 +++++++
drivers/usb/typec/tipd/Makefile | 12 ++++--
drivers/usb/typec/tipd/core.h | 14 ++++++-
drivers/usb/typec/tipd/spmi.c | 297 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
5 files changed, 332 insertions(+), 6 deletions(-)
diff --git a/MAINTAINERS b/MAINTAINERS
index b2819ca62e0c..821932703b60 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -2641,6 +2641,7 @@ F: drivers/soc/apple/*
F: drivers/spi/spi-apple.c
F: drivers/spmi/spmi-apple-controller.c
F: drivers/usb/dwc3/dwc3-apple.c
+F: drivers/usb/typec/tipd/spmi.c
F: drivers/video/backlight/apple_dwi_bl.c
F: drivers/watchdog/apple_wdt.c
F: include/dt-bindings/interrupt-controller/apple-aic.h
diff --git a/drivers/usb/typec/tipd/Kconfig b/drivers/usb/typec/tipd/Kconfig
index b82715293072..6aa2fa439fdd 100644
--- a/drivers/usb/typec/tipd/Kconfig
+++ b/drivers/usb/typec/tipd/Kconfig
@@ -10,3 +10,17 @@ config TYPEC_TPS6598X
If you choose to build this driver as a dynamically linked module, the
module will be called tps6598x.ko.
+
+config TYPEC_SN201202X
+ tristate "TI SN201202x USB Power Delivery controller support"
+ select POWER_SUPPLY
+ select REGMAP_I2C
+ select USB_ROLE_SWITCH
+ select REGMAP_SPMI
+ help
+ Say Y here to enable support for SN201202x, a TPS6598x variant
+ that uses SPMI as a host bus, and is found on Apple devices starting
+ from the M3 series.
+
+ If you choose to build this driver as a dynamically linked module, the
+ module will be called sn201202x.ko.
diff --git a/drivers/usb/typec/tipd/Makefile b/drivers/usb/typec/tipd/Makefile
index ba38daee9e72..9a547bb14831 100644
--- a/drivers/usb/typec/tipd/Makefile
+++ b/drivers/usb/typec/tipd/Makefile
@@ -1,6 +1,10 @@
# SPDX-License-Identifier: GPL-2.0
-CFLAGS_trace.o := -I$(src)
+CFLAGS_trace.o := -I$(src)
-obj-$(CONFIG_TYPEC_TPS6598X) += tps6598x.o
-tps6598x-y := i2c.o
-tps6598x-$(CONFIG_TRACING) += trace.o
+obj-$(CONFIG_TYPEC_TPS6598X) += tps6598x.o
+tps6598x-y := i2c.o
+tps6598x-$(CONFIG_TRACING) += trace.o
+
+obj-$(CONFIG_TYPEC_SN201202X) += sn201202x.o
+sn201202x-y := spmi.o
+sn201202x-$(CONFIG_TRACING) += trace.o
diff --git a/drivers/usb/typec/tipd/core.h b/drivers/usb/typec/tipd/core.h
index 1ba29e439e01..fed326a9788d 100644
--- a/drivers/usb/typec/tipd/core.h
+++ b/drivers/usb/typec/tipd/core.h
@@ -25,6 +25,10 @@
#include "tps6598x.h"
#include "trace.h"
+#ifdef CONFIG_TYPEC_SN201202X
+#include <linux/spmi.h>
+#endif
+
/* Register offsets */
#define TPS_REG_VID 0x00
#define TPS_REG_MODE 0x03
@@ -234,8 +238,14 @@ static const char *tps6598x_psy_name_prefix = "tps6598x-source-psy-";
static struct tps6598x *tps6598x_from_device(struct device *dev)
{
struct i2c_client *client = i2c_verify_client(dev);
- struct tps6598x *tps = i2c_get_clientdata(client);
- return tps;
+#ifdef CONFIG_TYPEC_SN201202X
+ if (!client) {
+ struct spmi_device *device = to_spmi_device(dev);
+
+ return spmi_device_get_drvdata(device);
+ }
+#endif
+ return i2c_get_clientdata(client);
}
static int
diff --git a/drivers/usb/typec/tipd/spmi.c b/drivers/usb/typec/tipd/spmi.c
new file mode 100644
index 000000000000..f94453131ea2
--- /dev/null
+++ b/drivers/usb/typec/tipd/spmi.c
@@ -0,0 +1,297 @@
+// SPDX-License-Identifier: GPL-2.0
+
+#include <linux/spmi.h>
+#include <linux/of_device.h>
+#include <linux/of_irq.h>
+
+#include "core.h"
+
+struct sn201202x {
+ struct cd321x cd;
+ struct completion select_completion;
+ struct completion sleep_completion;
+ struct completion wake_completion;
+ struct spmi_device *sdev;
+};
+
+#define tps_to_sn(tps) container_of_const((tps), struct sn201202x, cd.tps)
+
+static int regmap_sn201202x_select_reg(struct spmi_device *sdev, u8 reg)
+{
+ int err;
+ u8 val;
+ bool warned = false;
+ struct tps6598x *tps = spmi_device_get_drvdata(sdev);
+ struct sn201202x *sn = tps_to_sn(tps);
+
+ reinit_completion(&sn->select_completion);
+ err = spmi_register_zero_write(sdev, reg);
+ if (err)
+ return err;
+
+ if (!wait_for_completion_timeout(&sn->select_completion, msecs_to_jiffies(100)))
+ return -ETIMEDOUT;
+
+ while (1) {
+ err = spmi_register_read(sdev, 0, &val);
+ if (err)
+ return err;
+ if (val == (reg | 0x80)) {
+ if (!warned) {
+ dev_warn(tps->dev,
+ "Got interrupt but selection not complete?\n");
+ warned = true;
+ }
+ msleep(20);
+ continue;
+ }
+ if (val == reg)
+ break;
+ return -EIO;
+ }
+
+ return 0;
+}
+
+static int regmap_sn201202x_read(void *context,
+ const void *reg, size_t reg_size,
+ void *val, size_t val_size)
+{
+ int err;
+ unsigned int offset = 0x20;
+ size_t len;
+ u8 addr;
+
+ WARN_ON(reg_size != 1);
+ WARN_ON(val_size > 0x40);
+
+ addr = *(u8 *)reg;
+
+ err = regmap_sn201202x_select_reg(context, addr);
+ if (err)
+ return err;
+
+ while (val_size) {
+ len = min_t(size_t, val_size, 16);
+ err = spmi_ext_register_read(context, offset, val, len);
+ if (err)
+ return err;
+ offset += len;
+ val += len;
+ val_size -= len;
+ }
+
+ return 0;
+}
+
+static int regmap_sn201202x_write(void *context, const void *data,
+ size_t count)
+{
+ int err = 0;
+ unsigned int offset = 0xa0;
+ size_t len;
+ u8 addr;
+
+ WARN_ON(count < 1);
+
+ addr = *(u8 *)data;
+ data += 1;
+ count -= 1;
+
+ WARN_ON(count > 0x40);
+
+ err = regmap_sn201202x_select_reg(context, addr);
+ if (err)
+ return err;
+
+ while (count) {
+ len = min_t(size_t, count, 16);
+ err = spmi_ext_register_write(context, offset, data, len);
+ if (err)
+ return err;
+ offset += len;
+ data += len;
+ count -= len;
+ }
+
+ return err;
+}
+
+static irqreturn_t sn201202x_irq(int irq, void *data)
+{
+ struct completion *c = data;
+
+ complete(c);
+ return IRQ_HANDLED;
+}
+
+static const struct regmap_bus regmap_sn201202x = {
+ .read = regmap_sn201202x_read,
+ .write = regmap_sn201202x_write,
+ .reg_format_endian_default = REGMAP_ENDIAN_NATIVE,
+ .val_format_endian_default = REGMAP_ENDIAN_NATIVE,
+};
+
+static struct regmap *__devm_regmap_init_sn201202x(struct spmi_device *sdev,
+ const struct regmap_config *config,
+ struct lock_class_key *lock_key,
+ const char *lock_name)
+{
+ return __devm_regmap_init(&sdev->dev, ®map_sn201202x, sdev, config,
+ lock_key, lock_name);
+}
+
+#define devm_regmap_init_sn201202x(dev, config) \
+ __regmap_lockdep_wrapper(__devm_regmap_init_sn201202x, #config, \
+ dev, config)
+
+static const struct tipd_data sn201202x_data = {
+ .irq_handler = cd321x_interrupt,
+ .irq_mask1 = APPLE_CD_REG_INT_POWER_STATUS_UPDATE |
+ APPLE_CD_REG_INT_DATA_STATUS_UPDATE |
+ APPLE_CD_REG_INT_PLUG_EVENT,
+ .tps_struct_size = sizeof(struct sn201202x),
+ .remove = cd321x_remove,
+ .register_port = cd321x_register_port,
+ .unregister_port = cd321x_unregister_port,
+ .trace_data_status = trace_cd321x_data_status,
+ .trace_power_status = trace_tps6598x_power_status,
+ .trace_status = trace_tps6598x_status,
+ .init = cd321x_init,
+ .read_data_status = cd321x_read_data_status,
+ .reset = cd321x_reset,
+ .switch_power_state = cd321x_switch_power_state,
+ .connect = cd321x_connect,
+};
+
+static const struct of_device_id sn201202x_of_match[] = {
+ { .compatible = "apple,sn201202x", &sn201202x_data},
+ {}
+};
+
+static int sn201202x_probe(struct spmi_device *device)
+{
+ const struct of_device_id *match;
+ const struct tipd_data *data;
+ struct sn201202x *sn;
+ struct tps6598x *tps;
+ int irq_select, irq_sleep, irq_wake;
+ int ret;
+
+ match = of_match_device(sn201202x_of_match, &device->dev);
+ if (!match)
+ return -EINVAL;
+ data = match->data;
+
+ sn = devm_kzalloc(&device->dev, data->tps_struct_size, GFP_KERNEL);
+ if (!sn)
+ return -ENOMEM;
+ sn->sdev = device;
+ tps = &sn->cd.tps;
+
+ mutex_init(&tps->lock);
+ tps->dev = &device->dev;
+ tps->data = data;
+
+ tps->irq = of_irq_get_byname(device->dev.of_node, "irq");
+ if (tps->irq < 0)
+ return tps->irq;
+ irq_select = of_irq_get_byname(device->dev.of_node, "select");
+ if (irq_select < 0)
+ return irq_select;
+ irq_sleep = of_irq_get_byname(device->dev.of_node, "sleep");
+ if (irq_sleep < 0)
+ return irq_sleep;
+ irq_wake = of_irq_get_byname(device->dev.of_node, "wake");
+ if (irq_wake < 0)
+ return irq_wake;
+
+ init_completion(&sn->select_completion);
+ init_completion(&sn->sleep_completion);
+ init_completion(&sn->wake_completion);
+
+ ret = devm_request_threaded_irq(&device->dev, irq_select, NULL, sn201202x_irq,
+ IRQF_ONESHOT, NULL, &sn->select_completion);
+ if (ret)
+ return ret;
+ ret = devm_request_threaded_irq(&device->dev, irq_sleep, NULL, sn201202x_irq,
+ IRQF_ONESHOT, NULL, &sn->sleep_completion);
+ if (ret)
+ return ret;
+ ret = devm_request_threaded_irq(&device->dev, irq_wake, NULL, sn201202x_irq,
+ IRQF_ONESHOT, NULL, &sn->wake_completion);
+ if (ret)
+ return ret;
+
+ tps->regmap = devm_regmap_init_sn201202x(device, &tps6598x_regmap_config);
+ if (IS_ERR(tps->regmap))
+ return PTR_ERR(tps->regmap);
+
+ ret = spmi_command_wakeup(device);
+ if (ret)
+ return ret;
+ if (!wait_for_completion_timeout(&sn->wake_completion, msecs_to_jiffies(100)))
+ return -ETIMEDOUT;
+
+ spmi_device_set_drvdata(device, tps);
+ return tps6598x_probe(tps);
+}
+
+static void sn201202x_remove(struct spmi_device *device)
+{
+ struct tps6598x *tps = spmi_device_get_drvdata(device);
+
+ tps6598x_remove(tps);
+}
+
+static int __maybe_unused sn201202x_resume(struct device *dev)
+{
+ struct tps6598x *tps = dev_get_drvdata(dev);
+ struct sn201202x *sn = tps_to_sn(tps);
+ int err;
+
+ err = spmi_command_wakeup(sn->sdev);
+ if (err)
+ return err;
+ if (!wait_for_completion_timeout(&sn->wake_completion, msecs_to_jiffies(100)))
+ return -ETIMEDOUT;
+ return tps6598x_resume(dev);
+}
+
+static int __maybe_unused sn201202x_suspend(struct device *dev)
+{
+ struct tps6598x *tps = dev_get_drvdata(dev);
+ struct sn201202x *sn = tps_to_sn(tps);
+ int err;
+
+ err = tps6598x_suspend(dev);
+ if (err)
+ return err;
+ err = spmi_command_sleep(sn->sdev);
+ if (err)
+ return err;
+ if (!wait_for_completion_timeout(&sn->sleep_completion, msecs_to_jiffies(100)))
+ return -ETIMEDOUT;
+ return 0;
+}
+
+MODULE_DEVICE_TABLE(of, sn201202x_of_match);
+
+static const struct dev_pm_ops sn201202x_pm_ops = {
+ SET_SYSTEM_SLEEP_PM_OPS(sn201202x_suspend, sn201202x_resume)
+};
+
+static struct spmi_driver sn201202x_driver = {
+ .driver = {
+ .name = "sn201202x",
+ .pm = &sn201202x_pm_ops,
+ .of_match_table = sn201202x_of_match,
+ },
+ .probe = sn201202x_probe,
+ .remove = sn201202x_remove,
+};
+module_spmi_driver(sn201202x_driver);
+
+MODULE_AUTHOR("Heikki Krogerus <heikki.krogerus@linux.intel.com>");
+MODULE_LICENSE("GPL");
+MODULE_DESCRIPTION("TI SN201202x USB Power Delivery Controller Driver");
--
2.55.0
^ permalink raw reply related [flat|nested] 7+ messages in thread* Re: [PATCH 3/3] usb: typec: tipd: Add sn201202x support
2026-07-25 16:20 ` [PATCH 3/3] usb: typec: tipd: Add sn201202x support Sasha Finkelstein
@ 2026-07-25 16:36 ` sashiko-bot
0 siblings, 0 replies; 7+ messages in thread
From: sashiko-bot @ 2026-07-25 16:36 UTC (permalink / raw)
To: Sasha Finkelstein; +Cc: devicetree, robh, conor+dt
Thank you for your contribution! Sashiko AI review found 12 potential issue(s) to consider:
New issues:
- [High] Massive code duplication caused by including a large source file as a header.
- [High] Linking `trace.o` into multiple modules causes multiple definition linker errors.
- [High] Missing `reinit_completion()` before `wait_for_completion_timeout()` in suspend and resume paths.
- [High] Race condition causing NULL pointer dereference if regmap debugfs is accessed during probe.
- [High] The `while (1)` polling loop in `regmap_sn201202x_select_reg` lacks a timeout or retry limit, leading to an indefinite kernel thread hang.
- [High] Resume will always timeout and fail if a `reset-gpios` property is present due to incorrect ordering.
- [High] If `sn201202x_suspend` fails to put the device to sleep, it returns an error without rolling back the state of the core `tps6598x` controller.
- [High] Failing to return on invalid `count` in `regmap_sn201202x_write` causes a buffer underflow and out-of-bounds memory access.
- [Medium] Unmet direct Kconfig dependencies for REGMAP_SPMI and REGMAP_I2C.
- [Medium] Unnecessary latency overhead from using a threaded IRQ solely for `complete()`.
- [Medium] The SPMI device is woken up during probe but never put back to sleep on driver removal or probe failure.
Pre-existing issues:
- [High] Delayed works are left queued when `tps6598x_probe` fails, causing Use-After-Free.
--
--- Patch [3]: [PATCH 3/3] usb: typec: tipd: Add sn201202x support ---
commit 69eff8a2ada62351102ded8bb2c3563aa5e66570
Author: Sasha Finkelstein <k@chaosmail.tech>
usb: typec: tipd: Add sn201202x support
Add support for sn201202x (aka ACE3), a tipd variant that uses a very
similar register map, that is exposed over a "logical register"
interface on the SPMI bus.
> diff --git a/drivers/usb/typec/tipd/Kconfig b/drivers/usb/typec/tipd/Kconfig
> --- a/drivers/usb/typec/tipd/Kconfig
> +++ b/drivers/usb/typec/tipd/Kconfig
[ ... ]
> +config TYPEC_SN201202X
> + tristate "TI SN201202x USB Power Delivery controller support"
> + select POWER_SUPPLY
> + select REGMAP_I2C
> + select USB_ROLE_SWITCH
> + select REGMAP_SPMI
[Severity: Medium]
Will this trigger Kconfig unmet direct dependency warnings and build failures
if the underlying SPMI or I2C subsystems are disabled?
Kconfig rules mandate that when using select, the selector must also depend
on the selected item's dependencies. TYPEC_SN201202X selects REGMAP_SPMI
and REGMAP_I2C without inheriting their dependencies (depends on SPMI and
depends on I2C). Also, is REGMAP_I2C needed at all in the SPMI driver?
> diff --git a/drivers/usb/typec/tipd/Makefile b/drivers/usb/typec/tipd/Makefile
> --- a/drivers/usb/typec/tipd/Makefile
> +++ b/drivers/usb/typec/tipd/Makefile
[ ... ]
> -obj-$(CONFIG_TYPEC_TPS6598X) += tps6598x.o
> -tps6598x-y := i2c.o
> -tps6598x-$(CONFIG_TRACING) += trace.o
> +obj-$(CONFIG_TYPEC_TPS6598X) += tps6598x.o
> +tps6598x-y := i2c.o
> +tps6598x-$(CONFIG_TRACING) += trace.o
> +
> +obj-$(CONFIG_TYPEC_SN201202X) += sn201202x.o
> +sn201202x-y := spmi.o
[Severity: High]
Does including core.h from multiple modules cause massive binary bloat?
Because core.h contains a large amount of driver logic as static functions,
compiling i2c.c and spmi.c into separate modules forces the compiler to build
two independent copies of the entire driver logic (one for tps6598x.ko and
one for sn201202x.ko). Should shared core logic be placed in a common .c file
and exported instead?
> +sn201202x-$(CONFIG_TRACING) += trace.o
[Severity: High]
Does linking trace.o into both modules cause multiple definition errors?
The Makefile appends trace.o to both tps6598x-y and sn201202x-y. Because
trace.c defines CREATE_TRACE_POINTS, it instantiates global tracepoint symbols.
If both CONFIG_TYPEC_TPS6598X=y and CONFIG_TYPEC_SN201202X=y are set, Kbuild
will link trace.o into both composite objects, leading to multiple definition
errors for the tracepoint symbols during the vmlinux link.
> diff --git a/drivers/usb/typec/tipd/spmi.c b/drivers/usb/typec/tipd/spmi.c
> new file mode 100644
> --- /dev/null
> +++ b/drivers/usb/typec/tipd/spmi.c
[ ... ]
> +static int regmap_sn201202x_select_reg(struct spmi_device *sdev, u8 reg)
> +{
> + int err;
> + u8 val;
> + bool warned = false;
> + struct tps6598x *tps = spmi_device_get_drvdata(sdev);
> + struct sn201202x *sn = tps_to_sn(tps);
> +
> + reinit_completion(&sn->select_completion);
[Severity: High]
Can this dereference a NULL pointer if a user accesses regmap debugfs early?
In sn201202x_probe(), devm_regmap_init_sn201202x() is called before the device
drvdata is set with spmi_device_set_drvdata(). This makes the regmap debugfs
nodes immediately available.
If debugfs is read before probe finishes, regmap_sn201202x_read() will call
regmap_sn201202x_select_reg(), which fetches a NULL drvdata here and passes
it to tps_to_sn(). This will crash when accessing &sn->select_completion.
> + err = spmi_register_zero_write(sdev, reg);
> + if (err)
> + return err;
> +
> + if (!wait_for_completion_timeout(&sn->select_completion, msecs_to_jiffies(100)))
> + return -ETIMEDOUT;
> +
> + while (1) {
> + err = spmi_register_read(sdev, 0, &val);
> + if (err)
> + return err;
> + if (val == (reg | 0x80)) {
> + if (!warned) {
> + dev_warn(tps->dev,
> + "Got interrupt but selection not complete?\n");
> + warned = true;
> + }
> + msleep(20);
> + continue;
> + }
[Severity: High]
Does this polling loop lack a timeout condition?
If the device repeatedly returns val == (reg | 0x80) due to a hardware
malfunction, this loop will endlessly sleep for 20ms and retry without a
limit. This permanently hangs the calling kernel thread while holding the
regmap mutex.
> + if (val == reg)
> + break;
> + return -EIO;
> + }
[ ... ]
> +static int regmap_sn201202x_write(void *context, const void *data,
> + size_t count)
> +{
> + int err = 0;
> + unsigned int offset = 0xa0;
> + size_t len;
> + u8 addr;
> +
> + WARN_ON(count < 1);
> +
> + addr = *(u8 *)data;
> + data += 1;
> + count -= 1;
[Severity: High]
Does failing to return on an invalid count cause a massive out-of-bounds access?
WARN_ON(count < 1) evaluates the condition but does not abort execution.
If count is 0 (e.g., via a regmap debugfs zero-length write), the operations
above execute on the invalid data pointer, and count -= 1 causes count to
underflow to SIZE_MAX.
The subsequent while (count) loop will execute up to SIZE_MAX bytes, flooding
the SPMI bus with out-of-bounds memory accesses.
> +
> + WARN_ON(count > 0x40);
[ ... ]
> +static irqreturn_t sn201202x_irq(int irq, void *data)
> +{
> + struct completion *c = data;
> +
> + complete(c);
> + return IRQ_HANDLED;
> +}
[ ... ]
> +static int sn201202x_probe(struct spmi_device *device)
> +{
[ ... ]
> + ret = devm_request_threaded_irq(&device->dev, irq_select, NULL, sn201202x_irq,
> + IRQF_ONESHOT, NULL, &sn->select_completion);
[Severity: Medium]
Does registering a threaded IRQ handler that only calls complete() add
unnecessary context-switch latency?
Because the primary handler is NULL, the kernel must wake up and schedule a
dedicated thread for every single SPMI register read/write transaction or
state transition. Could this be handled in a primary hardirq context using
devm_request_irq() instead?
> + if (ret)
> + return ret;
[ ... ]
> + ret = spmi_command_wakeup(device);
> + if (ret)
> + return ret;
> + if (!wait_for_completion_timeout(&sn->wake_completion, msecs_to_jiffies(100)))
> + return -ETIMEDOUT;
> +
> + spmi_device_set_drvdata(device, tps);
> + return tps6598x_probe(tps);
> +}
[Severity: Medium]
If tps6598x_probe() fails, is the device left permanently awake?
This function wakes the SPMI device via spmi_command_wakeup(), but if
tps6598x_probe() returns an error, the function aborts without calling
spmi_command_sleep() to roll back the hardware state, leaking power.
[Severity: High]
This is a pre-existing issue, but does a failure in tps6598x_probe() leave
delayed works queued, causing a Use-After-Free?
In core.h, the tps6598x_probe() error paths jump to err_disconnect and
clean up, but fail to cancel scheduled delayed works (like cd321x->update_work).
When probe fails, devres frees the device structures, leaving the delayed
work queued to dereference freed memory later.
> +static void sn201202x_remove(struct spmi_device *device)
> +{
> + struct tps6598x *tps = spmi_device_get_drvdata(device);
> +
> + tps6598x_remove(tps);
> +}
[Severity: Medium]
Does removing the driver leave the hardware permanently awake?
sn201202x_probe() sends a wakeup command to the SPMI device, but this remove
function fails to issue an spmi_command_sleep() command to balance it.
> +static int __maybe_unused sn201202x_resume(struct device *dev)
> +{
> + struct tps6598x *tps = dev_get_drvdata(dev);
> + struct sn201202x *sn = tps_to_sn(tps);
> + int err;
> +
> + err = spmi_command_wakeup(sn->sdev);
> + if (err)
> + return err;
> + if (!wait_for_completion_timeout(&sn->wake_completion, msecs_to_jiffies(100)))
> + return -ETIMEDOUT;
[Severity: High]
Does this completion wait need a prior reinit_completion() call?
The completions for sleep and wake are never re-initialized. If a prior
wait timed out but the interrupt fired later, or a spurious interrupt
incremented the counter, this wait_for_completion_timeout() will observe
done > 0 and return immediately without waiting for the hardware transition.
[Severity: High]
Will this resume operation always timeout and fail if a reset-gpios property
is present?
This function sends an spmi_command_wakeup() and waits for the completion
BEFORE calling tps6598x_resume(). However, if a reset-gpios property is
present, tps6598x_suspend() asserts the hardware reset line.
Because tps6598x_resume() (which de-asserts the reset line) hasn't executed
yet, the device is physically held in hardware reset when the SPMI wake
command is sent, guaranteeing a timeout failure.
> + return tps6598x_resume(dev);
> +}
> +
> +static int __maybe_unused sn201202x_suspend(struct device *dev)
> +{
> + struct tps6598x *tps = dev_get_drvdata(dev);
> + struct sn201202x *sn = tps_to_sn(tps);
> + int err;
> +
> + err = tps6598x_suspend(dev);
> + if (err)
> + return err;
> + err = spmi_command_sleep(sn->sdev);
> + if (err)
> + return err;
[Severity: High]
If spmi_command_sleep() fails, is the core tps6598x controller left
indefinitely disabled?
tps6598x_suspend() disables the main IRQ and cancels active workqueues.
If the subsequent sleep command fails, this function returns an error without
calling tps6598x_resume(). The PM core will abort the system suspend, but it
will not call resume for devices that failed to suspend, leaving the hardware
inoperable.
> + if (!wait_for_completion_timeout(&sn->sleep_completion, msecs_to_jiffies(100)))
> + return -ETIMEDOUT;
[Severity: High]
Does this completion wait need a prior reinit_completion() call?
Similar to the resume path, sn->sleep_completion is not re-initialized before
the wait, which could cause it to incorrectly return immediately if the
completion state is stale.
> + return 0;
> +}
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260725-tipd-ace3-v1-0-73202d36138a@chaosmail.tech?part=3
^ permalink raw reply [flat|nested] 7+ messages in thread