[PATCHv5 0/8] 2c: Relax mandatory I2C ID table passing

[PATCHv5 0/8] 2c: Relax mandatory I2C ID table passing

[Kieran Bingham]

This patch set finally pops up again, after a long time stuck somewhere in the
midst of my stack.

As it stood last year, the requirements were to rename probe2 to probe_new, and
ensure that it was correctly tested. The rename was the easy bit, but the
testing took me more time to get things set up properly. And other commitments
then got in the way of things. Of course this patch set has also been rebased
as well, but there wasn't any major pain there.

Testing
-------

To try to establish testing, I have used a beagle-bone-black, and a DS1307 RTC
connected to the BBB SCL and SDA lines. The main reason for these choices is
accesibility. i.e. I have them, and the BBB readily boots a kernel for me to
test and iterate with.

I've tested the device with i2cdetect, and then worked through testing the
sysfs interface, device tree, and module autoloading, each time ensuring that
the RTC enumerates and operates

* new_device (built-in, and external module)
  echo ds1307 0x68 > /sys/bus/i2c/devices/i2c-2/new_device
  cat /sys/class/rtc/rtc0/date

 - Both of those worked fine.

* Device Tree
  I tested that the device would still register by adding a node in the device
  tree for the board, and testing with a built-in module. 

 - This worked fine.

* Module Autoloading
  With the device tree node in the board dts file, it wouldn't automatically
  load from the external module. This was due to the rtc-ds1307 module not
  exporting an of_match table, and not yet having Javier's "report OF style
  modalias when probing using DT" [0]  patch applied

 - With the module updated, and Javiers patch applied, the module autoloads

Finally, I feel I can safely add this tag to the patch set:
Tested-by: Kieran Bingham <kieran@bingham.xyz>

Please let me know if there is any other specific use case missing here that
needs to be tested.

[0] https://patchwork.ozlabs.org/patch/502201/

Patches
-------
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      | 75 +++++++++++++++++++++++++++++++++++++++------
 drivers/mfd/88pm860x-core.c |  5 ++-
 drivers/mfd/as3722.c        | 12 ++------
 include/linux/i2c.h         | 22 ++++++++++++-
 4 files changed, 91 insertions(+), 23 deletions(-)

-- 
2.5.0

[PATCHv5 1/8] 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>
Signed-off-by: Kieran Bingham <kieran@bingham.xyz>
Tested-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 e584d88ee337..e980c8257676 100644
--- a/drivers/i2c/i2c-core.c
+++ b/drivers/i2c/i2c-core.c
@@ -493,6 +493,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;
@@ -506,8 +509,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))
@@ -518,9 +519,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.5.0

[PATCHv5 2/8] 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]
Signed-off-by: Kieran Bingham <kieran@bingham.xyz>
Tested-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 e980c8257676..f75e78f6f9c1 100644
--- a/drivers/i2c/i2c-core.c
+++ b/drivers/i2c/i2c-core.c
@@ -1494,6 +1494,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.5.0

[PATCHv5 3/8] 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]
Signed-off-by: Kieran Bingham <kieran@bingham.xyz>
Tested-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 f75e78f6f9c1..88211009d282 100644
--- a/drivers/i2c/i2c-core.c
+++ b/drivers/i2c/i2c-core.c
@@ -1515,6 +1515,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 200cf13b00f6..547d213d2a33 100644
--- a/include/linux/i2c.h
+++ b/include/linux/i2c.h
@@ -689,6 +689,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)
@@ -705,6 +709,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.5.0

[PATCHv5 4/8] 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>
Signed-off-by: Kieran Bingham <kieran@bingham.xyz>
Tested-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 88211009d282..dd5a19ee92af 100644
--- a/drivers/i2c/i2c-core.c
+++ b/drivers/i2c/i2c-core.c
@@ -511,7 +511,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 */
@@ -696,7 +696,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.5.0

[PATCHv5 5/8] 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>
Signed-off-by: Kieran Bingham <kieran@bingham.xyz>
Tested-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 dd5a19ee92af..64d543b041b1 100644
--- a/drivers/i2c/i2c-core.c
+++ b/drivers/i2c/i2c-core.c
@@ -490,7 +490,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))
@@ -503,6 +503,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 547d213d2a33..e3aa3f5e59a8 100644
--- a/include/linux/i2c.h
+++ b/include/linux/i2c.h
@@ -235,6 +235,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.5.0

[PATCHv5 6/8] 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}]
Signed-off-by: Kieran Bingham <kieran@bingham.xyz>
Tested-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 64d543b041b1..268fec3b6931 100644
--- a/drivers/i2c/i2c-core.c
+++ b/drivers/i2c/i2c-core.c
@@ -697,8 +697,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
@@ -740,7 +738,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 e3aa3f5e59a8..c8f73b9f51fe 100644
--- a/include/linux/i2c.h
+++ b/include/linux/i2c.h
@@ -130,7 +130,8 @@ i2c_smbus_read_i2c_block_data_or_emulated(const struct i2c_client *client,
  * 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
@@ -173,6 +174,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.5.0

[PATCHv5 7/8] 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.5.0

[PATCHv5 8/8] 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 e1f597f97f86..28fc62af6861 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;
@@ -453,12 +452,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)
 };
@@ -469,9 +462,8 @@ static struct i2c_driver as3722_i2c_driver = {
 		.of_match_table = as3722_of_match,
 		.pm = &as3722_pm_ops,
 	},
-	.probe = as3722_i2c_probe,
+	.probe_new = as3722_i2c_probe,
 	.remove = as3722_i2c_remove,
-	.id_table = as3722_i2c_id,
 };
 
 module_i2c_driver(as3722_i2c_driver);
-- 
2.5.0

Re: [PATCHv5 0/8] 2c: Relax mandatory I2C ID table passing

[Lee Jones]

On Wed, 04 May 2016, Kieran Bingham wrote:

> This patch set finally pops up again, after a long time stuck somewhere in the
> midst of my stack.
> 
> As it stood last year, the requirements were to rename probe2 to probe_new, and
> ensure that it was correctly tested. The rename was the easy bit, but the
> testing took me more time to get things set up properly. And other commitments
> then got in the way of things. Of course this patch set has also been rebased
> as well, but there wasn't any major pain there.
> 
> Testing
> -------
> 
> To try to establish testing, I have used a beagle-bone-black, and a DS1307 RTC
> connected to the BBB SCL and SDA lines. The main reason for these choices is
> accesibility. i.e. I have them, and the BBB readily boots a kernel for me to
> test and iterate with.
> 
> I've tested the device with i2cdetect, and then worked through testing the
> sysfs interface, device tree, and module autoloading, each time ensuring that
> the RTC enumerates and operates
> 
> * new_device (built-in, and external module)
>   echo ds1307 0x68 > /sys/bus/i2c/devices/i2c-2/new_device
>   cat /sys/class/rtc/rtc0/date
> 
>  - Both of those worked fine.
> 
> * Device Tree
>   I tested that the device would still register by adding a node in the device
>   tree for the board, and testing with a built-in module. 
> 
>  - This worked fine.
> 
> * Module Autoloading
>   With the device tree node in the board dts file, it wouldn't automatically
>   load from the external module. This was due to the rtc-ds1307 module not
>   exporting an of_match table, and not yet having Javier's "report OF style
>   modalias when probing using DT" [0]  patch applied
> 
>  - With the module updated, and Javiers patch applied, the module autoloads
> 
> Finally, I feel I can safely add this tag to the patch set:
> Tested-by: Kieran Bingham <kieran@bingham.xyz>

