Re: [Patch v2 04/11] ASoC: codec: Add Maxim codec driver

[Mark Brown <broonie@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 1813 bytes --]

On Mon, Dec 08, 2014 at 02:01:06PM -0800, Kenneth Westfield wrote:

> +enum pinctrl_pin_state {
> +	STATE_DISABLED = 0,
> +	STATE_ENABLED = 1
> +};
> +static const char * const pin_states[] = {"Disabled", "Enabled"};

This looks like you are trying to reimplement some of the generic
support provided by the pinctrl framework - please don't do that.  It
looks like you should be using the standard idle and default states.

However I'm also questioning why this device is using pinctrl at all.
As far as I can see from the code it's a dumb external device with just
an enable control and hence no pin control support so it's not a device
I'd expect to have any pinmux to control.  Why is it doing this?  There
are also substantial problems throughout the relevant code but probably
the best thing is just to remove it all.

> +static int max98357a_codec_set_pinctrl(struct max98357a_codec_pinctrl *mi2s)
> +{
> +	int ret;
> +
> +	pr_debug("%s: curr_state = %s\n", __func__,
> +			pin_states[mi2s->curr_state]);

To repeat my previous review comments: use dev_ prints.

> +static struct snd_soc_dai_driver max98357a_codec_dai_driver = {
> +	.name = "max98357a-codec-dai",
> +	.playback = {
> +		.stream_name	= "max98357a-codec-playback",
> +		.formats	= SNDRV_PCM_FMTBIT_S16 |
> +					SNDRV_PCM_FMTBIT_S24 |
> +					SNDRV_PCM_FMTBIT_S32,
> +		.rates		= SNDRV_PCM_RATE_8000 |
> +					SNDRV_PCM_RATE_16000 |
> +					SNDRV_PCM_RATE_48000 |
> +					SNDRV_PCM_RATE_96000,
> +		.rate_min	= 8000,
> +		.rate_max	= 96000,
> +		.channels_min	= 1,
> +		.channels_max	= 2,
> +	},
> +	.probe = &max98357a_codec_dai_probe,
> +	.ops = &max98357a_codec_dai_ops,
> +};

This CODEC driver has no DAPM support.  I'm surprised this works at all,
it's certainly not OK for upstream - you need to implement at least stub
DAPM support.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]