[PATCH v1 1/2] drm/i2c: tda998x: access chip registers via a regmap

[PATCH v1 1/2] drm/i2c: tda998x: access chip registers via a regmap

[Sven Van Asbroeck]

The tda988x i2c chip registers are accessed through a
paged register scheme. The kernel regmap abstraction
supports paged accesses. Replace this driver's
dedicated i2c access functions with a standard i2c
regmap.

Pros:
- dedicated i2c access code disappears: accesses now
  go through well-maintained regmap code
- page atomicity requirements now handled by regmap
- ro/wo/rw access modes are now explicitly defined:
  any inappropriate register accesses (e.g. write to a
  read-only register) will now be explicitly rejected
  by the regmap core
- all tda988x registers are now visible via debugfs

Cons:
- this will probably require extensive testing

Tested on a TDA19988 using a Freescale/NXP imx6q.

Signed-off-by: Sven Van Asbroeck <TheSven73@gmail.com>
---

I originally hacked this together while debugging an incompatibility between
the tda988x's audio input and the audio codec I was driving it with.
That required me to have debug access to the chip's register values.
A regmap did the trick, it has good debugfs support.

 drivers/gpu/drm/i2c/tda998x_drv.c | 350 ++++++++++++++++++++----------
 1 file changed, 234 insertions(+), 116 deletions(-)

diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c b/drivers/gpu/drm/i2c/tda998x_drv.c
index 7f34601bb515..8153e2e19e18 100644
--- a/drivers/gpu/drm/i2c/tda998x_drv.c
+++ b/drivers/gpu/drm/i2c/tda998x_drv.c
@@ -21,6 +21,7 @@
 #include <linux/module.h>
 #include <linux/platform_data/tda9950.h>
 #include <linux/irq.h>
+#include <linux/regmap.h>
 #include <sound/asoundef.h>
 #include <sound/hdmi-codec.h>
 
@@ -43,10 +44,9 @@ struct tda998x_audio_port {
 struct tda998x_priv {
 	struct i2c_client *cec;
 	struct i2c_client *hdmi;
-	struct mutex mutex;
+	struct regmap *regmap;
 	u16 rev;
 	u8 cec_addr;
-	u8 current_page;
 	bool is_on;
 	bool supports_infoframes;
 	bool sink_has_audio;
@@ -88,12 +88,10 @@ struct tda998x_priv {
 /* The TDA9988 series of devices use a paged register scheme.. to simplify
  * things we encode the page # in upper bits of the register #.  To read/
  * write a given register, we need to make sure CURPAGE register is set
- * appropriately.  Which implies reads/writes are not atomic.  Fun!
+ * appropriately.
  */
 
 #define REG(page, addr) (((page) << 8) | (addr))
-#define REG2ADDR(reg)   ((reg) & 0xff)
-#define REG2PAGE(reg)   (((reg) >> 8) & 0xff)
 
 #define REG_CURPAGE               0xff                /* write */
 
@@ -285,8 +283,9 @@ struct tda998x_priv {
 
 
 /* Page 09h: EDID Control */
+/*	EDID_DATA consists of 128 successive registers   read */
 #define REG_EDID_DATA_0           REG(0x09, 0x00)     /* read */
-/* next 127 successive registers are the EDID block */
+#define REG_EDID_DATA_127         REG(0x09, 0x7f)     /* read */
 #define REG_EDID_CTRL             REG(0x09, 0xfa)     /* read/write */
 #define REG_DDC_ADDR              REG(0x09, 0xfb)     /* read/write */
 #define REG_DDC_OFFS              REG(0x09, 0xfc)     /* read/write */
@@ -295,11 +294,17 @@ struct tda998x_priv {
 
 
 /* Page 10h: information frames and packets */
+/*	REG_IF1_HB consists of 32 successive registers	 read/write */
 #define REG_IF1_HB0               REG(0x10, 0x20)     /* read/write */
+/*	REG_IF2_HB consists of 32 successive registers	 read/write */
 #define REG_IF2_HB0               REG(0x10, 0x40)     /* read/write */
+/*	REG_IF3_HB consists of 32 successive registers	 read/write */
 #define REG_IF3_HB0               REG(0x10, 0x60)     /* read/write */
+/*	REG_IF4_HB consists of 32 successive registers	 read/write */
 #define REG_IF4_HB0               REG(0x10, 0x80)     /* read/write */
+/*	REG_IF5_HB consists of 32 successive registers	 read/write */
 #define REG_IF5_HB0               REG(0x10, 0xa0)     /* read/write */
+#define REG_IF5_HB31              REG(0x10, 0xbf)     /* read/write */
 
 
 /* Page 11h: audio settings and content info packets */
@@ -542,93 +547,29 @@ static void tda998x_cec_hook_release(void *data)
 	cec_enamods(priv, CEC_ENAMODS_EN_CEC_CLK | CEC_ENAMODS_EN_CEC, false);
 }
 
-static int
-set_page(struct tda998x_priv *priv, u16 reg)
-{
-	if (REG2PAGE(reg) != priv->current_page) {
-		struct i2c_client *client = priv->hdmi;
-		u8 buf[] = {
-				REG_CURPAGE, REG2PAGE(reg)
-		};
-		int ret = i2c_master_send(client, buf, sizeof(buf));
-		if (ret < 0) {
-			dev_err(&client->dev, "%s %04x err %d\n", __func__,
-					reg, ret);
-			return ret;
-		}
-
-		priv->current_page = REG2PAGE(reg);
-	}
-	return 0;
-}
-
 static int
 reg_read_range(struct tda998x_priv *priv, u16 reg, char *buf, int cnt)
 {
-	struct i2c_client *client = priv->hdmi;
-	u8 addr = REG2ADDR(reg);
 	int ret;
 
-	mutex_lock(&priv->mutex);
-	ret = set_page(priv, reg);
-	if (ret < 0)
-		goto out;
-
-	ret = i2c_master_send(client, &addr, sizeof(addr));
+	ret = regmap_bulk_read(priv->regmap, reg, buf, cnt);
 	if (ret < 0)
-		goto fail;
-
-	ret = i2c_master_recv(client, buf, cnt);
-	if (ret < 0)
-		goto fail;
-
-	goto out;
-
-fail:
-	dev_err(&client->dev, "Error %d reading from 0x%x\n", ret, reg);
-out:
-	mutex_unlock(&priv->mutex);
-	return ret;
+		return ret;
+	return cnt;
 }
 
-#define MAX_WRITE_RANGE_BUF 32
-
 static void
 reg_write_range(struct tda998x_priv *priv, u16 reg, u8 *p, int cnt)
 {
-	struct i2c_client *client = priv->hdmi;
-	/* This is the maximum size of the buffer passed in */
-	u8 buf[MAX_WRITE_RANGE_BUF + 1];
-	int ret;
-
-	if (cnt > MAX_WRITE_RANGE_BUF) {
-		dev_err(&client->dev, "Fixed write buffer too small (%d)\n",
-				MAX_WRITE_RANGE_BUF);
-		return;
-	}
-
-	buf[0] = REG2ADDR(reg);
-	memcpy(&buf[1], p, cnt);
-
-	mutex_lock(&priv->mutex);
-	ret = set_page(priv, reg);
-	if (ret < 0)
-		goto out;
-
-	ret = i2c_master_send(client, buf, cnt + 1);
-	if (ret < 0)
-		dev_err(&client->dev, "Error %d writing to 0x%x\n", ret, reg);
-out:
-	mutex_unlock(&priv->mutex);
+	regmap_bulk_write(priv->regmap, reg, p, cnt);
 }
 
 static int
 reg_read(struct tda998x_priv *priv, u16 reg)
 {
-	u8 val = 0;
-	int ret;
+	int ret, val;
 
-	ret = reg_read_range(priv, reg, &val, sizeof(val));
+	ret = regmap_read(priv->regmap, reg, &val);
 	if (ret < 0)
 		return ret;
 	return val;
@@ -637,59 +578,27 @@ reg_read(struct tda998x_priv *priv, u16 reg)
 static void
 reg_write(struct tda998x_priv *priv, u16 reg, u8 val)
 {
-	struct i2c_client *client = priv->hdmi;
-	u8 buf[] = {REG2ADDR(reg), val};
-	int ret;
-
-	mutex_lock(&priv->mutex);
-	ret = set_page(priv, reg);
-	if (ret < 0)
-		goto out;
-
-	ret = i2c_master_send(client, buf, sizeof(buf));
-	if (ret < 0)
-		dev_err(&client->dev, "Error %d writing to 0x%x\n", ret, reg);
-out:
-	mutex_unlock(&priv->mutex);
+	regmap_write(priv->regmap, reg, val);
 }
 
 static void
 reg_write16(struct tda998x_priv *priv, u16 reg, u16 val)
 {
-	struct i2c_client *client = priv->hdmi;
-	u8 buf[] = {REG2ADDR(reg), val >> 8, val};
-	int ret;
-
-	mutex_lock(&priv->mutex);
-	ret = set_page(priv, reg);
-	if (ret < 0)
-		goto out;
+	u8 buf[] = {val >> 8, val};
 
-	ret = i2c_master_send(client, buf, sizeof(buf));
-	if (ret < 0)
-		dev_err(&client->dev, "Error %d writing to 0x%x\n", ret, reg);
-out:
-	mutex_unlock(&priv->mutex);
+	regmap_bulk_write(priv->regmap, reg, buf, ARRAY_SIZE(buf));
 }
 
 static void
 reg_set(struct tda998x_priv *priv, u16 reg, u8 val)
 {
-	int old_val;
-
-	old_val = reg_read(priv, reg);
-	if (old_val >= 0)
-		reg_write(priv, reg, old_val | val);
+	regmap_update_bits(priv->regmap, reg, val, val);
 }
 
 static void
 reg_clear(struct tda998x_priv *priv, u16 reg, u8 val)
 {
-	int old_val;
-
-	old_val = reg_read(priv, reg);
-	if (old_val >= 0)
-		reg_write(priv, reg, old_val & ~val);
+	regmap_update_bits(priv->regmap, reg, val, 0);
 }
 
 static void
@@ -816,7 +725,7 @@ static void
 tda998x_write_if(struct tda998x_priv *priv, u8 bit, u16 addr,
 		 union hdmi_infoframe *frame)
 {
-	u8 buf[MAX_WRITE_RANGE_BUF];
+	u8 buf[32];
 	ssize_t len;
 
 	len = hdmi_infoframe_pack(frame, buf, sizeof(buf));
@@ -1654,6 +1563,211 @@ static void tda998x_destroy(struct device *dev)
 		cec_notifier_put(priv->cec_notify);
 }
 
+static bool tda_is_edid_data_reg(unsigned int reg)
+{
+	return (reg >= REG_EDID_DATA_0) &&
+		(reg <= REG_EDID_DATA_127);
+}
+
+static bool tda_is_precious_volatile_reg(struct device *dev, unsigned int reg)
+{
+	if (tda_is_edid_data_reg(reg))
+		return true;
+	switch (reg) {
+	case REG_INT_FLAGS_0:
+	case REG_INT_FLAGS_1:
+	case REG_INT_FLAGS_2:
+		return true;
+	default:
+		return false;
+	}
+}
+
+static bool tda_is_rw_reg(unsigned int reg)
+{
+	if ((reg >= REG_IF1_HB0) && (reg <= REG_IF5_HB31))
+		return true;
+	switch (reg) {
+	case REG_MAIN_CNTRL0:
+	case REG_DDC_DISABLE:
+	case REG_CCLK_ON:
+	case REG_I2C_MASTER:
+	case REG_FEAT_POWERDOWN:
+	case REG_INT_FLAGS_0:
+	case REG_INT_FLAGS_1:
+	case REG_INT_FLAGS_2:
+	case REG_ENA_ACLK:
+	case REG_ENA_VP_0:
+	case REG_ENA_VP_1:
+	case REG_ENA_VP_2:
+	case REG_ENA_AP:
+	case REG_MUX_AP:
+	case REG_MUX_VP_VIP_OUT:
+	case REG_I2S_FORMAT:
+	case REG_PLL_SERIAL_1:
+	case REG_PLL_SERIAL_2:
+	case REG_PLL_SERIAL_3:
+	case REG_SERIALIZER:
+	case REG_BUFFER_OUT:
+	case REG_PLL_SCG1:
+	case REG_PLL_SCG2:
+	case REG_PLL_SCGN1:
+	case REG_PLL_SCGN2:
+	case REG_PLL_SCGR1:
+	case REG_PLL_SCGR2:
+	case REG_AUDIO_DIV:
+	case REG_SEL_CLK:
+	case REG_ANA_GENERAL:
+	case REG_EDID_CTRL:
+	case REG_DDC_ADDR:
+	case REG_DDC_OFFS:
+	case REG_DDC_SEGM_ADDR:
+	case REG_DDC_SEGM:
+	case REG_AIP_CNTRL_0:
+	case REG_CA_I2S:
+	case REG_LATENCY_RD:
+	case REG_ACR_CTS_0:
+	case REG_ACR_CTS_1:
+	case REG_ACR_CTS_2:
+	case REG_ACR_N_0:
+	case REG_ACR_N_1:
+	case REG_ACR_N_2:
+	case REG_CTS_N:
+	case REG_ENC_CNTRL:
+	case REG_DIP_FLAGS:
+	case REG_DIP_IF_FLAGS:
+	case REG_TX3:
+	case REG_TX4:
+	case REG_TX33:
+	case REG_CH_STAT_B(0):
+	case REG_CH_STAT_B(1):
+	case REG_CH_STAT_B(2):
+	case REG_CH_STAT_B(3):
+		return true;
+	default:
+		return false;
+	}
+}
+
+static bool tda_is_readable_reg(struct device *dev, unsigned int reg)
+{
+	if (tda_is_rw_reg(reg) || tda_is_edid_data_reg(reg))
+		return true;
+	switch (reg) {
+	case REG_VERSION_LSB:
+	case REG_VERSION_MSB:
+		return true;
+	default:
+		return false;
+	}
+}
+
+static bool tda_is_writeable_reg(struct device *dev, unsigned int reg)
+{
+	if (tda_is_rw_reg(reg))
+		return true;
+	switch (reg) {
+	case REG_CURPAGE:
+	case REG_SOFTRESET:
+	case REG_VIP_CNTRL_0:
+	case REG_VIP_CNTRL_1:
+	case REG_VIP_CNTRL_2:
+	case REG_VIP_CNTRL_3:
+	case REG_VIP_CNTRL_4:
+	case REG_VIP_CNTRL_5:
+	case REG_MAT_CONTRL:
+	case REG_VIDFORMAT:
+	case REG_REFPIX_MSB:
+	case REG_REFPIX_LSB:
+	case REG_REFLINE_MSB:
+	case REG_REFLINE_LSB:
+	case REG_NPIX_MSB:
+	case REG_NPIX_LSB:
+	case REG_NLINE_MSB:
+	case REG_NLINE_LSB:
+	case REG_VS_LINE_STRT_1_MSB:
+	case REG_VS_LINE_STRT_1_LSB:
+	case REG_VS_PIX_STRT_1_MSB:
+	case REG_VS_PIX_STRT_1_LSB:
+	case REG_VS_LINE_END_1_MSB:
+	case REG_VS_LINE_END_1_LSB:
+	case REG_VS_PIX_END_1_MSB:
+	case REG_VS_PIX_END_1_LSB:
+	case REG_VS_LINE_STRT_2_MSB:
+	case REG_VS_LINE_STRT_2_LSB:
+	case REG_VS_PIX_STRT_2_MSB:
+	case REG_VS_PIX_STRT_2_LSB:
+	case REG_VS_LINE_END_2_MSB:
+	case REG_VS_LINE_END_2_LSB:
+	case REG_VS_PIX_END_2_MSB:
+	case REG_VS_PIX_END_2_LSB:
+	case REG_HS_PIX_START_MSB:
+	case REG_HS_PIX_START_LSB:
+	case REG_HS_PIX_STOP_MSB:
+	case REG_HS_PIX_STOP_LSB:
+	case REG_VWIN_START_1_MSB:
+	case REG_VWIN_START_1_LSB:
+	case REG_VWIN_END_1_MSB:
+	case REG_VWIN_END_1_LSB:
+	case REG_VWIN_START_2_MSB:
+	case REG_VWIN_START_2_LSB:
+	case REG_VWIN_END_2_MSB:
+	case REG_VWIN_END_2_LSB:
+	case REG_DE_START_MSB:
+	case REG_DE_START_LSB:
+	case REG_DE_STOP_MSB:
+	case REG_DE_STOP_LSB:
+	case REG_TBG_CNTRL_0:
+	case REG_TBG_CNTRL_1:
+	case REG_ENABLE_SPACE:
+	case REG_HVF_CNTRL_0:
+	case REG_HVF_CNTRL_1:
+	case REG_RPT_CNTRL:
+	case REG_AIP_CLKSEL:
+		return true;
+	default:
+		return false;
+	}
+}
+
+static const struct reg_default tda_reg_default[] = {
+	{ REG_CURPAGE, 0xff },
+};
+
+static const struct regmap_range_cfg hdmi_range_cfg[] = {
+	{
+		.range_min = 0x00,
+		.range_max = REG_TX33,
+		.selector_reg = REG_CURPAGE,
+		.selector_mask = 0xff,
+		.selector_shift = 0,
+		.window_start = 0,
+		.window_len = 0x100,
+	},
+};
+
+/* Paged register scheme, with a write-only page register (CURPAGE).
+ * Make this work by marking CURPAGE write-only and cacheable. Give it
+ * an invalid page default value, which guarantees that it will get written to
+ * the first time we read/write the registers.
+ */
+
+static const struct regmap_config hdmi_regmap_config = {
+	.reg_bits = 8,
+	.val_bits = 8,
+	.ranges = hdmi_range_cfg,
+	.num_ranges = ARRAY_SIZE(hdmi_range_cfg),
+	.max_register = REG_TX33,
+
+	.cache_type = REGCACHE_RBTREE,
+	.volatile_reg = tda_is_precious_volatile_reg,
+	.precious_reg = tda_is_precious_volatile_reg,
+	.readable_reg = tda_is_readable_reg,
+	.writeable_reg = tda_is_writeable_reg,
+	.reg_defaults = tda_reg_default,
+	.num_reg_defaults = ARRAY_SIZE(tda_reg_default),
+};
+
 static int tda998x_create(struct device *dev)
 {
 	struct i2c_client *client = to_i2c_client(dev);
@@ -1666,10 +1780,15 @@ static int tda998x_create(struct device *dev)
 	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
 	if (!priv)
 		return -ENOMEM;
+	priv->regmap = devm_regmap_init_i2c(client, &hdmi_regmap_config);
+	if (IS_ERR(priv->regmap)) {
+		ret = PTR_ERR(priv->regmap);
+		dev_err(dev, "Failed to allocate register map: %d\n", ret);
+		return ret;
+	}
 
 	dev_set_drvdata(dev, priv);
 
-	mutex_init(&priv->mutex);	/* protect the page access */
 	mutex_init(&priv->audio_mutex); /* protect access from audio thread */
 	mutex_init(&priv->edid_mutex);
 	INIT_LIST_HEAD(&priv->bridge.list);
@@ -1683,7 +1802,6 @@ static int tda998x_create(struct device *dev)
 
 	/* CEC I2C address bound to TDA998x I2C addr by configuration pins */
 	priv->cec_addr = 0x34 + (client->addr & 0x03);
-	priv->current_page = 0xff;
 	priv->hdmi = client;
 
 	/* wake up the device: */
-- 
2.17.1

[PATCH v1 2/2] drm/i2c: tda998x: remove indirect reg_read/_write() calls

[Sven Van Asbroeck]

Remove indirect reg_read/_write() calls, and replace them
with direct calls to regmap functions.

For the sake of readability, keep the following indirect
register access calls:
- reg_set()
- reg_clear()
- reg_write16()

Signed-off-by: Sven Van Asbroeck <TheSven73@gmail.com>
---
 drivers/gpu/drm/i2c/tda998x_drv.c | 333 ++++++++++++++----------------
 1 file changed, 157 insertions(+), 176 deletions(-)

diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c b/drivers/gpu/drm/i2c/tda998x_drv.c
index 8153e2e19e18..1bddd2cf92ea 100644
--- a/drivers/gpu/drm/i2c/tda998x_drv.c
+++ b/drivers/gpu/drm/i2c/tda998x_drv.c
@@ -548,89 +548,59 @@ static void tda998x_cec_hook_release(void *data)
 }
 
 static int
-reg_read_range(struct tda998x_priv *priv, u16 reg, char *buf, int cnt)
-{
-	int ret;
-
-	ret = regmap_bulk_read(priv->regmap, reg, buf, cnt);
-	if (ret < 0)
-		return ret;
-	return cnt;
-}
-
-static void
-reg_write_range(struct tda998x_priv *priv, u16 reg, u8 *p, int cnt)
-{
-	regmap_bulk_write(priv->regmap, reg, p, cnt);
-}
-
-static int
-reg_read(struct tda998x_priv *priv, u16 reg)
-{
-	int ret, val;
-
-	ret = regmap_read(priv->regmap, reg, &val);
-	if (ret < 0)
-		return ret;
-	return val;
-}
-
-static void
-reg_write(struct tda998x_priv *priv, u16 reg, u8 val)
-{
-	regmap_write(priv->regmap, reg, val);
-}
-
-static void
-reg_write16(struct tda998x_priv *priv, u16 reg, u16 val)
+reg_write16(struct regmap *regmap, u16 reg, u16 val)
 {
 	u8 buf[] = {val >> 8, val};
 
-	regmap_bulk_write(priv->regmap, reg, buf, ARRAY_SIZE(buf));
+	return regmap_bulk_write(regmap, reg, buf, ARRAY_SIZE(buf));
 }
 
-static void
-reg_set(struct tda998x_priv *priv, u16 reg, u8 val)
+static int
+reg_set(struct regmap *regmap, u16 reg, u8 val)
 {
-	regmap_update_bits(priv->regmap, reg, val, val);
+	return regmap_update_bits(regmap, reg, val, val);
 }
 
-static void
-reg_clear(struct tda998x_priv *priv, u16 reg, u8 val)
+static int
+reg_clear(struct regmap *regmap, u16 reg, u8 val)
 {
-	regmap_update_bits(priv->regmap, reg, val, 0);
+	return regmap_update_bits(regmap, reg, val, 0);
 }
 
 static void
 tda998x_reset(struct tda998x_priv *priv)
 {
+	struct regmap *regmap = priv->regmap;
+
 	/* reset audio and i2c master: */
-	reg_write(priv, REG_SOFTRESET, SOFTRESET_AUDIO | SOFTRESET_I2C_MASTER);
+	regmap_write(regmap, REG_SOFTRESET,
+			SOFTRESET_AUDIO | SOFTRESET_I2C_MASTER);
 	msleep(50);
-	reg_write(priv, REG_SOFTRESET, 0);
+	regmap_write(regmap, REG_SOFTRESET, 0);
 	msleep(50);
 
 	/* reset transmitter: */
-	reg_set(priv, REG_MAIN_CNTRL0, MAIN_CNTRL0_SR);
-	reg_clear(priv, REG_MAIN_CNTRL0, MAIN_CNTRL0_SR);
+	reg_set(regmap, REG_MAIN_CNTRL0, MAIN_CNTRL0_SR);
+	reg_clear(regmap, REG_MAIN_CNTRL0, MAIN_CNTRL0_SR);
 
 	/* PLL registers common configuration */
-	reg_write(priv, REG_PLL_SERIAL_1, 0x00);
-	reg_write(priv, REG_PLL_SERIAL_2, PLL_SERIAL_2_SRL_NOSC(1));
-	reg_write(priv, REG_PLL_SERIAL_3, 0x00);
-	reg_write(priv, REG_SERIALIZER,   0x00);
-	reg_write(priv, REG_BUFFER_OUT,   0x00);
-	reg_write(priv, REG_PLL_SCG1,     0x00);
-	reg_write(priv, REG_AUDIO_DIV,    AUDIO_DIV_SERCLK_8);
-	reg_write(priv, REG_SEL_CLK,      SEL_CLK_SEL_CLK1 | SEL_CLK_ENA_SC_CLK);
-	reg_write(priv, REG_PLL_SCGN1,    0xfa);
-	reg_write(priv, REG_PLL_SCGN2,    0x00);
-	reg_write(priv, REG_PLL_SCGR1,    0x5b);
-	reg_write(priv, REG_PLL_SCGR2,    0x00);
-	reg_write(priv, REG_PLL_SCG2,     0x10);
+	regmap_write(regmap, REG_PLL_SERIAL_1, 0x00);
+	regmap_write(regmap, REG_PLL_SERIAL_2, PLL_SERIAL_2_SRL_NOSC(1));
+	regmap_write(regmap, REG_PLL_SERIAL_3, 0x00);
+	regmap_write(regmap, REG_SERIALIZER,   0x00);
+	regmap_write(regmap, REG_BUFFER_OUT,   0x00);
+	regmap_write(regmap, REG_PLL_SCG1,     0x00);
+	regmap_write(regmap, REG_AUDIO_DIV,    AUDIO_DIV_SERCLK_8);
+	regmap_write(regmap, REG_SEL_CLK,
+				SEL_CLK_SEL_CLK1 | SEL_CLK_ENA_SC_CLK);
+	regmap_write(regmap, REG_PLL_SCGN1,    0xfa);
+	regmap_write(regmap, REG_PLL_SCGN2,    0x00);
+	regmap_write(regmap, REG_PLL_SCGR1,    0x5b);
+	regmap_write(regmap, REG_PLL_SCGR2,    0x00);
+	regmap_write(regmap, REG_PLL_SCG2,     0x10);
 
 	/* Write the default value MUX register */
-	reg_write(priv, REG_MUX_VP_VIP_OUT, 0x24);
+	regmap_write(regmap, REG_MUX_VP_VIP_OUT, 0x24);
 }
 
 /*
@@ -685,16 +655,18 @@ static void tda998x_detect_work(struct work_struct *work)
 static irqreturn_t tda998x_irq_thread(int irq, void *data)
 {
 	struct tda998x_priv *priv = data;
-	u8 sta, cec, lvl, flag0, flag1, flag2;
+	struct regmap *regmap = priv->regmap;
+	u8 sta, cec, lvl;
+	unsigned int flag0, flag1, flag2;
 	bool handled = false;
 
 	sta = cec_read(priv, REG_CEC_INTSTATUS);
 	if (sta & CEC_INTSTATUS_HDMI) {
 		cec = cec_read(priv, REG_CEC_RXSHPDINT);
 		lvl = cec_read(priv, REG_CEC_RXSHPDLEV);
-		flag0 = reg_read(priv, REG_INT_FLAGS_0);
-		flag1 = reg_read(priv, REG_INT_FLAGS_1);
-		flag2 = reg_read(priv, REG_INT_FLAGS_2);
+		regmap_read(regmap, REG_INT_FLAGS_0, &flag0);
+		regmap_read(regmap, REG_INT_FLAGS_1, &flag1);
+		regmap_read(regmap, REG_INT_FLAGS_2, &flag2);
 		DRM_DEBUG_DRIVER(
 			"tda irq sta %02x cec %02x lvl %02x f0 %02x f1 %02x f2 %02x\n",
 			sta, cec, lvl, flag0, flag1, flag2);
@@ -725,6 +697,7 @@ static void
 tda998x_write_if(struct tda998x_priv *priv, u8 bit, u16 addr,
 		 union hdmi_infoframe *frame)
 {
+	struct regmap *regmap = priv->regmap;
 	u8 buf[32];
 	ssize_t len;
 
@@ -736,9 +709,9 @@ tda998x_write_if(struct tda998x_priv *priv, u8 bit, u16 addr,
 		return;
 	}
 
-	reg_clear(priv, REG_DIP_IF_FLAGS, bit);
-	reg_write_range(priv, addr, buf, len);
-	reg_set(priv, REG_DIP_IF_FLAGS, bit);
+	reg_clear(regmap, REG_DIP_IF_FLAGS, bit);
+	regmap_bulk_write(regmap, addr, buf, len);
+	reg_set(regmap, REG_DIP_IF_FLAGS, bit);
 }
 
 static int tda998x_write_aif(struct tda998x_priv *priv,
@@ -767,14 +740,14 @@ tda998x_write_avi(struct tda998x_priv *priv, const struct drm_display_mode *mode
 
 /* Audio support */
 
-static void tda998x_audio_mute(struct tda998x_priv *priv, bool on)
+static void tda998x_audio_mute(struct regmap *regmap, bool on)
 {
 	if (on) {
-		reg_set(priv, REG_SOFTRESET, SOFTRESET_AUDIO);
-		reg_clear(priv, REG_SOFTRESET, SOFTRESET_AUDIO);
-		reg_set(priv, REG_AIP_CNTRL_0, AIP_CNTRL_0_RST_FIFO);
+		reg_set(regmap, REG_SOFTRESET, SOFTRESET_AUDIO);
+		reg_clear(regmap, REG_SOFTRESET, SOFTRESET_AUDIO);
+		reg_set(regmap, REG_AIP_CNTRL_0, AIP_CNTRL_0_RST_FIFO);
 	} else {
-		reg_clear(priv, REG_AIP_CNTRL_0, AIP_CNTRL_0_RST_FIFO);
+		reg_clear(regmap, REG_AIP_CNTRL_0, AIP_CNTRL_0_RST_FIFO);
 	}
 }
 
@@ -782,25 +755,26 @@ static int
 tda998x_configure_audio(struct tda998x_priv *priv,
 			struct tda998x_audio_params *params)
 {
+	struct regmap *regmap = priv->regmap;
 	u8 buf[6], clksel_aip, clksel_fs, cts_n, adiv;
 	u32 n;
 
 	/* Enable audio ports */
-	reg_write(priv, REG_ENA_AP, params->config);
+	regmap_write(regmap, REG_ENA_AP, params->config);
 
 	/* Set audio input source */
 	switch (params->format) {
 	case AFMT_SPDIF:
-		reg_write(priv, REG_ENA_ACLK, 0);
-		reg_write(priv, REG_MUX_AP, MUX_AP_SELECT_SPDIF);
+		regmap_write(regmap, REG_ENA_ACLK, 0);
+		regmap_write(regmap, REG_MUX_AP, MUX_AP_SELECT_SPDIF);
 		clksel_aip = AIP_CLKSEL_AIP_SPDIF;
 		clksel_fs = AIP_CLKSEL_FS_FS64SPDIF;
 		cts_n = CTS_N_M(3) | CTS_N_K(3);
 		break;
 
 	case AFMT_I2S:
-		reg_write(priv, REG_ENA_ACLK, 1);
-		reg_write(priv, REG_MUX_AP, MUX_AP_SELECT_I2S);
+		regmap_write(regmap, REG_ENA_ACLK, 1);
+		regmap_write(regmap, REG_MUX_AP, MUX_AP_SELECT_I2S);
 		clksel_aip = AIP_CLKSEL_AIP_I2S;
 		clksel_fs = AIP_CLKSEL_FS_ACLK;
 		switch (params->sample_width) {
@@ -824,10 +798,10 @@ tda998x_configure_audio(struct tda998x_priv *priv,
 		return -EINVAL;
 	}
 
-	reg_write(priv, REG_AIP_CLKSEL, clksel_aip);
-	reg_clear(priv, REG_AIP_CNTRL_0, AIP_CNTRL_0_LAYOUT |
+	regmap_write(regmap, REG_AIP_CLKSEL, clksel_aip);
+	reg_clear(regmap, REG_AIP_CNTRL_0, AIP_CNTRL_0_LAYOUT |
 					AIP_CNTRL_0_ACR_MAN);	/* auto CTS */
-	reg_write(priv, REG_CTS_N, cts_n);
+	regmap_write(regmap, REG_CTS_N, cts_n);
 
 	/*
 	 * Audio input somehow depends on HDMI line rate which is
@@ -844,7 +818,7 @@ tda998x_configure_audio(struct tda998x_priv *priv,
 	if (params->format == AFMT_SPDIF)
 		adiv++;			/* AUDIO_DIV_SERCLK_16 or _32 */
 
-	reg_write(priv, REG_AUDIO_DIV, adiv);
+	regmap_write(regmap, REG_AUDIO_DIV, adiv);
 
 	/*
 	 * This is the approximate value of N, which happens to be
@@ -859,14 +833,14 @@ tda998x_configure_audio(struct tda998x_priv *priv,
 	buf[3] = n;
 	buf[4] = n >> 8;
 	buf[5] = n >> 16;
-	reg_write_range(priv, REG_ACR_CTS_0, buf, 6);
+	regmap_bulk_write(regmap, REG_ACR_CTS_0, buf, 6);
 
 	/* Set CTS clock reference */
-	reg_write(priv, REG_AIP_CLKSEL, clksel_aip | clksel_fs);
+	regmap_write(regmap, REG_AIP_CLKSEL, clksel_aip | clksel_fs);
 
 	/* Reset CTS generator */
-	reg_set(priv, REG_AIP_CNTRL_0, AIP_CNTRL_0_RST_CTS);
-	reg_clear(priv, REG_AIP_CNTRL_0, AIP_CNTRL_0_RST_CTS);
+	reg_set(regmap, REG_AIP_CNTRL_0, AIP_CNTRL_0_RST_CTS);
+	reg_clear(regmap, REG_AIP_CNTRL_0, AIP_CNTRL_0_RST_CTS);
 
 	/* Write the channel status
 	 * The REG_CH_STAT_B-registers skip IEC958 AES2 byte, because
@@ -876,11 +850,11 @@ tda998x_configure_audio(struct tda998x_priv *priv,
 	buf[1] = params->status[1];
 	buf[2] = params->status[3];
 	buf[3] = params->status[4];
-	reg_write_range(priv, REG_CH_STAT_B(0), buf, 4);
+	regmap_bulk_write(regmap, REG_CH_STAT_B(0), buf, 4);
 
-	tda998x_audio_mute(priv, true);
+	tda998x_audio_mute(regmap, true);
 	msleep(20);
-	tda998x_audio_mute(priv, false);
+	tda998x_audio_mute(regmap, false);
 
 	return tda998x_write_aif(priv, &params->cea);
 }
@@ -950,7 +924,7 @@ static void tda998x_audio_shutdown(struct device *dev, void *data)
 
 	mutex_lock(&priv->audio_mutex);
 
-	reg_write(priv, REG_ENA_AP, 0);
+	regmap_write(priv->regmap, REG_ENA_AP, 0);
 
 	priv->audio_params.format = AFMT_UNUSED;
 
@@ -963,7 +937,7 @@ int tda998x_audio_digital_mute(struct device *dev, void *data, bool enable)
 
 	mutex_lock(&priv->audio_mutex);
 
-	tda998x_audio_mute(priv, enable);
+	tda998x_audio_mute(priv->regmap, enable);
 
 	mutex_unlock(&priv->audio_mutex);
 	return 0;
@@ -1043,6 +1017,8 @@ static const struct drm_connector_funcs tda998x_connector_funcs = {
 static int read_edid_block(void *data, u8 *buf, unsigned int blk, size_t length)
 {
 	struct tda998x_priv *priv = data;
+	struct regmap *regmap = priv->regmap;
+	unsigned int flag2;
 	u8 offset, segptr;
 	int ret, i;
 
@@ -1051,17 +1027,17 @@ static int read_edid_block(void *data, u8 *buf, unsigned int blk, size_t length)
 
 	mutex_lock(&priv->edid_mutex);
 
-	reg_write(priv, REG_DDC_ADDR, 0xa0);
-	reg_write(priv, REG_DDC_OFFS, offset);
-	reg_write(priv, REG_DDC_SEGM_ADDR, 0x60);
-	reg_write(priv, REG_DDC_SEGM, segptr);
+	regmap_write(regmap, REG_DDC_ADDR, 0xa0);
+	regmap_write(regmap, REG_DDC_OFFS, offset);
+	regmap_write(regmap, REG_DDC_SEGM_ADDR, 0x60);
+	regmap_write(regmap, REG_DDC_SEGM, segptr);
 
 	/* enable reading EDID: */
 	priv->wq_edid_wait = 1;
-	reg_write(priv, REG_EDID_CTRL, 0x1);
+	regmap_write(regmap, REG_EDID_CTRL, 0x1);
 
 	/* flag must be cleared by sw: */
-	reg_write(priv, REG_EDID_CTRL, 0x0);
+	regmap_write(regmap, REG_EDID_CTRL, 0x0);
 
 	/* wait for block read to complete: */
 	if (priv->hdmi->irq) {
@@ -1076,10 +1052,10 @@ static int read_edid_block(void *data, u8 *buf, unsigned int blk, size_t length)
 	} else {
 		for (i = 100; i > 0; i--) {
 			msleep(1);
-			ret = reg_read(priv, REG_INT_FLAGS_2);
-			if (ret < 0)
+			ret = regmap_read(regmap, REG_INT_FLAGS_2, &flag2);
+			if (ret)
 				goto failed;
-			if (ret & INT_FLAGS_2_EDID_BLK_RD)
+			if (flag2 & INT_FLAGS_2_EDID_BLK_RD)
 				break;
 		}
 	}
@@ -1090,8 +1066,8 @@ static int read_edid_block(void *data, u8 *buf, unsigned int blk, size_t length)
 		goto failed;
 	}
 
-	ret = reg_read_range(priv, REG_EDID_DATA_0, buf, length);
-	if (ret != length) {
+	ret = regmap_bulk_read(regmap, REG_EDID_DATA_0, buf, length);
+	if (ret) {
 		dev_err(&priv->hdmi->dev, "failed to read edid block %d: %d\n",
 			blk, ret);
 		goto failed;
@@ -1107,6 +1083,7 @@ static int read_edid_block(void *data, u8 *buf, unsigned int blk, size_t length)
 static int tda998x_connector_get_modes(struct drm_connector *connector)
 {
 	struct tda998x_priv *priv = conn_to_tda998x_priv(connector);
+	struct regmap *regmap = priv->regmap;
 	struct edid *edid;
 	int n;
 
@@ -1119,12 +1096,12 @@ static int tda998x_connector_get_modes(struct drm_connector *connector)
 		return 0;
 
 	if (priv->rev == TDA19988)
-		reg_clear(priv, REG_TX4, TX4_PD_RAM);
+		reg_clear(regmap, REG_TX4, TX4_PD_RAM);
 
 	edid = drm_do_get_edid(connector, read_edid_block, priv);
 
 	if (priv->rev == TDA19988)
-		reg_set(priv, REG_TX4, TX4_PD_RAM);
+		reg_set(regmap, REG_TX4, TX4_PD_RAM);
 
 	if (!edid) {
 		dev_warn(&priv->hdmi->dev, "failed to read EDID\n");
@@ -1218,16 +1195,17 @@ static enum drm_mode_status tda998x_bridge_mode_valid(struct drm_bridge *bridge,
 static void tda998x_bridge_enable(struct drm_bridge *bridge)
 {
 	struct tda998x_priv *priv = bridge_to_tda998x_priv(bridge);
+	struct regmap *regmap = priv->regmap;
 
 	if (!priv->is_on) {
 		/* enable video ports, audio will be enabled later */
-		reg_write(priv, REG_ENA_VP_0, 0xff);
-		reg_write(priv, REG_ENA_VP_1, 0xff);
-		reg_write(priv, REG_ENA_VP_2, 0xff);
+		regmap_write(regmap, REG_ENA_VP_0, 0xff);
+		regmap_write(regmap, REG_ENA_VP_1, 0xff);
+		regmap_write(regmap, REG_ENA_VP_2, 0xff);
 		/* set muxing after enabling ports: */
-		reg_write(priv, REG_VIP_CNTRL_0, priv->vip_cntrl_0);
-		reg_write(priv, REG_VIP_CNTRL_1, priv->vip_cntrl_1);
-		reg_write(priv, REG_VIP_CNTRL_2, priv->vip_cntrl_2);
+		regmap_write(regmap, REG_VIP_CNTRL_0, priv->vip_cntrl_0);
+		regmap_write(regmap, REG_VIP_CNTRL_1, priv->vip_cntrl_1);
+		regmap_write(regmap, REG_VIP_CNTRL_2, priv->vip_cntrl_2);
 
 		priv->is_on = true;
 	}
@@ -1236,12 +1214,13 @@ static void tda998x_bridge_enable(struct drm_bridge *bridge)
 static void tda998x_bridge_disable(struct drm_bridge *bridge)
 {
 	struct tda998x_priv *priv = bridge_to_tda998x_priv(bridge);
+	struct regmap *regmap = priv->regmap;
 
 	if (priv->is_on) {
 		/* disable video ports */
-		reg_write(priv, REG_ENA_VP_0, 0x00);
-		reg_write(priv, REG_ENA_VP_1, 0x00);
-		reg_write(priv, REG_ENA_VP_2, 0x00);
+		regmap_write(regmap, REG_ENA_VP_0, 0x00);
+		regmap_write(regmap, REG_ENA_VP_1, 0x00);
+		regmap_write(regmap, REG_ENA_VP_2, 0x00);
 
 		priv->is_on = false;
 	}
@@ -1252,6 +1231,7 @@ static void tda998x_bridge_mode_set(struct drm_bridge *bridge,
 				    const struct drm_display_mode *adjusted_mode)
 {
 	struct tda998x_priv *priv = bridge_to_tda998x_priv(bridge);
+	struct regmap *regmap = priv->regmap;
 	unsigned long tmds_clock;
 	u16 ref_pix, ref_line, n_pix, n_line;
 	u16 hs_pix_s, hs_pix_e;
@@ -1340,43 +1320,43 @@ static void tda998x_bridge_mode_set(struct drm_bridge *bridge,
 	mutex_lock(&priv->audio_mutex);
 
 	/* mute the audio FIFO: */
-	reg_set(priv, REG_AIP_CNTRL_0, AIP_CNTRL_0_RST_FIFO);
+	reg_set(regmap, REG_AIP_CNTRL_0, AIP_CNTRL_0_RST_FIFO);
 
 	/* set HDMI HDCP mode off: */
-	reg_write(priv, REG_TBG_CNTRL_1, TBG_CNTRL_1_DWIN_DIS);
-	reg_clear(priv, REG_TX33, TX33_HDMI);
-	reg_write(priv, REG_ENC_CNTRL, ENC_CNTRL_CTL_CODE(0));
+	regmap_write(regmap, REG_TBG_CNTRL_1, TBG_CNTRL_1_DWIN_DIS);
+	reg_clear(regmap, REG_TX33, TX33_HDMI);
+	regmap_write(regmap, REG_ENC_CNTRL, ENC_CNTRL_CTL_CODE(0));
 
 	/* no pre-filter or interpolator: */
-	reg_write(priv, REG_HVF_CNTRL_0, HVF_CNTRL_0_PREFIL(0) |
+	regmap_write(regmap, REG_HVF_CNTRL_0, HVF_CNTRL_0_PREFIL(0) |
 			HVF_CNTRL_0_INTPOL(0));
-	reg_set(priv, REG_FEAT_POWERDOWN, FEAT_POWERDOWN_PREFILT);
-	reg_write(priv, REG_VIP_CNTRL_5, VIP_CNTRL_5_SP_CNT(0));
-	reg_write(priv, REG_VIP_CNTRL_4, VIP_CNTRL_4_BLANKIT(0) |
+	reg_set(regmap, REG_FEAT_POWERDOWN, FEAT_POWERDOWN_PREFILT);
+	regmap_write(regmap, REG_VIP_CNTRL_5, VIP_CNTRL_5_SP_CNT(0));
+	regmap_write(regmap, REG_VIP_CNTRL_4, VIP_CNTRL_4_BLANKIT(0) |
 			VIP_CNTRL_4_BLC(0));
 
-	reg_clear(priv, REG_PLL_SERIAL_1, PLL_SERIAL_1_SRL_MAN_IZ);
-	reg_clear(priv, REG_PLL_SERIAL_3, PLL_SERIAL_3_SRL_CCIR |
+	reg_clear(regmap, REG_PLL_SERIAL_1, PLL_SERIAL_1_SRL_MAN_IZ);
+	reg_clear(regmap, REG_PLL_SERIAL_3, PLL_SERIAL_3_SRL_CCIR |
 					  PLL_SERIAL_3_SRL_DE);
-	reg_write(priv, REG_SERIALIZER, 0);
-	reg_write(priv, REG_HVF_CNTRL_1, HVF_CNTRL_1_VQR(0));
+	regmap_write(regmap, REG_SERIALIZER, 0);
+	regmap_write(regmap, REG_HVF_CNTRL_1, HVF_CNTRL_1_VQR(0));
 
 	/* TODO enable pixel repeat for pixel rates less than 25Msamp/s */
 	rep = 0;
-	reg_write(priv, REG_RPT_CNTRL, 0);
-	reg_write(priv, REG_SEL_CLK, SEL_CLK_SEL_VRF_CLK(0) |
+	regmap_write(regmap, REG_RPT_CNTRL, 0);
+	regmap_write(regmap, REG_SEL_CLK, SEL_CLK_SEL_VRF_CLK(0) |
 			SEL_CLK_SEL_CLK1 | SEL_CLK_ENA_SC_CLK);
 
-	reg_write(priv, REG_PLL_SERIAL_2, PLL_SERIAL_2_SRL_NOSC(div) |
+	regmap_write(regmap, REG_PLL_SERIAL_2, PLL_SERIAL_2_SRL_NOSC(div) |
 			PLL_SERIAL_2_SRL_PR(rep));
 
 	/* set color matrix bypass flag: */
-	reg_write(priv, REG_MAT_CONTRL, MAT_CONTRL_MAT_BP |
+	regmap_write(regmap, REG_MAT_CONTRL, MAT_CONTRL_MAT_BP |
 				MAT_CONTRL_MAT_SC(1));
-	reg_set(priv, REG_FEAT_POWERDOWN, FEAT_POWERDOWN_CSC);
+	reg_set(regmap, REG_FEAT_POWERDOWN, FEAT_POWERDOWN_CSC);
 
 	/* set BIAS tmds value: */
-	reg_write(priv, REG_ANA_GENERAL, 0x09);
+	regmap_write(regmap, REG_ANA_GENERAL, 0x09);
 
 	/*
 	 * Sync on rising HSYNC/VSYNC
@@ -1391,33 +1371,33 @@ static void tda998x_bridge_mode_set(struct drm_bridge *bridge,
 		reg |= VIP_CNTRL_3_H_TGL;
 	if (mode->flags & DRM_MODE_FLAG_NVSYNC)
 		reg |= VIP_CNTRL_3_V_TGL;
-	reg_write(priv, REG_VIP_CNTRL_3, reg);
-
-	reg_write(priv, REG_VIDFORMAT, 0x00);
-	reg_write16(priv, REG_REFPIX_MSB, ref_pix);
-	reg_write16(priv, REG_REFLINE_MSB, ref_line);
-	reg_write16(priv, REG_NPIX_MSB, n_pix);
-	reg_write16(priv, REG_NLINE_MSB, n_line);
-	reg_write16(priv, REG_VS_LINE_STRT_1_MSB, vs1_line_s);
-	reg_write16(priv, REG_VS_PIX_STRT_1_MSB, vs1_pix_s);
-	reg_write16(priv, REG_VS_LINE_END_1_MSB, vs1_line_e);
-	reg_write16(priv, REG_VS_PIX_END_1_MSB, vs1_pix_e);
-	reg_write16(priv, REG_VS_LINE_STRT_2_MSB, vs2_line_s);
-	reg_write16(priv, REG_VS_PIX_STRT_2_MSB, vs2_pix_s);
-	reg_write16(priv, REG_VS_LINE_END_2_MSB, vs2_line_e);
-	reg_write16(priv, REG_VS_PIX_END_2_MSB, vs2_pix_e);
-	reg_write16(priv, REG_HS_PIX_START_MSB, hs_pix_s);
-	reg_write16(priv, REG_HS_PIX_STOP_MSB, hs_pix_e);
-	reg_write16(priv, REG_VWIN_START_1_MSB, vwin1_line_s);
-	reg_write16(priv, REG_VWIN_END_1_MSB, vwin1_line_e);
-	reg_write16(priv, REG_VWIN_START_2_MSB, vwin2_line_s);
-	reg_write16(priv, REG_VWIN_END_2_MSB, vwin2_line_e);
-	reg_write16(priv, REG_DE_START_MSB, de_pix_s);
-	reg_write16(priv, REG_DE_STOP_MSB, de_pix_e);
+	regmap_write(regmap, REG_VIP_CNTRL_3, reg);
+
+	regmap_write(regmap, REG_VIDFORMAT, 0x00);
+	reg_write16(regmap, REG_REFPIX_MSB, ref_pix);
+	reg_write16(regmap, REG_REFLINE_MSB, ref_line);
+	reg_write16(regmap, REG_NPIX_MSB, n_pix);
+	reg_write16(regmap, REG_NLINE_MSB, n_line);
+	reg_write16(regmap, REG_VS_LINE_STRT_1_MSB, vs1_line_s);
+	reg_write16(regmap, REG_VS_PIX_STRT_1_MSB, vs1_pix_s);
+	reg_write16(regmap, REG_VS_LINE_END_1_MSB, vs1_line_e);
+	reg_write16(regmap, REG_VS_PIX_END_1_MSB, vs1_pix_e);
+	reg_write16(regmap, REG_VS_LINE_STRT_2_MSB, vs2_line_s);
+	reg_write16(regmap, REG_VS_PIX_STRT_2_MSB, vs2_pix_s);
+	reg_write16(regmap, REG_VS_LINE_END_2_MSB, vs2_line_e);
+	reg_write16(regmap, REG_VS_PIX_END_2_MSB, vs2_pix_e);
+	reg_write16(regmap, REG_HS_PIX_START_MSB, hs_pix_s);
+	reg_write16(regmap, REG_HS_PIX_STOP_MSB, hs_pix_e);
+	reg_write16(regmap, REG_VWIN_START_1_MSB, vwin1_line_s);
+	reg_write16(regmap, REG_VWIN_END_1_MSB, vwin1_line_e);
+	reg_write16(regmap, REG_VWIN_START_2_MSB, vwin2_line_s);
+	reg_write16(regmap, REG_VWIN_END_2_MSB, vwin2_line_e);
+	reg_write16(regmap, REG_DE_START_MSB, de_pix_s);
+	reg_write16(regmap, REG_DE_STOP_MSB, de_pix_e);
 
 	if (priv->rev == TDA19988) {
 		/* let incoming pixels fill the active space (if any) */
-		reg_write(priv, REG_ENABLE_SPACE, 0x00);
+		regmap_write(regmap, REG_ENABLE_SPACE, 0x00);
 	}
 
 	/*
@@ -1429,10 +1409,10 @@ static void tda998x_bridge_mode_set(struct drm_bridge *bridge,
 		reg |= TBG_CNTRL_1_H_TGL;
 	if (mode->flags & DRM_MODE_FLAG_NVSYNC)
 		reg |= TBG_CNTRL_1_V_TGL;
-	reg_write(priv, REG_TBG_CNTRL_1, reg);
+	regmap_write(regmap, REG_TBG_CNTRL_1, reg);
 
 	/* must be last register set: */
-	reg_write(priv, REG_TBG_CNTRL_0, 0);
+	regmap_write(regmap, REG_TBG_CNTRL_0, 0);
 
 	priv->tmds_clock = adjusted_mode->clock;
 
@@ -1452,9 +1432,9 @@ static void tda998x_bridge_mode_set(struct drm_bridge *bridge,
 	if (priv->supports_infoframes) {
 		/* We need to turn HDMI HDCP stuff on to get audio through */
 		reg &= ~TBG_CNTRL_1_DWIN_DIS;
-		reg_write(priv, REG_TBG_CNTRL_1, reg);
-		reg_write(priv, REG_ENC_CNTRL, ENC_CNTRL_CTL_CODE(1));
-		reg_set(priv, REG_TX33, TX33_HDMI);
+		regmap_write(regmap, REG_TBG_CNTRL_1, reg);
+		regmap_write(regmap, REG_ENC_CNTRL, ENC_CNTRL_CTL_CODE(1));
+		reg_set(regmap, REG_TX33, TX33_HDMI);
 
 		tda998x_write_avi(priv, adjusted_mode);
 
@@ -1546,7 +1526,7 @@ static void tda998x_destroy(struct device *dev)
 
 	/* disable all IRQs and free the IRQ handler */
 	cec_write(priv, REG_CEC_RXSHPDINTENA, 0);
-	reg_clear(priv, REG_INT_FLAGS_2, INT_FLAGS_2_EDID_BLK_RD);
+	reg_clear(priv->regmap, REG_INT_FLAGS_2, INT_FLAGS_2_EDID_BLK_RD);
 
 	if (priv->audio_pdev)
 		platform_device_unregister(priv->audio_pdev);
@@ -1773,9 +1753,10 @@ static int tda998x_create(struct device *dev)
 	struct i2c_client *client = to_i2c_client(dev);
 	struct device_node *np = client->dev.of_node;
 	struct i2c_board_info cec_info;
+	unsigned int rev_lo, rev_hi, dummy;
 	struct tda998x_priv *priv;
 	u32 video;
-	int rev_lo, rev_hi, ret;
+	int ret;
 
 	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
 	if (!priv)
@@ -1811,16 +1792,16 @@ static int tda998x_create(struct device *dev)
 	tda998x_reset(priv);
 
 	/* read version: */
-	rev_lo = reg_read(priv, REG_VERSION_LSB);
-	if (rev_lo < 0) {
-		dev_err(dev, "failed to read version: %d\n", rev_lo);
-		return rev_lo;
+	ret = regmap_read(priv->regmap, REG_VERSION_LSB, &rev_lo);
+	if (ret) {
+		dev_err(dev, "failed to read version: %d\n", ret);
+		return ret;
 	}
 
-	rev_hi = reg_read(priv, REG_VERSION_MSB);
-	if (rev_hi < 0) {
-		dev_err(dev, "failed to read version: %d\n", rev_hi);
-		return rev_hi;
+	ret = regmap_read(priv->regmap, REG_VERSION_MSB, &rev_hi);
+	if (ret) {
+		dev_err(dev, "failed to read version: %d\n", ret);
+		return ret;
 	}
 
 	priv->rev = rev_lo | rev_hi << 8;
@@ -1847,14 +1828,14 @@ static int tda998x_create(struct device *dev)
 	}
 
 	/* after reset, enable DDC: */
-	reg_write(priv, REG_DDC_DISABLE, 0x00);
+	regmap_write(priv->regmap, REG_DDC_DISABLE, 0x00);
 
 	/* set clock on DDC channel: */
-	reg_write(priv, REG_TX3, 39);
+	regmap_write(priv->regmap, REG_TX3, 39);
 
 	/* if necessary, disable multi-master: */
 	if (priv->rev == TDA19989)
-		reg_set(priv, REG_I2C_MASTER, I2C_MASTER_DIS_MM);
+		reg_set(priv->regmap, REG_I2C_MASTER, I2C_MASTER_DIS_MM);
 
 	cec_write(priv, REG_CEC_FRO_IM_CLK_CTRL,
 			CEC_FRO_IM_CLK_CTRL_GHOST_DIS | CEC_FRO_IM_CLK_CTRL_IMCLK_SEL);
@@ -1864,9 +1845,9 @@ static int tda998x_create(struct device *dev)
 
 	/* clear pending interrupts */
 	cec_read(priv, REG_CEC_RXSHPDINT);
-	reg_read(priv, REG_INT_FLAGS_0);
-	reg_read(priv, REG_INT_FLAGS_1);
-	reg_read(priv, REG_INT_FLAGS_2);
+	regmap_read(priv->regmap, REG_INT_FLAGS_0, &dummy);
+	regmap_read(priv->regmap, REG_INT_FLAGS_1, &dummy);
+	regmap_read(priv->regmap, REG_INT_FLAGS_2, &dummy);
 
 	/* initialize the optional IRQ */
 	if (client->irq) {
@@ -1928,7 +1909,7 @@ static int tda998x_create(struct device *dev)
 	}
 
 	/* enable EDID read irq: */
-	reg_set(priv, REG_INT_FLAGS_2, INT_FLAGS_2_EDID_BLK_RD);
+	reg_set(priv->regmap, REG_INT_FLAGS_2, INT_FLAGS_2_EDID_BLK_RD);
 
 	if (np) {
 		/* get the device tree parameters */
-- 
2.17.1

Re: [PATCH v1 1/2] drm/i2c: tda998x: access chip registers via a regmap

[Russell King - ARM Linux admin]

On Mon, May 27, 2019 at 03:15:51PM -0400, Sven Van Asbroeck wrote:
> The tda988x i2c chip registers are accessed through a
> paged register scheme. The kernel regmap abstraction
> supports paged accesses. Replace this driver's
> dedicated i2c access functions with a standard i2c
> regmap.
> 
> Pros:
> - dedicated i2c access code disappears: accesses now
>   go through well-maintained regmap code
> - page atomicity requirements now handled by regmap
> - ro/wo/rw access modes are now explicitly defined:
>   any inappropriate register accesses (e.g. write to a
>   read-only register) will now be explicitly rejected
>   by the regmap core
> - all tda988x registers are now visible via debugfs
> 
> Cons:
> - this will probably require extensive testing

Another con is the need to keep the functions that detail the register
properties up to date, which if they're wrong may result in unexpected
behaviour.

I subscribe to the "keep it simple" approach, and regmap, although
useful, seems like a giant sledgehammer for this.

> 
> Tested on a TDA19988 using a Freescale/NXP imx6q.
> 
> Signed-off-by: Sven Van Asbroeck <TheSven73@gmail.com>
> ---
> 
> I originally hacked this together while debugging an incompatibility between
> the tda988x's audio input and the audio codec I was driving it with.
> That required me to have debug access to the chip's register values.
> A regmap did the trick, it has good debugfs support.
> 
>  drivers/gpu/drm/i2c/tda998x_drv.c | 350 ++++++++++++++++++++----------
>  1 file changed, 234 insertions(+), 116 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c b/drivers/gpu/drm/i2c/tda998x_drv.c
> index 7f34601bb515..8153e2e19e18 100644
> --- a/drivers/gpu/drm/i2c/tda998x_drv.c
> +++ b/drivers/gpu/drm/i2c/tda998x_drv.c
> @@ -21,6 +21,7 @@
>  #include <linux/module.h>
>  #include <linux/platform_data/tda9950.h>
>  #include <linux/irq.h>
> +#include <linux/regmap.h>
>  #include <sound/asoundef.h>
>  #include <sound/hdmi-codec.h>
>  
> @@ -43,10 +44,9 @@ struct tda998x_audio_port {
>  struct tda998x_priv {
>  	struct i2c_client *cec;
>  	struct i2c_client *hdmi;
> -	struct mutex mutex;
> +	struct regmap *regmap;
>  	u16 rev;
>  	u8 cec_addr;
> -	u8 current_page;
>  	bool is_on;
>  	bool supports_infoframes;
>  	bool sink_has_audio;
> @@ -88,12 +88,10 @@ struct tda998x_priv {
>  /* The TDA9988 series of devices use a paged register scheme.. to simplify
>   * things we encode the page # in upper bits of the register #.  To read/
>   * write a given register, we need to make sure CURPAGE register is set
> - * appropriately.  Which implies reads/writes are not atomic.  Fun!
> + * appropriately.
>   */
>  
>  #define REG(page, addr) (((page) << 8) | (addr))
> -#define REG2ADDR(reg)   ((reg) & 0xff)
> -#define REG2PAGE(reg)   (((reg) >> 8) & 0xff)
>  
>  #define REG_CURPAGE               0xff                /* write */
>  
> @@ -285,8 +283,9 @@ struct tda998x_priv {
>  
>  
>  /* Page 09h: EDID Control */
> +/*	EDID_DATA consists of 128 successive registers   read */
>  #define REG_EDID_DATA_0           REG(0x09, 0x00)     /* read */
> -/* next 127 successive registers are the EDID block */
> +#define REG_EDID_DATA_127         REG(0x09, 0x7f)     /* read */
>  #define REG_EDID_CTRL             REG(0x09, 0xfa)     /* read/write */
>  #define REG_DDC_ADDR              REG(0x09, 0xfb)     /* read/write */
>  #define REG_DDC_OFFS              REG(0x09, 0xfc)     /* read/write */
> @@ -295,11 +294,17 @@ struct tda998x_priv {
>  
>  
>  /* Page 10h: information frames and packets */
> +/*	REG_IF1_HB consists of 32 successive registers	 read/write */
>  #define REG_IF1_HB0               REG(0x10, 0x20)     /* read/write */
> +/*	REG_IF2_HB consists of 32 successive registers	 read/write */
>  #define REG_IF2_HB0               REG(0x10, 0x40)     /* read/write */
> +/*	REG_IF3_HB consists of 32 successive registers	 read/write */
>  #define REG_IF3_HB0               REG(0x10, 0x60)     /* read/write */
> +/*	REG_IF4_HB consists of 32 successive registers	 read/write */
>  #define REG_IF4_HB0               REG(0x10, 0x80)     /* read/write */
> +/*	REG_IF5_HB consists of 32 successive registers	 read/write */
>  #define REG_IF5_HB0               REG(0x10, 0xa0)     /* read/write */
> +#define REG_IF5_HB31              REG(0x10, 0xbf)     /* read/write */
>  
>  
>  /* Page 11h: audio settings and content info packets */
> @@ -542,93 +547,29 @@ static void tda998x_cec_hook_release(void *data)
>  	cec_enamods(priv, CEC_ENAMODS_EN_CEC_CLK | CEC_ENAMODS_EN_CEC, false);
>  }
>  
> -static int
> -set_page(struct tda998x_priv *priv, u16 reg)
> -{
> -	if (REG2PAGE(reg) != priv->current_page) {
> -		struct i2c_client *client = priv->hdmi;
> -		u8 buf[] = {
> -				REG_CURPAGE, REG2PAGE(reg)
> -		};
> -		int ret = i2c_master_send(client, buf, sizeof(buf));
> -		if (ret < 0) {
> -			dev_err(&client->dev, "%s %04x err %d\n", __func__,
> -					reg, ret);
> -			return ret;
> -		}
> -
> -		priv->current_page = REG2PAGE(reg);
> -	}
> -	return 0;
> -}
> -
>  static int
>  reg_read_range(struct tda998x_priv *priv, u16 reg, char *buf, int cnt)
>  {
> -	struct i2c_client *client = priv->hdmi;
> -	u8 addr = REG2ADDR(reg);
>  	int ret;
>  
> -	mutex_lock(&priv->mutex);
> -	ret = set_page(priv, reg);
> -	if (ret < 0)
> -		goto out;
> -
> -	ret = i2c_master_send(client, &addr, sizeof(addr));
> +	ret = regmap_bulk_read(priv->regmap, reg, buf, cnt);
>  	if (ret < 0)
> -		goto fail;
> -
> -	ret = i2c_master_recv(client, buf, cnt);
> -	if (ret < 0)
> -		goto fail;
> -
> -	goto out;
> -
> -fail:
> -	dev_err(&client->dev, "Error %d reading from 0x%x\n", ret, reg);
> -out:
> -	mutex_unlock(&priv->mutex);
> -	return ret;
> +		return ret;
> +	return cnt;
>  }
>  
> -#define MAX_WRITE_RANGE_BUF 32
> -
>  static void
>  reg_write_range(struct tda998x_priv *priv, u16 reg, u8 *p, int cnt)
>  {
> -	struct i2c_client *client = priv->hdmi;
> -	/* This is the maximum size of the buffer passed in */
> -	u8 buf[MAX_WRITE_RANGE_BUF + 1];
> -	int ret;
> -
> -	if (cnt > MAX_WRITE_RANGE_BUF) {
> -		dev_err(&client->dev, "Fixed write buffer too small (%d)\n",
> -				MAX_WRITE_RANGE_BUF);
> -		return;
> -	}
> -
> -	buf[0] = REG2ADDR(reg);
> -	memcpy(&buf[1], p, cnt);
> -
> -	mutex_lock(&priv->mutex);
> -	ret = set_page(priv, reg);
> -	if (ret < 0)
> -		goto out;
> -
> -	ret = i2c_master_send(client, buf, cnt + 1);
> -	if (ret < 0)
> -		dev_err(&client->dev, "Error %d writing to 0x%x\n", ret, reg);
> -out:
> -	mutex_unlock(&priv->mutex);
> +	regmap_bulk_write(priv->regmap, reg, p, cnt);
>  }
>  
>  static int
>  reg_read(struct tda998x_priv *priv, u16 reg)
>  {
> -	u8 val = 0;
> -	int ret;
> +	int ret, val;
>  
> -	ret = reg_read_range(priv, reg, &val, sizeof(val));
> +	ret = regmap_read(priv->regmap, reg, &val);
>  	if (ret < 0)
>  		return ret;
>  	return val;
> @@ -637,59 +578,27 @@ reg_read(struct tda998x_priv *priv, u16 reg)
>  static void
>  reg_write(struct tda998x_priv *priv, u16 reg, u8 val)
>  {
> -	struct i2c_client *client = priv->hdmi;
> -	u8 buf[] = {REG2ADDR(reg), val};
> -	int ret;
> -
> -	mutex_lock(&priv->mutex);
> -	ret = set_page(priv, reg);
> -	if (ret < 0)
> -		goto out;
> -
> -	ret = i2c_master_send(client, buf, sizeof(buf));
> -	if (ret < 0)
> -		dev_err(&client->dev, "Error %d writing to 0x%x\n", ret, reg);
> -out:
> -	mutex_unlock(&priv->mutex);
> +	regmap_write(priv->regmap, reg, val);
>  }
>  
>  static void
>  reg_write16(struct tda998x_priv *priv, u16 reg, u16 val)
>  {
> -	struct i2c_client *client = priv->hdmi;
> -	u8 buf[] = {REG2ADDR(reg), val >> 8, val};
> -	int ret;
> -
> -	mutex_lock(&priv->mutex);
> -	ret = set_page(priv, reg);
> -	if (ret < 0)
> -		goto out;
> +	u8 buf[] = {val >> 8, val};
>  
> -	ret = i2c_master_send(client, buf, sizeof(buf));
> -	if (ret < 0)
> -		dev_err(&client->dev, "Error %d writing to 0x%x\n", ret, reg);
> -out:
> -	mutex_unlock(&priv->mutex);
> +	regmap_bulk_write(priv->regmap, reg, buf, ARRAY_SIZE(buf));
>  }
>  
>  static void
>  reg_set(struct tda998x_priv *priv, u16 reg, u8 val)
>  {
> -	int old_val;
> -
> -	old_val = reg_read(priv, reg);
> -	if (old_val >= 0)
> -		reg_write(priv, reg, old_val | val);
> +	regmap_update_bits(priv->regmap, reg, val, val);
>  }
>  
>  static void
>  reg_clear(struct tda998x_priv *priv, u16 reg, u8 val)
>  {
> -	int old_val;
> -
> -	old_val = reg_read(priv, reg);
> -	if (old_val >= 0)
> -		reg_write(priv, reg, old_val & ~val);
> +	regmap_update_bits(priv->regmap, reg, val, 0);
>  }
>  
>  static void
> @@ -816,7 +725,7 @@ static void
>  tda998x_write_if(struct tda998x_priv *priv, u8 bit, u16 addr,
>  		 union hdmi_infoframe *frame)
>  {
> -	u8 buf[MAX_WRITE_RANGE_BUF];
> +	u8 buf[32];
>  	ssize_t len;
>  
>  	len = hdmi_infoframe_pack(frame, buf, sizeof(buf));
> @@ -1654,6 +1563,211 @@ static void tda998x_destroy(struct device *dev)
>  		cec_notifier_put(priv->cec_notify);
>  }
>  
> +static bool tda_is_edid_data_reg(unsigned int reg)
> +{
> +	return (reg >= REG_EDID_DATA_0) &&
> +		(reg <= REG_EDID_DATA_127);
> +}
> +
> +static bool tda_is_precious_volatile_reg(struct device *dev, unsigned int reg)
> +{
> +	if (tda_is_edid_data_reg(reg))
> +		return true;
> +	switch (reg) {
> +	case REG_INT_FLAGS_0:
> +	case REG_INT_FLAGS_1:
> +	case REG_INT_FLAGS_2:
> +		return true;
> +	default:
> +		return false;
> +	}
> +}
> +
> +static bool tda_is_rw_reg(unsigned int reg)
> +{
> +	if ((reg >= REG_IF1_HB0) && (reg <= REG_IF5_HB31))
> +		return true;
> +	switch (reg) {
> +	case REG_MAIN_CNTRL0:
> +	case REG_DDC_DISABLE:
> +	case REG_CCLK_ON:
> +	case REG_I2C_MASTER:
> +	case REG_FEAT_POWERDOWN:
> +	case REG_INT_FLAGS_0:
> +	case REG_INT_FLAGS_1:
> +	case REG_INT_FLAGS_2:
> +	case REG_ENA_ACLK:
> +	case REG_ENA_VP_0:
> +	case REG_ENA_VP_1:
> +	case REG_ENA_VP_2:
> +	case REG_ENA_AP:
> +	case REG_MUX_AP:
> +	case REG_MUX_VP_VIP_OUT:
> +	case REG_I2S_FORMAT:
> +	case REG_PLL_SERIAL_1:
> +	case REG_PLL_SERIAL_2:
> +	case REG_PLL_SERIAL_3:
> +	case REG_SERIALIZER:
> +	case REG_BUFFER_OUT:
> +	case REG_PLL_SCG1:
> +	case REG_PLL_SCG2:
> +	case REG_PLL_SCGN1:
> +	case REG_PLL_SCGN2:
> +	case REG_PLL_SCGR1:
> +	case REG_PLL_SCGR2:
> +	case REG_AUDIO_DIV:
> +	case REG_SEL_CLK:
> +	case REG_ANA_GENERAL:
> +	case REG_EDID_CTRL:
> +	case REG_DDC_ADDR:
> +	case REG_DDC_OFFS:
> +	case REG_DDC_SEGM_ADDR:
> +	case REG_DDC_SEGM:
> +	case REG_AIP_CNTRL_0:
> +	case REG_CA_I2S:
> +	case REG_LATENCY_RD:
> +	case REG_ACR_CTS_0:
> +	case REG_ACR_CTS_1:
> +	case REG_ACR_CTS_2:
> +	case REG_ACR_N_0:
> +	case REG_ACR_N_1:
> +	case REG_ACR_N_2:
> +	case REG_CTS_N:
> +	case REG_ENC_CNTRL:
> +	case REG_DIP_FLAGS:
> +	case REG_DIP_IF_FLAGS:
> +	case REG_TX3:
> +	case REG_TX4:
> +	case REG_TX33:
> +	case REG_CH_STAT_B(0):
> +	case REG_CH_STAT_B(1):
> +	case REG_CH_STAT_B(2):
> +	case REG_CH_STAT_B(3):
> +		return true;
> +	default:
> +		return false;
> +	}
> +}
> +
> +static bool tda_is_readable_reg(struct device *dev, unsigned int reg)
> +{
> +	if (tda_is_rw_reg(reg) || tda_is_edid_data_reg(reg))
> +		return true;
> +	switch (reg) {
> +	case REG_VERSION_LSB:
> +	case REG_VERSION_MSB:
> +		return true;
> +	default:
> +		return false;
> +	}
> +}
> +
> +static bool tda_is_writeable_reg(struct device *dev, unsigned int reg)
> +{
> +	if (tda_is_rw_reg(reg))
> +		return true;
> +	switch (reg) {
> +	case REG_CURPAGE:
> +	case REG_SOFTRESET:
> +	case REG_VIP_CNTRL_0:
> +	case REG_VIP_CNTRL_1:
> +	case REG_VIP_CNTRL_2:
> +	case REG_VIP_CNTRL_3:
> +	case REG_VIP_CNTRL_4:
> +	case REG_VIP_CNTRL_5:
> +	case REG_MAT_CONTRL:
> +	case REG_VIDFORMAT:
> +	case REG_REFPIX_MSB:
> +	case REG_REFPIX_LSB:
> +	case REG_REFLINE_MSB:
> +	case REG_REFLINE_LSB:
> +	case REG_NPIX_MSB:
> +	case REG_NPIX_LSB:
> +	case REG_NLINE_MSB:
> +	case REG_NLINE_LSB:
> +	case REG_VS_LINE_STRT_1_MSB:
> +	case REG_VS_LINE_STRT_1_LSB:
> +	case REG_VS_PIX_STRT_1_MSB:
> +	case REG_VS_PIX_STRT_1_LSB:
> +	case REG_VS_LINE_END_1_MSB:
> +	case REG_VS_LINE_END_1_LSB:
> +	case REG_VS_PIX_END_1_MSB:
> +	case REG_VS_PIX_END_1_LSB:
> +	case REG_VS_LINE_STRT_2_MSB:
> +	case REG_VS_LINE_STRT_2_LSB:
> +	case REG_VS_PIX_STRT_2_MSB:
> +	case REG_VS_PIX_STRT_2_LSB:
> +	case REG_VS_LINE_END_2_MSB:
> +	case REG_VS_LINE_END_2_LSB:
> +	case REG_VS_PIX_END_2_MSB:
> +	case REG_VS_PIX_END_2_LSB:
> +	case REG_HS_PIX_START_MSB:
> +	case REG_HS_PIX_START_LSB:
> +	case REG_HS_PIX_STOP_MSB:
> +	case REG_HS_PIX_STOP_LSB:
> +	case REG_VWIN_START_1_MSB:
> +	case REG_VWIN_START_1_LSB:
> +	case REG_VWIN_END_1_MSB:
> +	case REG_VWIN_END_1_LSB:
> +	case REG_VWIN_START_2_MSB:
> +	case REG_VWIN_START_2_LSB:
> +	case REG_VWIN_END_2_MSB:
> +	case REG_VWIN_END_2_LSB:
> +	case REG_DE_START_MSB:
> +	case REG_DE_START_LSB:
> +	case REG_DE_STOP_MSB:
> +	case REG_DE_STOP_LSB:
> +	case REG_TBG_CNTRL_0:
> +	case REG_TBG_CNTRL_1:
> +	case REG_ENABLE_SPACE:
> +	case REG_HVF_CNTRL_0:
> +	case REG_HVF_CNTRL_1:
> +	case REG_RPT_CNTRL:
> +	case REG_AIP_CLKSEL:
> +		return true;
> +	default:
> +		return false;
> +	}
> +}
> +
> +static const struct reg_default tda_reg_default[] = {
> +	{ REG_CURPAGE, 0xff },
> +};
> +
> +static const struct regmap_range_cfg hdmi_range_cfg[] = {
> +	{
> +		.range_min = 0x00,
> +		.range_max = REG_TX33,
> +		.selector_reg = REG_CURPAGE,
> +		.selector_mask = 0xff,
> +		.selector_shift = 0,
> +		.window_start = 0,
> +		.window_len = 0x100,
> +	},
> +};
> +
> +/* Paged register scheme, with a write-only page register (CURPAGE).
> + * Make this work by marking CURPAGE write-only and cacheable. Give it
> + * an invalid page default value, which guarantees that it will get written to
> + * the first time we read/write the registers.
> + */
> +
> +static const struct regmap_config hdmi_regmap_config = {
> +	.reg_bits = 8,
> +	.val_bits = 8,
> +	.ranges = hdmi_range_cfg,
> +	.num_ranges = ARRAY_SIZE(hdmi_range_cfg),
> +	.max_register = REG_TX33,
> +
> +	.cache_type = REGCACHE_RBTREE,
> +	.volatile_reg = tda_is_precious_volatile_reg,
> +	.precious_reg = tda_is_precious_volatile_reg,
> +	.readable_reg = tda_is_readable_reg,
> +	.writeable_reg = tda_is_writeable_reg,
> +	.reg_defaults = tda_reg_default,
> +	.num_reg_defaults = ARRAY_SIZE(tda_reg_default),
> +};
> +
>  static int tda998x_create(struct device *dev)
>  {
>  	struct i2c_client *client = to_i2c_client(dev);
> @@ -1666,10 +1780,15 @@ static int tda998x_create(struct device *dev)
>  	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
>  	if (!priv)
>  		return -ENOMEM;
> +	priv->regmap = devm_regmap_init_i2c(client, &hdmi_regmap_config);
> +	if (IS_ERR(priv->regmap)) {
> +		ret = PTR_ERR(priv->regmap);
> +		dev_err(dev, "Failed to allocate register map: %d\n", ret);
> +		return ret;
> +	}
>  
>  	dev_set_drvdata(dev, priv);
>  
> -	mutex_init(&priv->mutex);	/* protect the page access */
>  	mutex_init(&priv->audio_mutex); /* protect access from audio thread */
>  	mutex_init(&priv->edid_mutex);
>  	INIT_LIST_HEAD(&priv->bridge.list);
> @@ -1683,7 +1802,6 @@ static int tda998x_create(struct device *dev)
>  
>  	/* CEC I2C address bound to TDA998x I2C addr by configuration pins */
>  	priv->cec_addr = 0x34 + (client->addr & 0x03);
> -	priv->current_page = 0xff;
>  	priv->hdmi = client;
>  
>  	/* wake up the device: */
> -- 
> 2.17.1
> 
> 

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up

Re: [PATCH v1 2/2] drm/i2c: tda998x: remove indirect reg_read/_write() calls

[Russell King - ARM Linux admin]

On Mon, May 27, 2019 at 03:15:52PM -0400, Sven Van Asbroeck wrote:
> -static void
> -reg_set(struct tda998x_priv *priv, u16 reg, u8 val)
> +static int
> +reg_set(struct regmap *regmap, u16 reg, u8 val)

I don't see the point of making this return an 'int' - you don't modify
any of the callsites to check the returned value, so returning a value
is not useful.

>  {
> -	regmap_update_bits(priv->regmap, reg, val, val);
> +	return regmap_update_bits(regmap, reg, val, val);
>  }
>  
> -static void
> -reg_clear(struct tda998x_priv *priv, u16 reg, u8 val)
> +static int
> +reg_clear(struct regmap *regmap, u16 reg, u8 val)

Same here.

> @@ -685,16 +655,18 @@ static void tda998x_detect_work(struct work_struct *work)
>  static irqreturn_t tda998x_irq_thread(int irq, void *data)
>  {
>  	struct tda998x_priv *priv = data;
> -	u8 sta, cec, lvl, flag0, flag1, flag2;
> +	struct regmap *regmap = priv->regmap;
> +	u8 sta, cec, lvl;
> +	unsigned int flag0, flag1, flag2;
>  	bool handled = false;
>  
>  	sta = cec_read(priv, REG_CEC_INTSTATUS);
>  	if (sta & CEC_INTSTATUS_HDMI) {
>  		cec = cec_read(priv, REG_CEC_RXSHPDINT);
>  		lvl = cec_read(priv, REG_CEC_RXSHPDLEV);
> -		flag0 = reg_read(priv, REG_INT_FLAGS_0);
> -		flag1 = reg_read(priv, REG_INT_FLAGS_1);
> -		flag2 = reg_read(priv, REG_INT_FLAGS_2);
> +		regmap_read(regmap, REG_INT_FLAGS_0, &flag0);
> +		regmap_read(regmap, REG_INT_FLAGS_1, &flag1);
> +		regmap_read(regmap, REG_INT_FLAGS_2, &flag2);

Not particularly enamoured by this...

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up

Re: [PATCH v1 1/2] drm/i2c: tda998x: access chip registers via a regmap

[Sven Van Asbroeck]

On Fri, Jun 21, 2019 at 11:15 AM Russell King - ARM Linux admin
<linux@armlinux.org.uk> wrote:
>
> Another con is the need to keep the functions that detail the register
> properties up to date, which if they're wrong may result in unexpected
> behaviour.
>
> I subscribe to the "keep it simple" approach, and regmap, although
> useful, seems like a giant sledgehammer for this.
>

Thank you for the review !

I added this back when I was debugging audio artifacts related to this
chip. The regmap's debugfs binding was extremely useful. So I
dressed it up a bit in the hope that it would have some general use.

But if the cons outweigh the pros, then this is as far as this patch
will go...

Re: [PATCH v1 1/2] drm/i2c: tda998x: access chip registers via a regmap

[Russell King - ARM Linux admin]

On Fri, Jun 21, 2019 at 05:13:33PM -0400, Sven Van Asbroeck wrote:
> On Fri, Jun 21, 2019 at 11:15 AM Russell King - ARM Linux admin
> <linux@armlinux.org.uk> wrote:
> >
> > Another con is the need to keep the functions that detail the register
> > properties up to date, which if they're wrong may result in unexpected
> > behaviour.
> >
> > I subscribe to the "keep it simple" approach, and regmap, although
> > useful, seems like a giant sledgehammer for this.
> >
> 
> Thank you for the review !
> 
> I added this back when I was debugging audio artifacts related to this
> chip. The regmap's debugfs binding was extremely useful. So I
> dressed it up a bit in the hope that it would have some general use.
> 
> But if the cons outweigh the pros, then this is as far as this patch
> will go...

I won't disagree that debugfs access to the registers is useful,
especially as I keep this patch locally for that exact purpose:

8<===
From: Russell King <rmk+kernel@armlinux.org.uk>
Subject: [PATCH] drm/i2c: tda998x: debugfs register access

Add support for dumping the register contents via debugfs, with a
mechanism to write to the TDA998x registers to allow experimentation.

Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
---
 drivers/gpu/drm/i2c/tda998x_drv.c | 114 ++++++++++++++++++++++++++++++++++++++
 1 file changed, 114 insertions(+)

diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c b/drivers/gpu/drm/i2c/tda998x_drv.c
index 5312fde8b624..6ad1fd1d28a5 100644
--- a/drivers/gpu/drm/i2c/tda998x_drv.c
+++ b/drivers/gpu/drm/i2c/tda998x_drv.c
@@ -16,10 +16,13 @@
  */
 
 #include <linux/component.h>
+#include <linux/ctype.h>
+#include <linux/debugfs.h>
 #include <linux/gpio/consumer.h>
 #include <linux/hdmi.h>
 #include <linux/module.h>
 #include <linux/platform_data/tda9950.h>
+#include <linux/seq_file.h>
 #include <linux/irq.h>
 #include <sound/asoundef.h>
 #include <sound/hdmi-codec.h>
@@ -1239,6 +1242,116 @@ static int tda998x_audio_codec_init(struct tda998x_priv *priv,
 }
 
 /* DRM connector functions */
+static u8 tda998x_debugfs_pages[] = {
+	0x00, 0x01, 0x02, 0x09, 0x10, 0x11, 0x12, 0x13
+};
+
+static void *tda998x_regs_start(struct seq_file *s, loff_t *pos)
+{
+	return *pos < ARRAY_SIZE(tda998x_debugfs_pages) ? pos : NULL;
+}
+
+static void *tda998x_regs_next(struct seq_file *s, void *v, loff_t *pos)
+{
+	(*pos)++;
+	return *pos < ARRAY_SIZE(tda998x_debugfs_pages) ? pos : NULL;
+}
+
+static void tda998x_regs_stop(struct seq_file *s, void *v)
+{
+}
+
+static int tda998x_regs_show(struct seq_file *s, void *v)
+{
+	struct tda998x_priv *priv = s->private;
+	u8 page = tda998x_debugfs_pages[*(loff_t *)v];
+	unsigned int i, j;
+	u8 buf[16];
+
+	seq_printf(s, "==================== page %02x ======================\n", page);
+	seq_printf(s, "     0  1  2  3  4  5  6  7  8  9  a  b  c  d  e  f\n");
+	for (i = 0; i < 256; i += sizeof(buf)) {
+		reg_read_range(priv, REG(page, i), buf, sizeof(buf));
+
+		seq_printf(s, "%02x:", i);
+		for (j = 0; j < sizeof(buf); j++)
+			seq_printf(s, " %02x", buf[j]);
+		seq_printf(s, " : ");
+		for (j = 0; j < sizeof(buf); j++)
+			seq_printf(s, "%c",
+				   isascii(buf[j]) && isprint(buf[j]) ?
+				   buf[j] : '.');
+		seq_putc(s, '\n');
+	}
+	return 0;
+}
+
+static const struct seq_operations tda998x_regs_sops = {
+	.start	= tda998x_regs_start,
+	.next	= tda998x_regs_next,
+	.stop	= tda998x_regs_stop,
+	.show	= tda998x_regs_show,
+};
+
+static int tda998x_regs_open(struct inode *inode, struct file *file)
+{
+	int ret = seq_open(file, &tda998x_regs_sops);
+	if (ret < 0)
+		return ret;
+
+	((struct seq_file *)file->private_data)->private = inode->i_private;
+
+	return 0;
+}
+
+static ssize_t tda998x_regs_write(struct file *file, const char __user *buf,
+				  size_t count, loff_t *offset)
+{
+	struct tda998x_priv *priv =
+		((struct seq_file *)file->private_data)->private;
+	unsigned int page, addr, mask, val;
+	unsigned char rval;
+	char buffer[16];
+
+	memset(buffer, 0, sizeof(buffer));
+	if (count > sizeof(buffer) - 1)
+		count = sizeof(buffer) - 1;
+	if (copy_from_user(buffer, buf, count))
+		return -EFAULT;
+
+	if (sscanf(buffer, "%x %x %x %x", &page, &addr, &mask, &val) != 4)
+		return -EINVAL;
+	if (page > 0xff || addr > 0xff || mask > 0xff || val > 0xff)
+		return -ERANGE;
+
+	rval = reg_read(priv, REG(page, addr));
+	rval &= ~mask;
+	rval |= val & mask;
+	reg_write(priv, REG(page, addr), rval);
+
+	printk("i2c write %02x @ page:%02x address:%02x\n", rval, page, addr);
+
+	return count;
+}
+
+static const struct file_operations tda998x_regs_fops = {
+	.owner = THIS_MODULE,
+	.open = tda998x_regs_open,
+	.read = seq_read,
+	.write = tda998x_regs_write,
+	.llseek = seq_lseek,
+	.release = seq_release,
+};
+
+static int tda998x_late_register(struct drm_connector *connector)
+{
+	struct tda998x_priv *priv = conn_to_tda998x_priv(connector);
+
+	debugfs_create_file("tda998x-regs", 0600, connector->debugfs_entry,
+			    priv, &tda998x_regs_fops);
+
+	return 0;
+}
 
 static enum drm_connector_status
 tda998x_connector_detect(struct drm_connector *connector, bool force)
@@ -1259,6 +1372,7 @@ static const struct drm_connector_funcs tda998x_connector_funcs = {
 	.reset = drm_atomic_helper_connector_reset,
 	.fill_modes = drm_helper_probe_single_connector_modes,
 	.detect = tda998x_connector_detect,
+	.late_register = tda998x_late_register,
 	.destroy = tda998x_connector_destroy,
 	.atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state,
 	.atomic_destroy_state = drm_atomic_helper_connector_destroy_state,
-- 
2.7.4


-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up

Re: [PATCH v1 1/2] drm/i2c: tda998x: access chip registers via a regmap

[Russell King - ARM Linux admin]

On Fri, Jun 21, 2019 at 05:13:33PM -0400, Sven Van Asbroeck wrote:
> On Fri, Jun 21, 2019 at 11:15 AM Russell King - ARM Linux admin
> <linux@armlinux.org.uk> wrote:
> >
> > Another con is the need to keep the functions that detail the register
> > properties up to date, which if they're wrong may result in unexpected
> > behaviour.
> >
> > I subscribe to the "keep it simple" approach, and regmap, although
> > useful, seems like a giant sledgehammer for this.
> >
> 
> Thank you for the review !
> 
> I added this back when I was debugging audio artifacts related to this
> chip. The regmap's debugfs binding was extremely useful. So I
> dressed it up a bit in the hope that it would have some general use.
> 
> But if the cons outweigh the pros, then this is as far as this patch
> will go...

I won't disagree that debugfs access to the registers is useful,
especially as I keep this patch locally for that exact purpose:

8<===
From: Russell King <rmk+kernel@armlinux.org.uk>
Subject: [PATCH] drm/i2c: tda998x: debugfs register access

Add support for dumping the register contents via debugfs, with a
mechanism to write to the TDA998x registers to allow experimentation.

Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
---
 drivers/gpu/drm/i2c/tda998x_drv.c | 114 ++++++++++++++++++++++++++++++++++++++
 1 file changed, 114 insertions(+)

diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c b/drivers/gpu/drm/i2c/tda998x_drv.c
index 5312fde8b624..6ad1fd1d28a5 100644
--- a/drivers/gpu/drm/i2c/tda998x_drv.c
+++ b/drivers/gpu/drm/i2c/tda998x_drv.c
@@ -16,10 +16,13 @@
  */
 
 #include <linux/component.h>
+#include <linux/ctype.h>
+#include <linux/debugfs.h>
 #include <linux/gpio/consumer.h>
 #include <linux/hdmi.h>
 #include <linux/module.h>
 #include <linux/platform_data/tda9950.h>
+#include <linux/seq_file.h>
 #include <linux/irq.h>
 #include <sound/asoundef.h>
 #include <sound/hdmi-codec.h>
@@ -1239,6 +1242,116 @@ static int tda998x_audio_codec_init(struct tda998x_priv *priv,
 }
 
 /* DRM connector functions */
+static u8 tda998x_debugfs_pages[] = {
+	0x00, 0x01, 0x02, 0x09, 0x10, 0x11, 0x12, 0x13
+};
+
+static void *tda998x_regs_start(struct seq_file *s, loff_t *pos)
+{
+	return *pos < ARRAY_SIZE(tda998x_debugfs_pages) ? pos : NULL;
+}
+
+static void *tda998x_regs_next(struct seq_file *s, void *v, loff_t *pos)
+{
+	(*pos)++;
+	return *pos < ARRAY_SIZE(tda998x_debugfs_pages) ? pos : NULL;
+}
+
+static void tda998x_regs_stop(struct seq_file *s, void *v)
+{
+}
+
+static int tda998x_regs_show(struct seq_file *s, void *v)
+{
+	struct tda998x_priv *priv = s->private;
+	u8 page = tda998x_debugfs_pages[*(loff_t *)v];
+	unsigned int i, j;
+	u8 buf[16];
+
+	seq_printf(s, "==================== page %02x ======================\n", page);
+	seq_printf(s, "     0  1  2  3  4  5  6  7  8  9  a  b  c  d  e  f\n");
+	for (i = 0; i < 256; i += sizeof(buf)) {
+		reg_read_range(priv, REG(page, i), buf, sizeof(buf));
+
+		seq_printf(s, "%02x:", i);
+		for (j = 0; j < sizeof(buf); j++)
+			seq_printf(s, " %02x", buf[j]);
+		seq_printf(s, " : ");
+		for (j = 0; j < sizeof(buf); j++)
+			seq_printf(s, "%c",
+				   isascii(buf[j]) && isprint(buf[j]) ?
+				   buf[j] : '.');
+		seq_putc(s, '\n');
+	}
+	return 0;
+}
+
+static const struct seq_operations tda998x_regs_sops = {
+	.start	= tda998x_regs_start,
+	.next	= tda998x_regs_next,
+	.stop	= tda998x_regs_stop,
+	.show	= tda998x_regs_show,
+};
+
+static int tda998x_regs_open(struct inode *inode, struct file *file)
+{
+	int ret = seq_open(file, &tda998x_regs_sops);
+	if (ret < 0)
+		return ret;
+
+	((struct seq_file *)file->private_data)->private = inode->i_private;
+
+	return 0;
+}
+
+static ssize_t tda998x_regs_write(struct file *file, const char __user *buf,
+				  size_t count, loff_t *offset)
+{
+	struct tda998x_priv *priv =
+		((struct seq_file *)file->private_data)->private;
+	unsigned int page, addr, mask, val;
+	unsigned char rval;
+	char buffer[16];
+
+	memset(buffer, 0, sizeof(buffer));
+	if (count > sizeof(buffer) - 1)
+		count = sizeof(buffer) - 1;
+	if (copy_from_user(buffer, buf, count))
+		return -EFAULT;
+
+	if (sscanf(buffer, "%x %x %x %x", &page, &addr, &mask, &val) != 4)
+		return -EINVAL;
+	if (page > 0xff || addr > 0xff || mask > 0xff || val > 0xff)
+		return -ERANGE;
+
+	rval = reg_read(priv, REG(page, addr));
+	rval &= ~mask;
+	rval |= val & mask;
+	reg_write(priv, REG(page, addr), rval);
+
+	printk("i2c write %02x @ page:%02x address:%02x\n", rval, page, addr);
+
+	return count;
+}
+
+static const struct file_operations tda998x_regs_fops = {
+	.owner = THIS_MODULE,
+	.open = tda998x_regs_open,
+	.read = seq_read,
+	.write = tda998x_regs_write,
+	.llseek = seq_lseek,
+	.release = seq_release,
+};
+
+static int tda998x_late_register(struct drm_connector *connector)
+{
+	struct tda998x_priv *priv = conn_to_tda998x_priv(connector);
+
+	debugfs_create_file("tda998x-regs", 0600, connector->debugfs_entry,
+			    priv, &tda998x_regs_fops);
+
+	return 0;
+}
 
 static enum drm_connector_status
 tda998x_connector_detect(struct drm_connector *connector, bool force)
@@ -1259,6 +1372,7 @@ static const struct drm_connector_funcs tda998x_connector_funcs = {
 	.reset = drm_atomic_helper_connector_reset,
 	.fill_modes = drm_helper_probe_single_connector_modes,
 	.detect = tda998x_connector_detect,
+	.late_register = tda998x_late_register,
 	.destroy = tda998x_connector_destroy,
 	.atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state,
 	.atomic_destroy_state = drm_atomic_helper_connector_destroy_state,
-- 
2.7.4


-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel