LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Nicolas Frattaroli <frattaroli.nicolas@gmail.com>
To: Liam Girdwood <lgirdwood@gmail.com>,
	Mark Brown <broonie@kernel.org>, Rob Herring <robh+dt@kernel.org>,
	Heiko Stuebner <heiko@sntech.de>,
	Robin Murphy <robin.murphy@arm.com>
Cc: 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 15:52:55 +0200	[thread overview]
Message-ID: <2412250.zZEsDtmPgG@archbook> (raw)
In-Reply-To: <deb50900-fcfd-23d0-ab8a-0e64b2abb063@arm.com>

On Donnerstag, 19. August 2021 14:08:36 CEST Robin Murphy wrote:
> On 2021-08-17 11:11, Nicolas Frattaroli wrote:
> > +  rockchip,trcm-sync:
> > +    $ref: /schemas/types.yaml#/definitions/uint32
> > +    description:
> > +      Which lrck/bclk clocks each direction will sync to. You should use
> > the +      constants in <dt-bindings/sound/rockchip,i2s-tdm.h>
> > +    oneOf:
> > +      - const: 0
> > +        description:
> > +          RK_TRCM_TXRX. Use both the TX and the RX clock for TX and RX.
> > +      - const: 1
> > +        description:
> > +          RK_TRCM_TX. Use only the TX clock for TX and RX.
> > +      - const: 2
> > +        description:
> > +          RK_TRCM_RX. Use only the RX clock for TX and RX.
> 
> I wonder if that might make sense to have boolean properties to describe
> the latter two cases (which would effectively be mutually-exclusive),
> rather than a magic number? Or possibly even just make the respective
> clocks optional, if this is something which would be done per-SoC rather
> than per-board?
> 

From what I know from downstream vendor device trees, these are per
board, not for the SoC as a whole. There are I2S/TDM controllers on the
SoC which I think are hardwired to certain other IP blocks, such as I2S0
being connected to HDMI, but I2S1 can be routed outside of the SoC where
these come into play I believe.

As for making them boolean properties, I'd rather not. If I were to make it
two mutually exclusive booleans, this would result in 4 possible states
rather than 3, and require complexity to check it both in the schema and
in the probe function. Like this, I can get away with a switch case that
has a fallthrough, and a list of consts in the schema.

> > +
> > +  "#sound-dai-cells":
> > +    const: 0
> > +
> > +  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.

> 
> > +
> > +  rockchip,playback-only:
> > +    description: Specify that the controller only has playback
> > capability.
> > +    type: boolean
> > +
> > +  rockchip,capture-only:
> > +    description: Specify that the controller only has capture capability.
> > +    type: boolean
> 
> Could those be inferred from the compatible string, or are there cases
> where you have multiple instances of the IP block in different
> configurations within the same SoC? (Or if it's merely reflecting
> whether the respective interface is actually wired up externally, could
> that be inferred from the attached codec?)
> 
> Robin.
> 

They can't be inferred from the SoC because there are indeed multiple
instances of this IP block in different configurations on the same SoC.
The RK3566 and RK3568 have four in total, of two different categories,
each being able to be configured for a different format (though the
number of channels and available formats vary for the two categories,
one group only supports I2S and PCM with two channels)

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.

As an example, if I understand it correctly, I2S3 on the RK3566 and
RK3568 can do 2 channels RX and TX in I2S mode, but only 2 channels
either RX or TX in PCM mode, but I'm unsure of the language in the
(still not public) documentation I have.

Regards,
Nicolas Frattaroli



  reply	other threads:[~2021-08-19 13:53 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 [this message]
2021-08-19 14:16       ` Mark Brown
2021-08-19 17:01         ` Nicolas Frattaroli
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=2412250.zZEsDtmPgG@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).