Re: [PATCH v3] extcon: gpio: Add the support for Device tree bindings

[Chanwoo Choi]

Hi Rob,

First of all, thanks for your review.

On 2015ë 10ì 22ì 07:35, Rob Herring wrote:
> On Tue, Oct 20, 2015 at 11:37 PM, Chanwoo Choi <cw00.choi@xxxxxxxxxxx> wrote:
>> This patch adds the support for Device tree bindings of extcon-gpio driver.
>> The extcon-gpio device tree node must include the both 'extcon-id' and
>> 'extcon-gpio' property.
> 
> I think this is too tied to the Linux driver. Instead, think about
> what the connector contains. I think you should define a usb connector
> node and compatible (e.g. usb-connector or usb-ab-connector). This

The Extcon framework support the various external connector.
You mean that extcon-gpio.c should have the separate compatible
for each external connector?

For example,
static const struct of_device_id gpio_extcon_of_match[] = {
	{
		.compatible = "extcon-usb",	/* USB connector */
		.data = EXTCON_USB_DATA,
	}, {
		.compatible = "extcon-chg-sdp",	/* SDP charger connector */
		.data = EXTCON_CHG_SDP_DATA,
	}, {
		.compatible = "extcon-chg-dcp",	/* DCP charger connector */
		.data = EXTCON_CHG_DCP_DATA,
	}, {
		.compatible = "extcon-jack-microphone", /* Microphone jack connector */
		.data = EXTCON_JACK_MICROPHONE_DATA,
	}, {
		.compatible = "extcon-disp-hdmi", /* HDMI connector*/
		.data = EXTCON_DISP_HDMI_DATA,
	},
	......

};

static struct platform_driver extcon_gpio_driver = {
	.probe	= gpio_extcon_probe,
	.remove	= gpio_extcon_remove,
	.driver	= {
		.name = "extcon-gpio",
		.of_match_table = gpio_extcon_of_match,
	},
};

> probably needs to distinguish the connector type as well especially
> with TypeC coming.

You're right. Currently Extcon framework don't support the setting of connector type
(e.g., USB Type A, Type B, Mini-A, Type-C ...)
I'm planing to add the extcon core API to support the connector type.

> 
>>
>> For exmaple:
>>         usb_cable: extcon-gpio-0 {
>>                 compatible = "extcon-gpio";
>>                 extcon-id = <EXTCON_USB>;
>>                 extcon-gpio = <&gpio6 1 GPIO_ACTIVE_HIGH>;
> 
> This tied to a Vbus detect circuit?
> 
> So "vbus-detect-gpios" in the connector node.

If extcon-gpio.c driver support the multiple comaptible for each external connector,
this driver will support the separate property name for gpio to get it from devicetree.

> 
> For host side (or OTG host mode), you may also need a vbus-supply
> regulator property. OTG will also need an id-gpios for ID pin.

There is already drivers/extcon/extcon-usb-gpio.c driver for USB with GPIO.
[1] https://lkml.org/lkml/2015/2/2/187

> 
>>         }
>>
>>         ta_cable: extcon-gpio-1 {
> 
> This is all the same connector as above?
> 
>>                 compatible = "extcon-gpio";
>>                 extcon-id = <EXTCON_CHG_USB_DCP>;
>>                 extcon-gpio = <&gpio3 2 GPIO_ACTIVE_LOW>;
> 
> This is just detecting D+ and D- are both pulled high?

The extcon-gpio.c might be used to detect the external connector by using only GPIO
which has not correlation to both D+  and D-. This GPIO pin is just used to detect
the state whether external connector is attached or detached.

> 
> So "dcp-detect-gpios" in the connector node.

ditto.

> 
>>                 debounce-ms = <50>;     /* 50 millisecond */
>>                 wakeup-source;
> 
> wakeup-source implies an interrupt as I read Sudeep's series. Either
> gpios need to be allowed or these need to be defined as interrupts.

You are right. I'm going to consider it and modify this driver with more proper method.

> 
>>         }
>>
>>         &dwc3_usb {
>>                 extcon = <&usb_cable>;
>>         };
>>
>>         &charger {
>>                 extcon = <&ta_cable>;
> 
> Not sure what to do with this. Both can point to a single connector
> node I think.

There are both extcon provider driver and client driver.

The extcon provider driver provide client driver to get whether cable state is attached
or detached. And then client driver is waiting the notification from extcon provider driver.
Before waiting the notification from extcon providier driver, the client driver should get
the instance of specific extcon provider driver to register the notifier chain.

When getting the instance of specific extcon provider in device tree,
the client driver use the extcon_get_edev_by_phandle() function which
parses the phandle name of 'extcon'.

The latest Linux kernel includes the example[1][2] in arch/arm/boot/dts/omap5-cm-t54.dts

[1] http://www.spinics.net/lists/linux-omap/msg105074.html
- ARM: dts: sbc-t54: add support for sbc-t54 with cm-t54
[2] ARM: dts: cm-t54: setup omap_dwc3
- http://www.spinics.net/lists/linux-omap/msg111174.html

For example 
&i2c1 {
	......
	palmas: palmas@48 {
		extcon_usb3: palmas_usb {
			compatible = "ti,plamas-usb-vid";
			ti,enable-vbus-detection;
			ti,enable-id-detection;
			ti,wakeup;
		};
		......
	};
	......
};

&usb3 {
	extcon = <&extcon_usb3>;
	vbus-supply = <...>;
};

Regards,
Chanwoo Choi
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/