Re: [PATCH v2 1/4] pinctrl: add samsung pinctrl and gpiolib driver
On 28 August 2012 04:52, Linus Walleij wrote:
> On Tue, Aug 21, 2012 at 9:22 PM, Thomas Abraham
> wrote:
[...]
+/* list of all possible config options supported */
+struct pin_config {
+ char*prop_cfg;
+ unsigned intcfg_type;
+} pcfgs[] = {
+ { "samsung,pin-pud", PINCFG_TYPE_PUD },
+ { "samsung,pin-drv", PINCFG_TYPE_DRV },
+ { "samsung,pin-pud-pdn", PINCFG_TYPE_CON_PND },
+ { "samsung,pin-drv-pdn", PINCFG_TYPE_PUD_PND },
+};
>>>
>>> Hm it looks very much like this controller could make use of
>>> the generic pinconf library, but it's not mandatory so just a suggestion.
>>
>> Ok. The last two entries in the above table are Samsung specific and
>> not covered by generic-pinconf. So, I am not sure if it can be added
>> to generic-pinconf.
>
> What is so Samsung-specific about them?
>
> If you tell us the electrical property of setting them we can figure out
> if they should be generic or not...
>
The PINCFG_TYPE_CON_PND and PINCFG_TYPE_PUD_PND are "mux function" and
"pull up/down" settings that are automatically applied to the pin in
powert-down mode. It is same as the usual "mux function" and "pull
up/down" settings, but just that it is applied at suspend.
[...]
>
> OK whatever... looking formward to next iteration!
I have posted the updated version of this patch series based on your
comments (https://lkml.org/lkml/2012/8/23/183).
The only pending comment to be addressed at this point is about the
use of generic-pinconf as mentioned in the comment above. If
PINCFG_TYPE_CON_PND and PINCFG_TYPE_PUD_PND is available in
generic-pinconf, I wil switch to using generic-pinconf. Will that
qualify this Samsung pinctrl driver to be merged into your -devel
branch?
Thanks,
Thomas.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2 1/4] pinctrl: add samsung pinctrl and gpiolib driver
On Tue, Aug 21, 2012 at 9:22 PM, Thomas Abraham
wrote:
>>> + - samsung,pin-pud: Pull up/down configuration.
>>> + - samsung,pin-drv: Drive strength configuration.
>>> + - samsung,pin-pud-pdn: Pull up/down configuration in power down mode.
>>> + - samsung,pin-drv-pdn: Drive strength configuration in power down mode.
>>
>> This looks a bit scary, as it seems to be orthogonal to the pin config
>> interface. I.e. this will be programmed "behind the back" of the
>> pin config system. However as long as the pin config implementation
>> reads back these things from the registers it will work, too.
>
> These properties are converted to a PIN_MAP_TYPE_CONFIGS_GROUP map
> type and stored in a instance of 'struct pinctrl_map'. These can be
> read back from the registers and reverse-mapped as well.
OK
> All the dt bindings defined and used in the Samsung pinctrl driver are
> first translated into pinctrl subystem defined data structures and
> then used. Hence, there are no register configurations done that skip
> over the pinctrl subsystem (except for the gpio/wakeup interrupts).
OK fine.
>> In the U300 and Ux500 I explicitly use pin config hogs to set up
>> the pin configuration, and when we enter a state such as
>> "default" the mux setting and config settings are set from the
>> framework separately.
>>
>> See for example:
>> arch/arm/mach-ux500/board-mop500-pins.c
>>
>> This example is using platform data but it should be trivial to do with
>> device tree.
>>
>> I think the Tegra also works this way. Can you elaborate on
>> why you need this static setup from the device tree instead
>> of using default states?
>
> Sorry, I did not understand this question.
You answered above, no problem.
>>> + pinctrl_0: pinctrl@1140 {
>>> + compatible = "samsung,pinctrl-exynos4210";
>>> + reg = <0x1140 0x1000>;
>>> + interrupts = <0 47 0>;
>>> +
>>> + uart0_data: uart0-data {
>>> + samsung,pins = "gpa0-0", "gpa0-1";
>>> + samsung,pin-function = <2>;
>>> + samsung,pin-pud = <0>;
>>> + samsung,pin-drv = <0>;
>>> + };
>>
>> This setup needs to be associated with a certain state, it's possible to
>> do in the code or directly in the device tree.
>>
>> I.e. these settings for pin-pud and pin-drv needs to belong to a
>> certain pin config state, typically the state named "default"
>
> Yes, I agree. So, for example, the uart device node would have
>
> uart@1380 {
>compatilble = " ";
>
>
>pinctrl-names = "default";
>pinctrl-0 - <_data>;
> };
>
> The uart driver during probe can then call
>
>devm_pinctrl_get_select_default(>dev);
>
> For the example above, this call will set the 'mux', 'pud' and 'drv'
> values to gpa-0 and gpa-1 pins.
OK perfect, that's how it should work.
>>> +/* list of all possible config options supported */
>>> +struct pin_config {
>>> + char*prop_cfg;
>>> + unsigned intcfg_type;
>>> +} pcfgs[] = {
>>> + { "samsung,pin-pud", PINCFG_TYPE_PUD },
>>> + { "samsung,pin-drv", PINCFG_TYPE_DRV },
>>> + { "samsung,pin-pud-pdn", PINCFG_TYPE_CON_PND },
>>> + { "samsung,pin-drv-pdn", PINCFG_TYPE_PUD_PND },
>>> +};
>>
>> Hm it looks very much like this controller could make use of
>> the generic pinconf library, but it's not mandatory so just a suggestion.
>
> Ok. The last two entries in the above table are Samsung specific and
> not covered by generic-pinconf. So, I am not sure if it can be added
> to generic-pinconf.
What is so Samsung-specific about them?
If you tell us the electrical property of setting them we can figure out
if they should be generic or not...
> For now, since you are not enforcing the use of
> generic-pinconf, I will keep it the way it is now.
Sure that's OK.
>>> + /* Allocate memory for pin group name. The pin group name is derived
>>> +* from the node name from which these map entries are be created.
>>> +*/
>>> + gname = kzalloc(strlen(np->name) + 4, GFP_KERNEL);
>>
>> Why +4? I would have suspected +1 for the null terminator...
>
> The name of the pin group is derived from the node name and hence
> strlen(np->name). To this name, "-grp" is appended to imply that this
> is a group. Hence +4 is used. I will replace +4 with probably
> strlen(PINGRP_SUFFIX) where PINGRP_SUFFIX is defined as "-grp".
Aha OK I get it.
>>> +/* reading pin pull up/down and driver strength settings not implemented */
>>
>> OK why not? It seems very simple and straight-forward.
>> Just read the same registers and switch() then return...
>
> Ok, I will do that. I did not see how those would be used and hence skipped
> it.
One good usecase is to look at the state
Re: [PATCH v2 1/4] pinctrl: add samsung pinctrl and gpiolib driver
On Tue, Aug 21, 2012 at 9:22 PM, Thomas Abraham
thomas.abra...@linaro.org wrote:
+ - samsung,pin-pud: Pull up/down configuration.
+ - samsung,pin-drv: Drive strength configuration.
+ - samsung,pin-pud-pdn: Pull up/down configuration in power down mode.
+ - samsung,pin-drv-pdn: Drive strength configuration in power down mode.
This looks a bit scary, as it seems to be orthogonal to the pin config
interface. I.e. this will be programmed behind the back of the
pin config system. However as long as the pin config implementation
reads back these things from the registers it will work, too.
These properties are converted to a PIN_MAP_TYPE_CONFIGS_GROUP map
type and stored in a instance of 'struct pinctrl_map'. These can be
read back from the registers and reverse-mapped as well.
OK
All the dt bindings defined and used in the Samsung pinctrl driver are
first translated into pinctrl subystem defined data structures and
then used. Hence, there are no register configurations done that skip
over the pinctrl subsystem (except for the gpio/wakeup interrupts).
OK fine.
In the U300 and Ux500 I explicitly use pin config hogs to set up
the pin configuration, and when we enter a state such as
default the mux setting and config settings are set from the
framework separately.
See for example:
arch/arm/mach-ux500/board-mop500-pins.c
This example is using platform data but it should be trivial to do with
device tree.
I think the Tegra also works this way. Can you elaborate on
why you need this static setup from the device tree instead
of using default states?
Sorry, I did not understand this question.
You answered above, no problem.
+ pinctrl_0: pinctrl@1140 {
+ compatible = samsung,pinctrl-exynos4210;
+ reg = 0x1140 0x1000;
+ interrupts = 0 47 0;
+
+ uart0_data: uart0-data {
+ samsung,pins = gpa0-0, gpa0-1;
+ samsung,pin-function = 2;
+ samsung,pin-pud = 0;
+ samsung,pin-drv = 0;
+ };
This setup needs to be associated with a certain state, it's possible to
do in the code or directly in the device tree.
I.e. these settings for pin-pud and pin-drv needs to belong to a
certain pin config state, typically the state named default
Yes, I agree. So, for example, the uart device node would have
uart@1380 {
compatilble = ;
rest of the properties here
pinctrl-names = default;
pinctrl-0 - uart0_data;
};
The uart driver during probe can then call
devm_pinctrl_get_select_default(pdev-dev);
For the example above, this call will set the 'mux', 'pud' and 'drv'
values to gpa-0 and gpa-1 pins.
OK perfect, that's how it should work.
+/* list of all possible config options supported */
+struct pin_config {
+ char*prop_cfg;
+ unsigned intcfg_type;
+} pcfgs[] = {
+ { samsung,pin-pud, PINCFG_TYPE_PUD },
+ { samsung,pin-drv, PINCFG_TYPE_DRV },
+ { samsung,pin-pud-pdn, PINCFG_TYPE_CON_PND },
+ { samsung,pin-drv-pdn, PINCFG_TYPE_PUD_PND },
+};
Hm it looks very much like this controller could make use of
the generic pinconf library, but it's not mandatory so just a suggestion.
Ok. The last two entries in the above table are Samsung specific and
not covered by generic-pinconf. So, I am not sure if it can be added
to generic-pinconf.
What is so Samsung-specific about them?
If you tell us the electrical property of setting them we can figure out
if they should be generic or not...
For now, since you are not enforcing the use of
generic-pinconf, I will keep it the way it is now.
Sure that's OK.
+ /* Allocate memory for pin group name. The pin group name is derived
+* from the node name from which these map entries are be created.
+*/
+ gname = kzalloc(strlen(np-name) + 4, GFP_KERNEL);
Why +4? I would have suspected +1 for the null terminator...
The name of the pin group is derived from the node name and hence
strlen(np-name). To this name, -grp is appended to imply that this
is a group. Hence +4 is used. I will replace +4 with probably
strlen(PINGRP_SUFFIX) where PINGRP_SUFFIX is defined as -grp.
Aha OK I get it.
+/* reading pin pull up/down and driver strength settings not implemented */
OK why not? It seems very simple and straight-forward.
Just read the same registers and switch() then return...
Ok, I will do that. I did not see how those would be used and hence skipped
it.
One good usecase is to look at the state of pins in debugfs.
+static int __devinit samsung_pinctrl_probe(struct platform_device *pdev)
(...)
+ if (ctrl-eint_gpio_init)
+
Re: [PATCH v2 1/4] pinctrl: add samsung pinctrl and gpiolib driver
On 28 August 2012 04:52, Linus Walleij linus.wall...@linaro.org wrote:
On Tue, Aug 21, 2012 at 9:22 PM, Thomas Abraham
thomas.abra...@linaro.org wrote:
[...]
+/* list of all possible config options supported */
+struct pin_config {
+ char*prop_cfg;
+ unsigned intcfg_type;
+} pcfgs[] = {
+ { samsung,pin-pud, PINCFG_TYPE_PUD },
+ { samsung,pin-drv, PINCFG_TYPE_DRV },
+ { samsung,pin-pud-pdn, PINCFG_TYPE_CON_PND },
+ { samsung,pin-drv-pdn, PINCFG_TYPE_PUD_PND },
+};
Hm it looks very much like this controller could make use of
the generic pinconf library, but it's not mandatory so just a suggestion.
Ok. The last two entries in the above table are Samsung specific and
not covered by generic-pinconf. So, I am not sure if it can be added
to generic-pinconf.
What is so Samsung-specific about them?
If you tell us the electrical property of setting them we can figure out
if they should be generic or not...
The PINCFG_TYPE_CON_PND and PINCFG_TYPE_PUD_PND are mux function and
pull up/down settings that are automatically applied to the pin in
powert-down mode. It is same as the usual mux function and pull
up/down settings, but just that it is applied at suspend.
[...]
OK whatever... looking formward to next iteration!
I have posted the updated version of this patch series based on your
comments (https://lkml.org/lkml/2012/8/23/183).
The only pending comment to be addressed at this point is about the
use of generic-pinconf as mentioned in the comment above. If
PINCFG_TYPE_CON_PND and PINCFG_TYPE_PUD_PND is available in
generic-pinconf, I wil switch to using generic-pinconf. Will that
qualify this Samsung pinctrl driver to be merged into your -devel
branch?
Thanks,
Thomas.
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2 1/4] pinctrl: add samsung pinctrl and gpiolib driver
On 22 August 2012 03:08, Stephen Warren wrote: > On 08/21/2012 05:25 AM, Linus Walleij wrote: >> On Wed, Aug 15, 2012 at 9:57 PM, Thomas Abraham >> wrote: >> >>> Add a new device tree enabled pinctrl and gpiolib driver for Samsung >>> SoC's. > ... >>> + The child node can also optionally specify one or more of the pin >>> + configuration that should be applied on all the pins listed in the >>> + "samsung,pins" property of the child node. The following pin >>> configuration >>> + properties are supported. >>> + >>> + - samsung,pin-pud: Pull up/down configuration. >>> + - samsung,pin-drv: Drive strength configuration. >>> + - samsung,pin-pud-pdn: Pull up/down configuration in power down mode. >>> + - samsung,pin-drv-pdn: Drive strength configuration in power down mode. >> >> This looks a bit scary, as it seems to be orthogonal to the pin config >> interface. I.e. this will be programmed "behind the back" of the >> pin config system. However as long as the pin config implementation >> reads back these things from the registers it will work, too. >> >> In the U300 and Ux500 I explicitly use pin config hogs to set up >> the pin configuration, and when we enter a state such as >> "default" the mux setting and config settings are set from the >> framework separately. > > I know that some HW has a separate set of registers (or fields) for the > "awake" and "sleep" configuration, and the HW switches between the two > automatically when sleeping. I have no idea if the Samsung SoCs do this, > but I think if this were the case, it'd be quite legitimate to define > both these HW states as separate sets of properties within a single > pinctrl SW state. So, that might be the explanation here? > Hi Stephen, Yes, Samsung SoC support the "awake" and "sleep" configuration in that way you described. Thanks, Thomas. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2 1/4] pinctrl: add samsung pinctrl and gpiolib driver
Hi Linus,
Thanks for your time to review the Samsung pinctrl driver patches. I
have inlined the reply to your comments.
On 21 August 2012 16:55, Linus Walleij wrote:
> On Wed, Aug 15, 2012 at 9:57 PM, Thomas Abraham
> wrote:
>
>> Add a new device tree enabled pinctrl and gpiolib driver for Samsung
>> SoC's.
>
> Thanks for doing this Thomas, great work!
>
>> +++ b/Documentation/devicetree/bindings/pinctrl/samsung-pinctrl.txt
>
> I don't understand the rules around bindings very well, I would
> suggest you include
> devicetree-disc...@lists.ozlabs.org on the mails, besides you know
> this stuff way
> better than me anyway :-)
Yes, I missed Cc'ing devicetree-disc...@lists.ozlabs.org. I will do
that in the post of this patch set.
>
>> + The child node can also optionally specify one or more of the pin
>> + configuration that should be applied on all the pins listed in the
>> + "samsung,pins" property of the child node. The following pin configuration
>> + properties are supported.
>> +
>> + - samsung,pin-pud: Pull up/down configuration.
>> + - samsung,pin-drv: Drive strength configuration.
>> + - samsung,pin-pud-pdn: Pull up/down configuration in power down mode.
>> + - samsung,pin-drv-pdn: Drive strength configuration in power down mode.
>
> This looks a bit scary, as it seems to be orthogonal to the pin config
> interface. I.e. this will be programmed "behind the back" of the
> pin config system. However as long as the pin config implementation
> reads back these things from the registers it will work, too.
These properties are converted to a PIN_MAP_TYPE_CONFIGS_GROUP map
type and stored in a instance of 'struct pinctrl_map'. These can be
read back from the registers and reverse-mapped as well.
All the dt bindings defined and used in the Samsung pinctrl driver are
first translated into pinctrl subystem defined data structures and
then used. Hence, there are no register configurations done that skip
over the pinctrl subsystem (except for the gpio/wakeup interrupts).
>
> In the U300 and Ux500 I explicitly use pin config hogs to set up
> the pin configuration, and when we enter a state such as
> "default" the mux setting and config settings are set from the
> framework separately.
>
> See for example:
> arch/arm/mach-ux500/board-mop500-pins.c
>
> This example is using platform data but it should be trivial to do with
> device tree.
>
> I think the Tegra also works this way. Can you elaborate on
> why you need this static setup from the device tree instead
> of using default states?
Sorry, I did not understand this question.
>
> I also think this looks like it could use generic pin config to stash
> the configs, just expand the stuff in
>
> (...)
>> +Example 1:
>
> The examples seem to only pertain to the pin controller per se, maybe you
> could include a DT entry for a uart or something showing how the
> uart device binds to a certain pinctrl setting.
Yes, such an example will be informative here. I will add an example for this.
>
>> + pinctrl_0: pinctrl@1140 {
>> + compatible = "samsung,pinctrl-exynos4210";
>> + reg = <0x1140 0x1000>;
>> + interrupts = <0 47 0>;
>> +
>> + uart0_data: uart0-data {
>> + samsung,pins = "gpa0-0", "gpa0-1";
>> + samsung,pin-function = <2>;
>> + samsung,pin-pud = <0>;
>> + samsung,pin-drv = <0>;
>> + };
>
> This setup needs to be associated with a certain state, it's possible to
> do in the code or directly in the device tree.
>
> I.e. these settings for pin-pud and pin-drv needs to belong to a
> certain pin config state, typically the state named "default"
Yes, I agree. So, for example, the uart device node would have
uart@1380 {
compatilble = " ";
pinctrl-names = "default";
pinctrl-0 - <_data>;
};
The uart driver during probe can then call
devm_pinctrl_get_select_default(>dev);
For the example above, this call will set the 'mux', 'pud' and 'drv'
values to gpa-0 and gpa-1 pins.
>
>> diff --git a/drivers/pinctrl/Kconfig b/drivers/pinctrl/Kconfig
>
>> +config PINCTRL_SAMSUNG
>> + bool "Samsung pinctrl driver"
>> + depends on OF
>
> I don't understand how this can even compile. Do you need:
>
> select PINMUX
> select PINCONF
>
> to get the framework files you need to compile?
>
> Or are you selecting thes in some platform Kconfig or so?
> In that case please move them here.
These were selected if PINCTRL_EXYNOS4 config option is selected. This
is in the 2/4 patch of this series. But, as you said, PINMUX and
PINCONF should be selected with PINCTRL_SAMSUNG config option. I will
fix this.
>
>> +/* list of all possible config options supported */
>> +struct pin_config {
>> + char
Re: [PATCH v2 1/4] pinctrl: add samsung pinctrl and gpiolib driver
On 08/21/2012 05:25 AM, Linus Walleij wrote: > On Wed, Aug 15, 2012 at 9:57 PM, Thomas Abraham > wrote: > >> Add a new device tree enabled pinctrl and gpiolib driver for Samsung >> SoC's. ... >> + The child node can also optionally specify one or more of the pin >> + configuration that should be applied on all the pins listed in the >> + "samsung,pins" property of the child node. The following pin configuration >> + properties are supported. >> + >> + - samsung,pin-pud: Pull up/down configuration. >> + - samsung,pin-drv: Drive strength configuration. >> + - samsung,pin-pud-pdn: Pull up/down configuration in power down mode. >> + - samsung,pin-drv-pdn: Drive strength configuration in power down mode. > > This looks a bit scary, as it seems to be orthogonal to the pin config > interface. I.e. this will be programmed "behind the back" of the > pin config system. However as long as the pin config implementation > reads back these things from the registers it will work, too. > > In the U300 and Ux500 I explicitly use pin config hogs to set up > the pin configuration, and when we enter a state such as > "default" the mux setting and config settings are set from the > framework separately. I know that some HW has a separate set of registers (or fields) for the "awake" and "sleep" configuration, and the HW switches between the two automatically when sleeping. I have no idea if the Samsung SoCs do this, but I think if this were the case, it'd be quite legitimate to define both these HW states as separate sets of properties within a single pinctrl SW state. So, that might be the explanation here? -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2 1/4] pinctrl: add samsung pinctrl and gpiolib driver
On Wed, Aug 15, 2012 at 9:57 PM, Thomas Abraham
wrote:
> Add a new device tree enabled pinctrl and gpiolib driver for Samsung
> SoC's.
Thanks for doing this Thomas, great work!
> +++ b/Documentation/devicetree/bindings/pinctrl/samsung-pinctrl.txt
I don't understand the rules around bindings very well, I would
suggest you include
devicetree-disc...@lists.ozlabs.org on the mails, besides you know
this stuff way
better than me anyway :-)
> + The child node can also optionally specify one or more of the pin
> + configuration that should be applied on all the pins listed in the
> + "samsung,pins" property of the child node. The following pin configuration
> + properties are supported.
> +
> + - samsung,pin-pud: Pull up/down configuration.
> + - samsung,pin-drv: Drive strength configuration.
> + - samsung,pin-pud-pdn: Pull up/down configuration in power down mode.
> + - samsung,pin-drv-pdn: Drive strength configuration in power down mode.
This looks a bit scary, as it seems to be orthogonal to the pin config
interface. I.e. this will be programmed "behind the back" of the
pin config system. However as long as the pin config implementation
reads back these things from the registers it will work, too.
In the U300 and Ux500 I explicitly use pin config hogs to set up
the pin configuration, and when we enter a state such as
"default" the mux setting and config settings are set from the
framework separately.
See for example:
arch/arm/mach-ux500/board-mop500-pins.c
This example is using platform data but it should be trivial to do with
device tree.
I think the Tegra also works this way. Can you elaborate on
why you need this static setup from the device tree instead
of using default states?
I also think this looks like it could use generic pin config to stash
the configs, just expand the stuff in
(...)
> +Example 1:
The examples seem to only pertain to the pin controller per se, maybe you
could include a DT entry for a uart or something showing how the
uart device binds to a certain pinctrl setting.
> + pinctrl_0: pinctrl@1140 {
> + compatible = "samsung,pinctrl-exynos4210";
> + reg = <0x1140 0x1000>;
> + interrupts = <0 47 0>;
> +
> + uart0_data: uart0-data {
> + samsung,pins = "gpa0-0", "gpa0-1";
> + samsung,pin-function = <2>;
> + samsung,pin-pud = <0>;
> + samsung,pin-drv = <0>;
> + };
This setup needs to be associated with a certain state, it's possible to
do in the code or directly in the device tree.
I.e. these settings for pin-pud and pin-drv needs to belong to a
certain pin config state, typically the state named "default"
> diff --git a/drivers/pinctrl/Kconfig b/drivers/pinctrl/Kconfig
> +config PINCTRL_SAMSUNG
> + bool "Samsung pinctrl driver"
> + depends on OF
I don't understand how this can even compile. Do you need:
select PINMUX
select PINCONF
to get the framework files you need to compile?
Or are you selecting thes in some platform Kconfig or so?
In that case please move them here.
> +/* list of all possible config options supported */
> +struct pin_config {
> + char*prop_cfg;
> + unsigned intcfg_type;
> +} pcfgs[] = {
> + { "samsung,pin-pud", PINCFG_TYPE_PUD },
> + { "samsung,pin-drv", PINCFG_TYPE_DRV },
> + { "samsung,pin-pud-pdn", PINCFG_TYPE_CON_PND },
> + { "samsung,pin-drv-pdn", PINCFG_TYPE_PUD_PND },
> +};
Hm it looks very much like this controller could make use of
the generic pinconf library, but it's not mandatory so just a suggestion.
(...)
> +/* create pinctrl_map entries by parsing device tree nodes */
> +static int samsung_dt_node_to_map(struct pinctrl_dev *pctldev,
> + struct device_node *np, struct pinctrl_map **maps,
> + unsigned *nmaps)
> +{
(...)
> + /* Allocate memory for pin group name. The pin group name is derived
> +* from the node name from which these map entries are be created.
> +*/
> + gname = kzalloc(strlen(np->name) + 4, GFP_KERNEL);
Why +4? I would have suspected +1 for the null terminator...
> + if (!gname) {
> + dev_err(dev, "failed to alloc memory for group name\n");
> + goto free_map;
> + }
> + sprintf(gname, "%s-grp", np->name);
The rest of the pinmux implementation looks nice!
(...)
> +/* set the pull up/down and driver strength settings for a specified pin */
> +static int samsung_pinconf_set(struct pinctrl_dev *pctldev, unsigned int pin,
> + unsigned long config)
> +{
> + struct samsung_pinctrl_drv_data *drvdata;
> + unsigned long pin_offset;
> + struct samsung_pin_bank *bank;
> + void __iomem *reg;
> +
> + drvdata = pinctrl_dev_get_drvdata(pctldev);
> + pin_to_reg_bank(drvdata->gc, pin -
Re: [PATCH v2 1/4] pinctrl: add samsung pinctrl and gpiolib driver
On 08/21/2012 05:25 AM, Linus Walleij wrote: On Wed, Aug 15, 2012 at 9:57 PM, Thomas Abraham thomas.abra...@linaro.org wrote: Add a new device tree enabled pinctrl and gpiolib driver for Samsung SoC's. ... + The child node can also optionally specify one or more of the pin + configuration that should be applied on all the pins listed in the + samsung,pins property of the child node. The following pin configuration + properties are supported. + + - samsung,pin-pud: Pull up/down configuration. + - samsung,pin-drv: Drive strength configuration. + - samsung,pin-pud-pdn: Pull up/down configuration in power down mode. + - samsung,pin-drv-pdn: Drive strength configuration in power down mode. This looks a bit scary, as it seems to be orthogonal to the pin config interface. I.e. this will be programmed behind the back of the pin config system. However as long as the pin config implementation reads back these things from the registers it will work, too. In the U300 and Ux500 I explicitly use pin config hogs to set up the pin configuration, and when we enter a state such as default the mux setting and config settings are set from the framework separately. I know that some HW has a separate set of registers (or fields) for the awake and sleep configuration, and the HW switches between the two automatically when sleeping. I have no idea if the Samsung SoCs do this, but I think if this were the case, it'd be quite legitimate to define both these HW states as separate sets of properties within a single pinctrl SW state. So, that might be the explanation here? -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2 1/4] pinctrl: add samsung pinctrl and gpiolib driver
Hi Linus,
Thanks for your time to review the Samsung pinctrl driver patches. I
have inlined the reply to your comments.
On 21 August 2012 16:55, Linus Walleij linus.wall...@linaro.org wrote:
On Wed, Aug 15, 2012 at 9:57 PM, Thomas Abraham
thomas.abra...@linaro.org wrote:
Add a new device tree enabled pinctrl and gpiolib driver for Samsung
SoC's.
Thanks for doing this Thomas, great work!
+++ b/Documentation/devicetree/bindings/pinctrl/samsung-pinctrl.txt
I don't understand the rules around bindings very well, I would
suggest you include
devicetree-disc...@lists.ozlabs.org on the mails, besides you know
this stuff way
better than me anyway :-)
Yes, I missed Cc'ing devicetree-disc...@lists.ozlabs.org. I will do
that in the post of this patch set.
+ The child node can also optionally specify one or more of the pin
+ configuration that should be applied on all the pins listed in the
+ samsung,pins property of the child node. The following pin configuration
+ properties are supported.
+
+ - samsung,pin-pud: Pull up/down configuration.
+ - samsung,pin-drv: Drive strength configuration.
+ - samsung,pin-pud-pdn: Pull up/down configuration in power down mode.
+ - samsung,pin-drv-pdn: Drive strength configuration in power down mode.
This looks a bit scary, as it seems to be orthogonal to the pin config
interface. I.e. this will be programmed behind the back of the
pin config system. However as long as the pin config implementation
reads back these things from the registers it will work, too.
These properties are converted to a PIN_MAP_TYPE_CONFIGS_GROUP map
type and stored in a instance of 'struct pinctrl_map'. These can be
read back from the registers and reverse-mapped as well.
All the dt bindings defined and used in the Samsung pinctrl driver are
first translated into pinctrl subystem defined data structures and
then used. Hence, there are no register configurations done that skip
over the pinctrl subsystem (except for the gpio/wakeup interrupts).
In the U300 and Ux500 I explicitly use pin config hogs to set up
the pin configuration, and when we enter a state such as
default the mux setting and config settings are set from the
framework separately.
See for example:
arch/arm/mach-ux500/board-mop500-pins.c
This example is using platform data but it should be trivial to do with
device tree.
I think the Tegra also works this way. Can you elaborate on
why you need this static setup from the device tree instead
of using default states?
Sorry, I did not understand this question.
I also think this looks like it could use generic pin config to stash
the configs, just expand the stuff in linux/pinctrl/pinconf-generic.h
(...)
+Example 1:
The examples seem to only pertain to the pin controller per se, maybe you
could include a DT entry for a uart or something showing how the
uart device binds to a certain pinctrl setting.
Yes, such an example will be informative here. I will add an example for this.
+ pinctrl_0: pinctrl@1140 {
+ compatible = samsung,pinctrl-exynos4210;
+ reg = 0x1140 0x1000;
+ interrupts = 0 47 0;
+
+ uart0_data: uart0-data {
+ samsung,pins = gpa0-0, gpa0-1;
+ samsung,pin-function = 2;
+ samsung,pin-pud = 0;
+ samsung,pin-drv = 0;
+ };
This setup needs to be associated with a certain state, it's possible to
do in the code or directly in the device tree.
I.e. these settings for pin-pud and pin-drv needs to belong to a
certain pin config state, typically the state named default
Yes, I agree. So, for example, the uart device node would have
uart@1380 {
compatilble = ;
rest of the properties here
pinctrl-names = default;
pinctrl-0 - uart0_data;
};
The uart driver during probe can then call
devm_pinctrl_get_select_default(pdev-dev);
For the example above, this call will set the 'mux', 'pud' and 'drv'
values to gpa-0 and gpa-1 pins.
diff --git a/drivers/pinctrl/Kconfig b/drivers/pinctrl/Kconfig
+config PINCTRL_SAMSUNG
+ bool Samsung pinctrl driver
+ depends on OF
I don't understand how this can even compile. Do you need:
select PINMUX
select PINCONF
to get the framework files you need to compile?
Or are you selecting thes in some platform Kconfig or so?
In that case please move them here.
These were selected if PINCTRL_EXYNOS4 config option is selected. This
is in the 2/4 patch of this series. But, as you said, PINMUX and
PINCONF should be selected with PINCTRL_SAMSUNG config option. I will
fix this.
+/* list of all possible config options supported */
+struct pin_config {
+ char*prop_cfg;
+ unsigned
Re: [PATCH v2 1/4] pinctrl: add samsung pinctrl and gpiolib driver
On 22 August 2012 03:08, Stephen Warren swar...@wwwdotorg.org wrote: On 08/21/2012 05:25 AM, Linus Walleij wrote: On Wed, Aug 15, 2012 at 9:57 PM, Thomas Abraham thomas.abra...@linaro.org wrote: Add a new device tree enabled pinctrl and gpiolib driver for Samsung SoC's. ... + The child node can also optionally specify one or more of the pin + configuration that should be applied on all the pins listed in the + samsung,pins property of the child node. The following pin configuration + properties are supported. + + - samsung,pin-pud: Pull up/down configuration. + - samsung,pin-drv: Drive strength configuration. + - samsung,pin-pud-pdn: Pull up/down configuration in power down mode. + - samsung,pin-drv-pdn: Drive strength configuration in power down mode. This looks a bit scary, as it seems to be orthogonal to the pin config interface. I.e. this will be programmed behind the back of the pin config system. However as long as the pin config implementation reads back these things from the registers it will work, too. In the U300 and Ux500 I explicitly use pin config hogs to set up the pin configuration, and when we enter a state such as default the mux setting and config settings are set from the framework separately. I know that some HW has a separate set of registers (or fields) for the awake and sleep configuration, and the HW switches between the two automatically when sleeping. I have no idea if the Samsung SoCs do this, but I think if this were the case, it'd be quite legitimate to define both these HW states as separate sets of properties within a single pinctrl SW state. So, that might be the explanation here? Hi Stephen, Yes, Samsung SoC support the awake and sleep configuration in that way you described. Thanks, Thomas. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2 1/4] pinctrl: add samsung pinctrl and gpiolib driver
On Wed, Aug 15, 2012 at 9:57 PM, Thomas Abraham
thomas.abra...@linaro.org wrote:
Add a new device tree enabled pinctrl and gpiolib driver for Samsung
SoC's.
Thanks for doing this Thomas, great work!
+++ b/Documentation/devicetree/bindings/pinctrl/samsung-pinctrl.txt
I don't understand the rules around bindings very well, I would
suggest you include
devicetree-disc...@lists.ozlabs.org on the mails, besides you know
this stuff way
better than me anyway :-)
+ The child node can also optionally specify one or more of the pin
+ configuration that should be applied on all the pins listed in the
+ samsung,pins property of the child node. The following pin configuration
+ properties are supported.
+
+ - samsung,pin-pud: Pull up/down configuration.
+ - samsung,pin-drv: Drive strength configuration.
+ - samsung,pin-pud-pdn: Pull up/down configuration in power down mode.
+ - samsung,pin-drv-pdn: Drive strength configuration in power down mode.
This looks a bit scary, as it seems to be orthogonal to the pin config
interface. I.e. this will be programmed behind the back of the
pin config system. However as long as the pin config implementation
reads back these things from the registers it will work, too.
In the U300 and Ux500 I explicitly use pin config hogs to set up
the pin configuration, and when we enter a state such as
default the mux setting and config settings are set from the
framework separately.
See for example:
arch/arm/mach-ux500/board-mop500-pins.c
This example is using platform data but it should be trivial to do with
device tree.
I think the Tegra also works this way. Can you elaborate on
why you need this static setup from the device tree instead
of using default states?
I also think this looks like it could use generic pin config to stash
the configs, just expand the stuff in linux/pinctrl/pinconf-generic.h
(...)
+Example 1:
The examples seem to only pertain to the pin controller per se, maybe you
could include a DT entry for a uart or something showing how the
uart device binds to a certain pinctrl setting.
+ pinctrl_0: pinctrl@1140 {
+ compatible = samsung,pinctrl-exynos4210;
+ reg = 0x1140 0x1000;
+ interrupts = 0 47 0;
+
+ uart0_data: uart0-data {
+ samsung,pins = gpa0-0, gpa0-1;
+ samsung,pin-function = 2;
+ samsung,pin-pud = 0;
+ samsung,pin-drv = 0;
+ };
This setup needs to be associated with a certain state, it's possible to
do in the code or directly in the device tree.
I.e. these settings for pin-pud and pin-drv needs to belong to a
certain pin config state, typically the state named default
diff --git a/drivers/pinctrl/Kconfig b/drivers/pinctrl/Kconfig
+config PINCTRL_SAMSUNG
+ bool Samsung pinctrl driver
+ depends on OF
I don't understand how this can even compile. Do you need:
select PINMUX
select PINCONF
to get the framework files you need to compile?
Or are you selecting thes in some platform Kconfig or so?
In that case please move them here.
+/* list of all possible config options supported */
+struct pin_config {
+ char*prop_cfg;
+ unsigned intcfg_type;
+} pcfgs[] = {
+ { samsung,pin-pud, PINCFG_TYPE_PUD },
+ { samsung,pin-drv, PINCFG_TYPE_DRV },
+ { samsung,pin-pud-pdn, PINCFG_TYPE_CON_PND },
+ { samsung,pin-drv-pdn, PINCFG_TYPE_PUD_PND },
+};
Hm it looks very much like this controller could make use of
the generic pinconf library, but it's not mandatory so just a suggestion.
(...)
+/* create pinctrl_map entries by parsing device tree nodes */
+static int samsung_dt_node_to_map(struct pinctrl_dev *pctldev,
+ struct device_node *np, struct pinctrl_map **maps,
+ unsigned *nmaps)
+{
(...)
+ /* Allocate memory for pin group name. The pin group name is derived
+* from the node name from which these map entries are be created.
+*/
+ gname = kzalloc(strlen(np-name) + 4, GFP_KERNEL);
Why +4? I would have suspected +1 for the null terminator...
+ if (!gname) {
+ dev_err(dev, failed to alloc memory for group name\n);
+ goto free_map;
+ }
+ sprintf(gname, %s-grp, np-name);
The rest of the pinmux implementation looks nice!
(...)
+/* set the pull up/down and driver strength settings for a specified pin */
+static int samsung_pinconf_set(struct pinctrl_dev *pctldev, unsigned int pin,
+ unsigned long config)
+{
+ struct samsung_pinctrl_drv_data *drvdata;
+ unsigned long pin_offset;
+ struct samsung_pin_bank *bank;
+ void __iomem *reg;
+
+ drvdata = pinctrl_dev_get_drvdata(pctldev);
+ pin_to_reg_bank(drvdata-gc, pin - drvdata-ctrl-base, reg,
+
[PATCH v2 1/4] pinctrl: add samsung pinctrl and gpiolib driver
Add a new device tree enabled pinctrl and gpiolib driver for Samsung SoC's. This driver provides a common and extensible framework for all Samsung SoC's to interface with the pinctrl and gpiolib subsystems. This driver supports only device tree based instantiation and hence can be used only on those Samsung platforms that have device tree enabled. This driver is split into two parts: the pinctrl interface and the gpiolib interface. The pinctrl interface registers pinctrl devices with the pinctrl subsystem and gpiolib interface registers gpio chips with the gpiolib subsystem. The information about the pins, pin groups, pin functions and gpio chips, which are SoC specific, are parsed from device tree node. Cc: Linus Walleij Cc: Kukjin Kim Signed-off-by: Thomas Abraham --- .../bindings/pinctrl/samsung-pinctrl.txt | 176 drivers/pinctrl/Kconfig|4 + drivers/pinctrl/Makefile |1 + drivers/pinctrl/pinctrl-samsung.c | 873 drivers/pinctrl/pinctrl-samsung.h | 215 + 5 files changed, 1269 insertions(+), 0 deletions(-) create mode 100644 Documentation/devicetree/bindings/pinctrl/samsung-pinctrl.txt create mode 100644 drivers/pinctrl/pinctrl-samsung.c create mode 100644 drivers/pinctrl/pinctrl-samsung.h diff --git a/Documentation/devicetree/bindings/pinctrl/samsung-pinctrl.txt b/Documentation/devicetree/bindings/pinctrl/samsung-pinctrl.txt new file mode 100644 index 000..c30ee88 --- /dev/null +++ b/Documentation/devicetree/bindings/pinctrl/samsung-pinctrl.txt @@ -0,0 +1,176 @@ +Samsung GPIO and Pin Mux/Config controller + +Samsung's ARM based SoC's integrates a GPIO and Pin mux/config hardware +controller. It controls the input/output settings on the available pads/pins +and also provides ability to multiplex and configure the output of various +on-chip controllers onto these pads. + +Required Properties: +- compatible: should be one of the following. + - "samsung,pinctrl-exynos4210": for Exynos4210 compatible pin-controller. + - "samsung,pinctrl-exynos5250": for Exynos5250 compatible pin-controller. + +- reg: Base address of the pin controller hardware module and length of + the address space it occupies. + +- interrupts: interrupt specifier for the controller. The format and value of + the interrupt specifier depends on the interrupt parent for the controller. + +- Pin mux/config groups as child nodes: The pin mux (selecting pin function + mode) and pin config (pull up/down, driver strength) settings are represented + as child nodes of the pin-controller node. There should be atleast one + child node and there is no limit on the count of these child nodes. + + The child node should contain a list of pin(s) on which a particular pin + function selection or pin configuration (or both) have to applied. This + list of pins is specified using the property name "samsung,pins". There + should be atleast one pin specfied for this property and there is no upper + limit on the count of pins that can be specified. The pins are specified + using pin names which are derived from the hardware manual of the SoC. As + an example, the pins in GPA0 bank of the pin controller can be represented + as "gpa0-0", "gpa0-1", "gpa0-2" and so on. The names should be in lower case. + The format of the pin names should be (as per the hardware manual) + "[pin bank name]-[pin number within the bank]". + + The pin function selection that should be applied on the pins listed in the + child node is specified using the "samsung,pin-function" property. The value + of this property that should be applied to each of the pins listed in the + "samsung,pins" property should be picked from the hardware manual of the SoC + for the specified pin group. This property is optional in the child node if + no specific function selection is desired for the pins listed in the child + node. The value of this property is used as-is to program the pin-controller + function selector register of the pin-bank. + + The child node can also optionally specify one or more of the pin + configuration that should be applied on all the pins listed in the + "samsung,pins" property of the child node. The following pin configuration + properties are supported. + + - samsung,pin-pud: Pull up/down configuration. + - samsung,pin-drv: Drive strength configuration. + - samsung,pin-pud-pdn: Pull up/down configuration in power down mode. + - samsung,pin-drv-pdn: Drive strength configuration in power down mode. + + The values specified by these config properties should be dervied from the + hardware manual and these values are programmed as-is into the pin + pull up/down and driver strength register of the pin-controller. + + Note: A child should include atleast a pin function selection property or + pin configuration property (one or more) or both. + + The client nodes that require a particular pin
[PATCH v2 1/4] pinctrl: add samsung pinctrl and gpiolib driver
Add a new device tree enabled pinctrl and gpiolib driver for Samsung SoC's. This driver provides a common and extensible framework for all Samsung SoC's to interface with the pinctrl and gpiolib subsystems. This driver supports only device tree based instantiation and hence can be used only on those Samsung platforms that have device tree enabled. This driver is split into two parts: the pinctrl interface and the gpiolib interface. The pinctrl interface registers pinctrl devices with the pinctrl subsystem and gpiolib interface registers gpio chips with the gpiolib subsystem. The information about the pins, pin groups, pin functions and gpio chips, which are SoC specific, are parsed from device tree node. Cc: Linus Walleij linus.wall...@linaro.org Cc: Kukjin Kim kgene@samsung.com Signed-off-by: Thomas Abraham thomas.abra...@linaro.org --- .../bindings/pinctrl/samsung-pinctrl.txt | 176 drivers/pinctrl/Kconfig|4 + drivers/pinctrl/Makefile |1 + drivers/pinctrl/pinctrl-samsung.c | 873 drivers/pinctrl/pinctrl-samsung.h | 215 + 5 files changed, 1269 insertions(+), 0 deletions(-) create mode 100644 Documentation/devicetree/bindings/pinctrl/samsung-pinctrl.txt create mode 100644 drivers/pinctrl/pinctrl-samsung.c create mode 100644 drivers/pinctrl/pinctrl-samsung.h diff --git a/Documentation/devicetree/bindings/pinctrl/samsung-pinctrl.txt b/Documentation/devicetree/bindings/pinctrl/samsung-pinctrl.txt new file mode 100644 index 000..c30ee88 --- /dev/null +++ b/Documentation/devicetree/bindings/pinctrl/samsung-pinctrl.txt @@ -0,0 +1,176 @@ +Samsung GPIO and Pin Mux/Config controller + +Samsung's ARM based SoC's integrates a GPIO and Pin mux/config hardware +controller. It controls the input/output settings on the available pads/pins +and also provides ability to multiplex and configure the output of various +on-chip controllers onto these pads. + +Required Properties: +- compatible: should be one of the following. + - samsung,pinctrl-exynos4210: for Exynos4210 compatible pin-controller. + - samsung,pinctrl-exynos5250: for Exynos5250 compatible pin-controller. + +- reg: Base address of the pin controller hardware module and length of + the address space it occupies. + +- interrupts: interrupt specifier for the controller. The format and value of + the interrupt specifier depends on the interrupt parent for the controller. + +- Pin mux/config groups as child nodes: The pin mux (selecting pin function + mode) and pin config (pull up/down, driver strength) settings are represented + as child nodes of the pin-controller node. There should be atleast one + child node and there is no limit on the count of these child nodes. + + The child node should contain a list of pin(s) on which a particular pin + function selection or pin configuration (or both) have to applied. This + list of pins is specified using the property name samsung,pins. There + should be atleast one pin specfied for this property and there is no upper + limit on the count of pins that can be specified. The pins are specified + using pin names which are derived from the hardware manual of the SoC. As + an example, the pins in GPA0 bank of the pin controller can be represented + as gpa0-0, gpa0-1, gpa0-2 and so on. The names should be in lower case. + The format of the pin names should be (as per the hardware manual) + [pin bank name]-[pin number within the bank]. + + The pin function selection that should be applied on the pins listed in the + child node is specified using the samsung,pin-function property. The value + of this property that should be applied to each of the pins listed in the + samsung,pins property should be picked from the hardware manual of the SoC + for the specified pin group. This property is optional in the child node if + no specific function selection is desired for the pins listed in the child + node. The value of this property is used as-is to program the pin-controller + function selector register of the pin-bank. + + The child node can also optionally specify one or more of the pin + configuration that should be applied on all the pins listed in the + samsung,pins property of the child node. The following pin configuration + properties are supported. + + - samsung,pin-pud: Pull up/down configuration. + - samsung,pin-drv: Drive strength configuration. + - samsung,pin-pud-pdn: Pull up/down configuration in power down mode. + - samsung,pin-drv-pdn: Drive strength configuration in power down mode. + + The values specified by these config properties should be dervied from the + hardware manual and these values are programmed as-is into the pin + pull up/down and driver strength register of the pin-controller. + + Note: A child should include atleast a pin function selection property or + pin configuration property (one or more) or both. +
