Hi Pavel,

Thank you for the review.

> -----Original Message-----
> From: Pavel Machek <pavel@denx.de>
> Sent: 23 November 2020 19:38
> 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 2/7] memory: add Renesas RPC-IF driver
> 
> Hi!
> 
> > +EXPORT_SYMBOL(rpcif_sw_init);
> 
> EXPORT_SYMBOL_GPL?
> 
Agreed will fix that and squash that in the current patch.

> > +void rpcif_enable_rpm(struct rpcif *rpc)
> > +{
> > +	pm_runtime_enable(rpc->dev);
> > +}
> > +EXPORT_SYMBOL(rpcif_enable_rpm);
> > +
> > +void rpcif_disable_rpm(struct rpcif *rpc)
> > +{
> > +	pm_runtime_put_sync(rpc->dev);
> > +}
> > +EXPORT_SYMBOL(rpcif_disable_rpm);
> 
> Should these go to header as static inlines?
> 
Would be nice will make them inline and squash it in the current patch.

> > +static int wait_msg_xfer_end(struct rpcif *rpc)
> > +{
> > +	u32 sts;
> > +
> > +	return regmap_read_poll_timeout(rpc->regmap, RPCIF_CMNSR, sts,
> > +					sts & RPCIF_CMNSR_TEND, 0,
> > +					USEC_PER_SEC);
> > +}
> 
> This can't be right. sts is used uninitialized here.
> 
Third parameter in regmap_read_poll_timeout() is the variable in which value is read and the fourth parameter condition has to be tied with the third parameter (there are similar instance in the kernel).

> > +int rpcif_manual_xfer(struct rpcif *rpc)
> > +{
> > +	default:
> > +		regmap_write(rpc->regmap, RPCIF_SMENR, rpc->enable);
> > +		regmap_write(rpc->regmap, RPCIF_SMCR,
> > +			     rpc->smcr | RPCIF_SMCR_SPIE);
> > +		ret = wait_msg_xfer_end(rpc);
> > +		if (ret)
> > +			goto err_out;
> > +	}
> > +
> > +exit:
> > +	pm_runtime_put(rpc->dev);
> > +	return ret;
> > +
> > +err_out:
> > +	ret = reset_control_reset(rpc->rstc);
> > +	rpcif_hw_init(rpc, rpc->bus_size == 2);
> > +	goto exit;
> > +}
> 
> So we get failure in wait_msg_xfer_end(rpc); but then
> reset_control_reset(rpc->rstc); returns success and whole function
> returns success. Is that ok?
> 
Good catch this needs fixing (upstream too).

> > +static int rpcif_probe(struct platform_device *pdev)
> > +{
> > +	struct platform_device *vdev;
> > +	struct device_node *flash;
> > +	const char *name;
> > +
> > +	flash = of_get_next_child(pdev->dev.of_node, NULL);
> > +	if (!flash) {
> > +		dev_warn(&pdev->dev, "no flash node found\n");
> > +		return -ENODEV;
> > +	}
> 
> Does this need corresponding of_node_put()?
> 
Agreed this needs fixing (upstream too).

> > +struct	rpcif_op {
> 
> Weird formatting; we normally use spaces (not tabs) for this.
> 
Agreed will fix that.

> > +	struct {
> > +		u8 buswidth;
> ...
> > +	struct {
> > +		u8 ncycles;
> > +		u8 buswidth;
> > +	} dummy;
> 
> Are you sure this will be consistent accross architectures? Should the
> structure be marked attribute packed or something?
> 
Should be OK as this driver is intended only on R-Car Gen3 and RZ/G2x (arm64).

Cheers,
Prabhakar

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