lib: bitmap: remove the 'extern' keyword from function declarations

Review

Quoted 6 lines · Bartosz Golaszewski

From: Bartosz Golaszewski <[email protected]>

The 'extern' keyword doesn't have any benefits in header files. Remove it.

(quoted diff) +2 -0

+int __bitmap_equal(const unsigned long *bitmap1,
+                const unsigned long *bitmap2, unsigned int nbits);

Why not

int __bitmap_equal(const unsigned long *bitmap1, const unsigned long *bitmap2,

                   unsigned int nbits);

and so on?

It's even in 80 limit.

Quoted 14 lines · Andy Shevchenko

Review

Quoted 6 lines · Bartosz Golaszewski

From: Bartosz Golaszewski <[email protected]>

The 'extern' keyword doesn't have any benefits in header files. Remove it.

(quoted diff) +2 -0

+int __bitmap_equal(const unsigned long *bitmap1,
+                const unsigned long *bitmap2, unsigned int nbits);

Why not

int __bitmap_equal(const unsigned long *bitmap1, const unsigned long *bitmap2,

                   unsigned int nbits);

Quoted 4 lines

and so on?

It's even in 80 limit.

I feel like this is purely a matter of taste. No rules define exactly how the lines should be broken. I prefer the longer part to be below, it just looks better to my eyes.

Quoted 7 lines · Bartosz Golaszewski

From: Bartosz Golaszewski <[email protected]>

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.

...

Review

Quoted 12 lines

(quoted diff) +12 -0

+In order to destroy a simulated chip, it has to be moved back to pending first
+and then removed using rmdir().
+
+Currently supported configuration attributes are:
+
+  num_lines - an unsigned integer value defining the number of GPIO lines to
+              export
+
+  label - a string defining the label for the GPIO chip
+
+  line_names - a list of GPIO line names in the form of quoted strings
+               separated by commas, e.g.: '"foo", "bar", "", "foobar". The

Unmatched ' (single quote).

Quoted 5 lines

(quoted diff) +5 -0

+               number of strings doesn't have to be equal to the value set in
+               the num_lines attribute. If it's lower than the number of lines,
+               the remaining lines are unnamed. If it's larger, the superfluous
+               lines are ignored. A name of the form: '""' means the line
+               should be unnamed.

...

Quoted 19 lines

(quoted diff) +19 -0

+#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+
+#include <linux/bitmap.h>
+#include <linux/configfs.h>
+#include <linux/device.h>
+#include <linux/gpio/driver.h>
+#include <linux/idr.h>
+#include <linux/interrupt.h>
+#include <linux/irq.h>
+#include <linux/irq_sim.h>
+#include <linux/mod_devicetable.h>
+#include <linux/module.h>
+#include <linux/mutex.h>
+#include <linux/platform_device.h>
+#include <linux/property.h>
+#include <linux/slab.h>
+#include <linux/string.h>
+#include <linux/string_helpers.h>
+#include <linux/sysfs.h>

...

Quoted 8 lines

(quoted diff) +8 -0

+static int gpio_sim_apply_pull(struct gpio_sim_chip *chip,
+                            unsigned int offset, int value)
+{
+     int curr_val, irq, irq_type, ret;
+     struct gpio_desc *desc;
+     struct gpio_chip *gc;
+
+     gc = &chip->gc;

Review

Quoted 10 lines

(quoted diff) +9 -0

+     desc = &gc->gpiodev->descs[offset];
+
+     mutex_lock(&chip->lock);
+
+     if (test_bit(FLAG_REQUESTED, &desc->flags) &&
+         !test_bit(FLAG_IS_OUT, &desc->flags)) {
 
+             curr_val = test_bit(offset, chip->values);
+             if (curr_val == value)
+                     goto set_pull;

Theoretically (I haven't checked all of the arch code) test_bit() may return any value, hence, shouldn't you rather do something like

                if (!!curr_val == !!value)

?

Quoted 4 lines

(quoted diff) +4 -0

+             /*
+              * This is fine - it just means, nobody is listening
+              * for interrupts on this line, otherwise
+              * irq_create_mapping() would have been called from

Review

Quoted 10 lines

(quoted diff) +10 -0

+              * the to_irq() callback.
+              */
+             irq = irq_find_mapping(chip->irq_sim, offset);
+             if (!irq)
+                     goto set_value;
+
+             irq_type = irq_get_trigger_type(irq);
+
+             if ((value && (irq_type & IRQ_TYPE_EDGE_RISING)) ||
+                 (value == 0 && (irq_type & IRQ_TYPE_EDGE_FALLING))) {

!value ?

Quoted 18 lines

(quoted diff) +18 -0

+                     ret = irq_set_irqchip_state(irq, IRQCHIP_STATE_PENDING,
+                                                 true);
+                     if (ret)
+                             goto set_pull;
+             }
+     }
+
+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))
+             __assign_bit(offset, chip->values, value);
+
+set_pull:
+     __assign_bit(offset, chip->pulls, value);
+     mutex_unlock(&chip->lock);
+     return 0;
+}

...

Review

Quoted 10 lines

