[U-Boot] [PATCH v8 08/10] usb: host: ohci-generic: add CLOCK support

[Simon Glass]

On 21 June 2017 at 01:50,  <patrice.chotard at st.com> wrote:
> From: Patrice Chotard <patrice.chotard at st.com>
>
> use array to save enabled clocks reference in order to
> disabled them in case of error during probe() or during
> driver removal.
>
> Signed-off-by: Patrice Chotard <patrice.chotard at st.com>
> ---
> v8:     _ rework error path by propagating the initial error code until the end of probe()
>
> v7:     _ replace clk_count() by ofnode_count_phandle_with_args()
>
> v6:     _ none
>
> v5:     _ none
>
> v4:     _ use generic_phy_valid() before generic_phy_exit() call
>
> v3:     _ extract in this patch the CLOCK support add-on from previous patch 5
>         _ keep enabled clocks reference in list in order to
>           disable clocks in error path or in .remove callback
>
> v2:     _ add error path management
>         _ add .remove callback
>
>  drivers/usb/host/ohci-generic.c | 59 +++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 57 insertions(+), 2 deletions(-)

Reviewed-by: Simon Glass <sjg at chromium.org>

Nits below (sorry) for a respin or follow-on.

>
> diff --git a/drivers/usb/host/ohci-generic.c b/drivers/usb/host/ohci-generic.c
> index f85738f..15d1d60 100644
> --- a/drivers/usb/host/ohci-generic.c
> +++ b/drivers/usb/host/ohci-generic.c
> @@ -5,7 +5,9 @@
>   */
>
>  #include <common.h>
> +#include <clk.h>
>  #include <dm.h>
> +#include <dm/ofnode.h>
>  #include "ohci.h"
>
>  #if !defined(CONFIG_USB_OHCI_NEW)
> @@ -14,18 +16,71 @@
>
>  struct generic_ohci {
>         ohci_t ohci;
> +       struct clk *clocks;
> +       int clock_count;
>  };

Please can you comment this struct?

>
>  static int ohci_usb_probe(struct udevice *dev)
>  {
>         struct ohci_regs *regs = (struct ohci_regs *)devfdt_get_addr(dev);
> +       struct generic_ohci *priv = dev_get_priv(dev);
> +       int i, err, ret, clock_nb;
>
> -       return ohci_register(dev, regs);
> +       err = 0;
> +       priv->clock_count = 0;
> +       clock_nb = ofnode_count_phandle_with_args(dev_ofnode(dev), "clocks",
> +                                                 "#clock-cells");

Could we have a dev_read_...() version of this?

> +       if (clock_nb > 0) {
> +               priv->clocks = devm_kcalloc(dev, clock_nb, sizeof(struct clk),
> +                                           GFP_KERNEL);
> +               if (!priv->clocks)
> +                       return -ENOMEM;
> +
> +               for (i = 0; i < clock_nb; i++) {
> +                       err = clk_get_by_index(dev, i, &priv->clocks[i]);
> +                       if (err < 0)
> +                               break;
> +
> +                       err = clk_enable(&priv->clocks[i]);
> +                       if (err) {
> +                               error("failed to enable clock %d\n", i);
> +                               clk_free(&priv->clocks[i]);
> +                               goto clk_err;
> +                       }
> +                       priv->clock_count++;
> +                       clk_free(&priv->clocks[i]);
> +               }
> +       } else {
> +               if (clock_nb != -ENOENT) {

} else if (...

?

> +                       error("failed to get clock phandle(%d)\n", clock_nb);
> +                       return clock_nb;
> +               }
> +       }
> +
> +       err = ohci_register(dev, regs);
> +       if (err)
> +               goto clk_err;
> +
> +       return 0;
> +
> +clk_err:
> +       ret = clk_disable_all(priv->clocks, priv->clock_count);
> +       if (ret)
> +               error("failed to disable all clocks\n");
> +
> +       return err;
>  }
>
>  static int ohci_usb_remove(struct udevice *dev)
>  {
> -       return ohci_deregister(dev);
> +       struct generic_ohci *priv = dev_get_priv(dev);
> +       int ret;
> +
> +       ret = ohci_deregister(dev);
> +       if (ret)
> +               return ret;
> +
> +       return clk_disable_all(priv->clocks, priv->clock_count);
>  }
>
>  static const struct udevice_id ohci_usb_ids[] = {
> --
> 1.9.1
>

Regards,
Simon