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

[PATCH v2 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 v2 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.
The ADV7604 has thirteen 256-byte maps that can be accessed via the main
I²C ports. Each map has it own I²C address and acts
as a standard slave device on the I²C bus.

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      | 17 +++++-
 drivers/media/i2c/adv7604.c                        | 60 ++++++++++++++--------
 2 files changed, 55 insertions(+), 22 deletions(-)

diff --git a/Documentation/devicetree/bindings/media/i2c/adv7604.txt b/Documentation/devicetree/bindings/media/i2c/adv7604.txt
index c27cede..8486b5c 100644
--- a/Documentation/devicetree/bindings/media/i2c/adv7604.txt
+++ b/Documentation/devicetree/bindings/media/i2c/adv7604.txt
@@ -10,8 +10,12 @@ Required Properties:
 
   - compatible: Must contain one of the following
     - "adi,adv7611" for the ADV7611
+    - "adi,adv7604" for the ADV7604
 
-  - reg: I2C slave address
+  - reg: I2C slave addresses
+    The ADV7604 has thirteen 256-byte maps that can be accessed via the main
+    I²C ports. Each map has it own I²C address and acts
+    as a standard slave device on the I²C bus.
 
   - hpd-gpios: References to the GPIOs that control the HDMI hot-plug
     detection pins, one per HDMI input. The active flag indicates the GPIO
@@ -32,6 +36,12 @@ 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 maps with programmable addresses.
+		It can contain any map needing another address than default one.
+		Possible maps names are :
+ADV7604 : "main", "avlink", "cec", "infoframe", "esdp", "dpp", "afe", "rep",
+		"edid", "hdmi", "test", "cp", "vdp"
+ADV7611 : "main", "cec", "infoframe", "afe", "rep", "edid", "hdmi", "cp"
 
 Optional Endpoint Properties:
 
@@ -50,7 +60,10 @@ Example:
 
 	hdmi_receiver@4c {
 		compatible = "adi,adv7611";
-		reg = <0x4c>;
+		/* edid page will be accessible @ 0x66 on i2c bus */
+		/* other maps keep their default addresses */
+		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..56037dd 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,31 @@ 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;
+	unsigned int default_addr = io_read(sd, io_reg) >> 1;
+	struct i2c_client *new_client;
+
+	if (IS_ENABLED(CONFIG_OF)) {
+		/* Try to find it in DT */
+		new_client = i2c_new_secondary_device(client,
+			adv7604_secondary_names[i], default_addr);
+	} else if (pdata) {
+		if (pdata->i2c_addresses[i])
+			new_client = i2c_new_dummy(client->adapter,
+						pdata->i2c_addresses[i]);
+		else
+			new_client = i2c_new_dummy(client->adapter,
+						default_addr);
+	}
 
-	if (addr)
-		io_write(sd, io_reg, addr << 1);
-	return i2c_new_dummy(client->adapter, io_read(sd, io_reg) >> 1);
+	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 +2711,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 +2752,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 +2912,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 v2 1/2] Allow DT parsing of secondary devices

[Lars-Peter Clausen]

On 08/29/2014 05:15 PM, Jean-Michel Hautbois wrote:
> 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>

This needs a better description explaining the problem and how it is solved. 
How about
"""
i2c: Add generic support passing secondary devices addresses

Some I2C devices have multiple addresses assigned, for example each address 
corresponding to a different internal register map page of the device. So 
far drivers which need support for this have handled this with a driver 
specific and non-generic implementation, e.g. passing the additional address 
via platform data.

This patch provides a new helper function called i2c_new_secondary_device() 
which is intended to provide a generic way to get the secondary address as 
well as instantiate a struct i2c_client for the secondary address. The 
function expects a pointer to the primary i2c_client, a name for the 
secondary address and an optional default address. The name is used as a 
handle to specify which secondary address to get. The default address is 
used as a fallback in case no secondary address was explicitly specified. In 
case no secondary address and no default address were specified the function 
returns NULL.

For now the function only supports look-up of the secondary address from 
devicetree, but it can be extended in the future to for example support 
board files and/or ACPI.
"""

> ---
>   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);
>

