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

[Dan Carpenter <dan.carpenter@oracle.com>]

On Wed, Jun 15, 2022 at 08:58:53AM +0000, Conor.Dooley@microchip.com wrote:
> On 15/06/2022 09:40, Dan Carpenter wrote:
> > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> > 
> > On Wed, Jun 15, 2022 at 08:33:35AM +0000, Conor.Dooley@microchip.com 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.
> 
> Ehh I guess it is /possible/ that CONFIG_HAVE_CLK could be off
> if someone is accessing the FPGA from another device.
> In that case, neither option really particularly appeals to me.
> Just return -ENODEV I guess?
> 

To be honest, I always prefer just accepting the NULL check and adding
the checks but also philosophical debates are kind of a waste of time.

Do whatever is easiest.  :)

regards,
dan carpenter