[PATCH 001/001] PMAC HD runtime blinking control

[PATCH 001/001] PMAC HD runtime blinking control

[Cedric Pradalier]

Hi,

I've finally spend the time to mend the patch for control of
the HD led blinking at runtime. This is a patch against
2.6.15

The sysfs entry is attached to the PCI device and to the
MACIO device if available. I think that was what Ben was
asking for:

lauren-ph:/sys# find . | grep blink
./module/ide_core/parameters/noblink
./devices/pci0001:10/0001:10:17.0/blinking_led
./devices/pci0001:10/0001:10:17.0/0.80000000:mac-io/0.0001f000:ata-4/blinking_led


To activate blinking:
echo 1 > ./devices/pci0001:10/0001:10:17.0/blinking_led

To deactivate it:
echo 0 > ./devices/pci0001:10/0001:10:17.0/blinking_led

There is also a boot time parameter idecore:noblink to
deactivate the blinking at this stage.


signed-off-by: Cedric Pradalier <cedric.pradalier@free.fr>
---

--- drivers/ide/ppc/pmac.c.orig	2006-01-03
13:21:10.000000000 +1000 +++ drivers/ide/ppc/pmac.c
2006-01-22 11:51:11.000000000 +1000 @@ -36,6 +36,11 @@
 #include <linux/pmu.h>
 #include <linux/scatterlist.h>
 
+#ifdef CONFIG_BLK_DEV_IDE_PMAC_BLINK
+#include <linux/device.h>
+#include <asm/of_device.h>
+#endif
+
 #include <asm/prom.h>
 #include <asm/io.h>
 #include <asm/dbdma.h>
@@ -427,6 +432,15 @@ static void pmac_ide_kauai_selectproc
(id 
 #ifdef CONFIG_BLK_DEV_IDE_PMAC_BLINK
 
+MODULE_AUTHOR("Paul Mackerras & Ben. Herrenschmidt");
+MODULE_DESCRIPTION("Support for IDE interfaces on
PowerMacs"); +MODULE_LICENSE("GPL");
+
+static int blinking_led = 1;
+module_param_named(noblink,blinking_led, invbool, 0666);
+MODULE_PARM_DESC(noblink,"Enable/Disable blinking led
[Default: enabled]"); +
+
 /* Set to 50ms minimum led-on time (also used to limit
frequency
  * of requests sent to the PMU
  */
@@ -437,8 +451,7 @@ static spinlock_t pmu_blink_lock;
 static unsigned long pmu_blink_stoptime;
 static int pmu_blink_ledstate;
 static struct timer_list pmu_blink_timer;
-static int pmu_ide_blink_enabled;
-
+static int pmu_ide_blink_enabled = 0;
 
 static void
 pmu_hd_blink_timeout(unsigned long data)
@@ -468,6 +481,8 @@ static void
 pmu_hd_kick_blink(void *data, int rw)
 {
 	unsigned long flags;
+	if (!blinking_led)
+		return;
 	
 	pmu_blink_stoptime = jiffies + PMU_HD_BLINK_TIME;
 	wmb();
@@ -483,8 +498,28 @@ pmu_hd_kick_blink(void *data, int rw)
 	spin_unlock_irqrestore(&pmu_blink_lock, flags);
 }
 
+static ssize_t show_blinkingled_activity(struct device
*dev, struct device_attribute *attr, char *buf)\ +{  
+	return sprintf(buf, "%c\n", blinking_led?'1':'0'); 
+}
+
+static ssize_t set_blinkingled_activity(struct device
*dev, struct device_attribute *attr, 
+		const char *buf, size_t count)
+{
+	int blink;
+	if (sscanf (buf, "%d", &blink) != 1)
+		return -EINVAL;
+	blinking_led = (blink != 0);
+	printk(KERN_INFO "pmac blinking led initialized
(blink %s)\n",
+			blinking_led?"enabled":"disabled");
+	return count;
+}
+
+static DEVICE_ATTR (blinking_led, S_IRUGO | S_IWUSR, 
+		show_blinkingled_activity,
set_blinkingled_activity); +
 static int
-pmu_hd_blink_init(void)
+pmu_hd_blink_init(pmac_ide_hwif_t *pmif, ide_hwif_t *hwif)
 {
 	struct device_node *dt;
 	const char *model;
@@ -516,6 +551,13 @@ pmu_hd_blink_init(void)
 	init_timer(&pmu_blink_timer);
 	pmu_blink_timer.function = pmu_hd_blink_timeout;
 
+	device_create_file (&hwif->pci_dev->dev,
&dev_attr_blinking_led);
+	if (pmif->mdev != NULL)
+		device_create_file
(&pmif->mdev->ofdev.dev, &dev_attr_blinking_led);
+	
+	printk(KERN_INFO "pmac blinking led initialized
(blink %s)\n",
+			blinking_led?"enabled":"disabled");
+
 	return 1;
 }
 
@@ -1271,7 +1313,7 @@ static int
 pmac_ide_setup_device(pmac_ide_hwif_t *pmif, ide_hwif_t
*hwif) {
 	struct device_node *np = pmif->node;
-	int *bidp, i;
+	int *bidp;
 
 	pmif->cable_80 = 0;
 	pmif->broken_dma = pmif->broken_dma_warn = 0;
@@ -1375,7 +1417,7 @@ pmac_ide_setup_device(pmac_ide_hwif_t
*p hwif->speedproc = pmac_ide_tune_chipset;
 
 #ifdef CONFIG_BLK_DEV_IDE_PMAC_BLINK
-	pmu_ide_blink_enabled = pmu_hd_blink_init();
+	pmu_ide_blink_enabled = pmu_hd_blink_init
(pmif,hwif); 
 	if (pmu_ide_blink_enabled)
 		hwif->led_act = pmu_hd_kick_blink;
@@ -1476,6 +1518,7 @@ pmac_ide_macio_attach(struct
macio_dev * #endif /* CONFIG_BLK_DEV_IDEDMA_PMAC */
 	dev_set_drvdata(&mdev->ofdev.dev, hwif);
 
+	printk(KERN_INFO "pmac: using macio interface");
 	rc = pmac_ide_setup_device(pmif, hwif);
 	if (rc != 0) {
 		/* The inteface is released to the common
IDE layer */ @@ -1584,6 +1627,7 @@ pmac_ide_pci_attach
(struct pci_dev *pdev 
 	pci_set_drvdata(pdev, hwif);
 
+	printk(KERN_INFO "pmac: using PCI interface");
 	rc = pmac_ide_setup_device(pmif, hwif);
 	if (rc != 0) {
 		/* The inteface is released to the common
IDE layer */

Re: [PATCH 001/001] PMAC HD runtime blinking control

[Benjamin Herrenschmidt]

On Sun, 2006-01-22 at 12:19 +1000, Cedric Pradalier wrote:
> Hi,
> 
> I've finally spend the time to mend the patch for control of
> the HD led blinking at runtime. This is a patch against
> 2.6.15
> 
> The sysfs entry is attached to the PCI device and to the
> MACIO device if available. I think that was what Ben was
> asking for:

Heh, nice :) Almost ! It would be better if it was attached to the sysfs
node of the ide interface, you can find it in the HWIF array after
probe, but I won't be too much of a pain about that for now :)

Ben.

Re: [PATCH 001/001] PMAC HD runtime blinking control

[Cedric Pradalier]

According to Benjamin Herrenschmidt, on Sun, 22 Jan 2006
13:49:53 +1100, 
>On Sun, 2006-01-22 at 12:19 +1000, Cedric Pradalier wrote:
>> Hi,
>> 
>> I've finally spend the time to mend the patch for control of
>> the HD led blinking at runtime. This is a patch against
>> 2.6.15
>> 
>> The sysfs entry is attached to the PCI device and to the
>> MACIO device if available. I think that was what Ben was
>> asking for:
>
>Heh, nice :) Almost ! It would be better if it was attached to the sysfs
>node of the ide interface, you can find it in the HWIF array after
>probe, but I won't be too much of a pain about that for now :)
>

Yep, now I remember, that was what you where asking for.
Can you give me an idea of the sysfs path you're expecting?
Is it in 
bus/ide/..., 
bus/pci/drivers/ide-pmac,
bus/pci/drivers/ide-disk
devices/pci0001:10/0001:10:17.0/0.80000000:mac-io/0.0001f000:ata-4/ide0

I would guess on the latter. 
For now I'm using the pmif->mdev->ofdev.dev as attachment
device, which brings me into 0.0001f000:ata-4
What in pmif can brings me to the ide0? pmif->node? Or one
of its siblings?

In ide_hwif_t, I'm using the pci_dev. Where in the pci_dev
can I find ide0? or is it somewhere else in ide_hwif_t?

For the HWIF array, I guess you're talking about ide_hwifs.
How do I identify the interface itself there?

Hmm, plenty of questions. I hope at least one is relevant
for this problem...

Thanks
--
Cedric

Re: [PATCH 001/001] PMAC HD runtime blinking control

[Benjamin Herrenschmidt]

On Sun, 2006-01-22 at 18:11 +1000, Cedric Pradalier wrote:

> In ide_hwif_t, I'm using the pci_dev. Where in the pci_dev
> can I find ide0? or is it somewhere else in ide_hwif_t?

Yah, I think it's in there (in ide_hwif_t)

> For the HWIF array, I guess you're talking about ide_hwifs.
> How do I identify the interface itself there?

well, depends what you are working from...  pmif has an index into it
for example...

> Hmm, plenty of questions. I hope at least one is relevant
> for this problem...
> 
> Thanks
> --
> Cedric

Re: [PATCH 001/001] PMAC HD runtime blinking control

[Arkadiusz Miskiewicz]

On Sunday 22 January 2006 03:19, Cedric Pradalier wrote:
> Hi,
>
> I've finally spend the time to mend the patch for control of
> the HD led blinking at runtime. This is a patch against
> 2.6.15

I wonder if it's also possible to disable blinking when laptop sleeps?

=2D-=20
Arkadiusz Mi=B6kiewicz                    PLD/Linux Team
http://www.t17.ds.pwr.wroc.pl/~misiek/  http://ftp.pld-linux.org/

Re: [PATCH 001/001] PMAC HD runtime blinking control

[Benjamin Herrenschmidt]

On Sun, 2006-01-22 at 12:56 +0100, Arkadiusz Miskiewicz wrote:
> On Sunday 22 January 2006 03:19, Cedric Pradalier wrote:
> > Hi,
> >
> > I've finally spend the time to mend the patch for control of
> > the HD led blinking at runtime. This is a patch against
> > 2.6.15
> 
> I wonder if it's also possible to disable blinking when laptop sleeps?

Not that I know.. this is entirely done by the PMU.

Ben.

[PATCH 001/001 Updated] PMAC HD runtime blinking control

[Cedric Pradalier]

According to Benjamin Herrenschmidt, on Sun, 22 Jan 2006
13:49:53 +1100, 
>On Sun, 2006-01-22 at 12:19 +1000, Cedric Pradalier wrote:
>> Hi,
>> 
>> I've finally spend the time to mend the patch for control of
>> the HD led blinking at runtime. This is a patch against
>> 2.6.15
>> 
>> The sysfs entry is attached to the PCI device and to the
>> MACIO device if available. I think that was what Ben was
>> asking for:
>
>Heh, nice :) Almost ! It would be better if it was attached to the sysfs
>node of the ide interface, you can find it in the HWIF array after
>probe, but I won't be too much of a pain about that for now :)
>
>Ben.
>
>

OK, I think now I got it.

pradalie@lauren-ph:~$ find /sys/ | grep blink 
/sys/devices/pci0001:10/0001:10:17.0/0.80000000:mac-io/0.0001f000:ata-4/ide0/blinking_led

The key I could not understand was that hwif->gendev is
only initialised in the probe. So I had to move the
device creation after that.

Currently, it is blinking by default. Should it be that
way? I guess so, since it is activated by a kernel config
option. It is easy to change if required.

Anyway, here is the updated patch.

signed-off-by: Cedric Pradalier <cedric.pradalier@free.fr>
---
--- drivers/ide/ppc/pmac.c.orig	2006-01-03 13:21:10.000000000 +1000
+++ drivers/ide/ppc/pmac.c	2006-01-23 23:32:18.000000000 +1000
@@ -36,6 +36,11 @@
 #include <linux/pmu.h>
 #include <linux/scatterlist.h>
 
+#ifdef CONFIG_BLK_DEV_IDE_PMAC_BLINK
+#include <linux/device.h>
+#include <asm/of_device.h>
+#endif
+
 #include <asm/prom.h>
 #include <asm/io.h>
 #include <asm/dbdma.h>
@@ -427,6 +432,15 @@ static void pmac_ide_kauai_selectproc(id
 
 #ifdef CONFIG_BLK_DEV_IDE_PMAC_BLINK
 
+MODULE_AUTHOR("Paul Mackerras & Ben. Herrenschmidt");
+MODULE_DESCRIPTION("Support for IDE interfaces on PowerMacs");
+MODULE_LICENSE("GPL");
+
+static int blinking_led = 1;
+module_param_named(noblink,blinking_led, invbool, 0666);
+MODULE_PARM_DESC(noblink,"Enable/Disable blinking led [Default: enabled]");
+
+
 /* Set to 50ms minimum led-on time (also used to limit frequency
  * of requests sent to the PMU
  */
@@ -437,8 +451,7 @@ static spinlock_t pmu_blink_lock;
 static unsigned long pmu_blink_stoptime;
 static int pmu_blink_ledstate;
 static struct timer_list pmu_blink_timer;
-static int pmu_ide_blink_enabled;
-
+static int pmu_ide_blink_enabled = 0;
 
 static void
 pmu_hd_blink_timeout(unsigned long data)
@@ -468,6 +481,8 @@ static void
 pmu_hd_kick_blink(void *data, int rw)
 {
 	unsigned long flags;
+	if (!blinking_led)
+		return;
 	
 	pmu_blink_stoptime = jiffies + PMU_HD_BLINK_TIME;
 	wmb();
@@ -483,6 +498,26 @@ pmu_hd_kick_blink(void *data, int rw)
 	spin_unlock_irqrestore(&pmu_blink_lock, flags);
 }
 
+static ssize_t show_blinkingled_activity(struct device *dev, struct device_attribute *attr, char *buf)\
+{  
+	return sprintf(buf, "%c\n", blinking_led?'1':'0'); 
+}
+
+static ssize_t set_blinkingled_activity(struct device *dev, struct device_attribute *attr, 
+		const char *buf, size_t count)
+{
+	int blink;
+	if (sscanf (buf, "%d", &blink) != 1)
+		return -EINVAL;
+	blinking_led = (blink != 0);
+	printk(KERN_INFO "pmac blinking led initialized (blink %s)\n",
+			blinking_led?"enabled":"disabled");
+	return count;
+}
+
+static DEVICE_ATTR (blinking_led, S_IRUGO | S_IWUSR, 
+		show_blinkingled_activity, set_blinkingled_activity);
+
 static int
 pmu_hd_blink_init(void)
 {
@@ -516,6 +551,9 @@ pmu_hd_blink_init(void)
 	init_timer(&pmu_blink_timer);
 	pmu_blink_timer.function = pmu_hd_blink_timeout;
 
+	printk(KERN_INFO "pmac blinking led initialized (blink %s)\n",
+			blinking_led?"enabled":"disabled");
+
 	return 1;
 }
 
@@ -1271,7 +1309,7 @@ static int
 pmac_ide_setup_device(pmac_ide_hwif_t *pmif, ide_hwif_t *hwif)
 {
 	struct device_node *np = pmif->node;
-	int *bidp, i;
+	int *bidp;
 
 	pmif->cable_80 = 0;
 	pmif->broken_dma = pmif->broken_dma_warn = 0;
@@ -1401,6 +1439,12 @@ pmac_ide_setup_device(pmac_ide_hwif_t *p
 	/* We probe the hwif now */
 	probe_hwif_init(hwif);
 
+#ifdef CONFIG_BLK_DEV_IDE_PMAC_BLINK
+	/* We wait till here to have the gendev initialized in hwif */
+	device_create_file (&hwif->gendev, &dev_attr_blinking_led);
+#endif
+
+
 	return 0;
 }
 

Re: [PATCH 001/001 Updated] PMAC HD runtime blinking control

[Benjamin Herrenschmidt]

> The key I could not understand was that hwif->gendev is
> only initialised in the probe. So I had to move the
> device creation after that.
> 
> Currently, it is blinking by default. Should it be that
> way? I guess so, since it is activated by a kernel config
> option. It is easy to change if required.

Yes. In fact, by enabled default for ATA disks and by disabled for ATAPI
would make sense...

Also, we should think a bit about the file name... "blinking_led" isn't
terrific for something that will end up in a non-ppc specific location.
Or maybe on the contrary it's good ... what about "activity_led"
rather ?

> Anyway, here is the updated patch.
> 
> signed-off-by: Cedric Pradalier <cedric.pradalier@free.fr>
> ---
> --- drivers/ide/ppc/pmac.c.orig	2006-01-03 13:21:10.000000000 +1000
> +++ drivers/ide/ppc/pmac.c	2006-01-23 23:32:18.000000000 +1000
> @@ -36,6 +36,11 @@
>  #include <linux/pmu.h>
>  #include <linux/scatterlist.h>
>  
> +#ifdef CONFIG_BLK_DEV_IDE_PMAC_BLINK
> +#include <linux/device.h>
> +#include <asm/of_device.h>
> +#endif
> +
>  #include <asm/prom.h>
>  #include <asm/io.h>
>  #include <asm/dbdma.h>
> @@ -427,6 +432,15 @@ static void pmac_ide_kauai_selectproc(id
>  
>  #ifdef CONFIG_BLK_DEV_IDE_PMAC_BLINK
>  
> +MODULE_AUTHOR("Paul Mackerras & Ben. Herrenschmidt");
> +MODULE_DESCRIPTION("Support for IDE interfaces on PowerMacs");
> +MODULE_LICENSE("GPL");
> +
> +static int blinking_led = 1;
> +module_param_named(noblink,blinking_led, invbool, 0666);
> +MODULE_PARM_DESC(noblink,"Enable/Disable blinking led [Default: enabled]");
> +
> +
>  /* Set to 50ms minimum led-on time (also used to limit frequency
>   * of requests sent to the PMU
>   */
> @@ -437,8 +451,7 @@ static spinlock_t pmu_blink_lock;
>  static unsigned long pmu_blink_stoptime;
>  static int pmu_blink_ledstate;
>  static struct timer_list pmu_blink_timer;
> -static int pmu_ide_blink_enabled;
> -
> +static int pmu_ide_blink_enabled = 0;
>  
>  static void
>  pmu_hd_blink_timeout(unsigned long data)
> @@ -468,6 +481,8 @@ static void
>  pmu_hd_kick_blink(void *data, int rw)
>  {
>  	unsigned long flags;
> +	if (!blinking_led)
> +		return;
>  	
>  	pmu_blink_stoptime = jiffies + PMU_HD_BLINK_TIME;
>  	wmb();
> @@ -483,6 +498,26 @@ pmu_hd_kick_blink(void *data, int rw)
>  	spin_unlock_irqrestore(&pmu_blink_lock, flags);
>  }
>  
> +static ssize_t show_blinkingled_activity(struct device *dev, struct device_attribute *attr, char *buf)\
> +{  
> +	return sprintf(buf, "%c\n", blinking_led?'1':'0'); 
> +}
> +
> +static ssize_t set_blinkingled_activity(struct device *dev, struct device_attribute *attr, 
> +		const char *buf, size_t count)
> +{
> +	int blink;
> +	if (sscanf (buf, "%d", &blink) != 1)
> +		return -EINVAL;
> +	blinking_led = (blink != 0);
> +	printk(KERN_INFO "pmac blinking led initialized (blink %s)\n",
> +			blinking_led?"enabled":"disabled");
> +	return count;
> +}
> +
> +static DEVICE_ATTR (blinking_led, S_IRUGO | S_IWUSR, 
> +		show_blinkingled_activity, set_blinkingled_activity);
> +
>  static int
>  pmu_hd_blink_init(void)
>  {
> @@ -516,6 +551,9 @@ pmu_hd_blink_init(void)
>  	init_timer(&pmu_blink_timer);
>  	pmu_blink_timer.function = pmu_hd_blink_timeout;
>  
> +	printk(KERN_INFO "pmac blinking led initialized (blink %s)\n",
> +			blinking_led?"enabled":"disabled");
> +
>  	return 1;
>  }
>  
> @@ -1271,7 +1309,7 @@ static int
>  pmac_ide_setup_device(pmac_ide_hwif_t *pmif, ide_hwif_t *hwif)
>  {
>  	struct device_node *np = pmif->node;
> -	int *bidp, i;
> +	int *bidp;
>  
>  	pmif->cable_80 = 0;
>  	pmif->broken_dma = pmif->broken_dma_warn = 0;
> @@ -1401,6 +1439,12 @@ pmac_ide_setup_device(pmac_ide_hwif_t *p
>  	/* We probe the hwif now */
>  	probe_hwif_init(hwif);
>  
> +#ifdef CONFIG_BLK_DEV_IDE_PMAC_BLINK
> +	/* We wait till here to have the gendev initialized in hwif */
> +	device_create_file (&hwif->gendev, &dev_attr_blinking_led);
> +#endif
> +
> +
>  	return 0;
>  }
>  

Re: [PATCH 001/001 Updated] PMAC HD runtime blinking control

[Cedric Pradalier]

According to Benjamin Herrenschmidt, on Tue, 24 Jan 2006
00:47:16 +1100, 
>> The key I could not understand was that hwif->gendev is
>> only initialised in the probe. So I had to move the
>> device creation after that.
>> 
>> Currently, it is blinking by default. Should it be that
>> way? I guess so, since it is activated by a kernel config
>> option. It is easy to change if required.
>
>Yes. In fact, by enabled default for ATA disks and by disabled for ATAPI
>would make sense...

How do I tell the difference?. There is a 'kind' in pmif,
and also a atapi_dma flag in hwif. Which is more sensible?

>
>Also, we should think a bit about the file name... "blinking_led" isn't
>terrific for something that will end up in a non-ppc specific location.
>Or maybe on the contrary it's good ... what about "activity_led"
>rather ?
>

I'm open to any suggestion. I'll wait a bit to see if
someone else has a comment, then I'll change to
"activity_led". 

--
Cedric

Re: [PATCH 001/001 Updated] PMAC HD runtime blinking control

[Benjamin Herrenschmidt]

On Tue, 2006-01-24 at 07:31 +1000, Cedric Pradalier wrote:
> According to Benjamin Herrenschmidt, on Tue, 24 Jan 2006
> 00:47:16 +1100, 
> >> The key I could not understand was that hwif->gendev is
> >> only initialised in the probe. So I had to move the
> >> device creation after that.
> >> 
> >> Currently, it is blinking by default. Should it be that
> >> way? I guess so, since it is activated by a kernel config
> >> option. It is easy to change if required.
> >
> >Yes. In fact, by enabled default for ATA disks and by disabled for ATAPI
> >would make sense...
> 
> How do I tell the difference?. There is a 'kind' in pmif,
> and also a atapi_dma flag in hwif. Which is more sensible?

You need to check drive->media ... Which is a bit annoying since it mans
it's per drive, not per-HWIF... you may want to move your sysfs entry
down one more level :) (Each HWIF has an array of 2 drives, though check
drive->present before expecting a useful struct device there)

> >Also, we should think a bit about the file name... "blinking_led" isn't
> >terrific for something that will end up in a non-ppc specific location.
> >Or maybe on the contrary it's good ... what about "activity_led"
> >rather ?
> >
> 
> I'm open to any suggestion. I'll wait a bit to see if
> someone else has a comment, then I'll change to
> "activity_led". 
> 
> --
> Cedric

[PATCH 001/001 Updated again] PMAC HD runtime blinking control

[Cedric Pradalier]

According to Benjamin Herrenschmidt, on Tue, 24 Jan 2006
10:10:26 +1100, 
>On Tue, 2006-01-24 at 07:31 +1000, Cedric Pradalier wrote:
>> >
>> >Yes. In fact, by enabled default for ATA disks and by disabled for ATAPI
>> >would make sense...
>> 
>> How do I tell the difference?. There is a 'kind' in pmif,
>> and also a atapi_dma flag in hwif. Which is more sensible?
>
>You need to check drive->media ... Which is a bit annoying since it mans
>it's per drive, not per-HWIF... you may want to move your sysfs entry
>down one more level :) (Each HWIF has an array of 2 drives, though check
>drive->present before expecting a useful struct device there)
>

Well in this case, I'll let this part waiting for now. 
I don't think there will be enough real users of this minor
thing to justify putting any more efforts for now.

I think the practical solution is to disable the led by
default. The few user who will want it will be able to add
either ide_core.blink to their boot parameter or make a
small init.d script to put 1 into the sysfs entry.

That's the way I've implemented it, and  I've changed the
name into activity_led.

pradalier@localhost:~$ find /sys/ -name activity_led
/sys/devices/pci0001:10/0001:10:17.0/0.80000000:mac-io/0.0001f000:ata-4/ide0/activity_led

This is the updated version of the patch. I, for one, will
consider it stable now.

signed-off-by: Cedric Pradalier <cedric.pradalier@free.fr>
---
--- drivers/ide/ppc/pmac.c.orig	2006-01-03 13:21:10.000000000 +1000
+++ drivers/ide/ppc/pmac.c	2006-01-24 21:18:36.000000000 +1000
@@ -36,6 +36,11 @@
 #include <linux/pmu.h>
 #include <linux/scatterlist.h>
 
+#ifdef CONFIG_BLK_DEV_IDE_PMAC_BLINK
+#include <linux/device.h>
+#include <asm/of_device.h>
+#endif
+
 #include <asm/prom.h>
 #include <asm/io.h>
 #include <asm/dbdma.h>
@@ -427,6 +432,15 @@ static void pmac_ide_kauai_selectproc(id
 
 #ifdef CONFIG_BLK_DEV_IDE_PMAC_BLINK
 
+MODULE_AUTHOR("Paul Mackerras & Ben. Herrenschmidt");
+MODULE_DESCRIPTION("Support for IDE interfaces on PowerMacs");
+MODULE_LICENSE("GPL");
+
+static int activity_led = 0;
+module_param_named(blink,activity_led, bool, 0666);
+MODULE_PARM_DESC(blink,"Enable/Disable activity led [Default: disabled]");
+
+
 /* Set to 50ms minimum led-on time (also used to limit frequency
  * of requests sent to the PMU
  */
@@ -437,8 +451,7 @@ static spinlock_t pmu_blink_lock;
 static unsigned long pmu_blink_stoptime;
 static int pmu_blink_ledstate;
 static struct timer_list pmu_blink_timer;
-static int pmu_ide_blink_enabled;
-
+static int pmu_ide_blink_enabled = 0;
 
 static void
 pmu_hd_blink_timeout(unsigned long data)
@@ -468,6 +481,8 @@ static void
 pmu_hd_kick_blink(void *data, int rw)
 {
 	unsigned long flags;
+	if (!activity_led)
+		return;
 	
 	pmu_blink_stoptime = jiffies + PMU_HD_BLINK_TIME;
 	wmb();
@@ -483,6 +498,26 @@ pmu_hd_kick_blink(void *data, int rw)
 	spin_unlock_irqrestore(&pmu_blink_lock, flags);
 }
 
+static ssize_t show_activity_led_enable(struct device *dev, struct device_attribute *attr, char *buf)\
+{  
+	return sprintf(buf, "%c\n", activity_led?'1':'0'); 
+}
+
+static ssize_t set_activity_led_enable(struct device *dev, struct device_attribute *attr, 
+		const char *buf, size_t count)
+{
+	int blink=0;
+	if (sscanf (buf, "%d", &blink) != 1)
+		return -EINVAL;
+	activity_led = (blink != 0);
+	printk(KERN_INFO "pmac blinking led initialized (blink %s)\n",
+			activity_led?"enabled":"disabled");
+	return count;
+}
+
+static DEVICE_ATTR (activity_led, S_IRUGO | S_IWUSR, 
+		show_activity_led_enable, set_activity_led_enable);
+
 static int
 pmu_hd_blink_init(void)
 {
@@ -516,6 +551,9 @@ pmu_hd_blink_init(void)
 	init_timer(&pmu_blink_timer);
 	pmu_blink_timer.function = pmu_hd_blink_timeout;
 
+	printk(KERN_INFO "pmac blinking led initialized (blink %s)\n",
+			activity_led?"enabled":"disabled");
+
 	return 1;
 }
 
@@ -1271,7 +1309,7 @@ static int
 pmac_ide_setup_device(pmac_ide_hwif_t *pmif, ide_hwif_t *hwif)
 {
 	struct device_node *np = pmif->node;
-	int *bidp, i;
+	int *bidp;
 
 	pmif->cable_80 = 0;
 	pmif->broken_dma = pmif->broken_dma_warn = 0;
@@ -1401,6 +1439,12 @@ pmac_ide_setup_device(pmac_ide_hwif_t *p
 	/* We probe the hwif now */
 	probe_hwif_init(hwif);
 
+#ifdef CONFIG_BLK_DEV_IDE_PMAC_BLINK
+	/* We wait till here to have the gendev initialized in hwif */
+	device_create_file (&hwif->gendev, &dev_attr_activity_led);
+#endif
+
+
 	return 0;
 }
 

Re: [PATCH 001/001 Updated again] PMAC HD runtime blinking control

[Benjamin Herrenschmidt]

On Tue, 2006-01-24 at 22:25 +1000, Cedric Pradalier wrote:
> According to Benjamin Herrenschmidt, on Tue, 24 Jan 2006
> 10:10:26 +1100, 
> >On Tue, 2006-01-24 at 07:31 +1000, Cedric Pradalier wrote:
> >> >
> >> >Yes. In fact, by enabled default for ATA disks and by disabled for ATAPI
> >> >would make sense...
> >> 
> >> How do I tell the difference?. There is a 'kind' in pmif,
> >> and also a atapi_dma flag in hwif. Which is more sensible?
> >
> >You need to check drive->media ... Which is a bit annoying since it mans
> >it's per drive, not per-HWIF... you may want to move your sysfs entry
> >down one more level :) (Each HWIF has an array of 2 drives, though check
> >drive->present before expecting a useful struct device there)
> >
> 
> Well in this case, I'll let this part waiting for now. 
> I don't think there will be enough real users of this minor
> thing to justify putting any more efforts for now.
> 
> I think the practical solution is to disable the led by
> default. The few user who will want it will be able to add
> either ide_core.blink to their boot parameter or make a
> small init.d script to put 1 into the sysfs entry.
> 
> That's the way I've implemented it, and  I've changed the
> name into activity_led.

I'll have a look into making it work he way I want :) Then I'll submit
it.

Thanks,
Ben.

> pradalier@localhost:~$ find /sys/ -name activity_led
> /sys/devices/pci0001:10/0001:10:17.0/0.80000000:mac-io/0.0001f000:ata-4/ide0/activity_led
> 
> This is the updated version of the patch. I, for one, will
> consider it stable now.
> 
> signed-off-by: Cedric Pradalier <cedric.pradalier@free.fr>
> ---
> --- drivers/ide/ppc/pmac.c.orig	2006-01-03 13:21:10.000000000 +1000
> +++ drivers/ide/ppc/pmac.c	2006-01-24 21:18:36.000000000 +1000
> @@ -36,6 +36,11 @@
>  #include <linux/pmu.h>
>  #include <linux/scatterlist.h>
>  
> +#ifdef CONFIG_BLK_DEV_IDE_PMAC_BLINK
> +#include <linux/device.h>
> +#include <asm/of_device.h>
> +#endif
> +
>  #include <asm/prom.h>
>  #include <asm/io.h>
>  #include <asm/dbdma.h>
> @@ -427,6 +432,15 @@ static void pmac_ide_kauai_selectproc(id
>  
>  #ifdef CONFIG_BLK_DEV_IDE_PMAC_BLINK
>  
> +MODULE_AUTHOR("Paul Mackerras & Ben. Herrenschmidt");
> +MODULE_DESCRIPTION("Support for IDE interfaces on PowerMacs");
> +MODULE_LICENSE("GPL");
> +
> +static int activity_led = 0;
> +module_param_named(blink,activity_led, bool, 0666);
> +MODULE_PARM_DESC(blink,"Enable/Disable activity led [Default: disabled]");
> +
> +
>  /* Set to 50ms minimum led-on time (also used to limit frequency
>   * of requests sent to the PMU
>   */
> @@ -437,8 +451,7 @@ static spinlock_t pmu_blink_lock;
>  static unsigned long pmu_blink_stoptime;
>  static int pmu_blink_ledstate;
>  static struct timer_list pmu_blink_timer;
> -static int pmu_ide_blink_enabled;
> -
> +static int pmu_ide_blink_enabled = 0;
>  
>  static void
>  pmu_hd_blink_timeout(unsigned long data)
> @@ -468,6 +481,8 @@ static void
>  pmu_hd_kick_blink(void *data, int rw)
>  {
>  	unsigned long flags;
> +	if (!activity_led)
> +		return;
>  	
>  	pmu_blink_stoptime = jiffies + PMU_HD_BLINK_TIME;
>  	wmb();
> @@ -483,6 +498,26 @@ pmu_hd_kick_blink(void *data, int rw)
>  	spin_unlock_irqrestore(&pmu_blink_lock, flags);
>  }
>  
> +static ssize_t show_activity_led_enable(struct device *dev, struct device_attribute *attr, char *buf)\
> +{  
> +	return sprintf(buf, "%c\n", activity_led?'1':'0'); 
> +}
> +
> +static ssize_t set_activity_led_enable(struct device *dev, struct device_attribute *attr, 
> +		const char *buf, size_t count)
> +{
> +	int blink=0;
> +	if (sscanf (buf, "%d", &blink) != 1)
> +		return -EINVAL;
> +	activity_led = (blink != 0);
> +	printk(KERN_INFO "pmac blinking led initialized (blink %s)\n",
> +			activity_led?"enabled":"disabled");
> +	return count;
> +}
> +
> +static DEVICE_ATTR (activity_led, S_IRUGO | S_IWUSR, 
> +		show_activity_led_enable, set_activity_led_enable);
> +
>  static int
>  pmu_hd_blink_init(void)
>  {
> @@ -516,6 +551,9 @@ pmu_hd_blink_init(void)
>  	init_timer(&pmu_blink_timer);
>  	pmu_blink_timer.function = pmu_hd_blink_timeout;
>  
> +	printk(KERN_INFO "pmac blinking led initialized (blink %s)\n",
> +			activity_led?"enabled":"disabled");
> +
>  	return 1;
>  }
>  
> @@ -1271,7 +1309,7 @@ static int
>  pmac_ide_setup_device(pmac_ide_hwif_t *pmif, ide_hwif_t *hwif)
>  {
>  	struct device_node *np = pmif->node;
> -	int *bidp, i;
> +	int *bidp;
>  
>  	pmif->cable_80 = 0;
>  	pmif->broken_dma = pmif->broken_dma_warn = 0;
> @@ -1401,6 +1439,12 @@ pmac_ide_setup_device(pmac_ide_hwif_t *p
>  	/* We probe the hwif now */
>  	probe_hwif_init(hwif);
>  
> +#ifdef CONFIG_BLK_DEV_IDE_PMAC_BLINK
> +	/* We wait till here to have the gendev initialized in hwif */
> +	device_create_file (&hwif->gendev, &dev_attr_activity_led);
> +#endif
> +
> +
>  	return 0;
>  }
>  

Re: [PATCH 001/001 Updated again] PMAC HD runtime blinking control

[Andreas Schwab]

Cedric Pradalier <cedric.pradalier@inrialpes.fr> writes:

> @@ -427,6 +432,15 @@ static void pmac_ide_kauai_selectproc(id
>  
>  #ifdef CONFIG_BLK_DEV_IDE_PMAC_BLINK
>  
> +MODULE_AUTHOR("Paul Mackerras & Ben. Herrenschmidt");
> +MODULE_DESCRIPTION("Support for IDE interfaces on PowerMacs");
> +MODULE_LICENSE("GPL");
> +
> +static int activity_led = 0;

No need to zero-initialize a static variable.

> @@ -437,8 +451,7 @@ static spinlock_t pmu_blink_lock;
>  static unsigned long pmu_blink_stoptime;
>  static int pmu_blink_ledstate;
>  static struct timer_list pmu_blink_timer;
> -static int pmu_ide_blink_enabled;
> -
> +static int pmu_ide_blink_enabled = 0;

Same.

Andreas.

-- 
Andreas Schwab, SuSE Labs, schwab@suse.de
SuSE Linux Products GmbH, Maxfeldstraße 5, 90409 Nürnberg, Germany
PGP key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Re: [PATCH 001/001 Updated again] PMAC HD runtime blinking control

[Cedric Pradalier]

According to Andreas Schwab, on Wed, 25 Jan 2006 12:06:44
+0100, 
>Cedric Pradalier <cedric.pradalier@inrialpes.fr> writes:
>
>> @@ -427,6 +432,15 @@ static void pmac_ide_kauai_selectproc(id
>>  
>>  #ifdef CONFIG_BLK_DEV_IDE_PMAC_BLINK
>>  
>> +MODULE_AUTHOR("Paul Mackerras & Ben. Herrenschmidt");
>> +MODULE_DESCRIPTION("Support for IDE interfaces on PowerMacs");
>> +MODULE_LICENSE("GPL");
>> +
>> +static int activity_led = 0;
>
>No need to zero-initialize a static variable.

On the other hand, the cost is minor for an increased
readability and a behavior which does not rely on any 
assumption on the compiler (or even the C specification).

I tend to favor readability first in my coding.

Cheers
--
Cedric

Re: [PATCH 001/001 Updated again] PMAC HD runtime blinking control

[Andreas Schwab]

Cedric Pradalier <cedric.pradalier@inrialpes.fr> writes:

> I tend to favor readability first in my coding.

linux-kernel@ has a different opinion.

Andreas.

-- 
Andreas Schwab, SuSE Labs, schwab@suse.de
SuSE Linux Products GmbH, Maxfeldstraße 5, 90409 Nürnberg, Germany
PGP key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."