Re: [PATCH v5 1/3] drm/bridge: synopsys: Add audio support for dw-hdmi-qp
From: Quentin Schulz
Date: Tue Feb 04 2025 - 04:59:27 EST
Hi Detlev,
Just some drive-by comment inline.
On 2/3/25 6:16 PM, Detlev Casanova wrote:
From: Sugar Zhang <sugar.zhang@xxxxxxxxxxxxxx>
Register the dw-hdmi-qp bridge driver as an HDMI audio codec.
The register values computation functions (for n) are based on the
downstream driver, as well as the register writing functions.
The driver uses the generic HDMI Codec framework in order to implement
the HDMI audio support.
Signed-off-by: Sugar Zhang <sugar.zhang@xxxxxxxxxxxxxx>
Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@xxxxxxxxxx>
Tested-by: Quentin Schulz <quentin.schulz@xxxxxxxxx>
Signed-off-by: Detlev Casanova <detlev.casanova@xxxxxxxxxxxxx>
---
drivers/gpu/drm/bridge/synopsys/dw-hdmi-qp.c | 497 +++++++++++++++++++
1 file changed, 497 insertions(+)
diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi-qp.c b/drivers/gpu/drm/bridge/synopsys/dw-hdmi-qp.c
index b281cabfe992e..ea5d8599cb56a 100644
--- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi-qp.c
+++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi-qp.c
[...]
+static int dw_hdmi_qp_match_tmds_n_table(struct dw_hdmi_qp *hdmi,
+ unsigned long pixel_clk,
+ unsigned long freq)
+{
+ const struct dw_hdmi_audio_tmds_n *tmds_n = NULL;
+ int i;
+
+ for (i = 0; common_tmds_n_table[i].tmds != 0; i++) {
+ if (pixel_clk == common_tmds_n_table[i].tmds) {
+ tmds_n = &common_tmds_n_table[i];
+ break;
+ }
+ }
+
+ if (tmds_n == NULL)
+ return -ENOENT;
+
+ switch (freq) {
+ case 32000:
+ return tmds_n->n_32k;
+ case 44100:
+ case 88200:
+ case 176400:
+ return (freq / 44100) * tmds_n->n_44k1;
+ case 48000:
+ case 96000:
+ case 192000:
+ return (freq / 48000) * tmds_n->n_48k;
+ default:
+ return -ENOENT;
+ }
+}
+
+static u64 dw_hdmi_qp_audio_math_diff(unsigned int freq, unsigned int n,
+ unsigned int pixel_clk)
+{
+ u64 final, diff;
+ u64 cts;
+
+ final = (u64)pixel_clk * n;
+
+ cts = final;
+ do_div(cts, 128 * freq);
+
+ diff = final - (u64)cts * (128 * freq);
+
+ return diff;
I believe
final = (u64)pixel_clk * n;
do_div(cts, 128 * freq);
diff = final - (u64)cts * (128 * freq);
is just an elaborate way of doing
((u64)pixel_clk * n) % (128 * freq)
? If that's the case, then I believe do_div(a, b) actually already
returns the remainder of a by b.
If that's the case, isn't that function simply:
return do_div(mul_u32_u32(pixel_clk, n), mul_u32_u32(freq, 128))
?
We could probably replace mul_u32_u32(freq, 128) with freq * 128
considering that dw_hdmi_qp_match_tmds_n_table expects freq to be <
192000, so we won't get an overflow with that.
mul_u32_u32(pixel_clk, n) does the u32*u32->u64 without a temporary C
variable.
[...]
+static void dw_hdmi_qp_set_audio_interface(struct dw_hdmi_qp *hdmi,
+ struct hdmi_codec_daifmt *fmt,
+ struct hdmi_codec_params *hparms)
+{
+ u32 conf0 = 0;
+
+ /* Reset the audio data path of the AVP */
+ dw_hdmi_qp_write(hdmi, AVP_DATAPATH_PACKET_AUDIO_SWINIT_P, GLOBAL_SWRESET_REQUEST);
+
+ /* Disable AUDS, ACR, AUDI */
+ dw_hdmi_qp_mod(hdmi, 0,
+ PKTSCHED_ACR_TX_EN | PKTSCHED_AUDS_TX_EN | PKTSCHED_AUDI_TX_EN,
+ PKTSCHED_PKT_EN);
+
+ /* Clear the audio FIFO */
+ dw_hdmi_qp_write(hdmi, AUDIO_FIFO_CLR_P, AUDIO_INTERFACE_CONTROL0);
+
+ /* Select I2S interface as the audio source */
+ dw_hdmi_qp_mod(hdmi, AUD_IF_I2S, AUD_IF_SEL_MSK, AUDIO_INTERFACE_CONFIG0);
+
+ /* Enable the active i2s lanes */
+ switch (hparms->channels) {
+ case 7 ... 8:
+ conf0 |= I2S_LINES_EN(3);
+ fallthrough;
+ case 5 ... 6:
+ conf0 |= I2S_LINES_EN(2);
+ fallthrough;
+ case 3 ... 4:
+ conf0 |= I2S_LINES_EN(1);
+ fallthrough;
+ default:
+ conf0 |= I2S_LINES_EN(0);
+ break;
+ }
+
+ dw_hdmi_qp_mod(hdmi, conf0, I2S_LINES_EN_MSK, AUDIO_INTERFACE_CONFIG0);
+
+ /*
+ * Enable bpcuv generated internally for L-PCM, or received
+ * from stream for NLPCM/HBR.
+ */
+ switch (fmt->bit_fmt) {
+ case SNDRV_PCM_FORMAT_IEC958_SUBFRAME_LE:
+ conf0 = (hparms->channels == 8) ? AUD_HBR : AUD_ASP;
+ conf0 |= I2S_BPCUV_RCV_EN;
+ break;
+ default:
+ conf0 = AUD_ASP | I2S_BPCUV_RCV_DIS;
+ break;
+ }
+
+ dw_hdmi_qp_mod(hdmi, conf0, I2S_BPCUV_RCV_MSK | AUD_FORMAT_MSK,
+ AUDIO_INTERFACE_CONFIG0);
+
+ /* Enable audio FIFO auto clear when overflow */
+ dw_hdmi_qp_mod(hdmi, AUD_FIFO_INIT_ON_OVF_EN, AUD_FIFO_INIT_ON_OVF_MSK,
+ AUDIO_INTERFACE_CONFIG0);
This is all very I2S-centric while the HDMI controllers on RK3588 do
have the ability (according to the TRM) to use S/PDIF instead of I2S. I
assume the driver should be able to know which format to use based on
simple-audio-card,format property? Is that correct? Then current support
which doesn't even check for I2S would be fine and not conflict with a
later commit which would add support for S/PDIF? (Essentially asking if
we need another DT property for the HDMI controller node or elsewhere to
specify which mode to run in instead of expecting it to always be I2S).
Cheers,
Quentin