At Tue, 21 Apr 2015 13:43:47 +0100, Liam Girdwood wrote:
On Tue, 2015-04-21 at 12:02 +0200, Takashi Iwai wrote:
At Tue, 21 Apr 2015 10:47:53 +0100, Liam Girdwood wrote:
On Mon, 2015-04-20 at 22:30 +0100, Mark Brown wrote:
On Thu, Apr 16, 2015 at 09:48:15PM +0100, Liam Girdwood wrote:
+struct snd_soc_tplg_hdr {
- __le32 magic;
- __le32 abi; /* ABI version */
- __le32 version; /* optional vendor specific version details */
- __le32 type; /* SND_SOC_TPLG_ */
- __le32 vendor_type; /* optional vendor specific type info */
- __le32 size; /* data bytes, excluding this header */
- __le32 id; /* identifier for block */
- char reserved[128];
+} __attribute__((packed));
Not got a massively strong opinion here but given that we have ABI versioning can we just skip the 128 bytes of reserved space in most of the structs? Doesn't seem to be doing much except making the files bigger.
We had a similar discussion in Nuremburg last week, the intention is to keep the size of the structures constant so wont dont break older kernels with newer userspace ABIs etc.
Maybe a question is whether the size is sensible. But the argument here was "memory is cheap nowadays".
Ok, we can reduce the size here. I think Vinod wanted at least 4 * 4 byte words (i.e. 16 bytes) minimum IIRC, what about 16 bytes ? That would give us at least 4 new members for the future ?
+/*
- Mixer kcontrol.
- */
+struct snd_soc_tplg_mixer_control {
- struct snd_soc_tplg_control_hdr hdr;
- __le32 min;
- __le32 max;
- __le32 platform_max;
- __le32 reg;
- __le32 rreg;
- __le32 shift;
- __le32 rshift;
Do we want to convert this into an array of reg/shift tuples for the (dobutless forthcoming) 5.1 controls? Not sure it's worth it. I do think we probably need some explicit documentation for things like what to do with the left and right bits, I guess we hope other OSs or whatever can make use of the same topology if we're trying to make it standard.
Yeah, that's a good point which we should address :)
What about something like :-
struct snd_soc_mixer_channel { __le32 reg; __le32 shift; }
struct snd_soc_tplg_mixer_control { struct snd_soc_tplg_control_hdr hdr; __le32 min; __le32 max; __le32 platform_max; __le32 invert; __le32 num_channels; char reserved[64]; struct snd_soc_tplg_mixer_channel channel[0]; struct snd_soc_tplg_private priv;
A field after a variable array doesn't work. Either drop priv or make channel a fixed size array (with some max).
Oh I did not try and build this ;) A fixed size works for me. What about 8 channels (meaning we support upto 7.1) ?
I can imagine more than handful speakers :) But then it's a question whether they should be handled as a single mixer control.
Another thing that comes to mind is should we also include some channel mapping data ?
struct snd_soc_mixer_channel { __le32 map; /* Maps to ID for Left, Right, LFE etc */ __le32 reg; __le32 shift; }
This is an interesting question. The speaker mapping isn't always unique to the number of channels. Several channel maps are available for 8 channels, and it's the reason I came up with chmap API a couple of years ago.
Takashi