[PATCH v3] usb: common: usb-conn-gpio: Set last role to unknown before initial detection

[PATCH v3] usb: common: usb-conn-gpio: Set last role to unknown before initial detection

[Prashanth K]

Currently if we bootup a device without cable connected, then
usb-conn-gpio won't call set_role() since last_role is same as
current role. This happens because during probe last_role gets
initialised to zero.

To avoid this, added a new constant in enum usb_role, last_role
is set to USB_ROLE_UNKNOWN before performing initial detection.

Fixes: 4602f3bff266 ("usb: common: add USB GPIO based connection detection driver")
Signed-off-by: Prashanth K <quic_prashk@quicinc.com>
---
v3: Added a default case in drivers/usb/cdns3/core.c as pointed out by
    the test robot
v2: Added USB_ROLE_UNKNWON to enum usb_role

 drivers/usb/cdns3/core.c           | 2 ++
 drivers/usb/common/usb-conn-gpio.c | 3 +++
 include/linux/usb/role.h           | 1 +
 3 files changed, 6 insertions(+)

diff --git a/drivers/usb/cdns3/core.c b/drivers/usb/cdns3/core.c
index dbcdf3b..69d2921 100644
--- a/drivers/usb/cdns3/core.c
+++ b/drivers/usb/cdns3/core.c
@@ -252,6 +252,8 @@ static enum usb_role cdns_hw_role_state_machine(struct cdns *cdns)
 		if (!vbus)
 			role = USB_ROLE_NONE;
 		break;
+	default:
+		break;
 	}
 
 	dev_dbg(cdns->dev, "role %d -> %d\n", cdns->role, role);
diff --git a/drivers/usb/common/usb-conn-gpio.c b/drivers/usb/common/usb-conn-gpio.c
index e20874c..30bdb81 100644
--- a/drivers/usb/common/usb-conn-gpio.c
+++ b/drivers/usb/common/usb-conn-gpio.c
@@ -257,6 +257,9 @@ static int usb_conn_probe(struct platform_device *pdev)
 	platform_set_drvdata(pdev, info);
 	device_set_wakeup_capable(&pdev->dev, true);
 
+	/* Set last role to unknown before performing the initial detection */
+	info->last_role = USB_ROLE_UNKNOWN;
+
 	/* Perform initial detection */
 	usb_conn_queue_dwork(info, 0);
 
diff --git a/include/linux/usb/role.h b/include/linux/usb/role.h
index b5deafd..221d462 100644
--- a/include/linux/usb/role.h
+++ b/include/linux/usb/role.h
@@ -8,6 +8,7 @@
 struct usb_role_switch;
 
 enum usb_role {
+	USB_ROLE_UNKNOWN = -1,
 	USB_ROLE_NONE,
 	USB_ROLE_HOST,
 	USB_ROLE_DEVICE,
-- 
2.7.4

Re: [PATCH v3] usb: common: usb-conn-gpio: Set last role to unknown before initial detection

[AngeloGioacchino Del Regno]

Il 25/05/23 10:40, Prashanth K ha scritto:
> Currently if we bootup a device without cable connected, then
> usb-conn-gpio won't call set_role() since last_role is same as
> current role. This happens because during probe last_role gets
> initialised to zero.
> 
> To avoid this, added a new constant in enum usb_role, last_role
> is set to USB_ROLE_UNKNOWN before performing initial detection.
> 
> Fixes: 4602f3bff266 ("usb: common: add USB GPIO based connection detection driver")
> Signed-off-by: Prashanth K <quic_prashk@quicinc.com>

I'm sorry to make a call for v4, but you have to mention that you're touching
the cdns3 driver in the commit description, if you want to keep the entire
change set in one commit... otherwise you'll have to split it in two, one adding
the new entry to the enum and fixing cdns3; the other setting the last role to
unknown in usb-conn-gpio.c.

I can suggest text for keeping that in one commit, but the choice is yours;

"While at it, also handle default case for the usb_role switch
in cdns3 to avoid build warnings."

> ---
> v3: Added a default case in drivers/usb/cdns3/core.c as pointed out by
>      the test robot
> v2: Added USB_ROLE_UNKNWON to enum usb_role
> 
>   drivers/usb/cdns3/core.c           | 2 ++
>   drivers/usb/common/usb-conn-gpio.c | 3 +++
>   include/linux/usb/role.h           | 1 +
>   3 files changed, 6 insertions(+)
> 
> diff --git a/drivers/usb/cdns3/core.c b/drivers/usb/cdns3/core.c
> index dbcdf3b..69d2921 100644
> --- a/drivers/usb/cdns3/core.c
> +++ b/drivers/usb/cdns3/core.c
> @@ -252,6 +252,8 @@ static enum usb_role cdns_hw_role_state_machine(struct cdns *cdns)
>   		if (!vbus)
>   			role = USB_ROLE_NONE;
>   		break;
> +	default:
> +		break;
>   	}
>   
>   	dev_dbg(cdns->dev, "role %d -> %d\n", cdns->role, role);
> diff --git a/drivers/usb/common/usb-conn-gpio.c b/drivers/usb/common/usb-conn-gpio.c
> index e20874c..30bdb81 100644
> --- a/drivers/usb/common/usb-conn-gpio.c
> +++ b/drivers/usb/common/usb-conn-gpio.c
> @@ -257,6 +257,9 @@ static int usb_conn_probe(struct platform_device *pdev)
>   	platform_set_drvdata(pdev, info);
>   	device_set_wakeup_capable(&pdev->dev, true);
>   
> +	/* Set last role to unknown before performing the initial detection */
> +	info->last_role = USB_ROLE_UNKNOWN;
> +
>   	/* Perform initial detection */
>   	usb_conn_queue_dwork(info, 0);
>   
> diff --git a/include/linux/usb/role.h b/include/linux/usb/role.h
> index b5deafd..221d462 100644
> --- a/include/linux/usb/role.h
> +++ b/include/linux/usb/role.h
> @@ -8,6 +8,7 @@
>   struct usb_role_switch;
>   
>   enum usb_role {
> +	USB_ROLE_UNKNOWN = -1,
>   	USB_ROLE_NONE,
>   	USB_ROLE_HOST,
>   	USB_ROLE_DEVICE,

Re: [PATCH v3] usb: common: usb-conn-gpio: Set last role to unknown before initial detection

[Prashanth K]

On 25-05-23 02:15 pm, AngeloGioacchino Del Regno wrote:
> Il 25/05/23 10:40, Prashanth K ha scritto:
>> Currently if we bootup a device without cable connected, then
>> usb-conn-gpio won't call set_role() since last_role is same as
>> current role. This happens because during probe last_role gets
>> initialised to zero.
>>
>> To avoid this, added a new constant in enum usb_role, last_role
>> is set to USB_ROLE_UNKNOWN before performing initial detection.
>>
>> Fixes: 4602f3bff266 ("usb: common: add USB GPIO based connection 
>> detection driver")
>> Signed-off-by: Prashanth K <quic_prashk@quicinc.com>
> 
> I'm sorry to make a call for v4, but you have to mention that you're 
> touching
> the cdns3 driver in the commit description, if you want to keep the entire
> change set in one commit... otherwise you'll have to split it in two, 
> one adding
> the new entry to the enum and fixing cdns3; the other setting the last 
> role to
> unknown in usb-conn-gpio.c.
> 
> I can suggest text for keeping that in one commit, but the choice is yours;
> 
> "While at it, also handle default case for the usb_role switch
> in cdns3 to avoid build warnings."
> 
Yea that's right, will add it in next version. Thanks for the suggestion.

Regards,
Prashanth K