LinuxLists.cc - [PATCH 1/5] gpio: davinci: Remove unused member of davinci_gpio

[permalink] [raw]

Subject: [PATCH 5/5] gpio: davinci: Move driver local definitions to driver

These defines, structs and inline functions are used only internally by
the driver, they do not belong in platform_data. Move them.

Signed-off-by: Andrew F. Davis <[email protected]>
---
drivers/gpio/gpio-davinci.c | 28 ++++++++++++++++++++++
include/linux/platform_data/gpio-davinci.h | 28 ----------------------
2 files changed, 28 insertions(+), 28 deletions(-)

diff --git a/drivers/gpio/gpio-davinci.c b/drivers/gpio/gpio-davinci.c
index 121a7948f785..5c1564fcc24e 100644
--- a/drivers/gpio/gpio-davinci.c
+++ b/drivers/gpio/gpio-davinci.c
@@ -9,6 +9,7 @@
* the Free Software Foundation; either version 2 of the License, or
* (at your option) any later version.
*/
+
#include <linux/gpio/driver.h>
#include <linux/errno.h>
#include <linux/kernel.h>
@@ -24,6 +25,12 @@
#include <linux/platform_device.h>
#include <linux/platform_data/gpio-davinci.h>
#include <linux/irqchip/chained_irq.h>
+#include <linux/spinlock.h>
+
+#include <asm-generic/gpio.h>
+
+#define MAX_REGS_BANKS 5
+#define MAX_INT_PER_BANK 32

