On 03/14/2012 02:45 PM, Mark Brown wrote:

On Wed, Mar 14, 2012 at 02:27:13PM +0100, Ola LILJA2 wrote:

Fix your mailer to word wrap properly, I've reflowed your text for legibility.

...

...

...

diff --git a/sound/soc/codecs/ab8500_audio.c b/sound/soc/codecs/ab8500_audio.c

Everything else uses -codec.

Could just see one codec named with "-codec". The reason for appending _audio Is that the AB8500 is not only audio and we already have files elsewhere named ab8500.c but I probably could rename it to ab8500-codec.c.

The drivers for MFDs pretty much all call themselves -codec internally (as yours does).

OK, I thought you were comparing the namning of our file with the naming of other codec-files in the /soc/codecs-folder. But I guess that you are comparing the filename of our codec to the string we user for the device in the code. I'll rename it to ab8500-codec.c

...

...

Why are you doing this virtual register stuff? It's making the code a lot more complex and doesn't look at all driver specific.

There is a chain of events leading up to the decision of having it like this. The HW is designed so that only one coefficient-register is present and when we write a value to it the HW increases a HW-index so that the next time we write to it the value will set the next coefficient and so on. Also, we don't want to be able to first set all FIR/IIR-coefficients with the ALSA-controls And when we are happy we write all the coefficients to the HW by committing them using the strobe-control. Furthermore, we cannot read the coefficients from the HW, so to be able to provide this to userspace we use the SOC_HWDEP_MULTIPLE_1R, and since these values actually is not exposed as separate registers we use a set of virtual register. When we commit the coefficients we then take the values from the virtual registers and write them down to the HW in a loop.

Don't do this, this just makes things a lot more complicated in the register I/O code and making it much more difficult to work with both locally and from a framework point of view. Store the coefficients in your driver data rather than shoehorning them into the register cache.

OK...

...

...

...

+static const char * const enum_ena_dis[] = {"Enabled", "Disabled"}; +static const char * const enum_dis_ena[] = {"Disabled", "Enabled"};

Why are the controls using these enums and not Switch controls? UIs know how to display switches.

We need to make the switches virtual, as we don't want them to actually set any bits, just break or complete a DAPM-chain. That is why we made the as MUX:es.

This is completely orthogonal to how the controls are displayed to userspace, it's an implementation detail of your driver. Though if your routing control doesn't actually touch the device one has to wonder what it actually does...

I never found a way to have the playback-switch not touching any bits in the HW, so we used muxes instead. But if you say that is possible I will look into it again.

...

...

...

+/* Earpiece - Mute */ +static const struct snd_kcontrol_new dapm_ear_mute[] = {

• SOC_DAPM_SINGLE("Playback Switch", REG_MUTECONF,

+REG_MUTECONF_MUTEAR, 1, 1), };

This looks like it's just a mute rather than a mixer input enable (there's only one control...) so should be a regular sound control; unless there's a very good reason mutes other than mixer inputs normally don't affect the audio path as you might get pops and you probably don't want to do things like stop clocking just because the output is silenced. Similar thing applies in several other places.

Most of the mutes need to be apart of the DAPM-chain to actually prevent click-n-pops. So it cannot be used by the user as a normal ALSA-control. Muting can be done by setting certain gains to -inf.

This explanation doesn't correspond to what you've actually written - the code above will result in a user visible control.

That is why I said "most of", since this one was going to be converted to a virtual type (SND_SOC_ENUM_VIRT). And then we could use the bit REG_MUTECONF_MUTEAR in the DAPM-chain without having a userspace control setting it before the chain is getting executed. Also, the reason for not just changing control-types directly is because our customers are affected by these kind of changes so we need to do those changes at times where we can minimize the damge, have time to handle the effects and when customers actually accept that we do it (we try to diverge as little as possible from our main-branch).

...

...

...

• 	unsigned int fmt,
  

• 	unsigned int wl,
  

• 	unsigned int delay);
  

+void ab8500_audio_anc_configure(struct snd_soc_codec *codec,

• 	bool apply_fir, bool apply_iir);
  

No, you shouldn't be exporting this stuff - it all looks like basic framework stuff.

Hmm.. ok... the power_control is needed for reasons explained before (vibra-driver and Accessory-detection-magic), but I guess I have to remove these features for now and I can then remove this export.

