Message-ID: <20140908162206.GE9560@localhost>
Date:	Mon, 8 Sep 2014 18:22:06 +0200
From:	Johan Hovold <johan@...nel.org>
To:	Octavian Purdila <octavian.purdila@...el.com>
Cc:	gregkh@...uxfoundation.org, linus.walleij@...aro.org,
	gnurou@...il.com, wsa@...-dreams.de, sameo@...ux.intel.com,
	lee.jones@...aro.org, arnd@...db.de, johan@...nel.org,
	daniel.baluta@...el.com, laurentiu.palcu@...el.com,
	linux-usb@...r.kernel.org, linux-kernel@...r.kernel.org,
	linux-gpio@...r.kernel.org, linux-i2c@...r.kernel.org
Subject: Re: [PATCH v3 3/3] gpio: add support for the Diolan DLN-2 USB GPIO
 driver

On Fri, Sep 05, 2014 at 06:17:59PM +0300, Octavian Purdila wrote:
> From: Daniel Baluta <daniel.baluta@...el.com>
> 
> This patch adds GPIO and IRQ support for the Diolan DLN-2 GPIO module.
> 
> Information about the USB protocol interface can be found in the
> Programmer's Reference Manual [1], see section 2.9 for the GPIO
> module commands and responses.
> 
> [1] https://www.diolan.com/downloads/dln-api-manual.pdf
> 
> Signed-off-by: Daniel Baluta <daniel.baluta@...el.com>
> Signed-off-by: Octavian Purdila <octavian.purdila@...el.com>
> ---
>  drivers/gpio/Kconfig     |  12 ++
>  drivers/gpio/Makefile    |   1 +
>  drivers/gpio/gpio-dln2.c | 537 +++++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 550 insertions(+)
>  create mode 100644 drivers/gpio/gpio-dln2.c
> 
> diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
> index 9de1515..6a9e352 100644
> --- a/drivers/gpio/Kconfig
> +++ b/drivers/gpio/Kconfig
> @@ -897,4 +897,16 @@ config GPIO_VIPERBOARD
>            River Tech's viperboard.h for detailed meaning
>            of the module parameters.
>  
> +config GPIO_DLN2
> +	tristate "Diolan DLN2 GPIO support"
> +	depends on USB && MFD_DLN2

Just MFD_DLN2.

