From mboxrd@z Thu Jan 1 00:00:00 1970 From: Heiko Schocher Date: Mon, 15 Jul 2019 08:15:59 +0200 Subject: [U-Boot] [PATCH v4] gpio: add gpio-hog support In-Reply-To: References: <20190612041146.53854-1-hs@denx.de> <80897b0d-c5ed-4580-86a6-9c9bfbed4b49@denx.de> <0e953f9a-127f-f9ff-67ca-e99965f0cd40@denx.de> Message-ID: List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de Hello Simon, Am 12.07.2019 um 23:11 schrieb Simon Glass: > Hi Heiko, > > On Wed, 26 Jun 2019 at 22:12, Heiko Schocher wrote: >> >> Hello Simon, >> >> Am 26.06.2019 um 17:07 schrieb Simon Glass: >>> Hi Heiko, >>> >>> On Mon, 24 Jun 2019 at 00:16, Heiko Schocher wrote: >>>> >>>> Hello Simon, >>>> >>>> Am 22.06.2019 um 21:09 schrieb Simon Glass: >>>>> Hi Heiko, >>>>> >>>>> On Wed, 12 Jun 2019 at 05:12, 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 >>>>>> >>>>>> --- >>>>>> 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 >>>>> >>>>> >>> >>> [..] >>> >>>>>> +config DM_GPIO_HOG >>>>> >>>>> Can we drop the DM_ prefix? At some point DM_GPIO will go away and we >>>>> don't want to rename these sorts of options. >>>> >>>> Hmm... we also have the DM_GPIO symbol, as the gpio hog only works with >>>> DM_GPIO enabled, I thought DM_GPIO_HOG is the better choice, but of course >>>> I can change this. >>> >>> Yes I understand that, but in general we hope one day to rename >>> DM_GPIO to GPIO, once everything uses DM. That's why I am trying to >>> keep the DM_ suffix only for things that need it. >> >> Yep, already dropped, see temp version (travisbuild clean): >> https://github.com/hsdenx/u-boot-test/commit/509bad758b8abc94dbbebc2a387b9e97c30b6543 >> >>>> >>>>>> + bool "Enable GPIO hog support" >>>>>> + depends on DM_GPIO >>>>>> + default n >>>>>> + help >>>>>> + Enable gpio hog support >>>>>> + The GPIO chip may contain GPIO hog definitions. GPIO hogging >>>>>> + is a mechanism providing automatic GPIO request and config- >>>>>> + uration as part of the gpio-controller's driver probe function. >>>>>> + >>>>>> config ALTERA_PIO >>>>>> bool "Altera PIO driver" >>>>>> depends on DM_GPIO >>>>>> diff --git a/drivers/gpio/gpio-uclass.c b/drivers/gpio/gpio-uclass.c >>>>>> index da5e9ba6e5..308d0863ad 100644 >>>>>> --- a/drivers/gpio/gpio-uclass.c >>>>>> +++ b/drivers/gpio/gpio-uclass.c >>>>>> @@ -5,6 +5,9 @@ >>>>>> >>>>>> #include >>>>>> #include >>>>>> +#include >>>>>> +#include >>>>>> +#include >>>>>> #include >>>>>> #include >>>>>> #include >>>>>> @@ -141,6 +144,118 @@ static int gpio_find_and_xlate(struct gpio_desc *desc, >>>>>> return gpio_xlate_offs_flags(desc->dev, desc, args); >>>>>> } >>>>>> >>>>>> +#if defined(CONFIG_DM_GPIO_HOG) >>>>>> + >>>>>> +struct gpio_hog_priv { >>>>>> + struct gpio_desc gpiod; >>>>> >>>>> As above I don't think you need this. >>>> >>>> Hmm.. when probing I need to request the gpio with gpio_dev_request_index() >>>> for which I need to pass a pointer to struct gpio_desc ... >>> >>> The caller can declare this in a local variable. >> >> The caller is always the device probe function, as gpio_hog_probe_all() >> loops over all gpio hog devices and probes them with device_probe() >> which cannot return a gpio_desc pointer, nor can such a pointer passed to. >> >> Why isn't it valid to store in each gpio hog device the gpio desc it >> belongs too in device private data? > > I don't see any point. Your caller needs to store the pointer anyway, > or if not, could just as easily have a local variable with the info. > > Also, passing a pointer to the struct is how we consistently do it so far. > > What is the benefit of storing it in the callee rather than the caller? > >> >>>> And later I want to have the possibility to access the gpio for example >>>> with dm_gpio_set_value() and need therefore the gpio descriptor, so I must >>>> store it somewhere. Or do I overlook something obvious? >>> >>> In that case the caller should store it, perhaps in a local variable, >>> or perhaps in a private data structure if needed for a long time. >>> >>> [..] >> >> Of course I can introduce a list for each gpio hog definition filled >> when calling gpio_hog_probe_all() ... but than I have a second list >> which uses memory, may has to keep in sync the list if device are >> removed ... I want to avoid such an approach ... as we have already a >> list of gpio hog devices and the priv storage pointer for device private >> data. > > I understand what you are saying about devices being removed, but > actually removable is fine, it's only unbinding that is dangerous. And > we already have that problem. We would need to add refcounts to fix > it, and given that bootloaders don't normally need to unbind, I have > not worried about it too much. > > The approach I suggest would not use any more memory I think. > >> >> What do you think? > > Well I still think it is better to have the caller own the struct. > >> >>>>>> + >>>>>> + return 0; >>>>>> +} >>>>>> + >>>>>> +int gpio_hog_probe_all(void) >>>>>> +{ >>>>>> + struct udevice *dev; >>>>>> + int ret; >>>>>> + >>>>>> + for (uclass_first_device(UCLASS_NOP, &dev); >>>>> >>>>> Perhaps you should create a new uclass for this? >>>> >>>> Hmm.. yes, I had an approach with a new uclass, but I thought, the new [1] >>>> UCLASS_NOP is valid for this here to? >>>> >>>> [1] http://patchwork.ozlabs.org/patch/1098913/ >>> >>> I see your point and I can see arguments for each side. But the fact >>> that you have to check the driver suggests to me that a new uclass is >>> better. >>> >>> [..] >> >> Ok with this approach the gpio_hog additions to gpio-uclass can be >> moved to a gpio-hog-uclass.c file. Also the problem where to store >> the gpio descriptor moves from device private data to uclass private >> data? > > I don't think you have explained why you actually need to store the > GPIO descriptor. Where do you use it later? Is it just the > gpio_hog_probe_all() function? You can use it later! See example this patch adds in doc/device-tree-bindings/gpio/gpio.txt: + struct gpio_desc *desc; + int ret; + + ret = gpio_hog_lookup_name("boot_rescue", &desc); + if (ret) + return; + if (dm_gpio_get_value(desc) == 1) + printf("\nBooting into Rescue System\n"); + else if (dm_gpio_get_value(desc) == 0) + printf("\nBoot normal\n"); So in this example, gpio "board_rescue" is defined through DTS and code which uses this gpio is generic ... makes it nice to handle board differences (as I have on the aristainetos board, which I rework for DM support). 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