| Message ID | 20200904154547.3836-1-brgl@bgdev.pl |
|---|---|
| Headers | show |
| Series | gpio: mockup: support dynamically created and removed chips | expand |
On Fri, Sep 04, 2020 at 05:45:32PM +0200, Bartosz Golaszewski wrote: > From: Bartosz Golaszewski <bgolaszewski@baylibre.com> > > We don't need a custom logging helper. Let's use the standard pr_fmt() > macro which allows us to use all pr_*() routines with custom format. > > Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com> > --- > drivers/gpio/gpio-mockup.c | 9 +++++---- > 1 file changed, 5 insertions(+), 4 deletions(-) > > diff --git a/drivers/gpio/gpio-mockup.c b/drivers/gpio/gpio-mockup.c > index 349782cdb4d7..73cd51459c2a 100644 > --- a/drivers/gpio/gpio-mockup.c > +++ b/drivers/gpio/gpio-mockup.c > @@ -21,6 +21,9 @@ > > #include "gpiolib.h" > +#undef pr_fmt > +#define pr_fmt(fmt) GPIO_MOCKUP_NAME ": " fmt > + Just put definition to be first line of code before other inclusions and drop unnecessary #undef. > #define GPIO_MOCKUP_NAME "gpio-mockup" > #define GPIO_MOCKUP_MAX_GC 10 > /* > @@ -31,8 +34,6 @@ > /* Maximum of three properties + the sentinel. */ > #define GPIO_MOCKUP_MAX_PROP 4 > > -#define gpio_mockup_err(...) pr_err(GPIO_MOCKUP_NAME ": " __VA_ARGS__) > - > /* > * struct gpio_pin_status - structure describing a GPIO status > * @dir: Configures direction of gpio as "in" or "out" > @@ -549,7 +550,7 @@ static int __init gpio_mockup_init(void) > > err = platform_driver_register(&gpio_mockup_driver); > if (err) { > - gpio_mockup_err("error registering platform driver\n"); > + pr_err("error registering platform driver\n"); > return err; > } > > @@ -577,7 +578,7 @@ static int __init gpio_mockup_init(void) > > pdev = platform_device_register_full(&pdevinfo); > if (IS_ERR(pdev)) { > - gpio_mockup_err("error registering device"); > + pr_err("error registering device"); > platform_driver_unregister(&gpio_mockup_driver); > gpio_mockup_unregister_pdevs(); > return PTR_ERR(pdev); > -- > 2.26.1 >
On Fri, Sep 04, 2020 at 05:45:38PM +0200, Bartosz Golaszewski wrote: > From: Bartosz Golaszewski <bgolaszewski@baylibre.com> > > GPIO line names are currently created by the driver from the chip label. > We'll want to support custom formats for line names (for instance: to > name all lines the same) for user-space tests so create them in the > module init function and pass them to the driver using the standard > 'gpio-line-names' property. > > Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com> > --- > drivers/gpio/gpio-mockup.c | 70 +++++++++++++++++++++----------------- > 1 file changed, 38 insertions(+), 32 deletions(-) > > diff --git a/drivers/gpio/gpio-mockup.c b/drivers/gpio/gpio-mockup.c > index ce83f1df1933..96976ba66598 100644 > --- a/drivers/gpio/gpio-mockup.c > +++ b/drivers/gpio/gpio-mockup.c > @@ -18,6 +18,7 @@ > #include <linux/platform_device.h> > #include <linux/property.h> > #include <linux/slab.h> > +#include <linux/string_helpers.h> > #include <linux/uaccess.h> > > #include "gpiolib.h" > @@ -378,29 +379,6 @@ static void gpio_mockup_debugfs_setup(struct device *dev, > return; > } > > -static int gpio_mockup_name_lines(struct device *dev, > - struct gpio_mockup_chip *chip) > -{ > - struct gpio_chip *gc = &chip->gc; > - char **names; > - int i; > - > - names = devm_kcalloc(dev, gc->ngpio, sizeof(char *), GFP_KERNEL); > - if (!names) > - return -ENOMEM; > - > - for (i = 0; i < gc->ngpio; i++) { > - names[i] = devm_kasprintf(dev, GFP_KERNEL, > - "%s-%d", gc->label, i); > - if (!names[i]) > - return -ENOMEM; > - } > - > - gc->names = (const char *const *)names; > - > - return 0; > -} > - > static void gpio_mockup_dispose_mappings(void *data) > { > struct gpio_mockup_chip *chip = data; > @@ -468,12 +446,6 @@ static int gpio_mockup_probe(struct platform_device *pdev) > for (i = 0; i < gc->ngpio; i++) > chip->lines[i].dir = GPIO_LINE_DIRECTION_IN; > > - if (device_property_read_bool(dev, "named-gpio-lines")) { > - rv = gpio_mockup_name_lines(dev, chip); > - if (rv) > - return rv; > - } > - > chip->irq_sim_domain = devm_irq_domain_create_sim(dev, NULL, > gc->ngpio); > if (IS_ERR(chip->irq_sim_domain)) > @@ -524,6 +496,27 @@ static void gpio_mockup_unregister_devices(void) > } > } > > +static __init char **gpio_mockup_make_line_names(const char *label, > + unsigned int num_lines) > +{ > + unsigned int i; > + char **names; > + > + names = kcalloc(num_lines + 1, sizeof(char *), GFP_KERNEL); > + if (!names) > + return NULL; > + > + for (i = 0; i < num_lines; i++) { > + names[i] = kasprintf(GFP_KERNEL, "%s-%u", label, i); > + if (!names[i]) { > + kfree_strarray(names, i); > + return NULL; > + } > + } > + > + return names; > +} > + > static int __init gpio_mockup_init(void) > { > struct property_entry properties[GPIO_MOCKUP_MAX_PROP]; > @@ -531,6 +524,7 @@ static int __init gpio_mockup_init(void) > struct gpio_mockup_device *mockup_dev; > int i, prop, num_chips, err = 0, base; > struct platform_device_info pdevinfo; > + char **line_names; > u16 ngpio; > > if ((gpio_mockup_num_ranges < 2) || > @@ -563,6 +557,7 @@ static int __init gpio_mockup_init(void) > memset(properties, 0, sizeof(properties)); > memset(&pdevinfo, 0, sizeof(pdevinfo)); > prop = 0; > + line_names = NULL; > > snprintf(chip_label, sizeof(chip_label), > "gpio-mockup-%c", i + 'A'); > @@ -578,9 +573,18 @@ static int __init gpio_mockup_init(void) > : gpio_mockup_range_ngpio(i) - base; > properties[prop++] = PROPERTY_ENTRY_U16("nr-gpios", ngpio); > > - if (gpio_mockup_named_lines) > - properties[prop++] = PROPERTY_ENTRY_BOOL( > - "named-gpio-lines"); > + if (gpio_mockup_named_lines) { > + line_names = gpio_mockup_make_line_names(chip_label, > + ngpio); > + if (!line_names) { > + err = -ENOMEM; > + goto err_out; > + } > + > + properties[prop++] = PROPERTY_ENTRY_STRING_ARRAY_LEN( > + "gpio-line-names", > + line_names, ngpio); > + } Indentation here looks quite deep. Maybe introduce a helper in between where you assign properties? > pdevinfo.name = "gpio-mockup"; > pdevinfo.id = i; > @@ -588,11 +592,13 @@ static int __init gpio_mockup_init(void) > > mockup_dev = kzalloc(sizeof(*mockup_dev), GFP_KERNEL); > if (!mockup_dev) { > + kfree_strarray(line_names, ngpio); > err = -ENOMEM; > goto err_out; > } > > mockup_dev->pdev = platform_device_register_full(&pdevinfo); > + kfree_strarray(line_names, ngpio); > if (IS_ERR(mockup_dev->pdev)) { > pr_err("error registering device"); > kfree(mockup_dev); > -- > 2.26.1 >
On Fri, Sep 04, 2020 at 05:45:40PM +0200, Bartosz Golaszewski wrote: > From: Bartosz Golaszewski <bgolaszewski@baylibre.com> > > This is in preparation for dynamically created chips. > > Let's split out the code that can be reused when creating chips at > run-time. Let's also move the code preparing the device properties into > a separate routine. This has the advantage of simplifying the error > handling. Almost all contents of this patch should go to proposed helper as I mentioned before. Will make this patch quite small and understandable. > Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com> > --- > drivers/gpio/gpio-mockup.c | 165 ++++++++++++++++++++----------------- > 1 file changed, 90 insertions(+), 75 deletions(-) > > diff --git a/drivers/gpio/gpio-mockup.c b/drivers/gpio/gpio-mockup.c > index 995e37fef9c5..eb94ddac5fee 100644 > --- a/drivers/gpio/gpio-mockup.c > +++ b/drivers/gpio/gpio-mockup.c > @@ -524,16 +524,78 @@ static __init char **gpio_mockup_make_line_names(const char *label, > return names; > } > > -static int __init gpio_mockup_init(void) > +static int __init gpio_mockup_register_device(struct property_entry *properties) > { > - struct property_entry properties[GPIO_MOCKUP_MAX_PROP]; > - char chip_label[GPIO_MOCKUP_LABEL_SIZE]; > struct gpio_mockup_device *mockup_dev; > - int i, prop, num_chips, err = 0, base; > struct platform_device_info pdevinfo; > - char **line_names; > + > + memset(&pdevinfo, 0, sizeof(pdevinfo)); > + > + mockup_dev = kzalloc(sizeof(*mockup_dev), GFP_KERNEL); > + if (!mockup_dev) > + return -ENOMEM; > + > + pdevinfo.name = "gpio-mockup"; > + pdevinfo.properties = properties; > + > + pdevinfo.id = ida_alloc(&gpio_mockup_ida, GFP_KERNEL); > + if (pdevinfo.id < 0) { > + kfree(mockup_dev); > + return pdevinfo.id; > + } > + > + mockup_dev->pdev = platform_device_register_full(&pdevinfo); > + if (IS_ERR(mockup_dev->pdev)) { > + ida_free(&gpio_mockup_ida, pdevinfo.id); > + kfree(mockup_dev); > + return PTR_ERR(mockup_dev->pdev); > + } > + > + list_add(&mockup_dev->list, &gpio_mockup_devices); > + > + return 0; > +} > + > +static int __init gpio_mockup_register_one_chip_from_params(int idx) > +{ > + char chip_label[GPIO_MOCKUP_LABEL_SIZE], **line_names = NULL; > + struct property_entry properties[GPIO_MOCKUP_MAX_PROP]; > + int prop = 0, base, ret; > u16 ngpio; > > + memset(properties, 0, sizeof(properties)); > + > + snprintf(chip_label, sizeof(chip_label), "gpio-mockup-%c", idx + 'A'); > + properties[prop++] = PROPERTY_ENTRY_STRING("chip-label", > + chip_label); > + > + base = gpio_mockup_range_base(idx); > + if (base >= 0) > + properties[prop++] = PROPERTY_ENTRY_U32("gpio-base", > + base); > + > + ngpio = base < 0 ? gpio_mockup_range_ngpio(idx) > + : gpio_mockup_range_ngpio(idx) - base; > + properties[prop++] = PROPERTY_ENTRY_U16("nr-gpios", ngpio); > + > + if (gpio_mockup_named_lines) { > + line_names = gpio_mockup_make_line_names(chip_label, ngpio); > + if (!line_names) > + return -ENOMEM; > + > + properties[prop++] = PROPERTY_ENTRY_STRING_ARRAY_LEN( > + "gpio-line-names", line_names, ngpio); > + } > + > + ret = gpio_mockup_register_device(properties); > + kfree_strarray(line_names, ngpio); > + return ret; > +} > + > +static int __init gpio_mockup_register_chips_from_params(void) > +{ > + int num_chips, i, ret; > + > if ((gpio_mockup_num_ranges < 2) || > (gpio_mockup_num_ranges % 2) || > (gpio_mockup_num_ranges > GPIO_MOCKUP_MAX_RANGES)) > @@ -551,86 +613,39 @@ static int __init gpio_mockup_init(void) > return -EINVAL; > } > > - gpio_mockup_dbg_dir = debugfs_create_dir("gpio-mockup", NULL); > - > - err = platform_driver_register(&gpio_mockup_driver); > - if (err) { > - pr_err("error registering platform driver\n"); > - debugfs_remove_recursive(gpio_mockup_dbg_dir); > - return err; > - } > - > for (i = 0; i < num_chips; i++) { > - memset(properties, 0, sizeof(properties)); > - memset(&pdevinfo, 0, sizeof(pdevinfo)); > - prop = 0; > - line_names = NULL; > - > - snprintf(chip_label, sizeof(chip_label), > - "gpio-mockup-%c", i + 'A'); > - properties[prop++] = PROPERTY_ENTRY_STRING("chip-label", > - chip_label); > - > - base = gpio_mockup_range_base(i); > - if (base >= 0) > - properties[prop++] = PROPERTY_ENTRY_U32("gpio-base", > - base); > - > - ngpio = base < 0 ? gpio_mockup_range_ngpio(i) > - : gpio_mockup_range_ngpio(i) - base; > - properties[prop++] = PROPERTY_ENTRY_U16("nr-gpios", ngpio); > - > - if (gpio_mockup_named_lines) { > - line_names = gpio_mockup_make_line_names(chip_label, > - ngpio); > - if (!line_names) { > - err = -ENOMEM; > - goto err_out; > - } > - > - properties[prop++] = PROPERTY_ENTRY_STRING_ARRAY_LEN( > - "gpio-line-names", > - line_names, ngpio); > + ret = gpio_mockup_register_one_chip_from_params(i); > + if (ret) { > + gpio_mockup_unregister_devices(); > + return ret; > } > + } > > - pdevinfo.name = "gpio-mockup"; > - pdevinfo.properties = properties; > + return 0; > +} > > - pdevinfo.id = ida_alloc(&gpio_mockup_ida, GFP_KERNEL); > - if (pdevinfo.id < 0) { > - kfree_strarray(line_names, ngpio); > - err = pdevinfo.id; > - goto err_out; > - } > +static int __init gpio_mockup_init(void) > +{ > + int ret; > > - mockup_dev = kzalloc(sizeof(*mockup_dev), GFP_KERNEL); > - if (!mockup_dev) { > - kfree_strarray(line_names, ngpio); > - ida_free(&gpio_mockup_ida, pdevinfo.id); > - err = -ENOMEM; > - goto err_out; > - } > + gpio_mockup_dbg_dir = debugfs_create_dir("gpio-mockup", NULL); > > - mockup_dev->pdev = platform_device_register_full(&pdevinfo); > - kfree_strarray(line_names, ngpio); > - if (IS_ERR(mockup_dev->pdev)) { > - pr_err("error registering device"); > - ida_free(&gpio_mockup_ida, pdevinfo.id); > - kfree(mockup_dev); > - err = PTR_ERR(mockup_dev->pdev); > - goto err_out; > - } > + ret = platform_driver_register(&gpio_mockup_driver); > + if (ret) { > + pr_err("error registering platform driver\n"); > + debugfs_remove_recursive(gpio_mockup_dbg_dir); > + return ret; > + } > > - list_add(&mockup_dev->list, &gpio_mockup_devices); > + ret = gpio_mockup_register_chips_from_params(); > + if (ret) { > + pr_err("error registering device"); > + debugfs_remove_recursive(gpio_mockup_dbg_dir); > + platform_driver_unregister(&gpio_mockup_driver); > + return ret; > } > > return 0; > - > -err_out: > - platform_driver_unregister(&gpio_mockup_driver); > - gpio_mockup_unregister_devices(); > - debugfs_remove_recursive(gpio_mockup_dbg_dir); > - return err; > } > > static void __exit gpio_mockup_exit(void) > -- > 2.26.1 >
Hi, On 9/4/20 8:45 AM, Bartosz Golaszewski wrote: > From: Bartosz Golaszewski <bgolaszewski@baylibre.com> > > There's some documentation for gpio-mockup's debugfs interface in the > driver's source but it's not much. Add proper documentation for this > testing module. > > Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com> > --- > .../admin-guide/gpio/gpio-mockup.rst | 87 +++++++++++++++++++ > 1 file changed, 87 insertions(+) > create mode 100644 Documentation/admin-guide/gpio/gpio-mockup.rst > > diff --git a/Documentation/admin-guide/gpio/gpio-mockup.rst b/Documentation/admin-guide/gpio/gpio-mockup.rst > new file mode 100644 > index 000000000000..1d452ee55f8d > --- /dev/null > +++ b/Documentation/admin-guide/gpio/gpio-mockup.rst > @@ -0,0 +1,87 @@ > +.. SPDX-License-Identifier: GPL-2.0-only > + > +GPIO Testing Driver > +=================== > + > +The GPIO Testing Driver (gpio-mockup) provides a way to create simulated GPIO > +chips for testing purposes. There are two ways of configuring the chips exposed > +by the module. The lines can be accessed using the standard GPIO character > +device interface as well as manipulated using the dedicated debugfs directory > +structure. Could configfs be used for this instead of debugfs? debugfs is ad hoc. > + > +Creating simulated chips using debugfs > +-------------------------------------- > + > +When the gpio-mockup module is loaded (or builtin) it creates its own directory > +in debugfs. Assuming debugfs is mounted at /sys/kernel/debug/, the directory > +will be located at /sys/kernel/debug/gpio-mockup/. Inside this directory there > +are two attributes: new_device and delete_device. > + > +New chips can be created by writing a single line containing a number of > +options to "new_device". For example: > + > +.. code-block:: sh > + > + $ echo "label=my-mockup num_lines=4 named_lines" > /sys/kernel/debug/gpio-mockup/new_device > + > +Supported options: > + > + num_lines=<num_lines> - number of GPIO lines to expose > + > + label=<label> - label of the dummy chip > + > + named_lines - defines whether dummy lines should be named, the names are > + of the form X-Y where X is the chip's label and Y is the > + line's offset > + > +Note: only num_lines is mandatory. > + > +Chips can be dynamically removed by writing the chip's label to > +"delete_device". For example: > + > +.. code-block:: sh > + > + echo "gpio-mockup.0" > /sys/kernel/debug/gpio-mockup/delete_device > + > +Creating simulated chips using module params > +-------------------------------------------- > + > +Note: this is an older, now deprecated method kept for backward compatibility > +for user-space tools. > + > +When loading the gpio-mockup driver a number of parameters can be passed to the > +module. > + > + gpio_mockup_ranges > + > + This parameter takes an argument in the form of an array of integer > + pairs. Each pair defines the base GPIO number (if any) and the number > + of lines exposed by the chip. If the base GPIO is -1, the gpiolib > + will assign it automatically. > + > + Example: gpio_mockup_ranges=-1,8,-1,16,405,4 > + > + The line above creates three chips. The first one will expose 8 lines, > + the second 16 and the third 4. The base GPIO for the third chip is set > + to 405 while for two first chips it will be assigned automatically. > + > + gpio_named_lines > + > + This parameter doesn't take any arguments. It lets the driver know that > + GPIO lines exposed by it should be named. > + > + The name format is: gpio-mockup-X-Y where X is the letter associated > + with the mockup chip and Y is the line offset. Where does this 'X' letter associated with the mockup chip come from? > + > +Manipulating simulated lines > +---------------------------- > + > +Each mockup chip creates its own subdirectory in /sys/kernel/debug/gpio-mockup/. > +The directory is named after the chip's label. A symlink is also created, named > +after the chip's name, which points to the label directory. > + > +Inside each subdirectory, there's a separate attribute for each GPIO line. The > +name of the attribute represents the line's offset in the chip. > + > +Reading from a line attribute returns the current value. Writing to it (0 or 1) > +changes its pull. What does "pull" mean here? thanks.
On Fri, Sep 04, 2020 at 08:15:59PM -0700, Randy Dunlap wrote: > On 9/4/20 8:45 AM, Bartosz Golaszewski wrote: ... > > +GPIO Testing Driver > > +=================== > > + > > +The GPIO Testing Driver (gpio-mockup) provides a way to create simulated GPIO > > +chips for testing purposes. There are two ways of configuring the chips exposed > > +by the module. The lines can be accessed using the standard GPIO character > > +device interface as well as manipulated using the dedicated debugfs directory > > +structure. > > Could configfs be used for this instead of debugfs? > debugfs is ad hoc. Actually sounds like a good idea.
On Sat, Sep 5, 2020 at 5:16 AM Randy Dunlap <rdunlap@infradead.org> wrote: > > Hi, > > On 9/4/20 8:45 AM, Bartosz Golaszewski wrote: > > From: Bartosz Golaszewski <bgolaszewski@baylibre.com> > > > > There's some documentation for gpio-mockup's debugfs interface in the > > driver's source but it's not much. Add proper documentation for this > > testing module. > > > > Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com> > > --- > > .../admin-guide/gpio/gpio-mockup.rst | 87 +++++++++++++++++++ > > 1 file changed, 87 insertions(+) > > create mode 100644 Documentation/admin-guide/gpio/gpio-mockup.rst > > > > diff --git a/Documentation/admin-guide/gpio/gpio-mockup.rst b/Documentation/admin-guide/gpio/gpio-mockup.rst > > new file mode 100644 > > index 000000000000..1d452ee55f8d > > --- /dev/null > > +++ b/Documentation/admin-guide/gpio/gpio-mockup.rst > > @@ -0,0 +1,87 @@ > > +.. SPDX-License-Identifier: GPL-2.0-only > > + > > +GPIO Testing Driver > > +=================== > > + > > +The GPIO Testing Driver (gpio-mockup) provides a way to create simulated GPIO > > +chips for testing purposes. There are two ways of configuring the chips exposed > > +by the module. The lines can be accessed using the standard GPIO character > > +device interface as well as manipulated using the dedicated debugfs directory > > +structure. > > Could configfs be used for this instead of debugfs? > debugfs is ad hoc. > > > + > > +Creating simulated chips using debugfs > > +-------------------------------------- > > + > > +When the gpio-mockup module is loaded (or builtin) it creates its own directory > > +in debugfs. Assuming debugfs is mounted at /sys/kernel/debug/, the directory > > +will be located at /sys/kernel/debug/gpio-mockup/. Inside this directory there > > +are two attributes: new_device and delete_device. > > + > > +New chips can be created by writing a single line containing a number of > > +options to "new_device". For example: > > + > > +.. code-block:: sh > > + > > + $ echo "label=my-mockup num_lines=4 named_lines" > /sys/kernel/debug/gpio-mockup/new_device > > + > > +Supported options: > > + > > + num_lines=<num_lines> - number of GPIO lines to expose > > + > > + label=<label> - label of the dummy chip > > + > > + named_lines - defines whether dummy lines should be named, the names are > > + of the form X-Y where X is the chip's label and Y is the > > + line's offset > > + > > +Note: only num_lines is mandatory. > > + > > +Chips can be dynamically removed by writing the chip's label to > > +"delete_device". For example: > > + > > +.. code-block:: sh > > + > > + echo "gpio-mockup.0" > /sys/kernel/debug/gpio-mockup/delete_device > > + > > +Creating simulated chips using module params > > +-------------------------------------------- > > + > > +Note: this is an older, now deprecated method kept for backward compatibility > > +for user-space tools. > > + > > +When loading the gpio-mockup driver a number of parameters can be passed to the > > +module. > > + > > + gpio_mockup_ranges > > + > > + This parameter takes an argument in the form of an array of integer > > + pairs. Each pair defines the base GPIO number (if any) and the number > > + of lines exposed by the chip. If the base GPIO is -1, the gpiolib > > + will assign it automatically. > > + > > + Example: gpio_mockup_ranges=-1,8,-1,16,405,4 > > + > > + The line above creates three chips. The first one will expose 8 lines, > > + the second 16 and the third 4. The base GPIO for the third chip is set > > + to 405 while for two first chips it will be assigned automatically. > > + > > + gpio_named_lines > > + > > + This parameter doesn't take any arguments. It lets the driver know that > > + GPIO lines exposed by it should be named. > > + > > + The name format is: gpio-mockup-X-Y where X is the letter associated > > + with the mockup chip and Y is the line offset. > > Where does this 'X' letter associated with the mockup chip come from? > > > + > > +Manipulating simulated lines > > +---------------------------- > > + > > +Each mockup chip creates its own subdirectory in /sys/kernel/debug/gpio-mockup/. > > +The directory is named after the chip's label. A symlink is also created, named > > +after the chip's name, which points to the label directory. > > + > > +Inside each subdirectory, there's a separate attribute for each GPIO line. The > > +name of the attribute represents the line's offset in the chip. > > + > > +Reading from a line attribute returns the current value. Writing to it (0 or 1) > > +changes its pull. > > What does "pull" mean here? > Yeah I should probably clarify this. "Pull" here means a simulated pull-down/up resistor basically. Bart
On Fri, Sep 4, 2020 at 6:44 PM Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > > On Fri, Sep 04, 2020 at 05:45:28PM +0200, Bartosz Golaszewski wrote: > > From: Bartosz Golaszewski <bgolaszewski@baylibre.com> > > > > devprop_gpiochip_set_names() is overly complicated with taking the > > fwnode argument (which requires using dev_fwnode() & of_fwnode_handle() > > in ACPI and OF GPIO code respectively). Let's just switch to using the > > generic device properties. > > > > This allows us to pull the code setting line names directly into > > gpiochip_add_data_with_key() instead of handling it separately for > > ACPI and OF. > > > > Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com> > > --- > > drivers/gpio/gpiolib-acpi.c | 3 --- > > drivers/gpio/gpiolib-devprop.c | 19 ++++++++++--------- > > drivers/gpio/gpiolib-of.c | 5 ----- > > drivers/gpio/gpiolib.c | 8 ++++---- > > include/linux/gpio/driver.h | 3 +-- > > 5 files changed, 15 insertions(+), 23 deletions(-) > > > > diff --git a/drivers/gpio/gpiolib-acpi.c b/drivers/gpio/gpiolib-acpi.c > > index 54ca3c18b291..834a12f3219e 100644 > > --- a/drivers/gpio/gpiolib-acpi.c > > +++ b/drivers/gpio/gpiolib-acpi.c > > @@ -1221,9 +1221,6 @@ void acpi_gpiochip_add(struct gpio_chip *chip) > > return; > > } > > > > - if (!chip->names) > > - devprop_gpiochip_set_names(chip, dev_fwnode(chip->parent)); > > - > > acpi_gpiochip_request_regions(acpi_gpio); > > acpi_gpiochip_scan_gpios(acpi_gpio); > > acpi_walk_dep_device_list(handle); > > diff --git a/drivers/gpio/gpiolib-devprop.c b/drivers/gpio/gpiolib-devprop.c > > index 26741032fa9e..a28659b4f9c9 100644 > > --- a/drivers/gpio/gpiolib-devprop.c > > +++ b/drivers/gpio/gpiolib-devprop.c > > @@ -17,25 +17,24 @@ > > /** > > * devprop_gpiochip_set_names - Set GPIO line names using device properties > > * @chip: GPIO chip whose lines should be named, if possible > > - * @fwnode: Property Node containing the gpio-line-names property > > * > > * Looks for device property "gpio-line-names" and if it exists assigns > > * GPIO line names for the chip. The memory allocated for the assigned > > - * names belong to the underlying firmware node and should not be released > > + * names belong to the underlying software node and should not be released > > * by the caller. > > */ > > -void devprop_gpiochip_set_names(struct gpio_chip *chip, > > - const struct fwnode_handle *fwnode) > > +int devprop_gpiochip_set_names(struct gpio_chip *chip) > > { > > struct gpio_device *gdev = chip->gpiodev; > > + struct device *dev = chip->parent; > > const char **names; > > int ret, i; > > int count; > > > > - count = fwnode_property_read_string_array(fwnode, "gpio-line-names", > > + count = device_property_read_string_array(dev, "gpio-line-names", > > NULL, 0); > > if (count < 0) > > - return; > > + return 0; > > Can we introduce a followup to 33ee09cd59ce ("device property: Add helpers to > count items in an array") for strings? > Sure, I'll do this in v2. Bart
On Mon, Sep 07, 2020 at 01:05:54PM +0200, Bartosz Golaszewski wrote: > On Fri, Sep 4, 2020 at 6:57 PM Andy Shevchenko > <andriy.shevchenko@linux.intel.com> wrote: > > On Fri, Sep 04, 2020 at 05:45:40PM +0200, Bartosz Golaszewski wrote: > > > From: Bartosz Golaszewski <bgolaszewski@baylibre.com> > > > > > > This is in preparation for dynamically created chips. > > > > > > Let's split out the code that can be reused when creating chips at > > > run-time. Let's also move the code preparing the device properties into > > > a separate routine. This has the advantage of simplifying the error > > > handling. > > > > Almost all contents of this patch should go to proposed helper as I mentioned > > before. Will make this patch quite small and understandable. > Sorry, I'm not sure what you're referring to. I meant if you move changes done here to the patch where I complained about indentation you might have better series. Have you considered that?
On Mon, Sep 07, 2020 at 12:26:34PM +0200, Bartosz Golaszewski wrote: > On Mon, Sep 7, 2020 at 11:59 AM Andy Shevchenko > <andriy.shevchenko@linux.intel.com> wrote: > > > > On Fri, Sep 04, 2020 at 08:15:59PM -0700, Randy Dunlap wrote: > > > On 9/4/20 8:45 AM, Bartosz Golaszewski wrote: > > > > ... > > > > > > +GPIO Testing Driver > > > > +=================== > > > > + > > > > +The GPIO Testing Driver (gpio-mockup) provides a way to create simulated GPIO > > > > +chips for testing purposes. There are two ways of configuring the chips exposed > > > > +by the module. The lines can be accessed using the standard GPIO character > > > > +device interface as well as manipulated using the dedicated debugfs directory > > > > +structure. > > > > > > Could configfs be used for this instead of debugfs? > > > debugfs is ad hoc. > > > > Actually sounds like a good idea. > > > > Well, then we can go on and write an entirely new mockup driver > (ditching module params and dropping any backwards compatibility) > because we're already using debugfs for line values. > > How would we pass the device properties to configfs created GPIO chips > anyway? Devices seem to only be created using mkdir. Am I missing > something? Same way how USB composite works, no?
On Mon, Sep 7, 2020 at 1:53 PM Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > > On Mon, Sep 07, 2020 at 12:26:34PM +0200, Bartosz Golaszewski wrote: > > On Mon, Sep 7, 2020 at 11:59 AM Andy Shevchenko > > <andriy.shevchenko@linux.intel.com> wrote: > > > > > > On Fri, Sep 04, 2020 at 08:15:59PM -0700, Randy Dunlap wrote: > > > > On 9/4/20 8:45 AM, Bartosz Golaszewski wrote: > > > > > > ... > > > > > > > > +GPIO Testing Driver > > > > > +=================== > > > > > + > > > > > +The GPIO Testing Driver (gpio-mockup) provides a way to create simulated GPIO > > > > > +chips for testing purposes. There are two ways of configuring the chips exposed > > > > > +by the module. The lines can be accessed using the standard GPIO character > > > > > +device interface as well as manipulated using the dedicated debugfs directory > > > > > +structure. > > > > > > > > Could configfs be used for this instead of debugfs? > > > > debugfs is ad hoc. > > > > > > Actually sounds like a good idea. > > > > > > > Well, then we can go on and write an entirely new mockup driver > > (ditching module params and dropping any backwards compatibility) > > because we're already using debugfs for line values. > > > > How would we pass the device properties to configfs created GPIO chips > > anyway? Devices seem to only be created using mkdir. Am I missing > > something? > > Same way how USB composite works, no? > OK, so create a new chip directory in configfs, configure it using some defined configfs attributes and then finally instantiate it from sysfs? Makes sense and is probably the right way to go. Now the question is: is it fine to just entirely remove the previous gpio-mockup? Should we keep some backwards compatibility? Should we introduce an entirely new module and have a transition period before removing previous gpio-mockup? Also: this is a testing module so to me debugfs is just fine. Is configfs considered stable ABI like sysfs? Bart
On Mon, Sep 07, 2020 at 02:06:15PM +0200, Bartosz Golaszewski wrote: > On Mon, Sep 7, 2020 at 1:53 PM Andy Shevchenko > <andriy.shevchenko@linux.intel.com> wrote: > > > > On Mon, Sep 07, 2020 at 12:26:34PM +0200, Bartosz Golaszewski wrote: > > > On Mon, Sep 7, 2020 at 11:59 AM Andy Shevchenko > > > <andriy.shevchenko@linux.intel.com> wrote: > > > > > > > > On Fri, Sep 04, 2020 at 08:15:59PM -0700, Randy Dunlap wrote: > > > > > On 9/4/20 8:45 AM, Bartosz Golaszewski wrote: > > > > > > > > ... > > > > > > > > > > +GPIO Testing Driver > > > > > > +=================== > > > > > > + > > > > > > +The GPIO Testing Driver (gpio-mockup) provides a way to create simulated GPIO > > > > > > +chips for testing purposes. There are two ways of configuring the chips exposed > > > > > > +by the module. The lines can be accessed using the standard GPIO character > > > > > > +device interface as well as manipulated using the dedicated debugfs directory > > > > > > +structure. > > > > > > > > > > Could configfs be used for this instead of debugfs? > > > > > debugfs is ad hoc. > > > > > > > > Actually sounds like a good idea. > > > > > > > > > > Well, then we can go on and write an entirely new mockup driver > > > (ditching module params and dropping any backwards compatibility) > > > because we're already using debugfs for line values. > > > > > > How would we pass the device properties to configfs created GPIO chips > > > anyway? Devices seem to only be created using mkdir. Am I missing > > > something? > > > > Same way how USB composite works, no? > > > > OK, so create a new chip directory in configfs, configure it using > some defined configfs attributes and then finally instantiate it from > sysfs? > > Makes sense and is probably the right way to go. Now the question is: > is it fine to just entirely remove the previous gpio-mockup? Should we > keep some backwards compatibility? Should we introduce an entirely new > module and have a transition period before removing previous > gpio-mockup? > > Also: this is a testing module so to me debugfs is just fine. Is > configfs considered stable ABI like sysfs? Yes it is. Or at least until you fix all existing users so that if you do change it, no one notices it happening :) thanks, greg k-h
Hi Andy, On Mon, Sep 7, 2020 at 4:14 PM Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > On Mon, Sep 07, 2020 at 03:49:23PM +0200, Bartosz Golaszewski wrote: > > On Mon, Sep 7, 2020 at 2:22 PM Greg Kroah-Hartman > > <gregkh@linuxfoundation.org> wrote: > > > On Mon, Sep 07, 2020 at 02:06:15PM +0200, Bartosz Golaszewski wrote: > > ... > > > > Yes it is. Or at least until you fix all existing users so that if you > > > do change it, no one notices it happening :) > > > > > > > Then another question is: do we really want to commit to a stable ABI > > for a module we only use for testing purposes and which doesn't > > interact with any real hardware. > > > > Rewriting this module without any legacy cruft is tempting though. :) > > Another thought spoken loudly: maybe it can be unified with GPIO aggregator > code? In that case it makes sense. You want to aggregate GPIOs out of thin air?
On Fri, Sep 11, 2020 at 3:01 PM Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote: > > On Tue, Sep 08, 2020 at 07:03:30PM +0200, Bartosz Golaszewski wrote: > > On Mon, Sep 7, 2020 at 2:22 PM Greg Kroah-Hartman > > <gregkh@linuxfoundation.org> wrote: > > > > > > On Mon, Sep 07, 2020 at 02:06:15PM +0200, Bartosz Golaszewski wrote: > > > > On Mon, Sep 7, 2020 at 1:53 PM Andy Shevchenko > > > > <andriy.shevchenko@linux.intel.com> wrote: > > > > > > > > > > On Mon, Sep 07, 2020 at 12:26:34PM +0200, Bartosz Golaszewski wrote: > > > > > > On Mon, Sep 7, 2020 at 11:59 AM Andy Shevchenko > > > > > > <andriy.shevchenko@linux.intel.com> wrote: > > > > > > > > > > > > > > On Fri, Sep 04, 2020 at 08:15:59PM -0700, Randy Dunlap wrote: > > > > > > > > On 9/4/20 8:45 AM, Bartosz Golaszewski wrote: > > > > > > > > > > > > > > ... > > > > > > > > > > > > > > > > +GPIO Testing Driver > > > > > > > > > +=================== > > > > > > > > > + > > > > > > > > > +The GPIO Testing Driver (gpio-mockup) provides a way to create simulated GPIO > > > > > > > > > +chips for testing purposes. There are two ways of configuring the chips exposed > > > > > > > > > +by the module. The lines can be accessed using the standard GPIO character > > > > > > > > > +device interface as well as manipulated using the dedicated debugfs directory > > > > > > > > > +structure. > > > > > > > > > > > > > > > > Could configfs be used for this instead of debugfs? > > > > > > > > debugfs is ad hoc. > > > > > > > > > > > > > > Actually sounds like a good idea. > > > > > > > > > > > > > > > > > > > Well, then we can go on and write an entirely new mockup driver > > > > > > (ditching module params and dropping any backwards compatibility) > > > > > > because we're already using debugfs for line values. > > > > > > > > > > > > How would we pass the device properties to configfs created GPIO chips > > > > > > anyway? Devices seem to only be created using mkdir. Am I missing > > > > > > something? > > > > > > > > > > Same way how USB composite works, no? > > > > > > > > > > > > > OK, so create a new chip directory in configfs, configure it using > > > > some defined configfs attributes and then finally instantiate it from > > > > sysfs? > > > > > > > > Makes sense and is probably the right way to go. Now the question is: > > > > is it fine to just entirely remove the previous gpio-mockup? Should we > > > > keep some backwards compatibility? Should we introduce an entirely new > > > > module and have a transition period before removing previous > > > > gpio-mockup? > > > > > > > > Also: this is a testing module so to me debugfs is just fine. Is > > > > configfs considered stable ABI like sysfs? > > > > > > Yes it is. Or at least until you fix all existing users so that if you > > > do change it, no one notices it happening :) > > > > > > > Got it. One more question: the current debugfs interface we're using > > in gpio-mockup exists to allow to read current values of GPIO lines in > > output mode (check how the user drives dummy lines) and to set their > > simulated pull-up/pull-down resistors (what values the user reads in > > input mode). > > > > This works like this: in /sys/kernel/debug/gpio-mockup every dummy > > chip creates its own directory (e.g. > > /sys/kernel/debug/gpio-mockup/gpiochip0) and inside this directory > > there's an attribute per line named after the line's offset (e.g. > > /sys/kernel/debug/gpio-mockup/gpiochip0/4). Writing 0 or 1 to this > > attribute sets the pull resistor. Reading from it yields the current > > value (0 or 1 as well). > > > > This is pretty non-standard so I proposed to put it in debugfs. If we > > were to use configfs - is this where something like this should go? Or > > rather sysfs? Is it even suitable/acceptable for sysfs? > > That sounds like it would work in sysfs just fine as-is, why don't you > all want to use that? configfs is good for "set a bunch of attributes > to different values and then do a 'create/go/work'" type action. > I've started looking into it. I need to first implement committable items for configfs because mockup GPIO chips need to be configured before they're instantiated. It'll be configfs to configure and instantiate each chip and a set of sysfs attributes to manipulate existing chips. Bartosz
From: Bartosz Golaszewski <bgolaszewski@baylibre.com> We're about to merge w V2 user API for GPIO. In user-space we're using the gpio-mockup driver for testing but it's quite cumbersome (needs unloading and reloading to change chip configuration) and not very extensible (config is passed over module params). This series proposes to extend the debugfs interface to support dynamic creation and removal of dummy chips, with extensible options. First 3 patches add some lib functionality we'll use later on. Next 3 contain general gpiolib refactoring and can be picked up independently. Next we refactor gpio-mockup and finally add the delete_device and new_device attributes. Last patch adds documentation for gpio-mockup so I'm not going into detail on how the new interface works - the doc describes it pretty well. Bartosz Golaszewski (23): lib: cmdline: export next_arg() lib: string_helpers: provide kfree_strarray() lib: uaccess: provide getline_from_user() gpiolib: generalize devprop_gpiochip_set_names() for device properties gpiolib: unexport devprop_gpiochip_set_names() gpiolib: switch to simpler IDA interface gpio: mockup: drop unneeded includes gpio: mockup: use pr_fmt() gpio: mockup: use KBUILD_MODNAME gpio: mockup: fix resource leak in error path gpio: mockup: remove the limit on number of dummy chips gpio: mockup: define a constant for chip label size gpio: mockup: pass the chip label as device property gpio: mockup: use the generic 'gpio-line-names' property gpio: mockup: use dynamic device IDs gpio: mockup: refactor the module init function gpio: mockup: rename and move around debugfs callbacks gpio: mockup: require debugfs to build gpio: mockup: add a symlink for the per-chip debugfs directory gpio: mockup: add a lock for dummy device list gpio: mockup: provide a way to delete dummy chips gpio: mockup: provide a way to create new dummy chips Documentation: gpio: add documentation for gpio-mockup .../admin-guide/gpio/gpio-mockup.rst | 87 +++ drivers/gpio/Kconfig | 1 + drivers/gpio/Makefile | 1 - drivers/gpio/gpio-mockup.c | 614 ++++++++++++++---- drivers/gpio/gpiolib-acpi.c | 3 - drivers/gpio/gpiolib-devprop.c | 63 -- drivers/gpio/gpiolib-of.c | 5 - drivers/gpio/gpiolib.c | 62 +- include/linux/gpio/driver.h | 3 - include/linux/string_helpers.h | 2 + include/linux/uaccess.h | 3 + lib/cmdline.c | 1 + lib/string_helpers.c | 22 + lib/usercopy.c | 37 ++ 14 files changed, 705 insertions(+), 199 deletions(-) create mode 100644 Documentation/admin-guide/gpio/gpio-mockup.rst delete mode 100644 drivers/gpio/gpiolib-devprop.c