Re: [PATCH v5] ASoC: Add Freescale SGTL5000 codec support

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

On Thu, Feb 24, 2011 at 02:08:21AM +0800, zhaoming.zeng@freescale.com wrote:

This is mostly good, there's a few issues around the regulator usage
though.  I've applied the patch but please send incremental patches to
fix the issues below and also the issue Liam raised with the controls.

> +	.constraints = {
> +		.min_uV                 = 850000,
> +		.max_uV                 = 1600000,
> +		.valid_modes_mask       = REGULATOR_MODE_NORMAL,
> +		.valid_ops_mask         = REGULATOR_CHANGE_STATUS,
> +	},

This regulator supports a range of voltages but does not support
changing the voltage.

> +	/* set regulator value firstly */
> +	reg = (1600 - ldo->voltage / 1000) / 50;
> +	reg = clamp(reg, 0x0, 0xf);
> +
> +	/* amend the voltage value, unit: uV */
> +	ldo->voltage = (1600 - reg * 50) * 1000;
> +
> +	/* set voltage to register */
> +	snd_soc_update_bits(codec, SGTL5000_CHIP_LINREG_CTRL,
> +				(0x1 << 4) - 1, reg);

This looks like it should be in a regulator set_voltage() operation not
here:

> +
> +	snd_soc_update_bits(codec, SGTL5000_CHIP_ANA_POWER,
> +				SGTL5000_LINEREG_D_POWERUP,
> +				SGTL5000_LINEREG_D_POWERUP);

You have a separate enable bit which is probably the only thing you need
to deal with here.  If the hardware needs this better to explain why.

> +	vdda  = regulator_get_voltage(sgtl5000->supplies[VDDA].consumer);
> +	vddio = regulator_get_voltage(sgtl5000->supplies[VDDIO].consumer);
> +	vddd  = regulator_get_voltage(sgtl5000->supplies[VDDD].consumer);
> +
> +	vdda  = vdda / 1000;
> +	vddio = vddio / 1000;
> +	vddd  = vddd / 1000;
> +
> +	if (vdda <= 0 || vddio <= 0 || vddd < 0) {
> +		dev_err(codec->dev, "regulator voltage not set correctly\n");
> +
> +		return -EINVAL;
> +	}

This is going to fail on systems without voltage readback capabilities,
including systems which have the regulator API disabled and stubbed out.

> +	/*
> +	 * copy DAP default values to default value array.
> +	 * sgtl5000 register space has a big hole, merge it
> +	 * at init phase makes life easy.
> +	 * FIXME: should we drop 'const' of sgtl5000_regs?
> +	 */
> +	memcpy((void *)(&sgtl5000_regs[0] + (SGTL5000_DAP_REG_OFFSET >> 1)),
> +			sgtl5000_dap_regs,
> +			SGTL5000_MAX_REG_OFFSET - SGTL5000_DAP_REG_OFFSET);

As has been pointed out on previous revisions of the patch there is no
need to cast away from void in C and it can be actively harmful.