2015-05-15 11:43 GMT+02:00 Yong Wu [off-list ref]:

Maybe add something like:
MTK_CLK_FIRST = MTK_CLK_APB,
to make the for loops better readable.

+};
+
+struct mtk_smi_common {
+       void __iomem            *base;

That seems to be never used. Please delete it.

+       struct clk              *clk[MTK_CLK_MAX];
+};
+
+struct mtk_smi_larb {
+       void __iomem            *base;
+       spinlock_t              portlock; /* lock for config port */
+       struct clk              *clk[MTK_CLK_MAX];
+       struct device           *smi;
+};
+
+static const char * const mtk_smi_clk_name[MTK_CLK_MAX] = {
+       "apb", "smi"
+};
+
+static int mtk_smi_common_get(struct device *smidev)
+{
+       struct mtk_smi_common *smipriv = dev_get_drvdata(smidev);
+       int i, ret;
+
+       for (i = MTK_CLK_APB; i < MTK_CLK_MAX; i++) {
+               ret = clk_enable(smipriv->clk[i]);
+               if (ret) {
+                       dev_err(smidev,
+                               "Failed to enable the clock of smi_common\n");
+                       goto err_smi_clk;
+               }
+       }
+       return ret;
+
+err_smi_clk:
+       if (i == MTK_CLK_SMI)
+               clk_disable(smipriv->clk[MTK_CLK_APB]);
+       return ret;
+}
+
+static void mtk_smi_common_put(struct device *smidev)
+{
+       struct mtk_smi_common *smipriv = dev_get_drvdata(smidev);
+       int i;
+
+       for (i = MTK_CLK_SMI; i >= MTK_CLK_APB; i--)
+               clk_disable(smipriv->clk[i]);
+}
+
+int mtk_smi_larb_get(struct device *larbdev)
+{
+       struct mtk_smi_larb *larbpriv = dev_get_drvdata(larbdev);
+       int i, ret = 0;
+
+       ret = mtk_smi_common_get(larbpriv->smi);
+       if (ret)
+               return ret;
+
+       for (i = MTK_CLK_APB; i < MTK_CLK_MAX; i++) {
+               ret = clk_enable(larbpriv->clk[i]);
+               if (ret) {
+                       dev_err(larbdev,
+                               "Failed to enable larb clock%d. ret 0x%x\n",
+                               i, ret);
+                       goto err_larb_clk;
+               }
+       }
+
+       return ret;
+
+err_larb_clk:
+       if (i == MTK_CLK_SMI)
+               clk_disable(larbpriv->clk[MTK_CLK_APB]);
+       mtk_smi_common_put(larbpriv->smi);
+       return ret;
+}
+
+void mtk_smi_larb_put(struct device *larbdev)

You can pass mtk_smi_larb as parameter instead of struct device

