[PATCH 0/3] gpio: zynq: Update on gpio zynq driver

[PATCH 0/3] gpio: zynq: Update on gpio zynq driver

[Srinivas Neeli]

This patch series does the following:
-Simplified the code by using module_platform_driver().
-Fixing coverity warnings. 

Srinivas Neeli (3):
  gpio: zynq: use module_platform_driver to simplify the code
  gpio: zynq: Check return value of pm_runtime_get_sync
  gpio: zynq: Check return value of irq_get_irq_data

 drivers/gpio/gpio-zynq.c | 32 +++++++++++++++-----------------
 1 file changed, 15 insertions(+), 17 deletions(-)

-- 
2.9.1

[PATCH 1/3] gpio: zynq: use module_platform_driver to simplify the code

[Srinivas Neeli]

module_platform_driver() makes the code simpler by eliminating
boilerplate code.

Signed-off-by: Srinivas Neeli <srinivas.neeli@xilinx.com>
---
 drivers/gpio/gpio-zynq.c | 17 +----------------
 1 file changed, 1 insertion(+), 16 deletions(-)

diff --git a/drivers/gpio/gpio-zynq.c b/drivers/gpio/gpio-zynq.c
index 3521c1dc3ac0..bb1ac0c5cf26 100644
--- a/drivers/gpio/gpio-zynq.c
+++ b/drivers/gpio/gpio-zynq.c
@@ -1020,22 +1020,7 @@ static struct platform_driver zynq_gpio_driver = {
 	.remove = zynq_gpio_remove,
 };
 
-/**
- * zynq_gpio_init - Initial driver registration call
- *
- * Return: value from platform_driver_register
- */
-static int __init zynq_gpio_init(void)
-{
-	return platform_driver_register(&zynq_gpio_driver);
-}
-postcore_initcall(zynq_gpio_init);
-
-static void __exit zynq_gpio_exit(void)
-{
-	platform_driver_unregister(&zynq_gpio_driver);
-}
-module_exit(zynq_gpio_exit);
+module_platform_driver(zynq_gpio_driver);
 
 MODULE_AUTHOR("Xilinx Inc.");
 MODULE_DESCRIPTION("Zynq GPIO driver");
-- 
2.9.1

Re: [PATCH 1/3] gpio: zynq: use module_platform_driver to simplify the code

[Bartosz Golaszewski]

On Sat, Apr 10, 2021 at 12:08 AM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
>
>
>
> On Friday, April 9, 2021, Srinivas Neeli <srinivas.neeli@xilinx.com> wrote:
>>
>> module_platform_driver() makes the code simpler by eliminating
>> boilerplate code.
>>
>> Signed-off-by: Srinivas Neeli <srinivas.neeli@xilinx.com>
>> ---
>>  drivers/gpio/gpio-zynq.c | 17 +----------------
>>  1 file changed, 1 insertion(+), 16 deletions(-)
>>
>> diff --git a/drivers/gpio/gpio-zynq.c b/drivers/gpio/gpio-zynq.c
>> index 3521c1dc3ac0..bb1ac0c5cf26 100644
>> --- a/drivers/gpio/gpio-zynq.c
>> +++ b/drivers/gpio/gpio-zynq.c
>> @@ -1020,22 +1020,7 @@ static struct platform_driver zynq_gpio_driver = {
>>         .remove = zynq_gpio_remove,
>>  };
>>
>> -/**
>> - * zynq_gpio_init - Initial driver registration call
>> - *
>> - * Return: value from platform_driver_register
>> - */
>> -static int __init zynq_gpio_init(void)
>> -{
>> -       return platform_driver_register(&zynq_gpio_driver);
>> -}
>> -postcore_initcall(zynq_gpio_init);
>
>
>
> It’s not an equivalent. Have you tested on actual hardware? If no, there is no go for this change.
>

Yep, this has been like this since the initial introduction of this
driver. Unfortunately there's no documented reason so unless we can
test it, it has to stay this way.

Bartosz

RE: [PATCH 1/3] gpio: zynq: use module_platform_driver to simplify the code

[Srinivas Neeli]

HI baratosz and Andy,

> -----Original Message-----
> From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> Sent: Tuesday, April 13, 2021 4:14 PM
> To: Andy Shevchenko <andy.shevchenko@gmail.com>
> Cc: Srinivas Neeli <sneeli@xilinx.com>; linus.walleij@linaro.org; Michal Simek
> <michals@xilinx.com>; Shubhrajyoti Datta <shubhraj@xilinx.com>; Srinivas
> Goud <sgoud@xilinx.com>; linux-gpio@vger.kernel.org; linux-arm-
> kernel@lists.infradead.org; linux-kernel@vger.kernel.org; git
> <git@xilinx.com>
> Subject: Re: [PATCH 1/3] gpio: zynq: use module_platform_driver to simplify
> the code
> 
> On Sat, Apr 10, 2021 at 12:08 AM Andy Shevchenko
> <andy.shevchenko@gmail.com> wrote:
> >
> >
> >
> > On Friday, April 9, 2021, Srinivas Neeli <srinivas.neeli@xilinx.com> wrote:
> >>
> >> module_platform_driver() makes the code simpler by eliminating
> >> boilerplate code.
> >>
> >> Signed-off-by: Srinivas Neeli <srinivas.neeli@xilinx.com>
> >> ---
> >>  drivers/gpio/gpio-zynq.c | 17 +----------------
> >>  1 file changed, 1 insertion(+), 16 deletions(-)
> >>
> >> diff --git a/drivers/gpio/gpio-zynq.c b/drivers/gpio/gpio-zynq.c
> >> index 3521c1dc3ac0..bb1ac0c5cf26 100644
> >> --- a/drivers/gpio/gpio-zynq.c
> >> +++ b/drivers/gpio/gpio-zynq.c
> >> @@ -1020,22 +1020,7 @@ static struct platform_driver zynq_gpio_driver
> = {
> >>         .remove = zynq_gpio_remove,
> >>  };
> >>
> >> -/**
> >> - * zynq_gpio_init - Initial driver registration call
> >> - *
> >> - * Return: value from platform_driver_register
> >> - */
> >> -static int __init zynq_gpio_init(void) -{
> >> -       return platform_driver_register(&zynq_gpio_driver);
> >> -}
> >> -postcore_initcall(zynq_gpio_init);
> >
> >
> >
> > It’s not an equivalent. Have you tested on actual hardware? If no, there is
> no go for this change.
> >
> 
> Yep, this has been like this since the initial introduction of this driver.
> Unfortunately there's no documented reason so unless we can test it, it has
> to stay this way.
> 
I tested driver, functionality wise everything working fine.
Based on below conversation, I moved driver to module driver.
https://lore.kernel.org/patchwork/patch/818202/

Thanks
Srinivas Neeli

> Bartosz

Re: [PATCH 1/3] gpio: zynq: use module_platform_driver to simplify the code

[Bartosz Golaszewski]

On Wed, Apr 14, 2021 at 4:45 PM Srinivas Neeli <sneeli@xilinx.com> wrote:
>
> HI baratosz and Andy,
>

It's Bartosz. You literally just need to copy & paste the name from my email...

