Re: [PATCH v6][RESEND] platform: x86: New BayTrail IOSF-SB MBI driver

[Rafael J. Wysocki]

On Tuesday, January 07, 2014 01:43:00 PM David E. Box wrote:
> On Tue, Jan 07, 2014 at 09:46:57PM +0100, Rafael J. Wysocki wrote:
> > On Tuesday, January 07, 2014 10:48:05 AM David E. Box wrote:
> > > On Tue, Jan 07, 2014 at 10:15:03AM -0800, Randy Dunlap wrote:
> > > > On 01/07/14 10:03, David E. Box wrote:
> > > > > From: "David E. Box" <david.e.box@xxxxxxxxxxxxxxx>
> > > > > 
> > > > > diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
> > > > > index b51a746..6e199a5 100644
> > > > > --- a/drivers/platform/x86/Kconfig
> > > > > +++ b/drivers/platform/x86/Kconfig
> > > > > @@ -819,4 +819,14 @@ config PVPANIC
> > > > >  	  a paravirtualized device provided by QEMU; it lets a virtual machine
> > > > >  	  (guest) communicate panic events to the host.
> > > > >  
> > > > > +config INTEL_BAYTRAIL_MBI
> > > > > +	tristate
> > > > 
> > > > Is this kconfig option displayed when you run menuconfig/nconfig/xconfig/gconfig etc.?
> > > > Doesn't it need a prompt string?
> > > > How did you enable it and test it?
> > > > 
> > > 
> > > It is not displayed on purpose. The driver isn't exposed to user space.
> > > It was tested by adding the option to .config, both as module and built-in.
> > > 
> > > > > +	depends on PCI
> > > > > +	---help---
> > > > > +	  Needed on Baytrail platforms for access to the IOSF Sideband Mailbox
> > > > > +	  Interface. This is a requirement for systems that need to configure
> > > > > +	  the PUNIT for power management features such as RAPL. Register
> > > > > +	  addresses and r/w opcodes are defined in
> > > > 
> > > > Think of users reading this.  At least change "r/w" to read/write.
> > > > What is IOSF?  does it matter here?
> > > > PUNIT?  RAPL?
> > > > 
> > > 
> > > If you don't know what the IOSF Sideband is you probably shouldn't be enabling
> > > this feature.
> > 
> > What about generic x86 distro kernels?  They won't know in advance whether or
> > not they will need this feature.
> 
> Ok, I spoke with other developers and I apparently misunderstood the context
> here. Distro's enable these features and this is too detailed for them to know
> what to do with it. How about simply "Required to enable platform specific power
> managemnet features on Baytrail"?
> 
> KISS is easier said than done.

Well, I personally think that this code should go into arch/x86/ as library code
needed to access IOSF Sideband on some platforms.  I probably would make modules
depending on it select it, so for example if the RAPL driver is going to be
built, your driver should be build either and rather unconditionally in that
case.

So the rule should be "if something *may* need it, build it" in my opinion.

Thanks!

-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/