Re: [RFC PATCH] gpio: consumer: new virtual driver

[Bartosz Golaszewski]

On Thu, Aug 3, 2023 at 1:39 PM Andy Shevchenko
<andriy.shevchenko@xxxxxxxxxxxxxxx> wrote:
>

[snip]

>
> > +#include <linux/of_platform.h>
>
> Wrong header. Use mod_devicetable.h.
>
> > +#include <linux/platform_device.h>
> > +#include <linux/printk.h>
> > +#include <linux/property.h>
> > +#include <linux/slab.h>
> > +#include <linux/string.h>
> > +#include <linux/timer.h>
>
> And general recommendation is to revisit this block and refine it accordingly.
>

I kept track of the interfaces I used for most part, so it should be
mostly fine.

[snip]

> ...
>
> > +     flags = function == GPIO_CONSUMER_FUNCTION_MONITOR ?
> > +                                     GPIOD_IN : GPIOD_OUT_HIGH;
> > +     for (i = 0; i < num_lines; i++) {
> > +             desc = devm_gpiod_get(dev, lines[i], flags);
> > +             if (IS_ERR(desc))
> > +                     return dev_err_probe(dev, PTR_ERR(desc),
> > +                                          "Failed to get GPIO '%s'\n",
> > +                                          lines[i]);
>
> Would it make sense to request GPIOs via devm_gpiod_get_array() and then try
> the rest on them in a loop?
>

No it would not. gpiod_get_array() works for properties represented in DT as:

    foo-gpios = <&chip ...>, <&chip ...>, <&chip ...>;

while what we have here is:

    foo-gpios = <&chip ...>;
    bar-gpios = <&chip ...>;

Which makes me think that I need to add proper documentation for this module.

[snip]

>
> > +static ssize_t
> > +gpio_consumer_lookup_config_offset_store(struct config_item *item,
> > +                                      const char *page, size_t count)
> > +{
> > +     struct gpio_consumer_lookup *lookup = to_gpio_consumer_lookup(item);
> > +     struct gpio_consumer_device *dev = lookup->parent;
> > +     int offset, ret;
> > +
> > +     ret = kstrtoint(page, 0, &offset);
> > +     if (ret)
> > +             return ret;
> > +
> > +     /* Use -1 to indicate lookup by name. */
> > +     if (offset > (U16_MAX - 1))
> > +             return -EINVAL;
>
> So, offset here may be negative. Is it okay?
>

Yes. If negative - lookup line by name, if positive, by chip and
offset. I will document this properly for v2.

> > +     mutex_lock(&dev->lock);
> > +
> > +     if (gpio_consumer_device_is_live_unlocked(dev)) {
> > +             mutex_unlock(&dev->lock);
> > +             return -EBUSY;
> > +     }
> > +
> > +     lookup->offset = offset;
> > +
> > +     mutex_unlock(&dev->lock);
> > +
> > +     return count;
> > +}
>
> ...
>
> > +     if (flags & GPIO_OPEN_DRAIN)
> > +             repr = "open-drain";
> > +     else if (flags & GPIO_OPEN_SOURCE)
> > +             repr = "open-source";
>
> Can it be both flags set?
>

No!

> > +     else
> > +             repr = "push-pull";
>
> ...
>
> > +     if (sysfs_streq(page, "push-pull")) {
> > +             lookup->flags &= ~(GPIO_OPEN_DRAIN | GPIO_OPEN_SOURCE);
> > +     } else if (sysfs_streq(page, "open-drain")) {
> > +             lookup->flags &= ~GPIO_OPEN_SOURCE;
> > +             lookup->flags |= GPIO_OPEN_DRAIN;
> > +     } else if (sysfs_streq(page, "open-source")) {
> > +             lookup->flags &= ~GPIO_OPEN_DRAIN;
> > +             lookup->flags |= GPIO_OPEN_SOURCE;
> > +     } else {
> > +             count = -EINVAL;
> > +     }
>
> I prefer to see some kind of the array of constant string literals and do
> sysfs_match_string() here
>

I would generally agree but if the flag values ever change to ones
that make the resulting string array have holes in it, match_string()
will suddenly stop working. I think that with bit flags defined
elsewhere it's safer and more readable to do the above.

>         lookup->flags &= ~(GPIO_OPEN_DRAIN | GPIO_OPEN_SOURCE);
>         flag = sysfs_match_string(...);
>         if (flag < 0)
>                 count = flag
>         else
>                 lookup->flags |= flag;
>
> (or something similar). And respectively indexed access above.
>
> ...
>

> ...
>
> > +     if (list_empty(&dev->lookup_list))
> > +             return -ENODATA;
>
> Instead you may count nodes here and if 0, return an error, otherwise pass it
> to the callee.

I'm not following, please rephrase.

>
> > +     swnode = gpio_consumer_make_device_swnode(dev);
> > +     if (IS_ERR(swnode))
> > +             return PTR_ERR(swnode);
>
> ...
>
> > +static ssize_t
> > +gpio_consumer_device_config_live_store(struct config_item *item,
> > +                                    const char *page, size_t count)
> > +{
> > +     struct gpio_consumer_device *dev = to_gpio_consumer_device(item);
> > +     bool live;
> > +     int ret;
> > +
> > +     ret = kstrtobool(page, &live);
> > +     if (ret)
> > +             return ret;
> > +
> > +     mutex_lock(&dev->lock);
> > +
> > +     if ((!live && !gpio_consumer_device_is_live_unlocked(dev)) ||
> > +         (live && gpio_consumer_device_is_live_unlocked(dev)))
>
>         if (live ^ gpio_consumer_device_is_live_unlocked(dev))
>
> ?

Nah, let's not use bitwise operators for boolean logic.

[snip]

I commented on the ones that needed it, for others, I'll fix them for v2.

Bart