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

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

[Linus Walleij]

On Fri, Nov 21, 2014 at 4:43 PM, Felipe Balbi <balbi@ti.com> 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@free.fr>
>
> 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@linaro.org>

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

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

[Robert Jarzmik]

Linus Walleij <linus.walleij-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> writes:

> This definately make things better so:
> Acked-by: Linus Walleij <linus.walleij-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
Thanks.

> 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);
>>> +     }

> 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.

OK, if we were to do this, is there a way to build a static platform data with a
gpio descriptor ?
Ie. can I write something like :
struct generic_phy_pdata {
	struct gpio_desc *reset_gpio;
};

And in machine code :
static struct generic_phy_pdata usb_phy __initdata {
       .reset_gpio = XXX(19),
};

What should "XXX" be like to describe reset_gpio as a ACTIVE_HIGH gpio number 19
?

Cheers.

-- 
Robert
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Alexandre Courbot]

On Mon, Dec 1, 2014 at 5:32 AM, Robert Jarzmik <robert.jarzmik@free.fr> wrote:
> Linus Walleij <linus.walleij@linaro.org> writes:
>
>> This definately make things better so:
>> Acked-by: Linus Walleij <linus.walleij@linaro.org>
> Thanks.
>
>> 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);
>>>> +     }
>
>> 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.
>
> OK, if we were to do this, is there a way to build a static platform data with a
> gpio descriptor ?
> Ie. can I write something like :
> struct generic_phy_pdata {
>         struct gpio_desc *reset_gpio;
> };
>
> And in machine code :
> static struct generic_phy_pdata usb_phy __initdata {
>        .reset_gpio = XXX(19),
> };
>
> What should "XXX" be like to describe reset_gpio as a ACTIVE_HIGH gpio number 19
> ?

I think what you want to do is explained in the "Platform Data"
section of Documentation/gpio/board.txt

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

[Robert Jarzmik]

Alexandre Courbot <gnurou@gmail.com> writes:

> I think what you want to do is explained in the "Platform Data"
> section of Documentation/gpio/board.txt
Ah yeah, already one year and I never checked again this documemtation.

Thanks.

-- 
Robert