Date:	Thu, 29 Aug 2013 10:35:31 +0900
From:	Chanwoo Choi <cw00.choi@...sung.com>
To:	George Cherian <george.cherian@...com>
Cc:	balbi@...com, myungjoo.ham@...sung.com, linux-doc@...r.kernel.org,
	linux-kernel@...r.kernel.org, devicetree@...r.kernel.org,
	grant.likely@...aro.org, rob@...dley.net, ian.campbell@...rix.com,
	swarren@...dotorg.org, mark.rutland@....com, pawel.moll@....com,
	rob.herring@...xeda.com, linux-omap@...r.kernel.org,
	linux-usb@...r.kernel.org, bcousson@...libre.com,
	davidb@...eaurora.org, arnd@...db.de, swarren@...dia.com,
	popcornmix@...il.com
Subject: Re: [PATCH v3 1/3] extcon: extcon-gpio-usbvid: Generic USB VBUS/ID
 detection via GPIO

Hi George,

You didn't modify this patchset about my comment on v1 patchset.
Please pay attention to comment.

On 08/29/2013 02:33 AM, George Cherian wrote:
> Add a generic USB VBUS/ID detection EXTCON driver. This driver expects
> the ID/VBUS pin are connected via GPIOs. This driver is tested on
> DRA7x board were the ID pin is routed via GPIOs. The driver supports
> both VBUS and ID pin configuration and ID pin only configuration.
> 
> Signed-off-by: George Cherian <george.cherian@...com>
> ---
>  .../bindings/extcon/extcon-gpio-usbvid.txt         |  20 ++
>  drivers/extcon/Kconfig                             |   6 +
>  drivers/extcon/Makefile                            |   1 +
>  drivers/extcon/extcon-gpio-usbvid.c                | 286 +++++++++++++++++++++

You should keep following naming stlye. extcon-gpio-usbvid.c is wrong naming style.
- extcon-[device name].c
- extcon-gpio-usbvid.c -> extcon-dra7xx.c or etc.

Also, you should change the file name of extcon-gpio-usbvid.txt.

Finally, You used 'gpio_usbvid' prefix in extcon-gpio-usbvid.c.
It has caused the confusion that user would think extcon-gpio-usbvid.c driver
support all of extcon driver using gpio irq pin. So I'd like you to use
proper prefix including device name.

>  4 files changed, 313 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/extcon/extcon-gpio-usbvid.txt
>  create mode 100644 drivers/extcon/extcon-gpio-usbvid.c
> 
> diff --git a/Documentation/devicetree/bindings/extcon/extcon-gpio-usbvid.txt b/Documentation/devicetree/bindings/extcon/extcon-gpio-usbvid.txt
> new file mode 100644
> index 0000000..eea0741
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/extcon/extcon-gpio-usbvid.txt
> @@ -0,0 +1,20 @@
> +EXTCON FOR USB VIA GPIO
> +
> +Required Properties:
> + - compatible : Should be "ti,gpio-usb-vid" for USB VBUS-ID detector
> +		using gpios or "ti,gpio-usb-id" for USB ID pin detector
> + - gpios : phandle and args ID pin gpio and VBUS gpio.
> +	   The first gpio used  for ID pin detection
> +	   and the second used for VBUS detection.
> +	   ID pin gpio is mandatory and VBUS is optional
> +	   depending on implementation.
> +
> +Please refer to ../gpio/gpio.txt for details of the common GPIO bindings
> +
> +Example:
> +
> +	gpio_usbvid_extcon1 {
> +		compatible = "ti,gpio-usb-vid";
> +		gpios = <&gpio1 1 0>,
> +			<&gpio2 2 0>;
> +	};
> diff --git a/drivers/extcon/Kconfig b/drivers/extcon/Kconfig
> index f1d54a3..8097398 100644
> --- a/drivers/extcon/Kconfig
> +++ b/drivers/extcon/Kconfig
> @@ -64,4 +64,10 @@ config EXTCON_PALMAS
>  	  Say Y here to enable support for USB peripheral and USB host
>  	  detection by palmas usb.
>  
> +config EXTCON_GPIO_USBVID
> +	tristate "Generic USB VBUS/ID detection using GPIO EXTCON support"
> +	help
> +	  Say Y here to enable support for USB VBUS/ID deetction by GPIO.
> +
> +

Remove blank line.

