* Coding style details
@ 2015-06-19 8:35 Krzysztof Hałasa
2015-06-19 9:07 ` Frans Klaver
0 siblings, 1 reply; 11+ messages in thread
From: Krzysztof Hałasa @ 2015-06-19 8:35 UTC (permalink / raw)
To: lkml
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
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: Coding style details
2015-06-19 8:35 Coding style details Krzysztof Hałasa
@ 2015-06-19 9:07 ` Frans Klaver
2015-06-19 10:31 ` Coding style details (checkpatch) Krzysztof Hałasa
0 siblings, 1 reply; 11+ messages in thread
From: Frans Klaver @ 2015-06-19 9:07 UTC (permalink / raw)
To: Krzysztof Hałasa; +Cc: lkml
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
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: Coding style details (checkpatch)
2015-06-19 9:07 ` Frans Klaver
@ 2015-06-19 10:31 ` Krzysztof Hałasa
2015-06-19 10:37 ` Frans Klaver
0 siblings, 1 reply; 11+ messages in thread
From: Krzysztof Hałasa @ 2015-06-19 10:31 UTC (permalink / raw)
To: Frans Klaver; +Cc: Andy Whitcroft, Joe Perches, lkml
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
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: Coding style details (checkpatch)
2015-06-19 10:31 ` Coding style details (checkpatch) Krzysztof Hałasa
@ 2015-06-19 10:37 ` Frans Klaver
2015-06-19 10:52 ` Krzysztof Hałasa
0 siblings, 1 reply; 11+ messages in thread
From: Frans Klaver @ 2015-06-19 10:37 UTC (permalink / raw)
To: Krzysztof Hałasa; +Cc: Andy Whitcroft, Joe Perches, lkml
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
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: Coding style details (checkpatch)
2015-06-19 10:37 ` Frans Klaver
@ 2015-06-19 10:52 ` Krzysztof Hałasa
2015-06-19 14:54 ` Joe Perches
0 siblings, 1 reply; 11+ messages in thread
From: Krzysztof Hałasa @ 2015-06-19 10:52 UTC (permalink / raw)
To: Frans Klaver; +Cc: Andy Whitcroft, Joe Perches, lkml
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/
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: Coding style details (checkpatch)
2015-06-19 10:52 ` Krzysztof Hałasa
@ 2015-06-19 14:54 ` Joe Perches
2015-06-22 5:33 ` Krzysztof Hałasa
0 siblings, 1 reply; 11+ messages in thread
From: Joe Perches @ 2015-06-19 14:54 UTC (permalink / raw)
To: Krzysztof Hałasa; +Cc: Frans Klaver, Andy Whitcroft, lkml
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/
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: Coding style details (checkpatch)
2015-06-19 14:54 ` Joe Perches
@ 2015-06-22 5:33 ` Krzysztof Hałasa
2015-06-22 6:12 ` Joe Perches
0 siblings, 1 reply; 11+ messages in thread
From: Krzysztof Hałasa @ 2015-06-22 5:33 UTC (permalink / raw)
To: Joe Perches; +Cc: Frans Klaver, Andy Whitcroft, lkml
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/
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: Coding style details (checkpatch)
2015-06-22 5:33 ` Krzysztof Hałasa
@ 2015-06-22 6:12 ` Joe Perches
2015-06-22 6:38 ` Krzysztof Hałasa
0 siblings, 1 reply; 11+ messages in thread
From: Joe Perches @ 2015-06-22 6:12 UTC (permalink / raw)
To: Krzysztof Hałasa; +Cc: Frans Klaver, Andy Whitcroft, lkml
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/
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: Coding style details (checkpatch)
2015-06-22 6:12 ` Joe Perches
@ 2015-06-22 6:38 ` Krzysztof Hałasa
2015-06-22 6:47 ` Joe Perches
0 siblings, 1 reply; 11+ messages in thread
From: Krzysztof Hałasa @ 2015-06-22 6:38 UTC (permalink / raw)
To: Joe Perches; +Cc: Frans Klaver, Andy Whitcroft, lkml
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/
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: Coding style details (checkpatch)
2015-06-22 6:38 ` Krzysztof Hałasa
@ 2015-06-22 6:47 ` Joe Perches
2015-06-22 7:39 ` Krzysztof Hałasa
0 siblings, 1 reply; 11+ messages in thread
From: Joe Perches @ 2015-06-22 6:47 UTC (permalink / raw)
To: Krzysztof Hałasa; +Cc: Frans Klaver, Andy Whitcroft, lkml
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/
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: Coding style details (checkpatch)
2015-06-22 6:47 ` Joe Perches
@ 2015-06-22 7:39 ` Krzysztof Hałasa
0 siblings, 0 replies; 11+ messages in thread
From: Krzysztof Hałasa @ 2015-06-22 7:39 UTC (permalink / raw)
To: Joe Perches; +Cc: Frans Klaver, Andy Whitcroft, lkml
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/
^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2015-06-22 7:39 UTC | newest]
Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-06-19 8:35 Coding style details Krzysztof Hałasa
2015-06-19 9:07 ` Frans Klaver
2015-06-19 10:31 ` Coding style details (checkpatch) Krzysztof Hałasa
2015-06-19 10:37 ` Frans Klaver
2015-06-19 10:52 ` Krzysztof Hałasa
2015-06-19 14:54 ` Joe Perches
2015-06-22 5:33 ` Krzysztof Hałasa
2015-06-22 6:12 ` Joe Perches
2015-06-22 6:38 ` Krzysztof Hałasa
2015-06-22 6:47 ` Joe Perches
2015-06-22 7:39 ` Krzysztof Hałasa
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).