From mboxrd@z Thu Jan 1 00:00:00 1970 From: Heiko Schocher Date: Thu, 27 Jun 2019 06:12:26 +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> Message-ID: <0e953f9a-127f-f9ff-67ca-e99965f0cd40@denx.de> 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 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? >> 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. What do you think? >>>> + >>>> + 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? Is it worth? >>>> +/** >>>> + * gpio_hog_probe_all() - probe all gpio devices with >>>> + * gpio-hog subnodes. >>>> + * >>>> + * @return: Returns return value from device_probe() >>> >>> What if one fails and the others succeed? >> >> Currently I return with error code from device_probe(). So the >> rest of the gpio-hogs are not probed. >> >> Hmm.. may it is better to emmit a debug message and continue? > > Sounds reasonable. But it seems to me that if there is an error there > should be some visible report, so this could perhaps return the error > code? Then the caller could print a messag? Ok, so I add a printf message. I like more to add it in gpio_hog_probe_all(), because more than one gpio_hog can fail. 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