linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] ASoC: tegra20-spdif: remove "default m"
@ 2020-09-26 19:42 Michał Mirosław
  2020-09-28  8:10 ` Thierry Reding
  0 siblings, 1 reply; 3+ messages in thread
From: Michał Mirosław @ 2020-09-26 19:42 UTC (permalink / raw)
  To: Jaroslav Kysela, Takashi Iwai, Thierry Reding, Jonathan Hunter,
	Stephen Warren
  Cc: alsa-devel, linux-tegra, linux-kernel

Make tegra20-spdif default to N as all other drivers do.

Signed-off-by: Michał Mirosław <mirq-linux@rere.qmqm.pl>
Fixes: 774fec338bfc ("ASoC: Tegra: Implement SPDIF CPU DAI")
---
 sound/soc/tegra/Kconfig | 1 -
 1 file changed, 1 deletion(-)

diff --git a/sound/soc/tegra/Kconfig b/sound/soc/tegra/Kconfig
index 3d91bd3e59cd..a62cc87551ac 100644
--- a/sound/soc/tegra/Kconfig
+++ b/sound/soc/tegra/Kconfig
@@ -39,7 +39,6 @@ config SND_SOC_TEGRA20_I2S
 config SND_SOC_TEGRA20_SPDIF
 	tristate "Tegra20 SPDIF interface"
 	depends on SND_SOC_TEGRA
-	default m
 	help
 	  Say Y or M if you want to add support for the Tegra20 SPDIF interface.
 	  You will also need to select the individual machine drivers to support
-- 
2.20.1


^ permalink raw reply related	[flat|nested] 3+ messages in thread

* Re: [PATCH] ASoC: tegra20-spdif: remove "default m"
  2020-09-26 19:42 [PATCH] ASoC: tegra20-spdif: remove "default m" Michał Mirosław
@ 2020-09-28  8:10 ` Thierry Reding
  2020-09-28 18:52   ` Michał Mirosław
  0 siblings, 1 reply; 3+ messages in thread
From: Thierry Reding @ 2020-09-28  8:10 UTC (permalink / raw)
  To: Michał Mirosław
  Cc: Jaroslav Kysela, Takashi Iwai, Jonathan Hunter, Stephen Warren,
	alsa-devel, linux-tegra, linux-kernel

[-- Attachment #1: Type: text/plain, Size: 2089 bytes --]

On Sat, Sep 26, 2020 at 09:42:40PM +0200, Michał Mirosław wrote:
> Make tegra20-spdif default to N as all other drivers do.
> 
> Signed-off-by: Michał Mirosław <mirq-linux@rere.qmqm.pl>
> Fixes: 774fec338bfc ("ASoC: Tegra: Implement SPDIF CPU DAI")

I don't think this is warranted. This doesn't fix a bug or anything.
It's merely a change in the default configuration. The presence of a
Fixes: tag is typically used as a hint for people to pick this up into
stable releases, but I don't think this qualifies.

> ---
>  sound/soc/tegra/Kconfig | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/sound/soc/tegra/Kconfig b/sound/soc/tegra/Kconfig
> index 3d91bd3e59cd..a62cc87551ac 100644
> --- a/sound/soc/tegra/Kconfig
> +++ b/sound/soc/tegra/Kconfig
> @@ -39,7 +39,6 @@ config SND_SOC_TEGRA20_I2S
>  config SND_SOC_TEGRA20_SPDIF
>  	tristate "Tegra20 SPDIF interface"
>  	depends on SND_SOC_TEGRA
> -	default m
>  	help
>  	  Say Y or M if you want to add support for the Tegra20 SPDIF interface.
>  	  You will also need to select the individual machine drivers to support

So now by default this driver will be disabled, which means that Linux
is going to regress for people that rely on this driver.

You need to at least follow this up with a patch that makes the
corresponding change in both tegra_defconfig and multi_v7_defconfig to
ensure that this driver is going to get built by default.

Given the above it's probably also a good idea to explain a bit more in
the commit message about what you're trying to achieve. Yes, "default n"
is usually the right thing to do and I'm honestly not sure why Stephen
chose to make this "default m" back in the day. Given that it depends on
SND_SOC_TEGRA, which itself is "default n", I think this makes some
sense, even if in retrospect it ended up being a bit inconsistent (you
could probably argue that all patches after this are the ones that were
inconsistent instead). This was merged over 9 years ago and a lot of
common practices have changed over that period of time.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [PATCH] ASoC: tegra20-spdif: remove "default m"
  2020-09-28  8:10 ` Thierry Reding
@ 2020-09-28 18:52   ` Michał Mirosław
  0 siblings, 0 replies; 3+ messages in thread
From: Michał Mirosław @ 2020-09-28 18:52 UTC (permalink / raw)
  To: Thierry Reding
  Cc: Jaroslav Kysela, Takashi Iwai, Jonathan Hunter, Stephen Warren,
	alsa-devel, linux-tegra, linux-kernel

On Mon, Sep 28, 2020 at 10:10:26AM +0200, Thierry Reding wrote:
> On Sat, Sep 26, 2020 at 09:42:40PM +0200, Michał Mirosław wrote:
> > Make tegra20-spdif default to N as all other drivers do.
> > 
> > Signed-off-by: Michał Mirosław <mirq-linux@rere.qmqm.pl>
> > Fixes: 774fec338bfc ("ASoC: Tegra: Implement SPDIF CPU DAI")
> 
> I don't think this is warranted. This doesn't fix a bug or anything.
> It's merely a change in the default configuration. The presence of a
> Fixes: tag is typically used as a hint for people to pick this up into
> stable releases, but I don't think this qualifies.
[...]

Fixes is just for pointing where the bug or issue originated.  I usually
include it to help you -- the reviewer -- and backporters if they ever
want to use this patch. It is not specific to stable-directed patches.

For stable candidates there is 'Cc: stable' tag (no need for this patch).

> So now by default this driver will be disabled, which means that Linux
> is going to regress for people that rely on this driver.

The bug is that this driver (and only this driver in the whole
sound/soc/tegra directory) defaults to m, where all other drivers default
to n (as the policy aboud drivers seems to be [1]). This won't affect
oldconfig, allyesconfig nor allmodconfig, but will not be selected now
for clean builds - meaning less work for those not building for Tegra2.

[1] https://lkml.org/lkml/2017/11/18/257

> You need to at least follow this up with a patch that makes the
> corresponding change in both tegra_defconfig and multi_v7_defconfig to
> ensure that this driver is going to get built by default.

This I can do. Not all such drivers are enabled, though: eg. AHUB driver
is not. Maybe we need bigger refresh of the defconfigs instead?

> Given the above it's probably also a good idea to explain a bit more in
> the commit message about what you're trying to achieve. Yes, "default n"
> is usually the right thing to do and I'm honestly not sure why Stephen
> chose to make this "default m" back in the day. Given that it depends on
> SND_SOC_TEGRA, which itself is "default n", I think this makes some
> sense, even if in retrospect it ended up being a bit inconsistent (you
> could probably argue that all patches after this are the ones that were
> inconsistent instead). This was merged over 9 years ago and a lot of
> common practices have changed over that period of time.

Yes, this is a cleanup. :-)

Best Regards
Michał Mirosław

^ permalink raw reply	[flat|nested] 3+ messages in thread

end of thread, other threads:[~2020-09-28 18:52 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-09-26 19:42 [PATCH] ASoC: tegra20-spdif: remove "default m" Michał Mirosław
2020-09-28  8:10 ` Thierry Reding
2020-09-28 18:52   ` Michał Mirosław

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).