2018-02-13 17:52:23

by Kieran Bingham

[permalink] [raw]
Subject: [PATCH v4 0/5] Add support for i2c_new_secondary_device

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

v4:
- Normalise the usage of the I2C term throughout
- Fix registration/cleanup of packet client in adv7511
- Rename adv76xx structures
- Update commit titles of dt-bindings patches.



Jean-Michel Hautbois (2):
dt-bindings: media: adv7604: Extend bindings to allow specifying slave
map addresses
media: adv7604: Add support for i2c_new_secondary_device

Kieran Bingham (3):
dt-bindings: adv7511: Extend bindings to allow specifying slave map
addresses
[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



2018-02-13 17:50:27

by Kieran Bingham

[permalink] [raw]
Subject: [PATCH v4 5/5] drm: adv7511: Add support for i2c_new_secondary_device

From: Kieran Bingham <[email protected]>

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

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

Signed-off-by: Kieran Bingham <[email protected]>
Reviewed-by: Laurent Pinchart <[email protected]>
---
v2:
- Update missing edid-i2c address setting
- Split out DT bindings
- Rename and move the I2C default addresses to their own section

v3:
- No change

v4:
- Change registration order of packet/cec to fix error path and
simplify code change.
- Collect Laurent's RB tag

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..73d8ccb97742 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)

@@ -321,6 +326,7 @@ enum adv7511_type {
struct adv7511 {
struct i2c_client *i2c_main;
struct i2c_client *i2c_edid;
+ struct i2c_client *i2c_packet;
struct i2c_client *i2c_cec;

struct regmap *regmap;
diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
index efa29db5fc2b..802bc433f54a 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,23 +1151,34 @@ 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);
+
+ adv7511->i2c_packet = i2c_new_secondary_device(i2c, "packet",
+ ADV7511_PACKET_I2C_ADDR_DEFAULT);
+ if (!adv7511->i2c_packet) {
+ ret = -EINVAL;
+ goto err_i2c_unregister_edid;
+ }
+
+ regmap_write(adv7511->regmap, ADV7511_REG_PACKET_I2C_ADDR,
+ adv7511->i2c_packet->addr << 1);
+
ret = adv7511_init_cec_regmap(adv7511);
if (ret)
- goto err_i2c_unregister_edid;
+ goto err_i2c_unregister_packet;
+
+ regmap_write(adv7511->regmap, ADV7511_REG_CEC_I2C_ADDR,
+ adv7511->i2c_cec->addr << 1);

INIT_WORK(&adv7511->hpd_work, adv7511_hpd_work);

@@ -1207,6 +1216,8 @@ static int adv7511_probe(struct i2c_client *i2c, const struct i2c_device_id *id)
i2c_unregister_device(adv7511->i2c_cec);
if (adv7511->cec_clk)
clk_disable_unprepare(adv7511->cec_clk);
+err_i2c_unregister_packet:
+ i2c_unregister_device(adv7511->i2c_packet);
err_i2c_unregister_edid:
i2c_unregister_device(adv7511->i2c_edid);
uninit_regulators:
@@ -1233,6 +1244,7 @@ static int adv7511_remove(struct i2c_client *i2c)

cec_unregister_adapter(adv7511->cec_adap);

+ i2c_unregister_device(adv7511->i2c_packet);
i2c_unregister_device(adv7511->i2c_edid);

return 0;
--
2.7.4


2018-02-13 17:51:10

by Kieran Bingham

[permalink] [raw]
Subject: [PATCH v4 3/5] [RFT] ARM: dts: wheat: Fix ADV7513 address usage

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.

v4:
- Normalise I2C usage

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


2018-02-13 17:51:39

by Kieran Bingham

[permalink] [raw]
Subject: [PATCH v4 4/5] media: adv7604: Add support for i2c_new_secondary_device

From: Jean-Michel Hautbois <[email protected]>

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

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

Signed-off-by: Jean-Michel Hautbois <[email protected]>
[Kieran: Re-adapted for mainline]
Signed-off-by: Kieran Bingham <[email protected]>
Reviewed-by: Laurent Pinchart <[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()

v3:
- No change

v4:
- s/struct adv76xx_register/struct adv76xx_register_map/
- s/adv76xx_secondary_names/adv76xx_default_addresses/
- Normalise I2C usage (s/I²C/I2C/)

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..d52c624a2970 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_map {
+ const char *name;
+ u8 default_addr;
+};
+
+static const struct adv76xx_register_map adv76xx_default_addresses[] = {
+ [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 page)
{
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 + page;
+ struct i2c_client *new_client;
+
+ if (pdata && pdata->i2c_addresses[page])
+ new_client = i2c_new_dummy(client->adapter,
+ pdata->i2c_addresses[page]);
+ else
+ new_client = i2c_new_secondary_device(client,
+ adv76xx_default_addresses[page].name,
+ adv76xx_default_addresses[page].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


2018-02-13 17:52:12

by Kieran Bingham

[permalink] [raw]
Subject: [PATCH v4 1/5] dt-bindings: media: adv7604: Extend bindings to allow specifying slave map addresses

From: Jean-Michel Hautbois <[email protected]>

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

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]>
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.

v4:
- Collect Laurents RB tag
- Adapt commit title
- Normalise I2C usage (I²C is harder to grep for)

.../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..dcf57e7c60eb 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 I2C ports. Each map has it own I2C address and acts as a standard
+ slave device on the I2C 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


2018-02-13 17:52:32

by Kieran Bingham

[permalink] [raw]
Subject: [PATCH v4 2/5] dt-bindings: adv7511: Extend bindings to allow specifying slave map addresses

From: Kieran Bingham <[email protected]>

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

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]>
Reviewed-by: Laurent Pinchart <[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.

v4:
- Update commit title
- Collect Laurent's RB tag
- Fix nitpickings
- Normalise I2C usage (I²C is harder to grep for)

.../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..2c887536258c 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 I2C 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


2018-04-25 07:17:37

by Archit Taneja

[permalink] [raw]
Subject: Re: [PATCH v4 2/5] dt-bindings: adv7511: Extend bindings to allow specifying slave map addresses



On Tuesday 13 February 2018 11:18 PM, Kieran Bingham wrote:
> From: Kieran Bingham <[email protected]>
>
> The ADV7511 has four 256-byte maps that can be accessed via the main I2C
> ports. Each map has it own I2C address and acts as a standard slave
> device on the I2C bus.
>
> 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.
>

Queued to drm-misc-next

> Signed-off-by: Kieran Bingham <[email protected]>
> Reviewed-by: Rob Herring <[email protected]>
> Reviewed-by: Laurent Pinchart <[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.
>
> v4:
> - Update commit title
> - Collect Laurent's RB tag
> - Fix nitpickings
> - Normalise I2C usage (I²C is harder to grep for)
>
> .../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..2c887536258c 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 I2C 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>;
>

2018-04-25 07:17:48

by Archit Taneja

[permalink] [raw]
Subject: Re: [PATCH v4 5/5] drm: adv7511: Add support for i2c_new_secondary_device



On Tuesday 13 February 2018 11:18 PM, Kieran Bingham wrote:
> From: Kieran Bingham <[email protected]>
>
> The ADV7511 has four 256-byte maps that can be accessed via the main I2C
> ports. Each map has it own I2C address and acts as a standard slave
> device on the I2C bus.
>
> Allow a device tree node to override the default addresses so that
> address conflicts with other devices on the same bus may be resolved at
> the board description level.

Queued to drm-misc-next

>
> Signed-off-by: Kieran Bingham <[email protected]>
> Reviewed-by: Laurent Pinchart <[email protected]>
> ---
> v2:
> - Update missing edid-i2c address setting
> - Split out DT bindings
> - Rename and move the I2C default addresses to their own section
>
> v3:
> - No change
>
> v4:
> - Change registration order of packet/cec to fix error path and
> simplify code change.
> - Collect Laurent's RB tag
>
> 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..73d8ccb97742 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)
>
> @@ -321,6 +326,7 @@ enum adv7511_type {
> struct adv7511 {
> struct i2c_client *i2c_main;
> struct i2c_client *i2c_edid;
> + struct i2c_client *i2c_packet;
> struct i2c_client *i2c_cec;
>
> struct regmap *regmap;
> diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
> index efa29db5fc2b..802bc433f54a 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,23 +1151,34 @@ 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);
> +
> + adv7511->i2c_packet = i2c_new_secondary_device(i2c, "packet",
> + ADV7511_PACKET_I2C_ADDR_DEFAULT);
> + if (!adv7511->i2c_packet) {
> + ret = -EINVAL;
> + goto err_i2c_unregister_edid;
> + }
> +
> + regmap_write(adv7511->regmap, ADV7511_REG_PACKET_I2C_ADDR,
> + adv7511->i2c_packet->addr << 1);
> +
> ret = adv7511_init_cec_regmap(adv7511);
> if (ret)
> - goto err_i2c_unregister_edid;
> + goto err_i2c_unregister_packet;
> +
> + regmap_write(adv7511->regmap, ADV7511_REG_CEC_I2C_ADDR,
> + adv7511->i2c_cec->addr << 1);
>
> INIT_WORK(&adv7511->hpd_work, adv7511_hpd_work);
>
> @@ -1207,6 +1216,8 @@ static int adv7511_probe(struct i2c_client *i2c, const struct i2c_device_id *id)
> i2c_unregister_device(adv7511->i2c_cec);
> if (adv7511->cec_clk)
> clk_disable_unprepare(adv7511->cec_clk);
> +err_i2c_unregister_packet:
> + i2c_unregister_device(adv7511->i2c_packet);
> err_i2c_unregister_edid:
> i2c_unregister_device(adv7511->i2c_edid);
> uninit_regulators:
> @@ -1233,6 +1244,7 @@ static int adv7511_remove(struct i2c_client *i2c)
>
> cec_unregister_adapter(adv7511->cec_adap);
>
> + i2c_unregister_device(adv7511->i2c_packet);
> i2c_unregister_device(adv7511->i2c_edid);
>
> return 0;
>