[PATCH] docs: leds: uleds: Make the documentation match the code.

[PATCH] docs: leds: uleds: Make the documentation match the code.

[Björn Persson]

From: Björn Persson <Bjorn@Rombobjörn.se>

· max_brightness must be set. Leaving it uninitialized or just omitting it
  won't work.

· The maximum brightness is not 255 but the value given to max_brightness.

· Brightness values must be read as ints, not bytes.

· The ints are signed, so the word "unsigned" is misleading.

Signed-off-by: Björn Persson <Bjorn@Rombobjörn.se>
---
 Documentation/leds/uleds.rst | 18 +++++++++++-------
 1 file changed, 11 insertions(+), 7 deletions(-)

diff --git a/Documentation/leds/uleds.rst b/Documentation/leds/uleds.rst
index 83221098009c..9875a0fa4185 100644
--- a/Documentation/leds/uleds.rst
+++ b/Documentation/leds/uleds.rst
@@ -17,16 +17,20 @@ structure to it (found in kernel public header file linux/uleds.h)::
 
     struct uleds_user_dev {
 	char name[LED_MAX_NAME_SIZE];
+	int max_brightness;
     };
 
-A new LED class device will be created with the name given. The name can be
-any valid sysfs device node name, but consider using the LED class naming
-convention of "devicename:color:function".
+A new LED class device will be created with the given name and maximum
+brightness. The name can be any valid sysfs device node name, but consider
+using the LED class naming convention of "devicename:color:function".
 
-The current brightness is found by reading a single byte from the character
-device. Values are unsigned: 0 to 255. Reading will block until the brightness
-changes. The device node can also be polled to notify when the brightness value
-changes.
+Although max_brightness is a signed int, only positive values are valid:
+1 to INT_MAX.
+
+The current brightness is found by reading a whole int from the character
+device. The possible values are 0 to max_brightness. Reading will block until
+the brightness changes. The device node can also be polled to notify when the
+brightness value changes.
 
 The LED class device will be removed when the open file handle to /dev/uleds
 is closed.
-- 
2.53.0

Re: [PATCH] docs: leds: uleds: Make the documentation match the code.

[Lee Jones]

On Thu, 02 Apr 2026, Björn Persson wrote:

> From: Björn Persson <Bjorn@Rombobjörn.se>
> 
> · max_brightness must be set. Leaving it uninitialized or just omitting it
>   won't work.

What are these points?  How do you even type one of those?

Anyway, proper sentences / paragraphs is better.

> · The maximum brightness is not 255 but the value given to max_brightness.
> 
> · Brightness values must be read as ints, not bytes.
> 
> · The ints are signed, so the word "unsigned" is misleading.
> 
> Signed-off-by: Björn Persson <Bjorn@Rombobjörn.se>
> ---
>  Documentation/leds/uleds.rst | 18 +++++++++++-------
>  1 file changed, 11 insertions(+), 7 deletions(-)
> 
> diff --git a/Documentation/leds/uleds.rst b/Documentation/leds/uleds.rst
> index 83221098009c..9875a0fa4185 100644
> --- a/Documentation/leds/uleds.rst
> +++ b/Documentation/leds/uleds.rst
> @@ -17,16 +17,20 @@ structure to it (found in kernel public header file linux/uleds.h)::
>  
>      struct uleds_user_dev {
>  	char name[LED_MAX_NAME_SIZE];
> +	int max_brightness;
>      };
>  
> -A new LED class device will be created with the name given. The name can be
> -any valid sysfs device node name, but consider using the LED class naming
> -convention of "devicename:color:function".
> +A new LED class device will be created with the given name and maximum

Did you mean to revers "name given"?  A "given name" usually means
something else.

> +brightness. The name can be any valid sysfs device node name, but consider
> +using the LED class naming convention of "devicename:color:function".
>  
> -The current brightness is found by reading a single byte from the character
> -device. Values are unsigned: 0 to 255. Reading will block until the brightness
> -changes. The device node can also be polled to notify when the brightness value
> -changes.
> +Although max_brightness is a signed int, only positive values are valid:
> +1 to INT_MAX.

What about 0?

> +The current brightness is found by reading a whole int from the character

Try not to shorten names in documentation "integer".

Why do we need to specify "whole"?

> +device. The possible values are 0 to max_brightness. Reading will block until
> +the brightness changes. The device node can also be polled to notify when the
> +brightness value changes.
>  
>  The LED class device will be removed when the open file handle to /dev/uleds
>  is closed.
> -- 
> 2.53.0
> 

-- 
Lee Jones [李琼斯]

Re: [PATCH] docs: leds: uleds: Make the documentation match the code.

[Björn Persson]

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

Lee Jones wrote:
> On Thu, 02 Apr 2026, Björn Persson wrote:
> 
> > From: Björn Persson <Bjorn@Rombobjörn.se>
> > 
> > · max_brightness must be set. Leaving it uninitialized or just omitting it
> >   won't work.  
> 
> What are these points?  How do you even type one of those?

The bullet point is the character U+00B7 middle dot. I type it with
AltGr-period on a Swedish keyboard in Fedora. I don't know what keymap
your distro uses for your keyboard. I hear some keyboards lack an AltGr
key.

> Anyway, proper sentences / paragraphs is better.

You mean you dislike bulleted lists?

Or if you mean that the first sentence doesn't begin with a capital M,
that's because identifiers are case-sensitive in C. There is no
Max_brightness, and if it were defined, it would be different from
max_brightness.

Otherwise I don't understand what you mean with "proper sentences", as
I don't see any grammatical errors.

> > -A new LED class device will be created with the name given. The name can be
> > -any valid sysfs device node name, but consider using the LED class naming
> > -convention of "devicename:color:function".
> > +A new LED class device will be created with the given name and maximum  
> 
> Did you mean to revers "name given"?  A "given name" usually means
> something else.

I felt that "the name and maximum brightness given" would be
grammatically awkward.

To prevent misinterpretation, how about replacing "given" with a
synonym? Perhaps "the specified name and maximum brightness"? Another
option is "the given maximum brightness and name", but it feels a
little odd to mention the brightness before the name.

> > +Although max_brightness is a signed int, only positive values are valid:
> > +1 to INT_MAX.  
> 
> What about 0?

That will get you an EINVAL from uleds.c – presumably because a
brightness interval from 0 to 0 would be pointless. That LED would never
be lit.

> > +The current brightness is found by reading a whole int from the character  
> 
> Try not to shorten names in documentation "integer".

The type is named "int" in C. There are many integer types, but it would
be wrong to try to read a uint16_t or a size_t or any other integer
type. The document needs to use the actual type name to make it clear to
the reader that they must read sizeof(int) bytes.

> Why do we need to specify "whole"?

Because you can't read it piecemeal. Usually when you read from a disk
file, a pipe, a TCP socket or some other bytestream, the system call
will let you read one byte at a time if you want. A reader might assume
that /dev/uleds works the same way.

From a datagram socket you can read the beginning of a datagram and
discard the part that doesn't fit in your buffer. To a reader with a
little-endian system and max_brightness ≤ 255, it might seem logical
that they'd be able to read the first byte and discard the bits that
will always be zero.

I thought "whole" would communicate to the reader that they must read
sizeof(int) bytes in a single system call.

It seems this wording wasn't enough to get the point across that it's
necessary to read an int, a whole int, and nothing but an int. Do you
think the document needs to expound that point more?

Björn Persson

[-- Attachment #2: OpenPGP digital signatur --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH] docs: leds: uleds: Make the documentation match the code.

[Lee Jones]

On Fri, 24 Apr 2026, Björn Persson wrote:

> Lee Jones wrote:
> > On Thu, 02 Apr 2026, Björn Persson wrote:
> > 
> > > From: Björn Persson <Bjorn@Rombobjörn.se>
> > > 
> > > · max_brightness must be set. Leaving it uninitialized or just omitting it
> > >   won't work.  
> > 
> > What are these points?  How do you even type one of those?
> 
> The bullet point is the character U+00B7 middle dot. I type it with
> AltGr-period on a Swedish keyboard in Fedora. I don't know what keymap
> your distro uses for your keyboard. I hear some keyboards lack an AltGr
> key.

Please use ASCII or you'll freak out the Luddites (like me)!

- or * is fine.

> > Anyway, proper sentences / paragraphs is better.
> 
> You mean you dislike bulleted lists?
> 
> Or if you mean that the first sentence doesn't begin with a capital M,
> that's because identifiers are case-sensitive in C. There is no
> Max_brightness, and if it were defined, it would be different from
> max_brightness.
> 
> Otherwise I don't understand what you mean with "proper sentences", as
> I don't see any grammatical errors.

By all means use bullet points for lists, but a slab list of changes in
place of sentences is a little odd.  Simply describe your changes as you
would speak to another human.

> > > -A new LED class device will be created with the name given. The name can be
> > > -any valid sysfs device node name, but consider using the LED class naming
> > > -convention of "devicename:color:function".
> > > +A new LED class device will be created with the given name and maximum  
> > 
> > Did you mean to revers "name given"?  A "given name" usually means
> > something else.
> 
> I felt that "the name and maximum brightness given" would be
> grammatically awkward.
> 
> To prevent misinterpretation, how about replacing "given" with a
> synonym? Perhaps "the specified name and maximum brightness"? Another

This is nice.

> option is "the given maximum brightness and name", but it feels a
> little odd to mention the brightness before the name.
> 
> > > +Although max_brightness is a signed int, only positive values are valid:
> > > +1 to INT_MAX.  
> > 
> > What about 0?
> 
> That will get you an EINVAL from uleds.c – presumably because a
> brightness interval from 0 to 0 would be pointless. That LED would never
> be lit.

Ah, this is MAX brightness.

Okay so it's impossible to set a LED to always off.

> > > +The current brightness is found by reading a whole int from the character  
> > 
> > Try not to shorten names in documentation "integer".
> 
> The type is named "int" in C. There are many integer types, but it would
> be wrong to try to read a uint16_t or a size_t or any other integer
> type. The document needs to use the actual type name to make it clear to
> the reader that they must read sizeof(int) bytes.

Right, but you're not writing in C.

> > Why do we need to specify "whole"?
> 
> Because you can't read it piecemeal. Usually when you read from a disk
> file, a pipe, a TCP socket or some other bytestream, the system call
> will let you read one byte at a time if you want. A reader might assume
> that /dev/uleds works the same way.
> 
> From a datagram socket you can read the beginning of a datagram and
> discard the part that doesn't fit in your buffer. To a reader with a
> little-endian system and max_brightness ≤ 255, it might seem logical
> that they'd be able to read the first byte and discard the bits that
> will always be zero.
> 
> I thought "whole" would communicate to the reader that they must read
> sizeof(int) bytes in a single system call.
> 
> It seems this wording wasn't enough to get the point across that it's
> necessary to read an int, a whole int, and nothing but an int. Do you
> think the document needs to expound that point more?

I think it needs rephrasing a little.

  "Current brightness is obtained from the character device.  It is
  read in as an integer and must be done so in one go.

Or words to that effect.

-- 
Lee Jones

Re: [PATCH] docs: leds: uleds: Make the documentation match the code.

[Björn Persson]

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

Lee Jones wrote:
> On Fri, 24 Apr 2026, Björn Persson wrote:
> 
> > Lee Jones wrote:  
> > > On Thu, 02 Apr 2026, Björn Persson wrote:
> > >   
> > > > +The current brightness is found by reading a whole int from the character    
> > > 
> > > Try not to shorten names in documentation "integer".  
> > 
> > The type is named "int" in C. There are many integer types, but it would
> > be wrong to try to read a uint16_t or a size_t or any other integer
> > type. The document needs to use the actual type name to make it clear to
> > the reader that they must read sizeof(int) bytes.  
> 
> Right, but you're not writing in C.

That's technically true, as I wrote my program in C++. It's far from my
favorite, but I had to use a language that can include C header files
and use C types, because /dev/uleds is a very C-centric interface.

If API documentation isn't allowed to name a type, then I withdraw the
patch. It's pointless to continue. The next programmer will also have to
read the code to find out what the true API is, like I did.

Björn Persson

[-- Attachment #2: OpenPGP digital signatur --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH] docs: leds: uleds: Make the documentation match the code.

[Lee Jones]

On Sun, 10 May 2026, Björn Persson wrote:

> Lee Jones wrote:
> > On Fri, 24 Apr 2026, Björn Persson wrote:
> > 
> > > Lee Jones wrote:  
> > > > On Thu, 02 Apr 2026, Björn Persson wrote:
> > > >   
> > > > > +The current brightness is found by reading a whole int from the character    
> > > > 
> > > > Try not to shorten names in documentation "integer".  
> > > 
> > > The type is named "int" in C. There are many integer types, but it would
> > > be wrong to try to read a uint16_t or a size_t or any other integer
> > > type. The document needs to use the actual type name to make it clear to
> > > the reader that they must read sizeof(int) bytes.  
> > 
> > Right, but you're not writing in C.
> 
> That's technically true, as I wrote my program in C++. It's far from my
> favorite, but I had to use a language that can include C header files
> and use C types, because /dev/uleds is a very C-centric interface.
> 
> If API documentation isn't allowed to name a type, then I withdraw the
> patch. It's pointless to continue. The next programmer will also have to
> read the code to find out what the true API is, like I did.

It's not that it's "not allowed".  90% of my review comments are
suggestions.  These ones are for simply for the sake of readability.

Equally, I'm also not coming up for blackmail or hissy fits.  If you
don't want to continue with the review process, no one is going to beg
you.

-- 
Lee Jones