media: adv748x: add support for HDMI audio
Subsystems (5)
v3 cover media: adv748x: add support for HDMI audio
This adds minimal support for accessing the HDMI audio provided through the I2S port available on ADV7481 and ADV7482 decoder devices by ADI. The port carries audio signal from the decoded HDMI stream.
Currently, the driver only supports I2S in TDM, 8 channels a 24bit at 48kHz. Furthermore, only left-justified, 8 slots, 32bit/slot TDM, at 256fs has been ever tried.
An ADV7482 on the Renesas Salvator-X ES1.1 (R8A77950 SoC) was used during development of this code.
Changes since v2:
- prepare/enable the clock when it is used, as it seems nothing else does this otherwise
- give the clock a unique name to ensure it can be registered if there are multiple adv748x devices in the system
- remove optionality note from clock cell description to ensure the device description matches the real device (the line is always present, even if not used)
Changes since v1:
- Add ssi4_ctrl pin group to the sound pins. The pins are responsible for SCK4 (sample clock) WS4 and (word boundary input), and are required for SSI audio input over I2S.
- Removed the audio clock C from the list of clocks of adv748x, it is exactly the other way around.
- Add an instance of (currently) fixed rate I2S master clock (MCLK), connected to the audio_clk_c line of the R-Car SoC. Explicitly declare the device a clock producer and add it to the list of clocks used by the audio system of the Salvator-X board.
- The implementation of DAI driver has been moved in a separate file and modified to activate audio decoding and I2S streaming using snd_soc_dai_... interfaces. This allows the driver to be used with just ALSA interfaces.
- The ioctls for selecting audio output and muting have been removed, as not applicable.
I have left implementation of the QUERYCAP in, as it seems to be required by v4l-ctl to support loading of EDID for this node. And setting the EDID is one feature I desperately need: there are devices which plainly refuse to talk to the sink if it does not provide EDID they like.
- A device tree configuration without audio port will disable the audio code altogether, supporting integrations where the port is not connected.
- The patches have been re-arranged, starting with the generic changes and changes not related to audio directly. Those will be probably sent as a separate series later.
- The whole series has been rebased on top of v5.6-rc6
Alex Riesen (11):
media: adv748x: fix end-of-line terminators in diagnostic statements
media: adv748x: include everything adv748x.h needs into the file
media: adv748x: reduce amount of code for bitwise modifications of
device registers
media: adv748x: add definitions for audio output related registers
media: adv748x: add support for HDMI audio
media: adv748x: prepare/enable mclk when the audio is used
media: adv748x: only activate DAI if it is described in device tree
dt-bindings: adv748x: add information about serial audio interface
(I2S/TDM)
arm64: dts: renesas: salvator: add a connection from adv748x codec
(HDMI input) to the R-Car SoC
media: adv748x: add support for log_status ioctl
media: adv748x: allow the HDMI sub-device to accept EDID
.../devicetree/bindings/media/i2c/adv748x.txt | 16 +- .../boot/dts/renesas/r8a77950-salvator-x.dts | 3 +- .../boot/dts/renesas/salvator-common.dtsi | 47 ++- drivers/media/i2c/adv748x/Makefile | 3 +- drivers/media/i2c/adv748x/adv748x-afe.c | 6 +- drivers/media/i2c/adv748x/adv748x-core.c | 60 ++-- drivers/media/i2c/adv748x/adv748x-csi2.c | 8 +- drivers/media/i2c/adv748x/adv748x-dai.c | 282 ++++++++++++++++++ drivers/media/i2c/adv748x/adv748x-hdmi.c | 212 ++++++++++++- drivers/media/i2c/adv748x/adv748x.h | 67 ++++- 10 files changed, 662 insertions(+), 42 deletions(-) create mode 100644 drivers/media/i2c/adv748x/adv748x-dai.c
Signature · 1 line
2.25.1.25.g9ecbe7eb18
PATCH v3 1/11 media: adv748x: fix end-of-line terminators in diagnostic statements
--- drivers/media/i2c/adv748x/adv748x-core.c | 24 ++++++++++++------------ drivers/media/i2c/adv748x/adv748x-csi2.c | 2 +- 2 files changed, 13 insertions(+), 13 deletions(-)
drivers/media/i2c/adv748x/adv748x-core.c +12 -12
@@ -623,11 +623,11 @@ static int adv748x_parse_dt(struct adv748x_state *state)for_each_endpoint_of_node (state->dev->of_node, ep_np) {of_graph_parse_endpoint (ep_np, &ep); -adv_info (state,"Endpoint %pOF on port %d" , ep.local_node, +adv_info (state,"Endpoint %pOF on port %d\n" , ep.local_node, ep.port);if (ep.port >=ADV748X_PORT_MAX ) { -adv_err (state,"Invalid endpoint %pOF on port %d" , +adv_err (state,"Invalid endpoint %pOF on port %d\n" , ep.local_node, ep.port);continue ;
@@ -635,7 +635,7 @@ static int adv748x_parse_dt(struct adv748x_state *state)if (state->endpoints[ep.port]) {adv_err (state, -"Multiple port endpoints are not supported" ); +"Multiple port endpoints are not supported\n" );continue ; }
@@ -702,62 +702,62 @@ static int adv748x_probe(struct i2c_client *client)/* Discover and process ports declared by the Device tree endpoints */ ret =adv748x_parse_dt (state);if (ret) { -adv_err (state,"Failed to parse device tree" ); +adv_err (state,"Failed to parse device tree\n" ); goto err_free_mutex; }/* Configure IO Regmap region */ ret =adv748x_configure_regmap (state,ADV748X_PAGE_IO );if (ret) { -adv_err (state,"Error configuring IO regmap region" ); +adv_err (state,"Error configuring IO regmap region\n" ); goto err_cleanup_dt; } ret =adv748x_identify_chip (state);if (ret) { -adv_err (state,"Failed to identify chip" ); +adv_err (state,"Failed to identify chip\n" ); goto err_cleanup_dt; }/* Configure remaining pages as I2C clients with regmap access */ ret =adv748x_initialise_clients (state);if (ret) { -adv_err (state,"Failed to setup client regmap pages" ); +adv_err (state,"Failed to setup client regmap pages\n" ); goto err_cleanup_clients; }/* SW reset ADV748X to its default values */ ret =adv748x_reset (state);if (ret) { -adv_err (state,"Failed to reset hardware" ); +adv_err (state,"Failed to reset hardware\n" ); goto err_cleanup_clients; }/* Initialise HDMI */ ret =adv748x_hdmi_init (&state->hdmi);if (ret) { -adv_err (state,"Failed to probe HDMI" ); +adv_err (state,"Failed to probe HDMI\n" ); goto err_cleanup_clients; }/* Initialise AFE */ ret =adv748x_afe_init (&state->afe);if (ret) { -adv_err (state,"Failed to probe AFE" ); +adv_err (state,"Failed to probe AFE\n" ); goto err_cleanup_hdmi; }/* Initialise TXA */ ret =adv748x_csi2_init (state, &state->txa);if (ret) { -adv_err (state,"Failed to probe TXA" ); +adv_err (state,"Failed to probe TXA\n" ); goto err_cleanup_afe; }/* Initialise TXB */ ret =adv748x_csi2_init (state, &state->txb);if (ret) { -adv_err (state,"Failed to probe TXB" ); +adv_err (state,"Failed to probe TXB\n" ); goto err_cleanup_txa; }
drivers/media/i2c/adv748x/adv748x-csi2.c +1 -1
@@ -72,7 +72,7 @@ static int adv748x_csi2_registered(struct v4l2_subdev *sd)struct adv748x_state *state = tx->state;int ret; -adv_dbg (state,"Registered %s (%s)" ,is_txa (tx) ?"TXA" :"TXB" , +adv_dbg (state,"Registered %s (%s)\n" ,is_txa (tx) ?"TXA" :"TXB" , sd->name);/*
Signature · 1 line
2.25.1.25.g9ecbe7eb18
PATCH v3 2/11 media: adv748x: include everything adv748x.h needs into the file
In reply to Alex RiesenTo follow the established practice of not depending on others to pull everything in. While at it, make sure it stays like this.
--- drivers/media/i2c/adv748x/adv748x-afe.c | 6 ++---- drivers/media/i2c/adv748x/adv748x-core.c | 6 ++---- drivers/media/i2c/adv748x/adv748x-csi2.c | 6 ++---- drivers/media/i2c/adv748x/adv748x-hdmi.c | 6 ++---- drivers/media/i2c/adv748x/adv748x.h | 2 ++ 5 files changed, 10 insertions(+), 16 deletions(-)
drivers/media/i2c/adv748x/adv748x-afe.c +2 -4
@@ -6,18 +6,16 @@* Copyright (C) 2017 Renesas Electronics Corp. */ +#include "adv748x.h" +#include <linux/delay.h> #include <linux/module.h> #include <linux/mutex.h> #include <linux/v4l2-dv-timings.h> -#include <media/v4l2-ctrls.h> -#include <media/v4l2-device.h> #include <media/v4l2-dv-timings.h> #include <media/v4l2-ioctl.h> -#include "adv748x.h" -/* ----------------------------------------------------------------------------- * SDP */
drivers/media/i2c/adv748x/adv748x-core.c +2 -4
@@ -10,6 +10,8 @@* Kieran Bingham <[email protected]> */ +#include "adv748x.h" +#include <linux/delay.h> #include <linux/errno.h> #include <linux/i2c.h>
@@ -20,14 +22,10 @@#include <linux/slab.h> #include <linux/v4l2-dv-timings.h> -#include <media/v4l2-ctrls.h> -#include <media/v4l2-device.h> #include <media/v4l2-dv-timings.h> #include <media/v4l2-fwnode.h> #include <media/v4l2-ioctl.h> -#include "adv748x.h" -/* ----------------------------------------------------------------------------- * Register manipulation */
drivers/media/i2c/adv748x/adv748x-csi2.c +2 -4
@@ -5,15 +5,13 @@* Copyright (C) 2017 Renesas Electronics Corp. */ +#include "adv748x.h" +#include <linux/module.h> #include <linux/mutex.h> -#include <media/v4l2-ctrls.h> -#include <media/v4l2-device.h> #include <media/v4l2-ioctl.h> -#include "adv748x.h" -static int adv748x_csi2_set_virtual_channel (struct adv748x_csi2 *tx,unsigned int vc) {
drivers/media/i2c/adv748x/adv748x-hdmi.c +2 -4
@@ -5,18 +5,16 @@* Copyright (C) 2017 Renesas Electronics Corp. */ +#include "adv748x.h" +#include <linux/module.h> #include <linux/mutex.h> -#include <media/v4l2-ctrls.h> -#include <media/v4l2-device.h> #include <media/v4l2-dv-timings.h> #include <media/v4l2-ioctl.h> #include <uapi/linux/v4l2-dv-timings.h> -#include "adv748x.h" -/* ----------------------------------------------------------------------------- * HDMI and CP */
drivers/media/i2c/adv748x/adv748x.h +2 -0
@@ -19,6 +19,8 @@*/ #include <linux/i2c.h> +#include <media/v4l2-ctrls.h> +#include <media/v4l2-device.h> #ifndef _ADV748X_H_#define _ADV748X_H_
Signature · 1 line
2.25.1.25.g9ecbe7eb18
PATCH v3 3/11 media: adv748x: reduce amount of code for bitwise modifications of device registers
In reply to Alex RiesenThe regmap provides a convenient utility for this.
The hdmi_* and dpll_* register modification macros added for symmetry with the existing operations (io_*, sdp_*).
--
v3: remove _update name in favor of existing _clrset
--- drivers/media/i2c/adv748x/adv748x-core.c | 6 ++++++ drivers/media/i2c/adv748x/adv748x.h | 14 +++++++++++--- 2 files changed, 17 insertions(+), 3 deletions(-)
drivers/media/i2c/adv748x/adv748x-core.c +6 -0
@@ -133,6 +133,12 @@ static int adv748x_write_check(struct adv748x_state *state, u8 page, u8 reg,return *error; } +int adv748x_update_bits (struct adv748x_state *state,u8 page,u8 reg,u8 mask, +u8 value) +{ +return regmap_update_bits (state->regmap[page], reg, mask, value); +} +/* adv748x_write_block(): Write raw data with a maximum of I2C_SMBUS_BLOCK_MAX * size to one or more registers. *
drivers/media/i2c/adv748x/adv748x.h +11 -3
@@ -393,25 +393,33 @@ int adv748x_write(struct adv748x_state *state, u8 page, u8 reg, u8 value);int adv748x_write_block (struct adv748x_state *state,int client_page,unsigned int init_reg,const void *val,size_t val_len); +int adv748x_update_bits (struct adv748x_state *state,u8 page,u8 reg, +u8 mask,u8 value);#define io_read (s, r) adv748x_read(s, ADV748X_PAGE_IO, r)#define io_write (s, r, v) adv748x_write(s, ADV748X_PAGE_IO, r, v) -#define io_clrset (s, r, m, v) io_write(s, r, (io_read(s, r) & ~(m)) | (v)) +#define io_clrset (s, r, m, v) adv748x_update_bits(s, ADV748X_PAGE_IO, r, m, v)#define hdmi_read (s, r) adv748x_read(s, ADV748X_PAGE_HDMI, r)#define hdmi_read16 (s, r, m) (((hdmi_read(s, r) << 8) | hdmi_read(s, (r)+1)) & (m))#define hdmi_write (s, r, v) adv748x_write(s, ADV748X_PAGE_HDMI, r, v) +#define hdmi_clrset (s, r, m, v) \ + adv748x_update_bits(s, ADV748X_PAGE_HDMI, r, m, v) + +#define dpll_read (s, r) adv748x_read(s, ADV748X_PAGE_DPLL, r) +#define dpll_clrset (s, r, m, v) \ + adv748x_update_bits(s, ADV748X_PAGE_DPLL, r, m, v)#define repeater_read (s, r) adv748x_read(s, ADV748X_PAGE_REPEATER, r)#define repeater_write (s, r, v) adv748x_write(s, ADV748X_PAGE_REPEATER, r, v)#define sdp_read (s, r) adv748x_read(s, ADV748X_PAGE_SDP, r)#define sdp_write (s, r, v) adv748x_write(s, ADV748X_PAGE_SDP, r, v) -#define sdp_clrset (s, r, m, v) sdp_write(s, r, (sdp_read(s, r) & ~(m)) | (v)) +#define sdp_clrset (s, r, m, v) adv748x_update_bits(s, ADV748X_PAGE_SDP, r, m, v)#define cp_read (s, r) adv748x_read(s, ADV748X_PAGE_CP, r)#define cp_write (s, r, v) adv748x_write(s, ADV748X_PAGE_CP, r, v) -#define cp_clrset (s, r, m, v) cp_write(s, r, (cp_read(s, r) & ~(m)) | (v)) +#define cp_clrset (s, r, m, v) adv748x_update_bits(s, ADV748X_PAGE_CP, r, m, v)#define tx_read (t, r) adv748x_read(t->state, t->page, r)#define tx_write (t, r, v) adv748x_write(t->state, t->page, r, v)
Signature · 1 line
2.25.1.25.g9ecbe7eb18
PATCH v3 4/11 media: adv748x: add definitions for audio output related registers
In reply to Alex Riesen--- drivers/media/i2c/adv748x/adv748x.h | 32 +++++++++++++++++++++++++++++ 1 file changed, 32 insertions(+)
drivers/media/i2c/adv748x/adv748x.h +32 -0
@@ -226,6 +226,11 @@ struct adv748x_state {#define ADV748X_IO_VID_STD 0x05 +#define ADV748X_IO_PAD_CONTROLS 0x0e +#define ADV748X_IO_PAD_CONTROLS_TRI_AUD BIT(5) +#define ADV748X_IO_PAD_CONTROLS_PDN_AUD BIT(1) +#define ADV748X_IO_PAD_CONTROLS1 0x1d +#define ADV748X_IO_10 0x10/* io_reg_10 */ #define ADV748X_IO_10_CSI4_EN BIT(7)#define ADV748X_IO_10_CSI1_EN BIT(6)
@@ -248,7 +253,21 @@ struct adv748x_state {#define ADV748X_IO_REG_FF 0xff#define ADV748X_IO_REG_FF_MAIN_RESET 0xff +/* DPLL Map */ +#define ADV748X_DPLL_MCLK_FS 0xb5 +#define ADV748X_DPLL_MCLK_FS_N_MASK GENMASK(2, 0) +/* HDMI RX Map */ +#define ADV748X_HDMI_I2S 0x03/* I2S mode and width */ +#define ADV748X_HDMI_I2SBITWIDTH_MASK GENMASK(4, 0) +#define ADV748X_HDMI_I2SOUTMODE_SHIFT 5 +#define ADV748X_HDMI_I2SOUTMODE_MASK \ + GENMASK(6, ADV748X_HDMI_I2SOUTMODE_SHIFT) +#define ADV748X_I2SOUTMODE_I2S 0 +#define ADV748X_I2SOUTMODE_RIGHT_J 1 +#define ADV748X_I2SOUTMODE_LEFT_J 2 +#define ADV748X_I2SOUTMODE_SPDIF 3 +#define ADV748X_HDMI_LW1 0x07/* line width_1 */ #define ADV748X_HDMI_LW1_VERT_FILTER BIT(7)#define ADV748X_HDMI_LW1_DE_REGEN BIT(5)
@@ -260,6 +279,16 @@ struct adv748x_state {#define ADV748X_HDMI_F1H1 0x0b/* field1 height_1 */ #define ADV748X_HDMI_F1H1_INTERLACED BIT(5) +#define ADV748X_HDMI_MUTE_CTRL 0x1a +#define ADV748X_HDMI_MUTE_CTRL_MUTE_AUDIO BIT(4) +#define ADV748X_HDMI_MUTE_CTRL_WAIT_UNMUTE_MASK GENMASK(3, 1) +#define ADV748X_HDMI_MUTE_CTRL_NOT_AUTO_UNMUTE BIT(0) + +#define ADV748X_HDMI_AUDIO_MUTE_SPEED 0x0f +#define ADV748X_HDMI_AUDIO_MUTE_SPEED_MASK GENMASK(4, 0) +#define ADV748X_MAN_AUDIO_DL_BYPASS BIT(7) +#define ADV748X_AUDIO_DELAY_LINE_BYPASS BIT(6) +#define ADV748X_HDMI_HFRONT_PORCH 0x20/* hsync_front_porch_1 */ #define ADV748X_HDMI_HFRONT_PORCH_MASK 0x1fff
@@ -281,6 +310,9 @@ struct adv748x_state {#define ADV748X_HDMI_TMDS_1 0x51/* hdmi_reg_51 */ #define ADV748X_HDMI_TMDS_2 0x52/* hdmi_reg_52 */ +#define ADV748X_HDMI_REG_6D 0x6d/* hdmi_reg_6d */ +#define ADV748X_I2S_TDM_MODE_ENABLE BIT(7) +/* HDMI RX Repeater Map */ #define ADV748X_REPEATER_EDID_SZ 0x70/* primary_edid_size */ #define ADV748X_REPEATER_EDID_SZ_SHIFT 4
Signature · 1 line
2.25.1.25.g9ecbe7eb18
PATCH v3 5/11 media: adv748x: add support for HDMI audio
In reply to Alex RiesenThis adds an implemention of SoC DAI driver which provides access to the I2S port of the device.
--
v3: fix clock registration in case of multiple adv748x devices
--- drivers/media/i2c/adv748x/Makefile | 3 +- drivers/media/i2c/adv748x/adv748x-core.c | 9 +- drivers/media/i2c/adv748x/adv748x-dai.c | 265 +++++++++++++++++++++++ drivers/media/i2c/adv748x/adv748x.h | 17 +- 4 files changed, 291 insertions(+), 3 deletions(-) create mode 100644 drivers/media/i2c/adv748x/adv748x-dai.c
drivers/media/i2c/adv748x/Makefile +2 -1
@@ -3,6 +3,7 @@ adv748x-objs := \ adv748x-afe.o \ adv748x-core.o \ adv748x-csi2.o \ - adv748x-hdmi.o + adv748x-hdmi.o \ + adv748x-dai.o obj-$(CONFIG_VIDEO_ADV748X) += adv748x.o
drivers/media/i2c/adv748x/adv748x-core.c +8 -1
@@ -765,8 +765,14 @@ static int adv748x_probe(struct i2c_client *client) goto err_cleanup_txa; } + ret =adv748x_dai_init (&state->dai); +if (ret) { +adv_err (state,"Failed to probe DAI\n" ); + goto err_cleanup_txb; + }return 0 ; - +err_cleanup_txb: +adv748x_csi2_cleanup (&state->txb); err_cleanup_txa:adv748x_csi2_cleanup (&state->txa); err_cleanup_afe:
@@ -787,6 +793,7 @@ static int adv748x_remove(struct i2c_client *client) {struct adv748x_state *state =i2c_get_clientdata (client); +adv748x_dai_cleanup (&state->dai);adv748x_afe_cleanup (&state->afe);adv748x_hdmi_cleanup (&state->hdmi);
drivers/media/i2c/adv748x/adv748x-dai.c new file +265 -0
265 lines not on this page · raw message
drivers/media/i2c/adv748x/adv748x.h +16 -1
@@ -19,6 +19,7 @@*/ #include <linux/i2c.h> +#include <sound/soc.h> #include <media/v4l2-ctrls.h> #include <media/v4l2-device.h>
@@ -63,7 +64,8 @@ enum adv748x_ports {ADV748X_PORT_TTL =9 ,ADV748X_PORT_TXA =10 ,ADV748X_PORT_TXB =11 , - ADV748X_PORT_MAX = 12, +ADV748X_PORT_I2S =12 , + ADV748X_PORT_MAX = 13, }; enum adv748x_csi2_pads {
@@ -166,6 +168,13 @@ struct adv748x_afe { container_of(ctrl->handler, struct adv748x_afe, ctrl_hdl)#define adv748x_sd_to_afe (sd) container_of(sd, struct adv748x_afe, sd) +struct adv748x_dai { +struct snd_soc_dai_driver drv; +struct clk *mclk; +char *mclk_name; +unsigned int freq, fmt, tdm; +}; +/** * struct adv748x_state - State of ADV748X * @dev: (OF) device
@@ -182,6 +191,7 @@ struct adv748x_afe {* @afe: state of AFE receiver context * @txa: state of TXA transmitter context * @txb: state of TXB transmitter context +* @mclk: MCLK clock of the I2S port */ struct adv748x_state {struct device *dev;
@@ -197,10 +207,12 @@ struct adv748x_state {struct adv748x_afe afe;struct adv748x_csi2 txa;struct adv748x_csi2 txb; +struct adv748x_dai dai; };#define adv748x_hdmi_to_state (h) container_of(h, struct adv748x_state, hdmi)#define adv748x_afe_to_state (a) container_of(a, struct adv748x_state, afe) +#define adv748x_dai_to_state (p) container_of(p, struct adv748x_state, dai) #define adv_err(a, fmt, arg...) dev_err(a->dev, fmt, ##arg) #define adv_info(a, fmt, arg...) dev_info(a->dev, fmt, ##arg)
@@ -484,4 +496,7 @@ int adv748x_csi2_set_pixelrate(struct v4l2_subdev *sd, s64 rate);int adv748x_hdmi_init (struct adv748x_hdmi *hdmi);void adv748x_hdmi_cleanup (struct adv748x_hdmi *hdmi); +int adv748x_dai_init (struct adv748x_dai *); +void adv748x_dai_cleanup (struct adv748x_dai *); +#endif /* _ADV748X_H_ */
Signature · 1 line
2.25.1.25.g9ecbe7eb18
Stephen Boyd
maintainerQuoting Alex Riesen (2020-03-20 09:12:00)
Quoted 6 lines
drivers/media/i2c/adv748x/adv748x-dai.c new file +0
-0
drivers/media/i2c/adv748x/adv748x-dai.c line 9
Quoted 9 lines
drivers/media/i2c/adv748x/adv748x-dai.c new file +9
-0@@ -0,0 +1,265 @@ +// SPDX-License-Identifier: GPL-2.0+ +/* +* Driver for Analog Devices ADV748X HDMI receiver with AFE +* The implementation of DAI. +*/ + +#include "adv748x.h" + +#include <linux/clk.h>
Is this include used? Please try to make a clk provider or a clk consumer and not both unless necessary.
Quoted 10 lines
drivers/media/i2c/adv748x/adv748x-dai.c new file +10
-0+#include <linux/clk-provider.h> +#include <sound/pcm_params.h> + +#define state_of (soc_dai) \ + adv748x_dai_to_state(container_of((soc_dai)->driver, \ + struct adv748x_dai, \ + drv)) + +static const char ADV748X_DAI_NAME [] ="adv748x-i2s" ; +
[...]
Quoted 25 lines
drivers/media/i2c/adv748x/adv748x-dai.c new file +25
-0+ .set_sysclk = adv748x_dai_set_sysclk, + .set_fmt = adv748x_dai_set_fmt, + .startup = adv748x_dai_startup, + .hw_params = adv748x_dai_hw_params, + .mute_stream = adv748x_dai_mute_stream, + .shutdown = adv748x_dai_shutdown, +}; + +static int adv748x_of_xlate_dai_name (struct snd_soc_component *component, +struct of_phandle_args *args, +const char **dai_name) +{ +if (dai_name) + *dai_name =ADV748X_DAI_NAME ; +return 0 ; +} + +static const struct snd_soc_component_driver adv748x_codec = { + .of_xlate_dai_name = adv748x_of_xlate_dai_name, +}; + +int adv748x_dai_init (struct adv748x_dai *dai) +{ +int ret; +struct adv748x_state *state =adv748x_dai_to_state (dai);
drivers/media/i2c/adv748x/adv748x-dai.c line 215
Quoted 10 lines
drivers/media/i2c/adv748x/adv748x-dai.c new file +10
-0+ + dai->mclk_name =kasprintf (GFP_KERNEL ,"%s.%s-i2s-mclk" , + state->dev->driver->name, +dev_name (state->dev)); +if (!dai->mclk_name) { + ret = -ENOMEM ; +adv_err (state,"No memory for MCLK\n" ); + goto fail; + } + dai->mclk = clk_register_fixed_rate(state->dev,
Please register with clk_hw_register_fixed_rate() instead.
drivers/media/i2c/adv748x/adv748x-dai.c line 219
Quoted 4 lines
drivers/media/i2c/adv748x/adv748x-dai.c new file +4
-0+ dai->mclk_name, +NULL /* parent_name */ , +0 /* flags */ , + 12288000 /* rate */);
Not sure these comments are useful.
Quoted 4 lines
drivers/media/i2c/adv748x/adv748x-dai.c new file +4
-0+if (IS_ERR_OR_NULL (dai->mclk)) { + ret =PTR_ERR (dai->mclk); +adv_err (state,"Failed to register MCLK (%d)\n" , ret); + goto fail;
drivers/media/i2c/adv748x/adv748x-dai.c line 233
Quoted 10 lines
drivers/media/i2c/adv748x/adv748x-dai.c new file +10
-0+ } + ret =of_clk_add_provider (state->dev->of_node, of_clk_src_simple_get, + dai->mclk); +if (ret <0 ) { +adv_err (state,"Failed to add MCLK provider (%d)\n" , ret); + goto unreg_mclk; + } + dai->drv.name =ADV748X_DAI_NAME ; + dai->drv.ops = &adv748x_dai_ops; + dai->drv.capture = (struct snd_soc_pcm_stream ){
Can this be const?
Quoted 22 lines
drivers/media/i2c/adv748x/adv748x-dai.c new file +22
-0+ .stream_name = "Capture", + .channels_min = 8, + .channels_max = 8, + .rates = SNDRV_PCM_RATE_48000, + .formats = SNDRV_PCM_FMTBIT_S24_LE | SNDRV_PCM_FMTBIT_U24_LE, + }; + + ret =devm_snd_soc_register_component (state->dev, &adv748x_codec, + &dai->drv,1 ); +if (ret <0 ) { +adv_err (state,"Failed to register the codec (%d)\n" , ret); + goto cleanup_mclk; + } +return 0 ; + +cleanup_mclk: +of_clk_del_provider (state->dev->of_node); +unreg_mclk: +clk_unregister_fixed_rate (dai->mclk); +fail: +return ret; +}
drivers/media/i2c/adv748x/adv748x-dai.c line 265
Quoted 10 lines
drivers/media/i2c/adv748x/adv748x-dai.c new file +10
-0+ +void adv748x_dai_cleanup (struct adv748x_dai *dai) +{ +struct adv748x_state *state =adv748x_dai_to_state (dai); + +of_clk_del_provider (state->dev->of_node); +clk_unregister_fixed_rate (dai->mclk); +kfree (dai->mclk_name); +} +
Please drop extra newline at end of file.
Stephen Boyd, Sat, Mar 21, 2020 02:09:31 +0100:
Quoted 11 lines
Quoting Alex Riesen (2020-03-20 09:12:00)
Quoted 10 lines
drivers/media/i2c/adv748x/adv748x-dai.c new file +4
-0@@ -0,0 +1,265 @@ +// SPDX-License-Identifier: GPL-2.0+ +/* +* Driver for Analog Devices ADV748X HDMI receiver with AFE +* The implementation of DAI.
Quoted 8 lines
drivers/media/i2c/adv748x/adv748x-dai.c line 9
Quoted 5 lines
drivers/media/i2c/adv748x/adv748x-dai.c new file +5
-0+*/ + +#include "adv748x.h" + +#include <linux/clk.h> Is this include used? Please try to make a clk provider or a clk consumer and not both unless necessary.
Yes, they're both used: I use clk_get_rate to get the frequency of the clock to validate the master clock frequency configuration.
Quoted 8 lines
Quoted 8 lines
drivers/media/i2c/adv748x/adv748x-dai.c new file +8
-0+int adv748x_dai_init (struct adv748x_dai *dai) +{ +int ret; +struct adv748x_state *state =adv748x_dai_to_state (dai); + + dai->mclk_name =kasprintf (GFP_KERNEL ,"%s.%s-i2s-mclk" , + state->dev->driver->name, +dev_name (state->dev));
Quoted 8 lines
drivers/media/i2c/adv748x/adv748x-dai.c line 215
Quoted 6 lines
drivers/media/i2c/adv748x/adv748x-dai.c new file +6
-0+if (!dai->mclk_name) { + ret = -ENOMEM ; +adv_err (state,"No memory for MCLK\n" ); + goto fail; + } + dai->mclk = clk_register_fixed_rate(state->dev,Please register with
clk_hw_register_fixed_rate()instead.
Ok, I see the clk_register_fixed_rate isn't even commented... I shall change it to used clk_hw_register_fixed_rate.
But could you please point me at some documentation, example, or just add a few words to explain the preference?
BTW, the clock which the driver attempts to provide can actually be switched on and off (it output pad can be even tristated). As the device is connected by I2C to the how, I seems to be unable to use the existing gated clock implementation because of the mmio register it requires. Or am I wrong?
Quoted 6 lines
drivers/media/i2c/adv748x/adv748x-dai.c line 219
Quoted 4 lines
drivers/media/i2c/adv748x/adv748x-dai.c new file +4
-0+ dai->mclk_name, +NULL /* parent_name */ , +0 /* flags */ , + 12288000 /* rate */);Not sure these comments are useful.
You're right. Removed.
Quoted 5 lines
drivers/media/i2c/adv748x/adv748x-dai.c line 233
Quoted 3 lines
drivers/media/i2c/adv748x/adv748x-dai.c new file +3
-0+ dai->drv.name =ADV748X_DAI_NAME ; + dai->drv.ops = &adv748x_dai_ops; + dai->drv.capture = (struct snd_soc_pcm_stream ){Can this be const?
It can. Fixed.
Quoted 1 line
Please drop extra newline at end of file.
Done.
Thanks a lot for the review and commentary!
Regards,
Alex
PATCH v3 6/11 media: adv748x: prepare/enable mclk when the audio is used
In reply to Alex RiesenAs there is nothing else (the consumers are supposed to do that) which enables the clock, do it in the driver.
--
v3: added
--- drivers/media/i2c/adv748x/adv748x-dai.c | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-)
drivers/media/i2c/adv748x/adv748x-dai.c +13 -1
@@ -117,11 +117,22 @@ static int adv748x_dai_set_fmt(struct snd_soc_dai *dai, unsigned int fmt)static int adv748x_dai_startup (struct snd_pcm_substream *sub,struct snd_soc_dai *dai) { +int ret;struct adv748x_state *state =state_of (dai);if (sub->stream !=SNDRV_PCM_STREAM_CAPTURE )return -EINVAL ; -return set_audio_pads_state (state,1 ); + ret =set_audio_pads_state (state,1 ); +if (ret) + goto fail; + ret =clk_prepare_enable (state->dai.mclk); +if (ret) + goto fail_pwdn; +return 0 ; +fail_pwdn: +set_audio_pads_state (state,0 ); +fail: +return ret; } static int adv748x_dai_hw_params(struct snd_pcm_substream *sub,
@@ -174,6 +185,7 @@ static void adv748x_dai_shutdown(struct snd_pcm_substream *sub, struct snd_soc_d {struct adv748x_state *state =state_of (dai); +clk_disable_unprepare (state->dai.mclk);set_audio_pads_state (state,0 ); }
Signature · 1 line
2.25.1.25.g9ecbe7eb18
PATCH v3 7/11 media: adv748x: only activate DAI if it is described in device tree
In reply to Alex RiesenTo avoid setting it up even if the hardware is not actually connected to anything physically.
Besides, the bindings explicitly notes that port definitions are "optional if they are not connected to anything at the hardware level".
--- drivers/media/i2c/adv748x/adv748x-dai.c | 5 +++++ 1 file changed, 5 insertions(+)
drivers/media/i2c/adv748x/adv748x-dai.c +5 -0
@@ -216,6 +216,11 @@ int adv748x_dai_init(struct adv748x_dai *dai)int ret;struct adv748x_state *state =adv748x_dai_to_state (dai); +if (!state->endpoints[ADV748X_PORT_I2S ]) { +adv_info (state,"no I2S port, DAI disabled\n" ); + ret =0 ; + goto fail; + } dai->mclk_name =kasprintf (GFP_KERNEL ,"%s.%s-i2s-mclk" , state->dev->driver->name,dev_name (state->dev));
Signature · 1 line
2.25.1.25.g9ecbe7eb18
PATCH v3 8/11 dt-bindings: adv748x: add information about serial audio interface (I2S/TDM)
In reply to Alex RiesenAs the driver has some support for the audio interface of the device, the bindings file should mention it.
--
v3: remove optionality off MCLK clock cell to ensure the description
matches the hardware no matter if the line is connected.
--- .../devicetree/bindings/media/i2c/adv748x.txt | 16 +++++++++++++++- 1 file changed, 15 insertions(+), 1 deletion(-)
Documentation/devicetree/bindings/media/i2c/adv748x.txt +15 -1
@@ -2,7 +2,9 @@ The ADV7481 and ADV7482 are multi format video decoders with an integrated HDMI receiver. They can output CSI-2 on two independent outputs TXA and TXB -from three input sources HDMI, analog and TTL. +from three input sources HDMI, analog and TTL. There is also support for an +I2S-compatible interface connected to the audio processor of the HDMI decoder. +The interface has TDM capability (8 slots, 32 bits, left or right justified). Required Properties:
@@ -16,6 +18,8 @@ Required Properties: slave device on the I2C bus. The main address is mandatory, others are optional and remain at default values if not specified. + - #clock-cells: must be <0> + Optional Properties: - interrupt-names: Should specify the interrupts as "intrq1", "intrq2" and/or
@@ -47,6 +51,7 @@ are numbered as follows. TTL sink 9 TXA source 10 TXB source 11 + I2S source 12 The digital output port nodes, when present, shall contain at least one endpoint. Each of those endpoints shall contain the data-lanes property as
@@ -72,6 +77,7 @@ Example: #address-cells = <1>; #size-cells = <0>; + #clock-cells = <0>; interrupt-parent = <&gpio6>; interrupt-names = "intrq1", "intrq2";
@@ -113,4 +119,12 @@ Example: remote-endpoint = <&csi20_in>; }; }; + + port@c { + reg = <12>; + + adv7482_i2s: endpoint { + remote-endpoint = <&i2s_in>; + }; + }; };
Signature · 1 line
2.25.1.25.g9ecbe7eb18
PATCH v3 9/11 arm64: dts: renesas: salvator: add a connection from adv748x codec (HDMI input) to the R-Car SoC
In reply to Alex RiesenAs all known variants of the Salvator board have the HDMI decoder chip (the ADV7482) connected to the SSI4 on R-Car SoC, the ADV7482 endpoint and the connection definitions are placed in the common board file.
For the same reason, the CLK_C clock line and I2C configuration (similar to the ak4613, on the same interface) are added into the common file.
--
v2:
Also add ssi4_ctrl pin group in the sound pins. The pins are responsible for SCK4 (sample clock) WS4 and (word boundary input), and are required for SSI audio input over I2S.
The adv748x shall provide its own implementation of the output clock (MCLK), connected to the audio_clk_c line of the R-Car SoC.
If the frequency of the ADV748x MCLK were fixed, the clock implementation were not necessary, but it does not seem so: the MCLK depends on the value in a speed multiplier register and the input sample rate (48kHz).
Remove audio clock C from the clocks of adv7482.
The clocks property of the video-receiver node lists the input clocks of the device, which is quite the opposite from the original intention: the adv7482 on Salvator X boards is a provide of the MCLK clock for I2S audio output.
Remove old definition of &sound_card.dais and reduce size of changes in the Salvator-X specific device tree source.
Declare video-receiver a clock producer, as the adv748x driver implements the master clock used I2S audio output.
The driver provides only MCLK clock, not the SCLK and LRCLK, which are part of the I2S protocol.
--- .../boot/dts/renesas/r8a77950-salvator-x.dts | 3 +- .../boot/dts/renesas/salvator-common.dtsi | 47 +++++++++++++++++-- 2 files changed, 44 insertions(+), 6 deletions(-)
arch/arm64/boot/dts/renesas/r8a77950-salvator-x.dts +2 -1
@@ -146,7 +146,8 @@ &sata { &sound_card { dais = <&rsnd_port0 /* ak4613 */ &rsnd_port1 /* HDMI0 */ - &rsnd_port2>; /* HDMI1 */ + &rsnd_port2 /* HDMI1 */ + &rsnd_port3>; /* adv7482 hdmi-in */ }; &usb2_phy2 {
arch/arm64/boot/dts/renesas/salvator-common.dtsi +42 -5
90 lines not on this page · raw message
Signature · 1 line
2.25.1.25.g9ecbe7eb18
Re: [PATCH v3 09/11] arm64: dts: renesas: salvator: add a connection from adv748x codec (HDMI input) to the R-Car SoC
Hi Alex,
Quoted 9 lines · Alex Riesen
As all known variants of the Salvator board have the HDMI decoder chip (the ADV7482) connected to the SSI4 on R-Car SoC, the ADV7482 endpoint and the connection definitions are placed in the common board file.
For the same reason, theCLK_Cclock line and I2C configuration (similar to the ak4613, on the same interface) are added into the common file.Signed-off-by Alexander Riesen <[email protected]>Reviewed-by Geert Uytterhoeven <[email protected]>
Did I provide a Reviewed-by?
Quoted 4 lines
The driver provides only MCLK clock, not the SCLK and LRCLK, which are part of the I2S protocol.
Reviewed-by Laurent Pinchart <[email protected]>
Perhaps you mixed it up with Laurent's?
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]
In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
Re: [PATCH v3 09/11] arm64: dts: renesas: salvator: add a connection from adv748x codec (HDMI input) to the R-Car SoC
Geert Uytterhoeven, Mon, Mar 23, 2020 09:34:45 +0100:
Quoted 11 lines · Alex Riesen
Quoted 9 lines
As all known variants of the Salvator board have the HDMI decoder chip (the ADV7482) connected to the SSI4 on R-Car SoC, the ADV7482 endpoint and the connection definitions are placed in the common board file.
For the same reason, theCLK_Cclock line and I2C configuration (similar to the ak4613, on the same interface) are added into the common file.Signed-off-by Alexander Riesen <[email protected]>Reviewed-by Geert Uytterhoeven <[email protected]>
Quoted 8 lines
Did I provide a Reviewed-by?
Quoted 4 lines
The driver provides only MCLK clock, not the SCLK and LRCLK, which are part of the I2S protocol.
Reviewed-by Laurent Pinchart <[email protected]>Perhaps you mixed it up with Laurent's?
Sorry. I actually did: he did provded Reviewed-by in his email, and you did not. I was ... a little overwhelmed.
But you really did provide a lot of very useful information and it did help to improve the code. Shall I remove the tag still?
Regards,
Alex
Re: [PATCH v3 09/11] arm64: dts: renesas: salvator: add a connection from adv748x codec (HDMI input) to the R-Car SoC
Alex Riesen, Mon, Mar 23, 2020 09:40:11 +0100:
Quoted 17 lines
Geert Uytterhoeven, Mon, Mar 23, 2020 09:34:45 +0100:
Quoted 16 lines · Alex Riesen
Quoted 9 lines
As all known variants of the Salvator board have the HDMI decoder chip (the ADV7482) connected to the SSI4 on R-Car SoC, the ADV7482 endpoint and the connection definitions are placed in the common board file.
For the same reason, theCLK_Cclock line and I2C configuration (similar to the ak4613, on the same interface) are added into the common file.Signed-off-by Alexander Riesen <[email protected]>Reviewed-by Geert Uytterhoeven <[email protected]>Did I provide a Reviewed-by?
Quoted 3 lines
The driver provides only MCLK clock, not the SCLK and LRCLK, which are part of the I2S protocol.
Quoted 6 lines
Quoted 3 lines
Quoted 1 line
Reviewed-by Laurent Pinchart <[email protected]>Perhaps you mixed it up with Laurent's?
Sorry. I actually did: he did provded Reviewed-by in his email, and you did not. I was ... a little overwhelmed.
It's even worse: even Laurent didn't provide his Reviewed-by for this particular patch (it was bindings). Corrected.
Regards,
Alex
Re: [PATCH v3 09/11] arm64: dts: renesas: salvator: add a connection from adv748x codec (HDMI input) to the R-Car SoC
In reply to Alex RiesenHi Alex,
Quoted 19 lines · Alex Riesen
Geert Uytterhoeven, Mon, Mar 23, 2020 09:34:45 +0100:
Quoted 18 lines · Alex Riesen
Quoted 9 lines
As all known variants of the Salvator board have the HDMI decoder chip (the ADV7482) connected to the SSI4 on R-Car SoC, the ADV7482 endpoint and the connection definitions are placed in the common board file.
For the same reason, theCLK_Cclock line and I2C configuration (similar to the ak4613, on the same interface) are added into the common file.Signed-off-by Alexander Riesen <[email protected]>Reviewed-by Geert Uytterhoeven <[email protected]>Did I provide a Reviewed-by?
Quoted 4 lines
The driver provides only MCLK clock, not the SCLK and LRCLK, which are part of the I2S protocol.
Reviewed-by Laurent Pinchart <[email protected]>
Quoted 7 lines
Quoted 1 line
Perhaps you mixed it up with Laurent's?
Sorry. I actually did: he did provded Reviewed-by in his email, and you did not. I was ... a little overwhelmed.
But you really did provide a lot of very useful information and it did help to improve the code. Shall I remove the tag still?
Please do so.
While I can point out issues in audio patches, my audio-foo is not strong enough to provide an R-B, and I'll rely on the R-bs provided by others.
Gr{oetje,eeting}s,
Geert
Signature · 5 lines
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected] In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds
Re: [PATCH v3 09/11] arm64: dts: renesas: salvator: add a connection from adv748x codec (HDMI input) to the R-Car SoC
Hi Geert,
Geert Uytterhoeven, Mon, Mar 23, 2020 09:48:00 +0100:
Quoted 7 lines · Alex Riesen
Quoted 6 lines
Geert Uytterhoeven, Mon, Mar 23, 2020 09:34:45 +0100:
Quoted 5 lines · Alex Riesen
Quoted 1 line
Reviewed-by Geert Uytterhoeven <[email protected]>Did I provide a Reviewed-by?
Quoted 7 lines
Quoted 2 lines
But you really did provide a lot of very useful information and it did help to improve the code. Shall I remove the tag still?
Please do so.
While I can point out issues in audio patches, my audio-foo is not strong enough to provide an R-B, and I'll rely on the R-bs provided by others.
Done. I left the suggested-bys in the trailer. Hope those are alright.
Regards,
Alex
PATCH v3 10/11 media: adv748x: add support for log_status ioctl
In reply to Alex RiesenThe logged information provides insights about cable connection and the state of the HDMI decoder. It is very useful when debugging hardware problems in environments without easy access to the connectors.
This change adds a device-specific wrapper for register block read, because some of the devices I2C-accessible registers (for instance, cs_data for stereo channel information or tmds_params for TMDS channel information) located in adjacent cells. According to manufacturers information, these registers can be read using block transactions.
--- drivers/media/i2c/adv748x/adv748x-core.c | 15 ++ drivers/media/i2c/adv748x/adv748x-hdmi.c | 179 +++++++++++++++++++++++ drivers/media/i2c/adv748x/adv748x.h | 2 + 3 files changed, 196 insertions(+)
drivers/media/i2c/adv748x/adv748x-core.c +15 -0
@@ -95,6 +95,21 @@ static const struct adv748x_register_map adv748x_default_addresses[] = { [ADV748X_PAGE_TXA] = { "txa", 0x4a }, }; +int adv748x_read_block (struct adv748x_state *state,u8 client_page,u8 reg, +void *val,size_t reg_count) +{ +struct i2c_client *client = state->i2c_clients[client_page]; +int err; + + err =regmap_bulk_read (state->regmap[client_page], reg, val, reg_count); +if (err) { +adv_err (state,"error reading %02x, %02x-%02lx: %d\n" , + client->addr, reg, reg + reg_count -1 , err); +return err; + } +return 0 ; +} +static int adv748x_read_check (struct adv748x_state *state,int client_page,u8 reg) {
drivers/media/i2c/adv748x/adv748x-hdmi.c +179 -0
196 lines not on this page · raw message
drivers/media/i2c/adv748x/adv748x.h +2 -0
@@ -433,6 +433,8 @@ struct adv748x_state {/* Register handling */ int adv748x_read (struct adv748x_state *state,u8 addr,u8 reg); +int adv748x_read_block (struct adv748x_state *state,u8 page,u8 reg, +void *val,size_t reg_count);int adv748x_write (struct adv748x_state *state,u8 page,u8 reg,u8 value); int adv748x_write_block(struct adv748x_state *state, int client_page, unsigned int init_reg, const void *val,
Signature · 1 line
2.25.1.25.g9ecbe7eb18
PATCH v3 11/11 media: adv748x: allow the HDMI sub-device to accept EDID
In reply to Alex RiesenThis makes it possible to load a EDID reported by the device with v4l2-ctl utility:
vdev=/dev/$(grep -l '^adv748x.*hdmi$' /sys/class/video4linux/v4l-subdev*/name |cut -d/ -f5-5) v4l2-ctl -d $vdev --set-edid=pad=0,file=/etc/adv7482.edid
--
I would like to avoid doing that, but found no other way yet. Suggestions very welcome.
--- drivers/media/i2c/adv748x/adv748x-hdmi.c | 27 ++++++++++++++++++++++++ 1 file changed, 27 insertions(+)
drivers/media/i2c/adv748x/adv748x-hdmi.c +27 -0
@@ -775,7 +775,34 @@ static int adv748x_hdmi_log_status(struct v4l2_subdev *sd)return 0 ; } +static long adv748x_hdmi_querycap (struct adv748x_hdmi *hdmi, +struct v4l2_capability *cap) +{ +struct adv748x_state *state =adv748x_hdmi_to_state (hdmi); + + cap->version =LINUX_VERSION_CODE ; +strlcpy (cap->driver, state->dev->driver->name,sizeof (cap->driver)); +strlcpy (cap->card,"hdmi" ,sizeof (cap->card)); +snprintf (cap->bus_info,sizeof (cap->bus_info),"i2c:%d-%04x" , +i2c_adapter_id (state->client->adapter), + state->client->addr); + cap->device_caps =V4L2_CAP_AUDIO |V4L2_CAP_VIDEO_CAPTURE ; + cap->capabilities =V4L2_CAP_DEVICE_CAPS ; +return 0 ; +} + +static long adv748x_hdmi_ioctl (struct v4l2_subdev *sd, +unsigned int cmd,void *arg) +{ +struct adv748x_hdmi *hdmi =adv748x_sd_to_hdmi (sd); + +if (cmd ==VIDIOC_QUERYCAP ) +return adv748x_hdmi_querycap (hdmi, arg); +return -ENOTTY ; +} +static const struct v4l2_subdev_core_ops adv748x_core_ops_hdmi = { + .ioctl = adv748x_hdmi_ioctl, .log_status = adv748x_hdmi_log_status, };
Signature · 1 line
2.25.1.25.g9ecbe7eb18