On Wed, Nov 24, 2010 at 04:49:47PM -0800, Kevin Hilman wrote:
> Gregory CLEMENT <gregory.clement@xxxxxxxxxxxxxxxxxx> writes:
>
> > As request by Grant Likely, there is no more cover letter. Full changelog is following.
> > I am still reluctant to add this changelog in the patch description, as it adds no value to
> > the patch itself: when it was needed I try to updat comments or patch description.
>
> You're right, the changelog should not be in the patch description.
> This bit of meta-description and changelog should go after the '---'
> just after your signoff. That way, git tools can still auto-apply the
> email, and git ignores stuff after the '---' so it doesn't end up in the
> git history.
Actually, I used to have the same opinion, but dwmw2 clued me in that
it really is valuable and proper to have the revision history and
changelog in the patch description. When looking back at what
actually got committed into Linus' tree, the changelog gives hints as
to which exact version of the patch got committed (for instance, if a
v6 got merged, but a v7 was also posted that didn't get merged.)
BTW, this thread has some discussion about this patch not actually
working correctly. What is the state? Should this version get
merged, or do I need to wait for a v6?
g.
>
> > I understand that Grant Likely would need an ack from other user as this patch fix a corner case.
> > Kevin Hilman made a few comments on this patch so he could add his "Ack by" or at least his "Review by".
>
> I haven't actually tested it, only reviewed it, so you can add a
> Reviewed by for me, but I'm not SPI-aware enough to ack this patch or
> test it thoroughly. Also, I have some more comments below...
>
> Also, in the last patch I suggested you do more of a save/restore of
> this value instead of a restore to a hard-coded value. IOW, save the
> value in the suspend method, restore it in resume. I thought you had
> agreed to that. I'm not an SPI expert, so not sure if it makes sense,
> but it seems more robust that way and more future proof.
>
> >
> > Changelog:
> > * Change from v1 to v2:
> > Rebase on linus/master (after 2.6.37-rc1)
> > Do some clean-up and fix indentation on both patches
> > Add more explanations for patch 2
> >
> > * Change from v2 to v3:
> > Use directly resume function of spi_master instead of using function
> > from spi_device as Grant Likely pointed it out.
> > Force this transition explicitly for each CS used by a device.
> >
> > * Change from v3 to v4:
> > Patch clean-up according to Kevin Hilman and checkpatch.
> > Now force CS to be in inactive state only if it was inactive when it was
> > suspended.
> >
> > * Change from v4 to v5:
> > Rebase on linus/master (after 2.6.37-rc3)
> > Collapse some lines as pointed by Grant Likely
> > Fix a spelling
> >
> >
> > == CUT HERE ==
> > When SPI wake up from OFF mode, CS is in the wrong state: force it to the inactive state.
> >
> > During the system life, I monitored the CS behavior using a oscilloscope.
> > I also activated debug in omap2_mcspi, so I saw when driver disable the clocks and restore context when device is not used.
> > Each time the CS was in the correct state.
> > It was only when system was put suspend to ram with off-mode activated that on resume the CS was in wrong state( ie activated).
> >
> > Signed-off-by: Gregory CLEMENT <gregory.clement@xxxxxxxxxxxxxxxxxx>
> > ---
>
> This is where any changelog and extra info for reviewers should go.
>
> > drivers/spi/omap2_mcspi.c | 33 +++++++++++++++++++++++++++++++++
> > 1 files changed, 33 insertions(+), 0 deletions(-)
> >
> > diff --git a/drivers/spi/omap2_mcspi.c b/drivers/spi/omap2_mcspi.c
> > index 2a651e6..dcc024a 100644
> > --- a/drivers/spi/omap2_mcspi.c
> > +++ b/drivers/spi/omap2_mcspi.c
> > @@ -1305,11 +1305,44 @@ static int __exit omap2_mcspi_remove(struct platform_device *pdev)
> > /* work with hotplug and coldplug */
> > MODULE_ALIAS("platform:omap2_mcspi");
> >
> > +#ifdef CONFIG_PM
> > +/* When SPI wake up from off-mode, CS is in activate state. If it was in
> > + * unactive state when driver was suspend, then force it to unactive state at
> > + * wake up.
> > + */
>
> please fix multi-line comment style. Search for 'multi-line' in
> CodingStyle.
>
> > +static int omap2_mcspi_resume(struct platform_device *pdev)
> > +{
> > + struct spi_master *master = dev_get_drvdata(&pdev->dev);
> > + struct omap2_mcspi *mcspi = spi_master_get_devdata(master);
> > + struct omap2_mcspi_cs *cs;
> > +
> > + omap2_mcspi_enable_clocks(mcspi);
> > + list_for_each_entry(cs, &omap2_mcspi_ctx[master->bus_num - 1].cs,
> > + node) {
> > + if ((cs->chconf0 & OMAP2_MCSPI_CHCONF_FORCE) == 0) {
> > +
> > + /* We need to toggle CS state for OMAP take this
> > + * change in account.
> > + */
>
> multi-line coding style
>
> > + MOD_REG_BIT(cs->chconf0, OMAP2_MCSPI_CHCONF_FORCE, 1);
> > + __raw_writel(cs->chconf0, cs->base + OMAP2_MCSPI_CHCONF0);
> > + MOD_REG_BIT(cs->chconf0, OMAP2_MCSPI_CHCONF_FORCE, 0);
> > + __raw_writel(cs->chconf0, cs->base + OMAP2_MCSPI_CHCONF0);
> > + }
> > + }
> > + omap2_mcspi_disable_clocks(mcspi);
> > + return 0;
> > +}
> > +#else
> > +#define omap2_mcspi_resume NULL
> > +#endif
> > +
> > static struct platform_driver omap2_mcspi_driver = {
> > .driver = {
> > .name = "omap2_mcspi",
> > .owner = THIS_MODULE,
> > },
> > + .resume = omap2_mcspi_resume,
> > .remove = __exit_p(omap2_mcspi_remove),
> > };
> > -- 1.7.0.4
>
> Kevin
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
