[PATCHv6 00/11] i2c: Relax mandatory I2C ID table passing

[PATCHv6 00/11] i2c: Relax mandatory I2C ID table passing

[Kieran Bingham]

Version 6 of this patchset adds two new patches on top of the existing
submission to resolve the (final?) pending question from the previous
discussions during v5 [0]. Javier's tags are also collected here.

In particular, the remaining issue was that a user could provide a full
vendor prefixed string to the sysfs interface to instantiate an i2c
device. In version 5 of this submission this would not have matched
against a device driver as highlighted by the test patch provided by
Wolfram.

As such, in this series I have extended i2c_of_match_device_strip_vendor
so that it will also perform a search against a full compatible string
for the matching process. This was not done by the core OF framework as
the sysfs interface does not create an of_node to match against. If so
desired, this function could now be renamed to i2c_of_match_device_sysfs

Patch 11/11 of this series brings back Wolframs test case (correctly this
time) and also serves as a demonstration as to how to map the ID's when
matched against an of-style node. Note that this patch is *not* signed
off and not suitable for integration.

[0] [PATCHv5 0/8] 2c: Relax mandatory I2C ID table passing
    https://lkml.org/lkml/2016/5/4/534

Kieran Bingham (3):
  i2c: match dt-style device names from sysfs interface
  i2c: match vendorless strings on the internal string length
  [TESTPATCH] rtc: convert ds1307 to interim probe_new

Lee Jones (8):
  i2c: Add pointer dereference protection to i2c_match_id()
  i2c: Add the ability to match device to compatible string without an
    of_node
  i2c: Match using traditional OF methods, then by vendor-less
    compatible strings
  i2c: Make I2C ID tables non-mandatory for DT'ed devices
  i2c: Export i2c_match_id() for direct use by device drivers
  i2c: Provide a temporary .probe_new() call-back type
  mfd: 88pm860x: Move over to new I2C device .probe() call
  mfd: as3722: Rid driver of superfluous I2C device ID structure

 drivers/i2c/i2c-core.c      | 85 ++++++++++++++++++++++++++++++++++++++++-----
 drivers/mfd/88pm860x-core.c |  5 ++-
 drivers/mfd/as3722.c        | 12 ++-----
 drivers/rtc/rtc-ds1307.c    | 53 ++++++++++++++++------------
 include/linux/i2c.h         | 22 +++++++++++-
 5 files changed, 132 insertions(+), 45 deletions(-)

-- 
2.7.4

[PATCHv6 01/11] i2c: Add pointer dereference protection to i2c_match_id()

[Kieran Bingham]

From: Lee Jones <lee.jones@linaro.org>

Here we're providing dereference protection for i2c_match_id(), which
saves us having to do it each time it's called.  We're also stripping
out the (now) needless checks in i2c_device_match().  This patch paves
the way for other, similar code trimming.

Acked-by: Grant Likely <grant.likely@linaro.org>
Signed-off-by: Lee Jones <lee.jones@linaro.org>
Tested-by: Kieran Bingham <kieran@bingham.xyz>
Reviewed-by: Javier Martinez Canillas <javier@osg.samsung.com>
Tested-by: Javier Martinez Canillas <javier@osg.samsung.com>
Signed-off-by: Kieran Bingham <kieran@bingham.xyz>
---
 drivers/i2c/i2c-core.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
index da3a02ef4a31..eec4d4d81bf8 100644
--- a/drivers/i2c/i2c-core.c
+++ b/drivers/i2c/i2c-core.c
@@ -589,6 +589,9 @@ static inline int acpi_i2c_install_space_handler(struct i2c_adapter *adapter)
 static const struct i2c_device_id *i2c_match_id(const struct i2c_device_id *id,
 						const struct i2c_client *client)
 {
+	if (!(id && client))
+		return NULL;
+
 	while (id->name[0]) {
 		if (strcmp(client->name, id->name) == 0)
 			return id;
@@ -602,8 +605,6 @@ static int i2c_device_match(struct device *dev, struct device_driver *drv)
 	struct i2c_client	*client = i2c_verify_client(dev);
 	struct i2c_driver	*driver;
 
-	if (!client)
-		return 0;
 
 	/* Attempt an OF style match */
 	if (of_driver_match_device(dev, drv))
@@ -614,9 +615,10 @@ static int i2c_device_match(struct device *dev, struct device_driver *drv)
 		return 1;
 
 	driver = to_i2c_driver(drv);
-	/* match on an id table if there is one */
-	if (driver->id_table)
-		return i2c_match_id(driver->id_table, client) != NULL;
+
+	/* Finally an I2C match */
+	if (i2c_match_id(driver->id_table, client))
+		return 1;
 
 	return 0;
 }
-- 
2.7.4

[PATCHv6 02/11] i2c: Add the ability to match device to compatible string without an of_node

[Kieran Bingham]

From: Lee Jones <lee.jones@linaro.org>

A great deal of I2C devices are currently matched via DT node name, and
as such the compatible naming convention of '<vendor>,<device>' has gone
somewhat awry - some nodes don't supply one, some supply an arbitrary
string and others the correct device name with an arbitrary vendor prefix.

In an effort to correct this problem we have to supply a mechanism to
match a device by compatible string AND by simple device name.  This
function strips off the '<vendor>,' part of a supplied compatible string
and attempts to match without it.

The plan is to remove this function once all of the compatible strings
for each device have been brought into line.

Acked-by: Grant Likely <grant.likely@linaro.org>
Signed-off-by: Lee Jones <lee.jones@linaro.org>
[Kieran: strnicmp to strncasecmp]
Tested-by: Kieran Bingham <kieran@bingham.xyz>
Reviewed-by: Javier Martinez Canillas <javier@osg.samsung.com>
Tested-by: Javier Martinez Canillas <javier@osg.samsung.com>
Signed-off-by: Kieran Bingham <kieran@bingham.xyz>
---
 drivers/i2c/i2c-core.c | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)

diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
index eec4d4d81bf8..a699ca541702 100644
--- a/drivers/i2c/i2c-core.c
+++ b/drivers/i2c/i2c-core.c
@@ -1666,6 +1666,27 @@ struct i2c_adapter *of_get_i2c_adapter_by_node(struct device_node *node)
 	return adapter;
 }
 EXPORT_SYMBOL(of_get_i2c_adapter_by_node);
+
+static const struct of_device_id*
+i2c_of_match_device_strip_vendor(const struct of_device_id *matches,
+				  struct i2c_client *client)
+{
+	const char *name;
+
+	for (; matches->compatible[0]; matches++) {
+		name = strchr(matches->compatible, ',');
+		if (!name)
+			name = matches->compatible;
+		else
+			name++;
+
+		if (!strncasecmp(client->name, name, strlen(client->name)))
+			return matches;
+	}
+
+	return NULL;
+}
+
 #else
 static void of_i2c_register_devices(struct i2c_adapter *adap) { }
 #endif /* CONFIG_OF */
-- 
2.7.4

[PATCHv6 03/11] i2c: Match using traditional OF methods, then by vendor-less compatible strings

[Kieran Bingham]

From: Lee Jones <lee.jones@linaro.org>

This function provides a single call for all I2C devices which need to
match firstly using traditional OF means i.e by of_node, then if that
fails we attempt to match using the supplied I2C client name with a
list of supplied compatible strings with the '<vendor>,' string
removed.  The latter is required due to the unruly naming conventions
used currently by I2C devices.

Acked-by: Grant Likely <grant.likely@linaro.org>
Signed-off-by: Lee Jones <lee.jones@linaro.org>
[Kieran: Fix static inline usage on !CONFIG_OF]
Tested-by: Kieran Bingham <kieran@bingham.xyz>
Reviewed-by: Javier Martinez Canillas <javier@osg.samsung.com>
Tested-by: Javier Martinez Canillas <javier@osg.samsung.com>
Signed-off-by: Kieran Bingham <kieran@bingham.xyz>
---
 drivers/i2c/i2c-core.c | 16 ++++++++++++++++
 include/linux/i2c.h    | 12 ++++++++++++
 2 files changed, 28 insertions(+)

diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
index a699ca541702..d5eca0dc32be 100644
--- a/drivers/i2c/i2c-core.c
+++ b/drivers/i2c/i2c-core.c
@@ -1687,6 +1687,22 @@ i2c_of_match_device_strip_vendor(const struct of_device_id *matches,
 	return NULL;
 }
 
+const struct of_device_id
+*i2c_of_match_device(const struct of_device_id *matches,
+		     struct i2c_client *client)
+{
+	const struct of_device_id *match;
+
+	if (!(client && matches))
+		return NULL;
+
+	match = of_match_device(matches, &client->dev);
+	if (match)
+		return match;
+
+	return i2c_of_match_device_strip_vendor(matches, client);
+}
+EXPORT_SYMBOL_GPL(i2c_of_match_device);
 #else
 static void of_i2c_register_devices(struct i2c_adapter *adap) { }
 #endif /* CONFIG_OF */
diff --git a/include/linux/i2c.h b/include/linux/i2c.h
index fffdc270ca18..2608e731e401 100644
--- a/include/linux/i2c.h
+++ b/include/linux/i2c.h
@@ -748,6 +748,10 @@ extern struct i2c_adapter *of_find_i2c_adapter_by_node(struct device_node *node)
 /* must call i2c_put_adapter() when done with returned i2c_adapter device */
 struct i2c_adapter *of_get_i2c_adapter_by_node(struct device_node *node);
 
+extern const struct of_device_id
+*i2c_of_match_device(const struct of_device_id *matches,
+		     struct i2c_client *client);
+
 #else
 
 static inline struct i2c_client *of_find_i2c_device_by_node(struct device_node *node)
