On Thu, Dec 11, 2014 at 06:45:50PM +0100, Sylwester Nawrocki wrote:
> +Optional Properties:
>
> - samsung,idma-addr: Internal DMA register base address of the audio
> sub system(used in secondary sound source).
> - pinctrl-0: Should specify pin control groups used for this controller.
> - pinctrl-names: Should contain only one value - "default".
> +- #clock-cells: should be 1, this property must be present if the I2S device
> + is a clock provider in terms of the common clock bindings, described in
> + ../clock/clock-bindings.txt.
> +- clock-output-names: from the common clock bindings, names of the CDCLK
> + I2S output clocks, suggested values are "i2s_cdclk0", "i2s_cdclk1",
> + "i2s_cdclk3" for the I2S0, I2S1, I2S2 devices recpectively.
> +
Why not make this mandatory going forwards? You can add a note saying
that older DTs may not have it.
> +The assignment of indices for the I2Sx clock provider is as follows:
> + 0 - the CDCLK (CODECLKO) gate clock,
> + 1 - the RCLK prescaler divider clock (corresponding to the IISPSR register),
> + 2 - the RCLKSRC mux clock (corresponding to RCLKSRC bit in register IISMOD).
Why use the indicies here, they only seem to add obscurity?
> + clk_put(rclksrc);
> + }
> + /*
Missing blank line.
> + * Register the mux and div clocks only if both source clocks
> + * are available, i.e. for the I2S0 instance and versions with
> + * QUIRK_NO_MUXPSR quirk not set.
> + */
Why not proceed even if one is missing? This repeats in words the if
statement but doesn't explain why we're doing this.
> + ret = of_clk_add_provider(dev->of_node, of_clk_src_onecell_get,
> + &i2s->clk_data);
> + if (ret < 0) {
> + dev_err(dev, "failed to add clock provider\n");
Best practice is to print the error code.
Attachment:
signature.asc
Description: Digital signature