You've not mentioned accessory detection before... there's certainly no obvious excuse for doing the power management for accessory detection outside of DAPM, we've got a bunch of drivers in mainline already which manage to do this quite successfully, but since you've not explained what the issue you think you see is it's hard to comment.

Accessory detection is just another external user not able to go through the user-space interface and due to the fact that the algorithms need to detect several different headset-types by turning on/off regulators and sampling the voltage on the input in specific sequences, we found no convenient way to do this since DAPM was controlling the regulators. We then introduced the _inc and _dec for the power of the audio-part (ab8500_audio_power_control), so that DAPM one-sided would not turn of the chip if, for example, vibra is on or accessory detection is detecting. We could remove both vibra and acc.det from the driver and put the regulator-control inside the codec-driver, but we would drift even further from what we actually use internally at ST-Ericsson and what our customers use.

On 03/15/2012 04:29 PM, Mark Brown wrote:

On Thu, Mar 15, 2012 at 03:50:41PM +0100, Ola Lilja wrote:

...

On 03/14/2012 02:45 PM, Mark Brown wrote:

...

On Wed, Mar 14, 2012 at 02:27:13PM +0100, Ola LILJA2 wrote:

Please include blank lines between paragraphs in your mail for legibility.

...

...

This is completely orthogonal to how the controls are displayed to userspace, it's an implementation detail of your driver. Though if your routing control doesn't actually touch the device one has to wonder what it actually does...

...

I never found a way to have the playback-switch not touching any bits in the HW, so we used muxes instead. But if you say that is possible I will look into it again.

If the hardware has no control the idiomatic thing would be to have a pin switch in the machine driver.

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. The only solution I've found is to use the enum_virt, but if there is a possibility to have a switch_virt I would be able to use that.

...

...

...

Most of the mutes need to be apart of the DAPM-chain to actually prevent click-n-pops. So it cannot be used by the user as a normal ALSA-control. Muting can be done by setting certain gains to -inf.

...

...

This explanation doesn't correspond to what you've actually written - the code above will result in a user visible control.

...

That is why I said "most of", since this one was going to be converted to a virtual type (SND_SOC_ENUM_VIRT). And then we could use the bit REG_MUTECONF_MUTEAR in the DAPM-chain without having a userspace control setting it before the chain is getting executed.

...

Also, the reason for not just changing control-types directly is because our customers are affected by these kind of changes so we need to do those changes at times where we can minimize the damge, have time to handle the effects and when customers actually accept that we do it (we try to diverge as little as possible from our main-branch).

This sounds like a very good reason to go for mainline early.

Yes, but it doesn't solve our current problems. (see first comment above regarding the enum_virt-stuff)

...

...

...

Hmm.. ok... the power_control is needed for reasons explained before (vibra-driver and Accessory-detection-magic), but I guess I have to remove these features for now and I can then remove this export.

You've not mentioned accessory detection before... there's certainly no obvious excuse for doing the power management for accessory detection outside of DAPM, we've got a bunch of drivers in mainline already which manage to do this quite successfully, but since you've not explained what the issue you think you see is it's hard to comment.

...

Accessory detection is just another external user not able to go through the user-space interface and due to the fact that the algorithms need to detect several different headset-types by turning on/off regulators and sampling the voltage on the input in specific sequences, we found no convenient way to do this since DAPM was controlling the regulators. We then introduced the _inc and _dec for the power of the audio-part (ab8500_audio_power_control), so that DAPM one-sided would not turn of the chip if, for example, vibra is on or accessory detection is detecting.

...

We could remove both vibra and acc.det from the driver and put the

Remember that the regulator API is reference counting, multiple things can control the same regulator without you having to layer another layer of reference counting and whatever else your code is doing on top of it. This is pretty essential for the regulator API, after all it's entirely normal for one regulator to supply many devices.

Yes, if it were only regulator (and clock) that was needed in our extra reference-counted power-functions, it would be fine. But vibra needs to be able to turn on the power of the audio-part of AB8500 (our codec) before it can use the vibra-functionality. I could move our the reg/clock and assume that the vibra-driver does this itself and only have the codec-power-register shared between the external interface and the DAPM.

Remember also that anything that goes into mainline is expected to have been peer reviewed and might get used as a reference by others. Things like this that aren't working with the kernel frameworks but instead layer on top of them really don't fit well with that.

...

regulator-control inside the codec-driver, but we would drift even further from what we actually use internally at ST-Ericsson and what our customers use.

Given the drivers you originally posted I expect you're already a substantial distance away from your internal code anyway... Perhaps you can use these issues internally as an example of the problems out of tree development that ends up with substantial non-framework code.

I will try (again) to foward the opinion that we should make greater efforts (and earlier) to be in sync with mainline-kernel. However, if we get this driver mainlined we can use all input to push harder for our next generation of platforms. This driver is for a project that has nearly finished its execution, but we would like to mainline this first before we go ahead with new drivers.

On 03/17/2012 11:31 PM, Mark Brown wrote:

On Fri, Mar 16, 2012 at 02:09:38PM +0100, Ola Lilja wrote:

...

On 03/15/2012 04:29 PM, Mark Brown wrote:

Please fix the word wrapping in your mail client...

Well, I though I did that when I moved over to Thunderbird and set the wordwrapping to 80 chars (no float). It would help if you told me exactly what settings it is you want, or provide a link to a description of what settings are required.

...

...

If the hardware has no control the idiomatic thing would be to have a pin switch in the machine driver.

...

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.

...

The only solution I've found is to use the enum_virt, but if there is a possibility to have a switch_virt I would be able to use that.

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?

...

Yes, if it were only regulator (and clock) that was needed in our extra reference-counted power-functions, it would be fine. But vibra needs to be able to turn on the power of the audio-part of AB8500 (our codec) before it can use the vibra-functionality. I could move our the reg/clock and assume that the vibra-driver does this itself and only have the codec-power-register shared between the external interface and the DAPM.

As I said previously there's other drivers with the same setup which manage to integrate fairly easily without breaking the device model - do what they do with something like an MFD or just embed a trivial input driver in the CODEC for the vibra if it's small enough.

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? 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.

...

(and earlier) to be in sync with mainline-kernel. However, if we get this driver mainlined we can use all input to push harder for our next generation of platforms. This driver is for a project that has nearly finished its execution, but we would like to mainline this first before we go ahead with new drivers.

Equally well, if it gets mainline without actually meeting mainline standards that's sending the wrong message :)

I'd suggest at least adding regulator support into the CODEC driver so it's managing its own power when required, there's no reason for anything external to be doing that. I expect if you bring things into line with the device model the code will end up being a lot smaller and clearer and this won't be needed, it feels like a large part of why you're having to open code all this stuff is that other bits of the code stepped outside the frameworks.

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 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? 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 DAPM-widget which is defined in the machine-driver and connected to the chains in the codec-driver. The event in the widget will then call the _inc/_dec-functions in the machine-driver. All other regulators (analog/digital mic-bias) are also part of the DAPM-chains and turned on/off by the framework just as it should. 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.

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.

...

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.

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.

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.

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...

On Mon, Mar 19, 2012 at 03:54:19PM +0100, Ola Lilja wrote:

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.

This looks very standard, you've got a PGA at B and either further PGAs or output drivers at C and D.

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.

It sounds like this non-specific register bit is just the power control for C in which case it should just be a DAPM widget and machines should be using a pin switch or jack pins for the outputs.

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.

Again, this looks *very* standard - it's just a totally normal bypass/sidetone path as far as I can see. B is a mixer here and presumably there's some control on B which turns on and off the path?

Depending on how the path is used you might want to mark the route as a weak route to suppress power up from that path alone but really as with the first example this doesn't seem at all unusual unless there's more going on than your description.

...

...

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?

The device model is this whole idea that we have devices which are matched up automatically by the core. "Embedding" means "putting in" - see for example wm8962 which has a tiny little input driver in it.

...

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

You've completely failed to understand how requesting supplies in the regulator API works, a regulator API like you describe would mean that no chip driver was ever able to request a supply for itself. The whole point of the machine interface is to provide a mechanism for drivers to talk about their supplies without having to know the details of how they're wired up on an individual board.

Since your CODEC driver is a driver for a chip your CODEC driver should be requesting whatever the supplies the chip has.

...

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.

I'm sorry but really I don't see anything in the text below which sounds in the least bit unusual. Could you be specific about what you think is special about your device and/or system?

...

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:

Things that are in the machine driver are not in the CODEC driver and need to be cut and pasted into individual machine drivers. If it's something that's genuinely machine specific that's fine but we're talking about basic core device supplies here.