Coding style details

Coding style details

[Krzysztof Hałasa]

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

Re: Coding style details

[Frans Klaver]

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

Re: Coding style details (checkpatch)

[Krzysztof Hałasa]

Frans Klaver <fransklaver@gmail.com> writes:

>> #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.

Thanks. That's what I thought. It looks that checkpatch doesn't like
this:

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"?
-- 
Krzysztof Halasa

Industrial Research Institute for Automation and Measurements PIAP
Al. Jerozolimskie 202, 02-486 Warsaw, Poland

Re: Coding style details (checkpatch)

[Frans Klaver]

On Fri, Jun 19, 2015 at 12:31 PM, Krzysztof Hałasa <khalasa@piap.pl> wrote:
> Frans Klaver <fransklaver@gmail.com> writes:
>
>>> #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.
>
> Thanks. That's what I thought. It looks that checkpatch doesn't like
> this:
>
> 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"?

Ah, right. One might say that this is a false positive, but that's up
to Joe or Andy I guess.

It may be valid C code, but I think this construction is slightly
funky to begin with.

Which file is this?

Frans

Re: Coding style details (checkpatch)

[Krzysztof Hałasa]

Frans Klaver <fransklaver@gmail.com> writes:

> Ah, right. One might say that this is a false positive, but that's up
> to Joe or Andy I guess.
>
> It may be valid C code, but I think this construction is slightly
> funky to begin with.
>
> Which file is this?

A new file, not yet sent anywhere.
-- 
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/

Re: Coding style details (checkpatch)

[Joe Perches]

On Fri, 2015-06-19 at 12:52 +0200, Krzysztof Hałasa wrote:
> Frans Klaver <fransklaver@gmail.com> writes:
> 
> > Ah, right. One might say that this is a false positive, but that's up
> > to Joe or Andy I guess.
> >
> > It may be valid C code, but I think this construction is slightly
> > funky to begin with.
> >
> > Which file is this?
> 
> A new file, not yet sent anywhere.

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


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

Re: Coding style details (checkpatch)

[Krzysztof Hałasa]

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/

Re: Coding style details (checkpatch)

[Joe Perches]

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/

Re: Coding style details (checkpatch)

[Krzysztof Hałasa]

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/

Re: Coding style details (checkpatch)

[Joe Perches]

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/

Re: Coding style details (checkpatch)

[Krzysztof Hałasa]

Joe Perches <joe@perches.com> writes:

> #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.

Right.
XXX[8] is meant as an additional mental check. Not very effective,
though I think certain GCCs can issue a warning for obvious
out-of-bounds accesses (probably with both [] and [8]).
-- 
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/