Re: [PATCH 5/8] pinctrl-tz1090: add TZ1090 pinctrl driver
On Thu, May 16, 2013 at 11:12 AM, James Hogan wrote: > Hi Linus, > On 15/05/13 20:07, Linus Walleij wrote: >> On Tue, May 14, 2013 at 2:22 PM, James Hogan wrote: >> >>> I think that's the other way around, i.e. that's talking about mapping >>> several pingroups to the same function. The next paragraph is closer to >>> the problem: >>> >>> Documentation/pinctrl.txt - PINS for a certain FUNCTION using a certain PIN GROUP on a certain PIN CONTROLLER are provided on a first-come first-serve basis, so if some other device mux setting or GPIO pin request has already taken your physical pin, you will be denied the use of it. To get (activate) a new setting, the old one has to be put (deactivated) first. >>> >>> In my example the tft pingroup contains all the tft pins, but I might >>> want a particular pin inside that pingroup to never be controlled by the >>> mux (using the per-pin mux disable (SELECT) bits). >>> >>> So if I use pinmux I'd have something like: >>> * "tft" pingroup maps to "tft" function >>> * "tft_green0" pingroup (containing individual pin inside "tft" >>> pingroup) maps to "none" function to disable muxing (or the inverse, >>> with each pin in use mapping to "periph" to enable muxing). >>> in which case the pin has multiple muxes "controlling" it (even though >>> they're sort of nested). The above paragraph seems to condemn this >>> arrangement. >> >> So if this usecase is what you want to support, why don't you just >> exclude that one pin from the "tft" group and put it into another >> group? > > It's a very board specific thing (that was just one example). To > generalise your suggestion would make all muxing pingroups contain no > pins (which is perhaps accurate since the thing that's muxed by the > group is a set of internal signals that may be muxed out to pins via the > SELECT bits). Well there are different ways to skin this cat. Stephen usually says that if each pin can be muxed individually (such as if they each have a unique control register switching that very pin between different devices) then each pin should have its own pin group. Then you accumulate these groups into a state, there may be many of them. One group per pin is perfectly legal. Myself I've been soft on the issue. I think that is the hardware engineers designing the pin muxing have had certain usecases in mind, or (especially) if there are hardware restricts such that the mux from a system point of view can just be set if it is set on all pins in succession, we can define a group of pins for the usecase. You're making it sound like the one-group-per-pin is a more viable approach in your case and that will work, but myself I usually try to go over the usecases from a HW point of view and think which groups make sense to combine, and come up with a small set of groups that I combine with a function to get the right settings for a device. Please choose the solution you think fits your system best. However avoid falling into the combinatorial trap, pinctrl is not about listing all the possible *theoretical* combinations of pins in groups, that is mathematics. We're doing practical solutions here. Yours, Linus Walleij ___ devicetree-discuss mailing list devicetree-discuss@lists.ozlabs.org https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: [PATCH 5/8] pinctrl-tz1090: add TZ1090 pinctrl driver
Hi Linus, On 15/05/13 20:07, Linus Walleij wrote: > On Tue, May 14, 2013 at 2:22 PM, James Hogan wrote: > >> I think that's the other way around, i.e. that's talking about mapping >> several pingroups to the same function. The next paragraph is closer to >> the problem: >> >> Documentation/pinctrl.txt >>> - PINS for a certain FUNCTION using a certain PIN GROUP on a certain >>> PIN CONTROLLER are provided on a first-come first-serve basis, so if some >>> other device mux setting or GPIO pin request has already taken your >>> physical >>> pin, you will be denied the use of it. To get (activate) a new setting, >>> the >>> old one has to be put (deactivated) first. >> >> In my example the tft pingroup contains all the tft pins, but I might >> want a particular pin inside that pingroup to never be controlled by the >> mux (using the per-pin mux disable (SELECT) bits). >> >> So if I use pinmux I'd have something like: >> * "tft" pingroup maps to "tft" function >> * "tft_green0" pingroup (containing individual pin inside "tft" >> pingroup) maps to "none" function to disable muxing (or the inverse, >> with each pin in use mapping to "periph" to enable muxing). >> in which case the pin has multiple muxes "controlling" it (even though >> they're sort of nested). The above paragraph seems to condemn this >> arrangement. > > So if this usecase is what you want to support, why don't you just > exclude that one pin from the "tft" group and put it into another > group? It's a very board specific thing (that was just one example). To generalise your suggestion would make all muxing pingroups contain no pins (which is perhaps accurate since the thing that's muxed by the group is a set of internal signals that may be muxed out to pins via the SELECT bits). Cheers James ___ devicetree-discuss mailing list devicetree-discuss@lists.ozlabs.org https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: [PATCH 5/8] pinctrl-tz1090: add TZ1090 pinctrl driver
On Tue, May 14, 2013 at 2:22 PM, James Hogan wrote: > I think that's the other way around, i.e. that's talking about mapping > several pingroups to the same function. The next paragraph is closer to > the problem: > > Documentation/pinctrl.txt >> - PINS for a certain FUNCTION using a certain PIN GROUP on a certain >> PIN CONTROLLER are provided on a first-come first-serve basis, so if some >> other device mux setting or GPIO pin request has already taken your >> physical >> pin, you will be denied the use of it. To get (activate) a new setting, the >> old one has to be put (deactivated) first. > > In my example the tft pingroup contains all the tft pins, but I might > want a particular pin inside that pingroup to never be controlled by the > mux (using the per-pin mux disable (SELECT) bits). > > So if I use pinmux I'd have something like: > * "tft" pingroup maps to "tft" function > * "tft_green0" pingroup (containing individual pin inside "tft" > pingroup) maps to "none" function to disable muxing (or the inverse, > with each pin in use mapping to "periph" to enable muxing). > in which case the pin has multiple muxes "controlling" it (even though > they're sort of nested). The above paragraph seems to condemn this > arrangement. So if this usecase is what you want to support, why don't you just exclude that one pin from the "tft" group and put it into another group? If you want it activated anyway as part of some state e.g. "default", you can just specify multiple groups for that state. > Or using pinconf I'd have something like: > * "tft" pingroup maps to "tft" function > * "tft_green0" pin (in "tft" pingroup) has pinconf "no-periph" (or the > inverse, with each one in use having pinconf "periph") If you want some pinconf set up for this one pin as part of a configured state that's OK even if the pin isn't muxed in as part of this state. muxing and config are orthogonal. > Or modifying the pinctrl subsystem maybe something like: > * [optional: particular named pins in] "tft" pingroup map to "tft" function > that way enabling a mux doesn't necessarily apply to all pins in the > group, and drivers for hardware that doesn't support partial enabling of > the mux can reject it (I might experiment with this if I get the time). No thanks, this will be hopeless to understand, the pinctrl subsystem is already really hard for outsiders to understand, let us not raise the bar even more... Yours, Linus Walleij ___ devicetree-discuss mailing list devicetree-discuss@lists.ozlabs.org https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: [PATCH 5/8] pinctrl-tz1090: add TZ1090 pinctrl driver
On 14/05/13 12:52, Linus Walleij wrote: > On Fri, May 3, 2013 at 5:06 PM, James Hogan wrote: >> [me] >>> Thus this part of the problem (poking that "select" bit) >>> should be handled by the pinmux part of the driver. >>> >>> The pinconf part does not need to know about it. >> >> Okay, so how would you recommend handling the case of a pin in a muxing >> pingroup that shouldn't be put into peripheral mode? >> >> E.g. imagine an 18bit display is wired to the (24bit) tft pins (which >> are muxed as a group to "tft" function), and the least significant tft >> pins are used as GPIOs to control something like board power supplies. >> >> Without using pinconf I think the muxing pingroups would have to overlap >> like below (is that acceptable?): > > I don't know if I understand your example correctly but are you after > this part of the documentation from Documentation/pinctrl.txt: > > Pinmux conventions > == > (...) > It is possible to map several groups to the same combination of device, > pin controller and function. This is for cases where a certain function on > a certain pin controller may use different sets of pins in different > configurations. I think that's the other way around, i.e. that's talking about mapping several pingroups to the same function. The next paragraph is closer to the problem: Documentation/pinctrl.txt > - PINS for a certain FUNCTION using a certain PIN GROUP on a certain > PIN CONTROLLER are provided on a first-come first-serve basis, so if some > other device mux setting or GPIO pin request has already taken your physical > pin, you will be denied the use of it. To get (activate) a new setting, the > old one has to be put (deactivated) first. In my example the tft pingroup contains all the tft pins, but I might want a particular pin inside that pingroup to never be controlled by the mux (using the per-pin mux disable (SELECT) bits). So if I use pinmux I'd have something like: * "tft" pingroup maps to "tft" function * "tft_green0" pingroup (containing individual pin inside "tft" pingroup) maps to "none" function to disable muxing (or the inverse, with each pin in use mapping to "periph" to enable muxing). in which case the pin has multiple muxes "controlling" it (even though they're sort of nested). The above paragraph seems to condemn this arrangement. Or using pinconf I'd have something like: * "tft" pingroup maps to "tft" function * "tft_green0" pin (in "tft" pingroup) has pinconf "no-periph" (or the inverse, with each one in use having pinconf "periph") Or modifying the pinctrl subsystem maybe something like: * [optional: particular named pins in] "tft" pingroup map to "tft" function that way enabling a mux doesn't necessarily apply to all pins in the group, and drivers for hardware that doesn't support partial enabling of the mux can reject it (I might experiment with this if I get the time). Does that make more sense? Cheers James ___ devicetree-discuss mailing list devicetree-discuss@lists.ozlabs.org https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: [PATCH 5/8] pinctrl-tz1090: add TZ1090 pinctrl driver
On Fri, May 3, 2013 at 5:06 PM, James Hogan wrote: > [me] >> Thus this part of the problem (poking that "select" bit) >> should be handled by the pinmux part of the driver. >> >> The pinconf part does not need to know about it. > > Okay, so how would you recommend handling the case of a pin in a muxing > pingroup that shouldn't be put into peripheral mode? > > E.g. imagine an 18bit display is wired to the (24bit) tft pins (which > are muxed as a group to "tft" function), and the least significant tft > pins are used as GPIOs to control something like board power supplies. > > Without using pinconf I think the muxing pingroups would have to overlap > like below (is that acceptable?): I don't know if I understand your example correctly but are you after this part of the documentation from Documentation/pinctrl.txt: Pinmux conventions == (...) It is possible to map several groups to the same combination of device, pin controller and function. This is for cases where a certain function on a certain pin controller may use different sets of pins in different configurations. Yours, Linus Walleij ___ devicetree-discuss mailing list devicetree-discuss@lists.ozlabs.org https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: [PATCH 5/8] pinctrl-tz1090: add TZ1090 pinctrl driver
Hi Linus,
On 03/05/13 14:03, Linus Walleij wrote:
> On Fri, May 3, 2013 at 2:23 PM, James Hogan wrote:
>> [Me]
>>> If what you need is to set the pin into "GPIO mode" to drive it
>>> to some default state then from pinconf-generic.h you should use
>>> one of the existing defines like PIN_CONFIG_OUTPUT
>>> to actively drive it to high or low as default, or
>>> PIN_CONFIG_BIAS_HIGH_IMPEDANCE for some default
>>> GPIO input mode.
>>>
>>> Read the new section named "GPIO mode pitfalls" in
>>> Documentation/pinctrl.txt
>>
>> Thanks, that was interesting. I've had a think about this (and done some
>> experiments with a multimeter), and the problem is these generic
>> pinconfs already have meanings which don't match what the SELECT
>> register does. For example, having a pin be tristate and not controlled
>> by the peripheral, and having it tristate as far as the gpio hardware is
>> concerned (e.g. no pull-up) but still controlled by the peripheral, are
>> two very different things that need disambiguation.
>
> I don't know if that is necessary.
>
> While I do recognize that it is possible that we need to put
> pins into "GPIO mode", i.e. drive them actively low or high,
> as PIN_CONFIG_OUTPUT does, I'm not convinced that
> pin config should handle the case where a signal is passed
> through from a peripheral.
>
> I think that for every pin that is put to use for a peripheral
> you must anyway at some point call .enable() on the
> struct pinmux_ops of the pin controller.
>
> Thus this part of the problem (poking that "select" bit)
> should be handled by the pinmux part of the driver.
>
> The pinconf part does not need to know about it.
Okay, so how would you recommend handling the case of a pin in a muxing
pingroup that shouldn't be put into peripheral mode?
E.g. imagine an 18bit display is wired to the (24bit) tft pins (which
are muxed as a group to "tft" function), and the least significant tft
pins are used as GPIOs to control something like board power supplies.
Without using pinconf I think the muxing pingroups would have to overlap
like below (is that acceptable?):
pingroup "tft_pins"
pins: "red0"..."red7"
functions: "tft", "lcd"
pingroup "red0"
pins: "red0"
functions: "peripheral" (OR "none")
...
pingroup "red7"
pins: "red7"
functions: "peripheral" (OR "none")
and then do something like this?
map {
tft_mux {
pins = "tft_pins";
function = "tft";
/* mux tft pins to tft panel interface */
};
tft_pins {
pins = "red7", "red6", "red5", "red4", "red2";
function = "peripheral";
/* mux pins to peripherals */
};
};
or maybe this:
map {
tft_mux {
pins = "tft_pins";
function = "tft";
/* auto sets individual pins to peripheral */
};
tft_pins {
pins = "red1", "red0";
function = "none";
/* set individual pins to !peripheral */
};
};
Cheers
James
___
devicetree-discuss mailing list
devicetree-discuss@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: [PATCH 5/8] pinctrl-tz1090: add TZ1090 pinctrl driver
On Fri, May 3, 2013 at 2:23 PM, James Hogan wrote: > [Me] >> If what you need is to set the pin into "GPIO mode" to drive it >> to some default state then from pinconf-generic.h you should use >> one of the existing defines like PIN_CONFIG_OUTPUT >> to actively drive it to high or low as default, or >> PIN_CONFIG_BIAS_HIGH_IMPEDANCE for some default >> GPIO input mode. >> >> Read the new section named "GPIO mode pitfalls" in >> Documentation/pinctrl.txt > > Thanks, that was interesting. I've had a think about this (and done some > experiments with a multimeter), and the problem is these generic > pinconfs already have meanings which don't match what the SELECT > register does. For example, having a pin be tristate and not controlled > by the peripheral, and having it tristate as far as the gpio hardware is > concerned (e.g. no pull-up) but still controlled by the peripheral, are > two very different things that need disambiguation. I don't know if that is necessary. While I do recognize that it is possible that we need to put pins into "GPIO mode", i.e. drive them actively low or high, as PIN_CONFIG_OUTPUT does, I'm not convinced that pin config should handle the case where a signal is passed through from a peripheral. I think that for every pin that is put to use for a peripheral you must anyway at some point call .enable() on the struct pinmux_ops of the pin controller. Thus this part of the problem (poking that "select" bit) should be handled by the pinmux part of the driver. The pinconf part does not need to know about it. The idea is definately not for the pin config to act as a "backend" for pin muxing, rather to partition the problem into two independent parts. Yours, Linus Walleij ___ devicetree-discuss mailing list devicetree-discuss@lists.ozlabs.org https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: [PATCH 5/8] pinctrl-tz1090: add TZ1090 pinctrl driver
Hi Linus,
On 03/05/13 10:13, Linus Walleij wrote:
> On Fri, Apr 26, 2013 at 1:54 PM, James Hogan wrote:
>> On 25/04/13 23:39, Linus Walleij wrote:
>>> On Tue, Apr 23, 2013 at 4:33 PM, James Hogan wrote:
+static const struct cfg_param {
+ const char *property;
+ enum tz1090_pinconf_param param;
+} cfg_params[] = {
+ {"select", TZ1090_PINCONF_PARAM_SELECT},
+ {"pull",TZ1090_PINCONF_PARAM_PULL},
+ {"schmitt", TZ1090_PINCONF_PARAM_SCHMITT},
+ {"slew-rate", TZ1090_PINCONF_PARAM_SLEW_RATE},
+ {"drive-strength", TZ1090_PINCONF_PARAM_DRIVE_STRENGTH},
+};
>>>
>>> Almost all exist in .
>>>
>>> What is "select"? If you need another config parameter
>>> we can just add it, but explain what it is and we can tell
>>> if it fits.
>>
>> select refers to the registers CR_PADS_GPIO_SELECT{0,1,2} with
>> descriptions like this:
>> Reset values: 0x3fc0, 0x3fff, 0x3fff
>> 29:0CR_PADS_GPIO_SEL0 GPIO select (1 bit per GPIO pin)
>> 0 = serial interface
>> 1 = GPIO interface
>> [29] SCB1_SCLK
>> (etc)
>>
>> PARAM_GPIO may be a better name (select seems to have just stuck from
>> when the original gpio driver was written 3 years ago), although it
>> should be noted that the gpio system still has to enable it too, so it's
>> really about taking it out of the "serial interface" so that the
>> connected SoC peripheral cannot mess with it (1) by default (2) if it's
>> not connected to what the peripheral would expect, e.g. controlling
>> board power!
>
> The GPIO select should not be visible to the outside like this,
> as it is a particular bit that should only be set on request from the
> GPIO framework.
>
> If what you need is to set the pin into "GPIO mode" to drive it
> to some default state then from pinconf-generic.h you should use
> one of the existing defines like PIN_CONFIG_OUTPUT
> to actively drive it to high or low as default, or
> PIN_CONFIG_BIAS_HIGH_IMPEDANCE for some default
> GPIO input mode.
>
> Read the new section named "GPIO mode pitfalls" in
> Documentation/pinctrl.txt
Thanks, that was interesting. I've had a think about this (and done some
experiments with a multimeter), and the problem is these generic
pinconfs already have meanings which don't match what the SELECT
register does. For example, having a pin be tristate and not controlled
by the peripheral, and having it tristate as far as the gpio hardware is
concerned (e.g. no pull-up) but still controlled by the peripheral, are
two very different things that need disambiguation.
I think what it comes down to as far as pinctrl is concerned is that the
SELECT registers enable/disable peripheral muxes on a per-pin basis.
Therefore perhaps it makes best sense to just have a custom/generic
pinconf PIN_CONFIG_PERIPHERAL_ENABLE/"peripheral=<1>;" to enable
peripheral muxing in the first place (sets SELECT=0), and require it to
be set on all individual pins that need it (i.e. don't automatically set
SELECT=0 on all pins in a group when the mux is enabled). What do you think?
Thanks
James
___
devicetree-discuss mailing list
devicetree-discuss@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: [PATCH 5/8] pinctrl-tz1090: add TZ1090 pinctrl driver
On Fri, Apr 26, 2013 at 1:54 PM, James Hogan wrote:
> On 25/04/13 23:39, Linus Walleij wrote:
>> On Tue, Apr 23, 2013 at 4:33 PM, James Hogan wrote:
>> (...)
>>> +/* Pin names */
>>> +
>>> +static const struct pinctrl_pin_desc tz1090_pins[] = {
>>> + /* Normal GPIOs */
>>> + PINCTRL_PIN(TZ1090_PIN_SDIO_CLK,"sdio_clk"),
>>> + PINCTRL_PIN(TZ1090_PIN_SDIO_CMD,"sdio_cmd"),
>>
>> Are these actually the names from the datasheet? Usually these
>> have geographical names, like D1, A7... but just checking.
>
> Yes, these are the ball names in the datasheet, also the names of the
> corresponding gpios, and used on board schematics to refer to the SoC
> connections, although the corresponding ball indexes are also provided
> in one place in $letter$number form. I suspect it's done this way
> because many of the pins can only mux to a single peripheral (or gpio).
OK I'll live with it...
>> (...)
>>> +/* Sub muxes */
>>
>> Can you describe here briefly what a sub mux is and how it is
>> deployed in this system? It's getting complicated at this point
>> so some help would be appreciated.
>
> Sure. The TZ1090 doesn't have a lot of muxing capabilities (about 10
> bits devoted to muxing if you ignore debug signals and gpios), but the
> largest one (the TFT pins) has a 2 level mux. Here are the register fields:
>
> CR_PADS_TFT_CTL
> TFT pad mux control
> 0 - TFT display
> 1 - Ext DAC
> 2 - TS Out 1
> 3 - Meta2 Trace
> 4 - Phyway IF
>
> CR_PADS_AFE_DIR_CTL
> Control how the TFT pad direction is driven when CR_PADS_TFT_CTL = 1
> 0 - Pads are output to DAC
> 1 - not aqadc_stb
> 2 - iqdac_stb
>
> so the submux is an internal thing that allows the tft pins to appear to
> have all the above functions, and if any of the last 3 are chosen the
> first level mux is set to 1.
OK! Please put that whole block above in a
/*
* Multi-line comment in the driver so it will be obvious information
* to the poor souls that will later have to understand the code.
*/
>>> +/**
>>> + * struct tz1090_pmx - Private pinctrl data
>>> + * @dev: Platform device
>>> + * @pctl: Pin control device
>>> + * @regs: Register region
>>> + * @lock: Lock protecting coherency of pin_en, gpio_en, select_en, and
>>> + * SELECT regs
>>> + * @pin_en:Pins that have been enabled (32 pins packed into each
>>> element)
>>> + * @gpio_en: GPIOs that have been enabled (32 pins packed into each
>>> element)
>>> + * @select_en: Pins that have been force seleced by pinconf (32 pins packed
>>> + * into each element)
>>> + */
>>> +struct tz1090_pmx {
>>> + struct device *dev;
>>> + struct pinctrl_dev *pctl;
>>> + void __iomem*regs;
>>> + spinlock_t lock;
>>> + u32 pin_en[3];
>>> + u32 gpio_en[3];
>>> + u32 select_en[3];
>>> +};
>>
>> This looks real nice! :-)
>
> Thanks. Is the behaviour w.r.t. gpios correct/acceptable btw?...
> The pins default to "gpio mode" (SELECT=1, where the normal SoC
> peripheral cannot mess with it), so these last 3 fields basically allow
> a pin to be put in gpio mode iff:
>
> * gpio_en: gpio is requested by gpio driver
>
> * OR !pin_en: pin hasn't had a specific mux function enabled (where each
> pin can be individually muxed to "default" to take it out of gpio mode).
>
> * OR select_en: pin is force selected (pinconf select=1) in DT e.g. pin
> is part of a mux but isn't wired up to the normal thing, e.g. a TFT pin
> that's wired to a button.
>
> I.e. the gpio driver can always take control of a pin even if it's been
> muxed (e.g. allowing a driver to do a gpio bitbang bus reset).
Yes this is the case with most combined drivers.
(Maybe the framework isn't always as helpful as it could be,
improvements welcome.)
>>> +static const char *tz1090_pinctrl_get_group_name(struct pinctrl_dev
>>> *pctldev,
>>> +unsigned group)
>>> +{
>>> + if (group < ARRAY_SIZE(tz1090_groups)) {
>>> + /* normal pingroup */
>>> + return tz1090_groups[group].name;
>>> + } else {
>>> + /* individual gpio pin pseudo-pingroup */
>>> + unsigned int pin = group - ARRAY_SIZE(tz1090_groups);
>>> + return tz1090_pins[pin].name;
>>
>> At this point it is pretty important that the pin has a meaningful
>> name, that is why recycling the function names for pin names
>> is not good. "spi1_dout" doesn't make very much sense for a pin
>> that is now used as GPIO, so it better be named exactly like that.
>
> I think if the pin's actual name is "spi1_dout", and board schematics
> always refer to that pin name, e.g. having an LED connected to
> "spi1_dout", then it does make sense for the pin (and pingroup
> containing only that pin) to be referred to by
Re: [PATCH 5/8] pinctrl-tz1090: add TZ1090 pinctrl driver
Hi Linus,
On 25/04/13 23:39, Linus Walleij wrote:
> On Tue, Apr 23, 2013 at 4:33 PM, James Hogan wrote:
>
>> Add a pin control driver for the main pins on the TZ1090 SoC. This
>> doesn't include the low-power pins as they're controlled separately via
>> the Powerdown Controller (PDC) registers.
>>
>> Signed-off-by: James Hogan
>> Cc: Grant Likely
>> Cc: Rob Herring
>> Cc: Rob Landley
>> Cc: Linus Walleij
>> Cc: linux-...@vger.kernel.org
>
> (...)
>> +++ b/drivers/pinctrl/Kconfig
>> @@ -196,6 +196,12 @@ config PINCTRL_TEGRA114
>> bool
>> select PINCTRL_TEGRA
>>
>> +config PINCTRL_TZ1090
>> + bool "Toumaz Xenif TZ1090 pin control driver"
>> + depends on SOC_TZ1090
>> + select PINMUX
>> + select PINCONF
>
> Why are you not using GENERIC_PINCONF?
>
> It doesn't seem like this pin controller is using something
> that isn't covered by that library.
>
> This way you get rid of TZ1090_PINCONF_PACK() etc
> and can use the standard packing.
Cool, I wasn't actually aware of that, I'll look into it.
>> +#include
>
> As mentioned I want the pin definintions from the arch to be in this
> subsystem as well.
>
> If the GPIO driver is also using the, then move the GPIO driver
> into drivers/pinctrl, that is recommended in such cases.
okay, I'll sort that out.
>> + * @drv: Drive control supported, 0 if unsupported.
>> + * This means Schmitt, Slew, and Drive strength.
>> + * @slw_bit: Slew register bit. 0 if unsupported.
>> + * The same bit is used for Schmitt, and Drive (*2).
> (...)
>> + u32 drv:1;
>
> So what about you use a bool for that?
okay (I'll probably convert all the bitfields to normal types. since the
submux was added they don't actually offer any advantage).
>
>> + u32 slw_bit:5;
>> +};
>
> (...)
>> +/* Pin names */
>> +
>> +static const struct pinctrl_pin_desc tz1090_pins[] = {
>> + /* Normal GPIOs */
>> + PINCTRL_PIN(TZ1090_PIN_SDIO_CLK,"sdio_clk"),
>> + PINCTRL_PIN(TZ1090_PIN_SDIO_CMD,"sdio_cmd"),
>
> Are these actually the names from the datasheet? Usually these
> have geographical names, like D1, A7... but just checking.
Yes, these are the ball names in the datasheet, also the names of the
corresponding gpios, and used on board schematics to refer to the SoC
connections, although the corresponding ball indexes are also provided
in one place in $letter$number form. I suspect it's done this way
because many of the pins can only mux to a single peripheral (or gpio).
>
> (...)
>> +/* Sub muxes */
>
> Can you describe here briefly what a sub mux is and how it is
> deployed in this system? It's getting complicated at this point
> so some help would be appreciated.
Sure. The TZ1090 doesn't have a lot of muxing capabilities (about 10
bits devoted to muxing if you ignore debug signals and gpios), but the
largest one (the TFT pins) has a 2 level mux. Here are the register fields:
CR_PADS_TFT_CTL
TFT pad mux control
0 - TFT display
1 - Ext DAC
2 - TS Out 1
3 - Meta2 Trace
4 - Phyway IF
CR_PADS_AFE_DIR_CTL
Control how the TFT pad direction is driven when CR_PADS_TFT_CTL = 1
0 - Pads are output to DAC
1 - not aqadc_stb
2 - iqdac_stb
so the submux is an internal thing that allows the tft pins to appear to
have all the above functions, and if any of the last 3 are chosen the
first level mux is set to 1.
>
>> +/* Pin group with mux control */
>> +#define MUX_PG(pg_name, f0, f1, f2, f3, f4,\
>> + mux_r, mux_b, mux_w, slw_b) \
>> + { \
>> + .name = #pg_name, \
>> + .pins = pg_name##_pins, \
>> + .npins = ARRAY_SIZE(pg_name##_pins),\
>> + .mux = MUX(f0, f1, f2, f3, f4, \
>> + mux_r, mux_b, mux_w),\
>> + .drv = ((slw_b) >= 0), \
>> + .slw_bit = (slw_b), \
>> + }
>> +
>> +#define SIMPLE_PG(pg_name) \
>> + { \
>> + .name = #pg_name, \
>> + .pins = pg_name##_pins, \
>> + .npins = ARRAY_SIZE(pg_name##_pins),\
>> + }
>> +
>> +#define SIMPLE_DRV_PG(pg_name, slw_b) \
>> + { \
>> + .name = #pg_name, \
>> + .pins = pg_name##_pins, \
>> + .npins = ARRAY_SIZE(pg_name##_pins),\
>> + .drv
Re: [PATCH 5/8] pinctrl-tz1090: add TZ1090 pinctrl driver
On Tue, Apr 23, 2013 at 4:33 PM, James Hogan wrote:
> Add a pin control driver for the main pins on the TZ1090 SoC. This
> doesn't include the low-power pins as they're controlled separately via
> the Powerdown Controller (PDC) registers.
>
> Signed-off-by: James Hogan
> Cc: Grant Likely
> Cc: Rob Herring
> Cc: Rob Landley
> Cc: Linus Walleij
> Cc: linux-...@vger.kernel.org
(...)
> +++ b/drivers/pinctrl/Kconfig
> @@ -196,6 +196,12 @@ config PINCTRL_TEGRA114
> bool
> select PINCTRL_TEGRA
>
> +config PINCTRL_TZ1090
> + bool "Toumaz Xenif TZ1090 pin control driver"
> + depends on SOC_TZ1090
> + select PINMUX
> + select PINCONF
Why are you not using GENERIC_PINCONF?
It doesn't seem like this pin controller is using something
that isn't covered by that library.
This way you get rid of TZ1090_PINCONF_PACK() etc
and can use the standard packing.
> +#include
As mentioned I want the pin definintions from the arch to be in this
subsystem as well.
If the GPIO driver is also using the, then move the GPIO driver
into drivers/pinctrl, that is recommended in such cases.
> + * @drv: Drive control supported, 0 if unsupported.
> + * This means Schmitt, Slew, and Drive strength.
> + * @slw_bit: Slew register bit. 0 if unsupported.
> + * The same bit is used for Schmitt, and Drive (*2).
(...)
> + u32 drv:1;
So what about you use a bool for that?
> + u32 slw_bit:5;
> +};
(...)
> +/* Pin names */
> +
> +static const struct pinctrl_pin_desc tz1090_pins[] = {
> + /* Normal GPIOs */
> + PINCTRL_PIN(TZ1090_PIN_SDIO_CLK,"sdio_clk"),
> + PINCTRL_PIN(TZ1090_PIN_SDIO_CMD,"sdio_cmd"),
Are these actually the names from the datasheet? Usually these
have geographical names, like D1, A7... but just checking.
(...)
> +/* Sub muxes */
Can you describe here briefly what a sub mux is and how it is
deployed in this system? It's getting complicated at this point
so some help would be appreciated.
> +/* Pin group with mux control */
> +#define MUX_PG(pg_name, f0, f1, f2, f3, f4,\
> + mux_r, mux_b, mux_w, slw_b) \
> + { \
> + .name = #pg_name, \
> + .pins = pg_name##_pins, \
> + .npins = ARRAY_SIZE(pg_name##_pins),\
> + .mux = MUX(f0, f1, f2, f3, f4, \
> + mux_r, mux_b, mux_w),\
> + .drv = ((slw_b) >= 0), \
> + .slw_bit = (slw_b), \
> + }
> +
> +#define SIMPLE_PG(pg_name) \
> + { \
> + .name = #pg_name, \
> + .pins = pg_name##_pins, \
> + .npins = ARRAY_SIZE(pg_name##_pins),\
> + }
> +
> +#define SIMPLE_DRV_PG(pg_name, slw_b) \
> + { \
> + .name = #pg_name, \
> + .pins = pg_name##_pins, \
> + .npins = ARRAY_SIZE(pg_name##_pins),\
> + .drv = 1, \
> + .slw_bit = (slw_b), \
> + }
> +
> +#define DRV_PG(pg_name, slw_b) \
> + { \
> + .name = "drive_"#pg_name, \
> + .pins = drive_##pg_name##_pins, \
> + .npins = ARRAY_SIZE(drive_##pg_name##_pins),\
> + .drv = 1, \
> + .slw_bit = (slw_b), \
> + }
> +
> +/*name f0, f1,f2,f3, f4, mux r/b/w */
> +DEFINE_SUBMUX(ext_dac, DAC, NOT_IQADC_STB, IQDAC_STB, NA, NA, IF_CTL, 6, 2);
Again, this is not very easy to understand, so more commenting is warranted.
The macros may need individual documentation for being quite
hard to understand.
> +/**
> + * struct tz1090_pmx - Private pinctrl data
> + * @dev: Platform device
> + * @pctl: Pin control device
> + * @regs: Register region
> + * @lock: Lock protecting coherency of pin_en, gpio_en, select_en, and
> + * SELECT regs
> + * @pin_en:Pins that have been enabled (32 pins packed into each element)
> + * @gpio_en: GPIOs that have been enabled (32 pins packed into each
> element)
> + * @select_en: Pins that have been force seleced by pinconf (32 pins packed
> + * into eac
