| Message ID | 19358863deae03b1b26f473e878305a1c6e40d19.1649681638.git.geert+renesas@glider.be |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | memory: renesas-rpc-if: Simplify single/double data register access | expand |
On Mon, Apr 11, 2022 at 02:55:29PM +0200, Geert Uytterhoeven wrote: > For manual write and read, factor out the common access to the first > data register by keeping track of the current data pointer. > > Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be> Works fine with reading/writing on a V3U, so: Tested-by: Wolfram Sang <wsa+renesas@sang-engineering.com> I agree the code is more concise. I am not sure, though, if it is really more readable. But I don't mind very much, so except for a small nit: Reviewed-by: Wolfram Sang <wsa+renesas@sang-engineering.com> > + if (nbytes == 8) > + regmap_write(rpc->regmap, RPCIF_SMWDR1, *p++); > + regmap_write(rpc->regmap, RPCIF_SMWDR0, *p++); Last '++' can be omitted?
Hi Wolfram, On Tue, Apr 12, 2022 at 11:42 AM Wolfram Sang <wsa+renesas@sang-engineering.com> wrote: > On Mon, Apr 11, 2022 at 02:55:29PM +0200, Geert Uytterhoeven wrote: > > For manual write and read, factor out the common access to the first > > data register by keeping track of the current data pointer. > > > > Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be> > > Works fine with reading/writing on a V3U, so: > > Tested-by: Wolfram Sang <wsa+renesas@sang-engineering.com> > > I agree the code is more concise. I am not sure, though, if it is really > more readable. But I don't mind very much, so except for a small nit: > > Reviewed-by: Wolfram Sang <wsa+renesas@sang-engineering.com> Thanks! > > + if (nbytes == 8) > > + regmap_write(rpc->regmap, RPCIF_SMWDR1, *p++); > > + regmap_write(rpc->regmap, RPCIF_SMWDR0, *p++); > > Last '++' can be omitted? I know. But I think it looks nicer this way ;-) The compiler doesn't care anyway. Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds
> > > + regmap_write(rpc->regmap, RPCIF_SMWDR0, *p++); > > > > Last '++' can be omitted? > > I know. But I think it looks nicer this way ;-) I have to admit it looks a little like "I copy&pasted without thinking" to me :)
Hi Wolfram, On Tue, Apr 12, 2022 at 11:49 AM Wolfram Sang <wsa+renesas@sang-engineering.com> wrote: > > > > + regmap_write(rpc->regmap, RPCIF_SMWDR0, *p++); > > > > > > Last '++' can be omitted? > > > > I know. But I think it looks nicer this way ;-) > > I have to admit it looks a little like "I copy&pasted without thinking" > to me :) But that's not what happened. I even compared the assembler output of various solutions. Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds
> But that's not what happened. I even compared the assembler output of > various solutions. I am sure you did. I just said "it looks like..." to state a reason why I don't think it looks prettier. But this turns to bike-shedding for me, just keep it if everyone else is happy.
On 12/04/2022 12:17, Wolfram Sang wrote: > >> But that's not what happened. I even compared the assembler output of >> various solutions. > > I am sure you did. I just said "it looks like..." to state a reason why > I don't think it looks prettier. But this turns to bike-shedding for me, > just keep it if everyone else is happy. I vote for removal of ++, because later someone might wonder why it was incremented even if not used. Best regards, Krzysztof
diff --git a/drivers/memory/renesas-rpc-if.c b/drivers/memory/renesas-rpc-if.c index 019a0822bde0e413..fb2a507ca2badc70 100644 --- a/drivers/memory/renesas-rpc-if.c +++ b/drivers/memory/renesas-rpc-if.c @@ -488,7 +488,7 @@ int rpcif_manual_xfer(struct rpcif *rpc) case RPCIF_DATA_OUT: while (pos < rpc->xferlen) { u32 bytes_left = rpc->xferlen - pos; - u32 nbytes, data[2]; + u32 nbytes, data[2], *p = data; smcr = rpc->smcr | RPCIF_SMCR_SPIE; @@ -502,15 +502,9 @@ int rpcif_manual_xfer(struct rpcif *rpc) rpc->xfer_size = nbytes; memcpy(data, rpc->buffer + pos, nbytes); - if (nbytes == 8) { - regmap_write(rpc->regmap, RPCIF_SMWDR1, - data[0]); - regmap_write(rpc->regmap, RPCIF_SMWDR0, - data[1]); - } else { - regmap_write(rpc->regmap, RPCIF_SMWDR0, - data[0]); - } + if (nbytes == 8) + regmap_write(rpc->regmap, RPCIF_SMWDR1, *p++); + regmap_write(rpc->regmap, RPCIF_SMWDR0, *p++); regmap_write(rpc->regmap, RPCIF_SMCR, smcr); ret = wait_msg_xfer_end(rpc); @@ -552,7 +546,7 @@ int rpcif_manual_xfer(struct rpcif *rpc) } while (pos < rpc->xferlen) { u32 bytes_left = rpc->xferlen - pos; - u32 nbytes, data[2]; + u32 nbytes, data[2], *p = data; /* nbytes may only be 1, 2, 4, or 8 */ nbytes = bytes_left >= max ? max : (1 << ilog2(bytes_left)); @@ -569,15 +563,9 @@ int rpcif_manual_xfer(struct rpcif *rpc) if (ret) goto err_out; - if (nbytes == 8) { - regmap_read(rpc->regmap, RPCIF_SMRDR1, - &data[0]); - regmap_read(rpc->regmap, RPCIF_SMRDR0, - &data[1]); - } else { - regmap_read(rpc->regmap, RPCIF_SMRDR0, - &data[0]); - } + if (nbytes == 8) + regmap_read(rpc->regmap, RPCIF_SMRDR1, p++); + regmap_read(rpc->regmap, RPCIF_SMRDR0, p++); memcpy(rpc->buffer + pos, data, nbytes); pos += nbytes;
For manual write and read, factor out the common access to the first data register by keeping track of the current data pointer. Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be> --- drivers/memory/renesas-rpc-if.c | 28 ++++++++-------------------- 1 file changed, 8 insertions(+), 20 deletions(-)