[alsa-devel] [PATCH 1/2] ASoC: AD1980/AD73311: make sure to set dev member
The codec driver should initialize snd_soc_codec's dev member to the appropriate device when setting up the device, but these codecs weren't.
Signed-off-by: Mike Frysinger vapier@gentoo.org --- sound/soc/codecs/ad1980.c | 1 + sound/soc/codecs/ad73311.c | 1 + 2 files changed, 2 insertions(+), 0 deletions(-)
diff --git a/sound/soc/codecs/ad1980.c b/sound/soc/codecs/ad1980.c index d7440a9..0477a91 100644 --- a/sound/soc/codecs/ad1980.c +++ b/sound/soc/codecs/ad1980.c @@ -203,6 +203,7 @@ static int ad1980_soc_probe(struct platform_device *pdev) codec->reg_cache_size = sizeof(u16) * ARRAY_SIZE(ad1980_reg); codec->reg_cache_step = 2; codec->name = "AD1980"; + codec->dev = &pdev->dev; codec->owner = THIS_MODULE; codec->dai = &ad1980_dai; codec->num_dai = 1; diff --git a/sound/soc/codecs/ad73311.c b/sound/soc/codecs/ad73311.c index e61dac5..6e18149 100644 --- a/sound/soc/codecs/ad73311.c +++ b/sound/soc/codecs/ad73311.c @@ -50,6 +50,7 @@ static int ad73311_soc_probe(struct platform_device *pdev) return -ENOMEM; mutex_init(&codec->mutex); codec->name = "AD73311"; + codec->dev = &pdev->dev; codec->owner = THIS_MODULE; codec->dai = &ad73311_dai; codec->num_dai = 1;
Since these drivers rely on a SPI master and fail if the SPI functions aren't present, make sure the Kconfig reflects this dependency.
Signed-off-by: Mike Frysinger vapier@gentoo.org --- sound/soc/blackfin/Kconfig | 4 ++-- 1 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/sound/soc/blackfin/Kconfig b/sound/soc/blackfin/Kconfig index 97f1a25..b91e234 100644 --- a/sound/soc/blackfin/Kconfig +++ b/sound/soc/blackfin/Kconfig @@ -43,7 +43,7 @@ config SND_BF5XX_TDM
config SND_BF5XX_SOC_AD1836 tristate "SoC AD1836 Audio support for BF5xx" - depends on SND_BF5XX_TDM + depends on SND_BF5XX_TDM && SPI_MASTER select SND_BF5XX_SOC_TDM select SND_SOC_AD1836 help @@ -51,7 +51,7 @@ config SND_BF5XX_SOC_AD1836
config SND_BF5XX_SOC_AD1938 tristate "SoC AD1938 Audio support for Blackfin" - depends on SND_BF5XX_TDM + depends on SND_BF5XX_TDM && SPI_MASTER select SND_BF5XX_SOC_TDM select SND_SOC_AD1938 help
On Tue, Oct 06, 2009 at 01:51:51AM -0400, Mike Frysinger wrote:
Since these drivers rely on a SPI master and fail if the SPI functions aren't present, make sure the Kconfig reflects this dependency.
Signed-off-by: Mike Frysinger vapier@gentoo.org
Could you please redo this as a dependency on the relevant SPI controller driver? The audio driver isn't going to work without that anyway so it seems more sensible.
On Tue, Oct 6, 2009 at 05:33, Mark Brown wrote:
On Tue, Oct 06, 2009 at 01:51:51AM -0400, Mike Frysinger wrote:
Since these drivers rely on a SPI master and fail if the SPI functions aren't present, make sure the Kconfig reflects this dependency.
Could you please redo this as a dependency on the relevant SPI controller driver? The audio driver isn't going to work without that anyway so it seems more sensible.
there is no dependency on a specific spi bus master implementation. just like the i2c framework, such a dependency would be against the whole purpose of having a generic framework in the first place. fortunately, this is one piece of the Blackfin ASoC drivers done correctly. in other words, they should work just fine if hooked up to a GPIO SPI bus or a SPORT SPI bus or a Blackfin on-chip SPI bus or ...... -mike
On Tue, Oct 06, 2009 at 05:41:40AM -0400, Mike Frysinger wrote:
there is no dependency on a specific spi bus master implementation. just like the i2c framework, such a dependency would be against the whole purpose of having a generic framework in the first place. fortunately, this is one piece of the Blackfin ASoC drivers done correctly. in other words, they should work just fine if hooked up to a GPIO SPI bus or a SPORT SPI bus or a Blackfin on-chip SPI bus or ......
These are board-specific drivers to hook things up on a given system (presumably various eval boards), they're the non-generic part of the system that says how the generic DAI and CODEC drivers have been hooked up.
On Tue, Oct 6, 2009 at 05:50, Mark Brown wrote:
On Tue, Oct 06, 2009 at 05:41:40AM -0400, Mike Frysinger wrote:
there is no dependency on a specific spi bus master implementation. just like the i2c framework, such a dependency would be against the whole purpose of having a generic framework in the first place. fortunately, this is one piece of the Blackfin ASoC drivers done correctly. in other words, they should work just fine if hooked up to a GPIO SPI bus or a SPORT SPI bus or a Blackfin on-chip SPI bus or ......
These are board-specific drivers to hook things up on a given system (presumably various eval boards), they're the non-generic part of the system that says how the generic DAI and CODEC drivers have been hooked up.
except the Blackfin machine drivers are written in such a way that they arent specific to a board. they'll work on any system with a SPORT and a SPI/I2C bus. -mike
On Tue, Oct 06, 2009 at 06:00:23AM -0400, Mike Frysinger wrote:
On Tue, Oct 6, 2009 at 05:50, Mark Brown wrote:
These are board-specific drivers to hook things up on a given system (presumably various eval boards), they're the non-generic part of the system that says how the generic DAI and CODEC drivers have been hooked up.
except the Blackfin machine drivers are written in such a way that they arent specific to a board. they'll work on any system with a SPORT and a SPI/I2C bus.
That's not 100% clear - some of the drivers say they're specific to particular designs (either in the help text or by having board-specific GPIO setup). It's certainly the intention of the API that the board hookups be fixed at run time rather than at compile time, and that things like using multiple CPU DAIs should be possible.
The other option here is to make the CODEC drivers build without their control buses, which keeps the people doing random configurations happy.
On Tue, Oct 6, 2009 at 06:39, Mark Brown wrote:
On Tue, Oct 06, 2009 at 06:00:23AM -0400, Mike Frysinger wrote:
On Tue, Oct 6, 2009 at 05:50, Mark Brown wrote:
These are board-specific drivers to hook things up on a given system (presumably various eval boards), they're the non-generic part of the system that says how the generic DAI and CODEC drivers have been hooked up.
except the Blackfin machine drivers are written in such a way that they arent specific to a board. they'll work on any system with a SPORT and a SPI/I2C bus.
That's not 100% clear - some of the drivers say they're specific to particular designs (either in the help text or by having board-specific GPIO setup). It's certainly the intention of the API that the board hookups be fixed at run time rather than at compile time, and that things like using multiple CPU DAIs should be possible.
The other option here is to make the CODEC drivers build without their control buses, which keeps the people doing random configurations happy.
yes, there are many descriptions which are way more specific than they should be. the issue stems from people developing with a narrow focus rather than keeping the big picture in their head. they test one specific configuration and then just bang out the name/info with that. ive been trying to stem the flow and clean up existing code, but it doesnt always work out.
AD1836 is an example of this -- the Kconfig text says "BF5xx", but that's useless because that covers all Blackfin parts. "STAMP/EZKIT" is a lie -- there is no dependency on these base board designs. the machine driver only needs a SPORT (TDM) for transport and the codec needs SPI for configuration. every Blackfin part has at least one SPORT. but the core SPORT handling is the crux of the matter -- as i mentioned in another thread, our implementation is severely limited in terms of multiple instances and is in need of restructuring.
the GPIO handling is made "generic" by putting it into the Kconfig. so the machine-specific aspect is now in the .config file instead of the machine driver. really this needs to get pushed out into the board resources (another bad habit that we've been reigning in -- encoding flexible resources in Kconfig).
back to the original issue. the AD1836/AD1938 have their registers programed via SPI, but they dont care what SPI bus they're connected to. that is specified in the board resources. because of the way the Kconfig options are handled (machine drivers select codecs), the SPI_MASTER dependency cannot be added to the codec Kconfigs in codecs/Kconfig. so even though the SPI dependency is only in the codec and not the machine driver, the machine driver needs to declare the SPI_MASTER dependency to prevent incorrect config selections and link failures. -mike
On Tue, Oct 06, 2009 at 07:32:30AM -0400, Mike Frysinger wrote:
back to the original issue. the AD1836/AD1938 have their registers programed via SPI, but they dont care what SPI bus they're connected to. that is specified in the board resources. because of the way the Kconfig options are handled (machine drivers select codecs), the SPI_MASTER dependency cannot be added to the codec Kconfigs in codecs/Kconfig. so even though the SPI dependency is only in the codec and not the machine driver, the machine driver needs to declare the SPI_MASTER dependency to prevent incorrect config selections and link failures.
Or, like I say, the drivers for the CODEC should be changed to allow build with no control bus. This is a general problem for all the ASoC drivers and I'm still not sure which way to go. Depending on just SPI support fixes the random build case but doesn't help end users get the driver working so it doesn't seem so useful. If we're only going to be able to fix the random build case then it seems more useful to do so by having the CODEC driver able to build without the control bus, that way individual machine drivers don't need to worry about it.
On Tue, Oct 6, 2009 at 07:52, Mark Brown wrote:
On Tue, Oct 06, 2009 at 07:32:30AM -0400, Mike Frysinger wrote:
back to the original issue. the AD1836/AD1938 have their registers programed via SPI, but they dont care what SPI bus they're connected to. that is specified in the board resources. because of the way the Kconfig options are handled (machine drivers select codecs), the SPI_MASTER dependency cannot be added to the codec Kconfigs in codecs/Kconfig. so even though the SPI dependency is only in the codec and not the machine driver, the machine driver needs to declare the SPI_MASTER dependency to prevent incorrect config selections and link failures.
Or, like I say, the drivers for the CODEC should be changed to allow build with no control bus. This is a general problem for all the ASoC drivers and I'm still not sure which way to go. Depending on just SPI support fixes the random build case but doesn't help end users get the driver working so it doesn't seem so useful. If we're only going to be able to fix the random build case then it seems more useful to do so by having the CODEC driver able to build without the control bus, that way individual machine drivers don't need to worry about it.
i dont see how abstracting away the bus such that codec drivers are allowed to build without proper bus support is useful. the AD1836/AD1938 only work with the SPI bus, so if support for that is disabled, having the driver compiled and installed is pointless. -mike
On Tue, Oct 06, 2009 at 05:09:42PM -0400, Mike Frysinger wrote:
i dont see how abstracting away the bus such that codec drivers are allowed to build without proper bus support is useful. the AD1836/AD1938 only work with the SPI bus, so if support for that is disabled, having the driver compiled and installed is pointless.
Both approaches allow drivers to be built which can't actually be used since neither guarantees that there will be a driver for the SPI controller. Either way all we're doing is making sure that the kernel will build, the user can still build things so that the audio driver won't do anything useful.
On Tue, Oct 6, 2009 at 18:07, Mark Brown wrote:
On Tue, Oct 06, 2009 at 05:09:42PM -0400, Mike Frysinger wrote:
i dont see how abstracting away the bus such that codec drivers are allowed to build without proper bus support is useful. the AD1836/AD1938 only work with the SPI bus, so if support for that is disabled, having the driver compiled and installed is pointless.
Both approaches allow drivers to be built which can't actually be used since neither guarantees that there will be a driver for the SPI controller. Either way all we're doing is making sure that the kernel will build, the user can still build things so that the audio driver won't do anything useful.
hmm, that's certainly true. if we look at it as "screwed either way", then going to a generic bus indirection sounds like it'd only add runtime overhead for no real gain ?
in the face of this proposed effort being a ways off, doesnt it make sense to still merge the original proposed patch ? -mike
On Wed, Oct 07, 2009 at 04:32:06AM -0400, Mike Frysinger wrote:
hmm, that's certainly true. if we look at it as "screwed either way", then going to a generic bus indirection sounds like it'd only add runtime overhead for no real gain ?
I was mostly thinking ifdefs here - we'll always need some bus-specific stuff in the drivers to register them. It's not pretty but it meets the needs of people doing randconfig builds.
We already have as much bus indirection as ASoC needs, and there is actually already some bus access code sharing there as of 2.6.32 (in soc-cache.c) but it's optional and always will be since we need to cater for devices that are parts of MFDs which have device specific register acceses.
in the face of this proposed effort being a ways off, doesnt it make sense to still merge the original proposed patch ?
The ifdefery isn't technically hard to do and given your use case where you don't know which controller is in use it looks like the only way to go for this.
On Wed, Oct 7, 2009 at 06:19, Mark Brown wrote:
On Wed, Oct 07, 2009 at 04:32:06AM -0400, Mike Frysinger wrote:
hmm, that's certainly true. if we look at it as "screwed either way", then going to a generic bus indirection sounds like it'd only add runtime overhead for no real gain ?
I was mostly thinking ifdefs here - we'll always need some bus-specific stuff in the drivers to register them. It's not pretty but it meets the needs of people doing randconfig builds.
We already have as much bus indirection as ASoC needs, and there is actually already some bus access code sharing there as of 2.6.32 (in soc-cache.c) but it's optional and always will be since we need to cater for devices that are parts of MFDs which have device specific register acceses.
in the face of this proposed effort being a ways off, doesnt it make sense to still merge the original proposed patch ?
The ifdefery isn't technically hard to do and given your use case where you don't know which controller is in use it looks like the only way to go for this.
i dont think the soc-cache could be used currently by the ad1836 as it doesnt use a 7/9 split with the address/data. it uses like 6/10 (4 addr bits, one bit for read/write, one bit is always 0, and 10 data bits). guessing the write bit can be considered part of the addr as the read always comes from the cache, but that still gives us 5/10 split. maybe a new 6/10 set of funcs should be added ...
snd_soc_7_9_write() looks like it does a little more bit work than it needs to ? if data is declared as a u16, then you have: u16 data = (reg << 9) | (value & 0x01ff); this is what the ad1836 driver does now for its data split.
in the mean time, rather than adding #ifdef to the codec driver, we could create a local header like "bus-stubs.h" that stubs all the relevant functions to an error value. then all codec drivers that dont use soc-cache can use that instead and the only change needed is to add: #include "bus-stubs.h" -mike
On Wed, Oct 07, 2009 at 08:27:56PM -0400, Mike Frysinger wrote:
i dont think the soc-cache could be used currently by the ad1836 as it doesnt use a 7/9 split with the address/data. it uses like 6/10 (4 addr bits, one bit for read/write, one bit is always 0, and 10 data bits). guessing the write bit can be considered part of the addr as the read always comes from the cache, but that still gives us 5/10 split. maybe a new 6/10 set of funcs should be added ...
That's the idea - add new functions for any new register formats.
snd_soc_7_9_write() looks like it does a little more bit work than it needs to ? if data is declared as a u16, then you have: u16 data = (reg << 9) | (value & 0x01ff); this is what the ad1836 driver does now for its data split.
Probably. I'd need to check but I believe that's there to handle endianness variations in the host, though a cpu_to_ in what you have above ought to be able to take care of that. The code was cut'n'pasted from what was in the drivers already.
in the mean time, rather than adding #ifdef to the codec driver, we could create a local header like "bus-stubs.h" that stubs all the relevant functions to an error value. then all codec drivers that dont use soc-cache can use that instead and the only change needed is to add: #include "bus-stubs.h"
I'm not sure I feel up to doing that locally in ASoC rather than in the relevant subsystems.
On Thu, Oct 8, 2009 at 06:01, Mark Brown wrote:
On Wed, Oct 07, 2009 at 08:27:56PM -0400, Mike Frysinger wrote:
i dont think the soc-cache could be used currently by the ad1836 as it doesnt use a 7/9 split with the address/data. it uses like 6/10 (4 addr bits, one bit for read/write, one bit is always 0, and 10 data bits). guessing the write bit can be considered part of the addr as the read always comes from the cache, but that still gives us 5/10 split. maybe a new 6/10 set of funcs should be added ...
That's the idea - add new functions for any new register formats.
i'll add a tracker item to get the ad codecs converted to soc-cache when possible and that'll naturally address the SPI dependency
snd_soc_7_9_write() looks like it does a little more bit work than it needs to ? if data is declared as a u16, then you have: u16 data = (reg << 9) | (value & 0x01ff); this is what the ad1836 driver does now for its data split.
Probably. I'd need to check but I believe that's there to handle endianness variations in the host, though a cpu_to_ in what you have above ought to be able to take care of that. The code was cut'n'pasted from what was in the drivers already.
true ... being little endian sometimes makes you forget about these things ;). the ad1836 codec probably doesnt work on BE systems.
in the mean time, rather than adding #ifdef to the codec driver, we could create a local header like "bus-stubs.h" that stubs all the relevant functions to an error value. then all codec drivers that dont use soc-cache can use that instead and the only change needed is to add: #include "bus-stubs.h"
I'm not sure I feel up to doing that locally in ASoC rather than in the relevant subsystems.
i dont feel like convincing subsystem maintainers that this is a good idea for all consumers of SPI/I2C, especially since i'm not convinced myself. we'll just go the soc-cache route and not worry about it. -mike
On Thu, Oct 8, 2009 at 6:01 PM, Mark Brown broonie@opensource.wolfsonmicro.com wrote:
On Wed, Oct 07, 2009 at 08:27:56PM -0400, Mike Frysinger wrote:
i dont think the soc-cache could be used currently by the ad1836 as it doesnt use a 7/9 split with the address/data. it uses like 6/10 (4 addr bits, one bit for read/write, one bit is always 0, and 10 data bits). guessing the write bit can be considered part of the addr as the read always comes from the cache, but that still gives us 5/10 split. maybe a new 6/10 set of funcs should be added ...
That's the idea - add new functions for any new register formats.
we do those based on the idea that it is a waste all codecs need to use almost same ways to handle register access. If we use soc-cache to handle register access issues and run as abstract layer for all codecs, why not rename it to soc-reg or soc-bus? It seems cache is only a little part of the responsibility of this module. It's better that functions like xx_7_9_read xx_7_9_write xx_8_8_read xx_8_8_write should not become API because we never know what will be the format for codecs. We should have a xx_n_m_read/xx_n_m_write or just a xx_read/xx_write, with n and m as parameters? A codec using snd_soc_7_9_spi_write/snd_soc_8_16_read_i2c should select SPI and I2C in fact. Otherwise, why does it use these functions as codec->hw_read()/codec->hw_write() but not enable SPI and I2C? It seems that defining snd_soc_7_9_spi_write and so on as NULL when SPI is not selected is really useless, just let us pass the compiling to get a module which can't work at all!
snd_soc_7_9_write() looks like it does a little more bit work than it needs to ? if data is declared as a u16, then you have: u16 data = (reg << 9) | (value & 0x01ff); this is what the ad1836 driver does now for its data split.
Probably. I'd need to check but I believe that's there to handle endianness variations in the host, though a cpu_to_ in what you have above ought to be able to take care of that. The code was cut'n'pasted from what was in the drivers already.
in the mean time, rather than adding #ifdef to the codec driver, we could create a local header like "bus-stubs.h" that stubs all the relevant functions to an error value. then all codec drivers that dont use soc-cache can use that instead and the only change needed is to add: #include "bus-stubs.h"
I'm not sure I feel up to doing that locally in ASoC rather than in the relevant subsystems. _______________________________________________ Alsa-devel mailing list Alsa-devel@alsa-project.org http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
On Sat, Oct 10, 2009 at 11:47:10AM +0800, Barry Song wrote:
On Thu, Oct 8, 2009 at 6:01 PM, Mark Brown
That's the idea - add new functions for any new register formats.
we do those based on the idea that it is a waste all codecs need to use almost same ways to handle register access. If we use soc-cache to
Not just that, the goal is to make it easier to make changes to the way register accesses are managed by avoiding the need to go through each and every single driver and update it. The most obvious thing is adding support for powering off the CODEC rather than just bringing it down to standby.
handle register access issues and run as abstract layer for all codecs, why not rename it to soc-reg or soc-bus? It seems cache is only a little part of the responsibility of this module.
soc-reg (or probably register) would do equally well, like I say longer term the goal is to support doing interesting things with the cache. I don't know that it's worth renaming at this point.
It's better that functions like xx_7_9_read xx_7_9_write xx_8_8_read xx_8_8_write should not become API because we never know what will be the format for codecs. We should have a xx_n_m_read/xx_n_m_write or just a xx_read/xx_write, with n and m as parameters?
They're not APIs, they're only visible within the file. Drivers using the cache only see the one function they use to initialise the cache with everything else in there hidden from them. They're not specified as part of the interface for the read and write functions because they'd be a lot of noise for callers - they don't generally change at runtime and are something that should really be abstracted away from the generic code anyway.
A codec using snd_soc_7_9_spi_write/snd_soc_8_16_read_i2c should select SPI and I2C in fact. Otherwise, why does it use these functions
The CODECs can't do this since they are selected themselves and Kconfig ignores all dependencies from things that are selected. In any case, a given board is only going to need one or the other of the control interfaces so the CODEC driver needs to leave this up to the machine drivers anyway.
The register cache code is the wrong level to be making decisions about things like this, the CODEC drivers and the register cache code are themselves not usable without a machine driver - this is utility code which should just do whatever it is asked by its users rather than forcing decisons on them.
as codec->hw_read()/codec->hw_write() but not enable SPI and I2C? It seems that defining snd_soc_7_9_spi_write and so on as NULL when SPI is not selected is really useless, just let us pass the compiling to get a module which can't work at all!
It's no use from the point of view of getting a working driver, yes, but then to get something useful you really need the machine driver to ensure that the correct controller driver is being built - simple support for the bus is not enough, we also need the controller. As discussed earlier in this thread for some of the Blackfin machines it's not even possible to do that since we can't tell which controller driver needs to be used since the machine driver can be used with many boards.
Another thing to bear in mind here is that there are people who do things like build the kernel with random .configs and we want to avoid breaking their builds due to poor luck in the configuration they generate while still getting the build coverage that results. We also need to be able to support compilation with either I2C or SPI but not both in order to avoid forcing boards to include support for a bus that may not be used anywhere on the board at all. That means that both buses need to be individually optional at the CODEC level.
On Tue, Oct 06, 2009 at 01:51:50AM -0400, Mike Frysinger wrote:
The codec driver should initialize snd_soc_codec's dev member to the appropriate device when setting up the device, but these codecs weren't.
Signed-off-by: Mike Frysinger vapier@gentoo.org
No, this is the wrong approach - codec->dev should never be being set to the socdev device, it should be being set to the device model device for the CODEC (as should the dev members for the DAIs). For ad73311 this means it needs to be converted to use proper device model instantiation, see wm8731 for an example of doing this. ad1980 should be handled by generic AC97 code in the core.
On Tue, Oct 6, 2009 at 05:42, Mark Brown wrote:
On Tue, Oct 06, 2009 at 01:51:50AM -0400, Mike Frysinger wrote:
The codec driver should initialize snd_soc_codec's dev member to the appropriate device when setting up the device, but these codecs weren't.
No, this is the wrong approach - codec->dev should never be being set to the socdev device, it should be being set to the device model device for the CODEC (as should the dev members for the DAIs). For ad73311 this means it needs to be converted to use proper device model instantiation, see wm8731 for an example of doing this. ad1980 should be handled by generic AC97 code in the core.
meh, this sounds like effort. i'll trick Cliff/Barry into doing it ;). -mike
participants (4)
-
Barry Song -
Mark Brown -
Mike Frysinger -
Mike Frysinger