Linux-Devicetree Archive on lore.kernel.org
 help / color / Atom feed
From: Geert Uytterhoeven <geert@linux-m68k.org>
To: Linus Walleij <linus.walleij@linaro.org>
Cc: Geert Uytterhoeven <geert+renesas@glider.be>,
	Bartosz Golaszewski <bgolaszewski@baylibre.com>,
	Jonathan Corbet <corbet@lwn.net>,
	Rob Herring <robh+dt@kernel.org>,
	Mark Rutland <mark.rutland@arm.com>,
	Harish Jenny K N <harish_kandiga@mentor.com>,
	Eugeniu Rosca <erosca@de.adit-jv.com>,
	Alexander Graf <graf@amazon.com>,
	Peter Maydell <peter.maydell@linaro.org>,
	Paolo Bonzini <pbonzini@redhat.com>,
	Phil Reid <preid@electromag.com.au>,
	Marc Zyngier <marc.zyngier@arm.com>,
	Christoffer Dall <christoffer.dall@arm.com>,
	Magnus Damm <magnus.damm@gmail.com>,
	"open list:GPIO SUBSYSTEM" <linux-gpio@vger.kernel.org>,
	Linux Doc Mailing List <linux-doc@vger.kernel.org>,
	"open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS" 
	<devicetree@vger.kernel.org>,
	Linux-Renesas <linux-renesas-soc@vger.kernel.org>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	QEMU Developers <qemu-devel@nongnu.org>
Subject: Re: [PATCH v3 5/7] gpio: Add GPIO Aggregator/Repeater driver
Date: Thu, 12 Dec 2019 16:24:19 +0100
Message-ID: <CAMuHMdVbk5S__8OK-zNXmiW66=WVA8Jzyc=hUvf_hJSU=u9TFg@mail.gmail.com> (raw)
In-Reply-To: <CACRpkdaW7nmpE99FAvBDBTmkTZOTQ5WdM=JbMzBTLk7cbLRXPw@mail.gmail.com>

Hi Linus,

On Thu, Dec 12, 2019 at 3:34 PM Linus Walleij <linus.walleij@linaro.org> wrote:
> On Wed, Nov 27, 2019 at 9:43 AM Geert Uytterhoeven
> <geert+renesas@glider.be> wrote:
> > GPIO controllers are exported to userspace using /dev/gpiochip*
> > character devices.  Access control to these devices is provided by
> > standard UNIX file system permissions, on an all-or-nothing basis:
> > either a GPIO controller is accessible for a user, or it is not.
> > Currently no mechanism exists to control access to individual GPIOs.
> >
> > Hence add a GPIO driver to aggregate existing GPIOs, and expose them as
> > a new gpiochip.
> >
> > This supports the following use cases:
> >   1. Aggregating GPIOs using Sysfs
> >      This is useful for implementing access control, and assigning a set
> >      of GPIOs to a specific user or virtual machine.
> >
> >   2. GPIO Repeater in Device Tree
> >      This supports modelling e.g. GPIO inverters in DT.
> >
> >   3. Generic GPIO Driver
> >      This provides userspace access to a simple GPIO-operated device
> >      described in DT, cfr. e.g. spidev for SPI-operated devices.
> >
> > Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
>
> Overall I like how this is developing!
>
> > +config GPIO_AGGREGATOR
> > +       tristate "GPIO Aggregator/Repeater"
> > +       help
> > +         Say yes here to enable the GPIO Aggregator and repeater, which
> > +         provides a way to aggregate and/or repeat existing GPIOs into a new
> > +         GPIO device.
>
> Should it say a "new virtual GPIO chip"?

OK.

> > +         This can serve the following purposes:
> > +           1. Assign a collection of GPIOs to a user, or export them to a
> > +              virtual machine,
>
> This is ambiguous. What is a "user"? A process calling from
> userspace? A device tree node?

A user is an entity with a UID, typically listed in /etc/passwd.
This is similar to letting some, not all, people on the machine access
the CD-ROM drive.

> I would write "assign a collection of GPIO lines from any lines on
> existing physical GPIO chips to form a new virtual GPIO chip"
>
> That should be to the point, right?

Yes, that's WHAT it does. The WHY is the granular access control.

> > +           2. Support GPIOs that are connected to a physical inverter,
>
> s/to/through/g

OK.

> > +           3. Provide a generic driver for a GPIO-operated device, to be
> > +               controlled from userspace using the GPIO chardev interface.
>
> I don't understand this, it needs to be elaborated. What is meant
> by a "GPIO-operated device" in this context? Example?

