[RESEND LEDs] leds: remove led_brightness

[RESEND LEDs] leds: remove led_brightness

[Guilherme Giácomo Simões]

Hi community, I hope this email finds you well
I maked a change in kernel linux, for fulfill a TODO in
drivers/leds/TODO that say:
>* On/off LEDs should have max_brightness of 1
>* Get rid of enum led_brightness
>
>It is really an integer, as maximum is configurable. Get rid of it, or
>make it into typedef or something.

Then I removed the led_brightness. And in the files that enum
led_brightness was used i replace to "int" ... And in the file
"include/linux/leds.h" I created constants like:
+#define LED_OFF  0
+#define LED_ON   1
+#define LED_HALF 127
+#define LED_FULL 255

because in some files when has a compare like "brightness == LED_OFF"
it will work yet.

The includes/linux/leds.h diff:
-/* This is obsolete/useless. We now support variable maximum brightness. */
-enum led_brightness {
-       LED_OFF         = 0,
-       LED_ON          = 1,
-       LED_HALF        = 127,
-       LED_FULL        = 255,
-};
+// default values for leds brightness
+#define LED_OFF  0
+#define LED_ON   1
+#define LED_HALF 127
+#define LED_FULL 255

 enum led_default_state {
        LEDS_DEFSTATE_OFF       = 0,
@@ -127,15 +125,15 @@ struct led_classdev {
         * that can sleep while setting brightness.
         */
        void            (*brightness_set)(struct led_classdev *led_cdev,
-                                         enum led_brightness brightness);
+                                         int brightness);
        /*
         * Set LED brightness level immediately - it can block the caller for
         * the time required for accessing a LED device register.
         */
        int (*brightness_set_blocking)(struct led_classdev *led_cdev,
-                                      enum led_brightness brightness);
+                                      int brightness);
        /* Get LED brightness level */
-       enum led_brightness (*brightness_get)(struct led_classdev *led_cdev);
+       int (*brightness_get)(struct led_classdev *led_cdev);

        /*
         * Activate hardware accelerated blink, delays are in milliseconds
@@ -486,7 +484,7 @@ int devm_led_trigger_register(struct device *dev,
 void led_trigger_register_simple(const char *name,
                                struct led_trigger **trigger);
 void led_trigger_unregister_simple(struct led_trigger *trigger);
-void led_trigger_event(struct led_trigger *trigger,  enum
led_brightness event);
+void led_trigger_event(struct led_trigger *trigger,  int event);
 void led_trigger_blink(struct led_trigger *trigger, unsigned long delay_on,
                       unsigned long delay_off);



How the kernel has a lot of files that use this led_brightness, the
change is very very big.
I have some questions:

Does my change make sense?

How can I split the change into several patches for sending to
different email lists and still have the split change make sense in
the email lists I'm going to send it to? can I reference the commit in
which I delete the enum?

Re: [RESEND LEDs] leds: remove led_brightness

[Lee Jones]

On Sun, 21 Jul 2024, Guilherme Giácomo Simões wrote:

> Hi community, I hope this email finds you well
> I maked a change in kernel linux, for fulfill a TODO in
> drivers/leds/TODO that say:
> >* On/off LEDs should have max_brightness of 1
> >* Get rid of enum led_brightness
> >
> >It is really an integer, as maximum is configurable. Get rid of it, or
> >make it into typedef or something.
> 
> Then I removed the led_brightness. And in the files that enum
> led_brightness was used i replace to "int" ... And in the file
> "include/linux/leds.h" I created constants like:
> +#define LED_OFF  0
> +#define LED_ON   1
> +#define LED_HALF 127
> +#define LED_FULL 255
> 
> because in some files when has a compare like "brightness == LED_OFF"
> it will work yet.
> 
> The includes/linux/leds.h diff:
> -/* This is obsolete/useless. We now support variable maximum brightness. */
> -enum led_brightness {
> -       LED_OFF         = 0,
> -       LED_ON          = 1,
> -       LED_HALF        = 127,
> -       LED_FULL        = 255,
> -};

I'm not aware of the history of this, however I'm even less sure how
converting these from an enum to #defines makes this any better.

