Re: [PATCHv11 2/3] usb: USB Type-C connector class

[Greg KH]

On Mon, Nov 21, 2016 at 03:27:41PM +0200, Heikki Krogerus wrote:
> On Mon, Nov 21, 2016 at 11:33:11AM +0100, Greg KH wrote:
> > On Thu, Nov 17, 2016 at 12:50:35PM +0200, Heikki Krogerus wrote:
> > > The purpose of USB Type-C connector class is to provide
> > > unified interface for the user space to get the status and
> > > basic information about USB Type-C connectors on a system,
> > > control over data role swapping, and when the port supports
> > > USB Power Delivery, also control over power role swapping
> > > and Alternate Modes.
> > > 
> > > Signed-off-by: Heikki Krogerus <heikki.krogerus@xxxxxxxxxxxxxxx>
> > 
> > I'd like to get some acks or reviewed-by from others, but one thing
> > jumped out at me, which implies you did not run checkpatch.pl on the
> > patch:
> > 
> > 644
> 
> OK.
> 
> > > --- a/drivers/usb/Kconfig
> > > +++ b/drivers/usb/Kconfig
> > > @@ -152,6 +152,8 @@ source "drivers/usb/phy/Kconfig"
> > >  
> > >  source "drivers/usb/gadget/Kconfig"
> > >  
> > > +source "drivers/usb/typec/Kconfig"
> > > +
> > >  config USB_LED_TRIG
> > >  	bool "USB LED Triggers"
> > >  	depends on LEDS_CLASS && LEDS_TRIGGERS
> > > diff --git a/drivers/usb/Makefile b/drivers/usb/Makefile
> > > index 7791af6..c7f4098 100644
> > > --- a/drivers/usb/Makefile
> > > +++ b/drivers/usb/Makefile
> > > @@ -62,3 +62,5 @@ obj-$(CONFIG_USB_GADGET)	+= gadget/
> > >  obj-$(CONFIG_USB_COMMON)	+= common/
> > >  
> > >  obj-$(CONFIG_USBIP_CORE)	+= usbip/
> > > +
> > > +obj-$(CONFIG_TYPEC)		+= typec/
> > > diff --git a/drivers/usb/typec/Kconfig b/drivers/usb/typec/Kconfig
> > > new file mode 100644
> > > index 0000000..b229fb9
> > > --- /dev/null
> > > +++ b/drivers/usb/typec/Kconfig
> > > @@ -0,0 +1,7 @@
> > > +
> > > +menu "USB PD and Type-C drivers"
> > 
> > What is "PD"?  (yes, I know, but very few people do...)  Spell it out.
> 
> OK.
> 
> > > +
> > > +config TYPEC
> > > +	tristate
> > 
> > Hah, that says NOTHING about what this code is at all.
> 
> Alone the class driver does nothing. Why would the user need to be
> aware of if it when selecting the Type-C drivers, and what can the
> user use that information for?

If you see a blank Kconfig option, what are you supposed to do with it?
How do you know if you need to enable it or not?  Are you just supposed
to guess?

That is what the help text is for...

thanks,

greg k-h