[alsa-devel] [PATCH 0/2] ASoC: Override codec compress_type from the machine driver
This patch adds support for overriding the compress_type of the codec driver from the machine driver. This patch depends on the previous series of patches that I've submitted with Subject "ASoC: Laying the groundwork for compress_type overriding".
Dimitris Papastamos (2): ASoC: soc-cache: Use reg_def_copy instead of reg_cache_default ASoC: soc-core: Allow machine drivers to override compress_type
include/sound/soc.h | 2 + sound/soc/soc-cache.c | 36 +++++++++++++------- sound/soc/soc-core.c | 87 +++++++++++++++++++++++++++++++++++++++++++----- 3 files changed, 104 insertions(+), 21 deletions(-)
Make sure to use codec->reg_def_copy instead of codec_drv->reg_cache_default wherever necessary. This change is necessary because in the next patch we move the cache initialization code outside snd_soc_register_codec() and by that time any data marked as __devinitconst such as the original reg_cache_default array might have already been freed by the kernel.
Signed-off-by: Dimitris Papastamos dp@opensource.wolfsonmicro.com --- include/sound/soc.h | 1 + sound/soc/soc-cache.c | 36 ++++++++++++++++++++++++------------ sound/soc/soc-core.c | 17 +++++++++++++++++ 3 files changed, 42 insertions(+), 12 deletions(-)
diff --git a/include/sound/soc.h b/include/sound/soc.h index 4409e97..4cdba68 100644 --- a/include/sound/soc.h +++ b/include/sound/soc.h @@ -478,6 +478,7 @@ struct snd_soc_codec { hw_write_t hw_write; unsigned int (*hw_read)(struct snd_soc_codec *, unsigned int); void *reg_cache; + const void *reg_def_copy; const struct snd_soc_cache_ops *cache_ops; struct mutex cache_rw_mutex;
diff --git a/sound/soc/soc-cache.c b/sound/soc/soc-cache.c index 081221d..f8e285d 100644 --- a/sound/soc/soc-cache.c +++ b/sound/soc/soc-cache.c @@ -933,7 +933,7 @@ static int snd_soc_rbtree_cache_init(struct snd_soc_codec *codec) rbtree_ctx = codec->reg_cache; rbtree_ctx->root = RB_ROOT;
- if (!codec->driver->reg_cache_default) + if (!codec->reg_def_copy) return 0;
/* @@ -951,7 +951,7 @@ static int snd_soc_rbtree_cache_init(struct snd_soc_codec *codec) struct snd_soc_rbtree_node *rbtree_node; \ \ ret = 0; \ - cache = codec->driver->reg_cache_default; \ + cache = codec->reg_def_copy; \ for (i = 0; i < codec->driver->reg_cache_size; ++i) { \ if (!cache[i]) \ continue; \ @@ -1316,13 +1316,13 @@ static int snd_soc_lzo_cache_init(struct snd_soc_codec *codec) * and remember to free it afterwards. */ tofree = 0; - if (!codec_drv->reg_cache_default) + if (!codec->reg_def_copy) tofree = 1;
- if (!codec_drv->reg_cache_default) { - codec_drv->reg_cache_default = kzalloc(reg_size, + if (!codec->reg_def_copy) { + codec->reg_def_copy = kzalloc(reg_size, GFP_KERNEL); - if (!codec_drv->reg_cache_default) + if (!codec->reg_def_copy) return -ENOMEM; }
@@ -1368,8 +1368,8 @@ static int snd_soc_lzo_cache_init(struct snd_soc_codec *codec) }
blksize = snd_soc_lzo_get_blksize(codec); - p = codec_drv->reg_cache_default; - end = codec_drv->reg_cache_default + reg_size; + p = codec->reg_def_copy; + end = codec->reg_def_copy + reg_size; /* compress the register map and fill the lzo blocks */ for (i = 0; i < blkcount; ++i, p += blksize) { lzo_blocks[i]->src = p; @@ -1385,14 +1385,18 @@ static int snd_soc_lzo_cache_init(struct snd_soc_codec *codec) lzo_blocks[i]->src_len; }
- if (tofree) - kfree(codec_drv->reg_cache_default); + if (tofree) { + kfree(codec->reg_def_copy); + codec->reg_def_copy = NULL; + } return 0; err: snd_soc_cache_exit(codec); err_tofree: - if (tofree) - kfree(codec_drv->reg_cache_default); + if (tofree) { + kfree(codec->reg_def_copy); + codec->reg_def_copy = NULL; + } return ret; }
@@ -1506,6 +1510,14 @@ static int snd_soc_flat_cache_init(struct snd_soc_codec *codec) codec_drv = codec->driver; reg_size = codec_drv->reg_cache_size * codec_drv->reg_word_size;
+ /* + * for flat compression, we don't need to keep a copy of the + * original defaults register cache as it will definitely not + * be marked as __devinitconst + */ + kfree(codec->reg_def_copy); + codec->reg_def_copy = NULL; + if (codec_drv->reg_cache_default) codec->reg_cache = kmemdup(codec_drv->reg_cache_default, reg_size, GFP_KERNEL); diff --git a/sound/soc/soc-core.c b/sound/soc/soc-core.c index a6565eb..9d9364a 100644 --- a/sound/soc/soc-core.c +++ b/sound/soc/soc-core.c @@ -3430,6 +3430,7 @@ int snd_soc_register_codec(struct device *dev, struct snd_soc_codec_driver *codec_drv, struct snd_soc_dai_driver *dai_drv, int num_dai) { + size_t reg_size; struct snd_soc_codec *codec; int ret, i;
@@ -3463,6 +3464,19 @@ int snd_soc_register_codec(struct device *dev,
/* allocate CODEC register cache */ if (codec_drv->reg_cache_size && codec_drv->reg_word_size) { + reg_size = codec_drv->reg_cache_size * codec_drv->reg_word_size; + /* it is necessary to make a copy of the default register cache + * because in the case of using a compression type that requires + * the default register cache to be marked as __devinitconst the + * kernel might have freed the array by the time we initialize + * the cache. + */ + codec->reg_def_copy = kmemdup(codec_drv->reg_cache_default, + reg_size, GFP_KERNEL); + if (!codec->reg_def_copy) { + ret = -ENOMEM; + goto error_cache; + } ret = snd_soc_cache_init(codec); if (ret < 0) { dev_err(codec->dev, "Failed to set cache compression type: %d\n", @@ -3494,6 +3508,8 @@ int snd_soc_register_codec(struct device *dev, error_dais: snd_soc_cache_exit(codec); error_cache: + kfree(codec->reg_def_copy); + codec->reg_def_copy = NULL; kfree(codec->name); kfree(codec); return ret; @@ -3528,6 +3544,7 @@ found: pr_debug("Unregistered codec '%s'\n", codec->name);
snd_soc_cache_exit(codec); + kfree(codec->reg_def_copy); kfree(codec->name); kfree(codec); }
On Thu, Dec 2, 2010 at 10:11 AM, Dimitris Papastamos dp@opensource.wolfsonmicro.com wrote:
Make sure to use codec->reg_def_copy instead of codec_drv->reg_cache_default wherever necessary. This change is necessary because in the next patch we move the cache initialization code outside snd_soc_register_codec() and by that time any data marked as __devinitconst such as the original reg_cache_default array might have already been freed by the kernel.
This commit breaks cs4270.c:
Cirrus Logic CS4270 ALSA SoC Codec Driver cs4270-codec 0-004f: found device at i2c address 4F cs4270-codec 0-004f: hardware revision 3 Unable to handle kernel paging request for data at address 0x00000000 Faulting instruction address: 0xc001646c Oops: Kernel access of bad area, sig: 11 [#1] MPC86xx HPCD last sysfs file: Modules linked in: NIP: c001646c LR: c006cef8 CTR: 00000001 REGS: ef03dd20 TRAP: 0300 Not tainted (2.6.37-rc3-02080-g8e7bd55) MSR: 00009032 <EE,ME,IR,DR> CR: 22044028 XER: 20000000 DAR: 00000000, DSISR: 40000000 TASK = ef038000[1] 'swapper' THREAD: ef03c000 GPR00: 00000000 ef03ddd0 ef038000 ef2bb9e0 fffffffc 00000008 ef2bb9dc 00000001 GPR08: 2e302d30 00000000 fffffffe 00000006 22044082 100a38d4 00000000 3ffe7388 GPR16: 00000000 3f9a6378 00000000 3ffe6210 3ffe6210 3ffe6210 00000000 00000000 GPR24: c03c0560 ef03de20 c043c968 ef03de00 ef3da220 00000000 ef2bb9e0 00000008 NIP [c001646c] memcpy+0x1c/0x9c LR [c006cef8] kmemdup+0x3c/0x54 Call Trace: [ef03ddd0] [c006cee4] kmemdup+0x28/0x54 (unreliable) [ef03ddf0] [c0250664] snd_soc_register_codec+0x278/0x390 [ef03de70] [c0256514] cs4270_i2c_probe+0xb0/0x178 [ef03de90] [c021c8a8] i2c_device_probe+0xec/0x114 [ef03deb0] [c01af400] driver_probe_device+0xa0/0x1a8 [ef03ded0] [c01af5c4] __driver_attach+0xbc/0xc0 [ef03def0] [c01aea90] bus_for_each_dev+0x70/0xac [ef03df20] [c01af20c] driver_attach+0x24/0x34 [ef03df30] [c01ae314] bus_add_driver+0x1b8/0x298 [ef03df60] [c01af928] driver_register+0x88/0x158 [ef03df80] [c021cd38] i2c_register_driver+0x4c/0xa4 [ef03dfa0] [c040fffc] cs4270_init+0x2c/0x3c [ef03dfb0] [c0003f6c] do_one_initcall+0x15c/0x1c4 [ef03dfe0] [c03f31e0] kernel_init+0xc0/0x164 [ef03dff0] [c001086c] kernel_thread+0x4c/0x68 Instruction dump: 38c60001 4200fff0 4e800020 7c032040 418100a0 54a7e8ff 38c3fffc 3884fffc 41820028 70c00003 7ce903a6 40820054 <80e40004> 85040008 90e60004 95060008 ---[ end trace 40d01772bfab46f2 ]--- Kernel panic - not syncing: Attempted to kill init!
Which is this line:
@@ -3463,6 +3464,19 @@ int snd_soc_register_codec(struct device *dev,
/* allocate CODEC register cache */ if (codec_drv->reg_cache_size && codec_drv->reg_word_size) {
- reg_size = codec_drv->reg_cache_size * codec_drv->reg_word_size;
- /* it is necessary to make a copy of the default register cache
- * because in the case of using a compression type that requires
- * the default register cache to be marked as __devinitconst the
- * kernel might have freed the array by the time we initialize
- * the cache.
- */
- codec->reg_def_copy = kmemdup(codec_drv->reg_cache_default,
- reg_size, GFP_KERNEL);
Unlike all the other codec drivers, the CS4270 driver does not initialize codec_drv->reg_cache_default, so the pointer is NULL during the kmemdup() call.
What is the criteria for defining reg_cache_default? Many drivers don't define it:
$ grep -c reg_cache_default *.c | grep :0 88pm860x-codec.c:0 ac97.c:0 ad1836.c:0 ad73311.c:0 ads117x.c:0 ak4104.c:0 alc5623.c:0 cq93vc.c:0 cs4270.c:0 cs42l51.c:0 cx20442.c:0 l3.c:0 max9877.c:0 pcm3008.c:0 spdif_transciever.c:0 tlv320aic26.c:0 tpa6130a2.c:0 wl1273.c:0 wm2000.c:0 wm8350.c:0 wm8400.c:0 wm8727.c:0 wm8994-tables.c:0 wm_hubs.c:0
On Wed, Jan 05, 2011 at 03:04:31PM -0600, Timur Tabi wrote:
Unlike all the other codec drivers, the CS4270 driver does not initialize codec_drv->reg_cache_default, so the pointer is NULL during the kmemdup() call.
What is the criteria for defining reg_cache_default? Many drivers don't define it:
In general the expectation is that unless it's got a good reason not to a well written CODEC driver will use all the standard register cache infrastructure, including providing a set of defaults. Good reasons for this include things like not having any registers and indeterminate default hardware state.
As you will doubtless have seen when you looked at the code every other reference to reg_cache_default checks to see if it's set before using it. This does rather suggest that the intention of the code is that it be optional.
Mark Brown wrote:
In general the expectation is that unless it's got a good reason not to a well written CODEC driver will use all the standard register cache infrastructure, including providing a set of defaults. Good reasons for this include things like not having any registers and indeterminate default hardware state.
My concern is that I think it's unwise to hard-code the default values of the registers in the source file. Who's to say that a newer version of the chip won't have different power-on defaults?
I do want to support a register cache, but I don't want to hard-code the default values into cs4270.c. Is this supported?
As you will doubtless have seen when you looked at the code every other reference to reg_cache_default checks to see if it's set before using it. This does rather suggest that the intention of the code is that it be optional.
So are you saying that there's a bug in this patch? Perhaps that code should look like this:
if (codec_drv->reg_cache_default) codec->reg_def_copy = kmemdup(codec_drv->reg_cache_default, reg_size, GFP_KERNEL); else { codec->reg_def_copy = kmalloc(reg_size, GFP_KERNEL); // here we somehow tell the codec driver to initialize reg_def_copy }
On Wed, Jan 05, 2011 at 05:08:20PM -0600, Timur Tabi wrote:
My concern is that I think it's unwise to hard-code the default values of the registers in the source file. Who's to say that a newer version of the chip won't have different power-on defaults?
In practical terms this is vanishingly unlikely;
I do want to support a register cache, but I don't want to hard-code the default values into cs4270.c. Is this supported?
If something you'd like to do doesn't seem to be working or is otherwise not there and you can see a way to implement it sensibly then the most obvious thing would seem to be to implement it.
Please remember that this isn't a frozen commercial OS release from a third party, you can contribute to the core code if you see a need to. None of this is fixed in stone and a lot of it comes from looking at the code and taking opportunities to add appropriate abstractions - look at symmetric_rates, or the creation of soc-cache for example.
As you will doubtless have seen when you looked at the code every other reference to reg_cache_default checks to see if it's set before using it. This does rather suggest that the intention of the code is that it be optional.
So are you saying that there's a bug in this patch? Perhaps that code should look like this:
Perhaps it should, though my immediate question (without looking at the existing code again) is why the existing tests are being removed.
On Wed, Jan 05, 2011 at 11:29:47PM +0000, Mark Brown wrote:
So are you saying that there's a bug in this patch? Perhaps that code should look like this:
Perhaps it should, though my immediate question (without looking at the existing code again) is why the existing tests are being removed.
Having looked slightly further it does look like something I'd expect to work.
More generally though what I'm trying to say is more about the approach that's being taken here; with things like this it's generally much better to at least have a dig into what's not working for you and have an idea of what's going on rather than just stopping and asking. This is normally easier all round, either you'll be able to sort stuff out yourself or you'll be able to ask a much better question.
Mark Brown wrote:
More generally though what I'm trying to say is more about the approach that's being taken here; with things like this it's generally much better to at least have a dig into what's not working for you and have an idea of what's going on rather than just stopping and asking. This is normally easier all round, either you'll be able to sort stuff out yourself or you'll be able to ask a much better question.
Mark, you don't need to explain the philosophy of open source development to me. :-)
I just wanted to get the idea of what you and others intended with the current register cache code, just as a matter of context. I like to understand these things *before* I start digging around.
On Thu, Jan 06, 2011 at 12:15:17AM +0000, Tabi Timur-B04825 wrote:
I just wanted to get the idea of what you and others intended with the current register cache code, just as a matter of context. I like to
So ask that question; "I looked at this and I'm not really sure if it's supposed to work or not - thing X suggests yes, thing Y suggests no but looks like a bug so..." and so on.
In cases like this (although not I suspect this particular one) the answer is often that there was't any particular consideration for whatever unusual case you're looking at.
understand these things *before* I start digging around.
The important thing is to show that you're looking, especially in areas where you've done something unusual in your driver. "Looking at this briefly..." isn't a problem, just make it clear that that's what you've done.
Part of the reason I'm emphasising this is that stuff like this seems to cause you to get stuch relatively often.
Mark Brown wrote:
So ask that question; "I looked at this and I'm not really sure if it's supposed to work or not - thing X suggests yes, thing Y suggests no but looks like a bug so..." and so on.
In cases like this (although not I suspect this particular one) the answer is often that there was't any particular consideration for whatever unusual case you're looking at.
Ah, but I don't think my case is "unusual". Frankly, I think it's unusual to hard-code default register values into a driver. I just don't see how you can guarantee that the values will be correct for all supported revisions of the chip.
On the CS4270, for instance, one register contains the chip revision number. There's no way I can know which revision will be used on any given board.
Anyway, I'll work on fixing this and post a patch.
On Thu, Jan 06, 2011 at 10:26:43AM -0600, Timur Tabi wrote:
Mark Brown wrote:
In cases like this (although not I suspect this particular one) the answer is often that there was't any particular consideration for whatever unusual case you're looking at.
Ah, but I don't think my case is "unusual". Frankly, I think it's unusual to hard-code default register values into a driver. I just don't see how you can
*sigh* If you look at other ASoC drivers you'll see that providing register defaults is very much idiomatic.
guarantee that the values will be correct for all supported revisions of the chip.
We've been doing this for rather a long time; many devices don't support readback at all so there's not even any option for them. There's some fairly simple things you can do if there are issues, like write out the new default values explicitly (which will be a noop on new silicon).
There's fairly strong pressures on chip vendors to avoid anything that causes issues here. Consider what a chip vendor has to do to introduce an incompatible register change in production hardware: they need to notify all their customers and they need to make sure that their customers are happy and don't get upset about having to update their software (perhaps they don't like the new default). If the customers aren't happy then worst case you end up having to keep both old and new silicon in production.
On the CS4270, for instance, one register contains the chip revision number. There's no way I can know which revision will be used on any given board.
So that register should be marked as volatile, the register default value will be ignored and the cache will never be used for that register.
On Wed, 2011-01-05 at 15:04 -0600, Timur Tabi wrote:
Which is this line:
@@ -3463,6 +3464,19 @@ int snd_soc_register_codec(struct device *dev,
/* allocate CODEC register cache */ if (codec_drv->reg_cache_size && codec_drv->reg_word_size) {
reg_size = codec_drv->reg_cache_size * codec_drv->reg_word_size;/* it is necessary to make a copy of the default register cache* because in the case of using a compression type that requires* the default register cache to be marked as __devinitconst the* kernel might have freed the array by the time we initialize* the cache.*/codec->reg_def_copy = kmemdup(codec_drv->reg_cache_default,reg_size, GFP_KERNEL);
The semantics behind this code is that if the driver provides a reg_cache_size and a reg_word_size it *should* provide a defaults register cache as well. If you want to manage your own register cache in the driver which is not advised, you will have to add similar functionality in your _priv struct. If you require more flexible functionality you need to consider implementing a sensible strategy and submitting it as a patch.
Thanks, Dimitris
Dimitris Papastamos wrote:
The semantics behind this code is that if the driver provides a reg_cache_size and a reg_word_size it *should* provide a defaults register cache as well.
I *had* code which did this, but apparently it broke somewhere. Oh well.
If you want to manage your own register cache in the driver which is not advised, you will have to add similar functionality in your _priv struct. If you require more flexible functionality you need to consider implementing a sensible strategy and submitting it as a patch.
I will fix my driver to restore the functionality.
This patch allows machine drivers to override the compression type provided by the codec driver.
Signed-off-by: Dimitris Papastamos dp@opensource.wolfsonmicro.com --- include/sound/soc.h | 1 + sound/soc/soc-core.c | 74 ++++++++++++++++++++++++++++++++++++++++++------- 2 files changed, 64 insertions(+), 11 deletions(-)
diff --git a/include/sound/soc.h b/include/sound/soc.h index 4cdba68..e5b9bd7 100644 --- a/include/sound/soc.h +++ b/include/sound/soc.h @@ -472,6 +472,7 @@ struct snd_soc_codec { unsigned int ac97_registered:1; /* Codec has been AC97 registered */ unsigned int ac97_created:1; /* Codec has been created by SoC */ unsigned int sysfs_registered:1; /* codec has been sysfs registered */ + unsigned int cache_init:1; /* codec cache has been initialized */
/* codec IO */ void *control_data; /* codec control (i2c/3wire) data */ diff --git a/sound/soc/soc-core.c b/sound/soc/soc-core.c index 9d9364a..a9827f1 100644 --- a/sound/soc/soc-core.c +++ b/sound/soc/soc-core.c @@ -1726,9 +1726,36 @@ static void soc_remove_aux_dev(struct snd_soc_card *card, int num) } }
+static int snd_soc_init_codec_cache(struct snd_soc_codec *codec, + enum snd_soc_compress_type compress_type) +{ + int ret; + + if (codec->cache_init) + return 0; + + /* override the compress_type if necessary */ + if (compress_type && codec->compress_type != compress_type) + codec->compress_type = compress_type; + dev_dbg(codec->dev, "Cache compress_type for %s is %d\n", + codec->name, codec->compress_type); + ret = snd_soc_cache_init(codec); + if (ret < 0) { + dev_err(codec->dev, "Failed to set cache compression type: %d\n", + ret); + return ret; + } + codec->cache_init = 1; + return 0; +} + + static void snd_soc_instantiate_card(struct snd_soc_card *card) { struct platform_device *pdev = to_platform_device(card->dev); + struct snd_soc_codec *codec; + struct snd_soc_codec_conf *codec_conf; + enum snd_soc_compress_type compress_type; int ret, i;
mutex_lock(&card->mutex); @@ -1748,6 +1775,39 @@ static void snd_soc_instantiate_card(struct snd_soc_card *card) return; }
+ /* initialize the register cache for each available codec */ + list_for_each_entry(codec, &codec_list, list) { + if (codec->cache_init) + continue; + /* check to see if we need to override the compress_type */ + for (i = 0; i < card->num_configs; ++i) { + codec_conf = &card->codec_conf[i]; + if (!strcmp(codec->name, codec_conf->dev_name)) { + compress_type = codec_conf->compress_type; + if (compress_type && compress_type + != codec->compress_type) + break; + } + } + if (i == card->num_configs) { + /* no need to override the compress_type so + * go ahead and do the standard thing */ + ret = snd_soc_init_codec_cache(codec, 0); + if (ret < 0) { + mutex_unlock(&card->mutex); + return; + } + continue; + } + /* override the compress_type with the one supplied in + * the machine driver */ + ret = snd_soc_init_codec_cache(codec, compress_type); + if (ret < 0) { + mutex_unlock(&card->mutex); + return; + } + } + /* card bind complete so register a sound card */ ret = snd_card_create(SNDRV_DEFAULT_IDX1, SNDRV_DEFAULT_STR1, card->owner, 0, &card->snd_card); @@ -3475,13 +3535,7 @@ int snd_soc_register_codec(struct device *dev, reg_size, GFP_KERNEL); if (!codec->reg_def_copy) { ret = -ENOMEM; - goto error_cache; - } - ret = snd_soc_cache_init(codec); - if (ret < 0) { - dev_err(codec->dev, "Failed to set cache compression type: %d\n", - ret); - goto error_cache; + goto fail; } }
@@ -3494,7 +3548,7 @@ int snd_soc_register_codec(struct device *dev, if (num_dai) { ret = snd_soc_register_dais(dev, dai_drv, num_dai); if (ret < 0) - goto error_dais; + goto fail; }
mutex_lock(&client_mutex); @@ -3505,9 +3559,7 @@ int snd_soc_register_codec(struct device *dev, pr_debug("Registered codec '%s'\n", codec->name); return 0;
-error_dais: - snd_soc_cache_exit(codec); -error_cache: +fail: kfree(codec->reg_def_copy); codec->reg_def_copy = NULL; kfree(codec->name);
On Thu, 2010-12-02 at 16:11 +0000, Dimitris Papastamos wrote:
This patch adds support for overriding the compress_type of the codec driver from the machine driver. This patch depends on the previous series of patches that I've submitted with Subject "ASoC: Laying the groundwork for compress_type overriding".
Dimitris Papastamos (2): ASoC: soc-cache: Use reg_def_copy instead of reg_cache_default ASoC: soc-core: Allow machine drivers to override compress_type
include/sound/soc.h | 2 + sound/soc/soc-cache.c | 36 +++++++++++++------- sound/soc/soc-core.c | 87 +++++++++++++++++++++++++++++++++++++++++++----- 3 files changed, 104 insertions(+), 21 deletions(-)
All
Acked-by: Liam Girdwood lrg@slimlogic.co.uk
On Thu, Dec 02, 2010 at 04:11:04PM +0000, Dimitris Papastamos wrote:
This patch adds support for overriding the compress_type of the codec driver from the machine driver. This patch depends on the previous series of patches that I've submitted with Subject "ASoC: Laying the groundwork for compress_type overriding".
Applied, thanks.
participants (5)
-
Dimitris Papastamos -
Liam Girdwood -
Mark Brown -
Tabi Timur-B04825 -
Timur Tabi