On Mon, 2018-01-15 at 11:41 +0300, Sergei Shtylyov wrote:
> Hello!
>
> On 1/15/2018 1:59 AM, Benjamin Herrenschmidt wrote:
>
> > The Aspeed SoCs use uhci-platform. With the new dynamic clock
> > control framework, the corresponding IP block clock must be
> > properly enabled.
> >
> > This is a simplified variant of what ehci-platform does, it
> > looks for *one* clock attached to the device, and if it's
> > there, enables it.
> >
> > Signed-off-by: Benjamin Herrenschmidt <benh@xxxxxxxxxxxxxxxxxxx>
> > Acked-by: Alan Stern <stern@xxxxxxxxxxxxxxxxxxx>
> > ---
> >
> > v4. Remove unnecessary NULL checks
> >
> > v3. Fix tab vs. spaces and probe error path
> >
> > v2. Don't forget to git add latest changes :-) This adds the
> > part where we turn the clock off when removing the driver.
> > ---
> > drivers/usb/host/uhci-hcd.h | 3 +++
> > drivers/usb/host/uhci-platform.c | 20 ++++++++++++++++++--
> > 2 files changed, 21 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/usb/host/uhci-hcd.h b/drivers/usb/host/uhci-hcd.h
> > index f1cc47292a59..7f9f33c8c232 100644
> > --- a/drivers/usb/host/uhci-hcd.h
> > +++ b/drivers/usb/host/uhci-hcd.h
> > @@ -4,6 +4,7 @@
> >
> > #include <linux/list.h>
> > #include <linux/usb.h>
> > +#include <linux/clk.h>
> >
> > #define usb_packetid(pipe) (usb_pipein(pipe) ? USB_PID_IN : USB_PID_OUT)
> > #define PIPE_DEVEP_MASK 0x0007ff00
> > @@ -447,6 +448,8 @@ struct uhci_hcd {
> > int total_load; /* Sum of array values */
> > short load[MAX_PHASE]; /* Periodic allocations */
> >
> > + struct clk *clk; /* (optional) clock source */
> > +
> > /* Reset host controller */
> > void (*reset_hc) (struct uhci_hcd *uhci);
> > int (*check_and_reset_hc) (struct uhci_hcd *uhci);
> > diff --git a/drivers/usb/host/uhci-platform.c b/drivers/usb/host/uhci-platform.c
> > index 6cb16d4b2257..55c2d99bf7ec 100644
> > --- a/drivers/usb/host/uhci-platform.c
> > +++ b/drivers/usb/host/uhci-platform.c
> > @@ -89,6 +89,8 @@ static int uhci_hcd_platform_probe(struct platform_device *pdev)
> > if (!hcd)
> > return -ENOMEM;
> >
> > + uhci = hcd_to_uhci(hcd);
> > +
> > res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > hcd->regs = devm_ioremap_resource(&pdev->dev, res);
> > if (IS_ERR(hcd->regs)) {
> > @@ -98,8 +100,6 @@ static int uhci_hcd_platform_probe(struct platform_device *pdev)
> > hcd->rsrc_start = res->start;
> > hcd->rsrc_len = resource_size(res);
> >
> > - uhci = hcd_to_uhci(hcd);
> > -
> > uhci->regs = hcd->regs;
> >
> > /* Grab some things from the device-tree */
> > @@ -119,6 +119,19 @@ static int uhci_hcd_platform_probe(struct platform_device *pdev)
> > "Enabled Aspeed implementation workarounds\n");
> > }
> > }
> > +
> > + /* Get and enable clock if any specified */
> > + uhci->clk = devm_clk_get(&pdev->dev, NULL);
> > + if (IS_ERR(uhci->clk)) {
> > + ret = PTR_ERR(uhci->clk);
> > + goto err_rmr;
> > + }
> > + ret = clk_prepare_enable(uhci->clk);
> > + if (ret) {
> > + dev_err(&pdev->dev, "Error couldn't enable clock (%d)\n", ret);
> > + goto err_rmr;
> > + }
> > +
> > ret = usb_add_hcd(hcd, pdev->resource[1].start, IRQF_SHARED);
> > if (ret)
> > goto err_rmr;
> > @@ -127,6 +140,7 @@ static int uhci_hcd_platform_probe(struct platform_device *pdev)
> > return 0;
> >
> > err_rmr:
> > + clk_disable_unprepare(uhci->clk);
>
> Really? You haven't enabled the clock before either *goto* above, why
> disable it here?
If clk is NULL or an error, disable will do nothing. The only case
would be if enable at failed but then disable will do nothing in that
case.
The above change was done at the suggestion of Alan. If you really
prefer I could add a clk_put and clearing of "clk" in the error path of
clk_prepare_enable, but that's about it.
BTW. DO we really need to bike shed such a simple patch to death like
that ?
>
> > usb_put_hcd(hcd);
> >
> > return ret;
>
> [...]
>
> MBR, Sergei
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
