Re: [PATCH RESEND 2/2] usb: typec: tipd: fix event checking for tps6598x

[Heikki Krogerus]

On Thu, Mar 28, 2024 at 05:55:52PM +0100, Javier Carrasco wrote:
> The current interrupt service routine of the tps6598x only reads the
> first 64 bits of the INT_EVENT1 and INT_EVENT2 registers, which means
> that any event above that range will be ignored, leaving interrupts
> unattended. Moreover, those events will not be cleared, and the device
> will keep the interrupt enabled.
> 
> This issue has been observed while attempting to load patches, and the
> 'ReadyForPatch' field (bit 81) of INT_EVENT1 was set.
> 
> Read the complete INT_EVENT registers to handle all interrupts generated
> by the device in a similar fashion to what is already done for the
> tps25750.
> 
> Fixes: 0a4c005bd171 ("usb: typec: driver for TI TPS6598x USB Power Delivery controllers")
> Cc: stable@xxxxxxxxxxxxxxx
> Signed-off-by: Javier Carrasco <javier.carrasco@xxxxxxxxxxxxxx>
> ---
>  drivers/usb/typec/tipd/core.c | 31 ++++++++++++++++++-------------
>  1 file changed, 18 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/usb/typec/tipd/core.c b/drivers/usb/typec/tipd/core.c
> index 7c2f01344860..308748d6cae6 100644
> --- a/drivers/usb/typec/tipd/core.c
> +++ b/drivers/usb/typec/tipd/core.c
> @@ -637,48 +637,53 @@ static irqreturn_t tps25750_interrupt(int irq, void *data)
>  static irqreturn_t tps6598x_interrupt(int irq, void *data)
>  {
>  	struct tps6598x *tps = data;
> -	u64 event1 = 0;
> -	u64 event2 = 0;
> +	u64 event1[2] = { };
> +	u64 event2[2] = { };
>  	u32 status;
>  	int ret;
>  
>  	mutex_lock(&tps->lock);
>  
> -	ret = tps6598x_read64(tps, TPS_REG_INT_EVENT1, &event1);
> -	ret |= tps6598x_read64(tps, TPS_REG_INT_EVENT2, &event2);
> +	ret = tps6598x_block_read(tps, TPS_REG_INT_EVENT1, event1, 11);

This is not going to work with the older TI PD controllers.

The lenght of these registers is 8 bytes on the older TI PD
controllers (TPS65981, TPS65982, etc.). I think we need to split this
function.

>  	if (ret) {
> -		dev_err(tps->dev, "%s: failed to read events\n", __func__);
> +		dev_err(tps->dev, "%s: failed to read event1\n", __func__);
>  		goto err_unlock;
>  	}
> -	trace_tps6598x_irq(event1, event2);
> +	ret = tps6598x_block_read(tps, TPS_REG_INT_EVENT2, event2, 11);
> +	if (ret) {
> +		dev_err(tps->dev, "%s: failed to read event2\n", __func__);
> +		goto err_unlock;
> +	}
> +	trace_tps6598x_irq(event1[0], event2[0]);
>  
> -	if (!(event1 | event2))
> +	if (!(event1[0] | event1[1] | event2[0] | event2[1]))
>  		goto err_unlock;
>  
>  	if (!tps6598x_read_status(tps, &status))
>  		goto err_clear_ints;
>  
> -	if ((event1 | event2) & TPS_REG_INT_POWER_STATUS_UPDATE)
> +	if ((event1[0] | event2[0]) & TPS_REG_INT_POWER_STATUS_UPDATE)
>  		if (!tps6598x_read_power_status(tps))
>  			goto err_clear_ints;
>  
> -	if ((event1 | event2) & TPS_REG_INT_DATA_STATUS_UPDATE)
> +	if ((event1[0] | event2[0]) & TPS_REG_INT_DATA_STATUS_UPDATE)
>  		if (!tps6598x_read_data_status(tps))
>  			goto err_clear_ints;
>  
>  	/* Handle plug insert or removal */
> -	if ((event1 | event2) & TPS_REG_INT_PLUG_EVENT)
> +	if ((event1[0] | event2[0]) & TPS_REG_INT_PLUG_EVENT)
>  		tps6598x_handle_plug_event(tps, status);
>  
>  err_clear_ints:
> -	tps6598x_write64(tps, TPS_REG_INT_CLEAR1, event1);
> -	tps6598x_write64(tps, TPS_REG_INT_CLEAR2, event2);
> +	tps6598x_block_write(tps, TPS_REG_INT_CLEAR1, event1, 11);
> +	tps6598x_block_write(tps, TPS_REG_INT_CLEAR2, event2, 11);
>  
>  err_unlock:
>  	mutex_unlock(&tps->lock);
>  
> -	if (event1 | event2)
> +	if (event1[0] | event1[1] | event2[0] | event2[1])
>  		return IRQ_HANDLED;
> +
>  	return IRQ_NONE;
>  }
>  
> 
> -- 
> 2.40.1

-- 
heikki