On Tue, Mar 09, 2021 at 09:59:18PM +0100, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bgolaszewski@xxxxxxxxxxxx>
>
> 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.
Thanks for an update!
Looks much better now, although few minor comments below.
Reviewed-by: Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx>
>
> Signed-off-by: Bartosz Golaszewski <bgolaszewski@xxxxxxxxxxxx>
> ---
> Documentation/admin-guide/gpio/gpio-sim.rst | 72 ++
> drivers/gpio/Kconfig | 8 +
> drivers/gpio/Makefile | 1 +
> drivers/gpio/gpio-sim.c | 879 ++++++++++++++++++++
> 4 files changed, 960 insertions(+)
> create mode 100644 Documentation/admin-guide/gpio/gpio-sim.rst
> create mode 100644 drivers/gpio/gpio-sim.c
>
> diff --git a/Documentation/admin-guide/gpio/gpio-sim.rst b/Documentation/admin-guide/gpio/gpio-sim.rst
> new file mode 100644
> index 000000000000..08eac487e35e
> --- /dev/null
> +++ b/Documentation/admin-guide/gpio/gpio-sim.rst
> @@ -0,0 +1,72 @@
> +.. SPDX-License-Identifier: GPL-2.0-or-later
> +
> +Configfs GPIO Simulator
> +=======================
> +
> +The configfs GPIO Simulator (gpio-sim) provides a way to create simulated GPIO
> +chips for testing purposes. The lines exposed by these chips can be accessed
> +using the standard GPIO character device interface as well as manipulated
> +using sysfs attributes.
> +
> +Creating simulated chips
> +------------------------
> +
> +The gpio-sim module registers a configfs subsystem called 'gpio-sim'. It's a
> +subsystem with committable items which means two subdirectories are created in
> +the filesystem: pending and live. For more information on configfs and
> +committable items, please refer to Documentation/filesystems/configfs.rst.
> +
> +In order to instantiate a new simulated chip, the user needs to mkdir() a new
> +directory in pending/. Inside each new directory, there's a set of attributes
> +that can be used to configure the new chip. Once the configuration is complete,
> +the user needs to use rename() to move the chip to the live/ directory. This
> +creates and registers the new device.
> +
> +In order to destroy a simulated chip, it has to be moved back to pending first
> +and then removed using rmdir().
> +
> +Currently supported configuration attributes are:
> +
> + num_lines - an unsigned integer value defining the number of GPIO lines to
> + export
> +
> + label - a string defining the label for the GPIO chip
> +
> + line_names - a list of GPIO line names in the form of quoted strings
> + separated by commas, e.g.: '"foo", "bar", "", "foobar"'. The
> + number of strings doesn't have to be equal to the value set in
> + the num_lines attribute. If it's lower than the number of lines,
> + the remaining lines are unnamed. If it's larger, the superfluous
> + lines are ignored. A name of the form: '""' means the line
> + should be unnamed.
> +
> +Additionally two read-only attributes named 'chip_name' and 'dev_name' are
> +exposed in order to provide users with a mapping from configfs directories to
> +the actual devices created in the kernel. The former returns the name of the
> +GPIO device as assigned by gpiolib (i.e. "gpiochip0", "gpiochip1", etc.). The
> +latter returns the parent device name as defined by the gpio-sim driver (i.e.
> +"gpio-sim.0", "gpio-sim.1", etc.). This allows user-space to map the configfs
> +items both to the correct character device file as well as the associated entry
> +in sysfs.
> +
> +Simulated GPIO chips can also be defined in device-tree. The compatible string
> +must be: "gpio-simulator". Supported properties are:
> +
> + "gpio-sim,label" - chip label
> +
> + "gpio-sim,nr-gpios" - number of lines
> +
> +Other standard GPIO properties (like "gpio-line-names" and gpio-hog) are also
> +supported.
> +
> +Manipulating simulated lines
> +----------------------------
> +
> +Each simulated GPIO chip creates a sysfs attribute group under its device
> +directory called 'line-ctrl'. Inside each group, there's a separate attribute
> +for each GPIO line. The name of the attribute is of the form 'gpioX' where X
> +is the line's offset in the chip.
> +
> +Reading from a line attribute returns the current value. Writing to it (0 or 1)
> +changes the configuration of the simulated pull-up/pull-down resistor
> +(1 - pull-up, 0 - pull-down).
> diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
> index e3607ec4c2e8..b6b6150d0d04 100644
> --- a/drivers/gpio/Kconfig
> +++ b/drivers/gpio/Kconfig
> @@ -1633,6 +1633,14 @@ config GPIO_MOCKUP
> tools/testing/selftests/gpio/gpio-mockup.sh. Reference the usage in
> it.
>
> +config GPIO_SIM
> + tristate "GPIO Simulator Module"
> + select IRQ_SIM
> + select CONFIGFS_FS
> + help
> + This enables the GPIO simulator - a configfs-based GPIO testing
> + driver.
> +
> endmenu
>
> endif
> diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile
> index c58a90a3c3b1..d717aa85f8e1 100644
> --- a/drivers/gpio/Makefile
> +++ b/drivers/gpio/Makefile
> @@ -130,6 +130,7 @@ obj-$(CONFIG_GPIO_SAMA5D2_PIOBU) += gpio-sama5d2-piobu.o
> obj-$(CONFIG_GPIO_SCH311X) += gpio-sch311x.o
> obj-$(CONFIG_GPIO_SCH) += gpio-sch.o
> obj-$(CONFIG_GPIO_SIFIVE) += gpio-sifive.o
> +obj-$(CONFIG_GPIO_SIM) += gpio-sim.o
> obj-$(CONFIG_GPIO_SIOX) += gpio-siox.o
> obj-$(CONFIG_GPIO_SL28CPLD) += gpio-sl28cpld.o
> obj-$(CONFIG_GPIO_SODAVILLE) += gpio-sodaville.o
> diff --git a/drivers/gpio/gpio-sim.c b/drivers/gpio/gpio-sim.c
> new file mode 100644
> index 000000000000..253f5b4bd1f5
> --- /dev/null
> +++ b/drivers/gpio/gpio-sim.c
> @@ -0,0 +1,879 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * GPIO testing driver based on configfs.
> + *
> + * Copyright (C) 2021 Bartosz Golaszewski <bgolaszewski@xxxxxxxxxxxx>
> + */
> +
> +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> +
> +#include <linux/bitmap.h>
> +#include <linux/configfs.h>
> +#include <linux/device.h>
> +#include <linux/gpio/driver.h>
> +#include <linux/idr.h>
> +#include <linux/interrupt.h>
> +#include <linux/irq.h>
> +#include <linux/irq_sim.h>
> +#include <linux/mod_devicetable.h>
> +#include <linux/module.h>
> +#include <linux/mutex.h>
> +#include <linux/platform_device.h>
> +#include <linux/property.h>
> +#include <linux/slab.h>
> +#include <linux/string.h>
> +#include <linux/string_helpers.h>
> +#include <linux/sysfs.h>
> +
> +#include "gpiolib.h"
> +
> +/*
> + * This normally should correspond with the number of attributes exposed over
> + * configfs + sentinel.
> + */
> +#define GPIO_SIM_MAX_PROP 4
> +
> +static DEFINE_IDA(gpio_sim_ida);
> +
> +struct gpio_sim_chip {
> + struct gpio_chip gc;
> + unsigned long *directions;
> + unsigned long *values;
> + unsigned long *pulls;
> + struct irq_domain *irq_sim;
> + struct mutex lock;
> + struct attribute_group attr_group;
> +};
> +
> +struct gpio_sim_attribute {
> + struct device_attribute dev_attr;
> + unsigned int offset;
> +};
> +
> +static struct gpio_sim_attribute *
> +to_gpio_sim_attr(struct device_attribute *dev_attr)
> +{
> + return container_of(dev_attr, struct gpio_sim_attribute, dev_attr);
> +}
> +
> +static int gpio_sim_apply_pull(struct gpio_sim_chip *chip,
> + unsigned int offset, int value)
> +{
> + int curr_val, irq, irq_type, ret;
> + struct gpio_desc *desc;
> + struct gpio_chip *gc;
> +
> + gc = &chip->gc;
> + desc = &gc->gpiodev->descs[offset];
> +
> + mutex_lock(&chip->lock);
> +
> + if (test_bit(FLAG_REQUESTED, &desc->flags) &&
> + !test_bit(FLAG_IS_OUT, &desc->flags)) {
> + curr_val = !!test_bit(offset, chip->values);
> + if (curr_val == value)
> + goto set_pull;
> +
> + /*
> + * This is fine - it just means, nobody is listening
> + * for interrupts on this line, otherwise
> + * irq_create_mapping() would have been called from
> + * the to_irq() callback.
> + */
> + irq = irq_find_mapping(chip->irq_sim, offset);
> + if (!irq)
> + goto set_value;
> +
> + irq_type = irq_get_trigger_type(irq);
> +
> + if ((value && (irq_type & IRQ_TYPE_EDGE_RISING)) ||
> + (!value && (irq_type & IRQ_TYPE_EDGE_FALLING))) {
> + ret = irq_set_irqchip_state(irq, IRQCHIP_STATE_PENDING,
> + true);
> + if (ret)
> + goto set_pull;
> + }
> + }
> +
> +set_value:
> + /* Change the value unless we're actively driving the line. */
> + if (!test_bit(FLAG_REQUESTED, &desc->flags) ||
> + !test_bit(FLAG_IS_OUT, &desc->flags))
> + __assign_bit(offset, chip->values, value);
> +
> +set_pull:
> + __assign_bit(offset, chip->pulls, value);
> + mutex_unlock(&chip->lock);
> + return 0;
> +}
> +
> +static int gpio_sim_get(struct gpio_chip *gc, unsigned int offset)
> +{
> + struct gpio_sim_chip *chip = gpiochip_get_data(gc);
> + int ret;
> +
> + mutex_lock(&chip->lock);
> + ret = !!test_bit(offset, chip->values);
> + mutex_unlock(&chip->lock);
> +
> + return ret;
> +}
> +
> +static void gpio_sim_set(struct gpio_chip *gc, unsigned int offset, int value)
> +{
> + struct gpio_sim_chip *chip = gpiochip_get_data(gc);
> +
> + mutex_lock(&chip->lock);
> + __assign_bit(offset, chip->values, value);
> + mutex_unlock(&chip->lock);
> +}
> +
> +static int gpio_sim_get_multiple(struct gpio_chip *gc,
> + unsigned long *mask, unsigned long *bits)
> +{
> + struct gpio_sim_chip *chip = gpiochip_get_data(gc);
> +
> + mutex_lock(&chip->lock);
> + bitmap_copy(bits, chip->values, gc->ngpio);
> + mutex_unlock(&chip->lock);
> +
> + return 0;
> +}
> +
> +static void gpio_sim_set_multiple(struct gpio_chip *gc,
> + unsigned long *mask, unsigned long *bits)
> +{
> + struct gpio_sim_chip *chip = gpiochip_get_data(gc);
> +
> + mutex_lock(&chip->lock);
> + bitmap_copy(chip->values, bits, gc->ngpio);
> + mutex_unlock(&chip->lock);
> +}
> +
> +static int gpio_sim_direction_output(struct gpio_chip *gc,
> + unsigned int offset, int value)
> +{
> + struct gpio_sim_chip *chip = gpiochip_get_data(gc);
> +
> + mutex_lock(&chip->lock);
> + __clear_bit(offset, chip->directions);
> + __assign_bit(offset, chip->values, value);
> + mutex_unlock(&chip->lock);
> +
> + return 0;
> +}
> +
> +static int gpio_sim_direction_input(struct gpio_chip *gc, unsigned int offset)
> +{
> + struct gpio_sim_chip *chip = gpiochip_get_data(gc);
> +
> + mutex_lock(&chip->lock);
> + __set_bit(offset, chip->directions);
> + mutex_unlock(&chip->lock);
> +
> + return 0;
> +}
> +
> +static int gpio_sim_get_direction(struct gpio_chip *gc, unsigned int offset)
> +{
> + struct gpio_sim_chip *chip = gpiochip_get_data(gc);
> + int direction;
> +
> + mutex_lock(&chip->lock);
> + direction = !!test_bit(offset, chip->directions);
> + mutex_unlock(&chip->lock);
> +
> + return direction ? GPIO_LINE_DIRECTION_IN : GPIO_LINE_DIRECTION_OUT;
> +}
> +
> +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;
> +}
> +
> +static int gpio_sim_to_irq(struct gpio_chip *gc, unsigned int offset)
> +{
> + struct gpio_sim_chip *chip = gpiochip_get_data(gc);
> +
> + return irq_create_mapping(chip->irq_sim, offset);
> +}
> +
> +static void gpio_sim_free(struct gpio_chip *gc, unsigned int offset)
> +{
> + struct gpio_sim_chip *chip = gpiochip_get_data(gc);
> +
> + mutex_lock(&chip->lock);
> + __assign_bit(offset, chip->values, !!test_bit(offset, chip->pulls));
> + mutex_unlock(&chip->lock);
> +}
> +
> +static ssize_t gpio_sim_sysfs_line_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);
> + int ret;
> +
> + mutex_lock(&chip->lock);
> + ret = sprintf(buf, "%u\n", !!test_bit(line_attr->offset, chip->values));
Shouldn't we use sysfs_emit() in a new code?
> + mutex_unlock(&chip->lock);
> +
> + return ret;
> +}
> +
> +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;
> +
> + if (len > 2 || (buf[0] != '0' && buf[0] != '1'))
> + return -EINVAL;
> +
> + val = buf[0] == '0' ? 0 : 1;
> +
> + ret = gpio_sim_apply_pull(chip, line_attr->offset, val);
> + if (ret)
> + return ret;
> +
> + return len;
> +}
> +
> +static void gpio_sim_mutex_destroy(void *data)
> +{
> + struct mutex *lock = data;
> +
> + mutex_destroy(lock);
> +}
> +
> +static void gpio_sim_sysfs_remove(void *data)
> +{
> + struct gpio_sim_chip *chip = data;
> +
> + sysfs_remove_group(&chip->gc.parent->kobj, &chip->attr_group);
> +}
> +
> +static int gpio_sim_setup_sysfs(struct gpio_sim_chip *chip)
> +{
> + unsigned int i, num_lines = chip->gc.ngpio;
> + struct device *dev = chip->gc.parent;
> + struct gpio_sim_attribute *line_attr;
> + struct device_attribute *dev_attr;
> + struct attribute **attrs;
> + int ret;
> +
> + attrs = devm_kcalloc(dev, sizeof(*attrs), num_lines + 1, GFP_KERNEL);
> + if (!attrs)
> + return -ENOMEM;
> +
> + for (i = 0; i < num_lines; i++) {
> + line_attr = devm_kzalloc(dev, sizeof(*line_attr), GFP_KERNEL);
> + if (!line_attr)
> + return -ENOMEM;
> +
> + line_attr->offset = i;
> +
> + dev_attr = &line_attr->dev_attr;
> +
> + dev_attr->attr.name = devm_kasprintf(dev, GFP_KERNEL,
> + "gpio%u", i);
> + if (!dev_attr->attr.name)
> + return -ENOMEM;
> +
> + dev_attr->attr.mode = 0644;
> +
> + dev_attr->show = gpio_sim_sysfs_line_show;
> + dev_attr->store = gpio_sim_sysfs_line_store;
> +
> + attrs[i] = &dev_attr->attr;
> + }
> +
> + chip->attr_group.name = "line-ctrl";
> + chip->attr_group.attrs = attrs;
> +
> + ret = sysfs_create_group(&dev->kobj, &chip->attr_group);
> + if (ret)
> + return ret;
> +
> + return devm_add_action_or_reset(dev, gpio_sim_sysfs_remove, chip);
> +}
> +
> +static int gpio_sim_probe(struct platform_device *pdev)
> +{
> + struct device *dev = &pdev->dev;
> + struct gpio_sim_chip *chip;
> + struct gpio_chip *gc;
> + const char *label;
> + u32 num_lines;
> + int ret;
> +
> + ret = device_property_read_u32(dev, "gpio-sim,nr-gpios", &num_lines);
> + if (ret)
> + return ret;
> +
> + ret = device_property_read_string(dev, "gpio-sim,label", &label);
> + if (ret)
> + label = dev_name(dev);
> +
> + chip = devm_kzalloc(dev, sizeof(*chip), GFP_KERNEL);
> + if (!chip)
> + return -ENOMEM;
> +
> + chip->directions = devm_bitmap_alloc(dev, num_lines, GFP_KERNEL);
> + if (!chip->directions)
> + return -ENOMEM;
> +
> + /* Default to input mode. */
> + bitmap_fill(chip->directions, num_lines);
> +
> + chip->values = devm_bitmap_zalloc(dev, num_lines, GFP_KERNEL);
> + if (!chip->values)
> + return -ENOMEM;
> +
> + chip->pulls = devm_bitmap_zalloc(dev, num_lines, GFP_KERNEL);
> + if (!chip->pulls)
> + return -ENOMEM;
> +
> + chip->irq_sim = devm_irq_domain_create_sim(dev, NULL, num_lines);
> + if (IS_ERR(chip->irq_sim))
> + return PTR_ERR(chip->irq_sim);
> +
> + mutex_init(&chip->lock);
> + ret = devm_add_action_or_reset(dev, gpio_sim_mutex_destroy,
> + &chip->lock);
> + if (ret)
> + return ret;
> +
> + gc = &chip->gc;
> + gc->base = -1;
> + gc->ngpio = num_lines;
> + gc->label = label;
> + gc->owner = THIS_MODULE;
> + gc->parent = dev;
> + gc->get = gpio_sim_get;
> + gc->set = gpio_sim_set;
> + gc->get_multiple = gpio_sim_get_multiple;
> + gc->set_multiple = gpio_sim_set_multiple;
> + gc->direction_output = gpio_sim_direction_output;
> + gc->direction_input = gpio_sim_direction_input;
> + gc->get_direction = gpio_sim_get_direction;
> + gc->set_config = gpio_sim_set_config;
> + gc->to_irq = gpio_sim_to_irq;
> + gc->free = gpio_sim_free;
> +
> + ret = devm_gpiochip_add_data(dev, gc, chip);
> + if (ret)
> + return ret;
> +
> + /* Used by sysfs and configfs callbacks. */
> + dev_set_drvdata(dev, chip);
> +
> + ret = gpio_sim_setup_sysfs(chip);
> + if (ret)
> + return ret;
> +
> + return 0;
> +}
> +
> +static const struct of_device_id gpio_sim_of_match[] = {
> + { .compatible = "gpio-simulator" },
> + { }
> +};
> +MODULE_DEVICE_TABLE(of, gpio_sim_of_match);
> +
> +static struct platform_driver gpio_sim_driver = {
> + .driver = {
> + .name = "gpio-sim",
> + },
> + .probe = gpio_sim_probe,
> +};
> +
> +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;
> +
> + /*
> + * 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;
> +};
> +
> +static struct gpio_sim_chip_config *
> +to_gpio_sim_chip_config(struct config_item *item)
> +{
> + return container_of(item, struct gpio_sim_chip_config, item);
> +}
> +
> +static ssize_t gpio_sim_config_dev_name_show(struct config_item *item,
> + char *page)
> +{
> + struct gpio_sim_chip_config *config = to_gpio_sim_chip_config(item);
> + struct platform_device *pdev;
> + int ret;
> +
> + mutex_lock(&config->lock);
> + pdev = config->pdev;
> + if (pdev)
> + ret = sprintf(page, "%s\n", dev_name(&pdev->dev));
> + else
> + ret = sprintf(page, "n/a\n");
I dunno '/' (slash) is a good character to be handled in a shell.
I would prefer 'none' or 'not available' (I think space is easier,
because the rules to escape much simpler: need just to take it into
quotes, while / needs to be escaped separately).
> + mutex_unlock(&config->lock);
> +
> + return ret;
> +}
> +
> +CONFIGFS_ATTR_RO(gpio_sim_config_, dev_name);
> +
> +static ssize_t gpio_sim_config_chip_name_show(struct config_item *item,
> + char *page)
> +{
> + struct gpio_sim_chip_config *config = to_gpio_sim_chip_config(item);
> + struct gpio_sim_chip *chip = NULL;
> + struct platform_device *pdev;
> + int ret;
> +
> + mutex_lock(&config->lock);
> + pdev = config->pdev;
> + if (pdev)
> + chip = dev_get_drvdata(&pdev->dev);
> +
> + if (chip)
> + ret = sprintf(page, "%s\n", dev_name(&chip->gc.gpiodev->dev));
> + else
> + ret = sprintf(page, "n/a\n");
Ditto.
Also don't know what the rules about using s*printf() in the configfs.
Maybe we have sysfs_emit() analogue or it doesn't applicable here at all.
Greg?
> + mutex_unlock(&config->lock);
> +
> + return ret;
> +}
> +
> +CONFIGFS_ATTR_RO(gpio_sim_config_, chip_name);
> +
> +static ssize_t gpio_sim_config_label_show(struct config_item *item, char *page)
> +{
> + struct gpio_sim_chip_config *config = to_gpio_sim_chip_config(item);
> + int ret;
> +
> + mutex_lock(&config->lock);
> + ret = sprintf(page, "%s\n", config->label);
> + mutex_unlock(&config->lock);
> +
> + return ret;
> +}
> +
> +static ssize_t gpio_sim_config_label_store(struct config_item *item,
> + const char *page, size_t count)
> +{
> + struct gpio_sim_chip_config *config = to_gpio_sim_chip_config(item);
> + char *dup, *trimmed;
> + int ret;
> +
> + mutex_lock(&config->lock);
> +
> + if (config->pdev) {
> + mutex_unlock(&config->lock);
> + return -EBUSY;
> + }
> +
> + dup = kstrndup(page, count, GFP_KERNEL);
> + if (!dup) {
> + mutex_unlock(&config->lock);
> + return -ENOMEM;
> + }
> +
> + trimmed = strstrip(dup);
> + ret = snprintf(config->label, sizeof(config->label), "%s", trimmed);
> + kfree(dup);
> + if (ret < 0) {
> + mutex_unlock(&config->lock);
> + return ret;
> + }
> +
> + mutex_unlock(&config->lock);
> + return count;
> +}
> +
> +CONFIGFS_ATTR(gpio_sim_config_, label);
> +
> +static ssize_t gpio_sim_config_num_lines_show(struct config_item *item,
> + char *page)
> +{
> + struct gpio_sim_chip_config *config = to_gpio_sim_chip_config(item);
> + int ret;
> +
> + mutex_lock(&config->lock);
> + ret = sprintf(page, "%u\n", config->num_lines);
> + mutex_unlock(&config->lock);
> +
> + return ret;
> +}
> +
> +static ssize_t gpio_sim_config_num_lines_store(struct config_item *item,
> + const char *page, size_t count)
> +{
> + struct gpio_sim_chip_config *config = to_gpio_sim_chip_config(item);
> + unsigned int num_lines;
> + int ret;
> +
> + mutex_lock(&config->lock);
> +
> + if (config->pdev) {
> + mutex_unlock(&config->lock);
> + return -EBUSY;
> + }
> +
> + ret = kstrtouint(page, 10, &num_lines);
> + if (ret) {
> + mutex_unlock(&config->lock);
> + return ret;
> + }
> +
> + if (num_lines == 0) {
> + mutex_unlock(&config->lock);
> + return -EINVAL;
> + }
> +
> + config->num_lines = num_lines;
> +
> + mutex_unlock(&config->lock);
> + return count;
> +}
> +
> +CONFIGFS_ATTR(gpio_sim_config_, num_lines);
> +
> +static ssize_t gpio_sim_config_line_names_show(struct config_item *item,
> + char *page)
> +{
> + struct gpio_sim_chip_config *config = to_gpio_sim_chip_config(item);
> + int ret, i, written = 0;
> +
> + mutex_lock(&config->lock);
> +
> + if (!config->line_names) {
> + mutex_unlock(&config->lock);
> + return sprintf(page, "\n");
> + }
> +
> + for (i = 0; i < config->num_line_names; i++) {
> + ret = sprintf(page + written,
> + i < config->num_line_names - 1 ? "\"%s\", " : "\"%s\"\n",
> + config->line_names[i] ?: "");
> + if (ret < 0) {
> + mutex_unlock(&config->lock);
> + return ret;
> + }
> +
> + written += ret;
> + }
> +
> + mutex_unlock(&config->lock);
> + return written;
> +}
> +
> +static ssize_t gpio_sim_config_line_names_store(struct config_item *item,
> + const char *page, size_t count)
> +{
> + struct gpio_sim_chip_config *config = to_gpio_sim_chip_config(item);
> + unsigned int num_new_names = 1, num_old_names, name_idx = 0;
> + bool in_quote = false, got_comma = true;
> + char **new_names, **old_names, *name, c;
> + const char *start = page;
> + size_t pos, name_len;
> + int err = -EINVAL;
> +
> + mutex_lock(&config->lock);
> +
> + if (config->pdev) {
> + mutex_unlock(&config->lock);
> + return -EBUSY;
> + }
> +
> + /*
> + * Line names are stored in a pointer array so that we can easily
> + * pass them down to the GPIO subsystem in a "gpio-line-names"
> + * property.
> + *
> + * Line names must be passed as a list of quoted names separated by
> + * commas, for example: '"foo", "bar", "foobar"'.
> + */
> +
> + for (pos = 0; pos < count; pos++) {
> + /*
> + * Just count the commas and assume the number if strings
> + * equals the number of commas + 1. If the format is wrong
> + * we'll bail out anyway.
> + */
> + if (page[pos] == ',')
> + num_new_names++;
> + }
> +
> + new_names = kcalloc(num_new_names, sizeof(char *), GFP_KERNEL);
> + if (!new_names) {
> + mutex_unlock(&config->lock);
> + return -ENOMEM;
> + }
> +
> + /*
> + * REVISIT There are more users in the kernel that want to split
> + * comma-separated strings into tokens. We should generalize it and
> + * put it into lib/.
> + */
Thanks!
> + for (pos = 0; pos < count; pos++) {
> + c = page[pos];
> +
> + if (in_quote) {
> + if (c == '"') {
> + /* This is the end of the name. */
> + in_quote = got_comma = false;
> + name_len = (page + pos) - start;
> + if (name_len == 0) {
> + /* Name is empty (passed as ""). */
> + name_idx++;
> + continue;
> + }
> +
> + name = kmemdup(start, name_len + 1, GFP_KERNEL);
> + if (!name) {
> + err = -ENOMEM;
> + goto err_out;
> + }
> +
> + name[name_len] = '\0';
> + new_names[name_idx++] = name;
> + }
> + } else {
> + if (c == '"') {
> + /* Enforce separating names with commas. */
> + if (!got_comma)
> + goto err_out;
> +
> + start = page + pos + 1;
> + in_quote = true;
> + } else if (c == ',') {
> + if (!got_comma)
> + got_comma = true;
> + else
> + /* Double commas are not allowed. */
> + goto err_out;
> + } else if (!isspace(c)) {
> + goto err_out;
> + }
> + }
> + }
> +
> + /*
> + * End of input sanity checks, must not have a comma at the end and
> + * must have finished scanning the last name.
> + */
> + if (in_quote || got_comma)
> + goto err_out;
> +
> + old_names = config->line_names;
> + num_old_names = config->num_line_names;
> + config->line_names = new_names;
> + config->num_line_names = num_new_names;
> +
> + mutex_unlock(&config->lock);
> + kfree_strarray(old_names, num_old_names);
> + return count;
> +
> +err_out:
> + mutex_unlock(&config->lock);
> + kfree_strarray(new_names, name_idx);
> + return err;
> +}
> +
> +CONFIGFS_ATTR(gpio_sim_config_, line_names);
> +
> +static struct configfs_attribute *gpio_sim_config_attrs[] = {
> + &gpio_sim_config_attr_dev_name,
> + &gpio_sim_config_attr_chip_name,
> + &gpio_sim_config_attr_label,
> + &gpio_sim_config_attr_num_lines,
> + &gpio_sim_config_attr_line_names,
> + NULL
> +};
> +
> +static void gpio_sim_chip_config_release(struct config_item *item)
> +{
> + struct gpio_sim_chip_config *config = to_gpio_sim_chip_config(item);
> +
> + mutex_destroy(&config->lock);
> + kfree_strarray(config->line_names, config->num_line_names);
> + kfree(config);
> +}
> +
> +static struct configfs_item_operations gpio_sim_config_item_ops = {
> + .release = gpio_sim_chip_config_release,
> +};
> +
> +static const struct config_item_type gpio_sim_chip_config_type = {
> + .ct_item_ops = &gpio_sim_config_item_ops,
> + .ct_attrs = gpio_sim_config_attrs,
> + .ct_owner = THIS_MODULE,
> +};
> +
> +static struct config_item *
> +gpio_sim_config_make_item(struct config_group *group, const char *name)
> +{
> + struct gpio_sim_chip_config *config;
> +
> + config = kzalloc(sizeof(*config), GFP_KERNEL);
> + if (!config)
> + return ERR_PTR(-ENOMEM);
> +
> + config_item_init_type_name(&config->item, name,
> + &gpio_sim_chip_config_type);
> + config->num_lines = 1;
> + mutex_init(&config->lock);
> +
> + return &config->item;
> +}
> +
> +static int gpio_sim_config_commit_item(struct config_item *item)
> +{
> + struct gpio_sim_chip_config *config = to_gpio_sim_chip_config(item);
> + struct property_entry properties[GPIO_SIM_MAX_PROP];
> + struct platform_device_info pdevinfo;
> + struct platform_device *pdev;
> + unsigned int prop_idx = 0;
> +
> + memset(&pdevinfo, 0, sizeof(pdevinfo));
> + memset(properties, 0, sizeof(properties));
> +
> + mutex_lock(&config->lock);
> +
> + properties[prop_idx++] = PROPERTY_ENTRY_U32("gpio-sim,nr-gpios",
> + config->num_lines);
> + if (config->label[0] != '\0')
I'm wondering if we need this check. Isn't core taking care of it?
> + properties[prop_idx++] = PROPERTY_ENTRY_STRING("gpio-sim,label",
> + config->label);
> + if (config->line_names)
Ditto.
> + properties[prop_idx++] = PROPERTY_ENTRY_STRING_ARRAY_LEN(
> + "gpio-line-names",
> + config->line_names,
> + config->num_line_names);
> +
> + pdevinfo.id = ida_alloc(&gpio_sim_ida, GFP_KERNEL);
> + if (pdevinfo.id < 0) {
> + mutex_unlock(&config->lock);
> + return pdevinfo.id;
> + }
> +
> + pdevinfo.name = "gpio-sim";
> + pdevinfo.properties = properties;
> +
> + pdev = platform_device_register_full(&pdevinfo);
> + if (IS_ERR(pdev)) {
> + ida_free(&gpio_sim_ida, pdevinfo.id);
> + mutex_unlock(&config->lock);
> + return PTR_ERR(pdev);
> + }
> +
> + config->pdev = pdev;
> + mutex_unlock(&config->lock);
> +
> + return 0;
> +}
> +
> +static int gpio_sim_config_uncommit_item(struct config_item *item)
> +{
> + struct gpio_sim_chip_config *config = to_gpio_sim_chip_config(item);
> + int id;
> +
> + mutex_lock(&config->lock);
> + id = config->pdev->id;
> + platform_device_unregister(config->pdev);
> + config->pdev = NULL;
> + mutex_unlock(&config->lock);
> +
> + ida_free(&gpio_sim_ida, id);
> + return 0;
> +}
> +
> +static struct configfs_group_operations gpio_sim_config_group_ops = {
> + .make_item = gpio_sim_config_make_item,
> + .commit_item = gpio_sim_config_commit_item,
> + .uncommit_item = gpio_sim_config_uncommit_item,
> +};
> +
> +static const struct config_item_type gpio_sim_config_type = {
> + .ct_group_ops = &gpio_sim_config_group_ops,
> + .ct_owner = THIS_MODULE,
> +};
> +
> +static struct configfs_subsystem gpio_sim_config_subsys = {
> + .su_group = {
> + .cg_item = {
> + .ci_namebuf = "gpio-sim",
> + .ci_type = &gpio_sim_config_type,
> + },
> + },
> +};
> +
> +static int __init gpio_sim_init(void)
> +{
> + int ret;
> +
> + ret = platform_driver_register(&gpio_sim_driver);
> + if (ret) {
> + pr_err("Error %d while registering the platform driver\n", ret);
> + return ret;
> + }
> +
> + config_group_init(&gpio_sim_config_subsys.su_group);
> + mutex_init(&gpio_sim_config_subsys.su_mutex);
> + ret = configfs_register_subsystem(&gpio_sim_config_subsys);
> + if (ret) {
> + pr_err("Error %d while registering the configfs subsystem %s\n",
> + ret, gpio_sim_config_subsys.su_group.cg_item.ci_namebuf);
> + mutex_destroy(&gpio_sim_config_subsys.su_mutex);
> + platform_driver_unregister(&gpio_sim_driver);
> + return ret;
> + }
> +
> + return 0;
> +}
> +module_init(gpio_sim_init);
> +
> +static void __exit gpio_sim_exit(void)
> +{
> + configfs_unregister_subsystem(&gpio_sim_config_subsys);
> + mutex_destroy(&gpio_sim_config_subsys.su_mutex);
> + platform_driver_unregister(&gpio_sim_driver);
> +}
> +module_exit(gpio_sim_exit);
> +
> +MODULE_AUTHOR("Bartosz Golaszewski <bgolaszewski@xxxxxxxxxxxx>");
> +MODULE_DESCRIPTION("GPIO Simulator Module");
> +MODULE_LICENSE("GPL");
> --
> 2.30.1
>
--
With Best Regards,
Andy Shevchenko
