Hi Simon On 07/06/2017 06:48 AM, Simon Glass wrote: > On 21 June 2017 at 01:50, <patrice.chot...@st.com> wrote: >> From: Patrice Chotard <patrice.chot...@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.chot...@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 <s...@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?
Yes, i will add it > >> + 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 (... ok > > ? > >> + 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 > _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot