[PATCH 0/4] gpio: davinci: Add keystone-k2g support and few clean ups

[PATCH 0/4] gpio: davinci: Add keystone-k2g support and few clean ups

[Keerthy]

The patch series adds support for keystone-k2g soc GPIO.
Also does a couple of important return handling which were missed
earlier in the driver probe.

This is based on Suman's:

http://marc.info/?l=linux-arm-kernel&m=150034845427555&w=2
http://marc.info/?l=linux-arm-kernel&m=150034856627618&w=2

The driver patches should have no dependency but the device Tree
will need the above set.

Keerthy (4):
  gpio: davinci: Use devm_gpiochip_add_data in place of
    gpiochip_add_data
  gpio: davinci: Handle the return value of davinci_gpio_irq_setup
    function
  gpio: davinci: Add a separate compatible for keystone-k2g soc
  ARM: dts: keystone-k2g-evm: Add gpio nodes

 .../devicetree/bindings/gpio/gpio-davinci.txt      |  3 +-
 arch/arm/boot/dts/keystone-k2g.dtsi                | 42 ++++++++++++++++++++++
 drivers/gpio/gpio-davinci.c                        | 21 +++++++++--
 3 files changed, 62 insertions(+), 4 deletions(-)

-- 
1.9.1

[PATCH 1/4] gpio: davinci: Use devm_gpiochip_add_data in place of gpiochip_add_data

[Keerthy]

Use the devm version of gpiochip_add_data and pass on the
return value. Reset the static variables to 0 before returning.

Signed-off-by: Keerthy <j-keerthy@ti.com>
---
 drivers/gpio/gpio-davinci.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/drivers/gpio/gpio-davinci.c b/drivers/gpio/gpio-davinci.c
