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

[Andy Shevchenko]

On Mon, Feb 01, 2021 at 11:59:31AM +0100, Bartosz Golaszewski wrote:
> 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;
> > > > > +}

Up to you.
My personal vote for using the embedded param, because it makes code consistent
and if anybody takes this driver as an example for something, it will be better
example in such case..

...

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

Up to you also. I don't like such a strictness. OTOH we can relax afterwards if
needed.

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

Yes, I'm not objecting the platform bus choice. I'm objecting the keeping of
the pointer to the bus specific structure.

There are helpers like to_platform_device() which make the bus specific
pointers go away from the structures and easier code when you use exactly
pointer to struct device rather than bus specific one.

Anyway, up to you.

-- 
With Best Regards,
Andy Shevchenko