Re: [PATCH v2] mtd: Add LED trigger support "mtd-disk" to indicate activity

[Ezequiel Garcia <ezequiel.garcia@free-electrons.com>]

Hi Jens,

On Sun, Jun 02, 2013 at 05:53:13PM +0200, Jens Renner (EFE) wrote:
> Register a MTD LED trigger called "mtd-disk" (similar to the "ide-disk" and
> "nand-disk" triggers) to indicate read / write / erase acitivity. Panic writes
> and OOB reads / writes are not covered as of now. The trigger is global as it
> does not discriminate between individual devices or partitions.
> The patch uses the generic LED trigger interface which can be configured via
> SYSFS (/sys/class/leds/<name>/trigger) or DTS file entry for "gpio-leds"
> (linux,default-trigger = "mtd-disk").
> 
> Since the MTD framework is independant of the memory devices, driver-specific
> LED triggers like "nand-disk" will indicate a subset of all MTD activity.
> 
> Tested on Microblaze architecture with Micron N25Q256A serial flash.
> Added a bit of documentation (including other new LED triggers).
> 

Thanks for doing this and sorry for such late feedback.
For some reason I thought this would be already merged,
and now looking at it in detail I have some comments about it.

I'm Ccing Brian, David and Artem just to make sure I don't say something
foolish.

I think you should re-work this entirely and add this trigger as a
selectable module just as the rest of the led triggers do it.
See drivers/led/trigger/ for examples.

In addition you could move nand-disk over there as well.

Having all related code in a single location is always better than
spreading all over the place: you get to hide implementation details
easier, and you can spot common code or bugs by comparison.

What do you think?
-- 
Ezequiel García, Free Electrons
Embedded Linux, Kernel and Android Engineering
http://free-electrons.com