[PATCH] extcon: usbc-tusb320: Call the Type-C IRQ handler only if a port is registered

[PATCH] extcon: usbc-tusb320: Call the Type-C IRQ handler only if a port is registered

[Yassine Oudjana]

From: Yassine Oudjana <y.oudjana@protonmail.com>

Commit bf7571c00dca ("extcon: usbc-tusb320: Add USB TYPE-C support")
added an optional Type-C interface to the driver but missed to check
if it is in use when calling the IRQ handler. This causes an oops on
devices currently using the old extcon interface. Check if a Type-C
port is registered before calling the Type-C IRQ handler.

Fixes: bf7571c00dca ("extcon: usbc-tusb320: Add USB TYPE-C support")
Signed-off-by: Yassine Oudjana <y.oudjana@protonmail.com>
---
 drivers/extcon/extcon-usbc-tusb320.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/drivers/extcon/extcon-usbc-tusb320.c b/drivers/extcon/extcon-usbc-tusb320.c
index 41041ff0fadb..037bc11b2a48 100644
--- a/drivers/extcon/extcon-usbc-tusb320.c
+++ b/drivers/extcon/extcon-usbc-tusb320.c
@@ -327,7 +327,14 @@ static irqreturn_t tusb320_irq_handler(int irq, void *dev_id)
 		return IRQ_NONE;
 
 	tusb320_extcon_irq_handler(priv, reg);
-	tusb320_typec_irq_handler(priv, reg);
+
+	/*
+	 * Type-C support is optional for backward compatibility.
+	 * Only call the Type-C handler if a port had been registered
+	 * previously.
+	 */
+	if (priv->port)
+		tusb320_typec_irq_handler(priv, reg);
 
 	regmap_write(priv->regmap, TUSB320_REG9, reg);
 
-- 
2.38.1

Re: [PATCH] extcon: usbc-tusb320: Call the Type-C IRQ handler only if a port is registered

[Marek Vasut]

On 11/7/22 15:48, Yassine Oudjana wrote:
> From: Yassine Oudjana <y.oudjana@protonmail.com>
> 
> Commit bf7571c00dca ("extcon: usbc-tusb320: Add USB TYPE-C support")
> added an optional Type-C interface to the driver but missed to check
> if it is in use when calling the IRQ handler. This causes an oops on
> devices currently using the old extcon interface. Check if a Type-C
> port is registered before calling the Type-C IRQ handler.
> 
> Fixes: bf7571c00dca ("extcon: usbc-tusb320: Add USB TYPE-C support")
> Signed-off-by: Yassine Oudjana <y.oudjana@protonmail.com>
> ---
>   drivers/extcon/extcon-usbc-tusb320.c | 9 ++++++++-
>   1 file changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/extcon/extcon-usbc-tusb320.c b/drivers/extcon/extcon-usbc-tusb320.c
> index 41041ff0fadb..037bc11b2a48 100644
> --- a/drivers/extcon/extcon-usbc-tusb320.c
> +++ b/drivers/extcon/extcon-usbc-tusb320.c
> @@ -327,7 +327,14 @@ static irqreturn_t tusb320_irq_handler(int irq, void *dev_id)
>   		return IRQ_NONE;
>   
>   	tusb320_extcon_irq_handler(priv, reg);
> -	tusb320_typec_irq_handler(priv, reg);
> +
> +	/*
> +	 * Type-C support is optional for backward compatibility.

It's the other way around, extcon is the legacy, type-c is the new, right ?

> +	 * Only call the Type-C handler if a port had been registered
> +	 * previously.
> +	 */
> +	if (priv->port)
> +		tusb320_typec_irq_handler(priv, reg);
>   
>   	regmap_write(priv->regmap, TUSB320_REG9, reg);

Reviewed-by: Marek Vasut <marex@denx.de>

Thanks!

Re: [PATCH] extcon: usbc-tusb320: Call the Type-C IRQ handler only if a port is registered

[Yassine Oudjana]

