linux-kernel - Re: [PATCH 06/14] ASoC: Add sun8i digital audio codec

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Tue, 4 Oct 2016 15:07:27 +0200
From:   Mark Brown <broonie@...nel.org>
To:     Thomas Petazzoni <thomas.petazzoni@...e-electrons.com>
Cc:     Mylène Josserand 
        <mylene.josserand@...e-electrons.com>, vinod.koul@...el.com,
        maxime.ripard@...e-electrons.com, wens@...e.org,
        mturquette@...libre.com, sboyd@...eaurora.org, lgirdwood@...il.com,
        perex@...ex.cz, tiwai@...e.com, lee.jones@...aro.org,
        mark.rutland@....com, robh+dt@...nel.org,
        linux-kernel@...r.kernel.org, dmaengine@...r.kernel.org,
        linux-arm-kernel@...ts.infradead.org, linux-clk@...r.kernel.org,
        alsa-devel@...a-project.org, devicetree@...r.kernel.org,
        linux-sunxi@...glegroups.com, alexandre.belloni@...e-electrons.com
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().

Download attachment "signature.asc" of type "application/pgp-signature" (456 bytes)