[PATCH v1 1/4] gpio: dwapb: avoid error message for optional IRQ

[PATCH v1 1/4] gpio: dwapb: avoid error message for optional IRQ

[Andy Shevchenko]

platform_get_irq() will generate an error message if the requested IRQ
is not present. Use platform_get_irq_optional() to avoid the error message
being generated.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Cc: Serge Semin <fancer.lancer@gmail.com>
---
 drivers/gpio/gpio-dwapb.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpio/gpio-dwapb.c b/drivers/gpio/gpio-dwapb.c
index 8639c4a7f46974..5bc5057f071f37 100644
--- a/drivers/gpio/gpio-dwapb.c
+++ b/drivers/gpio/gpio-dwapb.c
@@ -542,7 +542,7 @@ static void dwapb_get_irq(struct device *dev, struct fwnode_handle *fwnode,
 		if (np)
 			pp->irq[j] = of_irq_get(np, j);
 		else if (has_acpi_companion(dev))
-			pp->irq[j] = platform_get_irq(to_platform_device(dev), j);
+			pp->irq[j] = platform_get_irq_optional(to_platform_device(dev), j);
 
 		if (pp->irq[j] >= 0)
 			pp->has_irq = true;
-- 
2.26.2

[PATCH v1 2/4] gpio: dwapb: Don't use 0 as valid Linux interrupt number

[Andy Shevchenko]

0 is not valid IRQ number in Linux interrupt number space.
Refactor the code with this kept in mind.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Cc: Serge Semin <fancer.lancer@gmail.com>
---
 drivers/gpio/gpio-dwapb.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/drivers/gpio/gpio-dwapb.c b/drivers/gpio/gpio-dwapb.c
index 5bc5057f071f37..78662d6d73634e 100644
--- a/drivers/gpio/gpio-dwapb.c
+++ b/drivers/gpio/gpio-dwapb.c
@@ -417,7 +417,7 @@ static void dwapb_configure_irqs(struct dwapb_gpio *gpio,
 		int i;
 
 		for (i = 0; i < pp->ngpio; i++) {
-			if (pp->irq[i] >= 0)
+			if (pp->irq[i])
 				irq_set_chained_handler_and_data(pp->irq[i],
 						dwapb_irq_handler, gpio);
 		}
@@ -531,23 +531,23 @@ static void dwapb_get_irq(struct device *dev, struct fwnode_handle *fwnode,
 			  struct dwapb_port_property *pp)
 {
 	struct device_node *np = NULL;
-	int j;
+	int err, j;
 
 	if (fwnode_property_read_bool(fwnode, "interrupt-controller"))
 		np = to_of_node(fwnode);
 
 	for (j = 0; j < pp->ngpio; j++) {
-		pp->irq[j] = -ENXIO;
-
 		if (np)
-			pp->irq[j] = of_irq_get(np, j);
+			err = of_irq_get(np, j);
 		else if (has_acpi_companion(dev))
-			pp->irq[j] = platform_get_irq_optional(to_platform_device(dev), j);
+			err = platform_get_irq_optional(to_platform_device(dev), j);
+		if (err < 0)
+			continue;
 
-		if (pp->irq[j] >= 0)
-			pp->has_irq = true;
+		pp->irq[j] = err;
 	}
 
+	pp->has_irq = memchr_inv(pp->irq, 0, sizeof(pp->irq)) != NULL;
 	if (!pp->has_irq)
 		dev_warn(dev, "no irq for port%d\n", pp->idx);
 }
-- 
2.26.2

Re: [PATCH v1 2/4] gpio: dwapb: Don't use 0 as valid Linux interrupt number

[Serge Semin]

On Tue, May 12, 2020 at 09:45:11PM +0300, Andy Shevchenko wrote:
> 0 is not valid IRQ number in Linux interrupt number space.
> Refactor the code with this kept in mind.
> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Cc: Serge Semin <fancer.lancer@gmail.com>
> ---
>  drivers/gpio/gpio-dwapb.c | 16 ++++++++--------
>  1 file changed, 8 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/gpio/gpio-dwapb.c b/drivers/gpio/gpio-dwapb.c
> index 5bc5057f071f37..78662d6d73634e 100644
> --- a/drivers/gpio/gpio-dwapb.c
> +++ b/drivers/gpio/gpio-dwapb.c
> @@ -417,7 +417,7 @@ static void dwapb_configure_irqs(struct dwapb_gpio *gpio,
>  		int i;
>  
>  		for (i = 0; i < pp->ngpio; i++) {
> -			if (pp->irq[i] >= 0)
> +			if (pp->irq[i])
>  				irq_set_chained_handler_and_data(pp->irq[i],
>  						dwapb_irq_handler, gpio);
>  		}
> @@ -531,23 +531,23 @@ static void dwapb_get_irq(struct device *dev, struct fwnode_handle *fwnode,
>  			  struct dwapb_port_property *pp)
>  {
>  	struct device_node *np = NULL;
> -	int j;
> +	int err, j;

Err might be used uninitialized here. The compiler also says so:

drivers/gpio/gpio-dwapb.c:560:14: warning: ‘err’ may be used uninitialized in this function [-Wmaybe-uninitialized]
   pp->irq[j] = err;

drivers/gpio/gpio-dwapb.c:547:6: note: ‘err’ was declared here
  int err, j;

Could you please make it initialized with error number like -ENXIO by default?

Also if it was just a single issue in my mind, I wouldn't have probably payed
much attention to this. But since you need to send v2 anyway, I'd prefer to have
a positive naming here since normally both of_irq_get() and
platform_get_irq_optional() return IRQ number, and error is returned only on
failure. So checking an erroneous value of a positively named variable seems
more natural, rather than copying an error-named variable to a normal variable.
To cut it short could you please rename err to something like irq?

>  
>  	if (fwnode_property_read_bool(fwnode, "interrupt-controller"))
>  		np = to_of_node(fwnode);
>  
>  	for (j = 0; j < pp->ngpio; j++) {
> -		pp->irq[j] = -ENXIO;
> -
>  		if (np)
> -			pp->irq[j] = of_irq_get(np, j);
> +			err = of_irq_get(np, j);
>  		else if (has_acpi_companion(dev))
> -			pp->irq[j] = platform_get_irq_optional(to_platform_device(dev), j);
> +			err = platform_get_irq_optional(to_platform_device(dev), j);
> +		if (err < 0)
> +			continue;
>  
> -		if (pp->irq[j] >= 0)
> -			pp->has_irq = true;
> +		pp->irq[j] = err;
>  	}
>  

> +	pp->has_irq = memchr_inv(pp->irq, 0, sizeof(pp->irq)) != NULL;

Sorry, but I don't see why is setting the has_irq flag in the loop above worse than
using memchr_inv()? As I see it since we need to perform the loop above anyway, it
would be normal to update the flag synchronously there instead of traversing the
irq's array byte-by-byte again in the memchr_inv() method. Moreover
(memchr_inv() != NULL) seems harder to read. A kernel hacker needs to keep in
mind the method semantics, that it returns non-null if unmatched character found
in the array, to get the line logic. Setting "has_irq = true" is straightforward -
if IRQ's found then set the flag to true. So if you don't have a strong opinion
against my reasoning could you please get the setting the has_irq flag back in to
the loop above?

-Sergey

>  	if (!pp->has_irq)
>  		dev_warn(dev, "no irq for port%d\n", pp->idx);
>  }
> -- 
> 2.26.2
> 

Re: [PATCH v1 2/4] gpio: dwapb: Don't use 0 as valid Linux interrupt number

[Andy Shevchenko]

On Sun, May 17, 2020 at 03:52:44PM +0300, Serge Semin wrote:
> On Tue, May 12, 2020 at 09:45:11PM +0300, Andy Shevchenko wrote:
> > 0 is not valid IRQ number in Linux interrupt number space.
> > Refactor the code with this kept in mind.

Serge, thanks for review, my answers below.

...

> > +	int err, j;
> 
> Err might be used uninitialized here. The compiler also says so:
> 
> drivers/gpio/gpio-dwapb.c:560:14: warning: ‘err’ may be used uninitialized in this function [-Wmaybe-uninitialized]
>    pp->irq[j] = err;
> 
> drivers/gpio/gpio-dwapb.c:547:6: note: ‘err’ was declared here
>   int err, j;

Good catch! I'm puzzled how I missed it :-(
I usually compile with W=1.

Hmm... gcc (Debian 9.3.0-12) 9.3.0 building for i386, doesn't give me a
warning, but I see that there is potentially possible that scenario, so, I'm
going to fix it.

> Could you please make it initialized with error number like -ENXIO by default?

Will do.

> Also if it was just a single issue in my mind, I wouldn't have probably payed
> much attention to this. But since you need to send v2 anyway, I'd prefer to have
> a positive naming here since normally both of_irq_get() and
> platform_get_irq_optional() return IRQ number, and error is returned only on
> failure. So checking an erroneous value of a positively named variable seems
> more natural, rather than copying an error-named variable to a normal variable.
> To cut it short could you please rename err to something like irq?

Makes sense.

...

> > +	pp->has_irq = memchr_inv(pp->irq, 0, sizeof(pp->irq)) != NULL;
> 
> Sorry, but I don't see why is setting the has_irq flag in the loop above worse than
> using memchr_inv()? As I see it since we need to perform the loop above anyway, it
> would be normal to update the flag synchronously there instead of traversing the
> irq's array byte-by-byte again in the memchr_inv() method. Moreover
> (memchr_inv() != NULL) seems harder to read. A kernel hacker needs to keep in
> mind the method semantics, that it returns non-null if unmatched character found
> in the array, to get the line logic. Setting "has_irq = true" is straightforward -
> if IRQ's found then set the flag to true. So if you don't have a strong opinion
> against my reasoning could you please get the setting the has_irq flag back in to
> the loop above?

It's done in that way for a reason of the next clean ups, i.e. moving this to
the actual user of it. I hope you already read further patches to see the
intention behind this change. So, I prefer to leave it as is, however I agree
with you in general.

Btw, we may also leave the domain when even no IRQ is available as some other
drivers do, but I consider it less plausible than using memchr_inv().

-- 
With Best Regards,
Andy Shevchenko

[PATCH v1 3/4] gpio: dwapb: Drop extra check to call acpi_gpiochip_request_interrupts()

[Andy Shevchenko]

There is no need to have an additional check to call
acpi_gpiochip_request_interrupts(). Even without any interrupts available
the registered ACPI Event handlers can be useful for debugging purposes.

While at it, add missed acpi_gpiochip_free_interrupts() call when
unregistering ports.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Cc: Serge Semin <fancer.lancer@gmail.com>
---
 drivers/gpio/gpio-dwapb.c | 25 ++++++++++++++++---------
 1 file changed, 16 insertions(+), 9 deletions(-)

diff --git a/drivers/gpio/gpio-dwapb.c b/drivers/gpio/gpio-dwapb.c
index 78662d6d73634e..2975f2d369874a 100644
--- a/drivers/gpio/gpio-dwapb.c
+++ b/drivers/gpio/gpio-dwapb.c
@@ -505,26 +505,33 @@ static int dwapb_gpio_add_port(struct dwapb_gpio *gpio,
 		dwapb_configure_irqs(gpio, port, pp);
 
 	err = gpiochip_add_data(&port->gc, port);
-	if (err)
+	if (err) {
 		dev_err(gpio->dev, "failed to register gpiochip for port%d\n",
 			port->idx);
-	else
-		port->is_registered = true;
+		return err;
+	}
 
 	/* Add GPIO-signaled ACPI event support */
-	if (pp->has_irq)
-		acpi_gpiochip_request_interrupts(&port->gc);
+	acpi_gpiochip_request_interrupts(&port->gc);
 
-	return err;
+	port->is_registered = true;
+
+	return 0;
 }
 
 static void dwapb_gpio_unregister(struct dwapb_gpio *gpio)
 {
 	unsigned int m;
 
-	for (m = 0; m < gpio->nr_ports; ++m)
-		if (gpio->ports[m].is_registered)
-			gpiochip_remove(&gpio->ports[m].gc);
+	for (m = 0; m < gpio->nr_ports; ++m) {
+		struct dwapb_gpio_port *port = &gpio->ports[m];
+
+		if (!port->is_registered)
+			continue;
+
+		acpi_gpiochip_free_interrupts(&port->gc);
+		gpiochip_remove(&port->gc);
+	}
 }
 
 static void dwapb_get_irq(struct device *dev, struct fwnode_handle *fwnode,
-- 
2.26.2

Re: [PATCH v1 3/4] gpio: dwapb: Drop extra check to call acpi_gpiochip_request_interrupts()

[Serge Semin]

On Tue, May 12, 2020 at 09:45:12PM +0300, Andy Shevchenko wrote:
> There is no need to have an additional check to call
> acpi_gpiochip_request_interrupts(). Even without any interrupts available
> the registered ACPI Event handlers can be useful for debugging purposes.
> 
> While at it, add missed acpi_gpiochip_free_interrupts() call when
> unregistering ports.
> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Cc: Serge Semin <fancer.lancer@gmail.com>
> ---
>  drivers/gpio/gpio-dwapb.c | 25 ++++++++++++++++---------
>  1 file changed, 16 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/gpio/gpio-dwapb.c b/drivers/gpio/gpio-dwapb.c
> index 78662d6d73634e..2975f2d369874a 100644
> --- a/drivers/gpio/gpio-dwapb.c
> +++ b/drivers/gpio/gpio-dwapb.c
> @@ -505,26 +505,33 @@ static int dwapb_gpio_add_port(struct dwapb_gpio *gpio,
>  		dwapb_configure_irqs(gpio, port, pp);
>  
>  	err = gpiochip_add_data(&port->gc, port);
> -	if (err)
> +	if (err) {
>  		dev_err(gpio->dev, "failed to register gpiochip for port%d\n",
>  			port->idx);
> -	else
> -		port->is_registered = true;
> +		return err;
> +	}
>  
>  	/* Add GPIO-signaled ACPI event support */

> -	if (pp->has_irq)
> -		acpi_gpiochip_request_interrupts(&port->gc);
> +	acpi_gpiochip_request_interrupts(&port->gc);

Hm, perhaps replacing it with:
+	if (pp->idx == 0)
+		acpi_gpiochip_request_interrupts(&port->gc);
could be more appropriate seeing Port A only supports IRQs, which we'd point
out by the (idx == 0) conditional statement. So we don't have to call
the method at most four times for each available port. Though judging by the
acpi_gpiochip_request_interrupts() function internals it will just ignore
GPIO chips with no IRQ support. Andy, It's up to you to decide. I'm not against
the change the way it is, but if you agree that signifying the IRQs affiliation
would be better, then please fill free to add the conditional statement I
suggested.

>  
> -	return err;
> +	port->is_registered = true;
> +
> +	return 0;
>  }
>  

>  static void dwapb_gpio_unregister(struct dwapb_gpio *gpio)
>  {
>  	unsigned int m;
>  
> -	for (m = 0; m < gpio->nr_ports; ++m)
> -		if (gpio->ports[m].is_registered)
> -			gpiochip_remove(&gpio->ports[m].gc);
> +	for (m = 0; m < gpio->nr_ports; ++m) {
> +		struct dwapb_gpio_port *port = &gpio->ports[m];
> +
> +		if (!port->is_registered)
> +			continue;
> +
> +		acpi_gpiochip_free_interrupts(&port->gc);
> +		gpiochip_remove(&port->gc);
> +	}
>  }

Could you please move this change to a dedicated patch? It seems to me this
alteration might be appropriate to be ported to the stable kernels seeing it
fixes e6cb3486f5a1 ("gpio: dwapb: add gpio-signaled acpi event support").
Linus, what do you think?

-Sergey

>  
>  static void dwapb_get_irq(struct device *dev, struct fwnode_handle *fwnode,
> -- 
> 2.26.2
> 

Re: [PATCH v1 3/4] gpio: dwapb: Drop extra check to call acpi_gpiochip_request_interrupts()

[Serge Semin]

On Sun, May 17, 2020 at 04:55:24PM +0300, Serge Semin wrote:
> On Tue, May 12, 2020 at 09:45:12PM +0300, Andy Shevchenko wrote:
> > There is no need to have an additional check to call
> > acpi_gpiochip_request_interrupts(). Even without any interrupts available
> > the registered ACPI Event handlers can be useful for debugging purposes.
> > 
> > While at it, add missed acpi_gpiochip_free_interrupts() call when
> > unregistering ports.
> > 
> > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > Cc: Serge Semin <fancer.lancer@gmail.com>
> > ---

[nip]

> > -	if (pp->has_irq)
> > -		acpi_gpiochip_request_interrupts(&port->gc);
> > +	acpi_gpiochip_request_interrupts(&port->gc);
> 
> Hm, perhaps replacing it with:
> +	if (pp->idx == 0)
> +		acpi_gpiochip_request_interrupts(&port->gc);
> could be more appropriate seeing Port A only supports IRQs, which we'd point
> out by the (idx == 0) conditional statement. So we don't have to call
> the method at most four times for each available port. Though judging by the
> acpi_gpiochip_request_interrupts() function internals it will just ignore
> GPIO chips with no IRQ support. Andy, It's up to you to decide. I'm not against
> the change the way it is, but if you agree that signifying the IRQs affiliation
> would be better, then please fill free to add the conditional statement I
> suggested.
> 

Please see my comment below, before you decide what to do with this part of the
patch.

[nip]

> 
> >  static void dwapb_gpio_unregister(struct dwapb_gpio *gpio)
> >  {
> >  	unsigned int m;
> >  
> > -	for (m = 0; m < gpio->nr_ports; ++m)
> > -		if (gpio->ports[m].is_registered)
> > -			gpiochip_remove(&gpio->ports[m].gc);
> > +	for (m = 0; m < gpio->nr_ports; ++m) {
> > +		struct dwapb_gpio_port *port = &gpio->ports[m];
> > +
> > +		if (!port->is_registered)
> > +			continue;
> > +
> > +		acpi_gpiochip_free_interrupts(&port->gc);
> > +		gpiochip_remove(&port->gc);
> > +	}
> >  }
> 
> Could you please move this change to a dedicated patch? It seems to me this
> alteration might be appropriate to be ported to the stable kernels seeing it
> fixes e6cb3486f5a1 ("gpio: dwapb: add gpio-signaled acpi event support").
> Linus, what do you think?
> 
> -Sergey
> 

BTW after moving the change with acpi_gpiochip_free_interrupts() into a
dedicated patch, you can freely merge the rest of this patch into the
last one of this series. So the has_irq flag cleanup would be performed in a
single commit. Especially if you implement the comment I provided above regarding
conditional (idx == 0) calling of the acpi_gpiochip_request_interrupts() method.

So your series will look like this:
gpio: dwapb: avoid error message for optional IRQ
gpio: dwapb: Don't use 0 as valid Linux interrupt number
gpio: dwapb: Call acpi_gpiochip_free_interrupts() on GPIO chip de-registration (<= This commit can be moved to the head of the series as being marked by the
Fixes tag)
gpio: dwapb: Remove redundant has_irq flag support

-Sergey

Re: [PATCH v1 3/4] gpio: dwapb: Drop extra check to call acpi_gpiochip_request_interrupts()

[Andy Shevchenko]

On Sun, May 17, 2020 at 05:47:37PM +0300, Serge Semin wrote:
> On Sun, May 17, 2020 at 04:55:24PM +0300, Serge Semin wrote:
> > On Tue, May 12, 2020 at 09:45:12PM +0300, Andy Shevchenko wrote:

...

> > >  static void dwapb_gpio_unregister(struct dwapb_gpio *gpio)
> > >  {
> > >  	unsigned int m;
> > >  
> > > -	for (m = 0; m < gpio->nr_ports; ++m)
> > > -		if (gpio->ports[m].is_registered)
> > > -			gpiochip_remove(&gpio->ports[m].gc);
> > > +	for (m = 0; m < gpio->nr_ports; ++m) {
> > > +		struct dwapb_gpio_port *port = &gpio->ports[m];
> > > +
> > > +		if (!port->is_registered)
> > > +			continue;
> > > +
> > > +		acpi_gpiochip_free_interrupts(&port->gc);
> > > +		gpiochip_remove(&port->gc);
> > > +	}
> > >  }
> > 
> > Could you please move this change to a dedicated patch? It seems to me this
> > alteration might be appropriate to be ported to the stable kernels seeing it
> > fixes e6cb3486f5a1 ("gpio: dwapb: add gpio-signaled acpi event support").
> > Linus, what do you think?
> > 
> > -Sergey
> > 
> 
> BTW after moving the change with acpi_gpiochip_free_interrupts() into a
> dedicated patch, you can freely merge the rest of this patch into the
> last one of this series. So the has_irq flag cleanup would be performed in a
> single commit. Especially if you implement the comment I provided above regarding
> conditional (idx == 0) calling of the acpi_gpiochip_request_interrupts() method.

I was thinking about this split and came to the conclusion that it will be a
bit awkward to introduce additional check in the ->dwapb_gpio_unregister() for
freeing ACPI Event handling which will be removed by one of following patch
(even taking into consideration backporting).

What I propose here is an alternative, i.e. I make this patch as first in the
series and will focus / dedicate it as a fix rather than clean up.

> So your series will look like this:
> gpio: dwapb: avoid error message for optional IRQ
> gpio: dwapb: Don't use 0 as valid Linux interrupt number

> gpio: dwapb: Call acpi_gpiochip_free_interrupts() on GPIO chip de-registration (<= This commit can be moved to the head of the series as being marked by the
> Fixes tag)

Yes, something like this, but keeping this form of the patch.

Thanks for the suggestion!

> gpio: dwapb: Remove redundant has_irq flag support

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 3/4] gpio: dwapb: Drop extra check to call acpi_gpiochip_request_interrupts()

[Andy Shevchenko]

On Sun, May 17, 2020 at 04:55:21PM +0300, Serge Semin wrote:
> On Tue, May 12, 2020 at 09:45:12PM +0300, Andy Shevchenko wrote:
> > There is no need to have an additional check to call
> > acpi_gpiochip_request_interrupts(). Even without any interrupts available
> > the registered ACPI Event handlers can be useful for debugging purposes.

...

> > -	if (pp->has_irq)
> > -		acpi_gpiochip_request_interrupts(&port->gc);
> > +	acpi_gpiochip_request_interrupts(&port->gc);
> 
> Hm, perhaps replacing it with:
> +	if (pp->idx == 0)
> +		acpi_gpiochip_request_interrupts(&port->gc);
> could be more appropriate seeing Port A only supports IRQs, which we'd point
> out by the (idx == 0) conditional statement. So we don't have to call
> the method at most four times for each available port. Though judging by the
> acpi_gpiochip_request_interrupts() function internals it will just ignore
> GPIO chips with no IRQ support. Andy, It's up to you to decide. I'm not against
> the change the way it is, but if you agree that signifying the IRQs affiliation
> would be better, then please fill free to add the conditional statement I
> suggested.

It's really harmless to call it for each port. It allows as a side effect see
issues with ACPI tables which may refer to a wrong port / device and thus
getting no certain event handled. I prefer to unconditionally call it.

-- 
With Best Regards,
Andy Shevchenko

[PATCH v1 4/4] gpio: dwapb: Remove unneeded has_irq member in struct dwapb_port_property

[Andy Shevchenko]

has_irq member of struct dwapb_port_property is used only in one place,
so, make it local test instead and remove from the structure.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Cc: Serge Semin <fancer.lancer@gmail.com>
---
 drivers/gpio/gpio-dwapb.c                | 12 +++++++-----
 include/linux/platform_data/gpio-dwapb.h |  1 -
 2 files changed, 7 insertions(+), 6 deletions(-)

diff --git a/drivers/gpio/gpio-dwapb.c b/drivers/gpio/gpio-dwapb.c
index 2975f2d369874a..87834adccc4534 100644
--- a/drivers/gpio/gpio-dwapb.c
+++ b/drivers/gpio/gpio-dwapb.c
@@ -366,6 +366,11 @@ static void dwapb_configure_irqs(struct dwapb_gpio *gpio,
 	irq_hw_number_t hwirq;
 	int err, i;
 
+	if (memchr_inv(pp->irq, 0, sizeof(pp->irq)) == NULL) {
+		dev_warn(gpio->dev, "no IRQ for port%d\n", pp->idx);
+		return;
+	}
+
 	gpio->domain = irq_domain_create_linear(fwnode, ngpio,
 						 &irq_generic_chip_ops, gpio);
 	if (!gpio->domain)
@@ -501,7 +506,8 @@ static int dwapb_gpio_add_port(struct dwapb_gpio *gpio,
 	if (pp->idx == 0)
 		port->gc.set_config = dwapb_gpio_set_config;
 
-	if (pp->has_irq)
+	/* Only port A can provide interrupts in all configurations of the IP */
+	if (pp->idx == 0)
 		dwapb_configure_irqs(gpio, port, pp);
 
 	err = gpiochip_add_data(&port->gc, port);
@@ -553,10 +559,6 @@ static void dwapb_get_irq(struct device *dev, struct fwnode_handle *fwnode,
 
 		pp->irq[j] = err;
 	}
-
-	pp->has_irq = memchr_inv(pp->irq, 0, sizeof(pp->irq)) != NULL;
-	if (!pp->has_irq)
-		dev_warn(dev, "no irq for port%d\n", pp->idx);
 }
 
 static struct dwapb_platform_data *dwapb_gpio_get_pdata(struct device *dev)
diff --git a/include/linux/platform_data/gpio-dwapb.h b/include/linux/platform_data/gpio-dwapb.h
index 3c606c450d0596..ff1be737bad6aa 100644
--- a/include/linux/platform_data/gpio-dwapb.h
+++ b/include/linux/platform_data/gpio-dwapb.h
@@ -12,7 +12,6 @@ struct dwapb_port_property {
 	unsigned int	ngpio;
 	unsigned int	gpio_base;
 	int		irq[32];
-	bool		has_irq;
 	bool		irq_shared;
 };
 
-- 
2.26.2

Re: [PATCH v1 4/4] gpio: dwapb: Remove unneeded has_irq member in struct dwapb_port_property

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 4808 bytes --]

Hi Andy,

I love your patch! Yet something to improve:

[auto build test ERROR on gpio/for-next]
[also build test ERROR on next-20200512]
[cannot apply to linus/master v5.7-rc5]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]

url:    https://github.com/0day-ci/linux/commits/Andy-Shevchenko/gpio-dwapb-avoid-error-message-for-optional-IRQ/20200513-025227
base:   https://git.kernel.org/pub/scm/linux/kernel/git/linusw/linux-gpio.git for-next
config: x86_64-randconfig-a003-20200512 (attached as .config)
reproduce:
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # install x86_64 cross compiling tool for clang build
        # apt-get install binutils-x86-64-linux-gnu
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=x86_64 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kbuild test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

drivers/mfd/intel_quark_i2c_gpio.c: In function 'intel_quark_gpio_setup':
>> drivers/mfd/intel_quark_i2c_gpio.c:219:19: error: 'struct dwapb_port_property' has no member named 'has_irq'
pdata->properties->has_irq = true;
^~

vim +219 drivers/mfd/intel_quark_i2c_gpio.c

60ae5b9f5cdd80 Raymond Tan   2015-02-02  189  
60ae5b9f5cdd80 Raymond Tan   2015-02-02  190  static int intel_quark_gpio_setup(struct pci_dev *pdev, struct mfd_cell *cell)
60ae5b9f5cdd80 Raymond Tan   2015-02-02  191  {
60ae5b9f5cdd80 Raymond Tan   2015-02-02  192  	struct dwapb_platform_data *pdata;
60ae5b9f5cdd80 Raymond Tan   2015-02-02  193  	struct resource *res = (struct resource *)cell->resources;
60ae5b9f5cdd80 Raymond Tan   2015-02-02  194  	struct device *dev = &pdev->dev;
60ae5b9f5cdd80 Raymond Tan   2015-02-02  195  
60ae5b9f5cdd80 Raymond Tan   2015-02-02  196  	res[INTEL_QUARK_IORES_MEM].start =
60ae5b9f5cdd80 Raymond Tan   2015-02-02  197  		pci_resource_start(pdev, MFD_GPIO_BAR);
60ae5b9f5cdd80 Raymond Tan   2015-02-02  198  	res[INTEL_QUARK_IORES_MEM].end =
60ae5b9f5cdd80 Raymond Tan   2015-02-02  199  		pci_resource_end(pdev, MFD_GPIO_BAR);
60ae5b9f5cdd80 Raymond Tan   2015-02-02  200  
60ae5b9f5cdd80 Raymond Tan   2015-02-02  201  	pdata = devm_kzalloc(dev, sizeof(*pdata), GFP_KERNEL);
60ae5b9f5cdd80 Raymond Tan   2015-02-02  202  	if (!pdata)
60ae5b9f5cdd80 Raymond Tan   2015-02-02  203  		return -ENOMEM;
60ae5b9f5cdd80 Raymond Tan   2015-02-02  204  
60ae5b9f5cdd80 Raymond Tan   2015-02-02  205  	/* For intel quark x1000, it has only one port: portA */
60ae5b9f5cdd80 Raymond Tan   2015-02-02  206  	pdata->nports = INTEL_QUARK_GPIO_NPORTS;
60ae5b9f5cdd80 Raymond Tan   2015-02-02  207  	pdata->properties = devm_kcalloc(dev, pdata->nports,
60ae5b9f5cdd80 Raymond Tan   2015-02-02  208  					 sizeof(*pdata->properties),
60ae5b9f5cdd80 Raymond Tan   2015-02-02  209  					 GFP_KERNEL);
60ae5b9f5cdd80 Raymond Tan   2015-02-02  210  	if (!pdata->properties)
60ae5b9f5cdd80 Raymond Tan   2015-02-02  211  		return -ENOMEM;
60ae5b9f5cdd80 Raymond Tan   2015-02-02  212  
60ae5b9f5cdd80 Raymond Tan   2015-02-02  213  	/* Set the properties for portA */
4ba8cfa79f44a9 Jiang Qiu     2016-04-28  214  	pdata->properties->fwnode	= NULL;
60ae5b9f5cdd80 Raymond Tan   2015-02-02  215  	pdata->properties->idx		= 0;
60ae5b9f5cdd80 Raymond Tan   2015-02-02  216  	pdata->properties->ngpio	= INTEL_QUARK_MFD_NGPIO;
60ae5b9f5cdd80 Raymond Tan   2015-02-02  217  	pdata->properties->gpio_base	= INTEL_QUARK_MFD_GPIO_BASE;
e6ca26abd37606 Phil Edworthy 2018-04-26  218  	pdata->properties->irq[0]	= pdev->irq;
e6ca26abd37606 Phil Edworthy 2018-04-26 @219  	pdata->properties->has_irq	= true;
60ae5b9f5cdd80 Raymond Tan   2015-02-02  220  	pdata->properties->irq_shared	= true;
60ae5b9f5cdd80 Raymond Tan   2015-02-02  221  
60ae5b9f5cdd80 Raymond Tan   2015-02-02  222  	cell->platform_data = pdata;
60ae5b9f5cdd80 Raymond Tan   2015-02-02  223  	cell->pdata_size = sizeof(*pdata);
60ae5b9f5cdd80 Raymond Tan   2015-02-02  224  
60ae5b9f5cdd80 Raymond Tan   2015-02-02  225  	return 0;
60ae5b9f5cdd80 Raymond Tan   2015-02-02  226  }
60ae5b9f5cdd80 Raymond Tan   2015-02-02  227  

:::::: The code at line 219 was first introduced by commit
:::::: e6ca26abd37606ba4864f20c85d3fe4a2173b93f gpio: dwapb: Add support for 1 interrupt per port A GPIO

:::::: TO: Phil Edworthy <phil.edworthy@renesas.com>
:::::: CC: Linus Walleij <linus.walleij@linaro.org>

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 40911 bytes --]

Re: [PATCH v1 4/4] gpio: dwapb: Remove unneeded has_irq member in struct dwapb_port_property

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 4893 bytes --]

Hi Andy,

I love your patch! Yet something to improve:

[auto build test ERROR on gpio/for-next]
[also build test ERROR on next-20200512]
[cannot apply to linus/master v5.7-rc5]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]

