[PATCH 0/2] LED trigger on MTD activity

[PATCH 0/2] LED trigger on MTD activity

[Ezequiel Garcia]

After our recent discussion [1], here's a proper patchset.

The first patch is a minor cleanup which moves mtd_write_oob
out of mtd.h header.

The second patch implements the LED trigger. Unfortunately,
it depends on the first patch, so it might make sense to take
this two through the MTD tree with Jacked acknowledgement.

Feedback is welcome!

[1] http://www.spinics.net/lists/linux-leds/msg05906.html

Ezequiel Garcia (2):
  mtd: Uninline mtd_write_oob and move it to mtdcore.c
  leds: trigger: Introduce a MTD (NAND/NOR) trigger

 drivers/leds/trigger/Kconfig       |  8 +++++++
 drivers/leds/trigger/Makefile      |  1 +
 drivers/leds/trigger/ledtrig-mtd.c | 49 ++++++++++++++++++++++++++++++++++++++
 drivers/mtd/mtdcore.c              | 19 +++++++++++++++
 drivers/mtd/nand/nand_base.c       | 29 +---------------------
 include/linux/leds.h               |  6 +++++
 include/linux/mtd/mtd.h            | 12 +---------
 7 files changed, 85 insertions(+), 39 deletions(-)
 create mode 100644 drivers/leds/trigger/ledtrig-mtd.c

-- 
2.7.0

[PATCH 1/2] mtd: Uninline mtd_write_oob and move it to mtdcore.c

[Ezequiel Garcia]

There's no reason for having mtd_write_oob inlined in mtd.h header.
Move it to mtdcore.c where it belongs.

Signed-off-by: Ezequiel Garcia <ezequiel@vanguardiasur.com.ar>
---
 drivers/mtd/mtdcore.c   | 12 ++++++++++++
 include/linux/mtd/mtd.h | 12 +-----------
 2 files changed, 13 insertions(+), 11 deletions(-)

diff --git a/drivers/mtd/mtdcore.c b/drivers/mtd/mtdcore.c
index 309625130b21..99d83f3331b0 100644
--- a/drivers/mtd/mtdcore.c
+++ b/drivers/mtd/mtdcore.c
@@ -997,6 +997,18 @@ int mtd_read_oob(struct mtd_info *mtd, loff_t from, struct mtd_oob_ops *ops)
 }
 EXPORT_SYMBOL_GPL(mtd_read_oob);
 
