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

[Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx>]

On Mon, Dec 06, 2021 at 04:38:44PM +0100, Bartosz Golaszewski wrote:
> 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:

...

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

And it's fine. It has remark about overlapping in case of readability and 81
characters on one line is fine as some cases when it is up to ~83 (for old
style).

Anyways, it doesn't worth of spending more time on this. Your choice then.

...

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

I haven't checked if it affects current code. Consider this as heads up,
because developers often forget about this nuance of bitmap_fill() /
bitmap_clear().

...

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

I thought about that, but since it's a mix of the bits, it might not be so
universal anyway, I would rather think of something which uses 1-bit bit
fields unified under a bit mask, and not a mix of 2 or more bits.

-- 
With Best Regards,
Andy Shevchenko