Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20230802152808.33037-1-brgl@bgdev.pl> <ZMuR0W303WCbS1K0@smile.fi.intel.com>
In-Reply-To: <ZMuR0W303WCbS1K0@smile.fi.intel.com>
From:   Bartosz Golaszewski <brgl@bgdev.pl>
Date:   Fri, 4 Aug 2023 18:03:47 +0200
Message-ID: <CAMRc=McEAG7Ezgb=OwMPoRhQzu_A66JMnB=aBSgmdZUvS-ZPhw@mail.gmail.com>
Subject: Re: [RFC PATCH] gpio: consumer: new virtual driver
To:     Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Cc:     Linus Walleij <linus.walleij@linaro.org>,
        Kent Gibson <warthog618@gmail.com>,
        Viresh Kumar <viresh.kumar@linaro.org>,
        Geert Uytterhoeven <geert@linux-m68k.org>,
        linux-gpio@vger.kernel.org, linux-kernel@vger.kernel.org,
        Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Thu, Aug 3, 2023 at 1:39=E2=80=AFPM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> 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 accordi=
ngly.
>

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

[snip]

> ...
>
> > +     flags =3D function =3D=3D GPIO_CONSUMER_FUNCTION_MONITOR ?
> > +                                     GPIOD_IN : GPIOD_OUT_HIGH;
> > +     for (i =3D 0; i < num_lines; i++) {
> > +             desc =3D 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 a=
s:

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

while what we have here is:

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

Which makes me think that I need to add proper documentation for this modul=
e.

[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 =3D to_gpio_consumer_lookup(i=
tem);
> > +     struct gpio_consumer_device *dev =3D lookup->parent;
> > +     int offset, ret;
> > +
> > +     ret =3D 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 =3D offset;
> > +
> > +     mutex_unlock(&dev->lock);
> > +
> > +     return count;
> > +}
>
> ...
>
> > +     if (flags & GPIO_OPEN_DRAIN)
> > +             repr =3D "open-drain";
> > +     else if (flags & GPIO_OPEN_SOURCE)
> > +             repr =3D "open-source";
>
> Can it be both flags set?
>

No!

> > +     else
> > +             repr =3D "push-pull";
>
> ...
>
> > +     if (sysfs_streq(page, "push-pull")) {
> > +             lookup->flags &=3D ~(GPIO_OPEN_DRAIN | GPIO_OPEN_SOURCE);
> > +     } else if (sysfs_streq(page, "open-drain")) {
> > +             lookup->flags &=3D ~GPIO_OPEN_SOURCE;
> > +             lookup->flags |=3D GPIO_OPEN_DRAIN;
> > +     } else if (sysfs_streq(page, "open-source")) {
> > +             lookup->flags &=3D ~GPIO_OPEN_DRAIN;
> > +             lookup->flags |=3D GPIO_OPEN_SOURCE;
> > +     } else {
> > +             count =3D -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 &=3D ~(GPIO_OPEN_DRAIN | GPIO_OPEN_SOURCE);
>         flag =3D sysfs_match_string(...);
>         if (flag < 0)
>                 count =3D flag
>         else
>                 lookup->flags |=3D 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 pas=
s it
> to the callee.

I'm not following, please rephrase.

>
> > +     swnode =3D 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 =3D to_gpio_consumer_device(item=
);
> > +     bool live;
> > +     int ret;
> > +
> > +     ret =3D 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