Hi,
v1: https://lore.kernel.org/linux-usb/[email protected]/
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
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 <[email protected]>
Suggested-by: Stan Skowronek <[email protected]>
Signed-off-by: Sven Peter <[email protected]>
---
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
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 <[email protected]>
Acked-by: Rob Herring <[email protected]>
Signed-off-by: Sven Peter <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
> ---
> 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
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 <[email protected]>
>> ---
>> 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
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