| Message ID | 20240227164659.705271-2-heiko@sntech.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add support for Theobroma-Systems Tiger SoM | expand |
Hi Heiko, On 2/27/24 17:46, Heiko Stuebner wrote: > From: Heiko Stuebner <heiko.stuebner@cherry.de> > > The rockchip,trcm-sync-tx-only property is at this time only documented > for the tdm variant of Rockchip i2s controllers. > > While there was a series [0] adding code and binding for the property, > it doesn't seem to have gone forward back in 2021. > > So for now fix the devicetree check by removing the property from rk3588 > i2s controllers until support for it gets merged. > It seems like tx-only should be supported if the dai_link has a symmetric rate, c.f. https://elixir.bootlin.com/linux/latest/source/sound/soc/rockchip/rockchip_i2s.c#L455 is doing the same as the patch from 3 years ago[1] was trying to do, only in the probe. [1] https://patchwork.kernel.org/project/linux-rockchip/patch/1629796734-4243-4-git-send-email-sugar.zhang@rock-chips.com/ Considering that the RK3588 doesn't have RX only support according to the documentation of that register.... However, I have no clue how the dai_link would get the symmetrical rate set, because the only place I could see it set is in set_link_flags if SND_SOC_TPLG_LNK_FLGBIT_SYMMETRIC_RATES if set in the flag mask and the flags, but my grep-fu returned nothing setting this flag anywhere... so maybe that's just dead code? In any case, with the current code: I2S_CKR_TRCM_TXRX is put into I2S_CKR register, regardless of that DT property. While the naming seems to suggest TX+RX should be possible, its value is 0 (offset 28) and in the TRM it says: 2'b00: Generates LRCK for TX only. Soooo... we are essentially in tx-only mode today with or without that property. Some things to fix later on :) Which made me look for the same thing for the i2s_tdm driver and the same mistake is made, though here there's no support for tx-only or rx-only according to the TRM, and it is required to write 0b01 (offset 28)... which we do by abusing the tx-only DT property which writes TRCM_TX whose value is 1 (offset 28). Considering that this is stored in clk_trcm member in the struct and that we do checks on that member to know in which situation we are (txrx, tx, rx), this seems very incorrect to me as we would configure it in the only mode it knows (txrx) but make the driver assume tx-only is selected. One more thing to fix later on :) In any case, this patch is for matching the dt-bindings so there's no reason to not merge this, so: Reviewed-by: Quentin Schulz <quentin.schulz@theobroma-systems.com> Thanks, Quentin
diff --git a/arch/arm64/boot/dts/rockchip/rk3588s.dtsi b/arch/arm64/boot/dts/rockchip/rk3588s.dtsi index 36b1b7acfe6a1..82350ddb262f2 100644 --- a/arch/arm64/boot/dts/rockchip/rk3588s.dtsi +++ b/arch/arm64/boot/dts/rockchip/rk3588s.dtsi @@ -1704,7 +1704,6 @@ i2s2_2ch: i2s@fe490000 { dmas = <&dmac1 0>, <&dmac1 1>; dma-names = "tx", "rx"; power-domains = <&power RK3588_PD_AUDIO>; - rockchip,trcm-sync-tx-only; pinctrl-names = "default"; pinctrl-0 = <&i2s2m1_lrck &i2s2m1_sclk @@ -1725,7 +1724,6 @@ i2s3_2ch: i2s@fe4a0000 { dmas = <&dmac1 2>, <&dmac1 3>; dma-names = "tx", "rx"; power-domains = <&power RK3588_PD_AUDIO>; - rockchip,trcm-sync-tx-only; pinctrl-names = "default"; pinctrl-0 = <&i2s3_lrck &i2s3_sclk