diff mbox

[1/2] ARM: dts: tegra: Header file for pinctrl constants

Message ID 1385992502-12771-1-git-send-email-ldewangan@nvidia.com (mailing list archive)
State New, archived
Headers show

Commit Message

Laxman Dewangan Dec. 2, 2013, 1:55 p.m. UTC
Defines pincontrol constants to use from Tegra's DTS file
for tegra pincontrol properties option.

Signed-off-by: Laxman Dewangan <ldewangan@nvidia.com>
---
 include/dt-bindings/pinctrl/pinctrl-tegra.h |   65 +++++++++++++++++++++++++++
 1 files changed, 65 insertions(+), 0 deletions(-)
 create mode 100644 include/dt-bindings/pinctrl/pinctrl-tegra.h

Comments

Thierry Reding Dec. 2, 2013, 2:25 p.m. UTC | #1
On Mon, Dec 02, 2013 at 07:25:01PM +0530, Laxman Dewangan wrote:

I think the canonical subject prefix for Tegra is:

	ARM: tegra:

Perhaps also mention that you "Add" the header file? Like so:

	ARM: tegra: Add header file for pinctrl constants

> Defines pincontrol constants to use from Tegra's DTS file

Perhaps: "This new header file defines..."? "DTS files"

> for tegra pincontrol properties option.

"Tegra"

> 
> Signed-off-by: Laxman Dewangan <ldewangan@nvidia.com>
> ---
>  include/dt-bindings/pinctrl/pinctrl-tegra.h |   65 +++++++++++++++++++++++++++
>  1 files changed, 65 insertions(+), 0 deletions(-)
>  create mode 100644 include/dt-bindings/pinctrl/pinctrl-tegra.h
> 
> diff --git a/include/dt-bindings/pinctrl/pinctrl-tegra.h b/include/dt-bindings/pinctrl/pinctrl-tegra.h
[...]
> +/* Pull up/down/normal */
> +#define TEGRA_PIN_PUPD_NORMAL				0
> +#define TEGRA_PIN_PUPD_PULL_DOWN			1
> +#define TEGRA_PIN_PUPD_PULL_UP				2

Perhaps these would be easier to use as:

	#define TEGRA_PIN_PULL_NONE	0
	#define TEGRA_PIN_PULL_DOWN	1
	#define TEGRA_PIN_PULL_UP	2

?

> +/* Tristate/normal */
> +#define TEGRA_PIN_NORMAL				0
> +#define TEGRA_PIN_TRISTATE				1
> +
> +/* Rcv Sel enable/disable */
> +#define TEGRA_PIN_RCV_SEL_DISABLE			0
> +#define TEGRA_PIN_RCV_SEL_ENABLE			1
> +
> +/* Lock enable/disable */
> +#define TEGRA_PIN_LOCK_DISABLE				0
> +#define TEGRA_PIN_LOCK_ENABLE				1
> +
> +/* Open drain enable/disable */
> +#define TEGRA_PIN_OPEN_DRAIN_DISABLE			0
> +#define TEGRA_PIN_OPEN_DRAIN_ENABLE			1
> +
> +/* High speed mode */
> +#define TEGRA_PIN_DRIVE_HIGH_SPEED_MODE_DISABLE		0
> +#define TEGRA_PIN_DRIVE_HIGH_SPEED_MODE_ENABLE		1
> +
> +/* Schmitt enable/disable */
> +#define TEGRA_PIN_DRIVE_SCHMITT_DISABLE			0
> +#define TEGRA_PIN_DRIVE_SCHMITT_ENABLE			1

These are all boolean, so I wonder if perhaps we should be simply
defining a single pair and reuse that in different contexts:

	#define TEGRA_PIN_DISABLE	0
	#define TEGRA_PIN_ENABLE	1

The property names should provide enough context for them to be used
unambiguously.

Thierry
Laxman Dewangan Dec. 3, 2013, 6:04 a.m. UTC | #2
On Monday 02 December 2013 07:55 PM, Thierry Reding wrote:
> * PGP Signed by an unknown key
>
> On Mon, Dec 02, 2013 at 07:25:01PM +0530, Laxman Dewangan wrote:
>

> +
> +/* Schmitt enable/disable */
> +#define TEGRA_PIN_DRIVE_SCHMITT_DISABLE			0
> +#define TEGRA_PIN_DRIVE_SCHMITT_ENABLE			1
> These are all boolean, so I wonder if perhaps we should be simply
> defining a single pair and reuse that in different contexts:
>
> 	#define TEGRA_PIN_DISABLE	0
> 	#define TEGRA_PIN_ENABLE	1
>
> The property names should provide enough context for them to be used
> unambiguously.
>
>

I can make generic ENABLE/DISABLE macro as you suggested but datasheet 
says as 0=NORMAL, 1 = TRISTATE. and that's why I kept name very near to 
the datasheet.

Thanks,
Laxman
Stephen Warren Dec. 3, 2013, 8:05 p.m. UTC | #3
On 12/02/2013 06:55 AM, Laxman Dewangan wrote:
> Defines pincontrol constants to use from Tegra's DTS file
> for tegra pincontrol properties option.

> diff --git a/include/dt-bindings/pinctrl/pinctrl-tegra.h b/include/dt-bindings/pinctrl/pinctrl-tegra.h

> +/* Tristate/normal */
> +#define TEGRA_PIN_NORMAL				0
> +#define TEGRA_PIN_TRISTATE				1

"NORMAL" is a bit generic. "DRIVEN" might work better.

> +/* High speed mode */
> +#define TEGRA_PIN_DRIVE_HIGH_SPEED_MODE_DISABLE		0
> +#define TEGRA_PIN_DRIVE_HIGH_SPEED_MODE_ENABLE		1
> +
> +/* Schmitt enable/disable */
> +#define TEGRA_PIN_DRIVE_SCHMITT_DISABLE			0
> +#define TEGRA_PIN_DRIVE_SCHMITT_ENABLE			1

I expect you can remove "DRIVE_" from all those names to make things
shorter.
Stephen Warren Dec. 3, 2013, 8:06 p.m. UTC | #4
On 12/02/2013 06:55 AM, Laxman Dewangan wrote:
> Defines pincontrol constants to use from Tegra's DTS file
> for tegra pincontrol properties option.

> diff --git a/include/dt-bindings/pinctrl/pinctrl-tegra.h b/include/dt-bindings/pinctrl/pinctrl-tegra.h

> +/* Pull up/down/normal */
> +#define TEGRA_PIN_PUPD_NORMAL				0
> +#define TEGRA_PIN_PUPD_PULL_DOWN			1
> +#define TEGRA_PIN_PUPD_PULL_UP				2

... and s/NORMAL/NONE/ here too, I think. Some people might consider
some other value e.g. pull-up as normal:-)
Stephen Warren Dec. 3, 2013, 8:08 p.m. UTC | #5
On 12/02/2013 11:04 PM, Laxman Dewangan wrote:
> On Monday 02 December 2013 07:55 PM, Thierry Reding wrote:
>> * PGP Signed by an unknown key
>>
>> On Mon, Dec 02, 2013 at 07:25:01PM +0530, Laxman Dewangan wrote:
>>
> 
>> +
>> +/* Schmitt enable/disable */
>> +#define TEGRA_PIN_DRIVE_SCHMITT_DISABLE            0
>> +#define TEGRA_PIN_DRIVE_SCHMITT_ENABLE            1
>> These are all boolean, so I wonder if perhaps we should be simply
>> defining a single pair and reuse that in different contexts:
>>
>>     #define TEGRA_PIN_DISABLE    0
>>     #define TEGRA_PIN_ENABLE    1
>>
>> The property names should provide enough context for them to be used
>> unambiguously.
>>
>>
> 
> I can make generic ENABLE/DISABLE macro as you suggested but datasheet
> says as 0=NORMAL, 1 = TRISTATE. and that's why I kept name very near to
> the datasheet.

That documentation is relative to a specific field, whereas the
namespace for #defines is global. Hence, we may have to name #defines
using stricter rules than the TRM's field values, in order to make them
unambiguous.
Laxman Dewangan Dec. 4, 2013, 5:49 a.m. UTC | #6
On Wednesday 04 December 2013 01:38 AM, Stephen Warren wrote:
> On 12/02/2013 11:04 PM, Laxman Dewangan wrote:
>> On Monday 02 December 2013 07:55 PM, Thierry Reding wrote:
>>> * PGP Signed by an unknown key
>>>
>>> On Mon, Dec 02, 2013 at 07:25:01PM +0530, Laxman Dewangan wrote:
>>>
>>> +
>>> +/* Schmitt enable/disable */
>>> +#define TEGRA_PIN_DRIVE_SCHMITT_DISABLE            0
>>> +#define TEGRA_PIN_DRIVE_SCHMITT_ENABLE            1
>>> These are all boolean, so I wonder if perhaps we should be simply
>>> defining a single pair and reuse that in different contexts:
>>>
>>>      #define TEGRA_PIN_DISABLE    0
>>>      #define TEGRA_PIN_ENABLE    1
>>>
>>> The property names should provide enough context for them to be used
>>> unambiguously.
>>>
>>>
>> I can make generic ENABLE/DISABLE macro as you suggested but datasheet
>> says as 0=NORMAL, 1 = TRISTATE. and that's why I kept name very near to
>> the datasheet.
> That documentation is relative to a specific field, whereas the
> namespace for #defines is global. Hence, we may have to name #defines
> using stricter rules than the TRM's field values, in order to make them
> unambiguous.

I send the V2 patches on which I have taken care of this.
Request you to please review.

Thanks,
Laxman
diff mbox

Patch

diff --git a/include/dt-bindings/pinctrl/pinctrl-tegra.h b/include/dt-bindings/pinctrl/pinctrl-tegra.h
new file mode 100644
index 0000000..c2bfa3f
--- /dev/null
+++ b/include/dt-bindings/pinctrl/pinctrl-tegra.h
@@ -0,0 +1,65 @@ 
+/*
+ * This header provides constants for TEGRA pinctrl bindings.
+ *
+ * Copyright (c) 2013, NVIDIA CORPORATION.  All rights reserved.
+ *
+ * Author: Laxman Dewangan <ldewangan@nvidia.com>
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms and conditions of the GNU General Public License,
+ * version 2, as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
+ * more details.
+ */
+
+#ifndef _DT_BINDINGS_PINCTRL_TEGRA_H
+#define _DT_BINDINGS_PINCTRL_TEGRA_H
+
+/* Input/output */
+#define TEGRA_PIN_OUTPUT				0
+#define TEGRA_PIN_INPUT					1
+
+/* Pull up/down/normal */
+#define TEGRA_PIN_PUPD_NORMAL				0
+#define TEGRA_PIN_PUPD_PULL_DOWN			1
+#define TEGRA_PIN_PUPD_PULL_UP				2
+
+/* Tristate/normal */
+#define TEGRA_PIN_NORMAL				0
+#define TEGRA_PIN_TRISTATE				1
+
+/* Rcv Sel enable/disable */
+#define TEGRA_PIN_RCV_SEL_DISABLE			0
+#define TEGRA_PIN_RCV_SEL_ENABLE			1
+
+/* Lock enable/disable */
+#define TEGRA_PIN_LOCK_DISABLE				0
+#define TEGRA_PIN_LOCK_ENABLE				1
+
+/* Open drain enable/disable */
+#define TEGRA_PIN_OPEN_DRAIN_DISABLE			0
+#define TEGRA_PIN_OPEN_DRAIN_ENABLE			1
+
+/* High speed mode */
+#define TEGRA_PIN_DRIVE_HIGH_SPEED_MODE_DISABLE		0
+#define TEGRA_PIN_DRIVE_HIGH_SPEED_MODE_ENABLE		1
+
+/* Schmitt enable/disable */
+#define TEGRA_PIN_DRIVE_SCHMITT_DISABLE			0
+#define TEGRA_PIN_DRIVE_SCHMITT_ENABLE			1
+
+/* Low power mode */
+#define TEGRA_PIN_LP_DRIVE_DIV_8			0
+#define TEGRA_PIN_LP_DRIVE_DIV_4			1
+#define TEGRA_PIN_LP_DRIVE_DIV_2			2
+#define TEGRA_PIN_LP_DRIVE_DIV_1			3
+
+#define TEGRA_PIN_SLEW_RATE_FASTEST			0
+#define TEGRA_PIN_SLEW_RATE_FAST			1
+#define TEGRA_PIN_SLEW_RATE_SLOW			2
+#define TEGRA_PIN_SLEW_RATE_SLOWEST			3
+
+#endif