[PATCH v3 3/3] gpio: add support for Cypress CYUSBS234 USB-GPIO adapter

[PATCH v3 3/3] gpio: add support for Cypress CYUSBS234 USB-GPIO adapter

[Muthu Mani]

Adds support for USB-GPIO interface of Cypress Semiconductor
CYUSBS234 USB-Serial Bridge controller.

The GPIO get/set can be done through vendor command on control endpoint
for the configured gpios.

Details about the device can be found at:
http://www.cypress.com/?rID=84126

Signed-off-by: Muthu Mani <muth-+wT8y+m8/X5BDgjK7y7TUQ@public.gmane.org>
Signed-off-by: Rajaram Regupathy <rera-+wT8y+m8/X5BDgjK7y7TUQ@public.gmane.org>
---
Changes since v2:
* added helper macros
* removed lock
* given gpio chip device for dev_xxx
* cleaned up the code

Changes since v1:
* allocated memory on heap for usb transfer data
* changed gpio label as platform device name to identify multiple devices

 drivers/gpio/Kconfig          |  13 ++++
 drivers/gpio/Makefile         |   1 +
 drivers/gpio/gpio-cyusbs23x.c | 174 ++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 188 insertions(+)
 create mode 100644 drivers/gpio/gpio-cyusbs23x.c

diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
index 9de1515..932e07c 100644
--- a/drivers/gpio/Kconfig
+++ b/drivers/gpio/Kconfig
@@ -886,6 +886,19 @@ config GPIO_BCM_KONA
 
 comment "USB GPIO expanders:"
 
+config GPIO_CYUSBS23X
+	tristate "CYUSBS23x GPIO support"
+	depends on MFD_CYUSBS23X && USB
+	help
+	  Say yes here to access the GPIO signals of Cypress
+	  Semiconductor CYUSBS23x USB Serial Bridge Controller.
+
+	  This driver enables the GPIO interface of CYUSBS23x USB Serial
+	  Bridge controller.
+
+	  This driver can also be built as a module. If so, the module will be
+	  called gpio-cyusbs23x.
+
 config GPIO_VIPERBOARD
 	tristate "Viperboard GPIO a & b support"
 	depends on MFD_VIPERBOARD && USB
diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile
index 5d024e3..3ad89f1 100644
--- a/drivers/gpio/Makefile
+++ b/drivers/gpio/Makefile
@@ -23,6 +23,7 @@ obj-$(CONFIG_GPIO_BT8XX)	+= gpio-bt8xx.o
 obj-$(CONFIG_GPIO_CLPS711X)	+= gpio-clps711x.o
 obj-$(CONFIG_GPIO_CS5535)	+= gpio-cs5535.o
 obj-$(CONFIG_GPIO_CRYSTAL_COVE)	+= gpio-crystalcove.o
+obj-$(CONFIG_GPIO_CYUSBS23X)	+= gpio-cyusbs23x.o
 obj-$(CONFIG_GPIO_DA9052)	+= gpio-da9052.o
 obj-$(CONFIG_GPIO_DA9055)	+= gpio-da9055.o
 obj-$(CONFIG_GPIO_DAVINCI)	+= gpio-davinci.o
diff --git a/drivers/gpio/gpio-cyusbs23x.c b/drivers/gpio/gpio-cyusbs23x.c
new file mode 100644
index 0000000..f2acbb8
--- /dev/null
+++ b/drivers/gpio/gpio-cyusbs23x.c
@@ -0,0 +1,174 @@
+/*
+ * GPIO subdriver for Cypress CYUSBS234 USB-Serial Bridge controller.
+ * Details about the device can be found at:
+ *    http://www.cypress.com/?rID=84126
+ *
+ * Copyright (c) 2014 Cypress Semiconductor Corporation.
+ *
+ * Author:
+ *   Muthu Mani <muth-+wT8y+m8/X5BDgjK7y7TUQ@public.gmane.org>
+ *
+ * Additional contributors include:
+ *   Rajaram Regupathy <rera-+wT8y+m8/X5BDgjK7y7TUQ@public.gmane.org>
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License version 2 as published by
+ * the Free Software Foundation.
+ */
+
+/*
+ * All GPIOs are exposed for get operation. Only the GPIOs which are configured
+ * by the user using the Configuration Utility can be set. Attempting to set
+ * value of unconfigured GPIOs would fail
+ */
+
+#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/platform_device.h>
+
+#include <linux/usb.h>
+#include <linux/gpio.h>
+
+#include <linux/mfd/cyusbs23x.h>
+
+#define CY_GPIO_GET_LEN		2
+
+struct cyusbs_gpio {
+	struct gpio_chip gpio;
+	struct cyusbs23x *cyusbs;
+};
+
+#define to_cyusbs_gpio(chip) container_of(chip, struct cyusbs_gpio, gpio)
+
+static int cy_gpio_get(struct gpio_chip *chip,
+			unsigned offset)
+{
+	int ret;
+	char *buf;
+	u16 wIndex, wValue;
+	struct cyusbs_gpio *gpio = to_cyusbs_gpio(chip);
+	struct cyusbs23x *cyusbs = gpio->cyusbs;
+
+	wValue = offset;
+	wIndex = 0;
+	buf = kmalloc(CY_GPIO_GET_LEN, GFP_KERNEL);
+	if (buf == NULL)
+		return -ENOMEM;
+
+	ret = usb_control_msg(cyusbs->usb_dev,
+			usb_rcvctrlpipe(cyusbs->usb_dev, 0),
+			CY_GPIO_GET_VALUE_CMD,
+			USB_TYPE_VENDOR | USB_RECIP_DEVICE | USB_DIR_IN,
+			wValue, wIndex, buf, CY_GPIO_GET_LEN,
+			CY_USBS_CTRL_XFER_TIMEOUT);
+	if (ret == CY_GPIO_GET_LEN) {
+		dev_dbg(chip->dev, "%s: %02X %02X\n", __func__, buf[0], buf[1]);
+		if (buf[0] == 0)
+			ret = buf[1];
+		else
+			ret = -EIO;
+	} else {
+		dev_err(chip->dev, "%s: %d\n", __func__, ret);
+		ret = -EIO;
+	}
+
+	kfree(buf);
+	return ret;
+}
+
+static void cy_gpio_set(struct gpio_chip *chip,
+			unsigned offset, int value)
+{
+	int ret;
+	u16 wIndex, wValue;
+	struct cyusbs_gpio *gpio = to_cyusbs_gpio(chip);
+	struct cyusbs23x *cyusbs = gpio->cyusbs;
+
+	wValue = offset;
+	wIndex = value;
+
+	ret = usb_control_msg(cyusbs->usb_dev,
+			usb_sndctrlpipe(cyusbs->usb_dev, 0),
+			CY_GPIO_SET_VALUE_CMD,
+			USB_TYPE_VENDOR | USB_RECIP_DEVICE | USB_DIR_IN,
+			wValue, wIndex, NULL, 0, CY_USBS_CTRL_XFER_TIMEOUT);
+	if (ret < 0)
+		dev_err(chip->dev, "error setting gpio: %d\n", ret);
+}
+
+static int cy_gpio_direction_input(struct gpio_chip *chip,
+					unsigned offset)
+{
+	return 0;
+}
+
+static int cy_gpio_direction_output(struct gpio_chip *chip,
+					unsigned offset, int value)
+{
+	return 0;
+}
+
+static int cyusbs23x_gpio_probe(struct platform_device *pdev)
+{
+	struct cyusbs23x *cyusbs;
+	struct cyusbs_gpio *cy_gpio;
+	int ret = 0;
+
+	dev_dbg(&pdev->dev, "%s\n", __func__);
+
+	cyusbs = dev_get_drvdata(pdev->dev.parent);
+
+	cy_gpio = devm_kzalloc(&pdev->dev, sizeof(*cy_gpio), GFP_KERNEL);
+	if (cy_gpio == NULL)
+		return -ENOMEM;
+
+	cy_gpio->cyusbs = cyusbs;
+	/* registering gpio */
+	cy_gpio->gpio.label = dev_name(&pdev->dev);
+	cy_gpio->gpio.dev = &pdev->dev;
+	cy_gpio->gpio.owner = THIS_MODULE;
+	cy_gpio->gpio.base = -1;
+	cy_gpio->gpio.ngpio = 12; /* total GPIOs */
+	cy_gpio->gpio.can_sleep = true;
+	cy_gpio->gpio.set = cy_gpio_set;
+	cy_gpio->gpio.get = cy_gpio_get;
+	cy_gpio->gpio.direction_input = cy_gpio_direction_input;
+	cy_gpio->gpio.direction_output = cy_gpio_direction_output;
+	ret = gpiochip_add(&cy_gpio->gpio);
+	if (ret < 0) {
+		dev_err(cy_gpio->gpio.dev, "could not add gpio\n");
+		return ret;
+	}
+
+	platform_set_drvdata(pdev, cy_gpio);
+
+	dev_dbg(&pdev->dev, "added GPIO\n");
+	return ret;
+}
+
+static int cyusbs23x_gpio_remove(struct platform_device *pdev)
+{
+	struct cyusbs_gpio *cy_gpio = platform_get_drvdata(pdev);
+
+	dev_dbg(&pdev->dev, "%s\n", __func__);
+	gpiochip_remove(&cy_gpio->gpio);
+	return 0;
+}
+
+static struct platform_driver cyusbs23x_gpio_driver = {
+	.driver.name    = "cyusbs23x-gpio",
+	.probe          = cyusbs23x_gpio_probe,
+	.remove         = cyusbs23x_gpio_remove,
+};
+
+module_platform_driver(cyusbs23x_gpio_driver);
+
+MODULE_AUTHOR("Rajaram Regupathy <rera-+wT8y+m8/X5BDgjK7y7TUQ@public.gmane.org>");
+MODULE_AUTHOR("Muthu Mani <muth-+wT8y+m8/X5BDgjK7y7TUQ@public.gmane.org>");
+MODULE_DESCRIPTION("GPIO driver for CYUSBS23x");
+MODULE_LICENSE("GPL v2");
+MODULE_ALIAS("platform:cyusbs23x-gpio");
-- 
1.8.3.2

Re: [PATCH v3 3/3] gpio: add support for Cypress CYUSBS234 USB-GPIO adapter

[Alexandre Courbot]

On Mon, Oct 6, 2014 at 11:47 PM, Muthu Mani <muth@cypress.com> wrote:
> +
> +static int cy_gpio_direction_input(struct gpio_chip *chip,
> +                                       unsigned offset)
> +{
> +       return 0;
> +}
> +
> +static int cy_gpio_direction_output(struct gpio_chip *chip,
> +                                       unsigned offset, int value)
> +{
> +       return 0;
> +}

If that chip is capable of both output and input, shouldn't these
functions be implemented? I think this has already been pointed out in
a previous version but you did not reply.

> +
> +static int cyusbs23x_gpio_probe(struct platform_device *pdev)
> +{
> +       struct cyusbs23x *cyusbs;
> +       struct cyusbs_gpio *cy_gpio;
> +       int ret = 0;
> +
> +       dev_dbg(&pdev->dev, "%s\n", __func__);

> +
> +       cyusbs = dev_get_drvdata(pdev->dev.parent);
> +
> +       cy_gpio = devm_kzalloc(&pdev->dev, sizeof(*cy_gpio), GFP_KERNEL);
> +       if (cy_gpio == NULL)
> +               return -ENOMEM;
> +
> +       cy_gpio->cyusbs = cyusbs;
> +       /* registering gpio */
> +       cy_gpio->gpio.label = dev_name(&pdev->dev);
> +       cy_gpio->gpio.dev = &pdev->dev;
> +       cy_gpio->gpio.owner = THIS_MODULE;
> +       cy_gpio->gpio.base = -1;
> +       cy_gpio->gpio.ngpio = 12; /* total GPIOs */

Isn't there a way to get the number of GPIOs from a reliable source?
If not, please at least use a macro here.

RE: [PATCH v3 3/3] gpio: add support for Cypress CYUSBS234 USB-GPIO adapter

[Muthu Mani]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 2532 bytes --]

> -----Original Message-----
> From: Alexandre Courbot [mailto:gnurou@gmail.com]
> Sent: Tuesday, October 07, 2014 3:34 PM
> To: Muthu Mani
> Cc: Samuel Ortiz; Lee Jones; Wolfram Sang; Linus Walleij; Greg Kroah-
> Hartman; linux-i2c@vger.kernel.org; linux-gpio@vger.kernel.org; linux-
> usb@vger.kernel.org; Linux Kernel Mailing List; Rajaram Regupathy; Johan
> Hovold
> Subject: Re: [PATCH v3 3/3] gpio: add support for Cypress CYUSBS234 USB-
> GPIO adapter
> 
> On Mon, Oct 6, 2014 at 11:47 PM, Muthu Mani <muth@cypress.com> wrote:
> > +
> > +static int cy_gpio_direction_input(struct gpio_chip *chip,
> > +                                       unsigned offset) {
> > +       return 0;
> > +}
> > +
> > +static int cy_gpio_direction_output(struct gpio_chip *chip,
> > +                                       unsigned offset, int value) {
> > +       return 0;
> > +}
> 
> If that chip is capable of both output and input, shouldn't these functions be
> implemented? I think this has already been pointed out in a previous version
> but you did not reply.

Thanks for your inputs.

Only the GPIOs which are configured to be output GPIO can be set.
The set operation would fail trying to set the input or unconfigured GPIOs.
In this version of driver, this support is not added, it can be introduced in future versions.
I will add a TODO note in the code.

> 
> > +
> > +static int cyusbs23x_gpio_probe(struct platform_device *pdev) {
> > +       struct cyusbs23x *cyusbs;
> > +       struct cyusbs_gpio *cy_gpio;
> > +       int ret = 0;
> > +
> > +       dev_dbg(&pdev->dev, "%s\n", __func__);
> 
> > +
> > +       cyusbs = dev_get_drvdata(pdev->dev.parent);
> > +
> > +       cy_gpio = devm_kzalloc(&pdev->dev, sizeof(*cy_gpio), GFP_KERNEL);
> > +       if (cy_gpio == NULL)
> > +               return -ENOMEM;
> > +
> > +       cy_gpio->cyusbs = cyusbs;
> > +       /* registering gpio */
> > +       cy_gpio->gpio.label = dev_name(&pdev->dev);
> > +       cy_gpio->gpio.dev = &pdev->dev;
> > +       cy_gpio->gpio.owner = THIS_MODULE;
> > +       cy_gpio->gpio.base = -1;
> > +       cy_gpio->gpio.ngpio = 12; /* total GPIOs */
> 
> Isn't there a way to get the number of GPIOs from a reliable source?
> If not, please at least use a macro here.

CYUSBS234 has 12 GPIOs in total as per the datasheet. I will add a macro and use it.
N‹§²æìr¸›yúèšØb²X¬¶Ç§vØ^–)Þº{.nÇ+‰·¥Š{±ºÆâžØ^n‡r¡ö¦zË\x1aëh™¨è­Ú&¢îý»\x05ËÛÔØï¦v¬Îf\x1dp)¹¹br	šê+€Ê+zf£¢·hšˆ§~†­†Ûiÿûàz¹\x1e®w¥¢¸?™¨è­Ú&¢)ߢ^[f

Re: [PATCH v3 3/3] gpio: add support for Cypress CYUSBS234 USB-GPIO adapter

[Alexandre Courbot]

On Wed, Oct 8, 2014 at 4:09 PM, Muthu Mani <muth-+wT8y+m8/X5BDgjK7y7TUQ@public.gmane.org> wrote:
>> -----Original Message-----
>> From: Alexandre Courbot [mailto:gnurou-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org]
>> Sent: Tuesday, October 07, 2014 3:34 PM
>> To: Muthu Mani
>> Cc: Samuel Ortiz; Lee Jones; Wolfram Sang; Linus Walleij; Greg Kroah-
>> Hartman; linux-i2c-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; linux-gpio-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; linux-
>> usb-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; Linux Kernel Mailing List; Rajaram Regupathy; Johan
>> Hovold
>> Subject: Re: [PATCH v3 3/3] gpio: add support for Cypress CYUSBS234 USB-
>> GPIO adapter
>>
>> On Mon, Oct 6, 2014 at 11:47 PM, Muthu Mani <muth-+wT8y+m8/X5BDgjK7y7TUQ@public.gmane.org> wrote:
>> > +
>> > +static int cy_gpio_direction_input(struct gpio_chip *chip,
>> > +                                       unsigned offset) {
>> > +       return 0;
>> > +}
>> > +
>> > +static int cy_gpio_direction_output(struct gpio_chip *chip,
>> > +                                       unsigned offset, int value) {
>> > +       return 0;
>> > +}
>>
>> If that chip is capable of both output and input, shouldn't these functions be
>> implemented? I think this has already been pointed out in a previous version
>> but you did not reply.
>
> Thanks for your inputs.
>
> Only the GPIOs which are configured to be output GPIO can be set.

In that case cy_gpio_set() should return an error for GPIOs which are
not configured as outputs. Is that guaranteed by the current
implementation?

> The set operation would fail trying to set the input or unconfigured GPIOs.
> In this version of driver, this support is not added, it can be introduced in future versions.
> I will add a TODO note in the code.

Argh, no TODO please. Actual code that will turn this code into a
solid driver that can be merged.

Can all GPIOs be set as input or output? If so, please implement
cy_gpio_direction_*() and make sure that cy_gpio_set() behaves
properly if a GPIO is not an output. If the input/output GPIOs are
fixed, please make cy_gpio_direction_*() return an error if the GPIO
capabilities does not correspond to what is asked, and again ensure
that cy_gpio_set() works as expected.
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 3/3] gpio: add support for Cypress CYUSBS234 USB-GPIO adapter

[RR]

On Wed, Oct 8, 2014 at 12:18 AM, Alexandre Courbot <gnurou@gmail.com> wrote:
> On Wed, Oct 8, 2014 at 4:09 PM, Muthu Mani <muth@cypress.com> wrote:
>>> -----Original Message-----
>>> From: Alexandre Courbot [mailto:gnurou@gmail.com]
>>> Sent: Tuesday, October 07, 2014 3:34 PM
>>> To: Muthu Mani
>>> Cc: Samuel Ortiz; Lee Jones; Wolfram Sang; Linus Walleij; Greg Kroah-
>>> Hartman; linux-i2c@vger.kernel.org; linux-gpio@vger.kernel.org; linux-
>>> usb@vger.kernel.org; Linux Kernel Mailing List; Rajaram Regupathy; Johan
>>> Hovold
>>> Subject: Re: [PATCH v3 3/3] gpio: add support for Cypress CYUSBS234 USB-
>>> GPIO adapter
>>>
>>> On Mon, Oct 6, 2014 at 11:47 PM, Muthu Mani <muth@cypress.com> wrote:
>>> > +
>>> > +static int cy_gpio_direction_input(struct gpio_chip *chip,
>>> > +                                       unsigned offset) {
>>> > +       return 0;
>>> > +}
>>> > +
>>> > +static int cy_gpio_direction_output(struct gpio_chip *chip,
>>> > +                                       unsigned offset, int value) {
>>> > +       return 0;
>>> > +}
>>>
>>> If that chip is capable of both output and input, shouldn't these functions be
>>> implemented? I think this has already been pointed out in a previous version
>>> but you did not reply.
>>
>> Thanks for your inputs.
>>
>> Only the GPIOs which are configured to be output GPIO can be set.
>
> In that case cy_gpio_set() should return an error for GPIOs which are
> not configured as outputs. Is that guaranteed by the current
> implementation?
>
>> The set operation would fail trying to set the input or unconfigured GPIOs.
>> In this version of driver, this support is not added, it can be introduced in future versions.
>> I will add a TODO note in the code.
>
> Argh, no TODO please. Actual code that will turn this code into a
> solid driver that can be merged.

Does a driver targeted for a custom device has to implement every
functionality in the 1st version ? My understanding is that Linux
follows incremental model and allows incremental merge.

(On a side note the driver is functionally verified with the necessary hardware)

Please correct me if I am missing something

>
> Can all GPIOs be set as input or output? If so, please implement
> cy_gpio_direction_*() and make sure that cy_gpio_set() behaves
> properly if a GPIO is not an output. If the input/output GPIOs are
> fixed, please make cy_gpio_direction_*() return an error if the GPIO
> capabilities does not correspond to what is asked, and again ensure
> that cy_gpio_set() works as expected.
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v3 3/3] gpio: add support for Cypress CYUSBS234 USB-GPIO adapter

[Alexandre Courbot]

On Thu, Oct 9, 2014 at 8:46 AM, RR <rajaram.officemails@gmail.com> wrote:
> On Wed, Oct 8, 2014 at 12:18 AM, Alexandre Courbot <gnurou@gmail.com> wrote:
>> On Wed, Oct 8, 2014 at 4:09 PM, Muthu Mani <muth@cypress.com> wrote:
>>>> -----Original Message-----
>>>> From: Alexandre Courbot [mailto:gnurou@gmail.com]
>>>> Sent: Tuesday, October 07, 2014 3:34 PM
>>>> To: Muthu Mani
>>>> Cc: Samuel Ortiz; Lee Jones; Wolfram Sang; Linus Walleij; Greg Kroah-
>>>> Hartman; linux-i2c@vger.kernel.org; linux-gpio@vger.kernel.org; linux-
>>>> usb@vger.kernel.org; Linux Kernel Mailing List; Rajaram Regupathy; Johan
>>>> Hovold
>>>> Subject: Re: [PATCH v3 3/3] gpio: add support for Cypress CYUSBS234 USB-
>>>> GPIO adapter
>>>>
>>>> On Mon, Oct 6, 2014 at 11:47 PM, Muthu Mani <muth@cypress.com> wrote:
>>>> > +
>>>> > +static int cy_gpio_direction_input(struct gpio_chip *chip,
>>>> > +                                       unsigned offset) {
>>>> > +       return 0;
>>>> > +}
>>>> > +
>>>> > +static int cy_gpio_direction_output(struct gpio_chip *chip,
>>>> > +                                       unsigned offset, int value) {
>>>> > +       return 0;
>>>> > +}
>>>>
>>>> If that chip is capable of both output and input, shouldn't these functions be
>>>> implemented? I think this has already been pointed out in a previous version
>>>> but you did not reply.
>>>
>>> Thanks for your inputs.
>>>
>>> Only the GPIOs which are configured to be output GPIO can be set.
>>
>> In that case cy_gpio_set() should return an error for GPIOs which are
>> not configured as outputs. Is that guaranteed by the current
>> implementation?
>>
>>> The set operation would fail trying to set the input or unconfigured GPIOs.
>>> In this version of driver, this support is not added, it can be introduced in future versions.
>>> I will add a TODO note in the code.
>>
>> Argh, no TODO please. Actual code that will turn this code into a
>> solid driver that can be merged.
>
> Does a driver targeted for a custom device has to implement every
> functionality in the 1st version ? My understanding is that Linux
> follows incremental model and allows incremental merge.

Certainly, but we are talking about very basic functionality here.

Right now this driver will incorrectly returns success when trying to
set the direction of a GPIO, even though it did not do anything. There
is also no hint that _set will return an error if called on an
input/unconfigured GPIO.

So while we are not asking for a driver to be perfect on the first
iteration, setting the direction of a GPIO is a very basic
functionality that is essential for a GPIO driver to just work. Custom
device or not, this is mainline. The rest of the driver seems to be
ok, so please take the last few steps that will allow us to merge a
featured driver.

Re: [PATCH v3 3/3] gpio: add support for Cypress CYUSBS234 USB-GPIO adapter

[Linus Walleij]

On Thu, Oct 9, 2014 at 1:46 AM, RR <rajaram.officemails@gmail.com> wrote:
> On Wed, Oct 8, 2014 at 12:18 AM, Alexandre Courbot <gnurou@gmail.com> wrote:
>> On Wed, Oct 8, 2014 at 4:09 PM, Muthu Mani <muth@cypress.com> wrote:

>>>> > +static int cy_gpio_direction_output(struct gpio_chip *chip,
>>>> > +                                       unsigned offset, int value) {
>>>> > +       return 0;
>>>> > +}
>>>>
>>>> If that chip is capable of both output and input, shouldn't these functions be
>>>> implemented? I think this has already been pointed out in a previous version
>>>> but you did not reply.
>>>
>>> Thanks for your inputs.
>>>
>>> Only the GPIOs which are configured to be output GPIO can be set.
>>
>> In that case cy_gpio_set() should return an error for GPIOs which are
>> not configured as outputs. Is that guaranteed by the current
>> implementation?
>>
>>> The set operation would fail trying to set the input or unconfigured GPIOs.
>>> In this version of driver, this support is not added, it can be introduced in future versions.
>>> I will add a TODO note in the code.
>>
>> Argh, no TODO please. Actual code that will turn this code into a
>> solid driver that can be merged.
>
> Does a driver targeted for a custom device has to implement every
> functionality in the 1st version ?

When you post a driver to the GPIO maintainers it is *NOT* tageted
at a consumer device, it is targeted at the kernel community and
upstream maintainers.

Of course you can deliver add-on patches out-of-tree to your
customers, it's generally a bad idea for the long term and maintenance
of your driver, but it's your pick.

Yours,
Linus Walleij

Re: [PATCH v3 3/3] gpio: add support for Cypress CYUSBS234 USB-GPIO adapter

[RR]

On Mon, Oct 27, 2014 at 9:00 PM, Linus Walleij <linus.walleij@linaro.org> wrote:
> On Thu, Oct 9, 2014 at 1:46 AM, RR <rajaram.officemails@gmail.com> wrote:
>> On Wed, Oct 8, 2014 at 12:18 AM, Alexandre Courbot <gnurou@gmail.com> wrote:
>>> On Wed, Oct 8, 2014 at 4:09 PM, Muthu Mani <muth@cypress.com> wrote:
>
>>>>> > +static int cy_gpio_direction_output(struct gpio_chip *chip,
>>>>> > +                                       unsigned offset, int value) {
>>>>> > +       return 0;
>>>>> > +}
>>>>>
>>>>> If that chip is capable of both output and input, shouldn't these functions be
>>>>> implemented? I think this has already been pointed out in a previous version
>>>>> but you did not reply.
>>>>
>>>> Thanks for your inputs.
>>>>
>>>> Only the GPIOs which are configured to be output GPIO can be set.
>>>
>>> In that case cy_gpio_set() should return an error for GPIOs which are
>>> not configured as outputs. Is that guaranteed by the current
>>> implementation?
>>>
>>>> The set operation would fail trying to set the input or unconfigured GPIOs.
>>>> In this version of driver, this support is not added, it can be introduced in future versions.
>>>> I will add a TODO note in the code.
>>>
>>> Argh, no TODO please. Actual code that will turn this code into a
>>> solid driver that can be merged.
>>
>> Does a driver targeted for a custom device has to implement every
>> functionality in the 1st version ?
>
> When you post a driver to the GPIO maintainers it is *NOT* tageted
> at a consumer device, it is targeted at the kernel community and
> upstream maintainers.

Totally agree. What I was conveying the patch has not modified
any "core" kernel function and is specific to a device thus will not
affect system.

>
> Of course you can deliver add-on patches out-of-tree to your
> customers, it's generally a bad idea for the long term and maintenance
> of your driver, but it's your pick.

AFAIR In the recent past xHCI or gadget core or musb or dw3
patches were added in increments. May be my analogy is incorrect and
I am ignorant of some philosophy here.

Sincerely I somehow was not convinced basic functionality is missing
as referred in the review comment.We have tested the driver for most of
the functionality of our DVK and is working perfectly.

Moreover currently we do not expect an user to set gpio direction as
it involves vendor specific usb control commands.

Having said that we have taken the feedback and working to close this.

>
> Yours,
> Linus Walleij

[PATCH v3 3/3] gpio: add support for Cypress CYUSBS234 USB-GPIO adapter

[Muthu Mani]

Adds support for USB-GPIO interface of Cypress Semiconductor
CYUSBS234 USB-Serial Bridge controller.

The GPIO get/set can be done through vendor command on control endpoint
for the configured gpios.

Details about the device can be found at:
http://www.cypress.com/?rID=84126

Signed-off-by: Muthu Mani <muth-+wT8y+m8/X5BDgjK7y7TUQ@public.gmane.org>
Signed-off-by: Rajaram Regupathy <rera-+wT8y+m8/X5BDgjK7y7TUQ@public.gmane.org>
---
 drivers/gpio/Kconfig          |  13 ++++
 drivers/gpio/Makefile         |   1 +
 drivers/gpio/gpio-cyusbs23x.c | 174 ++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 188 insertions(+)
 create mode 100644 drivers/gpio/gpio-cyusbs23x.c

diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
index 9de1515..932e07c 100644
--- a/drivers/gpio/Kconfig
+++ b/drivers/gpio/Kconfig
@@ -886,6 +886,19 @@ config GPIO_BCM_KONA
 
 comment "USB GPIO expanders:"
 
+config GPIO_CYUSBS23X
+	tristate "CYUSBS23x GPIO support"
+	depends on MFD_CYUSBS23X && USB
+	help
+	  Say yes here to access the GPIO signals of Cypress
+	  Semiconductor CYUSBS23x USB Serial Bridge Controller.
+
+	  This driver enables the GPIO interface of CYUSBS23x USB Serial
+	  Bridge controller.
+
+	  This driver can also be built as a module. If so, the module will be
+	  called gpio-cyusbs23x.
+
 config GPIO_VIPERBOARD
 	tristate "Viperboard GPIO a & b support"
 	depends on MFD_VIPERBOARD && USB
diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile
index 5d024e3..3ad89f1 100644
--- a/drivers/gpio/Makefile
+++ b/drivers/gpio/Makefile
@@ -23,6 +23,7 @@ obj-$(CONFIG_GPIO_BT8XX)	+= gpio-bt8xx.o
 obj-$(CONFIG_GPIO_CLPS711X)	+= gpio-clps711x.o
 obj-$(CONFIG_GPIO_CS5535)	+= gpio-cs5535.o
 obj-$(CONFIG_GPIO_CRYSTAL_COVE)	+= gpio-crystalcove.o
+obj-$(CONFIG_GPIO_CYUSBS23X)	+= gpio-cyusbs23x.o
 obj-$(CONFIG_GPIO_DA9052)	+= gpio-da9052.o
 obj-$(CONFIG_GPIO_DA9055)	+= gpio-da9055.o
 obj-$(CONFIG_GPIO_DAVINCI)	+= gpio-davinci.o
diff --git a/drivers/gpio/gpio-cyusbs23x.c b/drivers/gpio/gpio-cyusbs23x.c
new file mode 100644
index 0000000..f2acbb8
--- /dev/null
+++ b/drivers/gpio/gpio-cyusbs23x.c
@@ -0,0 +1,174 @@
+/*
+ * GPIO subdriver for Cypress CYUSBS234 USB-Serial Bridge controller.
+ * Details about the device can be found at:
+ *    http://www.cypress.com/?rID=84126
+ *
+ * Copyright (c) 2014 Cypress Semiconductor Corporation.
+ *
+ * Author:
+ *   Muthu Mani <muth-+wT8y+m8/X5BDgjK7y7TUQ@public.gmane.org>
+ *
+ * Additional contributors include:
+ *   Rajaram Regupathy <rera-+wT8y+m8/X5BDgjK7y7TUQ@public.gmane.org>
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License version 2 as published by
+ * the Free Software Foundation.
+ */
+
+/*
+ * All GPIOs are exposed for get operation. Only the GPIOs which are configured
+ * by the user using the Configuration Utility can be set. Attempting to set
+ * value of unconfigured GPIOs would fail
+ */
+
+#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/platform_device.h>
+
+#include <linux/usb.h>
+#include <linux/gpio.h>
+
+#include <linux/mfd/cyusbs23x.h>
+
+#define CY_GPIO_GET_LEN		2
+
+struct cyusbs_gpio {
+	struct gpio_chip gpio;
+	struct cyusbs23x *cyusbs;
+};
+
+#define to_cyusbs_gpio(chip) container_of(chip, struct cyusbs_gpio, gpio)
+
+static int cy_gpio_get(struct gpio_chip *chip,
+			unsigned offset)
+{
+	int ret;
+	char *buf;
+	u16 wIndex, wValue;
+	struct cyusbs_gpio *gpio = to_cyusbs_gpio(chip);
+	struct cyusbs23x *cyusbs = gpio->cyusbs;
+
+	wValue = offset;
+	wIndex = 0;
+	buf = kmalloc(CY_GPIO_GET_LEN, GFP_KERNEL);
+	if (buf == NULL)
+		return -ENOMEM;
+
+	ret = usb_control_msg(cyusbs->usb_dev,
+			usb_rcvctrlpipe(cyusbs->usb_dev, 0),
+			CY_GPIO_GET_VALUE_CMD,
+			USB_TYPE_VENDOR | USB_RECIP_DEVICE | USB_DIR_IN,
+			wValue, wIndex, buf, CY_GPIO_GET_LEN,
+			CY_USBS_CTRL_XFER_TIMEOUT);
+	if (ret == CY_GPIO_GET_LEN) {
+		dev_dbg(chip->dev, "%s: %02X %02X\n", __func__, buf[0], buf[1]);
+		if (buf[0] == 0)
+			ret = buf[1];
+		else
+			ret = -EIO;
+	} else {
+		dev_err(chip->dev, "%s: %d\n", __func__, ret);
+		ret = -EIO;
+	}
+
+	kfree(buf);
+	return ret;
+}
+
+static void cy_gpio_set(struct gpio_chip *chip,
+			unsigned offset, int value)
+{
+	int ret;
+	u16 wIndex, wValue;
+	struct cyusbs_gpio *gpio = to_cyusbs_gpio(chip);
+	struct cyusbs23x *cyusbs = gpio->cyusbs;
+
+	wValue = offset;
+	wIndex = value;
+
+	ret = usb_control_msg(cyusbs->usb_dev,
+			usb_sndctrlpipe(cyusbs->usb_dev, 0),
+			CY_GPIO_SET_VALUE_CMD,
+			USB_TYPE_VENDOR | USB_RECIP_DEVICE | USB_DIR_IN,
+			wValue, wIndex, NULL, 0, CY_USBS_CTRL_XFER_TIMEOUT);
+	if (ret < 0)
+		dev_err(chip->dev, "error setting gpio: %d\n", ret);
+}
+
+static int cy_gpio_direction_input(struct gpio_chip *chip,
+					unsigned offset)
+{
+	return 0;
+}
+
+static int cy_gpio_direction_output(struct gpio_chip *chip,
+					unsigned offset, int value)
+{
+	return 0;
+}
+
+static int cyusbs23x_gpio_probe(struct platform_device *pdev)
+{
+	struct cyusbs23x *cyusbs;
+	struct cyusbs_gpio *cy_gpio;
+	int ret = 0;
+
+	dev_dbg(&pdev->dev, "%s\n", __func__);
+
+	cyusbs = dev_get_drvdata(pdev->dev.parent);
+
+	cy_gpio = devm_kzalloc(&pdev->dev, sizeof(*cy_gpio), GFP_KERNEL);
+	if (cy_gpio == NULL)
+		return -ENOMEM;
+
+	cy_gpio->cyusbs = cyusbs;
+	/* registering gpio */
+	cy_gpio->gpio.label = dev_name(&pdev->dev);
+	cy_gpio->gpio.dev = &pdev->dev;
+	cy_gpio->gpio.owner = THIS_MODULE;
+	cy_gpio->gpio.base = -1;
+	cy_gpio->gpio.ngpio = 12; /* total GPIOs */
+	cy_gpio->gpio.can_sleep = true;
+	cy_gpio->gpio.set = cy_gpio_set;
+	cy_gpio->gpio.get = cy_gpio_get;
+	cy_gpio->gpio.direction_input = cy_gpio_direction_input;
+	cy_gpio->gpio.direction_output = cy_gpio_direction_output;
+	ret = gpiochip_add(&cy_gpio->gpio);
+	if (ret < 0) {
+		dev_err(cy_gpio->gpio.dev, "could not add gpio\n");
+		return ret;
+	}
+
+	platform_set_drvdata(pdev, cy_gpio);
+
+	dev_dbg(&pdev->dev, "added GPIO\n");
+	return ret;
+}
+
+static int cyusbs23x_gpio_remove(struct platform_device *pdev)
+{
+	struct cyusbs_gpio *cy_gpio = platform_get_drvdata(pdev);
+
+	dev_dbg(&pdev->dev, "%s\n", __func__);
+	gpiochip_remove(&cy_gpio->gpio);
+	return 0;
+}
+
+static struct platform_driver cyusbs23x_gpio_driver = {
+	.driver.name    = "cyusbs23x-gpio",
+	.probe          = cyusbs23x_gpio_probe,
+	.remove         = cyusbs23x_gpio_remove,
+};
+
+module_platform_driver(cyusbs23x_gpio_driver);
+
+MODULE_AUTHOR("Rajaram Regupathy <rera-+wT8y+m8/X5BDgjK7y7TUQ@public.gmane.org>");
+MODULE_AUTHOR("Muthu Mani <muth-+wT8y+m8/X5BDgjK7y7TUQ@public.gmane.org>");
+MODULE_DESCRIPTION("GPIO driver for CYUSBS23x");
+MODULE_LICENSE("GPL v2");
+MODULE_ALIAS("platform:cyusbs23x-gpio");
-- 
1.8.3.2

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html