[PATCH v2 0/6] usb: typec: tipd: Add Apple M1 support

[PATCH v2 0/6] usb: typec: tipd: Add Apple M1 support

[Sven Peter]

Hi,

v1: https://lore.kernel.org/linux-usb/20210918120934.28252-1-sven@svenpeter.dev/
Thanks again to everyone for your review of the initial version!

This series adds initial support for the Apple CD3217/3218 chip which is also
known as Apple ACE1/2. These chips are used on Apple M1 machines.
They are based on the TI TPS6598x chips with a few differences:

	- The interrupt numbers have been changed
	- The secondary i2c bus and its interrupt controller are connected to the
	  system management controller and must not be disturbed
	- The chip comes up in a low power state and must be booted using the
	  "SPSS" (System Power State Switch maybe) command which is not
	  documented in the TI manual
	- The interrupt mask must be set up explicitly

As suggested bei Heikki, this is now done by creating a separate interrupt handler
for the Apple chips and adding specific setup code to the probe function.
There should be no functional changes for existing TPS chips which is which
I've removed the RFT.

Best,

Sven

Sven Peter (6):
  dt-bindings: usb: tps6598x: Add Apple CD321x compatible
  usb: typec: tipd: Split interrupt handler
  usb: typec: tipd: Add short-circuit for no irqs
  usb: typec: tipd: Add support for Apple CD321X
  usb: typec: tipd: Switch CD321X power state to S0
  usb: typec: tipd: Remove FIXME about testing with I2C_FUNC_I2C

 .../devicetree/bindings/usb/ti,tps6598x.yaml  |   4 +
 drivers/usb/typec/tipd/core.c                 | 229 +++++++++++++++---
 drivers/usb/typec/tipd/tps6598x.h             |  12 +
 drivers/usb/typec/tipd/trace.h                |  23 ++
 4 files changed, 231 insertions(+), 37 deletions(-)

-- 
2.25.1

[PATCH v2 1/6] dt-bindings: usb: tps6598x: Add Apple CD321x compatible

[Sven Peter]

A variant of the TI TPS 6598x Type-C Port Switch and Power Delivery
controller known as Apple CD321x is present on boards with Apple SoCs
such as the M1. Add its compatible to the device tree binding.

Reviewed-by: Alyssa Rosenzweig <alyssa@rosenzweig.io>
Acked-by: Rob Herring <robh@kernel.org>
Signed-off-by: Sven Peter <sven@svenpeter.dev>
---
changes since v1:
  - added robh's ack and alyssa's rb

 Documentation/devicetree/bindings/usb/ti,tps6598x.yaml | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/Documentation/devicetree/bindings/usb/ti,tps6598x.yaml b/Documentation/devicetree/bindings/usb/ti,tps6598x.yaml
index f6819bf2a3b5..a4c53b1f1af3 100644
--- a/Documentation/devicetree/bindings/usb/ti,tps6598x.yaml
+++ b/Documentation/devicetree/bindings/usb/ti,tps6598x.yaml
@@ -12,10 +12,14 @@ maintainers:
 description: |
   Texas Instruments 6598x Type-C Port Switch and Power Delivery controller
 
+  A variant of this controller known as Apple CD321x or Apple ACE is also
+  present on hardware with Apple SoCs such as the M1.
+
 properties:
   compatible:
     enum:
       - ti,tps6598x
+      - apple,cd321x
   reg:
     maxItems: 1
 
-- 
2.25.1

[PATCH v2 2/6] usb: typec: tipd: Split interrupt handler

[Sven Peter]

Split the handlers for the individual interrupts into their own functions
to prepare for adding a second interrupt handler for the Apple CD321x
chips

Signed-off-by: Sven Peter <sven@svenpeter.dev>
---
changes since v1:
  - new commit since Heikki suggested to add a separate irq handler
    for the cd321x variant

 drivers/usb/typec/tipd/core.c | 96 ++++++++++++++++++++++++-----------
 1 file changed, 65 insertions(+), 31 deletions(-)

diff --git a/drivers/usb/typec/tipd/core.c b/drivers/usb/typec/tipd/core.c
index 21b3ae25c76d..162d405baa92 100644
--- a/drivers/usb/typec/tipd/core.c
+++ b/drivers/usb/typec/tipd/core.c
@@ -401,13 +401,69 @@ static const struct typec_operations tps6598x_ops = {
 	.pr_set = tps6598x_pr_set,
 };
 
