Re: [PATCH v2 3/3] media: i2c: adv748x: Add support for i2c_new_secondary_device

2018-03-01 Thread Niklas Söderlund 
Hi Kieran,

I like this change :-)

On 2018-02-27 15:05:50 +, Kieran Bingham wrote:
> From: Kieran Bingham 
> 
> The ADV748x has twelve 256-byte maps that can be accessed via the main
> I2C ports. Each map has it own I2C address and acts as a standard slave
> device on the I2C bus.
> 
> Allow a device tree node to override the default addresses so that
> address conflicts with other devices on the same bus may be resolved at
> the board description level.
> 
> Signed-off-by: Kieran Bingham 

Reviewed-by: Niklas Söderlund 

> ---
>  drivers/media/i2c/adv748x/adv748x-core.c | 77 
> +++-
>  drivers/media/i2c/adv748x/adv748x.h  | 14 --
>  2 files changed, 36 insertions(+), 55 deletions(-)
> 
> diff --git a/drivers/media/i2c/adv748x/adv748x-core.c 
> b/drivers/media/i2c/adv748x/adv748x-core.c
> index 9dcaadaca217..911ccf6eb68c 100644
> --- a/drivers/media/i2c/adv748x/adv748x-core.c
> +++ b/drivers/media/i2c/adv748x/adv748x-core.c
> @@ -80,21 +80,24 @@ static int adv748x_configure_regmap(struct adv748x_state 
> *state, int region)
>  
>   return 0;
>  }
> +struct adv748x_register_map {
> + const char *name;
> + u8 default_addr;
> +};
>  
> -/* Default addresses for the I2C pages */
> -static int adv748x_i2c_addresses[ADV748X_PAGE_MAX] = {
> - ADV748X_I2C_IO,
> - ADV748X_I2C_DPLL,
> - ADV748X_I2C_CP,
> - ADV748X_I2C_HDMI,
> - ADV748X_I2C_EDID,
> - ADV748X_I2C_REPEATER,
> - ADV748X_I2C_INFOFRAME,
> - ADV748X_I2C_CBUS,
> - ADV748X_I2C_CEC,
> - ADV748X_I2C_SDP,
> - ADV748X_I2C_TXB,
> - ADV748X_I2C_TXA,
> +static const struct adv748x_register_map adv748x_default_addresses[] = {
> + [ADV748X_PAGE_IO] = { "main", 0x70 },
> + [ADV748X_PAGE_DPLL] = { "dpll", 0x26 },
> + [ADV748X_PAGE_CP] = { "cp", 0x22 },
> + [ADV748X_PAGE_HDMI] = { "hdmi", 0x34 },
> + [ADV748X_PAGE_EDID] = { "edid", 0x36 },
> + [ADV748X_PAGE_REPEATER] = { "repeater", 0x32 },
> + [ADV748X_PAGE_INFOFRAME] = { "infoframe", 0x31 },
> + [ADV748X_PAGE_CBUS] = { "cbus", 0x30 },
> + [ADV748X_PAGE_CEC] = { "cec", 0x41 },
> + [ADV748X_PAGE_SDP] = { "sdp", 0x79 },
> + [ADV748X_PAGE_TXB] = { "txb", 0x48 },
> + [ADV748X_PAGE_TXA] = { "txa", 0x4a },
>  };
>  
>  static int adv748x_read_check(struct adv748x_state *state,
> @@ -143,15 +146,20 @@ int adv748x_write_block(struct adv748x_state *state, 
> int client_page,
>   return regmap_raw_write(regmap, init_reg, val, val_len);
>  }
>  
> -static struct i2c_client *adv748x_dummy_client(struct adv748x_state *state,
> -u8 addr, u8 io_reg)
> +static int adv748x_set_slave_addresses(struct adv748x_state *state)
>  {
> - struct i2c_client *client = state->client;
> + struct i2c_client *client;
> + unsigned int i;
> + u8 io_reg;
> +
> + for (i = ADV748X_PAGE_DPLL; i < ADV748X_PAGE_MAX; ++i) {
> + io_reg = ADV748X_IO_SLAVE_ADDR_BASE + i;
> + client = state->i2c_clients[i];
>  
> - if (addr)
> - io_write(state, io_reg, addr << 1);
> + io_write(state, io_reg, client->addr << 1);
> + }
>  
> - return i2c_new_dummy(client->adapter, io_read(state, io_reg) >> 1);
> + return 0;
>  }
>  
>  static void adv748x_unregister_clients(struct adv748x_state *state)
> @@ -164,13 +172,15 @@ static void adv748x_unregister_clients(struct 
> adv748x_state *state)
>  
>  static int adv748x_initialise_clients(struct adv748x_state *state)
>  {
> - int i;
> + unsigned int i;
>   int ret;
>  
>   for (i = ADV748X_PAGE_DPLL; i < ADV748X_PAGE_MAX; ++i) {
> - state->i2c_clients[i] =
> - adv748x_dummy_client(state, adv748x_i2c_addresses[i],
> -  ADV748X_IO_SLAVE_ADDR_BASE + i);
> + state->i2c_clients[i] = i2c_new_secondary_device(
> + state->client,
> + adv748x_default_addresses[i].name,
> + adv748x_default_addresses[i].default_addr);
> +
>   if (state->i2c_clients[i] == NULL) {
>   adv_err(state, "failed to create i2c client %u\n", i);
>   return -ENOMEM;
> @@ -181,7 +191,7 @@ static int adv748x_initialise_clients(struct 
> adv748x_state *state)
>   return ret;
>   }
>  
> - return 0;
> + return adv748x_set_slave_addresses(state);
>  }
>  
>  /**
> @@ -347,21 +357,6 @@ static const struct adv748x_reg_value adv748x_sw_reset[] 
> = {
>   {ADV748X_PAGE_EOR, 0xff, 0xff}  /* End of register table */
>  };
>  
> -static const struct adv748x_reg_value adv748x_set_slave_address[] = {
> - {ADV748X_PAGE_IO, 0xf3, ADV748X_I2C_DPLL << 1},
> - {ADV748X_PAGE_IO, 0xf4, ADV748X_I2C_CP << 1},
> - {ADV748X_PAGE_IO,

[PATCH v2 3/3] media: i2c: adv748x: Add support for i2c_new_secondary_device

2018-02-27 Thread Kieran Bingham 
From: Kieran Bingham 

The ADV748x has twelve 256-byte maps that can be accessed via the main
I2C ports. Each map has it own I2C address and acts as a standard slave
device on the I2C bus.

Allow a device tree node to override the default addresses so that
address conflicts with other devices on the same bus may be resolved at
the board description level.

Signed-off-by: Kieran Bingham 
---
 drivers/media/i2c/adv748x/adv748x-core.c | 77 +++-
 drivers/media/i2c/adv748x/adv748x.h  | 14 --
 2 files changed, 36 insertions(+), 55 deletions(-)

diff --git a/drivers/media/i2c/adv748x/adv748x-core.c 
b/drivers/media/i2c/adv748x/adv748x-core.c
index 9dcaadaca217..911ccf6eb68c 100644
--- a/drivers/media/i2c/adv748x/adv748x-core.c
+++ b/drivers/media/i2c/adv748x/adv748x-core.c
@@ -80,21 +80,24 @@ static int adv748x_configure_regmap(struct adv748x_state 
*state, int region)
 
return 0;
 }
+struct adv748x_register_map {
+   const char *name;
+   u8 default_addr;
+};
 
-/* Default addresses for the I2C pages */
-static int adv748x_i2c_addresses[ADV748X_PAGE_MAX] = {
-   ADV748X_I2C_IO,
-   ADV748X_I2C_DPLL,
-   ADV748X_I2C_CP,
-   ADV748X_I2C_HDMI,
-   ADV748X_I2C_EDID,
-   ADV748X_I2C_REPEATER,
-   ADV748X_I2C_INFOFRAME,
-   ADV748X_I2C_CBUS,
-   ADV748X_I2C_CEC,
-   ADV748X_I2C_SDP,
-   ADV748X_I2C_TXB,
-   ADV748X_I2C_TXA,
+static const struct adv748x_register_map adv748x_default_addresses[] = {
+   [ADV748X_PAGE_IO] = { "main", 0x70 },
+   [ADV748X_PAGE_DPLL] = { "dpll", 0x26 },
+   [ADV748X_PAGE_CP] = { "cp", 0x22 },
+   [ADV748X_PAGE_HDMI] = { "hdmi", 0x34 },
+   [ADV748X_PAGE_EDID] = { "edid", 0x36 },
+   [ADV748X_PAGE_REPEATER] = { "repeater", 0x32 },
+   [ADV748X_PAGE_INFOFRAME] = { "infoframe", 0x31 },
+   [ADV748X_PAGE_CBUS] = { "cbus", 0x30 },
+   [ADV748X_PAGE_CEC] = { "cec", 0x41 },
+   [ADV748X_PAGE_SDP] = { "sdp", 0x79 },
+   [ADV748X_PAGE_TXB] = { "txb", 0x48 },
+   [ADV748X_PAGE_TXA] = { "txa", 0x4a },
 };
 
 static int adv748x_read_check(struct adv748x_state *state,
@@ -143,15 +146,20 @@ int adv748x_write_block(struct adv748x_state *state, int 
client_page,
return regmap_raw_write(regmap, init_reg, val, val_len);
 }
 
-static struct i2c_client *adv748x_dummy_client(struct adv748x_state *state,
-  u8 addr, u8 io_reg)
+static int adv748x_set_slave_addresses(struct adv748x_state *state)
 {
-   struct i2c_client *client = state->client;
+   struct i2c_client *client;
+   unsigned int i;
+   u8 io_reg;
+
+   for (i = ADV748X_PAGE_DPLL; i < ADV748X_PAGE_MAX; ++i) {
+   io_reg = ADV748X_IO_SLAVE_ADDR_BASE + i;
+   client = state->i2c_clients[i];
 
-   if (addr)
-   io_write(state, io_reg, addr << 1);
+   io_write(state, io_reg, client->addr << 1);
+   }
 
-   return i2c_new_dummy(client->adapter, io_read(state, io_reg) >> 1);
+   return 0;
 }
 
 static void adv748x_unregister_clients(struct adv748x_state *state)
@@ -164,13 +172,15 @@ static void adv748x_unregister_clients(struct 
adv748x_state *state)
 
 static int adv748x_initialise_clients(struct adv748x_state *state)
 {
-   int i;
+   unsigned int i;
int ret;
 
for (i = ADV748X_PAGE_DPLL; i < ADV748X_PAGE_MAX; ++i) {
-   state->i2c_clients[i] =
-   adv748x_dummy_client(state, adv748x_i2c_addresses[i],
-ADV748X_IO_SLAVE_ADDR_BASE + i);
+   state->i2c_clients[i] = i2c_new_secondary_device(
+   state->client,
+   adv748x_default_addresses[i].name,
+   adv748x_default_addresses[i].default_addr);
+
if (state->i2c_clients[i] == NULL) {
adv_err(state, "failed to create i2c client %u\n", i);
return -ENOMEM;
@@ -181,7 +191,7 @@ static int adv748x_initialise_clients(struct adv748x_state 
*state)
return ret;
}
 
-   return 0;
+   return adv748x_set_slave_addresses(state);
 }
 
 /**
@@ -347,21 +357,6 @@ static const struct adv748x_reg_value adv748x_sw_reset[] = 
{
{ADV748X_PAGE_EOR, 0xff, 0xff}  /* End of register table */
 };
 
-static const struct adv748x_reg_value adv748x_set_slave_address[] = {
-   {ADV748X_PAGE_IO, 0xf3, ADV748X_I2C_DPLL << 1},
-   {ADV748X_PAGE_IO, 0xf4, ADV748X_I2C_CP << 1},
-   {ADV748X_PAGE_IO, 0xf5, ADV748X_I2C_HDMI << 1},
-   {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,