lkml.org 
[lkml]   [2016]   [Oct]   [4]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [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]
\
 
 \ /
  Last update: 2016-10-04 15:09    [W:1.358 / U:0.128 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site