struct davinci_gpio_regs {
u32 dir;
@@ -45,6 +52,27 @@ typedef struct irq_chip *(*gpio_get_irq_chip_cb_t)(unsigned int irq);
static void __iomem *gpio_base;
static unsigned int offset_array[5] = {0x10, 0x38, 0x60, 0x88, 0xb0};

+struct davinci_gpio_irq_data {
+ void __iomem *regs;
+ struct davinci_gpio_controller *chip;
+ int bank_num;
+};
+
+struct davinci_gpio_controller {
+ struct gpio_chip chip;
+ struct irq_domain *irq_domain;
+ /* Serialize access to GPIO registers */
+ spinlock_t lock;
+ void __iomem *regs[MAX_REGS_BANKS];
+ int gpio_unbanked;
+ int irqs[MAX_INT_PER_BANK];
+};
+
+static inline u32 __gpio_mask(unsigned gpio)
+{
+ return 1 << (gpio % 32);
+}
+
static inline struct davinci_gpio_regs __iomem *irq2regs(struct irq_data *d)
{
struct davinci_gpio_regs __iomem *g;
diff --git a/include/linux/platform_data/gpio-davinci.h b/include/linux/platform_data/gpio-davinci.h
index 47695b342883..f92a47e18034 100644
--- a/include/linux/platform_data/gpio-davinci.h
+++ b/include/linux/platform_data/gpio-davinci.h
@@ -16,40 +16,12 @@
#ifndef __DAVINCI_GPIO_PLATFORM_H
#define __DAVINCI_GPIO_PLATFORM_H

-#include <linux/io.h>
-#include <linux/spinlock.h>
-
-#include <asm-generic/gpio.h>
-
-#define MAX_REGS_BANKS 5
-#define MAX_INT_PER_BANK 32
-
struct davinci_gpio_platform_data {
u32 ngpio;
u32 gpio_unbanked;
};

-struct davinci_gpio_irq_data {
- void __iomem *regs;
- struct davinci_gpio_controller *chip;
- int bank_num;
-};
-
-struct davinci_gpio_controller {
- struct gpio_chip chip;
- struct irq_domain *irq_domain;
- /* Serialize access to GPIO registers */
- spinlock_t lock;
- void __iomem *regs[MAX_REGS_BANKS];
- int gpio_unbanked;
- int irqs[MAX_INT_PER_BANK];
-};
-
/* Convert GPIO signal to GPIO pin number */
#define GPIO_TO_PIN(bank, gpio) (16 * (bank) + (gpio))

-static inline u32 __gpio_mask(unsigned gpio)
-{
- return 1 << (gpio % 32);
-}
#endif
--
2.18.0

2018-08-31 19:14:57

[permalink] [raw]

Subject: [PATCH 3/5] gpio: davinci: Allocate the correct amount of memory for controller

Previously we created a controller structure per bank of GPIO pins. This
has since been changed to one per controller, but the allocation size
was not changed. Fix this here.

This also leaves the variable 'nbank' unused, instead of removing it,
move it down and use it to clean up a loop. For loops with multiple
initializers and/or iteration expressions, especially ones that don't
use those loop counters are quite hard to follow, fix this.

Signed-off-by: Andrew F. Davis <[email protected]>
---
drivers/gpio/gpio-davinci.c | 10 ++++------
1 file changed, 4 insertions(+), 6 deletions(-)

diff --git a/drivers/gpio/gpio-davinci.c b/drivers/gpio/gpio-davinci.c
index 14d1729927d3..121a7948f785 100644
--- a/drivers/gpio/gpio-davinci.c
+++ b/drivers/gpio/gpio-davinci.c
@@ -165,7 +165,7 @@ davinci_gpio_get_pdata(struct platform_device *pdev)

static int davinci_gpio_probe(struct platform_device *pdev)
{
- int gpio, bank, i, ret = 0;
+ int bank, i, ret = 0;
unsigned int ngpio, nbank, nirq;
struct davinci_gpio_controller *chips;
struct davinci_gpio_platform_data *pdata;
@@ -204,10 +204,7 @@ static int davinci_gpio_probe(struct platform_device *pdev)
else
nirq = DIV_ROUND_UP(ngpio, 16);

- nbank = DIV_ROUND_UP(ngpio, 32);
- chips = devm_kcalloc(dev,
- nbank, sizeof(struct davinci_gpio_controller),
- GFP_KERNEL);
+ chips = devm_kzalloc(dev, sizeof(*chips), GFP_KERNEL);
if (!chips)
return -ENOMEM;

@@ -247,7 +244,8 @@ static int davinci_gpio_probe(struct platform_device *pdev)
#endif
spin_lock_init(&chips->lock);

- for (gpio = 0, bank = 0; gpio < ngpio; gpio += 32, bank++)
+ nbank = DIV_ROUND_UP(ngpio, 32);
+ for (bank = 0; bank < nbank; bank++)
chips->regs[bank] = gpio_base + offset_array[bank];

ret = devm_gpiochip_add_data(dev, &chips->chip, chips);
--
2.18.0

2018-08-31 19:15:07

[permalink] [raw]

Subject: [PATCH 4/5] gpio: davinci: Remove unneeded GPIO macro

This macro does nothing and has only one user, remove it.

Signed-off-by: Andrew F. Davis <[email protected]>
---
arch/arm/mach-davinci/board-neuros-osd2.c | 8 ++++----
include/linux/platform_data/gpio-davinci.h | 5 -----
2 files changed, 4 insertions(+), 9 deletions(-)

diff --git a/arch/arm/mach-davinci/board-neuros-osd2.c b/arch/arm/mach-davinci/board-neuros-osd2.c
index 353f9e5a1454..efdaa27241c5 100644
--- a/arch/arm/mach-davinci/board-neuros-osd2.c
+++ b/arch/arm/mach-davinci/board-neuros-osd2.c
@@ -130,10 +130,10 @@ static struct platform_device davinci_fb_device = {
};

static const struct gpio_led ntosd2_leds[] = {
- { .name = "led1_green", .gpio = GPIO(10), },
- { .name = "led1_red", .gpio = GPIO(11), },
- { .name = "led2_green", .gpio = GPIO(12), },
- { .name = "led2_red", .gpio = GPIO(13), },
+ { .name = "led1_green", .gpio = 10, },
+ { .name = "led1_red", .gpio = 11, },
+ { .name = "led2_green", .gpio = 12, },
+ { .name = "led2_red", .gpio = 13, },
};

static struct gpio_led_platform_data ntosd2_leds_data = {
diff --git a/include/linux/platform_data/gpio-davinci.h b/include/linux/platform_data/gpio-davinci.h
index b8e4957e7568..47695b342883 100644
--- a/include/linux/platform_data/gpio-davinci.h
+++ b/include/linux/platform_data/gpio-davinci.h
@@ -45,11 +45,6 @@ struct davinci_gpio_controller {
int irqs[MAX_INT_PER_BANK];
};

-/*
- * basic gpio routines
- */
-#define GPIO(X) (X) /* 0 <= X <= (DAVINCI_N_GPIO - 1) */
-
/* Convert GPIO signal to GPIO pin number */
#define GPIO_TO_PIN(bank, gpio) (16 * (bank) + (gpio))

--
2.18.0

2018-08-31 19:16:44

[permalink] [raw]

Subject: [PATCH 2/5] gpio: davinci: Use dev name for label and automatic base selection

Use dev_name to get a unique label and use -1 for a base to get our
selection automatically. We pull in all GPIOs per chip now so this
does not have the effect of out of order labels like before.

We do these both together so we can drop all the static data in one
patch. This also lets us normalize the return paths as we don't need
any cleanup after this change.

Signed-off-by: Andrew F. Davis <[email protected]>
---
drivers/gpio/gpio-davinci.c | 22 ++++------------------
1 file changed, 4 insertions(+), 18 deletions(-)

diff --git a/drivers/gpio/gpio-davinci.c b/drivers/gpio/gpio-davinci.c
index a5ece8ea79bc..14d1729927d3 100644
--- a/drivers/gpio/gpio-davinci.c
+++ b/drivers/gpio/gpio-davinci.c
@@ -41,7 +41,6 @@ struct davinci_gpio_regs {
typedef struct irq_chip *(*gpio_get_irq_chip_cb_t)(unsigned int irq);

#define BINTEN 0x8 /* GPIO Interrupt Per-Bank Enable Register */
-#define MAX_LABEL_SIZE 20

static void __iomem *gpio_base;
static unsigned int offset_array[5] = {0x10, 0x38, 0x60, 0x88, 0xb0};
@@ -166,14 +165,12 @@ davinci_gpio_get_pdata(struct platform_device *pdev)

static int davinci_gpio_probe(struct platform_device *pdev)
{
- static int ctrl_num, bank_base;
int gpio, bank, i, ret = 0;
unsigned int ngpio, nbank, nirq;
struct davinci_gpio_controller *chips;
struct davinci_gpio_platform_data *pdata;
struct device *dev = &pdev->dev;
struct resource *res;
- char label[MAX_LABEL_SIZE];

pdata = davinci_gpio_get_pdata(pdev);
if (!pdata) {
@@ -228,10 +225,7 @@ static int davinci_gpio_probe(struct platform_device *pdev)
}
}

- snprintf(label, MAX_LABEL_SIZE, "davinci_gpio.%d", ctrl_num++);
- chips->chip.label = devm_kstrdup(dev, label, GFP_KERNEL);
- if (!chips->chip.label)
- return -ENOMEM;
+ chips->chip.label = dev_name(dev);

chips->chip.direction_input = davinci_direction_in;
chips->chip.get = davinci_gpio_get;
@@ -239,7 +233,7 @@ static int davinci_gpio_probe(struct platform_device *pdev)
chips->chip.set = davinci_gpio_set;

chips->chip.ngpio = ngpio;
- chips->chip.base = bank_base;
+ chips->chip.base = -1;

#ifdef CONFIG_OF_GPIO
chips->chip.of_gpio_n_cells = 2;
@@ -252,28 +246,20 @@ static int davinci_gpio_probe(struct platform_device *pdev)
}
#endif
spin_lock_init(&chips->lock);
- bank_base += ngpio;

for (gpio = 0, bank = 0; gpio < ngpio; gpio += 32, bank++)
chips->regs[bank] = gpio_base + offset_array[bank];

ret = devm_gpiochip_add_data(dev, &chips->chip, chips);
if (ret)
- goto err;
+ return ret;

platform_set_drvdata(pdev, chips);
ret = davinci_gpio_irq_setup(pdev);
if (ret)
- goto err;
+ return ret;

return 0;
-
-err:
- /* Revert the static variable increments */
- ctrl_num--;
- bank_base -= ngpio;
-
- return ret;
}

/*--------------------------------------------------------------------------*/
--
2.18.0

2018-09-03 05:41:50

[permalink] [raw]

Subject: Re: [PATCH 2/5] gpio: davinci: Use dev name for label and automatic base selection

On Saturday 01 September 2018 12:43 AM, Andrew F. Davis wrote:
> Use dev_name to get a unique label and use -1 for a base to get our
> selection automatically. We pull in all GPIOs per chip now so this
> does not have the effect of out of order labels like before.
>
> We do these both together so we can drop all the static data in one
> patch. This also lets us normalize the return paths as we don't need
> any cleanup after this change.

echo 28 > /sys/class/gpio/export
/ # echo 28 > /sys/class/gpi[ 12.839205] export_store: invalid GPIO 28
o/export
echo 2 > /sys/class/gp[ 22.165728] export_store: invalid GPIO 2
io/export
/ # echo 1 > /sys/class/gp[ 25.961392] export_store: invalid GPIO 1
io/export
/ # echo 3 > /sys/class/gp[ 29.981918] export_store: invalid GPIO 3
io/export

Export fails with this patch. I am testing this on keystone-k2g-evm.

>
> Signed-off-by: Andrew F. Davis <[email protected]>
> ---
> drivers/gpio/gpio-davinci.c | 22 ++++------------------
> 1 file changed, 4 insertions(+), 18 deletions(-)
>
> diff --git a/drivers/gpio/gpio-davinci.c b/drivers/gpio/gpio-davinci.c
> index a5ece8ea79bc..14d1729927d3 100644
> --- a/drivers/gpio/gpio-davinci.c
> +++ b/drivers/gpio/gpio-davinci.c
> @@ -41,7 +41,6 @@ struct davinci_gpio_regs {
> typedef struct irq_chip *(*gpio_get_irq_chip_cb_t)(unsigned int irq);
>
> #define BINTEN 0x8 /* GPIO Interrupt Per-Bank Enable Register */
> -#define MAX_LABEL_SIZE 20
>
> static void __iomem *gpio_base;
> static unsigned int offset_array[5] = {0x10, 0x38, 0x60, 0x88, 0xb0};
> @@ -166,14 +165,12 @@ davinci_gpio_get_pdata(struct platform_device *pdev)
>
> static int davinci_gpio_probe(struct platform_device *pdev)
> {
> - static int ctrl_num, bank_base;
> int gpio, bank, i, ret = 0;
> unsigned int ngpio, nbank, nirq;
> struct davinci_gpio_controller *chips;
> struct davinci_gpio_platform_data *pdata;
> struct device *dev = &pdev->dev;
> struct resource *res;
> - char label[MAX_LABEL_SIZE];
>
> pdata = davinci_gpio_get_pdata(pdev);
> if (!pdata) {
> @@ -228,10 +225,7 @@ static int davinci_gpio_probe(struct platform_device *pdev)
> }
> }
>
> - snprintf(label, MAX_LABEL_SIZE, "davinci_gpio.%d", ctrl_num++);
> - chips->chip.label = devm_kstrdup(dev, label, GFP_KERNEL);
> - if (!chips->chip.label)
> - return -ENOMEM;
> + chips->chip.label = dev_name(dev);
>
> chips->chip.direction_input = davinci_direction_in;
> chips->chip.get = davinci_gpio_get;
> @@ -239,7 +233,7 @@ static int davinci_gpio_probe(struct platform_device *pdev)
> chips->chip.set = davinci_gpio_set;
>
> chips->chip.ngpio = ngpio;
> - chips->chip.base = bank_base;
> + chips->chip.base = -1;
>
> #ifdef CONFIG_OF_GPIO
> chips->chip.of_gpio_n_cells = 2;
> @@ -252,28 +246,20 @@ static int davinci_gpio_probe(struct platform_device *pdev)
> }
> #endif
> spin_lock_init(&chips->lock);
> - bank_base += ngpio;
>
> for (gpio = 0, bank = 0; gpio < ngpio; gpio += 32, bank++)
> chips->regs[bank] = gpio_base + offset_array[bank];
>
> ret = devm_gpiochip_add_data(dev, &chips->chip, chips);
> if (ret)
> - goto err;
> + return ret;
>
> platform_set_drvdata(pdev, chips);
> ret = davinci_gpio_irq_setup(pdev);
> if (ret)
> - goto err;
> + return ret;
>
> return 0;
> -
> -err:
> - /* Revert the static variable increments */
> - ctrl_num--;
> - bank_base -= ngpio;
> -
> - return ret;
> }
>
> /*--------------------------------------------------------------------------*/
>

2018-09-05 10:39:19

[permalink] [raw]

Subject: Re: [PATCH 2/5] gpio: davinci: Use dev name for label and automatic base selection

On Mon, Sep 3, 2018 at 7:40 AM Keerthy <[email protected]> wrote:
> On Saturday 01 September 2018 12:43 AM, Andrew F. Davis wrote:
> > Use dev_name to get a unique label and use -1 for a base to get our
> > selection automatically. We pull in all GPIOs per chip now so this
> > does not have the effect of out of order labels like before.
> >
> > We do these both together so we can drop all the static data in one
> > patch. This also lets us normalize the return paths as we don't need
> > any cleanup after this change.
>
> echo 28 > /sys/class/gpio/export
> / # echo 28 > /sys/class/gpi[ 12.839205] export_store: invalid GPIO 28
> o/export
> echo 2 > /sys/class/gp[ 22.165728] export_store: invalid GPIO 2
> io/export
> / # echo 1 > /sys/class/gp[ 25.961392] export_store: invalid GPIO 1
> io/export
> / # echo 3 > /sys/class/gp[ 29.981918] export_store: invalid GPIO 3
> io/export
>
> Export fails with this patch. I am testing this on keystone-k2g-evm.

I think the GPIO got a new number didn't it?

Did you check the gpio file in debugfs to see which number
it got.

This is sadly the global numberspace that we are tying to
get rid of (new apps/scripts should use the chardev).

Are there applications that rely on the sysfs ABI on DaVinci?

In that case base needs to be prerseved.

Yours,
Linus Walleij

2018-09-06 19:19:51

[permalink] [raw]

Subject: Re: [PATCH 2/5] gpio: davinci: Use dev name for label and automatic base selection

On Wednesday 05 September 2018 04:07 PM, Linus Walleij wrote:
> On Mon, Sep 3, 2018 at 7:40 AM Keerthy <[email protected]> wrote:
>> On Saturday 01 September 2018 12:43 AM, Andrew F. Davis wrote:
>>> Use dev_name to get a unique label and use -1 for a base to get our
>>> selection automatically. We pull in all GPIOs per chip now so this
>>> does not have the effect of out of order labels like before.
>>>
>>> We do these both together so we can drop all the static data in one
>>> patch. This also lets us normalize the return paths as we don't need
>>> any cleanup after this change.
>>
>> echo 28 > /sys/class/gpio/export
>> / # echo 28 > /sys/class/gpi[ 12.839205] export_store: invalid GPIO 28
>> o/export
>> echo 2 > /sys/class/gp[ 22.165728] export_store: invalid GPIO 2
>> io/export
>> / # echo 1 > /sys/class/gp[ 25.961392] export_store: invalid GPIO 1
>> io/export
>> / # echo 3 > /sys/class/gp[ 29.981918] export_store: invalid GPIO 3
>> io/export
>>
>> Export fails with this patch. I am testing this on keystone-k2g-evm.
>
> I think the GPIO got a new number didn't it?
>
> Did you check the gpio file in debugfs to see which number
> it got.

Okay now its numbered differently:

cat /sys/class/gpio/gpiochip340/ngpio
144

cat /sys/class/gpio/gpiochip272/ngpio
68

So gpio bank2 and bank1 have different gpio numbers. Is that acceptable?

>
> This is sadly the global numberspace that we are tying to
> get rid of (new apps/scripts should use the chardev).
>
> Are there applications that rely on the sysfs ABI on DaVinci?
>
> In that case base needs to be prerseved.
>
> Yours,
> Linus Walleij
>

2018-09-08 19:45:13

by Grygorii Strashko

[permalink] [raw]

Subject: Re: [PATCH 2/5] gpio: davinci: Use dev name for label and automatic base selection

On 09/06/2018 09:16 AM, Keerthy wrote:
>
>
> On Wednesday 05 September 2018 04:07 PM, Linus Walleij wrote:
>> On Mon, Sep 3, 2018 at 7:40 AM Keerthy <[email protected]> wrote:
>>> On Saturday 01 September 2018 12:43 AM, Andrew F. Davis wrote:
>>>> Use dev_name to get a unique label and use -1 for a base to get our
>>>> selection automatically. We pull in all GPIOs per chip now so this
>>>> does not have the effect of out of order labels like before.
>>>>
>>>> We do these both together so we can drop all the static data in one
>>>> patch. This also lets us normalize the return paths as we don't need
>>>> any cleanup after this change.
>>>
>>> echo 28 > /sys/class/gpio/export
>>> / # echo 28 > /sys/class/gpi[ 12.839205] export_store: invalid GPIO 28
>>> o/export
>>> echo 2 > /sys/class/gp[ 22.165728] export_store: invalid GPIO 2
>>> io/export
>>> / # echo 1 > /sys/class/gp[ 25.961392] export_store: invalid GPIO 1
>>> io/export
>>> / # echo 3 > /sys/class/gp[ 29.981918] export_store: invalid GPIO 3
>>> io/export
>>>
>>> Export fails with this patch. I am testing this on keystone-k2g-evm.
>>
>> I think the GPIO got a new number didn't it?
>>
>> Did you check the gpio file in debugfs to see which number
>> it got.
>
> Okay now its numbered differently:
>
> cat /sys/class/gpio/gpiochip340/ngpio
> 144
>
> cat /sys/class/gpio/gpiochip272/ngpio
> 68

could you or Andrew provide content of /debug/gpio before/after?
And ls /sys/class/gpio/?
>
> So gpio bank2 and bank1 have different gpio numbers. Is that acceptable?
>
>>
>> This is sadly the global numberspace that we are tying to
>> get rid of (new apps/scripts should use the chardev).
>>
>> Are there applications that rely on the sysfs ABI on DaVinci?
>>
>> In that case base needs to be prerseved.

Not only base, but label also - /sys/class/gpio/gpiochip0/label, as this is
the way to find proper GPIO chip in sysfs using legacy GPIO ABI.

Linus, this platform is old and most of the users do not use new ABI (chardev),
so we could try change this, but need to be prepared for regressions reports.

--
regards,
-grygorii

2018-09-10 02:49:07

[permalink] [raw]

Subject: Re: [PATCH 2/5] gpio: davinci: Use dev name for label and automatic base selection

On Sunday 09 September 2018 01:11 AM, Grygorii Strashko wrote:
>
>
> On 09/06/2018 09:16 AM, Keerthy wrote:
>>
>>
>> On Wednesday 05 September 2018 04:07 PM, Linus Walleij wrote:
>>> On Mon, Sep 3, 2018 at 7:40 AM Keerthy <[email protected]> wrote:
>>>> On Saturday 01 September 2018 12:43 AM, Andrew F. Davis wrote:
>>>>> Use dev_name to get a unique label and use -1 for a base to get our
>>>>> selection automatically. We pull in all GPIOs per chip now so this
>>>>> does not have the effect of out of order labels like before.
>>>>>
>>>>> We do these both together so we can drop all the static data in one
>>>>> patch. This also lets us normalize the return paths as we don't need
>>>>> any cleanup after this change.
>>>>
>>>> echo 28 > /sys/class/gpio/export
>>>> / # echo 28 > /sys/class/gpi[ 12.839205] export_store: invalid GPIO 28
>>>> o/export
>>>> echo 2 > /sys/class/gp[ 22.165728] export_store: invalid GPIO 2
>>>> io/export
>>>> / # echo 1 > /sys/class/gp[ 25.961392] export_store: invalid GPIO 1
>>>> io/export
>>>> / # echo 3 > /sys/class/gp[ 29.981918] export_store: invalid GPIO 3
>>>> io/export
>>>>
>>>> Export fails with this patch. I am testing this on keystone-k2g-evm.
>>>
>>> I think the GPIO got a new number didn't it?
>>>
>>> Did you check the gpio file in debugfs to see which number
>>> it got.
>>
>> Okay now its numbered differently:
>>
>> cat /sys/class/gpio/gpiochip340/ngpio
>> 144
>>
>> cat /sys/class/gpio/gpiochip272/ngpio
>> 68
>
> could you or Andrew provide content of /debug/gpio before/after?
> And ls /sys/class/gpio/?

Output on K2G:

Before
======

cat /debug/gpio
gpiochip1: GPIOs 0-143, parent: platform/2603000.gpio, davinci_gpio.0:

gpiochip2: GPIOs 144-211, parent: platform/260a000.gpio, davinci_gpio.1:
gpio-156 ( |cd ) in lo

gpiochip0: GPIOs 484-511, parent: platform/2620240.keystone_dsp_gpio,
2620240.keystone_dsp_gpio:

ls /sys/class/gpio/
export gpiochip0 gpiochip144 gpiochip484 unexport

cat /sys/class/gpio/gpiochip0/label
davinci_gpio.0

cat /sys/class/gpio/gpiochip144/label
davinci_gpio.1

cat /sys/class/gpio/gpiochip144/ngpio
68
/ # cat /sys/class/gpio/gpiochip0/ngpio
144

After
=====

cat /debug/gpio
gpiochip2: GPIOs 272-339, parent: platform/260a000.gpio, 260a000.gpio:
gpio-284 ( |cd ) in lo

gpiochip1: GPIOs 340-483, parent: platform/2603000.gpio, 2603000.gpio:

gpiochip0: GPIOs 484-511, parent: platform/2620240.keystone_dsp_gpio,
2620240.keystone_dsp_gpio:

ls /sys/class/gpio/
export gpiochip272 gpiochip340 gpiochip484 unexport

cat /sys/class/gpio/gpiochip340/label
2603000.gpio
/ # cat /sys/class/gpio/gpiochip272/label
260a000.gpio
/ # cat /sys/class/gpio/gpiochip272/label

cat /sys/class/gpio/gpiochip272/ngpio
68
/ # cat /sys/class/gpio/gpiochip340/ngpio
144

In the case of SoCs that support multiple instances of Davinci GPIO IPs
it is harder to figure out the right gpio number to export.

>>
>> So gpio bank2 and bank1 have different gpio numbers. Is that acceptable?
>>
>>>
>>> This is sadly the global numberspace that we are tying to
>>> get rid of (new apps/scripts should use the chardev).
>>>
>>> Are there applications that rely on the sysfs ABI on DaVinci?
>>>
>>> In that case base needs to be prerseved.
>
> Not only base, but label also - /sys/class/gpio/gpiochip0/label, as this is
> the way to find proper GPIO chip in sysfs using legacy GPIO ABI.
>
> Linus, this platform is old and most of the users do not use new ABI (chardev),
> so we could try change this, but need to be prepared for regressions reports.
>

Totally agree with this.

2018-09-10 07:26:47

[permalink] [raw]

Subject: Re: [PATCH 2/5] gpio: davinci: Use dev name for label and automatic base selection

On Sat, Sep 8, 2018 at 9:41 PM Grygorii Strashko
<[email protected]> wrote:
> On 09/06/2018 09:16 AM, Keerthy wrote:

> > Okay now its numbered differently:
> >
> > cat /sys/class/gpio/gpiochip340/ngpio
> > 144
> >
> > cat /sys/class/gpio/gpiochip272/ngpio
> > 68
(...)

So:

> >> Are there applications that rely on the sysfs ABI on DaVinci?
> >>
> >> In that case base needs to be prerseved.
>
> Not only base, but label also - /sys/class/gpio/gpiochip0/label, as this is
> the way to find proper GPIO chip in sysfs using legacy GPIO ABI.
>
> Linus, this platform is old and most of the users do not use new ABI (chardev),
> so we could try change this, but need to be prepared for regressions reports.

So what I'm trying to ask you guys is if there are applications that you
are aware of that use the old ABI, and who will upgrade their
kernels and be upset.

We must not break userspace, but as you know if a tree falls in the
forest and nobody is there to hear it, it doesn't make a sound.

If you're not aware of any such applications I guess we should try
changing this.

Yours,
Linus Walleij

2018-09-10 07:39:55

by Sekhar Nori

[permalink] [raw]

Subject: Re: [PATCH 2/5] gpio: davinci: Use dev name for label and automatic base selection

On Monday 10 September 2018 12:55 PM, Linus Walleij wrote:
> On Sat, Sep 8, 2018 at 9:41 PM Grygorii Strashko
> <[email protected]> wrote:
>> On 09/06/2018 09:16 AM, Keerthy wrote:
>
>>> Okay now its numbered differently:
>>>
>>> cat /sys/class/gpio/gpiochip340/ngpio
>>> 144
>>>
>>> cat /sys/class/gpio/gpiochip272/ngpio
>>> 68
> (...)
>
> So:
>
>>>> Are there applications that rely on the sysfs ABI on DaVinci?
>>>>
>>>> In that case base needs to be prerseved.
>>
>> Not only base, but label also - /sys/class/gpio/gpiochip0/label, as this is
>> the way to find proper GPIO chip in sysfs using legacy GPIO ABI.
>>
>> Linus, this platform is old and most of the users do not use new ABI (chardev),
>> so we could try change this, but need to be prepared for regressions reports.
>
> So what I'm trying to ask you guys is if there are applications that you
> are aware of that use the old ABI, and who will upgrade their
> kernels and be upset.
>
> We must not break userspace, but as you know if a tree falls in the
> forest and nobody is there to hear it, it doesn't make a sound.
>
> If you're not aware of any such applications I guess we should try
> changing this.

Personally I am not really aware of any application which relies on the
ABI. I may have some scripts, but I can adjust them. I am also copying
some frequent users of DaVinci in mainline for any comments from them.

Thanks,
Sekhar

2018-09-10 17:05:11

by David Lechner

[permalink] [raw]

Subject: Re: [PATCH 2/5] gpio: davinci: Use dev name for label and automatic base selection

On 09/10/2018 02:37 AM, Sekhar Nori wrote:
> On Monday 10 September 2018 12:55 PM, Linus Walleij wrote:
>> On Sat, Sep 8, 2018 at 9:41 PM Grygorii Strashko
>> <[email protected]> wrote:
>>> On 09/06/2018 09:16 AM, Keerthy wrote:
>>
>>>> Okay now its numbered differently:
>>>>
>>>> cat /sys/class/gpio/gpiochip340/ngpio
>>>> 144
>>>>
>>>> cat /sys/class/gpio/gpiochip272/ngpio
>>>> 68
>> (...)
>>
>> So:
>>
>>>>> Are there applications that rely on the sysfs ABI on DaVinci?
>>>>>
>>>>> In that case base needs to be prerseved.
>>>
>>> Not only base, but label also - /sys/class/gpio/gpiochip0/label, as this is
>>> the way to find proper GPIO chip in sysfs using legacy GPIO ABI.
>>>
>>> Linus, this platform is old and most of the users do not use new ABI (chardev),
>>> so we could try change this, but need to be prepared for regressions reports.
>>
>> So what I'm trying to ask you guys is if there are applications that you
>> are aware of that use the old ABI, and who will upgrade their
>> kernels and be upset.
>>
>> We must not break userspace, but as you know if a tree falls in the
>> forest and nobody is there to hear it, it doesn't make a sound.
>>
>> If you're not aware of any such applications I guess we should try
>> changing this.
>
> Personally I am not really aware of any application which relies on the
> ABI. I may have some scripts, but I can adjust them. I am also copying
> some frequent users of DaVinci in mainline for any comments from them.
>
> Thanks,
> Sekhar
>

It won't break anything that I am aware of.

2018-09-10 22:07:05

by Grygorii Strashko

[permalink] [raw]

Subject: Re: [PATCH 2/5] gpio: davinci: Use dev name for label and automatic base selection

On 09/09/2018 09:47 PM, Keerthy wrote:
>
>
> On Sunday 09 September 2018 01:11 AM, Grygorii Strashko wrote:
>>
>>
>> On 09/06/2018 09:16 AM, Keerthy wrote:
>>>
>>>
>>> On Wednesday 05 September 2018 04:07 PM, Linus Walleij wrote:
>>>> On Mon, Sep 3, 2018 at 7:40 AM Keerthy <[email protected]> wrote:
>>>>> On Saturday 01 September 2018 12:43 AM, Andrew F. Davis wrote:
>>>>>> Use dev_name to get a unique label and use -1 for a base to get our
>>>>>> selection automatically. We pull in all GPIOs per chip now so this
>>>>>> does not have the effect of out of order labels like before.
>>>>>>
>>>>>> We do these both together so we can drop all the static data in one
>>>>>> patch. This also lets us normalize the return paths as we don't need
>>>>>> any cleanup after this change.
>>>>>
>>>>> echo 28 > /sys/class/gpio/export
>>>>> / # echo 28 > /sys/class/gpi[ 12.839205] export_store: invalid GPIO 28
>>>>> o/export
>>>>> echo 2 > /sys/class/gp[ 22.165728] export_store: invalid GPIO 2
>>>>> io/export
>>>>> / # echo 1 > /sys/class/gp[ 25.961392] export_store: invalid GPIO 1
>>>>> io/export
>>>>> / # echo 3 > /sys/class/gp[ 29.981918] export_store: invalid GPIO 3
>>>>> io/export
>>>>>
>>>>> Export fails with this patch. I am testing this on keystone-k2g-evm.
>>>>
>>>> I think the GPIO got a new number didn't it?
>>>>
>>>> Did you check the gpio file in debugfs to see which number
>>>> it got.
>>>
>>> Okay now its numbered differently:
>>>
>>> cat /sys/class/gpio/gpiochip340/ngpio
>>> 144
>>>
>>> cat /sys/class/gpio/gpiochip272/ngpio
>>> 68
>>
>> could you or Andrew provide content of /debug/gpio before/after?
>> And ls /sys/class/gpio/?
>
> Output on K2G:
>
> Before
> ======
>
> cat /debug/gpio
> gpiochip1: GPIOs 0-143, parent: platform/2603000.gpio, davinci_gpio.0:
>
> gpiochip2: GPIOs 144-211, parent: platform/260a000.gpio, davinci_gpio.1:
> gpio-156 ( |cd ) in lo
>
> gpiochip0: GPIOs 484-511, parent: platform/2620240.keystone_dsp_gpio,
> 2620240.keystone_dsp_gpio:
>
> ls /sys/class/gpio/
> export gpiochip0 gpiochip144 gpiochip484 unexport
>
> cat /sys/class/gpio/gpiochip0/label
> davinci_gpio.0
>
> cat /sys/class/gpio/gpiochip144/label
> davinci_gpio.1
>
> cat /sys/class/gpio/gpiochip144/ngpio
> 68
> / # cat /sys/class/gpio/gpiochip0/ngpio
> 144
>
>
> After
> =====
>
> cat /debug/gpio
> gpiochip2: GPIOs 272-339, parent: platform/260a000.gpio, 260a000.gpio:
> gpio-284 ( |cd ) in lo
>
> gpiochip1: GPIOs 340-483, parent: platform/2603000.gpio, 2603000.gpio:
>
> gpiochip0: GPIOs 484-511, parent: platform/2620240.keystone_dsp_gpio,
> 2620240.keystone_dsp_gpio:
>
> ls /sys/class/gpio/
> export gpiochip272 gpiochip340 gpiochip484 unexport
>
>
> cat /sys/class/gpio/gpiochip340/label
> 2603000.gpio
> / # cat /sys/class/gpio/gpiochip272/label
> 260a000.gpio
> / # cat /sys/class/gpio/gpiochip272/label
>
> cat /sys/class/gpio/gpiochip272/ngpio
> 68
> / # cat /sys/class/gpio/gpiochip340/ngpio
> 144
>
> In the case of SoCs that support multiple instances of Davinci GPIO IPs
> it is harder to figure out the right gpio number to export.
>

Just to clarify above for all:
- for the first registered gpio chip
- if base >= 0 then gpiolib: try allocate gpios [base, base + ngpio]
else gpiolib: try allocate gpios [ARCH_NR_GPIOS - ngpio, ARCH_NR_GPIOS]

so for the "after" case we can see gpio chip base allocation in hi to low order

--
regards,
-grygorii

2018-09-18 19:25:50

[permalink] [raw]

Subject: Re: [PATCH 1/5] gpio: davinci: Remove unused member of davinci_gpio_controller

On Fri, Aug 31, 2018 at 12:13 PM Andrew F. Davis <[email protected]> wrote:

> This was added as part of the patch in the fixes below, but was
> not needed or used, remove this here.
>
> Fixes: 8e11047b8f3c ("gpio: davinci: Add support for multiple GPIO controllers")
> Signed-off-by: Andrew F. Davis <[email protected]>

Patch applied.

Yours,
Linus Walleij

2018-09-18 19:27:14

[permalink] [raw]

Subject: Re: [PATCH 2/5] gpio: davinci: Use dev name for label and automatic base selection

On Fri, Aug 31, 2018 at 12:13 PM Andrew F. Davis <[email protected]> wrote:

> Use dev_name to get a unique label and use -1 for a base to get our
> selection automatically. We pull in all GPIOs per chip now so this
> does not have the effect of out of order labels like before.
>
> We do these both together so we can drop all the static data in one
> patch. This also lets us normalize the return paths as we don't need
> any cleanup after this change.
>
> Signed-off-by: Andrew F. Davis <[email protected]>

I have tentatively applied this patch set as we seem to have rough
consensus that it will be OK.

We can always pull it out or revert it if things break.

Yours,
Linus Walleij

2018-09-19 04:08:34

[permalink] [raw]

Subject: Re: [PATCH 2/5] gpio: davinci: Use dev name for label and automatic base selection

On Wednesday 19 September 2018 12:56 AM, Linus Walleij wrote:
> On Fri, Aug 31, 2018 at 12:13 PM Andrew F. Davis <[email protected]> wrote:
>
>> Use dev_name to get a unique label and use -1 for a base to get our
>> selection automatically. We pull in all GPIOs per chip now so this
>> does not have the effect of out of order labels like before.
>>
>> We do these both together so we can drop all the static data in one
>> patch. This also lets us normalize the return paths as we don't need
>> any cleanup after this change.
>>
>> Signed-off-by: Andrew F. Davis <[email protected]>
>
> I have tentatively applied this patch set as we seem to have rough
> consensus that it will be OK.
>
> We can always pull it out or revert it if things break.

Okay. If that is the case then i have already tested the entire series.

Apart from gpio numbering different, i have tested toggling and
interrupts with k2g-evm and omapl138.

>
> Yours,
> Linus Walleij
>

2018-09-19 05:02:43

[permalink] [raw]

Subject: Re: [PATCH 3/5] gpio: davinci: Allocate the correct amount of memory for controller

On Saturday 01 September 2018 12:43 AM, Andrew F. Davis wrote:
> Previously we created a controller structure per bank of GPIO pins. This
> has since been changed to one per controller, but the allocation size
> was not changed. Fix this here.
>
> This also leaves the variable 'nbank' unused, instead of removing it,
> move it down and use it to clean up a loop. For loops with multiple
> initializers and/or iteration expressions, especially ones that don't
> use those loop counters are quite hard to follow, fix this.
>

Tested for gpio interrupts on k2g and da850-lcdk

Tested-by: Keerthy <[email protected]>
Acked-by: Keerthy <[email protected]>

> Signed-off-by: Andrew F. Davis <[email protected]>
> ---
> drivers/gpio/gpio-davinci.c | 10 ++++------
> 1 file changed, 4 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/gpio/gpio-davinci.c b/drivers/gpio/gpio-davinci.c
> index 14d1729927d3..121a7948f785 100644
> --- a/drivers/gpio/gpio-davinci.c
> +++ b/drivers/gpio/gpio-davinci.c
> @@ -165,7 +165,7 @@ davinci_gpio_get_pdata(struct platform_device *pdev)
>
> static int davinci_gpio_probe(struct platform_device *pdev)
> {
> - int gpio, bank, i, ret = 0;
> + int bank, i, ret = 0;
> unsigned int ngpio, nbank, nirq;
> struct davinci_gpio_controller *chips;
> struct davinci_gpio_platform_data *pdata;
> @@ -204,10 +204,7 @@ static int davinci_gpio_probe(struct platform_device *pdev)
> else
> nirq = DIV_ROUND_UP(ngpio, 16);
>
> - nbank = DIV_ROUND_UP(ngpio, 32);
> - chips = devm_kcalloc(dev,
> - nbank, sizeof(struct davinci_gpio_controller),
> - GFP_KERNEL);
> + chips = devm_kzalloc(dev, sizeof(*chips), GFP_KERNEL);
> if (!chips)
> return -ENOMEM;
>
> @@ -247,7 +244,8 @@ static int davinci_gpio_probe(struct platform_device *pdev)
> #endif
> spin_lock_init(&chips->lock);
>
> - for (gpio = 0, bank = 0; gpio < ngpio; gpio += 32, bank++)
> + nbank = DIV_ROUND_UP(ngpio, 32);
> + for (bank = 0; bank < nbank; bank++)
> chips->regs[bank] = gpio_base + offset_array[bank];
>
> ret = devm_gpiochip_add_data(dev, &chips->chip, chips);
>

2018-09-19 05:03:11

[permalink] [raw]

Subject: Re: [PATCH 4/5] gpio: davinci: Remove unneeded GPIO macro

On Saturday 01 September 2018 12:43 AM, Andrew F. Davis wrote:
> This macro does nothing and has only one user, remove it.

Tested for gpio interrupts on k2g and da850-lcdk

Tested-by: Keerthy <[email protected]>
Acked-by: Keerthy <[email protected]>

>
> Signed-off-by: Andrew F. Davis <[email protected]>
> ---
> arch/arm/mach-davinci/board-neuros-osd2.c | 8 ++++----
> include/linux/platform_data/gpio-davinci.h | 5 -----
> 2 files changed, 4 insertions(+), 9 deletions(-)
>
> diff --git a/arch/arm/mach-davinci/board-neuros-osd2.c b/arch/arm/mach-davinci/board-neuros-osd2.c
> index 353f9e5a1454..efdaa27241c5 100644
> --- a/arch/arm/mach-davinci/board-neuros-osd2.c
> +++ b/arch/arm/mach-davinci/board-neuros-osd2.c
> @@ -130,10 +130,10 @@ static struct platform_device davinci_fb_device = {
> };
>
> static const struct gpio_led ntosd2_leds[] = {
> - { .name = "led1_green", .gpio = GPIO(10), },
> - { .name = "led1_red", .gpio = GPIO(11), },
> - { .name = "led2_green", .gpio = GPIO(12), },
> - { .name = "led2_red", .gpio = GPIO(13), },
> + { .name = "led1_green", .gpio = 10, },
> + { .name = "led1_red", .gpio = 11, },
> + { .name = "led2_green", .gpio = 12, },
> + { .name = "led2_red", .gpio = 13, },
> };
>
> static struct gpio_led_platform_data ntosd2_leds_data = {
> diff --git a/include/linux/platform_data/gpio-davinci.h b/include/linux/platform_data/gpio-davinci.h
> index b8e4957e7568..47695b342883 100644
> --- a/include/linux/platform_data/gpio-davinci.h
> +++ b/include/linux/platform_data/gpio-davinci.h
> @@ -45,11 +45,6 @@ struct davinci_gpio_controller {
> int irqs[MAX_INT_PER_BANK];
> };
>
> -/*
> - * basic gpio routines
> - */
> -#define GPIO(X) (X) /* 0 <= X <= (DAVINCI_N_GPIO - 1) */
> -
> /* Convert GPIO signal to GPIO pin number */
> #define GPIO_TO_PIN(bank, gpio) (16 * (bank) + (gpio))
>
>

2018-09-19 05:04:07