Skip to contentSkip to conversation

media: i2c: adv748x: Fix CBUS page issue

PATCH In review Kieran Bingham 3 participants 6 messages Feb 7, 2018 – Feb 9, 2018

Subsystems (2)
Media (V4L/DVB) Analog devices inc ADV748X driver
lore
Share & tools
lore

Kieran Bingham

author
#0 lore raw

cover media: i2c: adv748x: Fix CBUS page issue

From: Kieran Bingham <[email protected]>

The ADV748x has 12 pages mapped on to I2C addresses.

The existing implementation only has 11 of these in the map enumeration, and this can cause an off-by-one error when programming the map addresses.

This short series simplifies the regmap configuration tables, and adds the missing CBUS page to better model the device, and remove the off by one error.

Kieran Bingham (2):
media: i2c: adv748x: Simplify regmap configuration
media: i2c: adv748x: Add missing CBUS page.

 drivers/media/i2c/adv748x/adv748x-core.c | 114 +++++++------------------------
 drivers/media/i2c/adv748x/adv748x.h      |   2 +
 2 files changed, 27 insertions(+), 89 deletions(-)
Signature · 1 line
2.7.4
author
#1 lore raw

PATCH 1/2 media: i2c: adv748x: Simplify regmap configuration

From: Kieran Bingham <[email protected]>

The ADV748x has identical map configurations for each register map. The duplication of each map can be simplified using a helper macro such that each map is represented on a single line.

Define ADV748X_REGMAP_CONF for this purpose and un-define after it's use.

Signed-off-by Kieran Bingham <[email protected]>
---
 drivers/media/i2c/adv748x/adv748x-core.c | 111 ++++++-------------------------
 1 file changed, 22 insertions(+), 89 deletions(-)
drivers/media/i2c/adv748x/adv748x-core.c +22 -89
index fd92c9e4b519..71c69b816db2 100644
@@ -35,98 +35,31 @@
  * Register manipulation
  */
 
-static const struct regmap_config adv748x_regmap_cnf[] = {
-	{
-		.name			= "io",
-		.reg_bits		= 8,
-		.val_bits		= 8,
-
-		.max_register		= 0xff,
-		.cache_type		= REGCACHE_NONE,
-	},
-	{
-		.name			= "dpll",
-		.reg_bits		= 8,
-		.val_bits		= 8,
-
-		.max_register		= 0xff,
-		.cache_type		= REGCACHE_NONE,
-	},
-	{
-		.name			= "cp",
-		.reg_bits		= 8,
-		.val_bits		= 8,
-
-		.max_register		= 0xff,
-		.cache_type		= REGCACHE_NONE,
-	},
-	{
-		.name			= "hdmi",
-		.reg_bits		= 8,
-		.val_bits		= 8,
-
-		.max_register		= 0xff,
-		.cache_type		= REGCACHE_NONE,
-	},
-	{
-		.name			= "edid",
-		.reg_bits		= 8,
-		.val_bits		= 8,
-
-		.max_register		= 0xff,
-		.cache_type		= REGCACHE_NONE,
-	},
-	{
-		.name			= "repeater",
-		.reg_bits		= 8,
-		.val_bits		= 8,
-
-		.max_register		= 0xff,
-		.cache_type		= REGCACHE_NONE,
-	},
-	{
-		.name			= "infoframe",
-		.reg_bits		= 8,
-		.val_bits		= 8,
-
-		.max_register		= 0xff,
-		.cache_type		= REGCACHE_NONE,
-	},
-	{
-		.name			= "cec",
-		.reg_bits		= 8,
-		.val_bits		= 8,
-
-		.max_register		= 0xff,
-		.cache_type		= REGCACHE_NONE,
-	},
-	{
-		.name			= "sdp",
-		.reg_bits		= 8,
-		.val_bits		= 8,
-
-		.max_register		= 0xff,
-		.cache_type		= REGCACHE_NONE,
-	},
-
-	{
-		.name			= "txb",
-		.reg_bits		= 8,
-		.val_bits		= 8,
-
-		.max_register		= 0xff,
-		.cache_type		= REGCACHE_NONE,
-	},
-	{
-		.name			= "txa",
-		.reg_bits		= 8,
-		.val_bits		= 8,
+#define ADV748X_REGMAP_CONF(n) \
+{ \
+	.name = n, \
+	.reg_bits = 8, \
+	.val_bits = 8, \
+	.max_register = 0xff, \
+	.cache_type = REGCACHE_NONE, \
+}
 
-		.max_register		= 0xff,
-		.cache_type		= REGCACHE_NONE,
-	},
+static const struct regmap_config adv748x_regmap_cnf[] = {
+	ADV748X_REGMAP_CONF("io"),
+	ADV748X_REGMAP_CONF("dpll"),
+	ADV748X_REGMAP_CONF("cp"),
+	ADV748X_REGMAP_CONF("hdmi"),
+	ADV748X_REGMAP_CONF("edid"),
+	ADV748X_REGMAP_CONF("repeater"),
+	ADV748X_REGMAP_CONF("infoframe"),
+	ADV748X_REGMAP_CONF("cec"),
+	ADV748X_REGMAP_CONF("sdp"),
+	ADV748X_REGMAP_CONF("txa"),
+	ADV748X_REGMAP_CONF("txb"),
 };
 
+#undef ADV748X_REGMAP_CONF
+
 static int adv748x_configure_regmap(struct adv748x_state *state, int region)
 {
 	int err;
Signature · 1 line
2.7.4
#2 lore raw

Re: [PATCH 1/2] media: i2c: adv748x: Simplify regmap configuration

Hi Kieran,

Thanks for your patch.

Quoted 126 lines · Kieran Bingham

From: Kieran Bingham <[email protected]>

The ADV748x has identical map configurations for each register map. The duplication of each map can be simplified using a helper macro such that each map is represented on a single line.

Define ADV748X_REGMAP_CONF for this purpose and un-define after it's use.

Signed-off-by Kieran Bingham <[email protected]>
---
 drivers/media/i2c/adv748x/adv748x-core.c | 111 ++++++-------------------------
 1 file changed, 22 insertions(+), 89 deletions(-)
drivers/media/i2c/adv748x/adv748x-core.c +14 -89
index fd92c9e4b519..71c69b816db2 100644
@@ -35,98 +35,31 @@
  * Register manipulation
  */
 
-static const struct regmap_config adv748x_regmap_cnf[] = {
-	{
-		.name			= "io",
-		.reg_bits		= 8,
-		.val_bits		= 8,
-
-		.max_register		= 0xff,
-		.cache_type		= REGCACHE_NONE,
-	},
-	{
-		.name			= "dpll",
-		.reg_bits		= 8,
-		.val_bits		= 8,
-
-		.max_register		= 0xff,
-		.cache_type		= REGCACHE_NONE,
-	},
-	{
-		.name			= "cp",
-		.reg_bits		= 8,
-		.val_bits		= 8,
-
-		.max_register		= 0xff,
-		.cache_type		= REGCACHE_NONE,
-	},
-	{
-		.name			= "hdmi",
-		.reg_bits		= 8,
-		.val_bits		= 8,
-
-		.max_register		= 0xff,
-		.cache_type		= REGCACHE_NONE,
-	},
-	{
-		.name			= "edid",
-		.reg_bits		= 8,
-		.val_bits		= 8,
-
-		.max_register		= 0xff,
-		.cache_type		= REGCACHE_NONE,
-	},
-	{
-		.name			= "repeater",
-		.reg_bits		= 8,
-		.val_bits		= 8,
-
-		.max_register		= 0xff,
-		.cache_type		= REGCACHE_NONE,
-	},
-	{
-		.name			= "infoframe",
-		.reg_bits		= 8,
-		.val_bits		= 8,
-
-		.max_register		= 0xff,
-		.cache_type		= REGCACHE_NONE,
-	},
-	{
-		.name			= "cec",
-		.reg_bits		= 8,
-		.val_bits		= 8,
-
-		.max_register		= 0xff,
-		.cache_type		= REGCACHE_NONE,
-	},
-	{
-		.name			= "sdp",
-		.reg_bits		= 8,
-		.val_bits		= 8,
-
-		.max_register		= 0xff,
-		.cache_type		= REGCACHE_NONE,
-	},
-
-	{
-		.name			= "txb",
-		.reg_bits		= 8,
-		.val_bits		= 8,
-
-		.max_register		= 0xff,
-		.cache_type		= REGCACHE_NONE,
-	},
-	{
-		.name			= "txa",
-		.reg_bits		= 8,
-		.val_bits		= 8,
+#define ADV748X_REGMAP_CONF(n) \
+{ \
+	.name = n, \
+	.reg_bits = 8, \
+	.val_bits = 8, \
+	.max_register = 0xff, \
+	.cache_type = REGCACHE_NONE, \
+}
 
-		.max_register		= 0xff,
-		.cache_type		= REGCACHE_NONE,
-	},
+static const struct regmap_config adv748x_regmap_cnf[] = {
+	ADV748X_REGMAP_CONF("io"),
+	ADV748X_REGMAP_CONF("dpll"),
+	ADV748X_REGMAP_CONF("cp"),
+	ADV748X_REGMAP_CONF("hdmi"),
+	ADV748X_REGMAP_CONF("edid"),

drivers/media/i2c/adv748x/adv748x-core.c line 62

Quoted 10 lines
drivers/media/i2c/adv748x/adv748x-core.c +8 -0
+	ADV748X_REGMAP_CONF("repeater"),
+	ADV748X_REGMAP_CONF("infoframe"),
+	ADV748X_REGMAP_CONF("cec"),
+	ADV748X_REGMAP_CONF("sdp"),
+	ADV748X_REGMAP_CONF("txa"),
+	ADV748X_REGMAP_CONF("txb"),
 };
 
+#undef ADV748X_REGMAP_CONF
+

Why is this macro undefined here? It have a rather limited scope as it's only local to this C file and it have a good prefix of ADV748X_ so conflicts are highly unlikely. Is there something I'm missing?

Is it really customary to undefine helper macros like this once they are used to populate the structure?

Quoted 6 lines
drivers/media/i2c/adv748x/adv748x-core.c +0 -0
 static int adv748x_configure_regmap(struct adv748x_state *state, int region)
 {
 	int err;
Signature · 1 line
2.7.4
Signature · 2 lines
Regards,
Niklas S�derlund
maintainer
#3 lore raw

Re: [PATCH 1/2] media: i2c: adv748x: Simplify regmap configuration

Hi Niklas,

Quoted 146 lines · Niklas Söderlund

Hi Kieran,

Thanks for your patch.

Quoted 126 lines · Kieran Bingham

From: Kieran Bingham <[email protected]>

The ADV748x has identical map configurations for each register map. The duplication of each map can be simplified using a helper macro such that each map is represented on a single line.

Define ADV748X_REGMAP_CONF for this purpose and un-define after it's use.

Signed-off-by Kieran Bingham <[email protected]>
---
 drivers/media/i2c/adv748x/adv748x-core.c | 111 ++++++-------------------------
 1 file changed, 22 insertions(+), 89 deletions(-)
drivers/media/i2c/adv748x/adv748x-core.c +14 -89
index fd92c9e4b519..71c69b816db2 100644
@@ -35,98 +35,31 @@
  * Register manipulation
  */
 
-static const struct regmap_config adv748x_regmap_cnf[] = {
-	{
-		.name			= "io",
-		.reg_bits		= 8,
-		.val_bits		= 8,
-
-		.max_register		= 0xff,
-		.cache_type		= REGCACHE_NONE,
-	},
-	{
-		.name			= "dpll",
-		.reg_bits		= 8,
-		.val_bits		= 8,
-
-		.max_register		= 0xff,
-		.cache_type		= REGCACHE_NONE,
-	},
-	{
-		.name			= "cp",
-		.reg_bits		= 8,
-		.val_bits		= 8,
-
-		.max_register		= 0xff,
-		.cache_type		= REGCACHE_NONE,
-	},
-	{
-		.name			= "hdmi",
-		.reg_bits		= 8,
-		.val_bits		= 8,
-
-		.max_register		= 0xff,
-		.cache_type		= REGCACHE_NONE,
-	},
-	{
-		.name			= "edid",
-		.reg_bits		= 8,
-		.val_bits		= 8,
-
-		.max_register		= 0xff,
-		.cache_type		= REGCACHE_NONE,
-	},
-	{
-		.name			= "repeater",
-		.reg_bits		= 8,
-		.val_bits		= 8,
-
-		.max_register		= 0xff,
-		.cache_type		= REGCACHE_NONE,
-	},
-	{
-		.name			= "infoframe",
-		.reg_bits		= 8,
-		.val_bits		= 8,
-
-		.max_register		= 0xff,
-		.cache_type		= REGCACHE_NONE,
-	},
-	{
-		.name			= "cec",
-		.reg_bits		= 8,
-		.val_bits		= 8,
-
-		.max_register		= 0xff,
-		.cache_type		= REGCACHE_NONE,
-	},
-	{
-		.name			= "sdp",
-		.reg_bits		= 8,
-		.val_bits		= 8,
-
-		.max_register		= 0xff,
-		.cache_type		= REGCACHE_NONE,
-	},
-
-	{
-		.name			= "txb",
-		.reg_bits		= 8,
-		.val_bits		= 8,
-
-		.max_register		= 0xff,
-		.cache_type		= REGCACHE_NONE,
-	},
-	{
-		.name			= "txa",
-		.reg_bits		= 8,
-		.val_bits		= 8,
+#define ADV748X_REGMAP_CONF(n) \
+{ \
+	.name = n, \
+	.reg_bits = 8, \
+	.val_bits = 8, \
+	.max_register = 0xff, \
+	.cache_type = REGCACHE_NONE, \
+}
 
-		.max_register		= 0xff,
-		.cache_type		= REGCACHE_NONE,
-	},
+static const struct regmap_config adv748x_regmap_cnf[] = {
+	ADV748X_REGMAP_CONF("io"),
+	ADV748X_REGMAP_CONF("dpll"),
+	ADV748X_REGMAP_CONF("cp"),
+	ADV748X_REGMAP_CONF("hdmi"),
+	ADV748X_REGMAP_CONF("edid"),

drivers/media/i2c/adv748x/adv748x-core.c line 62

Quoted 10 lines
drivers/media/i2c/adv748x/adv748x-core.c +8 -0
+	ADV748X_REGMAP_CONF("repeater"),
+	ADV748X_REGMAP_CONF("infoframe"),
+	ADV748X_REGMAP_CONF("cec"),
+	ADV748X_REGMAP_CONF("sdp"),
+	ADV748X_REGMAP_CONF("txa"),
+	ADV748X_REGMAP_CONF("txb"),
 };
 
+#undef ADV748X_REGMAP_CONF
+

Why is this macro undefined here? It have a rather limited scope as it's only local to this C file and it have a good prefix of ADV748X_ so conflicts are highly unlikely. Is there something I'm missing?

Quoted 2 lines

Is it really customary to undefine helper macros like this once they are used to populate the structure?

You are right - this is probably unnecessary, as the scope is limited to this file. I guess I was trying to further highlight that the scope of the macro is limited to this specific table as well.

Certainly likely to be required if this was a header, but it's not. So I'm happy to remove...

Quoted 8 lines
Quoted 7 lines
drivers/media/i2c/adv748x/adv748x-core.c +0 -0
 static int adv748x_configure_regmap(struct adv748x_state *state, int region)
 {
 	int err;
Signature · 1 line
2.7.4
author
#4 lore raw

PATCH 2/2 media: i2c: adv748x: Add missing CBUS page.

In reply to Kieran Bingham

From: Kieran Bingham <[email protected]>

The ADV748x has 12 pages mapped onto I2C addresses.

In the existing implementation only 11 are mapped correctly in the page enumerations, which causes an off-by-one fault on pages above the infoframe definition due to a missing 'CBUS' page.

This causes the address for the CEC, SDP, TXA, and TXB to be incorrectly programmed during the iterations in adv748x_initialise_clients().

Until now this has gone un-noticed due to the fact that following the creation of the clients - the device is reset and the addresses are reprogrammed in manually by the call to "adv748x_write_regs(state, adv748x_set_slave_address);"

As part of moving to dynamic i2c address allocations repair this by providing the missing CBUS page definition.

Signed-off-by Kieran Bingham <[email protected]>
---
 drivers/media/i2c/adv748x/adv748x-core.c | 3 +++
 drivers/media/i2c/adv748x/adv748x.h      | 2 ++
 2 files changed, 5 insertions(+)
drivers/media/i2c/adv748x/adv748x-core.c +3 -0
index 71c69b816db2..6d62b817ed00 100644
@@ -52,6 +52,7 @@ static const struct regmap_config adv748x_regmap_cnf[] = {
 	ADV748X_REGMAP_CONF("edid"),
 	ADV748X_REGMAP_CONF("repeater"),
 	ADV748X_REGMAP_CONF("infoframe"),
+	ADV748X_REGMAP_CONF("cbus"),
 	ADV748X_REGMAP_CONF("cec"),
 	ADV748X_REGMAP_CONF("sdp"),
 	ADV748X_REGMAP_CONF("txa"),

@@ -91,6 +92,7 @@ static int adv748x_i2c_addresses[ADV748X_PAGE_MAX] = {
 	ADV748X_I2C_EDID,
 	ADV748X_I2C_REPEATER,
 	ADV748X_I2C_INFOFRAME,
+	ADV748X_I2C_CBUS,
 	ADV748X_I2C_CEC,
 	ADV748X_I2C_SDP,
 	ADV748X_I2C_TXB,

@@ -354,6 +356,7 @@ static const struct adv748x_reg_value adv748x_set_slave_address[] = {
 	{ADV748X_PAGE_IO, 0xf6, ADV748X_I2C_EDID << 1},
 	{ADV748X_PAGE_IO, 0xf7, ADV748X_I2C_REPEATER << 1},
 	{ADV748X_PAGE_IO, 0xf8, ADV748X_I2C_INFOFRAME << 1},
+	{ADV748X_PAGE_IO, 0xf9, ADV748X_I2C_CBUS << 1},
 	{ADV748X_PAGE_IO, 0xfa, ADV748X_I2C_CEC << 1},
 	{ADV748X_PAGE_IO, 0xfb, ADV748X_I2C_SDP << 1},
 	{ADV748X_PAGE_IO, 0xfc, ADV748X_I2C_TXB << 1},
drivers/media/i2c/adv748x/adv748x.h +2 -0
index 6789e2f3bc8c..725662edc4b8 100644
@@ -35,6 +35,7 @@
 #define ADV748X_I2C_EDID		0x36	/* EDID Map */
 #define ADV748X_I2C_REPEATER		0x32	/* HDMI RX Repeater Map */
 #define ADV748X_I2C_INFOFRAME		0x31	/* HDMI RX InfoFrame Map */
+#define ADV748X_I2C_CBUS		0x30	/* CBUS MHL Map */
 #define ADV748X_I2C_CEC			0x41	/* CEC Map */
 #define ADV748X_I2C_SDP			0x79	/* SDP Map */
 #define ADV748X_I2C_TXB			0x48	/* CSI-TXB Map */

@@ -48,6 +49,7 @@ enum adv748x_page {
 	ADV748X_PAGE_EDID,
 	ADV748X_PAGE_REPEATER,
 	ADV748X_PAGE_INFOFRAME,
+	ADV748X_PAGE_CBUS,
 	ADV748X_PAGE_CEC,
 	ADV748X_PAGE_SDP,
 	ADV748X_PAGE_TXB,
Signature · 1 line
2.7.4
Reviewed-by on v1 2/2
#5 lore raw

Re: [PATCH 2/2] media: i2c: adv748x: Add missing CBUS page.

Hi Kieran,

Thanks for your patch,

Quoted 16 lines · Kieran Bingham

From: Kieran Bingham <[email protected]>

The ADV748x has 12 pages mapped onto I2C addresses.

In the existing implementation only 11 are mapped correctly in the page enumerations, which causes an off-by-one fault on pages above the infoframe definition due to a missing 'CBUS' page.

This causes the address for the CEC, SDP, TXA, and TXB to be incorrectly programmed during the iterations in adv748x_initialise_clients().

Until now this has gone un-noticed due to the fact that following the creation of the clients - the device is reset and the addresses are reprogrammed in manually by the call to "adv748x_write_regs(state, adv748x_set_slave_address);"

Quoted 4 lines

As part of moving to dynamic i2c address allocations repair this by providing the missing CBUS page definition.

Signed-off-by Kieran Bingham <[email protected]>

I would drop the '.' from the subject line, with that fixed:

Reviewed-by Niklas Söderlund <[email protected]>
Quoted 56 lines
---
 drivers/media/i2c/adv748x/adv748x-core.c | 3 +++
 drivers/media/i2c/adv748x/adv748x.h      | 2 ++
 2 files changed, 5 insertions(+)
drivers/media/i2c/adv748x/adv748x-core.c +3 -0
index 71c69b816db2..6d62b817ed00 100644
@@ -52,6 +52,7 @@ static const struct regmap_config adv748x_regmap_cnf[] = {
 	ADV748X_REGMAP_CONF("edid"),
 	ADV748X_REGMAP_CONF("repeater"),
 	ADV748X_REGMAP_CONF("infoframe"),
+	ADV748X_REGMAP_CONF("cbus"),
 	ADV748X_REGMAP_CONF("cec"),
 	ADV748X_REGMAP_CONF("sdp"),
 	ADV748X_REGMAP_CONF("txa"),

@@ -91,6 +92,7 @@ static int adv748x_i2c_addresses[ADV748X_PAGE_MAX] = {
 	ADV748X_I2C_EDID,
 	ADV748X_I2C_REPEATER,
 	ADV748X_I2C_INFOFRAME,
+	ADV748X_I2C_CBUS,
 	ADV748X_I2C_CEC,
 	ADV748X_I2C_SDP,
 	ADV748X_I2C_TXB,

@@ -354,6 +356,7 @@ static const struct adv748x_reg_value adv748x_set_slave_address[] = {
 	{ADV748X_PAGE_IO, 0xf6, ADV748X_I2C_EDID << 1},
 	{ADV748X_PAGE_IO, 0xf7, ADV748X_I2C_REPEATER << 1},
 	{ADV748X_PAGE_IO, 0xf8, ADV748X_I2C_INFOFRAME << 1},
+	{ADV748X_PAGE_IO, 0xf9, ADV748X_I2C_CBUS << 1},
 	{ADV748X_PAGE_IO, 0xfa, ADV748X_I2C_CEC << 1},
 	{ADV748X_PAGE_IO, 0xfb, ADV748X_I2C_SDP << 1},
 	{ADV748X_PAGE_IO, 0xfc, ADV748X_I2C_TXB << 1},
drivers/media/i2c/adv748x/adv748x.h +2 -0
index 6789e2f3bc8c..725662edc4b8 100644
@@ -35,6 +35,7 @@
 #define ADV748X_I2C_EDID		0x36	/* EDID Map */
 #define ADV748X_I2C_REPEATER		0x32	/* HDMI RX Repeater Map */
 #define ADV748X_I2C_INFOFRAME		0x31	/* HDMI RX InfoFrame Map */
+#define ADV748X_I2C_CBUS		0x30	/* CBUS MHL Map */
 #define ADV748X_I2C_CEC			0x41	/* CEC Map */
 #define ADV748X_I2C_SDP			0x79	/* SDP Map */
 #define ADV748X_I2C_TXB			0x48	/* CSI-TXB Map */

@@ -48,6 +49,7 @@ enum adv748x_page {
 	ADV748X_PAGE_EDID,
 	ADV748X_PAGE_REPEATER,
 	ADV748X_PAGE_INFOFRAME,
+	ADV748X_PAGE_CBUS,
 	ADV748X_PAGE_CEC,
 	ADV748X_PAGE_SDP,
 	ADV748X_PAGE_TXB,
Signature · 1 line
2.7.4
Signature · 2 lines
Regards,
Niklas Söderlund