| Message ID | 20211203133003.31786-1-brgl@bgdev.pl |
|---|---|
| Headers | show |
| Series | gpio-sim: configfs-based GPIO simulator | expand |
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. > Signed-off-by: Bartosz Golaszewski <brgl@bgdev.pl> > --- > drivers/gpio/gpiolib-of.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/drivers/gpio/gpiolib-of.c b/drivers/gpio/gpiolib-of.c > index 0ad288ab6262..91dcf2c6cdd8 100644 > --- a/drivers/gpio/gpiolib-of.c > +++ b/drivers/gpio/gpiolib-of.c > @@ -1046,6 +1046,9 @@ void of_gpio_dev_init(struct gpio_chip *gc, struct gpio_device *gdev) > 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);
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(): > > > > 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 Heck, it's Friday... 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);
On Fri, Dec 03, 2021 at 02:30:00PM +0100, Bartosz Golaszewski wrote: > Implement a new, modern GPIO testing module controlled by configfs > attributes instead of module parameters. The goal of this driver is > to provide a replacement for gpio-mockup that will be easily extensible > with new features and doesn't require reloading the module to change > the setup. ... > +**Group:** ``/config/gpio-sim/gpio-device`` > + > +**Attribute:** ``/config/gpio-sim/gpio-device/dev_name`` > + > +**Attribute:** ``/config/gpio-sim/gpio-device/live`` > + > +This is a directory representing a GPIO platform device. The ``'dev_name'`` > +attribute is read-only and allows the user-space to read the platform device > +name (e.g. ``'gpio-sim.0'``). The ``'live'`` attribute allows to trigger the > +actual creation of the device once it's fully configured. The accepted values > +are: ``'1'`` to enable the simulated device and ``'0'`` to disable and tear > +it down. Perhaps it makes sense to describe properties in the order you expect to be used, then it will be naturally to 'read and repeat' without jumping forward and backward through the documentation. ... > +**Group:** ``/config/gpio-sim/gpio-device/gpio-bankX`` > + > +**Attribute:** ``/config/gpio-sim/gpio-device/gpio-bankX/chip_name`` > +**Attribute:** ``/config/gpio-sim/gpio-device/gpio-bankX/num_lines`` Why not to use the same name as in DT, i.e. ngpios? ... > +#include <linux/gpio/driver.h> > +#include <linux/gpio/machine.h> I would rather move this group below to emphasize that this is closer to GPIO then to other APIs. > +#include <linux/sysfs.h> > + ...here. > +#include "gpiolib.h" ... > +static int gpio_sim_apply_pull(struct gpio_sim_chip *chip, > + unsigned int offset, int value) I would use up to 100 here... > + if (test_bit(FLAG_REQUESTED, &desc->flags) && > + !test_bit(FLAG_IS_OUT, &desc->flags)) { ...here and so on. But it's up to you. ... > + curr_val = !!test_bit(offset, chip->value_map); > + if (curr_val == value) Do you use curr_val anywhere else? Perhaps combine these two lines. > + goto set_pull; ... > +static int gpio_sim_set_config(struct gpio_chip *gc, > + unsigned int offset, unsigned long config) > +{ > + struct gpio_sim_chip *chip = gpiochip_get_data(gc); > + > + switch (pinconf_to_config_param(config)) { > + case PIN_CONFIG_BIAS_PULL_UP: > + return gpio_sim_apply_pull(chip, offset, 1); > + case PIN_CONFIG_BIAS_PULL_DOWN: > + return gpio_sim_apply_pull(chip, offset, 0); > + default: > + break; > + } > + > + return -ENOTSUPP; return directly from switch-case? > +} ... > +static ssize_t gpio_sim_sysfs_pull_show(struct device *dev, > + struct device_attribute *attr, > + char *buf) > +{ > + struct gpio_sim_attribute *line_attr = to_gpio_sim_attr(attr); > + struct gpio_sim_chip *chip = dev_get_drvdata(dev); > + char *repr; > + int pull; int pull_up; ? Also, where is "pull-none"? > + mutex_lock(&chip->lock); > + pull = !!test_bit(line_attr->offset, chip->pull_map); > + mutex_unlock(&chip->lock); > + if (pull) > + repr = "pull-up"; > + else > + repr = "pull-down"; > + > + return sysfs_emit(buf, "%s\n", repr); return sysfs_emit(buf, "%pull-s\n", pull_up ? "up" : "down"); ? > +} ... > +static ssize_t gpio_sim_sysfs_pull_store(struct device *dev, > + struct device_attribute *attr, > + const char *buf, size_t len) > +{ > + struct gpio_sim_attribute *line_attr = to_gpio_sim_attr(attr); > + struct gpio_sim_chip *chip = dev_get_drvdata(dev); > + int ret, pull; > + > + if (sysfs_streq(buf, "pull-down")) > + pull = 0; > + else if (sysfs_streq(buf, "pull-up")) > + pull = 1; > + else > + return -EINVAL; sysfs_match_string() and use the very same string array in the above function to print them? Same question about "pull-none". > + ret = gpio_sim_apply_pull(chip, line_attr->offset, pull); > + if (ret) > + return ret; > + > + return len; > +} ... > + attr_group->name = devm_kasprintf(dev, GFP_KERNEL, > + "sim_gpio%u", i); Wondering if you can use devm_kasprintf_strarray(). > + if (!attr_group->name) > + return -ENOMEM; ... > + /* Default to input mode. */ > + bitmap_fill(chip->direction_map, num_lines); More accurate is to use bitmap_set(). If we ever debug this it also helpful. ... > + ret = devm_add_action_or_reset(dev, gpio_sim_mutex_destroy, > + &chip->lock); It's 81, fine to have on one line. > + if (ret) > + return ret; ... > +static char *gpio_sim_strdup_trimmed(const char *str, size_t count) > +{ > + char *dup, *trimmed, *ret; > + > + dup = kstrndup(str, count, GFP_KERNEL); > + if (!dup) > + return NULL; > + > + trimmed = strstrip(dup); > + ret = kstrdup(trimmed, GFP_KERNEL); > + kfree(dup); > + return ret; Why not memmove() instead of additional memory allocation? Or if you really want to save bytes, krealloc() after? char *dup, *start, *ret; size_t len; dup = kstrndup(str, count, GFP_KERNEL); if (!dup) return NULL; start = strstrip(dup); len = strlen(start) - (start - dup); memmove(dup, start, len + 1); ret = krealloc(dup, len + 1, GFP_KERNEL); if (ret) return ret; kfree(dup); return NULL; ? > +} ... > + return sprintf(page, "%c\n", live ? '1' : '0'); return sprintf(page, "%d\n", live ? 1 : 0); ? ... > + list_for_each_entry(bank, &dev->bank_list, siblings) { > + list_for_each_entry(line, &bank->line_list, siblings) { > + if (line->hog) > + num_hogs++; > + } > + } > + No need to have a blank line here, but up to you. > + if (!num_hogs) > + return 0; ... > + list_for_each_entry(pos, &dev->bank_list, siblings) { > + if (this == pos || (!this->label || !pos->label)) Too many parentheses. > + continue; > + > + if (strcmp(this->label, pos->label) == 0) > + return true; > + } ... > + ret = kstrtouint(page, 10, &live); Why not kstrtobool() (according to the documentation)? > + if (ret) > + return ret; > + > + mutex_lock(&dev->lock); > + > + if ((live == 0 && !gpio_sim_device_is_live_unlocked(dev)) || > + (live == 1 && gpio_sim_device_is_live_unlocked(dev))) > + ret = -EPERM; > + else if (live == 1) > + ret = gpio_sim_device_activate_unlocked(dev); > + else if (live == 0) > + gpio_sim_device_deactivate_unlocked(dev); > + else > + ret = -EINVAL; This will gone if above is being applied. > + mutex_unlock(&dev->lock); ... > + mutex_lock(&dev->lock); > + ret = sprintf(page, "%s\n", bank->label ?: ""); Don't we use "?" in the GPIO library for similar situations? > + mutex_unlock(&dev->lock); ... > + ret = kstrtouint(page, 10, &num_lines); Why not allowing any digit base? > + if (ret) > + return ret; ... > + switch (dir) { > + case GPIOD_IN: > + repr = "input"; > + break; > + case GPIOD_OUT_HIGH: > + repr = "output-high"; > + break; > + case GPIOD_OUT_LOW: > + repr = "output-low"; > + break; > + default: > + /* This would be a programmer bug. */ > + WARN(1, "Unexpected hog direction value: %d", dir); > + return -EINVAL; > + } > + if (strcmp(trimmed, "input") == 0) > + dir = GPIOD_IN; > + else if (strcmp(trimmed, "output-high") == 0) > + dir = GPIOD_OUT_HIGH; > + else if (strcmp(trimmed, "output-low") == 0) > + dir = GPIOD_OUT_LOW; > + else > + dir = -EINVAL; Same idea, i.e. static string array and use it above and here with help of match_string(). ... > +static struct configfs_attribute *gpio_sim_hog_config_attrs[] = { > + &gpio_sim_hog_config_attr_name, > + &gpio_sim_hog_config_attr_direction, > + NULL, Comma is not needed. > +}; ... > + id = ida_alloc(&gpio_sim_ida, GFP_KERNEL); > + if (id < 0) { > + kfree(dev); > + return ERR_PTR(id); > + } > + > + config_group_init_type_name(&dev->group, name, > + &gpio_sim_device_config_group_type); > + dev->id = id; If you group this assignment with above allocation it would look better.
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:08 PM Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > > On Fri, Dec 03, 2021 at 02:30:00PM +0100, Bartosz Golaszewski wrote: > > Implement a new, modern GPIO testing module controlled by configfs > > attributes instead of module parameters. The goal of this driver is > > to provide a replacement for gpio-mockup that will be easily extensible > > with new features and doesn't require reloading the module to change > > the setup. > > ... > > > +**Group:** ``/config/gpio-sim/gpio-device`` > > + > > +**Attribute:** ``/config/gpio-sim/gpio-device/dev_name`` > > + > > +**Attribute:** ``/config/gpio-sim/gpio-device/live`` > > + > > +This is a directory representing a GPIO platform device. The ``'dev_name'`` > > +attribute is read-only and allows the user-space to read the platform device > > +name (e.g. ``'gpio-sim.0'``). The ``'live'`` attribute allows to trigger the > > +actual creation of the device once it's fully configured. The accepted values > > +are: ``'1'`` to enable the simulated device and ``'0'`` to disable and tear > > +it down. > > Perhaps it makes sense to describe properties in the order you expect to be > used, then it will be naturally to 'read and repeat' without jumping forward > and backward through the documentation. > This is such order though. You naturally configure the device, then bank, then lines, then hogs. > ... > > > +**Group:** ``/config/gpio-sim/gpio-device/gpio-bankX`` > > + > > +**Attribute:** ``/config/gpio-sim/gpio-device/gpio-bankX/chip_name`` > > > +**Attribute:** ``/config/gpio-sim/gpio-device/gpio-bankX/num_lines`` > > Why not to use the same name as in DT, i.e. ngpios? > This would be the only attribute that follows the DT naming, the label, line names etc. wouldn't use the same name anyway. I don't see any advantage of it as num_lines is actually more intuitive than ngpios IMO. > ... > > > +#include <linux/gpio/driver.h> > > +#include <linux/gpio/machine.h> > > I would rather move this group below to emphasize that this is closer to GPIO > then to other APIs. > > > +#include <linux/sysfs.h> > > + > > ...here. > With the number of headers in this file, I'd stick with alphabetical order. > > +#include "gpiolib.h" > > ... > > > +static int gpio_sim_apply_pull(struct gpio_sim_chip *chip, > > + unsigned int offset, int value) > > I would use up to 100 here... > > > + if (test_bit(FLAG_REQUESTED, &desc->flags) && > > + !test_bit(FLAG_IS_OUT, &desc->flags)) { > > ...here and so on. > > But it's up to you. > Nah, the lines are broken just fine. Let's not overuse the limit. > ... > > > + curr_val = !!test_bit(offset, chip->value_map); > > + if (curr_val == value) > > Do you use curr_val anywhere else? Perhaps combine these two lines. > > > + goto set_pull; > Good point. > ... > > > +static int gpio_sim_set_config(struct gpio_chip *gc, > > + unsigned int offset, unsigned long config) > > +{ > > + struct gpio_sim_chip *chip = gpiochip_get_data(gc); > > + > > + switch (pinconf_to_config_param(config)) { > > + case PIN_CONFIG_BIAS_PULL_UP: > > + return gpio_sim_apply_pull(chip, offset, 1); > > + case PIN_CONFIG_BIAS_PULL_DOWN: > > + return gpio_sim_apply_pull(chip, offset, 0); > > + default: > > > + break; > > + } > > + > > + return -ENOTSUPP; > > return directly from switch-case? > This may be a personal preference but I don't like returning functions without a return at the end. Always looks wrong at first glance. I'd like to keep it. > > +} > > ... > > > +static ssize_t gpio_sim_sysfs_pull_show(struct device *dev, > > + struct device_attribute *attr, > > + char *buf) > > +{ > > + struct gpio_sim_attribute *line_attr = to_gpio_sim_attr(attr); > > + struct gpio_sim_chip *chip = dev_get_drvdata(dev); > > + char *repr; > > + int pull; > > int pull_up; > > ? Also, where is "pull-none"? > There isn't. If it's ever needed, we can extend the driver but so far there hasn't been a need for it when testing from user-space. > > + mutex_lock(&chip->lock); > > + pull = !!test_bit(line_attr->offset, chip->pull_map); > > + mutex_unlock(&chip->lock); > > > + if (pull) > > + repr = "pull-up"; > > + else > > + repr = "pull-down"; > > + > > + return sysfs_emit(buf, "%s\n", repr); > > return sysfs_emit(buf, "%pull-s\n", pull_up ? "up" : "down"); > > ? Sure. > > > +} > > ... > > > +static ssize_t gpio_sim_sysfs_pull_store(struct device *dev, > > + struct device_attribute *attr, > > + const char *buf, size_t len) > > +{ > > + struct gpio_sim_attribute *line_attr = to_gpio_sim_attr(attr); > > + struct gpio_sim_chip *chip = dev_get_drvdata(dev); > > + int ret, pull; > > + > > + if (sysfs_streq(buf, "pull-down")) > > + pull = 0; > > + else if (sysfs_streq(buf, "pull-up")) > > + pull = 1; > > + else > > + return -EINVAL; > > sysfs_match_string() and use the very same string array in the above function > to print them? > > Same question about "pull-none". > > > + ret = gpio_sim_apply_pull(chip, line_attr->offset, pull); > > + if (ret) > > + return ret; > > + > > + return len; > > +} > > ... > > > + attr_group->name = devm_kasprintf(dev, GFP_KERNEL, > > + "sim_gpio%u", i); > > Wondering if you can use devm_kasprintf_strarray(). > I would need to do that in a separate loop and handle the new array, I think it's an overkill here. > > + if (!attr_group->name) > > + return -ENOMEM; > > ... > > > + /* Default to input mode. */ > > + bitmap_fill(chip->direction_map, num_lines); > > More accurate is to use bitmap_set(). If we ever debug this it also helpful. > I'm not sure what you mean, this sets all bits to 1. > ... > > > + ret = devm_add_action_or_reset(dev, gpio_sim_mutex_destroy, > > + &chip->lock); > > It's 81, fine to have on one line. > > > + if (ret) > > + return ret; > > ... > > > +static char *gpio_sim_strdup_trimmed(const char *str, size_t count) > > +{ > > + char *dup, *trimmed, *ret; > > + > > + dup = kstrndup(str, count, GFP_KERNEL); > > + if (!dup) > > + return NULL; > > + > > + trimmed = strstrip(dup); > > + ret = kstrdup(trimmed, GFP_KERNEL); > > + kfree(dup); > > + return ret; > > Why not memmove() instead of additional memory allocation? > > Or if you really want to save bytes, krealloc() after? > > char *dup, *start, *ret; > size_t len; > > dup = kstrndup(str, count, GFP_KERNEL); > if (!dup) > return NULL; > > start = strstrip(dup); > len = strlen(start) - (start - dup); > > memmove(dup, start, len + 1); > > ret = krealloc(dup, len + 1, GFP_KERNEL); > if (ret) > return ret; > > kfree(dup); > return NULL; > > ? > > > +} > > ... > > > + return sprintf(page, "%c\n", live ? '1' : '0'); > > return sprintf(page, "%d\n", live ? 1 : 0); > > ? > > ... > > > + list_for_each_entry(bank, &dev->bank_list, siblings) { > > + list_for_each_entry(line, &bank->line_list, siblings) { > > + if (line->hog) > > + num_hogs++; > > + } > > + } > > > + > > No need to have a blank line here, but up to you. > > > + if (!num_hogs) > > + return 0; > > ... > > > + list_for_each_entry(pos, &dev->bank_list, siblings) { > > + if (this == pos || (!this->label || !pos->label)) > > Too many parentheses. > No, this is the logic here. Skip either if both pointers point to the same object or check if the labels are missing in at least one. > > + continue; > > + > > + if (strcmp(this->label, pos->label) == 0) > > + return true; > > + } > > ... > > > + ret = kstrtouint(page, 10, &live); > > Why not kstrtobool() (according to the documentation)? > Sure. > > + if (ret) > > + return ret; > > + > > + mutex_lock(&dev->lock); > > + > > + if ((live == 0 && !gpio_sim_device_is_live_unlocked(dev)) || > > + (live == 1 && gpio_sim_device_is_live_unlocked(dev))) > > + ret = -EPERM; > > + else if (live == 1) > > + ret = gpio_sim_device_activate_unlocked(dev); > > + else if (live == 0) > > + gpio_sim_device_deactivate_unlocked(dev); > > > + else > > + ret = -EINVAL; > > This will gone if above is being applied. > > > + mutex_unlock(&dev->lock); > > ... > > > + mutex_lock(&dev->lock); > > + ret = sprintf(page, "%s\n", bank->label ?: ""); > > Don't we use "?" in the GPIO library for similar situations? > We use it in gpiolib to indicate a lack of label but here, it's the configuration part. I think an empty string works fine. > > + mutex_unlock(&dev->lock); > > ... > > > + ret = kstrtouint(page, 10, &num_lines); > > Why not allowing any digit base? > Sure. > > + if (ret) > > + return ret; > > ... > > > + switch (dir) { > > + case GPIOD_IN: > > + repr = "input"; > > + break; > > + case GPIOD_OUT_HIGH: > > + repr = "output-high"; > > + break; > > + case GPIOD_OUT_LOW: > > + repr = "output-low"; > > + break; > > + default: > > + /* This would be a programmer bug. */ > > + WARN(1, "Unexpected hog direction value: %d", dir); > > + return -EINVAL; > > + } > > > > + if (strcmp(trimmed, "input") == 0) > > + dir = GPIOD_IN; > > + else if (strcmp(trimmed, "output-high") == 0) > > + dir = GPIOD_OUT_HIGH; > > + else if (strcmp(trimmed, "output-low") == 0) > > + dir = GPIOD_OUT_LOW; > > + else > > + dir = -EINVAL; > > > Same idea, i.e. static string array and use it above and here with help > of match_string(). > It would be great but GPIOD_IN etc. are bit flags and not sequence enums. > ... > > > +static struct configfs_attribute *gpio_sim_hog_config_attrs[] = { > > + &gpio_sim_hog_config_attr_name, > > + &gpio_sim_hog_config_attr_direction, > > > + NULL, > > Comma is not needed. > > > +}; > > ... > > > + id = ida_alloc(&gpio_sim_ida, GFP_KERNEL); > > + if (id < 0) { > > + kfree(dev); > > + return ERR_PTR(id); > > + } > > + > > + config_group_init_type_name(&dev->group, name, > > + &gpio_sim_device_config_group_type); > > + dev->id = id; > > If you group this assignment with above allocation it would look better. > Actually I think it looks better now with allocating the resources first, then setting up the structure. Bart > -- > With Best Regards, > Andy Shevchenko > >
On Mon, Dec 06, 2021 at 10:48:00AM +0100, Bartosz Golaszewski wrote: > On Fri, Dec 3, 2021 at 9:08 PM Andy Shevchenko > <andriy.shevchenko@linux.intel.com> wrote: > > On Fri, Dec 03, 2021 at 02:30:00PM +0100, Bartosz Golaszewski wrote: ... > > > +#include <linux/gpio/driver.h> > > > +#include <linux/gpio/machine.h> > > > > I would rather move this group below to emphasize that this is closer to GPIO > > then to other APIs. > > > > > +#include <linux/sysfs.h> > > > + > > > > ...here. > > > > With the number of headers in this file, I'd stick with alphabetical order. I understand that and agree, but my point is orthogonal to this. The idea is to emphasize that "hey. this driver has tough relations with the GPIO subsystem". This is the way, for example, IIO does and I like it. > > > +#include "gpiolib.h" ... > > > +static int gpio_sim_apply_pull(struct gpio_sim_chip *chip, > > > + unsigned int offset, int value) > > > > I would use up to 100 here... > > > > > + if (test_bit(FLAG_REQUESTED, &desc->flags) && > > > + !test_bit(FLAG_IS_OUT, &desc->flags)) { > > > > ...here and so on. > > > > But it's up to you. > > > > Nah, the lines are broken just fine. Let's not overuse the limit. Yes, but I would consider to join back those which are up to ~83 characters (I already pointed out at least to one example like this). ... > > > + if (sysfs_streq(buf, "pull-down")) > > > + pull = 0; > > > + else if (sysfs_streq(buf, "pull-up")) > > > + pull = 1; > > > + else > > > + return -EINVAL; > > > > sysfs_match_string() and use the very same string array in the above function > > to print them? I suppose you agree on this? ... > > > + /* Default to input mode. */ > > > + bitmap_fill(chip->direction_map, num_lines); > > > > More accurate is to use bitmap_set(). If we ever debug this it also helpful. > > I'm not sure what you mean, this sets all bits to 1. Nope, it may set _more_ than all bits. That's why bitmap_set() is more accurate, because it will do exact setting. ... > > > + if (strcmp(trimmed, "input") == 0) > > > + dir = GPIOD_IN; > > > + else if (strcmp(trimmed, "output-high") == 0) > > > + dir = GPIOD_OUT_HIGH; > > > + else if (strcmp(trimmed, "output-low") == 0) > > > + dir = GPIOD_OUT_LOW; > > > + else > > > + dir = -EINVAL; > > > > Same idea, i.e. static string array and use it above and here with help > > of match_string(). > > It would be great but GPIOD_IN etc. are bit flags and not sequence enums. Ah, okay, it will make rather sparse array.
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 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 Mon, Dec 06, 2021 at 04:38:44PM +0100, Bartosz Golaszewski wrote: > On Mon, Dec 6, 2021 at 2:33 PM Andy Shevchenko > <andriy.shevchenko@linux.intel.com> wrote: > > On Mon, Dec 06, 2021 at 10:48:00AM +0100, Bartosz Golaszewski wrote: > > > On Fri, Dec 3, 2021 at 9:08 PM Andy Shevchenko > > > <andriy.shevchenko@linux.intel.com> wrote: ... > > > Nah, the lines are broken just fine. Let's not overuse the limit. > > > > Yes, but I would consider to join back those which are up to ~83 characters > > (I already pointed out at least to one example like this). > > I like the old-style limit TBH. And it's fine. It has remark about overlapping in case of readability and 81 characters on one line is fine as some cases when it is up to ~83 (for old style). Anyways, it doesn't worth of spending more time on this. Your choice then. ... > > > > > + /* Default to input mode. */ > > > > > + bitmap_fill(chip->direction_map, num_lines); > > > > > > > > More accurate is to use bitmap_set(). If we ever debug this it also helpful. > > > > > > I'm not sure what you mean, this sets all bits to 1. > > > > Nope, it may set _more_ than all bits. That's why bitmap_set() is more > > accurate, because it will do exact setting. > > Can this in any way affect any of the code? If the driver is correct, > it will never use anything beyond the last line bit. If it does, it > needs fixing. It's as if we cared about what happens to padding added > to structures by the compiler (as long as we're not passing it to > user-space of course). I haven't checked if it affects current code. Consider this as heads up, because developers often forget about this nuance of bitmap_fill() / bitmap_clear(). ... > > > > > + if (strcmp(trimmed, "input") == 0) > > > > > + dir = GPIOD_IN; > > > > > + else if (strcmp(trimmed, "output-high") == 0) > > > > > + dir = GPIOD_OUT_HIGH; > > > > > + else if (strcmp(trimmed, "output-low") == 0) > > > > > + dir = GPIOD_OUT_LOW; > > > > > + else > > > > > + dir = -EINVAL; > > > > > > > > Same idea, i.e. static string array and use it above and here with help > > > > of match_string(). > > > > > > It would be great but GPIOD_IN etc. are bit flags and not sequence enums. > > > > Ah, okay, it will make rather sparse array. > > Idea for the future: introduce match_string_ext() with flags one of > which would allow sparse string arrays? I thought about that, but since it's a mix of the bits, it might not be so universal anyway, I would rather think of something which uses 1-bit bit fields unified under a bit mask, and not a mix of 2 or more bits.
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