diff mbox

[V2,2/3] gpio: tegra: Remove the need of keeping device handle for gpio driver

Message ID 1461059020-25373-2-git-send-email-ldewangan@nvidia.com
State New
Headers show

Commit Message

Laxman Dewangan April 19, 2016, 9:43 a.m. UTC
Remove the file static device handle variable for keeping device handle
of driver as this is just required for error prints. The required device
handle are available from gpiochip structure.

Signed-off-by: Laxman Dewangan <ldewangan@nvidia.com>
---
 drivers/gpio/gpio-tegra.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

Comments

Thierry Reding April 19, 2016, 12:33 p.m. UTC | #1
On Tue, Apr 19, 2016 at 03:13:39PM +0530, Laxman Dewangan wrote:
> Remove the file static device handle variable for keeping device handle
> of driver as this is just required for error prints. The required device
> handle are available from gpiochip structure.
> 
> Signed-off-by: Laxman Dewangan <ldewangan@nvidia.com>
> ---
>  drivers/gpio/gpio-tegra.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpio/gpio-tegra.c b/drivers/gpio/gpio-tegra.c
> index 1b0c497..de022a9 100644
> --- a/drivers/gpio/gpio-tegra.c
> +++ b/drivers/gpio/gpio-tegra.c
> @@ -80,7 +80,6 @@ struct tegra_gpio_soc_config {
>  	u32 upper_offset;
>  };
>  
> -static struct device *dev;
>  static struct irq_domain *irq_domain;
>  static void __iomem *regs;
>  static u32 tegra_gpio_bank_count;
> @@ -240,7 +239,8 @@ static int tegra_gpio_irq_set_type(struct irq_data *d, unsigned int type)
>  
>  	ret = gpiochip_lock_as_irq(&tegra_gpio_chip, gpio);
>  	if (ret) {
> -		dev_err(dev, "unable to lock Tegra GPIO %d as IRQ\n", gpio);
> +		dev_err(tegra_gpio_chip.parent,
> +			"unable to lock Tegra GPIO %d as IRQ\n", gpio);

Can't we get rid of the tegra_gpio_chip global variable altogether? We
set a struct tegra_gpio_bank * as the chip data for each of the
interrupts, so if we added a back link to the GPIO chip to each bank
we could easily get at the GPIO chip (and its parent device) from the
IRQ chip implementation.

>  		return ret;
>  	}
>  
> @@ -465,8 +465,6 @@ static int tegra_gpio_probe(struct platform_device *pdev)
>  	int i;
>  	int j;
>  
> -	dev = &pdev->dev;
> -
>  	config = of_device_get_match_data(&pdev->dev);
>  	if (!config) {
>  		dev_err(&pdev->dev, "Error: No device match found\n");
> @@ -488,6 +486,8 @@ static int tegra_gpio_probe(struct platform_device *pdev)
>  	}
>  
>  	tegra_gpio_chip.ngpio = tegra_gpio_bank_count * 32;
> +	tegra_gpio_chip.parent = &pdev->dev;
> +

This adds a gratuitous blank line.

Thierry
Laxman Dewangan April 19, 2016, 12:43 p.m. UTC | #2
On Tuesday 19 April 2016 06:03 PM, Thierry Reding wrote:
> * PGP Signed by an unknown key
>
> On Tue, Apr 19, 2016 at 03:13:39PM +0530, Laxman Dewangan wrote:
>> Remove the file static device handle variable for keeping device handle
>> of driver as this is just required for error prints. The required device
>> handle are available from gpiochip structure.
>>
>> Signed-off-by: Laxman Dewangan <ldewangan@nvidia.com>
>> ---
>>   drivers/gpio/gpio-tegra.c | 8 ++++----
>>   1 file changed, 4 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/gpio/gpio-tegra.c b/drivers/gpio/gpio-tegra.c
>> index 1b0c497..de022a9 100644
>> --- a/drivers/gpio/gpio-tegra.c
>> +++ b/drivers/gpio/gpio-tegra.c
>> @@ -80,7 +80,6 @@ struct tegra_gpio_soc_config {
>>   	u32 upper_offset;
>>   };
>>   
>> -static struct device *dev;
>>   static struct irq_domain *irq_domain;
>>   static void __iomem *regs;
>>   static u32 tegra_gpio_bank_count;
>> @@ -240,7 +239,8 @@ static int tegra_gpio_irq_set_type(struct irq_data *d, unsigned int type)
>>   
>>   	ret = gpiochip_lock_as_irq(&tegra_gpio_chip, gpio);
>>   	if (ret) {
>> -		dev_err(dev, "unable to lock Tegra GPIO %d as IRQ\n", gpio);
>> +		dev_err(tegra_gpio_chip.parent,
>> +			"unable to lock Tegra GPIO %d as IRQ\n", gpio);
> Can't we get rid of the tegra_gpio_chip global variable altogether? We
> set a struct tegra_gpio_bank * as the chip data for each of the
> interrupts, so if we added a back link to the GPIO chip to each bank
> we could easily get at the GPIO chip (and its parent device) from the
> IRQ chip implementation.
>

We can get rid of all the global variables and can have only one (this 
is because our REGS macros uses the stride and changing then part of 
argument is too much change for single patch) as suggested by Stephen.

I am planning to have rid of all global variables to on  follow on 
patches once this series is fine.

--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Stephen Warren April 19, 2016, 4:04 p.m. UTC | #3
On 04/19/2016 06:43 AM, Laxman Dewangan wrote:
>
> On Tuesday 19 April 2016 06:03 PM, Thierry Reding wrote:
>> * PGP Signed by an unknown key
>>
>> On Tue, Apr 19, 2016 at 03:13:39PM +0530, Laxman Dewangan wrote:
>>> Remove the file static device handle variable for keeping device handle
>>> of driver as this is just required for error prints. The required device
>>> handle are available from gpiochip structure.
>>>
>>> Signed-off-by: Laxman Dewangan <ldewangan@nvidia.com>
>>> ---
>>>   drivers/gpio/gpio-tegra.c | 8 ++++----
>>>   1 file changed, 4 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/drivers/gpio/gpio-tegra.c b/drivers/gpio/gpio-tegra.c
>>> index 1b0c497..de022a9 100644
>>> --- a/drivers/gpio/gpio-tegra.c
>>> +++ b/drivers/gpio/gpio-tegra.c
>>> @@ -80,7 +80,6 @@ struct tegra_gpio_soc_config {
>>>       u32 upper_offset;
>>>   };
>>> -static struct device *dev;
>>>   static struct irq_domain *irq_domain;
>>>   static void __iomem *regs;
>>>   static u32 tegra_gpio_bank_count;
>>> @@ -240,7 +239,8 @@ static int tegra_gpio_irq_set_type(struct
>>> irq_data *d, unsigned int type)
>>>       ret = gpiochip_lock_as_irq(&tegra_gpio_chip, gpio);
>>>       if (ret) {
>>> -        dev_err(dev, "unable to lock Tegra GPIO %d as IRQ\n", gpio);
>>> +        dev_err(tegra_gpio_chip.parent,
>>> +            "unable to lock Tegra GPIO %d as IRQ\n", gpio);
>> Can't we get rid of the tegra_gpio_chip global variable altogether? We
>> set a struct tegra_gpio_bank * as the chip data for each of the
>> interrupts, so if we added a back link to the GPIO chip to each bank
>> we could easily get at the GPIO chip (and its parent device) from the
>> IRQ chip implementation.
>>
>
> We can get rid of all the global variables and can have only one (this
> is because our REGS macros uses the stride and changing then part of
> argument is too much change for single patch) as suggested by Stephen.

The REGS macro can take a parameter that points to the struct 
tegra_gpio_chip, which in turn can point at the struct 
tegra_gpio_soc_config. That doesn't sound like too many parameters/... 
to me. There's little point getting rid of all-but-one global.
--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/drivers/gpio/gpio-tegra.c b/drivers/gpio/gpio-tegra.c
index 1b0c497..de022a9 100644
--- a/drivers/gpio/gpio-tegra.c
+++ b/drivers/gpio/gpio-tegra.c
@@ -80,7 +80,6 @@  struct tegra_gpio_soc_config {
 	u32 upper_offset;
 };
 
-static struct device *dev;
 static struct irq_domain *irq_domain;
 static void __iomem *regs;
 static u32 tegra_gpio_bank_count;
@@ -240,7 +239,8 @@  static int tegra_gpio_irq_set_type(struct irq_data *d, unsigned int type)
 
 	ret = gpiochip_lock_as_irq(&tegra_gpio_chip, gpio);
 	if (ret) {
-		dev_err(dev, "unable to lock Tegra GPIO %d as IRQ\n", gpio);
+		dev_err(tegra_gpio_chip.parent,
+			"unable to lock Tegra GPIO %d as IRQ\n", gpio);
 		return ret;
 	}
 
@@ -465,8 +465,6 @@  static int tegra_gpio_probe(struct platform_device *pdev)
 	int i;
 	int j;
 
-	dev = &pdev->dev;
-
 	config = of_device_get_match_data(&pdev->dev);
 	if (!config) {
 		dev_err(&pdev->dev, "Error: No device match found\n");
@@ -488,6 +486,8 @@  static int tegra_gpio_probe(struct platform_device *pdev)
 	}
 
 	tegra_gpio_chip.ngpio = tegra_gpio_bank_count * 32;
+	tegra_gpio_chip.parent = &pdev->dev;
+
 
 	tegra_gpio_banks = devm_kzalloc(&pdev->dev,
 			tegra_gpio_bank_count * sizeof(*tegra_gpio_banks),