Skip to content

Commit

Permalink
pinctrl: bcm2835: Only return non-GPIOs to inputs
Browse files Browse the repository at this point in the history 
Allowing GPIO state to persist allows the use of gpioset to control
GPIO levels without having to use the --mode=wait feature.

Signed-off-by: Phil Elwell <phil@raspberrypi.com>
  • Loading branch information
@pelwell @popcornmix
pelwell authored and popcornmix committed Mar 14, 2023
1 parent c4163ba commit 022689f
Showing 1 changed file with 6 additions and 6 deletions.
12 changes: 6 additions & 6 deletions drivers/pinctrl/bcm/pinctrl-bcm2835.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -918,9 +918,12 @@ static int bcm2835_pmx_free(struct pinctrl_dev *pctldev,
unsigned offset)
{
struct bcm2835_pinctrl *pc = pinctrl_dev_get_drvdata(pctldev);
enum bcm2835_fsel fsel = bcm2835_pinctrl_fsel_get(pc, offset);

/* Return non-GPIOs to GPIO_IN */
if (fsel != BCM2835_FSEL_GPIO_IN && fsel != BCM2835_FSEL_GPIO_OUT)
bcm2835_pinctrl_fsel_set(pc, offset, BCM2835_FSEL_GPIO_IN);

/* disable by setting to GPIO_IN */
bcm2835_pinctrl_fsel_set(pc, offset, BCM2835_FSEL_GPIO_IN);
return 0;
}

Expand Down Expand Up @@ -962,10 +965,7 @@ static void bcm2835_pmx_gpio_disable_free(struct pinctrl_dev *pctldev,
struct pinctrl_gpio_range *range,
unsigned offset)
{
struct bcm2835_pinctrl *pc = pinctrl_dev_get_drvdata(pctldev);

/* disable by setting to GPIO_IN */
bcm2835_pinctrl_fsel_set(pc, offset, BCM2835_FSEL_GPIO_IN);
(void)bcm2835_pmx_free(pctldev, offset);
}

static int bcm2835_pmx_gpio_set_direction(struct pinctrl_dev *pctldev,
Expand Down

12 comments on commit 022689f

@seamusdemora
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would this change have an effect on how the gpioset ver. 2.1 works?

I ask b/c I've built libgpiod v 2.1 on my 'bullseye' system, and it doesn't work. My kernel ver is 6.1.21-v7+ #1642 SMP Mon Apr 3 17:20:52 BST 2023.

Thanks~

@lategoodbye
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@seamusdemora Yes, it has. Before this change a released GPIO always changed to input, which make it lose it output state.

@jue89
Copy link
Contributor

@jue89 jue89 commented on 022689f Apr 18, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, this one actually really surprising. This makes the Raspberry Pi working against the design principles of libgpiod. It's going back to INPUT on purpose to get the GPIO back into a known state. For those running scripts, using the sysfs gpio interface is available and perfectly fine.

I actually rely on this feature to get my machine into a safe state if my application just crashes.

@pelwell
Copy link
Contributor Author

@pelwell pelwell commented on 022689f Apr 19, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The problem with libgpiod isn't libgpiod as such - it's the way it interacts with the gpiod utilities. Your application is presumably running in a single process pulling in libgpiod in some way, and its lifetime can be easily tracked. If your application is a shell script that invokes the utilities directly, you lose that lifetime tracking, and all of a sudden you need to think in terms of running a gpioset in the background just to maintain a pin as an output driving in a particular direction - stuff that was easy with the sysfs interface.

If there was a gpiod agent (literally a gpiod(aemon)), similar to an ssh agent, that the utilities can coordinate with to maintain their state, or some other way of tracking a session, then I would never have made that change.

I'm open to adding a Device Tree/module parameter to toggle this behaviour - I do understand the attraction for a certain kind of user with a certain kind of application - but the out-of-the-box default on RPiOS is likely to remain as it is now.

@jue89
Copy link
Contributor

@jue89 jue89 commented on 022689f Apr 19, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see why this patch got into your kernel and what the use-case is. It's copying the known patterns introduced by the sysfs interface. That's why I'm recommending to keep using the sysfs driver when such a behavior is desired.

I was very surprised since no other platform does it like this and gpioset --help makes the following note:

Note
The state of a GPIO line controlled over the character device reverts to default
when the last process referencing the file descriptor representing the device file exits.
This means that it's wrong to run gpioset, have it exit and expect the line to continue
being driven high or low. It may happen if given pin is floating but it must be interpreted
as undefined behavior.

Your patch works around this (in my opinion sane) behavior.

My problem with this is a safety concern: I'm controlling a heater with a solid state relay which is controlled utilizing libgpiod. Having the heater powered on all the time makes the device overheating and eventually trips a thermal fuse. This happened to me yesterday, since a bug in my application crashed it and your patch let the heater in the powered state.

I guess to prevent from this happening again, I need to revert this patch and build my own kernel. Relying on the last-resort thermal fuse doesn't feel safe enough to me.

@pelwell
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does my offer of an option to disable this change at runtime, either with a module parameter or a Device Tree parameter, not appeal?

@jue89
Copy link
Contributor

@jue89 jue89 commented on 022689f Apr 19, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, that would help! Thank you! I didn't want to sound grumpy. The tripped thermal fuse still is bugging me :-D

I need a solution for that now. The kernel is already being built ;-) But for the long-term it would be great to be able to use your builds.

I'm curious - are users already relying on this behavior?

@pelwell
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's hard to tell. What I am certain of is that without this patch it would be harder to get people off sysfs and onto the gpiod utilities - they would be more likely to end up using one of the direct-to-hardware mechanisms, which we want to avoid.

@lategoodbye
Copy link
Contributor

@lategoodbye lategoodbye commented on 022689f Apr 19, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was very surprised since no other platform does it like this and gpioset --help makes the following note:

Another problem is here is that the libgpiod documentation is misleading and this has been fixed this year.

I also had the idea to making it configurable on my TODO, but didn't had the time for addressing this. The most valuable approach would be to control the behavior on pin level, but this would include a lot of work.

So at first i prefer a short term solution by making it configurable via module parameter, but applying the same behavior for all. For the mainline kernel there is still this fallback behavior and there we need to assume that there are users relying on this behavior. So there is a need for this, just coming from the other side.

@pelwell
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll add a module parameter, and a DT parameter to enable it in the downstream world.

@jue89
Copy link
Contributor

@jue89 jue89 commented on 022689f Apr 19, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was very surprised since no other platform does it like this and gpioset --help makes the following note:

Another problem is here is that the libgpiod documentation is misleading and this has been fixed this year.

What a mess ... I'd say that's a breaking change - at least for my use-case.

@pelwell
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See #6117. I suggest discussion continues there, because it is easy to lose commit comments.

Please sign in to comment.