[lkml]   [2011]   [Mar]   [2]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
Messages in this thread
    SubjectRe: [PATCHv1] ARM: imx: Add support for low power suspend on MX51.

    On Wed, 2 Mar 2011, Uwe Kleine-König wrote:
    > On Wed, Mar 02, 2011 at 11:17:58AM -0600, wrote:
    > > From: Dinh Nguyen <>

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

    > > + ccm_clpcr |= (0x3 << MXC_CCM_CLPCR_STBY_COUNT_OFFSET);
    > the parentheses aren't needed here

    Could you finally provide a patch to 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.

    > > + ccm_clpcr |= (0x1 << MXC_CCM_CLPCR_LPM_OFFSET);
    > 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

    Again, I'd like to have a pony.

    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.

    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?

    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.

    Just for the record. The obvious bug with this code is this:

    > > + plat_lpc |= MXC_CORTEXA8_PLAT_LPC_DSM
    > > + arm_srpgcr |= MXC_SRPGCR_PCR;
    > > +
    > > + __raw_writel(plat_lpc, MXC_CORTEXA8_PLAT_LPC);
    > > + __raw_writel(ccm_clpcr, MXC_CCM_CLPCR);
    > > + __raw_writel(arm_srpgcr, MXC_SRPG_ARM_SRPGCR);
    > > + __raw_writel(arm_srpgcr, MXC_SRPG_NEON_SRPGCR);
    > > +
    > > + if (tzic_enable_wake(0) != 0)
    > > + return -EAGAIN;

    It happily returns here with -EAGAIN w/o undoing the already committed
    changes which are preparatory to the tzic_enable_wake() check.

    I might be wrong as usual, but if this is cleaned up by the calling
    code magically then the "return -EINVAL"; lacks a big fat
    comment. While such an omission is not a triggerable bug it documents
    the lack of tought and taste by creating asymetric mechanisms w/o the
    courtesy to document them properly. Even if documented, asymetric
    interfaces are crap most of the time.

    > > + cpu_do_idle();
    > > + return 0;
    > > +}


     \ /
      Last update: 2011-03-03 00:53    [W:0.056 / U:38.056 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site