On Mon, Nov 7 2022 at 15:51:55 +01:00:00, Marek Vasut <marex@denx.de> 
wrote:
> On 11/7/22 15:48, Yassine Oudjana wrote:
>> From: Yassine Oudjana <y.oudjana@protonmail.com>
>> 
>> Commit bf7571c00dca ("extcon: usbc-tusb320: Add USB TYPE-C support")
>> added an optional Type-C interface to the driver but missed to check
>> if it is in use when calling the IRQ handler. This causes an oops on
>> devices currently using the old extcon interface. Check if a Type-C
>> port is registered before calling the Type-C IRQ handler.
>> 
>> Fixes: bf7571c00dca ("extcon: usbc-tusb320: Add USB TYPE-C support")
>> Signed-off-by: Yassine Oudjana <y.oudjana@protonmail.com>
>> ---
>>   drivers/extcon/extcon-usbc-tusb320.c | 9 ++++++++-
>>   1 file changed, 8 insertions(+), 1 deletion(-)
>> 
>> diff --git a/drivers/extcon/extcon-usbc-tusb320.c 
>> b/drivers/extcon/extcon-usbc-tusb320.c
>> index 41041ff0fadb..037bc11b2a48 100644
>> --- a/drivers/extcon/extcon-usbc-tusb320.c
>> +++ b/drivers/extcon/extcon-usbc-tusb320.c
>> @@ -327,7 +327,14 @@ static irqreturn_t tusb320_irq_handler(int irq, 
>> void *dev_id)
>>   		return IRQ_NONE;
>>   \x7f  	tusb320_extcon_irq_handler(priv, reg);
>> -	tusb320_typec_irq_handler(priv, reg);
>> +
>> +	/*
>> +	 * Type-C support is optional for backward compatibility.
> 
> It's the other way around, extcon is the legacy, type-c is the new, 
> right ?

Type-C is the new one, yes. This comment is somewhat similar to the one 
in tusb320_typec_probe():

/* The Type-C connector is optional, for backward compatibility. */

Perhaps a better way to say this in both comments would be "to 
maintain" instead of "for".

> 
>> +	 * Only call the Type-C handler if a port had been registered
>> +	 * previously.
>> +	 */
>> +	if (priv->port)
>> +		tusb320_typec_irq_handler(priv, reg);
>>   \x7f  	regmap_write(priv->regmap, TUSB320_REG9, reg);
> 
> Reviewed-by: Marek Vasut <marex@denx.de>
> 
> Thanks!

Thanks for the quick review!
Yassine

Re: [PATCH] extcon: usbc-tusb320: Call the Type-C IRQ handler only if a port is registered

[Marek Vasut]

On 11/7/22 16:02, Yassine Oudjana wrote:
> 
> On Mon, Nov 7 2022 at 15:51:55 +01:00:00, Marek Vasut <marex@denx.de> 
> wrote:
>> On 11/7/22 15:48, Yassine Oudjana wrote:
>>> From: Yassine Oudjana <y.oudjana@protonmail.com>
>>>
>>> Commit bf7571c00dca ("extcon: usbc-tusb320: Add USB TYPE-C support")
>>> added an optional Type-C interface to the driver but missed to check
>>> if it is in use when calling the IRQ handler. This causes an oops on
>>> devices currently using the old extcon interface. Check if a Type-C
>>> port is registered before calling the Type-C IRQ handler.
>>>
>>> Fixes: bf7571c00dca ("extcon: usbc-tusb320: Add USB TYPE-C support")
>>> Signed-off-by: Yassine Oudjana <y.oudjana@protonmail.com>
>>> ---
>>>   drivers/extcon/extcon-usbc-tusb320.c | 9 ++++++++-
>>>   1 file changed, 8 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/extcon/extcon-usbc-tusb320.c 
>>> b/drivers/extcon/extcon-usbc-tusb320.c
>>> index 41041ff0fadb..037bc11b2a48 100644
>>> --- a/drivers/extcon/extcon-usbc-tusb320.c
>>> +++ b/drivers/extcon/extcon-usbc-tusb320.c
>>> @@ -327,7 +327,14 @@ static irqreturn_t tusb320_irq_handler(int irq, 
>>> void *dev_id)
>>>           return IRQ_NONE;
>>>   \x7f      tusb320_extcon_irq_handler(priv, reg);
>>> -    tusb320_typec_irq_handler(priv, reg);
>>> +
>>> +    /*
>>> +     * Type-C support is optional for backward compatibility.
>>
>> It's the other way around, extcon is the legacy, type-c is the new, 
>> right ?
> 
> Type-C is the new one, yes. This comment is somewhat similar to the one 
> in tusb320_typec_probe():
> 
> /* The Type-C connector is optional, for backward compatibility. */

Ahhh, The Type-C connector support is indeed optional to avoid breaking 
any of the older systems which only use/provide extcon.

> Perhaps a better way to say this in both comments would be "to maintain" 
> instead of "for".

I think best just drop the "for backward compatibility" altogether, like so:

/*
  * Type-C support is optional. Only call the Type-C handler if a
  * port had been registered previously.
  */