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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [thread-next>] [day] [month] [year] [list]

Message-ID: <63a467164c985cadce0e28e50508363a8d2f6622.camel@pengutronix.de>
Date:   Fri, 26 Nov 2021 11:36:29 +0100
From:   Philipp Zabel <p.zabel@...gutronix.de>
To:     Lh Kuo 郭力豪 
        <lh.Kuo@...plus.com>, Andy Shevchenko <andy.shevchenko@...il.com>
Cc:     Mark Brown <broonie@...nel.org>, "LH.Kuo" <lhjeff911@...il.com>,
        Rob Herring <robh+dt@...nel.org>,
        linux-spi <linux-spi@...r.kernel.org>,
        devicetree <devicetree@...r.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        "dvorkin@...bo.com" <dvorkin@...bo.com>,
        "qinjian@...lus1.com" <qinjian@...lus1.com>,
        Wells Lu 呂芳騰 
        <wells.lu@...plus.com>
Subject: Re: [PATCH v3 1/2] SPI: Add SPI driver for Sunplus SP7021

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