2020-05-11 14:56:26

by Geert Uytterhoeven

[permalink] [raw]
Subject: [PATCH v7 0/6] gpio: Add GPIO Aggregator

Hi all,

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 this adds a GPIO driver to aggregate existing GPIOs, and expose
them as a new gpiochip. This is useful for implementing access control,
and assigning a set of GPIOs to a specific user. Furthermore, this
simplifies and hardens exporting GPIOs to a virtual machine, as the VM
can just grab the full GPIO controller, and no longer needs to care
about which GPIOs to grab and which not, reducing the attack surface.
This has been implemented for ARM virt in QEMU[1].

Recently, other use cases have been discovered[2], like describing
simple GPIO-operated devices in DT, and using the GPIO Aggregator as a
generic GPIO driver for userspace, which is useful for industrial
control.

Note that the first patch of this series ("i2c: i801: Use GPIO_LOOKUP()
helper macro") has been applied to i2c/for-next.

Changes compared to v6[3]:
- Document non-uniqueness of line names,
- Rebase on top of commit a0b66a73785ccc8f ("gpio: Rename variable in
core APIs"),
- Maintained => Supported,
- Add Reviewed-by, Acked-by,
- Drop applied patches:
- "ARM: integrator: impd1: Use GPIO_LOOKUP() helper macro",
- "gpiolib: Introduce gpiod_set_config()".

Changes compared to v5[4]:
- Convert raw gpiod_lookup users to GPIO_LOOKUP*(),
- Update Documentation/driver-api/gpio/board.rst for gpiod_lookup
changes,
- Reword rationale behing looking up GPIOs by line name,
- Introduce gpiod_set_config(),
- Use gpiod_to_chip() instead of open-coding,
- Drop debug print of gpio_desc.label, as it usually points to the
GPIO Aggregator itself,
- Drop no longer needed #include "gpiolib.h",
- Fix missing offset translation in gpio_fwd_set_config(),
- Fix "allows" without object,
- Drop "gpiochipN" support,
- Extend example.

Changes compared to v4[5]:
- Add Reviewed-by, Tested-by,
- Fix inconsistent indentation in documentation.

Changes compared to v3[6] (more details in the individual patches):
- Drop controversial GPIO repeater,
- Drop support for legacy sysfs interface based name matching,
- Drop applied "gpiolib: Add GPIOCHIP_NAME definition",
- Documentation improvements,
- Lots of small cleanups.

Changes compared to v2[7] (more details in the individual patches):
- Integrate GPIO Repeater functionality,
- Absorb GPIO forwarder library, as the Aggregator and Repeater are
now a single driver,
- Use the aggregator parameters to create a GPIO lookup table instead
of an array of GPIO descriptors,
- Add documentation,
- New patches:
- "gpiolib: Add GPIOCHIP_NAME definition",
- "gpiolib: Add support for gpiochipN-based table lookup",
- "gpiolib: Add support for GPIO line table lookup",
- "dt-bindings: gpio: Add gpio-repeater bindings",
- "docs: gpio: Add GPIO Aggregator/Repeater documentation",
- "MAINTAINERS: Add GPIO Aggregator/Repeater section".
- Dropped patches:
- "gpio: Export gpiod_{request,free}() to modular GPIO code",
- "gpio: Export gpiochip_get_desc() to modular GPIO code",
- "gpio: Export gpio_name_to_desc() to modular GPIO code",
- "gpio: Add GPIO Forwarder Helper".

Changes compared to v1[8]:
- Drop "virtual", rename to gpio-aggregator,
- Create and use new GPIO Forwarder Helper, to allow sharing code with
the GPIO inverter,
- Lift limit on the maximum number of GPIOs,
- Improve parsing of GPIO specifiers,
- Fix modular build.

Aggregating GPIOs and exposing them as a new gpiochip was suggested in
response to my proof-of-concept for GPIO virtualization with QEMU[9][10].

For the first use case, aggregated GPIO controllers are instantiated and
destroyed by writing to atribute files in sysfs.
Sample session on the Renesas Koelsch development board:

- Unbind LEDs from leds-gpio driver:

echo leds > /sys/bus/platform/drivers/leds-gpio/unbind

- Create aggregators:

$ echo e6052000.gpio 19,20 \
> /sys/bus/platform/drivers/gpio-aggregator/new_device

gpio-aggregator gpio-aggregator.0: gpio 0 => gpio-953
gpio-aggregator gpio-aggregator.0: gpio 1 => gpio-954
gpiochip_find_base: found new base at 758
gpio gpiochip12: (gpio-aggregator.0): added GPIO chardev (254:13)
gpiochip_setup_dev: registered GPIOs 758 to 759 on device: gpiochip12 (gpio-aggregator.0)

$ echo e6052000.gpio 21 e6050000.gpio 20-22 \
> /sys/bus/platform/drivers/gpio-aggregator/new_device

gpio-aggregator gpio-aggregator.1: gpio 0 => gpio-955
gpio-aggregator gpio-aggregator.1: gpio 1 => gpio-1012
gpio-aggregator gpio-aggregator.1: gpio 2 => gpio-1013
gpio-aggregator gpio-aggregator.1: gpio 3 => gpio-1014
gpiochip_find_base: found new base at 754
gpio gpiochip13: (gpio-aggregator.1): added GPIO chardev (254:13)
gpiochip_setup_dev: registered GPIOs 754 to 757 on device: gpiochip13 (gpio-aggregator.1)

- Adjust permissions on /dev/gpiochip1[23] (optional)

- Control LEDs:

$ gpioset gpiochip12 0=0 1=1 # LED6 OFF, LED7 ON
$ gpioset gpiochip12 0=1 1=0 # LED6 ON, LED7 OFF
$ gpioset gpiochip13 0=1 # LED8 ON
$ gpioset gpiochip13 0=0 # LED8 OFF

- Destroy aggregators:

$ echo gpio-aggregator.0 \
> /sys/bus/platform/drivers/gpio-aggregator/delete_device
$ echo gpio-aggregator.1 \
> /sys/bus/platform/drivers/gpio-aggregator/delete_device

To ease testing, I have pushed this series to the
topic/gpio-aggregator-v7 branch of my renesas-drivers repository at
git://git.kernel.org/pub/scm/linux/kernel/git/geert/renesas-drivers.git.

Thanks!

References:
[1] "[PATCH QEMU v2 0/5] Add a GPIO backend"
(https://lore.kernel.org/linux-gpio/[email protected]/)
[2] "[PATCH V4 2/2] gpio: inverter: document the inverter bindings"
(https://lore.kernel.org/r/[email protected]/)
[3] "[PATCH v6 0/8] gpio: Add GPIO Aggregator"
(https://lore.kernel.org/linux-doc/[email protected]/)
[4] "[PATCH v5 0/5] gpio: Add GPIO Aggregator"
(https://lore.kernel.org/r/[email protected]/)
[5] "[PATCH v4 0/5] gpio: Add GPIO Aggregator"
(https://lore.kernel.org/r/[email protected])
[6] "[PATCH v3 0/7] gpio: Add GPIO Aggregator/Repeater"
(https://lore.kernel.org/r/[email protected]/)
[7] "[PATCH/RFC v2 0/5] gpio: Add GPIO Aggregator Driver"
(https://lore.kernel.org/r/[email protected]/)
[8] "[PATCH RFC] gpio: Add Virtual Aggregator GPIO Driver"
(https://lore.kernel.org/r/[email protected]/)
[9] "[PATCH QEMU POC] Add a GPIO backend"
(https://lore.kernel.org/r/[email protected]/)
[10] "Getting To Blinky: Virt Edition / Making device pass-through
work on embedded ARM"
(https://fosdem.org/2019/schedule/event/vai_getting_to_blinky/)

Geert Uytterhoeven (6):
i2c: i801: Use GPIO_LOOKUP() helper macro
mfd: sm501: Use GPIO_LOOKUP_IDX() helper macro
gpiolib: Add support for GPIO lookup by line name
gpio: Add GPIO Aggregator
docs: gpio: Add GPIO Aggregator documentation
MAINTAINERS: Add GPIO Aggregator section

.../admin-guide/gpio/gpio-aggregator.rst | 111 ++++
Documentation/admin-guide/gpio/index.rst | 1 +
Documentation/driver-api/gpio/board.rst | 15 +-
MAINTAINERS | 7 +
drivers/gpio/Kconfig | 12 +
drivers/gpio/Makefile | 1 +
drivers/gpio/gpio-aggregator.c | 568 ++++++++++++++++++
drivers/gpio/gpiolib.c | 22 +-
drivers/i2c/busses/i2c-i801.c | 6 +-
drivers/mfd/sm501.c | 24 +-
include/linux/gpio/machine.h | 17 +-
11 files changed, 748 insertions(+), 36 deletions(-)
create mode 100644 Documentation/admin-guide/gpio/gpio-aggregator.rst
create mode 100644 drivers/gpio/gpio-aggregator.c

--
2.17.1

Gr{oetje,eeting}s,

Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]

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


2020-05-11 14:57:54

by Geert Uytterhoeven

[permalink] [raw]
Subject: [PATCH v7 1/6] i2c: i801: Use GPIO_LOOKUP() helper macro

i801_add_mux() fills in the GPIO lookup table by manually populating an
array of gpiod_lookup structures. Use the existing GPIO_LOOKUP() helper
macro instead, to relax a dependency on the gpiod_lookup structure's
member names.

Signed-off-by: Geert Uytterhoeven <[email protected]>
Reviewed-by: Jean Delvare <[email protected]>
---
This is now commit be1b92c133cc91b2 ("i2c: i801: Use GPIO_LOOKUP()
helper macro") in i2c/for-next.

v7:
- Add Reviewed-by,

v6:
- New.
---
drivers/i2c/busses/i2c-i801.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
index a9c03f5c34825a95..4f333889489c882a 100644
--- a/drivers/i2c/busses/i2c-i801.c
+++ b/drivers/i2c/busses/i2c-i801.c
@@ -1439,9 +1439,9 @@ static int i801_add_mux(struct i801_priv *priv)
return -ENOMEM;
lookup->dev_id = "i2c-mux-gpio";
for (i = 0; i < mux_config->n_gpios; i++) {
- lookup->table[i].chip_label = mux_config->gpio_chip;
- lookup->table[i].chip_hwnum = mux_config->gpios[i];
- lookup->table[i].con_id = "mux";
+ lookup->table[i] = (struct gpiod_lookup)
+ GPIO_LOOKUP(mux_config->gpio_chip,
+ mux_config->gpios[i], "mux", 0);
}
gpiod_add_lookup_table(lookup);
priv->lookup = lookup;
--
2.17.1

2020-05-18 08:19:54

by Linus Walleij

[permalink] [raw]
Subject: Re: [PATCH v7 0/6] gpio: Add GPIO Aggregator

Hi Geert,

I have queued this v7 patch set in an immutable branch for testing and also
merged to my "devel" branch for testing.

If all goes well it also hits linux-next soon.

Yours,
Linus Walleij

2020-05-18 09:25:15

by Geert Uytterhoeven

[permalink] [raw]
Subject: Re: [PATCH v7 0/6] gpio: Add GPIO Aggregator

Hi Linus,

On Mon, May 18, 2020 at 10:17 AM Linus Walleij <[email protected]> wrote:
> I have queued this v7 patch set in an immutable branch for testing and also
> merged to my "devel" branch for testing.
>
> If all goes well it also hits linux-next soon.

Thank a lot!

Back to the QEMU side...

Gr{oetje,eeting}s,

Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]

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

2020-05-20 12:16:17

by Andy Shevchenko

[permalink] [raw]
Subject: Re: [PATCH v7 0/6] gpio: Add GPIO Aggregator

On Mon, May 11, 2020 at 04:52:51PM +0200, Geert Uytterhoeven wrote:
> Hi all,
>
> 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 this adds a GPIO driver to aggregate existing GPIOs, and expose
> them as a new gpiochip. This is useful for implementing access control,
> and assigning a set of GPIOs to a specific user. Furthermore, this
> simplifies and hardens exporting GPIOs to a virtual machine, as the VM
> can just grab the full GPIO controller, and no longer needs to care
> about which GPIOs to grab and which not, reducing the attack surface.
> This has been implemented for ARM virt in QEMU[1].
>
> Recently, other use cases have been discovered[2], like describing
> simple GPIO-operated devices in DT, and using the GPIO Aggregator as a
> generic GPIO driver for userspace, which is useful for industrial
> control.
>
> Note that the first patch of this series ("i2c: i801: Use GPIO_LOOKUP()
> helper macro") has been applied to i2c/for-next.

Sorry for late reply, recently noticed this nice idea.
The comment I have is, please, can we reuse bitmap parse algorithm and syntax?
We have too many different formats and parsers in the kernel and bitmap's one
seems suitable here.

(Despite other small clean ups, like strstrip() use)

> Changes compared to v6[3]:
> - Document non-uniqueness of line names,
> - Rebase on top of commit a0b66a73785ccc8f ("gpio: Rename variable in
> core APIs"),
> - Maintained => Supported,
> - Add Reviewed-by, Acked-by,
> - Drop applied patches:
> - "ARM: integrator: impd1: Use GPIO_LOOKUP() helper macro",
> - "gpiolib: Introduce gpiod_set_config()".
>
> Changes compared to v5[4]:
> - Convert raw gpiod_lookup users to GPIO_LOOKUP*(),
> - Update Documentation/driver-api/gpio/board.rst for gpiod_lookup
> changes,
> - Reword rationale behing looking up GPIOs by line name,
> - Introduce gpiod_set_config(),
> - Use gpiod_to_chip() instead of open-coding,
> - Drop debug print of gpio_desc.label, as it usually points to the
> GPIO Aggregator itself,
> - Drop no longer needed #include "gpiolib.h",
> - Fix missing offset translation in gpio_fwd_set_config(),
> - Fix "allows" without object,
> - Drop "gpiochipN" support,
> - Extend example.
>
> Changes compared to v4[5]:
> - Add Reviewed-by, Tested-by,
> - Fix inconsistent indentation in documentation.
>
> Changes compared to v3[6] (more details in the individual patches):
> - Drop controversial GPIO repeater,
> - Drop support for legacy sysfs interface based name matching,
> - Drop applied "gpiolib: Add GPIOCHIP_NAME definition",
> - Documentation improvements,
> - Lots of small cleanups.
>
> Changes compared to v2[7] (more details in the individual patches):
> - Integrate GPIO Repeater functionality,
> - Absorb GPIO forwarder library, as the Aggregator and Repeater are
> now a single driver,
> - Use the aggregator parameters to create a GPIO lookup table instead
> of an array of GPIO descriptors,
> - Add documentation,
> - New patches:
> - "gpiolib: Add GPIOCHIP_NAME definition",
> - "gpiolib: Add support for gpiochipN-based table lookup",
> - "gpiolib: Add support for GPIO line table lookup",
> - "dt-bindings: gpio: Add gpio-repeater bindings",
> - "docs: gpio: Add GPIO Aggregator/Repeater documentation",
> - "MAINTAINERS: Add GPIO Aggregator/Repeater section".
> - Dropped patches:
> - "gpio: Export gpiod_{request,free}() to modular GPIO code",
> - "gpio: Export gpiochip_get_desc() to modular GPIO code",
> - "gpio: Export gpio_name_to_desc() to modular GPIO code",
> - "gpio: Add GPIO Forwarder Helper".
>
> Changes compared to v1[8]:
> - Drop "virtual", rename to gpio-aggregator,
> - Create and use new GPIO Forwarder Helper, to allow sharing code with
> the GPIO inverter,
> - Lift limit on the maximum number of GPIOs,
> - Improve parsing of GPIO specifiers,
> - Fix modular build.
>
> Aggregating GPIOs and exposing them as a new gpiochip was suggested in
> response to my proof-of-concept for GPIO virtualization with QEMU[9][10].
>
> For the first use case, aggregated GPIO controllers are instantiated and
> destroyed by writing to atribute files in sysfs.
> Sample session on the Renesas Koelsch development board:
>
> - Unbind LEDs from leds-gpio driver:
>
> echo leds > /sys/bus/platform/drivers/leds-gpio/unbind
>
> - Create aggregators:
>
> $ echo e6052000.gpio 19,20 \
> > /sys/bus/platform/drivers/gpio-aggregator/new_device
>
> gpio-aggregator gpio-aggregator.0: gpio 0 => gpio-953
> gpio-aggregator gpio-aggregator.0: gpio 1 => gpio-954
> gpiochip_find_base: found new base at 758
> gpio gpiochip12: (gpio-aggregator.0): added GPIO chardev (254:13)
> gpiochip_setup_dev: registered GPIOs 758 to 759 on device: gpiochip12 (gpio-aggregator.0)
>
> $ echo e6052000.gpio 21 e6050000.gpio 20-22 \
> > /sys/bus/platform/drivers/gpio-aggregator/new_device
>
> gpio-aggregator gpio-aggregator.1: gpio 0 => gpio-955
> gpio-aggregator gpio-aggregator.1: gpio 1 => gpio-1012
> gpio-aggregator gpio-aggregator.1: gpio 2 => gpio-1013
> gpio-aggregator gpio-aggregator.1: gpio 3 => gpio-1014
> gpiochip_find_base: found new base at 754
> gpio gpiochip13: (gpio-aggregator.1): added GPIO chardev (254:13)
> gpiochip_setup_dev: registered GPIOs 754 to 757 on device: gpiochip13 (gpio-aggregator.1)
>
> - Adjust permissions on /dev/gpiochip1[23] (optional)
>
> - Control LEDs:
>
> $ gpioset gpiochip12 0=0 1=1 # LED6 OFF, LED7 ON
> $ gpioset gpiochip12 0=1 1=0 # LED6 ON, LED7 OFF
> $ gpioset gpiochip13 0=1 # LED8 ON
> $ gpioset gpiochip13 0=0 # LED8 OFF
>
> - Destroy aggregators:
>
> $ echo gpio-aggregator.0 \
> > /sys/bus/platform/drivers/gpio-aggregator/delete_device
> $ echo gpio-aggregator.1 \
> > /sys/bus/platform/drivers/gpio-aggregator/delete_device
>
> To ease testing, I have pushed this series to the
> topic/gpio-aggregator-v7 branch of my renesas-drivers repository at
> git://git.kernel.org/pub/scm/linux/kernel/git/geert/renesas-drivers.git.
>
> Thanks!
>
> References:
> [1] "[PATCH QEMU v2 0/5] Add a GPIO backend"
> (https://lore.kernel.org/linux-gpio/[email protected]/)
> [2] "[PATCH V4 2/2] gpio: inverter: document the inverter bindings"
> (https://lore.kernel.org/r/[email protected]/)
> [3] "[PATCH v6 0/8] gpio: Add GPIO Aggregator"
> (https://lore.kernel.org/linux-doc/[email protected]/)
> [4] "[PATCH v5 0/5] gpio: Add GPIO Aggregator"
> (https://lore.kernel.org/r/[email protected]/)
> [5] "[PATCH v4 0/5] gpio: Add GPIO Aggregator"
> (https://lore.kernel.org/r/[email protected])
> [6] "[PATCH v3 0/7] gpio: Add GPIO Aggregator/Repeater"
> (https://lore.kernel.org/r/[email protected]/)
> [7] "[PATCH/RFC v2 0/5] gpio: Add GPIO Aggregator Driver"
> (https://lore.kernel.org/r/[email protected]/)
> [8] "[PATCH RFC] gpio: Add Virtual Aggregator GPIO Driver"
> (https://lore.kernel.org/r/[email protected]/)
> [9] "[PATCH QEMU POC] Add a GPIO backend"
> (https://lore.kernel.org/r/[email protected]/)
> [10] "Getting To Blinky: Virt Edition / Making device pass-through
> work on embedded ARM"
> (https://fosdem.org/2019/schedule/event/vai_getting_to_blinky/)
>
> Geert Uytterhoeven (6):
> i2c: i801: Use GPIO_LOOKUP() helper macro
> mfd: sm501: Use GPIO_LOOKUP_IDX() helper macro
> gpiolib: Add support for GPIO lookup by line name
> gpio: Add GPIO Aggregator
> docs: gpio: Add GPIO Aggregator documentation
> MAINTAINERS: Add GPIO Aggregator section
>
> .../admin-guide/gpio/gpio-aggregator.rst | 111 ++++
> Documentation/admin-guide/gpio/index.rst | 1 +
> Documentation/driver-api/gpio/board.rst | 15 +-
> MAINTAINERS | 7 +
> drivers/gpio/Kconfig | 12 +
> drivers/gpio/Makefile | 1 +
> drivers/gpio/gpio-aggregator.c | 568 ++++++++++++++++++
> drivers/gpio/gpiolib.c | 22 +-
> drivers/i2c/busses/i2c-i801.c | 6 +-
> drivers/mfd/sm501.c | 24 +-
> include/linux/gpio/machine.h | 17 +-
> 11 files changed, 748 insertions(+), 36 deletions(-)
> create mode 100644 Documentation/admin-guide/gpio/gpio-aggregator.rst
> create mode 100644 drivers/gpio/gpio-aggregator.c
>
> --
> 2.17.1
>
> Gr{oetje,eeting}s,
>
> Geert
>
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]
>
> 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

--
With Best Regards,
Andy Shevchenko


2020-05-20 12:38:38

by Geert Uytterhoeven

[permalink] [raw]
Subject: Re: [PATCH v7 0/6] gpio: Add GPIO Aggregator

Hi Andy,

On Wed, May 20, 2020 at 2:14 PM Andy Shevchenko
<[email protected]> wrote:
> On Mon, May 11, 2020 at 04:52:51PM +0200, Geert Uytterhoeven 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 this adds a GPIO driver to aggregate existing GPIOs, and expose
> > them as a new gpiochip. This is useful for implementing access control,
> > and assigning a set of GPIOs to a specific user. Furthermore, this
> > simplifies and hardens exporting GPIOs to a virtual machine, as the VM
> > can just grab the full GPIO controller, and no longer needs to care
> > about which GPIOs to grab and which not, reducing the attack surface.
> > This has been implemented for ARM virt in QEMU[1].
> >
> > Recently, other use cases have been discovered[2], like describing
> > simple GPIO-operated devices in DT, and using the GPIO Aggregator as a
> > generic GPIO driver for userspace, which is useful for industrial
> > control.
> >
> > Note that the first patch of this series ("i2c: i801: Use GPIO_LOOKUP()
> > helper macro") has been applied to i2c/for-next.
>
> Sorry for late reply, recently noticed this nice idea.
> The comment I have is, please, can we reuse bitmap parse algorithm and syntax?
> We have too many different formats and parsers in the kernel and bitmap's one
> seems suitable here.

Thank you, I wasn't aware of that.

Which one do you mean? The documentation seems to be confusing,
and incomplete.
My first guess was bitmap_parse(), but that one assumes hex values?
And given it processes the unsigned long bitmap in u32 chunks, I guess
it doesn't work as expected on big-endian 64-bit?

bitmap_parselist() looks more suitable, and the format seems to be
compatible with what's currently used, so it won't change ABI.
Is that the one you propose?

> (Despite other small clean ups, like strstrip() use)

Aka strim()? There are too many of them, to know all of them by heart ;-)

Gr{oetje,eeting}s,

Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]

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

2020-05-20 12:42:07

by Andy Shevchenko

[permalink] [raw]
Subject: Re: [PATCH v7 0/6] gpio: Add GPIO Aggregator

On Wed, May 20, 2020 at 3:38 PM Geert Uytterhoeven <[email protected]> wrote:
> On Wed, May 20, 2020 at 2:14 PM Andy Shevchenko
> <[email protected]> wrote:
> > On Mon, May 11, 2020 at 04:52:51PM +0200, Geert Uytterhoeven wrote:

...

> > Sorry for late reply, recently noticed this nice idea.
> > The comment I have is, please, can we reuse bitmap parse algorithm and syntax?
> > We have too many different formats and parsers in the kernel and bitmap's one
> > seems suitable here.
>
> Thank you, I wasn't aware of that.
>
> Which one do you mean? The documentation seems to be confusing,
> and incomplete.
> My first guess was bitmap_parse(), but that one assumes hex values?
> And given it processes the unsigned long bitmap in u32 chunks, I guess
> it doesn't work as expected on big-endian 64-bit?
>
> bitmap_parselist() looks more suitable, and the format seems to be
> compatible with what's currently used, so it won't change ABI.
> Is that the one you propose?

Yes, sorry for the confusion.

> > (Despite other small clean ups, like strstrip() use)
>
> Aka strim()? There are too many of them, to know all of them by heart ;-)

The difference between them is __must_check flag. But yes.

--
With Best Regards,
Andy Shevchenko

2020-05-20 12:42:47

by Andy Shevchenko

[permalink] [raw]
Subject: Re: [PATCH v7 0/6] gpio: Add GPIO Aggregator

On Wed, May 20, 2020 at 3:40 PM Andy Shevchenko
<[email protected]> wrote:
> On Wed, May 20, 2020 at 3:38 PM Geert Uytterhoeven <[email protected]> wrote:
> > On Wed, May 20, 2020 at 2:14 PM Andy Shevchenko
> > <[email protected]> wrote:
> > > On Mon, May 11, 2020 at 04:52:51PM +0200, Geert Uytterhoeven wrote:
>
> ...
>
> > > Sorry for late reply, recently noticed this nice idea.
> > > The comment I have is, please, can we reuse bitmap parse algorithm and syntax?
> > > We have too many different formats and parsers in the kernel and bitmap's one
> > > seems suitable here.
> >
> > Thank you, I wasn't aware of that.
> >
> > Which one do you mean? The documentation seems to be confusing,
> > and incomplete.
> > My first guess was bitmap_parse(), but that one assumes hex values?
> > And given it processes the unsigned long bitmap in u32 chunks, I guess
> > it doesn't work as expected on big-endian 64-bit?
> >
> > bitmap_parselist() looks more suitable, and the format seems to be

> > compatible with what's currently used, so it won't change ABI.

What ABI? We didn't have a release with it, right? So, we are quite
flexible for few more weeks to amend it.

> > Is that the one you propose?
>
> Yes, sorry for the confusion.
>
> > > (Despite other small clean ups, like strstrip() use)
> >
> > Aka strim()? There are too many of them, to know all of them by heart ;-)
>
> The difference between them is __must_check flag. But yes.



--
With Best Regards,
Andy Shevchenko

2020-05-20 12:59:28

by Geert Uytterhoeven

[permalink] [raw]
Subject: Re: [PATCH v7 0/6] gpio: Add GPIO Aggregator

Hi Andy,

On Wed, May 20, 2020 at 2:41 PM Andy Shevchenko
<[email protected]> wrote:
> On Wed, May 20, 2020 at 3:40 PM Andy Shevchenko
> <[email protected]> wrote:
> > On Wed, May 20, 2020 at 3:38 PM Geert Uytterhoeven <[email protected]> wrote:
> > > On Wed, May 20, 2020 at 2:14 PM Andy Shevchenko
> > > <[email protected]> wrote:
> > > > On Mon, May 11, 2020 at 04:52:51PM +0200, Geert Uytterhoeven wrote:
> >
> > ...
> >
> > > > Sorry for late reply, recently noticed this nice idea.
> > > > The comment I have is, please, can we reuse bitmap parse algorithm and syntax?
> > > > We have too many different formats and parsers in the kernel and bitmap's one
> > > > seems suitable here.
> > >
> > > Thank you, I wasn't aware of that.
> > >
> > > Which one do you mean? The documentation seems to be confusing,
> > > and incomplete.
> > > My first guess was bitmap_parse(), but that one assumes hex values?
> > > And given it processes the unsigned long bitmap in u32 chunks, I guess
> > > it doesn't work as expected on big-endian 64-bit?
> > >
> > > bitmap_parselist() looks more suitable, and the format seems to be
>
> > > compatible with what's currently used, so it won't change ABI.
>
> What ABI? We didn't have a release with it, right? So, we are quite

ABI = the parameters it accepts currently.

> flexible for few more weeks to amend it.

Indeed, we have time to make changes until the release of v5.8.

Gr{oetje,eeting}s,

Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]

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