Re: [PATCH -next] usb: roles: Hide option USB_ROLE_SWITCH

[Dan Carpenter]

On Tue, Nov 05, 2019 at 05:26:24PM +0200, Heikki Krogerus wrote:
> Hi Dan,
> 
> On Tue, Nov 05, 2019 at 04:16:05PM +0300, Dan Carpenter wrote:
> > On Tue, Nov 05, 2019 at 02:42:18PM +0200, Heikki Krogerus wrote:
> > > On Mon, Nov 04, 2019 at 10:48:50PM +0800, Mao Wenan wrote:
> > > > The USB role switch class is, after all,
> > > > not useful by itself. Hiding USB_ROLE_SWITCH
> > > > so we can avoid any of the pitfalls associated
> > > > with user-visible symbols and "select".
> > > > 
> > > > Signed-off-by: Mao Wenan <maowenan@xxxxxxxxxx>
> > > > ---
> > > >  drivers/usb/roles/Kconfig | 2 +-
> > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > > 
> > > > diff --git a/drivers/usb/roles/Kconfig b/drivers/usb/roles/Kconfig
> > > > index f8b31aa..1da58d4 100644
> > > > --- a/drivers/usb/roles/Kconfig
> > > > +++ b/drivers/usb/roles/Kconfig
> > > > @@ -1,7 +1,7 @@
> > > >  # SPDX-License-Identifier: GPL-2.0
> > > >  
> > > >  config USB_ROLE_SWITCH
> > > > -	tristate "USB Role Switch Support"
> > > > +	tristate
> > > >  	help
> > > >  	  USB Role Switch is a device that can select the USB role - host or
> > > >  	  device - for a USB port (connector). In most cases dual-role capable
> > > 
> > > You didn't actually convert the "depends on USB_ROLE_SWTICH" to
> > > "select USB_ROLE_SWITCH" before this. You also left the help text that
> > > is now useless.
> > > 
> > > I really think that instead of this, we should just convert all
> > > "select USB_ROLE_SWTICH" to "depends on USB_ROLE_SWITCH".
> > 
> > The you have to find USB_ROLE_SWITCH first when you want to enable your
> > hardware...  It's feels really confusing when you want to create a
> > .config file...
> 
> Unfortunately selecting the class alone is not enough. The USB role
> switch on the system may be a dual-role capable USB controller, but it
> may also be a mux that has its own separate driver.
> 
> It's equally or even more confusing for the user if the USB drivers
> are enabled, including the dual-role mode, but the connector still
> works only in one role, or in worst case not at all (if there is no
> mux driver and the mux is left in "safe mode" so that the pins on the
> connector are not connected to anything).
> 
> I still think that we should make these drivers depend on the class
> instead of just selecting it. That way we at least give the user a
> hint that there are also separate USB role switch drivers that may be
> needed.
> 

I guess I see your point.  My problem then is just with the menuconfig
system, not really with this patch.

> > I sometimes think maybe I'm too stupid to configure a kernel these days
> > and that's sort of sad because how is Aunt Tillie supposed to manage?
> 
> We can always use something like conditional comments in the
> Kconfig files to make sure that the user is told that in order to
> select the driver, a dependency must be satisfied:
> 
>         config MY_AWESOME_DRIVER
>                 tristate "My awesome driver!"
>                 depends on USB_ROLE_SWITCH
>                 help
>                   That's right! IT REALLY IS AWESOME!
> 
>         comment "My awesome driver depends on USB_ROLE_SWITCH..."
>                 depends on USB_ROLE_SWITCH=n
> 

Those sorts of help don't show up unless you already meet the
dependencies or if you search for it.  And if you search for it then it
already lists the depnds so the comment isn't required...

I guess that in the olden days you used to go through the menus and
look at all the high level options and figure it out.  These days you
sort of have to know what you want already.  And then you use the search
feature to enable it.

The menuconfig system really is very broken.  I've looked at alternative
ways to do it, but it quite a bit of work involved...

regards,
dan carpenter