| Message ID | 20211203133003.31786-1-brgl@bgdev.pl |
|---|---|
| Headers | show |
| Series | gpio-sim: configfs-based GPIO simulator | expand |
On Fri, Dec 03, 2021 at 08:51:56PM +0200, Andy Shevchenko wrote: > On Fri, Dec 03, 2021 at 02:29:59PM +0100, Bartosz Golaszewski wrote: ... > > if (gc->parent) > > gdev->dev.of_node = gc->parent->of_node; > > > > + if (gc->fwnode) > > + gc->of_node = to_of_node(gc->fwnode); > > + > > /* If the gpiochip has an assigned OF node this takes precedence */ > > if (gc->of_node) > > gdev->dev.of_node = gc->of_node; > > Similar should be done in acpi_gpio_dev_init(): > > if (gc->fwnode) > device_set_node(&gdev->dev, gc->fwnode); Hmm... On the second though this should be rather if (gc->fwnode) set_secondary_fwnode(&gdev->dev, gc->fwnode); So the logic will be that: - if we have parent, set primary fwnode to it - if we have fwnode, set secondary one to it - otherwise do nothing
On Fri, Dec 03, 2021 at 02:29:56PM +0100, Bartosz Golaszewski wrote: > Another iteration of gpio-sim patches. This time the changes are quite > small. I removed the ifdefs from gpiolib.c as requested by Andy. In this > version gpiolib-of will also prefer fwnodes over of_nodes and - if set - > will convert them to of_nodes before proceeding. > > Tested both with configfs as well as device-tree. After addressing a comment in patch 3: Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> for patches 1-3. Patch 4 has got full review but nothing specifically critical there.
On Fri, Dec 3, 2021 at 9:10 PM Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > > On Fri, Dec 03, 2021 at 08:28:34PM +0100, Bartosz Golaszewski wrote: > > On Fri, Dec 3, 2021 at 8:04 PM Andy Shevchenko > > <andriy.shevchenko@linux.intel.com> wrote: > > > On Fri, Dec 03, 2021 at 08:56:27PM +0200, Andy Shevchenko wrote: > > > > On Fri, Dec 03, 2021 at 08:51:56PM +0200, Andy Shevchenko wrote: > > > > > On Fri, Dec 03, 2021 at 02:29:59PM +0100, Bartosz Golaszewski wrote: > > ... > > > > > > > if (gc->parent) > > > > > > gdev->dev.of_node = gc->parent->of_node; > > > > > > > > > > > > + if (gc->fwnode) > > > > > > + gc->of_node = to_of_node(gc->fwnode); > > > > > > + > > > > > > /* If the gpiochip has an assigned OF node this takes precedence */ > > > > > > if (gc->of_node) > > > > > > gdev->dev.of_node = gc->of_node; > > > > > > > > > > Similar should be done in acpi_gpio_dev_init(): > > > ^^^^^^ (1) > > ... > > > > If we have parent device for several GPIO devices, this won't work right now > > > due to limitations of fwnode regarding to the sturct device. > > > > > > So, it means we may not have shared primary with different secondary fwnodes. > > > > > > So, come back to the initial suggestion (overwrite it for now): > > > > > > /* > > > * If custom fwnode provided, use it. Currently we may not > > > * handle the case where shared primary node has different > > > * secondary ones. Ideally we have to use > > > * set_secondary_fwnode() here. > > > */ > > > if (gc->fwnode) > > > device_set_node(&gdev->dev, gc->fwnode); > > > > > > > Other parts of gpiolib-of depend on the of_node being there. > > Converting it to fwnode is a whole other task so for now I suggest we > > just convert the fwnode to of_node in struct gpio_chip as per my > > patch. > > But this is about ACPI counterpart. If you do this, do this in both cases. > Above code for ACPI (1). > This series concerns the gpio-sim driver and it only uses configfs (with manually created platform devices) or device-tree. I would prefer to do ACPI separately and I'd like you to lead that because I neither have any HW to test nor claim to understand it. :) Bart
On Mon, Dec 06, 2021 at 09:41:33AM +0100, Bartosz Golaszewski wrote: > On Fri, Dec 3, 2021 at 9:10 PM Andy Shevchenko > <andriy.shevchenko@linux.intel.com> wrote: ... > This series concerns the gpio-sim driver and it only uses configfs > (with manually created platform devices) or device-tree. I would > prefer to do ACPI separately and I'd like you to lead that because I > neither have any HW to test nor claim to understand it. :) Please, mention this in the commit message that ACPI is not covered (yet).
On Mon, Dec 6, 2021 at 2:34 PM Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > > On Mon, Dec 06, 2021 at 09:41:33AM +0100, Bartosz Golaszewski wrote: > > On Fri, Dec 3, 2021 at 9:10 PM Andy Shevchenko > > <andriy.shevchenko@linux.intel.com> wrote: > > ... > > > This series concerns the gpio-sim driver and it only uses configfs > > (with manually created platform devices) or device-tree. I would > > prefer to do ACPI separately and I'd like you to lead that because I > > neither have any HW to test nor claim to understand it. :) > > Please, mention this in the commit message that ACPI is not covered (yet). > But the commit message says: "gpiolib: of: make fwnode take precedence in struct gpio_chip" - it says OF right here. :) Bart
On Mon, Dec 06, 2021 at 02:40:36PM +0100, Bartosz Golaszewski wrote: > On Mon, Dec 6, 2021 at 2:34 PM Andy Shevchenko > <andriy.shevchenko@linux.intel.com> wrote: > > > > On Mon, Dec 06, 2021 at 09:41:33AM +0100, Bartosz Golaszewski wrote: > > > On Fri, Dec 3, 2021 at 9:10 PM Andy Shevchenko > > > <andriy.shevchenko@linux.intel.com> wrote: > > > > ... > > > > > This series concerns the gpio-sim driver and it only uses configfs > > > (with manually created platform devices) or device-tree. I would > > > prefer to do ACPI separately and I'd like you to lead that because I > > > neither have any HW to test nor claim to understand it. :) > > > > Please, mention this in the commit message that ACPI is not covered (yet). > > But the commit message says: "gpiolib: of: make fwnode take precedence > in struct gpio_chip" - it says OF right here. :) It implies that reader should have a 6th sense to know about ACPI and what else? Please, be explicit over implicit.
On Mon, Dec 06, 2021 at 03:48:39PM +0200, Andy Shevchenko wrote: > On Mon, Dec 06, 2021 at 02:40:36PM +0100, Bartosz Golaszewski wrote: > > On Mon, Dec 6, 2021 at 2:34 PM Andy Shevchenko > > <andriy.shevchenko@linux.intel.com> wrote: > > > > > > On Mon, Dec 06, 2021 at 09:41:33AM +0100, Bartosz Golaszewski wrote: > > > > On Fri, Dec 3, 2021 at 9:10 PM Andy Shevchenko > > > > <andriy.shevchenko@linux.intel.com> wrote: > > > > > > ... > > > > > > > This series concerns the gpio-sim driver and it only uses configfs > > > > (with manually created platform devices) or device-tree. I would > > > > prefer to do ACPI separately and I'd like you to lead that because I > > > > neither have any HW to test nor claim to understand it. :) > > > > > > Please, mention this in the commit message that ACPI is not covered (yet). > > > > But the commit message says: "gpiolib: of: make fwnode take precedence > > in struct gpio_chip" - it says OF right here. :) > > It implies that reader should have a 6th sense to know about ACPI and what > else? Please, be explicit over implicit. The problem here is that you change one case and haven't touched the rest which brings inconsistency in the behaviour on different resource providers. We need a record to be sure what disbalance this patch brought. That's why I asked to mention ACPI branch.
On Fri, Dec 03, 2021 at 02:29:59PM +0100, Bartosz Golaszewski wrote: > If the driver sets the fwnode in struct gpio_chip, let it take > precedence over the of_node. By the way, have you tried this on pure DT-less/ACPI-less platform (CONFIG_OF=n, CONFIG_ACPI=n)? I believe gpio-sim in that case won't work, because this doesn't affect swnode case, right?
On Mon, Dec 6, 2021 at 2:55 PM Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > > On Fri, Dec 03, 2021 at 02:29:59PM +0100, Bartosz Golaszewski wrote: > > If the driver sets the fwnode in struct gpio_chip, let it take > > precedence over the of_node. > > By the way, have you tried this on pure DT-less/ACPI-less platform > (CONFIG_OF=n, CONFIG_ACPI=n)? I believe gpio-sim in that case won't work, > because this doesn't affect swnode case, right? > Works just fine on a BeagleBone Black - both the regular GPIO controllers as well as DT-instantiated gpio-sim. Bart
On Mon, Dec 06, 2021 at 03:54:09PM +0200, Andy Shevchenko wrote: > On Fri, Dec 03, 2021 at 02:29:59PM +0100, Bartosz Golaszewski wrote: > > If the driver sets the fwnode in struct gpio_chip, let it take > > precedence over the of_node. > > By the way, have you tried this on pure DT-less/ACPI-less platform > (CONFIG_OF=n, CONFIG_ACPI=n)? I believe gpio-sim in that case won't work, > because this doesn't affect swnode case, right? Okay, swnode will work (*) due to previous patch.
On Mon, Dec 06, 2021 at 03:03:31PM +0100, Bartosz Golaszewski wrote: > On Mon, Dec 6, 2021 at 2:55 PM Andy Shevchenko > <andriy.shevchenko@linux.intel.com> wrote: > > > > On Fri, Dec 03, 2021 at 02:29:59PM +0100, Bartosz Golaszewski wrote: > > > If the driver sets the fwnode in struct gpio_chip, let it take > > > precedence over the of_node. > > > > By the way, have you tried this on pure DT-less/ACPI-less platform > > (CONFIG_OF=n, CONFIG_ACPI=n)? I believe gpio-sim in that case won't work, > > because this doesn't affect swnode case, right? > > > > Works just fine on a BeagleBone Black - both the regular GPIO > controllers as well as DT-instantiated gpio-sim. Yeah, I realized that myself why.
Another iteration of gpio-sim patches. This time the changes are quite small. I removed the ifdefs from gpiolib.c as requested by Andy. In this version gpiolib-of will also prefer fwnodes over of_nodes and - if set - will convert them to of_nodes before proceeding. Tested both with configfs as well as device-tree. v1 -> v2: - add selftests for gpio-sim - add helper programs for selftests - update the configfs rename callback to work with the new API introduced in v5.11 - fix a missing quote in the documentation - use !! whenever using bits operation that are required to return 0 or 1 - use provided bitmap API instead of reimplementing copy or fill operations - fix a deadlock in gpio_sim_direction_output() - add new read-only configfs attributes for mapping of configfs items to GPIO device names - and address other minor issues pointed out in reviews of v1 v2 -> v3: - use devm_bitmap_alloc() instead of the zalloc variant if we're initializing the bitmap with 1s - drop the patch exporting device_is_bound() - don't return -ENODEV from dev_nam and chip_name configfs attributes, return a string indicating that the device is not available yet ('n/a') - fix indentation where it makes sense - don't protect IDA functions which use their own locking and where it's not needed - use kmemdup() instead of kzalloc() + memcpy() - collected review tags - minor coding style fixes v3 -> v4: - return 'none' instead of 'n/a' from dev_name and chip_name before the device is registered - use sysfs_emit() instead of s*printf() - drop GPIO_SIM_MAX_PROP as it's only used in an array's definition where it's fine to hardcode the value v4 -> v5: - drop lib patches that are already upstream - use BIT() instead of (1UL << bit) for flags - fix refcounting for the configfs_dirent in rename() - drop d_move() from the rename() callback - free memory allocated for the live and pending groups in configfs_d_iput() and not in detach_groups() - make sure that if a group of some name is in the live directory, a new group with the same name cannot be created in the pending directory v5 -> v6: - go back to using (1UL << bit) instead of BIT() - if the live group dentry doesn't exist for whatever reason at the time when mkdir() in the pending group is called (would be a BUG()), return -ENOENT instead of -EEXIST which should only be returned if given subsystem already exists in either live or pending group v6 -> v7: - as detailed by Andy in commit 6fda593f3082 ("gpio: mockup: Convert to use software nodes") removing device properties after the platform device is removed but before the GPIO device gets dropped can lead to a use-after-free bug - use software nodes to manually control the freeing of the properties v7 -> v8: - fixed some minor coding style issues as pointed out by Andy v8 -> v9: - dropped the patches implementing committable-items and reworked the driver to not use them - reworked the gpio-line-names property and configuring specific lines in general - many smaller tweaks here and there v9 -> v10: - make writing to 'live' wait for the probe to finish and report an error to user-space if it failed - add the ability to hog lines from the kernel-space - rework locking (drop separate locks for line context objects) - rework the sysfs interface (create a separate group for each line with a constant number of attributes instead of going the other way around) v10 -> v11: - rework the configfs structure to represent a deeper hierarchy that gpiolib supports, namely: multiple banks per platform device v11 -> v12: - simplify patch 2/7 by removing any mentions of OF from gpiolib.c - improve the documentation by adding rest markups - add a device-tree sample to the docs - drop some trailing whitespaces from the driver - make gpio_sim_make_bank_swnode() static - fix coding style in patch 6/7 - add patch 3/7 that makes the OF part of gpiolib prefer to use gpio_chip's fwnode (if set) over of_node Bartosz Golaszewski (7): gpiolib: provide gpiod_remove_hogs() gpiolib: allow to specify the firmware node in struct gpio_chip gpiolib: of: make fwnode take precedence in struct gpio_chip gpio: sim: new testing module selftests: gpio: provide a helper for reading chip info selftests: gpio: add a helper for reading GPIO line names selftests: gpio: add test cases for gpio-sim Documentation/admin-guide/gpio/gpio-sim.rst | 134 ++ drivers/gpio/Kconfig | 8 + drivers/gpio/Makefile | 1 + drivers/gpio/gpio-sim.c | 1594 +++++++++++++++++ drivers/gpio/gpiolib-of.c | 3 + drivers/gpio/gpiolib.c | 18 +- include/linux/gpio/driver.h | 2 + include/linux/gpio/machine.h | 2 + tools/testing/selftests/gpio/.gitignore | 2 + tools/testing/selftests/gpio/Makefile | 4 +- tools/testing/selftests/gpio/config | 1 + tools/testing/selftests/gpio/gpio-chip-info.c | 57 + tools/testing/selftests/gpio/gpio-line-name.c | 55 + tools/testing/selftests/gpio/gpio-sim.sh | 396 ++++ 14 files changed, 2274 insertions(+), 3 deletions(-) create mode 100644 Documentation/admin-guide/gpio/gpio-sim.rst create mode 100644 drivers/gpio/gpio-sim.c create mode 100644 tools/testing/selftests/gpio/gpio-chip-info.c create mode 100644 tools/testing/selftests/gpio/gpio-line-name.c create mode 100755 tools/testing/selftests/gpio/gpio-sim.sh