| Message ID | 20190506234631.113226-4-chris.brandt@renesas.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | usb: Add host and device support for RZ/A2 | expand |
On 07.05.2019 2:46, Chris Brandt wrote: > When not using OTG, the PHY will need to know if it should function as > host or peripheral by checking dr_mode in the PHY node (not the parent > controller node). > > Signed-off-by: Chris Brandt <chris.brandt@renesas.com> > --- > drivers/phy/renesas/phy-rcar-gen3-usb2.c | 8 +++++++- > 1 file changed, 7 insertions(+), 1 deletion(-) > > diff --git a/drivers/phy/renesas/phy-rcar-gen3-usb2.c b/drivers/phy/renesas/phy-rcar-gen3-usb2.c > index 218b32e458cb..4eaa228ebd30 100644 > --- a/drivers/phy/renesas/phy-rcar-gen3-usb2.c > +++ b/drivers/phy/renesas/phy-rcar-gen3-usb2.c > @@ -408,7 +408,12 @@ static int rcar_gen3_phy_usb2_init(struct phy *p) > if (rcar_gen3_needs_init_otg(channel)) > rcar_gen3_init_otg(channel); > rphy->otg_initialized = true; > - } > + } else > + /* Not OTG, so dr_mode should be set in PHY node */ > + if (usb_get_dr_mode(channel->dev) == USB_DR_MODE_PERIPHERAL) > + writel(0x80000000, usb2_base + USB2_COMMCTRL); > + else > + writel(0x00000000, usb2_base + USB2_COMMCTRL); > > rphy->initialized = true; > > @@ -638,6 +643,7 @@ static int rcar_gen3_phy_usb2_probe(struct platform_device *pdev) > if (of_property_read_bool(dev->of_node, "renesas,uses_usb_x1")) > channel->uses_usb_x1 = true; > > + Unrelated whitespace tweaking? :-) > /* > * devm_phy_create() will call pm_runtime_enable(&phy->dev); > * And then, phy-core will manage runtime pm for this device. MBR, Sergei
On 05/07/2019 02:46 AM, Chris Brandt wrote: > When not using OTG, the PHY will need to know if it should function as > host or peripheral by checking dr_mode in the PHY node (not the parent > controller node). > > Signed-off-by: Chris Brandt <chris.brandt@renesas.com> > --- > drivers/phy/renesas/phy-rcar-gen3-usb2.c | 8 +++++++- > 1 file changed, 7 insertions(+), 1 deletion(-) > > diff --git a/drivers/phy/renesas/phy-rcar-gen3-usb2.c b/drivers/phy/renesas/phy-rcar-gen3-usb2.c > index 218b32e458cb..4eaa228ebd30 100644 > --- a/drivers/phy/renesas/phy-rcar-gen3-usb2.c > +++ b/drivers/phy/renesas/phy-rcar-gen3-usb2.c > @@ -408,7 +408,12 @@ static int rcar_gen3_phy_usb2_init(struct phy *p) > if (rcar_gen3_needs_init_otg(channel)) > rcar_gen3_init_otg(channel); > rphy->otg_initialized = true; > - } > + } else Wait, don't we neeed {} here? > + /* Not OTG, so dr_mode should be set in PHY node */ > + if (usb_get_dr_mode(channel->dev) == USB_DR_MODE_PERIPHERAL) > + writel(0x80000000, usb2_base + USB2_COMMCTRL); > + else > + writel(0x00000000, usb2_base + USB2_COMMCTRL); > > rphy->initialized = true; > MBR, Sergei
On Tue, May 07, 2019, Sergei Shtylyov wrote: > > --- a/drivers/phy/renesas/phy-rcar-gen3-usb2.c > > +++ b/drivers/phy/renesas/phy-rcar-gen3-usb2.c > > @@ -408,7 +408,12 @@ static int rcar_gen3_phy_usb2_init(struct phy *p) > > if (rcar_gen3_needs_init_otg(channel)) > > rcar_gen3_init_otg(channel); > > rphy->otg_initialized = true; > > - } > > + } else > > Wait, don't we neeed {} here? > > > + /* Not OTG, so dr_mode should be set in PHY node */ > > + if (usb_get_dr_mode(channel->dev) == USB_DR_MODE_PERIPHERAL) > > + writel(0x80000000, usb2_base + USB2_COMMCTRL); > > + else > > + writel(0x00000000, usb2_base + USB2_COMMCTRL); Technically there is only 1 statement after the else (the 'if' which will also include the 'else') statement. The coding rules say not to use { } if there is only 1 statement. Chris
On 05/07/2019 02:45 PM, Chris Brandt wrote: >>> --- a/drivers/phy/renesas/phy-rcar-gen3-usb2.c >>> +++ b/drivers/phy/renesas/phy-rcar-gen3-usb2.c >>> @@ -408,7 +408,12 @@ static int rcar_gen3_phy_usb2_init(struct phy *p) >>> if (rcar_gen3_needs_init_otg(channel)) >>> rcar_gen3_init_otg(channel); >>> rphy->otg_initialized = true; >>> - } >>> + } else >> >> Wait, don't we neeed {} here? >> >>> + /* Not OTG, so dr_mode should be set in PHY node */ >>> + if (usb_get_dr_mode(channel->dev) == USB_DR_MODE_PERIPHERAL) >>> + writel(0x80000000, usb2_base + USB2_COMMCTRL); >>> + else >>> + writel(0x00000000, usb2_base + USB2_COMMCTRL); > > Technically there is only 1 statement after the else (the 'if' which > will also include the 'else') statement. The coding rules say not to use > { } if there is only 1 statement. Don't you remember another rule: use {} in all branches if at least one branch uses {}? > Chris MBR, Sergei
On Tue, May 07, 2019 1, Sergei Shtylyov wrote: > Don't you remember another rule: use {} in all branches if at least > one branch uses {}? Ah, I see it now. Documentation/process/coding-style.rst: This does not apply if only one branch of a conditional statement is a single statement; in the latter case use braces in both branches: .. code-block:: c if (condition) { do_this(); do_that(); } else { otherwise(); } I will change it. Thanks! Chris
Hi Chrisさん Thank you for the patch! > From: Chris Brandt, Sent: Tuesday, May 7, 2019 8:46 AM > > When not using OTG, the PHY will need to know if it should function as > host or peripheral by checking dr_mode in the PHY node (not the parent > controller node). > > Signed-off-by: Chris Brandt <chris.brandt@renesas.com> > --- > drivers/phy/renesas/phy-rcar-gen3-usb2.c | 8 +++++++- > 1 file changed, 7 insertions(+), 1 deletion(-) > > diff --git a/drivers/phy/renesas/phy-rcar-gen3-usb2.c b/drivers/phy/renesas/phy-rcar-gen3-usb2.c > index 218b32e458cb..4eaa228ebd30 100644 > --- a/drivers/phy/renesas/phy-rcar-gen3-usb2.c > +++ b/drivers/phy/renesas/phy-rcar-gen3-usb2.c > @@ -408,7 +408,12 @@ static int rcar_gen3_phy_usb2_init(struct phy *p) > if (rcar_gen3_needs_init_otg(channel)) > rcar_gen3_init_otg(channel); > rphy->otg_initialized = true; > - } > + } else As Sergei-san said, this should be "} else {" > + /* Not OTG, so dr_mode should be set in PHY node */ > + if (usb_get_dr_mode(channel->dev) == USB_DR_MODE_PERIPHERAL) > + writel(0x80000000, usb2_base + USB2_COMMCTRL); > + else I would like to add "else if usb_get_dr_mode(channel->dev) == USB_DR_MODE_HOST)" for a PHY node without "dr_mode" property. In other words, if the PHY node doesn't have dr_mode property like R-Car, this condition can be the same behavior as previous. > + writel(0x00000000, usb2_base + USB2_COMMCTRL); > > rphy->initialized = true; > > @@ -638,6 +643,7 @@ static int rcar_gen3_phy_usb2_probe(struct platform_device *pdev) > if (of_property_read_bool(dev->of_node, "renesas,uses_usb_x1")) > channel->uses_usb_x1 = true; > > + As Sergei-san said, this is not needed :) Best regards, Yoshihiro Shimoda
diff --git a/drivers/phy/renesas/phy-rcar-gen3-usb2.c b/drivers/phy/renesas/phy-rcar-gen3-usb2.c index 218b32e458cb..4eaa228ebd30 100644 --- a/drivers/phy/renesas/phy-rcar-gen3-usb2.c +++ b/drivers/phy/renesas/phy-rcar-gen3-usb2.c @@ -408,7 +408,12 @@ static int rcar_gen3_phy_usb2_init(struct phy *p) if (rcar_gen3_needs_init_otg(channel)) rcar_gen3_init_otg(channel); rphy->otg_initialized = true; - } + } else + /* Not OTG, so dr_mode should be set in PHY node */ + if (usb_get_dr_mode(channel->dev) == USB_DR_MODE_PERIPHERAL) + writel(0x80000000, usb2_base + USB2_COMMCTRL); + else + writel(0x00000000, usb2_base + USB2_COMMCTRL); rphy->initialized = true; @@ -638,6 +643,7 @@ static int rcar_gen3_phy_usb2_probe(struct platform_device *pdev) if (of_property_read_bool(dev->of_node, "renesas,uses_usb_x1")) channel->uses_usb_x1 = true; + /* * devm_phy_create() will call pm_runtime_enable(&phy->dev); * And then, phy-core will manage runtime pm for this device.
When not using OTG, the PHY will need to know if it should function as host or peripheral by checking dr_mode in the PHY node (not the parent controller node). Signed-off-by: Chris Brandt <chris.brandt@renesas.com> --- drivers/phy/renesas/phy-rcar-gen3-usb2.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-)