@@ -764,6 +768,14 @@ static inline struct i2c_adapter *of_get_i2c_adapter_by_node(struct device_node
 {
 	return NULL;
 }
+
+static inline const struct of_device_id
+*i2c_of_match_device(const struct of_device_id *matches,
+		     struct i2c_client *client)
+{
+	return NULL;
+}
+
 #endif /* CONFIG_OF */
 
 #endif /* _LINUX_I2C_H */
-- 
2.7.4

[PATCHv6 04/11] i2c: Make I2C ID tables non-mandatory for DT'ed devices

[Kieran Bingham]

From: Lee Jones <lee.jones@linaro.org>

Currently the I2C framework insists on devices supplying an I2C ID
table.  Many of the devices which do so unnecessarily adding quite a
few wasted lines to kernel code.  This patch allows drivers a means
to 'not' supply the aforementioned table and match on DT match tables
instead.

Acked-by: Grant Likely <grant.likely@linaro.org>
Signed-off-by: Lee Jones <lee.jones@linaro.org>
Tested-by: Kieran Bingham <kieran@bingham.xyz>
Reviewed-by: Javier Martinez Canillas <javier@osg.samsung.com>
Tested-by: Javier Martinez Canillas <javier@osg.samsung.com>
Signed-off-by: Kieran Bingham <kieran@bingham.xyz>
---
 drivers/i2c/i2c-core.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
index d5eca0dc32be..a69b47990ae7 100644
--- a/drivers/i2c/i2c-core.c
+++ b/drivers/i2c/i2c-core.c
@@ -607,7 +607,7 @@ static int i2c_device_match(struct device *dev, struct device_driver *drv)
 
 
 	/* Attempt an OF style match */
-	if (of_driver_match_device(dev, drv))
+	if (i2c_of_match_device(drv->of_match_table, client))
 		return 1;
 
 	/* Then ACPI style match */
@@ -833,7 +833,15 @@ static int i2c_device_probe(struct device *dev)
 	}
 
 	driver = to_i2c_driver(dev->driver);
-	if (!driver->probe || !driver->id_table)
+	if (!driver->probe)
+		return -EINVAL;
+
+	/*
+	 * An I2C ID table is not mandatory, if and only if, a suitable Device
+	 * Tree match table entry is supplied for the probing device.
+	 */
+	if (!driver->id_table &&
+	    !i2c_of_match_device(dev->driver->of_match_table, client))
 		return -ENODEV;
 
 	if (client->flags & I2C_CLIENT_WAKE) {
-- 
2.7.4

[PATCHv6 05/11] i2c: Export i2c_match_id() for direct use by device drivers

[Kieran Bingham]

From: Lee Jones <lee.jones@linaro.org>

When there was no other way to match a I2C device to driver i2c_match_id()
was exclusively used.  However, now there are other types of tables which
are commonly supplied, matching on an i2c_device_id table is used less
frequently.  Instead of _always_ calling i2c_match_id() from within the
framework, we only need to do so from drivers which have no other way of
matching.  This patch makes i2c_match_id() available to the aforementioned
device drivers.

Acked-by: Grant Likely <grant.likely@linaro.org>
Signed-off-by: Lee Jones <lee.jones@linaro.org>
Tested-by: Kieran Bingham <kieran@bingham.xyz>
Reviewed-by: Javier Martinez Canillas <javier@osg.samsung.com>
Tested-by: Javier Martinez Canillas <javier@osg.samsung.com>
Signed-off-by: Kieran Bingham <kieran@bingham.xyz>
---
 drivers/i2c/i2c-core.c | 3 ++-
 include/linux/i2c.h    | 2 ++
 2 files changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
index a69b47990ae7..f20c9b229570 100644
--- a/drivers/i2c/i2c-core.c
+++ b/drivers/i2c/i2c-core.c
@@ -586,7 +586,7 @@ static inline int acpi_i2c_install_space_handler(struct i2c_adapter *adapter)
 
 /* ------------------------------------------------------------------------- */
 
-static const struct i2c_device_id *i2c_match_id(const struct i2c_device_id *id,
+const struct i2c_device_id *i2c_match_id(const struct i2c_device_id *id,
 						const struct i2c_client *client)
 {
 	if (!(id && client))
@@ -599,6 +599,7 @@ static const struct i2c_device_id *i2c_match_id(const struct i2c_device_id *id,
 	}
 	return NULL;
 }
+EXPORT_SYMBOL_GPL(i2c_match_id);
 
 static int i2c_device_match(struct device *dev, struct device_driver *drv)
 {
diff --git a/include/linux/i2c.h b/include/linux/i2c.h
index 2608e731e401..39f1131a58d1 100644
--- a/include/linux/i2c.h
+++ b/include/linux/i2c.h
@@ -243,6 +243,8 @@ struct i2c_client {
 
 extern struct i2c_client *i2c_verify_client(struct device *dev);
 extern struct i2c_adapter *i2c_verify_adapter(struct device *dev);
+extern const struct i2c_device_id *i2c_match_id(const struct i2c_device_id *id,
+					const struct i2c_client *client);
 
 static inline struct i2c_client *kobj_to_i2c_client(struct kobject *kobj)
 {
-- 
2.7.4

[PATCHv6 06/11] i2c: Provide a temporary .probe_new() call-back type

[Kieran Bingham]

From: Lee Jones <lee.jones@linaro.org>

This will aid the seamless removal of the current probe()'s, more
commonly unused than used second parameter.  Most I2C drivers can
simply switch over to the new interface, others which have DT
support can use its own matching instead and others can call
i2c_match_id() themselves.  This brings I2C's device probe method
into line with other similar interfaces in the kernel and prevents
the requirement to pass an i2c_device_id table.

Suggested-by: Grant Likely <grant.likely@linaro.org>
Signed-off-by: Lee Jones <lee.jones@linaro.org>
[Kieran: fix rebase conflicts and adapt for dev_pm_domain_{attach,detach}]
Tested-by: Kieran Bingham <kieran@bingham.xyz>
Reviewed-by: Javier Martinez Canillas <javier@osg.samsung.com>
Tested-by: Javier Martinez Canillas <javier@osg.samsung.com>
Signed-off-by: Kieran Bingham <kieran@bingham.xyz>
---
Changes since v4 [Kieran]
 - Rename .probe2 to probe_new
 - Checkpatch warnings fixed
---
 drivers/i2c/i2c-core.c | 15 ++++++++++++---
 include/linux/i2c.h    |  8 +++++++-
 2 files changed, 19 insertions(+), 4 deletions(-)

diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
index f20c9b229570..2003334b5246 100644
--- a/drivers/i2c/i2c-core.c
+++ b/drivers/i2c/i2c-core.c
@@ -834,8 +834,6 @@ static int i2c_device_probe(struct device *dev)
 	}
 
 	driver = to_i2c_driver(dev->driver);
-	if (!driver->probe)
-		return -EINVAL;
 
 	/*
 	 * An I2C ID table is not mandatory, if and only if, a suitable Device
@@ -877,7 +875,18 @@ static int i2c_device_probe(struct device *dev)
 	if (status == -EPROBE_DEFER)
 		goto err_clear_wakeup_irq;
 
-	status = driver->probe(client, i2c_match_id(driver->id_table, client));
+	/*
+	 * When there are no more users of probe(),
+	 * rename probe_new to probe.
+	 */
+	if (driver->probe_new)
+		status = driver->probe_new(client);
+	else if (driver->probe)
+		status = driver->probe(client,
+				       i2c_match_id(driver->id_table, client));
+	else
+		status = -EINVAL;
+
 	if (status)
 		goto err_detach_pm_domain;
 
diff --git a/include/linux/i2c.h b/include/linux/i2c.h
index 39f1131a58d1..d31f37e90d11 100644
--- a/include/linux/i2c.h
+++ b/include/linux/i2c.h
@@ -135,7 +135,8 @@ enum i2c_alert_protocol {
  * struct i2c_driver - represent an I2C device driver
  * @class: What kind of i2c device we instantiate (for detect)
  * @attach_adapter: Callback for bus addition (deprecated)
- * @probe: Callback for device binding
+ * @probe: Callback for device binding - soon to be deprecated
+ * @probe_new: New callback for device binding
  * @remove: Callback for device unbinding
  * @shutdown: Callback for device shutdown
  * @alert: Alert callback, for example for the SMBus alert protocol
@@ -178,6 +179,11 @@ struct i2c_driver {
 	int (*probe)(struct i2c_client *, const struct i2c_device_id *);
 	int (*remove)(struct i2c_client *);
 
+	/* New driver model interface to aid the seamless removal of the
+	 * current probe()'s, more commonly unused than used second parameter.
+	 */
+	int (*probe_new)(struct i2c_client *);
+
 	/* driver model interfaces that don't relate to enumeration  */
 	void (*shutdown)(struct i2c_client *);
 
-- 
2.7.4

[PATCHv6 07/11] i2c: match dt-style device names from sysfs interface

[Kieran Bingham]

A user can choose to instantiate a device on an i2c bus using the sysfs
interface by providing a string and address to match and communicate
with the device on the bus. Presently this string is only matched
against the old i2c device id style strings, even in the presence of
full device tree compatible strings with vendor prefixes.

Providing a vendor-prefixed string to the sysfs interface will not match
against the device tree of_match_device() calls as there is no device
tree node to parse from the sysfs interface.

Convert i2c_of_match_device_strip_vendor() such that it can match both
vendor prefixed and stripped compatible strings on the sysfs interface.

Signed-off-by: Kieran Bingham <kieran@bingham.xyz>
---
 drivers/i2c/i2c-core.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
index 2003334b5246..01bce56f733a 100644
--- a/drivers/i2c/i2c-core.c
+++ b/drivers/i2c/i2c-core.c
@@ -1692,6 +1692,16 @@ i2c_of_match_device_strip_vendor(const struct of_device_id *matches,
 	const char *name;
 
 	for (; matches->compatible[0]; matches++) {
+		/*
+		 * Adding devices through the i2c sysfs interface provides us
+		 * a string to match which may be compatible with the device
+		 * tree compatible strings, however with no actual of_node the
+		 * of_match_device() will not match
+		 */
+		if (!strncasecmp(client->name, matches->compatible,
+				strlen(matches->compatible)))
+			return matches;
+
 		name = strchr(matches->compatible, ',');
 		if (!name)
 			name = matches->compatible;
-- 
2.7.4

Re: [PATCHv6 07/11] i2c: match dt-style device names from sysfs interface

[Lee Jones]

On Tue, 25 Oct 2016, Kieran Bingham wrote:

> A user can choose to instantiate a device on an i2c bus using the sysfs
> interface by providing a string and address to match and communicate
> with the device on the bus. Presently this string is only matched
> against the old i2c device id style strings, even in the presence of
> full device tree compatible strings with vendor prefixes.
> 
> Providing a vendor-prefixed string to the sysfs interface will not match
> against the device tree of_match_device() calls as there is no device
> tree node to parse from the sysfs interface.
> 
> Convert i2c_of_match_device_strip_vendor() such that it can match both
> vendor prefixed and stripped compatible strings on the sysfs interface.
> 
> Signed-off-by: Kieran Bingham <kieran@bingham.xyz>

Acked-by: Lee Jones <lee.jones@linaro.org>

> ---
>  drivers/i2c/i2c-core.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
> index 2003334b5246..01bce56f733a 100644
> --- a/drivers/i2c/i2c-core.c
> +++ b/drivers/i2c/i2c-core.c
> @@ -1692,6 +1692,16 @@ i2c_of_match_device_strip_vendor(const struct of_device_id *matches,
>  	const char *name;
>  
>  	for (; matches->compatible[0]; matches++) {
> +		/*
> +		 * Adding devices through the i2c sysfs interface provides us
> +		 * a string to match which may be compatible with the device
> +		 * tree compatible strings, however with no actual of_node the
> +		 * of_match_device() will not match
> +		 */
> +		if (!strncasecmp(client->name, matches->compatible,
> +				strlen(matches->compatible)))
> +			return matches;
> +
>  		name = strchr(matches->compatible, ',');
>  		if (!name)
>  			name = matches->compatible;

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

[PATCHv6 08/11] i2c: match vendorless strings on the internal string length

[Kieran Bingham]

If a user provides a shortened string to match a device to the sysfs i2c
interface it will match on the first string that contains that string
prefix.

for example:
  echo a 0x68 > /sys/bus/i2c/devices/i2c-2/new_device

will match as3711, as3722, and ak8975 incorrectly.

Signed-off-by: Kieran Bingham <kieran@bingham.xyz>
---
 drivers/i2c/i2c-core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
index 01bce56f733a..50c9cfdb87b7 100644
--- a/drivers/i2c/i2c-core.c
+++ b/drivers/i2c/i2c-core.c
@@ -1708,7 +1708,7 @@ i2c_of_match_device_strip_vendor(const struct of_device_id *matches,
 		else
 			name++;
 
-		if (!strncasecmp(client->name, name, strlen(client->name)))
+		if (!strncasecmp(client->name, name, strlen(name)))
 			return matches;
 	}
 
-- 
2.7.4

Re: [PATCHv6 08/11] i2c: match vendorless strings on the internal string length

[Lee Jones]

On Tue, 25 Oct 2016, Kieran Bingham wrote:

> If a user provides a shortened string to match a device to the sysfs i2c
> interface it will match on the first string that contains that string
> prefix.
> 
> for example:
>   echo a 0x68 > /sys/bus/i2c/devices/i2c-2/new_device
> 
> will match as3711, as3722, and ak8975 incorrectly.
> 
> Signed-off-by: Kieran Bingham <kieran@bingham.xyz>

Acked-by: Lee Jones <lee.jones@linaro.org>
  
> ---
>  drivers/i2c/i2c-core.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
> index 01bce56f733a..50c9cfdb87b7 100644
> --- a/drivers/i2c/i2c-core.c
> +++ b/drivers/i2c/i2c-core.c
> @@ -1708,7 +1708,7 @@ i2c_of_match_device_strip_vendor(const struct of_device_id *matches,
>  		else
>  			name++;
>  
> -		if (!strncasecmp(client->name, name, strlen(client->name)))
> +		if (!strncasecmp(client->name, name, strlen(name)))
>  			return matches;
>  	}
>  

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCHv6 08/11] i2c: match vendorless strings on the internal string length

[Peter Rosin]

On 2016-10-26 10:53, Lee Jones wrote:
> On Tue, 25 Oct 2016, Kieran Bingham wrote:
> 
>> If a user provides a shortened string to match a device to the sysfs i2c
>> interface it will match on the first string that contains that string
>> prefix.
>>
>> for example:
>>   echo a 0x68 > /sys/bus/i2c/devices/i2c-2/new_device
>>
>> will match as3711, as3722, and ak8975 incorrectly.
>>
>> Signed-off-by: Kieran Bingham <kieran@bingham.xyz>
> 
> Acked-by: Lee Jones <lee.jones@linaro.org>
>   
>> ---
>>  drivers/i2c/i2c-core.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
>> index 01bce56f733a..50c9cfdb87b7 100644
>> --- a/drivers/i2c/i2c-core.c
>> +++ b/drivers/i2c/i2c-core.c
>> @@ -1708,7 +1708,7 @@ i2c_of_match_device_strip_vendor(const struct of_device_id *matches,
>>  		else
>>  			name++;
>>  
>> -		if (!strncasecmp(client->name, name, strlen(client->name)))
>> +		if (!strncasecmp(client->name, name, strlen(name)))
>>  			return matches;
>>  	}
>>  
> 

Is that really so much better?

With this patch
	echo as3711CRAP 0x68 > /sys/...
will match as3711.

What if there is some as37112 driver that is the real target?

Cheers,
Peter

Re: [PATCHv6 08/11] i2c: match vendorless strings on the internal string length

[Peter Rosin]

On 2016-10-26 10:53, Lee Jones wrote:
> On Tue, 25 Oct 2016, Kieran Bingham wrote:
> 
>> If a user provides a shortened string to match a device to the sysfs i2c
>> interface it will match on the first string that contains that string
>> prefix.
>>
>> for example:
>>   echo a 0x68 > /sys/bus/i2c/devices/i2c-2/new_device
>>
>> will match as3711, as3722, and ak8975 incorrectly.
>>
>> Signed-off-by: Kieran Bingham <kieran@bingham.xyz>
> 
> Acked-by: Lee Jones <lee.jones@linaro.org>
>   
>> ---
>>  drivers/i2c/i2c-core.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
>> index 01bce56f733a..50c9cfdb87b7 100644
>> --- a/drivers/i2c/i2c-core.c
>> +++ b/drivers/i2c/i2c-core.c
>> @@ -1708,7 +1708,7 @@ i2c_of_match_device_strip_vendor(const struct of_device_id *matches,
>>  		else
>>  			name++;
>>  
>> -		if (!strncasecmp(client->name, name, strlen(client->name)))
>> +		if (!strncasecmp(client->name, name, strlen(name)))
>>  			return matches;
>>  	}
>>  
> 

Is that really so much better?

With this patch
	echo as3711CRAP 0x68 > /sys/...
will match as3711.

What if there is some as37112 driver that is the real target?

Cheers,
Peter

Re: [PATCHv6 08/11] i2c: match vendorless strings on the internal string length

[Kieran Bingham]

Hi Peter,

Thanks for your review

On 31/10/16 13:55, Peter Rosin wrote:
> On 2016-10-26 10:53, Lee Jones wrote:
>> On Tue, 25 Oct 2016, Kieran Bingham wrote:
>>
>>> If a user provides a shortened string to match a device to the sysfs i2c
>>> interface it will match on the first string that contains that string
>>> prefix.
>>>
>>> for example:
>>>   echo a 0x68 > /sys/bus/i2c/devices/i2c-2/new_device
>>>
>>> will match as3711, as3722, and ak8975 incorrectly.
>>>
>>> Signed-off-by: Kieran Bingham <kieran@bingham.xyz>
>>
>> Acked-by: Lee Jones <lee.jones@linaro.org>
>>   
>>> ---
>>>  drivers/i2c/i2c-core.c | 2 +-
>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
>>> index 01bce56f733a..50c9cfdb87b7 100644
>>> --- a/drivers/i2c/i2c-core.c
>>> +++ b/drivers/i2c/i2c-core.c
>>> @@ -1708,7 +1708,7 @@ i2c_of_match_device_strip_vendor(const struct of_device_id *matches,
>>>  		else
>>>  			name++;
>>>  
>>> -		if (!strncasecmp(client->name, name, strlen(client->name)))
>>> +		if (!strncasecmp(client->name, name, strlen(name)))
>>>  			return matches;
>>>  	}
>>>  
>>
> 
> Is that really so much better?

My original thought was that it verifies 'more' of the userspace input.
but...

> With this patch
> 	echo as3711CRAP 0x68 > /sys/...
> will match as3711.
>
> What if there is some as37112 driver that is the real target?

You're right - It looks like the only way to do this correctly is to
match the strncasecmp and the strlen of both strings.

So really we should be using sysfs_streq(). The only limitation there is
that this original code was performing a case-insensitive compare.

Lee - Where did the requirement for case insensitive matching come from
in your original code. Is it expected to be case-insensitive from the
I2C sysfs interface? or are dt-nodes expected to be case-sensitive?

Does anyone see reason that this shouldn't be using sysfs_streq()? or do
we need a sysfs_strcaseeq()...

> Cheers,
> Peter
> 

-- 
Regards

Kieran Bingham

Re: [PATCHv6 08/11] i2c: match vendorless strings on the internal string length

[Lee Jones]

On Mon, 31 Oct 2016, Kieran Bingham wrote:
> On 31/10/16 13:55, Peter Rosin wrote:
> > On 2016-10-26 10:53, Lee Jones wrote:
> >> On Tue, 25 Oct 2016, Kieran Bingham wrote:
> >>
> >>> If a user provides a shortened string to match a device to the sysfs i2c
> >>> interface it will match on the first string that contains that string
> >>> prefix.
> >>>
> >>> for example:
> >>>   echo a 0x68 > /sys/bus/i2c/devices/i2c-2/new_device
> >>>
> >>> will match as3711, as3722, and ak8975 incorrectly.
> >>>
> >>> Signed-off-by: Kieran Bingham <kieran@bingham.xyz>
> >>
> >> Acked-by: Lee Jones <lee.jones@linaro.org>
> >>   
> >>> ---
> >>>  drivers/i2c/i2c-core.c | 2 +-
> >>>  1 file changed, 1 insertion(+), 1 deletion(-)
> >>>
> >>> diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
> >>> index 01bce56f733a..50c9cfdb87b7 100644
> >>> --- a/drivers/i2c/i2c-core.c
> >>> +++ b/drivers/i2c/i2c-core.c
> >>> @@ -1708,7 +1708,7 @@ i2c_of_match_device_strip_vendor(const struct of_device_id *matches,
> >>>  		else
> >>>  			name++;
> >>>  
> >>> -		if (!strncasecmp(client->name, name, strlen(client->name)))
> >>> +		if (!strncasecmp(client->name, name, strlen(name)))
> >>>  			return matches;
> >>>  	}
> >>>  
> >>
> > 
> > Is that really so much better?
> 
> My original thought was that it verifies 'more' of the userspace input.
> but...
> 
> > With this patch
> > 	echo as3711CRAP 0x68 > /sys/...
> > will match as3711.
> >
> > What if there is some as37112 driver that is the real target?
> 
> You're right - It looks like the only way to do this correctly is to
> match the strncasecmp and the strlen of both strings.
> 
> So really we should be using sysfs_streq(). The only limitation there is
> that this original code was performing a case-insensitive compare.
> 
> Lee - Where did the requirement for case insensitive matching come from
> in your original code. Is it expected to be case-insensitive from the
> I2C sysfs interface? or are dt-nodes expected to be case-sensitive?

Compatible strings are always lower-case.

> Does anyone see reason that this shouldn't be using sysfs_streq()? or do
> we need a sysfs_strcaseeq()...

... but I don't see an issue with not being case sensitive.  Certainly
if it makes the logic easier/more consistent.

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

[PATCHv6 09/11] mfd: 88pm860x: Move over to new I2C device .probe() call

[Kieran Bingham]

From: Lee Jones <lee.jones@linaro.org>

As part of an effort to rid the mostly unused second parameter for I2C
related .probe() functions and to conform to other existing frameworks
we're moving over to a temporary replacement .probe() call-back.

Acked-by: Grant Likely <grant.likely@linaro.org>
Signed-off-by: Lee Jones <lee.jones@linaro.org>
Signed-off-by: Kieran Bingham <kieran@bingham.xyz>

---
Changes since v4
 - Rename .probe2 to probe_new
---
 drivers/mfd/88pm860x-core.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/mfd/88pm860x-core.c b/drivers/mfd/88pm860x-core.c
index 25e1aafae60c..227b99018657 100644
--- a/drivers/mfd/88pm860x-core.c
+++ b/drivers/mfd/88pm860x-core.c
@@ -1132,8 +1132,7 @@ static int pm860x_dt_init(struct device_node *np,
 	return 0;
 }
 
-static int pm860x_probe(struct i2c_client *client,
-				  const struct i2c_device_id *id)
+static int pm860x_probe(struct i2c_client *client)
 {
 	struct pm860x_platform_data *pdata = dev_get_platdata(&client->dev);
 	struct device_node *node = client->dev.of_node;
@@ -1259,7 +1258,7 @@ static struct i2c_driver pm860x_driver = {
 		.pm     = &pm860x_pm_ops,
 		.of_match_table	= pm860x_dt_ids,
 	},
-	.probe		= pm860x_probe,
+	.probe_new	= pm860x_probe,
 	.remove		= pm860x_remove,
 	.id_table	= pm860x_id_table,
 };
-- 
2.7.4

[PATCHv6 10/11] mfd: as3722: Rid driver of superfluous I2C device ID structure

[Kieran Bingham]

From: Lee Jones <lee.jones@linaro.org>

Also remove unused second probe() parameter 'i2c_device_id'.

Acked-by: Grant Likely <grant.likely@linaro.org>
Signed-off-by: Lee Jones <lee.jones@linaro.org>
Signed-off-by: Kieran Bingham <kieran@bingham.xyz>

---
Changes since v4
 - Rename .probe2 to probe_new
---
 drivers/mfd/as3722.c | 12 ++----------
 1 file changed, 2 insertions(+), 10 deletions(-)

diff --git a/drivers/mfd/as3722.c b/drivers/mfd/as3722.c
index f87342c211bc..7d8c5e4136e2 100644
--- a/drivers/mfd/as3722.c
+++ b/drivers/mfd/as3722.c
@@ -354,8 +354,7 @@ static int as3722_i2c_of_probe(struct i2c_client *i2c,
 	return 0;
 }
 
-static int as3722_i2c_probe(struct i2c_client *i2c,
-			const struct i2c_device_id *id)
+static int as3722_i2c_probe(struct i2c_client *i2c)
 {
 	struct as3722 *as3722;
 	unsigned long irq_flags;
@@ -441,12 +440,6 @@ static const struct of_device_id as3722_of_match[] = {
 };
 MODULE_DEVICE_TABLE(of, as3722_of_match);
 
-static const struct i2c_device_id as3722_i2c_id[] = {
-	{ "as3722", 0 },
-	{},
-};
-MODULE_DEVICE_TABLE(i2c, as3722_i2c_id);
-
 static const struct dev_pm_ops as3722_pm_ops = {
 	SET_SYSTEM_SLEEP_PM_OPS(as3722_i2c_suspend, as3722_i2c_resume)
 };
@@ -457,8 +450,7 @@ static struct i2c_driver as3722_i2c_driver = {
 		.of_match_table = as3722_of_match,
 		.pm = &as3722_pm_ops,
 	},
-	.probe = as3722_i2c_probe,
-	.id_table = as3722_i2c_id,
+	.probe_new = as3722_i2c_probe,
 };
 
 module_i2c_driver(as3722_i2c_driver);
-- 
2.7.4

[PATCHv6 11/11] [TESTPATCH] rtc: convert ds1307 to interim probe_new

[Kieran Bingham]

Just for testing, specify a maxim,ds1307 device to identify the code path used
when instantiating the driver from userspace.

root@arm:~# echo maxim,ds1307 0x68 > /sys/bus/i2c/devices/i2c-2/new_device
[   49.807383] rtc-ds1307 2-0068: I'm a Maxim ...
[   49.814706] rtc-ds1307 2-0068: rtc core: registered maxim,ds1307 as rtc0
[   49.821813] rtc-ds1307 2-0068: 56 bytes nvram
[   49.826444] i2c i2c-2: new_device: Instantiated device maxim,ds1307 at 0x68

This patch also demonstrates how to obtain data from the id's using the
new methods.
---
 drivers/rtc/rtc-ds1307.c | 53 ++++++++++++++++++++++++++++--------------------
 1 file changed, 31 insertions(+), 22 deletions(-)

diff --git a/drivers/rtc/rtc-ds1307.c b/drivers/rtc/rtc-ds1307.c
index 8e1c5cb6ece6..f760b7cf1fdb 100644
--- a/drivers/rtc/rtc-ds1307.c
+++ b/drivers/rtc/rtc-ds1307.c
@@ -31,6 +31,7 @@
  */
 enum ds_type {
 	ds_1307,
+	maxim_1307,
 	ds_1337,
 	ds_1338,
 	ds_1339,
@@ -144,6 +145,10 @@ static struct chip_desc chips[last_ds_type] = {
 		.nvram_offset	= 8,
 		.nvram_size	= 56,
 	},
+	[maxim_1307] = {
+		.nvram_offset	= 8,
+		.nvram_size	= 56,
+	},
 	[ds_1337] = {
 		.alarm		= 1,
 	},
@@ -173,22 +178,6 @@ static struct chip_desc chips[last_ds_type] = {
 	},
 };
 
-static const struct i2c_device_id ds1307_id[] = {
-	{ "ds1307", ds_1307 },
-	{ "ds1337", ds_1337 },
-	{ "ds1338", ds_1338 },
-	{ "ds1339", ds_1339 },
-	{ "ds1388", ds_1388 },
-	{ "ds1340", ds_1340 },
-	{ "ds3231", ds_3231 },
-	{ "m41t00", m41t00 },
-	{ "mcp7940x", mcp794xx },
-	{ "mcp7941x", mcp794xx },
-	{ "pt7c4338", ds_1307 },
-	{ "rx8025", rx_8025 },
-	{ }
-};
-MODULE_DEVICE_TABLE(i2c, ds1307_id);
 
 /*----------------------------------------------------------------------*/
 
@@ -1223,13 +1212,20 @@ static void ds1307_clks_register(struct ds1307 *ds1307)
 
 #endif /* CONFIG_COMMON_CLK */
 
-static int ds1307_probe(struct i2c_client *client,
-			const struct i2c_device_id *id)
+static const struct of_device_id ds1307_dt_ids[] = {
+	{ .compatible = "dallas,ds1307", .data = (void *)ds_1307 },
+	{ .compatible = "maxim,ds1307", .data = (void *)maxim_1307 },
+	{ /* sentinel */ }
+};
+MODULE_DEVICE_TABLE(of, ds1307_dt_ids);
+
+static int ds1307_probe(struct i2c_client *client)
 {
 	struct ds1307		*ds1307;
 	int			err = -ENODEV;
 	int			tmp, wday;
-	struct chip_desc	*chip = &chips[id->driver_data];
+	const struct of_device_id 	*idof;
+	struct chip_desc	*chip;
 	struct i2c_adapter	*adapter = to_i2c_adapter(client->dev.parent);
 	bool			want_irq = false;
 	bool			ds1307_can_wakeup_device = false;
@@ -1255,10 +1251,20 @@ static int ds1307_probe(struct i2c_client *client,
 	if (!ds1307)
 		return -ENOMEM;
 
+	/* If we've got this far, this shouldn't be able to fail - but check anyway for now */
+	idof = i2c_of_match_device(ds1307_dt_ids, client);
+	if (!idof) {
+		dev_err(&client->dev, "Probe failed to find an id entry\n");
+		return -ENODEV;
+	}
+
+	/* Now we can set our chip entry */
+	chip = &chips[(int)idof->data];
+
 	i2c_set_clientdata(client, ds1307);
 
 	ds1307->client	= client;
-	ds1307->type	= id->driver_data;
+	ds1307->type	= (int) idof->data;
 
 	if (!pdata && client->dev.of_node)
 		ds1307_trickle_of_init(client, chip);
@@ -1435,6 +1441,9 @@ read_rtc:
 	 */
 	tmp = ds1307->regs[DS1307_REG_SECS];
 	switch (ds1307->type) {
+	case maxim_1307:
+		dev_info(&client->dev, "I'm a Maxim ... \n");
+		/* fallthrough */
 	case ds_1307:
 	case m41t00:
 		/* clock halted?  turn it on, so clock can tick. */
@@ -1634,10 +1643,10 @@ static int ds1307_remove(struct i2c_client *client)
 static struct i2c_driver ds1307_driver = {
 	.driver = {
 		.name	= "rtc-ds1307",
+		.of_match_table = of_match_ptr(ds1307_dt_ids),
 	},
-	.probe		= ds1307_probe,
+	.probe_new	= ds1307_probe,
 	.remove		= ds1307_remove,
-	.id_table	= ds1307_id,
 };
 
 module_i2c_driver(ds1307_driver);
-- 
2.7.4

Re: [PATCHv6 00/11] i2c: Relax mandatory I2C ID table passing

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 572 bytes --]


> As such, in this series I have extended i2c_of_match_device_strip_vendor
> so that it will also perform a search against a full compatible string
> for the matching process. This was not done by the core OF framework as
> the sysfs interface does not create an of_node to match against. If so
> desired, this function could now be renamed to i2c_of_match_device_sysfs

Looks good from a glimpse. Now I want to test, but I can't apply to
v4.9-rc3. What is your base? Is there a branch available? Or could you
even resend, rebased with the sysfs_streq() change applied?


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]