[PATCH RFC 0/3] WM97xx AC97 GPIO support

[PATCH RFC 0/3] WM97xx AC97 GPIO support

[Manuel Lauss]

Here's 3 patches which implement simple AC97 GPIO support to the WM97xx touch
drivers and the Alchemy (au1x) AC97 ASoC implementation.

Tested on Au1200 hardware with a WM9712 codec:  According to Mark Brown the
WM9712 only sets GPIOs when data is sent through slot 12, which prompted me
to implement this.

Thanks,
       Manuel Lauss


Manuel Lauss (3):
  sound: ac97: Add GPIO manipulation hooks
  input: wm97xx: set GPIOs with AC97 GPIO busops if implemented
  sound: asoc: au1x: implement AC97 GPIO access

 drivers/input/touchscreen/wm97xx-core.c |    5 +++-
 include/sound/ac97_codec.h              |    2 +
 sound/soc/au1x/psc-ac97.c               |   39 +++++++++++++++++++++++++++++-
 sound/soc/au1x/psc.h                    |    2 +
 4 files changed, 45 insertions(+), 3 deletions(-)

[PATCH RFC 1/3] sound: ac97: Add GPIO manipulation hooks

[Manuel Lauss]

Add callbacks to struct snd_ac97 to set and retrieve ac97 codec
GPIO status.

Signed-off-by: Manuel Lauss <manuel.lauss@googlemail.com>
---
 include/sound/ac97_codec.h |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/include/sound/ac97_codec.h b/include/sound/ac97_codec.h
index 4940045..db3aee1 100644
--- a/include/sound/ac97_codec.h
+++ b/include/sound/ac97_codec.h
@@ -437,6 +437,8 @@ struct snd_ac97_bus_ops {
 	unsigned short (*read) (struct snd_ac97 *ac97, unsigned short reg);
 	void (*wait) (struct snd_ac97 *ac97);
 	void (*init) (struct snd_ac97 *ac97);
+	void (*setgpio) (struct snd_ac97 *ac97, unsigned short gpio);
+	unsigned short (*getgpio) (struct snd_ac97 *ac97);
 };
 
 struct snd_ac97_bus {
-- 
1.7.1

Re: [PATCH RFC 1/3] sound: ac97: Add GPIO manipulation hooks

[Mark Brown]

On Tue, Jun 15, 2010 at 05:55:45PM +0200, Manuel Lauss wrote:
> Add callbacks to struct snd_ac97 to set and retrieve ac97 codec
> GPIO status.

> Signed-off-by: Manuel Lauss <manuel.lauss@googlemail.com>

CCing in Takashi since this is generic AC'97 stuff.

> ---
>  include/sound/ac97_codec.h |    2 ++
>  1 files changed, 2 insertions(+), 0 deletions(-)
> 
> diff --git a/include/sound/ac97_codec.h b/include/sound/ac97_codec.h
> index 4940045..db3aee1 100644
> --- a/include/sound/ac97_codec.h
> +++ b/include/sound/ac97_codec.h
> @@ -437,6 +437,8 @@ struct snd_ac97_bus_ops {
>  	unsigned short (*read) (struct snd_ac97 *ac97, unsigned short reg);
>  	void (*wait) (struct snd_ac97 *ac97);
>  	void (*init) (struct snd_ac97 *ac97);
> +	void (*setgpio) (struct snd_ac97 *ac97, unsigned short gpio);
> +	unsigned short (*getgpio) (struct snd_ac97 *ac97);
>  };
>  
>  struct snd_ac97_bus {
> -- 
> 1.7.1
> 

So, I'd kind of expect some sort of interaction with gpiolib somewhere
if we do support GPIOs (I know the WM97xx custom stuff doesn't currently
use GPIOs but...).

Re: [PATCH RFC 1/3] sound: ac97: Add GPIO manipulation hooks

[Manuel Lauss]

On Tue, Jun 15, 2010 at 6:02 PM, Mark Brown
<broonie@opensource.wolfsonmicro.com> wrote:
> On Tue, Jun 15, 2010 at 05:55:45PM +0200, Manuel Lauss wrote:
>> Add callbacks to struct snd_ac97 to set and retrieve ac97 codec
>> GPIO status.
>
>> Signed-off-by: Manuel Lauss <manuel.lauss@googlemail.com>
>
> CCing in Takashi since this is generic AC'97 stuff.
>
>> ---
>>  include/sound/ac97_codec.h |    2 ++
>>  1 files changed, 2 insertions(+), 0 deletions(-)
>>
>> diff --git a/include/sound/ac97_codec.h b/include/sound/ac97_codec.h
>> index 4940045..db3aee1 100644
>> --- a/include/sound/ac97_codec.h
>> +++ b/include/sound/ac97_codec.h
>> @@ -437,6 +437,8 @@ struct snd_ac97_bus_ops {
>>       unsigned short (*read) (struct snd_ac97 *ac97, unsigned short reg);
>>       void (*wait) (struct snd_ac97 *ac97);
>>       void (*init) (struct snd_ac97 *ac97);
>> +     void (*setgpio) (struct snd_ac97 *ac97, unsigned short gpio);
>> +     unsigned short (*getgpio) (struct snd_ac97 *ac97);
>>  };
>>
>>  struct snd_ac97_bus {
>> --
>> 1.7.1
>>
>
> So, I'd kind of expect some sort of interaction with gpiolib somewhere
> if we do support GPIOs (I know the WM97xx custom stuff doesn't currently
> use GPIOs but...).

If I ever get access to the testhardware again, I'll implement it in
the wm97xx core.

        Manuel
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH RFC 1/3] sound: ac97: Add GPIO manipulation hooks

[Takashi Iwai]

At Tue, 15 Jun 2010 17:02:53 +0100,
Mark Brown wrote:
> 
> On Tue, Jun 15, 2010 at 05:55:45PM +0200, Manuel Lauss wrote:
> > Add callbacks to struct snd_ac97 to set and retrieve ac97 codec
> > GPIO status.
> 
> > Signed-off-by: Manuel Lauss <manuel.lauss@googlemail.com>
> 
> CCing in Takashi since this is generic AC'97 stuff.

Thanks.

> > ---
> >  include/sound/ac97_codec.h |    2 ++
> >  1 files changed, 2 insertions(+), 0 deletions(-)
> > 
> > diff --git a/include/sound/ac97_codec.h b/include/sound/ac97_codec.h
> > index 4940045..db3aee1 100644
> > --- a/include/sound/ac97_codec.h
> > +++ b/include/sound/ac97_codec.h
> > @@ -437,6 +437,8 @@ struct snd_ac97_bus_ops {
> >  	unsigned short (*read) (struct snd_ac97 *ac97, unsigned short reg);
> >  	void (*wait) (struct snd_ac97 *ac97);
> >  	void (*init) (struct snd_ac97 *ac97);
> > +	void (*setgpio) (struct snd_ac97 *ac97, unsigned short gpio);
> > +	unsigned short (*getgpio) (struct snd_ac97 *ac97);

Can this be handled simply in read/write callbacks in the controller
side?  In callbacks, you can check whether reg == AC97_GPIO_STATUS and
handle differently for such an access.


thanks,

Takashi

Re: [PATCH RFC 1/3] sound: ac97: Add GPIO manipulation hooks

[Manuel Lauss]

On Wed, Jun 16, 2010 at 9:55 AM, Takashi Iwai <tiwai@suse.de> wrote:
>> >  include/sound/ac97_codec.h |    2 ++
>> >  1 files changed, 2 insertions(+), 0 deletions(-)
>> >
>> > diff --git a/include/sound/ac97_codec.h b/include/sound/ac97_codec.h
>> > index 4940045..db3aee1 100644
>> > --- a/include/sound/ac97_codec.h
>> > +++ b/include/sound/ac97_codec.h
>> > @@ -437,6 +437,8 @@ struct snd_ac97_bus_ops {
>> >     unsigned short (*read) (struct snd_ac97 *ac97, unsigned short reg);
>> >     void (*wait) (struct snd_ac97 *ac97);
>> >     void (*init) (struct snd_ac97 *ac97);
>> > +   void (*setgpio) (struct snd_ac97 *ac97, unsigned short gpio);
>> > +   unsigned short (*getgpio) (struct snd_ac97 *ac97);
>
> Can this be handled simply in read/write callbacks in the controller
> side?  In callbacks, you can check whether reg == AC97_GPIO_STATUS and
> handle differently for such an access.

Interesting Idea, and as a bonus requires no changes to the wm97xx code.

Thank you!
      Manuel Lauss
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH RFC 1/3] sound: ac97: Add GPIO manipulation hooks

[Mark Brown]

On Wed, Jun 16, 2010 at 10:00:19AM +0200, Manuel Lauss wrote:
> On Wed, Jun 16, 2010 at 9:55 AM, Takashi Iwai <tiwai@suse.de> wrote:

> >> > +   void (*setgpio) (struct snd_ac97 *ac97, unsigned short gpio);
> >> > +   unsigned short (*getgpio) (struct snd_ac97 *ac97);

> > Can this be handled simply in read/write callbacks in the controller
> > side?  In callbacks, you can check whether reg == AC97_GPIO_STATUS and
> > handle differently for such an access.

> Interesting Idea, and as a bonus requires no changes to the wm97xx code.

The only thing that worries me there is any non-standard behavior the
chip may implement - it may actually want a write to that register for
some extended functionality.
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH RFC 2/3] input: wm97xx: set GPIOs with AC97 GPIO busops if implemented

[Manuel Lauss]

If the AC97 controller implements the ->setgpio callback, use it.
Some codecs (i.e. WM9712) only accept GPIO data through AC97 slot 12.

Signed-off-by: Manuel Lauss <manuel.lauss@googlemail.com>
---
 drivers/input/touchscreen/wm97xx-core.c |    5 ++++-
 1 files changed, 4 insertions(+), 1 deletions(-)

diff --git a/drivers/input/touchscreen/wm97xx-core.c b/drivers/input/touchscreen/wm97xx-core.c
index cbfef1e..4f7b38c 100644
--- a/drivers/input/touchscreen/wm97xx-core.c
+++ b/drivers/input/touchscreen/wm97xx-core.c
@@ -206,7 +206,10 @@ void wm97xx_set_gpio(struct wm97xx *wm, u32 gpio,
 		reg &= ~gpio;
 
 	if (wm->id == WM9712_ID2 && wm->variant != WM97xx_WM1613)
-		wm97xx_reg_write(wm, AC97_GPIO_STATUS, reg << 1);
+		reg <<= 1;
+
+	if (wm->ac97->bus->ops->setgpio)
+		wm->ac97->bus->ops->setgpio(wm->ac97, reg);
 	else
 		wm97xx_reg_write(wm, AC97_GPIO_STATUS, reg);
 	mutex_unlock(&wm->codec_mutex);
-- 
1.7.1

Re: [PATCH RFC 2/3] input: wm97xx: set GPIOs with AC97 GPIO busops if implemented

[Mark Brown]

On Tue, Jun 15, 2010 at 05:55:46PM +0200, Manuel Lauss wrote:
> If the AC97 controller implements the ->setgpio callback, use it.
> Some codecs (i.e. WM9712) only accept GPIO data through AC97 slot 12.
> 
> Signed-off-by: Manuel Lauss <manuel.lauss@googlemail.com>

It seems like it'd be easier to just do this as a generic AC'97 thing
rather than adding it to the WM97xx driver - that way any CODEC that has
GPIO support can use it, we don't have to add custom per-CODEC GPIO APIs.
If you're using the WM97xx-specific code you're already doing something
custom so it's not much loss to use a generic thing.

Like I said in reply to the previous mail super bonus fun points for
doing it in terms of gpiolib.

Re: [PATCH RFC 2/3] input: wm97xx: set GPIOs with AC97 GPIO busops if implemented

[Manuel Lauss]

On Tue, Jun 15, 2010 at 6:08 PM, Mark Brown
<broonie@opensource.wolfsonmicro.com> wrote:
> On Tue, Jun 15, 2010 at 05:55:46PM +0200, Manuel Lauss wrote:
>> If the AC97 controller implements the ->setgpio callback, use it.
>> Some codecs (i.e. WM9712) only accept GPIO data through AC97 slot 12.
>>
>> Signed-off-by: Manuel Lauss <manuel.lauss@googlemail.com>
>
> It seems like it'd be easier to just do this as a generic AC'97 thing
> rather than adding it to the WM97xx driver - that way any CODEC that has
> GPIO support can use it, we don't have to add custom per-CODEC GPIO APIs.
> If you're using the WM97xx-specific code you're already doing something
> custom so it's not much loss to use a generic thing.

Oh, now I get it.  Hmm, what about setting pin direction?  That
certainly is per-codec, no?

Manuel

Re: [PATCH RFC 2/3] input: wm97xx: set GPIOs with AC97 GPIO busops if implemented

[Mark Brown]

On Tue, Jun 15, 2010 at 06:17:18PM +0200, Manuel Lauss wrote:
> On Tue, Jun 15, 2010 at 6:08 PM, Mark Brown

> > It seems like it'd be easier to just do this as a generic AC'97 thing
> > rather than adding it to the WM97xx driver - that way any CODEC that has
> > GPIO support can use it, we don't have to add custom per-CODEC GPIO APIs.
> > If you're using the WM97xx-specific code you're already doing something
> > custom so it's not much loss to use a generic thing.

> Oh, now I get it.  Hmm, what about setting pin direction?  That
> certainly is per-codec, no?

IIRC, yes - I'd need to check.  I'd guess a lot of devices will be fixed
function and only support one direction.

[PATCH RFC 3/3] sound: asoc: au1x: implement AC97 GPIO access

[Manuel Lauss]

The Alchemy PSC AC97 controller can receive/transmit AC97 Slot 12 data
(Codec GPIO) through 2 registers.

Signed-off-by: Manuel Lauss <manuel.lauss@googlemail.com>
---
 sound/soc/au1x/psc-ac97.c |   39 +++++++++++++++++++++++++++++++++++++--
 sound/soc/au1x/psc.h      |    2 ++
 2 files changed, 39 insertions(+), 2 deletions(-)

diff --git a/sound/soc/au1x/psc-ac97.c b/sound/soc/au1x/psc-ac97.c
index a61ccd2..1da2949 100644
--- a/sound/soc/au1x/psc-ac97.c
+++ b/sound/soc/au1x/psc-ac97.c
@@ -56,6 +56,31 @@
 /* instance data. There can be only one, MacLeod!!!! */
 static struct au1xpsc_audio_data *au1xpsc_ac97_workdata;
 
+static void au1xpsc_ac97_setgpio(struct snd_ac97 *ac97, unsigned short gpio)
+{
+	/* FIXME */
+	struct au1xpsc_audio_data *pscdata = au1xpsc_ac97_workdata;
+	au_writel(gpio, AC97_GPO(pscdata));
+	au_sync();
+}
+
+static unsigned short au1xpsc_ac97_getgpio(struct snd_ac97 *ac97)
+{
+	/* FIXME */
+	struct au1xpsc_audio_data *pscdata = au1xpsc_ac97_workdata;
+	unsigned long data;
+
+	while (!(au_readl(AC97_EVNT(pscdata)) & PSC_AC97EVNT_GR))
+		cpu_relax();
+	au_writel(PSC_AC97EVNT_GR, AC97_EVNT(pscdata));
+	au_sync();
+
+	data = au_readl(AC97_GPI(pscdata));
+	data = (data >> 4) & 0xffff;
+
+	return data;
+}
+
 /* AC97 controller reads codec register */
 static unsigned short au1xpsc_ac97_read(struct snd_ac97 *ac97,
 					unsigned short reg)
@@ -195,6 +220,8 @@ struct snd_ac97_bus_ops soc_ac97_ops = {
 	.write		= au1xpsc_ac97_write,
 	.reset		= au1xpsc_ac97_cold_reset,
 	.warm_reset	= au1xpsc_ac97_warm_reset,
+	.setgpio	= au1xpsc_ac97_setgpio,
+	.getgpio	= au1xpsc_ac97_getgpio,
 };
 EXPORT_SYMBOL_GPL(soc_ac97_ops);
 
@@ -229,10 +256,12 @@ static int au1xpsc_ac97_hw_params(struct snd_pcm_substream *substream,
 			r &= ~PSC_AC97CFG_TXSLOT_MASK;
 			r |= PSC_AC97CFG_TXSLOT_ENA(3);
 			r |= PSC_AC97CFG_TXSLOT_ENA(4);
+			r |= PSC_AC97CFG_TXSLOT_ENA(12);  /* GPIO */
 		} else {
 			r &= ~PSC_AC97CFG_RXSLOT_MASK;
 			r |= PSC_AC97CFG_RXSLOT_ENA(3);
 			r |= PSC_AC97CFG_RXSLOT_ENA(4);
+			r |= PSC_AC97CFG_RXSLOT_ENA(12);  /* GPIO */
 		}
 
 		/* do we need to poke the hardware? */
@@ -384,9 +413,15 @@ static int __devinit au1xpsc_ac97_drvprobe(struct platform_device *pdev)
 	if (!wd->mmio)
 		goto out1;
 
-	/* configuration: max dma trigger threshold, enable ac97 */
+	/* configuration: max dma trigger threshold, enable ac97,
+	 * enable GPIO regs, enable GPIO slots, 16bit len default
+	 * so it clocks out all GPIO info.
+	 */
 	wd->cfg = PSC_AC97CFG_RT_FIFO8 | PSC_AC97CFG_TT_FIFO8 |
-		  PSC_AC97CFG_DE_ENABLE;
+		  PSC_AC97CFG_DE_ENABLE | PSC_AC97CFG_GE_ENABLE |
+		  PSC_AC97CFG_TXSLOT_ENA(12) |
+		  PSC_AC97CFG_RXSLOT_ENA(12) |
+		  PSC_AC97CFG_SET_LEN(16);
 
 	/* preserve PSC clock source set up by platform	 */
 	sel = au_readl(PSC_SEL(wd)) & PSC_SEL_CLK_MASK;
diff --git a/sound/soc/au1x/psc.h b/sound/soc/au1x/psc.h
index 32d3807..915c3de 100644
--- a/sound/soc/au1x/psc.h
+++ b/sound/soc/au1x/psc.h
@@ -55,5 +55,7 @@ struct au1xpsc_audio_data {
 #define AC97_PCR(x)	((unsigned long)((x)->mmio) + PSC_AC97PCR_OFFSET)
 #define AC97_RST(x)	((unsigned long)((x)->mmio) + PSC_AC97RST_OFFSET)
 #define AC97_STAT(x)	((unsigned long)((x)->mmio) + PSC_AC97STAT_OFFSET)
+#define AC97_GPO(x)	((unsigned long)((x)->mmio) + PSC_AC97GPO_OFFSET)
+#define AC97_GPI(x)	((unsigned long)((x)->mmio) + PSC_AC97GPI_OFFSET)
 
 #endif
-- 
1.7.1