Re: [PATCH v2 09/12] gpio: sim: new testing module

[Bartosz Golaszewski]

On Thu, Mar 4, 2021 at 2:15 PM Andy Shevchenko
<andriy.shevchenko@xxxxxxxxxxxxxxx> wrote:
>
> On Thu, Mar 04, 2021 at 11:24:49AM +0100, Bartosz Golaszewski wrote:
> > From: Bartosz Golaszewski <bgolaszewski@xxxxxxxxxxxx>
> >
> > Implement a new, modern GPIO testing module controlled by configfs
> > attributes instead of module parameters. The goal of this driver is
> > to provide a replacement for gpio-mockup that will be easily extensible
> > with new features and doesn't require reloading the module to change
> > the setup.
>
> Shall we put a reference to this in the gpio-mockup documentation and mark the
> latter deprecated?
>

I don't think it's necessary right away. Let's phase out gpio-mockup
once this one gets some attention (for example: after libgpiod
switches to using it).

[snip]

>
> > +             dev_attr->attr.name = devm_kasprintf(dev, GFP_KERNEL,
> > +                                                  "gpio%u", i);
>
> Reads better as one line.
>

Yeah, so the removal of the 80 characters limit should not be abused
when there's no need for it - this doesn't look that bad really with a
broken line. Same elsewhere where the limit is exceeded.

[snip]

>
> > +             ret = sprintf(page + written,
> > +                     i < config->num_line_names - 1 ?
> > +                             "\"%s\", " : "\"%s\"\n",
> > +                     config->line_names[i] ?: "");
>
> Indentation here looks not the best...
>

So this is the place where it may make sense to go over 80 chars.

[snip]

> > +
> > +     /*
> > +      * FIXME If anyone knows a better way to parse that - please let me
> > +      * know.
> > +      */
>
> If comma can be replaced with ' ' (space) then why not to use next_arg() from
> cmdline.c? I.o.w. do you have strong opinion why should we use comma here?
>

My opinion is not very strong but I wanted to make the list of names
resemble what we pass to the gpio-line-names property in device tree.
Doesn't next_arg() react differently to string of the form: "foo=bar"?

[snip]

> > +
> > +static int gpio_sim_config_uncommit_item(struct config_item *item)
> > +{
> > +     struct gpio_sim_chip_config *config = to_gpio_sim_chip_config(item);
> > +     int id;
> > +
> > +     mutex_lock(&config->lock);
> > +     id = config->pdev->id;
> > +     platform_device_unregister(config->pdev);
> > +     config->pdev = NULL;
>
> > +     ida_free(&gpio_sim_ida, id);
>
> Isn't it atomic per se? I mean that IDA won't give the same ID until you free
> it. I.o.w. why is it under the mutex?
>

You're right but if we rapidly create and destroy chips we'll be left
with holes in the numbering (because new devices would be created
before the IDA numbers are freed, so the driver would take a larger
number that's currently free). It doesn't hurt but it would look worse
IMO. Do you have a strong opinion on this?

[snip]

I'll address issues I didn't comment on.

Thanks for the review!
Bart