(quoted diff) +9 -0

+static int gpio_sim_get_multiple(struct gpio_chip *gc,
+                              unsigned long *mask, unsigned long *bits)
+{
+     struct gpio_sim_chip *chip = gpiochip_get_data(gc);
+     unsigned int bit;
+
+     mutex_lock(&chip->lock);
 
+     for_each_set_bit(bit, mask, gc->ngpio)
+             __assign_bit(bit, bits, test_bit(bit, chip->values));

I tried to understand the logic here and why it's different to bitmap_copy(). Any hints?

Quoted 4 lines

(quoted diff) +4 -0

+     mutex_unlock(&chip->lock);
+
+     return 0;
+}

...

Review

Quoted 9 lines

(quoted diff) +9 -0

+static void gpio_sim_set_multiple(struct gpio_chip *gc,
+                               unsigned long *mask, unsigned long *bits)
+{
+     struct gpio_sim_chip *chip = gpiochip_get_data(gc);
+     unsigned int bit;
+
+     mutex_lock(&chip->lock);
+     for_each_set_bit(bit, mask, gc->ngpio)
+             __assign_bit(bit, chip->values, test_bit(bit, bits));

Ditto.

Quoted 2 lines

(quoted diff) +2 -0

+     mutex_unlock(&chip->lock);
+}

...

Review

Quoted 11 lines

(quoted diff) +10 -0

+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);

But aren't we got a parameter (1 or 0) from config? And hence

        case PIN_CONFIG_BIAS_PULL_UP:
        case PIN_CONFIG_BIAS_PULL_DOWN:
                return gpio_sim_apply_pull(chip, offset, <param>);

?

Quoted 7 lines

(quoted diff) +6 -0

+     default:
+             break;
+     }
+
+     return -ENOTSUPP;
+}

...

Quoted 7 lines

(quoted diff) +7 -0

+static ssize_t gpio_sim_sysfs_line_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, val;

Review

Quoted 5 lines

(quoted diff) +5 -0

+     ret = kstrtoint(buf, 0, &val);
+     if (ret)
+             return ret;
+     if (val != 0 && val != 1)
+             return -EINVAL;

kstrtobool() ?

Quoted 6 lines

(quoted diff) +6 -0

+     ret = gpio_sim_apply_pull(chip, line_attr->offset, val);
+     if (ret)
+             return ret;
+
+     return len;
+}

...

Review

Quoted 10 lines

(quoted diff) +10 -0

+static int gpio_sim_setup_sysfs(struct gpio_sim_chip *chip)
+{
+     unsigned int i, num_lines = chip->gc.ngpio;
+     struct device *dev = chip->gc.parent;
+     struct gpio_sim_attribute *line_attr;
+     struct device_attribute *dev_attr;
+     struct attribute **attrs;
+     int ret;
+
+     attrs = devm_kcalloc(dev, sizeof(struct attribute *),

sizeof(*attr) ?

Quoted 1 line

+ num_lines + 1, GFP_KERNEL);

and hence one line?

Quoted 34 lines

(quoted diff) +34 -0

+     if (!attrs)
+             return -ENOMEM;
+
+     for (i = 0; i < num_lines; i++) {
+             line_attr = devm_kzalloc(dev, sizeof(*line_attr), GFP_KERNEL);
+             if (!line_attr)
+                     return -ENOMEM;
+
+             line_attr->offset = i;
+
+             dev_attr = &line_attr->dev_attr;
+
+             dev_attr->attr.name = devm_kasprintf(dev, GFP_KERNEL,
+                                                  "gpio%u", i);
+             if (!dev_attr->attr.name)
+                     return -ENOMEM;
+
+             dev_attr->attr.mode = 0644;
+
+             dev_attr->show = gpio_sim_sysfs_line_show;
+             dev_attr->store = gpio_sim_sysfs_line_store;
+
+             attrs[i] = &dev_attr->attr;
+     }
+
+     chip->attr_group.name = "line-ctrl";
+     chip->attr_group.attrs = attrs;
+
+     ret = sysfs_create_group(&dev->kobj, &chip->attr_group);
+     if (ret)
+             return ret;
+
+     return devm_add_action_or_reset(dev, gpio_sim_sysfs_remove, chip);
+}

...

Quoted 19 lines

(quoted diff) +19 -0