+int mtd_write_oob(struct mtd_info *mtd, loff_t to,
+				struct mtd_oob_ops *ops)
+{
+	ops->retlen = ops->oobretlen = 0;
+	if (!mtd->_write_oob)
+		return -EOPNOTSUPP;
+	if (!(mtd->flags & MTD_WRITEABLE))
+		return -EROFS;
+	return mtd->_write_oob(mtd, to, ops);
+}
+EXPORT_SYMBOL_GPL(mtd_write_oob);
+
 /*
  * Method to access the protection register area, present in some flash
  * devices. The user data is one time programmable but the factory data is read
diff --git a/include/linux/mtd/mtd.h b/include/linux/mtd/mtd.h
index 771272187316..ef9fea4fc400 100644
--- a/include/linux/mtd/mtd.h
+++ b/include/linux/mtd/mtd.h
@@ -283,17 +283,7 @@ int mtd_panic_write(struct mtd_info *mtd, loff_t to, size_t len, size_t *retlen,
 		    const u_char *buf);
 
 int mtd_read_oob(struct mtd_info *mtd, loff_t from, struct mtd_oob_ops *ops);
-
-static inline int mtd_write_oob(struct mtd_info *mtd, loff_t to,
-				struct mtd_oob_ops *ops)
-{
-	ops->retlen = ops->oobretlen = 0;
-	if (!mtd->_write_oob)
-		return -EOPNOTSUPP;
-	if (!(mtd->flags & MTD_WRITEABLE))
-		return -EROFS;
-	return mtd->_write_oob(mtd, to, ops);
-}
+int mtd_write_oob(struct mtd_info *mtd, loff_t to, struct mtd_oob_ops *ops);
 
 int mtd_get_fact_prot_info(struct mtd_info *mtd, size_t len, size_t *retlen,
 			   struct otp_info *buf);
-- 
2.7.0

[PATCH 2/2] leds: trigger: Introduce a MTD (NAND/NOR) trigger

[Ezequiel Garcia]

This commit introduces a MTD trigger for flash (NAND/NOR) device
activity. The implementation is copied from IDE disk.

This deprecates the "nand-disk" LED trigger, but for backwards
compatibility, we still keep the "nand-disk" trigger around.

The motivation for deprecating the "nand-disk" LED trigger is that
it only works for NAND drivers, whereas the "mtd" LED trigger
is more generic (in fact, "nand-disk" currently only works for
certain NAND drivers).

Signed-off-by: Ezequiel Garcia <ezequiel@vanguardiasur.com.ar>
---
 drivers/leds/trigger/Kconfig       |  8 +++++++
 drivers/leds/trigger/Makefile      |  1 +
 drivers/leds/trigger/ledtrig-mtd.c | 49 ++++++++++++++++++++++++++++++++++++++
 drivers/mtd/mtdcore.c              |  7 ++++++
 drivers/mtd/nand/nand_base.c       | 29 +---------------------
 include/linux/leds.h               |  6 +++++
 6 files changed, 72 insertions(+), 28 deletions(-)
 create mode 100644 drivers/leds/trigger/ledtrig-mtd.c

diff --git a/drivers/leds/trigger/Kconfig b/drivers/leds/trigger/Kconfig
index 554f5bfbeced..beac8c31c51b 100644
--- a/drivers/leds/trigger/Kconfig
+++ b/drivers/leds/trigger/Kconfig
@@ -41,6 +41,14 @@ config LEDS_TRIGGER_IDE_DISK
 	  This allows LEDs to be controlled by IDE disk activity.
 	  If unsure, say Y.
 
+config LEDS_TRIGGER_MTD
+	bool "LED MTD (NAND/NOR) Trigger"
+	depends on MTD
+	depends on LEDS_TRIGGERS
+	help
+	  This allows LEDs to be controlled by MTD activity.
+	  If unsure, say N.
+
 config LEDS_TRIGGER_HEARTBEAT
 	tristate "LED Heartbeat Trigger"
 	depends on LEDS_TRIGGERS
diff --git a/drivers/leds/trigger/Makefile b/drivers/leds/trigger/Makefile
index 547bf5c80e52..8cc64a4f4e25 100644
--- a/drivers/leds/trigger/Makefile
+++ b/drivers/leds/trigger/Makefile
@@ -1,6 +1,7 @@
 obj-$(CONFIG_LEDS_TRIGGER_TIMER)	+= ledtrig-timer.o
 obj-$(CONFIG_LEDS_TRIGGER_ONESHOT)	+= ledtrig-oneshot.o
 obj-$(CONFIG_LEDS_TRIGGER_IDE_DISK)	+= ledtrig-ide-disk.o
+obj-$(CONFIG_LEDS_TRIGGER_MTD)		+= ledtrig-mtd.o
 obj-$(CONFIG_LEDS_TRIGGER_HEARTBEAT)	+= ledtrig-heartbeat.o
 obj-$(CONFIG_LEDS_TRIGGER_BACKLIGHT)	+= ledtrig-backlight.o
 obj-$(CONFIG_LEDS_TRIGGER_GPIO)		+= ledtrig-gpio.o
diff --git a/drivers/leds/trigger/ledtrig-mtd.c b/drivers/leds/trigger/ledtrig-mtd.c
new file mode 100644
index 000000000000..32af377976ec
--- /dev/null
+++ b/drivers/leds/trigger/ledtrig-mtd.c
@@ -0,0 +1,49 @@
+/*
+ * LED MTD trigger
+ *
+ * Copyright 2016 Ezequiel Garcia <ezequiel@vanguardiasur.com.ar>
+ *
+ * Based on LED IDE-Disk Activity Trigger
+ *
+ * Copyright 2006 Openedhand Ltd.
+ *
+ * Author: Richard Purdie <rpurdie@openedhand.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ */
+
+#include <linux/kernel.h>
+#include <linux/init.h>
+#include <linux/leds.h>
+
+#define BLINK_DELAY 30
+
+DEFINE_LED_TRIGGER(ledtrig_mtd);
+DEFINE_LED_TRIGGER(ledtrig_nand);
+
+void ledtrig_mtd_activity(void)
+{
+	unsigned long blink_delay = BLINK_DELAY;
+
+	led_trigger_blink_oneshot(ledtrig_nand,
+				  &blink_delay, &blink_delay, 0);
+	led_trigger_blink_oneshot(ledtrig_mtd,
+				  &blink_delay, &blink_delay, 0);
+}
+EXPORT_SYMBOL(ledtrig_mtd_activity);
+
+static int __init ledtrig_mtd_init(void)
+{
+	/* For backwards compatibility, we need to keep the nand-disk
+	 * trigger around. Users are encouraged to switch to the
+	 * "mtd" trigger.
+	 */
+	led_trigger_register_simple("nand-disk", &ledtrig_nand);
+	led_trigger_register_simple("mtd", &ledtrig_mtd);
+
+	return 0;
+}
+device_initcall(ledtrig_mtd_init);
diff --git a/drivers/mtd/mtdcore.c b/drivers/mtd/mtdcore.c
index 99d83f3331b0..bee180bd11e7 100644
--- a/drivers/mtd/mtdcore.c
+++ b/drivers/mtd/mtdcore.c
@@ -40,6 +40,7 @@
 #include <linux/slab.h>
 #include <linux/reboot.h>
 #include <linux/kconfig.h>
