On 08/23/2012 06:31 PM, Shane Huang wrote:
This patch enables Aggressive Device Sleep only if both host controller
and device support it.
[...]
@@ -2323,6 +2324,26 @@ int ata_dev_configure(struct ata_device *dev)
}
}
+ if (ata_id_has_devslp(dev->id)) {
+ err_mask = ata_dev_set_feature(dev,
+ SETFEATURES_SATA_ENABLE,
+ SATA_DEVSLP);
+ if (err_mask)
+ ata_dev_err(dev,
+ "failed to enable DEVSLP (err_mask=0x%x)\n",
+ err_mask);
+ else {
+ dev->flags |= ATA_DFLAG_DEVSLP;
+ err_mask = ata_read_log_page(dev,
+ ATA_LOG_SATA_SETTINGS,
+ dev->sata_settings,
+ 1);
+ if (err_mask)
+ ata_dev_warn(dev,
+ "failed to get Identify Device Data\n");
+ }
+ }
+
dev->cdb_len = 16;
}
1) Contra the patch description (quoted above), you are enabling
SATA_DEVSLP on the device regardless of power policy or host controller
support.
I'm not sure about the ata_dev_configure() change. If the power policy
is not enabling aggressive sleep, we should not send this command to the
device.
2) If we are going to unconditionally add ATA_SECT_SIZE bytes to every
ata_device structure, let's at least move the ata_read_log_page() call
outside of the devslp test, so that others may have this information
even if the device does not support devslp.
3) please define constants in linux/ata.h for sata_settings information,
rather than using hexidecimal constants ("magic numbers").
4) is it wise to issue SET_FEATURES / SATA_DEVSLP prior to programming
the host controller? that order seems wrong.
Jeff
--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
