| Message ID | 1507615110-26206-3-git-send-email-yoshihiro.shimoda.uh@renesas.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Geert Uytterhoeven |
| Headers | show |
On Tue, Oct 10, 2017 at 02:58:28PM +0900, Yoshihiro Shimoda wrote: > This patch cleanups the role_{store,show}() and replaces the local > "bool" for host/device mode selection with the "enum phy_mode" in > the role_store(). > > Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com> > --- > drivers/phy/renesas/phy-rcar-gen3-usb2.c | 58 ++++++++++++++++++++++---------- > 1 file changed, 40 insertions(+), 18 deletions(-) > > diff --git a/drivers/phy/renesas/phy-rcar-gen3-usb2.c b/drivers/phy/renesas/phy-rcar-gen3-usb2.c > index e00e99a..7619468 100644 > --- a/drivers/phy/renesas/phy-rcar-gen3-usb2.c > +++ b/drivers/phy/renesas/phy-rcar-gen3-usb2.c > @@ -219,14 +219,10 @@ static bool rcar_gen3_is_host(struct rcar_gen3_chan *ch) > return !(readl(ch->base + USB2_COMMCTRL) & USB2_COMMCTRL_OTG_PERI); > } > > -static ssize_t role_store(struct device *dev, struct device_attribute *attr, > - const char *buf, size_t count) > +static ssize_t rcar_gen3_otg_change_role(struct rcar_gen3_chan *ch, > + enum phy_mode mode) > { > - struct rcar_gen3_chan *ch = dev_get_drvdata(dev); > - bool is_b_device, is_host, new_mode_is_host; > - > - if (!ch->has_otg || !ch->phy->init_count) > - return -EIO; > + bool is_b_device, is_host; > > /* > * is_b_device: true is B-Device. false is A-Device. > @@ -234,18 +230,13 @@ static ssize_t role_store(struct device *dev, struct device_attribute *attr, > */ > is_b_device = rcar_gen3_check_id(ch); > is_host = rcar_gen3_is_host(ch); > - if (!strncmp(buf, "host", strlen("host"))) > - new_mode_is_host = true; > - else if (!strncmp(buf, "peripheral", strlen("peripheral"))) > - new_mode_is_host = false; > - else > - return -EINVAL; > > /* If current and new mode is the same, this returns the error */ > - if (is_host == new_mode_is_host) > + if ((is_host && mode == PHY_MODE_USB_HOST) || > + (!is_host && mode == PHY_MODE_USB_DEVICE)) Maybe it is not worth the effort, but it seems that if rcar_gen3_is_host returned enum phy_mode then the above could be changed into a single comparison. > return -EINVAL; > > - if (new_mode_is_host) { /* And is_host must be false */ > + if (mode == PHY_MODE_USB_HOST) { /* And is_host must be false */ > if (!is_b_device) /* A-Peripheral */ > rcar_gen3_init_from_a_peri_to_a_host(ch); > else /* B-Peripheral */ > @@ -257,6 +248,32 @@ static ssize_t role_store(struct device *dev, struct device_attribute *attr, > rcar_gen3_init_for_peri(ch); > } > > + return 0; > +} > + > +static ssize_t role_store(struct device *dev, struct device_attribute *attr, > + const char *buf, size_t count) > +{ > + struct rcar_gen3_chan *ch = dev_get_drvdata(dev); > + enum phy_mode mode; > + ssize_t ret = -EIO; > + > + if (!ch->phy->init_count) > + return -EIO; > + > + if (!strncmp(buf, "host", strlen("host"))) > + mode = PHY_MODE_USB_HOST; > + else if (!strncmp(buf, "peripheral", strlen("peripheral"))) > + mode = PHY_MODE_USB_DEVICE; > + else > + return -EINVAL; > + > + if (ch->has_otg) > + ret = rcar_gen3_otg_change_role(ch, mode); > + > + if (ret < 0) > + return ret; > + > return count; > } > > @@ -264,12 +281,17 @@ static ssize_t role_show(struct device *dev, struct device_attribute *attr, > char *buf) > { > struct rcar_gen3_chan *ch = dev_get_drvdata(dev); > + bool is_host; > > - if (!ch->has_otg || !ch->phy->init_count) > + if (!ch->phy->init_count) > + return -EIO; > + > + if (ch->has_otg) > + is_host = rcar_gen3_is_host(ch); > + else > return -EIO; > > - return sprintf(buf, "%s\n", rcar_gen3_is_host(ch) ? "host" : > - "peripheral"); > + return sprintf(buf, "%s\n", is_host ? "host" : "peripheral"); > } I'm not sure why the above hunk is necessary. They function seems logically the same before and after. > static DEVICE_ATTR_RW(role); > > -- > 1.9.1 >
Hi Simon-san, Thank you for the comments! > From: Simon Horman, Sent: Tuesday, October 10, 2017 4:46 PM > > On Tue, Oct 10, 2017 at 02:58:28PM +0900, Yoshihiro Shimoda wrote: > > This patch cleanups the role_{store,show}() and replaces the local > > "bool" for host/device mode selection with the "enum phy_mode" in > > the role_store(). > > > > Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com> > > --- > > drivers/phy/renesas/phy-rcar-gen3-usb2.c | 58 ++++++++++++++++++++++---------- > > 1 file changed, 40 insertions(+), 18 deletions(-) > > > > diff --git a/drivers/phy/renesas/phy-rcar-gen3-usb2.c b/drivers/phy/renesas/phy-rcar-gen3-usb2.c > > index e00e99a..7619468 100644 > > --- a/drivers/phy/renesas/phy-rcar-gen3-usb2.c > > +++ b/drivers/phy/renesas/phy-rcar-gen3-usb2.c > > @@ -219,14 +219,10 @@ static bool rcar_gen3_is_host(struct rcar_gen3_chan *ch) > > return !(readl(ch->base + USB2_COMMCTRL) & USB2_COMMCTRL_OTG_PERI); > > } > > > > -static ssize_t role_store(struct device *dev, struct device_attribute *attr, > > - const char *buf, size_t count) > > +static ssize_t rcar_gen3_otg_change_role(struct rcar_gen3_chan *ch, > > + enum phy_mode mode) > > { > > - struct rcar_gen3_chan *ch = dev_get_drvdata(dev); > > - bool is_b_device, is_host, new_mode_is_host; > > - > > - if (!ch->has_otg || !ch->phy->init_count) > > - return -EIO; > > + bool is_b_device, is_host; > > > > /* > > * is_b_device: true is B-Device. false is A-Device. > > @@ -234,18 +230,13 @@ static ssize_t role_store(struct device *dev, struct device_attribute *attr, > > */ > > is_b_device = rcar_gen3_check_id(ch); > > is_host = rcar_gen3_is_host(ch); > > - if (!strncmp(buf, "host", strlen("host"))) > > - new_mode_is_host = true; > > - else if (!strncmp(buf, "peripheral", strlen("peripheral"))) > > - new_mode_is_host = false; > > - else > > - return -EINVAL; > > > > /* If current and new mode is the same, this returns the error */ > > - if (is_host == new_mode_is_host) > > + if ((is_host && mode == PHY_MODE_USB_HOST) || > > + (!is_host && mode == PHY_MODE_USB_DEVICE)) > > Maybe it is not worth the effort, but it seems that if > rcar_gen3_is_host returned enum phy_mode then the above > could be changed into a single comparison. It's a nice idea! Since role_show() below should not be changed, I will add rcar_gen3_is_phy_mode_host() and changed the above code into a single comparison. > > return -EINVAL; > > > > - if (new_mode_is_host) { /* And is_host must be false */ > > + if (mode == PHY_MODE_USB_HOST) { /* And is_host must be false */ > > if (!is_b_device) /* A-Peripheral */ > > rcar_gen3_init_from_a_peri_to_a_host(ch); > > else /* B-Peripheral */ > > @@ -257,6 +248,32 @@ static ssize_t role_store(struct device *dev, struct device_attribute *attr, > > rcar_gen3_init_for_peri(ch); > > } > > > > + return 0; > > +} > > + > > +static ssize_t role_store(struct device *dev, struct device_attribute *attr, > > + const char *buf, size_t count) > > +{ > > + struct rcar_gen3_chan *ch = dev_get_drvdata(dev); > > + enum phy_mode mode; > > + ssize_t ret = -EIO; > > + > > + if (!ch->phy->init_count) > > + return -EIO; > > + > > + if (!strncmp(buf, "host", strlen("host"))) > > + mode = PHY_MODE_USB_HOST; > > + else if (!strncmp(buf, "peripheral", strlen("peripheral"))) > > + mode = PHY_MODE_USB_DEVICE; > > + else > > + return -EINVAL; > > + > > + if (ch->has_otg) > > + ret = rcar_gen3_otg_change_role(ch, mode); > > + > > + if (ret < 0) > > + return ret; > > + > > return count; > > } > > > > @@ -264,12 +281,17 @@ static ssize_t role_show(struct device *dev, struct device_attribute *attr, > > char *buf) > > { > > struct rcar_gen3_chan *ch = dev_get_drvdata(dev); > > + bool is_host; > > > > - if (!ch->has_otg || !ch->phy->init_count) > > + if (!ch->phy->init_count) > > + return -EIO; > > + > > + if (ch->has_otg) > > + is_host = rcar_gen3_is_host(ch); > > + else > > return -EIO; > > > > - return sprintf(buf, "%s\n", rcar_gen3_is_host(ch) ? "host" : > > - "peripheral"); > > + return sprintf(buf, "%s\n", is_host ? "host" : "peripheral"); > > } > > I'm not sure why the above hunk is necessary. > They function seems logically the same before and after. Indeed. So, I will keep rcar_gen3_is_host() as bool and remove the role_show() modification. Best regards, Yoshihiro Shimoda > > static DEVICE_ATTR_RW(role); > > > > -- > > 1.9.1 > >
diff --git a/drivers/phy/renesas/phy-rcar-gen3-usb2.c b/drivers/phy/renesas/phy-rcar-gen3-usb2.c index e00e99a..7619468 100644 --- a/drivers/phy/renesas/phy-rcar-gen3-usb2.c +++ b/drivers/phy/renesas/phy-rcar-gen3-usb2.c @@ -219,14 +219,10 @@ static bool rcar_gen3_is_host(struct rcar_gen3_chan *ch) return !(readl(ch->base + USB2_COMMCTRL) & USB2_COMMCTRL_OTG_PERI); } -static ssize_t role_store(struct device *dev, struct device_attribute *attr, - const char *buf, size_t count) +static ssize_t rcar_gen3_otg_change_role(struct rcar_gen3_chan *ch, + enum phy_mode mode) { - struct rcar_gen3_chan *ch = dev_get_drvdata(dev); - bool is_b_device, is_host, new_mode_is_host; - - if (!ch->has_otg || !ch->phy->init_count) - return -EIO; + bool is_b_device, is_host; /* * is_b_device: true is B-Device. false is A-Device. @@ -234,18 +230,13 @@ static ssize_t role_store(struct device *dev, struct device_attribute *attr, */ is_b_device = rcar_gen3_check_id(ch); is_host = rcar_gen3_is_host(ch); - if (!strncmp(buf, "host", strlen("host"))) - new_mode_is_host = true; - else if (!strncmp(buf, "peripheral", strlen("peripheral"))) - new_mode_is_host = false; - else - return -EINVAL; /* If current and new mode is the same, this returns the error */ - if (is_host == new_mode_is_host) + if ((is_host && mode == PHY_MODE_USB_HOST) || + (!is_host && mode == PHY_MODE_USB_DEVICE)) return -EINVAL; - if (new_mode_is_host) { /* And is_host must be false */ + if (mode == PHY_MODE_USB_HOST) { /* And is_host must be false */ if (!is_b_device) /* A-Peripheral */ rcar_gen3_init_from_a_peri_to_a_host(ch); else /* B-Peripheral */ @@ -257,6 +248,32 @@ static ssize_t role_store(struct device *dev, struct device_attribute *attr, rcar_gen3_init_for_peri(ch); } + return 0; +} + +static ssize_t role_store(struct device *dev, struct device_attribute *attr, + const char *buf, size_t count) +{ + struct rcar_gen3_chan *ch = dev_get_drvdata(dev); + enum phy_mode mode; + ssize_t ret = -EIO; + + if (!ch->phy->init_count) + return -EIO; + + if (!strncmp(buf, "host", strlen("host"))) + mode = PHY_MODE_USB_HOST; + else if (!strncmp(buf, "peripheral", strlen("peripheral"))) + mode = PHY_MODE_USB_DEVICE; + else + return -EINVAL; + + if (ch->has_otg) + ret = rcar_gen3_otg_change_role(ch, mode); + + if (ret < 0) + return ret; + return count; } @@ -264,12 +281,17 @@ static ssize_t role_show(struct device *dev, struct device_attribute *attr, char *buf) { struct rcar_gen3_chan *ch = dev_get_drvdata(dev); + bool is_host; - if (!ch->has_otg || !ch->phy->init_count) + if (!ch->phy->init_count) + return -EIO; + + if (ch->has_otg) + is_host = rcar_gen3_is_host(ch); + else return -EIO; - return sprintf(buf, "%s\n", rcar_gen3_is_host(ch) ? "host" : - "peripheral"); + return sprintf(buf, "%s\n", is_host ? "host" : "peripheral"); } static DEVICE_ATTR_RW(role);
This patch cleanups the role_{store,show}() and replaces the local "bool" for host/device mode selection with the "enum phy_mode" in the role_store(). Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com> --- drivers/phy/renesas/phy-rcar-gen3-usb2.c | 58 ++++++++++++++++++++++---------- 1 file changed, 40 insertions(+), 18 deletions(-)