On 03/19/2012 01:09 PM, Mark Brown wrote:
On Mon, Mar 19, 2012 at 09:07:16AM +0100, Ola Lilja wrote:
On 03/17/2012 11:31 PM, Mark Brown wrote:
We need to break chains on very specific positions to be able to affect only the parts intended. We also have switches (e.g. loopback) that is not belonging to a specific pin but is a brigde-switch in the middle of two chains, thus I don't think we can use what you suggest.
But if you have a control that actually does something then surely there should be some interaction with the chip when it is changed?
No, we don't want the control to do anything with the chip before playback/capture starts, but rather just indicate that a specific path is active, and then when the stream starts the chain gets complete and the widgets then do all interaction with the chip in the order that our HW requires. There is no bit that we can set prior to the execution of the DAPM-chain, without introducing clicks.
In the case of things that go external pins then I'd really expect the existing solution with pin switches to work, in other cases something else may be needed but we need to understand what you're trying to do. The issues with internal paths may need something but please be more concrete about what the actual register controls are and why you're doing such unusual things. The fact that you're using isolated switches and there's no mixers involved is really surprisng.
I'll try to give some more details with two scenarios below.
Scenario 1:
We have a routing situation that could be rendered something like this:
D----->Speaker | I2S (playback)------>A----->B----->C----->Headset
In this case, let's say that the bit at B needs to be set in HW before C is set to avoid pop/click noises. If we just use a normal playback-switch that is associated with the bit at C, then this switch would directly set the C bit before the stream is active and then later when the stream is activated, the other widgets are enabled thus B would be set (i.e. after C was set which is not the desired order). Our solution to handle this is to introduce a virtual switch (in the form of an enum_virt) between B and Headset. Please note that moving the playback-switch to B would not work since that would affect the Speaker-path enabling/disabling this as well which of course is not desired to be controlled in this particular use-case.
Scenario 2:
We have a switch with purpose of forwarding sound from LineIn to be mixed with HeadSet-audio coming from the I2S. However, we also (in parallell) have the possibility to record the data coming from LineIn independently of the HeadSet routing.
The routing situation could be rendered something like this:
I2S (playback)------>A----->B----->C----->Headset ^ | LineIn----->D------E----->F----->I2S (capture)
As you can see neither of the endpoints "belongs" to the bypass-path (E->B) and therefore I can't see how it should be possible to use a simple pin switch here as it would always affect other paths.
One of the nice things about open source code is that it's always possible to extend the core if required.
OK, so do you want us to make it possible to have NOPM for a playback/capture-switch? Can you confirm that this is OK and that it is possible to accomplish without destroying any mechanism in how it works today?
I'm saying that if you're doing things within your driver that go trough contortions or break the userspace interfaces because the frameworks didn't support things then you should be fixing the frameworks rather than doing whatever in the driver. This sort of thing seems to be at the root of several of the problems I'm seeing here, it's similar to all the stuff you're doing with regulators.
So, what you want is another device/driver pair where our current acc.det.-driver adds a driver to the device we add in the ASoC-driver?
I can't parse what you're saying here, sorry.
OK, I was trying to understand what you meant with these comments: "breaking the device model" and "just embed a trivial input driver in the CODEC for the vibra if it's small enough." Could you explain this abit more?
We are thinking of modifying so that we put the Android-vibra-functionality in userspace, and then it can use vibra through the controls we provide in the ASoC-driver.
That'd obviously remove any kernel level problems.
Regarding the regulators, I was under the impression that everything outside the actual codec, we could put in the machine-driver (which I think looks pretty good). This would make a clear distinction between all codec-register-handling and usage of those external frameworks (regulators and clocks). I have started to move the code location of
No. That's exactly the sort of stuff we don't want to see. The fact that the device needs power to operate isn't something that's specific to a particular board, it's a property of the silicon.
I agree that the fact that it needs power is not specific to the board, but we could have different regulators feeding the codec-chip. The regulators could be located outside the codec-chip and putting the codec in another board when the codec-driver requests a specific regulator not present in the new board would fail. Then we would need to have something like get_regulator("5 volts") to make it generic between boards having regulators with different names providing the same voltage. I'm not into how the regulators work and as the trend is to have the regulator stuff inside the codec-driver, I'll move it there. Also, see the last comment below for more info on our regulator-usage.
regulators/clocks into the codec-file and this results in that the machine-driver is basically empty of functionality and the codec-file is growing even bigger. Is this really what we want?
Yes, of course! You need to get away from the idea that the only board your device will ever be used on is the reference board, things that are generic to the device should be supported by the device driver for the device. If there's a device driver for the part we shouldn't be in the situation where any new system using the device needs to replicate basic functionality needed to get the device working.
I'm also not sure if you have seen that we actually do control ALL regulators and clocks via DAPM. The main power-regulator is controlled through a
I've not suggested that. Look at how other drivers manage their power.
I have looked alot at other drivers, but in many cases I've found that our situation looks pretty different and often more complex. See the last comment below for more info on our regulator-usage.
You are right that a large part of this code is there because it is needed by external requirements, but it does not mean that DAPM is not controlling the regulators and clocks, just that there is another interface that can also turn on/off the power.
I'm not seeing any code in the driver which manages the regulators...
This is all our regulator-widgets, currently located in the machine-driver and connected to chains located in the codec-driver:
/* Power AB8500 audio-block when AD/DA is active */ {"DAC", NULL, "AUDIO Regulator"}, {"ADC", NULL, "AUDIO Regulator"},
/* Power configured regulator when an analog mic is enabled */ {"MIC1A Input", NULL, "AMIC1A Regulator"}, {"MIC1B Input", NULL, "AMIC1B Regulator"}, {"MIC2 Input", NULL, "AMIC2 Regulator"},
/* Power DMIC-regulator when any digital mic is enabled */ {"DMic 1", NULL, "DMIC Regulator"}, {"DMic 2", NULL, "DMIC Regulator"}, {"DMic 3", NULL, "DMIC Regulator"}, {"DMic 4", NULL, "DMIC Regulator"}, {"DMic 5", NULL, "DMIC Regulator"}, {"DMic 6", NULL, "DMIC Regulator"},
The regulators are SND_SOC_DAPM_SUPPLY and connected to appropriate widgets in the codec-driver, e.g.
"ADC Input" ------------> "DAC" ^ | "AUDIO Regulator"
So whenever a stream is active this would lead to the event dapm_audioreg_event which will call the power_control_inc(w->codec), turning on the regulator, clock and power-up-register. This also means that the extra machine-driver interface for acc.det and vibra can also call the power_control_inc/power_control_dec.
Another example is the mic-regulators, e.g.
"MIC1A Input" --------> "Mic 1A or 1B Select Capture Route" -----> ..... ^ | "AMIC1A Regulator"
Activating a switch completing the Mic1a-path to an output would lead to the execution of the event in "AMIC1A Regulator": dapm_amic1areg_event. This leads to regulator_enable, enabling the regulator associated with the Mic1a.
So all paths in the codec-driver leads to an event in the machine-driver enabling/disabling the regulator.