Re: [PATCH 2/3] ARM: u8540: Add Pinctrl Device Tree settings foruart0, uart2

From: Lee Jones
Date: Tue May 28 2013 - 06:09:51 EST

On Mon, 27 May 2013, Gabriel Fernandez wrote:

> From: Gabriel Fernandez <gabriel.fernandez@xxxxxx>
>
> This patch adds pinctrl device tree settings for uart0 and uart2
> for ccu8540 board.
>
> Signed-off-by: Gabriel Fernandez <gabriel.fernandez@xxxxxx>
> ---
> arch/arm/boot/dts/ccu8540-pinctrl.dtsi | 77 ++++++++++++++++++++++++
> arch/arm/boot/dts/ccu8540.dts | 7 +++
> arch/arm/boot/dts/dbx5x0.dtsi | 2 +-
> arch/arm/boot/dts/ste-nomadik-pinctrl.dtsi | 95 ++++++++++++++++++++++++++++++

This is starting to get a bit confusing. How intrusive would it be to
place the ccu8540-pinctrl information inside ccu8540.dts instead of
breaking it out into different files and convoluting the issue?

Also, please correct me if I'm wrong, but isn't what we call the
Nomadik Pinctrl/GPIO really the same as DBX500 Pinctrl/GPIO? I wonder
if this would be a better naming convention?

Linus, what do you think?

> include/dt-bindings/pinctrl/nomadik.h | 36 +++++++++++
> 5 files changed, 216 insertions(+), 1 deletion(-)
> create mode 100644 arch/arm/boot/dts/ccu8540-pinctrl.dtsi
> create mode 100644 arch/arm/boot/dts/ste-nomadik-pinctrl.dtsi
> create mode 100644 include/dt-bindings/pinctrl/nomadik.h
>
> diff --git a/arch/arm/boot/dts/ccu8540-pinctrl.dtsi b/arch/arm/boot/dts/ccu8540-pinctrl.dtsi
> new file mode 100644
> index 0000000..26e718b
> --- /dev/null
> +++ b/arch/arm/boot/dts/ccu8540-pinctrl.dtsi
> @@ -0,0 +1,77 @@
> +/*
> + * Copyright 2012 ST-Ericsson
> + *
> + * The code contained herein is licensed under the GNU General Public
> + * License. You may obtain a copy of the GNU General Public License
> + * Version 2 or later at the following locations:
> + *
> + * http://www.opensource.org/licenses/gpl-license.html
> + * http://www.gnu.org/copyleft/gpl.html
> + */
> +#include "ste-nomadik-pinctrl.dtsi"
> +
> +&pinctrl_dbx500 {

What does the '&' do?

> + uart0 {
> + uart0_default_mux: uart0_mux {
> + default_mux {
> + ste,function = "u0";
> + ste,pins = "u0_a_1";
> + };
> + };
> +
> + uart0_default_mode: uart0_default {
> + default_cfg1 {
> + ste,pins = "GPIO0", "GPIO2";
> + ste,config = <&in_pu>;
> + };
> +
> + default_cfg2 {
> + ste,pins = "GPIO1", "GPIO3";
> + ste,config = <&out_hi>;
> + };
> + };
> +
> + uart0_sleep_mode: uart0_sleep {
> + sleep_cfg1 {
> + ste,pins = "GPIO0", "GPIO2";
> + ste,config = <&slpm_in_pu>;
> + };
> +
> + sleep_cfg2 {
> + ste,pins = "GPIO1", "GPIO3";
> + ste,config = <&slpm_out_hi>;
> + };
> + };
> + };
> +
> + uart2 {
> + uart2_default_mode: uart2_default {
> + default_mux {
> + ste,function = "u2";
> + ste,pins = "u2txrx_a_1";
> + };
> +
> + default_cfg1 {
> + ste,pins = "GPIO120";
> + ste,config = <&in_pu>;
> + };
> +
> + default_cfg2 {
> + ste,pins = "GPIO121";
> + ste,config = <&out_hi>;
> + };
> + };
> +
> + uart2_sleep_mode: uart2_sleep {
> + sleep_cfg1 {
> + ste,pins = "GPIO120";
> + ste,config = <&slpm_in_pu>;
> + };
> +
> + sleep_cfg2 {
> + ste,pins = "GPIO121";
> + ste,config = <&slpm_out_hi>;
> + };
> + };
> + };
> +};
> diff --git a/arch/arm/boot/dts/ccu8540.dts b/arch/arm/boot/dts/ccu8540.dts
> index 5de9e1e..4f93795 100644
> --- a/arch/arm/boot/dts/ccu8540.dts
> +++ b/arch/arm/boot/dts/ccu8540.dts
> @@ -11,6 +11,7 @@
>
> /dts-v1/;
> #include "dbx5x0.dtsi"
> +#include "ccu8540-pinctrl.dtsi"
>
> / {
> model = "ST-Ericsson U8540 platform with Device Tree";
> @@ -27,6 +28,9 @@
> };
>
> uart@80120000 {
> + pinctrl-names = "default", "sleep";
> + pinctrl-0 = <&uart0_default_mux>, <&uart0_default_mode>;
> + pinctrl-1 = <&uart0_sleep_mode>;
> status = "okay";
> };
>
> @@ -35,6 +39,9 @@
> };
>
> uart@80007000 {
> + pinctrl-names = "default", "sleep";
> + pinctrl-0 = <&uart2_default_mode>;
> + pinctrl-1 = <&uart2_sleep_mode>;
> status = "okay";
> };
> };
> diff --git a/arch/arm/boot/dts/dbx5x0.dtsi b/arch/arm/boot/dts/dbx5x0.dtsi
> index f85ff85..7507148 100644
> --- a/arch/arm/boot/dts/dbx5x0.dtsi
> +++ b/arch/arm/boot/dts/dbx5x0.dtsi
> @@ -170,7 +170,7 @@
> gpio-bank = <8>;
> };
>
> - pinctrl {
> + pinctrl_dbx500: pinctrl {
> compatible = "stericsson,nmk-pinctrl";
> prcm = <&prcmu>;
> };
> diff --git a/arch/arm/boot/dts/ste-nomadik-pinctrl.dtsi b/arch/arm/boot/dts/ste-nomadik-pinctrl.dtsi
> new file mode 100644
> index 0000000..efddee9
> --- /dev/null
> +++ b/arch/arm/boot/dts/ste-nomadik-pinctrl.dtsi
> @@ -0,0 +1,95 @@
> +/*
> + * Copyright 2012 ST-Ericsson
> + *
> + * The code contained herein is licensed under the GNU General Public
> + * License. You may obtain a copy of the GNU General Public License
> + * Version 2 or later at the following locations:
> + *
> + * http://www.opensource.org/licenses/gpl-license.html
> + * http://www.gnu.org/copyleft/gpl.html
> + */
> +#include <dt-bindings/pinctrl/nomadik.h>
> +
> +/ {
> + in_nopull: in_nopull {
> + ste,input = <INPUT_NOPULL>;
> + };
> +
> + in_pu: input_pull_up {
> + ste,input = <INPUT_PULLUP>;
> + };
> +
> + in_pd: input_pull_down {
> + ste,input = <INPUT_PULLDOWN>;
> + };
> +
> + out_hi: output_high {
> + ste,output = <OUTPUT_HIGH>;
> + };
> +
> + out_lo: output_low {
> + ste,output = <OUTPUT_LOW>;
> + };
> +
> + gpio_out_lo: gpio_output_low {
> + ste,gpio = <GPIOMODE_ENABLED>;
> + ste,output = <OUTPUT_LOW>;
> + };
> +
> + slpm_in_pu: slpm_in_pu {
> + ste,sleep = <SLPM_ENABLED>;
> + ste,sleep-input = <SLPM_INPUT_PULLUP>;
> + ste,sleep-wakeup = <SLPM_WAKEUP_ENABLE>;
> + };
> +
> + slpm_in_wkup_pdis: slpm_in_wkup_pdis {
> + ste,sleep = <SLPM_ENABLED>;
> + ste,sleep-input = <SLPM_DIR_INPUT>;
> + ste,sleep-wakeup = <SLPM_WAKEUP_ENABLE>;
> + ste,sleep-pull-disable = <SLPM_PDIS_DISABLED>;
> + };
> +
> + slpm_out_lo: slpm_out_lo {
> + ste,sleep = <SLPM_ENABLED>;
> + ste,sleep-output = <SLPM_OUTPUT_LOW>;
> + ste,sleep-wakeup = <SLPM_WAKEUP_ENABLE>;
> + };
> +
> + slpm_out_hi: slpm_out_hi {
> + ste,sleep = <SLPM_ENABLED>;
> + ste,sleep-output = <SLPM_OUTPUT_HIGH>;
> + ste,sleep-wakeup = <SLPM_WAKEUP_ENABLE>;
> + };
> +
> + slpm_out_hi_wkup_pdis: slpm_out_hi_wkup_pdis {
> + ste,sleep = <SLPM_ENABLED>;
> + ste,sleep-output = <SLPM_OUTPUT_HIGH>;
> + ste,sleep-wakeup = <SLPM_WAKEUP_ENABLE>;
> + ste,sleep-pull-disable = <SLPM_PDIS_DISABLED>;
> + };
> +
> + slpm_out_wkup_pdis: slpm_out_wkup_pdis {
> + ste,sleep = <SLPM_ENABLED>;
> + ste,sleep-output = <SLPM_DIR_OUTPUT>;
> + ste,sleep-wakeup = <SLPM_WAKEUP_ENABLE>;
> + ste,sleep-pull-disable = <SLPM_PDIS_DISABLED>;
> + };
> +
> + in_wkup_pdis: in_wkup_pdis {
> + ste,sleep-input = <SLPM_DIR_INPUT>;
> + ste,sleep-wakeup = <SLPM_WAKEUP_ENABLE>;
> + ste,sleep-pull-disable = <SLPM_PDIS_DISABLED>;
> + };
> +
> + out_hi_wkup_pdis: out_hi_wkup_pdis {
> + ste,sleep-output = <SLPM_OUTPUT_HIGH>;
> + ste,sleep-wakeup = <SLPM_WAKEUP_ENABLE>;
> + ste,sleep-pull-disable = <SLPM_PDIS_DISABLED>;
> + };
> +
> + out_wkup_pdis: out_wkup_pdis {
> + ste,sleep-output = <SLPM_DIR_OUTPUT>;
> + ste,sleep-wakeup = <SLPM_WAKEUP_ENABLE>;
> + ste,sleep-pull-disable = <SLPM_PDIS_DISABLED>;
> + };
> +};

Nodes look pretty good, except shouldn't 'ste' really be 'stericsson'?

Yes, it's not as succinct, but it is the standard.

> diff --git a/include/dt-bindings/pinctrl/nomadik.h b/include/dt-bindings/pinctrl/nomadik.h
> new file mode 100644
> index 0000000..638fb32
> --- /dev/null
> +++ b/include/dt-bindings/pinctrl/nomadik.h
> @@ -0,0 +1,36 @@
> +/*
> + * nomadik.h
> + *
> + * Copyright (C) ST-Ericsson SA 2013
> + * Author: Gabriel Fernandez <gabriel.fernandez@xxxxxx> for ST-Ericsson.
> + * License terms: GNU General Public License (GPL), version 2
> + */
> +
> +#define INPUT_NOPULL 0
> +#define INPUT_PULLUP 1
> +#define INPUT_PULLDOWN 2
> +
> +#define OUTPUT_LOW 0
> +#define OUTPUT_HIGH 1
> +#define DIR_OUTPUT 2
> +
> +#define SLPM_DISABLED 0
> +#define SLPM_ENABLED 1
> +
> +#define SLPM_INPUT_NOPULL 0
> +#define SLPM_INPUT_PULLUP 1
> +#define SLPM_INPUT_PULLDOWN 2
> +#define SLPM_DIR_INPUT 3
> +
> +#define SLPM_OUTPUT_LOW 0
> +#define SLPM_OUTPUT_HIGH 1
> +#define SLPM_DIR_OUTPUT 2
> +
> +#define SLPM_WAKEUP_DISABLE 0
> +#define SLPM_WAKEUP_ENABLE 1
> +
> +#define GPIOMODE_DISABLED 0
> +#define GPIOMODE_ENABLED 1
> +
> +#define SLPM_PDIS_DISABLED 0
> +#define SLPM_PDIS_ENABLED 1

Some stray tabbing above.

--
Lee Jones
Linaro ST-Ericsson Landing Team Lead
Linaro.org â Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/