[PATCH 1/2] media: adv7604: Add support for i2c_new_secondary_device

[PATCH 1/2] media: adv7604: Add support for i2c_new_secondary_device

[Kieran Bingham]

From: Jean-Michel Hautbois <jean-michel.hautbois@vodalys.com>

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.

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: Jean-Michel Hautbois <jean-michel.hautbois@vodalys.com>
[Kieran: Re-adapted for mainline]
Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
---
Based upon the original posting :
  https://lkml.org/lkml/2014/10/22/469
---
 .../devicetree/bindings/media/i2c/adv7604.txt      | 18 ++++++-
 drivers/media/i2c/adv7604.c                        | 60 ++++++++++++++--------
 2 files changed, 55 insertions(+), 23 deletions(-)

diff --git a/Documentation/devicetree/bindings/media/i2c/adv7604.txt b/Documentation/devicetree/bindings/media/i2c/adv7604.txt
index 9cbd92eb5d05..b64e313dcc66 100644
--- a/Documentation/devicetree/bindings/media/i2c/adv7604.txt
+++ b/Documentation/devicetree/bindings/media/i2c/adv7604.txt
@@ -13,7 +13,11 @@ Required Properties:
     - "adi,adv7611" for the ADV7611
     - "adi,adv7612" for the ADV7612
 
-  - reg: I2C slave address
+  - reg: I2C slave addresses
+    The ADV76xx has up to 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. The main address is mandatory, others are
+    optional and revert to defaults if not specified.
 
   - hpd-gpios: References to the GPIOs that control the HDMI hot-plug
     detection pins, one per HDMI input. The active flag indicates the GPIO
@@ -35,6 +39,11 @@ Optional Properties:
 
   - reset-gpios: Reference to the GPIO connected to the device's reset pin.
   - default-input: Select which input is selected after reset.
+  - reg-names : Names of maps with programmable addresses.
+		It can contain any map needing a non-default address.
+		Possible maps names are :
+		  "main", "avlink", "cec", "infoframe", "esdp", "dpp", "afe",
+		  "rep", "edid", "hdmi", "test", "cp", "vdp"
 
 Optional Endpoint Properties:
 
@@ -52,7 +61,12 @@ Example:
 
 	hdmi_receiver@4c {
 		compatible = "adi,adv7611";
-		reg = <0x4c>;
+		/*
+		 * The edid page will be accessible @ 0x66 on the i2c bus. All
+		 * other maps will retain 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 1544920ec52d..c346b9a8fb57 100644
--- a/drivers/media/i2c/adv7604.c
+++ b/drivers/media/i2c/adv7604.c
@@ -2734,6 +2734,27 @@ static const struct v4l2_ctrl_config adv76xx_ctrl_free_run_color = {
 
 /* ----------------------------------------------------------------------- */
 
+struct adv76xx_register {
+	const char *name;
+	u8 default_addr;
+};
+
+static const struct adv76xx_register adv76xx_secondary_names[] = {
+	[ADV76XX_PAGE_IO] = { "main", 0x4c },
+	[ADV7604_PAGE_AVLINK] = { "avlink", 0x42 },
+	[ADV76XX_PAGE_CEC] = { "cec", 0x40 },
+	[ADV76XX_PAGE_INFOFRAME] = { "infoframe", 0x3e },
+	[ADV7604_PAGE_ESDP] = { "esdp", 0x38 },
+	[ADV7604_PAGE_DPP] = { "dpp", 0x3c },
+	[ADV76XX_PAGE_AFE] = { "afe", 0x26 },
+	[ADV76XX_PAGE_REP] = { "rep", 0x32 },
+	[ADV76XX_PAGE_EDID] = { "edid", 0x36 },
+	[ADV76XX_PAGE_HDMI] = { "hdmi", 0x34 },
+	[ADV76XX_PAGE_TEST] = { "test", 0x30 },
+	[ADV76XX_PAGE_CP] = { "cp", 0x22 },
+	[ADV7604_PAGE_VDP] = { "vdp", 0x24 },
+};
+
 static int adv76xx_core_init(struct v4l2_subdev *sd)
 {
 	struct adv76xx_state *state = to_state(sd);
@@ -2834,13 +2855,26 @@ static void adv76xx_unregister_clients(struct adv76xx_state *state)
 }
 
 static struct i2c_client *adv76xx_dummy_client(struct v4l2_subdev *sd,
-							u8 addr, u8 io_reg)
+					       unsigned int i)
 {
 	struct i2c_client *client = v4l2_get_subdevdata(sd);
+	struct adv76xx_state *state = to_state(sd);
+	struct adv76xx_platform_data *pdata = &state->pdata;
+	unsigned int io_reg = 0xf2 + i;
+	struct i2c_client *new_client;
+
+	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,
+				adv76xx_secondary_names[i].name,
+				adv76xx_secondary_names[i].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 adv76xx_reg_seq adv7604_recommended_settings_afe[] = {
@@ -3115,20 +3149,6 @@ static int adv76xx_parse_dt(struct adv76xx_state *state)
 	/* Disable the interrupt for now as no DT-based board uses it. */
 	state->pdata.int1_config = ADV76XX_INT1_CONFIG_DISABLED;
 
-	/* Use the default I2C addresses. */
-	state->pdata.i2c_addresses[ADV7604_PAGE_AVLINK] = 0x42;
-	state->pdata.i2c_addresses[ADV76XX_PAGE_CEC] = 0x40;
-	state->pdata.i2c_addresses[ADV76XX_PAGE_INFOFRAME] = 0x3e;
-	state->pdata.i2c_addresses[ADV7604_PAGE_ESDP] = 0x38;
-	state->pdata.i2c_addresses[ADV7604_PAGE_DPP] = 0x3c;
-	state->pdata.i2c_addresses[ADV76XX_PAGE_AFE] = 0x26;
-	state->pdata.i2c_addresses[ADV76XX_PAGE_REP] = 0x32;
-	state->pdata.i2c_addresses[ADV76XX_PAGE_EDID] = 0x36;
-	state->pdata.i2c_addresses[ADV76XX_PAGE_HDMI] = 0x34;
-	state->pdata.i2c_addresses[ADV76XX_PAGE_TEST] = 0x30;
-	state->pdata.i2c_addresses[ADV76XX_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;
@@ -3478,9 +3498,7 @@ static int adv76xx_probe(struct i2c_client *client,
 		if (!(BIT(i) & state->info->page_mask))
 			continue;
 
-		state->i2c_clients[i] =
-			adv76xx_dummy_client(sd, state->pdata.i2c_addresses[i],
-					     0xf2 + i);
+		state->i2c_clients[i] = adv76xx_dummy_client(sd, i);
 		if (!state->i2c_clients[i]) {
 			err = -ENOMEM;
 			v4l2_err(sd, "failed to create i2c client %u\n", i);
-- 
2.7.4

Re: [PATCH 1/2] media: adv7604: Add support for i2c_new_secondary_device

[Rob Herring]

On Mon, Jan 22, 2018 at 12:49:56PM +0000, Kieran Bingham wrote:
> From: Jean-Michel Hautbois <jean-michel.hautbois@vodalys.com>
> 
> 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.
> 
> 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: Jean-Michel Hautbois <jean-michel.hautbois@vodalys.com>
> [Kieran: Re-adapted for mainline]
> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> ---
> Based upon the original posting :
>   https://lkml.org/lkml/2014/10/22/469
> ---
>  .../devicetree/bindings/media/i2c/adv7604.txt      | 18 ++++++-

Reviewed-by: Rob Herring <robh@kernel.org>

In the future, please split bindings to separate patch.

>  drivers/media/i2c/adv7604.c                        | 60 ++++++++++++++--------
>  2 files changed, 55 insertions(+), 23 deletions(-)
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/2] media: adv7604: Add support for i2c_new_secondary_device

[Kieran Bingham]

Hi Rob,

On 29/01/18 20:08, Rob Herring wrote:
> On Mon, Jan 22, 2018 at 12:49:56PM +0000, Kieran Bingham wrote:
>> From: Jean-Michel Hautbois <jean-michel.hautbois-B+Q8N6RmIDZBDgjK7y7TUQ@public.gmane.org>
>>
>> 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.
>>
>> 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: Jean-Michel Hautbois <jean-michel.hautbois-B+Q8N6RmIDZBDgjK7y7TUQ@public.gmane.org>
>> [Kieran: Re-adapted for mainline]
>> Signed-off-by: Kieran Bingham <kieran.bingham-ryLnwIuWjnjg/C1BVhZhaw@public.gmane.org>
>> ---
>> Based upon the original posting :
>>   https://lkml.org/lkml/2014/10/22/469
>> ---
>>  .../devicetree/bindings/media/i2c/adv7604.txt      | 18 ++++++-
> 
> Reviewed-by: Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>

Thank you.

> In the future, please split bindings to separate patch.

Yes, of course - sorry - I should probably have known better here.

I was clearly being lazy as the original patch had bindings in with the driver.
Although I don't think I've got an excuse for the second patch in the series :D

I've split them out for the v2.

--
Kieran

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

[PATCH 2/2] drm: adv7511: Add support for i2c_new_secondary_device

[Kieran Bingham]

The ADV7511 has four 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.

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 <kieran.bingham@ideasonboard.com>
---
 .../bindings/display/bridge/adi,adv7511.txt        | 10 +++++-
 drivers/gpu/drm/bridge/adv7511/adv7511.h           |  4 +++
 drivers/gpu/drm/bridge/adv7511/adv7511_drv.c       | 36 ++++++++++++++--------
 3 files changed, 37 insertions(+), 13 deletions(-)

diff --git a/Documentation/devicetree/bindings/display/bridge/adi,adv7511.txt b/Documentation/devicetree/bindings/display/bridge/adi,adv7511.txt
index 0047b1394c70..f6bb9f6d3f48 100644
--- a/Documentation/devicetree/bindings/display/bridge/adi,adv7511.txt
+++ b/Documentation/devicetree/bindings/display/bridge/adi,adv7511.txt
@@ -70,6 +70,9 @@ Optional properties:
   rather than generate its own timings for HDMI output.
 - clocks: from common clock binding: reference to the CEC clock.
 - clock-names: from common clock binding: must be "cec".
+- reg-names : Names of maps with programmable addresses.
+	It can contain any map needing a non-default address.
+	Possible maps names are : "main", "edid", "cec", "packet"
 
 Required nodes:
 
@@ -88,7 +91,12 @@ Example
 
 	adv7511w: hdmi@39 {
 		compatible = "adi,adv7511w";
-		reg = <39>;
+		/*
+		 * The EDID page will be accessible on address 0x66 on the i2c
+		 * bus. All other maps continue to use their default addresses.
+		 */
+		reg = <0x39 0x66>;
+		reg-names = "main", "edid";
 		interrupt-parent = <&gpio3>;
 		interrupts = <29 IRQ_TYPE_EDGE_FALLING>;
 		clocks = <&cec_clock>;
diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511.h b/drivers/gpu/drm/bridge/adv7511/adv7511.h
index d034b2cb5eee..7d81ce3808e0 100644
--- a/drivers/gpu/drm/bridge/adv7511/adv7511.h
+++ b/drivers/gpu/drm/bridge/adv7511/adv7511.h
@@ -53,8 +53,10 @@
 #define ADV7511_REG_POWER			0x41
 #define ADV7511_REG_STATUS			0x42
 #define ADV7511_REG_EDID_I2C_ADDR		0x43
+#define ADV7511_REG_EDID_I2C_ADDR_DEFAULT	0x3f
 #define ADV7511_REG_PACKET_ENABLE1		0x44
 #define ADV7511_REG_PACKET_I2C_ADDR		0x45
+#define ADV7511_REG_PACKET_I2C_ADDR_DEFAULT	0x38
 #define ADV7511_REG_DSD_ENABLE			0x46
 #define ADV7511_REG_VIDEO_INPUT_CFG2		0x48
 #define ADV7511_REG_INFOFRAME_UPDATE		0x4a
@@ -89,6 +91,7 @@
 #define ADV7511_REG_TMDS_CLOCK_INV		0xde
 #define ADV7511_REG_ARC_CTRL			0xdf
 #define ADV7511_REG_CEC_I2C_ADDR		0xe1
+#define ADV7511_REG_CEC_I2C_ADDR_DEFAULT	0x3c
 #define ADV7511_REG_CEC_CTRL			0xe2
 #define ADV7511_REG_CHIP_ID_HIGH		0xf5
 #define ADV7511_REG_CHIP_ID_LOW			0xf6
@@ -322,6 +325,7 @@ struct adv7511 {
 	struct i2c_client *i2c_main;
 	struct i2c_client *i2c_edid;
 	struct i2c_client *i2c_cec;
+	struct i2c_client *i2c_packet;
 
 	struct regmap *regmap;
 	struct regmap *regmap_cec;
diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
index efa29db5fc2b..7ec33837752b 100644
--- a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
+++ b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
@@ -969,8 +969,8 @@ static int adv7511_init_cec_regmap(struct adv7511 *adv)
 {
 	int ret;
 
-	adv->i2c_cec = i2c_new_dummy(adv->i2c_main->adapter,
-				     adv->i2c_main->addr - 1);
+	adv->i2c_cec = i2c_new_secondary_device(adv->i2c_main, "cec",
+					ADV7511_REG_CEC_I2C_ADDR_DEFAULT);
 	if (!adv->i2c_cec)
 		return -ENOMEM;
 	i2c_set_clientdata(adv->i2c_cec, adv);
@@ -1082,8 +1082,6 @@ static int adv7511_probe(struct i2c_client *i2c, const struct i2c_device_id *id)
 	struct adv7511_link_config link_config;
 	struct adv7511 *adv7511;
 	struct device *dev = &i2c->dev;
-	unsigned int main_i2c_addr = i2c->addr << 1;
-	unsigned int edid_i2c_addr = main_i2c_addr + 4;
 	unsigned int val;
 	int ret;
 
@@ -1153,24 +1151,35 @@ static int adv7511_probe(struct i2c_client *i2c, const struct i2c_device_id *id)
 	if (ret)
 		goto uninit_regulators;
 
-	regmap_write(adv7511->regmap, ADV7511_REG_EDID_I2C_ADDR, edid_i2c_addr);
-	regmap_write(adv7511->regmap, ADV7511_REG_PACKET_I2C_ADDR,
-		     main_i2c_addr - 0xa);
-	regmap_write(adv7511->regmap, ADV7511_REG_CEC_I2C_ADDR,
-		     main_i2c_addr - 2);
-
 	adv7511_packet_disable(adv7511, 0xffff);
 
-	adv7511->i2c_edid = i2c_new_dummy(i2c->adapter, edid_i2c_addr >> 1);
+	adv7511->i2c_edid = i2c_new_secondary_device(i2c, "edid",
+					ADV7511_REG_EDID_I2C_ADDR_DEFAULT);
 	if (!adv7511->i2c_edid) {
 		ret = -ENOMEM;
 		goto uninit_regulators;
 	}
 
+	regmap_write(adv7511->regmap, ADV7511_REG_EDID_I2C_ADDR,
+		     adv7511->i2c_edid->addr << 1);
+
 	ret = adv7511_init_cec_regmap(adv7511);
 	if (ret)
 		goto err_i2c_unregister_edid;
 
+	regmap_write(adv7511->regmap, ADV7511_REG_CEC_I2C_ADDR,
+		     adv7511->i2c_cec->addr << 1);
+
+	adv7511->i2c_packet = i2c_new_secondary_device(i2c, "packet",
+					ADV7511_REG_PACKET_I2C_ADDR_DEFAULT);
+	if (!adv7511->i2c_packet) {
+		ret = -ENOMEM;
+		goto err_unregister_cec;
+	}
+
+	regmap_write(adv7511->regmap, ADV7511_REG_PACKET_I2C_ADDR,
+		     adv7511->i2c_packet->addr << 1);
+
 	INIT_WORK(&adv7511->hpd_work, adv7511_hpd_work);
 
 	if (i2c->irq) {
@@ -1181,7 +1190,7 @@ static int adv7511_probe(struct i2c_client *i2c, const struct i2c_device_id *id)
 						IRQF_ONESHOT, dev_name(dev),
 						adv7511);
 		if (ret)
-			goto err_unregister_cec;
+			goto err_unregister_packet;
 	}
 
 	adv7511_power_off(adv7511);
@@ -1203,6 +1212,8 @@ static int adv7511_probe(struct i2c_client *i2c, const struct i2c_device_id *id)
 	adv7511_audio_init(dev, adv7511);
 	return 0;
 
+err_unregister_packet:
+	i2c_unregister_device(adv7511->i2c_packet);
 err_unregister_cec:
 	i2c_unregister_device(adv7511->i2c_cec);
 	if (adv7511->cec_clk)
@@ -1234,6 +1245,7 @@ static int adv7511_remove(struct i2c_client *i2c)
 	cec_unregister_adapter(adv7511->cec_adap);
 
 	i2c_unregister_device(adv7511->i2c_edid);
+	i2c_unregister_device(adv7511->i2c_packet);
 
 	return 0;
 }
-- 
2.7.4

Re: [PATCH 2/2] drm: adv7511: Add support for i2c_new_secondary_device

[Lars-Peter Clausen]

On 01/22/2018 01:50 PM, Kieran Bingham wrote:
> The ADV7511 has four 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.
> 
> 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 <kieran.bingham@ideasonboard.com>

I've been working on the same thing, but you've beat me to it! Patch looks
mostly OK, but I think you are missing this piece:

https://github.com/analogdevicesinc/linux/commit/ba9b57507cb78724a606eb24104e22fea942437d#diff-2cf1828c644e351adefabe9509410400L553

> ---
>  .../bindings/display/bridge/adi,adv7511.txt        | 10 +++++-
>  drivers/gpu/drm/bridge/adv7511/adv7511.h           |  4 +++
>  drivers/gpu/drm/bridge/adv7511/adv7511_drv.c       | 36 ++++++++++++++--------
>  3 files changed, 37 insertions(+), 13 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/display/bridge/adi,adv7511.txt b/Documentation/devicetree/bindings/display/bridge/adi,adv7511.txt
> index 0047b1394c70..f6bb9f6d3f48 100644
> --- a/Documentation/devicetree/bindings/display/bridge/adi,adv7511.txt
> +++ b/Documentation/devicetree/bindings/display/bridge/adi,adv7511.txt
> @@ -70,6 +70,9 @@ Optional properties:
>    rather than generate its own timings for HDMI output.
>  - clocks: from common clock binding: reference to the CEC clock.
>  - clock-names: from common clock binding: must be "cec".
> +- reg-names : Names of maps with programmable addresses.
> +	It can contain any map needing a non-default address.
> +	Possible maps names are : "main", "edid", "cec", "packet"
>  
>  Required nodes:
>  
> @@ -88,7 +91,12 @@ Example
>  
>  	adv7511w: hdmi@39 {
>  		compatible = "adi,adv7511w";
> -		reg = <39>;
> +		/*
> +		 * The EDID page will be accessible on address 0x66 on the i2c
> +		 * bus. All other maps continue to use their default addresses.
> +		 */
> +		reg = <0x39 0x66>;
> +		reg-names = "main", "edid";
>  		interrupt-parent = <&gpio3>;
>  		interrupts = <29 IRQ_TYPE_EDGE_FALLING>;
>  		clocks = <&cec_clock>;
> diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511.h b/drivers/gpu/drm/bridge/adv7511/adv7511.h
> index d034b2cb5eee..7d81ce3808e0 100644
> --- a/drivers/gpu/drm/bridge/adv7511/adv7511.h
> +++ b/drivers/gpu/drm/bridge/adv7511/adv7511.h
> @@ -53,8 +53,10 @@
>  #define ADV7511_REG_POWER			0x41
>  #define ADV7511_REG_STATUS			0x42
>  #define ADV7511_REG_EDID_I2C_ADDR		0x43
> +#define ADV7511_REG_EDID_I2C_ADDR_DEFAULT	0x3f
>  #define ADV7511_REG_PACKET_ENABLE1		0x44
>  #define ADV7511_REG_PACKET_I2C_ADDR		0x45
> +#define ADV7511_REG_PACKET_I2C_ADDR_DEFAULT	0x38
>  #define ADV7511_REG_DSD_ENABLE			0x46
>  #define ADV7511_REG_VIDEO_INPUT_CFG2		0x48
>  #define ADV7511_REG_INFOFRAME_UPDATE		0x4a
> @@ -89,6 +91,7 @@
>  #define ADV7511_REG_TMDS_CLOCK_INV		0xde
>  #define ADV7511_REG_ARC_CTRL			0xdf
>  #define ADV7511_REG_CEC_I2C_ADDR		0xe1
> +#define ADV7511_REG_CEC_I2C_ADDR_DEFAULT	0x3c
>  #define ADV7511_REG_CEC_CTRL			0xe2
>  #define ADV7511_REG_CHIP_ID_HIGH		0xf5
>  #define ADV7511_REG_CHIP_ID_LOW			0xf6
> @@ -322,6 +325,7 @@ struct adv7511 {
>  	struct i2c_client *i2c_main;
>  	struct i2c_client *i2c_edid;
>  	struct i2c_client *i2c_cec;
> +	struct i2c_client *i2c_packet;
>  
>  	struct regmap *regmap;
>  	struct regmap *regmap_cec;
> diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
> index efa29db5fc2b..7ec33837752b 100644
> --- a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
> +++ b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
> @@ -969,8 +969,8 @@ static int adv7511_init_cec_regmap(struct adv7511 *adv)
>  {
>  	int ret;
>  
> -	adv->i2c_cec = i2c_new_dummy(adv->i2c_main->adapter,
> -				     adv->i2c_main->addr - 1);
> +	adv->i2c_cec = i2c_new_secondary_device(adv->i2c_main, "cec",
> +					ADV7511_REG_CEC_I2C_ADDR_DEFAULT);
>  	if (!adv->i2c_cec)
>  		return -ENOMEM;
>  	i2c_set_clientdata(adv->i2c_cec, adv);
> @@ -1082,8 +1082,6 @@ static int adv7511_probe(struct i2c_client *i2c, const struct i2c_device_id *id)
>  	struct adv7511_link_config link_config;
>  	struct adv7511 *adv7511;
>  	struct device *dev = &i2c->dev;
> -	unsigned int main_i2c_addr = i2c->addr << 1;
> -	unsigned int edid_i2c_addr = main_i2c_addr + 4;
>  	unsigned int val;
>  	int ret;
>  
> @@ -1153,24 +1151,35 @@ static int adv7511_probe(struct i2c_client *i2c, const struct i2c_device_id *id)
>  	if (ret)
>  		goto uninit_regulators;
>  
> -	regmap_write(adv7511->regmap, ADV7511_REG_EDID_I2C_ADDR, edid_i2c_addr);
> -	regmap_write(adv7511->regmap, ADV7511_REG_PACKET_I2C_ADDR,
> -		     main_i2c_addr - 0xa);
> -	regmap_write(adv7511->regmap, ADV7511_REG_CEC_I2C_ADDR,
> -		     main_i2c_addr - 2);
> -
>  	adv7511_packet_disable(adv7511, 0xffff);
>  
> -	adv7511->i2c_edid = i2c_new_dummy(i2c->adapter, edid_i2c_addr >> 1);
> +	adv7511->i2c_edid = i2c_new_secondary_device(i2c, "edid",
> +					ADV7511_REG_EDID_I2C_ADDR_DEFAULT);
>  	if (!adv7511->i2c_edid) {
>  		ret = -ENOMEM;
>  		goto uninit_regulators;
>  	}
>  
> +	regmap_write(adv7511->regmap, ADV7511_REG_EDID_I2C_ADDR,
> +		     adv7511->i2c_edid->addr << 1);
> +
>  	ret = adv7511_init_cec_regmap(adv7511);
>  	if (ret)
>  		goto err_i2c_unregister_edid;
>  
> +	regmap_write(adv7511->regmap, ADV7511_REG_CEC_I2C_ADDR,
> +		     adv7511->i2c_cec->addr << 1);
> +
> +	adv7511->i2c_packet = i2c_new_secondary_device(i2c, "packet",
> +					ADV7511_REG_PACKET_I2C_ADDR_DEFAULT);
> +	if (!adv7511->i2c_packet) {
> +		ret = -ENOMEM;
> +		goto err_unregister_cec;
> +	}
> +
> +	regmap_write(adv7511->regmap, ADV7511_REG_PACKET_I2C_ADDR,
> +		     adv7511->i2c_packet->addr << 1);
> +
>  	INIT_WORK(&adv7511->hpd_work, adv7511_hpd_work);
>  
>  	if (i2c->irq) {
> @@ -1181,7 +1190,7 @@ static int adv7511_probe(struct i2c_client *i2c, const struct i2c_device_id *id)
>  						IRQF_ONESHOT, dev_name(dev),
>  						adv7511);
>  		if (ret)
> -			goto err_unregister_cec;
> +			goto err_unregister_packet;
>  	}
>  
>  	adv7511_power_off(adv7511);
> @@ -1203,6 +1212,8 @@ static int adv7511_probe(struct i2c_client *i2c, const struct i2c_device_id *id)
>  	adv7511_audio_init(dev, adv7511);
>  	return 0;
>  
> +err_unregister_packet:
> +	i2c_unregister_device(adv7511->i2c_packet);
>  err_unregister_cec:
>  	i2c_unregister_device(adv7511->i2c_cec);
>  	if (adv7511->cec_clk)
> @@ -1234,6 +1245,7 @@ static int adv7511_remove(struct i2c_client *i2c)
>  	cec_unregister_adapter(adv7511->cec_adap);
>  
>  	i2c_unregister_device(adv7511->i2c_edid);
> +	i2c_unregister_device(adv7511->i2c_packet);
>  
>  	return 0;
>  }
> 

Re: [PATCH 2/2] drm: adv7511: Add support for i2c_new_secondary_device

[Kieran Bingham]

On 22/01/18 13:00, Lars-Peter Clausen wrote:
> On 01/22/2018 01:50 PM, Kieran Bingham wrote:
>> The ADV7511 has four 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.
>>
>> 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 <kieran.bingham@ideasonboard.com>
> 
> I've been working on the same thing, but you've beat me to it! Patch looks
> mostly OK, but I think you are missing this piece:
> 
> https://github.com/analogdevicesinc/linux/commit/ba9b57507cb78724a606eb24104e22fea942437d#diff-2cf1828c644e351adefabe9509410400L553

Ah yes - Thanks, - you're correct - I had missed that bit.

Added locally for a v2.
--
Kieran

>> ---
>>  .../bindings/display/bridge/adi,adv7511.txt        | 10 +++++-
>>  drivers/gpu/drm/bridge/adv7511/adv7511.h           |  4 +++
>>  drivers/gpu/drm/bridge/adv7511/adv7511_drv.c       | 36 ++++++++++++++--------
>>  3 files changed, 37 insertions(+), 13 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/display/bridge/adi,adv7511.txt b/Documentation/devicetree/bindings/display/bridge/adi,adv7511.txt
>> index 0047b1394c70..f6bb9f6d3f48 100644
>> --- a/Documentation/devicetree/bindings/display/bridge/adi,adv7511.txt
>> +++ b/Documentation/devicetree/bindings/display/bridge/adi,adv7511.txt
>> @@ -70,6 +70,9 @@ Optional properties:
>>    rather than generate its own timings for HDMI output.
>>  - clocks: from common clock binding: reference to the CEC clock.
>>  - clock-names: from common clock binding: must be "cec".
>> +- reg-names : Names of maps with programmable addresses.
>> +	It can contain any map needing a non-default address.
>> +	Possible maps names are : "main", "edid", "cec", "packet"
>>  
>>  Required nodes:
>>  
>> @@ -88,7 +91,12 @@ Example
>>  
>>  	adv7511w: hdmi@39 {
>>  		compatible = "adi,adv7511w";
>> -		reg = <39>;
>> +		/*
>> +		 * The EDID page will be accessible on address 0x66 on the i2c
>> +		 * bus. All other maps continue to use their default addresses.
>> +		 */
>> +		reg = <0x39 0x66>;
>> +		reg-names = "main", "edid";
>>  		interrupt-parent = <&gpio3>;
>>  		interrupts = <29 IRQ_TYPE_EDGE_FALLING>;
>>  		clocks = <&cec_clock>;
>> diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511.h b/drivers/gpu/drm/bridge/adv7511/adv7511.h
>> index d034b2cb5eee..7d81ce3808e0 100644
>> --- a/drivers/gpu/drm/bridge/adv7511/adv7511.h
>> +++ b/drivers/gpu/drm/bridge/adv7511/adv7511.h
>> @@ -53,8 +53,10 @@
>>  #define ADV7511_REG_POWER			0x41
>>  #define ADV7511_REG_STATUS			0x42
>>  #define ADV7511_REG_EDID_I2C_ADDR		0x43
>> +#define ADV7511_REG_EDID_I2C_ADDR_DEFAULT	0x3f
>>  #define ADV7511_REG_PACKET_ENABLE1		0x44
>>  #define ADV7511_REG_PACKET_I2C_ADDR		0x45
>> +#define ADV7511_REG_PACKET_I2C_ADDR_DEFAULT	0x38
>>  #define ADV7511_REG_DSD_ENABLE			0x46
>>  #define ADV7511_REG_VIDEO_INPUT_CFG2		0x48
>>  #define ADV7511_REG_INFOFRAME_UPDATE		0x4a
>> @@ -89,6 +91,7 @@
>>  #define ADV7511_REG_TMDS_CLOCK_INV		0xde
>>  #define ADV7511_REG_ARC_CTRL			0xdf
>>  #define ADV7511_REG_CEC_I2C_ADDR		0xe1
>> +#define ADV7511_REG_CEC_I2C_ADDR_DEFAULT	0x3c
>>  #define ADV7511_REG_CEC_CTRL			0xe2
>>  #define ADV7511_REG_CHIP_ID_HIGH		0xf5
>>  #define ADV7511_REG_CHIP_ID_LOW			0xf6
>> @@ -322,6 +325,7 @@ struct adv7511 {
>>  	struct i2c_client *i2c_main;
>>  	struct i2c_client *i2c_edid;
>>  	struct i2c_client *i2c_cec;
>> +	struct i2c_client *i2c_packet;
>>  
>>  	struct regmap *regmap;
>>  	struct regmap *regmap_cec;
>> diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
>> index efa29db5fc2b..7ec33837752b 100644
>> --- a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
>> +++ b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
>> @@ -969,8 +969,8 @@ static int adv7511_init_cec_regmap(struct adv7511 *adv)
>>  {
>>  	int ret;
>>  
>> -	adv->i2c_cec = i2c_new_dummy(adv->i2c_main->adapter,
>> -				     adv->i2c_main->addr - 1);
>> +	adv->i2c_cec = i2c_new_secondary_device(adv->i2c_main, "cec",
>> +					ADV7511_REG_CEC_I2C_ADDR_DEFAULT);
>>  	if (!adv->i2c_cec)
>>  		return -ENOMEM;
>>  	i2c_set_clientdata(adv->i2c_cec, adv);
>> @@ -1082,8 +1082,6 @@ static int adv7511_probe(struct i2c_client *i2c, const struct i2c_device_id *id)
>>  	struct adv7511_link_config link_config;
>>  	struct adv7511 *adv7511;
>>  	struct device *dev = &i2c->dev;
>> -	unsigned int main_i2c_addr = i2c->addr << 1;
>> -	unsigned int edid_i2c_addr = main_i2c_addr + 4;
>>  	unsigned int val;
>>  	int ret;
>>  
>> @@ -1153,24 +1151,35 @@ static int adv7511_probe(struct i2c_client *i2c, const struct i2c_device_id *id)
>>  	if (ret)
>>  		goto uninit_regulators;
>>  
>> -	regmap_write(adv7511->regmap, ADV7511_REG_EDID_I2C_ADDR, edid_i2c_addr);
>> -	regmap_write(adv7511->regmap, ADV7511_REG_PACKET_I2C_ADDR,
>> -		     main_i2c_addr - 0xa);
>> -	regmap_write(adv7511->regmap, ADV7511_REG_CEC_I2C_ADDR,
>> -		     main_i2c_addr - 2);
>> -
>>  	adv7511_packet_disable(adv7511, 0xffff);
>>  
>> -	adv7511->i2c_edid = i2c_new_dummy(i2c->adapter, edid_i2c_addr >> 1);
>> +	adv7511->i2c_edid = i2c_new_secondary_device(i2c, "edid",
>> +					ADV7511_REG_EDID_I2C_ADDR_DEFAULT);
>>  	if (!adv7511->i2c_edid) {
>>  		ret = -ENOMEM;
>>  		goto uninit_regulators;
>>  	}
>>  
>> +	regmap_write(adv7511->regmap, ADV7511_REG_EDID_I2C_ADDR,
>> +		     adv7511->i2c_edid->addr << 1);
>> +
>>  	ret = adv7511_init_cec_regmap(adv7511);
>>  	if (ret)
>>  		goto err_i2c_unregister_edid;
>>  
>> +	regmap_write(adv7511->regmap, ADV7511_REG_CEC_I2C_ADDR,
>> +		     adv7511->i2c_cec->addr << 1);
>> +
>> +	adv7511->i2c_packet = i2c_new_secondary_device(i2c, "packet",
>> +					ADV7511_REG_PACKET_I2C_ADDR_DEFAULT);
>> +	if (!adv7511->i2c_packet) {
>> +		ret = -ENOMEM;
>> +		goto err_unregister_cec;
>> +	}
>> +
>> +	regmap_write(adv7511->regmap, ADV7511_REG_PACKET_I2C_ADDR,
>> +		     adv7511->i2c_packet->addr << 1);
>> +
>>  	INIT_WORK(&adv7511->hpd_work, adv7511_hpd_work);
>>  
>>  	if (i2c->irq) {
>> @@ -1181,7 +1190,7 @@ static int adv7511_probe(struct i2c_client *i2c, const struct i2c_device_id *id)
>>  						IRQF_ONESHOT, dev_name(dev),
>>  						adv7511);
>>  		if (ret)
>> -			goto err_unregister_cec;
>> +			goto err_unregister_packet;
>>  	}
>>  
>>  	adv7511_power_off(adv7511);
>> @@ -1203,6 +1212,8 @@ static int adv7511_probe(struct i2c_client *i2c, const struct i2c_device_id *id)
>>  	adv7511_audio_init(dev, adv7511);
>>  	return 0;
>>  
>> +err_unregister_packet:
>> +	i2c_unregister_device(adv7511->i2c_packet);
>>  err_unregister_cec:
>>  	i2c_unregister_device(adv7511->i2c_cec);
>>  	if (adv7511->cec_clk)
>> @@ -1234,6 +1245,7 @@ static int adv7511_remove(struct i2c_client *i2c)
>>  	cec_unregister_adapter(adv7511->cec_adap);
>>  
>>  	i2c_unregister_device(adv7511->i2c_edid);
>> +	i2c_unregister_device(adv7511->i2c_packet);
>>  
>>  	return 0;
>>  }
>>
> 

Re: [PATCH 2/2] drm: adv7511: Add support for i2c_new_secondary_device

[Archit Taneja]

Hi,

On 01/22/2018 06:20 PM, Kieran Bingham wrote:
> The ADV7511 has four 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.
> 
> 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 <kieran.bingham-ryLnwIuWjnjg/C1BVhZhaw@public.gmane.org>
> ---
>   .../bindings/display/bridge/adi,adv7511.txt        | 10 +++++-
>   drivers/gpu/drm/bridge/adv7511/adv7511.h           |  4 +++
>   drivers/gpu/drm/bridge/adv7511/adv7511_drv.c       | 36 ++++++++++++++--------
>   3 files changed, 37 insertions(+), 13 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/display/bridge/adi,adv7511.txt b/Documentation/devicetree/bindings/display/bridge/adi,adv7511.txt
> index 0047b1394c70..f6bb9f6d3f48 100644
> --- a/Documentation/devicetree/bindings/display/bridge/adi,adv7511.txt
> +++ b/Documentation/devicetree/bindings/display/bridge/adi,adv7511.txt
> @@ -70,6 +70,9 @@ Optional properties:
>     rather than generate its own timings for HDMI output.
>   - clocks: from common clock binding: reference to the CEC clock.
>   - clock-names: from common clock binding: must be "cec".
> +- reg-names : Names of maps with programmable addresses.
> +	It can contain any map needing a non-default address.
> +	Possible maps names are : "main", "edid", "cec", "packet"
>   
>   Required nodes:
>   
> @@ -88,7 +91,12 @@ Example
>   
>   	adv7511w: hdmi@39 {
>   		compatible = "adi,adv7511w";
> -		reg = <39>;
> +		/*
> +		 * The EDID page will be accessible on address 0x66 on the i2c
> +		 * bus. All other maps continue to use their default addresses.
> +		 */
> +		reg = <0x39 0x66>;
> +		reg-names = "main", "edid";
>   		interrupt-parent = <&gpio3>;
>   		interrupts = <29 IRQ_TYPE_EDGE_FALLING>;
>   		clocks = <&cec_clock>;
> diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511.h b/drivers/gpu/drm/bridge/adv7511/adv7511.h
> index d034b2cb5eee..7d81ce3808e0 100644
> --- a/drivers/gpu/drm/bridge/adv7511/adv7511.h
> +++ b/drivers/gpu/drm/bridge/adv7511/adv7511.h
> @@ -53,8 +53,10 @@
>   #define ADV7511_REG_POWER			0x41
>   #define ADV7511_REG_STATUS			0x42
>   #define ADV7511_REG_EDID_I2C_ADDR		0x43
> +#define ADV7511_REG_EDID_I2C_ADDR_DEFAULT	0x3f
>   #define ADV7511_REG_PACKET_ENABLE1		0x44	
>   #define ADV7511_REG_PACKET_I2C_ADDR		0x45
> +#define ADV7511_REG_PACKET_I2C_ADDR_DEFAULT	0x38
>   #define ADV7511_REG_DSD_ENABLE			0x46
>   #define ADV7511_REG_VIDEO_INPUT_CFG2		0x48
>   #define ADV7511_REG_INFOFRAME_UPDATE		0x4a
> @@ -89,6 +91,7 @@
>   #define ADV7511_REG_TMDS_CLOCK_INV		0xde
>   #define ADV7511_REG_ARC_CTRL			0xdf
>   #define ADV7511_REG_CEC_I2C_ADDR		0xe1
> +#define ADV7511_REG_CEC_I2C_ADDR_DEFAULT	0x3c

Minor comment: The defines above make look like new register
defines. It would be nice to remove the "REG_" from them, and
place them somewhere after the register definitions.

>   #define ADV7511_REG_CEC_CTRL			0xe2
>   #define ADV7511_REG_CHIP_ID_HIGH		0xf5
>   #define ADV7511_REG_CHIP_ID_LOW			0xf6
> @@ -322,6 +325,7 @@ struct adv7511 {
>   	struct i2c_client *i2c_main;
>   	struct i2c_client *i2c_edid;
>   	struct i2c_client *i2c_cec;
> +	struct i2c_client *i2c_packet;
>   
>   	struct regmap *regmap;
>   	struct regmap *regmap_cec;
> diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
> index efa29db5fc2b..7ec33837752b 100644
> --- a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
> +++ b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
> @@ -969,8 +969,8 @@ static int adv7511_init_cec_regmap(struct adv7511 *adv)
>   {
>   	int ret;
>   
> -	adv->i2c_cec = i2c_new_dummy(adv->i2c_main->adapter,
> -				     adv->i2c_main->addr - 1);

This patch avoids deriving the CEC/EDID map addresses from the main map. I think this would break 
what the original patch tried to do:

d25a4cbba4b9da7c2d674b2f8ecf84af1b24988e
"drm/bridge: adv7511: add support for the 2nd chip"

Maybe the default macros can be a function of the main address?


> +	adv->i2c_cec = i2c_new_secondary_device(adv->i2c_main, "cec",
> +					ADV7511_REG_CEC_I2C_ADDR_DEFAULT);

Also, I'm a bit unclear on the default address values. For example, previously, the CEC
address was calculated as "adv->i2c_main->addr - 1", which is 0x38. The new CEC_I2C_ADDR_DEFAULT
define sets it to 0x3c.

Thanks,
Archit

>   	if (!adv->i2c_cec)
>   		return -ENOMEM;
>   	i2c_set_clientdata(adv->i2c_cec, adv);
> @@ -1082,8 +1082,6 @@ static int adv7511_probe(struct i2c_client *i2c, const struct i2c_device_id *id)
>   	struct adv7511_link_config link_config;
>   	struct adv7511 *adv7511;
>   	struct device *dev = &i2c->dev;
> -	unsigned int main_i2c_addr = i2c->addr << 1;
> -	unsigned int edid_i2c_addr = main_i2c_addr + 4;
>   	unsigned int val;
>   	int ret;
>   
> @@ -1153,24 +1151,35 @@ static int adv7511_probe(struct i2c_client *i2c, const struct i2c_device_id *id)
>   	if (ret)
>   		goto uninit_regulators;
>   
> -	regmap_write(adv7511->regmap, ADV7511_REG_EDID_I2C_ADDR, edid_i2c_addr);
> -	regmap_write(adv7511->regmap, ADV7511_REG_PACKET_I2C_ADDR,
> -		     main_i2c_addr - 0xa);
> -	regmap_write(adv7511->regmap, ADV7511_REG_CEC_I2C_ADDR,
> -		     main_i2c_addr - 2);
> -
>   	adv7511_packet_disable(adv7511, 0xffff);
>   
> -	adv7511->i2c_edid = i2c_new_dummy(i2c->adapter, edid_i2c_addr >> 1);
> +	adv7511->i2c_edid = i2c_new_secondary_device(i2c, "edid",
> +					ADV7511_REG_EDID_I2C_ADDR_DEFAULT);
>   	if (!adv7511->i2c_edid) {
>   		ret = -ENOMEM;
>   		goto uninit_regulators;
>   	}
>   
> +	regmap_write(adv7511->regmap, ADV7511_REG_EDID_I2C_ADDR,
> +		     adv7511->i2c_edid->addr << 1);
> +
>   	ret = adv7511_init_cec_regmap(adv7511);
>   	if (ret)
>   		goto err_i2c_unregister_edid;
>   
> +	regmap_write(adv7511->regmap, ADV7511_REG_CEC_I2C_ADDR,
> +		     adv7511->i2c_cec->addr << 1);
> +
> +	adv7511->i2c_packet = i2c_new_secondary_device(i2c, "packet",
> +					ADV7511_REG_PACKET_I2C_ADDR_DEFAULT);
> +	if (!adv7511->i2c_packet) {
> +		ret = -ENOMEM;
> +		goto err_unregister_cec;
> +	}
> +
> +	regmap_write(adv7511->regmap, ADV7511_REG_PACKET_I2C_ADDR,
> +		     adv7511->i2c_packet->addr << 1);
> +
>   	INIT_WORK(&adv7511->hpd_work, adv7511_hpd_work);
>   
>   	if (i2c->irq) {
> @@ -1181,7 +1190,7 @@ static int adv7511_probe(struct i2c_client *i2c, const struct i2c_device_id *id)
>   						IRQF_ONESHOT, dev_name(dev),
>   						adv7511);
>   		if (ret)
> -			goto err_unregister_cec;
> +			goto err_unregister_packet;
>   	}
>   
>   	adv7511_power_off(adv7511);
> @@ -1203,6 +1212,8 @@ static int adv7511_probe(struct i2c_client *i2c, const struct i2c_device_id *id)
>   	adv7511_audio_init(dev, adv7511);
>   	return 0;
>   
> +err_unregister_packet:
> +	i2c_unregister_device(adv7511->i2c_packet);
>   err_unregister_cec:
>   	i2c_unregister_device(adv7511->i2c_cec);
>   	if (adv7511->cec_clk)
> @@ -1234,6 +1245,7 @@ static int adv7511_remove(struct i2c_client *i2c)
>   	cec_unregister_adapter(adv7511->cec_adap);
>   
>   	i2c_unregister_device(adv7511->i2c_edid);
> +	i2c_unregister_device(adv7511->i2c_packet);
>   
>   	return 0;
>   }
> 

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
--
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 2/2] drm: adv7511: Add support for i2c_new_secondary_device

[Kieran Bingham]

Hi Archit,

Thank you for your review,

On 29/01/18 04:11, Archit Taneja wrote:
> Hi,
> 
> On 01/22/2018 06:20 PM, Kieran Bingham wrote:
>> The ADV7511 has four 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.
>>
>> 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 <kieran.bingham@ideasonboard.com>
>> ---
>>   .../bindings/display/bridge/adi,adv7511.txt        | 10 +++++-
>>   drivers/gpu/drm/bridge/adv7511/adv7511.h           |  4 +++
>>   drivers/gpu/drm/bridge/adv7511/adv7511_drv.c       | 36 ++++++++++++++--------
>>   3 files changed, 37 insertions(+), 13 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/display/bridge/adi,adv7511.txt
>> b/Documentation/devicetree/bindings/display/bridge/adi,adv7511.txt
>> index 0047b1394c70..f6bb9f6d3f48 100644
>> --- a/Documentation/devicetree/bindings/display/bridge/adi,adv7511.txt
>> +++ b/Documentation/devicetree/bindings/display/bridge/adi,adv7511.txt
>> @@ -70,6 +70,9 @@ Optional properties:
>>     rather than generate its own timings for HDMI output.
>>   - clocks: from common clock binding: reference to the CEC clock.
>>   - clock-names: from common clock binding: must be "cec".
>> +- reg-names : Names of maps with programmable addresses.
>> +    It can contain any map needing a non-default address.
>> +    Possible maps names are : "main", "edid", "cec", "packet"
>>     Required nodes:
>>   @@ -88,7 +91,12 @@ Example
>>         adv7511w: hdmi@39 {
>>           compatible = "adi,adv7511w";
>> -        reg = <39>;
>> +        /*
>> +         * The EDID page will be accessible on address 0x66 on the i2c
>> +         * bus. All other maps continue to use their default addresses.
>> +         */
>> +        reg = <0x39 0x66>;
>> +        reg-names = "main", "edid";
>>           interrupt-parent = <&gpio3>;
>>           interrupts = <29 IRQ_TYPE_EDGE_FALLING>;
>>           clocks = <&cec_clock>;
>> diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511.h
>> b/drivers/gpu/drm/bridge/adv7511/adv7511.h
>> index d034b2cb5eee..7d81ce3808e0 100644
>> --- a/drivers/gpu/drm/bridge/adv7511/adv7511.h
>> +++ b/drivers/gpu/drm/bridge/adv7511/adv7511.h
>> @@ -53,8 +53,10 @@
>>   #define ADV7511_REG_POWER            0x41
>>   #define ADV7511_REG_STATUS            0x42
>>   #define ADV7511_REG_EDID_I2C_ADDR        0x43
>> +#define ADV7511_REG_EDID_I2C_ADDR_DEFAULT    0x3f
>>   #define ADV7511_REG_PACKET_ENABLE1        0x44   
>>   #define ADV7511_REG_PACKET_I2C_ADDR        0x45
>> +#define ADV7511_REG_PACKET_I2C_ADDR_DEFAULT    0x38
>>   #define ADV7511_REG_DSD_ENABLE            0x46
>>   #define ADV7511_REG_VIDEO_INPUT_CFG2        0x48
>>   #define ADV7511_REG_INFOFRAME_UPDATE        0x4a
>> @@ -89,6 +91,7 @@
>>   #define ADV7511_REG_TMDS_CLOCK_INV        0xde
>>   #define ADV7511_REG_ARC_CTRL            0xdf
>>   #define ADV7511_REG_CEC_I2C_ADDR        0xe1
>> +#define ADV7511_REG_CEC_I2C_ADDR_DEFAULT    0x3c
> 
> Minor comment: The defines above make look like new register
> defines. It would be nice to remove the "REG_" from them, and
> place them somewhere after the register definitions.


Sure.


>>   #define ADV7511_REG_CEC_CTRL            0xe2
>>   #define ADV7511_REG_CHIP_ID_HIGH        0xf5
>>   #define ADV7511_REG_CHIP_ID_LOW            0xf6
>> @@ -322,6 +325,7 @@ struct adv7511 {
>>       struct i2c_client *i2c_main;
>>       struct i2c_client *i2c_edid;
>>       struct i2c_client *i2c_cec;
>> +    struct i2c_client *i2c_packet;
>>         struct regmap *regmap;
>>       struct regmap *regmap_cec;
>> diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
>> b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
>> index efa29db5fc2b..7ec33837752b 100644
>> --- a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
>> +++ b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
>> @@ -969,8 +969,8 @@ static int adv7511_init_cec_regmap(struct adv7511 *adv)
>>   {
>>       int ret;
>>   -    adv->i2c_cec = i2c_new_dummy(adv->i2c_main->adapter,
>> -                     adv->i2c_main->addr - 1);
> 
> This patch avoids deriving the CEC/EDID map addresses from the main map. I think
> this would break what the original patch tried to do:

That was intentional.

The ADV7511 data-sheet defines default addresses for these maps.
 (or rather the hardware does)

> 
> d25a4cbba4b9da7c2d674b2f8ecf84af1b24988e
> "drm/bridge: adv7511: add support for the 2nd chip"
> 
> Maybe the default macros can be a function of the main address?

I'm loathed to do that, because then intrinsic knowledge must be known that if I
define a device at address X ... it will also use magic offset A B and C.

IMO - the driver should define the defaults to match the hardware.

Anything else is an override ...


<Welcoming comments, and arguments here>


>> +    adv->i2c_cec = i2c_new_secondary_device(adv->i2c_main, "cec",
>> +                    ADV7511_REG_CEC_I2C_ADDR_DEFAULT);
> 
> Also, I'm a bit unclear on the default address values. For example, previously,
> the CEC
> address was calculated as "adv->i2c_main->addr - 1", which is 0x38. The new
> CEC_I2C_ADDR_DEFAULT
> define sets it to 0x3c.


The registers defined in the programming guide [0] define default addresses
which is set by the hardware (and noteworthy, reset to; at low power mode!)

Page : Register : Default   : Purpose

165  : 0x43     : 0x3f << 1 : EDID Memory Address
166  : 0x45     : 0x38 << 1 : Packet Memory I2C Map Address
181  : 0xE1     : 0x3c << 1 : CEC ID

Actually - I've just seen there's a good table at page 16 :D

The ADV7511 main register map can appear at either 0x39 or 0x3D depending on the
input to a pull-up/down pin.

Thus - that implies that perhaps Sergei must have been working on a device
located at 0x3D, to determine the offset of -1 ... but we have two values for
computing the offset depending on whether you start at 0x39 or 0x3D.

And also - I can't even be sure that I can infer the starting address, as the
offsets for the EDID and Packet do not correlate here.

[0]
http://www.analog.com/media/en/technical-documentation/user-guides/ADV7511_Programming_Guide.pdf

> Thanks,
> Archit
> 
>>       if (!adv->i2c_cec)
>>           return -ENOMEM;
>>       i2c_set_clientdata(adv->i2c_cec, adv);
>> @@ -1082,8 +1082,6 @@ static int adv7511_probe(struct i2c_client *i2c, const
>> struct i2c_device_id *id)
>>       struct adv7511_link_config link_config;
>>       struct adv7511 *adv7511;
>>       struct device *dev = &i2c->dev;
>> -    unsigned int main_i2c_addr = i2c->addr << 1;
>> -    unsigned int edid_i2c_addr = main_i2c_addr + 4;

This computation doesn't correspond to anything except a random (perhaps
platform specific) offset.


>>       unsigned int val;
>>       int ret;
>>   @@ -1153,24 +1151,35 @@ static int adv7511_probe(struct i2c_client *i2c,
>> const struct i2c_device_id *id)
>>       if (ret)
>>           goto uninit_regulators;
>>   -    regmap_write(adv7511->regmap, ADV7511_REG_EDID_I2C_ADDR, edid_i2c_addr);
>> -    regmap_write(adv7511->regmap, ADV7511_REG_PACKET_I2C_ADDR,
>> -             main_i2c_addr - 0xa);


Packet address here is written as an offset of -0x0a, which gives 0x2f or 0x33 ... ?

I think these current offsets are platform specific to a specific platform :)

--
Regards

Kieran


>> -    regmap_write(adv7511->regmap, ADV7511_REG_CEC_I2C_ADDR,
>> -             main_i2c_addr - 2);
>> -
>>       adv7511_packet_disable(adv7511, 0xffff);
>>   -    adv7511->i2c_edid = i2c_new_dummy(i2c->adapter, edid_i2c_addr >> 1);
>> +    adv7511->i2c_edid = i2c_new_secondary_device(i2c, "edid",
>> +                    ADV7511_REG_EDID_I2C_ADDR_DEFAULT);
>>       if (!adv7511->i2c_edid) {
>>           ret = -ENOMEM;
>>           goto uninit_regulators;
>>       }
>>   +    regmap_write(adv7511->regmap, ADV7511_REG_EDID_I2C_ADDR,
>> +             adv7511->i2c_edid->addr << 1);
>> +
>>       ret = adv7511_init_cec_regmap(adv7511);
>>       if (ret)
>>           goto err_i2c_unregister_edid;
>>   +    regmap_write(adv7511->regmap, ADV7511_REG_CEC_I2C_ADDR,
>> +             adv7511->i2c_cec->addr << 1);
>> +
>> +    adv7511->i2c_packet = i2c_new_secondary_device(i2c, "packet",
>> +                    ADV7511_REG_PACKET_I2C_ADDR_DEFAULT);
>> +    if (!adv7511->i2c_packet) {
>> +        ret = -ENOMEM;
>> +        goto err_unregister_cec;
>> +    }
>> +
>> +    regmap_write(adv7511->regmap, ADV7511_REG_PACKET_I2C_ADDR,
>> +             adv7511->i2c_packet->addr << 1);
>> +
>>       INIT_WORK(&adv7511->hpd_work, adv7511_hpd_work);
>>         if (i2c->irq) {
>> @@ -1181,7 +1190,7 @@ static int adv7511_probe(struct i2c_client *i2c, const
>> struct i2c_device_id *id)
>>                           IRQF_ONESHOT, dev_name(dev),
>>                           adv7511);
>>           if (ret)
>> -            goto err_unregister_cec;
>> +            goto err_unregister_packet;
>>       }
>>         adv7511_power_off(adv7511);
>> @@ -1203,6 +1212,8 @@ static int adv7511_probe(struct i2c_client *i2c, const
>> struct i2c_device_id *id)
>>       adv7511_audio_init(dev, adv7511);
>>       return 0;
>>   +err_unregister_packet:
>> +    i2c_unregister_device(adv7511->i2c_packet);
>>   err_unregister_cec:
>>       i2c_unregister_device(adv7511->i2c_cec);
>>       if (adv7511->cec_clk)
>> @@ -1234,6 +1245,7 @@ static int adv7511_remove(struct i2c_client *i2c)
>>       cec_unregister_adapter(adv7511->cec_adap);
>>         i2c_unregister_device(adv7511->i2c_edid);
>> +    i2c_unregister_device(adv7511->i2c_packet);
>>         return 0;
>>   }
>>
> 
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 2/2] drm: adv7511: Add support for i2c_new_secondary_device

[Kieran Bingham]

Hi Archit,

On 07/02/18 12:33, Kieran Bingham wrote:
> Hi Archit,
> 
> Thank you for your review,
> 

<snip>

>>>       unsigned int val;
>>>       int ret;
>>>   @@ -1153,24 +1151,35 @@ static int adv7511_probe(struct i2c_client *i2c,
>>> const struct i2c_device_id *id)
>>>       if (ret)
>>>           goto uninit_regulators;
>>>   -    regmap_write(adv7511->regmap, ADV7511_REG_EDID_I2C_ADDR, edid_i2c_addr);
>>> -    regmap_write(adv7511->regmap, ADV7511_REG_PACKET_I2C_ADDR,
>>> -             main_i2c_addr - 0xa);
> 
> 
> Packet address here is written as an offset of -0x0a, which gives 0x2f or 0x33 ... ?
> 
> I think these current offsets are platform specific to a specific platform :)

I appear to be using platform specific maths specific to a non-conforming
platform or universe :-)

Sorry for the incorrect assertion here. - I mixed up 8bit and 7bit values.

The offsets are valid (when calculated correctly) - however - as per my reply to
Laurent - I believe the patch which determined the offsets was using the wrong
base :).

Also - due to a hardware issue on the ADV7511 - the Wheat (as the only known
user of two chips on the same bus) will need to be updated to ensure the current
assignments don't conflict.

--
Kieran

--
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 2/2] drm: adv7511: Add support for i2c_new_secondary_device

[Laurent Pinchart]

Hi Kieran,

Thank you for the patch.

On Monday, 22 January 2018 14:50:00 EET Kieran Bingham wrote:
> The ADV7511 has four 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.
> 
> 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 <kieran.bingham-ryLnwIuWjnjg/C1BVhZhaw@public.gmane.org>
> ---
>  .../bindings/display/bridge/adi,adv7511.txt        | 10 +++++-

I don't mind personally, but device tree maintainers usually ask for DT 
bindings changes to be split to a separate patch.

>  drivers/gpu/drm/bridge/adv7511/adv7511.h           |  4 +++
>  drivers/gpu/drm/bridge/adv7511/adv7511_drv.c       | 36 ++++++++++++-------
>  3 files changed, 37 insertions(+), 13 deletions(-)
> 
> diff --git
> a/Documentation/devicetree/bindings/display/bridge/adi,adv7511.txt
> b/Documentation/devicetree/bindings/display/bridge/adi,adv7511.txt
> index 0047b1394c70..f6bb9f6d3f48 100644
> --- a/Documentation/devicetree/bindings/display/bridge/adi,adv7511.txt
> +++ b/Documentation/devicetree/bindings/display/bridge/adi,adv7511.txt
> @@ -70,6 +70,9 @@ Optional properties:
>    rather than generate its own timings for HDMI output.
>  - clocks: from common clock binding: reference to the CEC clock.
>  - clock-names: from common clock binding: must be "cec".
> +- reg-names : Names of maps with programmable addresses.
> +	It can contain any map needing a non-default address.
> +	Possible maps names are : "main", "edid", "cec", "packet"

Is the reg-names property (and the additional maps) mandatory or optional ? If 
mandatory you should also update the existing DT sources that use those 
bindings. If optional you should define which I2C addresses will be used when 
the maps are not specified (and in that case I think we should go for the 
addresses listed as default in the datasheet, which correspond to the current 
driver implementation when the main address is 0x3d/0x7a).

You should also update the definition of the reg property that currently just 
states

- reg: I2C slave address

And finally you might want to define the term "map" in this context. Here's a 
proposal (if we make all maps mandatory).

The ADV7511 internal registers are split into four pages exposed through 
different I2C addresses, creating four register maps. The I2C addresses of all 
four maps shall be specified by the reg and reg-names property.

- reg: I2C slave addresses, one per reg-names entry
- reg-names: map names, shall be "main", "edid", "cec", "packet"

>  Required nodes:
>  
> @@ -88,7 +91,12 @@ Example
>  
>  	adv7511w: hdmi@39 {
>  		compatible = "adi,adv7511w";
> -		reg = <39>;
> +		/*
> +		 * The EDID page will be accessible on address 0x66 on the i2c
> +		 * bus. All other maps continue to use their default addresses.
> +		 */
> +		reg = <0x39 0x66>;
> +		reg-names = "main", "edid";
>  		interrupt-parent = <&gpio3>;
>  		interrupts = <29 IRQ_TYPE_EDGE_FALLING>;
>  		clocks = <&cec_clock>;
> diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511.h
> b/drivers/gpu/drm/bridge/adv7511/adv7511.h
> index d034b2cb5eee..7d81ce3808e0 100644
> --- a/drivers/gpu/drm/bridge/adv7511/adv7511.h
> +++ b/drivers/gpu/drm/bridge/adv7511/adv7511.h
> @@ -53,8 +53,10 @@
>  #define ADV7511_REG_POWER			0x41
>  #define ADV7511_REG_STATUS			0x42
>  #define ADV7511_REG_EDID_I2C_ADDR		0x43
> +#define ADV7511_REG_EDID_I2C_ADDR_DEFAULT	0x3f
>  #define ADV7511_REG_PACKET_ENABLE1		0x44
>  #define ADV7511_REG_PACKET_I2C_ADDR		0x45
> +#define ADV7511_REG_PACKET_I2C_ADDR_DEFAULT	0x38
>  #define ADV7511_REG_DSD_ENABLE			0x46
>  #define ADV7511_REG_VIDEO_INPUT_CFG2		0x48
>  #define ADV7511_REG_INFOFRAME_UPDATE		0x4a
> @@ -89,6 +91,7 @@
>  #define ADV7511_REG_TMDS_CLOCK_INV		0xde
>  #define ADV7511_REG_ARC_CTRL			0xdf
>  #define ADV7511_REG_CEC_I2C_ADDR		0xe1
> +#define ADV7511_REG_CEC_I2C_ADDR_DEFAULT	0x3c
>  #define ADV7511_REG_CEC_CTRL			0xe2
>  #define ADV7511_REG_CHIP_ID_HIGH		0xf5
>  #define ADV7511_REG_CHIP_ID_LOW			0xf6
> @@ -322,6 +325,7 @@ struct adv7511 {
>  	struct i2c_client *i2c_main;
>  	struct i2c_client *i2c_edid;
>  	struct i2c_client *i2c_cec;
> +	struct i2c_client *i2c_packet;
>  
>  	struct regmap *regmap;
>  	struct regmap *regmap_cec;
> diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
> b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
> index efa29db5fc2b..7ec33837752b 100644
> --- a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
> +++ b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
> @@ -969,8 +969,8 @@ static int adv7511_init_cec_regmap(struct adv7511 *adv)
> {
>  	int ret;
>  
> -	adv->i2c_cec = i2c_new_dummy(adv->i2c_main->adapter,
> -				     adv->i2c_main->addr - 1);
> +	adv->i2c_cec = i2c_new_secondary_device(adv->i2c_main, "cec",
> +					ADV7511_REG_CEC_I2C_ADDR_DEFAULT);
>  	if (!adv->i2c_cec)
>  		return -ENOMEM;
>  	i2c_set_clientdata(adv->i2c_cec, adv);
> @@ -1082,8 +1082,6 @@ static int adv7511_probe(struct i2c_client *i2c, const
> struct i2c_device_id *id)
>  	struct adv7511_link_config link_config;
>  	struct adv7511 *adv7511;
>  	struct device *dev = &i2c->dev;
> -	unsigned int main_i2c_addr = i2c->addr << 1;
> -	unsigned int edid_i2c_addr = main_i2c_addr + 4;
>  	unsigned int val;
>  	int ret;
>  
> @@ -1153,24 +1151,35 @@ static int adv7511_probe(struct i2c_client *i2c,
> const struct i2c_device_id *id)
>  	if (ret)
>  		goto uninit_regulators;
>  
> -	regmap_write(adv7511->regmap, ADV7511_REG_EDID_I2C_ADDR, edid_i2c_addr);
> -	regmap_write(adv7511->regmap, ADV7511_REG_PACKET_I2C_ADDR,
> -		     main_i2c_addr - 0xa);
> -	regmap_write(adv7511->regmap, ADV7511_REG_CEC_I2C_ADDR,
> -		     main_i2c_addr - 2);
> -
>  	adv7511_packet_disable(adv7511, 0xffff);
>  
> -	adv7511->i2c_edid = i2c_new_dummy(i2c->adapter, edid_i2c_addr >> 1);
> +	adv7511->i2c_edid = i2c_new_secondary_device(i2c, "edid",
> +					ADV7511_REG_EDID_I2C_ADDR_DEFAULT);
>  	if (!adv7511->i2c_edid) {
>  		ret = -ENOMEM;

I wonder if this is the right error code. Maybe -EINVAL ? In most cases errors 
will be caused by invalid addresses (out of memory and device_register() 
failures can happen too but should be less common).

It would be nice if i2c_new_secondary_device() returned an ERR_PTR, but that's 
out of scope.

>  		goto uninit_regulators;
>  	}
>  
> +	regmap_write(adv7511->regmap, ADV7511_REG_EDID_I2C_ADDR,
> +		     adv7511->i2c_edid->addr << 1);
> +
>  	ret = adv7511_init_cec_regmap(adv7511);
>  	if (ret)
>  		goto err_i2c_unregister_edid;
>  
> +	regmap_write(adv7511->regmap, ADV7511_REG_CEC_I2C_ADDR,
> +		     adv7511->i2c_cec->addr << 1);
> +
> +	adv7511->i2c_packet = i2c_new_secondary_device(i2c, "packet",
> +					ADV7511_REG_PACKET_I2C_ADDR_DEFAULT);
> +	if (!adv7511->i2c_packet) {
> +		ret = -ENOMEM;
> +		goto err_unregister_cec;
> +	}
> +
> +	regmap_write(adv7511->regmap, ADV7511_REG_PACKET_I2C_ADDR,
> +		     adv7511->i2c_packet->addr << 1);
> +
>  	INIT_WORK(&adv7511->hpd_work, adv7511_hpd_work);
>  
>  	if (i2c->irq) {
> @@ -1181,7 +1190,7 @@ static int adv7511_probe(struct i2c_client *i2c, const
> struct i2c_device_id *id)
>  						IRQF_ONESHOT, dev_name(dev),
>  						adv7511);
>  		if (ret)
> -			goto err_unregister_cec;
> +			goto err_unregister_packet;
>  	}
>  
>  	adv7511_power_off(adv7511);
> @@ -1203,6 +1212,8 @@ static int adv7511_probe(struct i2c_client *i2c, const
> struct i2c_device_id *id)
>  	adv7511_audio_init(dev, adv7511);
>  	return 0;
>  
> +err_unregister_packet:
> +	i2c_unregister_device(adv7511->i2c_packet);
>  err_unregister_cec:
>  	i2c_unregister_device(adv7511->i2c_cec);
>  	if (adv7511->cec_clk)
> @@ -1234,6 +1245,7 @@ static int adv7511_remove(struct i2c_client *i2c)
>  	cec_unregister_adapter(adv7511->cec_adap);
>  
>  	i2c_unregister_device(adv7511->i2c_edid);
> +	i2c_unregister_device(adv7511->i2c_packet);
>  
>  	return 0;
>  }

-- 
Regards,

Laurent Pinchart

--
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 2/2] drm: adv7511: Add support for i2c_new_secondary_device

[Rob Herring]

On Mon, Jan 29, 2018 at 12:26:00PM +0200, Laurent Pinchart wrote:
> Hi Kieran,
> 
> Thank you for the patch.
> 
> On Monday, 22 January 2018 14:50:00 EET Kieran Bingham wrote:
> > The ADV7511 has four 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.
> > 
> > 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 <kieran.bingham@ideasonboard.com>
> > ---
> >  .../bindings/display/bridge/adi,adv7511.txt        | 10 +++++-
> 
> I don't mind personally, but device tree maintainers usually ask for DT 
> bindings changes to be split to a separate patch.

Or perfect bindings, then I won't ask to split it just for that 
(usually).

> >  drivers/gpu/drm/bridge/adv7511/adv7511.h           |  4 +++
> >  drivers/gpu/drm/bridge/adv7511/adv7511_drv.c       | 36 ++++++++++++-------
> >  3 files changed, 37 insertions(+), 13 deletions(-)
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 2/2] drm: adv7511: Add support for i2c_new_secondary_device

[Kieran Bingham]

Hi Laurent,

On 29/01/18 10:26, Laurent Pinchart wrote:
> Hi Kieran,
> 
> Thank you for the patch.

Thanks for your review,

> On Monday, 22 January 2018 14:50:00 EET Kieran Bingham wrote:
>> The ADV7511 has four 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.
>>
>> 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 <kieran.bingham@ideasonboard.com>
>> ---
>>  .../bindings/display/bridge/adi,adv7511.txt        | 10 +++++-
> 
> I don't mind personally, but device tree maintainers usually ask for DT 
> bindings changes to be split to a separate patch.
> 
>>  drivers/gpu/drm/bridge/adv7511/adv7511.h           |  4 +++
>>  drivers/gpu/drm/bridge/adv7511/adv7511_drv.c       | 36 ++++++++++++-------
>>  3 files changed, 37 insertions(+), 13 deletions(-)
>>
>> diff --git
>> a/Documentation/devicetree/bindings/display/bridge/adi,adv7511.txt
>> b/Documentation/devicetree/bindings/display/bridge/adi,adv7511.txt
>> index 0047b1394c70..f6bb9f6d3f48 100644
>> --- a/Documentation/devicetree/bindings/display/bridge/adi,adv7511.txt
>> +++ b/Documentation/devicetree/bindings/display/bridge/adi,adv7511.txt
>> @@ -70,6 +70,9 @@ Optional properties:
>>    rather than generate its own timings for HDMI output.
>>  - clocks: from common clock binding: reference to the CEC clock.
>>  - clock-names: from common clock binding: must be "cec".
>> +- reg-names : Names of maps with programmable addresses.
>> +	It can contain any map needing a non-default address.
>> +	Possible maps names are : "main", "edid", "cec", "packet"
> 
> Is the reg-names property (and the additional maps) mandatory or optional ? > If mandatory you should also update the existing DT sources that use those
> bindings.