> > -----Original Message-----
> > From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> > Sent: Tuesday, April 13, 2021 4:14 PM
> > To: Andy Shevchenko <andy.shevchenko@gmail.com>
> > Cc: Srinivas Neeli <sneeli@xilinx.com>; linus.walleij@linaro.org; Michal Simek
> > <michals@xilinx.com>; Shubhrajyoti Datta <shubhraj@xilinx.com>; Srinivas
> > Goud <sgoud@xilinx.com>; linux-gpio@vger.kernel.org; linux-arm-
> > kernel@lists.infradead.org; linux-kernel@vger.kernel.org; git
> > <git@xilinx.com>
> > Subject: Re: [PATCH 1/3] gpio: zynq: use module_platform_driver to simplify
> > the code
> >
> > On Sat, Apr 10, 2021 at 12:08 AM Andy Shevchenko
> > <andy.shevchenko@gmail.com> wrote:
> > >
> > >
> > >
> > > On Friday, April 9, 2021, Srinivas Neeli <srinivas.neeli@xilinx.com> wrote:
> > >>
> > >> module_platform_driver() makes the code simpler by eliminating
> > >> boilerplate code.
> > >>
> > >> Signed-off-by: Srinivas Neeli <srinivas.neeli@xilinx.com>
> > >> ---
> > >>  drivers/gpio/gpio-zynq.c | 17 +----------------
> > >>  1 file changed, 1 insertion(+), 16 deletions(-)
> > >>
> > >> diff --git a/drivers/gpio/gpio-zynq.c b/drivers/gpio/gpio-zynq.c
> > >> index 3521c1dc3ac0..bb1ac0c5cf26 100644
> > >> --- a/drivers/gpio/gpio-zynq.c
> > >> +++ b/drivers/gpio/gpio-zynq.c
> > >> @@ -1020,22 +1020,7 @@ static struct platform_driver zynq_gpio_driver
> > = {
> > >>         .remove = zynq_gpio_remove,
> > >>  };
> > >>
> > >> -/**
> > >> - * zynq_gpio_init - Initial driver registration call
> > >> - *
> > >> - * Return: value from platform_driver_register
> > >> - */
> > >> -static int __init zynq_gpio_init(void) -{
> > >> -       return platform_driver_register(&zynq_gpio_driver);
> > >> -}
> > >> -postcore_initcall(zynq_gpio_init);
> > >
> > >
> > >
> > > It’s not an equivalent. Have you tested on actual hardware? If no, there is
> > no go for this change.
> > >
> >
> > Yep, this has been like this since the initial introduction of this driver.
> > Unfortunately there's no documented reason so unless we can test it, it has
> > to stay this way.
> >
> I tested driver, functionality wise everything working fine.
> Based on below conversation, I moved driver to module driver.
> https://lore.kernel.org/patchwork/patch/818202/
>

Andy: How about we give it a try then? If anyone yells, we'll just revert it.

> Thanks
> Srinivas Neeli
>
> > Bartosz

Bartosz

Re: [PATCH 1/3] gpio: zynq: use module_platform_driver to simplify the code

[Andy Shevchenko]

On Fri, Apr 16, 2021 at 9:28 PM Bartosz Golaszewski
<bgolaszewski@baylibre.com> wrote:
> On Wed, Apr 14, 2021 at 4:45 PM Srinivas Neeli <sneeli@xilinx.com> wrote:
> > > From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> > > Sent: Tuesday, April 13, 2021 4:14 PM
> > > On Sat, Apr 10, 2021 at 12:08 AM Andy Shevchenko
> > > <andy.shevchenko@gmail.com> wrote:

...

> > > Yep, this has been like this since the initial introduction of this driver.
> > > Unfortunately there's no documented reason so unless we can test it, it has
> > > to stay this way.
> > >
> > I tested driver, functionality wise everything working fine.
> > Based on below conversation, I moved driver to module driver.
> > https://lore.kernel.org/patchwork/patch/818202/
> >
>
> Andy: How about we give it a try then? If anyone yells, we'll just revert it.

Sounds like a plan!

-- 
With Best Regards,
Andy Shevchenko

RE: [PATCH 1/3] gpio: zynq: use module_platform_driver to simplify the code

[Srinivas Neeli]

Hi,

