Re: [cip-dev] [PATCH 4.19.y-cip 7/7] spi: add Renesas RPC-IF driver

From: "Pavel Machek" <pavel@denx.de>
To: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
Cc: cip-dev@lists.cip-project.org,
	Nobuhiro Iwamatsu <nobuhiro1.iwamatsu@toshiba.co.jp>,
	Pavel Machek <pavel@denx.de>,
	Biju Das <biju.das.jz@bp.renesas.com>
Subject: Re: [cip-dev] [PATCH 4.19.y-cip 7/7] spi: add Renesas RPC-IF driver
Date: Mon, 23 Nov 2020 20:52:28 +0100	[thread overview]
Message-ID: <20201123195228.GE5619@amd> (raw)
In-Reply-To: <20201123120354.26413-8-prabhakar.mahadev-lad.rj@bp.renesas.com>

[-- Attachment #1.1: Type: text/plain, Size: 2832 bytes --]

Hi!

> commit eb8d6d464a27850498dced21a8450e85d4a02009 upstream.
> 
> Add the SPI driver for the Renesas RPC-IF.  It's the "front end" driver
> using the "back end" APIs in the main driver to talk to the real hardware.
> We only implement the 'spi-mem' interface -- there's no need to implement
> the usual SPI driver methods...
> 
> Based on the original patch by Mason Yang <masonccyang@mxic.com.tw>.
> 
> Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>
> Link: https://lore.kernel.org/r/1ece0e6c-71af-f0f1-709e-571f4b0b4853@cogentembedded.com
> Signed-off-by: Mark Brown <broonie@kernel.org>
> Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> ---
>  drivers/spi/Kconfig      |   6 ++
>  drivers/spi/Makefile     |   1 +
>  drivers/spi/spi-rpc-if.c | 216 +++++++++++++++++++++++++++++++++++++++
>  3 files changed, 223 insertions(+)
>  create mode 100644 drivers/spi/spi-rpc-if.c
> 
> diff --git a/drivers/spi/Kconfig b/drivers/spi/Kconfig
> index 0a7fd56c1ed9..461315967f96 100644
> --- a/drivers/spi/Kconfig
> +++ b/drivers/spi/Kconfig
> @@ -514,6 +514,12 @@ config SPI_RB4XX
>  	help
>  	  SPI controller driver for the Mikrotik RB4xx series boards.
>  
> +config SPI_RPCIF
> +	tristate "Renesas RPC-IF SPI driver"
> +	depends on RENESAS_RPCIF
> +	help
> +	  SPI driver for Renesas R-Car Gen3 RPC-IF.

Few lines here explaining the acronyms etc. would be nice.

> +	error = spi_register_controller(ctlr);
> +	if (error) {
> +		dev_err(&pdev->dev, "spi_register_controller failed\n");
> +		goto err_put_ctlr;
> +	}
> +	return 0;
> +
> +err_put_ctlr:
> +	rpcif_disable_rpm(rpc);
> +	spi_controller_put(ctlr);
> +
> +	return error;
> +}

With just one user of the error exit, I'd avoid the goto.

> +#ifdef CONFIG_PM_SLEEP
> +static int rpcif_spi_suspend(struct device *dev)
> +{
> +	struct spi_controller *ctlr = dev_get_drvdata(dev);
> +
> +	return spi_controller_suspend(ctlr);
> +}
> +
> +static int rpcif_spi_resume(struct device *dev)
> +{
> +	struct spi_controller *ctlr = dev_get_drvdata(dev);
> +
> +	return spi_controller_resume(ctlr);
> +}
> +
> +static SIMPLE_DEV_PM_OPS(rpcif_spi_pm_ops, rpcif_spi_suspend, rpcif_spi_resume);
> +#define DEV_PM_OPS	(&rpcif_spi_pm_ops)
> +#else
> +#define DEV_PM_OPS	NULL
> +#endif
> +
> +static struct platform_driver rpcif_spi_driver = {
> +	.probe	= rpcif_spi_probe,
> +	.remove	= rpcif_spi_remove,
> +	.driver = {
> +		.name	= "rpc-if-spi",
> +		.pm	= DEV_PM_OPS,
> +	},
> +};

Can you just add "__maybe_unused" annotation and avoid the ifdefs,
like atmel-quadspi.c does?

Best regards,
								Pavel
-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

[-- Attachment #2: Type: text/plain, Size: 420 bytes --]

-=-=-=-=-=-=-=-=-=-=-=-
Links: You receive all messages sent to this group.
View/Reply Online (#5831): https://lists.cip-project.org/g/cip-dev/message/5831
Mute This Topic: https://lists.cip-project.org/mt/78451549/4520388
Group Owner: cip-dev+owner@lists.cip-project.org
Unsubscribe: https://lists.cip-project.org/g/cip-dev/leave/8129055/727948398/xyzzy [cip-dev@archiver.kernel.org]
-=-=-=-=-=-=-=-=-=-=-=-