[v7,01/11] leds: add support for hardware driven LEDs

  Some LEDs can be driven by hardware (for example a LED connected to
an ethernet PHY or an ethernet switch can be configured to blink on
activity on the network, which in software is done by the netdev trigger).

To do such offloading, LED driver must support this and a supported
trigger must be used.

LED driver should declare the correct blink_mode supported and should set
the blink_mode parameter to one of HARDWARE_CONTROLLED or
SOFTWARE_HARDWARE_CONTROLLED.
The trigger will check this option and fail to activate if the blink_mode
is not supported. By default if a LED driver doesn't declare blink_mode,
SOFTWARE_CONTROLLED is assumed.

The LED must implement 3 main API:
- hw_control_status(): This asks the LED driver if hardware mode is
    enabled or not.
    Triggers will check if the offload mode is supported and will be
    activated accordingly. If the trigger can't run in software mode,
    return -EOPNOTSUPP as the blinking can't be simulated by software.
- hw_control_start(): This will simply enable the hardware mode for
    the LED.
    With this not declared and hw_control_status() returning true,
    it's assumed that the LED is always in hardware mode.
- hw_control_stop(): This will simply disable the hardware mode for
    the LED.
    With this not declared and hw_control_status() returning true,
    it's assumed that the LED is always in hardware mode.
    It's advised to the driver to put the LED in the old state but this
    is not enforcerd and putting the LED off is also accepted.

With HARDWARE_CONTROLLED blink_mode hw_control_status/start/stop is
optional and any software only trigger will reject activation as the LED
supports only hardware mode.

An additional config CONFIG_LEDS_HARDWARE_CONTROL is added to add support
for LEDs that can be controlled by hardware.

Cc: Marek Behún <kabel@kernel.org>
Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
---
 Documentation/leds/leds-class.rst | 28 ++++++++++++++++++
 drivers/leds/Kconfig              | 11 ++++++++
 drivers/leds/led-class.c          | 27 ++++++++++++++++++
 drivers/leds/led-triggers.c       | 29 +++++++++++++++++++
 include/linux/leds.h              | 47 ++++++++++++++++++++++++++++++-
 5 files changed, 141 insertions(+), 1 deletion(-)
  

Hi Christian,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on pavel-leds/for-next]
[also build test ERROR on robh/for-next net-next/master net/master linus/master v6.1 next-20221215]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Christian-Marangi/Adds-support-for-PHY-LEDs-with-offload-triggers/20221215-080414
base:   git://git.kernel.org/pub/scm/linux/kernel/git/pavel/linux-leds.git for-next
patch link:    https://lore.kernel.org/r/20221214235438.30271-2-ansuelsmth%40gmail.com
patch subject: [PATCH v7 01/11] leds: add support for hardware driven LEDs
config: arc-randconfig-r005-20221214
compiler: arceb-elf-gcc (GCC) 12.1.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/intel-lab-lkp/linux/commit/f714ac6012266bf89c4c1b7d5e6ebaf50d4c525c
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Christian-Marangi/Adds-support-for-PHY-LEDs-with-offload-triggers/20221215-080414
        git checkout f714ac6012266bf89c4c1b7d5e6ebaf50d4c525c
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=arc SHELL=/bin/bash drivers/leds/

If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   drivers/leds/led-triggers.c: In function 'led_trigger_set':
>> drivers/leds/led-triggers.c:205:29: error: 'struct led_classdev' has no member named 'hw_control_status'
     205 |                     led_cdev->hw_control_status(led_cdev))
         |                             ^~
>> drivers/leds/led-triggers.c:206:33: error: 'struct led_classdev' has no member named 'hw_control_stop'
     206 |                         led_cdev->hw_control_stop(led_cdev);
         |                                 ^~


vim +205 drivers/leds/led-triggers.c

   177	
   178	/* Caller must ensure led_cdev->trigger_lock held */
   179	int led_trigger_set(struct led_classdev *led_cdev, struct led_trigger *trig)
   180	{
   181		char *event = NULL;
   182		char *envp[2];
   183		const char *name;
   184		int ret;
   185	
   186		if (!led_cdev->trigger && !trig)
   187			return 0;
   188	
   189		name = trig ? trig->name : "none";
   190		event = kasprintf(GFP_KERNEL, "TRIGGER=%s", name);
   191	
   192		/* Remove any existing trigger */
   193		if (led_cdev->trigger) {
   194			spin_lock(&led_cdev->trigger->leddev_list_lock);
   195			list_del_rcu(&led_cdev->trig_list);
   196			spin_unlock(&led_cdev->trigger->leddev_list_lock);
   197	
   198			/* ensure it's no longer visible on the led_cdevs list */
   199			synchronize_rcu();
   200	
   201			cancel_work_sync(&led_cdev->set_brightness_work);
   202			led_stop_software_blink(led_cdev);
   203			/* Disable hardware mode on trigger change if supported */
   204			if (led_cdev->blink_mode != SOFTWARE_CONTROLLED &&
 > 205			    led_cdev->hw_control_status(led_cdev))
 > 206				led_cdev->hw_control_stop(led_cdev);
   207			if (led_cdev->trigger->deactivate)
   208				led_cdev->trigger->deactivate(led_cdev);
   209			device_remove_groups(led_cdev->dev, led_cdev->trigger->groups);
   210			led_cdev->trigger = NULL;
   211			led_cdev->trigger_data = NULL;
   212			led_cdev->activated = false;
   213			led_set_brightness(led_cdev, LED_OFF);
   214		}
   215		if (trig) {
   216			/* Make sure the trigger support the LED blink mode */
   217			if (!led_trigger_is_supported(led_cdev, trig))
   218				return -EINVAL;
   219	
   220			spin_lock(&trig->leddev_list_lock);
   221			list_add_tail_rcu(&led_cdev->trig_list, &trig->led_cdevs);
   222			spin_unlock(&trig->leddev_list_lock);
   223			led_cdev->trigger = trig;
   224	
   225			if (trig->activate)
   226				ret = trig->activate(led_cdev);
   227			else
   228				ret = 0;
   229	
   230			if (ret)
   231				goto err_activate;
   232	
   233			ret = device_add_groups(led_cdev->dev, trig->groups);
   234			if (ret) {
   235				dev_err(led_cdev->dev, "Failed to add trigger attributes\n");
   236				goto err_add_groups;
   237			}
   238		}
   239	
   240		if (event) {
   241			envp[0] = event;
   242			envp[1] = NULL;
   243			if (kobject_uevent_env(&led_cdev->dev->kobj, KOBJ_CHANGE, envp))
   244				dev_err(led_cdev->dev,
   245					"%s: Error sending uevent\n", __func__);
   246			kfree(event);
   247		}
   248	
   249		return 0;
   250	
   251	err_add_groups:
   252	
   253		if (trig->deactivate)
   254			trig->deactivate(led_cdev);
   255	err_activate:
   256	
   257		spin_lock(&led_cdev->trigger->leddev_list_lock);
   258		list_del_rcu(&led_cdev->trig_list);
   259		spin_unlock(&led_cdev->trigger->leddev_list_lock);
   260		synchronize_rcu();
   261		led_cdev->trigger = NULL;
   262		led_cdev->trigger_data = NULL;
   263		led_set_brightness(led_cdev, LED_OFF);
   264		kfree(event);
   265	
   266		return ret;
   267	}
   268	EXPORT_SYMBOL_GPL(led_trigger_set);
   269
  

Hi Christian,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on pavel-leds/for-next]
[also build test ERROR on robh/for-next net-next/master net/master linus/master v6.1 next-20221215]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Christian-Marangi/Adds-support-for-PHY-LEDs-with-offload-triggers/20221215-080414
base:   git://git.kernel.org/pub/scm/linux/kernel/git/pavel/linux-leds.git for-next
patch link:    https://lore.kernel.org/r/20221214235438.30271-2-ansuelsmth%40gmail.com
patch subject: [PATCH v7 01/11] leds: add support for hardware driven LEDs
config: riscv-randconfig-r001-20221214
compiler: clang version 16.0.0 (https://github.com/llvm/llvm-project 98b13979fb05f3ed288a900deb843e7b27589e58)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # install riscv cross compiling tool for clang build
        # apt-get install binutils-riscv-linux-gnu
        # https://github.com/intel-lab-lkp/linux/commit/f714ac6012266bf89c4c1b7d5e6ebaf50d4c525c
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Christian-Marangi/Adds-support-for-PHY-LEDs-with-offload-triggers/20221215-080414
        git checkout f714ac6012266bf89c4c1b7d5e6ebaf50d4c525c
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=riscv SHELL=/bin/bash drivers/leds/

If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

>> drivers/leds/led-triggers.c:205:17: error: no member named 'hw_control_status' in 'struct led_classdev'
                       led_cdev->hw_control_status(led_cdev))
                       ~~~~~~~~  ^
