On Tue, Mar 19, 2019 at 11:42:48AM +0100, Benjamin Tissoires wrote:
> 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:
> > > 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.
>
My solution does somewhat change the priority on IRQ sources,
which might require some thought although I would hope that cases
where we have two sources specifying different IRQs and only one
of those works are rare.
> 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.
>
Which would really lead us down the copying the whole info
structure I guess. It does use more memory although I guess the
impact is fairly minimal most systems don't have hundreds of I2C
devices. My concern is really more on the its basically
functionally the same as just saving the IRQ but significantly
more complex.
> 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()
>
This also looks reasonable as a solution to me.
Thanks,
Charles
