Re: [PATCH 8/8] gpio: sim: new testing module

[Bartosz Golaszewski]

On Mon, Feb 1, 2021 at 11:28 AM Andy Shevchenko
<andriy.shevchenko@xxxxxxxxxxxxxxx> wrote:
>
> On Sat, Jan 30, 2021 at 09:37:55PM +0100, Bartosz Golaszewski wrote:
> > On Fri, Jan 29, 2021 at 4:57 PM Andy Shevchenko
> > <andriy.shevchenko@xxxxxxxxxxxxxxx> wrote:
> > > On Fri, Jan 29, 2021 at 02:46:24PM +0100, Bartosz Golaszewski wrote:
>
> ...
>
> > > > +static int gpio_sim_set_config(struct gpio_chip *gc,
> > > > +                               unsigned int offset, unsigned long config)
> > > > +{
> > > > +     struct gpio_sim_chip *chip = gpiochip_get_data(gc);
> > > > +
> > > > +     switch (pinconf_to_config_param(config)) {
> > >
> > > > +     case PIN_CONFIG_BIAS_PULL_UP:
> > > > +             return gpio_sim_apply_pull(chip, offset, 1);
> > > > +     case PIN_CONFIG_BIAS_PULL_DOWN:
> > > > +             return gpio_sim_apply_pull(chip, offset, 0);
> > >
> > > But aren't we got a parameter (1 or 0) from config? And hence
> > >
> > >         case PIN_CONFIG_BIAS_PULL_UP:
> > >         case PIN_CONFIG_BIAS_PULL_DOWN:
> > >                 return gpio_sim_apply_pull(chip, offset, <param>);
> > >
> > > ?
> >
> > I believe this is more explicit and so easier to read if you don't
> > know the GPIO and pinctrl internals.
>
>
> If we ever go to change meanings of the values in param, it will require to fix
> this occurrence which seems to me suboptimal.
>

Why would we do it? This is internal to this driver.

> > > > +     default:
> > > > +             break;
> > > > +     }
> > > > +
> > > > +     return -ENOTSUPP;
> > > > +}
>
> ...
>
> > > > +static ssize_t gpio_sim_sysfs_line_store(struct device *dev,
> > > > +                                      struct device_attribute *attr,
> > > > +                                      const char *buf, size_t len)
> > > > +{
> > > > +     struct gpio_sim_attribute *line_attr = to_gpio_sim_attr(attr);
> > > > +     struct gpio_sim_chip *chip = dev_get_drvdata(dev);
> > > > +     int ret, val;
> > >
> > > > +     ret = kstrtoint(buf, 0, &val);
> > > > +     if (ret)
> > > > +             return ret;
> > > > +     if (val != 0 && val != 1)
> > > > +             return -EINVAL;
> > >
> > > kstrtobool() ?
> > >
> >
> > No, we really only want 0 or 1, no yes, Y etc.
>
> Side note: But you allow 0x00001, for example...

Good point. In that case we should check if len > 2 and if buf[0] ==
'1' or '0' and that's all we allow.

>
> Then why not to use unsigned type from the first place and add a comment?
>
> > > > +     ret = gpio_sim_apply_pull(chip, line_attr->offset, val);
> > > > +     if (ret)
> > > > +             return ret;
> > > > +
> > > > +     return len;
> > > > +}
>
> ...
>
> > > > +struct gpio_sim_chip_config {
> > > > +     struct config_item item;
> > > > +
> > > > +     /*
> > > > +      * If pdev is NULL, the item is 'pending' (waiting for configuration).
> > > > +      * Once the pointer is assigned, the device has been created and the
> > > > +      * item is 'live'.
> > > > +      */
> > > > +     struct platform_device *pdev;
> > >
> > > Are you sure
> > >
> > >         struct device *dev;
> > >
> > > is not sufficient?
> > >
> >
> > It may be but I really prefer those simulated devices to be on the platform bus.
>
> My point here is that there is no need to keep specific bus devices type,
> because you may easily derive it from the struct device pointer. Basically if
> you are almost using struct device in your code (seems to me the case), you
> won't need to carry bus specific one and dereference it each time.
>

But don't we need a bus to even register a device? I haven't checked
in a long time but IIRC it's mandatory.

Let me give you a different argument - the platform device offers a
very simple API for registering devices with properties being
duplicated behind the scenes etc. It seems to me that registering a
bare struct device * would take more boiler-plate code for not much
gain.

Bartosz

> > > > +     /*
> > > > +      * Each configfs filesystem operation is protected with the subsystem
> > > > +      * mutex. Each separate attribute is protected with the buffer mutex.
> > > > +      * This structure however can be modified by callbacks of different
> > > > +      * attributes so we need another lock.
> > > > +      */
> > > > +     struct mutex lock;
> > > > +
> > > > +     char label[32];
> > > > +     unsigned int num_lines;
> > > > +     char **line_names;
> > > > +     unsigned int num_line_names;
> > > > +};
>
> ...
>
> > > Honestly, I don't like the idea of Yet Another (custom) Parser in the kernel.
> > >
> > > Have you investigated existing parsers? We have cmdline.c, gpio-aggregator.c,
> > > etc. Besides the fact of test cases which are absent here. And who knows what
> > > we allow to be entered.
> > >
> >
> > Yes, I looked all around the kernel to find something I could reuse
> > but failed to find anything useful for this particular purpose. If you
> > have something you could point me towards, I'm open to alternatives.
> >
> > Once we agree on the form of the module, I'll port self-tests to using
> > it instead of gpio-mockup, so we'll have some tests in the tree.
>
> I will look again when you send a new version, so I might give some hints.
>
> --
> With Best Regards,
> Andy Shevchenko
>
>