>> drivers/leds/led-triggers.c:206:14: error: no member named 'hw_control_stop' in 'struct led_classdev'
                           led_cdev->hw_control_stop(led_cdev);
                           ~~~~~~~~  ^
   2 errors generated.


vim +205 drivers/leds/led-triggers.c

   177	
   178	/* Caller must ensure led_cdev->trigger_lock held */
   179	int led_trigger_set(struct led_classdev *led_cdev, struct led_trigger *trig)
   180	{
   181		char *event = NULL;
   182		char *envp[2];
   183		const char *name;
   184		int ret;
   185	
   186		if (!led_cdev->trigger && !trig)
   187			return 0;
   188	
   189		name = trig ? trig->name : "none";
   190		event = kasprintf(GFP_KERNEL, "TRIGGER=%s", name);
   191	
   192		/* Remove any existing trigger */
   193		if (led_cdev->trigger) {
   194			spin_lock(&led_cdev->trigger->leddev_list_lock);
   195			list_del_rcu(&led_cdev->trig_list);
   196			spin_unlock(&led_cdev->trigger->leddev_list_lock);
   197	
   198			/* ensure it's no longer visible on the led_cdevs list */
   199			synchronize_rcu();
   200	
   201			cancel_work_sync(&led_cdev->set_brightness_work);
   202			led_stop_software_blink(led_cdev);
   203			/* Disable hardware mode on trigger change if supported */
   204			if (led_cdev->blink_mode != SOFTWARE_CONTROLLED &&
 > 205			    led_cdev->hw_control_status(led_cdev))
 > 206				led_cdev->hw_control_stop(led_cdev);
   207			if (led_cdev->trigger->deactivate)
   208				led_cdev->trigger->deactivate(led_cdev);
   209			device_remove_groups(led_cdev->dev, led_cdev->trigger->groups);
   210			led_cdev->trigger = NULL;
   211			led_cdev->trigger_data = NULL;
   212			led_cdev->activated = false;
   213			led_set_brightness(led_cdev, LED_OFF);
   214		}
   215		if (trig) {
   216			/* Make sure the trigger support the LED blink mode */
   217			if (!led_trigger_is_supported(led_cdev, trig))
   218				return -EINVAL;
   219	
   220			spin_lock(&trig->leddev_list_lock);
   221			list_add_tail_rcu(&led_cdev->trig_list, &trig->led_cdevs);
   222			spin_unlock(&trig->leddev_list_lock);
   223			led_cdev->trigger = trig;
   224	
   225			if (trig->activate)
   226				ret = trig->activate(led_cdev);
   227			else
   228				ret = 0;
   229	
   230			if (ret)
   231				goto err_activate;
   232	
   233			ret = device_add_groups(led_cdev->dev, trig->groups);
   234			if (ret) {
   235				dev_err(led_cdev->dev, "Failed to add trigger attributes\n");
   236				goto err_add_groups;
   237			}
   238		}
   239	
   240		if (event) {
   241			envp[0] = event;
   242			envp[1] = NULL;
   243			if (kobject_uevent_env(&led_cdev->dev->kobj, KOBJ_CHANGE, envp))
   244				dev_err(led_cdev->dev,
   245					"%s: Error sending uevent\n", __func__);
   246			kfree(event);
   247		}
   248	
   249		return 0;
   250	
   251	err_add_groups:
   252	
   253		if (trig->deactivate)
   254			trig->deactivate(led_cdev);
   255	err_activate:
   256	
   257		spin_lock(&led_cdev->trigger->leddev_list_lock);
   258		list_del_rcu(&led_cdev->trig_list);
   259		spin_unlock(&led_cdev->trigger->leddev_list_lock);
   260		synchronize_rcu();
   261		led_cdev->trigger = NULL;
   262		led_cdev->trigger_data = NULL;
   263		led_set_brightness(led_cdev, LED_OFF);
   264		kfree(event);
   265	
   266		return ret;
   267	}
   268	EXPORT_SYMBOL_GPL(led_trigger_set);
   269
  

