Re: [PATCH 1/8] ASoC: TPA6130A2 amplifier driver

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

On Thu, Oct 08, 2009 at 02:58:50PM +0300, Eduardo Valentin wrote:

> +struct tpa6130a2_platform_data {
> +	int (*set_power)(int state);
> +};

Why is this a callback and not just a GPIO number?  That'd seem simpler
for users.

> +int tpa6130a2_add_controls(struct snd_soc_codec *codec)
> +{
> +	snd_soc_dapm_new_controls(codec, tpa6130a2_dapm_widgets,
> +				ARRAY_SIZE(tpa6130a2_dapm_widgets));
> +
> +	return snd_soc_add_controls(codec, tpa6130a2_controls,
> +				ARRAY_SIZE(tpa6130a2_controls));
> +
> +}
> +EXPORT_SYMBOL(tpa6130a2_add_controls);

All the ASoC APIs are EXPORT_SYMBOL_GPL().

> +	pdata = (struct tpa6130a2_platform_data *)client->dev.platform_data;
> +	/* Set default register values */
> +	data->regs[TPA6130A2_REG_CONTROL] = TPA6130A2_SWS |
> +					    TPA6130A2_HP_EN_R |
> +					    TPA6130A2_HP_EN_L;

This looks like you could implement stereo paths with individual power
control?

> +	data->regs[TPA6130A2_REG_VOL_MUTE] = TPA6130A2_VOLUME(20);

The standard thing is to use hardware defaults and leave decisions like
this up to user space.

> +	data->set_power = pdata->set_power;
> +	if (!pdata->set_power) {
> +		data->power_state = 1;
> +		tpa6130a2_initialize();
> +	}
> +	tpa6130a2_power(1);
> +
> +	/* Read version */
> +	err = tpa6130a2_i2c_read(TPA6130A2_REG_VERSION) &
> +				 TPA6130A2_VERSION_MASK;
> +	if ((err != 1) && (err != 2)) {
> +		dev_err(dev, "Unexpected headphone amplifier chip version "
> +		       "of 0x%02x, was expecting 0x01 or 0x02\n", err);
> +		err = -ENODEV;

This seems a bit excessive - is it really likely that the register map
would be changed incompatibly in a future version?