Hi Linus,
On Thu, Dec 12, 2019 at 3:34 PM Linus Walleij <linus.walleij@xxxxxxxxxx> wrote:
> On Wed, Nov 27, 2019 at 9:43 AM Geert Uytterhoeven
> <geert+renesas@xxxxxxxxx> 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@xxxxxxxxx>
>
> 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@xxxxxxxxxxxxxx
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
