| Message ID | 1414512524-24466-4-git-send-email-thomas.petazzoni@free-electrons.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Oct 28, 2014 at 05:08:41PM +0100, Thomas Petazzoni wrote: > Now that the Armada 370 DB audio complex is fully described in the > Device Tree using the simple-card DT binding, the armada-370-db audio > machine driver can be removed. This is just removing support for the old binding which is incompatible...
Dear Mark Brown, On Tue, 28 Oct 2014 22:35:21 +0000, Mark Brown wrote: > On Tue, Oct 28, 2014 at 05:08:41PM +0100, Thomas Petazzoni wrote: > > Now that the Armada 370 DB audio complex is fully described in the > > Device Tree using the simple-card DT binding, the armada-370-db audio > > machine driver can be removed. > > This is just removing support for the old binding which is > incompatible... Hum, I'm not sure to follow you here. In a subsequent patch, I change the Armada 370 DB audio complex DT description to use the simple-audio-card DT binding, which makes the Armada 370 DB audio machine driver irrelevant. Of course, this means that if someone uses an old Armada 370 DB Device Tree with a new kernel, it will no longer. But I believe this is kind of expected for this specific case: when we originally introduced the Armada 370 DB audio support, we knew a proper DT binding to describe sound complex was arriving, and therefore the Armada 370 DB audio machine driver was only a temporary solution until the pure DT solution was available. Therefore, with the agreement of the mvebu maintainers, I'd like to be allowed to break the DT backward compatibility here, and get rid of this audio machine driver which would otherwise have no users left. Thanks! Thomas
On Tue, Oct 28, 2014 at 11:54:56PM +0100, Thomas Petazzoni wrote: > On Tue, 28 Oct 2014 22:35:21 +0000, Mark Brown wrote: > > This is just removing support for the old binding which is > > incompatible... > Hum, I'm not sure to follow you here. In a subsequent patch, I change > the Armada 370 DB audio complex DT description to use the > simple-audio-card DT binding, which makes the Armada 370 DB audio > machine driver irrelevant. > Of course, this means that if someone uses an old Armada 370 DB Device > Tree with a new kernel, it will no longer. But I believe this is kind Yes, this is the entire point of device tree as an ABI. We also need to care about out of tree users. > of expected for this specific case: when we originally introduced the > Armada 370 DB audio support, we knew a proper DT binding to describe > sound complex was arriving, and therefore the Armada 370 DB audio > machine driver was only a temporary solution until the pure DT solution > was available. No, that's not the case - these drivers predate DT IIRC and while it's good to avoid adding new drivers there's nothing inherently bad about having a machine driver or adaption layer into simple card (you could do this just as platform data for simple card for many devices). In this particular case I'm especially worried since we've got the whole thing with not having a good story for supporting simulataneous use of the S/PDIF and I2S links worked out yet on the controller side and on the simple-card side it's pretty much just the most basic CPUs that are supported. > Therefore, with the agreement of the mvebu maintainers, I'd like to be > allowed to break the DT backward compatibility here, and get rid of > this audio machine driver which would otherwise have no users left. ...in mainline. Doesn't this hardware tend to have lots of small variants on the design floating around?
Mark, On Tue, 28 Oct 2014 23:07:40 +0000, Mark Brown wrote: > > Hum, I'm not sure to follow you here. In a subsequent patch, I change > > the Armada 370 DB audio complex DT description to use the > > simple-audio-card DT binding, which makes the Armada 370 DB audio > > machine driver irrelevant. > > > Of course, this means that if someone uses an old Armada 370 DB Device > > Tree with a new kernel, it will no longer. But I believe this is kind > > Yes, this is the entire point of device tree as an ABI. We also need to > care about out of tree users. Right, but I believe it has been discussed multiple times that when a subsystem doesn't yet have the proper generic DT bindings and that temporary solutions are used, we are allowed to break the compatibility when moving to the proper generic DT bindings. Another example is a platform moving from clocks defined in the platform code to clocks defined in the Device Tree and using the common clock framework. Should we keep around the code defining the clocks in the platform code forever, just because old Device Tree didn't carry the clock description? > > of expected for this specific case: when we originally introduced the > > Armada 370 DB audio support, we knew a proper DT binding to describe > > sound complex was arriving, and therefore the Armada 370 DB audio > > machine driver was only a temporary solution until the pure DT solution > > was available. > > No, that's not the case - these drivers predate DT IIRC and while it's No, the armada-370-db driver does not predate DT. The mach-mvebu platforms started to be supported in kernel 3.6, in full DT mode from the beginning. The armada-370-db audio machine driver was added in 3.15, and is *only* used for the Marvell Armada 370 DB platform, which is only described in DT. In addition, the Marvell Armada 370 DB board is an internal evaluation board that is not publicly available. So the rules in terms of DT backward compatibility should I believe be a bit relaxed for such platforms. > good to avoid adding new drivers there's nothing inherently bad about > having a machine driver or adaption layer into simple card (you could do > this just as platform data for simple card for many devices). I'm sorry but I don't understand what you mean: the simple card DT binding is completely sufficient, in its current state, to describe the audio complex of the Armada 370 DB. > In this particular case I'm especially worried since we've got the whole > thing with not having a good story for supporting simulataneous use of > the S/PDIF and I2S links worked out yet on the controller side and on > the simple-card side it's pretty much just the most basic CPUs that are > supported. I agree that the simple card binding may not be sufficient for all situations, but in the case of this platform, it is sufficient. The audio data gets sent simultaneously to the codec and the S/PDIF transceiver, with no way of enabling or disabling those outputs independently. So I believe the simple card DT binding is good enough for this case. > > Therefore, with the agreement of the mvebu maintainers, I'd like to be > > allowed to break the DT backward compatibility here, and get rid of > > this audio machine driver which would otherwise have no users left. > > ...in mainline. Doesn't this hardware tend to have lots of small > variants on the design floating around? Again, the armada-370-db driver is a *board* specific driver, that works just and only for the Armada 370 DB board. Other boards may have a different codec, maybe no S/PDIF transceivers, etc. So any "variant" on the design would anyway have to use a different driver. All that being said, if you really want to keep the armada-370-db audio machine driver around that's fine with me. It's going to be just an unused piece of code and therefore a piece of code that will probably be broken in the near future, but I'm not the ASoC maintainer, so it's your call. Best regards, Thomas
On Wed, Oct 29, 2014 at 09:24:19AM +0100, Thomas Petazzoni wrote: > On Tue, 28 Oct 2014 23:07:40 +0000, Mark Brown wrote: > > Yes, this is the entire point of device tree as an ABI. We also need to > > care about out of tree users. > Right, but I believe it has been discussed multiple times that when a > subsystem doesn't yet have the proper generic DT bindings and that > temporary solutions are used, we are allowed to break the compatibility > when moving to the proper generic DT bindings. *sigh* As has been discussed *repeatedly* audio system integration is itself frequently intersting hardware and "proper" "generic" bindings are not a useful goal for a large class of systems, we need to apply some taste in describing things in DT - it's the same problem we've got with people just constantly dumping Linux implementation details into DT really. > > good to avoid adding new drivers there's nothing inherently bad about > > having a machine driver or adaption layer into simple card (you could do > > this just as platform data for simple card for many devices). > I'm sorry but I don't understand what you mean: the simple card DT > binding is completely sufficient, in its current state, to describe the > audio complex of the Armada 370 DB. That's nice but like I just said that still doesn't mean that there's anything inherently bad about a machine driver. > In addition, the Marvell Armada 370 DB board is an internal evaluation > board that is not publicly available. So the rules in terms of DT > backward compatibility should I believe be a bit relaxed for such > platforms. Which you're absolutely sure will never have been used as a reference design or otherwise cloned? The board you have might not be widely available but that doesn't mean other people aren't using boards with a similar design. > > In this particular case I'm especially worried since we've got the whole > > thing with not having a good story for supporting simulataneous use of > > the S/PDIF and I2S links worked out yet on the controller side and on > > the simple-card side it's pretty much just the most basic CPUs that are > > supported. > I agree that the simple card binding may not be sufficient for all > situations, but in the case of this platform, it is sufficient. The > audio data gets sent simultaneously to the codec and the S/PDIF > transceiver, with no way of enabling or disabling those outputs > independently. So I believe the simple card DT binding is good enough > for this case. You're looking at the current driver implementation, there are actually separate enable bits for I2S and S/PDIF in the hardware. > All that being said, if you really want to keep the armada-370-db audio > machine driver around that's fine with me. It's going to be just an > unused piece of code and therefore a piece of code that will probably > be broken in the near future, but I'm not the ASoC maintainer, so it's > your call. What I'm seeing is a patch that just totally ignores DT compatibility, there is no reference to ABI stability in the changelog and no effort to maintain it in the code. If you're sending patches like that you should expect them not to be applied, DT stability is something we're trying to maintian. Patches breaking compatibility need to both justify the benefits of the break and explain why this isn't going to hurt users.
diff --git a/sound/soc/kirkwood/Kconfig b/sound/soc/kirkwood/Kconfig index 132bb83..c1b9822 100644 --- a/sound/soc/kirkwood/Kconfig +++ b/sound/soc/kirkwood/Kconfig @@ -3,15 +3,4 @@ config SND_KIRKWOOD_SOC depends on ARCH_DOVE || ARCH_MVEBU || COMPILE_TEST help Say Y or M if you want to add support for codecs attached to - the Kirkwood I2S interface. You will also need to select the - audio interfaces to support below. - -config SND_KIRKWOOD_SOC_ARMADA370_DB - tristate "SoC Audio support for Armada 370 DB" - depends on SND_KIRKWOOD_SOC && (ARCH_MVEBU || COMPILE_TEST) && I2C - select SND_SOC_CS42L51 - select SND_SOC_SPDIF - help - Say Y if you want to add support for SoC audio on - the Armada 370 Development Board. - + the Kirkwood I2S interface. diff --git a/sound/soc/kirkwood/Makefile b/sound/soc/kirkwood/Makefile index c36b03d..8cff64e 100644 --- a/sound/soc/kirkwood/Makefile +++ b/sound/soc/kirkwood/Makefile @@ -1,7 +1,3 @@ snd-soc-kirkwood-objs := kirkwood-dma.o kirkwood-i2s.o obj-$(CONFIG_SND_KIRKWOOD_SOC) += snd-soc-kirkwood.o - -snd-soc-armada-370-db-objs := armada-370-db.o - -obj-$(CONFIG_SND_KIRKWOOD_SOC_ARMADA370_DB) += snd-soc-armada-370-db.o diff --git a/sound/soc/kirkwood/armada-370-db.c b/sound/soc/kirkwood/armada-370-db.c deleted file mode 100644 index c443338..0000000 --- a/sound/soc/kirkwood/armada-370-db.c +++ /dev/null @@ -1,148 +0,0 @@ -/* - * Copyright (C) 2014 Marvell - * - * Thomas Petazzoni <thomas.petazzoni@free-electrons.com> - * - * This program is free software; you can redistribute it and/or - * modify it under the terms of the GNU General Public License as - * published by the Free Software Foundation; either version 2 of the - * License, or (at your option) any later version. - */ - -#include <linux/module.h> -#include <linux/moduleparam.h> -#include <linux/interrupt.h> -#include <linux/platform_device.h> -#include <linux/slab.h> -#include <sound/soc.h> -#include <linux/of.h> -#include <linux/platform_data/asoc-kirkwood.h> -#include "../codecs/cs42l51.h" - -static int a370db_hw_params(struct snd_pcm_substream *substream, - struct snd_pcm_hw_params *params) -{ - struct snd_soc_pcm_runtime *rtd = substream->private_data; - struct snd_soc_dai *codec_dai = rtd->codec_dai; - unsigned int freq; - - switch (params_rate(params)) { - default: - case 44100: - freq = 11289600; - break; - case 48000: - freq = 12288000; - break; - case 96000: - freq = 24576000; - break; - } - - return snd_soc_dai_set_sysclk(codec_dai, 0, freq, SND_SOC_CLOCK_IN); -} - -static struct snd_soc_ops a370db_ops = { - .hw_params = a370db_hw_params, -}; - -static const struct snd_soc_dapm_widget a370db_dapm_widgets[] = { - SND_SOC_DAPM_HP("Out Jack", NULL), - SND_SOC_DAPM_LINE("In Jack", NULL), -}; - -static const struct snd_soc_dapm_route a370db_route[] = { - { "Out Jack", NULL, "HPL" }, - { "Out Jack", NULL, "HPR" }, - { "AIN1L", NULL, "In Jack" }, - { "AIN1L", NULL, "In Jack" }, -}; - -static struct snd_soc_dai_link a370db_dai[] = { -{ - .name = "CS42L51", - .stream_name = "analog", - .cpu_dai_name = "i2s", - .codec_dai_name = "cs42l51-hifi", - .dai_fmt = SND_SOC_DAIFMT_I2S | SND_SOC_DAIFMT_CBS_CFS, - .ops = &a370db_ops, -}, -{ - .name = "S/PDIF out", - .stream_name = "spdif-out", - .cpu_dai_name = "spdif", - .codec_dai_name = "dit-hifi", - .dai_fmt = SND_SOC_DAIFMT_I2S | SND_SOC_DAIFMT_CBS_CFS, -}, -{ - .name = "S/PDIF in", - .stream_name = "spdif-in", - .cpu_dai_name = "spdif", - .codec_dai_name = "dir-hifi", - .dai_fmt = SND_SOC_DAIFMT_I2S | SND_SOC_DAIFMT_CBS_CFS, -}, -}; - -static struct snd_soc_card a370db = { - .name = "a370db", - .owner = THIS_MODULE, - .dai_link = a370db_dai, - .num_links = ARRAY_SIZE(a370db_dai), - .dapm_widgets = a370db_dapm_widgets, - .num_dapm_widgets = ARRAY_SIZE(a370db_dapm_widgets), - .dapm_routes = a370db_route, - .num_dapm_routes = ARRAY_SIZE(a370db_route), -}; - -static int a370db_probe(struct platform_device *pdev) -{ - struct snd_soc_card *card = &a370db; - - card->dev = &pdev->dev; - - a370db_dai[0].cpu_of_node = - of_parse_phandle(pdev->dev.of_node, - "marvell,audio-controller", 0); - a370db_dai[0].platform_of_node = a370db_dai[0].cpu_of_node; - - a370db_dai[0].codec_of_node = - of_parse_phandle(pdev->dev.of_node, - "marvell,audio-codec", 0); - - a370db_dai[1].cpu_of_node = a370db_dai[0].cpu_of_node; - a370db_dai[1].platform_of_node = a370db_dai[0].cpu_of_node; - - a370db_dai[1].codec_of_node = - of_parse_phandle(pdev->dev.of_node, - "marvell,audio-codec", 1); - - a370db_dai[2].cpu_of_node = a370db_dai[0].cpu_of_node; - a370db_dai[2].platform_of_node = a370db_dai[0].cpu_of_node; - - a370db_dai[2].codec_of_node = - of_parse_phandle(pdev->dev.of_node, - "marvell,audio-codec", 2); - - return devm_snd_soc_register_card(card->dev, card); -} - -static const struct of_device_id a370db_dt_ids[] = { - { .compatible = "marvell,a370db-audio" }, - { }, -}; - -static struct platform_driver a370db_driver = { - .driver = { - .name = "a370db-audio", - .owner = THIS_MODULE, - .of_match_table = of_match_ptr(a370db_dt_ids), - }, - .probe = a370db_probe, -}; - -module_platform_driver(a370db_driver); - -MODULE_AUTHOR("Thomas Petazzoni <thomas.petazzoni@free-electrons.com>"); -MODULE_DESCRIPTION("ALSA SoC a370db audio client"); -MODULE_LICENSE("GPL"); -MODULE_ALIAS("platform:a370db-audio");
Now that the Armada 370 DB audio complex is fully described in the Device Tree using the simple-card DT binding, the armada-370-db audio machine driver can be removed. Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com> --- sound/soc/kirkwood/Kconfig | 13 +--- sound/soc/kirkwood/Makefile | 4 - sound/soc/kirkwood/armada-370-db.c | 148 ------------------------------------- 3 files changed, 1 insertion(+), 164 deletions(-) delete mode 100644 sound/soc/kirkwood/armada-370-db.c