From: Matthieu CASTET <matthieu.castet@parrot.com> To: Linus Walleij <linus.walleij@linaro.org> Cc: Sascha Hauer <s.hauer@pengutronix.de>, Grant Likely <grant.likely@secretlab.ca>, Martin Persson <martin.persson@stericsson.com>, Lee Jones <lee.jones@linaro.org>, "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>, "linux-arm-kernel@lists.infradead.org" <linux-arm-kernel@lists.infradead.org> Subject: Re: RE : [PATCH 0/4] Pinmux subsystem Date: Fri, 13 May 2011 18:03:08 +0200 [thread overview] Message-ID: <4DCD563C.8040508@parrot.com> (raw) In-Reply-To: <BANLkTinMxzrPS1GsTV1+poNGTxBpUFyBiw@mail.gmail.com> Linus Walleij a écrit : > 2011/5/12 Matthieu Castet <matthieu.castet@parrot.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
WARNING: multiple messages have this Message-ID (diff)
From: matthieu.castet@parrot.com (Matthieu CASTET) To: linux-arm-kernel@lists.infradead.org Subject: [PATCH 0/4] Pinmux subsystem Date: Fri, 13 May 2011 18:03:08 +0200 [thread overview] Message-ID: <4DCD563C.8040508@parrot.com> (raw) In-Reply-To: <BANLkTinMxzrPS1GsTV1+poNGTxBpUFyBiw@mail.gmail.com> Linus Walleij a ?crit : > 2011/5/12 Matthieu Castet <matthieu.castet@parrot.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
next prev parent reply other threads:[~2011-05-13 16:03 UTC|newest] Thread overview: 48+ messages / expand[flat|nested] mbox.gz Atom feed top 2011-05-02 19:16 [PATCH 0/4] Pinmux subsystem Linus Walleij 2011-05-02 19:16 ` Linus Walleij 2011-05-02 22:57 ` Russell King - ARM Linux 2011-05-02 22:57 ` Russell King - ARM Linux 2011-05-10 21:25 ` Linus Walleij 2011-05-10 21:25 ` Linus Walleij 2011-05-10 21:45 ` Russell King - ARM Linux 2011-05-10 21:45 ` Russell King - ARM Linux 2011-05-10 23:15 ` Linus Walleij 2011-05-10 23:15 ` Linus Walleij 2011-05-03 17:27 ` Andrew Lunn 2011-05-03 17:27 ` Andrew Lunn 2011-05-03 19:29 ` Valdis.Kletnieks 2011-05-03 19:29 ` Valdis.Kletnieks at vt.edu 2011-05-10 21:42 ` Linus Walleij 2011-05-10 21:42 ` Linus Walleij 2011-05-11 9:50 ` Andrew Lunn 2011-05-11 9:50 ` Andrew Lunn 2011-05-12 0:41 ` Linus Walleij 2011-05-12 0:41 ` Linus Walleij 2011-05-12 7:00 ` Andrew Lunn 2011-05-12 7:00 ` Andrew Lunn 2011-05-15 13:33 ` Andrew Lunn 2011-05-15 13:33 ` Andrew Lunn 2011-05-15 17:50 ` Linus Walleij 2011-05-15 17:50 ` Linus Walleij 2011-05-17 1:57 ` Kyungmin Park 2011-05-17 1:57 ` Kyungmin Park 2011-05-18 20:02 ` Linus Walleij 2011-05-18 20:02 ` Linus Walleij 2011-05-18 21:21 ` Mark Brown 2011-05-18 21:21 ` Mark Brown 2011-05-12 7:44 ` Sascha Hauer 2011-05-12 7:44 ` Sascha Hauer 2011-05-12 9:40 ` Tony Lindgren 2011-05-12 9:40 ` Tony Lindgren 2011-05-12 14:02 ` Linus Walleij 2011-05-12 14:02 ` Linus Walleij 2011-05-12 21:17 ` RE : " Matthieu Castet 2011-05-12 21:17 ` Matthieu Castet 2011-05-13 7:05 ` RE : " Linus Walleij 2011-05-13 7:05 ` Linus Walleij 2011-05-13 16:03 ` Matthieu CASTET [this message] 2011-05-13 16:03 ` Matthieu CASTET 2011-05-14 7:57 ` RE : " Linus Walleij 2011-05-14 7:57 ` Linus Walleij 2011-05-13 9:59 ` Sascha Hauer 2011-05-13 9:59 ` Sascha Hauer
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=4DCD563C.8040508@parrot.com \ --to=matthieu.castet@parrot.com \ --cc=grant.likely@secretlab.ca \ --cc=lee.jones@linaro.org \ --cc=linus.walleij@linaro.org \ --cc=linux-arm-kernel@lists.infradead.org \ --cc=linux-kernel@vger.kernel.org \ --cc=martin.persson@stericsson.com \ --cc=s.hauer@pengutronix.de \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.