Support to generic GPIOs class path names

[bmeneg]

Older kernel versions might use gpio paths in the format of gpio<pin_number>_<pin_name> or any other way, as before device trees were supported proprietary hardware description files were passed from bootloader to kernel and hence leading to different naming convetions. For instance, on Allwinner platforms a specific .fex file was passed instead of the "Device Tree Binary" (.dtb).

Libmraa didn't support this kind of paths because all gpio operations were done with hard coded path names. With this commit it's possible to run libmraa either on newer kernel version and older ones, with both formats of gpio sysfs paths.

This PR also guarantees the kernel version discovery either when the gpio was already exported by any other process or when it is the first time used.

[alext-mkrs]

Haven't had a chance to analyze it in full yet, but what's immediately apparent is that it makes sense to squash both commits into one. And looks like you've removed a couple of "not owner" comments, which IMHO were helpful.

[bmeneg]

@alext-mkrs done, squashed both commits. But about the comments I removed: I added new comments a little bit more generic just before the 'if' block about this 'ownership', do you think would be better to revert it?

Now, about the Trevis CI failure state: I saw that on Travis CI the commits failed, but the failure report shows a problem to the test script itself:

$ sudo apt-get install -y -qq swig3.0 python git cmake

WARNING: The following packages cannot be authenticated!

  cmake cmake-data

E: There are problems and -y was used without --force-yes

The command "sudo apt-get install -y -qq swig3.0 python git cmake" failed and exited with 100 during .

I will commit the tests modification we discussed on Issue #520, I hope the CI passes then =).

[alext-mkrs]

Thanks for squashing, but now you have duplicated Signed-off-by line and two commit messages in one, I'd suggest just rephrase and merge them.

As for the comment - let me take a closer look on a computer in a day or two, as phone is not an ideal tool :smiley:

[bmeneg]

@Hbrinj well pointed. Indeed it's also needed to check its state after allocation and freed in case of error (goto init_internal_cleanup). Now it's done.

@alext-mkrs sorry for these wrong commits =), but now it's done, I squashed all commits together because all are related to the same thing. And don't worry, take a look when you have some time, phones isn't the better tool for sure.

[bmeneg]

Indeed, before we continue, I'll edit the Pull Request title, because I made a terrible mistake using the sentence "old kernel version", because this different GPIO class path name might be particularity to old kernel versions adopted by the Allwinner SoC vendor, which has its own internal things inside kernel translating the gpios to these schema. I'll change some things and come back later just with the dev->gpio_path generic attribute part, the path schema itself (gpio<pin_number>_<pin_name>) I'll handle inside Cubietruck's code, with gpio_init_internal_replace(dev, pin), and other platforms based on Allwinner might handle the same way. It's a better way, right?

[alext-mkrs]

FWIW aside from those line comments I've added it looks ok to me by inspection. Haven't tried to run or explore it in runtime though.

[alext-mkrs]

<...> I'll change some things and come back later just with the dev->gpio_path generic attribute part, the path schema itself (gpio<pin_number>_<pin_name>) I'll handle inside Cubietruck's code, with gpio_init_internal_replace(dev, pin), and other platforms based on Allwinner might handle the same way. It's a better way, right?

Yes, I think so. That indeed changes the angle a bit :smiley: If that's more of a platform-specific quirk, replace functions are there to help that.

and come back later just with the dev->gpio_path generic attribute part

I wonder if in this case this infrastructure is going to be useful anywhere else and if not - wouldn't using replace functions across the board for other GPIO functionalities (and adding the pin name part in those) be a better solution. Do you think that's feasible?

[bmeneg]

I wonder if in this case this infrastructure is going to be useful anywhere else and if not - wouldn't using replace functions across the board for other GPIO functionalities (and adding the pin name part in those) be a better solution. Do you think that's feasible?

Initially I thought it would be the solution, but there are hard coded path names are all around GPIO internal functions that doesn't have replaces. Then I though that the gpio_path attribute on gpio_context_t would help.

[alext-mkrs]

I see. You can try adding replaces though. Similar to what I did with pre- and post- (though replaces would be harder) in #515

Otherwise you could probably try the split way you mentioned (adding only path and the rest is in replace) and see what @arfoll has to say about that.

[bmeneg]

I'll try to use the gpio_path first, it's almost done :D, just change little things to clear the "Allwinner" specific parts. But if @arfoll prefers the replace functions, won't be a problem too. But thank you very much @alext-mkrs .. I'll be back soon.

[arfoll]

I think that by just concatenating either the pin num or the pin path we should be able to get both working relatively easily. I can't review well on my phone but I'll be back next week with a closer look :)

[bmeneg]

Well, I'm back with new changes, but now I didn't do any style fixes and everything specific to the gpio<pin_number>_<pin_name> were removed, because as I said before it was a specific case to Allwinner SoC based platforms using old kernel versions. The only thing I made in this commit was to add the gpio_path attribute to struct _gpio and handled it on gpio.c/.h and on the APIs to other languages.

Everything specific to Allwinner SoC must be handled in _replace functions on each platform support, as it's being made in the new support to Cubietruck I'm developing.

[alext-mkrs]

In addition to those inline comments - what's your idea about mraa_gpio_get_valfp(), where you haven't updated the /gpio%d/value piece? While it's used only behind _replace() checks in this file, I think it still would make sense to update it to use gpio_path for consistency in case there's a situation where there's no need for _replace(), but gpio_path is custom. And in case it's not custom gpio_path is gpio%d anyway, so we have backwards compatibility. What do you think?

[arfoll]

@bmeneguele can you push an example of a platform that works with this stuff as well? this seems to require a mix of _replace and gpio_path rather than just all moving to gpio_path. I'd much rather simply use gpio_path everywhere and have a small replace in the _init() that just sets gpio_path differently for platforms with names and by default set it to "gpio+dev->pin". Does that makes sense?

[bmeneg]

@alext-mkrs You are right.. I just forgot that peace, there should have the gpio_path as well.

@arfoll Yeah, I will open an PR to the cubietruck support I am working on. It isn't complete yet, I'm facing some SPI problems, but GPIO is working correctly. But as you can see on lines 108 and 109:

snprintf(dev->gpio_path, MAX_SIZE, "/gpio%d", dev->pin);
snprintf(bu, sizeof(bu), SYSFS_CLASS_GPIO "%s", dev->gpio_path);

the gpio_path is being set to gpio+dev->pin by default if there isn't any init_internal_replace() function on platform's _init() code.

[bmeneg]

@arfoll take a look at https://github.com/bmeneguele/mraa/commit/e93e261285586d4ec04c4758c541e93760cf8a7d this is the initial support to the Cubietruck board I'm working on right now. I won't pull request yet because I need to change the coding style to the standard of the project, but take a look at line 139 on src/arm/cubietruck.c, that is the _replace function to the gpio_init_internal function that handles the dev->gpio_path.