[RESEND PATCH v4 0/8] i2c: Relax mandatory I2C ID table passing

[RESEND PATCH v4 0/8] i2c: Relax mandatory I2C ID table passing

[Kieran Bingham]

Hi Wolfram,

I have picked this patchset [0] up from Lee to rebase it, with an aim to
get this series moving again.

This resend fixes up my SoB's as highlighted by Lee

A couple of minor issues were resolved in the rebase. As it stood, Javier
proposed [1] to merge this series, and use a follow up series to make sure
that all I2C drivers are using a MODLE_DEVICE_TABLE(of,...)

I have prepared a Coccinelle patch to work through the bulk of the changes
required for the conversion, which will assist the transition process.

Once this patch set is accepted, I will commence converting the other
drivers, and submitting with a per subsystem breakdown or simliar to
reduce traffic.

[0] https://lkml.org/lkml/2014/8/28/283
[1] https://lkml.org/lkml/2014/9/12/496

Lee's most recent cover-letter (from 12 months ago) follows:

Hi Wolfram,

Placing this firmly back on your plate.  I truly hope we don't miss
another merge-window.  This patch-set has the support of some pretty
senior kernel maintainers, so I hope acceptance shouldn't be too
difficult.

As previously discussed I believe it should be okay for an I2C device
driver _not_ supply an I2C ID table to match to.  The I2C subsystem
should be able to match via other means, such as via OF tables.  The
blocking factor during our previous conversation was to keep
registering via sysfs up and running.  This set does that.

After thinking more deeply about the problem, it occurred to me that
any I2C device driver which uses the sysfs method and issues an
of_match_device() would also fail their probe().  Bolted on to this
set is a new, more generic way for these devices to match against
either of the I2C/OF tables.

I hope this ticks all of your boxes.

v4: [Kieran Bingham]
  - Rebase to v4.2
  - adapt for dev_pm_domain_{attach,detach}
  - strnicmp to strncasecmp

v3: [Lee Jones]
  - Insist on passing 'struct i2c_client' instead of 'struct device'
  - Remove hook from of_match_device()

v2: [Lee Jones]
  - Removal of ACPI support (this is really an OF issue).
  - Add a new .probe2( with will seamlessly replace
  - Supply a warning on devices matching via OF without a suitable compatible
  - Remove unified match_device() - bad idea as it subverts type-safe behaviour
  - Provide examples of the kind of clean-up possible after this set.
    - I already have the full support from the maintainer of these drivers =;-)

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

-- 
2.1.4

[RESEND PATCH v4 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 <kieranbingham@gmail.com>
---
 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 c83e4d1..30d8a77 100644
--- a/drivers/i2c/i2c-core.c
+++ b/drivers/i2c/i2c-core.c
@@ -450,6 +450,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;
@@ -463,8 +466,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))
@@ -475,9 +476,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.1.4

[RESEND PATCH v4 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 <kieranbingham@gmail.com>
---
 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 30d8a77..0788c1f 100644
--- a/drivers/i2c/i2c-core.c
+++ b/drivers/i2c/i2c-core.c
@@ -1373,6 +1373,27 @@ struct i2c_adapter *of_find_i2c_adapter_by_node(struct device_node *node)
 	return adapter;
 }
 EXPORT_SYMBOL(of_find_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.1.4

[RESEND PATCH v4 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>
Signed-off-by: Kieran Bingham <kieranbingham@gmail.com>
---
 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 0788c1f..94ca76e 100644
--- a/drivers/i2c/i2c-core.c
+++ b/drivers/i2c/i2c-core.c
@@ -1394,6 +1394,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 e83a738..48bbbab 100644
--- a/include/linux/i2c.h
+++ b/include/linux/i2c.h
@@ -638,6 +638,10 @@ extern struct i2c_client *of_find_i2c_device_by_node(struct device_node *node);
 /* must call put_device() when done with returned i2c_adapter device */
 extern struct i2c_adapter *of_find_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)
@@ -649,6 +653,14 @@ static inline struct i2c_adapter *of_find_i2c_adapter_by_node(struct device_node
 {
 	return NULL;
 }
+
+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.1.4

[RESEND PATCH v4 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 <kieranbingham@gmail.com>
---
 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 2ebc64d..0e40136 100644
--- a/drivers/i2c/i2c-core.c
+++ b/drivers/i2c/i2c-core.c
@@ -447,7 +447,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))
@@ -460,6 +460,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 48bbbab..787066b 100644
--- a/include/linux/i2c.h
+++ b/include/linux/i2c.h
@@ -232,6 +232,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.1.4

[RESEND PATCH v4 4/8] i2c: Make I2C ID tables non-mandatory for DT'ed devices

[Kieran Bingham]

From: Lee Jones <lee.jones-QSEj5FYQhm4dnm+yROfE0A@public.gmane.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-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
Signed-off-by: Lee Jones <lee.jones-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
Signed-off-by: Kieran Bingham <kieranbingham-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
---
 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 94ca76e..2ebc64d 100644
--- a/drivers/i2c/i2c-core.c
+++ b/drivers/i2c/i2c-core.c
@@ -468,7 +468,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 */
@@ -657,7 +657,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 (!device_can_wakeup(&client->dev))
-- 
2.1.4

[RESEND PATCH v4 6/8] i2c: Provide a temporary .probe2() call-back type

[Kieran Bingham]

From: Lee Jones <lee.jones-QSEj5FYQhm4dnm+yROfE0A@public.gmane.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-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
Signed-off-by: Lee Jones <lee.jones-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
[Kieran: fix rebase conflicts and adapt for dev_pm_domain_{attach,detach}]
Signed-off-by: Kieran Bingham <kieranbingham-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
---
 drivers/i2c/i2c-core.c | 22 ++++++++++++++--------
 include/linux/i2c.h    |  8 +++++++-
 2 files changed, 21 insertions(+), 9 deletions(-)

diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
index 0e40136..a28b423 100644
--- a/drivers/i2c/i2c-core.c
+++ b/drivers/i2c/i2c-core.c
@@ -658,8 +658,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
@@ -679,12 +677,20 @@ static int i2c_device_probe(struct device *dev)
 		return status;
 
 	status = dev_pm_domain_attach(&client->dev, true);
-	if (status != -EPROBE_DEFER) {
-		status = driver->probe(client, i2c_match_id(driver->id_table,
-					client));
-		if (status)
-			dev_pm_domain_detach(&client->dev, true);
-	}
+	if (status == -EPROBE_DEFER)
+		return status;
+
+	/* When there are no more users of probe(), rename probe2 to probe. */
+	if (driver->probe2)
+		status = driver->probe2(client);
+	else if (driver->probe)
+		status = driver->probe(client,
+					i2c_match_id(driver->id_table, client));
+	else
+		status = -EINVAL;
+
+	if (status)
+		dev_pm_domain_detach(&client->dev, true);
 
 	return status;
 }
