| Message ID | 1576660591-10383-4-git-send-email-nkristam@nvidia.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Tegra XUSB OTG support | expand |
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 >
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; > }; > >
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; >> }; >> >>
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
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
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 --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; };
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(+)