[alsa-devel] [RFC-PATCH 0/2] ASoC: simple_card: support for hw-params rules
From: Martin Sperl kernel@martin.sperl.org
Simple_card does require under some circumstances the ability to configure certain hw_parameters based on clocks, bits, channels.
This patchset adds a generic way to configure these kind of things via the device tree easily. This patchset implements this for simple_card, but other drivers can just as easily make use of this.
For now we have the following matchers and actions: * matchers: * match_sample_bits * match_rate * match_channels * actions: * set_fixed_bclk_size
As a note: the available matching rules and action rules right now are hard-coded, but this could in principle get extended to be more dynamic via kallsyms_lookup_name that would lookup the requested symbol and assume it is a struct asoc_generic_hw_params_method, on which it could apply several sanity-checks before using the pointers for real.
This would allow other kinds of action methods to get exposed in codec-drivers, that can then get used to handle some settings specific to the codec.
I leave this is an exercise for later...
Martin Sperl (2): ASoC: hw-params-rules: add generic hw_params-rules ASoC: simple_card: add support for hw_params_rules
.../devicetree/bindings/sound/hw-params-rules.txt | 86 +++++ .../devicetree/bindings/sound/simple-card.txt | 1 + sound/soc/generic/Kconfig | 6 + sound/soc/generic/Makefile | 2 + sound/soc/generic/hw-params-rules.c | 402 +++++++++++++++++++++ sound/soc/generic/hw-params-rules.h | 49 +++ sound/soc/generic/simple-card.c | 11 +- 7 files changed, 554 insertions(+), 3 deletions(-) create mode 100644 Documentation/devicetree/bindings/sound/hw-params-rules.txt create mode 100644 sound/soc/generic/hw-params-rules.c create mode 100644 sound/soc/generic/hw-params-rules.h
-- 2.1.4
From: Martin Sperl kernel@martin.sperl.org
Add generic rules that may execute when running hw_params methods.
These rules are configured via the device-tree.
Right now only a static list of match and action rules exist. This can get changed in the future to something more dynamic if security concerns can get handled propperly.
Signed-off-by: Martin Sperl kernel@martin.sperl.org --- .../devicetree/bindings/sound/hw-params-rules.txt | 86 +++++ sound/soc/generic/Kconfig | 6 + sound/soc/generic/Makefile | 2 + sound/soc/generic/hw-params-rules.c | 402 +++++++++++++++++++++ sound/soc/generic/hw-params-rules.h | 49 +++ 5 files changed, 545 insertions(+) create mode 100644 Documentation/devicetree/bindings/sound/hw-params-rules.txt create mode 100644 sound/soc/generic/hw-params-rules.c create mode 100644 sound/soc/generic/hw-params-rules.h
diff --git a/Documentation/devicetree/bindings/sound/hw-params-rules.txt b/Documentation/devicetree/bindings/sound/hw-params-rules.txt new file mode 100644 index 0000000..8e5f2b5 --- /dev/null +++ b/Documentation/devicetree/bindings/sound/hw-params-rules.txt @@ -0,0 +1,86 @@ +HW-params-rules: + +Rules that execute actions during hw-params method calls. + +The following optional nodes will sit inside the sound card nodes: + +hw-params-rule subnodes: + +Required properties: +- priority: the priority of the rule - lowest is applied first. + +contains further subnodes: + +- match: list of matches that have to apply +- action: list of actions to execute + +match and action subnodes: + +Required properties: + +- method: the method name (string) to execute + +Possible values for method: +* asoc_generic_hw_params_match_sample_bits +* asoc_generic_hw_params_match_rate +* asoc_generic_hw_params_match_channel +* asoc_generic_hw_params_set_fixed_bclk_size + + +Required/optional properties depend on the method defined +typical properies: + +- value: u32 value to pass to method. +- values: u32 value array to pass to method. + +Example: +sound { + compatible = "simple-audio-card"; + ... + /* + * set bclk_size to 80 when encountering: + * 48kHz or 96kHz with 2 channels with 32bit/channel + */ + hw-params-rule@0 { + priority = <0>; + match@0 { + method = "asoc_generic_hw_params_match_sample_bits"; + values = <32>; + }; + match@1 { + method = "asoc_generic_hw_params_match_rate"; + values = <48000>, <96000>; + }; + match@2 { + method = "asoc_generic_hw_params_match_channels"; + values = <2>; + }; + action@0 { + method = "asoc_generic_hw_params_set_fixed_bclk_size"; + value = <80>; + }; + }; + /* + * set bclk_size to 40 when encountering: + * 48kHz with 2 channels with 16bit/channel + */ + hw-params-rule@1 { + priority = <1>; + match@0 { + method = "asoc_generic_hw_params_match_sample_bits"; + values = <16>; + }; + match@1 { + method = "asoc_generic_hw_params_match_rate"; + values = <48000>; + }; + match@2 { + method = "asoc_generic_hw_params_match_channels"; + values = <2>; + }; + action@0 { + method = "asoc_generic_hw_params_set_fixed_bclk_size"; + value = <40>; + }; + }; +}; diff --git a/sound/soc/generic/Kconfig b/sound/soc/generic/Kconfig index 610f612..2c9f0c1 100644 --- a/sound/soc/generic/Kconfig +++ b/sound/soc/generic/Kconfig @@ -1,3 +1,9 @@ +config SND_HW_PARAMS_RULES + tristate "ASoC hw_param rules support" + depends on OF + help + This option enables generic hw_param_rules support + config SND_SIMPLE_CARD tristate "ASoC Simple sound card support" help diff --git a/sound/soc/generic/Makefile b/sound/soc/generic/Makefile index 9c3b246..4eaad07 100644 --- a/sound/soc/generic/Makefile +++ b/sound/soc/generic/Makefile @@ -1,3 +1,5 @@ snd-soc-simple-card-objs := simple-card.o +snd-soc-hw-params-rules-objs := hw-params-rules.o
obj-$(CONFIG_SND_SIMPLE_CARD) += snd-soc-simple-card.o +obj-$(CONFIG_SND_HW_PARAMS_RULES) += snd-soc-hw-params-rules.o diff --git a/sound/soc/generic/hw-params-rules.c b/sound/soc/generic/hw-params-rules.c new file mode 100644 index 0000000..46dbdff --- /dev/null +++ b/sound/soc/generic/hw-params-rules.c @@ -0,0 +1,402 @@ +/* + * ASoC generic hw_params_rules support + * + * Copyright (C) 2016 Martin Sperl + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License version 2 as + * published by the Free Software Foundation. + */ + +#include <linux/device.h> +#include <linux/module.h> +#include <linux/of.h> +#include <linux/list_sort.h> +#include <sound/pcm_params.h> +#include <sound/simple_card.h> +#include <sound/soc-dai.h> +#include <sound/soc.h> + +struct snd_soc_hw_params_actionmatch { + struct list_head list; + int (*method)(struct snd_pcm_substream *substream, + struct snd_pcm_hw_params *params, + void *data); + void *data; +}; + +struct snd_soc_hw_param_rule { + struct list_head list; + const char *name; + u32 priority; + struct list_head matches; + struct list_head actions; +}; + +struct snd_soc_size_u32array { + size_t size; + u32 data[]; +}; + +static int asoc_generic_hw_params_read_u32array( + struct device *dev, struct device_node *node, void **data) +{ + int i, size, ret; + struct snd_soc_size_u32array *array; + + size = of_property_count_elems_of_size(node, "values", sizeof(u32)); + if (size < 0) { + dev_err(dev, + "%s: Could not read size of property "values" - %d\n", + of_node_full_name(node), size); + return size; + } + + array = devm_kzalloc(dev, sizeof(*array) + sizeof(u32) * size, + GFP_KERNEL); + if (!array) + return -ENOMEM; + *data = array; + + array->size = size; + + for (i = 0; i < size; i++) { + ret = of_property_read_u32(node, "values", &array->data[i]); + if (ret) + return ret; + } + + return 0; +} + +static int asoc_generic_hw_params_read_u32( + struct device *dev, struct device_node *node, void **data) +{ + return of_property_read_u32(node, "value", (u32 *)data); +} + +static int asoc_generic_hw_params_match_sample_bits( + struct snd_pcm_substream *substream, + struct snd_pcm_hw_params *params, + void *data) +{ + long int bits = + snd_pcm_format_physical_width(params_format(params)); + struct snd_soc_size_u32array *array = data; + int i; + + for (i = 0; i < array->size; i++) { + if (bits == array->data[i]) + return 1; + } + + return 0; +} + +static int asoc_generic_hw_params_match_channels( + struct snd_pcm_substream *substream, + struct snd_pcm_hw_params *params, + void *data) +{ + int channels = params_channels(params); + struct snd_soc_size_u32array *array = data; + int i; + + for (i = 0; i < array->size; i++) { + if (channels == array->data[i]) + return 1; + } + + return 0; +} + +static int asoc_generic_hw_params_match_rate( + struct snd_pcm_substream *substream, + struct snd_pcm_hw_params *params, + void *data) +{ + long int rate = params_rate(params); + + struct snd_soc_size_u32array *array = data; + int i; + + for (i = 0; i < array->size; i++) { + if (rate == array->data[i]) + return 1; + } + + return 0; +} + +static int asoc_generic_hw_params_set_fixed_bclk_size( + struct snd_pcm_substream *substream, + struct snd_pcm_hw_params *params, + void *data) +{ + struct snd_soc_pcm_runtime *rtd = substream->private_data; + struct snd_soc_dai *cpu_dai = rtd->cpu_dai; + + return snd_soc_dai_set_bclk_ratio(cpu_dai, (unsigned int)data); +} + +struct asoc_generic_hw_params_method { + const char *name; + int (*method)(struct snd_pcm_substream *substream, + struct snd_pcm_hw_params *params, + void *data); + int (*parse)(struct device *dev, struct device_node *node, + void **data); +}; + +#define HW_PARAMS_METHOD(m, p) \ + {.name = #m, .method = m, .parse = p } +#define HW_PARAMS_METHOD_U32(n) \ + HW_PARAMS_METHOD(n, asoc_generic_hw_params_read_u32) +#define HW_PARAMS_METHOD_U32ARRAY(n) \ + HW_PARAMS_METHOD(n, asoc_generic_hw_params_read_u32array) + +static const struct asoc_generic_hw_params_method +asoc_generic_hw_params_methods[] = { + HW_PARAMS_METHOD_U32ARRAY(asoc_generic_hw_params_match_sample_bits), + HW_PARAMS_METHOD_U32ARRAY(asoc_generic_hw_params_match_rate), + HW_PARAMS_METHOD_U32ARRAY(asoc_generic_hw_params_match_channels), + HW_PARAMS_METHOD_U32(asoc_generic_hw_params_set_fixed_bclk_size) +}; + +static int asoc_generic_hw_params_lookup_methods( + struct device *dev, const char *method, + struct device_node *node, + struct snd_soc_hw_params_actionmatch *am) +{ + const struct asoc_generic_hw_params_method *m; + size_t i; + + /* + * hardcoded list of "allowed" methods + * maybe a more dynamic approach using kallsyms could also be taken + */ + for (i = 0; i < ARRAY_SIZE(asoc_generic_hw_params_methods); i++) { + m = &asoc_generic_hw_params_methods[i]; + if (strcmp(m->name, method) == 0) { + am->method = m->method; + if (m->parse) + return m->parse(dev, node, &am->data); + else + return 0; + } + } + + dev_err(dev, "%s: method %s not found\n", + of_node_full_name(node), method); + return -EINVAL; +} + +static int asoc_generic_hw_params_handle_rule( + struct snd_soc_hw_param_rule *rule, + struct snd_pcm_substream *substream, + struct snd_pcm_hw_params *params) +{ + struct snd_soc_pcm_runtime *rtd = substream->private_data; + struct device *dev = rtd->dev; + struct snd_soc_hw_params_actionmatch *am; + int ret; + + dev_dbg(dev, "Trying to apply rule: %s\n", rule->name); + + /* apply match rules */ + list_for_each_entry(am, &rule->matches, list) { + dev_dbg(dev, "\tRunning match %pf(%pK)\n", + am->method, am->data); + /* match method return 0 on match, 1 otherwise */ + ret = am->method(substream, params, am->data); + if (!ret) + return 1; + } + + /* so we match, so run all the actions */ + list_for_each_entry(am, &rule->actions, list) { + dev_dbg(dev, "\tRunning action %pf(%pK)\n", + am->method, am->data); + /* action method returns 0 on success */ + ret = am->method(substream, params, am->data); + if (ret) + return ret; + } + + return 0; +} + +int asoc_generic_hw_params_process_rules( + struct list_head *list_head, + struct snd_pcm_substream *substream, + struct snd_pcm_hw_params *params) +{ + struct snd_soc_hw_param_rule *rule; + int ret; + + /* check if the list_head is initialized */ + if (!list_head->next) + return 0; + + /* iterate all rules */ + list_for_each_entry(rule, list_head, list) { + ret = asoc_generic_hw_params_handle_rule( + rule, substream, params); + if (ret <= 0) + return ret; + } + + return 0; +} +EXPORT_SYMBOL_GPL(asoc_generic_hw_params_process_rules); + +static int asoc_generic_hw_params_actionmatch_parse_of( + struct device *dev, struct device_node *node, + struct list_head *list_head, const char *nodename) +{ + struct snd_soc_hw_params_actionmatch *am; + const char *methodname; + int ret; + + /* get the method name */ + ret = of_property_read_string(node, "method", &methodname); + if (ret) { + dev_err(dev, "%s: missing "method" property - %d\n", + of_node_full_name(node), ret); + return ret; + } + + /* alloc the action/match */ + am = devm_kzalloc(dev, sizeof(*am), GFP_KERNEL); + if (!am) + return -ENOMEM; + + /* lookup the method */ + ret = asoc_generic_hw_params_lookup_methods(dev, methodname, + node, am); + if (ret) + return ret; + + /* append to list */ + list_add_tail(&am->list, list_head); + + dev_dbg(dev, "\t\tadded %s: %s - %pf(%pK)\n", nodename, + of_node_full_name(node), am->method, am->data); + + return 0; +} + +static int asoc_generic_hw_params_actionmatches_parse_of( + struct device *dev, struct device_node *node, + struct list_head *list_head, const char *nodename) +{ + struct device_node *np = NULL; + int ret = 0; + + /* init matchers */ + INIT_LIST_HEAD(list_head); + + /* iterate over all child nodes */ + for_each_child_of_node(node, np) { + if (np->name && (of_node_cmp(np->name, nodename) == 0)) { + ret = asoc_generic_hw_params_actionmatch_parse_of( + dev, np, list_head, nodename); + if (ret) + return ret; + } + } + + return 0; +} + +static int asoc_generic_hw_params_rule_parse_of( + struct device *dev, struct device_node *node, + struct list_head *list_head) +{ + struct snd_soc_hw_param_rule *rule; + int ret; + + rule = devm_kzalloc(dev, sizeof(*rule), GFP_KERNEL); + if (!rule) + return -ENOMEM; + + rule->name = of_node_full_name(node); + + dev_dbg(dev, "\tadding Rule: %s\n", rule->name); + + /* read priority */ + ret = of_property_read_u32(node, "priority", &rule->priority); + if (ret) { + dev_err(dev, "%s: can not read "priority" - %d\n", + rule->name, ret); + return ret; + } + + /* parse all matches sub-nodes */ + ret = asoc_generic_hw_params_actionmatches_parse_of( + dev, node, &rule->matches, "match"); + if (ret) + return ret; + + /* parse all action sub-nodes */ + ret = asoc_generic_hw_params_actionmatches_parse_of( + dev, node, &rule->actions, "action"); + if (ret) + return ret; + + /* append to list */ + list_add_tail(&rule->list, list_head); + + return 0; +} + +static int asoc_generic_hw_params_rules_sort( + void *data, struct list_head *a, struct list_head *b) +{ + struct snd_soc_hw_param_rule *rulea = + container_of(a, typeof(*rulea), list); + struct snd_soc_hw_param_rule *ruleb = + container_of(b, typeof(*ruleb), list); + + if (rulea->priority < ruleb->priority) + return -1; + if (rulea->priority > ruleb->priority) + return 1; + + return 0; +} + +int asoc_generic_hw_params_rules_parse_of( + struct device *dev, + struct device_node *node, + struct list_head *list_head) +{ + const char *nodename = "hw-params-rule"; + struct device_node *np = NULL; + int ret = 0; + + /* init matchers */ + INIT_LIST_HEAD(list_head); + + if (!of_get_child_by_name(node, nodename)) + return 0; + + for (np = of_find_node_by_name(node, nodename); np; + np = of_find_node_by_name(np, nodename)) { + ret = asoc_generic_hw_params_rule_parse_of( + dev, np, list_head); + if (ret) + return ret; + } + + /* and sort by name */ + list_sort(dev, list_head, asoc_generic_hw_params_rules_sort); + + /* iterate the sub-nodes */ + return 0; +} +EXPORT_SYMBOL_GPL(asoc_generic_hw_params_rules_parse_of); + +MODULE_AUTHOR("Martin Sperl"); +MODULE_DESCRIPTION("generic hw_params_rules support"); +MODULE_LICENSE("GPL"); diff --git a/sound/soc/generic/hw-params-rules.h b/sound/soc/generic/hw-params-rules.h new file mode 100644 index 0000000..0b054ce --- /dev/null +++ b/sound/soc/generic/hw-params-rules.h @@ -0,0 +1,49 @@ +/* + * ASoC generic hw_params_rules support + * + * Copyright (C) 2016 Martin Sperl + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License version 2 as + * published by the Free Software Foundation. + */ + +#ifndef __HW_PARAMS_RULES_H +#define __HW_PARAMS_RULES_H + +#include <linux/device.h> +#include <linux/list.h> +#include <linux/of.h> +#include <sound/pcm.h> + +#if defined(CONFIG_SND_HW_PARAMS_RULES) || \ + defined(CONFIG_SND_HW_PARAMS_RULES_MODULE) + +int asoc_generic_hw_params_rules_parse_of( + struct device *dev, + struct device_node *node, + struct list_head *list_head); + +int asoc_generic_hw_params_process_rules( + struct list_head *list_head, + struct snd_pcm_substream *substream, + struct snd_pcm_hw_params *params); +#else +static int asoc_generic_hw_params_rules_parse_of( + struct device *dev, + struct device_node *node, + struct list_head *list_head) +{ + return 0; +} + +static int asoc_generic_hw_params_process_rules( + struct list_head *list_head, + struct snd_pcm_substream *substream, + struct snd_pcm_hw_params *params) +{ + return 0; +} +#endif + +#endif
From: Martin Sperl kernel@martin.sperl.org
Add support for hw_params_rules to simple_card.
Signed-off-by: Martin Sperl kernel@martin.sperl.org --- Documentation/devicetree/bindings/sound/simple-card.txt | 1 + sound/soc/generic/simple-card.c | 11 ++++++++--- 2 files changed, 9 insertions(+), 3 deletions(-)
diff --git a/Documentation/devicetree/bindings/sound/simple-card.txt b/Documentation/devicetree/bindings/sound/simple-card.txt index cf3979e..2cfed39 100644 --- a/Documentation/devicetree/bindings/sound/simple-card.txt +++ b/Documentation/devicetree/bindings/sound/simple-card.txt @@ -31,6 +31,7 @@ Optional subnodes: omitted when the card has only one DAI link. See the examples and the section bellow. +- hw-params-rules : Please refer to hw-params-rules.txt.
Dai-link subnode properties and subnodes:
diff --git a/sound/soc/generic/simple-card.c b/sound/soc/generic/simple-card.c index 2389ab4..6b95ea1 100644 --- a/sound/soc/generic/simple-card.c +++ b/sound/soc/generic/simple-card.c @@ -8,6 +8,7 @@ * it under the terms of the GNU General Public License version 2 as * published by the Free Software Foundation. */ +#include "hw-params-rules.h" #include <linux/clk.h> #include <linux/device.h> #include <linux/gpio.h> @@ -33,6 +34,7 @@ struct simple_card_data { int gpio_hp_det_invert; int gpio_mic_det; int gpio_mic_det_invert; + struct list_head hw_params_rules; struct snd_soc_dai_link dai_link[]; /* dynamically allocated */ };
@@ -51,7 +53,7 @@ static int asoc_simple_card_startup(struct snd_pcm_substream *substream) ret = clk_prepare_enable(dai_props->cpu_dai.clk); if (ret) return ret; - + ret = clk_prepare_enable(dai_props->codec_dai.clk); if (ret) clk_disable_unprepare(dai_props->cpu_dai.clk); @@ -99,7 +101,9 @@ static int asoc_simple_card_hw_params(struct snd_pcm_substream *substream, if (ret && ret != -ENOTSUPP) goto err; } - return 0; + + return asoc_generic_hw_params_process_rules( + &priv->hw_params_rules, substream, params); err: return ret; } @@ -509,7 +513,8 @@ static int asoc_simple_card_parse_of(struct device_node *node, if (!priv->snd_card.name) priv->snd_card.name = priv->snd_card.dai_link->name;
- return 0; + return asoc_generic_hw_params_rules_parse_of( + dev, node, &priv->hw_params_rules); }
/* Decrease the reference count of the device nodes */
On 05/20/2016 02:30 PM, kernel@martin.sperl.org wrote:
From: Martin Sperl kernel@martin.sperl.org
Simple_card does require under some circumstances the ability to configure certain hw_parameters based on clocks, bits, channels.
This patchset adds a generic way to configure these kind of things via the device tree easily. This patchset implements this for simple_card, but other drivers can just as easily make use of this.
For now we have the following matchers and actions:
- matchers:
- match_sample_bits
- match_rate
- match_channels
- actions:
- set_fixed_bclk_size
As a note: the available matching rules and action rules right now are hard-coded, but this could in principle get extended to be more dynamic via kallsyms_lookup_name that would lookup the requested symbol and assume it is a struct asoc_generic_hw_params_method, on which it could apply several sanity-checks before using the pointers for real.
How about allowing to embed eBPF bytecode in the DT that can be installed as a constraint set? This would allow maximum for flexibility and also make the implementation a lot easier.
On Fri, May 20, 2016 at 09:16:09PM +0200, Lars-Peter Clausen wrote:
How about allowing to embed eBPF bytecode in the DT that can be installed as a constraint set? This would allow maximum for flexibility and also make the implementation a lot easier.
That seems somewhat Linux specific, I'd like to see some DT maintainer buy in on that one as well as Takashi's thoughts.
On Mon, May 23, 2016 at 05:59:31PM +0100, Mark Brown wrote:
On Fri, May 20, 2016 at 09:16:09PM +0200, Lars-Peter Clausen wrote:
How about allowing to embed eBPF bytecode in the DT that can be installed as a constraint set? This would allow maximum for flexibility and also make the implementation a lot easier.
That seems somewhat Linux specific, I'd like to see some DT maintainer buy in on that one as well as Takashi's thoughts.
I am very much not in favour of embedding eBPF bytecode into the DT.
Mark.
On 05/23/2016 07:15 PM, Mark Rutland wrote:
On Mon, May 23, 2016 at 05:59:31PM +0100, Mark Brown wrote:
On Fri, May 20, 2016 at 09:16:09PM +0200, Lars-Peter Clausen wrote:
How about allowing to embed eBPF bytecode in the DT that can be installed as a constraint set? This would allow maximum for flexibility and also make the implementation a lot easier.
That seems somewhat Linux specific, I'd like to see some DT maintainer buy in on that one as well as Takashi's thoughts.
I am very much not in favour of embedding eBPF bytecode into the DT.
This has to be seen in the context of the suggestion to reference kernel symbols by name from the DT.
My opinion is that if the interdependence between the components in your system is non-trivial you should write a proper driver. Otherwise we'll end up with DT-script and replicate all the terrible things from the ACPI world where people ship completely horrendous and broken byte code in the firmware.
On Mon, May 23, 2016 at 08:26:41PM +0200, Lars-Peter Clausen wrote:
My opinion is that if the interdependence between the components in your system is non-trivial you should write a proper driver. Otherwise we'll end
Yes, I tend to agree - I've not looked at the actual patch yet but there's a good, solid reason why we have explicit drivers for system integration. There is no value in pushing absoutely everything into DT, at some point you just end up with something at least as complicated as a real programming language with worse tooling.
up with DT-script and replicate all the terrible things from the ACPI world where people ship completely horrendous and broken byte code in the firmware.
I think that's unfair, there are good solid reasons for having AML and any interface is open to abuse.
On Mon, May 23, 2016 at 06:15:16PM +0100, Mark Rutland wrote:
On Mon, May 23, 2016 at 05:59:31PM +0100, Mark Brown wrote:
On Fri, May 20, 2016 at 09:16:09PM +0200, Lars-Peter Clausen wrote:
How about allowing to embed eBPF bytecode in the DT that can be installed as a constraint set? This would allow maximum for flexibility and also make the implementation a lot easier.
That seems somewhat Linux specific, I'd like to see some DT maintainer buy in on that one as well as Takashi's thoughts.
I am very much not in favour of embedding eBPF bytecode into the DT.
+1 What's wrong with Forth? ;)
Rob
On Mon, 23 May 2016 18:59:31 +0200, Mark Brown wrote:
On Fri, May 20, 2016 at 09:16:09PM +0200, Lars-Peter Clausen wrote:
How about allowing to embed eBPF bytecode in the DT that can be installed as a constraint set? This would allow maximum for flexibility and also make the implementation a lot easier.
That seems somewhat Linux specific, I'd like to see some DT maintainer buy in on that one as well as Takashi's thoughts.
Using eBPF is a good idea for our purposes, but I understand Mark's concern, too. That said, I'm in favor of eBPF as long as DT people don't mind. We'd need some tools but the rest would be easier (less patching) once after it's done.
thanks,
Takashi
On Fri, May 20, 2016 at 12:30:43PM +0000, kernel@martin.sperl.org wrote:
From: Martin Sperl kernel@martin.sperl.org
Simple_card does require under some circumstances the ability to configure certain hw_parameters based on clocks, bits, channels.
This patchset adds a generic way to configure these kind of things via the device tree easily. This patchset implements this for simple_card, but other drivers can just as easily make use of this.
I'm not familiar with the class of hardware here.
What exactly needs to be configured, under which situations?
How varied is this in practice?
Why does this make more sense than having individual drivers?
For now we have the following matchers and actions:
- matchers:
- match_sample_bits
- match_rate
- match_channels
- actions:
- set_fixed_bclk_size
As a note: the available matching rules and action rules right now are hard-coded, but this could in principle get extended to be more dynamic via kallsyms_lookup_name that would lookup the requested symbol and assume it is a struct asoc_generic_hw_params_method, on which it could apply several sanity-checks before using the pointers for real.
I'm hoping that's a joke. ;)
Thanks, Mark.
On 23.05.2016, at 19:18, Mark Rutland mark.rutland@arm.com wrote:
On Fri, May 20, 2016 at 12:30:43PM +0000, kernel@martin.sperl.org wrote:
From: Martin Sperl kernel@martin.sperl.org
Simple_card does require under some circumstances the ability to configure certain hw_parameters based on clocks, bits, channels.
This patchset adds a generic way to configure these kind of things via the device tree easily. This patchset implements this for simple_card, but other drivers can just as easily make use of this.
I'm not familiar with the class of hardware here.
What exactly needs to be configured, under which situations?
Well one example - here specifically is to set up bclk rates that the I2S hw itself is transferring for specific combinations.
Some of these rules depend on: * the i2s driver hw itself - the i2s bcm2835 can only handle 32 bit transfers - so the block size needs to be set as a multiple of 2. in this case: sample-bits * channels. * HW limits - e.g 24 bit/sample need to get transferred as 32 bit for the rpi-pcm5102a combination * Clock selection dependent - the choice of the block size may be also 40 or 80 bit so that a integer divider clock can get selected as a preference. E.g 32 bit/sample * 2 channels * 48kHz gives a divider of 25/4 using the main oscillator at 19.2MHz, which introduces some (possibly) audible noise due to jitter. while 40bit/sample * 2 channels * 48kHz gives an integer divider of 20.
So controlling those helps using the hw setup properly.
How varied is this in practice?
I heard about some other devices besides the HifiBerry DAC that also would like to control the blockrate the same way.
I remember having seen some other possible things we want to control on the driver side during hw-params invocation, but I can not remember what these were on top of my head (GPIO, external clock selection if I remember correctly)
Why does this make more sense than having individual drivers?
Because then the driver would have to know a lot more about the combination of board and codec.
Also the idea proposed by Mark was to use simple-card instead of a dedicated driver which does not allow for these kind of controls.
The idea was also to make it generic enough that it could get even become part of the framework.
For now we have the following matchers and actions:
- matchers:
- match_sample_bits
- match_rate
- match_channels
- actions:
- set_fixed_bclk_size
As a note: the available matching rules and action rules right now are hard-coded, but this could in principle get extended to be more dynamic via kallsyms_lookup_name that would lookup the requested symbol and assume it is a struct asoc_generic_hw_params_method, on which it could apply several sanity-checks before using the pointers for real.
I'm hoping that's a joke. ;)
Just food for thought, because otherwise there would be dependencies on compiled codecs (or a long #ifdef list)
As an alternative we could also add a method tables to the codec structure, so that codecs could expose additional methods, that could then get used.
Martin
participants (6)
-
kernel＠martin.sperl.org -
Lars-Peter Clausen -
Mark Brown -
Mark Rutland -
Rob Herring -
Takashi Iwai