| Message ID | 20250401-gpio-todo-remove-nonexclusive-v2-0-7c1380797b0d@linaro.org |
|---|---|
| Headers | show |
| Series | gpio: deprecate and track the removal of GPIO workarounds for regulators | expand |
On Tue, Apr 01, 2025 at 02:46:41PM +0200, Bartosz Golaszewski wrote: > Let's deprecate both symbols officially, add them to the MAINTAINERS > keywords so that it pops up on our radars when used again, add a task to > track it and I plan to use the power sequencing subsystem to handle the > cases where non-exclusive access to GPIOs is required. What exactly is the plan here? The regulator (and reset) usage seems like a reasonable one TBH - the real problem is having an API from the GPIO subsystem to discover sharing, at the minute you can't resolve a binding enough to find out if there's sharing without actually requesting the GPIO.
On Tue, Apr 01, 2025 at 04:42:40PM +0200, Bartosz Golaszewski wrote: > On Tue, Apr 1, 2025 at 3:27 PM Mark Brown <broonie@kernel.org> wrote: > > On Tue, Apr 01, 2025 at 02:46:41PM +0200, Bartosz Golaszewski wrote: > > > Let's deprecate both symbols officially, add them to the MAINTAINERS > > > keywords so that it pops up on our radars when used again, add a task to > > > track it and I plan to use the power sequencing subsystem to handle the > > > cases where non-exclusive access to GPIOs is required. > > What exactly is the plan here? The regulator (and reset) usage seems > > like a reasonable one TBH - the real problem is having an API from the > > GPIO subsystem to discover sharing, at the minute you can't resolve a > > binding enough to find out if there's sharing without actually > > requesting the GPIO. > Hard disagree on the reasonable usage. Let's consider the following: > You have two users and one goes gpiod_set_value(desc, 0), the other: > gpiod_set_value(desc, 1). Who is right? Depending on the timing the > resulting value may be either. That's why we need to figure out if there's sharing - the usage here is that we have some number of regulators all of which share the same GPIO and we want to know that this is the case, provide our own reference counting and use that to decide when to both update the GPIO and do the additional stuff like delays that are required. When the API was number based we could look up the GPIO numbers for each regulator, compare them with other GPIOs we've already got to identify sharing and then request only once. > For it to make sense, you'd have to add new interfaces: > gpiod_enable(desc) and gpiod_disable(), that would keep track of the > enable count. However you can't remove the hundreds of existing users > of gpiod_set_value() so the problem doesn't go away. But even if you > did introduce these new routines, what about > gpiod_direction_input/output()? My point is: the GPIO consumer API is > designed with exclusive usage in mind and it makes no sense to try to > ram shared GPIOs into the GPIO core. That's exactly what the regulator code was doing, as far as the GPIO API saw there was only ever one user at once. Since we can't look up numbers any more what we now do is use non-exclusive requests and check to see if we already have the GPIO descriptor, if we do then we group together like we were doing based on the GPIO numbers. The multiple gets are just there because that's how the gpiod API tells us if we've got two references to the same underlying GPIO, only one thing ever actually configures the GPIO. > 1. Audit all users of GPIOD_FLAGS_BIT_NONEXCLUSIVE > Outside of drivers/regulator/ it seems that there are several users > who don't really needs this (especially under sound/) and where using > this flag is just a result of a copy-paste. The sound use cases are roughly the same one - there's a bunch of audio designs where we've got shared reset lines, they're not actually doing the reference counting since the use cases mean that practically speaking all the users will make the same change at the same time (or at least never have actively conflicting needs) so practically it all ends up working fine. IIRC the long term plan was to move over to the reset API to clean this up rather than redoing the reference counting, if we're doing this properly we do want to get the thing the regulator API has where we know and can control when an actual transition happens. > 3. Use pwrseq where drivers really need non-exclusive GPIOs. > The power sequencing subsystem seems like a good candidate to fix the > issue. I imagine a faux_bus pwrseq driver that would plug into the > right places and provide pwrseq handles which the affected drivers > could either call directly via the pwrseq_get(), pwrseq_power_on/off() > interfaces, or we could have this pwrseq provider register as a GPIO > chip through which the gpiod_ calls from these consumers would go and > the sharing mediated by pwrseq. This seems complicated, and I'm not sure that obscuring the concrete thing we're dealing with isn't going to store up surprises for ourselves. It's also not clear to me that pwrseq doesn't just have the same problem with trying to figure out if two GPIO properties are actually pointing to the same underlying GPIO that everything else does? It seems like the base issue you've got here is that we can't figure out if we've got two things referencing the same GPIO without fully requesting it.
On Tue, Apr 1, 2025 at 6:00 PM Mark Brown <broonie@kernel.org> wrote: > > On Tue, Apr 01, 2025 at 04:42:40PM +0200, Bartosz Golaszewski wrote: > > On Tue, Apr 1, 2025 at 3:27 PM Mark Brown <broonie@kernel.org> wrote: > > > On Tue, Apr 01, 2025 at 02:46:41PM +0200, Bartosz Golaszewski wrote: > > > > > Let's deprecate both symbols officially, add them to the MAINTAINERS > > > > keywords so that it pops up on our radars when used again, add a task to > > > > track it and I plan to use the power sequencing subsystem to handle the > > > > cases where non-exclusive access to GPIOs is required. > > > > What exactly is the plan here? The regulator (and reset) usage seems > > > like a reasonable one TBH - the real problem is having an API from the > > > GPIO subsystem to discover sharing, at the minute you can't resolve a > > > binding enough to find out if there's sharing without actually > > > requesting the GPIO. > > > Hard disagree on the reasonable usage. Let's consider the following: > > > You have two users and one goes gpiod_set_value(desc, 0), the other: > > gpiod_set_value(desc, 1). Who is right? Depending on the timing the > > resulting value may be either. > > That's why we need to figure out if there's sharing - the usage here is > that we have some number of regulators all of which share the same GPIO > and we want to know that this is the case, provide our own reference > counting and use that to decide when to both update the GPIO and do the > additional stuff like delays that are required. When the API was number > based we could look up the GPIO numbers for each regulator, compare them > with other GPIOs we've already got to identify sharing and then request > only once. > That's not a good design though either, is it? For one: it relies on an implementation detail for which there's no API contract, namely the idea that the address of the struct gpiod_descr handed out by the call to gpiod_get() is the same for the same hardware offset on the same chip. It does work like that at the moment but it's a fragile assumption. The way pwrseq is implemented for instance, the "descriptor" obtained from the call to pwrseq_get() is instantiated per-user, meaning that each user of the same sequence has their own, unique descriptor. I don't see why such an approach could not be used in GPIOLIB one day. IOW: nobody ever said that there's a single struct gpiod_desc per GPIO line. > > For it to make sense, you'd have to add new interfaces: > > gpiod_enable(desc) and gpiod_disable(), that would keep track of the > > enable count. However you can't remove the hundreds of existing users > > of gpiod_set_value() so the problem doesn't go away. But even if you > > did introduce these new routines, what about > > gpiod_direction_input/output()? My point is: the GPIO consumer API is > > designed with exclusive usage in mind and it makes no sense to try to > > ram shared GPIOs into the GPIO core. > > That's exactly what the regulator code was doing, as far as the GPIO API > saw there was only ever one user at once. Since we can't look up > numbers any more what we now do is use non-exclusive requests and check > to see if we already have the GPIO descriptor, if we do then we group > together like we were doing based on the GPIO numbers. The multiple > gets are just there because that's how the gpiod API tells us if we've > got two references to the same underlying GPIO, only one thing ever > actually configures the GPIO. > That's not an unusual situation. For reset-gpios we now have the implicit wrapper in the form of the reset-gpio.c driver. Unfortunately we cannot just make it the fallback for all kinds of shared GPIOs so I suggested a bit more generalized approach with pwrseq. In any case: having this logic in the regulator core is not only wonky but also makes it impossible to unduplicate similar use-cases in audio and networking where shared GPIOs have nothing to do with regulators. > > 1. Audit all users of GPIOD_FLAGS_BIT_NONEXCLUSIVE > > > Outside of drivers/regulator/ it seems that there are several users > > who don't really needs this (especially under sound/) and where using > > this flag is just a result of a copy-paste. > > The sound use cases are roughly the same one - there's a bunch of audio > designs where we've got shared reset lines, they're not actually doing > the reference counting since the use cases mean that practically > speaking all the users will make the same change at the same time (or at > least never have actively conflicting needs) so practically it all ends > up working fine. IIRC the long term plan was to move over to the reset > API to clean this up rather than redoing the reference counting, if > we're doing this properly we do want to get the thing the regulator API > has where we know and can control when an actual transition happens. > If they actually exist as "reset-gpios" in DT then they could probably use the reset-gpio.c driver. I will take a look. > > 3. Use pwrseq where drivers really need non-exclusive GPIOs. > > > The power sequencing subsystem seems like a good candidate to fix the > > issue. I imagine a faux_bus pwrseq driver that would plug into the > > right places and provide pwrseq handles which the affected drivers > > could either call directly via the pwrseq_get(), pwrseq_power_on/off() > > interfaces, or we could have this pwrseq provider register as a GPIO > > chip through which the gpiod_ calls from these consumers would go and > > the sharing mediated by pwrseq. > > This seems complicated, and I'm not sure that obscuring the concrete > thing we're dealing with isn't going to store up surprises for > ourselves. IMO It would be equally as obscured if you used a shared GPIO wrapped in a reset driver. > > It's also not clear to me that pwrseq doesn't just have the same problem > with trying to figure out if two GPIO properties are actually pointing > to the same underlying GPIO that everything else does? It seems like > the base issue you've got here is that we can't figure out if we've got > two things referencing the same GPIO without fully requesting it. Whether that's feasible (I think it is but I'll have a definite answer once I spend more time on this) is one question. Another is: do you have anything against removing this flag given it's replaced with a better solution? If not, then I'd still like to apply this series and we can discuss the actual solution once I send out the code. I hope this will at least start happening this release cycle. Bart
On Tue, Apr 01, 2025 at 08:57:56PM +0200, Bartosz Golaszewski wrote: > On Tue, Apr 1, 2025 at 6:00 PM Mark Brown <broonie@kernel.org> wrote: > > On Tue, Apr 01, 2025 at 04:42:40PM +0200, Bartosz Golaszewski wrote: > > > You have two users and one goes gpiod_set_value(desc, 0), the other: > > > gpiod_set_value(desc, 1). Who is right? Depending on the timing the > > > resulting value may be either. > > That's why we need to figure out if there's sharing - the usage here is > > that we have some number of regulators all of which share the same GPIO > > and we want to know that this is the case, provide our own reference > > counting and use that to decide when to both update the GPIO and do the > > additional stuff like delays that are required. When the API was number > > based we could look up the GPIO numbers for each regulator, compare them > > with other GPIOs we've already got to identify sharing and then request > > only once. > That's not a good design though either, is it? For one: it relies on > an implementation detail for which there's no API contract, namely the There is an API contract as far as I'm concerned, this was discussed when Russell was converting things over to use descriptors since we need something to maintain functionality. I agree that this is an interface that is more convenient than elegant but it's what was on offer, I think the enthusiasm for converting to gpiod was such people were OK with it since it does actually do the right thing. > idea that the address of the struct gpiod_descr handed out by the call > to gpiod_get() is the same for the same hardware offset on the same > chip. It does work like that at the moment but it's a fragile > assumption. The way pwrseq is implemented for instance, the > "descriptor" obtained from the call to pwrseq_get() is instantiated > per-user, meaning that each user of the same sequence has their own, > unique descriptor. I don't see why such an approach could not be used > in GPIOLIB one day. IOW: nobody ever said that there's a single struct > gpiod_desc per GPIO line. If gpiolib were to change this API we'd need some other way of getting the same functionality, I'd be totally fine with that happening. For regulators we don't really want the pwrseq behaviour, we want to know that there's a single underlying GPIO that we're updating. > > That's exactly what the regulator code was doing, as far as the GPIO API > > saw there was only ever one user at once. Since we can't look up > > numbers any more what we now do is use non-exclusive requests and check > > to see if we already have the GPIO descriptor, if we do then we group > > together like we were doing based on the GPIO numbers. The multiple > > gets are just there because that's how the gpiod API tells us if we've > > got two references to the same underlying GPIO, only one thing ever > > actually configures the GPIO. > That's not an unusual situation. For reset-gpios we now have the > implicit wrapper in the form of the reset-gpio.c driver. Unfortunately > we cannot just make it the fallback for all kinds of shared GPIOs so I > suggested a bit more generalized approach with pwrseq. In any case: > having this logic in the regulator core is not only wonky but also > makes it impossible to unduplicate similar use-cases in audio and > networking where shared GPIOs have nothing to do with regulators. Impossible seems pretty strong here? Part of the thing here is that the higher level users want to understand that there is GPIO sharing going on and do something about it, the working out that the thing is shared isn't really the interesting bit it's rather the part where we do something about that. It's not that you can't share some code but it feels more like a library than an opaque abstraction. > > The sound use cases are roughly the same one - there's a bunch of audio > > designs where we've got shared reset lines, they're not actually doing > > the reference counting since the use cases mean that practically > > speaking all the users will make the same change at the same time (or at > > least never have actively conflicting needs) so practically it all ends > > up working fine. IIRC the long term plan was to move over to the reset > > API to clean this up rather than redoing the reference counting, if > > we're doing this properly we do want to get the thing the regulator API > > has where we know and can control when an actual transition happens. > If they actually exist as "reset-gpios" in DT then they could probably > use the reset-gpio.c driver. I will take a look. Yes, that was the idea - there was some issue I can't remember that meant it needed a bit of work on the reset API the last time someone looked at it. The properties might have different names reflecting the pins or something but that seems like a readily solvable problem. Though now I think again some of them might be closer to the regulator enables rather than resets so those ones would not fit there and would more want to librify what regulator is doing... Something like that would definitely not feel right being described as a power sequence. > > > 3. Use pwrseq where drivers really need non-exclusive GPIOs. > > > The power sequencing subsystem seems like a good candidate to fix the > > > issue. I imagine a faux_bus pwrseq driver that would plug into the > > > right places and provide pwrseq handles which the affected drivers > > > could either call directly via the pwrseq_get(), pwrseq_power_on/off() > > > interfaces, or we could have this pwrseq provider register as a GPIO > > > chip through which the gpiod_ calls from these consumers would go and > > > the sharing mediated by pwrseq. > > This seems complicated, and I'm not sure that obscuring the concrete > > thing we're dealing with isn't going to store up surprises for > > ourselves. > IMO It would be equally as obscured if you used a shared GPIO wrapped > in a reset driver. Yeah, it's a bit indirected but it's at least clear that it's just the reset and not also any other aspect of the power management so you don't have to worry about timing requirements around enabling supplies or whatever. > > It's also not clear to me that pwrseq doesn't just have the same problem > > with trying to figure out if two GPIO properties are actually pointing > > to the same underlying GPIO that everything else does? It seems like > > the base issue you've got here is that we can't figure out if we've got > > two things referencing the same GPIO without fully requesting it. > Whether that's feasible (I think it is but I'll have a definite answer > once I spend more time on this) is one question. Another is: do you > have anything against removing this flag given it's replaced with a > better solution? If not, then I'd still like to apply this series and > we can discuss the actual solution once I send out the code. I hope > this will at least start happening this release cycle. I'm in no way attached to this specific solution, from my point of view the important thing is that given two devices using GPIOs we have some reasonably convenient way of telling if they're using the same underlying GPIO and can coordinate between the devices appropriately.
On Tue, Apr 1, 2025 at 11:55 PM Mark Brown <broonie@kernel.org> wrote: > > On Tue, Apr 01, 2025 at 08:57:56PM +0200, Bartosz Golaszewski wrote: > > On Tue, Apr 1, 2025 at 6:00 PM Mark Brown <broonie@kernel.org> wrote: > > > On Tue, Apr 01, 2025 at 04:42:40PM +0200, Bartosz Golaszewski wrote: > > > > > You have two users and one goes gpiod_set_value(desc, 0), the other: > > > > gpiod_set_value(desc, 1). Who is right? Depending on the timing the > > > > resulting value may be either. > > > > That's why we need to figure out if there's sharing - the usage here is > > > that we have some number of regulators all of which share the same GPIO > > > and we want to know that this is the case, provide our own reference > > > counting and use that to decide when to both update the GPIO and do the > > > additional stuff like delays that are required. When the API was number > > > based we could look up the GPIO numbers for each regulator, compare them > > > with other GPIOs we've already got to identify sharing and then request > > > only once. > > > That's not a good design though either, is it? For one: it relies on > > an implementation detail for which there's no API contract, namely the > > There is an API contract as far as I'm concerned, this was discussed > when Russell was converting things over to use descriptors since we need > something to maintain functionality. I agree that this is an interface > that is more convenient than elegant but it's what was on offer, I think > the enthusiasm for converting to gpiod was such people were OK with it > since it does actually do the right thing. > Something having been discussed years ago in times of a different maintainer does not really constitute an API contract :). I understand the reasoning at the time but I really dislike comparing raw pointers in particular and this whole abstraction reversal in general. I'd like to at least have something like gpiod_cmp() or gpiod_is_equivalent() that would hide the actual logic. Even if, for now, it just remains a simple pointer comparison. > > idea that the address of the struct gpiod_descr handed out by the call > > to gpiod_get() is the same for the same hardware offset on the same > > chip. It does work like that at the moment but it's a fragile > > assumption. The way pwrseq is implemented for instance, the > > "descriptor" obtained from the call to pwrseq_get() is instantiated > > per-user, meaning that each user of the same sequence has their own, > > unique descriptor. I don't see why such an approach could not be used > > in GPIOLIB one day. IOW: nobody ever said that there's a single struct > > gpiod_desc per GPIO line. > > If gpiolib were to change this API we'd need some other way of getting > the same functionality, I'd be totally fine with that happening. For > regulators we don't really want the pwrseq behaviour, we want to know > that there's a single underlying GPIO that we're updating. > This is bothering me. This is the abstraction reversal I'm talking about. Should the regulator drivers even be concerned about whether they share resources or not? It's not like consumers of regulators are concerned about sharing them with other devices. I'm not saying GPIOs should become reference counted - as I said in a previous email, I don't believe this makes sense - but GPIOLIB is a lower-level abstraction to regulators thus the latter shouldn't really reach into the GPIO core and inspect its structures in order to figure out whether the lines are shared. This is where an intermediate abstraction layer may be helpful. The regulator drivers then just go: handle = pwrseq_get(dev, "enable-gpios"); pwrseq_power_on(handle); Even if we do it in multiple places, as long as the enable count is balanced, we're good. The consumers are not concerned by what's happening behind the scenes just as if it was a reset handle. > > > That's exactly what the regulator code was doing, as far as the GPIO API > > > saw there was only ever one user at once. Since we can't look up > > > numbers any more what we now do is use non-exclusive requests and check > > > to see if we already have the GPIO descriptor, if we do then we group > > > together like we were doing based on the GPIO numbers. The multiple > > > gets are just there because that's how the gpiod API tells us if we've > > > got two references to the same underlying GPIO, only one thing ever > > > actually configures the GPIO. > > > That's not an unusual situation. For reset-gpios we now have the > > implicit wrapper in the form of the reset-gpio.c driver. Unfortunately > > we cannot just make it the fallback for all kinds of shared GPIOs so I > > suggested a bit more generalized approach with pwrseq. In any case: > > having this logic in the regulator core is not only wonky but also > > makes it impossible to unduplicate similar use-cases in audio and > > networking where shared GPIOs have nothing to do with regulators. > > Impossible seems pretty strong here? Part of the thing here is that the > higher level users want to understand that there is GPIO sharing going > on and do something about it, the working out that the thing is shared > isn't really the interesting bit it's rather the part where we do > something about that. It's not that you can't share some code but it > feels more like a library than an opaque abstraction. > The part where "the higher level users want to understand that there is GPIO sharing going on" does not sound correct. Let's take the example of drivers/net/phy/mscc/mscc_ptp.c which uses the non-exclusive flag for gpiod_get() because on one of the MIPS platforms, there are four instances of this PHY that share a GPIO. IMO it's a hack, this driver shouldn't care about it. It reverses the idea of the DT providing hardware information to drivers and instead the driver knows that the DT may describe a shared GPIO. > > > The sound use cases are roughly the same one - there's a bunch of audio > > > designs where we've got shared reset lines, they're not actually doing > > > the reference counting since the use cases mean that practically > > > speaking all the users will make the same change at the same time (or at > > > least never have actively conflicting needs) so practically it all ends > > > up working fine. IIRC the long term plan was to move over to the reset > > > API to clean this up rather than redoing the reference counting, if > > > we're doing this properly we do want to get the thing the regulator API > > > has where we know and can control when an actual transition happens. > > > If they actually exist as "reset-gpios" in DT then they could probably > > use the reset-gpio.c driver. I will take a look. > > Yes, that was the idea - there was some issue I can't remember that > meant it needed a bit of work on the reset API the last time someone > looked at it. The properties might have different names reflecting the > pins or something but that seems like a readily solvable problem. > As long as the hardware description says it's a reset GPIO, we're fine. It's when people try to use the reset framework for something that's not a reset when DT maintainers complain. > Though now I think again some of them might be closer to the regulator > enables rather than resets so those ones would not fit there and would > more want to librify what regulator is doing... Something like that > would definitely not feel right being described as a power sequence. > Well, pwrseq is just a naming convention for want of a better one. It really is just a subsystem that mediates usage of shared resources and doesn't bind to any specific kernel subsystem. [snip] > > > > It's also not clear to me that pwrseq doesn't just have the same problem > > > with trying to figure out if two GPIO properties are actually pointing > > > to the same underlying GPIO that everything else does? It seems like > > > the base issue you've got here is that we can't figure out if we've got > > > two things referencing the same GPIO without fully requesting it. > > > Whether that's feasible (I think it is but I'll have a definite answer > > once I spend more time on this) is one question. Another is: do you > > have anything against removing this flag given it's replaced with a > > better solution? If not, then I'd still like to apply this series and > > we can discuss the actual solution once I send out the code. I hope > > this will at least start happening this release cycle. > > I'm in no way attached to this specific solution, from my point of view > the important thing is that given two devices using GPIOs we have some > reasonably convenient way of telling if they're using the same underlying > GPIO and can coordinate between the devices appropriately. I think that logically, consumers of GPIOs shouldn't care about whether they're shared. IOW: the non-exclusive flag passed to gpiod_get() should go. If the opposition to using pwrseq here is strong, then I'd suggest at least moving the handling of the non-exclusive flag into gpiolib quirks (in gpiolib-of.c or elsewhere) and not exposing it to consumers who have no business knowing it. I believe pwrseq could actually be used to hide the enable counting for GPIOs behind a faux GPIO chip and the consumer would never see a pwrseq handle - they would instead use GPIO consumer interfaces and we'd have to agree on what logic would we put behind gpiod_set_value() (should it effectively work as gpiod_enable() meaning: value is 1 as long as at least one user sets it to 1?) and gpiod_direction_input()/output() (same thing: highest priority is gpiod_direction_output(HIGH) and as long as at least one user sets it as such, we keep it). Bartosz
On Wed, Apr 02, 2025 at 12:05:24PM +0200, Bartosz Golaszewski wrote: > On Tue, Apr 1, 2025 at 11:55 PM Mark Brown <broonie@kernel.org> wrote: > > If gpiolib were to change this API we'd need some other way of getting > > the same functionality, I'd be totally fine with that happening. For > > regulators we don't really want the pwrseq behaviour, we want to know > > that there's a single underlying GPIO that we're updating. > This is bothering me. This is the abstraction reversal I'm talking > about. Should the regulator drivers even be concerned about whether > they share resources or not? It's not like consumers of regulators are For regulators there's similar issues with needing to know when things happen (eg, to know if the device actually got reset and needs to be reintialised) but it's much more likely that we'll both be sharing and not actually have any control at all even for unshared regulators so we provide notifiers for this case instead. > concerned about sharing them with other devices. I'm not saying GPIOs > should become reference counted - as I said in a previous email, I > don't believe this makes sense - but GPIOLIB is a lower-level > abstraction to regulators thus the latter shouldn't really reach into > the GPIO core and inspect its structures in order to figure out > whether the lines are shared. This is where an intermediate > abstraction layer may be helpful. The regulator drivers then just go: > handle = pwrseq_get(dev, "enable-gpios"); > pwrseq_power_on(handle); > Even if we do it in multiple places, as long as the enable count is > balanced, we're good. The consumers are not concerned by what's > happening behind the scenes just as if it was a reset handle. No, it's important when (or if) the enable was physically released so we need to know when the actual hardware operation happened - there's some delay before the hardware has finished implementing the enable. > > Impossible seems pretty strong here? Part of the thing here is that the > > higher level users want to understand that there is GPIO sharing going > > on and do something about it, the working out that the thing is shared > > isn't really the interesting bit it's rather the part where we do > > something about that. It's not that you can't share some code but it > > feels more like a library than an opaque abstraction. > The part where "the higher level users want to understand that there > is GPIO sharing going on" does not sound correct. Let's take the > example of drivers/net/phy/mscc/mscc_ptp.c which uses the > non-exclusive flag for gpiod_get() because on one of the MIPS > platforms, there are four instances of this PHY that share a GPIO. IMO > it's a hack, this driver shouldn't care about it. It reverses the idea > of the DT providing hardware information to drivers and instead the > driver knows that the DT may describe a shared GPIO. Knowing if and when a reset line is actually asserted seems like an important an useful thing for a driver to know, for example if a chip is actually reset then we may need to do expensive reinitialisation which could be skipped if that didn't happen. > > Though now I think again some of them might be closer to the regulator > > enables rather than resets so those ones would not fit there and would > > more want to librify what regulator is doing... Something like that > > would definitely not feel right being described as a power sequence. > Well, pwrseq is just a naming convention for want of a better one. It > really is just a subsystem that mediates usage of shared resources and > doesn't bind to any specific kernel subsystem. So what's the sequencing bit about then? Having something that just shares resources might be useful here, but the sequencing bit seems like it's asking for landmines. > > I'm in no way attached to this specific solution, from my point of view > > the important thing is that given two devices using GPIOs we have some > > reasonably convenient way of telling if they're using the same underlying > > GPIO and can coordinate between the devices appropriately. > I think that logically, consumers of GPIOs shouldn't care about > whether they're shared. IOW: the non-exclusive flag passed to > gpiod_get() should go. If the opposition to using pwrseq here is > strong, then I'd suggest at least moving the handling of the > non-exclusive flag into gpiolib quirks (in gpiolib-of.c or elsewhere) > and not exposing it to consumers who have no business knowing it. I don't think the nonexclusive flag is essential so long as we can provide some way for users to discover what's going on and coordinate with each other. I do think it's important for users to know about at least the impacts of sharing, and I suspect that for GPIOs usability means knowing about the sharing. > I believe pwrseq could actually be used to hide the enable counting > for GPIOs behind a faux GPIO chip and the consumer would never see a > pwrseq handle - they would instead use GPIO consumer interfaces and > we'd have to agree on what logic would we put behind gpiod_set_value() > (should it effectively work as gpiod_enable() meaning: value is 1 as > long as at least one user sets it to 1?) and > gpiod_direction_input()/output() (same thing: highest priority is > gpiod_direction_output(HIGH) and as long as at least one user sets it > as such, we keep it). Like I say that doesn't do the right thing since other users need to be able to see when something changes on the GPIO. If that just happens on normal gpiolib then that complicates usage for the default case since they now have to worry about things not actually happening when requested which doesn't seem ideal.
The GPIOD_FLAGS_BIT_NONEXCLUSIVE flag and devm_gpiod_unhinge() helpers were introduced as hacky workarounds for resource ownership issues in the regulator subsystem. Unfortunately, people started using the former in other places too and now it's in all kinds of drivers. Let's deprecate both symbols officially, add them to the MAINTAINERS keywords so that it pops up on our radars when used again, add a task to track it and I plan to use the power sequencing subsystem to handle the cases where non-exclusive access to GPIOs is required. Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> --- Changes in v2: - reword the task description to focus more on the solution than the problem description - add a task for removing devm_gpiod_unhinge() as well - Link to v1: https://lore.kernel.org/r/20250331-gpio-todo-remove-nonexclusive-v1-0-25f72675f304@linaro.org --- Bartosz Golaszewski (4): gpio: deprecate the GPIOD_FLAGS_BIT_NONEXCLUSIVE flag gpio: deprecate devm_gpiod_unhinge() MAINTAINERS: add more keywords for the GPIO subsystem entry gpio: TODO: track the removal of regulator-related workarounds MAINTAINERS | 2 ++ drivers/gpio/TODO | 34 ++++++++++++++++++++++++++++++++++ drivers/gpio/gpiolib-devres.c | 6 +++++- include/linux/gpio/consumer.h | 1 + 4 files changed, 42 insertions(+), 1 deletion(-) --- base-commit: 405e2241def89c88f008dcb899eb5b6d4be8b43c change-id: 20250331-gpio-todo-remove-nonexclusive-ed875467eb56 Best regards,