Re: [PATCH V2 2/3] gpio: xilinx: Add interrupt support
On 2020-07-23 12:03 p.m., Andy Shevchenko wrote:
+/**
+ * xgpio_xlate - Translate gpio_spec to the GPIO number and flags
+ * @gc: Pointer to gpio_chip device structure.
+ * @gpiospec: gpio specifier as found in the device tree
+ * @flags: A flags pointer based on binding
+ *
+ * Return:
+ * irq number otherwise -EINVAL
+ */
+static int xgpio_xlate(struct gpio_chip *gc,
+ const struct of_phandle_args *gpiospec, u32 *flags)
+{
+ if (gc->of_gpio_n_cells < 2) {
+ WARN_ON(1);
+ return -EINVAL;
+ }
+
+ if (WARN_ON(gpiospec->args_count < gc->of_gpio_n_cells))
+ return -EINVAL;
+
+ if (gpiospec->args[0] >= gc->ngpio)
+ return -EINVAL;
+
+ if (flags)
+ *flags = gpiospec->args[1];
+
+ return gpiospec->args[0];
+}
This looks like a very standart xlate function for GPIO. Why do you
need to open-code it?
Indeed, this seems the same as the of_gpio_simple_xlate callback which
is used if no xlate callback is specified, so I'm not sure why this is
necessary?
...
+/**
+ * xgpio_irq_ack - Acknowledge a child GPIO interrupt.
+ * This currently does nothing, but irq_ack is unconditionally called by
+ * handle_edge_irq and therefore must be defined.
This should go after parameter description(s).
+ * @irq_data: per irq and chip data passed down to chip functions
+ */
...
/**
+ * xgpio_irq_mask - Write the specified signal of the GPIO device.
+ * @irq_data: per irq and chip data passed down to chip functions
In all comments irq -> IRQ.
+ */
+static void xgpio_irq_mask(struct irq_data *irq_data)
+{
+ unsigned long flags;
+ struct xgpio_instance *chip = irq_data_get_irq_chip_data(irq_data);
+ int irq_offset = irqd_to_hwirq(irq_data);
+ int index = xgpio_index(chip, irq_offset);
+ int offset = xgpio_offset(chip, irq_offset);
+
+ spin_lock_irqsave(>gpio_lock, flags);
+
+ chip->irq_enable[index] &= ~BIT(offset);
If you convert your data structure to use bitmaps (and respective API) like
#define XILINX_NGPIOS 64
...
DECLARE_BITMAP(irq_enable, XILINX_NGPIOS);
...
it will make code better to read and understand. For example, here it
will be just
__clear_bit(offset, chip->irq_enable);
+ dev_dbg(chip->gc.parent, "Disable %d irq, irq_enable_mask 0x%x\n",
+ irq_offset, chip->irq_enable[index]);
Under spin lock?! Hmm...
+ if (!chip->irq_enable[index]) {
+ /* Disable per channel interrupt */
+ u32 temp = xgpio_readreg(chip->regs + XGPIO_IPIER_OFFSET);
+
+ temp &= ~BIT(index);
+ xgpio_writereg(chip->regs + XGPIO_IPIER_OFFSET, temp);
+ }
+ spin_unlock_irqrestore(>gpio_lock, flags);
+}
...
+ for (index = 0; index < num_channels; index++) {
+ if ((status & BIT(index))) {
If gpio_width is the same among banks, you can use for_each_set_bit()
here as well.
...
+ for_each_set_bit(bit, _events, 32) {
+ generic_handle_irq(irq_find_mapping
+ (chip->gc.irq.domain, offset + bit));
Strange indentation. Maybe a temporary variable helps?
...
+ chip->irq = platform_get_irq_optional(pdev, 0);
+ if (chip->irq <= 0) {
+ dev_info(>dev, "GPIO IRQ not set\n");
Why do you need an optional variant if you print an error anyway?
+ } else {
...
+ chip->gc.irq.parents = (unsigned int *)>irq;
+ chip->gc.irq.num_parents = 1;
Current pattern is to use devm_kcalloc() for it (Linus has plans to
simplify this in the future and this will help him to find what
patterns are being used)
--
Robert Hancock
Senior Hardware Designer
SED Systems, a division of Calian Ltd.
Email: hanc...@sedsystems.ca
Re: [PATCH V2 2/3] gpio: xilinx: Add interrupt support
On Fri, Jul 24, 2020 at 8:15 PM Srinivas Neeli wrote:
...
> > > +#include
> >
> > Not sure I see a user of it.
> >
> > ...
> we are using chained_irq_enter() and chained_irq_exit()
> APIs , so need "chained_irq.h"
I see. But gpio/driver.h does it for you.
...
> > > + for (index = 0; index < num_channels; index++) {
> > > + if ((status & BIT(index))) {
> >
> > If gpio_width is the same among banks, you can use for_each_set_bit()
> > here as well.
> >
> > ...
> gpio_wdith vary depends on design. We can configure gpio pins for each bank.
I see.
...
> > > + chip->irq = platform_get_irq_optional(pdev, 0);
> > > + if (chip->irq <= 0) {
> > > + dev_info(>dev, "GPIO IRQ not set\n");
> >
> > Why do you need an optional variant if you print an error anyway?
>
> Here intention is just printing a debug message to user.
Debug message should be debug, and not info. But in any case, I would
rather drop it, or use platform_get_irq() b/c now you have a code
controversy.
...
> > > + chip->gc.irq.parents = (unsigned int *)>irq;
> > > + chip->gc.irq.num_parents = 1;
> >
> > Current pattern is to use devm_kcalloc() for it (Linus has plans to
> > simplify this in the future and this will help him to find what
> > patterns are being used)
>
> I didn't get this , Could you please explain more.
girq->num_parents = 1;
girq->parents = devm_kcalloc(dev, girq->num_parents, ...);
if (!girq->parents)
return -ENOMEM;
girq->parents[0] = chip->irq;
Also, use temporary variable for IRQ chip structure:
girq = >gc.irq;
--
With Best Regards,
Andy Shevchenko
RE: [PATCH V2 2/3] gpio: xilinx: Add interrupt support
Hi Andy Shevchenko,
Thanks for the review.
Accepted comments will address in V3 and Added few comments in inline.
> -Original Message-
> From: Andy Shevchenko
> Sent: Thursday, July 23, 2020 11:33 PM
> To: Srinivas Neeli
> Cc: Linus Walleij ; Bartosz Golaszewski
> ; Michal Simek ;
> Shubhrajyoti Datta ; Srinivas Goud
> ; open list:GPIO SUBSYSTEM g...@vger.kernel.org>; linux-arm Mailing List ker...@lists.infradead.org>; Linux Kernel Mailing List ker...@vger.kernel.org>; git
> Subject: Re: [PATCH V2 2/3] gpio: xilinx: Add interrupt support
>
> On Thu, Jul 23, 2020 at 5:08 PM Srinivas Neeli
> wrote:
> >
> > Adds interrupt support to the Xilinx GPIO driver so that rising and
> > falling edge line events can be supported. Since interrupt support is
> > an optional feature in the Xilinx IP, the driver continues to support
> > devices which have no interrupt provided.
>
> ...
>
> > +#include
>
> Not sure I see a user of it.
>
> ...
we are using chained_irq_enter() and chained_irq_exit()
APIs , so need "chained_irq.h"
>
> > +/**
> > + * xgpio_xlate - Translate gpio_spec to the GPIO number and flags
> > + * @gc: Pointer to gpio_chip device structure.
> > + * @gpiospec: gpio specifier as found in the device tree
> > + * @flags: A flags pointer based on binding
> > + *
> > + * Return:
> > + * irq number otherwise -EINVAL
> > + */
> > +static int xgpio_xlate(struct gpio_chip *gc,
> > + const struct of_phandle_args *gpiospec, u32
> > +*flags) {
> > + if (gc->of_gpio_n_cells < 2) {
> > + WARN_ON(1);
> > + return -EINVAL;
> > + }
> > +
> > + if (WARN_ON(gpiospec->args_count < gc->of_gpio_n_cells))
> > + return -EINVAL;
> > +
> > + if (gpiospec->args[0] >= gc->ngpio)
> > + return -EINVAL;
> > +
> > + if (flags)
> > + *flags = gpiospec->args[1];
> > +
> > + return gpiospec->args[0];
> > +}
>
> This looks like a very standart xlate function for GPIO. Why do you need to
> open-code it?
>
> ...
>
> > +/**
> > + * xgpio_irq_ack - Acknowledge a child GPIO interrupt.
>
> > + * This currently does nothing, but irq_ack is unconditionally called
> > + by
> > + * handle_edge_irq and therefore must be defined.
>
> This should go after parameter description(s).
>
> > + * @irq_data: per irq and chip data passed down to chip functions */
>
> ...
>
> > /**
> > + * xgpio_irq_mask - Write the specified signal of the GPIO device.
> > + * @irq_data: per irq and chip data passed down to chip functions
>
> In all comments irq -> IRQ.
>
> > + */
> > +static void xgpio_irq_mask(struct irq_data *irq_data)
> > +{
> > + unsigned long flags;
> > + struct xgpio_instance *chip = irq_data_get_irq_chip_data(irq_data);
> > + int irq_offset = irqd_to_hwirq(irq_data);
> > + int index = xgpio_index(chip, irq_offset);
> > + int offset = xgpio_offset(chip, irq_offset);
> > +
> > + spin_lock_irqsave(>gpio_lock, flags);
> > +
>
> > + chip->irq_enable[index] &= ~BIT(offset);
>
> If you convert your data structure to use bitmaps (and respective API) like
>
> #define XILINX_NGPIOS 64
> ...
> DECLARE_BITMAP(irq_enable, XILINX_NGPIOS);
> ...
>
> it will make code better to read and understand. For example, here it
> will be just
> __clear_bit(offset, chip->irq_enable);
>
> > + dev_dbg(chip->gc.parent, "Disable %d irq, irq_enable_mask 0x%x\n",
> > + irq_offset, chip->irq_enable[index]);
>
> Under spin lock?! Hmm...
>
> > + if (!chip->irq_enable[index]) {
> > + /* Disable per channel interrupt */
> > + u32 temp = xgpio_readreg(chip->regs + XGPIO_IPIER_OFFSET);
> > +
> > + temp &= ~BIT(index);
> > + xgpio_writereg(chip->regs + XGPIO_IPIER_OFFSET, temp);
> > + }
> > + spin_unlock_irqrestore(>gpio_lock, flags);
> > +}
>
> ...
>
> > + for (index = 0; index < num_channels; index++) {
> > + if ((status & BIT(index))) {
>
> If gpio_width is the same among banks, you can use for_each_set_bit()
> here as well.
>
> ...
gpio_wdith vary depends on design. We can configure gpio pins for each bank.
>
> > + for_each_set_bit(bit, _events, 32
RE: [PATCH V2 2/3] gpio: xilinx: Add interrupt support
Hi Linus, Thanks for the review > -Original Message- > From: Linus Walleij > Sent: Friday, July 24, 2020 2:52 PM > To: kernel test robot > Cc: Srinivas Neeli ; Bartosz Golaszewski > ; Michal Simek ; > Shubhrajyoti Datta ; Srinivas Goud > ; kbuild-...@lists.01.org; open list:GPIO SUBSYSTEM > ; Linux ARM ker...@lists.infradead.org>; linux-kernel@vger.kernel.org; git > > Subject: Re: [PATCH V2 2/3] gpio: xilinx: Add interrupt support > > On Fri, Jul 24, 2020 at 6:12 AM kernel test robot wrote: > > >drivers/gpio/gpio-xilinx.c: In function 'xgpio_probe': > >drivers/gpio/gpio-xilinx.c:638:10: error: 'struct gpio_chip' has no > > member > named 'of_gpio_n_cells' > > 638 | chip->gc.of_gpio_n_cells = cells; > > | ^ > > >> drivers/gpio/gpio-xilinx.c:639:10: error: 'struct gpio_chip' has no > member named 'of_xlate' > > 639 | chip->gc.of_xlate = xgpio_xlate; > > | ^ > > This is probably because your driver needs: > > depends on OF_GPIO > > in KConfig > I will address in v3. > Yours, > Linus Walleij
Re: [PATCH V2 2/3] gpio: xilinx: Add interrupt support
On Fri, Jul 24, 2020 at 6:12 AM kernel test robot wrote: >drivers/gpio/gpio-xilinx.c: In function 'xgpio_probe': >drivers/gpio/gpio-xilinx.c:638:10: error: 'struct gpio_chip' has no member > named 'of_gpio_n_cells' > 638 | chip->gc.of_gpio_n_cells = cells; > | ^ > >> drivers/gpio/gpio-xilinx.c:639:10: error: 'struct gpio_chip' has no member > >> named 'of_xlate' > 639 | chip->gc.of_xlate = xgpio_xlate; > | ^ This is probably because your driver needs: depends on OF_GPIO in KConfig Yours, Linus Walleij
Re: [PATCH V2 2/3] gpio: xilinx: Add interrupt support
Hi Srinivas,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on gpio/for-next]
[also build test ERROR on linus/master v5.8-rc6 next-20200723]
[cannot apply to xlnx/master]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url:
https://github.com/0day-ci/linux/commits/Srinivas-Neeli/gpio-xilinx-Update-on-xilinx-gpio-driver/20200723-220826
base: https://git.kernel.org/pub/scm/linux/kernel/git/linusw/linux-gpio.git
for-next
config: x86_64-randconfig-a003-20200723 (attached as .config)
compiler: gcc-9 (Debian 9.3.0-14) 9.3.0
reproduce (this is a W=1 build):
# save the attached .config to linux build tree
make W=1 ARCH=x86_64
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot
All errors (new ones prefixed by >>):
drivers/gpio/gpio-xilinx.c: In function 'xgpio_xlate':
>> drivers/gpio/gpio-xilinx.c:325:8: error: 'struct gpio_chip' has no member
>> named 'of_gpio_n_cells'
325 | if (gc->of_gpio_n_cells < 2) {
|^~
In file included from arch/x86/include/asm/bug.h:92,
from include/linux/bug.h:5,
from include/linux/thread_info.h:12,
from arch/x86/include/asm/preempt.h:7,
from include/linux/preempt.h:78,
from include/linux/spinlock.h:51,
from include/linux/seqlock.h:36,
from include/linux/time.h:6,
from include/linux/stat.h:19,
from include/linux/module.h:13,
from drivers/gpio/gpio-xilinx.c:11:
drivers/gpio/gpio-xilinx.c:330:39: error: 'struct gpio_chip' has no member
named 'of_gpio_n_cells'
330 | if (WARN_ON(gpiospec->args_count < gc->of_gpio_n_cells))
| ^~
include/asm-generic/bug.h:118:25: note: in definition of macro 'WARN_ON'
118 | int __ret_warn_on = !!(condition);\
| ^
drivers/gpio/gpio-xilinx.c: In function 'xgpio_probe':
drivers/gpio/gpio-xilinx.c:638:10: error: 'struct gpio_chip' has no member
named 'of_gpio_n_cells'
638 | chip->gc.of_gpio_n_cells = cells;
| ^
>> drivers/gpio/gpio-xilinx.c:639:10: error: 'struct gpio_chip' has no member
>> named 'of_xlate'
639 | chip->gc.of_xlate = xgpio_xlate;
| ^
vim +325 drivers/gpio/gpio-xilinx.c
312
313 /**
314 * xgpio_xlate - Translate gpio_spec to the GPIO number and flags
315 * @gc: Pointer to gpio_chip device structure.
316 * @gpiospec: gpio specifier as found in the device tree
317 * @flags: A flags pointer based on binding
318 *
319 * Return:
320 * irq number otherwise -EINVAL
321 */
322 static int xgpio_xlate(struct gpio_chip *gc,
323 const struct of_phandle_args *gpiospec, u32
*flags)
324 {
> 325 if (gc->of_gpio_n_cells < 2) {
326 WARN_ON(1);
327 return -EINVAL;
328 }
329
330 if (WARN_ON(gpiospec->args_count < gc->of_gpio_n_cells))
331 return -EINVAL;
332
333 if (gpiospec->args[0] >= gc->ngpio)
334 return -EINVAL;
335
336 if (flags)
337 *flags = gpiospec->args[1];
338
339 return gpiospec->args[0];
340 }
341
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-...@lists.01.org
.config.gz
Description: application/gzip
Re: [PATCH V2 2/3] gpio: xilinx: Add interrupt support
On Thu, Jul 23, 2020 at 5:08 PM Srinivas Neeli
wrote:
>
> Adds interrupt support to the Xilinx GPIO driver so that rising and
> falling edge line events can be supported. Since interrupt support is
> an optional feature in the Xilinx IP, the driver continues to support
> devices which have no interrupt provided.
...
> +#include
Not sure I see a user of it.
...
> +/**
> + * xgpio_xlate - Translate gpio_spec to the GPIO number and flags
> + * @gc: Pointer to gpio_chip device structure.
> + * @gpiospec: gpio specifier as found in the device tree
> + * @flags: A flags pointer based on binding
> + *
> + * Return:
> + * irq number otherwise -EINVAL
> + */
> +static int xgpio_xlate(struct gpio_chip *gc,
> + const struct of_phandle_args *gpiospec, u32 *flags)
> +{
> + if (gc->of_gpio_n_cells < 2) {
> + WARN_ON(1);
> + return -EINVAL;
> + }
> +
> + if (WARN_ON(gpiospec->args_count < gc->of_gpio_n_cells))
> + return -EINVAL;
> +
> + if (gpiospec->args[0] >= gc->ngpio)
> + return -EINVAL;
> +
> + if (flags)
> + *flags = gpiospec->args[1];
> +
> + return gpiospec->args[0];
> +}
This looks like a very standart xlate function for GPIO. Why do you
need to open-code it?
...
> +/**
> + * xgpio_irq_ack - Acknowledge a child GPIO interrupt.
> + * This currently does nothing, but irq_ack is unconditionally called by
> + * handle_edge_irq and therefore must be defined.
This should go after parameter description(s).
> + * @irq_data: per irq and chip data passed down to chip functions
> + */
...
> /**
> + * xgpio_irq_mask - Write the specified signal of the GPIO device.
> + * @irq_data: per irq and chip data passed down to chip functions
In all comments irq -> IRQ.
> + */
> +static void xgpio_irq_mask(struct irq_data *irq_data)
> +{
> + unsigned long flags;
> + struct xgpio_instance *chip = irq_data_get_irq_chip_data(irq_data);
> + int irq_offset = irqd_to_hwirq(irq_data);
> + int index = xgpio_index(chip, irq_offset);
> + int offset = xgpio_offset(chip, irq_offset);
> +
> + spin_lock_irqsave(>gpio_lock, flags);
> +
> + chip->irq_enable[index] &= ~BIT(offset);
If you convert your data structure to use bitmaps (and respective API) like
#define XILINX_NGPIOS 64
...
DECLARE_BITMAP(irq_enable, XILINX_NGPIOS);
...
it will make code better to read and understand. For example, here it
will be just
__clear_bit(offset, chip->irq_enable);
> + dev_dbg(chip->gc.parent, "Disable %d irq, irq_enable_mask 0x%x\n",
> + irq_offset, chip->irq_enable[index]);
Under spin lock?! Hmm...
> + if (!chip->irq_enable[index]) {
> + /* Disable per channel interrupt */
> + u32 temp = xgpio_readreg(chip->regs + XGPIO_IPIER_OFFSET);
> +
> + temp &= ~BIT(index);
> + xgpio_writereg(chip->regs + XGPIO_IPIER_OFFSET, temp);
> + }
> + spin_unlock_irqrestore(>gpio_lock, flags);
> +}
...
> + for (index = 0; index < num_channels; index++) {
> + if ((status & BIT(index))) {
If gpio_width is the same among banks, you can use for_each_set_bit()
here as well.
...
> + for_each_set_bit(bit, _events, 32) {
> + generic_handle_irq(irq_find_mapping
> + (chip->gc.irq.domain, offset + bit));
Strange indentation. Maybe a temporary variable helps?
...
> + chip->irq = platform_get_irq_optional(pdev, 0);
> + if (chip->irq <= 0) {
> + dev_info(>dev, "GPIO IRQ not set\n");
Why do you need an optional variant if you print an error anyway?
> + } else {
...
> + chip->gc.irq.parents = (unsigned int *)>irq;
> + chip->gc.irq.num_parents = 1;
Current pattern is to use devm_kcalloc() for it (Linus has plans to
simplify this in the future and this will help him to find what
patterns are being used)
--
With Best Regards,
Andy Shevchenko
