Re: [PATCH 5/8] lib: bitmap: remove the 'extern' keyword from function declarations

Re: [PATCH 5/8] lib: bitmap: remove the 'extern' keyword from function declarations

[Bartosz Golaszewski]

On Fri, Jan 29, 2021 at 4:59 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> On Fri, Jan 29, 2021 at 02:46:21PM +0100, Bartosz Golaszewski wrote:
> > From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> >
> > The 'extern' keyword doesn't have any benefits in header files. Remove it.
>
> > +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.
>

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.

Bart

Re: [PATCH 8/8] gpio: sim: new testing module

[Bartosz Golaszewski]

On Fri, Jan 29, 2021 at 4:57 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> On Fri, Jan 29, 2021 at 02:46:24PM +0100, Bartosz Golaszewski wrote:
> > From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> >
> > 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.
>
> ...
>
> > +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).
>
> > +               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.
>
> ...
>
> > +#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>
>
> ...
>
> > +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;
> > +     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)
>
> ?
>
> > +             /*
> > +              * This is fine - it just means, nobody is listening
> > +              * for interrupts on this line, otherwise
> > +              * irq_create_mapping() would have been called from
> > +              * 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 ?
>
> > +                     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;
> > +}
>
> ...
>
> > +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?
>
> > +     mutex_unlock(&chip->lock);
> > +
> > +     return 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.
>
> > +     mutex_unlock(&chip->lock);
> > +}
>
> ...
>
> > +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>);
>
> ?

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

>
> > +     default:
> > +             break;
> > +     }
> > +
> > +     return -ENOTSUPP;
> > +}
>
> ...
>
> > +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() ?
>

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

> > +     ret = gpio_sim_apply_pull(chip, line_attr->offset, val);
> > +     if (ret)
> > +             return ret;
> > +
> > +     return len;
> > +}
>
> ...
>
> > +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) ?
>
> > +                          num_lines + 1, GFP_KERNEL);
>
> and hence one line?
>
> > +     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);
> > +}
>
> ...
>
> > +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);
> > +     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);
>
> ?
>
> > +     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;
> > +
> > +     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?
>
> > +     return 0;
> > +}
>
> ...
>
> > +static const struct of_device_id gpio_sim_of_match[] = {
> > +     { .compatible = "gpio-simulator" },
> > +     { },
>
> No comma needed.
>
> > +};
>
> ...
>
> > +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?
>

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

> > +     /*
> > +      * 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;
> > +};
>
> ...
>
> > +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;
> > +     }
>
> > +     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().
>

Except that we have a buffer already allocated that holds 32
characters and we just put the string there. The buffer corresponds
with the one the character device uses. Trimming whitespace is just so
that it doesn't use up the few characters we have. On the other hand -
since the kernel doesn't really limit the label to 32 chars (only uAPI
does) maybe I'll address this somehow.

> > +     if (ret < 0) {
> > +             mutex_unlock(&config->lock);
> > +             return ret;
> > +     }
> > +
> > +     mutex_unlock(&config->lock);
> > +     return count;
> > +}
>
> ...
>
> > +     for (i = 0; i < config->num_line_names; i++) {
> > +             ret = sprintf(page + written,
> > +                     i < config->num_line_names - 1 ?
> > +                             "\"%s\", " : "\"%s\"\n",
> > +                     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?
>

Indeed! Nice catch.

>
> ...
>
> 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.

> > +     /*
> > +      * 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"'.
> > +      */
>
> ...
>
> > +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.
>
> > +};
>
> ...
>
> > +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
>
>

I'll address all issues I didn't comment on in v2. Thanks for the
thorough (as always) review!

Bartosz

Re: [PATCH 0/8] gpio: implement the configfs testing module

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 2062 bytes --]

Hello,

On Fri, Jan 29, 2021 at 02:46:16PM +0100, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> 
> 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.

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

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 8/8] gpio: sim: new testing module

[Kent Gibson]

On Sat, Jan 30, 2021 at 09:37:55PM +0100, Bartosz Golaszewski wrote:
> On Fri, Jan 29, 2021 at 4:57 PM Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
> >
> > On Fri, Jan 29, 2021 at 02:46:24PM +0100, Bartosz Golaszewski wrote:
> > > From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> > ...
> >

[snip]

> > 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.
> 

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.

Re: [PATCH 0/8] gpio: implement the configfs testing module

[Bartosz Golaszewski]

On Sat, Jan 30, 2021 at 10:20 PM Uwe Kleine-König
<u.kleine-koenig@pengutronix.de> wrote:
>
> Hello,
>
> On Fri, Jan 29, 2021 at 02:46:16PM +0100, Bartosz Golaszewski wrote:
> > From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> >
> > 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.
>
> 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.

