[PATCH 1/2] Allow DT parsing of secondary devices

[PATCH 1/2] Allow DT parsing of secondary devices

[Jean-Michel Hautbois]

This is based on reg and reg-names in DT.
Example:

reg = <0x10 0x20 0x30>;
reg-names = "main", "io", "test";

This function will create dummy devices io and test
with addresses 0x20 and 0x30 respectively.

Signed-off-by: Jean-Michel Hautbois <jean-michel.hautbois-B+Q8N6RmIDZBDgjK7y7TUQ@public.gmane.org>
---
 drivers/i2c/i2c-core.c | 20 ++++++++++++++++++++
 include/linux/i2c.h    |  6 ++++++
 2 files changed, 26 insertions(+)

diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
index 632057a..5eb414d 100644
--- a/drivers/i2c/i2c-core.c
+++ b/drivers/i2c/i2c-core.c
@@ -798,6 +798,26 @@ struct i2c_client *i2c_new_dummy(struct i2c_adapter *adapter, u16 address)
 }
 EXPORT_SYMBOL_GPL(i2c_new_dummy);
 
+struct i2c_client *i2c_new_secondary_device(struct i2c_client *client,
+						const char *name,
+						u32 default_addr)
+{
+	int i, addr;
+	struct device_node *np;
+
+	np = client->dev.of_node;
+	i = of_property_match_string(np, "reg-names", name);
+	if (i >= 0)
+		of_property_read_u32_index(np, "reg", i, &addr);
+	else
+		addr = default_addr;
+
+	dev_dbg(&client->adapter->dev, "Address for %s : 0x%x\n", name, addr);
+	return i2c_new_dummy(client->adapter, addr);
+}
+EXPORT_SYMBOL_GPL(i2c_new_secondary_device);
+
+
 /* ------------------------------------------------------------------------- */
 
 /* I2C bus adapters -- one roots each I2C or SMBUS segment */
diff --git a/include/linux/i2c.h b/include/linux/i2c.h
index a95efeb..2d143d7 100644
--- a/include/linux/i2c.h
+++ b/include/linux/i2c.h
@@ -322,6 +322,12 @@ extern int i2c_probe_func_quick_read(struct i2c_adapter *, unsigned short addr);
 extern struct i2c_client *
 i2c_new_dummy(struct i2c_adapter *adap, u16 address);
 
+/* Use reg/reg-names in DT in order to get extra addresses */
+extern struct i2c_client *
+i2c_new_secondary_device(struct i2c_client *client,
+				const char *name,
+				u32 default_addr);
+
 extern void i2c_unregister_device(struct i2c_client *);
 #endif /* I2C */
 
-- 
2.0.4

[PATCH 2/2] adv7604: Use DT parsing in dummy creation

[Jean-Michel Hautbois]

This patch uses DT in order to parse addresses for dummy devices of adv7604.
If nothing is defined, it uses default addresses.
The main prupose is using two adv76xx on the same i2c bus.

Signed-off-by: Jean-Michel Hautbois <jean-michel.hautbois-B+Q8N6RmIDZBDgjK7y7TUQ@public.gmane.org>
---
 .../devicetree/bindings/media/i2c/adv7604.txt      |  7 ++-
 drivers/media/i2c/adv7604.c                        | 56 ++++++++++++++--------
 2 files changed, 42 insertions(+), 21 deletions(-)

diff --git a/Documentation/devicetree/bindings/media/i2c/adv7604.txt b/Documentation/devicetree/bindings/media/i2c/adv7604.txt
index c27cede..221b75c 100644
--- a/Documentation/devicetree/bindings/media/i2c/adv7604.txt
+++ b/Documentation/devicetree/bindings/media/i2c/adv7604.txt
@@ -10,6 +10,7 @@ Required Properties:
 
   - compatible: Must contain one of the following
     - "adi,adv7611" for the ADV7611
+    - "adi,adv7604" for the ADV7604
 
   - reg: I2C slave address
 
@@ -32,6 +33,8 @@ The digital output port node must contain at least one endpoint.
 Optional Properties:
 
   - reset-gpios: Reference to the GPIO connected to the device's reset pin.
+  - reg-names : Names of registers to be reprogrammed.
+		Refer to source code for possible values.
 
 Optional Endpoint Properties:
 
@@ -50,7 +53,9 @@ Example:
 
 	hdmi_receiver@4c {
 		compatible = "adi,adv7611";
-		reg = <0x4c>;
+		/* edid page will be accessible @ 0x66 on i2c bus*/
+		reg = <0x4c 0x66>;
+		reg-names = "main", "edid";
 
 		reset-gpios = <&ioexp 0 GPIO_ACTIVE_LOW>;
 		hpd-gpios = <&ioexp 2 GPIO_ACTIVE_HIGH>;
diff --git a/drivers/media/i2c/adv7604.c b/drivers/media/i2c/adv7604.c
index d4fa213..4e660a2 100644
--- a/drivers/media/i2c/adv7604.c
+++ b/drivers/media/i2c/adv7604.c
@@ -326,6 +326,22 @@ static const struct adv7604_video_standards adv7604_prim_mode_hdmi_gr[] = {
 	{ },
 };
 
+static const char const *adv7604_secondary_names[] = {
+	"main", /* ADV7604_PAGE_IO */
+	"avlink", /* ADV7604_PAGE_AVLINK */
+	"cec", /* ADV7604_PAGE_CEC */
+	"infoframe", /* ADV7604_PAGE_INFOFRAME */
+	"esdp", /* ADV7604_PAGE_ESDP */
+	"dpp", /* ADV7604_PAGE_DPP */
+	"afe", /* ADV7604_PAGE_AFE */
+	"rep", /* ADV7604_PAGE_REP */
+	"edid", /* ADV7604_PAGE_EDID */
+	"hdmi", /* ADV7604_PAGE_HDMI */
+	"test", /* ADV7604_PAGE_TEST */
+	"cp", /* ADV7604_PAGE_CP */
+	"vdp" /* ADV7604_PAGE_VDP */
+};
+
 /* ----------------------------------------------------------------------- */
 
 static inline struct adv7604_state *to_state(struct v4l2_subdev *sd)
@@ -2528,13 +2544,27 @@ static void adv7604_unregister_clients(struct adv7604_state *state)
 }
 
 static struct i2c_client *adv7604_dummy_client(struct v4l2_subdev *sd,
-							u8 addr, u8 io_reg)
+						unsigned int i)
 {
 	struct i2c_client *client = v4l2_get_subdevdata(sd);
+	struct adv7604_platform_data *pdata = client->dev.platform_data;
+	unsigned int io_reg = 0xf2 + i;
+	struct i2c_client *new_client;
+
+	/* Try to find it in DT */
+	new_client = i2c_new_secondary_device(client,
+			adv7604_secondary_names[i], io_read(sd, io_reg) >> 1);
 
-	if (addr)
-		io_write(sd, io_reg, addr << 1);
-	return i2c_new_dummy(client->adapter, io_read(sd, io_reg) >> 1);
+	if (!new_client)
+		/* if not defined in DT, use default if available */
+		if (pdata && pdata->i2c_addresses[i])
+			new_client = i2c_new_dummy(client->adapter,
+						pdata->i2c_addresses[i]);
+
+	if (new_client)
+		io_write(sd, io_reg, new_client->addr << 1);
+
+	return new_client;
 }
 
 static const struct adv7604_reg_seq adv7604_recommended_settings_afe[] = {
@@ -2677,6 +2707,7 @@ MODULE_DEVICE_TABLE(i2c, adv7604_i2c_id);
 
 static struct of_device_id adv7604_of_id[] __maybe_unused = {
 	{ .compatible = "adi,adv7611", .data = &adv7604_chip_info[ADV7611] },
+	{ .compatible = "adi,adv7604", .data = &adv7604_chip_info[ADV7604] },
 	{ }
 };
 MODULE_DEVICE_TABLE(of, adv7604_of_id);
@@ -2717,20 +2748,6 @@ static int adv7604_parse_dt(struct adv7604_state *state)
 	/* Disable the interrupt for now as no DT-based board uses it. */
 	state->pdata.int1_config = ADV7604_INT1_CONFIG_DISABLED;
 
-	/* Use the default I2C addresses. */
-	state->pdata.i2c_addresses[ADV7604_PAGE_AVLINK] = 0x42;
-	state->pdata.i2c_addresses[ADV7604_PAGE_CEC] = 0x40;
-	state->pdata.i2c_addresses[ADV7604_PAGE_INFOFRAME] = 0x3e;
-	state->pdata.i2c_addresses[ADV7604_PAGE_ESDP] = 0x38;
-	state->pdata.i2c_addresses[ADV7604_PAGE_DPP] = 0x3c;
-	state->pdata.i2c_addresses[ADV7604_PAGE_AFE] = 0x26;
-	state->pdata.i2c_addresses[ADV7604_PAGE_REP] = 0x32;
-	state->pdata.i2c_addresses[ADV7604_PAGE_EDID] = 0x36;
-	state->pdata.i2c_addresses[ADV7604_PAGE_HDMI] = 0x34;
-	state->pdata.i2c_addresses[ADV7604_PAGE_TEST] = 0x30;
-	state->pdata.i2c_addresses[ADV7604_PAGE_CP] = 0x22;
-	state->pdata.i2c_addresses[ADV7604_PAGE_VDP] = 0x24;
-
 	/* Hardcode the remaining platform data fields. */
 	state->pdata.disable_pwrdnb = 0;
 	state->pdata.disable_cable_det_rst = 0;
@@ -2891,8 +2908,7 @@ static int adv7604_probe(struct i2c_client *client,
 			continue;
 
 		state->i2c_clients[i] =
-			adv7604_dummy_client(sd, state->pdata.i2c_addresses[i],
-					     0xf2 + i);
+			adv7604_dummy_client(sd, i);
 		if (state->i2c_clients[i] == NULL) {
 			err = -ENOMEM;
 			v4l2_err(sd, "failed to create i2c client %u\n", i);
-- 
2.0.4

Re: [PATCH 2/2] adv7604: Use DT parsing in dummy creation

[Hans Verkuil]

On 08/29/2014 03:28 PM, Jean-Michel Hautbois wrote:
> This patch uses DT in order to parse addresses for dummy devices of adv7604.
> If nothing is defined, it uses default addresses.
> The main prupose is using two adv76xx on the same i2c bus.
> 
> Signed-off-by: Jean-Michel Hautbois <jean-michel.hautbois@vodalys.com>
> ---
>  .../devicetree/bindings/media/i2c/adv7604.txt      |  7 ++-
>  drivers/media/i2c/adv7604.c                        | 56 ++++++++++++++--------
>  2 files changed, 42 insertions(+), 21 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/media/i2c/adv7604.txt b/Documentation/devicetree/bindings/media/i2c/adv7604.txt
> index c27cede..221b75c 100644
> --- a/Documentation/devicetree/bindings/media/i2c/adv7604.txt
> +++ b/Documentation/devicetree/bindings/media/i2c/adv7604.txt
> @@ -10,6 +10,7 @@ Required Properties:
>  
>    - compatible: Must contain one of the following
>      - "adi,adv7611" for the ADV7611
> +    - "adi,adv7604" for the ADV7604
>  
>    - reg: I2C slave address
>  
> @@ -32,6 +33,8 @@ The digital output port node must contain at least one endpoint.
>  Optional Properties:
>  
>    - reset-gpios: Reference to the GPIO connected to the device's reset pin.
> +  - reg-names : Names of registers to be reprogrammed.
> +		Refer to source code for possible values.
>  
>  Optional Endpoint Properties:
>  
> @@ -50,7 +53,9 @@ Example:
>  
>  	hdmi_receiver@4c {
>  		compatible = "adi,adv7611";
> -		reg = <0x4c>;
> +		/* edid page will be accessible @ 0x66 on i2c bus*/
> +		reg = <0x4c 0x66>;
> +		reg-names = "main", "edid";
>  
>  		reset-gpios = <&ioexp 0 GPIO_ACTIVE_LOW>;
>  		hpd-gpios = <&ioexp 2 GPIO_ACTIVE_HIGH>;
> diff --git a/drivers/media/i2c/adv7604.c b/drivers/media/i2c/adv7604.c
> index d4fa213..4e660a2 100644
> --- a/drivers/media/i2c/adv7604.c
> +++ b/drivers/media/i2c/adv7604.c
> @@ -326,6 +326,22 @@ static const struct adv7604_video_standards adv7604_prim_mode_hdmi_gr[] = {
>  	{ },
>  };
>  
> +static const char const *adv7604_secondary_names[] = {
> +	"main", /* ADV7604_PAGE_IO */
> +	"avlink", /* ADV7604_PAGE_AVLINK */
> +	"cec", /* ADV7604_PAGE_CEC */
> +	"infoframe", /* ADV7604_PAGE_INFOFRAME */
> +	"esdp", /* ADV7604_PAGE_ESDP */
> +	"dpp", /* ADV7604_PAGE_DPP */
> +	"afe", /* ADV7604_PAGE_AFE */
> +	"rep", /* ADV7604_PAGE_REP */
> +	"edid", /* ADV7604_PAGE_EDID */
> +	"hdmi", /* ADV7604_PAGE_HDMI */
> +	"test", /* ADV7604_PAGE_TEST */
> +	"cp", /* ADV7604_PAGE_CP */
> +	"vdp" /* ADV7604_PAGE_VDP */
> +};
> +
>  /* ----------------------------------------------------------------------- */
>  
>  static inline struct adv7604_state *to_state(struct v4l2_subdev *sd)
> @@ -2528,13 +2544,27 @@ static void adv7604_unregister_clients(struct adv7604_state *state)
>  }
>  
>  static struct i2c_client *adv7604_dummy_client(struct v4l2_subdev *sd,
> -							u8 addr, u8 io_reg)
> +						unsigned int i)
>  {
>  	struct i2c_client *client = v4l2_get_subdevdata(sd);
> +	struct adv7604_platform_data *pdata = client->dev.platform_data;
> +	unsigned int io_reg = 0xf2 + i;
> +	struct i2c_client *new_client;
> +
> +	/* Try to find it in DT */
> +	new_client = i2c_new_secondary_device(client,
> +			adv7604_secondary_names[i], io_read(sd, io_reg) >> 1);

Does this work if CONFIG_OF isn't set? I suspect not.

>  
> -	if (addr)
> -		io_write(sd, io_reg, addr << 1);
> -	return i2c_new_dummy(client->adapter, io_read(sd, io_reg) >> 1);
> +	if (!new_client)
> +		/* if not defined in DT, use default if available */
> +		if (pdata && pdata->i2c_addresses[i])
> +			new_client = i2c_new_dummy(client->adapter,
> +						pdata->i2c_addresses[i]);

This is not the same as the original code. If pdata->i2c_addresses[i] == 0,
then it should use io_read(sd, io_reg) >> 1 as the address for i2c_new_dummy.

This would break existing code that uses platform_data (such as on our PCIe
board).

Regards,

	Hans

> +
> +	if (new_client)
> +		io_write(sd, io_reg, new_client->addr << 1);
> +
> +	return new_client;
>  }
>  
>  static const struct adv7604_reg_seq adv7604_recommended_settings_afe[] = {
> @@ -2677,6 +2707,7 @@ MODULE_DEVICE_TABLE(i2c, adv7604_i2c_id);
>  
>  static struct of_device_id adv7604_of_id[] __maybe_unused = {
>  	{ .compatible = "adi,adv7611", .data = &adv7604_chip_info[ADV7611] },
> +	{ .compatible = "adi,adv7604", .data = &adv7604_chip_info[ADV7604] },
>  	{ }
>  };
>  MODULE_DEVICE_TABLE(of, adv7604_of_id);
> @@ -2717,20 +2748,6 @@ static int adv7604_parse_dt(struct adv7604_state *state)
>  	/* Disable the interrupt for now as no DT-based board uses it. */
>  	state->pdata.int1_config = ADV7604_INT1_CONFIG_DISABLED;
>  
> -	/* Use the default I2C addresses. */
> -	state->pdata.i2c_addresses[ADV7604_PAGE_AVLINK] = 0x42;
> -	state->pdata.i2c_addresses[ADV7604_PAGE_CEC] = 0x40;
> -	state->pdata.i2c_addresses[ADV7604_PAGE_INFOFRAME] = 0x3e;
> -	state->pdata.i2c_addresses[ADV7604_PAGE_ESDP] = 0x38;
> -	state->pdata.i2c_addresses[ADV7604_PAGE_DPP] = 0x3c;
> -	state->pdata.i2c_addresses[ADV7604_PAGE_AFE] = 0x26;
> -	state->pdata.i2c_addresses[ADV7604_PAGE_REP] = 0x32;
> -	state->pdata.i2c_addresses[ADV7604_PAGE_EDID] = 0x36;
> -	state->pdata.i2c_addresses[ADV7604_PAGE_HDMI] = 0x34;
> -	state->pdata.i2c_addresses[ADV7604_PAGE_TEST] = 0x30;
> -	state->pdata.i2c_addresses[ADV7604_PAGE_CP] = 0x22;
> -	state->pdata.i2c_addresses[ADV7604_PAGE_VDP] = 0x24;
> -
>  	/* Hardcode the remaining platform data fields. */
>  	state->pdata.disable_pwrdnb = 0;
>  	state->pdata.disable_cable_det_rst = 0;
> @@ -2891,8 +2908,7 @@ static int adv7604_probe(struct i2c_client *client,
>  			continue;
>  
>  		state->i2c_clients[i] =
> -			adv7604_dummy_client(sd, state->pdata.i2c_addresses[i],
> -					     0xf2 + i);
> +			adv7604_dummy_client(sd, i);
>  		if (state->i2c_clients[i] == NULL) {
>  			err = -ENOMEM;
>  			v4l2_err(sd, "failed to create i2c client %u\n", i);
> 

Re: [PATCH 2/2] adv7604: Use DT parsing in dummy creation

[Mark Rutland]

Hi,

On Fri, Aug 29, 2014 at 02:28:17PM +0100, Jean-Michel Hautbois wrote:
> This patch uses DT in order to parse addresses for dummy devices of adv7604.
> If nothing is defined, it uses default addresses.
> The main prupose is using two adv76xx on the same i2c bus.

This is rather opaque.

It seems from the code below that a single adv7611 device has multiple
I2C addresses at which different registers may be accessed. I guess the
secondary instances of the unit have different addresses for all of the
pages?

> Signed-off-by: Jean-Michel Hautbois <jean-michel.hautbois@vodalys.com>
> ---
>  .../devicetree/bindings/media/i2c/adv7604.txt      |  7 ++-
>  drivers/media/i2c/adv7604.c                        | 56 ++++++++++++++--------
>  2 files changed, 42 insertions(+), 21 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/media/i2c/adv7604.txt b/Documentation/devicetree/bindings/media/i2c/adv7604.txt
> index c27cede..221b75c 100644
> --- a/Documentation/devicetree/bindings/media/i2c/adv7604.txt
> +++ b/Documentation/devicetree/bindings/media/i2c/adv7604.txt
> @@ -10,6 +10,7 @@ Required Properties:
>  
>    - compatible: Must contain one of the following
>      - "adi,adv7611" for the ADV7611
> +    - "adi,adv7604" for the ADV7604
>  
>    - reg: I2C slave address

This should be updated, at least to say "address(es)".

>  
> @@ -32,6 +33,8 @@ The digital output port node must contain at least one endpoint.
>  Optional Properties:
>  
>    - reset-gpios: Reference to the GPIO connected to the device's reset pin.
> +  - reg-names : Names of registers to be reprogrammed.

This doesn't describe _which_ names you expect, and I have no idea what
is meant by "to be reprogrammed".

> +		Refer to source code for possible values.

Bindings shouldn't say things like this. The binding should describe the
contract between the DTB and the OS, which this clearly doesn't.

A binding document shouldn't necessitate reading code.

>  Optional Endpoint Properties:
>  
> @@ -50,7 +53,9 @@ Example:
>  
>  	hdmi_receiver@4c {
>  		compatible = "adi,adv7611";
> -		reg = <0x4c>;
> +		/* edid page will be accessible @ 0x66 on i2c bus*/
> +		reg = <0x4c 0x66>;
> +		reg-names = "main", "edid";

What about the other IDs? Are they accessible or not?

Why didn't we always list the full set of IDs in the first place? That
would have made this far less painful.

Thanks,
Mark.