* [U-Boot] [PATCH 1/2] gpio: tegra: remove unused type
@ 2015-09-25 16:44 Stephen Warren
2015-09-25 16:44 ` [U-Boot] [PATCH 2/2] gpio: tegra: use named constants Stephen Warren
2015-10-01 23:00 ` [U-Boot] [PATCH 1/2] gpio: tegra: remove unused type Simon Glass
0 siblings, 2 replies; 7+ messages in thread
From: Stephen Warren @ 2015-09-25 16:44 UTC (permalink / raw)
To: u-boot
From: Stephen Warren <swarren@nvidia.com>
These enum values aren't used anywhere. Remove them.
Signed-off-by: Stephen Warren <swarren@nvidia.com>
---
This series depends on my previous "ARM: tegra: don't enable GPIOs
until direction is set"
drivers/gpio/tegra_gpio.c | 7 -------
1 file changed, 7 deletions(-)
diff --git a/drivers/gpio/tegra_gpio.c b/drivers/gpio/tegra_gpio.c
index 2dfd02d62053..f9c06fe88b71 100644
--- a/drivers/gpio/tegra_gpio.c
+++ b/drivers/gpio/tegra_gpio.c
@@ -25,13 +25,6 @@
DECLARE_GLOBAL_DATA_PTR;
-enum {
- TEGRA_CMD_INFO,
- TEGRA_CMD_PORT,
- TEGRA_CMD_OUTPUT,
- TEGRA_CMD_INPUT,
-};
-
struct tegra_gpio_platdata {
struct gpio_ctlr_bank *bank;
const char *port_name; /* Name of port, e.g. "B" */
--
1.9.1
^ permalink raw reply related [flat|nested] 7+ messages in thread
* [U-Boot] [PATCH 2/2] gpio: tegra: use named constants
2015-09-25 16:44 [U-Boot] [PATCH 1/2] gpio: tegra: remove unused type Stephen Warren
@ 2015-09-25 16:44 ` Stephen Warren
2015-10-01 23:00 ` Simon Glass
2015-10-01 23:00 ` [U-Boot] [PATCH 1/2] gpio: tegra: remove unused type Simon Glass
1 sibling, 1 reply; 7+ messages in thread
From: Stephen Warren @ 2015-09-25 16:44 UTC (permalink / raw)
To: u-boot
From: Stephen Warren <swarren@nvidia.com>
In order to make it clear what the parameters to set_config() and
set_direction() mean, and similarly for the return values from the
respective get_*(), define named constants for these values.
Disassembly shows no diff in the generated code, except that the
order of the code in the branches of tegra_gpio_get_function() gets
modified without affecting behaviour.
Suggested-by: Tom Warren <twarren@nvidia.com>
Signed-off-by: Stephen Warren <swarren@nvidia.com>
---
drivers/gpio/tegra_gpio.c | 33 +++++++++++++++++++--------------
1 file changed, 19 insertions(+), 14 deletions(-)
diff --git a/drivers/gpio/tegra_gpio.c b/drivers/gpio/tegra_gpio.c
index f9c06fe88b71..8e880e276f0a 100644
--- a/drivers/gpio/tegra_gpio.c
+++ b/drivers/gpio/tegra_gpio.c
@@ -1,6 +1,6 @@
/*
* NVIDIA Tegra20 GPIO handling.
- * (C) Copyright 2010-2012
+ * (C) Copyright 2010-2012,2015
* NVIDIA Corporation <www.nvidia.com>
*
* SPDX-License-Identifier: GPL-2.0+
@@ -25,6 +25,11 @@
DECLARE_GLOBAL_DATA_PTR;
+static const int CONFIG_SFIO = 0;
+static const int CONFIG_GPIO = 1;
+static const int DIRECTION_INPUT = 0;
+static const int DIRECTION_OUTPUT = 1;
+
struct tegra_gpio_platdata {
struct gpio_ctlr_bank *bank;
const char *port_name; /* Name of port, e.g. "B" */
@@ -37,7 +42,7 @@ struct tegra_port_info {
int base_gpio; /* Port number for this port (0, 1,.., n-1) */
};
-/* Return config of pin 'gpio' as GPIO (1) or SFPIO (0) */
+/* Return config of pin 'gpio' as GPIO (1) or SFIO (0) */
static int get_config(unsigned gpio)
{
struct gpio_ctlr *ctlr = (struct gpio_ctlr *)NV_PA_GPIO_BASE;
@@ -46,15 +51,15 @@ static int get_config(unsigned gpio)
int type;
u = readl(&bank->gpio_config[GPIO_PORT(gpio)]);
- type = (u >> GPIO_BIT(gpio)) & 1;
+ type = (u >> GPIO_BIT(gpio)) & 1;
debug("get_config: port = %d, bit = %d is %s\n",
GPIO_FULLPORT(gpio), GPIO_BIT(gpio), type ? "GPIO" : "SFPIO");
- return type;
+ return type ? CONFIG_GPIO : CONFIG_SFIO;
}
-/* Config pin 'gpio' as GPIO or SFPIO, based on 'type' */
+/* Config pin 'gpio' as GPIO or SFIO, based on 'type' */
static void set_config(unsigned gpio, int type)
{
struct gpio_ctlr *ctlr = (struct gpio_ctlr *)NV_PA_GPIO_BASE;
@@ -65,7 +70,7 @@ static void set_config(unsigned gpio, int type)
GPIO_FULLPORT(gpio), GPIO_BIT(gpio), type ? "GPIO" : "SFPIO");
u = readl(&bank->gpio_config[GPIO_PORT(gpio)]);
- if (type) /* GPIO */
+ if (type != CONFIG_SFIO)
u |= 1 << GPIO_BIT(gpio);
else
u &= ~(1 << GPIO_BIT(gpio));
@@ -86,7 +91,7 @@ static int get_direction(unsigned gpio)
debug("get_direction: port = %d, bit = %d, %s\n",
GPIO_FULLPORT(gpio), GPIO_BIT(gpio), dir ? "OUT" : "IN");
- return dir;
+ return dir ? DIRECTION_OUTPUT : DIRECTION_INPUT;
}
/* Config GPIO pin 'gpio' as input or output (OE) as per 'output' */
@@ -100,7 +105,7 @@ static void set_direction(unsigned gpio, int output)
GPIO_FULLPORT(gpio), GPIO_BIT(gpio), output ? "OUT" : "IN");
u = readl(&bank->gpio_dir_out[GPIO_PORT(gpio)]);
- if (output)
+ if (output != DIRECTION_INPUT)
u |= 1 << GPIO_BIT(gpio);
else
u &= ~(1 << GPIO_BIT(gpio));
@@ -135,7 +140,7 @@ static int tegra_gpio_direction_input(struct udevice *dev, unsigned offset)
struct tegra_port_info *state = dev_get_priv(dev);
/* Configure GPIO direction as input. */
- set_direction(state->base_gpio + offset, 0);
+ set_direction(state->base_gpio + offset, DIRECTION_INPUT);
/* Enable the pin as a GPIO */
set_config(state->base_gpio + offset, 1);
@@ -154,7 +159,7 @@ static int tegra_gpio_direction_output(struct udevice *dev, unsigned offset,
set_level(gpio, value);
/* Configure GPIO direction as output. */
- set_direction(gpio, 1);
+ set_direction(gpio, DIRECTION_OUTPUT);
/* Enable the pin as a GPIO */
set_config(state->base_gpio + offset, 1);
@@ -199,18 +204,18 @@ void gpio_config_table(const struct tegra_gpio_config *config, int len)
for (i = 0; i < len; i++) {
switch (config[i].init) {
case TEGRA_GPIO_INIT_IN:
- set_direction(config[i].gpio, 0);
+ set_direction(config[i].gpio, DIRECTION_INPUT);
break;
case TEGRA_GPIO_INIT_OUT0:
set_level(config[i].gpio, 0);
- set_direction(config[i].gpio, 1);
+ set_direction(config[i].gpio, DIRECTION_OUTPUT);
break;
case TEGRA_GPIO_INIT_OUT1:
set_level(config[i].gpio, 1);
- set_direction(config[i].gpio, 1);
+ set_direction(config[i].gpio, DIRECTION_OUTPUT);
break;
}
- set_config(config[i].gpio, 1);
+ set_config(config[i].gpio, CONFIG_GPIO);
}
}
--
1.9.1
^ permalink raw reply related [flat|nested] 7+ messages in thread
* [U-Boot] [PATCH 2/2] gpio: tegra: use named constants
2015-09-25 16:44 ` [U-Boot] [PATCH 2/2] gpio: tegra: use named constants Stephen Warren
@ 2015-10-01 23:00 ` Simon Glass
2015-10-01 23:29 ` Stephen Warren
0 siblings, 1 reply; 7+ messages in thread
From: Simon Glass @ 2015-10-01 23:00 UTC (permalink / raw)
To: u-boot
Hi Stephen.
On Friday, 25 September 2015, Stephen Warren <swarren@wwwdotorg.org> wrote:
>
> From: Stephen Warren <swarren@nvidia.com>
>
> In order to make it clear what the parameters to set_config() and
> set_direction() mean, and similarly for the return values from the
> respective get_*(), define named constants for these values.
>
> Disassembly shows no diff in the generated code, except that the
> order of the code in the branches of tegra_gpio_get_function() gets
> modified without affecting behaviour.
>
> Suggested-by: Tom Warren <twarren@nvidia.com>
> Signed-off-by: Stephen Warren <swarren@nvidia.com>
> ---
> drivers/gpio/tegra_gpio.c | 33 +++++++++++++++++++--------------
> 1 file changed, 19 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/gpio/tegra_gpio.c b/drivers/gpio/tegra_gpio.c
> index f9c06fe88b71..8e880e276f0a 100644
> --- a/drivers/gpio/tegra_gpio.c
> +++ b/drivers/gpio/tegra_gpio.c
> @@ -1,6 +1,6 @@
> /*
> * NVIDIA Tegra20 GPIO handling.
> - * (C) Copyright 2010-2012
> + * (C) Copyright 2010-2012,2015
> * NVIDIA Corporation <www.nvidia.com>
> *
> * SPDX-License-Identifier: GPL-2.0+
> @@ -25,6 +25,11 @@
>
> DECLARE_GLOBAL_DATA_PTR;
>
> +static const int CONFIG_SFIO = 0;
> +static const int CONFIG_GPIO = 1;
> +static const int DIRECTION_INPUT = 0;
> +static const int DIRECTION_OUTPUT = 1;
> +
Why not use an enum?
>
> struct tegra_gpio_platdata {
> struct gpio_ctlr_bank *bank;
> const char *port_name; /* Name of port, e.g. "B" */
> @@ -37,7 +42,7 @@ struct tegra_port_info {
> int base_gpio; /* Port number for this port (0, 1,.., n-1) */
> };
>
> -/* Return config of pin 'gpio' as GPIO (1) or SFPIO (0) */
> +/* Return config of pin 'gpio' as GPIO (1) or SFIO (0) */
> static int get_config(unsigned gpio)
> {
> struct gpio_ctlr *ctlr = (struct gpio_ctlr *)NV_PA_GPIO_BASE;
> @@ -46,15 +51,15 @@ static int get_config(unsigned gpio)
> int type;
>
> u = readl(&bank->gpio_config[GPIO_PORT(gpio)]);
> - type = (u >> GPIO_BIT(gpio)) & 1;
> + type = (u >> GPIO_BIT(gpio)) & 1;
>
> debug("get_config: port = %d, bit = %d is %s\n",
> GPIO_FULLPORT(gpio), GPIO_BIT(gpio), type ? "GPIO" : "SFPIO");
>
> - return type;
> + return type ? CONFIG_GPIO : CONFIG_SFIO;
> }
>
> -/* Config pin 'gpio' as GPIO or SFPIO, based on 'type' */
> +/* Config pin 'gpio' as GPIO or SFIO, based on 'type' */
> static void set_config(unsigned gpio, int type)
> {
> struct gpio_ctlr *ctlr = (struct gpio_ctlr *)NV_PA_GPIO_BASE;
> @@ -65,7 +70,7 @@ static void set_config(unsigned gpio, int type)
> GPIO_FULLPORT(gpio), GPIO_BIT(gpio), type ? "GPIO" : "SFPIO");
>
> u = readl(&bank->gpio_config[GPIO_PORT(gpio)]);
> - if (type) /* GPIO */
> + if (type != CONFIG_SFIO)
> u |= 1 << GPIO_BIT(gpio);
> else
> u &= ~(1 << GPIO_BIT(gpio));
> @@ -86,7 +91,7 @@ static int get_direction(unsigned gpio)
> debug("get_direction: port = %d, bit = %d, %s\n",
> GPIO_FULLPORT(gpio), GPIO_BIT(gpio), dir ? "OUT" : "IN");
>
> - return dir;
> + return dir ? DIRECTION_OUTPUT : DIRECTION_INPUT;
> }
>
> /* Config GPIO pin 'gpio' as input or output (OE) as per 'output' */
> @@ -100,7 +105,7 @@ static void set_direction(unsigned gpio, int output)
> GPIO_FULLPORT(gpio), GPIO_BIT(gpio), output ? "OUT" : "IN");
>
> u = readl(&bank->gpio_dir_out[GPIO_PORT(gpio)]);
> - if (output)
> + if (output != DIRECTION_INPUT)
> u |= 1 << GPIO_BIT(gpio);
> else
> u &= ~(1 << GPIO_BIT(gpio));
> @@ -135,7 +140,7 @@ static int tegra_gpio_direction_input(struct udevice *dev, unsigned offset)
> struct tegra_port_info *state = dev_get_priv(dev);
>
> /* Configure GPIO direction as input. */
> - set_direction(state->base_gpio + offset, 0);
> + set_direction(state->base_gpio + offset, DIRECTION_INPUT);
>
> /* Enable the pin as a GPIO */
> set_config(state->base_gpio + offset, 1);
> @@ -154,7 +159,7 @@ static int tegra_gpio_direction_output(struct udevice *dev, unsigned offset,
> set_level(gpio, value);
>
> /* Configure GPIO direction as output. */
> - set_direction(gpio, 1);
> + set_direction(gpio, DIRECTION_OUTPUT);
>
> /* Enable the pin as a GPIO */
> set_config(state->base_gpio + offset, 1);
> @@ -199,18 +204,18 @@ void gpio_config_table(const struct tegra_gpio_config *config, int len)
> for (i = 0; i < len; i++) {
> switch (config[i].init) {
> case TEGRA_GPIO_INIT_IN:
> - set_direction(config[i].gpio, 0);
> + set_direction(config[i].gpio, DIRECTION_INPUT);
> break;
> case TEGRA_GPIO_INIT_OUT0:
> set_level(config[i].gpio, 0);
> - set_direction(config[i].gpio, 1);
> + set_direction(config[i].gpio, DIRECTION_OUTPUT);
> break;
> case TEGRA_GPIO_INIT_OUT1:
> set_level(config[i].gpio, 1);
> - set_direction(config[i].gpio, 1);
> + set_direction(config[i].gpio, DIRECTION_OUTPUT);
> break;
> }
> - set_config(config[i].gpio, 1);
> + set_config(config[i].gpio, CONFIG_GPIO);
> }
> }
>
> --
> 1.9.1
>
Regards,
Simon
^ permalink raw reply [flat|nested] 7+ messages in thread
* [U-Boot] [PATCH 1/2] gpio: tegra: remove unused type
2015-09-25 16:44 [U-Boot] [PATCH 1/2] gpio: tegra: remove unused type Stephen Warren
2015-09-25 16:44 ` [U-Boot] [PATCH 2/2] gpio: tegra: use named constants Stephen Warren
@ 2015-10-01 23:00 ` Simon Glass
1 sibling, 0 replies; 7+ messages in thread
From: Simon Glass @ 2015-10-01 23:00 UTC (permalink / raw)
To: u-boot
On Friday, 25 September 2015, Stephen Warren <swarren@wwwdotorg.org> wrote:
>
> From: Stephen Warren <swarren@nvidia.com>
>
> These enum values aren't used anywhere. Remove them.
>
> Signed-off-by: Stephen Warren <swarren@nvidia.com>
> ---
> This series depends on my previous "ARM: tegra: don't enable GPIOs
> until direction is set"
>
> drivers/gpio/tegra_gpio.c | 7 -------
> 1 file changed, 7 deletions(-)
>
Reviewed-by: Simon Glass <sjg@chromium.org>
^ permalink raw reply [flat|nested] 7+ messages in thread
* [U-Boot] [PATCH 2/2] gpio: tegra: use named constants
2015-10-01 23:00 ` Simon Glass
@ 2015-10-01 23:29 ` Stephen Warren
2015-10-03 14:14 ` Simon Glass
0 siblings, 1 reply; 7+ messages in thread
From: Stephen Warren @ 2015-10-01 23:29 UTC (permalink / raw)
To: u-boot
On 10/01/2015 05:00 PM, Simon Glass wrote:
> On Friday, 25 September 2015, Stephen Warren <swarren@wwwdotorg.org> wrote:
>> From: Stephen Warren <swarren@nvidia.com>
>>
>> In order to make it clear what the parameters to set_config() and
>> set_direction() mean, and similarly for the return values from the
>> respective get_*(), define named constants for these values.
>>
>> Disassembly shows no diff in the generated code, except that the
>> order of the code in the branches of tegra_gpio_get_function() gets
>> modified without affecting behaviour.
>> diff --git a/drivers/gpio/tegra_gpio.c b/drivers/gpio/tegra_gpio.c
>> +static const int CONFIG_SFIO = 0;
>> +static const int CONFIG_GPIO = 1;
>> +static const int DIRECTION_INPUT = 0;
>> +static const int DIRECTION_OUTPUT = 1;
>
> Why not use an enum?
I don't think it gives any benefit does it?
Doing so would entail 5 extra lines of overhead for the enum { and }
lines. I'd want to define two separate enum blocks since I dislike
putting logically unrelated enum values into a single enum definition.
Even if I didn't do that, it's still 2 lines of useless overhead to add
everything into a single enum.
^ permalink raw reply [flat|nested] 7+ messages in thread
* [U-Boot] [PATCH 2/2] gpio: tegra: use named constants
2015-10-01 23:29 ` Stephen Warren
@ 2015-10-03 14:14 ` Simon Glass
2015-10-03 19:17 ` Stephen Warren
0 siblings, 1 reply; 7+ messages in thread
From: Simon Glass @ 2015-10-03 14:14 UTC (permalink / raw)
To: u-boot
Hi Stephen,
On 2 October 2015 at 00:29, Stephen Warren <swarren@wwwdotorg.org> wrote:
> On 10/01/2015 05:00 PM, Simon Glass wrote:
>>
>> On Friday, 25 September 2015, Stephen Warren <swarren@wwwdotorg.org>
>> wrote:
>>>
>>> From: Stephen Warren <swarren@nvidia.com>
>>>
>>> In order to make it clear what the parameters to set_config() and
>>> set_direction() mean, and similarly for the return values from the
>>> respective get_*(), define named constants for these values.
>>>
>>> Disassembly shows no diff in the generated code, except that the
>>> order of the code in the branches of tegra_gpio_get_function() gets
>>> modified without affecting behaviour.
>
>
>>> diff --git a/drivers/gpio/tegra_gpio.c b/drivers/gpio/tegra_gpio.c
>
>
>>> +static const int CONFIG_SFIO = 0;
>>> +static const int CONFIG_GPIO = 1;
>>> +static const int DIRECTION_INPUT = 0;
>>> +static const int DIRECTION_OUTPUT = 1;
>>
>>
>> Why not use an enum?
>
>
> I don't think it gives any benefit does it?
>
> Doing so would entail 5 extra lines of overhead for the enum { and } lines.
> I'd want to define two separate enum blocks since I dislike putting
> logically unrelated enum values into a single enum definition. Even if I
> didn't do that, it's still 2 lines of useless overhead to add everything
> into a single enum.
It's just odd to use const int instead of enum I think. Particularly
as you are using capital letters.
Since it's Tegra code I'll leave it up to you.
Regards,
Simon
^ permalink raw reply [flat|nested] 7+ messages in thread
* [U-Boot] [PATCH 2/2] gpio: tegra: use named constants
2015-10-03 14:14 ` Simon Glass
@ 2015-10-03 19:17 ` Stephen Warren
0 siblings, 0 replies; 7+ messages in thread
From: Stephen Warren @ 2015-10-03 19:17 UTC (permalink / raw)
To: u-boot
On 10/03/2015 08:14 AM, Simon Glass wrote:
> Hi Stephen,
>
> On 2 October 2015 at 00:29, Stephen Warren <swarren@wwwdotorg.org> wrote:
>> On 10/01/2015 05:00 PM, Simon Glass wrote:
>>>
>>> On Friday, 25 September 2015, Stephen Warren <swarren@wwwdotorg.org>
>>> wrote:
>>>>
>>>> From: Stephen Warren <swarren@nvidia.com>
>>>>
>>>> In order to make it clear what the parameters to set_config() and
>>>> set_direction() mean, and similarly for the return values from the
>>>> respective get_*(), define named constants for these values.
>>>>
>>>> Disassembly shows no diff in the generated code, except that the
>>>> order of the code in the branches of tegra_gpio_get_function() gets
>>>> modified without affecting behaviour.
>>
>>
>>>> diff --git a/drivers/gpio/tegra_gpio.c b/drivers/gpio/tegra_gpio.c
>>
>>
>>>> +static const int CONFIG_SFIO = 0;
>>>> +static const int CONFIG_GPIO = 1;
>>>> +static const int DIRECTION_INPUT = 0;
>>>> +static const int DIRECTION_OUTPUT = 1;
>>>
>>>
>>> Why not use an enum?
>>
>>
>> I don't think it gives any benefit does it?
>>
>> Doing so would entail 5 extra lines of overhead for the enum { and } lines.
>> I'd want to define two separate enum blocks since I dislike putting
>> logically unrelated enum values into a single enum definition. Even if I
>> didn't do that, it's still 2 lines of useless overhead to add everything
>> into a single enum.
>
> It's just odd to use const int instead of enum I think.
What makes it odd?
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2015-10-03 19:17 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2015-09-25 16:44 [U-Boot] [PATCH 1/2] gpio: tegra: remove unused type Stephen Warren
2015-09-25 16:44 ` [U-Boot] [PATCH 2/2] gpio: tegra: use named constants Stephen Warren
2015-10-01 23:00 ` Simon Glass
2015-10-01 23:29 ` Stephen Warren
2015-10-03 14:14 ` Simon Glass
2015-10-03 19:17 ` Stephen Warren
2015-10-01 23:00 ` [U-Boot] [PATCH 1/2] gpio: tegra: remove unused type Simon Glass
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox