| Message ID | 1357668987-3610-1-git-send-email-josh.coombs@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Jan 08, 2013 at 01:16:27PM -0500, Joshua Coombs wrote: > 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 <josh.coombs@gmail.com> > --- > 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. thx, Jason.
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. Would it make more sense to add a second trigger for SATA instead? Josh C On Tue, Jan 8, 2013 at 2:06 PM, Jason Cooper <jason@lakedaemon.net> wrote: > On Tue, Jan 08, 2013 at 01:16:27PM -0500, Joshua Coombs wrote: >> 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 <josh.coombs@gmail.com> >> --- >> 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. > > thx, > > Jason.
On Tue, Jan 08, 2013 at 02:18:04PM -0500, Josh Coombs wrote: > On Tue, Jan 8, 2013 at 2:06 PM, Jason Cooper <jason@lakedaemon.net> wrote: > > On Tue, Jan 08, 2013 at 01:16:27PM -0500, Joshua Coombs wrote: > >> 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 <josh.coombs@gmail.com> > >> --- > >> 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.
> -----Original Message----- > From: linux-leds-owner@vger.kernel.org [mailto:linux-leds- > owner@vger.kernel.org] On Behalf Of Jason Cooper > Sent: Wednesday, January 09, 2013 5:04 AM > To: Josh Coombs > Cc: cooloney@gmail.com; linux-kernel@vger.kernel.org; linux- > ide@vger.kernel.org; rpurdie@rpsys.net; linux ARM; jgarzik@pobox.com; > linux-leds@vger.kernel.org > Subject: Re: [PATCH] Allow Marvell SATA driver to work with > LEDS_TRIGGER_IDE_DISK > > On Tue, Jan 08, 2013 at 02:18:04PM -0500, Josh Coombs wrote: > > 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. I agree with Jason's suggestion. One more thing to consider. If we replace the name, 'ide-disk' or 'ide_disk' with general one, then we should change the value of 'default_trigger' under arch directory also. LED trigger works if the name is matched with trigger name of LED device. Thanks, Milo
I'm testing a revised version of this patch that renames the trigger to just 'disk' and updates all documentation and default_trigger definitions to match. I'm also registering the trigger name 'ide-disk' for now so external tools/scripts/distros won't break while a depreciation time is chosen. Once I've verified this behaves on my system, I'm assuming a patch that touches 25 files should be broken up? I'm thinking of submitting in the following chunks: ledtrig-ide-disk to ledtrig-disk documentation updates default-trigger updates Marvell SATA adjusted to call the trigger Any objections or comments on this plan of attack? Josh C On Tue, Jan 8, 2013 at 8:29 PM, Kim, Milo <Milo.Kim@ti.com> wrote: >> -----Original Message----- >> From: linux-leds-owner@vger.kernel.org [mailto:linux-leds- >> owner@vger.kernel.org] On Behalf Of Jason Cooper >> Sent: Wednesday, January 09, 2013 5:04 AM >> To: Josh Coombs >> Cc: cooloney@gmail.com; linux-kernel@vger.kernel.org; linux- >> ide@vger.kernel.org; rpurdie@rpsys.net; linux ARM; jgarzik@pobox.com; >> linux-leds@vger.kernel.org >> Subject: Re: [PATCH] Allow Marvell SATA driver to work with >> LEDS_TRIGGER_IDE_DISK >> >> On Tue, Jan 08, 2013 at 02:18:04PM -0500, Josh Coombs wrote: >> > 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. > > I agree with Jason's suggestion. One more thing to consider. > > If we replace the name, 'ide-disk' or 'ide_disk' with general one, > then we should change the value of 'default_trigger' under arch directory also. > LED trigger works if the name is matched with trigger name of LED device. > > Thanks, > Milo
On 01/08/2013 01:16 PM, Joshua Coombs wrote: > 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 <josh.coombs@gmail.com> > --- > drivers/ata/sata_mv.c | 3 +++ > drivers/leds/Kconfig | 3 +-- > 2 files changed, 4 insertions(+), 2 deletions(-) NAK. This is not appropriate at the specific driver level. This would be better implemented at a higher level.
Digging back through the archives, this came up once in 2007, Alan Cox did not like the idea of globally adding this call into the critical paths of libata at the time. (I've seen it done via external patches in ata_qc_issue and ata_qc_complete for example.) If that concern is no longer in place, I can redo with it within libata so it'll work for all libata driven devices similar to how it's handled for legacy IDE drivers now. Josh C On Wed, Jan 9, 2013 at 11:56 AM, Jeff Garzik <jgarzik@pobox.com> wrote: > On 01/08/2013 01:16 PM, Joshua Coombs wrote: >> >> 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 <josh.coombs@gmail.com> >> --- >> drivers/ata/sata_mv.c | 3 +++ >> drivers/leds/Kconfig | 3 +-- >> 2 files changed, 4 insertions(+), 2 deletions(-) > > > NAK. This is not appropriate at the specific driver level. This would be > better implemented at a higher level. > > >
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
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 <josh.coombs@gmail.com> --- drivers/ata/sata_mv.c | 3 +++ drivers/leds/Kconfig | 3 +-- 2 files changed, 4 insertions(+), 2 deletions(-)