Message-ID: <4751EE36.6050506@garzik.org>
Date: Sat, 01 Dec 2007 18:28:54 -0500
From: Jeff Garzik <jeff@garzik.org>
User-Agent: Thunderbird 2.0.0.9 (X11/20071115)
MIME-Version: 1.0
To: kristen.c.accardi@intel.com
CC: akpm@linux-foundation.org, linux-ide@vger.kernel.org,
       linux-kernel@vger.kernel.org
Subject: Re: [patch] ata: ahci: Enclosure Management via LED rev2
References: <20071129121925.7d178915@appleyard> <20071130163443.2feabc5d@appleyard>
In-Reply-To: <20071130163443.2feabc5d@appleyard>
Content-Type: text/plain; charset=US-ASCII; format=flowed
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 1266
Lines: 32

Kristen Carlson Accardi wrote:
> Enclosure Management via LED
> 
> This patch implements Enclosure Management via the LED protocol as specified
> in AHCI specification.
> 
> Signed-off-by: Kristen Carlson Accardi <kristen.c.accardi@intel.com>
> ---
> This revision makes the change to the comment requested by Mark Lord,
> fixes some bugs in the bit shifting for writing the new led state,
> and implements a show function so that led status can be read as
> well as written.

Overall looks pretty good, from a technical review perspective.

Two worries:

1) exporting ata_scsi_find_dev(), and assuming a scsi device is 
attached.  the latter can be fixed by a !NULL check (and should be), but 
its a bit of a layering violation since long term we want to make the 
SCSI simulator optional for all ATA devices.

2) vaguely related to #1, I'm not so sure the attributes should be 
implemented directly in ahci.  if this __or something like it__ appears 
on non-Intel hardware, the code should be somewhere more generic.


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/