[PATCH 0/3] usb: typec: tipd: Add sn201202x (ACE3) support

[PATCH 0/3] usb: typec: tipd: Add sn201202x (ACE3) support

[Sasha Finkelstein]

Add support for sn201202x (a.k.a ACE3), an Apple-specific tps6598x
variant that wraps the tps6598x core register set with a SPMI based
regmap-like protocol, first appearing in Apple M3 devices. This series
first makes the core parts bus-agnostic, and then adds the new SPMI
interface as a custom regmap implementation.

Signed-off-by: Sasha Finkelstein <k@chaosmail.tech>
---
Alyssa Milburn (1):
      usb: typec: tipd: Factor out i2c specifics

Sasha Finkelstein (2):
      dt-bindings: usb: tps6598x: Add sn201202x/ACE3
      usb: typec: tipd: Add sn201202x support

 Documentation/devicetree/bindings/usb/apple,sn201202x.yaml |  78 +++++++++++++++++++++++++++++++
 MAINTAINERS                                                |   2 +
 drivers/usb/typec/tipd/Kconfig                             |  14 ++++++
 drivers/usb/typec/tipd/Makefile                            |  12 +++--
 drivers/usb/typec/tipd/{core.c => core.h}                  | 119 +++++++++++++++--------------------------------
 drivers/usb/typec/tipd/i2c.c                               |  86 ++++++++++++++++++++++++++++++++++
 drivers/usb/typec/tipd/spmi.c                              | 297 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 7 files changed, 523 insertions(+), 85 deletions(-)
---
base-commit: 0ce37745d4bfbc493f718169c3974898ffec8ee7
change-id: 20260725-tipd-ace3-090bf570ef59

Best regards,
--  
Sasha Finkelstein <k@chaosmail.tech>

[PATCH 1/3] dt-bindings: usb: tps6598x: Add sn201202x/ACE3

[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

[PATCH 2/3] usb: typec: tipd: Factor out i2c specifics

[Sasha Finkelstein]

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

[PATCH 3/3] usb: typec: tipd: Add sn201202x support

[Sasha Finkelstein]

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, &regmap_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

Re: [PATCH 2/3] usb: typec: tipd: Factor out i2c specifics

[Heikki Krogerus]

Hi,

On Sat, Jul 25, 2026 at 06:20:57PM +0200, Sasha Finkelstein wrote:
> 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 ++++++++++++++++++++++++++++---------------------------------------------------------------------------------

That has to be a mistake, right? You don't move code into a header
like that.

>  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

Now I'm worried. Are you really planning on putting the code into the
header?

You should keep the core independent of the glue drivers, even if core
is just a library. I'm expecting to see something like this here:

obj-$(CONFIG_TYPEC_TPS6598X)	+= tps6598x.o
tps6598x-y			:= core.o
+tps6598x-$(CONFIG_I2C)		+= 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;
> +}

You should not need anything like that.

>  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)

So you really were planning on putting this into a header :(

The header will have only the definitions and prototypes that the
glue drivers such as that i2c.c need. The implementation of this
functions stays in core.c. It just not static anymore.
You should have something like this in core.c:

-static int tps6598x_probe(struct i2c_client *client)
+int tipd_init(struct tps6598x *tps)
{
        ...
}
+EXPORT_SYMBOL_GPL(tipd_init);

>  {
> -	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));

The PM callbacks should be in the glue drivers. You can provide
separate suspend and resume functions here that the glue drivers can
call from their PM callbacks:

int tipd_suspend(struct tps6598x *tps)
{
        ...
}
EXPORT_SYMBOL_GPL(tipd_suspend);

int tipd_resume(struct tps6598x *tps)
{
        ...
}
EXPORT_SYMBOL_GPL(tipd_suspend);

> @@ -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>
> + */

This driver is not coming from me.

> +#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");

You need to change that too. The glue drivers will be separate
modules and the core will stay as its own module.

Thanks,

-- 
heikki

Re: [PATCH 2/3] usb: typec: tipd: Factor out i2c specifics

[Sasha Finkelstein]

On Jul 27, 2026, at 14:56, Heikki Krogerus <heikki.krogerus@linux.intel.com> wrote:
> 
> Hi,
> 
> On Sat, Jul 25, 2026 at 06:20:57PM +0200, Sasha Finkelstein wrote:
>> 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 ++++++++++++++++++++++++++++---------------------------------------------------------------------------------
> 
> That has to be a mistake, right? You don't move code into a header
> like that.

Yes, it was a bad idea, already fixed in a v2 that will be sent in the
near future.

>> +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;
>> +}
> 
> You should not need anything like that.

This should make more sense together with the following patch, as it
can get the tps6598x from either the i2c or spmi backend.

>> --- /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>
>> + */
> 
> This driver is not coming from me.

I kept the attribution as both core and i2c are your driver, but split
into two. Should I have done something else?

> 
> Thanks,
> 
> -- 
> heikki

Re: [PATCH 2/3] usb: typec: tipd: Factor out i2c specifics

[Heikki Krogerus]

On Mon, Jul 27, 2026 at 03:05:35PM +0200, Sasha Finkelstein wrote:
> On Jul 27, 2026, at 14:56, Heikki Krogerus <heikki.krogerus@linux.intel.com> wrote:
> > 
> > Hi,
> > 
> > On Sat, Jul 25, 2026 at 06:20:57PM +0200, Sasha Finkelstein wrote:
> >> 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 ++++++++++++++++++++++++++++---------------------------------------------------------------------------------
> > 
> > That has to be a mistake, right? You don't move code into a header
> > like that.
> 
> Yes, it was a bad idea, already fixed in a v2 that will be sent in the
> near future.
> 
> >> +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;
> >> +}
> > 
> > You should not need anything like that.
> 
> This should make more sense together with the following patch, as it
> can get the tps6598x from either the i2c or spmi backend.

You would only need this in the PM callbacks, and those you need to
keep in the glue drivers for i2c and spmi. The core.c can export
common functions for suspend and resume like I told you.

> >> --- /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>
> >> + */
> > 
> > This driver is not coming from me.
> 
> I kept the attribution as both core and i2c are your driver, but split
> into two. Should I have done something else?

This will not be the same module as the core. You will have separate
modules for the core and for both glue driver.

Note. You can also refactor the core a little so that by default it
works with i2c, but it also exports the init/probe function so that it
can be used as a library with spmi. In that way you don't need to add
the i2c.c at all. But the you will depend on i2c also when you use the
spmi.

Thanks,

-- 
heikki