diff mbox series

[5.10.y-cip,20/22] dmaengine: sh: make array ds_lut static

Message ID 20211220133139.21624-21-prabhakar.mahadev-lad.rj@bp.renesas.com (mailing list archive)
State Accepted
Delegated to: Pavel Machek
Headers show
Series RZ/G2L: Add support for pinctrl/dmac/iic | expand

Commit Message

Lad Prabhakar Dec. 20, 2021, 1:31 p.m. UTC 
From: Colin Ian King <colin.king@canonical.com>

commit 4c0eee50658746b0333d35a75d3db6e0aac08ef9 upstream.

Don't populate the read-only array ds_lut on the stack but instead it
static. Also makes the object code smaller by 163 bytes:

Before:
   text    data     bss     dec     hex filename
  23508    4796       0   28304    6e90 ./drivers/dma/sh/rz-dmac.o

After:
   text    data     bss     dec     hex filename
  23281    4860       0   28141    6ded ./drivers/dma/sh/rz-dmac.o

(gcc version 11.2.0)

Signed-off-by: Colin Ian King <colin.king@canonical.com>
Link: https://lore.kernel.org/r/20210915112038.12407-1-colin.king@canonical.com
Signed-off-by: Vinod Koul <vkoul@kernel.org>
Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
---
 drivers/dma/sh/rz-dmac.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Pavel Machek Dec. 22, 2021, 10:04 a.m. UTC | #1 
Hi!

> commit 4c0eee50658746b0333d35a75d3db6e0aac08ef9 upstream.
> 
> Don't populate the read-only array ds_lut on the stack but instead it
> static. Also makes the object code smaller by 163 bytes:
> 
> Before:
>    text    data     bss     dec     hex filename
>   23508    4796       0   28304    6e90 ./drivers/dma/sh/rz-dmac.o
> 
> After:
>    text    data     bss     dec     hex filename
>   23281    4860       0   28141    6ded ./drivers/dma/sh/rz-dmac.o

Heh.

> @@ -574,7 +574,7 @@ static void rz_dmac_issue_pending(struct dma_chan *chan)
>  static u8 rz_dmac_ds_to_val_mapping(enum dma_slave_buswidth ds)
>  {
>  	u8 i;
> -	const enum dma_slave_buswidth ds_lut[] = {
> +	static const enum dma_slave_buswidth ds_lut[] = {
>  		DMA_SLAVE_BUSWIDTH_1_BYTE,
>  		DMA_SLAVE_BUSWIDTH_2_BYTES,
>  		DMA_SLAVE_BUSWIDTH_4_BYTES,

Array could be avoided altogether; you could check for power of two
and then count bits. That would give even shorter code, but I'm not
sure about readability.

I'd also not mind using usual convention here: return int, >= 0
success, < 0 errno.

Best regards,
								Pavel
Lad Prabhakar Dec. 22, 2021, 10:26 a.m. UTC | #2 
Hi Pavel,

Thank you for the review.

> -----Original Message-----
> From: Pavel Machek <pavel@denx.de>
> Sent: 22 December 2021 10:05
> 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 5.10.y-cip 20/22] dmaengine: sh: make array ds_lut static
> 
> Hi!
> 
> > commit 4c0eee50658746b0333d35a75d3db6e0aac08ef9 upstream.
> >
> > Don't populate the read-only array ds_lut on the stack but instead it
> > static. Also makes the object code smaller by 163 bytes:
> >
> > Before:
> >    text    data     bss     dec     hex filename
> >   23508    4796       0   28304    6e90 ./drivers/dma/sh/rz-dmac.o
> >
> > After:
> >    text    data     bss     dec     hex filename
> >   23281    4860       0   28141    6ded ./drivers/dma/sh/rz-dmac.o
> 
> Heh.
> 
> > @@ -574,7 +574,7 @@ static void rz_dmac_issue_pending(struct dma_chan
> > *chan)  static u8 rz_dmac_ds_to_val_mapping(enum dma_slave_buswidth
> > ds)  {
> >  	u8 i;
> > -	const enum dma_slave_buswidth ds_lut[] = {
> > +	static const enum dma_slave_buswidth ds_lut[] = {
> >  		DMA_SLAVE_BUSWIDTH_1_BYTE,
> >  		DMA_SLAVE_BUSWIDTH_2_BYTES,
> >  		DMA_SLAVE_BUSWIDTH_4_BYTES,
> 
> Array could be avoided altogether; you could check for power of two and then count bits. That would
> give even shorter code, but I'm not sure about readability.
> 
We might loose readability, Ill let Biju decide on this.

> I'd also not mind using usual convention here: return int, >= 0 success, < 0 errno.
> 
Agreed.

Cheers,
Prabhakar

> Best regards,
> 								Pavel
> --
> DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
> HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Biju Das Jan. 4, 2022, 10:39 a.m. UTC | #3 
Hi Pavel,

> Subject: Re: [PATCH 5.10.y-cip 20/22] dmaengine: sh: make array ds_lut
> static
> 
> Hi!
> 
> > commit 4c0eee50658746b0333d35a75d3db6e0aac08ef9 upstream.
> >
> > Don't populate the read-only array ds_lut on the stack but instead it
> > static. Also makes the object code smaller by 163 bytes:
> >
> > Before:
> >    text    data     bss     dec     hex filename
> >   23508    4796       0   28304    6e90 ./drivers/dma/sh/rz-dmac.o
> >
> > After:
> >    text    data     bss     dec     hex filename
> >   23281    4860       0   28141    6ded ./drivers/dma/sh/rz-dmac.o
> 
> Heh.
> 
> > @@ -574,7 +574,7 @@ static void rz_dmac_issue_pending(struct dma_chan
> > *chan)  static u8 rz_dmac_ds_to_val_mapping(enum dma_slave_buswidth
> > ds)  {
> >  	u8 i;
> > -	const enum dma_slave_buswidth ds_lut[] = {
> > +	static const enum dma_slave_buswidth ds_lut[] = {
> >  		DMA_SLAVE_BUSWIDTH_1_BYTE,
> >  		DMA_SLAVE_BUSWIDTH_2_BYTES,
> >  		DMA_SLAVE_BUSWIDTH_4_BYTES,
> 
> Array could be avoided altogether; you could check for power of two and
> then count bits. That would give even shorter code, but I'm not sure about
> readability.
> 
> I'd also not mind using usual convention here: return int, >= 0 success, <
> 0 errno.

Readability is ok with current implementation and moreover it is faster.

Otherwise, you need to check whether it is power of 2, then do count bits to make sure it is bounded between
DMA_SLAVE_BUSWIDTH_1_BYTE and DMA_SLAVE_BUSWIDTH_128_BYTES and then return the index.

Regards,
Biju
diff mbox series

Patch

diff --git a/drivers/dma/sh/rz-dmac.c b/drivers/dma/sh/rz-dmac.c
index d9f2cfef878e..ee2872e7d64c 100644
--- a/drivers/dma/sh/rz-dmac.c
+++ b/drivers/dma/sh/rz-dmac.c
@@ -574,7 +574,7 @@  static void rz_dmac_issue_pending(struct dma_chan *chan)
 static u8 rz_dmac_ds_to_val_mapping(enum dma_slave_buswidth ds)
 {
 	u8 i;
-	const enum dma_slave_buswidth ds_lut[] = {
+	static const enum dma_slave_buswidth ds_lut[] = {
 		DMA_SLAVE_BUSWIDTH_1_BYTE,
 		DMA_SLAVE_BUSWIDTH_2_BYTES,
 		DMA_SLAVE_BUSWIDTH_4_BYTES,