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
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: 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: 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: 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
.../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..f1b5466b1ca7 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: 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.
.../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..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>;
--
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]>
---
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..c94f330392ee 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
On 02/12/2018 07:11 PM, Kieran Bingham wrote:
[...]
> + /*
> + * 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>;
To have the correct semantics this should be:
reg = <0x3d>, <0x2d>, <0x4d>, <0x5d>;
It is a set of 4 single cell addresses. The other thing is a single 4 cell
address. It will get compiled to the same bytes, but the DT tools should
complain about it, because it doesn't match #address-cells.
> + 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>;
Same here.
> + reg-names = "main", "cec", "edid", "packet";
>
> adi,input-depth = <8>;
> adi,input-colorspace = "rgb";
>
Hi Lars,
Thanks for your review!
On 12/02/18 18:24, Lars-Peter Clausen wrote:
> On 02/12/2018 07:11 PM, Kieran Bingham wrote:
> [...]
>> + /*
>> + * 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>;
>
> To have the correct semantics this should be:
> reg = <0x3d>, <0x2d>, <0x4d>, <0x5d>;>
> It is a set of 4 single cell addresses. The other thing is a single 4 cell
> address. It will get compiled to the same bytes, but the DT tools should
> complain about it, because it doesn't match #address-cells.
Not to mention the spurious comma's!!! (at least I marked the patch RFT :D)
I'll resend a v2.1 RFT here, and update my local changes (with the same fault,
sans comma) to my other DT files!
Thanks for the fast review.
--
Kieran
>
>> + 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>;
>
> Same here.
>
>> + reg-names = "main", "cec", "edid", "packet";
>>
>> adi,input-depth = <8>;
>> adi,input-colorspace = "rgb";
>>
>
On Mon, Feb 12, 2018 at 06:11:57PM +0000, Kieran Bingham wrote:
> + 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);
There is another goto which needs to be updated if adv7511_cec_init()
fails.
> @@ -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)
regards,
dan carpenter
Hi Dan
Thank you for the review,
On 13/02/18 07:23, Dan Carpenter wrote:
> On Mon, Feb 12, 2018 at 06:11:57PM +0000, Kieran Bingham wrote:
>> + 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);
>
> There is another goto which needs to be updated if adv7511_cec_init()
> fails.
Aha - thanks - I'll look into this now.
>
>> @@ -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)
>
>
> regards,
> dan carpenter
>
Hi Kieran,
I love your patch! Yet something to improve:
[auto build test ERROR on linuxtv-media/master]
[also build test ERROR on v4.16-rc1 next-20180213]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
url: https://github.com/0day-ci/linux/commits/Kieran-Bingham/dt-bindings-media-adv7604-Add-support-for-i2c_new_secondary_device/20180214-032943
base: git://linuxtv.org/media_tree.git master
config: arm-allyesconfig (attached as .config)
compiler: arm-linux-gnueabi-gcc (Debian 7.2.0-11) 7.2.0
reproduce:
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
make.cross ARCH=arm
All errors (new ones prefixed by >>):
>> Error: arch/arm/boot/dts/r8a7792-wheat.dts:251.24-25 syntax error
FATAL ERROR: Unable to parse input tree
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation