On 06/17/2014 06:14 AM, Bartlomiej Zolnierkiewicz wrote:

Hi,

On Wednesday, June 04, 2014 02:32:38 PM Mikko Perttunen wrote:

quoted

This adds support for the integrated AHCI-compliant Serial ATA
controller present on the NVIDIA Tegra124 system-on-chip.

quoted

+static inline void sata_writel(struct tegra_ahci_priv *tegra, u32 value,
+				 unsigned long offset)
+{
+	writel(value, tegra->sata_regs + offset);
+}
+
+static inline u32 sata_readl(struct tegra_ahci_priv *tegra,
+				unsigned long offset)
+{
+	return readl(tegra->sata_regs + offset);
+}

sata_writel() and sata_read() static inlines don't seem to add any value.

Can they be removed?

Such functions are quite idiomatic in drivers, and serve to simplify all
the call-sites by removing the need to write out the addition of the
base address everywhere.

quoted

+static struct platform_driver tegra_ahci_driver = {
+	.probe = tegra_ahci_probe,
+	.remove = ata_platform_remove_one,
+	.driver = {
+		.name = "tegra-ahci",
+		.owner = THIS_MODULE,
+		.of_match_table = tegra_ahci_of_match,
+	},

This driver lacks PM suspend/resume support.  I assume that
the Tegra124 platform also doesn't support suspend/resume yet
(if so a comment in the driver code about this would be useful),
otherwise the driver should be fixed.

We do have basic system suspend/resume support. However, I don't think
missing suspend/resume in an individual driver should stop it from being
merged. It can certainly be added later.