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

From: Bartosz Golaszewski
Date: Fri Aug 04 2023 - 12:04:25 EST


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