On Mon, Mar 11, 2019 at 10:19:19PM -0700, Jim Broadus wrote:
> Hi all. I've been trying to spend some cycles thinking about the IRQ acquisition
> refactor, following up on 93b6604c5a66 ("i2c: Allow recovery of the initial IRQ
> by an I2C client device."), and wanted to run some ideas by you. Apologies if
> there's already something in the works.
>
> My first thought was that we could add a get_irq function pointer to the client
> that, if non-null, would be called during probe to obtain the IRQ. Initially,
> the i2c_new_device function would probably need to determine which get_irq
> function to use. It would have logic similar to that existing in
> i2c_device_probe. This change would be moving in the wrong direction in that
> regard, but the idea would be that the this could eventually be specified in
> i2_board_info.
>
> A second option would be to provide get_info and release_info functions. This
> would be more flexible and allow more of the initialization to be moved out
> of i2c_new_device. It could also allow the client to see some changes
> without reprobing, if that's desirable.
>
> Finally, there is an option to copy more data from the board info structure
> in the i2c_new_device function. This is obviously the simplest and least
> invasive, but also duplicates data in many cases and allocates memory that
> isn't used in several others.
>
> I was initially leaning towards the first option, but as I'm writing this, I
> can see more benefits of the second. The third could certainly be a step
> in the direction one of the other two options since there are cases where
> we need to copy data to the client in all three strategies. What do you think?
>
Firstly apologies for the very slow reply hopefully been a busy
few weeks and hopefully I am now getting a bit more time to look
at this.
I am not sure really any of these greatly improve things over the
current init_irq solution, they just provide a different
mechanism of passing the IRQ. I had really been imagining something
like moving the loop from i2c_acpi_get_info into
i2c_device_probe, but the problem is that would really still
leave, how do we handle things which actually register using the
board_info? Really the issue there is we have the i2c_client
structure which seems to double as both holding the device time
and probe time information. And really I can't help but wonder is
it actually the stuff done at probe time that is the odd one out
as a lot is done at device time.
But looking at this again today it occurred to me are we just
over complicating things here. What about if just force the remap
of the IRQ in i2c_device_probe and only update client->irq if we
get a valid IRQ. Something like the following (note not really
tested yet).
diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c
index 38af18645133c..90d56ef7cfa90 100644
--- a/drivers/i2c/i2c-core-base.c
+++ b/drivers/i2c/i2c-core-base.c
@@ -322,7 +322,7 @@ static int i2c_device_probe(struct device *dev)
driver = to_i2c_driver(dev->driver);
- if (!client->irq && !driver->disable_i2c_core_irq_mapping) {
+ if (!driver->disable_i2c_core_irq_mapping) {
int irq = -ENOENT;
if (client->flags & I2C_CLIENT_HOST_NOTIFY) {
@@ -338,10 +338,8 @@ static int i2c_device_probe(struct device *dev)
if (irq == -EPROBE_DEFER)
return irq;
- if (irq < 0)
- irq = 0;
-
- client->irq = irq;
+ if (irq > 0)
+ client->irq = irq;
}
/*
@@ -430,8 +428,6 @@ static int i2c_device_remove(struct device *dev)
dev_pm_clear_wake_irq(&client->dev);
device_init_wakeup(&client->dev, false);
- client->irq = client->init_irq;
-
return status;
}
@@ -741,11 +737,10 @@ i2c_new_device(struct i2c_adapter *adap, struct i2c_board_info const *info)
client->flags = info->flags;
client->addr = info->addr;
- client->init_irq = info->irq;
- if (!client->init_irq)
- client->init_irq = i2c_dev_irq_from_resources(info->resources,
+ client->irq = info->irq;
+ if (!client->irq)
+ client->irq = i2c_dev_irq_from_resources(info->resources,
info->num_resources);
- client->irq = client->init_irq;
strlcpy(client->name, info->type, sizeof(client->name));
diff --git a/include/linux/i2c.h b/include/linux/i2c.h
index 383510b4f0832..1f45331924d6f 100644
--- a/include/linux/i2c.h
+++ b/include/linux/i2c.h
@@ -333,7 +333,6 @@ struct i2c_client {
char name[I2C_NAME_SIZE];
struct i2c_adapter *adapter; /* the adapter we sit on */
struct device dev; /* the device structure */
- int init_irq; /* irq set at initialization */
int irq; /* irq issued by device */
struct list_head detected;
#if IS_ENABLED(CONFIG_I2C_SLAVE)
--
2.11.0