> +// default values for leds brightness
> +#define LED_OFF  0
> +#define LED_ON   1
> +#define LED_HALF 127
> +#define LED_FULL 255
> 
>  enum led_default_state {
>         LEDS_DEFSTATE_OFF       = 0,
> @@ -127,15 +125,15 @@ struct led_classdev {
>          * that can sleep while setting brightness.
>          */
>         void            (*brightness_set)(struct led_classdev *led_cdev,
> -                                         enum led_brightness brightness);
> +                                         int brightness);
>         /*
>          * Set LED brightness level immediately - it can block the caller for
>          * the time required for accessing a LED device register.
>          */
>         int (*brightness_set_blocking)(struct led_classdev *led_cdev,
> -                                      enum led_brightness brightness);
> +                                      int brightness);
>         /* Get LED brightness level */
> -       enum led_brightness (*brightness_get)(struct led_classdev *led_cdev);
> +       int (*brightness_get)(struct led_classdev *led_cdev);
> 
>         /*
>          * Activate hardware accelerated blink, delays are in milliseconds
> @@ -486,7 +484,7 @@ int devm_led_trigger_register(struct device *dev,
>  void led_trigger_register_simple(const char *name,
>                                 struct led_trigger **trigger);
>  void led_trigger_unregister_simple(struct led_trigger *trigger);
> -void led_trigger_event(struct led_trigger *trigger,  enum
> led_brightness event);
> +void led_trigger_event(struct led_trigger *trigger,  int event);
>  void led_trigger_blink(struct led_trigger *trigger, unsigned long delay_on,
>                        unsigned long delay_off);
> 
> 
> 
> How the kernel has a lot of files that use this led_brightness, the
> change is very very big.
> I have some questions:
> 
> Does my change make sense?
> 
> How can I split the change into several patches for sending to
> different email lists and still have the split change make sense in
> the email lists I'm going to send it to? can I reference the commit in
> which I delete the enum?
> 

-- 
Lee Jones [李琼斯]

Re: [RESEND LEDs] leds: remove led_brightness

[Guilherme Giácomo Simões]

 Lee Jones <lee@kernel.org> writes:

> >
> > The includes/linux/leds.h diff:
> > -/* This is obsolete/useless. We now support variable maximum brightness. */
> > -enum led_brightness {
> > -       LED_OFF         = 0,
> > -       LED_ON          = 1,
> > -       LED_HALF        = 127,
> > -       LED_FULL        = 255,
> > -};
> > +// default values for leds brightness
> > +#define LED_OFF  0
> > +#define LED_ON   1
> > +#define LED_HALF 127
> > +#define LED_FULL 255
> >
> I'm not aware of the history of this, however I'm even less sure how
> converting these from an enum to #defines makes this any better.
>

Yeah. The TODO says:
-* On/off LEDs should have max_brightness of 1
-* Get rid of enum led_brightness
-
-It is really an integer, as maximum is configurable. Get rid of it, or
-make it into typedef or something.

I could only remove enum led_brightness, but in some places, the
LED_FULL, LED_HALF...
are used. This is why I created this #defines.
I don't know what we can do in these cases that LED_FULL, FULL_HALF is used.
For example, in the drivers/leds/leds-ss4200.c on line 223 have this code

u32 setting = 0;
if (brightness >= LED_HALF)
    setting = 1;

In TODO where say "LEDs sould have max_brightness of 1" , I don't know
how this can work.
Because if the file needs to test if led brightness is half, the
integer number does not work in these cases
because, 0.5 (half of 1) is a double/float number.

If the max brightness is configurable, what drivers can do this?

Re: [RESEND LEDs] leds: remove led_brightness

[Pavel Machek]

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

Hi!

> > > The includes/linux/leds.h diff:
> > > -/* This is obsolete/useless. We now support variable maximum brightness. */
> > > -enum led_brightness {
> > > -       LED_OFF         = 0,
> > > -       LED_ON          = 1,
> > > -       LED_HALF        = 127,
> > > -       LED_FULL        = 255,
> > > -};
> > > +// default values for leds brightness
> > > +#define LED_OFF  0
> > > +#define LED_ON   1
> > > +#define LED_HALF 127
> > > +#define LED_FULL 255
> > >
> > I'm not aware of the history of this, however I'm even less sure how
> > converting these from an enum to #defines makes this any better.
> >
> 
> Yeah. The TODO says:
> -* On/off LEDs should have max_brightness of 1
> -* Get rid of enum led_brightness
> -
> -It is really an integer, as maximum is configurable. Get rid of it, or
> -make it into typedef or something.
> 
> I could only remove enum led_brightness, but in some places, the
> LED_FULL, LED_HALF...
> are used. This is why I created this #defines.
> I don't know what we can do in these cases that LED_FULL, FULL_HALF is used.
> For example, in the drivers/leds/leds-ss4200.c on line 223 have this code
> 
> u32 setting = 0;
> if (brightness >= LED_HALF)
>     setting = 1;

