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@...>
Sent: 23 November 2020 19:38
To: Prabhakar Mahadev Lad <prabhakar.mahadev-lad.rj@...>
Cc: cip-dev@...; Nobuhiro Iwamatsu <nobuhiro1.iwamatsu@...>; Pavel Machek
<pavel@...>; Biju Das <biju.das.jz@...>
Subject: Re: [PATCH 4.19.y-cip 2/7] memory: add Renesas RPC-IF driver


Agreed will fix that and squash that in the current patch.

+void rpcif_enable_rpm(struct rpcif *rpc)
+ pm_runtime_enable(rpc->dev);
+void rpcif_disable_rpm(struct rpcif *rpc)
+ pm_runtime_put_sync(rpc->dev);
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,
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;
+ }
+ pm_runtime_put(rpc->dev);
+ return ret;
+ 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).


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

Join to automatically receive all group messages.