index 65cb359..2c88054 100644
--- a/drivers/gpio/gpio-davinci.c
+++ b/drivers/gpio/gpio-davinci.c
@@ -166,7 +166,7 @@ static int davinci_gpio_get(struct gpio_chip *chip, unsigned offset)
 static int davinci_gpio_probe(struct platform_device *pdev)
 {
 	static int ctrl_num, bank_base;
-	int gpio, bank;
+	int gpio, bank, ret = 0;
 	unsigned ngpio, nbank;
 	struct davinci_gpio_controller *chips;
 	struct davinci_gpio_platform_data *pdata;
@@ -232,7 +232,13 @@ static int davinci_gpio_probe(struct platform_device *pdev)
 	for (gpio = 0, bank = 0; gpio < ngpio; gpio += 32, bank++)
 		chips->regs[bank] = gpio_base + offset_array[bank];
 
-	gpiochip_add_data(&chips->chip, chips);
+	ret = devm_gpiochip_add_data(dev, &chips->chip, chips);
+	if (ret) {
+		ctrl_num = 0;
+		bank_base = 0;
+		return ret;
+	}
+
 	platform_set_drvdata(pdev, chips);
 	davinci_gpio_irq_setup(pdev);
 	return 0;
-- 
1.9.1

[PATCH 1/4] gpio: davinci: Use devm_gpiochip_add_data in place of gpiochip_add_data

[Suman Anna]

Hi Keerthy,

On 07/18/2017 05:57 AM, Keerthy wrote:
> Use the devm version of gpiochip_add_data and pass on the
> return value. Reset the static variables to 0 before returning.
> 
> Signed-off-by: Keerthy <j-keerthy@ti.com>
> ---
>  drivers/gpio/gpio-davinci.c | 10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpio/gpio-davinci.c b/drivers/gpio/gpio-davinci.c
> index 65cb359..2c88054 100644
> --- a/drivers/gpio/gpio-davinci.c
> +++ b/drivers/gpio/gpio-davinci.c
> @@ -166,7 +166,7 @@ static int davinci_gpio_get(struct gpio_chip *chip, unsigned offset)
>  static int davinci_gpio_probe(struct platform_device *pdev)
>  {
>  	static int ctrl_num, bank_base;
> -	int gpio, bank;
> +	int gpio, bank, ret = 0;
>  	unsigned ngpio, nbank;
>  	struct davinci_gpio_controller *chips;
>  	struct davinci_gpio_platform_data *pdata;
> @@ -232,7 +232,13 @@ static int davinci_gpio_probe(struct platform_device *pdev)
>  	for (gpio = 0, bank = 0; gpio < ngpio; gpio += 32, bank++)
>  		chips->regs[bank] = gpio_base + offset_array[bank];
>  
> -	gpiochip_add_data(&chips->chip, chips);
> +	ret = devm_gpiochip_add_data(dev, &chips->chip, chips);
> +	if (ret) {
> +		ctrl_num = 0;
> +		bank_base = 0;

Hmm, this doesn't look right to me. These variables are defined as
static, and you are resetting them unconditionally. This should be an
issue when you have multiple devices and one of them fails.

regards
Suman

> +		return ret;
> +	}
> +
>  	platform_set_drvdata(pdev, chips);
>  	davinci_gpio_irq_setup(pdev);
>  	return 0;
> 

[PATCH 1/4] gpio: davinci: Use devm_gpiochip_add_data in place of gpiochip_add_data

[Keerthy]

On Tuesday 18 July 2017 10:20 PM, Suman Anna wrote:
> Hi Keerthy,
> 
> On 07/18/2017 05:57 AM, Keerthy wrote:
>> Use the devm version of gpiochip_add_data and pass on the
>> return value. Reset the static variables to 0 before returning.
>>
>> Signed-off-by: Keerthy <j-keerthy@ti.com>
>> ---
>>  drivers/gpio/gpio-davinci.c | 10 ++++++++--
>>  1 file changed, 8 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/gpio/gpio-davinci.c b/drivers/gpio/gpio-davinci.c
>> index 65cb359..2c88054 100644
>> --- a/drivers/gpio/gpio-davinci.c
>> +++ b/drivers/gpio/gpio-davinci.c
>> @@ -166,7 +166,7 @@ static int davinci_gpio_get(struct gpio_chip *chip, unsigned offset)
>>  static int davinci_gpio_probe(struct platform_device *pdev)
>>  {
>>  	static int ctrl_num, bank_base;
>> -	int gpio, bank;
>> +	int gpio, bank, ret = 0;
>>  	unsigned ngpio, nbank;
>>  	struct davinci_gpio_controller *chips;
>>  	struct davinci_gpio_platform_data *pdata;
>> @@ -232,7 +232,13 @@ static int davinci_gpio_probe(struct platform_device *pdev)
>>  	for (gpio = 0, bank = 0; gpio < ngpio; gpio += 32, bank++)
>>  		chips->regs[bank] = gpio_base + offset_array[bank];
>>  
>> -	gpiochip_add_data(&chips->chip, chips);
>> +	ret = devm_gpiochip_add_data(dev, &chips->chip, chips);
>> +	if (ret) {
>> +		ctrl_num = 0;
>> +		bank_base = 0;
> 
> Hmm, this doesn't look right to me. These variables are defined as
> static, and you are resetting them unconditionally. This should be an
> issue when you have multiple devices and one of them fails.

Agreed. With multiple instances this can reset the successful count also.

Upon failure in any iteration, the following should take care of those
static variables:

/* Revert the static variable increments */
        ctrl_num--;
        bank_base -= ngpio;

Thanks for the quick review!

Reagrds,
Keerthy

> 
> regards
> Suman
> 
>> +		return ret;
>> +	}
>> +
>>  	platform_set_drvdata(pdev, chips);
>>  	davinci_gpio_irq_setup(pdev);
>>  	return 0;
>>
> 

[PATCH 1/4] gpio: davinci: Use devm_gpiochip_add_data in place of gpiochip_add_data

[Johan Hovold]

On Tue, Jul 18, 2017 at 04:27:13PM +0530, Keerthy wrote:
> Use the devm version of gpiochip_add_data and pass on the
> return value. Reset the static variables to 0 before returning.

You need to describe not just what you do, but also why you it. In this
case, your fixing memory leaks and the gpio chip being left registered
if this driver is unbound.

Johan

[PATCH 1/4] gpio: davinci: Use devm_gpiochip_add_data in place of gpiochip_add_data

[Keerthy]

On Wednesday 19 July 2017 03:07 PM, Johan Hovold wrote:
> On Tue, Jul 18, 2017 at 04:27:13PM +0530, Keerthy wrote:
>> Use the devm version of gpiochip_add_data and pass on the
>> return value. Reset the static variables to 0 before returning.
> 
> You need to describe not just what you do, but also why you it. In this
> case, your fixing memory leaks and the gpio chip being left registered
> if this driver is unbound.

Sure i can add more description as mentioned above.

> 
> Johan
> 

[PATCH 2/4] gpio: davinci: Handle the return value of davinci_gpio_irq_setup function

[Keerthy]

Currently davinci_gpio_irq_setup return value is ignored. Handle the
return value appropriately.

Signed-off-by: Keerthy <j-keerthy@ti.com>
---
 drivers/gpio/gpio-davinci.c | 18 +++++++++++++-----
 1 file changed, 13 insertions(+), 5 deletions(-)

diff --git a/drivers/gpio/gpio-davinci.c b/drivers/gpio/gpio-davinci.c
index 2c88054..932f270 100644
--- a/drivers/gpio/gpio-davinci.c
+++ b/drivers/gpio/gpio-davinci.c
@@ -233,15 +233,23 @@ static int davinci_gpio_probe(struct platform_device *pdev)
 		chips->regs[bank] = gpio_base + offset_array[bank];
 
 	ret = devm_gpiochip_add_data(dev, &chips->chip, chips);
+	if (ret)
+		goto err;
+
+	platform_set_drvdata(pdev, chips);
+	ret = davinci_gpio_irq_setup(pdev);
 	if (ret) {
-		ctrl_num = 0;
-		bank_base = 0;
-		return ret;
+		platform_set_drvdata(pdev, NULL);
+		goto err;
 	}
 
-	platform_set_drvdata(pdev, chips);
-	davinci_gpio_irq_setup(pdev);
 	return 0;
+
+err:
+	ctrl_num = 0;
+	bank_base = 0;
+
+	return ret;
 }
 
 /*--------------------------------------------------------------------------*/
-- 
1.9.1

[PATCH 2/4] gpio: davinci: Handle the return value of davinci_gpio_irq_setup function

[Suman Anna]

Hi Keerthy,

On 07/18/2017 05:57 AM, Keerthy wrote:
> Currently davinci_gpio_irq_setup return value is ignored. Handle the
> return value appropriately.
> 
> Signed-off-by: Keerthy <j-keerthy@ti.com>
> ---
>  drivers/gpio/gpio-davinci.c | 18 +++++++++++++-----
>  1 file changed, 13 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpio/gpio-davinci.c b/drivers/gpio/gpio-davinci.c
> index 2c88054..932f270 100644
> --- a/drivers/gpio/gpio-davinci.c
> +++ b/drivers/gpio/gpio-davinci.c
> @@ -233,15 +233,23 @@ static int davinci_gpio_probe(struct platform_device *pdev)
>  		chips->regs[bank] = gpio_base + offset_array[bank];
>  
>  	ret = devm_gpiochip_add_data(dev, &chips->chip, chips);
> +	if (ret)
> +		goto err;
> +
> +	platform_set_drvdata(pdev, chips);
> +	ret = davinci_gpio_irq_setup(pdev);
>  	if (ret) {
> -		ctrl_num = 0;
> -		bank_base = 0;
> -		return ret;
> +		platform_set_drvdata(pdev, NULL);

This is not needed, driver core will set this automatically if probe fails.

> +		goto err;
>  	}
>  
> -	platform_set_drvdata(pdev, chips);
> -	davinci_gpio_irq_setup(pdev);
>  	return 0;
> +
> +err:
> +	ctrl_num = 0;
> +	bank_base = 0;

Same comments as on Patch 1.

regards
Suman

[PATCH 2/4] gpio: davinci: Handle the return value of davinci_gpio_irq_setup function

[Keerthy]

On Tuesday 18 July 2017 10:24 PM, Suman Anna wrote:
> Hi Keerthy,
> 
> On 07/18/2017 05:57 AM, Keerthy wrote:
>> Currently davinci_gpio_irq_setup return value is ignored. Handle the
>> return value appropriately.
>>
>> Signed-off-by: Keerthy <j-keerthy@ti.com>
>> ---
>>  drivers/gpio/gpio-davinci.c | 18 +++++++++++++-----
>>  1 file changed, 13 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/gpio/gpio-davinci.c b/drivers/gpio/gpio-davinci.c
>> index 2c88054..932f270 100644
>> --- a/drivers/gpio/gpio-davinci.c
>> +++ b/drivers/gpio/gpio-davinci.c
>> @@ -233,15 +233,23 @@ static int davinci_gpio_probe(struct platform_device *pdev)
>>  		chips->regs[bank] = gpio_base + offset_array[bank];
>>  
>>  	ret = devm_gpiochip_add_data(dev, &chips->chip, chips);
>> +	if (ret)
>> +		goto err;
>> +
>> +	platform_set_drvdata(pdev, chips);
>> +	ret = davinci_gpio_irq_setup(pdev);
>>  	if (ret) {
>> -		ctrl_num = 0;
>> -		bank_base = 0;
>> -		return ret;
>> +		platform_set_drvdata(pdev, NULL);
> 
> This is not needed, driver core will set this automatically if probe fails.

okay. I will remove this.

> 
>> +		goto err;
>>  	}
>>  
>> -	platform_set_drvdata(pdev, chips);
>> -	davinci_gpio_irq_setup(pdev);
>>  	return 0;
>> +
>> +err:
>> +	ctrl_num = 0;
>> +	bank_base = 0;
> 
> Same comments as on Patch 1.

Yup will fix this as i have done with Patch 1.

> 
> regards
> Suman
> 

[PATCH 2/4] gpio: davinci: Handle the return value of davinci_gpio_irq_setup function

[Johan Hovold]

On Tue, Jul 18, 2017 at 04:27:14PM +0530, Keerthy wrote:
> Currently davinci_gpio_irq_setup return value is ignored. Handle the
> return value appropriately.
>
> Signed-off-by: Keerthy <j-keerthy@ti.com>
> ---
>  drivers/gpio/gpio-davinci.c | 18 +++++++++++++-----
>  1 file changed, 13 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpio/gpio-davinci.c b/drivers/gpio/gpio-davinci.c
> index 2c88054..932f270 100644
> --- a/drivers/gpio/gpio-davinci.c
> +++ b/drivers/gpio/gpio-davinci.c
> @@ -233,15 +233,23 @@ static int davinci_gpio_probe(struct platform_device *pdev)
>  		chips->regs[bank] = gpio_base + offset_array[bank];
>  
>  	ret = devm_gpiochip_add_data(dev, &chips->chip, chips);
> +	if (ret)
> +		goto err;
> +
> +	platform_set_drvdata(pdev, chips);
> +	ret = davinci_gpio_irq_setup(pdev);
>  	if (ret) {
> -		ctrl_num = 0;
> -		bank_base = 0;
> -		return ret;
> +		platform_set_drvdata(pdev, NULL);
> +		goto err;
>  	}
>  
> -	platform_set_drvdata(pdev, chips);
> -	davinci_gpio_irq_setup(pdev);
>  	return 0;

There's a separate but related bug here too as the clk_prepare_enable()
in davinci_gpio_irq_setup() is never balanced on driver unbind.

Johan

[PATCH 2/4] gpio: davinci: Handle the return value of davinci_gpio_irq_setup function

[Keerthy]

On Wednesday 19 July 2017 04:40 PM, Johan Hovold wrote:
> On Tue, Jul 18, 2017 at 04:27:14PM +0530, Keerthy wrote:
>> Currently davinci_gpio_irq_setup return value is ignored. Handle the
>> return value appropriately.
>>
>> Signed-off-by: Keerthy <j-keerthy@ti.com>
>> ---
>>  drivers/gpio/gpio-davinci.c | 18 +++++++++++++-----
>>  1 file changed, 13 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/gpio/gpio-davinci.c b/drivers/gpio/gpio-davinci.c
>> index 2c88054..932f270 100644
>> --- a/drivers/gpio/gpio-davinci.c
>> +++ b/drivers/gpio/gpio-davinci.c
>> @@ -233,15 +233,23 @@ static int davinci_gpio_probe(struct platform_device *pdev)
>>  		chips->regs[bank] = gpio_base + offset_array[bank];
>>  
>>  	ret = devm_gpiochip_add_data(dev, &chips->chip, chips);
>> +	if (ret)
>> +		goto err;
>> +
>> +	platform_set_drvdata(pdev, chips);
>> +	ret = davinci_gpio_irq_setup(pdev);
>>  	if (ret) {
>> -		ctrl_num = 0;
>> -		bank_base = 0;
>> -		return ret;
>> +		platform_set_drvdata(pdev, NULL);
>> +		goto err;
>>  	}
>>  
>> -	platform_set_drvdata(pdev, chips);
>> -	davinci_gpio_irq_setup(pdev);
>>  	return 0;
> 
> There's a separate but related bug here too as the clk_prepare_enable()
> in davinci_gpio_irq_setup() is never balanced on driver unbind.

Yes Johan. I will send that as a separate patch.

> 
> Johan
> 

[PATCH 2/4] gpio: davinci: Handle the return value of davinci_gpio_irq_setup function

[Keerthy]

On Thursday 20 July 2017 12:14 PM, Keerthy wrote:
> 
> 
> On Wednesday 19 July 2017 04:40 PM, Johan Hovold wrote:
>> On Tue, Jul 18, 2017 at 04:27:14PM +0530, Keerthy wrote:
>>> Currently davinci_gpio_irq_setup return value is ignored. Handle the
>>> return value appropriately.
>>>
>>> Signed-off-by: Keerthy <j-keerthy@ti.com>
>>> ---
>>>  drivers/gpio/gpio-davinci.c | 18 +++++++++++++-----
>>>  1 file changed, 13 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/drivers/gpio/gpio-davinci.c b/drivers/gpio/gpio-davinci.c
>>> index 2c88054..932f270 100644
>>> --- a/drivers/gpio/gpio-davinci.c
>>> +++ b/drivers/gpio/gpio-davinci.c
>>> @@ -233,15 +233,23 @@ static int davinci_gpio_probe(struct platform_device *pdev)
>>>  		chips->regs[bank] = gpio_base + offset_array[bank];
>>>  
>>>  	ret = devm_gpiochip_add_data(dev, &chips->chip, chips);
>>> +	if (ret)
>>> +		goto err;
>>> +
>>> +	platform_set_drvdata(pdev, chips);
>>> +	ret = davinci_gpio_irq_setup(pdev);
>>>  	if (ret) {
>>> -		ctrl_num = 0;
>>> -		bank_base = 0;
>>> -		return ret;
>>> +		platform_set_drvdata(pdev, NULL);
>>> +		goto err;
>>>  	}
>>>  
>>> -	platform_set_drvdata(pdev, chips);
>>> -	davinci_gpio_irq_setup(pdev);
>>>  	return 0;
>>
>> There's a separate but related bug here too as the clk_prepare_enable()
>> in davinci_gpio_irq_setup() is never balanced on driver unbind.
> 
> Yes Johan. I will send that as a separate patch.

This is already fixed in the latest kernel:

commit 6dc0048cff988858254fcc26becfc1e9753efa79
Author: Arvind Yadav <arvind.yadav.cs@gmail.com>
Date:   Tue May 23 14:48:57 2017 +0530

Regards,
Keerthy
> 
>>
>> Johan
>>

[PATCH 2/4] gpio: davinci: Handle the return value of davinci_gpio_irq_setup function

[Johan Hovold]

On Thu, Jul 20, 2017 at 02:40:37PM +0530, Keerthy wrote:
> 
> 
> On Thursday 20 July 2017 12:14 PM, Keerthy wrote:
> > 
> > 
> > On Wednesday 19 July 2017 04:40 PM, Johan Hovold wrote:
> >> On Tue, Jul 18, 2017 at 04:27:14PM +0530, Keerthy wrote:
> >>> Currently davinci_gpio_irq_setup return value is ignored. Handle the
> >>> return value appropriately.
> >>>
> >>> Signed-off-by: Keerthy <j-keerthy@ti.com>
> >>> ---
> >>>  drivers/gpio/gpio-davinci.c | 18 +++++++++++++-----
> >>>  1 file changed, 13 insertions(+), 5 deletions(-)
> >>>
> >>> diff --git a/drivers/gpio/gpio-davinci.c b/drivers/gpio/gpio-davinci.c
> >>> index 2c88054..932f270 100644
> >>> --- a/drivers/gpio/gpio-davinci.c
> >>> +++ b/drivers/gpio/gpio-davinci.c
> >>> @@ -233,15 +233,23 @@ static int davinci_gpio_probe(struct platform_device *pdev)
> >>>  		chips->regs[bank] = gpio_base + offset_array[bank];
> >>>  
> >>>  	ret = devm_gpiochip_add_data(dev, &chips->chip, chips);
> >>> +	if (ret)
> >>> +		goto err;
> >>> +
> >>> +	platform_set_drvdata(pdev, chips);
> >>> +	ret = davinci_gpio_irq_setup(pdev);
> >>>  	if (ret) {
> >>> -		ctrl_num = 0;
> >>> -		bank_base = 0;
> >>> -		return ret;
> >>> +		platform_set_drvdata(pdev, NULL);
> >>> +		goto err;
> >>>  	}
> >>>  
> >>> -	platform_set_drvdata(pdev, chips);
> >>> -	davinci_gpio_irq_setup(pdev);
> >>>  	return 0;
> >>
> >> There's a separate but related bug here too as the clk_prepare_enable()
> >> in davinci_gpio_irq_setup() is never balanced on driver unbind.
> > 
> > Yes Johan. I will send that as a separate patch.
> 
> This is already fixed in the latest kernel:
> 
> commit 6dc0048cff988858254fcc26becfc1e9753efa79
> Author: Arvind Yadav <arvind.yadav.cs@gmail.com>
> Date:   Tue May 23 14:48:57 2017 +0530

That change only handles errors in davinci_gpio_irq_setup() (i.e. during
probe) and not the imbalance at driver unbind that I was referring to.

Johan

[PATCH 2/4] gpio: davinci: Handle the return value of davinci_gpio_irq_setup function

[Keerthy]

On Thursday 20 July 2017 03:20 PM, Johan Hovold wrote:
> On Thu, Jul 20, 2017 at 02:40:37PM +0530, Keerthy wrote:
>>
>>
>> On Thursday 20 July 2017 12:14 PM, Keerthy wrote:
>>>
>>>
>>> On Wednesday 19 July 2017 04:40 PM, Johan Hovold wrote:
>>>> On Tue, Jul 18, 2017 at 04:27:14PM +0530, Keerthy wrote:
>>>>> Currently davinci_gpio_irq_setup return value is ignored. Handle the
>>>>> return value appropriately.
>>>>>
>>>>> Signed-off-by: Keerthy <j-keerthy@ti.com>
>>>>> ---
>>>>>  drivers/gpio/gpio-davinci.c | 18 +++++++++++++-----
>>>>>  1 file changed, 13 insertions(+), 5 deletions(-)
>>>>>
>>>>> diff --git a/drivers/gpio/gpio-davinci.c b/drivers/gpio/gpio-davinci.c
>>>>> index 2c88054..932f270 100644
>>>>> --- a/drivers/gpio/gpio-davinci.c
>>>>> +++ b/drivers/gpio/gpio-davinci.c
>>>>> @@ -233,15 +233,23 @@ static int davinci_gpio_probe(struct platform_device *pdev)
>>>>>  		chips->regs[bank] = gpio_base + offset_array[bank];
>>>>>  
>>>>>  	ret = devm_gpiochip_add_data(dev, &chips->chip, chips);
>>>>> +	if (ret)
>>>>> +		goto err;
>>>>> +
>>>>> +	platform_set_drvdata(pdev, chips);
>>>>> +	ret = davinci_gpio_irq_setup(pdev);
>>>>>  	if (ret) {
>>>>> -		ctrl_num = 0;
>>>>> -		bank_base = 0;
>>>>> -		return ret;
>>>>> +		platform_set_drvdata(pdev, NULL);
>>>>> +		goto err;
>>>>>  	}
>>>>>  
>>>>> -	platform_set_drvdata(pdev, chips);
>>>>> -	davinci_gpio_irq_setup(pdev);
>>>>>  	return 0;
>>>>
>>>> There's a separate but related bug here too as the clk_prepare_enable()
>>>> in davinci_gpio_irq_setup() is never balanced on driver unbind.
>>>
>>> Yes Johan. I will send that as a separate patch.
>>
>> This is already fixed in the latest kernel:
>>
>> commit 6dc0048cff988858254fcc26becfc1e9753efa79
>> Author: Arvind Yadav <arvind.yadav.cs@gmail.com>
>> Date:   Tue May 23 14:48:57 2017 +0530
> 
> That change only handles errors in davinci_gpio_irq_setup() (i.e. during
> probe) and not the imbalance at driver unbind that I was referring to.

Okay got it. One more clk_unprepare_disable() call needs to be there in
probe err path.

> 
> Johan
> 

[PATCH 2/4] gpio: davinci: Handle the return value of davinci_gpio_irq_setup function

[Johan Hovold]

On Thu, Jul 20, 2017 at 03:32:27PM +0530, Keerthy wrote:
> On Thursday 20 July 2017 03:20 PM, Johan Hovold wrote:
> > On Thu, Jul 20, 2017 at 02:40:37PM +0530, Keerthy wrote:
> >> On Thursday 20 July 2017 12:14 PM, Keerthy wrote:
> >>> On Wednesday 19 July 2017 04:40 PM, Johan Hovold wrote:

> >>>> There's a separate but related bug here too as the clk_prepare_enable()
> >>>> in davinci_gpio_irq_setup() is never balanced on driver unbind.
> >>>
> >>> Yes Johan. I will send that as a separate patch.
> >>
> >> This is already fixed in the latest kernel:
> >>
> >> commit 6dc0048cff988858254fcc26becfc1e9753efa79
> >> Author: Arvind Yadav <arvind.yadav.cs@gmail.com>
> >> Date:   Tue May 23 14:48:57 2017 +0530
> > 
> > That change only handles errors in davinci_gpio_irq_setup() (i.e. during
> > probe) and not the imbalance at driver unbind that I was referring to.
> 
> Okay got it. One more clk_unprepare_disable() call needs to be there in
> probe err path.

No, you need to balance it on driver unbind, that is, in a new remove()
callback.

Johan

[PATCH 2/4] gpio: davinci: Handle the return value of davinci_gpio_irq_setup function

[Keerthy]

On Thursday 20 July 2017 03:35 PM, Johan Hovold wrote:
> On Thu, Jul 20, 2017 at 03:32:27PM +0530, Keerthy wrote:
>> On Thursday 20 July 2017 03:20 PM, Johan Hovold wrote:
>>> On Thu, Jul 20, 2017 at 02:40:37PM +0530, Keerthy wrote:
>>>> On Thursday 20 July 2017 12:14 PM, Keerthy wrote:
>>>>> On Wednesday 19 July 2017 04:40 PM, Johan Hovold wrote:
> 
>>>>>> There's a separate but related bug here too as the clk_prepare_enable()
>>>>>> in davinci_gpio_irq_setup() is never balanced on driver unbind.
>>>>>
>>>>> Yes Johan. I will send that as a separate patch.
>>>>
>>>> This is already fixed in the latest kernel:
>>>>
>>>> commit 6dc0048cff988858254fcc26becfc1e9753efa79
>>>> Author: Arvind Yadav <arvind.yadav.cs@gmail.com>
>>>> Date:   Tue May 23 14:48:57 2017 +0530
>>>
>>> That change only handles errors in davinci_gpio_irq_setup() (i.e. during
>>> probe) and not the imbalance at driver unbind that I was referring to.
>>
>> Okay got it. One more clk_unprepare_disable() call needs to be there in
>> probe err path.
> 
> No, you need to balance it on driver unbind, that is, in a new remove()
> callback.

Okay yes davinci_gpio_irq_setup is the last call in probe so no need of
that in probe error path. I will add a new remove() to balance.

Thanks,
Keerthy

> 
> Johan
> 

[PATCH 2/4] gpio: davinci: Handle the return value of davinci_gpio_irq_setup function

[Grygorii Strashko]

On 07/20/2017 05:05 AM, Johan Hovold wrote:
> On Thu, Jul 20, 2017 at 03:32:27PM +0530, Keerthy wrote:
>> On Thursday 20 July 2017 03:20 PM, Johan Hovold wrote:
>>> On Thu, Jul 20, 2017 at 02:40:37PM +0530, Keerthy wrote:
>>>> On Thursday 20 July 2017 12:14 PM, Keerthy wrote:
>>>>> On Wednesday 19 July 2017 04:40 PM, Johan Hovold wrote:
> 
>>>>>> There's a separate but related bug here too as the clk_prepare_enable()
>>>>>> in davinci_gpio_irq_setup() is never balanced on driver unbind.
>>>>>
>>>>> Yes Johan. I will send that as a separate patch.
>>>>
>>>> This is already fixed in the latest kernel:
>>>>
>>>> commit 6dc0048cff988858254fcc26becfc1e9753efa79
>>>> Author: Arvind Yadav <arvind.yadav.cs@gmail.com>
>>>> Date:   Tue May 23 14:48:57 2017 +0530
>>>
>>> That change only handles errors in davinci_gpio_irq_setup() (i.e. during
>>> probe) and not the imbalance at driver unbind that I was referring to.
>>
>> Okay got it. One more clk_unprepare_disable() call needs to be there in
>> probe err path.
> 
> No, you need to balance it on driver unbind, that is, in a new remove()
> callback.
> 

Sry, but manual driver unbind for this driver is really smth unexpected ;(
So, I'm not sure if it need to be implemented and even yes - it should not be
a part of this patch. Probably, smth like "convert driver to be a module".

By the way, I've tried to unbind gpio-omap, result - failure (expected),
as unbind does not take into account module refcnt state.


-- 
regards,
-grygorii

[PATCH 2/4] gpio: davinci: Handle the return value of davinci_gpio_irq_setup function

[Keerthy]

On Friday 21 July 2017 03:04 AM, Grygorii Strashko wrote:
> 
> 
> On 07/20/2017 05:05 AM, Johan Hovold wrote:
>> On Thu, Jul 20, 2017 at 03:32:27PM +0530, Keerthy wrote:
>>> On Thursday 20 July 2017 03:20 PM, Johan Hovold wrote:
>>>> On Thu, Jul 20, 2017 at 02:40:37PM +0530, Keerthy wrote:
>>>>> On Thursday 20 July 2017 12:14 PM, Keerthy wrote:
>>>>>> On Wednesday 19 July 2017 04:40 PM, Johan Hovold wrote:
>>
>>>>>>> There's a separate but related bug here too as the clk_prepare_enable()
>>>>>>> in davinci_gpio_irq_setup() is never balanced on driver unbind.
>>>>>>
>>>>>> Yes Johan. I will send that as a separate patch.
>>>>>
>>>>> This is already fixed in the latest kernel:
>>>>>
>>>>> commit 6dc0048cff988858254fcc26becfc1e9753efa79
>>>>> Author: Arvind Yadav <arvind.yadav.cs@gmail.com>
>>>>> Date:   Tue May 23 14:48:57 2017 +0530
>>>>
>>>> That change only handles errors in davinci_gpio_irq_setup() (i.e. during
>>>> probe) and not the imbalance at driver unbind that I was referring to.
>>>
>>> Okay got it. One more clk_unprepare_disable() call needs to be there in
>>> probe err path.
>>
>> No, you need to balance it on driver unbind, that is, in a new remove()
>> callback.
>>
> 
> Sry, but manual driver unbind for this driver is really smth unexpected ;(
> So, I'm not sure if it need to be implemented and even yes - it should not be
> a part of this patch. Probably, smth like "convert driver to be a module".
> 

The GPIO_DAVINCI config is bool. Thanks for checking on that Grygorii.

> By the way, I've tried to unbind gpio-omap, result - failure (expected),
> as unbind does not take into account module refcnt state.

Okay.

> 
> 

[PATCH 2/4] gpio: davinci: Handle the return value of davinci_gpio_irq_setup function

[Johan Hovold]

On Thu, Jul 20, 2017 at 04:34:42PM -0500, Grygorii Strashko wrote:
> 
> 
> On 07/20/2017 05:05 AM, Johan Hovold wrote:
> > On Thu, Jul 20, 2017 at 03:32:27PM +0530, Keerthy wrote:
> >> On Thursday 20 July 2017 03:20 PM, Johan Hovold wrote:
> >>> On Thu, Jul 20, 2017 at 02:40:37PM +0530, Keerthy wrote:
> >>>> On Thursday 20 July 2017 12:14 PM, Keerthy wrote:
> >>>>> On Wednesday 19 July 2017 04:40 PM, Johan Hovold wrote:
> > 
> >>>>>> There's a separate but related bug here too as the clk_prepare_enable()
> >>>>>> in davinci_gpio_irq_setup() is never balanced on driver unbind.
> >>>>>
> >>>>> Yes Johan. I will send that as a separate patch.
> >>>>
> >>>> This is already fixed in the latest kernel:
> >>>>
> >>>> commit 6dc0048cff988858254fcc26becfc1e9753efa79
> >>>> Author: Arvind Yadav <arvind.yadav.cs@gmail.com>
> >>>> Date:   Tue May 23 14:48:57 2017 +0530
> >>>
> >>> That change only handles errors in davinci_gpio_irq_setup() (i.e. during
> >>> probe) and not the imbalance at driver unbind that I was referring to.
> >>
> >> Okay got it. One more clk_unprepare_disable() call needs to be there in
> >> probe err path.
> > 
> > No, you need to balance it on driver unbind, that is, in a new remove()
> > callback.
> > 
> 
> Sry, but manual driver unbind for this driver is really smth unexpected ;(

It certainly wouldn't be something often used (e.g. besides during
development) but that doesn't mean it should not be implemented.

> So, I'm not sure if it need to be implemented and even yes - it should not be
> a part of this patch.

That's why I said "separate, but related" above.

> Probably, smth like "convert driver to be a module".
>
> By the way, I've tried to unbind gpio-omap, result - failure (expected),
> as unbind does not take into account module refcnt state.

Indeed. We also have CONFIG_DEBUG_TEST_DRIVER_REMOVE which would try to
unbind this driver during probe if enabled.

Getting into the habit of properly cleaning up allocated and enabled
resources is only a good thing; it shows that the author has thought
this through and serves as documentation of what needs to be released in
both probe error paths and driver unbind callbacks.

Assumptions also change over time (e.g. deferred probe and
CONFIG_DEBUG_TEST_DRIVER_REMOVE), and by not taking such shortcuts we
are also preventing incomplete code from being copied and reproduced in
other drivers (e.g. on hotpluggable buses).

So, just add the remove callback (in a separate patch) and everything is
good.

Thanks,
Johan

[PATCH 3/4] gpio: davinci: Add a separate compatible for keystone-k2g soc

[Keerthy]

Add a separate compatible for keystone-k2g soc

Signed-off-by: Keerthy <j-keerthy@ti.com>
---
 Documentation/devicetree/bindings/gpio/gpio-davinci.txt | 3 ++-
 drivers/gpio/gpio-davinci.c                             | 1 +
 2 files changed, 3 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/gpio/gpio-davinci.txt b/Documentation/devicetree/bindings/gpio/gpio-davinci.txt
index 5079ba7..1a5c1a2 100644
--- a/Documentation/devicetree/bindings/gpio/gpio-davinci.txt
+++ b/Documentation/devicetree/bindings/gpio/gpio-davinci.txt
@@ -1,7 +1,8 @@
 Davinci/Keystone GPIO controller bindings
 
 Required Properties:
-- compatible: should be "ti,dm6441-gpio", "ti,keystone-gpio"
+- compatible: should be "ti,dm6441-gpio", "ti,keystone-gpio",
+	      "ti,keystone-k2g-gpio"
 
 - reg: Physical base address of the controller and the size of memory mapped
        registers.
diff --git a/drivers/gpio/gpio-davinci.c b/drivers/gpio/gpio-davinci.c
index 932f270..a8d8dd9 100644
--- a/drivers/gpio/gpio-davinci.c
+++ b/drivers/gpio/gpio-davinci.c
@@ -610,6 +610,7 @@ static int davinci_gpio_irq_setup(struct platform_device *pdev)
 static const struct of_device_id davinci_gpio_ids[] = {
 	{ .compatible = "ti,keystone-gpio", keystone_gpio_get_irq_chip},
 	{ .compatible = "ti,dm6441-gpio", davinci_gpio_get_irq_chip},
+	{ .compatible = "ti,keystone-k2g-gpio", keystone_gpio_get_irq_chip},
 	{ /* sentinel */ },
 };
 MODULE_DEVICE_TABLE(of, davinci_gpio_ids);
-- 
1.9.1

[PATCH 3/4] gpio: davinci: Add a separate compatible for keystone-k2g soc

[Sekhar Nori]

Hi Keerthy,

On Tuesday 18 July 2017 04:27 PM, Keerthy wrote:
> Add a separate compatible for keystone-k2g soc
> 
> Signed-off-by: Keerthy <j-keerthy@ti.com>
> ---
>  Documentation/devicetree/bindings/gpio/gpio-davinci.txt | 3 ++-
>  drivers/gpio/gpio-davinci.c                             | 1 +
>  2 files changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/devicetree/bindings/gpio/gpio-davinci.txt b/Documentation/devicetree/bindings/gpio/gpio-davinci.txt
> index 5079ba7..1a5c1a2 100644
> --- a/Documentation/devicetree/bindings/gpio/gpio-davinci.txt
> +++ b/Documentation/devicetree/bindings/gpio/gpio-davinci.txt
> @@ -1,7 +1,8 @@
>  Davinci/Keystone GPIO controller bindings
>  
>  Required Properties:
> -- compatible: should be "ti,dm6441-gpio", "ti,keystone-gpio"
> +- compatible: should be "ti,dm6441-gpio", "ti,keystone-gpio",
> +	      "ti,keystone-k2g-gpio"
>  
>  - reg: Physical base address of the controller and the size of memory mapped
>         registers.
> diff --git a/drivers/gpio/gpio-davinci.c b/drivers/gpio/gpio-davinci.c
> index 932f270..a8d8dd9 100644
> --- a/drivers/gpio/gpio-davinci.c
> +++ b/drivers/gpio/gpio-davinci.c
> @@ -610,6 +610,7 @@ static int davinci_gpio_irq_setup(struct platform_device *pdev)
>  static const struct of_device_id davinci_gpio_ids[] = {
>  	{ .compatible = "ti,keystone-gpio", keystone_gpio_get_irq_chip},
>  	{ .compatible = "ti,dm6441-gpio", davinci_gpio_get_irq_chip},
> +	{ .compatible = "ti,keystone-k2g-gpio", keystone_gpio_get_irq_chip},

You don't have to add compatible matching to the driver if you don't
need any special handling for K2G ATM. Your dts should have:

compatible = "ti,keystone-k2g-gpio", "ti,keystone-gpio";

this way, the driver continues to probe using "ti,keystone-gpio", and
when you really discover a need to do some special handling for K2G, a
kernel update will do without the need for a DT update.

Thanks,
Sekhar

[PATCH 3/4] gpio: davinci: Add a separate compatible for keystone-k2g soc

[Keerthy]

On Tuesday 18 July 2017 05:01 PM, Sekhar Nori wrote:
> Hi Keerthy,
> 
> On Tuesday 18 July 2017 04:27 PM, Keerthy wrote:
>> Add a separate compatible for keystone-k2g soc
>>
>> Signed-off-by: Keerthy <j-keerthy@ti.com>
>> ---
>>  Documentation/devicetree/bindings/gpio/gpio-davinci.txt | 3 ++-
>>  drivers/gpio/gpio-davinci.c                             | 1 +
>>  2 files changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/Documentation/devicetree/bindings/gpio/gpio-davinci.txt b/Documentation/devicetree/bindings/gpio/gpio-davinci.txt
>> index 5079ba7..1a5c1a2 100644
>> --- a/Documentation/devicetree/bindings/gpio/gpio-davinci.txt
>> +++ b/Documentation/devicetree/bindings/gpio/gpio-davinci.txt
>> @@ -1,7 +1,8 @@
>>  Davinci/Keystone GPIO controller bindings
>>  
>>  Required Properties:
>> -- compatible: should be "ti,dm6441-gpio", "ti,keystone-gpio"
>> +- compatible: should be "ti,dm6441-gpio", "ti,keystone-gpio",
>> +	      "ti,keystone-k2g-gpio"
>>  
>>  - reg: Physical base address of the controller and the size of memory mapped
>>         registers.
>> diff --git a/drivers/gpio/gpio-davinci.c b/drivers/gpio/gpio-davinci.c
>> index 932f270..a8d8dd9 100644
>> --- a/drivers/gpio/gpio-davinci.c
>> +++ b/drivers/gpio/gpio-davinci.c
>> @@ -610,6 +610,7 @@ static int davinci_gpio_irq_setup(struct platform_device *pdev)
>>  static const struct of_device_id davinci_gpio_ids[] = {
>>  	{ .compatible = "ti,keystone-gpio", keystone_gpio_get_irq_chip},
>>  	{ .compatible = "ti,dm6441-gpio", davinci_gpio_get_irq_chip},
>> +	{ .compatible = "ti,keystone-k2g-gpio", keystone_gpio_get_irq_chip},
> 
> You don't have to add compatible matching to the driver if you don't
> need any special handling for K2G ATM. Your dts should have:
> 
> compatible = "ti,keystone-k2g-gpio", "ti,keystone-gpio";
> 
> this way, the driver continues to probe using "ti,keystone-gpio", and
> when you really discover a need to do some special handling for K2G, a
> kernel update will do without the need for a DT update.

Sure that is better to do as currently i do not see any differences
between k2g and other k2 devices.

> 
> Thanks,
> Sekhar
> 

[PATCH 3/4] gpio: davinci: Add a separate compatible for keystone-k2g soc

[Rob Herring]

On Tue, Jul 18, 2017 at 04:27:15PM +0530, Keerthy wrote:
> Add a separate compatible for keystone-k2g soc
> 
> Signed-off-by: Keerthy <j-keerthy@ti.com>
> ---
>  Documentation/devicetree/bindings/gpio/gpio-davinci.txt | 3 ++-
>  drivers/gpio/gpio-davinci.c                             | 1 +
>  2 files changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/devicetree/bindings/gpio/gpio-davinci.txt b/Documentation/devicetree/bindings/gpio/gpio-davinci.txt
> index 5079ba7..1a5c1a2 100644
> --- a/Documentation/devicetree/bindings/gpio/gpio-davinci.txt
> +++ b/Documentation/devicetree/bindings/gpio/gpio-davinci.txt
> @@ -1,7 +1,8 @@
>  Davinci/Keystone GPIO controller bindings
>  
>  Required Properties:
> -- compatible: should be "ti,dm6441-gpio", "ti,keystone-gpio"
> +- compatible: should be "ti,dm6441-gpio", "ti,keystone-gpio",
> +	      "ti,keystone-k2g-gpio"

Reformat to one valid combination per line.

>  
>  - reg: Physical base address of the controller and the size of memory mapped
>         registers.
> diff --git a/drivers/gpio/gpio-davinci.c b/drivers/gpio/gpio-davinci.c
> index 932f270..a8d8dd9 100644
> --- a/drivers/gpio/gpio-davinci.c
> +++ b/drivers/gpio/gpio-davinci.c
> @@ -610,6 +610,7 @@ static int davinci_gpio_irq_setup(struct platform_device *pdev)
>  static const struct of_device_id davinci_gpio_ids[] = {
>  	{ .compatible = "ti,keystone-gpio", keystone_gpio_get_irq_chip},
>  	{ .compatible = "ti,dm6441-gpio", davinci_gpio_get_irq_chip},
> +	{ .compatible = "ti,keystone-k2g-gpio", keystone_gpio_get_irq_chip},
>  	{ /* sentinel */ },
>  };
>  MODULE_DEVICE_TABLE(of, davinci_gpio_ids);
> -- 
> 1.9.1
> 

[PATCH 3/4] gpio: davinci: Add a separate compatible for keystone-k2g soc

[Keerthy]

On Monday 24 July 2017 11:54 PM, Rob Herring wrote:
> On Tue, Jul 18, 2017 at 04:27:15PM +0530, Keerthy wrote:
>> Add a separate compatible for keystone-k2g soc
>>
>> Signed-off-by: Keerthy <j-keerthy@ti.com>
>> ---
>>  Documentation/devicetree/bindings/gpio/gpio-davinci.txt | 3 ++-
>>  drivers/gpio/gpio-davinci.c                             | 1 +
>>  2 files changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/Documentation/devicetree/bindings/gpio/gpio-davinci.txt b/Documentation/devicetree/bindings/gpio/gpio-davinci.txt
>> index 5079ba7..1a5c1a2 100644
>> --- a/Documentation/devicetree/bindings/gpio/gpio-davinci.txt
>> +++ b/Documentation/devicetree/bindings/gpio/gpio-davinci.txt
>> @@ -1,7 +1,8 @@
>>  Davinci/Keystone GPIO controller bindings
>>  
>>  Required Properties:
>> -- compatible: should be "ti,dm6441-gpio", "ti,keystone-gpio"
>> +- compatible: should be "ti,dm6441-gpio", "ti,keystone-gpio",
>> +	      "ti,keystone-k2g-gpio"
> 
> Reformat to one valid combination per line.

Sure Rob. Thanks for reviewing.

> 
>>  
>>  - reg: Physical base address of the controller and the size of memory mapped
>>         registers.
>> diff --git a/drivers/gpio/gpio-davinci.c b/drivers/gpio/gpio-davinci.c
>> index 932f270..a8d8dd9 100644
>> --- a/drivers/gpio/gpio-davinci.c
>> +++ b/drivers/gpio/gpio-davinci.c
>> @@ -610,6 +610,7 @@ static int davinci_gpio_irq_setup(struct platform_device *pdev)
>>  static const struct of_device_id davinci_gpio_ids[] = {
>>  	{ .compatible = "ti,keystone-gpio", keystone_gpio_get_irq_chip},
>>  	{ .compatible = "ti,dm6441-gpio", davinci_gpio_get_irq_chip},
>> +	{ .compatible = "ti,keystone-k2g-gpio", keystone_gpio_get_irq_chip},
>>  	{ /* sentinel */ },
>>  };
>>  MODULE_DEVICE_TABLE(of, davinci_gpio_ids);
>> -- 
>> 1.9.1
>>

[PATCH 4/4] ARM: dts: keystone-k2g-evm: Add gpio nodes

[Keerthy]

keystone-k2g has 2 instances of gpio. The first one has all the 144 GPIOs
functional( 9 banks with 16 gpios = 144). The second instance has
only the GPIO0:GPIO67 functional and rest are marked reserved.

Signed-off-by: Keerthy <j-keerthy@ti.com>
---
 arch/arm/boot/dts/keystone-k2g.dtsi | 42 +++++++++++++++++++++++++++++++++++++
 1 file changed, 42 insertions(+)

diff --git a/arch/arm/boot/dts/keystone-k2g.dtsi b/arch/arm/boot/dts/keystone-k2g.dtsi
index bf4d1fa..58ac3db 100644
--- a/arch/arm/boot/dts/keystone-k2g.dtsi
+++ b/arch/arm/boot/dts/keystone-k2g.dtsi
@@ -15,6 +15,7 @@
 
 #include <dt-bindings/interrupt-controller/arm-gic.h>
 #include <dt-bindings/pinctrl/keystone.h>
+#include <dt-bindings/gpio/gpio.h>
 
 / {
 	compatible = "ti,k2g","ti,keystone";
@@ -168,5 +169,46 @@
 				#reset-cells = <2>;
 			};
 		};
+
+		gpio0: gpio at 2603000 {
+			compatible = "ti,keystone-k2g-gpio";
+			reg = <0x02603000 0x100>;
+			gpio-controller;
+			#gpio-cells = <2>;
+
+			interrupts = <GIC_SPI 432 IRQ_TYPE_EDGE_RISING>,
+					<GIC_SPI 433 IRQ_TYPE_EDGE_RISING>,
+					<GIC_SPI 434 IRQ_TYPE_EDGE_RISING>,
+					<GIC_SPI 435 IRQ_TYPE_EDGE_RISING>,
+					<GIC_SPI 436 IRQ_TYPE_EDGE_RISING>,
+					<GIC_SPI 437 IRQ_TYPE_EDGE_RISING>,
+					<GIC_SPI 438 IRQ_TYPE_EDGE_RISING>,
+					<GIC_SPI 439 IRQ_TYPE_EDGE_RISING>,
+					<GIC_SPI 440 IRQ_TYPE_EDGE_RISING>;
+			interrupt-controller;
+			#interrupt-cells = <2>;
+			ti,ngpio = <144>;
+			ti,davinci-gpio-unbanked = <0>;
+			clocks = <&k2g_clks 0x001b 0x0>;
+			clock-names = "gpio";
+		};
+
+		gpio1: gpio at 260a000 {
+			compatible = "ti,keystone-k2g-gpio";
+			reg = <0x0260a000 0x100>;
+			gpio-controller;
+			#gpio-cells = <2>;
+			interrupts = <GIC_SPI 442 IRQ_TYPE_EDGE_RISING>,
+					<GIC_SPI 443 IRQ_TYPE_EDGE_RISING>,
+					<GIC_SPI 444 IRQ_TYPE_EDGE_RISING>,
+					<GIC_SPI 445 IRQ_TYPE_EDGE_RISING>,
+					<GIC_SPI 446 IRQ_TYPE_EDGE_RISING>;
+			interrupt-controller;
+			#interrupt-cells = <2>;
+			ti,ngpio = <68>;
+			ti,davinci-gpio-unbanked = <0>;
+			clocks = <&k2g_clks 0x001c 0x0>;
+			clock-names = "gpio";
+		};
 	};
 };
-- 
1.9.1

[PATCH 4/4] ARM: dts: keystone-k2g-evm: Add gpio nodes

[Suman Anna]

Hi Keerthy,

On 07/18/2017 05:57 AM, Keerthy wrote:
> keystone-k2g has 2 instances of gpio. The first one has all the 144 GPIOs

Please use 66AK2G for keystone-k2g.

> functional( 9 banks with 16 gpios = 144). The second instance has
> only the GPIO0:GPIO67 functional and rest are marked reserved.
> 
> Signed-off-by: Keerthy <j-keerthy@ti.com>
> ---
>  arch/arm/boot/dts/keystone-k2g.dtsi | 42 +++++++++++++++++++++++++++++++++++++
>  1 file changed, 42 insertions(+)
> 
> diff --git a/arch/arm/boot/dts/keystone-k2g.dtsi b/arch/arm/boot/dts/keystone-k2g.dtsi
> index bf4d1fa..58ac3db 100644
> --- a/arch/arm/boot/dts/keystone-k2g.dtsi
> +++ b/arch/arm/boot/dts/keystone-k2g.dtsi
> @@ -15,6 +15,7 @@
>  
>  #include <dt-bindings/interrupt-controller/arm-gic.h>
>  #include <dt-bindings/pinctrl/keystone.h>
> +#include <dt-bindings/gpio/gpio.h>
>  
>  / {
>  	compatible = "ti,k2g","ti,keystone";
> @@ -168,5 +169,46 @@
>  				#reset-cells = <2>;
>  			};
>  		};
> +
> +		gpio0: gpio at 2603000 {
> +			compatible = "ti,keystone-k2g-gpio";
> +			reg = <0x02603000 0x100>;
> +			gpio-controller;
> +			#gpio-cells = <2>;
> +
> +			interrupts = <GIC_SPI 432 IRQ_TYPE_EDGE_RISING>,
> +					<GIC_SPI 433 IRQ_TYPE_EDGE_RISING>,
> +					<GIC_SPI 434 IRQ_TYPE_EDGE_RISING>,
> +					<GIC_SPI 435 IRQ_TYPE_EDGE_RISING>,
> +					<GIC_SPI 436 IRQ_TYPE_EDGE_RISING>,
> +					<GIC_SPI 437 IRQ_TYPE_EDGE_RISING>,
> +					<GIC_SPI 438 IRQ_TYPE_EDGE_RISING>,
> +					<GIC_SPI 439 IRQ_TYPE_EDGE_RISING>,
> +					<GIC_SPI 440 IRQ_TYPE_EDGE_RISING>;
> +			interrupt-controller;
> +			#interrupt-cells = <2>;
> +			ti,ngpio = <144>;
> +			ti,davinci-gpio-unbanked = <0>;
> +			clocks = <&k2g_clks 0x001b 0x0>;
> +			clock-names = "gpio";

I don't see the clocks and clock-names documented in the binding.
Looking at davinci_gpio_irq_setup(), these are required, and a specific
clock name is what the driver is looking for. And you have different
semantics for this on K2G and non-K2G SoCs. Davinci platforms are using
non-DT clocks, so their DT nodes didn't have them.

regards
Suman

> +		};
> +
> +		gpio1: gpio at 260a000 {
> +			compatible = "ti,keystone-k2g-gpio";
> +			reg = <0x0260a000 0x100>;
> +			gpio-controller;
> +			#gpio-cells = <2>;
> +			interrupts = <GIC_SPI 442 IRQ_TYPE_EDGE_RISING>,
> +					<GIC_SPI 443 IRQ_TYPE_EDGE_RISING>,
> +					<GIC_SPI 444 IRQ_TYPE_EDGE_RISING>,
> +					<GIC_SPI 445 IRQ_TYPE_EDGE_RISING>,
> +					<GIC_SPI 446 IRQ_TYPE_EDGE_RISING>;
> +			interrupt-controller;
> +			#interrupt-cells = <2>;
> +			ti,ngpio = <68>;
> +			ti,davinci-gpio-unbanked = <0>;
> +			clocks = <&k2g_clks 0x001c 0x0>;
> +			clock-names = "gpio";
> +		};
>  	};
>  };
> 

[PATCH 4/4] ARM: dts: keystone-k2g-evm: Add gpio nodes

[Keerthy]

On Wednesday 19 July 2017 12:43 AM, Suman Anna wrote:
> Hi Keerthy,
> 
> On 07/18/2017 05:57 AM, Keerthy wrote:
>> keystone-k2g has 2 instances of gpio. The first one has all the 144 GPIOs
> 
> Please use 66AK2G for keystone-k2g.

Okay

> 
>> functional( 9 banks with 16 gpios = 144). The second instance has
>> only the GPIO0:GPIO67 functional and rest are marked reserved.
>>
>> Signed-off-by: Keerthy <j-keerthy@ti.com>
>> ---
>>  arch/arm/boot/dts/keystone-k2g.dtsi | 42 +++++++++++++++++++++++++++++++++++++
>>  1 file changed, 42 insertions(+)
>>
>> diff --git a/arch/arm/boot/dts/keystone-k2g.dtsi b/arch/arm/boot/dts/keystone-k2g.dtsi
>> index bf4d1fa..58ac3db 100644
>> --- a/arch/arm/boot/dts/keystone-k2g.dtsi
>> +++ b/arch/arm/boot/dts/keystone-k2g.dtsi
>> @@ -15,6 +15,7 @@
>>  
>>  #include <dt-bindings/interrupt-controller/arm-gic.h>
>>  #include <dt-bindings/pinctrl/keystone.h>
>> +#include <dt-bindings/gpio/gpio.h>
>>  
>>  / {
>>  	compatible = "ti,k2g","ti,keystone";
>> @@ -168,5 +169,46 @@
>>  				#reset-cells = <2>;
>>  			};
>>  		};
>> +
>> +		gpio0: gpio at 2603000 {
>> +			compatible = "ti,keystone-k2g-gpio";
>> +			reg = <0x02603000 0x100>;
>> +			gpio-controller;
>> +			#gpio-cells = <2>;
>> +
>> +			interrupts = <GIC_SPI 432 IRQ_TYPE_EDGE_RISING>,
>> +					<GIC_SPI 433 IRQ_TYPE_EDGE_RISING>,
>> +					<GIC_SPI 434 IRQ_TYPE_EDGE_RISING>,
>> +					<GIC_SPI 435 IRQ_TYPE_EDGE_RISING>,
>> +					<GIC_SPI 436 IRQ_TYPE_EDGE_RISING>,
>> +					<GIC_SPI 437 IRQ_TYPE_EDGE_RISING>,
>> +					<GIC_SPI 438 IRQ_TYPE_EDGE_RISING>,
>> +					<GIC_SPI 439 IRQ_TYPE_EDGE_RISING>,
>> +					<GIC_SPI 440 IRQ_TYPE_EDGE_RISING>;
>> +			interrupt-controller;
>> +			#interrupt-cells = <2>;
>> +			ti,ngpio = <144>;
>> +			ti,davinci-gpio-unbanked = <0>;
>> +			clocks = <&k2g_clks 0x001b 0x0>;
>> +			clock-names = "gpio";
> 
> I don't see the clocks and clock-names documented in the binding.
> Looking at davinci_gpio_irq_setup(), these are required, and a specific
> clock name is what the driver is looking for. And you have different
> semantics for this on K2G and non-K2G SoCs. Davinci platforms are using
> non-DT clocks, so their DT nodes didn't have them.

I will document the same.

> 
> regards
> Suman
> 
>> +		};
>> +
>> +		gpio1: gpio at 260a000 {
>> +			compatible = "ti,keystone-k2g-gpio";
>> +			reg = <0x0260a000 0x100>;
>> +			gpio-controller;
>> +			#gpio-cells = <2>;
>> +			interrupts = <GIC_SPI 442 IRQ_TYPE_EDGE_RISING>,
>> +					<GIC_SPI 443 IRQ_TYPE_EDGE_RISING>,
>> +					<GIC_SPI 444 IRQ_TYPE_EDGE_RISING>,
>> +					<GIC_SPI 445 IRQ_TYPE_EDGE_RISING>,
>> +					<GIC_SPI 446 IRQ_TYPE_EDGE_RISING>;
>> +			interrupt-controller;
>> +			#interrupt-cells = <2>;
>> +			ti,ngpio = <68>;
>> +			ti,davinci-gpio-unbanked = <0>;
>> +			clocks = <&k2g_clks 0x001c 0x0>;
>> +			clock-names = "gpio";
>> +		};
>>  	};
>>  };
>>
>