Great work Kieran.

Wolfram, Javier,

Looks like Kieran has ticked each of your boxes.

I guess there is nothing stopping this set from being applied now,
right?

> Please let me know if there is any other specific use case missing here that
> needs to be tested.
> 
> [0] https://patchwork.ozlabs.org/patch/502201/
> 
> Patches
> -------
> 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      | 75 +++++++++++++++++++++++++++++++++++++++------
>  drivers/mfd/88pm860x-core.c |  5 ++-
>  drivers/mfd/as3722.c        | 12 ++------
>  include/linux/i2c.h         | 22 ++++++++++++-
>  4 files changed, 91 insertions(+), 23 deletions(-)
> 

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

Re: [PATCHv5 0/8] 2c: Relax mandatory I2C ID table passing

[Javier Martinez Canillas]

Hello Lee,

On 05/09/2016 05:14 AM, Lee Jones wrote:
> On Wed, 04 May 2016, Kieran Bingham wrote:
> 

[snip].

>>
>> Finally, I feel I can safely add this tag to the patch set:
>> Tested-by: Kieran Bingham <kieran@bingham.xyz>
> 
> Great work Kieran.
> 
> Wolfram, Javier,
> 
> Looks like Kieran has ticked each of your boxes.
> 
> I guess there is nothing stopping this set from being applied now,
> right?
> 

That's correct, on a quick looks it seems that this set is ready to be
merged and it would be great if that is sooner rather than later. I'll
do a detailed review and testing today.

Best regards,
-- 
Javier Martinez Canillas
Open Source Group
Samsung Research America

Re: [PATCHv5 1/8] i2c: Add pointer dereference protection to i2c_match_id()

[Javier Martinez Canillas]

On 05/04/2016 11:14 AM, Kieran Bingham wrote:
> 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>
> Signed-off-by: Kieran Bingham <kieran@bingham.xyz>
> 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>

Best regards,
-- 
Javier Martinez Canillas
Open Source Group
Samsung Research America

Re: [PATCHv5 2/8] i2c: Add the ability to match device to compatible string without an of_node

[Javier Martinez Canillas]

On 05/04/2016 11:14 AM, Kieran Bingham wrote:
> 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]
> Signed-off-by: Kieran Bingham <kieran@bingham.xyz>
> 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>

Best regards,
-- 
Javier Martinez Canillas
Open Source Group
Samsung Research America

Re: [PATCHv5 3/8] i2c: Match using traditional OF methods, then by vendor-less compatible strings

[Javier Martinez Canillas]

On 05/04/2016 11:14 AM, Kieran Bingham wrote:
> 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]
> Signed-off-by: Kieran Bingham <kieran@bingham.xyz>
> 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>

Best regards,
-- 
Javier Martinez Canillas
Open Source Group
Samsung Research America

Re: [PATCHv5 4/8] i2c: Make I2C ID tables non-mandatory for DT'ed devices

[Javier Martinez Canillas]

On 05/04/2016 11:14 AM, Kieran Bingham wrote:
> 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>
> Signed-off-by: Kieran Bingham <kieran@bingham.xyz>
> 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>

Best regards,
-- 
Javier Martinez Canillas
Open Source Group
Samsung Research America

Re: [PATCHv5 5/8] i2c: Export i2c_match_id() for direct use by device drivers

[Javier Martinez Canillas]

On 05/04/2016 11:14 AM, Kieran Bingham wrote:
> 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>
> Signed-off-by: Kieran Bingham <kieran@bingham.xyz>
> 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>

Best regards,
-- 
Javier Martinez Canillas
Open Source Group
Samsung Research America

Re: [PATCHv5 6/8] i2c: Provide a temporary .probe_new() call-back type

[Javier Martinez Canillas]

On 05/04/2016 11:14 AM, Kieran Bingham wrote:
> 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}]
> Signed-off-by: Kieran Bingham <kieran@bingham.xyz>
> 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>

Best regards,
-- 
Javier Martinez Canillas
Open Source Group
Samsung Research America

Re: [PATCHv5 7/8] mfd: 88pm860x: Move over to new I2C device .probe() call

[Javier Martinez Canillas]

On 05/04/2016 11:14 AM, Kieran Bingham wrote:
> 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>
> 

Reviewed-by: Javier Martinez Canillas <javier@osg.samsung.com>

Best regards,
-- 
Javier Martinez Canillas
Open Source Group
Samsung Research America

Re: [PATCHv5 8/8] mfd: as3722: Rid driver of superfluous I2C device ID structure

[Javier Martinez Canillas]

On 05/04/2016 11:14 AM, Kieran Bingham wrote:
> 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 e1f597f97f86..28fc62af6861 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;
> @@ -453,12 +452,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);
> -

Just a note that this can only be made because the driver's Kconfig symbol
is bool and not tristate. Since for drivers that can be built as a module,
the I2C core always reports a MODALIAS of the form "i2c:as3722" and so the
i2c_device_id array and the MODULE_DEVICE_TABLE() are needed even when not
used by the driver.

As mentioned the change is correct for this driver but I just wanted to
point out in case other authors try to do the same change for drivers that
can be built as a module and so breaking module auto-loading.

Reviewed-by: Javier Martinez Canillas <javier@osg.samsung.com>

Best regards,
-- 
Javier Martinez Canillas
Open Source Group
Samsung Research America

Re: [PATCHv5 0/8] 2c: Relax mandatory I2C ID table passing

[Javier Martinez Canillas]

Hello Kieran,

On 05/04/2016 11:14 AM, Kieran Bingham wrote:
> This patch set finally pops up again, after a long time stuck somewhere in the
> midst of my stack.
> 
> As it stood last year, the requirements were to rename probe2 to probe_new, and
> ensure that it was correctly tested. The rename was the easy bit, but the
> testing took me more time to get things set up properly. And other commitments
> then got in the way of things. Of course this patch set has also been rebased
> as well, but there wasn't any major pain there.
> 
> Testing
> -------
> 
> To try to establish testing, I have used a beagle-bone-black, and a DS1307 RTC
> connected to the BBB SCL and SDA lines. The main reason for these choices is
> accesibility. i.e. I have them, and the BBB readily boots a kernel for me to
> test and iterate with.
> 
> I've tested the device with i2cdetect, and then worked through testing the
> sysfs interface, device tree, and module autoloading, each time ensuring that
> the RTC enumerates and operates
> 
> * new_device (built-in, and external module)
>   echo ds1307 0x68 > /sys/bus/i2c/devices/i2c-2/new_device
>   cat /sys/class/rtc/rtc0/date
> 
>  - Both of those worked fine.
> 
> * Device Tree
>   I tested that the device would still register by adding a node in the device
>   tree for the board, and testing with a built-in module. 
> 
>  - This worked fine.
> 
> * Module Autoloading
>   With the device tree node in the board dts file, it wouldn't automatically
>   load from the external module. This was due to the rtc-ds1307 module not
>   exporting an of_match table, and not yet having Javier's "report OF style
>   modalias when probing using DT" [0]  patch applied
> 
>  - With the module updated, and Javiers patch applied, the module autoloads
> 
> Finally, I feel I can safely add this tag to the patch set:
> Tested-by: Kieran Bingham <kieran@bingham.xyz>
>