>  endif # MULTISTATE_SWITCH
> diff --git a/drivers/extcon/Makefile b/drivers/extcon/Makefile
> index e4fa8ba..0451f698 100644
> --- a/drivers/extcon/Makefile
> +++ b/drivers/extcon/Makefile
> @@ -10,3 +10,4 @@ obj-$(CONFIG_EXTCON_MAX77693)	+= extcon-max77693.o
>  obj-$(CONFIG_EXTCON_MAX8997)	+= extcon-max8997.o
>  obj-$(CONFIG_EXTCON_ARIZONA)	+= extcon-arizona.o
>  obj-$(CONFIG_EXTCON_PALMAS)	+= extcon-palmas.o
> +obj-$(CONFIG_EXTCON_GPIO_USBVID)	+= extcon-gpio-usbvid.o
> diff --git a/drivers/extcon/extcon-gpio-usbvid.c b/drivers/extcon/extcon-gpio-usbvid.c
> new file mode 100644
> index 0000000..e9bc2a97
> --- /dev/null
> +++ b/drivers/extcon/extcon-gpio-usbvid.c
> @@ -0,0 +1,286 @@
> +/*
> + * Generic  USB VBUS-ID pin detection driver
> + *
> + * Copyright (C) 2013 Texas Instruments Incorporated - http://www.ti.com
> + * 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; either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * Author: George Cherian <george.cherian@...com>
> + *
> + * Based on extcon-palmas.c
> + *
> + * Author: Kishon Vijay Abraham I <kishon@...com>
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + */
> +
> +#include <linux/module.h>
> +#include <linux/interrupt.h>
> +#include <linux/kthread.h>
> +#include <linux/freezer.h>

kthead.h, freezer.h headerfile is used in this file?

> +#include <linux/platform_device.h>
> +#include <linux/extcon.h>
> +#include <linux/err.h>
> +#include <linux/of.h>
> +#include <linux/gpio.h>
> +#include <linux/of_gpio.h>
> +#include <linux/of_platform.h>

Order headerfile alphabetically.

> +
> +struct gpio_usbvid {
> +	struct device *dev;
> +
> +	struct extcon_dev edev;
> +
> +	/*GPIO pin */

I commented previous your patch about this wrong coding style.
Why did not you fix this coding style?

> +	int id_gpio;
> +	int vbus_gpio;
> +
> +	int id_irq;
> +	int vbus_irq;
> +	int type;
> +};
> +
> +static const char *dra7xx_extcon_cable[] = {
> +	[0] = "USB",
> +	[1] = "USB-HOST",
> +	NULL,
> +};
> +
> +static const int mutually_exclusive[] = {0x3, 0x0};
> +
> +/* Two types of support are provided.
> + * Systems which has
> + *	1) VBUS and ID pin connected via GPIO
> + *	2)  only ID pin connected via GPIO

Remove blank between '2)' and 'only'.

> + *  For Case 1 both the gpios should be provided via DT
> + *  Always the first GPIO in dt is considered ID pin GPIO
> + */
> +
> +enum {
> +	UNKNOWN = 0,
> +	ID_DETECT,
> +	VBUS_ID_DETECT,
> +};
> +
> +#define ID_GND		0
> +#define ID_FLOAT	1
> +#define VBUS_OFF	0
> +#define VBUS_ON		1

I think you could only use two constant instead of four constant definition.

> +
> +

This blank line isn't necessary.

> +static irqreturn_t id_irq_handler(int irq, void *data)
> +{
> +	struct gpio_usbvid *gpio_usbvid = (struct gpio_usbvid *) data;

You should delete blank between ')' and 'data' as follwong: 
	- (struct gpio_usbvid *)data;

> +	int id_current;
> +
> +	id_current = gpio_get_value_cansleep(gpio_usbvid->id_gpio);
> +	if (id_current == ID_GND) {
> +		if (gpio_usbvid->type == ID_DETECT)
> +			extcon_set_cable_state(&gpio_usbvid->edev,
> +							"USB", false);
> +		extcon_set_cable_state(&gpio_usbvid->edev, "USB-HOST", true);

As else statement, you should set "USB-HOST" cable state to improve readability.

		extcon_set_cable_state(&gpio_usbvid->edev, "USB-HOST", true);
		if (gpio_usbvid->type == ID_DETECT)
			extcon_set_cable_state(&gpio_usbvid->edev,
							"USB", false);

> +	} else {
> +		extcon_set_cable_state(&gpio_usbvid->edev, "USB-HOST", false);
> +		if (gpio_usbvid->type == ID_DETECT)
> +			extcon_set_cable_state(&gpio_usbvid->edev,
> +							"USB", true);
> +	}

Add blank line.

> +	return IRQ_HANDLED;
> +}
> +
> +static irqreturn_t vbus_irq_handler(int irq, void *data)
> +{
> +	struct gpio_usbvid *gpio_usbvid = (struct gpio_usbvid *) data;

ditto.

> +	int vbus_current;
> +
> +	vbus_current = gpio_get_value_cansleep(gpio_usbvid->vbus_gpio);
> +	if (vbus_current == VBUS_OFF)
> +		extcon_set_cable_state(&gpio_usbvid->edev, "USB", false);
> +	else
> +		extcon_set_cable_state(&gpio_usbvid->edev, "USB", true);
> +
> +	return IRQ_HANDLED;
> +}
> +
> +

