Re: [PATCH v6 2/9] misc: minimal mux subsystem and gpio-based mux controller

On 2016-12-31 17:19, Jonathan Cameron wrote:

On 30/11/16 08:16, Peter Rosin wrote:

quoted

Add a new minimalistic subsystem that handles multiplexer controllers.
When multiplexers are used in various places in the kernel, and the
same multiplexer controller can be used for several independent things,
there should be one place to implement support for said multiplexer
controller.

A single multiplexer controller can also be used to control several
parallel multiplexers, that are in turn used by different subsystems
in the kernel, leading to a need to coordinate multiplexer accesses.
The multiplexer subsystem handles this coordination.

This new mux controller subsystem initially comes with a single backend
driver that controls gpio based multiplexers. Even though not needed by
this initial driver, the mux controller subsystem is prepared to handle
chips with multiple (independent) mux controllers.

Signed-off-by: Peter Rosin <redacted>

Few trivial bits inline + question of whether misc is the right location..
It's small, but not totally trivial as subsystems go, so perhaps it needs it's
own directory.

In [9/9] you come to the conclusion that muxes should have their own
directory, but do you think it should be
	drivers/mux/*
or
	drivers/misc/mux/*
?

quoted

---
 Documentation/driver-model/devres.txt |   6 +-
 MAINTAINERS                           |   2 +
 drivers/misc/Kconfig                  |  30 ++++
 drivers/misc/Makefile                 |   2 +
 drivers/misc/mux-core.c               | 311 ++++++++++++++++++++++++++++++++++
 drivers/misc/mux-gpio.c               | 138 +++++++++++++++
 include/linux/mux.h                   | 197 +++++++++++++++++++++
 7 files changed, 685 insertions(+), 1 deletion(-)
 create mode 100644 drivers/misc/mux-core.c
 create mode 100644 drivers/misc/mux-gpio.c
 create mode 100644 include/linux/mux.h

diff --git a/Documentation/driver-model/devres.txt b/Documentation/driver-model/devres.txt
index ca9d1eb46bc0..d64ede85b61b 100644
--- a/Documentation/driver-model/devres.txt
+++ b/Documentation/driver-model/devres.txt

@@ -330,7 +330,11 @@ MEM
   devm_kzalloc()
 
 MFD
- devm_mfd_add_devices()

Technically should be in a separate cleanup patch...

quoted

+  devm_mfd_add_devices()
+
+MUX
+  devm_mux_control_get()
+  devm_mux_control_put()
 
 PER-CPU MEM
   devm_alloc_percpu()

diff --git a/MAINTAINERS b/MAINTAINERS
index 3d4d0efc2b64..dc7498682752 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS

@@ -8407,6 +8407,8 @@ MULTIPLEXER SUBSYSTEM
 M:	Peter Rosin <peda@axentia.se>
 S:	Maintained
 F:	Documentation/devicetree/bindings/misc/mux-*
+F:	include/linux/mux.h
+F:	drivers/misc/mux-*
 
 MULTISOUND SOUND DRIVER
 M:	Andrew Veliath <andrewtv@usa.net>

diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig
index 64971baf11fa..2ce675e410c5 100644
--- a/drivers/misc/Kconfig
+++ b/drivers/misc/Kconfig

@@ -766,6 +766,36 @@ config PANEL_BOOT_MESSAGE
 	  An empty message will only clear the display at driver init time. Any other
 	  printf()-formatted message is valid with newline and escape codes.
 
+menuconfig MULTIPLEXER
+	bool "Multiplexer subsystem"
+	help
+	  Multiplexer controller subsystem. Multiplexers are used in a
+	  variety of settings, and this subsystem abstracts their use
+	  so that the rest of the kernel sees a common interface. When
+	  multiple parallel multiplexers are controlled by one single
+	  multiplexer controller, this subsystem also coordinates the
+	  multiplexer accesses.
+
+	  If unsure, say no.
+

Fun question of the day. Is misc the place to put this or should it
have it's own directory. I'd go for own directory...

I thought it too small for its own dir and that it could always be
moved later. But I don't really care...

On the plus side, whilst it's in misc you get to pester Greg and Arnd
for review.

I know :-]

quoted

+if MULTIPLEXER
+
+config MUX_GPIO
+	tristate "GPIO-controlled Multiplexer"
+	depends on OF && GPIOLIB
+	help
+	  GPIO-controlled Multiplexer controller.
+
+	  The driver builds a single multiplexer controller using a number
+	  of gpio pins. For N pins, there will be 2^N possible multiplexer
+	  states. The GPIO pins can be connected (by the hardware) to several

*snip*

quoted

+
+void mux_chip_free(struct mux_chip *mux_chip)
+{
+	if (!mux_chip)
+		return;

I'd drop a blank line in here. Slightly nicer to read.

Right.

quoted

+	put_device(&mux_chip->dev);
+}
+EXPORT_SYMBOL_GPL(mux_chip_free);

*snip*

quoted

+
+static int mux_gpio_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct device_node *np = pdev->dev.of_node;
+	struct mux_chip *mux_chip;
+	struct mux_gpio *mux_gpio;
+	int pins;
+	u32 idle_state;
+	int ret;
+
+	if (!np)
+		return -ENODEV;
+
+	pins = gpiod_count(dev, "mux");
+	if (pins < 0)
+		return pins;
+
+	mux_chip = mux_chip_alloc(dev, 1, sizeof(*mux_gpio) +
+				  pins * sizeof(*mux_gpio->val));
+	if (!mux_chip)
+		return -ENOMEM;

Rather feels like mux_chip_alloc is a good candidate for a managed
allocator. Might be worth doing the register as well then the remove
below would go away.  I guess perhaps not that worthwhile as not many
mux types are likely to ever exist...

To me it seemed like too much extra support code for just two drivers.
And it can always be added later if/when more mux drivers show up...

quoted

+
+	mux_gpio = mux_chip_priv(mux_chip);
+	mux_gpio->val = (int *)(mux_gpio + 1);

*snip*

Cheers,
peda