Since EUD is physically present between the USB connector and
the USB controller, it should relay the usb role notifications
from the connector. Hence register a role switch handler to
process and relay these roles to the USB controller. This results
in a common framework to send both connector related events
and eud attach/detach events to the USB controller.

Signed-off-by: Elson Roy Serrao <quic_eserrao@quicinc.com>
---
 drivers/usb/misc/qcom_eud.c | 91 ++++++++++++++++++++++++++++---------
 1 file changed, 69 insertions(+), 22 deletions(-)

diff --git a/drivers/usb/misc/qcom_eud.c b/drivers/usb/misc/qcom_eud.c
index 3de7d465912c..9a49c934e8cf 100644
--- a/drivers/usb/misc/qcom_eud.c
+++ b/drivers/usb/misc/qcom_eud.c
@@ -10,6 +10,7 @@
 #include <linux/iopoll.h>
 #include <linux/kernel.h>
 #include <linux/module.h>
+#include <linux/mutex.h>
 #include <linux/of.h>
 #include <linux/phy/phy.h>
 #include <linux/platform_device.h>
@@ -35,12 +36,16 @@ struct eud_chip {
 	struct device			*dev;
 	struct usb_role_switch		*role_sw;
 	struct phy			*usb2_phy;
+
+	/* mode lock */
+	struct mutex			mutex;
 	void __iomem			*base;
 	void __iomem			*mode_mgr;
 	unsigned int			int_status;
 	int				irq;
 	bool				enabled;
 	bool				usb_attached;
+	enum usb_role			current_role;
 };
 
 static int eud_phy_enable(struct eud_chip *chip)
@@ -64,6 +69,38 @@ static void eud_phy_disable(struct eud_chip *chip)
 	phy_exit(chip->usb2_phy);
 }
 
+static int eud_usb_role_set(struct eud_chip *chip, enum usb_role role)
+{
+	struct usb_role_switch *sw;
+	int ret = 0;
+
+	mutex_lock(&chip->mutex);
+
+	/* Avoid duplicate role handling */
+	if (role == chip->current_role)
+		goto err;
+
+	sw = usb_role_switch_get(chip->dev);
+	if (IS_ERR_OR_NULL(sw)) {
+		dev_err(chip->dev, "failed to get usb switch\n");
+		ret = -EINVAL;
+		goto err;
+	}
+
+	ret = usb_role_switch_set_role(sw, role);
+	usb_role_switch_put(sw);
+
+	if (ret) {
+		dev_err(chip->dev, "failed to set role\n");
+		goto err;
+	}
+	chip->current_role = role;
+err:
+	mutex_unlock(&chip->mutex);
+
+	return ret;
+}
+
 static int enable_eud(struct eud_chip *priv)
 {
 	int ret;
@@ -77,7 +114,7 @@ static int enable_eud(struct eud_chip *priv)
 			priv->base + EUD_REG_INT1_EN_MASK);
 	writel(1, priv->mode_mgr + EUD_REG_EUD_EN2);
 
-	return usb_role_switch_set_role(priv->role_sw, USB_ROLE_DEVICE);
+	return ret;
 }
 
 static void disable_eud(struct eud_chip *priv)
@@ -106,15 +143,20 @@ static ssize_t enable_store(struct device *dev,
 	if (kstrtobool(buf, &enable))
 		return -EINVAL;
 
+	/* EUD enable is applicable only in DEVICE mode */
+	if (enable && chip->current_role != USB_ROLE_DEVICE)
+		return -EINVAL;
+
 	if (enable) {
 		ret = enable_eud(chip);
-		if (!ret)
-			chip->enabled = enable;
-		else
-			disable_eud(chip);
+		if (ret) {
+			dev_err(chip->dev, "failed to enable eud\n");
+			return count;
+		}
 	} else {
 		disable_eud(chip);
 	}
+	chip->enabled = enable;
 
 	return count;
 }
@@ -185,11 +227,9 @@ static irqreturn_t handle_eud_irq_thread(int irq, void *data)
 	int ret;
 
 	if (chip->usb_attached)
