Re: [PATCH v4 4/9] usb: misc: qcom_eud: add per-path role switch support

[Peter Chen <peter.chen@kernel.org>]

On 26-05-01 10:06:30, Elson Serrao wrote:
> The EUD hardware can support multiple High-Speed USB paths, each connected
> to different USB controllers. The current implementation uses a single
> chip-level role switch, which cannot properly handle multi-path
> configurations where each path needs independent role management. Since
> EUD is physically present between the USB connector and the controller,
> it should also relay the role change requests from the connector.
> 
> Restructure the driver to support per-path role switches and remove the
> chip-level role switch. Additionally, as EUD need not modify the USB
> role upon enabling, remove the unnecessary role switch call from
> enable_eud().
> 
> Signed-off-by: Elson Serrao <elson.serrao@oss.qualcomm.com>
> ---
>  drivers/usb/misc/qcom_eud.c | 89 +++++++++++++++++++++++++++++--------
>  1 file changed, 70 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/usb/misc/qcom_eud.c b/drivers/usb/misc/qcom_eud.c
> index a624c44d5d59..7fbb74bf1599 100644
> --- a/drivers/usb/misc/qcom_eud.c
> +++ b/drivers/usb/misc/qcom_eud.c
> @@ -44,12 +44,15 @@ static const char * const eud_port_names[] = {
>  struct eud_path {
>  	struct eud_chip		*chip;
>  	struct phy		*phy;
> +	struct usb_role_switch	*controller_sw;
> +	struct usb_role_switch	*eud_sw;
> +	enum usb_role		curr_role;
> +	char			name[16];
>  	u8			num;
>  };
>  
>  struct eud_chip {
>  	struct device			*dev;
> -	struct usb_role_switch		*role_sw;
>  	void __iomem			*base;
>  	struct eud_path			*paths[EUD_MAX_PORTS];
>  	phys_addr_t			mode_mgr;
> @@ -123,7 +126,7 @@ static int enable_eud(struct eud_chip *priv)
>  	writel(EUD_INT_VBUS | EUD_INT_SAFE_MODE,
>  			priv->base + EUD_REG_INT1_EN_MASK);
>  
> -	return usb_role_switch_set_role(priv->role_sw, USB_ROLE_DEVICE);
> +	return 0;
>  }
>  
>  static int disable_eud(struct eud_chip *priv)
> @@ -274,12 +277,15 @@ static irqreturn_t handle_eud_irq(int irq, void *data)
>  static irqreturn_t handle_eud_irq_thread(int irq, void *data)
>  {
>  	struct eud_chip *chip = data;
> +	struct eud_path *path;
>  	int ret;
>  
> +	path = chip->paths[chip->port_idx];
> +
>  	if (chip->usb_attached)
> -		ret = usb_role_switch_set_role(chip->role_sw, USB_ROLE_DEVICE);
> +		ret = usb_role_switch_set_role(path->controller_sw, USB_ROLE_DEVICE);
>  	else
> -		ret = usb_role_switch_set_role(chip->role_sw, USB_ROLE_HOST);
> +		ret = usb_role_switch_set_role(path->controller_sw, USB_ROLE_HOST);
>  	if (ret)
>  		dev_err(chip->dev, "failed to set role switch\n");
>  
> @@ -290,15 +296,36 @@ 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_role_switch_set(struct usb_role_switch *sw, enum usb_role role)
>  {
> -	struct eud_chip *chip = data;
> +	struct eud_path *path = usb_role_switch_get_drvdata(sw);
> +	int ret;
> +
> +	/* Forward the role request to the USB controller */
> +	ret = usb_role_switch_set_role(path->controller_sw, role);
> +	if (ret) {
> +		dev_err(path->chip->dev, "failed to set role %s for port %u: %d\n",
> +			usb_role_string(role), path->num, ret);
> +		return ret;
> +	}
>  
> -	usb_role_switch_put(chip->role_sw);
> +	path->curr_role = role;
> +
> +	return 0;
> +}
> +
> +static void eud_path_role_switch_release(void *data)
> +{
> +	struct eud_path *path = data;
> +
> +	usb_role_switch_unregister(path->eud_sw);
> +	usb_role_switch_put(path->controller_sw);
>  }
>  
>  static int eud_init_path(struct eud_chip *chip, struct device_node *np)
>  {
> +	struct usb_role_switch_desc role_sw_desc;

Should role_sw_desc be zero-initialized here? Otherwise, some entries
of path->eud_sw are uncertain after calling usb_role_switch_register?

Peter

> +	struct usb_role_switch *sw;
>  	struct device_node *controller_node;
>  	struct eud_path *path;
>  	u32 path_num;
> @@ -331,11 +358,44 @@ static int eud_init_path(struct eud_chip *chip, struct device_node *np)
>  				     "failed to get controller node for path %u\n", path_num);
>  
>  	path->phy = devm_of_phy_get_by_index(chip->dev, controller_node, 0);
> -	of_node_put(controller_node);
> -
> -	if (IS_ERR(path->phy))
> +	if (IS_ERR(path->phy)) {
> +		of_node_put(controller_node);
>  		return dev_err_probe(chip->dev, PTR_ERR(path->phy),
>  				     "failed to get PHY for path %d\n", path_num);
> +	}
> +
> +	path->curr_role = USB_ROLE_NONE;
> +
> +	/* Fetch controller role switch if it is role switch capable */
> +	if (of_property_read_bool(controller_node, "usb-role-switch")) {
> +		sw = usb_role_switch_find_by_fwnode(of_fwnode_handle(controller_node));
> +		if (!sw) {
> +			of_node_put(controller_node);
> +			return dev_err_probe(chip->dev, -EPROBE_DEFER,
> +					     "Failed to get controller role switch for path %d\n",
> +					     path_num);
> +		}
> +		path->controller_sw = sw;
> +	}
> +
> +	of_node_put(controller_node);
> +
> +	role_sw_desc.fwnode = of_fwnode_handle(np);
> +	role_sw_desc.set = eud_role_switch_set;
> +	role_sw_desc.driver_data = path;
> +	snprintf(path->name, sizeof(path->name), "eud-path%u", path_num);
> +	role_sw_desc.name = path->name;
> +
> +	path->eud_sw = usb_role_switch_register(chip->dev, &role_sw_desc);
> +	if (IS_ERR(path->eud_sw)) {
> +		usb_role_switch_put(path->controller_sw);
> +		return dev_err_probe(chip->dev, PTR_ERR(path->eud_sw),
> +				     "Failed to register EUD role switch for path %d\n", path_num);
> +	}
> +
> +	ret = devm_add_action_or_reset(chip->dev, eud_path_role_switch_release, path);
> +	if (ret)
> +		return ret;
>  
>  	chip->paths[path_num] = path;
>  
> @@ -365,15 +425,6 @@ static int eud_probe(struct platform_device *pdev)
>  	if (!chip->paths[0])
>  		return dev_err_probe(chip->dev, -ENODEV, "primary path not found\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 ret;
> -
>  	chip->base = devm_platform_ioremap_resource(pdev, 0);
>  	if (IS_ERR(chip->base))
>  		return PTR_ERR(chip->base);
> -- 
> 2.34.1
> 
> 

-- 

Thanks,
Peter Chen