| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <DM6PR02MB53867D532A3298BE06E32B86AF770@DM6PR02MB5386.namprd02.prod.outlook.com>
Date: Fri, 24 Jul 2020 17:15:13 +0000
From: Srinivas Neeli <sneeli@...inx.com>
To: Andy Shevchenko <andy.shevchenko@...il.com>
CC: Linus Walleij <linus.walleij@...aro.org>,
Bartosz Golaszewski <bgolaszewski@...libre.com>,
Michal Simek <michals@...inx.com>,
Shubhrajyoti Datta <shubhraj@...inx.com>,
Srinivas Goud <sgoud@...inx.com>,
"open list:GPIO SUBSYSTEM" <linux-gpio@...r.kernel.org>,
linux-arm Mailing List <linux-arm-kernel@...ts.infradead.org>,
Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
git <git@...inx.com>, Robert Hancock <hancock@...systems.ca>
Subject: 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 <andy.shevchenko@...il.com>
> Sent: Thursday, July 23, 2020 11:33 PM
> To: Srinivas Neeli <sneeli@...inx.com>
> Cc: Linus Walleij <linus.walleij@...aro.org>; Bartosz Golaszewski
> <bgolaszewski@...libre.com>; Michal Simek <michals@...inx.com>;
> Shubhrajyoti Datta <shubhraj@...inx.com>; Srinivas Goud
> <sgoud@...inx.com>; open list:GPIO SUBSYSTEM <linux-
> gpio@...r.kernel.org>; linux-arm Mailing List <linux-arm-
> kernel@...ts.infradead.org>; Linux Kernel Mailing List <linux-
> kernel@...r.kernel.org>; git <git@...inx.com>
> Subject: Re: [PATCH V2 2/3] gpio: xilinx: Add interrupt support
>
> On Thu, Jul 23, 2020 at 5:08 PM Srinivas Neeli <srinivas.neeli@...inx.com>
> 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 <linux/irqchip/chained_irq.h>
>
> 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(&chip->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(&chip->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, &all_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(&pdev->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.
>
> > + } else {
>
>
> ...
>
> > + chip->gc.irq.parents = (unsigned int *)&chip->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.
>
> --
> With Best Regards,
> Andy Shevchenko
Adding Robert Hancock to mail chain ( by mistake suppressed cc list) .
Hi Robert,
Could you please provide your comments.
Thanks
Srinivas Neeli
Powered by blists - more mailing lists