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

["Sven Peter" <sven@svenpeter.dev>]

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