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


Lad Prabhakar
 

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

Join cip-dev@lists.cip-project.org to automatically receive all group messages.