| Message ID | 14631865.01KLlU2iKL@wasted.cogentembedded.com |
|---|---|
| State | Needs Review / ACK, archived |
| Headers | show |
| Context | Check | Description |
|---|---|---|
| robh/checkpatch | warning | total: 1 errors, 0 warnings, 0 lines checked |
| robh/patch-applied | success |
Hi Sergei, I reviewed this patch quickly. This driver has just two gpio without any registers for IP. I wonder whether MAX3355E is separate chip to detect external connector or just use gpio of AP. Could you send the datasheet of MAX3355E to me? Thanks, Chanwoo Choi On 12/11/2014 08:28 AM, Sergei Shtylyov wrote: > MAX3355E chip integrates a charge pump and comparators to enable a system with > an integrated USB OTG dual-role transceiver to function as a USB OTG dual-role > device. In addition to sensing/controlling Vbus, the chip also passes thru the > ID signal from the USB OTG connector. On some Renesas boards, this signal is > just fed into the SoC thru a GPIO pin -- there's no real OTG controller, only > host and gadget USB controllers sharing the same USB bus; however, we'd like > to allow host or gadget drivers to be loaded depending on the cable type, hence > the need for the MAX3355 extcon driver. The Vbus status signals are also wired > to GPIOs (however, we're not currently intested in them), the OFFVBUS# signal > is controlled by the host controllers, there's also the SHDN# signal wired to > GPIO, which should be high for normal operation. > > Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com> > > --- > The patch is against the 'extcon-next' branch of the 'extcon.git' repo. > > Documentation/devicetree/bindings/extcon/extcon-max3355.txt | 21 ++ > drivers/extcon/Kconfig | 6 > drivers/extcon/Makefile | 1 > drivers/extcon/extcon-max3355.c | 122 ++++++++++++ > 4 files changed, 150 insertions(+) > > Index: extcon/Documentation/devicetree/bindings/extcon/extcon-max3355.txt > =================================================================== > --- /dev/null > +++ extcon/Documentation/devicetree/bindings/extcon/extcon-max3355.txt > @@ -0,0 +1,21 @@ > +MAX3355 USB OTG chip > +-------------------- > + > +MAX3355 integrates a charge pump and comparators to enable a system with an > +integrated USB OTG dual-role transceiver to function as a USB OTG dual-role > +device. > + > +Required properties: > +- compatible: should be "maxim,max3355"; > +- maxim,shdn-gpio: should contain a phandle and GPIO specifier for the GPIO pin > + connected to the MAX3355's SHDN# pin; > +- maxim,id-gpio: should contain a phandle and GPIO specifier for the GPIO pin > + connected to the MAX3355's ID_OUT pin. > + > +Example (Koelsch board): > + > + usb-otg { > + compatible = "maxim,max3355"; > + maxim,shdn-gpio = <&gpio2 4 GPIO_ACTIVE_LOW>; > + maxim,id-gpio = <&gpio5 31 GPIO_ACTIVE_HIGH>; > + }; > Index: extcon/drivers/extcon/Kconfig > =================================================================== > --- extcon.orig/drivers/extcon/Kconfig > +++ extcon/drivers/extcon/Kconfig > @@ -45,6 +45,12 @@ config EXTCON_MAX14577 > Maxim MAX14577/77836. The MAX14577/77836 MUIC is a USB port accessory > detector and switch. > > +config EXTCON_MAX3355 > + tristate "MAX3355 USB OTG EXTCON Support" > + help > + Say Y here to enable support for the USB host detection by MAX3355 > + OTG USB chip. > + > config EXTCON_MAX77693 > tristate "MAX77693 EXTCON Support" > depends on MFD_MAX77693 && INPUT > Index: extcon/drivers/extcon/Makefile > =================================================================== > --- extcon.orig/drivers/extcon/Makefile > +++ extcon/drivers/extcon/Makefile > @@ -7,6 +7,7 @@ obj-$(CONFIG_EXTCON_ADC_JACK) += extcon- > obj-$(CONFIG_EXTCON_ARIZONA) += extcon-arizona.o > obj-$(CONFIG_EXTCON_GPIO) += extcon-gpio.o > obj-$(CONFIG_EXTCON_MAX14577) += extcon-max14577.o > +obj-$(CONFIG_EXTCON_MAX3355) += extcon-max3355.o > obj-$(CONFIG_EXTCON_MAX77693) += extcon-max77693.o > obj-$(CONFIG_EXTCON_MAX8997) += extcon-max8997.o > obj-$(CONFIG_EXTCON_PALMAS) += extcon-palmas.o > Index: extcon/drivers/extcon/extcon-max3355.c > =================================================================== > --- /dev/null > +++ extcon/drivers/extcon/extcon-max3355.c > @@ -0,0 +1,122 @@ > +/* > + * MAX3355 USB OTG chip extcon driver > + * > + * Copyright (C) 2014 Cogent Embedded, Inc. > + * Author: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com> > + * > + * This software is licensed under the terms of the GNU General Public > + * License version 2, as published by the Free Software Foundation, and > + * may be copied, distributed, and modified under those terms. > + */ > + > +#include <linux/extcon.h> > +#include <linux/gpio.h> > +#include <linux/init.h> > +#include <linux/interrupt.h> > +#include <linux/kernel.h> > +#include <linux/module.h> > +#include <linux/of_gpio.h> > +#include <linux/platform_device.h> > +#include <linux/slab.h> > +#include <linux/workqueue.h> > + > +struct max3355_data { > + struct extcon_dev *edev; > + int id_gpio; > +}; > + > +static const char *max3355_cable[] = { > + [0] = "USB-HOST", > + NULL, > +}; > + > +static irqreturn_t max3355_id_irq(int irq, void *dev_id) > +{ > + struct max3355_data *data = dev_id; > + int id = gpio_get_value(data->id_gpio); > + > + extcon_set_cable_state(data->edev, "USB-HOST", !id); > + > + return IRQ_HANDLED; > +} > + > +static int max3355_probe(struct platform_device *pdev) > +{ > + struct device_node *np = pdev->dev.of_node; > + struct max3355_data *data; > + int gpio, irq, ret; > + > + data = devm_kzalloc(&pdev->dev, sizeof(struct max3355_data), > + GFP_KERNEL); > + if (!data) > + return -ENOMEM; > + > + data->edev = devm_extcon_dev_allocate(&pdev->dev, max3355_cable); > + if (IS_ERR(data->edev)) { > + dev_err(&pdev->dev, "failed to allocate extcon device\n"); > + return PTR_ERR(data->edev); > + } > + data->edev->name = kstrdup(np->name, GFP_KERNEL); > + > + gpio = of_get_named_gpio(np, "maxim,id-gpio", 0); > + if (gpio < 0) > + return gpio; > + data->id_gpio = gpio; > + > + gpio = of_get_named_gpio(np, "maxim,shdn-gpio", 0); > + if (gpio < 0) > + return gpio; > + > + ret = devm_gpio_request_one(&pdev->dev, gpio, GPIOF_DIR_OUT | > + GPIOF_INIT_HIGH, pdev->name); > + if (ret < 0) > + return ret; > + > + ret = devm_gpio_request_one(&pdev->dev, data->id_gpio, GPIOF_DIR_IN, > + pdev->name); > + if (ret < 0) > + return ret; > + > + ret = devm_extcon_dev_register(&pdev->dev, data->edev); > + if (ret < 0) > + return ret; > + > + irq = gpio_to_irq(data->id_gpio); > + if (irq < 0) > + return irq; > + > + ret = devm_request_threaded_irq(&pdev->dev, irq, NULL, max3355_id_irq, > + IRQF_TRIGGER_RISING | > + IRQF_TRIGGER_FALLING | IRQF_ONESHOT, > + pdev->name, data); > + if (ret < 0) > + return ret; > + > + platform_set_drvdata(pdev, data); > + > + /* Perform initial detection */ > + max3355_id_irq(irq, data); > + > + return 0; > +} > + > +static const struct of_device_id max3355_match_table[] = { > + { .compatible = "maxim,max3355", }, > + { } > +}; > +MODULE_DEVICE_TABLE(of, max3355_match_table); > + > +static struct platform_driver max3355_driver = { > + .probe = max3355_probe, > + .driver = { > + .name = "extcon-max3355", > + .of_match_table = max3355_match_table, > + .owner = THIS_MODULE, > + }, > +}; > + > +module_platform_driver(max3355_driver); > + > +MODULE_AUTHOR("Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>"); > +MODULE_DESCRIPTION("MAX3355 extcon driver"); > +MODULE_LICENSE("GPL v2"); > > -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Dec 11, 2014 at 2:46 AM, Chanwoo Choi <cw00.choi@samsung.com> wrote:
> Could you send the datasheet of MAX3355E to me?
First Google hit:
http://datasheets.maximintegrated.com/en/ds/MAX3355E.pdf
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Hello. On 12/11/2014 4:46 AM, Chanwoo Choi wrote: > I reviewed this patch quickly. This driver has just two gpio without any registers for IP. Because there are no registers. > I wonder whether MAX3355E is separate chip to detect external connector Yes. I thought my description has made that clear... > or just use gpio of AP. What do you mean by AP? MAX3355 is indeed partly connected to GPIOs and partly to the USB pins. > Could you send the datasheet of MAX3355E to me? Looks like Geert has beaten me to it. :-) > Thanks, > Chanwoo Choi WBR, Sergei -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 12/11/2014 09:47 PM, Sergei Shtylyov wrote: > Hello. > > On 12/11/2014 4:46 AM, Chanwoo Choi wrote: > >> I reviewed this patch quickly. This driver has just two gpio without any registers for IP. > > Because there are no registers. > >> I wonder whether MAX3355E is separate chip to detect external connector > > Yes. I thought my description has made that clear... > >> or just use gpio of AP. > > What do you mean by AP? > MAX3355 is indeed partly connected to GPIOs and partly to the USB pins. > >> Could you send the datasheet of MAX3355E to me? > > Looks like Geert has beaten me to it. :-) > >> Thanks, >> Chanwoo Choi > OK, I'll review it. Thanks, Chanwoo Choi -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 12/11/2014 08:28 AM, Sergei Shtylyov wrote: > MAX3355E chip integrates a charge pump and comparators to enable a system with > an integrated USB OTG dual-role transceiver to function as a USB OTG dual-role > device. In addition to sensing/controlling Vbus, the chip also passes thru the > ID signal from the USB OTG connector. On some Renesas boards, this signal is > just fed into the SoC thru a GPIO pin -- there's no real OTG controller, only > host and gadget USB controllers sharing the same USB bus; however, we'd like > to allow host or gadget drivers to be loaded depending on the cable type, hence > the need for the MAX3355 extcon driver. The Vbus status signals are also wired > to GPIOs (however, we're not currently intested in them), the OFFVBUS# signal > is controlled by the host controllers, there's also the SHDN# signal wired to > GPIO, which should be high for normal operation. > > Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com> > > --- > The patch is against the 'extcon-next' branch of the 'extcon.git' repo. > > Documentation/devicetree/bindings/extcon/extcon-max3355.txt | 21 ++ > drivers/extcon/Kconfig | 6 > drivers/extcon/Makefile | 1 > drivers/extcon/extcon-max3355.c | 122 ++++++++++++ > 4 files changed, 150 insertions(+) > > Index: extcon/Documentation/devicetree/bindings/extcon/extcon-max3355.txt > =================================================================== > --- /dev/null > +++ extcon/Documentation/devicetree/bindings/extcon/extcon-max3355.txt > @@ -0,0 +1,21 @@ > +MAX3355 USB OTG chip Need manufactor information as following : -> Maxim MAX3355 > +-------------------- > + > +MAX3355 integrates a charge pump and comparators to enable a system with an > +integrated USB OTG dual-role transceiver to function as a USB OTG dual-role > +device. > + > +Required properties: > +- compatible: should be "maxim,max3355"; > +- maxim,shdn-gpio: should contain a phandle and GPIO specifier for the GPIO pin > + connected to the MAX3355's SHDN# pin; > +- maxim,id-gpio: should contain a phandle and GPIO specifier for the GPIO pin > + connected to the MAX3355's ID_OUT pin. > + > +Example (Koelsch board): > + > + usb-otg { > + compatible = "maxim,max3355"; > + maxim,shdn-gpio = <&gpio2 4 GPIO_ACTIVE_LOW>; > + maxim,id-gpio = <&gpio5 31 GPIO_ACTIVE_HIGH>; Kernel already supported the gpio helper function to get gpio from devicetree. I prefer use follwoing style by using of_get_gpio()/of_get_gpio_flags() in include/linux/of_gpio.h. gpios = <&gpio2 4 GPIO_ACTIVE_LOW>, <&gpio5 31 GPIO_ACTIVE_HIGH>; > + }; > Index: extcon/drivers/extcon/Kconfig > =================================================================== > --- extcon.orig/drivers/extcon/Kconfig > +++ extcon/drivers/extcon/Kconfig > @@ -45,6 +45,12 @@ config EXTCON_MAX14577 > Maxim MAX14577/77836. The MAX14577/77836 MUIC is a USB port accessory > detector and switch. > > +config EXTCON_MAX3355 > + tristate "MAX3355 USB OTG EXTCON Support" > + help > + Say Y here to enable support for the USB host detection by MAX3355 > + OTG USB chip. > + > config EXTCON_MAX77693 > tristate "MAX77693 EXTCON Support" > depends on MFD_MAX77693 && INPUT > Index: extcon/drivers/extcon/Makefile > =================================================================== > --- extcon.orig/drivers/extcon/Makefile > +++ extcon/drivers/extcon/Makefile > @@ -7,6 +7,7 @@ obj-$(CONFIG_EXTCON_ADC_JACK) += extcon- > obj-$(CONFIG_EXTCON_ARIZONA) += extcon-arizona.o > obj-$(CONFIG_EXTCON_GPIO) += extcon-gpio.o > obj-$(CONFIG_EXTCON_MAX14577) += extcon-max14577.o > +obj-$(CONFIG_EXTCON_MAX3355) += extcon-max3355.o > obj-$(CONFIG_EXTCON_MAX77693) += extcon-max77693.o > obj-$(CONFIG_EXTCON_MAX8997) += extcon-max8997.o > obj-$(CONFIG_EXTCON_PALMAS) += extcon-palmas.o > Index: extcon/drivers/extcon/extcon-max3355.c > =================================================================== > --- /dev/null > +++ extcon/drivers/extcon/extcon-max3355.c > @@ -0,0 +1,122 @@ > +/* > + * MAX3355 USB OTG chip extcon driver ditto. > + * > + * Copyright (C) 2014 Cogent Embedded, Inc. > + * Author: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com> > + * > + * This software is licensed under the terms of the GNU General Public > + * License version 2, as published by the Free Software Foundation, and > + * may be copied, distributed, and modified under those terms. > + */ > + > +#include <linux/extcon.h> > +#include <linux/gpio.h> > +#include <linux/init.h> > +#include <linux/interrupt.h> > +#include <linux/kernel.h> > +#include <linux/module.h> > +#include <linux/of_gpio.h> > +#include <linux/platform_device.h> > +#include <linux/slab.h> > +#include <linux/workqueue.h> > + > +struct max3355_data { > + struct extcon_dev *edev; > + int id_gpio; > +}; > + > +static const char *max3355_cable[] = { > + [0] = "USB-HOST", > + NULL, > +}; > + > +static irqreturn_t max3355_id_irq(int irq, void *dev_id) > +{ > + struct max3355_data *data = dev_id; > + int id = gpio_get_value(data->id_gpio); > + > + extcon_set_cable_state(data->edev, "USB-HOST", !id); You have to get the gpio flag in the devicetree by using of_get_gpio_flags() function and then you would set the cable state according to gpio state and flag. > + > + return IRQ_HANDLED; > +} > + > +static int max3355_probe(struct platform_device *pdev) > +{ > + struct device_node *np = pdev->dev.of_node; > + struct max3355_data *data; > + int gpio, irq, ret; > + > + data = devm_kzalloc(&pdev->dev, sizeof(struct max3355_data), > + GFP_KERNEL); > + if (!data) > + return -ENOMEM; > + > + data->edev = devm_extcon_dev_allocate(&pdev->dev, max3355_cable); > + if (IS_ERR(data->edev)) { > + dev_err(&pdev->dev, "failed to allocate extcon device\n"); > + return PTR_ERR(data->edev); > + } > + data->edev->name = kstrdup(np->name, GFP_KERNEL); > + > + gpio = of_get_named_gpio(np, "maxim,id-gpio", 0); Use of_get_gpio() or of_get_gpio_flags(). "maxim,id-gpio" property has strong dependency on this driver and it is not standard. > + if (gpio < 0) > + return gpio; Need to add error message. > + data->id_gpio = gpio; > + > + gpio = of_get_named_gpio(np, "maxim,shdn-gpio", 0); ditto. > + if (gpio < 0) > + return gpio; Need to add error message. > + > + ret = devm_gpio_request_one(&pdev->dev, gpio, GPIOF_DIR_OUT | > + GPIOF_INIT_HIGH, pdev->name); > + if (ret < 0) > + return ret; Need to add error message. > + > + ret = devm_gpio_request_one(&pdev->dev, data->id_gpio, GPIOF_DIR_IN, > + pdev->name); Why do you use same name for two gpio when devm_gpio_request_one? I prefer to use other name because two gpio is different. > + if (ret < 0) > + return ret; > + > + ret = devm_extcon_dev_register(&pdev->dev, data->edev); > + if (ret < 0) > + return ret; Need to add error message. > + > + irq = gpio_to_irq(data->id_gpio); > + if (irq < 0) > + return irq; Need to add error message. > + > + ret = devm_request_threaded_irq(&pdev->dev, irq, NULL, max3355_id_irq, > + IRQF_TRIGGER_RISING | > + IRQF_TRIGGER_FALLING | IRQF_ONESHOT, Is it right to add both RISING and FALLING trigger? and need to add IRQF_NO_SUSPEND to wakeup from suspend state when interrupt happen. > + pdev->name, data); > + if (ret < 0) > + return ret; Need to add error message. > + > + platform_set_drvdata(pdev, data); > + > + /* Perform initial detection */ > + max3355_id_irq(irq, data); > + > + return 0; > +} > + > +static const struct of_device_id max3355_match_table[] = { > + { .compatible = "maxim,max3355", }, > + { } > +}; > +MODULE_DEVICE_TABLE(of, max3355_match_table); > + > +static struct platform_driver max3355_driver = { > + .probe = max3355_probe, > + .driver = { > + .name = "extcon-max3355", > + .of_match_table = max3355_match_table, > + .owner = THIS_MODULE, > + }, > +}; > + > +module_platform_driver(max3355_driver); > + Don't need un-used blank line. > +MODULE_AUTHOR("Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>"); > +MODULE_DESCRIPTION("MAX3355 extcon driver"); Maxim MAX3355. > +MODULE_LICENSE("GPL v2"); > > Thanks, Chanwoo Choi -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hello. On 12/17/2014 03:31 AM, Chanwoo Choi wrote: >> MAX3355E chip integrates a charge pump and comparators to enable a system with >> an integrated USB OTG dual-role transceiver to function as a USB OTG dual-role >> device. In addition to sensing/controlling Vbus, the chip also passes thru the >> ID signal from the USB OTG connector. On some Renesas boards, this signal is >> just fed into the SoC thru a GPIO pin -- there's no real OTG controller, only >> host and gadget USB controllers sharing the same USB bus; however, we'd like >> to allow host or gadget drivers to be loaded depending on the cable type, hence >> the need for the MAX3355 extcon driver. The Vbus status signals are also wired >> to GPIOs (however, we're not currently intested in them), the OFFVBUS# signal >> is controlled by the host controllers, there's also the SHDN# signal wired to >> GPIO, which should be high for normal operation. >> Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com> >> --- >> The patch is against the 'extcon-next' branch of the 'extcon.git' repo. >> Documentation/devicetree/bindings/extcon/extcon-max3355.txt | 21 ++ >> drivers/extcon/Kconfig | 6 >> drivers/extcon/Makefile | 1 >> drivers/extcon/extcon-max3355.c | 122 ++++++++++++ >> 4 files changed, 150 insertions(+) >> Index: extcon/Documentation/devicetree/bindings/extcon/extcon-max3355.txt >> =================================================================== >> --- /dev/null >> +++ extcon/Documentation/devicetree/bindings/extcon/extcon-max3355.txt >> @@ -0,0 +1,21 @@ >> +MAX3355 USB OTG chip > Need manufactor information as following : > -> Maxim MAX3355 Would be Maxim enough? Or perhaps I should use Maxim Integrated [Products]? >> +-------------------- >> + >> +MAX3355 integrates a charge pump and comparators to enable a system with an >> +integrated USB OTG dual-role transceiver to function as a USB OTG dual-role >> +device. >> + >> +Required properties: >> +- compatible: should be "maxim,max3355"; >> +- maxim,shdn-gpio: should contain a phandle and GPIO specifier for the GPIO pin >> + connected to the MAX3355's SHDN# pin; >> +- maxim,id-gpio: should contain a phandle and GPIO specifier for the GPIO pin >> + connected to the MAX3355's ID_OUT pin. >> + >> +Example (Koelsch board): >> + >> + usb-otg { >> + compatible = "maxim,max3355"; >> + maxim,shdn-gpio = <&gpio2 4 GPIO_ACTIVE_LOW>; >> + maxim,id-gpio = <&gpio5 31 GPIO_ACTIVE_HIGH>; > Kernel already supported the gpio helper function to get gpio from devicetree. > I prefer use follwoing style by using of_get_gpio()/of_get_gpio_flags() in include/linux/of_gpio.h. > gpios = <&gpio2 4 GPIO_ACTIVE_LOW>, <&gpio5 31 GPIO_ACTIVE_HIGH>; OK, though Documentation/devicetree/bindings/gpio/gpio.txt does not seem to insist on using this one... [...] >> Index: extcon/drivers/extcon/extcon-max3355.c >> =================================================================== >> --- /dev/null >> +++ extcon/drivers/extcon/extcon-max3355.c >> @@ -0,0 +1,122 @@ >> +/* >> + * MAX3355 USB OTG chip extcon driver > ditto. OK, though the other Maxim extcon drivers don't serve as a good example here... [...] >> +static irqreturn_t max3355_id_irq(int irq, void *dev_id) >> +{ >> + struct max3355_data *data = dev_id; >> + int id = gpio_get_value(data->id_gpio); >> + >> + extcon_set_cable_state(data->edev, "USB-HOST", !id); > You have to get the gpio flag in the devicetree by using of_get_gpio_flags() function > and then you would set the cable state according to gpio state and flag. I'm sorry but I just don't see why I have to do it. This is not a generic GPIO driver, and the polarities of the GPIOs are determined solely by the MAX3355 specs. [...] >> +static int max3355_probe(struct platform_device *pdev) >> +{ >> + struct device_node *np = pdev->dev.of_node; >> + struct max3355_data *data; >> + int gpio, irq, ret; >> + >> + data = devm_kzalloc(&pdev->dev, sizeof(struct max3355_data), >> + GFP_KERNEL); >> + if (!data) >> + return -ENOMEM; >> + >> + data->edev = devm_extcon_dev_allocate(&pdev->dev, max3355_cable); >> + if (IS_ERR(data->edev)) { >> + dev_err(&pdev->dev, "failed to allocate extcon device\n"); >> + return PTR_ERR(data->edev); >> + } >> + data->edev->name = kstrdup(np->name, GFP_KERNEL); >> + >> + gpio = of_get_named_gpio(np, "maxim,id-gpio", 0); > Use of_get_gpio() or of_get_gpio_flags(). OK, I'll use the first one. > "maxim,id-gpio" property has strong dependency on this driver and it is not standard. I indented it to be Maxim specific. Not sure what using the standard property buys... >> + if (gpio < 0) >> + return gpio; > Need to add error message. OK for all cases; extcon-gpio.c was probably a bad example... [...] >> + >> + ret = devm_gpio_request_one(&pdev->dev, gpio, GPIOF_DIR_OUT | >> + GPIOF_INIT_HIGH, pdev->name); >> + if (ret < 0) >> + return ret; > Need to add error message. >> + >> + ret = devm_gpio_request_one(&pdev->dev, data->id_gpio, GPIOF_DIR_IN, >> + pdev->name); > Why do you use same name for two gpio when devm_gpio_request_one? > I prefer to use other name because two gpio is different. And you're absolutely right here. Mindlessly copy-pasted the code from extcon-gpio.c... [...] >> + >> + ret = devm_request_threaded_irq(&pdev->dev, irq, NULL, max3355_id_irq, >> + IRQF_TRIGGER_RISING | >> + IRQF_TRIGGER_FALLING | IRQF_ONESHOT, > Is it right to add both RISING and FALLING trigger? How else I'm supposed to know when the OTG ID signal goes low and high? > and need to add IRQF_NO_SUSPEND to wakeup from suspend state when interrupt happen. Hm, are you sure we want to wake up on plugging another kind of USB cable? [...] >> +static struct platform_driver max3355_driver = { >> + .probe = max3355_probe, >> + .driver = { >> + .name = "extcon-max3355", >> + .of_match_table = max3355_match_table, >> + .owner = THIS_MODULE, >> + }, >> +}; >> + >> +module_platform_driver(max3355_driver); >> + > Don't need un-used blank line. I don't understand. There's empty line in such case in extcon-gpio.c... >> +MODULE_AUTHOR("Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>"); >> +MODULE_DESCRIPTION("MAX3355 extcon driver"); > Maxim MAX3355. OK, seeing it in the other Maxim drivers. >> +MODULE_LICENSE("GPL v2"); > Thanks, > Chanwoo Choi WBR, Sergei -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hello. On 12/18/2014 12:58 AM, Sergei Shtylyov wrote: >>> MAX3355E chip integrates a charge pump and comparators to enable a system with >>> an integrated USB OTG dual-role transceiver to function as a USB OTG dual-role >>> device. In addition to sensing/controlling Vbus, the chip also passes thru the >>> ID signal from the USB OTG connector. On some Renesas boards, this signal is >>> just fed into the SoC thru a GPIO pin -- there's no real OTG controller, only >>> host and gadget USB controllers sharing the same USB bus; however, we'd like >>> to allow host or gadget drivers to be loaded depending on the cable type, >>> hence >>> the need for the MAX3355 extcon driver. The Vbus status signals are also wired >>> to GPIOs (however, we're not currently intested in them), the OFFVBUS# signal >>> is controlled by the host controllers, there's also the SHDN# signal wired to >>> GPIO, which should be high for normal operation. > >>> Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com> > >>> --- >>> The patch is against the 'extcon-next' branch of the 'extcon.git' repo. > >>> Documentation/devicetree/bindings/extcon/extcon-max3355.txt | 21 ++ >>> drivers/extcon/Kconfig | 6 >>> drivers/extcon/Makefile | 1 >>> drivers/extcon/extcon-max3355.c | 122 >>> ++++++++++++ >>> 4 files changed, 150 insertions(+) > >>> Index: extcon/Documentation/devicetree/bindings/extcon/extcon-max3355.txt >>> =================================================================== >>> --- /dev/null >>> +++ extcon/Documentation/devicetree/bindings/extcon/extcon-max3355.txt >>> @@ -0,0 +1,21 @@ >>> +MAX3355 USB OTG chip > >> Need manufactor information as following : >> -> Maxim MAX3355 > > Would be Maxim enough? Or perhaps I should use Maxim Integrated [Products]? You haven't replied to my questions. >>> +-------------------- >>> + >>> +MAX3355 integrates a charge pump and comparators to enable a system with an >>> +integrated USB OTG dual-role transceiver to function as a USB OTG dual-role >>> +device. >>> + >>> +Required properties: >>> +- compatible: should be "maxim,max3355"; >>> +- maxim,shdn-gpio: should contain a phandle and GPIO specifier for the >>> GPIO pin >>> + connected to the MAX3355's SHDN# pin; >>> +- maxim,id-gpio: should contain a phandle and GPIO specifier for the GPIO pin >>> + connected to the MAX3355's ID_OUT pin. >>> + >>> +Example (Koelsch board): >>> + >>> + usb-otg { >>> + compatible = "maxim,max3355"; >>> + maxim,shdn-gpio = <&gpio2 4 GPIO_ACTIVE_LOW>; >>> + maxim,id-gpio = <&gpio5 31 GPIO_ACTIVE_HIGH>; > >> Kernel already supported the gpio helper function to get gpio from devicetree. >> I prefer use follwoing style by using of_get_gpio()/of_get_gpio_flags() in >> include/linux/of_gpio.h. > >> gpios = <&gpio2 4 GPIO_ACTIVE_LOW>, <&gpio5 31 GPIO_ACTIVE_HIGH>; > > OK, though Documentation/devicetree/bindings/gpio/gpio.txt does not seem > to insist on using this one... Moreover, it now says "gpios" isn't allowed for the new bindings. So I have to strongly disagree here... [...] >>> +static irqreturn_t max3355_id_irq(int irq, void *dev_id) >>> +{ >>> + struct max3355_data *data = dev_id; >>> + int id = gpio_get_value(data->id_gpio); >>> + >>> + extcon_set_cable_state(data->edev, "USB-HOST", !id); > >> You have to get the gpio flag in the devicetree by using of_get_gpio_flags() >> function >> and then you would set the cable state according to gpio state and flag. > > I'm sorry but I just don't see why I have to do it. This is not a generic > GPIO driver, and the polarities of the GPIOs are determined solely by the > MAX3355 specs. Again, got not reply... > [...] >>> +static int max3355_probe(struct platform_device *pdev) >>> +{ >>> + struct device_node *np = pdev->dev.of_node; >>> + struct max3355_data *data; >>> + int gpio, irq, ret; >>> + >>> + data = devm_kzalloc(&pdev->dev, sizeof(struct max3355_data), >>> + GFP_KERNEL); >>> + if (!data) >>> + return -ENOMEM; >>> + >>> + data->edev = devm_extcon_dev_allocate(&pdev->dev, max3355_cable); >>> + if (IS_ERR(data->edev)) { >>> + dev_err(&pdev->dev, "failed to allocate extcon device\n"); >>> + return PTR_ERR(data->edev); >>> + } >>> + data->edev->name = kstrdup(np->name, GFP_KERNEL); >>> + >>> + gpio = of_get_named_gpio(np, "maxim,id-gpio", 0); > >> Use of_get_gpio() or of_get_gpio_flags(). > > OK, I'll use the first one. No, I won't due to not being able to use "gpios" anymore. [...] >>> + >>> + ret = devm_request_threaded_irq(&pdev->dev, irq, NULL, max3355_id_irq, >>> + IRQF_TRIGGER_RISING | >>> + IRQF_TRIGGER_FALLING | IRQF_ONESHOT, > >> Is it right to add both RISING and FALLING trigger? > > How else I'm supposed to know when the OTG ID signal goes low and high? > >> and need to add IRQF_NO_SUSPEND to wakeup from suspend state when interrupt >> happen. > > Hm, are you sure we want to wake up on plugging another kind of USB cable? No reply to that as well... [...] >>> +static struct platform_driver max3355_driver = { >>> + .probe = max3355_probe, >>> + .driver = { >>> + .name = "extcon-max3355", >>> + .of_match_table = max3355_match_table, >>> + .owner = THIS_MODULE, >>> + }, >>> +}; >>> + >>> +module_platform_driver(max3355_driver); >>> + > >> Don't need un-used blank line. > > I don't understand. There's empty line in such case in extcon-gpio.c... No reply... [...] >> Thanks, >> Chanwoo Choi MBR, Sergei -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Sergei, I think this patch is too much delay. I recommend you better to develop this driver based on latest extcon-next branch[1]. [1] https://git.kernel.org/cgit/linux/kernel/git/chanwoo/extcon.git/log/?h=extcon-next Thanks, Chanwoo Choi On 2015년 10월 21일 03:20, Sergei Shtylyov wrote: > Hello. > > On 12/18/2014 12:58 AM, Sergei Shtylyov wrote: > >>>> MAX3355E chip integrates a charge pump and comparators to enable a system with >>>> an integrated USB OTG dual-role transceiver to function as a USB OTG dual-role >>>> device. In addition to sensing/controlling Vbus, the chip also passes thru the >>>> ID signal from the USB OTG connector. On some Renesas boards, this signal is >>>> just fed into the SoC thru a GPIO pin -- there's no real OTG controller, only >>>> host and gadget USB controllers sharing the same USB bus; however, we'd like >>>> to allow host or gadget drivers to be loaded depending on the cable type, >>>> hence >>>> the need for the MAX3355 extcon driver. The Vbus status signals are also wired >>>> to GPIOs (however, we're not currently intested in them), the OFFVBUS# signal >>>> is controlled by the host controllers, there's also the SHDN# signal wired to >>>> GPIO, which should be high for normal operation. >> >>>> Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com> >> >>>> --- >>>> The patch is against the 'extcon-next' branch of the 'extcon.git' repo. >> >>>> Documentation/devicetree/bindings/extcon/extcon-max3355.txt | 21 ++ >>>> drivers/extcon/Kconfig | 6 >>>> drivers/extcon/Makefile | 1 >>>> drivers/extcon/extcon-max3355.c | 122 >>>> ++++++++++++ >>>> 4 files changed, 150 insertions(+) >> >>>> Index: extcon/Documentation/devicetree/bindings/extcon/extcon-max3355.txt >>>> =================================================================== >>>> --- /dev/null >>>> +++ extcon/Documentation/devicetree/bindings/extcon/extcon-max3355.txt >>>> @@ -0,0 +1,21 @@ >>>> +MAX3355 USB OTG chip >> >>> Need manufactor information as following : >>> -> Maxim MAX3355 >> >> Would be Maxim enough? Or perhaps I should use Maxim Integrated [Products]? > > You haven't replied to my questions. > >>>> +-------------------- >>>> + >>>> +MAX3355 integrates a charge pump and comparators to enable a system with an >>>> +integrated USB OTG dual-role transceiver to function as a USB OTG dual-role >>>> +device. >>>> + >>>> +Required properties: >>>> +- compatible: should be "maxim,max3355"; >>>> +- maxim,shdn-gpio: should contain a phandle and GPIO specifier for the >>>> GPIO pin >>>> + connected to the MAX3355's SHDN# pin; >>>> +- maxim,id-gpio: should contain a phandle and GPIO specifier for the GPIO pin >>>> + connected to the MAX3355's ID_OUT pin. >>>> + >>>> +Example (Koelsch board): >>>> + >>>> + usb-otg { >>>> + compatible = "maxim,max3355"; >>>> + maxim,shdn-gpio = <&gpio2 4 GPIO_ACTIVE_LOW>; >>>> + maxim,id-gpio = <&gpio5 31 GPIO_ACTIVE_HIGH>; >> >>> Kernel already supported the gpio helper function to get gpio from devicetree. >>> I prefer use follwoing style by using of_get_gpio()/of_get_gpio_flags() in >>> include/linux/of_gpio.h. >> >>> gpios = <&gpio2 4 GPIO_ACTIVE_LOW>, <&gpio5 31 GPIO_ACTIVE_HIGH>; >> >> OK, though Documentation/devicetree/bindings/gpio/gpio.txt does not seem >> to insist on using this one... > > Moreover, it now says "gpios" isn't allowed for the new bindings. So I have to strongly disagree here... > > [...] > >>>> +static irqreturn_t max3355_id_irq(int irq, void *dev_id) >>>> +{ >>>> + struct max3355_data *data = dev_id; >>>> + int id = gpio_get_value(data->id_gpio); >>>> + >>>> + extcon_set_cable_state(data->edev, "USB-HOST", !id); >> >>> You have to get the gpio flag in the devicetree by using of_get_gpio_flags() >>> function >>> and then you would set the cable state according to gpio state and flag. >> >> I'm sorry but I just don't see why I have to do it. This is not a generic >> GPIO driver, and the polarities of the GPIOs are determined solely by the >> MAX3355 specs. > > Again, got not reply... > >> [...] >>>> +static int max3355_probe(struct platform_device *pdev) >>>> +{ >>>> + struct device_node *np = pdev->dev.of_node; >>>> + struct max3355_data *data; >>>> + int gpio, irq, ret; >>>> + >>>> + data = devm_kzalloc(&pdev->dev, sizeof(struct max3355_data), >>>> + GFP_KERNEL); >>>> + if (!data) >>>> + return -ENOMEM; >>>> + >>>> + data->edev = devm_extcon_dev_allocate(&pdev->dev, max3355_cable); >>>> + if (IS_ERR(data->edev)) { >>>> + dev_err(&pdev->dev, "failed to allocate extcon device\n"); >>>> + return PTR_ERR(data->edev); >>>> + } >>>> + data->edev->name = kstrdup(np->name, GFP_KERNEL); >>>> + >>>> + gpio = of_get_named_gpio(np, "maxim,id-gpio", 0); >> >>> Use of_get_gpio() or of_get_gpio_flags(). >> >> OK, I'll use the first one. > > No, I won't due to not being able to use "gpios" anymore. > > [...] > >>>> + >>>> + ret = devm_request_threaded_irq(&pdev->dev, irq, NULL, max3355_id_irq, >>>> + IRQF_TRIGGER_RISING | >>>> + IRQF_TRIGGER_FALLING | IRQF_ONESHOT, >> >>> Is it right to add both RISING and FALLING trigger? >> >> How else I'm supposed to know when the OTG ID signal goes low and high? >> >>> and need to add IRQF_NO_SUSPEND to wakeup from suspend state when interrupt >>> happen. >> >> Hm, are you sure we want to wake up on plugging another kind of USB cable? > > No reply to that as well... > > [...] >>>> +static struct platform_driver max3355_driver = { >>>> + .probe = max3355_probe, >>>> + .driver = { >>>> + .name = "extcon-max3355", >>>> + .of_match_table = max3355_match_table, >>>> + .owner = THIS_MODULE, >>>> + }, >>>> +}; >>>> + >>>> +module_platform_driver(max3355_driver); >>>> + >> >>> Don't need un-used blank line. >> >> I don't understand. There's empty line in such case in extcon-gpio.c... > > No reply... > > [...] > >>> Thanks, >>> Chanwoo Choi > > MBR, Sergei > > -- > 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/ > -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hello. On 10/21/2015 05:57 AM, Chanwoo Choi wrote: > I think this patch is too much delay. I recommend you better to develop > this driver based on latest extcon-next branch[1]. > [1] https://git.kernel.org/cgit/linux/kernel/git/chanwoo/extcon.git/log/?h=extcon-next I would really appreciate if you answer my questions, I think most of them are still valid... MBR, Sergei -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 2015년 10월 21일 03:20, Sergei Shtylyov wrote: > Hello. > > On 12/18/2014 12:58 AM, Sergei Shtylyov wrote: > >>>> MAX3355E chip integrates a charge pump and comparators to enable a system with >>>> an integrated USB OTG dual-role transceiver to function as a USB OTG dual-role >>>> device. In addition to sensing/controlling Vbus, the chip also passes thru the >>>> ID signal from the USB OTG connector. On some Renesas boards, this signal is >>>> just fed into the SoC thru a GPIO pin -- there's no real OTG controller, only >>>> host and gadget USB controllers sharing the same USB bus; however, we'd like >>>> to allow host or gadget drivers to be loaded depending on the cable type, >>>> hence >>>> the need for the MAX3355 extcon driver. The Vbus status signals are also wired >>>> to GPIOs (however, we're not currently intested in them), the OFFVBUS# signal >>>> is controlled by the host controllers, there's also the SHDN# signal wired to >>>> GPIO, which should be high for normal operation. >> >>>> Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com> >> >>>> --- >>>> The patch is against the 'extcon-next' branch of the 'extcon.git' repo. >> >>>> Documentation/devicetree/bindings/extcon/extcon-max3355.txt | 21 ++ >>>> drivers/extcon/Kconfig | 6 >>>> drivers/extcon/Makefile | 1 >>>> drivers/extcon/extcon-max3355.c | 122 >>>> ++++++++++++ >>>> 4 files changed, 150 insertions(+) >> >>>> Index: extcon/Documentation/devicetree/bindings/extcon/extcon-max3355.txt >>>> =================================================================== >>>> --- /dev/null >>>> +++ extcon/Documentation/devicetree/bindings/extcon/extcon-max3355.txt >>>> @@ -0,0 +1,21 @@ >>>> +MAX3355 USB OTG chip >> >>> Need manufactor information as following : >>> -> Maxim MAX3355 >> >> Would be Maxim enough? Or perhaps I should use Maxim Integrated [Products]? > > You haven't replied to my questions. > >>>> +-------------------- >>>> + >>>> +MAX3355 integrates a charge pump and comparators to enable a system with an >>>> +integrated USB OTG dual-role transceiver to function as a USB OTG dual-role >>>> +device. >>>> + >>>> +Required properties: >>>> +- compatible: should be "maxim,max3355"; >>>> +- maxim,shdn-gpio: should contain a phandle and GPIO specifier for the >>>> GPIO pin >>>> + connected to the MAX3355's SHDN# pin; >>>> +- maxim,id-gpio: should contain a phandle and GPIO specifier for the GPIO pin >>>> + connected to the MAX3355's ID_OUT pin. >>>> + >>>> +Example (Koelsch board): >>>> + >>>> + usb-otg { >>>> + compatible = "maxim,max3355"; >>>> + maxim,shdn-gpio = <&gpio2 4 GPIO_ACTIVE_LOW>; >>>> + maxim,id-gpio = <&gpio5 31 GPIO_ACTIVE_HIGH>; >> >>> Kernel already supported the gpio helper function to get gpio from devicetree. >>> I prefer use follwoing style by using of_get_gpio()/of_get_gpio_flags() in >>> include/linux/of_gpio.h. >> >>> gpios = <&gpio2 4 GPIO_ACTIVE_LOW>, <&gpio5 31 GPIO_ACTIVE_HIGH>; >> >> OK, though Documentation/devicetree/bindings/gpio/gpio.txt does not seem >> to insist on using this one... > > Moreover, it now says "gpios" isn't allowed for the new bindings. So I have to strongly disagree here... OK. But, I recommend you use the 'gpiod' with devm_gpiod_get instead of using devm_gpio_request_one() directly as following: You can refer drivers/extcon/extcon-usb-gpio.c about using gpiod. For example, data->shdn_gpiod = devm_gpiod_get(dev, "maxim,shdn", GPIOD_IN); data->id_gpiod = devm_gpiod_get(dev, "maxim,id", GPIOD_IN); > > [...] > >>>> +static irqreturn_t max3355_id_irq(int irq, void *dev_id) >>>> +{ >>>> + struct max3355_data *data = dev_id; >>>> + int id = gpio_get_value(data->id_gpio); >>>> + >>>> + extcon_set_cable_state(data->edev, "USB-HOST", !id); >> >>> You have to get the gpio flag in the devicetree by using of_get_gpio_flags() >>> function >>> and then you would set the cable state according to gpio state and flag. >> >> I'm sorry but I just don't see why I have to do it. This is not a generic >> GPIO driver, and the polarities of the GPIOs are determined solely by the >> MAX3355 specs. > > Again, got not reply... OK. you don't need to consider the gpio flag from devicetree. > >> [...] >>>> +static int max3355_probe(struct platform_device *pdev) >>>> +{ >>>> + struct device_node *np = pdev->dev.of_node; >>>> + struct max3355_data *data; >>>> + int gpio, irq, ret; >>>> + >>>> + data = devm_kzalloc(&pdev->dev, sizeof(struct max3355_data), >>>> + GFP_KERNEL); >>>> + if (!data) >>>> + return -ENOMEM; >>>> + >>>> + data->edev = devm_extcon_dev_allocate(&pdev->dev, max3355_cable); >>>> + if (IS_ERR(data->edev)) { >>>> + dev_err(&pdev->dev, "failed to allocate extcon device\n"); >>>> + return PTR_ERR(data->edev); >>>> + } >>>> + data->edev->name = kstrdup(np->name, GFP_KERNEL); >>>> + >>>> + gpio = of_get_named_gpio(np, "maxim,id-gpio", 0); >> >>> Use of_get_gpio() or of_get_gpio_flags(). >> >> OK, I'll use the first one. > > No, I won't due to not being able to use "gpios" anymore. OK. I add the comment on upper. You can use the gpiod API. > > [...] > >>>> + >>>> + ret = devm_request_threaded_irq(&pdev->dev, irq, NULL, max3355_id_irq, >>>> + IRQF_TRIGGER_RISING | >>>> + IRQF_TRIGGER_FALLING | IRQF_ONESHOT, >> >>> Is it right to add both RISING and FALLING trigger? >> >> How else I'm supposed to know when the OTG ID signal goes low and high? >> >>> and need to add IRQF_NO_SUSPEND to wakeup from suspend state when interrupt >>> happen. >> >> Hm, are you sure we want to wake up on plugging another kind of USB cable? > > No reply to that as well... I'm sure. If external connector (USB, Charger cable, Earjack etc..) is attached or detached in the suspended state, system should be wake-up from suspended state. If kernel don't handle the any interrupt in suspended state, after wakeup from suspended state, there is mismatch data between 'before entering suspend state' and 'after wakeup from suspend state'. > > [...] >>>> +static struct platform_driver max3355_driver = { >>>> + .probe = max3355_probe, >>>> + .driver = { >>>> + .name = "extcon-max3355", >>>> + .of_match_table = max3355_match_table, >>>> + .owner = THIS_MODULE, >>>> + }, >>>> +}; >>>> + >>>> +module_platform_driver(max3355_driver); >>>> + >> >>> Don't need un-used blank line. >> >> I don't understand. There's empty line in such case in extcon-gpio.c... > > No reply... OK. No problem. Ignore this comment. Thanks, Chanwoo Choi -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hello. On 10/23/2015 08:56 AM, Chanwoo Choi wrote: >>>>> MAX3355E chip integrates a charge pump and comparators to enable a system with >>>>> an integrated USB OTG dual-role transceiver to function as a USB OTG dual-role >>>>> device. In addition to sensing/controlling Vbus, the chip also passes thru the >>>>> ID signal from the USB OTG connector. On some Renesas boards, this signal is >>>>> just fed into the SoC thru a GPIO pin -- there's no real OTG controller, only >>>>> host and gadget USB controllers sharing the same USB bus; however, we'd like >>>>> to allow host or gadget drivers to be loaded depending on the cable type, >>>>> hence >>>>> the need for the MAX3355 extcon driver. The Vbus status signals are also wired >>>>> to GPIOs (however, we're not currently intested in them), the OFFVBUS# signal >>>>> is controlled by the host controllers, there's also the SHDN# signal wired to >>>>> GPIO, which should be high for normal operation. >>> >>>>> Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com> >>> >>>>> --- >>>>> The patch is against the 'extcon-next' branch of the 'extcon.git' repo. >>> >>>>> Documentation/devicetree/bindings/extcon/extcon-max3355.txt | 21 ++ >>>>> drivers/extcon/Kconfig | 6 >>>>> drivers/extcon/Makefile | 1 >>>>> drivers/extcon/extcon-max3355.c | 122 >>>>> ++++++++++++ >>>>> 4 files changed, 150 insertions(+) >>> >>>>> Index: extcon/Documentation/devicetree/bindings/extcon/extcon-max3355.txt >>>>> =================================================================== >>>>> --- /dev/null >>>>> +++ extcon/Documentation/devicetree/bindings/extcon/extcon-max3355.txt >>>>> @@ -0,0 +1,21 @@ >>>>> +MAX3355 USB OTG chip >>> >>>> Need manufactor information as following : >>>> -> Maxim MAX3355 >>> >>> Would be Maxim enough? Or perhaps I should use Maxim Integrated [Products]? >> >> You haven't replied to my questions. >> >>>>> +-------------------- >>>>> + >>>>> +MAX3355 integrates a charge pump and comparators to enable a system with an >>>>> +integrated USB OTG dual-role transceiver to function as a USB OTG dual-role >>>>> +device. >>>>> + >>>>> +Required properties: >>>>> +- compatible: should be "maxim,max3355"; >>>>> +- maxim,shdn-gpio: should contain a phandle and GPIO specifier for the >>>>> GPIO pin >>>>> + connected to the MAX3355's SHDN# pin; >>>>> +- maxim,id-gpio: should contain a phandle and GPIO specifier for the GPIO pin >>>>> + connected to the MAX3355's ID_OUT pin. >>>>> + >>>>> +Example (Koelsch board): >>>>> + >>>>> + usb-otg { >>>>> + compatible = "maxim,max3355"; >>>>> + maxim,shdn-gpio = <&gpio2 4 GPIO_ACTIVE_LOW>; >>>>> + maxim,id-gpio = <&gpio5 31 GPIO_ACTIVE_HIGH>; >>> >>>> Kernel already supported the gpio helper function to get gpio from devicetree. >>>> I prefer use follwoing style by using of_get_gpio()/of_get_gpio_flags() in >>>> include/linux/of_gpio.h. >>> >>>> gpios = <&gpio2 4 GPIO_ACTIVE_LOW>, <&gpio5 31 GPIO_ACTIVE_HIGH>; >>> >>> OK, though Documentation/devicetree/bindings/gpio/gpio.txt does not seem >>> to insist on using this one... >> >> Moreover, it now says "gpios" isn't allowed for the new bindings. So I have to strongly disagree here... > > OK. But, I recommend you use the 'gpiod' with devm_gpiod_get > instead of using devm_gpio_request_one() directly as following: > You can refer drivers/extcon/extcon-usb-gpio.c about using gpiod. > > For example, > data->shdn_gpiod = devm_gpiod_get(dev, "maxim,shdn", GPIOD_IN); > data->id_gpiod = devm_gpiod_get(dev, "maxim,id", GPIOD_IN); Thanks, done now. I just had another idea: how about I add an optional "enable-gpio" property to the 'extcon-usb-gpio' driver? I wouldn't need my own driver then at all... :-) MBR, Sergei -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hello, I received the reply from you after too long time (17~18day). You better to reply it more a little more quickly if you have the question or new patches. On 2015년 11월 10일 03:24, Sergei Shtylyov wrote: > Hello. > > On 10/23/2015 08:56 AM, Chanwoo Choi wrote: > >>>>>> MAX3355E chip integrates a charge pump and comparators to enable a system with >>>>>> an integrated USB OTG dual-role transceiver to function as a USB OTG dual-role >>>>>> device. In addition to sensing/controlling Vbus, the chip also passes thru the >>>>>> ID signal from the USB OTG connector. On some Renesas boards, this signal is >>>>>> just fed into the SoC thru a GPIO pin -- there's no real OTG controller, only >>>>>> host and gadget USB controllers sharing the same USB bus; however, we'd like >>>>>> to allow host or gadget drivers to be loaded depending on the cable type, >>>>>> hence >>>>>> the need for the MAX3355 extcon driver. The Vbus status signals are also wired >>>>>> to GPIOs (however, we're not currently intested in them), the OFFVBUS# signal >>>>>> is controlled by the host controllers, there's also the SHDN# signal wired to >>>>>> GPIO, which should be high for normal operation. >>>> >>>>>> Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com> >>>> >>>>>> --- >>>>>> The patch is against the 'extcon-next' branch of the 'extcon.git' repo. >>>> >>>>>> Documentation/devicetree/bindings/extcon/extcon-max3355.txt | 21 ++ >>>>>> drivers/extcon/Kconfig | 6 >>>>>> drivers/extcon/Makefile | 1 >>>>>> drivers/extcon/extcon-max3355.c | 122 >>>>>> ++++++++++++ >>>>>> 4 files changed, 150 insertions(+) >>>> >>>>>> Index: extcon/Documentation/devicetree/bindings/extcon/extcon-max3355.txt >>>>>> =================================================================== >>>>>> --- /dev/null >>>>>> +++ extcon/Documentation/devicetree/bindings/extcon/extcon-max3355.txt >>>>>> @@ -0,0 +1,21 @@ >>>>>> +MAX3355 USB OTG chip >>>> >>>>> Need manufactor information as following : >>>>> -> Maxim MAX3355 >>>> >>>> Would be Maxim enough? Or perhaps I should use Maxim Integrated [Products]? >>> >>> You haven't replied to my questions. >>> >>>>>> +-------------------- >>>>>> + >>>>>> +MAX3355 integrates a charge pump and comparators to enable a system with an >>>>>> +integrated USB OTG dual-role transceiver to function as a USB OTG dual-role >>>>>> +device. >>>>>> + >>>>>> +Required properties: >>>>>> +- compatible: should be "maxim,max3355"; >>>>>> +- maxim,shdn-gpio: should contain a phandle and GPIO specifier for the >>>>>> GPIO pin >>>>>> + connected to the MAX3355's SHDN# pin; >>>>>> +- maxim,id-gpio: should contain a phandle and GPIO specifier for the GPIO pin >>>>>> + connected to the MAX3355's ID_OUT pin. >>>>>> + >>>>>> +Example (Koelsch board): >>>>>> + >>>>>> + usb-otg { >>>>>> + compatible = "maxim,max3355"; >>>>>> + maxim,shdn-gpio = <&gpio2 4 GPIO_ACTIVE_LOW>; >>>>>> + maxim,id-gpio = <&gpio5 31 GPIO_ACTIVE_HIGH>; >>>> >>>>> Kernel already supported the gpio helper function to get gpio from devicetree. >>>>> I prefer use follwoing style by using of_get_gpio()/of_get_gpio_flags() in >>>>> include/linux/of_gpio.h. >>>> >>>>> gpios = <&gpio2 4 GPIO_ACTIVE_LOW>, <&gpio5 31 GPIO_ACTIVE_HIGH>; >>>> >>>> OK, though Documentation/devicetree/bindings/gpio/gpio.txt does not seem >>>> to insist on using this one... >>> >>> Moreover, it now says "gpios" isn't allowed for the new bindings. So I have to strongly disagree here... >> >> OK. But, I recommend you use the 'gpiod' with devm_gpiod_get >> instead of using devm_gpio_request_one() directly as following: >> You can refer drivers/extcon/extcon-usb-gpio.c about using gpiod. >> >> For example, >> data->shdn_gpiod = devm_gpiod_get(dev, "maxim,shdn", GPIOD_IN); >> data->id_gpiod = devm_gpiod_get(dev, "maxim,id", GPIOD_IN); > > Thanks, done now. I just had another idea: how about I add an optional "enable-gpio" property to the 'extcon-usb-gpio' driver? I wouldn't need my own driver then at all... :-) What is meaning of 'enable-gpio' property? You better to explain your goal about 'enable-gpio' property Also, If you think that it is generic way about adding 'enable-gpio' property to extcon-usb-gpio.c, you can try it. Thanks, Chanwoo Choi -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hello. On 11/10/2015 2:52 AM, Chanwoo Choi wrote: > I received the reply from you after too long time (17~18day). > You better to reply it more a little more quickly > if you have the question or new patches. I've replied as soon as I had my new idea. >>>>>>> MAX3355E chip integrates a charge pump and comparators to enable a system with >>>>>>> an integrated USB OTG dual-role transceiver to function as a USB OTG dual-role >>>>>>> device. In addition to sensing/controlling Vbus, the chip also passes thru the >>>>>>> ID signal from the USB OTG connector. On some Renesas boards, this signal is >>>>>>> just fed into the SoC thru a GPIO pin -- there's no real OTG controller, only >>>>>>> host and gadget USB controllers sharing the same USB bus; however, we'd like >>>>>>> to allow host or gadget drivers to be loaded depending on the cable type, >>>>>>> hence >>>>>>> the need for the MAX3355 extcon driver. The Vbus status signals are also wired >>>>>>> to GPIOs (however, we're not currently intested in them), the OFFVBUS# signal >>>>>>> is controlled by the host controllers, there's also the SHDN# signal wired to >>>>>>> GPIO, which should be high for normal operation. >>>>> >>>>>>> Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com> >>>>> >>>>>>> --- >>>>>>> The patch is against the 'extcon-next' branch of the 'extcon.git' repo. >>>>> >>>>>>> Documentation/devicetree/bindings/extcon/extcon-max3355.txt | 21 ++ >>>>>>> drivers/extcon/Kconfig | 6 >>>>>>> drivers/extcon/Makefile | 1 >>>>>>> drivers/extcon/extcon-max3355.c | 122 >>>>>>> ++++++++++++ >>>>>>> 4 files changed, 150 insertions(+) >>>>> >>>>>>> Index: extcon/Documentation/devicetree/bindings/extcon/extcon-max3355.txt >>>>>>> =================================================================== >>>>>>> --- /dev/null >>>>>>> +++ extcon/Documentation/devicetree/bindings/extcon/extcon-max3355.txt >>>>>>> @@ -0,0 +1,21 @@ >>>>>>> +MAX3355 USB OTG chip >>>>> >>>>>> Need manufactor information as following : >>>>>> -> Maxim MAX3355 >>>>> >>>>> Would be Maxim enough? Or perhaps I should use Maxim Integrated [Products]? >>>> >>>> You haven't replied to my questions. >>>> >>>>>>> +-------------------- >>>>>>> + >>>>>>> +MAX3355 integrates a charge pump and comparators to enable a system with an >>>>>>> +integrated USB OTG dual-role transceiver to function as a USB OTG dual-role >>>>>>> +device. >>>>>>> + >>>>>>> +Required properties: >>>>>>> +- compatible: should be "maxim,max3355"; >>>>>>> +- maxim,shdn-gpio: should contain a phandle and GPIO specifier for the >>>>>>> GPIO pin >>>>>>> + connected to the MAX3355's SHDN# pin; >>>>>>> +- maxim,id-gpio: should contain a phandle and GPIO specifier for the GPIO pin >>>>>>> + connected to the MAX3355's ID_OUT pin. >>>>>>> + >>>>>>> +Example (Koelsch board): >>>>>>> + >>>>>>> + usb-otg { >>>>>>> + compatible = "maxim,max3355"; >>>>>>> + maxim,shdn-gpio = <&gpio2 4 GPIO_ACTIVE_LOW>; >>>>>>> + maxim,id-gpio = <&gpio5 31 GPIO_ACTIVE_HIGH>; >>>>> >>>>>> Kernel already supported the gpio helper function to get gpio from devicetree. >>>>>> I prefer use follwoing style by using of_get_gpio()/of_get_gpio_flags() in >>>>>> include/linux/of_gpio.h. >>>>> >>>>>> gpios = <&gpio2 4 GPIO_ACTIVE_LOW>, <&gpio5 31 GPIO_ACTIVE_HIGH>; >>>>> >>>>> OK, though Documentation/devicetree/bindings/gpio/gpio.txt does not seem >>>>> to insist on using this one... >>>> >>>> Moreover, it now says "gpios" isn't allowed for the new bindings. So I have to strongly disagree here... >>> >>> OK. But, I recommend you use the 'gpiod' with devm_gpiod_get >>> instead of using devm_gpio_request_one() directly as following: >>> You can refer drivers/extcon/extcon-usb-gpio.c about using gpiod. >>> >>> For example, >>> data->shdn_gpiod = devm_gpiod_get(dev, "maxim,shdn", GPIOD_IN); >>> data->id_gpiod = devm_gpiod_get(dev, "maxim,id", GPIOD_IN); >> >> Thanks, done now. I just had another idea: how about I add an optional "enable-gpio" property to the 'extcon-usb-gpio' driver? I wouldn't need my own driver then at all... :-) > > What is meaning of 'enable-gpio' property? > You better to explain your goal about 'enable-gpio' property This optional property would serve for enabling the valid signal on the ID GPIO, the same way I'm using the SHDN# GPIO in the MAX3355 driver. > Also, If you think that it is generic way about > adding 'enable-gpio' property to extcon-usb-gpio.c, > you can try it. Yes, I think it would be generic enough. > Thanks, > Chanwoo Choi MBR, Sergei -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Index: extcon/Documentation/devicetree/bindings/extcon/extcon-max3355.txt =================================================================== --- /dev/null +++ extcon/Documentation/devicetree/bindings/extcon/extcon-max3355.txt @@ -0,0 +1,21 @@ +MAX3355 USB OTG chip +-------------------- + +MAX3355 integrates a charge pump and comparators to enable a system with an +integrated USB OTG dual-role transceiver to function as a USB OTG dual-role +device. + +Required properties: +- compatible: should be "maxim,max3355"; +- maxim,shdn-gpio: should contain a phandle and GPIO specifier for the GPIO pin + connected to the MAX3355's SHDN# pin; +- maxim,id-gpio: should contain a phandle and GPIO specifier for the GPIO pin + connected to the MAX3355's ID_OUT pin. + +Example (Koelsch board): + + usb-otg { + compatible = "maxim,max3355"; + maxim,shdn-gpio = <&gpio2 4 GPIO_ACTIVE_LOW>; + maxim,id-gpio = <&gpio5 31 GPIO_ACTIVE_HIGH>; + }; Index: extcon/drivers/extcon/Kconfig =================================================================== --- extcon.orig/drivers/extcon/Kconfig +++ extcon/drivers/extcon/Kconfig @@ -45,6 +45,12 @@ config EXTCON_MAX14577 Maxim MAX14577/77836. The MAX14577/77836 MUIC is a USB port accessory detector and switch. +config EXTCON_MAX3355 + tristate "MAX3355 USB OTG EXTCON Support" + help + Say Y here to enable support for the USB host detection by MAX3355 + OTG USB chip. + config EXTCON_MAX77693 tristate "MAX77693 EXTCON Support" depends on MFD_MAX77693 && INPUT Index: extcon/drivers/extcon/Makefile =================================================================== --- extcon.orig/drivers/extcon/Makefile +++ extcon/drivers/extcon/Makefile @@ -7,6 +7,7 @@ obj-$(CONFIG_EXTCON_ADC_JACK) += extcon- obj-$(CONFIG_EXTCON_ARIZONA) += extcon-arizona.o obj-$(CONFIG_EXTCON_GPIO) += extcon-gpio.o obj-$(CONFIG_EXTCON_MAX14577) += extcon-max14577.o +obj-$(CONFIG_EXTCON_MAX3355) += extcon-max3355.o obj-$(CONFIG_EXTCON_MAX77693) += extcon-max77693.o obj-$(CONFIG_EXTCON_MAX8997) += extcon-max8997.o obj-$(CONFIG_EXTCON_PALMAS) += extcon-palmas.o Index: extcon/drivers/extcon/extcon-max3355.c =================================================================== --- /dev/null +++ extcon/drivers/extcon/extcon-max3355.c @@ -0,0 +1,122 @@ +/* + * MAX3355 USB OTG chip extcon driver + * + * Copyright (C) 2014 Cogent Embedded, Inc. + * Author: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com> + * + * This software is licensed under the terms of the GNU General Public + * License version 2, as published by the Free Software Foundation, and + * may be copied, distributed, and modified under those terms. + */ + +#include <linux/extcon.h> +#include <linux/gpio.h> +#include <linux/init.h> +#include <linux/interrupt.h> +#include <linux/kernel.h> +#include <linux/module.h> +#include <linux/of_gpio.h> +#include <linux/platform_device.h> +#include <linux/slab.h> +#include <linux/workqueue.h> + +struct max3355_data { + struct extcon_dev *edev; + int id_gpio; +}; + +static const char *max3355_cable[] = { + [0] = "USB-HOST", + NULL, +}; + +static irqreturn_t max3355_id_irq(int irq, void *dev_id) +{ + struct max3355_data *data = dev_id; + int id = gpio_get_value(data->id_gpio); + + extcon_set_cable_state(data->edev, "USB-HOST", !id); + + return IRQ_HANDLED; +} + +static int max3355_probe(struct platform_device *pdev) +{ + struct device_node *np = pdev->dev.of_node; + struct max3355_data *data; + int gpio, irq, ret; + + data = devm_kzalloc(&pdev->dev, sizeof(struct max3355_data), + GFP_KERNEL); + if (!data) + return -ENOMEM; + + data->edev = devm_extcon_dev_allocate(&pdev->dev, max3355_cable); + if (IS_ERR(data->edev)) { + dev_err(&pdev->dev, "failed to allocate extcon device\n"); + return PTR_ERR(data->edev); + } + data->edev->name = kstrdup(np->name, GFP_KERNEL); + + gpio = of_get_named_gpio(np, "maxim,id-gpio", 0); + if (gpio < 0) + return gpio; + data->id_gpio = gpio; + + gpio = of_get_named_gpio(np, "maxim,shdn-gpio", 0); + if (gpio < 0) + return gpio; + + ret = devm_gpio_request_one(&pdev->dev, gpio, GPIOF_DIR_OUT | + GPIOF_INIT_HIGH, pdev->name); + if (ret < 0) + return ret; + + ret = devm_gpio_request_one(&pdev->dev, data->id_gpio, GPIOF_DIR_IN, + pdev->name); + if (ret < 0) + return ret; + + ret = devm_extcon_dev_register(&pdev->dev, data->edev); + if (ret < 0) + return ret; + + irq = gpio_to_irq(data->id_gpio); + if (irq < 0) + return irq; + + ret = devm_request_threaded_irq(&pdev->dev, irq, NULL, max3355_id_irq, + IRQF_TRIGGER_RISING | + IRQF_TRIGGER_FALLING | IRQF_ONESHOT, + pdev->name, data); + if (ret < 0) + return ret; + + platform_set_drvdata(pdev, data); + + /* Perform initial detection */ + max3355_id_irq(irq, data); + + return 0; +} + +static const struct of_device_id max3355_match_table[] = { + { .compatible = "maxim,max3355", }, + { } +}; +MODULE_DEVICE_TABLE(of, max3355_match_table); + +static struct platform_driver max3355_driver = { + .probe = max3355_probe, + .driver = { + .name = "extcon-max3355", + .of_match_table = max3355_match_table, + .owner = THIS_MODULE, + }, +}; + +module_platform_driver(max3355_driver); + +MODULE_AUTHOR("Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>"); +MODULE_DESCRIPTION("MAX3355 extcon driver"); +MODULE_LICENSE("GPL v2");
MAX3355E chip integrates a charge pump and comparators to enable a system with an integrated USB OTG dual-role transceiver to function as a USB OTG dual-role device. In addition to sensing/controlling Vbus, the chip also passes thru the ID signal from the USB OTG connector. On some Renesas boards, this signal is just fed into the SoC thru a GPIO pin -- there's no real OTG controller, only host and gadget USB controllers sharing the same USB bus; however, we'd like to allow host or gadget drivers to be loaded depending on the cable type, hence the need for the MAX3355 extcon driver. The Vbus status signals are also wired to GPIOs (however, we're not currently intested in them), the OFFVBUS# signal is controlled by the host controllers, there's also the SHDN# signal wired to GPIO, which should be high for normal operation. Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com> --- The patch is against the 'extcon-next' branch of the 'extcon.git' repo. Documentation/devicetree/bindings/extcon/extcon-max3355.txt | 21 ++ drivers/extcon/Kconfig | 6 drivers/extcon/Makefile | 1 drivers/extcon/extcon-max3355.c | 122 ++++++++++++ 4 files changed, 150 insertions(+) -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html