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

[Patrice CHOTARD]

Hi Simon

On 07/06/2017 06:48 AM, Simon Glass wrote:
> 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?

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
>