On Tue, Mar 19, 2019 at 10:56 AM Charles Keepax
<ckeepax@xxxxxxxxxxxxxxxxxxxxx> wrote:
>
> On Mon, Mar 18, 2019 at 10:40:40PM -0700, Jim Broadus wrote:
> > On Mon, Mar 18, 2019 at 10:08 AM Charles Keepax
> > <ckeepax@xxxxxxxxxxxxxxxxxxxxx> wrote:
> > > On Mon, Mar 11, 2019 at 10:19:19PM -0700, Jim Broadus wrote:
> > > 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).
> > >
> >
> > Hi Charles. Thanks for the response. This patch is definitely more elegant than
> > what we have now, though behavior may change in some corner cases, for better or
> > worse. But it still doesn't solve the concerns, as I understood them, that there
> > are multiple ways that an IRQ can be determined in multiple places. This is
> > isn't entirely avoidable since there will always be cases where the IRQ is known
> > ahead of time, but I was looking at common interfaces that could be used in the
> > probe.
> >
>
> Certainly that was my initial reaction as well, it felt nasty
> to have some IRQs obtained at device time and some at probe. I
> am not sure it is really avoidable though (at least not without
> major refactoring of large parts of the kernel out side I2C).
> Which leaves us in a situation where really the only thing that
> can be done is store something from device time and handle
> that at probe time. Each of your solutions basically handle
> that store differently, but they are all doing it.
>
> Of the options I feel like storing the whole info would have
> some milage if we expected to want to look back at other parts
> of it in the future. But for now I would argue that we only
> need the IRQ and thus the simplest solution is to just store
> the IRQ as the code did before I broke it :-)
>
> > But if this is in line with what Benjamin and Wolfram had in mind, then the
> > code seems good to me.
> >
>
> Yeah lets see if they have any thoughts.
>
I certainly prefer a simpler solution :)
But there is still the corner case that we might overwrite the irq we
got from i2c_dev_irq_from_resources() or from info->irq.
Ideally, I would prefer having the retrieval of info->irq and
i2c_dev_irq_from_resources() being called in .probe(), but if this is
too much to store, we could just keep it in init_irq, and make the
initial assignment in probe only.
This would likely give (based on upstream):
---
diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c
index af87a16ac3a5..6c0a3001490f 100644
--- a/drivers/i2c/i2c-core-base.c
+++ b/drivers/i2c/i2c-core-base.c
@@ -322,6 +322,7 @@ static int i2c_device_probe(struct device *dev)
driver = to_i2c_driver(dev->driver);
+ client->irq = client->init_irq;
if (!client->irq && !driver->disable_i2c_core_irq_mapping) {
int irq = -ENOENT;
@@ -430,7 +431,7 @@ 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;
+ client->irq = 0;
return status;
}
@@ -745,7 +746,6 @@ i2c_new_device(struct i2c_adapter *adap, struct
i2c_board_info const *info)
if (!client->init_irq)
client->init_irq = i2c_dev_irq_from_resources(info->resources,
info->num_resources);
- client->irq = client->init_irq;
strlcpy(client->name, info->type, sizeof(client->name));
---
(with tabs being replaced by spaces because of my MUA I wouldn't say
the name here).
There is not much change in the code, but this way:
- init_irq is set during i2c_device_new() only
- client->irq is only filled during .probe()
Cheers,
Benjamin
