Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1394438 > unrolled thread

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

Back to article view | Back to linux.kernel

Contents

  [PATCHv5 0/8] 2c: Relax mandatory I2C ID table passing Kieran Bingham <kieran@ksquared.org.uk> - 2016-05-04 17:20 +0200
    [PATCHv5 6/8] i2c: Provide a temporary .probe_new() call-back type Kieran Bingham <kieran@ksquared.org.uk> - 2016-05-04 17:20 +0200
    [PATCHv5 1/8] i2c: Add pointer dereference protection to i2c_match_id() Kieran Bingham <kieran@ksquared.org.uk> - 2016-05-04 17:20 +0200
      Re: [PATCHv5 1/8] i2c: Add pointer dereference protection to  i2c_match_id() Javier Martinez Canillas <javier@osg.samsung.com> - 2016-05-10 07:00 +0200
    [PATCHv5 3/8] i2c: Match using traditional OF methods, then by vendor-less compatible strings Kieran Bingham <kieran@ksquared.org.uk> - 2016-05-04 17:20 +0200
    [PATCHv5 7/8] mfd: 88pm860x: Move over to new I2C device .probe() call Kieran Bingham <kieran@ksquared.org.uk> - 2016-05-04 17:20 +0200
      Re: [PATCHv5 7/8] mfd: 88pm860x: Move over to new I2C device .probe()  call Javier Martinez Canillas <javier@osg.samsung.com> - 2016-05-10 07:10 +0200
    [PATCHv5 8/8] mfd: as3722: Rid driver of superfluous I2C device ID structure Kieran Bingham <kieran@ksquared.org.uk> - 2016-05-04 17:20 +0200
      Re: [PATCHv5 8/8] mfd: as3722: Rid driver of superfluous I2C device  ID structure Javier Martinez Canillas <javier@osg.samsung.com> - 2016-05-10 07:30 +0200
        Re: [PATCHv5 8/8] mfd: as3722: Rid driver of superfluous I2C device  ID structure Lee Jones <lee.jones@linaro.org> - 2016-05-10 09:40 +0200
          Re: [PATCHv5 8/8] mfd: as3722: Rid driver of superfluous I2C device  ID structure Javier Martinez Canillas <javier@osg.samsung.com> - 2016-05-10 15:30 +0200
            Re: [PATCHv5 8/8] mfd: as3722: Rid driver of superfluous I2C device  ID structure Lee Jones <lee.jones@linaro.org> - 2016-05-10 16:10 +0200
            [PATCH] cocci: Find i2c drivers with an of_device table that isn't exported Kieran Bingham <kieran@ksquared.org.uk> - 2016-05-10 16:40 +0200
            [PATCH] cocci: Provide script to find i2c_tables missing exports Kieran Bingham <kieran@ksquared.org.uk> - 2016-05-10 17:10 +0200
              Re: [PATCH] cocci: Provide script to find i2c_tables missing exports Javier Martinez Canillas <javier@osg.samsung.com> - 2016-05-11 22:10 +0200
    Re: [PATCHv5 0/8] 2c: Relax mandatory I2C ID table passing Lee Jones <lee.jones@linaro.org> - 2016-05-09 11:20 +0200
      Re: [PATCHv5 0/8] 2c: Relax mandatory I2C ID table passing Javier Martinez Canillas <javier@osg.samsung.com> - 2016-05-09 15:30 +0200
    Re: [PATCHv5 0/8] 2c: Relax mandatory I2C ID table passing Javier Martinez Canillas <javier@osg.samsung.com> - 2016-05-10 07:40 +0200
      Re: [PATCHv5 0/8] 2c: Relax mandatory I2C ID table passing Kieran Bingham <kieran@ksquared.org.uk> - 2016-05-10 09:50 +0200

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

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

[toc] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

#1396875

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

[toc] | [prev] | [next] | [standalone]

#1397049

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

[toc] | [prev] | [next] | [standalone]

#1397705

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

[toc] | [prev] | [next] | [standalone]

#1397780

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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web