On Sat, Oct 07, 2023 at 09:17:59PM +0530, Krishna Kurapati wrote: > From: Harsh Agarwal <quic_harshq@xxxxxxxxxxx> > > Currently the DWC3 driver supports only single port controller > which requires at most one HS and one SS PHY. Should that not be "at least one HS PHY and at most one SS PHY"? > But the DWC3 USB controller can be connected to multiple ports and > each port can have their own PHYs. Each port of the multiport > controller can either be HS+SS capable or HS only capable > Proper quantification of them is required to modify GUSB2PHYCFG > and GUSB3PIPECTL registers appropriately. > > Add support for detecting, obtaining and configuring phy's supported "PHYs" for consistency, no apostrophe > by a multiport controller and. Limit the max number of ports "and." what? Looks like part of the sentence is missing? Or just drop " and"? > supported to 4 as only SC8280 which is a quad port controller supports s/4/four/ Just change this to Limit support to multiport controllers with up to four ports for now (e.g. as needed for SC8280XP). > Multiport currently. You use capitalised "Multiport" in several places it seems. Is this an established term for these controllers or should it just be "multiport" or "multiple ports"? > Reported-by: kernel test robot <lkp@xxxxxxxxx> > Closes: https://lore.kernel.org/oe-kbuild-all/202309200156.CxQ3yaLY-lkp@xxxxxxxxx/ Drop these two lines, as people have already suggested. > Co-developed-by: Harsh Agarwal <quic_harshq@xxxxxxxxxxx> > Signed-off-by: Harsh Agarwal <quic_harshq@xxxxxxxxxxx> > Co-developed-by:Krishna Kurapati <quic_kriskura@xxxxxxxxxxx> > Signed-off-by: Krishna Kurapati <quic_kriskura@xxxxxxxxxxx> Thinh pointed out the problems with the above which were also reported by checkpatch.pl. > --- > Changes in v13: > Compiler issues found by kernel test robot have been fixed and tags added. > So removing maintainers reviewed-by tag as we have made a minor change > in the patch. In general this is the right thing to do when the change in question was non-trivial. I'm not sure that's the case here, but the robots tend to complain about smaller (but sometimes important) things. > @@ -748,23 +781,32 @@ static int dwc3_phy_setup(struct dwc3 *dwc) > static int dwc3_phy_init(struct dwc3 *dwc) > { > int ret; > + int i; > + int j; These could be declared on one line (same throughout). > usb_phy_init(dwc->usb2_phy); > usb_phy_init(dwc->usb3_phy); > > - ret = phy_init(dwc->usb2_generic_phy); > - if (ret < 0) > - goto err_shutdown_usb3_phy; > + for (i = 0; i < dwc->num_usb2_ports; i++) { > + ret = phy_init(dwc->usb2_generic_phy[i]); > + if (ret < 0) > + goto err_exit_phy; > > - ret = phy_init(dwc->usb3_generic_phy); > - if (ret < 0) > - goto err_exit_usb2_phy; > + ret = phy_init(dwc->usb3_generic_phy[i]); > + if (ret < 0) { > + phy_exit(dwc->usb2_generic_phy[i]); > + goto err_exit_phy; > + } > + } > > return 0; > > -err_exit_usb2_phy: > - phy_exit(dwc->usb2_generic_phy); > -err_shutdown_usb3_phy: > +err_exit_phy: > + for (j = i - 1; j >= 0; j--) { > + phy_exit(dwc->usb2_generic_phy[j]); > + phy_exit(dwc->usb3_generic_phy[j]); Try to always unwind in reverse order so in this case move phy_exit() for usb3 before usb2 here too. > + } > + > usb_phy_shutdown(dwc->usb3_phy); > usb_phy_shutdown(dwc->usb2_phy); > @@ -783,23 +829,32 @@ static void dwc3_phy_exit(struct dwc3 *dwc) > static int dwc3_phy_power_on(struct dwc3 *dwc) > { > int ret; > + int i; > + int j; > > usb_phy_set_suspend(dwc->usb2_phy, 0); > usb_phy_set_suspend(dwc->usb3_phy, 0); > > - ret = phy_power_on(dwc->usb2_generic_phy); > - if (ret < 0) > - goto err_suspend_usb3_phy; > + for (i = 0; i < dwc->num_usb2_ports; i++) { > + ret = phy_power_on(dwc->usb2_generic_phy[i]); > + if (ret < 0) > + goto err_power_off_phy; > > - ret = phy_power_on(dwc->usb3_generic_phy); > - if (ret < 0) > - goto err_power_off_usb2_phy; > + ret = phy_power_on(dwc->usb3_generic_phy[i]); > + if (ret < 0) { > + phy_power_off(dwc->usb2_generic_phy[i]); > + goto err_power_off_phy; > + } > + } > > return 0; > > -err_power_off_usb2_phy: > - phy_power_off(dwc->usb2_generic_phy); > -err_suspend_usb3_phy: > +err_power_off_phy: > + for (j = i - 1; j >= 0; j--) { > + phy_power_off(dwc->usb2_generic_phy[j]); > + phy_power_off(dwc->usb3_generic_phy[j]); Same here. > + } > + > usb_phy_set_suspend(dwc->usb3_phy, 1); > usb_phy_set_suspend(dwc->usb2_phy, 1); > @@ -1346,7 +1410,9 @@ static int dwc3_core_get_phy(struct dwc3 *dwc) > { > struct device *dev = dwc->dev; > struct device_node *node = dev->of_node; > + char phy_name[13]; > int ret; > + int i; > > if (node) { > dwc->usb2_phy = devm_usb_get_phy_by_phandle(dev, "usb-phy", 0); > @@ -1372,22 +1438,36 @@ static int dwc3_core_get_phy(struct dwc3 *dwc) > return dev_err_probe(dev, ret, "no usb3 phy configured\n"); > } > > - dwc->usb2_generic_phy = devm_phy_get(dev, "usb2-phy"); > - if (IS_ERR(dwc->usb2_generic_phy)) { > - ret = PTR_ERR(dwc->usb2_generic_phy); > - if (ret == -ENOSYS || ret == -ENODEV) > - dwc->usb2_generic_phy = NULL; > + for (i = 0; i < dwc->num_usb2_ports; i++) { > + if (dwc->num_usb2_ports == 1) > + sprintf(phy_name, "usb2-phy"); > else > - return dev_err_probe(dev, ret, "no usb2 phy configured\n"); > - } > + sprintf(phy_name, "usb2-port%d", i); > > - dwc->usb3_generic_phy = devm_phy_get(dev, "usb3-phy"); > - if (IS_ERR(dwc->usb3_generic_phy)) { > - ret = PTR_ERR(dwc->usb3_generic_phy); > - if (ret == -ENOSYS || ret == -ENODEV) > - dwc->usb3_generic_phy = NULL; > + dwc->usb2_generic_phy[i] = devm_phy_get(dev, phy_name); > + if (IS_ERR(dwc->usb2_generic_phy[i])) { > + ret = PTR_ERR(dwc->usb2_generic_phy[i]); > + if (ret == -ENOSYS || ret == -ENODEV) > + dwc->usb2_generic_phy[i] = NULL; > + else > + return dev_err_probe(dev, ret, > + "failed to lookup phy %s\n", phy_name); Continuation lines should be intented at least two tabs further. I generally suggest adding brackets around blocks with multiline statements to improve readability but if you move the string to the previous line and intend the continuation line (phy_name) one tab more I guess that's fine. > + } > + > + if (dwc->num_usb2_ports == 1) > + sprintf(phy_name, "usb3-phy"); > else > - return dev_err_probe(dev, ret, "no usb3 phy configured\n"); > + sprintf(phy_name, "usb3-port%d", i); > + > + dwc->usb3_generic_phy[i] = devm_phy_get(dev, phy_name); > + if (IS_ERR(dwc->usb3_generic_phy[i])) { > + ret = PTR_ERR(dwc->usb3_generic_phy[i]); > + if (ret == -ENOSYS || ret == -ENODEV) > + dwc->usb3_generic_phy[i] = NULL; > + else > + return dev_err_probe(dev, ret, > + "failed to lookup phy %s\n", phy_name); Same here. > + } > } > > return 0; > @@ -1892,9 +1975,12 @@ static int dwc3_read_port_info(struct dwc3 *dwc) > > dev_dbg(dwc->dev, "hs-ports: %u ss-ports: %u\n", > dwc->num_usb2_ports, dwc->num_usb3_ports); > - Drop this random change. > iounmap(base); > > + if ((dwc->num_usb2_ports > DWC3_MAX_PORTS) || > + (dwc->num_usb3_ports > DWC3_MAX_PORTS)) Again, continuation lines should be indented at least two tabs further. > + return -ENOMEM; > + > return 0; > } > diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h > index 2ea6df7e6571..fc5d15edab1c 100644 > --- a/drivers/usb/dwc3/core.h > +++ b/drivers/usb/dwc3/core.h > @@ -33,6 +33,9 @@ > > #include <linux/power_supply.h> > > +/* Number of ports supported by a multiport controller */ /* * Maximum number of ports currently supported for multiport * controllers. */ > +#define DWC3_MAX_PORTS 4 > + > #define DWC3_MSG_MAX 500 > > /* Global constants */ > @@ -1029,8 +1032,8 @@ struct dwc3_scratchpad_array { > * @usb_psy: pointer to power supply interface. > * @usb2_phy: pointer to USB2 PHY > * @usb3_phy: pointer to USB3 PHY > - * @usb2_generic_phy: pointer to USB2 PHY > - * @usb3_generic_phy: pointer to USB3 PHY > + * @usb2_generic_phy: pointer to array of USB2 PHY > + * @usb3_generic_phy: pointer to array of USB3 PHY s/PHY/PHYs/ > * @num_usb2_ports: number of USB2 ports > * @num_usb3_ports: number of USB3 ports > * @phys_ready: flag to indicate that PHYs are ready Johan
