Re: sata_inic162x LED enable request

[Tejun Heo <tj@kernel.org>]

Hello,

Bob Stewart wrote:
> I've been using the new driver to boot my second machine for about 2
> weeks, now, as well as having another 200GB disk hanging off it; all
> without any problems.  Well done!  What I would like to see is the
> LED enabled properly, although the board uses a different header pin
> for each drive.  Anyway, if I've
> 
> got it right, here is my suggested change.
> 
> --- linux-2.6.27-rc5.a/drivers/ata/sata_inic162x.c	2008-08-28
> 18:52:02.000000000 -0400
> +++ linux-2.6.27-rc5.b/drivers/ata/sata_inic162x.c	2008-09-01
> 14:05:24.000000000 -0400
> @@ -96,6 +96,7 @@
>  	PORT_SCR		= 0x20,
>  
>  	/* HOST_CTL bits */
> +	HCTL_LEDEN		= (1 << 3),  /* enable LED operation */

Hmm... according to the datasheet, this is SWLED.

  SOFTWARE CONTROL LED ENABLE: When set, software has full control of
  LED activity. When clear, the LED’s are controlled by hardware.

And then there are two more bits, LED0 and LED1 which seem to be the
actual control knob for LEDs.  From the description it looks like the
bit should stay off so tha the controller hardware can drive LEDs.  I
presume this doesn't work as described?

>  	HCTL_IRQOFF		= (1 << 8),  /* global IRQ off */
>  	HCTL_FTHD0		= (1 << 10), /* fifo threshold 0 */
>  	HCTL_FTHD1		= (1 << 11), /* fifo threshold 1*/
> @@ -540,7 +541,7 @@
>  	void __iomem *port_base = inic_port_base(ap);
>  
>  	/* fire up the ADMA engine */
> -	writew(HCTL_FTHD0, port_base + HOST_CTL);
> +	writew(HCTL_FTHD0 + HCTL_LEDEN, port_base + HOST_CTL);

Nitpick: for flags, it's customary to use | instead of +.

Thanks.

-- 
tejun