[PATCH 1/3] ahci: exynos: add ahci sata support on Exynos platform

On Tuesday 01 October 2013 12:03 PM, Yuvaraj Kumar C D wrote:

Can we select GENERIC_PHY here?

                                    ^^
                             spurious space here

phy_get should be called from probe.

+	if (IS_ERR(priv->phy))
+		return PTR_ERR(priv->phy);
+
+	ret = phy_init(priv->phy);
+	if (ret < 0) {
+			dev_err(dev, "failed to init SATA PHY\n");

single tab is sufficient here and below.

+			return ret;
+	}
+
+	ret = clk_prepare_enable(priv->sclk);
+	if (ret < 0) {
+		dev_err(dev, "failed to enable source clk\n");
+		return ret;
+	}
+
+	ret = clk_set_rate(priv->sclk, priv->freq * MHZ);
+	if (ret < 0) {
+		dev_err(dev, "failed to set clk frequency\n");
+		clk_disable_unprepare(priv->sclk);
+		return ret;
+	}
+
+	return 0;
+}
+
+static void exynos_sata_exit(struct device *dev)
+{
+	struct exynos_ahci_priv *priv = dev_get_drvdata(dev->parent);
+	if (!IS_ERR(priv->sclk))

This error check is not needed. You fail probe if you dont get clock anyways.

+		clk_disable_unprepare(priv->sclk);
+}
+
+static int exynos_sata_suspend(struct device *dev)
+{
+	struct exynos_ahci_priv *priv = dev_get_drvdata(dev->parent);
+
+	if (!IS_ERR(priv->sclk))

ditto..

+		clk_disable_unprepare(priv->sclk);
+	phy_power_off(priv->phy);
+	return 0;
+}
+
+static int exynos_sata_resume(struct device *dev)
+{
+	struct exynos_ahci_priv *priv = dev_get_drvdata(dev->parent);
+	phy_power_on(priv->phy);
+	exynos_sata_init(dev, NULL);
+	return 0;
+}
+
+static struct ahci_platform_data exynos_sata_pdata = {
+	.init = exynos_sata_init,

Why is exynos_sata_init given as callback and also in exynos_sata_resume?

+	.exit = exynos_sata_exit,
+	.suspend = exynos_sata_suspend,
+	.resume = exynos_sata_resume,

Can't we use this in runtime_suspend/runtime_resume or suspend/resume pm ops?

+};
+
+static const struct of_device_id exynos_ahci_of_match[] = {
+	{ .compatible = "samsung,exynos5250-sata-ahci",
+		.data = &exynos_sata_pdata},
+	{},
+};
+MODULE_DEVICE_TABLE(of, exynos_ahci_of_match);
+
+static int exynos_sata_parse_dt(struct device_node *np,
+				struct exynos_ahci_priv *priv)
+{
+	if (!np)
+		return -EINVAL;
+	return of_property_read_u32(np, "samsung,sata-freq",
+						&priv->freq);
+}
+
+static int exynos_ahci_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct resource *mem, *irq, res[2];
+	const struct of_device_id *of_id;
+	const struct ahci_platform_data *pdata = NULL;
+	struct exynos_ahci_priv *priv;
+	struct device *ahci_dev;
+	struct platform_device *ahci_pdev;
+	int ret;
+
+	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
+	if (!priv) {
+		dev_err(dev, "can't alloc ahci_host_priv\n");

error message not needed. kzalloc will give a dump if it fails.

+		return -ENOMEM;
+	}
+
+	ahci_pdev = platform_device_alloc("ahci", -1);

Should be using PLATFORM_DEVID_AUTO unless it is absolutely necessary.

+	if (!ahci_pdev)
+		return -ENODEV;
+
+	ahci_dev = &ahci_pdev->dev;
+	ahci_dev->parent = dev;
+
+	ret = exynos_sata_parse_dt(dev->of_node, priv);
+	if (ret < 0) {
+			dev_err(dev, "failed to parse device tree\n");
+			goto err_out;
+	}
+
+	priv->sclk = devm_clk_get(dev, "sclk_sata");
+	if (IS_ERR(priv->sclk)) {
+		dev_err(dev, "failed to get sclk_sata\n");
+		ret = PTR_ERR(priv->sclk);
+		goto err_out;
+	}
+	priv->ahci_pdev = ahci_pdev;
+	platform_set_drvdata(pdev, priv);
+
+	of_id = of_match_device(exynos_ahci_of_match, dev);
+	if (of_id) {
+		pdata = of_id->data;
+	} else {
+		ret = -EINVAL;
+		goto err_out;
+	}
+
+	mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	irq = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
+	if (!mem || !irq) {
+		dev_err(dev, "no mmio/irq resource\n");
+		ret = -ENOMEM;
+		goto err_out;
+	}
+
+	res[0] = *mem;
+	res[1] = *irq;
+
+	ahci_dev->coherent_dma_mask = DMA_BIT_MASK(32);
+	ahci_dev->dma_mask = &ahci_dev->coherent_dma_mask;
+	ahci_dev->of_node = dev->of_node;
+
+	ret = platform_device_add_resources(ahci_pdev, res, 2);
+	if (ret)
+		goto err_out;
+
+	ret = platform_device_add_data(ahci_pdev, pdata, sizeof(*pdata));
+	if (ret)
+		goto err_out;
+
+	ret = platform_device_add(ahci_pdev);
+	if (ret) {
+
+
+err_out:
+		platform_set_drvdata(pdev, NULL);
+		platform_device_put(ahci_pdev);
+		return ret;

Move this to the end of this function and then goto err_out for better readability.

+	}
+
+	return 0;
+}
+
+static int exynos_ahci_remove(struct platform_device *pdev)
+{
+	struct exynos_ahci_priv *priv = platform_get_drvdata(pdev);
+	struct platform_device *ahci_pdev = priv->ahci_pdev;
+
+	platform_device_unregister(ahci_pdev);
+	platform_set_drvdata(pdev, NULL);
+
+	return 0;
+}
+
+static struct platform_driver exynos_ahci_driver = {
+	.probe = exynos_ahci_probe,
+	.remove = exynos_ahci_remove,
+	.driver = {
+		.name = "sata-exynos",
+		.owner = THIS_MODULE,
+		.of_match_table = exynos_ahci_of_match,
+	},
+};
+module_platform_driver(exynos_ahci_driver);
+
+MODULE_DESCRIPTION("Samsung Exynos AHCI SATA platform driver");
+MODULE_AUTHOR("Yuvaraj C D [off-list ref]");
+MODULE_LICENSE("GPL");

GPL v2?

Thanks
Kishon