diff mbox series

[V2,03/18] phy: tegra: xusb: Add usb-role-switch support

Message ID 1576660591-10383-4-git-send-email-nkristam@nvidia.com (mailing list archive)
State Superseded
Headers show
Series Tegra XUSB OTG support | expand

Commit Message

Nagarjuna Kristam Dec. 18, 2019, 9:16 a.m. UTC
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@nvidia.com>
---
V2:
 - Removed dev_set_drvdata for port->dev.
 - Added of_platform_depopulate during error handling and driver removal.
---
 drivers/phy/tegra/Kconfig |  1 +
 drivers/phy/tegra/xusb.c  | 42 ++++++++++++++++++++++++++++++++++++++++++
 drivers/phy/tegra/xusb.h  |  3 +++
 3 files changed, 46 insertions(+)

Comments

Thierry Reding Dec. 19, 2019, 1:26 p.m. UTC | #1
On Wed, Dec 18, 2019 at 02:46:16PM +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@nvidia.com>
> ---
> V2:
>  - Removed dev_set_drvdata for port->dev.
>  - Added of_platform_depopulate during error handling and driver removal.
> ---
>  drivers/phy/tegra/Kconfig |  1 +
>  drivers/phy/tegra/xusb.c  | 42 ++++++++++++++++++++++++++++++++++++++++++
>  drivers/phy/tegra/xusb.h  |  3 +++
>  3 files changed, 46 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..dc00b42 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)
> @@ -541,6 +542,10 @@ static int tegra_xusb_port_init(struct tegra_xusb_port *port,
>  
>  static void tegra_xusb_port_unregister(struct tegra_xusb_port *port)
>  {
> +	if (!IS_ERR_OR_NULL(port->usb_role_sw)) {
> +		of_platform_depopulate(&port->dev);
> +		usb_role_switch_unregister(port->usb_role_sw);
> +	}
>  	device_unregister(&port->dev);

Nit: I prefer blank lines after blocks for readability.

>  }
>  
> @@ -551,11 +556,42 @@ 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);

I don't understand what "for role is %d" means here. I think perhaps you
meant to simply say "for role %d"? Also, perhaps add parentheses after
the "%s" to clarify that you're referring to a function.

> +
> +	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),
> +						   };

The indentation here is odd. Use a single tab to indent lines after the
opening { and put the closing } on the same column as "struct". Also,
the above may become more readable if you follow the "inverse Christmas
tree" style of declaring functions, where you order lines by their
length, with the longest line first, like so:

	struct usb_role_switch_desc role_sx_desc = {
		.fwnode = dev_fwnode(&port->dev),
		.set = tegra_xusb_role_sw_set,
	};
	int err = 0;

> +
> +	port->usb_role_sw = usb_role_switch_register(&port->dev,
> +						&role_sx_desc);

&role_sx_desc should be aligned with &port->dev.

> +	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",

Error messages typically start with a lowercase letter (at least in this
driver). Also perhaps spell out "switch" above because SW could easily
be confused with "software".

> +				err);

Shouldn't we abort here? Consider the case where this indeed defers
probe. If we don't abort here, the of_platform_populate() below will be
called multiple times. Shouldn't it only be called when we actually
succeed in registering the switch?

Also, looking at usb_role_switch_register(), I don't think it ever can
return -EPROBE_DEFER, so I think you can drop that check and print the
error unconditionally.

Thierry

> +	}
> +
> +	/* populate connector entry */
> +	of_platform_populate(port->dev.of_node, NULL, NULL, &port->dev);
> +
> +	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 +608,12 @@ 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")) {
> +		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
>
JC Kuo Dec. 26, 2019, 6:42 a.m. UTC | #2
On 12/18/19 5:16 PM, 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@nvidia.com>
> ---
> V2:
>  - Removed dev_set_drvdata for port->dev.
>  - Added of_platform_depopulate during error handling and driver removal.
> ---
>  drivers/phy/tegra/Kconfig |  1 +
>  drivers/phy/tegra/xusb.c  | 42 ++++++++++++++++++++++++++++++++++++++++++
>  drivers/phy/tegra/xusb.h  |  3 +++
>  3 files changed, 46 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..dc00b42 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)
> @@ -541,6 +542,10 @@ static int tegra_xusb_port_init(struct tegra_xusb_port *port,
>  
>  static void tegra_xusb_port_unregister(struct tegra_xusb_port *port)
>  {
> +	if (!IS_ERR_OR_NULL(port->usb_role_sw)) {
> +		of_platform_depopulate(&port->dev);
> +		usb_role_switch_unregister(port->usb_role_sw);
> +	}
>  	device_unregister(&port->dev);
>  }
>  
> @@ -551,11 +556,42 @@ 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);
> +	}
> +
> +	/* populate connector entry */
> +	of_platform_populate(port->dev.of_node, NULL, NULL, &port->dev);
> +
> +	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 +608,12 @@ static int tegra_xusb_usb2_port_parse_dt(struct tegra_xusb_usb2_port *usb2)
>  		usb2->mode = USB_DR_MODE_HOST;
>  	}
>  
"usb-role-switch" is not required for host mode port. If a otg or a peripheral
port doesn't have "usb-role-switch" property, .probe() should abort.