E.g. a motor. Or a door opener.

        door-opener {
                compatible = "mydoor,opener";

                gpios = <&gpio2 19 GPIO_ACTIVE_HIGH>;
        };

You don't need a full-featured kernel driver for that, so just bind the
gpio-aggregator to the door-opener, and control it through libgpiod.

> I consistently use the term "GPIO line" as opposed to "GPIO"
> or "GPIO number" etc that are abigous, so please rephrase using
> "GPIO lines" rather than just "GPIOs" above.

OK.

> > +#include "gpiolib.h"
>
> Whenever this is included in a driver I want it to come with a comment
> explicitly stating exactly why and which internal symbols the driver
> needs to access. Ideally all drivers should just need <linux/gpio/driver.h>...

"gpiolib.h" is needed to access gpio_desc.gdev->chip in
gpio_fwd_set_config().  And for gpio_chip_hwgpio() (see below).

But indeed, I should add #include <linux/gpio/consumer.h>, for e.g. the
various gpiod_[gs]et_*() functions.

> > +static int aggr_add_gpio(struct gpio_aggregator *aggr, const char *label,
> > +                        int hwnum, unsigned int *n)
>
> u16 hwnum for the hardware number but if it is always -1/U16_MAX
> then why pass the parameter at all.
>
> Is "label" the right name of this parameter if that is going to actually
> be line_name then use that.

It's not always -1.
This function can be called either with a gpiochip label/name and an
offset, or a line-name and -1.

> > +{
> > +       struct gpiod_lookup_table *lookups;
> > +
> > +       lookups = krealloc(aggr->lookups, struct_size(lookups, table, *n + 2),
> > +                          GFP_KERNEL);
> > +       if (!lookups)
> > +               return -ENOMEM;
> > +
> > +       lookups->table[*n].chip_label = label;
>
> This is pending the discussion on whether to just use "key" for this
> name.

Which would require touching all users (board files and mfd drivers).

> > +       lookups->table[*n].chip_hwnum = hwnum;
>
> If this is always going to be U16_MAX (-1 in the current code)
> then it can just be assigned as that here instead of passed as
> parameter.

So it's not, see above.

> > +static int aggr_parse(struct gpio_aggregator *aggr)
> > +{
> > +       char *name, *offsets, *first, *last, *next;
> > +       unsigned int a, b, i, n = 0;
> > +       char *args = aggr->args;
> > +       int error;
> > +
> > +       for (name = get_arg(&args), offsets = get_arg(&args); name;
> > +            offsets = get_arg(&args)) {
> > +               if (IS_ERR(name)) {
> > +                       pr_err("Cannot get GPIO specifier: %ld\n",
> > +                              PTR_ERR(name));
> > +                       return PTR_ERR(name);
> > +               }
> > +
> > +               if (!isrange(offsets)) {
> > +                       /* Named GPIO line */
> > +                       error = aggr_add_gpio(aggr, name, -1, &n);
>
> So the third argument woule be U16_MAX here. Or not pass
> a parameter at all.
>
> But honestly, when I look at this I don't understand why you
> have to avoid so hard to use offsets for the GPIO lines on
> your aggregator?
>
> Just put a u16 ngpios in your
> struct gpio_aggregator and count it up every time you
> add some new offsets here and you have
> offset numbers for all your GPIO lines on the aggregator
> and you can just drop the patch for lookup up lines by line
> names.
>
> Is there something wrong with my reasoning here?

Yes, I think there is.
The offsets are not offsets on the aggregated gpiochip, but on the
original target gpiochip.

> At the pointe later when the lines are counted from the
> allocated lookups using gpiod_count() that will just figure
> out this number anyways, so it is not like we don't know
> it at the end of the day.
>
> So it seems the patch to gpiolib is just to use machine
> descriptor tables as a substitute for a simple counter
> variable in this local struct to me.

Nope, it's used for looking up the target GPIO lines.

> > +static void __exit gpio_aggregator_remove_all(void)
> > +{
> > +       mutex_lock(&gpio_aggregator_lock);
> > +       idr_for_each(&gpio_aggregator_idr, gpio_aggregator_idr_remove, NULL);
> > +       idr_destroy(&gpio_aggregator_idr);
> > +       mutex_unlock(&gpio_aggregator_lock);
> > +}
> > +
> > +
> > +       /*
> > +        *  Common GPIO Forwarder
> > +        */
> > +
>
> Nitpick: lots and weird spacing here.

OK.

> > +struct gpiochip_fwd {
> > +       struct gpio_chip chip;
> > +       struct gpio_desc **descs;
> > +       union {
> > +               struct mutex mlock;     /* protects tmp[] if can_sleep */
> > +               spinlock_t slock;       /* protects tmp[] if !can_sleep */
> > +       };
>
> That was a very elegant use of union!
>
> > +static int gpio_fwd_get_multiple(struct gpio_chip *chip, unsigned long *mask,
> > +                                unsigned long *bits)
> > +static void gpio_fwd_set_multiple(struct gpio_chip *chip, unsigned long *mask,
> > +                                 unsigned long *bits)
>
> I guess these can both be optimized to use get/set_multiple on
> the target chip if the offsets are consecutive?
>
> However that is going to be tricky so I'm not saying you should
> implement that. So for now, let's say just add a TODO: comment
> about it.

Doesn't gpiod_[gs]et_array_value() already call .[gs]et_multiple()?

> > +static int gpio_fwd_init_valid_mask(struct gpio_chip *chip,
> > +                                   unsigned long *valid_mask,
> > +                                   unsigned int ngpios)
> > +{
> > +       struct gpiochip_fwd *fwd = gpiochip_get_data(chip);
> > +       unsigned int i;
> > +
> > +       for (i = 0; i < ngpios; i++) {
> > +               if (!gpiochip_line_is_valid(fwd->descs[i]->gdev->chip,
> > +                                           gpio_chip_hwgpio(fwd->descs[i])))
> > +                       clear_bit(i, valid_mask);
> > +       }
>
> This is what uses "gpiolib.h" is it not?
>
> devm_gpiod_get_index() will not succeed if the line
> is not valid so I think this can be just dropped, since
> what you do before this is exactly devm_gpiod_get_index()
> on each line, then you call gpiochip_fwd_create()
> with the result.
>
> So I think you can just drop this entire function.
> This will not happen.

OK, if all lines are valid, the mask handling is indeed not needed.

> If it does happen, add a comment above this loop
> explaining which circumstances would make lines on
> the forwarder invalid.

OK, so cannot happen.

> > +       for (i = 0; i < ngpios; i++) {
> > +               dev_dbg(dev, "gpio %u => gpio-%d (%s)\n", i,
> > +                       desc_to_gpio(descs[i]), descs[i]->label ? : "?");
> > +
> > +               if (gpiod_cansleep(descs[i]))
> > +                       chip->can_sleep = true;
> > +               if (descs[i]->gdev->chip->set_config)
> > +                       chip->set_config = gpio_fwd_set_config;
> > +               if (descs[i]->gdev->chip->init_valid_mask)
> > +                       chip->init_valid_mask = gpio_fwd_init_valid_mask;
> > +       }
>
> I do not think you should need to inspect the init_valid_mask()
> as explained above.

OK.

> Add a comment above the loop that if any of the GPIO lines
> are sleeping then the entire forwarder will be sleeping
> and if any of the chips support .set_config() we will support
> setting configs.

OK.

> However the way that the .gpio_fwd_set_config() is coded
> it looks like you can just unconditionally assign it and
> only check the cansleep condition in this loop.

I wanted to avoid the overhead of calling into gpio_fwd_set_config() if
none of the targets gpiochips support .set_config(), see
gpiod_set_transitory().

> > +}
> > +
> > +
> > +       /*
> > +        *  Common GPIO Aggregator/Repeater platform device
> > +        */
> > +
>
> Nitpick: weird and excess spacing again.

Yeah, this dates back from when the aggregator, repeater, and
forwarder were all separate files and modules.

> > +       for (i = 0; i < n; i++) {
> > +               descs[i] = devm_gpiod_get_index(dev, NULL, i, GPIOD_ASIS);
> > +               if (IS_ERR(descs[i]))
> > +                       return PTR_ERR(descs[i]);
> > +       }
>
> If this succeeds none of the obtained gpio_desc:s can be
> invalid.

OK.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

  reply index

Thread overview: 48+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-11-27  8:42 [PATCH v3 0/7] gpio: Add GPIO Aggregator/Repeater Geert Uytterhoeven
2019-11-27  8:42 ` [PATCH v3 1/7] gpiolib: Add GPIOCHIP_NAME definition Geert Uytterhoeven
2019-11-28  3:38   ` Ulrich Hecht
2019-12-02 21:17   ` Eugeniu Rosca
2019-12-12 10:37   ` Linus Walleij
2019-11-27  8:42 ` [PATCH v3 2/7] gpiolib: Add support for gpiochipN-based table lookup Geert Uytterhoeven
2019-11-28  3:38   ` Ulrich Hecht
2019-12-12 13:20   ` Linus Walleij
2019-12-12 13:33     ` Geert Uytterhoeven
2019-12-12 14:36       ` Linus Walleij
2019-11-27  8:42 ` [PATCH v3 3/7] gpiolib: Add support for GPIO line " Geert Uytterhoeven
2019-11-28  3:39   ` Ulrich Hecht
2019-12-12 13:40   ` Linus Walleij
2019-11-27  8:42 ` [PATCH v3 4/7] dt-bindings: gpio: Add gpio-repeater bindings Geert Uytterhoeven
2019-11-28  3:39   ` Ulrich Hecht
2019-12-03  5:51   ` Harish Jenny K N
2019-12-05 21:06   ` Rob Herring
2019-12-06  9:17     ` Geert Uytterhoeven
2019-12-06 15:03       ` Rob Herring
2020-01-06  8:12         ` Geert Uytterhoeven
2020-01-07  9:22           ` Harish Jenny K N
2020-01-16  5:09             ` Harish Jenny K N
2019-11-27  8:42 ` [PATCH v3 5/7] gpio: Add GPIO Aggregator/Repeater driver Geert Uytterhoeven
2019-11-27 14:15   ` Eugeniu Rosca
2019-11-27 14:33     ` Geert Uytterhoeven
2019-11-28  3:40   ` Ulrich Hecht
2019-12-03  5:42   ` Harish Jenny K N
2019-12-03  8:17     ` Geert Uytterhoeven
2019-12-03  8:51       ` Harish Jenny K N
2019-12-03 10:51   ` Eugeniu Rosca
2020-01-09 13:35     ` Geert Uytterhoeven
2020-01-09 13:49       ` Eugeniu Rosca
2019-12-12 14:34   ` Linus Walleij
2019-12-12 15:24     ` Geert Uytterhoeven [this message]
2020-01-04  0:38       ` Linus Walleij
2020-01-06  8:23         ` Geert Uytterhoeven
2020-01-08 23:12           ` Linus Walleij
2019-11-27  8:42 ` [PATCH v3 6/7] docs: gpio: Add GPIO Aggregator/Repeater documentation Geert Uytterhoeven
2019-11-28  3:41   ` Ulrich Hecht
2019-12-12 14:42   ` Linus Walleij
2019-12-12 14:48     ` Geert Uytterhoeven
2020-01-04  0:21       ` Linus Walleij
2020-01-06  8:06         ` Geert Uytterhoeven
2019-11-27  8:42 ` [PATCH v3 7/7] MAINTAINERS: Add GPIO Aggregator/Repeater section Geert Uytterhoeven
2019-12-03  5:38   ` Harish Jenny K N
2020-01-18  1:46 ` [PATCH v3 0/7] gpio: Add GPIO Aggregator/Repeater Eugeniu Rosca
2020-01-20  9:33   ` Geert Uytterhoeven
2020-01-20 12:14     ` Eugeniu Rosca

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to='CAMuHMdVbk5S__8OK-zNXmiW66=WVA8Jzyc=hUvf_hJSU=u9TFg@mail.gmail.com' \
    --to=geert@linux-m68k.org \
    --cc=bgolaszewski@baylibre.com \
    --cc=christoffer.dall@arm.com \
    --cc=corbet@lwn.net \
    --cc=devicetree@vger.kernel.org \
    --cc=erosca@de.adit-jv.com \
    --cc=geert+renesas@glider.be \
    --cc=graf@amazon.com \
    --cc=harish_kandiga@mentor.com \
    --cc=linus.walleij@linaro.org \
    --cc=linux-doc@vger.kernel.org \
    --cc=linux-gpio@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-renesas-soc@vger.kernel.org \
    --cc=magnus.damm@gmail.com \
    --cc=marc.zyngier@arm.com \
    --cc=mark.rutland@arm.com \
    --cc=pbonzini@redhat.com \
    --cc=peter.maydell@linaro.org \
    --cc=preid@electromag.com.au \
    --cc=qemu-devel@nongnu.org \
    --cc=robh+dt@kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Linux-Devicetree Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-devicetree/0 linux-devicetree/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-devicetree linux-devicetree/ https://lore.kernel.org/linux-devicetree \
		devicetree@vger.kernel.org
	public-inbox-index linux-devicetree

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-devicetree


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git