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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