-		ret = usb_role_switch_set_role(chip->role_sw, USB_ROLE_DEVICE);
+		ret = eud_usb_role_set(chip, USB_ROLE_DEVICE);
 	else
-		ret = usb_role_switch_set_role(chip->role_sw, USB_ROLE_HOST);
-	if (ret)
-		dev_err(chip->dev, "failed to set role switch\n");
+		ret = eud_usb_role_set(chip, USB_ROLE_HOST);
 
 	/* set and clear vbus_int_clr[0] to clear interrupt */
 	writel(BIT(0), chip->base + EUD_REG_VBUS_INT_CLR);
@@ -198,16 +238,18 @@ static irqreturn_t handle_eud_irq_thread(int irq, void *data)
 	return IRQ_HANDLED;
 }
 
-static void eud_role_switch_release(void *data)
+static int eud_usb_role_switch_set(struct usb_role_switch *sw,
+				   enum usb_role role)
 {
-	struct eud_chip *chip = data;
+	struct eud_chip *chip = usb_role_switch_get_drvdata(sw);
 
-	usb_role_switch_put(chip->role_sw);
+	return eud_usb_role_set(chip, role);
 }
 
 static int eud_probe(struct platform_device *pdev)
 {
 	struct eud_chip *chip;
+	struct usb_role_switch_desc eud_role_switch = {NULL};
 	int ret;
 
 	chip = devm_kzalloc(&pdev->dev, sizeof(*chip), GFP_KERNEL);
@@ -221,16 +263,6 @@ static int eud_probe(struct platform_device *pdev)
 		return dev_err_probe(chip->dev, PTR_ERR(chip->usb2_phy),
 				     "no usb2 phy configured\n");
 
-	chip->role_sw = usb_role_switch_get(&pdev->dev);
-	if (IS_ERR(chip->role_sw))
-		return dev_err_probe(chip->dev, PTR_ERR(chip->role_sw),
-					"failed to get role switch\n");
-
-	ret = devm_add_action_or_reset(chip->dev, eud_role_switch_release, chip);
-	if (ret)
-		return dev_err_probe(chip->dev, ret,
-				"failed to add role switch release action\n");
-
 	chip->base = devm_platform_ioremap_resource(pdev, 0);
 	if (IS_ERR(chip->base))
 		return PTR_ERR(chip->base);
@@ -248,6 +280,18 @@ static int eud_probe(struct platform_device *pdev)
 	if (ret)
 		return dev_err_probe(chip->dev, ret, "failed to allocate irq\n");
 
+	eud_role_switch.fwnode = dev_fwnode(chip->dev);
+	eud_role_switch.set = eud_usb_role_switch_set;
+	eud_role_switch.get = NULL;
+	eud_role_switch.driver_data = chip;
+	chip->role_sw = usb_role_switch_register(chip->dev, &eud_role_switch);
+
+	if (IS_ERR(chip->role_sw))
+		return dev_err_probe(chip->dev, PTR_ERR(chip->role_sw),
+				"failed to register role switch\n");
+
+	mutex_init(&chip->mutex);
+
 	enable_irq_wake(chip->irq);
 
 	platform_set_drvdata(pdev, chip);
@@ -262,6 +306,9 @@ static void eud_remove(struct platform_device *pdev)
 	if (chip->enabled)
 		disable_eud(chip);
 
+	if (chip->role_sw)
+		usb_role_switch_unregister(chip->role_sw);
+
 	device_init_wakeup(&pdev->dev, false);
 	disable_irq_wake(chip->irq);
 }
-- 
2.17.1

Hi Elson,

kernel test robot noticed the following build warnings:

[auto build test WARNING on robh/for-next]
[also build test WARNING on usb/usb-testing usb/usb-next usb/usb-linus linus/master v6.11-rc1 next-20240801]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Elson-Roy-Serrao/dt-bindings-soc-qcom-eud-Add-phy-related-bindings/20240801-210521
base:   https://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git for-next
patch link:    https://lore.kernel.org/r/20240730222439.3469-8-quic_eserrao%40quicinc.com
patch subject: [PATCH 7/8] usb: misc: eud: Handle usb role switch notifications
config: m68k-allmodconfig (https://download.01.org/0day-ci/archive/20240802/202408020600.vU0uKLa7-lkp@intel.com/config)
compiler: m68k-linux-gcc (GCC) 14.1.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240802/202408020600.vU0uKLa7-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202408020600.vU0uKLa7-lkp@intel.com/

All warnings (new ones prefixed by >>):

   drivers/usb/misc/qcom_eud.c: In function 'handle_eud_irq_thread':
>> drivers/usb/misc/qcom_eud.c:227:13: warning: variable 'ret' set but not used [-Wunused-but-set-variable]
     227 |         int ret;
         |             ^~~


vim +/ret +227 drivers/usb/misc/qcom_eud.c

9a1bf58ccd4432 Souradeep Chowdhury 2022-02-08  223  
9a1bf58ccd4432 Souradeep Chowdhury 2022-02-08  224  static irqreturn_t handle_eud_irq_thread(int irq, void *data)
9a1bf58ccd4432 Souradeep Chowdhury 2022-02-08  225  {
9a1bf58ccd4432 Souradeep Chowdhury 2022-02-08  226  	struct eud_chip *chip = data;
9a1bf58ccd4432 Souradeep Chowdhury 2022-02-08 @227  	int ret;
9a1bf58ccd4432 Souradeep Chowdhury 2022-02-08  228  
9a1bf58ccd4432 Souradeep Chowdhury 2022-02-08  229  	if (chip->usb_attached)
c007e96bfd0471 Elson Roy Serrao    2024-07-30  230  		ret = eud_usb_role_set(chip, USB_ROLE_DEVICE);
9a1bf58ccd4432 Souradeep Chowdhury 2022-02-08  231  	else
c007e96bfd0471 Elson Roy Serrao    2024-07-30  232  		ret = eud_usb_role_set(chip, USB_ROLE_HOST);
9a1bf58ccd4432 Souradeep Chowdhury 2022-02-08  233  
9a1bf58ccd4432 Souradeep Chowdhury 2022-02-08  234  	/* set and clear vbus_int_clr[0] to clear interrupt */
9a1bf58ccd4432 Souradeep Chowdhury 2022-02-08  235  	writel(BIT(0), chip->base + EUD_REG_VBUS_INT_CLR);
9a1bf58ccd4432 Souradeep Chowdhury 2022-02-08  236  	writel(0, chip->base + EUD_REG_VBUS_INT_CLR);
9a1bf58ccd4432 Souradeep Chowdhury 2022-02-08  237  
9a1bf58ccd4432 Souradeep Chowdhury 2022-02-08  238  	return IRQ_HANDLED;
9a1bf58ccd4432 Souradeep Chowdhury 2022-02-08  239  }
9a1bf58ccd4432 Souradeep Chowdhury 2022-02-08  240  

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

On Tue, Jul 30, 2024 at 03:24:38PM GMT, Elson Roy Serrao wrote:
> Since EUD is physically present between the USB connector and
> the USB controller, it should relay the usb role notifications
> from the connector. Hence register a role switch handler to
> process and relay these roles to the USB controller. This results
> in a common framework to send both connector related events
> and eud attach/detach events to the USB controller.
> 
> Signed-off-by: Elson Roy Serrao <quic_eserrao@quicinc.com>
> ---
>  drivers/usb/misc/qcom_eud.c | 91 ++++++++++++++++++++++++++++---------
>  1 file changed, 69 insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/usb/misc/qcom_eud.c b/drivers/usb/misc/qcom_eud.c
> index 3de7d465912c..9a49c934e8cf 100644
> --- a/drivers/usb/misc/qcom_eud.c
> +++ b/drivers/usb/misc/qcom_eud.c
> @@ -10,6 +10,7 @@
>  #include <linux/iopoll.h>
>  #include <linux/kernel.h>
>  #include <linux/module.h>
> +#include <linux/mutex.h>
>  #include <linux/of.h>
>  #include <linux/phy/phy.h>
>  #include <linux/platform_device.h>
> @@ -35,12 +36,16 @@ struct eud_chip {
>  	struct device			*dev;
>  	struct usb_role_switch		*role_sw;
>  	struct phy			*usb2_phy;
> +
> +	/* mode lock */
> +	struct mutex			mutex;
>  	void __iomem			*base;
>  	void __iomem			*mode_mgr;
>  	unsigned int			int_status;
>  	int				irq;
>  	bool				enabled;
>  	bool				usb_attached;
> +	enum usb_role			current_role;
>  };
>  
>  static int eud_phy_enable(struct eud_chip *chip)
> @@ -64,6 +69,38 @@ static void eud_phy_disable(struct eud_chip *chip)
>  	phy_exit(chip->usb2_phy);
>  }
>  
> +static int eud_usb_role_set(struct eud_chip *chip, enum usb_role role)
> +{
> +	struct usb_role_switch *sw;
> +	int ret = 0;
> +
> +	mutex_lock(&chip->mutex);
> +
> +	/* Avoid duplicate role handling */
> +	if (role == chip->current_role)
> +		goto err;
> +
> +	sw = usb_role_switch_get(chip->dev);

Why isn't chip->role_sw good enough? Why do you need to get it each
time?

> +	if (IS_ERR_OR_NULL(sw)) {
> +		dev_err(chip->dev, "failed to get usb switch\n");
> +		ret = -EINVAL;
> +		goto err;
> +	}
> +
> +	ret = usb_role_switch_set_role(sw, role);
> +	usb_role_switch_put(sw);
> +
> +	if (ret) {
> +		dev_err(chip->dev, "failed to set role\n");
> +		goto err;
> +	}
> +	chip->current_role = role;
> +err:
> +	mutex_unlock(&chip->mutex);
> +
> +	return ret;
> +}
> +
>  static int enable_eud(struct eud_chip *priv)
>  {
>  	int ret;
> @@ -77,7 +114,7 @@ static int enable_eud(struct eud_chip *priv)
>  			priv->base + EUD_REG_INT1_EN_MASK);
>  	writel(1, priv->mode_mgr + EUD_REG_EUD_EN2);
>  
> -	return usb_role_switch_set_role(priv->role_sw, USB_ROLE_DEVICE);
> +	return ret;
>  }
>  
>  static void disable_eud(struct eud_chip *priv)
> @@ -106,15 +143,20 @@ static ssize_t enable_store(struct device *dev,
>  	if (kstrtobool(buf, &enable))
>  		return -EINVAL;
>  
> +	/* EUD enable is applicable only in DEVICE mode */
> +	if (enable && chip->current_role != USB_ROLE_DEVICE)
> +		return -EINVAL;
> +
>  	if (enable) {
>  		ret = enable_eud(chip);
> -		if (!ret)
> -			chip->enabled = enable;
> -		else
> -			disable_eud(chip);
> +		if (ret) {
> +			dev_err(chip->dev, "failed to enable eud\n");
> +			return count;
> +		}
>  	} else {
>  		disable_eud(chip);
>  	}
> +	chip->enabled = enable;
>  
>  	return count;
>  }
> @@ -185,11 +227,9 @@ static irqreturn_t handle_eud_irq_thread(int irq, void *data)
>  	int ret;
>  
>  	if (chip->usb_attached)
> -		ret = usb_role_switch_set_role(chip->role_sw, USB_ROLE_DEVICE);
> +		ret = eud_usb_role_set(chip, USB_ROLE_DEVICE);
>  	else
> -		ret = usb_role_switch_set_role(chip->role_sw, USB_ROLE_HOST);
> -	if (ret)
> -		dev_err(chip->dev, "failed to set role switch\n");
> +		ret = eud_usb_role_set(chip, USB_ROLE_HOST);
>  
>  	/* set and clear vbus_int_clr[0] to clear interrupt */
>  	writel(BIT(0), chip->base + EUD_REG_VBUS_INT_CLR);
> @@ -198,16 +238,18 @@ static irqreturn_t handle_eud_irq_thread(int irq, void *data)
>  	return IRQ_HANDLED;
>  }
>  
> -static void eud_role_switch_release(void *data)
> +static int eud_usb_role_switch_set(struct usb_role_switch *sw,
> +				   enum usb_role role)
>  {
> -	struct eud_chip *chip = data;
> +	struct eud_chip *chip = usb_role_switch_get_drvdata(sw);
>  
> -	usb_role_switch_put(chip->role_sw);
> +	return eud_usb_role_set(chip, role);
>  }
>  
>  static int eud_probe(struct platform_device *pdev)
>  {
>  	struct eud_chip *chip;
> +	struct usb_role_switch_desc eud_role_switch = {NULL};
>  	int ret;
>  
>  	chip = devm_kzalloc(&pdev->dev, sizeof(*chip), GFP_KERNEL);
> @@ -221,16 +263,6 @@ static int eud_probe(struct platform_device *pdev)
>  		return dev_err_probe(chip->dev, PTR_ERR(chip->usb2_phy),
>  				     "no usb2 phy configured\n");
>  
> -	chip->role_sw = usb_role_switch_get(&pdev->dev);
> -	if (IS_ERR(chip->role_sw))
> -		return dev_err_probe(chip->dev, PTR_ERR(chip->role_sw),
> -					"failed to get role switch\n");
> -
> -	ret = devm_add_action_or_reset(chip->dev, eud_role_switch_release, chip);
> -	if (ret)
> -		return dev_err_probe(chip->dev, ret,
> -				"failed to add role switch release action\n");
> -
>  	chip->base = devm_platform_ioremap_resource(pdev, 0);
>  	if (IS_ERR(chip->base))
>  		return PTR_ERR(chip->base);
> @@ -248,6 +280,18 @@ static int eud_probe(struct platform_device *pdev)
>  	if (ret)
>  		return dev_err_probe(chip->dev, ret, "failed to allocate irq\n");
>  
> +	eud_role_switch.fwnode = dev_fwnode(chip->dev);
> +	eud_role_switch.set = eud_usb_role_switch_set;
> +	eud_role_switch.get = NULL;
> +	eud_role_switch.driver_data = chip;
> +	chip->role_sw = usb_role_switch_register(chip->dev, &eud_role_switch);
> +
> +	if (IS_ERR(chip->role_sw))
> +		return dev_err_probe(chip->dev, PTR_ERR(chip->role_sw),
> +				"failed to register role switch\n");
> +
> +	mutex_init(&chip->mutex);

please move mutex_init earlier.

> +
>  	enable_irq_wake(chip->irq);
>  
>  	platform_set_drvdata(pdev, chip);
> @@ -262,6 +306,9 @@ static void eud_remove(struct platform_device *pdev)
>  	if (chip->enabled)
>  		disable_eud(chip);
>  
> +	if (chip->role_sw)
> +		usb_role_switch_unregister(chip->role_sw);
> +
>  	device_init_wakeup(&pdev->dev, false);
>  	disable_irq_wake(chip->irq);
>  }
> -- 
> 2.17.1
> 

-- 
With best wishes
Dmitry

On 7/31/2024 6:06 AM, Dmitry Baryshkov wrote:
> On Tue, Jul 30, 2024 at 03:24:38PM GMT, Elson Roy Serrao wrote:
>> Since EUD is physically present between the USB connector and
>> the USB controller, it should relay the usb role notifications
>> from the connector. Hence register a role switch handler to
>> process and relay these roles to the USB controller. This results
>> in a common framework to send both connector related events
>> and eud attach/detach events to the USB controller.
>>
>> Signed-off-by: Elson Roy Serrao <quic_eserrao@quicinc.com>
>> ---
>>  drivers/usb/misc/qcom_eud.c | 91 ++++++++++++++++++++++++++++---------
>>  1 file changed, 69 insertions(+), 22 deletions(-)
>>
>> diff --git a/drivers/usb/misc/qcom_eud.c b/drivers/usb/misc/qcom_eud.c
>> index 3de7d465912c..9a49c934e8cf 100644
>> --- a/drivers/usb/misc/qcom_eud.c
>> +++ b/drivers/usb/misc/qcom_eud.c
>> @@ -10,6 +10,7 @@
>>  #include <linux/iopoll.h>
>>  #include <linux/kernel.h>
>>  #include <linux/module.h>
>> +#include <linux/mutex.h>
>>  #include <linux/of.h>
>>  #include <linux/phy/phy.h>
>>  #include <linux/platform_device.h>
>> @@ -35,12 +36,16 @@ struct eud_chip {
>>  	struct device			*dev;
>>  	struct usb_role_switch		*role_sw;
>>  	struct phy			*usb2_phy;
>> +
>> +	/* mode lock */
>> +	struct mutex			mutex;
>>  	void __iomem			*base;
>>  	void __iomem			*mode_mgr;
>>  	unsigned int			int_status;
>>  	int				irq;
>>  	bool				enabled;
>>  	bool				usb_attached;
>> +	enum usb_role			current_role;
>>  };
>>  
>>  static int eud_phy_enable(struct eud_chip *chip)
>> @@ -64,6 +69,38 @@ static void eud_phy_disable(struct eud_chip *chip)
>>  	phy_exit(chip->usb2_phy);
>>  }
>>  
>> +static int eud_usb_role_set(struct eud_chip *chip, enum usb_role role)
>> +{
>> +	struct usb_role_switch *sw;
>> +	int ret = 0;
>> +
>> +	mutex_lock(&chip->mutex);
>> +
>> +	/* Avoid duplicate role handling */
>> +	if (role == chip->current_role)
>> +		goto err;
>> +
>> +	sw = usb_role_switch_get(chip->dev);
> 
> Why isn't chip->role_sw good enough? Why do you need to get it each
> time?
>

Hi Dmitry

chip->role_sw is the eud role switch handler to receive role switch notifications from the
USB connector. The 'sw' I am getting above is the role switch handler of the USB controller.
As per this design, EUD receives role switch notification from the connector 
(via chip->role_sw) and then relays it to the 'sw' switch handler of the USB controller.


Thanks
Elson
 
>> +	if (IS_ERR_OR_NULL(sw)) {
>> +		dev_err(chip->dev, "failed to get usb switch\n");
>> +		ret = -EINVAL;
>> +		goto err;
>> +	}
>> +
>> +	ret = usb_role_switch_set_role(sw, role);
>> +	usb_role_switch_put(sw);
>> +
>> +	if (ret) {
>> +		dev_err(chip->dev, "failed to set role\n");
>> +		goto err;
>> +	}
>> +	chip->current_role = role;
>> +err:
>> +	mutex_unlock(&chip->mutex);
>> +
>> +	return ret;
>> +}
>> +
>>  static int enable_eud(struct eud_chip *priv)
>>  {
>>  	int ret;
>> @@ -77,7 +114,7 @@ static int enable_eud(struct eud_chip *priv)
>>  			priv->base + EUD_REG_INT1_EN_MASK);
>>  	writel(1, priv->mode_mgr + EUD_REG_EUD_EN2);
>>  
>> -	return usb_role_switch_set_role(priv->role_sw, USB_ROLE_DEVICE);
>> +	return ret;
>>  }
>>  
>>  static void disable_eud(struct eud_chip *priv)
>> @@ -106,15 +143,20 @@ static ssize_t enable_store(struct device *dev,
>>  	if (kstrtobool(buf, &enable))
>>  		return -EINVAL;
>>  
>> +	/* EUD enable is applicable only in DEVICE mode */
>> +	if (enable && chip->current_role != USB_ROLE_DEVICE)
>> +		return -EINVAL;
>> +
>>  	if (enable) {
>>  		ret = enable_eud(chip);
>> -		if (!ret)
>> -			chip->enabled = enable;
>> -		else
>> -			disable_eud(chip);
>> +		if (ret) {
>> +			dev_err(chip->dev, "failed to enable eud\n");
>> +			return count;
>> +		}
>>  	} else {
>>  		disable_eud(chip);
>>  	}
>> +	chip->enabled = enable;
>>  
>>  	return count;
>>  }
>> @@ -185,11 +227,9 @@ static irqreturn_t handle_eud_irq_thread(int irq, void *data)
>>  	int ret;
>>  
>>  	if (chip->usb_attached)
>> -		ret = usb_role_switch_set_role(chip->role_sw, USB_ROLE_DEVICE);
>> +		ret = eud_usb_role_set(chip, USB_ROLE_DEVICE);
>>  	else
>> -		ret = usb_role_switch_set_role(chip->role_sw, USB_ROLE_HOST);
>> -	if (ret)
>> -		dev_err(chip->dev, "failed to set role switch\n");
>> +		ret = eud_usb_role_set(chip, USB_ROLE_HOST);
>>  
>>  	/* set and clear vbus_int_clr[0] to clear interrupt */
>>  	writel(BIT(0), chip->base + EUD_REG_VBUS_INT_CLR);
>> @@ -198,16 +238,18 @@ static irqreturn_t handle_eud_irq_thread(int irq, void *data)
>>  	return IRQ_HANDLED;
>>  }
>>  
>> -static void eud_role_switch_release(void *data)
>> +static int eud_usb_role_switch_set(struct usb_role_switch *sw,
>> +				   enum usb_role role)
>>  {
>> -	struct eud_chip *chip = data;
>> +	struct eud_chip *chip = usb_role_switch_get_drvdata(sw);
>>  
>> -	usb_role_switch_put(chip->role_sw);
>> +	return eud_usb_role_set(chip, role);
>>  }
>>  
>>  static int eud_probe(struct platform_device *pdev)
>>  {
>>  	struct eud_chip *chip;
>> +	struct usb_role_switch_desc eud_role_switch = {NULL};
>>  	int ret;
>>  
>>  	chip = devm_kzalloc(&pdev->dev, sizeof(*chip), GFP_KERNEL);
>> @@ -221,16 +263,6 @@ static int eud_probe(struct platform_device *pdev)
>>  		return dev_err_probe(chip->dev, PTR_ERR(chip->usb2_phy),
>>  				     "no usb2 phy configured\n");
>>  
>> -	chip->role_sw = usb_role_switch_get(&pdev->dev);
>> -	if (IS_ERR(chip->role_sw))
>> -		return dev_err_probe(chip->dev, PTR_ERR(chip->role_sw),
>> -					"failed to get role switch\n");
>> -
>> -	ret = devm_add_action_or_reset(chip->dev, eud_role_switch_release, chip);
>> -	if (ret)
>> -		return dev_err_probe(chip->dev, ret,
>> -				"failed to add role switch release action\n");
>> -
>>  	chip->base = devm_platform_ioremap_resource(pdev, 0);
>>  	if (IS_ERR(chip->base))
>>  		return PTR_ERR(chip->base);
>> @@ -248,6 +280,18 @@ static int eud_probe(struct platform_device *pdev)
>>  	if (ret)
>>  		return dev_err_probe(chip->dev, ret, "failed to allocate irq\n");
>>  
>> +	eud_role_switch.fwnode = dev_fwnode(chip->dev);
>> +	eud_role_switch.set = eud_usb_role_switch_set;
>> +	eud_role_switch.get = NULL;
>> +	eud_role_switch.driver_data = chip;
>> +	chip->role_sw = usb_role_switch_register(chip->dev, &eud_role_switch);
>> +
>> +	if (IS_ERR(chip->role_sw))
>> +		return dev_err_probe(chip->dev, PTR_ERR(chip->role_sw),
>> +				"failed to register role switch\n");
>> +
>> +	mutex_init(&chip->mutex);
> 
> please move mutex_init earlier.
> 

Ack
>> +
>>  	enable_irq_wake(chip->irq);
>>  
>>  	platform_set_drvdata(pdev, chip);
>> @@ -262,6 +306,9 @@ static void eud_remove(struct platform_device *pdev)
>>  	if (chip->enabled)
>>  		disable_eud(chip);
>>  
>> +	if (chip->role_sw)
>> +		usb_role_switch_unregister(chip->role_sw);
>> +
>>  	device_init_wakeup(&pdev->dev, false);
>>  	disable_irq_wake(chip->irq);
>>  }
>> -- 
>> 2.17.1
>>
>

