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

["Lad Prabhakar" <prabhakar.mahadev-lad.rj@bp.renesas.com>]

[-- Attachment #1: Type: text/plain, Size: 3548 bytes --]

Hi Pavel,

Thank you for the review.

> -----Original Message-----
> From: Pavel Machek <pavel@denx.de>
> Sent: 23 November 2020 19:52
> To: Prabhakar Mahadev Lad <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: [PATCH 4.19.y-cip 7/7] spi: add Renesas RPC-IF driver
> 
> 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.
> 
Will do.

> > +	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.
> 
Agreed shall fix that.

> > +#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?
> 
Agreed will replace that.

Cheers,
Prabhakar

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

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


-=-=-=-=-=-=-=-=-=-=-=-
Links: You receive all messages sent to this group.
View/Reply Online (#5836): https://lists.cip-project.org/g/cip-dev/message/5836
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]
-=-=-=-=-=-=-=-=-=-=-=-