+{
+       struct mtk_smi_larb *larbpriv = dev_get_drvdata(larbdev);
+       int i;
+
+       for (i = MTK_CLK_SMI; i >= MTK_CLK_APB; i--)
+               clk_disable(larbpriv->clk[i]);
+
+       mtk_smi_common_put(larbpriv->smi);
+}
+
+int mtk_smi_config_port(struct device *larbdev,        unsigned int larbportid,
+                       bool iommuen)
+{
+       struct mtk_smi_larb *larbpriv = dev_get_drvdata(larbdev);
+       unsigned long flags;
+       int ret;
+       u32 reg;
+
+       ret = mtk_smi_larb_get(larbdev);
+       if (ret)
+               return ret;
+
+       spin_lock_irqsave(&larbpriv->portlock, flags);
+       reg = readl(larbpriv->base + SMI_LARB_MMU_EN);
+       reg &= ~F_SMI_MMU_EN(larbportid);
+       if (iommuen)
+               reg |= F_SMI_MMU_EN(larbportid);
+       writel(reg, larbpriv->base + SMI_LARB_MMU_EN);
+       spin_unlock_irqrestore(&larbpriv->portlock, flags);
+
+       mtk_smi_larb_put(larbdev);
+
+       return 0;
+}
+
+static int mtk_smi_larb_probe(struct platform_device *pdev)
+{
+       struct mtk_smi_larb *larbpriv;
+       struct resource *res;
+       struct device *dev = &pdev->dev;
+       struct device_node *smi_node;
+       struct platform_device *smi_pdev;
+       int i, ret;
+
+       larbpriv = devm_kzalloc(dev, sizeof(struct mtk_smi_larb), GFP_KERNEL);
+       if (!larbpriv)
+               return -ENOMEM;
+
+       res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+       larbpriv->base = devm_ioremap_resource(dev, res);
+       if (IS_ERR(larbpriv->base)) {
+               dev_err(dev, "Failed to get the larbbase (0x%lx)\n",
+                       PTR_ERR(larbpriv->base));
+               return PTR_ERR(larbpriv->base);
+       }
+
+       for (i = MTK_CLK_APB; i < MTK_CLK_MAX; i++) {
+               larbpriv->clk[i] = devm_clk_get(dev, mtk_smi_clk_name[i]);
+
+               if (IS_ERR(larbpriv->clk[i])) {

Please delete the blank line before the if.

+                       ret = PTR_ERR(larbpriv->clk[i]);
+                       dev_err(dev, "Failed to get larb%d clock (0x%x)\n",
+                               i, ret);
+                       goto fail_larb_clk;
+               } else {
+                       ret = clk_prepare(larbpriv->clk[i]);
+                       if (ret) {
+                               dev_err(dev, "Failed to prepare larb clock%d (0x%x)\n",
+                                       i, ret);
+                               goto fail_larb_clk;
+                       }
+               }
+       }
+
+       smi_node = of_parse_phandle(dev->of_node, "smi", 0);
+       if (!smi_node) {
+               dev_err(dev, "Failed to get smi node\n");
+               ret = -EINVAL;
+               goto fail_larb_clk;
+       }
+
+       smi_pdev = of_find_device_by_node(smi_node);
+       of_node_put(smi_node);
+       if (smi_pdev) {
+               larbpriv->smi = &smi_pdev->dev;
+       } else {
+               dev_err(dev, "Failed to get the smi_common device\n");
+               ret = -EINVAL;
+               goto fail_larb_clk;
+       }
+
+       spin_lock_init(&larbpriv->portlock);
+       dev_set_drvdata(dev, larbpriv);
+       return 0;
+
+fail_larb_clk:
+       while (--i >= MTK_CLK_APB)
+               clk_unprepare(larbpriv->clk[i]);
+       return ret;
+}
+
+static const struct of_device_id mtk_smi_larb_of_ids[] = {
+       { .compatible = "mediatek,mt8173-smi-larb",
+       },
+       {}
+};
+
+static struct platform_driver mtk_smi_larb_driver = {
+       .probe  = mtk_smi_larb_probe,
+       .driver = {
+               .name = "mtksmilarb",

Huh, what about something more readable like mtk-smi-larb?

+               .of_match_table = mtk_smi_larb_of_ids,
+       }
+};
+
+static int mtk_smi_probe(struct platform_device *pdev)
+{
+       struct device *dev = &pdev->dev;
+       struct mtk_smi_common *smipriv;
+       int ret, i;
+
+       smipriv = devm_kzalloc(dev, sizeof(*smipriv), GFP_KERNEL);
+       if (!smipriv)
+               return -ENOMEM;
+
+       for (i = MTK_CLK_APB; i < MTK_CLK_MAX; i++) {
+               smipriv->clk[i] = devm_clk_get(dev, mtk_smi_clk_name[i]);
+
+               if (IS_ERR(smipriv->clk[i])) {
+                       ret = PTR_ERR(smipriv->clk[i]);
+                       dev_err(dev, "Failed to get smi-%s clock (0x%x)\n",
+                               mtk_smi_clk_name[i], ret);
+                       goto fail_smi_clk;
+               } else {
+                       ret = clk_prepare(smipriv->clk[i]);
+                       if (ret) {
+                               dev_err(dev, "Failed to prepare smi%d clock 0x%x\n",
+                                       i, ret);
+                               goto fail_smi_clk;
+                       }
+               }
+       }
+
+       dev_set_drvdata(dev, smipriv);
+       return ret;
+
+fail_smi_clk:
+       if (i == MTK_CLK_SMI)
+               clk_unprepare(smipriv->clk[MTK_CLK_APB]);
+       return ret;
+}
+
+static const struct of_device_id mtk_smi_of_ids[] = {
+       { .compatible = "mediatek,mt8173-smi",
+       },
+       {}
+};
+
+static struct platform_driver mtk_smi_driver = {
+       .probe  = mtk_smi_probe,
+       .driver = {
+               .name = "mtksmi",

Same here.

Thanks,
Matthias