+#include <linux/leds.h>
 
 #include <linux/mtd/mtd.h>
 #include <linux/mtd/partitions.h>
@@ -862,6 +863,7 @@ int mtd_erase(struct mtd_info *mtd, struct erase_info *instr)
 		mtd_erase_callback(instr);
 		return 0;
 	}
+	ledtrig_mtd_activity();
 	return mtd->_erase(mtd, instr);
 }
 EXPORT_SYMBOL_GPL(mtd_erase);
@@ -925,6 +927,7 @@ int mtd_read(struct mtd_info *mtd, loff_t from, size_t len, size_t *retlen,
 	if (!len)
 		return 0;
 
+	ledtrig_mtd_activity();
 	/*
 	 * In the absence of an error, drivers return a non-negative integer
 	 * representing the maximum number of bitflips that were corrected on
@@ -949,6 +952,7 @@ int mtd_write(struct mtd_info *mtd, loff_t to, size_t len, size_t *retlen,
 		return -EROFS;
 	if (!len)
 		return 0;
+	ledtrig_mtd_activity();
 	return mtd->_write(mtd, to, len, retlen, buf);
 }
 EXPORT_SYMBOL_GPL(mtd_write);
@@ -982,6 +986,8 @@ int mtd_read_oob(struct mtd_info *mtd, loff_t from, struct mtd_oob_ops *ops)
 	ops->retlen = ops->oobretlen = 0;
 	if (!mtd->_read_oob)
 		return -EOPNOTSUPP;
+
+	ledtrig_mtd_activity();
 	/*
 	 * In cases where ops->datbuf != NULL, mtd->_read_oob() has semantics
 	 * similar to mtd->_read(), returning a non-negative integer
@@ -1005,6 +1011,7 @@ int mtd_write_oob(struct mtd_info *mtd, loff_t to,
 		return -EOPNOTSUPP;
 	if (!(mtd->flags & MTD_WRITEABLE))
 		return -EROFS;
+	ledtrig_mtd_activity();
 	return mtd->_write_oob(mtd, to, ops);
 }
 EXPORT_SYMBOL_GPL(mtd_write_oob);
diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
index 6a0e80c1ba02..f036f958200a 100644
--- a/drivers/mtd/nand/nand_base.c
+++ b/drivers/mtd/nand/nand_base.c
@@ -43,7 +43,6 @@
 #include <linux/mtd/nand_bch.h>
 #include <linux/interrupt.h>
 #include <linux/bitops.h>
-#include <linux/leds.h>
 #include <linux/io.h>
 #include <linux/mtd/partitions.h>
 #include <linux/of_mtd.h>
@@ -97,12 +96,6 @@ static int nand_get_device(struct mtd_info *mtd, int new_state);
 static int nand_do_write_oob(struct mtd_info *mtd, loff_t to,
 			     struct mtd_oob_ops *ops);
 
-/*
- * For devices which display every fart in the system on a separate LED. Is
- * compiled away when LED support is disabled.
- */
-DEFINE_LED_TRIGGER(nand_led_trigger);
-
 static int check_offs_len(struct mtd_info *mtd,
 					loff_t ofs, uint64_t len)
 {
@@ -540,19 +533,16 @@ void nand_wait_ready(struct mtd_info *mtd)
 	if (in_interrupt() || oops_in_progress)
 		return panic_nand_wait_ready(mtd, timeo);
 
-	led_trigger_event(nand_led_trigger, LED_FULL);
 	/* Wait until command is processed or timeout occurs */
 	timeo = jiffies + msecs_to_jiffies(timeo);
 	do {
 		if (chip->dev_ready(mtd))
-			goto out;
+			return;
 		cond_resched();
 	} while (time_before(jiffies, timeo));
 
 	if (!chip->dev_ready(mtd))
 		pr_warn_ratelimited("timeout while waiting for chip to become ready\n");
-out:
-	led_trigger_event(nand_led_trigger, LED_OFF);
 }
 EXPORT_SYMBOL_GPL(nand_wait_ready);
 
@@ -885,8 +875,6 @@ static int nand_wait(struct mtd_info *mtd, struct nand_chip *chip)
 	int status;
 	unsigned long timeo = 400;
 
-	led_trigger_event(nand_led_trigger, LED_FULL);
-
 	/*
 	 * Apply this short delay always to ensure that we do wait tWB in any
 	 * case on any machine.
@@ -910,7 +898,6 @@ static int nand_wait(struct mtd_info *mtd, struct nand_chip *chip)
 			cond_resched();
 		} while (time_before(jiffies, timeo));
 	}
-	led_trigger_event(nand_led_trigger, LED_OFF);
 
 	status = (int)chip->read_byte(mtd);
 	/* This can happen if in case of timeout or buggy dev_ready */
@@ -4499,20 +4486,6 @@ void nand_release(struct mtd_info *mtd)
 }
 EXPORT_SYMBOL_GPL(nand_release);
 
