Re: [PATCH v3 1/2] SPI: Add SPI driver for Sunplus SP7021

[Philipp Zabel]

Hi LH,

On Fri, 2021-11-26 at 07:40 +0000, Lh Kuo 郭力豪 wrote:
[...]
> Amended as follows, is it okay?
> 
> 	ret = devm_request_irq(dev, pspim->mas_irq, sp7021_spi_mas_irq
> 			, IRQF_TRIGGER_RISING, pdev->name, pspim);
> 	if (ret)
> 		return ret;

Comma at the end of the line and align the next line with the opening
parenthesis:

	ret = devm_request_irq(dev, pspim->mas_irq, sp7021_spi_mas_irq,
			       IRQF_TRIGGER_RISING, pdev->name, pspim);

You can use scripts/checkpatch --strict to find these issues before
review.

> > >         pspim->rstc = devm_reset_control_get_exclusive(dev, NULL);
> > >         if (IS_ERR(pspim->rstc)) {
> > >                 return dev_err_probe(dev, PTR_ERR(pspim->rstc), "rst
> > > get fail\n");
> > 
> 
> Amended as follows, is it okay?
> 
> 	pspim->rstc = devm_reset_control_get_exclusive(dev, NULL);
> 	if (IS_ERR(pspim->rstc))
> 		return dev_err_probe(dev, PTR_ERR(pspim->rstc), "rst get fail\n");

Yes.
> > 

> > >         ret = devm_spi_register_controller(dev, ctlr);
> > 
> > You can't mix non-devm with devm APIs. Either all non-devm, or devm followed by non-devm.
> > 
> 
>   I don't understand so I need to change to spi_register_controller(ctlr)?   why?

devm_spi_register_controller() shouldn't be called after
pm_runtime_enable().

You could either switch to devm_pm_runtime_enable() or move the
pm_runtime_enable() after the devm_spi_register_controller() call if
possible, or switch to spi_register_controller().

> I modified the remove-function as follows. I think devm_spi_register_controller(dev, ctlr); should be no problem in the probe funciton.
> static int sp7021_spi_controller_remove(struct platform_device *pdev)
> {
> 	struct spi_controller *ctlr = dev_get_drvdata(&pdev->dev);
> 	struct sp7021_spi_ctlr *pspim = spi_master_get_devdata(ctlr);
> 
> 	pm_runtime_disable(&pdev->dev);

I'm not sure if the SPI framework requires the spi_controller to be
unregistered before hardware is powered off, maybe it is enough to call
spi_controller_suspend() in the right place?

> 	pm_runtime_set_suspended(&pdev->dev);
> 	reset_control_assert(pspim->rstc);
> 	clk_disable_unprepare(pspim->spi_clk);
> 
> 	return 0;
> }

regards
Philipp