2018-02-07 17:37:59

by Kieran Bingham

Subject: [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(-)

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

2018-02-09 15:40:19

by Niklas Söderlund

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

Hi Kieran,

Thanks for your patch.

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

2018-02-09 15:44:42

by Kieran Bingham

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

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 <[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(-)
>>
>> 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
>>
>