From: Paul Kocialkowski <contact@paulk.fr>
To: Stephen Warren <swarren@wwwdotorg.org>,
Thierry Reding <thierry.reding@gmail.com>
Cc: linux-kernel@vger.kernel.org, alsa-devel@alsa-project.org,
linux-tegra@vger.kernel.org, Liam Girdwood <lgirdwood@gmail.com>,
Mark Brown <broonie@kernel.org>, Jaroslav Kysela <perex@perex.cz>,
Takashi Iwai <tiwai@suse.com>,
Alexandre Courbot <gnurou@gmail.com>,
Marcel Ziswiler <marcel.ziswiler@toradex.com>,
Rob Herring <robh@kernel.org>
Subject: Re: [PATCH 3/3] sound: ASoC: tegra: Select tegra30 i2s and ahub for tegra124 SoC
Date: Wed, 31 May 2017 18:59:08 +0200 [thread overview]
Message-ID: <1496249948.1774.3.camel@paulk.fr> (raw)
In-Reply-To: <6c308e93-7dda-96e1-1be5-64d9f9da6dfe@wwwdotorg.org>
[-- Attachment #1: Type: text/plain, Size: 9271 bytes --]
Hi,
Le lundi 08 mai 2017 à 09:58 -0600, Stephen Warren a écrit :
> On 05/07/2017 12:12 PM, Paul Kocialkowski wrote:
> > Hi,
> >
> > Le mardi 25 avril 2017 à 10:57 -0600, Stephen Warren a écrit :
> > > On 04/24/2017 12:41 PM, Paul Kocialkowski wrote:
> > > > Le lundi 24 avril 2017 à 09:35 -0600, Stephen Warren a écrit :
> > > > > On 04/24/2017 09:07 AM, Paul Kocialkowski wrote:
> > > > > > Le mercredi 19 avril 2017 à 16:00 -0600, Stephen Warren a écrit :
> > > > > > > On 04/18/2017 10:38 AM, Paul Kocialkowski wrote:
> > > > > > > > Le mardi 18 avril 2017 à 10:15 -0600, Stephen Warren a écrit :
> > > > > > > > > On 04/18/2017 09:11 AM, Paul Kocialkowski wrote:
> > > > > > > > > > This selects the tegra30 i2s and ahub controllers for the
> > > > > > > > > > tegra124
> > > > > > > > > > SoC.
> > > > > > > > > > These are needed when building without ARCH_TEGRA_3x_SOC
> > > > > > > > > > set.
> > > > > > > > > > diff --git a/sound/soc/tegra/Kconfig
> > > > > > > > > > b/sound/soc/tegra/Kconfig
> > > > > > > > > > index efbe8d4c019e..bcd18d2cf7a7 100644
> > > > > > > > > > --- a/sound/soc/tegra/Kconfig
> > > > > > > > > > +++ b/sound/soc/tegra/Kconfig
> > > > > > > > > > @@ -46,7 +46,7 @@ config SND_SOC_TEGRA20_SPDIF
> > > > > > > > > >
> > > > > > > > > > config SND_SOC_TEGRA30_AHUB
> > > > > > > > > > tristate
> > > > > > > > > > - depends on SND_SOC_TEGRA && ARCH_TEGRA_3x_SOC
> > > > > > > > > > + depends on SND_SOC_TEGRA && (ARCH_TEGRA_3x_SOC ||
> > > > > > > > > > ARCH_TEGRA_124_SOC)
> > > > > > > > >
> > > > > > > > > Is this really a compile-time dependency?
> > > > > > > >
> > > > > > > > From a quick look at the code, I doubt this is really a build
> > > > > > > > dependency.
> > > > > > > >
> > > > > > > > > If so, don't we need to add T210 and T186 entries into that ||
> > > > > > > > > condition
> > > > > > > > > too,
> > > > > > > > > since we could be building a kernel with just T210/T186
> > > > > > > > > support
> > > > > > > > > and no
> > > > > > > > > T124
> > > > > > > > > support?
> > > > > > > >
> > > > > > > > In the spirit of this patch, adding entries for other tegra
> > > > > > > > platforms
> > > > > > > > would
> > > > > > > > make
> > > > > > > > sense. Would you prefer that we leave out the dependency from
> > > > > > > > SND_SOC_TEGRA30_*
> > > > > > > > and only select the right I2S driver to use in each codec
> > > > > > > > driver?
> > > > > > > >
> > > > > > > > If not, we'd have to list all relevant platforms both in the
> > > > > > > > I2S/AHUB
> > > > > > > > drivers
> > > > > > > > and in each codec's rules (which is not necessarily and issue,
> > > > > > > > but
> > > > > > > > there's
> > > > > > > > no
> > > > > > > > need to have artificial platform dependencies).
> > > > > > > >
> > > > > > > > What do you think?
> > > > > > >
> > > > > > > I think we should just remove most of these "depends on" since
> > > > > > > they're
> > > > > > > mostly set up to reflect runtime requirements rather than build
> > > > > > > time
> > > > > > > requirements. The only points I'd make are:
> > > > > >
> > > > > > I definitely agree we should do that for all the codec Kconfig
> > > > > > options.
> > > > > >
> > > > > > > 1)
> > > > > > >
> > > > > > > Everything should "depends on SND_SOC_TEGRA" simply so the options
> > > > > > > don't
> > > > > > > show up and clutter menuconfig menus unless SND_SOC_TEGRA is
> > > > > > > enabled.
> > > > > >
> > > > > > Agreed.
> > > > > >
> > > > > > > 2)
> > > > > > >
> > > > > > > SND_SOC_TEGRA30_I2S does need the Tegra30 AHUB driver in order to
> > > > > > > compile/link, since it directly calls functions in that driver.
> > > > > > > This
> > > > > > > is
> > > > > > > already handled by SND_SOC_TEGRA30_I2S doing "select
> > > > > > > SND_SOC_TEGRA30_AHUB".
> > > > > >
> > > > > > Agreed.
> > > > > >
> > > > > > > 3)
> > > > > > >
> > > > > > > The machine drivers all do e.g. "select SND_SOC_TEGRA30_I2S if
> > > > > > > ARCH_TEGRA_3x_SOC". This was an attempt to make the machine
> > > > > > > drivers
> > > > > > > only
> > > > > > > pull in the relevant drivers for the SoC(s) being compiled for.
> > > > > > > I'm
> > > > > > > not
> > > > > > > sure this still makes sense; this won't work on kernels that only
> > > > > > > support T124/T210/T186 since ARCH_TEGRA_3x_SOC isn't enabled then.
> > > > > > > Should we just remove all those and make sure the defconfigs are
> > > > > > > updated
> > > > > > > to make sure the relevant I2S/AHUB/SPDIF/AC97 drivers are
> > > > > > > explicitly
> > > > > > > enabled? Perhaps we should default all the I2S/AHUB/SPDIF/AC97 to
> > > > > > > y
> > > > > > > (which will only apply if SND_SOC_TEGRA is enabled)?
> > > > > >
> > > > > > I think it would be easier for everyone to just auto-select the
> > > > > > machine
> > > > > > drivers
> > > > > > automatically based on the architecture (so we could have the list
> > > > > > of
> > > > > > ARCH_TEGRA_*_SOC here) when SND_SOC_TEGRA is selected.
> > > > >
> > > > > I don't think selecting the machine drivers is the correct approach,
> > > > > since then they can't be disabled.
> > > > >
> > > > > Making certain machine drivers "default y if ARCH_TEGRA_nn_SOC" would
> > > > > address that,
> > > >
> > > > That's right, my mistake. Let's take that as the solution I'm backing
> > > > then.
> > > >
> > > > > but still isn't very scalable since we need to go back and
> > > > > edit the Kconfig every time we define a new SoC, in order to add that
> > > > > SoC into the default statement.
> > > >
> > > > Well, that's what platform bringup is all about, isn't it? I think it
> > > > makes
> > > > a
> > > > lot more sense to have to add a new platform once (and it's not like one
> > > > will
> > > > forget to look at the sound part when adding a new platform) rather than
> > > > requiring users to hand-pick the option.
> > >
> > > Personally I'd expect all the Tegra machine drivers to be enabled in the
> > > upstream defconfig files all the time, such that we don't need to make
> > > the Kconfig options default y, nor have users manually turn the relevant
> > > options on, at all.
> >
> > I think I was previously mistaken about what the machine drivers are, so I
> > got a
> > good occasion to find out about them an learn something new. Thanks!
> >
> > Agreed regarding the machine drivers, they should indeed all be enabled in
> > the
> > defconfig by default.
> >
> > > > > > Not only does this preserve existing configs (including external
> > > > > > ones
> > > > > > that
> > > > > > aren't part of the kernel tree), it also clearly maps which machine
> > > > > > driver
> > > > > > to use for which SoC instead of having users do it by hand.
> > > > >
> > > > > The machine drivers aren't terribly tied to SoCs by design; most of
> > > > > them
> > > > > would work on pretty much any SoC. They're only tied to SoCs as a
> > > > > side-effect of a machine driver being tied to a certain CODEC, and
> > > > > certain CODECS just by chance are only used (so far) on specific
> > > > > boards,
> > > > > which have specific SoCs.
> > > >
> > > > I'm a bit confused: aren't the machine driver (i2s/ahub/spdif/ac97) tied
> > > > to
> > > > specific hardware blocks that are found in specific SoCs and not in
> > > > others?
> > > > I
> > > > can see these blocks haven't evolved much across generations, but
> > > > they're
> > > > still
> > > > either part of a specific SoC or not, aren't they?
> > > >
> > > > The compatible strings in the common SoC dts seem to indicate that only
> > > > one
> > > > of
> > > > these blocks is found at a time.
> > >
> > > There are SoC-specific drivers for the components (Tegra20 I2S, Tegra30
> > > I2S, etc.), but the *machine* drivers generally don't know anything
> > > about that. DT instantiates the correct component drivers for the SoC,
> > > and the machine driver just uses whatever it's linked to without caring
> > > too-much/at-all about whether it's hooked to a Tegra20 I2S or a Tegra30
> > > I2S device.
> >
> > That makes sense now. Regarding the SoC-specific components, would you agree
> > with auto-selecting those based on ARCH_TEGRA_xx_SOC so that only the
> > required
> > drivers are pulled-in? Otherwise, I'm also fine with explicitely enabling
> > them
> > in the defconfig at this point.
>
> I believe we don't generally "select" Kconfig options that aren't
> absolutely required for basic system operation, since we don't want to
> select stuff that's optional. Enabling the options in defconfig seems
> like the right approach, or perhaps "default y" (in which case anyone
> who doesn't want them can simply turn them off).
Understood. I guess I'll (finally) craft a patch that leaves it up to users to
enable these in the config and add them to the tegra defconfig.
--
Paul Kocialkowski, developer of free digital technology and hardware support
Website: https://www.paulk.fr/
Coding blog: https://code.paulk.fr/
Git repositories: https://git.paulk.fr/ https://git.code.paulk.fr/
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
prev parent reply other threads:[~2017-05-31 17:00 UTC|newest]
Thread overview: 11+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-04-18 15:11 [PATCH 3/3] sound: ASoC: tegra: Select tegra30 i2s and ahub for tegra124 SoC Paul Kocialkowski
2017-04-18 16:15 ` Stephen Warren
2017-04-18 16:38 ` Paul Kocialkowski
2017-04-19 22:00 ` Stephen Warren
2017-04-24 15:07 ` Paul Kocialkowski
2017-04-24 15:35 ` Stephen Warren
2017-04-24 18:41 ` Paul Kocialkowski
2017-04-25 16:57 ` Stephen Warren
2017-05-07 18:12 ` Paul Kocialkowski
2017-05-08 15:58 ` Stephen Warren
2017-05-31 16:59 ` Paul Kocialkowski [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=1496249948.1774.3.camel@paulk.fr \
--to=contact@paulk.fr \
--cc=alsa-devel@alsa-project.org \
--cc=broonie@kernel.org \
--cc=gnurou@gmail.com \
--cc=lgirdwood@gmail.com \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-tegra@vger.kernel.org \
--cc=marcel.ziswiler@toradex.com \
--cc=perex@perex.cz \
--cc=robh@kernel.org \
--cc=swarren@wwwdotorg.org \
--cc=thierry.reding@gmail.com \
--cc=tiwai@suse.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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).