* [PATCH v13 01/10] devres: trivial whitespace fix
2017-04-13 16:43 [PATCH v13 00/10] mux controller abstraction and iio/i2c muxes Peter Rosin
@ 2017-04-13 16:43 ` Peter Rosin
2017-04-13 16:43 ` [PATCH v13 02/10] dt-bindings: document devicetree bindings for mux-controllers and gpio-mux Peter Rosin
` (8 subsequent siblings)
9 siblings, 0 replies; 38+ messages in thread
From: Peter Rosin @ 2017-04-13 16:43 UTC (permalink / raw)
To: linux-kernel, Greg Kroah-Hartman
Cc: Peter Rosin, Wolfram Sang, Rob Herring, Mark Rutland,
Jonathan Cameron, Hartmut Knaack, Lars-Peter Clausen,
Peter Meerwald-Stadler, Jonathan Corbet, linux-i2c, devicetree,
linux-iio, linux-doc, Andrew Morton, Colin Ian King,
Paul Gortmaker, Philipp Zabel, kernel
Everything else is indented with two spaces, so fix the odd one out.
Acked-by: Jonathan Cameron <jic23@kernel.org>
Signed-off-by: Peter Rosin <peda@axentia.se>
---
Documentation/driver-model/devres.txt | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/Documentation/driver-model/devres.txt b/Documentation/driver-model/devres.txt
index bf34d5b3a733..efb8200819d6 100644
--- a/Documentation/driver-model/devres.txt
+++ b/Documentation/driver-model/devres.txt
@@ -335,7 +335,7 @@ MEM
devm_kzalloc()
MFD
- devm_mfd_add_devices()
+ devm_mfd_add_devices()
PER-CPU MEM
devm_alloc_percpu()
--
2.1.4
^ permalink raw reply related [flat|nested] 38+ messages in thread* [PATCH v13 02/10] dt-bindings: document devicetree bindings for mux-controllers and gpio-mux
2017-04-13 16:43 [PATCH v13 00/10] mux controller abstraction and iio/i2c muxes Peter Rosin
2017-04-13 16:43 ` [PATCH v13 01/10] devres: trivial whitespace fix Peter Rosin
@ 2017-04-13 16:43 ` Peter Rosin
2017-04-18 10:06 ` Philipp Zabel
2017-04-13 16:43 ` [PATCH v13 03/10] mux: minimal mux subsystem and gpio-based mux controller Peter Rosin
` (7 subsequent siblings)
9 siblings, 1 reply; 38+ messages in thread
From: Peter Rosin @ 2017-04-13 16:43 UTC (permalink / raw)
To: linux-kernel, Greg Kroah-Hartman
Cc: Peter Rosin, Wolfram Sang, Rob Herring, Mark Rutland,
Jonathan Cameron, Hartmut Knaack, Lars-Peter Clausen,
Peter Meerwald-Stadler, Jonathan Corbet, linux-i2c, devicetree,
linux-iio, linux-doc, Andrew Morton, Colin Ian King,
Paul Gortmaker, Philipp Zabel, kernel
Allow specifying that a single multiplexer controller can be used to
control several parallel multiplexers, thus enabling sharing of the
multiplexer controller by different consumers.
Add a binding for a first mux controller in the form of a GPIO based mux
controller.
Acked-by: Jonathan Cameron <jic23@kernel.org>
Acked-by: Rob Herring <robh@kernel.org>
Signed-off-by: Peter Rosin <peda@axentia.se>
---
Documentation/devicetree/bindings/mux/gpio-mux.txt | 69 +++++++++
.../devicetree/bindings/mux/mux-controller.txt | 157 +++++++++++++++++++++
MAINTAINERS | 6 +
include/dt-bindings/mux/mux.h | 16 +++
4 files changed, 248 insertions(+)
create mode 100644 Documentation/devicetree/bindings/mux/gpio-mux.txt
create mode 100644 Documentation/devicetree/bindings/mux/mux-controller.txt
create mode 100644 include/dt-bindings/mux/mux.h
diff --git a/Documentation/devicetree/bindings/mux/gpio-mux.txt b/Documentation/devicetree/bindings/mux/gpio-mux.txt
new file mode 100644
index 000000000000..b8f746344d80
--- /dev/null
+++ b/Documentation/devicetree/bindings/mux/gpio-mux.txt
@@ -0,0 +1,69 @@
+GPIO-based multiplexer controller bindings
+
+Define what GPIO pins are used to control a multiplexer. Or several
+multiplexers, if the same pins control more than one multiplexer.
+
+Required properties:
+- compatible : "gpio-mux"
+- mux-gpios : list of gpios used to control the multiplexer, least
+ significant bit first.
+- #mux-control-cells : <0>
+* Standard mux-controller bindings as decribed in mux-controller.txt
+
+Optional properties:
+- idle-state : if present, the state the mux will have when idle. The
+ special state MUX_IDLE_AS_IS is the default.
+
+The multiplexer state is defined as the number represented by the
+multiplexer GPIO pins, where the first pin is the least significant
+bit. An active pin is a binary 1, an inactive pin is a binary 0.
+
+Example:
+
+ mux: mux-controller {
+ compatible = "gpio-mux";
+ #mux-control-cells = <0>;
+
+ mux-gpios = <&pioA 0 GPIO_ACTIVE_HIGH>,
+ <&pioA 1 GPIO_ACTIVE_HIGH>;
+ };
+
+ adc-mux {
+ compatible = "io-channel-mux";
+ io-channels = <&adc 0>;
+ io-channel-names = "parent";
+
+ mux-controls = <&mux>;
+
+ channels = "sync-1", "in", "out", "sync-2";
+ };
+
+ i2c-mux {
+ compatible = "i2c-mux";
+ i2c-parent = <&i2c1>;
+
+ mux-controls = <&mux>;
+
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ i2c@0 {
+ reg = <0>;
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ ssd1307: oled@3c {
+ /* ... */
+ };
+ };
+
+ i2c@3 {
+ reg = <3>;
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ pca9555: pca9555@20 {
+ /* ... */
+ };
+ };
+ };
diff --git a/Documentation/devicetree/bindings/mux/mux-controller.txt b/Documentation/devicetree/bindings/mux/mux-controller.txt
new file mode 100644
index 000000000000..4f47e4bd2fa0
--- /dev/null
+++ b/Documentation/devicetree/bindings/mux/mux-controller.txt
@@ -0,0 +1,157 @@
+Common multiplexer controller bindings
+======================================
+
+A multiplexer (or mux) controller will have one, or several, consumer devices
+that uses the mux controller. Thus, a mux controller can possibly control
+several parallel multiplexers. Presumably there will be at least one
+multiplexer needed by each consumer, but a single mux controller can of course
+control several multiplexers for a single consumer.
+
+A mux controller provides a number of states to its consumers, and the state
+space is a simple zero-based enumeration. I.e. 0-1 for a 2-way multiplexer,
+0-7 for an 8-way multiplexer, etc.
+
+
+Consumers
+---------
+
+Mux controller consumers should specify a list of mux controllers that they
+want to use with a property containing a 'mux-ctrl-list':
+
+ mux-ctrl-list ::= <single-mux-ctrl> [mux-ctrl-list]
+ single-mux-ctrl ::= <mux-ctrl-phandle> [mux-ctrl-specifier]
+ mux-ctrl-phandle : phandle to mux controller node
+ mux-ctrl-specifier : array of #mux-control-cells specifying the
+ given mux controller (controller specific)
+
+Mux controller properties should be named "mux-controls". The exact meaning of
+each mux controller property must be documented in the device tree binding for
+each consumer. An optional property "mux-control-names" may contain a list of
+strings to label each of the mux controllers listed in the "mux-controls"
+property.
+
+Drivers for devices that use more than a single mux controller can use the
+"mux-control-names" property to map the name of the requested mux controller
+to an index into the list given by the "mux-controls" property.
+
+mux-ctrl-specifier typically encodes the chip-relative mux controller number.
+If the mux controller chip only provides a single mux controller, the
+mux-ctrl-specifier can typically be left out.
+
+Example:
+
+ /* One consumer of a 2-way mux controller (one GPIO-line) */
+ mux: mux-controller {
+ compatible = "gpio-mux";
+ #mux-control-cells = <0>;
+
+ mux-gpios = <&pioA 0 GPIO_ACTIVE_HIGH>;
+ };
+
+ adc-mux {
+ compatible = "io-channel-mux";
+ io-channels = <&adc 0>;
+ io-channel-names = "parent";
+
+ mux-controls = <&mux>;
+ mux-control-names = "adc";
+
+ channels = "sync", "in";
+ };
+
+Note that in the example above, specifying the "mux-control-names" is redundant
+because there is only one mux controller in the list. However, if the driver
+for the consumer node in fact asks for a named mux controller, that name is of
+course still required.
+
+ /*
+ * Two consumers (one for an ADC line and one for an i2c bus) of
+ * parallel 4-way multiplexers controlled by the same two GPIO-lines.
+ */
+ mux: mux-controller {
+ compatible = "gpio-mux";
+ #mux-control-cells = <0>;
+
+ mux-gpios = <&pioA 0 GPIO_ACTIVE_HIGH>,
+ <&pioA 1 GPIO_ACTIVE_HIGH>;
+ };
+
+ adc-mux {
+ compatible = "io-channel-mux";
+ io-channels = <&adc 0>;
+ io-channel-names = "parent";
+
+ mux-controls = <&mux>;
+
+ channels = "sync-1", "in", "out", "sync-2";
+ };
+
+ i2c-mux {
+ compatible = "i2c-mux";
+ i2c-parent = <&i2c1>;
+
+ mux-controls = <&mux>;
+
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ i2c@0 {
+ reg = <0>;
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ ssd1307: oled@3c {
+ /* ... */
+ };
+ };
+
+ i2c@3 {
+ reg = <3>;
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ pca9555: pca9555@20 {
+ /* ... */
+ };
+ };
+ };
+
+
+Mux controller nodes
+--------------------
+
+Mux controller nodes must specify the number of cells used for the
+specifier using the '#mux-control-cells' property.
+
+Optionally, mux controller nodes can also specify the state the mux should
+have when it is idle. The idle-state property is used for this. If the
+idle-state is not present, the mux controller is typically left as is when
+it is idle. For multiplexer chips that expose several mux controllers, the
+idle-state property is an array with one idle state for each mux controller.
+
+The special value (-1) may be used to indicate that the mux should be left
+as is when it is idle. This is the default, but can still be useful for
+mux controller chips with more than one mux controller, particularly when
+there is a need to "step past" a mux controller and set some other idle
+state for a mux controller with a higher index.
+
+Some mux controllers have the ability to disconnect the input/output of the
+multiplexer. Using this disconnected high-impedance state as the idle state
+is indicated with idle state (-2).
+
+These constants are available in
+
+ #include <dt-bindings/mux/mux.h>
+
+as MUX_IDLE_AS_IS (-1) and MUX_IDLE_DISCONNECT (-2).
+
+An example mux controller node look like this (the adg972a chip is a triple
+4-way multiplexer):
+
+ mux: mux-controller@50 {
+ compatible = "adi,adg792a";
+ reg = <0x50>;
+ #mux-control-cells = <1>;
+
+ idle-state = <MUX_IDLE_DISCONNECT MUX_IDLE_AS_IS 2>;
+ };
diff --git a/MAINTAINERS b/MAINTAINERS
index c265a5fe4848..7fc06739c8ad 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -8558,6 +8558,12 @@ S: Orphan
F: drivers/mmc/host/mmc_spi.c
F: include/linux/spi/mmc_spi.h
+MULTIPLEXER SUBSYSTEM
+M: Peter Rosin <peda@axentia.se>
+S: Maintained
+F: Documentation/devicetree/bindings/mux/
+F: include/linux/dt-bindings/mux/
+
MULTISOUND SOUND DRIVER
M: Andrew Veliath <andrewtv@usa.net>
S: Maintained
diff --git a/include/dt-bindings/mux/mux.h b/include/dt-bindings/mux/mux.h
new file mode 100644
index 000000000000..c8e855c4a609
--- /dev/null
+++ b/include/dt-bindings/mux/mux.h
@@ -0,0 +1,16 @@
+/*
+ * This header provides constants for most Multiplexer bindings.
+ *
+ * Most Multiplexer bindings specify an idle state. In most cases, the
+ * the multiplexer can be left as is when idle, and in some cases it can
+ * disconnect the input/output and leave the multiplexer in a high
+ * impedance state.
+ */
+
+#ifndef _DT_BINDINGS_MUX_MUX_H
+#define _DT_BINDINGS_MUX_MUX_H
+
+#define MUX_IDLE_AS_IS (-1)
+#define MUX_IDLE_DISCONNECT (-2)
+
+#endif
--
2.1.4
^ permalink raw reply related [flat|nested] 38+ messages in thread* Re: [PATCH v13 02/10] dt-bindings: document devicetree bindings for mux-controllers and gpio-mux
2017-04-13 16:43 ` [PATCH v13 02/10] dt-bindings: document devicetree bindings for mux-controllers and gpio-mux Peter Rosin
@ 2017-04-18 10:06 ` Philipp Zabel
2017-04-18 13:36 ` Peter Rosin
0 siblings, 1 reply; 38+ messages in thread
From: Philipp Zabel @ 2017-04-18 10:06 UTC (permalink / raw)
To: Peter Rosin
Cc: linux-kernel, Greg Kroah-Hartman, Mark Rutland, devicetree,
Lars-Peter Clausen, Wolfram Sang, linux-iio, Jonathan Corbet,
linux-doc, kernel, Paul Gortmaker, Rob Herring, linux-i2c,
Peter Meerwald-Stadler, Hartmut Knaack, Colin Ian King,
Andrew Morton, Jonathan Cameron
On Thu, 2017-04-13 at 18:43 +0200, Peter Rosin wrote:
> Allow specifying that a single multiplexer controller can be used to
> control several parallel multiplexers, thus enabling sharing of the
> multiplexer controller by different consumers.
>
> Add a binding for a first mux controller in the form of a GPIO based mux
> controller.
>
> Acked-by: Jonathan Cameron <jic23@kernel.org>
> Acked-by: Rob Herring <robh@kernel.org>
> Signed-off-by: Peter Rosin <peda@axentia.se>
> ---
> Documentation/devicetree/bindings/mux/gpio-mux.txt | 69 +++++++++
> .../devicetree/bindings/mux/mux-controller.txt | 157 +++++++++++++++++++++
> MAINTAINERS | 6 +
> include/dt-bindings/mux/mux.h | 16 +++
> 4 files changed, 248 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/mux/gpio-mux.txt
> create mode 100644 Documentation/devicetree/bindings/mux/mux-controller.txt
> create mode 100644 include/dt-bindings/mux/mux.h
>
> diff --git a/Documentation/devicetree/bindings/mux/gpio-mux.txt b/Documentation/devicetree/bindings/mux/gpio-mux.txt
> new file mode 100644
> index 000000000000..b8f746344d80
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/mux/gpio-mux.txt
> @@ -0,0 +1,69 @@
> +GPIO-based multiplexer controller bindings
> +
> +Define what GPIO pins are used to control a multiplexer. Or several
> +multiplexers, if the same pins control more than one multiplexer.
> +
> +Required properties:
> +- compatible : "gpio-mux"
> +- mux-gpios : list of gpios used to control the multiplexer, least
> + significant bit first.
> +- #mux-control-cells : <0>
> +* Standard mux-controller bindings as decribed in mux-controller.txt
> +
> +Optional properties:
> +- idle-state : if present, the state the mux will have when idle. The
> + special state MUX_IDLE_AS_IS is the default.
> +
> +The multiplexer state is defined as the number represented by the
> +multiplexer GPIO pins, where the first pin is the least significant
> +bit. An active pin is a binary 1, an inactive pin is a binary 0.
> +
> +Example:
> +
> + mux: mux-controller {
> + compatible = "gpio-mux";
> + #mux-control-cells = <0>;
> +
> + mux-gpios = <&pioA 0 GPIO_ACTIVE_HIGH>,
> + <&pioA 1 GPIO_ACTIVE_HIGH>;
> + };
> +
> + adc-mux {
> + compatible = "io-channel-mux";
> + io-channels = <&adc 0>;
> + io-channel-names = "parent";
> +
> + mux-controls = <&mux>;
> +
> + channels = "sync-1", "in", "out", "sync-2";
> + };
Could you explain in more detail the reasoning behind this split between
the mux controller and the actual mux?
For SoC internal video bus muxes that are controlled by a register
bitfield, it seems a bit strange to have to split them into two device
tree nodes.
Basically I'm trying to figure out whether a video mux (which has a mux
control plus OF-graph bindings to describe its ports and connections)
would fit into the same category as an adc-mux or i2c-mux, or whether it
would be better to handle them as a specialized form of mux-controller.
regards
Philipp
^ permalink raw reply [flat|nested] 38+ messages in thread* Re: [PATCH v13 02/10] dt-bindings: document devicetree bindings for mux-controllers and gpio-mux
2017-04-18 10:06 ` Philipp Zabel
@ 2017-04-18 13:36 ` Peter Rosin
2017-04-19 9:17 ` Philipp Zabel
0 siblings, 1 reply; 38+ messages in thread
From: Peter Rosin @ 2017-04-18 13:36 UTC (permalink / raw)
To: Philipp Zabel
Cc: linux-kernel, Greg Kroah-Hartman, Mark Rutland, devicetree,
Lars-Peter Clausen, Wolfram Sang, linux-iio, Jonathan Corbet,
linux-doc, kernel, Paul Gortmaker, Rob Herring, linux-i2c,
Peter Meerwald-Stadler, Hartmut Knaack, Colin Ian King,
Andrew Morton, Jonathan Cameron
On 2017-04-18 12:06, Philipp Zabel wrote:
> On Thu, 2017-04-13 at 18:43 +0200, Peter Rosin wrote:
>> Allow specifying that a single multiplexer controller can be used to
>> control several parallel multiplexers, thus enabling sharing of the
>> multiplexer controller by different consumers.
>>
>> Add a binding for a first mux controller in the form of a GPIO based mux
>> controller.
>>
>> Acked-by: Jonathan Cameron <jic23@kernel.org>
>> Acked-by: Rob Herring <robh@kernel.org>
>> Signed-off-by: Peter Rosin <peda@axentia.se>
>> ---
>> Documentation/devicetree/bindings/mux/gpio-mux.txt | 69 +++++++++
>> .../devicetree/bindings/mux/mux-controller.txt | 157 +++++++++++++++++++++
>> MAINTAINERS | 6 +
>> include/dt-bindings/mux/mux.h | 16 +++
>> 4 files changed, 248 insertions(+)
>> create mode 100644 Documentation/devicetree/bindings/mux/gpio-mux.txt
>> create mode 100644 Documentation/devicetree/bindings/mux/mux-controller.txt
>> create mode 100644 include/dt-bindings/mux/mux.h
>>
>> diff --git a/Documentation/devicetree/bindings/mux/gpio-mux.txt b/Documentation/devicetree/bindings/mux/gpio-mux.txt
>> new file mode 100644
>> index 000000000000..b8f746344d80
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/mux/gpio-mux.txt
>> @@ -0,0 +1,69 @@
>> +GPIO-based multiplexer controller bindings
>> +
>> +Define what GPIO pins are used to control a multiplexer. Or several
>> +multiplexers, if the same pins control more than one multiplexer.
>> +
>> +Required properties:
>> +- compatible : "gpio-mux"
>> +- mux-gpios : list of gpios used to control the multiplexer, least
>> + significant bit first.
>> +- #mux-control-cells : <0>
>> +* Standard mux-controller bindings as decribed in mux-controller.txt
>> +
>> +Optional properties:
>> +- idle-state : if present, the state the mux will have when idle. The
>> + special state MUX_IDLE_AS_IS is the default.
>> +
>> +The multiplexer state is defined as the number represented by the
>> +multiplexer GPIO pins, where the first pin is the least significant
>> +bit. An active pin is a binary 1, an inactive pin is a binary 0.
>> +
>> +Example:
>> +
>> + mux: mux-controller {
>> + compatible = "gpio-mux";
>> + #mux-control-cells = <0>;
>> +
>> + mux-gpios = <&pioA 0 GPIO_ACTIVE_HIGH>,
>> + <&pioA 1 GPIO_ACTIVE_HIGH>;
>> + };
>> +
>> + adc-mux {
>> + compatible = "io-channel-mux";
>> + io-channels = <&adc 0>;
>> + io-channel-names = "parent";
>> +
>> + mux-controls = <&mux>;
>> +
>> + channels = "sync-1", "in", "out", "sync-2";
>> + };
>
> Could you explain in more detail the reasoning behind this split between
> the mux controller and the actual mux?
> For SoC internal video bus muxes that are controlled by a register
> bitfield, it seems a bit strange to have to split them into two device
> tree nodes.
The background for the split is in the cover letter.
Basically, when the same set of gpio lines control several muxes, and
when these muxes are used for unrelated things, you needs some extra
complexity.
The mux controller is what controls those gpio lines, and thus the mux
state for all muxes they are connected to. The consumers refer to the
mux controller and request a certain state. So, the consumers naturally
need to interact or else they would destroy the mux state for each other.
Regarding the device tree layout, perhaps the mux consumers could be
children of the mux controller, but I think that would make the mux
controller more like a bus instead of a class. Anyway, I don't want to
go there again, because I remember it as a messy place. Maybe I could
do better now than I did way back when, but I'm not going willingly
and someone would have to force me.
The benefit of the split is that the mux consumer need no longer concern
itself with if the mux is controlled by gpio lines, an I2C based chip
like the ADG792 or if it is controlled by some mmio register. You can
thus avoid building muxing sub-sub-systems like drivers/i2c/muxes for
every subsystem needing muxing.
The drawback is that you get an extra device tree node for the mux
controller that may not make sense if it is in no way possible to
reuse your driver for HW with a different mux. Which may be the case
for your video case? But for generic stuff like ADC lines and I2C
buses, muxing options are diverse...
> Basically I'm trying to figure out whether a video mux (which has a mux
> control plus OF-graph bindings to describe its ports and connections)
> would fit into the same category as an adc-mux or i2c-mux, or whether it
> would be better to handle them as a specialized form of mux-controller.
I did read some earlier thread about your muxing requirements and I got
the impression that you also had HW which controlled the mux with
gpio lines? In that case, the mux subsystem seems like a perfect fit
with a new syscon/mmio/reg based mux driver (or whatever the name should
be, I think I'd go with syscon) pretty much as suggested in your RFC
patches. And then of course reuse the existing gpio-mux driver for the
other case.
The video-mux would fit as a mux consumer just like the iio-mux and the
i2c-mux are mux consumers, with input 0/input 1 being the port that
would be selected with the mux I guess. I don't think there should be a
bunch of video code inside the drivers/mux subdir, for the same reason
there's no iio/i2c code in there.
If I got things wrong when I skimmed whatever I came across, and if the
mmio register is the only mux control option in the stars, it becomes
less obvious... It's of course still possible to hook into the mux
subsystem, but the benefit is questionable. And you do get the extra
device tree node. You could of course also implement a mux driver
outside of drivers/mux and thus make use of the mux api, but it's tiny
and any benefit is truly small.
Cheers,
peda
^ permalink raw reply [flat|nested] 38+ messages in thread* Re: [PATCH v13 02/10] dt-bindings: document devicetree bindings for mux-controllers and gpio-mux
2017-04-18 13:36 ` Peter Rosin
@ 2017-04-19 9:17 ` Philipp Zabel
2017-04-19 10:41 ` Peter Rosin
0 siblings, 1 reply; 38+ messages in thread
From: Philipp Zabel @ 2017-04-19 9:17 UTC (permalink / raw)
To: Peter Rosin
Cc: linux-kernel, Greg Kroah-Hartman, Mark Rutland, devicetree,
Lars-Peter Clausen, Wolfram Sang, linux-iio, Jonathan Corbet,
linux-doc, kernel, Paul Gortmaker, Rob Herring, linux-i2c,
Peter Meerwald-Stadler, Hartmut Knaack, Colin Ian King,
Andrew Morton, Jonathan Cameron
On Tue, 2017-04-18 at 15:36 +0200, Peter Rosin wrote:
> On 2017-04-18 12:06, Philipp Zabel wrote:
> > On Thu, 2017-04-13 at 18:43 +0200, Peter Rosin wrote:
> >> Allow specifying that a single multiplexer controller can be used to
> >> control several parallel multiplexers, thus enabling sharing of the
> >> multiplexer controller by different consumers.
> >>
> >> Add a binding for a first mux controller in the form of a GPIO based mux
> >> controller.
> >>
> >> Acked-by: Jonathan Cameron <jic23@kernel.org>
> >> Acked-by: Rob Herring <robh@kernel.org>
> >> Signed-off-by: Peter Rosin <peda@axentia.se>
> >> ---
> >> Documentation/devicetree/bindings/mux/gpio-mux.txt | 69 +++++++++
> >> .../devicetree/bindings/mux/mux-controller.txt | 157 +++++++++++++++++++++
> >> MAINTAINERS | 6 +
> >> include/dt-bindings/mux/mux.h | 16 +++
> >> 4 files changed, 248 insertions(+)
> >> create mode 100644 Documentation/devicetree/bindings/mux/gpio-mux.txt
> >> create mode 100644 Documentation/devicetree/bindings/mux/mux-controller.txt
> >> create mode 100644 include/dt-bindings/mux/mux.h
> >>
> >> diff --git a/Documentation/devicetree/bindings/mux/gpio-mux.txt b/Documentation/devicetree/bindings/mux/gpio-mux.txt
> >> new file mode 100644
> >> index 000000000000..b8f746344d80
> >> --- /dev/null
> >> +++ b/Documentation/devicetree/bindings/mux/gpio-mux.txt
> >> @@ -0,0 +1,69 @@
> >> +GPIO-based multiplexer controller bindings
> >> +
> >> +Define what GPIO pins are used to control a multiplexer. Or several
> >> +multiplexers, if the same pins control more than one multiplexer.
> >> +
> >> +Required properties:
> >> +- compatible : "gpio-mux"
> >> +- mux-gpios : list of gpios used to control the multiplexer, least
> >> + significant bit first.
> >> +- #mux-control-cells : <0>
> >> +* Standard mux-controller bindings as decribed in mux-controller.txt
> >> +
> >> +Optional properties:
> >> +- idle-state : if present, the state the mux will have when idle. The
> >> + special state MUX_IDLE_AS_IS is the default.
> >> +
> >> +The multiplexer state is defined as the number represented by the
> >> +multiplexer GPIO pins, where the first pin is the least significant
> >> +bit. An active pin is a binary 1, an inactive pin is a binary 0.
> >> +
> >> +Example:
> >> +
> >> + mux: mux-controller {
> >> + compatible = "gpio-mux";
> >> + #mux-control-cells = <0>;
> >> +
> >> + mux-gpios = <&pioA 0 GPIO_ACTIVE_HIGH>,
> >> + <&pioA 1 GPIO_ACTIVE_HIGH>;
> >> + };
> >> +
> >> + adc-mux {
> >> + compatible = "io-channel-mux";
> >> + io-channels = <&adc 0>;
> >> + io-channel-names = "parent";
> >> +
> >> + mux-controls = <&mux>;
> >> +
> >> + channels = "sync-1", "in", "out", "sync-2";
> >> + };
> >
> > Could you explain in more detail the reasoning behind this split between
> > the mux controller and the actual mux?
> > For SoC internal video bus muxes that are controlled by a register
> > bitfield, it seems a bit strange to have to split them into two device
> > tree nodes.
>
> The background for the split is in the cover letter.
Thanks for explaining anyway, I didn't read past the changelog earlier.
[...]
> > Basically I'm trying to figure out whether a video mux (which has a mux
> > control plus OF-graph bindings to describe its ports and connections)
> > would fit into the same category as an adc-mux or i2c-mux, or whether it
> > would be better to handle them as a specialized form of mux-controller.
>
> I did read some earlier thread about your muxing requirements and I got
> the impression that you also had HW which controlled the mux with
> gpio lines? In that case, the mux subsystem seems like a perfect fit
> with a new syscon/mmio/reg based mux driver (or whatever the name should
> be, I think I'd go with syscon) pretty much as suggested in your RFC
> patches. And then of course reuse the existing gpio-mux driver for the
> other case.
Yes, the requirement on hand is for MMIO controlled SoC internal muxes
for the i.MX6 video capture subsystem, but I'd like to also support GPIO
controlled external muxes to switch between two camera sources on those
boards that have them.
> The video-mux would fit as a mux consumer just like the iio-mux and the
> i2c-mux are mux consumers, with input 0/input 1 being the port that
> would be selected with the mux I guess.
Exactly. An N-input mux would have N+1 ports with port N being the
output.
[...]
> If I got things wrong when I skimmed whatever I came across, and if the
> mmio register is the only mux control option in the stars, it becomes
> less obvious... It's of course still possible to hook into the mux
> subsystem, but the benefit is questionable. And you do get the extra
> device tree node. You could of course also implement a mux driver
> outside of drivers/mux and thus make use of the mux api, but it's tiny
> and any benefit is truly small.
What I wondered mostly is whether it would be a good idea to move the
OF-graph ports into the mux controller node, and let the video capture
device be the consumer of the mux.
But this wouldn't fit well with the clear split between the mux
controller and the actual mux hardware in the mux DT bindings.
regards
Philipp
^ permalink raw reply [flat|nested] 38+ messages in thread* Re: [PATCH v13 02/10] dt-bindings: document devicetree bindings for mux-controllers and gpio-mux
2017-04-19 9:17 ` Philipp Zabel
@ 2017-04-19 10:41 ` Peter Rosin
2017-04-19 11:05 ` Philipp Zabel
0 siblings, 1 reply; 38+ messages in thread
From: Peter Rosin @ 2017-04-19 10:41 UTC (permalink / raw)
To: Philipp Zabel
Cc: linux-kernel, Greg Kroah-Hartman, Mark Rutland, devicetree,
Lars-Peter Clausen, Wolfram Sang, linux-iio, Jonathan Corbet,
linux-doc, kernel, Paul Gortmaker, Rob Herring, linux-i2c,
Peter Meerwald-Stadler, Hartmut Knaack, Colin Ian King,
Andrew Morton, Jonathan Cameron
On 2017-04-19 11:17, Philipp Zabel wrote:
> On Tue, 2017-04-18 at 15:36 +0200, Peter Rosin wrote:
>> If I got things wrong when I skimmed whatever I came across, and if the
>> mmio register is the only mux control option in the stars, it becomes
>> less obvious... It's of course still possible to hook into the mux
>> subsystem, but the benefit is questionable. And you do get the extra
>> device tree node. You could of course also implement a mux driver
>> outside of drivers/mux and thus make use of the mux api, but it's tiny
>> and any benefit is truly small.
>
> What I wondered mostly is whether it would be a good idea to move the
> OF-graph ports into the mux controller node, and let the video capture
> device be the consumer of the mux.
> But this wouldn't fit well with the clear split between the mux
> controller and the actual mux hardware in the mux DT bindings.
I have tried to do something similar. I think. The current
drivers/i2c/muxes/i2c-mux-gpio.c is a good candidate for the same thing
IIUC.
That dedicated driver and the general purpose i2c mux driver does pretty
much the same thing with these two DT snippets:
Dedicated i2c-mux-gpio DT snippet:
i2c-mux {
compatible = "i2c-mux-gpio";
i2c-parent = <&i2c1>;
mux-gpios = <&gpio1 22 0 &gpio1 23 0>;
#address-cells = <1>;
#size-cells = <0>;
i2c@1 {
...
};
i2c@3 {
...
};
};
General purpose mux DT snippet:
mux: mux-controller {
compatible = "gpio-mux";
#mux-control-cells = <0>;
mux-gpios = <&gpio1 22 0 &gpio1 23 0>;
};
i2c-mux {
compatible = "i2c-mux";
i2c-parent = <&i2c1>;
mux-controls = <&mux>;
#address-cells = <1>;
#size-cells = <0>;
i2c@1 {
...
};
i2c@3 {
...
};
};
I would love to find a way to cleanly get the mux framework to handle
the first DT as well, and thus being able to obsolete the dedicated
i2c-mux-gpio driver. I have not figured out how to accomplish that
without abusing the driver-model to a point that it's not working.
Help with that task is dearly appreciated.
What I have stumbled on, I think, is that two drivers needs to be
instantiated from the same DT node. At the same time, I need the
mux framework to handle the current out-of-node thing with a
phandle as well, so that several mux consumers can share a common
mux controller. My understanding of these matters are apparently not
deep enough...
I think you would like a DT that looks more like the first DT
snippet but still enjoy the flexibility of the mux framework and w/o
implementing a (another) full muxing sub-sub-system like the i2c
sub-system has done. Correct?
Cheers,
peda
^ permalink raw reply [flat|nested] 38+ messages in thread* Re: [PATCH v13 02/10] dt-bindings: document devicetree bindings for mux-controllers and gpio-mux
2017-04-19 10:41 ` Peter Rosin
@ 2017-04-19 11:05 ` Philipp Zabel
2017-04-19 11:23 ` Peter Rosin
0 siblings, 1 reply; 38+ messages in thread
From: Philipp Zabel @ 2017-04-19 11:05 UTC (permalink / raw)
To: Peter Rosin
Cc: linux-kernel, Greg Kroah-Hartman, Mark Rutland, devicetree,
Lars-Peter Clausen, Wolfram Sang, linux-iio, Jonathan Corbet,
linux-doc, kernel, Paul Gortmaker, Rob Herring, linux-i2c,
Peter Meerwald-Stadler, Hartmut Knaack, Colin Ian King,
Andrew Morton, Jonathan Cameron
On Wed, 2017-04-19 at 12:41 +0200, Peter Rosin wrote:
> On 2017-04-19 11:17, Philipp Zabel wrote:
> > On Tue, 2017-04-18 at 15:36 +0200, Peter Rosin wrote:
> >> If I got things wrong when I skimmed whatever I came across, and if the
> >> mmio register is the only mux control option in the stars, it becomes
> >> less obvious... It's of course still possible to hook into the mux
> >> subsystem, but the benefit is questionable. And you do get the extra
> >> device tree node. You could of course also implement a mux driver
> >> outside of drivers/mux and thus make use of the mux api, but it's tiny
> >> and any benefit is truly small.
> >
> > What I wondered mostly is whether it would be a good idea to move the
> > OF-graph ports into the mux controller node, and let the video capture
> > device be the consumer of the mux.
> > But this wouldn't fit well with the clear split between the mux
> > controller and the actual mux hardware in the mux DT bindings.
>
> I have tried to do something similar. I think. The current
> drivers/i2c/muxes/i2c-mux-gpio.c is a good candidate for the same thing
> IIUC.
>
> That dedicated driver and the general purpose i2c mux driver does pretty
> much the same thing with these two DT snippets:
>
> Dedicated i2c-mux-gpio DT snippet:
>
> i2c-mux {
> compatible = "i2c-mux-gpio";
> i2c-parent = <&i2c1>;
>
> mux-gpios = <&gpio1 22 0 &gpio1 23 0>;
>
> #address-cells = <1>;
> #size-cells = <0>;
>
> i2c@1 {
> ...
> };
>
> i2c@3 {
> ...
> };
> };
>
> General purpose mux DT snippet:
>
> mux: mux-controller {
> compatible = "gpio-mux";
> #mux-control-cells = <0>;
>
> mux-gpios = <&gpio1 22 0 &gpio1 23 0>;
> };
>
> i2c-mux {
> compatible = "i2c-mux";
> i2c-parent = <&i2c1>;
>
> mux-controls = <&mux>;
>
> #address-cells = <1>;
> #size-cells = <0>;
>
> i2c@1 {
> ...
> };
>
> i2c@3 {
> ...
> };
> };
Yes, replace i2c-mux with video-mux and the i2c@x nodes with port@x
nodes, and this is very close to what I am thinking about.
> I would love to find a way to cleanly get the mux framework to handle
> the first DT as well, and thus being able to obsolete the dedicated
> i2c-mux-gpio driver. I have not figured out how to accomplish that
> without abusing the driver-model to a point that it's not working.
> Help with that task is dearly appreciated.
>
> What I have stumbled on, I think, is that two drivers needs to be
> instantiated from the same DT node. At the same time, I need the
> mux framework to handle the current out-of-node thing with a
> phandle as well, so that several mux consumers can share a common
> mux controller. My understanding of these matters are apparently not
> deep enough...
Not necessarily, if the framework could export a function to create a
gpio/mmio mux_chip on a given device and the gpio-mux and *-mux-gpio
drivers just reuse that.
> I think you would like a DT that looks more like the first DT
> snippet but still enjoy the flexibility of the mux framework and w/o
> implementing a (another) full muxing sub-sub-system like the i2c
> sub-system has done. Correct?
Correct.
regards
Philipp
^ permalink raw reply [flat|nested] 38+ messages in thread* Re: [PATCH v13 02/10] dt-bindings: document devicetree bindings for mux-controllers and gpio-mux
2017-04-19 11:05 ` Philipp Zabel
@ 2017-04-19 11:23 ` Peter Rosin
2017-04-19 16:34 ` Philipp Zabel
0 siblings, 1 reply; 38+ messages in thread
From: Peter Rosin @ 2017-04-19 11:23 UTC (permalink / raw)
To: Philipp Zabel
Cc: linux-kernel, Greg Kroah-Hartman, Mark Rutland, devicetree,
Lars-Peter Clausen, Wolfram Sang, linux-iio, Jonathan Corbet,
linux-doc, kernel, Paul Gortmaker, Rob Herring, linux-i2c,
Peter Meerwald-Stadler, Hartmut Knaack, Colin Ian King,
Andrew Morton, Jonathan Cameron
On 2017-04-19 13:05, Philipp Zabel wrote:
> On Wed, 2017-04-19 at 12:41 +0200, Peter Rosin wrote:
>> On 2017-04-19 11:17, Philipp Zabel wrote:
>>> On Tue, 2017-04-18 at 15:36 +0200, Peter Rosin wrote:
>>>> If I got things wrong when I skimmed whatever I came across, and if the
>>>> mmio register is the only mux control option in the stars, it becomes
>>>> less obvious... It's of course still possible to hook into the mux
>>>> subsystem, but the benefit is questionable. And you do get the extra
>>>> device tree node. You could of course also implement a mux driver
>>>> outside of drivers/mux and thus make use of the mux api, but it's tiny
>>>> and any benefit is truly small.
>>>
>>> What I wondered mostly is whether it would be a good idea to move the
>>> OF-graph ports into the mux controller node, and let the video capture
>>> device be the consumer of the mux.
>>> But this wouldn't fit well with the clear split between the mux
>>> controller and the actual mux hardware in the mux DT bindings.
>>
>> I have tried to do something similar. I think. The current
>> drivers/i2c/muxes/i2c-mux-gpio.c is a good candidate for the same thing
>> IIUC.
>>
>> That dedicated driver and the general purpose i2c mux driver does pretty
>> much the same thing with these two DT snippets:
>>
>> Dedicated i2c-mux-gpio DT snippet:
>>
>> i2c-mux {
>> compatible = "i2c-mux-gpio";
>> i2c-parent = <&i2c1>;
>>
>> mux-gpios = <&gpio1 22 0 &gpio1 23 0>;
>>
>> #address-cells = <1>;
>> #size-cells = <0>;
>>
>> i2c@1 {
>> ...
>> };
>>
>> i2c@3 {
>> ...
>> };
>> };
>>
>> General purpose mux DT snippet:
>>
>> mux: mux-controller {
>> compatible = "gpio-mux";
>> #mux-control-cells = <0>;
>>
>> mux-gpios = <&gpio1 22 0 &gpio1 23 0>;
>> };
>>
>> i2c-mux {
>> compatible = "i2c-mux";
>> i2c-parent = <&i2c1>;
>>
>> mux-controls = <&mux>;
>>
>> #address-cells = <1>;
>> #size-cells = <0>;
>>
>> i2c@1 {
>> ...
>> };
>>
>> i2c@3 {
>> ...
>> };
>> };
>
> Yes, replace i2c-mux with video-mux and the i2c@x nodes with port@x
> nodes, and this is very close to what I am thinking about.
>
>> I would love to find a way to cleanly get the mux framework to handle
>> the first DT as well, and thus being able to obsolete the dedicated
>> i2c-mux-gpio driver. I have not figured out how to accomplish that
>> without abusing the driver-model to a point that it's not working.
>> Help with that task is dearly appreciated.
>>
>> What I have stumbled on, I think, is that two drivers needs to be
>> instantiated from the same DT node. At the same time, I need the
>> mux framework to handle the current out-of-node thing with a
>> phandle as well, so that several mux consumers can share a common
>> mux controller. My understanding of these matters are apparently not
>> deep enough...
>
> Not necessarily, if the framework could export a function to create a
> gpio/mmio mux_chip on a given device and the gpio-mux and *-mux-gpio
> drivers just reuse that.
I've been up that creek. Why should the gpio mux be special cased?
That's not clean, the implication is that all mux consumers need
to handle the gpio case and have a special compatible for that
case etc. Then someone thinks the DT should look equally "clean" for
some i2c based mux, and the weeds start piling up. This is exactly
what we don't want. We want the mux consumer drivers to be totally
agnostic about the fact that they happen to use a gpio mux.
Cheers,
peda
^ permalink raw reply [flat|nested] 38+ messages in thread* Re: [PATCH v13 02/10] dt-bindings: document devicetree bindings for mux-controllers and gpio-mux
2017-04-19 11:23 ` Peter Rosin
@ 2017-04-19 16:34 ` Philipp Zabel
0 siblings, 0 replies; 38+ messages in thread
From: Philipp Zabel @ 2017-04-19 16:34 UTC (permalink / raw)
To: Peter Rosin
Cc: linux-kernel, Greg Kroah-Hartman, Mark Rutland, devicetree,
Lars-Peter Clausen, Wolfram Sang, linux-iio, Jonathan Corbet,
linux-doc, kernel, Paul Gortmaker, Rob Herring, linux-i2c,
Peter Meerwald-Stadler, Hartmut Knaack, Colin Ian King,
Andrew Morton, Jonathan Cameron
On Wed, 2017-04-19 at 13:23 +0200, Peter Rosin wrote:
> On 2017-04-19 13:05, Philipp Zabel wrote:
> > On Wed, 2017-04-19 at 12:41 +0200, Peter Rosin wrote:
> >> On 2017-04-19 11:17, Philipp Zabel wrote:
> >>> On Tue, 2017-04-18 at 15:36 +0200, Peter Rosin wrote:
> >>>> If I got things wrong when I skimmed whatever I came across, and if the
> >>>> mmio register is the only mux control option in the stars, it becomes
> >>>> less obvious... It's of course still possible to hook into the mux
> >>>> subsystem, but the benefit is questionable. And you do get the extra
> >>>> device tree node. You could of course also implement a mux driver
> >>>> outside of drivers/mux and thus make use of the mux api, but it's tiny
> >>>> and any benefit is truly small.
> >>>
> >>> What I wondered mostly is whether it would be a good idea to move the
> >>> OF-graph ports into the mux controller node, and let the video capture
> >>> device be the consumer of the mux.
> >>> But this wouldn't fit well with the clear split between the mux
> >>> controller and the actual mux hardware in the mux DT bindings.
> >>
> >> I have tried to do something similar. I think. The current
> >> drivers/i2c/muxes/i2c-mux-gpio.c is a good candidate for the same thing
> >> IIUC.
> >>
> >> That dedicated driver and the general purpose i2c mux driver does pretty
> >> much the same thing with these two DT snippets:
> >>
> >> Dedicated i2c-mux-gpio DT snippet:
> >>
> >> i2c-mux {
> >> compatible = "i2c-mux-gpio";
> >> i2c-parent = <&i2c1>;
> >>
> >> mux-gpios = <&gpio1 22 0 &gpio1 23 0>;
> >>
> >> #address-cells = <1>;
> >> #size-cells = <0>;
> >>
> >> i2c@1 {
> >> ...
> >> };
> >>
> >> i2c@3 {
> >> ...
> >> };
> >> };
> >>
> >> General purpose mux DT snippet:
> >>
> >> mux: mux-controller {
> >> compatible = "gpio-mux";
> >> #mux-control-cells = <0>;
> >>
> >> mux-gpios = <&gpio1 22 0 &gpio1 23 0>;
> >> };
> >>
> >> i2c-mux {
> >> compatible = "i2c-mux";
> >> i2c-parent = <&i2c1>;
> >>
> >> mux-controls = <&mux>;
> >>
> >> #address-cells = <1>;
> >> #size-cells = <0>;
> >>
> >> i2c@1 {
> >> ...
> >> };
> >>
> >> i2c@3 {
> >> ...
> >> };
> >> };
> >
> > Yes, replace i2c-mux with video-mux and the i2c@x nodes with port@x
> > nodes, and this is very close to what I am thinking about.
> >
> >> I would love to find a way to cleanly get the mux framework to handle
> >> the first DT as well, and thus being able to obsolete the dedicated
> >> i2c-mux-gpio driver. I have not figured out how to accomplish that
> >> without abusing the driver-model to a point that it's not working.
> >> Help with that task is dearly appreciated.
> >>
> >> What I have stumbled on, I think, is that two drivers needs to be
> >> instantiated from the same DT node. At the same time, I need the
> >> mux framework to handle the current out-of-node thing with a
> >> phandle as well, so that several mux consumers can share a common
> >> mux controller. My understanding of these matters are apparently not
> >> deep enough...
> >
> > Not necessarily, if the framework could export a function to create a
> > gpio/mmio mux_chip on a given device and the gpio-mux and *-mux-gpio
> > drivers just reuse that.
>
> I've been up that creek. Why should the gpio mux be special cased?
You are right, this does not scale.
> That's not clean, the implication is that all mux consumers need
> to handle the gpio case and have a special compatible for that
> case etc. Then someone thinks the DT should look equally "clean" for
> some i2c based mux, and the weeds start piling up. This is exactly
> what we don't want. We want the mux consumer drivers to be totally
> agnostic about the fact that they happen to use a gpio mux.
If you want to have i2c-mux-gpio and i2c-mux compatibles, and a single
driver to handle them both, it must at least match both compatibles, so
it can't be completely agnostic.
Why not then have it call
if (/* compatible == "i2c-mux" */)
mux = devm_mux_control_get(dev, NULL);
else /* if (compatible == "i2c-mux-gpio/mmio/etc.") */
mux = devm_mux_control_create(dev);
? The mux framework core could hold a list of those <usage>-mux-<type>
compatibles and dispatch creation of the correct mux (or mux platform
device, if necessary).
regards
Philipp
^ permalink raw reply [flat|nested] 38+ messages in thread
* [PATCH v13 03/10] mux: minimal mux subsystem and gpio-based mux controller
2017-04-13 16:43 [PATCH v13 00/10] mux controller abstraction and iio/i2c muxes Peter Rosin
2017-04-13 16:43 ` [PATCH v13 01/10] devres: trivial whitespace fix Peter Rosin
2017-04-13 16:43 ` [PATCH v13 02/10] dt-bindings: document devicetree bindings for mux-controllers and gpio-mux Peter Rosin
@ 2017-04-13 16:43 ` Peter Rosin
2017-04-18 8:34 ` Philipp Zabel
` (4 more replies)
2017-04-13 16:43 ` [PATCH v13 04/10] iio: inkern: api for manipulating ext_info of iio channels Peter Rosin
` (6 subsequent siblings)
9 siblings, 5 replies; 38+ messages in thread
From: Peter Rosin @ 2017-04-13 16:43 UTC (permalink / raw)
To: linux-kernel, Greg Kroah-Hartman
Cc: Peter Rosin, Wolfram Sang, Rob Herring, Mark Rutland,
Jonathan Cameron, Hartmut Knaack, Lars-Peter Clausen,
Peter Meerwald-Stadler, Jonathan Corbet, linux-i2c, devicetree,
linux-iio, linux-doc, Andrew Morton, Colin Ian King,
Paul Gortmaker, Philipp Zabel, kernel
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.
Reviewed-by: Jonathan Cameron <jic23@kernel.org>
Signed-off-by: Peter Rosin <peda@axentia.se>
---
Documentation/driver-model/devres.txt | 8 +
MAINTAINERS | 2 +
drivers/Kconfig | 2 +
drivers/Makefile | 1 +
drivers/mux/Kconfig | 34 +++
drivers/mux/Makefile | 6 +
drivers/mux/mux-core.c | 422 ++++++++++++++++++++++++++++++++++
drivers/mux/mux-gpio.c | 114 +++++++++
include/linux/mux.h | 252 ++++++++++++++++++++
9 files changed, 841 insertions(+)
create mode 100644 drivers/mux/Kconfig
create mode 100644 drivers/mux/Makefile
create mode 100644 drivers/mux/mux-core.c
create mode 100644 drivers/mux/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 efb8200819d6..e2343d9cbec7 100644
--- a/Documentation/driver-model/devres.txt
+++ b/Documentation/driver-model/devres.txt
@@ -337,6 +337,14 @@ MEM
MFD
devm_mfd_add_devices()
+MUX
+ devm_mux_chip_alloc()
+ devm_mux_chip_free()
+ devm_mux_chip_register()
+ devm_mux_chip_unregister()
+ devm_mux_control_get()
+ devm_mux_control_put()
+
PER-CPU MEM
devm_alloc_percpu()
devm_free_percpu()
diff --git a/MAINTAINERS b/MAINTAINERS
index 7fc06739c8ad..591eba737678 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -8563,6 +8563,8 @@ M: Peter Rosin <peda@axentia.se>
S: Maintained
F: Documentation/devicetree/bindings/mux/
F: include/linux/dt-bindings/mux/
+F: include/linux/mux.h
+F: drivers/mux/
MULTISOUND SOUND DRIVER
M: Andrew Veliath <andrewtv@usa.net>
diff --git a/drivers/Kconfig b/drivers/Kconfig
index 117ca14ccf85..a7ea13e1b869 100644
--- a/drivers/Kconfig
+++ b/drivers/Kconfig
@@ -204,4 +204,6 @@ source "drivers/fpga/Kconfig"
source "drivers/fsi/Kconfig"
+source "drivers/mux/Kconfig"
+
endmenu
diff --git a/drivers/Makefile b/drivers/Makefile
index 2eced9afba53..c0436f6dd5a9 100644
--- a/drivers/Makefile
+++ b/drivers/Makefile
@@ -177,3 +177,4 @@ obj-$(CONFIG_ANDROID) += android/
obj-$(CONFIG_NVMEM) += nvmem/
obj-$(CONFIG_FPGA) += fpga/
obj-$(CONFIG_FSI) += fsi/
+obj-$(CONFIG_MULTIPLEXER) += mux/
diff --git a/drivers/mux/Kconfig b/drivers/mux/Kconfig
new file mode 100644
index 000000000000..41dfe08ead84
--- /dev/null
+++ b/drivers/mux/Kconfig
@@ -0,0 +1,34 @@
+#
+# Multiplexer devices
+#
+
+menuconfig MULTIPLEXER
+ tristate "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.
+
+ To compile the subsystem as a module, choose M here: the module will
+ be called mux-core.
+
+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
+ multiplexers, which in that case will be operated in parallel.
+
+ To compile the driver as a module, choose M here: the module will
+ be called mux-gpio.
+
+endif
diff --git a/drivers/mux/Makefile b/drivers/mux/Makefile
new file mode 100644
index 000000000000..bb16953f6290
--- /dev/null
+++ b/drivers/mux/Makefile
@@ -0,0 +1,6 @@
+#
+# Makefile for multiplexer devices.
+#
+
+obj-$(CONFIG_MULTIPLEXER) += mux-core.o
+obj-$(CONFIG_MUX_GPIO) += mux-gpio.o
diff --git a/drivers/mux/mux-core.c b/drivers/mux/mux-core.c
new file mode 100644
index 000000000000..66a8bccfc3d7
--- /dev/null
+++ b/drivers/mux/mux-core.c
@@ -0,0 +1,422 @@
+/*
+ * Multiplexer subsystem
+ *
+ * Copyright (C) 2017 Axentia Technologies AB
+ *
+ * Author: Peter Rosin <peda@axentia.se>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#define pr_fmt(fmt) "mux-core: " fmt
+
+#include <linux/device.h>
+#include <linux/err.h>
+#include <linux/export.h>
+#include <linux/idr.h>
+#include <linux/init.h>
+#include <linux/module.h>
+#include <linux/mux.h>
+#include <linux/of.h>
+#include <linux/of_platform.h>
+#include <linux/slab.h>
+
+/*
+ * The idle-as-is "state" is not an actual state that may be selected, it
+ * only implies that the state should not be changed. So, use that state
+ * as indication that the cached state of the multiplexer is unknown.
+ */
+#define MUX_CACHE_UNKNOWN MUX_IDLE_AS_IS
+
+static struct class mux_class = {
+ .name = "mux",
+ .owner = THIS_MODULE,
+};
+
+static int __init mux_init(void)
+{
+ return class_register(&mux_class);
+}
+
+static DEFINE_IDA(mux_ida);
+
+static void mux_chip_release(struct device *dev)
+{
+ struct mux_chip *mux_chip = to_mux_chip(dev);
+
+ ida_simple_remove(&mux_ida, mux_chip->id);
+ kfree(mux_chip);
+}
+
+static struct device_type mux_type = {
+ .name = "mux-chip",
+ .release = mux_chip_release,
+};
+
+struct mux_chip *mux_chip_alloc(struct device *dev,
+ unsigned int controllers, size_t sizeof_priv)
+{
+ struct mux_chip *mux_chip;
+ int i;
+
+ if (WARN_ON(!dev || !controllers))
+ return NULL;
+
+ mux_chip = kzalloc(sizeof(*mux_chip) +
+ controllers * sizeof(*mux_chip->mux) +
+ sizeof_priv, GFP_KERNEL);
+ if (!mux_chip)
+ return NULL;
+
+ mux_chip->mux = (struct mux_control *)(mux_chip + 1);
+ mux_chip->dev.class = &mux_class;
+ mux_chip->dev.type = &mux_type;
+ mux_chip->dev.parent = dev;
+ mux_chip->dev.of_node = dev->of_node;
+ dev_set_drvdata(&mux_chip->dev, mux_chip);
+
+ mux_chip->id = ida_simple_get(&mux_ida, 0, 0, GFP_KERNEL);
+ if (mux_chip->id < 0) {
+ pr_err("muxchipX failed to get a device id\n");
+ kfree(mux_chip);
+ return NULL;
+ }
+ dev_set_name(&mux_chip->dev, "muxchip%d", mux_chip->id);
+
+ mux_chip->controllers = controllers;
+ for (i = 0; i < controllers; ++i) {
+ struct mux_control *mux = &mux_chip->mux[i];
+
+ mux->chip = mux_chip;
+ init_rwsem(&mux->lock);
+ mux->cached_state = MUX_CACHE_UNKNOWN;
+ mux->idle_state = MUX_IDLE_AS_IS;
+ }
+
+ device_initialize(&mux_chip->dev);
+
+ return mux_chip;
+}
+EXPORT_SYMBOL_GPL(mux_chip_alloc);
+
+static int mux_control_set(struct mux_control *mux, int state)
+{
+ int ret = mux->chip->ops->set(mux, state);
+
+ mux->cached_state = ret < 0 ? MUX_CACHE_UNKNOWN : state;
+
+ return ret;
+}
+
+int mux_chip_register(struct mux_chip *mux_chip)
+{
+ int i;
+ int ret;
+
+ for (i = 0; i < mux_chip->controllers; ++i) {
+ struct mux_control *mux = &mux_chip->mux[i];
+
+ if (mux->idle_state == mux->cached_state)
+ continue;
+
+ ret = mux_control_set(mux, mux->idle_state);
+ if (ret < 0) {
+ dev_err(&mux_chip->dev, "unable to set idle state\n");
+ return ret;
+ }
+ }
+
+ ret = device_add(&mux_chip->dev);
+ if (ret < 0)
+ dev_err(&mux_chip->dev,
+ "device_add failed in mux_chip_register: %d\n", ret);
+ return ret;
+}
+EXPORT_SYMBOL_GPL(mux_chip_register);
+
+void mux_chip_unregister(struct mux_chip *mux_chip)
+{
+ device_del(&mux_chip->dev);
+}
+EXPORT_SYMBOL_GPL(mux_chip_unregister);
+
+void mux_chip_free(struct mux_chip *mux_chip)
+{
+ if (!mux_chip)
+ return;
+
+ put_device(&mux_chip->dev);
+}
+EXPORT_SYMBOL_GPL(mux_chip_free);
+
+static void devm_mux_chip_release(struct device *dev, void *res)
+{
+ struct mux_chip *mux_chip = *(struct mux_chip **)res;
+
+ mux_chip_free(mux_chip);
+}
+
+struct mux_chip *devm_mux_chip_alloc(struct device *dev,
+ unsigned int controllers,
+ size_t sizeof_priv)
+{
+ struct mux_chip **ptr, *mux_chip;
+
+ ptr = devres_alloc(devm_mux_chip_release, sizeof(*ptr), GFP_KERNEL);
+ if (!ptr)
+ return NULL;
+
+ mux_chip = mux_chip_alloc(dev, controllers, sizeof_priv);
+ if (!mux_chip) {
+ devres_free(ptr);
+ return NULL;
+ }
+
+ *ptr = mux_chip;
+ devres_add(dev, ptr);
+
+ return mux_chip;
+}
+EXPORT_SYMBOL_GPL(devm_mux_chip_alloc);
+
+static int devm_mux_chip_match(struct device *dev, void *res, void *data)
+{
+ struct mux_chip **r = res;
+
+ if (WARN_ON(!r || !*r))
+ return 0;
+
+ return *r == data;
+}
+
+void devm_mux_chip_free(struct device *dev, struct mux_chip *mux_chip)
+{
+ WARN_ON(devres_release(dev, devm_mux_chip_release,
+ devm_mux_chip_match, mux_chip));
+}
+EXPORT_SYMBOL_GPL(devm_mux_chip_free);
+
+static void devm_mux_chip_reg_release(struct device *dev, void *res)
+{
+ struct mux_chip *mux_chip = *(struct mux_chip **)res;
+
+ mux_chip_unregister(mux_chip);
+}
+
+int devm_mux_chip_register(struct device *dev,
+ struct mux_chip *mux_chip)
+{
+ struct mux_chip **ptr;
+ int res;
+
+ ptr = devres_alloc(devm_mux_chip_reg_release, sizeof(*ptr), GFP_KERNEL);
+ if (!ptr)
+ return -ENOMEM;
+
+ res = mux_chip_register(mux_chip);
+ if (res) {
+ devres_free(ptr);
+ return res;
+ }
+
+ *ptr = mux_chip;
+ devres_add(dev, ptr);
+
+ return res;
+}
+EXPORT_SYMBOL_GPL(devm_mux_chip_register);
+
+void devm_mux_chip_unregister(struct device *dev, struct mux_chip *mux_chip)
+{
+ WARN_ON(devres_release(dev, devm_mux_chip_reg_release,
+ devm_mux_chip_match, mux_chip));
+}
+EXPORT_SYMBOL_GPL(devm_mux_chip_unregister);
+
+int mux_control_select(struct mux_control *mux, int state)
+{
+ int ret;
+
+ if (down_read_trylock(&mux->lock)) {
+ if (mux->cached_state == state)
+ return 0;
+
+ /* Sigh, the mux needs updating... */
+ up_read(&mux->lock);
+ }
+
+ /* ...or it's just contended. */
+ down_write(&mux->lock);
+
+ if (mux->cached_state == state) {
+ /*
+ * Hmmm, someone else changed the mux to my liking.
+ * That makes me wonder how long I waited for nothing?
+ */
+ downgrade_write(&mux->lock);
+ return 0;
+ }
+
+ ret = mux_control_set(mux, state);
+ if (ret < 0) {
+ if (mux->idle_state != MUX_IDLE_AS_IS)
+ mux_control_set(mux, mux->idle_state);
+
+ up_write(&mux->lock);
+ return ret;
+ }
+
+ downgrade_write(&mux->lock);
+
+ return 1;
+}
+EXPORT_SYMBOL_GPL(mux_control_select);
+
+int mux_control_deselect(struct mux_control *mux)
+{
+ int ret = 0;
+
+ if (mux->idle_state != MUX_IDLE_AS_IS &&
+ mux->idle_state != mux->cached_state)
+ ret = mux_control_set(mux, mux->idle_state);
+
+ up_read(&mux->lock);
+
+ return ret;
+}
+EXPORT_SYMBOL_GPL(mux_control_deselect);
+
+static int of_dev_node_match(struct device *dev, const void *data)
+{
+ return dev->of_node == data;
+}
+
+static struct mux_chip *of_find_mux_chip_by_node(struct device_node *np)
+{
+ struct device *dev;
+
+ dev = class_find_device(&mux_class, NULL, np, of_dev_node_match);
+
+ return dev ? to_mux_chip(dev) : NULL;
+}
+
+struct mux_control *mux_control_get(struct device *dev, const char *mux_name)
+{
+ struct device_node *np = dev->of_node;
+ struct of_phandle_args args;
+ struct mux_chip *mux_chip;
+ unsigned int controller;
+ int index = 0;
+ int ret;
+
+ if (mux_name) {
+ index = of_property_match_string(np, "mux-control-names",
+ mux_name);
+ if (index < 0) {
+ dev_err(dev, "mux controller '%s' not found\n",
+ mux_name);
+ return ERR_PTR(index);
+ }
+ }
+
+ ret = of_parse_phandle_with_args(np,
+ "mux-controls", "#mux-control-cells",
+ index, &args);
+ if (ret) {
+ dev_err(dev, "%s: failed to get mux-control %s(%i)\n",
+ np->full_name, mux_name ?: "", index);
+ return ERR_PTR(ret);
+ }
+
+ mux_chip = of_find_mux_chip_by_node(args.np);
+ of_node_put(args.np);
+ if (!mux_chip)
+ return ERR_PTR(-EPROBE_DEFER);
+
+ if (args.args_count > 1 ||
+ (!args.args_count && (mux_chip->controllers > 1))) {
+ dev_err(dev, "%s: wrong #mux-control-cells for %s\n",
+ np->full_name, args.np->full_name);
+ return ERR_PTR(-EINVAL);
+ }
+
+ controller = 0;
+ if (args.args_count)
+ controller = args.args[0];
+
+ if (controller >= mux_chip->controllers) {
+ dev_err(dev, "%s: bad mux controller %u specified in %s\n",
+ np->full_name, controller, args.np->full_name);
+ return ERR_PTR(-EINVAL);
+ }
+
+ get_device(&mux_chip->dev);
+ return &mux_chip->mux[controller];
+}
+EXPORT_SYMBOL_GPL(mux_control_get);
+
+void mux_control_put(struct mux_control *mux)
+{
+ put_device(&mux->chip->dev);
+}
+EXPORT_SYMBOL_GPL(mux_control_put);
+
+static void devm_mux_control_release(struct device *dev, void *res)
+{
+ struct mux_control *mux = *(struct mux_control **)res;
+
+ mux_control_put(mux);
+}
+
+struct mux_control *devm_mux_control_get(struct device *dev,
+ const char *mux_name)
+{
+ struct mux_control **ptr, *mux;
+
+ ptr = devres_alloc(devm_mux_control_release, sizeof(*ptr), GFP_KERNEL);
+ if (!ptr)
+ return ERR_PTR(-ENOMEM);
+
+ mux = mux_control_get(dev, mux_name);
+ if (IS_ERR(mux)) {
+ devres_free(ptr);
+ return mux;
+ }
+
+ *ptr = mux;
+ devres_add(dev, ptr);
+
+ return mux;
+}
+EXPORT_SYMBOL_GPL(devm_mux_control_get);
+
+static int devm_mux_control_match(struct device *dev, void *res, void *data)
+{
+ struct mux_control **r = res;
+
+ if (WARN_ON(!r || !*r))
+ return 0;
+
+ return *r == data;
+}
+
+void devm_mux_control_put(struct device *dev, struct mux_control *mux)
+{
+ WARN_ON(devres_release(dev, devm_mux_control_release,
+ devm_mux_control_match, mux));
+}
+EXPORT_SYMBOL_GPL(devm_mux_control_put);
+
+/*
+ * Using subsys_initcall instead of module_init here to ensure - for the
+ * non-modular case - that the subsystem is initialized when mux consumers
+ * and mux controllers start to use it /without/ relying on link order.
+ * For the modular case, the ordering is ensured with module dependencies.
+ */
+subsys_initcall(mux_init);
+
+MODULE_DESCRIPTION("Multiplexer subsystem");
+MODULE_AUTHOR("Peter Rosin <peda@axentia.se>");
+MODULE_LICENSE("GPL v2");
diff --git a/drivers/mux/mux-gpio.c b/drivers/mux/mux-gpio.c
new file mode 100644
index 000000000000..227d3572e6db
--- /dev/null
+++ b/drivers/mux/mux-gpio.c
@@ -0,0 +1,114 @@
+/*
+ * GPIO-controlled multiplexer driver
+ *
+ * Copyright (C) 2017 Axentia Technologies AB
+ *
+ * Author: Peter Rosin <peda@axentia.se>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#include <linux/err.h>
+#include <linux/gpio/consumer.h>
+#include <linux/module.h>
+#include <linux/mux.h>
+#include <linux/of_platform.h>
+#include <linux/platform_device.h>
+#include <linux/property.h>
+
+struct mux_gpio {
+ struct gpio_descs *gpios;
+ int *val;
+};
+
+static int mux_gpio_set(struct mux_control *mux, int state)
+{
+ struct mux_gpio *mux_gpio = mux_chip_priv(mux->chip);
+ int i;
+
+ for (i = 0; i < mux_gpio->gpios->ndescs; i++)
+ mux_gpio->val[i] = (state >> i) & 1;
+
+ gpiod_set_array_value_cansleep(mux_gpio->gpios->ndescs,
+ mux_gpio->gpios->desc,
+ mux_gpio->val);
+
+ return 0;
+}
+
+static const struct mux_control_ops mux_gpio_ops = {
+ .set = mux_gpio_set,
+};
+
+static const struct of_device_id mux_gpio_dt_ids[] = {
+ { .compatible = "gpio-mux", },
+ { /* sentinel */ }
+};
+MODULE_DEVICE_TABLE(of, mux_gpio_dt_ids);
+
+static int mux_gpio_probe(struct platform_device *pdev)
+{
+ struct device *dev = &pdev->dev;
+ struct mux_chip *mux_chip;
+ struct mux_gpio *mux_gpio;
+ int pins;
+ s32 idle_state;
+ int ret;
+
+ pins = gpiod_count(dev, "mux");
+ if (pins < 0)
+ return pins;
+
+ mux_chip = devm_mux_chip_alloc(dev, 1, sizeof(*mux_gpio) +
+ pins * sizeof(*mux_gpio->val));
+ if (!mux_chip)
+ return -ENOMEM;
+
+ mux_gpio = mux_chip_priv(mux_chip);
+ mux_gpio->val = (int *)(mux_gpio + 1);
+ mux_chip->ops = &mux_gpio_ops;
+
+ mux_gpio->gpios = devm_gpiod_get_array(dev, "mux", GPIOD_OUT_LOW);
+ if (IS_ERR(mux_gpio->gpios)) {
+ ret = PTR_ERR(mux_gpio->gpios);
+ if (ret != -EPROBE_DEFER)
+ dev_err(dev, "failed to get gpios\n");
+ return ret;
+ }
+ WARN_ON(pins != mux_gpio->gpios->ndescs);
+ mux_chip->mux->states = 1 << pins;
+
+ ret = device_property_read_u32(dev, "idle-state", (u32 *)&idle_state);
+ if (ret >= 0 && idle_state != MUX_IDLE_AS_IS) {
+ if (idle_state < 0 || idle_state >= mux_chip->mux->states) {
+ dev_err(dev, "invalid idle-state %u\n", idle_state);
+ return -EINVAL;
+ }
+
+ mux_chip->mux->idle_state = idle_state;
+ }
+
+ ret = devm_mux_chip_register(dev, mux_chip);
+ if (ret < 0)
+ return ret;
+
+ dev_info(dev, "%u-way mux-controller registered\n",
+ mux_chip->mux->states);
+
+ return 0;
+}
+
+static struct platform_driver mux_gpio_driver = {
+ .driver = {
+ .name = "gpio-mux",
+ .of_match_table = of_match_ptr(mux_gpio_dt_ids),
+ },
+ .probe = mux_gpio_probe,
+};
+module_platform_driver(mux_gpio_driver);
+
+MODULE_DESCRIPTION("GPIO-controlled multiplexer driver");
+MODULE_AUTHOR("Peter Rosin <peda@axentia.se>");
+MODULE_LICENSE("GPL v2");
diff --git a/include/linux/mux.h b/include/linux/mux.h
new file mode 100644
index 000000000000..febdde4246df
--- /dev/null
+++ b/include/linux/mux.h
@@ -0,0 +1,252 @@
+/*
+ * mux.h - definitions for the multiplexer interface
+ *
+ * Copyright (C) 2017 Axentia Technologies AB
+ *
+ * Author: Peter Rosin <peda@axentia.se>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#ifndef _LINUX_MUX_H
+#define _LINUX_MUX_H
+
+#include <linux/device.h>
+#include <linux/rwsem.h>
+
+struct mux_chip;
+struct mux_control;
+struct platform_device;
+
+/**
+ * struct mux_control_ops - Mux controller operations for a mux chip.
+ * @set: Set the state of the given mux controller.
+ */
+struct mux_control_ops {
+ int (*set)(struct mux_control *mux, int state);
+};
+
+/* These defines match the constants from the dt-bindings. On purpose. */
+#define MUX_IDLE_AS_IS (-1)
+#define MUX_IDLE_DISCONNECT (-2)
+
+/**
+ * struct mux_control - Represents a mux controller.
+ * @lock: Protects the mux controller state.
+ * @chip: The mux chip that is handling this mux controller.
+ * @states: The number of mux controller states.
+ * @cached_state: The current mux controller state, or -1 if none.
+ * @idle_state: The mux controller state to use when inactive, or one
+ * of MUX_IDLE_AS_IS and MUX_IDLE_DISCONNECT.
+ */
+struct mux_control {
+ struct rw_semaphore lock; /* protects the state of the mux */
+
+ struct mux_chip *chip;
+
+ unsigned int states;
+ int cached_state;
+ int idle_state;
+};
+
+/**
+ * struct mux_chip - Represents a chip holding mux controllers.
+ * @controllers: Number of mux controllers handled by the chip.
+ * @mux: Array of mux controllers that are handled.
+ * @dev: Device structure.
+ * @id: Used to identify the device internally.
+ * @ops: Mux controller operations.
+ */
+struct mux_chip {
+ unsigned int controllers;
+ struct mux_control *mux;
+ struct device dev;
+ int id;
+
+ const struct mux_control_ops *ops;
+};
+
+#define to_mux_chip(x) container_of((x), struct mux_chip, dev)
+
+/**
+ * mux_chip_priv() - Get the extra memory reserved by mux_chip_alloc().
+ * @mux_chip: The mux-chip to get the private memory from.
+ *
+ * Return: Pointer to the private memory reserved by the allocator.
+ */
+static inline void *mux_chip_priv(struct mux_chip *mux_chip)
+{
+ return &mux_chip->mux[mux_chip->controllers];
+}
+
+/**
+ * mux_chip_alloc() - Allocate a mux-chip.
+ * @dev: The parent device implementing the mux interface.
+ * @controllers: The number of mux controllers to allocate for this chip.
+ * @sizeof_priv: Size of extra memory area for private use by the caller.
+ *
+ * Return: A pointer to the new mux-chip, NULL on failure.
+ */
+struct mux_chip *mux_chip_alloc(struct device *dev,
+ unsigned int controllers, size_t sizeof_priv);
+
+/**
+ * mux_chip_register() - Register a mux-chip, thus readying the controllers
+ * for use.
+ * @mux_chip: The mux-chip to register.
+ *
+ * Do not retry registration of the same mux-chip on failure. You should
+ * instead put it away with mux_chip_free() and allocate a new one, if you
+ * for some reason would like to retry registration.
+ *
+ * Return: Zero on success or a negative errno on error.
+ */
+int mux_chip_register(struct mux_chip *mux_chip);
+
+/**
+ * mux_chip_unregister() - Take the mux-chip off-line.
+ * @mux_chip: The mux-chip to unregister.
+ *
+ * mux_chip_unregister() reverses the effects of mux_chip_register().
+ * But not completely, you should not try to call mux_chip_register()
+ * on a mux-chip that has been registered before.
+ */
+void mux_chip_unregister(struct mux_chip *mux_chip);
+
+/**
+ * mux_chip_free() - Free the mux-chip for good.
+ * @mux_chip: The mux-chip to free.
+ *
+ * mux_chip_free() reverses the effects of mux_chip_alloc().
+ */
+void mux_chip_free(struct mux_chip *mux_chip);
+
+/**
+ * devm_mux_chip_alloc() - Resource-managed version of mux_chip_alloc().
+ * @dev: The parent device implementing the mux interface.
+ * @controllers: The number of mux controllers to allocate for this chip.
+ * @sizeof_priv: Size of extra memory area for private use by the caller.
+ *
+ * See mux_chip_alloc() for more details.
+ *
+ * Return: A pointer to the new mux-chip, NULL on failure.
+ */
+struct mux_chip *devm_mux_chip_alloc(struct device *dev,
+ unsigned int controllers,
+ size_t sizeof_priv);
+
+/**
+ * devm_mux_chip_register() - Resource-managed version mux_chip_register().
+ * @dev: The parent device implementing the mux interface.
+ * @mux_chip: The mux-chip to register.
+ *
+ * See mux_chip_register() for more details.
+ *
+ * Return: Zero on success or a negative errno on error.
+ */
+int devm_mux_chip_register(struct device *dev, struct mux_chip *mux_chip);
+
+/**
+ * devm_mux_chip_unregister() - Resource-managed version mux_chip_unregister().
+ * @dev: The device that originally registered the mux-chip.
+ * @mux_chip: The mux-chip to unregister.
+ *
+ * See mux_chip_unregister() for more details.
+ *
+ * Note that you do not normally need to call this function.
+ */
+void devm_mux_chip_unregister(struct device *dev, struct mux_chip *mux_chip);
+
+/**
+ * devm_mux_chip_free() - Resource-managed version mux_chip_free().
+ * @dev: The device that originally got the mux-chip.
+ * @mux_chip: The mux-chip to free.
+ *
+ * See mux_chip_free() for more details.
+ *
+ * Note that you do not normally need to call this function.
+ */
+void devm_mux_chip_free(struct device *dev, struct mux_chip *mux_chip);
+
+/**
+ * mux_control_select() - Select the given multiplexer state.
+ * @mux: The mux-control to request a change of state from.
+ * @state: The new requested state.
+ *
+ * Make sure to call mux_control_deselect() when the operation is complete and
+ * the mux-control is free for others to use, but do not call
+ * mux_control_deselect() if mux_control_select() fails.
+ *
+ * Return: 0 if the requested state was already active, or 1 it the
+ * mux-control state was changed to the requested state. Or a negative
+ * errno on error.
+ *
+ * Note that the difference in return value of zero or one is of
+ * questionable value; especially if the mux-control has several independent
+ * consumers, which is something the consumers should perhaps not be making
+ * assumptions about.
+ */
+int mux_control_select(struct mux_control *mux, int state);
+
+/**
+ * mux_control_deselect() - Deselect the previously selected multiplexer state.
+ * @mux: The mux-control to deselect.
+ *
+ * Return: 0 on success and a negative errno on error. An error can only
+ * occur if the mux has an idle state. Note that even if an error occurs, the
+ * mux-control is unlocked and is thus free for the next access.
+ */
+int mux_control_deselect(struct mux_control *mux);
+
+/**
+ * mux_control_get_index() - Get the index of the given mux controller
+ * @mux: The mux-control to get the index for.
+ *
+ * Return: The index of the mux controller within the mux chip the mux
+ * controller is a part of.
+ */
+static inline unsigned int mux_control_get_index(struct mux_control *mux)
+{
+ return mux - mux->chip->mux;
+}
+
+/**
+ * mux_control_get() - Get the mux-control for a device.
+ * @dev: The device that needs a mux-control.
+ * @mux_name: The name identifying the mux-control.
+ *
+ * Return: A pointer to the mux-control, or an ERR_PTR with a negative errno.
+ */
+struct mux_control *mux_control_get(struct device *dev, const char *mux_name);
+
+/**
+ * mux_control_put() - Put away the mux-control for good.
+ * @mux: The mux-control to put away.
+ *
+ * mux_control_put() reverses the effects of mux_control_get().
+ */
+void mux_control_put(struct mux_control *mux);
+
+/**
+ * devm_mux_control_get() - Get the mux-control for a device, with resource
+ * management.
+ * @dev: The device that needs a mux-control.
+ * @mux_name: The name identifying the mux-control.
+ *
+ * Return: Pointer to the mux-control, or an ERR_PTR with a negative errno.
+ */
+struct mux_control *devm_mux_control_get(struct device *dev,
+ const char *mux_name);
+
+/**
+ * devm_mux_control_put() - Resource-managed version mux_control_put().
+ * @dev: The device that originally got the mux-control.
+ * @mux: The mux-control to put away.
+ *
+ * Note that you do not normally need to call this function.
+ */
+void devm_mux_control_put(struct device *dev, struct mux_control *mux);
+
+#endif /* _LINUX_MUX_H */
--
2.1.4
^ permalink raw reply related [flat|nested] 38+ messages in thread* Re: [PATCH v13 03/10] mux: minimal mux subsystem and gpio-based mux controller
2017-04-13 16:43 ` [PATCH v13 03/10] mux: minimal mux subsystem and gpio-based mux controller Peter Rosin
@ 2017-04-18 8:34 ` Philipp Zabel
2017-04-18 8:51 ` Greg Kroah-Hartman
` (3 subsequent siblings)
4 siblings, 0 replies; 38+ messages in thread
From: Philipp Zabel @ 2017-04-18 8:34 UTC (permalink / raw)
To: Peter Rosin
Cc: linux-kernel, Greg Kroah-Hartman, Wolfram Sang, Rob Herring,
Mark Rutland, Jonathan Cameron, Hartmut Knaack,
Lars-Peter Clausen, Peter Meerwald-Stadler, Jonathan Corbet,
linux-i2c, devicetree, linux-iio, linux-doc, Andrew Morton,
Colin Ian King, Paul Gortmaker, kernel
Hi Peter,
On Thu, 2017-04-13 at 18:43 +0200, Peter Rosin wrote:
[...]
> diff --git a/include/linux/mux.h b/include/linux/mux.h
> new file mode 100644
> index 000000000000..febdde4246df
> --- /dev/null
> +++ b/include/linux/mux.h
[...]
Consider separating mux.h into a consumer header and a driver header.
Right now there is no separation between the consumer API and the
framework internals.
regards
Philipp
^ permalink raw reply [flat|nested] 38+ messages in thread* Re: [PATCH v13 03/10] mux: minimal mux subsystem and gpio-based mux controller
2017-04-13 16:43 ` [PATCH v13 03/10] mux: minimal mux subsystem and gpio-based mux controller Peter Rosin
2017-04-18 8:34 ` Philipp Zabel
@ 2017-04-18 8:51 ` Greg Kroah-Hartman
2017-04-18 10:59 ` Peter Rosin
2017-04-19 9:06 ` Philipp Zabel
` (2 subsequent siblings)
4 siblings, 1 reply; 38+ messages in thread
From: Greg Kroah-Hartman @ 2017-04-18 8:51 UTC (permalink / raw)
To: Peter Rosin
Cc: linux-kernel, Wolfram Sang, Rob Herring, Mark Rutland,
Jonathan Cameron, Hartmut Knaack, Lars-Peter Clausen,
Peter Meerwald-Stadler, Jonathan Corbet, linux-i2c, devicetree,
linux-iio, linux-doc, Andrew Morton, Colin Ian King,
Paul Gortmaker, Philipp Zabel, kernel
On Thu, Apr 13, 2017 at 06:43:07PM +0200, Peter Rosin wrote:
> +config MUX_GPIO
> + tristate "GPIO-controlled Multiplexer"
> + depends on OF && GPIOLIB
Why have the gpio and mux core in the same patch?
And why does this depend on OF?
> + 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
> + multiplexers, which in that case will be operated in parallel.
> +
> + To compile the driver as a module, choose M here: the module will
> + be called mux-gpio.
> +
> +endif
> diff --git a/drivers/mux/Makefile b/drivers/mux/Makefile
> new file mode 100644
> index 000000000000..bb16953f6290
> --- /dev/null
> +++ b/drivers/mux/Makefile
> @@ -0,0 +1,6 @@
> +#
> +# Makefile for multiplexer devices.
> +#
> +
> +obj-$(CONFIG_MULTIPLEXER) += mux-core.o
> +obj-$(CONFIG_MUX_GPIO) += mux-gpio.o
> diff --git a/drivers/mux/mux-core.c b/drivers/mux/mux-core.c
> new file mode 100644
> index 000000000000..66a8bccfc3d7
> --- /dev/null
> +++ b/drivers/mux/mux-core.c
> @@ -0,0 +1,422 @@
> +/*
> + * Multiplexer subsystem
> + *
> + * Copyright (C) 2017 Axentia Technologies AB
> + *
> + * Author: Peter Rosin <peda@axentia.se>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + */
> +
> +#define pr_fmt(fmt) "mux-core: " fmt
> +
> +#include <linux/device.h>
> +#include <linux/err.h>
> +#include <linux/export.h>
> +#include <linux/idr.h>
> +#include <linux/init.h>
> +#include <linux/module.h>
> +#include <linux/mux.h>
> +#include <linux/of.h>
> +#include <linux/of_platform.h>
> +#include <linux/slab.h>
> +
> +/*
> + * The idle-as-is "state" is not an actual state that may be selected, it
> + * only implies that the state should not be changed. So, use that state
> + * as indication that the cached state of the multiplexer is unknown.
> + */
> +#define MUX_CACHE_UNKNOWN MUX_IDLE_AS_IS
> +
> +static struct class mux_class = {
> + .name = "mux",
> + .owner = THIS_MODULE,
> +};
No Documentation/ABI/ update for your sysfs files? Please do so.
> +
> +static int __init mux_init(void)
> +{
> + return class_register(&mux_class);
> +}
> +
> +static DEFINE_IDA(mux_ida);
When your module is unloaded, you forgot to clean this structure up with
what was done with it.
> +
> +static void mux_chip_release(struct device *dev)
> +{
> + struct mux_chip *mux_chip = to_mux_chip(dev);
> +
> + ida_simple_remove(&mux_ida, mux_chip->id);
> + kfree(mux_chip);
> +}
> +
> +static struct device_type mux_type = {
> + .name = "mux-chip",
> + .release = mux_chip_release,
> +};
> +
> +struct mux_chip *mux_chip_alloc(struct device *dev,
> + unsigned int controllers, size_t sizeof_priv)
> +{
> + struct mux_chip *mux_chip;
> + int i;
> +
> + if (WARN_ON(!dev || !controllers))
> + return NULL;
> +
> + mux_chip = kzalloc(sizeof(*mux_chip) +
> + controllers * sizeof(*mux_chip->mux) +
> + sizeof_priv, GFP_KERNEL);
> + if (!mux_chip)
> + return NULL;
You don't return PTR_ERR(-ENOMEM)? Ok, why not? (I'm not arguing for
it, just curious...)
> +
> + mux_chip->mux = (struct mux_control *)(mux_chip + 1);
> + mux_chip->dev.class = &mux_class;
> + mux_chip->dev.type = &mux_type;
> + mux_chip->dev.parent = dev;
> + mux_chip->dev.of_node = dev->of_node;
> + dev_set_drvdata(&mux_chip->dev, mux_chip);
> +
> + mux_chip->id = ida_simple_get(&mux_ida, 0, 0, GFP_KERNEL);
> + if (mux_chip->id < 0) {
> + pr_err("muxchipX failed to get a device id\n");
> + kfree(mux_chip);
> + return NULL;
> + }
> + dev_set_name(&mux_chip->dev, "muxchip%d", mux_chip->id);
> +
> + mux_chip->controllers = controllers;
> + for (i = 0; i < controllers; ++i) {
> + struct mux_control *mux = &mux_chip->mux[i];
> +
> + mux->chip = mux_chip;
> + init_rwsem(&mux->lock);
> + mux->cached_state = MUX_CACHE_UNKNOWN;
> + mux->idle_state = MUX_IDLE_AS_IS;
> + }
> +
> + device_initialize(&mux_chip->dev);
Why are you not registering the device here as well? Why have this be a
two step process?
> +
> + return mux_chip;
> +}
> +EXPORT_SYMBOL_GPL(mux_chip_alloc);
> +
> +static int mux_control_set(struct mux_control *mux, int state)
> +{
> + int ret = mux->chip->ops->set(mux, state);
> +
> + mux->cached_state = ret < 0 ? MUX_CACHE_UNKNOWN : state;
> +
> + return ret;
> +}
> +
> +int mux_chip_register(struct mux_chip *mux_chip)
> +{
> + int i;
> + int ret;
> +
> + for (i = 0; i < mux_chip->controllers; ++i) {
> + struct mux_control *mux = &mux_chip->mux[i];
> +
> + if (mux->idle_state == mux->cached_state)
> + continue;
> +
> + ret = mux_control_set(mux, mux->idle_state);
> + if (ret < 0) {
> + dev_err(&mux_chip->dev, "unable to set idle state\n");
> + return ret;
> + }
> + }
> +
> + ret = device_add(&mux_chip->dev);
> + if (ret < 0)
> + dev_err(&mux_chip->dev,
> + "device_add failed in mux_chip_register: %d\n", ret);
Did you run checkpatch.pl in strict mode on this new file? Please do so :)
> + return ret;
> +}
> +EXPORT_SYMBOL_GPL(mux_chip_register);
> +
> +void mux_chip_unregister(struct mux_chip *mux_chip)
> +{
> + device_del(&mux_chip->dev);
> +}
> +EXPORT_SYMBOL_GPL(mux_chip_unregister);
> +
> +void mux_chip_free(struct mux_chip *mux_chip)
> +{
> + if (!mux_chip)
> + return;
> +
> + put_device(&mux_chip->dev);
> +}
> +EXPORT_SYMBOL_GPL(mux_chip_free);
> +
> +static void devm_mux_chip_release(struct device *dev, void *res)
> +{
> + struct mux_chip *mux_chip = *(struct mux_chip **)res;
> +
> + mux_chip_free(mux_chip);
> +}
> +
> +struct mux_chip *devm_mux_chip_alloc(struct device *dev,
> + unsigned int controllers,
> + size_t sizeof_priv)
> +{
> + struct mux_chip **ptr, *mux_chip;
> +
> + ptr = devres_alloc(devm_mux_chip_release, sizeof(*ptr), GFP_KERNEL);
> + if (!ptr)
> + return NULL;
> +
> + mux_chip = mux_chip_alloc(dev, controllers, sizeof_priv);
> + if (!mux_chip) {
> + devres_free(ptr);
> + return NULL;
> + }
> +
> + *ptr = mux_chip;
> + devres_add(dev, ptr);
> +
> + return mux_chip;
> +}
> +EXPORT_SYMBOL_GPL(devm_mux_chip_alloc);
Having devm functions that create/destroy other struct devices worries
me, do we have other examples of this happening today? Are you sure you
got the reference counting all correct?
> +
> +static int devm_mux_chip_match(struct device *dev, void *res, void *data)
> +{
> + struct mux_chip **r = res;
> +
> + if (WARN_ON(!r || !*r))
How can this happen?
> + return 0;
> +
> + return *r == data;
> +}
> +
> +void devm_mux_chip_free(struct device *dev, struct mux_chip *mux_chip)
> +{
> + WARN_ON(devres_release(dev, devm_mux_chip_release,
> + devm_mux_chip_match, mux_chip));
What can someone do with these WARN_ON() splats in the kernel log?
> +}
> +EXPORT_SYMBOL_GPL(devm_mux_chip_free);
> +
> +static void devm_mux_chip_reg_release(struct device *dev, void *res)
> +{
> + struct mux_chip *mux_chip = *(struct mux_chip **)res;
> +
> + mux_chip_unregister(mux_chip);
> +}
> +
> +int devm_mux_chip_register(struct device *dev,
> + struct mux_chip *mux_chip)
> +{
> + struct mux_chip **ptr;
> + int res;
> +
> + ptr = devres_alloc(devm_mux_chip_reg_release, sizeof(*ptr), GFP_KERNEL);
> + if (!ptr)
> + return -ENOMEM;
> +
> + res = mux_chip_register(mux_chip);
> + if (res) {
> + devres_free(ptr);
> + return res;
> + }
> +
> + *ptr = mux_chip;
> + devres_add(dev, ptr);
> +
> + return res;
> +}
> +EXPORT_SYMBOL_GPL(devm_mux_chip_register);
> +
> +void devm_mux_chip_unregister(struct device *dev, struct mux_chip *mux_chip)
> +{
> + WARN_ON(devres_release(dev, devm_mux_chip_reg_release,
> + devm_mux_chip_match, mux_chip));
> +}
> +EXPORT_SYMBOL_GPL(devm_mux_chip_unregister);
> +
> +int mux_control_select(struct mux_control *mux, int state)
> +{
> + int ret;
> +
> + if (down_read_trylock(&mux->lock)) {
> + if (mux->cached_state == state)
> + return 0;
> +
> + /* Sigh, the mux needs updating... */
> + up_read(&mux->lock);
> + }
> +
> + /* ...or it's just contended. */
> + down_write(&mux->lock);
Why use a read/write lock at all? Have you tested this to verify it
really is faster and needed?
> +
> + if (mux->cached_state == state) {
> + /*
> + * Hmmm, someone else changed the mux to my liking.
> + * That makes me wonder how long I waited for nothing?
> + */
> + downgrade_write(&mux->lock);
Oh that always scares me... Are you _sure_ this is correct? And
needed?
> + return 0;
> + }
> +
> + ret = mux_control_set(mux, state);
> + if (ret < 0) {
> + if (mux->idle_state != MUX_IDLE_AS_IS)
> + mux_control_set(mux, mux->idle_state);
> +
> + up_write(&mux->lock);
> + return ret;
> + }
> +
> + downgrade_write(&mux->lock);
> +
> + return 1;
> +}
> +EXPORT_SYMBOL_GPL(mux_control_select);
> +
> +int mux_control_deselect(struct mux_control *mux)
> +{
> + int ret = 0;
> +
> + if (mux->idle_state != MUX_IDLE_AS_IS &&
> + mux->idle_state != mux->cached_state)
> + ret = mux_control_set(mux, mux->idle_state);
> +
> + up_read(&mux->lock);
You require a lock to be held for a "global" function? Without
documentation? Or even a sparse marking? That's asking for trouble...
> +
> + return ret;
> +}
> +EXPORT_SYMBOL_GPL(mux_control_deselect);
> +
> +static int of_dev_node_match(struct device *dev, const void *data)
> +{
> + return dev->of_node == data;
> +}
> +
> +static struct mux_chip *of_find_mux_chip_by_node(struct device_node *np)
> +{
> + struct device *dev;
> +
> + dev = class_find_device(&mux_class, NULL, np, of_dev_node_match);
> +
> + return dev ? to_mux_chip(dev) : NULL;
> +}
> +
> +struct mux_control *mux_control_get(struct device *dev, const char *mux_name)
> +{
> + struct device_node *np = dev->of_node;
> + struct of_phandle_args args;
> + struct mux_chip *mux_chip;
> + unsigned int controller;
> + int index = 0;
> + int ret;
> +
> + if (mux_name) {
> + index = of_property_match_string(np, "mux-control-names",
> + mux_name);
> + if (index < 0) {
> + dev_err(dev, "mux controller '%s' not found\n",
> + mux_name);
> + return ERR_PTR(index);
> + }
> + }
> +
> + ret = of_parse_phandle_with_args(np,
> + "mux-controls", "#mux-control-cells",
> + index, &args);
> + if (ret) {
> + dev_err(dev, "%s: failed to get mux-control %s(%i)\n",
> + np->full_name, mux_name ?: "", index);
> + return ERR_PTR(ret);
> + }
> +
> + mux_chip = of_find_mux_chip_by_node(args.np);
> + of_node_put(args.np);
> + if (!mux_chip)
> + return ERR_PTR(-EPROBE_DEFER);
> +
> + if (args.args_count > 1 ||
> + (!args.args_count && (mux_chip->controllers > 1))) {
> + dev_err(dev, "%s: wrong #mux-control-cells for %s\n",
> + np->full_name, args.np->full_name);
> + return ERR_PTR(-EINVAL);
> + }
> +
> + controller = 0;
> + if (args.args_count)
> + controller = args.args[0];
> +
> + if (controller >= mux_chip->controllers) {
> + dev_err(dev, "%s: bad mux controller %u specified in %s\n",
> + np->full_name, controller, args.np->full_name);
> + return ERR_PTR(-EINVAL);
> + }
> +
> + get_device(&mux_chip->dev);
> + return &mux_chip->mux[controller];
> +}
> +EXPORT_SYMBOL_GPL(mux_control_get);
> +
> +void mux_control_put(struct mux_control *mux)
> +{
> + put_device(&mux->chip->dev);
> +}
> +EXPORT_SYMBOL_GPL(mux_control_put);
> +
> +static void devm_mux_control_release(struct device *dev, void *res)
> +{
> + struct mux_control *mux = *(struct mux_control **)res;
> +
> + mux_control_put(mux);
> +}
> +
> +struct mux_control *devm_mux_control_get(struct device *dev,
> + const char *mux_name)
> +{
> + struct mux_control **ptr, *mux;
> +
> + ptr = devres_alloc(devm_mux_control_release, sizeof(*ptr), GFP_KERNEL);
> + if (!ptr)
> + return ERR_PTR(-ENOMEM);
> +
> + mux = mux_control_get(dev, mux_name);
> + if (IS_ERR(mux)) {
> + devres_free(ptr);
> + return mux;
> + }
> +
> + *ptr = mux;
> + devres_add(dev, ptr);
> +
> + return mux;
> +}
> +EXPORT_SYMBOL_GPL(devm_mux_control_get);
> +
> +static int devm_mux_control_match(struct device *dev, void *res, void *data)
> +{
> + struct mux_control **r = res;
> +
> + if (WARN_ON(!r || !*r))
> + return 0;
Same here, how can this happen?
> +
> + return *r == data;
> +}
> +
> +void devm_mux_control_put(struct device *dev, struct mux_control *mux)
> +{
> + WARN_ON(devres_release(dev, devm_mux_control_release,
> + devm_mux_control_match, mux));
> +}
> +EXPORT_SYMBOL_GPL(devm_mux_control_put);
> +
> +/*
> + * Using subsys_initcall instead of module_init here to ensure - for the
> + * non-modular case - that the subsystem is initialized when mux consumers
> + * and mux controllers start to use it /without/ relying on link order.
> + * For the modular case, the ordering is ensured with module dependencies.
> + */
> +subsys_initcall(mux_init);
Even with subsys_initcall you are relying on link order, you do realize
that? What about other subsystems that rely on this? :)
> +
> +MODULE_DESCRIPTION("Multiplexer subsystem");
> +MODULE_AUTHOR("Peter Rosin <peda@axentia.se>");
> +MODULE_LICENSE("GPL v2");
> diff --git a/drivers/mux/mux-gpio.c b/drivers/mux/mux-gpio.c
> new file mode 100644
> index 000000000000..227d3572e6db
> --- /dev/null
> +++ b/drivers/mux/mux-gpio.c
> @@ -0,0 +1,114 @@
> +/*
> + * GPIO-controlled multiplexer driver
> + *
> + * Copyright (C) 2017 Axentia Technologies AB
> + *
> + * Author: Peter Rosin <peda@axentia.se>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + */
> +
> +#include <linux/err.h>
> +#include <linux/gpio/consumer.h>
> +#include <linux/module.h>
> +#include <linux/mux.h>
> +#include <linux/of_platform.h>
> +#include <linux/platform_device.h>
> +#include <linux/property.h>
> +
> +struct mux_gpio {
> + struct gpio_descs *gpios;
> + int *val;
> +};
> +
> +static int mux_gpio_set(struct mux_control *mux, int state)
> +{
> + struct mux_gpio *mux_gpio = mux_chip_priv(mux->chip);
> + int i;
> +
> + for (i = 0; i < mux_gpio->gpios->ndescs; i++)
> + mux_gpio->val[i] = (state >> i) & 1;
> +
> + gpiod_set_array_value_cansleep(mux_gpio->gpios->ndescs,
> + mux_gpio->gpios->desc,
> + mux_gpio->val);
> +
> + return 0;
> +}
> +
> +static const struct mux_control_ops mux_gpio_ops = {
> + .set = mux_gpio_set,
> +};
> +
> +static const struct of_device_id mux_gpio_dt_ids[] = {
> + { .compatible = "gpio-mux", },
> + { /* sentinel */ }
> +};
> +MODULE_DEVICE_TABLE(of, mux_gpio_dt_ids);
> +
> +static int mux_gpio_probe(struct platform_device *pdev)
> +{
> + struct device *dev = &pdev->dev;
> + struct mux_chip *mux_chip;
> + struct mux_gpio *mux_gpio;
> + int pins;
> + s32 idle_state;
> + int ret;
> +
> + pins = gpiod_count(dev, "mux");
> + if (pins < 0)
> + return pins;
> +
> + mux_chip = devm_mux_chip_alloc(dev, 1, sizeof(*mux_gpio) +
> + pins * sizeof(*mux_gpio->val));
> + if (!mux_chip)
> + return -ENOMEM;
> +
> + mux_gpio = mux_chip_priv(mux_chip);
> + mux_gpio->val = (int *)(mux_gpio + 1);
> + mux_chip->ops = &mux_gpio_ops;
> +
> + mux_gpio->gpios = devm_gpiod_get_array(dev, "mux", GPIOD_OUT_LOW);
> + if (IS_ERR(mux_gpio->gpios)) {
> + ret = PTR_ERR(mux_gpio->gpios);
> + if (ret != -EPROBE_DEFER)
> + dev_err(dev, "failed to get gpios\n");
> + return ret;
> + }
> + WARN_ON(pins != mux_gpio->gpios->ndescs);
> + mux_chip->mux->states = 1 << pins;
> +
> + ret = device_property_read_u32(dev, "idle-state", (u32 *)&idle_state);
> + if (ret >= 0 && idle_state != MUX_IDLE_AS_IS) {
> + if (idle_state < 0 || idle_state >= mux_chip->mux->states) {
> + dev_err(dev, "invalid idle-state %u\n", idle_state);
> + return -EINVAL;
> + }
> +
> + mux_chip->mux->idle_state = idle_state;
> + }
> +
> + ret = devm_mux_chip_register(dev, mux_chip);
> + if (ret < 0)
> + return ret;
> +
> + dev_info(dev, "%u-way mux-controller registered\n",
> + mux_chip->mux->states);
> +
> + return 0;
> +}
> +
> +static struct platform_driver mux_gpio_driver = {
> + .driver = {
> + .name = "gpio-mux",
> + .of_match_table = of_match_ptr(mux_gpio_dt_ids),
> + },
> + .probe = mux_gpio_probe,
> +};
> +module_platform_driver(mux_gpio_driver);
> +
> +MODULE_DESCRIPTION("GPIO-controlled multiplexer driver");
> +MODULE_AUTHOR("Peter Rosin <peda@axentia.se>");
> +MODULE_LICENSE("GPL v2");
> diff --git a/include/linux/mux.h b/include/linux/mux.h
> new file mode 100644
> index 000000000000..febdde4246df
> --- /dev/null
> +++ b/include/linux/mux.h
> @@ -0,0 +1,252 @@
> +/*
> + * mux.h - definitions for the multiplexer interface
> + *
> + * Copyright (C) 2017 Axentia Technologies AB
> + *
> + * Author: Peter Rosin <peda@axentia.se>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + */
> +
> +#ifndef _LINUX_MUX_H
> +#define _LINUX_MUX_H
> +
> +#include <linux/device.h>
> +#include <linux/rwsem.h>
> +
> +struct mux_chip;
> +struct mux_control;
> +struct platform_device;
> +
> +/**
> + * struct mux_control_ops - Mux controller operations for a mux chip.
> + * @set: Set the state of the given mux controller.
> + */
> +struct mux_control_ops {
> + int (*set)(struct mux_control *mux, int state);
> +};
> +
> +/* These defines match the constants from the dt-bindings. On purpose. */
Why on purpose?
> +#define MUX_IDLE_AS_IS (-1)
> +#define MUX_IDLE_DISCONNECT (-2)
> +
> +/**
> + * struct mux_control - Represents a mux controller.
> + * @lock: Protects the mux controller state.
> + * @chip: The mux chip that is handling this mux controller.
> + * @states: The number of mux controller states.
> + * @cached_state: The current mux controller state, or -1 if none.
> + * @idle_state: The mux controller state to use when inactive, or one
> + * of MUX_IDLE_AS_IS and MUX_IDLE_DISCONNECT.
> + */
> +struct mux_control {
> + struct rw_semaphore lock; /* protects the state of the mux */
> +
> + struct mux_chip *chip;
> +
> + unsigned int states;
> + int cached_state;
> + int idle_state;
> +};
> +
> +/**
> + * struct mux_chip - Represents a chip holding mux controllers.
> + * @controllers: Number of mux controllers handled by the chip.
> + * @mux: Array of mux controllers that are handled.
> + * @dev: Device structure.
> + * @id: Used to identify the device internally.
> + * @ops: Mux controller operations.
> + */
> +struct mux_chip {
> + unsigned int controllers;
> + struct mux_control *mux;
> + struct device dev;
> + int id;
> +
> + const struct mux_control_ops *ops;
> +};
> +
> +#define to_mux_chip(x) container_of((x), struct mux_chip, dev)
> +
> +/**
> + * mux_chip_priv() - Get the extra memory reserved by mux_chip_alloc().
> + * @mux_chip: The mux-chip to get the private memory from.
> + *
> + * Return: Pointer to the private memory reserved by the allocator.
> + */
> +static inline void *mux_chip_priv(struct mux_chip *mux_chip)
> +{
> + return &mux_chip->mux[mux_chip->controllers];
> +}
> +
> +/**
> + * mux_chip_alloc() - Allocate a mux-chip.
> + * @dev: The parent device implementing the mux interface.
> + * @controllers: The number of mux controllers to allocate for this chip.
> + * @sizeof_priv: Size of extra memory area for private use by the caller.
> + *
> + * Return: A pointer to the new mux-chip, NULL on failure.
> + */
> +struct mux_chip *mux_chip_alloc(struct device *dev,
> + unsigned int controllers, size_t sizeof_priv);
> +
Don't put kernel doc comments in a .h file, they normally go into the .c
file, next to the code itself. That makes it easier to fix up and
realise when they need to be changed when the code changes. The .h file
rarely changes.
> +/**
> + * mux_chip_register() - Register a mux-chip, thus readying the controllers
> + * for use.
> + * @mux_chip: The mux-chip to register.
> + *
> + * Do not retry registration of the same mux-chip on failure. You should
> + * instead put it away with mux_chip_free() and allocate a new one, if you
> + * for some reason would like to retry registration.
> + *
> + * Return: Zero on success or a negative errno on error.
> + */
> +int mux_chip_register(struct mux_chip *mux_chip);
> +
> +/**
> + * mux_chip_unregister() - Take the mux-chip off-line.
> + * @mux_chip: The mux-chip to unregister.
> + *
> + * mux_chip_unregister() reverses the effects of mux_chip_register().
> + * But not completely, you should not try to call mux_chip_register()
> + * on a mux-chip that has been registered before.
> + */
> +void mux_chip_unregister(struct mux_chip *mux_chip);
> +
> +/**
> + * mux_chip_free() - Free the mux-chip for good.
> + * @mux_chip: The mux-chip to free.
> + *
> + * mux_chip_free() reverses the effects of mux_chip_alloc().
> + */
> +void mux_chip_free(struct mux_chip *mux_chip);
> +
> +/**
> + * devm_mux_chip_alloc() - Resource-managed version of mux_chip_alloc().
> + * @dev: The parent device implementing the mux interface.
> + * @controllers: The number of mux controllers to allocate for this chip.
> + * @sizeof_priv: Size of extra memory area for private use by the caller.
> + *
> + * See mux_chip_alloc() for more details.
> + *
> + * Return: A pointer to the new mux-chip, NULL on failure.
> + */
> +struct mux_chip *devm_mux_chip_alloc(struct device *dev,
> + unsigned int controllers,
> + size_t sizeof_priv);
> +
> +/**
> + * devm_mux_chip_register() - Resource-managed version mux_chip_register().
> + * @dev: The parent device implementing the mux interface.
> + * @mux_chip: The mux-chip to register.
> + *
> + * See mux_chip_register() for more details.
> + *
> + * Return: Zero on success or a negative errno on error.
> + */
> +int devm_mux_chip_register(struct device *dev, struct mux_chip *mux_chip);
> +
> +/**
> + * devm_mux_chip_unregister() - Resource-managed version mux_chip_unregister().
> + * @dev: The device that originally registered the mux-chip.
> + * @mux_chip: The mux-chip to unregister.
> + *
> + * See mux_chip_unregister() for more details.
> + *
> + * Note that you do not normally need to call this function.
Odd, then why is it exported???
> + */
> +void devm_mux_chip_unregister(struct device *dev, struct mux_chip *mux_chip);
> +
> +/**
> + * devm_mux_chip_free() - Resource-managed version mux_chip_free().
> + * @dev: The device that originally got the mux-chip.
> + * @mux_chip: The mux-chip to free.
> + *
> + * See mux_chip_free() for more details.
> + *
> + * Note that you do not normally need to call this function.
> + */
> +void devm_mux_chip_free(struct device *dev, struct mux_chip *mux_chip);
> +
> +/**
> + * mux_control_select() - Select the given multiplexer state.
> + * @mux: The mux-control to request a change of state from.
> + * @state: The new requested state.
> + *
> + * Make sure to call mux_control_deselect() when the operation is complete and
> + * the mux-control is free for others to use, but do not call
> + * mux_control_deselect() if mux_control_select() fails.
> + *
> + * Return: 0 if the requested state was already active, or 1 it the
> + * mux-control state was changed to the requested state. Or a negative
> + * errno on error.
> + *
> + * Note that the difference in return value of zero or one is of
> + * questionable value; especially if the mux-control has several independent
> + * consumers, which is something the consumers should perhaps not be making
> + * assumptions about.
I don't understand this note, what is a user of this api supposed to do
differently between 1 and 0? Why make the difference at all?
And I agree with the comment to split this up into 2 different .h files,
if possible.
thanks,
greg k-h
^ permalink raw reply [flat|nested] 38+ messages in thread* Re: [PATCH v13 03/10] mux: minimal mux subsystem and gpio-based mux controller
2017-04-18 8:51 ` Greg Kroah-Hartman
@ 2017-04-18 10:59 ` Peter Rosin
2017-04-18 11:44 ` Greg Kroah-Hartman
2017-05-05 13:19 ` Peter Rosin
0 siblings, 2 replies; 38+ messages in thread
From: Peter Rosin @ 2017-04-18 10:59 UTC (permalink / raw)
To: Greg Kroah-Hartman
Cc: linux-kernel, Wolfram Sang, Rob Herring, Mark Rutland,
Jonathan Cameron, Hartmut Knaack, Lars-Peter Clausen,
Peter Meerwald-Stadler, Jonathan Corbet, linux-i2c, devicetree,
linux-iio, linux-doc, Andrew Morton, Colin Ian King,
Paul Gortmaker, Philipp Zabel, kernel
On 2017-04-18 10:51, Greg Kroah-Hartman wrote:
> On Thu, Apr 13, 2017 at 06:43:07PM +0200, Peter Rosin wrote:
>> +config MUX_GPIO
>> + tristate "GPIO-controlled Multiplexer"
>> + depends on OF && GPIOLIB
>
> Why have the gpio and mux core in the same patch?
One is not usable w/o the other. I can split them if you want to?
> And why does this depend on OF?
That's historical, I was originally using of_property_read_u32.
I'll remove the dep...
>> + 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
>> + multiplexers, which in that case will be operated in parallel.
>> +
>> + To compile the driver as a module, choose M here: the module will
>> + be called mux-gpio.
>> +
>> +endif
>> diff --git a/drivers/mux/Makefile b/drivers/mux/Makefile
>> new file mode 100644
>> index 000000000000..bb16953f6290
>> --- /dev/null
>> +++ b/drivers/mux/Makefile
>> @@ -0,0 +1,6 @@
>> +#
>> +# Makefile for multiplexer devices.
>> +#
>> +
>> +obj-$(CONFIG_MULTIPLEXER) += mux-core.o
>> +obj-$(CONFIG_MUX_GPIO) += mux-gpio.o
>> diff --git a/drivers/mux/mux-core.c b/drivers/mux/mux-core.c
>> new file mode 100644
>> index 000000000000..66a8bccfc3d7
>> --- /dev/null
>> +++ b/drivers/mux/mux-core.c
>> @@ -0,0 +1,422 @@
>> +/*
>> + * Multiplexer subsystem
>> + *
>> + * Copyright (C) 2017 Axentia Technologies AB
>> + *
>> + * Author: Peter Rosin <peda@axentia.se>
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License version 2 as
>> + * published by the Free Software Foundation.
>> + */
>> +
>> +#define pr_fmt(fmt) "mux-core: " fmt
>> +
>> +#include <linux/device.h>
>> +#include <linux/err.h>
>> +#include <linux/export.h>
>> +#include <linux/idr.h>
>> +#include <linux/init.h>
>> +#include <linux/module.h>
>> +#include <linux/mux.h>
>> +#include <linux/of.h>
>> +#include <linux/of_platform.h>
>> +#include <linux/slab.h>
>> +
>> +/*
>> + * The idle-as-is "state" is not an actual state that may be selected, it
>> + * only implies that the state should not be changed. So, use that state
>> + * as indication that the cached state of the multiplexer is unknown.
>> + */
>> +#define MUX_CACHE_UNKNOWN MUX_IDLE_AS_IS
>> +
>> +static struct class mux_class = {
>> + .name = "mux",
>> + .owner = THIS_MODULE,
>> +};
>
> No Documentation/ABI/ update for your sysfs files? Please do so.
Ok I'll look into it. Wasn't even aware that I added any. But there's the
new class of course...
>> +
>> +static int __init mux_init(void)
>> +{
>> + return class_register(&mux_class);
>> +}
>> +
>> +static DEFINE_IDA(mux_ida);
>
> When your module is unloaded, you forgot to clean this structure up with
> what was done with it.
I was under the impression that not providing an exit function for modules
made the module infrastructure prevent unloading (by bumping some reference
counter). Maybe that is a misconception?
>> +
>> +static void mux_chip_release(struct device *dev)
>> +{
>> + struct mux_chip *mux_chip = to_mux_chip(dev);
>> +
>> + ida_simple_remove(&mux_ida, mux_chip->id);
>> + kfree(mux_chip);
>> +}
>> +
>> +static struct device_type mux_type = {
>> + .name = "mux-chip",
>> + .release = mux_chip_release,
>> +};
>> +
>> +struct mux_chip *mux_chip_alloc(struct device *dev,
>> + unsigned int controllers, size_t sizeof_priv)
>> +{
>> + struct mux_chip *mux_chip;
>> + int i;
>> +
>> + if (WARN_ON(!dev || !controllers))
>> + return NULL;
>> +
>> + mux_chip = kzalloc(sizeof(*mux_chip) +
>> + controllers * sizeof(*mux_chip->mux) +
>> + sizeof_priv, GFP_KERNEL);
>> + if (!mux_chip)
>> + return NULL;
>
> You don't return PTR_ERR(-ENOMEM)? Ok, why not? (I'm not arguing for
> it, just curious...)
There's no particular reason. Do you think I should change it?
>> +
>> + mux_chip->mux = (struct mux_control *)(mux_chip + 1);
>> + mux_chip->dev.class = &mux_class;
>> + mux_chip->dev.type = &mux_type;
>> + mux_chip->dev.parent = dev;
>> + mux_chip->dev.of_node = dev->of_node;
>> + dev_set_drvdata(&mux_chip->dev, mux_chip);
>> +
>> + mux_chip->id = ida_simple_get(&mux_ida, 0, 0, GFP_KERNEL);
>> + if (mux_chip->id < 0) {
>> + pr_err("muxchipX failed to get a device id\n");
>> + kfree(mux_chip);
>> + return NULL;
>> + }
>> + dev_set_name(&mux_chip->dev, "muxchip%d", mux_chip->id);
>> +
>> + mux_chip->controllers = controllers;
>> + for (i = 0; i < controllers; ++i) {
>> + struct mux_control *mux = &mux_chip->mux[i];
>> +
>> + mux->chip = mux_chip;
>> + init_rwsem(&mux->lock);
>> + mux->cached_state = MUX_CACHE_UNKNOWN;
>> + mux->idle_state = MUX_IDLE_AS_IS;
>> + }
>> +
>> + device_initialize(&mux_chip->dev);
>
> Why are you not registering the device here as well? Why have this be a
> two step process?
Because of idle state handling. The drivers are expected to fill in
the desired idle state(s) after allocating the mux controller(s).
Then, when registering, the desired idle state is activated (if the
idle state is not idle-as-is, of course) and as a last step the mux
is "advertised".
>> +
>> + return mux_chip;
>> +}
>> +EXPORT_SYMBOL_GPL(mux_chip_alloc);
>> +
>> +static int mux_control_set(struct mux_control *mux, int state)
>> +{
>> + int ret = mux->chip->ops->set(mux, state);
>> +
>> + mux->cached_state = ret < 0 ? MUX_CACHE_UNKNOWN : state;
>> +
>> + return ret;
>> +}
>> +
>> +int mux_chip_register(struct mux_chip *mux_chip)
>> +{
>> + int i;
>> + int ret;
>> +
>> + for (i = 0; i < mux_chip->controllers; ++i) {
>> + struct mux_control *mux = &mux_chip->mux[i];
>> +
>> + if (mux->idle_state == mux->cached_state)
>> + continue;
>> +
>> + ret = mux_control_set(mux, mux->idle_state);
>> + if (ret < 0) {
>> + dev_err(&mux_chip->dev, "unable to set idle state\n");
>> + return ret;
>> + }
>> + }
>> +
>> + ret = device_add(&mux_chip->dev);
>> + if (ret < 0)
>> + dev_err(&mux_chip->dev,
>> + "device_add failed in mux_chip_register: %d\n", ret);
>
> Did you run checkpatch.pl in strict mode on this new file? Please do so :)
I did, and did it again just to be sure, and I do not get any complaints.
So, what's wrong?
$ scripts/checkpatch.pl --strict mux-13/0003-mux-minimal-mux-subsystem-and-gpio-based-mux-control.patch
total: 0 errors, 0 warnings, 0 checks, 860 lines checked
mux-13/0003-mux-minimal-mux-subsystem-and-gpio-based-mux-control.patch has no obvious style problems and is ready for submission.
The chackpatch warnings I am aware of are three instances of (for the
'prop', 's' and 'i' arguments):
mux-13/0006-iio-multiplexer-new-iio-category-and-iio-mux-driver.patch
---------------------------------------------------------------------
CHECK: Macro argument reuse 'prop' - possible side-effects?
#433: FILE: drivers/iio/multiplexer/iio-mux.c:326:
+#define of_property_for_each_string_index(np, propname, prop, s, i) \
+ for (prop = of_find_property(np, propname, NULL), \
+ s = of_prop_next_string(prop, NULL), \
+ i = 0; \
+ s; \
+ s = of_prop_next_string(prop, s), \
+ i++)
But those kinds of warnings are also present in the code I plagiarized,
so I don't feel too bad...
And then there are a couple of false positives about files added w/o
adding an entry to MAINTAINERS (the files are covers by wildcards).
>> + return ret;
>> +}
>> +EXPORT_SYMBOL_GPL(mux_chip_register);
>> +
>> +void mux_chip_unregister(struct mux_chip *mux_chip)
>> +{
>> + device_del(&mux_chip->dev);
>> +}
>> +EXPORT_SYMBOL_GPL(mux_chip_unregister);
>> +
>> +void mux_chip_free(struct mux_chip *mux_chip)
>> +{
>> + if (!mux_chip)
>> + return;
>> +
>> + put_device(&mux_chip->dev);
>> +}
>> +EXPORT_SYMBOL_GPL(mux_chip_free);
>> +
>> +static void devm_mux_chip_release(struct device *dev, void *res)
>> +{
>> + struct mux_chip *mux_chip = *(struct mux_chip **)res;
>> +
>> + mux_chip_free(mux_chip);
>> +}
>> +
>> +struct mux_chip *devm_mux_chip_alloc(struct device *dev,
>> + unsigned int controllers,
>> + size_t sizeof_priv)
>> +{
>> + struct mux_chip **ptr, *mux_chip;
>> +
>> + ptr = devres_alloc(devm_mux_chip_release, sizeof(*ptr), GFP_KERNEL);
>> + if (!ptr)
>> + return NULL;
>> +
>> + mux_chip = mux_chip_alloc(dev, controllers, sizeof_priv);
>> + if (!mux_chip) {
>> + devres_free(ptr);
>> + return NULL;
>> + }
>> +
>> + *ptr = mux_chip;
>> + devres_add(dev, ptr);
>> +
>> + return mux_chip;
>> +}
>> +EXPORT_SYMBOL_GPL(devm_mux_chip_alloc);
>
>
> Having devm functions that create/destroy other struct devices worries
> me, do we have other examples of this happening today? Are you sure you
> got the reference counting all correct?
drivers/iio/industrialio_core.c:devm_iio_device_alloc
Or is the iio case different in some subtle way that I'm missing?
>> +
>> +static int devm_mux_chip_match(struct device *dev, void *res, void *data)
>> +{
>> + struct mux_chip **r = res;
>> +
>> + if (WARN_ON(!r || !*r))
>
> How can this happen?
It shouldn't. I copied the pattern from the iio subsystem.
>> + return 0;
>> +
>> + return *r == data;
>> +}
>> +
>> +void devm_mux_chip_free(struct device *dev, struct mux_chip *mux_chip)
>> +{
>> + WARN_ON(devres_release(dev, devm_mux_chip_release,
>> + devm_mux_chip_match, mux_chip));
>
> What can someone do with these WARN_ON() splats in the kernel log?
Don't know. Again, I copied the pattern from the iio subsystem.
>
>> +}
>> +EXPORT_SYMBOL_GPL(devm_mux_chip_free);
>> +
>> +static void devm_mux_chip_reg_release(struct device *dev, void *res)
>> +{
>> + struct mux_chip *mux_chip = *(struct mux_chip **)res;
>> +
>> + mux_chip_unregister(mux_chip);
>> +}
>> +
>> +int devm_mux_chip_register(struct device *dev,
>> + struct mux_chip *mux_chip)
>> +{
>> + struct mux_chip **ptr;
>> + int res;
>> +
>> + ptr = devres_alloc(devm_mux_chip_reg_release, sizeof(*ptr), GFP_KERNEL);
>> + if (!ptr)
>> + return -ENOMEM;
>> +
>> + res = mux_chip_register(mux_chip);
>> + if (res) {
>> + devres_free(ptr);
>> + return res;
>> + }
>> +
>> + *ptr = mux_chip;
>> + devres_add(dev, ptr);
>> +
>> + return res;
>> +}
>> +EXPORT_SYMBOL_GPL(devm_mux_chip_register);
>> +
>> +void devm_mux_chip_unregister(struct device *dev, struct mux_chip *mux_chip)
>> +{
>> + WARN_ON(devres_release(dev, devm_mux_chip_reg_release,
>> + devm_mux_chip_match, mux_chip));
>> +}
>> +EXPORT_SYMBOL_GPL(devm_mux_chip_unregister);
>> +
>> +int mux_control_select(struct mux_control *mux, int state)
>> +{
>> + int ret;
>> +
>> + if (down_read_trylock(&mux->lock)) {
>> + if (mux->cached_state == state)
>> + return 0;
>> +
>> + /* Sigh, the mux needs updating... */
>> + up_read(&mux->lock);
>> + }
>> +
>> + /* ...or it's just contended. */
>> + down_write(&mux->lock);
>
> Why use a read/write lock at all? Have you tested this to verify it
> really is faster and needed?
For one of the HW configuration that drove the development, the same mux
controller is used to mux both an I2C channel and a couple of ADC lines.
If there is no kind of reader/writer locking going on, there is no way to
do ADC readings concurrently with an I2C transfer even when the consumers
want the mux in the same position. With an ordinary mutex controlling the
mux position, the consumers will unconditionally get serialized, which
seems like a waste to me. Or maybe I'm missing something?
>> +
>> + if (mux->cached_state == state) {
>> + /*
>> + * Hmmm, someone else changed the mux to my liking.
>> + * That makes me wonder how long I waited for nothing?
>> + */
>> + downgrade_write(&mux->lock);
>
> Oh that always scares me... Are you _sure_ this is correct? And
> needed?
It might not be needed, and it would probably work ok to just fall
through and call mux_control_set unconditionally. What is it that
always scares you exactly? Relying on cached state to be correct?
Downgrading writer locks?
>> + return 0;
>> + }
>> +
>> + ret = mux_control_set(mux, state);
>> + if (ret < 0) {
>> + if (mux->idle_state != MUX_IDLE_AS_IS)
>> + mux_control_set(mux, mux->idle_state);
>> +
>> + up_write(&mux->lock);
>> + return ret;
>> + }
>> +
>> + downgrade_write(&mux->lock);
>> +
>> + return 1;
>> +}
>> +EXPORT_SYMBOL_GPL(mux_control_select);
>> +
>> +int mux_control_deselect(struct mux_control *mux)
>> +{
>> + int ret = 0;
>> +
>> + if (mux->idle_state != MUX_IDLE_AS_IS &&
>> + mux->idle_state != mux->cached_state)
>> + ret = mux_control_set(mux, mux->idle_state);
>> +
>> + up_read(&mux->lock);
>
> You require a lock to be held for a "global" function? Without
> documentation? Or even a sparse marking? That's asking for trouble...
Documentation I can handle, but where should I look to understand how I
should add sparse markings?
The mux needs to be locked somehow. But as I stated in the cover letter
the rwsem isn't a perfect fit.
I'm using an rwsem to lock a mux, but that isn't really a
perfect fit. Is there a better locking primitive that I don't
know about that fits better? I had a mutex at one point, but
that didn't allow any concurrent accesses at all. At least
the rwsem allows concurrent access as long as all users
agree on the mux state, but I suspect that the rwsem will
degrade to the mutex situation pretty quickly if there is
any contention.
Also, the lock doesn't add anything if there is only one consumer of
a mux controller. Maybe there should be some mechanism for shortcutting
the locking for the (more common?) single-consumer case?
But again, I need the locking for my multi-consumer use case.
>> +
>> + return ret;
>> +}
>> +EXPORT_SYMBOL_GPL(mux_control_deselect);
>> +
>> +static int of_dev_node_match(struct device *dev, const void *data)
>> +{
>> + return dev->of_node == data;
>> +}
>> +
>> +static struct mux_chip *of_find_mux_chip_by_node(struct device_node *np)
>> +{
>> + struct device *dev;
>> +
>> + dev = class_find_device(&mux_class, NULL, np, of_dev_node_match);
>> +
>> + return dev ? to_mux_chip(dev) : NULL;
>> +}
>> +
>> +struct mux_control *mux_control_get(struct device *dev, const char *mux_name)
>> +{
>> + struct device_node *np = dev->of_node;
>> + struct of_phandle_args args;
>> + struct mux_chip *mux_chip;
>> + unsigned int controller;
>> + int index = 0;
>> + int ret;
>> +
>> + if (mux_name) {
>> + index = of_property_match_string(np, "mux-control-names",
>> + mux_name);
>> + if (index < 0) {
>> + dev_err(dev, "mux controller '%s' not found\n",
>> + mux_name);
>> + return ERR_PTR(index);
>> + }
>> + }
>> +
>> + ret = of_parse_phandle_with_args(np,
>> + "mux-controls", "#mux-control-cells",
>> + index, &args);
>> + if (ret) {
>> + dev_err(dev, "%s: failed to get mux-control %s(%i)\n",
>> + np->full_name, mux_name ?: "", index);
>> + return ERR_PTR(ret);
>> + }
>> +
>> + mux_chip = of_find_mux_chip_by_node(args.np);
>> + of_node_put(args.np);
>> + if (!mux_chip)
>> + return ERR_PTR(-EPROBE_DEFER);
>> +
>> + if (args.args_count > 1 ||
>> + (!args.args_count && (mux_chip->controllers > 1))) {
>> + dev_err(dev, "%s: wrong #mux-control-cells for %s\n",
>> + np->full_name, args.np->full_name);
>> + return ERR_PTR(-EINVAL);
>> + }
>> +
>> + controller = 0;
>> + if (args.args_count)
>> + controller = args.args[0];
>> +
>> + if (controller >= mux_chip->controllers) {
>> + dev_err(dev, "%s: bad mux controller %u specified in %s\n",
>> + np->full_name, controller, args.np->full_name);
>> + return ERR_PTR(-EINVAL);
>> + }
>> +
>> + get_device(&mux_chip->dev);
>> + return &mux_chip->mux[controller];
>> +}
>> +EXPORT_SYMBOL_GPL(mux_control_get);
>> +
>> +void mux_control_put(struct mux_control *mux)
>> +{
>> + put_device(&mux->chip->dev);
>> +}
>> +EXPORT_SYMBOL_GPL(mux_control_put);
>> +
>> +static void devm_mux_control_release(struct device *dev, void *res)
>> +{
>> + struct mux_control *mux = *(struct mux_control **)res;
>> +
>> + mux_control_put(mux);
>> +}
>> +
>> +struct mux_control *devm_mux_control_get(struct device *dev,
>> + const char *mux_name)
>> +{
>> + struct mux_control **ptr, *mux;
>> +
>> + ptr = devres_alloc(devm_mux_control_release, sizeof(*ptr), GFP_KERNEL);
>> + if (!ptr)
>> + return ERR_PTR(-ENOMEM);
>> +
>> + mux = mux_control_get(dev, mux_name);
>> + if (IS_ERR(mux)) {
>> + devres_free(ptr);
>> + return mux;
>> + }
>> +
>> + *ptr = mux;
>> + devres_add(dev, ptr);
>> +
>> + return mux;
>> +}
>> +EXPORT_SYMBOL_GPL(devm_mux_control_get);
>> +
>> +static int devm_mux_control_match(struct device *dev, void *res, void *data)
>> +{
>> + struct mux_control **r = res;
>> +
>> + if (WARN_ON(!r || !*r))
>> + return 0;
>
> Same here, how can this happen?
Same response as above.
>> +
>> + return *r == data;
>> +}
>> +
>> +void devm_mux_control_put(struct device *dev, struct mux_control *mux)
>> +{
>> + WARN_ON(devres_release(dev, devm_mux_control_release,
>> + devm_mux_control_match, mux));
>> +}
>> +EXPORT_SYMBOL_GPL(devm_mux_control_put);
>> +
>> +/*
>> + * Using subsys_initcall instead of module_init here to ensure - for the
>> + * non-modular case - that the subsystem is initialized when mux consumers
>> + * and mux controllers start to use it /without/ relying on link order.
>> + * For the modular case, the ordering is ensured with module dependencies.
>> + */
>> +subsys_initcall(mux_init);
>
> Even with subsys_initcall you are relying on link order, you do realize
> that? What about other subsystems that rely on this? :)
Yes, that is true, but if others start relying on this, that's their problem,
right? :-)
>
>> +
>> +MODULE_DESCRIPTION("Multiplexer subsystem");
>> +MODULE_AUTHOR("Peter Rosin <peda@axentia.se>");
>> +MODULE_LICENSE("GPL v2");
>> diff --git a/drivers/mux/mux-gpio.c b/drivers/mux/mux-gpio.c
>> new file mode 100644
>> index 000000000000..227d3572e6db
>> --- /dev/null
>> +++ b/drivers/mux/mux-gpio.c
>> @@ -0,0 +1,114 @@
>> +/*
>> + * GPIO-controlled multiplexer driver
>> + *
>> + * Copyright (C) 2017 Axentia Technologies AB
>> + *
>> + * Author: Peter Rosin <peda@axentia.se>
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License version 2 as
>> + * published by the Free Software Foundation.
>> + */
>> +
>> +#include <linux/err.h>
>> +#include <linux/gpio/consumer.h>
>> +#include <linux/module.h>
>> +#include <linux/mux.h>
>> +#include <linux/of_platform.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/property.h>
>> +
>> +struct mux_gpio {
>> + struct gpio_descs *gpios;
>> + int *val;
>> +};
>> +
>> +static int mux_gpio_set(struct mux_control *mux, int state)
>> +{
>> + struct mux_gpio *mux_gpio = mux_chip_priv(mux->chip);
>> + int i;
>> +
>> + for (i = 0; i < mux_gpio->gpios->ndescs; i++)
>> + mux_gpio->val[i] = (state >> i) & 1;
>> +
>> + gpiod_set_array_value_cansleep(mux_gpio->gpios->ndescs,
>> + mux_gpio->gpios->desc,
>> + mux_gpio->val);
>> +
>> + return 0;
>> +}
>> +
>> +static const struct mux_control_ops mux_gpio_ops = {
>> + .set = mux_gpio_set,
>> +};
>> +
>> +static const struct of_device_id mux_gpio_dt_ids[] = {
>> + { .compatible = "gpio-mux", },
>> + { /* sentinel */ }
>> +};
>> +MODULE_DEVICE_TABLE(of, mux_gpio_dt_ids);
>> +
>> +static int mux_gpio_probe(struct platform_device *pdev)
>> +{
>> + struct device *dev = &pdev->dev;
>> + struct mux_chip *mux_chip;
>> + struct mux_gpio *mux_gpio;
>> + int pins;
>> + s32 idle_state;
>> + int ret;
>> +
>> + pins = gpiod_count(dev, "mux");
>> + if (pins < 0)
>> + return pins;
>> +
>> + mux_chip = devm_mux_chip_alloc(dev, 1, sizeof(*mux_gpio) +
>> + pins * sizeof(*mux_gpio->val));
>> + if (!mux_chip)
>> + return -ENOMEM;
>> +
>> + mux_gpio = mux_chip_priv(mux_chip);
>> + mux_gpio->val = (int *)(mux_gpio + 1);
>> + mux_chip->ops = &mux_gpio_ops;
>> +
>> + mux_gpio->gpios = devm_gpiod_get_array(dev, "mux", GPIOD_OUT_LOW);
>> + if (IS_ERR(mux_gpio->gpios)) {
>> + ret = PTR_ERR(mux_gpio->gpios);
>> + if (ret != -EPROBE_DEFER)
>> + dev_err(dev, "failed to get gpios\n");
>> + return ret;
>> + }
>> + WARN_ON(pins != mux_gpio->gpios->ndescs);
>> + mux_chip->mux->states = 1 << pins;
>> +
>> + ret = device_property_read_u32(dev, "idle-state", (u32 *)&idle_state);
>> + if (ret >= 0 && idle_state != MUX_IDLE_AS_IS) {
>> + if (idle_state < 0 || idle_state >= mux_chip->mux->states) {
>> + dev_err(dev, "invalid idle-state %u\n", idle_state);
>> + return -EINVAL;
>> + }
>> +
>> + mux_chip->mux->idle_state = idle_state;
>> + }
>> +
>> + ret = devm_mux_chip_register(dev, mux_chip);
>> + if (ret < 0)
>> + return ret;
>> +
>> + dev_info(dev, "%u-way mux-controller registered\n",
>> + mux_chip->mux->states);
>> +
>> + return 0;
>> +}
>> +
>> +static struct platform_driver mux_gpio_driver = {
>> + .driver = {
>> + .name = "gpio-mux",
>> + .of_match_table = of_match_ptr(mux_gpio_dt_ids),
>> + },
>> + .probe = mux_gpio_probe,
>> +};
>> +module_platform_driver(mux_gpio_driver);
>> +
>> +MODULE_DESCRIPTION("GPIO-controlled multiplexer driver");
>> +MODULE_AUTHOR("Peter Rosin <peda@axentia.se>");
>> +MODULE_LICENSE("GPL v2");
>> diff --git a/include/linux/mux.h b/include/linux/mux.h
>> new file mode 100644
>> index 000000000000..febdde4246df
>> --- /dev/null
>> +++ b/include/linux/mux.h
>> @@ -0,0 +1,252 @@
>> +/*
>> + * mux.h - definitions for the multiplexer interface
>> + *
>> + * Copyright (C) 2017 Axentia Technologies AB
>> + *
>> + * Author: Peter Rosin <peda@axentia.se>
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License version 2 as
>> + * published by the Free Software Foundation.
>> + */
>> +
>> +#ifndef _LINUX_MUX_H
>> +#define _LINUX_MUX_H
>> +
>> +#include <linux/device.h>
>> +#include <linux/rwsem.h>
>> +
>> +struct mux_chip;
>> +struct mux_control;
>> +struct platform_device;
>> +
>> +/**
>> + * struct mux_control_ops - Mux controller operations for a mux chip.
>> + * @set: Set the state of the given mux controller.
>> + */
>> +struct mux_control_ops {
>> + int (*set)(struct mux_control *mux, int state);
>> +};
>> +
>> +/* These defines match the constants from the dt-bindings. On purpose. */
>
> Why on purpose?
I sure wasn't an accident? :-)
Want me to remove it?
>> +#define MUX_IDLE_AS_IS (-1)
>> +#define MUX_IDLE_DISCONNECT (-2)
>> +
>> +/**
>> + * struct mux_control - Represents a mux controller.
>> + * @lock: Protects the mux controller state.
>> + * @chip: The mux chip that is handling this mux controller.
>> + * @states: The number of mux controller states.
>> + * @cached_state: The current mux controller state, or -1 if none.
>> + * @idle_state: The mux controller state to use when inactive, or one
>> + * of MUX_IDLE_AS_IS and MUX_IDLE_DISCONNECT.
>> + */
>> +struct mux_control {
>> + struct rw_semaphore lock; /* protects the state of the mux */
>> +
>> + struct mux_chip *chip;
>> +
>> + unsigned int states;
>> + int cached_state;
>> + int idle_state;
>> +};
>> +
>> +/**
>> + * struct mux_chip - Represents a chip holding mux controllers.
>> + * @controllers: Number of mux controllers handled by the chip.
>> + * @mux: Array of mux controllers that are handled.
>> + * @dev: Device structure.
>> + * @id: Used to identify the device internally.
>> + * @ops: Mux controller operations.
>> + */
>> +struct mux_chip {
>> + unsigned int controllers;
>> + struct mux_control *mux;
>> + struct device dev;
>> + int id;
>> +
>> + const struct mux_control_ops *ops;
>> +};
>> +
>> +#define to_mux_chip(x) container_of((x), struct mux_chip, dev)
>> +
>> +/**
>> + * mux_chip_priv() - Get the extra memory reserved by mux_chip_alloc().
>> + * @mux_chip: The mux-chip to get the private memory from.
>> + *
>> + * Return: Pointer to the private memory reserved by the allocator.
>> + */
>> +static inline void *mux_chip_priv(struct mux_chip *mux_chip)
>> +{
>> + return &mux_chip->mux[mux_chip->controllers];
>> +}
>> +
>> +/**
>> + * mux_chip_alloc() - Allocate a mux-chip.
>> + * @dev: The parent device implementing the mux interface.
>> + * @controllers: The number of mux controllers to allocate for this chip.
>> + * @sizeof_priv: Size of extra memory area for private use by the caller.
>> + *
>> + * Return: A pointer to the new mux-chip, NULL on failure.
>> + */
>> +struct mux_chip *mux_chip_alloc(struct device *dev,
>> + unsigned int controllers, size_t sizeof_priv);
>> +
>
> Don't put kernel doc comments in a .h file, they normally go into the .c
> file, next to the code itself. That makes it easier to fix up and
> realise when they need to be changed when the code changes. The .h file
> rarely changes.
I'll move the lot over.
>> +/**
>> + * mux_chip_register() - Register a mux-chip, thus readying the controllers
>> + * for use.
>> + * @mux_chip: The mux-chip to register.
>> + *
>> + * Do not retry registration of the same mux-chip on failure. You should
>> + * instead put it away with mux_chip_free() and allocate a new one, if you
>> + * for some reason would like to retry registration.
>> + *
>> + * Return: Zero on success or a negative errno on error.
>> + */
>> +int mux_chip_register(struct mux_chip *mux_chip);
>> +
>> +/**
>> + * mux_chip_unregister() - Take the mux-chip off-line.
>> + * @mux_chip: The mux-chip to unregister.
>> + *
>> + * mux_chip_unregister() reverses the effects of mux_chip_register().
>> + * But not completely, you should not try to call mux_chip_register()
>> + * on a mux-chip that has been registered before.
>> + */
>> +void mux_chip_unregister(struct mux_chip *mux_chip);
>> +
>> +/**
>> + * mux_chip_free() - Free the mux-chip for good.
>> + * @mux_chip: The mux-chip to free.
>> + *
>> + * mux_chip_free() reverses the effects of mux_chip_alloc().
>> + */
>> +void mux_chip_free(struct mux_chip *mux_chip);
>> +
>> +/**
>> + * devm_mux_chip_alloc() - Resource-managed version of mux_chip_alloc().
>> + * @dev: The parent device implementing the mux interface.
>> + * @controllers: The number of mux controllers to allocate for this chip.
>> + * @sizeof_priv: Size of extra memory area for private use by the caller.
>> + *
>> + * See mux_chip_alloc() for more details.
>> + *
>> + * Return: A pointer to the new mux-chip, NULL on failure.
>> + */
>> +struct mux_chip *devm_mux_chip_alloc(struct device *dev,
>> + unsigned int controllers,
>> + size_t sizeof_priv);
>> +
>> +/**
>> + * devm_mux_chip_register() - Resource-managed version mux_chip_register().
>> + * @dev: The parent device implementing the mux interface.
>> + * @mux_chip: The mux-chip to register.
>> + *
>> + * See mux_chip_register() for more details.
>> + *
>> + * Return: Zero on success or a negative errno on error.
>> + */
>> +int devm_mux_chip_register(struct device *dev, struct mux_chip *mux_chip);
>> +
>> +/**
>> + * devm_mux_chip_unregister() - Resource-managed version mux_chip_unregister().
>> + * @dev: The device that originally registered the mux-chip.
>> + * @mux_chip: The mux-chip to unregister.
>> + *
>> + * See mux_chip_unregister() for more details.
>> + *
>> + * Note that you do not normally need to call this function.
>
> Odd, then why is it exported???
You normally don't call the devm_foo_{free,release,unregister,etc} functions.
The intention is of course that the resourse cleans up automatically. But there
are no cases where the manual clean up is not available, at least not that I can
find?
>> + */
>> +void devm_mux_chip_unregister(struct device *dev, struct mux_chip *mux_chip);
>> +
>> +/**
>> + * devm_mux_chip_free() - Resource-managed version mux_chip_free().
>> + * @dev: The device that originally got the mux-chip.
>> + * @mux_chip: The mux-chip to free.
>> + *
>> + * See mux_chip_free() for more details.
>> + *
>> + * Note that you do not normally need to call this function.
>> + */
>> +void devm_mux_chip_free(struct device *dev, struct mux_chip *mux_chip);
>> +
>> +/**
>> + * mux_control_select() - Select the given multiplexer state.
>> + * @mux: The mux-control to request a change of state from.
>> + * @state: The new requested state.
>> + *
>> + * Make sure to call mux_control_deselect() when the operation is complete and
>> + * the mux-control is free for others to use, but do not call
>> + * mux_control_deselect() if mux_control_select() fails.
>> + *
>> + * Return: 0 if the requested state was already active, or 1 it the
>> + * mux-control state was changed to the requested state. Or a negative
>> + * errno on error.
>> + *
>> + * Note that the difference in return value of zero or one is of
>> + * questionable value; especially if the mux-control has several independent
>> + * consumers, which is something the consumers should perhaps not be making
>> + * assumptions about.
>
> I don't understand this note, what is a user of this api supposed to do
> differently between 1 and 0? Why make the difference at all?
If the consumer somehow *knows* that it is the only user of a mux controller,
it can use the return value to shortcut (perhaps costly) actions only needed
when the mux changes. The 1/0 difference was also a "free" extra given the
current implementation of mux_control_select. But it's cheep for the consumer
to keep track of this by itself if it needs it.
It's when there are several (independent) consumers of a mux controller that
the information in the return value is questionable and can't be used to
shortcut actions only needed on mux changes.
Now that you point the finger at it and I have been made to spell out the
problem, I think it's probably wise to remove the distiction so that users
do not start to use the return value when they shouldn't. It's generally
better to keep track of the expected mux state in the consumer and use that
local information to shortcut those (perhaps costly) actions instead.
> And I agree with the comment to split this up into 2 different .h files,
> if possible.
Will split.
> thanks,
>
> greg k-h
>
I'll wait for further feedback before posting a v14.
Cheers and thanks,
peda
^ permalink raw reply [flat|nested] 38+ messages in thread* Re: [PATCH v13 03/10] mux: minimal mux subsystem and gpio-based mux controller
2017-04-18 10:59 ` Peter Rosin
@ 2017-04-18 11:44 ` Greg Kroah-Hartman
2017-04-18 21:53 ` Peter Rosin
2017-05-05 13:19 ` Peter Rosin
1 sibling, 1 reply; 38+ messages in thread
From: Greg Kroah-Hartman @ 2017-04-18 11:44 UTC (permalink / raw)
To: Peter Rosin
Cc: linux-kernel, Wolfram Sang, Rob Herring, Mark Rutland,
Jonathan Cameron, Hartmut Knaack, Lars-Peter Clausen,
Peter Meerwald-Stadler, Jonathan Corbet, linux-i2c, devicetree,
linux-iio, linux-doc, Andrew Morton, Colin Ian King,
Paul Gortmaker, Philipp Zabel, kernel
On Tue, Apr 18, 2017 at 12:59:50PM +0200, Peter Rosin wrote:
> On 2017-04-18 10:51, Greg Kroah-Hartman wrote:
> > On Thu, Apr 13, 2017 at 06:43:07PM +0200, Peter Rosin wrote:
> >> +config MUX_GPIO
> >> + tristate "GPIO-controlled Multiplexer"
> >> + depends on OF && GPIOLIB
> >
> > Why have the gpio and mux core in the same patch?
>
> One is not usable w/o the other. I can split them if you want to?
Then why are they two different config options? Add the core code in
one patch, and then add the gpio controled mutiplexer in a separate
patch.
> >> +static struct class mux_class = {
> >> + .name = "mux",
> >> + .owner = THIS_MODULE,
> >> +};
> >
> > No Documentation/ABI/ update for your sysfs files? Please do so.
>
> Ok I'll look into it. Wasn't even aware that I added any. But there's the
> new class of course...
Hint, you have files, the devices that belong to the class :)
> >> +static int __init mux_init(void)
> >> +{
> >> + return class_register(&mux_class);
> >> +}
> >> +
> >> +static DEFINE_IDA(mux_ida);
> >
> > When your module is unloaded, you forgot to clean this structure up with
> > what was done with it.
>
> I was under the impression that not providing an exit function for modules
> made the module infrastructure prevent unloading (by bumping some reference
> counter). Maybe that is a misconception?
Ah, messy, don't do that. Make it so you can unload your module please,
why wouldn't you want that to happen?
> >> + mux_chip = kzalloc(sizeof(*mux_chip) +
> >> + controllers * sizeof(*mux_chip->mux) +
> >> + sizeof_priv, GFP_KERNEL);
> >> + if (!mux_chip)
> >> + return NULL;
> >
> > You don't return PTR_ERR(-ENOMEM)? Ok, why not? (I'm not arguing for
> > it, just curious...)
>
> There's no particular reason. Do you think I should change it?
What does the caller do with an error? Pass it up to where? Who gets
it? Don't you want the caller to know you are out of memory?
> >> +
> >> + device_initialize(&mux_chip->dev);
> >
> > Why are you not registering the device here as well? Why have this be a
> > two step process?
>
> Because of idle state handling. The drivers are expected to fill in
> the desired idle state(s) after allocating the mux controller(s).
> Then, when registering, the desired idle state is activated (if the
> idle state is not idle-as-is, of course) and as a last step the mux
> is "advertised".
Ok, is that documented in the functions somewhere?
> >> + ret = device_add(&mux_chip->dev);
> >> + if (ret < 0)
> >> + dev_err(&mux_chip->dev,
> >> + "device_add failed in mux_chip_register: %d\n", ret);
> >
> > Did you run checkpatch.pl in strict mode on this new file? Please do so :)
>
> I did, and did it again just to be sure, and I do not get any complaints.
> So, what's wrong?
You list the function name in the printk string, it should complain
that __func__ should be used. Oh well, it's just a perl script, it
doesn't always catch everything.
isn't always correct :)
> >> +EXPORT_SYMBOL_GPL(devm_mux_chip_alloc);
> >
> >
> > Having devm functions that create/destroy other struct devices worries
> > me, do we have other examples of this happening today? Are you sure you
> > got the reference counting all correct?
>
> drivers/iio/industrialio_core.c:devm_iio_device_alloc
>
> Or is the iio case different in some subtle way that I'm missing?
I don't know, hopefully you got it all correct, I haven't audited that
code path in a long time :)
> >> +
> >> +static int devm_mux_chip_match(struct device *dev, void *res, void *data)
> >> +{
> >> + struct mux_chip **r = res;
> >> +
> >> + if (WARN_ON(!r || !*r))
> >
> > How can this happen?
>
> It shouldn't. I copied the pattern from the iio subsystem.
Then it should be removed there too...
> >> +void devm_mux_chip_free(struct device *dev, struct mux_chip *mux_chip)
> >> +{
> >> + WARN_ON(devres_release(dev, devm_mux_chip_release,
> >> + devm_mux_chip_match, mux_chip));
> >
> > What can someone do with these WARN_ON() splats in the kernel log?
>
> Don't know. Again, I copied the pattern from the iio subsystem.
If you don't know what it should be used for, don't copy it!
Cargo-cult coding is horrible, please no.
> >> + /* ...or it's just contended. */
> >> + down_write(&mux->lock);
> >
> > Why use a read/write lock at all? Have you tested this to verify it
> > really is faster and needed?
>
> For one of the HW configuration that drove the development, the same mux
> controller is used to mux both an I2C channel and a couple of ADC lines.
>
> If there is no kind of reader/writer locking going on, there is no way to
> do ADC readings concurrently with an I2C transfer even when the consumers
> want the mux in the same position. With an ordinary mutex controlling the
> mux position, the consumers will unconditionally get serialized, which
> seems like a waste to me. Or maybe I'm missing something?
Why is serializing things a "waste"? Again, a rw semaphore is slower,
takes more logic to get correct, and is very complex at times. If you
are not SURE you need it, and that it matters, don't use it. And if you
do use it, document the heck out of it how you need it and why.
> >> +
> >> + if (mux->cached_state == state) {
> >> + /*
> >> + * Hmmm, someone else changed the mux to my liking.
> >> + * That makes me wonder how long I waited for nothing?
> >> + */
> >> + downgrade_write(&mux->lock);
> >
> > Oh that always scares me... Are you _sure_ this is correct? And
> > needed?
>
> It might not be needed, and it would probably work ok to just fall
> through and call mux_control_set unconditionally. What is it that
> always scares you exactly? Relying on cached state to be correct?
> Downgrading writer locks?
downgrading a writer lock scares me, especially for something as
"simple" as this type of interface. Again, don't use it unless you
_have_ to. Simple is good, start with that always.
> >> + if (mux->idle_state != MUX_IDLE_AS_IS &&
> >> + mux->idle_state != mux->cached_state)
> >> + ret = mux_control_set(mux, mux->idle_state);
> >> +
> >> + up_read(&mux->lock);
> >
> > You require a lock to be held for a "global" function? Without
> > documentation? Or even a sparse marking? That's asking for trouble...
>
> Documentation I can handle, but where should I look to understand how I
> should add sparse markings?
Run sparse on the code and see what it says :)
> The mux needs to be locked somehow. But as I stated in the cover letter
> the rwsem isn't a perfect fit.
>
> I'm using an rwsem to lock a mux, but that isn't really a
> perfect fit. Is there a better locking primitive that I don't
> know about that fits better? I had a mutex at one point, but
> that didn't allow any concurrent accesses at all. At least
> the rwsem allows concurrent access as long as all users
> agree on the mux state, but I suspect that the rwsem will
> degrade to the mutex situation pretty quickly if there is
> any contention.
>
> Also, the lock doesn't add anything if there is only one consumer of
> a mux controller. Maybe there should be some mechanism for shortcutting
> the locking for the (more common?) single-consumer case?
>
> But again, I need the locking for my multi-consumer use case.
Go back to a mutex, and having a function that requires it to be held
is, icky.
> >> +/*
> >> + * Using subsys_initcall instead of module_init here to ensure - for the
> >> + * non-modular case - that the subsystem is initialized when mux consumers
> >> + * and mux controllers start to use it /without/ relying on link order.
> >> + * For the modular case, the ordering is ensured with module dependencies.
> >> + */
> >> +subsys_initcall(mux_init);
> >
> > Even with subsys_initcall you are relying on link order, you do realize
> > that? What about other subsystems that rely on this? :)
>
> Yes, that is true, but if others start relying on this, that's their problem,
> right? :-)
Yes, but no need to document something that isn't true. You are relying
on link order here.
> >> +struct mux_control_ops {
> >> + int (*set)(struct mux_control *mux, int state);
> >> +};
> >> +
> >> +/* These defines match the constants from the dt-bindings. On purpose. */
> >
> > Why on purpose?
>
> I sure wasn't an accident? :-)
>
> Want me to remove it?
At least explain _why_ you are doing this, that would help, right?
thanks,
greg k-h
^ permalink raw reply [flat|nested] 38+ messages in thread* Re: [PATCH v13 03/10] mux: minimal mux subsystem and gpio-based mux controller
2017-04-18 11:44 ` Greg Kroah-Hartman
@ 2017-04-18 21:53 ` Peter Rosin
2017-04-19 2:23 ` Joe Perches
2017-04-20 21:53 ` Peter Rosin
0 siblings, 2 replies; 38+ messages in thread
From: Peter Rosin @ 2017-04-18 21:53 UTC (permalink / raw)
To: Greg Kroah-Hartman
Cc: linux-kernel, Wolfram Sang, Rob Herring, Mark Rutland,
Jonathan Cameron, Hartmut Knaack, Lars-Peter Clausen,
Peter Meerwald-Stadler, Jonathan Corbet, linux-i2c, devicetree,
linux-iio, linux-doc, Andrew Morton, Colin Ian King,
Paul Gortmaker, Philipp Zabel, kernel
On 2017-04-18 13:44, Greg Kroah-Hartman wrote:
> On Tue, Apr 18, 2017 at 12:59:50PM +0200, Peter Rosin wrote:
>> On 2017-04-18 10:51, Greg Kroah-Hartman wrote:
>>> On Thu, Apr 13, 2017 at 06:43:07PM +0200, Peter Rosin wrote:
>>>> +config MUX_GPIO
>>>> + tristate "GPIO-controlled Multiplexer"
>>>> + depends on OF && GPIOLIB
>>>
>>> Why have the gpio and mux core in the same patch?
>>
>> One is not usable w/o the other. I can split them if you want to?
>
> Then why are they two different config options? Add the core code in
> one patch, and then add the gpio controled mutiplexer in a separate
> patch.
Ah, I meant when there are not yet any other mux drivers. I'll just
split the patch.
>>>> +static struct class mux_class = {
>>>> + .name = "mux",
>>>> + .owner = THIS_MODULE,
>>>> +};
>>>
>>> No Documentation/ABI/ update for your sysfs files? Please do so.
>>
>> Ok I'll look into it. Wasn't even aware that I added any. But there's the
>> new class of course...
>
> Hint, you have files, the devices that belong to the class :)
Right.
>>>> +static int __init mux_init(void)
>>>> +{
>>>> + return class_register(&mux_class);
>>>> +}
>>>> +
>>>> +static DEFINE_IDA(mux_ida);
>>>
>>> When your module is unloaded, you forgot to clean this structure up with
>>> what was done with it.
>>
>> I was under the impression that not providing an exit function for modules
>> made the module infrastructure prevent unloading (by bumping some reference
>> counter). Maybe that is a misconception?
>
> Ah, messy, don't do that. Make it so you can unload your module please,
> why wouldn't you want that to happen?
What made me do it was the worry that mux drivers might be left dangling
w/o the core. But that can probably only happen if someone is very
deliberately trying to break stuff by forcing unloads??
>>>> + mux_chip = kzalloc(sizeof(*mux_chip) +
>>>> + controllers * sizeof(*mux_chip->mux) +
>>>> + sizeof_priv, GFP_KERNEL);
>>>> + if (!mux_chip)
>>>> + return NULL;
>>>
>>> You don't return PTR_ERR(-ENOMEM)? Ok, why not? (I'm not arguing for
>>> it, just curious...)
>>
>> There's no particular reason. Do you think I should change it?
>
> What does the caller do with an error? Pass it up to where? Who gets
> it? Don't you want the caller to know you are out of memory?
The current callers return -ENOMEM when NULL is returned here. Looks
like I'm going to be doing some fairly major changes anyway so I'll
just change this too while at it...
>>>> +
>>>> + device_initialize(&mux_chip->dev);
>>>
>>> Why are you not registering the device here as well? Why have this be a
>>> two step process?
>>
>> Because of idle state handling. The drivers are expected to fill in
>> the desired idle state(s) after allocating the mux controller(s).
>> Then, when registering, the desired idle state is activated (if the
>> idle state is not idle-as-is, of course) and as a last step the mux
>> is "advertised".
>
> Ok, is that documented in the functions somewhere?
I'll make sure to add it if it's missing.
>>>> + ret = device_add(&mux_chip->dev);
>>>> + if (ret < 0)
>>>> + dev_err(&mux_chip->dev,
>>>> + "device_add failed in mux_chip_register: %d\n", ret);
>>>
>>> Did you run checkpatch.pl in strict mode on this new file? Please do so :)
>>
>> I did, and did it again just to be sure, and I do not get any complaints.
>> So, what's wrong?
>
> You list the function name in the printk string, it should complain
> that __func__ should be used. Oh well, it's just a perl script, it
> doesn't always catch everything.
> isn't always correct :)
Ah, ok.
>>>> +EXPORT_SYMBOL_GPL(devm_mux_chip_alloc);
>>>
>>>
>>> Having devm functions that create/destroy other struct devices worries
>>> me, do we have other examples of this happening today? Are you sure you
>>> got the reference counting all correct?
>>
>> drivers/iio/industrialio_core.c:devm_iio_device_alloc
>>
>> Or is the iio case different in some subtle way that I'm missing?
>
> I don't know, hopefully you got it all correct, I haven't audited that
> code path in a long time :)
Looks as ok to me as it did before. Moving on... :-)
>>>> +
>>>> +static int devm_mux_chip_match(struct device *dev, void *res, void *data)
>>>> +{
>>>> + struct mux_chip **r = res;
>>>> +
>>>> + if (WARN_ON(!r || !*r))
>>>
>>> How can this happen?
>>
>> It shouldn't. I copied the pattern from the iio subsystem.
>
> Then it should be removed there too...
Ok, I'll see if I can find time to suggest some patch(es) to Jonathan.
>>>> +void devm_mux_chip_free(struct device *dev, struct mux_chip *mux_chip)
>>>> +{
>>>> + WARN_ON(devres_release(dev, devm_mux_chip_release,
>>>> + devm_mux_chip_match, mux_chip));
>>>
>>> What can someone do with these WARN_ON() splats in the kernel log?
>>
>> Don't know. Again, I copied the pattern from the iio subsystem.
>
> If you don't know what it should be used for, don't copy it!
>
> Cargo-cult coding is horrible, please no.
Ok, I'll just drop those WARNs...
>>>> + /* ...or it's just contended. */
>>>> + down_write(&mux->lock);
>>>
>>> Why use a read/write lock at all? Have you tested this to verify it
>>> really is faster and needed?
>>
>> For one of the HW configuration that drove the development, the same mux
>> controller is used to mux both an I2C channel and a couple of ADC lines.
>>
>> If there is no kind of reader/writer locking going on, there is no way to
>> do ADC readings concurrently with an I2C transfer even when the consumers
>> want the mux in the same position. With an ordinary mutex controlling the
>> mux position, the consumers will unconditionally get serialized, which
>> seems like a waste to me. Or maybe I'm missing something?
>
> Why is serializing things a "waste"? Again, a rw semaphore is slower,
> takes more logic to get correct, and is very complex at times. If you
> are not SURE you need it, and that it matters, don't use it. And if you
> do use it, document the heck out of it how you need it and why.
It's a waste of time because two independent mux consumers of the same
mux controller can't do things concurrently even if they want the same
thing from the mux. Let's say that one mux consumer is an iio-mux and
one is an i2c-mux. Also, let's say that you for some reason need to get
a lot of samples at a determined rate through the iio-mux. With a mutex,
that can't happen if there is an access through the i2c-mux taking
"forever" in the eyes of the ADC/iio-mux, even if they both want the
shared mux controller to be in the same position. The ADC/iio-mux and
the i2c-mux would be waiting for each other for no purpose at all.
>>>> +
>>>> + if (mux->cached_state == state) {
>>>> + /*
>>>> + * Hmmm, someone else changed the mux to my liking.
>>>> + * That makes me wonder how long I waited for nothing?
>>>> + */
>>>> + downgrade_write(&mux->lock);
>>>
>>> Oh that always scares me... Are you _sure_ this is correct? And
>>> needed?
>>
>> It might not be needed, and it would probably work ok to just fall
>> through and call mux_control_set unconditionally. What is it that
>> always scares you exactly? Relying on cached state to be correct?
>> Downgrading writer locks?
>
> downgrading a writer lock scares me, especially for something as
> "simple" as this type of interface. Again, don't use it unless you
> _have_ to. Simple is good, start with that always.
Some kind of lock needs to be grabbed in mux_control_select and
released in mux_control_deselect that fixes the mux state while the
mux consumer goes about its business. For the reasons stated above I
went with a reader/writer lock instead of the mutex I had originally.
Agreed, the code in mux_control_select is a few more lines than I
like, but I suspected the big issue to be holding *any* lock over
a pair of "global" functions. Changing from holding a rw-lock as
reader to instead holding a mutex changes very little in my eyes.
mux_control_select is simply not *that* complicated...
>>>> + if (mux->idle_state != MUX_IDLE_AS_IS &&
>>>> + mux->idle_state != mux->cached_state)
>>>> + ret = mux_control_set(mux, mux->idle_state);
>>>> +
>>>> + up_read(&mux->lock);
>>>
>>> You require a lock to be held for a "global" function? Without
>>> documentation? Or even a sparse marking? That's asking for trouble...
>>
>> Documentation I can handle, but where should I look to understand how I
>> should add sparse markings?
>
> Run sparse on the code and see what it says :)
Will do.
>> The mux needs to be locked somehow. But as I stated in the cover letter
>> the rwsem isn't a perfect fit.
>>
>> I'm using an rwsem to lock a mux, but that isn't really a
>> perfect fit. Is there a better locking primitive that I don't
>> know about that fits better? I had a mutex at one point, but
>> that didn't allow any concurrent accesses at all. At least
>> the rwsem allows concurrent access as long as all users
>> agree on the mux state, but I suspect that the rwsem will
>> degrade to the mutex situation pretty quickly if there is
>> any contention.
>>
>> Also, the lock doesn't add anything if there is only one consumer of
>> a mux controller. Maybe there should be some mechanism for shortcutting
>> the locking for the (more common?) single-consumer case?
>>
>> But again, I need the locking for my multi-consumer use case.
>
> Go back to a mutex, and having a function that requires it to be held
> is, icky.
But how do you propose that the ickyness is avoided? It's a requirement
that any waiters are released when the mux is available...
*thinking/coding for a bit*
I'm going to experiment with the below (untested) code which AFAICT
has the issues that starvation is possible, that it isn't first-come
first-serve, and that once there is contention the waiters may wait
for a longer time than needed...
On the positive side there are no actual locks held from select over
to deselect and there's no rwsem. But a missing (or an extra) deselect
still messes things up pretty fatally, since the below is really just
some kind of open coded locking thingy that I thought was one of the
things I should stay away from. But maybe you like it better?
(In the below, mux->wait_sem is assumed to be initialized to zero.)
int mux_control_select(struct mux_control *mux, int state)
{
int ret = 0;
again:
mutex_lock(&mux->lock);
if (mux->cached_state == state) {
/* The mux is already correct, just bump the user count. */
++mux->users;
goto done;
}
if (mux->users) {
/* The mux needs updating but is in use, wait... */
++mux->waiters;
mutex_unlock(&mux->lock);
down(&mux->wait_sem);
goto again;
}
/* The mux needs updating and is unused. */
ret = mux_control_set(mux, state);
if (ret >= 0) {
++mux->users;
goto done;
}
/* The mux update failed, try to revert if appropriate... */
if (mux->idle_state != MUX_IDLE_AS_IS)
mux_control_set(mux, mux->idle_state);
/* ...and release a waiter if there is one. */
if (mux->waiters) {
--mux->waiters;
up(&mux->wait_sem);
}
done:
mutex_unlock(&mux->lock);
return ret;
}
int mux_control_deselect(struct mux_control *mux)
{
int ret = 0;
mutex_lock(&mux->lock);
if (--mux->users)
goto done;
/* This was the last user, idle the mux if appropriate... */
if (mux->idle_state != MUX_IDLE_AS_IS &&
mux->idle_state != mux->cached_state)
ret = mux_control_set(mux, mux->idle_state);
/* ...and release a waiter if there is one. */
if (mux->waiters) {
--mux->waiters;
up(&mux->wait_sem);
}
done:
mutex_unlock(&mux->lock);
return ret;
}
>>>> +/*
>>>> + * Using subsys_initcall instead of module_init here to ensure - for the
>>>> + * non-modular case - that the subsystem is initialized when mux consumers
>>>> + * and mux controllers start to use it /without/ relying on link order.
>>>> + * For the modular case, the ordering is ensured with module dependencies.
>>>> + */
>>>> +subsys_initcall(mux_init);
>>>
>>> Even with subsys_initcall you are relying on link order, you do realize
>>> that? What about other subsystems that rely on this? :)
>>
>> Yes, that is true, but if others start relying on this, that's their problem,
>> right? :-)
>
> Yes, but no need to document something that isn't true. You are relying
> on link order here.
Well, am I? If I change it to module_init I do get runtime errors for a
non-modular build (when mux consumers get initialized before the mux core).
With subsys_init the mux core get initialized before all (current)
consumers. I don't really see how the link order *currently* matters?
So, just making sure that we are on the same page, the thing that relies on
link order are any mux consumers/drivers that in the future may be added as
part of some other subsys_init call (or earlier). Right?
Hmmm, or are you perhaps referring to the fact that the mux core depends on
other subsystems being initialized first?
>>>> +struct mux_control_ops {
>>>> + int (*set)(struct mux_control *mux, int state);
>>>> +};
>>>> +
>>>> +/* These defines match the constants from the dt-bindings. On purpose. */
>>>
>>> Why on purpose?
>>
>> I sure wasn't an accident? :-)
>>
>> Want me to remove it?
>
> At least explain _why_ you are doing this, that would help, right?
Should I perhaps just #include <linux/dt-bindings/mux/mux.h> instead?
Is that an OK thing to do? I didn't because I feared it might come back
to haunt me at some point if the bindings header needed stuff in the
future that made it incompatible...
It's also not terribly common to include bindings from an include...
Cheers,
peda
^ permalink raw reply [flat|nested] 38+ messages in thread* Re: [PATCH v13 03/10] mux: minimal mux subsystem and gpio-based mux controller
2017-04-18 21:53 ` Peter Rosin
@ 2017-04-19 2:23 ` Joe Perches
2017-04-20 21:53 ` Peter Rosin
1 sibling, 0 replies; 38+ messages in thread
From: Joe Perches @ 2017-04-19 2:23 UTC (permalink / raw)
To: Peter Rosin, Greg Kroah-Hartman
Cc: linux-kernel, Wolfram Sang, Rob Herring, Mark Rutland,
Jonathan Cameron, Hartmut Knaack, Lars-Peter Clausen,
Peter Meerwald-Stadler, Jonathan Corbet, linux-i2c, devicetree,
linux-iio, linux-doc, Andrew Morton, Colin Ian King,
Paul Gortmaker, Philipp Zabel, kernel
On Tue, 2017-04-18 at 23:53 +0200, Peter Rosin wrote:
> On 2017-04-18 13:44, Greg Kroah-Hartman wrote:
> > On Tue, Apr 18, 2017 at 12:59:50PM +0200, Peter Rosin wrote:
[]
> > > > > + ret = device_add(&mux_chip->dev);
> > > > > + if (ret < 0)
> > > > > + dev_err(&mux_chip->dev,
> > > > > + "device_add failed in mux_chip_register: %d\n", ret);
> > > >
> > > > Did you run checkpatch.pl in strict mode on this new file? Please do so :)
> > >
> > > I did, and did it again just to be sure, and I do not get any complaints.
> > > So, what's wrong?
> >
> > You list the function name in the printk string, it should complain
> > that __func__ should be used. Oh well, it's just a perl script, it
> > doesn't always catch everything.
> > isn't always correct :)
>
> Ah, ok.
Also, please use the checkpatch in -next as it has a
slightly better mechanism to identify functions and
uses in strings.
$ ./scripts/checkpatch.pl ~/1.patch
WARNING: Prefer using '"%s...", __func__' to using 'mux_chip_register', this function's name, in a string
#302: FILE: drivers/mux/mux-core.c:134:
+ "device_add failed in mux_chip_register: %d\n", ret);
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH v13 03/10] mux: minimal mux subsystem and gpio-based mux controller
2017-04-18 21:53 ` Peter Rosin
2017-04-19 2:23 ` Joe Perches
@ 2017-04-20 21:53 ` Peter Rosin
2017-04-21 23:28 ` Peter Rosin
1 sibling, 1 reply; 38+ messages in thread
From: Peter Rosin @ 2017-04-20 21:53 UTC (permalink / raw)
To: Greg Kroah-Hartman
Cc: linux-kernel, Wolfram Sang, Rob Herring, Mark Rutland,
Jonathan Cameron, Hartmut Knaack, Lars-Peter Clausen,
Peter Meerwald-Stadler, Jonathan Corbet, linux-i2c, devicetree,
linux-iio, linux-doc, Andrew Morton, Colin Ian King,
Paul Gortmaker, Philipp Zabel, kernel
On 2017-04-18 23:53, Peter Rosin wrote:
> On 2017-04-18 13:44, Greg Kroah-Hartman wrote:
>> On Tue, Apr 18, 2017 at 12:59:50PM +0200, Peter Rosin wrote:
>>> On 2017-04-18 10:51, Greg Kroah-Hartman wrote:
>>>> On Thu, Apr 13, 2017 at 06:43:07PM +0200, Peter Rosin wrote:
*snip*
>>>>> + if (mux->idle_state != MUX_IDLE_AS_IS &&
>>>>> + mux->idle_state != mux->cached_state)
>>>>> + ret = mux_control_set(mux, mux->idle_state);
>>>>> +
>>>>> + up_read(&mux->lock);
>>>>
>>>> You require a lock to be held for a "global" function? Without
>>>> documentation? Or even a sparse marking? That's asking for trouble...
>>>
>>> Documentation I can handle, but where should I look to understand how I
>>> should add sparse markings?
>>
>> Run sparse on the code and see what it says :)
>
> Will do.
I just did, and even went through the trouble of getting the bleeding
edge sparse from the git repo when sparse 0.5.0 came up empty, but it's
all silence for me. So, how do I add sparse markings?
Cheers,
peda
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH v13 03/10] mux: minimal mux subsystem and gpio-based mux controller
2017-04-20 21:53 ` Peter Rosin
@ 2017-04-21 23:28 ` Peter Rosin
0 siblings, 0 replies; 38+ messages in thread
From: Peter Rosin @ 2017-04-21 23:28 UTC (permalink / raw)
To: Greg Kroah-Hartman
Cc: linux-kernel, Wolfram Sang, Rob Herring, Mark Rutland,
Jonathan Cameron, Hartmut Knaack, Lars-Peter Clausen,
Peter Meerwald-Stadler, Jonathan Corbet, linux-i2c, devicetree,
linux-iio, linux-doc, Andrew Morton, Colin Ian King,
Paul Gortmaker, Philipp Zabel, kernel
On 2017-04-20 23:53, Peter Rosin wrote:
> On 2017-04-18 23:53, Peter Rosin wrote:
>> On 2017-04-18 13:44, Greg Kroah-Hartman wrote:
>>> On Tue, Apr 18, 2017 at 12:59:50PM +0200, Peter Rosin wrote:
>>>> On 2017-04-18 10:51, Greg Kroah-Hartman wrote:
>>>>> On Thu, Apr 13, 2017 at 06:43:07PM +0200, Peter Rosin wrote:
>
> *snip*
>
>>>>>> + if (mux->idle_state != MUX_IDLE_AS_IS &&
>>>>>> + mux->idle_state != mux->cached_state)
>>>>>> + ret = mux_control_set(mux, mux->idle_state);
>>>>>> +
>>>>>> + up_read(&mux->lock);
>>>>>
>>>>> You require a lock to be held for a "global" function? Without
>>>>> documentation? Or even a sparse marking? That's asking for trouble...
>>>>
>>>> Documentation I can handle, but where should I look to understand how I
>>>> should add sparse markings?
>>>
>>> Run sparse on the code and see what it says :)
>>
>> Will do.
>
> I just did, and even went through the trouble of getting the bleeding
> edge sparse from the git repo when sparse 0.5.0 came up empty, but it's
> all silence for me. So, how do I add sparse markings?
I looked some more into this, and the markings I find that seem related
are __acquire() and __release(). But neither mutex_lock() nor up_read()
has markings like that, so adding them when using those kinds of locks
in an imbalanced way seems like a sure way of *getting* sparse messages
about context imbalance...
So, either that, or you are talking about __must_check markings?
I feel like I'm missing something, please advise further.
Cheers,
peda
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH v13 03/10] mux: minimal mux subsystem and gpio-based mux controller
2017-04-18 10:59 ` Peter Rosin
2017-04-18 11:44 ` Greg Kroah-Hartman
@ 2017-05-05 13:19 ` Peter Rosin
1 sibling, 0 replies; 38+ messages in thread
From: Peter Rosin @ 2017-05-05 13:19 UTC (permalink / raw)
To: Greg Kroah-Hartman
Cc: linux-kernel, Wolfram Sang, Rob Herring, Mark Rutland,
Jonathan Cameron, Hartmut Knaack, Lars-Peter Clausen,
Peter Meerwald-Stadler, Jonathan Corbet, linux-i2c, devicetree,
linux-iio, linux-doc, Andrew Morton, Colin Ian King,
Paul Gortmaker, Philipp Zabel, kernel
On 2017-04-18 12:59, Peter Rosin wrote:
> On 2017-04-18 10:51, Greg Kroah-Hartman wrote:
>> On Thu, Apr 13, 2017 at 06:43:07PM +0200, Peter Rosin wrote:
>>> +/**
>>> + * devm_mux_chip_unregister() - Resource-managed version mux_chip_unregister().
>>> + * @dev: The device that originally registered the mux-chip.
>>> + * @mux_chip: The mux-chip to unregister.
>>> + *
>>> + * See mux_chip_unregister() for more details.
>>> + *
>>> + * Note that you do not normally need to call this function.
>>
>> Odd, then why is it exported???
>
> You normally don't call the devm_foo_{free,release,unregister,etc} functions.
> The intention is of course that the resourse cleans up automatically. But there
> are no cases where the manual clean up is not available, at least not that I can
> find?
I had a second look and there are of course plenty of examples of missing clean up
versions for devm function. I simply hadn't looked very hard at all.
So, for v15 I intend to remove all of devm_mux_chip_unregister, devm_mux_chip_free
and devm_mux_control_put. They are all just sitting there with no callers. And the
mux-mmio/video-mux drivers by Philipp Zabel that build on top of this series don't
need them either. Besides, easy to resurrect if needed...
I will do v15 with the above, the change from mutex to semaphore for locking the
mux controller state [1] and a few small documentation improvements. That will be
rebased onto v4.12-rc1 and sent in 10 days or so, or whenever v4.12-rc1 is out.
Meanwhile, what I currently intend for v15 but based on v4.11 is available from
https://gitlab.com/peda-linux/mux.git in the "mux" branch.
Cheers,
peda
[1] https://lkml.org/lkml/2017/4/25/411
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH v13 03/10] mux: minimal mux subsystem and gpio-based mux controller
2017-04-13 16:43 ` [PATCH v13 03/10] mux: minimal mux subsystem and gpio-based mux controller Peter Rosin
2017-04-18 8:34 ` Philipp Zabel
2017-04-18 8:51 ` Greg Kroah-Hartman
@ 2017-04-19 9:06 ` Philipp Zabel
2017-04-19 12:00 ` Peter Rosin
2017-04-21 14:18 ` Philipp Zabel
2017-04-21 14:23 ` Philipp Zabel
4 siblings, 1 reply; 38+ messages in thread
From: Philipp Zabel @ 2017-04-19 9:06 UTC (permalink / raw)
To: Peter Rosin
Cc: linux-kernel, Greg Kroah-Hartman, Wolfram Sang, Rob Herring,
Mark Rutland, Jonathan Cameron, Hartmut Knaack,
Lars-Peter Clausen, Peter Meerwald-Stadler, Jonathan Corbet,
linux-i2c, devicetree, linux-iio, linux-doc, Andrew Morton,
Colin Ian King, Paul Gortmaker, kernel
On Thu, 2017-04-13 at 18:43 +0200, Peter Rosin wrote:
> 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.
>
> Reviewed-by: Jonathan Cameron <jic23@kernel.org>
> Signed-off-by: Peter Rosin <peda@axentia.se>
> ---
> Documentation/driver-model/devres.txt | 8 +
> MAINTAINERS | 2 +
> drivers/Kconfig | 2 +
> drivers/Makefile | 1 +
> drivers/mux/Kconfig | 34 +++
> drivers/mux/Makefile | 6 +
> drivers/mux/mux-core.c | 422 ++++++++++++++++++++++++++++++++++
> drivers/mux/mux-gpio.c | 114 +++++++++
> include/linux/mux.h | 252 ++++++++++++++++++++
> 9 files changed, 841 insertions(+)
> create mode 100644 drivers/mux/Kconfig
> create mode 100644 drivers/mux/Makefile
> create mode 100644 drivers/mux/mux-core.c
> create mode 100644 drivers/mux/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 efb8200819d6..e2343d9cbec7 100644
> --- a/Documentation/driver-model/devres.txt
> +++ b/Documentation/driver-model/devres.txt
> @@ -337,6 +337,14 @@ MEM
> MFD
> devm_mfd_add_devices()
>
> +MUX
> + devm_mux_chip_alloc()
> + devm_mux_chip_free()
> + devm_mux_chip_register()
> + devm_mux_chip_unregister()
> + devm_mux_control_get()
> + devm_mux_control_put()
> +
> PER-CPU MEM
> devm_alloc_percpu()
> devm_free_percpu()
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 7fc06739c8ad..591eba737678 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -8563,6 +8563,8 @@ M: Peter Rosin <peda@axentia.se>
> S: Maintained
> F: Documentation/devicetree/bindings/mux/
> F: include/linux/dt-bindings/mux/
> +F: include/linux/mux.h
> +F: drivers/mux/
>
> MULTISOUND SOUND DRIVER
> M: Andrew Veliath <andrewtv@usa.net>
> diff --git a/drivers/Kconfig b/drivers/Kconfig
> index 117ca14ccf85..a7ea13e1b869 100644
> --- a/drivers/Kconfig
> +++ b/drivers/Kconfig
> @@ -204,4 +204,6 @@ source "drivers/fpga/Kconfig"
>
> source "drivers/fsi/Kconfig"
>
> +source "drivers/mux/Kconfig"
> +
> endmenu
> diff --git a/drivers/Makefile b/drivers/Makefile
> index 2eced9afba53..c0436f6dd5a9 100644
> --- a/drivers/Makefile
> +++ b/drivers/Makefile
> @@ -177,3 +177,4 @@ obj-$(CONFIG_ANDROID) += android/
> obj-$(CONFIG_NVMEM) += nvmem/
> obj-$(CONFIG_FPGA) += fpga/
> obj-$(CONFIG_FSI) += fsi/
> +obj-$(CONFIG_MULTIPLEXER) += mux/
> diff --git a/drivers/mux/Kconfig b/drivers/mux/Kconfig
> new file mode 100644
> index 000000000000..41dfe08ead84
> --- /dev/null
> +++ b/drivers/mux/Kconfig
> @@ -0,0 +1,34 @@
> +#
> +# Multiplexer devices
> +#
> +
> +menuconfig MULTIPLEXER
> + tristate "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.
> +
> + To compile the subsystem as a module, choose M here: the module will
> + be called mux-core.
> +
> +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
> + multiplexers, which in that case will be operated in parallel.
> +
> + To compile the driver as a module, choose M here: the module will
> + be called mux-gpio.
> +
> +endif
> diff --git a/drivers/mux/Makefile b/drivers/mux/Makefile
> new file mode 100644
> index 000000000000..bb16953f6290
> --- /dev/null
> +++ b/drivers/mux/Makefile
> @@ -0,0 +1,6 @@
> +#
> +# Makefile for multiplexer devices.
> +#
> +
> +obj-$(CONFIG_MULTIPLEXER) += mux-core.o
> +obj-$(CONFIG_MUX_GPIO) += mux-gpio.o
> diff --git a/drivers/mux/mux-core.c b/drivers/mux/mux-core.c
> new file mode 100644
> index 000000000000..66a8bccfc3d7
> --- /dev/null
> +++ b/drivers/mux/mux-core.c
> @@ -0,0 +1,422 @@
> +/*
> + * Multiplexer subsystem
> + *
> + * Copyright (C) 2017 Axentia Technologies AB
> + *
> + * Author: Peter Rosin <peda@axentia.se>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + */
> +
> +#define pr_fmt(fmt) "mux-core: " fmt
> +
> +#include <linux/device.h>
> +#include <linux/err.h>
> +#include <linux/export.h>
> +#include <linux/idr.h>
> +#include <linux/init.h>
> +#include <linux/module.h>
> +#include <linux/mux.h>
> +#include <linux/of.h>
> +#include <linux/of_platform.h>
> +#include <linux/slab.h>
> +
> +/*
> + * The idle-as-is "state" is not an actual state that may be selected, it
> + * only implies that the state should not be changed. So, use that state
> + * as indication that the cached state of the multiplexer is unknown.
> + */
> +#define MUX_CACHE_UNKNOWN MUX_IDLE_AS_IS
> +
> +static struct class mux_class = {
> + .name = "mux",
> + .owner = THIS_MODULE,
> +};
> +
> +static int __init mux_init(void)
> +{
> + return class_register(&mux_class);
> +}
> +
> +static DEFINE_IDA(mux_ida);
> +
> +static void mux_chip_release(struct device *dev)
> +{
> + struct mux_chip *mux_chip = to_mux_chip(dev);
> +
> + ida_simple_remove(&mux_ida, mux_chip->id);
> + kfree(mux_chip);
> +}
> +
> +static struct device_type mux_type = {
> + .name = "mux-chip",
> + .release = mux_chip_release,
> +};
> +
> +struct mux_chip *mux_chip_alloc(struct device *dev,
> + unsigned int controllers, size_t sizeof_priv)
> +{
> + struct mux_chip *mux_chip;
> + int i;
> +
> + if (WARN_ON(!dev || !controllers))
> + return NULL;
> +
> + mux_chip = kzalloc(sizeof(*mux_chip) +
> + controllers * sizeof(*mux_chip->mux) +
> + sizeof_priv, GFP_KERNEL);
> + if (!mux_chip)
> + return NULL;
> +
> + mux_chip->mux = (struct mux_control *)(mux_chip + 1);
> + mux_chip->dev.class = &mux_class;
> + mux_chip->dev.type = &mux_type;
> + mux_chip->dev.parent = dev;
> + mux_chip->dev.of_node = dev->of_node;
> + dev_set_drvdata(&mux_chip->dev, mux_chip);
> +
> + mux_chip->id = ida_simple_get(&mux_ida, 0, 0, GFP_KERNEL);
> + if (mux_chip->id < 0) {
> + pr_err("muxchipX failed to get a device id\n");
> + kfree(mux_chip);
> + return NULL;
> + }
> + dev_set_name(&mux_chip->dev, "muxchip%d", mux_chip->id);
> +
> + mux_chip->controllers = controllers;
> + for (i = 0; i < controllers; ++i) {
> + struct mux_control *mux = &mux_chip->mux[i];
> +
> + mux->chip = mux_chip;
> + init_rwsem(&mux->lock);
> + mux->cached_state = MUX_CACHE_UNKNOWN;
> + mux->idle_state = MUX_IDLE_AS_IS;
> + }
> +
> + device_initialize(&mux_chip->dev);
> +
> + return mux_chip;
> +}
> +EXPORT_SYMBOL_GPL(mux_chip_alloc);
> +
> +static int mux_control_set(struct mux_control *mux, int state)
> +{
> + int ret = mux->chip->ops->set(mux, state);
> +
> + mux->cached_state = ret < 0 ? MUX_CACHE_UNKNOWN : state;
> +
> + return ret;
> +}
> +
> +int mux_chip_register(struct mux_chip *mux_chip)
> +{
> + int i;
> + int ret;
> +
> + for (i = 0; i < mux_chip->controllers; ++i) {
> + struct mux_control *mux = &mux_chip->mux[i];
> +
> + if (mux->idle_state == mux->cached_state)
> + continue;
> +
> + ret = mux_control_set(mux, mux->idle_state);
> + if (ret < 0) {
> + dev_err(&mux_chip->dev, "unable to set idle state\n");
> + return ret;
> + }
> + }
> +
> + ret = device_add(&mux_chip->dev);
> + if (ret < 0)
> + dev_err(&mux_chip->dev,
> + "device_add failed in mux_chip_register: %d\n", ret);
> + return ret;
> +}
> +EXPORT_SYMBOL_GPL(mux_chip_register);
> +
> +void mux_chip_unregister(struct mux_chip *mux_chip)
> +{
> + device_del(&mux_chip->dev);
> +}
> +EXPORT_SYMBOL_GPL(mux_chip_unregister);
> +
> +void mux_chip_free(struct mux_chip *mux_chip)
> +{
> + if (!mux_chip)
> + return;
> +
> + put_device(&mux_chip->dev);
> +}
> +EXPORT_SYMBOL_GPL(mux_chip_free);
> +
> +static void devm_mux_chip_release(struct device *dev, void *res)
> +{
> + struct mux_chip *mux_chip = *(struct mux_chip **)res;
> +
> + mux_chip_free(mux_chip);
> +}
> +
> +struct mux_chip *devm_mux_chip_alloc(struct device *dev,
> + unsigned int controllers,
> + size_t sizeof_priv)
> +{
> + struct mux_chip **ptr, *mux_chip;
> +
> + ptr = devres_alloc(devm_mux_chip_release, sizeof(*ptr), GFP_KERNEL);
> + if (!ptr)
> + return NULL;
> +
> + mux_chip = mux_chip_alloc(dev, controllers, sizeof_priv);
> + if (!mux_chip) {
> + devres_free(ptr);
> + return NULL;
> + }
> +
> + *ptr = mux_chip;
> + devres_add(dev, ptr);
> +
> + return mux_chip;
> +}
> +EXPORT_SYMBOL_GPL(devm_mux_chip_alloc);
> +
> +static int devm_mux_chip_match(struct device *dev, void *res, void *data)
> +{
> + struct mux_chip **r = res;
> +
> + if (WARN_ON(!r || !*r))
> + return 0;
> +
> + return *r == data;
> +}
> +
> +void devm_mux_chip_free(struct device *dev, struct mux_chip *mux_chip)
> +{
> + WARN_ON(devres_release(dev, devm_mux_chip_release,
> + devm_mux_chip_match, mux_chip));
> +}
> +EXPORT_SYMBOL_GPL(devm_mux_chip_free);
> +
> +static void devm_mux_chip_reg_release(struct device *dev, void *res)
> +{
> + struct mux_chip *mux_chip = *(struct mux_chip **)res;
> +
> + mux_chip_unregister(mux_chip);
> +}
> +
> +int devm_mux_chip_register(struct device *dev,
> + struct mux_chip *mux_chip)
> +{
> + struct mux_chip **ptr;
> + int res;
> +
> + ptr = devres_alloc(devm_mux_chip_reg_release, sizeof(*ptr), GFP_KERNEL);
> + if (!ptr)
> + return -ENOMEM;
> +
> + res = mux_chip_register(mux_chip);
> + if (res) {
> + devres_free(ptr);
> + return res;
> + }
> +
> + *ptr = mux_chip;
> + devres_add(dev, ptr);
> +
> + return res;
> +}
> +EXPORT_SYMBOL_GPL(devm_mux_chip_register);
> +
> +void devm_mux_chip_unregister(struct device *dev, struct mux_chip *mux_chip)
> +{
> + WARN_ON(devres_release(dev, devm_mux_chip_reg_release,
> + devm_mux_chip_match, mux_chip));
> +}
> +EXPORT_SYMBOL_GPL(devm_mux_chip_unregister);
> +
> +int mux_control_select(struct mux_control *mux, int state)
If we let two of these race, ...
> +{
> + int ret;
> +
> + if (down_read_trylock(&mux->lock)) {
> + if (mux->cached_state == state)
> + return 0;
> + /* Sigh, the mux needs updating... */
> + up_read(&mux->lock);
... and both decide the mux needs updating ...
> + }
> +
> + /* ...or it's just contended. */
> + down_write(&mux->lock);
... then the last to get to down_write will just wait here forever (or
until the first consumer calls mux_control_deselect, which may never
happen)?
> +
> + if (mux->cached_state == state) {
> + /*
> + * Hmmm, someone else changed the mux to my liking.
> + * That makes me wonder how long I waited for nothing?
> + */
> + downgrade_write(&mux->lock);
> + return 0;
> + }
> +
> + ret = mux_control_set(mux, state);
> + if (ret < 0) {
> + if (mux->idle_state != MUX_IDLE_AS_IS)
> + mux_control_set(mux, mux->idle_state);
> +
> + up_write(&mux->lock);
> + return ret;
> + }
> +
> + downgrade_write(&mux->lock);
> +
> + return 1;
> +}
> +EXPORT_SYMBOL_GPL(mux_control_select);
I wonder if these should be called mux_control_lock/unlock instead,
which would allow for try_lock and lock_timeout variants.
regards
Philipp
^ permalink raw reply [flat|nested] 38+ messages in thread* Re: [PATCH v13 03/10] mux: minimal mux subsystem and gpio-based mux controller
2017-04-19 9:06 ` Philipp Zabel
@ 2017-04-19 12:00 ` Peter Rosin
2017-04-19 13:49 ` Philipp Zabel
0 siblings, 1 reply; 38+ messages in thread
From: Peter Rosin @ 2017-04-19 12:00 UTC (permalink / raw)
To: Philipp Zabel
Cc: linux-kernel, Greg Kroah-Hartman, Wolfram Sang, Rob Herring,
Mark Rutland, Jonathan Cameron, Hartmut Knaack,
Lars-Peter Clausen, Peter Meerwald-Stadler, Jonathan Corbet,
linux-i2c, devicetree, linux-iio, linux-doc, Andrew Morton,
Colin Ian King, Paul Gortmaker, kernel
On 2017-04-19 11:06, Philipp Zabel wrote:
> On Thu, 2017-04-13 at 18:43 +0200, Peter Rosin wrote:
>> 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.
>>
>> Reviewed-by: Jonathan Cameron <jic23@kernel.org>
>> Signed-off-by: Peter Rosin <peda@axentia.se>
>> ---
*snip*
>> +int mux_control_select(struct mux_control *mux, int state)
>
> If we let two of these race, ...
The window for this "race" is positively huge. If there are several
mux consumers of a single mux controller, it is self-evident that
if one of them grabs the mux for a long time, the others will suffer.
The design is that the rwsem is reader-locked for the full duration
of a select/deselect operation by the mux consumer.
>> +{
>> + int ret;
>> +
>> + if (down_read_trylock(&mux->lock)) {
>> + if (mux->cached_state == state)
>> + return 0;
>> + /* Sigh, the mux needs updating... */
>> + up_read(&mux->lock);
>
> ... and both decide the mux needs updating ...
>
>> + }
>> +
>> + /* ...or it's just contended. */
>> + down_write(&mux->lock);
>
> ... then the last to get to down_write will just wait here forever (or
> until the first consumer calls mux_control_deselect, which may never
> happen)?
It is vital that the mux consumer call _deselect when it is done with
the mux. Not doing so will surely starve out any other mux consumers.
The whole thing is designed around the fact that mux consumers should
deselect the mux as soon as it's no longer needed.
It's simply not possible to share something as fundamental as a mux
without some cooperation. It's not like suffering mux consumers can
go off and use some other mux, and it's also not possible for a
"competing" mux consumer to just clobber the mux state.
>> +
>> + if (mux->cached_state == state) {
>> + /*
>> + * Hmmm, someone else changed the mux to my liking.
>> + * That makes me wonder how long I waited for nothing?
>> + */
>> + downgrade_write(&mux->lock);
>> + return 0;
>> + }
>> +
>> + ret = mux_control_set(mux, state);
>> + if (ret < 0) {
>> + if (mux->idle_state != MUX_IDLE_AS_IS)
>> + mux_control_set(mux, mux->idle_state);
>> +
>> + up_write(&mux->lock);
>> + return ret;
>> + }
>> +
>> + downgrade_write(&mux->lock);
>> +
>> + return 1;
>> +}
>> +EXPORT_SYMBOL_GPL(mux_control_select);
>
> I wonder if these should be called mux_control_lock/unlock instead,
> which would allow for try_lock and lock_timeout variants.
Maybe, I'm not totally against it. Do others care to opine?
But mux_control_try_select and mux_control_select_timeout does not
look all that bad either. But maybe foo_lock is making it clearer
that a foo_unlock is needed, if you compared it to foo_select and
foo_unselect? I'm probably not the best person to make the call,
as I know all to well what to expect from the functions...
Cheers,
peda
^ permalink raw reply [flat|nested] 38+ messages in thread* Re: [PATCH v13 03/10] mux: minimal mux subsystem and gpio-based mux controller
2017-04-19 12:00 ` Peter Rosin
@ 2017-04-19 13:49 ` Philipp Zabel
2017-04-19 21:04 ` Peter Rosin
0 siblings, 1 reply; 38+ messages in thread
From: Philipp Zabel @ 2017-04-19 13:49 UTC (permalink / raw)
To: Peter Rosin
Cc: linux-kernel, Greg Kroah-Hartman, Wolfram Sang, Rob Herring,
Mark Rutland, Jonathan Cameron, Hartmut Knaack,
Lars-Peter Clausen, Peter Meerwald-Stadler, Jonathan Corbet,
linux-i2c, devicetree, linux-iio, linux-doc, Andrew Morton,
Colin Ian King, Paul Gortmaker, kernel
On Wed, 2017-04-19 at 14:00 +0200, Peter Rosin wrote:
[...]
> >> +int mux_control_select(struct mux_control *mux, int state)
> >
> > If we let two of these race, ...
>
> The window for this "race" is positively huge. If there are several
> mux consumers of a single mux controller, it is self-evident that
> if one of them grabs the mux for a long time, the others will suffer.
>
> The design is that the rwsem is reader-locked for the full duration
> of a select/deselect operation by the mux consumer.
I was not clear. I meant: I think this can also happen if we let them
race with the same state target.
> >> +{
> >> + int ret;
> >> +
> >> + if (down_read_trylock(&mux->lock)) {
> >> + if (mux->cached_state == state)
> >> + return 0;
This check makes it clear that a second select call is not intended to
block if the intended state is already selected. But if the instance we
will lose the race against has not yet updated cached_state, ...
> >> + /* Sigh, the mux needs updating... */
> >> + up_read(&mux->lock);
> >> + }
> >> +
> >> + /* ...or it's just contended. */
> >> + down_write(&mux->lock);
... we are blocking here until the other instance calls up_read. Even
though in this case (same state target) we would only have to block
until the other instance calls downgrade_write after the mux control is
set to the correct state.
Basically there is a small window before down_write with no lock at all,
where multiple instances can already have decided they must change the
mux (to the same state). If this happens, they go on to block each other
unnecessarily.
> > ... then the last to get to down_write will just wait here forever (or
> > until the first consumer calls mux_control_deselect, which may never
> > happen)?
>
> It is vital that the mux consumer call _deselect when it is done with
> the mux. Not doing so will surely starve out any other mux consumers.
> The whole thing is designed around the fact that mux consumers should
> deselect the mux as soon as it's no longer needed.
I'd like to use this for video bus multiplexers. Those would not be
selected for the duration of an i2c transfer, but for the duration of a
running video capture stream, or for the duration of an enabled display
path. While I currently have no use case for multiple consumers
controlling the same mux, this scenario is what shapes my perspective.
For such long running selections the consumer should have the option to
return -EBUSY instead of blocking when the lock can't be taken.
regards
Philipp
^ permalink raw reply [flat|nested] 38+ messages in thread* Re: [PATCH v13 03/10] mux: minimal mux subsystem and gpio-based mux controller
2017-04-19 13:49 ` Philipp Zabel
@ 2017-04-19 21:04 ` Peter Rosin
0 siblings, 0 replies; 38+ messages in thread
From: Peter Rosin @ 2017-04-19 21:04 UTC (permalink / raw)
To: Philipp Zabel
Cc: linux-kernel, Greg Kroah-Hartman, Wolfram Sang, Rob Herring,
Mark Rutland, Jonathan Cameron, Hartmut Knaack,
Lars-Peter Clausen, Peter Meerwald-Stadler, Jonathan Corbet,
linux-i2c, devicetree, linux-iio, linux-doc, Andrew Morton,
Colin Ian King, Paul Gortmaker, kernel
On 2017-04-19 15:49, Philipp Zabel wrote:
> On Wed, 2017-04-19 at 14:00 +0200, Peter Rosin wrote:
> [...]
>>>> +int mux_control_select(struct mux_control *mux, int state)
>>>
>>> If we let two of these race, ...
>>
>> The window for this "race" is positively huge. If there are several
>> mux consumers of a single mux controller, it is self-evident that
>> if one of them grabs the mux for a long time, the others will suffer.
>>
>> The design is that the rwsem is reader-locked for the full duration
>> of a select/deselect operation by the mux consumer.
>
> I was not clear. I meant: I think this can also happen if we let them
> race with the same state target.
Right, but there is another glaring problem with the v13 implementation
of select/deselect. If there are three consumers and the first one
holds the mux while the other two tries to select it to some other
position, then even if the two "new" consumers agree on the mux state,
then both of them will end up in the "it's just contended" case and
then be serialized. So, yes, the select/deselect implementation is not
perfect. To quote the cover letter:
I'm using an rwsem to lock a mux, but that isn't really a
perfect fit. Is there a better locking primitive that I don't
know about that fits better? I had a mutex at one point, but
that didn't allow any concurrent accesses at all. At least
the rwsem allows concurrent access as long as all users
agree on the mux state, but I suspect that the rwsem will
degrade to the mutex situation pretty quickly if there is
any contention.
But with your discovery that there's a race when two consumers go
for the same state in a free mux, in addition to the above contention
problem, I'm about ready to go with Greg's suggestion and just use a
mutex. I.e. ignore the desire to allow concurrent use. Because it's
not like the sketchy thing I threw out in the other part of the thread
solves any of these problems. I can live without the concurrency, and
I guess I can also live with passing the buck to the poor sod that
eventually needs it.
>>>> +{
>>>> + int ret;
>>>> +
>>>> + if (down_read_trylock(&mux->lock)) {
>>>> + if (mux->cached_state == state)
>>>> + return 0;
>
> This check makes it clear that a second select call is not intended to
> block if the intended state is already selected. But if the instance we
> will lose the race against has not yet updated cached_state, ...
>
>>>> + /* Sigh, the mux needs updating... */
>>>> + up_read(&mux->lock);
>>>> + }
>>>> +
>>>> + /* ...or it's just contended. */
>>>> + down_write(&mux->lock);
>
> ... we are blocking here until the other instance calls up_read. Even
> though in this case (same state target) we would only have to block
> until the other instance calls downgrade_write after the mux control is
> set to the correct state.
>
> Basically there is a small window before down_write with no lock at all,
> where multiple instances can already have decided they must change the
> mux (to the same state). If this happens, they go on to block each other
> unnecessarily.
>
>>> ... then the last to get to down_write will just wait here forever (or
>>> until the first consumer calls mux_control_deselect, which may never
>>> happen)?
>>
>> It is vital that the mux consumer call _deselect when it is done with
>> the mux. Not doing so will surely starve out any other mux consumers.
>> The whole thing is designed around the fact that mux consumers should
>> deselect the mux as soon as it's no longer needed.
>
> I'd like to use this for video bus multiplexers. Those would not be
> selected for the duration of an i2c transfer, but for the duration of a
> running video capture stream, or for the duration of an enabled display
> path. While I currently have no use case for multiple consumers
> controlling the same mux, this scenario is what shapes my perspective.
> For such long running selections the consumer should have the option to
> return -EBUSY instead of blocking when the lock can't be taken.
I'll see if I can implement a try variant for the mutex based select I
will probably end up with in v14...
Cheers,
peda
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH v13 03/10] mux: minimal mux subsystem and gpio-based mux controller
2017-04-13 16:43 ` [PATCH v13 03/10] mux: minimal mux subsystem and gpio-based mux controller Peter Rosin
` (2 preceding siblings ...)
2017-04-19 9:06 ` Philipp Zabel
@ 2017-04-21 14:18 ` Philipp Zabel
2017-04-21 15:08 ` Peter Rosin
2017-04-21 14:23 ` Philipp Zabel
4 siblings, 1 reply; 38+ messages in thread
From: Philipp Zabel @ 2017-04-21 14:18 UTC (permalink / raw)
To: Peter Rosin
Cc: linux-kernel, Greg Kroah-Hartman, Wolfram Sang, Rob Herring,
Mark Rutland, Jonathan Cameron, Hartmut Knaack,
Lars-Peter Clausen, Peter Meerwald-Stadler, Jonathan Corbet,
linux-i2c, devicetree, linux-iio, linux-doc, Andrew Morton,
Colin Ian King, Paul Gortmaker, kernel
Hi Peter,
On Thu, 2017-04-13 at 18:43 +0200, Peter Rosin wrote:
[...]
> +int mux_control_select(struct mux_control *mux, int state)
state could be unsigned int for the consumer facing API.
> +{
> + int ret;
And mux_control_select should check that (0 <= state < mux->states).
regards
Philipp
^ permalink raw reply [flat|nested] 38+ messages in thread* Re: [PATCH v13 03/10] mux: minimal mux subsystem and gpio-based mux controller
2017-04-21 14:18 ` Philipp Zabel
@ 2017-04-21 15:08 ` Peter Rosin
0 siblings, 0 replies; 38+ messages in thread
From: Peter Rosin @ 2017-04-21 15:08 UTC (permalink / raw)
To: Philipp Zabel
Cc: linux-kernel, Greg Kroah-Hartman, Wolfram Sang, Rob Herring,
Mark Rutland, Jonathan Cameron, Hartmut Knaack,
Lars-Peter Clausen, Peter Meerwald-Stadler, Jonathan Corbet,
linux-i2c, devicetree, linux-iio, linux-doc, Andrew Morton,
Colin Ian King, Paul Gortmaker, kernel
On 2017-04-21 16:18, Philipp Zabel wrote:
> Hi Peter,
>
> On Thu, 2017-04-13 at 18:43 +0200, Peter Rosin wrote:
> [...]
>> +int mux_control_select(struct mux_control *mux, int state)
>
> state could be unsigned int for the consumer facing API.
>
>> +{
>> + int ret;
>
> And mux_control_select should check that (0 <= state < mux->states).
Yes, that makes sense. I worried that we might end up with
signed/unsigned comparisons since the internal state still needs
to be signed, but that didn't happen when I tried...
I'll include this change in v14.
Cheers,
peda
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH v13 03/10] mux: minimal mux subsystem and gpio-based mux controller
2017-04-13 16:43 ` [PATCH v13 03/10] mux: minimal mux subsystem and gpio-based mux controller Peter Rosin
` (3 preceding siblings ...)
2017-04-21 14:18 ` Philipp Zabel
@ 2017-04-21 14:23 ` Philipp Zabel
2017-04-21 14:32 ` Peter Rosin
4 siblings, 1 reply; 38+ messages in thread
From: Philipp Zabel @ 2017-04-21 14:23 UTC (permalink / raw)
To: Peter Rosin
Cc: linux-kernel, Greg Kroah-Hartman, Wolfram Sang, Rob Herring,
Mark Rutland, Jonathan Cameron, Hartmut Knaack,
Lars-Peter Clausen, Peter Meerwald-Stadler, Jonathan Corbet,
linux-i2c, devicetree, linux-iio, linux-doc, Andrew Morton,
Colin Ian King, Paul Gortmaker, kernel
On Thu, 2017-04-13 at 18:43 +0200, Peter Rosin wrote:
[...]
> +int mux_chip_register(struct mux_chip *mux_chip)
> +{
> + int i;
> + int ret;
> +
> + for (i = 0; i < mux_chip->controllers; ++i) {
> + struct mux_control *mux = &mux_chip->mux[i];
> +
> + if (mux->idle_state == mux->cached_state)
> + continue;
I think this should be changed to
- if (mux->idle_state == mux->cached_state)
+ if (mux->idle_state == mux->cached_state ||
+ mux->idle_state == MUX_IDLE_AS_IS)
continue;
or the following mux_control_set will be called with state ==
MUX_IDLE_AS_IS. Alternatively, mux_control_set should return when passed
this value.
> + ret = mux_control_set(mux, mux->idle_state);
> + if (ret < 0) {
> + dev_err(&mux_chip->dev, "unable to set idle state\n");
> + return ret;
> + }
> + }
> +
> + ret = device_add(&mux_chip->dev);
> + if (ret < 0)
> + dev_err(&mux_chip->dev,
> + "device_add failed in mux_chip_register: %d\n", ret);
> + return ret;
> +}
> +EXPORT_SYMBOL_GPL(mux_chip_register);
regards
Philipp
^ permalink raw reply [flat|nested] 38+ messages in thread* Re: [PATCH v13 03/10] mux: minimal mux subsystem and gpio-based mux controller
2017-04-21 14:23 ` Philipp Zabel
@ 2017-04-21 14:32 ` Peter Rosin
2017-04-21 14:41 ` Philipp Zabel
0 siblings, 1 reply; 38+ messages in thread
From: Peter Rosin @ 2017-04-21 14:32 UTC (permalink / raw)
To: Philipp Zabel
Cc: linux-kernel, Greg Kroah-Hartman, Wolfram Sang, Rob Herring,
Mark Rutland, Jonathan Cameron, Hartmut Knaack,
Lars-Peter Clausen, Peter Meerwald-Stadler, Jonathan Corbet,
linux-i2c, devicetree, linux-iio, linux-doc, Andrew Morton,
Colin Ian King, Paul Gortmaker, kernel
On 2017-04-21 16:23, Philipp Zabel wrote:
> On Thu, 2017-04-13 at 18:43 +0200, Peter Rosin wrote:
> [...]
>> +int mux_chip_register(struct mux_chip *mux_chip)
>> +{
>> + int i;
>> + int ret;
>> +
>> + for (i = 0; i < mux_chip->controllers; ++i) {
>> + struct mux_control *mux = &mux_chip->mux[i];
>> +
>> + if (mux->idle_state == mux->cached_state)
>> + continue;
>
> I think this should be changed to
>
> - if (mux->idle_state == mux->cached_state)
> + if (mux->idle_state == mux->cached_state ||
> + mux->idle_state == MUX_IDLE_AS_IS)
> continue;
>
> or the following mux_control_set will be called with state ==
> MUX_IDLE_AS_IS. Alternatively, mux_control_set should return when passed
> this value.
That cannot happen because ->cached_state is initialized to -1
in mux_chip_alloc, so should always be == MUX_IDLE_AS_IS when
registering. And drivers are not supposed to touch ->cached_state.
I.e., ->cached_state is "owned" by the core.
Cheers,
peda
>> + ret = mux_control_set(mux, mux->idle_state);
>> + if (ret < 0) {
>> + dev_err(&mux_chip->dev, "unable to set idle state\n");
>> + return ret;
>> + }
>> + }
>> +
>> + ret = device_add(&mux_chip->dev);
>> + if (ret < 0)
>> + dev_err(&mux_chip->dev,
>> + "device_add failed in mux_chip_register: %d\n", ret);
>> + return ret;
>> +}
>> +EXPORT_SYMBOL_GPL(mux_chip_register);
>
> regards
> Philipp
>
^ permalink raw reply [flat|nested] 38+ messages in thread* Re: [PATCH v13 03/10] mux: minimal mux subsystem and gpio-based mux controller
2017-04-21 14:32 ` Peter Rosin
@ 2017-04-21 14:41 ` Philipp Zabel
2017-04-21 14:55 ` Peter Rosin
0 siblings, 1 reply; 38+ messages in thread
From: Philipp Zabel @ 2017-04-21 14:41 UTC (permalink / raw)
To: Peter Rosin
Cc: Mark Rutland, devicetree, Lars-Peter Clausen, kernel,
Wolfram Sang, linux-iio, Greg Kroah-Hartman, Jonathan Corbet,
linux-doc, linux-kernel, Paul Gortmaker, Rob Herring, linux-i2c,
Peter Meerwald-Stadler, Hartmut Knaack, Colin Ian King,
Andrew Morton, Jonathan Cameron
On Fri, 2017-04-21 at 16:32 +0200, Peter Rosin wrote:
> On 2017-04-21 16:23, Philipp Zabel wrote:
> > On Thu, 2017-04-13 at 18:43 +0200, Peter Rosin wrote:
> > [...]
> >> +int mux_chip_register(struct mux_chip *mux_chip)
> >> +{
> >> + int i;
> >> + int ret;
> >> +
> >> + for (i = 0; i < mux_chip->controllers; ++i) {
> >> + struct mux_control *mux = &mux_chip->mux[i];
> >> +
> >> + if (mux->idle_state == mux->cached_state)
> >> + continue;
> >
> > I think this should be changed to
> >
> > - if (mux->idle_state == mux->cached_state)
> > + if (mux->idle_state == mux->cached_state ||
> > + mux->idle_state == MUX_IDLE_AS_IS)
> > continue;
> >
> > or the following mux_control_set will be called with state ==
> > MUX_IDLE_AS_IS. Alternatively, mux_control_set should return when passed
> > this value.
>
> That cannot happen because ->cached_state is initialized to -1
> in mux_chip_alloc, so should always be == MUX_IDLE_AS_IS when
> registering. And drivers are not supposed to touch ->cached_state.
> I.e., ->cached_state is "owned" by the core.
So this was caused by me filling cached_state from register reads in the
mmio driver. Makes me wonder why I am not allowed to do this, though, if
I am able to read back the initial state?
regards
Philipp
^ permalink raw reply [flat|nested] 38+ messages in thread* Re: [PATCH v13 03/10] mux: minimal mux subsystem and gpio-based mux controller
2017-04-21 14:41 ` Philipp Zabel
@ 2017-04-21 14:55 ` Peter Rosin
2017-04-21 15:19 ` Philipp Zabel
0 siblings, 1 reply; 38+ messages in thread
From: Peter Rosin @ 2017-04-21 14:55 UTC (permalink / raw)
To: Philipp Zabel
Cc: Mark Rutland, devicetree, Lars-Peter Clausen, kernel,
Wolfram Sang, linux-iio, Greg Kroah-Hartman, Jonathan Corbet,
linux-doc, linux-kernel, Paul Gortmaker, Rob Herring, linux-i2c,
Peter Meerwald-Stadler, Hartmut Knaack, Colin Ian King,
Andrew Morton, Jonathan Cameron
On 2017-04-21 16:41, Philipp Zabel wrote:
> On Fri, 2017-04-21 at 16:32 +0200, Peter Rosin wrote:
>> On 2017-04-21 16:23, Philipp Zabel wrote:
>>> On Thu, 2017-04-13 at 18:43 +0200, Peter Rosin wrote:
>>> [...]
>>>> +int mux_chip_register(struct mux_chip *mux_chip)
>>>> +{
>>>> + int i;
>>>> + int ret;
>>>> +
>>>> + for (i = 0; i < mux_chip->controllers; ++i) {
>>>> + struct mux_control *mux = &mux_chip->mux[i];
>>>> +
>>>> + if (mux->idle_state == mux->cached_state)
>>>> + continue;
>>>
>>> I think this should be changed to
>>>
>>> - if (mux->idle_state == mux->cached_state)
>>> + if (mux->idle_state == mux->cached_state ||
>>> + mux->idle_state == MUX_IDLE_AS_IS)
>>> continue;
>>>
>>> or the following mux_control_set will be called with state ==
>>> MUX_IDLE_AS_IS. Alternatively, mux_control_set should return when passed
>>> this value.
>>
>> That cannot happen because ->cached_state is initialized to -1
>> in mux_chip_alloc, so should always be == MUX_IDLE_AS_IS when
>> registering. And drivers are not supposed to touch ->cached_state.
>> I.e., ->cached_state is "owned" by the core.
>
> So this was caused by me filling cached_state from register reads in the
> mmio driver. Makes me wonder why I am not allowed to do this, though, if
> I am able to read back the initial state?
You gain fairly little by reading back the original state. If the mux
should idle-as-is, you can avoid a maximum of one mux update if the first
consumer happens to starts by requesting the previously active state.
Similarly, if the mux should idle in a specific state, you can avoid a
maximum of one mux update.
In both cases it costs one unconditional read of the mux state.
Sure, in some cases reads are cheaper than writes, but I didn't think
support for seeding the cache was worth it. Is it worth it?
Cheers,
peda
^ permalink raw reply [flat|nested] 38+ messages in thread* Re: [PATCH v13 03/10] mux: minimal mux subsystem and gpio-based mux controller
2017-04-21 14:55 ` Peter Rosin
@ 2017-04-21 15:19 ` Philipp Zabel
0 siblings, 0 replies; 38+ messages in thread
From: Philipp Zabel @ 2017-04-21 15:19 UTC (permalink / raw)
To: Peter Rosin
Cc: Mark Rutland, devicetree, Lars-Peter Clausen, kernel,
Wolfram Sang, linux-iio, Greg Kroah-Hartman, Jonathan Corbet,
linux-doc, linux-kernel, Paul Gortmaker, Rob Herring, linux-i2c,
Peter Meerwald-Stadler, Hartmut Knaack, Colin Ian King,
Andrew Morton, Jonathan Cameron
On Fri, 2017-04-21 at 16:55 +0200, Peter Rosin wrote:
> On 2017-04-21 16:41, Philipp Zabel wrote:
> > On Fri, 2017-04-21 at 16:32 +0200, Peter Rosin wrote:
> >> On 2017-04-21 16:23, Philipp Zabel wrote:
> >>> On Thu, 2017-04-13 at 18:43 +0200, Peter Rosin wrote:
> >>> [...]
> >>>> +int mux_chip_register(struct mux_chip *mux_chip)
> >>>> +{
> >>>> + int i;
> >>>> + int ret;
> >>>> +
> >>>> + for (i = 0; i < mux_chip->controllers; ++i) {
> >>>> + struct mux_control *mux = &mux_chip->mux[i];
> >>>> +
> >>>> + if (mux->idle_state == mux->cached_state)
> >>>> + continue;
> >>>
> >>> I think this should be changed to
> >>>
> >>> - if (mux->idle_state == mux->cached_state)
> >>> + if (mux->idle_state == mux->cached_state ||
> >>> + mux->idle_state == MUX_IDLE_AS_IS)
> >>> continue;
> >>>
> >>> or the following mux_control_set will be called with state ==
> >>> MUX_IDLE_AS_IS. Alternatively, mux_control_set should return when passed
> >>> this value.
> >>
> >> That cannot happen because ->cached_state is initialized to -1
> >> in mux_chip_alloc, so should always be == MUX_IDLE_AS_IS when
> >> registering. And drivers are not supposed to touch ->cached_state.
> >> I.e., ->cached_state is "owned" by the core.
> >
> > So this was caused by me filling cached_state from register reads in the
> > mmio driver. Makes me wonder why I am not allowed to do this, though, if
> > I am able to read back the initial state?
>
> You gain fairly little by reading back the original state. If the mux
> should idle-as-is, you can avoid a maximum of one mux update if the first
> consumer happens to starts by requesting the previously active state.
> Similarly, if the mux should idle in a specific state, you can avoid a
> maximum of one mux update.
>
> In both cases it costs one unconditional read of the mux state.
>
> Sure, in some cases reads are cheaper than writes, but I didn't think
> support for seeding the cache was worth it. Is it worth it?
Probably not, I'll just drop the cached_state initialization. It should
be documented in the mux.h that this field is framework internal and not
to be touched by the drivers. At least I was surprised.
regards
Philipp
^ permalink raw reply [flat|nested] 38+ messages in thread
* [PATCH v13 04/10] iio: inkern: api for manipulating ext_info of iio channels
2017-04-13 16:43 [PATCH v13 00/10] mux controller abstraction and iio/i2c muxes Peter Rosin
` (2 preceding siblings ...)
2017-04-13 16:43 ` [PATCH v13 03/10] mux: minimal mux subsystem and gpio-based mux controller Peter Rosin
@ 2017-04-13 16:43 ` Peter Rosin
2017-04-13 16:43 ` [PATCH v13 05/10] dt-bindings: iio: io-channel-mux: document io-channel-mux bindings Peter Rosin
` (5 subsequent siblings)
9 siblings, 0 replies; 38+ messages in thread
From: Peter Rosin @ 2017-04-13 16:43 UTC (permalink / raw)
To: linux-kernel, Greg Kroah-Hartman
Cc: Peter Rosin, Wolfram Sang, Rob Herring, Mark Rutland,
Jonathan Cameron, Hartmut Knaack, Lars-Peter Clausen,
Peter Meerwald-Stadler, Jonathan Corbet, linux-i2c, devicetree,
linux-iio, linux-doc, Andrew Morton, Colin Ian King,
Paul Gortmaker, Philipp Zabel, kernel
Extend the inkern api with functions for reading and writing ext_info
of iio channels.
Acked-by: Jonathan Cameron <jic23@kernel.org>
Signed-off-by: Peter Rosin <peda@axentia.se>
---
drivers/iio/inkern.c | 60 ++++++++++++++++++++++++++++++++++++++++++++
include/linux/iio/consumer.h | 37 +++++++++++++++++++++++++++
2 files changed, 97 insertions(+)
diff --git a/drivers/iio/inkern.c b/drivers/iio/inkern.c
index 7a13535dc3e9..8292ad4435ea 100644
--- a/drivers/iio/inkern.c
+++ b/drivers/iio/inkern.c
@@ -869,3 +869,63 @@ int iio_write_channel_raw(struct iio_channel *chan, int val)
return ret;
}
EXPORT_SYMBOL_GPL(iio_write_channel_raw);
+
+unsigned int iio_get_channel_ext_info_count(struct iio_channel *chan)
+{
+ const struct iio_chan_spec_ext_info *ext_info;
+ unsigned int i = 0;
+
+ if (!chan->channel->ext_info)
+ return i;
+
+ for (ext_info = chan->channel->ext_info; ext_info->name; ext_info++)
+ ++i;
+
+ return i;
+}
+EXPORT_SYMBOL_GPL(iio_get_channel_ext_info_count);
+
+static const struct iio_chan_spec_ext_info *iio_lookup_ext_info(
+ const struct iio_channel *chan,
+ const char *attr)
+{
+ const struct iio_chan_spec_ext_info *ext_info;
+
+ if (!chan->channel->ext_info)
+ return NULL;
+
+ for (ext_info = chan->channel->ext_info; ext_info->name; ++ext_info) {
+ if (!strcmp(attr, ext_info->name))
+ return ext_info;
+ }
+
+ return NULL;
+}
+
+ssize_t iio_read_channel_ext_info(struct iio_channel *chan,
+ const char *attr, char *buf)
+{
+ const struct iio_chan_spec_ext_info *ext_info;
+
+ ext_info = iio_lookup_ext_info(chan, attr);
+ if (!ext_info)
+ return -EINVAL;
+
+ return ext_info->read(chan->indio_dev, ext_info->private,
+ chan->channel, buf);
+}
+EXPORT_SYMBOL_GPL(iio_read_channel_ext_info);
+
+ssize_t iio_write_channel_ext_info(struct iio_channel *chan, const char *attr,
+ const char *buf, size_t len)
+{
+ const struct iio_chan_spec_ext_info *ext_info;
+
+ ext_info = iio_lookup_ext_info(chan, attr);
+ if (!ext_info)
+ return -EINVAL;
+
+ return ext_info->write(chan->indio_dev, ext_info->private,
+ chan->channel, buf, len);
+}
+EXPORT_SYMBOL_GPL(iio_write_channel_ext_info);
diff --git a/include/linux/iio/consumer.h b/include/linux/iio/consumer.h
index 47eeec3218b5..5e347a9805fd 100644
--- a/include/linux/iio/consumer.h
+++ b/include/linux/iio/consumer.h
@@ -312,4 +312,41 @@ int iio_read_channel_scale(struct iio_channel *chan, int *val,
int iio_convert_raw_to_processed(struct iio_channel *chan, int raw,
int *processed, unsigned int scale);
+/**
+ * iio_get_channel_ext_info_count() - get number of ext_info attributes
+ * connected to the channel.
+ * @chan: The channel being queried
+ *
+ * Returns the number of ext_info attributes
+ */
+unsigned int iio_get_channel_ext_info_count(struct iio_channel *chan);
+
+/**
+ * iio_read_channel_ext_info() - read ext_info attribute from a given channel
+ * @chan: The channel being queried.
+ * @attr: The ext_info attribute to read.
+ * @buf: Where to store the attribute value. Assumed to hold
+ * at least PAGE_SIZE bytes.
+ *
+ * Returns the number of bytes written to buf (perhaps w/o zero termination;
+ * it need not even be a string), or an error code.
+ */
+ssize_t iio_read_channel_ext_info(struct iio_channel *chan,
+ const char *attr, char *buf);
+
+/**
+ * iio_write_channel_ext_info() - write ext_info attribute from a given channel
+ * @chan: The channel being queried.
+ * @attr: The ext_info attribute to read.
+ * @buf: The new attribute value. Strings needs to be zero-
+ * terminated, but the terminator should not be included
+ * in the below len.
+ * @len: The size of the new attribute value.
+ *
+ * Returns the number of accepted bytes, which should be the same as len.
+ * An error code can also be returned.
+ */
+ssize_t iio_write_channel_ext_info(struct iio_channel *chan, const char *attr,
+ const char *buf, size_t len);
+
#endif
--
2.1.4
^ permalink raw reply related [flat|nested] 38+ messages in thread* [PATCH v13 05/10] dt-bindings: iio: io-channel-mux: document io-channel-mux bindings
2017-04-13 16:43 [PATCH v13 00/10] mux controller abstraction and iio/i2c muxes Peter Rosin
` (3 preceding siblings ...)
2017-04-13 16:43 ` [PATCH v13 04/10] iio: inkern: api for manipulating ext_info of iio channels Peter Rosin
@ 2017-04-13 16:43 ` Peter Rosin
2017-04-13 16:43 ` [PATCH v13 06/10] iio: multiplexer: new iio category and iio-mux driver Peter Rosin
` (4 subsequent siblings)
9 siblings, 0 replies; 38+ messages in thread
From: Peter Rosin @ 2017-04-13 16:43 UTC (permalink / raw)
To: linux-kernel, Greg Kroah-Hartman
Cc: Peter Rosin, Wolfram Sang, Rob Herring, Mark Rutland,
Jonathan Cameron, Hartmut Knaack, Lars-Peter Clausen,
Peter Meerwald-Stadler, Jonathan Corbet, linux-i2c, devicetree,
linux-iio, linux-doc, Andrew Morton, Colin Ian King,
Paul Gortmaker, Philipp Zabel, kernel
Describe how a multiplexer can be used to select which signal is fed to
an io-channel.
Acked-by: Jonathan Cameron <jic23@kernel.org>
Acked-by: Rob Herring <robh@kernel.org>
Signed-off-by: Peter Rosin <peda@axentia.se>
---
.../bindings/iio/multiplexer/io-channel-mux.txt | 39 ++++++++++++++++++++++
MAINTAINERS | 6 ++++
2 files changed, 45 insertions(+)
create mode 100644 Documentation/devicetree/bindings/iio/multiplexer/io-channel-mux.txt
diff --git a/Documentation/devicetree/bindings/iio/multiplexer/io-channel-mux.txt b/Documentation/devicetree/bindings/iio/multiplexer/io-channel-mux.txt
new file mode 100644
index 000000000000..c82794002595
--- /dev/null
+++ b/Documentation/devicetree/bindings/iio/multiplexer/io-channel-mux.txt
@@ -0,0 +1,39 @@
+I/O channel multiplexer bindings
+
+If a multiplexer is used to select which hardware signal is fed to
+e.g. an ADC channel, these bindings describe that situation.
+
+Required properties:
+- compatible : "io-channel-mux"
+- io-channels : Channel node of the parent channel that has multiplexed
+ input.
+- io-channel-names : Should be "parent".
+- #address-cells = <1>;
+- #size-cells = <0>;
+- mux-controls : Mux controller node to use for operating the mux
+- channels : List of strings, labeling the mux controller states.
+
+For each non-empty string in the channels property, an io-channel will
+be created. The number of this io-channel is the same as the index into
+the list of strings in the channels property, and also matches the mux
+controller state. The mux controller state is described in
+../mux/mux-controller.txt
+
+Example:
+ mux: mux-controller {
+ compatible = "mux-gpio";
+ #mux-control-cells = <0>;
+
+ mux-gpios = <&pioA 0 GPIO_ACTIVE_HIGH>,
+ <&pioA 1 GPIO_ACTIVE_HIGH>;
+ };
+
+ adc-mux {
+ compatible = "io-channel-mux";
+ io-channels = <&adc 0>;
+ io-channel-names = "parent";
+
+ mux-controls = <&mux>;
+
+ channels = "sync", "in", "system-regulator";
+ };
diff --git a/MAINTAINERS b/MAINTAINERS
index 591eba737678..4af912305d2c 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -6360,6 +6360,12 @@ F: Documentation/ABI/testing/sysfs-bus-iio-adc-envelope-detector
F: Documentation/devicetree/bindings/iio/adc/envelope-detector.txt
F: drivers/iio/adc/envelope-detector.c
+IIO MULTIPLEXER
+M: Peter Rosin <peda@axentia.se>
+L: linux-iio@vger.kernel.org
+S: Maintained
+F: Documentation/devicetree/bindings/iio/multiplexer/iio-mux.txt
+
IIO SUBSYSTEM AND DRIVERS
M: Jonathan Cameron <jic23@kernel.org>
R: Hartmut Knaack <knaack.h@gmx.de>
--
2.1.4
^ permalink raw reply related [flat|nested] 38+ messages in thread* [PATCH v13 06/10] iio: multiplexer: new iio category and iio-mux driver
2017-04-13 16:43 [PATCH v13 00/10] mux controller abstraction and iio/i2c muxes Peter Rosin
` (4 preceding siblings ...)
2017-04-13 16:43 ` [PATCH v13 05/10] dt-bindings: iio: io-channel-mux: document io-channel-mux bindings Peter Rosin
@ 2017-04-13 16:43 ` Peter Rosin
2017-04-13 16:43 ` [PATCH v13 07/10] dt-bindings: i2c: i2c-mux: document general purpose i2c-mux bindings Peter Rosin
` (3 subsequent siblings)
9 siblings, 0 replies; 38+ messages in thread
From: Peter Rosin @ 2017-04-13 16:43 UTC (permalink / raw)
To: linux-kernel, Greg Kroah-Hartman
Cc: Peter Rosin, Wolfram Sang, Rob Herring, Mark Rutland,
Jonathan Cameron, Hartmut Knaack, Lars-Peter Clausen,
Peter Meerwald-Stadler, Jonathan Corbet, linux-i2c, devicetree,
linux-iio, linux-doc, Andrew Morton, Colin Ian King,
Paul Gortmaker, Philipp Zabel, kernel
When a multiplexer changes how an iio device behaves (for example
by feeding different signals to an ADC), this driver can be used
to create one virtual iio channel for each multiplexer state.
Depends on the generic multiplexer subsystem.
Cache any ext_info values from the parent iio channel, creating a private
copy of the ext_info attributes for each multiplexer state/channel.
Reviewed-by: Jonathan Cameron <jic23@kernel.org>
Signed-off-by: Peter Rosin <peda@axentia.se>
---
MAINTAINERS | 1 +
drivers/iio/Kconfig | 1 +
drivers/iio/Makefile | 1 +
drivers/iio/multiplexer/Kconfig | 18 ++
drivers/iio/multiplexer/Makefile | 6 +
drivers/iio/multiplexer/iio-mux.c | 459 ++++++++++++++++++++++++++++++++++++++
6 files changed, 486 insertions(+)
create mode 100644 drivers/iio/multiplexer/Kconfig
create mode 100644 drivers/iio/multiplexer/Makefile
create mode 100644 drivers/iio/multiplexer/iio-mux.c
diff --git a/MAINTAINERS b/MAINTAINERS
index 4af912305d2c..23cfd5bc2158 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -6365,6 +6365,7 @@ M: Peter Rosin <peda@axentia.se>
L: linux-iio@vger.kernel.org
S: Maintained
F: Documentation/devicetree/bindings/iio/multiplexer/iio-mux.txt
+F: drivers/iio/multiplexer/iio-mux.c
IIO SUBSYSTEM AND DRIVERS
M: Jonathan Cameron <jic23@kernel.org>
diff --git a/drivers/iio/Kconfig b/drivers/iio/Kconfig
index a918270d6f54..b3c8c6ef0dff 100644
--- a/drivers/iio/Kconfig
+++ b/drivers/iio/Kconfig
@@ -83,6 +83,7 @@ source "drivers/iio/humidity/Kconfig"
source "drivers/iio/imu/Kconfig"
source "drivers/iio/light/Kconfig"
source "drivers/iio/magnetometer/Kconfig"
+source "drivers/iio/multiplexer/Kconfig"
source "drivers/iio/orientation/Kconfig"
if IIO_TRIGGER
source "drivers/iio/trigger/Kconfig"
diff --git a/drivers/iio/Makefile b/drivers/iio/Makefile
index 33fa4026f92c..93c769cd99bf 100644
--- a/drivers/iio/Makefile
+++ b/drivers/iio/Makefile
@@ -28,6 +28,7 @@ obj-y += humidity/
obj-y += imu/
obj-y += light/
obj-y += magnetometer/
+obj-y += multiplexer/
obj-y += orientation/
obj-y += potentiometer/
obj-y += potentiostat/
diff --git a/drivers/iio/multiplexer/Kconfig b/drivers/iio/multiplexer/Kconfig
new file mode 100644
index 000000000000..70a044510686
--- /dev/null
+++ b/drivers/iio/multiplexer/Kconfig
@@ -0,0 +1,18 @@
+#
+# Multiplexer drivers
+#
+# When adding new entries keep the list in alphabetical order
+
+menu "Multiplexers"
+
+config IIO_MUX
+ tristate "IIO multiplexer driver"
+ select MULTIPLEXER
+ depends on OF
+ help
+ Say yes here to build support for the IIO multiplexer.
+
+ To compile this driver as a module, choose M here: the
+ module will be called iio-mux.
+
+endmenu
diff --git a/drivers/iio/multiplexer/Makefile b/drivers/iio/multiplexer/Makefile
new file mode 100644
index 000000000000..68be3c4abd07
--- /dev/null
+++ b/drivers/iio/multiplexer/Makefile
@@ -0,0 +1,6 @@
+#
+# Makefile for industrial I/O multiplexer drivers
+#
+
+# When adding new entries keep the list in alphabetical order
+obj-$(CONFIG_IIO_MUX) += iio-mux.o
diff --git a/drivers/iio/multiplexer/iio-mux.c b/drivers/iio/multiplexer/iio-mux.c
new file mode 100644
index 000000000000..bab9e6902090
--- /dev/null
+++ b/drivers/iio/multiplexer/iio-mux.c
@@ -0,0 +1,459 @@
+/*
+ * IIO multiplexer driver
+ *
+ * Copyright (C) 2017 Axentia Technologies AB
+ *
+ * Author: Peter Rosin <peda@axentia.se>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#include <linux/err.h>
+#include <linux/iio/consumer.h>
+#include <linux/iio/iio.h>
+#include <linux/module.h>
+#include <linux/mutex.h>
+#include <linux/mux.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+
+struct mux_ext_info_cache {
+ char *data;
+ ssize_t size;
+};
+
+struct mux_child {
+ struct mux_ext_info_cache *ext_info_cache;
+};
+
+struct mux {
+ int cached_state;
+ struct mux_control *control;
+ struct iio_channel *parent;
+ struct iio_dev *indio_dev;
+ struct iio_chan_spec *chan;
+ struct iio_chan_spec_ext_info *ext_info;
+ struct mux_child *child;
+};
+
+static int iio_mux_select(struct mux *mux, int idx)
+{
+ struct mux_child *child = &mux->child[idx];
+ struct iio_chan_spec const *chan = &mux->chan[idx];
+ int ret;
+ int i;
+
+ ret = mux_control_select(mux->control, chan->channel);
+ if (ret < 0) {
+ mux->cached_state = -1;
+ return ret;
+ }
+
+ if (mux->cached_state == chan->channel)
+ return 0;
+
+ if (chan->ext_info) {
+ for (i = 0; chan->ext_info[i].name; ++i) {
+ const char *attr = chan->ext_info[i].name;
+ struct mux_ext_info_cache *cache;
+
+ cache = &child->ext_info_cache[i];
+
+ if (cache->size < 0)
+ continue;
+
+ ret = iio_write_channel_ext_info(mux->parent, attr,
+ cache->data,
+ cache->size);
+
+ if (ret < 0) {
+ mux_control_deselect(mux->control);
+ mux->cached_state = -1;
+ return ret;
+ }
+ }
+ }
+ mux->cached_state = chan->channel;
+
+ return 0;
+}
+
+static void iio_mux_deselect(struct mux *mux)
+{
+ mux_control_deselect(mux->control);
+}
+
+static int mux_read_raw(struct iio_dev *indio_dev,
+ struct iio_chan_spec const *chan,
+ int *val, int *val2, long mask)
+{
+ struct mux *mux = iio_priv(indio_dev);
+ int idx = chan - mux->chan;
+ int ret;
+
+ ret = iio_mux_select(mux, idx);
+ if (ret < 0)
+ return ret;
+
+ switch (mask) {
+ case IIO_CHAN_INFO_RAW:
+ ret = iio_read_channel_raw(mux->parent, val);
+ break;
+
+ case IIO_CHAN_INFO_SCALE:
+ ret = iio_read_channel_scale(mux->parent, val, val2);
+ break;
+
+ default:
+ ret = -EINVAL;
+ }
+
+ iio_mux_deselect(mux);
+
+ return ret;
+}
+
+static int mux_read_avail(struct iio_dev *indio_dev,
+ struct iio_chan_spec const *chan,
+ const int **vals, int *type, int *length,
+ long mask)
+{
+ struct mux *mux = iio_priv(indio_dev);
+ int idx = chan - mux->chan;
+ int ret;
+
+ ret = iio_mux_select(mux, idx);
+ if (ret < 0)
+ return ret;
+
+ switch (mask) {
+ case IIO_CHAN_INFO_RAW:
+ *type = IIO_VAL_INT;
+ ret = iio_read_avail_channel_raw(mux->parent, vals, length);
+ break;
+
+ default:
+ ret = -EINVAL;
+ }
+
+ iio_mux_deselect(mux);
+
+ return ret;
+}
+
+static int mux_write_raw(struct iio_dev *indio_dev,
+ struct iio_chan_spec const *chan,
+ int val, int val2, long mask)
+{
+ struct mux *mux = iio_priv(indio_dev);
+ int idx = chan - mux->chan;
+ int ret;
+
+ ret = iio_mux_select(mux, idx);
+ if (ret < 0)
+ return ret;
+
+ switch (mask) {
+ case IIO_CHAN_INFO_RAW:
+ ret = iio_write_channel_raw(mux->parent, val);
+ break;
+
+ default:
+ ret = -EINVAL;
+ }
+
+ iio_mux_deselect(mux);
+
+ return ret;
+}
+
+static const struct iio_info mux_info = {
+ .read_raw = mux_read_raw,
+ .read_avail = mux_read_avail,
+ .write_raw = mux_write_raw,
+ .driver_module = THIS_MODULE,
+};
+
+static ssize_t mux_read_ext_info(struct iio_dev *indio_dev, uintptr_t private,
+ struct iio_chan_spec const *chan, char *buf)
+{
+ struct mux *mux = iio_priv(indio_dev);
+ int idx = chan - mux->chan;
+ ssize_t ret;
+
+ ret = iio_mux_select(mux, idx);
+ if (ret < 0)
+ return ret;
+
+ ret = iio_read_channel_ext_info(mux->parent,
+ mux->ext_info[private].name,
+ buf);
+
+ iio_mux_deselect(mux);
+
+ return ret;
+}
+
+static ssize_t mux_write_ext_info(struct iio_dev *indio_dev, uintptr_t private,
+ struct iio_chan_spec const *chan,
+ const char *buf, size_t len)
+{
+ struct device *dev = indio_dev->dev.parent;
+ struct mux *mux = iio_priv(indio_dev);
+ int idx = chan - mux->chan;
+ char *new;
+ ssize_t ret;
+
+ if (len >= PAGE_SIZE)
+ return -EINVAL;
+
+ ret = iio_mux_select(mux, idx);
+ if (ret < 0)
+ return ret;
+
+ new = devm_kmemdup(dev, buf, len + 1, GFP_KERNEL);
+ if (!new) {
+ iio_mux_deselect(mux);
+ return -ENOMEM;
+ }
+
+ new[len] = 0;
+
+ ret = iio_write_channel_ext_info(mux->parent,
+ mux->ext_info[private].name,
+ buf, len);
+ if (ret < 0) {
+ iio_mux_deselect(mux);
+ devm_kfree(dev, new);
+ return ret;
+ }
+
+ devm_kfree(dev, mux->child[idx].ext_info_cache[private].data);
+ mux->child[idx].ext_info_cache[private].data = new;
+ mux->child[idx].ext_info_cache[private].size = len;
+
+ iio_mux_deselect(mux);
+
+ return ret;
+}
+
+static int mux_configure_channel(struct device *dev, struct mux *mux,
+ u32 state, const char *label, int idx)
+{
+ struct mux_child *child = &mux->child[idx];
+ struct iio_chan_spec *chan = &mux->chan[idx];
+ struct iio_chan_spec const *pchan = mux->parent->channel;
+ char *page = NULL;
+ int num_ext_info;
+ int i;
+ int ret;
+
+ chan->indexed = 1;
+ chan->output = pchan->output;
+ chan->datasheet_name = label;
+ chan->ext_info = mux->ext_info;
+
+ ret = iio_get_channel_type(mux->parent, &chan->type);
+ if (ret < 0) {
+ dev_err(dev, "failed to get parent channel type\n");
+ return ret;
+ }
+
+ if (iio_channel_has_info(pchan, IIO_CHAN_INFO_RAW))
+ chan->info_mask_separate |= BIT(IIO_CHAN_INFO_RAW);
+ if (iio_channel_has_info(pchan, IIO_CHAN_INFO_SCALE))
+ chan->info_mask_separate |= BIT(IIO_CHAN_INFO_SCALE);
+
+ if (iio_channel_has_available(pchan, IIO_CHAN_INFO_RAW))
+ chan->info_mask_separate_available |= BIT(IIO_CHAN_INFO_RAW);
+
+ if (state >= mux->control->states) {
+ dev_err(dev, "too many channels\n");
+ return -EINVAL;
+ }
+
+ chan->channel = state;
+
+ num_ext_info = iio_get_channel_ext_info_count(mux->parent);
+ if (num_ext_info) {
+ page = devm_kzalloc(dev, PAGE_SIZE, GFP_KERNEL);
+ if (!page)
+ return -ENOMEM;
+ }
+ child->ext_info_cache = devm_kzalloc(dev,
+ sizeof(*child->ext_info_cache) *
+ num_ext_info, GFP_KERNEL);
+ for (i = 0; i < num_ext_info; ++i) {
+ child->ext_info_cache[i].size = -1;
+
+ if (!pchan->ext_info[i].write)
+ continue;
+ if (!pchan->ext_info[i].read)
+ continue;
+
+ ret = iio_read_channel_ext_info(mux->parent,
+ mux->ext_info[i].name,
+ page);
+ if (ret < 0) {
+ dev_err(dev, "failed to get ext_info '%s'\n",
+ pchan->ext_info[i].name);
+ return ret;
+ }
+ if (ret >= PAGE_SIZE) {
+ dev_err(dev, "too large ext_info '%s'\n",
+ pchan->ext_info[i].name);
+ return -EINVAL;
+ }
+
+ child->ext_info_cache[i].data = devm_kmemdup(dev, page, ret + 1,
+ GFP_KERNEL);
+ child->ext_info_cache[i].data[ret] = 0;
+ child->ext_info_cache[i].size = ret;
+ }
+
+ if (page)
+ devm_kfree(dev, page);
+
+ return 0;
+}
+
+/*
+ * Same as of_property_for_each_string(), but also keeps track of the
+ * index of each string.
+ */
+#define of_property_for_each_string_index(np, propname, prop, s, i) \
+ for (prop = of_find_property(np, propname, NULL), \
+ s = of_prop_next_string(prop, NULL), \
+ i = 0; \
+ s; \
+ s = of_prop_next_string(prop, s), \
+ i++)
+
+static int mux_probe(struct platform_device *pdev)
+{
+ struct device *dev = &pdev->dev;
+ struct device_node *np = pdev->dev.of_node;
+ struct iio_dev *indio_dev;
+ struct iio_channel *parent;
+ struct mux *mux;
+ struct property *prop;
+ const char *label;
+ u32 state;
+ int sizeof_ext_info;
+ int children;
+ int sizeof_priv;
+ int i;
+ int ret;
+
+ if (!np)
+ return -ENODEV;
+
+ parent = devm_iio_channel_get(dev, "parent");
+ if (IS_ERR(parent)) {
+ if (PTR_ERR(parent) != -EPROBE_DEFER)
+ dev_err(dev, "failed to get parent channel\n");
+ return PTR_ERR(parent);
+ }
+
+ sizeof_ext_info = iio_get_channel_ext_info_count(parent);
+ if (sizeof_ext_info) {
+ sizeof_ext_info += 1; /* one extra entry for the sentinel */
+ sizeof_ext_info *= sizeof(*mux->ext_info);
+ }
+
+ children = 0;
+ of_property_for_each_string(np, "channels", prop, label) {
+ if (*label)
+ children++;
+ }
+ if (children <= 0) {
+ dev_err(dev, "not even a single child\n");
+ return -EINVAL;
+ }
+
+ sizeof_priv = sizeof(*mux);
+ sizeof_priv += sizeof(*mux->child) * children;
+ sizeof_priv += sizeof(*mux->chan) * children;
+ sizeof_priv += sizeof_ext_info;
+
+ indio_dev = devm_iio_device_alloc(dev, sizeof_priv);
+ if (!indio_dev)
+ return -ENOMEM;
+
+ mux = iio_priv(indio_dev);
+ mux->child = (struct mux_child *)(mux + 1);
+ mux->chan = (struct iio_chan_spec *)(mux->child + children);
+
+ platform_set_drvdata(pdev, indio_dev);
+
+ mux->parent = parent;
+ mux->cached_state = -1;
+
+ indio_dev->name = dev_name(dev);
+ indio_dev->dev.parent = dev;
+ indio_dev->info = &mux_info;
+ indio_dev->modes = INDIO_DIRECT_MODE;
+ indio_dev->channels = mux->chan;
+ indio_dev->num_channels = children;
+ if (sizeof_ext_info) {
+ mux->ext_info = devm_kmemdup(dev,
+ parent->channel->ext_info,
+ sizeof_ext_info, GFP_KERNEL);
+ if (!mux->ext_info)
+ return -ENOMEM;
+
+ for (i = 0; mux->ext_info[i].name; ++i) {
+ if (parent->channel->ext_info[i].read)
+ mux->ext_info[i].read = mux_read_ext_info;
+ if (parent->channel->ext_info[i].write)
+ mux->ext_info[i].write = mux_write_ext_info;
+ mux->ext_info[i].private = i;
+ }
+ }
+
+ mux->control = devm_mux_control_get(dev, NULL);
+ if (IS_ERR(mux->control)) {
+ if (PTR_ERR(mux->control) != -EPROBE_DEFER)
+ dev_err(dev, "failed to get control-mux\n");
+ return PTR_ERR(mux->control);
+ }
+
+ i = 0;
+ of_property_for_each_string_index(np, "channels", prop, label, state) {
+ if (!*label)
+ continue;
+
+ ret = mux_configure_channel(dev, mux, state, label, i++);
+ if (ret < 0)
+ return ret;
+ }
+
+ ret = devm_iio_device_register(dev, indio_dev);
+ if (ret) {
+ dev_err(dev, "failed to register iio device\n");
+ return ret;
+ }
+
+ return 0;
+}
+
+static const struct of_device_id mux_match[] = {
+ { .compatible = "io-channel-mux" },
+ { /* sentinel */ }
+};
+MODULE_DEVICE_TABLE(of, mux_match);
+
+static struct platform_driver mux_driver = {
+ .probe = mux_probe,
+ .driver = {
+ .name = "iio-mux",
+ .of_match_table = mux_match,
+ },
+};
+module_platform_driver(mux_driver);
+
+MODULE_DESCRIPTION("IIO multiplexer driver");
+MODULE_AUTHOR("Peter Rosin <peda@axentia.se>");
+MODULE_LICENSE("GPL v2");
--
2.1.4
^ permalink raw reply related [flat|nested] 38+ messages in thread* [PATCH v13 07/10] dt-bindings: i2c: i2c-mux: document general purpose i2c-mux bindings
2017-04-13 16:43 [PATCH v13 00/10] mux controller abstraction and iio/i2c muxes Peter Rosin
` (5 preceding siblings ...)
2017-04-13 16:43 ` [PATCH v13 06/10] iio: multiplexer: new iio category and iio-mux driver Peter Rosin
@ 2017-04-13 16:43 ` Peter Rosin
2017-04-13 16:43 ` [PATCH v13 08/10] i2c: i2c-mux-gpmux: new driver Peter Rosin
` (2 subsequent siblings)
9 siblings, 0 replies; 38+ messages in thread
From: Peter Rosin @ 2017-04-13 16:43 UTC (permalink / raw)
To: linux-kernel, Greg Kroah-Hartman
Cc: Peter Rosin, Wolfram Sang, Rob Herring, Mark Rutland,
Jonathan Cameron, Hartmut Knaack, Lars-Peter Clausen,
Peter Meerwald-Stadler, Jonathan Corbet, linux-i2c, devicetree,
linux-iio, linux-doc, Andrew Morton, Colin Ian King,
Paul Gortmaker, Philipp Zabel, kernel
Describe how a general purpose multiplexer controller is used to mux an
i2c bus.
Acked-by: Jonathan Cameron <jic23@kernel.org>
Reviewed-by: Rob Herring <robh@kernel.org>
Signed-off-by: Peter Rosin <peda@axentia.se>
---
.../devicetree/bindings/i2c/i2c-mux-gpmux.txt | 99 ++++++++++++++++++++++
1 file changed, 99 insertions(+)
create mode 100644 Documentation/devicetree/bindings/i2c/i2c-mux-gpmux.txt
diff --git a/Documentation/devicetree/bindings/i2c/i2c-mux-gpmux.txt b/Documentation/devicetree/bindings/i2c/i2c-mux-gpmux.txt
new file mode 100644
index 000000000000..2907dab56298
--- /dev/null
+++ b/Documentation/devicetree/bindings/i2c/i2c-mux-gpmux.txt
@@ -0,0 +1,99 @@
+General Purpose I2C Bus Mux
+
+This binding describes an I2C bus multiplexer that uses a mux controller
+from the mux subsystem to route the I2C signals.
+
+ .-----. .-----.
+ | dev | | dev |
+ .------------. '-----' '-----'
+ | SoC | | |
+ | | .--------+--------'
+ | .------. | .------+ child bus A, on MUX value set to 0
+ | | I2C |-|--| Mux |
+ | '------' | '--+---+ child bus B, on MUX value set to 1
+ | .------. | | '----------+--------+--------.
+ | | MUX- | | | | | |
+ | | Ctrl |-|-----+ .-----. .-----. .-----.
+ | '------' | | dev | | dev | | dev |
+ '------------' '-----' '-----' '-----'
+
+Required properties:
+- compatible: i2c-mux
+- i2c-parent: The phandle of the I2C bus that this multiplexer's master-side
+ port is connected to.
+- mux-controls: The phandle of the mux controller to use for operating the
+ mux.
+* Standard I2C mux properties. See i2c-mux.txt in this directory.
+* I2C child bus nodes. See i2c-mux.txt in this directory. The sub-bus number
+ is also the mux-controller state described in ../mux/mux-controller.txt
+
+Optional properties:
+- mux-locked: If present, explicitly allow unrelated I2C transactions on the
+ parent I2C adapter at these times:
+ + during setup of the multiplexer
+ + between setup of the multiplexer and the child bus I2C transaction
+ + between the child bus I2C transaction and releasing of the multiplexer
+ + during releasing of the multiplexer
+ However, I2C transactions to devices behind all I2C multiplexers connected
+ to the same parent adapter that this multiplexer is connected to are blocked
+ for the full duration of the complete multiplexed I2C transaction (i.e.
+ including the times covered by the above list).
+ If mux-locked is not present, the multiplexer is assumed to be parent-locked.
+ This means that no unrelated I2C transactions are allowed on the parent I2C
+ adapter for the complete multiplexed I2C transaction.
+ The properties of mux-locked and parent-locked multiplexers are discussed
+ in more detail in Documentation/i2c/i2c-topology.
+
+For each i2c child node, an I2C child bus will be created. They will
+be numbered based on their order in the device tree.
+
+Whenever an access is made to a device on a child bus, the value set
+in the relevant node's reg property will be set as the state in the
+mux controller.
+
+Example:
+ mux: mux-controller {
+ compatible = "gpio-mux";
+ #mux-control-cells = <0>;
+
+ mux-gpios = <&pioA 0 GPIO_ACTIVE_HIGH>,
+ <&pioA 1 GPIO_ACTIVE_HIGH>;
+ };
+
+ i2c-mux {
+ compatible = "i2c-mux";
+ mux-locked;
+ i2c-parent = <&i2c1>;
+
+ mux-controls = <&mux>;
+
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ i2c@1 {
+ reg = <1>;
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ ssd1307: oled@3c {
+ compatible = "solomon,ssd1307fb-i2c";
+ reg = <0x3c>;
+ pwms = <&pwm 4 3000>;
+ reset-gpios = <&gpio2 7 1>;
+ reset-active-low;
+ };
+ };
+
+ i2c@3 {
+ reg = <3>;
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ pca9555: pca9555@20 {
+ compatible = "nxp,pca9555";
+ gpio-controller;
+ #gpio-cells = <2>;
+ reg = <0x20>;
+ };
+ };
+ };
--
2.1.4
^ permalink raw reply related [flat|nested] 38+ messages in thread* [PATCH v13 08/10] i2c: i2c-mux-gpmux: new driver
2017-04-13 16:43 [PATCH v13 00/10] mux controller abstraction and iio/i2c muxes Peter Rosin
` (6 preceding siblings ...)
2017-04-13 16:43 ` [PATCH v13 07/10] dt-bindings: i2c: i2c-mux: document general purpose i2c-mux bindings Peter Rosin
@ 2017-04-13 16:43 ` Peter Rosin
2017-04-13 16:43 ` [PATCH v13 09/10] dt-bindings: mux-adg792a: document devicetree bindings for ADG792A/G mux Peter Rosin
2017-04-13 16:43 ` [PATCH v13 10/10] mux: adg792a: add mux controller driver for ADG792A/G Peter Rosin
9 siblings, 0 replies; 38+ messages in thread
From: Peter Rosin @ 2017-04-13 16:43 UTC (permalink / raw)
To: linux-kernel, Greg Kroah-Hartman
Cc: Peter Rosin, Wolfram Sang, Rob Herring, Mark Rutland,
Jonathan Cameron, Hartmut Knaack, Lars-Peter Clausen,
Peter Meerwald-Stadler, Jonathan Corbet, linux-i2c, devicetree,
linux-iio, linux-doc, Andrew Morton, Colin Ian King,
Paul Gortmaker, Philipp Zabel, kernel
This is a general purpose i2c mux that uses a multiplexer controlled by
the multiplexer subsystem to do the muxing.
The user can select if the mux is to be mux-locked and parent-locked
as described in Documentation/i2c/i2c-topology.
Acked-by: Jonathan Cameron <jic23@kernel.org>
Acked-by: Wolfram Sang <wsa@the-dreams.de>
Signed-off-by: Peter Rosin <peda@axentia.se>
---
drivers/i2c/muxes/Kconfig | 13 +++
drivers/i2c/muxes/Makefile | 1 +
drivers/i2c/muxes/i2c-mux-gpmux.c | 173 ++++++++++++++++++++++++++++++++++++++
3 files changed, 187 insertions(+)
create mode 100644 drivers/i2c/muxes/i2c-mux-gpmux.c
diff --git a/drivers/i2c/muxes/Kconfig b/drivers/i2c/muxes/Kconfig
index 10b3d17ae3ea..11115fb34f24 100644
--- a/drivers/i2c/muxes/Kconfig
+++ b/drivers/i2c/muxes/Kconfig
@@ -30,6 +30,19 @@ config I2C_MUX_GPIO
This driver can also be built as a module. If so, the module
will be called i2c-mux-gpio.
+config I2C_MUX_GPMUX
+ tristate "General Purpose I2C multiplexer"
+ select MULTIPLEXER
+ depends on OF
+ help
+ If you say yes to this option, support will be included for a
+ general purpose I2C multiplexer. This driver provides access to
+ I2C busses connected through a MUX, which in turn is controlled
+ by a MUX-controller from the MUX subsystem.
+
+ This driver can also be built as a module. If so, the module
+ will be called i2c-mux-gpmux.
+
config I2C_MUX_PCA9541
tristate "NXP PCA9541 I2C Master Selector"
help
diff --git a/drivers/i2c/muxes/Makefile b/drivers/i2c/muxes/Makefile
index 9948fa45037f..af43c6c3e861 100644
--- a/drivers/i2c/muxes/Makefile
+++ b/drivers/i2c/muxes/Makefile
@@ -6,6 +6,7 @@ obj-$(CONFIG_I2C_ARB_GPIO_CHALLENGE) += i2c-arb-gpio-challenge.o
obj-$(CONFIG_I2C_DEMUX_PINCTRL) += i2c-demux-pinctrl.o
obj-$(CONFIG_I2C_MUX_GPIO) += i2c-mux-gpio.o
+obj-$(CONFIG_I2C_MUX_GPMUX) += i2c-mux-gpmux.o
obj-$(CONFIG_I2C_MUX_MLXCPLD) += i2c-mux-mlxcpld.o
obj-$(CONFIG_I2C_MUX_PCA9541) += i2c-mux-pca9541.o
obj-$(CONFIG_I2C_MUX_PCA954x) += i2c-mux-pca954x.o
diff --git a/drivers/i2c/muxes/i2c-mux-gpmux.c b/drivers/i2c/muxes/i2c-mux-gpmux.c
new file mode 100644
index 000000000000..fb23b2278462
--- /dev/null
+++ b/drivers/i2c/muxes/i2c-mux-gpmux.c
@@ -0,0 +1,173 @@
+/*
+ * General Purpose I2C multiplexer
+ *
+ * Copyright (C) 2017 Axentia Technologies AB
+ *
+ * Author: Peter Rosin <peda@axentia.se>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#include <linux/i2c.h>
+#include <linux/i2c-mux.h>
+#include <linux/module.h>
+#include <linux/mux.h>
+#include <linux/of_device.h>
+#include <linux/platform_device.h>
+
+struct mux {
+ struct mux_control *control;
+
+ bool do_not_deselect;
+};
+
+static int i2c_mux_select(struct i2c_mux_core *muxc, u32 chan)
+{
+ struct mux *mux = i2c_mux_priv(muxc);
+ int ret;
+
+ ret = mux_control_select(mux->control, chan);
+ mux->do_not_deselect = ret < 0;
+
+ return ret;
+}
+
+static int i2c_mux_deselect(struct i2c_mux_core *muxc, u32 chan)
+{
+ struct mux *mux = i2c_mux_priv(muxc);
+
+ if (mux->do_not_deselect)
+ return 0;
+
+ return mux_control_deselect(mux->control);
+}
+
+static struct i2c_adapter *mux_parent_adapter(struct device *dev)
+{
+ struct device_node *np = dev->of_node;
+ struct device_node *parent_np;
+ struct i2c_adapter *parent;
+
+ parent_np = of_parse_phandle(np, "i2c-parent", 0);
+ if (!parent_np) {
+ dev_err(dev, "Cannot parse i2c-parent\n");
+ return ERR_PTR(-ENODEV);
+ }
+ parent = of_find_i2c_adapter_by_node(parent_np);
+ of_node_put(parent_np);
+ if (!parent)
+ return ERR_PTR(-EPROBE_DEFER);
+
+ return parent;
+}
+
+static const struct of_device_id i2c_mux_of_match[] = {
+ { .compatible = "i2c-mux", },
+ {},
+};
+MODULE_DEVICE_TABLE(of, i2c_mux_of_match);
+
+static int i2c_mux_probe(struct platform_device *pdev)
+{
+ struct device *dev = &pdev->dev;
+ struct device_node *np = dev->of_node;
+ struct device_node *child;
+ struct i2c_mux_core *muxc;
+ struct mux *mux;
+ struct i2c_adapter *parent;
+ int children;
+ int ret;
+
+ if (!np)
+ return -ENODEV;
+
+ mux = devm_kzalloc(dev, sizeof(*mux), GFP_KERNEL);
+ if (!mux)
+ return -ENOMEM;
+
+ mux->control = devm_mux_control_get(dev, NULL);
+ if (IS_ERR(mux->control)) {
+ if (PTR_ERR(mux->control) != -EPROBE_DEFER)
+ dev_err(dev, "failed to get control-mux\n");
+ return PTR_ERR(mux->control);
+ }
+
+ parent = mux_parent_adapter(dev);
+ if (IS_ERR(parent)) {
+ if (PTR_ERR(parent) != -EPROBE_DEFER)
+ dev_err(dev, "failed to get i2c-parent adapter\n");
+ return PTR_ERR(parent);
+ }
+
+ children = of_get_child_count(np);
+
+ muxc = i2c_mux_alloc(parent, dev, children, 0, 0,
+ i2c_mux_select, i2c_mux_deselect);
+ if (!muxc) {
+ ret = -ENOMEM;
+ goto err_parent;
+ }
+ muxc->priv = mux;
+
+ platform_set_drvdata(pdev, muxc);
+
+ muxc->mux_locked = of_property_read_bool(np, "mux-locked");
+
+ for_each_child_of_node(np, child) {
+ u32 chan;
+
+ ret = of_property_read_u32(child, "reg", &chan);
+ if (ret < 0) {
+ dev_err(dev, "no reg property for node '%s'\n",
+ child->name);
+ goto err_children;
+ }
+
+ if (chan >= mux->control->states) {
+ dev_err(dev, "invalid reg %u\n", chan);
+ ret = -EINVAL;
+ goto err_children;
+ }
+
+ ret = i2c_mux_add_adapter(muxc, 0, chan, 0);
+ if (ret)
+ goto err_children;
+ }
+
+ dev_info(dev, "%d-port mux on %s adapter\n", children, parent->name);
+
+ return 0;
+
+err_children:
+ i2c_mux_del_adapters(muxc);
+err_parent:
+ i2c_put_adapter(parent);
+
+ return ret;
+}
+
+static int i2c_mux_remove(struct platform_device *pdev)
+{
+ struct i2c_mux_core *muxc = platform_get_drvdata(pdev);
+
+ i2c_mux_del_adapters(muxc);
+ i2c_put_adapter(muxc->parent);
+
+ return 0;
+}
+
+static struct platform_driver i2c_mux_driver = {
+ .probe = i2c_mux_probe,
+ .remove = i2c_mux_remove,
+ .driver = {
+ .name = "i2c-mux-gpmux",
+ .of_match_table = i2c_mux_of_match,
+ },
+};
+module_platform_driver(i2c_mux_driver);
+
+MODULE_DESCRIPTION("General Purpose I2C multiplexer driver");
+MODULE_AUTHOR("Peter Rosin <peda@axentia.se>");
+MODULE_LICENSE("GPL v2");
--
2.1.4
^ permalink raw reply related [flat|nested] 38+ messages in thread* [PATCH v13 09/10] dt-bindings: mux-adg792a: document devicetree bindings for ADG792A/G mux
2017-04-13 16:43 [PATCH v13 00/10] mux controller abstraction and iio/i2c muxes Peter Rosin
` (7 preceding siblings ...)
2017-04-13 16:43 ` [PATCH v13 08/10] i2c: i2c-mux-gpmux: new driver Peter Rosin
@ 2017-04-13 16:43 ` Peter Rosin
2017-04-13 16:43 ` [PATCH v13 10/10] mux: adg792a: add mux controller driver for ADG792A/G Peter Rosin
9 siblings, 0 replies; 38+ messages in thread
From: Peter Rosin @ 2017-04-13 16:43 UTC (permalink / raw)
To: linux-kernel, Greg Kroah-Hartman
Cc: Peter Rosin, Wolfram Sang, Rob Herring, Mark Rutland,
Jonathan Cameron, Hartmut Knaack, Lars-Peter Clausen,
Peter Meerwald-Stadler, Jonathan Corbet, linux-i2c, devicetree,
linux-iio, linux-doc, Andrew Morton, Colin Ian King,
Paul Gortmaker, Philipp Zabel, kernel
Analog Devices ADG792A/G is a triple 4:1 mux.
Acked-by: Jonathan Cameron <jic23@kernel.org>
Reviewed-by: Rob Herring <robh@kernel.org>
Signed-off-by: Peter Rosin <peda@axentia.se>
---
.../devicetree/bindings/mux/adi,adg792a.txt | 75 ++++++++++++++++++++++
1 file changed, 75 insertions(+)
create mode 100644 Documentation/devicetree/bindings/mux/adi,adg792a.txt
diff --git a/Documentation/devicetree/bindings/mux/adi,adg792a.txt b/Documentation/devicetree/bindings/mux/adi,adg792a.txt
new file mode 100644
index 000000000000..96b787a69f50
--- /dev/null
+++ b/Documentation/devicetree/bindings/mux/adi,adg792a.txt
@@ -0,0 +1,75 @@
+Bindings for Analog Devices ADG792A/G Triple 4:1 Multiplexers
+
+Required properties:
+- compatible : "adi,adg792a" or "adi,adg792g"
+- #mux-control-cells : <0> if parallel (the three muxes are bound together
+ with a single mux controller controlling all three muxes), or <1> if
+ not (one mux controller for each mux).
+* Standard mux-controller bindings as described in mux-controller.txt
+
+Optional properties for ADG792G:
+- gpio-controller : if present, #gpio-cells below is required.
+- #gpio-cells : should be <2>
+ - First cell is the GPO line number, i.e. 0 or 1
+ - Second cell is used to specify active high (0)
+ or active low (1)
+
+Optional properties:
+- idle-state : if present, array of states that the mux controllers will have
+ when idle. The special state MUX_IDLE_AS_IS is the default and
+ MUX_IDLE_DISCONNECT is also supported.
+
+States 0 through 3 correspond to signals A through D in the datasheet.
+
+Example:
+
+ /*
+ * Three independent mux controllers (of which one is used).
+ * Mux 0 is disconnected when idle, mux 1 idles in the previously
+ * selected state and mux 2 idles with signal B.
+ */
+ &i2c0 {
+ mux: mux-controller@50 {
+ compatible = "adi,adg792a";
+ reg = <0x50>;
+ #mux-control-cells = <1>;
+
+ idle-state = <MUX_IDLE_DISCONNECT MUX_IDLE_AS_IS 1>;
+ };
+ };
+
+ adc-mux {
+ compatible = "io-channel-mux";
+ io-channels = <&adc 0>;
+ io-channel-names = "parent";
+
+ mux-controls = <&mux 2>;
+
+ channels = "sync-1", "", "out";
+ };
+
+
+ /*
+ * Three parallel muxes with one mux controller, useful e.g. if
+ * the adc is differential, thus needing two signals to be muxed
+ * simultaneously for correct operation.
+ */
+ &i2c0 {
+ pmux: mux-controller@50 {
+ compatible = "adi,adg792a";
+ reg = <0x50>;
+ #mux-control-cells = <0>;
+
+ idle-state = <1>;
+ };
+ };
+
+ diff-adc-mux {
+ compatible = "io-channel-mux";
+ io-channels = <&adc 0>;
+ io-channel-names = "parent";
+
+ mux-controls = <&pmux>;
+
+ channels = "sync-1", "", "out";
+ };
--
2.1.4
^ permalink raw reply related [flat|nested] 38+ messages in thread* [PATCH v13 10/10] mux: adg792a: add mux controller driver for ADG792A/G
2017-04-13 16:43 [PATCH v13 00/10] mux controller abstraction and iio/i2c muxes Peter Rosin
` (8 preceding siblings ...)
2017-04-13 16:43 ` [PATCH v13 09/10] dt-bindings: mux-adg792a: document devicetree bindings for ADG792A/G mux Peter Rosin
@ 2017-04-13 16:43 ` Peter Rosin
9 siblings, 0 replies; 38+ messages in thread
From: Peter Rosin @ 2017-04-13 16:43 UTC (permalink / raw)
To: linux-kernel, Greg Kroah-Hartman
Cc: Peter Rosin, Wolfram Sang, Rob Herring, Mark Rutland,
Jonathan Cameron, Hartmut Knaack, Lars-Peter Clausen,
Peter Meerwald-Stadler, Jonathan Corbet, linux-i2c, devicetree,
linux-iio, linux-doc, Andrew Morton, Colin Ian King,
Paul Gortmaker, Philipp Zabel, kernel
Analog Devices ADG792A/G is a triple 4:1 mux.
Reviewed-by: Jonathan Cameron <jic23@kernel.org>
Signed-off-by: Peter Rosin <peda@axentia.se>
---
drivers/mux/Kconfig | 12 ++++
drivers/mux/Makefile | 1 +
drivers/mux/mux-adg792a.c | 141 ++++++++++++++++++++++++++++++++++++++++++++++
3 files changed, 154 insertions(+)
create mode 100644 drivers/mux/mux-adg792a.c
diff --git a/drivers/mux/Kconfig b/drivers/mux/Kconfig
index 41dfe08ead84..86668b4d2fc5 100644
--- a/drivers/mux/Kconfig
+++ b/drivers/mux/Kconfig
@@ -17,6 +17,18 @@ menuconfig MULTIPLEXER
if MULTIPLEXER
+config MUX_ADG792A
+ tristate "Analog Devices ADG792A/ADG792G Multiplexers"
+ depends on I2C
+ help
+ ADG792A and ADG792G Wide Bandwidth Triple 4:1 Multiplexers
+
+ The driver supports both operating the three multiplexers in
+ parallel and operating them independently.
+
+ To compile the driver as a module, choose M here: the module will
+ be called mux-adg792a.
+
config MUX_GPIO
tristate "GPIO-controlled Multiplexer"
depends on OF && GPIOLIB
diff --git a/drivers/mux/Makefile b/drivers/mux/Makefile
index bb16953f6290..b00a7d37d2fb 100644
--- a/drivers/mux/Makefile
+++ b/drivers/mux/Makefile
@@ -3,4 +3,5 @@
#
obj-$(CONFIG_MULTIPLEXER) += mux-core.o
+obj-$(CONFIG_MUX_ADG792A) += mux-adg792a.o
obj-$(CONFIG_MUX_GPIO) += mux-gpio.o
diff --git a/drivers/mux/mux-adg792a.c b/drivers/mux/mux-adg792a.c
new file mode 100644
index 000000000000..58c0ecf49a4a
--- /dev/null
+++ b/drivers/mux/mux-adg792a.c
@@ -0,0 +1,141 @@
+/*
+ * Multiplexer driver for Analog Devices ADG792A/G Triple 4:1 mux
+ *
+ * Copyright (C) 2017 Axentia Technologies AB
+ *
+ * Author: Peter Rosin <peda@axentia.se>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#include <linux/err.h>
+#include <linux/i2c.h>
+#include <linux/module.h>
+#include <linux/mux.h>
+
+#define ADG792A_LDSW BIT(0)
+#define ADG792A_RESETB BIT(1) /* active low, reset when zero */
+#define ADG792A_DISABLE(mux) (0x50 | (mux))
+#define ADG792A_DISABLE_ALL (0x5f)
+#define ADG792A_MUX(mux, state) (0xc0 | (((mux) + 1) << 2) | (state))
+#define ADG792A_MUX_ALL(state) (0xc0 | (state))
+
+static int adg792a_set(struct mux_control *mux, int state)
+{
+ struct i2c_client *i2c = to_i2c_client(mux->chip->dev.parent);
+ u8 cmd;
+
+ if (mux->chip->controllers == 1) {
+ /* parallel mux controller operation */
+ if (state == MUX_IDLE_DISCONNECT)
+ cmd = ADG792A_DISABLE_ALL;
+ else
+ cmd = ADG792A_MUX_ALL(state);
+ } else {
+ unsigned int controller = mux_control_get_index(mux);
+
+ if (state == MUX_IDLE_DISCONNECT)
+ cmd = ADG792A_DISABLE(controller);
+ else
+ cmd = ADG792A_MUX(controller, state);
+ }
+
+ return i2c_smbus_write_byte_data(i2c, cmd,
+ ADG792A_RESETB | ADG792A_LDSW);
+}
+
+static const struct mux_control_ops adg792a_ops = {
+ .set = adg792a_set,
+};
+
+static int adg792a_probe(struct i2c_client *i2c,
+ const struct i2c_device_id *id)
+{
+ struct device *dev = &i2c->dev;
+ struct mux_chip *mux_chip;
+ u32 cells;
+ int ret;
+ int i;
+
+ ret = of_property_read_u32(dev->of_node, "#mux-control-cells", &cells);
+ if (ret < 0)
+ return ret;
+ if (cells >= 2)
+ return -EINVAL;
+
+ mux_chip = devm_mux_chip_alloc(dev, cells ? 3 : 1, 0);
+ if (!mux_chip)
+ return -ENOMEM;
+
+ mux_chip->ops = &adg792a_ops;
+
+ ret = i2c_smbus_write_byte_data(i2c, ADG792A_DISABLE_ALL,
+ ADG792A_LDSW);
+ if (ret < 0)
+ return ret;
+
+ for (i = 0; i < mux_chip->controllers; ++i) {
+ struct mux_control *mux = &mux_chip->mux[i];
+ s32 idle_state;
+
+ mux->states = 4;
+
+ ret = of_property_read_u32_index(dev->of_node, "idle-state", i,
+ (s32 *)&idle_state);
+ if (ret < 0)
+ continue;
+
+ switch (idle_state) {
+ case 0 ... 4:
+ case MUX_IDLE_DISCONNECT:
+ mux_chip->mux[i].idle_state = idle_state;
+ break;
+ case MUX_IDLE_AS_IS:
+ break;
+ default:
+ dev_err(dev, "invalid idle-state %d\n", idle_state);
+ return -EINVAL;
+ }
+ }
+
+ ret = devm_mux_chip_register(dev, mux_chip);
+ if (ret < 0)
+ return ret;
+
+ if (cells)
+ dev_info(dev, "3x single pole quadruple throw muxes registered\n");
+ else
+ dev_info(dev, "triple pole quadruple throw mux registered\n");
+
+ return 0;
+}
+
+static const struct i2c_device_id adg792a_id[] = {
+ { .name = "adg792a", },
+ { .name = "adg792g", },
+ { }
+};
+MODULE_DEVICE_TABLE(i2c, adg792a_id);
+
+static const struct of_device_id adg792a_of_match[] = {
+ { .compatible = "adi,adg792a", },
+ { .compatible = "adi,adg792g", },
+ { }
+};
+MODULE_DEVICE_TABLE(of, adg792a_of_match);
+
+static struct i2c_driver adg792a_driver = {
+ .driver = {
+ .name = "adg792a",
+ .of_match_table = of_match_ptr(adg792a_of_match),
+ },
+ .probe = adg792a_probe,
+ .id_table = adg792a_id,
+};
+module_i2c_driver(adg792a_driver);
+
+MODULE_DESCRIPTION("Analog Devices ADG792A/G Triple 4:1 mux driver");
+MODULE_AUTHOR("Peter Rosin <peda@axentia.se>");
+MODULE_LICENSE("GPL v2");
--
2.1.4
^ permalink raw reply related [flat|nested] 38+ messages in thread