+static int gpio_sim_probe(struct platform_device *pdev)
+{
+     struct device *dev = &pdev->dev;
+     struct gpio_sim_chip *chip;
+     struct gpio_chip *gc;
+     const char *label;
+     unsigned int bit;
+     u32 num_lines;
+     int ret;
+
+     ret = device_property_read_u32(dev, "gpio-sim,nr-gpios", &num_lines);
+     if (ret)
+             return ret;
+
+     ret = device_property_read_string(dev, "gpio-sim,label", &label);
+     if (ret)
+             label = dev_name(dev);
+
+     chip = devm_kzalloc(dev, sizeof(*chip), GFP_KERNEL);

Review

Quoted 10 lines

(quoted diff) +9 -0

+     if (!chip)
+             return -ENOMEM;
+
+     chip->directions = devm_bitmap_zalloc(dev, num_lines, GFP_KERNEL);
+     if (!chip->directions)
+             return -ENOMEM;
 
+     /* Default to input mode. */
+     for_each_clear_bit(bit, chip->directions, num_lines)
+             __set_bit(bit, chip->directions);

Again, what's the difference to have it as

        bitmap_fill(..., num_lines);

?

Quoted 35 lines

(quoted diff) +35 -0

+     chip->values = devm_bitmap_zalloc(dev, num_lines, GFP_KERNEL);
+     if (!chip->values)
+             return -ENOMEM;
+
+     chip->pulls = devm_bitmap_zalloc(dev, num_lines, GFP_KERNEL);
+     if (!chip->pulls)
+             return -ENOMEM;
+
+     chip->irq_sim = devm_irq_domain_create_sim(dev, NULL, num_lines);
+     if (IS_ERR(chip->irq_sim))
+             return PTR_ERR(chip->irq_sim);
+
+     mutex_init(&chip->lock);
+     ret = devm_add_action_or_reset(dev, gpio_sim_mutex_destroy,
+                                    &chip->lock);
+     if (ret)
+             return ret;
+
+     gc = &chip->gc;
+     gc->base = -1;
+     gc->ngpio = num_lines;
+     gc->label = label;
+     gc->owner = THIS_MODULE;
+     gc->parent = dev;
+     gc->get = gpio_sim_get;
+     gc->set = gpio_sim_set;
+     gc->get_multiple = gpio_sim_get_multiple;
+     gc->set_multiple = gpio_sim_set_multiple;
+     gc->direction_output = gpio_sim_direction_output;
+     gc->direction_input = gpio_sim_direction_input;
+     gc->get_direction = gpio_sim_get_direction;
+     gc->set_config = gpio_sim_set_config;
+     gc->to_irq = gpio_sim_to_irq;
+     gc->free = gpio_sim_free;
+

Review

Quoted 10 lines

(quoted diff) +9 -0

+     ret = devm_gpiochip_add_data(dev, gc, chip);
+     if (ret)
+             return ret;
+
+     ret = gpio_sim_setup_sysfs(chip);
+     if (ret)
+             return ret;
 
+     /* For sysfs callbacks. */
+     dev_set_drvdata(dev, chip);

Shouldn't it be set _before_ sysfs node registration?

Quoted 2 lines

(quoted diff) +2 -0

+     return 0;
+}

...

Review

Quoted 3 lines

(quoted diff) +3 -0

+static const struct of_device_id gpio_sim_of_match[] = {
+     { .compatible = "gpio-simulator" },
+     { },

No comma needed.

Quoted 1 line

+};

...

Review

Quoted 9 lines

(quoted diff) +9 -0

+struct gpio_sim_chip_config {
+     struct config_item item;
+
+     /*
+      * If pdev is NULL, the item is 'pending' (waiting for configuration).
+      * Once the pointer is assigned, the device has been created and the
+      * item is 'live'.
+      */
+     struct platform_device *pdev;

Are you sure

        struct device *dev;

is not sufficient?

Quoted 13 lines

(quoted diff) +13 -0

+     /*
+      * Each configfs filesystem operation is protected with the subsystem
+      * mutex. Each separate attribute is protected with the buffer mutex.
+      * This structure however can be modified by callbacks of different
+      * attributes so we need another lock.
+      */
+     struct mutex lock;
+
+     char label[32];
+     unsigned int num_lines;
+     char **line_names;
+     unsigned int num_line_names;
+};

...

Quoted 13 lines

(quoted diff) +13 -0

+static ssize_t gpio_sim_config_label_store(struct config_item *item,
+                                        const char *page, size_t count)
+{
+     struct gpio_sim_chip_config *config = to_gpio_sim_chip_config(item);
+     char *dup, *trimmed;
+     int ret;
+
+     mutex_lock(&config->lock);
+
+     if (config->pdev) {
+             mutex_unlock(&config->lock);
+             return -EBUSY;
+     }

Review

Quoted 10 lines

(quoted diff) +9 -0

 
+     dup = kstrndup(page, count, GFP_KERNEL);
+     if (!dup) {
+             mutex_unlock(&config->lock);
+             return -ENOMEM;
+     }
+
+     trimmed = strstrip(dup);
+     ret = snprintf(config->label, sizeof(config->label), "%s", trimmed);
+     kfree(dup);

All these sounds like reinvention of kasprintf().

Quoted 8 lines

(quoted diff) +8 -0

+     if (ret < 0) {
+             mutex_unlock(&config->lock);
+             return ret;
+     }
+
+     mutex_unlock(&config->lock);
+     return count;
+}

...

Quoted 4 lines

(quoted diff) +4 -0

+     for (i = 0; i < config->num_line_names; i++) {
+             ret = sprintf(page + written,
+                     i < config->num_line_names - 1 ?
+                             "\"%s\", " : "\"%s\"\n",

Review

Quoted 10 lines

(quoted diff) +10 -0

+                     config->line_names[i] ?: "");
+             if (ret < 0) {
+                     mutex_unlock(&config->lock);
+                     return ret;
+             }
+
+             written += ret;
+     }
+
+     written += ret;

