Re: [PATCH 3/6] ASoC: ux500: Drop pinctrl sleep support

[Fabio Baltieri <fabio.baltieri@linaro.org>]

On Wed, May 08, 2013 at 09:07:08AM +0100, Lee Jones wrote:
> On Wed, 08 May 2013, Fabio Baltieri wrote:
> 
> > Drop pinctrl default/sleep state switching code, as it was breaking the
> > capture interface by putting the I2S pins in hi-z mode regardless of its
> > usage status, and not giving any real benefit.
> > 
> > Pinctrl default mode configuration is already managed automatically by a
> > specific pinctrl hog.
> 
> I'm sure we should support pinctrl though shouldn't we?
> 
> Is there no way of fixing the implementation instead of ripping it out?

Yes, but requesting the default pinctrl configuration should be enough,
and as those pins are shared with multiple device ids, a "hog"
configuration should be the cleanest.

Actually I asked Linus an opinion before doing this, so maybe he can ack
this patch or suggest a better way of doing this, such as declaring the
same pins for multiple device ids, but I'm not sure that would work as
expected.

Thanks,
Fabio

> > Signed-off-by: Fabio Baltieri <fabio.baltieri@linaro.org>
> > ---
> >  sound/soc/ux500/ux500_msp_i2s.c | 56 ++---------------------------------------
> >  sound/soc/ux500/ux500_msp_i2s.h |  6 -----
> >  2 files changed, 2 insertions(+), 60 deletions(-)
> > 
> > diff --git a/sound/soc/ux500/ux500_msp_i2s.c b/sound/soc/ux500/ux500_msp_i2s.c
> > index 964cfd6..8512c78 100644
> > --- a/sound/soc/ux500/ux500_msp_i2s.c
> > +++ b/sound/soc/ux500/ux500_msp_i2s.c
> > @@ -15,7 +15,6 @@
> >  
> >  #include <linux/module.h>
> >  #include <linux/platform_device.h>
> > -#include <linux/pinctrl/consumer.h>
> >  #include <linux/delay.h>
> >  #include <linux/slab.h>
> >  #include <linux/io.h>
> > @@ -28,9 +27,6 @@
> >  
> >  #include "ux500_msp_i2s.h"
> >  
> > -/* MSP1/3 Tx/Rx usage protection */
> > -static DEFINE_SPINLOCK(msp_rxtx_lock);
> > -
> >   /* Protocol desciptors */
> >  static const struct msp_protdesc prot_descs[] = {
> >  	{ /* I2S */
> > @@ -358,24 +354,8 @@ static int configure_multichannel(struct ux500_msp *msp,
> >  
> >  static int enable_msp(struct ux500_msp *msp, struct ux500_msp_config *config)
> >  {
> > -	int status = 0, retval = 0;
> > +	int status = 0;
> >  	u32 reg_val_DMACR, reg_val_GCR;
> > -	unsigned long flags;
> > -
> > -	/* Check msp state whether in RUN or CONFIGURED Mode */
> > -	if (msp->msp_state == MSP_STATE_IDLE) {
> > -		spin_lock_irqsave(&msp_rxtx_lock, flags);
> > -		if (msp->pinctrl_rxtx_ref == 0 &&
> > -			!(IS_ERR(msp->pinctrl_p) || IS_ERR(msp->pinctrl_def))) {
> > -			retval = pinctrl_select_state(msp->pinctrl_p,
> > -						msp->pinctrl_def);
> > -			if (retval)
> > -				pr_err("could not set MSP defstate\n");
> > -		}
> > -		if (!retval)
> > -			msp->pinctrl_rxtx_ref++;
> > -		spin_unlock_irqrestore(&msp_rxtx_lock, flags);
> > -	}
> >  
> >  	/* Configure msp with protocol dependent settings */
> >  	configure_protocol(msp, config);
> > @@ -632,8 +612,7 @@ int ux500_msp_i2s_trigger(struct ux500_msp *msp, int cmd, int direction)
> >  
> >  int ux500_msp_i2s_close(struct ux500_msp *msp, unsigned int dir)
> >  {
> > -	int status = 0, retval = 0;
> > -	unsigned long flags;
> > +	int status = 0;
> >  
> >  	dev_dbg(msp->dev, "%s: Enter (dir = 0x%01x).\n", __func__, dir);
> >  
> > @@ -645,18 +624,6 @@ int ux500_msp_i2s_close(struct ux500_msp *msp, unsigned int dir)
> >  			       (~(FRAME_GEN_ENABLE | SRG_ENABLE))),
> >  			      msp->registers + MSP_GCR);
> >  
> > -		spin_lock_irqsave(&msp_rxtx_lock, flags);
> > -		WARN_ON(!msp->pinctrl_rxtx_ref);
> > -		msp->pinctrl_rxtx_ref--;
> > -		if (msp->pinctrl_rxtx_ref == 0 &&
> > -			!(IS_ERR(msp->pinctrl_p) || IS_ERR(msp->pinctrl_sleep))) {
> > -			retval = pinctrl_select_state(msp->pinctrl_p,
> > -						msp->pinctrl_sleep);
> > -			if (retval)
> > -				pr_err("could not set MSP sleepstate\n");
> > -		}
> > -		spin_unlock_irqrestore(&msp_rxtx_lock, flags);
> > -
> >  		writel(0, msp->registers + MSP_GCR);
> >  		writel(0, msp->registers + MSP_TCF);
> >  		writel(0, msp->registers + MSP_RCF);
> > @@ -745,25 +712,6 @@ int ux500_msp_i2s_init_msp(struct platform_device *pdev,
> >  	dev_dbg(&pdev->dev, "I2S device-name: '%s'\n", i2s_cont->name);
> >  	msp->i2s_cont = i2s_cont;
> >  
> > -	msp->pinctrl_p = pinctrl_get(msp->dev);
> > -	if (IS_ERR(msp->pinctrl_p))
> > -		dev_err(&pdev->dev, "could not get MSP pinctrl\n");
> > -	else {
> > -		msp->pinctrl_def = pinctrl_lookup_state(msp->pinctrl_p,
> > -						PINCTRL_STATE_DEFAULT);
> > -		if (IS_ERR(msp->pinctrl_def)) {
> > -			dev_err(&pdev->dev,
> > -				"could not get MSP defstate (%li)\n",
> > -				PTR_ERR(msp->pinctrl_def));
> > -		}
> > -		msp->pinctrl_sleep = pinctrl_lookup_state(msp->pinctrl_p,
> > -						PINCTRL_STATE_SLEEP);
> > -		if (IS_ERR(msp->pinctrl_sleep))
> > -			dev_err(&pdev->dev,
> > -				"could not get MSP idlestate (%li)\n",
> > -				PTR_ERR(msp->pinctrl_def));
> > -	}
> > -
> >  	return 0;
> >  }
> >  
> > diff --git a/sound/soc/ux500/ux500_msp_i2s.h b/sound/soc/ux500/ux500_msp_i2s.h
> > index 1311c0d..1ce336f 100644
> > --- a/sound/soc/ux500/ux500_msp_i2s.h
> > +++ b/sound/soc/ux500/ux500_msp_i2s.h
> > @@ -530,12 +530,6 @@ struct ux500_msp {
> >  	int loopback_enable;
> >  	u32 backup_regs[MAX_MSP_BACKUP_REGS];
> >  	unsigned int f_bitclk;
> > -	/* Pin modes */
> > -	struct pinctrl *pinctrl_p;
> > -	struct pinctrl_state *pinctrl_def;
> > -	struct pinctrl_state *pinctrl_sleep;
> > -	/* Reference Count */
> > -	int pinctrl_rxtx_ref;
> >  };
> >  
> >  struct ux500_msp_dma_params {

-- 
Fabio Baltieri