* [PATCH 0/2] LED trigger on MTD activity @ 2016-04-12 16:26 Ezequiel Garcia 2016-04-12 16:26 ` [PATCH 1/2] mtd: Uninline mtd_write_oob and move it to mtdcore.c Ezequiel Garcia 2016-04-12 16:26 ` [PATCH 2/2] leds: trigger: Introduce a MTD (NAND/NOR) trigger Ezequiel Garcia 0 siblings, 2 replies; 10+ messages in thread From: Ezequiel Garcia @ 2016-04-12 16:26 UTC (permalink / raw) To: linux-mtd, linux-leds Cc: Brian Norris, Boris Brezillon, Jacek Anaszewski, 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH 1/2] mtd: Uninline mtd_write_oob and move it to mtdcore.c 2016-04-12 16:26 [PATCH 0/2] LED trigger on MTD activity Ezequiel Garcia @ 2016-04-12 16:26 ` Ezequiel Garcia 2016-04-12 18:15 ` Boris Brezillon 2016-04-12 16:26 ` [PATCH 2/2] leds: trigger: Introduce a MTD (NAND/NOR) trigger Ezequiel Garcia 1 sibling, 1 reply; 10+ messages in thread From: Ezequiel Garcia @ 2016-04-12 16:26 UTC (permalink / raw) To: linux-mtd, linux-leds Cc: Brian Norris, Boris Brezillon, Jacek Anaszewski, 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 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH 1/2] mtd: Uninline mtd_write_oob and move it to mtdcore.c 2016-04-12 16:26 ` [PATCH 1/2] mtd: Uninline mtd_write_oob and move it to mtdcore.c Ezequiel Garcia @ 2016-04-12 18:15 ` Boris Brezillon 2016-04-12 18:35 ` Ezequiel Garcia 0 siblings, 1 reply; 10+ messages in thread From: Boris Brezillon @ 2016-04-12 18:15 UTC (permalink / raw) To: Ezequiel Garcia; +Cc: linux-mtd, linux-leds, Brian Norris, Jacek Anaszewski 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/2] mtd: Uninline mtd_write_oob and move it to mtdcore.c 2016-04-12 18:15 ` Boris Brezillon @ 2016-04-12 18:35 ` Ezequiel Garcia 0 siblings, 0 replies; 10+ messages in thread From: Ezequiel Garcia @ 2016-04-12 18:35 UTC (permalink / raw) To: Boris Brezillon Cc: linux-mtd@lists.infradead.org, Linux LED Subsystem, Brian Norris, Jacek Anaszewski 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH 2/2] leds: trigger: Introduce a MTD (NAND/NOR) trigger 2016-04-12 16:26 [PATCH 0/2] LED trigger on MTD activity Ezequiel Garcia 2016-04-12 16:26 ` [PATCH 1/2] mtd: Uninline mtd_write_oob and move it to mtdcore.c Ezequiel Garcia @ 2016-04-12 16:26 ` Ezequiel Garcia 2016-04-12 18:27 ` Boris Brezillon 1 sibling, 1 reply; 10+ messages in thread From: Ezequiel Garcia @ 2016-04-12 16:26 UTC (permalink / raw) To: linux-mtd, linux-leds Cc: Brian Norris, Boris Brezillon, Jacek Anaszewski, 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 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH 2/2] leds: trigger: Introduce a MTD (NAND/NOR) trigger 2016-04-12 16:26 ` [PATCH 2/2] leds: trigger: Introduce a MTD (NAND/NOR) trigger Ezequiel Garcia @ 2016-04-12 18:27 ` Boris Brezillon 2016-04-12 18:40 ` Ezequiel Garcia 0 siblings, 1 reply; 10+ messages in thread From: Boris Brezillon @ 2016-04-12 18:27 UTC (permalink / raw) To: Ezequiel Garcia; +Cc: linux-mtd, linux-leds, Brian Norris, Jacek Anaszewski 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 2/2] leds: trigger: Introduce a MTD (NAND/NOR) trigger 2016-04-12 18:27 ` Boris Brezillon @ 2016-04-12 18:40 ` Ezequiel Garcia 2016-04-12 19:11 ` Boris Brezillon 2016-04-12 19:35 ` Ezequiel Garcia 0 siblings, 2 replies; 10+ messages in thread From: Ezequiel Garcia @ 2016-04-12 18:40 UTC (permalink / raw) To: Boris Brezillon Cc: linux-mtd@lists.infradead.org, Linux LED Subsystem, Brian Norris, Jacek Anaszewski 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 2/2] leds: trigger: Introduce a MTD (NAND/NOR) trigger 2016-04-12 18:40 ` Ezequiel Garcia @ 2016-04-12 19:11 ` Boris Brezillon 2016-04-12 19:35 ` Ezequiel Garcia 1 sibling, 0 replies; 10+ messages in thread From: Boris Brezillon @ 2016-04-12 19:11 UTC (permalink / raw) To: Ezequiel Garcia Cc: linux-mtd@lists.infradead.org, Linux LED Subsystem, Brian Norris, Jacek Anaszewski 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 2/2] leds: trigger: Introduce a MTD (NAND/NOR) trigger 2016-04-12 18:40 ` Ezequiel Garcia 2016-04-12 19:11 ` Boris Brezillon @ 2016-04-12 19:35 ` Ezequiel Garcia 2016-04-12 19:46 ` Boris Brezillon 1 sibling, 1 reply; 10+ messages in thread From: Ezequiel Garcia @ 2016-04-12 19:35 UTC (permalink / raw) To: Boris Brezillon Cc: linux-mtd@lists.infradead.org, Linux LED Subsystem, Brian Norris, Jacek Anaszewski 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 2/2] leds: trigger: Introduce a MTD (NAND/NOR) trigger 2016-04-12 19:35 ` Ezequiel Garcia @ 2016-04-12 19:46 ` Boris Brezillon 0 siblings, 0 replies; 10+ messages in thread From: Boris Brezillon @ 2016-04-12 19:46 UTC (permalink / raw) To: Ezequiel Garcia Cc: linux-mtd@lists.infradead.org, Linux LED Subsystem, Brian Norris, Jacek Anaszewski 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2016-04-12 19:46 UTC | newest] Thread overview: 10+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2016-04-12 16:26 [PATCH 0/2] LED trigger on MTD activity Ezequiel Garcia 2016-04-12 16:26 ` [PATCH 1/2] mtd: Uninline mtd_write_oob and move it to mtdcore.c Ezequiel Garcia 2016-04-12 18:15 ` Boris Brezillon 2016-04-12 18:35 ` Ezequiel Garcia 2016-04-12 16:26 ` [PATCH 2/2] leds: trigger: Introduce a MTD (NAND/NOR) trigger Ezequiel Garcia 2016-04-12 18:27 ` Boris Brezillon 2016-04-12 18:40 ` Ezequiel Garcia 2016-04-12 19:11 ` Boris Brezillon 2016-04-12 19:35 ` Ezequiel Garcia 2016-04-12 19:46 ` Boris Brezillon
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).