Desai,
Some nits about comments. And suggestions that you can chose to ignore. :)
On Mon, Apr 20, 2009 at 3:43 AM, Kashyap, Desai <kashyap.desai@xxxxxxx> wrote:
...
> diff --git a/drivers/message/fusion/mptsas.h b/drivers/message/fusion/mptsas.h
> index 9b47379..36578df 100644
> --- a/drivers/message/fusion/mptsas.h
> +++ b/drivers/message/fusion/mptsas.h
> @@ -83,6 +83,12 @@ struct sas_device_info {
> u16 slot; /* enclosure slot id */
> u64 enclosure_logical_id; /*enclosure address */
> u8 is_logical_volume; /* is this logical volume */
"is this logical volume" comment is just repeating the variable name. :)
Maybe point at a description (e.g. "See page XX of Programmers Guide")
or omit this comment.
> + /* this belongs to volume */
> + u8 is_hidden_raid_component;
Does "belongs to a volume" mean a "logical volume"?
Suggestion: replace with something like /* is physical dev part of volume? */
> + /* this valid when is_hidden_raid_component set */
> + u8 volume_id;
When looking at code, I would have associated volume_id
with "is_logical_volume" variable. Maybe call this "hidden_raid_VID" ?
> + /* cached data for a removed device */
> + u8 is_cached;
"cached data" generally refers to user payload data.
I'm pretty sure that's not meant here.
Suggestion: /* Device info is cached (or not) */
It's interesting to know this if the device has been removed.
But the act of caching has to take place before the device is
gone and thus, this flag will tell us the data was
read once "before" and may not be current.
Some devices will change the SCSI model string after
they've spun up and loaded the full firmware from reserved
tracks on the media.
> };
>
> struct mptsas_hotplug_event {
> --
> To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
> the body of a message to majordomo@xxxxxxxxxxxxxxx
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