-static int __init nand_base_init(void)
-{
-	led_trigger_register_simple("nand-disk", &nand_led_trigger);
-	return 0;
-}
-
-static void __exit nand_base_exit(void)
-{
-	led_trigger_unregister_simple(nand_led_trigger);
-}
-
-module_init(nand_base_init);
-module_exit(nand_base_exit);
-
 MODULE_LICENSE("GPL");
 MODULE_AUTHOR("Steven J. Hill <sjhill@realitydiluted.com>");
 MODULE_AUTHOR("Thomas Gleixner <tglx@linutronix.de>");
diff --git a/include/linux/leds.h b/include/linux/leds.h
index f203a8f89d30..19eb10278bea 100644
--- a/include/linux/leds.h
+++ b/include/linux/leds.h
@@ -329,6 +329,12 @@ extern void ledtrig_ide_activity(void);
 static inline void ledtrig_ide_activity(void) {}
 #endif
 
+#ifdef CONFIG_LEDS_TRIGGER_MTD
+extern void ledtrig_mtd_activity(void);
+#else
+static inline void ledtrig_mtd_activity(void) {}
+#endif
+
 #if defined(CONFIG_LEDS_TRIGGER_CAMERA) || defined(CONFIG_LEDS_TRIGGER_CAMERA_MODULE)
 extern void ledtrig_flash_ctrl(bool on);
 extern void ledtrig_torch_ctrl(bool on);
-- 
2.7.0

Re: [PATCH 1/2] mtd: Uninline mtd_write_oob and move it to mtdcore.c

[Boris Brezillon]

On Tue, 12 Apr 2016 13:26:34 -0300
Ezequiel Garcia <ezequiel@vanguardiasur.com.ar> wrote:

> There's no reason for having mtd_write_oob inlined in mtd.h header.
> Move it to mtdcore.c where it belongs.

I guess the real motivation here is to avoid including leds.h in
mtd.h :).

> 
> Signed-off-by: Ezequiel Garcia <ezequiel@vanguardiasur.com.ar>

Acked-by: Boris Brezillon <boris.brezillon@free-electrons.com>

