ASoC: dwc: Add device tree support to designware I2S

From: Andrew Jackson <[email protected]>

Flush the FIFOs when the stream is prepared for use. This avoids an inadvertent swapping of the left/right channels if the FIFOs are not empty at startup.

The Designware core can be configured with up to four stereo channels. Each stereo channel is individually configured so, when the driver's hw_params call is made, each requested stereo channel has to be programmed.

Quoted 4 lines · Andrew Jackson

The Designware core can be configured with up to four stereo channels. Each stereo channel is individually configured so, when the driver's hw_params call is made, each requested stereo channel has to be programmed.

This is quite unclear to someone who doesn't know the hardware, is this a bug fix or a new feature? It looks like it's a fix...

From: Andrew Jackson <[email protected]>

The Designware core can be configured with up to four stereo channels. Each stereo channel is individually configured so, when the driver's hw_params call is made, each requested stereo channel has to be programmed.

From: Andrew Jackson <[email protected]>

Move code that configures the DAI and DMA into a separate function. This reduces the size of the dw_i2s_probe function and will make it easier to add support for device tree to the driver.

sound/soc/dwc/designware_i2s.c +6 -0

+/* Maximum resolution of a channel - not uniformly spaced */
+static const u32 fifo_width[] = {
+	12, 16, 20, 24, 32, 0, 0, 0
+};
+
+/* Width of (DMA) bus */

sound/soc/dwc/designware_i2s.c line 86

Quoted 5 lines · Andrew Jackson

sound/soc/dwc/designware_i2s.c +5 -0

+union snd_dma_data {
+	struct i2s_dma_data pd;
+	struct snd_dmaengine_dai_dma_data dt;
+};
+

This is a driver local union with a very generic name, it seems likely that this will collide in future causing build breaks

sound/soc/dwc/designware_i2s.c static int dw_i2s_hw_params(struct snd_pcm_substream *substream, · line 284

Quoted 9 lines

sound/soc/dwc/designware_i2s.c +3 -4

-	ret = dev->i2s_clk_cfg(config);
-	if (ret < 0) {
-		dev_err(dev->dev, "runtime audio clk config fail\n");
-		return ret;
+		/* TODO: Validate sample rate against permissible set */
+		bitclk = config->sample_rate * config->data_width * 2;
+		clk_set_rate(dev->clk, bitclk);
 	}

This is ignoring errors in clk_set_rate().

Quoted 12 lines

sound/soc/dwc/designware_i2s.c +12 -0

+/* Maximum resolution of a channel - not uniformly spaced */
+static const u32 fifo_width[] = {
+	12, 16, 20, 24, 32, 0, 0, 0
+};
+
+/* Width of (DMA) bus */
+static const u32 bus_widths[] = {
+	DMA_SLAVE_BUSWIDTH_1_BYTE,
+	DMA_SLAVE_BUSWIDTH_2_BYTES,
+	DMA_SLAVE_BUSWIDTH_4_BYTES,
+	DMA_SLAVE_BUSWIDTH_UNDEFINED
+};

sound/soc/dwc/designware_i2s.c static void dw_configure_dai_by_pd(struct dw_i2s_dev *dev, · line 442

Quoted 3 lines

sound/soc/dwc/designware_i2s.c +3 -0

+	u32 bus_width = bus_widths[COMP1_APB_DATA_WIDTH(comp1)];
+	u32 fifo_depth = 1 << (1 + COMP1_FIFO_DEPTH_GLOBAL(comp1));
+	u32 max_size;

I'd feel a lot more comfortable if there were bounds checking on these array indexes, especially since the arrays aren't explicitly sized and instead just have the number of elements that is (hopefully) safe with no comments or anything. As things stand this is all using really fraigle idioms, this could easily be broken if someone is updating the driver for new IP features or even just cleaning up the code.

sound/soc/dwc/designware_i2s.c static int dw_i2s_probe(struct platform_device *pdev) · line 526

Quoted 9 lines

sound/soc/dwc/designware_i2s.c +6 -2

-	dev->i2s_clk_cfg = pdata->i2s_clk_cfg;
-	dev->clk = clk_get(&pdev->dev, NULL);
+		dev->clk = clk_get(&pdev->dev, NULL);
+	} else {
+		dw_configure_dai_by_dt(dev, dw_i2s_dai, res);
+
+		dev->clk = devm_clk_get(&pdev->dev, "i2sclk");
+	}

This changes from clk_get() to devm_clk_get() but I'm not seeing anything that removes clk_put() calls.

sound/soc/dwc/designware_i2s.c static int dw_i2s_remove(struct platform_device *pdev) · line 591

Quoted 3 lines

sound/soc/dwc/designware_i2s.c +3 -0

+#ifdef CONFIG_OF
+		.of_match_table = dw_i2s_of_match,
+#endif

of_match_ptr().

Add documentation for Designware I2S hardware block. The block requires two clocks (one for audio sampling, the other for APB) and DMA channels for receive and transmit.

Quoted 3 lines · Andrew Jackson

Add documentation for Designware I2S hardware block. The block requires two clocks (one for audio sampling, the other for APB) and DMA channels for receive and transmit.

You should generally include the binding before the code to parse it, both because the binding is required in order to tell if the code is doing the right thing and also because people will often not even look at code with a missing binding.

Documentation/devicetree/bindings/sound/designware-i2s.txt line 10

Quoted 5 lines

+ - clocks : Pairs of phandle and specifier referencing the controller's clocks.

Documentation/devicetree/bindings/sound/designware-i2s.txt new file +4 -0

+   The controller expects two clocks, the clock used for the APB interface and
+   the clock used as the sampling rate reference clock sample.
+ - clock-names : "apb_plck" for the clock to the APB interface, "i2sclk" for the sample
+   rate reference clock.

This is a name based lookup of clocks but the code doesn't use apb_pclk at all; it needs to or the binding needs to say that apb_pclk must be the first listed clock (which would not be good).

Documentation/devicetree/bindings/sound/designware-i2s.txt line 32

Quoted 9 lines

Documentation/devicetree/bindings/sound/designware-i2s.txt new file +9 -0

+	soc_i2s: i2s at 7ff90000 {
+		compatible = "snps,designware-i2s";
+		reg = <0x0 0x7ff90000 0x0 0x1000>;
+		clocks = <&scpi_i2sclk 0>, <&soc_refclk100mhz>;
+		clock-names = "i2sclk", "apb_pclk";
+		#sound-dai-cells = <0>;
+		dmas = <&dma0 5>;
+		dma-names = "tx";
+	};

This omits a lot of configurability that is in platform data and

replaces it by reading back the parameters from the hardware. If this is a viable approach to that configuration you should do this for both platform data and device tree rather than only device tree. The point with keeping platform data is that it's not good to make the device DT only, improving the usability of platform data in a way that happens to also make the DT case easier is totally fine. If we can determine how the IP is configured from the hardware that's both less work and more robust no matter how the device is instantiated.

Quoted 3 lines · Mark Brown

This is a name based lookup of clocks but the code doesn't use apb_pclk at all; it needs to or the binding needs to say that apb_pclk must be the first listed clock (which would not be good).

I can remove apb_pclk: I was modelling the device tree entry on various PLxxx examples (c.f. amba-pl011) which also reference an AMBA clock but don't use it. (The effect being to document what clock the block is driven by.)