The function needs a kernel doc description.

> +struct i2c_client *i2c_new_secondary_device(struct i2c_client *client,
> +						const char *name,
> +						u32 default_addr)

The I2C framework commonly uses u16 for the type of a I2C address.


> +{
> +	int i, addr;

addr needs to be u32 here since it is passed to of_property_read_u32_index().

> +	struct device_node *np;
> +
> +	np = client->dev.of_node;

of_node can be NULL, this needs to be handled. Ideally by using the default 
address.

> +	i = of_property_match_string(np, "reg-names", name);
> +	if (i >= 0)
> +		of_property_read_u32_index(np, "reg", i, &addr);

of_property_read_u32_index() can fail, this needs to be handled.

> +	else
> +		addr = default_addr;

If no address was specified and default_addr is 0 the function should return 
NULL.

> +
> +	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 */

The comment should go into i2c-core.c and be in proper kernel doc fully 
explaining the function, its parameters and the behavior.

> +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 */
>
>

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

[Lars-Peter Clausen]

On 08/29/2014 05:15 PM, Jean-Michel Hautbois wrote:
> 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@vodalys.com>

This needs a better description explaining the problem and how it is solved. 
How about
"""
i2c: Add generic support passing secondary devices addresses

Some I2C devices have multiple addresses assigned, for example each address 
corresponding to a different internal register map page of the device. So 
far drivers which need support for this have handled this with a driver 
specific and non-generic implementation, e.g. passing the additional address 
via platform data.

This patch provides a new helper function called i2c_new_secondary_device() 
which is intended to provide a generic way to get the secondary address as 
well as instantiate a struct i2c_client for the secondary address. The 
function expects a pointer to the primary i2c_client, a name for the 
secondary address and an optional default address. The name is used as a 
handle to specify which secondary address to get. The default address is 
used as a fallback in case no secondary address was explicitly specified. In 
case no secondary address and no default address were specified the function 
returns NULL.

For now the function only supports look-up of the secondary address from 
devicetree, but it can be extended in the future to for example support 
board files and/or ACPI.
"""

The patch should also update the I2C devicetree bindings documentation to 
explain how bindings for devices with multiple addresses work.

> ---
>   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);
>

The function needs a kernel doc description.

> +struct i2c_client *i2c_new_secondary_device(struct i2c_client *client,
> +						const char *name,
> +						u32 default_addr)

The I2C framework commonly uses u16 for the type of a I2C address.