> ---
>  drivers/mtd/mtdcore.c   | 12 ++++++++++++
>  include/linux/mtd/mtd.h | 12 +-----------
>  2 files changed, 13 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/mtd/mtdcore.c b/drivers/mtd/mtdcore.c
> index 309625130b21..99d83f3331b0 100644
> --- a/drivers/mtd/mtdcore.c
> +++ b/drivers/mtd/mtdcore.c
> @@ -997,6 +997,18 @@ int mtd_read_oob(struct mtd_info *mtd, loff_t from, struct mtd_oob_ops *ops)
>  }
>  EXPORT_SYMBOL_GPL(mtd_read_oob);
>  
> +int mtd_write_oob(struct mtd_info *mtd, loff_t to,
> +				struct mtd_oob_ops *ops)
> +{
> +	ops->retlen = ops->oobretlen = 0;
> +	if (!mtd->_write_oob)
> +		return -EOPNOTSUPP;
> +	if (!(mtd->flags & MTD_WRITEABLE))
> +		return -EROFS;
> +	return mtd->_write_oob(mtd, to, ops);
> +}
> +EXPORT_SYMBOL_GPL(mtd_write_oob);
> +
>  /*
>   * Method to access the protection register area, present in some flash
>   * devices. The user data is one time programmable but the factory data is read
> diff --git a/include/linux/mtd/mtd.h b/include/linux/mtd/mtd.h
> index 771272187316..ef9fea4fc400 100644
> --- a/include/linux/mtd/mtd.h
> +++ b/include/linux/mtd/mtd.h
> @@ -283,17 +283,7 @@ int mtd_panic_write(struct mtd_info *mtd, loff_t to, size_t len, size_t *retlen,
>  		    const u_char *buf);
>  
>  int mtd_read_oob(struct mtd_info *mtd, loff_t from, struct mtd_oob_ops *ops);
> -
> -static inline int mtd_write_oob(struct mtd_info *mtd, loff_t to,
> -				struct mtd_oob_ops *ops)
> -{
> -	ops->retlen = ops->oobretlen = 0;
> -	if (!mtd->_write_oob)
> -		return -EOPNOTSUPP;
> -	if (!(mtd->flags & MTD_WRITEABLE))
> -		return -EROFS;
> -	return mtd->_write_oob(mtd, to, ops);
> -}
> +int mtd_write_oob(struct mtd_info *mtd, loff_t to, struct mtd_oob_ops *ops);
>  
>  int mtd_get_fact_prot_info(struct mtd_info *mtd, size_t len, size_t *retlen,
>  			   struct otp_info *buf);



-- 
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

Re: [PATCH 2/2] leds: trigger: Introduce a MTD (NAND/NOR) trigger

[Boris Brezillon]

Hi Ezequiel,

On Tue, 12 Apr 2016 13:26:35 -0300
Ezequiel Garcia <ezequiel@vanguardiasur.com.ar> wrote:

> This commit introduces a MTD trigger for flash (NAND/NOR) device
> activity. The implementation is copied from IDE disk.
> 
> This deprecates the "nand-disk" LED trigger, but for backwards
> compatibility, we still keep the "nand-disk" trigger around.
> 
> The motivation for deprecating the "nand-disk" LED trigger is that
> it only works for NAND drivers, whereas the "mtd" LED trigger
> is more generic (in fact, "nand-disk" currently only works for
> certain NAND drivers).
> 
> Signed-off-by: Ezequiel Garcia <ezequiel@vanguardiasur.com.ar>
> ---
>  drivers/leds/trigger/Kconfig       |  8 +++++++
>  drivers/leds/trigger/Makefile      |  1 +
>  drivers/leds/trigger/ledtrig-mtd.c | 49 ++++++++++++++++++++++++++++++++++++++
>  drivers/mtd/mtdcore.c              |  7 ++++++
>  drivers/mtd/nand/nand_base.c       | 29 +---------------------
>  include/linux/leds.h               |  6 +++++

I'd suggest splitting this patch in 2, one adding ledtrig-mtd code, and
another one make use of ledtrig_mtd_activity() and removing
nand-trigger code.

IMHO, we should also factorize ledtrig-ide and ledtrig-mtd code
(after all, you just copied the implementation from ledtrig-ide).
How about renaming the ledtrig-ide into ledtrig-storage and
ledtrig_ide_activity() into ledtrig_storage_activity()?

You can then add a trigger named "storage" in addition to the existing
"nand-disk" and "ide-disk" ones.

Anyway, this is just a suggestion, and I let leds maintainers decide
whether this is appropriate or not.

Best Regards,

Boris

-- 
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

Re: [PATCH 1/2] mtd: Uninline mtd_write_oob and move it to mtdcore.c

[Ezequiel Garcia]

On 12 April 2016 at 15:15, Boris Brezillon
<boris.brezillon@free-electrons.com> wrote:
> On Tue, 12 Apr 2016 13:26:34 -0300
> Ezequiel Garcia <ezequiel@vanguardiasur.com.ar> wrote:
>
>> There's no reason for having mtd_write_oob inlined in mtd.h header.
>> Move it to mtdcore.c where it belongs.
>
> I guess the real motivation here is to avoid including leds.h in
> mtd.h :).
>

Let's say that's what triggers this patch, but it still makes sense to do this
disregarding the led stuff.

>>
>> Signed-off-by: Ezequiel Garcia <ezequiel@vanguardiasur.com.ar>
>
> Acked-by: Boris Brezillon <boris.brezillon@free-electrons.com>
>

Thanks,

>> ---
>>  drivers/mtd/mtdcore.c   | 12 ++++++++++++
>>  include/linux/mtd/mtd.h | 12 +-----------
>>  2 files changed, 13 insertions(+), 11 deletions(-)
>>
>> diff --git a/drivers/mtd/mtdcore.c b/drivers/mtd/mtdcore.c
>> index 309625130b21..99d83f3331b0 100644
>> --- a/drivers/mtd/mtdcore.c
>> +++ b/drivers/mtd/mtdcore.c
>> @@ -997,6 +997,18 @@ int mtd_read_oob(struct mtd_info *mtd, loff_t from, struct mtd_oob_ops *ops)
>>  }
>>  EXPORT_SYMBOL_GPL(mtd_read_oob);
>>
>> +int mtd_write_oob(struct mtd_info *mtd, loff_t to,
>> +                             struct mtd_oob_ops *ops)
>> +{
>> +     ops->retlen = ops->oobretlen = 0;
>> +     if (!mtd->_write_oob)
>> +             return -EOPNOTSUPP;
>> +     if (!(mtd->flags & MTD_WRITEABLE))
>> +             return -EROFS;
>> +     return mtd->_write_oob(mtd, to, ops);
>> +}
>> +EXPORT_SYMBOL_GPL(mtd_write_oob);
>> +
>>  /*
>>   * Method to access the protection register area, present in some flash
>>   * devices. The user data is one time programmable but the factory data is read
>> diff --git a/include/linux/mtd/mtd.h b/include/linux/mtd/mtd.h
>> index 771272187316..ef9fea4fc400 100644
>> --- a/include/linux/mtd/mtd.h
>> +++ b/include/linux/mtd/mtd.h
>> @@ -283,17 +283,7 @@ int mtd_panic_write(struct mtd_info *mtd, loff_t to, size_t len, size_t *retlen,
>>                   const u_char *buf);
>>
>>  int mtd_read_oob(struct mtd_info *mtd, loff_t from, struct mtd_oob_ops *ops);
>> -
>> -static inline int mtd_write_oob(struct mtd_info *mtd, loff_t to,
>> -                             struct mtd_oob_ops *ops)
>> -{
>> -     ops->retlen = ops->oobretlen = 0;
>> -     if (!mtd->_write_oob)
>> -             return -EOPNOTSUPP;
>> -     if (!(mtd->flags & MTD_WRITEABLE))
>> -             return -EROFS;
>> -     return mtd->_write_oob(mtd, to, ops);
>> -}
>> +int mtd_write_oob(struct mtd_info *mtd, loff_t to, struct mtd_oob_ops *ops);
>>
>>  int mtd_get_fact_prot_info(struct mtd_info *mtd, size_t len, size_t *retlen,
>>                          struct otp_info *buf);
>
>
>
> --
> Boris Brezillon, Free Electrons
> Embedded Linux and Kernel engineering
> http://free-electrons.com



-- 
Ezequiel García, VanguardiaSur
www.vanguardiasur.com.ar

Re: [PATCH 2/2] leds: trigger: Introduce a MTD (NAND/NOR) trigger

[Ezequiel Garcia]

On 12 April 2016 at 15:27, Boris Brezillon
<boris.brezillon@free-electrons.com> wrote:
> Hi Ezequiel,
>
> On Tue, 12 Apr 2016 13:26:35 -0300
> Ezequiel Garcia <ezequiel@vanguardiasur.com.ar> wrote:
>
>> This commit introduces a MTD trigger for flash (NAND/NOR) device
>> activity. The implementation is copied from IDE disk.
>>
>> This deprecates the "nand-disk" LED trigger, but for backwards
>> compatibility, we still keep the "nand-disk" trigger around.
>>
>> The motivation for deprecating the "nand-disk" LED trigger is that
>> it only works for NAND drivers, whereas the "mtd" LED trigger
>> is more generic (in fact, "nand-disk" currently only works for
>> certain NAND drivers).
>>
>> Signed-off-by: Ezequiel Garcia <ezequiel@vanguardiasur.com.ar>
>> ---
>>  drivers/leds/trigger/Kconfig       |  8 +++++++
>>  drivers/leds/trigger/Makefile      |  1 +
>>  drivers/leds/trigger/ledtrig-mtd.c | 49 ++++++++++++++++++++++++++++++++++++++
>>  drivers/mtd/mtdcore.c              |  7 ++++++
>>  drivers/mtd/nand/nand_base.c       | 29 +---------------------
>>  include/linux/leds.h               |  6 +++++
>
> I'd suggest splitting this patch in 2, one adding ledtrig-mtd code, and
> another one make use of ledtrig_mtd_activity() and removing
> nand-trigger code.
>

Sure, that sounds good.

> IMHO, we should also factorize ledtrig-ide and ledtrig-mtd code
> (after all, you just copied the implementation from ledtrig-ide).
> How about renaming the ledtrig-ide into ledtrig-storage and
> ledtrig_ide_activity() into ledtrig_storage_activity()?
>
> You can then add a trigger named "storage" in addition to the existing
> "nand-disk" and "ide-disk" ones.
>

I'm not exactly following you: are you suggesting to suggest factorize
the code, or to meld the two triggers in a single "storage" trigger?

In the former, the code is so small, that I'm not sure it will be actually
reduced with some factorization. In the latter, wouldn't we loose
the capability to trigger on MTD and IDE activity independently?

> Anyway, this is just a suggestion, and I let leds maintainers decide
> whether this is appropriate or not.
>

Thanks for the reviewing Boris!

> Best Regards,
>
> Boris
>
> --
> Boris Brezillon, Free Electrons
> Embedded Linux and Kernel engineering
> http://free-electrons.com

-- 
Ezequiel García, VanguardiaSur
www.vanguardiasur.com.ar

Re: [PATCH 2/2] leds: trigger: Introduce a MTD (NAND/NOR) trigger

[Boris Brezillon]

On Tue, 12 Apr 2016 15:40:17 -0300
Ezequiel Garcia <ezequiel@vanguardiasur.com.ar> wrote:

> On 12 April 2016 at 15:27, Boris Brezillon
> <boris.brezillon@free-electrons.com> wrote:
> > Hi Ezequiel,
> >
> > On Tue, 12 Apr 2016 13:26:35 -0300
> > Ezequiel Garcia <ezequiel@vanguardiasur.com.ar> wrote:
> >
> >> This commit introduces a MTD trigger for flash (NAND/NOR) device
> >> activity. The implementation is copied from IDE disk.
> >>
> >> This deprecates the "nand-disk" LED trigger, but for backwards
> >> compatibility, we still keep the "nand-disk" trigger around.
> >>
> >> The motivation for deprecating the "nand-disk" LED trigger is that
> >> it only works for NAND drivers, whereas the "mtd" LED trigger
> >> is more generic (in fact, "nand-disk" currently only works for
> >> certain NAND drivers).
> >>
> >> Signed-off-by: Ezequiel Garcia <ezequiel@vanguardiasur.com.ar>
> >> ---
> >>  drivers/leds/trigger/Kconfig       |  8 +++++++
> >>  drivers/leds/trigger/Makefile      |  1 +
> >>  drivers/leds/trigger/ledtrig-mtd.c | 49 ++++++++++++++++++++++++++++++++++++++
> >>  drivers/mtd/mtdcore.c              |  7 ++++++
> >>  drivers/mtd/nand/nand_base.c       | 29 +---------------------
> >>  include/linux/leds.h               |  6 +++++
> >
> > I'd suggest splitting this patch in 2, one adding ledtrig-mtd code, and
> > another one make use of ledtrig_mtd_activity() and removing
> > nand-trigger code.
> >
> 
> Sure, that sounds good.
> 
> > IMHO, we should also factorize ledtrig-ide and ledtrig-mtd code
> > (after all, you just copied the implementation from ledtrig-ide).
> > How about renaming the ledtrig-ide into ledtrig-storage and
> > ledtrig_ide_activity() into ledtrig_storage_activity()?
> >
> > You can then add a trigger named "storage" in addition to the existing
> > "nand-disk" and "ide-disk" ones.
> >
> 
> I'm not exactly following you: are you suggesting to suggest factorize
> the code, or to meld the two triggers in a single "storage" trigger?
> 
> In the former, the code is so small, that I'm not sure it will be actually
> reduced with some factorization. In the latter, wouldn't we loose
> the capability to trigger on MTD and IDE activity independently?

I was suggesting the latter, but didn't think about this mtd/nand vs ide
distinction. Not sure if this is really important, but it does change
the existing behavior.

Ideally, we should be able to select which device(s) trigger the
blinking, but that's clearly not trivial to implement.

BTW, you're already introducing a change in that non-NAND MTD devices
will now make the led blink even when only 'nand-disk' is set (I don't
think that's a real issue, but it slightly changes the
existing behavior).  


-- 
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

Re: [PATCH 2/2] leds: trigger: Introduce a MTD (NAND/NOR) trigger

[Ezequiel Garcia]

On 12 April 2016 at 15:40, Ezequiel Garcia
<ezequiel@vanguardiasur.com.ar> wrote:
> On 12 April 2016 at 15:27, Boris Brezillon
> <boris.brezillon@free-electrons.com> wrote:
>> Hi Ezequiel,
>>
>> On Tue, 12 Apr 2016 13:26:35 -0300
>> Ezequiel Garcia <ezequiel@vanguardiasur.com.ar> wrote:
>>
>>> This commit introduces a MTD trigger for flash (NAND/NOR) device
>>> activity. The implementation is copied from IDE disk.
>>>
>>> This deprecates the "nand-disk" LED trigger, but for backwards
>>> compatibility, we still keep the "nand-disk" trigger around.
>>>
>>> The motivation for deprecating the "nand-disk" LED trigger is that
>>> it only works for NAND drivers, whereas the "mtd" LED trigger
>>> is more generic (in fact, "nand-disk" currently only works for
>>> certain NAND drivers).
>>>
>>> Signed-off-by: Ezequiel Garcia <ezequiel@vanguardiasur.com.ar>
>>> ---
>>>  drivers/leds/trigger/Kconfig       |  8 +++++++
>>>  drivers/leds/trigger/Makefile      |  1 +
>>>  drivers/leds/trigger/ledtrig-mtd.c | 49 ++++++++++++++++++++++++++++++++++++++
>>>  drivers/mtd/mtdcore.c              |  7 ++++++
>>>  drivers/mtd/nand/nand_base.c       | 29 +---------------------
>>>  include/linux/leds.h               |  6 +++++
>>
>> I'd suggest splitting this patch in 2, one adding ledtrig-mtd code, and
>> another one make use of ledtrig_mtd_activity() and removing
>> nand-trigger code.
>>
>
> Sure, that sounds good.
>

One comment about the above: notice that if we split in two patches
as  you suggest, we would create a dependency between patches.

I don't have any problem doing this, but it sounds like it might make
maintainers
life harder.
-- 
Ezequiel García, VanguardiaSur
www.vanguardiasur.com.ar

Re: [PATCH 2/2] leds: trigger: Introduce a MTD (NAND/NOR) trigger

[Boris Brezillon]

On Tue, 12 Apr 2016 16:35:43 -0300
Ezequiel Garcia <ezequiel@vanguardiasur.com.ar> wrote:

> On 12 April 2016 at 15:40, Ezequiel Garcia
> <ezequiel@vanguardiasur.com.ar> wrote:
> > On 12 April 2016 at 15:27, Boris Brezillon
> > <boris.brezillon@free-electrons.com> wrote:
> >> Hi Ezequiel,
> >>
> >> On Tue, 12 Apr 2016 13:26:35 -0300
> >> Ezequiel Garcia <ezequiel@vanguardiasur.com.ar> wrote:
> >>
> >>> This commit introduces a MTD trigger for flash (NAND/NOR) device
> >>> activity. The implementation is copied from IDE disk.
> >>>
> >>> This deprecates the "nand-disk" LED trigger, but for backwards
> >>> compatibility, we still keep the "nand-disk" trigger around.
> >>>
> >>> The motivation for deprecating the "nand-disk" LED trigger is that
> >>> it only works for NAND drivers, whereas the "mtd" LED trigger
> >>> is more generic (in fact, "nand-disk" currently only works for
> >>> certain NAND drivers).
> >>>
> >>> Signed-off-by: Ezequiel Garcia <ezequiel@vanguardiasur.com.ar>
> >>> ---
> >>>  drivers/leds/trigger/Kconfig       |  8 +++++++
> >>>  drivers/leds/trigger/Makefile      |  1 +
> >>>  drivers/leds/trigger/ledtrig-mtd.c | 49 ++++++++++++++++++++++++++++++++++++++
> >>>  drivers/mtd/mtdcore.c              |  7 ++++++
> >>>  drivers/mtd/nand/nand_base.c       | 29 +---------------------
> >>>  include/linux/leds.h               |  6 +++++
> >>
> >> I'd suggest splitting this patch in 2, one adding ledtrig-mtd code, and
> >> another one make use of ledtrig_mtd_activity() and removing
> >> nand-trigger code.
> >>
> >
> > Sure, that sounds good.
> >
> 
> One comment about the above: notice that if we split in two patches
> as  you suggest, we would create a dependency between patches.
> 
> I don't have any problem doing this, but it sounds like it might make
> maintainers
> life harder.

I guess both patches will go through the same tree even if they are
split (either leds or mtd), so that's not really a problem.

Note that you have the same dependency problem with the single patch
approach (if another leds or mtd patch is touching one of the file
modified here, we may have a merge conflict).

-- 
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com