Messages in this thread | | | From | John Stultz <> | Date | Wed, 20 Jul 2016 09:53:09 -0700 | Subject | Re: [RFC][PATCH 07/10 v2] ASoC: hisilicon: Add hi6210 i2s audio driver |
| |
On Tue, Jul 19, 2016 at 5:28 PM, Mark Brown <broonie@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
| |