Hi Christian,

Thanks for the patch.

I think Andrew's email is offline at the moment.

On Thu, Dec 15, 2022 at 12:54:28AM +0100, Christian Marangi wrote:
> +static bool led_trigger_is_supported(struct led_classdev *led_cdev,
> +				     struct led_trigger *trigger)
> +{
> +	switch (led_cdev->blink_mode) {
> +	case SOFTWARE_CONTROLLED:
> +		if (trigger->supported_blink_modes == HARDWARE_ONLY)
> +			return 0;
> +		break;
> +	case HARDWARE_CONTROLLED:
> +		if (trigger->supported_blink_modes == SOFTWARE_ONLY)
> +			return 0;
> +		break;
> +	case SOFTWARE_HARDWARE_CONTROLLED:
> +		break;
> +	default:
> +		return 0;
> +	}
> +
> +	return 1;

Should be returning true/false. I'm not sure I'm a fan of the style of
this though - wouldn't the following be easier to read?

	switch (led_cdev->blink_mode) {
	case SOFTWARE_CONTROLLED:
		return trigger->supported_blink_modes != HARDWARE_ONLY;

	case HARDWARE_CONTROLLED:
		return trigger->supported_blink_modes != SOFTWARE_ONLY;

	case SOFTWARE_HARDWARE_CONTROLLED:
		return true;
	}
?

Also, does it really need a default case - without it, when the
led_blink_modes enum is expanded and the switch statement isn't
updated, we'll get a compiler warning which will prompt this to be
updated - whereas, with a default case, it won't.

> @@ -188,6 +213,10 @@ int led_trigger_set(struct led_classdev *led_cdev, struct led_trigger *trig)
>  		led_set_brightness(led_cdev, LED_OFF);
>  	}
>  	if (trig) {
> +		/* Make sure the trigger support the LED blink mode */
> +		if (!led_trigger_is_supported(led_cdev, trig))
> +			return -EINVAL;

Shouldn't validation happen before we start taking any actions? In other
words, before we remove the previous trigger?

> @@ -350,12 +381,26 @@ static inline bool led_sysfs_is_disabled(struct led_classdev *led_cdev)
>  
>  #define TRIG_NAME_MAX 50
>  
> +enum led_trigger_blink_supported_modes {
> +	SOFTWARE_ONLY = SOFTWARE_CONTROLLED,
> +	HARDWARE_ONLY = HARDWARE_CONTROLLED,
> +	SOFTWARE_HARDWARE = SOFTWARE_HARDWARE_CONTROLLED,

I suspect all these generic names are asking for eventual namespace
clashes. Maybe prefix them with LED_ ?

Thanks.
  

On Thu, Dec 15, 2022 at 04:13:03PM +0000, Russell King (Oracle) wrote:
> Hi Christian,
> 
> Thanks for the patch.
> 
> I think Andrew's email is offline at the moment.
>

Notice by gmail spamming me "I CAN'T SEND IT AHHHHH"
Holidy times I guess?

> On Thu, Dec 15, 2022 at 12:54:28AM +0100, Christian Marangi wrote:
> > +static bool led_trigger_is_supported(struct led_classdev *led_cdev,
> > +				     struct led_trigger *trigger)
> > +{
> > +	switch (led_cdev->blink_mode) {
> > +	case SOFTWARE_CONTROLLED:
> > +		if (trigger->supported_blink_modes == HARDWARE_ONLY)
> > +			return 0;
> > +		break;
> > +	case HARDWARE_CONTROLLED:
> > +		if (trigger->supported_blink_modes == SOFTWARE_ONLY)
> > +			return 0;
> > +		break;
> > +	case SOFTWARE_HARDWARE_CONTROLLED:
> > +		break;
> > +	default:
> > +		return 0;
> > +	}
> > +
> > +	return 1;
> 
> Should be returning true/false. I'm not sure I'm a fan of the style of
> this though - wouldn't the following be easier to read?
> 
> 	switch (led_cdev->blink_mode) {
> 	case SOFTWARE_CONTROLLED:
> 		return trigger->supported_blink_modes != HARDWARE_ONLY;
> 
> 	case HARDWARE_CONTROLLED:
> 		return trigger->supported_blink_modes != SOFTWARE_ONLY;
> 
> 	case SOFTWARE_HARDWARE_CONTROLLED:
> 		return true;
> 	}
> ?

Much better!

> 
> Also, does it really need a default case - without it, when the
> led_blink_modes enum is expanded and the switch statement isn't
> updated, we'll get a compiler warning which will prompt this to be
> updated - whereas, with a default case, it won't.
> 

I added the default just to mute some compiler warning. But guess if
every enum is handled the warning should not be reported.

> > @@ -188,6 +213,10 @@ int led_trigger_set(struct led_classdev *led_cdev, struct led_trigger *trig)
> >  		led_set_brightness(led_cdev, LED_OFF);
> >  	}
> >  	if (trig) {
> > +		/* Make sure the trigger support the LED blink mode */
> > +		if (!led_trigger_is_supported(led_cdev, trig))
> > +			return -EINVAL;
> 
> Shouldn't validation happen before we start taking any actions? In other
> words, before we remove the previous trigger?
> 

trigger_set first remove any trigger and set the led off. Then apply the
new trigger. So the validation is done only when a trigger is actually
applied. Think we should understand the best case here.

> > @@ -350,12 +381,26 @@ static inline bool led_sysfs_is_disabled(struct led_classdev *led_cdev)
> >  
> >  #define TRIG_NAME_MAX 50
> >  
> > +enum led_trigger_blink_supported_modes {
> > +	SOFTWARE_ONLY = SOFTWARE_CONTROLLED,
> > +	HARDWARE_ONLY = HARDWARE_CONTROLLED,
> > +	SOFTWARE_HARDWARE = SOFTWARE_HARDWARE_CONTROLLED,
> 
> I suspect all these generic names are asking for eventual namespace
> clashes. Maybe prefix them with LED_ ?

Agree they are pretty generic so I can see why... My only concern was
making them too long... Maybe reduce them to SW or HW? LEDS_SW_ONLY...
LEDS_SW_CONTROLLED?

> 
> Thanks.
> 
> -- 
> RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
> FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!
  

On Fri, Dec 16, 2022 at 05:45:25PM +0100, Christian Marangi wrote:
> On Thu, Dec 15, 2022 at 04:13:03PM +0000, Russell King (Oracle) wrote:
> > Hi Christian,
> > 
> > Thanks for the patch.
> > 
> > I think Andrew's email is offline at the moment.
> >
> 
> Notice by gmail spamming me "I CAN'T SEND IT AHHHHH"
> Holidy times I guess?

Sadly, Andrew's email has done this a number of times - and Andrew
used to be on IRC so I could prod him about it, but it seems he
doesn't hang out on IRC anymore. It's been like it a few days now.

> > On Thu, Dec 15, 2022 at 12:54:28AM +0100, Christian Marangi wrote:
> > > @@ -188,6 +213,10 @@ int led_trigger_set(struct led_classdev *led_cdev, struct led_trigger *trig)
> > >  		led_set_brightness(led_cdev, LED_OFF);
> > >  	}
> > >  	if (trig) {
> > > +		/* Make sure the trigger support the LED blink mode */
> > > +		if (!led_trigger_is_supported(led_cdev, trig))
> > > +			return -EINVAL;
> > 
> > Shouldn't validation happen before we start taking any actions? In other
> > words, before we remove the previous trigger?
> > 
> 
> trigger_set first remove any trigger and set the led off. Then apply the
> new trigger. So the validation is done only when a trigger is actually
> applied. Think we should understand the best case here.

I think this is a question that needs to be answered by the LEDs folk,
as it's an interface behaviour / quality of implementation issue.

> > > @@ -350,12 +381,26 @@ static inline bool led_sysfs_is_disabled(struct led_classdev *led_cdev)
> > >  
> > >  #define TRIG_NAME_MAX 50
> > >  
> > > +enum led_trigger_blink_supported_modes {
> > > +	SOFTWARE_ONLY = SOFTWARE_CONTROLLED,
> > > +	HARDWARE_ONLY = HARDWARE_CONTROLLED,
> > > +	SOFTWARE_HARDWARE = SOFTWARE_HARDWARE_CONTROLLED,
> > 
> > I suspect all these generic names are asking for eventual namespace
> > clashes. Maybe prefix them with LED_ ?
> 
> Agree they are pretty generic so I can see why... My only concern was
> making them too long... Maybe reduce them to SW or HW? LEDS_SW_ONLY...
> LEDS_SW_CONTROLLED?

Seems sensible to me - and as a bonus they get shorter than the above!
  

On Fri, Dec 16, 2022 at 05:45:25PM +0100, Christian Marangi wrote:
> On Thu, Dec 15, 2022 at 04:13:03PM +0000, Russell King (Oracle) wrote:
> > Hi Christian,
> > 
> > Thanks for the patch.
> > 
> > I think Andrew's email is offline at the moment.
> >
> 
> Notice by gmail spamming me "I CAN'T SEND IT AHHHHH"
> Holidy times I guess?

That was part of the problem. Away travelling when foot gun hit foot,
and i didn't have access to the tools to fix it while away.

    Andrew
  

On Thu, Dec 15, 2022 at 12:54:28AM +0100, Christian Marangi wrote:
> The LED must implement 3 main API:
> - hw_control_status(): This asks the LED driver if hardware mode is
>     enabled or not.
>     Triggers will check if the offload mode is supported and will be
>     activated accordingly. If the trigger can't run in software mode,
>     return -EOPNOTSUPP as the blinking can't be simulated by software.
> - hw_control_start(): This will simply enable the hardware mode for
>     the LED.
>     With this not declared and hw_control_status() returning true,
>     it's assumed that the LED is always in hardware mode.
> - hw_control_stop(): This will simply disable the hardware mode for
>     the LED.
>     With this not declared and hw_control_status() returning true,
>     it's assumed that the LED is always in hardware mode.
>     It's advised to the driver to put the LED in the old state but this
>     is not enforcerd and putting the LED off is also accepted.
> 

Building htmldocs with Sphinx, I got:

Documentation/leds/leds-class.rst:190: WARNING: Unexpected indentation.
Documentation/leds/leds-class.rst:192: WARNING: Block quote ends without a blank line; unexpected unindent.

I have applied the fixup on the API bullet list above:

---- >8 ----

diff --git a/Documentation/leds/leds-class.rst b/Documentation/leds/leds-class.rst
index efd2f68c46a7f9..fc16b503747800 100644
--- a/Documentation/leds/leds-class.rst
+++ b/Documentation/leds/leds-class.rst
@@ -186,13 +186,15 @@ supported. By default if a LED driver doesn't declare blink_mode, SOFTWARE_CONTR
 is assumed.
 
 The LED must implement 3 main API:
+
 - hw_control_status(): This asks the LED driver if hardware mode is enabled
-    or not. Triggers will check if the hardware mode is active and will try
-    to offload their triggers if supported by the driver.
+  or not. Triggers will check if the hardware mode is active and will try
+  to offload their triggers if supported by the driver.
 - hw_control_start(): This will simply enable the hardware mode for the LED.
+
 - hw_control_stop(): This will simply disable the hardware mode for the LED.
-    It's advised to the driver to put the LED in the old state but this is not
-    enforcerd and putting the LED off is also accepted.
+  It's advised to the driver to put the LED in the old state but this is not
+  enforcerd and putting the LED off is also accepted.
 
 With HARDWARE_CONTROLLED blink_mode hw_control_status/start/stop is optional
 and any software only trigger will reject activation as the LED supports only

Thanks.