They are currently optional. I do prefer it that way - but perhaps due to an
issue mentioned below we might need to make this driver mandatory ?


> If optional you should define which I2C addresses will be used when 
> the maps are not specified > (and in that case I think we should go for the
> addresses listed as default in the datasheet, which correspond to the current 
> driver implementation when the main address is 0x3d/0x7a).

The current addresses do not correspond to the datasheet, even when the
implementation main address is set to 0x3d.

Thus, in my opinion - they are currently 'wrong' - but perhaps changing them is
considered breakage too.

A particular issue will arise here too - as on this device - when the device is
in low-power mode (after probe, before use) - the maps will respond on their
*hardware default* addresses (the ones implemented in this patch), and thus
consume that address on the I2C bus regardless of their settings in the driver.


> You should also update the definition of the reg property that currently just 
> states
> 
> - reg: I2C slave address
> 
> And finally you might want to define the term "map" in this context. Here's a 
> proposal (if we make all maps mandatory).
> 
> The ADV7511 internal registers are split into four pages exposed through 
> different I2C addresses, creating four register maps. The I2C addresses of all 
> four maps shall be specified by the reg and reg-names property.
> 
> - reg: I2C slave addresses, one per reg-names entry
> - reg-names: map names, shall be "main", "edid", "cec", "packet"
> 
>>  Required nodes:
>>  
>> @@ -88,7 +91,12 @@ Example
>>  
>>  	adv7511w: hdmi@39 {
>>  		compatible = "adi,adv7511w";
>> -		reg = <39>;
>> +		/*
>> +		 * The EDID page will be accessible on address 0x66 on the i2c
>> +		 * bus. All other maps continue to use their default addresses.
>> +		 */
>> +		reg = <0x39 0x66>;
>> +		reg-names = "main", "edid";
>>  		interrupt-parent = <&gpio3>;
>>  		interrupts = <29 IRQ_TYPE_EDGE_FALLING>;
>>  		clocks = <&cec_clock>;
>> diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511.h
>> b/drivers/gpu/drm/bridge/adv7511/adv7511.h
>> index d034b2cb5eee..7d81ce3808e0 100644
>> --- a/drivers/gpu/drm/bridge/adv7511/adv7511.h
>> +++ b/drivers/gpu/drm/bridge/adv7511/adv7511.h
>> @@ -53,8 +53,10 @@
>>  #define ADV7511_REG_POWER			0x41
>>  #define ADV7511_REG_STATUS			0x42
>>  #define ADV7511_REG_EDID_I2C_ADDR		0x43
>> +#define ADV7511_REG_EDID_I2C_ADDR_DEFAULT	0x3f
>>  #define ADV7511_REG_PACKET_ENABLE1		0x44
>>  #define ADV7511_REG_PACKET_I2C_ADDR		0x45
>> +#define ADV7511_REG_PACKET_I2C_ADDR_DEFAULT	0x38
>>  #define ADV7511_REG_DSD_ENABLE			0x46
>>  #define ADV7511_REG_VIDEO_INPUT_CFG2		0x48
>>  #define ADV7511_REG_INFOFRAME_UPDATE		0x4a
>> @@ -89,6 +91,7 @@
>>  #define ADV7511_REG_TMDS_CLOCK_INV		0xde
>>  #define ADV7511_REG_ARC_CTRL			0xdf
>>  #define ADV7511_REG_CEC_I2C_ADDR		0xe1
>> +#define ADV7511_REG_CEC_I2C_ADDR_DEFAULT	0x3c
>>  #define ADV7511_REG_CEC_CTRL			0xe2
>>  #define ADV7511_REG_CHIP_ID_HIGH		0xf5
>>  #define ADV7511_REG_CHIP_ID_LOW			0xf6
>> @@ -322,6 +325,7 @@ struct adv7511 {
>>  	struct i2c_client *i2c_main;
>>  	struct i2c_client *i2c_edid;
>>  	struct i2c_client *i2c_cec;
>> +	struct i2c_client *i2c_packet;
>>  
>>  	struct regmap *regmap;
>>  	struct regmap *regmap_cec;
>> diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
>> b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
>> index efa29db5fc2b..7ec33837752b 100644
>> --- a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
>> +++ b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
>> @@ -969,8 +969,8 @@ static int adv7511_init_cec_regmap(struct adv7511 *adv)
>> {
>>  	int ret;
>>  
>> -	adv->i2c_cec = i2c_new_dummy(adv->i2c_main->adapter,
>> -				     adv->i2c_main->addr - 1);
>> +	adv->i2c_cec = i2c_new_secondary_device(adv->i2c_main, "cec",
>> +					ADV7511_REG_CEC_I2C_ADDR_DEFAULT);
>>  	if (!adv->i2c_cec)
>>  		return -ENOMEM;
>>  	i2c_set_clientdata(adv->i2c_cec, adv);
>> @@ -1082,8 +1082,6 @@ static int adv7511_probe(struct i2c_client *i2c, const
>> struct i2c_device_id *id)
>>  	struct adv7511_link_config link_config;
>>  	struct adv7511 *adv7511;
>>  	struct device *dev = &i2c->dev;
>> -	unsigned int main_i2c_addr = i2c->addr << 1;
>> -	unsigned int edid_i2c_addr = main_i2c_addr + 4;
>>  	unsigned int val;
>>  	int ret;
>>  
>> @@ -1153,24 +1151,35 @@ static int adv7511_probe(struct i2c_client *i2c,
>> const struct i2c_device_id *id)
>>  	if (ret)
>>  		goto uninit_regulators;
>>  
>> -	regmap_write(adv7511->regmap, ADV7511_REG_EDID_I2C_ADDR, edid_i2c_addr);
>> -	regmap_write(adv7511->regmap, ADV7511_REG_PACKET_I2C_ADDR,
>> -		     main_i2c_addr - 0xa);
>> -	regmap_write(adv7511->regmap, ADV7511_REG_CEC_I2C_ADDR,
>> -		     main_i2c_addr - 2);
>> -
>>  	adv7511_packet_disable(adv7511, 0xffff);
>>  
>> -	adv7511->i2c_edid = i2c_new_dummy(i2c->adapter, edid_i2c_addr >> 1);
>> +	adv7511->i2c_edid = i2c_new_secondary_device(i2c, "edid",
>> +					ADV7511_REG_EDID_I2C_ADDR_DEFAULT);
>>  	if (!adv7511->i2c_edid) {
>>  		ret = -ENOMEM;
> 
> I wonder if this is the right error code. Maybe -EINVAL ? In most cases errors 
> will be caused by invalid addresses (out of memory and device_register() 
> failures can happen too but should be less common).

It's arbitrary, as multiple error paths simply return NULL, but I think you are
right - the 'most common' fault here is likely to be an in use address.

-ENOMEM was chosen here to mirror the equivalent code returned from the adv7604
driver.

Either way - -EINVAL does feel better at the moment. Although when mirrored back
to the ADV7604 that becomes a distinct change there.

I don't think that matters though - so I'll apply this fix to both patches.



> 
> It would be nice if i2c_new_secondary_device() returned an ERR_PTR, but that's 
> out of scope.

I was about to say - well I'll add this, but the reality is it might require
updating all users of i2c_new_device, and i2c_new_dummy. Updating
i2c_new_secondary_device() on it's own seems a bit pointless otherwise.

And that is definitely out of scope for the amount of time I currently have :D



>>  		goto uninit_regulators;
>>  	}
>>  
>> +	regmap_write(adv7511->regmap, ADV7511_REG_EDID_I2C_ADDR,
>> +		     adv7511->i2c_edid->addr << 1);
>> +
>>  	ret = adv7511_init_cec_regmap(adv7511);
>>  	if (ret)
>>  		goto err_i2c_unregister_edid;
>>  
>> +	regmap_write(adv7511->regmap, ADV7511_REG_CEC_I2C_ADDR,
>> +		     adv7511->i2c_cec->addr << 1);
>> +
>> +	adv7511->i2c_packet = i2c_new_secondary_device(i2c, "packet",
>> +					ADV7511_REG_PACKET_I2C_ADDR_DEFAULT);
>> +	if (!adv7511->i2c_packet) {
>> +		ret = -ENOMEM;
>> +		goto err_unregister_cec;
>> +	}
>> +
>> +	regmap_write(adv7511->regmap, ADV7511_REG_PACKET_I2C_ADDR,
>> +		     adv7511->i2c_packet->addr << 1);
>> +
>>  	INIT_WORK(&adv7511->hpd_work, adv7511_hpd_work);
>>  
>>  	if (i2c->irq) {
>> @@ -1181,7 +1190,7 @@ static int adv7511_probe(struct i2c_client *i2c, const
>> struct i2c_device_id *id)
>>  						IRQF_ONESHOT, dev_name(dev),
>>  						adv7511);
>>  		if (ret)
>> -			goto err_unregister_cec;
>> +			goto err_unregister_packet;
>>  	}
>>  
>>  	adv7511_power_off(adv7511);
>> @@ -1203,6 +1212,8 @@ static int adv7511_probe(struct i2c_client *i2c, const
>> struct i2c_device_id *id)
>>  	adv7511_audio_init(dev, adv7511);
>>  	return 0;
>>  
>> +err_unregister_packet:
>> +	i2c_unregister_device(adv7511->i2c_packet);
>>  err_unregister_cec:
>>  	i2c_unregister_device(adv7511->i2c_cec);
>>  	if (adv7511->cec_clk)
>> @@ -1234,6 +1245,7 @@ static int adv7511_remove(struct i2c_client *i2c)
>>  	cec_unregister_adapter(adv7511->cec_adap);
>>  
>>  	i2c_unregister_device(adv7511->i2c_edid);
>> +	i2c_unregister_device(adv7511->i2c_packet);
>>  
>>  	return 0;
>>  }
> 

Re: [PATCH 2/2] drm: adv7511: Add support for i2c_new_secondary_device

[Laurent Pinchart]

Hi Kieran,

On Wednesday, 7 February 2018 17:14:09 EET Kieran Bingham wrote:
> On 29/01/18 10:26, Laurent Pinchart wrote:
> > On Monday, 22 January 2018 14:50:00 EET Kieran Bingham wrote:
> >> The ADV7511 has four 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.
> >> 
> >> 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 <kieran.bingham@ideasonboard.com>
> >> ---
> >> 
> >>  .../bindings/display/bridge/adi,adv7511.txt        | 10 +++++-
> > 
> > I don't mind personally, but device tree maintainers usually ask for DT
> > bindings changes to be split to a separate patch.
> > 
> >>  drivers/gpu/drm/bridge/adv7511/adv7511.h           |  4 +++
> >>  drivers/gpu/drm/bridge/adv7511/adv7511_drv.c       | 36 ++++++++++-----
> >>  3 files changed, 37 insertions(+), 13 deletions(-)
> >> 
> >> diff --git
> >> a/Documentation/devicetree/bindings/display/bridge/adi,adv7511.txt
> >> b/Documentation/devicetree/bindings/display/bridge/adi,adv7511.txt
> >> index 0047b1394c70..f6bb9f6d3f48 100644
> >> --- a/Documentation/devicetree/bindings/display/bridge/adi,adv7511.txt
> >> +++ b/Documentation/devicetree/bindings/display/bridge/adi,adv7511.txt
> >> 
> >> @@ -70,6 +70,9 @@ Optional properties:
> >>    rather than generate its own timings for HDMI output.
> >>  
> >>  - clocks: from common clock binding: reference to the CEC clock.
> >>  - clock-names: from common clock binding: must be "cec".
> >> 
> >> +- reg-names : Names of maps with programmable addresses.
> >> +	It can contain any map needing a non-default address.
> >> +	Possible maps names are : "main", "edid", "cec", "packet"
> > 
> > Is the reg-names property (and the additional maps) mandatory or optional
> > ? If mandatory you should also update the existing DT sources that use
> > those bindings.
> 
> They are currently optional. I do prefer it that way - but perhaps due to an
> issue mentioned below we might need to make this driver mandatory ?
> 
> > If optional you should define which I2C addresses will be used when
> > the maps are not specified (and in that case I think we should go for
> > the addresses listed as default in the datasheet, which correspond to the
> > current driver implementation when the main address is 0x3d/0x7a).
> 
> The current addresses do not correspond to the datasheet, even when the
> implementation main address is set to 0x3d.

Don't they ? The driver currently uses the following (8-bit) I2C addresses:

EDID:   main + 4  = 0x7e (0x3f)
Packet: main - 10 = 0x70 (0x38)
CEC:    main - 2  = 0x78 (0x3c)

Those are the default addresses according to section 4.1 of the ADV7511W 
programming guide (rev. B), and they match the ones you set in this patch.

> Thus, in my opinion - they are currently 'wrong' - but perhaps changing them
> is considered breakage too.
> 
> A particular issue will arise here too - as on this device - when the device
> is in low-power mode (after probe, before use) - the maps will respond on
> their *hardware default* addresses (the ones implemented in this patch),
> and thus consume that address on the I2C bus regardless of their settings
> in the driver.

We've discussed this previously and I share you concern. Just to make sure I 
remember correctly, did all the secondary maps reset to their default 
addresses, or just the EDID map ?

> > You should also update the definition of the reg property that currently
> > just states
> > 
> > - reg: I2C slave address
> > 
> > And finally you might want to define the term "map" in this context.
> > Here's a proposal (if we make all maps mandatory).
> > 
> > The ADV7511 internal registers are split into four pages exposed through
> > different I2C addresses, creating four register maps. The I2C addresses of
> > all four maps shall be specified by the reg and reg-names property.
> > 
> > - reg: I2C slave addresses, one per reg-names entry
> > - reg-names: map names, shall be "main", "edid", "cec", "packet"
> > 
> >>  Required nodes:
> >> @@ -88,7 +91,12 @@ Example
> >> 
> >>  	adv7511w: hdmi@39 {
> >>  		compatible = "adi,adv7511w";
> >> -		reg = <39>;
> >> +		/*
> >> +		 * The EDID page will be accessible on address 0x66 on the i2c
> >> +		 * bus. All other maps continue to use their default addresses.
> >> +		 */
> >> +		reg = <0x39 0x66>;
> >> +		reg-names = "main", "edid";
> >>  		interrupt-parent = <&gpio3>;
> >>  		interrupts = <29 IRQ_TYPE_EDGE_FALLING>;
> >>  		clocks = <&cec_clock>;

[snip]

> >> diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
> >> b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
> >> index efa29db5fc2b..7ec33837752b 100644
> >> --- a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
> >> +++ b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c

[snip]

> >> @@ -1153,24 +1151,35 @@ static int adv7511_probe(struct i2c_client *i2c,
> >> const struct i2c_device_id *id)
> >>  	if (ret)
> >>  		goto uninit_regulators;
> >> 
> >> -	regmap_write(adv7511->regmap, ADV7511_REG_EDID_I2C_ADDR,
> >> edid_i2c_addr);
> >> -	regmap_write(adv7511->regmap, ADV7511_REG_PACKET_I2C_ADDR,
> >> -		     main_i2c_addr - 0xa);
> >> -	regmap_write(adv7511->regmap, ADV7511_REG_CEC_I2C_ADDR,
> >> -		     main_i2c_addr - 2);
> >> -
> >>  	adv7511_packet_disable(adv7511, 0xffff);
> >> -	adv7511->i2c_edid = i2c_new_dummy(i2c->adapter, edid_i2c_addr >> 1);
> >> +	adv7511->i2c_edid = i2c_new_secondary_device(i2c, "edid",
> >> +					ADV7511_REG_EDID_I2C_ADDR_DEFAULT);
> >>  	if (!adv7511->i2c_edid) {
> >>  		ret = -ENOMEM;
> > 
> > I wonder if this is the right error code. Maybe -EINVAL ? In most cases
> > errors will be caused by invalid addresses (out of memory and
> > device_register() failures can happen too but should be less common).
> 
> It's arbitrary, as multiple error paths simply return NULL, but I think you
> are right - the 'most common' fault here is likely to be an in use address.
> 
> -ENOMEM was chosen here to mirror the equivalent code returned from the
> adv7604 driver.
> 
> Either way - -EINVAL does feel better at the moment. Although when mirrored
> back to the ADV7604 that becomes a distinct change there.
> 
> I don't think that matters though - so I'll apply this fix to both patches.
> 
> > It would be nice if i2c_new_secondary_device() returned an ERR_PTR, but
> > that's out of scope.
> 
> I was about to say - well I'll add this, but the reality is it might require
> updating all users of i2c_new_device, and i2c_new_dummy. Updating
> i2c_new_secondary_device() on it's own seems a bit pointless otherwise.
> 
> And that is definitely out of scope for the amount of time I currently have
> :D
> 
> >>  		goto uninit_regulators;
> >>  	}
> >> 
> >> +	regmap_write(adv7511->regmap, ADV7511_REG_EDID_I2C_ADDR,
> >> +		     adv7511->i2c_edid->addr << 1);
> >> +
> >>  	ret = adv7511_init_cec_regmap(adv7511);
> >>  	if (ret)
> >>  		goto err_i2c_unregister_edid;
> >> 
> >> +	regmap_write(adv7511->regmap, ADV7511_REG_CEC_I2C_ADDR,
> >> +		     adv7511->i2c_cec->addr << 1);
> >> +
> >> +	adv7511->i2c_packet = i2c_new_secondary_device(i2c, "packet",
> >> +					ADV7511_REG_PACKET_I2C_ADDR_DEFAULT);
> >> +	if (!adv7511->i2c_packet) {
> >> +		ret = -ENOMEM;
> >> +		goto err_unregister_cec;
> >> +	}
> >> +
> >> +	regmap_write(adv7511->regmap, ADV7511_REG_PACKET_I2C_ADDR,
> >> +		     adv7511->i2c_packet->addr << 1);
> >> +
> >>  	INIT_WORK(&adv7511->hpd_work, adv7511_hpd_work);
> >>  	
> >>  	if (i2c->irq) {

[snip]

-- 
Regards,

Laurent Pinchart

Re: [PATCH 2/2] drm: adv7511: Add support for i2c_new_secondary_device

[Kieran Bingham]

Hi Laurent,

On 07/02/18 21:59, Laurent Pinchart wrote:
> Hi Kieran,
> 
> On Wednesday, 7 February 2018 17:14:09 EET Kieran Bingham wrote:
>> On 29/01/18 10:26, Laurent Pinchart wrote:
>>> On Monday, 22 January 2018 14:50:00 EET Kieran Bingham wrote:
>>>> The ADV7511 has four 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.
>>>>
>>>> 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 <kieran.bingham@ideasonboard.com>
>>>> ---
>>>>
>>>>  .../bindings/display/bridge/adi,adv7511.txt        | 10 +++++-
>>>
>>> I don't mind personally, but device tree maintainers usually ask for DT
>>> bindings changes to be split to a separate patch.
>>>
>>>>  drivers/gpu/drm/bridge/adv7511/adv7511.h           |  4 +++
>>>>  drivers/gpu/drm/bridge/adv7511/adv7511_drv.c       | 36 ++++++++++-----
>>>>  3 files changed, 37 insertions(+), 13 deletions(-)
>>>>
>>>> diff --git
>>>> a/Documentation/devicetree/bindings/display/bridge/adi,adv7511.txt
>>>> b/Documentation/devicetree/bindings/display/bridge/adi,adv7511.txt
>>>> index 0047b1394c70..f6bb9f6d3f48 100644
>>>> --- a/Documentation/devicetree/bindings/display/bridge/adi,adv7511.txt
>>>> +++ b/Documentation/devicetree/bindings/display/bridge/adi,adv7511.txt
>>>>
>>>> @@ -70,6 +70,9 @@ Optional properties:
>>>>    rather than generate its own timings for HDMI output.
>>>>  
>>>>  - clocks: from common clock binding: reference to the CEC clock.
>>>>  - clock-names: from common clock binding: must be "cec".
>>>>
>>>> +- reg-names : Names of maps with programmable addresses.
>>>> +	It can contain any map needing a non-default address.
>>>> +	Possible maps names are : "main", "edid", "cec", "packet"
>>>
>>> Is the reg-names property (and the additional maps) mandatory or optional
>>> ? If mandatory you should also update the existing DT sources that use
>>> those bindings.
>>
>> They are currently optional. I do prefer it that way - but perhaps due to an
>> issue mentioned below we might need to make this driver mandatory ?
>>
>>> If optional you should define which I2C addresses will be used when
>>> the maps are not specified (and in that case I think we should go for
>>> the addresses listed as default in the datasheet, which correspond to the
>>> current driver implementation when the main address is 0x3d/0x7a).
>>
>> The current addresses do not correspond to the datasheet, even when the
>> implementation main address is set to 0x3d.
> 
> Don't they ? The driver currently uses the following (8-bit) I2C addresses:
> 
> EDID:   main + 4  = 0x7e (0x3f)
> Packet: main - 10 = 0x70 (0x38)
> CEC:    main - 2  = 0x78 (0x3c)
> 
> Those are the default addresses according to section 4.1 of the ADV7511W 
> programming guide (rev. B), and they match the ones you set in this patch.

Sorry - I was clearly subtracting 8bit values from a 7bit address in my failed
assertion, to both you and Archit.



>> Thus, in my opinion - they are currently 'wrong' - but perhaps changing them
>> is considered breakage too.
>>
>> A particular issue will arise here too - as on this device - when the device
>> is in low-power mode (after probe, before use) - the maps will respond on
>> their *hardware default* addresses (the ones implemented in this patch),
>> and thus consume that address on the I2C bus regardless of their settings
>> in the driver.
> 
> We've discussed this previously and I share you concern. Just to make sure I 
> remember correctly, did all the secondary maps reset to their default 
> addresses, or just the EDID map ?


The following responds on the bus when programmed at alternative addresses, and
in low power mode. The responses are all 0, but that's still going to conflict
with other hardware if it tries to use the 'un-used' addresses.

Packet (0x38),
Main (0x39),
Fixed (set to 0 by software, but shows up at 0x3e)
and EDID (0x3f).

So actually it's only the CEC which don't respond when in "low-power-mode".


As far as I can see, (git grep  -B3 adi,adv75) - The r8a7792-wheat.dts is the
only instance of a device using 0x3d, which means that Sergei's patch changed
the behaviour of all the existing devices before that.

Thus - this patch could be seen to be a 'correction' back to the original
behaviour for all devices except the Wheat, and possibly devices added after
Sergei's patch went in.

However - by my understanding, - any device which has only one ADV75(3,1)+
should use the hardware defined addresses (the hardware defaults will be
conflicting on the bus anyway, thus should be assigned to the ADV7511)

Any platform which uses *two* ADV7511 devices on the same bus should actually
set *both* devices to use entirely separate addresses - or they will still
conflict with each other.

Now - if my understanding is correct - then I believe - that means that all
existing devices except Wheat *should* be using the default addresses as this
patch updates them to.

The Wheat - has an invalid configuration - and thus should be updated anyway.

Regards

Kieran

>>> You should also update the definition of the reg property that currently
>>> just states
>>>
>>> - reg: I2C slave address
>>>
>>> And finally you might want to define the term "map" in this context.
>>> Here's a proposal (if we make all maps mandatory).
>>>
>>> The ADV7511 internal registers are split into four pages exposed through
>>> different I2C addresses, creating four register maps. The I2C addresses of
>>> all four maps shall be specified by the reg and reg-names property.
>>>
>>> - reg: I2C slave addresses, one per reg-names entry
>>> - reg-names: map names, shall be "main", "edid", "cec", "packet"
>>>
>>>>  Required nodes:
>>>> @@ -88,7 +91,12 @@ Example
>>>>
>>>>  	adv7511w: hdmi@39 {
>>>>  		compatible = "adi,adv7511w";
>>>> -		reg = <39>;
>>>> +		/*
>>>> +		 * The EDID page will be accessible on address 0x66 on the i2c
>>>> +		 * bus. All other maps continue to use their default addresses.
>>>> +		 */
>>>> +		reg = <0x39 0x66>;
>>>> +		reg-names = "main", "edid";
>>>>  		interrupt-parent = <&gpio3>;
>>>>  		interrupts = <29 IRQ_TYPE_EDGE_FALLING>;
>>>>  		clocks = <&cec_clock>;
> 
> [snip]
> 
>>>> diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
>>>> b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
>>>> index efa29db5fc2b..7ec33837752b 100644
>>>> --- a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
>>>> +++ b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
> 
> [snip]
> 
>>>> @@ -1153,24 +1151,35 @@ static int adv7511_probe(struct i2c_client *i2c,
>>>> const struct i2c_device_id *id)
>>>>  	if (ret)
>>>>  		goto uninit_regulators;
>>>>
>>>> -	regmap_write(adv7511->regmap, ADV7511_REG_EDID_I2C_ADDR,
>>>> edid_i2c_addr);
>>>> -	regmap_write(adv7511->regmap, ADV7511_REG_PACKET_I2C_ADDR,
>>>> -		     main_i2c_addr - 0xa);
>>>> -	regmap_write(adv7511->regmap, ADV7511_REG_CEC_I2C_ADDR,
>>>> -		     main_i2c_addr - 2);
>>>> -
>>>>  	adv7511_packet_disable(adv7511, 0xffff);
>>>> -	adv7511->i2c_edid = i2c_new_dummy(i2c->adapter, edid_i2c_addr >> 1);
>>>> +	adv7511->i2c_edid = i2c_new_secondary_device(i2c, "edid",
>>>> +					ADV7511_REG_EDID_I2C_ADDR_DEFAULT);
>>>>  	if (!adv7511->i2c_edid) {
>>>>  		ret = -ENOMEM;
>>>
>>> I wonder if this is the right error code. Maybe -EINVAL ? In most cases
>>> errors will be caused by invalid addresses (out of memory and
>>> device_register() failures can happen too but should be less common).
>>
>> It's arbitrary, as multiple error paths simply return NULL, but I think you
>> are right - the 'most common' fault here is likely to be an in use address.
>>
>> -ENOMEM was chosen here to mirror the equivalent code returned from the
>> adv7604 driver.
>>
>> Either way - -EINVAL does feel better at the moment. Although when mirrored
>> back to the ADV7604 that becomes a distinct change there.
>>
>> I don't think that matters though - so I'll apply this fix to both patches.
>>
>>> It would be nice if i2c_new_secondary_device() returned an ERR_PTR, but
>>> that's out of scope.
>>
>> I was about to say - well I'll add this, but the reality is it might require
>> updating all users of i2c_new_device, and i2c_new_dummy. Updating
>> i2c_new_secondary_device() on it's own seems a bit pointless otherwise.
>>
>> And that is definitely out of scope for the amount of time I currently have
>> :D
>>
>>>>  		goto uninit_regulators;
>>>>  	}
>>>>
>>>> +	regmap_write(adv7511->regmap, ADV7511_REG_EDID_I2C_ADDR,
>>>> +		     adv7511->i2c_edid->addr << 1);
>>>> +
>>>>  	ret = adv7511_init_cec_regmap(adv7511);
>>>>  	if (ret)
>>>>  		goto err_i2c_unregister_edid;
>>>>
>>>> +	regmap_write(adv7511->regmap, ADV7511_REG_CEC_I2C_ADDR,
>>>> +		     adv7511->i2c_cec->addr << 1);
>>>> +
>>>> +	adv7511->i2c_packet = i2c_new_secondary_device(i2c, "packet",
>>>> +					ADV7511_REG_PACKET_I2C_ADDR_DEFAULT);
>>>> +	if (!adv7511->i2c_packet) {
>>>> +		ret = -ENOMEM;
>>>> +		goto err_unregister_cec;
>>>> +	}
>>>> +
>>>> +	regmap_write(adv7511->regmap, ADV7511_REG_PACKET_I2C_ADDR,
>>>> +		     adv7511->i2c_packet->addr << 1);
>>>> +
>>>>  	INIT_WORK(&adv7511->hpd_work, adv7511_hpd_work);
>>>>  	
>>>>  	if (i2c->irq) {
> 
> [snip]
> 

Re: [PATCH 2/2] drm: adv7511: Add support for i2c_new_secondary_device

[Laurent Pinchart]

Hi Kieran,

On Thursday, 8 February 2018 01:30:43 EET Kieran Bingham wrote:
> On 07/02/18 21:59, Laurent Pinchart wrote:
> > On Wednesday, 7 February 2018 17:14:09 EET Kieran Bingham wrote:
> >> On 29/01/18 10:26, Laurent Pinchart wrote:
> >>> On Monday, 22 January 2018 14:50:00 EET Kieran Bingham wrote:
> >>>> The ADV7511 has four 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.
> >>>> 
> >>>> 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 <kieran.bingham@ideasonboard.com>
> >>>> ---
> >>>> 
> >>>>  .../bindings/display/bridge/adi,adv7511.txt        | 10 +++++-
> >>> 
> >>> I don't mind personally, but device tree maintainers usually ask for DT
> >>> bindings changes to be split to a separate patch.
> >>> 
> >>>>  drivers/gpu/drm/bridge/adv7511/adv7511.h           |  4 +++
> >>>>  drivers/gpu/drm/bridge/adv7511/adv7511_drv.c       | 36++++++++++-----
> >>>>  3 files changed, 37 insertions(+), 13 deletions(-)
> >>>> 
> >>>> diff --git
> >>>> a/Documentation/devicetree/bindings/display/bridge/adi,adv7511.txt
> >>>> b/Documentation/devicetree/bindings/display/bridge/adi,adv7511.txt
> >>>> index 0047b1394c70..f6bb9f6d3f48 100644
> >>>> --- a/Documentation/devicetree/bindings/display/bridge/adi,adv7511.txt
> >>>> +++ b/Documentation/devicetree/bindings/display/bridge/adi,adv7511.txt
> >>>> 
> >>>> @@ -70,6 +70,9 @@ Optional properties:
> >>>>    rather than generate its own timings for HDMI output.
> >>>>  - clocks: from common clock binding: reference to the CEC clock.
> >>>>  - clock-names: from common clock binding: must be "cec".
> >>>> +- reg-names : Names of maps with programmable addresses.
> >>>> +	It can contain any map needing a non-default address.
> >>>> +	Possible maps names are : "main", "edid", "cec", "packet"
> >>> 
> >>> Is the reg-names property (and the additional maps) mandatory or
> >>> optional ? If mandatory you should also update the existing DT sources
> >>> that use those bindings.
> >> 
> >> They are currently optional. I do prefer it that way - but perhaps due to
> >> an issue mentioned below we might need to make this driver mandatory ?>> 
> >> 
> >>> If optional you should define which I2C addresses will be used when
> >>> the maps are not specified (and in that case I think we should go for
> >>> the addresses listed as default in the datasheet, which correspond to
> >>> the current driver implementation when the main address is 0x3d/0x7a).
> >> 
> >> The current addresses do not correspond to the datasheet, even when the
> >> implementation main address is set to 0x3d.
> > 
> > Don't they ? The driver currently uses the following (8-bit) I2C
> > addresses:
> > 
> > EDID:   main + 4  = 0x7e (0x3f)
> > Packet: main - 10 = 0x70 (0x38)
> > CEC:    main - 2  = 0x78 (0x3c)
> > 
> > Those are the default addresses according to section 4.1 of the ADV7511W
> > programming guide (rev. B), and they match the ones you set in this patch.
> 
> Sorry - I was clearly subtracting 8bit values from a 7bit address in my
> failed assertion, to both you and Archit.

No worries.

> >> Thus, in my opinion - they are currently 'wrong' - but perhaps changing
> >> them is considered breakage too.
> >> 
> >> A particular issue will arise here too - as on this device - when the
> >> device is in low-power mode (after probe, before use) - the maps will
> >> respond on their *hardware default* addresses (the ones implemented in
> >> this patch), and thus consume that address on the I2C bus regardless of
> >> their settings in the driver.
> > 
> > We've discussed this previously and I share you concern. Just to make sure
> > I remember correctly, did all the secondary maps reset to their default
> > addresses, or just the EDID map ?
> 
> The following responds on the bus when programmed at alternative addresses,
> and in low power mode. The responses are all 0, but that's still going to
> conflict with other hardware if it tries to use the 'un-used' addresses.
> 
> Packet (0x38),
> Main (0x39),
> Fixed (set to 0 by software, but shows up at 0x3e)
> and EDID (0x3f).
> 
> So actually it's only the CEC which don't respond when in "low-power-mode".
> 
> As far as I can see, (git grep  -B3 adi,adv75) - The r8a7792-wheat.dts is
> the only instance of a device using 0x3d, which means that Sergei's patch
> changed the behaviour of all the existing devices before that.
> 
> Thus - this patch could be seen to be a 'correction' back to the original
> behaviour for all devices except the Wheat, and possibly devices added after
> Sergei's patch went in.
> 
> However - by my understanding, - any device which has only one ADV75(3,1)+
> should use the hardware defined addresses (the hardware defaults will be
> conflicting on the bus anyway, thus should be assigned to the ADV7511)
> 
> Any platform which uses *two* ADV7511 devices on the same bus should
> actually set *both* devices to use entirely separate addresses - or they
> will still conflict with each other.

Agreed. No access should be made to the default addresses for the secondary 
I2C clients, otherwise there's a risk of conflict.

When only one ADV7511 is present, but conflicts with another device, we could 
reprogram the other device only (assuming it doesn't lose its configuration in 
low-power mode), or reprogram both.

> Now - if my understanding is correct - then I believe - that means that all
> existing devices except Wheat *should* be using the default addresses as
> this patch updates them to.
> 
> The Wheat - has an invalid configuration - and thus should be updated
> anyway.

I agree.

> >>> You should also update the definition of the reg property that currently
> >>> just states
> >>> 
> >>> - reg: I2C slave address
> >>> 
> >>> And finally you might want to define the term "map" in this context.
> >>> Here's a proposal (if we make all maps mandatory).
> >>> 
> >>> The ADV7511 internal registers are split into four pages exposed through
> >>> different I2C addresses, creating four register maps. The I2C addresses
> >>> of all four maps shall be specified by the reg and reg-names property.
> >>> 
> >>> - reg: I2C slave addresses, one per reg-names entry
> >>> - reg-names: map names, shall be "main", "edid", "cec", "packet"
> >>> 
> >>>>  Required nodes:
> >>>> @@ -88,7 +91,12 @@ Example
> >>>> 
> >>>>  	adv7511w: hdmi@39 {
> >>>>  		compatible = "adi,adv7511w";
> >>>> 
> >>>> -		reg = <39>;
> >>>> +		/*
> >>>> +		 * The EDID page will be accessible on address 0x66 on the i2c
> >>>> +		 * bus. All other maps continue to use their default addresses.
> >>>> +		 */
> >>>> +		reg = <0x39 0x66>;
> >>>> +		reg-names = "main", "edid";
> >>>>  		interrupt-parent = <&gpio3>;
> >>>>  		interrupts = <29 IRQ_TYPE_EDGE_FALLING>;
> >>>>  		clocks = <&cec_clock>;

[snip]

-- 
Regards,

Laurent Pinchart

Re: [PATCH 2/2] drm: adv7511: Add support for i2c_new_secondary_device

[Rob Herring]

On Mon, Jan 22, 2018 at 12:50:01PM +0000, Kieran Bingham wrote:
> The ADV7511 has four 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.
> 
> 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 <kieran.bingham@ideasonboard.com>
> ---
>  .../bindings/display/bridge/adi,adv7511.txt        | 10 +++++-

Reviewed-by: Rob Herring <robh@kernel.org>

>  drivers/gpu/drm/bridge/adv7511/adv7511.h           |  4 +++
>  drivers/gpu/drm/bridge/adv7511/adv7511_drv.c       | 36 ++++++++++++++--------
>  3 files changed, 37 insertions(+), 13 deletions(-)
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel