Hi Conor, On Mon, Aug 1, 2022 at 4:19 PM <Conor.Dooley@xxxxxxxxxxxxx> wrote: > > On 01/08/2022 10:42, Naga Sureshkumar Relli wrote: > > Add a driver for Microchip FPGA QSPI controllers. This driver also > > supports "hard" QSPI controllers on Polarfire SoC. > > > > Signed-off-by: Naga Sureshkumar Relli <nagasuresh.relli@xxxxxxxxxxxxx> > > --- > > ---8<--- > > > +static int mchp_coreqspi_probe(struct platform_device *pdev) > > +{ > > + struct spi_controller *ctlr; > > + struct mchp_coreqspi *qspi; > > + struct device *dev = &pdev->dev; > > + struct device_node *np = dev->of_node; > > + int ret; > > + > > + ctlr = spi_alloc_master(&pdev->dev, sizeof(*qspi)); > > + if (!ctlr) > > + return -ENOMEM; > > Argh... I am sorry for not mentioning this when you asked me if > I thought the driver was ready to be sent upstream, but I think > we should try to use the devm_ versions of these functions. Ok. > > > + > > + qspi = spi_controller_get_devdata(ctlr); > > Is there a reason to use spi_controller_get_devdata() rather than > spi_master_get_devdata() ? Both are the same. https://github.com/torvalds/linux/blob/master/include/linux/spi/spi.h#L1497 Ref: https://github.com/torvalds/linux/commit/8caab75fd2c2a92667cbb1cd315720bede3feaa9 > > > + platform_set_drvdata(pdev, qspi); > > + > > + qspi->regs = devm_platform_ioremap_resource(pdev, 0); > > + if (IS_ERR(qspi->regs)) { > > + ret = PTR_ERR(qspi->regs); > > + goto remove_master; > > Using devm_spi_alloc_master() above would simplify this > to just a return of dev_err_probe() & ditto for every > time we do a "goto remove_master" Ok. I will update to use devm_ versions. > > > + } > > + > > + qspi->clk = devm_clk_get(&pdev->dev, NULL); > > + if (IS_ERR(qspi->clk)) { > > + dev_err(&pdev->dev, "clock not found.\n"); > > + ret = PTR_ERR(qspi->clk); > > + goto remove_master; > > + } > > + > > + ret = clk_prepare_enable(qspi->clk); > > + if (ret) { > > + dev_err(&pdev->dev, "failed to enable clock\n"); > > + goto remove_master; > > + } > > + > > + init_completion(&qspi->data_completion); > > + mutex_init(&qspi->op_lock); > > + > > + qspi->irq = platform_get_irq(pdev, 0); > > + if (qspi->irq <= 0) { > > + ret = qspi->irq; > > + goto clk_dis_all; > > + } > > + > > + ret = devm_request_irq(&pdev->dev, qspi->irq, mchp_coreqspi_isr, > > + 0, pdev->name, qspi); > > + if (ret != 0) { > > + dev_err(&pdev->dev, "request_irq failed %d\n", ret); > > + goto clk_dis_all; > > + } > > + > > + ctlr->bits_per_word_mask = SPI_BPW_MASK(8); > > + ctlr->mem_ops = &mchp_coreqspi_mem_ops; > > + ctlr->setup = mchp_coreqspi_setup_op; > > + ctlr->mode_bits = SPI_CPOL | SPI_CPHA | SPI_RX_DUAL | SPI_RX_QUAD | > > + SPI_TX_DUAL | SPI_TX_QUAD; > > + ctlr->dev.of_node = np; > > + > > + ret = devm_spi_register_controller(&pdev->dev, ctlr); > > + if (ret) { > > + dev_err(&pdev->dev, "spi_register_controller failed\n"); > > + goto clk_dis_all; > > + } > > + > > + return 0; > > + > > +clk_dis_all: > > + clk_disable_unprepare(qspi->clk); > > If we move the clk prepare & enable later in probe, ie after > getting the irq, this goto could be removed too because the > only place requiring teardown of the clock would be the error > path of devm_spi_register_controller(). ok. > > > +remove_master: > > + spi_controller_put(ctlr); > > With devm_spi_alloc_master() this put is no longer needed & > we can return a nice dev_err_probe() for each error :) ok. I will send the next version with these updates. > > > + > > + return ret; > > +} > > Thanks Suresh. > Conor.