Messages in this thread | | | Date | Tue, 4 Oct 2016 15:07:27 +0200 | From | Mark Brown <> | Subject | Re: [PATCH 06/14] ASoC: Add sun8i digital audio codec |
| |
On Tue, Oct 04, 2016 at 02:40:08PM +0200, Thomas Petazzoni wrote:
> > +/* CODEC_OFFSET represents the offset of the codec registers > > + * and not all the DAI registers > > + */
> This is not the proper comment style I believe for audio code, it > should be:
> /* > * ... > */
I don't care, IIRC that's something from CodingStyle which checkpatch moans about.
> > + /* pcm operations */ > > + .ops = &sun8i_codec_dai_ops, > > +}; > > +EXPORT_SYMBOL(sun8i_codec_dai);
> This EXPORT_SYMBOL looks wrong. First because it doesn't seem to be > used outside of this module. And second because using EXPORT_SYMBOL on > a function defined as static doesn't make much sense, as the "static" > qualifier limits the visibility of the symbol to the current > compilation unit.
Also all the ASoC code is _GPL so a non-GPL export is an issue.
> > + .component_driver = { > > + .dapm_widgets = sun8i_codec_dapm_widgets, > > + .num_dapm_widgets = ARRAY_SIZE(sun8i_codec_dapm_widgets), > > + .dapm_routes = sun8i_codec_dapm_routes, > > + .num_dapm_routes = ARRAY_SIZE(sun8i_codec_dapm_routes),
> I'm probably missing something, but in the sun4i-codec.c driver, those > fields are initialized directly in the snd_soc_codec_driver structure, > not in the .component_driver sub-structure.
We're in the process of pushing everything out to component level, this update should be made in the old code if it's not happened already.
> > + if (clk_prepare_enable(scodec->clk_module)) > > + pr_err("err:open failed;\n");
> Grr, pr_err, not good. Plus you want to return with an error from the > probe() function.
Also when printing an error message use dev_err(). [unhandled content-type:application/pgp-signature] | |