> +{
> +	int i, addr;

addr needs to be u32 here since it is passed to of_property_read_u32_index().

> +	struct device_node *np;
> +
> +	np = client->dev.of_node;

of_node can be NULL, this needs to be handled. Ideally by using the default 
address.

> +	i = of_property_match_string(np, "reg-names", name);
> +	if (i >= 0)
> +		of_property_read_u32_index(np, "reg", i, &addr);

of_property_read_u32_index() can fail, this needs to be handled.

> +	else
> +		addr = default_addr;

If no address was specified and default_addr is 0 the function should return 
NULL.

> +
> +	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 */

The comment should go into i2c-core.c and be in proper kernel doc fully 
explaining the function, its parameters and the behavior.

> +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 */
>
>

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

[Lars-Peter Clausen]

On 08/29/2014 05:15 PM, Jean-Michel Hautbois wrote:
> This patch uses DT in order to parse addresses for dummy devices of adv7604.
> The ADV7604 has thirteen 256-byte maps that can be accessed via the main
> I²C ports. Each map has it own I²C address and acts
> as a standard slave device on the I²C bus.
>
> If nothing is defined, it uses default addresses.
> The main prupose is using two adv76xx on the same i2c bus.

Ideally this patch is split up in two patches. One patch adding support for 
i2c_new_secondary_device() and one patch adding support for DT for the adv7604.

[...]
> +static const char const *adv7604_secondary_names[] = {
> +	"main", /* ADV7604_PAGE_IO */

How about [ADV7604_PAGE_IO] = "main", instead of having the comment, this 
makes things more explicit.

> +	"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,31 @@ 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;
> +	unsigned int default_addr = io_read(sd, io_reg) >> 1;
> +	struct i2c_client *new_client;
> +
> +	if (IS_ENABLED(CONFIG_OF)) {

No CONFIG_OF. i2c_new_secondary_device() is supposed to be the generic 
method of instantiating the secondary i2c_client, regardless of how the 
address is specified. For this driver we still need to keep the old method 
of instantiation via platform data for legacy reasons for now. So what this 
should look like is:


	if (pdata && pdata->i2c_addresses[i])
		new_client = i2c_new_dummy(client->adapter,
			pdata->i2c_addresses[i]);	
	else
		new_client = i2c_new_secondary_device(client,
			adv7604_secondary_names[i], default_addr);


> +		/* Try to find it in DT */
> +		new_client = i2c_new_secondary_device(client,
> +			adv7604_secondary_names[i], default_addr);
> +	} else if (pdata) {
> +		if (pdata->i2c_addresses[i])
> +			new_client = i2c_new_dummy(client->adapter,
> +						pdata->i2c_addresses[i]);
> +		else
> +			new_client = i2c_new_dummy(client->adapter,
> +						default_addr);
> +	}
>
[...]
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Laurent Pinchart]

Hi Jean-Michel,

Thank you for the patch.

On Friday 29 August 2014 17:15:03 Jean-Michel Hautbois wrote:
> This patch uses DT in order to parse addresses for dummy devices of adv7604.
> The ADV7604 has thirteen 256-byte maps that can be accessed via the main
> I²C ports. Each map has it own I²C address and acts
> as a standard slave device on the I²C bus.
> 
> 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      | 17 +++++-
>  drivers/media/i2c/adv7604.c                        | 60 ++++++++++++-------
>  2 files changed, 55 insertions(+), 22 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/media/i2c/adv7604.txt
> b/Documentation/devicetree/bindings/media/i2c/adv7604.txt index
> c27cede..8486b5c 100644
> --- a/Documentation/devicetree/bindings/media/i2c/adv7604.txt
> +++ b/Documentation/devicetree/bindings/media/i2c/adv7604.txt
> @@ -10,8 +10,12 @@ Required Properties:
> 
>    - compatible: Must contain one of the following
>      - "adi,adv7611" for the ADV7611
> +    - "adi,adv7604" for the ADV7604

Addition of ADV7604 support is unrelated to the subject and needs to be split 
into a separate patch.

> -  - reg: I2C slave address
> +  - reg: I2C slave addresses
> +    The ADV7604 has thirteen 256-byte maps that can be accessed via the
> main
> +    I²C ports. Each map has it own I²C address and acts
> +    as a standard slave device on the I²C bus.
> 
>    - hpd-gpios: References to the GPIOs that control the HDMI hot-plug
>      detection pins, one per HDMI input. The active flag indicates the GPIO
> @@ -32,6 +36,12 @@ 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 maps with programmable addresses.
> +		It can contain any map needing another address than default one.
> +		Possible maps names are :
> +ADV7604 : "main", "avlink", "cec", "infoframe", "esdp", "dpp", "afe",
> "rep",
> +		"edid", "hdmi", "test", "cp", "vdp"
> +ADV7611 : "main", "cec", "infoframe", "afe", "rep", "edid", "hdmi", "cp"
> 
>  Optional Endpoint Properties:
> 
> @@ -50,7 +60,10 @@ Example:
> 
>  	hdmi_receiver@4c {
>  		compatible = "adi,adv7611";
> -		reg = <0x4c>;
> +		/* edid page will be accessible @ 0x66 on i2c bus */
> +		/* other maps keep their default addresses */
> +		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..56037dd 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,31 @@ 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;
> +	unsigned int default_addr = io_read(sd, io_reg) >> 1;

This modifies the behaviour of the driver. It previously used fixed default 
addresses in the DT case, and now defaults to whatever has been programmed in 
the chip. This might not be an issue in itself, but it should be documented in 
the commit message (and possibly split to a separate patch).

> +	struct i2c_client *new_client;
> +
> +	if (IS_ENABLED(CONFIG_OF)) {
> +		/* Try to find it in DT */
> +		new_client = i2c_new_secondary_device(client,
> +			adv7604_secondary_names[i], default_addr);
> +	} else if (pdata) {
> +		if (pdata->i2c_addresses[i])
> +			new_client = i2c_new_dummy(client->adapter,
> +						pdata->i2c_addresses[i]);
> +		else
> +			new_client = i2c_new_dummy(client->adapter,
> +						default_addr);
> +	}
> 
> -	if (addr)
> -		io_write(sd, io_reg, addr << 1);
> -	return i2c_new_dummy(client->adapter, io_read(sd, io_reg) >> 1);
> +	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 +2711,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 +2752,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 +2912,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);

-- 
Regards,

Laurent Pinchart

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

[Jean-Michel Hautbois]

2014-08-31 19:18 GMT+02:00 Laurent Pinchart <laurent.pinchart@ideasonboard.com>:
> Hi Jean-Michel,
>
> Thank you for the patch.
>
> On Friday 29 August 2014 17:15:03 Jean-Michel Hautbois wrote:
>> This patch uses DT in order to parse addresses for dummy devices of adv7604.
>> The ADV7604 has thirteen 256-byte maps that can be accessed via the main
>> I²C ports. Each map has it own I²C address and acts
>> as a standard slave device on the I²C bus.
>>
>> 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+Q8N6RmIDaIwRZHo2/mJg@public.gmane.orgm>
>> ---
>>  .../devicetree/bindings/media/i2c/adv7604.txt      | 17 +++++-
>>  drivers/media/i2c/adv7604.c                        | 60 ++++++++++++-------
>>  2 files changed, 55 insertions(+), 22 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/media/i2c/adv7604.txt
>> b/Documentation/devicetree/bindings/media/i2c/adv7604.txt index
>> c27cede..8486b5c 100644
>> --- a/Documentation/devicetree/bindings/media/i2c/adv7604.txt
>> +++ b/Documentation/devicetree/bindings/media/i2c/adv7604.txt
>> @@ -10,8 +10,12 @@ Required Properties:
>>
>>    - compatible: Must contain one of the following
>>      - "adi,adv7611" for the ADV7611
>> +    - "adi,adv7604" for the ADV7604
>
> Addition of ADV7604 support is unrelated to the subject and needs to be split
> into a separate patch.

OK, I will do that.

>> -  - reg: I2C slave address
>> +  - reg: I2C slave addresses
>> +    The ADV7604 has thirteen 256-byte maps that can be accessed via the
>> main
>> +    I²C ports. Each map has it own I²C address and acts
>> +    as a standard slave device on the I²C bus.
>>
>>    - hpd-gpios: References to the GPIOs that control the HDMI hot-plug
>>      detection pins, one per HDMI input. The active flag indicates the GPIO
>> @@ -32,6 +36,12 @@ 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 maps with programmable addresses.
>> +             It can contain any map needing another address than default one.
>> +             Possible maps names are :
>> +ADV7604 : "main", "avlink", "cec", "infoframe", "esdp", "dpp", "afe",
>> "rep",
>> +             "edid", "hdmi", "test", "cp", "vdp"
>> +ADV7611 : "main", "cec", "infoframe", "afe", "rep", "edid", "hdmi", "cp"
>>
>>  Optional Endpoint Properties:
>>
>> @@ -50,7 +60,10 @@ Example:
>>
>>       hdmi_receiver@4c {
>>               compatible = "adi,adv7611";
>> -             reg = <0x4c>;
>> +             /* edid page will be accessible @ 0x66 on i2c bus */
>> +             /* other maps keep their default addresses */
>> +             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..56037dd 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,31 @@ 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;
>> +     unsigned int default_addr = io_read(sd, io_reg) >> 1;
>
> This modifies the behaviour of the driver. It previously used fixed default
> addresses in the DT case, and now defaults to whatever has been programmed in
> the chip. This might not be an issue in itself, but it should be documented in
> the commit message (and possibly split to a separate patch).

Then, let's decide if this is a problem or not :) ? I naively thought
that it would be good to have default address, if defined in platform
data, use this one instead, and if it is in DT, it should be the prior
address to use (priority on DT and not on platform data).
But this is probably ideal for me but not for others ?

Thanks,
JM