Re: [PATCH 4/4] ASoC: firmware core: Add core support to create and destroy firmware components.

[Mark Brown <broonie@opensource.wolfsonmicro.com>]

[-- Attachment #1.1: Type: text/plain, Size: 2159 bytes --]

On Tue, Nov 20, 2012 at 03:14:30PM +0000, Liam Girdwood wrote:
> On 20/11/12 03:27, Mark Brown wrote:

> >With coefficeints we have been moving towards a model where we just
> >expose them as binary controls rather than doing the enumeration thing.

> For ABE we have different coefficients tied to each enumerated EQU
> kcontrol setting and just configure the ABE coefficient depending on
> the kcontrol value. We load the coefficients and EQU kcontrol at the
> same time since it's enumerated text strings are coupled to the
> coefficients.

So, the text strings for coefficients were what drivers used to be using 

> We need to store on file, either an array of strings or an array of
> u32 values for the enumerated kcontrols (based on soc_enum). This
> gives us a fixed size file entry which is easy to read, write and
> count. But maybe I'm missing something from your question...

The issue is that when we have the u32 array we also need the strings to
be able to present the control - I would suggest just having the format
always include both the numbers and the strings so we don't need to
worry about the special case where the numbers are 0, 1, 2...  or
matching things up if the idea is that we have two separate records.

> >>+	dev_info(sfw->dev, "ASoC: no handler specified for ext widget %s\n",
> >>+		w->name);
> >>+	return 0;

> >Shouldn't this be returning an error?  The same is true for a bunch of
> >other functions.  Or shouild it be a dev_dbg()?

> It should probably have a comment. This is an optional callback to
> component drivers in case they want to use set the widgets private
> data at widget init time.

In that case the log message should definitely be dev_dbg().

> >This one is going to be especially bad for example.

> >>+	if (se->dvalues)
> >>+		kfree(se->dvalues);
> >>+	else {

> >Braces on both sides if they're needed on one.

> The last three points are personal taste, I don't mind too much so
> I'll change them ;)

They're all kernel coding style rules (the issue with breaking log
messages is a particular bugbear of Linus IIRC, rmk also gets very
upset).

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]