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

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

[Kieran Bingham]

From: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>

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 <kieran.bingham+renesas@ideasonboard.com>
---
 drivers/media/i2c/adv748x/adv748x-core.c | 111 ++++++-------------------------
 1 file changed, 22 insertions(+), 89 deletions(-)

diff --git a/drivers/media/i2c/adv748x/adv748x-core.c b/drivers/media/i2c/adv748x/adv748x-core.c
index fd92c9e4b519..71c69b816db2 100644
--- a/drivers/media/i2c/adv748x/adv748x-core.c
+++ b/drivers/media/i2c/adv748x/adv748x-core.c
@@ -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;
-- 
2.7.4

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

[Kieran Bingham]

From: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>

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 <kieran.bingham+renesas@ideasonboard.com>
---
 drivers/media/i2c/adv748x/adv748x-core.c | 3 +++
 drivers/media/i2c/adv748x/adv748x.h      | 2 ++
 2 files changed, 5 insertions(+)

diff --git a/drivers/media/i2c/adv748x/adv748x-core.c b/drivers/media/i2c/adv748x/adv748x-core.c
index 71c69b816db2..6d62b817ed00 100644
--- a/drivers/media/i2c/adv748x/adv748x-core.c
+++ b/drivers/media/i2c/adv748x/adv748x-core.c
@@ -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},
diff --git a/drivers/media/i2c/adv748x/adv748x.h b/drivers/media/i2c/adv748x/adv748x.h
index 6789e2f3bc8c..725662edc4b8 100644
--- a/drivers/media/i2c/adv748x/adv748x.h
+++ b/drivers/media/i2c/adv748x/adv748x.h
@@ -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,
-- 
2.7.4

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

[Niklas Söderlund]

Hi Kieran,

Thanks for your patch.

On 2018-02-07 17:34:45 +0000, Kieran Bingham wrote:
> From: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
> 
> 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 <kieran.bingham+renesas@ideasonboard.com>
> ---
>  drivers/media/i2c/adv748x/adv748x-core.c | 111 ++++++-------------------------
>  1 file changed, 22 insertions(+), 89 deletions(-)
> 
> diff --git a/drivers/media/i2c/adv748x/adv748x-core.c b/drivers/media/i2c/adv748x/adv748x-core.c
> index fd92c9e4b519..71c69b816db2 100644
> --- a/drivers/media/i2c/adv748x/adv748x-core.c
> +++ b/drivers/media/i2c/adv748x/adv748x-core.c
> @@ -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
> +

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?

>  static int adv748x_configure_regmap(struct adv748x_state *state, int region)
>  {
>  	int err;
> -- 
> 2.7.4
> 

-- 
Regards,
Niklas Söderlund

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

[Niklas Söderlund]

Hi Kieran,

Thanks for your patch,

On 2018-02-07 17:34:46 +0000, Kieran Bingham wrote:
> From: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
> 
> 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 <kieran.bingham+renesas@ideasonboard.com>

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

Reviewed-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>

> ---
>  drivers/media/i2c/adv748x/adv748x-core.c | 3 +++
>  drivers/media/i2c/adv748x/adv748x.h      | 2 ++
>  2 files changed, 5 insertions(+)
> 
> diff --git a/drivers/media/i2c/adv748x/adv748x-core.c b/drivers/media/i2c/adv748x/adv748x-core.c
> index 71c69b816db2..6d62b817ed00 100644
> --- a/drivers/media/i2c/adv748x/adv748x-core.c
> +++ b/drivers/media/i2c/adv748x/adv748x-core.c
> @@ -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},
> diff --git a/drivers/media/i2c/adv748x/adv748x.h b/drivers/media/i2c/adv748x/adv748x.h
> index 6789e2f3bc8c..725662edc4b8 100644
> --- a/drivers/media/i2c/adv748x/adv748x.h
> +++ b/drivers/media/i2c/adv748x/adv748x.h
> @@ -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,
> -- 
> 2.7.4
> 

-- 
Regards,
Niklas Söderlund

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

[Kieran Bingham]

Hi Niklas,

On 09/02/18 15:39, Niklas Söderlund wrote:
> Hi Kieran,
> 
> Thanks for your patch.
> 
> On 2018-02-07 17:34:45 +0000, Kieran Bingham wrote:
>> From: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
>>
>> 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 <kieran.bingham+renesas@ideasonboard.com>
>> ---
>>  drivers/media/i2c/adv748x/adv748x-core.c | 111 ++++++-------------------------
>>  1 file changed, 22 insertions(+), 89 deletions(-)
>>
>> diff --git a/drivers/media/i2c/adv748x/adv748x-core.c b/drivers/media/i2c/adv748x/adv748x-core.c
>> index fd92c9e4b519..71c69b816db2 100644
>> --- a/drivers/media/i2c/adv748x/adv748x-core.c
>> +++ b/drivers/media/i2c/adv748x/adv748x-core.c
>> @@ -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
>> +
> 
> 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?

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...

> 
>>  static int adv748x_configure_regmap(struct adv748x_state *state, int region)
>>  {
>>  	int err;
>> -- 
>> 2.7.4
>>
>