Thanks,
JC

> +	if (of_property_read_bool(np, "usb-role-switch")) {
> +		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;
>  };
>  
>
Nagarjuna Kristam Dec. 27, 2019, 6:18 a.m. UTC | #3
On 26-12-2019 12:12, JC Kuo wrote:
> 
> On 12/18/19 5:16 PM, 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@nvidia.com>
>> ---
>> V2:
>>   - Removed dev_set_drvdata for port->dev.
>>   - Added of_platform_depopulate during error handling and driver removal.
>> ---
>>   drivers/phy/tegra/Kconfig |  1 +
>>   drivers/phy/tegra/xusb.c  | 42 ++++++++++++++++++++++++++++++++++++++++++
>>   drivers/phy/tegra/xusb.h  |  3 +++
>>   3 files changed, 46 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..dc00b42 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)
>> @@ -541,6 +542,10 @@ static int tegra_xusb_port_init(struct tegra_xusb_port *port,
>>   
>>   static void tegra_xusb_port_unregister(struct tegra_xusb_port *port)
>>   {
>> +	if (!IS_ERR_OR_NULL(port->usb_role_sw)) {
>> +		of_platform_depopulate(&port->dev);
>> +		usb_role_switch_unregister(port->usb_role_sw);
>> +	}
>>   	device_unregister(&port->dev);
>>   }
>>   
>> @@ -551,11 +556,42 @@ 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);
>> +	}
>> +
>> +	/* populate connector entry */
>> +	of_platform_populate(port->dev.of_node, NULL, NULL, &port->dev);
>> +
>> +	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 +608,12 @@ static int tegra_xusb_usb2_port_parse_dt(struct tegra_xusb_usb2_port *usb2)
>>   		usb2->mode = USB_DR_MODE_HOST;
>>   	}
>>   
> "usb-role-switch" is not required for host mode port. If a otg or a peripheral
> port doesn't have "usb-role-switch" property, .probe() should abort.
> 
> Thanks,
> JC
> 

Yes, will add the checks accordingly.
-nagarjuna

>> +	if (of_property_read_bool(np, "usb-role-switch")) {
>> +		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;
>>   };
>>   
>>
Nagarjuna Kristam Dec. 27, 2019, 6:39 a.m. UTC | #4
On 19-12-2019 18:56, Thierry Reding wrote:
> 
> On Wed, Dec 18, 2019 at 02:46:16PM +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@nvidia.com>
>> ---
>> V2:
>>   - Removed dev_set_drvdata for port->dev.
>>   - Added of_platform_depopulate during error handling and driver removal.
>> ---
>>   drivers/phy/tegra/Kconfig |  1 +
>>   drivers/phy/tegra/xusb.c  | 42 ++++++++++++++++++++++++++++++++++++++++++
>>   drivers/phy/tegra/xusb.h  |  3 +++
>>   3 files changed, 46 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..dc00b42 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)
>> @@ -541,6 +542,10 @@ static int tegra_xusb_port_init(struct tegra_xusb_port *port,
>>   
>>   static void tegra_xusb_port_unregister(struct tegra_xusb_port *port)
>>   {
>> +	if (!IS_ERR_OR_NULL(port->usb_role_sw)) {
>> +		of_platform_depopulate(&port->dev);
>> +		usb_role_switch_unregister(port->usb_role_sw);
>> +	}
>>   	device_unregister(&port->dev);
> Nit: I prefer blank lines after blocks for readability.
> 
Will update

>>   }
>>   
>> @@ -551,11 +556,42 @@ 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);
> I don't understand what "for role is %d" means here. I think perhaps you
> meant to simply say "for role %d"? Also, perhaps add parentheses after
> the "%s" to clarify that you're referring to a function.
> 
Yes, intention is to print role, Will update as mentioned "for role %d"

>> +
>> +	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),
>> +						   };
> The indentation here is odd. Use a single tab to indent lines after the
> opening { and put the closing } on the same column as "struct". Also,
> the above may become more readable if you follow the "inverse Christmas
> tree" style of declaring functions, where you order lines by their
> length, with the longest line first, like so:
> 
> 	struct usb_role_switch_desc role_sx_desc = {
> 		.fwnode = dev_fwnode(&port->dev),
> 		.set = tegra_xusb_role_sw_set,
> 	};
> 	int err = 0;
> 
Thanks for inputs, will update accordingly.

>> +
>> +	port->usb_role_sw = usb_role_switch_register(&port->dev,
>> +						&role_sx_desc);
> &role_sx_desc should be aligned with &port->dev.
> 
Will align here and at other places wherever missed.

>> +	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",
> Error messages typically start with a lowercase letter (at least in this
> driver). Also perhaps spell out "switch" above because SW could easily
> be confused with "software".
> 
Will update.

>> +				err);
> Shouldn't we abort here? Consider the case where this indeed defers
> probe. If we don't abort here, the of_platform_populate() below will be
> called multiple times. Shouldn't it only be called when we actually
> succeed in registering the switch?
> 
Yes, we should abort here, "return err;" got moved to next patch during
re-base. Will move to current patch.
> Also, looking at usb_role_switch_register(), I don't think it ever can
> return -EPROBE_DEFER, so I think you can drop that check and print the
> error unconditionally.
> 
> Thierry
> 
Will update.
-Nagarjuna
>> +	}
>> +
>> +	/* populate connector entry */
>> +	of_platform_populate(port->dev.of_node, NULL, NULL, &port->dev);
>> +
>> +	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 +608,12 @@ 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")) {
>> +		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
Thierry Reding Dec. 29, 2019, 9:36 a.m. UTC | #5
On Fri, Dec 27, 2019 at 12:09:22PM +0530, Nagarjuna Kristam wrote:
> On 19-12-2019 18:56, Thierry Reding wrote:
> > On Wed, Dec 18, 2019 at 02:46:16PM +0530, Nagarjuna Kristam wrote:
[...]
> > > @@ -551,11 +556,42 @@ 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);
> > I don't understand what "for role is %d" means here. I think perhaps you
> > meant to simply say "for role %d"? Also, perhaps add parentheses after
> > the "%s" to clarify that you're referring to a function.
> > 
> Yes, intention is to print role, Will update as mentioned "for role %d"

It might be better to print the name of the role here because I suspect
very few people know the value of the role defines without looking them
up. I thought there was already a function to return the role name for
a given enum usb_role, but it seems like we only have name -> enum
conversion. Might be worth adding that, but doesn't have to be as part
of this series.

Thierry
Nagarjuna Kristam Dec. 30, 2019, 5:17 a.m. UTC | #6
On 29-12-2019 15:06, Thierry Reding wrote:
> On Fri, Dec 27, 2019 at 12:09:22PM +0530, Nagarjuna Kristam wrote:
>> On 19-12-2019 18:56, Thierry Reding wrote:
>>> On Wed, Dec 18, 2019 at 02:46:16PM +0530, Nagarjuna Kristam wrote:
> [...]
>>>> @@ -551,11 +556,42 @@ 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);
>>> I don't understand what "for role is %d" means here. I think perhaps you
>>> meant to simply say "for role %d"? Also, perhaps add parentheses after
>>> the "%s" to clarify that you're referring to a function.
>>>
>> Yes, intention is to print role, Will update as mentioned "for role %d"
> It might be better to print the name of the role here because I suspect
> very few people know the value of the role defines without looking them
> up. I thought there was already a function to return the role name for
> a given enum usb_role, but it seems like we only have name -> enum
> conversion. Might be worth adding that, but doesn't have to be as part
> of this series.
> 
> Thierry

Will add a structure to define strings for roles enums, similar to modes
usage in this driver.

-Nagarjuna
diff mbox series

Patch

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..dc00b42 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)
@@ -541,6 +542,10 @@  static int tegra_xusb_port_init(struct tegra_xusb_port *port,
 
 static void tegra_xusb_port_unregister(struct tegra_xusb_port *port)
 {
+	if (!IS_ERR_OR_NULL(port->usb_role_sw)) {
+		of_platform_depopulate(&port->dev);
+		usb_role_switch_unregister(port->usb_role_sw);
+	}
 	device_unregister(&port->dev);
 }
 
@@ -551,11 +556,42 @@  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);
+	}
+
+	/* populate connector entry */
+	of_platform_populate(port->dev.of_node, NULL, NULL, &port->dev);
+
+	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 +608,12 @@  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")) {
+		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;
 };