linux-kernel - RE: [PATCH] drivers: create a pinmux subsystem v2

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Tue, 17 May 2011 14:48:53 -0700
From:	Stephen Warren <swarren@...dia.com>
To:	Linus Walleij <linus.walleij@...aro.org>
CC:	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"linux-arm-kernel@...ts.infradead.org" 
	<linux-arm-kernel@...ts.infradead.org>,
	Martin Persson <martin.persson@...ricsson.com>,
	Joe Perches <joe@...ches.com>,
	Lee Jones <lee.jones@...aro.org>,
	Russell King <linux@....linux.org.uk>
Subject: RE: [PATCH] drivers: create a pinmux subsystem v2

OK, I think I get it now.

Just to make sure, let me augment your simple driver example from
Documentation/pinmux.txt in your patch for a hypothetical machine that
has a bus that can be 2, 4, or 8-bits in size:

static unsigned int spi0_0_pins[] = { 0, 8, 16, 24 };
static unsigned int i2c0_pins[] = { 24, 25 };
static unsigned int spi0_1_pins[] = { 38, 46, 54, 62 };
static unsigned int bus0_1_0_pins[] = { 1, 2, };
static unsigned int bus0_3_2_pins[] = { 3, 4, };
static unsigned int bus0_7_4_pins[] = { 5, 6, 7, 9 };

static struct foo_pmx_func myfuncs[] = {
	{
		.name = "spi0-0",
		.pins = spi0_0_pins,
		.num_pins = ARRAY_SIZE(spi0_1_pins),
	},
	{
		.name = "i2c0",
		.pins = i2c0_pins,
		.num_pins = ARRAY_SIZE(i2c0_pins),
	},
	{
		.name = "spi0-1",
		.pins = spi0_1_pins,
		.num_pins = ARRAY_SIZE(spi0_1_pins),
	},
	{
		.name = "bus0-1:0",
		.pins = bus0_1_0_pins,
		.num_pins = ARRAY_SIZE(bus0_1_0_pins),
	},
	{
		.name = "bus0-3:2",
		.pins = bus0_3_2_pins,
		.num_pins = ARRAY_SIZE(bus0_3_2_pins),
	},
	{
		.name = "bus0-7:4",
		.pins = bus0_7_4_pins,
		.num_pins = ARRAY_SIZE(bus0_7_4_pins),
	},
};

Now, some driver wishing to use those functions could pinmux_get() on:

* Just "bus0-1:0"
* Both "bus0-1:0" and "bus0-3:2"
* All of "bus0-1:0" and "bus0-3:2" and "bus0-7:4"

That all makes sense to me.

It'd be great if Documentation/pinmux.txt could spell out the "variable
sized bus" scenario above in a little more detail; it looks like at
least 2 or 3 people had similar questions regarding the explosion of
function definitions based on cross-products of configurations, all
I assume driven by the assumption there was a 1:1 mapping between device
and pinmux function, rather than 1:n.

The one remaining thing I don't understand:

The board provides a mapping from driver name to pinmux selections.
The example documentation includes:

+static struct pinmux_map pmx_mapping[] = {
+	{
+		.function = "spi0-1",
+		.dev_name = "foo-spi.0",
+	},
+	{
+		.function = "i2c0",
+		.dev_name = "foo-i2c.0",
+	},
+};

I don't think this fits the model of a driver needing to pinmux_get
e.g. 1, 2, or 3 pinmux functions. Rather, I think the .function field
should be an array of functions needed by the driver:

+static struct pinmux_map pmx_mapping[] = {
+	{
+		.functions = "spi0-1",
+		.dev_name = "foo-spi.0",
+	},
+	{
+		.function = "i2c0",
+		.dev_name = "foo-i2c.0",
+	},
+	{
+		.function = {"bus0-7:4", "bus0-3:2", "bus0-1:0"},
+		.dev_name = "foo-bus.0",
+	},
+};

(obviously that syntax is bogus, but you get the idea)

The intent being that the driver could perform single pinmux_get call
for "foo-bus.0" and then the subsequent pinmux_enable would enable all 3
entries in that device's .function array.

Now, you could argue that there be 3 entries in pmx_mapping[] above, each
with a different .dev_name, and each mapping to 1 of the 3 required
functions. However, since pinmux_get takes a struct device and extracts
the name from there, that wouldn't work.

As further justification for this, consider the following scenario,
which I imagine is pretty common in some incarnation:

Manufacturer creates an SoC. There are two packaging variants of this,
with just packaging and pinmuxing variations; all the SPI/I2C/foo-bus
modules are identical. Thus the driver is the same. The pinmux
differences extend to:

Package A: bus0 is split two ways for bits 7:4 and 3:0

Package B: bus0 is split the three ways from my previous example; 7:4,
3:2, 1:0.

In order to isolate the bus0 driver from this pure packaging difference,
The board's pinmux_map array above would be either:

Package A:

+	{
+		.function = {"bus0-7:4", "bus0-3:0"},
+		.dev_name = "foo-bus.0",
+	},

Package B:

+	{
+		.function = {"bus0-7:4", "bus0-3:2", "bus0-1:0"},
+		.dev_name = "foo-bus.0",
+	},

Does this seem reasonable?

Thanks for patiently answering my long-winded questions:-)

-- 
nvpublic

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/