Ticket #1420 (closed enhancement: fixed)

Opened 7 years ago

Last modified 7 years ago

Audio driver for AD1888 - power verification.

Reported by: jg Owned by: J5
Priority: blocker Milestone: Trial-2
Component: kernel Version:
Keywords: power Cc: jayakumar, dilinger, jg, rsmith, J5
Action Needed: Verified: no
Deployments affected: Blocked By:
Blocking:

Description (last modified by cjb) (diff)

The Analog Devices AD1888 can disable all sections of itself it is not using. As it can drive 5.1 audio output, for example, it is important that we verify that all but the stereo channels are disabled.

It also has a "deep sleep" feature, in which it can be entirely disabled. If its device driver is closed, the AD1888 should be put into deep sleep, and restored when the device opened.

.. but, currently the mic LED is not turning off when the device is idle. This bug investigates that first.

Attachments

stap-out (75.7 kB) - added by cjb 7 years ago.
1420.patch (1.9 kB) - added by jayakumar 7 years ago.
Patch to fix Mic LED not turning off after playback or internal mic recording
vref-while-recording.patch (3.2 kB) - added by cjb 7 years ago.

Change History

  Changed 7 years ago by jg

Yes, I've already implemented full suspend (PR0..6 + EAPD) in cs5535audio and ad1888. See cs5535audio_pm.c and ac97_patch.c for details.

By the way, CONFIG_SND_AC97_POWER_SAVE is not enabled in your default olpc-2.6 config. That will save additional power.

Regards, jaya

  Changed 7 years ago by jg

  • cc rsmith added
  • owner changed from rsmith to dilinger

Andres, could you please make the config file change and then reassign this bug back to richard to verify everything is working as expected?

  Changed 7 years ago by jg

  • priority changed from normal to blocker
  • verified unset
  • milestone changed from Trial-2 to BTest-4

  Changed 7 years ago by dilinger

It looks like we default to power_save=0; do we really want that? Seems like it should be inverted, with people disabling power_save if they encounter bugs..

  Changed 7 years ago by dilinger

I inverted the bit in stable kernels; once we decide about whether or not we want inversion on a wider scale, I'll deal with master.

Richard, care to do some power measurements? Toggling power_save mode can be done via /sys/module/snd_ac97_codec/parameters/power_save

  Changed 7 years ago by dilinger

  • owner changed from dilinger to rsmith

  Changed 7 years ago by cjb

Does the mic light on the case go off when you turn on power_save, or is turning the hardware off going to be a different bug?

  Changed 7 years ago by dilinger

The mic light does not go off.

  Changed 7 years ago by jg

  • owner changed from rsmith to cjb
  • milestone changed from BTest-4 to Trial-2

follow-up: ↓ 11   Changed 7 years ago by cjb

  • description modified (diff)

Hi Jaya,

Let's move the "driver doesn't power down when idle" bug over here, and leave #1735 for the popping problem.

I took a (large, attached) systemtap log of all snd_* function entry (so, it includes snd_{ac97,cs5535audio,pcm}* and more), in case it helps us figure out which functions are failing to fire. I can write a more complex script to do anything systemtap can do (e.g. print function arguments) if you can think of what else would help.

Thanks!

Changed 7 years ago by cjb

in reply to: ↑ 10   Changed 7 years ago by jayakumar

Replying to cjb:

I took a (large, attached) systemtap log of all snd_* function entry (so, it includes snd_{ac97,cs5535audio,pcm}* and more), in case it helps us figure out which functions are failing to

The stap log was very useful. I see the problem in that the workqueue scheme changed and drivers need to tell ac97 to rearm on close. I've got a patch against 2.6.21 and it seems to work with basic testing. I'll prepare a patch against olpc-2.6 and attach it here.

Changed 7 years ago by jayakumar

Patch to fix Mic LED not turning off after playback or internal mic recording

follow-up: ↓ 13   Changed 7 years ago by jayakumar

Ok. Attached the patch against olpc-2.6. One more note, if one was recording in analog input mode, then V_Ref is turned off and thus the Mic LED is off. I'm not sure if that's the behavior that is desired. I assume there's a direct trace from the V_Ref pin to the LED, so the driver can't change that behavior.

in reply to: ↑ 12 ; follow-ups: ↓ 14 ↓ 15   Changed 7 years ago by cjb

Replying to jayakumar:

Ok. Attached the patch against olpc-2.6. One more note, if one was recording in analog input mode, then V_Ref is turned off and thus the Mic LED is off. I'm not sure if that's the behavior that is desired. I assume there's a direct trace from the V_Ref pin to the LED, so the driver can't change that behavior.

Without the voltage from V_ref, it's unlikely that you could hear anything from the mic, and the purpose of the LED is as a privacy light, so I don't think this is a problem. We have an intern looking at applications of analog input mode; I'll ask him to verify that there isn't useful microphone data lurking there. Thanks!