+static bool tps6598x_read_status(struct tps6598x *tps, u32 *status)
+{
+	int ret;
+
+	ret = tps6598x_read32(tps, TPS_REG_STATUS, status);
+	if (ret) {
+		dev_err(tps->dev, "%s: failed to read status\n", __func__);
+		return false;
+	}
+	trace_tps6598x_status(*status);
+
+	return true;
+}
+
+static bool tps6598x_read_data_status(struct tps6598x *tps)
+{
+	u32 data_status;
+	int ret;
+
+	ret = tps6598x_read32(tps, TPS_REG_DATA_STATUS, &data_status);
+	if (ret < 0) {
+		dev_err(tps->dev, "failed to read data status: %d\n", ret);
+		return false;
+	}
+	trace_tps6598x_data_status(data_status);
+
+	return true;
+}
+
+static bool tps6598x_read_power_status(struct tps6598x *tps)
+{
+	u16 pwr_status;
+	int ret;
+
+	ret = tps6598x_read16(tps, TPS_REG_POWER_STATUS, &pwr_status);
+	if (ret < 0) {
+		dev_err(tps->dev, "failed to read power status: %d\n", ret);
+		return false;
+	}
+	trace_tps6598x_power_status(pwr_status);
+
+	return true;
+}
+
+static void tps6598x_handle_plug_event(struct tps6598x *tps, u32 status)
+{
+	int ret;
+
+	if (status & TPS_STATUS_PLUG_PRESENT) {
+		ret = tps6598x_connect(tps, status);
+		if (ret)
+			dev_err(tps->dev, "failed to register partner\n");
+	} else {
+		tps6598x_disconnect(tps, status);
+	}
+}
+
 static irqreturn_t tps6598x_interrupt(int irq, void *data)
 {
 	struct tps6598x *tps = data;
 	u64 event1;
 	u64 event2;
-	u32 status, data_status;
-	u16 pwr_status;
+	u32 status;
 	int ret;
 
 	mutex_lock(&tps->lock);
@@ -420,42 +476,20 @@ static irqreturn_t tps6598x_interrupt(int irq, void *data)
 	}
 	trace_tps6598x_irq(event1, event2);
 
-	ret = tps6598x_read32(tps, TPS_REG_STATUS, &status);
-	if (ret) {
-		dev_err(tps->dev, "%s: failed to read status\n", __func__);
+	if (!tps6598x_read_status(tps, &status))
 		goto err_clear_ints;
-	}
-	trace_tps6598x_status(status);
 
-	if ((event1 | event2) & TPS_REG_INT_POWER_STATUS_UPDATE) {
-		ret = tps6598x_read16(tps, TPS_REG_POWER_STATUS, &pwr_status);
-		if (ret < 0) {
-			dev_err(tps->dev, "failed to read power status: %d\n", ret);
+	if ((event1 | event2) & TPS_REG_INT_POWER_STATUS_UPDATE)
+		if (!tps6598x_read_power_status(tps))
 			goto err_clear_ints;
-		}
-		trace_tps6598x_power_status(pwr_status);
-	}
 
-	if ((event1 | event2) & TPS_REG_INT_DATA_STATUS_UPDATE) {
-		ret = tps6598x_read32(tps, TPS_REG_DATA_STATUS, &data_status);
-		if (ret < 0) {
-			dev_err(tps->dev, "failed to read data status: %d\n", ret);
+	if ((event1 | event2) & TPS_REG_INT_DATA_STATUS_UPDATE)
+		if (!tps6598x_read_data_status(tps))
 			goto err_clear_ints;
-		}
-		trace_tps6598x_data_status(data_status);
-	}
 
 	/* Handle plug insert or removal */
-	if ((event1 | event2) & TPS_REG_INT_PLUG_EVENT) {
-		if (status & TPS_STATUS_PLUG_PRESENT) {
-			ret = tps6598x_connect(tps, status);
-			if (ret)
-				dev_err(tps->dev,
-					"failed to register partner\n");
-		} else {
-			tps6598x_disconnect(tps, status);
-		}
-	}
+	if ((event1 | event2) & TPS_REG_INT_PLUG_EVENT)
+		tps6598x_handle_plug_event(tps, status);
 
 err_clear_ints:
 	tps6598x_write64(tps, TPS_REG_INT_CLEAR1, event1);
-- 
2.25.1

[PATCH v2 3/6] usb: typec: tipd: Add short-circuit for no irqs

[Sven Peter]

If no interrupts are set in IntEventX directly skip to the end of the
interrupt handler and return IRQ_NONE instead of IRQ_HANDLED.
This possibly allows to detect spurious interrupts if the i2c bus is fast
enough.

Reviewed-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
Signed-off-by: Sven Peter <sven@svenpeter.dev>
---
changes since v1:
  - added Heikki's r-b
  - s/event/(event1 | event2)/

 drivers/usb/typec/tipd/core.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/typec/tipd/core.c b/drivers/usb/typec/tipd/core.c
index 162d405baa92..cd1e37eb8a0c 100644
--- a/drivers/usb/typec/tipd/core.c
+++ b/drivers/usb/typec/tipd/core.c
@@ -476,6 +476,9 @@ static irqreturn_t tps6598x_interrupt(int irq, void *data)
 	}
 	trace_tps6598x_irq(event1, event2);
 
+	if (!(event1 | event2))
+		goto err_unlock;
+
 	if (!tps6598x_read_status(tps, &status))
 		goto err_clear_ints;
 
@@ -498,7 +501,9 @@ static irqreturn_t tps6598x_interrupt(int irq, void *data)
 err_unlock:
 	mutex_unlock(&tps->lock);
 
-	return IRQ_HANDLED;
+	if (event1 | event2)
+		return IRQ_HANDLED;
+	return IRQ_NONE;
 }
 
 static int tps6598x_check_mode(struct tps6598x *tps)
-- 
2.25.1

[PATCH v2 4/6] usb: typec: tipd: Add support for Apple CD321X

[Sven Peter]

Apple CD321x chips are a variant of the TI TPS 6598x chips.
The major differences are the changed interrupt numbers and
the concurrent connection to the SMC which we must not disturb.