On Wed, Jul 31, 2024 at 05:51:17PM GMT, Elson Serrao wrote:
> 
> 
> On 7/31/2024 6:06 AM, Dmitry Baryshkov wrote:
> > On Tue, Jul 30, 2024 at 03:24:38PM GMT, Elson Roy Serrao wrote:
> >> Since EUD is physically present between the USB connector and
> >> the USB controller, it should relay the usb role notifications
> >> from the connector. Hence register a role switch handler to
> >> process and relay these roles to the USB controller. This results
> >> in a common framework to send both connector related events
> >> and eud attach/detach events to the USB controller.
> >>
> >> Signed-off-by: Elson Roy Serrao <quic_eserrao@quicinc.com>
> >> ---
> >>  drivers/usb/misc/qcom_eud.c | 91 ++++++++++++++++++++++++++++---------
> >>  1 file changed, 69 insertions(+), 22 deletions(-)
> >>
> >> diff --git a/drivers/usb/misc/qcom_eud.c b/drivers/usb/misc/qcom_eud.c
> >> index 3de7d465912c..9a49c934e8cf 100644
> >> --- a/drivers/usb/misc/qcom_eud.c
> >> +++ b/drivers/usb/misc/qcom_eud.c
> >> @@ -10,6 +10,7 @@
> >>  #include <linux/iopoll.h>
> >>  #include <linux/kernel.h>
> >>  #include <linux/module.h>
> >> +#include <linux/mutex.h>
> >>  #include <linux/of.h>
> >>  #include <linux/phy/phy.h>
> >>  #include <linux/platform_device.h>
> >> @@ -35,12 +36,16 @@ struct eud_chip {
> >>  	struct device			*dev;
> >>  	struct usb_role_switch		*role_sw;
> >>  	struct phy			*usb2_phy;
> >> +
> >> +	/* mode lock */
> >> +	struct mutex			mutex;
> >>  	void __iomem			*base;
> >>  	void __iomem			*mode_mgr;
> >>  	unsigned int			int_status;
> >>  	int				irq;
> >>  	bool				enabled;
> >>  	bool				usb_attached;
> >> +	enum usb_role			current_role;
> >>  };
> >>  
> >>  static int eud_phy_enable(struct eud_chip *chip)
> >> @@ -64,6 +69,38 @@ static void eud_phy_disable(struct eud_chip *chip)
> >>  	phy_exit(chip->usb2_phy);
> >>  }
> >>  
> >> +static int eud_usb_role_set(struct eud_chip *chip, enum usb_role role)
> >> +{
> >> +	struct usb_role_switch *sw;
> >> +	int ret = 0;
> >> +
> >> +	mutex_lock(&chip->mutex);
> >> +
> >> +	/* Avoid duplicate role handling */
> >> +	if (role == chip->current_role)
> >> +		goto err;
> >> +
> >> +	sw = usb_role_switch_get(chip->dev);
> > 
> > Why isn't chip->role_sw good enough? Why do you need to get it each
> > time?
> >
> 
> Hi Dmitry
> 
> chip->role_sw is the eud role switch handler to receive role switch notifications from the
> USB connector. The 'sw' I am getting above is the role switch handler of the USB controller.
> As per this design, EUD receives role switch notification from the connector 
> (via chip->role_sw) and then relays it to the 'sw' switch handler of the USB controller.

The fact that you have repurposed existing structure field is not a
waiver for the inefficiency.

So what about keeping chip->role_sw as is and adding _new_ field for the
self-provided role switch?


-- 
With best wishes
Dmitry