Prev: [tip:perf/urgent] perf: Fix crash in swevents
Next: patch staging-phison-depends-on-ata_bmdma.patch added to gregkh-2.6 tree
From: Mark Brown on 3 Jun 2010 14:00
On Wed, Jun 02, 2010 at 09:12:26PM +0200, Lars-Peter Clausen wrote:

> This patch adds support for the JZ4740 internal codec.

This looks very good, there are some issues but nothing too major. I
may be repeating some things others have said but hopefully not too
much.

> snd-soc-wm9712-objs := wm9712.o
> snd-soc-wm9713-objs := wm9713.o
> snd-soc-wm-hubs-objs := wm_hubs.o
> +snd-soc-jz4740-codec-objs := jz4740-codec.o

Keep the devices sorted in both Makefile and Kconfig.

> +static int jz4740_codec_set_fmt(struct snd_soc_dai *dai, unsigned int fmt)
> +{
> + switch (fmt & SND_SOC_DAIFMT_MASTER_MASK) {
> + case SND_SOC_DAIFMT_CBM_CFM:
> + break;
> + default:
> + return -EINVAL;
> + }

This does nothing except validate some parameters. Is there actually an
externally visible DAI for this CODEC? If it's just integrated into the
SoC and there's nothing to configure then just omit the DAI
configuration since it's not even useful to document the signal format.

> + .capture = {
> + .stream_name = "Capture",
> + .channels_min = 2,
> + .channels_max = 2,
> + .rates = SNDRV_PCM_RATE_8000_48000,
> + .formats = SNDRV_PCM_FMTBIT_S16_LE | SNDRV_PCM_FMTBIT_S8,

You listed an 18 bit format in hw_params - one or other of this and
hw_params is presumably out of date.

> +static int jz4740_codec_set_bias_level(struct snd_soc_codec *codec,
> + enum snd_soc_bias_level level)
> +{
> +
> + if (codec->bias_level == SND_SOC_BIAS_OFF && level != SND_SOC_BIAS_OFF) {
> + snd_soc_update_bits(codec, JZ4740_REG_CODEC_1,
> + JZ4740_CODEC_1_RESET, JZ4740_CODEC_1_RESET);
> + udelay(2);

I'd expect to see this as part of the _OFF in the main switch
statement.

> + switch (level) {
> + case SND_SOC_BIAS_ON:
> + snd_soc_update_bits(codec, JZ4740_REG_CODEC_1,
> + JZ4740_CODEC_1_VREF_DISABLE |
> + JZ4740_CODEC_1_VREF_AMP_DISABLE |
> + JZ4740_CODEC_1_HEADPHONE_POWER_DOWN_M |
> + JZ4740_CODEC_1_VREF_LOW_CURRENT |
> + JZ4740_CODEC_1_VREF_HIGH_CURRENT,
> + 0);

This looks suspiciously like you should be using DAPM for the headphone
at least, though if there's only headphone out that's possibly not worth
it. Also, are you sure that you want both low and high current VREF
configuring here? I'm not clear what these settings do but the way
they're being managed both here and in _PREPARE seems odd.

> + codec = &jz4740_codec->codec;
> +
> + codec->dev = &pdev->dev;
> + codec->name = "jz-codec";

Seems a bit odd to use the part number in some places and not others.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Lars-Peter Clausen on 3 Jun 2010 20:00
Hi

Mark Brown wrote:
> On Wed, Jun 02, 2010 at 09:12:26PM +0200, Lars-Peter Clausen wrote:
>
>
>> This patch adds support for the JZ4740 internal codec.
>>
>
> This looks very good, there are some issues but nothing too major. I
> may be repeating some things others have said but hopefully not too
> much.
>
>
>> snd-soc-wm9712-objs := wm9712.o
>> snd-soc-wm9713-objs := wm9713.o
>> snd-soc-wm-hubs-objs := wm_hubs.o
>> +snd-soc-jz4740-codec-objs := jz4740-codec.o
>>
>
> Keep the devices sorted in both Makefile and Kconfig.
>
>
>> +static int jz4740_codec_set_fmt(struct snd_soc_dai *dai, unsigned int fmt)
>> +{
>> + switch (fmt & SND_SOC_DAIFMT_MASTER_MASK) {
>> + case SND_SOC_DAIFMT_CBM_CFM:
>> + break;
>> + default:
>> + return -EINVAL;
>> + }
>>
>
> This does nothing except validate some parameters. Is there actually an
> externally visible DAI for this CODEC? If it's just integrated into the
> SoC and there's nothing to configure then just omit the DAI
> configuration since it's not even useful to document the signal format.
>
Nope, there is no externally visible DAI for the codec, but internally
it is connected through the i2s controller of the JZ4740 which supports
different operating modes. And if you'll do
"snd_soc_dai_set_fmt(codec_dai, BOARD_DAIFMT);
snd_soc_dai_set_fmt(cpu_dai, BOARD_DAIFMT);" with a wrong dai format it
will be noticed.
>> + .capture = {
>> + .stream_name = "Capture",
>> + .channels_min = 2,
>> + .channels_max = 2,
>> + .rates = SNDRV_PCM_RATE_8000_48000,
>> + .formats = SNDRV_PCM_FMTBIT_S16_LE | SNDRV_PCM_FMTBIT_S8,
>>
>
> You listed an 18 bit format in hw_params - one or other of this and
> hw_params is presumably out of date.
>
In theory the codec supports 18 bit for playback, but the i2s controller
requires it to be 32 bit aligned, while alsa appears to have only
support for a 24 bit aligned 18 bit format(Correct me if I'm wrong). So
I dropped it, I'll remove the whole format check in hw_params as Liam
suggested.
>> +static int jz4740_codec_set_bias_level(struct snd_soc_codec *codec,
>> + enum snd_soc_bias_level level)
>> +{
>> +
>> + if (codec->bias_level == SND_SOC_BIAS_OFF && level != SND_SOC_BIAS_OFF) {
>> + snd_soc_update_bits(codec, JZ4740_REG_CODEC_1,
>> + JZ4740_CODEC_1_RESET, JZ4740_CODEC_1_RESET);
>> + udelay(2);
>>
>
> I'd expect to see this as part of the _OFF in the main switch
> statement.
>
Uhm, actually this code path is taken when switching from _OFF to
another state. If it is guaranteed that _OFF is always followed by a
certain other state I could put the reset code in its part of the switch
statement.
>
>> + switch (level) {
>> + case SND_SOC_BIAS_ON:
>> + snd_soc_update_bits(codec, JZ4740_REG_CODEC_1,
>> + JZ4740_CODEC_1_VREF_DISABLE |
>> + JZ4740_CODEC_1_VREF_AMP_DISABLE |
>> + JZ4740_CODEC_1_HEADPHONE_POWER_DOWN_M |
>> + JZ4740_CODEC_1_VREF_LOW_CURRENT |
>> + JZ4740_CODEC_1_VREF_HIGH_CURRENT,
>> + 0);
>>
>
> This looks suspiciously like you should be using DAPM for the headphone
> at least, though if there's only headphone out that's possibly not worth
> it. Also, are you sure that you want both low and high current VREF
> configuring here? I'm not clear what these settings do but the way
> they're being managed both here and in _PREPARE seems odd.
>
Hm, I'll take a look.
>
>> + codec = &jz4740_codec->codec;
>> +
>> + codec->dev = &pdev->dev;
>> + codec->name = "jz-codec";
>>
>
> Seems a bit odd to use the part number in some places and not others.
>
I renamed the driver from jz-codec to jz4740-codec shortly before
submitting it after I realized that the codec component on other jz47xx
chips is completely different. Seems like I missed the codec name.

Thanks for reviewing the patch
- Lars

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Mark Brown on 3 Jun 2010 20:00
On 4 Jun 2010, at 00:57, Lars-Peter Clausen wrote:

> Mark Brown wrote:
> Uhm, actually this code path is taken when switching from _OFF to
> another state. If it is guaranteed that _OFF is always followed by a
> certain other state I could put the reset code in its part of the switch
> statement.

The only possible transitions are OFF <-> STANDBY <-> PREPARE <-> ON
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Mark Brown on 20 Jun 2010 09:20
On Sat, Jun 19, 2010 at 04:49:53PM +0200, Lars-Peter Clausen wrote:

This looks good, just one thing:

> +#ifdef CONFIG_PM_SLEEP
> +
> +static int jz4740_codec_suspend(struct device *dev)
> +{
> + struct jz4740_codec *jz4740_codec = dev_get_drvdata(dev);
> + return jz4740_codec_set_bias_level(&jz4740_codec->codec,
> + SND_SOC_BIAS_OFF);
> +}

You've got these set up on the CODEC platform device rather than the
ASoC CODEC. This means that the suspend and resume will happen out of
sequence with the rest of the ASoC suspend and resume which could result
in poor performance or bugs if the device is suspended while the core
still thinks it's active. For example, ASoC will use DAPM to shut down
the CODEC and it's possible that the CODEC could be suspended (and
generate an audible noise) while an external amplifier is still powered,
worsening the problem.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Liam Girdwood on 22 Jun 2010 06:20
On Tue, 2010-06-22 at 00:46 +0200, Lars-Peter Clausen wrote:
> This patch adds support for the JZ4740 internal codec.
>
> Signed-off-by: Lars-Peter Clausen <lars(a)metafoo.de>
> Cc: Mark Brown <broonie(a)opensource.wolfsonmicro.com>
> Cc: Liam Girdwood <lrg(a)slimlogic.co.uk>
> Cc: alsa-devel(a)alsa-project.org
>
> ---
> Changes since v1
> - Put Kconfig entry in alphabetic order
> - Drop codec_set_fmt since the codec supports only one format
> - Rename "Capture Volume" control to "Master Capture Volume"
> - Drop unnecessary format checks
> - Add suspend/resume
> - Cleanup jz4740_codec_set_bias_level
>
> Changes since v2
> - Drop codec prefix from the filename. Note that I keeped it for the object
> name, because otherwise when build as a module it will clash with the ASoC
> JZ4740 platform support.
>
> Changes since v3
> - Hook up suspend/resume callbacks to the codec instead of the platform device

Acked-by: Liam Girdwood <lrg(a)slimlogic.co.uk>
--
Freelance Developer, SlimLogic Ltd
ASoC and Voltage Regulator Maintainer.
http://www.slimlogic.co.uk

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
 |  Next  |  Last
Pages: 1 2
Prev: [tip:perf/urgent] perf: Fix crash in swevents
Next: patch staging-phison-depends-on-ata_bmdma.patch added to gregkh-2.6 tree