* [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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.