This blank line isn't necessary.
I commented unnecessary blank line on previous review.

> +static void gpio_usbvid_set_initial_state(struct gpio_usbvid *gpio_usbvid)
> +{
> +	int id_current;
> +	int vbus_current;

Define loacal variable on one line as following:
	int id_current, vbus_current;

> +
> +	switch (gpio_usbvid->type) {
> +	case ID_DETECT:
> +		id_current = gpio_get_value_cansleep(gpio_usbvid->id_gpio);
> +		if (!!id_current == ID_FLOAT) {
> +			extcon_set_cable_state(&gpio_usbvid->edev,
> +							"USB-HOST", false);
> +			extcon_set_cable_state(&gpio_usbvid->edev,
> +							"USB", true);
> +		} else {
> +			extcon_set_cable_state(&gpio_usbvid->edev,
> +							"USB", false);
> +			extcon_set_cable_state(&gpio_usbvid->edev,
> +							"USB-HOST", true);
> +		}
> +		break;
> +
> +	case VBUS_ID_DETECT:
> +		id_current = gpio_get_value_cansleep(gpio_usbvid->id_gpio);
> +		if (!!id_current == ID_FLOAT)
> +			extcon_set_cable_state(&gpio_usbvid->edev,
> +							"USB-HOST", false);
> +		else
> +			extcon_set_cable_state(&gpio_usbvid->edev,
> +							"USB-HOST", true);
> +
> +		vbus_current = gpio_get_value_cansleep(gpio_usbvid->vbus_gpio);
> +		if (!!vbus_current == VBUS_ON)
> +			extcon_set_cable_state(&gpio_usbvid->edev,
> +							"USB", true);
> +		else
> +			extcon_set_cable_state(&gpio_usbvid->edev,
> +							"USB", false);
> +		break;
> +
> +	default:
> +		dev_err(gpio_usbvid->dev, "Unknown VBUS-ID type\n");
> +	}
> +}
> +
> +static int gpio_usbvid_request_irq(struct gpio_usbvid *gpio_usbvid)
> +{
> +	int ret;

Add blank line.

> +	ret = devm_request_threaded_irq(gpio_usbvid->dev, gpio_usbvid->id_irq,
> +					NULL, id_irq_handler,
> +					IRQF_ONESHOT | IRQF_TRIGGER_FALLING,
> +					dev_name(gpio_usbvid->dev),
> +					(void *) gpio_usbvid);
> +	if (ret) {
> +		dev_err(gpio_usbvid->dev, "failed to request id irq #%d\n",
> +					gpio_usbvid->id_irq);
> +		return ret;
> +	}
> +	if (gpio_usbvid->type == VBUS_ID_DETECT) {
> +		ret = devm_request_threaded_irq(gpio_usbvid->dev,
> +					gpio_usbvid->vbus_irq, NULL,
> +					vbus_irq_handler,
> +					IRQF_ONESHOT | IRQF_TRIGGER_FALLING,
> +					dev_name(gpio_usbvid->dev),

Why do you use the same interrupt name both gpio_usbvid->id_irq and gpio_usbvid->vbus_irq?
You should use characteristic interrupt name.

> +					(void *) gpio_usbvid);
> +		if (ret)
> +			dev_err(gpio_usbvid->dev, "failed to request vbus irq #%d\n",
> +						gpio_usbvid->vbus_irq);
> +	}

Add blank line.

> +	return ret;
> +}
> +
> +static int gpio_usbvid_probe(struct platform_device *pdev)
> +{
> +	struct device_node *node = pdev->dev.of_node;
> +	struct gpio_usbvid *gpio_usbvid;
> +	int ret;
> +	int gpio;

Define loacal variable on one line as following:
	int ret, gpio;

> +
> +	gpio_usbvid = devm_kzalloc(&pdev->dev, sizeof(*gpio_usbvid),
> +								GFP_KERNEL);

If this statement over 80 line, you have to keep proper indentation as following:
	gpio_usbvid = devm_kzalloc(&pdev->dev, sizeof(*gpio_usbvid),
				  GFP_KERNEL);

> +	if (!gpio_usbvid)
> +		return -ENOMEM;
> +
> +

Remove blank line.

> +	gpio_usbvid->dev	 = &pdev->dev;

Use space instead of tab.

> +
> +	platform_set_drvdata(pdev, gpio_usbvid);
> +
> +	gpio_usbvid->edev.name = dev_name(&pdev->dev);

I add as follwong comment about your v1 patchset

	"If edev name is equal with device name, this line is unnecessary.
	Because extcon_dev_register() use dev_name(&pdev->dev) as edev name
	in extcon-class.c"

Why did not apply for my comment to v3 patchset?
Plesae pay attention for previous comment.

> +	gpio_usbvid->edev.supported_cable = dra7xx_extcon_cable;
> +	gpio_usbvid->edev.mutually_exclusive = mutually_exclusive;
> +
> +	if (of_device_is_compatible(node, "ti,gpio-usb-id"))
> +		gpio_usbvid->type = ID_DETECT;
> +
> +	gpio = of_get_gpio(node, 0);
> +	if (gpio_is_valid(gpio)) {
> +		gpio_usbvid->id_gpio = gpio;
> +		ret = devm_gpio_request(&pdev->dev, gpio_usbvid->id_gpio,
> +					"id_gpio");
> +		if (ret)
> +			return ret;

Add blank line.

> +		gpio_usbvid->id_irq = gpio_to_irq(gpio_usbvid->id_gpio);
> +	} else {
> +		dev_err(&pdev->dev, "failed to get id gpio\n");
> +		return -ENODEV;
> +	}
> +
> +	if (of_device_is_compatible(node, "ti,gpio-usb-vid")) {
> +		gpio_usbvid->type = VBUS_ID_DETECT;
> +		gpio = of_get_gpio(node, 1);
> +		if (gpio_is_valid(gpio)) {
> +			gpio_usbvid->vbus_gpio = gpio;
> +			ret = devm_gpio_request(&pdev->dev,
> +						gpio_usbvid->vbus_gpio,
> +						"vbus_gpio");
> +			if (ret)
> +				return ret;

Add blank line.

> +			gpio_usbvid->vbus_irq =
> +					gpio_to_irq(gpio_usbvid->vbus_gpio);
> +		} else {
> +			dev_err(&pdev->dev, "failed to get vbus gpio\n");
> +			return -ENODEV;
> +		}
> +	}
> +
> +	ret = extcon_dev_register(&gpio_usbvid->edev, gpio_usbvid->dev);
> +	if (ret) {
> +		dev_err(&pdev->dev, "failed to register extcon device\n");
> +		return ret;
> +	}
> +
> +	gpio_usbvid_set_initial_state(gpio_usbvid);
> +	ret = gpio_usbvid_request_irq(gpio_usbvid);

You should move gpio_usbvid_request_irq() call before extcon_dev_register().

> +	if (ret)
> +		goto err0;

? As following previous comment about v1 patchset:
	I need correct meaning name as err_thread or etc ...

> +
> +	return 0;
> +
> +err0:

ditto.

> +	extcon_dev_unregister(&gpio_usbvid->edev);
> +
> +	return ret;
> +}
> +
> +static int gpio_usbvid_remove(struct platform_device *pdev)
> +{
> +	struct gpio_usbvid *gpio_usbvid = platform_get_drvdata(pdev);
> +
> +	extcon_dev_unregister(&gpio_usbvid->edev);
> +	return 0;
> +}
> +
> +static struct of_device_id of_gpio_usbvid_match_tbl[] = {
> +	{ .compatible = "ti,gpio-usb-vid", },
> +	{ .compatible = "ti,gpio-usb-id", },
> +	{ /* end */ }
> +};
> +
> +static struct platform_driver gpio_usbvid_driver = {
> +	.probe = gpio_usbvid_probe,
> +	.remove = gpio_usbvid_remove,
> +	.driver = {
> +		.name = "gpio-usbvid",
> +		.of_match_table = of_gpio_usbvid_match_tbl,
> +		.owner = THIS_MODULE,
> +	},
> +};
> +
> +module_platform_driver(gpio_usbvid_driver);
> +
> +MODULE_ALIAS("platform:gpio-usbvid");
> +MODULE_AUTHOR("George Cherian <george.cherian@...com>");
> +MODULE_DESCRIPTION("GPIO based USB Connector driver");
> +MODULE_LICENSE("GPL");
> +MODULE_DEVICE_TABLE(of, of_gpio_usbvid_match_tbl);
> 

Cheers,
Chanwoo Choi
--
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