awful kconfig help texts.

awful kconfig help texts.

[Dave Jones]

PWM Support (PWM) [N/y/?] (NEW) ?

CONFIG_PWM:

This enables PWM support through the generic PWM framework.


Well that's.. enlightening. I'm picking on PWM here, but this isn't an
isolated case.  Every merge window we see a slew of new options with useless
help texts. They may as well be non-existent. (Actually in some cases, they are).

If someone has to read the code to find out what the driver is, your help text probably sucks.


(I'll leave "why does this option even show up on x86" as a separate rant)

	Dave

Re: awful kconfig help texts.

[Borislav Petkov]

On Tue, Jul 31, 2012 at 11:16:00AM -0400, Dave Jones wrote:
> 
> PWM Support (PWM) [N/y/?] (NEW) ?
> 
> CONFIG_PWM:
> 
> This enables PWM support through the generic PWM framework.
> 
> 
> Well that's.. enlightening.

Oh, there's one more enlightening sentence in the help:

"You only need to enable this, if you also want to enable one or more of
the PWM drivers below."

Got it? :-)

> I'm picking on PWM here, but this isn't an
> isolated case.  Every merge window we see a slew of new options with useless
> help texts. They may as well be non-existent. (Actually in some cases, they are).
> 
> If someone has to read the code to find out what the driver is, your help text probably sucks.
> 
> 
> (I'll leave "why does this option even show up on x86" as a separate rant)

Thierry, can you guys please fix this?

Thanks.

-- 
Regards/Gruss,
Boris.

Re: awful kconfig help texts.

[Steven Rostedt]

On Tue, Jul 31, 2012 at 07:07:41PM +0200, Borislav Petkov wrote:
> On Tue, Jul 31, 2012 at 11:16:00AM -0400, Dave Jones wrote:
> > 
> > PWM Support (PWM) [N/y/?] (NEW) ?
> > 
> > CONFIG_PWM:
> > 
> > This enables PWM support through the generic PWM framework.
> > 
> > 
> > Well that's.. enlightening.
> 
> Oh, there's one more enlightening sentence in the help:
> 
> "You only need to enable this, if you also want to enable one or more of
> the PWM drivers below."

Then shouldn't this not have a prompt and just be selected by those
PWM drivers below?


> 
> Got it? :-)
> 
> > I'm picking on PWM here, but this isn't an
> > isolated case.  Every merge window we see a slew of new options with useless
> > help texts. They may as well be non-existent. (Actually in some cases, they are).
> > 
> > If someone has to read the code to find out what the driver is, your help text probably sucks.
> > 
> > 
> > (I'll leave "why does this option even show up on x86" as a separate rant)

/me is looking forward to this rant ;-)

-- Steve

Re: awful kconfig help texts.

[Borislav Petkov]

On Tue, Jul 31, 2012 at 01:26:38PM -0400, Steven Rostedt wrote:
> Then shouldn't this not have a prompt and just be selected by those
> PWM drivers below?

It gives an empty menu due to the deps of the single PWM drivers.

But the whole CONFIG_PWM thing should simply depend on !X86 so that it
doesn't appear in drivers/.

-- 
Regards/Gruss,
Boris.

Re: awful kconfig help texts.

[Dave Jones]

On Tue, Jul 31, 2012 at 07:07:41PM +0200, Borislav Petkov wrote:
 > On Tue, Jul 31, 2012 at 11:16:00AM -0400, Dave Jones wrote:
 > > 
 > > PWM Support (PWM) [N/y/?] (NEW) ?
 > > 
 > > CONFIG_PWM:
 > > 
 > > This enables PWM support through the generic PWM framework.
 > > 
 > > 
 > > Well that's.. enlightening.
 > 
 > Oh, there's one more enlightening sentence in the help:
 > 
 > "You only need to enable this, if you also want to enable one or more of
 > the PWM drivers below."
 > 
 > Got it? :-)

No, it doesn't add anything useful at all.

I know PWM is probably pulse width modulation, but why would my kernel
need to care about this ? Why would I need drivers for it ?

Also, think about what happens with 'oldconfig'. You don't get to even
see 'the drivers below' until you've answered this question.

It couldn't be more opaque if it tried.

	Dave
 

Re: awful kconfig help texts.

[Borislav Petkov]

On Tue, Jul 31, 2012 at 02:43:13PM -0400, Dave Jones wrote:
> On Tue, Jul 31, 2012 at 07:07:41PM +0200, Borislav Petkov wrote:
>  > Oh, there's one more enlightening sentence in the help:
>  > 
>  > "You only need to enable this, if you also want to enable one or more of
>  > the PWM drivers below."
>  > 
>  > Got it? :-)
> 
> No, it doesn't add anything useful at all.

