Hi,

On Tuesday 09 of October 2012 17:18:52 Vasanth Ananthan wrote:

Please use lowercase for variable names.

+	if ((readl(base + reg) & checkbit) == Status)
+		return true;
+	else
+		return false;
+}
+
+static bool wait_for_reg_status(void __iomem *base, u32 reg, u32
checkbit, +				u32 Status)

Same here.

+{
+	u16 time_limit_cnt = 0;
+	while (!sata_is_reg(base, reg, checkbit, Status)) {
+		if (time_limit_cnt == SATA_TIME_LIMIT)
+			return false;
+		udelay(1000);
+		time_limit_cnt++;
+	}
+	return true;
+}
+
+int sataphy_init(struct sata_phy *phy)

static int sataphy_init(struct sata_phy *phy) ?

+{
+	int ret;
+	u32 val;
+
+	/* Values to be written to enable 40 bits interface */
+	u8 buf[] = { 0x3A, 0x0B };
+
+	struct exynos_sata_phy *sata_phy;
+
+	sata_phy = (struct exynos_sata_phy *)phy->priv_data;
+
+	clk_enable(sata_phy->clk);
+
+	writel(S5P_PMU_SATA_PHY_CONTROL_EN, EXYNOS5_SATA_PHY_CONTROL);
+
+	val = 0;
+	writel(val, sata_phy->mmio + EXYNOS5_SATA_RESET);
+
+	val = readl(sata_phy->mmio + EXYNOS5_SATA_RESET);
+	val |= 0xFF;
+	writel(val, sata_phy->mmio + EXYNOS5_SATA_RESET);
+
+	val = readl(sata_phy->mmio + EXYNOS5_SATA_RESET);
+	val |= LINK_RESET;
+	writel(val, sata_phy->mmio + EXYNOS5_SATA_RESET);
+
+	val = readl(sata_phy->mmio + EXYNOS5_SATA_RESET);
+	val |= RESET_CMN_RST_N;
+	writel(val, sata_phy->mmio + EXYNOS5_SATA_RESET);
+
+	val = readl(sata_phy->mmio + EXYNOS5_SATA_PHSATA_CTRLM);
+	val &= ~PHCTRLM_REF_RATE;
+	writel(val, sata_phy->mmio + EXYNOS5_SATA_PHSATA_CTRLM);
+
+	/* High speed enable for Gen3 */
+	val = readl(sata_phy->mmio + EXYNOS5_SATA_PHSATA_CTRLM);
+	val |= PHCTRLM_HIGH_SPEED;
+	writel(val, sata_phy->mmio + EXYNOS5_SATA_PHSATA_CTRLM);
+
+	val = readl(sata_phy->mmio + EXYNOS5_SATA_CTRL0);
+	val |= CTRL0_P0_PHY_CALIBRATED_SEL | CTRL0_P0_PHY_CALIBRATED;
+	writel(val, sata_phy->mmio + EXYNOS5_SATA_CTRL0);
+
+	writel(SATA_PHY_GENERATION3, sata_phy->mmio + EXYNOS5_SATA_MODE0);
+
+	ret = i2c_master_send(i2c_client, buf, sizeof(buf));
+	if (ret < 0)
+		return -EINVAL;
+
+	/* release cmu reset */
+	val = readl(sata_phy->mmio + EXYNOS5_SATA_RESET);
+	val &= ~RESET_CMN_RST_N;
+	writel(val, sata_phy->mmio + EXYNOS5_SATA_RESET);
+
+	val = readl(sata_phy->mmio + EXYNOS5_SATA_RESET);
+	val |= RESET_CMN_RST_N;
+	writel(val, sata_phy->mmio + EXYNOS5_SATA_RESET);
+
+	if (wait_for_reg_status(sata_phy->mmio, EXYNOS5_SATA_PHSATA_STATM,
+				PHSTATM_PLL_LOCKED, 1)) {
+		return 0;
+	}
+	return -EINVAL;
+}
+
+int sataphy_shutdown(struct sata_phy *phy)

static int sataphy_shutdown(struct sata_phy *phy) ?

+{
+
+	struct exynos_sata_phy *sata_phy;
+
+	sata_phy = (struct exynos_sata_phy *)phy->priv_data;
+
+	clk_disable(sata_phy->clk);
+
+	return 0;
+}
+
+static int sata_i2c_probe(struct i2c_client *client,
+			  const struct i2c_device_id *i2c_id)
+{
+	i2c_client = client;
+	return 0;
+}
+
+static int __init sata_phy_probe(struct platform_device *pdev)
+{
+	struct exynos_sata_phy *sataphy;
+	struct sata_phy *phy;
+	struct resource *res;
+	int ret = 0;
+
+	phy = kzalloc(sizeof(struct sata_phy), GFP_KERNEL);

devm_

+	if (!phy) {
+		dev_err(&pdev->dev, "failed to allocate memory\n");
+		ret = -ENOMEM;
+		goto out;
+	}
+
+	sataphy = kzalloc(sizeof(struct exynos_sata_phy), GFP_KERNEL);

devm_

+	if (!sataphy) {
+		dev_err(&pdev->dev, "failed to allocate memory\n");
+		ret = -ENOMEM;
+		goto err0;
+	}
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	if (!res) {
+		dev_err(&pdev->dev, "Could not find IO resource\n");
+		ret = -EINVAL;
+		goto err1;
+	}
+
+	sataphy->mem = request_mem_region(res->start, resource_size(res),
+								pdev->name);

devm_

+	if (!sataphy->mem) {
+		dev_err(&pdev->dev, "Could not request IO resource\n");
+		ret = -EINVAL;
+		goto err1;
+	}
+
+	sataphy->mmio =
+	    ioremap(res->start, resource_size(res));

devm_

+

nitpick: Unnecessary blank line.

+	if (!sataphy->mmio) {
+		dev_err(&pdev->dev, "failed to remap IO\n");
+		ret = -ENOMEM;
+		goto err2;
+	}
+
+	sataphy->clk = clk_get(&pdev->dev, "sata_phy");

devm_

+	if (IS_ERR(sataphy->clk)) {
+		dev_err(&pdev->dev, "failed to get clk for PHY\n");
+		ret = PTR_ERR(sataphy->clk);
+		sataphy->clk = NULL;
+		goto err3;
+	}
+
+	phy->init = sataphy_init;
+	phy->shutdown = sataphy_shutdown;
+	phy->priv_data = (void *)sataphy;
+	phy->dev = &pdev->dev;
+
+	ret = sata_add_phy(phy, SATA_PHY_GENERATION3);
+	if (ret < 0)
+		goto err4;

Do you have any warranties that phy callbacks won't get called before i2c 
device probes and sets i2c_client?

+
+	ret = i2c_add_driver(&sataphy_i2c_driver);
+	if (ret < 0)
+		goto err5;
+
+	platform_set_drvdata(pdev, phy);
+
+	return ret;
+
+ err5:
+	sata_remove_phy(phy);
+
+ err4:
+	clk_put(sataphy->clk);
+
+ err3:
+	iounmap(sataphy->mmio);
+
+ err2:
+	release_resource(sataphy->mem);
+
+ err1:
+	kfree(sataphy);
+
+ err0:
+	kfree(phy);
+
+ out:
+	return ret;
+}
+
+static int sata_phy_remove(struct platform_device *pdev)
+{
+	struct sata_phy *phy;
+	struct exynos_sata_phy *sataphy;
+
+	phy = platform_get_drvdata(pdev);
+
+	sataphy = (struct exynos_sata_phy *)phy->priv_data;
+	sata_remove_phy(phy);
+
+	iounmap(sataphy->mmio);
+
+	release_resource(sataphy->mem);
+
+	clk_disable(sataphy->clk);
+	clk_put(sataphy->clk);
+
+	kfree(sataphy);
+	kfree(phy);
+
+	return 0;
+}
+
+static const struct of_device_id sata_phy_of_match[] = {
+	{.compatible = "samsung,exynos-sata-phy",},

nitpick: Space after opening and before closing brackets.

+	{},
+};
+
+MODULE_DEVICE_TABLE(of, sata_phy_of_match);
+
+static const struct i2c_device_id phy_i2c_device_match[] = {
+	{"sataphy", 0},

Same here.

+};
+
+MODULE_DEVICE_TABLE(of, phy_i2c_device_match);
+
+static struct platform_driver sata_phy_driver = {
+	.probe = sata_phy_probe,
+	.remove = sata_phy_remove,
+	.driver = {
+		   .name = "sata-phy",
+		   .owner = THIS_MODULE,
+		   .of_match_table = sata_phy_of_match,
+		   },

nitpick: Wrong indentation of closing bracket.

+};
+
+static struct i2c_driver sataphy_i2c_driver = {
+	.driver = {
+		   .name = "i2c-phy",
+		   .owner = THIS_MODULE,
+		   .of_match_table = phy_i2c_device_match,
+		   },

nitpick: Wrong indentation of closing bracket.

Best regards,
Tomasz Figa