On Mon, Feb 1, 2021 at 11:28 AM Andy Shevchenko
[off-list ref] wrote:

On Sat, Jan 30, 2021 at 09:37:55PM +0100, Bartosz Golaszewski wrote:

quoted

On Fri, Jan 29, 2021 at 4:57 PM Andy Shevchenko
[off-list ref] wrote:

quoted

On Fri, Jan 29, 2021 at 02:46:24PM +0100, Bartosz Golaszewski wrote:

...

quoted

quoted

quoted

+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)) {

quoted

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

quoted

quoted

quoted

+     default:
+             break;
+     }
+
+     return -ENOTSUPP;
+}

...

quoted

quoted

quoted

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

quoted

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

quoted

quoted

quoted

+     ret = gpio_sim_apply_pull(chip, line_attr->offset, val);
+     if (ret)
+             return ret;
+
+     return len;
+}

...

quoted

quoted

quoted

+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

quoted

quoted

quoted

+     /*
+      * 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;
+};

...

quoted

quoted

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