Re: [PATCH 1/3] sound/soc/codecs: add LAPIS Semiconductor ML26124

Re: [PATCH 1/3] sound/soc/codecs: add LAPIS Semiconductor ML26124

[Mark Brown]

On Mon, Nov 21, 2011 at 01:08:50PM +0900, Tomoya MORINAGA wrote:

> ML26124-01HB/ML26124-02GD is 16bit monaural audio CODEC which has high
> resistance to voltage noise. On chip regulator realizes power supply
> rejection

So, in general the big overall issue with this code is that it's really
not following the general coding styles and standards for a modern ASoC
driver.  I've pointed out some issues below but there's rather a lot of
them.  In general you should always try to ensure that the code you're
submitting looks like other code for the relevant subsystem.

The big thing that's sticking out at me is the register write sequence
you've got in the middle of the driver - it looks awfully like it's hard
coding a particular use case.

> +struct ml26124_priv {
> +	enum snd_soc_control_type control_type;
> +	spinlock_t lock;
> +	struct snd_pcm_substream *substream;
> +	unsigned int rate;
> +	unsigned int ch;
> +	struct mutex i2c_mutex;

What are these locks for?  Especially the spinlock looks highly
suspicous.

> +static const char *ml26124_lrmcon[] = {"Use L", "Use R", "Use (L+R)", "Use (L+R)/2"};

The "Use" isn't terribly idomatic, remove it.

> +static const struct soc_enum ml26124_enum[] = {
> +/* #define SOC_ENUM_SINGLE(xreg, xshift, xmax, xtexts) */
> +	SOC_ENUM_SINGLE(ML26124_MIXER_VOL_CTL, 0, 4, ml26124_lrmcon),
> +	SOC_ENUM_SINGLE(ML26124_MIXER_VOL_CTL, 4, 15, ml26124_dvfconcon),

Don't use a big table of enums, indexing into it is illegible and error
prone.

> +static const struct snd_kcontrol_new ml26124_snd_controls[] = {
> +	SOC_SINGLE_TLV("Record Digital Volume", ML26124_RECORD_DIG_VOL, 0, 0xff, 1, rec_play_digi_vol),

Capture, not Record.

> +	SOC_SINGLE_TLV("EQ Band1 Gain Setting",  ML26124_EQ_GAIN_BRAND1, 0, 0xff, 1, eq_band_gain),

Volume not Gain Setting.  As a rule anything with TLV information should
be a Volume.

> +static const struct snd_kcontrol_new ml26124_dsp_controls[] = {
> +	SOC_SINGLE("Play Limitter ON/OFF", ML26124_FILTER_EN, 0, 1, 0),

Switch.

> +	SOC_SINGLE("Ditital Volume MUTE", ML26124_FILTER_EN, 4, 1, 0),

Switch.

> +	SOC_SINGLE("Set ALC position", ML26124_FILTER_EN, 5, 1, 0),

What does this actually do?  From the name it *really* doesn't look like
a mixer input.

> +	SND_SOC_DAPM_SPK("Speaker", NULL),
> +	SND_SOC_DAPM_LINE("LINEOUT", NULL),

These should be _OUTPUT() pins - SPK and LINE widgets should only be
used by machine drivers.

> +#define	CODEC_DEV_ADDR		(0x1A)

> +static struct i2c_board_info ioh_hwmon_info[] = {
> +	{I2C_BOARD_INFO("ioh_i2c-0", CODEC_DEV_ADDR + 1)},
> +	{I2C_BOARD_INFO("ioh_i2c-1", CODEC_DEV_ADDR + 2)},
> +	{I2C_BOARD_INFO("ioh_i2c-2", CODEC_DEV_ADDR + 3)},
> +	{I2C_BOARD_INFO("ioh_i2c-3", CODEC_DEV_ADDR + 4)},
> +	{I2C_BOARD_INFO("ioh_i2c-4", CODEC_DEV_ADDR + 5)},
> +	{I2C_BOARD_INFO("ioh_i2c-5", CODEC_DEV_ADDR + 0)},
> +};

Absolutely no.  Your driver should be a standard I2C driver, device
registration should be done by the board not the chip driver.

> +static const struct snd_soc_dapm_route intercon[] = {
> +};

I can't see how any of the DAPM widgets you specify will ever work with
no interconnections.

> +
> +static int snd_card_codec_reg_read(struct ml26124_priv *priv,
> +				   unsigned char reg, unsigned char *val)
> +{
> +	unsigned char data;
> +	struct i2c_client *i2c;

Use the standard register access code, don't open code things in your
driver unless there's a good reason to.  Current best practice for most
I2C or SPI devices is to use regmap, see any recently added driver for
examples.

> +static int snd_card_codec_set(int number, struct ml26124_priv *priv)
> +{

Namespacing - this isn't a generic ALSA or ASoC function.

> +	unsigned char data;
> +	unsigned int rate = priv->rate;
> +	unsigned int channels = priv->ch;
> +
> +	snd_card_codec_reg_read(priv, 0x30, &data);	 /* Read MICVIAS Voltage */
> +
> +	snd_card_codec_reg_write(priv, 0x10, 0x01);	/* soft reset assert */
> +	snd_card_codec_reg_write(priv, 0x10, 0x00);	/* soft reset negate */
> +
> +	snd_card_codec_reg_write(priv, 0x0c, 0x00);	/* Stop clock  */ 

Use snd_soc_update_bits() and the other standard register access
functions.

> +	switch (rate) {
> +	case 16000:
> +		snd_card_codec_reg_write(priv, 0x00, 0x03);
> +		snd_card_codec_reg_write(priv, 0x02, 0x0c);
> +		snd_card_codec_reg_write(priv, 0x04, 0x00);
> +		snd_card_codec_reg_write(priv, 0x06, 0x20);
> +		snd_card_codec_reg_write(priv, 0x08, 0x00);
> +		snd_card_codec_reg_write(priv, 0x0a, 0x04);
> +		break;

This sort of magic number based code without even registers being named
isn't really of the standard required for maintainability or review.

> +	default:
> +		pr_err("%s:this rate is no support for ml26124\n", __func__);
> +		break;
> +	}

If the driver has detected an error it should return an error; printing
the rate that was requested as part of the error message would also be
useful.

> +	snd_card_codec_reg_write(priv, 0x64, 0x00); /* master/slave set slave */

This looks like it should be in set_dai_fmt().

> +
> +	snd_card_codec_reg_write(priv, 0x20, 0x02); /* VMID on. normal mode */
> +	msleep(50);

This and much of the code looks like it should be in set_bias_level().

> +	snd_card_codec_reg_write(priv, 0x26, 0x1f); /* Speaker Amplified Poer Management */
> +	snd_card_codec_reg_write(priv, 0x28, 0x02); /* LOUT Control Regsister */
> +
> +	snd_card_codec_reg_write(priv, 0x54, 0x02); /* Speaker Amplifier output Control */
> +	snd_card_codec_reg_write(priv, 0x5a, 0x00); /* Mic Interface Control Register */

This looks awfully like it's supposed to be DAPM.

> +static int ml26124_hw_params(struct snd_pcm_substream *substream,
> +			    struct snd_pcm_hw_params *hw_params,
> +			    struct snd_soc_dai *dai)
> +{
> +	struct snd_soc_codec *codec = dai->codec;
> +	struct ml26124_priv *priv = snd_soc_codec_get_drvdata(codec);
> +
> +	if (snd_card_codec_set(substream->number, priv))
> +		return -1;

Return real error codes if you're going to detect errors.

> +
> +	return snd_pcm_lib_malloc_pages(substream,
> +					params_buffer_bytes(hw_params));

Why is a CODEC driver allocing memory to buffer audio?

> +static int ml26124_pcm_prepare(struct snd_pcm_substream *substream,
> +			      struct snd_soc_dai *dai)
> +{
> +	return 0;
> +}

Remove empty functions.

> +static int ml26124_mute(struct snd_soc_dai *dai, int mute)
> +{
> +	struct snd_soc_codec *codec = dai->codec;
> +	struct ml26124_priv *priv = snd_soc_codec_get_drvdata(codec);
> +
> +	if (mute)
> +		snd_card_codec_reg_write(priv, ML26124_DVOL_CTL,
> +					 DVOL_CTL_DVMUTE_ON);
> +	else
> +		snd_card_codec_reg_write(priv, ML26124_DVOL_CTL,
> +					 DVOL_CTL_DVMUTE_OFF);

I suspect you mean to use snd_soc_update_bits() here.

> +static int ml26124_set_bias_level(struct snd_soc_codec *codec,
> +		enum snd_soc_bias_level level)
> +{
> +	struct ml26124_priv *priv = snd_soc_codec_get_drvdata(codec);
> +
> +	switch (level) {
> +	case SND_SOC_BIAS_ON:
> +	case SND_SOC_BIAS_PREPARE:
> +	case SND_SOC_BIAS_STANDBY:
> +		snd_card_codec_reg_write(priv, ML26124_REF_PM, REF_PM_VMID_ON);
> +		msleep(50);
> +		snd_card_codec_reg_write(priv, ML26124_REF_PM,
> +					 REF_PM_VMID_ON | REF_PM_MICBEN);

This looks much more like what I'd expect in terms of coding style -
meaingful defines and use of set_bias_level().  Also note that during
audio startup you'll transition STANDBY->PREPARE->ON and the reverse on
shutdown, I really don't think you want to be doing this for anything
except STANDBY.

I also can't see how this will play nicely with the above register write
sequence - you probably want to just remove the register write sequence.

> +static int ml26124_probe(struct snd_soc_codec *codec)
> +{
> +	snd_soc_add_controls(codec, ml26124_snd_controls,
> +			     ARRAY_SIZE(ml26124_snd_controls));
> +
> +	snd_soc_dapm_new_controls(codec, ml26124_dapm_widgets,
> +				  ARRAY_SIZE(ml26124_dapm_widgets));

Initialise these from your codec driver structure.

> +#if defined(CONFIG_I2C) || defined(CONFIG_I2C_MODULE)
> +static __devinit int ml26124_i2c_probe(struct i2c_client *i2c,
> +				      const struct i2c_device_id *id)

Since there's no visible SPI support just unconditionally use I2C.

> +static struct i2c_driver ml26124_i2c_driver = {
> +	.driver = {
> +		.name = "ml26124-codec",

Remove the -codec, it's redundant.

Re: [PATCH 1/3] sound/soc/codecs: add LAPIS Semiconductor ML26124

[Tomoya MORINAGA]

Hi Mark,

Thank you for your comments.
I have some questions.

2011/11/21 Mark Brown <broonie@opensource.wolfsonmicro.com> wrote:
>> +static const struct snd_kcontrol_new ml26124_snd_controls[] = {
>> +     SOC_SINGLE_TLV("Record Digital Volume", ML26124_RECORD_DIG_VOL, 0, 0xff, 1, rec_play_digi_vol),
>
> Capture, not Record.

OK,
BTW, What's TLV ? Let me know the full spell of this "TLV".

>
>> +static const struct snd_kcontrol_new ml26124_dsp_controls[] = {
>> +     SOC_SINGLE("Play Limitter ON/OFF", ML26124_FILTER_EN, 0, 1, 0),
>
Do you mean SOC_SINGLE("Play Limitter Switch", ML26124_FILTER_EN, 0, 1, 0) ?


>> +     SOC_SINGLE("Set ALC position", ML26124_FILTER_EN, 5, 1, 0),
>
> What does this actually do?  From the name it *really* doesn't look like
> a mixer input.
>
The above means where connects the ALC.
So, this doesn't relate to mixer input.


>> +static const struct snd_soc_dapm_route intercon[] = {
>> +};
>
> I can't see how any of the DAPM widgets you specify will ever work with
> no interconnections.
I see.
However I couldn't understand the meaning of the widgets. So I didn't
write anything.
For example, wm8731 writes like below.
Could you explain this ?

static const struct snd_soc_dapm_route wm8731_intercon[] = {
	{"DAC", NULL, "OSC", wm8731_check_osc},
	{"ADC", NULL, "OSC", wm8731_check_osc},
	{"DAC", NULL, "ACTIVE"},
	{"ADC", NULL, "ACTIVE"},

	/* output mixer */
	{"Output Mixer", "Line Bypass Switch", "Line Input"},
	{"Output Mixer", "HiFi Playback Switch", "DAC"},
	{"Output Mixer", "Mic Sidetone Switch", "Mic Bias"},

	/* outputs */
	{"RHPOUT", NULL, "Output Mixer"},
	{"ROUT", NULL, "Output Mixer"},
	{"LHPOUT", NULL, "Output Mixer"},
	{"LOUT", NULL, "Output Mixer"},

	/* input mux */
	{"Input Mux", "Line In", "Line Input"},
	{"Input Mux", "Mic", "Mic Bias"},
	{"ADC", NULL, "Input Mux"},

	/* inputs */
	{"Line Input", NULL, "LLINEIN"},
	{"Line Input", NULL, "RLINEIN"},
	{"Mic Bias", NULL, "MICIN"},
};



>
>> +
>> +static int snd_card_codec_reg_read(struct ml26124_priv *priv,
>> +                                unsigned char reg, unsigned char *val)
>> +{
>> +     unsigned char data;
>> +     struct i2c_client *i2c;
>
> Use the standard register access code, don't open code things in your
> driver unless there's a good reason to.  Current best practice for most
> I2C or SPI devices is to use regmap, see any recently added driver for
> examples.
What's "regmap" mean ? or do you mean drivers/mfd/* ?
Could you tell me this ?

>> +     unsigned char data;
>> +     unsigned int rate = priv->rate;
>> +     unsigned int channels = priv->ch;
>> +
>> +     snd_card_codec_reg_read(priv, 0x30, &data);      /* Read MICVIAS Voltage */
>> +
>> +     snd_card_codec_reg_write(priv, 0x10, 0x01);     /* soft reset assert */
>> +     snd_card_codec_reg_write(priv, 0x10, 0x00);     /* soft reset negate */
>> +
>> +     snd_card_codec_reg_write(priv, 0x0c, 0x00);     /* Stop clock  */
>
> Use snd_soc_update_bits() and the other standard register access
> functions.
Using snd_soc_update_bits(), need to register ".read" method.
Is the same as the above  snd_card_codec_reg_read ?

Thanks in advance,
---
tomoya
ROHM Co., Ltd

Re: [PATCH 1/3] sound/soc/codecs: add LAPIS Semiconductor ML26124

[Mark Brown]

On Tue, Nov 22, 2011 at 07:47:10PM +0900, Tomoya MORINAGA wrote:
> 2011/11/21 Mark Brown <broonie@opensource.wolfsonmicro.com> wrote:

> BTW, What's TLV ? Let me know the full spell of this "TLV".

Tag/Length/Value.

> >> +static const struct snd_kcontrol_new ml26124_dsp_controls[] = {
> >> +     SOC_SINGLE("Play Limitter ON/OFF", ML26124_FILTER_EN, 0, 1, 0),

> Do you mean SOC_SINGLE("Play Limitter Switch", ML26124_FILTER_EN, 0, 1, 0) ?

Yes.

> >> +     SOC_SINGLE("Set ALC position", ML26124_FILTER_EN, 5, 1, 0),

> > What does this actually do?  From the name it *really* doesn't look like
> > a mixer input.

> The above means where connects the ALC.
> So, this doesn't relate to mixer input.

If this control is not a mixer input it should not be a mixer input in
the driver.  Though with your explanation I still don't understand what
it does so presumably the naming also needs to be improved.

> However I couldn't understand the meaning of the widgets. So I didn't
> write anything.

See Documentation/sound/alsa/soc/dapm.txt.

> >> +static int snd_card_codec_reg_read(struct ml26124_priv *priv,
> >> +                                unsigned char reg, unsigned char *val)
> >> +{
> >> +     unsigned char data;
> >> +     struct i2c_client *i2c;

> > Use the standard register access code, don't open code things in your
> > driver unless there's a good reason to.  Current best practice for most
> > I2C or SPI devices is to use regmap, see any recently added driver for
> > examples.

> What's "regmap" mean ? or do you mean drivers/mfd/* ?
> Could you tell me this ?

drivers/base/regmap and also grep in sound/soc/codecs/*.c

> > Use snd_soc_update_bits() and the other standard register access
> > functions.

> Using snd_soc_update_bits(), need to register ".read" method.

Or use a cache.

> Is the same as the above  snd_card_codec_reg_read ?

I don't understand this question.

Re: [PATCH 1/3] sound/soc/codecs: add LAPIS Semiconductor ML26124

[Tomoya MORINAGA]

Hi Mark,

>> > Use the standard register access code, don't open code things in your
>> > driver unless there's a good reason to.  Current best practice for most
>> > I2C or SPI devices is to use regmap, see any recently added driver for
>> > examples.
>
>> What's "regmap" mean ? or do you mean drivers/mfd/* ?
>> Could you tell me this ?
>
> drivers/base/regmap and also grep in sound/soc/codecs/*.c
Though grepping "regmap" at sound/soc/codecs/*.c, I couldn't find any file.
Is this true ?

Thanks,
tomoya

2011/11/22 Mark Brown <broonie@opensource.wolfsonmicro.com>:
> On Tue, Nov 22, 2011 at 07:47:10PM +0900, Tomoya MORINAGA wrote:
>> 2011/11/21 Mark Brown <broonie@opensource.wolfsonmicro.com> wrote:
>
>> BTW, What's TLV ? Let me know the full spell of this "TLV".
>
> Tag/Length/Value.
>
>> >> +static const struct snd_kcontrol_new ml26124_dsp_controls[] = {
>> >> +     SOC_SINGLE("Play Limitter ON/OFF", ML26124_FILTER_EN, 0, 1, 0),
>
>> Do you mean SOC_SINGLE("Play Limitter Switch", ML26124_FILTER_EN, 0, 1, 0) ?
>
> Yes.
>
>> >> +     SOC_SINGLE("Set ALC position", ML26124_FILTER_EN, 5, 1, 0),
>
>> > What does this actually do?  From the name it *really* doesn't look like
>> > a mixer input.
>
>> The above means where connects the ALC.
>> So, this doesn't relate to mixer input.
>
> If this control is not a mixer input it should not be a mixer input in
> the driver.  Though with your explanation I still don't understand what
> it does so presumably the naming also needs to be improved.
>
>> However I couldn't understand the meaning of the widgets. So I didn't
>> write anything.
>
> See Documentation/sound/alsa/soc/dapm.txt.
>
>> >> +static int snd_card_codec_reg_read(struct ml26124_priv *priv,
>> >> +                                unsigned char reg, unsigned char *val)
>> >> +{
>> >> +     unsigned char data;
>> >> +     struct i2c_client *i2c;
>
>> > Use the standard register access code, don't open code things in your
>> > driver unless there's a good reason to.  Current best practice for most
>> > I2C or SPI devices is to use regmap, see any recently added driver for
>> > examples.
>
>> What's "regmap" mean ? or do you mean drivers/mfd/* ?
>> Could you tell me this ?
>
> drivers/base/regmap and also grep in sound/soc/codecs/*.c
>
>> > Use snd_soc_update_bits() and the other standard register access
>> > functions.
>
>> Using snd_soc_update_bits(), need to register ".read" method.
>
> Or use a cache.
>
>> Is the same as the above  snd_card_codec_reg_read ?
>
> I don't understand this question.
>

Re: [PATCH 1/3] sound/soc/codecs: add LAPIS Semiconductor ML26124

[Mark Brown]

On Fri, Nov 25, 2011 at 11:03:17AM +0900, Tomoya MORINAGA wrote:

> > drivers/base/regmap and also grep in sound/soc/codecs/*.c

> Though grepping "regmap" at sound/soc/codecs/*.c, I couldn't find any file.
> Is this true ?

You should always submit new code against the current development
version of the subsystem you're submitting against.

Re: [PATCH 2/3] sound/soc/lapis: add machine driver

[Mark Brown]

On Mon, Nov 21, 2011 at 01:08:51PM +0900, Tomoya MORINAGA wrote:
> Add machine driver for LAPIS Semiconductor ML7213 IOH.

This needs to come after you add the base platform support in the series
- clearly this won't work without the CPU side drivers it depends on.

As with the CODEC driver you really should be looking at the standards
for modern drivers and following them.

> @@ -60,6 +60,7 @@ source "sound/soc/s6000/Kconfig"
>  source "sound/soc/sh/Kconfig"
>  source "sound/soc/tegra/Kconfig"
>  source "sound/soc/txx9/Kconfig"
> +source "sound/soc/lapis/Kconfig"

Keep the Makefile and Kconfig entries sorted.

> +static struct snd_soc_dai_link ioh_i2s_dai = {
> +	.name = "I2S",
> +	.stream_name = "I2S HiFi",
> +	.cpu_dai_name = "ml7213ioh-i2s",
> +	.codec_dai_name = "ml7213ioh-hifi",

I'm very surprised this works...  If nothing else I'd expec the CODEC
DAI to have a different name to the CPU.

> +static int __init ioh_i2s_probe(struct platform_device *pdev)
> +{
> +	int ret;
> +
> +	soc_pdev = platform_device_alloc("soc-audio", -1);
> +	if (!soc_pdev)
> +		return -ENOMEM;

You should register the card as a standard platform device for new
drivers and use snd_soc_register_card().

> +static struct platform_driver ioh_i2s_driver = {
> +	.remove = ioh_i2s_remove,
> +	.driver = {
> +		.name = "ml7213ioh-machine",
> +		.owner = THIS_MODULE,
> +	},
> +};

You're already doing the platform device bit so it should be be a case
of changing the body of probe() and remove().

> +static int __init ioh_i2s_init(void)
> +{
> +	return platform_driver_probe(&ioh_i2s_driver,
> +				     ioh_i2s_probe);
> +}

Just make this a standard platform_driver_register().

Re: [PATCH 2/3] sound/soc/lapis: add machine driver

[Tomoya MORINAGA]

2011/11/21 Mark Brown <broonie@opensource.wolfsonmicro.com>:
> On Mon, Nov 21, 2011 at 01:08:51PM +0900, Tomoya MORINAGA wrote:
>> Add machine driver for LAPIS Semiconductor ML7213 IOH.
>
> This needs to come after you add the base platform support in the series
> - clearly this won't work without the CPU side drivers it depends on.

Most of the chipset drivers for Intel Atom E6xx series have already
accepted by upstream.
e.g. i2c-eg20t, pch_dma, pch_uart, spi-topcliff-pch, ml-ioh-gpio, etc...

BTW, every time I send mail to "alsa-devel@alsa-project.org",
I receive returned mail "Your message to Alsa-devel awaits moderator approval".
Is this normal things ?
Of course, I've already subscribed to "alsa-devel@alsa-project.org".

thanks,
tomoya

Re: [PATCH 2/3] sound/soc/lapis: add machine driver

[Takashi Iwai]

At Fri, 2 Dec 2011 14:33:59 +0900,
Tomoya MORINAGA wrote:
> BTW, every time I send mail to "alsa-devel@alsa-project.org",
> I receive returned mail "Your message to Alsa-devel awaits moderator approval".
> Is this normal things ?
> Of course, I've already subscribed to "alsa-devel@alsa-project.org".

Did you subscribe with your gmail address, too?


Takashi

Re: [PATCH 2/3] sound/soc/lapis: add machine driver

[Tomoya MORINAGA]

2011/12/2 Takashi Iwai <tiwai@suse.de>:
> At Fri, 2 Dec 2011 14:33:59 +0900,
> Tomoya MORINAGA wrote:
>> BTW, every time I send mail to "alsa-devel@alsa-project.org",
>> I receive returned mail "Your message to Alsa-devel awaits moderator approval".
>> Is this normal things ?
>> Of course, I've already subscribed to "alsa-devel@alsa-project.org".
>
> Did you subscribe with your gmail address, too?
Yes, I subscribed this gmail address.

thanks,
tomoya

Re: [PATCH 2/3] sound/soc/lapis: add machine driver

[Takashi Iwai]

At Fri, 2 Dec 2011 15:18:06 +0900,
Tomoya MORINAGA wrote:
> 
> 2011/12/2 Takashi Iwai <tiwai@suse.de>:
> > At Fri, 2 Dec 2011 14:33:59 +0900,
> > Tomoya MORINAGA wrote:
> >> BTW, every time I send mail to "alsa-devel@alsa-project.org",
> >> I receive returned mail "Your message to Alsa-devel awaits moderator approval".
> >> Is this normal things ?
> >> Of course, I've already subscribed to "alsa-devel@alsa-project.org".
> >
> > Did you subscribe with your gmail address, too?
> Yes, I subscribed this gmail address.

The mail-server doesn't think so.
Maybe with a different address like mail.google.com or such?


Takashi

Re: [PATCH 2/3] sound/soc/lapis: add machine driver

[Tomoya MORINAGA]

2011/12/2 Takashi Iwai <tiwai@suse.de>:
> At Fri, 2 Dec 2011 15:18:06 +0900,
> Tomoya MORINAGA wrote:
>>
>> 2011/12/2 Takashi Iwai <tiwai@suse.de>:
>> > At Fri, 2 Dec 2011 14:33:59 +0900,
>> > Tomoya MORINAGA wrote:
>> >> BTW, every time I send mail to "alsa-devel@alsa-project.org",
>> >> I receive returned mail "Your message to Alsa-devel awaits moderator approval".
>> >> Is this normal things ?
>> >> Of course, I've already subscribed to "alsa-devel@alsa-project.org".
>> >
>> > Did you subscribe with your gmail address, too?
>> Yes, I subscribed this gmail address.
>
> The mail-server doesn't think so.
> Maybe with a different address like mail.google.com or such?
>
For confirmation, I registered my e-mail account again yesterday.
As a result, I got the following mail (Already registered).

From: Majordomo@vger.kernel.org
12月1日 (1日前)

>>>> auth bd2de885 subscribe alsa-devel tomoya.rohm@gmail.com
**** Address already subscribed to alsa-devel

So, I think registration for mailing list is success with this gmail address.
What should I do for next?

thanks,
tomoya

Re: [PATCH 2/3] sound/soc/lapis: add machine driver

[Takashi Iwai]

At Fri, 2 Dec 2011 15:52:28 +0900,
Tomoya MORINAGA wrote:
> 
> 2011/12/2 Takashi Iwai <tiwai@suse.de>:
> > At Fri, 2 Dec 2011 15:18:06 +0900,
> > Tomoya MORINAGA wrote:
> >>
> >> 2011/12/2 Takashi Iwai <tiwai@suse.de>:
> >> > At Fri, 2 Dec 2011 14:33:59 +0900,
> >> > Tomoya MORINAGA wrote:
> >> >> BTW, every time I send mail to "alsa-devel@alsa-project.org",
> >> >> I receive returned mail "Your message to Alsa-devel awaits moderator approval".
> >> >> Is this normal things ?
> >> >> Of course, I've already subscribed to "alsa-devel@alsa-project.org".
> >> >
> >> > Did you subscribe with your gmail address, too?
> >> Yes, I subscribed this gmail address.
> >
> > The mail-server doesn't think so.
> > Maybe with a different address like mail.google.com or such?
> >
> For confirmation, I registered my e-mail account again yesterday.
> As a result, I got the following mail (Already registered).
> 
> From: Majordomo@vger.kernel.org
> 12月1日 (1日前)

Why vger?


Takashi
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: [PATCH 2/3] sound/soc/lapis: add machine driver

[Tomoya MORINAGA]

2011/12/2 Takashi Iwai <tiwai@suse.de>:>> For confirmation, I
registered my e-mail account again yesterday.
>> As a result, I got the following mail (Already registered).>>>> From: Majordomo@vger.kernel.org>> 12月1日 (1日前)>> Why vger?>
I referred http://vger.kernel.org/vger-lists.html#alsa-devel.
This looks not TRUE.

I must subscribe from below ?
https://lists.sourceforge.net/lists/listinfo/alsa-announce

thanks
tomoya

Re: [PATCH 2/3] sound/soc/lapis: add machine driver

[Takashi Iwai]

At Fri, 2 Dec 2011 16:13:04 +0900,
Tomoya MORINAGA wrote:
> 
> 2011/12/2 Takashi Iwai <tiwai@suse.de>:>> For confirmation, I
> registered my e-mail account again yesterday.
> >> As a result, I got the following mail (Already registered).>>>> From: Majordomo@vger.kernel.org>> 12月1日 (1日前)>> Why vger?>
> I referred http://vger.kernel.org/vger-lists.html#alsa-devel.
> This looks not TRUE.

alsa-devel isn't hosted on vger.
There is one list, but it's just passed to alsa-project.org host.

> I must subscribe from below ?
> https://lists.sourceforge.net/lists/listinfo/alsa-announce

No, it's a different list.  Go through the navigation on
www.alsa-project.org.


Takashi
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: [PATCH 2/3] sound/soc/lapis: add machine driver

[Mark Brown]

On Fri, Dec 02, 2011 at 02:33:59PM +0900, Tomoya MORINAGA wrote:
> 2011/11/21 Mark Brown <broonie@opensource.wolfsonmicro.com>:

> > This needs to come after you add the base platform support in the series
> > - clearly this won't work without the CPU side drivers it depends on.

> Most of the chipset drivers for Intel Atom E6xx series have already
> accepted by upstream.
> e.g. i2c-eg20t, pch_dma, pch_uart, spi-topcliff-pch, ml-ioh-gpio, etc...

The CPU side audio drivers are not there - there seemed to be some quite
serious review issues with those.

Re: [PATCH 2/3] sound/soc/lapis: add machine driver

[Tomoya MORINAGA]

2011/12/2 Mark Brown <broonie@opensource.wolfsonmicro.com>:
> On Fri, Dec 02, 2011 at 02:33:59PM +0900, Tomoya MORINAGA wrote:
>> 2011/11/21 Mark Brown <broonie@opensource.wolfsonmicro.com>:
>
>> > This needs to come after you add the base platform support in the series
>> > - clearly this won't work without the CPU side drivers it depends on.
>
>> Most of the chipset drivers for Intel Atom E6xx series have already
>> accepted by upstream.
>> e.g. i2c-eg20t, pch_dma, pch_uart, spi-topcliff-pch, ml-ioh-gpio, etc...
>
> The CPU side audio drivers are not there - there seemed to be some quite
> serious review issues with those.
>
Let me clarify your saying.
Does the above "audio drivers" mean i2s (of south bridge) driver or HD
audio (of north bridge) or another?

Thanks,
tomoya

Re: [PATCH 2/3] sound/soc/lapis: add machine driver

[Mark Brown]

On Fri, Dec 02, 2011 at 09:09:34PM +0900, Tomoya MORINAGA wrote:
> 2011/12/2 Mark Brown <broonie@opensource.wolfsonmicro.com>:

> > The CPU side audio drivers are not there - there seemed to be some quite
> > serious review issues with those.

> Let me clarify your saying.
> Does the above "audio drivers" mean i2s (of south bridge) driver or HD
> audio (of north bridge) or another?

I2S and audio DMA.

Re: [PATCH 2/3] sound/soc/lapis: add machine driver

[Tomoya MORINAGA]

2011/12/2 Mark Brown <broonie@opensource.wolfsonmicro.com>:
> On Fri, Dec 02, 2011 at 09:09:34PM +0900, Tomoya MORINAGA wrote:
>> 2011/12/2 Mark Brown <broonie@opensource.wolfsonmicro.com>:
>
>> > The CPU side audio drivers are not there - there seemed to be some quite
>> > serious review issues with those.
>
>> Let me clarify your saying.
>> Does the above "audio drivers" mean i2s (of south bridge) driver or HD
>> audio (of north bridge) or another?
>
> I2S and audio DMA.

OK.
As you know, I post i2s/dma driver once. I understand need to do much
modification.
I'm going to develop codec driver, machine driver and
platform(i2s/dma) driver in turn obeying modern ASoC framework.

Do you mean you cannot review this codec driver any more unless I
provide other 2 drivers (machine and platform) ?

Thanks,
tomoya

Re: [PATCH 2/3] sound/soc/lapis: add machine driver

[Mark Brown]

On Fri, Dec 02, 2011 at 09:35:57PM +0900, Tomoya MORINAGA wrote:

> Do you mean you cannot review this codec driver any more unless I
> provide other 2 drivers (machine and platform) ?

No.  This is the review of the machine driver.  The machine driver
depends on both the CODEC and the CPU drivers.

Re: [PATCH 3/3] sound/soc/lapis: add platform driver

[Mark Brown]

On Mon, Nov 21, 2011 at 01:08:52PM +0900, Tomoya MORINAGA wrote:

> +/* =================== I2S CH0 config =================== */
> +#define	I2S_CH0_MCLK		(12288000) /* Master Clock Frequency[Hz] */

This looks like it should be board specific?

> +  #if IOH_I2S_USE_PARAM
> +	#define	I2S_CH0_FS_16000	16000
> +	#define	I2S_CH0_FS_32000	32000
> +	#define	I2S_CH0_FS_48000	48000
> +  #else
> +	#define	I2S_CH0_FS_8000		8000
> +	#define	I2S_CH0_FS_11025	11025
> +	#define	I2S_CH0_FS_22050	22050
> +	#define	I2S_CH0_FS_44100	44100
> +  #endif

What are these ifdefs for?  This should be configured at runtime.

> +/* select master or slave. The value is ioh_mssel_t */
> +#define	I2S_CH0_MSSEL			(ioh_mssel_master)
> +/* select MCLK or MLBCLK into Master Clock. The value is enum ioh_masterclk_t */
> +#define	I2S_CH0_MASTERCLKSEL		(ioh_masterclksel_mclk)

This and most of the following defines also look like compile time
configuration for things that should be runtime configured.  As with
the previous patches in the series the big picture issue here is that
you need to update your code to reflect the practices of modern
mainline Linux code - here the major issue is that you appear to be
doing all your configuration with #defines rather than by using runtime
configuration from the machine driver as is normal for ASoC code.

I got through a bunch more defines but stopped reviewing at some point
as there was so much of this issue.

Re: [PATCH 3/3] sound/soc/lapis: add platform driver

[Tomoya MORINAGA]

Searching git log,
tegra_i2s.c seems modern.
I have a question.

According to platfom.txt, describes like below.
struct snd_soc_ops {
...
};

On the other hand, tegra_i2s.c describes like below.

static const struct snd_soc_dai_ops tegra_i2s_dai_ops = {
...
};

Which is true as modern driver ? or case by case ?


>> +#define      I2S_CH0_MCLK            (12288000) /* Master Clock Frequency[Hz] */
> This looks like it should be board specific?
Should our platform driver use "clk_get()" ?
If no, how should our driver get the value ?

thanks in advance.

tomoya

Re: [PATCH 3/3] sound/soc/lapis: add platform driver

[Mark Brown]

On Mon, Dec 12, 2011 at 05:28:13PM +0900, Tomoya MORINAGA wrote:

> struct snd_soc_ops {
> ...
> };

> On the other hand, tegra_i2s.c describes like below.

> static const struct snd_soc_dai_ops tegra_i2s_dai_ops = {
> ...
> };

> Which is true as modern driver ? or case by case ?

The latter.  With things like this it would be *really* helpful if you
could take a step back and think about what the differences mean and why
they are different.

> 
> >> +#define      I2S_CH0_MCLK            (12288000) /* Master Clock Frequency[Hz] */
> > This looks like it should be board specific?
> Should our platform driver use "clk_get()" ?
> If no, how should our driver get the value ?

Again, with things like this it would be really helpful if you could
attempt to answer questions for yourself.  Have you looked at how other
platforms configure clock rates from machine drivers?

Re: [PATCH 3/3] sound/soc/lapis: add platform driver

[Tomoya MORINAGA]

2011/12/12 Mark Brown <broonie@opensource.wolfsonmicro.com>:
> On Mon, Dec 12, 2011 at 05:28:13PM +0900, Tomoya MORINAGA wrote:
>
>> struct snd_soc_ops {
>> ...
>> };
>
>> On the other hand, tegra_i2s.c describes like below.
>
>> static const struct snd_soc_dai_ops tegra_i2s_dai_ops = {
>> ...
>> };
>
>> Which is true as modern driver ? or case by case ?
>
> The latter.  With things like this it would be *really* helpful if you
> could take a step back and think about what the differences mean and why
> they are different.

Our driver needs ".pointer" method.
However, "struct snd_soc_dai_ops" doesn't have the method.
So, I think "struct snd_soc_dai_ops" cannot be applied to our driver.
Searching other drivers, "blackfin/bf5xx-i2s-pcm.c" uses "struct
snd_soc_ops" not "struct snd_soc_dai_ops".
Let me know your opinion.

Thanks,

tomoya

Re: [PATCH 3/3] sound/soc/lapis: add platform driver

[Mark Brown]

On Tue, Dec 13, 2011 at 01:38:25PM +0900, Tomoya MORINAGA wrote:
> 2011/12/12 Mark Brown <broonie@opensource.wolfsonmicro.com>:

> > The latter.  With things like this it would be *really* helpful if you
> > could take a step back and think about what the differences mean and why
> > they are different.

> Our driver needs ".pointer" method.
> However, "struct snd_soc_dai_ops" doesn't have the method.
> So, I think "struct snd_soc_dai_ops" cannot be applied to our driver.

What makes you claim this - *why* does your DAI driver need a pointer
method?

> Searching other drivers, "blackfin/bf5xx-i2s-pcm.c" uses "struct
> snd_soc_ops" not "struct snd_soc_dai_ops".
> Let me know your opinion.

Of course the DMA driver uses the ops for DMA drivers!  Alternatively if
your question above is about your DMA driver then why is your DMA driver
using DAI ops?

To repeat what I said above in a different way *please* put more effort
into understanding things and trying to solve problems for yourself, or
improving the way you ask questions.