| Message ID | 20230901183240.102701-1-brgl@bgdev.pl |
|---|---|
| State | Superseded |
| Headers | show |
| Series | gpio: sim: don't fiddle with GPIOLIB private members | expand |
On Fri, Sep 01, 2023 at 08:32:40PM +0200, Bartosz Golaszewski wrote: > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> > > We access internals of struct gpio_device and struct gpio_desc because > it's easier but it can actually be avoided and we're working towards a > better encapsulation of GPIO data structures across the kernel so let's > start at home. > > Instead of checking gpio_desc flags, let's just track the requests of > GPIOs in the driver. We also already store the information about > direction of simulated lines. > > For kobjects needed by sysfs callbacks: we can leverage the fact that > once created for a software node, struct device is accessible from that > fwnode_handle. We don't need to dereference gpio_device. > > While at it: fix one line break and remove the untrue part about > configfs callbacks using dev_get_drvdata() from a comment. ... > -static void gpio_sim_free(struct gpio_chip *gc, unsigned int offset) Why is this? > +static int gpio_sim_request(struct gpio_chip *gc, unsigned int offset) > { > struct gpio_sim_chip *chip = gpiochip_get_data(gc); > > scoped_guard(mutex, &chip->lock) > + __set_bit(offset, chip->request_map); > + > + return 0; > +} > + > +static void gpio_sim_free(struct gpio_chip *gc, unsigned int offset) > +{ > + struct gpio_sim_chip *chip = gpiochip_get_data(gc); > + > + scoped_guard(mutex, &chip->lock) { > __assign_bit(offset, chip->value_map, > !!test_bit(offset, chip->pull_map)); > + __clear_bit(offset, chip->request_map); > + } > } Seems to me like you. shuffled the order of the two functions. Can you leave _free() at the same location in the file? ... > - /* Used by sysfs and configfs callbacks. */ > - dev_set_drvdata(&gc->gpiodev->dev, chip); > + /* Used by sysfs callbacks. */ > + dev_set_drvdata(swnode->dev, chip); dev pointer of firmware node is solely for dev links. Is it the case here? Seems to me you luckily abuse it.
On Fri, Sep 1, 2023 at 11:10 PM Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > > On Fri, Sep 01, 2023 at 08:32:40PM +0200, Bartosz Golaszewski wrote: > > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> > > > > We access internals of struct gpio_device and struct gpio_desc because > > it's easier but it can actually be avoided and we're working towards a > > better encapsulation of GPIO data structures across the kernel so let's > > start at home. > > > > Instead of checking gpio_desc flags, let's just track the requests of > > GPIOs in the driver. We also already store the information about > > direction of simulated lines. > > > > For kobjects needed by sysfs callbacks: we can leverage the fact that > > once created for a software node, struct device is accessible from that > > fwnode_handle. We don't need to dereference gpio_device. > > > > While at it: fix one line break and remove the untrue part about > > configfs callbacks using dev_get_drvdata() from a comment. > > ... > > > -static void gpio_sim_free(struct gpio_chip *gc, unsigned int offset) > > Why is this? > Dunno, some git shenanigans? > > +static int gpio_sim_request(struct gpio_chip *gc, unsigned int offset) > > { > > struct gpio_sim_chip *chip = gpiochip_get_data(gc); > > > > scoped_guard(mutex, &chip->lock) > > + __set_bit(offset, chip->request_map); > > + > > + return 0; > > +} > > + > > +static void gpio_sim_free(struct gpio_chip *gc, unsigned int offset) > > +{ > > + struct gpio_sim_chip *chip = gpiochip_get_data(gc); > > + > > + scoped_guard(mutex, &chip->lock) { > > __assign_bit(offset, chip->value_map, > > !!test_bit(offset, chip->pull_map)); > > + __clear_bit(offset, chip->request_map); > > + } > > } > > Seems to me like you. shuffled the order of the two functions. > Can you leave _free() at the same location in the file? > I didn't. Request comes before free but they're next to each other. > ... > > > - /* Used by sysfs and configfs callbacks. */ > > - dev_set_drvdata(&gc->gpiodev->dev, chip); > > + /* Used by sysfs callbacks. */ > > + dev_set_drvdata(swnode->dev, chip); > > dev pointer of firmware node is solely for dev links. Is it the case here? > Seems to me you luckily abuse it. > I don't think so. If anything we have a helper in the form of get_dev_from_fwnode() but it takes reference to the device while we don't need it - we know it'll be there because we created it. This information (struct device of the GPIO device) can also be retrieved by iterating over the device children of the top platform device and comparing their fwnodes against the one we got passed down from probe() but it's just so many extra steps. Or we can have a getter in gpio/driver.h for that but I don't want to expose another interface is we can simply use the fwnode. Bart > -- > With Best Regards, > Andy Shevchenko > >
On Mon, Sep 4, 2023 at 10:59 AM Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > > On Sat, Sep 02, 2023 at 04:40:05PM +0200, Bartosz Golaszewski wrote: > > On Fri, Sep 1, 2023 at 11:10 PM Andy Shevchenko > > <andriy.shevchenko@linux.intel.com> wrote: > > > On Fri, Sep 01, 2023 at 08:32:40PM +0200, Bartosz Golaszewski wrote: > > ... > > > > > -static void gpio_sim_free(struct gpio_chip *gc, unsigned int offset) > > > > > > Why is this? > > > > Dunno, some git shenanigans? > > Time to use --patience then? > Ha! I wasn't even aware you can use this with format-patch. Looks much better indeed. > ... > > > > > - /* Used by sysfs and configfs callbacks. */ > > > > - dev_set_drvdata(&gc->gpiodev->dev, chip); > > > > + /* Used by sysfs callbacks. */ > > > > + dev_set_drvdata(swnode->dev, chip); > > > > > > dev pointer of firmware node is solely for dev links. Is it the case here? > > > Seems to me you luckily abuse it. > > > > I don't think so. If anything we have a helper in the form of > > get_dev_from_fwnode() but it takes reference to the device while we > > don't need it - we know it'll be there because we created it. > > > > This information (struct device of the GPIO device) can also be > > retrieved by iterating over the device children of the top platform > > device and comparing their fwnodes against the one we got passed down > > from probe() but it's just so many extra steps. > > > > Or we can have a getter in gpio/driver.h for that but I don't want to > > expose another interface is we can simply use the fwnode. > > dev pointer in the fwnode strictly speaking is optional. No-one, except > its solely user, should rely on it (its presence and lifetime). > Where is this documented? Because just by a quick glance into drivers/base/core.c I can tell that if a device has an fwnode then fwnode->dev gets assigned when the device is created and cleared when it's removed (note: note even attached to driver, just created/removed). Seems like pretty reliable behavior to me. Bart > -- > With Best Regards, > Andy Shevchenko > >
On Mon, Sep 04, 2023 at 11:22:32AM +0200, Bartosz Golaszewski wrote: > On Mon, Sep 4, 2023 at 10:59 AM Andy Shevchenko > <andriy.shevchenko@linux.intel.com> wrote: > > On Sat, Sep 02, 2023 at 04:40:05PM +0200, Bartosz Golaszewski wrote: > > > On Fri, Sep 1, 2023 at 11:10 PM Andy Shevchenko > > > <andriy.shevchenko@linux.intel.com> wrote: > > > > On Fri, Sep 01, 2023 at 08:32:40PM +0200, Bartosz Golaszewski wrote: ... > > > > > - /* Used by sysfs and configfs callbacks. */ > > > > > - dev_set_drvdata(&gc->gpiodev->dev, chip); > > > > > + /* Used by sysfs callbacks. */ > > > > > + dev_set_drvdata(swnode->dev, chip); > > > > > > > > dev pointer of firmware node is solely for dev links. Is it the case here? > > > > Seems to me you luckily abuse it. > > > > > > I don't think so. If anything we have a helper in the form of > > > get_dev_from_fwnode() but it takes reference to the device while we > > > don't need it - we know it'll be there because we created it. > > > > > > This information (struct device of the GPIO device) can also be > > > retrieved by iterating over the device children of the top platform > > > device and comparing their fwnodes against the one we got passed down > > > from probe() but it's just so many extra steps. > > > > > > Or we can have a getter in gpio/driver.h for that but I don't want to > > > expose another interface is we can simply use the fwnode. > > > > dev pointer in the fwnode strictly speaking is optional. No-one, except > > its solely user, should rely on it (its presence and lifetime). > > Where is this documented? Because just by a quick glance into > drivers/base/core.c I can tell that if a device has an fwnode then > fwnode->dev gets assigned when the device is created and cleared when > it's removed (note: note even attached to driver, just > created/removed). Seems like pretty reliable behavior to me. Yes, and even that member in fwnode is a hack in my opinion. We should not mix layers and the idea in the future to get rid of the fwnode_handle to be _embedded_ into struct device. It should be separate entity, and device instance may use it as a linked list. Currently we have a few problems because of the this design mistake. The get_dev_from_fwnode() is used only in devlink and I want to keep it that way. Nobody else should use it, really. We can discuss with Saravana, but I don't believe he can convince me otherwise.
On Mon, Sep 04, 2023 at 11:47:54AM +0200, Bartosz Golaszewski wrote: > On Mon, Sep 4, 2023 at 11:40 AM Andy Shevchenko > <andriy.shevchenko@linux.intel.com> wrote: > > On Mon, Sep 04, 2023 at 11:22:32AM +0200, Bartosz Golaszewski wrote: > > > On Mon, Sep 4, 2023 at 10:59 AM Andy Shevchenko > > > <andriy.shevchenko@linux.intel.com> wrote: > > > > On Sat, Sep 02, 2023 at 04:40:05PM +0200, Bartosz Golaszewski wrote: > > > > > On Fri, Sep 1, 2023 at 11:10 PM Andy Shevchenko > > > > > <andriy.shevchenko@linux.intel.com> wrote: > > > > > > On Fri, Sep 01, 2023 at 08:32:40PM +0200, Bartosz Golaszewski wrote: ... > > > > > > > - /* Used by sysfs and configfs callbacks. */ > > > > > > > - dev_set_drvdata(&gc->gpiodev->dev, chip); > > > > > > > + /* Used by sysfs callbacks. */ > > > > > > > + dev_set_drvdata(swnode->dev, chip); > > > > > > > > > > > > dev pointer of firmware node is solely for dev links. Is it the case here? > > > > > > Seems to me you luckily abuse it. > > > > > > > > > > I don't think so. If anything we have a helper in the form of > > > > > get_dev_from_fwnode() but it takes reference to the device while we > > > > > don't need it - we know it'll be there because we created it. > > > > > > > > > > This information (struct device of the GPIO device) can also be > > > > > retrieved by iterating over the device children of the top platform > > > > > device and comparing their fwnodes against the one we got passed down > > > > > from probe() but it's just so many extra steps. > > > > > > > > > > Or we can have a getter in gpio/driver.h for that but I don't want to > > > > > expose another interface is we can simply use the fwnode. > > > > > > > > dev pointer in the fwnode strictly speaking is optional. No-one, except > > > > its solely user, should rely on it (its presence and lifetime). > > > > > > Where is this documented? Because just by a quick glance into > > > drivers/base/core.c I can tell that if a device has an fwnode then > > > fwnode->dev gets assigned when the device is created and cleared when > > > it's removed (note: note even attached to driver, just > > > created/removed). Seems like pretty reliable behavior to me. > > > > Yes, and even that member in fwnode is a hack in my opinion. We should not mix > > layers and the idea in the future to get rid of the fwnode_handle to be > > _embedded_ into struct device. It should be separate entity, and device > > instance may use it as a linked list. Currently we have a few problems because > > of the this design mistake. > > I don't see how this would work if fwnodes can exist before struct > device is even created. That's whole idea behind swnodes. They (ideally) should be created _before_ any other object they are being used with. This is how it works today. And doing swnode->dev = ... contradicts a lot: layering, lifetime objects, etc. > They - after all - represent the actual > physical device hierarchy which may or may not be populated at > run-time depending on many factors. No. This is a mistaken assumption. > Once populated, being able to retrieve the software representation of > the device (struct device) from the node from which it was populated > sounds like a reasonable thing to do. What are those problems and are > they even linked to this issue? > > > The get_dev_from_fwnode() is used only in devlink and I want to keep it that way. > > Nobody else should use it, really. > > I don't care all that much, I can get the device from the children of > the platform device. Still comparing fwnodes, though this time the > other way around. Fine, but do not use dev pointer from fwnode, esp. software node. > > We can discuss with Saravana, but I don't believe he can convince me otherwise.
On Mon, Sep 04, 2023 at 12:12:44PM +0200, Bartosz Golaszewski wrote: > On Mon, Sep 4, 2023 at 12:05 PM Andy Shevchenko > <andriy.shevchenko@linux.intel.com> wrote: > > On Mon, Sep 04, 2023 at 11:47:54AM +0200, Bartosz Golaszewski wrote: > > > On Mon, Sep 4, 2023 at 11:40 AM Andy Shevchenko > > > <andriy.shevchenko@linux.intel.com> wrote: > > > > On Mon, Sep 04, 2023 at 11:22:32AM +0200, Bartosz Golaszewski wrote: > > > > > On Mon, Sep 4, 2023 at 10:59 AM Andy Shevchenko > > > > > <andriy.shevchenko@linux.intel.com> wrote: > > > > > > On Sat, Sep 02, 2023 at 04:40:05PM +0200, Bartosz Golaszewski wrote: > > > > > > > On Fri, Sep 1, 2023 at 11:10 PM Andy Shevchenko > > > > > > > <andriy.shevchenko@linux.intel.com> wrote: > > > > > > > > On Fri, Sep 01, 2023 at 08:32:40PM +0200, Bartosz Golaszewski wrote: ... > > > > > > > > > - /* Used by sysfs and configfs callbacks. */ > > > > > > > > > - dev_set_drvdata(&gc->gpiodev->dev, chip); > > > > > > > > > + /* Used by sysfs callbacks. */ > > > > > > > > > + dev_set_drvdata(swnode->dev, chip); > > > > > > > > > > > > > > > > dev pointer of firmware node is solely for dev links. Is it the case here? > > > > > > > > Seems to me you luckily abuse it. > > > > > > > > > > > > > > I don't think so. If anything we have a helper in the form of > > > > > > > get_dev_from_fwnode() but it takes reference to the device while we > > > > > > > don't need it - we know it'll be there because we created it. > > > > > > > > > > > > > > This information (struct device of the GPIO device) can also be > > > > > > > retrieved by iterating over the device children of the top platform > > > > > > > device and comparing their fwnodes against the one we got passed down > > > > > > > from probe() but it's just so many extra steps. > > > > > > > > > > > > > > Or we can have a getter in gpio/driver.h for that but I don't want to > > > > > > > expose another interface is we can simply use the fwnode. > > > > > > > > > > > > dev pointer in the fwnode strictly speaking is optional. No-one, except > > > > > > its solely user, should rely on it (its presence and lifetime). > > > > > > > > > > Where is this documented? Because just by a quick glance into > > > > > drivers/base/core.c I can tell that if a device has an fwnode then > > > > > fwnode->dev gets assigned when the device is created and cleared when > > > > > it's removed (note: note even attached to driver, just > > > > > created/removed). Seems like pretty reliable behavior to me. > > > > > > > > Yes, and even that member in fwnode is a hack in my opinion. We should not mix > > > > layers and the idea in the future to get rid of the fwnode_handle to be > > > > _embedded_ into struct device. It should be separate entity, and device > > > > instance may use it as a linked list. Currently we have a few problems because > > > > of the this design mistake. > > > > > > I don't see how this would work if fwnodes can exist before struct > > > device is even created. > > > > That's whole idea behind swnodes. They (ideally) should be created _before_ > > any other object they are being used with. This is how it works today. > > Yes, this is what I meant: if fwnodes can be created before struct > device (as it is now) and their life-time is separated then how could > you possibly make the fwnode part of struct device? > > > And doing swnode->dev = ... contradicts a lot: layering, lifetime objects, etc. > > No it doesn't. We have the software node - the template for the > device. It can only be populated with a single device entry. Which is wrong assumption. Software nodes (and firmware nodes) in general can be shared. Which device pointer you want to add there? Which one should be next when one of the devices is gone? No, simply no. Do not use it! > Once it's done, I don't see why you wouldn't want to assign this device to > its corresponding software node. Provided locking is in place etc. > > > > They - after all - represent the actual > > > physical device hierarchy which may or may not be populated at > > > run-time depending on many factors. > > > > No. This is a mistaken assumption. > > How so? See above. > > > Once populated, being able to retrieve the software representation of > > > the device (struct device) from the node from which it was populated > > > sounds like a reasonable thing to do. What are those problems and are > > > they even linked to this issue? > > > > > > > The get_dev_from_fwnode() is used only in devlink and I want to keep it that way. > > > > Nobody else should use it, really. > > > > > > I don't care all that much, I can get the device from the children of > > > the platform device. Still comparing fwnodes, though this time the > > > other way around. > > > > Fine, but do not use dev pointer from fwnode, esp. software node. > > I will do it but I'd like to clarify the above at some point. The relationship between device instance(s) and firmware node instance(s) is m:n, where each of them can be from 0 to ... x or y. There is no unique mapping between two. > > > > We can discuss with Saravana, but I don't believe he can convince me otherwise.
On Mon, Sep 4, 2023 at 3:29 AM Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > > On Mon, Sep 04, 2023 at 12:12:44PM +0200, Bartosz Golaszewski wrote: > > On Mon, Sep 4, 2023 at 12:05 PM Andy Shevchenko > > <andriy.shevchenko@linux.intel.com> wrote: > > > On Mon, Sep 04, 2023 at 11:47:54AM +0200, Bartosz Golaszewski wrote: > > > > On Mon, Sep 4, 2023 at 11:40 AM Andy Shevchenko > > > > <andriy.shevchenko@linux.intel.com> wrote: > > > > > On Mon, Sep 04, 2023 at 11:22:32AM +0200, Bartosz Golaszewski wrote: > > > > > > On Mon, Sep 4, 2023 at 10:59 AM Andy Shevchenko > > > > > > <andriy.shevchenko@linux.intel.com> wrote: > > > > > > > On Sat, Sep 02, 2023 at 04:40:05PM +0200, Bartosz Golaszewski wrote: > > > > > > > > On Fri, Sep 1, 2023 at 11:10 PM Andy Shevchenko > > > > > > > > <andriy.shevchenko@linux.intel.com> wrote: > > > > > > > > > On Fri, Sep 01, 2023 at 08:32:40PM +0200, Bartosz Golaszewski wrote: > > ... > > > > > > > > > > > - /* Used by sysfs and configfs callbacks. */ > > > > > > > > > > - dev_set_drvdata(&gc->gpiodev->dev, chip); > > > > > > > > > > + /* Used by sysfs callbacks. */ > > > > > > > > > > + dev_set_drvdata(swnode->dev, chip); > > > > > > > > > > > > > > > > > > dev pointer of firmware node is solely for dev links. Is it the case here? > > > > > > > > > Seems to me you luckily abuse it. > > > > > > > > > > > > > > > > I don't think so. If anything we have a helper in the form of > > > > > > > > get_dev_from_fwnode() but it takes reference to the device while we > > > > > > > > don't need it - we know it'll be there because we created it. > > > > > > > > > > > > > > > > This information (struct device of the GPIO device) can also be > > > > > > > > retrieved by iterating over the device children of the top platform > > > > > > > > device and comparing their fwnodes against the one we got passed down > > > > > > > > from probe() but it's just so many extra steps. > > > > > > > > > > > > > > > > Or we can have a getter in gpio/driver.h for that but I don't want to > > > > > > > > expose another interface is we can simply use the fwnode. > > > > > > > Sorry for being late to the party. > > > > > > > dev pointer in the fwnode strictly speaking is optional. No-one, except > > > > > > > its solely user, should rely on it (its presence and lifetime). > > > > > > > > > > > > Where is this documented? Because just by a quick glance into > > > > > > drivers/base/core.c I can tell that if a device has an fwnode then > > > > > > fwnode->dev gets assigned when the device is created and cleared when > > > > > > it's removed (note: note even attached to driver, just > > > > > > created/removed). Seems like pretty reliable behavior to me. > > > > > > > > > > Yes, and even that member in fwnode is a hack in my opinion. We should not mix > > > > > layers and the idea in the future to get rid of the fwnode_handle to be > > > > > _embedded_ into struct device. It should be separate entity, and device > > > > > instance may use it as a linked list. Currently we have a few problems because > > > > > of the this design mistake. > > > > > > > > I don't see how this would work if fwnodes can exist before struct > > > > device is even created. > > > > > > That's whole idea behind swnodes. They (ideally) should be created _before_ > > > any other object they are being used with. This is how it works today. > > > > Yes, this is what I meant: if fwnodes can be created before struct > > device (as it is now) and their life-time is separated then how could > > you possibly make the fwnode part of struct device? > > > > > And doing swnode->dev = ... contradicts a lot: layering, lifetime objects, etc. I understand what you are trying to say about layering, but there are no lifetime violations here. > > > > No it doesn't. We have the software node - the template for the > > device. It can only be populated with a single device entry. > > Which is wrong assumption. Software nodes (and firmware nodes) in general > can be shared. Which device pointer you want to add there? I don't think this is any harder to handle than how a device's secondary fwnode is handled in set_primary_fwnode(). For secondary fwnodes, you just WARN and overwrite it and move on. > Which one > should be next when one of the devices is gone? Similar to how set_primary_fwnode() handles deletion (NULL), you can handle the same for when a device is removed. You can check the parent or the bus for another device with the same fwnode and set it. > No, simply no. Do not use it! Using fwnode_handle->dev is no different than searching a bus for a device which has dev->fwnode match the fwnode you are looking for. In both cases, you are just going to get the first device that was added. It's completely pointless to force searching a bus to find the device with a specific fwnode. In the special cases where one fwnode has multiple devices, no generic code is going to always handle the device search correctly. The framework adding those devices probably knows what's the right thing to do based on which of the N devices with the same fwnode they are trying to find. I understand it's not great, but blindly saying "search the bus" isn't really improving anything here and just makes things unnecessarily inefficient. -Saravana > > > Once it's done, I don't see why you wouldn't want to assign this device to > > its corresponding software node. Provided locking is in place etc. > > > > > > They - after all - represent the actual > > > > physical device hierarchy which may or may not be populated at > > > > run-time depending on many factors. > > > > > > No. This is a mistaken assumption. > > > > How so? > > See above. > > > > > Once populated, being able to retrieve the software representation of > > > > the device (struct device) from the node from which it was populated > > > > sounds like a reasonable thing to do. What are those problems and are > > > > they even linked to this issue? > > > > > > > > > The get_dev_from_fwnode() is used only in devlink and I want to keep it that way. > > > > > Nobody else should use it, really. > > > > > > > > I don't care all that much, I can get the device from the children of > > > > the platform device. Still comparing fwnodes, though this time the > > > > other way around. > > > > > > Fine, but do not use dev pointer from fwnode, esp. software node. > > > > I will do it but I'd like to clarify the above at some point. > > The relationship between device instance(s) and firmware node instance(s) > is m:n, where each of them can be from 0 to ... x or y. > > There is no unique mapping between two.
On Wed, Feb 21, 2024 at 2:47 AM Saravana Kannan <saravanak@google.com> wrote: > > On Mon, Sep 4, 2023 at 3:29 AM Andy Shevchenko > <andriy.shevchenko@linux.intel.com> wrote: > > > > On Mon, Sep 04, 2023 at 12:12:44PM +0200, Bartosz Golaszewski wrote: > > > On Mon, Sep 4, 2023 at 12:05 PM Andy Shevchenko > > > <andriy.shevchenko@linux.intel.com> wrote: > > > > On Mon, Sep 04, 2023 at 11:47:54AM +0200, Bartosz Golaszewski wrote: > > > > > On Mon, Sep 4, 2023 at 11:40 AM Andy Shevchenko > > > > > <andriy.shevchenko@linux.intel.com> wrote: > > > > > > On Mon, Sep 04, 2023 at 11:22:32AM +0200, Bartosz Golaszewski wrote: > > > > > > > On Mon, Sep 4, 2023 at 10:59 AM Andy Shevchenko > > > > > > > <andriy.shevchenko@linux.intel.com> wrote: > > > > > > > > On Sat, Sep 02, 2023 at 04:40:05PM +0200, Bartosz Golaszewski wrote: > > > > > > > > > On Fri, Sep 1, 2023 at 11:10 PM Andy Shevchenko > > > > > > > > > <andriy.shevchenko@linux.intel.com> wrote: > > > > > > > > > > On Fri, Sep 01, 2023 at 08:32:40PM +0200, Bartosz Golaszewski wrote: > > > > ... > > > > > > > > > > > > > - /* Used by sysfs and configfs callbacks. */ > > > > > > > > > > > - dev_set_drvdata(&gc->gpiodev->dev, chip); > > > > > > > > > > > + /* Used by sysfs callbacks. */ > > > > > > > > > > > + dev_set_drvdata(swnode->dev, chip); > > > > > > > > > > > > > > > > > > > > dev pointer of firmware node is solely for dev links. Is it the case here? > > > > > > > > > > Seems to me you luckily abuse it. > > > > > > > > > > > > > > > > > > I don't think so. If anything we have a helper in the form of > > > > > > > > > get_dev_from_fwnode() but it takes reference to the device while we > > > > > > > > > don't need it - we know it'll be there because we created it. > > > > > > > > > > > > > > > > > > This information (struct device of the GPIO device) can also be > > > > > > > > > retrieved by iterating over the device children of the top platform > > > > > > > > > device and comparing their fwnodes against the one we got passed down > > > > > > > > > from probe() but it's just so many extra steps. > > > > > > > > > > > > > > > > > > Or we can have a getter in gpio/driver.h for that but I don't want to > > > > > > > > > expose another interface is we can simply use the fwnode. > > > > > > > > > > Sorry for being late to the party. > > > > > > > > > dev pointer in the fwnode strictly speaking is optional. No-one, except > > > > > > > > its solely user, should rely on it (its presence and lifetime). > > > > > > > > > > > > > > Where is this documented? Because just by a quick glance into > > > > > > > drivers/base/core.c I can tell that if a device has an fwnode then > > > > > > > fwnode->dev gets assigned when the device is created and cleared when > > > > > > > it's removed (note: note even attached to driver, just > > > > > > > created/removed). Seems like pretty reliable behavior to me. > > > > > > > > > > > > Yes, and even that member in fwnode is a hack in my opinion. We should not mix > > > > > > layers and the idea in the future to get rid of the fwnode_handle to be > > > > > > _embedded_ into struct device. It should be separate entity, and device > > > > > > instance may use it as a linked list. Currently we have a few problems because > > > > > > of the this design mistake. > > > > > > > > > > I don't see how this would work if fwnodes can exist before struct > > > > > device is even created. > > > > > > > > That's whole idea behind swnodes. They (ideally) should be created _before_ > > > > any other object they are being used with. This is how it works today. > > > > > > Yes, this is what I meant: if fwnodes can be created before struct > > > device (as it is now) and their life-time is separated then how could > > > you possibly make the fwnode part of struct device? > > > > > > > And doing swnode->dev = ... contradicts a lot: layering, lifetime objects, etc. > > I understand what you are trying to say about layering, but there are > no lifetime violations here. > > > > > > > No it doesn't. We have the software node - the template for the > > > device. It can only be populated with a single device entry. > > > > Which is wrong assumption. Software nodes (and firmware nodes) in general > > can be shared. Which device pointer you want to add there? > > I don't think this is any harder to handle than how a device's > secondary fwnode is handled in set_primary_fwnode(). For secondary > fwnodes, you just WARN and overwrite it and move on. > > > Which one > > should be next when one of the devices is gone? > > Similar to how set_primary_fwnode() handles deletion (NULL), you can > handle the same for when a device is removed. You can check the parent > or the bus for another device with the same fwnode and set it. > > > No, simply no. Do not use it! > > Using fwnode_handle->dev is no different than searching a bus for a > device which has dev->fwnode match the fwnode you are looking for. > > In both cases, you are just going to get the first device that was > added. It's completely pointless to force searching a bus to find the > device with a specific fwnode. > > In the special cases where one fwnode has multiple devices, no generic > code is going to always handle the device search correctly. The > framework adding those devices probably knows what's the right thing > to do based on which of the N devices with the same fwnode they are > trying to find. > > I understand it's not great, but blindly saying "search the bus" isn't > really improving anything here and just makes things unnecessarily > inefficient. > > -Saravana Thanks for the input. I've since moved to using device_find_child() but will keep it in mind for the future. Bart > > > > > > Once it's done, I don't see why you wouldn't want to assign this device to > > > its corresponding software node. Provided locking is in place etc. > > > > > > > > They - after all - represent the actual > > > > > physical device hierarchy which may or may not be populated at > > > > > run-time depending on many factors. > > > > > > > > No. This is a mistaken assumption. > > > > > > How so? > > > > See above. > > > > > > > Once populated, being able to retrieve the software representation of > > > > > the device (struct device) from the node from which it was populated > > > > > sounds like a reasonable thing to do. What are those problems and are > > > > > they even linked to this issue? > > > > > > > > > > > The get_dev_from_fwnode() is used only in devlink and I want to keep it that way. > > > > > > Nobody else should use it, really. > > > > > > > > > > I don't care all that much, I can get the device from the children of > > > > > the platform device. Still comparing fwnodes, though this time the > > > > > other way around. > > > > > > > > Fine, but do not use dev pointer from fwnode, esp. software node. > > > > > > I will do it but I'd like to clarify the above at some point. > > > > The relationship between device instance(s) and firmware node instance(s) > > is m:n, where each of them can be from 0 to ... x or y. > > > > There is no unique mapping between two.
On Tue, Feb 20, 2024 at 05:46:27PM -0800, Saravana Kannan wrote: > On Mon, Sep 4, 2023 at 3:29 AM Andy Shevchenko > <andriy.shevchenko@linux.intel.com> wrote: > > On Mon, Sep 04, 2023 at 12:12:44PM +0200, Bartosz Golaszewski wrote: > > > On Mon, Sep 4, 2023 at 12:05 PM Andy Shevchenko > > > <andriy.shevchenko@linux.intel.com> wrote: > > > > On Mon, Sep 04, 2023 at 11:47:54AM +0200, Bartosz Golaszewski wrote: > > > > > On Mon, Sep 4, 2023 at 11:40 AM Andy Shevchenko > > > > > <andriy.shevchenko@linux.intel.com> wrote: > > > > > > On Mon, Sep 04, 2023 at 11:22:32AM +0200, Bartosz Golaszewski wrote: > > > > > > > On Mon, Sep 4, 2023 at 10:59 AM Andy Shevchenko > > > > > > > <andriy.shevchenko@linux.intel.com> wrote: > > > > > > > > On Sat, Sep 02, 2023 at 04:40:05PM +0200, Bartosz Golaszewski wrote: > > > > > > > > > On Fri, Sep 1, 2023 at 11:10 PM Andy Shevchenko > > > > > > > > > <andriy.shevchenko@linux.intel.com> wrote: > > > > > > > > > > On Fri, Sep 01, 2023 at 08:32:40PM +0200, Bartosz Golaszewski wrote: ... > > > > > > > > > > > - /* Used by sysfs and configfs callbacks. */ > > > > > > > > > > > - dev_set_drvdata(&gc->gpiodev->dev, chip); > > > > > > > > > > > + /* Used by sysfs callbacks. */ > > > > > > > > > > > + dev_set_drvdata(swnode->dev, chip); > > > > > > > > > > > > > > > > > > > > dev pointer of firmware node is solely for dev links. Is it the case here? > > > > > > > > > > Seems to me you luckily abuse it. > > > > > > > > > > > > > > > > > > I don't think so. If anything we have a helper in the form of > > > > > > > > > get_dev_from_fwnode() but it takes reference to the device while we > > > > > > > > > don't need it - we know it'll be there because we created it. > > > > > > > > > > > > > > > > > > This information (struct device of the GPIO device) can also be > > > > > > > > > retrieved by iterating over the device children of the top platform > > > > > > > > > device and comparing their fwnodes against the one we got passed down > > > > > > > > > from probe() but it's just so many extra steps. > > > > > > > > > > > > > > > > > > Or we can have a getter in gpio/driver.h for that but I don't want to > > > > > > > > > expose another interface is we can simply use the fwnode. > > > > > > > > > > Sorry for being late to the party. You decided to make a blast from the past due to the last patches from me? :-) > > > > > > > > dev pointer in the fwnode strictly speaking is optional. No-one, except > > > > > > > > its solely user, should rely on it (its presence and lifetime). > > > > > > > > > > > > > > Where is this documented? Because just by a quick glance into > > > > > > > drivers/base/core.c I can tell that if a device has an fwnode then > > > > > > > fwnode->dev gets assigned when the device is created and cleared when > > > > > > > it's removed (note: note even attached to driver, just > > > > > > > created/removed). Seems like pretty reliable behavior to me. > > > > > > > > > > > > Yes, and even that member in fwnode is a hack in my opinion. We should not mix > > > > > > layers and the idea in the future to get rid of the fwnode_handle to be > > > > > > _embedded_ into struct device. It should be separate entity, and device > > > > > > instance may use it as a linked list. Currently we have a few problems because > > > > > > of the this design mistake. > > > > > > > > > > I don't see how this would work if fwnodes can exist before struct > > > > > device is even created. > > > > > > > > That's whole idea behind swnodes. They (ideally) should be created _before_ > > > > any other object they are being used with. This is how it works today. > > > > > > Yes, this is what I meant: if fwnodes can be created before struct > > > device (as it is now) and their life-time is separated then how could > > > you possibly make the fwnode part of struct device? > > > > > > > And doing swnode->dev = ... contradicts a lot: layering, lifetime objects, etc. > > I understand what you are trying to say about layering, but there are > no lifetime violations here. There is. Software node is not firmware node, their lifetime is the same or wider than the respective device (often, they are statically defined without any device in mind). > > > No it doesn't. We have the software node - the template for the > > > device. It can only be populated with a single device entry. > > > > Which is wrong assumption. Software nodes (and firmware nodes) in general > > can be shared. Which device pointer you want to add there? > > I don't think this is any harder to handle than how a device's > secondary fwnode is handled in set_primary_fwnode(). For secondary > fwnodes, you just WARN and overwrite it and move on. The whole concept of a single linked list with limitation to up to two nodes and being the part of the struct fwnode_handle itself appears to be problematic. We have a lot of tricks here and there instead of properly having a list head in the struct device without any limitations in number of nodes with a priority based on the appearance in the list. For the details you may ask USB DWC3 developers and related to that. > > Which one should be next when one of the devices is gone? > > Similar to how set_primary_fwnode() handles deletion (NULL), you can > handle the same for when a device is removed. You can check the parent > or the bus for another device with the same fwnode and set it. > > No, simply no. Do not use it! > > Using fwnode_handle->dev is no different than searching a bus for a > device which has dev->fwnode match the fwnode you are looking for. > > In both cases, you are just going to get the first device that was > added. It's completely pointless to force searching a bus to find the > device with a specific fwnode. > > In the special cases where one fwnode has multiple devices, no generic > code is going to always handle the device search correctly. The > framework adding those devices probably knows what's the right thing > to do based on which of the N devices with the same fwnode they are > trying to find. > > I understand it's not great, but blindly saying "search the bus" isn't > really improving anything here and just makes things unnecessarily > inefficient. Is there any _good_ documentation for devlinks and all that fields in the struct fwnode? Why should we use that without any understanding of the purposes of that field. We, as device property developers, hadn't introduced that field and never required it. It's an alien to device properties APIs.
On Wed, Feb 21, 2024 at 4:59 AM Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > > On Tue, Feb 20, 2024 at 05:46:27PM -0800, Saravana Kannan wrote: > > On Mon, Sep 4, 2023 at 3:29 AM Andy Shevchenko > > <andriy.shevchenko@linux.intel.com> wrote: > > > On Mon, Sep 04, 2023 at 12:12:44PM +0200, Bartosz Golaszewski wrote: > > > > On Mon, Sep 4, 2023 at 12:05 PM Andy Shevchenko > > > > <andriy.shevchenko@linux.intel.com> wrote: > > > > > On Mon, Sep 04, 2023 at 11:47:54AM +0200, Bartosz Golaszewski wrote: > > > > > > On Mon, Sep 4, 2023 at 11:40 AM Andy Shevchenko > > > > > > <andriy.shevchenko@linux.intel.com> wrote: > > > > > > > On Mon, Sep 04, 2023 at 11:22:32AM +0200, Bartosz Golaszewski wrote: > > > > > > > > On Mon, Sep 4, 2023 at 10:59 AM Andy Shevchenko > > > > > > > > <andriy.shevchenko@linux.intel.com> wrote: > > > > > > > > > On Sat, Sep 02, 2023 at 04:40:05PM +0200, Bartosz Golaszewski wrote: > > > > > > > > > > On Fri, Sep 1, 2023 at 11:10 PM Andy Shevchenko > > > > > > > > > > <andriy.shevchenko@linux.intel.com> wrote: > > > > > > > > > > > On Fri, Sep 01, 2023 at 08:32:40PM +0200, Bartosz Golaszewski wrote: > > ... > > > > > > > > > > > > > - /* Used by sysfs and configfs callbacks. */ > > > > > > > > > > > > - dev_set_drvdata(&gc->gpiodev->dev, chip); > > > > > > > > > > > > + /* Used by sysfs callbacks. */ > > > > > > > > > > > > + dev_set_drvdata(swnode->dev, chip); > > > > > > > > > > > > > > > > > > > > > > dev pointer of firmware node is solely for dev links. Is it the case here? > > > > > > > > > > > Seems to me you luckily abuse it. > > > > > > > > > > > > > > > > > > > > I don't think so. If anything we have a helper in the form of > > > > > > > > > > get_dev_from_fwnode() but it takes reference to the device while we > > > > > > > > > > don't need it - we know it'll be there because we created it. > > > > > > > > > > > > > > > > > > > > This information (struct device of the GPIO device) can also be > > > > > > > > > > retrieved by iterating over the device children of the top platform > > > > > > > > > > device and comparing their fwnodes against the one we got passed down > > > > > > > > > > from probe() but it's just so many extra steps. > > > > > > > > > > > > > > > > > > > > Or we can have a getter in gpio/driver.h for that but I don't want to > > > > > > > > > > expose another interface is we can simply use the fwnode. > > > > > > > > > > > > > Sorry for being late to the party. > > You decided to make a blast from the past due to the last patches from me? :-) Yeah :) I meant to reply to this when you sent it, but was swamped and forgot about it. > > > > > > > > > > dev pointer in the fwnode strictly speaking is optional. No-one, except > > > > > > > > > its solely user, should rely on it (its presence and lifetime). > > > > > > > > > > > > > > > > Where is this documented? Because just by a quick glance into > > > > > > > > drivers/base/core.c I can tell that if a device has an fwnode then > > > > > > > > fwnode->dev gets assigned when the device is created and cleared when > > > > > > > > it's removed (note: note even attached to driver, just > > > > > > > > created/removed). Seems like pretty reliable behavior to me. > > > > > > > > > > > > > > Yes, and even that member in fwnode is a hack in my opinion. We should not mix > > > > > > > layers and the idea in the future to get rid of the fwnode_handle to be > > > > > > > _embedded_ into struct device. It should be separate entity, and device > > > > > > > instance may use it as a linked list. Currently we have a few problems because > > > > > > > of the this design mistake. > > > > > > > > > > > > I don't see how this would work if fwnodes can exist before struct > > > > > > device is even created. > > > > > > > > > > That's whole idea behind swnodes. They (ideally) should be created _before_ > > > > > any other object they are being used with. This is how it works today. > > > > > > > > Yes, this is what I meant: if fwnodes can be created before struct > > > > device (as it is now) and their life-time is separated then how could > > > > you possibly make the fwnode part of struct device? > > > > > > > > > And doing swnode->dev = ... contradicts a lot: layering, lifetime objects, etc. > > > > I understand what you are trying to say about layering, but there are > > no lifetime violations here. > > There is. Software node is not firmware node, their lifetime is the same or > wider than the respective device (often, they are statically defined without > any device in mind). > > > > > No it doesn't. We have the software node - the template for the > > > > device. It can only be populated with a single device entry. > > > > > > Which is wrong assumption. Software nodes (and firmware nodes) in general > > > can be shared. Which device pointer you want to add there? > > > > I don't think this is any harder to handle than how a device's > > secondary fwnode is handled in set_primary_fwnode(). For secondary > > fwnodes, you just WARN and overwrite it and move on. > > The whole concept of a single linked list with limitation to up to two > nodes and being the part of the struct fwnode_handle itself appears to > be problematic. We have a lot of tricks here and there instead of properly > having a list head in the struct device without any limitations in number > of nodes with a priority based on the appearance in the list. > > For the details you may ask USB DWC3 developers and related to that. > > > > Which one should be next when one of the devices is gone? > > > > Similar to how set_primary_fwnode() handles deletion (NULL), you can > > handle the same for when a device is removed. You can check the parent > > or the bus for another device with the same fwnode and set it. > > > > No, simply no. Do not use it! > > > > Using fwnode_handle->dev is no different than searching a bus for a > > device which has dev->fwnode match the fwnode you are looking for. > > > > In both cases, you are just going to get the first device that was > > added. It's completely pointless to force searching a bus to find the > > device with a specific fwnode. > > > > In the special cases where one fwnode has multiple devices, no generic > > code is going to always handle the device search correctly. The > > framework adding those devices probably knows what's the right thing > > to do based on which of the N devices with the same fwnode they are > > trying to find. > > > > I understand it's not great, but blindly saying "search the bus" isn't > > really improving anything here and just makes things unnecessarily > > inefficient. > > Is there any _good_ documentation for devlinks and all that fields in the > struct fwnode? Why should we use that without any understanding of the > purposes of that field. We, as device property developers, hadn't introduced > that field and never required it. It's an alien to device properties APIs. If I add some inline documentation for these fields, will you be more open to letting people use this as a way to look up devices? I'm happy to do that for you. Thanks, Saravana
On Thu, Feb 22, 2024 at 05:01:04PM -0800, Saravana Kannan wrote: > On Wed, Feb 21, 2024 at 4:59 AM Andy Shevchenko > <andriy.shevchenko@linux.intel.com> wrote: > > On Tue, Feb 20, 2024 at 05:46:27PM -0800, Saravana Kannan wrote: ... > > Is there any _good_ documentation for devlinks and all that fields in the > > struct fwnode? Why should we use that without any understanding of the > > purposes of that field. We, as device property developers, hadn't introduced > > that field and never required it. It's an alien to device properties APIs. > > If I add some inline documentation for these fields, will you be more > open to letting people use this as a way to look up devices? I'm happy > to do that for you. I consider documentation patches to be always welcome. But it doesn't mean we allow to use that fields in device property APIs without a very good justification.
diff --git a/drivers/gpio/gpio-sim.c b/drivers/gpio/gpio-sim.c index 271db3639a78..5f52d77567a1 100644 --- a/drivers/gpio/gpio-sim.c +++ b/drivers/gpio/gpio-sim.c @@ -12,6 +12,7 @@ #include <linux/completion.h> #include <linux/configfs.h> #include <linux/device.h> +#include <linux/gpio/consumer.h> #include <linux/gpio/driver.h> #include <linux/gpio/machine.h> #include <linux/idr.h> @@ -30,8 +31,6 @@ #include <linux/string_helpers.h> #include <linux/sysfs.h> -#include "gpiolib.h" - #define GPIO_SIM_NGPIO_MAX 1024 #define GPIO_SIM_PROP_MAX 4 /* Max 3 properties + sentinel. */ #define GPIO_SIM_NUM_ATTRS 3 /* value, pull and sentinel */ @@ -40,6 +39,8 @@ static DEFINE_IDA(gpio_sim_ida); struct gpio_sim_chip { struct gpio_chip gc; + struct fwnode_handle *swnode; + unsigned long *request_map; unsigned long *direction_map; unsigned long *value_map; unsigned long *pull_map; @@ -63,16 +64,11 @@ static int gpio_sim_apply_pull(struct gpio_sim_chip *chip, unsigned int offset, int value) { int irq, irq_type, ret; - struct gpio_desc *desc; - struct gpio_chip *gc; - - gc = &chip->gc; - desc = &gc->gpiodev->descs[offset]; guard(mutex)(&chip->lock); - if (test_bit(FLAG_REQUESTED, &desc->flags) && - !test_bit(FLAG_IS_OUT, &desc->flags)) { + if (test_bit(offset, chip->request_map) && + test_bit(offset, chip->direction_map)) { if (value == !!test_bit(offset, chip->value_map)) goto set_pull; @@ -99,8 +95,8 @@ static int gpio_sim_apply_pull(struct gpio_sim_chip *chip, set_value: /* Change the value unless we're actively driving the line. */ - if (!test_bit(FLAG_REQUESTED, &desc->flags) || - !test_bit(FLAG_IS_OUT, &desc->flags)) + if (!test_bit(offset, chip->request_map) || + test_bit(offset, chip->direction_map)) __assign_bit(offset, chip->value_map, value); set_pull: @@ -181,7 +177,7 @@ static int gpio_sim_get_direction(struct gpio_chip *gc, unsigned int offset) } static int gpio_sim_set_config(struct gpio_chip *gc, - unsigned int offset, unsigned long config) + unsigned int offset, unsigned long config) { struct gpio_sim_chip *chip = gpiochip_get_data(gc); @@ -204,13 +200,25 @@ static int gpio_sim_to_irq(struct gpio_chip *gc, unsigned int offset) return irq_create_mapping(chip->irq_sim, offset); } -static void gpio_sim_free(struct gpio_chip *gc, unsigned int offset) +static int gpio_sim_request(struct gpio_chip *gc, unsigned int offset) { struct gpio_sim_chip *chip = gpiochip_get_data(gc); scoped_guard(mutex, &chip->lock) + __set_bit(offset, chip->request_map); + + return 0; +} + +static void gpio_sim_free(struct gpio_chip *gc, unsigned int offset) +{ + struct gpio_sim_chip *chip = gpiochip_get_data(gc); + + scoped_guard(mutex, &chip->lock) { __assign_bit(offset, chip->value_map, !!test_bit(offset, chip->pull_map)); + __clear_bit(offset, chip->request_map); + } } static ssize_t gpio_sim_sysfs_val_show(struct device *dev, @@ -295,7 +303,7 @@ static void gpio_sim_sysfs_remove(void *data) { struct gpio_sim_chip *chip = data; - sysfs_remove_groups(&chip->gc.gpiodev->dev.kobj, chip->attr_groups); + sysfs_remove_groups(&chip->swnode->dev->kobj, chip->attr_groups); } static int gpio_sim_setup_sysfs(struct gpio_sim_chip *chip) @@ -352,7 +360,7 @@ static int gpio_sim_setup_sysfs(struct gpio_sim_chip *chip) chip->attr_groups[i] = attr_group; } - ret = sysfs_create_groups(&chip->gc.gpiodev->dev.kobj, + ret = sysfs_create_groups(&chip->swnode->dev->kobj, chip->attr_groups); if (ret) return ret; @@ -387,6 +395,12 @@ static int gpio_sim_add_bank(struct fwnode_handle *swnode, struct device *dev) if (!chip) return -ENOMEM; + chip->swnode = swnode; + + chip->request_map = devm_bitmap_zalloc(dev, num_lines, GFP_KERNEL); + if (!chip->request_map) + return -ENOMEM; + chip->direction_map = devm_bitmap_alloc(dev, num_lines, GFP_KERNEL); if (!chip->direction_map) return -ENOMEM; @@ -432,6 +446,7 @@ static int gpio_sim_add_bank(struct fwnode_handle *swnode, struct device *dev) gc->get_direction = gpio_sim_get_direction; gc->set_config = gpio_sim_set_config; gc->to_irq = gpio_sim_to_irq; + gc->request = gpio_sim_request; gc->free = gpio_sim_free; gc->can_sleep = true; @@ -439,8 +454,8 @@ static int gpio_sim_add_bank(struct fwnode_handle *swnode, struct device *dev) if (ret) return ret; - /* Used by sysfs and configfs callbacks. */ - dev_set_drvdata(&gc->gpiodev->dev, chip); + /* Used by sysfs callbacks. */ + dev_set_drvdata(swnode->dev, chip); return gpio_sim_setup_sysfs(chip); }