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@xxxxxxxxxxx>
> ---
> 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
>
