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

[Bartosz Golaszewski <brgl@xxxxxxxx>]

On Mon, Dec 6, 2021 at 2:33 PM Andy Shevchenko
<andriy.shevchenko@xxxxxxxxxxxxxxx> wrote:
>
> 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@xxxxxxxxxxxxxxx> 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.
>

I really don't think this is necessary.

> > > > +#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).
>

I like the old-style limit TBH.

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

Yes, already changed that in v13.

> ...
>
> > > > +     /* 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.
>

Can this in any way affect any of the code? If the driver is correct,
it will never use anything beyond the last line bit. If it does, it
needs fixing. It's as if we cared about what happens to padding added
to structures by the compiler (as long as we're not passing it to
user-space of course).

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

Idea for the future: introduce match_string_ext() with flags one of
which would allow sparse string arrays?

Bart