> -----Original Message-----
> From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> Sent: Friday, April 16, 2021 11:58 PM
> To: Srinivas Neeli <sneeli@xilinx.com>
> Cc: Andy Shevchenko <andy.shevchenko@gmail.com>;
> linus.walleij@linaro.org; Michal Simek <michals@xilinx.com>; Shubhrajyoti
> Datta <shubhraj@xilinx.com>; Srinivas Goud <sgoud@xilinx.com>; linux-
> gpio@vger.kernel.org; linux-arm-kernel@lists.infradead.org; linux-
> kernel@vger.kernel.org; git <git@xilinx.com>
> Subject: Re: [PATCH 1/3] gpio: zynq: use module_platform_driver to simplify
> the code
> 
> On Wed, Apr 14, 2021 at 4:45 PM Srinivas Neeli <sneeli@xilinx.com> wrote:
> >
> > HI baratosz and Andy,
> >
> 
> It's Bartosz. You literally just need to copy & paste the name from my email...
> 
> > > -----Original Message-----
> > > From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> > > Sent: Tuesday, April 13, 2021 4:14 PM
> > > To: Andy Shevchenko <andy.shevchenko@gmail.com>
> > > Cc: Srinivas Neeli <sneeli@xilinx.com>; linus.walleij@linaro.org;
> > > Michal Simek <michals@xilinx.com>; Shubhrajyoti Datta
> > > <shubhraj@xilinx.com>; Srinivas Goud <sgoud@xilinx.com>;
> > > linux-gpio@vger.kernel.org; linux-arm- kernel@lists.infradead.org;
> > > linux-kernel@vger.kernel.org; git <git@xilinx.com>
> > > Subject: Re: [PATCH 1/3] gpio: zynq: use module_platform_driver to
> > > simplify the code
> > >
> > > On Sat, Apr 10, 2021 at 12:08 AM Andy Shevchenko
> > > <andy.shevchenko@gmail.com> wrote:
> > > >
> > > >
> > > >
> > > > On Friday, April 9, 2021, Srinivas Neeli <srinivas.neeli@xilinx.com>
> wrote:
> > > >>
> > > >> module_platform_driver() makes the code simpler by eliminating
> > > >> boilerplate code.
> > > >>
> > > >> Signed-off-by: Srinivas Neeli <srinivas.neeli@xilinx.com>
> > > >> ---
> > > >>  drivers/gpio/gpio-zynq.c | 17 +----------------
> > > >>  1 file changed, 1 insertion(+), 16 deletions(-)
> > > >>
> > > >> diff --git a/drivers/gpio/gpio-zynq.c b/drivers/gpio/gpio-zynq.c
> > > >> index 3521c1dc3ac0..bb1ac0c5cf26 100644
> > > >> --- a/drivers/gpio/gpio-zynq.c
> > > >> +++ b/drivers/gpio/gpio-zynq.c
> > > >> @@ -1020,22 +1020,7 @@ static struct platform_driver
> > > >> zynq_gpio_driver
> > > = {
> > > >>         .remove = zynq_gpio_remove,  };
> > > >>
> > > >> -/**
> > > >> - * zynq_gpio_init - Initial driver registration call
> > > >> - *
> > > >> - * Return: value from platform_driver_register
> > > >> - */
> > > >> -static int __init zynq_gpio_init(void) -{
> > > >> -       return platform_driver_register(&zynq_gpio_driver);
> > > >> -}
> > > >> -postcore_initcall(zynq_gpio_init);
> > > >
> > > >
> > > >
> > > > It’s not an equivalent. Have you tested on actual hardware? If no,
> > > > there is
> > > no go for this change.
> > > >
> > >
> > > Yep, this has been like this since the initial introduction of this driver.
> > > Unfortunately there's no documented reason so unless we can test it,
> > > it has to stay this way.
> > >
> > I tested driver, functionality wise everything working fine.
> > Based on below conversation, I moved driver to module driver.
> > https://lore.kernel.org/patchwork/patch/818202/
> >
> 
> Andy: How about we give it a try then? If anyone yells, we'll just revert it.

Could you please apply this series to gpio for-next branch if there are no issues .

> 
> > Thanks
> > Srinivas Neeli
> >
> > > Bartosz
> 
> Bartosz

Thanks
Srinivas Neeli

Re: [PATCH 1/3] gpio: zynq: use module_platform_driver to simplify the code

[Bartosz Golaszewski]

On Mon, Jun 14, 2021 at 12:39 PM Srinivas Neeli <sneeli@xilinx.com> wrote:
>
> >
> > Andy: How about we give it a try then? If anyone yells, we'll just revert it.
>
> Could you please apply this series to gpio for-next branch if there are no issues .
>

Applied, thanks!

Bart

[PATCH 2/3] gpio: zynq: Check return value of pm_runtime_get_sync

[Srinivas Neeli]

Return value of "pm_runtime_get_sync" API was neither captured nor checked.
Fixed it by capturing the return value and then checking for any warning.

Addresses-Coverity: "check_return"
Signed-off-by: Srinivas Neeli <srinivas.neeli@xilinx.com>
---
 drivers/gpio/gpio-zynq.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/gpio/gpio-zynq.c b/drivers/gpio/gpio-zynq.c
index bb1ac0c5cf26..c91302a16c77 100644
--- a/drivers/gpio/gpio-zynq.c
+++ b/drivers/gpio/gpio-zynq.c
@@ -1001,8 +1001,11 @@ static int zynq_gpio_probe(struct platform_device *pdev)
 static int zynq_gpio_remove(struct platform_device *pdev)
 {
 	struct zynq_gpio *gpio = platform_get_drvdata(pdev);
+	int ret;
 
-	pm_runtime_get_sync(&pdev->dev);
+	ret = pm_runtime_get_sync(&pdev->dev);
+	if (ret < 0)
+		dev_warn(&pdev->dev, "pm_runtime_get_sync() Failed\n");
 	gpiochip_remove(&gpio->chip);
 	clk_disable_unprepare(gpio->clk);
 	device_set_wakeup_capable(&pdev->dev, 0);
-- 
2.9.1

[PATCH 3/3] gpio: zynq: Check return value of irq_get_irq_data

[Srinivas Neeli]

In two different instances the return value of "irq_get_irq_data"
API was neither captured nor checked.
Fixed it by capturing the return value and then checking for any error.

Addresses-Coverity: "returned_null"
Signed-off-by: Srinivas Neeli <srinivas.neeli@xilinx.com>
---
 drivers/gpio/gpio-zynq.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/drivers/gpio/gpio-zynq.c b/drivers/gpio/gpio-zynq.c
index c91302a16c77..f0cb8ccd03ed 100644
--- a/drivers/gpio/gpio-zynq.c
+++ b/drivers/gpio/gpio-zynq.c
@@ -736,6 +736,11 @@ static int __maybe_unused zynq_gpio_suspend(struct device *dev)
 	struct zynq_gpio *gpio = dev_get_drvdata(dev);
 	struct irq_data *data = irq_get_irq_data(gpio->irq);
 
+	if (!data) {
+		dev_err(dev, "irq_get_irq_data() failed\n");
+		return -EINVAL;
+	}
+
 	if (!device_may_wakeup(dev))
 		disable_irq(gpio->irq);
 
@@ -753,6 +758,11 @@ static int __maybe_unused zynq_gpio_resume(struct device *dev)
 	struct irq_data *data = irq_get_irq_data(gpio->irq);
 	int ret;
 
+	if (!data) {
+		dev_err(dev, "irq_get_irq_data() failed\n");
+		return -EINVAL;
+	}
+
 	if (!device_may_wakeup(dev))
 		enable_irq(gpio->irq);
 
-- 
2.9.1

Re: [PATCH 3/3] gpio: zynq: Check return value of irq_get_irq_data

[Deepak R Varma]

On Fri, Apr 09, 2021 at 07:38:06PM +0530, Srinivas Neeli wrote:
> In two different instances the return value of "irq_get_irq_data"
> API was neither captured nor checked.
> Fixed it by capturing the return value and then checking for any error.
> 
> Addresses-Coverity: "returned_null"
> Signed-off-by: Srinivas Neeli <srinivas.neeli@xilinx.com>
> ---
>  drivers/gpio/gpio-zynq.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/drivers/gpio/gpio-zynq.c b/drivers/gpio/gpio-zynq.c
> index c91302a16c77..f0cb8ccd03ed 100644
> --- a/drivers/gpio/gpio-zynq.c
> +++ b/drivers/gpio/gpio-zynq.c
> @@ -736,6 +736,11 @@ static int __maybe_unused zynq_gpio_suspend(struct device *dev)
>  	struct zynq_gpio *gpio = dev_get_drvdata(dev);
>  	struct irq_data *data = irq_get_irq_data(gpio->irq);
>  
> +	if (!data) {
> +		dev_err(dev, "irq_get_irq_data() failed\n");

It will be useful to include a tag such as "suspend: " in the error
message to uniquely identify where it failed from.

> +		return -EINVAL;
> +	}
> +
>  	if (!device_may_wakeup(dev))
>  		disable_irq(gpio->irq);
>  
> @@ -753,6 +758,11 @@ static int __maybe_unused zynq_gpio_resume(struct device *dev)
>  	struct irq_data *data = irq_get_irq_data(gpio->irq);
>  	int ret;
>  
> +	if (!data) {
> +		dev_err(dev, "irq_get_irq_data() failed\n");

Ditto. Suggest using "resume: " tag here.

> +		return -EINVAL;
> +	}
> +
>  	if (!device_may_wakeup(dev))
>  		enable_irq(gpio->irq);
>  
> -- 
> 2.9.1
>