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

2018-02-09 Thread 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 +, Kieran Bingham wrote:
>> From: Kieran Bingham 
>>
>> 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 
>> ---
>>  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

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

2018-02-09 Thread Niklas Söderlund 
Hi Kieran,

Thanks for your patch.

On 2018-02-07 17:34:45 +, Kieran Bingham wrote:
> From: Kieran Bingham 
> 
> 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 
> ---
>  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