On Tue, Jul 19, 2016 at 5:28 PM, Mark Brown <broo...@kernel.org> wrote: > On Tue, Jul 19, 2016 at 04:22:43PM -0700, John Stultz wrote: > >> sound/soc/Kconfig | 1 + >> sound/soc/Makefile | 1 + >> sound/soc/hisilicon/Kconfig | 5 + >> sound/soc/hisilicon/Makefile | 1 + >> sound/soc/hisilicon/hi6210-i2s.c | 678 >> +++++++++++++++++++++++++++++++++++++++ >> sound/soc/hisilicon/hi6210-i2s.h | 276 ++++++++++++++++ >> 6 files changed, 962 insertions(+) > > This is adding a new binding without documenting it and still looks like > it's far more than an I2S controller.
So I added some binding documentation in the patch that proceeded this... Oh Crud. I forgot to add the cc list to those files. Sorry for that. Here they are on the list: https://lkml.org/lkml/2016/7/19/820 https://lkml.org/lkml/2016/7/19/815 As for being more then an i2s controller, I don't have access to the docs this was written with, so I'm not really sure what I can do to properly extend this driver beyond acting as an i2s driver. I'll take a look, but again, my lack of familiarity with ASoC means I may need some extra guidance. So apologies up front. >> + switch (params_rate(params)) { >> + default: >> + dev_err(cpu_dai->dev, "Bad rate\n"); >> + return -EINVAL; > > We should tell the user what rate. > >> + if (bits == HII2S_BITS_24) { >> + i2s->bits = 32; >> + dma_data->addr_width = 3; >> + } else { >> + i2s->bits = 16; >> + dma_data->addr_width = 2; >> + } > > This looks like it should be a switch statement, there's some similar > stuff for the channels. So yea, the switch selection above this does the validation and then I'm just applying the change here. I can duplicate the switch in both cases, but this seems a little more terse. >> + _hi6210_i2s_set_fmt(i2s, substream); > > Why is this not in line given that this is the only user? I think it breaks up the function some, but I can move it inline. >> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); >> + if (!res) { >> + ret = -ENODEV; >> + goto err2; >> + } >> + >> + i2s->base = devm_ioremap_resource(dev, res); > > devm_ioremap_resource() will check the error. > >> +err3: >> + while (--i2s->clocks) >> + clk_put(i2s->clk[i2s->clocks]); >> + >> +err2: >> + kfree(i2s); > > You switched to using devm_ but left the error handling. Ok, will address. Thanks for the review and feedback! -john
