Re: [alsa-devel] [PATCH] ASoC: dwc: disallow building designware_pcm as a module
Hi, On 21-04-2017 11:49, Mark Brown wrote: > On Fri, Apr 21, 2017 at 12:39:30PM +0200, Takashi Iwai wrote: >> Jose Abreu wrote: >>> Maybe rename to "dwc-i2s.c" and "dwc-pcm.c" (as the folder is >>> called "dwc") and let the module still be called "designware-i2s"? >> Lubomir's patch keeps the module name intact. My point is that rename >> of a file isn't nice to look at the git commit history, so it's better >> to be avoided as much as possible. But in this case, it looks >> unavoidable. > Right. Renaming the source file is the lesser evil, it's possible > people might have the module name in their configuration for their > systems somewhere. I agree. I just noticed today that without a valid license tag we get unresolved symbols when inserting pcm module so this patch is really needed as a fix. Lubomir, could you address the review comments and resend? If you are not available let me know and I will fix and resend the patch with your sign-off. Best regards, Jose Miguel Abreu
Re: [alsa-devel] [PATCH] ASoC: dwc: disallow building designware_pcm as a module
Hi, On 21-04-2017 11:49, Mark Brown wrote: > On Fri, Apr 21, 2017 at 12:39:30PM +0200, Takashi Iwai wrote: >> Jose Abreu wrote: >>> Maybe rename to "dwc-i2s.c" and "dwc-pcm.c" (as the folder is >>> called "dwc") and let the module still be called "designware-i2s"? >> Lubomir's patch keeps the module name intact. My point is that rename >> of a file isn't nice to look at the git commit history, so it's better >> to be avoided as much as possible. But in this case, it looks >> unavoidable. > Right. Renaming the source file is the lesser evil, it's possible > people might have the module name in their configuration for their > systems somewhere. I agree. I just noticed today that without a valid license tag we get unresolved symbols when inserting pcm module so this patch is really needed as a fix. Lubomir, could you address the review comments and resend? If you are not available let me know and I will fix and resend the patch with your sign-off. Best regards, Jose Miguel Abreu
Re: [alsa-devel] [PATCH] ASoC: dwc: disallow building designware_pcm as a module
On Fri, Apr 21, 2017 at 12:39:30PM +0200, Takashi Iwai wrote: > Jose Abreu wrote: > > Maybe rename to "dwc-i2s.c" and "dwc-pcm.c" (as the folder is > > called "dwc") and let the module still be called "designware-i2s"? > Lubomir's patch keeps the module name intact. My point is that rename > of a file isn't nice to look at the git commit history, so it's better > to be avoided as much as possible. But in this case, it looks > unavoidable. Right. Renaming the source file is the lesser evil, it's possible people might have the module name in their configuration for their systems somewhere. signature.asc Description: PGP signature
Re: [alsa-devel] [PATCH] ASoC: dwc: disallow building designware_pcm as a module
On Fri, Apr 21, 2017 at 12:39:30PM +0200, Takashi Iwai wrote: > Jose Abreu wrote: > > Maybe rename to "dwc-i2s.c" and "dwc-pcm.c" (as the folder is > > called "dwc") and let the module still be called "designware-i2s"? > Lubomir's patch keeps the module name intact. My point is that rename > of a file isn't nice to look at the git commit history, so it's better > to be avoided as much as possible. But in this case, it looks > unavoidable. Right. Renaming the source file is the lesser evil, it's possible people might have the module name in their configuration for their systems somewhere. signature.asc Description: PGP signature
Re: [alsa-devel] [PATCH] ASoC: dwc: disallow building designware_pcm as a module
On Fri, 21 Apr 2017 12:34:00 +0200, Jose Abreu wrote: > > Hi, > > > On 20-04-2017 21:24, Takashi Iwai wrote: > > So, I think Lubomir's change is right. But the patch subject and > > description should be rephrased. > > > > One thing I don't like is the rename of the file. But in this > > particular case, it's unavoidable unless we rename the module name. > > > > Maybe rename to "dwc-i2s.c" and "dwc-pcm.c" (as the folder is > called "dwc") and let the module still be called "designware-i2s"? Lubomir's patch keeps the module name intact. My point is that rename of a file isn't nice to look at the git commit history, so it's better to be avoided as much as possible. But in this case, it looks unavoidable. Takashi
Re: [alsa-devel] [PATCH] ASoC: dwc: disallow building designware_pcm as a module
On Fri, 21 Apr 2017 12:34:00 +0200, Jose Abreu wrote: > > Hi, > > > On 20-04-2017 21:24, Takashi Iwai wrote: > > So, I think Lubomir's change is right. But the patch subject and > > description should be rephrased. > > > > One thing I don't like is the rename of the file. But in this > > particular case, it's unavoidable unless we rename the module name. > > > > Maybe rename to "dwc-i2s.c" and "dwc-pcm.c" (as the folder is > called "dwc") and let the module still be called "designware-i2s"? Lubomir's patch keeps the module name intact. My point is that rename of a file isn't nice to look at the git commit history, so it's better to be avoided as much as possible. But in this case, it looks unavoidable. Takashi
Re: [alsa-devel] [PATCH] ASoC: dwc: disallow building designware_pcm as a module
Hi, On 20-04-2017 21:24, Takashi Iwai wrote: > So, I think Lubomir's change is right. But the patch subject and > description should be rephrased. > > One thing I don't like is the rename of the file. But in this > particular case, it's unavoidable unless we rename the module name. > Maybe rename to "dwc-i2s.c" and "dwc-pcm.c" (as the folder is called "dwc") and let the module still be called "designware-i2s"? > BTW, we should drop the superfluous EXPORT_SYMBOL*(), too. > > > Lubomir, could you please remove the EXPORT_SYMBOL, change the commit message and resend? Best regards, Jose Miguel Abreu
Re: [alsa-devel] [PATCH] ASoC: dwc: disallow building designware_pcm as a module
Hi, On 20-04-2017 21:24, Takashi Iwai wrote: > So, I think Lubomir's change is right. But the patch subject and > description should be rephrased. > > One thing I don't like is the rename of the file. But in this > particular case, it's unavoidable unless we rename the module name. > Maybe rename to "dwc-i2s.c" and "dwc-pcm.c" (as the folder is called "dwc") and let the module still be called "designware-i2s"? > BTW, we should drop the superfluous EXPORT_SYMBOL*(), too. > > > Lubomir, could you please remove the EXPORT_SYMBOL, change the commit message and resend? Best regards, Jose Miguel Abreu
Re: [alsa-devel] [PATCH] ASoC: dwc: disallow building designware_pcm as a module
On Thu, Apr 20, 2017 at 10:24:14PM +0200, Takashi Iwai wrote: > Mark Brown wrote: > > I think forcing this to be built in to the kernel (which is what the > > commit message says the change is going to do) is an obviously bad > > idea. Anything we add to the base kernel image needs to have a good > > reason to be there and it is hard to think what that reason might be for > > any audio driver, we need to be able to put this code into a module. > Well, I guess the original patch description caused a big confusion. > As far as I see, the intention of the patch is not about the module or > built-in kernel. Instead it's rather to fold designware_pcm stuff > into the single designware_i2s driver. Ah, right. That'd be fine. signature.asc Description: PGP signature
Re: [alsa-devel] [PATCH] ASoC: dwc: disallow building designware_pcm as a module
On Thu, Apr 20, 2017 at 10:24:14PM +0200, Takashi Iwai wrote: > Mark Brown wrote: > > I think forcing this to be built in to the kernel (which is what the > > commit message says the change is going to do) is an obviously bad > > idea. Anything we add to the base kernel image needs to have a good > > reason to be there and it is hard to think what that reason might be for > > any audio driver, we need to be able to put this code into a module. > Well, I guess the original patch description caused a big confusion. > As far as I see, the intention of the patch is not about the module or > built-in kernel. Instead it's rather to fold designware_pcm stuff > into the single designware_i2s driver. Ah, right. That'd be fine. signature.asc Description: PGP signature
Re: [alsa-devel] [PATCH] ASoC: dwc: disallow building designware_pcm as a module
On Thu, 20 Apr 2017 21:46:46 +0200, Mark Brown wrote: > > On Wed, Apr 19, 2017 at 05:48:15PM +0100, Jose Abreu wrote: > > > What do you think Mark? If you want to keep the PCM as a module > > then we will need to abstract this more, by reducing the > > dependencies. > > I think forcing this to be built in to the kernel (which is what the > commit message says the change is going to do) is an obviously bad > idea. Anything we add to the base kernel image needs to have a good > reason to be there and it is hard to think what that reason might be for > any audio driver, we need to be able to put this code into a module. Well, I guess the original patch description caused a big confusion. As far as I see, the intention of the patch is not about the module or built-in kernel. Instead it's rather to fold designware_pcm stuff into the single designware_i2s driver. The former is merely an extension of the latter driver, and the latter invokes the former directly. Thus there is little merit to keep them separate. I think the current code is even buggy, which allows to leave CONFIG_SND_DESIGNWARE_I2S=y and CONFIG_SND_DESIGNWARE_PCM=m. So, I think Lubomir's change is right. But the patch subject and description should be rephrased. One thing I don't like is the rename of the file. But in this particular case, it's unavoidable unless we rename the module name. BTW, we should drop the superfluous EXPORT_SYMBOL*(), too. thanks, Takashi
Re: [alsa-devel] [PATCH] ASoC: dwc: disallow building designware_pcm as a module
On Thu, 20 Apr 2017 21:46:46 +0200, Mark Brown wrote: > > On Wed, Apr 19, 2017 at 05:48:15PM +0100, Jose Abreu wrote: > > > What do you think Mark? If you want to keep the PCM as a module > > then we will need to abstract this more, by reducing the > > dependencies. > > I think forcing this to be built in to the kernel (which is what the > commit message says the change is going to do) is an obviously bad > idea. Anything we add to the base kernel image needs to have a good > reason to be there and it is hard to think what that reason might be for > any audio driver, we need to be able to put this code into a module. Well, I guess the original patch description caused a big confusion. As far as I see, the intention of the patch is not about the module or built-in kernel. Instead it's rather to fold designware_pcm stuff into the single designware_i2s driver. The former is merely an extension of the latter driver, and the latter invokes the former directly. Thus there is little merit to keep them separate. I think the current code is even buggy, which allows to leave CONFIG_SND_DESIGNWARE_I2S=y and CONFIG_SND_DESIGNWARE_PCM=m. So, I think Lubomir's change is right. But the patch subject and description should be rephrased. One thing I don't like is the rename of the file. But in this particular case, it's unavoidable unless we rename the module name. BTW, we should drop the superfluous EXPORT_SYMBOL*(), too. thanks, Takashi
