| 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
| ||
|
Date: Mon, 18 Oct 2010 16:06:30 -0700
From: Andrew Morton <akpm@...ux-foundation.org>
To: Mike Frysinger <vapier@...too.org>
Cc: linux-kernel@...r.kernel.org,
device-drivers-devel@...ckfin.uclinux.org,
Michael Hennerich <michael.hennerich@...log.com>
Subject: Re: [PATCH 1/2] gpio: adp5588-gpio: support interrupt controller
On Mon, 18 Oct 2010 18:50:17 -0400
Mike Frysinger <vapier@...too.org> wrote:
> From: Michael Hennerich <michael.hennerich@...log.com>
>
> This patch implements irq_chip functionality on ADP5588/5587 GPIO
> expanders. Only level sensitive interrupts are supported.
> Interrupts provided by this irq_chip must be requested using
> request_threaded_irq().
>
>
> ...
>
> + /* Configuration Register1 */
> +#define AUTO_INC (1 << 7)
> +#define GPIEM_CFG (1 << 6)
> +#define OVR_FLOW_M (1 << 5)
> +#define INT_CFG (1 << 4)
> +#define OVR_FLOW_IEN (1 << 3)
> +#define K_LCK_IM (1 << 2)
> +#define GPI_IEN (1 << 1)
> +#define KE_IEN (1 << 0)
> +
> +/* Interrupt Status Register */
> +#define GPI_INT (1 << 1)
> +#define KE_INT (1 << 0)
All copied-n-pasted from drivers/input/keyboard/adp5588-keys.c?
Bad. Put it in a shared header file please.
It might be a good idea to rename them all as well. Things like
INT_CFG are rather generic and there is a risk of conflict against
unrelated headers which use the same symbols.
> #define DRV_NAME "adp5588-gpio"
> #define MAXGPIO 18
> #define ADP_BANK(offs) ((offs) >> 3)
> #define ADP_BIT(offs) (1u << ((offs) & 0x7))
>
> +/*
> + * Early pre 4.0 Silicon required to delay readout by at least 25ms,
> + * since the Event Counter Register updated 25ms after the interrupt
> + * asserted.
> + */
> +#define WA_DELAYED_READOUT_REVID(rev) ((rev) < 4)
> +
> struct adp5588_gpio {
> struct i2c_client *client;
> struct gpio_chip gpio_chip;
> struct mutex lock; /* protect cached dir, dat_out */
> + struct mutex irq_lock; /* P: IRQ */
One wonders what "P: IRQ" means.
It's rare for code to be damaged by excessively verbose description of
struct fields ;)
> unsigned gpio_start;
> + unsigned irq_base;
> uint8_t dat_out[3];
> uint8_t dir[3];
> + uint8_t int_lvl[3];
> + uint8_t int_en[3];
> + uint8_t irq_mask[3];
> + uint8_t irq_stat[3];
> };
>
>
> ...
>
> +static void adp5588_irq_bus_sync_unlock(unsigned int irq)
> +{
> + struct adp5588_gpio *dev = get_irq_chip_data(irq);
> + int i;
> +
> + for (i = 0; i <= ADP_BANK(MAXGPIO); i++)
> + if (dev->int_en[i] ^ dev->irq_mask[i]) {
> + dev->int_en[i] = dev->irq_mask[i];
> + adp5588_gpio_write(dev->client, GPIO_INT_EN1 + i,
> + dev->int_en[i]);
> + }
Some description of what this code is doing and why it does it would be
useful. This drive-by reader doesn't have a clue.
> + mutex_unlock(&dev->irq_lock);
> +}
> +
>
> ...
>
> +static int adp5588_irq_set_type(unsigned int irq, unsigned int type)
> +{
> + struct adp5588_gpio *dev = get_irq_chip_data(irq);
> + uint16_t gpio = irq - dev->irq_base;
> + unsigned bank, bit;
> +
> + if ((type & IRQ_TYPE_EDGE_BOTH)) {
> + dev_err(&dev->client->dev, "irq %d: unsupported type %d\n",
> + irq, type);
> + return -EINVAL;
> + }
> +
> + bank = ADP_BANK(gpio);
> + bit = ADP_BIT(gpio);
> +
> + if (type & IRQ_TYPE_LEVEL_HIGH)
> + dev->int_lvl[bank] |= bit;
> + else if (type & IRQ_TYPE_LEVEL_LOW)
> + dev->int_lvl[bank] &= ~bit;
> + else
> + return -EINVAL;
> +
> + might_sleep();
Seems a bit unnecessary - adp5588_gpio_direction_input() does a
mutex_lock() and mutex_lock() does a might_sleep().
> + adp5588_gpio_direction_input(&dev->gpio_chip, gpio);
> + adp5588_gpio_write(dev->client, GPIO_INT_LVL1 + bank,
> + dev->int_lvl[bank]);
> +
> + return 0;
> +}
> +
>
> ...
>
> +static int adp5588_irq_setup(struct adp5588_gpio *dev)
> +{
> + struct i2c_client *client = dev->client;
> + struct adp5588_gpio_platform_data *pdata = client->dev.platform_data;
> + unsigned gpio;
> + int ret;
> +
> + adp5588_gpio_write(client, CFG, AUTO_INC);
> + adp5588_gpio_write(client, INT_STAT, -1); /* status is W1C */
> + adp5588_gpio_read_intstat(client, dev->irq_stat); /* read to clear */
> +
> + dev->irq_base = pdata->irq_base;
> + mutex_init(&dev->irq_lock);
> +
> + for (gpio = 0; gpio < dev->gpio_chip.ngpio; gpio++) {
> + int irq = gpio + dev->irq_base;
> + set_irq_chip_data(irq, dev);
> + set_irq_chip_and_handler(irq, &adp5588_irq_chip,
> + handle_level_irq);
> + set_irq_nested_thread(irq, 1);
> +#ifdef CONFIG_ARM
> + set_irq_flags(irq, IRQF_VALID);
> +#else
> + set_irq_noprobe(irq);
> +#endif
Needs a code comment explaining why ARM is different. How am I
possibly to review this without mind-reading powers?
Why _is_ ARM different? Is something busted?
> + }
> +
> + ret = request_threaded_irq(client->irq,
> + NULL,
> + adp5588_irq_handler,
> + IRQF_TRIGGER_FALLING | IRQF_ONESHOT,
> + dev_name(&client->dev), dev);
> + if (ret) {
> + dev_err(&client->dev, "failed to request irq %d\n",
> + client->irq);
> + goto out;
> + }
> +
> + dev->gpio_chip.to_irq = adp5588_gpio_to_irq;
> + adp5588_gpio_write(client, CFG, AUTO_INC | INT_CFG | GPI_INT);
> +
> + return 0;
> +
> +out:
> + dev->irq_base = 0;
> + return ret;
> +}
> +static void adp5588_irq_teardown(struct adp5588_gpio *dev)
Missing a newline.
> +{
> + if (dev->irq_base)
> + free_irq(dev->client->irq, dev);
> +}
> +
> +#else
> +static int adp5588_irq_setup(struct adp5588_gpio *dev)
> +{
> + struct i2c_client *client = dev->client;
> + dev_warn(&client->dev, "interrupt support not compiled in\n");
> +
> + return 0;
> +}
> +
>
> ...
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists