[alsa-devel] [PATCH] ASoC: add rt5665 codec driver

[Mark Brown]

On Mon, Oct 31, 2016 at 04:37:59PM +0800, Bard Liao wrote:

> +	{"TDM2 Data Mux", "1324", "IF1_2 ADC TDM"},
> +	{"TDM2 Data Mux", "1342", "IF1_2 ADC TDM"},

These aren't really done properly - they're squashing all the data paths
down into one.  Ideally there'd be a separate AIF widget for each
channel.  It's not super urgent right now since it won't make a
practical difference.

> +	regulator_1v8 = devm_regulator_get_optional(&i2c->dev,
> +						rt5665->pdata.regulator_1v8);
> +	if (IS_ERR(regulator_1v8))
> +		dev_err(&i2c->dev, "Fail to get regulator_1v8\n");
> +	else if (regulator_enable(regulator_1v8))
> +		dev_err(&i2c->dev, "Fail to enable regulator_1v8\n");

This isn't good, while we log if we fail to enable the regulator we
don't otherwise handle the error.  If the we failed to power on a power
supply the chances are that the device isn't going to work so I'd expect
us to fail the proble.

> +	regulator_3v3 = devm_regulator_get_optional(&i2c->dev,
> +						rt5665->pdata.regulator_3v3);

> +	regulator_5v = devm_regulator_get_optional(&i2c->dev,
> +						rt5665->pdata.regulator_5v);

I also just don't believe that *all* of the regulators the device has
are optional.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 455 bytes
Desc: not available
URL: <http://mailman.alsa-project.org/pipermail/alsa-devel/attachments/20161031/4c37498e/attachment.sig>