diff mbox series

[v12,4/7] gpio: sim: new testing module

Message ID 20211203133003.31786-5-brgl@bgdev.pl (mailing list archive)
State New
Headers show
Series gpio-sim: configfs-based GPIO simulator | expand

Commit Message

Bartosz Golaszewski Dec. 3, 2021, 1:30 p.m. UTC
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.

Signed-off-by: Bartosz Golaszewski <brgl@bgdev.pl>
---
 Documentation/admin-guide/gpio/gpio-sim.rst |  134 ++
 drivers/gpio/Kconfig                        |    8 +
 drivers/gpio/Makefile                       |    1 +
 drivers/gpio/gpio-sim.c                     | 1594 +++++++++++++++++++
 4 files changed, 1737 insertions(+)
 create mode 100644 Documentation/admin-guide/gpio/gpio-sim.rst
 create mode 100644 drivers/gpio/gpio-sim.c

Comments

Andy Shevchenko Dec. 3, 2021, 8:07 p.m. UTC | #1
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.

...

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

...

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

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

...

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

...

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

> +}

...

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

> +	mutex_lock(&chip->lock);
> +	pull = !!test_bit(line_attr->offset, chip->pull_map);
> +	mutex_unlock(&chip->lock);

> +	if (pull)
> +		repr = "pull-up";
> +	else
> +		repr = "pull-down";
> +
> +	return sysfs_emit(buf, "%s\n", repr);

	return sysfs_emit(buf, "%pull-s\n", pull_up ? "up" : "down");

?

> +}

...

> +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;
> +	else
> +		return -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().

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

...

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

> +				continue;
> +
> +			if (strcmp(this->label, pos->label) == 0)
> +				return true;
> +		}

...

> +	ret = kstrtouint(page, 10, &live);

Why not kstrtobool() (according to the documentation)?

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

> +	else
> +		ret = -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?

> +	mutex_unlock(&dev->lock);

...

> +	ret = kstrtouint(page, 10, &num_lines);

Why not allowing any digit base?

> +	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;
> +	else
> +		dir = -EINVAL;


Same idea, i.e. static string array and use it above and here with help
of match_string().

...

> +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.
Bartosz Golaszewski Dec. 6, 2021, 9:48 a.m. UTC | #2
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";
> > +     else
> > +             repr = "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;
> > +     else
> > +             return -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);
>
> > +     else
> > +             ret = -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;
> > +     else
> > +             dir = -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
>
>
Andy Shevchenko Dec. 6, 2021, 1:32 p.m. UTC | #3
On Mon, Dec 06, 2021 at 10:48:00AM +0100, Bartosz Golaszewski wrote:
> 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:

...

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

I understand that and agree, but my point is orthogonal to this. The idea is to
emphasize that "hey. this driver has tough relations with the GPIO subsystem".
This is the way, for example, IIO does and I like it.

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

Yes, but I would consider to join back those which are up to ~83 characters
(I already pointed out at least to one example like this).

...

> > > +     if (sysfs_streq(buf, "pull-down"))
> > > +             pull = 0;
> > > +     else if (sysfs_streq(buf, "pull-up"))
> > > +             pull = 1;
> > > +     else
> > > +             return -EINVAL;
> >
> > sysfs_match_string() and use the very same string array in the above function
> > to print them?

I suppose you agree on this?

...

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

Nope, it may set _more_ than all bits. That's why bitmap_set() is more
accurate, because it will do exact setting.

...

> > > +     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;
> > > +     else
> > > +             dir = -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.

Ah, okay, it will make rather sparse array.
Bartosz Golaszewski Dec. 6, 2021, 3:38 p.m. UTC | #4
On Mon, Dec 6, 2021 at 2:33 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> On Mon, Dec 06, 2021 at 10:48:00AM +0100, Bartosz Golaszewski wrote:
> > 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:
>
> ...
>
> > > > +#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.
>
> I understand that and agree, but my point is orthogonal to this. The idea is to
> emphasize that "hey. this driver has tough relations with the GPIO subsystem".
> This is the way, for example, IIO does and I like it.
>

I really don't think this is necessary.

> > > > +#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.
>
> Yes, but I would consider to join back those which are up to ~83 characters
> (I already pointed out at least to one example like this).
>

I like the old-style limit TBH.

> ...
>
> > > > +     if (sysfs_streq(buf, "pull-down"))
> > > > +             pull = 0;
> > > > +     else if (sysfs_streq(buf, "pull-up"))
> > > > +             pull = 1;
> > > > +     else
> > > > +             return -EINVAL;
> > >
> > > sysfs_match_string() and use the very same string array in the above function
> > > to print them?
>
> I suppose you agree on this?
>

Yes, already changed that in v13.

> ...
>
> > > > +     /* 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.
>
> Nope, it may set _more_ than all bits. That's why bitmap_set() is more
> accurate, because it will do exact setting.
>

Can this in any way affect any of the code? If the driver is correct,
it will never use anything beyond the last line bit. If it does, it
needs fixing. It's as if we cared about what happens to padding added
to structures by the compiler (as long as we're not passing it to
user-space of course).

> ...
>
> > > > +     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;
> > > > +     else
> > > > +             dir = -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.
>
> Ah, okay, it will make rather sparse array.
>

Idea for the future: introduce match_string_ext() with flags one of
which would allow sparse string arrays?

Bart
Andy Shevchenko Dec. 6, 2021, 5 p.m. UTC | #5
On Mon, Dec 06, 2021 at 04:38:44PM +0100, Bartosz Golaszewski wrote:
> On Mon, Dec 6, 2021 at 2:33 PM Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
> > On Mon, Dec 06, 2021 at 10:48:00AM +0100, Bartosz Golaszewski wrote:
> > > On Fri, Dec 3, 2021 at 9:08 PM Andy Shevchenko
> > > <andriy.shevchenko@linux.intel.com> wrote:

...

> > > Nah, the lines are broken just fine. Let's not overuse the limit.
> >
> > Yes, but I would consider to join back those which are up to ~83 characters
> > (I already pointed out at least to one example like this).
> 
> I like the old-style limit TBH.

And it's fine. It has remark about overlapping in case of readability and 81
characters on one line is fine as some cases when it is up to ~83 (for old
style).

Anyways, it doesn't worth of spending more time on this. Your choice then.

...

> > > > > +     /* 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.
> >
> > Nope, it may set _more_ than all bits. That's why bitmap_set() is more
> > accurate, because it will do exact setting.
> 
> Can this in any way affect any of the code? If the driver is correct,
> it will never use anything beyond the last line bit. If it does, it
> needs fixing. It's as if we cared about what happens to padding added
> to structures by the compiler (as long as we're not passing it to
> user-space of course).

I haven't checked if it affects current code. Consider this as heads up,
because developers often forget about this nuance of bitmap_fill() /
bitmap_clear().

...

> > > > > +     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;
> > > > > +     else
> > > > > +             dir = -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.
> >
> > Ah, okay, it will make rather sparse array.
> 
> Idea for the future: introduce match_string_ext() with flags one of
> which would allow sparse string arrays?

I thought about that, but since it's a mix of the bits, it might not be so
universal anyway, I would rather think of something which uses 1-bit bit
fields unified under a bit mask, and not a mix of 2 or more bits.
diff mbox series

Patch

diff --git a/Documentation/admin-guide/gpio/gpio-sim.rst b/Documentation/admin-guide/gpio/gpio-sim.rst
new file mode 100644
index 000000000000..d8a90c81b9ee
--- /dev/null
+++ b/Documentation/admin-guide/gpio/gpio-sim.rst
@@ -0,0 +1,134 @@ 
+.. 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'``. For
+details of the configfs filesystem, please refer to the configfs documentation.
+
+The user can create a hierarchy of configfs groups and items as well as modify
+values of exposed attributes. Once the chip is instantiated, this hierarchy
+will be translated to appropriate device properties. The general structure is:
+
+**Group:** ``/config/gpio-sim``
+
+This is the top directory of the gpio-sim configfs tree.
+
+**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.
+
+**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``
+
+This group represents a bank of GPIOs under the top platform device. The
+``'chip_name'`` attribute is read-only and allows the user-space to read the
+device name of the bank device. The ``'num_lines'`` attribute allows to specify
+the number of lines exposed by this bank.
+
+**Group:** ``/config/gpio-sim/gpio-device/gpio-bankX/lineY``
+
+**Attribute:** ``/config/gpio-sim/gpio-device/gpio-bankX/lineY/name``
+
+This group represents a single line at the offset Y. The 'name' attribute
+allows to set the line name as represented by the 'gpio-line-names' property.
+
+**Item:** ``/config/gpio-sim/gpio-device/gpio-bankX/lineY/hog``
+
+**Attribute:** ``/config/gpio-sim/gpio-device/gpio-bankX/lineY/hog/name``
+
+**Attribute:** ``/config/gpio-sim/gpio-device/gpio-bankX/lineY/hog/direction``
+
+This item makes the gpio-sim module hog the associated line. The ``'name'``
+attribute specifies the in-kernel consumer name to use. The ``'direction'``
+attribute specifies the hog direction and must be one of: ``'input'``,
+``'output-high'`` and ``'output-low'``.
+
+Inside each bank directory, there's a set of attributes that can be used to
+configure the new chip. Additionally the user can ``mkdir()`` subdirectories
+inside the chip's directory that allow to pass additional configuration for
+specific lines. The name of those subdirectories must take the form of:
+``'line<offset>'`` (e.g. ``'line0'``, ``'line20'``, etc.) as the name will be
+used by the module to assign the config to the specific line at given offset.
+
+Once the confiuration is complete, the ``'live'`` attribute must be set to 1 in
+order to instantiate the chip. It can be set back to 0 to destroy the simulated
+chip. The module will synchronously wait for the new simulated device to be
+successfully probed and if this doesn't happen, writing to ``'live'`` will
+result in an error.
+
+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
+
+Other standard GPIO properties (like ``"gpio-line-names"``, ``"ngpios"`` or
+``"gpio-hog"``) are also supported. Please refer to the GPIO documentation for
+details.
+
+An example device-tree code defining a GPIO simulator:
+
+.. code-block :: none
+
+    gpio-sim {
+        compatible = "gpio-simulator";
+
+        bank0 {
+            gpio-controller;
+            #gpio-cells = <2>;
+            ngpios = <16>;
+            gpio-sim,label = "dt-bank0";
+            gpio-line-names = "", "sim-foo", "", "sim-bar";
+        };
+
+        bank1 {
+            gpio-controller;
+            #gpio-cells = <2>;
+            ngpios = <8>;
+            gpio-sim,label = "dt-bank1";
+
+            line3 {
+                gpio-hog;
+                gpios = <3 0>;
+                output-high;
+                line-name = "sim-hog-from-dt";
+            };
+        };
+    };
+
+Manipulating simulated lines
+----------------------------
+
+Each simulated GPIO chip creates a separate sysfs group under its device
+directory for each exposed line
+(e.g. ``/sys/devices/platform/gpio-sim.X/gpiochipY/``). The name of each group
+is of the form: ``'sim_gpioX'`` where X is the offset of the line. Inside each
+group there are two attibutes:
+
+    ``pull`` - allows to read and set the current simulated pull setting for
+               every line, when writing the value must be one of: ``'pull-up'``,
+               ``'pull-down'``
+
+    ``value`` - allows to read the current value of the line which may be
+                different from the pull if the line is being driven from
+                user-space
diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
index 60d9374c72c0..9acdb4d1047b 100644
--- a/drivers/gpio/Kconfig
+++ b/drivers/gpio/Kconfig
@@ -1694,6 +1694,14 @@  config GPIO_VIRTIO
 	  These virtual GPIOs can be routed to real GPIOs or attached to
 	  simulators on the host (like QEMU).
 
+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 71ee9fc2ff83..f21577de2474 100644
--- a/drivers/gpio/Makefile
+++ b/drivers/gpio/Makefile
@@ -133,6 +133,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..0f8399a61733
--- /dev/null
+++ b/drivers/gpio/gpio-sim.c
@@ -0,0 +1,1594 @@ 
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * GPIO testing driver based on configfs.
+ *
+ * Copyright (C) 2021 Bartosz Golaszewski <brgl@bgdev.pl>
+ */
+
+#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+
+#include <linux/bitmap.h>
+#include <linux/completion.h>
+#include <linux/configfs.h>
+#include <linux/device.h>
+#include <linux/gpio/driver.h>
+#include <linux/gpio/machine.h>
+#include <linux/idr.h>
+#include <linux/interrupt.h>
+#include <linux/irq.h>
+#include <linux/irq_sim.h>
+#include <linux/list.h>
+#include <linux/mod_devicetable.h>
+#include <linux/module.h>
+#include <linux/mutex.h>
+#include <linux/notifier.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"
+
+#define GPIO_SIM_PROP_MAX	4 /* Max 3 properties + sentinel. */
+#define GPIO_SIM_NUM_ATTRS	3 /* value, pull and sentinel */
+
+static DEFINE_IDA(gpio_sim_ida);
+
+struct gpio_sim_chip {
+	struct gpio_chip gc;
+	unsigned long *direction_map;
+	unsigned long *value_map;
+	unsigned long *pull_map;
+	struct irq_domain *irq_sim;
+	struct mutex lock;
+	const struct attribute_group **attr_groups;
+};
+
+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->value_map);
+		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->value_map, value);
+
+set_pull:
+	__assign_bit(offset, chip->pull_map, 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->value_map);
+	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->value_map, 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->value_map, 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->value_map, 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->direction_map);
+	__assign_bit(offset, chip->value_map, 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->direction_map);
+	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->direction_map);
+	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->value_map, !!test_bit(offset, chip->pull_map));
+	mutex_unlock(&chip->lock);
+}
+
+static ssize_t gpio_sim_sysfs_val_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 val;
+
+	mutex_lock(&chip->lock);
+	val = !!test_bit(line_attr->offset, chip->value_map);
+	mutex_unlock(&chip->lock);
+
+	return sysfs_emit(buf, "%d\n", val);
+}
+
+static ssize_t gpio_sim_sysfs_val_store(struct device *dev,
+					struct device_attribute *attr,
+					const char *buf, size_t count)
+{
+	/*
+	 * Not assigning this function will result in write() returning -EIO
+	 * which is confusing. Return -EPERM explicitly.
+	 */
+	return -EPERM;
+}
+
+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;
+
+	mutex_lock(&chip->lock);
+	pull = !!test_bit(line_attr->offset, chip->pull_map);
+	mutex_unlock(&chip->lock);
+
+	if (pull)
+		repr = "pull-up";
+	else
+		repr = "pull-down";
+
+	return sysfs_emit(buf, "%s\n", repr);
+}
+
+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;
+	else
+		return -EINVAL;
+
+	ret = gpio_sim_apply_pull(chip, line_attr->offset, pull);
+	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_groups(&chip->gc.gpiodev->dev.kobj, chip->attr_groups);
+}
+
+static int gpio_sim_setup_sysfs(struct gpio_sim_chip *chip)
+{
+	struct device_attribute *val_dev_attr, *pull_dev_attr;
+	struct gpio_sim_attribute *val_attr, *pull_attr;
+	unsigned int num_lines = chip->gc.ngpio;
+	struct device *dev = chip->gc.parent;
+	struct attribute_group *attr_group;
+	struct attribute **attrs;
+	int i, ret;
+
+	chip->attr_groups = devm_kcalloc(dev, sizeof(*chip->attr_groups),
+					 num_lines + 1, GFP_KERNEL);
+	if (!chip->attr_groups)
+		return -ENOMEM;
+
+	for (i = 0; i < num_lines; i++) {
+		attr_group = devm_kzalloc(dev, sizeof(*attr_group), GFP_KERNEL);
+		attrs = devm_kcalloc(dev, sizeof(*attrs),
+				     GPIO_SIM_NUM_ATTRS, GFP_KERNEL);
+		val_attr = devm_kzalloc(dev, sizeof(*val_attr), GFP_KERNEL);
+		pull_attr = devm_kzalloc(dev, sizeof(*pull_attr), GFP_KERNEL);
+		if (!attr_group || !attrs || !val_attr || !pull_attr)
+			return -ENOMEM;
+
+		attr_group->name = devm_kasprintf(dev, GFP_KERNEL,
+						  "sim_gpio%u", i);
+		if (!attr_group->name)
+			return -ENOMEM;
+
+		val_attr->offset = pull_attr->offset = i;
+
+		val_dev_attr = &val_attr->dev_attr;
+		pull_dev_attr = &pull_attr->dev_attr;
+
+		sysfs_attr_init(&val_dev_attr->attr);
+		sysfs_attr_init(&pull_dev_attr->attr);
+
+		val_dev_attr->attr.name = "value";
+		pull_dev_attr->attr.name = "pull";
+
+		val_dev_attr->attr.mode = pull_dev_attr->attr.mode = 0644;
+
+		val_dev_attr->show = gpio_sim_sysfs_val_show;
+		val_dev_attr->store = gpio_sim_sysfs_val_store;
+		pull_dev_attr->show = gpio_sim_sysfs_pull_show;
+		pull_dev_attr->store = gpio_sim_sysfs_pull_store;
+
+		attrs[0] = &val_dev_attr->attr;
+		attrs[1] = &pull_dev_attr->attr;
+
+		attr_group->attrs = attrs;
+		chip->attr_groups[i] = attr_group;
+	}
+
+	ret = sysfs_create_groups(&chip->gc.gpiodev->dev.kobj,
+				  chip->attr_groups);
+	if (ret)
+		return ret;
+
+	return devm_add_action_or_reset(dev, gpio_sim_sysfs_remove, chip);
+}
+
+static int gpio_sim_add_bank(struct fwnode_handle *swnode, struct device *dev)
+{
+	struct gpio_sim_chip *chip;
+	struct gpio_chip *gc;
+	const char *label;
+	u32 num_lines;
+	int ret;
+
+	ret = fwnode_property_read_u32(swnode, "ngpios", &num_lines);
+	if (ret)
+		return ret;
+
+	ret = fwnode_property_read_string(swnode, "gpio-sim,label", &label);
+	if (ret) {
+		label = devm_kasprintf(dev, GFP_KERNEL, "%s-%s",
+				       dev_name(dev), fwnode_get_name(swnode));
+		if (!label)
+			return -ENOMEM;
+	}
+
+	chip = devm_kzalloc(dev, sizeof(*chip), GFP_KERNEL);
+	if (!chip)
+		return -ENOMEM;
+
+	chip->direction_map = devm_bitmap_alloc(dev, num_lines, GFP_KERNEL);
+	if (!chip->direction_map)
+		return -ENOMEM;
+
+	/* Default to input mode. */
+	bitmap_fill(chip->direction_map, num_lines);
+
+	chip->value_map = devm_bitmap_zalloc(dev, num_lines, GFP_KERNEL);
+	if (!chip->value_map)
+		return -ENOMEM;
+
+	chip->pull_map = devm_bitmap_zalloc(dev, num_lines, GFP_KERNEL);
+	if (!chip->pull_map)
+		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->fwnode = swnode;
+	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(&gc->gpiodev->dev, chip);
+
+	return gpio_sim_setup_sysfs(chip);
+}
+
+static int gpio_sim_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct fwnode_handle *swnode;
+	int ret;
+
+	device_for_each_child_node(dev, swnode) {
+		ret = gpio_sim_add_bank(swnode, dev);
+		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",
+		.of_match_table = gpio_sim_of_match,
+	},
+	.probe = gpio_sim_probe,
+};
+
+struct gpio_sim_device {
+	struct config_group group;
+
+	/*
+	 * If pdev is NULL, the device is 'pending' (waiting for configuration).
+	 * Once the pointer is assigned, the device has been created and the
+	 * item is 'live'.
+	 */
+	struct platform_device *pdev;
+	int id;
+
+	/*
+	 * 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.
+	 *
+	 * We use this lock fo protecting all data structures owned by this
+	 * object too.
+	 */
+	struct mutex lock;
+
+	/*
+	 * This is used to synchronously wait for the driver's probe to complete
+	 * and notify the user-space about any errors.
+	 */
+	struct notifier_block bus_notifier;
+	struct completion probe_completion;
+	bool driver_bound;
+
+	struct gpiod_hog *hogs;
+
+	struct list_head bank_list;
+};
+
+/* This is called with dev->lock already taken. */
+static int gpio_sim_bus_notifier_call(struct notifier_block *nb,
+				      unsigned long action, void *data)
+{
+	struct gpio_sim_device *simdev = container_of(nb,
+						      struct gpio_sim_device,
+						      bus_notifier);
+	struct device *dev = data;
+	char devname[32];
+
+	snprintf(devname, sizeof(devname), "gpio-sim.%u", simdev->id);
+
+	if (strcmp(dev_name(dev), devname) == 0) {
+		if (action == BUS_NOTIFY_BOUND_DRIVER)
+			simdev->driver_bound = true;
+		else if (action == BUS_NOTIFY_DRIVER_NOT_BOUND)
+			simdev->driver_bound = false;
+		else
+			return NOTIFY_DONE;
+
+		complete(&simdev->probe_completion);
+		return NOTIFY_OK;
+	}
+
+	return NOTIFY_DONE;
+}
+
+static struct gpio_sim_device *to_gpio_sim_device(struct config_item *item)
+{
+	struct config_group *group = to_config_group(item);
+
+	return container_of(group, struct gpio_sim_device, group);
+}
+
+struct gpio_sim_bank {
+	struct config_group group;
+
+	/*
+	 * We could have used the ci_parent field of the config_item but
+	 * configfs is stupid and calls the item's release callback after
+	 * already having cleared the parent pointer even though the parent
+	 * is guaranteed to survive the child...
+	 *
+	 * So we need to store the pointer to the parent struct here. We can
+	 * dereference it anywhere we need with no checks and no locking as
+	 * it's guaranteed to survive the childred and protected by configfs
+	 * locks.
+	 *
+	 * Same for other structures.
+	 */
+	struct gpio_sim_device *parent;
+	struct list_head siblings;
+
+	char *label;
+	unsigned int num_lines;
+
+	struct list_head line_list;
+
+	struct fwnode_handle *swnode;
+};
+
+static struct gpio_sim_bank *to_gpio_sim_bank(struct config_item *item)
+{
+	struct config_group *group = to_config_group(item);
+	return container_of(group, struct gpio_sim_bank, group);
+}
+
+static struct gpio_sim_device *
+gpio_sim_bank_get_device(struct gpio_sim_bank *bank)
+{
+	return bank->parent;
+}
+
+struct gpio_sim_hog;
+
+struct gpio_sim_line {
+	struct config_group group;
+
+	struct gpio_sim_bank *parent;
+	struct list_head siblings;
+
+	unsigned int offset;
+	char *name;
+
+	/* There can only be one hog per line. */
+	struct gpio_sim_hog *hog;
+};
+
+static struct gpio_sim_line *to_gpio_sim_line(struct config_item *item)
+{
+	struct config_group *group = to_config_group(item);
+
+	return container_of(group, struct gpio_sim_line, group);
+}
+
+static struct gpio_sim_device *
+gpio_sim_line_get_device(struct gpio_sim_line *line)
+{
+	struct gpio_sim_bank *bank = line->parent;
+
+	return gpio_sim_bank_get_device(bank);
+}
+
+struct gpio_sim_hog {
+	struct config_item item;
+	struct gpio_sim_line *parent;
+
+	char *name;
+	int dir;
+};
+
+static struct gpio_sim_hog *to_gpio_sim_hog(struct config_item *item)
+{
+	return container_of(item, struct gpio_sim_hog, item);
+}
+
+static struct gpio_sim_device *gpio_sim_hog_get_device(struct gpio_sim_hog *hog)
+{
+	struct gpio_sim_line *line = hog->parent;
+
+	return gpio_sim_line_get_device(line);
+}
+
+static bool gpio_sim_device_is_live_unlocked(struct gpio_sim_device *dev)
+{
+	return !!dev->pdev;
+}
+
+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;
+}
+
+static ssize_t gpio_sim_device_config_dev_name_show(struct config_item *item,
+						    char *page)
+{
+	struct gpio_sim_device *dev = to_gpio_sim_device(item);
+	struct platform_device *pdev;
+	int ret;
+
+	mutex_lock(&dev->lock);
+	pdev = dev->pdev;
+	if (pdev)
+		ret = sprintf(page, "%s\n", dev_name(&pdev->dev));
+	else
+		ret = sprintf(page, "gpio-sim.%d\n", dev->id);
+	mutex_unlock(&dev->lock);
+
+	return ret;
+}
+
+CONFIGFS_ATTR_RO(gpio_sim_device_config_, dev_name);
+
+static ssize_t
+gpio_sim_device_config_live_show(struct config_item *item, char *page)
+{
+	struct gpio_sim_device *dev = to_gpio_sim_device(item);
+	bool live;
+
+	mutex_lock(&dev->lock);
+	live = gpio_sim_device_is_live_unlocked(dev);
+	mutex_unlock(&dev->lock);
+
+	return sprintf(page, "%c\n", live ? '1' : '0');
+}
+
+static char **gpio_sim_make_line_names(struct gpio_sim_bank *bank,
+				       unsigned int *line_names_size)
+{
+	unsigned int max_offset = 0;
+	bool has_line_names = false;
+	struct gpio_sim_line *line;
+	char **line_names;
+
+	list_for_each_entry(line, &bank->line_list, siblings) {
+		if (line->name) {
+			if (line->offset > max_offset)
+				max_offset = line->offset;
+
+			/*
+			 * max_offset can stay at 0 so it's not an indicator
+			 * of whether line names were configured at all.
+			 */
+			has_line_names = true;
+		}
+	}
+
+	if (!has_line_names)
+		/*
+		 * This is not an error - NULL means, there are no line
+		 * names configured.
+		 */
+		return NULL;
+
+	*line_names_size = max_offset + 1;
+
+	line_names = kcalloc(*line_names_size, sizeof(*line_names), GFP_KERNEL);
+	if (!line_names)
+		return ERR_PTR(-ENOMEM);
+
+	list_for_each_entry(line, &bank->line_list, siblings)
+		line_names[line->offset] = line->name;
+
+	return line_names;
+}
+
+static void gpio_sim_remove_hogs(struct gpio_sim_device *dev)
+{
+	struct gpiod_hog *hog;
+
+	if (!dev->hogs)
+		return;
+
+	gpiod_remove_hogs(dev->hogs);
+
+	for (hog = dev->hogs; !hog->chip_label; hog++) {
+		kfree(hog->chip_label);
+		kfree(hog->line_name);
+	}
+
+	kfree(dev->hogs);
+	dev->hogs = NULL;
+}
+
+static int gpio_sim_add_hogs(struct gpio_sim_device *dev)
+{
+	unsigned int num_hogs = 0, idx = 0;
+	struct gpio_sim_bank *bank;
+	struct gpio_sim_line *line;
+	struct gpiod_hog *hog;
+
+	list_for_each_entry(bank, &dev->bank_list, siblings) {
+		list_for_each_entry(line, &bank->line_list, siblings) {
+			if (line->hog)
+				num_hogs++;
+		}
+	}
+
+	if (!num_hogs)
+		return 0;
+
+	/* Allocate one more for the sentinel. */
+	dev->hogs = kcalloc(num_hogs + 1, sizeof(*dev->hogs), GFP_KERNEL);
+	if (!dev->hogs)
+		return -ENOMEM;
+
+	list_for_each_entry(bank, &dev->bank_list, siblings) {
+		list_for_each_entry(line, &bank->line_list, siblings) {
+			if (!line->hog)
+				continue;
+
+			hog = &dev->hogs[idx++];
+
+			/*
+			 * We need to make this string manually because at this
+			 * point the device doesn't exist yet and so dev_name()
+			 * is not available.
+			 */
+			hog->chip_label = kasprintf(GFP_KERNEL,
+						    "gpio-sim.%u-%s", dev->id,
+						    fwnode_get_name(bank->swnode));
+			if (!hog->chip_label) {
+				gpio_sim_remove_hogs(dev);
+				return -ENOMEM;
+			}
+
+			/*
+			 * We need to duplicate this because the hog config
+			 * item can be removed at any time (and we can't block
+			 * it) and gpiolib doesn't make a deep copy of the hog
+			 * data.
+			 */
+			if (line->hog->name) {
+				hog->line_name = kstrdup(line->hog->name,
+							 GFP_KERNEL);
+				if (!hog->line_name) {
+					gpio_sim_remove_hogs(dev);
+					return -ENOMEM;
+				}
+			}
+
+			hog->chip_hwnum = line->offset;
+			hog->dflags = line->hog->dir;
+		}
+	}
+
+	gpiod_add_hogs(dev->hogs);
+
+	return 0;
+}
+
+static struct fwnode_handle *
+gpio_sim_make_bank_swnode(struct gpio_sim_bank *bank,
+			  struct fwnode_handle *parent)
+{
+	struct property_entry properties[GPIO_SIM_PROP_MAX];
+	unsigned int prop_idx = 0, line_names_size = 0;
+	struct fwnode_handle *swnode;
+	char **line_names;
+
+	memset(properties, 0, sizeof(properties));
+
+	properties[prop_idx++] = PROPERTY_ENTRY_U32("ngpios", bank->num_lines);
+
+	if (bank->label)
+		properties[prop_idx++] = PROPERTY_ENTRY_STRING("gpio-sim,label",
+							       bank->label);
+
+	line_names = gpio_sim_make_line_names(bank, &line_names_size);
+	if (IS_ERR(line_names))
+		return ERR_CAST(line_names);
+
+	if (line_names)
+		properties[prop_idx++] = PROPERTY_ENTRY_STRING_ARRAY_LEN(
+						"gpio-line-names",
+						line_names, line_names_size);
+
+	swnode = fwnode_create_software_node(properties, parent);
+	kfree(line_names);
+	return swnode;
+}
+
+static void gpio_sim_remove_swnode_recursive(struct fwnode_handle *swnode)
+{
+	struct fwnode_handle *child;
+
+	fwnode_for_each_child_node(swnode, child)
+		fwnode_remove_software_node(child);
+
+	fwnode_remove_software_node(swnode);
+}
+
+static bool gpio_sim_bank_labels_non_unique(struct gpio_sim_device *dev)
+{
+	struct gpio_sim_bank *this, *pos;
+
+	list_for_each_entry(this, &dev->bank_list, siblings) {
+		list_for_each_entry(pos, &dev->bank_list, siblings) {
+			if (this == pos || (!this->label || !pos->label))
+				continue;
+
+			if (strcmp(this->label, pos->label) == 0)
+				return true;
+		}
+	}
+
+	return false;
+}
+
+static int gpio_sim_device_activate_unlocked(struct gpio_sim_device *dev)
+{
+	struct platform_device_info pdevinfo;
+	struct fwnode_handle *swnode;
+	struct platform_device *pdev;
+	struct gpio_sim_bank *bank;
+	int ret;
+
+	if (list_empty(&dev->bank_list))
+		return -ENODATA;
+
+	/*
+	 * Non-unique GPIO device labels are a corner-case we don't support
+	 * as it would interfere with machine hogging mechanism and has little
+	 * use in real life.
+	 */
+	if (gpio_sim_bank_labels_non_unique(dev))
+		return -EINVAL;
+
+	memset(&pdevinfo, 0, sizeof(pdevinfo));
+
+	swnode = fwnode_create_software_node(NULL, NULL);
+	if (IS_ERR(swnode))
+		return PTR_ERR(swnode);
+
+	list_for_each_entry(bank, &dev->bank_list, siblings) {
+		bank->swnode = gpio_sim_make_bank_swnode(bank, swnode);
+		if (ret) {
+			gpio_sim_remove_swnode_recursive(swnode);
+			return ret;
+		}
+	}
+
+	ret = gpio_sim_add_hogs(dev);
+	if (ret) {
+		gpio_sim_remove_swnode_recursive(swnode);
+		return ret;
+	}
+
+	pdevinfo.name = "gpio-sim";
+	pdevinfo.fwnode = swnode;
+	pdevinfo.id = dev->id;
+
+	reinit_completion(&dev->probe_completion);
+	dev->driver_bound = false;
+	bus_register_notifier(&platform_bus_type, &dev->bus_notifier);
+
+	pdev = platform_device_register_full(&pdevinfo);
+	if (IS_ERR(pdev)) {
+		bus_unregister_notifier(&platform_bus_type, &dev->bus_notifier);
+		gpio_sim_remove_hogs(dev);
+		gpio_sim_remove_swnode_recursive(swnode);
+		return PTR_ERR(pdev);
+	}
+
+	wait_for_completion(&dev->probe_completion);
+	bus_unregister_notifier(&platform_bus_type, &dev->bus_notifier);
+
+	if (!dev->driver_bound) {
+		/* Probe failed, check kernel log. */
+		platform_device_unregister(pdev);
+		gpio_sim_remove_hogs(dev);
+		gpio_sim_remove_swnode_recursive(swnode);
+		return -ENXIO;
+	}
+
+	dev->pdev = pdev;
+
+	return 0;
+}
+
+static void gpio_sim_device_deactivate_unlocked(struct gpio_sim_device *dev)
+{
+	struct fwnode_handle *swnode;
+
+	swnode = dev_fwnode(&dev->pdev->dev);
+	platform_device_unregister(dev->pdev);
+	gpio_sim_remove_swnode_recursive(swnode);
+	dev->pdev = NULL;
+	gpio_sim_remove_hogs(dev);
+}
+
+static ssize_t
+gpio_sim_device_config_live_store(struct config_item *item,
+				  const char *page, size_t count)
+{
+	struct gpio_sim_device *dev = to_gpio_sim_device(item);
+	int live, ret;
+
+	ret = kstrtouint(page, 10, &live);
+	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);
+	else
+		ret = -EINVAL;
+
+	mutex_unlock(&dev->lock);
+
+	return ret ?: count;
+}
+
+CONFIGFS_ATTR(gpio_sim_device_config_, live);
+
+static struct configfs_attribute *gpio_sim_device_config_attrs[] = {
+	&gpio_sim_device_config_attr_dev_name,
+	&gpio_sim_device_config_attr_live,
+	NULL
+};
+
+struct gpio_sim_chip_name_ctx {
+	struct gpio_sim_device *dev;
+	char *page;
+};
+
+static int gpio_sim_emit_chip_name(struct device *dev, void *data)
+{
+	struct gpio_sim_chip_name_ctx *ctx = data;
+	struct fwnode_handle *swnode;
+	struct gpio_sim_bank *bank;
+
+	/* This would be the sysfs device exported in /sys/class/gpio. */
+	if (dev->class)
+		return 0;
+
+	swnode = dev_fwnode(dev);
+
+	list_for_each_entry(bank, &ctx->dev->bank_list, siblings) {
+		if (bank->swnode == swnode)
+			return sprintf(ctx->page, "%s\n", dev_name(dev));
+	}
+
+	return -ENODATA;
+}
+
+static ssize_t gpio_sim_bank_config_chip_name_show(struct config_item *item,
+						   char *page)
+{
+	struct gpio_sim_bank *bank = to_gpio_sim_bank(item);
+	struct gpio_sim_device *dev = gpio_sim_bank_get_device(bank);
+	struct gpio_sim_chip_name_ctx ctx = { dev, page };
+	int ret;
+
+	mutex_lock(&dev->lock);
+	if (gpio_sim_device_is_live_unlocked(dev))
+		ret = device_for_each_child(&dev->pdev->dev, &ctx,
+					    gpio_sim_emit_chip_name);
+	else
+		ret = sprintf(page, "none\n");
+	mutex_unlock(&dev->lock);
+
+	return ret;
+}
+
+CONFIGFS_ATTR_RO(gpio_sim_bank_config_, chip_name);
+
+static ssize_t
+gpio_sim_bank_config_label_show(struct config_item *item, char *page)
+{
+	struct gpio_sim_bank *bank = to_gpio_sim_bank(item);
+	struct gpio_sim_device *dev = gpio_sim_bank_get_device(bank);
+	int ret;
+
+	mutex_lock(&dev->lock);
+	ret = sprintf(page, "%s\n", bank->label ?: "");
+	mutex_unlock(&dev->lock);
+
+	return ret;
+}
+
+static ssize_t gpio_sim_bank_config_label_store(struct config_item *item,
+						const char *page, size_t count)
+{
+	struct gpio_sim_bank *bank = to_gpio_sim_bank(item);
+	struct gpio_sim_device *dev = gpio_sim_bank_get_device(bank);
+	char *trimmed;
+
+	mutex_lock(&dev->lock);
+
+	if (gpio_sim_device_is_live_unlocked(dev)) {
+		mutex_unlock(&dev->lock);
+		return -EBUSY;
+	}
+
+	trimmed = gpio_sim_strdup_trimmed(page, count);
+	if (!trimmed) {
+		mutex_unlock(&dev->lock);
+		return -ENOMEM;
+	}
+
+	kfree(bank->label);
+	bank->label = trimmed;
+
+	mutex_unlock(&dev->lock);
+	return count;
+}
+
+CONFIGFS_ATTR(gpio_sim_bank_config_, label);
+
+static ssize_t
+gpio_sim_bank_config_num_lines_show(struct config_item *item, char *page)
+{
+	struct gpio_sim_bank *bank = to_gpio_sim_bank(item);
+	struct gpio_sim_device *dev = gpio_sim_bank_get_device(bank);
+	int ret;
+
+	mutex_lock(&dev->lock);
+	ret = sprintf(page, "%u\n", bank->num_lines);
+	mutex_unlock(&dev->lock);
+
+	return ret;
+}
+
+static ssize_t
+gpio_sim_bank_config_num_lines_store(struct config_item *item,
+				     const char *page, size_t count)
+{
+	struct gpio_sim_bank *bank = to_gpio_sim_bank(item);
+	struct gpio_sim_device *dev = gpio_sim_bank_get_device(bank);
+	unsigned int num_lines;
+	int ret;
+
+	ret = kstrtouint(page, 10, &num_lines);
+	if (ret)
+		return ret;
+
+	if (num_lines == 0)
+		return -EINVAL;
+
+	mutex_lock(&dev->lock);
+
+	if (gpio_sim_device_is_live_unlocked(dev)) {
+		mutex_unlock(&dev->lock);
+		return -EBUSY;
+	}
+
+	bank->num_lines = num_lines;
+
+	mutex_unlock(&dev->lock);
+	return count;
+}
+
+CONFIGFS_ATTR(gpio_sim_bank_config_, num_lines);
+
+static struct configfs_attribute *gpio_sim_bank_config_attrs[] = {
+	&gpio_sim_bank_config_attr_chip_name,
+	&gpio_sim_bank_config_attr_label,
+	&gpio_sim_bank_config_attr_num_lines,
+	NULL
+};
+
+static ssize_t
+gpio_sim_line_config_name_show(struct config_item *item, char *page)
+{
+	struct gpio_sim_line *line = to_gpio_sim_line(item);
+	struct gpio_sim_device *dev = gpio_sim_line_get_device(line);
+	int ret;
+
+	mutex_lock(&dev->lock);
+	ret = sprintf(page, "%s\n", line->name ?: "");
+	mutex_unlock(&dev->lock);
+
+	return ret;
+}
+
+static ssize_t gpio_sim_line_config_name_store(struct config_item *item,
+					       const char *page, size_t count)
+{
+	struct gpio_sim_line *line = to_gpio_sim_line(item);
+	struct gpio_sim_device *dev = gpio_sim_line_get_device(line);
+	char *trimmed;
+
+	mutex_lock(&dev->lock);
+
+	if (gpio_sim_device_is_live_unlocked(dev)) {
+		mutex_unlock(&dev->lock);
+		return -EBUSY;
+	}
+
+	trimmed = gpio_sim_strdup_trimmed(page, count);
+	if (!trimmed) {
+		mutex_unlock(&dev->lock);
+		return -ENOMEM;
+	}
+
+	kfree(line->name);
+	line->name = trimmed;
+
+	mutex_unlock(&dev->lock);
+
+	return count;
+}
+
+CONFIGFS_ATTR(gpio_sim_line_config_, name);
+
+static struct configfs_attribute *gpio_sim_line_config_attrs[] = {
+	&gpio_sim_line_config_attr_name,
+	NULL,
+};
+
+static ssize_t gpio_sim_hog_config_name_show(struct config_item *item,
+					     char *page)
+{
+	struct gpio_sim_hog *hog = to_gpio_sim_hog(item);
+	struct gpio_sim_device *dev = gpio_sim_hog_get_device(hog);
+	int ret;
+
+	mutex_lock(&dev->lock);
+	ret = sprintf(page, "%s\n", hog->name ?: "");
+	mutex_unlock(&dev->lock);
+
+	return ret;
+}
+
+static ssize_t gpio_sim_hog_config_name_store(struct config_item *item,
+					      const char *page, size_t count)
+{
+	struct gpio_sim_hog *hog = to_gpio_sim_hog(item);
+	struct gpio_sim_device *dev = gpio_sim_hog_get_device(hog);
+	char *trimmed;
+
+	mutex_lock(&dev->lock);
+
+	if (gpio_sim_device_is_live_unlocked(dev)) {
+		mutex_unlock(&dev->lock);
+		return -EBUSY;
+	}
+
+	trimmed = gpio_sim_strdup_trimmed(page, count);
+	if (!trimmed) {
+		mutex_unlock(&dev->lock);
+		return -ENOMEM;
+	}
+
+	kfree(hog->name);
+	hog->name = trimmed;
+
+	mutex_unlock(&dev->lock);
+
+	return count;
+}
+
+CONFIGFS_ATTR(gpio_sim_hog_config_, name);
+
+static ssize_t gpio_sim_hog_config_direction_show(struct config_item *item,
+						  char *page)
+{
+	struct gpio_sim_hog *hog = to_gpio_sim_hog(item);
+	struct gpio_sim_device *dev = gpio_sim_hog_get_device(hog);
+	char *repr;
+	int dir;
+
+	mutex_lock(&dev->lock);
+	dir = hog->dir;
+	mutex_unlock(&dev->lock);
+
+	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;
+	}
+
+	return sprintf(page, "%s\n", repr);
+}
+
+static ssize_t
+gpio_sim_hog_config_direction_store(struct config_item *item,
+				    const char *page, size_t count)
+{
+	struct gpio_sim_hog *hog = to_gpio_sim_hog(item);
+	struct gpio_sim_device *dev = gpio_sim_hog_get_device(hog);
+	char *trimmed;
+	int dir;
+
+	mutex_lock(&dev->lock);
+
+	if (gpio_sim_device_is_live_unlocked(dev)) {
+		mutex_unlock(&dev->lock);
+		return -EBUSY;
+	}
+
+	trimmed = gpio_sim_strdup_trimmed(page, count);
+	if (!trimmed) {
+		mutex_unlock(&dev->lock);
+		return -ENOMEM;
+	}
+
+	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;
+	else
+		dir = -EINVAL;
+
+	kfree(trimmed);
+
+	if (dir < 0) {
+		mutex_unlock(&dev->lock);
+		return dir;
+	}
+
+	hog->dir = dir;
+
+	mutex_unlock(&dev->lock);
+
+	return count;
+}
+
+CONFIGFS_ATTR(gpio_sim_hog_config_, direction);
+
+static struct configfs_attribute *gpio_sim_hog_config_attrs[] = {
+	&gpio_sim_hog_config_attr_name,
+	&gpio_sim_hog_config_attr_direction,
+	NULL,
+};
+
+static void gpio_sim_hog_config_item_release(struct config_item *item)
+{
+	struct gpio_sim_hog *hog = to_gpio_sim_hog(item);
+	struct gpio_sim_line *line = hog->parent;
+	struct gpio_sim_device *dev = gpio_sim_hog_get_device(hog);
+
+	mutex_lock(&dev->lock);
+	line->hog = NULL;
+	mutex_unlock(&dev->lock);
+
+	kfree(hog->name);
+	kfree(hog);
+}
+
+struct configfs_item_operations gpio_sim_hog_config_item_ops = {
+	.release	= gpio_sim_hog_config_item_release,
+};
+
+static const struct config_item_type gpio_sim_hog_config_type = {
+	.ct_item_ops	= &gpio_sim_hog_config_item_ops,
+	.ct_attrs	= gpio_sim_hog_config_attrs,
+	.ct_owner	= THIS_MODULE,
+};
+
+static struct config_item *
+gpio_sim_line_config_make_hog_item(struct config_group *group, const char *name)
+{
+	struct gpio_sim_line *line = to_gpio_sim_line(&group->cg_item);
+	struct gpio_sim_device *dev = gpio_sim_line_get_device(line);
+	struct gpio_sim_hog *hog;
+
+	if (strcmp(name, "hog") != 0)
+		return ERR_PTR(-EINVAL);
+
+	mutex_lock(&dev->lock);
+
+	hog = kzalloc(sizeof(*hog), GFP_KERNEL);
+	if (!hog) {
+		mutex_unlock(&dev->lock);
+		return ERR_PTR(-ENOMEM);
+	}
+
+	config_item_init_type_name(&hog->item, name,
+				   &gpio_sim_hog_config_type);
+
+	hog->dir = GPIOD_IN;
+	hog->name = NULL;
+	hog->parent = line;
+	line->hog = hog;
+
+	mutex_unlock(&dev->lock);
+
+	return &hog->item;
+}
+
+static void gpio_sim_line_config_group_release(struct config_item *item)
+{
+	struct gpio_sim_line *line = to_gpio_sim_line(item);
+	struct gpio_sim_device *dev = gpio_sim_line_get_device(line);
+
+	mutex_lock(&dev->lock);
+	list_del(&line->siblings);
+	mutex_unlock(&dev->lock);
+
+	kfree(line->name);
+	kfree(line);
+}
+
+static struct configfs_item_operations gpio_sim_line_config_item_ops = {
+	.release	= gpio_sim_line_config_group_release,
+};
+
+static struct configfs_group_operations gpio_sim_line_config_group_ops = {
+	.make_item	= gpio_sim_line_config_make_hog_item,
+};
+
+static const struct config_item_type gpio_sim_line_config_type = {
+	.ct_item_ops	= &gpio_sim_line_config_item_ops,
+	.ct_group_ops	= &gpio_sim_line_config_group_ops,
+	.ct_attrs	= gpio_sim_line_config_attrs,
+	.ct_owner       = THIS_MODULE,
+};
+
+static struct config_group *
+gpio_sim_bank_config_make_line_group(struct config_group *group,
+				     const char *name)
+{
+	struct gpio_sim_bank *bank = to_gpio_sim_bank(&group->cg_item);
+	struct gpio_sim_device *dev = gpio_sim_bank_get_device(bank);
+	struct gpio_sim_line *line;
+	unsigned int offset;
+	int ret, nchar;
+
+	ret = sscanf(name, "line%u%n", &offset, &nchar);
+	if (ret != 1 || nchar != strlen(name))
+		return ERR_PTR(-EINVAL);
+
+	mutex_lock(&dev->lock);
+
+	if (gpio_sim_device_is_live_unlocked(dev)) {
+		mutex_unlock(&dev->lock);
+		return ERR_PTR(-EBUSY);
+	}
+
+	line = kzalloc(sizeof(*line), GFP_KERNEL);
+	if (!line) {
+		mutex_unlock(&dev->lock);
+		return ERR_PTR(-ENOMEM);
+	}
+
+	config_group_init_type_name(&line->group, name,
+				    &gpio_sim_line_config_type);
+
+	line->parent = bank;
+	line->offset = offset;
+	list_add_tail(&line->siblings, &bank->line_list);
+
+	mutex_unlock(&dev->lock);
+
+	return &line->group;
+}
+
+static void gpio_sim_bank_config_group_release(struct config_item *item)
+{
+	struct gpio_sim_bank *bank = to_gpio_sim_bank(item);
+	struct gpio_sim_device *dev = gpio_sim_bank_get_device(bank);
+
+	mutex_lock(&dev->lock);
+	list_del(&bank->siblings);
+	mutex_unlock(&dev->lock);
+
+	kfree(bank->label);
+	kfree(bank);
+}
+
+static struct configfs_item_operations gpio_sim_bank_config_item_ops = {
+	.release	= gpio_sim_bank_config_group_release,
+};
+
+static struct configfs_group_operations gpio_sim_bank_config_group_ops = {
+	.make_group	= gpio_sim_bank_config_make_line_group,
+};
+
+static const struct config_item_type gpio_sim_bank_config_group_type = {
+	.ct_item_ops	= &gpio_sim_bank_config_item_ops,
+	.ct_group_ops	= &gpio_sim_bank_config_group_ops,
+	.ct_attrs	= gpio_sim_bank_config_attrs,
+	.ct_owner	= THIS_MODULE,
+};
+
+static struct config_group *
+gpio_sim_device_config_make_bank_group(struct config_group *group,
+				       const char *name)
+{
+	struct gpio_sim_device *dev = to_gpio_sim_device(&group->cg_item);
+	struct gpio_sim_bank *bank;
+
+	mutex_lock(&dev->lock);
+
+	if (gpio_sim_device_is_live_unlocked(dev)) {
+		mutex_unlock(&dev->lock);
+		return ERR_PTR(-EBUSY);
+	}
+
+	bank = kzalloc(sizeof(*bank), GFP_KERNEL);
+	if (!bank) {
+		mutex_unlock(&dev->lock);
+		return ERR_PTR(-ENOMEM);
+	}
+
+	config_group_init_type_name(&bank->group, name,
+				    &gpio_sim_bank_config_group_type);
+	bank->num_lines = 1;
+	bank->parent = dev;
+	INIT_LIST_HEAD(&bank->line_list);
+	list_add_tail(&bank->siblings, &dev->bank_list);
+
+	mutex_unlock(&dev->lock);
+
+	return &bank->group;
+}
+
+static void gpio_sim_device_config_group_release(struct config_item *item)
+{
+	struct gpio_sim_device *dev = to_gpio_sim_device(item);
+
+	mutex_lock(&dev->lock);
+	if (gpio_sim_device_is_live_unlocked(dev))
+		gpio_sim_device_deactivate_unlocked(dev);
+	mutex_unlock(&dev->lock);
+
+	mutex_destroy(&dev->lock);
+	ida_free(&gpio_sim_ida, dev->id);
+	kfree(dev);
+}
+
+static struct configfs_item_operations gpio_sim_device_config_item_ops = {
+	.release	= gpio_sim_device_config_group_release,
+};
+
+static struct configfs_group_operations gpio_sim_device_config_group_ops = {
+	.make_group	= gpio_sim_device_config_make_bank_group,
+};
+
+static const struct config_item_type gpio_sim_device_config_group_type = {
+	.ct_item_ops	= &gpio_sim_device_config_item_ops,
+	.ct_group_ops	= &gpio_sim_device_config_group_ops,
+	.ct_attrs	= gpio_sim_device_config_attrs,
+	.ct_owner	= THIS_MODULE,
+};
+
+static struct config_group *
+gpio_sim_config_make_device_group(struct config_group *group, const char *name)
+{
+	struct gpio_sim_device *dev;
+	int id;
+
+	dev = kzalloc(sizeof(*dev), GFP_KERNEL);
+	if (!dev)
+		return ERR_PTR(-ENOMEM);
+
+	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;
+	mutex_init(&dev->lock);
+	INIT_LIST_HEAD(&dev->bank_list);
+
+	dev->bus_notifier.notifier_call = gpio_sim_bus_notifier_call;
+	init_completion(&dev->probe_completion);
+
+	return &dev->group;
+}
+
+static struct configfs_group_operations gpio_sim_config_group_ops = {
+	.make_group	= gpio_sim_config_make_device_group,
+};
+
+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 <brgl@bgdev.pl");
+MODULE_DESCRIPTION("GPIO Simulator Module");
+MODULE_LICENSE("GPL");