From: Kieran Bingham <[email protected]>
Back in 2014, Jean-Michel provided patches [0] to implement a means of
describing software defined I2C addresses for devices through the DT nodes.
The patch to implement the function "i2c_new_secondary_device()" was integrated,
but the corresponding driver update didn't get applied.
This short series re-bases Jean-Michel's patch to mainline for the ADV7604 driver
in linux-media, and also provides a patch for the ADV7511 DRM Bridge driver taking
the same approach.
This series allows us to define the I2C address allocations of these devices in
the device tree for the Renesas D3 platform where these two devices reside on
the same bus and conflict with each other presently..
[0] https://lkml.org/lkml/2014/10/22/468
[1] https://lkml.org/lkml/2014/10/22/469
v2:
- dt bindings split from driver changes
- fixed up dt binding property descriptions
- Update missing edid-i2c address setting (adv7511)
- Provide update for r8a7792 DTB to account for address conflict
v3:
- Split map register addresses into individual declarations across all uses
Jean-Michel Hautbois (2):
dt-bindings: media: adv7604: Add support for i2c_new_secondary_device
media: adv7604: Add support for i2c_new_secondary_device
Kieran Bingham (3):
dt-bindings: adv7511: Add support for i2c_new_secondary_device
[RFT] ARM: dts: wheat: Fix ADV7513 address usage
drm: adv7511: Add support for i2c_new_secondary_device
.../bindings/display/bridge/adi,adv7511.txt | 18 ++++++-
.../devicetree/bindings/media/i2c/adv7604.txt | 18 ++++++-
arch/arm/boot/dts/r8a7792-wheat.dts | 12 ++++-
drivers/gpu/drm/bridge/adv7511/adv7511.h | 6 +++
drivers/gpu/drm/bridge/adv7511/adv7511_drv.c | 42 +++++++++------
drivers/media/i2c/adv7604.c | 62 ++++++++++++++--------
6 files changed, 115 insertions(+), 43 deletions(-)
--
2.7.4
From: Kieran Bingham <[email protected]>
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.
Extend the device tree node bindings to be able 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 <[email protected]>
Reviewed-by: Rob Herring <[email protected]>
---
v2:
- Fixed up reg: property description to account for multiple optional
addresses.
- Minor reword to commit message to account for DT only change
- Collected Robs RB tag
v3:
- Split map register addresses into individual declarations.
.../devicetree/bindings/display/bridge/adi,adv7511.txt | 18 ++++++++++++++++--
1 file changed, 16 insertions(+), 2 deletions(-)
diff --git a/Documentation/devicetree/bindings/display/bridge/adi,adv7511.txt b/Documentation/devicetree/bindings/display/bridge/adi,adv7511.txt
index 0047b1394c70..3f85c351dd39 100644
--- a/Documentation/devicetree/bindings/display/bridge/adi,adv7511.txt
+++ b/Documentation/devicetree/bindings/display/bridge/adi,adv7511.txt
@@ -14,7 +14,13 @@ Required properties:
"adi,adv7513"
"adi,adv7533"
-- reg: I2C slave address
+- reg: I2C slave addresses
+ The ADV7511 internal registers are split into four pages exposed through
+ different I2C addresses, creating four register maps. Each map has it own
+ I2C 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.
+
The ADV7511 supports a large number of input data formats that differ by their
color depth, color format, clock mode, bit justification and random
@@ -70,6 +76,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 +97,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>;
--
2.7.4
From: Kieran Bingham <[email protected]>
The r8a7792 Wheat board has two ADV7513 devices sharing a single i2c
bus, however in low power mode the ADV7513 will reset it's slave maps to
use the hardware defined default addresses.
The ADV7511 driver was adapted to allow the two devices to be registered
correctly - but it did not take into account the fault whereby the
devices reset the addresses.
This results in an address conflict between the device using the default
addresses, and the other device if it is in low-power-mode.
Repair this issue by moving both devices away from the default address
definitions.
Signed-off-by: Kieran Bingham <[email protected]>
---
v2:
- Addition to series
v3:
- Split map register addresses into individual declarations.
arch/arm/boot/dts/r8a7792-wheat.dts | 12 ++++++++++--
1 file changed, 10 insertions(+), 2 deletions(-)
diff --git a/arch/arm/boot/dts/r8a7792-wheat.dts b/arch/arm/boot/dts/r8a7792-wheat.dts
index b9471b67b728..42fff8837eab 100644
--- a/arch/arm/boot/dts/r8a7792-wheat.dts
+++ b/arch/arm/boot/dts/r8a7792-wheat.dts
@@ -240,9 +240,16 @@
status = "okay";
clock-frequency = <400000>;
+ /*
+ * The adv75xx resets its addresses to defaults during low power power
+ * mode. Because we have two ADV7513 devices on the same bus, we must
+ * change both of them away from the defaults so that they do not
+ * conflict.
+ */
hdmi@3d {
compatible = "adi,adv7513";
- reg = <0x3d>;
+ reg = <0x3d>, <0x2d>, <0x4d>, <0x5d>;
+ reg-names = "main", "cec", "edid", "packet";
adi,input-depth = <8>;
adi,input-colorspace = "rgb";
@@ -272,7 +279,8 @@
hdmi@39 {
compatible = "adi,adv7513";
- reg = <0x39>;
+ reg = <0x39>, <0x29>, <0x49>, <0x59>;
+ reg-names = "main", "cec", "edid", "packet";
adi,input-depth = <8>;
adi,input-colorspace = "rgb";
--
2.7.4
From: Kieran Bingham <[email protected]>
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 <[email protected]>
---
v2:
- Update missing edid-i2c address setting
- Split out DT bindings
- Rename and move the I2C default addresses to their own section
drivers/gpu/drm/bridge/adv7511/adv7511.h | 6 ++++
drivers/gpu/drm/bridge/adv7511/adv7511_drv.c | 42 ++++++++++++++++++----------
2 files changed, 33 insertions(+), 15 deletions(-)
diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511.h b/drivers/gpu/drm/bridge/adv7511/adv7511.h
index d034b2cb5eee..04e6759ee45b 100644
--- a/drivers/gpu/drm/bridge/adv7511/adv7511.h
+++ b/drivers/gpu/drm/bridge/adv7511/adv7511.h
@@ -93,6 +93,11 @@
#define ADV7511_REG_CHIP_ID_HIGH 0xf5
#define ADV7511_REG_CHIP_ID_LOW 0xf6
+/* Hardware defined default addresses for i2c register maps */
+#define ADV7511_CEC_I2C_ADDR_DEFAULT 0x3c
+#define ADV7511_EDID_I2C_ADDR_DEFAULT 0x3f
+#define ADV7511_PACKET_I2C_ADDR_DEFAULT 0x38
+
#define ADV7511_CSC_ENABLE BIT(7)
#define ADV7511_CSC_UPDATE_MODE BIT(5)
@@ -322,6 +327,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..5e61b928c9c0 100644
--- a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
+++ b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
@@ -586,7 +586,7 @@ static int adv7511_get_modes(struct adv7511 *adv7511,
/* Reading the EDID only works if the device is powered */
if (!adv7511->powered) {
unsigned int edid_i2c_addr =
- (adv7511->i2c_main->addr << 1) + 4;
+ (adv7511->i2c_edid->addr << 1);
__adv7511_power_on(adv7511);
@@ -969,10 +969,10 @@ 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_CEC_I2C_ADDR_DEFAULT);
if (!adv->i2c_cec)
- return -ENOMEM;
+ return -EINVAL;
i2c_set_clientdata(adv->i2c_cec, adv);
adv->regmap_cec = devm_regmap_init_i2c(adv->i2c_cec,
@@ -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_EDID_I2C_ADDR_DEFAULT);
if (!adv7511->i2c_edid) {
- ret = -ENOMEM;
+ ret = -EINVAL;
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_PACKET_I2C_ADDR_DEFAULT);
+ if (!adv7511->i2c_packet) {
+ ret = -EINVAL;
+ 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
From: Jean-Michel Hautbois <[email protected]>
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 <[email protected]>
[Kieran: Re-adapted for mainline]
Signed-off-by: Kieran Bingham <[email protected]>
---
Based upon the original posting :
https://lkml.org/lkml/2014/10/22/469
v2:
- Split out DT bindings from driver updates
- Return -EINVAL on error paths from adv76xx_dummy_client()
drivers/media/i2c/adv7604.c | 62 +++++++++++++++++++++++++++++----------------
1 file changed, 40 insertions(+), 22 deletions(-)
diff --git a/drivers/media/i2c/adv7604.c b/drivers/media/i2c/adv7604.c
index 1544920ec52d..872e124793f8 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,11 +3498,9 @@ 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;
+ err = -EINVAL;
v4l2_err(sd, "failed to create i2c client %u\n", i);
goto err_i2c;
}
--
2.7.4
From: Jean-Michel Hautbois <[email protected]>
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.
Extend the device tree node bindings to be able 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 <[email protected]>
[Kieran: Re-adapted for mainline]
Signed-off-by: Kieran Bingham <[email protected]>
Reviewed-by: Rob Herring <[email protected]>
---
Based upon the original posting :
https://lkml.org/lkml/2014/10/22/469
v2:
- DT Binding update separated from code change
- Minor reword to commit message to account for DT only change.
- Collected Rob's RB tag.
v3:
- Split map register addresses into individual declarations.
.../devicetree/bindings/media/i2c/adv7604.txt | 18 ++++++++++++++++--
1 file changed, 16 insertions(+), 2 deletions(-)
diff --git a/Documentation/devicetree/bindings/media/i2c/adv7604.txt b/Documentation/devicetree/bindings/media/i2c/adv7604.txt
index 9cbd92eb5d05..ebb5f070c05b 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>;
--
2.7.4
Hi Kieran,
Thank you for the patch.
On Tuesday, 13 February 2018 00:07:49 EET Kieran Bingham wrote:
> From: Jean-Michel Hautbois <[email protected]>
>
> 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.
>
> Extend the device tree node bindings to be able 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 <[email protected]>
> [Kieran: Re-adapted for mainline]
> Signed-off-by: Kieran Bingham <[email protected]>
> Reviewed-by: Rob Herring <[email protected]>
Nitpicking, I might not mention i2c_new_secondary_device in the subject, as
this is a DT bindings change. I don't mind too much though, as long as the
bindings themselves don't contain Linux-specific information, and they don't,
so
Reviewed-by: Laurent Pinchart <[email protected]>
> ---
> Based upon the original posting :
> https://lkml.org/lkml/2014/10/22/469
>
> v2:
> - DT Binding update separated from code change
> - Minor reword to commit message to account for DT only change.
> - Collected Rob's RB tag.
>
> v3:
> - Split map register addresses into individual declarations.
>
> .../devicetree/bindings/media/i2c/adv7604.txt | 18
> ++++++++++++++++-- 1 file changed, 16 insertions(+), 2 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/media/i2c/adv7604.txt
> b/Documentation/devicetree/bindings/media/i2c/adv7604.txt index
> 9cbd92eb5d05..ebb5f070c05b 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>;
--
Regards,
Laurent Pinchart
Hi Kieran,
Thank you for the patch.
On Tuesday, 13 February 2018 00:07:50 EET Kieran Bingham wrote:
> From: Kieran Bingham <[email protected]>
>
> 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.
>
> Extend the device tree node bindings to be able 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 <[email protected]>
> Reviewed-by: Rob Herring <[email protected]>
Same comment as for 1/5 about the subject line.
> ---
> v2:
> - Fixed up reg: property description to account for multiple optional
> addresses.
> - Minor reword to commit message to account for DT only change
> - Collected Robs RB tag
>
> v3:
> - Split map register addresses into individual declarations.
>
> .../devicetree/bindings/display/bridge/adi,adv7511.txt | 18 +++++++++++++--
> 1 file changed, 16 insertions(+), 2 deletions(-)
>
> diff --git
> a/Documentation/devicetree/bindings/display/bridge/adi,adv7511.txt
> b/Documentation/devicetree/bindings/display/bridge/adi,adv7511.txt index
> 0047b1394c70..3f85c351dd39 100644
> --- a/Documentation/devicetree/bindings/display/bridge/adi,adv7511.txt
> +++ b/Documentation/devicetree/bindings/display/bridge/adi,adv7511.txt
> @@ -14,7 +14,13 @@ Required properties:
> "adi,adv7513"
> "adi,adv7533"
>
> -- reg: I2C slave address
> +- reg: I2C slave addresses
> + The ADV7511 internal registers are split into four pages exposed through
> + different I2C addresses, creating four register maps. Each map has it own
> + I2C 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.
Nitpicking again, you're mixing I2C and I?C.
> +
>
> The ADV7511 supports a large number of input data formats that differ by
> their color depth, color format, clock mode, bit justification and random
> @@ -70,6 +76,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 +97,12 @@ Example
>
> adv7511w: hdmi@39 {
> compatible = "adi,adv7511w";
> - reg = <39>;
> + /*
> + * The EDID page will be accessible on address 0x66 on the i2c
And now you're using lowercase :-)
> + * 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>;
With these fixed (or not, up to you),
Reviewed-by: Laurent Pinchart <[email protected]>
--
Regards,
Laurent Pinchart
Hi Kieran,
Thank you for the patch.
On Tuesday, 13 February 2018 00:07:51 EET Kieran Bingham wrote:
> From: Kieran Bingham <[email protected]>
>
> The r8a7792 Wheat board has two ADV7513 devices sharing a single i2c
> bus, however in low power mode the ADV7513 will reset it's slave maps to
> use the hardware defined default addresses.
>
> The ADV7511 driver was adapted to allow the two devices to be registered
> correctly - but it did not take into account the fault whereby the
> devices reset the addresses.
>
> This results in an address conflict between the device using the default
> addresses, and the other device if it is in low-power-mode.
>
> Repair this issue by moving both devices away from the default address
> definitions.
>
> Signed-off-by: Kieran Bingham <[email protected]>
Reviewed-by: Laurent Pinchart <[email protected]>
> ---
> v2:
> - Addition to series
>
> v3:
> - Split map register addresses into individual declarations.
>
> arch/arm/boot/dts/r8a7792-wheat.dts | 12 ++++++++++--
> 1 file changed, 10 insertions(+), 2 deletions(-)
>
> diff --git a/arch/arm/boot/dts/r8a7792-wheat.dts
> b/arch/arm/boot/dts/r8a7792-wheat.dts index b9471b67b728..42fff8837eab
> 100644
> --- a/arch/arm/boot/dts/r8a7792-wheat.dts
> +++ b/arch/arm/boot/dts/r8a7792-wheat.dts
> @@ -240,9 +240,16 @@
> status = "okay";
> clock-frequency = <400000>;
>
> + /*
> + * The adv75xx resets its addresses to defaults during low power power
> + * mode. Because we have two ADV7513 devices on the same bus, we must
> + * change both of them away from the defaults so that they do not
> + * conflict.
> + */
> hdmi@3d {
> compatible = "adi,adv7513";
> - reg = <0x3d>;
> + reg = <0x3d>, <0x2d>, <0x4d>, <0x5d>;
> + reg-names = "main", "cec", "edid", "packet";
>
> adi,input-depth = <8>;
> adi,input-colorspace = "rgb";
> @@ -272,7 +279,8 @@
>
> hdmi@39 {
> compatible = "adi,adv7513";
> - reg = <0x39>;
> + reg = <0x39>, <0x29>, <0x49>, <0x59>;
> + reg-names = "main", "cec", "edid", "packet";
>
> adi,input-depth = <8>;
> adi,input-colorspace = "rgb";
--
Regards,
Laurent Pinchart
Hi Kieran,
Thank you for the patch.
On Tuesday, 13 February 2018 00:07:52 EET Kieran Bingham wrote:
> From: Jean-Michel Hautbois <[email protected]>
>
> 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 <[email protected]>
> [Kieran: Re-adapted for mainline]
> Signed-off-by: Kieran Bingham <[email protected]>
>
> ---
> Based upon the original posting :
> https://lkml.org/lkml/2014/10/22/469
>
> v2:
> - Split out DT bindings from driver updates
> - Return -EINVAL on error paths from adv76xx_dummy_client()
>
> drivers/media/i2c/adv7604.c | 62 ++++++++++++++++++++++++++----------------
> 1 file changed, 40 insertions(+), 22 deletions(-)
>
> diff --git a/drivers/media/i2c/adv7604.c b/drivers/media/i2c/adv7604.c
> index 1544920ec52d..872e124793f8 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 {
adv76xx_register seems to imply that this describes a particular register,
while the structure describes a registers map. How about adv76xx_register_map,
adv76xx_register_bank or adv76xx_register_page ?
> + const char *name;
> + u8 default_addr;
> +};
> +
> +static const struct adv76xx_register adv76xx_secondary_names[] = {
The table doesn't contain secondary names only as there's an entry for the
main map. How about calling it adv76xx_default_addresses or something along
the same line ?
> + [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)
Maybe unsigned int page ?
With these fixed,
Reviewed-by: Laurent Pinchart <[email protected]>
> {
> 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,11 +3498,9 @@ 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;
> + err = -EINVAL;
> v4l2_err(sd, "failed to create i2c client %u\n", i);
> goto err_i2c;
> }
--
Regards,
Laurent Pinchart
Hi Kieran,
Thank you for the patch.
On Tuesday, 13 February 2018 00:07:53 EET Kieran Bingham wrote:
> From: Kieran Bingham <[email protected]>
>
> 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 <[email protected]>
> ---
> v2:
> - Update missing edid-i2c address setting
> - Split out DT bindings
> - Rename and move the I2C default addresses to their own section
>
> drivers/gpu/drm/bridge/adv7511/adv7511.h | 6 ++++
> drivers/gpu/drm/bridge/adv7511/adv7511_drv.c | 42 ++++++++++++++++--------
> 2 files changed, 33 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511.h
> b/drivers/gpu/drm/bridge/adv7511/adv7511.h index d034b2cb5eee..04e6759ee45b
> 100644
> --- a/drivers/gpu/drm/bridge/adv7511/adv7511.h
> +++ b/drivers/gpu/drm/bridge/adv7511/adv7511.h
> @@ -93,6 +93,11 @@
> #define ADV7511_REG_CHIP_ID_HIGH 0xf5
> #define ADV7511_REG_CHIP_ID_LOW 0xf6
>
> +/* Hardware defined default addresses for i2c register maps */
s/i2c/I2C/ ? That's really because I had to find something :-)
Reviewed-by: Laurent Pinchart <[email protected]>
> +#define ADV7511_CEC_I2C_ADDR_DEFAULT 0x3c
> +#define ADV7511_EDID_I2C_ADDR_DEFAULT 0x3f
> +#define ADV7511_PACKET_I2C_ADDR_DEFAULT 0x38
> +
> #define ADV7511_CSC_ENABLE BIT(7)
> #define ADV7511_CSC_UPDATE_MODE BIT(5)
>
> @@ -322,6 +327,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..5e61b928c9c0 100644
> --- a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
> +++ b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
> @@ -586,7 +586,7 @@ static int adv7511_get_modes(struct adv7511 *adv7511,
> /* Reading the EDID only works if the device is powered */
> if (!adv7511->powered) {
> unsigned int edid_i2c_addr =
> - (adv7511->i2c_main->addr << 1) + 4;
> + (adv7511->i2c_edid->addr << 1);
>
> __adv7511_power_on(adv7511);
>
> @@ -969,10 +969,10 @@ 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_CEC_I2C_ADDR_DEFAULT);
> if (!adv->i2c_cec)
> - return -ENOMEM;
> + return -EINVAL;
> i2c_set_clientdata(adv->i2c_cec, adv);
>
> adv->regmap_cec = devm_regmap_init_i2c(adv->i2c_cec,
> @@ -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_EDID_I2C_ADDR_DEFAULT);
> if (!adv7511->i2c_edid) {
> - ret = -ENOMEM;
> + ret = -EINVAL;
> 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_PACKET_I2C_ADDR_DEFAULT);
> + if (!adv7511->i2c_packet) {
> + ret = -EINVAL;
> + 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
Hi Laurent,
On 13/02/18 12:06, Laurent Pinchart wrote:
> Hi Kieran,
>
> Thank you for the patch.
Thank you for your review,
> On Tuesday, 13 February 2018 00:07:49 EET Kieran Bingham wrote:
>> From: Jean-Michel Hautbois <[email protected]>
>>
>> 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.
>>
>> Extend the device tree node bindings to be able 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 <[email protected]>
>> [Kieran: Re-adapted for mainline]
>> Signed-off-by: Kieran Bingham <[email protected]>
>> Reviewed-by: Rob Herring <[email protected]>
>
> Nitpicking, I might not mention i2c_new_secondary_device in the subject, as
> this is a DT bindings change. I don't mind too much though, as long as the
> bindings themselves don't contain Linux-specific information, and they don't,
> so
How about: ... adv7604: Extend bindings to allow specifying slave map addresses
> Reviewed-by: Laurent Pinchart <[email protected]>
Collected, thanks.
--
Kieran
>
>> ---
>> Based upon the original posting :
>> https://lkml.org/lkml/2014/10/22/469
>>
>> v2:
>> - DT Binding update separated from code change
>> - Minor reword to commit message to account for DT only change.
>> - Collected Rob's RB tag.
>>
>> v3:
>> - Split map register addresses into individual declarations.
>>
>> .../devicetree/bindings/media/i2c/adv7604.txt | 18
>> ++++++++++++++++-- 1 file changed, 16 insertions(+), 2 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/media/i2c/adv7604.txt
>> b/Documentation/devicetree/bindings/media/i2c/adv7604.txt index
>> 9cbd92eb5d05..ebb5f070c05b 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>;
>
>
Hi Kieran,
On Tuesday, 13 February 2018 15:14:43 EET Kieran Bingham wrote:
> On 13/02/18 12:06, Laurent Pinchart wrote:
> > On Tuesday, 13 February 2018 00:07:49 EET Kieran Bingham wrote:
> >> From: Jean-Michel Hautbois <[email protected]>
> >>
> >> 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.
> >>
> >> Extend the device tree node bindings to be able 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 <[email protected]>
> >> [Kieran: Re-adapted for mainline]
> >> Signed-off-by: Kieran Bingham <[email protected]>
> >> Reviewed-by: Rob Herring <[email protected]>
> >
> > Nitpicking, I might not mention i2c_new_secondary_device in the subject,
> > as this is a DT bindings change. I don't mind too much though, as long as
> > the bindings themselves don't contain Linux-specific information, and they
> > don't, so
>
> How about: ... adv7604: Extend bindings to allow specifying slave map
> addresses
Sounds good to me.
> > Reviewed-by: Laurent Pinchart <[email protected]>
>
> Collected, thanks.
>
> --
> Kieran
>
> >> ---
> >>
> >> Based upon the original posting :
> >> https://lkml.org/lkml/2014/10/22/469
> >>
> >> v2:
> >> - DT Binding update separated from code change
> >> - Minor reword to commit message to account for DT only change.
> >> - Collected Rob's RB tag.
> >>
> >> v3:
> >> - Split map register addresses into individual declarations.
> >>
> >> .../devicetree/bindings/media/i2c/adv7604.txt | 18
> >>
> >> ++++++++++++++++-- 1 file changed, 16 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/Documentation/devicetree/bindings/media/i2c/adv7604.txt
> >> b/Documentation/devicetree/bindings/media/i2c/adv7604.txt index
> >> 9cbd92eb5d05..ebb5f070c05b 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>;
--
Regards,
Laurent Pinchart
Hi Laurent,
Thanks for the review,
On 13/02/18 12:23, Laurent Pinchart wrote:
> Hi Kieran,
>
> Thank you for the patch.
>
> On Tuesday, 13 February 2018 00:07:53 EET Kieran Bingham wrote:
>> From: Kieran Bingham <[email protected]>
>>
>> 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 <[email protected]>
>> ---
>> v2:
>> - Update missing edid-i2c address setting
>> - Split out DT bindings
>> - Rename and move the I2C default addresses to their own section
>>
>> drivers/gpu/drm/bridge/adv7511/adv7511.h | 6 ++++
>> drivers/gpu/drm/bridge/adv7511/adv7511_drv.c | 42 ++++++++++++++++--------
>> 2 files changed, 33 insertions(+), 15 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511.h
>> b/drivers/gpu/drm/bridge/adv7511/adv7511.h index d034b2cb5eee..04e6759ee45b
>> 100644
>> --- a/drivers/gpu/drm/bridge/adv7511/adv7511.h
>> +++ b/drivers/gpu/drm/bridge/adv7511/adv7511.h
>> @@ -93,6 +93,11 @@
>> #define ADV7511_REG_CHIP_ID_HIGH 0xf5
>> #define ADV7511_REG_CHIP_ID_LOW 0xf6
>>
>> +/* Hardware defined default addresses for i2c register maps */
>
> s/i2c/I2C/ ? That's really because I had to find something :-)
The I²C comes from JMH's original patch, but is much harder to grep for, so
normalising to I2C throughout.
>
> Reviewed-by: Laurent Pinchart <[email protected]
Collected, Thanks,
--
Kieran
>
>> +#define ADV7511_CEC_I2C_ADDR_DEFAULT 0x3c
>> +#define ADV7511_EDID_I2C_ADDR_DEFAULT 0x3f
>> +#define ADV7511_PACKET_I2C_ADDR_DEFAULT 0x38
>> +
>> #define ADV7511_CSC_ENABLE BIT(7)
>> #define ADV7511_CSC_UPDATE_MODE BIT(5)
>>
>> @@ -322,6 +327,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..5e61b928c9c0 100644
>> --- a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
>> +++ b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
>> @@ -586,7 +586,7 @@ static int adv7511_get_modes(struct adv7511 *adv7511,
>> /* Reading the EDID only works if the device is powered */
>> if (!adv7511->powered) {
>> unsigned int edid_i2c_addr =
>> - (adv7511->i2c_main->addr << 1) + 4;
>> + (adv7511->i2c_edid->addr << 1);
>>
>> __adv7511_power_on(adv7511);
>>
>> @@ -969,10 +969,10 @@ 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_CEC_I2C_ADDR_DEFAULT);
>> if (!adv->i2c_cec)
>> - return -ENOMEM;
>> + return -EINVAL;
>> i2c_set_clientdata(adv->i2c_cec, adv);
>>
>> adv->regmap_cec = devm_regmap_init_i2c(adv->i2c_cec,
>> @@ -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_EDID_I2C_ADDR_DEFAULT);
>> if (!adv7511->i2c_edid) {
>> - ret = -ENOMEM;
>> + ret = -EINVAL;
>> 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_PACKET_I2C_ADDR_DEFAULT);
>> + if (!adv7511->i2c_packet) {
>> + ret = -EINVAL;
>> + 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;
>> }
>