From: Nagarjuna Kristam <nkristam@nvidia.com> To: Thierry Reding <thierry.reding@gmail.com> Cc: balbi@kernel.org, gregkh@linuxfoundation.org, jonathanh@nvidia.com, mark.rutland@arm.com, robh+dt@kernel.org, kishon@ti.com, devicetree@vger.kernel.org, linux-tegra@vger.kernel.org, linux-usb@vger.kernel.org, linux-kernel@vger.kernel.org Subject: Re: [Patch V2 03/18] phy: tegra: xusb: Add usb-role-switch support Date: Fri, 27 Dec 2019 12:09:22 +0530 [thread overview] Message-ID: <74d94004-2022-3b04-6cd9-d6b1e9eca230@nvidia.com> (raw) In-Reply-To: <20191219132650.GJ1440537@ulmo> 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
WARNING: multiple messages have this Message-ID (diff)
From: Nagarjuna Kristam <nkristam@nvidia.com> To: Thierry Reding <thierry.reding@gmail.com> Cc: <balbi@kernel.org>, <gregkh@linuxfoundation.org>, <jonathanh@nvidia.com>, <mark.rutland@arm.com>, <robh+dt@kernel.org>, <kishon@ti.com>, <devicetree@vger.kernel.org>, <linux-tegra@vger.kernel.org>, <linux-usb@vger.kernel.org>, <linux-kernel@vger.kernel.org> Subject: Re: [Patch V2 03/18] phy: tegra: xusb: Add usb-role-switch support Date: Fri, 27 Dec 2019 12:09:22 +0530 [thread overview] Message-ID: <74d94004-2022-3b04-6cd9-d6b1e9eca230@nvidia.com> (raw) In-Reply-To: <20191219132650.GJ1440537@ulmo> 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
next prev parent reply other threads:[~2019-12-27 6:39 UTC|newest] Thread overview: 67+ messages / expand[flat|nested] mbox.gz Atom feed top 2019-12-18 9:16 [Patch V2 00/18] Tegra XUSB OTG support Nagarjuna Kristam 2019-12-18 9:16 ` Nagarjuna Kristam 2019-12-18 9:16 ` [Patch V2 01/18] dt-bindings: phy: tegra-xusb: Add usb-role-switch Nagarjuna Kristam 2019-12-18 9:16 ` Nagarjuna Kristam 2019-12-19 13:05 ` Thierry Reding 2019-12-20 8:08 ` JC Kuo 2019-12-20 8:08 ` JC Kuo [not found] ` <fe47fd52-efd0-4f84-d1e4-4bce5571e425-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org> 2020-01-10 11:16 ` Thierry Reding 2020-01-10 11:16 ` Thierry Reding 2020-01-13 4:37 ` Nagarjuna Kristam 2020-01-13 4:37 ` Nagarjuna Kristam [not found] ` <6eb31a50-472f-439c-3bc7-bcad1c0c9ff4-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org> 2020-01-13 15:06 ` Thierry Reding 2020-01-13 15:06 ` Thierry Reding 2019-12-18 9:16 ` [Patch V2 02/18] dt-bindings: usb: Add NVIDIA Tegra XUSB device mode controller binding Nagarjuna Kristam 2019-12-18 9:16 ` Nagarjuna Kristam 2019-12-18 23:24 ` Rob Herring 2019-12-19 13:10 ` Thierry Reding 2019-12-18 9:16 ` [Patch V2 03/18] phy: tegra: xusb: Add usb-role-switch support Nagarjuna Kristam 2019-12-18 9:16 ` Nagarjuna Kristam 2019-12-19 13:26 ` Thierry Reding 2019-12-27 6:39 ` Nagarjuna Kristam [this message] 2019-12-27 6:39 ` Nagarjuna Kristam 2019-12-29 9:36 ` Thierry Reding 2019-12-30 5:17 ` Nagarjuna Kristam 2019-12-30 5:17 ` Nagarjuna Kristam 2019-12-26 6:42 ` JC Kuo 2019-12-26 6:42 ` JC Kuo 2019-12-27 6:18 ` Nagarjuna Kristam 2019-12-27 6:18 ` Nagarjuna Kristam 2019-12-18 9:16 ` [Patch V2 04/18] phy: tegra: xusb: Add usb-phy support Nagarjuna Kristam 2019-12-18 9:16 ` Nagarjuna Kristam 2019-12-19 13:37 ` Thierry Reding 2019-12-27 7:06 ` Nagarjuna Kristam 2019-12-27 7:06 ` Nagarjuna Kristam 2019-12-18 9:16 ` [Patch V2 05/18] phy: tegra: xusb: Add support to get companion USB 3 port Nagarjuna Kristam 2019-12-18 9:16 ` Nagarjuna Kristam 2019-12-26 7:03 ` JC Kuo 2019-12-26 7:03 ` JC Kuo 2019-12-18 9:16 ` [Patch V2 06/18] phy: tegra: xusb: Add set_mode support for USB 2 phy on Tegra210 Nagarjuna Kristam 2019-12-18 9:16 ` Nagarjuna Kristam 2019-12-18 9:16 ` [Patch V2 07/18] phy: tegra: xusb: Add set_mode support for utmi phy on Tegra186 Nagarjuna Kristam 2019-12-18 9:16 ` Nagarjuna Kristam 2019-12-18 9:16 ` [Patch V2 08/18] usb: xhci-tegra: Add OTG support Nagarjuna Kristam 2019-12-18 9:16 ` Nagarjuna Kristam 2019-12-18 9:16 ` [Patch V2 09/18] usb: gadget: tegra-xudc: Remove usb-role-switch support Nagarjuna Kristam 2019-12-18 9:16 ` Nagarjuna Kristam 2019-12-18 9:16 ` [Patch V2 10/18] usb: gadget: tegra-xudc: Add usb-phy support Nagarjuna Kristam 2019-12-18 9:16 ` Nagarjuna Kristam 2019-12-18 9:16 ` [Patch V2 11/18] usb: gadget: tegra-xudc: use phy_set_mode to set/unset device mode Nagarjuna Kristam 2019-12-18 9:16 ` Nagarjuna Kristam 2019-12-18 9:16 ` [Patch V2 12/18] usb: gadget: tegra-xudc: support multiple device modes Nagarjuna Kristam 2019-12-18 9:16 ` Nagarjuna Kristam 2019-12-18 9:16 ` [Patch V2 13/18] arm64: tegra: update OTG port entries for jetson-tx1 Nagarjuna Kristam 2019-12-18 9:16 ` Nagarjuna Kristam 2019-12-18 9:16 ` [Patch V2 14/18] arm64: tegra: update OTG port entries for jetson-tx2 Nagarjuna Kristam 2019-12-18 9:16 ` Nagarjuna Kristam 2019-12-18 9:16 ` [Patch V2 15/18] arm64: tegra: Add xudc node for Tegra210 Nagarjuna Kristam 2019-12-18 9:16 ` Nagarjuna Kristam 2019-12-18 9:16 ` [Patch V2 16/18] arm64: tegra: Enable xudc on Jetson TX1 Nagarjuna Kristam 2019-12-18 9:16 ` Nagarjuna Kristam 2019-12-18 9:16 ` [Patch V2 17/18] arm64: tegra: Add xudc node for Tegra186 Nagarjuna Kristam 2019-12-18 9:16 ` Nagarjuna Kristam 2019-12-18 9:16 ` [Patch V2 18/18] arm64: tegra: Enable xudc node on Jetson TX2 Nagarjuna Kristam 2019-12-18 9:16 ` Nagarjuna Kristam 2019-12-19 13:13 ` [Patch V2 00/18] Tegra XUSB OTG support Thierry Reding 2019-12-20 7:35 ` JC Kuo 2019-12-20 7:35 ` JC Kuo
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=74d94004-2022-3b04-6cd9-d6b1e9eca230@nvidia.com \ --to=nkristam@nvidia.com \ --cc=balbi@kernel.org \ --cc=devicetree@vger.kernel.org \ --cc=gregkh@linuxfoundation.org \ --cc=jonathanh@nvidia.com \ --cc=kishon@ti.com \ --cc=linux-kernel@vger.kernel.org \ --cc=linux-tegra@vger.kernel.org \ --cc=linux-usb@vger.kernel.org \ --cc=mark.rutland@arm.com \ --cc=robh+dt@kernel.org \ --cc=thierry.reding@gmail.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.