Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1168620 > unrolled thread

Coding style details

Started bykhalasa@piap.pl (Krzysztof Hałasa)
First post2015-06-19 10:40 +0200
Last post2015-06-22 08:50 +0200
Articles 6 — 3 participants

Back to article view | Back to linux.kernel

Contents

  Coding style details khalasa@piap.pl (Krzysztof Hałasa) - 2015-06-19 10:40 +0200
    Re: Coding style details Frans Klaver <fransklaver@gmail.com> - 2015-06-19 11:10 +0200
      Re: Coding style details (checkpatch) khalasa@piap.pl (Krzysztof Hałasa) - 2015-06-22 07:40 +0200
        Re: Coding style details (checkpatch) Joe Perches <joe@perches.com> - 2015-06-22 08:20 +0200
          Re: Coding style details (checkpatch) khalasa@piap.pl (Krzysztof Hałasa) - 2015-06-22 08:40 +0200
            Re: Coding style details (checkpatch) Joe Perches <joe@perches.com> - 2015-06-22 08:50 +0200

#1168620 — Coding style details

Fromkhalasa@piap.pl (Krzysztof Hałasa)
Date2015-06-19 10:40 +0200
SubjectCoding style details
Message-ID<pD00a-25F-29@gated-at.bofh.it>
Hi,

a simple question: which style is preferred?

#define REG8_1(a0) ((const u16[8]){a0, a0 + 1, a0 + 2, a0 + 3})

vs

#define REG8_1(a0) ((const u16[8]) {a0, a0 + 1, a0 + 2, a0 + 3})
                                ^^^^^
-- 
Krzysztof Halasa

Industrial Research Institute for Automation and Measurements PIAP
Al. Jerozolimskie 202, 02-486 Warsaw, Poland
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

#1168643

FromFrans Klaver <fransklaver@gmail.com>
Date2015-06-19 11:10 +0200
Message-ID<pD0tc-2Tl-3@gated-at.bofh.it>
In reply to#1168620
Hi,

On Fri, Jun 19, 2015 at 10:35 AM, Krzysztof Hałasa <khalasa@piap.pl> wrote:
> Hi,
>
> a simple question: which style is preferred?
>
> #define REG8_1(a0) ((const u16[8]){a0, a0 + 1, a0 + 2, a0 + 3})
>
> vs
>
> #define REG8_1(a0) ((const u16[8]) {a0, a0 + 1, a0 + 2, a0 + 3})
>                                 ^^^^^

The prescribed style is to have no space between cast and castee. So,
the top option.

Frans
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1169697 — Re: Coding style details (checkpatch)

Fromkhalasa@piap.pl (Krzysztof Hałasa)
Date2015-06-22 07:40 +0200
SubjectRe: Coding style details (checkpatch)
Message-ID<pE2CC-2QP-7@gated-at.bofh.it>
In reply to#1168643
Joe Perches <joe@perches.com> writes:

> How is the macro used?
> #define REG8_1(a0) ((const u16[8]){a0, a0 + 1, a0 + 2, a0 + 3})

#define REG8_1(a0) ((const u16[8]){a0, a0 + 1, a0 + 2, a0 + 3, a0 + 4, a0 + 5, a0 + 6, a0 + 7})
#define REG8_2(a0) ((const u16[8]){a0, a0 + 2, a0 + 4, a0 + 6, a0 + 8, a0 + 0xA, a0 + 0xC, a0 + 0xE})
#define REG8_8(a0) ((const u16[8]){a0, a0 + 8, a0 + 0x10, a0 + 0x18, a0 + 0x20, a0 + 0x28, a0 + 0x30, a0 + 0x38})

#define VDMA_CHANNEL_CONFIG     REG8_1(0x10)
#define ADMA_P_ADDR             REG8_2(0x18)
#define ADMA_B_ADDR             REG8_2(0x19)
#define INTL_HBAR_CTRL          REG8_1(0x30)
#define VIDEO_FIELD_CTRL        REG8_1(0x39)
#define HSCALER_CTRL            REG8_1(0x42)
#define VIDEO_SIZE              REG8_1(0x4A)
#define VIDEO_SIZE_F2           REG8_1(0x52)
#define MD_CONF                 REG8_1(0x60)
#define MD_INIT                 REG8_1(0x68)
#define MD_MAP0                 REG8_1(0x70)
#define VDMA_P_ADDR             REG8_8(0x80) /* not used in DMA SG mode */
#define VDMA_WHP                REG8_8(0x81)
#define VDMA_B_ADDR             REG8_8(0x82)
#define VDMA_F2_P_ADDR          REG8_8(0x84)
#define VDMA_F2_WHP             REG8_8(0x85)
#define VDMA_F2_B_ADDR          REG8_8(0x86)

then
        reg_write(vc->dev, VDMA_CHANNEL_CONFIG[vc->ch], dma_cfg);
        reg_write(dev, SAT_U[ch], ctrl->val);
        reg_write(dev, SAT_V[ch], ctrl->val);

etc.
-- 
Krzysztof Halasa

Industrial Research Institute for Automation and Measurements PIAP
Al. Jerozolimskie 202, 02-486 Warsaw, Poland
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1169703 — Re: Coding style details (checkpatch)

FromJoe Perches <joe@perches.com>
Date2015-06-22 08:20 +0200
SubjectRe: Coding style details (checkpatch)
Message-ID<pE3fj-3OG-5@gated-at.bofh.it>
In reply to#1169697
On Mon, 2015-06-22 at 07:33 +0200, Krzysztof Hałasa wrote:
> Joe Perches <joe@perches.com> writes:
> 
> > How is the macro used?
> > #define REG8_1(a0) ((const u16[8]){a0, a0 + 1, a0 + 2, a0 + 3})
> 
> #define REG8_1(a0) ((const u16[8]){a0, a0 + 1, a0 + 2, a0 + 3, a0 + 4, a0 + 5, a0 + 6, a0 + 7})
[]
> #define VDMA_CHANNEL_CONFIG     REG8_1(0x10)
[]
>         reg_write(vc->dev, VDMA_CHANNEL_CONFIG[vc->ch], dma_cfg);
[]
> ERROR: space required before the open brace '{'
> +#define REG8_1(a0) ((const u16[8]){a0, a0 + 1, a0 + 2, a0 + 3})
> 
> Does this qualify as the "false positive"?

Probably, yes.  Is it worth fixing?  Probably not.

It might be better to use some base + index macro
as it could be smaller object code.

Something like:

#define REG_NO(base, multiplier, index)	(base + (multiplier * index))

	reg_write(vc->dev, REG_NO(0x10, 1, vc->ch), dma_cfg);
or

#define VDMA_CHANNEL_CONFIG	0x10

	reg_write(vc->dev, REG_NO(VDMA_CHANNEL_CONFIG, 1, vc->ch), dma_cfg);

etc...


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1169724 — Re: Coding style details (checkpatch)

Fromkhalasa@piap.pl (Krzysztof Hałasa)
Date2015-06-22 08:40 +0200
SubjectRe: Coding style details (checkpatch)
Message-ID<pE3yF-4bg-11@gated-at.bofh.it>
In reply to#1169703
Joe Perches <joe@perches.com> writes:

> It might be better to use some base + index macro
> as it could be smaller object code.
>
> Something like:
>
> #define REG_NO(base, multiplier, index)	(base + (multiplier * index))
>
> 	reg_write(vc->dev, REG_NO(0x10, 1, vc->ch), dma_cfg);
> or
>
> #define VDMA_CHANNEL_CONFIG	0x10
>
> 	reg_write(vc->dev, REG_NO(VDMA_CHANNEL_CONFIG, 1, vc->ch), dma_cfg);

Wouldn't work, the register map is a bit messy.
E.g.

#define DMA_PAGE_TABLE0_ADDR    ((const u16[8]){0x08, 0xD0, 0xD2, 0xD4, 0xD6, 0xD8, 0xDA, 0xDC})
#define DMA_PAGE_TABLE1_ADDR    ((const u16[8]){0x09, 0xD1, 0xD3, 0xD5, 0xD7, 0xD9, 0xDB, 0xDD})

also

#define VDREG8(a0) ((const u16[8]){                     \
        a0 + 0x000, a0 + 0x010, a0 +0x020, a0 + 0x030,  \
        a0 + 0x100, a0 + 0x110, a0 +0x120, a0 + 0x130})
#define VIDSTAT                 VDREG8(0x100)
#define BRIGHT                  VDREG8(0x101)
#define CONTRAST                VDREG8(0x102)

etc.

One would have to remember (writing .c code) which scheme applies to
which access. The tables, while probably less than optimal WRT CPU
cycles used, are consistent, and the addressing details are grouped in
one place - the *regs.h file.
-- 
Krzysztof Halasa

Industrial Research Institute for Automation and Measurements PIAP
Al. Jerozolimskie 202, 02-486 Warsaw, Poland
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1169736 — Re: Coding style details (checkpatch)

FromJoe Perches <joe@perches.com>
Date2015-06-22 08:50 +0200
SubjectRe: Coding style details (checkpatch)
Message-ID<pE3Im-4mu-15@gated-at.bofh.it>
In reply to#1169724
On Mon, 2015-06-22 at 08:38 +0200, Krzysztof Hałasa wrote:
> Joe Perches <joe@perches.com> writes:
> 
> > It might be better to use some base + index macro
> > as it could be smaller object code.
> >
> > Something like:
> >
> > #define REG_NO(base, multiplier, index)	(base + (multiplier * index))
> >
> > 	reg_write(vc->dev, REG_NO(0x10, 1, vc->ch), dma_cfg);
> > or
> >
> > #define VDMA_CHANNEL_CONFIG	0x10
> >
> > 	reg_write(vc->dev, REG_NO(VDMA_CHANNEL_CONFIG, 1, vc->ch), dma_cfg);
> 
> Wouldn't work, the register map is a bit messy.
> E.g.
> 
> #define DMA_PAGE_TABLE0_ADDR    ((const u16[8]){0x08, 0xD0, 0xD2, 0xD4, 0xD6, 0xD8, 0xDA, 0xDC})
> #define DMA_PAGE_TABLE1_ADDR    ((const u16[8]){0x09, 0xD1, 0xD3, 0xD5, 0xD7, 0xD9, 0xDB, 0xDD})

Erk, yes, a bit messy.

You could elide the 8 and checkpatch wouldn't emit a warning.

#define VDREG8(a0) ((const u16[]){			\
	a0 + 0x000, a0 + 0x010, a0 +0x020, a0 + 0x030,	\
	a0 + 0x100, a0 + 0x110, a0 +0x120, a0 + 0x130})

as "const u16[]" is a $Type but "const u16[<digits>]" is not.

Still, as written, the code seems fragile as MACRO[index]
allows index to be any value, maybe larger than the array.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web