On Tue, 12 Jul 2016, Moritz Fischer wrote:
> Hi Alan,
>
> couple of nits inline below.
>
Hi Moritz!
Thanks for your review!
> On Tue, Jul 12, 2016 at 12:07 PM, Alan Tull <atull@xxxxxxxxxxxxxxxxxxxxx> wrote:
>
> > +static int socfpga_a10_fpga_write_complete(struct fpga_manager *mgr, u32 flags)
> > +{
> > + struct a10_fpga_priv *priv = mgr->priv;
> > + u32 reg;
> > + int ret;
> > +
> > + /* Wait for pr_done */
> > + ret = socfpga_a10_fpga_wait_for_pr_done(priv);
> > +
> > + /* Clear pr_request */
> > + regmap_update_bits(priv->regmap, A10_FPGAMGR_IMGCFG_CTL_01_OFST,
> > + A10_FPGAMGR_IMGCFG_CTL_01_S2F_PR_REQUEST, 0);
> > +
> > + /* Send some clocks to clear out any errors */
> > + socfpga_a10_fpga_generate_dclks(priv, 256);
> > +
> > + /* Disable s2f dclk and data */
> > + regmap_update_bits(priv->regmap, A10_FPGAMGR_IMGCFG_CTL_02_OFST,
> > + A10_FPGAMGR_IMGCFG_CTL_02_EN_CFG_CTRL, 0);
>
> Maybe replace 0 with named constant.
Generally I use named constants, but since regmap_updates_bits uses a
mask, it is clear that we're clearing the
A10_FPGAMGR_IMGCFG_CTL_02_EN_CFG_CTRL bit.
> > +static int socfpga_a10_fpga_probe(struct platform_device *pdev)
> > +{
> > + struct device *dev = &pdev->dev;
> > + struct a10_fpga_priv *priv;
> > + void __iomem *reg_base;
> > + struct resource *res;
> > + int ret;
> > +
> > + priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> > + if (!priv)
> > + return -ENOMEM;
> > +
> > + /* First mmio base is for register access */
> > + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > + reg_base = devm_ioremap_resource(dev, res);
> > + if (IS_ERR(reg_base))
> > + return PTR_ERR(reg_base);
> > +
> > + /* Second mmio base is for writing FPGA image data */
> > + res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
> > + priv->fpga_data_addr = devm_ioremap_resource(dev, res);
> > + if (IS_ERR(priv->fpga_data_addr))
> > + return PTR_ERR(priv->fpga_data_addr);
> > +
> > + /* regmap for register access */
> > + priv->regmap = devm_regmap_init_mmio(dev, reg_base,
> > + &socfpga_a10_fpga_regmap_config);
> > + if (IS_ERR(priv->regmap))
> > + return -ENODEV;
> > +
> > + priv->clk = devm_clk_get(dev, NULL);
> > + if (IS_ERR(priv->clk)) {
> > + dev_err(dev, "no clock specified\n");
> > + return PTR_ERR(priv->clk);
> > + }
> > +
> > + ret = clk_prepare_enable(priv->clk);
> > + if (ret) {
> > + dev_err(dev, "could not enable clock\n");
> > + clk_put(priv->clk);
>
> Seen that you used devm_clk_get() is this one necessary?
Yes, this is wrong. I'll fix it.
Thanks,
Alan
>
> > +static int socfpga_a10_fpga_remove(struct platform_device *pdev)
> > +{
> > + struct fpga_manager *mgr = platform_get_drvdata(pdev);
> > + struct a10_fpga_priv *priv = mgr->priv;
> > +
> > + fpga_mgr_unregister(&pdev->dev);
> > + clk_disable_unprepare(priv->clk);
> > + clk_put(priv->clk);
>
> Same here, if needed at all shouldn't it be devm_clk_put() ?
>
> Cheers,
>
> Moritz
>
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
