Re: [PATCH v5 1/1] OMAP2: Spi: Force CS to be in inactive state after off-mode transition

[Kevin Hilman <khilman@deeprootsystems.com>]

Gregory CLEMENT <gregory.clement@free-electrons.com> 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.

> 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@free-electrons.com>
> ---

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