linux-kernel - Re: [PATCH v3 4/5] ASoC: dwc: Add devicetree support for Designware I2S

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Mon, 22 Dec 2014 15:58:49 +0000
From:	Andrew Jackson <Andrew.Jackson@....com>
To:	Mark Brown <broonie@...nel.org>
CC:	Jaroslav Kysela <perex@...ex.cz>, Takashi Iwai <tiwai@...e.de>,
	Liam Girdwood <lgirdwood@...il.com>,
	Rajeev Kumar <rajeevkumar.linux@...il.com>,
	Liviu Dudau <Liviu.Dudau@....com>,
	Lars-Peter Clausen <lars@...afoo.de>,
	Arnd Bergmann <arnd@...db.de>,
	"linux-arm-kernel@...ts.infradead.org" 
	<linux-arm-kernel@...ts.infradead.org>,
	"alsa-devel@...a-project.org" <alsa-devel@...a-project.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v3 4/5] ASoC: dwc: Add devicetree support for Designware
 I2S

On 12/22/14 14:10, Mark Brown wrote:
> On Fri, Dec 19, 2014 at 04:18:08PM +0000, Andrew Jackson wrote:
> 
>> +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

I'll change it.

> 
>> -	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().
> 
>> +/* 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
>> +};
> 
>> +	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.

I will add robustness.  

> 
>> -	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.
> 
>> +#ifdef CONFIG_OF
>> +		.of_match_table = dw_i2s_of_match,
>> +#endif
> 
> of_match_ptr().
> 

Thanks for all the comments.

	Andrew
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/