Messages in this thread | | | Date | Thu, 3 Mar 2011 10:35:32 +0100 | From | Uwe Kleine-König <> | Subject | Re: [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/
| |