Re: [PATCH 1/2] serial: 8250_lpss: fix Kconfig dependencies

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Andy,

On Fri, 16 Dec 2016 16:02:27 +0200, Andy Shevchenko wrote:
> On Fri, 2016-12-16 at 14:28 +0100, Jean Delvare wrote:
> > On Fri, 16 Dec 2016 13:31:19 +0200, Andy Shevchenko wrote:
> > > On Fri, 2016-12-16 at 11:50 +0100, Jean Delvare wrote:
> > > > Hiding tristate options with "if EXPERT" is usually not a good
> > > > idea. You can decide that the driver should be included by
> > > > default, but you don't know if the user wants it built-in or as
> > > > a module. Hiding the option prevents the user from making that
> > > > decision.
> > > 
> > > Then please start from 8250_PCI.
> > 
> > 8250_PCI is indeed a candidate for a similar change. However it is
> > less specific and also doesn't select other drivers, so fixing it
> > had lower priority for me. I can look into it later.
> 
> Okay, just to make sure you know that drivers we are talking about used
> to be a part of 8250_pci

I wasn't aware of that, I'll look into it, thanks.

> > > (...)
> > > That's why I would like to keep this for EXPERTs.
> > > 
> > > CONFIG_EXPERT=y
> > > CONFIG_SERIAL_8250_LPSS=m 
> > > 
> > > will not work?
> > 
> > It would "work" in that it would allow us to modularize the drivers.
> > However enabling CONFIG_EXPERT would make more than 100 extra kernel
> > options visible, making our kernel configuration work even more
> > tedious and error-prone.
> 
> It worked for now, no one complained about having 8250_pci compiled in
> until now.

Maybe because 8250_pci doesn't draw a bunch of dependencies as
8250_lpss and 8250_mid do, and also because it looks less
hardware-specific. Nevertheless, 8250_pci isn't small (53 kiB on my
x86_64 build) and most users don't actually need that driver (think of
laptops, although it seems many of them have a serial port device
enabled despite not offering any physical connector to it, *sigh*), so
let me be the first one to complain about it being build into the
kernel. I'll send a patch.

> > > (...)
> > > No. You forget Intel Quark. It has nothing to do with
> > > X86_INTEL_LPSS.
> > 
> > OK, it wasn't obvious to me. No big deal, we can omit this part of the
> > patch, it was only an "optimization" which I can easily live without.
> 
> Yeah, please, remove those parts from your patches and re-submit. For
> "if EXPERT" part I'm not the one who is supporting it (I remember
> shouting Linus when we had enabled two HSU_DMA options instead of one).
> I leave this to more experienced developers / maintainers.

I reverted these changes and updated the patch descriptions. I'll
resubmit in a minute, thanks for the review.

-- 
Jean Delvare
SUSE L3 Support
--
To unsubscribe from this list: send the line "unsubscribe linux-serial" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html



[Index of Archives]     [Kernel Newbies]     [Security]     [Netfilter]     [Bugtraq]     [Linux PPP]     [Linux FS]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Samba]     [Video 4 Linux]     [Linmodem]     [Device Mapper]     [Linux Kernel for ARM]

  Powered by Linux