[PATCH] usb: host: ohci-generic: Make usage of clock/reset bulk() API

[Fabrice Gasnier]

On 8/30/23 17:18, Marek Vasut wrote:
> On 8/30/23 10:01, Fabrice Gasnier wrote:
>> Make usage of clock and reset bulk API in order to simplify the code
>>
>> Signed-off-by: Fabrice Gasnier <fabrice.gasnier at foss.st.com>
>> ---
>>
>>   drivers/usb/host/ohci-generic.c | 92 +++++++++++----------------------
>>   1 file changed, 29 insertions(+), 63 deletions(-)
>>
>> diff --git a/drivers/usb/host/ohci-generic.c
>> b/drivers/usb/host/ohci-generic.c
>> index 2d8d38ce9a40..95aa608d8c19 100644
>> --- a/drivers/usb/host/ohci-generic.c
>> +++ b/drivers/usb/host/ohci-generic.c
>> @@ -16,75 +16,41 @@
>>     struct generic_ohci {
>>       ohci_t ohci;
>> -    struct clk *clocks;    /* clock list */
>> -    struct reset_ctl *resets; /* reset list */
>> +    struct clk_bulk clocks;    /* clock list */
>> +    struct reset_ctl_bulk resets; /* reset list */
>>       struct phy phy;
>> -    int clock_count;    /* number of clock in clock list */
>> -    int reset_count;    /* number of reset in reset list */
>>   };
>>     static int ohci_usb_probe(struct udevice *dev)
>>   {
>>       struct ohci_regs *regs = dev_read_addr_ptr(dev);
>>       struct generic_ohci *priv = dev_get_priv(dev);
>> -    int i, err, ret, clock_nb, reset_nb;
>> -
>> -    err = 0;
>> -    priv->clock_count = 0;
>> -    clock_nb = dev_count_phandle_with_args(dev, "clocks",
>> "#clock-cells",
>> -                           0);
>> -    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 && err != -ENOSYS) {
>> -                dev_err(dev, "failed to enable clock %d\n", i);
>> -                clk_free(&priv->clocks[i]);
>> -                goto clk_err;
>> -            }
>> -            priv->clock_count++;
>> -        }
>> -    } else if (clock_nb != -ENOENT) {
>> -        dev_err(dev, "failed to get clock phandle(%d)\n", clock_nb);
>> -        return clock_nb;
>> +    int err, ret;
>> +
>> +    ret = clk_get_bulk(dev, &priv->clocks);
>> +    if (ret && ret != -ENOENT) {
>> +        dev_err(dev, "Failed to get clocks (ret=%d)\n", ret);
> 
> Plural of 'clock' is still 'clock' , please fix just the text, keep the
> variable name .

Hi Marek,

Are you sure ? Taking a closer look on the web, also in Linux or u-boot,
I can see plural of clock is clocks. Documentation also deals with
multiple clocks too.

> 
>> +        return ret;
>> +    }
>> +
>> +    err = clk_enable_bulk(&priv->clocks);
>> +    if (err) {
>> +        dev_err(dev, "Failed to enable clocks (err=%d)\n", err);
> 
> DTTO here
> 
>> +        goto clk_err;
>>       }
>>   -    priv->reset_count = 0;
>> -    reset_nb = dev_count_phandle_with_args(dev, "resets",
>> "#reset-cells",
>> -                           0);
>> -    if (reset_nb > 0) {
>> -        priv->resets = devm_kcalloc(dev, reset_nb,
>> -                        sizeof(struct reset_ctl),
>> -                        GFP_KERNEL);
>> -        if (!priv->resets)
>> -            return -ENOMEM;
>> -
>> -        for (i = 0; i < reset_nb; i++) {
>> -            err = reset_get_by_index(dev, i, &priv->resets[i]);
>> -            if (err < 0)
>> -                break;
>> -
>> -            err = reset_deassert(&priv->resets[i]);
>> -            if (err) {
>> -                dev_err(dev, "failed to deassert reset %d\n", i);
>> -                reset_free(&priv->resets[i]);
>> -                goto reset_err;
>> -            }
>> -            priv->reset_count++;
>> -        }
>> -    } else if (reset_nb != -ENOENT) {
>> -        dev_err(dev, "failed to get reset phandle(%d)\n", reset_nb);
>> +    err = reset_get_bulk(dev, &priv->resets);
>> +    if (err && err != -ENOENT) {
>> +        dev_err(dev, "failed to get resets (err=%d)\n", err);
>>           goto clk_err;
>>       }
>>   +    err = reset_deassert_bulk(&priv->resets);
>> +    if (err) {
>> +        dev_err(dev, "failed to get deassert resets (err=%d)\n", err);
> 
> Drop the 'get' from the text here, I think it's copy-paste error.

ack, will update in v2

> 
>> +        goto reset_err;
>> +    }
>> +
>>       err = generic_setup_phy(dev, &priv->phy, 0);
>>       if (err)
>>           goto reset_err;
> 
> With that fixed:
> 
> Reviewed-by: Marek Vasut <marex at denx.de>

Thanks, will add in v2.
BR,
Fabrice

> 
> Nice cleanup, thanks !