Re: [PATCH v1] usb: phy: generic: migrate to gpio_desc

[Linus Walleij <linus.walleij@xxxxxxxxxx>]

On Fri, Nov 21, 2014 at 4:43 PM, Felipe Balbi <balbi@xxxxxx> wrote:
> On Sun, Nov 09, 2014 at 01:23:07PM +0100, Robert Jarzmik wrote:
>>
>> Change internal gpio handling from integer gpios into gpio
>> descriptors. This change only addresses the internal API and
>> device-tree/ACPI, while the legacy platform data remains integer space
>> based.
>>
>> This change is only build compile tested, and very prone to error. I
>> leave this comment for now in the commit message so that this patch gets
>> some testing as I'm pretty sure it's buggy.
>>
>> Signed-off-by: Robert Jarzmik <robert.jarzmik@xxxxxxx>
>
> To my eyes, patch looks fine, but let's get a review from Linus W.
>
> Linus, can you have a look below ? Is this being used the way you
> intended ? BTW, we need support DT and pdata platforms here.

This definately make things better so:
Acked-by: Linus Walleij <linus.walleij@xxxxxxxxxx>

One comment though:

>>       if (dev->of_node) {
(...)
>> +             nop->gpiod_reset = devm_gpiod_get(dev, "reset-gpios");
>> +             err = PTR_ERR(nop->gpiod_reset);
>>       } else if (pdata) {
(...)
>> +             err = devm_gpio_request_one(dev, pdata->gpio_reset, 0,
>> +                                         dev_name(dev));
>> +             if (!err)
>> +                     nop->gpiod_reset = gpio_to_desc(pdata->gpio_reset);
>> +     }

This construction implies that if we don't have a DT node, the
GPIO is passed as a fixed number in pdata->gpio_reset,
but it is actually possible to use descriptors in board files
by adding a fixed table.

So a next step would be to add support for getting the
devm_gpiod_get(dev, "reset-gpios"); outside of the if (dev->of_node)
clause, and possibly convert the board files for affected
platforms to use descriptors, if they will not be replaced by
device tree only.

I know this is a major work, so this patch is still a good start!
Using descriptors internally is always preferred.

Yours,
Linus Walleij
--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html