Signed-off-by: Sven Peter <sven@svenpeter.dev>
---
changes since v1:
  - new commit since Heikki suggested to just add a separate irq handler

 drivers/usb/typec/tipd/core.c     | 86 ++++++++++++++++++++++++++++++-
 drivers/usb/typec/tipd/tps6598x.h |  6 +++
 drivers/usb/typec/tipd/trace.h    | 23 +++++++++
 3 files changed, 113 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/typec/tipd/core.c b/drivers/usb/typec/tipd/core.c
index cd1e37eb8a0c..6c9c8f19a1cf 100644
--- a/drivers/usb/typec/tipd/core.c
+++ b/drivers/usb/typec/tipd/core.c
@@ -9,6 +9,7 @@
 #include <linux/i2c.h>
 #include <linux/acpi.h>
 #include <linux/module.h>
+#include <linux/of_device.h>
 #include <linux/power_supply.h>
 #include <linux/regmap.h>
 #include <linux/interrupt.h>
@@ -76,6 +77,16 @@ static const char *const modes[] = {
 /* Unrecognized commands will be replaced with "!CMD" */
 #define INVALID_CMD(_cmd_)		(_cmd_ == 0x444d4321)
 
+enum tipd_hw_type {
+	HW_TPS6598X,
+	HW_CD321X
+};
+
+struct tipd_hw {
+	enum tipd_hw_type type;
+	irq_handler_t irq_handler;
+};
+
 struct tps6598x {
 	struct device *dev;
 	struct regmap *regmap;
@@ -458,6 +469,51 @@ static void tps6598x_handle_plug_event(struct tps6598x *tps, u32 status)
 	}
 }
 
+static irqreturn_t cd321x_interrupt(int irq, void *data)
+{
+	struct tps6598x *tps = data;
+	u64 event = 0;
+	u32 status;
+	int ret;
+
+	mutex_lock(&tps->lock);
+
+	ret = tps6598x_read64(tps, TPS_REG_INT_EVENT1, &event);
+	if (ret) {
+		dev_err(tps->dev, "%s: failed to read events\n", __func__);
+		goto err_unlock;
+	}
+	trace_cd321x_irq(event);
+
+	if (!event)
+		goto err_unlock;
+
+	if (!tps6598x_read_status(tps, &status))
+		goto err_clear_ints;
+
+	if (event & APPLE_CD_REG_INT_POWER_STATUS_UPDATE)
+		if (!tps6598x_read_power_status(tps))
+			goto err_clear_ints;
+
+	if (event & APPLE_CD_REG_INT_DATA_STATUS_UPDATE)
+		if (!tps6598x_read_data_status(tps))
+			goto err_clear_ints;
+
+	/* Handle plug insert or removal */
+	if (event & APPLE_CD_REG_INT_PLUG_EVENT)
+		tps6598x_handle_plug_event(tps, status);
+
+err_clear_ints:
+	tps6598x_write64(tps, TPS_REG_INT_CLEAR1, event);
+
+err_unlock:
+	mutex_unlock(&tps->lock);
+
+	if (event)
+		return IRQ_HANDLED;
+	return IRQ_NONE;
+}
+
 static irqreturn_t tps6598x_interrupt(int irq, void *data)
 {
 	struct tps6598x *tps = data;
@@ -615,8 +671,19 @@ static int devm_tps6598_psy_register(struct tps6598x *tps)
 	return PTR_ERR_OR_ZERO(tps->psy);
 }
 
+static const struct tipd_hw ti_tps6598x_data = {
+	.type = HW_TPS6598X,
+	.irq_handler = tps6598x_interrupt,
+};
+
+static const struct tipd_hw apple_cd321x_data = {
+	.type = HW_CD321X,
+	.irq_handler = cd321x_interrupt,
+};
+
 static int tps6598x_probe(struct i2c_client *client)
 {
+	const struct tipd_hw *hw;
 	struct typec_capability typec_cap = { };
 	struct tps6598x *tps;
 	struct fwnode_handle *fwnode;
@@ -629,6 +696,10 @@ static int tps6598x_probe(struct i2c_client *client)
 	if (!tps)
 		return -ENOMEM;
 
+	hw = of_device_get_match_data(&client->dev);
+	if (!hw)
+		hw = &ti_tps6598x_data;
+
 	mutex_init(&tps->lock);
 	tps->dev = &client->dev;
 
@@ -655,6 +726,16 @@ static int tps6598x_probe(struct i2c_client *client)
 	if (ret)
 		return ret;
 
+	if (hw->type == HW_CD321X) {
+		/* CD321X chips have all interrupts masked initially */
+		ret = tps6598x_write64(tps, TPS_REG_INT_MASK1,
+					APPLE_CD_REG_INT_POWER_STATUS_UPDATE |
+					APPLE_CD_REG_INT_DATA_STATUS_UPDATE |
+					APPLE_CD_REG_INT_PLUG_EVENT);
+		if (ret)
+			return ret;
+	}
+
 	ret = tps6598x_read32(tps, TPS_REG_STATUS, &status);
 	if (ret < 0)
 		return ret;
@@ -736,7 +817,7 @@ static int tps6598x_probe(struct i2c_client *client)
 	}
 
 	ret = devm_request_threaded_irq(&client->dev, client->irq, NULL,
-					tps6598x_interrupt,
+					hw->irq_handler,
 					IRQF_SHARED | IRQF_ONESHOT,
 					dev_name(&client->dev), tps);
 	if (ret) {
@@ -769,7 +850,8 @@ static int tps6598x_remove(struct i2c_client *client)
 }
 
 static const struct of_device_id tps6598x_of_match[] = {
-	{ .compatible = "ti,tps6598x", },
+	{ .compatible = "ti,tps6598x", .data = &ti_tps6598x_data },
+	{ .compatible = "apple,cd321x", .data = &apple_cd321x_data },
 	{}
 };
 MODULE_DEVICE_TABLE(of, tps6598x_of_match);
