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: 5476 bytes --]

On Mon, Nov 19, 2012 at 06:12:45PM +0000, Liam Girdwood wrote:
> Add generic support to create ASoC kcontrols, widgets, graphs and coefficients from

Word wrapping issue with the commit log here...

This is all really good stuff.  I've nitpicked below and there's a few
bigger things but overall it looks good.

The main thing that's standing out for me is the issue I mentioned in my
reply to patch 3 with multiple firmwares per device; we've got a bunch
of CODECs in mainline already with several independant DSPs in them
which would want to be managed independently.  It'd be good to at least
write the APIs to support that so that we don't churn the driver APIs
too much.

One thing I'm missing is signal generator widgets - those seem like
something that's likely to be often software defined.  It'd also be good
to see a bit more documentation for the file format, like an explanation
of what things like coefficients are since we're going to make it an
ABI.

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

> + * File andBlock header data types.

Typo.

> +/*
> + * File and Block Header
> + */
> +struct snd_soc_fw_hdr {
> +	u32 magic;
> +	u32 type;
> +	u32 vendor_type; /* optional vendor specific type info */
> +	u32 version; /* optional vendor specific version details */
> +	u32 size; /* data bytes, excluding this header */
> +	/* file data contents start here */
> +};

As Takashi says we should take care of byte ordering issues with this
stuff.  For the stuff I've done recently I ended up marking all the
structs __packed and using __be32 and similar for the types.

Obviously on-SoC devices will only be used with a single AP so it's not
a big deal there but when this is used on external devices we're going
to run into issues sooner or later.

> +struct snd_soc_fw_ctl_tlv {
> +	u32 numid;	/* control element numeric identification */
> +	u32 length;	/* in bytes aligned to 4 */
> +	/* tlv data starts here */
> +};

This is just raw ALSA TLV data (it's already an ABI after all).

> +/*
> + * Mixer KControl.
> + */

kcontrol.

> +struct snd_soc_fw_mixer_control {
> +	struct snd_soc_fw_control_hdr hdr;
> +	s32 min;
> +	s32 max;
> +	s32 platform_max;

I guess platform_max isn't needed here as it's something the platform
adds to further restrict capabilities of devices.  Though quite how
platforms will do that for firmwares is an interesting question in
itself :)

> +struct snd_soc_fw_enum_control {
> +	struct snd_soc_fw_control_hdr hdr;
> +	u32 reg;
> +	u32 reg2;
> +	u32 shift_l;
> +	u32 shift_r;
> +	u32 max;
> +	u32 mask;
> +	union {	/* both texts and values are the same size */
> +		char texts[SND_SOC_FW_NUM_TEXTS][SND_SOC_FW_TEXT_SIZE];
> +		u32 values[SND_SOC_FW_NUM_TEXTS * SND_SOC_FW_TEXT_SIZE / 4];

I'm having a hard time visualising how this will get filled in - I'd
expect we'd have something more like:

	u32 num_values;
	struct {
		u32 value;
		char text[SND_SOC_FW_TEXT_SIZE];
	};

- I can't visualise how something with only values would work?  But
perhaps I'm just missing something.

> +/*
> + * DAPM Pin Element.
> + */
> +struct snd_soc_fw_dapm_pin_elem {
> +	char name[SND_SOC_FW_TEXT_SIZE];
> +	u32 disconnect:1;
> +	u32 ignore_suspend:1;
> +};

I think we should probably just skip pins for now - software defined
pins are a bit fun in hardware and 

ignore_suspend is a machine driver thing, and if I were a firmware
author I'd just be skipping disconnected pins entirely rather than
showing people I'd been using firmware resources for things that don't
do anything :)

> +static inline struct snd_soc_dapm_context *soc_fw_dapm_get(struct soc_fw *sfw)
> +{
> +	if (sfw->codec)
> +		return &sfw->codec->dapm;
> +	else if (sfw->platform)
> +		return &sfw->platform->dapm;
> +	else if (sfw->card)
> +		return &sfw->card->dapm;
> +	BUG();

We should probably have those BUG() calls on the sets too.

> +static int soc_fw_widget_load(struct soc_fw *sfw, struct snd_soc_dapm_widget *w)
> +{
> +	if (sfw->codec && sfw->codec_ops && sfw->codec_ops->widget_load)
> +		return sfw->codec_ops->widget_load(sfw->codec, w);
> +
> +	if (sfw->platform && sfw->platform_ops && sfw->platform_ops->widget_load)
> +		return sfw->platform_ops->widget_load(sfw->platform, w);
> +
> +	if (sfw->card && sfw->card_ops && sfw->card_ops->widget_load)
> +		return sfw->card_ops->widget_load(sfw->card, w);
> +
> +	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()?

> +static inline void soc_fw_free_tlv(struct soc_fw *sfw,
> +	struct snd_kcontrol_new *kc)
> +{
> +	kfree (kc->tlv.p);

Coding style, kfree().

> +		dev_dbg(sfw->dev, "ASoC: adding mixer kcontrol %s with access"
> +			" 0x%x\n", mc->hdr.name, mc->hdr.access);

Don't split log messages like this, makes it harder to grep - just break
the 80 columns or wrap after sfw->dev.

> +			if (ext) {
> +				dev_err(sfw->dev, "ASoC: no complete mixer IO"
> +					"handler for %s type (g,p,i) %d:%d:%d\n",

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.

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

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