url:    https://github.com/0day-ci/linux/commits/Andy-Shevchenko/gpio-dwapb-avoid-error-message-for-optional-IRQ/20200513-025227
base:   https://git.kernel.org/pub/scm/linux/kernel/git/linusw/linux-gpio.git for-next
config: x86_64-randconfig-a003-20200512 (attached as .config)
reproduce:
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # install x86_64 cross compiling tool for clang build
        # apt-get install binutils-x86-64-linux-gnu
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=x86_64 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kbuild test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

drivers/mfd/intel_quark_i2c_gpio.c: In function 'intel_quark_gpio_setup':
>> drivers/mfd/intel_quark_i2c_gpio.c:219:19: error: 'struct dwapb_port_property' has no member named 'has_irq'
pdata->properties->has_irq = true;
^~

vim +219 drivers/mfd/intel_quark_i2c_gpio.c

60ae5b9f5cdd80 Raymond Tan   2015-02-02  189  
60ae5b9f5cdd80 Raymond Tan   2015-02-02  190  static int intel_quark_gpio_setup(struct pci_dev *pdev, struct mfd_cell *cell)
60ae5b9f5cdd80 Raymond Tan   2015-02-02  191  {
60ae5b9f5cdd80 Raymond Tan   2015-02-02  192  	struct dwapb_platform_data *pdata;
60ae5b9f5cdd80 Raymond Tan   2015-02-02  193  	struct resource *res = (struct resource *)cell->resources;
60ae5b9f5cdd80 Raymond Tan   2015-02-02  194  	struct device *dev = &pdev->dev;
60ae5b9f5cdd80 Raymond Tan   2015-02-02  195  
60ae5b9f5cdd80 Raymond Tan   2015-02-02  196  	res[INTEL_QUARK_IORES_MEM].start =
60ae5b9f5cdd80 Raymond Tan   2015-02-02  197  		pci_resource_start(pdev, MFD_GPIO_BAR);
60ae5b9f5cdd80 Raymond Tan   2015-02-02  198  	res[INTEL_QUARK_IORES_MEM].end =
60ae5b9f5cdd80 Raymond Tan   2015-02-02  199  		pci_resource_end(pdev, MFD_GPIO_BAR);
60ae5b9f5cdd80 Raymond Tan   2015-02-02  200  
60ae5b9f5cdd80 Raymond Tan   2015-02-02  201  	pdata = devm_kzalloc(dev, sizeof(*pdata), GFP_KERNEL);
60ae5b9f5cdd80 Raymond Tan   2015-02-02  202  	if (!pdata)
60ae5b9f5cdd80 Raymond Tan   2015-02-02  203  		return -ENOMEM;
60ae5b9f5cdd80 Raymond Tan   2015-02-02  204  
60ae5b9f5cdd80 Raymond Tan   2015-02-02  205  	/* For intel quark x1000, it has only one port: portA */
60ae5b9f5cdd80 Raymond Tan   2015-02-02  206  	pdata->nports = INTEL_QUARK_GPIO_NPORTS;
60ae5b9f5cdd80 Raymond Tan   2015-02-02  207  	pdata->properties = devm_kcalloc(dev, pdata->nports,
60ae5b9f5cdd80 Raymond Tan   2015-02-02  208  					 sizeof(*pdata->properties),
60ae5b9f5cdd80 Raymond Tan   2015-02-02  209  					 GFP_KERNEL);
60ae5b9f5cdd80 Raymond Tan   2015-02-02  210  	if (!pdata->properties)
60ae5b9f5cdd80 Raymond Tan   2015-02-02  211  		return -ENOMEM;
60ae5b9f5cdd80 Raymond Tan   2015-02-02  212  
60ae5b9f5cdd80 Raymond Tan   2015-02-02  213  	/* Set the properties for portA */
4ba8cfa79f44a9 Jiang Qiu     2016-04-28  214  	pdata->properties->fwnode	= NULL;
60ae5b9f5cdd80 Raymond Tan   2015-02-02  215  	pdata->properties->idx		= 0;
60ae5b9f5cdd80 Raymond Tan   2015-02-02  216  	pdata->properties->ngpio	= INTEL_QUARK_MFD_NGPIO;
60ae5b9f5cdd80 Raymond Tan   2015-02-02  217  	pdata->properties->gpio_base	= INTEL_QUARK_MFD_GPIO_BASE;
e6ca26abd37606 Phil Edworthy 2018-04-26  218  	pdata->properties->irq[0]	= pdev->irq;
e6ca26abd37606 Phil Edworthy 2018-04-26 @219  	pdata->properties->has_irq	= true;
60ae5b9f5cdd80 Raymond Tan   2015-02-02  220  	pdata->properties->irq_shared	= true;
60ae5b9f5cdd80 Raymond Tan   2015-02-02  221  
60ae5b9f5cdd80 Raymond Tan   2015-02-02  222  	cell->platform_data = pdata;
60ae5b9f5cdd80 Raymond Tan   2015-02-02  223  	cell->pdata_size = sizeof(*pdata);
60ae5b9f5cdd80 Raymond Tan   2015-02-02  224  
60ae5b9f5cdd80 Raymond Tan   2015-02-02  225  	return 0;
60ae5b9f5cdd80 Raymond Tan   2015-02-02  226  }
60ae5b9f5cdd80 Raymond Tan   2015-02-02  227  

:::::: The code at line 219 was first introduced by commit
:::::: e6ca26abd37606ba4864f20c85d3fe4a2173b93f gpio: dwapb: Add support for 1 interrupt per port A GPIO

:::::: TO: Phil Edworthy <phil.edworthy@renesas.com>
:::::: CC: Linus Walleij <linus.walleij@linaro.org>

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org

[-- Attachment #2: config.gz --]
[-- Type: application/gzip, Size: 40911 bytes --]

Re: [PATCH v1 4/4] gpio: dwapb: Remove unneeded has_irq member in struct dwapb_port_property

[Andy Shevchenko]

On Wed, May 13, 2020 at 06:53:09AM +0800, kbuild test robot wrote:
> Hi Andy,
> 
> I love your patch! Yet something to improve:
> 
> [auto build test ERROR on gpio/for-next]
> [also build test ERROR on next-20200512]
> [cannot apply to linus/master v5.7-rc5]
> [if your patch is applied to the wrong git tree, please drop us a note to help
> improve the system. BTW, we also suggest to use '--base' option to specify the
> base tree in git format-patch, please see https://stackoverflow.com/a/37406982]
> 
> url:    https://github.com/0day-ci/linux/commits/Andy-Shevchenko/gpio-dwapb-avoid-error-message-for-optional-IRQ/20200513-025227
> base:   https://git.kernel.org/pub/scm/linux/kernel/git/linusw/linux-gpio.git for-next
> config: x86_64-randconfig-a003-20200512 (attached as .config)
> reproduce:
>         wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
>         chmod +x ~/bin/make.cross
>         # install x86_64 cross compiling tool for clang build
>         # apt-get install binutils-x86-64-linux-gnu
>         # save the attached .config to linux build tree
>         COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=x86_64 
> 
> If you fix the issue, kindly add following tag as appropriate
> Reported-by: kbuild test robot <lkp@intel.com>
> 
> All errors (new ones prefixed by >>):
> 
> drivers/mfd/intel_quark_i2c_gpio.c: In function 'intel_quark_gpio_setup':
> >> drivers/mfd/intel_quark_i2c_gpio.c:219:19: error: 'struct dwapb_port_property' has no member named 'has_irq'
> pdata->properties->has_irq = true;
> ^~

Indeed. I have that removed by other WIP patch in my tree.
Thanks!

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 4/4] gpio: dwapb: Remove unneeded has_irq member in struct dwapb_port_property

[Andy Shevchenko]

[-- Attachment #1: Type: text/plain, Size: 1748 bytes --]

On Wed, May 13, 2020 at 06:53:09AM +0800, kbuild test robot wrote:
> Hi Andy,
> 
> I love your patch! Yet something to improve:
> 
> [auto build test ERROR on gpio/for-next]
> [also build test ERROR on next-20200512]
> [cannot apply to linus/master v5.7-rc5]
> [if your patch is applied to the wrong git tree, please drop us a note to help
> improve the system. BTW, we also suggest to use '--base' option to specify the
> base tree in git format-patch, please see https://stackoverflow.com/a/37406982]
> 
> url:    https://github.com/0day-ci/linux/commits/Andy-Shevchenko/gpio-dwapb-avoid-error-message-for-optional-IRQ/20200513-025227
> base:   https://git.kernel.org/pub/scm/linux/kernel/git/linusw/linux-gpio.git for-next
> config: x86_64-randconfig-a003-20200512 (attached as .config)
> reproduce:
>         wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
>         chmod +x ~/bin/make.cross
>         # install x86_64 cross compiling tool for clang build
>         # apt-get install binutils-x86-64-linux-gnu
>         # save the attached .config to linux build tree
>         COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=x86_64 
> 
> If you fix the issue, kindly add following tag as appropriate
> Reported-by: kbuild test robot <lkp@intel.com>
> 
> All errors (new ones prefixed by >>):
> 
> drivers/mfd/intel_quark_i2c_gpio.c: In function 'intel_quark_gpio_setup':
> >> drivers/mfd/intel_quark_i2c_gpio.c:219:19: error: 'struct dwapb_port_property' has no member named 'has_irq'
> pdata->properties->has_irq = true;
> ^~

Indeed. I have that removed by other WIP patch in my tree.
Thanks!

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 4/4] gpio: dwapb: Remove unneeded has_irq member in struct dwapb_port_property

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 4837 bytes --]

Hi Andy,

I love your patch! Yet something to improve:

[auto build test ERROR on gpio/for-next]
[also build test ERROR on next-20200512]
[cannot apply to linus/master v5.7-rc5]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]

