LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Nicolas Frattaroli <frattaroli.nicolas@gmail.com>
To: Mark Brown <broonie@kernel.org>
Cc: Liam Girdwood <lgirdwood@gmail.com>,
	Rob Herring <robh+dt@kernel.org>,
	Heiko Stuebner <heiko@sntech.de>,
	Robin Murphy <robin.murphy@arm.com>,
	alsa-devel@alsa-project.org, devicetree@vger.kernel.org,
	linux-arm-kernel@lists.infradead.org,
	linux-rockchip@lists.infradead.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH 2/4] dt-bindings: sound: add rockchip i2s-tdm binding
Date: Thu, 19 Aug 2021 19:01:25 +0200	[thread overview]
Message-ID: <1870492.5CqhBlFY90@archbook> (raw)
In-Reply-To: <20210819141617.GM4177@sirena.org.uk>

On Donnerstag, 19. August 2021 16:16:17 CEST Mark Brown wrote:
> On Thu, Aug 19, 2021 at 03:52:55PM +0200, Nicolas Frattaroli wrote:
> > On Donnerstag, 19. August 2021 14:08:36 CEST Robin Murphy wrote:
> > > > +  rockchip,no-dmaengine:
> > > > +    description:
> > > > +      If present, driver will not register a pcm dmaengine, only the
> > > > dai.
> > > > +      If the dai is part of multi-dais, the property should be
> > > > present.
> > > > +    type: boolean
> > > 
> > > That sounds a lot more like a policy decision specific to the Linux
> > > driver implementation, than something which really belongs in DT as a
> > > description of the platform.
> > 
> > I agree. Should I be refactoring this into a module parameter or
> > something along those lines? I'm unsure of where this goes.
> 
> Why is this even required?  What is "multi-dais" and why would
> registering the DMA stuff cause a problem?

After some research, it appears that multi-dais is a special driver that
downstream uses to allow multiple sub-DAIs to be combined into one DAI
that has all the channels of the sub-DAIs. This does not seem like
something that should be done at that level to me, because it seems
like it's pushing a sound driver configuration into the realm of
hardware description.

In retrospect, I should have stripped this out before submitting it,
because I should not be submitting things I don't understand completely.
I apologise.

> > The particular configuration may even vary per-board; an I2S/TDM
> > controller may be connected to an external codec which does not
> > support capture, whereas on another board it may be connected to
> > one that does.
> 
> If the external device doesn't support both directions then why does the
> driver for the I2S controller in the CPU care?  The constraint handling
> code in the core will ensure that nothing tries to start something that
> isn't supported

I went over the downstream text binding description again and from that
it appears that the playback/capture-only capability is something
specific to the controller, not to any device connected to it.

The downstream device tree for the rk3568 specifies playback-only for
I2S0, a.k.a. the one connected to the HDMI that I can't test because
we currently don't have a video clock. Another downstream device tree,
specific to what appears to be a robot demo for the px30 SoC, uses this
property on i2s1, which tells me that this is not an actual description
of the controller hardware but just a description of the application.

While not relevant to the device tree schema, the driver reacts to these
properties by setting the opposite directions _minimum_ channel number
to 0 (from the default of 2.)

My conclusion from this is that this reeks of nonsense and I will look
into what happens when I simply remove these properties and lower the
channels_min to 0 in the driver. If it turns out that on some SoC for
some I2S/TDM controller instance there is a limitation where specifying
that the controller only handles either capture or playback does make
sense, we can always add it later.

Thank you for your comments,
Nicolas Frattaroli



  reply	other threads:[~2021-08-19 17:01 UTC|newest]

Thread overview: 12+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-08-17 10:11 [PATCH 0/4] Rockchip I2S/TDM controller Nicolas Frattaroli
2021-08-17 10:11 ` [PATCH 1/4] ASoC: rockchip: add support for i2s-tdm controller Nicolas Frattaroli
2021-08-17 10:11 ` [PATCH 2/4] dt-bindings: sound: add rockchip i2s-tdm binding Nicolas Frattaroli
2021-08-17 13:39   ` kernel test robot
2021-08-18 16:44   ` Rob Herring
2021-08-19 12:08   ` Robin Murphy
2021-08-19 13:52     ` Nicolas Frattaroli
2021-08-19 14:16       ` Mark Brown
2021-08-19 17:01         ` Nicolas Frattaroli [this message]
2021-08-19 17:02       ` Robin Murphy
2021-08-17 10:11 ` [PATCH 3/4] arm64: dts: rockchip: add i2s1 on rk356x Nicolas Frattaroli
2021-08-17 10:11 ` [PATCH 4/4] arm64: dts: rockchip: add analog audio on Quartz64 Nicolas Frattaroli

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=1870492.5CqhBlFY90@archbook \
    --to=frattaroli.nicolas@gmail.com \
    --cc=alsa-devel@alsa-project.org \
    --cc=broonie@kernel.org \
    --cc=devicetree@vger.kernel.org \
    --cc=heiko@sntech.de \
    --cc=lgirdwood@gmail.com \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-rockchip@lists.infradead.org \
    --cc=robh+dt@kernel.org \
    --cc=robin.murphy@arm.com \
    --subject='Re: [PATCH 2/4] dt-bindings: sound: add rockchip i2s-tdm binding' \
    /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

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).