Re: [PATCH] ASoC: arizona: Add delay for output disable

Re: [PATCH] ASoC: arizona: Add delay for output disable

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 930 bytes --]

On Mon, Jan 19, 2015 at 03:49:46PM +0000, Charles Keepax wrote:

> +	case SND_SOC_DAPM_POST_PMD:
> +		switch (w->shift) {
> +		case ARIZONA_OUT1L_ENA_SHIFT:
> +		case ARIZONA_OUT1R_ENA_SHIFT:
> +		case ARIZONA_OUT2L_ENA_SHIFT:
> +		case ARIZONA_OUT2R_ENA_SHIFT:
> +		case ARIZONA_OUT3L_ENA_SHIFT:
> +		case ARIZONA_OUT3R_ENA_SHIFT:
> +			udelay(750);
> +			break;

That's a really quite long udelay() especially given that as the code is
written it's going to be cumalative over all the outputs being torn down
so typically will be 1.5ms for headphones (not sure if that's the
intention or not).  msleep() would be more friendly than udelay() and
it'd also be good to arrange to coalesce the delays.  Perhaps set a flag
in _PRE_PMD and check if a delay is needed in the clock teardown?  Or
better yet record a timestamp and then figure out if a delay is needed
when tearing down the clock though that is probably overengineering.

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH] ASoC: arizona: Add delay for output disable

[Charles Keepax]

On Mon, Jan 19, 2015 at 04:24:32PM +0000, Mark Brown wrote:
> On Mon, Jan 19, 2015 at 03:49:46PM +0000, Charles Keepax wrote:
> 
> > +	case SND_SOC_DAPM_POST_PMD:
> > +		switch (w->shift) {
> > +		case ARIZONA_OUT1L_ENA_SHIFT:
> > +		case ARIZONA_OUT1R_ENA_SHIFT:
> > +		case ARIZONA_OUT2L_ENA_SHIFT:
> > +		case ARIZONA_OUT2R_ENA_SHIFT:
> > +		case ARIZONA_OUT3L_ENA_SHIFT:
> > +		case ARIZONA_OUT3R_ENA_SHIFT:
> > +			udelay(750);
> > +			break;
> 
> That's a really quite long udelay() especially given that as the code is
> written it's going to be cumalative over all the outputs being torn down
> so typically will be 1.5ms for headphones (not sure if that's the
> intention or not).  msleep() would be more friendly than udelay() and
> it'd also be good to arrange to coalesce the delays.  Perhaps set a flag
> in _PRE_PMD and check if a delay is needed in the clock teardown?  Or
> better yet record a timestamp and then figure out if a delay is needed
> when tearing down the clock though that is probably overengineering.

Similar to the power up the delay needs to be cumulative as the
write sequences will queue themselves up if the last one is still
running when the next one is started. It is fairly long for a
udelay I will replace with an msleep(1) instead.

Thanks,
Charles

Re: [PATCH] ASoC: arizona: Add delay for output disable

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 709 bytes --]

On Mon, Jan 19, 2015 at 04:58:50PM +0000, Charles Keepax wrote:

> Similar to the power up the delay needs to be cumulative as the
> write sequences will queue themselves up if the last one is still
> running when the next one is started. It is fairly long for a
> udelay I will replace with an msleep(1) instead.

Given that you are needing to delay per output it's probably going to be
better to do the coalescing of the delays - record how much delay is
going to be needed when stopping the clock then do that at once when
doing that.  It's going to be more friendly for the rest of the system
to see a single delay than to delay, schedule the thread again and then
almost immediately enter another delay.

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH] ASoC: arizona: Add delay for output disable

[Charles Keepax]

On Mon, Jan 19, 2015 at 05:07:06PM +0000, Mark Brown wrote:
> On Mon, Jan 19, 2015 at 04:58:50PM +0000, Charles Keepax wrote:
> 
> > Similar to the power up the delay needs to be cumulative as the
> > write sequences will queue themselves up if the last one is still
> > running when the next one is started. It is fairly long for a
> > udelay I will replace with an msleep(1) instead.
> 
> Given that you are needing to delay per output it's probably going to be
> better to do the coalescing of the delays - record how much delay is
> going to be needed when stopping the clock then do that at once when
> doing that.  It's going to be more friendly for the rest of the system
> to see a single delay than to delay, schedule the thread again and then
> almost immediately enter another delay.

That makes sense I will look at adding that.

Thanks,
Charles