diff --git a/drivers/usb/typec/tipd/tps6598x.h b/drivers/usb/typec/tipd/tps6598x.h
index 003a577be216..e13b16419843 100644
--- a/drivers/usb/typec/tipd/tps6598x.h
+++ b/drivers/usb/typec/tipd/tps6598x.h
@@ -129,6 +129,12 @@
 #define TPS_REG_INT_HARD_RESET				BIT(1)
 #define TPS_REG_INT_PD_SOFT_RESET			BIT(0)
 
+/* Apple-specific TPS_REG_INT_* bits */
+#define APPLE_CD_REG_INT_DATA_STATUS_UPDATE		BIT(10)
+#define APPLE_CD_REG_INT_POWER_STATUS_UPDATE		BIT(9)
+#define APPLE_CD_REG_INT_STATUS_UPDATE			BIT(8)
+#define APPLE_CD_REG_INT_PLUG_EVENT			BIT(1)
+
 /* TPS_REG_POWER_STATUS bits */
 #define TPS_POWER_STATUS_CONNECTION(x)  TPS_FIELD_GET(BIT(0), (x))
 #define TPS_POWER_STATUS_SOURCESINK(x)	TPS_FIELD_GET(BIT(1), (x))
diff --git a/drivers/usb/typec/tipd/trace.h b/drivers/usb/typec/tipd/trace.h
index 5d09d6f78930..12cad1bde7cc 100644
--- a/drivers/usb/typec/tipd/trace.h
+++ b/drivers/usb/typec/tipd/trace.h
@@ -67,6 +67,13 @@
 		{ TPS_REG_INT_USER_VID_ALT_MODE_ATTN_VDM,	"USER_VID_ALT_MODE_ATTN_VDM" }, \
 		{ TPS_REG_INT_USER_VID_ALT_MODE_OTHER_VDM,	"USER_VID_ALT_MODE_OTHER_VDM" })
 
+#define show_cd321x_irq_flags(flags) \
+	__print_flags_u64(flags, "|", \
+		{ APPLE_CD_REG_INT_PLUG_EVENT,			"PLUG_EVENT" }, \
+		{ APPLE_CD_REG_INT_POWER_STATUS_UPDATE,		"POWER_STATUS_UPDATE" }, \
+		{ APPLE_CD_REG_INT_DATA_STATUS_UPDATE,		"DATA_STATUS_UPDATE" }, \
+		{ APPLE_CD_REG_INT_STATUS_UPDATE,		"STATUS_UPDATE" })
+
 #define TPS6598X_STATUS_FLAGS_MASK (GENMASK(31, 0) ^ (TPS_STATUS_CONN_STATE_MASK | \
 						      TPS_STATUS_PP_5V0_SWITCH_MASK | \
 						      TPS_STATUS_PP_HV_SWITCH_MASK | \
@@ -207,6 +214,22 @@ TRACE_EVENT(tps6598x_irq,
 		      show_irq_flags(__entry->event2))
 );
 