Yep. Such drivers should be modified to set max_brightness to real
number of steps hardware can do... then we can remove LED_HALF,
LED_FULL and such defines.

Best regards,
								Pavel
-- 
People of Russia, stop Putin before his war on Ukraine escalates.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

Re: [RESEND LEDs] leds: remove led_brightness

[Guilherme Giácomo Simões]

Pavel Machek <pavel@ucw.cz> writes:
>
> Hi!
>
> > > > The includes/linux/leds.h diff:
> > > > -/* This is obsolete/useless. We now support variable maximum brightness. */
> > > > -enum led_brightness {
> > > > -       LED_OFF         = 0,
> > > > -       LED_ON          = 1,
> > > > -       LED_HALF        = 127,
> > > > -       LED_FULL        = 255,
> > > > -};
> > > > +// default values for leds brightness
> > > > +#define LED_OFF  0
> > > > +#define LED_ON   1
> > > > +#define LED_HALF 127
> > > > +#define LED_FULL 255
> > > >
> > > I'm not aware of the history of this, however I'm even less sure how
> > > converting these from an enum to #defines makes this any better.
> > >
> >
> > Yeah. The TODO says:
> > -* On/off LEDs should have max_brightness of 1
> > -* Get rid of enum led_brightness
> > -
> > -It is really an integer, as maximum is configurable. Get rid of it, or
> > -make it into typedef or something.
> >
> > I could only remove enum led_brightness, but in some places, the
> > LED_FULL, LED_HALF...
> > are used. This is why I created this #defines.
> > I don't know what we can do in these cases that LED_FULL, FULL_HALF is used.
> > For example, in the drivers/leds/leds-ss4200.c on line 223 have this code
> >
> > u32 setting = 0;
> > if (brightness >= LED_HALF)
> >     setting = 1;
>
> Yep. Such drivers should be modified to set max_brightness to real
> number of steps hardware can do... then we can remove LED_HALF,
> LED_FULL and such defines.
>
> Best regards,
>                                                                 Pavel

but this will require the effort of everyone who has already written drivers
for some LED hardware. Because only the driver author himself will know
all the steps for that specific LED.

or, maybe we can adapt this drivers for understand the brightness as a 0 or 1.
0 for OFF and 1 for HALF and FULL. This is possble ?

Re: [RESEND LEDs] leds: remove led_brightness

[Guilherme Giácomo Simões]

In the drivers/leds/leds-clevo-mail.c for example, the LED_HALF is
used on line 102.
In drivers/leds/leds-pca955x.c the LED_HALF is used in line 260.

How can I remove these keywords if manufacturers still use them?

Em qui., 25 de jul. de 2024 às 10:07, Guilherme Giácomo Simões
<trintaeoitogc@gmail.com> escreveu:

>
> Pavel Machek <pavel@ucw.cz> writes:
> >
> > Hi!
> >
> > > > > The includes/linux/leds.h diff:
> > > > > -/* This is obsolete/useless. We now support variable maximum brightness. */
> > > > > -enum led_brightness {
> > > > > -       LED_OFF         = 0,
> > > > > -       LED_ON          = 1,
> > > > > -       LED_HALF        = 127,
> > > > > -       LED_FULL        = 255,
> > > > > -};
> > > > > +// default values for leds brightness
> > > > > +#define LED_OFF  0
> > > > > +#define LED_ON   1
> > > > > +#define LED_HALF 127
> > > > > +#define LED_FULL 255
> > > > >
> > > > I'm not aware of the history of this, however I'm even less sure how
> > > > converting these from an enum to #defines makes this any better.
> > > >
> > >
> > > Yeah. The TODO says:
> > > -* On/off LEDs should have max_brightness of 1
> > > -* Get rid of enum led_brightness
> > > -
> > > -It is really an integer, as maximum is configurable. Get rid of it, or
> > > -make it into typedef or something.
> > >
> > > I could only remove enum led_brightness, but in some places, the
> > > LED_FULL, LED_HALF...
> > > are used. This is why I created this #defines.
> > > I don't know what we can do in these cases that LED_FULL, FULL_HALF is used.
> > > For example, in the drivers/leds/leds-ss4200.c on line 223 have this code
> > >
> > > u32 setting = 0;
> > > if (brightness >= LED_HALF)
> > >     setting = 1;
> >
> > Yep. Such drivers should be modified to set max_brightness to real
> > number of steps hardware can do... then we can remove LED_HALF,
> > LED_FULL and such defines.
> >
> > Best regards,
> >                                                                 Pavel
>
> but this will require the effort of everyone who has already written drivers
> for some LED hardware. Because only the driver author himself will know
> all the steps for that specific LED.
>
> or, maybe we can adapt this drivers for understand the brightness as a 0 or 1.
> 0 for OFF and 1 for HALF and FULL. This is possble ?

