Message-ID: <20171114162536.GA20665@kroah.com>
Date:   Tue, 14 Nov 2017 17:25:36 +0100
From:   Greg KH <gregkh@...uxfoundation.org>
To:     Heikki Krogerus <heikki.krogerus@...ux.intel.com>
Cc:     Linus Torvalds <torvalds@...ux-foundation.org>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        USB list <linux-usb@...r.kernel.org>
Subject: Re: [GIT PULL] USB/PHY driver changes for 4.15-rc1

On Tue, Nov 14, 2017 at 03:23:33PM +0200, Heikki Krogerus wrote:
> On Mon, Nov 13, 2017 at 09:29:36PM -0800, Linus Torvalds wrote:
> > On Mon, Nov 13, 2017 at 8:19 AM, Greg KH <gregkh@...uxfoundation.org> wrote:
> > >
> > > Other major thing is the typec code that moved out of staging and into
> > > the "real" part of the drivers/usb/ tree, which was nice to see happen.
> > 
> > Hmm. So now it asks me about Type-C Port Controller Manager. Fair
> > enough. I say "N", because I have none. But then it still asks me
> > about that TI TPS6598x driver...
> > 
> > So I do see the _technical_ logic in there - the "TYPEC" config option
> > is a hidden internal option, and it's selected by the things that need
> > it.
> > 
> > But from a user perspective, this configuration model is really strange.
> > 
> > Why is TYPEC_TCPM something you ask the user, but not "do you want
> > Type-C support"?  And if you single out the PCM side to ask about, why
> > don't you single out the power delivery side?
> > 
> > Wouldn't it make more sense to at least ask whether I want Type-C
> > power delivery chips before it then starts asking about individual PD
> > drivers, the same way you asked about the port controller before you
> > started asking ab out individual port controller drivers?
> 
> True. The options were made originally the way they are as the
> assumption was that the OS will always handle the USB Type-C and PD
> state machines, meaning we would always depend on the Type-C Port
> Controller Manager, which of course is not the case any more.
> 
> Would the attached patch be sufficient?
> 
> 
> Thanks,
> 
> -- 
> heikki

> >From 3bbd624a67df91c23db996db5f2f931fde77fcc1 Mon Sep 17 00:00:00 2001
> From: Heikki Krogerus <heikki.krogerus@...ux.intel.com>
> Date: Tue, 14 Nov 2017 14:45:27 +0300
> Subject: [PATCH] usb: add user selectable option for the whole USB Type-C
>  Support
> 
> It is more clear from user perspective to wrap the whole USB
> Type-C support under a single option that the user can
> select, then it is to always ask the user for every USB
> Type-C and USB Power Delivery driver separately.
> 
> Signed-off-by: Heikki Krogerus <heikki.krogerus@...ux.intel.com>
> ---
>  drivers/usb/typec/Kconfig      | 54 +++++++++++++++++++++++++++++++++++-------
>  drivers/usb/typec/ucsi/Kconfig |  1 -
>  2 files changed, 46 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/usb/typec/Kconfig b/drivers/usb/typec/Kconfig
> index 465d7da849c3..bcb2744c5977 100644
> --- a/drivers/usb/typec/Kconfig
> +++ b/drivers/usb/typec/Kconfig
> @@ -1,13 +1,53 @@
>  
> -menu "USB Power Delivery and Type-C drivers"
> +menuconfig TYPEC
> +	tristate "USB Type-C Support"
> +	help
> +	  USB Type-C Specification defines a cable and connector for USB where
> +	  only one type of plug is supported on both ends, i.e. there will not
> +	  be Type-A plug on one end of the cable and Type-B plug on the other.
> +	  Determination of the host-to-device relationship happens through a
> +	  specific Configuration Channel (CC) which goes through the USB Type-C
> +	  cable. The Configuration Channel may also be used to detect optional
> +	  Accessory Modes - Analog Audio and Debug - and if USB Power Delivery
> +	  is supported, the Alternate Modes, where the connector is used for
> +	  something else then USB communication.
> +
> +	  USB Power Delivery Specification defines a protocol that can be used
> +	  to negotiate the voltage and current levels with the connected
> +	  partners. USB Power Delivery allows higher voltages then the normal
> +	  5V, up to 20V, and current up to 5A over the cable. The USB Power
> +	  Delivery protocol is also used to negotiate the optional Alternate
> +	  Modes when they are supported. USB Power Delivery does not depend on
> +	  USB Type-C connector, however it is mostly used together with USB
> +	  Type-C connectors.
> +
> +	  USB Type-C and USB Power Delivery Specifications define a set of state
> +	  machines that need to be implemented in either software or firmware.
> +	  Simple USB Type-C PHYs, for example USB Type-C Port Controller
> +	  Interface Specification compliant "Port Controllers" need the state
> +	  machines to be handled in the OS, but stand-alone USB Type-C and Power
> +	  Delivery controllers handle the state machines inside their firmware.
> +	  The USB Type-C and Power Delivery controllers usually function
> +	  autonomously, and do not necessarily require drivers.
> +
> +	  Enable this configurations option if you have USB Type-C connectors on
> +	  your system and 1) you know your USB Type-C hardware requires OS
> +	  control (a driver) to function, or 2) if you need to be able to read
> +	  the status of the USB Type-C ports in your system, or 3) if you need
> +	  to be able to swap the power role (decide are you supplying or
> +	  consuming power over the cable) or data role (host or device) when
> +	  both roles are supported.
> +
> +	  For more information, see the kernel documentation for USB Type-C
> +	  Connector Class API (Documentation/driver-api/usb/typec.rst)
> +	  <https://www.kernel.org/doc/html/latest/driver-api/usb/typec.html>
> +	  and ABI (Documentation/ABI/testing/sysfs-class-typec).
>  
> -config TYPEC
> -	tristate
> +if TYPEC
>  
>  config TYPEC_TCPM
>  	tristate "USB Type-C Port Controller Manager"
>  	depends on USB
> -	select TYPEC
>  	help
>  	  The Type-C Port Controller Manager provides a USB PD and USB Type-C
>  	  state machine for use with Type-C Port Controllers.
> @@ -22,7 +62,6 @@ config TYPEC_WCOVE
>  	depends on INTEL_SOC_PMIC
>  	depends on INTEL_PMC_IPC
>  	depends on BXT_WC_PMIC_OPREGION
> -	select TYPEC
>  	help
>  	  This driver adds support for USB Type-C detection on Intel Broxton
>  	  platforms that have Intel Whiskey Cove PMIC. The driver can detect the
> @@ -31,14 +70,13 @@ config TYPEC_WCOVE
>  	  To compile this driver as module, choose M here: the module will be
>  	  called typec_wcove
>  
> -endif
> +endif # TYPEC_TCPM
>  
>  source "drivers/usb/typec/ucsi/Kconfig"
>  
>  config TYPEC_TPS6598X
>  	tristate "TI TPS6598x USB Power Delivery controller driver"
>  	depends on I2C
> -	select TYPEC
>  	help
>  	  Say Y or M here if your system has TI TPS65982 or TPS65983 USB Power
>  	  Delivery controller.
> @@ -46,4 +84,4 @@ config TYPEC_TPS6598X
>  	  If you choose to build this driver as a dynamically linked module, the
>  	  module will be called tps6598x.ko.
>  
> -endmenu
> +endif # TYPEC
> diff --git a/drivers/usb/typec/ucsi/Kconfig b/drivers/usb/typec/ucsi/Kconfig
> index d0c31cee4720..e36d6c73c4a4 100644
> --- a/drivers/usb/typec/ucsi/Kconfig
> +++ b/drivers/usb/typec/ucsi/Kconfig
> @@ -1,7 +1,6 @@
>  config TYPEC_UCSI
>  	tristate "USB Type-C Connector System Software Interface driver"
>  	depends on !CPU_BIG_ENDIAN
> -	select TYPEC
>  	help
>  	  USB Type-C Connector System Software Interface (UCSI) is a
>  	  specification for an interface that allows the operating system to

This looks great to me, I'll queue it up for later this week with a few
other USB fixes.

thanks for doing this,

greg k-h

Powered by blists - more mailing lists