sound/soc/codecs: add LAPIS Semiconductor ML26124

This driver is for LAPIS Semiconductor ML7213 IOH I2S.

Quoted 1 line

This driver is for LAPIS Semiconductor ML7213 IOH I2S.

I just merged Lars-Peter's dmaengine library code which has been on the list for a week or so - this should be updated to use that next time it's posted. That should save a lot of code from the driver and make sure it's following best practices for dmaengine use.

sound/soc/lapis/ml7213ioh-plat.c line 37

Quoted 2 lines

sound/soc/lapis/ml7213ioh-plat.c new file +2 -0

+static struct ioh_i2s_data *i2s_data;
+static struct ioh_i2s_dma dmadata[MAX_I2S_CH];

Why are these needed, aren't they dynamically allocated by the driver?

sound/soc/lapis/ml7213ioh-plat.c line 786

Quoted 3 lines

sound/soc/lapis/ml7213ioh-plat.c new file +3 -0

+     case SNDRV_PCM_STREAM_CAPTURE:
+             offset =\
+                 ioh_rtd->dma->rx_cur_period * ioh_rtd->dma->period_bytes;

Drop the continuations, line breaks are just whitespace in C outside of macros and strings.

sound/soc/lapis/ml7213ioh-plat.c line 894

Quoted 4 lines

sound/soc/lapis/ml7213ioh-plat.c new file +3 -0

+     case SNDRV_PCM_FORMAT_S32_LE:
+             byte = 24;
+             break;

That looks wrong... are you sure you don't support S24_LE or something?

sound/soc/lapis/ml7213ioh-plat.c line 1009

Quoted 6 lines

sound/soc/lapis/ml7213ioh-plat.c new file +6 -0

+             switch (fmt & SND_SOC_DAIFMT_INV_MASK) {
+             case SND_SOC_DAIFMT_NB_NF:
+                     cmn_reg[i] &= ~ML7213I2S_BCLKPOL;
+                     break;
+             default:
+                     return -EINVAL;

Looking at that code I suspect at least IB_NF is supported too...

sound/soc/lapis/ml7213ioh-plat.c line 1049

Quoted 6 lines

sound/soc/lapis/ml7213ioh-plat.c new file +5 -0

+static int ml7213i2s_dai_set_clkdiv(struct snd_soc_dai *dai,
+                               int div_id, int div)
+{
 
+     switch (div_id) {
+     case ML7213IOH_BCLKFS0:

This all looks like BCLK/sample calculations that the driver should really be able to figure out for itself rather than forcing every user to replicate the code to do the calculation, calculation in hw_params would be more normal.

sound/soc/lapis/ml7213ioh-plat.c line 1179

Quoted 3 lines

sound/soc/lapis/ml7213ioh-plat.c new file +2 -0

+#ifdef CONFIG_PM
+static void ioh_i2s_save_reg_conf(void)

This stuff has exactly one caller, just inline it. It's also *very* suspicious that it's not taking an argument specifying the device...

sound/soc/lapis/ml7213ioh-plat.c line 1269

Quoted 4 lines

sound/soc/lapis/ml7213ioh-plat.c new file +3 -0

+#else
+#define ml7213i2s_soc_suspend NULL
+#define spdif_soc_resume NULL

Hrm?

sound/soc/lapis/ml7213ioh-plat.c line 1300

Quoted 5 lines

sound/soc/lapis/ml7213ioh-plat.c new file +4 -0

+#ifdef CONFIG_PM
+     .suspend = ml7213i2s_soc_suspend,
+     .resume = ml7213i2s_soc_resume,
+#endif

The whole point with defining the functions to NULL above is to avoid the ifdef here.

sound/soc/lapis/ml7213ioh-plat.c line 1351

Quoted 7 lines

sound/soc/lapis/ml7213ioh-plat.c new file +4 -0

+static struct platform_driver ioh_i2s_driver_plat = {
 
+static struct platform_driver ioh_dai_driver_plat = {
 
+static int ioh_i2s_pci_probe(struct pci_dev *pdev,
+                          const struct pci_device_id *id)

Why are you creating these platform devices? I don't understand the function they serve. The code handling them looks to have quite a few problems but I'm not clear they should be there in the first place.

sound/soc/lapis/ml7213ioh-plat.c line 1393

Quoted 6 lines

sound/soc/lapis/ml7213ioh-plat.c new file +6 -0

+     rv = request_irq(pdev->irq, ioh_i2s_irq, IRQF_SHARED, "ml7213_ioh",
+                      pdev);
+     if (rv != 0) {
+             printk(KERN_ERR "Failed to allocate irq\n");
+             goto out_irq;
+     }

Are you *sure* you're ready to handle interrupts at this point?

2012年3月3日1:39 Mark Brown <[email protected]>:

Quoted 4 lines

I just merged Lars-Peter's dmaengine library code which has been on the list for a week or so - this should be updated to use that next time it's posted. That should save a lot of code from the driver and make sure it's following best practices for dmaengine use.

Sorry, we can't use this library.

Quoted 4 lines

sound/soc/lapis/ml7213ioh-plat.c line 37

Quoted 2 lines

sound/soc/lapis/ml7213ioh-plat.c new file +2 -0

+static struct ioh_i2s_data *i2s_data;
+static struct ioh_i2s_dma dmadata[MAX_I2S_CH];

Why are these needed, aren't they dynamically allocated by the driver?

You mean ASoC driver can't use global variable ?

Quoted 5 lines

sound/soc/lapis/ml7213ioh-plat.c line 894

Quoted 3 lines

sound/soc/lapis/ml7213ioh-plat.c new file +3 -0

+     case SNDRV_PCM_FORMAT_S32_LE:
+             byte = 24;
+             break;

That looks wrong... are you sure you don't support S24_LE or something?

This is correct.

Because maximum transmit size of ML7213's I2S hw is 24bit.

Quoted 6 lines

Quoted 6 lines

sound/soc/lapis/ml7213ioh-plat.c new file +4 -0

+static struct platform_driver ioh_i2s_driver_plat = {
 
+static struct platform_driver ioh_dai_driver_plat = {
 
+static int ioh_i2s_pci_probe(struct pci_dev *pdev,
+                          const struct pci_device_id *id)

Quoted 3 lines

Why are you creating these platform devices? I don't understand the function they serve. The code handling them looks to have quite a few problems but I'm not clear they should be there in the first place.

if these platform devices aren't used, device detection doesn't work correctly. So, I added these.

Quoted 6 lines

Quoted 6 lines

sound/soc/lapis/ml7213ioh-plat.c new file +6 -0

+     rv = request_irq(pdev->irq, ioh_i2s_irq, IRQF_SHARED, "ml7213_ioh",
+                      pdev);
+     if (rv != 0) {
+             printk(KERN_ERR "Failed to allocate irq\n");
+             goto out_irq;
+     }

Quoted 1 line

Are you *sure* you're ready to handle interrupts at this point?

This is just registering interrupt handler.
As long as interrupt register is not enabled, the interrupt handler is not called.
This is common request_irq description.
What's is your concern ?

Quoted 5 lines

Quoted 4 lines

I just merged Lars-Peter's dmaengine library code which has been on the list for a week or so - this should be updated to use that next time it's posted.  That should save a lot of code from the driver and make sure it's following best practices for dmaengine use.

Sorry, we can't use this library.

This sort of response really isn't on.  Do you have some reason for saying this?  Flat out refusing to do things with no reason is not useful.

Quoted 4 lines

Quoted 3 lines

sound/soc/lapis/ml7213ioh-plat.c line 37

Quoted 2 lines

sound/soc/lapis/ml7213ioh-plat.c new file +2 -0

+static struct ioh_i2s_data *i2s_data;
+static struct ioh_i2s_dma dmadata[MAX_I2S_CH];

Why are these needed, aren't they dynamically allocated by the driver?

You mean ASoC driver can't use global variable ?

In general no Linux driver should be using a global variable except for things like this.

Quoted 6 lines

Quoted 4 lines

Quoted 3 lines

+     case SNDRV_PCM_FORMAT_S32_LE:
+             byte = 24;
+             break;

That looks wrong...  are you sure you don't support S24_LE or something?

This is correct.
Because maximum transmit size of ML7213's I2S hw is 24bit.

Note we often lay out 24 bit audio in 32 bit blocks.

Quoted 4 lines

Quoted 4 lines

Quoted 3 lines

sound/soc/lapis/ml7213ioh-plat.c new file +2 -0

+static struct platform_driver ioh_i2s_driver_plat = {
 
+static struct platform_driver ioh_dai_driver_plat = {

Quoted 7 lines

Quoted 5 lines

Quoted 2 lines

+static int ioh_i2s_pci_probe(struct pci_dev *pdev,
+                          const struct pci_device_id *id)

Why are you creating these platform devices?  I don't understand the function they serve.  The code handling them looks to have quite a few problems but I'm not clear they should be there in the first place.

if these platform devices aren't used, device detection doesn't work correctly. So, I added these.

You've not actually mentioned the problem you were seeing...

Quoted 7 lines

Quoted 7 lines

Quoted 6 lines

+     rv = request_irq(pdev->irq, ioh_i2s_irq, IRQF_SHARED, "ml7213_ioh",
+                      pdev);
+     if (rv != 0) {
+             printk(KERN_ERR "Failed to allocate irq\n");
+             goto out_irq;
+     }

Are you *sure* you're ready to handle interrupts at this point?

Quoted 5 lines

This is just registering interrupt handler.
As long as interrupt register is not enabled, the interrupt handler is not called.
This is common request_irq description.
What's is your concern ?

Apart from anything else you've got the interrupt requested as IRQF_SHARED so the interrupt could get called at any time.  It's also not clear that you've got the hardware in a known good state.

On Tue, Mar 6, 2012 at 8:52 PM, Mark Brown

Quoted 4 lines

Quoted 2 lines

if these platform devices aren't used, device detection doesn't work correctly. So, I added these.

You've not actually mentioned the problem you were seeing...

I saw mapping problem between machine driver and platform driver. e.g. cpu_dai_name "ml7213ioh"

Quoted 3 lines

Apart from anything else you've got the interrupt requested as IRQF_SHARED so the interrupt could get called at any time.  It's also not clear that you've got the hardware in a known good state.

Do you mean request_irq should move to somewhere, like open() or hw_params() or ... ?

Quoted 5 lines

Quoted 3 lines

Quoted 2 lines

if these platform devices aren't used, device detection doesn't work correctly. So, I added these.

You've not actually mentioned the problem you were seeing...

I saw mapping problem between machine driver and platform driver. e.g.  cpu_dai_name "ml7213ioh"

You've still not explained the problem you're seeing but this sounds like something that should work.  Please address the

Quoted 5 lines

Quoted 3 lines

Apart from anything else you've got the interrupt requested as IRQF_SHARED so the interrupt could get called at any time.  It's also not clear that you've got the hardware in a known good state.

Do you mean request_irq should move to somewhere, like open() or hw_params() or ... ?

You need to do enough hardware and software initialisation prior to requesting the interrupt to ensure that if an interrupt does happen it won't cause any ill effects.  Checks in the interrupt handler to make sure things like the ALSA layer stuff have been set up can also be used to make the interrupt handler safer.

On Wed, Mar 7, 2012 at 8:46 PM, Mark Brown

Quoted 2 lines

You've still not explained the problem you're seeing but this sounds like something that should work.  Please address the

* Faced problem if there are no platform settings.
machine driver's hw_params is not called
platform_driver's functions(snd_pcm_ops, snd_soc_platform_driver) are not called.

As a result, the following message is not showed . (by soc_pcm.c) printk(KERN_INFO "asoc: %s <-> %s mapping ok\n", codec_dai->name,

         cpu_dai->name);

I can understand your concern.
Let me clarify your think.
You think that interrupt handler must check whether ALSA initialization has already finished or not before interrupt process executes. Right?

Quoted 9 lines

Quoted 2 lines

You've still not explained the problem you're seeing but this sounds like something that should work.  Please address the

* Faced problem if there are no platform settings.
machine driver's hw_params is not called
platform_driver's functions(snd_pcm_ops, snd_soc_platform_driver) are not called.
As a result, the following message is not showed . (by soc_pcm.c) printk(KERN_INFO "asoc: %s <-> %s mapping ok\n", codec_dai->name, cpu_dai->name);

This makes no sense.  Registering a platform device will have *no* impact on ASoC, it only cares if the relevant ASoC function drivers have come up.  Probably there was some other problem with what you were doing but it's hard to say...

This driver is for LAPIS Semiconductor ML7213 IOH I2S.

sound/soc/lapis/ml7213ioh-plat.c new file +4 -0

+static int ml7213i2s_dai_set_dai_sysclk(struct snd_soc_dai *dai,
+					int clk_id, unsigned int freq, int dir)
+{
+	u32 reg[MAX_I2S_CH];

So, I just said to the SPEAr guys that I really want to see support for non-cyclic dmaengine added to the helper library rather than open coding.  We're seeing lots of platforms moving to dmaengine (as well as those that are in already yours is one of three out of tree platforms I'm aware of that have actively submitted stuff) and with this number of platforms we really need to get stuff factored out - there's enough people working on these platforms that it should be possible to add the support to the dmaengine library code.

On Thu, Mar 22, 2012 at 1:09 AM, Mark Brown

Quoted 3 lines

So, I just said to the SPEAr guys that I really want to see support for non-cyclic dmaengine added to the helper library rather than open coding. ?We're seeing lots of platforms moving to dmaengine (as well as

Do you mean you don't accept our platform/machine driver's patches unless your DMA library will have supported non-cyclic dmaengine ? Or if I'll modify your minor comments, can you accept our patches ?

So, I just said to the SPEAr guys that I really want to see support for non-cyclic dmaengine added to the helper library rather than open coding. ?We're seeing lots of platforms moving to dmaengine (as well as

On Tue, Mar 27, 2012 at 12:40 AM, Mark Brown

Quoted 3 lines

So, I just said to the SPEAr guys that I really want to see support for non-cyclic dmaengine added to the helper library rather than open coding. ?We're seeing lots of platforms moving to dmaengine (as well as

What is going on this ?
Let me know this status.

Quoted 6 lines

Quoted 3 lines

So, I just said to the SPEAr guys that I really want to see support for non-cyclic dmaengine added to the helper library rather than open coding. ?We're seeing lots of platforms moving to dmaengine (as well as

What is going on this ?
Let me know this status.

Nobody seems to be working on it as far as I can tell, I've certainly not seen any patches.

On Wed, May 23, 2012 at 6:41 PM, Mark Brown

Quoted 2 lines

Nobody seems to be working on it as far as I can tell, I've certainly not seen any patches.

If so, could you accept current platform/machine driver ?

Quoted 4 lines

Quoted 2 lines

Nobody seems to be working on it as far as I can tell, I've certainly not seen any patches.

If so, could you accept current platform/machine driver ?

Ideally what would be happening here is that you or other people who have such systems would be working to add the required support to the core code, there's clearly a need for common code here as there are a number of different systems that don't have cyclic DMA and it wouldn't be great to end up with duplicated code.

Is there some great difficulty in factoring out the support for non-cyclic audio DMA on dmaengine - it seems like if there is we must have a serious problem in dmaengine which we should fix?  If there is a substantial difficulty then that's different but it doesn't feel like we've tried doing common code yet, if there's problems doing that I'd like to understand what they are before we jump ahead of ourselves.

Quoted 11 lines · Mark Brown

Quoted 4 lines

Quoted 2 lines

Nobody seems to be working on it as far as I can tell, I've certainly not seen any patches.

If so, could you accept current platform/machine driver ?

Ideally what would be happening here is that you or other people who have such systems would be working to add the required support to the core code, there's clearly a need for common code here as there are a number of different systems that don't have cyclic DMA and it wouldn't be great to end up with duplicated code.

Quoted 6 lines

Is there some great difficulty in factoring out the support for non-cyclic audio DMA on dmaengine - it seems like if there is we must have a serious problem in dmaengine which we should fix? If there is a substantial difficulty then that's different but it doesn't feel like we've tried doing common code yet, if there's problems doing that I'd like to understand what they are before we jump ahead of ourselves.

I'm not so familiar with Linux's DMA idea.
So we don't know whether non-cyclic dmaengine has problem or not.

Until now, we've developed device drivers use DMA driver like UART, SPI. These drivers are implemented using the same way, you called "non-cyclic", and already applied.

As you said, common code for DMA code can be best solution. However, currently, the code is nothing.
So, I want you to accept our driver as first step.
Because I think supporting new device is more important for linux than dmaengine common.

thanks

Quoted 4 lines · Tomoya MORINAGA

On Thu, May 24, 2012 at 7:07 PM, Mark Brown

I'm not so familiar with Linux's DMA idea.
So we don't know whether non-cyclic dmaengine has problem or not.

First you should not be writing your own dma driver, it *needs* to use dmaenegine. We already have bunch of driver supported, so there may be a

Quoted 5 lines

As you said, common code for DMA code can be best solution. However, currently, the code is nothing.
So, I want you to accept our driver as first step.
Because I think supporting new device is more important for linux than dmaengine common.

Quoted 2 lines

Quoted 2 lines

I'm not so familiar with Linux's DMA idea.
So we don't know whether non-cyclic dmaengine has problem or not.

Nobody has written the code, this is the problem!  If the code is not there, you should try to write it.  If there is some great problem writing the code then you should t

Quoted 2 lines

First you should not be writing your own dma driver, it *needs* to use dmaenegine. We already have bunch of driver supported, so there may be a

He's already done that, their current code is all open coded dmaengine stuff.

Quoted 5 lines

Quoted 5 lines

As you said, common code for DMA code can be best solution. However, currently, the code is nothing.
So, I want you to accept our driver as first step.
Because I think supporting new device is more important for linux than dmaengine common.

The existing code is far from nothing, there is a fairly substantial dmaengine library there already which should share a big chunk of code with any cyclic support.  If you were saying "this is too hard for $REASON" that'd be one thing but that's not what you're saying here.

It's possible that there is actually some substantial difficult but my first instinct would be that it should be relatively straightforward.

Quoted 3 lines · Mark Brown

Nobody has written the code, this is the problem!  If the code is not there, you should try to write it.  If there is some great problem writing the code then you should t

The latter of the above seems dropped...

Quoted 5 lines

Quoted 2 lines

First you should not be writing your own dma driver, it *needs* to use dmaenegine. We already have bunch of driver supported, so there may be a

He's already done that, their current code is all open coded dmaengine stuff.

I don't understand why you say so ?
I don't use any own dma driver, right ? I use only dmaengine's. If there is own, let me show.

Quoted 4 lines

The existing code is far from nothing, there is a fairly substantial dmaengine library there already which should share a big chunk of code with any cyclic support.  If you were saying "this is too hard for $REASON" that'd be one thing but that's not what you're saying here.

If our ASoC supports cyclic dma mode, we must modify both pch_dma driver and our ASoC driver.

I don't want to do this.
Because I can't understand the merit. In plain words, to me, this looks insignificant things.

Quoted 2 lines

It's possible that there is actually some substantial difficult but my first instinct would be that it should be relatively straightforward.

Let me clarify your saying again.
Which do you want ?
1) pch_dma must support cyclic dma mode and our ASoC driver must use
the cyclic dma function.
2) Non-cyclic dma engine should be added to alsa-dmaengine by myself.

3) Other

Quoted 7 lines

Quoted 4 lines

Quoted 2 lines

First you should not be writing your own dma driver, it *needs* to use dmaenegine. We already have bunch of driver supported, so there may be a

He's already done that, their current code is all open coded dmaengine stuff.

I don't understand why you say so ?
I don't use any own dma driver, right ? I use only dmaengine's. If there is own, let me show.

Please re-read what I wrote.

Quoted 7 lines

Quoted 4 lines

The existing code is far from nothing, there is a fairly substantial dmaengine library there already which should share a big chunk of code with any cyclic support.  If you were saying "this is too hard for $REASON" that'd be one thing but that's not what you're saying here.

If our ASoC supports cyclic dma mode, we must modify both pch_dma driver and our ASoC driver.

No, all current mainline drivers using the library use cyclic DMA.

Quoted 3 lines

I don't want to do this.
Because I can't understand the merit. In plain words, to me, this looks insignificant things.

The purpose of this change is to factor code out of individual drivers into generic code rather than having lots of people writing exactly the same code.  Code duplication at this level is pointless and makes more work for everyone who will have to maintain the code going forward.

Having looked at Russell's out of tree code I'm even more convinced that the amount of new code needed for non-cyclic DMA should be pretty trivial.

On Wed, May 30, 2012 at 9:14 PM, Mark Brown

Quoted 1 line

Please re-read what I wrote.

Let me clarify.
Do you say native DMA driver API like dmaengine_prep_slave_sg(), dmaengine_submit() shouldn't be used from ASoC driver, right ?

Quoted 1 line

No, all current mainline drivers using the library use cyclic DMA.

I can't see any driver uses cyclic DMA. (I saw linux-next's tree from kernel.org.)
Where is your saying "current mainline" ?

However I think it seems difficult for supporting all devices. Because hardware dependency control code can't be added.
For example, for ML7213, needs interrupt control both before/after DMA transfer. However, in case of using soc-dmaengine, the control can't be done. Because the processing is in soc-dmaengine.

Quoted 5 lines

Quoted 1 line

Please re-read what I wrote.

Let me clarify.
Do you say native DMA driver API like dmaengine_prep_slave_sg(), dmaengine_submit() shouldn't be used from ASoC driver, right ?

Your driver should be written in terms of the ASoC DMA framework.

Quoted 4 lines

Quoted 1 line

No, all current mainline drivers using the library use cyclic DMA.

I can't see any driver uses cyclic DMA. (I saw linux-next's tree from kernel.org.)
Where is your saying "current mainline" ?

Linus' tree, or mine.

Have you even looked at the soc-dmaengine-pcm code?  It uncondtionally requests a cyclic channel.

Quoted 5 lines

However I think it seems difficult for supporting all devices. Because hardware dependency control code can't be added.
For example, for ML7213, needs interrupt control both before/after DMA transfer. However,  in case of using soc-dmaengine, the control can't be done. Because the processing is in soc-dmaengine.

Please be more specific.  What are the concrete problems that you see? Why is it not possible to address them within the framework?

Quoted 1 line

Linus' tree, or mine.

Sorry, I couldn't find.
Is your saying Linus's tree this ?
git://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git
However, any ASoC driver doesn't use cyclic DMA's.

Quoted 4 lines

Sorry, I couldn't find.
Is your saying Linus's tree this ?
git://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git
However, any ASoC driver doesn't use cyclic DMA's.

$ grep CYCLIC sound/soc/soc-dmaengine-pcm.c
       dma_cap_set(DMA_CYCLIC, mask);

That is to say, some drivers ASoC DMA framework. However some drivers don't use any ASoC DMA framework.

Why didn't you do porting to your saying "ASoC DMA framework"?

There are some drivers that predate the availibility of the dmaengine framework.

This, and the fact that the PCH driver uses dmaengine without using any shared code, is the topic of the discussion.  It is entirely natural that there are no open coded drivers using cyclic DMA via dmaengine, they have all had that code factored out.

Please stop this and at least look at factoring this code out.  If it turns out that there's some reason why it's a lot of work that's one thing but it seems clear that you've not even looked yet.  It's like the previous issues where you were sending patches which would clearly not run successfully, it's not a very positive approach.

On Mon, Jun 11, 2012 at 5:54 PM, Mark Brown

Quoted 2 lines

There are some drivers that predate the availibility of the dmaengine framework.

In case, you develop a new ASoC framework, all previous ASoC drivers should be adopted to the framework by yourself, don't you ? Because a new developer must be confused by this.

Quoted 18 lines

Quoted 17 lines

Quoted 4 lines

Quoted 2 lines

Nobody seems to be working on it as far as I can tell, I've certainly not seen any patches.

If so, could you accept current platform/machine driver ?

Ideally what would be happening here is that you or other people who have such systems would be working to add the required support to the core code, there's clearly a need for common code here as there are a number of different systems that don't have cyclic DMA and it wouldn't be great to end up with duplicated code.

Is there some great difficulty in factoring out the support for non-cyclic audio DMA on dmaengine - it seems like if there is we must have a serious problem in dmaengine which we should fix?  If there is a substantial difficulty then that's different but it doesn't feel like we've tried doing common code yet, if there's problems doing that I'd like to understand what they are before we jump ahead of ourselves.

Quoted 2 lines

I'm not so familiar with Linux's DMA idea.
So we don't know whether non-cyclic dmaengine has problem or not.

First you should not be writing your own dma driver, it *needs* to use dmaenegine. We already have bunch of driver supported, so there may be a case that existing driver works straight or with little modifications. Using your own dma driver is a dead road, so sooner you move over better it is.

sound/soc/lapis/Kconfig config SND_SOC_ML7213_PLATFORM · line 9

Quoted 7 lines

This option enables support for the AC Link Controllers in ML7213 IOH SoC.

sound/soc/lapis/Kconfig +5 -0

+config SND_SOC_ML7213_MACHINE
+     tristate "ML7213 IOH ASoC machine driver"
+     select SND_SOC_ML7213_PLATFORM
+     help
+       This is ASoC machine driver for ML7213 IOH

This needs to depend on the CODEC as well, how have you tested?

You're also missing a blank line between this and the previous stanza.

sound/soc/lapis/ml7213ioh-machine.c line 184

Quoted 10 lines

sound/soc/lapis/ml7213ioh-machine.c new file +9 -0

+static struct platform_device *ioh_snd_device;
+
+
+static int __init ioh_i2s_init(void)
+{
+     int ret;
+     ioh_snd_device = platform_device_alloc("soc-audio", -1);
+     if (!ioh_snd_device)
+             return -ENOMEM;

Use snd_soc_register_card() and a real device.

2012年3月2日22:05 Mark Brown <[email protected]>:

Quoted 1 line

You're also missing a blank line between this and the previous stanza.

Sorry, I can't understand your saying.

Quoted 1 line

Use snd_soc_register_card() and a real device.

OK.
As a matter of interest, why do you recommend me to use snd_soc_register_card() ?

Quoted 2 lines

Quoted 1 line

You're also missing a blank line between this and the previous stanza.

Sorry, I can't understand your saying.

Your file should look like other similar files in the kernel.

Quoted 9 lines · Tomoya MORINAGA

+     /* set codec DAI configuration */
+     ret = snd_soc_dai_set_fmt(codec_dai, SND_SOC_DAIFMT_I2S |
+                              SND_SOC_DAIFMT_NB_NF | SND_SOC_DAIFMT_CBS_CFS);
+     if (ret < 0)
+             return ret;
+
+     /* set cpu DAI configuration */
+     ret = snd_soc_dai_set_fmt(cpu_dai, SND_SOC_DAIFMT_I2S |
+                              SND_SOC_DAIFMT_NB_NF | SND_SOC_DAIFMT_CBM_CFM);

This looks very broken, you're setting the two ends of the link to different configurations.  If this is needed it should be documented why.

Also, this should be set in the dai_link structure as data rather than set each time.

OK.
codec is slave, i2s controller is master.
So, I'll use SND_SOC_DAIFMT_CBS_CFM.

Quoted 3 lines · Tomoya MORINAGA

OK.
codec is slave, i2s controller is master.
So, I'll use SND_SOC_DAIFMT_CBS_CFM.

That means "Codec bit clock master frame clock slave".  Are you sure that's what you want?

On Thu, Mar 15, 2012 at 7:50 PM, Mark Brown

Quoted 2 lines

That means "Codec bit clock master frame clock slave".  Are you sure that's what you want?

Let me clarify the above.
Does the above "frame clock" mean "data line" of I2S? If yes, I should have used 'SND_SOC_DAIFMT_CBS_CFS'.

Quoted 6 lines

Quoted 2 lines

That means "Codec bit clock master frame clock slave".  Are you sure that's what you want?

Let me clarify the above.
Does the above "frame clock" mean "data line" of I2S? If yes, I should have used 'SND_SOC_DAIFMT_CBS_CFS'.

No, it means the clock which runs at sample rate (usually called the frame clock or LRCLK).

+MODULE_ALIAS("platform: lapis-ml7213ioh-CRB");

Quoted 3 lines

+     snd_soc_dapm_enable_pin(dapm, "Speaker");
+     snd_soc_dapm_enable_pin(dapm, "AnalogMIC");
+     snd_soc_dapm_enable_pin(dapm, "MICBIAS");

Why are you doing thiS?  You never disable these things so if this were needed you may as well do it on init, but since the default state is enabled anyway this shouldn't be doing anytihng.

Quoted 1 line

+ SOC_SINGLE("Digital Switch", ML26124_DVOL_CTL, 4, 1, 0),

This should probably be Digital Playback Switch so it lines up with the volume control in the UIs (and the Volume controls should be Digital X Volume).

Quoted 4 lines

sound/soc/codecs/ml26124.c new file +4 -0

+static int ml26124_set_dai_clkdiv(struct snd_soc_dai *codec_dai,
+                               int div_id, int div)
+{
+     struct snd_soc_codec *codec = codec_dai->codec;

sound/soc/codecs/ml26124.c line 515

Quoted 6 lines

sound/soc/codecs/ml26124.c new file +6 -0

+
+     switch (div_id) {
+     case ML26124_MCLK:
+             ml26124_update_bits(codec, ML26124_CLK_CTL,
+                                   BIT(0) | BIT(1), div);
+             break;

Why can't the driver calculate this automatically given the MCLK?

Quoted 6 lines

sound/soc/codecs/ml26124.c new file +6 -0

+static int ml26124_pcm_trigger(struct snd_pcm_substream *substream,
+                           int cmd, struct snd_soc_dai *codec_dai)
+{
+     struct snd_soc_codec *codec = codec_dai->codec;
+
+     switch (cmd) {

sound/soc/codecs/ml26124.c line 563

Quoted 5 lines

sound/soc/codecs/ml26124.c new file +5 -0

+     case SNDRV_PCM_TRIGGER_STOP:
+     case SNDRV_PCM_TRIGGER_SUSPEND:
+     case SNDRV_PCM_TRIGGER_PAUSE_PUSH:
+             ml26124_update_bits(codec, ML26124_REC_PLYBAK_RUN, 0x3,
+                                 0);

I'm not sure you've tested this, you can't do I2C I/O from atomic context and trigger is atomic. What does this control actually do?

sound/soc/codecs/ml26124.c line 640

Quoted 12 lines

sound/soc/codecs/ml26124.c new file +11 -0

+static int ml26124_probe(struct snd_soc_codec *codec)
+{
+     int ret;
+     struct ml26124_priv *priv = snd_soc_codec_get_drvdata(codec);
+     codec->control_data = priv->regmap;
+
+     ret = snd_soc_codec_set_cache_io(codec, 8, 8, SND_SOC_I2C);
+     if (ret < 0) {
+             dev_err(codec->dev, "Failed to set cache I/O: %d\n", ret);
+             return ret;
+     }

You're mixing regmap API usage and ASoC level I2C, this should be _REGMAP.

sound/soc/codecs/ml26124.c line 727

Quoted 6 lines

sound/soc/codecs/ml26124.c new file +6 -0

+static int __init ml26124_modinit(void)
+{
+     int ret;
+
+     ret = i2c_add_driver(&ml26124_i2c_driver);
+     if (ret != 0)

module_i2c_driver().

2012年3月1日8:51 Mark Brown <[email protected]>:

Quoted 10 lines

Quoted 10 lines

sound/soc/codecs/ml26124.c new file +9 -0

+static int ml26124_set_dai_clkdiv(struct snd_soc_dai *codec_dai,
+                               int div_id, int div)
+{
+     struct snd_soc_codec *codec = codec_dai->codec;
 
+     switch (div_id) {
+     case ML26124_MCLK:
+             ml26124_update_bits(codec, ML26124_CLK_CTL,
+                                   BIT(0) | BIT(1), div);
+             break;

Quoted 1 line

Why can't the driver calculate this automatically given the MCLK?

Sorry, I can't understand your saying.
Why does this driver need to calculate ?
I think this driver can use "div" value directly.

Quoted 4 lines

Quoted 1 line

+ ret = snd_soc_codec_set_cache_io(codec, 8, 8, SND_SOC_I2C);

You're mixing regmap API usage and ASoC level I2C, this should be _REGMAP.

Currently, it seems codec control via i2c works well. Let me know why I should use SND_SOC_REGMAP not _I2C.

Quoted 8 lines

sound/soc/codecs/ml26124.c line 727

Quoted 6 lines

sound/soc/codecs/ml26124.c new file +6 -0

+static int __init ml26124_modinit(void)
+{
+     int ret;
+
+     ret = i2c_add_driver(&ml26124_i2c_driver);
+     if (ret != 0)

module_i2c_driver().

Sorry, I can't understand your saying.

Quoted 12 lines

Quoted 12 lines

sound/soc/codecs/ml26124.c line 515

Quoted 10 lines

sound/soc/codecs/ml26124.c new file +9 -0

+static int ml26124_set_dai_clkdiv(struct snd_soc_dai *codec_dai,
+                               int div_id, int div)
+{
+     struct snd_soc_codec *codec = codec_dai->codec;
 
+     switch (div_id) {
+     case ML26124_MCLK:
+             ml26124_update_bits(codec, ML26124_CLK_CTL,
+                                   BIT(0) | BIT(1), div);
+             break;

Why can't the driver calculate this automatically given the MCLK?

Quoted 3 lines

Sorry, I can't understand your saying.
Why does this driver need to calculate ?
I think this driver can use "div" value directly.

Why does the user have to set this divider by hand at all? There's already a set_sysclk() function, just have the user specify the clock they're providing.

Quoted 5 lines

Quoted 3 lines

Quoted 1 line

+ ret = snd_soc_codec_set_cache_io(codec, 8, 8, SND_SOC_I2C);

You're mixing regmap API usage and ASoC level I2C, this should be _REGMAP.

Currently, it seems codec control via i2c works well. Let me know why I should use SND_SOC_REGMAP not _I2C.

Have you checked what _I2C and _REGMAP do?

Quoted 9 lines

Quoted 9 lines

sound/soc/codecs/ml26124.c line 727

Quoted 7 lines

sound/soc/codecs/ml26124.c new file +6 -0

+static int __init ml26124_modinit(void)
+{
+     int ret;
+
+     ret = i2c_add_driver(&ml26124_i2c_driver);
+     if (ret != 0)

module_i2c_driver().

Quoted 1 line

Sorry, I can't understand your saying.

Use module_i2c_driver().

sound/soc/codecs/ml26124.c line 299

Quoted 2 lines

sound/soc/codecs/ml26124.c new file +2 -0

+static int ml26124_update_bits(struct snd_soc_codec *codec, unsigned short reg,
+                             unsigned int mask, unsigned int value)

Why are you open coding this in your driver? There is no point in replicating subsystem functionality.

2012年3月2日21:58 Mark Brown <[email protected]>:

Quoted 3 lines

Quoted 1 line

Let me know why I should use SND_SOC_REGMAP not _I2C.

Have you checked what _I2C and _REGMAP do?

I've just checked it.
If SND_SOC_REGMAP is used, it seems there is no processing in it.

So, can I delete snd_soc_codec_set_cache_io ?

Quoted 1 line

Use module_i2c_driver().

I understand.

As a matter of interest, why can I use module_i2c_driver ?

There is no ASoC driver uses module_i2c_driver.

Quoted 5 lines

sound/soc/codecs/ml26124.c line 299

Quoted 2 lines

sound/soc/codecs/ml26124.c new file +2 -0

+static int ml26124_update_bits(struct snd_soc_codec *codec, unsigned short reg,
+                             unsigned int mask, unsigned int value)

Why are you open coding this in your driver? There is no point in replicating subsystem functionality.

For ML26124 spec, it's necessary.
Because the codec's register read-address and write-address is not the same. E.G.
Sampling Rate Register
R: offset=0
W: offset=1

This is not the case. As I have *REPEATEDLY* told you you should be sumbitting code against current development versions of the subsystem.

Quoted 11 lines

Quoted 4 lines

sound/soc/codecs/ml26124.c line 299

Quoted 2 lines

sound/soc/codecs/ml26124.c new file +2 -0

+static int ml26124_update_bits(struct snd_soc_codec *codec, unsigned short reg,
+                             unsigned int mask, unsigned int value)

Why are you open coding this in your driver? There is no point in replicating subsystem functionality.

For ML26124 spec, it's necessary.
Because the codec's register read-address and write-address is not the same. E.G.
Sampling Rate Register
R: offset=0
W: offset=1

This is not what the above prototype says, the above prototype has only one register in it. You need to make this *much* clearer for the registers it's affecting.

On Tue, Mar 06, 2012　19:00 Mark Brown <[email protected]>:

Quoted 3 lines

This is not what the above prototype says, the above prototype has only one register in it. You need to make this *much* clearer for the registers it's affecting.

Sorry, I couldn't understand your saying / concern.
Do you mean I shouldn't use ml26124_update_bits() but snd_soc_update_bits() ?

Quoted 5 lines

Quoted 3 lines

This is not what the above prototype says, the above prototype has only one register in it.  You need to make this *much* clearer for the registers it's affecting.

Sorry, I couldn't understand your saying / concern.
Do you mean I shouldn't use ml26124_update_bits() but snd_soc_update_bits() ?

Ideally you should be using the framework features.  If there is some reason why you can't you need to make it much clearer what the reason is so people reading the code can understand.

If possible, I want to use framework features. However, I judged it is not possible.
Let me confirm again.
As I said before, all ML26124 registers need to be accessed by different address when read and write.
Like below
R W
$00h $01h Sampling setting
$02h $03h PLL setting
...

Even so, can I use framework features ?

Quoted 11 lines

If possible, I want to use framework features. However, I judged it is not possible.
Let me confirm again.
As I said before, all ML26124 registers need to be accessed by different address when read and write.
Like below
R       W
$00h $01h Sampling setting
$02h $03h PLL setting
...
Even so, can I use framework features ?

No, in which case (to repeat what I said previously) you need to make the code *much* clearer so the reader can understand what it is doing and why it's having to open code this stuff.

If so, do you mean that I should just add explanation (like as I explained before)
why the function() is necessary to above prototype of ml26124_update_bits() ?

sound/soc/codecs/ml26124.c new file +13 -0

+static int ml26124_set_dai_sysclk(struct snd_soc_dai *codec_dai,
+		int clk_id, unsigned int freq, int dir)
+{
+	struct snd_soc_codec *codec = codec_dai->codec;
+	struct ml26124_priv *priv = snd_soc_codec_get_drvdata(codec);
+
+	switch (clk_id) {
+	case ML26124_USE_PLL:
+		ml26124_update_bits(codec, ML26124_CLK_CTL,
+				    BIT(0) | BIT(1), 0);
+		break;
+	case ML26124_USE_MCLKI_256FS:
+		ml26124_update_bits(codec, ML26124_CLK_CTL,

Quoted 4 lines · Tomoya MORINAGA

+     /* SAI configuration */
+     if (priv->channels == 1) {
+             ml26124_update_bits(codec, ML26124_SAI_TRANS_CTL, mask,
+                             ML26124_SAI_NO_DELAY | ML26124_SAI_FRAME_SYNC);

Quoted 8 lines

+             ml26124_update_bits(codec, ML26124_SAI_RCV_CTL, mask,
+                             ML26124_SAI_NO_DELAY | ML26124_SAI_FRAME_SYNC);
+     } else {
+             ml26124_update_bits(codec, ML26124_SAI_TRANS_CTL, mask,
+                                 ML26124_SAI_NO_DELAY);
+             ml26124_update_bits(codec, ML26124_SAI_RCV_CTL, mask,
+                                 ML26124_SAI_NO_DELAY);
+     }

What happens if the system switches between 1 channel mode and multi-channel mode at runtime?  It looks like _SYNC will never be cleared.

Quoted 18 lines

sound/soc/codecs/ml26124.c new file +18 -0

+static int ml26124_set_dai_sysclk(struct snd_soc_dai *codec_dai,
+             int clk_id, unsigned int freq, int dir)
+{
+     struct snd_soc_codec *codec = codec_dai->codec;
+     struct ml26124_priv *priv = snd_soc_codec_get_drvdata(codec);
+
+     switch (clk_id) {
+     case ML26124_USE_PLL:
+             ml26124_update_bits(codec, ML26124_CLK_CTL,
+                                 BIT(0) | BIT(1), 0);
+             break;
+     case ML26124_USE_MCLKI_256FS:
+             ml26124_update_bits(codec, ML26124_CLK_CTL,
+                                 BIT(0) | BIT(1), 1);
+             break;
+     case ML26124_USE_MCLKI_512FS:
+             ml26124_update_bits(codec, ML26124_CLK_CTL,
+                                 BIT(0) | BIT(1), 2);

Quoted 5 lines

+             break;
+     case ML26124_USE_MCLKI_1024FS:
+             ml26124_update_bits(codec, ML26124_CLK_CTL,
+                                 BIT(0) | BIT(1), 3);
+             break;

Why not just specify the MCLK rate and then calculate the division based on the sample rate?

Quoted 6 lines

+     case SND_SOC_BIAS_STANDBY:
+             /* VMID ON */
+             ml26124_update_bits(codec, ML26124_PW_REF_PW_MNG,
+                                 ML26124_VMID, ML26124_VMID);
+             msleep(500);

This will sleep for 500ms when powering down which probably isn't what's desired...

Quoted 11 lines · Mark Brown

Quoted 11 lines

If possible, I want to use framework features. However, I judged it is not possible.
Let me confirm again.
As I said before, all ML26124 registers need to be accessed by different address when read and write.
Like below
R W
$00h $01h Sampling setting
$02h $03h PLL setting
...
Even so, can I use framework features ?

Quoted 3 lines

No, in which case (to repeat what I said previously) you need to make the code *much* clearer so the reader can understand what it is doing and why it's having to open code this stuff.

thanks.

If this is for all registers, how does stuff like DAPM work, which uses the framework internal update_bits function?

If this is for all registers, how does stuff like DAPM work, which uses the framework internal update_bits function?

If it's the case that the write address is always read address + 1 you can still use the framework if you set write_flag_mask to 0x01 in your regmap config. In my opinion it even makes sense to shift all the register numbers to the left by one and use a 7/9 instead of a 8/8 addr/reg layout. This will reduce the amount of memory wasted due to holes in the register cache.

Quoted 8 lines

If this is for all registers, how does stuff like DAPM work, which uses the framework internal update_bits function?

If it's the case that the write address is always read address + 1 you can still use the framework if you set write_flag_mask to 0x01 in your regmap config. In my opinion it even makes sense to shift all the register numbers to the left by one and use a 7/9 instead of a 8/8 addr/reg layout. This will reduce the amount of memory wasted due to holes in the register cache.

Thank you for your information.
I've just been investigating these.
I also found ASoC core access i2c driver via snd_soc_update_bits or others. As a result, write register address is the same as read's.

I'll try to implement these.

BTW, currently, by settting ML26124 codec with fixed value, I've confirmed both playback and capture work well.

When SND_SOC_DAPM_SUPPLY is executed ?
or
Do I need to execute from application (like amixer) ?

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

Quoted 2 lines

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

Applied, thanks, but:

 - Stop sending patches as followups to old patches, this really doesn't
 - Do try to use subject lines for your patches which are consistent with the subsystem.