Hmm... twice for the last element, is it correct?

...

Honestly, I don't like the idea of Yet Another (custom) Parser in the kernel.

Have you investigated existing parsers? We have cmdline.c, gpio-aggregator.c, etc. Besides the fact of test cases which are absent here. And who knows what we allow to be entered.

Quoted 8 lines

(quoted diff) +8 -0

+     /*
+      * Line names are stored in a pointer array so that we can easily
+      * pass them down to the GPIO subsystem in a "gpio-line-names"
+      * property.
+      *
+      * Line names must be passed as a list of quoted names separated by
+      * commas, for example: '"foo", "bar", "foobar"'.
+      */

...

Review

Quoted 6 lines

(quoted diff) +5 -0

+static struct configfs_attribute *gpio_sim_config_attrs[] = {
+     &gpio_sim_config_attr_label,
+     &gpio_sim_config_attr_num_lines,
+     &gpio_sim_config_attr_line_names,
 
+     NULL,

No need to have comma.

Quoted 1 line

+};

...

Review

Quoted 2 lines

(quoted diff) +2 -0

+module_init(gpio_sim_init);
+module_exit(gpio_sim_exit);

Perhaps each one closer to the function it refers to?

--
With Best Regards,
Andy Shevchenko

Quoted 5 lines · Andy Shevchenko

Quoted 1 line · Bartosz Golaszewski

From: Bartosz Golaszewski <[email protected]>

...

Quoted 6 lines

Honestly, I don't like the idea of Yet Another (custom) Parser in the kernel.

Have you investigated existing parsers? We have cmdline.c, gpio-aggregator.c, etc. Besides the fact of test cases which are absent here. And who knows what we allow to be entered.

Yes, I looked all around the kernel to find something I could reuse but failed to find anything useful for this particular purpose. If you have something you could point me towards, I'm open to alternatives.

Once we agree on the form of the module, I'll port self-tests to using it instead of gpio-mockup, so we'll have some tests in the tree.

Quoted 7 lines · Bartosz Golaszewski

Quoted 5 lines · Andy Shevchenko

Quoted 1 line · Bartosz Golaszewski

From: Bartosz Golaszewski <[email protected]>

...

[snip]

Quoted 7 lines

Quoted 6 lines

Honestly, I don't like the idea of Yet Another (custom) Parser in the kernel.

Have you investigated existing parsers? We have cmdline.c, gpio-aggregator.c, etc. Besides the fact of test cases which are absent here. And who knows what we allow to be entered.

Quoted 7 lines

Yes, I looked all around the kernel to find something I could reuse but failed to find anything useful for this particular purpose. If you have something you could point me towards, I'm open to alternatives.

Once we agree on the form of the module, I'll port self-tests to using it instead of gpio-mockup, so we'll have some tests in the tree.

Given the existing selftests focus on testing the gpio-mockup itself, it would be more appropriate that you add separate tests for gpio-sim.

As an end user I'm interested in the concrete example of driving gpio-sim that selftests would provide, so I'm looking forward to seeing that.

Cheers,
Kent.

Quoted 1 line · Andy Shevchenko

On Fri, Jan 29, 2021 at 02:46:24PM +0100, Bartosz Golaszewski wrote:

Quoted 14 lines

Review

Quoted 11 lines

(quoted diff) +10 -0

+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);

But aren't we got a parameter (1 or 0) from config? And hence

Quoted 5 lines

        case PIN_CONFIG_BIAS_PULL_UP:
        case PIN_CONFIG_BIAS_PULL_DOWN:
                return gpio_sim_apply_pull(chip, offset, <param>);

?

I believe this is more explicit and so easier to read if you don't know the GPIO and pinctrl internals.

Quoted 6 lines

Quoted 6 lines

(quoted diff) +6 -0

+     default:
+             break;
+     }
+
+     return -ENOTSUPP;
+}

Quoted 14 lines

Quoted 13 lines

(quoted diff) +12 -0

+static ssize_t gpio_sim_sysfs_line_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, val;
 
+     ret = kstrtoint(buf, 0, &val);
+     if (ret)
+             return ret;
+     if (val != 0 && val != 1)
+             return -EINVAL;

Quoted 2 lines

kstrtobool() ?

No, we really only want 0 or 1, no yes, Y etc.

Quoted 6 lines

Quoted 6 lines

(quoted diff) +6 -0

+     ret = gpio_sim_apply_pull(chip, line_attr->offset, val);
+     if (ret)
+             return ret;
+
+     return len;
+}

Quoted 12 lines

Review

Quoted 9 lines

(quoted diff) +9 -0

