[PATCH] i2c: revert "i2c: core: Allow drivers to disable i2c-core irq mapping"

[PATCH] i2c: revert "i2c: core: Allow drivers to disable i2c-core irq mapping"

[Wolfram Sang]

This manually reverts commit d1d84bb95364ed604015c2b788caaf3dbca0262f.
The only user has gone two years ago with commit 589edb56b424 ("ACPI /
scan: Create platform device for INT33FE ACPI nodes") and no new user
has showed up. Remove and hope we will never need it again.

Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
---

Build tested only. Looking for Hans' opinion here.

 drivers/i2c/i2c-core-base.c | 6 +++---
 include/linux/i2c.h         | 3 ---
 2 files changed, 3 insertions(+), 6 deletions(-)

diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c
index 26f03a14a478..dc43242a85ba 100644
--- a/drivers/i2c/i2c-core-base.c
+++ b/drivers/i2c/i2c-core-base.c
@@ -319,11 +319,9 @@ static int i2c_device_probe(struct device *dev)
 	if (!client)
 		return 0;
 
-	driver = to_i2c_driver(dev->driver);
-
 	client->irq = client->init_irq;
 
-	if (!client->irq && !driver->disable_i2c_core_irq_mapping) {
+	if (!client->irq) {
 		int irq = -ENOENT;
 
 		if (client->flags & I2C_CLIENT_HOST_NOTIFY) {
@@ -349,6 +347,8 @@ static int i2c_device_probe(struct device *dev)
 		client->irq = irq;
 	}
 
+	driver = to_i2c_driver(dev->driver);
+
 	/*
 	 * An I2C ID table is not mandatory, if and only if, a suitable OF
 	 * or ACPI ID table is supplied for the probing device.
diff --git a/include/linux/i2c.h b/include/linux/i2c.h
index b8b8963f8bb9..098405df431f 100644
--- a/include/linux/i2c.h
+++ b/include/linux/i2c.h
@@ -231,7 +231,6 @@ enum i2c_alert_protocol {
  * @detect: Callback for device detection
  * @address_list: The I2C addresses to probe (for detect)
  * @clients: List of detected clients we created (for i2c-core use only)
- * @disable_i2c_core_irq_mapping: Tell the i2c-core to not do irq-mapping
  *
  * The driver.owner field should be set to the module owner of this driver.
  * The driver.name field should be set to the name of this driver.
@@ -290,8 +289,6 @@ struct i2c_driver {
 	int (*detect)(struct i2c_client *client, struct i2c_board_info *info);
 	const unsigned short *address_list;
 	struct list_head clients;
-
-	bool disable_i2c_core_irq_mapping;
 };
 #define to_i2c_driver(d) container_of(d, struct i2c_driver, driver)
 
-- 
2.27.0

Re: [PATCH] i2c: revert "i2c: core: Allow drivers to disable i2c-core irq mapping"

[Hans de Goede]

Hi,

On 6/30/20 6:24 PM, Wolfram Sang wrote:
> This manually reverts commit d1d84bb95364ed604015c2b788caaf3dbca0262f.
> The only user has gone two years ago with commit 589edb56b424 ("ACPI /
> scan: Create platform device for INT33FE ACPI nodes") and no new user
> has showed up. Remove and hope we will never need it again.
> 
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
> ---
> 
> Build tested only. Looking for Hans' opinion here.

I'm fine with removing this.

Semi off-topic:

Recently I did have another special case, see:

https://fedoraproject.org/wiki/Changes/RemoveDeviceMapperMultipathFromWorkstationLiveCD

During one of the iterations trying to deal with this
it would have been useful if the code instantiating
the client (rather then the driver for it) could have disabled
the i2c-core code for searching for an irq, without actually
specifying one. This would allow passing through the ACPI fwnode
as fwnode in the board_info, without triggering the ACPI IRQ
lookup code in the core.

So basically allow board_info to say:

"There is no IRQ and do not try to find one"

This could be as simple as having the instantiating code do:

	board_info.irq = -ENOENT;

Combined with the a tiny i2c-core change,
to deal with drivers checking for:

	if (i2c_client->irq)

Rather then for:

	if (i2c_client->irq > 0)

This tiny i2c-core change would look like this:

diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c
index 26f03a14a478..a7b05ef31f5f 100644
--- a/drivers/i2c/i2c-core-base.c
+++ b/drivers/i2c/i2c-core-base.c
@@ -343,11 +343,10 @@ static int i2c_device_probe(struct device *dev)
  			goto put_sync_adapter;
  		}

-		if (irq < 0)
-			irq = 0;
-
  		client->irq = irq;
  	}
+	if (client->irq < 0)
+		client->irq = 0;

  	/*
  	 * An I2C ID table is not mandatory, if and only if, a suitable OF

ATM I do not have a use-case for this, still I think this would be
useful to have. Would you be willing to take a patch with the above
change for this?

Regards,

Hans




> 
>   drivers/i2c/i2c-core-base.c | 6 +++---
>   include/linux/i2c.h         | 3 ---
>   2 files changed, 3 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c
> index 26f03a14a478..dc43242a85ba 100644
> --- a/drivers/i2c/i2c-core-base.c
> +++ b/drivers/i2c/i2c-core-base.c
> @@ -319,11 +319,9 @@ static int i2c_device_probe(struct device *dev)
>   	if (!client)
>   		return 0;
>   
> -	driver = to_i2c_driver(dev->driver);
> -
>   	client->irq = client->init_irq;
>   
> -	if (!client->irq && !driver->disable_i2c_core_irq_mapping) {
> +	if (!client->irq) {
>   		int irq = -ENOENT;
>   
>   		if (client->flags & I2C_CLIENT_HOST_NOTIFY) {
> @@ -349,6 +347,8 @@ static int i2c_device_probe(struct device *dev)
>   		client->irq = irq;
>   	}
>   
> +	driver = to_i2c_driver(dev->driver);
> +
>   	/*
>   	 * An I2C ID table is not mandatory, if and only if, a suitable OF
>   	 * or ACPI ID table is supplied for the probing device.
> diff --git a/include/linux/i2c.h b/include/linux/i2c.h
> index b8b8963f8bb9..098405df431f 100644
> --- a/include/linux/i2c.h
> +++ b/include/linux/i2c.h
> @@ -231,7 +231,6 @@ enum i2c_alert_protocol {
>    * @detect: Callback for device detection
>    * @address_list: The I2C addresses to probe (for detect)
>    * @clients: List of detected clients we created (for i2c-core use only)
> - * @disable_i2c_core_irq_mapping: Tell the i2c-core to not do irq-mapping
>    *
>    * The driver.owner field should be set to the module owner of this driver.
>    * The driver.name field should be set to the name of this driver.
> @@ -290,8 +289,6 @@ struct i2c_driver {
>   	int (*detect)(struct i2c_client *client, struct i2c_board_info *info);
>   	const unsigned short *address_list;
>   	struct list_head clients;
> -
> -	bool disable_i2c_core_irq_mapping;
>   };
>   #define to_i2c_driver(d) container_of(d, struct i2c_driver, driver)
>   
> 

Re: [PATCH] i2c: revert "i2c: core: Allow drivers to disable i2c-core irq mapping"

[Wolfram Sang]

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

On Tue, Jun 30, 2020 at 06:24:40PM +0200, Wolfram Sang wrote:
> This manually reverts commit d1d84bb95364ed604015c2b788caaf3dbca0262f.
> The only user has gone two years ago with commit 589edb56b424 ("ACPI /
> scan: Create platform device for INT33FE ACPI nodes") and no new user
> has showed up. Remove and hope we will never need it again.
> 
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>

Applied to for-next, thanks!


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

Re: [PATCH] i2c: revert "i2c: core: Allow drivers to disable i2c-core irq mapping"

[Wolfram Sang]

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

Hi Hans,

> > Build tested only. Looking for Hans' opinion here.
> 
> I'm fine with removing this.

Thanks!

[semi off-topic case]

> So basically allow board_info to say:
> 
> "There is no IRQ and do not try to find one"
> 
> This could be as simple as having the instantiating code do:
> 
> 	board_info.irq = -ENOENT;

...

> ATM I do not have a use-case for this, still I think this would be
> useful to have. Would you be willing to take a patch with the above
> change for this?

I haven't checked your code change in detail to check for side-effects.
In general, I think we could have something like this. However, I am a
bit conservative when it comes to changing something without a use case.

All the best,

   Wolfram


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