Message-ID: <BANLkTi=SKJFHPgBzi9WdeiPhkkmgs8JGMQ@mail.gmail.com>
Date:	Sat, 14 May 2011 09:57:35 +0200
From:	Linus Walleij <linus.walleij@...aro.org>
To:	Matthieu CASTET <matthieu.castet@...rot.com>
Cc:	Sascha Hauer <s.hauer@...gutronix.de>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	Grant Likely <grant.likely@...retlab.ca>,
	Martin Persson <martin.persson@...ricsson.com>,
	Lee Jones <lee.jones@...aro.org>,
	"linux-arm-kernel@...ts.infradead.org" 
	<linux-arm-kernel@...ts.infradead.org>
Subject: Re: RE : [PATCH 0/4] Pinmux subsystem

2011/5/13 Matthieu CASTET <matthieu.castet@...rot.com>:
> Linus Walleij a écrit :
>> 2011/5/12 Matthieu Castet <matthieu.castet@...rot.com>:
>>> 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.

Sorry I'm not following, what is inflexible in this case?

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

It's what I define as different functions from the pinmux:

Say if this is SPI0 that can use either 4, 3 or 2 wires you *can* call these
just "spi0-4wire", "spi0-3wire", "spi0-2wire". The only thing the pinmux
subsystem deals with it groups of pins, this is what is called functions.

It does not assume anything about how these functions are defined and
used, it just makes sure they do not overlap.

So with pinmux you can activate function "spi0-2wire" and then use the
two remaining pins as GPIO, no problem, it won't conflict since the
two free pins aren't part of that function.

> This is board specific not package specific.

It relates to how the stuff is connected to the packages I think, and whether
you call that package or board specific is no big issue. What I can say for
sure is if the package didn't have 4 wires wherof you could use only
two, you couldn't do any of this.

> 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), ...

No problem at all. You can define the functions you want.

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

Now, the framework does not deal with how the groups of pins, i.e. the
functions, are defined. Currently that is done by the drivers. The only
thing it deals with is handling conflicts among pins, and selecting
such groups.

If you want to create these groups from board data (what I call package
data) or say device tree, that is perfectly fine.

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

Again this is an example of a simple driver, it's entirely up to your
driver to do. There are many similar example in Documentation/*
where we don't use #define to simplify examples.

>> +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 ?

Again this is an example. If this data came from the device tree for
example, or from arch/arm/mach-foo/package-db3350.c  in some
struct db3350_pin_platform_data{} that I come up with does not
matter to the framework.

It's no different from how say your MMC driver knows how to handle
which GPIO pin is card detect, you have to tell it what pin it is, likewise
you can tell your pinmux driver what pins to handle.

>> +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 ?

Since the board define function mappings such as:

PINMUX_MAP("sdi0-my-group", "sdi0.0"),

The driver can reference that one association:

pinmux_get(dev, NULL);

If you have different muxings you can also use an optional function
name. This is no different to how you get clocks with clk_get(dev, NULL)
or regulators.

> How this work if the board want a special mux ? I have to modify also the driver ?

No I don't think so. If you mean specify new functions, you write your driver
so that your platform data/package data/device tree/etc can pass muxes in to
it. Don't lock on to the fact that the example driver does all that inside the
driver, it is an example only.

Imagine the same question for regulators. No different, just that the regulator
framework has some standard structure for how to pass in platform data
and I have not defined that. The clk framework has no such standard (yet)
and is comparable, if you had you clock driver in drivers/clk/foo.c you would
have the same problem.

If what you mean is that you want another muxing device, that is fully
supported in the same manner as we support several gpio_chip:s.

Yours,
Linus Walleij
--
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/

Powered by blists - more mailing lists