+struct gpio_sim_chip_config {
+     struct config_item item;
+
+     /*
+      * If pdev is NULL, the item is 'pending' (waiting for configuration).
+      * Once the pointer is assigned, the device has been created and the
+      * item is 'live'.
+      */
+     struct platform_device *pdev;

Are you sure

Quoted 4 lines

        struct device *dev;

is not sufficient?

It may be but I really prefer those simulated devices to be on the platform bus.

Quoted 13 lines

Quoted 13 lines

(quoted diff) +13 -0

+     /*
+      * Each configfs filesystem operation is protected with the subsystem
+      * mutex. Each separate attribute is protected with the buffer mutex.
+      * This structure however can be modified by callbacks of different
+      * attributes so we need another lock.
+      */
+     struct mutex lock;
+
+     char label[32];
+     unsigned int num_lines;
+     char **line_names;
+     unsigned int num_line_names;
+};

Quoted 6 lines

Honestly, I don't like the idea of Yet Another (custom) Parser in the kernel.

Have you investigated existing parsers? We have cmdline.c, gpio-aggregator.c, etc. Besides the fact of test cases which are absent here. And who knows what we allow to be entered.

Yes, I looked all around the kernel to find something I could reuse but failed to find anything useful for this particular purpose. If you have something you could point me towards, I'm open to alternatives.

Once we agree on the form of the module, I'll port self-tests to using it instead of gpio-mockup, so we'll have some tests in the tree.

Quoted 3 lines · Bartosz Golaszewski

Quoted 1 line · Andy Shevchenko

On Fri, Jan 29, 2021 at 02:46:24PM +0100, Bartosz Golaszewski wrote:

...

Quoted 18 lines

Quoted 18 lines

Review

Quoted 11 lines

(quoted diff) +10 -0

+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);

But aren't we got a parameter (1 or 0) from config? And hence

        case PIN_CONFIG_BIAS_PULL_UP:
        case PIN_CONFIG_BIAS_PULL_DOWN:
                return gpio_sim_apply_pull(chip, offset, <param>);

Quoted 4 lines

Quoted 1 line

?

I believe this is more explicit and so easier to read if you don't know the GPIO and pinctrl internals.

If we ever go to change meanings of the values in param, it will require to fix this occurrence which seems to me suboptimal.

Quoted 6 lines

Quoted 6 lines

Quoted 6 lines

(quoted diff) +6 -0

+     default:
+             break;
+     }
+
+     return -ENOTSUPP;
+}

...

Quoted 14 lines

Quoted 14 lines

Quoted 13 lines

(quoted diff) +12 -0

+static ssize_t gpio_sim_sysfs_line_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, val;
 
+     ret = kstrtoint(buf, 0, &val);
+     if (ret)
+             return ret;
+     if (val != 0 && val != 1)
+             return -EINVAL;

Quoted 4 lines

Quoted 2 lines

kstrtobool() ?

No, we really only want 0 or 1, no yes, Y etc.

Side note: But you allow 0x00001, for example...

Then why not to use unsigned type from the first place and add a comment?

Quoted 6 lines

Quoted 6 lines

Quoted 6 lines

(quoted diff) +6 -0

+     ret = gpio_sim_apply_pull(chip, line_attr->offset, val);
+     if (ret)
+             return ret;
+
+     return len;
+}

...

Quoted 17 lines

Quoted 16 lines

Review

Quoted 9 lines

(quoted diff) +9 -0

+struct gpio_sim_chip_config {
+     struct config_item item;
+
+     /*
+      * If pdev is NULL, the item is 'pending' (waiting for configuration).
+      * Once the pointer is assigned, the device has been created and the
+      * item is 'live'.
+      */
+     struct platform_device *pdev;

Are you sure

        struct device *dev;

is not sufficient?

Quoted 1 line

It may be but I really prefer those simulated devices to be on the platform bus.

My point here is that there is no need to keep specific bus devices type, because you may easily derive it from the struct device pointer. Basically if you are almost using struct device in your code (seems to me the case), you won't need to carry bus specific one and dereference it each time.

Quoted 13 lines

Quoted 13 lines

Quoted 13 lines

(quoted diff) +13 -0

+     /*
+      * Each configfs filesystem operation is protected with the subsystem
+      * mutex. Each separate attribute is protected with the buffer mutex.
+      * This structure however can be modified by callbacks of different
+      * attributes so we need another lock.
+      */
+     struct mutex lock;
+
+     char label[32];
+     unsigned int num_lines;
+     char **line_names;
+     unsigned int num_line_names;
+};

...

Quoted 7 lines

Quoted 6 lines

Honestly, I don't like the idea of Yet Another (custom) Parser in the kernel.

Have you investigated existing parsers? We have cmdline.c, gpio-aggregator.c, etc. Besides the fact of test cases which are absent here. And who knows what we allow to be entered.

Quoted 6 lines

Yes, I looked all around the kernel to find something I could reuse but failed to find anything useful for this particular purpose. If you have something you could point me towards, I'm open to alternatives.

Once we agree on the form of the module, I'll port self-tests to using it instead of gpio-mockup, so we'll have some tests in the tree.

I will look again when you send a new version, so I might give some hints.

--
With Best Regards,
Andy Shevchenko

Quoted 4 lines · Andy Shevchenko

Quoted 3 lines · Bartosz Golaszewski

Quoted 1 line · Andy Shevchenko

On Fri, Jan 29, 2021 at 02:46:24PM +0100, Bartosz Golaszewski wrote:

Quoted 18 lines

Quoted 18 lines

Quoted 18 lines

Review

Quoted 11 lines

(quoted diff) +10 -0

+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);

But aren't we got a parameter (1 or 0) from config? And hence

        case PIN_CONFIG_BIAS_PULL_UP:
        case PIN_CONFIG_BIAS_PULL_DOWN:
                return gpio_sim_apply_pull(chip, offset, <param>);

Quoted 8 lines

Quoted 4 lines

Quoted 1 line

?

I believe this is more explicit and so easier to read if you don't know the GPIO and pinctrl internals.

If we ever go to change meanings of the values in param, it will require to fix this occurrence which seems to me suboptimal.

Why would we do it? This is internal to this driver.

Quoted 6 lines

Quoted 6 lines

Quoted 6 lines

Review

Quoted 6 lines

(quoted diff) +6 -0

+     default:
+             break;
+     }
+
+     return -ENOTSUPP;
+}

Quoted 17 lines

Quoted 17 lines

Quoted 16 lines

Review

Quoted 13 lines

(quoted diff) +12 -0

+static ssize_t gpio_sim_sysfs_line_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, val;
 
+     ret = kstrtoint(buf, 0, &val);
+     if (ret)
+             return ret;
+     if (val != 0 && val != 1)
+             return -EINVAL;

kstrtobool() ?

Quoted 3 lines

Quoted 1 line

No, we really only want 0 or 1, no yes, Y etc.

Side note: But you allow 0x00001, for example...

Good point. In that case we should check if len > 2 and if buf[0] == '1' or '0' and that's all we allow.

Quoted 8 lines

Then why not to use unsigned type from the first place and add a comment?

Quoted 6 lines

Quoted 6 lines

Quoted 6 lines

(quoted diff) +6 -0

+     ret = gpio_sim_apply_pull(chip, line_attr->offset, val);
+     if (ret)
+             return ret;
+
+     return len;
+}

Quoted 17 lines

Quoted 17 lines

Quoted 16 lines

Review

Quoted 9 lines

(quoted diff) +9 -0

+struct gpio_sim_chip_config {
+     struct config_item item;
+
+     /*
+      * If pdev is NULL, the item is 'pending' (waiting for configuration).
+      * Once the pointer is assigned, the device has been created and the
+      * item is 'live'.
+      */
+     struct platform_device *pdev;

Are you sure

        struct device *dev;

is not sufficient?

Quoted 6 lines

Quoted 1 line

It may be but I really prefer those simulated devices to be on the platform bus.

My point here is that there is no need to keep specific bus devices type, because you may easily derive it from the struct device pointer. Basically if you are almost using struct device in your code (seems to me the case), you won't need to carry bus specific one and dereference it each time.

But don't we need a bus to even register a device? I haven't checked in a long time but IIRC it's mandatory.

Let me give you a different argument - the platform device offers a very simple API for registering devices with properties being duplicated behind the scenes etc. It seems to me that registering a bare struct device * would take more boiler-plate code for not much gain.

Quoted 6 lines · Bartosz Golaszewski

Quoted 4 lines · Andy Shevchenko

Quoted 3 lines · Bartosz Golaszewski

Quoted 1 line · Andy Shevchenko

On Fri, Jan 29, 2021 at 02:46:24PM +0100, Bartosz Golaszewski wrote:

...

Quoted 27 lines

Quoted 18 lines

Quoted 18 lines

Quoted 18 lines

Review

Quoted 11 lines

(quoted diff) +10 -0

+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);

But aren't we got a parameter (1 or 0) from config? And hence

        case PIN_CONFIG_BIAS_PULL_UP:
        case PIN_CONFIG_BIAS_PULL_DOWN:
                return gpio_sim_apply_pull(chip, offset, <param>);

Quoted 8 lines

Quoted 4 lines

Quoted 1 line

?

I believe this is more explicit and so easier to read if you don't know the GPIO and pinctrl internals.

If we ever go to change meanings of the values in param, it will require to fix this occurrence which seems to me suboptimal.

Quoted 8 lines

Why would we do it? This is internal to this driver.

Quoted 6 lines

Quoted 6 lines

Quoted 6 lines

Review

Quoted 6 lines

(quoted diff) +6 -0

+     default:
+             break;
+     }
+
+     return -ENOTSUPP;
+}

Up to you.
My personal vote for using the embedded param, because it makes code consistent and if anybody takes this driver as an example for something, it will be better example in such case..

...

Quoted 17 lines

Quoted 17 lines

Quoted 17 lines

Quoted 16 lines

Review

Quoted 13 lines

(quoted diff) +12 -0

+static ssize_t gpio_sim_sysfs_line_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, val;
 
+     ret = kstrtoint(buf, 0, &val);
+     if (ret)
+             return ret;
+     if (val != 0 && val != 1)
+             return -EINVAL;

kstrtobool() ?

Quoted 6 lines

Quoted 3 lines

Quoted 1 line

No, we really only want 0 or 1, no yes, Y etc.

Side note: But you allow 0x00001, for example...

Good point. In that case we should check if len > 2 and if buf[0] == '1' or '0' and that's all we allow.

Up to you also. I don't like such a strictness. OTOH we can relax afterwards if needed.

Quoted 8 lines

Quoted 8 lines

Then why not to use unsigned type from the first place and add a comment?

Quoted 6 lines

Quoted 6 lines

Quoted 6 lines

(quoted diff) +6 -0

+     ret = gpio_sim_apply_pull(chip, line_attr->offset, val);
+     if (ret)
+             return ret;
+
+     return len;
+}

...

Quoted 24 lines

Quoted 17 lines

Quoted 17 lines

Quoted 16 lines

Review

Quoted 9 lines

(quoted diff) +9 -0

+struct gpio_sim_chip_config {
+     struct config_item item;
+
+     /*
+      * If pdev is NULL, the item is 'pending' (waiting for configuration).
+      * Once the pointer is assigned, the device has been created and the
+      * item is 'live'.
+      */
+     struct platform_device *pdev;

Are you sure

        struct device *dev;

is not sufficient?

Quoted 6 lines

Quoted 1 line

It may be but I really prefer those simulated devices to be on the platform bus.

My point here is that there is no need to keep specific bus devices type, because you may easily derive it from the struct device pointer. Basically if you are almost using struct device in your code (seems to me the case), you won't need to carry bus specific one and dereference it each time.

Quoted 8 lines

But don't we need a bus to even register a device? I haven't checked in a long time but IIRC it's mandatory.

Let me give you a different argument - the platform device offers a very simple API for registering devices with properties being duplicated behind the scenes etc. It seems to me that registering a bare struct device * would take more boiler-plate code for not much gain.

Yes, I'm not objecting the platform bus choice. I'm objecting the keeping of the pointer to the bus specific structure.

There are helpers like to_platform_device() which make the bus specific pointers go away from the structures and easier code when you use exactly pointer to struct device rather than bus specific one.

Quoted 7 lines · Andy Shevchenko

Quoted 6 lines · Bartosz Golaszewski

Quoted 4 lines · Andy Shevchenko

Quoted 3 lines · Bartosz Golaszewski

Quoted 1 line · Andy Shevchenko

On Fri, Jan 29, 2021 at 02:46:24PM +0100, Bartosz Golaszewski wrote:

Quoted 33 lines

Quoted 24 lines

Quoted 17 lines

Quoted 17 lines

Quoted 16 lines

Review

Quoted 9 lines

(quoted diff) +9 -0

+struct gpio_sim_chip_config {
+     struct config_item item;
+
+     /*
+      * If pdev is NULL, the item is 'pending' (waiting for configuration).
+      * Once the pointer is assigned, the device has been created and the
+      * item is 'live'.
+      */
+     struct platform_device *pdev;

Are you sure

        struct device *dev;

is not sufficient?

Quoted 6 lines

Quoted 1 line

It may be but I really prefer those simulated devices to be on the platform bus.

My point here is that there is no need to keep specific bus devices type, because you may easily derive it from the struct device pointer. Basically if you are almost using struct device in your code (seems to me the case), you won't need to carry bus specific one and dereference it each time.

Quoted 8 lines

But don't we need a bus to even register a device? I haven't checked in a long time but IIRC it's mandatory.

Let me give you a different argument - the platform device offers a very simple API for registering devices with properties being duplicated behind the scenes etc. It seems to me that registering a bare struct device * would take more boiler-plate code for not much gain.

Quoted 7 lines

Yes, I'm not objecting the platform bus choice. I'm objecting the keeping of the pointer to the bus specific structure.

There are helpers like to_platform_device() which make the bus specific pointers go away from the structures and easier code when you use exactly pointer to struct device rather than bus specific one.

Ok I get it. We almost never dereference it though. We do it in probe, but there's no way around it. In sysfs callbacks we already get a pointer to struct device. And when unregistering the platform device, we need to pass it as struct platform_device anyway. I don't see any gain from that and would prefer to keep it as is.

From: Bartosz Golaszewski <[email protected]>

This series adds a new GPIO testing module based on configfs committable items and sysfs. The goal is to provide a testing driver that will be configurable at runtime (won't need module reload) and easily extensible. The control over the attributes is also much more fine-grained than in gpio-mockup.

I am aware that Uwe submitted a virtual driver called gpio-simulator some time ago and I was against merging it as it wasn't much different from gpio-mockup. I would ideally want to have a single testing driver to maintain so I am proposing this module as a replacement for gpio-mockup but since selftests and libgpiod depend on it and it also has users in the community, we can't outright remove it until everyone switched to the new interface. As for Uwe's idea for linking two simulated chips so that one controls the other - while I prefer to have an independent code path for controlling the lines (hence the sysfs attributes), I'm open to implementing it in this new driver. It should be much more feature friendly thanks to configfs than gpio-mockup.

Hello,

Quoted 7 lines · Bartosz Golaszewski

From: Bartosz Golaszewski <[email protected]>

This series adds a new GPIO testing module based on configfs committable items and sysfs. The goal is to provide a testing driver that will be configurable at runtime (won't need module reload) and easily extensible. The control over the attributes is also much more fine-grained than in gpio-mockup.

Quoted 10 lines

I am aware that Uwe submitted a virtual driver called gpio-simulator some time ago and I was against merging it as it wasn't much different from gpio-mockup. I would ideally want to have a single testing driver to maintain so I am proposing this module as a replacement for gpio-mockup but since selftests and libgpiod depend on it and it also has users in the community, we can't outright remove it until everyone switched to the new interface. As for Uwe's idea for linking two simulated chips so that one controls the other - while I prefer to have an independent code path for controlling the lines (hence the sysfs attributes), I'm open to implementing it in this new driver. It should be much more feature friendly thanks to configfs than gpio-mockup.

Funny you still think about my simulator driver. I recently thought

about reanimating it for my private use. The idea was to implement a rotary-encoder driver (that contrast to drivers/input/misc/rotary_encoder.c really implements an encoder and not a decoder). With the two linked chips I can plug drivers/input/misc/rotary_encoder.c on one side and my encoder on the other to test both drivers completely in software.

I didn't look into your driver yet, but getting such a driver into mainline would be very welcome!

I intend to look into your driver next week, but please don't hold back on merging for my feedback.

Best regards
Uwe

Quoted 22 lines · Uwe Kleine-König

Hello,

Quoted 7 lines · Bartosz Golaszewski

From: Bartosz Golaszewski <[email protected]>

This series adds a new GPIO testing module based on configfs committable items and sysfs. The goal is to provide a testing driver that will be configurable at runtime (won't need module reload) and easily extensible. The control over the attributes is also much more fine-grained than in gpio-mockup.

Quoted 10 lines

I am aware that Uwe submitted a virtual driver called gpio-simulator some time ago and I was against merging it as it wasn't much different from gpio-mockup. I would ideally want to have a single testing driver to maintain so I am proposing this module as a replacement for gpio-mockup but since selftests and libgpiod depend on it and it also has users in the community, we can't outright remove it until everyone switched to the new interface. As for Uwe's idea for linking two simulated chips so that one controls the other - while I prefer to have an independent code path for controlling the lines (hence the sysfs attributes), I'm open to implementing it in this new driver. It should be much more feature friendly thanks to configfs than gpio-mockup.

Quoted 1 line

Funny you still think about my simulator driver. I recently thought

It's because I always feel bad when I refuse to merge someone's hard work.

Quoted 7 lines

about reanimating it for my private use. The idea was to implement a rotary-encoder driver (that contrast to drivers/input/misc/rotary_encoder.c really implements an encoder and not a decoder). With the two linked chips I can plug drivers/input/misc/rotary_encoder.c on one side and my encoder on the other to test both drivers completely in software.

Quoted 3 lines

I didn't look into your driver yet, but getting such a driver into mainline would be very welcome!

My idea for linking chips (although that's not implemented yet) is an attribute in each configfs group called 'link' or something like that, that would take as argument the name of the chip to link to making the 'linker' the input and the 'linkee' the output.

Quoted 35 lines · Bartosz Golaszewski

Quoted 22 lines · Uwe Kleine-König

Hello,

Quoted 7 lines · Bartosz Golaszewski

From: Bartosz Golaszewski <[email protected]>

This series adds a new GPIO testing module based on configfs committable items and sysfs. The goal is to provide a testing driver that will be configurable at runtime (won't need module reload) and easily extensible. The control over the attributes is also much more fine-grained than in gpio-mockup.

Quoted 10 lines

I am aware that Uwe submitted a virtual driver called gpio-simulator some time ago and I was against merging it as it wasn't much different from gpio-mockup. I would ideally want to have a single testing driver to maintain so I am proposing this module as a replacement for gpio-mockup but since selftests and libgpiod depend on it and it also has users in the community, we can't outright remove it until everyone switched to the new interface. As for Uwe's idea for linking two simulated chips so that one controls the other - while I prefer to have an independent code path for controlling the lines (hence the sysfs attributes), I'm open to implementing it in this new driver. It should be much more feature friendly thanks to configfs than gpio-mockup.

Quoted 1 line

Funny you still think about my simulator driver. I recently thought

It's because I always feel bad when I refuse to merge someone's hard work.

Quoted 7 lines

about reanimating it for my private use. The idea was to implement a rotary-encoder driver (that contrast to drivers/input/misc/rotary_encoder.c really implements an encoder and not a decoder). With the two linked chips I can plug drivers/input/misc/rotary_encoder.c on one side and my encoder on the other to test both drivers completely in software.

Quoted 8 lines

Quoted 3 lines

I didn't look into your driver yet, but getting such a driver into mainline would be very welcome!

My idea for linking chips (although that's not implemented yet) is an attribute in each configfs group called 'link' or something like that, that would take as argument the name of the chip to link to making the 'linker' the input and the 'linkee' the output.

I still wonder why you prefer to drive the lines using configfs (or sysfs before). Using the idea of two interlinked chips and being able to use gpio functions on one side to modify the other side is (in my eyes) so simple and beautiful that it's obviously the right choice. But note I still didn't look into details so there might be stuff you can modify that wouldn't be possible with my idea. But obviously your mileage varies here.