On Tue, Jan 08, 2013 at 02:18:04PM -0500, Josh Coombs wrote:

On Tue, Jan 8, 2013 at 2:06 PM, Jason Cooper [off-list ref] wrote:

quoted

On Tue, Jan 08, 2013 at 01:16:27PM -0500, Joshua Coombs wrote:

quoted

Add a call to the IDE LED Trigger within the Marvell SATA driver to allow
Marvell SoC devices to show SATA activity via GPIO connected LEDs.

Signed-off-by: Joshua Coombs <redacted>
---
 drivers/ata/sata_mv.c | 3 +++
 drivers/leds/Kconfig  | 3 +--
 2 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/ata/sata_mv.c b/drivers/ata/sata_mv.c
index 68f4fb5..4aaf6f0 100644
--- a/drivers/ata/sata_mv.c
+++ b/drivers/ata/sata_mv.c

@@ -71,6 +71,7 @@
 #include <scsi/scsi_cmnd.h>
 #include <scsi/scsi_device.h>
 #include <linux/libata.h>
+#include <linux/leds.h>

 #define DRV_NAME     "sata_mv"
 #define DRV_VERSION  "1.28"

@@ -1156,6 +1157,8 @@ static void mv_start_edma(struct ata_port *ap, void __iomem *port_mmio,
 {
      int want_ncq = (protocol == ATA_PROT_NCQ);

+     ledtrig_ide_activity();
+
      if (pp->pp_flags & MV_PP_FLAG_EDMA_EN) {
              int using_ncq = ((pp->pp_flags & MV_PP_FLAG_NCQ_EN) != 0);
              if (want_ncq != using_ncq)

diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
index b58bc8a..d2071d0 100644
--- a/drivers/leds/Kconfig
+++ b/drivers/leds/Kconfig

@@ -496,10 +496,9 @@ config LEDS_TRIGGER_ONESHOT

 config LEDS_TRIGGER_IDE_DISK
      bool "LED IDE Disk Trigger"
-     depends on IDE_GD_ATA
      depends on LEDS_TRIGGERS
      help
-       This allows LEDs to be controlled by IDE disk activity.
+       This allows LEDs to be controlled by IDE or SATA disk activity.
        If unsure, say Y.

 config LEDS_TRIGGER_HEARTBEAT

Hmmm, I'm not sure about this.  Ideally, wouldn't all sata users want to
have this option?  In which case, it should be changed to
LEDS_TRIGGER_DISK_IO or similar.  However, this seems to be the first
attempt at it:

$ git grep ledtrig_ide_activity -- drivers/ata/*.c
...nada...
$

At any rate, I'd like to see this patch extended to facilitate other
sata drivers using it.  eg adjusting the naming more appropriately, and
'depends on IDE_GD_ATA || ATA' or similar.  In it's current state, it
should depend on IDE_GD_ATA || SATA_MV.

I only have access to Marvell's SATA controller for testing, so that
is why I only targeted it.  The Kconfig depends change makes perfect
sense.

Changing the name of the trigger might cause hardship for those
already using it, as they will have to update scripts/etc to account
for the name change.  

I agree wrt to the sysfs interface, the current is 'ide-disk'.  Perhaps
a 'disk-io' could be added, and then ide-disk could be
legacy/deprecated?

Of course, /dev/sda refers to the first SCSI disk on my system, which
doesn't have SCSI...  Maybe it's best just to leave the sysfs interface
alone and extend the driver to support sata-attached disks as well.  I
could also see device-mapper read/writes as triggers as well.

Would it make more sense to add a second trigger
for SATA instead?

I'll leave that up to the led guys, I just wanted to raise the point
that the driver could logically support other types of disks, and we
should come up with a migration path instead of adding capabilities
ad-hoc.

thx,

Jason.