RE: [PATCH v4 2/2] spi: spi-mem: Add Realtek SPI NOR flash controller driver

["Yu-Chun Lin [林祐君]" <eleanor.lin@realtek.com>]

Hi Philipp,

Thanks for your review.

> > +static int rtk_spi_probe(struct platform_device *pdev) {
> > +     struct device *dev = &pdev->dev;
> > +     struct spi_controller *ctrl;
> > +     struct rtk_spi_host *host;
> > +     struct resource *res;
> > +     int ret;
> > +
> > +     ctrl = devm_spi_alloc_host(dev, sizeof(*host));
> > +     if (!ctrl)
> > +             return -ENOMEM;
> > +
> > +     platform_set_drvdata(pdev, ctrl);
> > +     host = spi_controller_get_devdata(ctrl);
> > +     host->dev = dev;
> > +
> > +     host->clk = devm_clk_get(dev, NULL);
> > +     if (IS_ERR(host->clk))
> > +             return PTR_ERR(host->clk);
> > +
> > +     ret = clk_prepare_enable(host->clk);
> > +     if (ret)
> > +             return ret;
> > +
> > +     host->rstc = devm_reset_control_get_optional_exclusive(dev, NULL);
> > +     if (IS_ERR(host->rstc)) {
> > +             ret = PTR_ERR(host->rstc);
> > +             goto err_disable_clk;
> 
> If you moved clk_prepare_enable() below this, you could just return here.
> 

Ack.

> > +     }
> > +
> > +     reset_control_assert(host->rstc);
> > +     usleep_range(10, 20);
> > +     reset_control_deassert(host->rstc);
> > +
> > +     ret = dma_set_mask_and_coherent(dev, DMA_BIT_MASK(35));
> > +     if (ret) {
> > +             dev_err(dev, "Failed to set dma mask\n");
> > +             goto err_disable_clk;
> 
> No reset_control_assert() in this error path?
> 
> > +     }
> > +
> > +     host->buffer = dmam_alloc_coherent(dev, SFC_DMA_MAX_LEN,
> > +                                        &host->dma_buffer,
> GFP_KERNEL);
> > +     if (!host->buffer) {
> > +             ret = -ENOMEM;
> > +             goto err_disable_clk;
> > +     }
> > +
> > +     host->regbase = devm_platform_ioremap_resource_byname(pdev,
> "ctrl");
> > +     if (IS_ERR(host->regbase)) {
> > +             ret = PTR_ERR(host->regbase);
> > +             goto err_disable_clk;
> > +     }
> > +
> > +     host->mdbase = devm_platform_ioremap_resource_byname(pdev,
> "dma");
> > +     if (IS_ERR(host->mdbase)) {
> > +             ret = PTR_ERR(host->mdbase);
> > +             goto err_disable_clk;
> > +     }
> > +
> > +     res = platform_get_resource_byname(pdev, IORESOURCE_MEM,
> "dirmap");
> > +     host->iobase = devm_ioremap_resource(dev, res);
> > +     if (IS_ERR(host->iobase)) {
> > +             ret = PTR_ERR(host->iobase);
> > +             goto err_disable_clk;
> > +     }
> 
> It looks to me like all these dmam_ and devm_ initialization calls should be
> moved up before the clock enable and reset assert/deassert.
> 
> > +
> > +     host->dirmap_size = resource_size(res);
> > +     host->flash_phys_base = res->start;
> > +     host->is_4byte = false;
> > +
> > +     ctrl->mode_bits = SPI_RX_DUAL | SPI_TX_DUAL;
> > +     ctrl->bus_num = -1;
> > +     ctrl->mem_ops = &rtk_spi_mem_ops;
> > +     ctrl->num_chipselect = 1;
> > +     ctrl->auto_runtime_pm = true;
> > +
> > +     rtk_spi_init(host);
> 
> This is the first call that actually needs clocks running and reset triggered.
> 

I will update the sequence as follows:

1. Get the clock and reset controls, then complete other resource initialization.
2. Enable the clock and perform the reset sequence.
3. Call rtk_spi_init().

> > +
> > +     pm_runtime_set_autosuspend_delay(dev,
> SFC_AUTOSUSPEND_TIMEOUT);
> > +     pm_runtime_use_autosuspend(dev);
> > +     pm_runtime_set_active(dev);
> > +     pm_runtime_enable(dev);
> > +     pm_runtime_get_noresume(dev);
> > +
> > +     ret = spi_register_controller(ctrl);
> > +     if (ret < 0) {
> > +             dev_err(dev, "failed to register controller\n");
> > +             goto err_pm_disable;
> > +     }
> > +
> > +     pm_runtime_put_autosuspend(dev);
> > +
> > +     return 0;
> > +
> > +err_pm_disable:
> > +     pm_runtime_put_noidle(dev);
> > +     pm_runtime_disable(dev);
> > +     pm_runtime_set_suspended(dev);
> > +     pm_runtime_dont_use_autosuspend(dev);
> > +     reset_control_assert(host->rstc);
> > +
> > +err_disable_clk:
> > +     clk_disable_unprepare(host->clk);
> > +
> > +     return ret;
> > +}
> > +
> > +static void rtk_spi_remove(struct platform_device *pdev) {
> > +     struct spi_controller *ctrl = platform_get_drvdata(pdev);
> > +     struct rtk_spi_host *host = spi_controller_get_devdata(ctrl);
> > +     struct device *dev = &pdev->dev;
> > +
> > +     spi_unregister_controller(ctrl);
> > +
> > +     pm_runtime_get_sync(dev);
> > +     pm_runtime_disable(dev);
> > +     reset_control_assert(host->rstc);
> > +     clk_disable_unprepare(host->clk);
> > +     pm_runtime_put_noidle(dev);
> > +     pm_runtime_set_suspended(dev);
> 
> Why is this cleanup order different from the probe error path?
> 

I will update both paths to use the same cleanup sequence.

Best Regards,
Yu-Chun

> 
> regards
> Philipp