Same here, I've tested this series using an Exynos5800 Peach Pi Chromebook
that has a I2C touchpad device (Atmel mXT540S). So I used this series and
removed the i2c_device_id table from the device driver.

The driver could match correctly using the of_device_id table and also the
module was auto-loaded when using my mentioned RFC patch to report OF style
module aliases instead of always using the legacy one.

I've also reviewed the patches and the changes looks good to me. I hope the
patches can finally land since have been in the list for almost 2 years [0].

[0]: https://lkml.org/lkml/2014/8/28/283

Best regards,
-- 
Javier Martinez Canillas
Open Source Group
Samsung Research America

Re: [PATCHv5 8/8] mfd: as3722: Rid driver of superfluous I2C device ID structure

[Lee Jones]

On Tue, 10 May 2016, Javier Martinez Canillas wrote:

> On 05/04/2016 11:14 AM, Kieran Bingham wrote:
> > 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 e1f597f97f86..28fc62af6861 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;
> > @@ -453,12 +452,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);
> > -
> 
> Just a note that this can only be made because the driver's Kconfig symbol
> is bool and not tristate. Since for drivers that can be built as a module,
> the I2C core always reports a MODALIAS of the form "i2c:as3722" and so the
> i2c_device_id array and the MODULE_DEVICE_TABLE() are needed even when not
> used by the driver.
> 
> As mentioned the change is correct for this driver but I just wanted to
> point out in case other authors try to do the same change for drivers that
> can be built as a module and so breaking module auto-loading.

Sounds like a subsequent patch might be required to fix that use-case
too.  I'll add it to my TODO. :)

> Reviewed-by: Javier Martinez Canillas <javier@osg.samsung.com>
> 
> Best regards,

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

Re: [PATCHv5 0/8] 2c: Relax mandatory I2C ID table passing

[Kieran Bingham]

Hi Javier,

On 10/05/16 06:31, Javier Martinez Canillas wrote:
> Hello Kieran,
> 
> On 05/04/2016 11:14 AM, Kieran Bingham wrote:
>> This patch set finally pops up again, after a long time stuck somewhere in the
>> midst of my stack.
>>
>> As it stood last year, the requirements were to rename probe2 to probe_new, and
>> ensure that it was correctly tested. The rename was the easy bit, but the
>> testing took me more time to get things set up properly. And other commitments
>> then got in the way of things. Of course this patch set has also been rebased
>> as well, but there wasn't any major pain there.
>>
>> Testing
>> -------
>>
>> To try to establish testing, I have used a beagle-bone-black, and a DS1307 RTC
>> connected to the BBB SCL and SDA lines. The main reason for these choices is
>> accesibility. i.e. I have them, and the BBB readily boots a kernel for me to
>> test and iterate with.
>>
>> I've tested the device with i2cdetect, and then worked through testing the
>> sysfs interface, device tree, and module autoloading, each time ensuring that
>> the RTC enumerates and operates
>>
>> * new_device (built-in, and external module)
>>   echo ds1307 0x68 > /sys/bus/i2c/devices/i2c-2/new_device
>>   cat /sys/class/rtc/rtc0/date
>>
>>  - Both of those worked fine.
>>
>> * Device Tree
>>   I tested that the device would still register by adding a node in the device
>>   tree for the board, and testing with a built-in module. 
>>
>>  - This worked fine.
>>
>> * Module Autoloading
>>   With the device tree node in the board dts file, it wouldn't automatically
>>   load from the external module. This was due to the rtc-ds1307 module not
>>   exporting an of_match table, and not yet having Javier's "report OF style
>>   modalias when probing using DT" [0]  patch applied
>>
>>  - With the module updated, and Javiers patch applied, the module autoloads
>>
>> Finally, I feel I can safely add this tag to the patch set:
>> Tested-by: Kieran Bingham <kieran@bingham.xyz>
>>
> 
> Same here, I've tested this series using an Exynos5800 Peach Pi Chromebook
> that has a I2C touchpad device (Atmel mXT540S). So I used this series and
> removed the i2c_device_id table from the device driver.
> 
> The driver could match correctly using the of_device_id table and also the
> module was auto-loaded when using my mentioned RFC patch to report OF style
> module aliases instead of always using the legacy one.
> 
> I've also reviewed the patches and the changes looks good to me. I hope the
> patches can finally land since have been in the list for almost 2 years [0].

Looks like the original submission [1] is even closer to 2 years ago,
at 2nd June 2014!

> [0]: https://lkml.org/lkml/2014/8/28/283
  [1]: https://lkml.org/lkml/2014/6/2/274

Thanks for taking the time to test as well. Lets aim to complete the
conversion in less than 2 years :)

(Note to self when referencing in the future, "Hello 2018")

-- 
Regards

Kieran Bingham

Re: [PATCHv5 8/8] mfd: as3722: Rid driver of superfluous I2C device ID structure

[Javier Martinez Canillas]

Hello Lee,

On 05/10/2016 03:33 AM, Lee Jones wrote:
> On Tue, 10 May 2016, Javier Martinez Canillas wrote:

[snip]

>>>  
>>> -static const struct i2c_device_id as3722_i2c_id[] = {
>>> -	{ "as3722", 0 },
>>> -	{},
>>> -};
>>> -MODULE_DEVICE_TABLE(i2c, as3722_i2c_id);
>>> -
>>
>> Just a note that this can only be made because the driver's Kconfig symbol
>> is bool and not tristate. Since for drivers that can be built as a module,
>> the I2C core always reports a MODALIAS of the form "i2c:as3722" and so the
>> i2c_device_id array and the MODULE_DEVICE_TABLE() are needed even when not
>> used by the driver.
>>
>> As mentioned the change is correct for this driver but I just wanted to
>> point out in case other authors try to do the same change for drivers that
>> can be built as a module and so breaking module auto-loading.
> 
> Sounds like a subsequent patch might be required to fix that use-case
> too.  I'll add it to my TODO. :)
>

Well, the fix is really trivial and I've posted it as an RFC patch a [0] a
long time ago. That is what Kieran and my used to test module autoload with
this patch series.

The problem is that a lot of I2C drivers are relying on how the subsystem
behave (always match using i2c_device_id table and report i2c: modalias)
and so OF drivers don't have an of_device_id table since was not necessary.

So if the RFC patch lands [0], that will break a lot of drivers since after
that, the I2C devices registered via OF will report a of: modalias but will
not have a OF aliases in their modules.

We need a flag day to change the I2C subsystem behaviour and that can only
happen after all the in-tree I2C drivers have proper exported of_device_id.

I posted a patch series almost a year ago [1] trying to fix the I2C drivers
that I could find using a script but then found that removing the I2C table
was not possible also due how the subsystem did the matching. Fortunately
your patch series fixed this :)

So after your series land, I plan to do the same investigation again and
post patches to fix all the remaining I2C drivers so the modalias patch can
finally land and the I2C subsystem report modalias like other subsystems do.

[0]: https://patchwork.ozlabs.org/patch/502201/
[1]: https://lkml.org/lkml/2015/7/30/519

Best regards,
-- 
Javier Martinez Canillas
Open Source Group
Samsung Research America

Re: [PATCHv5 8/8] mfd: as3722: Rid driver of superfluous I2C device ID structure

[Lee Jones]

On Tue, 10 May 2016, Javier Martinez Canillas wrote:

> Hello Lee,
> 
> On 05/10/2016 03:33 AM, Lee Jones wrote:
> > On Tue, 10 May 2016, Javier Martinez Canillas wrote:
> 
> [snip]
> 
> >>>  
> >>> -static const struct i2c_device_id as3722_i2c_id[] = {
> >>> -	{ "as3722", 0 },
> >>> -	{},
> >>> -};
> >>> -MODULE_DEVICE_TABLE(i2c, as3722_i2c_id);
> >>> -
> >>
> >> Just a note that this can only be made because the driver's Kconfig symbol
> >> is bool and not tristate. Since for drivers that can be built as a module,
> >> the I2C core always reports a MODALIAS of the form "i2c:as3722" and so the
> >> i2c_device_id array and the MODULE_DEVICE_TABLE() are needed even when not
> >> used by the driver.
> >>
> >> As mentioned the change is correct for this driver but I just wanted to
> >> point out in case other authors try to do the same change for drivers that
> >> can be built as a module and so breaking module auto-loading.
> > 
> > Sounds like a subsequent patch might be required to fix that use-case
> > too.  I'll add it to my TODO. :)
> >
> 
> Well, the fix is really trivial and I've posted it as an RFC patch a [0] a
> long time ago. That is what Kieran and my used to test module autoload with
> this patch series.
> 
> The problem is that a lot of I2C drivers are relying on how the subsystem
> behave (always match using i2c_device_id table and report i2c: modalias)
> and so OF drivers don't have an of_device_id table since was not necessary.
> 
> So if the RFC patch lands [0], that will break a lot of drivers since after
> that, the I2C devices registered via OF will report a of: modalias but will
> not have a OF aliases in their modules.
> 
> We need a flag day to change the I2C subsystem behaviour and that can only
> happen after all the in-tree I2C drivers have proper exported of_device_id.
> 
> I posted a patch series almost a year ago [1] trying to fix the I2C drivers
> that I could find using a script but then found that removing the I2C table
> was not possible also due how the subsystem did the matching. Fortunately
> your patch series fixed this :)
> 
> So after your series land, I plan to do the same investigation again and
> post patches to fix all the remaining I2C drivers so the modalias patch can
> finally land and the I2C subsystem report modalias like other subsystems do.

Sounds perfect.  Thanks for the explanation.

Wolfram,
 Things are looking up for the subsystem, please do your thing, so we
 can ensure awesomeness. :)

> [0]: https://patchwork.ozlabs.org/patch/502201/
> [1]: https://lkml.org/lkml/2015/7/30/519

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

[PATCH] cocci: Find i2c drivers with an of_device table that isn't exported

[Kieran Bingham]

This script will then add the MODULE_DEVICE_TABLE(of, ...) to correctly export
the device tree table

Usage: spatch --sp-file scripts/coccinelle/i2c/i2c_table_missing_export.cocci . --in-place
---

Javier,

I've been working on scripts to automate some of the conversion processes
involved in this DT_I2C project.

This one adds missing exports for i2c device tables.

Feel free to use it if it helps.

I have another one which searches for drivers which have both an I2C device table, and an OF device table,
and a probe function which does not use the second parameter.

It then performs the conversion to the probe_new() and removes the I2C device table.

Should help with the bulk of the conversion

--
Kieran

 .../coccinelle/i2c/of_table_missing_export.cocci   | 34 ++++++++++++++++++++++
 1 file changed, 34 insertions(+)
 create mode 100644 scripts/coccinelle/i2c/of_table_missing_export.cocci

diff --git a/scripts/coccinelle/i2c/of_table_missing_export.cocci b/scripts/coccinelle/i2c/of_table_missing_export.cocci
new file mode 100644
index 000000000000..5d1950fdf8a3
--- /dev/null
+++ b/scripts/coccinelle/i2c/of_table_missing_export.cocci
@@ -0,0 +1,34 @@
+// Look for I2C drivers without an exported of_device_id table
+//
+
+// C1 : Identify the i2c_device_id array
+
+@ i2c_dev_id @
+identifier arr;
+@@
+struct i2c_device_id arr[] = { ... };
+
+// C2 : For now, we only want to match on I2C drivers
+
+@ dev_id depends on i2c_dev_id @
+identifier arr;
+@@
+struct of_device_id arr[] = { ... };
+
+// C2 : Check if we already export the MODULE_DEVICE_TABLE
+
+@ of_dev_table depends on dev_id @
+declarer name MODULE_DEVICE_TABLE;
+identifier of;
+identifier dev_id.arr;
+@@
+ MODULE_DEVICE_TABLE(of, arr);
+
+
+// A1: Export it!
+
+@ add_mod_dev_table depends on !of_dev_table @
+identifier dev_id.arr;
+@@
+struct of_device_id arr[] = { ... };
++ MODULE_DEVICE_TABLE(of, arr);
-- 
2.5.0

[PATCH] cocci: Provide script to find i2c_tables missing exports

[Kieran Bingham]

Adds MODULE_DEVICE_TABLE(i2c, ...) to correctly export tables in i2c drivers
---
Ahem, My appologies, I wrongly sent the patch I was working on for OF tables,

Of course this is the correct patch for adding the i2c_device_id exports,
which I think is what you are currently looking at
--
Kieran

 .../coccinelle/i2c/i2c_table_missing_export.cocci  | 30 ++++++++++++++++++++++
 1 file changed, 30 insertions(+)
 create mode 100644 scripts/coccinelle/i2c/i2c_table_missing_export.cocci

diff --git a/scripts/coccinelle/i2c/i2c_table_missing_export.cocci b/scripts/coccinelle/i2c/i2c_table_missing_export.cocci
new file mode 100644
index 000000000000..58c06856e4d4
--- /dev/null
+++ b/scripts/coccinelle/i2c/i2c_table_missing_export.cocci
@@ -0,0 +1,30 @@
+// Look for I2C drivers without an exported i2c_device_id table,
+// and export it using the MODULE_DEVICE_TABLE();
+//
+// Usage:
+// spatch --sp-file scripts/coccinelle/i2c/i2c_table_missing_export.cocci . --in-place
+
+// C1 : Identify the i2c_device_id array
+
+@ dev_id @
+identifier arr;
+@@
+struct i2c_device_id arr[] = { ... };
+
+// C2 : Check if we already export the MODULE_DEVICE_TABLE
+
+@ i2c_dev_table depends on dev_id @
+declarer name MODULE_DEVICE_TABLE;
+identifier i2c;
+identifier dev_id.arr;
+@@
+ MODULE_DEVICE_TABLE(i2c, arr);
+
+
+// A1: Export it!
+
+@ add_mod_dev_table depends on !i2c_dev_table @
+identifier dev_id.arr;
+@@
+struct i2c_device_id arr[] = { ... };
++ MODULE_DEVICE_TABLE(i2c, arr);
-- 
2.5.0

Re: [PATCH] cocci: Provide script to find i2c_tables missing exports

[Javier Martinez Canillas]

Hello Kieran,

Thanks for the patch, it would be great to automate this to avoid new
drivers to introduce new issues!

On 05/10/2016 11:07 AM, Kieran Bingham wrote:
> Adds MODULE_DEVICE_TABLE(i2c, ...) to correctly export tables in i2c drivers
> ---
> Ahem, My appologies, I wrongly sent the patch I was working on for OF tables,
>

No worries and in fact we need to identify both (I2C && OF tables not exported).

There are currently 3 issues that have to be fixed on I2C drivers w.r.t to device
and driver match and module auto-loading:

1) Drivers with I2C table but not exported (match will work but autoload no).

2) Drivers with OF table but not exported (match will work but autoload no).

3) Drivers with an I2C table but with no OF table even when the I2C devices
   are registered via OF. Match and autoload works because the core always
   reports a MODALIAS=i2c:* for both but once we change it to reports the
   corret modalias, things will break since now will be MODALIAS=of:N*fT*C
   and the module won't have a modalias of that form.

1) and 2) are somehow easy and your semantic patches can be used to find
and fix them.

Now 3) is more tricky since is hard to say if a DT is relying on the fact
that the I2C core strips the manufacture from the compatible string and
just matches using the model part.

I think the best we can do is to check if there are such a users in the
in-tree DTS or documented compatible in the DT bindings documents. But if
someone is using with a DT that's not in mainline, then things will break. 

So if you can think of a way to automatize 3) using coccinelle, that will
be great. My coccinelle knowledge is near to non-existent but one issue I
see is that coccinelle context seems to be restricted to just the current
file so it can't check in external files like DTS or DT bindings docs.

> Of course this is the correct patch for adding the i2c_device_id exports,
> which I think is what you are currently looking at
> --
> Kieran
> 

[snip]

> +
> +// A1: Export it!
> +
> +@ add_mod_dev_table depends on !i2c_dev_table @
> +identifier dev_id.arr;
> +@@
> +struct i2c_device_id arr[] = { ... };
> ++ MODULE_DEVICE_TABLE(i2c, arr);
> 

A problem with this semantic patch is that is going to give a lot of
false positives. Not always a MODULE_DEVICE_TABLE(i2c,...) is needed
after a i2c_device_id array, drivers that can't be build as a module
(i.e: whose Kconfig is not tristate) or board files are two examples.

One way I think we can minimize this is by checking if the driver is
including the module.h header file or not. This still will give some
false positives, since many drivers that can't be build as module
wrongly include module.h but in any case that's something that needs
to be fixed in those drivers.

So I think you need something like the following change to squash with
your patch:

diff --git a/scripts/coccinelle/i2c/i2c_table_missing_export.cocci b/scripts/coccinelle/i2c/i2c_table_missing_export.cocci
index 58c06856e4d4..df98279e326d 100644
--- a/scripts/coccinelle/i2c/i2c_table_missing_export.cocci
+++ b/scripts/coccinelle/i2c/i2c_table_missing_export.cocci
@@ -4,9 +4,16 @@
 // Usage:
 // spatch --sp-file scripts/coccinelle/i2c/i2c_table_missing_export.cocci . --in-place
 
+// C0 : Check if module.h is included
+
+@ includes_module @
+@@
+
+# include <linux/module.h>
+
 // C1 : Identify the i2c_device_id array
 
-@ dev_id @
+@ dev_id depends on includes_module @
 identifier arr;
 @@
 struct i2c_device_id arr[] = { ... };

Best regards,
-- 
Javier Martinez Canillas
Open Source Group
Samsung Research America

Re: [PATCHv5 0/8] 2c: Relax mandatory I2C ID table passing

[Lee Jones]

Wolfram,

Any reason this still has not been accepted?

On Wed, 04 May 2016, Kieran Bingham wrote:

> This patch set finally pops up again, after a long time stuck somewhere in the
> midst of my stack.
> 
> As it stood last year, the requirements were to rename probe2 to probe_new, and
> ensure that it was correctly tested. The rename was the easy bit, but the
> testing took me more time to get things set up properly. And other commitments
> then got in the way of things. Of course this patch set has also been rebased
> as well, but there wasn't any major pain there.
> 
> Testing
> -------
> 
> To try to establish testing, I have used a beagle-bone-black, and a DS1307 RTC
> connected to the BBB SCL and SDA lines. The main reason for these choices is
> accesibility. i.e. I have them, and the BBB readily boots a kernel for me to
> test and iterate with.
> 
> I've tested the device with i2cdetect, and then worked through testing the
> sysfs interface, device tree, and module autoloading, each time ensuring that
> the RTC enumerates and operates
> 
> * new_device (built-in, and external module)
>   echo ds1307 0x68 > /sys/bus/i2c/devices/i2c-2/new_device
>   cat /sys/class/rtc/rtc0/date
> 
>  - Both of those worked fine.
> 
> * Device Tree
>   I tested that the device would still register by adding a node in the device
>   tree for the board, and testing with a built-in module. 
> 
>  - This worked fine.
> 
> * Module Autoloading
>   With the device tree node in the board dts file, it wouldn't automatically
>   load from the external module. This was due to the rtc-ds1307 module not
>   exporting an of_match table, and not yet having Javier's "report OF style
>   modalias when probing using DT" [0]  patch applied
> 
>  - With the module updated, and Javiers patch applied, the module autoloads
> 
> Finally, I feel I can safely add this tag to the patch set:
> Tested-by: Kieran Bingham <kieran@bingham.xyz>
> 
> Please let me know if there is any other specific use case missing here that
> needs to be tested.
> 
> [0] https://patchwork.ozlabs.org/patch/502201/
> 
> Patches
> -------
> 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      | 75 +++++++++++++++++++++++++++++++++++++++------
>  drivers/mfd/88pm860x-core.c |  5 ++-
>  drivers/mfd/as3722.c        | 12 ++------
>  include/linux/i2c.h         | 22 ++++++++++++-
>  4 files changed, 91 insertions(+), 23 deletions(-)
> 

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

Re: [PATCHv5 0/8] 2c: Relax mandatory I2C ID table passing

[Wolfram Sang]

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

Hi Kieran,

> * Device Tree
>   I tested that the device would still register by adding a node in the device
>   tree for the board, and testing with a built-in module. 
> 
>  - This worked fine.
> 
> * Module Autoloading
>   With the device tree node in the board dts file, it wouldn't automatically
>   load from the external module. This was due to the rtc-ds1307 module not
>   exporting an of_match table, and not yet having Javier's "report OF style
>   modalias when probing using DT" [0]  patch applied

What I didn't get here: did your version of the RTC driver use probe()
or probe_new() without i2c_device_id table or did you try both? I assume
module autoloading only fails with probe_new(), otherwise we would be in
serious trouble. But I'd wonder then that userspace instantiation works.

Thanks to you and Javier for the testing. I pushed the patches to a
local branch for now and will merge once this question is clear.

Regards,

   Wolfram


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

Re: [PATCHv5 0/8] 2c: Relax mandatory I2C ID table passing

[Javier Martinez Canillas]

Hello Wolfram,

