Broken outmixer_event function in wm8400/wm8990/wm8991

Broken outmixer_event function in wm8400/wm8990/wm8991

[Lars-Peter Clausen]

Hi,

I was looking at the drivers that were using the SND_SOC_DAPM_PRE_REG event 
and I noticed that the outmixer_event function for the wm8400, wm8990 and 
wm8991 is broken. They do a switch statement to determine which kcontrol 
cause the event in the form of 'switch(kcontrol->private_value) { case (REG 
| (SHIFT << 8)): ...'. Long long time ago we used to store the register and 
the shift in the same integer which was stored in private_value. But this 
was changed in commit 4eaa9819 ("ALSA: ASoC: Convert bitfields in ASoC into 
full int width"). This was long before the wm8400 and wm8991 drivers were 
merged and just a month after the wm8990 driver was merged. So these 
functions have been dead code pretty much for as long as they existed. Do 
you want to fix this up? If not I'm going to send a patch that removes the 
functions. If you want to fix this up, this should be re-implemented as a 
custom put handler for the kcontrols rather than a event callback, which 
checks whether the change is valid before doing any DAPM changes. The event 
callback runs in the middle of the DAPM sequence, so half of the changes 
will already have been applied before the check is made whether the change 
should be done or not.

- Lars

Re: Broken outmixer_event function in wm8400/wm8990/wm8991

[Mark Brown]

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

On Mon, May 12, 2014 at 10:19:26AM +0200, Lars-Peter Clausen wrote:

> I was looking at the drivers that were using the SND_SOC_DAPM_PRE_REG event
> and I noticed that the outmixer_event function for the wm8400, wm8990 and
> wm8991 is broken. They do a switch statement to determine which kcontrol
> cause the event in the form of 'switch(kcontrol->private_value) { case (REG
> | (SHIFT << 8)): ...'. Long long time ago we used to store the register and
> the shift in the same integer which was stored in private_value. But this
> was changed in commit 4eaa9819 ("ALSA: ASoC: Convert bitfields in ASoC into
> full int width"). This was long before the wm8400 and wm8991 drivers were
> merged and just a month after the wm8990 driver was merged. So these
> functions have been dead code pretty much for as long as they existed. Do
> you want to fix this up? If not I'm going to send a patch that removes the
> functions. If you want to fix this up, this should be re-implemented as a
> custom put handler for the kcontrols rather than a event callback, which
> checks whether the change is valid before doing any DAPM changes. The event
> callback runs in the middle of the DAPM sequence, so half of the changes
> will already have been applied before the check is made whether the change
> should be done or not.

Paragraphs are good for legibility...  it would be better to fix this up
since it's a useful example for people to have, I have seen this sort of
"A but not B" thing in other devices.  Looking briefly at the WM8400
datasheet custom put controls would be better though looking at the code
I'm not sure that the intention with the original code (which predates
me) wasn't to enforce a limitation based on the outputs being enabled.

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

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

Re: Broken outmixer_event function in wm8400/wm8990/wm8991

[Charles Keepax]

On Mon, May 12, 2014 at 09:47:34PM +0100, Mark Brown wrote:
> On Mon, May 12, 2014 at 10:19:26AM +0200, Lars-Peter Clausen wrote:
> 
> > I was looking at the drivers that were using the SND_SOC_DAPM_PRE_REG event
> > and I noticed that the outmixer_event function for the wm8400, wm8990 and
> > wm8991 is broken. They do a switch statement to determine which kcontrol
> > cause the event in the form of 'switch(kcontrol->private_value) { case (REG
> > | (SHIFT << 8)): ...'. Long long time ago we used to store the register and
> > the shift in the same integer which was stored in private_value. But this
> > was changed in commit 4eaa9819 ("ALSA: ASoC: Convert bitfields in ASoC into
> > full int width"). This was long before the wm8400 and wm8991 drivers were
> > merged and just a month after the wm8990 driver was merged. So these
> > functions have been dead code pretty much for as long as they existed. Do
> > you want to fix this up? If not I'm going to send a patch that removes the
> > functions. If you want to fix this up, this should be re-implemented as a
> > custom put handler for the kcontrols rather than a event callback, which
> > checks whether the change is valid before doing any DAPM changes. The event
> > callback runs in the middle of the DAPM sequence, so half of the changes
> > will already have been applied before the check is made whether the change
> > should be done or not.

It is a PRE_REG event so should it not run before the other
changes? But a failed event callback doesn't stop the DAPM
sequence, so it will actually proceed with the broken config and
all we actually do at the moment is print a message.

> 
> Paragraphs are good for legibility...  it would be better to fix this up
> since it's a useful example for people to have, I have seen this sort of
> "A but not B" thing in other devices.  Looking briefly at the WM8400
> datasheet custom put controls would be better though looking at the code
> I'm not sure that the intention with the original code (which predates
> me) wasn't to enforce a limitation based on the outputs being enabled.

Yeah I think the intention was to avoid imposing limitations on
the order you set the controls, as the error checking is only
done when the power sequence runs so you can move through an
invalid state as long as when things power up they are valid.

I would be inclined to go with the custom put/get method as well,
but it leaves a question of what to do if the user sets an
invalid value. Refusing to set the control feels nicer, but adds
an ordering limitation on setting the controls. Updating the
conflicting control is also possible, but might be a little
surprising?

I am happy to update the driver here but might take me a little
while to get around to it.

Thanks,
Charles

Re: Broken outmixer_event function in wm8400/wm8990/wm8991

[Lars-Peter Clausen]

On 05/13/2014 03:53 PM, Charles Keepax wrote:
> On Mon, May 12, 2014 at 09:47:34PM +0100, Mark Brown wrote:
>> On Mon, May 12, 2014 at 10:19:26AM +0200, Lars-Peter Clausen wrote:
>>
>>> I was looking at the drivers that were using the SND_SOC_DAPM_PRE_REG event
>>> and I noticed that the outmixer_event function for the wm8400, wm8990 and
>>> wm8991 is broken. They do a switch statement to determine which kcontrol
>>> cause the event in the form of 'switch(kcontrol->private_value) { case (REG
>>> | (SHIFT << 8)): ...'. Long long time ago we used to store the register and
>>> the shift in the same integer which was stored in private_value. But this
>>> was changed in commit 4eaa9819 ("ALSA: ASoC: Convert bitfields in ASoC into
>>> full int width"). This was long before the wm8400 and wm8991 drivers were
>>> merged and just a month after the wm8990 driver was merged. So these
>>> functions have been dead code pretty much for as long as they existed. Do
>>> you want to fix this up? If not I'm going to send a patch that removes the
>>> functions. If you want to fix this up, this should be re-implemented as a
>>> custom put handler for the kcontrols rather than a event callback, which
>>> checks whether the change is valid before doing any DAPM changes. The event
>>> callback runs in the middle of the DAPM sequence, so half of the changes
>>> will already have been applied before the check is made whether the change
>>> should be done or not.
>
> It is a PRE_REG event so should it not run before the other
> changes? But a failed event callback doesn't stop the DAPM
> sequence, so it will actually proceed with the broken config and
> all we actually do at the moment is print a message.

The PRE_REG event runs before the change to the register of the kcontrol. 
Before that we already run DAPM widgets power down sequence. (Although 
admittedly there probably won't be many power down events as a result from 
turning a switch on).

>
>>
>> Paragraphs are good for legibility...  it would be better to fix this up
>> since it's a useful example for people to have, I have seen this sort of
>> "A but not B" thing in other devices.  Looking briefly at the WM8400
>> datasheet custom put controls would be better though looking at the code
>> I'm not sure that the intention with the original code (which predates
>> me) wasn't to enforce a limitation based on the outputs being enabled.
>
> Yeah I think the intention was to avoid imposing limitations on
> the order you set the controls, as the error checking is only
> done when the power sequence runs so you can move through an
> invalid state as long as when things power up they are valid.
>
> I would be inclined to go with the custom put/get method as well,
> but it leaves a question of what to do if the user sets an
> invalid value. Refusing to set the control feels nicer, but adds
> an ordering limitation on setting the controls. Updating the
> conflicting control is also possible, but might be a little
> surprising?

The event PRE_REG event is generated as soon as the control is changed, not 
when the path powers up. So there is no difference to a custom put handler 
in this regard.

You could automatically mute the other control if one of the controls get 
unmuted, that at least solves the ordering issue.

- Lars

Re: Broken outmixer_event function in wm8400/wm8990/wm8991

[Charles Keepax]

On Tue, May 13, 2014 at 04:11:03PM +0200, Lars-Peter Clausen wrote:
> On 05/13/2014 03:53 PM, Charles Keepax wrote:
>> On Mon, May 12, 2014 at 09:47:34PM +0100, Mark Brown wrote:
>>> On Mon, May 12, 2014 at 10:19:26AM +0200, Lars-Peter Clausen wrote:
>> I would be inclined to go with the custom put/get method as well,
>> but it leaves a question of what to do if the user sets an
>> invalid value. Refusing to set the control feels nicer, but adds
>> an ordering limitation on setting the controls. Updating the
>> conflicting control is also possible, but might be a little
>> surprising?
>
> The event PRE_REG event is generated as soon as the control is changed, 
> not when the path powers up. So there is no difference to a custom put 
> handler in this regard.

Right, thanks, in that case yeah I can see no reason not to
switch to a custom put. I will shove it on the todo list.

Thanks,
Charles

Re: Broken outmixer_event function in wm8400/wm8990/wm8991

[Mark Brown]

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

On Tue, May 13, 2014 at 02:53:05PM +0100, Charles Keepax wrote:

> Yeah I think the intention was to avoid imposing limitations on
> the order you set the controls, as the error checking is only
> done when the power sequence runs so you can move through an
> invalid state as long as when things power up they are valid.

Practically speaking we already have ordering restrictions - in general
you need to do a teardown followed by a setup otherwise there's a good
chance of causing audible noise from the switching during the setup.
Like Lars says we do run the event immediately anyway since the register
update is buried in DAPM processing for exactly this reason, we're
trying to avoid introducing a sudden DC offset by switching to/from an
active output so we see if we can power things down before we touch
anything.

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

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