[...] > +static int meson_mmc_clk_set(struct meson_host *host, unsigned long clk_rate) > +{ > + struct mmc_host *mmc = host->mmc; > + int ret = 0; > + u32 cfg; > + > + if (clk_rate) { > + if (WARN_ON(clk_rate > mmc->f_max)) > + clk_rate = mmc->f_max; > + else if (WARN_ON(clk_rate < mmc->f_min)) > + clk_rate = mmc->f_min; > + } > + > + if (clk_rate == mmc->actual_clock) > + return 0; > + > + /* stop clock */ > + cfg = readl(host->regs + SD_EMMC_CFG); > + if (!(cfg & CFG_STOP_CLOCK)) { > + cfg |= CFG_STOP_CLOCK; > + writel(cfg, host->regs + SD_EMMC_CFG); > + } > + > + dev_dbg(host->dev, "change clock rate %u -> %lu\n", > + mmc->actual_clock, clk_rate); > + ret = clk_set_rate(host->cfg_div_clk, clk_rate); Shouldn't you check the error before proceeding? > + if (clk_rate && clk_rate != clk_get_rate(host->cfg_div_clk)) > + dev_warn(host->dev, "divider requested rate %lu != actual rate %lu: ret=%d\n", > + clk_rate, clk_get_rate(host->cfg_div_clk), ret); > + else > + mmc->actual_clock = clk_rate; > + > + /* (re)start clock, if non-zero */ > + if (clk_rate) { > + cfg = readl(host->regs + SD_EMMC_CFG); > + cfg &= ~CFG_STOP_CLOCK; > + writel(cfg, host->regs + SD_EMMC_CFG); > + } > + > + return ret; > +} > + > +/* > + * The SD/eMMC IP block has an internal mux and divider used for > + * generating the MMC clock. Use the clock framework to create and > + * manage these clocks. > + */ > +static int meson_mmc_clk_init(struct meson_host *host) > +{ > + struct clk_init_data init; > + char clk_name[32]; > + int i, ret = 0; > + const char *mux_parent_names[MUX_CLK_NUM_PARENTS]; > + unsigned int mux_parent_count = 0; > + const char *clk_div_parents[1]; > + unsigned int f_min = UINT_MAX; > + u32 clk_reg, cfg; > + > + /* get the mux parents */ > + for (i = 0; i < MUX_CLK_NUM_PARENTS; i++) { > + char name[16]; > + > + snprintf(name, sizeof(name), "clkin%d", i); > + host->mux_parent[i] = devm_clk_get(host->dev, name); > + if (IS_ERR(host->mux_parent[i])) { > + ret = PTR_ERR(host->mux_parent[i]); > + if (PTR_ERR(host->mux_parent[i]) != -EPROBE_DEFER) > + dev_err(host->dev, "Missing clock %s\n", name); > + host->mux_parent[i] = NULL; > + return ret; > + } > + > + host->mux_parent_rate[i] = clk_get_rate(host->mux_parent[i]); > + mux_parent_names[i] = __clk_get_name(host->mux_parent[i]); > + mux_parent_count++; > + if (host->mux_parent_rate[i] < f_min) > + f_min = host->mux_parent_rate[i]; > + } > + > + /* cacluate f_min based on input clocks, and max divider value */ > + if (f_min != UINT_MAX) > + f_min = DIV_ROUND_UP(CLK_SRC_XTAL_RATE, CLK_DIV_MAX); > + else > + f_min = 4000000; /* default min: 400 MHz */ > + host->mmc->f_min = f_min; > + > + /* create the mux */ > + snprintf(clk_name, sizeof(clk_name), "%s#mux", dev_name(host->dev)); > + init.name = clk_name; > + init.ops = &clk_mux_ops; > + init.flags = 0; > + init.parent_names = mux_parent_names; > + init.num_parents = mux_parent_count; > + > + host->mux.reg = host->regs + SD_EMMC_CLOCK; > + host->mux.shift = CLK_SRC_SHIFT; > + host->mux.mask = CLK_SRC_MASK; > + host->mux.flags = 0; > + host->mux.table = NULL; > + host->mux.hw.init = &init; > + > + host->mux_clk = devm_clk_register(host->dev, &host->mux.hw); > + if (WARN_ON(IS_ERR(host->mux_clk))) > + return PTR_ERR(host->mux_clk); > + > + /* create the divider */ > + snprintf(clk_name, sizeof(clk_name), "%s#div", dev_name(host->dev)); > + init.name = devm_kstrdup(host->dev, clk_name, GFP_KERNEL); > + init.ops = &clk_divider_ops; > + init.flags = CLK_SET_RATE_PARENT; > + clk_div_parents[0] = __clk_get_name(host->mux_clk); > + init.parent_names = clk_div_parents; > + init.num_parents = ARRAY_SIZE(clk_div_parents); > + > + host->cfg_div.reg = host->regs + SD_EMMC_CLOCK; > + host->cfg_div.shift = CLK_DIV_SHIFT; > + host->cfg_div.width = CLK_DIV_WIDTH; > + host->cfg_div.hw.init = &init; > + host->cfg_div.flags = CLK_DIVIDER_ONE_BASED | > + CLK_DIVIDER_ROUND_CLOSEST | CLK_DIVIDER_ALLOW_ZERO; > + > + host->cfg_div_clk = devm_clk_register(host->dev, &host->cfg_div.hw); > + if (WARN_ON(PTR_ERR_OR_ZERO(host->cfg_div_clk))) > + return PTR_ERR(host->cfg_div_clk); > + > + /* init SD_EMMC_CLOCK to sane defaults w/min clock rate */ > + clk_reg = 0; > + clk_reg |= CLK_PHASE_180 << CLK_PHASE_SHIFT; > + clk_reg |= CLK_SRC_XTAL << CLK_SRC_SHIFT; > + clk_reg |= CLK_DIV_MAX << CLK_DIV_SHIFT; > + clk_reg &= ~CLK_ALWAYS_ON; > + writel(clk_reg, host->regs + SD_EMMC_CLOCK); > + > + /* Ensure clock starts in "auto" mode, not "always on" */ > + cfg = readl(host->regs + SD_EMMC_CFG); > + cfg &= ~CFG_CLK_ALWAYS_ON; > + cfg |= CFG_AUTO_CLK; > + writel(cfg, host->regs + SD_EMMC_CFG); > + > + ret = clk_prepare_enable(host->cfg_div_clk); > + if (!ret) > + ret = meson_mmc_clk_set(host, f_min); > + In case of errors, I guess you should disable/unprepare the clock. > + return ret; > +} > + [...] > + > +static int meson_mmc_probe(struct platform_device *pdev) > +{ > + struct resource *res; > + struct meson_host *host; > + struct mmc_host *mmc; > + int ret; > + > + mmc = mmc_alloc_host(sizeof(struct meson_host), &pdev->dev); > + if (!mmc) > + return -ENOMEM; > + host = mmc_priv(mmc); > + host->mmc = mmc; > + host->dev = &pdev->dev; > + dev_set_drvdata(&pdev->dev, host); > + > + spin_lock_init(&host->lock); > + > + host->core_clk = devm_clk_get(&pdev->dev, "core"); > + if (IS_ERR(host->core_clk)) { > + ret = PTR_ERR(host->core_clk); > + goto free_host; > + } > + > + /* Get regulators and the supported OCR mask */ > + host->vqmmc_enabled = false; > + ret = mmc_regulator_get_supply(mmc); > + if (ret == -EPROBE_DEFER) > + goto free_host; In free_host you call clk_disable_unprepare(). You don't want to do that unless you already called clk_prepare_enable(). So, I think you need another label to distinguish between these two cases. > + > + ret = mmc_of_parse(mmc); > + if (ret) { > + dev_warn(&pdev->dev, "error parsing DT: %d\n", ret); > + goto free_host; > + } > + > + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > + host->regs = devm_ioremap_resource(&pdev->dev, res); > + if (IS_ERR(host->regs)) { > + ret = PTR_ERR(host->regs); > + goto free_host; > + } > + > + host->irq = platform_get_irq(pdev, 0); > + if (host->irq == 0) { > + dev_err(&pdev->dev, "failed to get interrupt resource.\n"); > + ret = -EINVAL; > + goto free_host; > + } > + > + ret = clk_prepare_enable(host->core_clk); > + if (ret) > + goto free_host; > + > + ret = meson_mmc_clk_init(host); > + if (ret) > + goto free_host; > + > + /* Stop execution */ > + writel(0, host->regs + SD_EMMC_START); > + > + /* clear, ack, enable all interrupts */ > + writel(0, host->regs + SD_EMMC_IRQ_EN); > + writel(IRQ_EN_MASK, host->regs + SD_EMMC_STATUS); > + > + ret = devm_request_threaded_irq(&pdev->dev, host->irq, > + meson_mmc_irq, meson_mmc_irq_thread, > + IRQF_SHARED, DRIVER_NAME, host); > + if (ret) > + goto free_host; Besides the earlier issue mentiond for the label free_host, here you also need to consider yet another clock that has be been enabled in meson_mmc_clk_init(). I assume you want to disable this clock in the error path. > + > + /* data bounce buffer */ > + host->bounce_buf_size = SZ_512K; > + host->bounce_buf = > + dma_alloc_coherent(host->dev, host->bounce_buf_size, > + &host->bounce_dma_addr, GFP_KERNEL); > + if (host->bounce_buf == NULL) { > + dev_err(host->dev, "Unable to map allocate DMA bounce buffer.\n"); > + ret = -ENOMEM; > + goto free_host; > + } > + > + mmc->ops = &meson_mmc_ops; > + mmc_add_host(mmc); > + > + return 0; > + > +free_host: > + dev_dbg(host->dev, "Failed to probe: ret=%d\n", ret); No need to print this, already managed by driver core. > + if (host->core_clk) No need to check above, already done by the clock framework. > + clk_disable_unprepare(host->core_clk); > + mmc_free_host(mmc); > + return ret; > +} > + > +static int meson_mmc_remove(struct platform_device *pdev) > +{ > + struct meson_host *host = dev_get_drvdata(&pdev->dev); > + > + if (WARN_ON(!host)) > + return 0; > + > + if (host->bounce_buf) > + dma_free_coherent(host->dev, host->bounce_buf_size, > + host->bounce_buf, host->bounce_dma_addr); > + > + if (host->cfg_div_clk) No need to check this, aleady done by the clock framework. > + clk_disable_unprepare(host->cfg_div_clk); > + > + if (host->core_clk) Ditto. > + clk_disable_unprepare(host->core_clk); > + > + mmc_free_host(host->mmc); > + return 0; > +} > + [...] Besides the minor stuff pointed out above, this looks great to me! Before you re-spin, please drop this series from your branch which is being integrated into linux-next, as otherwise I can't pick it up. Kind regards Uffe -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html