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

Tue Jun 25 07:20:32 UTC 2019

Hi Heiko,

> From: Heiko Schocher <hs at denx.de>
> Sent: samedi 22 juin 2019 07:32
> 
> 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...

Ok. It is safe.... 

and anyway any board can choose the call the function earlier if the hog was required 
(for nay initcall before board_late_init : initr_nand, initr_mmc ....)

> > 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():

Fine.

>   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=0d15e5062b66123cd364bd9803e076db7
> e7dd97c;hb=77f6e2dd0551d8a825bab391a1bd6b838874bcd4#l319
> 
> May you can test if this works for you?

I have the same idea, it is why I say is was no't blocking for me.

Already tested and it is working (gpio hog is called only one time even if gpio_hog_probe_all several time).

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

Regards
Patrick