Re: [PATCH 03/18] phy: tegra: xusb: Add usb-role-switch support

[Thierry Reding]

On Fri, Dec 06, 2019 at 04:20:06PM +0530, Nagarjuna Kristam wrote:
> If usb-role-switch property is present in USB 2 port, register
> usb-role-switch to receive usb role changes.
> 
> Signed-off-by: Nagarjuna Kristam <nkristam@xxxxxxxxxx>
> ---
>  drivers/phy/tegra/Kconfig |  1 +
>  drivers/phy/tegra/xusb.c  | 40 ++++++++++++++++++++++++++++++++++++++++
>  drivers/phy/tegra/xusb.h  |  3 +++
>  3 files changed, 44 insertions(+)
> 
> diff --git a/drivers/phy/tegra/Kconfig b/drivers/phy/tegra/Kconfig
> index f9817c3..df07c4d 100644
> --- a/drivers/phy/tegra/Kconfig
> +++ b/drivers/phy/tegra/Kconfig
> @@ -2,6 +2,7 @@
>  config PHY_TEGRA_XUSB
>  	tristate "NVIDIA Tegra XUSB pad controller driver"
>  	depends on ARCH_TEGRA
> +	select USB_CONN_GPIO
>  	help
>  	  Choose this option if you have an NVIDIA Tegra SoC.
>  
> diff --git a/drivers/phy/tegra/xusb.c b/drivers/phy/tegra/xusb.c
> index f98ec39..da60a63 100644
> --- a/drivers/phy/tegra/xusb.c
> +++ b/drivers/phy/tegra/xusb.c
> @@ -523,6 +523,7 @@ static int tegra_xusb_port_init(struct tegra_xusb_port *port,
>  	port->dev.type = &tegra_xusb_port_type;
>  	port->dev.of_node = of_node_get(np);
>  	port->dev.parent = padctl->dev;
> +	port->dev.driver = padctl->dev->driver;
>  
>  	err = dev_set_name(&port->dev, "%s-%u", name, index);
>  	if (err < 0)
> @@ -532,6 +533,7 @@ static int tegra_xusb_port_init(struct tegra_xusb_port *port,
>  	if (err < 0)
>  		goto unregister;
>  
> +	dev_set_drvdata(&port->dev, port);

You never seem to use dev_get_drvdata() to get at this. Also, you can
get at it via container_of(), so this is only marginally useful to begin
with.

>  	return 0;
>  
>  unregister:
> @@ -541,6 +543,7 @@ static int tegra_xusb_port_init(struct tegra_xusb_port *port,
>  
>  static void tegra_xusb_port_unregister(struct tegra_xusb_port *port)
>  {
> +	usb_role_switch_unregister(port->usb_role_sw);
>  	device_unregister(&port->dev);
>  }
>  
> @@ -551,11 +554,39 @@ static const char *const modes[] = {
>  	[USB_DR_MODE_OTG] = "otg",
>  };
>  
> +static int tegra_xusb_role_sw_set(struct device *dev, enum usb_role role)
> +{
> +	dev_dbg(dev, "%s calling notifier for role is %d\n", __func__, role);
> +
> +	return 0;
> +}
> +
> +static int tegra_xusb_setup_usb_role_switch(struct tegra_xusb_port *port)
> +{
> +	int err = 0;
> +	struct usb_role_switch_desc role_sx_desc = {
> +					.set = tegra_xusb_role_sw_set,
> +					.fwnode = dev_fwnode(&port->dev),
> +						   };
> +
> +	port->usb_role_sw = usb_role_switch_register(&port->dev,
> +						&role_sx_desc);
> +	if (IS_ERR(port->usb_role_sw)) {
> +		err = PTR_ERR(port->usb_role_sw);
> +		if (err != EPROBE_DEFER)
> +			dev_err(&port->dev, "Failed to register USB role SW: %d",
> +				err);
> +	}
> +
> +	return err;
> +}
> +
>  static int tegra_xusb_usb2_port_parse_dt(struct tegra_xusb_usb2_port *usb2)
>  {
>  	struct tegra_xusb_port *port = &usb2->base;
>  	struct device_node *np = port->dev.of_node;
>  	const char *mode;
> +	int err;
>  
>  	usb2->internal = of_property_read_bool(np, "nvidia,internal");
>  
> @@ -572,6 +603,15 @@ static int tegra_xusb_usb2_port_parse_dt(struct tegra_xusb_usb2_port *usb2)
>  		usb2->mode = USB_DR_MODE_HOST;
>  	}
>  
> +	if (of_property_read_bool(np, "usb-role-switch")) {
> +		/* populate connector entry */
> +		of_platform_populate(np, NULL, NULL, &port->dev);

I think we want to clean this up on failure, don't we? Otherwise we
might end up trying to register the same platform device multiple times.
Also, do we want to depopulate when the port is removed again?

Have you tried unloading and loading the driver to see if that works?

Thierry

> +
> +		err = tegra_xusb_setup_usb_role_switch(port);
> +		if (err < 0)
> +			return err;
> +	}
> +
>  	usb2->supply = devm_regulator_get(&port->dev, "vbus");
>  	return PTR_ERR_OR_ZERO(usb2->supply);
>  }
> diff --git a/drivers/phy/tegra/xusb.h b/drivers/phy/tegra/xusb.h
> index da94fcc..9f27899 100644
> --- a/drivers/phy/tegra/xusb.h
> +++ b/drivers/phy/tegra/xusb.h
> @@ -12,6 +12,7 @@
>  #include <linux/workqueue.h>
>  
>  #include <linux/usb/otg.h>
> +#include <linux/usb/role.h>
>  
>  /* legacy entry points for backwards-compatibility */
>  int tegra_xusb_padctl_legacy_probe(struct platform_device *pdev);
> @@ -266,6 +267,8 @@ struct tegra_xusb_port {
>  	struct list_head list;
>  	struct device dev;
>  
> +	struct usb_role_switch *usb_role_sw;
> +
>  	const struct tegra_xusb_port_ops *ops;
>  };
>  
> -- 
> 2.7.4
>

Attachment: signature.asc
Description: PGP signature