On 06/09/2016 03:15 PM, Wolfram Sang wrote:
> Hi Kieran,
> 
>> * Device Tree
>>   I tested that the device would still register by adding a node in the device
>>   tree for the board, and testing with a built-in module. 
>>
>>  - This worked fine.
>>
>> * Module Autoloading
>>   With the device tree node in the board dts file, it wouldn't automatically
>>   load from the external module. This was due to the rtc-ds1307 module not
>>   exporting an of_match table, and not yet having Javier's "report OF style
>>   modalias when probing using DT" [0]  patch applied
> 
> What I didn't get here: did your version of the RTC driver use probe()
> or probe_new() without i2c_device_id table or did you try both? I assume
> module autoloading only fails with probe_new(), otherwise we would be in
> serious trouble. But I'd wonder then that userspace instantiation works.
>

I can't answer for Kieran but you trimmed this last sentence from him:

>  - With the module updated, and Javiers patch applied, the module autoloads
>

So my understanding is that by updated he meant a patched rtc-ds1307 driver
using a .probe_new, whose i2c_device_id table was removed and of_device_id
table added (that's not present in the mainline driver).

And that's why he needed my RFC patch to report a MODALIAS=of:N*T*Cfoo,bar
and match what's exported to the module using the of_device_id table.

Because drivers that only use .probe and have an i2c_device_id table will
continue to match and report MODALIAS=i2c:foo as before after this series.

> Thanks to you and Javier for the testing. I pushed the patches to a
> local branch for now and will merge once this question is clear.
>
> Regards,
> 
>    Wolfram
> 

Best regards,
-- 
Javier Martinez Canillas
Open Source Group
Samsung Research America

Re: [PATCHv5 0/8] 2c: Relax mandatory I2C ID table passing

[Wolfram Sang]

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

On Thu, Jun 09, 2016 at 03:45:52PM -0400, Javier Martinez Canillas wrote:
> Hello Wolfram,
> 
> On 06/09/2016 03:15 PM, Wolfram Sang wrote:
> > Hi Kieran,
> > 
> >> * Device Tree
> >>   I tested that the device would still register by adding a node in the device
> >>   tree for the board, and testing with a built-in module. 
> >>
> >>  - This worked fine.
> >>
> >> * Module Autoloading
> >>   With the device tree node in the board dts file, it wouldn't automatically
> >>   load from the external module. This was due to the rtc-ds1307 module not
> >>   exporting an of_match table, and not yet having Javier's "report OF style
> >>   modalias when probing using DT" [0]  patch applied

Let's call this a)

> > 
> > What I didn't get here: did your version of the RTC driver use probe()
> > or probe_new() without i2c_device_id table or did you try both? I assume
> > module autoloading only fails with probe_new(), otherwise we would be in
> > serious trouble. But I'd wonder then that userspace instantiation works.
> >
> 
> I can't answer for Kieran but you trimmed this last sentence from him:
> 
> >  - With the module updated, and Javiers patch applied, the module autoloads

Let's call this b)

> >
> 
> So my understanding is that by updated he meant a patched rtc-ds1307 driver
> using a .probe_new, whose i2c_device_id table was removed and of_device_id
> table added (that's not present in the mainline driver).
> 
> And that's why he needed my RFC patch to report a MODALIAS=of:N*T*Cfoo,bar
> and match what's exported to the module using the of_device_id table.
> 
> Because drivers that only use .probe and have an i2c_device_id table will
> continue to match and report MODALIAS=i2c:foo as before after this series.

Yes, this is my understanding and expectation, too. However, he wrote
that module loading fails on a) where he never wrote anything about an
updated module before. That comes only later with b). So what I would
have expected:

1) update the module (which is "b)" above)
2) autoloading fails (which is "a)" above)
3) applying your patch
4) everything works

which implies

0) nothing done, everything works

But I don't see this in the text, so I better ask. This also raises the
question open how userspace instantiation was tested. With or without
updating (ideally both).

Thanks,

   Wolfram


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

Re: [PATCHv5 0/8] 2c: Relax mandatory I2C ID table passing

[Kieran Bingham]

Hi Wolfram,

On 09/06/16 21:04, Wolfram Sang wrote:
> On Thu, Jun 09, 2016 at 03:45:52PM -0400, Javier Martinez Canillas wrote:
>> Hello Wolfram,
>>
>> On 06/09/2016 03:15 PM, Wolfram Sang wrote:
>>> Hi Kieran,
>>>
>>>> * Device Tree
>>>>   I tested that the device would still register by adding a node in the device
>>>>   tree for the board, and testing with a built-in module. 
>>>>
>>>>  - This worked fine.
>>>>
>>>> * Module Autoloading
>>>>   With the device tree node in the board dts file, it wouldn't automatically
>>>>   load from the external module. This was due to the rtc-ds1307 module not
>>>>   exporting an of_match table, and not yet having Javier's "report OF style
>>>>   modalias when probing using DT" [0]  patch applied
> 
> Let's call this a)
> 
>>>
>>> What I didn't get here: did your version of the RTC driver use probe()
>>> or probe_new() without i2c_device_id table or did you try both? I assume
>>> module autoloading only fails with probe_new(), otherwise we would be in
>>> serious trouble. But I'd wonder then that userspace instantiation works.
>>>
>>
>> I can't answer for Kieran but you trimmed this last sentence from him:
>>
>>>  - With the module updated, and Javiers patch applied, the module autoloads

> Let's call this b)
> 
>>>
>>
>> So my understanding is that by updated he meant a patched rtc-ds1307 driver
>> using a .probe_new, whose i2c_device_id table was removed and of_device_id
>> table added (that's not present in the mainline driver).
>>
>> And that's why he needed my RFC patch to report a MODALIAS=of:N*T*Cfoo,bar
>> and match what's exported to the module using the of_device_id table.
>>
>> Because drivers that only use .probe and have an i2c_device_id table will
>> continue to match and report MODALIAS=i2c:foo as before after this series.

Yes, Javier's interpretation is correct here.

> Yes, this is my understanding and expectation, too. However, he wrote
> that module loading fails on a) where he never wrote anything about an
> updated module before. That comes only later with b). So what I would
> have expected:
> 
> 1) update the module (which is "b)" above)
> 2) autoloading fails (which is "a)" above)
> 3) applying your patch
> 4) everything works
> 
> which implies
> 
> 0) nothing done, everything works
> 
> But I don't see this in the text, so I better ask. This also raises the
> question open how userspace instantiation was tested. With or without
> updating (ideally both).

Well it's a month later, and I can't remember - so I've retested.

Kernel built with this patchset - and *without* Javiers patch for 0)
position test.

RTC_DS1307 driver *unmodified*

root@arm:~# uname -a
Linux arm 4.7.0-rc2-00027-g72baec98c9f0 #27 SMP Fri Jun 10 10:42:09 BST
2016 armv7l GNU/Linux

root@arm:~# lsmod
Module                  Size  Used by    Not tainted
omap_rng                3927  0
rng_core                6410  1 omap_rng
rtc_ds1307             12121  0

root@arm:~# cat /sys/class/rtc/rtc0/device/modalias
i2c:ds1307

root@arm:~# cat /sys/class/rtc/rtc0/date
2016-06-10


* Module autoload successful,
* i2c read successful.


Code tested at
repository https://github.com/kbingham/linux.git
tag i2c-dt/v4.7-rc2-relax-conversion-zero-test

