lkml.org 
[lkml]   [2011]   [Mar]   [3]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCHv1] ARM: imx: Add support for low power suspend on MX51.
Guten Morgen Thomas,

On Thu, Mar 03, 2011 at 12:51:32AM +0100, Thomas Gleixner wrote:
> Uwe,
>
> On Wed, 2 Mar 2011, Uwe Kleine-König wrote:
> > On Wed, Mar 02, 2011 at 11:17:58AM -0600, Dinh.Nguyen@freescale.com wrote:
> > > From: Dinh Nguyen <Dinh.Nguyen@freescale.com>
>
> > > --- /dev/null
> > > +++ b/arch/arm/mach-mx5/pm.c
> > I'd like to have that called pm-imx51.c
>
> And I'd like to have a pony.
http://www.carl-russ-schule.de/files/Pony.jpg

>
> > > + ccm_clpcr |= (0x3 << MXC_CCM_CLPCR_STBY_COUNT_OFFSET);
> > the parentheses aren't needed here
>
> Could you finally provide a patch to checkpatch.pl or git commit which
> resolves that issue once and forever ?
>
> Not to mention the fact, that those parentheses are not disturbing the
> readability of that code at all.
So it seems we're different.

>
> > > + ccm_clpcr |= (0x1 << MXC_CCM_CLPCR_LPM_OFFSET);
> > ditto
>
> Ditto.
>
> > > +static int __init mx5_pm_init(void)
> > I'd prefer to have that called by imx51_init_early.
>
> And the reason is?
>
> 1) your personal preference
> 2) there is some useful technical reason
>
> If #1, then this comment was just waste of electrons
> If #2, you failed to provide some reasonable explanation
Actually it's #2, and to quote a different review[1]:

Reviewers hint to a correct solution and you are supposed to
lookup what that solution means and act accordingly. If you do
not understand the hint or its implications please ask [...]

> Again, I'd like to have a pony.
http://h-6.abload.de/img/pony01_pixelquelle_dal8701.jpg

> Seriously, while all of us admire your invaluable skills of running
> scripts over patches and kernel code, that kind of review you are
> trying to provide is utterly useless.
actually it's not a script, but I guess that doesn't matter much.

> 1) The patch itself has been questioned about its correctness hours
> before you added the output of your secret script. It was already
> reported to be non functional. So what's the value of adding
> scriptable review to it?
It might save the patch sender from a third iteration.

> 2) As long as you do not see the most obvious functional problems with
> a patch please spare your script computing power and the bandwidth
> you are consuming by your futile attempts to gain a profile as a
> patch reviewer.
The functionallity of the patch has been questioned before so I didn't
see a value in repeating that :-) To be honest, I didn't check for
functional problems, but IMHO it's OK to provide feedback about a part
of the problems if you don't see all of them.

Best regards
Uwe

[1] http://article.gmane.org/gmane.linux.kernel/1107265

--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | http://www.pengutronix.de/ |
--
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/

\
 
 \ /
  Last update: 2011-03-03 10:39    [W:0.093 / U:0.372 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site