Hi everyone,
This series adds support LED trigger for each ATA port indicating disk activity
in Armada 38x. I pick up the work done by Daniel Golle which can be found at [1]
Helios4 which is based on Armada 388, has four LEDs designed to indicate disk
activity on each SATA ports. As the final switch (CONFIG_ATA_LEDS) to enable
the codepath by default is no, it should not affect the rest of Armada 38x
based boards.
[1] https://patchwork.ozlabs.org/patch/420733/
---
Notes
checkpatch.pl complains about "WARNING: please write a paragraph that
describes the config symbol fully" but I think the description is clear
enough to ignore the warning.
Some performance number tested on Western Digital Red 2TB WD20EFRX
using fio randrw
* CONFIG_ATA_LEDS disabled and selected trigger is none
read : iops=326
write : iops=109
* CONFIG_ATA_LEDS disabled and selected trigger is disk-activity
read : iops=325
write : iops=108
* CONFIG_ATA_LEDS enabled and selected trigger is ata1
read : iops=325
write : iops=108
---
Aditya Prayoga (1):
ARM: mvebu: Enable ARCH_WANT_LIBATA_LEDS in Armada 38x
Daniel Golle (1):
libata: add ledtrig support
arch/arm/mach-mvebu/Kconfig | 1 +
drivers/ata/Kconfig | 16 +++++++++++++
drivers/ata/libata-core.c | 56 +++++++++++++++++++++++++++++++++++++++++++++
include/linux/libata.h | 7 ++++++
4 files changed, 80 insertions(+)
--
2.7.4
From: Daniel Golle <[email protected]>
This adds a LED trigger for each ATA port indicating disk activity.
As this is needed only on specific platforms (NAS SoCs and such),
these platforms should define ARCH_WANTS_LIBATA_LEDS if there
are boards with LED(s) intended to indicate ATA disk activity and
need the OS to take care of that.
In that way, if not selected, LED trigger support not will be
included in libata-core and both, codepaths and structures remain
untouched.
I'm currently deploying this for the oxnas target in OpenWrt
https://dev.openwrt.org/changeset/43675/
v2: rebased to kernel/git/tj/libata.git
plus small corrections and comments added
Signed-off-by: Daniel Golle <[email protected]>
URL: https://patchwork.ozlabs.org/patch/420733/
[Aditya Prayoga:
* Port forward
* Change ATA_LEDS default to no
* Reduce performance impact by moving ata_led_act() call from
ata_qc_new() to ata_qc_complete()]
Signed-off-by: Aditya Prayoga <[email protected]>
---
If there is anything fundamentally wrong with that approach, I'd be
glad for some advise on how to do it better.
I conciously choose an #ifdev approach to make sure performance will
not be affected for non-users of that code.
Thanks
Daniel
---
---
drivers/ata/Kconfig | 16 ++++++++++++++
drivers/ata/libata-core.c | 56 +++++++++++++++++++++++++++++++++++++++++++++++
include/linux/libata.h | 7 ++++++
3 files changed, 79 insertions(+)
diff --git a/drivers/ata/Kconfig b/drivers/ata/Kconfig
index 39b181d..a2c64ce 100644
--- a/drivers/ata/Kconfig
+++ b/drivers/ata/Kconfig
@@ -46,6 +46,22 @@ config ATA_VERBOSE_ERROR
If unsure, say Y.
+config ARCH_WANT_LIBATA_LEDS
+ bool
+
+config ATA_LEDS
+ bool "support ATA port LED triggers"
+ depends on ARCH_WANT_LIBATA_LEDS
+ select NEW_LEDS
+ select LEDS_CLASS
+ select LEDS_TRIGGERS
+ default n
+ help
+ This option adds a LED trigger for each registered ATA port.
+ It is used to drive disk activity leds connected via GPIO.
+
+ If unsure, say N.
+
config ATA_ACPI
bool "ATA ACPI Support"
depends on ACPI
diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
index 599e01b..65228f5 100644
--- a/drivers/ata/libata-core.c
+++ b/drivers/ata/libata-core.c
@@ -5105,6 +5105,30 @@ void swap_buf_le16(u16 *buf, unsigned int buf_words)
}
/**
+ * ata_led_act - Trigger port activity LED
+ * @ap: indicating port
+ *
+ * Blinks any LEDs registered to the trigger.
+ * Commonly used with leds-gpio on NAS systems with disk activity
+ * indicator LEDs.
+ *
+ * LOCKING:
+ * None.
+ */
+static inline void ata_led_act(struct ata_port *ap)
+{
+#ifdef CONFIG_ATA_LEDS
+#define LIBATA_BLINK_DELAY 20 /* ms */
+ unsigned long led_delay = LIBATA_BLINK_DELAY;
+
+ if (unlikely(!ap->ledtrig))
+ return;
+
+ led_trigger_blink_oneshot(ap->ledtrig, &led_delay, &led_delay, 0);
+#endif /* CONFIG_ATA_LEDS */
+}
+
+/**
* ata_qc_new_init - Request an available ATA command, and initialize it
* @dev: Device from whom we request an available command structure
* @tag: tag
@@ -5249,6 +5273,10 @@ void ata_qc_complete(struct ata_queued_cmd *qc)
/* Trigger the LED (if available) */
ledtrig_disk_activity(!!(qc->tf.flags & ATA_TFLAG_WRITE));
+#ifdef CONFIG_ATA_LEDS
+ ata_led_act(ap);
+#endif
+
/* XXX: New EH and old EH use different mechanisms to
* synchronize EH with regular execution path.
*
@@ -6028,6 +6056,9 @@ struct ata_port *ata_port_alloc(struct ata_host *host)
ap->stats.unhandled_irq = 1;
ap->stats.idle_irq = 1;
#endif
+#ifdef CONFIG_ATA_LEDS
+ ap->ledtrig = kzalloc(sizeof(struct led_trigger), GFP_KERNEL);
+#endif
ata_sff_port_init(ap);
return ap;
@@ -6063,6 +6094,12 @@ static void ata_host_release(struct kref *kref)
kfree(ap->pmp_link);
kfree(ap->slave_link);
+#ifdef CONFIG_ATA_LEDS
+ if (ap->ledtrig) {
+ led_trigger_unregister(ap->ledtrig);
+ kfree(ap->ledtrig);
+ };
+#endif
kfree(ap);
host->ports[i] = NULL;
}
@@ -6527,6 +6564,25 @@ int ata_host_register(struct ata_host *host, struct scsi_host_template *sht)
host->ports[i]->local_port_no = i + 1;
}
+#ifdef CONFIG_ATA_LEDS
+ /* register LED triggers for all ports */
+ for (i = 0; i < host->n_ports; i++) {
+ if (unlikely(!host->ports[i]->ledtrig))
+ continue;
+
+ snprintf(host->ports[i]->ledtrig_name,
+ sizeof(host->ports[i]->ledtrig_name), "ata%u",
+ host->ports[i]->print_id);
+
+ host->ports[i]->ledtrig->name = host->ports[i]->ledtrig_name;
+
+ if (led_trigger_register(host->ports[i]->ledtrig)) {
+ kfree(host->ports[i]->ledtrig);
+ host->ports[i]->ledtrig = NULL;
+ }
+ }
+#endif
+
/* Create associated sysfs transport objects */
for (i = 0; i < host->n_ports; i++) {
rc = ata_tport_add(host->dev,host->ports[i]);
diff --git a/include/linux/libata.h b/include/linux/libata.h
index 38c95d6..3cc5f63 100644
--- a/include/linux/libata.h
+++ b/include/linux/libata.h
@@ -38,6 +38,7 @@
#include <linux/acpi.h>
#include <linux/cdrom.h>
#include <linux/sched.h>
+#include <linux/leds.h>
/*
* Define if arch has non-standard setup. This is a _PCI_ standard
@@ -893,6 +894,12 @@ struct ata_port {
#ifdef CONFIG_ATA_ACPI
struct ata_acpi_gtm __acpi_init_gtm; /* use ata_acpi_init_gtm() */
#endif
+
+#ifdef CONFIG_ATA_LEDS
+ struct led_trigger *ledtrig;
+ char ledtrig_name[8];
+#endif
+
/* owned by EH */
u8 sector_buf[ATA_SECT_SIZE] ____cacheline_aligned;
};
--
2.7.4
Enable hidden symbol ARCH_WANT_LIBATA_LEDS so CONFIG_ATA_LEDS can be
used in kernel configuration.
URL: https://lists.openwrt.org/pipermail/openwrt-
devel/2017-March/006582.html
Signed-off-by: Aditya Prayoga <[email protected]>
---
arch/arm/mach-mvebu/Kconfig | 1 +
1 file changed, 1 insertion(+)
diff --git a/arch/arm/mach-mvebu/Kconfig b/arch/arm/mach-mvebu/Kconfig
index 2c20599..51f3256 100644
--- a/arch/arm/mach-mvebu/Kconfig
+++ b/arch/arm/mach-mvebu/Kconfig
@@ -68,6 +68,7 @@ config MACH_ARMADA_38X
select HAVE_SMP
select MACH_MVEBU_V7
select PINCTRL_ARMADA_38X
+ select ARCH_WANT_LIBATA_LEDS
help
Say 'Y' here if you want your kernel to support boards based
on the Marvell Armada 380/385 SoC with device tree.
--
2.7.4
On Wed, Sep 19, 2018 at 11:45:29AM +0800, Aditya Prayoga wrote:
> From: Daniel Golle <[email protected]>
>
> This adds a LED trigger for each ATA port indicating disk activity.
>
> As this is needed only on specific platforms (NAS SoCs and such),
> these platforms should define ARCH_WANTS_LIBATA_LEDS if there
> are boards with LED(s) intended to indicate ATA disk activity and
> need the OS to take care of that.
> In that way, if not selected, LED trigger support not will be
> included in libata-core and both, codepaths and structures remain
> untouched.
>
> I'm currently deploying this for the oxnas target in OpenWrt
> https://dev.openwrt.org/changeset/43675/
>
> v2: rebased to kernel/git/tj/libata.git
> plus small corrections and comments added
>
> Signed-off-by: Daniel Golle <[email protected]>
> URL: https://patchwork.ozlabs.org/patch/420733/
> [Aditya Prayoga:
> * Port forward
> * Change ATA_LEDS default to no
> * Reduce performance impact by moving ata_led_act() call from
> ata_qc_new() to ata_qc_complete()]
> Signed-off-by: Aditya Prayoga <[email protected]>
>
> ---
>
> If there is anything fundamentally wrong with that approach, I'd be
> glad for some advise on how to do it better.
> I conciously choose an #ifdev approach to make sure performance will
> not be affected for non-users of that code.
Hi Aditya
I remember something like this being discussed a long time ago, and it
was rejected because of the overheads. So it is good you have some
performance numbers. I will let others decide if the approach is
acceptable.
> /**
> + * ata_led_act - Trigger port activity LED
> + * @ap: indicating port
> + *
> + * Blinks any LEDs registered to the trigger.
> + * Commonly used with leds-gpio on NAS systems with disk activity
> + * indicator LEDs.
> + *
> + * LOCKING:
> + * None.
> + */
> +static inline void ata_led_act(struct ata_port *ap)
inline should not be used in C code, just header files. A function
this small the compiler is likely to inline anyway.
> +{
> +#ifdef CONFIG_ATA_LEDS
It is better to use if (IS_ENABLED(CONFIG_ATA_LEDS) {}. That gets
turned into if(0) {}, so the code still gets compiled to find any
errors, but then optimised out. This is important if the code is going
to be disabled by default.
> +#define LIBATA_BLINK_DELAY 20 /* ms */
> + unsigned long led_delay = LIBATA_BLINK_DELAY;
> +
> + if (unlikely(!ap->ledtrig))
> + return;
> +
> + led_trigger_blink_oneshot(ap->ledtrig, &led_delay, &led_delay, 0);
> +#endif /* CONFIG_ATA_LEDS */
> +}
> +
> +/**
> * ata_qc_new_init - Request an available ATA command, and initialize it
> * @dev: Device from whom we request an available command structure
> * @tag: tag
> @@ -5249,6 +5273,10 @@ void ata_qc_complete(struct ata_queued_cmd *qc)
> /* Trigger the LED (if available) */
> ledtrig_disk_activity(!!(qc->tf.flags & ATA_TFLAG_WRITE));
>
> +#ifdef CONFIG_ATA_LEDS
> + ata_led_act(ap);
> +#endif
No need for this #ifdef'ery.
> +
> /* XXX: New EH and old EH use different mechanisms to
> * synchronize EH with regular execution path.
> *
> @@ -6028,6 +6056,9 @@ struct ata_port *ata_port_alloc(struct ata_host *host)
> ap->stats.unhandled_irq = 1;
> ap->stats.idle_irq = 1;
> #endif
> +#ifdef CONFIG_ATA_LEDS
> + ap->ledtrig = kzalloc(sizeof(struct led_trigger), GFP_KERNEL);
Maybe use devm_kzalloc() and devm_led_trigger_register()?
Andrew
Hi Daniel,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on block/for-next]
[also build test ERROR on v4.19-rc4 next-20180919]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
url: https://github.com/0day-ci/linux/commits/Aditya-Prayoga/Add-support-per-ATA-port-ledtrigger-on-armada-38x/20180920-002037
base: https://git.kernel.org/pub/scm/linux/kernel/git/axboe/linux-block.git for-next
config: i386-randconfig-s1-201837 (attached as .config)
compiler: gcc-6 (Debian 6.4.0-9) 6.4.0 20171026
reproduce:
# save the attached .config to linux build tree
make ARCH=i386
All errors (new ones prefixed by >>):
In file included from drivers/scsi/mvsas/mv_94xx.c:27:0:
>> drivers/scsi/mvsas/mv_94xx.h:294:2: error: redeclaration of enumerator 'LED_OFF'
LED_OFF = 0,
^~~~~~~
In file included from include/linux/libata.h:41:0,
from include/scsi/libsas.h:33,
from drivers/scsi/mvsas/mv_sas.h:43,
from drivers/scsi/mvsas/mv_94xx.c:26:
include/linux/leds.h:30:2: note: previous definition of 'LED_OFF' was here
LED_OFF = 0,
^~~~~~~
In file included from drivers/scsi/mvsas/mv_94xx.c:27:0:
>> drivers/scsi/mvsas/mv_94xx.h:295:2: error: redeclaration of enumerator 'LED_ON'
LED_ON = 1,
^~~~~~
In file included from include/linux/libata.h:41:0,
from include/scsi/libsas.h:33,
from drivers/scsi/mvsas/mv_sas.h:43,
from drivers/scsi/mvsas/mv_94xx.c:26:
include/linux/leds.h:31:2: note: previous definition of 'LED_ON' was here
LED_ON = 1,
^~~~~~
vim +/LED_OFF +294 drivers/scsi/mvsas/mv_94xx.h
c56f5f1d Wilfried Weissmann 2015-12-27 292
c56f5f1d Wilfried Weissmann 2015-12-27 293 enum sgpio_led_status {
c56f5f1d Wilfried Weissmann 2015-12-27 @294 LED_OFF = 0,
c56f5f1d Wilfried Weissmann 2015-12-27 @295 LED_ON = 1,
c56f5f1d Wilfried Weissmann 2015-12-27 296 LED_BLINKA = 2,
c56f5f1d Wilfried Weissmann 2015-12-27 297 LED_BLINKA_INV = 3,
c56f5f1d Wilfried Weissmann 2015-12-27 298 LED_BLINKA_SOF = 4,
c56f5f1d Wilfried Weissmann 2015-12-27 299 LED_BLINKA_EOF = 5,
c56f5f1d Wilfried Weissmann 2015-12-27 300 LED_BLINKB = 6,
c56f5f1d Wilfried Weissmann 2015-12-27 301 LED_BLINKB_INV = 7,
c56f5f1d Wilfried Weissmann 2015-12-27 302 };
c56f5f1d Wilfried Weissmann 2015-12-27 303
:::::: The code at line 294 was first introduced by commit
:::::: c56f5f1de3a6ab8ec985edbc358e1fd8d4e36a65 mvsas: Add SGPIO support to Marvell 94xx
:::::: TO: Wilfried Weissmann <[email protected]>
:::::: CC: Martin K. Petersen <[email protected]>
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
Hi Daniel,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on block/for-next]
[also build test WARNING on v4.19-rc4 next-20180919]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
url: https://github.com/0day-ci/linux/commits/Aditya-Prayoga/Add-support-per-ATA-port-ledtrigger-on-armada-38x/20180920-002037
base: https://git.kernel.org/pub/scm/linux/kernel/git/axboe/linux-block.git for-next
coccinelle warnings: (new ones prefixed by >>)
>> drivers/ata/libata-core.c:6101:3-4: Unneeded semicolon
Please review and possibly fold the followup patch.
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
From: kbuild test robot <[email protected]>
drivers/ata/libata-core.c:6101:3-4: Unneeded semicolon
Remove unneeded semicolon.
Generated by: scripts/coccinelle/misc/semicolon.cocci
Fixes: 0ea8bf5a708e ("libata: add ledtrig support")
CC: Daniel Golle <[email protected]>
Signed-off-by: kbuild test robot <[email protected]>
---
url: https://github.com/0day-ci/linux/commits/Aditya-Prayoga/Add-support-per-ATA-port-ledtrigger-on-armada-38x/20180920-002037
base: https://git.kernel.org/pub/scm/linux/kernel/git/axboe/linux-block.git for-next
libata-core.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
--- a/drivers/ata/libata-core.c
+++ b/drivers/ata/libata-core.c
@@ -6098,7 +6098,7 @@ static void ata_host_release(struct kref
if (ap->ledtrig) {
led_trigger_unregister(ap->ledtrig);
kfree(ap->ledtrig);
- };
+ }
#endif
kfree(ap);
host->ports[i] = NULL;
Hi!
> +#ifdef CONFIG_ATA_LEDS
> + /* register LED triggers for all ports */
> + for (i = 0; i < host->n_ports; i++) {
> + if (unlikely(!host->ports[i]->ledtrig))
> + continue;
> +
> + snprintf(host->ports[i]->ledtrig_name,
> + sizeof(host->ports[i]->ledtrig_name), "ata%u",
> + host->ports[i]->print_id);
> + host->ports[i]->ledtrig->name = host->ports[i]->ledtrig_name;
> +
> + if (led_trigger_register(host->ports[i]->ledtrig)) {
> + kfree(host->ports[i]->ledtrig);
> + host->ports[i]->ledtrig = NULL;
> + }
> + }
> +#endif
No, we don't want you to register multiple triggers. We want one
trigger, than has parameter "which port to watch". (Number of triggers
is limited as by sysfs limitations).
Otherwise yes, ata trigger makes sense.
Pavel
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
On Wed 2018-09-19 11:45:30, Aditya Prayoga wrote:
> Enable hidden symbol ARCH_WANT_LIBATA_LEDS so CONFIG_ATA_LEDS can be
> used in kernel configuration.
Should that be hidden symbol and should that be architecture specific?
For a notebook, I may want scrolllock LED to indicate ATA activity
(because I'm using USB keyboard and can't see the disk led).
Hmm. And while at it... it would be nice to have option to watch all
ATA ports combined, as well.
Pavel
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
Hi!
On Thu, Sep 20, 2018 at 09:23:54AM +0200, Pavel Machek wrote:
> Hi!
>
> > +#ifdef CONFIG_ATA_LEDS
> > + /* register LED triggers for all ports */
> > + for (i = 0; i < host->n_ports; i++) {
> > + if (unlikely(!host->ports[i]->ledtrig))
> > + continue;
> > +
> > + snprintf(host->ports[i]->ledtrig_name,
> > + sizeof(host->ports[i]->ledtrig_name), "ata%u",
> > + host->ports[i]->print_id);
>
> > + host->ports[i]->ledtrig->name = host->ports[i]->ledtrig_name;
> > +
> > + if (led_trigger_register(host->ports[i]->ledtrig)) {
> > + kfree(host->ports[i]->ledtrig);
> > + host->ports[i]->ledtrig = NULL;
> > + }
> > + }
> > +#endif
>
> No, we don't want you to register multiple triggers. We want one
> trigger, than has parameter "which port to watch". (Number of triggers
> is limited as by sysfs limitations).
Back then I implemented it that way to be able to define the
default trigger for each LED in device tree and "trigger-sources"
didn't exist yet (it was introduced for USB ports and isn't yet used
for anything other than that)
However, the problem till today is also that ATA ports are often not
individual device-tree objects we can refer to, see for example
marvell,armada-370-sata which appears as one opaque controller. Ie.
all SATA drivers have to be converted to expose individual ports on
device-tree before the trigger-sources approach can be applied...
>
> Otherwise yes, ata trigger makes sense.
> Pavel
> --
> (english) http://www.livejournal.com/~pavelmachek
> (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
Hi Andrew,
thank you for your feedback. It seem i also need to resolve the issue
reported by kbuild test robot.
Aditya
Hi!
> > > +#ifdef CONFIG_ATA_LEDS
> > > + /* register LED triggers for all ports */
> > > + for (i = 0; i < host->n_ports; i++) {
> > > + if (unlikely(!host->ports[i]->ledtrig))
> > > + continue;
> > > +
> > > + snprintf(host->ports[i]->ledtrig_name,
> > > + sizeof(host->ports[i]->ledtrig_name), "ata%u",
> > > + host->ports[i]->print_id);
> >
> > > + host->ports[i]->ledtrig->name = host->ports[i]->ledtrig_name;
> > > +
> > > + if (led_trigger_register(host->ports[i]->ledtrig)) {
> > > + kfree(host->ports[i]->ledtrig);
> > > + host->ports[i]->ledtrig = NULL;
> > > + }
> > > + }
> > > +#endif
> >
> > No, we don't want you to register multiple triggers. We want one
> > trigger, than has parameter "which port to watch". (Number of triggers
> > is limited as by sysfs limitations).
>
> Back then I implemented it that way to be able to define the
> default trigger for each LED in device tree and "trigger-sources"
> didn't exist yet (it was introduced for USB ports and isn't yet used
> for anything other than that)
I see why you did it... BUt I believe we still want single trigger solution...
> However, the problem till today is also that ATA ports are often not
> individual device-tree objects we can refer to, see for example
> marvell,armada-370-sata which appears as one opaque controller. Ie.
> all SATA drivers have to be converted to expose individual ports on
> device-tree before the trigger-sources approach can be applied...
Yep, well... something to do in SATA then.
Perhaps this should also have an option for single LED for _any_ SATA activity,
and 90% devices will be happy with that?
Pavel
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
Hi Pavel,
On Fri, Sep 21, 2018 at 12:04:49AM +0200, Pavel Machek wrote:
> Hi!
>
> > > > +#ifdef CONFIG_ATA_LEDS
> > > > + /* register LED triggers for all ports */
> > > > + for (i = 0; i < host->n_ports; i++) {
> > > > + if (unlikely(!host->ports[i]->ledtrig))
> > > > + continue;
> > > > +
> > > > + snprintf(host->ports[i]->ledtrig_name,
> > > > + sizeof(host->ports[i]->ledtrig_name), "ata%u",
> > > > + host->ports[i]->print_id);
> > >
> > > > + host->ports[i]->ledtrig->name = host->ports[i]->ledtrig_name;
> > > > +
> > > > + if (led_trigger_register(host->ports[i]->ledtrig)) {
> > > > + kfree(host->ports[i]->ledtrig);
> > > > + host->ports[i]->ledtrig = NULL;
> > > > + }
> > > > + }
> > > > +#endif
> > >
> > > No, we don't want you to register multiple triggers. We want one
> > > trigger, than has parameter "which port to watch". (Number of triggers
> > > is limited as by sysfs limitations).
> >
> > Back then I implemented it that way to be able to define the
> > default trigger for each LED in device tree and "trigger-sources"
> > didn't exist yet (it was introduced for USB ports and isn't yet used
> > for anything other than that)
>
> I see why you did it... BUt I believe we still want single trigger solution...
>
> > However, the problem till today is also that ATA ports are often not
> > individual device-tree objects we can refer to, see for example
> > marvell,armada-370-sata which appears as one opaque controller. Ie.
> > all SATA drivers have to be converted to expose individual ports on
> > device-tree before the trigger-sources approach can be applied...
>
> Yep, well... something to do in SATA then.
>
> Perhaps this should also have an option for single LED for _any_ SATA activity,
> and 90% devices will be happy with that?
The whole reason for not using one of the existing disk-activity
triggers was to address individual SATA ports to individual LEDs of NAS
devices (in my case Shuttle KD20)...
On Thu, Sep 20, 2018 at 2:26 PM Pavel Machek <[email protected]> wrote:
>
> On Wed 2018-09-19 11:45:30, Aditya Prayoga wrote:
> > Enable hidden symbol ARCH_WANT_LIBATA_LEDS so CONFIG_ATA_LEDS can be
> > used in kernel configuration.
>
> Should that be hidden symbol and should that be architecture specific?
>
The intention was to minimize the risk of the ledtrig code being
included by accident. The
code itself does not have architecture specific instruction
> For a notebook, I may want scrolllock LED to indicate ATA activity
> (because I'm using USB keyboard and can't see the disk led).
>
> Hmm. And while at it... it would be nice to have option to watch all
> ATA ports combined, as well.
but there is disk-activity trigger for that purpose.
Aditya
> Pavel
> --
> (english) http://www.livejournal.com/~pavelmachek
> (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html