Need help with WM8960

Need help with WM8960

[Timur Tabi]

Mark,

I'm trying to port some old Linux code to upstream for a third-party board
that uses a Freescale P1022 and a Wolfson WM8960. I've got audio working,
but it's using a hack from the third-party code, and I don't know how to
do implement the change properly, mostly because I don't understand DAPM.

The first problem is that I get this:

	wm8960 0-001a: No platform data supplied

What kind of platform data is the driver expecting, and how do I get it
that data?  I don't seem to need any platform data to make the code work.

Second, I uploaded the hack to wm8960.c at http://pastebin.com/RwVy6cQg

You can see that he disables the DAPM controls for DACL, DACR, ADCL, and
ADCR, and forces those bits set in the wm8960_probe function.  There's
also a comment that documents some limitations.  What is the proper way to
implement this change?

Third, in order to actually hear audio, I had to unmute the "Left Output
Mixer PCM" and "Right Output Mixer PCM".  Is this something that the
machine driver should do automatically?  And if so, how?

-- 
Timur Tabi
Linux kernel developer at Freescale

Re: Need help with WM8960

[Mark Brown]

On Wed, Sep 12, 2012 at 06:09:00PM -0500, Timur Tabi wrote:

> The first problem is that I get this:

> 	wm8960 0-001a: No platform data supplied

> What kind of platform data is the driver expecting, and how do I get it
> that data?  I don't seem to need any platform data to make the code work.

So, it should be fairly clear from the code that its looking for a
struct wm8960_data.  You can actually leave this totally blank but it's
important that the capless setting is correct one way or another - you
need to look at the schematic and check to see if there are capacitors
on the output not.

> You can see that he disables the DAPM controls for DACL, DACR, ADCL, and
> ADCR, and forces those bits set in the wm8960_probe function.  There's
> also a comment that documents some limitations.  What is the proper way to
> implement this change?

I'm not entirely sure why the user is doing this - unfortuantely they
just bodged the driver without reporting the issue.  The first issue
does not make much sense, the issues described are system level issues
that are unrelated to the ADC for the most part (for example, things
related to the micbias generally depend on the micbias enable).  Without
more information it's hard to comment.  The second sounds like they
missed the LRCM bit, I'll just add the ability to control that from
platform data.

> Third, in order to actually hear audio, I had to unmute the "Left Output
> Mixer PCM" and "Right Output Mixer PCM".  Is this something that the
> machine driver should do automatically?  And if so, how?

No, it's something that the application should do.  

Re: Need help with WM8960

[Tabi Timur-B04825]

Mark Brown wrote:
>
>> >Third, in order to actually hear audio, I had to unmute the "Left Output
>> >Mixer PCM" and "Right Output Mixer PCM".  Is this something that the
>> >machine driver should do automatically?  And if so, how?

> No, it's something that the application should do.

How would the application know to unmute those two particular controls? 
The only way I figured it out is by launching alsamixer and playing with 
every control until I heard sound.

-- 
Timur Tabi
Linux kernel developer at Freescale

Re: Need help with WM8960

[Mark Brown]

On Thu, Sep 13, 2012 at 03:26:03AM +0000, Tabi Timur-B04825 wrote:
> Mark Brown wrote:

> > No, it's something that the application should do.

> How would the application know to unmute those two particular controls? 
> The only way I figured it out is by launching alsamixer and playing with 
> every control until I heard sound.

The user needs to configure this as part of their system integration.
There's no real way to know what configuration is sane for a given
machine and the user is already doing system integration anyway; putting
things into userspace makes things much easier to maintain.  Even for
something like this we'd need to know if the output is connected to
headphone or line outputs as the volumes configured would generally be
rather different (and of course that can vary at runtime...).

Re: Need help with WM8960

[Timur Tabi]

Mark Brown wrote:

> So, it should be fairly clear from the code that its looking for a
> struct wm8960_data.  You can actually leave this totally blank but it's
> important that the capless setting is correct one way or another - you
> need to look at the schematic and check to see if there are capacitors
> on the output not.

