search

eclipse / mraa

Linux Library for low speed IO Communication in C with bindings for C++, Python, Node.js & Java. Supports generic io platforms, as well as Intel Edison, Intel Joule, Raspberry Pi and many more.
http://mraa.io
MIT License
1.37k stars 613 forks source link

gpio: Avoid spurious value reset without output mode changes #1063

Closed jan-kiszka closed 3 years ago

jan-kiszka commented 3 years ago

"Standard" phrase: I'm in the process of resolving the missing ECA signature. Please already review technically.

Propanu commented 3 years ago

Hi @jan-kiszka, thanks for submitting this. I'm not convinced however this is needed as a generic gpio patch and it may need to be limited to the affected platform instead. Any chance you can let us know what board you are using?

The reasoning behind it is quite simple. You mentioned this fixes an issue "for a pin that was already set". If it's already set (e.g. by firmware prior to starting your application), simply start using the pin as you normally would, without a call to set the direction. If the pin is set to a different mode, you have no choice but this patch also doesn't fix your issue.

Here's the caveat I see with it. Mraa supports a small set of gpio expanders in addition to regular gpio capable platforms. And for such devices, you have no guarantee the direction configuration register is readable. It can very well be write only, in which case your patch would cause a direction request to fail, potentially rendering such devices unusable.

jan-kiszka commented 3 years ago

I've tested the gpio sysfs behavior on different systems - it is a generic issue that mraa stumbles over here.

But I understand your concern that the direction may not be readable. I'll check the kernel code, how it handles the case when the low-level driver does not provide a read-out. If there is a chance of seeing an error, I will ignore that case. Then we cannot fix those platforms, unfortunately.

jan-kiszka commented 3 years ago

As you can see in https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/gpio/gpiolib-sysfs.c?h=v5.12#n59 (also in older versions), this error scenario is not possible. So there is no issue with platforms not supporting hardware read-back. Linux will initialize the direction and from then on report the shadow state. So the fix is safe.

Propanu commented 3 years ago

I would simply remove the error check on L1297 while you wait on the ECA. If there's an issue with reading the direction, it will fail on the previous check on L1280. I don't see any other issues with your patch. You're right, the gpio driver won't crash as long as the /sys/class/gpio/gpioN/direction file is there. The expanders I mentioned usually define their own functions to set direction so this should be good to go.

jan-kiszka commented 3 years ago

Ping. To my understanding, there were no changes needed in the end, and this could go in as-is.