[U-Boot] [PATCH 19/55] dm: gpio: Add support for setting a GPIO's pull direction

[Masahiro Yamada]

Hi Simon,



2015-07-07 1:39 GMT+09:00 Simon Glass <sjg at chromium.org>:
> Hi Masahiro,
>
> On 4 July 2015 at 22:55, Masahiro Yamada <yamada.masahiro at socionext.com> wrote:
>> Hi Simon,
>>
>>
>> 2015-07-03 9:15 GMT+09:00 Simon Glass <sjg at chromium.org>:
>>> At present the driver model GPIO API does not support pull-up/pull-down on
>>> input GPIOs. This is required in some cases.
>>>
>>> Add this feature to the API with two new methods that drivers can optionally
>>> implement.
>>>
>>> Signed-off-by: Simon Glass <sjg at chromium.org>
>>> ---
>>>
>>>  drivers/gpio/gpio-uclass.c | 31 +++++++++++++++++++++++++++++++
>>>  include/asm-generic/gpio.h | 46 ++++++++++++++++++++++++++++++++++++++++++++++
>>>  2 files changed, 77 insertions(+)
>>>
>>> diff --git a/drivers/gpio/gpio-uclass.c b/drivers/gpio/gpio-uclass.c
>>> index 4efda31..c4ba580 100644
>>> --- a/drivers/gpio/gpio-uclass.c
>>> +++ b/drivers/gpio/gpio-uclass.c
>>> @@ -374,6 +374,37 @@ int dm_gpio_set_dir(struct gpio_desc *desc)
>>>         return dm_gpio_set_dir_flags(desc, desc->flags);
>>>  }
>>>
>>> +int dm_gpio_set_pull(struct gpio_desc *desc, enum gpio_pull pull)
>>> +{
>>> +       struct dm_gpio_ops *ops = gpio_get_ops(desc->dev);
>>> +       int ret;
>>> +
>>> +       ret = check_reserved(desc, "set_pull");
>>> +       if (ret)
>>> +               return ret;
>>> +
>>> +       if (!ops->set_pull)
>>> +               return -ENOSYS;
>>> +
>>> +       return ops->set_pull(desc->dev, desc->offset, pull);
>>> +}
>>> +
>>> +int dm_gpio_get_pull(struct gpio_desc *desc, unsigned offset)
>>> +{
>>> +       struct dm_gpio_ops *ops = gpio_get_ops(desc->dev);
>>> +       int ret;
>>> +
>>> +       ret = check_reserved(desc, "get_pull");
>>> +       if (ret)
>>> +               return ret;
>>> +
>>> +       if (!ops->get_pull)
>>> +               return -ENOSYS;
>>> +
>>> +       return ops->get_pull(desc->dev, desc->offset);
>>> +}
>>> +
>>> +
>>>  /**
>>>   * gpio_get_value() - [COMPAT] Sample GPIO pin and return it's value
>>>   * gpio:       GPIO number
>>> diff --git a/include/asm-generic/gpio.h b/include/asm-generic/gpio.h
>>> index 0af599f..db5504f 100644
>>> --- a/include/asm-generic/gpio.h
>>> +++ b/include/asm-generic/gpio.h
>>> @@ -126,6 +126,13 @@ struct gpio_desc {
>>>          */
>>>  };
>>>
>>> +/* GPIO pull directions (used for input lines) */
>>> +enum gpio_pull {
>>> +       GPIO_PULL_NONE,
>>> +       GPIO_PULL_DOWN,
>>> +       GPIO_PULL_UP,
>>> +};
>>> +
>>>  /**
>>>   * dm_gpio_is_valid() - Check if a GPIO is valid
>>>   *
>>> @@ -276,6 +283,26 @@ struct dm_gpio_ops {
>>>          */
>>>         int (*xlate)(struct udevice *dev, struct gpio_desc *desc,
>>>                      struct fdtdec_phandle_args *args);
>>> +
>>> +       /**
>>> +        * set_pull() - set pull direction
>>> +        *
>>> +        * @dev:        Device to adjust
>>> +        * @offset:     GPIO offset within device
>>> +        * @pull:       New pull direction
>>> +        * @return 0 if OK, -ve on error
>>> +        */
>>> +       int (*set_pull)(struct udevice *dev, unsigned offset,
>>> +                       enum gpio_pull pull);
>>> +
>>> +       /**
>>> +        * get_pull() - get pull direction
>>> +        *
>>> +        * @dev:        Device to check
>>> +        * @offset:     GPIO offset within device
>>> +        * @return current pull direction or -ve on error
>>> +        */
>>> +       int (*get_pull)(struct udevice *dev, unsigned offset);
>>>  };
>>>
>>
>>
>> Do you want to control pull-up/down only for GPIOs?
>> Pin-biasing makes sense for dedicated functions as well as GPIOs.
>> At least, I am inclined to enable pull-up registers for SCL/SDA (I2C)
>> pins on my boards.
>>
>> As far as I know, in Linux, such stuff is handled in pinctrl drivers.
>> I see no .set_pull() callback in struct gpio_chip in Linux.
>>
>> As far as I see your pinctrl implementation,
>> (http://patchwork.ozlabs.org/patch/487801/)
>> it looks like "pinctrl = pinmuxing" in U-Boot, whereas "pinctrl =
>> pinmuxing + pin-configuration" in Linux.
>
> That's true, but still it is useful to be able to set pull direction
> for GPIOs. I did sent a pinctrl uclass patch recently. Perhaps we
> could add a new function there. But how would we identify which pin to
> change? The GPIO pin name is convenient but may not be available in
> the pinctrl driver, since this adds a lot of size to the device tree.


As you may already know, in Linux, GPIO numbers are mapped into pin numbers
via "gpio-ranges" property, but we need much effort
to implement the framework in U-Boot.

Do you think it would make sense to support full-spec pinctrl drivers
(much closer to Linux's one) in U-Boot?   Or is it crazy?

I just skimmed over your pinctrl patches, but
I could not understand fully how it works.  (especially, .get_periph_id).


I will leave some comments on your rockchip pinctrl patch.

-- 
Best Regards
Masahiro Yamada