* [PATCH 0/2] media: i2c: adv748x: Fix CBUS page issue
@ 2018-02-07 17:34 Kieran Bingham
2018-02-07 17:34 ` [PATCH 1/2] media: i2c: adv748x: Simplify regmap configuration Kieran Bingham
2018-02-07 17:34 ` [PATCH 2/2] media: i2c: adv748x: Add missing CBUS page Kieran Bingham
0 siblings, 2 replies; 8+ messages in thread
From: Kieran Bingham @ 2018-02-07 17:34 UTC (permalink / raw)
To: Kieran Bingham, linux-media, linux-renesas-soc; +Cc: Kieran Bingham
From: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
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(-)
--
2.7.4
^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH 1/2] media: i2c: adv748x: Simplify regmap configuration
2018-02-07 17:34 [PATCH 0/2] media: i2c: adv748x: Fix CBUS page issue Kieran Bingham
@ 2018-02-07 17:34 ` Kieran Bingham
2018-02-09 15:39 ` Niklas Söderlund
2018-02-07 17:34 ` [PATCH 2/2] media: i2c: adv748x: Add missing CBUS page Kieran Bingham
1 sibling, 1 reply; 8+ messages in thread
From: Kieran Bingham @ 2018-02-07 17:34 UTC (permalink / raw)
To: Kieran Bingham, linux-media, linux-renesas-soc
Cc: Kieran Bingham, Mauro Carvalho Chehab, open list
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
^ permalink raw reply related [flat|nested] 8+ messages in thread
* [PATCH 2/2] media: i2c: adv748x: Add missing CBUS page.
2018-02-07 17:34 [PATCH 0/2] media: i2c: adv748x: Fix CBUS page issue Kieran Bingham
2018-02-07 17:34 ` [PATCH 1/2] media: i2c: adv748x: Simplify regmap configuration Kieran Bingham
@ 2018-02-07 17:34 ` Kieran Bingham
2018-02-09 15:41 ` Niklas Söderlund
1 sibling, 1 reply; 8+ messages in thread
From: Kieran Bingham @ 2018-02-07 17:34 UTC (permalink / raw)
To: Kieran Bingham, linux-media, linux-renesas-soc
Cc: Kieran Bingham, Mauro Carvalho Chehab, open list
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
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH 1/2] media: i2c: adv748x: Simplify regmap configuration
2018-02-07 17:34 ` [PATCH 1/2] media: i2c: adv748x: Simplify regmap configuration Kieran Bingham
@ 2018-02-09 15:39 ` Niklas Söderlund
0 siblings, 0 replies; 8+ messages in thread
From: Niklas Söderlund @ 2018-02-09 15:39 UTC (permalink / raw)
To: Kieran Bingham
Cc: Kieran Bingham, linux-media, linux-renesas-soc, Kieran Bingham,
Mauro Carvalho Chehab, open list
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
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 1/2] media: i2c: adv748x: Simplify regmap configuration
@ 2018-02-09 15:39 ` Niklas Söderlund
0 siblings, 0 replies; 8+ messages in thread
From: Niklas Söderlund @ 2018-02-09 15:39 UTC (permalink / raw)
To: Kieran Bingham
Cc: Kieran Bingham, linux-media, linux-renesas-soc, Kieran Bingham,
Mauro Carvalho Chehab, open list
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
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 2/2] media: i2c: adv748x: Add missing CBUS page.
2018-02-07 17:34 ` [PATCH 2/2] media: i2c: adv748x: Add missing CBUS page Kieran Bingham
@ 2018-02-09 15:41 ` Niklas Söderlund
0 siblings, 0 replies; 8+ messages in thread
From: Niklas Söderlund @ 2018-02-09 15:41 UTC (permalink / raw)
To: Kieran Bingham
Cc: Kieran Bingham, linux-media, linux-renesas-soc, Kieran Bingham,
Mauro Carvalho Chehab, open list
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
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 2/2] media: i2c: adv748x: Add missing CBUS page.
@ 2018-02-09 15:41 ` Niklas Söderlund
0 siblings, 0 replies; 8+ messages in thread
From: Niklas Söderlund @ 2018-02-09 15:41 UTC (permalink / raw)
To: Kieran Bingham
Cc: Kieran Bingham, linux-media, linux-renesas-soc, Kieran Bingham,
Mauro Carvalho Chehab, open list
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
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 1/2] media: i2c: adv748x: Simplify regmap configuration
2018-02-09 15:39 ` Niklas Söderlund
(?)
@ 2018-02-09 15:43 ` Kieran Bingham
-1 siblings, 0 replies; 8+ messages in thread
From: Kieran Bingham @ 2018-02-09 15:43 UTC (permalink / raw)
To: Niklas Söderlund
Cc: Kieran Bingham, linux-media, linux-renesas-soc, Kieran Bingham,
Mauro Carvalho Chehab, open list
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
>>
>
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2018-02-09 15:43 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-02-07 17:34 [PATCH 0/2] media: i2c: adv748x: Fix CBUS page issue Kieran Bingham
2018-02-07 17:34 ` [PATCH 1/2] media: i2c: adv748x: Simplify regmap configuration Kieran Bingham
2018-02-09 15:39 ` Niklas Söderlund
2018-02-09 15:39 ` Niklas Söderlund
2018-02-09 15:43 ` Kieran Bingham
2018-02-07 17:34 ` [PATCH 2/2] media: i2c: adv748x: Add missing CBUS page Kieran Bingham
2018-02-09 15:41 ` Niklas Söderlund
2018-02-09 15:41 ` Niklas Söderlund
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.