[alsa-devel] Tegra ASoC, multiple boards, platform data, and conditionals in machine driver
Mark,
I'm thinking of upstreaming ASoC support for some of the other Tegra boards that got added later in 2.6.39; Seaboard, Kaen, and Aebl.
[Ventana could also be supported, but it's identical to Harmony. I imagine people will want Toshiba AC100 and Trimslice support in the not-too- distant future, but I don't have schematics for those boards at present]
This raises a couple questions:
1)
Currently, the Harmony machine driver gets all its GPIO information from platform data defined in arch/arch/mach-tegra/include/mach/harmony_audio.h. Supporting new machines would entail one of:
a) Creating a new header for each new machine (or set of similar machines). This could mean many headers pretty quickly.
b) Expanding that structure to include extra fields, thus making it generic enough to support any Tegra machine, which would probably also entail renaming the file to not be Harmony-specific. Some GPIO fields would be optional (i.e. initialized to -1) if a feature wasn't available on a given board.
c) Perhaps switch to using platform_get_resource() to retrieve the data rather than custom per-machine/SoC structures, although that'd require adding new resource types for GPIOs etc.
That said, I noticed that all(?) the other machine drivers simply use hard-coded GPIO numbers (sometimes defined locally, probably sometimes from machine header files), rather than having the values passed in through platform_data. This eliminates the need for platform_data structures etc. Is this simply a legacy because ASoC machine drivers weren't able to be first-class platform_devices, or is this still an acceptable style?
I'm tempted to convert the Tegra Harmony driver to this style (but including headers to define the GPIO names instead of duplicating definitions in the machine file), and also use it for new machines.
2)
Seaboard has a couple of derivatives which are very similar, but do have some differences in the audio path and elsewhere. In the ChromeOS kernel, we have a single ASoC machine driver covering Seaboard and two derivatives, with conditional code. I've placed an example at the end of this mail. Do you have a preference for having separate files for each machine without the conditional code v.s. a single machine driver with a lot of conditional code? If we go the conditional route, it might even be possible to merge the Harmony and Seaboard machine drivers into one.
Thanks for any feedback.
Example from static int seaboard_asoc_init():
ret = gpio_request(pdata->gpio_spkr_en, "spkr_en"); if (ret) { dev_err(card->dev, "cannot get spkr_en gpio\n"); return ret; } seaboard->gpio_requested |= GPIO_SPKR_EN;
gpio_direction_output(pdata->gpio_spkr_en, 0);
if (pdata->gpio_hp_mute != -1) { ret = gpio_request(pdata->gpio_hp_mute, "hp_mute"); if (ret) { dev_err(card->dev, "cannot get hp_mute gpio\n"); return ret; } seaboard->gpio_requested |= GPIO_HP_MUTE;
gpio_direction_output(pdata->gpio_hp_mute, 0); } ... if (machine_is_seaboard()) { snd_soc_dapm_add_routes(dapm, seaboard_audio_map, ARRAY_SIZE(seaboard_audio_map)); } else if (machine_is_kaen()) { snd_soc_dapm_add_routes(dapm, kaen_audio_map, ARRAY_SIZE(kaen_audio_map)); } else { snd_soc_dapm_add_routes(dapm, aebl_audio_map, ARRAY_SIZE(aebl_audio_map)); } ... snd_soc_dapm_nc_pin(dapm, "IN1L"); if (machine_is_kaen()) snd_soc_dapm_nc_pin(dapm, "IN1R"); snd_soc_dapm_nc_pin(dapm, "IN2L"); if (!machine_is_kaen()) snd_soc_dapm_nc_pin(dapm, "IN2R"); snd_soc_dapm_nc_pin(dapm, "IN2L"); snd_soc_dapm_nc_pin(dapm, "IN3R"); snd_soc_dapm_nc_pin(dapm, "IN3L");
if (machine_is_aebl()) { snd_soc_dapm_nc_pin(dapm, "LON"); snd_soc_dapm_nc_pin(dapm, "RON"); snd_soc_dapm_nc_pin(dapm, "ROP"); snd_soc_dapm_nc_pin(dapm, "LOP"); } else { snd_soc_dapm_nc_pin(dapm, "LINEOUTR"); snd_soc_dapm_nc_pin(dapm, "LINEOUTL"); }
On Fri, Apr 08, 2011 at 12:43:37PM -0700, Stephen Warren wrote:
That said, I noticed that all(?) the other machine drivers simply use hard-coded GPIO numbers (sometimes defined locally, probably sometimes from machine header files), rather than having the values passed in through platform_data. This eliminates the need for platform_data structures etc. Is this simply a legacy because ASoC machine drivers weren't able to be first-class platform_devices, or is this still an acceptable style?
Partly due to the historical reasons, partly due to the fact that unless you actually have multiple boards based off the same design with different data there's no point in bouncing the data through platform data as there's only one possible answer - the hard coding also includes the connections between the devices, the clocking arrangements and so on.
Seaboard has a couple of derivatives which are very similar, but do have some differences in the audio path and elsewhere. In the ChromeOS kernel, we have a single ASoC machine driver covering Seaboard and two derivatives, with conditional code. I've placed an example at the end of this mail. Do you have a preference for having separate files for each machine without the conditional code v.s. a single machine driver with a lot of conditional code? If we go the conditional route, it might even be possible to merge the Harmony and Seaboard machine drivers into one.
If the machines are very similar merging the driver is good. Your code looked like a good example of where merging is helpful.
Colin, Erik, Olof,
Do you have any objections to moving arch/arm/mach-tegra/board-*.h and gpio-names.h into arch/arch/mach-tegra/include/mach so that the audio driver source can include those headers simply?
See the thread below for background.
If this is OK, I'll submit the rename as part of my patch-set, and have Mark process it through a separate branch that both the ASoC and Tegra trees can both merge.
Thanks.
Mark Brown wrote at Friday, April 08, 2011 8:31 PM:
On Fri, Apr 08, 2011 at 12:43:37PM -0700, Stephen Warren wrote:
That said, I noticed that all(?) the other machine drivers simply use hard-coded GPIO numbers (sometimes defined locally, probably sometimes from machine header files), rather than having the values passed in through platform_data. This eliminates the need for platform_data structures etc. Is this simply a legacy because ASoC machine drivers weren't able to be first-class platform_devices, or is this still an acceptable style?
Partly due to the historical reasons, partly due to the fact that unless you actually have multiple boards based off the same design with different data there's no point in bouncing the data through platform data as there's only one possible answer - the hard coding also includes the connections between the devices, the clocking arrangements and so on.
Seaboard has a couple of derivatives which are very similar, but do have some differences in the audio path and elsewhere. In the ChromeOS kernel, we have a single ASoC machine driver covering Seaboard and two derivatives, with conditional code. I've placed an example at the end of this mail. Do you have a preference for having separate files for each machine without the conditional code v.s. a single machine driver with a lot of conditional code? If we go the conditional route, it might even be possible to merge the Harmony and Seaboard machine drivers into one.
If the machines are very similar merging the driver is good. Your code looked like a good example of where merging is helpful.
Hi,
On Sat, Apr 9, 2011 at 7:52 PM, Stephen Warren swarren@nvidia.com wrote:
Colin, Erik, Olof,
Do you have any objections to moving arch/arm/mach-tegra/board-*.h and gpio-names.h into arch/arch/mach-tegra/include/mach so that the audio driver source can include those headers simply?
See the thread below for background.
I would prefer to use platform_data and export the information through there.
I know ASoC is in some sense an exception to the rule, but in general there should be no need for any code outside of arch/arm/mach-tegra to have to know anything about the board it is running on, drivers should be generic enough. Some other platforms use a lot of shared header files between platform code and drivers, and I have always found that to be very awkward to follow when reading code, especially if the same constants are defined in multiple places depending on what board is built. It also makes it harder to maintain a multiplatform kernel if configuration data is pulled in at build time instead of runtime (i.e. headers vs platform_data or similar).
In other words, I would prefer option (b) above.
-Olof
On Sun, Apr 10, 2011 at 11:18:17AM -0700, Olof Johansson wrote:
built. It also makes it harder to maintain a multiplatform kernel if configuration data is pulled in at build time instead of runtime (i.e. headers vs platform_data or similar).
Note that whatever option is chosen the configuration should be pulled in at runtime, it's purely a question of where the data table goes - do you pass the full thing through platform datra or do you just pass the machine type.
In other words, I would prefer option (b) above.
Another thing to bear in mind is that it's only feasable to try to combine the Tegra/WM8903 combinations - trying to combine machine drivers for multiple unrelated CODECs isn't going to work well.
Mark Brown wrote at Sunday, April 10, 2011 7:58 PM:
On Sun, Apr 10, 2011 at 11:18:17AM -0700, Olof Johansson wrote:
built. It also makes it harder to maintain a multiplatform kernel if configuration data is pulled in at build time instead of runtime (i.e. headers vs platform_data or similar).
Note that whatever option is chosen the configuration should be pulled in at runtime, it's purely a question of where the data table goes - do you pass the full thing through platform datra or do you just pass the machine type.
Well, I was thinking of eliminating platform data completely; the selection of the correct ASoC machine driver would be based on the name of the platform device, and the machine driver would use machine_is_*() to determine any machine-specific actions/data. So yes, at run-time, but not using runtime-determined data *tables* at all.
Olof Johansson wrote at Sunday, April 10, 2011 12:18 PM:
On Sat, Apr 9, 2011 at 7:52 PM, Stephen Warren swarren@nvidia.com wrote:
Colin, Erik, Olof,
Do you have any objections to moving arch/arm/mach-tegra/board-*.h and gpio-names.h into arch/arch/mach-tegra/include/mach so that the audio driver source can include those headers simply?
See the thread below for background.
I would prefer to use platform_data and export the information through there.
I know ASoC is in some sense an exception to the rule, but in general there should be no need for any code outside of arch/arm/mach-tegra to have to know anything about the board it is running on, drivers should be generic enough.
In the ASoC case, the whole point of the driver is to be for a specific board, or perhaps set of related/similar boards. This includes defining which CPU DMA engine and which audio codec to hook up. So there isn't really a possibility of being more generic.
Some other platforms use a lot of shared header files between platform code and drivers, and I have always found that to be very awkward to follow when reading code, especially if the same constants are defined in multiple places depending on what board is built.
The whole point of having the ASoC driver include the platform's headers was to avoid defining the same GPIO constants in multiple places.
It also makes it harder to maintain a multiplatform kernel if configuration data is pulled in at build time instead of runtime (i.e. headers vs platform_data or similar).
That's true. I was thinking this through in more detail last night, and did realize that if the ASoC driver includes <mach/board-harmony.h>, then defining what <mach/> means in a multi-SoC kernel is going to be problematic.
Perhaps the best approach is to:
* Keep a single machine driver (at least for Tegra+WM8903; as Mark noted, attempting to share any wider doesn't make sense)
* Rename the driver e.g. tegra_wm8903.c rather than harmony.c since it'll be more generic. Rename the platform data structure and header likewise.
* Expand the platform data structure used by that driver to add anything necessary for Seaboard and derivatives.
* Expand the driver to handle optional/missing GPIO definitions in the platform data (i.e. Harmony/Ventana will fill in int/ext_mic_en, and other boards will set it to -1. Only Kaen will fill in hp_mute GPIO, etc.)
That way, we can still avoid a proliferation in the number of platform headers, platform data types, and ASoC machine drivers.
Does that sound like a plan?
Thanks.
Hi,
On Mon, Apr 11, 2011 at 8:27 AM, Stephen Warren swarren@nvidia.com wrote:
Perhaps the best approach is to:
- Keep a single machine driver (at least for Tegra+WM8903; as Mark noted,
attempting to share any wider doesn't make sense)
- Rename the driver e.g. tegra_wm8903.c rather than harmony.c since it'll
be more generic. Rename the platform data structure and header likewise.
- Expand the platform data structure used by that driver to add anything
necessary for Seaboard and derivatives.
- Expand the driver to handle optional/missing GPIO definitions in the
platform data (i.e. Harmony/Ventana will fill in int/ext_mic_en, and other boards will set it to -1. Only Kaen will fill in hp_mute GPIO, etc.)
That way, we can still avoid a proliferation in the number of platform headers, platform data types, and ASoC machine drivers.
Does that sound like a plan?
Sounds good to me.
-Olof
On Mon, Apr 11, 2011 at 08:27:51AM -0700, Stephen Warren wrote:
Mark Brown wrote at Sunday, April 10, 2011 7:58 PM:
you pass the full thing through platform datra or do you just pass the machine type.
Well, I was thinking of eliminating platform data completely; the selection of the correct ASoC machine driver would be based on the
Right, the machine type is essentially platform data.
name of the platform device, and the machine driver would use machine_is_*() to determine any machine-specific actions/data. So yes, at run-time, but not using runtime-determined data *tables* at all.
There'd be data of some kind in the machine driver, even if they happened to be coded as if statements.
participants (3)
-
Mark Brown -
Olof Johansson -
Stephen Warren