[PATCH v5 0/6] drm/bridge: sii902x: HDMI-audio support and some fixes

[PATCH v5 0/6] drm/bridge: sii902x: HDMI-audio support and some fixes

[Jyri Sarha]

The sil,i2s-data-lanes property is still under negotiation. The
question really is if there could be a generic audio i2s property to
handle that. alsa_devel added to the list of recipients.

Changes since v3:
- Change i2s-data-lanes to sil,i2s-data-lanes

The already reviewed patches are in front. Not reviewed still:
- "dt-bindings: display: sii902x: Add HDMI audio bindings"
- "drm/bridge: sii902x: Implement HDMI audio support"

The previous round:
https://patchwork.kernel.org/cover/10852643/

Jyri Sarha (5):
  drm/bridge: sii902x: Set output mode to HDMI or DVI according to EDID
  drm/bridge: sii902x: pixel clock unit is 10kHz instead of 1kHz
  dt-bindings: display: sii902x: Remove trailing white space
  dt-bindings: display: sii902x: Add HDMI audio bindings
  drm/bridge: sii902x: Implement HDMI audio support

Tomi Valkeinen (1):
  drm/bridge: sii902x: add input_bus_flags

 .../bindings/display/bridge/sii902x.txt       |  35 +-
 drivers/gpu/drm/bridge/sii902x.c              | 478 +++++++++++++++++-
 2 files changed, 505 insertions(+), 8 deletions(-)

-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v5 1/6] drm/bridge: sii902x: add input_bus_flags

[Jyri Sarha]

From: Tomi Valkeinen <tomi.valkeinen@ti.com>

The driver always sets InputBusFmt:EDGE to 0 (falling edge).

Add drm_bridge_timings's input_bus_flags to reflect that the bridge
samples on falling edges.

Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
Signed-off-by: Jyri Sarha <jsarha@ti.com>
Reviewed-by: Andrzej Hajda <a.hajda@samsung.com>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 drivers/gpu/drm/bridge/sii902x.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/gpu/drm/bridge/sii902x.c b/drivers/gpu/drm/bridge/sii902x.c
index bfa902013aa4..1afa000141d5 100644
--- a/drivers/gpu/drm/bridge/sii902x.c
+++ b/drivers/gpu/drm/bridge/sii902x.c
@@ -459,6 +459,12 @@ static int sii902x_i2c_bypass_deselect(struct i2c_mux_core *mux, u32 chan_id)
 	return 0;
 }
 
+static const struct drm_bridge_timings default_sii902x_timings = {
+	.input_bus_flags = DRM_BUS_FLAG_PIXDATA_SAMPLE_NEGEDGE
+		 | DRM_BUS_FLAG_SYNC_SAMPLE_NEGEDGE
+		 | DRM_BUS_FLAG_DE_HIGH,
+};
+
 static int sii902x_probe(struct i2c_client *client,
 			 const struct i2c_device_id *id)
 {
@@ -529,6 +535,7 @@ static int sii902x_probe(struct i2c_client *client,
 
 	sii902x->bridge.funcs = &sii902x_bridge_funcs;
 	sii902x->bridge.of_node = dev->of_node;
+	sii902x->bridge.timings = &default_sii902x_timings;
 	drm_bridge_add(&sii902x->bridge);
 
 	i2c_set_clientdata(client, sii902x);
-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v5 2/6] drm/bridge: sii902x: Set output mode to HDMI or DVI according to EDID

[Jyri Sarha]

Set output mode to HDMI or DVI according to EDID HDMI signature.

Signed-off-by: Jyri Sarha <jsarha@ti.com>
Reviewed-by: Andrzej Hajda <a.hajda@samsung.com>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 drivers/gpu/drm/bridge/sii902x.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/drivers/gpu/drm/bridge/sii902x.c b/drivers/gpu/drm/bridge/sii902x.c
index 1afa000141d5..f73eaa6d729a 100644
--- a/drivers/gpu/drm/bridge/sii902x.c
+++ b/drivers/gpu/drm/bridge/sii902x.c
@@ -180,12 +180,16 @@ static int sii902x_get_modes(struct drm_connector *connector)
 {
 	struct sii902x *sii902x = connector_to_sii902x(connector);
 	u32 bus_format = MEDIA_BUS_FMT_RGB888_1X24;
+	u8 output_mode = SII902X_SYS_CTRL_OUTPUT_DVI;
 	struct edid *edid;
 	int num = 0, ret;
 
 	edid = drm_get_edid(connector, sii902x->i2cmux->adapter[0]);
 	drm_connector_update_edid_property(connector, edid);
 	if (edid) {
+	        if (drm_detect_hdmi_monitor(edid))
+			output_mode = SII902X_SYS_CTRL_OUTPUT_HDMI;
+
 		num = drm_add_edid_modes(connector, edid);
 		kfree(edid);
 	}
@@ -195,6 +199,11 @@ static int sii902x_get_modes(struct drm_connector *connector)
 	if (ret)
 		return ret;
 
+	ret = regmap_update_bits(sii902x->regmap, SII902X_SYS_CTRL_DATA,
+				 SII902X_SYS_CTRL_OUTPUT_MODE, output_mode);
+	if (ret)
+		return ret;
+
 	return num;
 }
 
-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v5 3/6] drm/bridge: sii902x: pixel clock unit is 10kHz instead of 1kHz

[Jyri Sarha]

The pixel clock unit in the first two registers (0x00 and 0x01) of
sii9022 is 10kHz, not 1kHz as in struct drm_display_mode. Division by
10 fixes the issue.

Signed-off-by: Jyri Sarha <jsarha@ti.com>
Reviewed-by: Andrzej Hajda <a.hajda@samsung.com>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 drivers/gpu/drm/bridge/sii902x.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/bridge/sii902x.c b/drivers/gpu/drm/bridge/sii902x.c
index f73eaa6d729a..358cf81c5ea4 100644
--- a/drivers/gpu/drm/bridge/sii902x.c
+++ b/drivers/gpu/drm/bridge/sii902x.c
@@ -248,10 +248,11 @@ static void sii902x_bridge_mode_set(struct drm_bridge *bridge,
 	struct regmap *regmap = sii902x->regmap;
 	u8 buf[HDMI_INFOFRAME_SIZE(AVI)];
 	struct hdmi_avi_infoframe frame;
+	u16 pixel_clock_10kHz = adj->clock / 10;
 	int ret;
 
-	buf[0] = adj->clock;
-	buf[1] = adj->clock >> 8;
+	buf[0] = pixel_clock_10kHz & 0xff;
+	buf[1] = pixel_clock_10kHz >> 8;
 	buf[2] = adj->vrefresh;
 	buf[3] = 0x00;
 	buf[4] = adj->hdisplay;
-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v5 4/6] dt-bindings: display: sii902x: Remove trailing white space

[Jyri Sarha]

Remove trailing white space from sii902x display bridge binding.

Signed-off-by: Jyri Sarha <jsarha@ti.com>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 Documentation/devicetree/bindings/display/bridge/sii902x.txt | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/display/bridge/sii902x.txt b/Documentation/devicetree/bindings/display/bridge/sii902x.txt
index 72d2dc6c3e6b..c4c1855ca654 100644
--- a/Documentation/devicetree/bindings/display/bridge/sii902x.txt
+++ b/Documentation/devicetree/bindings/display/bridge/sii902x.txt
@@ -5,7 +5,7 @@ Required properties:
 	- reg: i2c address of the bridge
 
 Optional properties:
-	- interrupts: describe the interrupt line used to inform the host 
+	- interrupts: describe the interrupt line used to inform the host
 	  about hotplug events.
 	- reset-gpios: OF device-tree gpio specification for RST_N pin.
 
-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v5 4/6] dt-bindings: display: sii902x: Remove trailing white space

[Rob Herring]

On Fri, 22 Mar 2019 10:06:13 +0200, Jyri Sarha wrote:
> Remove trailing white space from sii902x display bridge binding.
> 
> Signed-off-by: Jyri Sarha <jsarha@ti.com>
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
>  Documentation/devicetree/bindings/display/bridge/sii902x.txt | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 

Please add Acked-by/Reviewed-by tags when posting new versions. However,
there's no need to repost patches *only* to add the tags. The upstream
maintainer will do that for acks received on the version they apply.

If a tag was not added on purpose, please state why and what changed.
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v5 5/6] dt-bindings: display: sii902x: Add HDMI audio bindings

[Jyri Sarha]

The sii902x chip family supports also HDMI audio. Add binding for
describing the necessary i2s and mclk wiring for it.

Signed-off-by: Jyri Sarha <jsarha@ti.com>
---
 .../bindings/display/bridge/sii902x.txt       | 33 +++++++++++++++++++
 1 file changed, 33 insertions(+)

diff --git a/Documentation/devicetree/bindings/display/bridge/sii902x.txt b/Documentation/devicetree/bindings/display/bridge/sii902x.txt
index c4c1855ca654..e78d069da439 100644
--- a/Documentation/devicetree/bindings/display/bridge/sii902x.txt
+++ b/Documentation/devicetree/bindings/display/bridge/sii902x.txt
@@ -9,6 +9,33 @@ Optional properties:
 	  about hotplug events.
 	- reset-gpios: OF device-tree gpio specification for RST_N pin.
 
+	HDMI audio properties:
+	- sil,i2s-data-lanes: Array of up to 4 integers with values of 0-3
+	   Each integer indicates which i2s pin is connected to which
+	   audio fifo. The first integer selects i2s audio pin for the
+	   first audio fifo#0 (HDMI channels 1&2), second for fifo#1
+	   (HDMI channels 3&4), and so on. There is 4 fifos and 4 i2s
+	   pins (SD0 - SD3). Any i2s pin can be connected to any fifo,
+	   but there can be no gaps. E.g. an i2s pin must be mapped to
+	   fifo#0 and fifo#1 before mapping a channel to fifo#2.
+	   I2S HDMI audio is configured only if this property is found.
+	- clocks: phandle and clock specifier for each clock listed in
+          the clock-names property
+	- clock-names: "mclk"
+	    Describes SII902x MCLK input. MCLK is used to produce
+	    HDMI audio CTS values. This property is required if
+	    "i2s-data-lanes"-property is present. This property follows
+	    Documentation/devicetree/bindings/clock/clock-bindings.txt
+	    consumer binding.
+
+	If HDMI audio is configured the sii902x device becomes an ASoC
+	codec component, that can be used in configuring full audio
+	devices with ASoC simple-card or audio-graph-card. See their
+	binding documents on how to describe how the sii902x device is
+	connected to the rest of the audio system:
+	Documentation/devicetree/bindings/sound/simple-card.txt
+	Documentation/devicetree/bindings/sound/audio-graph-card.txt
+
 Optional subnodes:
 	- video input: this subnode can contain a video input port node
 	  to connect the bridge to a display controller output (See this
@@ -21,6 +48,12 @@ Example:
 		compatible = "sil,sii9022";
 		reg = <0x39>;
 		reset-gpios = <&pioA 1 0>;
+
+		#sound-dai-cells = <0>;
+		i2s-data-lanes = < 0 1 2 >;
+		clocks = <&mclk>;
+		clock-names = "mclk";
+
 		ports {
 			#address-cells = <1>;
 			#size-cells = <0>;
-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v5 5/6] dt-bindings: display: sii902x: Add HDMI audio bindings

[Laurent Pinchart]

Hi Jyri,

Thank you for the patch.

On Fri, Mar 22, 2019 at 10:06:14AM +0200, Jyri Sarha wrote:
> The sii902x chip family supports also HDMI audio. Add binding for
> describing the necessary i2s and mclk wiring for it.
> 
> Signed-off-by: Jyri Sarha <jsarha@ti.com>
> ---
>  .../bindings/display/bridge/sii902x.txt       | 33 +++++++++++++++++++
>  1 file changed, 33 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/display/bridge/sii902x.txt b/Documentation/devicetree/bindings/display/bridge/sii902x.txt
> index c4c1855ca654..e78d069da439 100644
> --- a/Documentation/devicetree/bindings/display/bridge/sii902x.txt
> +++ b/Documentation/devicetree/bindings/display/bridge/sii902x.txt
> @@ -9,6 +9,33 @@ Optional properties:
>  	  about hotplug events.
>  	- reset-gpios: OF device-tree gpio specification for RST_N pin.
>  
> +	HDMI audio properties:
> +	- sil,i2s-data-lanes: Array of up to 4 integers with values of 0-3
> +	   Each integer indicates which i2s pin is connected to which
> +	   audio fifo. The first integer selects i2s audio pin for the
> +	   first audio fifo#0 (HDMI channels 1&2), second for fifo#1
> +	   (HDMI channels 3&4), and so on. There is 4 fifos and 4 i2s
> +	   pins (SD0 - SD3). Any i2s pin can be connected to any fifo,
> +	   but there can be no gaps. E.g. an i2s pin must be mapped to
> +	   fifo#0 and fifo#1 before mapping a channel to fifo#2.
> +	   I2S HDMI audio is configured only if this property is found.
> +	- clocks: phandle and clock specifier for each clock listed in
> +          the clock-names property
> +	- clock-names: "mclk"
> +	    Describes SII902x MCLK input. MCLK is used to produce
> +	    HDMI audio CTS values. This property is required if
> +	    "i2s-data-lanes"-property is present. This property follows
> +	    Documentation/devicetree/bindings/clock/clock-bindings.txt
> +	    consumer binding.
> +
> +	If HDMI audio is configured the sii902x device becomes an ASoC
> +	codec component, that can be used in configuring full audio
> +	devices with ASoC simple-card or audio-graph-card. See their
> +	binding documents on how to describe how the sii902x device is
> +	connected to the rest of the audio system:
> +	Documentation/devicetree/bindings/sound/simple-card.txt
> +	Documentation/devicetree/bindings/sound/audio-graph-card.txt
> +
>  Optional subnodes:
>  	- video input: this subnode can contain a video input port node
>  	  to connect the bridge to a display controller output (See this
> @@ -21,6 +48,12 @@ Example:
>  		compatible = "sil,sii9022";
>  		reg = <0x39>;
>  		reset-gpios = <&pioA 1 0>;
> +
> +		#sound-dai-cells = <0>;
> +		i2s-data-lanes = < 0 1 2 >;

This doesn't have the sil, prefix documented above. As stated in a
previous review, I wouldn't add a vendor prefix to this property as it
isn't vendor-specific, but I would then document it in a separate file
for common I2S properties.

Rob, what's your opinion ?

> +		clocks = <&mclk>;
> +		clock-names = "mclk";
> +
>  		ports {
>  			#address-cells = <1>;
>  			#size-cells = <0>;

-- 
Regards,

Laurent Pinchart
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v5 6/6] drm/bridge: sii902x: Implement HDMI audio support

[Jyri Sarha]

Implement HDMI audio support by using ASoC HDMI codec. The commit
implements the necessary callbacks and configuration for the HDMI
codec and registers a virtual platform device for the codec to attach.

Signed-off-by: Jyri Sarha <jsarha@ti.com>
---
 drivers/gpu/drm/bridge/sii902x.c | 459 ++++++++++++++++++++++++++++++-
 1 file changed, 453 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/bridge/sii902x.c b/drivers/gpu/drm/bridge/sii902x.c
index 358cf81c5ea4..cb12e264111d 100644
--- a/drivers/gpu/drm/bridge/sii902x.c
+++ b/drivers/gpu/drm/bridge/sii902x.c
@@ -27,12 +27,15 @@
 #include <linux/i2c.h>
 #include <linux/module.h>
 #include <linux/regmap.h>
+#include <linux/clk.h>
 
 #include <drm/drmP.h>
 #include <drm/drm_atomic_helper.h>
 #include <drm/drm_crtc_helper.h>
 #include <drm/drm_edid.h>
 
+#include <sound/hdmi-codec.h>
+
 #define SII902X_TPI_VIDEO_DATA			0x0
 
 #define SII902X_TPI_PIXEL_REPETITION		0x8
@@ -74,6 +77,77 @@
 #define SII902X_AVI_POWER_STATE_MSK		GENMASK(1, 0)
 #define SII902X_AVI_POWER_STATE_D(l)		((l) & SII902X_AVI_POWER_STATE_MSK)
 
+/* Audio  */
+#define SII902X_TPI_I2S_ENABLE_MAPPING_REG	0x1f
+#define SII902X_TPI_I2S_CONFIG_FIFO0			(0 << 0)
+#define SII902X_TPI_I2S_CONFIG_FIFO1			(1 << 0)
+#define SII902X_TPI_I2S_CONFIG_FIFO2			(2 << 0)
+#define SII902X_TPI_I2S_CONFIG_FIFO3			(3 << 0)
+#define SII902X_TPI_I2S_LEFT_RIGHT_SWAP			(1 << 2)
+#define SII902X_TPI_I2S_AUTO_DOWNSAMPLE			(1 << 3)
+#define SII902X_TPI_I2S_SELECT_SD0			(0 << 4)
+#define SII902X_TPI_I2S_SELECT_SD1			(1 << 4)
+#define SII902X_TPI_I2S_SELECT_SD2			(2 << 4)
+#define SII902X_TPI_I2S_SELECT_SD3			(3 << 4)
+#define SII902X_TPI_I2S_FIFO_ENABLE			(1 << 7)
+
+#define SII902X_TPI_I2S_INPUT_CONFIG_REG	0x20
+#define SII902X_TPI_I2S_FIRST_BIT_SHIFT_YES		(0 << 0)
+#define SII902X_TPI_I2S_FIRST_BIT_SHIFT_NO		(1 << 0)
+#define SII902X_TPI_I2S_SD_DIRECTION_MSB_FIRST		(0 << 1)
+#define SII902X_TPI_I2S_SD_DIRECTION_LSB_FIRST		(1 << 1)
+#define SII902X_TPI_I2S_SD_JUSTIFY_LEFT			(0 << 2)
+#define SII902X_TPI_I2S_SD_JUSTIFY_RIGHT		(1 << 2)
+#define SII902X_TPI_I2S_WS_POLARITY_LOW			(0 << 3)
+#define SII902X_TPI_I2S_WS_POLARITY_HIGH		(1 << 3)
+#define SII902X_TPI_I2S_MCLK_MULTIPLIER_128		(0 << 4)
+#define SII902X_TPI_I2S_MCLK_MULTIPLIER_256		(1 << 4)
+#define SII902X_TPI_I2S_MCLK_MULTIPLIER_384		(2 << 4)
+#define SII902X_TPI_I2S_MCLK_MULTIPLIER_512		(3 << 4)
+#define SII902X_TPI_I2S_MCLK_MULTIPLIER_768		(4 << 4)
+#define SII902X_TPI_I2S_MCLK_MULTIPLIER_1024		(5 << 4)
+#define SII902X_TPI_I2S_MCLK_MULTIPLIER_1152		(6 << 4)
+#define SII902X_TPI_I2S_MCLK_MULTIPLIER_192		(7 << 4)
+#define SII902X_TPI_I2S_SCK_EDGE_FALLING		(0 << 7)
+#define SII902X_TPI_I2S_SCK_EDGE_RISING			(1 << 7)
+
+#define SII902X_TPI_I2S_STRM_HDR_BASE	0x21
+#define SII902X_TPI_I2S_STRM_HDR_SIZE	5
+
+#define SII902X_TPI_AUDIO_CONFIG_BYTE2_REG	0x26
+#define SII902X_TPI_AUDIO_CODING_STREAM_HEADER		(0 << 0)
+#define SII902X_TPI_AUDIO_CODING_PCM			(1 << 0)
+#define SII902X_TPI_AUDIO_CODING_AC3			(2 << 0)
+#define SII902X_TPI_AUDIO_CODING_MPEG1			(3 << 0)
+#define SII902X_TPI_AUDIO_CODING_MP3			(4 << 0)
+#define SII902X_TPI_AUDIO_CODING_MPEG2			(5 << 0)
+#define SII902X_TPI_AUDIO_CODING_AAC			(6 << 0)
+#define SII902X_TPI_AUDIO_CODING_DTS			(7 << 0)
+#define SII902X_TPI_AUDIO_CODING_ATRAC			(8 << 0)
+#define SII902X_TPI_AUDIO_MUTE_DISABLE			(0 << 4)
+#define SII902X_TPI_AUDIO_MUTE_ENABLE			(1 << 4)
+#define SII902X_TPI_AUDIO_LAYOUT_2_CHANNELS		(0 << 5)
+#define SII902X_TPI_AUDIO_LAYOUT_8_CHANNELS		(1 << 5)
+#define SII902X_TPI_AUDIO_INTERFACE_DISABLE		(0 << 6)
+#define SII902X_TPI_AUDIO_INTERFACE_SPDIF		(1 << 6)
+#define SII902X_TPI_AUDIO_INTERFACE_I2S			(2 << 6)
+
+#define SII902X_TPI_AUDIO_CONFIG_BYTE3_REG	0x27
+#define SII902X_TPI_AUDIO_FREQ_STREAM			(0 << 3)
+#define SII902X_TPI_AUDIO_FREQ_32KHZ			(1 << 3)
+#define SII902X_TPI_AUDIO_FREQ_44KHZ			(2 << 3)
+#define SII902X_TPI_AUDIO_FREQ_48KHZ			(3 << 3)
+#define SII902X_TPI_AUDIO_FREQ_88KHZ			(4 << 3)
+#define SII902X_TPI_AUDIO_FREQ_96KHZ			(5 << 3)
+#define SII902X_TPI_AUDIO_FREQ_176KHZ			(6 << 3)
+#define SII902X_TPI_AUDIO_FREQ_192KHZ			(7 << 3)
+#define SII902X_TPI_AUDIO_SAMPLE_SIZE_STREAM		(0 << 6)
+#define SII902X_TPI_AUDIO_SAMPLE_SIZE_16		(1 << 6)
+#define SII902X_TPI_AUDIO_SAMPLE_SIZE_20		(2 << 6)
+#define SII902X_TPI_AUDIO_SAMPLE_SIZE_24		(3 << 6)
+
+#define SII902X_TPI_AUDIO_CONFIG_BYTE4_REG	0x28
+
 #define SII902X_INT_ENABLE			0x3c
 #define SII902X_INT_STATUS			0x3d
 #define SII902X_HOTPLUG_EVENT			BIT(0)
@@ -81,6 +155,16 @@
 
 #define SII902X_REG_TPI_RQB			0xc7
 
+/* Indirect internal register access */
+#define SII902X_IND_SET_PAGE			0xbc
+#define SII902X_IND_OFFSET			0xbd
+#define SII902X_IND_VALUE			0xbe
+
+#define SII902X_TPI_MISC_INFOFRAME_BASE		0xbf
+#define SII902X_TPI_MISC_INFOFRAME_END		0xde
+#define SII902X_TPI_MISC_INFOFRAME_SIZE	\
+	(SII902X_TPI_MISC_INFOFRAME_END - SII902X_TPI_MISC_INFOFRAME_BASE)
+
 #define SII902X_I2C_BUS_ACQUISITION_TIMEOUT_MS	500
 
 struct sii902x {
@@ -90,6 +174,16 @@ struct sii902x {
 	struct drm_connector connector;
 	struct gpio_desc *reset_gpio;
 	struct i2c_mux_core *i2cmux;
+	/*
+	 * Mutex protects audio and video functions from interfering
+	 * each other, by keeping their i2c command sequences atomic.
+	 */
+	struct mutex mutex;
+	struct sii902x_audio {
+		struct platform_device *pdev;
+		struct clk *mclk;
+		u32 i2s_fifo_sequence[4];
+	} audio;
 };
 
 static int sii902x_read_unlocked(struct i2c_client *i2c, u8 reg, u8 *val)
@@ -161,8 +255,12 @@ sii902x_connector_detect(struct drm_connector *connector, bool force)
 	struct sii902x *sii902x = connector_to_sii902x(connector);
 	unsigned int status;
 
+	mutex_lock(&sii902x->mutex);
+
 	regmap_read(sii902x->regmap, SII902X_INT_STATUS, &status);
 
+	mutex_unlock(&sii902x->mutex);
+
 	return (status & SII902X_PLUGGED_STATUS) ?
 	       connector_status_connected : connector_status_disconnected;
 }
@@ -184,6 +282,8 @@ static int sii902x_get_modes(struct drm_connector *connector)
 	struct edid *edid;
 	int num = 0, ret;
 
+	mutex_lock(&sii902x->mutex);
+
 	edid = drm_get_edid(connector, sii902x->i2cmux->adapter[0]);
 	drm_connector_update_edid_property(connector, edid);
 	if (edid) {
@@ -197,14 +297,19 @@ static int sii902x_get_modes(struct drm_connector *connector)
 	ret = drm_display_info_set_bus_formats(&connector->display_info,
 					       &bus_format, 1);
 	if (ret)
-		return ret;
+		goto error_out;
 
 	ret = regmap_update_bits(sii902x->regmap, SII902X_SYS_CTRL_DATA,
 				 SII902X_SYS_CTRL_OUTPUT_MODE, output_mode);
 	if (ret)
-		return ret;
+		goto error_out;
+
+	ret = num;
+
+error_out:
+	mutex_unlock(&sii902x->mutex);
 
-	return num;
+	return ret;
 }
 
 static enum drm_mode_status sii902x_mode_valid(struct drm_connector *connector,
@@ -224,20 +329,28 @@ static void sii902x_bridge_disable(struct drm_bridge *bridge)
 {
 	struct sii902x *sii902x = bridge_to_sii902x(bridge);
 
+	mutex_lock(&sii902x->mutex);
+
 	regmap_update_bits(sii902x->regmap, SII902X_SYS_CTRL_DATA,
 			   SII902X_SYS_CTRL_PWR_DWN,
 			   SII902X_SYS_CTRL_PWR_DWN);
+
+	mutex_unlock(&sii902x->mutex);
 }
 
 static void sii902x_bridge_enable(struct drm_bridge *bridge)
 {
 	struct sii902x *sii902x = bridge_to_sii902x(bridge);
 
+	mutex_lock(&sii902x->mutex);
+
 	regmap_update_bits(sii902x->regmap, SII902X_PWR_STATE_CTRL,
 			   SII902X_AVI_POWER_STATE_MSK,
 			   SII902X_AVI_POWER_STATE_D(0));
 	regmap_update_bits(sii902x->regmap, SII902X_SYS_CTRL_DATA,
 			   SII902X_SYS_CTRL_PWR_DWN, 0);
+
+	mutex_unlock(&sii902x->mutex);
 }
 
 static void sii902x_bridge_mode_set(struct drm_bridge *bridge,
@@ -264,26 +377,31 @@ static void sii902x_bridge_mode_set(struct drm_bridge *bridge,
 	buf[9] = SII902X_TPI_AVI_INPUT_RANGE_AUTO |
 		 SII902X_TPI_AVI_INPUT_COLORSPACE_RGB;
 
+	mutex_lock(&sii902x->mutex);
+
 	ret = regmap_bulk_write(regmap, SII902X_TPI_VIDEO_DATA, buf, 10);
 	if (ret)
-		return;
+		goto out;
 
 	ret = drm_hdmi_avi_infoframe_from_display_mode(&frame, adj, false);
 	if (ret < 0) {
 		DRM_ERROR("couldn't fill AVI infoframe\n");
-		return;
+		goto out;
 	}
 
 	ret = hdmi_avi_infoframe_pack(&frame, buf, sizeof(buf));
 	if (ret < 0) {
 		DRM_ERROR("failed to pack AVI infoframe: %d\n", ret);
-		return;
+		goto out;
 	}
 
 	/* Do not send the infoframe header, but keep the CRC field. */
 	regmap_bulk_write(regmap, SII902X_TPI_AVI_INFOFRAME,
 			  buf + HDMI_INFOFRAME_HEADER_SIZE - 1,
 			  HDMI_AVI_INFOFRAME_SIZE + 1);
+
+out:
+	mutex_unlock(&sii902x->mutex);
 }
 
 static int sii902x_bridge_attach(struct drm_bridge *bridge)
@@ -324,6 +442,330 @@ static const struct drm_bridge_funcs sii902x_bridge_funcs = {
 	.enable = sii902x_bridge_enable,
 };
 
+static int sii902x_mute(struct sii902x *sii902x, bool mute)
+{
+	struct device *dev = &sii902x->i2c->dev;
+	unsigned int val = mute ? SII902X_TPI_AUDIO_MUTE_ENABLE :
+		SII902X_TPI_AUDIO_MUTE_DISABLE;
+
+	dev_dbg(dev, "%s: %s\n", __func__, mute ? "Muted" : "Unmuted");
+
+	return regmap_update_bits(sii902x->regmap,
+				  SII902X_TPI_AUDIO_CONFIG_BYTE2_REG,
+				  SII902X_TPI_AUDIO_MUTE_ENABLE, val);
+}
+
+static const unsigned int sii902x_mclk_div_table[] = {
+	128, 256, 384, 512, 768, 1024, 1152, 192 };
+
+static int sii902x_select_mclk_div(u8 *i2s_config_reg, unsigned int rate,
+				   unsigned int mclk)
+{
+	unsigned int div = mclk / rate;
+	int distance = 100000;
+	u8 i, nearest = 0;
+
+	for (i = 0; i < ARRAY_SIZE(sii902x_mclk_div_table); i++) {
+		unsigned int d = abs(div - sii902x_mclk_div_table[i]);
+
+		if (d >= distance)
+			continue;
+
+		nearest = i;
+		distance = d;
+		if (d == 0)
+			break;
+	}
+
+	*i2s_config_reg |= nearest << 4;
+
+	if (distance != 0)
+		return sii902x_mclk_div_table[nearest];
+
+	return 0;
+}
+
+static const struct sii902x_sample_freq {
+	u32 freq;
+	u8 val;
+} sii902x_sample_freq[] = {
+	{ .freq = 32000,	.val = SII902X_TPI_AUDIO_FREQ_32KHZ },
+	{ .freq = 44000,	.val = SII902X_TPI_AUDIO_FREQ_44KHZ },
+	{ .freq = 48000,	.val = SII902X_TPI_AUDIO_FREQ_48KHZ },
+	{ .freq = 88000,	.val = SII902X_TPI_AUDIO_FREQ_88KHZ },
+	{ .freq = 96000,	.val = SII902X_TPI_AUDIO_FREQ_96KHZ },
+	{ .freq = 176000,	.val = SII902X_TPI_AUDIO_FREQ_176KHZ },
+	{ .freq = 192000,	.val = SII902X_TPI_AUDIO_FREQ_192KHZ },
+};
+
+static int sii902x_audio_hw_params(struct device *dev, void *data,
+				   struct hdmi_codec_daifmt *daifmt,
+				   struct hdmi_codec_params *params)
+{
+	struct sii902x *sii902x = dev_get_drvdata(dev);
+	u8 i2s_config_reg = SII902X_TPI_I2S_SD_DIRECTION_MSB_FIRST;
+	u8 config_byte2_reg = (SII902X_TPI_AUDIO_INTERFACE_I2S |
+			       SII902X_TPI_AUDIO_MUTE_ENABLE |
+			       SII902X_TPI_AUDIO_CODING_PCM);
+	u8 config_byte3_reg = 0;
+	u8 infoframe_buf[HDMI_INFOFRAME_SIZE(AUDIO)];
+	unsigned long mclk_rate;
+	int i, ret;
+
+	if (daifmt->bit_clk_master || daifmt->frame_clk_master) {
+		dev_dbg(dev, "%s: I2S master mode not supported\n", __func__);
+		return -EINVAL;
+	}
+
+	switch (daifmt->fmt) {
+	case HDMI_I2S:
+		i2s_config_reg |= SII902X_TPI_I2S_FIRST_BIT_SHIFT_YES |
+			SII902X_TPI_I2S_SD_JUSTIFY_LEFT;
+		break;
+	case HDMI_RIGHT_J:
+		i2s_config_reg |= SII902X_TPI_I2S_SD_JUSTIFY_RIGHT;
+		break;
+	case HDMI_LEFT_J:
+		i2s_config_reg |= SII902X_TPI_I2S_SD_JUSTIFY_LEFT;
+		break;
+	default:
+		dev_dbg(dev, "%s: Unsupported i2s format %u\n", __func__,
+			daifmt->fmt);
+		return -EINVAL;
+	}
+
+	if (daifmt->bit_clk_inv)
+		i2s_config_reg |= SII902X_TPI_I2S_SCK_EDGE_FALLING;
+	else
+		i2s_config_reg |= SII902X_TPI_I2S_SCK_EDGE_RISING;
+
+	if (daifmt->frame_clk_inv)
+		i2s_config_reg |= SII902X_TPI_I2S_WS_POLARITY_LOW;
+	else
+		i2s_config_reg |= SII902X_TPI_I2S_WS_POLARITY_HIGH;
+
+	if (params->channels > 2)
+		config_byte2_reg |= SII902X_TPI_AUDIO_LAYOUT_8_CHANNELS;
+	else
+		config_byte2_reg |= SII902X_TPI_AUDIO_LAYOUT_2_CHANNELS;
+
+	switch (params->sample_width) {
+	case 16:
+		config_byte3_reg |= SII902X_TPI_AUDIO_SAMPLE_SIZE_16;
+		break;
+	case 20:
+		config_byte3_reg |= SII902X_TPI_AUDIO_SAMPLE_SIZE_20;
+		break;
+	case 24:
+	case 32:
+		config_byte3_reg |= SII902X_TPI_AUDIO_SAMPLE_SIZE_24;
+		break;
+	default:
+		dev_err(dev, "%s: Unsupported sample width %u\n", __func__,
+			params->sample_width);
+		return -EINVAL;
+	}
+
+	for (i = 0; i < ARRAY_SIZE(sii902x_sample_freq); i++) {
+		if (params->sample_rate == sii902x_sample_freq[i].freq) {
+			config_byte3_reg |= sii902x_sample_freq[i].val;
+			break;
+		}
+	}
+
+	ret = clk_prepare_enable(sii902x->audio.mclk);
+	if (ret) {
+		dev_err(dev, "Enabling mclk failed: %d\n", ret);
+		return ret;
+	}
+
+	mclk_rate = clk_get_rate(sii902x->audio.mclk);
+
+	ret = sii902x_select_mclk_div(&i2s_config_reg, params->sample_rate,
+				      mclk_rate);
+	if (ret)
+		dev_dbg(dev, "Inaccurate reference clock (%ld/%d != %u)\n",
+			mclk_rate, ret, params->sample_rate);
+
+	mutex_lock(&sii902x->mutex);
+
+	ret = regmap_write(sii902x->regmap,
+			   SII902X_TPI_AUDIO_CONFIG_BYTE2_REG,
+			   config_byte2_reg);
+	if (ret < 0)
+		goto out;
+
+	ret = regmap_write(sii902x->regmap, SII902X_TPI_I2S_INPUT_CONFIG_REG,
+			   i2s_config_reg);
+	if (ret)
+		goto out;
+
+	for (i = 0; sii902x->audio.i2s_fifo_sequence[i] &&
+		     i < ARRAY_SIZE(sii902x->audio.i2s_fifo_sequence); i++)
+		regmap_write(sii902x->regmap,
+			     SII902X_TPI_I2S_ENABLE_MAPPING_REG,
+			     sii902x->audio.i2s_fifo_sequence[i]);
+
+	ret = regmap_write(sii902x->regmap, SII902X_TPI_AUDIO_CONFIG_BYTE3_REG,
+			   config_byte3_reg);
+	if (ret)
+		goto out;
+
+	ret = regmap_bulk_write(sii902x->regmap, SII902X_TPI_I2S_STRM_HDR_BASE,
+				params->iec.status,
+				min((size_t) SII902X_TPI_I2S_STRM_HDR_SIZE,
+				    sizeof(params->iec.status)));
+	if (ret)
+		goto out;
+
+	ret = hdmi_audio_infoframe_pack(&params->cea, infoframe_buf,
+					sizeof(infoframe_buf));
+	if (ret < 0) {
+		dev_err(dev, "%s: Failed to pack audio infoframe: %d\n",
+			__func__, ret);
+		goto out;
+	}
+
+	ret = regmap_bulk_write(sii902x->regmap,
+				SII902X_TPI_MISC_INFOFRAME_BASE,
+				infoframe_buf,
+				min(ret, SII902X_TPI_MISC_INFOFRAME_SIZE));
+	if (ret)
+		goto out;
+
+	/* Decode Level 0 Packets */
+	ret = regmap_write(sii902x->regmap, SII902X_IND_SET_PAGE, 0x02);
+	if (ret)
+		goto out;
+
+	ret = regmap_write(sii902x->regmap, SII902X_IND_OFFSET, 0x24);
+	if (ret)
+		goto out;
+
+	ret = regmap_write(sii902x->regmap, SII902X_IND_VALUE, 0x02);
+	if (ret)
+		goto out;
+
+	dev_dbg(dev, "%s: hdmi audio enabled\n", __func__);
+out:
+	mutex_unlock(&sii902x->mutex);
+
+	if (ret) {
+		clk_disable_unprepare(sii902x->audio.mclk);
+		dev_err(dev, "%s: hdmi audio enable failed: %d\n", __func__,
+			ret);
+	}
+
+	return ret;
+}
+
+static void sii902x_audio_shutdown(struct device *dev, void *data)
+{
+	struct sii902x *sii902x = dev_get_drvdata(dev);
+
+	mutex_lock(&sii902x->mutex);
+
+	regmap_write(sii902x->regmap, SII902X_TPI_AUDIO_CONFIG_BYTE2_REG,
+		     SII902X_TPI_AUDIO_INTERFACE_DISABLE);
+
+	mutex_unlock(&sii902x->mutex);
+
+	clk_disable_unprepare(sii902x->audio.mclk);
+}
+
+int sii902x_audio_digital_mute(struct device *dev, void *data, bool enable)
+{
+	struct sii902x *sii902x = dev_get_drvdata(dev);
+
+	mutex_lock(&sii902x->mutex);
+
+	sii902x_mute(sii902x, enable);
+
+	mutex_unlock(&sii902x->mutex);
+
+	return 0;
+}
+
+static int sii902x_audio_get_eld(struct device *dev, void *data,
+				 uint8_t *buf, size_t len)
+{
+	struct sii902x *sii902x = dev_get_drvdata(dev);
+
+	mutex_lock(&sii902x->mutex);
+
+	memcpy(buf, sii902x->connector.eld,
+	       min(sizeof(sii902x->connector.eld), len));
+
+	mutex_unlock(&sii902x->mutex);
+
+	return 0;
+}
+
+static const struct hdmi_codec_ops sii902x_audio_codec_ops = {
+	.hw_params = sii902x_audio_hw_params,
+	.audio_shutdown = sii902x_audio_shutdown,
+	.digital_mute = sii902x_audio_digital_mute,
+	.get_eld = sii902x_audio_get_eld,
+};
+
+static int sii902x_audio_codec_init(struct sii902x *sii902x,
+				    struct device *dev)
+{
+	static const u8 audio_fifo_id[] = {
+		SII902X_TPI_I2S_CONFIG_FIFO0,
+		SII902X_TPI_I2S_CONFIG_FIFO1,
+		SII902X_TPI_I2S_CONFIG_FIFO2,
+		SII902X_TPI_I2S_CONFIG_FIFO3,
+	};
+	static const u8 i2s_lane_id[] = {
+		SII902X_TPI_I2S_SELECT_SD0,
+		SII902X_TPI_I2S_SELECT_SD1,
+		SII902X_TPI_I2S_SELECT_SD2,
+		SII902X_TPI_I2S_SELECT_SD3,
+	};
+	struct hdmi_codec_pdata codec_data = {
+		.ops = &sii902x_audio_codec_ops,
+		.i2s = 1, /* Only i2s support for now. */
+		.spdif = 0,
+		.max_i2s_channels = 0,
+	};
+	u8 lanes[4];
+	u32 num_lanes, i;
+
+	num_lanes = of_property_read_variable_u8_array(dev->of_node,
+						       "sil,i2s-data-lanes",
+						       lanes, 1,
+						       ARRAY_SIZE(lanes));
+	if (num_lanes < 0) {
+		if (num_lanes == -EINVAL)
+			dev_dbg(dev,
+				"%s: No \"sil,i2s-data-lanes\", no audio\n",
+				__func__);
+		else
+			dev_err(dev,
+				"%s: Error gettin \"sil,i2s-data-lanes\": %d\n",
+				__func__, num_lanes);
+		return 0;
+	}
+	codec_data.max_i2s_channels = 2 * num_lanes;
+
+	for (i = 0; i < num_lanes; i++)
+		sii902x->audio.i2s_fifo_sequence[i] |= audio_fifo_id[i] |
+			i2s_lane_id[lanes[i]] |	SII902X_TPI_I2S_FIFO_ENABLE;
+
+	if (IS_ERR(sii902x->audio.mclk)) {
+		dev_err(dev, "%s: No clock (audio mclk) found: %ld\n",
+			__func__, PTR_ERR(sii902x->audio.mclk));
+		return 0;
+	}
+
+	sii902x->audio.pdev = platform_device_register_data(
+		dev, HDMI_CODEC_DRV_NAME, PLATFORM_DEVID_AUTO,
+		&codec_data, sizeof(codec_data));
+
+	return PTR_ERR_OR_ZERO(sii902x->audio.pdev);
+}
+
 static const struct regmap_range sii902x_volatile_ranges[] = {
 	{ .range_min = 0, .range_max = 0xff },
 };
@@ -336,6 +778,7 @@ static const struct regmap_access_table sii902x_volatile_table = {
 static const struct regmap_config sii902x_regmap_config = {
 	.reg_bits = 8,
 	.val_bits = 8,
+	.max_register = SII902X_TPI_MISC_INFOFRAME_END,
 	.volatile_table = &sii902x_volatile_table,
 	.cache_type = REGCACHE_NONE,
 };
@@ -508,6 +951,8 @@ static int sii902x_probe(struct i2c_client *client,
 		return PTR_ERR(sii902x->reset_gpio);
 	}
 
+	mutex_init(&sii902x->mutex);
+
 	sii902x_reset(sii902x);
 
 	ret = regmap_write(sii902x->regmap, SII902X_REG_TPI_RQB, 0x0);
@@ -548,6 +993,8 @@ static int sii902x_probe(struct i2c_client *client,
 	sii902x->bridge.timings = &default_sii902x_timings;
 	drm_bridge_add(&sii902x->bridge);
 
+	sii902x_audio_codec_init(sii902x, dev);
+
 	i2c_set_clientdata(client, sii902x);
 
 	sii902x->i2cmux = i2c_mux_alloc(client->adapter, dev,
-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v5 6/6] drm/bridge: sii902x: Implement HDMI audio support

[Andrzej Hajda]

On 22.03.2019 09:06, Jyri Sarha wrote:
> Implement HDMI audio support by using ASoC HDMI codec. The commit
> implements the necessary callbacks and configuration for the HDMI
> codec and registers a virtual platform device for the codec to attach.
>
> Signed-off-by: Jyri Sarha <jsarha@ti.com>
> ---
>  drivers/gpu/drm/bridge/sii902x.c | 459 ++++++++++++++++++++++++++++++-
>  1 file changed, 453 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/sii902x.c b/drivers/gpu/drm/bridge/sii902x.c
> index 358cf81c5ea4..cb12e264111d 100644
> --- a/drivers/gpu/drm/bridge/sii902x.c
> +++ b/drivers/gpu/drm/bridge/sii902x.c
> @@ -27,12 +27,15 @@
>  #include <linux/i2c.h>
>  #include <linux/module.h>
>  #include <linux/regmap.h>
> +#include <linux/clk.h>
>  
>  #include <drm/drmP.h>
>  #include <drm/drm_atomic_helper.h>
>  #include <drm/drm_crtc_helper.h>
>  #include <drm/drm_edid.h>
>  
> +#include <sound/hdmi-codec.h>
> +
>  #define SII902X_TPI_VIDEO_DATA			0x0
>  
>  #define SII902X_TPI_PIXEL_REPETITION		0x8
> @@ -74,6 +77,77 @@
>  #define SII902X_AVI_POWER_STATE_MSK		GENMASK(1, 0)
>  #define SII902X_AVI_POWER_STATE_D(l)		((l) & SII902X_AVI_POWER_STATE_MSK)
>  
> +/* Audio  */
> +#define SII902X_TPI_I2S_ENABLE_MAPPING_REG	0x1f
> +#define SII902X_TPI_I2S_CONFIG_FIFO0			(0 << 0)
> +#define SII902X_TPI_I2S_CONFIG_FIFO1			(1 << 0)
> +#define SII902X_TPI_I2S_CONFIG_FIFO2			(2 << 0)
> +#define SII902X_TPI_I2S_CONFIG_FIFO3			(3 << 0)
> +#define SII902X_TPI_I2S_LEFT_RIGHT_SWAP			(1 << 2)
> +#define SII902X_TPI_I2S_AUTO_DOWNSAMPLE			(1 << 3)
> +#define SII902X_TPI_I2S_SELECT_SD0			(0 << 4)
> +#define SII902X_TPI_I2S_SELECT_SD1			(1 << 4)
> +#define SII902X_TPI_I2S_SELECT_SD2			(2 << 4)
> +#define SII902X_TPI_I2S_SELECT_SD3			(3 << 4)
> +#define SII902X_TPI_I2S_FIFO_ENABLE			(1 << 7)
> +
> +#define SII902X_TPI_I2S_INPUT_CONFIG_REG	0x20
> +#define SII902X_TPI_I2S_FIRST_BIT_SHIFT_YES		(0 << 0)
> +#define SII902X_TPI_I2S_FIRST_BIT_SHIFT_NO		(1 << 0)
> +#define SII902X_TPI_I2S_SD_DIRECTION_MSB_FIRST		(0 << 1)
> +#define SII902X_TPI_I2S_SD_DIRECTION_LSB_FIRST		(1 << 1)
> +#define SII902X_TPI_I2S_SD_JUSTIFY_LEFT			(0 << 2)
> +#define SII902X_TPI_I2S_SD_JUSTIFY_RIGHT		(1 << 2)
> +#define SII902X_TPI_I2S_WS_POLARITY_LOW			(0 << 3)
> +#define SII902X_TPI_I2S_WS_POLARITY_HIGH		(1 << 3)
> +#define SII902X_TPI_I2S_MCLK_MULTIPLIER_128		(0 << 4)
> +#define SII902X_TPI_I2S_MCLK_MULTIPLIER_256		(1 << 4)
> +#define SII902X_TPI_I2S_MCLK_MULTIPLIER_384		(2 << 4)
> +#define SII902X_TPI_I2S_MCLK_MULTIPLIER_512		(3 << 4)
> +#define SII902X_TPI_I2S_MCLK_MULTIPLIER_768		(4 << 4)
> +#define SII902X_TPI_I2S_MCLK_MULTIPLIER_1024		(5 << 4)
> +#define SII902X_TPI_I2S_MCLK_MULTIPLIER_1152		(6 << 4)
> +#define SII902X_TPI_I2S_MCLK_MULTIPLIER_192		(7 << 4)
> +#define SII902X_TPI_I2S_SCK_EDGE_FALLING		(0 << 7)
> +#define SII902X_TPI_I2S_SCK_EDGE_RISING			(1 << 7)
> +
> +#define SII902X_TPI_I2S_STRM_HDR_BASE	0x21
> +#define SII902X_TPI_I2S_STRM_HDR_SIZE	5
> +
> +#define SII902X_TPI_AUDIO_CONFIG_BYTE2_REG	0x26
> +#define SII902X_TPI_AUDIO_CODING_STREAM_HEADER		(0 << 0)
> +#define SII902X_TPI_AUDIO_CODING_PCM			(1 << 0)
> +#define SII902X_TPI_AUDIO_CODING_AC3			(2 << 0)
> +#define SII902X_TPI_AUDIO_CODING_MPEG1			(3 << 0)
> +#define SII902X_TPI_AUDIO_CODING_MP3			(4 << 0)
> +#define SII902X_TPI_AUDIO_CODING_MPEG2			(5 << 0)
> +#define SII902X_TPI_AUDIO_CODING_AAC			(6 << 0)
> +#define SII902X_TPI_AUDIO_CODING_DTS			(7 << 0)
> +#define SII902X_TPI_AUDIO_CODING_ATRAC			(8 << 0)
> +#define SII902X_TPI_AUDIO_MUTE_DISABLE			(0 << 4)
> +#define SII902X_TPI_AUDIO_MUTE_ENABLE			(1 << 4)
> +#define SII902X_TPI_AUDIO_LAYOUT_2_CHANNELS		(0 << 5)
> +#define SII902X_TPI_AUDIO_LAYOUT_8_CHANNELS		(1 << 5)
> +#define SII902X_TPI_AUDIO_INTERFACE_DISABLE		(0 << 6)
> +#define SII902X_TPI_AUDIO_INTERFACE_SPDIF		(1 << 6)
> +#define SII902X_TPI_AUDIO_INTERFACE_I2S			(2 << 6)
> +
> +#define SII902X_TPI_AUDIO_CONFIG_BYTE3_REG	0x27
> +#define SII902X_TPI_AUDIO_FREQ_STREAM			(0 << 3)
> +#define SII902X_TPI_AUDIO_FREQ_32KHZ			(1 << 3)
> +#define SII902X_TPI_AUDIO_FREQ_44KHZ			(2 << 3)
> +#define SII902X_TPI_AUDIO_FREQ_48KHZ			(3 << 3)
> +#define SII902X_TPI_AUDIO_FREQ_88KHZ			(4 << 3)
> +#define SII902X_TPI_AUDIO_FREQ_96KHZ			(5 << 3)
> +#define SII902X_TPI_AUDIO_FREQ_176KHZ			(6 << 3)
> +#define SII902X_TPI_AUDIO_FREQ_192KHZ			(7 << 3)
> +#define SII902X_TPI_AUDIO_SAMPLE_SIZE_STREAM		(0 << 6)
> +#define SII902X_TPI_AUDIO_SAMPLE_SIZE_16		(1 << 6)
> +#define SII902X_TPI_AUDIO_SAMPLE_SIZE_20		(2 << 6)
> +#define SII902X_TPI_AUDIO_SAMPLE_SIZE_24		(3 << 6)
> +
> +#define SII902X_TPI_AUDIO_CONFIG_BYTE4_REG	0x28
> +
>  #define SII902X_INT_ENABLE			0x3c
>  #define SII902X_INT_STATUS			0x3d
>  #define SII902X_HOTPLUG_EVENT			BIT(0)
> @@ -81,6 +155,16 @@
>  
>  #define SII902X_REG_TPI_RQB			0xc7
>  
> +/* Indirect internal register access */
> +#define SII902X_IND_SET_PAGE			0xbc
> +#define SII902X_IND_OFFSET			0xbd
> +#define SII902X_IND_VALUE			0xbe
> +
> +#define SII902X_TPI_MISC_INFOFRAME_BASE		0xbf
> +#define SII902X_TPI_MISC_INFOFRAME_END		0xde
> +#define SII902X_TPI_MISC_INFOFRAME_SIZE	\
> +	(SII902X_TPI_MISC_INFOFRAME_END - SII902X_TPI_MISC_INFOFRAME_BASE)
> +
>  #define SII902X_I2C_BUS_ACQUISITION_TIMEOUT_MS	500
>  
>  struct sii902x {
> @@ -90,6 +174,16 @@ struct sii902x {
>  	struct drm_connector connector;
>  	struct gpio_desc *reset_gpio;
>  	struct i2c_mux_core *i2cmux;
> +	/*
> +	 * Mutex protects audio and video functions from interfering
> +	 * each other, by keeping their i2c command sequences atomic.
> +	 */
> +	struct mutex mutex;
> +	struct sii902x_audio {
> +		struct platform_device *pdev;
> +		struct clk *mclk;
> +		u32 i2s_fifo_sequence[4];
> +	} audio;
>  };
>  
>  static int sii902x_read_unlocked(struct i2c_client *i2c, u8 reg, u8 *val)
> @@ -161,8 +255,12 @@ sii902x_connector_detect(struct drm_connector *connector, bool force)
>  	struct sii902x *sii902x = connector_to_sii902x(connector);
>  	unsigned int status;
>  
> +	mutex_lock(&sii902x->mutex);
> +
>  	regmap_read(sii902x->regmap, SII902X_INT_STATUS, &status);
>  
> +	mutex_unlock(&sii902x->mutex);
> +


regmap uses its own lock so we have here and in other places redundant
locks.

Probably you should disable regmap locking (config->disable_locking = 1).


>  	return (status & SII902X_PLUGGED_STATUS) ?
>  	       connector_status_connected : connector_status_disconnected;
>  }
> @@ -184,6 +282,8 @@ static int sii902x_get_modes(struct drm_connector *connector)
>  	struct edid *edid;
>  	int num = 0, ret;
>  
> +	mutex_lock(&sii902x->mutex);
> +
>  	edid = drm_get_edid(connector, sii902x->i2cmux->adapter[0]);
>  	drm_connector_update_edid_property(connector, edid);
>  	if (edid) {
> @@ -197,14 +297,19 @@ static int sii902x_get_modes(struct drm_connector *connector)
>  	ret = drm_display_info_set_bus_formats(&connector->display_info,
>  					       &bus_format, 1);
>  	if (ret)
> -		return ret;
> +		goto error_out;
>  
>  	ret = regmap_update_bits(sii902x->regmap, SII902X_SYS_CTRL_DATA,
>  				 SII902X_SYS_CTRL_OUTPUT_MODE, output_mode);
>  	if (ret)
> -		return ret;
> +		goto error_out;
> +
> +	ret = num;
> +
> +error_out:
> +	mutex_unlock(&sii902x->mutex);
>  
> -	return num;
> +	return ret;
>  }
>  
>  static enum drm_mode_status sii902x_mode_valid(struct drm_connector *connector,
> @@ -224,20 +329,28 @@ static void sii902x_bridge_disable(struct drm_bridge *bridge)
>  {
>  	struct sii902x *sii902x = bridge_to_sii902x(bridge);
>  
> +	mutex_lock(&sii902x->mutex);
> +
>  	regmap_update_bits(sii902x->regmap, SII902X_SYS_CTRL_DATA,
>  			   SII902X_SYS_CTRL_PWR_DWN,
>  			   SII902X_SYS_CTRL_PWR_DWN);
> +
> +	mutex_unlock(&sii902x->mutex);
>  }
>  
>  static void sii902x_bridge_enable(struct drm_bridge *bridge)
>  {
>  	struct sii902x *sii902x = bridge_to_sii902x(bridge);
>  
> +	mutex_lock(&sii902x->mutex);
> +
>  	regmap_update_bits(sii902x->regmap, SII902X_PWR_STATE_CTRL,
>  			   SII902X_AVI_POWER_STATE_MSK,
>  			   SII902X_AVI_POWER_STATE_D(0));
>  	regmap_update_bits(sii902x->regmap, SII902X_SYS_CTRL_DATA,
>  			   SII902X_SYS_CTRL_PWR_DWN, 0);
> +
> +	mutex_unlock(&sii902x->mutex);
>  }
>  
>  static void sii902x_bridge_mode_set(struct drm_bridge *bridge,
> @@ -264,26 +377,31 @@ static void sii902x_bridge_mode_set(struct drm_bridge *bridge,
>  	buf[9] = SII902X_TPI_AVI_INPUT_RANGE_AUTO |
>  		 SII902X_TPI_AVI_INPUT_COLORSPACE_RGB;
>  
> +	mutex_lock(&sii902x->mutex);
> +
>  	ret = regmap_bulk_write(regmap, SII902X_TPI_VIDEO_DATA, buf, 10);
>  	if (ret)
> -		return;
> +		goto out;
>  
>  	ret = drm_hdmi_avi_infoframe_from_display_mode(&frame, adj, false);
>  	if (ret < 0) {
>  		DRM_ERROR("couldn't fill AVI infoframe\n");
> -		return;
> +		goto out;
>  	}
>  
>  	ret = hdmi_avi_infoframe_pack(&frame, buf, sizeof(buf));
>  	if (ret < 0) {
>  		DRM_ERROR("failed to pack AVI infoframe: %d\n", ret);
> -		return;
> +		goto out;
>  	}
>  
>  	/* Do not send the infoframe header, but keep the CRC field. */
>  	regmap_bulk_write(regmap, SII902X_TPI_AVI_INFOFRAME,
>  			  buf + HDMI_INFOFRAME_HEADER_SIZE - 1,
>  			  HDMI_AVI_INFOFRAME_SIZE + 1);
> +
> +out:
> +	mutex_unlock(&sii902x->mutex);
>  }
>  
>  static int sii902x_bridge_attach(struct drm_bridge *bridge)
> @@ -324,6 +442,330 @@ static const struct drm_bridge_funcs sii902x_bridge_funcs = {
>  	.enable = sii902x_bridge_enable,
>  };
>  
> +static int sii902x_mute(struct sii902x *sii902x, bool mute)
> +{
> +	struct device *dev = &sii902x->i2c->dev;
> +	unsigned int val = mute ? SII902X_TPI_AUDIO_MUTE_ENABLE :
> +		SII902X_TPI_AUDIO_MUTE_DISABLE;
> +
> +	dev_dbg(dev, "%s: %s\n", __func__, mute ? "Muted" : "Unmuted");
> +
> +	return regmap_update_bits(sii902x->regmap,
> +				  SII902X_TPI_AUDIO_CONFIG_BYTE2_REG,
> +				  SII902X_TPI_AUDIO_MUTE_ENABLE, val);
> +}
> +
> +static const unsigned int sii902x_mclk_div_table[] = {
> +	128, 256, 384, 512, 768, 1024, 1152, 192 };
> +
> +static int sii902x_select_mclk_div(u8 *i2s_config_reg, unsigned int rate,
> +				   unsigned int mclk)
> +{
> +	unsigned int div = mclk / rate;
> +	int distance = 100000;
> +	u8 i, nearest = 0;
> +
> +	for (i = 0; i < ARRAY_SIZE(sii902x_mclk_div_table); i++) {
> +		unsigned int d = abs(div - sii902x_mclk_div_table[i]);


Using unsigned types in this context seems to be asking for troubles.


> +
> +		if (d >= distance)
> +			continue;
> +
> +		nearest = i;
> +		distance = d;
> +		if (d == 0)
> +			break;
> +	}
> +
> +	*i2s_config_reg |= nearest << 4;
> +
> +	if (distance != 0)
> +		return sii902x_mclk_div_table[nearest];
> +
> +	return 0;


ret value is inconsistent, 0 in case of exact match, closest value
otherwise, code will be cleaner if you return always closest match.


> +}
> +
> +static const struct sii902x_sample_freq {
> +	u32 freq;
> +	u8 val;
> +} sii902x_sample_freq[] = {
> +	{ .freq = 32000,	.val = SII902X_TPI_AUDIO_FREQ_32KHZ },
> +	{ .freq = 44000,	.val = SII902X_TPI_AUDIO_FREQ_44KHZ },
> +	{ .freq = 48000,	.val = SII902X_TPI_AUDIO_FREQ_48KHZ },
> +	{ .freq = 88000,	.val = SII902X_TPI_AUDIO_FREQ_88KHZ },
> +	{ .freq = 96000,	.val = SII902X_TPI_AUDIO_FREQ_96KHZ },
> +	{ .freq = 176000,	.val = SII902X_TPI_AUDIO_FREQ_176KHZ },
> +	{ .freq = 192000,	.val = SII902X_TPI_AUDIO_FREQ_192KHZ },
> +};
> +
> +static int sii902x_audio_hw_params(struct device *dev, void *data,
> +				   struct hdmi_codec_daifmt *daifmt,
> +				   struct hdmi_codec_params *params)
> +{
> +	struct sii902x *sii902x = dev_get_drvdata(dev);
> +	u8 i2s_config_reg = SII902X_TPI_I2S_SD_DIRECTION_MSB_FIRST;
> +	u8 config_byte2_reg = (SII902X_TPI_AUDIO_INTERFACE_I2S |
> +			       SII902X_TPI_AUDIO_MUTE_ENABLE |
> +			       SII902X_TPI_AUDIO_CODING_PCM);
> +	u8 config_byte3_reg = 0;
> +	u8 infoframe_buf[HDMI_INFOFRAME_SIZE(AUDIO)];
> +	unsigned long mclk_rate;
> +	int i, ret;
> +
> +	if (daifmt->bit_clk_master || daifmt->frame_clk_master) {
> +		dev_dbg(dev, "%s: I2S master mode not supported\n", __func__);
> +		return -EINVAL;
> +	}
> +
> +	switch (daifmt->fmt) {
> +	case HDMI_I2S:
> +		i2s_config_reg |= SII902X_TPI_I2S_FIRST_BIT_SHIFT_YES |
> +			SII902X_TPI_I2S_SD_JUSTIFY_LEFT;
> +		break;
> +	case HDMI_RIGHT_J:
> +		i2s_config_reg |= SII902X_TPI_I2S_SD_JUSTIFY_RIGHT;
> +		break;
> +	case HDMI_LEFT_J:
> +		i2s_config_reg |= SII902X_TPI_I2S_SD_JUSTIFY_LEFT;
> +		break;
> +	default:
> +		dev_dbg(dev, "%s: Unsupported i2s format %u\n", __func__,
> +			daifmt->fmt);
> +		return -EINVAL;
> +	}
> +
> +	if (daifmt->bit_clk_inv)
> +		i2s_config_reg |= SII902X_TPI_I2S_SCK_EDGE_FALLING;
> +	else
> +		i2s_config_reg |= SII902X_TPI_I2S_SCK_EDGE_RISING;
> +
> +	if (daifmt->frame_clk_inv)
> +		i2s_config_reg |= SII902X_TPI_I2S_WS_POLARITY_LOW;
> +	else
> +		i2s_config_reg |= SII902X_TPI_I2S_WS_POLARITY_HIGH;
> +
> +	if (params->channels > 2)
> +		config_byte2_reg |= SII902X_TPI_AUDIO_LAYOUT_8_CHANNELS;
> +	else
> +		config_byte2_reg |= SII902X_TPI_AUDIO_LAYOUT_2_CHANNELS;
> +
> +	switch (params->sample_width) {
> +	case 16:
> +		config_byte3_reg |= SII902X_TPI_AUDIO_SAMPLE_SIZE_16;
> +		break;
> +	case 20:
> +		config_byte3_reg |= SII902X_TPI_AUDIO_SAMPLE_SIZE_20;
> +		break;
> +	case 24:
> +	case 32:
> +		config_byte3_reg |= SII902X_TPI_AUDIO_SAMPLE_SIZE_24;
> +		break;
> +	default:
> +		dev_err(dev, "%s: Unsupported sample width %u\n", __func__,
> +			params->sample_width);
> +		return -EINVAL;
> +	}
> +
> +	for (i = 0; i < ARRAY_SIZE(sii902x_sample_freq); i++) {
> +		if (params->sample_rate == sii902x_sample_freq[i].freq) {
> +			config_byte3_reg |= sii902x_sample_freq[i].val;
> +			break;
> +		}
> +	}
> +
> +	ret = clk_prepare_enable(sii902x->audio.mclk);
> +	if (ret) {
> +		dev_err(dev, "Enabling mclk failed: %d\n", ret);
> +		return ret;
> +	}
> +
> +	mclk_rate = clk_get_rate(sii902x->audio.mclk);
> +
> +	ret = sii902x_select_mclk_div(&i2s_config_reg, params->sample_rate,
> +				      mclk_rate);
> +	if (ret)
> +		dev_dbg(dev, "Inaccurate reference clock (%ld/%d != %u)\n",
> +			mclk_rate, ret, params->sample_rate);
> +
> +	mutex_lock(&sii902x->mutex);
> +
> +	ret = regmap_write(sii902x->regmap,
> +			   SII902X_TPI_AUDIO_CONFIG_BYTE2_REG,
> +			   config_byte2_reg);
> +	if (ret < 0)
> +		goto out;
> +
> +	ret = regmap_write(sii902x->regmap, SII902X_TPI_I2S_INPUT_CONFIG_REG,
> +			   i2s_config_reg);
> +	if (ret)
> +		goto out;
> +
> +	for (i = 0; sii902x->audio.i2s_fifo_sequence[i] &&
> +		     i < ARRAY_SIZE(sii902x->audio.i2s_fifo_sequence); i++)
> +		regmap_write(sii902x->regmap,
> +			     SII902X_TPI_I2S_ENABLE_MAPPING_REG,
> +			     sii902x->audio.i2s_fifo_sequence[i]);
> +
> +	ret = regmap_write(sii902x->regmap, SII902X_TPI_AUDIO_CONFIG_BYTE3_REG,
> +			   config_byte3_reg);
> +	if (ret)
> +		goto out;
> +
> +	ret = regmap_bulk_write(sii902x->regmap, SII902X_TPI_I2S_STRM_HDR_BASE,
> +				params->iec.status,
> +				min((size_t) SII902X_TPI_I2S_STRM_HDR_SIZE,
> +				    sizeof(params->iec.status)));
> +	if (ret)
> +		goto out;
> +
> +	ret = hdmi_audio_infoframe_pack(&params->cea, infoframe_buf,
> +					sizeof(infoframe_buf));
> +	if (ret < 0) {
> +		dev_err(dev, "%s: Failed to pack audio infoframe: %d\n",
> +			__func__, ret);
> +		goto out;
> +	}
> +
> +	ret = regmap_bulk_write(sii902x->regmap,
> +				SII902X_TPI_MISC_INFOFRAME_BASE,
> +				infoframe_buf,
> +				min(ret, SII902X_TPI_MISC_INFOFRAME_SIZE));
> +	if (ret)
> +		goto out;
> +
> +	/* Decode Level 0 Packets */
> +	ret = regmap_write(sii902x->regmap, SII902X_IND_SET_PAGE, 0x02);
> +	if (ret)
> +		goto out;
> +
> +	ret = regmap_write(sii902x->regmap, SII902X_IND_OFFSET, 0x24);
> +	if (ret)
> +		goto out;
> +
> +	ret = regmap_write(sii902x->regmap, SII902X_IND_VALUE, 0x02);
> +	if (ret)
> +		goto out;
> +
> +	dev_dbg(dev, "%s: hdmi audio enabled\n", __func__);
> +out:
> +	mutex_unlock(&sii902x->mutex);
> +
> +	if (ret) {
> +		clk_disable_unprepare(sii902x->audio.mclk);
> +		dev_err(dev, "%s: hdmi audio enable failed: %d\n", __func__,
> +			ret);
> +	}
> +
> +	return ret;
> +}
> +
> +static void sii902x_audio_shutdown(struct device *dev, void *data)
> +{
> +	struct sii902x *sii902x = dev_get_drvdata(dev);
> +
> +	mutex_lock(&sii902x->mutex);
> +
> +	regmap_write(sii902x->regmap, SII902X_TPI_AUDIO_CONFIG_BYTE2_REG,
> +		     SII902X_TPI_AUDIO_INTERFACE_DISABLE);
> +
> +	mutex_unlock(&sii902x->mutex);
> +
> +	clk_disable_unprepare(sii902x->audio.mclk);
> +}
> +
> +int sii902x_audio_digital_mute(struct device *dev, void *data, bool enable)
> +{
> +	struct sii902x *sii902x = dev_get_drvdata(dev);
> +
> +	mutex_lock(&sii902x->mutex);
> +
> +	sii902x_mute(sii902x, enable);
> +
> +	mutex_unlock(&sii902x->mutex);
> +
> +	return 0;
> +}
> +
> +static int sii902x_audio_get_eld(struct device *dev, void *data,
> +				 uint8_t *buf, size_t len)
> +{
> +	struct sii902x *sii902x = dev_get_drvdata(dev);
> +
> +	mutex_lock(&sii902x->mutex);
> +
> +	memcpy(buf, sii902x->connector.eld,
> +	       min(sizeof(sii902x->connector.eld), len));


I am not familiar with audio stuff, so forgive my ignorance :)

What happens when this or other audio callbacks is called:

1. before sth is plugged in connector.

2. after unplug.

3. plug/un-plug happens between these calls.


Also I do not see hotplug/unplug notification mechanism to audio
subsystem. It looks suspicious.


Regards

Andrzej



> +
> +	mutex_unlock(&sii902x->mutex);
> +
> +	return 0;
> +}
> +
> +static const struct hdmi_codec_ops sii902x_audio_codec_ops = {
> +	.hw_params = sii902x_audio_hw_params,
> +	.audio_shutdown = sii902x_audio_shutdown,
> +	.digital_mute = sii902x_audio_digital_mute,
> +	.get_eld = sii902x_audio_get_eld,
> +};
> +
> +static int sii902x_audio_codec_init(struct sii902x *sii902x,
> +				    struct device *dev)
> +{
> +	static const u8 audio_fifo_id[] = {
> +		SII902X_TPI_I2S_CONFIG_FIFO0,
> +		SII902X_TPI_I2S_CONFIG_FIFO1,
> +		SII902X_TPI_I2S_CONFIG_FIFO2,
> +		SII902X_TPI_I2S_CONFIG_FIFO3,
> +	};
> +	static const u8 i2s_lane_id[] = {
> +		SII902X_TPI_I2S_SELECT_SD0,
> +		SII902X_TPI_I2S_SELECT_SD1,
> +		SII902X_TPI_I2S_SELECT_SD2,
> +		SII902X_TPI_I2S_SELECT_SD3,
> +	};
> +	struct hdmi_codec_pdata codec_data = {
> +		.ops = &sii902x_audio_codec_ops,
> +		.i2s = 1, /* Only i2s support for now. */
> +		.spdif = 0,
> +		.max_i2s_channels = 0,
> +	};
> +	u8 lanes[4];
> +	u32 num_lanes, i;
> +
> +	num_lanes = of_property_read_variable_u8_array(dev->of_node,
> +						       "sil,i2s-data-lanes",
> +						       lanes, 1,
> +						       ARRAY_SIZE(lanes));
> +	if (num_lanes < 0) {
> +		if (num_lanes == -EINVAL)
> +			dev_dbg(dev,
> +				"%s: No \"sil,i2s-data-lanes\", no audio\n",
> +				__func__);
> +		else
> +			dev_err(dev,
> +				"%s: Error gettin \"sil,i2s-data-lanes\": %d\n",
> +				__func__, num_lanes);
> +		return 0;
> +	}
> +	codec_data.max_i2s_channels = 2 * num_lanes;
> +
> +	for (i = 0; i < num_lanes; i++)
> +		sii902x->audio.i2s_fifo_sequence[i] |= audio_fifo_id[i] |
> +			i2s_lane_id[lanes[i]] |	SII902X_TPI_I2S_FIFO_ENABLE;
> +
> +	if (IS_ERR(sii902x->audio.mclk)) {
> +		dev_err(dev, "%s: No clock (audio mclk) found: %ld\n",
> +			__func__, PTR_ERR(sii902x->audio.mclk));
> +		return 0;
> +	}
> +
> +	sii902x->audio.pdev = platform_device_register_data(
> +		dev, HDMI_CODEC_DRV_NAME, PLATFORM_DEVID_AUTO,
> +		&codec_data, sizeof(codec_data));
> +
> +	return PTR_ERR_OR_ZERO(sii902x->audio.pdev);
> +}
> +
>  static const struct regmap_range sii902x_volatile_ranges[] = {
>  	{ .range_min = 0, .range_max = 0xff },
>  };
> @@ -336,6 +778,7 @@ static const struct regmap_access_table sii902x_volatile_table = {
>  static const struct regmap_config sii902x_regmap_config = {
>  	.reg_bits = 8,
>  	.val_bits = 8,
> +	.max_register = SII902X_TPI_MISC_INFOFRAME_END,
>  	.volatile_table = &sii902x_volatile_table,
>  	.cache_type = REGCACHE_NONE,
>  };
> @@ -508,6 +951,8 @@ static int sii902x_probe(struct i2c_client *client,
>  		return PTR_ERR(sii902x->reset_gpio);
>  	}
>  
> +	mutex_init(&sii902x->mutex);
> +
>  	sii902x_reset(sii902x);
>  
>  	ret = regmap_write(sii902x->regmap, SII902X_REG_TPI_RQB, 0x0);
> @@ -548,6 +993,8 @@ static int sii902x_probe(struct i2c_client *client,
>  	sii902x->bridge.timings = &default_sii902x_timings;
>  	drm_bridge_add(&sii902x->bridge);
>  
> +	sii902x_audio_codec_init(sii902x, dev);
> +
>  	i2c_set_clientdata(client, sii902x);
>  
>  	sii902x->i2cmux = i2c_mux_alloc(client->adapter, dev,


_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v5 6/6] drm/bridge: sii902x: Implement HDMI audio support

[Jyri Sarha]

On 12/04/2019 11:52, Andrzej Hajda wrote:
> On 22.03.2019 09:06, Jyri Sarha wrote:
>> Implement HDMI audio support by using ASoC HDMI codec. The commit
>> implements the necessary callbacks and configuration for the HDMI
>> codec and registers a virtual platform device for the codec to attach.
>>
>> Signed-off-by: Jyri Sarha <jsarha@ti.com>
>> ---
>>  drivers/gpu/drm/bridge/sii902x.c | 459 ++++++++++++++++++++++++++++++-
>>  1 file changed, 453 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/bridge/sii902x.c b/drivers/gpu/drm/bridge/sii902x.c
>> index 358cf81c5ea4..cb12e264111d 100644
>> --- a/drivers/gpu/drm/bridge/sii902x.c
>> +++ b/drivers/gpu/drm/bridge/sii902x.c
>> @@ -27,12 +27,15 @@
>>  #include <linux/i2c.h>
>>  #include <linux/module.h>
>>  #include <linux/regmap.h>
>> +#include <linux/clk.h>
>>  
>>  #include <drm/drmP.h>
>>  #include <drm/drm_atomic_helper.h>
>>  #include <drm/drm_crtc_helper.h>
>>  #include <drm/drm_edid.h>
>>  
>> +#include <sound/hdmi-codec.h>
>> +
>>  #define SII902X_TPI_VIDEO_DATA			0x0
>>  
>>  #define SII902X_TPI_PIXEL_REPETITION		0x8
>> @@ -74,6 +77,77 @@
>>  #define SII902X_AVI_POWER_STATE_MSK		GENMASK(1, 0)
>>  #define SII902X_AVI_POWER_STATE_D(l)		((l) & SII902X_AVI_POWER_STATE_MSK)
>>  
>> +/* Audio  */
>> +#define SII902X_TPI_I2S_ENABLE_MAPPING_REG	0x1f
>> +#define SII902X_TPI_I2S_CONFIG_FIFO0			(0 << 0)
>> +#define SII902X_TPI_I2S_CONFIG_FIFO1			(1 << 0)
>> +#define SII902X_TPI_I2S_CONFIG_FIFO2			(2 << 0)
>> +#define SII902X_TPI_I2S_CONFIG_FIFO3			(3 << 0)
>> +#define SII902X_TPI_I2S_LEFT_RIGHT_SWAP			(1 << 2)
>> +#define SII902X_TPI_I2S_AUTO_DOWNSAMPLE			(1 << 3)
>> +#define SII902X_TPI_I2S_SELECT_SD0			(0 << 4)
>> +#define SII902X_TPI_I2S_SELECT_SD1			(1 << 4)
>> +#define SII902X_TPI_I2S_SELECT_SD2			(2 << 4)
>> +#define SII902X_TPI_I2S_SELECT_SD3			(3 << 4)
>> +#define SII902X_TPI_I2S_FIFO_ENABLE			(1 << 7)
>> +
>> +#define SII902X_TPI_I2S_INPUT_CONFIG_REG	0x20
>> +#define SII902X_TPI_I2S_FIRST_BIT_SHIFT_YES		(0 << 0)
>> +#define SII902X_TPI_I2S_FIRST_BIT_SHIFT_NO		(1 << 0)
>> +#define SII902X_TPI_I2S_SD_DIRECTION_MSB_FIRST		(0 << 1)
>> +#define SII902X_TPI_I2S_SD_DIRECTION_LSB_FIRST		(1 << 1)
>> +#define SII902X_TPI_I2S_SD_JUSTIFY_LEFT			(0 << 2)
>> +#define SII902X_TPI_I2S_SD_JUSTIFY_RIGHT		(1 << 2)
>> +#define SII902X_TPI_I2S_WS_POLARITY_LOW			(0 << 3)
>> +#define SII902X_TPI_I2S_WS_POLARITY_HIGH		(1 << 3)
>> +#define SII902X_TPI_I2S_MCLK_MULTIPLIER_128		(0 << 4)
>> +#define SII902X_TPI_I2S_MCLK_MULTIPLIER_256		(1 << 4)
>> +#define SII902X_TPI_I2S_MCLK_MULTIPLIER_384		(2 << 4)
>> +#define SII902X_TPI_I2S_MCLK_MULTIPLIER_512		(3 << 4)
>> +#define SII902X_TPI_I2S_MCLK_MULTIPLIER_768		(4 << 4)
>> +#define SII902X_TPI_I2S_MCLK_MULTIPLIER_1024		(5 << 4)
>> +#define SII902X_TPI_I2S_MCLK_MULTIPLIER_1152		(6 << 4)
>> +#define SII902X_TPI_I2S_MCLK_MULTIPLIER_192		(7 << 4)
>> +#define SII902X_TPI_I2S_SCK_EDGE_FALLING		(0 << 7)
>> +#define SII902X_TPI_I2S_SCK_EDGE_RISING			(1 << 7)
>> +
>> +#define SII902X_TPI_I2S_STRM_HDR_BASE	0x21
>> +#define SII902X_TPI_I2S_STRM_HDR_SIZE	5
>> +
>> +#define SII902X_TPI_AUDIO_CONFIG_BYTE2_REG	0x26
>> +#define SII902X_TPI_AUDIO_CODING_STREAM_HEADER		(0 << 0)
>> +#define SII902X_TPI_AUDIO_CODING_PCM			(1 << 0)
>> +#define SII902X_TPI_AUDIO_CODING_AC3			(2 << 0)
>> +#define SII902X_TPI_AUDIO_CODING_MPEG1			(3 << 0)
>> +#define SII902X_TPI_AUDIO_CODING_MP3			(4 << 0)
>> +#define SII902X_TPI_AUDIO_CODING_MPEG2			(5 << 0)
>> +#define SII902X_TPI_AUDIO_CODING_AAC			(6 << 0)
>> +#define SII902X_TPI_AUDIO_CODING_DTS			(7 << 0)
>> +#define SII902X_TPI_AUDIO_CODING_ATRAC			(8 << 0)
>> +#define SII902X_TPI_AUDIO_MUTE_DISABLE			(0 << 4)
>> +#define SII902X_TPI_AUDIO_MUTE_ENABLE			(1 << 4)
>> +#define SII902X_TPI_AUDIO_LAYOUT_2_CHANNELS		(0 << 5)
>> +#define SII902X_TPI_AUDIO_LAYOUT_8_CHANNELS		(1 << 5)
>> +#define SII902X_TPI_AUDIO_INTERFACE_DISABLE		(0 << 6)
>> +#define SII902X_TPI_AUDIO_INTERFACE_SPDIF		(1 << 6)
>> +#define SII902X_TPI_AUDIO_INTERFACE_I2S			(2 << 6)
>> +
>> +#define SII902X_TPI_AUDIO_CONFIG_BYTE3_REG	0x27
>> +#define SII902X_TPI_AUDIO_FREQ_STREAM			(0 << 3)
>> +#define SII902X_TPI_AUDIO_FREQ_32KHZ			(1 << 3)
>> +#define SII902X_TPI_AUDIO_FREQ_44KHZ			(2 << 3)
>> +#define SII902X_TPI_AUDIO_FREQ_48KHZ			(3 << 3)
>> +#define SII902X_TPI_AUDIO_FREQ_88KHZ			(4 << 3)
>> +#define SII902X_TPI_AUDIO_FREQ_96KHZ			(5 << 3)
>> +#define SII902X_TPI_AUDIO_FREQ_176KHZ			(6 << 3)
>> +#define SII902X_TPI_AUDIO_FREQ_192KHZ			(7 << 3)
>> +#define SII902X_TPI_AUDIO_SAMPLE_SIZE_STREAM		(0 << 6)
>> +#define SII902X_TPI_AUDIO_SAMPLE_SIZE_16		(1 << 6)
>> +#define SII902X_TPI_AUDIO_SAMPLE_SIZE_20		(2 << 6)
>> +#define SII902X_TPI_AUDIO_SAMPLE_SIZE_24		(3 << 6)
>> +
>> +#define SII902X_TPI_AUDIO_CONFIG_BYTE4_REG	0x28
>> +
>>  #define SII902X_INT_ENABLE			0x3c
>>  #define SII902X_INT_STATUS			0x3d
>>  #define SII902X_HOTPLUG_EVENT			BIT(0)
>> @@ -81,6 +155,16 @@
>>  
>>  #define SII902X_REG_TPI_RQB			0xc7
>>  
>> +/* Indirect internal register access */
>> +#define SII902X_IND_SET_PAGE			0xbc
>> +#define SII902X_IND_OFFSET			0xbd
>> +#define SII902X_IND_VALUE			0xbe
>> +
>> +#define SII902X_TPI_MISC_INFOFRAME_BASE		0xbf
>> +#define SII902X_TPI_MISC_INFOFRAME_END		0xde
>> +#define SII902X_TPI_MISC_INFOFRAME_SIZE	\
>> +	(SII902X_TPI_MISC_INFOFRAME_END - SII902X_TPI_MISC_INFOFRAME_BASE)
>> +
>>  #define SII902X_I2C_BUS_ACQUISITION_TIMEOUT_MS	500
>>  
>>  struct sii902x {
>> @@ -90,6 +174,16 @@ struct sii902x {
>>  	struct drm_connector connector;
>>  	struct gpio_desc *reset_gpio;
>>  	struct i2c_mux_core *i2cmux;
>> +	/*
>> +	 * Mutex protects audio and video functions from interfering
>> +	 * each other, by keeping their i2c command sequences atomic.
>> +	 */
>> +	struct mutex mutex;
>> +	struct sii902x_audio {
>> +		struct platform_device *pdev;
>> +		struct clk *mclk;
>> +		u32 i2s_fifo_sequence[4];
>> +	} audio;
>>  };
>>  
>>  static int sii902x_read_unlocked(struct i2c_client *i2c, u8 reg, u8 *val)
>> @@ -161,8 +255,12 @@ sii902x_connector_detect(struct drm_connector *connector, bool force)
>>  	struct sii902x *sii902x = connector_to_sii902x(connector);
>>  	unsigned int status;
>>  
>> +	mutex_lock(&sii902x->mutex);
>> +
>>  	regmap_read(sii902x->regmap, SII902X_INT_STATUS, &status);
>>  
>> +	mutex_unlock(&sii902x->mutex);
>> +
> 
> 
> regmap uses its own lock so we have here and in other places redundant
> locks.
> 
> Probably you should disable regmap locking (config->disable_locking = 1).
> 

Makes sense. I'll add that.

> 
>>  	return (status & SII902X_PLUGGED_STATUS) ?
>>  	       connector_status_connected : connector_status_disconnected;
>>  }
>> @@ -184,6 +282,8 @@ static int sii902x_get_modes(struct drm_connector *connector)
>>  	struct edid *edid;
>>  	int num = 0, ret;
>>  
>> +	mutex_lock(&sii902x->mutex);
>> +
>>  	edid = drm_get_edid(connector, sii902x->i2cmux->adapter[0]);
>>  	drm_connector_update_edid_property(connector, edid);
>>  	if (edid) {
>> @@ -197,14 +297,19 @@ static int sii902x_get_modes(struct drm_connector *connector)
>>  	ret = drm_display_info_set_bus_formats(&connector->display_info,
>>  					       &bus_format, 1);
>>  	if (ret)
>> -		return ret;
>> +		goto error_out;
>>  
>>  	ret = regmap_update_bits(sii902x->regmap, SII902X_SYS_CTRL_DATA,
>>  				 SII902X_SYS_CTRL_OUTPUT_MODE, output_mode);
>>  	if (ret)
>> -		return ret;
>> +		goto error_out;
>> +
>> +	ret = num;
>> +
>> +error_out:
>> +	mutex_unlock(&sii902x->mutex);
>>  
>> -	return num;
>> +	return ret;
>>  }
>>  
>>  static enum drm_mode_status sii902x_mode_valid(struct drm_connector *connector,
>> @@ -224,20 +329,28 @@ static void sii902x_bridge_disable(struct drm_bridge *bridge)
>>  {
>>  	struct sii902x *sii902x = bridge_to_sii902x(bridge);
>>  
>> +	mutex_lock(&sii902x->mutex);
>> +
>>  	regmap_update_bits(sii902x->regmap, SII902X_SYS_CTRL_DATA,
>>  			   SII902X_SYS_CTRL_PWR_DWN,
>>  			   SII902X_SYS_CTRL_PWR_DWN);
>> +
>> +	mutex_unlock(&sii902x->mutex);
>>  }
>>  
>>  static void sii902x_bridge_enable(struct drm_bridge *bridge)
>>  {
>>  	struct sii902x *sii902x = bridge_to_sii902x(bridge);
>>  
>> +	mutex_lock(&sii902x->mutex);
>> +
>>  	regmap_update_bits(sii902x->regmap, SII902X_PWR_STATE_CTRL,
>>  			   SII902X_AVI_POWER_STATE_MSK,
>>  			   SII902X_AVI_POWER_STATE_D(0));
>>  	regmap_update_bits(sii902x->regmap, SII902X_SYS_CTRL_DATA,
>>  			   SII902X_SYS_CTRL_PWR_DWN, 0);
>> +
>> +	mutex_unlock(&sii902x->mutex);
>>  }
>>  
>>  static void sii902x_bridge_mode_set(struct drm_bridge *bridge,
>> @@ -264,26 +377,31 @@ static void sii902x_bridge_mode_set(struct drm_bridge *bridge,
>>  	buf[9] = SII902X_TPI_AVI_INPUT_RANGE_AUTO |
>>  		 SII902X_TPI_AVI_INPUT_COLORSPACE_RGB;
>>  
>> +	mutex_lock(&sii902x->mutex);
>> +
>>  	ret = regmap_bulk_write(regmap, SII902X_TPI_VIDEO_DATA, buf, 10);
>>  	if (ret)
>> -		return;
>> +		goto out;
>>  
>>  	ret = drm_hdmi_avi_infoframe_from_display_mode(&frame, adj, false);
>>  	if (ret < 0) {
>>  		DRM_ERROR("couldn't fill AVI infoframe\n");
>> -		return;
>> +		goto out;
>>  	}
>>  
>>  	ret = hdmi_avi_infoframe_pack(&frame, buf, sizeof(buf));
>>  	if (ret < 0) {
>>  		DRM_ERROR("failed to pack AVI infoframe: %d\n", ret);
>> -		return;
>> +		goto out;
>>  	}
>>  
>>  	/* Do not send the infoframe header, but keep the CRC field. */
>>  	regmap_bulk_write(regmap, SII902X_TPI_AVI_INFOFRAME,
>>  			  buf + HDMI_INFOFRAME_HEADER_SIZE - 1,
>>  			  HDMI_AVI_INFOFRAME_SIZE + 1);
>> +
>> +out:
>> +	mutex_unlock(&sii902x->mutex);
>>  }
>>  
>>  static int sii902x_bridge_attach(struct drm_bridge *bridge)
>> @@ -324,6 +442,330 @@ static const struct drm_bridge_funcs sii902x_bridge_funcs = {
>>  	.enable = sii902x_bridge_enable,
>>  };
>>  
>> +static int sii902x_mute(struct sii902x *sii902x, bool mute)
>> +{
>> +	struct device *dev = &sii902x->i2c->dev;
>> +	unsigned int val = mute ? SII902X_TPI_AUDIO_MUTE_ENABLE :
>> +		SII902X_TPI_AUDIO_MUTE_DISABLE;
>> +
>> +	dev_dbg(dev, "%s: %s\n", __func__, mute ? "Muted" : "Unmuted");
>> +
>> +	return regmap_update_bits(sii902x->regmap,
>> +				  SII902X_TPI_AUDIO_CONFIG_BYTE2_REG,
>> +				  SII902X_TPI_AUDIO_MUTE_ENABLE, val);
>> +}
>> +
>> +static const unsigned int sii902x_mclk_div_table[] = {
>> +	128, 256, 384, 512, 768, 1024, 1152, 192 };
>> +
>> +static int sii902x_select_mclk_div(u8 *i2s_config_reg, unsigned int rate,
>> +				   unsigned int mclk)
>> +{
>> +	unsigned int div = mclk / rate;
>> +	int distance = 100000;
>> +	u8 i, nearest = 0;
>> +
>> +	for (i = 0; i < ARRAY_SIZE(sii902x_mclk_div_table); i++) {
>> +		unsigned int d = abs(div - sii902x_mclk_div_table[i]);
> 
> 
> Using unsigned types in this context seems to be asking for troubles.
> 

Why? Isn't return value of abs() by definition unsigned? Using signed
integers when comparing absolute distances would seem awkward to me.

> 
>> +
>> +		if (d >= distance)
>> +			continue;
>> +
>> +		nearest = i;
>> +		distance = d;
>> +		if (d == 0)
>> +			break;
>> +	}
>> +
>> +	*i2s_config_reg |= nearest << 4;
>> +
>> +	if (distance != 0)
>> +		return sii902x_mclk_div_table[nearest];
>> +
>> +	return 0;
> 
> 
> ret value is inconsistent, 0 in case of exact match, closest value
> otherwise, code will be cleaner if you return always closest match.
> 

Then I do not have the information to print a warning if an exact match
was not found. The return value is only used in that warning message,
the register configuration value is always delivered trough
*i2s_config_reg. I do not see any inconsistency here.

> 
>> +}
>> +
>> +static const struct sii902x_sample_freq {
>> +	u32 freq;
>> +	u8 val;
>> +} sii902x_sample_freq[] = {
>> +	{ .freq = 32000,	.val = SII902X_TPI_AUDIO_FREQ_32KHZ },
>> +	{ .freq = 44000,	.val = SII902X_TPI_AUDIO_FREQ_44KHZ },
>> +	{ .freq = 48000,	.val = SII902X_TPI_AUDIO_FREQ_48KHZ },
>> +	{ .freq = 88000,	.val = SII902X_TPI_AUDIO_FREQ_88KHZ },
>> +	{ .freq = 96000,	.val = SII902X_TPI_AUDIO_FREQ_96KHZ },
>> +	{ .freq = 176000,	.val = SII902X_TPI_AUDIO_FREQ_176KHZ },
>> +	{ .freq = 192000,	.val = SII902X_TPI_AUDIO_FREQ_192KHZ },
>> +};
>> +
>> +static int sii902x_audio_hw_params(struct device *dev, void *data,
>> +				   struct hdmi_codec_daifmt *daifmt,
>> +				   struct hdmi_codec_params *params)
>> +{
>> +	struct sii902x *sii902x = dev_get_drvdata(dev);
>> +	u8 i2s_config_reg = SII902X_TPI_I2S_SD_DIRECTION_MSB_FIRST;
>> +	u8 config_byte2_reg = (SII902X_TPI_AUDIO_INTERFACE_I2S |
>> +			       SII902X_TPI_AUDIO_MUTE_ENABLE |
>> +			       SII902X_TPI_AUDIO_CODING_PCM);
>> +	u8 config_byte3_reg = 0;
>> +	u8 infoframe_buf[HDMI_INFOFRAME_SIZE(AUDIO)];
>> +	unsigned long mclk_rate;
>> +	int i, ret;
>> +
>> +	if (daifmt->bit_clk_master || daifmt->frame_clk_master) {
>> +		dev_dbg(dev, "%s: I2S master mode not supported\n", __func__);
>> +		return -EINVAL;
>> +	}
>> +
>> +	switch (daifmt->fmt) {
>> +	case HDMI_I2S:
>> +		i2s_config_reg |= SII902X_TPI_I2S_FIRST_BIT_SHIFT_YES |
>> +			SII902X_TPI_I2S_SD_JUSTIFY_LEFT;
>> +		break;
>> +	case HDMI_RIGHT_J:
>> +		i2s_config_reg |= SII902X_TPI_I2S_SD_JUSTIFY_RIGHT;
>> +		break;
>> +	case HDMI_LEFT_J:
>> +		i2s_config_reg |= SII902X_TPI_I2S_SD_JUSTIFY_LEFT;
>> +		break;
>> +	default:
>> +		dev_dbg(dev, "%s: Unsupported i2s format %u\n", __func__,
>> +			daifmt->fmt);
>> +		return -EINVAL;
>> +	}
>> +
>> +	if (daifmt->bit_clk_inv)
>> +		i2s_config_reg |= SII902X_TPI_I2S_SCK_EDGE_FALLING;
>> +	else
>> +		i2s_config_reg |= SII902X_TPI_I2S_SCK_EDGE_RISING;
>> +
>> +	if (daifmt->frame_clk_inv)
>> +		i2s_config_reg |= SII902X_TPI_I2S_WS_POLARITY_LOW;
>> +	else
>> +		i2s_config_reg |= SII902X_TPI_I2S_WS_POLARITY_HIGH;
>> +
>> +	if (params->channels > 2)
>> +		config_byte2_reg |= SII902X_TPI_AUDIO_LAYOUT_8_CHANNELS;
>> +	else
>> +		config_byte2_reg |= SII902X_TPI_AUDIO_LAYOUT_2_CHANNELS;
>> +
>> +	switch (params->sample_width) {
>> +	case 16:
>> +		config_byte3_reg |= SII902X_TPI_AUDIO_SAMPLE_SIZE_16;
>> +		break;
>> +	case 20:
>> +		config_byte3_reg |= SII902X_TPI_AUDIO_SAMPLE_SIZE_20;
>> +		break;
>> +	case 24:
>> +	case 32:
>> +		config_byte3_reg |= SII902X_TPI_AUDIO_SAMPLE_SIZE_24;
>> +		break;
>> +	default:
>> +		dev_err(dev, "%s: Unsupported sample width %u\n", __func__,
>> +			params->sample_width);
>> +		return -EINVAL;
>> +	}
>> +
>> +	for (i = 0; i < ARRAY_SIZE(sii902x_sample_freq); i++) {
>> +		if (params->sample_rate == sii902x_sample_freq[i].freq) {
>> +			config_byte3_reg |= sii902x_sample_freq[i].val;
>> +			break;
>> +		}
>> +	}
>> +
>> +	ret = clk_prepare_enable(sii902x->audio.mclk);
>> +	if (ret) {
>> +		dev_err(dev, "Enabling mclk failed: %d\n", ret);
>> +		return ret;
>> +	}
>> +
>> +	mclk_rate = clk_get_rate(sii902x->audio.mclk);
>> +
>> +	ret = sii902x_select_mclk_div(&i2s_config_reg, params->sample_rate,
>> +				      mclk_rate);
>> +	if (ret)
>> +		dev_dbg(dev, "Inaccurate reference clock (%ld/%d != %u)\n",
>> +			mclk_rate, ret, params->sample_rate);
>> +
>> +	mutex_lock(&sii902x->mutex);
>> +
>> +	ret = regmap_write(sii902x->regmap,
>> +			   SII902X_TPI_AUDIO_CONFIG_BYTE2_REG,
>> +			   config_byte2_reg);
>> +	if (ret < 0)
>> +		goto out;
>> +
>> +	ret = regmap_write(sii902x->regmap, SII902X_TPI_I2S_INPUT_CONFIG_REG,
>> +			   i2s_config_reg);
>> +	if (ret)
>> +		goto out;
>> +
>> +	for (i = 0; sii902x->audio.i2s_fifo_sequence[i] &&
>> +		     i < ARRAY_SIZE(sii902x->audio.i2s_fifo_sequence); i++)
>> +		regmap_write(sii902x->regmap,
>> +			     SII902X_TPI_I2S_ENABLE_MAPPING_REG,
>> +			     sii902x->audio.i2s_fifo_sequence[i]);
>> +
>> +	ret = regmap_write(sii902x->regmap, SII902X_TPI_AUDIO_CONFIG_BYTE3_REG,
>> +			   config_byte3_reg);
>> +	if (ret)
>> +		goto out;
>> +
>> +	ret = regmap_bulk_write(sii902x->regmap, SII902X_TPI_I2S_STRM_HDR_BASE,
>> +				params->iec.status,
>> +				min((size_t) SII902X_TPI_I2S_STRM_HDR_SIZE,
>> +				    sizeof(params->iec.status)));
>> +	if (ret)
>> +		goto out;
>> +
>> +	ret = hdmi_audio_infoframe_pack(&params->cea, infoframe_buf,
>> +					sizeof(infoframe_buf));
>> +	if (ret < 0) {
>> +		dev_err(dev, "%s: Failed to pack audio infoframe: %d\n",
>> +			__func__, ret);
>> +		goto out;
>> +	}
>> +
>> +	ret = regmap_bulk_write(sii902x->regmap,
>> +				SII902X_TPI_MISC_INFOFRAME_BASE,
>> +				infoframe_buf,
>> +				min(ret, SII902X_TPI_MISC_INFOFRAME_SIZE));
>> +	if (ret)
>> +		goto out;
>> +
>> +	/* Decode Level 0 Packets */
>> +	ret = regmap_write(sii902x->regmap, SII902X_IND_SET_PAGE, 0x02);
>> +	if (ret)
>> +		goto out;
>> +
>> +	ret = regmap_write(sii902x->regmap, SII902X_IND_OFFSET, 0x24);
>> +	if (ret)
>> +		goto out;
>> +
>> +	ret = regmap_write(sii902x->regmap, SII902X_IND_VALUE, 0x02);
>> +	if (ret)
>> +		goto out;
>> +
>> +	dev_dbg(dev, "%s: hdmi audio enabled\n", __func__);
>> +out:
>> +	mutex_unlock(&sii902x->mutex);
>> +
>> +	if (ret) {
>> +		clk_disable_unprepare(sii902x->audio.mclk);
>> +		dev_err(dev, "%s: hdmi audio enable failed: %d\n", __func__,
>> +			ret);
>> +	}
>> +
>> +	return ret;
>> +}
>> +
>> +static void sii902x_audio_shutdown(struct device *dev, void *data)
>> +{
>> +	struct sii902x *sii902x = dev_get_drvdata(dev);
>> +
>> +	mutex_lock(&sii902x->mutex);
>> +
>> +	regmap_write(sii902x->regmap, SII902X_TPI_AUDIO_CONFIG_BYTE2_REG,
>> +		     SII902X_TPI_AUDIO_INTERFACE_DISABLE);
>> +
>> +	mutex_unlock(&sii902x->mutex);
>> +
>> +	clk_disable_unprepare(sii902x->audio.mclk);
>> +}
>> +
>> +int sii902x_audio_digital_mute(struct device *dev, void *data, bool enable)
>> +{
>> +	struct sii902x *sii902x = dev_get_drvdata(dev);
>> +
>> +	mutex_lock(&sii902x->mutex);
>> +
>> +	sii902x_mute(sii902x, enable);
>> +
>> +	mutex_unlock(&sii902x->mutex);
>> +
>> +	return 0;
>> +}
>> +
>> +static int sii902x_audio_get_eld(struct device *dev, void *data,
>> +				 uint8_t *buf, size_t len)
>> +{
>> +	struct sii902x *sii902x = dev_get_drvdata(dev);
>> +
>> +	mutex_lock(&sii902x->mutex);
>> +
>> +	memcpy(buf, sii902x->connector.eld,
>> +	       min(sizeof(sii902x->connector.eld), len));
> 
> 
> I am not familiar with audio stuff, so forgive my ignorance :)
> 
> What happens when this or other audio callbacks is called:
> 
> 1. before sth is plugged in connector.
> > 2. after unplug.
> 
> 3. plug/un-plug happens between these calls.
>

The audio side calls this at the time of audio playback request. If
there is no valid eld at that time, the playback request fails. The eld
is only used at the playback start time, never after it.

When an audio playback is running it we do not do anything to until the
playback is stopped. E.g. if plug/un-plug happens we just keep playing
with the original configuration (my first version had an abort call back
to abort playback in unplug event, but that was refused in the reviews).

If the cable is never plugged again, or the new display does not have
audio support, the i2s just keeps banging the bits to dev null and audio
data keeps drainging away. When cable is again plugged to audio capable
display, it continues playing there (and it never stopped in between.

This comes closest to already established behaviour with already
existing HDMI audio implementations. This is also usually the preferred
behavior, because resebles how video stream playback behaves.

I know very well that this is not the perfect solution. The perfect
solution would have the abort callback there, and some mechanism above
that to deal with the audio playback being aborted and the device
becoming available again. This would require co-operation from
userspace. However, the world is not ready and this is a best effort
implementation.

> 
> Also I do not see hotplug/unplug notification mechanism to audio
> subsystem. It looks suspicious.
> 

Yes, as mentioned above, I had it in my first version of hdmi-codec, but
it was never accepted to mainline. However, in practice this has not
been a problem. The HDMI audio implementation on the encoder chips I
have seen is usually quite independent from the video side workings.

> 
> Regards
> 
> Andrzej
> 

Thanks,
Jyri

> 
> 
>> +
>> +	mutex_unlock(&sii902x->mutex);
>> +
>> +	return 0;
>> +}
>> +
>> +static const struct hdmi_codec_ops sii902x_audio_codec_ops = {
>> +	.hw_params = sii902x_audio_hw_params,
>> +	.audio_shutdown = sii902x_audio_shutdown,
>> +	.digital_mute = sii902x_audio_digital_mute,
>> +	.get_eld = sii902x_audio_get_eld,
>> +};
>> +
>> +static int sii902x_audio_codec_init(struct sii902x *sii902x,
>> +				    struct device *dev)
>> +{
>> +	static const u8 audio_fifo_id[] = {
>> +		SII902X_TPI_I2S_CONFIG_FIFO0,
>> +		SII902X_TPI_I2S_CONFIG_FIFO1,
>> +		SII902X_TPI_I2S_CONFIG_FIFO2,
>> +		SII902X_TPI_I2S_CONFIG_FIFO3,
>> +	};
>> +	static const u8 i2s_lane_id[] = {
>> +		SII902X_TPI_I2S_SELECT_SD0,
>> +		SII902X_TPI_I2S_SELECT_SD1,
>> +		SII902X_TPI_I2S_SELECT_SD2,
>> +		SII902X_TPI_I2S_SELECT_SD3,
>> +	};
>> +	struct hdmi_codec_pdata codec_data = {
>> +		.ops = &sii902x_audio_codec_ops,
>> +		.i2s = 1, /* Only i2s support for now. */
>> +		.spdif = 0,
>> +		.max_i2s_channels = 0,
>> +	};
>> +	u8 lanes[4];
>> +	u32 num_lanes, i;
>> +
>> +	num_lanes = of_property_read_variable_u8_array(dev->of_node,
>> +						       "sil,i2s-data-lanes",
>> +						       lanes, 1,
>> +						       ARRAY_SIZE(lanes));
>> +	if (num_lanes < 0) {
>> +		if (num_lanes == -EINVAL)
>> +			dev_dbg(dev,
>> +				"%s: No \"sil,i2s-data-lanes\", no audio\n",
>> +				__func__);
>> +		else
>> +			dev_err(dev,
>> +				"%s: Error gettin \"sil,i2s-data-lanes\": %d\n",
>> +				__func__, num_lanes);
>> +		return 0;
>> +	}
>> +	codec_data.max_i2s_channels = 2 * num_lanes;
>> +
>> +	for (i = 0; i < num_lanes; i++)
>> +		sii902x->audio.i2s_fifo_sequence[i] |= audio_fifo_id[i] |
>> +			i2s_lane_id[lanes[i]] |	SII902X_TPI_I2S_FIFO_ENABLE;
>> +
>> +	if (IS_ERR(sii902x->audio.mclk)) {
>> +		dev_err(dev, "%s: No clock (audio mclk) found: %ld\n",
>> +			__func__, PTR_ERR(sii902x->audio.mclk));
>> +		return 0;
>> +	}
>> +
>> +	sii902x->audio.pdev = platform_device_register_data(
>> +		dev, HDMI_CODEC_DRV_NAME, PLATFORM_DEVID_AUTO,
>> +		&codec_data, sizeof(codec_data));
>> +
>> +	return PTR_ERR_OR_ZERO(sii902x->audio.pdev);
>> +}
>> +
>>  static const struct regmap_range sii902x_volatile_ranges[] = {
>>  	{ .range_min = 0, .range_max = 0xff },
>>  };
>> @@ -336,6 +778,7 @@ static const struct regmap_access_table sii902x_volatile_table = {
>>  static const struct regmap_config sii902x_regmap_config = {
>>  	.reg_bits = 8,
>>  	.val_bits = 8,
>> +	.max_register = SII902X_TPI_MISC_INFOFRAME_END,
>>  	.volatile_table = &sii902x_volatile_table,
>>  	.cache_type = REGCACHE_NONE,
>>  };
>> @@ -508,6 +951,8 @@ static int sii902x_probe(struct i2c_client *client,
>>  		return PTR_ERR(sii902x->reset_gpio);
>>  	}
>>  
>> +	mutex_init(&sii902x->mutex);
>> +
>>  	sii902x_reset(sii902x);
>>  
>>  	ret = regmap_write(sii902x->regmap, SII902X_REG_TPI_RQB, 0x0);
>> @@ -548,6 +993,8 @@ static int sii902x_probe(struct i2c_client *client,
>>  	sii902x->bridge.timings = &default_sii902x_timings;
>>  	drm_bridge_add(&sii902x->bridge);
>>  
>> +	sii902x_audio_codec_init(sii902x, dev);
>> +
>>  	i2c_set_clientdata(client, sii902x);
>>  
>>  	sii902x->i2cmux = i2c_mux_alloc(client->adapter, dev,
> 
> 


-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v5 6/6] drm/bridge: sii902x: Implement HDMI audio support

[Andrzej Hajda]

On 12.04.2019 13:53, Jyri Sarha wrote:
> On 12/04/2019 11:52, Andrzej Hajda wrote:
>> On 22.03.2019 09:06, Jyri Sarha wrote:
>>> Implement HDMI audio support by using ASoC HDMI codec. The commit
>>> implements the necessary callbacks and configuration for the HDMI
>>> codec and registers a virtual platform device for the codec to attach.
>>>
>>> Signed-off-by: Jyri Sarha <jsarha@ti.com>
>>> ---
>>>  drivers/gpu/drm/bridge/sii902x.c | 459 ++++++++++++++++++++++++++++++-
>>>  1 file changed, 453 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/bridge/sii902x.c b/drivers/gpu/drm/bridge/sii902x.c
>>> index 358cf81c5ea4..cb12e264111d 100644
>>> --- a/drivers/gpu/drm/bridge/sii902x.c
>>> +++ b/drivers/gpu/drm/bridge/sii902x.c
>>> @@ -27,12 +27,15 @@
>>>  #include <linux/i2c.h>
>>>  #include <linux/module.h>
>>>  #include <linux/regmap.h>
>>> +#include <linux/clk.h>
>>>  
>>>  #include <drm/drmP.h>
>>>  #include <drm/drm_atomic_helper.h>
>>>  #include <drm/drm_crtc_helper.h>
>>>  #include <drm/drm_edid.h>
>>>  
>>> +#include <sound/hdmi-codec.h>
>>> +
>>>  #define SII902X_TPI_VIDEO_DATA			0x0
>>>  
>>>  #define SII902X_TPI_PIXEL_REPETITION		0x8
>>> @@ -74,6 +77,77 @@
>>>  #define SII902X_AVI_POWER_STATE_MSK		GENMASK(1, 0)
>>>  #define SII902X_AVI_POWER_STATE_D(l)		((l) & SII902X_AVI_POWER_STATE_MSK)
>>>  
>>> +/* Audio  */
>>> +#define SII902X_TPI_I2S_ENABLE_MAPPING_REG	0x1f
>>> +#define SII902X_TPI_I2S_CONFIG_FIFO0			(0 << 0)
>>> +#define SII902X_TPI_I2S_CONFIG_FIFO1			(1 << 0)
>>> +#define SII902X_TPI_I2S_CONFIG_FIFO2			(2 << 0)
>>> +#define SII902X_TPI_I2S_CONFIG_FIFO3			(3 << 0)
>>> +#define SII902X_TPI_I2S_LEFT_RIGHT_SWAP			(1 << 2)
>>> +#define SII902X_TPI_I2S_AUTO_DOWNSAMPLE			(1 << 3)
>>> +#define SII902X_TPI_I2S_SELECT_SD0			(0 << 4)
>>> +#define SII902X_TPI_I2S_SELECT_SD1			(1 << 4)
>>> +#define SII902X_TPI_I2S_SELECT_SD2			(2 << 4)
>>> +#define SII902X_TPI_I2S_SELECT_SD3			(3 << 4)
>>> +#define SII902X_TPI_I2S_FIFO_ENABLE			(1 << 7)
>>> +
>>> +#define SII902X_TPI_I2S_INPUT_CONFIG_REG	0x20
>>> +#define SII902X_TPI_I2S_FIRST_BIT_SHIFT_YES		(0 << 0)
>>> +#define SII902X_TPI_I2S_FIRST_BIT_SHIFT_NO		(1 << 0)
>>> +#define SII902X_TPI_I2S_SD_DIRECTION_MSB_FIRST		(0 << 1)
>>> +#define SII902X_TPI_I2S_SD_DIRECTION_LSB_FIRST		(1 << 1)
>>> +#define SII902X_TPI_I2S_SD_JUSTIFY_LEFT			(0 << 2)
>>> +#define SII902X_TPI_I2S_SD_JUSTIFY_RIGHT		(1 << 2)
>>> +#define SII902X_TPI_I2S_WS_POLARITY_LOW			(0 << 3)
>>> +#define SII902X_TPI_I2S_WS_POLARITY_HIGH		(1 << 3)
>>> +#define SII902X_TPI_I2S_MCLK_MULTIPLIER_128		(0 << 4)
>>> +#define SII902X_TPI_I2S_MCLK_MULTIPLIER_256		(1 << 4)
>>> +#define SII902X_TPI_I2S_MCLK_MULTIPLIER_384		(2 << 4)
>>> +#define SII902X_TPI_I2S_MCLK_MULTIPLIER_512		(3 << 4)
>>> +#define SII902X_TPI_I2S_MCLK_MULTIPLIER_768		(4 << 4)
>>> +#define SII902X_TPI_I2S_MCLK_MULTIPLIER_1024		(5 << 4)
>>> +#define SII902X_TPI_I2S_MCLK_MULTIPLIER_1152		(6 << 4)
>>> +#define SII902X_TPI_I2S_MCLK_MULTIPLIER_192		(7 << 4)
>>> +#define SII902X_TPI_I2S_SCK_EDGE_FALLING		(0 << 7)
>>> +#define SII902X_TPI_I2S_SCK_EDGE_RISING			(1 << 7)
>>> +
>>> +#define SII902X_TPI_I2S_STRM_HDR_BASE	0x21
>>> +#define SII902X_TPI_I2S_STRM_HDR_SIZE	5
>>> +
>>> +#define SII902X_TPI_AUDIO_CONFIG_BYTE2_REG	0x26
>>> +#define SII902X_TPI_AUDIO_CODING_STREAM_HEADER		(0 << 0)
>>> +#define SII902X_TPI_AUDIO_CODING_PCM			(1 << 0)
>>> +#define SII902X_TPI_AUDIO_CODING_AC3			(2 << 0)
>>> +#define SII902X_TPI_AUDIO_CODING_MPEG1			(3 << 0)
>>> +#define SII902X_TPI_AUDIO_CODING_MP3			(4 << 0)
>>> +#define SII902X_TPI_AUDIO_CODING_MPEG2			(5 << 0)
>>> +#define SII902X_TPI_AUDIO_CODING_AAC			(6 << 0)
>>> +#define SII902X_TPI_AUDIO_CODING_DTS			(7 << 0)
>>> +#define SII902X_TPI_AUDIO_CODING_ATRAC			(8 << 0)
>>> +#define SII902X_TPI_AUDIO_MUTE_DISABLE			(0 << 4)
>>> +#define SII902X_TPI_AUDIO_MUTE_ENABLE			(1 << 4)
>>> +#define SII902X_TPI_AUDIO_LAYOUT_2_CHANNELS		(0 << 5)
>>> +#define SII902X_TPI_AUDIO_LAYOUT_8_CHANNELS		(1 << 5)
>>> +#define SII902X_TPI_AUDIO_INTERFACE_DISABLE		(0 << 6)
>>> +#define SII902X_TPI_AUDIO_INTERFACE_SPDIF		(1 << 6)
>>> +#define SII902X_TPI_AUDIO_INTERFACE_I2S			(2 << 6)
>>> +
>>> +#define SII902X_TPI_AUDIO_CONFIG_BYTE3_REG	0x27
>>> +#define SII902X_TPI_AUDIO_FREQ_STREAM			(0 << 3)
>>> +#define SII902X_TPI_AUDIO_FREQ_32KHZ			(1 << 3)
>>> +#define SII902X_TPI_AUDIO_FREQ_44KHZ			(2 << 3)
>>> +#define SII902X_TPI_AUDIO_FREQ_48KHZ			(3 << 3)
>>> +#define SII902X_TPI_AUDIO_FREQ_88KHZ			(4 << 3)
>>> +#define SII902X_TPI_AUDIO_FREQ_96KHZ			(5 << 3)
>>> +#define SII902X_TPI_AUDIO_FREQ_176KHZ			(6 << 3)
>>> +#define SII902X_TPI_AUDIO_FREQ_192KHZ			(7 << 3)
>>> +#define SII902X_TPI_AUDIO_SAMPLE_SIZE_STREAM		(0 << 6)
>>> +#define SII902X_TPI_AUDIO_SAMPLE_SIZE_16		(1 << 6)
>>> +#define SII902X_TPI_AUDIO_SAMPLE_SIZE_20		(2 << 6)
>>> +#define SII902X_TPI_AUDIO_SAMPLE_SIZE_24		(3 << 6)
>>> +
>>> +#define SII902X_TPI_AUDIO_CONFIG_BYTE4_REG	0x28
>>> +
>>>  #define SII902X_INT_ENABLE			0x3c
>>>  #define SII902X_INT_STATUS			0x3d
>>>  #define SII902X_HOTPLUG_EVENT			BIT(0)
>>> @@ -81,6 +155,16 @@
>>>  
>>>  #define SII902X_REG_TPI_RQB			0xc7
>>>  
>>> +/* Indirect internal register access */
>>> +#define SII902X_IND_SET_PAGE			0xbc
>>> +#define SII902X_IND_OFFSET			0xbd
>>> +#define SII902X_IND_VALUE			0xbe
>>> +
>>> +#define SII902X_TPI_MISC_INFOFRAME_BASE		0xbf
>>> +#define SII902X_TPI_MISC_INFOFRAME_END		0xde
>>> +#define SII902X_TPI_MISC_INFOFRAME_SIZE	\
>>> +	(SII902X_TPI_MISC_INFOFRAME_END - SII902X_TPI_MISC_INFOFRAME_BASE)
>>> +
>>>  #define SII902X_I2C_BUS_ACQUISITION_TIMEOUT_MS	500
>>>  
>>>  struct sii902x {
>>> @@ -90,6 +174,16 @@ struct sii902x {
>>>  	struct drm_connector connector;
>>>  	struct gpio_desc *reset_gpio;
>>>  	struct i2c_mux_core *i2cmux;
>>> +	/*
>>> +	 * Mutex protects audio and video functions from interfering
>>> +	 * each other, by keeping their i2c command sequences atomic.
>>> +	 */
>>> +	struct mutex mutex;
>>> +	struct sii902x_audio {
>>> +		struct platform_device *pdev;
>>> +		struct clk *mclk;
>>> +		u32 i2s_fifo_sequence[4];
>>> +	} audio;
>>>  };
>>>  
>>>  static int sii902x_read_unlocked(struct i2c_client *i2c, u8 reg, u8 *val)
>>> @@ -161,8 +255,12 @@ sii902x_connector_detect(struct drm_connector *connector, bool force)
>>>  	struct sii902x *sii902x = connector_to_sii902x(connector);
>>>  	unsigned int status;
>>>  
>>> +	mutex_lock(&sii902x->mutex);
>>> +
>>>  	regmap_read(sii902x->regmap, SII902X_INT_STATUS, &status);
>>>  
>>> +	mutex_unlock(&sii902x->mutex);
>>> +
>>
>> regmap uses its own lock so we have here and in other places redundant
>> locks.
>>
>> Probably you should disable regmap locking (config->disable_locking = 1).
>>
> Makes sense. I'll add that.
>
>>>  	return (status & SII902X_PLUGGED_STATUS) ?
>>>  	       connector_status_connected : connector_status_disconnected;
>>>  }
>>> @@ -184,6 +282,8 @@ static int sii902x_get_modes(struct drm_connector *connector)
>>>  	struct edid *edid;
>>>  	int num = 0, ret;
>>>  
>>> +	mutex_lock(&sii902x->mutex);
>>> +
>>>  	edid = drm_get_edid(connector, sii902x->i2cmux->adapter[0]);
>>>  	drm_connector_update_edid_property(connector, edid);
>>>  	if (edid) {
>>> @@ -197,14 +297,19 @@ static int sii902x_get_modes(struct drm_connector *connector)
>>>  	ret = drm_display_info_set_bus_formats(&connector->display_info,
>>>  					       &bus_format, 1);
>>>  	if (ret)
>>> -		return ret;
>>> +		goto error_out;
>>>  
>>>  	ret = regmap_update_bits(sii902x->regmap, SII902X_SYS_CTRL_DATA,
>>>  				 SII902X_SYS_CTRL_OUTPUT_MODE, output_mode);
>>>  	if (ret)
>>> -		return ret;
>>> +		goto error_out;
>>> +
>>> +	ret = num;
>>> +
>>> +error_out:
>>> +	mutex_unlock(&sii902x->mutex);
>>>  
>>> -	return num;
>>> +	return ret;
>>>  }
>>>  
>>>  static enum drm_mode_status sii902x_mode_valid(struct drm_connector *connector,
>>> @@ -224,20 +329,28 @@ static void sii902x_bridge_disable(struct drm_bridge *bridge)
>>>  {
>>>  	struct sii902x *sii902x = bridge_to_sii902x(bridge);
>>>  
>>> +	mutex_lock(&sii902x->mutex);
>>> +
>>>  	regmap_update_bits(sii902x->regmap, SII902X_SYS_CTRL_DATA,
>>>  			   SII902X_SYS_CTRL_PWR_DWN,
>>>  			   SII902X_SYS_CTRL_PWR_DWN);
>>> +
>>> +	mutex_unlock(&sii902x->mutex);
>>>  }
>>>  
>>>  static void sii902x_bridge_enable(struct drm_bridge *bridge)
>>>  {
>>>  	struct sii902x *sii902x = bridge_to_sii902x(bridge);
>>>  
>>> +	mutex_lock(&sii902x->mutex);
>>> +
>>>  	regmap_update_bits(sii902x->regmap, SII902X_PWR_STATE_CTRL,
>>>  			   SII902X_AVI_POWER_STATE_MSK,
>>>  			   SII902X_AVI_POWER_STATE_D(0));
>>>  	regmap_update_bits(sii902x->regmap, SII902X_SYS_CTRL_DATA,
>>>  			   SII902X_SYS_CTRL_PWR_DWN, 0);
>>> +
>>> +	mutex_unlock(&sii902x->mutex);
>>>  }
>>>  
>>>  static void sii902x_bridge_mode_set(struct drm_bridge *bridge,
>>> @@ -264,26 +377,31 @@ static void sii902x_bridge_mode_set(struct drm_bridge *bridge,
>>>  	buf[9] = SII902X_TPI_AVI_INPUT_RANGE_AUTO |
>>>  		 SII902X_TPI_AVI_INPUT_COLORSPACE_RGB;
>>>  
>>> +	mutex_lock(&sii902x->mutex);
>>> +
>>>  	ret = regmap_bulk_write(regmap, SII902X_TPI_VIDEO_DATA, buf, 10);
>>>  	if (ret)
>>> -		return;
>>> +		goto out;
>>>  
>>>  	ret = drm_hdmi_avi_infoframe_from_display_mode(&frame, adj, false);
>>>  	if (ret < 0) {
>>>  		DRM_ERROR("couldn't fill AVI infoframe\n");
>>> -		return;
>>> +		goto out;
>>>  	}
>>>  
>>>  	ret = hdmi_avi_infoframe_pack(&frame, buf, sizeof(buf));
>>>  	if (ret < 0) {
>>>  		DRM_ERROR("failed to pack AVI infoframe: %d\n", ret);
>>> -		return;
>>> +		goto out;
>>>  	}
>>>  
>>>  	/* Do not send the infoframe header, but keep the CRC field. */
>>>  	regmap_bulk_write(regmap, SII902X_TPI_AVI_INFOFRAME,
>>>  			  buf + HDMI_INFOFRAME_HEADER_SIZE - 1,
>>>  			  HDMI_AVI_INFOFRAME_SIZE + 1);
>>> +
>>> +out:
>>> +	mutex_unlock(&sii902x->mutex);
>>>  }
>>>  
>>>  static int sii902x_bridge_attach(struct drm_bridge *bridge)
>>> @@ -324,6 +442,330 @@ static const struct drm_bridge_funcs sii902x_bridge_funcs = {
>>>  	.enable = sii902x_bridge_enable,
>>>  };
>>>  
>>> +static int sii902x_mute(struct sii902x *sii902x, bool mute)
>>> +{
>>> +	struct device *dev = &sii902x->i2c->dev;
>>> +	unsigned int val = mute ? SII902X_TPI_AUDIO_MUTE_ENABLE :
>>> +		SII902X_TPI_AUDIO_MUTE_DISABLE;
>>> +
>>> +	dev_dbg(dev, "%s: %s\n", __func__, mute ? "Muted" : "Unmuted");
>>> +
>>> +	return regmap_update_bits(sii902x->regmap,
>>> +				  SII902X_TPI_AUDIO_CONFIG_BYTE2_REG,
>>> +				  SII902X_TPI_AUDIO_MUTE_ENABLE, val);
>>> +}
>>> +
>>> +static const unsigned int sii902x_mclk_div_table[] = {
>>> +	128, 256, 384, 512, 768, 1024, 1152, 192 };
>>> +
>>> +static int sii902x_select_mclk_div(u8 *i2s_config_reg, unsigned int rate,
>>> +				   unsigned int mclk)
>>> +{
>>> +	unsigned int div = mclk / rate;
>>> +	int distance = 100000;
>>> +	u8 i, nearest = 0;
>>> +
>>> +	for (i = 0; i < ARRAY_SIZE(sii902x_mclk_div_table); i++) {
>>> +		unsigned int d = abs(div - sii902x_mclk_div_table[i]);
>>
>> Using unsigned types in this context seems to be asking for troubles.
>>
> Why? Isn't return value of abs() by definition unsigned? Using signed
> integers when comparing absolute distances would seem awkward to me.


(div - sii902x_mclk_div_table[i]) is unsigned, if div is lower, there is overflow, and the value is big int, I suppose this is not what you want.


>
>>> +
>>> +		if (d >= distance)
>>> +			continue;
>>> +
>>> +		nearest = i;
>>> +		distance = d;
>>> +		if (d == 0)
>>> +			break;
>>> +	}
>>> +
>>> +	*i2s_config_reg |= nearest << 4;
>>> +
>>> +	if (distance != 0)
>>> +		return sii902x_mclk_div_table[nearest];
>>> +
>>> +	return 0;
>>
>> ret value is inconsistent, 0 in case of exact match, closest value
>> otherwise, code will be cleaner if you return always closest match.
>>
> Then I do not have the information to print a warning if an exact match
> was not found. The return value is only used in that warning message,
> the register configuration value is always delivered trough
> *i2s_config_reg. I do not see any inconsistency here.

You have it:

+    ret = sii902x_select_mclk_div(&i2s_config_reg, params->sample_rate,
+                      mclk_rate);
+    if (mclk_rate / ret != params->sample_rate)
+        dev_dbg(dev, "Inaccurate reference clock (%ld/%d != %u)\n",
+            mclk_rate, ret, params->sample_rate);
+

or to avoid division: if (mclk_rate != ret * params->sample_rate) ...


But this is not something I would die for, it just hurts my teeth :)


>
>>> +}
>>> +
>>> +static const struct sii902x_sample_freq {
>>> +	u32 freq;
>>> +	u8 val;
>>> +} sii902x_sample_freq[] = {
>>> +	{ .freq = 32000,	.val = SII902X_TPI_AUDIO_FREQ_32KHZ },
>>> +	{ .freq = 44000,	.val = SII902X_TPI_AUDIO_FREQ_44KHZ },
>>> +	{ .freq = 48000,	.val = SII902X_TPI_AUDIO_FREQ_48KHZ },
>>> +	{ .freq = 88000,	.val = SII902X_TPI_AUDIO_FREQ_88KHZ },
>>> +	{ .freq = 96000,	.val = SII902X_TPI_AUDIO_FREQ_96KHZ },
>>> +	{ .freq = 176000,	.val = SII902X_TPI_AUDIO_FREQ_176KHZ },
>>> +	{ .freq = 192000,	.val = SII902X_TPI_AUDIO_FREQ_192KHZ },
>>> +};
>>> +
>>> +static int sii902x_audio_hw_params(struct device *dev, void *data,
>>> +				   struct hdmi_codec_daifmt *daifmt,
>>> +				   struct hdmi_codec_params *params)
>>> +{
>>> +	struct sii902x *sii902x = dev_get_drvdata(dev);
>>> +	u8 i2s_config_reg = SII902X_TPI_I2S_SD_DIRECTION_MSB_FIRST;
>>> +	u8 config_byte2_reg = (SII902X_TPI_AUDIO_INTERFACE_I2S |
>>> +			       SII902X_TPI_AUDIO_MUTE_ENABLE |
>>> +			       SII902X_TPI_AUDIO_CODING_PCM);
>>> +	u8 config_byte3_reg = 0;
>>> +	u8 infoframe_buf[HDMI_INFOFRAME_SIZE(AUDIO)];
>>> +	unsigned long mclk_rate;
>>> +	int i, ret;
>>> +
>>> +	if (daifmt->bit_clk_master || daifmt->frame_clk_master) {
>>> +		dev_dbg(dev, "%s: I2S master mode not supported\n", __func__);
>>> +		return -EINVAL;
>>> +	}
>>> +
>>> +	switch (daifmt->fmt) {
>>> +	case HDMI_I2S:
>>> +		i2s_config_reg |= SII902X_TPI_I2S_FIRST_BIT_SHIFT_YES |
>>> +			SII902X_TPI_I2S_SD_JUSTIFY_LEFT;
>>> +		break;
>>> +	case HDMI_RIGHT_J:
>>> +		i2s_config_reg |= SII902X_TPI_I2S_SD_JUSTIFY_RIGHT;
>>> +		break;
>>> +	case HDMI_LEFT_J:
>>> +		i2s_config_reg |= SII902X_TPI_I2S_SD_JUSTIFY_LEFT;
>>> +		break;
>>> +	default:
>>> +		dev_dbg(dev, "%s: Unsupported i2s format %u\n", __func__,
>>> +			daifmt->fmt);
>>> +		return -EINVAL;
>>> +	}
>>> +
>>> +	if (daifmt->bit_clk_inv)
>>> +		i2s_config_reg |= SII902X_TPI_I2S_SCK_EDGE_FALLING;
>>> +	else
>>> +		i2s_config_reg |= SII902X_TPI_I2S_SCK_EDGE_RISING;
>>> +
>>> +	if (daifmt->frame_clk_inv)
>>> +		i2s_config_reg |= SII902X_TPI_I2S_WS_POLARITY_LOW;
>>> +	else
>>> +		i2s_config_reg |= SII902X_TPI_I2S_WS_POLARITY_HIGH;
>>> +
>>> +	if (params->channels > 2)
>>> +		config_byte2_reg |= SII902X_TPI_AUDIO_LAYOUT_8_CHANNELS;
>>> +	else
>>> +		config_byte2_reg |= SII902X_TPI_AUDIO_LAYOUT_2_CHANNELS;
>>> +
>>> +	switch (params->sample_width) {
>>> +	case 16:
>>> +		config_byte3_reg |= SII902X_TPI_AUDIO_SAMPLE_SIZE_16;
>>> +		break;
>>> +	case 20:
>>> +		config_byte3_reg |= SII902X_TPI_AUDIO_SAMPLE_SIZE_20;
>>> +		break;
>>> +	case 24:
>>> +	case 32:
>>> +		config_byte3_reg |= SII902X_TPI_AUDIO_SAMPLE_SIZE_24;
>>> +		break;
>>> +	default:
>>> +		dev_err(dev, "%s: Unsupported sample width %u\n", __func__,
>>> +			params->sample_width);
>>> +		return -EINVAL;
>>> +	}
>>> +
>>> +	for (i = 0; i < ARRAY_SIZE(sii902x_sample_freq); i++) {
>>> +		if (params->sample_rate == sii902x_sample_freq[i].freq) {
>>> +			config_byte3_reg |= sii902x_sample_freq[i].val;
>>> +			break;
>>> +		}
>>> +	}
>>> +
>>> +	ret = clk_prepare_enable(sii902x->audio.mclk);
>>> +	if (ret) {
>>> +		dev_err(dev, "Enabling mclk failed: %d\n", ret);
>>> +		return ret;
>>> +	}
>>> +
>>> +	mclk_rate = clk_get_rate(sii902x->audio.mclk);
>>> +
>>> +	ret = sii902x_select_mclk_div(&i2s_config_reg, params->sample_rate,
>>> +				      mclk_rate);
>>> +	if (ret)
>>> +		dev_dbg(dev, "Inaccurate reference clock (%ld/%d != %u)\n",
>>> +			mclk_rate, ret, params->sample_rate);
>>> +
>>> +	mutex_lock(&sii902x->mutex);
>>> +
>>> +	ret = regmap_write(sii902x->regmap,
>>> +			   SII902X_TPI_AUDIO_CONFIG_BYTE2_REG,
>>> +			   config_byte2_reg);
>>> +	if (ret < 0)
>>> +		goto out;
>>> +
>>> +	ret = regmap_write(sii902x->regmap, SII902X_TPI_I2S_INPUT_CONFIG_REG,
>>> +			   i2s_config_reg);
>>> +	if (ret)
>>> +		goto out;
>>> +
>>> +	for (i = 0; sii902x->audio.i2s_fifo_sequence[i] &&
>>> +		     i < ARRAY_SIZE(sii902x->audio.i2s_fifo_sequence); i++)
>>> +		regmap_write(sii902x->regmap,
>>> +			     SII902X_TPI_I2S_ENABLE_MAPPING_REG,
>>> +			     sii902x->audio.i2s_fifo_sequence[i]);
>>> +
>>> +	ret = regmap_write(sii902x->regmap, SII902X_TPI_AUDIO_CONFIG_BYTE3_REG,
>>> +			   config_byte3_reg);
>>> +	if (ret)
>>> +		goto out;
>>> +
>>> +	ret = regmap_bulk_write(sii902x->regmap, SII902X_TPI_I2S_STRM_HDR_BASE,
>>> +				params->iec.status,
>>> +				min((size_t) SII902X_TPI_I2S_STRM_HDR_SIZE,
>>> +				    sizeof(params->iec.status)));
>>> +	if (ret)
>>> +		goto out;
>>> +
>>> +	ret = hdmi_audio_infoframe_pack(&params->cea, infoframe_buf,
>>> +					sizeof(infoframe_buf));
>>> +	if (ret < 0) {
>>> +		dev_err(dev, "%s: Failed to pack audio infoframe: %d\n",
>>> +			__func__, ret);
>>> +		goto out;
>>> +	}
>>> +
>>> +	ret = regmap_bulk_write(sii902x->regmap,
>>> +				SII902X_TPI_MISC_INFOFRAME_BASE,
>>> +				infoframe_buf,
>>> +				min(ret, SII902X_TPI_MISC_INFOFRAME_SIZE));
>>> +	if (ret)
>>> +		goto out;
>>> +
>>> +	/* Decode Level 0 Packets */
>>> +	ret = regmap_write(sii902x->regmap, SII902X_IND_SET_PAGE, 0x02);
>>> +	if (ret)
>>> +		goto out;
>>> +
>>> +	ret = regmap_write(sii902x->regmap, SII902X_IND_OFFSET, 0x24);
>>> +	if (ret)
>>> +		goto out;
>>> +
>>> +	ret = regmap_write(sii902x->regmap, SII902X_IND_VALUE, 0x02);
>>> +	if (ret)
>>> +		goto out;
>>> +
>>> +	dev_dbg(dev, "%s: hdmi audio enabled\n", __func__);
>>> +out:
>>> +	mutex_unlock(&sii902x->mutex);
>>> +
>>> +	if (ret) {
>>> +		clk_disable_unprepare(sii902x->audio.mclk);
>>> +		dev_err(dev, "%s: hdmi audio enable failed: %d\n", __func__,
>>> +			ret);
>>> +	}
>>> +
>>> +	return ret;
>>> +}
>>> +
>>> +static void sii902x_audio_shutdown(struct device *dev, void *data)
>>> +{
>>> +	struct sii902x *sii902x = dev_get_drvdata(dev);
>>> +
>>> +	mutex_lock(&sii902x->mutex);
>>> +
>>> +	regmap_write(sii902x->regmap, SII902X_TPI_AUDIO_CONFIG_BYTE2_REG,
>>> +		     SII902X_TPI_AUDIO_INTERFACE_DISABLE);
>>> +
>>> +	mutex_unlock(&sii902x->mutex);
>>> +
>>> +	clk_disable_unprepare(sii902x->audio.mclk);
>>> +}
>>> +
>>> +int sii902x_audio_digital_mute(struct device *dev, void *data, bool enable)
>>> +{
>>> +	struct sii902x *sii902x = dev_get_drvdata(dev);
>>> +
>>> +	mutex_lock(&sii902x->mutex);
>>> +
>>> +	sii902x_mute(sii902x, enable);
>>> +
>>> +	mutex_unlock(&sii902x->mutex);
>>> +
>>> +	return 0;
>>> +}
>>> +
>>> +static int sii902x_audio_get_eld(struct device *dev, void *data,
>>> +				 uint8_t *buf, size_t len)
>>> +{
>>> +	struct sii902x *sii902x = dev_get_drvdata(dev);
>>> +
>>> +	mutex_lock(&sii902x->mutex);
>>> +
>>> +	memcpy(buf, sii902x->connector.eld,
>>> +	       min(sizeof(sii902x->connector.eld), len));
>>
>> I am not familiar with audio stuff, so forgive my ignorance :)
>>
>> What happens when this or other audio callbacks is called:
>>
>> 1. before sth is plugged in connector.
>>> 2. after unplug.
>> 3. plug/un-plug happens between these calls.
>>
> The audio side calls this at the time of audio playback request. If
> there is no valid eld at that time, the playback request fails. The eld
> is only used at the playback start time, never after it.
>
> When an audio playback is running it we do not do anything to until the
> playback is stopped. E.g. if plug/un-plug happens we just keep playing
> with the original configuration (my first version had an abort call back
> to abort playback in unplug event, but that was refused in the reviews).
>
> If the cable is never plugged again, or the new display does not have
> audio support, the i2s just keeps banging the bits to dev null and audio
> data keeps drainging away. When cable is again plugged to audio capable
> display, it continues playing there (and it never stopped in between.
>
> This comes closest to already established behaviour with already
> existing HDMI audio implementations. This is also usually the preferred
> behavior, because resebles how video stream playback behaves.
>
> I know very well that this is not the perfect solution. The perfect
> solution would have the abort callback there, and some mechanism above
> that to deal with the audio playback being aborted and the device
> becoming available again. This would require co-operation from
> userspace. However, the world is not ready and this is a best effort
> implementation.


OK, thanks for explanation.


>
>> Also I do not see hotplug/unplug notification mechanism to audio
>> subsystem. It looks suspicious.
>>
> Yes, as mentioned above, I had it in my first version of hdmi-codec, but
> it was never accepted to mainline. However, in practice this has not
> been a problem. The HDMI audio implementation on the encoder chips I
> have seen is usually quite independent from the video side workings.
>
>> Regards
>>
>> Andrzej
>>
> Thanks,
> Jyri
>
>>
>>> +
>>> +	mutex_unlock(&sii902x->mutex);
>>> +
>>> +	return 0;
>>> +}
>>> +
>>> +static const struct hdmi_codec_ops sii902x_audio_codec_ops = {
>>> +	.hw_params = sii902x_audio_hw_params,
>>> +	.audio_shutdown = sii902x_audio_shutdown,
>>> +	.digital_mute = sii902x_audio_digital_mute,
>>> +	.get_eld = sii902x_audio_get_eld,
>>> +};
>>> +
>>> +static int sii902x_audio_codec_init(struct sii902x *sii902x,
>>> +				    struct device *dev)
>>> +{
>>> +	static const u8 audio_fifo_id[] = {
>>> +		SII902X_TPI_I2S_CONFIG_FIFO0,
>>> +		SII902X_TPI_I2S_CONFIG_FIFO1,
>>> +		SII902X_TPI_I2S_CONFIG_FIFO2,
>>> +		SII902X_TPI_I2S_CONFIG_FIFO3,
>>> +	};
>>> +	static const u8 i2s_lane_id[] = {
>>> +		SII902X_TPI_I2S_SELECT_SD0,
>>> +		SII902X_TPI_I2S_SELECT_SD1,
>>> +		SII902X_TPI_I2S_SELECT_SD2,
>>> +		SII902X_TPI_I2S_SELECT_SD3,
>>> +	};
>>> +	struct hdmi_codec_pdata codec_data = {
>>> +		.ops = &sii902x_audio_codec_ops,
>>> +		.i2s = 1, /* Only i2s support for now. */
>>> +		.spdif = 0,
>>> +		.max_i2s_channels = 0,
>>> +	};
>>> +	u8 lanes[4];
>>> +	u32 num_lanes, i;
>>> +
>>> +	num_lanes = of_property_read_variable_u8_array(dev->of_node,
>>> +						       "sil,i2s-data-lanes",
>>> +						       lanes, 1,
>>> +						       ARRAY_SIZE(lanes));
>>> +	if (num_lanes < 0) {
>>> +		if (num_lanes == -EINVAL)
>>> +			dev_dbg(dev,
>>> +				"%s: No \"sil,i2s-data-lanes\", no audio\n",
>>> +				__func__);
>>> +		else
>>> +			dev_err(dev,
>>> +				"%s: Error gettin \"sil,i2s-data-lanes\": %d\n",
>>> +				__func__, num_lanes);
>>> +		return 0;
>>> +	}
>>> +	codec_data.max_i2s_channels = 2 * num_lanes;
>>> +
>>> +	for (i = 0; i < num_lanes; i++)
>>> +		sii902x->audio.i2s_fifo_sequence[i] |= audio_fifo_id[i] |
>>> +			i2s_lane_id[lanes[i]] |	SII902X_TPI_I2S_FIFO_ENABLE;
>>> +
>>> +	if (IS_ERR(sii902x->audio.mclk)) {
>>> +		dev_err(dev, "%s: No clock (audio mclk) found: %ld\n",
>>> +			__func__, PTR_ERR(sii902x->audio.mclk));
>>> +		return 0;
>>> +	}
>>> +
>>> +	sii902x->audio.pdev = platform_device_register_data(
>>> +		dev, HDMI_CODEC_DRV_NAME, PLATFORM_DEVID_AUTO,
>>> +		&codec_data, sizeof(codec_data));
>>> +
>>> +	return PTR_ERR_OR_ZERO(sii902x->audio.pdev);
>>> +}
>>> +
>>>  static const struct regmap_range sii902x_volatile_ranges[] = {
>>>  	{ .range_min = 0, .range_max = 0xff },
>>>  };
>>> @@ -336,6 +778,7 @@ static const struct regmap_access_table sii902x_volatile_table = {
>>>  static const struct regmap_config sii902x_regmap_config = {
>>>  	.reg_bits = 8,
>>>  	.val_bits = 8,
>>> +	.max_register = SII902X_TPI_MISC_INFOFRAME_END,
>>>  	.volatile_table = &sii902x_volatile_table,
>>>  	.cache_type = REGCACHE_NONE,
>>>  };
>>> @@ -508,6 +951,8 @@ static int sii902x_probe(struct i2c_client *client,
>>>  		return PTR_ERR(sii902x->reset_gpio);
>>>  	}
>>>  
>>> +	mutex_init(&sii902x->mutex);
>>> +
>>>  	sii902x_reset(sii902x);
>>>  
>>>  	ret = regmap_write(sii902x->regmap, SII902X_REG_TPI_RQB, 0x0);
>>> @@ -548,6 +993,8 @@ static int sii902x_probe(struct i2c_client *client,
>>>  	sii902x->bridge.timings = &default_sii902x_timings;
>>>  	drm_bridge_add(&sii902x->bridge);
>>>  
>>> +	sii902x_audio_codec_init(sii902x, dev);
>>> +
>>>  	i2c_set_clientdata(client, sii902x);
>>>  
>>>  	sii902x->i2cmux = i2c_mux_alloc(client->adapter, dev,
>>
>

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v5 6/6] drm/bridge: sii902x: Implement HDMI audio support

[Jyri Sarha]

On 12/04/2019 15:30, Andrzej Hajda wrote:
>>>> +static const unsigned int sii902x_mclk_div_table[] = {
>>>> +	128, 256, 384, 512, 768, 1024, 1152, 192 };
>>>> +
>>>> +static int sii902x_select_mclk_div(u8 *i2s_config_reg, unsigned int rate,
>>>> +				   unsigned int mclk)
>>>> +{
>>>> +	unsigned int div = mclk / rate;
>>>> +	int distance = 100000;
>>>> +	u8 i, nearest = 0;
>>>> +
>>>> +	for (i = 0; i < ARRAY_SIZE(sii902x_mclk_div_table); i++) {
>>>> +		unsigned int d = abs(div - sii902x_mclk_div_table[i]);
>>> Using unsigned types in this context seems to be asking for troubles.
>>>
>> Why? Isn't return value of abs() by definition unsigned? Using signed
>> integers when comparing absolute distances would seem awkward to me.
> 
> (div - sii902x_mclk_div_table[i]) is unsigned, if div is lower, there is overflow, and the value is big int, I suppose this is not what you want.
> 

Oh yes. I had my eyes fixed on wrong unsigned. The first operand of
subtraction should indeed be signed for the result to be signed, I
completely overlooked that.

Thanks,
Jyri



-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v5 6/6] drm/bridge: sii902x: Implement HDMI audio support

[Andrzej Hajda]

On 15.04.2019 09:26, Jyri Sarha wrote:
> On 12/04/2019 15:30, Andrzej Hajda wrote:
>>>>> +static const unsigned int sii902x_mclk_div_table[] = {
>>>>> +	128, 256, 384, 512, 768, 1024, 1152, 192 };
>>>>> +
>>>>> +static int sii902x_select_mclk_div(u8 *i2s_config_reg, unsigned int rate,
>>>>> +				   unsigned int mclk)
>>>>> +{
>>>>> +	unsigned int div = mclk / rate;
>>>>> +	int distance = 100000;
>>>>> +	u8 i, nearest = 0;
>>>>> +
>>>>> +	for (i = 0; i < ARRAY_SIZE(sii902x_mclk_div_table); i++) {
>>>>> +		unsigned int d = abs(div - sii902x_mclk_div_table[i]);
>>>> Using unsigned types in this context seems to be asking for troubles.
>>>>
>>> Why? Isn't return value of abs() by definition unsigned? Using signed
>>> integers when comparing absolute distances would seem awkward to me.
>> (div - sii902x_mclk_div_table[i]) is unsigned, if div is lower, there is overflow, and the value is big int, I suppose this is not what you want.
>>
> Oh yes. I had my eyes fixed on wrong unsigned. The first operand of
> subtraction should indeed be signed for the result to be signed, I
> completely overlooked that.


Worse, both operands must be int to avoid casting to unsigned.


Regards

Andrzej


>
> Thanks,
> Jyri
>
>
>

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel