Re: [PATCH v2 1/2] usb: phy: Introduce one extcon device into usb phy

[NeilBrown]

On Thu, Mar 23 2017, Baolin Wang wrote:

> Usually usb phy need register one extcon device to get the connection
> notifications. It will remove some duplicate code if the extcon device
> is registered using common code instead of each phy driver having its
> own related extcon APIs. So we add one pointer of extcon device into
> usb phy structure, and some other helper functions to register extcon.
>
> Suggested-by: NeilBrown <neilb@xxxxxxxx>
> Signed-off-by: Baolin Wang <baolin.wang@xxxxxxxxxx>
> ---
> Changes since v1:
>  - Fix build errors with random config.
> ---
>  drivers/usb/phy/Kconfig |    6 +++---
>  drivers/usb/phy/phy.c   |   44 ++++++++++++++++++++++++++++++++++++++++++++
>  include/linux/usb/phy.h |    6 ++++++
>  3 files changed, 53 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/usb/phy/Kconfig b/drivers/usb/phy/Kconfig
> index 61cef75..c9c61a8 100644
> --- a/drivers/usb/phy/Kconfig
> +++ b/drivers/usb/phy/Kconfig
> @@ -4,6 +4,7 @@
>  menu "USB Physical Layer drivers"
>  
>  config USB_PHY
> +	select EXTCON
>  	def_bool n
>  
>  #
> @@ -116,7 +117,7 @@ config OMAP_OTG
>  
>  config TAHVO_USB
>  	tristate "Tahvo USB transceiver driver"
> -	depends on MFD_RETU && EXTCON
> +	depends on MFD_RETU
>  	depends on USB_GADGET || !USB_GADGET # if USB_GADGET=m, this can't be 'y'
>  	select USB_PHY
>  	help
> @@ -148,7 +149,6 @@ config USB_MSM_OTG
>  	depends on (USB || USB_GADGET) && (ARCH_QCOM || COMPILE_TEST)
>  	depends on USB_GADGET || !USB_GADGET # if USB_GADGET=m, this can't be 'y'
>  	depends on RESET_CONTROLLER
> -	depends on EXTCON
>  	select USB_PHY
>  	help
>  	  Enable this to support the USB OTG transceiver on Qualcomm chips. It
> @@ -162,7 +162,7 @@ config USB_MSM_OTG
>  config USB_QCOM_8X16_PHY
>  	tristate "Qualcomm APQ8016/MSM8916 on-chip USB PHY controller support"
>  	depends on ARCH_QCOM || COMPILE_TEST
> -	depends on RESET_CONTROLLER && EXTCON
> +	depends on RESET_CONTROLLER
>  	select USB_PHY
>  	select USB_ULPI_VIEWPORT
>  	help
> diff --git a/drivers/usb/phy/phy.c b/drivers/usb/phy/phy.c
> index 98f75d2..baa8b18 100644
> --- a/drivers/usb/phy/phy.c
> +++ b/drivers/usb/phy/phy.c
> @@ -100,6 +100,41 @@ static int devm_usb_phy_match(struct device *dev, void *res, void *match_data)
>  	return *phy == match_data;
>  }
>  
> +static int usb_add_extcon(struct usb_phy *x)
> +{
> +	int ret;
> +
> +	if (of_property_read_bool(x->dev->of_node, "extcon")) {
> +		x->edev = extcon_get_edev_by_phandle(x->dev, 0);

This is not what I meant to suggest.
This provides an extcon only for some phy devices.  I meant that every
single usb_phy should always have an extcon.
So phy.c should probably call extcon_dev_allocate() and
extcon_dev_register() - or similar.

The driver then calls extcon_set_state() or similar on this extcon
device in whatever way might be appropriate.
For a phy driver like phy-qcom-8x16-usb it might just pass on
events from some separate extcon device.

For the (hypothetical?) device that Felipe suggests with separate
ID and VBUS extcons, it would pass on events from multiple different
extcons.  i.e. it would act like a multiplexer.

Other drivers would do things from interrupt handlers, based on values
read from registers.

The important point is that each usb_phy doesn't get a pointer to some
separate extcon.  Rather it has an extcon that it completely owns.  The
name of the extcon is the same of the phy.  The extcon should be seen as
a part of the phy, not a separate thing which provides service to the
phy.

Thanks,
NeilBrown

Attachment: signature.asc
Description: PGP signature