Re: Question about pxa2xx-ac97-lib.c

Re: Question about pxa2xx-ac97-lib.c

[Robert Jarzmik]

Marek Vasut <marek.vasut@gmail.com> writes:
> One more thing, I'd like to ask how to change the reset_gpio in the above 
> file. Will I have to make copy of pxa_ac97_dai[PXA2XX_DAI_AC97_HIFI] in 
> palm27x.c and change the .probe pointer to my own function that sets the 
> proper platform data? That is, something like the following? Thanks

I'll add Mark to the discussion here, for the ASoC point of view. Mark, the
original mail is in [1].

I originaly thought we would change pxa_set_ac97_info(), to add a parameter
which will be the platform data for the gpio.

IOW, have something like :
void __init pxa_set_ac97_info(struct pxa2xx_ac97_platform_data *pdata)
{
        pxa_register_device(&pxa_device_ac97, ops);
}

The trouble is, I saw references to the platform data as pxa2xx_audio_ops_t*,
mostly to suspend and resume a card. I've seen them in pxa2xx_ac97_do_suspend(),
pxa2xx_ac97_do_resume().

Mark, could you please have a glance at it, and tell us the right approach to
have the struct pxa2xx_ac97_platform_data passed to pxa2xx-ac97 please ?

Cheers.

--
Robert

[1] :
> diff --git a/sound/soc/pxa/palm27x.c b/sound/soc/pxa/palm27x.c
> index 48a73f6..f134f95 100644
> --- a/sound/soc/pxa/palm27x.c
> +++ b/sound/soc/pxa/palm27x.c
> @@ -19,11 +19,13 @@
>  #include <linux/gpio.h>
>  #include <linux/interrupt.h>
>  #include <linux/irq.h>
> +#include <linux/platform_device.h>
>
>  #include <sound/core.h>
>  #include <sound/pcm.h>
>  #include <sound/soc.h>
>  #include <sound/soc-dapm.h>
> +#include <sound/pxa2xx-lib.h>
>
>  #include <asm/mach-types.h>
>  #include <mach/audio.h>
> @@ -168,6 +170,21 @@ static int palm27x_ac97_init(struct snd_soc_codec *codec)
>         return 0;
>  }
> // this is the altered probe function
> +/* Palms use GPIO95 for AC97 reset */
> +static int palm_ac97_probe(struct platform_device *pdev,
> +                            struct snd_soc_dai *dai)
> +{
> +       struct platform_device *pd = to_platform_device(dai->dev);
> +       static struct pxa2xx_ac97_platform_data pdata = {
> +               .reset_gpio = 95,
> +       };
> +       printk("%s[%i]\n", __FUNCTION__, __LINE__);
> +       pd->dev.platform_data = &pdata;
> +       return pxa2xx_ac97_hw_probe(pd);
> +}
> +
> +static struct snd_soc_dai hifi_cpu_dai;
> +
>  static struct snd_soc_dai_link palm27x_dai[] = {
>  {
>         .name = "AC97 HiFi",
> @@ -204,6 +221,8 @@ static int __init palm27x_asoc_init(void)
>  {
>         int ret;
> // here it'll be copied 
> +       pxa_ac97_dai[PXA2XX_DAI_AC97_HIFI].probe = palm_ac97_probe;
> +
>         if (!(machine_is_palmtx() || machine_is_palmt5() ||
>                 machine_is_palmld()))
>                 return -ENODEV;
>
>
> -------------------------------------------------------------------
> List admin: http://lists.arm.linux.org.uk/mailman/listinfo/linux-arm-kernel
> FAQ:        http://www.arm.linux.org.uk/mailinglists/faq.php
> Etiquette:  http://www.arm.linux.org.uk/mailinglists/etiquette.php

Re: Question about pxa2xx-ac97-lib.c

[Mark Brown]

On Sun, Apr 12, 2009 at 06:36:49PM +0200, Robert Jarzmik wrote:

> I originaly thought we would change pxa_set_ac97_info(), to add a parameter
> which will be the platform data for the gpio.

Yes, this is the approach that should be adopted.  The platform data
should not change per board.

> IOW, have something like :
> void __init pxa_set_ac97_info(struct pxa2xx_ac97_platform_data *pdata)
> {
>         pxa_register_device(&pxa_device_ac97, ops);
> }

Note that we already have platform data for the PXA AC97 devices.

> The trouble is, I saw references to the platform data as pxa2xx_audio_ops_t*,
> mostly to suspend and resume a card. I've seen them in pxa2xx_ac97_do_suspend(),
> pxa2xx_ac97_do_resume().

> Mark, could you please have a glance at it, and tell us the right approach to
> have the struct pxa2xx_ac97_platform_data passed to pxa2xx-ac97 please ?

Those are platform data, currently the only mainline use I'm aware of is
for mainstone - it's been present since before the pxa2xx-ac97-lib was
introduced.

Looking at the code it appears we've broken mainstone, though
unfortunately my mainstone has some independant problem with resume so I
can't test but I'll make a patch merging the two structures on Monday.

Re: Question about pxa2xx-ac97-lib.c

[Marek Vasut]

On Sunday 12 of April 2009 18:36:49 Robert Jarzmik wrote:
> Marek Vasut <marek.vasut@gmail.com> writes:
> > One more thing, I'd like to ask how to change the reset_gpio in the above
> > file. Will I have to make copy of pxa_ac97_dai[PXA2XX_DAI_AC97_HIFI] in
> > palm27x.c and change the .probe pointer to my own function that sets the
> > proper platform data? That is, something like the following? Thanks
>
> I'll add Mark to the discussion here, for the ASoC point of view. Mark, the
> original mail is in [1].
>
> I originaly thought we would change pxa_set_ac97_info(), to add a parameter
> which will be the platform data for the gpio.
>
> IOW, have something like :
> void __init pxa_set_ac97_info(struct pxa2xx_ac97_platform_data *pdata)
> {
>         pxa_register_device(&pxa_device_ac97, ops);
> }
>
> The trouble is, I saw references to the platform data as
> pxa2xx_audio_ops_t*, mostly to suspend and resume a card. I've seen them in
> pxa2xx_ac97_do_suspend(), pxa2xx_ac97_do_resume().
>
> Mark, could you please have a glance at it, and tell us the right approach
> to have the struct pxa2xx_ac97_platform_data passed to pxa2xx-ac97 please ?
>
> Cheers.
>
> --
> Robert
I've already noticed this way of doing it is very bad ;-) I made a patch that 
should be correct and I'm sending it to LAK in a while. I'll CC you for your 
opinion, thanks.
>
> [1] :
> > diff --git a/sound/soc/pxa/palm27x.c b/sound/soc/pxa/palm27x.c
> > index 48a73f6..f134f95 100644
> > --- a/sound/soc/pxa/palm27x.c
> > +++ b/sound/soc/pxa/palm27x.c
> > @@ -19,11 +19,13 @@
> >  #include <linux/gpio.h>
> >  #include <linux/interrupt.h>
> >  #include <linux/irq.h>
> > +#include <linux/platform_device.h>
> >
> >  #include <sound/core.h>
> >  #include <sound/pcm.h>
> >  #include <sound/soc.h>
> >  #include <sound/soc-dapm.h>
> > +#include <sound/pxa2xx-lib.h>
> >
> >  #include <asm/mach-types.h>
> >  #include <mach/audio.h>
> > @@ -168,6 +170,21 @@ static int palm27x_ac97_init(struct snd_soc_codec
> > *codec) return 0;
> >  }
> > // this is the altered probe function
> > +/* Palms use GPIO95 for AC97 reset */
> > +static int palm_ac97_probe(struct platform_device *pdev,
> > +                            struct snd_soc_dai *dai)
> > +{
> > +       struct platform_device *pd = to_platform_device(dai->dev);
> > +       static struct pxa2xx_ac97_platform_data pdata = {
> > +               .reset_gpio = 95,
> > +       };
> > +       printk("%s[%i]\n", __FUNCTION__, __LINE__);
> > +       pd->dev.platform_data = &pdata;
> > +       return pxa2xx_ac97_hw_probe(pd);
> > +}
> > +
> > +static struct snd_soc_dai hifi_cpu_dai;
> > +
> >  static struct snd_soc_dai_link palm27x_dai[] = {
> >  {
> >         .name = "AC97 HiFi",
> > @@ -204,6 +221,8 @@ static int __init palm27x_asoc_init(void)
> >  {
> >         int ret;
> > // here it'll be copied
> > +       pxa_ac97_dai[PXA2XX_DAI_AC97_HIFI].probe = palm_ac97_probe;
> > +
> >         if (!(machine_is_palmtx() || machine_is_palmt5() ||
> >                 machine_is_palmld()))
> >                 return -ENODEV;
> >
> >
> > -------------------------------------------------------------------
> > List admin:
> > http://lists.arm.linux.org.uk/mailman/listinfo/linux-arm-kernel FAQ:     
> >   http://www.arm.linux.org.uk/mailinglists/faq.php
> > Etiquette:  http://www.arm.linux.org.uk/mailinglists/etiquette.php

Re: Question about pxa2xx-ac97-lib.c

[Mark Brown]

On Sun, Apr 12, 2009 at 04:47:39PM +0200, Marek Vasut wrote:
> One more thing, I'd like to ask how to change the reset_gpio in the above 
> file. Will I have to make copy of pxa_ac97_dai[PXA2XX_DAI_AC97_HIFI] in 
> palm27x.c and change the .probe pointer to my own function that sets the 
> proper platform data? That is, something like the following? Thanks

BTW, I meant to say in reply to the previous mail - you really should
copy all audio discussion to at least alsa-devel.

> +/* Palms use GPIO95 for AC97 reset */
> +static int palm_ac97_probe(struct platform_device *pdev,
> +                            struct snd_soc_dai *dai)
> +{
> +       struct platform_device *pd = to_platform_device(dai->dev);
> +       static struct pxa2xx_ac97_platform_data pdata = {
> +               .reset_gpio = 95,
> +       };
> +       printk("%s[%i]\n", __FUNCTION__, __LINE__);
> +       pd->dev.platform_data = &pdata;
> +       return pxa2xx_ac97_hw_probe(pd);
> +}

Look in mainline - pxa2xx-ac97-lib already provides platform data based
configuration of the GPIO to use for reset.

Re: Question about pxa2xx-ac97-lib.c

[Marek Vasut]

On Sunday 12 of April 2009 19:00:37 Mark Brown wrote:
> On Sun, Apr 12, 2009 at 04:47:39PM +0200, Marek Vasut wrote:
> > One more thing, I'd like to ask how to change the reset_gpio in the above
> > file. Will I have to make copy of pxa_ac97_dai[PXA2XX_DAI_AC97_HIFI] in
> > palm27x.c and change the .probe pointer to my own function that sets the
> > proper platform data? That is, something like the following? Thanks
>
> BTW, I meant to say in reply to the previous mail - you really should
> copy all audio discussion to at least alsa-devel.
>
> > +/* Palms use GPIO95 for AC97 reset */
> > +static int palm_ac97_probe(struct platform_device *pdev,
> > +                            struct snd_soc_dai *dai)
> > +{
> > +       struct platform_device *pd = to_platform_device(dai->dev);
> > +       static struct pxa2xx_ac97_platform_data pdata = {
> > +               .reset_gpio = 95,
> > +       };
> > +       printk("%s[%i]\n", __FUNCTION__, __LINE__);
> > +       pd->dev.platform_data = &pdata;
> > +       return pxa2xx_ac97_hw_probe(pd);
> > +}
>
> Look in mainline - pxa2xx-ac97-lib already provides platform data based
> configuration of the GPIO to use for reset.

Yes, I sent the new version and CCed alsa-devel.