[PATCH 000/182] Rid struct gpio_chip from container_of() usage
This removes the use of container_of() constructions from *all* GPIO drivers in the kernel. It is done by instead adding an optional void *data pointer to the struct gpio_chip and an accessor function, gpiochip_get_data() to get it from a driver. WHY? Because we want to have a proper userspace ABI for GPIO chips, which involves using a character device that the user opens and closes. While the character device is open, the underlying kernel objects must not go away. Currently the GPIO drivers keep their state in the struct gpio_chip, and that is often allocated by the drivers, very often as a part of a containing per-instance state container struct for the driver: struct foo_state { struct gpio_chip chip; <- OMG my state is there }; Drivers cannot allocate and manage this state: if a user has the character device open, the objects allocated must stay around even if the driver goes away. Instead drivers need to pass a descriptor to the GPIO core, and then the core should allocate and manage the lifecycle of things related to the device, such as the chardev itself or the struct device related to the GPIO device. Thus what is today gpiochip_add() (and after this patch set gpiochip_add_data()) must return a pointer/cookie to an object maintained elsewhere. The object passed back to the functions in the vtable of a gpio_chip like .request(), .get_value(), .set_value() etc must also be changed. Thus container_of() constructions will not work, because container_of(), clever as it is, does not work with a member that is a pointer: it needs a full struct inside the containing struct. This patch set takes the first step by eliminating container_of() and replacing it with a data pointer mechanism. The patch set is just a step in the larger refactoring needed but stands on its own as it happens to remove 500+ lines of code and (IMO) makes the code in all drivers easier to read and understand. I need ACKs to merge this through the GPIO tree. I do not think we will see collissions in the merge window but if you're adding new gpio_chips or deleting drivers, write me so we can work something out. (Immutable branches that I can pull and make fixes on top of would be the best.) And as mentioned this kind of series will continue. If you think it looks invasive, think of the changes done for irq_chip over the year. GPIO is in a similar situation. Linus Walleij (182): gpio: forward-declare enum gpiod_flags gpio: add a data pointer to gpio_chip gpio: of: provide optional of_mm_gpiochip_add_data() function gpio: generic: factor into gpio_chip struct gpio: 104-idi-48: use gpiochip data pointer gpio: 104-idio-16: use gpiochip data pointer gpio: 74x164: use gpiochip data pointer gpio: adnp: use gpiochip data pointer gpio: adp5520: use gpiochip data pointer gpio: adp5588: use gpiochip data pointer gpio: altera: use gpiochip data pointer gpio: amd8111: use gpiochip data pointer gpio: amdpt: use gpiochip data pointer gpio: arizona: use gpiochip data pointer gpio: ath79: use gpiochip data pointer gpio: bcm-kona: use gpiochip data pointer gpio: bt8xx: use gpiochip data pointer gpio: crystalcove: use gpiochip data pointer gpio: cs5535: use gpiochip data pointer gpio: da9052: use gpiochip data pointer gpio: da9055: use gpiochip data pointer gpio: davinci: use gpiochip data pointer gpio: dln2: use gpiochip data pointer gpio: em: use gpiochip data pointer gpio: f7188: use gpiochip data pointer gpio: intel-mid: use gpiochip data pointer gpio: it87: use gpiochip data pointer gpio: kempld: use gpiochip data pointer gpio: lp3943: use gpiochip data pointer gpio: lpc18xx: use gpiochip data pointer gpio: lpc32xx: use gpiochip data pointer gpio: lynxpoint: use gpiochip data pointer gpio: max730x: use gpiochip data pointer gpio: max732x: use gpiochip data pointer gpio: mb86s7x: use gpiochip data pointer gpio: mc33880: use gpiochip data pointer gpio: mc9s08dz60: use gpiochip data pointer gpio: mcp: use gpiochip data pointer gpio: ml-ioh: use gpiochip data pointer gpio: mm-lantiq: use gpiochip data pointer gpio: mpc5200: use gpiochip data pointer gpio: mpc8xxx: use gpiochip data pointer gpio: msic: use gpiochip data pointer gpio: mvebu: use gpiochip data pointer gpio: octeon: use gpiochip data pointer gpio: omap: use gpiochip data pointer gpio: palmas: use gpiochip data pointer gpio: pca953x: use gpiochip data pointer gpio: pcf857x: use gpiochip data pointer gpio: pch: use gpiochip data pointer gpio: pl061: use gpiochip data pointer gpio: pxa: use gpiochip data pointer gpio: rc5t583: use gpiochip data pointer gpio: rcar: use gpiochip data pointer gpio: rdc321x: use gpiochip data pointer gpio: samsung: use gpiochip data pointer gpio: sch: use gpiochip data pointer gpio: sch311x: use gpiochip data pointer gpio: spear-spics: use gpiochip data pointer gpio: sta2x11: use gpiochip data pointer gpio: stmpe: use gpiochip
Re: [PATCH 000/182] Rid struct gpio_chip from container_of() usage
On Wed, Dec 09, 2015 at 02:08:35PM +0100, Linus Walleij wrote: > Because we want to have a proper userspace ABI for GPIO chips, > which involves using a character device that the user opens > and closes. While the character device is open, the underlying > kernel objects must not go away. Okay, so you stop the gpio_chip struct from going away. What about the code which is called via gpio_chip - say, if userspace keep shte chardev open, and someone rmmod's the driver providing the GPIO driver. I'm not sure that splitting up objects in this way really solves anything at all. Yes, it divorses the driver's private data from the subsystem data, but is that really an advantage? Network drivers have a similar issue, and the way this problem is solved there is that alloc_netdev() is always used to allocate the subsystem data structure and any driver private data structure as one allocation, and the lifetime of both objects remains under the control of the subsystem. The allocated memory is only freed when the last user goes away, and net has protection to prevent an unregistered driver from being called (via locks on every path into the layer.) Things get a little more complex with gpio, because there's the issue that some methods are spinlocked while others can take semaphores, but it should be possible to come up with a solution to that - maybe an atomic_t which is incremented whenever we're in some operation provided it's >= 0 (otherwise it fails), and decremented when the operation completes. We can then control in the unregistration path further GPIO accesses, and also prevent new accesses occuring by setting the atomic_t to -1. This shouldn't require any additional locking in any path. It does mean that the unregistration path needs careful thought to ensure that when we set it to -1, we wait for it to be dropped by the appropriate amount. -- RMK's Patch system: http://www.arm.linux.org.uk/developer/patches/ FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up according to speedtest.net. -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 000/182] Rid struct gpio_chip from container_of() usage
On Wed, Dec 9, 2015 at 2:44 PM, Russell King - ARM Linux wrote: Thanks Russell, I think you speed up the design and shorten the development time by providing these ideas, so it is much, much appreciated. > On Wed, Dec 09, 2015 at 02:08:35PM +0100, Linus Walleij wrote: >> Because we want to have a proper userspace ABI for GPIO chips, >> which involves using a character device that the user opens >> and closes. While the character device is open, the underlying >> kernel objects must not go away. > > Okay, so you stop the gpio_chip struct from going away. Actually I was going to create a new struct gpio_device, but yes. > What > about the code which is called via gpio_chip - say, if userspace > keeps the chardev open, and someone rmmod's the driver providing > the GPIO driver. The idea was that when what is today gpiochip_remove() is called by the gpiochip driver, the static data pointer to the vtable with all callbacks is set to NULL (in some safe way), and the code avoids calling these, so the device goes numb. I think you give me the right solution to this "safe way" below, thanks! > I'm not sure that splitting up objects in this way really solves > anything at all. Yes, it divorses the driver's private data from > the subsystem data, but is that really an advantage? I like the design pattern you describe below, and I have no strong opinion on it. If there is a precedent in the kernel to do it with a separate alloc_foo() function I can do that. (Would like Greg's and/or Johan's opinion on this though.) > Network drivers have a similar issue, and the way this problem is > solved there is that alloc_netdev() is always used to allocate the > subsystem data structure and any driver private data structure as > one allocation, and the lifetime of both objects remains under the > control of the subsystem. > > The allocated memory is only freed when the last user goes away, > and net has protection to prevent an unregistered driver from > being called (via locks on every path into the layer.) That's a viable way to solve this. The refactoring would be bigger in size and different. The current patch set would still be needed though, as the drivers will still not be able to use the container_of() design pattern with this design either, as the gpiolib core handles allocation of its own struct and then some more. With what I had in mind it would be something like: struct foo_gpio { struct gpio_device *gd; }; const struct gpio_ops foo_ops = { .set = ... .get = ... }; foo = devm_kzalloc(dev, ...); foo->gd = gpio_add_device(&foo_ops, ...); ... gpio_remove_device(foo->gd); But with the alloc_gpiodev() pattern we get this: struct foo_gpio { }; static void setup(struct gpio_device *gd) { gd->set = ...; gd->get = ...; } foo = gpio_alloc_device(sizeof(foo), name, setup); struct gpio_device *gd = gpio_add_device(foo); ... gpio_remove_device(gd); The setup call is clever to use also with GPIO because we can use that to setup mmio-gpio drivers and easier refactor those drivers that assign members to gpio_chip dynamically. Also I think this case would involve getting rid of all static vtables and only use that setup call to assign function pointers akin to what we have for gpio_chip. Lots of work, but I like it. We can do allocation in early boot these days even if the gpiochip is added with an early initcall right? > Things get a little more complex with gpio, because there's the > issue that some methods are spinlocked while others can take > semaphores, but it should be possible to come up with a solution > to that - maybe an atomic_t which is incremented whenever we're > in some operation provided it's >= 0 (otherwise it fails), and > decremented when the operation completes. We can then control > in the unregistration path further GPIO accesses, and also > prevent new accesses occuring by setting the atomic_t to -1. > This shouldn't require any additional locking in any path. It > does mean that the unregistration path needs careful thought to > ensure that when we set it to -1, we wait for it to be dropped > by the appropriate amount. Yeah we will both in spinlocks/hotpath and semaphores/mutexes/slowpath in these ops for sure :/ The atomic hack should work: I understand that you mean we read it and set it to -1 and if it was 2 wait for it to become -3, then conclude unregistration with callbacks numbed. Then there is a particular case that occurs with USB or similar pluggable buses, where there is a glitch or powercycle on the bus, and the same device goes away and comes back in a few milliseconds, and that means it should reattach to the character device that is already open. Making that work is however non-trivial :( Yours, Linus Walleij -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 000/182] Rid struct gpio_chip from container_of() usage
On Wed, Dec 09, 2015 at 02:08:35PM +0100, Linus Walleij wrote: > This removes the use of container_of() constructions from *all* > GPIO drivers in the kernel. It is done by instead adding an > optional void *data pointer to the struct gpio_chip and an > accessor function, gpiochip_get_data() to get it from a driver. > > WHY? > > Because we want to have a proper userspace ABI for GPIO chips, > which involves using a character device that the user opens > and closes. While the character device is open, the underlying > kernel objects must not go away. > > Currently the GPIO drivers keep their state in the struct > gpio_chip, and that is often allocated by the drivers, very > often as a part of a containing per-instance state container > struct for the driver: > > struct foo_state { >struct gpio_chip chip; <- OMG my state is there > }; > > Drivers cannot allocate and manage this state: if a user has the > character device open, the objects allocated must stay around > even if the driver goes away. Instead drivers need to pass a > descriptor to the GPIO core, and then the core should allocate > and manage the lifecycle of things related to the device, such > as the chardev itself or the struct device related to the GPIO > device. Yes, but it does not mean that the object that is being maintained by the subsystem and that us attached to character device needs to be gpio_chip itself. You can have something like struct gpio_chip_chardev { struct cdev chardev; struct gpio_chip *chip; bool dead; }; struct gpio_chip { ... struct gpio_chip_chardev *chardev; ... }; You alloctae the new structure when you register/export gpio chip in gpio subsystem core and leave all the individual drivers alone. Thanks. -- Dmitry -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 000/182] Rid struct gpio_chip from container_of() usage
On Wed, Dec 9, 2015 at 8:30 PM, Dmitry Torokhov wrote: > On Wed, Dec 09, 2015 at 02:08:35PM +0100, Linus Walleij wrote: >> This removes the use of container_of() constructions from *all* >> GPIO drivers in the kernel. It is done by instead adding an >> optional void *data pointer to the struct gpio_chip and an >> accessor function, gpiochip_get_data() to get it from a driver. >> >> WHY? >> >> Because we want to have a proper userspace ABI for GPIO chips, >> which involves using a character device that the user opens >> and closes. While the character device is open, the underlying >> kernel objects must not go away. >> >> Currently the GPIO drivers keep their state in the struct >> gpio_chip, and that is often allocated by the drivers, very >> often as a part of a containing per-instance state container >> struct for the driver: >> >> struct foo_state { >>struct gpio_chip chip; <- OMG my state is there >> }; >> >> Drivers cannot allocate and manage this state: if a user has the >> character device open, the objects allocated must stay around >> even if the driver goes away. Instead drivers need to pass a >> descriptor to the GPIO core, and then the core should allocate >> and manage the lifecycle of things related to the device, such >> as the chardev itself or the struct device related to the GPIO >> device. > > Yes, but it does not mean that the object that is being maintained by > the subsystem and that us attached to character device needs to be > gpio_chip itself. You can have something like > > struct gpio_chip_chardev { > struct cdev chardev; > struct gpio_chip *chip; > bool dead; > }; There needs to be a struct device too, amongst other things. > > struct gpio_chip { > ... > struct gpio_chip_chardev *chardev; > ... > }; > > You alloctae the new structure when you register/export gpio chip in > gpio subsystem core and leave all the individual drivers alone. The current idea I have is something in the middle. Drivers have to change a bit. The important part is that gpiolib handles allocation of anything containing states. I'm thinking along the lines of Russell's proposal to use netdev_alloc()'s design pattern. The problem is that currently gpio_chip contains a lot of stateful variables (like the dynamically allocated array of GPIO descriptors etc) and those are used by the gpiolib core, so they have to be moved away from gpio_chip. So what happens if I don't change the design pattern: int ret = gpiochip_add(&my_chip); ... gpiochip_remove(&my_chip); At this point we have to cross-reference the pointer to my chip to find the chip to remove. This goes for anything that takes the struct gpio_chip * as parameter, like gpiochip_add_pin_range(), gpiochip_request_own_desc() etc etc. So something inside gpiolib must take a gpio_chip * pointer and turn that into the actual state container, e.g, a struct gpio_device. Since struct gpio_chip needs to be static and stateless, it cannot contain a pointer back to its struct gpio_device. That means basically comparing pointers across a list of gpio_device's to find it. And that's ... very kludgy. But if people think it's better to avoid changing all drivers I will consider it. I think it is better if the GPIO drivers get a handle on the real gpio_device * to be used when calling these gpiochip_* related functions and also in the callbacks, which is a bigger refactoring indeed. Part of this is trying to be elegant and mimic other subsystems and not have GPIO be too hopelessly wayward about things. If I compare to how struct input_dev is done, you appear to also use the pattern Russell suggested with input_dev_allocate() akin to netdev_alloc(), and the allocated struct holds all the vtable and states etc, and I think it is a good pattern, and that GPIO should conform. This current patch series however, just give us the equivalent of input_get_drvdata()/input_set_drvdata() and that seems valuable on its own, as it reduces code size and make things more readable. Yours, Linus Walleij -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 000/182] Rid struct gpio_chip from container_of() usage
On Wed, Dec 09, 2015 at 03:46:00PM +0100, Linus Walleij wrote: > On Wed, Dec 9, 2015 at 2:44 PM, Russell King - ARM Linux > wrote: > > On Wed, Dec 09, 2015 at 02:08:35PM +0100, Linus Walleij wrote: > >> Because we want to have a proper userspace ABI for GPIO chips, > >> which involves using a character device that the user opens > >> and closes. While the character device is open, the underlying > >> kernel objects must not go away. > > > > Okay, so you stop the gpio_chip struct from going away. > > Actually I was going to create a new struct gpio_device, but yes. > > > What > > about the code which is called via gpio_chip - say, if userspace > > keeps the chardev open, and someone rmmod's the driver providing > > the GPIO driver. > > The idea was that when what is today gpiochip_remove() is called by the > gpiochip driver, the static data pointer to the vtable with all > callbacks is set to NULL (in some safe way), and the code avoids > calling these, so the device goes numb. > > I think you give me the right solution to this "safe way" below, > thanks! > > > I'm not sure that splitting up objects in this way really solves > > anything at all. Yes, it divorses the driver's private data from > > the subsystem data, but is that really an advantage? > > I like the design pattern you describe below, and > I have no strong opinion on it. If there is a precedent in the kernel > to do it with a separate alloc_foo() function I can do that. > > (Would like Greg's and/or Johan's opinion on this though.) I'd prefer separating allocation and registration rather than using a setup callback. > > Things get a little more complex with gpio, because there's the > > issue that some methods are spinlocked while others can take > > semaphores, but it should be possible to come up with a solution > > to that - maybe an atomic_t which is incremented whenever we're > > in some operation provided it's >= 0 (otherwise it fails), and > > decremented when the operation completes. We can then control > > in the unregistration path further GPIO accesses, and also > > prevent new accesses occuring by setting the atomic_t to -1. > > This shouldn't require any additional locking in any path. It > > does mean that the unregistration path needs careful thought to > > ensure that when we set it to -1, we wait for it to be dropped > > by the appropriate amount. > > Yeah we will both in spinlocks/hotpath and > semaphores/mutexes/slowpath in these ops for sure :/ > > The atomic hack should work: I understand that you mean > we read it and set it to -1 and if it was 2 wait for it to > become -3, then conclude unregistration with callbacks > numbed. Ok, but let's take a step back. So you have all this in place and a consumer calls gpiod_get_value() that returns an errno because the device is gone. Note that this wasn't even possible before e20538b82f1f ("gpio: Propagate errors from chip->get()") that went into *v4.3*, and I assume most drivers would need to be updated to even handle that that gpio call, and all future calls, are now suddenly failing. So this ties into the generic problem of inter-device dependencies. Does it even make sense to keep the consumer around, now that its gpio resources have gone away? If your current concern is a new gpio chardev interface, perhaps solving only that use case in the way that Dimitry suggested elsewhere in this thread is what you should be doing. > Then there is a particular case that occurs with USB or similar > pluggable buses, where there is a glitch or powercycle on the > bus, and the same device goes away and comes back in > a few milliseconds, and that means it should reattach to the > character device that is already open. No, that does not follow. A USB device being disconnected and reconnected is considered to be a new device. All state is gone, and the dangling character device will be unusable. > Making that work is however non-trivial :( Fortunately, you don't need to worry about that. Johan -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 000/182] Rid struct gpio_chip from container_of() usage
On Mon, Dec 14, 2015 at 1:18 AM, Linus Walleij wrote: > On Wed, Dec 9, 2015 at 8:30 PM, Dmitry Torokhov > wrote: >> On Wed, Dec 09, 2015 at 02:08:35PM +0100, Linus Walleij wrote: >>> This removes the use of container_of() constructions from *all* >>> GPIO drivers in the kernel. It is done by instead adding an >>> optional void *data pointer to the struct gpio_chip and an >>> accessor function, gpiochip_get_data() to get it from a driver. >>> >>> WHY? >>> >>> Because we want to have a proper userspace ABI for GPIO chips, >>> which involves using a character device that the user opens >>> and closes. While the character device is open, the underlying >>> kernel objects must not go away. >>> >>> Currently the GPIO drivers keep their state in the struct >>> gpio_chip, and that is often allocated by the drivers, very >>> often as a part of a containing per-instance state container >>> struct for the driver: >>> >>> struct foo_state { >>>struct gpio_chip chip; <- OMG my state is there >>> }; >>> >>> Drivers cannot allocate and manage this state: if a user has the >>> character device open, the objects allocated must stay around >>> even if the driver goes away. Instead drivers need to pass a >>> descriptor to the GPIO core, and then the core should allocate >>> and manage the lifecycle of things related to the device, such >>> as the chardev itself or the struct device related to the GPIO >>> device. >> >> Yes, but it does not mean that the object that is being maintained by >> the subsystem and that us attached to character device needs to be >> gpio_chip itself. You can have something like >> >> struct gpio_chip_chardev { >> struct cdev chardev; >> struct gpio_chip *chip; >> bool dead; >> }; > > There needs to be a struct device too, amongst other things. Could be; I was not trying to provide finished data structure but just illustrate possible solution. > >> >> struct gpio_chip { >> ... >> struct gpio_chip_chardev *chardev; >> ... >> }; >> >> You alloctae the new structure when you register/export gpio chip in >> gpio subsystem core and leave all the individual drivers alone. > > The current idea I have is something in the middle. Drivers have to > change a bit. The important part is that gpiolib handles allocation of > anything containing states. I'm thinking along the lines of Russell's > proposal to use netdev_alloc()'s design pattern. > > The problem is that currently gpio_chip contains a lot of > stateful variables (like the dynamically allocated array of GPIO descriptors > etc) and those are used by the gpiolib core, so they have to be moved away > from gpio_chip. > > So what happens if I don't change the design pattern: > > int ret = gpiochip_add(&my_chip); > ... > gpiochip_remove(&my_chip); > > At this point we have to cross-reference the pointer to my chip to > find the chip to remove. This goes for anything that takes the struct > gpio_chip * > as parameter, like gpiochip_add_pin_range(), gpiochip_request_own_desc() > etc etc. So something inside gpiolib must take a gpio_chip * pointer and > turn that into the actual state container, e.g, a struct gpio_device. > Since struct gpio_chip needs to be static and stateless, it cannot contain > a pointer back to its struct gpio_device. Why does gpio_chip have to be stateless? I am not saying that it should or should not, I just want to better understand what you are trying to achieve. > > That means basically comparing pointers across a list of gpio_device's > to find it. And that's ... very kludgy. But if people think it's better to > avoid > changing all drivers I will consider it. > > I think it is better if the GPIO drivers get a handle on the > real gpio_device * to be used when calling these gpiochip_* related > functions and also in the callbacks, which is a bigger refactoring > indeed. > > Part of this is trying to be elegant and mimic other subsystems and not > have GPIO be too hopelessly wayward about things. > > If I compare to how struct input_dev is done, you appear to also use the > pattern Russell suggested with input_dev_allocate() akin to > netdev_alloc(), and the allocated struct holds all the vtable and states etc, > and I think it is a good pattern, and that GPIO should conform. The main difference between gpio_chip (at least as it stands currently) and input devices and corresponding private driver data is that input device and driver data has different lifetimes; input devices may stick around even though driver is unbound from corresponding device and driver's private data freed. Thanks. -- Dmitry -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 000/182] Rid struct gpio_chip from container_of() usage
On Mon, Dec 14, 2015 at 1:46 PM, Johan Hovold wrote: > Ok, but let's take a step back. So you have all this in place and a > consumer calls gpiod_get_value() that returns an errno because the device > is gone. Note that this wasn't even possible before e20538b82f1f ("gpio: > Propagate errors from chip->get()") that went into *v4.3*, and I assume > most drivers would need to be updated to even handle that that gpio > call, and all future calls, are now suddenly failing. I actually have a revert of that in my fixes queue. So another step back and two forward for v4.5 (hopefully): audit all drivers to respect this. Yours, Linus Walleij -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 000/182] Rid struct gpio_chip from container_of() usage
On Tue, Dec 15, 2015 at 8:25 AM, Dmitry Torokhov wrote: > On Mon, Dec 14, 2015 at 1:18 AM, Linus Walleij > wrote: >> At this point we have to cross-reference the pointer to my chip to >> find the chip to remove. This goes for anything that takes the struct >> gpio_chip * >> as parameter, like gpiochip_add_pin_range(), gpiochip_request_own_desc() >> etc etc. So something inside gpiolib must take a gpio_chip * pointer and >> turn that into the actual state container, e.g, a struct gpio_device. >> Since struct gpio_chip needs to be static and stateless, it cannot contain >> a pointer back to its struct gpio_device. > > Why does gpio_chip have to be stateless? I am not saying that it > should or should not, I just want to better understand what you are > trying to achieve. Because the allocation of gpio_chip is currently inside each driver (often as part of their own state struct) and will go away with the driver. I want to make that const parameter that the drivers supply to the core gpiolib, and the gpiolib handled all states using something like that struct gpio_device you suggested or a more elaborate solution. >> If I compare to how struct input_dev is done, you appear to also use the >> pattern Russell suggested with input_dev_allocate() akin to >> netdev_alloc(), and the allocated struct holds all the vtable and states etc, >> and I think it is a good pattern, and that GPIO should conform. > > The main difference between gpio_chip (at least as it stands > currently) and input devices and corresponding private driver data is > that input device and driver data has different lifetimes; input > devices may stick around even though driver is unbound from > corresponding device and driver's private data freed. I would like to achieve something similar for GPIO devices. Yours, Linus Walleij -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html