There are 120uF capacitors connected to HP_L and HP_R.  Unfortunately, I
can't quite figure out whether I need platform data or not.

Also, how do I get the platform data to the codec driver?  Do I need to
create some arch code that reads the device tree properties and parses
them into a wm8960_data object, and then (somehow) injects that into the
wm8960 driver?

-- 
Timur Tabi
Linux kernel developer at Freescale

Re: Need help with WM8960

[Timur Tabi]

Timur Tabi wrote:
> Also, how do I get the platform data to the codec driver?  Do I need to
> create some arch code that reads the device tree properties and parses
> them into a wm8960_data object, and then (somehow) injects that into the
> wm8960 driver?

So, I'm guessing that there is no magical way to inject platform data into
an OF-initialized driver.  What do you think about this patch:

@@ -1029,6 +1030,7 @@ static const struct regmap_config wm8960_regmap = {
 static __devinit int wm8960_i2c_probe(struct i2c_client *i2c,
                                      const struct i2c_device_id *id)
 {
+       struct device_node *np = i2c->dev.of_node;
        struct wm8960_data *pdata = dev_get_platdata(&i2c->dev);
        struct wm8960_priv *wm8960;
        int ret;
@@ -1038,6 +1040,31 @@ static __devinit int wm8960_i2c_probe(struct
i2c_client *i2c,
        if (wm8960 == NULL)
                return -ENOMEM;

+       /*
+        * Are we running on an OF platform?  If so, then parse the device
+        * tree node for properties and create the platform data object
+        * ourselves.
+        */
+       if (np && !pdata) {
+               const __be32 *iprop;
+               int len;
+
+               pdata = devm_kzalloc(&i2c->dev, sizeof(struct wm8960_data),
+                                    GFP_KERNEL);
+               if (!pdata)
+                       return -ENOMEM;
+
+               if (of_find_property(np, "wlf,capless", NULL))
+                       pdata->capless = true;
+
+               iprop = of_get_property(np, "wlf,discharge-resistance", &len);
+               if (iprop && len == sizeof(uint32_t))
+                       pdata->dres = be32_to_cpup(iprop);
+
+               if (of_find_property(np, "wlf,shared-lrclk", NULL))
+                       pdata->shared_lrclk = true;
+
+               i2c->dev.platform_data = pdata;
+       }
+
        wm8960->regmap = regmap_init_i2c(i2c, &wm8960_regmap);
        if (IS_ERR(wm8960->regmap))
                return PTR_ERR(wm8960->regmap);


-- 
Timur Tabi
Linux kernel developer at Freescale

Re: Need help with WM8960

[Timur Tabi]

Timur Tabi wrote:
> +               iprop = of_get_property(np, "wlf,discharge-resistance", &len);
> +               if (iprop && len == sizeof(uint32_t))
> +                       pdata->dres = be32_to_cpup(iprop);

I just noticed that pdata->dres isn't actually used in the driver.  In
fact, I'm also confused by the DISOP bit.  The datasheet says that the
default value of DISOP is 0.  However, the only time we touch register R29
is here:

case SND_SOC_BIAS_STANDBY:
	switch (codec->dapm.bias_level) {
	case SND_SOC_BIAS_PREPARE:
		/* Disable HP discharge */
		snd_soc_update_bits(codec, WM8960_APOP2,
				    WM8960_DISOP | WM8960_DRES_MASK,
				    0);

All we do here is set DISOP and DRES_MASK to 0, which they should already be.

Am I missing something?

-- 
Timur Tabi
Linux kernel developer at Freescale

Re: Need help with WM8960

[Mark Brown]

On Thu, Sep 13, 2012 at 11:29:10AM -0500, Timur Tabi wrote:

> I just noticed that pdata->dres isn't actually used in the driver.  In

This was used in the original version of the driver when entering
standby from off but we stopped using it when we switched from having a
single startup sequence to having separate cap and capless sequences so
it's redundant now and could be removed.