> 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!
>

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.

It would be tempting to use symbolic links too but I'm afraid this
would need further extension of configfs.

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

Don't worry, I'm not really aiming at v5.12 with this.

> Best regards
> Uwe
>

Bart

Re: [PATCH 8/8] gpio: sim: new testing module

[Bartosz Golaszewski]

On Sun, Jan 31, 2021 at 1:43 AM Kent Gibson <warthog618@gmail.com> wrote:
>
> On Sat, Jan 30, 2021 at 09:37:55PM +0100, Bartosz Golaszewski wrote:
> > On Fri, Jan 29, 2021 at 4:57 PM Andy Shevchenko
> > <andriy.shevchenko@linux.intel.com> wrote:
> > >
> > > On Fri, Jan 29, 2021 at 02:46:24PM +0100, Bartosz Golaszewski wrote:
> > > > From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> > > ...
> > >
>
> [snip]
>
> > > 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.
> >
>
> 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.

Makes sense, I'll add tests in v2.

Bartosz

Re: [PATCH 0/8] gpio: implement the configfs testing module

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 3060 bytes --]

On Mon, Feb 01, 2021 at 09:37:30AM +0100, Bartosz Golaszewski wrote:
> On Sat, Jan 30, 2021 at 10:20 PM Uwe Kleine-König
> <u.kleine-koenig@pengutronix.de> wrote:
> >
> > Hello,
> >
> > On Fri, Jan 29, 2021 at 02:46:16PM +0100, Bartosz Golaszewski wrote:
> > > From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> > >
> > > 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.
> >
> > 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.
> 
> > 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!
> >
> 
> 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.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 5/8] lib: bitmap: remove the 'extern' keyword from function declarations

[Andy Shevchenko]

On Sat, Jan 30, 2021 at 09:25:08PM +0100, Bartosz Golaszewski wrote:
> On Fri, Jan 29, 2021 at 4:59 PM Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
> >
> > On Fri, Jan 29, 2021 at 02:46:21PM +0100, Bartosz Golaszewski wrote:
> > > From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> > >
> > > The 'extern' keyword doesn't have any benefits in header files. Remove it.
> >
> > > +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.
> >
> 
> 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.

In above case it's even logically better to split as I proposed.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 8/8] gpio: sim: new testing module

[Andy Shevchenko]

On Sat, Jan 30, 2021 at 09:37:55PM +0100, Bartosz Golaszewski wrote:
> On Fri, Jan 29, 2021 at 4:57 PM Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
> > On Fri, Jan 29, 2021 at 02:46:24PM +0100, Bartosz Golaszewski wrote:

...

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

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

...

> > > +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() ?
> >
> 
> 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?

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

...

> > > +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?
> >
> 
> 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.

> > > +     /*
> > > +      * 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;
> > > +};

...

> > 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.

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

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 8/8] gpio: sim: new testing module

[Bartosz Golaszewski]

On Mon, Feb 1, 2021 at 11:28 AM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> On Sat, Jan 30, 2021 at 09:37:55PM +0100, Bartosz Golaszewski wrote:
> > On Fri, Jan 29, 2021 at 4:57 PM Andy Shevchenko
> > <andriy.shevchenko@linux.intel.com> wrote:
> > > On Fri, Jan 29, 2021 at 02:46:24PM +0100, Bartosz Golaszewski wrote:
>
> ...
>
> > > > +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>);
> > >
> > > ?
> >
> > 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.

> > > > +     default:
> > > > +             break;
> > > > +     }
> > > > +
> > > > +     return -ENOTSUPP;
> > > > +}
>
> ...
>
> > > > +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() ?
> > >
> >
> > 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.

>
> Then why not to use unsigned type from the first place and add a comment?
>
> > > > +     ret = gpio_sim_apply_pull(chip, line_attr->offset, val);
> > > > +     if (ret)
> > > > +             return ret;
> > > > +
> > > > +     return len;
> > > > +}
>
> ...
>
> > > > +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?
> > >
> >
> > 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.

Bartosz

> > > > +     /*
> > > > +      * 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;
> > > > +};
>
> ...
>
> > > 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.
>
> I will look again when you send a new version, so I might give some hints.
>
> --
> With Best Regards,
> Andy Shevchenko
>
>

Re: [PATCH 8/8] gpio: sim: new testing module

[Andy Shevchenko]

On Mon, Feb 01, 2021 at 11:59:31AM +0100, Bartosz Golaszewski wrote:
> On Mon, Feb 1, 2021 at 11:28 AM Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
> > On Sat, Jan 30, 2021 at 09:37:55PM +0100, Bartosz Golaszewski wrote:
> > > On Fri, Jan 29, 2021 at 4:57 PM Andy Shevchenko
> > > <andriy.shevchenko@linux.intel.com> wrote:
> > > > On Fri, Jan 29, 2021 at 02:46:24PM +0100, Bartosz Golaszewski wrote:

...

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

...

> > > > > +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() ?
> > > >
> > >
> > > 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.

> > Then why not to use unsigned type from the first place and add a comment?
> >
> > > > > +     ret = gpio_sim_apply_pull(chip, line_attr->offset, val);
> > > > > +     if (ret)
> > > > > +             return ret;
> > > > > +
> > > > > +     return len;
> > > > > +}

...

> > > > > +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?
> > > >
> > >
> > > 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.

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.

Anyway, up to you.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 0/8] gpio: implement the configfs testing module

[Bartosz Golaszewski]

On Mon, Feb 1, 2021 at 10:24 AM Uwe Kleine-König
<u.kleine-koenig@pengutronix.de> wrote:
>
> On Mon, Feb 01, 2021 at 09:37:30AM +0100, Bartosz Golaszewski wrote:
> > On Sat, Jan 30, 2021 at 10:20 PM Uwe Kleine-König
> > <u.kleine-koenig@pengutronix.de> wrote:
> > >
> > > Hello,
> > >
> > > On Fri, Jan 29, 2021 at 02:46:16PM +0100, Bartosz Golaszewski wrote:
> > > > From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> > > >
> > > > 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.
> > >
> > > 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.
> >
> > > 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!
> > >
> >
> > 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.
>

Not only drive but also check the input mode using a different code
path. My thinking is this: if, for example, we're checking the input
mode, let's not involve the core gpiolib's output code from a
different chip. Let's try to isolate the specific use-cases. Keep in
mind that my particular use-case is testing the uAPI with libgpiod's
test suite.

Also: previously it was debugfs, now we're switching to configs (for
configuring the devices) and sysfs (for controlling them).

Bart

Re: [PATCH 8/8] gpio: sim: new testing module

[Bartosz Golaszewski]

On Mon, Feb 1, 2021 at 1:49 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> On Mon, Feb 01, 2021 at 11:59:31AM +0100, Bartosz Golaszewski wrote:
> > On Mon, Feb 1, 2021 at 11:28 AM Andy Shevchenko
> > <andriy.shevchenko@linux.intel.com> wrote:
> > > On Sat, Jan 30, 2021 at 09:37:55PM +0100, Bartosz Golaszewski wrote:
> > > > On Fri, Jan 29, 2021 at 4:57 PM Andy Shevchenko
> > > > <andriy.shevchenko@linux.intel.com> wrote:
> > > > > On Fri, Jan 29, 2021 at 02:46:24PM +0100, Bartosz Golaszewski wrote:
>
> ...
>
> > > > > > +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>);
> > > > >
> > > > > ?
> > > >
> > > > 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.
> >
> > > > > > +     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..
>
> ...
>
> > > > > > +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() ?
> > > > >
> > > >
> > > > 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.
>
> > > Then why not to use unsigned type from the first place and add a comment?
> > >
> > > > > > +     ret = gpio_sim_apply_pull(chip, line_attr->offset, val);
> > > > > > +     if (ret)
> > > > > > +             return ret;
> > > > > > +
> > > > > > +     return len;
> > > > > > +}
>
> ...
>
> > > > > > +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?
> > > > >
> > > >
> > > > 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.
>
> 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.

Bart

Re: [PATCH 8/8] gpio: sim: new testing module

[Andy Shevchenko]

On Mon, Feb 01, 2021 at 01:53:16PM +0100, Bartosz Golaszewski wrote:
> On Mon, Feb 1, 2021 at 1:49 PM Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
> > On Mon, Feb 01, 2021 at 11:59:31AM +0100, Bartosz Golaszewski wrote:
> > > On Mon, Feb 1, 2021 at 11:28 AM Andy Shevchenko
> > > <andriy.shevchenko@linux.intel.com> wrote:
> > > > On Sat, Jan 30, 2021 at 09:37:55PM +0100, Bartosz Golaszewski wrote:
> > > > > On Fri, Jan 29, 2021 at 4:57 PM Andy Shevchenko
> > > > > <andriy.shevchenko@linux.intel.com> wrote:
> > > > > > On Fri, Jan 29, 2021 at 02:46:24PM +0100, Bartosz Golaszewski wrote:

...

> > > > > > > +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?
> > > > > >
> > > > >
> > > > > 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.
> >
> > 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.

It's purely trade off, if you have more *dev in use, better to use *dev, if
*pdev, then use it, although my practice shows that in most cases keeping bus
specific pointer is an overkill.

-- 
With Best Regards,
Andy Shevchenko