linux-kernel - Re: RE : [PATCH 0/4] Pinmux subsystem

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 for Android: free password hash cracker in your pocket
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date:	Fri, 13 May 2011 18:03:08 +0200
From:	Matthieu CASTET <matthieu.castet@...rot.com>
To:	Linus Walleij <linus.walleij@...aro.org>
CC:	Sascha Hauer <s.hauer@...gutronix.de>,
	Grant Likely <grant.likely@...retlab.ca>,
	Martin Persson <martin.persson@...ricsson.com>,
	Lee Jones <lee.jones@...aro.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"linux-arm-kernel@...ts.infradead.org" 
	<linux-arm-kernel@...ts.infradead.org>
Subject: Re: RE : [PATCH 0/4] Pinmux subsystem

Linus Walleij a écrit :
> 2011/5/12 Matthieu Castet <matthieu.castet@...rot.com>:
> 
>> Enumerating all possible case will be impossible because of the number of possible cases
>> (and hardware guys can be very creative).
> 
> As mentioned in the document the subsystem is not about discrete
> mathematics, i.e
> we have no intent on enumerating every possible mux setting, only the ones that
> are relevant for your electronics at hand.
> 
>> If spi can be in  { A8, A7, A6, A5 } and
>> { G4, G3, G2, G1 }, Then you can output the spi on :
>> - { A8, A7, A6, A5 }
>> - { A8, A7, A6, G1 }
>> - { A8, A7, G2, A5 }
>> [...]
>> - { G4, G3, G2, A5 }
>> - { G4, G3, G2, G1 }
>> You have 2^4 = 16 cases
> 
> Do you use all of them in practice?
Of course not all, but more than the 2 { A8, A7, A6, A5 } or { G4, G3, G2, G1 }.

Also when we doing the bsp for a processor, it is better to allow flexibility
for future board.

So with your pin mux how to you handle the fact that on spi you can have some
signal not connected ?
For { A8, A7, A6, A5 } spi, some board want all 4 spi wire, other want 3 (CS,
MOSI, CLK) or (MISO, CS, CLK), other want 2.

This is board specific not package specific.

And that's doesn't apply only to spi. That's the same problem for uart (no
rts/cts), sdcard (one data vs 4 data), ...



> 
>> Pin muxing is really board specific  and shouldn't be in a "generic" driver.
> 
> It is rather package (PGA/BGA etc) specific than board specific. The board
> is about what of the packaging options you actually use. As
> mentioned in previous discussions you can pass in the actual configuration
> of the mux settings from platform data, if we have device tree we can let the
> board file dts inherit a package descriptor. All of this outside the
> kernel tree.
> 
> So we define the function groups for the package that will actually be used
> by the devices in the board files that we have.
> 
> And my first assumption is that those really aren't that many, and my second
> assumption is that you would still have to have board-specific code to handle
> every individual pin somewhere under mach-xxxx and this is what we're
> currently trying to get away from.
> 
>> But what you could abstract is a way to select a configuration of a pin,
>> not a group of pin for the board files.
> 
> The groups of pins are used when you're muxing devices, usually these use
> more than one pin. And that is why we connect them to the devices
> themselves with a mapping.
I believe there should be 2 different things :
- something for select pin. Omap stuff is nice : omap3_mux_init,
omap_mux_init_gpio, omap_mux_init_signal, ...
- something for grouping pins, but the board can add new group of pin if it
doesn't exist.


Also what i don't like in your system is the naming :
> +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 };
What's 0, 8, 16, .... It should be define.

> +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),
> +       },
> +};
How I am supposed to know what's spi0-0, i2c0, spi0-1 without reading the code ?

> +foo_probe()
> +{
> +       /* Allocate a state holder named "state" etc */
> +       struct pinmux pmx;
> +
> +       pmx = pinmux_get(&device, NULL);
> +       if IS_ERR(pmx)
> +               return PTR_ERR(pmx);
> +       pinmux_enable(pmx);
> +
> +       state->pmx = pmx;
> +}
> +If you want a specific mux setting and not just the first one found for this
> +device you can specify a specific mux setting, for example in the above example
> +the second i2c0 setting: pinmux_get(&device, "spi0-2");

How a driver that is generic for example sdchi, mmci, ... is supposed to know
the pinmux name ?
How this work if the board want a special mux ? I have to modify also the driver ?


Matthieu


--
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/