> +	select GPIOLIB_IRQCHIP
> +
> +	help
> +	  Select this option to enable GPIO driver for the Diolan DLN2
> +	  board.
> +
> +	  This driver can also be built as a module. If so, the module
> +	  will be called gpio-dln2.
> +
>  endif
> diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile
> index 5d024e3..eaa97a0 100644
> --- a/drivers/gpio/Makefile
> +++ b/drivers/gpio/Makefile
> @@ -26,6 +26,7 @@ obj-$(CONFIG_GPIO_CRYSTAL_COVE)	+= gpio-crystalcove.o
>  obj-$(CONFIG_GPIO_DA9052)	+= gpio-da9052.o
>  obj-$(CONFIG_GPIO_DA9055)	+= gpio-da9055.o
>  obj-$(CONFIG_GPIO_DAVINCI)	+= gpio-davinci.o
> +obj-$(CONFIG_GPIO_DLN2)		+= gpio-dln2.o
>  obj-$(CONFIG_GPIO_DWAPB)	+= gpio-dwapb.o
>  obj-$(CONFIG_GPIO_EM)		+= gpio-em.o
>  obj-$(CONFIG_GPIO_EP93XX)	+= gpio-ep93xx.o
> diff --git a/drivers/gpio/gpio-dln2.c b/drivers/gpio/gpio-dln2.c
> new file mode 100644
> index 0000000..f8c0bcb
> --- /dev/null
> +++ b/drivers/gpio/gpio-dln2.c
> @@ -0,0 +1,537 @@
> +/*
> + * Driver for the Diolan DLN-2 USB-GPIO adapter
> + *
> + * Copyright (c) 2014 Intel Corporation
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License as
> + * published by the Free Software Foundation, version 2.
> + */
> +
> +#include <linux/kernel.h>
> +#include <linux/errno.h>
> +#include <linux/module.h>
> +#include <linux/slab.h>
> +#include <linux/types.h>
> +#include <linux/mutex.h>
> +#include <linux/irqdomain.h>
> +#include <linux/irq.h>
> +#include <linux/irqchip/chained_irq.h>
> +#include <linux/usb.h>
> +#include <linux/gpio.h>
> +#include <linux/gpio/driver.h>
> +#include <linux/ptrace.h>
> +#include <linux/wait.h>
> +#include <linux/platform_device.h>
> +#include <linux/mfd/dln2.h>
> +
> +#define DRIVER_NAME "dln2-gpio"
> +
> +#define DLN2_GPIO_ID			0x01
> +
> +#define DLN2_GPIO_GET_PORT_COUNT	DLN2_CMD(0x00, DLN2_GPIO_ID)
> +#define DLN2_GPIO_GET_PIN_COUNT		DLN2_CMD(0x01, DLN2_GPIO_ID)
> +#define DLN2_GPIO_SET_DEBOUNCE		DLN2_CMD(0x04, DLN2_GPIO_ID)
> +#define DLN2_GPIO_GET_DEBOUNCE		DLN2_CMD(0x05, DLN2_GPIO_ID)
> +#define DLN2_GPIO_PORT_GET_VAL		DLN2_CMD(0x06, DLN2_GPIO_ID)
> +#define DLN2_GPIO_PIN_GET_VAL		DLN2_CMD(0x0B, DLN2_GPIO_ID)
> +#define DLN2_GPIO_PIN_SET_OUT_VAL	DLN2_CMD(0x0C, DLN2_GPIO_ID)
> +#define DLN2_GPIO_PIN_GET_OUT_VAL	DLN2_CMD(0x0D, DLN2_GPIO_ID)
> +#define DLN2_GPIO_CONDITION_MET_EV	DLN2_CMD(0x0F, DLN2_GPIO_ID)
> +#define DLN2_GPIO_PIN_ENABLE		DLN2_CMD(0x10, DLN2_GPIO_ID)
> +#define DLN2_GPIO_PIN_DISABLE		DLN2_CMD(0x11, DLN2_GPIO_ID)
> +#define DLN2_GPIO_PIN_SET_DIRECTION	DLN2_CMD(0x13, DLN2_GPIO_ID)
> +#define DLN2_GPIO_PIN_GET_DIRECTION	DLN2_CMD(0x14, DLN2_GPIO_ID)
> +#define DLN2_GPIO_PIN_SET_EVENT_CFG	DLN2_CMD(0x1E, DLN2_GPIO_ID)
> +#define DLN2_GPIO_PIN_GET_EVENT_CFG	DLN2_CMD(0x1F, DLN2_GPIO_ID)
> +
> +#define DLN2_GPIO_EVENT_NONE		0
> +#define DLN2_GPIO_EVENT_CHANGE		1
> +#define DLN2_GPIO_EVENT_LVL_HIGH	2
> +#define DLN2_GPIO_EVENT_LVL_LOW		3
> +#define DLN2_GPIO_EVENT_CHANGE_RISING	0x11
> +#define DLN2_GPIO_EVENT_CHANGE_FALLING  0x21
> +#define DLN2_GPIO_EVENT_MASK		0x0F
> +
> +#define DLN2_GPIO_MAX_PINS 32
> +
> +struct dln2_irq_work {
> +	struct work_struct work;
> +	struct dln2_gpio *dln2;
> +	int pin, type;

One declaration per line.

Please consider my previous style comments also for this patch.

> +};
> +
> +struct dln2_remove_work {
> +	struct delayed_work work;
> +	struct dln2_gpio *dln2;
> +};
> +
> +struct dln2_gpio {
> +	struct platform_device *pdev;
> +	struct gpio_chip gpio;
> +
> +	DECLARE_BITMAP(irqs_masked, DLN2_GPIO_MAX_PINS);
> +	DECLARE_BITMAP(irqs_enabled, DLN2_GPIO_MAX_PINS);
> +	DECLARE_BITMAP(irqs_pending, DLN2_GPIO_MAX_PINS);
> +	struct dln2_irq_work irq_work[DLN2_GPIO_MAX_PINS];
> +	struct dln2_remove_work remove_work;
> +};
> +
> +struct dln2_gpio_pin {
> +	__le16 pin;
> +} __packed;
> +
> +struct dln2_gpio_pin_val {
> +	__le16 pin;
> +	u8 value;
> +} __packed;
> +
> +static int dln2_gpio_get_pin_count(struct platform_device *pdev)
> +{
> +	__le16 count;
> +	int ret, len = sizeof(count);

Dito.

> +
> +	ret = dln2_transfer(pdev, DLN2_GPIO_GET_PIN_COUNT, NULL, 0, &count,
> +			    &len);
> +	if (ret < 0)
> +		return ret;
> +
> +	if (len < sizeof(count))
> +		return -EPROTO;
> +
> +	return le16_to_cpu(count);
> +}
> +
> +static int dln2_gpio_pin_cmd(struct dln2_gpio *dln2, int cmd, unsigned pin)
> +{
> +	struct dln2_gpio_pin req = {
> +		.pin = cpu_to_le16(pin),
> +	};
> +
> +	return dln2_transfer(dln2->pdev, cmd, &req, sizeof(req), NULL, NULL);
> +}
> +
> +static int dln2_gpio_pin_val(struct dln2_gpio *dln2, int cmd, unsigned int pin)
> +{
> +	struct dln2_gpio_pin req = {
> +		.pin = cpu_to_le16(pin),
> +	};
> +	struct dln2_gpio_pin_val rsp;
> +	int ret, len = sizeof(rsp);
> +
> +	ret = dln2_transfer(dln2->pdev, cmd, &req, sizeof(req), &rsp, &len);
> +	if (ret < 0)
> +		return ret;
> +
> +	if (len < sizeof(rsp) || req.pin != rsp.pin)
> +		return -EPROTO;
> +
> +	return rsp.value;
> +}
> +
> +static int dln2_gpio_pin_get_in_val(struct dln2_gpio *dln2, unsigned int pin)
> +{
> +	int ret = dln2_gpio_pin_val(dln2, DLN2_GPIO_PIN_GET_VAL, pin);

Dito.

> +
> +	if (ret < 0)
> +		return ret;
> +	return !!ret;
> +}
> +
> +static int dln2_gpio_pin_get_out_val(struct dln2_gpio *dln2, unsigned int pin)
> +{
> +	int ret = dln2_gpio_pin_val(dln2, DLN2_GPIO_PIN_GET_OUT_VAL, pin);
> +
> +	if (ret < 0)
> +		return ret;
> +	return !!ret;
> +}
> +
> +static void dln2_gpio_pin_set_out_val(struct dln2_gpio *dln2,
> +				      unsigned int pin, int value)
> +{
> +	struct dln2_gpio_pin_val req = {
> +		.pin = cpu_to_le16(pin),
> +		.value = cpu_to_le16(value),
> +	};
> +
> +	dln2_transfer(dln2->pdev, DLN2_GPIO_PIN_SET_OUT_VAL, &req, sizeof(req),
> +		      NULL, NULL);
> +}
> +
> +static int dln2_gpio_request(struct gpio_chip *chip, unsigned offset)
> +{
> +	struct dln2_gpio *dln2 = container_of(chip, struct dln2_gpio, gpio);
> +
> +	return dln2_gpio_pin_cmd(dln2, DLN2_GPIO_PIN_ENABLE, offset);
> +}
> +
> +static void dln2_gpio_free(struct gpio_chip *chip, unsigned offset)
> +{
> +	struct dln2_gpio *dln2 = container_of(chip, struct dln2_gpio, gpio);
> +
> +	dln2_gpio_pin_cmd(dln2, DLN2_GPIO_PIN_DISABLE, offset);
> +}
> +
> +#define DLN2_GPIO_DIRECTION_IN		0
> +#define DLN2_GPIO_DIRECTION_OUT		1
> +
> +static int dln2_gpio_get_direction(struct gpio_chip *chip, unsigned offset)
> +{
> +	struct dln2_gpio *dln2 = container_of(chip, struct dln2_gpio, gpio);
> +	struct dln2_gpio_pin req = {
> +		.pin = cpu_to_le16(offset),
> +	};
> +	struct dln2_gpio_pin_val rsp;
> +	int ret, len = sizeof(rsp);
> +
> +	ret = dln2_transfer(dln2->pdev, DLN2_GPIO_PIN_GET_DIRECTION,
> +			    &req, sizeof(req), &rsp, &len);
> +	if (ret < 0)
> +		return ret;
> +
> +	if (len < sizeof(rsp) || req.pin != rsp.pin)
> +		return -EPROTO;
> +
> +	switch (rsp.value) {
> +	case DLN2_GPIO_DIRECTION_IN:
> +		return 1;
> +	case DLN2_GPIO_DIRECTION_OUT:
> +		return 0;
> +	default:
> +		return -EPROTO;
> +	}
> +}
> +
> +static int dln2_gpio_get(struct gpio_chip *chip, unsigned int offset)
> +{
> +	struct dln2_gpio *dln2 = container_of(chip, struct dln2_gpio, gpio);
> +	int dir = dln2_gpio_get_direction(chip, offset);

Do you really want the overhead of this call for every get?

> +
> +	if (dir < 0)
> +		return dir;
> +
> +	if (dir)
> +		return dln2_gpio_pin_get_in_val(dln2, offset);
> +
> +	return dln2_gpio_pin_get_out_val(dln2, offset);
> +}
> +
> +static void dln2_gpio_set(struct gpio_chip *chip, unsigned offset, int value)
> +{
> +	struct dln2_gpio *dln2 = container_of(chip, struct dln2_gpio, gpio);
> +
> +	dln2_gpio_pin_set_out_val(dln2, offset, value);
> +}
> +
> +static int dln2_gpio_set_direction(struct gpio_chip *chip, unsigned offset,
> +				   unsigned dir)
> +{
> +	struct dln2_gpio *dln2 = container_of(chip, struct dln2_gpio, gpio);
> +	struct dln2_gpio_pin_val req = {
> +		.pin = cpu_to_le16(offset),
> +		.value = cpu_to_le16(dir),
> +	};
> +
> +	return dln2_transfer(dln2->pdev, DLN2_GPIO_PIN_SET_DIRECTION,
> +			     &req, sizeof(req), NULL, NULL);
> +}
> +
> +static int dln2_gpio_direction_input(struct gpio_chip *chip, unsigned offset)
> +{
> +	return dln2_gpio_set_direction(chip, offset, DLN2_GPIO_DIRECTION_IN);
> +}
> +
> +static int dln2_gpio_direction_output(struct gpio_chip *chip, unsigned offset,
> +				      int value)
> +{
> +	return dln2_gpio_set_direction(chip, offset, DLN2_GPIO_DIRECTION_OUT);
> +}
> +
> +static int dln2_gpio_set_debounce(struct gpio_chip *chip, unsigned offset,
> +				  unsigned debounce)
> +{
> +	struct dln2_gpio *dln2 = container_of(chip, struct dln2_gpio, gpio);
> +	struct {
> +		__le32 duration;
> +	} __packed req = {
> +		.duration = cpu_to_le32(debounce),
> +	};
> +
> +	return dln2_transfer(dln2->pdev, DLN2_GPIO_SET_DEBOUNCE,
> +			     &req, sizeof(req), NULL, NULL);
> +}
> +
> +static int dln2_gpio_set_event_cfg(struct dln2_gpio *dln2, unsigned pin,
> +				   unsigned type, unsigned period)
> +{
> +	struct {
> +		__le16 pin;
> +		u8 type;
> +		__le16 period;
> +	} __packed req = {
> +		.pin = cpu_to_le16(pin),
> +		.type = type,
> +		.period = cpu_to_le16(period),
> +	};
> +
> +	return dln2_transfer(dln2->pdev, DLN2_GPIO_PIN_SET_EVENT_CFG,
> +			     &req, sizeof(req), NULL, NULL);
> +}
> +
> +static void dln2_irq_work(struct work_struct *w)
> +{
> +	struct dln2_irq_work *iw = container_of(w, struct dln2_irq_work, work);
> +	struct dln2_gpio *dln2 = iw->dln2;
> +	u8 type = iw->type & DLN2_GPIO_EVENT_MASK;
> +
> +	if (test_bit(iw->pin, dln2->irqs_enabled))
> +		dln2_gpio_set_event_cfg(dln2, iw->pin, type, 0);
> +	else
> +		dln2_gpio_set_event_cfg(dln2, iw->pin, DLN2_GPIO_EVENT_NONE, 0);
> +}
> +
> +static void dln2_irq_enable(struct irq_data *irqd)
> +{
> +	struct gpio_chip *gc = irq_data_get_irq_chip_data(irqd);
> +	struct dln2_gpio *dln2 = container_of(gc, struct dln2_gpio, gpio);
> +	int pin = irqd_to_hwirq(irqd);
> +
> +	set_bit(pin, dln2->irqs_enabled);
> +	schedule_work(&dln2->irq_work[pin].work);
> +}
> +
> +static void dln2_irq_disable(struct irq_data *irqd)
> +{
> +	struct gpio_chip *gc = irq_data_get_irq_chip_data(irqd);
> +	struct dln2_gpio *dln2 = container_of(gc, struct dln2_gpio, gpio);
> +	int pin = irqd_to_hwirq(irqd);
> +
> +	clear_bit(pin, dln2->irqs_enabled);
> +	schedule_work(&dln2->irq_work[pin].work);
> +}
> +
> +static void dln2_irq_mask(struct irq_data *irqd)
> +{
> +	struct gpio_chip *gc = irq_data_get_irq_chip_data(irqd);
> +	struct dln2_gpio *dln2 = container_of(gc, struct dln2_gpio, gpio);
> +	int pin = irqd_to_hwirq(irqd);
> +
> +	set_bit(pin, dln2->irqs_masked);
> +}
> +
> +static void dln2_irq_unmask(struct irq_data *irqd)
> +{
> +	struct gpio_chip *gc = irq_data_get_irq_chip_data(irqd);
> +	struct dln2_gpio *dln2 = container_of(gc, struct dln2_gpio, gpio);
> +	int pin = irqd_to_hwirq(irqd);
> +
> +	if (test_and_clear_bit(pin, dln2->irqs_pending))
> +		generic_handle_irq(pin);
> +}
> +
> +static int dln2_irq_set_type(struct irq_data *irqd, unsigned type)
> +{
> +	struct gpio_chip *gc = irq_data_get_irq_chip_data(irqd);
> +	struct dln2_gpio *dln2 = container_of(gc, struct dln2_gpio, gpio);
> +	int pin = irqd_to_hwirq(irqd);
> +
> +	switch (type) {
> +	case IRQ_TYPE_LEVEL_HIGH:
> +		dln2->irq_work[pin].type = DLN2_GPIO_EVENT_LVL_HIGH;
> +		break;
> +	case IRQ_TYPE_LEVEL_LOW:
> +		dln2->irq_work[pin].type = DLN2_GPIO_EVENT_LVL_LOW;
> +		break;
> +	case IRQ_TYPE_EDGE_BOTH:
> +		dln2->irq_work[pin].type = DLN2_GPIO_EVENT_CHANGE;
> +		break;
> +	case IRQ_TYPE_EDGE_RISING:
> +		dln2->irq_work[pin].type = DLN2_GPIO_EVENT_CHANGE_RISING;
> +		break;
> +	case IRQ_TYPE_EDGE_FALLING:
> +		dln2->irq_work[pin].type = DLN2_GPIO_EVENT_CHANGE_FALLING;
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	return 0;
> +}
> +
> +static struct irq_chip dln2_gpio_irqchip = {
> +	.name = "dln2-irq",
> +	.irq_enable = dln2_irq_enable,
> +	.irq_disable = dln2_irq_disable,
> +	.irq_mask = dln2_irq_mask,
> +	.irq_unmask = dln2_irq_unmask,
> +	.irq_set_type = dln2_irq_set_type,
> +};
> +
> +static void dln2_gpio_event(struct platform_device *pdev, u16 echo,
> +			    const void *data, int len)
> +{
> +	int pin, irq;
> +	const struct {
> +		__le16 count;
> +		__u8 type;
> +		__le16 pin;
> +		__u8 value;
> +	} __packed *event = data;

You never verify the length of the received data before parsing it.

> +	struct dln2_gpio *dln2 = platform_get_drvdata(pdev);
> +
> +	pin = le16_to_cpu(event->pin);
> +	irq = irq_find_mapping(dln2->gpio.irqdomain, pin);
> +
> +	if (!irq) {
> +		dev_err(dln2->gpio.dev, "pin %d not mapped to IRQ\n", pin);
> +		return;
> +	}
> +
> +	if (!test_bit(pin, dln2->irqs_enabled))
> +		return;
> +	if (test_bit(pin, dln2->irqs_masked)) {
> +		set_bit(pin, dln2->irqs_pending);
> +		return;
> +	}
> +
> +	switch (dln2->irq_work[pin].type) {
> +	case DLN2_GPIO_EVENT_CHANGE_RISING:
> +		if (event->value)
> +			generic_handle_irq(irq);
> +		break;
> +	case DLN2_GPIO_EVENT_CHANGE_FALLING:
> +		if (!event->value)
> +			generic_handle_irq(irq);
> +		break;
> +	default:
> +		generic_handle_irq(irq);
> +	}
> +}
> +
> +static int dln2_do_remove(struct dln2_gpio *dln2)
> +{
> +	/* When removing the DLN2 USB device, gpiochip_remove may fail
> +	 * due to i2c drivers holding a GPIO pin. However, the i2c bus
> +	 * will eventually be removed triggering an i2c driver remove
> +	 * which will release the GPIO pin. So retrying the operation
> +	 * later should succeed. */
> +	int ret = gpiochip_remove(&dln2->gpio);
> +	struct device *dev = dln2->gpio.dev;
> +
> +	if (ret < 0) {
> +		if (ret == -EBUSY)
> +			schedule_delayed_work(&dln2->remove_work.work, HZ/10);
> +		else
> +			dev_warn(dev, "error removing gpio chip: %d\n", ret);
> +	} else {
> +		kfree(dln2);
> +	}
> +
> +	return ret;
> +}

Wow. You really need to get rid of this hack.

As I already mentioned when you first posted this, the return value of
gpiochip_remove is going away. Furthermore, this is not a problem
specific to this driver, so if anything this would belong in gpiolib.

And finally, as I also mentioned, a gpio may stay requested
indefinitely so you could be looping here forever.

> +
> +static void dln2_remove_work(struct work_struct *w)
> +{
> +	struct delayed_work *dw = to_delayed_work(w);
> +	struct dln2_remove_work *rw = container_of(dw, struct dln2_remove_work,
> +						   work);
> +	struct dln2_gpio *dln2 = rw->dln2;
> +
> +	dln2_do_remove(dln2);
> +}
> +
> +static int dln2_gpio_probe(struct platform_device *pdev)
> +{
> +	struct dln2_gpio *dln2;
> +	struct device *dev = &pdev->dev;
> +	int pins = dln2_gpio_get_pin_count(pdev), i, ret;
> +
> +	if (pins < 0) {
> +		dev_err(dev, "failed to get pin count: %d\n", pins);
> +		return pins;
> +	}
> +	if (pins > DLN2_GPIO_MAX_PINS)
> +		dev_warn(dev, "clamping pins to %d\n", DLN2_GPIO_MAX_PINS);

You never seem to actually clamp the number of pins, as you set set
ngpio bases on pins below. This is bound to lead to crashes eventually.

> +
> +	dln2 = kzalloc(sizeof(*dln2), GFP_KERNEL);
> +	if (!dln2)
> +		return -ENOMEM;
> +
> +	for (i = 0; i < DLN2_GPIO_MAX_PINS; i++) {
> +		INIT_WORK(&dln2->irq_work[i].work, dln2_irq_work);
> +		dln2->irq_work[i].pin = i;
> +		dln2->irq_work[i].dln2 = dln2;
> +	}
> +	INIT_DELAYED_WORK(&dln2->remove_work.work, dln2_remove_work);
> +	dln2->remove_work.dln2 = dln2;
> +
> +	ret = dln2_register_event_cb(pdev, DLN2_GPIO_CONDITION_MET_EV,
> +				     dln2_gpio_event);
> +	if (ret) {
> +		kfree(dln2);
> +		return ret;
> +	}
> +
> +	dln2->pdev = pdev;
> +
> +	dln2->gpio.label = "dln2";
> +	dln2->gpio.dev = dev;
> +	dln2->gpio.owner = THIS_MODULE;
> +	dln2->gpio.base = -1;
> +	dln2->gpio.ngpio = pins;
> +	dln2->gpio.exported = 1;
> +	dln2->gpio.set = dln2_gpio_set;
> +	dln2->gpio.get = dln2_gpio_get;
> +	dln2->gpio.request = dln2_gpio_request;
> +	dln2->gpio.free = dln2_gpio_free;
> +	dln2->gpio.get_direction = dln2_gpio_get_direction;
> +	dln2->gpio.direction_input = dln2_gpio_direction_input;
> +	dln2->gpio.direction_output = dln2_gpio_direction_output;
> +	dln2->gpio.set_debounce = dln2_gpio_set_debounce;
> +
> +	ret = gpiochip_add(&dln2->gpio);
> +	if (ret < 0) {
> +		dev_err(dev, "failed to add gpio chip: %d\n", ret);
> +		dln2_unregister_event_cb(pdev, DLN2_GPIO_CONDITION_MET_EV);
> +		kfree(dln2);

You should probably add a common error path for this.

> +		return ret;
> +	}
> +
> +	ret = gpiochip_irqchip_add(&dln2->gpio, &dln2_gpio_irqchip, 0,
> +				   handle_simple_irq, IRQ_TYPE_NONE);
> +	if (ret < 0) {
> +		dev_err(dev, "failed to add irq chip: %d\n", ret);
> +		dln2_unregister_event_cb(pdev, DLN2_GPIO_CONDITION_MET_EV);
> +		gpiochip_remove(&dln2->gpio);
> +		kfree(dln2);
> +		return ret;
> +	}

You dereference the platform data in dln2_gpio_event, which could
possibly be NULL if you get a callback here.

> +
> +	platform_set_drvdata(pdev, dln2);
> +
> +	return 0;
> +}
> +
> +static int dln2_gpio_remove(struct platform_device *pdev)
> +{
> +	struct dln2_gpio *dln2 = platform_get_drvdata(pdev);
> +
> +	dln2_unregister_event_cb(pdev, DLN2_GPIO_CONDITION_MET_EV);
> +	dln2->pdev = NULL;
> +
> +	return dln2_do_remove(dln2);
> +}
> +
> +static struct platform_driver dln2_gpio_driver = {
> +	.driver.name	= DRIVER_NAME,
> +	.driver.owner	= THIS_MODULE,
> +	.probe		= dln2_gpio_probe,
> +	.remove		= dln2_gpio_remove,
> +};
> +
> +module_platform_driver(dln2_gpio_driver);
> +
> +MODULE_AUTHOR("Daniel Baluta <daniel.baluta@...el.com");
> +MODULE_DESCRIPTION(DRIVER_NAME "driver");
> +MODULE_LICENSE("GPL");

Johan
--
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