On Fri, Dec 3, 2021 at 9:08 PM Andy Shevchenko andriy.shevchenko@linux.intel.com wrote:
On Fri, Dec 03, 2021 at 02:30:00PM +0100, Bartosz Golaszewski wrote:
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.
...
+**Group:** ``/config/gpio-sim/gpio-device``
+**Attribute:** ``/config/gpio-sim/gpio-device/dev_name``
+**Attribute:** ``/config/gpio-sim/gpio-device/live``
+This is a directory representing a GPIO platform device. The ``'dev_name'`` +attribute is read-only and allows the user-space to read the platform device +name (e.g. ``'gpio-sim.0'``). The ``'live'`` attribute allows to trigger the +actual creation of the device once it's fully configured. The accepted values +are: ``'1'`` to enable the simulated device and ``'0'`` to disable and tear +it down.
Perhaps it makes sense to describe properties in the order you expect to be used, then it will be naturally to 'read and repeat' without jumping forward and backward through the documentation.
This is such order though. You naturally configure the device, then bank, then lines, then hogs.
...
+**Group:** ``/config/gpio-sim/gpio-device/gpio-bankX``
+**Attribute:** ``/config/gpio-sim/gpio-device/gpio-bankX/chip_name``
+**Attribute:** ``/config/gpio-sim/gpio-device/gpio-bankX/num_lines``
Why not to use the same name as in DT, i.e. ngpios?
This would be the only attribute that follows the DT naming, the label, line names etc. wouldn't use the same name anyway. I don't see any advantage of it as num_lines is actually more intuitive than ngpios IMO.
...
+#include <linux/gpio/driver.h> +#include <linux/gpio/machine.h>
I would rather move this group below to emphasize that this is closer to GPIO then to other APIs.
+#include <linux/sysfs.h>
...here.
With the number of headers in this file, I'd stick with alphabetical order.
+#include "gpiolib.h"
...
+static int gpio_sim_apply_pull(struct gpio_sim_chip *chip,
unsigned int offset, int value)I would use up to 100 here...
if (test_bit(FLAG_REQUESTED, &desc->flags) &&!test_bit(FLAG_IS_OUT, &desc->flags)) {...here and so on.
But it's up to you.
Nah, the lines are broken just fine. Let's not overuse the limit.
...
curr_val = !!test_bit(offset, chip->value_map);if (curr_val == value)Do you use curr_val anywhere else? Perhaps combine these two lines.
goto set_pull;
Good point.
...
+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);default:
break;}return -ENOTSUPP;return directly from switch-case?
This may be a personal preference but I don't like returning functions without a return at the end. Always looks wrong at first glance. I'd like to keep it.
+}
...
+static ssize_t gpio_sim_sysfs_pull_show(struct device *dev,
struct device_attribute *attr,char *buf)+{
struct gpio_sim_attribute *line_attr = to_gpio_sim_attr(attr);struct gpio_sim_chip *chip = dev_get_drvdata(dev);char *repr;int pull;int pull_up;? Also, where is "pull-none"?
There isn't. If it's ever needed, we can extend the driver but so far there hasn't been a need for it when testing from user-space.
mutex_lock(&chip->lock);pull = !!test_bit(line_attr->offset, chip->pull_map);mutex_unlock(&chip->lock);
if (pull)repr = "pull-up";elserepr = "pull-down";return sysfs_emit(buf, "%s\n", repr);return sysfs_emit(buf, "%pull-s\n", pull_up ? "up" : "down");?
Sure.
+}
...
+static ssize_t gpio_sim_sysfs_pull_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, pull;if (sysfs_streq(buf, "pull-down"))pull = 0;else if (sysfs_streq(buf, "pull-up"))pull = 1;elsereturn -EINVAL;sysfs_match_string() and use the very same string array in the above function to print them?
Same question about "pull-none".
ret = gpio_sim_apply_pull(chip, line_attr->offset, pull);if (ret)return ret;return len;+}
...
attr_group->name = devm_kasprintf(dev, GFP_KERNEL,"sim_gpio%u", i);Wondering if you can use devm_kasprintf_strarray().
I would need to do that in a separate loop and handle the new array, I think it's an overkill here.
if (!attr_group->name)return -ENOMEM;...
/* Default to input mode. */bitmap_fill(chip->direction_map, num_lines);More accurate is to use bitmap_set(). If we ever debug this it also helpful.
I'm not sure what you mean, this sets all bits to 1.
...
ret = devm_add_action_or_reset(dev, gpio_sim_mutex_destroy,&chip->lock);It's 81, fine to have on one line.
if (ret)return ret;...
+static char *gpio_sim_strdup_trimmed(const char *str, size_t count) +{
char *dup, *trimmed, *ret;dup = kstrndup(str, count, GFP_KERNEL);if (!dup)return NULL;trimmed = strstrip(dup);ret = kstrdup(trimmed, GFP_KERNEL);kfree(dup);return ret;Why not memmove() instead of additional memory allocation?
Or if you really want to save bytes, krealloc() after?
char *dup, *start, *ret; size_t len; dup = kstrndup(str, count, GFP_KERNEL); if (!dup) return NULL; start = strstrip(dup); len = strlen(start) - (start - dup); memmove(dup, start, len + 1); ret = krealloc(dup, len + 1, GFP_KERNEL); if (ret) return ret; kfree(dup); return NULL;?
+}
...
return sprintf(page, "%c\n", live ? '1' : '0');return sprintf(page, "%d\n", live ? 1 : 0);?
...
list_for_each_entry(bank, &dev->bank_list, siblings) {list_for_each_entry(line, &bank->line_list, siblings) {if (line->hog)num_hogs++;}}
No need to have a blank line here, but up to you.
if (!num_hogs)return 0;...
list_for_each_entry(pos, &dev->bank_list, siblings) {if (this == pos || (!this->label || !pos->label))Too many parentheses.
No, this is the logic here. Skip either if both pointers point to the same object or check if the labels are missing in at least one.
continue;if (strcmp(this->label, pos->label) == 0)return true;}...
ret = kstrtouint(page, 10, &live);Why not kstrtobool() (according to the documentation)?
Sure.
if (ret)return ret;mutex_lock(&dev->lock);if ((live == 0 && !gpio_sim_device_is_live_unlocked(dev)) ||(live == 1 && gpio_sim_device_is_live_unlocked(dev)))ret = -EPERM;else if (live == 1)ret = gpio_sim_device_activate_unlocked(dev);else if (live == 0)gpio_sim_device_deactivate_unlocked(dev);
elseret = -EINVAL;This will gone if above is being applied.
mutex_unlock(&dev->lock);...
mutex_lock(&dev->lock);ret = sprintf(page, "%s\n", bank->label ?: "");Don't we use "?" in the GPIO library for similar situations?
We use it in gpiolib to indicate a lack of label but here, it's the configuration part. I think an empty string works fine.
mutex_unlock(&dev->lock);...
ret = kstrtouint(page, 10, &num_lines);Why not allowing any digit base?
Sure.
if (ret)return ret;...
switch (dir) {case GPIOD_IN:repr = "input";break;case GPIOD_OUT_HIGH:repr = "output-high";break;case GPIOD_OUT_LOW:repr = "output-low";break;default:/* This would be a programmer bug. */WARN(1, "Unexpected hog direction value: %d", dir);return -EINVAL;}
if (strcmp(trimmed, "input") == 0)dir = GPIOD_IN;else if (strcmp(trimmed, "output-high") == 0)dir = GPIOD_OUT_HIGH;else if (strcmp(trimmed, "output-low") == 0)dir = GPIOD_OUT_LOW;elsedir = -EINVAL;Same idea, i.e. static string array and use it above and here with help of match_string().
It would be great but GPIOD_IN etc. are bit flags and not sequence enums.
...
+static struct configfs_attribute *gpio_sim_hog_config_attrs[] = {
&gpio_sim_hog_config_attr_name,&gpio_sim_hog_config_attr_direction,
NULL,Comma is not needed.
+};
...
id = ida_alloc(&gpio_sim_ida, GFP_KERNEL);if (id < 0) {kfree(dev);return ERR_PTR(id);}config_group_init_type_name(&dev->group, name,&gpio_sim_device_config_group_type);dev->id = id;If you group this assignment with above allocation it would look better.
Actually I think it looks better now with allocating the resources first, then setting up the structure.
Bart
-- With Best Regards, Andy Shevchenko