Re: [alsa-devel] [PATCH/RFC 12/14] ASoC: samsung: i2s: Add clock provider for the I2S internal clocks

[Mark Brown <broonie@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 1823 bytes --]

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 #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]