Sorry, I think you misunderstood me and I didn't express my irony
adequately - I was trying to be ironic, maybe even sarcastic here. But
that's hard to convey in written form.

> I know PWM is probably pulse width modulation, but why would my kernel
> need to care about this ? Why would I need drivers for it ?
>
> Also, think about what happens with 'oldconfig'. You don't get to even
> see 'the drivers below' until you've answered this question.
>
> It couldn't be more opaque if it tried.

I agree with all you've said and you're preaching to the choir. Sorry
that I didn't make my sarcasm more explicit.

Thanks.

-- 
Regards/Gruss,
    Boris.

Re: awful kconfig help texts.

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 1144 bytes --]

On Tue, Jul 31, 2012 at 07:07:41PM +0200, Borislav Petkov wrote:
> On Tue, Jul 31, 2012 at 11:16:00AM -0400, Dave Jones wrote:
> > 
> > PWM Support (PWM) [N/y/?] (NEW) ?
> > 
> > CONFIG_PWM:
> > 
> > This enables PWM support through the generic PWM framework.
> > 
> > 
> > Well that's.. enlightening.
> 
> Oh, there's one more enlightening sentence in the help:
> 
> "You only need to enable this, if you also want to enable one or more of
> the PWM drivers below."
> 
> Got it? :-)
> 
> > I'm picking on PWM here, but this isn't an
> > isolated case.  Every merge window we see a slew of new options with useless
> > help texts. They may as well be non-existent. (Actually in some cases, they are).
> > 
> > If someone has to read the code to find out what the driver is, your help text probably sucks.
> > 
> > 
> > (I'll leave "why does this option even show up on x86" as a separate rant)
> 
> Thierry, can you guys please fix this?

Hehe, those aren't very descriptive, that's true. I was going to go over
the documentation anyway, so I'll make a note to revise the Kconfig help
texts as well.

Thierry

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]

Re: awful kconfig help texts.

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 752 bytes --]

On Tue, Jul 31, 2012 at 07:42:49PM +0200, Borislav Petkov wrote:
> On Tue, Jul 31, 2012 at 01:26:38PM -0400, Steven Rostedt wrote:
> > Then shouldn't this not have a prompt and just be selected by those
> > PWM drivers below?
> 
> It gives an empty menu due to the deps of the single PWM drivers.
> 
> But the whole CONFIG_PWM thing should simply depend on !X86 so that it
> doesn't appear in drivers/.

I don't think that's a good idea. That would mean I would have to add
!SPARC and !S390 and many other to that list as well. Also there are a
couple of drivers that are in the process of being ported which are not
restricted to ARM or embedded in general. So even if we make this !X86
now, eventually it will pop up again.

Thierry

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]

Re: awful kconfig help texts.

[Borislav Petkov]

On Wed, Aug 01, 2012 at 09:47:42AM +0200, Thierry Reding wrote:
> On Tue, Jul 31, 2012 at 07:42:49PM +0200, Borislav Petkov wrote:
> > On Tue, Jul 31, 2012 at 01:26:38PM -0400, Steven Rostedt wrote:
> > > Then shouldn't this not have a prompt and just be selected by those
> > > PWM drivers below?
> > 
> > It gives an empty menu due to the deps of the single PWM drivers.
> > 
> > But the whole CONFIG_PWM thing should simply depend on !X86 so that it
> > doesn't appear in drivers/.
> 
> I don't think that's a good idea. That would mean I would have to add
> !SPARC and !S390 and many other to that list as well. Also there are a
> couple of drivers that are in the process of being ported which are not
> restricted to ARM or embedded in general. So even if we make this !X86
> now, eventually it will pop up again.

Hmm, how about having a synthetic define CONFIG_ARCH_PWM and each arch
which has such a driver can select it and then CONFIG_PWM would depend
on that. Would that even work?

-- 
Regards/Gruss,
    Boris.

Re: awful kconfig help texts.

[Lars-Peter Clausen]

On 08/01/2012 10:56 AM, Borislav Petkov wrote:
> On Wed, Aug 01, 2012 at 09:47:42AM +0200, Thierry Reding wrote:
>> On Tue, Jul 31, 2012 at 07:42:49PM +0200, Borislav Petkov wrote:
>>> On Tue, Jul 31, 2012 at 01:26:38PM -0400, Steven Rostedt wrote:
>>>> Then shouldn't this not have a prompt and just be selected by those
>>>> PWM drivers below?
>>>
>>> It gives an empty menu due to the deps of the single PWM drivers.
>>>
>>> But the whole CONFIG_PWM thing should simply depend on !X86 so that it
>>> doesn't appear in drivers/.
>>
>> I don't think that's a good idea. That would mean I would have to add
>> !SPARC and !S390 and many other to that list as well. Also there are a
>> couple of drivers that are in the process of being ported which are not
>> restricted to ARM or embedded in general. So even if we make this !X86
>> now, eventually it will pop up again.
> 
> Hmm, how about having a synthetic define CONFIG_ARCH_PWM and each arch
> which has such a driver can select it and then CONFIG_PWM would depend
> on that. Would that even work?
> 

One major reason for the new PWM framework is to be able to support arch
independent PWM drivers, like those for companion chips with PWM
capabilities. Restricting the config option to certain architectures
wouldn't work.

- Lars

Re: awful kconfig help texts.

[Borislav Petkov]

On Wed, Aug 01, 2012 at 11:21:59AM +0200, Lars-Peter Clausen wrote:
> One major reason for the new PWM framework is to be able to support
> arch independent PWM drivers, like those for companion chips with PWM
> capabilities. Restricting the config option to certain architectures
> wouldn't work.

Right,

but when I enable the CONFIG_PWM option on x86, I don't see any drivers
there. So the logical thing to do would be to hide that option on arches
which don't have such chips.

-- 
Regards/Gruss,
    Boris.

Re: awful kconfig help texts.

[Jan Engelhardt]

On Wednesday 2012-08-01 09:43, Thierry Reding wrote:
>> > 
>> > PWM Support (PWM) [N/y/?] (NEW) ?
>> > 
>> > CONFIG_PWM:
>> > This enables PWM support through the generic PWM framework.
>> 
>> Oh, there's one more enlightening sentence in the help:
>> 
>> "You only need to enable this, if you also want to enable one or more of
>> the PWM drivers below."
>> 
>> Got it? :-)
>> Thierry, can you guys please fix this?
>
>Hehe, those aren't very descriptive, that's true. I was going to go over
>the documentation anyway, so I'll make a note to revise the Kconfig help
>texts as well.

Also, instead of
	bool "PWM Support"
this should be
	menuconfig "PWM support"

so that you can disable the whole submenu at once.

Re: awful kconfig help texts.

[Lars-Peter Clausen]

On 08/01/2012 11:26 AM, Borislav Petkov wrote:
> On Wed, Aug 01, 2012 at 11:21:59AM +0200, Lars-Peter Clausen wrote:
>> One major reason for the new PWM framework is to be able to support
>> arch independent PWM drivers, like those for companion chips with PWM
>> capabilities. Restricting the config option to certain architectures
>> wouldn't work.
> 
> Right,
> 
> but when I enable the CONFIG_PWM option on x86, I don't see any drivers
> there. So the logical thing to do would be to hide that option on arches
> which don't have such chips.

You don't see any drivers, because the subsystem is still young and no such
arch independent drivers have been added yet, but they will get added in the
future. The arch independent companion or PWM expander chips usually
interface via I2S or SPI and I would consider it quite likely that you'll
also find them on some embedded X86 boards. If we add a arch restriction to
the config item now we'd quite likely have to remove it again in the next
release.

Re: awful kconfig help texts.

[Borislav Petkov]

On Wed, Aug 01, 2012 at 11:38:16AM +0200, Lars-Peter Clausen wrote:
> You don't see any drivers, because the subsystem is still young and no
> such arch independent drivers have been added yet, but they will get
> added in the future. The arch independent companion or PWM expander
> chips usually interface via I2S or SPI and I would consider it quite
> likely that you'll also find them on some embedded X86 boards. If we
> add a arch restriction to the config item now we'd quite likely have
> to remove it again in the next release.

Yes please.

Kconfig is overcrowded as it is now and adding yet another option which
is irrelevant for some arches (for now, as you say) simply causes
confusion to people with absolutely no gain.

Simply take a look at all arch/<archname>/Kconfig files and look at all
the "select ..." statements right at the beginning of the respective
Kconfig file.

Each arch which has PWM drivers would select the synthetic
CONFIG_ARCH_PWM option and CONFIG_PWM would depend on it.

-- 
Regards/Gruss,
    Boris.

Re: awful kconfig help texts.

[Lars-Peter Clausen]

On 08/01/2012 12:04 PM, Borislav Petkov wrote:
> On Wed, Aug 01, 2012 at 11:38:16AM +0200, Lars-Peter Clausen wrote:
>> You don't see any drivers, because the subsystem is still young and no
>> such arch independent drivers have been added yet, but they will get
>> added in the future. The arch independent companion or PWM expander
>> chips usually interface via I2S or SPI and I would consider it quite
>> likely that you'll also find them on some embedded X86 boards. If we
>> add a arch restriction to the config item now we'd quite likely have
>> to remove it again in the next release.
> 
> Yes please.
> 
> Kconfig is overcrowded as it is now and adding yet another option which
> is irrelevant for some arches (for now, as you say) simply causes
> confusion to people with absolutely no gain.
> 
> Simply take a look at all arch/<archname>/Kconfig files and look at all
> the "select ..." statements right at the beginning of the respective
> Kconfig file.

Yes and these select statements make sense, what you suggest though doesn't,
at least from my point of view. What you want is that you don't get
presented the option to select the PWM system if there no PWM driver
available based on your other config options. But whether a PWM driver is
available or not is not a issue of which arch you are building for.

You could do that by using a construct like below, but well...

config HAS_PWM
    bool

config PWM
    bool "PWM
    depends on HAS_PWM

config PWM_CAN_BUILD_DRIVER_X
    defbool I2C
    select HAS_PWM

config PWM_DRIVER_X
    tristate "PWM chip X support"
    depends on PWM && PWM_CAN_BUILD_DRIVER_X

config PWM_CAN_BUILD_DRIVER_Y
    defbool ARCH_Y
    select HAS_PWM

config PWM_DRIVER_Y
    tristate "PWM chip Y support"
    depends on PWM && PWM_CAN_BUILD_DRIVER_Y

Re: awful kconfig help texts.

[Steven Rostedt]

On Wed, 2012-08-01 at 12:56 +0200, Lars-Peter Clausen wrote:

> You could do that by using a construct like below, but well...
> 
> config HAS_PWM
>     bool
> 
> config PWM
>     bool "PWM
>     depends on HAS_PWM
> 
> config PWM_CAN_BUILD_DRIVER_X
>     defbool I2C
>     select HAS_PWM
> 
> config PWM_DRIVER_X
>     tristate "PWM chip X support"
>     depends on PWM && PWM_CAN_BUILD_DRIVER_X
> 
> config PWM_CAN_BUILD_DRIVER_Y
>     defbool ARCH_Y
>     select HAS_PWM
> 
> config PWM_DRIVER_Y
>     tristate "PWM chip Y support"
>     depends on PWM && PWM_CAN_BUILD_DRIVER_Y

What selects the 'PWM_CAN_BUILD_DRIVER_FOO'?

-- Steve

Re: awful kconfig help texts.

[Lars-Peter Clausen]

On 08/01/2012 02:46 PM, Steven Rostedt wrote:
> On Wed, 2012-08-01 at 12:56 +0200, Lars-Peter Clausen wrote:
> 
>> You could do that by using a construct like below, but well...
>>
>> config HAS_PWM
>>     bool
>>
>> config PWM
>>     bool "PWM
>>     depends on HAS_PWM
>>
>> config PWM_CAN_BUILD_DRIVER_X
>>     defbool I2C
>>     select HAS_PWM
>>
>> config PWM_DRIVER_X
>>     tristate "PWM chip X support"
>>     depends on PWM && PWM_CAN_BUILD_DRIVER_X
>>
>> config PWM_CAN_BUILD_DRIVER_Y
>>     defbool ARCH_Y
>>     select HAS_PWM
>>
>> config PWM_DRIVER_Y
>>     tristate "PWM chip Y support"
>>     depends on PWM && PWM_CAN_BUILD_DRIVER_Y
> 
> What selects the 'PWM_CAN_BUILD_DRIVER_FOO'?
> 

Its def_bool statement, which lists the prerequisites to build the driver.
E.g. for a I2C PWM expander I2S support, for a on-SoC PWM core support for
the SoC family, etc.. So it will be true if the driver can actually be built
and false otherwise. If one of the PWM_CAN_BUILD_DRIVER_FOO symbols is true
also HAS_PWM will be true and PWM becomes selectable.

But it seems to be a bid tedious to have these extra lines for each driver
and I guess it is not a PWM subsystem specific issue. There are other
subsystems where this probably applies as well, e.g. the MFD subsystem.
Also such a solution would rule out out-of-tree PWM driver modules, since it
is not possible to get CONFIG_PWM selected.

- Lars

Re: awful kconfig help texts.

[Mark Brown]

On Wed, Aug 01, 2012 at 11:26:25AM +0200, Borislav Petkov wrote:

> but when I enable the CONFIG_PWM option on x86, I don't see any drivers
> there. So the logical thing to do would be to hide that option on arches
> which don't have such chips.

That's because the subsystem was only just added.  As soon as we get
support for platform independant devices the situation will change
(though you may need to turn on some MFD cores).  This is an issue in
general, Kconfig could perhaps be smarter about checking if the submenu
is empty.

Re: awful kconfig help texts.

[Thierry Reding]

[-- Attachment #1.1: Type: text/plain, Size: 1015 bytes --]

On Tue, Jul 31, 2012 at 07:07:41PM +0200, Borislav Petkov wrote:
> On Tue, Jul 31, 2012 at 11:16:00AM -0400, Dave Jones wrote:
> > 
> > PWM Support (PWM) [N/y/?] (NEW) ?
> > 
> > CONFIG_PWM:
> > 
> > This enables PWM support through the generic PWM framework.
> > 
> > 
> > Well that's.. enlightening.
> 
> Oh, there's one more enlightening sentence in the help:
> 
> "You only need to enable this, if you also want to enable one or more of
> the PWM drivers below."
> 
> Got it? :-)
> 
> > I'm picking on PWM here, but this isn't an
> > isolated case.  Every merge window we see a slew of new options with useless
> > help texts. They may as well be non-existent. (Actually in some cases, they are).
> > 
> > If someone has to read the code to find out what the driver is, your help text probably sucks.
> > 
> > 
> > (I'll leave "why does this option even show up on x86" as a separate rant)
> 
> Thierry, can you guys please fix this?

How does the attached patch look?

Thierry

[-- Attachment #1.2: 0001-pwm-Improve-Kconfig-help-text.patch --]
[-- Type: text/plain, Size: 2026 bytes --]

From cd3199b94e697cccd38766b4a60d2e91474d0539 Mon Sep 17 00:00:00 2001
From: Thierry Reding <thierry.reding@avionic-design.de>
Date: Thu, 16 Aug 2012 08:01:21 +0200
Subject: [PATCH] pwm: Improve Kconfig help text

The Kconfig help text should help the user understand what functionality
is provided by an option. This is especially true for new subsystems. An
improved help text is provided by this commit in the hopes of clarifying
the usefulness of the PWM framework.

Signed-off-by: Thierry Reding <thierry.reding@avionic-design.de>
---
 drivers/pwm/Kconfig | 24 ++++++++++++++++++------
 1 file changed, 18 insertions(+), 6 deletions(-)

diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
index 8fc3808..88ddb9e 100644
--- a/drivers/pwm/Kconfig
+++ b/drivers/pwm/Kconfig
@@ -1,12 +1,24 @@
 menuconfig PWM
-	bool "PWM Support"
+	bool "Pulse-Width Modulation (PWM) Support"
 	depends on !MACH_JZ4740 && !PUV3_PWM
 	help
-	  This enables PWM support through the generic PWM framework.
-	  You only need to enable this, if you also want to enable
-	  one or more of the PWM drivers below.
-
-	  If unsure, say N.
+	  Generic Pulse-Width Modulation (PWM) support.
+
+	  This framework provides a generic interface to PWM devices
+	  within the Linux kernel. On the driver side it provides an API
+	  to register and unregister a PWM chip, an abstraction of a PWM
+	  controller, that supports one or more PWM devices. Client
+	  drivers can request PWM devices and use the generic framework
+	  to configure as well as enable and disable them.
+
+	  The new generic framework replaces the legacy PWM framework
+	  which allows only a single driver implementing the required
+	  API. Not all legacy implementations have been ported to the
+	  new framework yet. The new framework provides an API that is
+	  backward compatible with the legacy framework so that existing
+	  client drivers continue to work as expected.
+
+	  If unsure, say no.
 
 if PWM
 
-- 
1.7.11.5


[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]

Re: awful kconfig help texts.

[Borislav Petkov]

On Thu, Aug 16, 2012 at 11:54:39AM +0200, Thierry Reding wrote:
> How does the attached patch look?
> 
> Thierry

> From cd3199b94e697cccd38766b4a60d2e91474d0539 Mon Sep 17 00:00:00 2001
> From: Thierry Reding <thierry.reding@avionic-design.de>
> Date: Thu, 16 Aug 2012 08:01:21 +0200
> Subject: [PATCH] pwm: Improve Kconfig help text
> 
> The Kconfig help text should help the user understand what functionality
> is provided by an option. This is especially true for new subsystems. An
> improved help text is provided by this commit in the hopes of clarifying
> the usefulness of the PWM framework.
> 
> Signed-off-by: Thierry Reding <thierry.reding@avionic-design.de>
> ---
>  drivers/pwm/Kconfig | 24 ++++++++++++++++++------
>  1 file changed, 18 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
> index 8fc3808..88ddb9e 100644
> --- a/drivers/pwm/Kconfig
> +++ b/drivers/pwm/Kconfig
> @@ -1,12 +1,24 @@
>  menuconfig PWM
> -	bool "PWM Support"
> +	bool "Pulse-Width Modulation (PWM) Support"
>  	depends on !MACH_JZ4740 && !PUV3_PWM
>  	help
> -	  This enables PWM support through the generic PWM framework.
> -	  You only need to enable this, if you also want to enable
> -	  one or more of the PWM drivers below.
> -
> -	  If unsure, say N.
> +	  Generic Pulse-Width Modulation (PWM) support.
> +
> +	  This framework provides a generic interface to PWM devices
> +	  within the Linux kernel. On the driver side it provides an API
> +	  to register and unregister a PWM chip, an abstraction of a PWM
> +	  controller, that supports one or more PWM devices. Client
> +	  drivers can request PWM devices and use the generic framework
> +	  to configure as well as enable and disable them.
> +
> +	  The new generic framework replaces the legacy PWM framework
> +	  which allows only a single driver implementing the required
> +	  API. Not all legacy implementations have been ported to the
> +	  new framework yet. The new framework provides an API that is
> +	  backward compatible with the legacy framework so that existing
> +	  client drivers continue to work as expected.
> +
> +	  If unsure, say no.

Yes, this is much better than what we had before. I'd suggest though,
that instead of describing the high-level software design of the PWM
subsystem, the text should be more addressed to the simple user like me
who would like to know what this thing is and why do I want to enable
it. Basically summarize what PWN devices are and what they're used for.
I.e., https://en.wikipedia.org/wiki/Pulse-width_modulation#Applications
but in 4-5 lines of text.

IMHO, of course.

Thanks.

-- 
Regards/Gruss,
Boris.

Re: awful kconfig help texts.

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 3025 bytes --]

On Thu, Aug 16, 2012 at 01:10:44PM +0200, Borislav Petkov wrote:
> On Thu, Aug 16, 2012 at 11:54:39AM +0200, Thierry Reding wrote:
> > How does the attached patch look?
> > 
> > Thierry
> 
> > From cd3199b94e697cccd38766b4a60d2e91474d0539 Mon Sep 17 00:00:00 2001
> > From: Thierry Reding <thierry.reding@avionic-design.de>
> > Date: Thu, 16 Aug 2012 08:01:21 +0200
> > Subject: [PATCH] pwm: Improve Kconfig help text
> > 
> > The Kconfig help text should help the user understand what functionality
> > is provided by an option. This is especially true for new subsystems. An
> > improved help text is provided by this commit in the hopes of clarifying
> > the usefulness of the PWM framework.
> > 
> > Signed-off-by: Thierry Reding <thierry.reding@avionic-design.de>
> > ---
> >  drivers/pwm/Kconfig | 24 ++++++++++++++++++------
> >  1 file changed, 18 insertions(+), 6 deletions(-)
> > 
> > diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
> > index 8fc3808..88ddb9e 100644
> > --- a/drivers/pwm/Kconfig
> > +++ b/drivers/pwm/Kconfig
> > @@ -1,12 +1,24 @@
> >  menuconfig PWM
> > -	bool "PWM Support"
> > +	bool "Pulse-Width Modulation (PWM) Support"
> >  	depends on !MACH_JZ4740 && !PUV3_PWM
> >  	help
> > -	  This enables PWM support through the generic PWM framework.
> > -	  You only need to enable this, if you also want to enable
> > -	  one or more of the PWM drivers below.
> > -
> > -	  If unsure, say N.
> > +	  Generic Pulse-Width Modulation (PWM) support.
> > +
> > +	  This framework provides a generic interface to PWM devices
> > +	  within the Linux kernel. On the driver side it provides an API
> > +	  to register and unregister a PWM chip, an abstraction of a PWM
> > +	  controller, that supports one or more PWM devices. Client
> > +	  drivers can request PWM devices and use the generic framework
> > +	  to configure as well as enable and disable them.
> > +
> > +	  The new generic framework replaces the legacy PWM framework
> > +	  which allows only a single driver implementing the required
> > +	  API. Not all legacy implementations have been ported to the
> > +	  new framework yet. The new framework provides an API that is
> > +	  backward compatible with the legacy framework so that existing
> > +	  client drivers continue to work as expected.
> > +
> > +	  If unsure, say no.
> 
> Yes, this is much better than what we had before. I'd suggest though,
> that instead of describing the high-level software design of the PWM
> subsystem, the text should be more addressed to the simple user like me
> who would like to know what this thing is and why do I want to enable
> it. Basically summarize what PWN devices are and what they're used for.
> I.e., https://en.wikipedia.org/wiki/Pulse-width_modulation#Applications
> but in 4-5 lines of text.
> 
> IMHO, of course.

Okay, I'll add something along those lines in addition to the above.
That way everybody should find something useful in it.

Thanks,
Thierry

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]

Re: awful kconfig help texts.

[Thierry Reding]

[-- Attachment #1.1: Type: text/plain, Size: 3243 bytes --]

On Thu, Aug 16, 2012 at 02:05:41PM +0200, Thierry Reding wrote:
> On Thu, Aug 16, 2012 at 01:10:44PM +0200, Borislav Petkov wrote:
> > On Thu, Aug 16, 2012 at 11:54:39AM +0200, Thierry Reding wrote:
> > > How does the attached patch look?
> > > 
> > > Thierry
> > 
> > > From cd3199b94e697cccd38766b4a60d2e91474d0539 Mon Sep 17 00:00:00 2001
> > > From: Thierry Reding <thierry.reding@avionic-design.de>
> > > Date: Thu, 16 Aug 2012 08:01:21 +0200
> > > Subject: [PATCH] pwm: Improve Kconfig help text
> > > 
> > > The Kconfig help text should help the user understand what functionality
> > > is provided by an option. This is especially true for new subsystems. An
> > > improved help text is provided by this commit in the hopes of clarifying
> > > the usefulness of the PWM framework.
> > > 
> > > Signed-off-by: Thierry Reding <thierry.reding@avionic-design.de>
> > > ---
> > >  drivers/pwm/Kconfig | 24 ++++++++++++++++++------
> > >  1 file changed, 18 insertions(+), 6 deletions(-)
> > > 
> > > diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
> > > index 8fc3808..88ddb9e 100644
> > > --- a/drivers/pwm/Kconfig
> > > +++ b/drivers/pwm/Kconfig
> > > @@ -1,12 +1,24 @@
> > >  menuconfig PWM
> > > -	bool "PWM Support"
> > > +	bool "Pulse-Width Modulation (PWM) Support"
> > >  	depends on !MACH_JZ4740 && !PUV3_PWM
> > >  	help
> > > -	  This enables PWM support through the generic PWM framework.
> > > -	  You only need to enable this, if you also want to enable
> > > -	  one or more of the PWM drivers below.
> > > -
> > > -	  If unsure, say N.
> > > +	  Generic Pulse-Width Modulation (PWM) support.
> > > +
> > > +	  This framework provides a generic interface to PWM devices
> > > +	  within the Linux kernel. On the driver side it provides an API
> > > +	  to register and unregister a PWM chip, an abstraction of a PWM
> > > +	  controller, that supports one or more PWM devices. Client
> > > +	  drivers can request PWM devices and use the generic framework
> > > +	  to configure as well as enable and disable them.
> > > +
> > > +	  The new generic framework replaces the legacy PWM framework
> > > +	  which allows only a single driver implementing the required
> > > +	  API. Not all legacy implementations have been ported to the
> > > +	  new framework yet. The new framework provides an API that is
> > > +	  backward compatible with the legacy framework so that existing
> > > +	  client drivers continue to work as expected.
> > > +
> > > +	  If unsure, say no.
> > 
> > Yes, this is much better than what we had before. I'd suggest though,
> > that instead of describing the high-level software design of the PWM
> > subsystem, the text should be more addressed to the simple user like me
> > who would like to know what this thing is and why do I want to enable
> > it. Basically summarize what PWN devices are and what they're used for.
> > I.e., https://en.wikipedia.org/wiki/Pulse-width_modulation#Applications
> > but in 4-5 lines of text.
> > 
> > IMHO, of course.
> 
> Okay, I'll add something along those lines in addition to the above.
> That way everybody should find something useful in it.

How about the patch below?

Thierry

[-- Attachment #1.2: 0001-pwm-Improve-Kconfig-help-text.patch --]
[-- Type: text/plain, Size: 2393 bytes --]

From deb6852dc5e9da365a33c744f2dc107ae11b407a Mon Sep 17 00:00:00 2001
From: Thierry Reding <thierry.reding@avionic-design.de>
Date: Thu, 16 Aug 2012 08:01:21 +0200
Subject: [PATCH v2] pwm: Improve Kconfig help text

The Kconfig help text should help the user understand what functionality
is provided by an option. This is especially true for new subsystems. An
improved help text is provided by this commit in the hopes of clarifying
the usefulness of the PWM framework.

Signed-off-by: Thierry Reding <thierry.reding@avionic-design.de>
---
 drivers/pwm/Kconfig | 31 +++++++++++++++++++++++++------
 1 file changed, 25 insertions(+), 6 deletions(-)

diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
index 8fc3808..cd602e7 100644
--- a/drivers/pwm/Kconfig
+++ b/drivers/pwm/Kconfig
@@ -1,12 +1,31 @@
 menuconfig PWM
-	bool "PWM Support"
+	bool "Pulse-Width Modulation (PWM) Support"
 	depends on !MACH_JZ4740 && !PUV3_PWM
 	help
-	  This enables PWM support through the generic PWM framework.
-	  You only need to enable this, if you also want to enable
-	  one or more of the PWM drivers below.
-
-	  If unsure, say N.
+	  Generic Pulse-Width Modulation (PWM) support.
+
+	  In Pulse-Width Modulation, a variation of the width of pulses
+	  in a rectangular pulse signal is used as a means to alter the
+	  average power of the signal. Applications include efficient
+	  power delivery and voltage regulation. In computer systems,
+	  PWMs are commonly used to control fans or the brightness of
+	  display backlights.
+
+	  This framework provides a generic interface to PWM devices
+	  within the Linux kernel. On the driver side it provides an API
+	  to register and unregister a PWM chip, an abstraction of a PWM
+	  controller, that supports one or more PWM devices. Client
+	  drivers can request PWM devices and use the generic framework
+	  to configure as well as enable and disable them.
+
+	  The new generic framework replaces the legacy PWM framework
+	  which allows only a single driver implementing the required
+	  API. Not all legacy implementations have been ported to the
+	  new framework yet. The new framework provides an API that is
+	  backward compatible with the legacy framework so that existing
+	  client drivers continue to work as expected.
+
+	  If unsure, say no.
 
 if PWM
 
-- 
1.7.11.5


[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]

Re: awful kconfig help texts.

[Borislav Petkov]

On Fri, Aug 17, 2012 at 08:11:02AM +0200, Thierry Reding wrote:
> How about the patch below?
> 
> Thierry

> From deb6852dc5e9da365a33c744f2dc107ae11b407a Mon Sep 17 00:00:00 2001
> From: Thierry Reding <thierry.reding@avionic-design.de>
> Date: Thu, 16 Aug 2012 08:01:21 +0200
> Subject: [PATCH v2] pwm: Improve Kconfig help text
> 
> The Kconfig help text should help the user understand what functionality
> is provided by an option. This is especially true for new subsystems. An
> improved help text is provided by this commit in the hopes of clarifying
> the usefulness of the PWM framework.
> 
> Signed-off-by: Thierry Reding <thierry.reding@avionic-design.de>

Absolutely! Now even I understand it :-).

Acked-by: Borislav Petkov <bp@alien8.de>

Thanks.

-- 
Regards/Gruss,
    Boris.

Re: awful kconfig help texts.

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 930 bytes --]

On Fri, Aug 17, 2012 at 09:00:42AM +0200, Borislav Petkov wrote:
> On Fri, Aug 17, 2012 at 08:11:02AM +0200, Thierry Reding wrote:
> > How about the patch below?
> > 
> > Thierry
> 
> > From deb6852dc5e9da365a33c744f2dc107ae11b407a Mon Sep 17 00:00:00 2001
> > From: Thierry Reding <thierry.reding@avionic-design.de>
> > Date: Thu, 16 Aug 2012 08:01:21 +0200
> > Subject: [PATCH v2] pwm: Improve Kconfig help text
> > 
> > The Kconfig help text should help the user understand what functionality
> > is provided by an option. This is especially true for new subsystems. An
> > improved help text is provided by this commit in the hopes of clarifying
> > the usefulness of the PWM framework.
> > 
> > Signed-off-by: Thierry Reding <thierry.reding@avionic-design.de>
> 
> Absolutely! Now even I understand it :-).
> 
> Acked-by: Borislav Petkov <bp@alien8.de>

Thanks, I'll queue this for 3.6-rc3.

Thierry

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]

Re: awful kconfig help texts.

[Cam Hutchison]

>How about the patch below?

>[...]
>+	  Generic Pulse-Width Modulation (PWM) support.
>+
>+	  In Pulse-Width Modulation, a variation of the width of pulses
>+	  in a rectangular pulse signal is used as a means to alter the
>+	  average power of the signal. Applications include efficient
>+	  power delivery and voltage regulation. In computer systems,
>+	  PWMs are commonly used to control fans or the brightness of
>+	  display backlights.
>+
>+	  This framework provides a generic interface to PWM devices
>+	  within the Linux kernel. On the driver side it provides an API
>+	  to register and unregister a PWM chip, an abstraction of a PWM
>+	  controller, that supports one or more PWM devices. Client
>+	  drivers can request PWM devices and use the generic framework
>+	  to configure as well as enable and disable them.
>+
>+	  The new generic framework replaces the legacy PWM framework
>+	  which allows only a single driver implementing the required
>+	  API. Not all legacy implementations have been ported to the
>+	  new framework yet. The new framework provides an API that is
>+	  backward compatible with the legacy framework so that existing
>+	  client drivers continue to work as expected.

Instead of writing "[Tt]he new framework", write "[Tt]his framework". In 4
years, this will no longer be so new. And when the next framework comes
along, this will look like it is referencing the new one instead of itself.