Tegra ASoC, multiple boards, platform data, and conditionals in machine driver

Tegra ASoC, multiple boards, platform data, and conditionals in machine driver

[Stephen Warren]

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");
	}

-- 
nvpublic

Re: Tegra ASoC, multiple boards, platform data, and conditionals in machine driver

[Mark Brown]

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.

RE: Tegra ASoC, multiple boards, platform data, and conditionals in machine driver

[Stephen Warren]

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.

-- 
nvpublic

Re: Tegra ASoC, multiple boards, platform data, and conditionals in machine driver

[Olof Johansson]

Hi,

On Sat, Apr 9, 2011 at 7:52 PM, Stephen Warren <swarren-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org> 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

Re: [alsa-devel] Tegra ASoC, multiple boards, platform data, and conditionals in machine driver

[Mark Brown]

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.

RE: [alsa-devel] Tegra ASoC, multiple boards, platform data, and conditionals in machine driver

[Stephen Warren]

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-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org> 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.

-- 
nvpublic

Re: [alsa-devel] Tegra ASoC, multiple boards, platform data, and conditionals in machine driver

[Olof Johansson]

Hi,

On Mon, Apr 11, 2011 at 8:27 AM, Stephen Warren <swarren-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org> 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

Re: Tegra ASoC, multiple boards, platform data, and conditionals in machine driver

[Mark Brown]

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.