+TRACE_EVENT(cd321x_irq,
+	    TP_PROTO(u64 event),
+	    TP_ARGS(event),
+
+	    TP_STRUCT__entry(
+			     __field(u64, event)
+			     ),
+
+	    TP_fast_assign(
+			   __entry->event = event;
+			   ),
+
+	    TP_printk("event=%s",
+		      show_cd321x_irq_flags(__entry->event))
+);
+
 TRACE_EVENT(tps6598x_status,
 	    TP_PROTO(u32 status),
 	    TP_ARGS(status),
-- 
2.25.1

Re: [PATCH v2 4/6] usb: typec: tipd: Add support for Apple CD321X

[Heikki Krogerus]

Hi,

One more question below.

On Thu, Sep 23, 2021 at 08:13:19PM +0200, Sven Peter wrote:
> Apple CD321x chips are a variant of the TI TPS 6598x chips.
> The major differences are the changed interrupt numbers and
> the concurrent connection to the SMC which we must not disturb.
> 
> Signed-off-by: Sven Peter <sven@svenpeter.dev>
> ---
> changes since v1:
>   - new commit since Heikki suggested to just add a separate irq handler
> 
>  drivers/usb/typec/tipd/core.c     | 86 ++++++++++++++++++++++++++++++-
>  drivers/usb/typec/tipd/tps6598x.h |  6 +++
>  drivers/usb/typec/tipd/trace.h    | 23 +++++++++
>  3 files changed, 113 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/usb/typec/tipd/core.c b/drivers/usb/typec/tipd/core.c
> index cd1e37eb8a0c..6c9c8f19a1cf 100644
> --- a/drivers/usb/typec/tipd/core.c
> +++ b/drivers/usb/typec/tipd/core.c
> @@ -9,6 +9,7 @@
>  #include <linux/i2c.h>
>  #include <linux/acpi.h>
>  #include <linux/module.h>
> +#include <linux/of_device.h>
>  #include <linux/power_supply.h>
>  #include <linux/regmap.h>
>  #include <linux/interrupt.h>
> @@ -76,6 +77,16 @@ static const char *const modes[] = {
>  /* Unrecognized commands will be replaced with "!CMD" */
>  #define INVALID_CMD(_cmd_)		(_cmd_ == 0x444d4321)
>  
> +enum tipd_hw_type {
> +	HW_TPS6598X,
> +	HW_CD321X
> +};
> +
> +struct tipd_hw {
> +	enum tipd_hw_type type;
> +	irq_handler_t irq_handler;
> +};
> +
>  struct tps6598x {
>  	struct device *dev;
>  	struct regmap *regmap;
> @@ -458,6 +469,51 @@ static void tps6598x_handle_plug_event(struct tps6598x *tps, u32 status)
>  	}
>  }
>  
> +static irqreturn_t cd321x_interrupt(int irq, void *data)
> +{
> +	struct tps6598x *tps = data;
> +	u64 event = 0;
> +	u32 status;
> +	int ret;
> +
> +	mutex_lock(&tps->lock);
> +
> +	ret = tps6598x_read64(tps, TPS_REG_INT_EVENT1, &event);
> +	if (ret) {
> +		dev_err(tps->dev, "%s: failed to read events\n", __func__);
> +		goto err_unlock;
> +	}
> +	trace_cd321x_irq(event);
> +
> +	if (!event)
> +		goto err_unlock;
> +
> +	if (!tps6598x_read_status(tps, &status))
> +		goto err_clear_ints;
> +
> +	if (event & APPLE_CD_REG_INT_POWER_STATUS_UPDATE)
> +		if (!tps6598x_read_power_status(tps))
> +			goto err_clear_ints;
> +
> +	if (event & APPLE_CD_REG_INT_DATA_STATUS_UPDATE)
> +		if (!tps6598x_read_data_status(tps))
> +			goto err_clear_ints;
> +
> +	/* Handle plug insert or removal */
> +	if (event & APPLE_CD_REG_INT_PLUG_EVENT)
> +		tps6598x_handle_plug_event(tps, status);
> +
> +err_clear_ints:
> +	tps6598x_write64(tps, TPS_REG_INT_CLEAR1, event);
> +
> +err_unlock:
> +	mutex_unlock(&tps->lock);
> +
> +	if (event)
> +		return IRQ_HANDLED;
> +	return IRQ_NONE;
> +}
> +
>  static irqreturn_t tps6598x_interrupt(int irq, void *data)
>  {
>  	struct tps6598x *tps = data;
> @@ -615,8 +671,19 @@ static int devm_tps6598_psy_register(struct tps6598x *tps)
>  	return PTR_ERR_OR_ZERO(tps->psy);
>  }
>  
> +static const struct tipd_hw ti_tps6598x_data = {
> +	.type = HW_TPS6598X,
> +	.irq_handler = tps6598x_interrupt,
> +};
> +
> +static const struct tipd_hw apple_cd321x_data = {
> +	.type = HW_CD321X,
> +	.irq_handler = cd321x_interrupt,
> +};
> +
>  static int tps6598x_probe(struct i2c_client *client)
>  {
> +	const struct tipd_hw *hw;
>  	struct typec_capability typec_cap = { };
>  	struct tps6598x *tps;
>  	struct fwnode_handle *fwnode;
> @@ -629,6 +696,10 @@ static int tps6598x_probe(struct i2c_client *client)
>  	if (!tps)
>  		return -ENOMEM;
>  
> +	hw = of_device_get_match_data(&client->dev);
> +	if (!hw)
> +		hw = &ti_tps6598x_data;
> +
>  	mutex_init(&tps->lock);
>  	tps->dev = &client->dev;
>  
> @@ -655,6 +726,16 @@ static int tps6598x_probe(struct i2c_client *client)
>  	if (ret)
>  		return ret;
>  
> +	if (hw->type == HW_CD321X) {
> +		/* CD321X chips have all interrupts masked initially */
> +		ret = tps6598x_write64(tps, TPS_REG_INT_MASK1,
> +					APPLE_CD_REG_INT_POWER_STATUS_UPDATE |
> +					APPLE_CD_REG_INT_DATA_STATUS_UPDATE |
> +					APPLE_CD_REG_INT_PLUG_EVENT);
> +		if (ret)
> +			return ret;
> +	}
> +
>  	ret = tps6598x_read32(tps, TPS_REG_STATUS, &status);
>  	if (ret < 0)
>  		return ret;
> @@ -736,7 +817,7 @@ static int tps6598x_probe(struct i2c_client *client)
>  	}
>  
>  	ret = devm_request_threaded_irq(&client->dev, client->irq, NULL,
> -					tps6598x_interrupt,
> +					hw->irq_handler,
>  					IRQF_SHARED | IRQF_ONESHOT,
>  					dev_name(&client->dev), tps);

Couldn't you just use the compatible property and local variable here?

        irq_handler_t irq_handler = tps6598x_interrupt;
        struct device_node *np = client->dev.of_node;

        if (np && of_device_is_compatible(np, "apple,cd321x")) {
                /* CD321X chips have all interrupts masked initially */
                ret = tps6598x_write64(tps, TPS_REG_INT_MASK1,
                                        APPLE_CD_REG_INT_POWER_STATUS_UPDATE |
                                        APPLE_CD_REG_INT_DATA_STATUS_UPDATE |
                                        APPLE_CD_REG_INT_PLUG_EVENT);
                if (ret)
                        return ret;

                irq_handler = cd321x_interrupt;
        }

	ret = devm_request_threaded_irq(&client->dev, client->irq, NULL,
					irq_handler,
					IRQF_SHARED | IRQF_ONESHOT,
					dev_name(&client->dev), tps);

I did not go over the whole series yet, so I may have missed
something.

thanks,

-- 
heikki

Re: [PATCH v2 4/6] usb: typec: tipd: Add support for Apple CD321X

[Sven Peter]

Hi,


On Fri, Sep 24, 2021, at 16:41, Heikki Krogerus wrote:
> Hi,
>
> One more question below.
>
> On Thu, Sep 23, 2021 at 08:13:19PM +0200, Sven Peter wrote:
>> Apple CD321x chips are a variant of the TI TPS 6598x chips.
>> The major differences are the changed interrupt numbers and
>> the concurrent connection to the SMC which we must not disturb.
>> 
>> Signed-off-by: Sven Peter <sven@svenpeter.dev>
>> ---
>> changes since v1:
>>   - new commit since Heikki suggested to just add a separate irq handler
>> 
>>  drivers/usb/typec/tipd/core.c     | 86 ++++++++++++++++++++++++++++++-
>>  drivers/usb/typec/tipd/tps6598x.h |  6 +++
>>  drivers/usb/typec/tipd/trace.h    | 23 +++++++++
>>  3 files changed, 113 insertions(+), 2 deletions(-)
>> 
>> diff --git a/drivers/usb/typec/tipd/core.c b/drivers/usb/typec/tipd/core.c
>> index cd1e37eb8a0c..6c9c8f19a1cf 100644
>> --- a/drivers/usb/typec/tipd/core.c
>> +++ b/drivers/usb/typec/tipd/core.c
>> @@ -9,6 +9,7 @@
>>  #include <linux/i2c.h>
>>  #include <linux/acpi.h>
>>  #include <linux/module.h>
>> +#include <linux/of_device.h>
>>  #include <linux/power_supply.h>
>>  #include <linux/regmap.h>
>>  #include <linux/interrupt.h>
>> @@ -76,6 +77,16 @@ static const char *const modes[] = {
>>  /* Unrecognized commands will be replaced with "!CMD" */
>>  #define INVALID_CMD(_cmd_)		(_cmd_ == 0x444d4321)
>>  
>> +enum tipd_hw_type {
>> +	HW_TPS6598X,
>> +	HW_CD321X
>> +};
>> +
>> +struct tipd_hw {
>> +	enum tipd_hw_type type;
>> +	irq_handler_t irq_handler;
>> +};
>> +
>>  struct tps6598x {
>>  	struct device *dev;
>>  	struct regmap *regmap;
>> @@ -458,6 +469,51 @@ static void tps6598x_handle_plug_event(struct tps6598x *tps, u32 status)
>>  	}
>>  }
>>  
>> +static irqreturn_t cd321x_interrupt(int irq, void *data)
>> +{
>> +	struct tps6598x *tps = data;
>> +	u64 event = 0;
>> +	u32 status;
>> +	int ret;
>> +
>> +	mutex_lock(&tps->lock);
>> +
>> +	ret = tps6598x_read64(tps, TPS_REG_INT_EVENT1, &event);
>> +	if (ret) {
>> +		dev_err(tps->dev, "%s: failed to read events\n", __func__);
>> +		goto err_unlock;
>> +	}
>> +	trace_cd321x_irq(event);
>> +
>> +	if (!event)
>> +		goto err_unlock;
>> +
>> +	if (!tps6598x_read_status(tps, &status))
>> +		goto err_clear_ints;
>> +
>> +	if (event & APPLE_CD_REG_INT_POWER_STATUS_UPDATE)
>> +		if (!tps6598x_read_power_status(tps))
>> +			goto err_clear_ints;
>> +
>> +	if (event & APPLE_CD_REG_INT_DATA_STATUS_UPDATE)
>> +		if (!tps6598x_read_data_status(tps))
>> +			goto err_clear_ints;
>> +
>> +	/* Handle plug insert or removal */
>> +	if (event & APPLE_CD_REG_INT_PLUG_EVENT)
>> +		tps6598x_handle_plug_event(tps, status);
>> +
>> +err_clear_ints:
>> +	tps6598x_write64(tps, TPS_REG_INT_CLEAR1, event);
>> +
>> +err_unlock:
>> +	mutex_unlock(&tps->lock);
>> +
>> +	if (event)
>> +		return IRQ_HANDLED;
>> +	return IRQ_NONE;
>> +}
>> +
>>  static irqreturn_t tps6598x_interrupt(int irq, void *data)
>>  {
>>  	struct tps6598x *tps = data;
>> @@ -615,8 +671,19 @@ static int devm_tps6598_psy_register(struct tps6598x *tps)
>>  	return PTR_ERR_OR_ZERO(tps->psy);
>>  }
>>  
>> +static const struct tipd_hw ti_tps6598x_data = {
>> +	.type = HW_TPS6598X,
>> +	.irq_handler = tps6598x_interrupt,
>> +};
>> +
>> +static const struct tipd_hw apple_cd321x_data = {
>> +	.type = HW_CD321X,
>> +	.irq_handler = cd321x_interrupt,
>> +};
>> +
>>  static int tps6598x_probe(struct i2c_client *client)
>>  {
>> +	const struct tipd_hw *hw;
>>  	struct typec_capability typec_cap = { };
>>  	struct tps6598x *tps;
>>  	struct fwnode_handle *fwnode;
>> @@ -629,6 +696,10 @@ static int tps6598x_probe(struct i2c_client *client)
>>  	if (!tps)
>>  		return -ENOMEM;
>>  
>> +	hw = of_device_get_match_data(&client->dev);
>> +	if (!hw)
>> +		hw = &ti_tps6598x_data;
>> +
>>  	mutex_init(&tps->lock);
>>  	tps->dev = &client->dev;
>>  
>> @@ -655,6 +726,16 @@ static int tps6598x_probe(struct i2c_client *client)
>>  	if (ret)
>>  		return ret;
>>  
>> +	if (hw->type == HW_CD321X) {
>> +		/* CD321X chips have all interrupts masked initially */
>> +		ret = tps6598x_write64(tps, TPS_REG_INT_MASK1,
>> +					APPLE_CD_REG_INT_POWER_STATUS_UPDATE |
>> +					APPLE_CD_REG_INT_DATA_STATUS_UPDATE |
>> +					APPLE_CD_REG_INT_PLUG_EVENT);
>> +		if (ret)
>> +			return ret;
>> +	}
>> +
>>  	ret = tps6598x_read32(tps, TPS_REG_STATUS, &status);
>>  	if (ret < 0)
>>  		return ret;
>> @@ -736,7 +817,7 @@ static int tps6598x_probe(struct i2c_client *client)
>>  	}
>>  
>>  	ret = devm_request_threaded_irq(&client->dev, client->irq, NULL,
>> -					tps6598x_interrupt,
>> +					hw->irq_handler,
>>  					IRQF_SHARED | IRQF_ONESHOT,
>>  					dev_name(&client->dev), tps);
>
> Couldn't you just use the compatible property and local variable here?
>
>         irq_handler_t irq_handler = tps6598x_interrupt;
>         struct device_node *np = client->dev.of_node;
>
>         if (np && of_device_is_compatible(np, "apple,cd321x")) {
>                 /* CD321X chips have all interrupts masked initially */
>                 ret = tps6598x_write64(tps, TPS_REG_INT_MASK1,
>                                         APPLE_CD_REG_INT_POWER_STATUS_UPDATE |
>                                         APPLE_CD_REG_INT_DATA_STATUS_UPDATE |
>                                         APPLE_CD_REG_INT_PLUG_EVENT);
>                 if (ret)
>                         return ret;
>
>                 irq_handler = cd321x_interrupt;
>         }
>
> 	ret = devm_request_threaded_irq(&client->dev, client->irq, NULL,
> 					irq_handler,
> 					IRQF_SHARED | IRQF_ONESHOT,
> 					dev_name(&client->dev), tps);
>
> I did not go over the whole series yet, so I may have missed
> something.

Sure, that will work and get rid of the slightly awkward and redundant interrupt/enum
combination. I'll wait for your comments for the rest of the series and do that for v3 then :)


Thanks,


Sven

Re: [PATCH v2 4/6] usb: typec: tipd: Add support for Apple CD321X

[Heikki Krogerus]

Hi Sven,

On Fri, Sep 24, 2021 at 04:58:52PM +0200, Sven Peter wrote:
> > Couldn't you just use the compatible property and local variable here?
> >
> >         irq_handler_t irq_handler = tps6598x_interrupt;
> >         struct device_node *np = client->dev.of_node;
> >
> >         if (np && of_device_is_compatible(np, "apple,cd321x")) {
> >                 /* CD321X chips have all interrupts masked initially */
> >                 ret = tps6598x_write64(tps, TPS_REG_INT_MASK1,
> >                                         APPLE_CD_REG_INT_POWER_STATUS_UPDATE |
> >                                         APPLE_CD_REG_INT_DATA_STATUS_UPDATE |
> >                                         APPLE_CD_REG_INT_PLUG_EVENT);
> >                 if (ret)
> >                         return ret;
> >
> >                 irq_handler = cd321x_interrupt;
> >         }
> >
> > 	ret = devm_request_threaded_irq(&client->dev, client->irq, NULL,
> > 					irq_handler,
> > 					IRQF_SHARED | IRQF_ONESHOT,
> > 					dev_name(&client->dev), tps);
> >
> > I did not go over the whole series yet, so I may have missed
> > something.
> 
> Sure, that will work and get rid of the slightly awkward and redundant interrupt/enum
> combination. I'll wait for your comments for the rest of the series and do that for v3 then :)

The rest of the series look OK to me.

thanks,

-- 
heikki

[PATCH v2 5/6] usb: typec: tipd: Switch CD321X power state to S0

[Sven Peter]

The Apple CD321x comes up in a low-power state after boot. Usually, the
bootloader will already power it up to S0 but let's do it here as well
in case that didn't happen.

Reviewed-by: Alyssa Rosenzweig <alyssa@rosenzweig.io>
Suggested-by: Stan Skowronek <stan@corellium.com>
Signed-off-by: Sven Peter <sven@svenpeter.dev>
---
changes since v1:
  - dropped the supports_spss flag and only call this for the Apple chip
  - added Alyssa's r-b

 drivers/usb/typec/tipd/core.c     | 37 +++++++++++++++++++++++++++++++
 drivers/usb/typec/tipd/tps6598x.h |  6 +++++
 2 files changed, 43 insertions(+)

diff --git a/drivers/usb/typec/tipd/core.c b/drivers/usb/typec/tipd/core.c
index 6c9c8f19a1cf..20d9f89208ff 100644
--- a/drivers/usb/typec/tipd/core.c
+++ b/drivers/usb/typec/tipd/core.c
@@ -30,6 +30,7 @@
 #define TPS_REG_INT_MASK2		0x17
 #define TPS_REG_INT_CLEAR1		0x18
 #define TPS_REG_INT_CLEAR2		0x19
+#define TPS_REG_SYSTEM_POWER_STATE	0x20
 #define TPS_REG_STATUS			0x1a
 #define TPS_REG_SYSTEM_CONF		0x28
 #define TPS_REG_CTRL_CONF		0x29
@@ -159,6 +160,11 @@ static int tps6598x_block_write(struct tps6598x *tps, u8 reg,
 	return regmap_raw_write(tps->regmap, reg, data, sizeof(data));
 }
 
+static inline int tps6598x_read8(struct tps6598x *tps, u8 reg, u8 *val)
+{
+	return tps6598x_block_read(tps, reg, val, sizeof(u8));
+}
+
 static inline int tps6598x_read16(struct tps6598x *tps, u8 reg, u16 *val)
 {
 	return tps6598x_block_read(tps, reg, val, sizeof(u16));
@@ -642,6 +648,32 @@ static int tps6598x_psy_get_prop(struct power_supply *psy,
 	return ret;
 }
 
+static int cd321x_switch_power_state(struct tps6598x *tps, u8 target_state)
+{
+	u8 state;
+	int ret;
+
+	ret = tps6598x_read8(tps, TPS_REG_SYSTEM_POWER_STATE, &state);
+	if (ret)
+		return ret;
+
+	if (state == target_state)
+		return 0;
+
+	ret = tps6598x_exec_cmd(tps, "SPSS", sizeof(u8), &target_state, 0, NULL);
+	if (ret)
+		return ret;
+
+	ret = tps6598x_read8(tps, TPS_REG_SYSTEM_POWER_STATE, &state);
+	if (ret)
+		return ret;
+
+	if (state != target_state)
+		return -EINVAL;
+
+	return 0;
+}
+
 static int devm_tps6598_psy_register(struct tps6598x *tps)
 {
 	struct power_supply_config psy_cfg = {};
@@ -727,6 +759,11 @@ static int tps6598x_probe(struct i2c_client *client)
 		return ret;
 
 	if (hw->type == HW_CD321X) {
+		/* Switch CD321X chips to the correct system power state */
+		ret = cd321x_switch_power_state(tps, TPS_SYSTEM_POWER_STATE_S0);
+		if (ret)
+			return ret;
+
 		/* CD321X chips have all interrupts masked initially */
 		ret = tps6598x_write64(tps, TPS_REG_INT_MASK1,
 					APPLE_CD_REG_INT_POWER_STATUS_UPDATE |
diff --git a/drivers/usb/typec/tipd/tps6598x.h b/drivers/usb/typec/tipd/tps6598x.h
index e13b16419843..3dae84c524fb 100644
--- a/drivers/usb/typec/tipd/tps6598x.h
+++ b/drivers/usb/typec/tipd/tps6598x.h
@@ -135,6 +135,12 @@
 #define APPLE_CD_REG_INT_STATUS_UPDATE			BIT(8)
 #define APPLE_CD_REG_INT_PLUG_EVENT			BIT(1)
 
+/* TPS_REG_SYSTEM_POWER_STATE states */
+#define TPS_SYSTEM_POWER_STATE_S0	0x00
+#define TPS_SYSTEM_POWER_STATE_S3	0x03
+#define TPS_SYSTEM_POWER_STATE_S4	0x04
+#define TPS_SYSTEM_POWER_STATE_S5	0x05
+
 /* TPS_REG_POWER_STATUS bits */
 #define TPS_POWER_STATUS_CONNECTION(x)  TPS_FIELD_GET(BIT(0), (x))
 #define TPS_POWER_STATUS_SOURCESINK(x)	TPS_FIELD_GET(BIT(1), (x))
-- 
2.25.1

[PATCH v2 6/6] usb: typec: tipd: Remove FIXME about testing with I2C_FUNC_I2C

[Sven Peter]

The Apple i2c bus uses I2C_FUNC_I2C and I've tested this quite
extensivly in the past days. Remove the FIXME about that testing :-)

Reviewed-by: Alyssa Rosenzweig <alyssa@rosenzweig.io>
Reviewed-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
Signed-off-by: Sven Peter <sven@svenpeter.dev>
---
changes since v1:
  - added r-b tags

 drivers/usb/typec/tipd/core.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/drivers/usb/typec/tipd/core.c b/drivers/usb/typec/tipd/core.c
index 20d9f89208ff..51cebb41884c 100644
--- a/drivers/usb/typec/tipd/core.c
+++ b/drivers/usb/typec/tipd/core.c
@@ -746,9 +746,6 @@ static int tps6598x_probe(struct i2c_client *client)
 	/*
 	 * Checking can the adapter handle SMBus protocol. If it can not, the
 	 * driver needs to take care of block reads separately.
-	 *
-	 * FIXME: Testing with I2C_FUNC_I2C. regmap-i2c uses I2C protocol
-	 * unconditionally if the adapter has I2C_FUNC_I2C set.
 	 */
 	if (i2c_check_functionality(client->adapter, I2C_FUNC_I2C))
 		tps->i2c_protocol = true;
-- 
2.25.1