Re: [PATCH 2/2 v3] sound/soc: mpc5200_psc_ac97: Use gpio pins for cold reset

[Grant Likely]

On Mon, Jun 14, 2010 at 3:55 PM, Eric Millbrandt
<emillbra...@dekaresearch.com> wrote:
> Call the gpio reset platform function instead of using the flawed
> ac97 functionality of the MPC5200(b)
>
> From MPC5200B User's Manual:
> "Some AC97 devices goes to a test mode, if the Sync line is high
> during the Res line is low (reset phase). To avoid this behavior the
> Sync line must be also forced to zero during the reset phase. To do
> that, the pin muxing should switch to GPIO mode and the GPIO control
> register should be used to control the output lines."
>
> Signed-off-by: Eric Millbrandt <emillbra...@dekaresearch.com>
> ---
>
> changes since v1
> - Amended with comments from Mark Brown
> - Fall back to the original reset implementation if no gpio pins are defined
>  in the device tree
>
> changes since v2
> - Refactored to move the port_config manipulation to platform code.
> - Remove the gpio pins from the device-tree
>
>  sound/soc/fsl/mpc5200_psc_ac97.c |   33 +++++++++++++++++++++++++++++----
>  1 files changed, 29 insertions(+), 4 deletions(-)
>
> diff --git a/sound/soc/fsl/mpc5200_psc_ac97.c 
> b/sound/soc/fsl/mpc5200_psc_ac97.c
> index e2ee220..380a127 100644
> --- a/sound/soc/fsl/mpc5200_psc_ac97.c
> +++ b/sound/soc/fsl/mpc5200_psc_ac97.c
> @@ -20,6 +20,7 @@
>
>  #include <asm/time.h>
>  #include <asm/delay.h>
> +#include <asm/mpc52xx.h>
>  #include <asm/mpc52xx_psc.h>
>
>  #include "mpc5200_dma.h"
> @@ -100,19 +101,43 @@ static void psc_ac97_warm_reset(struct snd_ac97 *ac97)
>  {
>        struct mpc52xx_psc __iomem *regs = psc_dma->psc_regs;
>
> +       mutex_lock(&psc_dma->mutex);
> +
>        out_be32(&regs->sicr, psc_dma->sicr | MPC52xx_PSC_SICR_AWR);
>        udelay(3);
>        out_be32(&regs->sicr, psc_dma->sicr);
> +
> +       mutex_unlock(&psc_dma->mutex);
>  }
>
> +#define  MPC52xx_PSC_SICR_ACRB (0x8 << 24)

Put this #define with the rest of the MPC52xx_PSC_SICR_* #defines.

>  static void psc_ac97_cold_reset(struct snd_ac97 *ac97)
>  {
>        struct mpc52xx_psc __iomem *regs = psc_dma->psc_regs;
>
> -       /* Do a cold reset */
> -       out_8(&regs->op1, MPC52xx_PSC_OP_RES);
> -       udelay(10);
> -       out_8(&regs->op0, MPC52xx_PSC_OP_RES);
> +       mutex_lock(&psc_dma->mutex);
> +
> +       switch (psc_dma->id) {
> +       case 0:
> +       case 1:
> +               mpc5200_psc_ac97_gpio_reset(psc_dma->id);
> +               dev_info(psc_dma->dev, "cold reset\n");
> +
> +               /* Notify the PSC that a reset has occurred */
> +               out_be32(&regs->sicr, psc_dma->sicr | MPC52xx_PSC_SICR_ACRB);
> +
> +               /* Re-enable RX and TX */
> +               out_8(&regs->command, MPC52xx_PSC_TX_ENABLE | 
> MPC52xx_PSC_RX_ENABLE);
> +
> +               break;
> +       default:
> +               dev_err(psc_dma->dev,
> +                       "Unable to determine PSC, no cold-reset will be "
> +                       "performed\n");
> +       }

You can drop the switch block and the error message.  You do exactly
the same thing in the common code.  Just call
mpc5200_psc_ac97_gpio_reset() unconditionally.

Otherwise, this version looks pretty good.  After you've respun both
patches (I've got comments to make on the other too), and if it's okay
by Mark, then I can merge both patches through my tree.

Cheers,
g.
_______________________________________________
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev