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
