From: NeilBrown <neilb@suse.com>
To: Baolin Wang <baolin.wang@linaro.org>, balbi@kernel.org,
        gregkh@linuxfoundation.org
Date: Wed, 29 Mar 2017 09:56:32 +1100
Cc: linux-usb@vger.kernel.org, linux-kernel@vger.kernel.org,
        linaro-kernel@lists.linaro.org, jun.li@nxp.com,
        peter.chen@freescale.com, broonie@kernel.org, baolin.wang@linaro.org
Subject: Re: [PATCH v2 1/2] usb: phy: Introduce one extcon device into usb phy
In-Reply-To: <0b16763df04e197fb5b3c21b19799280d3bce275.1490248054.git.baolin.wang@linaro.org>
References: <0b16763df04e197fb5b3c21b19799280d3bce275.1490248054.git.baolin.wang@linaro.org>
Message-ID: <8760it6me7.fsf@notabene.neil.brown.name>
MIME-Version: 1.0
Content-Type: multipart/signed; boundary="=-=-=";
        micalg=pgp-sha256; protocol="application/pgp-signature"
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 4555
Lines: 127

--=-=-=
Content-Type: text/plain
Content-Transfer-Encoding: quoted-printable

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@suse.com>
> Signed-off-by: Baolin Wang <baolin.wang@linaro.org>
> ---
> 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"
>=20=20
>  config USB_PHY
> +	select EXTCON
>  	def_bool n
>=20=20
>  #
> @@ -116,7 +117,7 @@ config OMAP_OTG
>=20=20
>  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=3Dm, 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=3Dm, 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, vo=
id *res, void *match_data)
>  	return *phy =3D=3D match_data;
>  }
>=20=20
> +static int usb_add_extcon(struct usb_phy *x)
> +{
> +	int ret;
> +
> +	if (of_property_read_bool(x->dev->of_node, "extcon")) {
> +		x->edev =3D 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

--=-=-=
Content-Type: application/pgp-signature; name="signature.asc"

-----BEGIN PGP SIGNATURE-----

iQIzBAEBCAAdFiEEG8Yp69OQ2HB7X0l6Oeye3VZigbkFAlja6iAACgkQOeye3VZi
gbnnvg//QLRsxBT870miNuwRU0MZErEd66LEt2oXCFBfcMXLYjALoXx50IlvbNGq
7UaRPSaqM3nZvcbZnNUUnzfFkMxfj97m78FZ236ov34CcJoa2WcLE/CCYXJ6Tht/
Ti3BvFDpFi7NFjh6P9zMCk+o2DRegARNLUCYH2ZerNLomHQZFs1Lj9o28cueF7hz
oLAJh8uXopksrjvyrpZAdf6CEmXsXOo5G7jwAe454QqhHPnPn2b0JkYY/v1Tr/xr
QlaS4uu9uaEoXE/t1O6SYHvSFUgbospjDpywhUnPL+VfeiHgVN8SjX2M+Lr57rzy
/vDptsiZB2V7MZnFdboetfhZ/2C81eelmNPSXkDSGfXPQxxvCv8K86DhlWdAv5ul
njwX7V1vkshTXoCLoTyn7v4sb0BKPA8yVRoz7isMiemqk7L5Ptzfmi7wU6NxmN+z
Zi2QFvE6zK9G0c6616/4XAkBedipT2ld1zLiDqM1uhGYHZ9vWg6Yhk1cwrvTR7BG
uvu+jaKJ90AmX25djf9w1AdnwL5hlO6n7K8YPTMtiyCZdqoS/mVbsqKiI1kpZD6p
RgXRFGom8++lWt/1x+VSc+N/bLQe617S5Tq5iyrgDlGY9A4gRGD1gKBxteTf7inB
eXcLo03vF6Nc1Kmtg2nCSxVtdzbiOiggrULRJtILxaB4C1O9CBo=
=hv9K
-----END PGP SIGNATURE-----
--=-=-=--