Re: [PATCH v5 4/4] gpio: add support for the Diolan DLN-2 USB GPIO driver
On Fri, Sep 19, 2014 at 11:22:45PM +0300, Octavian Purdila wrote:
> +struct dln2_gpio {
> + struct platform_device *pdev;
> + struct gpio_chip gpio;
> +
> + /*
> + * Cache pin direction to save us one transfer, since the
> + * hardware has separate commands to read the in and out
> + * values. Bit set for out, bit clear for in.
> + */
> + DECLARE_BITMAP(pin_dir, DLN2_GPIO_MAX_PINS);
Using the more common name output_enabled might be preferred? Then you
can even get rid of (part of) the comment.
> +
> + 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;
> +};
[...]
> +#define DLN2_GPIO_DIRECTION_IN 0
> +#define DLN2_GPIO_DIRECTION_OUT 1
> +
> +static int dln2_gpio_request(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 len = sizeof(rsp);
> + int ret;
> +
> + ret = dln2_gpio_pin_cmd(dln2, DLN2_GPIO_PIN_ENABLE, offset);
> + if (ret < 0)
> + return ret;
> +
> + /* cache the pin direction */
> + 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;
Disable the pin?
> +
> + switch (rsp.value) {
> + case DLN2_GPIO_DIRECTION_IN:
> + clear_bit(offset, dln2->pin_dir);
> + return 0;
> + case DLN2_GPIO_DIRECTION_OUT:
> + set_bit(offset, dln2->pin_dir);
> + return 0;
> + default:
> + return -EPROTO;
Disable the pin?
> + }
> +}
[...]
> +static struct platform_driver dln2_gpio_driver = {
> + .driver.name= "dln2-gpio",
> + .driver.owner = THIS_MODULE,
No need to set the owner field when using the module_platform_driver
macro.
> + .probe = dln2_gpio_probe,
> + .remove = dln2_gpio_remove,
> +};
> +
> +module_platform_driver(dln2_gpio_driver);
> +
> +MODULE_AUTHOR("Daniel Baluta +MODULE_DESCRIPTION("Driver for the Diolan DLN2 GPIO interface");
> +MODULE_LICENSE("GPL v2");
> +MODULE_ALIAS("platform:dln2-gpio");
Johan
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v5 4/4] gpio: add support for the Diolan DLN-2 USB GPIO driver
On Sat, Sep 20, 2014 at 5:48 AM, Arnd Bergmann wrote:
> On Friday 19 September 2014, Octavian Purdila wrote:
>> +struct dln2_gpio_pin {
>> + __le16 pin;
>> +} __packed;
>
> This does not need to be marked packed, since it is never embedded in another
> structure.
>
Will do.
>> +struct dln2_gpio_pin_val {
>> + __le16 pin;
>> + u8 value;
>> +} __packed;
>
> It's enough here to mark just the 'pin' member as packed.
>
OK.
>> +static int dln2_gpio_get_pin_count(struct platform_device *pdev)
>> +{
>> + int ret;
>> + __le16 count;
>> + int len = sizeof(count);
>> +
>> + ret = dln2_transfer(pdev, DLN2_GPIO_GET_PIN_COUNT, NULL, 0, &count,
>> + &len);
>
> You must not do a USB transaction on stack memory.
>
dln2_transfer allocate a new buffer (in dln2_prep_buf(), with
kmalloc()) and does the USB transfer with that buffer.
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v5 4/4] gpio: add support for the Diolan DLN-2 USB GPIO driver
On Friday 19 September 2014, Octavian Purdila wrote:
> +struct dln2_gpio_pin {
> + __le16 pin;
> +} __packed;
This does not need to be marked packed, since it is never embedded in another
structure.
> +struct dln2_gpio_pin_val {
> + __le16 pin;
> + u8 value;
> +} __packed;
It's enough here to mark just the 'pin' member as packed.
> +static int dln2_gpio_get_pin_count(struct platform_device *pdev)
> +{
> + int ret;
> + __le16 count;
> + int len = sizeof(count);
> +
> + ret = dln2_transfer(pdev, DLN2_GPIO_GET_PIN_COUNT, NULL, 0, &count,
> + &len);
You must not do a USB transaction on stack memory.
> +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);
> +}
Same here
> +static int dln2_gpio_pin_val(struct dln2_gpio *dln2, int cmd, unsigned int
> pin)
> +{
> + int ret;
> + struct dln2_gpio_pin req = {
> + .pin = cpu_to_le16(pin),
> + };
> + struct dln2_gpio_pin_val rsp;
And here.
> +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);
> +}
Here you also have a strange __packed attribute that makes no sense
for a local variable, in addition to the stack problem.
I think the only correct way to handle these is to add a dynamic
allocation of an entire page for the DMA, which can probably be
part of the dln2_transfer function so you don't have to do it
in each caller.
Arnd
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v5 4/4] gpio: add support for the Diolan DLN-2 USB GPIO driver
From: Daniel Baluta
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
Signed-off-by: Octavian Purdila
---
drivers/gpio/Kconfig | 12 +
drivers/gpio/Makefile| 1 +
drivers/gpio/gpio-dln2.c | 554 +++
3 files changed, 567 insertions(+)
create mode 100644 drivers/gpio/gpio-dln2.c
diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
index 9de1515..44ec206 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 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 000..95d8ca3
--- /dev/null
+++ b/drivers/gpio/gpio-dln2.c
@@ -0,0 +1,554 @@
+/*
+ * 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
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+
+#define DLN2_GPIO_ID 0x01
+
+#define DLN2_GPIO_GET_PORT_COUNT DLN2_CMD(0x00, DLN2_GPIO_ID)
+#define DLN2_GPIO_GET_PIN_COUNTDLN2_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_DIRECTIONDLN2_CMD(0x13, DLN2_GPIO_ID)
+#define DLN2_GPIO_PIN_GET_DIRECTIONDLN2_CMD(0x14, DLN2_GPIO_ID)
+#define DLN2_GPIO_PIN_SET_EVENT_CFGDLN2_CMD(0x1E, DLN2_GPIO_ID)
+#define DLN2_GPIO_PIN_GET_EVENT_CFGDLN2_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_LOW3
+#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;
+ int type;
+};
+
+struct dln2_gpio {
+ struct platform_device *pdev;
+ struct gpio_chip gpio;
+
+ /*
+* Cache pin direction to save us one transfer, since the
+* hardware has separate commands to read the in and out
+* values. Bit set for out, bit clear for in.
+*/
+ DECLARE_BITMAP(pin_dir, DLN2_GPIO_MAX_PINS);
+
+ 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;
+};
+
+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)
+{
+ int ret;
+ __le16 count;
+ int len = sizeof(count);
+
+ ret = dln2_transfer(pdev, DLN2_GPIO_GET_PIN_COUNT, NULL, 0, &count,
+ &len);
+ if (ret < 0)
+ return ret;
+ if (len <
