[U-Boot] Checkpatch warnings for "volatile"

[U-Boot] Checkpatch warnings for "volatile"

[Prabhakar Lad]

Hi Wolfgang,

The checkpatch complains for volatile  keyword, if "volatile" is necessary
and cannot be removed,
Is it necessary to inform in  the cover letter or the patch itself stating
that these warnings should be neglected ?

Regards
--Prabhakar Lad

[U-Boot] Checkpatch warnings for "volatile"

[Prabhakar Lad]

any one?

Regards
--Prabhakar Lad

On Wed, Oct 12, 2011 at 4:05 PM, Prabhakar Lad
<prabhakar.csengg@gmail.com>wrote:

> Hi Wolfgang,
>
> The checkpatch complains for volatile  keyword, if "volatile" is necessary
> and cannot be removed,
> Is it necessary to inform in  the cover letter or the patch itself stating
> that these warnings should be neglected ?
>
> Regards
> --Prabhakar Lad

[U-Boot] Checkpatch warnings for "volatile"

[Jason]

Prabhakar,

On Wed, Oct 12, 2011 at 04:05:57PM +0530, Prabhakar Lad wrote:
> The checkpatch complains for volatile  keyword,

Just to make sure [1], since there is a lot of misunderstanding regarding
volatile.

> if "volatile" is necessary and cannot be removed,

Please see [2], people are more inclined to help if you provide the code
and the warning.  Stating what research you've already done is helpful
as well.

> Is it necessary to inform in  the cover letter or the patch itself
> stating that these warnings should be neglected ?

Assuming you've read [1] and [2] and it is indeed a valid use of
volatile, I would put it in the comment area of the patch email, eg:

---

Notes:

	1.) checkpatch.pl complained about volatile in source.c:312.  This is
       a register we bitbang in a loop and we don't want the compiler
       optimizing it out.

diff...


hth,

Jason.

[1]
http://www.mjmwired.net/kernel/Documentation/volatile-considered-harmful.txt
[2] http://catb.org/~esr/faqs/smart-questions.html

[U-Boot] Checkpatch warnings for "volatile"

[Wolfgang Denk]

Dear Jason,

In message <20111014131245.GF7102@titan.lakedaemon.net> you wrote:
> 
> 	1.) checkpatch.pl complained about volatile in source.c:312.  This is
>        a register we bitbang in a loop and we don't want the compiler
>        optimizing it out.

This would, in almost all cases, trigger a NAK due to the fact that
device register accesses should be done through I/O accesors, and
never through volatile pointers.

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
A quarrel is quickly settled when deserted by one party; there is  no
battle unless there be two.                                  - Seneca

[U-Boot] Checkpatch warnings for "volatile"

[Jason]

Wolfgang,

On Fri, Oct 14, 2011 at 09:58:44PM +0200, Wolfgang Denk wrote:
> Dear Jason,
> 
> In message <20111014131245.GF7102@titan.lakedaemon.net> you wrote:
> > 
> > 	1.) checkpatch.pl complained about volatile in source.c:312.  This is
> >        a register we bitbang in a loop and we don't want the compiler
> >        optimizing it out.
> 
> This would, in almost all cases, trigger a NAK due to the fact that
> device register accesses should be done through I/O accesors, and
> never through volatile pointers.

very true, I was attempting to give a generic example.  Sorry if that
led to any confusion.

thx,

Jason.

[U-Boot] Checkpatch warnings for "volatile"

[Wolfgang Denk]

Dear Jason,

In message <20111014202224.GI7102@titan.lakedaemon.net> you wrote:
> 
> > > 	1.) checkpatch.pl complained about volatile in source.c:312.  This is
> > >        a register we bitbang in a loop and we don't want the compiler
> > >        optimizing it out.
> > 
> > This would, in almost all cases, trigger a NAK due to the fact that
> > device register accesses should be done through I/O accesors, and
> > never through volatile pointers.
> 
> very true, I was attempting to give a generic example.  Sorry if that
> led to any confusion.

I've explained this a number of times recently - there are actually
very, very few occasions where "volatile" actually makes sense.

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
Conceptual integrity in turn dictates that the  design  must  proceed
from  one  mind,  or  from  a  very small number of agreeing resonant
minds.               - Frederick Brooks Jr., "The Mythical Man Month" 

[U-Boot] Checkpatch warnings for "volatile"

[Prabhakar Lad]

Hi Wolfgang,

On Sat, Oct 15, 2011 at 2:54 AM, Wolfgang Denk <wd@denx.de> wrote:

> Dear Jason,
>
> In message <20111014202224.GI7102@titan.lakedaemon.net> you wrote:
> >
> > > >   1.) checkpatch.pl complained about volatile in source.c:312.  This
> is
> > > >        a register we bitbang in a loop and we don't want the compiler
> > > >        optimizing it out.
> > >
> > > This would, in almost all cases, trigger a NAK due to the fact that
> > > device register accesses should be done through I/O accesors, and
> > > never through volatile pointers.
> >
> > very true, I was attempting to give a generic example.  Sorry if that
> > led to any confusion.
>
> I've explained this a number of times recently - there are actually
> very, very few occasions where "volatile" actually makes sense.
>
>      Agreed, but I see a piece of code where virtual address are compared.
     For example in arch/arm/cpu/arm926ejs/davinci/cpu.c
     In this function static inline unsigned pll_prediv(unsigned pllbase)
and
     also in this static inline unsigned pll_postdiv(unsigned pllbase)

     Any suggestion on this on how to tackle or let it remain stagnant?

   Regards
   -- Prabhakar Lad


> Best regards,
>
> Wolfgang Denk
>
> --
> DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
> HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
> Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
> Conceptual integrity in turn dictates that the  design  must  proceed
> from  one  mind,  or  from  a  very small number of agreeing resonant
> minds.               - Frederick Brooks Jr., "The Mythical Man Month"
>

[U-Boot] Checkpatch warnings for "volatile"

[Wolfgang Denk]

Dear Prabhakar Lad,

In message <CA+V-a8sYRZJDZojEpQ55ZGRZ6--Niq0ThKVV8e_RtQrRuShE8A@mail.gmail.com> you wrote:
> 
> > I've explained this a number of times recently - there are actually
> > very, very few occasions where "volatile" actually makes sense.
> >
> >      Agreed, but I see a piece of code where virtual address are compared.
>      For example in arch/arm/cpu/arm926ejs/davinci/cpu.c
>      In this function static inline unsigned pll_prediv(unsigned pllbase)
> and
>      also in this static inline unsigned pll_postdiv(unsigned pllbase)
> 
>      Any suggestion on this on how to tackle or let it remain stagnant?

I cannot see a justification for any of the ""volatile" in this file.

Of course, all these ugly REG() calls should be converted to proper
use of I/O accessors.

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
Hindsight is an exact science.

[U-Boot] Checkpatch warnings for "volatile"

[Prabhakar Lad]

Hi Wolfgang

On 10/15/11, Wolfgang Denk <wd@denx.de> wrote:
> Dear Prabhakar Lad,
>
> In message
> <CA+V-a8sYRZJDZojEpQ55ZGRZ6--Niq0ThKVV8e_RtQrRuShE8A@mail.gmail.com> you
> wrote:
>>
>> > I've explained this a number of times recently - there are actually
>> > very, very few occasions where "volatile" actually makes sense.
>> >
>> >      Agreed, but I see a piece of code where virtual address are
>> > compared.
>>      For example in arch/arm/cpu/arm926ejs/davinci/cpu.c
>>      In this function static inline unsigned pll_prediv(unsigned pllbase)
>> and
>>      also in this static inline unsigned pll_postdiv(unsigned pllbase)
>>
>>      Any suggestion on this on how to tackle or let it remain stagnant?
>
> I cannot see a justification for any of the ""volatile" in this file.
>
> Of course, all these ugly REG() calls should be converted to proper
> use of I/O accessors.
>
  Thanks for the reply.

Regards
--Prabhakar Lad

> Best regards,
>
> Wolfgang Denk
>
> --
> DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
> HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
> Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
> Hindsight is an exact science.
>

[U-Boot] Checkpatch warnings for "volatile"

[Marek Vasut]

On Saturday, October 15, 2011 10:56:54 AM Wolfgang Denk wrote:
> Dear Prabhakar Lad,
> 
> In message <CA+V-a8sYRZJDZojEpQ55ZGRZ6--
Niq0ThKVV8e_RtQrRuShE8A@mail.gmail.com> you wrote:
> > > I've explained this a number of times recently - there are actually
> > > very, very few occasions where "volatile" actually makes sense.
> > > 
> > >      Agreed, but I see a piece of code where virtual address are
> > >      compared.
> >      
> >      For example in arch/arm/cpu/arm926ejs/davinci/cpu.c
> >      In this function static inline unsigned pll_prediv(unsigned pllbase)
> > 
> > and
> > 
> >      also in this static inline unsigned pll_postdiv(unsigned pllbase)
> >      
> >      Any suggestion on this on how to tackle or let it remain stagnant?
> 
> I cannot see a justification for any of the ""volatile" in this file.
> 
> Of course, all these ugly REG() calls should be converted to proper
> use of I/O accessors.

Definitelly ... but I'm not swiping this one, I have enough mess on my hands 
already ;-)

Cheers