Re: [bug report] spi: add support for microchip fpga spi controllers

[Dan Carpenter <dan.carpenter@xxxxxxxxxx>]

On Wed, Jun 15, 2022 at 08:33:35AM +0000, Conor.Dooley@xxxxxxxxxxxxx wrote:
> >      541         spi->irq = platform_get_irq(pdev, 0);
> >      542         if (spi->irq <= 0) {
> >      543                 dev_err(&pdev->dev, "invalid IRQ %d for SPI controller\n", spi->irq);
> >      544                 ret = spi->irq;
> >      545                 goto error_release_master;
> >      546         }
> >      547
> >      548         ret = devm_request_irq(&pdev->dev, spi->irq, mchp_corespi_interrupt,
> >      549                                IRQF_SHARED, dev_name(&pdev->dev), master);
> >      550         if (ret) {
> >      551                 dev_err(&pdev->dev, "could not request irq: %d\n", ret);
> >      552                 goto error_release_master;
> >      553         }
> >      554
> >      555         spi->clk = devm_clk_get(&pdev->dev, NULL);
> >      556         if (!spi->clk || IS_ERR(spi->clk)) {
> >                       ^^^^^^^^
> > NULL
> > 
> > --> 557                 ret = PTR_ERR(spi->clk);
> > 
> > ret is 0/success.
> > 
> > Normally when functions like this return NULL, you're supposed to just
> > accept the NULL and add tests for it to avoid NULL related bugs.  In
> > this driver if spi->clk is NULL then it leads to spi_hz == 0 which leads
> > to a divide by zero bug.  So it's not clear which way to go on this?
> > Fix the error code or add more checks for NULL?
> 
> Am I being dumb here, or should the null check just be removed like
> every other driver? As in, devm_clk_get will only return a valid
> clk or an IS_ERR() condition.

It can return NULL if CONFIG_HAVE_CLK is disabled.  I don't know the
hardware or if that CONFIG_ is essential for booting.

> 
> The correct solution seems to me to be remove the !spi->clk check?

That's the normal solution, yes.  But if you do that, then please add a
check to prevent the divide by zero:
`grep -w clk drivers/spi/spi-microchip-core.c`

regards,
dan carpenter