Discussion:
[PATCH 1/2] i2c: Add generic support passing secondary devices addresses
(too old to reply)
Jean-Michel Hautbois
2014-10-22 15:30:47 UTC
Permalink
Raw Message
Some I2C devices have multiple addresses assigned, for example each address
corresponding to a different internal register map page of the device.
So far drivers which need support for this have handled this with a driver
specific and non-generic implementation, e.g. passing the additional address
via platform data.

This patch provides a new helper function called i2c_new_secondary_device()
which is intended to provide a generic way to get the secondary address
as well as instantiate a struct i2c_client for the secondary address.

The function expects a pointer to the primary i2c_client, a name
for the secondary address and an optional default address. The name is used
as a handle to specify which secondary address to get.

The default address is used as a fallback in case no secondary address
was explicitly specified. In case no secondary address and no default
address were specified the function returns NULL.

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

Signed-off-by: Jean-Michel Hautbois <jean-michel.hautbois-B+***@public.gmane.org>
---
drivers/i2c/i2c-core.c | 40 ++++++++++++++++++++++++++++++++++++++++
include/linux/i2c.h | 8 ++++++++
2 files changed, 48 insertions(+)

diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
index 2f90ac6..fd3b07c 100644
--- a/drivers/i2c/i2c-core.c
+++ b/drivers/i2c/i2c-core.c
@@ -1166,6 +1166,46 @@ struct i2c_client *i2c_new_dummy(struct i2c_adapter *adapter, u16 address)
}
EXPORT_SYMBOL_GPL(i2c_new_dummy);

+/**
+ * i2c_new_secondary_device - Helper to get the instantiated secondary address
+ * @client: Handle to the primary client
+ * @name: Handle to specify which secondary address to get
+ * @default_addr: Used as a fallback if no secondary address was specified
+ * Context: can sleep
+ *
+ * This returns an I2C client bound to the "dummy" driver based on DT parsing.
+ *
+ * This returns the new i2c client, which should be saved for later use with
+ * i2c_unregister_device(); or NULL to indicate an error.
+ */
+struct i2c_client *i2c_new_secondary_device(struct i2c_client *client,
+ const char *name,
+ u16 default_addr)
+{
+ int i;
+ u32 addr;
+ struct device_node *np;
+
+ np = client->dev.of_node;
+
+ if (np) {
+ i = of_property_match_string(np, "reg-names", name);
+ if (i >= 0)
+ of_property_read_u32_index(np, "reg", i, &addr);
+ else if (default_addr != 0)
+ addr = default_addr;
+ else
+ addr = NULL;
+ } else {
+ addr = default_addr;
+ }
+
+ dev_dbg(&client->adapter->dev, "Address for %s : 0x%x\n", name, addr);
+ return i2c_new_dummy(client->adapter, addr);
+}
+EXPORT_SYMBOL_GPL(i2c_new_secondary_device);
+
+
/* ------------------------------------------------------------------------- */

/* I2C bus adapters -- one roots each I2C or SMBUS segment */
diff --git a/include/linux/i2c.h b/include/linux/i2c.h
index b556e0a..8629287 100644
--- a/include/linux/i2c.h
+++ b/include/linux/i2c.h
@@ -322,6 +322,14 @@ extern int i2c_probe_func_quick_read(struct i2c_adapter *, unsigned short addr);
extern struct i2c_client *
i2c_new_dummy(struct i2c_adapter *adap, u16 address);

+/* Helper function providing a generic way to get the secondary address
+ * as well as a client handle to this extra address.
+ */
+extern struct i2c_client *
+i2c_new_secondary_device(struct i2c_client *client,
+ const char *name,
+ u16 default_addr);
+
extern void i2c_unregister_device(struct i2c_client *);
#endif /* I2C */
--
2.1.2

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-***@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Jean-Michel Hautbois
2014-10-22 15:30:48 UTC
Permalink
Raw Message
The ADV7604 has thirteen 256-byte maps that can be accessed via the mai=
n
I=C2=B2C ports. Each map has it own I=C2=B2C address and acts
as a standard slave device on the I=C2=B2C bus.

If nothing is defined, it uses default addresses.
The main purpose is using two adv76xx on the same i2c bus.

Signed-off-by: Jean-Michel Hautbois <jean-***@vodalys.com>
---
.../devicetree/bindings/media/i2c/adv7604.txt | 16 +++++-
drivers/media/i2c/adv7604.c | 59 ++++++++++++++=
--------
2 files changed, 53 insertions(+), 22 deletions(-)

diff --git a/Documentation/devicetree/bindings/media/i2c/adv7604.txt b/=
Documentation/devicetree/bindings/media/i2c/adv7604.txt
index 5c8b3e6..8486b5c 100644
--- a/Documentation/devicetree/bindings/media/i2c/adv7604.txt
+++ b/Documentation/devicetree/bindings/media/i2c/adv7604.txt
@@ -12,7 +12,10 @@ Required Properties:
- "adi,adv7611" for the ADV7611
- "adi,adv7604" for the ADV7604
=20
- - reg: I2C slave address
+ - reg: I2C slave addresses
+ The ADV7604 has thirteen 256-byte maps that can be accessed via th=
e main
+ I=C2=B2C ports. Each map has it own I=C2=B2C address and acts
+ as a standard slave device on the I=C2=B2C bus.
=20
- hpd-gpios: References to the GPIOs that control the HDMI hot-plug
detection pins, one per HDMI input. The active flag indicates the =
GPIO
@@ -33,6 +36,12 @@ The digital output port node must contain at least o=
ne endpoint.
Optional Properties:
=20
- reset-gpios: Reference to the GPIO connected to the device's reset=
pin.
+ - reg-names : Names of maps with programmable addresses.
+ It can contain any map needing another address than default one.
+ Possible maps names are :
+ADV7604 : "main", "avlink", "cec", "infoframe", "esdp", "dpp", "afe", =
"rep",
+ "edid", "hdmi", "test", "cp", "vdp"
+ADV7611 : "main", "cec", "infoframe", "afe", "rep", "edid", "hdmi", "c=
p"
=20
Optional Endpoint Properties:
=20
@@ -51,7 +60,10 @@ Example:
=20
***@4c {
compatible =3D "adi,adv7611";
- reg =3D <0x4c>;
+ /* edid page will be accessible @ 0x66 on i2c bus */
+ /* other maps keep their default addresses */
+ reg =3D <0x4c 0x66>;
+ reg-names =3D "main", "edid";
=20
reset-gpios =3D <&ioexp 0 GPIO_ACTIVE_LOW>;
hpd-gpios =3D <&ioexp 2 GPIO_ACTIVE_HIGH>;
diff --git a/drivers/media/i2c/adv7604.c b/drivers/media/i2c/adv7604.c
index 421035f..e4e30a2 100644
--- a/drivers/media/i2c/adv7604.c
+++ b/drivers/media/i2c/adv7604.c
@@ -326,6 +326,27 @@ static const struct adv7604_video_standards adv760=
4_prim_mode_hdmi_gr[] =3D {
{ },
};
=20
+struct adv7604_register {
+ const char *name;
+ u8 default_addr;
+};
+
+static const struct adv7604_register adv7604_secondary_names[] =3D {
+ [ADV7604_PAGE_IO] =3D { "main", 0x4c },
+ [ADV7604_PAGE_AVLINK] =3D { "avlink", 0x42 },
+ [ADV7604_PAGE_CEC] =3D { "cec", 0x40 },
+ [ADV7604_PAGE_INFOFRAME] =3D { "infoframe", 0x3e },
+ [ADV7604_PAGE_ESDP] =3D { "esdp", 0x38 },
+ [ADV7604_PAGE_DPP] =3D { "dpp", 0x3c },
+ [ADV7604_PAGE_AFE] =3D { "afe", 0x26 },
+ [ADV7604_PAGE_REP] =3D { "rep", 0x32 },
+ [ADV7604_PAGE_EDID] =3D { "edid", 0x36 },
+ [ADV7604_PAGE_HDMI] =3D { "hdmi", 0x34 },
+ [ADV7604_PAGE_TEST] =3D { "test", 0x30 },
+ [ADV7604_PAGE_CP] =3D { "cp", 0x22 },
+ [ADV7604_PAGE_VDP] =3D { "vdp", 0x24 },
+};
+
/* -------------------------------------------------------------------=
---- */
=20
static inline struct adv7604_state *to_state(struct v4l2_subdev *sd)
@@ -2528,13 +2549,26 @@ static void adv7604_unregister_clients(struct a=
dv7604_state *state)
}
=20
static struct i2c_client *adv7604_dummy_client(struct v4l2_subdev *sd,
- u8 addr, u8 io_reg)
+ unsigned int i)
{
struct i2c_client *client =3D v4l2_get_subdevdata(sd);
+ struct adv7604_platform_data *pdata =3D client->dev.platform_data;
+ unsigned int io_reg =3D 0xf2 + i;
+ unsigned int default_addr =3D io_read(sd, io_reg) >> 1;
+ struct i2c_client *new_client;
+
+ if (pdata && pdata->i2c_addresses[i])
+ new_client =3D i2c_new_dummy(client->adapter,
+ pdata->i2c_addresses[i]);
+ else
+ new_client =3D i2c_new_secondary_device(client,
+ adv7604_secondary_names[i].name,
+ adv7604_secondary_names[i].default_addr);
+
+ if (new_client)
+ io_write(sd, io_reg, new_client->addr << 1);
=20
- if (addr)
- io_write(sd, io_reg, addr << 1);
- return i2c_new_dummy(client->adapter, io_read(sd, io_reg) >> 1);
+ return new_client;
}
=20
static const struct adv7604_reg_seq adv7604_recommended_settings_afe[]=
=3D {
@@ -2718,20 +2752,6 @@ static int adv7604_parse_dt(struct adv7604_state=
*state)
/* Disable the interrupt for now as no DT-based board uses it. */
state->pdata.int1_config =3D ADV7604_INT1_CONFIG_DISABLED;
=20
- /* Use the default I2C addresses. */
- state->pdata.i2c_addresses[ADV7604_PAGE_AVLINK] =3D 0x42;
- state->pdata.i2c_addresses[ADV7604_PAGE_CEC] =3D 0x40;
- state->pdata.i2c_addresses[ADV7604_PAGE_INFOFRAME] =3D 0x3e;
- state->pdata.i2c_addresses[ADV7604_PAGE_ESDP] =3D 0x38;
- state->pdata.i2c_addresses[ADV7604_PAGE_DPP] =3D 0x3c;
- state->pdata.i2c_addresses[ADV7604_PAGE_AFE] =3D 0x26;
- state->pdata.i2c_addresses[ADV7604_PAGE_REP] =3D 0x32;
- state->pdata.i2c_addresses[ADV7604_PAGE_EDID] =3D 0x36;
- state->pdata.i2c_addresses[ADV7604_PAGE_HDMI] =3D 0x34;
- state->pdata.i2c_addresses[ADV7604_PAGE_TEST] =3D 0x30;
- state->pdata.i2c_addresses[ADV7604_PAGE_CP] =3D 0x22;
- state->pdata.i2c_addresses[ADV7604_PAGE_VDP] =3D 0x24;
-
/* Hardcode the remaining platform data fields. */
state->pdata.disable_pwrdnb =3D 0;
state->pdata.disable_cable_det_rst =3D 0;
@@ -2892,8 +2912,7 @@ static int adv7604_probe(struct i2c_client *clien=
t,
continue;
=20
state->i2c_clients[i] =3D
- adv7604_dummy_client(sd, state->pdata.i2c_addresses[i],
- 0xf2 + i);
+ adv7604_dummy_client(sd, i);
if (state->i2c_clients[i] =3D=3D NULL) {
err =3D -ENOMEM;
v4l2_err(sd, "failed to create i2c client %u\n", i);
--=20
2.1.2
Laurent Pinchart
2014-10-22 23:44:14 UTC
Permalink
Raw Message
Hi Jean-Michel,

Thank you for the patch.
The ADV7604 has thirteen 256-byte maps that can be accessed via the m=
ain
I=B2C ports. Each map has it own I=B2C address and acts
as a standard slave device on the I=B2C bus.
=20
If nothing is defined, it uses default addresses.
The main purpose is using two adv76xx on the same i2c bus.
=20
---
.../devicetree/bindings/media/i2c/adv7604.txt | 16 +++++-
drivers/media/i2c/adv7604.c | 59 ++++++++++++=
-------
2 files changed, 53 insertions(+), 22 deletions(-)
=20
diff --git a/Documentation/devicetree/bindings/media/i2c/adv7604.txt
b/Documentation/devicetree/bindings/media/i2c/adv7604.txt index
5c8b3e6..8486b5c 100644
--- a/Documentation/devicetree/bindings/media/i2c/adv7604.txt
+++ b/Documentation/devicetree/bindings/media/i2c/adv7604.txt
- "adi,adv7611" for the ADV7611
- "adi,adv7604" for the ADV7604
Given that I'll have comment on the independent patch that adds support=
for=20
the adv7604, you should rebase this series to remote that dependency if=
you=20
want to get it merged now, otherwise it will get delayed.
=20
- - reg: I2C slave address
+ - reg: I2C slave addresses
+ The ADV7604 has thirteen 256-byte maps that can be accessed via =
the
main
+ I=B2C ports. Each map has it own I=B2C address and acts
+ as a standard slave device on the I=B2C bus.
=20
- hpd-gpios: References to the GPIOs that control the HDMI hot-plu=
g
detection pins, one per HDMI input. The active flag indicates th=
e GPIO
@@ -33,6 +36,12 @@ The digital output port node must contain at least=
one
=20
- reset-gpios: Reference to the GPIO connected to the device's res=
et pin.
+ - reg-names : Names of maps with programmable addresses.
+ It can contain any map needing another address than default one.
+ADV7604 : "main", "avlink", "cec", "infoframe", "esdp", "dpp", "afe"=
,
"rep",
+ "edid", "hdmi", "test", "cp", "vdp"
If you rebase the series in order to avoid depending on the adv7604 DT =
support=20
patch this line should be moved to "adv7604: Add DT parsing support".
+ADV7611 : "main", "cec", "infoframe", "afe", "rep", "edid", "hdmi", =
"cp"
=20
=20
=20
compatible =3D "adi,adv7611";
- reg =3D <0x4c>;
+ /* other maps keep their default addresses */
+ reg =3D <0x4c 0x66>;
+ reg-names =3D "main", "edid";
=20
reset-gpios =3D <&ioexp 0 GPIO_ACTIVE_LOW>;
hpd-gpios =3D <&ioexp 2 GPIO_ACTIVE_HIGH>;
diff --git a/drivers/media/i2c/adv7604.c b/drivers/media/i2c/adv7604.=
c
index 421035f..e4e30a2 100644
--- a/drivers/media/i2c/adv7604.c
+++ b/drivers/media/i2c/adv7604.c
@@ -326,6 +326,27 @@ static const struct adv7604_video_standards
adv7604_prim_mode_hdmi_gr[] =3D { { },
};
=20
+struct adv7604_register {
+ const char *name;
+ u8 default_addr;
+};
+
+static const struct adv7604_register adv7604_secondary_names[] =3D {
+ [ADV7604_PAGE_IO] =3D { "main", 0x4c },
+ [ADV7604_PAGE_AVLINK] =3D { "avlink", 0x42 },
+ [ADV7604_PAGE_CEC] =3D { "cec", 0x40 },
+ [ADV7604_PAGE_INFOFRAME] =3D { "infoframe", 0x3e },
+ [ADV7604_PAGE_ESDP] =3D { "esdp", 0x38 },
+ [ADV7604_PAGE_DPP] =3D { "dpp", 0x3c },
+ [ADV7604_PAGE_AFE] =3D { "afe", 0x26 },
+ [ADV7604_PAGE_REP] =3D { "rep", 0x32 },
+ [ADV7604_PAGE_EDID] =3D { "edid", 0x36 },
+ [ADV7604_PAGE_HDMI] =3D { "hdmi", 0x34 },
+ [ADV7604_PAGE_TEST] =3D { "test", 0x30 },
+ [ADV7604_PAGE_CP] =3D { "cp", 0x22 },
+ [ADV7604_PAGE_VDP] =3D { "vdp", 0x24 },
+};
+
/* -----------------------------------------------------------------=
------
*/
=20
static inline struct adv7604_state *to_state(struct v4l2_subdev *sd)
@@ -2528,13 +2549,26 @@ static void adv7604_unregister_clients(struct
adv7604_state *state) }
=20
static struct i2c_client *adv7604_dummy_client(struct v4l2_subdev *s=
d,
- u8 addr, u8 io_reg)
+ unsigned int i)
{
struct i2c_client *client =3D v4l2_get_subdevdata(sd);
+ struct adv7604_platform_data *pdata =3D client->dev.platform_data;
+ unsigned int io_reg =3D 0xf2 + i;
+ unsigned int default_addr =3D io_read(sd, io_reg) >> 1;
The variable isn't used.
+ struct i2c_client *new_client;
+
+ if (pdata && pdata->i2c_addresses[i])
+ new_client =3D i2c_new_dummy(client->adapter,
+ pdata->i2c_addresses[i]);
+ else
+ new_client =3D i2c_new_secondary_device(client,
+ adv7604_secondary_names[i].name,
+ adv7604_secondary_names[i].default_addr);
+
+ if (new_client)
+ io_write(sd, io_reg, new_client->addr << 1);
=20
- if (addr)
- io_write(sd, io_reg, addr << 1);
- return i2c_new_dummy(client->adapter, io_read(sd, io_reg) >> 1);
+ return new_client;
}
=20
static const struct adv7604_reg_seq adv7604_recommended_settings_afe=
[] =3D {
@@ -2718,20 +2752,6 @@ static int adv7604_parse_dt(struct adv7604_sta=
te
*state) /* Disable the interrupt for now as no DT-based board uses it=
=2E */
state->pdata.int1_config =3D ADV7604_INT1_CONFIG_DISABLED;
=20
- /* Use the default I2C addresses. */
- state->pdata.i2c_addresses[ADV7604_PAGE_AVLINK] =3D 0x42;
- state->pdata.i2c_addresses[ADV7604_PAGE_CEC] =3D 0x40;
- state->pdata.i2c_addresses[ADV7604_PAGE_INFOFRAME] =3D 0x3e;
- state->pdata.i2c_addresses[ADV7604_PAGE_ESDP] =3D 0x38;
- state->pdata.i2c_addresses[ADV7604_PAGE_DPP] =3D 0x3c;
- state->pdata.i2c_addresses[ADV7604_PAGE_AFE] =3D 0x26;
- state->pdata.i2c_addresses[ADV7604_PAGE_REP] =3D 0x32;
- state->pdata.i2c_addresses[ADV7604_PAGE_EDID] =3D 0x36;
- state->pdata.i2c_addresses[ADV7604_PAGE_HDMI] =3D 0x34;
- state->pdata.i2c_addresses[ADV7604_PAGE_TEST] =3D 0x30;
- state->pdata.i2c_addresses[ADV7604_PAGE_CP] =3D 0x22;
- state->pdata.i2c_addresses[ADV7604_PAGE_VDP] =3D 0x24;
-
/* Hardcode the remaining platform data fields. */
state->pdata.disable_pwrdnb =3D 0;
state->pdata.disable_cable_det_rst =3D 0;
@@ -2892,8 +2912,7 @@ static int adv7604_probe(struct i2c_client *cli=
ent,
continue;
=20
state->i2c_clients[i] =3D
- adv7604_dummy_client(sd, state->pdata.i2c_addresses[i],
- 0xf2 + i);
+ adv7604_dummy_client(sd, i);
if (state->i2c_clients[i] =3D=3D NULL) {
err =3D -ENOMEM;
v4l2_err(sd, "failed to create i2c client %u\n", i);
--=20
Regards,

Laurent Pinchart
Jean-Michel Hautbois
2014-10-23 06:02:16 UTC
Permalink
Raw Message
Hi Laurent,

Thank you for reviewing,
Post by Laurent Pinchart
Hi Jean-Michel,
Thank you for the patch.
The ADV7604 has thirteen 256-byte maps that can be accessed via the =
main
Post by Laurent Pinchart
I=C2=B2C ports. Each map has it own I=C2=B2C address and acts
as a standard slave device on the I=C2=B2C bus.
If nothing is defined, it uses default addresses.
The main purpose is using two adv76xx on the same i2c bus.
m>
Post by Laurent Pinchart
---
.../devicetree/bindings/media/i2c/adv7604.txt | 16 +++++-
drivers/media/i2c/adv7604.c | 59 +++++++++++=
+-------
Post by Laurent Pinchart
2 files changed, 53 insertions(+), 22 deletions(-)
diff --git a/Documentation/devicetree/bindings/media/i2c/adv7604.txt
b/Documentation/devicetree/bindings/media/i2c/adv7604.txt index
5c8b3e6..8486b5c 100644
--- a/Documentation/devicetree/bindings/media/i2c/adv7604.txt
+++ b/Documentation/devicetree/bindings/media/i2c/adv7604.txt
- "adi,adv7611" for the ADV7611
- "adi,adv7604" for the ADV7604
Given that I'll have comment on the independent patch that adds suppo=
rt for
Post by Laurent Pinchart
the adv7604, you should rebase this series to remote that dependency =
if you
Post by Laurent Pinchart
want to get it merged now, otherwise it will get delayed.
Oh, you are right, of course, I forgot rebasing (as well as numbering
the patch as v2)...
Post by Laurent Pinchart
- - reg: I2C slave address
+ - reg: I2C slave addresses
+ The ADV7604 has thirteen 256-byte maps that can be accessed via=
the
Post by Laurent Pinchart
main
+ I=C2=B2C ports. Each map has it own I=C2=B2C address and acts
+ as a standard slave device on the I=C2=B2C bus.
- hpd-gpios: References to the GPIOs that control the HDMI hot-pl=
ug
Post by Laurent Pinchart
detection pins, one per HDMI input. The active flag indicates t=
he GPIO
Post by Laurent Pinchart
@@ -33,6 +36,12 @@ The digital output port node must contain at leas=
t one
Post by Laurent Pinchart
- reset-gpios: Reference to the GPIO connected to the device's re=
set pin.
Post by Laurent Pinchart
+ - reg-names : Names of maps with programmable addresses.
+ It can contain any map needing another address than de=
fault one.
Post by Laurent Pinchart
+ADV7604 : "main", "avlink", "cec", "infoframe", "esdp", "dpp", "afe=
",
Post by Laurent Pinchart
"rep",
+ "edid", "hdmi", "test", "cp", "vdp"
If you rebase the series in order to avoid depending on the adv7604 D=
T support
Post by Laurent Pinchart
patch this line should be moved to "adv7604: Add DT parsing support".
+ADV7611 : "main", "cec", "infoframe", "afe", "rep", "edid", "hdmi",=
"cp"
Post by Laurent Pinchart
compatible =3D "adi,adv7611";
- reg =3D <0x4c>;
+ /* other maps keep their default addresses */
+ reg =3D <0x4c 0x66>;
+ reg-names =3D "main", "edid";
reset-gpios =3D <&ioexp 0 GPIO_ACTIVE_LOW>;
hpd-gpios =3D <&ioexp 2 GPIO_ACTIVE_HIGH>;
diff --git a/drivers/media/i2c/adv7604.c b/drivers/media/i2c/adv7604=
=2Ec
Post by Laurent Pinchart
index 421035f..e4e30a2 100644
--- a/drivers/media/i2c/adv7604.c
+++ b/drivers/media/i2c/adv7604.c
@@ -326,6 +326,27 @@ static const struct adv7604_video_standards
adv7604_prim_mode_hdmi_gr[] =3D { { },
};
+struct adv7604_register {
+ const char *name;
+ u8 default_addr;
+};
+
+static const struct adv7604_register adv7604_secondary_names[] =3D =
{
Post by Laurent Pinchart
+ [ADV7604_PAGE_IO] =3D { "main", 0x4c },
+ [ADV7604_PAGE_AVLINK] =3D { "avlink", 0x42 },
+ [ADV7604_PAGE_CEC] =3D { "cec", 0x40 },
+ [ADV7604_PAGE_INFOFRAME] =3D { "infoframe", 0x3e },
+ [ADV7604_PAGE_ESDP] =3D { "esdp", 0x38 },
+ [ADV7604_PAGE_DPP] =3D { "dpp", 0x3c },
+ [ADV7604_PAGE_AFE] =3D { "afe", 0x26 },
+ [ADV7604_PAGE_REP] =3D { "rep", 0x32 },
+ [ADV7604_PAGE_EDID] =3D { "edid", 0x36 },
+ [ADV7604_PAGE_HDMI] =3D { "hdmi", 0x34 },
+ [ADV7604_PAGE_TEST] =3D { "test", 0x30 },
+ [ADV7604_PAGE_CP] =3D { "cp", 0x22 },
+ [ADV7604_PAGE_VDP] =3D { "vdp", 0x24 },
+};
+
/* ----------------------------------------------------------------=
-------
Post by Laurent Pinchart
*/
static inline struct adv7604_state *to_state(struct v4l2_subdev *sd=
)
Post by Laurent Pinchart
@@ -2528,13 +2549,26 @@ static void adv7604_unregister_clients(struc=
t
Post by Laurent Pinchart
adv7604_state *state) }
static struct i2c_client *adv7604_dummy_client(struct v4l2_subdev *=
sd,
Post by Laurent Pinchart
- u8 addr, u8 io=
_reg)
Post by Laurent Pinchart
+ unsigned int i)
{
struct i2c_client *client =3D v4l2_get_subdevdata(sd);
+ struct adv7604_platform_data *pdata =3D client->dev.platform_d=
ata;
Post by Laurent Pinchart
+ unsigned int io_reg =3D 0xf2 + i;
+ unsigned int default_addr =3D io_read(sd, io_reg) >> 1;
The variable isn't used.
Removed. Again, didn't check sufficiently my compiler warnings...
Post by Laurent Pinchart
+ struct i2c_client *new_client;
+
+ if (pdata && pdata->i2c_addresses[i])
+ new_client =3D i2c_new_dummy(client->adapter,
+ pdata->i2c_addresses[i]);
+ else
+ new_client =3D i2c_new_secondary_device(client,
+ adv7604_secondary_names[i].name,
+ adv7604_secondary_names[i].default_addr);
+
+ if (new_client)
+ io_write(sd, io_reg, new_client->addr << 1);
- if (addr)
- io_write(sd, io_reg, addr << 1);
- return i2c_new_dummy(client->adapter, io_read(sd, io_reg) >> 1=
);
Post by Laurent Pinchart
+ return new_client;
}
static const struct adv7604_reg_seq adv7604_recommended_settings_af=
e[] =3D {
Post by Laurent Pinchart
@@ -2718,20 +2752,6 @@ static int adv7604_parse_dt(struct adv7604_st=
ate
Post by Laurent Pinchart
*state) /* Disable the interrupt for now as no DT-based board uses i=
t. */
Post by Laurent Pinchart
state->pdata.int1_config =3D ADV7604_INT1_CONFIG_DISABLED;
- /* Use the default I2C addresses. */
- state->pdata.i2c_addresses[ADV7604_PAGE_AVLINK] =3D 0x42;
- state->pdata.i2c_addresses[ADV7604_PAGE_CEC] =3D 0x40;
- state->pdata.i2c_addresses[ADV7604_PAGE_INFOFRAME] =3D 0x3e;
- state->pdata.i2c_addresses[ADV7604_PAGE_ESDP] =3D 0x38;
- state->pdata.i2c_addresses[ADV7604_PAGE_DPP] =3D 0x3c;
- state->pdata.i2c_addresses[ADV7604_PAGE_AFE] =3D 0x26;
- state->pdata.i2c_addresses[ADV7604_PAGE_REP] =3D 0x32;
- state->pdata.i2c_addresses[ADV7604_PAGE_EDID] =3D 0x36;
- state->pdata.i2c_addresses[ADV7604_PAGE_HDMI] =3D 0x34;
- state->pdata.i2c_addresses[ADV7604_PAGE_TEST] =3D 0x30;
- state->pdata.i2c_addresses[ADV7604_PAGE_CP] =3D 0x22;
- state->pdata.i2c_addresses[ADV7604_PAGE_VDP] =3D 0x24;
-
/* Hardcode the remaining platform data fields. */
state->pdata.disable_pwrdnb =3D 0;
state->pdata.disable_cable_det_rst =3D 0;
@@ -2892,8 +2912,7 @@ static int adv7604_probe(struct i2c_client *cl=
ient,
Post by Laurent Pinchart
continue;
state->i2c_clients[i] =3D
- adv7604_dummy_client(sd, state->pdata.i2c_addr=
esses[i],
Post by Laurent Pinchart
- 0xf2 + i);
+ adv7604_dummy_client(sd, i);
if (state->i2c_clients[i] =3D=3D NULL) {
err =3D -ENOMEM;
v4l2_err(sd, "failed to create i2c client %u\n=
", i);
Post by Laurent Pinchart
--
Regards,
Laurent Pinchart
Thanks,
JM

Laurent Pinchart
2014-10-22 23:37:41 UTC
Permalink
Raw Message
Hi Jean-Michel,

Thank you for the patch.
Post by Jean-Michel Hautbois
Some I2C devices have multiple addresses assigned, for example each address
corresponding to a different internal register map page of the device.
So far drivers which need support for this have handled this with a driver
specific and non-generic implementation, e.g. passing the additional address
via platform data.
This patch provides a new helper function called i2c_new_secondary_device()
which is intended to provide a generic way to get the secondary address
as well as instantiate a struct i2c_client for the secondary address.
The function expects a pointer to the primary i2c_client, a name
for the secondary address and an optional default address. The name is used
as a handle to specify which secondary address to get.
The default address is used as a fallback in case no secondary address
was explicitly specified. In case no secondary address and no default
address were specified the function returns NULL.
For now the function only supports look-up of the secondary address
from devicetree, but it can be extended in the future
to for example support board files and/or ACPI.
As this is core code I believe the DT bindings should be documented somewhere
in Documentation/devicetree/bindings/i2c/.
Post by Jean-Michel Hautbois
---
drivers/i2c/i2c-core.c | 40 ++++++++++++++++++++++++++++++++++++++++
include/linux/i2c.h | 8 ++++++++
2 files changed, 48 insertions(+)
diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
index 2f90ac6..fd3b07c 100644
--- a/drivers/i2c/i2c-core.c
+++ b/drivers/i2c/i2c-core.c
@@ -1166,6 +1166,46 @@ struct i2c_client *i2c_new_dummy(struct i2c_adapter
*adapter, u16 address) }
EXPORT_SYMBOL_GPL(i2c_new_dummy);
+/**
+ * i2c_new_secondary_device - Helper to get the instantiated secondary address
It does more than that, it also creates the device.
Post by Jean-Michel Hautbois
+ * Context: can sleep
+ *
+ * This returns an I2C client bound to the "dummy" driver based on DT parsing.
Could you elaborate on that ? I would explain that the address is retrieved
from the firmware based on the name, and that default_addr is used in case the
firmware doesn't provide any information.
Post by Jean-Michel Hautbois
+ *
+ * This returns the new i2c client, which should be saved for later use with
+ * i2c_unregister_device(); or NULL to indicate an error.
+ */
+struct i2c_client *i2c_new_secondary_device(struct i2c_client *client,
+ const char *name,
+ u16 default_addr)
+{
+ int i;
+ u32 addr;
+ struct device_node *np;
+
+ np = client->dev.of_node;
+
+ if (np) {
+ i = of_property_match_string(np, "reg-names", name);
+ if (i >= 0)
+ of_property_read_u32_index(np, "reg", i, &addr);
This call could fail in which case addr will be uninitialized.
Post by Jean-Michel Hautbois
+ else if (default_addr != 0)
+ addr = default_addr;
+ else
+ addr = NULL;
addr isn't a pointer. I'm surprised the compiler hasn't warned you.
Post by Jean-Michel Hautbois
+ } else {
+ addr = default_addr;
+ }
The whole logic can be simplified to

struct device_node *np = client->dev.of_node;
u32 addr = default_addr;
int i;

if (np) {
i = of_property_match_string(np, "reg-names", name);
if (i >= 0)
of_property_read_u32_index(np, "reg", i, &addr);
}
Post by Jean-Michel Hautbois
+
+ dev_dbg(&client->adapter->dev, "Address for %s : 0x%x\n", name, addr);
+ return i2c_new_dummy(client->adapter, addr);
+}
+EXPORT_SYMBOL_GPL(i2c_new_secondary_device);
+
+
/*
-------------------------------------------------------------------------
*/
/* I2C bus adapters -- one roots each I2C or SMBUS segment */
diff --git a/include/linux/i2c.h b/include/linux/i2c.h
index b556e0a..8629287 100644
--- a/include/linux/i2c.h
+++ b/include/linux/i2c.h
@@ -322,6 +322,14 @@ extern int i2c_probe_func_quick_read(struct i2c_adapter
*, unsigned short addr); extern struct i2c_client *
i2c_new_dummy(struct i2c_adapter *adap, u16 address);
+/* Helper function providing a generic way to get the secondary address
+ * as well as a client handle to this extra address.
+ */
The function is already documented in i2c-core.c, I would ditch this comment.
Post by Jean-Michel Hautbois
+extern struct i2c_client *
+i2c_new_secondary_device(struct i2c_client *client,
+ const char *name,
+ u16 default_addr);
+
extern void i2c_unregister_device(struct i2c_client *);
#endif /* I2C */
--
Regards,

Laurent Pinchart
Jean-Michel Hautbois
2014-10-23 05:59:53 UTC
Permalink
Raw Message
Hi Laurent,

Thank you for your review,
Post by Laurent Pinchart
Hi Jean-Michel,
Thank you for the patch.
Post by Jean-Michel Hautbois
Some I2C devices have multiple addresses assigned, for example each address
corresponding to a different internal register map page of the device.
So far drivers which need support for this have handled this with a driver
specific and non-generic implementation, e.g. passing the additional address
via platform data.
This patch provides a new helper function called i2c_new_secondary_device()
which is intended to provide a generic way to get the secondary address
as well as instantiate a struct i2c_client for the secondary address.
The function expects a pointer to the primary i2c_client, a name
for the secondary address and an optional default address. The name is used
as a handle to specify which secondary address to get.
The default address is used as a fallback in case no secondary address
was explicitly specified. In case no secondary address and no default
address were specified the function returns NULL.
For now the function only supports look-up of the secondary address
from devicetree, but it can be extended in the future
to for example support board files and/or ACPI.
As this is core code I believe the DT bindings should be documented somewhere
in Documentation/devicetree/bindings/i2c/.
Mmh, probably yes, but I don't know where precisely, as all the
bindings are devices specific here...
Post by Laurent Pinchart
Post by Jean-Michel Hautbois
---
drivers/i2c/i2c-core.c | 40 ++++++++++++++++++++++++++++++++++++++++
include/linux/i2c.h | 8 ++++++++
2 files changed, 48 insertions(+)
diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
index 2f90ac6..fd3b07c 100644
--- a/drivers/i2c/i2c-core.c
+++ b/drivers/i2c/i2c-core.c
@@ -1166,6 +1166,46 @@ struct i2c_client *i2c_new_dummy(struct i2c_adapter
*adapter, u16 address) }
EXPORT_SYMBOL_GPL(i2c_new_dummy);
+/**
+ * i2c_new_secondary_device - Helper to get the instantiated secondary address
It does more than that, it also creates the device.
Right, how about :
+ * i2c_new_secondary_device - Helper to get the instantiated secondary address
+ * and create the associated device
Post by Laurent Pinchart
Post by Jean-Michel Hautbois
+ * Context: can sleep
+ *
+ * This returns an I2C client bound to the "dummy" driver based on DT parsing.
Could you elaborate on that ? I would explain that the address is retrieved
from the firmware based on the name, and that default_addr is used in case the
firmware doesn't provide any information.
Something like that ?
+ * This returns an I2C client bound to the "dummy" driver based on DT parsing.
+ * It retrieves the address based on the name.
+ * It uses default_addr if no information is provided by firmware.
Post by Laurent Pinchart
Post by Jean-Michel Hautbois
+ *
+ * This returns the new i2c client, which should be saved for later use with
+ * i2c_unregister_device(); or NULL to indicate an error.
+ */
+struct i2c_client *i2c_new_secondary_device(struct i2c_client *client,
+ const char *name,
+ u16 default_addr)
+{
+ int i;
+ u32 addr;
+ struct device_node *np;
+
+ np = client->dev.of_node;
+
+ if (np) {
+ i = of_property_match_string(np, "reg-names", name);
+ if (i >= 0)
+ of_property_read_u32_index(np, "reg", i, &addr);
This call could fail in which case addr will be uninitialized.
Post by Jean-Michel Hautbois
+ else if (default_addr != 0)
+ addr = default_addr;
+ else
+ addr = NULL;
addr isn't a pointer. I'm surprised the compiler hasn't warned you.
It has, just didn't notice it, sorry fir the noise.
Post by Laurent Pinchart
Post by Jean-Michel Hautbois
+ } else {
+ addr = default_addr;
+ }
The whole logic can be simplified to
struct device_node *np = client->dev.of_node;
u32 addr = default_addr;
int i;
if (np) {
i = of_property_match_string(np, "reg-names", name);
if (i >= 0)
of_property_read_u32_index(np, "reg", i, &addr);
}
OK, applied on my side.

Thanks,
JM
Loading...