diff --git a/include/linux/i2c.h b/include/linux/i2c.h
index 787066b..bf675d5 100644
--- a/include/linux/i2c.h
+++ b/include/linux/i2c.h
@@ -127,7 +127,8 @@ extern s32 i2c_smbus_write_i2c_block_data(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
+ * @probe2: 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
@@ -170,6 +171,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 (*probe2)(struct i2c_client *);
+
 	/* driver model interfaces that don't relate to enumeration  */
 	void (*shutdown)(struct i2c_client *);
 
-- 
2.1.4

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

[Kieran Bingham]

From: Lee Jones <lee.jones-QSEj5FYQhm4dnm+yROfE0A@public.gmane.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-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
Signed-off-by: Lee Jones <lee.jones-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
Signed-off-by: Kieran Bingham <kieranbingham-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
---
 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 3269a99..a499d70 100644
--- a/drivers/mfd/88pm860x-core.c
+++ b/drivers/mfd/88pm860x-core.c
@@ -1130,8 +1130,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;
@@ -1257,7 +1256,7 @@ static struct i2c_driver pm860x_driver = {
 		.pm     = &pm860x_pm_ops,
 		.of_match_table	= pm860x_dt_ids,
 	},
-	.probe		= pm860x_probe,
+	.probe2		= pm860x_probe,
 	.remove		= pm860x_remove,
 	.id_table	= pm860x_id_table,
 };
-- 
2.1.4

[RESEND PATCH v4 8/8] mfd: as3722: Rid driver of superfluous I2C device ID structure

[Kieran Bingham]

From: Lee Jones <lee.jones-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>

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

Acked-by: Grant Likely <grant.likely-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
Signed-off-by: Lee Jones <lee.jones-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
Signed-off-by: Kieran Bingham <kieranbingham-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
---
 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 924ea90..6ac4be3 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;
@@ -428,20 +427,13 @@ 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 struct i2c_driver as3722_i2c_driver = {
 	.driver = {
 		.name = "as3722",
 		.of_match_table = as3722_of_match,
 	},
-	.probe = as3722_i2c_probe,
+	.probe2 = as3722_i2c_probe,
 	.remove = as3722_i2c_remove,
-	.id_table = as3722_i2c_id,
 };
 
 module_i2c_driver(as3722_i2c_driver);
-- 
2.1.4

Re: [RESEND PATCH v4 0/8] i2c: Relax mandatory I2C ID table passing

[Javier Martinez Canillas]

Hello,

On 09/11/2015 01:55 PM, Kieran Bingham wrote:
> Hi Wolfram,
> 
> I have picked this patchset [0] up from Lee to rebase it, with an aim to
> get this series moving again.
> 
> This resend fixes up my SoB's as highlighted by Lee
> 
> A couple of minor issues were resolved in the rebase. As it stood, Javier
> proposed [1] to merge this series, and use a follow up series to make sure
> that all I2C drivers are using a MODLE_DEVICE_TABLE(of,...)
> 
> I have prepared a Coccinelle patch to work through the bulk of the changes
> required for the conversion, which will assist the transition process.
> 
> Once this patch set is accepted, I will commence converting the other
> drivers, and submitting with a per subsystem breakdown or simliar to
> reduce traffic.
> 
> [0] https://lkml.org/lkml/2014/8/28/283
> [1] https://lkml.org/lkml/2014/9/12/496
> 
> Lee's most recent cover-letter (from 12 months ago) follows:
> 
> Hi Wolfram,
> 
> Placing this firmly back on your plate.  I truly hope we don't miss
> another merge-window.  This patch-set has the support of some pretty
> senior kernel maintainers, so I hope acceptance shouldn't be too
> difficult.
> 
> As previously discussed I believe it should be okay for an I2C device
> driver _not_ supply an I2C ID table to match to.  The I2C subsystem
> should be able to match via other means, such as via OF tables.  The
> blocking factor during our previous conversation was to keep
> registering via sysfs up and running.  This set does that.
> 
> After thinking more deeply about the problem, it occurred to me that
> any I2C device driver which uses the sysfs method and issues an
> of_match_device() would also fail their probe().  Bolted on to this
> set is a new, more generic way for these devices to match against
> either of the I2C/OF tables.
> 

I reviewed this series but wonder if we shouldn't take another approach.

There are two reasons why a I2C device ID table is needed even when devices
are registered via OF:

1) Export the module aliases from the I2C device ID table so userspace
   can auto-load the correct module. This is because i2c_device_uevent
   always reports a MODALIAS of the form i2c:<client->name>.

2) Match the I2C client with a I2C device ID so a struct i2c_device_id
   is passed to the I2C driver probe() function.

As Kieran mentioned I proposed a transition path to fix 1) and posted
these patches: https://lkml.org/lkml/2015/7/30/519

While this series are fixing 2) by changing the matching logic and adding
a second probe callback for drivers that don't need to get a i2c_device_id.

Now, the problem I see with this approach is that drivers will likely want
to get the struct of_device_id .data field since that is the reason why
the I2C core pass a pointer to the struct i2c_device_id in the first place.
So drivers could get the .driver_data field and take decisions depending on
which device was registered / matched.

If the parameter is removed from probe, it means that drivers will have to
to open code to get it. This is the same issue that exist with OF today, if
a driver needs the .data field from the of_device_id table, is has to do:

struct of_device_id *match of_match_node(of_match_table, i2c->dev.of_node);

to get the match->data. And a similar helper will be needed to get the
struct i2c_device_id since the core won't do it anymore.

So what I propose is to change the probe callback signature instead to have
a const void *data as the second parameter and the core can either lookup
from the I2C device ID table or the OF device ID table depending on which
mechanism was used to register the I2C device.

That way legacy drivers will only need a I2C device ID table and DT drivers
will only need a OF device ID table and drivers won't need to open code the
matching logic to get the data stored in the tables.

Drivers that support both legacy platform and OF based registration, will
have both tables and the I2C core will lookup the data from the right table.

An alternative is to keep this series but have a generic function that gets
a pointer to a struct i2c_client as parameter and returns the data from the
correct table so drivers that don't need that information won't get it at
probe time but drivers that need it, can get it easily assisted by the core.

Both options are similar, the question is if the I2C core should match and
lookup the entry from the correct table on probe or let drivers do it later.

Any thoughts?

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

Re: [RESEND PATCH v4 0/8] i2c: Relax mandatory I2C ID table passing

[Lee Jones]

On Thu, 17 Sep 2015, Javier Martinez Canillas wrote:

> Hello,
> 
> On 09/11/2015 01:55 PM, Kieran Bingham wrote:
> > Hi Wolfram,
> > 
> > I have picked this patchset [0] up from Lee to rebase it, with an aim to
> > get this series moving again.
> > 
> > This resend fixes up my SoB's as highlighted by Lee
> > 
> > A couple of minor issues were resolved in the rebase. As it stood, Javier
> > proposed [1] to merge this series, and use a follow up series to make sure
> > that all I2C drivers are using a MODLE_DEVICE_TABLE(of,...)
> > 
> > I have prepared a Coccinelle patch to work through the bulk of the changes
> > required for the conversion, which will assist the transition process.
> > 
> > Once this patch set is accepted, I will commence converting the other
> > drivers, and submitting with a per subsystem breakdown or simliar to
> > reduce traffic.
> > 
> > [0] https://lkml.org/lkml/2014/8/28/283
> > [1] https://lkml.org/lkml/2014/9/12/496
> > 
> > Lee's most recent cover-letter (from 12 months ago) follows:
> > 
> > Hi Wolfram,
> > 
> > Placing this firmly back on your plate.  I truly hope we don't miss
> > another merge-window.  This patch-set has the support of some pretty
> > senior kernel maintainers, so I hope acceptance shouldn't be too
> > difficult.
> > 
> > As previously discussed I believe it should be okay for an I2C device
> > driver _not_ supply an I2C ID table to match to.  The I2C subsystem
> > should be able to match via other means, such as via OF tables.  The
> > blocking factor during our previous conversation was to keep
> > registering via sysfs up and running.  This set does that.
> > 
> > After thinking more deeply about the problem, it occurred to me that
> > any I2C device driver which uses the sysfs method and issues an
> > of_match_device() would also fail their probe().  Bolted on to this
> > set is a new, more generic way for these devices to match against
> > either of the I2C/OF tables.
> > 
> 
> I reviewed this series but wonder if we shouldn't take another approach.
> 
> There are two reasons why a I2C device ID table is needed even when devices
> are registered via OF:
> 
> 1) Export the module aliases from the I2C device ID table so userspace
>    can auto-load the correct module. This is because i2c_device_uevent
>    always reports a MODALIAS of the form i2c:<client->name>.
> 
> 2) Match the I2C client with a I2C device ID so a struct i2c_device_id
>    is passed to the I2C driver probe() function.
> 
> As Kieran mentioned I proposed a transition path to fix 1) and posted
> these patches: https://lkml.org/lkml/2015/7/30/519
> 
> While this series are fixing 2) by changing the matching logic and adding
> a second probe callback for drivers that don't need to get a i2c_device_id.
> 
> Now, the problem I see with this approach is that drivers will likely want
> to get the struct of_device_id .data field since that is the reason why
> the I2C core pass a pointer to the struct i2c_device_id in the first place.
> So drivers could get the .driver_data field and take decisions depending on
> which device was registered / matched.
> 
> If the parameter is removed from probe, it means that drivers will have to
> to open code to get it. This is the same issue that exist with OF today, if
> a driver needs the .data field from the of_device_id table, is has to do:
> 
> struct of_device_id *match of_match_node(of_match_table, i2c->dev.of_node);
> 
> to get the match->data. And a similar helper will be needed to get the
> struct i2c_device_id since the core won't do it anymore.
> 
> So what I propose is to change the probe callback signature instead to have
> a const void *data as the second parameter and the core can either lookup
> from the I2C device ID table or the OF device ID table depending on which
> mechanism was used to register the I2C device.
> 
> That way legacy drivers will only need a I2C device ID table and DT drivers
> will only need a OF device ID table and drivers won't need to open code the
> matching logic to get the data stored in the tables.
> 
> Drivers that support both legacy platform and OF based registration, will
> have both tables and the I2C core will lookup the data from the right table.
> 
> An alternative is to keep this series but have a generic function that gets
> a pointer to a struct i2c_client as parameter and returns the data from the
> correct table so drivers that don't need that information won't get it at
> probe time but drivers that need it, can get it easily assisted by the core.

You mean like i2c_match_id()? ;)

This patch already takes care of this issue.  Please see:

  i2c: Export i2c_match_id() for direct use by device drivers

Drivers will know if they either only supply an I2C or OF table, so
they will know which call to use in order to obtain their
.driver_data|.data. attributes.  We can generify the call if you think
that makes things easier, but I don't see a need for it ATM.

> Both options are similar, the question is if the I2C core should match and
> lookup the entry from the correct table on probe or let drivers do it later.

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

Re: [RESEND PATCH v4 0/8] i2c: Relax mandatory I2C ID table passing

[Javier Martinez Canillas]

Hello Lee,

On 09/20/2015 06:15 AM, Lee Jones wrote:
> On Thu, 17 Sep 2015, Javier Martinez Canillas wrote:
> 
>> Hello,
>>
>> On 09/11/2015 01:55 PM, Kieran Bingham wrote:
>>> Hi Wolfram,
>>>
>>> I have picked this patchset [0] up from Lee to rebase it, with an aim to
>>> get this series moving again.
>>>
>>> This resend fixes up my SoB's as highlighted by Lee
>>>
>>> A couple of minor issues were resolved in the rebase. As it stood, Javier
>>> proposed [1] to merge this series, and use a follow up series to make sure
>>> that all I2C drivers are using a MODLE_DEVICE_TABLE(of,...)
>>>
>>> I have prepared a Coccinelle patch to work through the bulk of the changes
>>> required for the conversion, which will assist the transition process.
>>>
>>> Once this patch set is accepted, I will commence converting the other
>>> drivers, and submitting with a per subsystem breakdown or simliar to
>>> reduce traffic.
>>>
>>> [0] https://lkml.org/lkml/2014/8/28/283
>>> [1] https://lkml.org/lkml/2014/9/12/496
>>>
>>> Lee's most recent cover-letter (from 12 months ago) follows:
>>>
>>> Hi Wolfram,
>>>
>>> Placing this firmly back on your plate.  I truly hope we don't miss
>>> another merge-window.  This patch-set has the support of some pretty
>>> senior kernel maintainers, so I hope acceptance shouldn't be too
>>> difficult.
>>>
>>> As previously discussed I believe it should be okay for an I2C device
>>> driver _not_ supply an I2C ID table to match to.  The I2C subsystem
>>> should be able to match via other means, such as via OF tables.  The
>>> blocking factor during our previous conversation was to keep
>>> registering via sysfs up and running.  This set does that.
>>>
>>> After thinking more deeply about the problem, it occurred to me that
>>> any I2C device driver which uses the sysfs method and issues an
>>> of_match_device() would also fail their probe().  Bolted on to this
>>> set is a new, more generic way for these devices to match against
>>> either of the I2C/OF tables.
>>>
>>
>> I reviewed this series but wonder if we shouldn't take another approach.
>>
>> There are two reasons why a I2C device ID table is needed even when devices
>> are registered via OF:
>>
>> 1) Export the module aliases from the I2C device ID table so userspace
>>    can auto-load the correct module. This is because i2c_device_uevent
>>    always reports a MODALIAS of the form i2c:<client->name>.
>>
>> 2) Match the I2C client with a I2C device ID so a struct i2c_device_id
>>    is passed to the I2C driver probe() function.
>>
>> As Kieran mentioned I proposed a transition path to fix 1) and posted
>> these patches: https://lkml.org/lkml/2015/7/30/519
>>
>> While this series are fixing 2) by changing the matching logic and adding
>> a second probe callback for drivers that don't need to get a i2c_device_id.
>>
>> Now, the problem I see with this approach is that drivers will likely want
>> to get the struct of_device_id .data field since that is the reason why
>> the I2C core pass a pointer to the struct i2c_device_id in the first place.
>> So drivers could get the .driver_data field and take decisions depending on
>> which device was registered / matched.
>>
>> If the parameter is removed from probe, it means that drivers will have to
>> to open code to get it. This is the same issue that exist with OF today, if
>> a driver needs the .data field from the of_device_id table, is has to do:
>>
>> struct of_device_id *match of_match_node(of_match_table, i2c->dev.of_node);
>>
>> to get the match->data. And a similar helper will be needed to get the
>> struct i2c_device_id since the core won't do it anymore.
>>
>> So what I propose is to change the probe callback signature instead to have
>> a const void *data as the second parameter and the core can either lookup
>> from the I2C device ID table or the OF device ID table depending on which
>> mechanism was used to register the I2C device.
>>
>> That way legacy drivers will only need a I2C device ID table and DT drivers
>> will only need a OF device ID table and drivers won't need to open code the
>> matching logic to get the data stored in the tables.
>>
>> Drivers that support both legacy platform and OF based registration, will
>> have both tables and the I2C core will lookup the data from the right table.
>>
>> An alternative is to keep this series but have a generic function that gets
>> a pointer to a struct i2c_client as parameter and returns the data from the
>> correct table so drivers that don't need that information won't get it at
>> probe time but drivers that need it, can get it easily assisted by the core.
> 
> You mean like i2c_match_id()? ;)
> 

Well, I meant a more generic function that returns const void * or unsigned long
instead of a struct i2c_device_id *

> This patch already takes care of this issue.  Please see:
> 
>   i2c: Export i2c_match_id() for direct use by device drivers
>

I know but as I said that is only used to get a struct i2c_device_id *, a
driver has to call i2c_of_match_device() to get a struct of_device_id *

> Drivers will know if they either only supply an I2C or OF table, so
> they will know which call to use in order to obtain their

Yes but that is not true for drivers that support both OF and legacy board
files. For those drivers, there will be a lot of boiler plate code duplicated
that would look something like:

     unsigned long data;
     struct of_device_id *match;
     struct i2c_devicd_id *id;

     if (i2c->dev.of_node) {
            match = i2c_of_match_device(of_match_table, i2c);
	    if (!match)
	           return -EINVAL;

            data = (unsigned long)match->data;
     } else {
            id = i2c_match_id(id_table, i2c);
	    if (!id)
	           return -EINVAL;

            data = id->driver_data;
     }

While it would be nice to have something like:

    data = i2c_get_data(i2c);

and let the core handle which table should be looked up depending on
which mechanism was used to register the i2c device (legacy or OF).

> .driver_data|.data. attributes.  We can generify the call if you think
> that makes things easier, but I don't see a need for it ATM.
>

As I explained above, it will make easier for drivers but I raised the
point to discuss if the table data should be looked up by the driver
or if the core should get it and pass to the probe() function as it is
made right now for the I2C device ID table. i.e:

static int foo_i2c_probe(struct i2c_client *i2c, const void *data)

If the correct approach is the former, then this series is the right
direction and as you said a generic match function can be added later.

But if the correct approach is the latter, then this series is not
the right direction and a different approach is needed. I don't have
a strong opinion but wanted to mention that we have two options here.

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

Re: [RESEND PATCH v4 0/8] i2c: Relax mandatory I2C ID table passing

[Lee Jones]

On Thu, 24 Sep 2015, Javier Martinez Canillas wrote:
> On 09/20/2015 06:15 AM, Lee Jones wrote:
> > On Thu, 17 Sep 2015, Javier Martinez Canillas wrote:
> > 
> >> Hello,
> >>
> >> On 09/11/2015 01:55 PM, Kieran Bingham wrote:
> >>> Hi Wolfram,
> >>>
> >>> I have picked this patchset [0] up from Lee to rebase it, with an aim to
> >>> get this series moving again.
> >>>
> >>> This resend fixes up my SoB's as highlighted by Lee
> >>>
> >>> A couple of minor issues were resolved in the rebase. As it stood, Javier
> >>> proposed [1] to merge this series, and use a follow up series to make sure
> >>> that all I2C drivers are using a MODLE_DEVICE_TABLE(of,...)
> >>>
> >>> I have prepared a Coccinelle patch to work through the bulk of the changes
> >>> required for the conversion, which will assist the transition process.
> >>>
> >>> Once this patch set is accepted, I will commence converting the other
> >>> drivers, and submitting with a per subsystem breakdown or simliar to
> >>> reduce traffic.
> >>>
> >>> [0] https://lkml.org/lkml/2014/8/28/283
> >>> [1] https://lkml.org/lkml/2014/9/12/496
> >>>
> >>> Lee's most recent cover-letter (from 12 months ago) follows:
> >>>
> >>> Hi Wolfram,
> >>>
> >>> Placing this firmly back on your plate.  I truly hope we don't miss
> >>> another merge-window.  This patch-set has the support of some pretty
> >>> senior kernel maintainers, so I hope acceptance shouldn't be too
> >>> difficult.
> >>>
> >>> As previously discussed I believe it should be okay for an I2C device
> >>> driver _not_ supply an I2C ID table to match to.  The I2C subsystem
> >>> should be able to match via other means, such as via OF tables.  The
> >>> blocking factor during our previous conversation was to keep
> >>> registering via sysfs up and running.  This set does that.
> >>>
> >>> After thinking more deeply about the problem, it occurred to me that
> >>> any I2C device driver which uses the sysfs method and issues an
> >>> of_match_device() would also fail their probe().  Bolted on to this
> >>> set is a new, more generic way for these devices to match against
> >>> either of the I2C/OF tables.
> >>>
> >>
> >> I reviewed this series but wonder if we shouldn't take another approach.
> >>
> >> There are two reasons why a I2C device ID table is needed even when devices
> >> are registered via OF:
> >>
> >> 1) Export the module aliases from the I2C device ID table so userspace
> >>    can auto-load the correct module. This is because i2c_device_uevent
> >>    always reports a MODALIAS of the form i2c:<client->name>.
> >>
> >> 2) Match the I2C client with a I2C device ID so a struct i2c_device_id
> >>    is passed to the I2C driver probe() function.
> >>
> >> As Kieran mentioned I proposed a transition path to fix 1) and posted
> >> these patches: https://lkml.org/lkml/2015/7/30/519
> >>
> >> While this series are fixing 2) by changing the matching logic and adding
> >> a second probe callback for drivers that don't need to get a i2c_device_id.
> >>
> >> Now, the problem I see with this approach is that drivers will likely want
> >> to get the struct of_device_id .data field since that is the reason why
> >> the I2C core pass a pointer to the struct i2c_device_id in the first place.
> >> So drivers could get the .driver_data field and take decisions depending on
> >> which device was registered / matched.
> >>
> >> If the parameter is removed from probe, it means that drivers will have to
> >> to open code to get it. This is the same issue that exist with OF today, if
> >> a driver needs the .data field from the of_device_id table, is has to do:
> >>
> >> struct of_device_id *match of_match_node(of_match_table, i2c->dev.of_node);
> >>
> >> to get the match->data. And a similar helper will be needed to get the
> >> struct i2c_device_id since the core won't do it anymore.
> >>
> >> So what I propose is to change the probe callback signature instead to have
> >> a const void *data as the second parameter and the core can either lookup
> >> from the I2C device ID table or the OF device ID table depending on which
> >> mechanism was used to register the I2C device.
> >>
> >> That way legacy drivers will only need a I2C device ID table and DT drivers
> >> will only need a OF device ID table and drivers won't need to open code the
> >> matching logic to get the data stored in the tables.
> >>
> >> Drivers that support both legacy platform and OF based registration, will
> >> have both tables and the I2C core will lookup the data from the right table.
> >>
> >> An alternative is to keep this series but have a generic function that gets
> >> a pointer to a struct i2c_client as parameter and returns the data from the
> >> correct table so drivers that don't need that information won't get it at
> >> probe time but drivers that need it, can get it easily assisted by the core.
> > 
> > You mean like i2c_match_id()? ;)
> > 
> 
> Well, I meant a more generic function that returns const void * or unsigned long
> instead of a struct i2c_device_id *
> 
> > This patch already takes care of this issue.  Please see:
> > 
> >   i2c: Export i2c_match_id() for direct use by device drivers
> >
> 
> I know but as I said that is only used to get a struct i2c_device_id *, a
> driver has to call i2c_of_match_device() to get a struct of_device_id *
> 
> > Drivers will know if they either only supply an I2C or OF table, so
> > they will know which call to use in order to obtain their
> 
> Yes but that is not true for drivers that support both OF and legacy board
> files. For those drivers, there will be a lot of boiler plate code duplicated
> that would look something like:
> 
>      unsigned long data;
>      struct of_device_id *match;
>      struct i2c_devicd_id *id;
> 
>      if (i2c->dev.of_node) {
>             match = i2c_of_match_device(of_match_table, i2c);
> 	    if (!match)
> 	           return -EINVAL;
> 
>             data = (unsigned long)match->data;
>      } else {
>             id = i2c_match_id(id_table, i2c);
> 	    if (!id)
> 	           return -EINVAL;
> 
>             data = id->driver_data;
>      }
> 
> While it would be nice to have something like:
> 
>     data = i2c_get_data(i2c);
> 
> and let the core handle which table should be looked up depending on
> which mechanism was used to register the i2c device (legacy or OF).

I'm fine with a new API for this stuff.  I'm even happy to go ahead
and code it up, but it's important to note that this is work which
should be based on this set and not a blocker for this set to be
accepted.

> > .driver_data|.data. attributes.  We can generify the call if you think
> > that makes things easier, but I don't see a need for it ATM.
> >
> 
> As I explained above, it will make easier for drivers but I raised the
> point to discuss if the table data should be looked up by the driver
> or if the core should get it and pass to the probe() function as it is
> made right now for the I2C device ID table. i.e:
> 
> static int foo_i2c_probe(struct i2c_client *i2c, const void *data)
> 
> If the correct approach is the former, then this series is the right
> direction and as you said a generic match function can be added later.
> 
> But if the correct approach is the latter, then this series is not
> the right direction and a different approach is needed. I don't have
> a strong opinion but wanted to mention that we have two options here.

The correct approach is the former.  One of the aims of this set was
to bring the I2C .probe() call-back more into line with the majority
of the other .probe() calls in the kernel i.e. with only a single
parameter.  I'm really not a fan of passing some random void pointer
in.  Using a look-up call to fetch ACPI/OF/I2C/etc data is the current
norm and is a very viable option.

Wolfram, please (finally :D) take this set.

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

Re: [RESEND PATCH v4 0/8] i2c: Relax mandatory I2C ID table passing

[Javier Martinez Canillas]

Hello Lee,

On 09/24/2015 06:58 PM, Lee Jones wrote:

[snip]

>>
>>> Drivers will know if they either only supply an I2C or OF table, so
>>> they will know which call to use in order to obtain their
>>
>> Yes but that is not true for drivers that support both OF and legacy board
>> files. For those drivers, there will be a lot of boiler plate code duplicated
>> that would look something like:
>>
>>      unsigned long data;
>>      struct of_device_id *match;
>>      struct i2c_devicd_id *id;
>>
>>      if (i2c->dev.of_node) {
>>             match = i2c_of_match_device(of_match_table, i2c);
>> 	    if (!match)
>> 	           return -EINVAL;
>>
>>             data = (unsigned long)match->data;
>>      } else {
>>             id = i2c_match_id(id_table, i2c);
>> 	    if (!id)
>> 	           return -EINVAL;
>>
>>             data = id->driver_data;
>>      }
>>
>> While it would be nice to have something like:
>>
>>     data = i2c_get_data(i2c);
>>
>> and let the core handle which table should be looked up depending on
>> which mechanism was used to register the i2c device (legacy or OF).
> 
> I'm fine with a new API for this stuff.  I'm even happy to go ahead
> and code it up, but it's important to note that this is work which
> should be based on this set and not a blocker for this set to be
> accepted.
>

I didn't mean this should be a blocker and yes can be done as a follow up.
 
>>> .driver_data|.data. attributes.  We can generify the call if you think
>>> that makes things easier, but I don't see a need for it ATM.
>>>
>>
>> As I explained above, it will make easier for drivers but I raised the
>> point to discuss if the table data should be looked up by the driver
>> or if the core should get it and pass to the probe() function as it is
>> made right now for the I2C device ID table. i.e:
>>
>> static int foo_i2c_probe(struct i2c_client *i2c, const void *data)
>>
>> If the correct approach is the former, then this series is the right
>> direction and as you said a generic match function can be added later.
>>
>> But if the correct approach is the latter, then this series is not
>> the right direction and a different approach is needed. I don't have
>> a strong opinion but wanted to mention that we have two options here.
> 
> The correct approach is the former.  One of the aims of this set was
> to bring the I2C .probe() call-back more into line with the majority
> of the other .probe() calls in the kernel i.e. with only a single
> parameter.  I'm really not a fan of passing some random void pointer
> in.  Using a look-up call to fetch ACPI/OF/I2C/etc data is the current
> norm and is a very viable option.
>

Ok, as I said I don't have a strong opinion and you are right that this
set will make I2C to be more aligned with other subsystems (i.e: SPI that
the I2C implementation is very similar to).

> Wolfram, please (finally :D) take this set.
>

Indeed :)

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

Re: [RESEND PATCH v4 0/8] i2c: Relax mandatory I2C ID table passing

[Wolfram Sang]

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


> > Yes but that is not true for drivers that support both OF and legacy board
> > files. For those drivers, there will be a lot of boiler plate code duplicated
> > that would look something like:
> > 
> >      unsigned long data;
> >      struct of_device_id *match;
> >      struct i2c_devicd_id *id;
> > 
> >      if (i2c->dev.of_node) {
> >             match = i2c_of_match_device(of_match_table, i2c);
> > 	    if (!match)
> > 	           return -EINVAL;
> > 
> >             data = (unsigned long)match->data;
> >      } else {
> >             id = i2c_match_id(id_table, i2c);
> > 	    if (!id)
> > 	           return -EINVAL;
> > 
> >             data = id->driver_data;
> >      }

I said this before: It is not only the additional code, I think it is
quite unelegant to to do the matching again which has already been done.
(and DT boottime has already increased, partly due to the excessive
string matching). Also, I wouldn't like to see an I2C specific solution;
this problem exists for other subsystems, too.

> I'm fine with a new API for this stuff.  I'm even happy to go ahead
> and code it up, but it's important to note that this is work which
> should be based on this set and not a blocker for this set to be
> accepted.

Is that a promise? :)

> The correct approach is the former.  One of the aims of this set was
> to bring the I2C .probe() call-back more into line with the majority
> of the other .probe() calls in the kernel i.e. with only a single
> parameter.  I'm really not a fan of passing some random void pointer

Yes, I like this about this series.

> in.  Using a look-up call to fetch ACPI/OF/I2C/etc data is the current
> norm and is a very viable option.

It is the status quo, but that doesn't make it better IMO.

> Wolfram, please (finally :D) take this set.

I tend to give in ;) Maybe we can talk in Dublin a bit about a possible
next step after this series?

Thanks,

   Wolfram


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

Re: [RESEND PATCH v4 0/8] i2c: Relax mandatory I2C ID table passing

[Kieran Bingham]

Hi Wolfram,

On 1 October 2015 at 21:50, Wolfram Sang <wsa@the-dreams.de> wrote:
>
>> > Yes but that is not true for drivers that support both OF and legacy board
>> > files. For those drivers, there will be a lot of boiler plate code duplicated
>> > that would look something like:
>> >
>> >      unsigned long data;
>> >      struct of_device_id *match;
>> >      struct i2c_devicd_id *id;
>> >
>> >      if (i2c->dev.of_node) {
>> >             match = i2c_of_match_device(of_match_table, i2c);
>> >         if (!match)
>> >                return -EINVAL;
>> >
>> >             data = (unsigned long)match->data;
>> >      } else {
>> >             id = i2c_match_id(id_table, i2c);
>> >         if (!id)
>> >                return -EINVAL;
>> >
>> >             data = id->driver_data;
>> >      }
>
> I said this before: It is not only the additional code, I think it is
> quite unelegant to to do the matching again which has already been done.
> (and DT boottime has already increased, partly due to the excessive
> string matching). Also, I wouldn't like to see an I2C specific solution;
> this problem exists for other subsystems, too.
>
>> I'm fine with a new API for this stuff.  I'm even happy to go ahead
>> and code it up, but it's important to note that this is work which
>> should be based on this set and not a blocker for this set to be
>> accepted.
>
> Is that a promise? :)

Well if Lee doesn't, then I'll be trying to take it on.
I've already written an spatch to help with the conversion of other
drivers to follow on for this series.

Between us I think we've got motivation to make progress.

>> The correct approach is the former.  One of the aims of this set was
>> to bring the I2C .probe() call-back more into line with the majority
>> of the other .probe() calls in the kernel i.e. with only a single
>> parameter.  I'm really not a fan of passing some random void pointer
>
> Yes, I like this about this series.
>
>> in.  Using a look-up call to fetch ACPI/OF/I2C/etc data is the current
>> norm and is a very viable option.
>
> It is the status quo, but that doesn't make it better IMO.
>
>> Wolfram, please (finally :D) take this set.
>
> I tend to give in ;) Maybe we can talk in Dublin a bit about a possible
> next step after this series?

I don't think Lee is coming to Dublin, but I'll be there all week, if
you find time for a chat.
I'll look out for you in the hallway track, or at least on stage for
the final ELCE games :)

>
> Thanks,
>
>    Wolfram
>

--
Regards

Kieran

Re: [RESEND PATCH v4 0/8] i2c: Relax mandatory I2C ID table passing

[Lee Jones]

On Thu, 01 Oct 2015, Wolfram Sang wrote:

> 
> > > Yes but that is not true for drivers that support both OF and legacy board
> > > files. For those drivers, there will be a lot of boiler plate code duplicated
> > > that would look something like:
> > > 
> > >      unsigned long data;
> > >      struct of_device_id *match;
> > >      struct i2c_devicd_id *id;
> > > 
> > >      if (i2c->dev.of_node) {
> > >             match = i2c_of_match_device(of_match_table, i2c);
> > > 	    if (!match)
> > > 	           return -EINVAL;
> > > 
> > >             data = (unsigned long)match->data;
> > >      } else {
> > >             id = i2c_match_id(id_table, i2c);
> > > 	    if (!id)
> > > 	           return -EINVAL;
> > > 
> > >             data = id->driver_data;
> > >      }
> 
> I said this before: It is not only the additional code, I think it is
> quite unelegant to to do the matching again which has already been done.
> (and DT boottime has already increased, partly due to the excessive
> string matching). Also, I wouldn't like to see an I2C specific solution;
> this problem exists for other subsystems, too.
> 
> > I'm fine with a new API for this stuff.  I'm even happy to go ahead
> > and code it up, but it's important to note that this is work which
> > should be based on this set and not a blocker for this set to be
> > accepted.
> 
> Is that a promise? :)

Yes.

> > The correct approach is the former.  One of the aims of this set was
> > to bring the I2C .probe() call-back more into line with the majority
> > of the other .probe() calls in the kernel i.e. with only a single
> > parameter.  I'm really not a fan of passing some random void pointer
> 
> Yes, I like this about this series.
> 
> > in.  Using a look-up call to fetch ACPI/OF/I2C/etc data is the current
> > norm and is a very viable option.
> 
> It is the status quo, but that doesn't make it better IMO.
> 
> > Wolfram, please (finally :D) take this set.
> 
> I tend to give in ;)

Great.  Although, I don't see a "applied, thanks". :)

Please just take it, so we can breathe a sigh of relief and move on to
the next stage. :D

> Maybe we can talk in Dublin a bit about a possible
> next step after this series?

I'm happy to chat, although I'm afraid I won't be in Dublin this
time.  Kieran will be though, so feel free to so some lobby loitering
and I'll discuss with him when he returns.  Failing that we can
hook-up on Gtalk or IRC etc.

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

Re: [RESEND PATCH v4 0/8] i2c: Relax mandatory I2C ID table passing

[Lee Jones]

On Fri, 11 Sep 2015, Kieran Bingham wrote:
> Hi Wolfram,
> 
> I have picked this patchset [0] up from Lee to rebase it, with an aim to
> get this series moving again.
> 
> This resend fixes up my SoB's as highlighted by Lee
> 
> A couple of minor issues were resolved in the rebase. As it stood, Javier
> proposed [1] to merge this series, and use a follow up series to make sure
> that all I2C drivers are using a MODLE_DEVICE_TABLE(of,...)
> 
> I have prepared a Coccinelle patch to work through the bulk of the changes
> required for the conversion, which will assist the transition process.
> 
> Once this patch set is accepted, I will commence converting the other
> drivers, and submitting with a per subsystem breakdown or simliar to
> reduce traffic.
> 
> [0] https://lkml.org/lkml/2014/8/28/283
> [1] https://lkml.org/lkml/2014/9/12/496

I appreciate that my SoB is on every patch, but this set still looks
good to me, so for extra clarification:

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

[...]

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

Re: [RESEND PATCH v4 0/8] i2c: Relax mandatory I2C ID table passing

[Wolfram Sang]

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


As said to Kieran personally in Dublin, I want a verification that all
binding methods still work, especially runtime instantiation for drivers
without i2c_device_ids. Also, for the last patch, a verification should
be done if the drivers i2c_device_id hasn't been used meanwhile. I'd
also like to see 'probe_new' instead of 'probe2' for the new function
name. That should be it.

Thanks,

   Wolfram


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

Re: [RESEND PATCH v4 0/8] i2c: Relax mandatory I2C ID table passing

[Kieran Bingham]

Hi Wolfram,

On 9 October 2015 at 22:16, Wolfram Sang <wsa@the-dreams.de> wrote:
>
> As said to Kieran personally in Dublin, I want a verification that all
> binding methods still work, especially runtime instantiation for drivers
> without i2c_device_ids.

Ok, I should be able to find some time to look at that this week.

>  Also, for the last patch, a verification should
> be done if the drivers i2c_device_id hasn't been used meanwhile.

I'll see what I can do ...

> I'd also like to see 'probe_new' instead of 'probe2' for the new function
> name. That should be it.

Ok, obviously this is only a temporary naming so I don't mind either way,
I'll do a rename for the next version

I've also just found a compile failure to fix up on !CONFIG_OF, this
can make its way into the respin.

>
> Thanks,
>
>    Wolfram

Regards

Kieran

Re: [RESEND PATCH v4 0/8] i2c: Relax mandatory I2C ID table passing

[Lee Jones]

On Mon, 12 Oct 2015, Kieran Bingham wrote:

> Hi Wolfram,
> 
> On 9 October 2015 at 22:16, Wolfram Sang <wsa@the-dreams.de> wrote:
> >
> > As said to Kieran personally in Dublin, I want a verification that all
> > binding methods still work, especially runtime instantiation for drivers
> > without i2c_device_ids.
> 
> Ok, I should be able to find some time to look at that this week.
> 
> >  Also, for the last patch, a verification should
> > be done if the drivers i2c_device_id hasn't been used meanwhile.
> 
> I'll see what I can do ...
> 
> > I'd also like to see 'probe_new' instead of 'probe2' for the new function
> > name. That should be it.
> 
> Ok, obviously this is only a temporary naming so I don't mind either way,
> I'll do a rename for the next version
> 
> I've also just found a compile failure to fix up on !CONFIG_OF, this
> can make its way into the respin.

I still don't see this upstream.  What's the latest status?

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

Re: [RESEND PATCH v4 0/8] i2c: Relax mandatory I2C ID table passing

[Lee Jones]

On Tue, 08 Mar 2016, Kieran Bingham wrote:

> On 8 Mar 2016 11:22, "Lee Jones" <lee.jones@linaro.org> wrote:
> >
> > On Mon, 12 Oct 2015, Kieran Bingham wrote:
> >
> > > Hi Wolfram,
> > >
> > > On 9 October 2015 at 22:16, Wolfram Sang <wsa@the-dreams.de> wrote:
> > > >
> > > > As said to Kieran personally in Dublin, I want a verification that all
> > > > binding methods still work, especially runtime instantiation for
> drivers
> > > > without i2c_device_ids.
> > >
> > > Ok, I should be able to find some time to look at that this week.
> > >
> > > >  Also, for the last patch, a verification should
> > > > be done if the drivers i2c_device_id hasn't been used meanwhile.
> > >
> > > I'll see what I can do ...
> > >
> > > > I'd also like to see 'probe_new' instead of 'probe2' for the new
> function
> > > > name. That should be it.
> > >
> > > Ok, obviously this is only a temporary naming so I don't mind either
> way,
> > > I'll do a rename for the next version
> > >
> > > I've also just found a compile failure to fix up on !CONFIG_OF, this
> > > can make its way into the respin.
> >
> > I still don't see this upstream.  What's the latest status?
> 
> Needs correct testing:
> "verification that all binding methods still work, especially runtime
> instantiation for drivers without i2c_device_ids."
> 
> Actually I rebased this set last week.  I was going to try and see if I can
> test in qemu. Just need to work out how to load DT fragments at runtime.

Sounds like an over-the-top solution.  Can't you just modprobe some modules?

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