(Also, thanks very much indeed for the patch, which I'll test tomorrow.)

in reply to: ↑ 13   Changed 7 years ago by jayakumar

While in analog input mode the Mic LED will be off and the incoming data can be recorded. If one used any non-powered/passive mic (like a dynamic mic), externally powered mic or line in, then meaningful audio data would be recorded.

We have an intern looking at applications of analog input mode

Cool. I imagine things like electronic barometers, humidity monitors, soil chemistry monitor, ECG/electronic stethoscope, passive blood pressure monitor, electronic blood glucose monitor and stuff like that for telemedicine would be useful to implement.

in reply to: ↑ 13 ; follow-up: ↓ 16   Changed 7 years ago by cjb

Hi Jaya,

Replying to jayakumar:

Ok. Attached the patch against olpc-2.6.

Works like a charm, with the LED coming on when recording starts, and off a short time after it ends. Thanks!

It would be nice to not have the LED come on during playback, so that it's only on when we're actively using the microphone -- at the moment it seems to come on regardless of whether the device is being used for playback or record. ("aplay" turns it on, as well as the other activities in our builds that do audio playback)

Any idea what we'd need to change to achieve this? (If you're busy, I'd be happy to try it out myself, given a description of what needs to be done.)

Thanks again.

in reply to: ↑ 15   Changed 7 years ago by jayakumar

Replying to cjb:

Works like a charm, with the LED coming on when recording starts, and off a short time after it ends. Thanks!

Cool.

It would be nice to not have the LED come on during playback, so that it's only on when we're actively using the microphone -- at the moment it seems to come on regardless of whether the device is being used for playback or record. ("aplay" turns it on, as well as the other activities in our builds that do audio playback)

V_Ref bias gets turned on at resume for playback/aplay because the default AD1888 codec state (reg 0x76) is for V_Ref to be on.

Any idea what we'd need to change to achieve this? (If you're busy, I'd be happy to try it out myself, given a description of what needs to be done.)

If you inverted the default to be V_Ref off, then when aplay is run, the LED won't come on. But then it wouldn't come on when arecord is run.

You would need to independently track whether the user wanted to record in a biased Mic mode (V_Ref on, LED on) or wanted to record in an unbiased /analog input mode (V_Ref off, LED off). It would not be correct for recording to always turn on V_Ref since that would break analog input mode.

I think it'd be better to have the Recording LED driven by a gpio pin and have the driver turn it on when recording is started/stopped rather than making the Recording LED depend on V_Ref bias which is a property independent to recording state.

follow-up: ↓ 18   Changed 7 years ago by cjb

Hi Jaya,

You would need to independently track whether the user wanted to record in a biased Mic mode (V_Ref on, LED on) or wanted to record in an unbiased /analog input mode (V_Ref off, LED off). It would not be correct for recording to always turn on V_Ref since that would break analog input mode.

But we have a separate "analog input" mixer control that applications will toggle if they want to use analog in, so the above idea could work if we check the value of that control when deciding whether to turn on V_Ref as we start recording. Might that work?

I think it'd be better to have the Recording LED driven by a gpio pin and have the driver turn it on when recording is started/stopped rather than making the Recording LED depend on V_Ref bias which is a property independent to recording state.

The idea behind putting the LED in series with the V_Ref bias is that, even in the face of a totally compromised machine running an arbitrary kernel, you know if you're being listened to.

in reply to: ↑ 17   Changed 7 years ago by cjb

Replying to cjb:

But we have a separate "analog input" mixer control that applications will toggle if they want to use analog in, so the above idea could work if we check the value of that control when deciding whether to turn on V_Ref as we start recording. Might that work?

This fails 'cause we might sometimes want to be in analog in but with V_Ref *on*. One solution is to decouple the Analog In control from affecting other control and let applications that want to use Analog In set up the high-pass filter and V_Ref themselves.

Once that's done, we can add:

  • if we're about to start [or stop] recording
    • if analog in is turned on, leave V_Ref alone
    • else turn V_Ref on [or off]

Make sense?

  Changed 7 years ago by cjb

Attached is a patch that does exactly the above. Andres, please apply both patches in this ticket to master.

J5, we need to change the alsa state file to leave the V_REFOUT mixer off by default, rather than on. It looks like the Tamtam team already have a change request in -- we should pull theirs:

http://dev.laptop.org/ticket/1939

and then make the switch. Note this will break recording on builds that don't contain the kernel patches here; fine for Trial-2, which should have both, but we'll need to be careful with 406.

Changed 7 years ago by cjb

  Changed 7 years ago by cjb

  • cc J5 added

  Changed 7 years ago by cjb

  • owner changed from cjb to J5

Reassigning to J5, to flip the initial state of V_REFOUT to off in alsa.

  Changed 7 years ago by J5

  • status changed from new to closed
  • resolution set to fixed

fixed in 496

Note: See TracTickets for help on using tickets.