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

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.