[U-Boot] [PATCH v4] gpio: add gpio-hog support

[Heiko Schocher]

Hello Patrick,

Am 21.06.2019 um 18:09 schrieb Patrick DELAUNAY:
> Hi Mickael
> 
>> Subject: Re: [PATCH v4] gpio: add gpio-hog support
>> Importance: High
>>
>> On 12. 06. 19 6:11, Heiko Schocher wrote:
>>> add gpio-hog support. GPIO hogging is a mechanism providing automatic
>>> GPIO request and configuration as part of the gpio-controller's driver
>>> probe function.
>>>
>>> for more infos see:
>>> doc/device-tree-bindings/gpio/gpio.txt
>>>
>>> Signed-off-by: Heiko Schocher <hs at denx.de>
>>>
>>> ---
>>> clean travis build, see result:
>>> https://travis-ci.org/hsdenx/u-boot-test/builds/544040419
>>>
>>> Changes in v4:
>>> - rework as suggested by Patrick
>>>    based on patch:
>>>    http://patchwork.ozlabs.org/patch/1098913/
>>>    use new UCLASS_NOP
>>>    hog detection + DT parsing in gpio_post_bind() .... info saved in platdata
>>>    gpio configuration for hog (request and set_dir) => probe function
>>>
>>> Changes in v3:
>>> - probe now all gpio devices with gpio-hogs
>>>    from board_r before board_late_init() call
>>>    or if someone wants to access gpio-hog
>>>    based gpios with gpio_hog_lookup_name()
>>>    This is needed, because we cannot be sure,
>>>    if a gpio device which contains gpio-hogs
>>>    is probed.
>>> - add line-name property as Michal recommended
>>> - renamed gpio_priv_one into gpio_priv_one_hog
>>>    and protected through CONFIG_DM_GPIO_HOG
>>>
>>> Changes in v2:
>>> - move gpio_hog() call from post_probe() to post_bind()
>>>    call. Check if current gpio device has gpio-hog
>>>    definitions, if so, probe it.
>>>
>>>   common/board_r.c                       |   6 +
>>>   doc/device-tree-bindings/gpio/gpio.txt |  55 ++++++++
>>>   drivers/gpio/Kconfig                   |  10 ++
>>>   drivers/gpio/gpio-uclass.c             | 181 ++++++++++++++++++++++---
>>>   include/asm-generic/gpio.h             |  32 +++++
>>>   5 files changed, 268 insertions(+), 16 deletions(-)
>>>
>>> diff --git a/common/board_r.c b/common/board_r.c index
>>> 150e8cd424..507da4c74a 100644
>>> --- a/common/board_r.c
>>> +++ b/common/board_r.c
>>> @@ -49,6 +49,9 @@
>>>   #include <linux/err.h>
>>>   #include <efi_loader.h>
>>>   #include <wdt.h>
>>> +#if defined(CONFIG_DM_GPIO_HOG)
>>> +#include <asm/gpio.h>
>>> +#endif
>>>
>>>   DECLARE_GLOBAL_DATA_PTR;
>>>
>>> @@ -796,6 +799,9 @@ static init_fnc_t init_sequence_r[] = {  #ifdef
>>> CONFIG_CMD_NET
>>>   	initr_ethaddr,
>>>   #endif
>>> +#if defined(CONFIG_DM_GPIO_HOG)
>>> +	gpio_hog_probe_all,
>>> +#endif
> 
> Any reason to have hog at this place ?

Not really, but we need to check at some point if all gpio hogs
are processed, and I thought at a late time it does not break
anything...

> Before board_late_init....
> 
> I expect more before board_init() for my future use-case,
> to replace the current (dirty)  implementation with pinctrl-0
> 
> #if defined(CONFIG_WDT)
> 	initr_watchdog,
> #endif
> +#if defined(CONFIG_DM_GPIO_HOG)
> +	gpio_hog_probe_all,
> +#endif
> #if defined(CONFIG_ARM) || defined(CONFIG_NDS32) || defined(CONFIG_RISCV) || \
> 	defined(CONFIG_SANDBOX)
> 	board_init,	/* Setup chipselects */
> #endif
> 
> 
> In my device tree I have :
> 
> 	stmfx: stmfx at 42 {
> 		compatible = "st,stmfx-0300";
> 		reg = <0x42>;
> 		interrupts = <8 IRQ_TYPE_EDGE_RISING>;
> 		interrupt-parent = <&gpioi>;
> 		vdd-supply = <&v3v3>;
> 
> 		stmfx_pinctrl: stmfx-pin-controller {
> 			compatible = "st,stmfx-0300-pinctrl";
> 			gpio-controller;
> 			#gpio-cells = <2>;
> 			interrupt-controller;
> 			#interrupt-cells = <2>;
> 			gpio-ranges = <&stmfx_pinctrl 0 0 24>;
> 			pinctrl-names = "default";
> 			pinctrl-0 = <&hog_pins>;
> 
> 			hog_pins: hog {
> 				pins = "gpio14";
> 				drive-push-pull;
> 				bias-pull-down;
> 			};
> 	};
> 
> 
> And in board, I force probe to configure HOG....before to initialize the DISPLAY, so in
> 
> ./board/st/stm32mp1/stm32mp1.c : board _init()
> 
> /* board dependent setup after realloc */
> int board_init(void)
> {
> .....
> 	/* probe all PINCTRL for hog */
> 	for (uclass_first_device(UCLASS_PINCTRL, &dev);
> 	     dev;
> 	     uclass_next_device(&dev)) {
> 		pr_debug("probe pincontrol = %s\n", dev->name);
> 	}
> 
> 
> Or you can let each board choose when call the function
> as it is already done for other function as regulators_enable_boot_on()
> So  don't add it in init_sequence_r[]  / board_r.c ?...

Hmm... I do not like this, because if you enable gpio-hog support,
you don;t want to think to call gpio_hog_probe_all().

But it should be possible to call gpio_hog_probe_all() earlier from board
code, as gpio_hog_probe_all() calls device_probe():

  212 int gpio_hog_probe_all(void)
  213 {
  214         struct udevice *dev;
  215         int ret;
  216
  217         for (uclass_first_device(UCLASS_NOP, &dev);
  218              dev;
  219              uclass_find_next_device(&dev)) {
  220                 if (dev->driver == DM_GET_DRIVER(gpio_hog)) {
  221                         ret = device_probe(dev);


device_probe() checks if it is already probed, and simply returns if
so, see:

http://git.denx.de/?p=u-boot.git;a=blob;f=drivers/core/device.c;h=0d15e5062b66123cd364bd9803e076db7e7dd97c;hb=77f6e2dd0551d8a825bab391a1bd6b838874bcd4#l319

May you can test if this works for you?

> 
> Anyway it is not blocking for me, it just a question.

Fine.

>>>   #ifdef CONFIG_BOARD_LATE_INIT
>>>   	board_late_init,
[...]
> 
> For the patch Review and tested on stm32mp1 board.
> 
> Tested-by: Patrick Delaunay <patrick.delaunay at st.com>

Thanks for testing!

bye,
Heiko
-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-52   Fax: +49-8142-66989-80   Email: hs at denx.de