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 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).
>
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.
But if this is in line with what Benjamin and Wolfram had in mind, then the
code seems good to me.
> 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