Re: [RESEND LEDs] leds: remove led_brightness

[Guilherme Giácomo Simões]

Guilherme Giácomo Simões <trintaeoitogc@gmail.com> writes:
>
> Pavel Machek <pavel@ucw.cz> writes:
> >
> > Hi!
> > ,,,,
> > Yep. Such drivers should be modified to set max_brightness to real
> > number of steps hardware can do... then we can remove LED_HALF,
> > LED_FULL and such defines.
> >
> > Best regards,
> >                                                                 Pavel
>
> but this will require the effort of everyone who has already written drivers
> for some LED hardware. Because only the driver author himself will know
> all the steps for that specific LED.
>
> or, maybe we can adapt this drivers for understand the brightness as a 0 or 1.
> 0 for OFF and 1 for HALF and FULL. This is possble ?

If the manufacturers of drivers use the LED_FULL , LED_HALF , like in the
drivers/leds/leds-pca955x.c  on line 254 , how can I remove this ?

In some files, the driver uses the LED_FULL for make arithmetics

Re: [RESEND LEDs] leds: remove led_brightness

[Lee Jones]

Please do not top-post (moving your comments down to the bottom).

On Mon, 29 Jul 2024, Guilherme Giácomo Simões wrote:
> Em qui., 25 de jul. de 2024 às 10:07, Guilherme Giácomo Simões
> <trintaeoitogc@gmail.com> escreveu:
> 
> >
> > Pavel Machek <pavel@ucw.cz> writes:
> > >
> > > Hi!
> > >
> > > > > > The includes/linux/leds.h diff:
> > > > > > -/* This is obsolete/useless. We now support variable maximum brightness. */
> > > > > > -enum led_brightness {
> > > > > > -       LED_OFF         = 0,
> > > > > > -       LED_ON          = 1,
> > > > > > -       LED_HALF        = 127,
> > > > > > -       LED_FULL        = 255,
> > > > > > -};
> > > > > > +// default values for leds brightness
> > > > > > +#define LED_OFF  0
> > > > > > +#define LED_ON   1
> > > > > > +#define LED_HALF 127
> > > > > > +#define LED_FULL 255
> > > > > >
> > > > > I'm not aware of the history of this, however I'm even less sure how
> > > > > converting these from an enum to #defines makes this any better.
> > > > >
> > > >
> > > > Yeah. The TODO says:
> > > > -* On/off LEDs should have max_brightness of 1
> > > > -* Get rid of enum led_brightness
> > > > -
> > > > -It is really an integer, as maximum is configurable. Get rid of it, or
> > > > -make it into typedef or something.
> > > >
> > > > I could only remove enum led_brightness, but in some places, the
> > > > LED_FULL, LED_HALF...
> > > > are used. This is why I created this #defines.
> > > > I don't know what we can do in these cases that LED_FULL, FULL_HALF is used.
> > > > For example, in the drivers/leds/leds-ss4200.c on line 223 have this code
> > > >
> > > > u32 setting = 0;
> > > > if (brightness >= LED_HALF)
> > > >     setting = 1;
> > >
> > > Yep. Such drivers should be modified to set max_brightness to real
> > > number of steps hardware can do... then we can remove LED_HALF,
> > > LED_FULL and such defines.
> > >
> > > Best regards,
> > >                                                                 Pavel
> >
> > but this will require the effort of everyone who has already written drivers
> > for some LED hardware. Because only the driver author himself will know
> > all the steps for that specific LED.
> >
> > or, maybe we can adapt this drivers for understand the brightness as a 0 or 1.
> > 0 for OFF and 1 for HALF and FULL. This is possble ?

> In the drivers/leds/leds-clevo-mail.c for example, the LED_HALF is
> used on line 102.
> In drivers/leds/leds-pca955x.c the LED_HALF is used in line 260.
> 
> How can I remove these keywords if manufacturers still use them?

And now you see why no one has removed them before now.

Once all uses have been converted, we can remove the enum.

-- 
Lee Jones [李琼斯]