On Fri, Oct 25, 2013 at 03:14:20AM +0100, Kuninori Morimoto wrote:
> Support for loading the simple-card module via DeviceTree.
> It requests CPU/CODEC information.
>
> Signed-off-by: Kuninori Morimoto <[email protected]>
> ---
> v3 -> v4
>
> - explain detail of each properties on simple-card.txt
> - fixup odd examples on simple-card.txt
> - remove "simple-card,card-name". create it from cpu/codec name
> - use of_get_child_by_name()
> - remove odd pointer info from dev_dbg()
> - remove subnode format which are no longer needed
>
> This is based on asoc/topic/simple branch
>
> .../devicetree/bindings/sound/simple-card.txt | 73 +++++++++
> sound/soc/generic/simple-card.c | 156
> +++++++++++++++++++-
> 2 files changed, 223 insertions(+), 6 deletions(-)
> create mode 100644 Documentation/devicetree/bindings/sound/simple-card.txt
>
> diff --git a/Documentation/devicetree/bindings/sound/simple-card.txt
> b/Documentation/devicetree/bindings/sound/simple-card.txt
> new file mode 100644
> index 0000000..4871e91
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/sound/simple-card.txt
> @@ -0,0 +1,73 @@
> +Simple-Card:
> +
> +Simple-Card specifies audio DAI connection of SoC <-> codec.
> +
> +Required properties:
> +
> +- compatible : "simple-audio"
> +
> +Optional properties:
> +
> +- simple-audio,format : CPU/CODEC common audio format.
> + "i2s", "right_j", "left_j" , "dsp_a"
> + "dsp_b", "ac97", "pdm", "msb", "lsb"
> +Required subnodes:
> +
> +- simple-audio,cpu : CPU sub-node
> +- simple-audio,codec : CODEC sub-node
> +
> +Required CPU/CODEC subnodes properties:
> +
> +- sound-dai : phandle and port of CPU/CODEC
Is there a class binding for audio devices this derives from?
> +
> +Optional CPU/CODEC subnodes properties:
Do these all apply to both sub-nodes?
> +- frame-master : bool property. add this if
> subnode was frame master
> +- bitclock-master : bool property. add this if subnode
> was bitclock master
s/was/is/
> +- bitclock-inversion : bool property. add this if subnode
> has clock inversion
> +- frame-inversion : bool property. add this if subnode
> has frame inversion
> +- clocks / system-clock-frequency : specify subnode's clock if needed.
> + it can be specified via "clocks" if
> system has clock node,
> + or "system-clock-frequency" if
> system doesn't have it.
What does "if system doesn't have it" mean? If it doesn't have a clock,
how does said non-existent clock have a frequency?
It would be possible to use a fixed-clock in place of
system-clock-frequency, which would make the binding more consistent and
the driver simpler, at the cost of making the dt marginally more
complex.
> +
> +Example:
> +
> +clock {
Why is this container here? It's confusing and unnecessary.
> + osc: oscillator {
> + #clock-cells = <0>;
> + compatible = "fixed-clock";
> + clock-frequency = <11289600>;
> + };
> +};
[...]
> +static int
> +asoc_simple_card_sub_parse_of(struct device_node *np,
> + struct asoc_simple_dai *dai,
> + struct device_node **node)
> +{
> + struct clk *clk;
> + int ret;
> +
> + /*
> + * get node via "sound-dai = <&phandle port>"
> + * it will be used as xxx_of_node on soc_bind_dai_link()
> + */
> + *node = of_parse_phandle(np, "sound-dai", 0);
> + if (!*node)
> + return -ENODEV;
> +
> + /* get dai->name */
> + ret = snd_soc_of_get_dai_name(np, &dai->name);
> + if (ret < 0)
> + goto parse_error;
> +
> + /*
> + * bitclock-inversion, frame-inversion
> + * bitclock-master, frame-master
> + * and specific "format" if it has
> + */
This comment looks confusing to me. I'm not sure it's all that helpful.
> + dai->fmt = snd_soc_of_parse_daifmt(np, NULL);
As a general note, I'm surprised there isn't a helper that does all of
the above, from of_parse_phandle to here.
> +
> + /*
> + * dai->sysclk come from
> + * "clolks = <&xxx>" or "system-clock-frequency = <xxx>"
s/clolks/clocks/
> + */
> + clk = of_clk_get(np, 0);
> + if (IS_ERR(clk))
> + of_property_read_u32(np,
> + "system-clock-frequency",
> + &dai->sysclk);
What it this isn't present?
> + else
> + dai->sysclk = clk_get_rate(clk);
> +
> + ret = 0;
> +
> +parse_error:
> + of_node_put(*node);
> +
> + return ret;
> +}
> +
> +static int asoc_simple_card_parse_of(struct device_node *node,
> + struct asoc_simple_card_info *info,
> + struct device *dev,
> + struct device_node **of_cpu,
> + struct device_node **of_codec,
> + struct device_node **of_platform)
> +{
> + struct device_node *np;
> + char *name;
> + int ret = 0;
> +
> + /* get CPU/CODEC common format via simple-audio,format */
> + info->daifmt = snd_soc_of_parse_daifmt(node, "simple-audio,") &
> + (SND_SOC_DAIFMT_FORMAT_MASK | SND_SOC_DAIFMT_INV_MASK);
> +
> + /* CPU sub-node */
> + ret = -EINVAL;
> + np = of_get_child_by_name(node, "simple-audio,cpu");
> + if (np)
> + ret = asoc_simple_card_sub_parse_of(np,
> + &info->cpu_dai,
> + of_cpu);
> + if (ret < 0)
> + return ret;
> +
> + /* CODEC sub-node */
> + ret = -EINVAL;
> + np = of_get_child_by_name(node, "simple-audio,codec");
> + if (np)
> + ret = asoc_simple_card_sub_parse_of(np,
> + &info->codec_dai,
> + of_codec);
> + if (ret < 0)
> + return ret;
> +
> + /* card name is created from CPU/CODEC dai name */
> + of_property_read_string(node, "simple-audio,card-name", &info->card);
> + name = devm_kzalloc(dev,
> + strlen(info->cpu_dai.name) +
> + strlen(info->codec_dai.name) + 2,
> + GFP_KERNEL);
> + sprintf(name, "%s-%s", info->cpu_dai.name, info->codec_dai.name);
> + info->name = info->card = name;
> +
> + /* simple-card assumes platform == cpu */
> + *of_platform = *of_cpu;
Why? What does this imply?
Thanks,
Mark.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