Is this what you were looking for?


> Thanks,
> 
>    Wolfram
> 

-- 
Regards

Kieran Bingham

Re: [PATCHv5 0/8] 2c: Relax mandatory I2C ID table passing

[Wolfram Sang]

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


> Is this what you were looking for?

Mostly, thanks. This verifies that the old way still works. Good!

The new way (no i2c_device_ids, just compatibles) will need Javier's
patch to work with module auto loading, I know. But what about userspace
instantiation with built-in driver? I didn't understand if this was
tested using the new way. And do you need then the full-compatible or
the vendor-stripped string?

Thanks,

   Wolfram


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

Re: [PATCHv5 0/8] 2c: Relax mandatory I2C ID table passing

[Kieran Bingham]

Hi Wolfram,

On 10/06/16 12:00, Wolfram Sang wrote:
> 
>> Is this what you were looking for?
> 
> Mostly, thanks. This verifies that the old way still works. Good!
> 
> The new way (no i2c_device_ids, just compatibles) will need Javier's
> patch to work with module auto loading, I know. But what about userspace
> instantiation with built-in driver? I didn't understand if this was
> tested using the new way. And do you need then the full-compatible or
> the vendor-stripped string?

When I reported :

> * new_device (built-in, and external module)
> echo ds1307 0x68 > /sys/bus/i2c/devices/i2c-2/new_device
> cat /sys/class/rtc/rtc0/date
> 
> - Both of those worked fine.

That was *without* Javier's patch, but hopefully obviously *with* Lee's
patchset.

Do you need this testing *with* Javiers patch as well?
I have not tested this combination.

I've already switched my dev board environment back to the Salvator-X
for $DAYJOB and switching back now will have to wait to the weekend.


> Thanks,
> 
>    Wolfram
> 

-- 
Regards

Kieran Bingham

Re: [PATCHv5 0/8] 2c: Relax mandatory I2C ID table passing

[Wolfram Sang]

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


> When I reported :
> 
> > * new_device (built-in, and external module)
> > echo ds1307 0x68 > /sys/bus/i2c/devices/i2c-2/new_device
> > cat /sys/class/rtc/rtc0/date
> > 
> > - Both of those worked fine.
> 
> That was *without* Javier's patch, but hopefully obviously *with* Lee's
> patchset.
> 
> Do you need this testing *with* Javiers patch as well?

Without Javiers patch. But with a modified rtc driver which will only
use proper compatibles, no i2c_device_ids, and probe_new. And this one
should be able to instantiate via userspace with the driver builtin.

We have documented ways of instantiating. All I ask for is to make sure
the old style and new style work with them. Check the attached sketch
for an example (only compile-tested and conversion to probe_new is
missing). Can you instantiate the "maxim,ds1307" (with the additional
printout) and "dallas,ds1307" via userspace?

diff --git a/drivers/rtc/rtc-ds1307.c b/drivers/rtc/rtc-ds1307.c
index 821d9c089cdb48..10a4e5bc923e07 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,23 +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);
-
 /*----------------------------------------------------------------------*/
 
 #define BLOCK_DATA_MAX_TRIES 10
@@ -1435,6 +1423,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. */
@@ -1610,13 +1601,22 @@ static int ds1307_remove(struct i2c_client *client)
 	return 0;
 }
 
+#ifdef CONFIG_OF
+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, ds1302_dt_ids);
+#endif
+
 static struct i2c_driver ds1307_driver = {
 	.driver = {
 		.name	= "rtc-ds1307",
+		.of_match_table = of_match_ptr(ds1307_dt_ids),
 	},
 	.probe		= ds1307_probe,
 	.remove		= ds1307_remove,
-	.id_table	= ds1307_id,
 };
 
 module_i2c_driver(ds1307_driver);



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

[TEST PATCH] rtc: convert ds1307 to interim probe_new

[Kieran Bingham]

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

As we match on only the device, not the manufacturer, I've changed your sketch
so that the test maxim line is on a compatible with name ds9999 to make it
unique. Otherwise, we would match to the dallas,ds1307 id type.

If you would prefer that we support separate manufacturers, I can update the
match function so that it attempts a full match first, followed by a stripped
manufacturer match. I'm not certain if we have a need for that at the moment
though, as the current drivers simply match on the device name:

This patch also demonstrates a method to obtain the device ID from the new
match system for drivers which would normally have expected this information to
be passed in.


Testing:
=======-

root@arm:~# echo ds9999 0x68 > /sys/bus/i2c/devices/i2c-2/new_device
[ 43.262432] rtc-ds1307 2-0068: I'm a Maxim ...
[ 43.268707] rtc-ds1307 2-0068: rtc core: registered ds9999 as rtc0
[ 43.275276] rtc-ds1307 2-0068: 56 bytes nvram
[ 43.279920] i2c i2c-2: new_device: Instantiated device ds9999 at 0x68

root@arm:~# cat /sys/class/rtc/rtc0/date 
2016-06-12

root@arm:~# cat /sys/class/rtc/rtc0/name 
ds9999


---
 drivers/rtc/rtc-ds1307.c | 60 ++++++++++++++++++++++++++++++------------------
 1 file changed, 38 insertions(+), 22 deletions(-)

diff --git a/drivers/rtc/rtc-ds1307.c b/drivers/rtc/rtc-ds1307.c
index 821d9c089cdb..d97e8adb866b 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);
 
 /*----------------------------------------------------------------------*/
 
@@ -1226,13 +1215,27 @@ 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[] = {
+	/* We are only matching on the device name, *NOT* the manufacturer name
+	 * I.e. dallas, maxim, are dropped in the search when someone tries to load a
+	 * 'ds1307', and hence first match wins.
+	 *
+	 * We could extend this to do a full match first, followed by a fallback match
+	 * to just the device name.
+	 */
+	{ .compatible = "dallas,ds1307", .data = (void *)ds_1307 },
+	{ .compatible = "maxim,ds9999", .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;
-	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 +1258,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 +1448,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. */
@@ -1613,10 +1629,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: [TEST PATCH] rtc: convert ds1307 to interim probe_new

[kbuild test robot]

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

Hi,

[auto build test ERROR on abelloni/rtc-next]
[also build test ERROR on v4.7-rc3 next-20160609]
[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/rtc-convert-ds1307-to-interim-probe_new/20160613-051618
base:   https://git.kernel.org/pub/scm/linux/kernel/git/abelloni/linux.git rtc-next
config: sparc64-allmodconfig (attached as .config)
compiler: sparc64-linux-gnu-gcc (Debian 5.3.1-8) 5.3.1 20160205
reproduce:
        wget https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        make.cross ARCH=sparc64 

All error/warnings (new ones prefixed by >>):

   drivers/rtc/rtc-ds1307.c: In function 'ds1307_probe':
>> drivers/rtc/rtc-ds1307.c:1262:9: error: implicit declaration of function 'i2c_of_match_device' [-Werror=implicit-function-declaration]
     idof = i2c_of_match_device(ds1307_dt_ids, client);
            ^
>> drivers/rtc/rtc-ds1307.c:1262:7: warning: assignment makes pointer from integer without a cast [-Wint-conversion]
     idof = i2c_of_match_device(ds1307_dt_ids, client);
          ^
>> drivers/rtc/rtc-ds1307.c:1269:16: warning: cast from pointer to integer of different size [-Wpointer-to-int-cast]
     chip = &chips[(int)idof->data];
                   ^
   drivers/rtc/rtc-ds1307.c:1274:17: warning: cast from pointer to integer of different size [-Wpointer-to-int-cast]
     ds1307->type = (int) idof->data;
                    ^
   drivers/rtc/rtc-ds1307.c: At top level:
>> drivers/rtc/rtc-ds1307.c:1634:2: error: unknown field 'probe_new' specified in initializer
     .probe_new = ds1307_probe,
     ^
>> drivers/rtc/rtc-ds1307.c:1634:15: error: initialization from incompatible pointer type [-Werror=incompatible-pointer-types]
     .probe_new = ds1307_probe,
                  ^
   drivers/rtc/rtc-ds1307.c:1634:15: note: (near initialization for 'ds1307_driver.id_table')
   cc1: some warnings being treated as errors

vim +/i2c_of_match_device +1262 drivers/rtc/rtc-ds1307.c

  1256	
  1257		ds1307 = devm_kzalloc(&client->dev, sizeof(struct ds1307), GFP_KERNEL);
  1258		if (!ds1307)
  1259			return -ENOMEM;
  1260	
  1261		/* If we've got this far, this shouldn't be able to fail - but check anyway for now */
> 1262		idof = i2c_of_match_device(ds1307_dt_ids, client);
  1263		if (!idof) {
  1264			dev_err(&client->dev, "Probe failed to find an id entry\n");
  1265			return -ENODEV;
  1266		}
  1267	
  1268		/* Now we can set our chip entry */
> 1269		chip = &chips[(int)idof->data];
  1270	
  1271		i2c_set_clientdata(client, ds1307);
  1272	
  1273		ds1307->client	= client;
> 1274		ds1307->type	= (int) idof->data;
  1275	
  1276		if (!pdata && client->dev.of_node)
  1277			ds1307_trickle_of_init(client, chip);

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/octet-stream, Size: 46361 bytes --]

Re: [TEST PATCH] rtc: convert ds1307 to interim probe_new

[kbuild test robot]

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

Hi,

[auto build test WARNING on abelloni/rtc-next]
[also build test WARNING on v4.7-rc3 next-20160609]
[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/rtc-convert-ds1307-to-interim-probe_new/20160613-051618
base:   https://git.kernel.org/pub/scm/linux/kernel/git/abelloni/linux.git rtc-next
config: tile-allmodconfig (attached as .config)
compiler: tilegx-linux-gcc (GCC) 4.6.2
reproduce:
        wget https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        make.cross ARCH=tile 

All warnings (new ones prefixed by >>):

   drivers/rtc/rtc-ds1307.c: In function 'ds1307_probe':
   drivers/rtc/rtc-ds1307.c:1262:2: error: implicit declaration of function 'i2c_of_match_device'
   drivers/rtc/rtc-ds1307.c:1262:7: warning: assignment makes pointer from integer without a cast [enabled by default]
   drivers/rtc/rtc-ds1307.c:1269:16: warning: cast from pointer to integer of different size
   drivers/rtc/rtc-ds1307.c:1274:17: warning: cast from pointer to integer of different size
   drivers/rtc/rtc-ds1307.c: At top level:
   drivers/rtc/rtc-ds1307.c:1634:2: error: unknown field 'probe_new' specified in initializer
>> drivers/rtc/rtc-ds1307.c:1634:2: warning: initialization from incompatible pointer type [enabled by default]
   drivers/rtc/rtc-ds1307.c:1634:2: warning: (near initialization for 'ds1307_driver.id_table') [enabled by default]
   cc1: some warnings being treated as errors

vim +1634 drivers/rtc/rtc-ds1307.c

  1618	
  1619	static int ds1307_remove(struct i2c_client *client)
  1620	{
  1621		struct ds1307 *ds1307 = i2c_get_clientdata(client);
  1622	
  1623		if (test_and_clear_bit(HAS_NVRAM, &ds1307->flags))
  1624			sysfs_remove_bin_file(&client->dev.kobj, ds1307->nvram);
  1625	
  1626		return 0;
  1627	}
  1628	
  1629	static struct i2c_driver ds1307_driver = {
  1630		.driver = {
  1631			.name	= "rtc-ds1307",
  1632			.of_match_table = of_match_ptr(ds1307_dt_ids),
  1633		},
> 1634		.probe_new	= ds1307_probe,
  1635		.remove		= ds1307_remove,
  1636	};
  1637	
  1638	module_i2c_driver(ds1307_driver);
  1639	
  1640	MODULE_DESCRIPTION("RTC driver for DS1307 and similar chips");
  1641	MODULE_LICENSE("GPL");

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/octet-stream, Size: 44854 bytes --]

Re: [TEST PATCH] rtc: convert ds1307 to interim probe_new

[Wolfram Sang]

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


> As we match on only the device, not the manufacturer, I've changed your sketch
> so that the test maxim line is on a compatible with name ds9999 to make it
> unique. Otherwise, we would match to the dallas,ds1307 id type.

OK, so I assumed correctly that userspace instantiation wouldn't have
worked fully.

> If you would prefer that we support separate manufacturers, I can update the
> match function so that it attempts a full match first, followed by a stripped
> manufacturer match. I'm not certain if we have a need for that at the moment
> though, as the current drivers simply match on the device name:

I think we *need* that. We can't instantiate my previous example via
userspace otherwise. Also, stripping vendor names is what we want to get
rid of with this series, no?

> This patch also demonstrates a method to obtain the device ID from the new
> match system for drivers which would normally have expected this information to
> be passed in.

Thanks for the testing!

   Wolfram


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

Re: [TEST PATCH] rtc: convert ds1307 to interim probe_new

[Kieran Bingham]

Hi Wolfram,

On 13/06/16 18:13, Wolfram Sang wrote:
> 
>> As we match on only the device, not the manufacturer, I've changed your sketch
>> so that the test maxim line is on a compatible with name ds9999 to make it
>> unique. Otherwise, we would match to the dallas,ds1307 id type.
> 
> OK, so I assumed correctly that userspace instantiation wouldn't have
> worked fully.
> 
>> If you would prefer that we support separate manufacturers, I can update the
>> match function so that it attempts a full match first, followed by a stripped
>> manufacturer match. I'm not certain if we have a need for that at the moment
>> though, as the current drivers simply match on the device name:
> 
> I think we *need* that. We can't instantiate my previous example via
> userspace otherwise. Also, stripping vendor names is what we want to get
> rid of with this series, no?

Awww <multiple expletives removed>

I just re-read this - Has this series been blocked on me without me
realising?

/me cowers in a corner in shame...

I think the aim of this series was to simplify the code structures.

Making the vendor names usable, makes sense IMO, but I don't think that
was a goal of the original series.

I think the series was trying to make an improvement *without* changing
functionality / usage.

Re-introducing vendor names into the i2c matching would be a change in
scope from my view, but if it's required to get this series moving let
me know.

>> This patch also demonstrates a method to obtain the device ID from the new
>> match system for drivers which would normally have expected this information to
>> be passed in.
> 
> Thanks for the testing!
> 
>    Wolfram
> 

-- 
Regards

Kieran Bingham