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 [thread overview]
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
next prev parent reply other threads:[~2019-12-12 15:24 UTC|newest]
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
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).