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