Re: [PATCH 2/4] dt-bindings: sound: add rockchip i2s-tdm binding

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel