linux-kernel - Re: [PATCH v12 4/7] gpio: sim: new testing module

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <Ya4Q7s9sbk2UHNA3@smile.fi.intel.com>
Date:   Mon, 6 Dec 2021 15:32:30 +0200
From:   Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
To:     Bartosz Golaszewski <brgl@...ev.pl>
Cc:     Kent Gibson <warthog618@...il.com>,
        Linus Walleij <linus.walleij@...aro.org>,
        Shuah Khan <shuah@...nel.org>,
        Geert Uytterhoeven <geert@...ux-m68k.org>,
        Viresh Kumar <viresh.kumar@...aro.org>,
        "open list:GPIO SUBSYSTEM" <linux-gpio@...r.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        linux-kselftest@...r.kernel.org
Subject: Re: [PATCH v12 4/7] gpio: sim: new testing module

On Mon, Dec 06, 2021 at 10:48:00AM +0100, Bartosz Golaszewski wrote:
> On Fri, Dec 3, 2021 at 9:08 PM Andy Shevchenko
> <andriy.shevchenko@...ux.intel.com> wrote:
> > On Fri, Dec 03, 2021 at 02:30:00PM +0100, Bartosz Golaszewski wrote:

...

> > > +#include <linux/gpio/driver.h>
> > > +#include <linux/gpio/machine.h>
> >
> > I would rather move this group below to emphasize that this is closer to GPIO
> > then to other APIs.
> >
> > > +#include <linux/sysfs.h>
> > > +
> >
> > ...here.
> >
> 
> With the number of headers in this file, I'd stick with alphabetical order.

I understand that and agree, but my point is orthogonal to this. The idea is to
emphasize that "hey. this driver has tough relations with the GPIO subsystem".
This is the way, for example, IIO does and I like it.

> > > +#include "gpiolib.h"

...

> > > +static int gpio_sim_apply_pull(struct gpio_sim_chip *chip,
> > > +                            unsigned int offset, int value)
> >
> > I would use up to 100 here...
> >
> > > +     if (test_bit(FLAG_REQUESTED, &desc->flags) &&
> > > +         !test_bit(FLAG_IS_OUT, &desc->flags)) {
> >
> > ...here and so on.
> >
> > But it's up to you.
> >
> 
> Nah, the lines are broken just fine. Let's not overuse the limit.

Yes, but I would consider to join back those which are up to ~83 characters
(I already pointed out at least to one example like this).

...

> > > +     if (sysfs_streq(buf, "pull-down"))
> > > +             pull = 0;
> > > +     else if (sysfs_streq(buf, "pull-up"))
> > > +             pull = 1;
> > > +     else
> > > +             return -EINVAL;
> >
> > sysfs_match_string() and use the very same string array in the above function
> > to print them?

I suppose you agree on this?

...

> > > +     /* Default to input mode. */
> > > +     bitmap_fill(chip->direction_map, num_lines);
> >
> > More accurate is to use bitmap_set(). If we ever debug this it also helpful.
> 
> I'm not sure what you mean, this sets all bits to 1.

Nope, it may set _more_ than all bits. That's why bitmap_set() is more
accurate, because it will do exact setting.

...

> > > +     if (strcmp(trimmed, "input") == 0)
> > > +             dir = GPIOD_IN;
> > > +     else if (strcmp(trimmed, "output-high") == 0)
> > > +             dir = GPIOD_OUT_HIGH;
> > > +     else if (strcmp(trimmed, "output-low") == 0)
> > > +             dir = GPIOD_OUT_LOW;
> > > +     else
> > > +             dir = -EINVAL;
> >
> > Same idea, i.e. static string array and use it above and here with help
> > of match_string().
> 
> It would be great but GPIOD_IN etc. are bit flags and not sequence enums.

Ah, okay, it will make rather sparse array.

-- 
With Best Regards,
Andy Shevchenko