url:    https://github.com/0day-ci/linux/commits/Andy-Shevchenko/gpio-dwapb-avoid-error-message-for-optional-IRQ/20200513-025227
base:   https://git.kernel.org/pub/scm/linux/kernel/git/linusw/linux-gpio.git for-next
config: i386-allyesconfig (attached as .config)
compiler: gcc-7 (Ubuntu 7.5.0-6ubuntu2) 7.5.0
reproduce:
        # save the attached .config to linux build tree
        make ARCH=i386 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kbuild test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

drivers/mfd/intel_quark_i2c_gpio.c: In function 'intel_quark_gpio_setup':
>> drivers/mfd/intel_quark_i2c_gpio.c:219:19: error: 'struct dwapb_port_property' has no member named 'has_irq'
pdata->properties->has_irq = true;
^~

# https://github.com/0day-ci/linux/commit/f8686f0bb4ff7fdded832484a559f1454a2a51d2
git remote add linux-review https://github.com/0day-ci/linux
git remote update linux-review
git checkout f8686f0bb4ff7fdded832484a559f1454a2a51d2
vim +219 drivers/mfd/intel_quark_i2c_gpio.c

60ae5b9f5cdd80c5 Raymond Tan   2015-02-02  189  
60ae5b9f5cdd80c5 Raymond Tan   2015-02-02  190  static int intel_quark_gpio_setup(struct pci_dev *pdev, struct mfd_cell *cell)
60ae5b9f5cdd80c5 Raymond Tan   2015-02-02  191  {
60ae5b9f5cdd80c5 Raymond Tan   2015-02-02  192  	struct dwapb_platform_data *pdata;
60ae5b9f5cdd80c5 Raymond Tan   2015-02-02  193  	struct resource *res = (struct resource *)cell->resources;
60ae5b9f5cdd80c5 Raymond Tan   2015-02-02  194  	struct device *dev = &pdev->dev;
60ae5b9f5cdd80c5 Raymond Tan   2015-02-02  195  
60ae5b9f5cdd80c5 Raymond Tan   2015-02-02  196  	res[INTEL_QUARK_IORES_MEM].start =
60ae5b9f5cdd80c5 Raymond Tan   2015-02-02  197  		pci_resource_start(pdev, MFD_GPIO_BAR);
60ae5b9f5cdd80c5 Raymond Tan   2015-02-02  198  	res[INTEL_QUARK_IORES_MEM].end =
60ae5b9f5cdd80c5 Raymond Tan   2015-02-02  199  		pci_resource_end(pdev, MFD_GPIO_BAR);
60ae5b9f5cdd80c5 Raymond Tan   2015-02-02  200  
60ae5b9f5cdd80c5 Raymond Tan   2015-02-02  201  	pdata = devm_kzalloc(dev, sizeof(*pdata), GFP_KERNEL);
60ae5b9f5cdd80c5 Raymond Tan   2015-02-02  202  	if (!pdata)
60ae5b9f5cdd80c5 Raymond Tan   2015-02-02  203  		return -ENOMEM;
60ae5b9f5cdd80c5 Raymond Tan   2015-02-02  204  
60ae5b9f5cdd80c5 Raymond Tan   2015-02-02  205  	/* For intel quark x1000, it has only one port: portA */
60ae5b9f5cdd80c5 Raymond Tan   2015-02-02  206  	pdata->nports = INTEL_QUARK_GPIO_NPORTS;
60ae5b9f5cdd80c5 Raymond Tan   2015-02-02  207  	pdata->properties = devm_kcalloc(dev, pdata->nports,
60ae5b9f5cdd80c5 Raymond Tan   2015-02-02  208  					 sizeof(*pdata->properties),
60ae5b9f5cdd80c5 Raymond Tan   2015-02-02  209  					 GFP_KERNEL);
60ae5b9f5cdd80c5 Raymond Tan   2015-02-02  210  	if (!pdata->properties)
60ae5b9f5cdd80c5 Raymond Tan   2015-02-02  211  		return -ENOMEM;
60ae5b9f5cdd80c5 Raymond Tan   2015-02-02  212  
60ae5b9f5cdd80c5 Raymond Tan   2015-02-02  213  	/* Set the properties for portA */
4ba8cfa79f44a948 Jiang Qiu     2016-04-28  214  	pdata->properties->fwnode	= NULL;
60ae5b9f5cdd80c5 Raymond Tan   2015-02-02  215  	pdata->properties->idx		= 0;
60ae5b9f5cdd80c5 Raymond Tan   2015-02-02  216  	pdata->properties->ngpio	= INTEL_QUARK_MFD_NGPIO;
60ae5b9f5cdd80c5 Raymond Tan   2015-02-02  217  	pdata->properties->gpio_base	= INTEL_QUARK_MFD_GPIO_BASE;
e6ca26abd37606ba Phil Edworthy 2018-04-26  218  	pdata->properties->irq[0]	= pdev->irq;
e6ca26abd37606ba Phil Edworthy 2018-04-26 @219  	pdata->properties->has_irq	= true;
60ae5b9f5cdd80c5 Raymond Tan   2015-02-02  220  	pdata->properties->irq_shared	= true;
60ae5b9f5cdd80c5 Raymond Tan   2015-02-02  221  
60ae5b9f5cdd80c5 Raymond Tan   2015-02-02  222  	cell->platform_data = pdata;
60ae5b9f5cdd80c5 Raymond Tan   2015-02-02  223  	cell->pdata_size = sizeof(*pdata);
60ae5b9f5cdd80c5 Raymond Tan   2015-02-02  224  
60ae5b9f5cdd80c5 Raymond Tan   2015-02-02  225  	return 0;
60ae5b9f5cdd80c5 Raymond Tan   2015-02-02  226  }
60ae5b9f5cdd80c5 Raymond Tan   2015-02-02  227  

:::::: The code at line 219 was first introduced by commit
:::::: e6ca26abd37606ba4864f20c85d3fe4a2173b93f gpio: dwapb: Add support for 1 interrupt per port A GPIO

:::::: TO: Phil Edworthy <phil.edworthy@renesas.com>
:::::: CC: Linus Walleij <linus.walleij@linaro.org>

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 72422 bytes --]

[-- Attachment #3: Type: text/plain, Size: 149 bytes --]

_______________________________________________
kbuild mailing list -- kbuild@lists.01.org
To unsubscribe send an email to kbuild-leave@lists.01.org

Re: [PATCH v1 4/4] gpio: dwapb: Remove unneeded has_irq member in struct dwapb_port_property

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 5081 bytes --]

Hi Andy,

I love your patch! Yet something to improve:

[auto build test ERROR on gpio/for-next]
[also build test ERROR on next-20200512]
[cannot apply to linus/master v5.7-rc5]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]

url:    https://github.com/0day-ci/linux/commits/Andy-Shevchenko/gpio-dwapb-avoid-error-message-for-optional-IRQ/20200513-025227
base:   https://git.kernel.org/pub/scm/linux/kernel/git/linusw/linux-gpio.git for-next
config: i386-allyesconfig (attached as .config)
compiler: gcc-7 (Ubuntu 7.5.0-6ubuntu2) 7.5.0
reproduce:
        # save the attached .config to linux build tree
        make ARCH=i386 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kbuild test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

drivers/mfd/intel_quark_i2c_gpio.c: In function 'intel_quark_gpio_setup':
>> drivers/mfd/intel_quark_i2c_gpio.c:219:19: error: 'struct dwapb_port_property' has no member named 'has_irq'
pdata->properties->has_irq = true;
^~

# https://github.com/0day-ci/linux/commit/f8686f0bb4ff7fdded832484a559f1454a2a51d2
git remote add linux-review https://github.com/0day-ci/linux
git remote update linux-review
git checkout f8686f0bb4ff7fdded832484a559f1454a2a51d2
vim +219 drivers/mfd/intel_quark_i2c_gpio.c

60ae5b9f5cdd80c5 Raymond Tan   2015-02-02  189  
60ae5b9f5cdd80c5 Raymond Tan   2015-02-02  190  static int intel_quark_gpio_setup(struct pci_dev *pdev, struct mfd_cell *cell)
60ae5b9f5cdd80c5 Raymond Tan   2015-02-02  191  {
60ae5b9f5cdd80c5 Raymond Tan   2015-02-02  192  	struct dwapb_platform_data *pdata;
60ae5b9f5cdd80c5 Raymond Tan   2015-02-02  193  	struct resource *res = (struct resource *)cell->resources;
60ae5b9f5cdd80c5 Raymond Tan   2015-02-02  194  	struct device *dev = &pdev->dev;
60ae5b9f5cdd80c5 Raymond Tan   2015-02-02  195  
60ae5b9f5cdd80c5 Raymond Tan   2015-02-02  196  	res[INTEL_QUARK_IORES_MEM].start =
60ae5b9f5cdd80c5 Raymond Tan   2015-02-02  197  		pci_resource_start(pdev, MFD_GPIO_BAR);
60ae5b9f5cdd80c5 Raymond Tan   2015-02-02  198  	res[INTEL_QUARK_IORES_MEM].end =
60ae5b9f5cdd80c5 Raymond Tan   2015-02-02  199  		pci_resource_end(pdev, MFD_GPIO_BAR);
60ae5b9f5cdd80c5 Raymond Tan   2015-02-02  200  
60ae5b9f5cdd80c5 Raymond Tan   2015-02-02  201  	pdata = devm_kzalloc(dev, sizeof(*pdata), GFP_KERNEL);
60ae5b9f5cdd80c5 Raymond Tan   2015-02-02  202  	if (!pdata)
60ae5b9f5cdd80c5 Raymond Tan   2015-02-02  203  		return -ENOMEM;
60ae5b9f5cdd80c5 Raymond Tan   2015-02-02  204  
60ae5b9f5cdd80c5 Raymond Tan   2015-02-02  205  	/* For intel quark x1000, it has only one port: portA */
60ae5b9f5cdd80c5 Raymond Tan   2015-02-02  206  	pdata->nports = INTEL_QUARK_GPIO_NPORTS;
60ae5b9f5cdd80c5 Raymond Tan   2015-02-02  207  	pdata->properties = devm_kcalloc(dev, pdata->nports,
60ae5b9f5cdd80c5 Raymond Tan   2015-02-02  208  					 sizeof(*pdata->properties),
60ae5b9f5cdd80c5 Raymond Tan   2015-02-02  209  					 GFP_KERNEL);
60ae5b9f5cdd80c5 Raymond Tan   2015-02-02  210  	if (!pdata->properties)
60ae5b9f5cdd80c5 Raymond Tan   2015-02-02  211  		return -ENOMEM;
60ae5b9f5cdd80c5 Raymond Tan   2015-02-02  212  
60ae5b9f5cdd80c5 Raymond Tan   2015-02-02  213  	/* Set the properties for portA */
4ba8cfa79f44a948 Jiang Qiu     2016-04-28  214  	pdata->properties->fwnode	= NULL;
60ae5b9f5cdd80c5 Raymond Tan   2015-02-02  215  	pdata->properties->idx		= 0;
60ae5b9f5cdd80c5 Raymond Tan   2015-02-02  216  	pdata->properties->ngpio	= INTEL_QUARK_MFD_NGPIO;
60ae5b9f5cdd80c5 Raymond Tan   2015-02-02  217  	pdata->properties->gpio_base	= INTEL_QUARK_MFD_GPIO_BASE;
e6ca26abd37606ba Phil Edworthy 2018-04-26  218  	pdata->properties->irq[0]	= pdev->irq;
e6ca26abd37606ba Phil Edworthy 2018-04-26 @219  	pdata->properties->has_irq	= true;
60ae5b9f5cdd80c5 Raymond Tan   2015-02-02  220  	pdata->properties->irq_shared	= true;
60ae5b9f5cdd80c5 Raymond Tan   2015-02-02  221  
60ae5b9f5cdd80c5 Raymond Tan   2015-02-02  222  	cell->platform_data = pdata;
60ae5b9f5cdd80c5 Raymond Tan   2015-02-02  223  	cell->pdata_size = sizeof(*pdata);
60ae5b9f5cdd80c5 Raymond Tan   2015-02-02  224  
60ae5b9f5cdd80c5 Raymond Tan   2015-02-02  225  	return 0;
60ae5b9f5cdd80c5 Raymond Tan   2015-02-02  226  }
60ae5b9f5cdd80c5 Raymond Tan   2015-02-02  227  

