* Re: [PATCH 1/2] i2c: Add generic support passing secondary devices addresses
@ 2014-10-22 20:05 John de la Garza
0 siblings, 0 replies; 5+ messages in thread
From: John de la Garza @ 2014-10-22 20:05 UTC (permalink / raw)
To: linux-i2c-u79uwXL29TY76Z2rM5mHXA
Cc: jean-michel.hautbois-B+Q8N6RmIDZBDgjK7y7TUQ
instead of:
+ 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;
+ }
how about making the middle part like this to remove the repeated code
np = client->dev.of_node;
addr = default_addr;
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 = NULL;
}
^ permalink raw reply [flat|nested] 5+ messages in thread* [PATCH 1/2] i2c: Add generic support passing secondary devices addresses
@ 2014-10-22 15:30 Jean-Michel Hautbois
2014-10-22 23:37 ` Laurent Pinchart
0 siblings, 1 reply; 5+ messages in thread
From: Jean-Michel Hautbois @ 2014-10-22 15:30 UTC (permalink / raw)
To: linux-media-u79uwXL29TY76Z2rM5mHXA,
linux-i2c-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
devicetree-u79uwXL29TY76Z2rM5mHXA
Cc: laurent.pinchart-ryLnwIuWjnjg/C1BVhZhaw,
wsa-z923LK4zBo2bacvFa/9K2g, lars-Qo5EllUWu/uELgA04lAiVw,
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.
Signed-off-by: Jean-Michel Hautbois <jean-michel.hautbois-B+Q8N6RmIDZBDgjK7y7TUQ@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-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply related [flat|nested] 5+ messages in thread* Re: [PATCH 1/2] i2c: Add generic support passing secondary devices addresses
2014-10-22 15:30 Jean-Michel Hautbois
@ 2014-10-22 23:37 ` Laurent Pinchart
2014-10-23 5:59 ` Jean-Michel Hautbois
0 siblings, 1 reply; 5+ messages in thread
From: Laurent Pinchart @ 2014-10-22 23:37 UTC (permalink / raw)
To: Jean-Michel Hautbois
Cc: linux-media, linux-i2c, linux-kernel, devicetree, wsa, lars
Hi Jean-Michel,
Thank you for the patch.
On Wednesday 22 October 2014 17:30:47 Jean-Michel Hautbois wrote:
> 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/.
> Signed-off-by: Jean-Michel Hautbois <jean-michel.hautbois@vodalys.com>
> ---
> 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.
> + * @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.
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.
> + *
> + * 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.
> + 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.
> + } 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);
}
> +
> + 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.
> +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
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH 1/2] i2c: Add generic support passing secondary devices addresses
2014-10-22 23:37 ` Laurent Pinchart
@ 2014-10-23 5:59 ` Jean-Michel Hautbois
[not found] ` <CAL8zT=hM+ua3hdzVXAvQF9EcKbjou3HpHivfntJWYD1E+Ts8Xg-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
0 siblings, 1 reply; 5+ messages in thread
From: Jean-Michel Hautbois @ 2014-10-23 5:59 UTC (permalink / raw)
To: Laurent Pinchart
Cc: Linux Media Mailing List, linux-i2c-u79uwXL29TY76Z2rM5mHXA,
linux-kernel, devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
wsa-z923LK4zBo2bacvFa/9K2g, Lars-Peter Clausen
Hi Laurent,
Thank you for your review,
2014-10-23 1:37 GMT+02:00 Laurent Pinchart <laurent.pinchart-ryLnwIuWjnjg/C1BVhZhaw@public.gmane.org>:
> Hi Jean-Michel,
>
> Thank you for the patch.
>
> On Wednesday 22 October 2014 17:30:47 Jean-Michel Hautbois wrote:
>> 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...
>> Signed-off-by: Jean-Michel Hautbois <jean-michel.hautbois-B+Q8N6RmIDZBDgjK7y7TUQ@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
>
> 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
>> + * @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.
>
> 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.
>> + *
>> + * 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.
>
>> + 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.
>> + } 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
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2014-10-26 23:25 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2014-10-22 20:05 [PATCH 1/2] i2c: Add generic support passing secondary devices addresses John de la Garza
-- strict thread matches above, loose matches on Subject: below --
2014-10-22 15:30 Jean-Michel Hautbois
2014-10-22 23:37 ` Laurent Pinchart
2014-10-23 5:59 ` Jean-Michel Hautbois
[not found] ` <CAL8zT=hM+ua3hdzVXAvQF9EcKbjou3HpHivfntJWYD1E+Ts8Xg-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
2014-10-26 23:25 ` Laurent Pinchart
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).