Hi,
On Fri, Oct 04, 2013 at 01:04:41AM +0100, Kuninori Morimoto wrote:
> Support for loading the simple-card module via DeviceTree.
> It requests CPU/CODEC information,
> and .of_xlate_dai_name support on each component driver.
>
> Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@xxxxxxxxxxx>
> ---
> v1 -> v2
>
> - add common clock support, system-clock-frequency can over-write it
> - add some comment on code
>
> v2 -> v3
>
> - add devicetree ML address
>
> .../devicetree/bindings/sound/simple-card.txt | 85 ++++++++++++
> sound/soc/generic/simple-card.c | 146 +++++++++++++++++++-
> 2 files changed, 226 insertions(+), 5 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..75bbc5a
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/sound/simple-card.txt
> @@ -0,0 +1,85 @@
> +Simple-Card:
It's really difficult to review this without a description. Please explain what
this binding represents.
What hardware is this applicable to?
Does this inherit from some class of binding?
> +
> +Required properties:
> +
> +- compatible : "simple-audio"
> +- simple-audio,card-name : simple-audio card name
What's this used for?
> +- simple-audio,cpu : CPU sub-node, see below
> +- simple-audio,codec : CODEC sub-node, see below
Describe these as required subnodes. Nodes are not properties.
> +
> +Optional properties:
> +
> +- simple-audio,format : CPU/CODEC common format, see below
> +
> +Required cpu/codec subnode properties:
> +
> +- sound-dai : phandle and port for CPU/CODEC
> +- #sound-dai-cells : sound-dai phandle's node
This description makes no sense, the organisation seems structurally wrong.
What does this mean? What does it affect?
> +
> +Optional subnode properties:
> +
> +- format : specific format if needed, see below
> +- frame-master : frame master
> +- bitclock-master : bitclock master
> +- bitclock-inversion : clock inversion
> +- frame-inversion : frame inversion
What do these mean? Repeating the name without a dash is completely unhelpful.
Describe what these imply.
What type are they?
Which subnode(s) do they apply to?
> +- clocks : phandle for system clock rate
Just one clock?
Nit: clocks are specified with a phandle + clock-specifier pair, not just a phandle.
> +- system-clock-frequency : system clock rate
> + it will overwrite clocks's rate
This seems very odd.
Why do you want to overwrite a clock's rate?
> +
> +simple-audio,format
> + "i2s"
> + "right_j"
> + "left_j"
> + "dsp_a"
> + "dsp_b"
> + "ac97"
> + "pdm"
> + "msb"
> + "lsb"
What do these mean? Why are they not described when the property was defined above?
Does this also apply for the "format" property?
> +
> +Example:
> +
> +clock {
> + osc: oscillator {
> + #clock-cells = <0>;
> + compatible = "fixed-clock";
> + clock-frequency = <11289600>;
> + };
> +};
> +
> +sound {
> + compatible = "simple-audio";
> +
> + simple-audio,card-name = "FSI2A-AK4648";
> + simple-audio,format = "left_j";
> +
> + simple-audio,cpu {
> + sound-dai = <&sh_fsi2 0>;
> + };
> +
> + simple-audio,codec {
> + sound-dai = <&ak4648>;
> + bitclock-master;
> + frame-master;
> + clocks = <&osc>;
> + /* it can use this instead of clocks
> + * system-clock-frequency = <11289600>; */
This just confuses matters. If ou want to show this off, have two examples.
> + };
> +};
> +
> +&i2c0 {
> + ak4648: ak4648@0x12 {
Nit: remove the 0x on the unit-address
> + #sound-dai-cells = <0>;
> + compatible = "asahi-kasei,ak4648";
> + reg = <0x12>;
> + };
> +};
> +
> +sh_fsi2: sh_fsi2@0xec230000 {
Again, fix the unit-address please.
> + #sound-dai-cells = <1>;
> + compatible = "renesas,sh_fsi2";
> + reg = <0xec230000 0x400>;
> + interrupt-parent = <&gic>;
> + interrupts = <0 146 0x4>;
> +};
> diff --git a/sound/soc/generic/simple-card.c b/sound/soc/generic/simple-card.c
> index 8c49147..62befbd 100644
> --- a/sound/soc/generic/simple-card.c
> +++ b/sound/soc/generic/simple-card.c
> @@ -9,6 +9,8 @@
> * published by the Free Software Foundation.
> */
>
> +#include <linux/clk.h>
> +#include <linux/of.h>
> #include <linux/platform_device.h>
> #include <linux/module.h>
> #include <sound/simple_card.h>
> @@ -52,11 +54,135 @@ static int asoc_simple_card_dai_init(struct snd_soc_pcm_runtime *rtd)
> return 0;
> }
>
> +static int
> +__asoc_simple_card_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;
> +
> + of_node_put(*node);
Why?
You're refrering to it, so why do you not want to have it refcounted?
It could disappear under your feet.
> +
> + /* get dai->name */
> + ret = snd_soc_of_get_dai_name(np, &dai->name);
> + if (ret < 0)
> + return ret;
> +
> + /*
> + * bitclock-inversion, frame-inversion
> + * bitclock-master, frame-master
> + * and specific "format" if it has
> + */
> + dai->fmt = snd_soc_of_parse_daifmt(np, NULL);
> +
> + /* dai->sysclk via "clolks = <xxx>" */
> + clk = of_clk_get(np, 0);
> + if (IS_ERR(clk))
> + dai->sysclk = 0;
> + else
> + dai->sysclk = clk_get_rate(clk);
This seems like an odd assumption to make. Why not error?
> +
> + /*
> + * overwrite dai->sysclk if it has
> + * "system-clock-frequency = <xxx>"
> + */
> + of_property_read_u32(np,
> + "system-clock-frequency",
> + &dai->sysclk);
Is dai->sysclk defined as a u32?
> +
> + return 0;
> +}
> +
> +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;
> + int ret = 0;
> +
> + /* get card name */
> + of_property_read_string(node, "simple-audio,card-name", &info->card);
> + info->name = info->card;
What if the string is not in the DT?
Does the core code handle these being NULL?
> +
> + /* 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/CODEC sub-node */
> + for_each_child_of_node(node, np) {
> + if (0 == strcmp("simple-audio,cpu", np->name))
> + ret = __asoc_simple_card_parse_of(np,
> + &info->cpu_dai,
> + of_cpu);
> + if (0 == strcmp("simple-audio,codec", np->name))
> + ret = __asoc_simple_card_parse_of(np,
> + &info->codec_dai,
> + of_codec);
of_get_child_by_name?
> + if (ret < 0)
> + return ret;
> + }
What if there were no children?
> +
> + /* simple-card assumes platform == cpu */
> + *of_platform = *of_cpu;
> +
> + dev_dbg(dev, "card-name : %s\n", info->card);
> + dev_dbg(dev, "platform : %04x / %p\n",
> + info->daifmt,
> + *of_platform);
Why is the pointer helpful, rather than the info it points to?
Thanks,
Mark.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