:::::: The code at line 219 was first introduced by commit
:::::: e6ca26abd37606ba4864f20c85d3fe4a2173b93f gpio: dwapb: Add support for 1 interrupt per port A GPIO

:::::: TO: Phil Edworthy <phil.edworthy@renesas.com>
:::::: CC: Linus Walleij <linus.walleij@linaro.org>

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org

_______________________________________________
kbuild mailing list -- kbuild(a)lists.01.org
To unsubscribe send an email to kbuild-leave(a)lists.01.org

[-- Attachment #2: config.gz --]
[-- Type: application/gzip, Size: 72422 bytes --]

Re: [PATCH v1 4/4] gpio: dwapb: Remove unneeded has_irq member in struct dwapb_port_property

[Serge Semin]

On Tue, May 12, 2020 at 09:45:13PM +0300, Andy Shevchenko wrote:
> has_irq member of struct dwapb_port_property is used only in one place,
> so, make it local test instead and remove from the structure.
> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Cc: Serge Semin <fancer.lancer@gmail.com>
> ---
>  drivers/gpio/gpio-dwapb.c                | 12 +++++++-----
>  include/linux/platform_data/gpio-dwapb.h |  1 -
>  2 files changed, 7 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/gpio/gpio-dwapb.c b/drivers/gpio/gpio-dwapb.c
> index 2975f2d369874a..87834adccc4534 100644
> --- a/drivers/gpio/gpio-dwapb.c
> +++ b/drivers/gpio/gpio-dwapb.c
> @@ -366,6 +366,11 @@ static void dwapb_configure_irqs(struct dwapb_gpio *gpio,
>  	irq_hw_number_t hwirq;
>  	int err, i;
>  

> +	if (memchr_inv(pp->irq, 0, sizeof(pp->irq)) == NULL) {
> +		dev_warn(gpio->dev, "no IRQ for port%d\n", pp->idx);
> +		return;
> +	}
> +

Ah, that's why you added the memchr_inv() method in patch 2. So to move it
to dwapb_configure_irqs() at this point. Anyway I still think, that it would be
better to leave the has_irq initialization in the loop there, but here you could
just remove that assignment. For this patch:

Reviewed-by: Serge Semin <fancer.lancer@gmail.com>

I'll test the whole series up when you send v2. Our hardware is equipped with
two DW APB GPIO IPs with Port A enabled for each. One of them is connected to an
interrupt controller by a single line.

-Sergey

>  	gpio->domain = irq_domain_create_linear(fwnode, ngpio,
>  						 &irq_generic_chip_ops, gpio);
>  	if (!gpio->domain)
> @@ -501,7 +506,8 @@ static int dwapb_gpio_add_port(struct dwapb_gpio *gpio,
>  	if (pp->idx == 0)
>  		port->gc.set_config = dwapb_gpio_set_config;
>  
> -	if (pp->has_irq)
> +	/* Only port A can provide interrupts in all configurations of the IP */
> +	if (pp->idx == 0)
>  		dwapb_configure_irqs(gpio, port, pp);
>  
>  	err = gpiochip_add_data(&port->gc, port);
> @@ -553,10 +559,6 @@ static void dwapb_get_irq(struct device *dev, struct fwnode_handle *fwnode,
>  
>  		pp->irq[j] = err;
>  	}
> -
> -	pp->has_irq = memchr_inv(pp->irq, 0, sizeof(pp->irq)) != NULL;
> -	if (!pp->has_irq)
> -		dev_warn(dev, "no irq for port%d\n", pp->idx);
>  }
>  
>  static struct dwapb_platform_data *dwapb_gpio_get_pdata(struct device *dev)
> diff --git a/include/linux/platform_data/gpio-dwapb.h b/include/linux/platform_data/gpio-dwapb.h
> index 3c606c450d0596..ff1be737bad6aa 100644
> --- a/include/linux/platform_data/gpio-dwapb.h
> +++ b/include/linux/platform_data/gpio-dwapb.h
> @@ -12,7 +12,6 @@ struct dwapb_port_property {
>  	unsigned int	ngpio;
>  	unsigned int	gpio_base;
>  	int		irq[32];
> -	bool		has_irq;
>  	bool		irq_shared;
>  };
>  
> -- 
> 2.26.2
> 

Re: [PATCH v1 4/4] gpio: dwapb: Remove unneeded has_irq member in struct dwapb_port_property

[Andy Shevchenko]

On Sun, May 17, 2020 at 05:26:30PM +0300, Serge Semin wrote:
> On Tue, May 12, 2020 at 09:45:13PM +0300, Andy Shevchenko wrote:
> > has_irq member of struct dwapb_port_property is used only in one place,
> > so, make it local test instead and remove from the structure.

...

> > +	if (memchr_inv(pp->irq, 0, sizeof(pp->irq)) == NULL) {
> > +		dev_warn(gpio->dev, "no IRQ for port%d\n", pp->idx);
> > +		return;
> > +	}
> > +
> 
> Ah, that's why you added the memchr_inv() method in patch 2. So to move it
> to dwapb_configure_irqs() at this point.

Yes.

> Anyway I still think, that it would be
> better to leave the has_irq initialization in the loop there, but here you could
> just remove that assignment.

I think you noticed that I don't like to ping-pong in the series, for what you
propose it would be something like

original --->>>

	if (pp->irq[j] >= 0)
		pp->has_irq = true;

after patch 2 (if your suggestion applied) --->>>

	if (irq > 0) {
		pp->irq[j] = irq;
		pp->has->irq = true;
	}

after this patch --->>>

	if (irq > 0)
		pp->irq[j] = irq;

I prefer not to do this.

OTOH, I guess it might work if we leave original conditional separate to
assignment of IRQ itself (with '>= 0' -> '> 0' being replaced).

I'll look what can I do here.

> For this patch:
> 
> Reviewed-by: Serge Semin <fancer.lancer@gmail.com>

Thank you!

> I'll test the whole series up when you send v2. Our hardware is equipped with
> two DW APB GPIO IPs with Port A enabled for each. One of them is connected to an
> interrupt controller by a single line.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 1/4] gpio: dwapb: avoid error message for optional IRQ

[Andy Shevchenko]

On Tue, May 12, 2020 at 09:45:10PM +0300, Andy Shevchenko wrote:
> platform_get_irq() will generate an error message if the requested IRQ
> is not present. Use platform_get_irq_optional() to avoid the error message
> being generated.

I will wait for Serge and others to comment before I'll send v2 with a fix to
the last patch.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 1/4] gpio: dwapb: avoid error message for optional IRQ

[Serge Semin]

Hello Andy

On Wed, May 13, 2020 at 05:43:49PM +0300, Andy Shevchenko wrote:
> On Tue, May 12, 2020 at 09:45:10PM +0300, Andy Shevchenko wrote:
> > platform_get_irq() will generate an error message if the requested IRQ
> > is not present. Use platform_get_irq_optional() to avoid the error message
> > being generated.
> 
> I will wait for Serge and others to comment before I'll send v2 with a fix to
> the last patch.

Thanks for the patchset. I'll have it reviewed it in a few days.

-Sergey

> 
> -- 
> With Best Regards,
> Andy Shevchenko
> 
> 

Re: [PATCH v1 1/4] gpio: dwapb: avoid error message for optional IRQ

[Serge Semin]

Hello Andy,

Thanks for sending this cleanup series. By comments are below and inlined in the
individual patches further in the patchset.

On Tue, May 12, 2020 at 09:45:10PM +0300, Andy Shevchenko wrote:
> platform_get_irq() will generate an error message if the requested IRQ
> is not present. Use platform_get_irq_optional() to avoid the error message
> being generated.
> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Cc: Serge Semin <fancer.lancer@gmail.com>
> ---
>  drivers/gpio/gpio-dwapb.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/gpio/gpio-dwapb.c b/drivers/gpio/gpio-dwapb.c
> index 8639c4a7f46974..5bc5057f071f37 100644
> --- a/drivers/gpio/gpio-dwapb.c
> +++ b/drivers/gpio/gpio-dwapb.c
> @@ -542,7 +542,7 @@ static void dwapb_get_irq(struct device *dev, struct fwnode_handle *fwnode,
>  		if (np)
>  			pp->irq[j] = of_irq_get(np, j);
>  		else if (has_acpi_companion(dev))
> -			pp->irq[j] = platform_get_irq(to_platform_device(dev), j);
> +			pp->irq[j] = platform_get_irq_optional(to_platform_device(dev), j);

If only we didn't have sub-nodes describing the GPIO ports, we could have used
the platform_get_irq_optional() here for both DT and ACPI based devices...
Since we do, there is no way but to have the "if (np) else if(has_acpi_companion())"
conditional statement here. Thanks for sending this patch.

Reviewed-by: Serge Semin <fancer.lancer@gmail.com>

-Sergey

>  
>  		if (pp->irq[j] >= 0)
>  			pp->has_irq = true;
> -- 
> 2.26.2
>