lkml.org 
[lkml]   [2019]   [Dec]   [12]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    Date
    SubjectRe: [PATCH v3 5/7] gpio: Add GPIO Aggregator/Repeater driver
    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

    \
     
     \ /
      Last update: 2019-12-12 16:25    [W:2.481 / U:0.140 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site