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

[Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx>]

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.

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