| Message ID | 20190311110555.30653-1-hdegoede@redhat.com |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | i2c: Do _not_ clear client->irq on remove if the irq comes from the board_info | expand |
Hi Hans, On Mon, Mar 11, 2019 at 12:06 PM Hans de Goede <hdegoede@redhat.com> wrote: > > Commit 6f108dd70d30 ("i2c: Clear client->irq in i2c_device_remove"), makes > i2c_device_remove set client->irq=0, so that irqs assigned by > i2c_device_probe get cleared (and re-assigned) when rebinding the driver. > > This breaks driver-rebinding for i2c_client-s where the irq comes from the > board_info, since the irq from the board_info is now lost after the > i2c_device_remove call. > > This commit fixes this by not clearing the irq on remove if the irq > originally came from the board_info. > > Fixes: 6f108dd70d30 ("i2c: Clear client->irq in i2c_device_remove") > Cc: Charles Keepax <ckeepax@opensource.cirrus.com> > Cc: Benjamin Tissoires <benjamin.tissoires@redhat.com> > Signed-off-by: Hans de Goede <hdegoede@redhat.com> Commit https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=93b6604c5a669d84e45fe5129294875bf82eb1ff in Linus' tree already fixes this. Neither Wolfram nor myself were quite happy with the patch. Your approach seems slightly better. It still doesn't fix the root of the issue: setting up the irq while in .probe() You can find the whole discussion at https://patchwork.ozlabs.org/patch/1044865/ Cheers, Benjamin > --- > drivers/i2c/i2c-core-base.c | 5 ++++- > include/linux/i2c.h | 1 + > 2 files changed, 5 insertions(+), 1 deletion(-) > > diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c > index 28460f6a60cc..c3d94ffdff29 100644 > --- a/drivers/i2c/i2c-core-base.c > +++ b/drivers/i2c/i2c-core-base.c > @@ -430,7 +430,8 @@ static int i2c_device_remove(struct device *dev) > dev_pm_clear_wake_irq(&client->dev); > device_init_wakeup(&client->dev, false); > > - client->irq = 0; > + if (!(client->flags & I2C_CLIENT_BINFO_IRQ)) > + client->irq = 0; > > return status; > } > @@ -745,6 +746,8 @@ i2c_new_device(struct i2c_adapter *adap, struct i2c_board_info const *info) > if (!client->irq) > client->irq = i2c_dev_irq_from_resources(info->resources, > info->num_resources); > + if (client->irq) > + client->flags |= I2C_CLIENT_BINFO_IRQ; > > strlcpy(client->name, info->type, sizeof(client->name)); > > diff --git a/include/linux/i2c.h b/include/linux/i2c.h > index 65b4eaed1d96..1ff3c78a6c1b 100644 > --- a/include/linux/i2c.h > +++ b/include/linux/i2c.h > @@ -769,6 +769,7 @@ i2c_unlock_bus(struct i2c_adapter *adapter, unsigned int flags) > #define I2C_CLIENT_SLAVE 0x20 /* we are the slave */ > #define I2C_CLIENT_HOST_NOTIFY 0x40 /* We want to use I2C host notify */ > #define I2C_CLIENT_WAKE 0x80 /* for board_info; true iff can wake */ > +#define I2C_CLIENT_BINFO_IRQ 0x100 /* irq comes from the board_info */ > #define I2C_CLIENT_SCCB 0x9000 /* Use Omnivision SCCB protocol */ > /* Must match I2C_M_STOP|IGNORE_NAK */ > > -- > 2.20.1 >
Hi, On 11-03-19 14:27, Benjamin Tissoires wrote: > Hi Hans, > > On Mon, Mar 11, 2019 at 12:06 PM Hans de Goede <hdegoede@redhat.com> wrote: >> >> Commit 6f108dd70d30 ("i2c: Clear client->irq in i2c_device_remove"), makes >> i2c_device_remove set client->irq=0, so that irqs assigned by >> i2c_device_probe get cleared (and re-assigned) when rebinding the driver. >> >> This breaks driver-rebinding for i2c_client-s where the irq comes from the >> board_info, since the irq from the board_info is now lost after the >> i2c_device_remove call. >> >> This commit fixes this by not clearing the irq on remove if the irq >> originally came from the board_info. >> >> Fixes: 6f108dd70d30 ("i2c: Clear client->irq in i2c_device_remove") >> Cc: Charles Keepax <ckeepax@opensource.cirrus.com> >> Cc: Benjamin Tissoires <benjamin.tissoires@redhat.com> >> Signed-off-by: Hans de Goede <hdegoede@redhat.com> > > Commit https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=93b6604c5a669d84e45fe5129294875bf82eb1ff > in Linus' tree already fixes this. > Neither Wolfram nor myself were quite happy with the patch. Your > approach seems slightly better. It still doesn't fix the root of the > issue: setting up the irq while in .probe() > > You can find the whole discussion at https://patchwork.ozlabs.org/patch/1044865/ I see, my personal tree is based on 5.0, (I rebase it after rc1) so I missed this already being fixed. Thank you for the pointer. Regards, Hans >> --- >> drivers/i2c/i2c-core-base.c | 5 ++++- >> include/linux/i2c.h | 1 + >> 2 files changed, 5 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c >> index 28460f6a60cc..c3d94ffdff29 100644 >> --- a/drivers/i2c/i2c-core-base.c >> +++ b/drivers/i2c/i2c-core-base.c >> @@ -430,7 +430,8 @@ static int i2c_device_remove(struct device *dev) >> dev_pm_clear_wake_irq(&client->dev); >> device_init_wakeup(&client->dev, false); >> >> - client->irq = 0; >> + if (!(client->flags & I2C_CLIENT_BINFO_IRQ)) >> + client->irq = 0; >> >> return status; >> } >> @@ -745,6 +746,8 @@ i2c_new_device(struct i2c_adapter *adap, struct i2c_board_info const *info) >> if (!client->irq) >> client->irq = i2c_dev_irq_from_resources(info->resources, >> info->num_resources); >> + if (client->irq) >> + client->flags |= I2C_CLIENT_BINFO_IRQ; >> >> strlcpy(client->name, info->type, sizeof(client->name)); >> >> diff --git a/include/linux/i2c.h b/include/linux/i2c.h >> index 65b4eaed1d96..1ff3c78a6c1b 100644 >> --- a/include/linux/i2c.h >> +++ b/include/linux/i2c.h >> @@ -769,6 +769,7 @@ i2c_unlock_bus(struct i2c_adapter *adapter, unsigned int flags) >> #define I2C_CLIENT_SLAVE 0x20 /* we are the slave */ >> #define I2C_CLIENT_HOST_NOTIFY 0x40 /* We want to use I2C host notify */ >> #define I2C_CLIENT_WAKE 0x80 /* for board_info; true iff can wake */ >> +#define I2C_CLIENT_BINFO_IRQ 0x100 /* irq comes from the board_info */ >> #define I2C_CLIENT_SCCB 0x9000 /* Use Omnivision SCCB protocol */ >> /* Must match I2C_M_STOP|IGNORE_NAK */ >> >> -- >> 2.20.1 >>
> Commit https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=93b6604c5a669d84e45fe5129294875bf82eb1ff > in Linus' tree already fixes this. > Neither Wolfram nor myself were quite happy with the patch. Your > approach seems slightly better. It still doesn't fix the root of the > issue: setting up the irq while in .probe() I actually like the current workaround a tad better - no new flag we later need to remove. But yeah, it is still a workaround. Let's cheer for Charles who was interested in having another look at this.
diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c index 28460f6a60cc..c3d94ffdff29 100644 --- a/drivers/i2c/i2c-core-base.c +++ b/drivers/i2c/i2c-core-base.c @@ -430,7 +430,8 @@ static int i2c_device_remove(struct device *dev) dev_pm_clear_wake_irq(&client->dev); device_init_wakeup(&client->dev, false); - client->irq = 0; + if (!(client->flags & I2C_CLIENT_BINFO_IRQ)) + client->irq = 0; return status; } @@ -745,6 +746,8 @@ i2c_new_device(struct i2c_adapter *adap, struct i2c_board_info const *info) if (!client->irq) client->irq = i2c_dev_irq_from_resources(info->resources, info->num_resources); + if (client->irq) + client->flags |= I2C_CLIENT_BINFO_IRQ; strlcpy(client->name, info->type, sizeof(client->name)); diff --git a/include/linux/i2c.h b/include/linux/i2c.h index 65b4eaed1d96..1ff3c78a6c1b 100644 --- a/include/linux/i2c.h +++ b/include/linux/i2c.h @@ -769,6 +769,7 @@ i2c_unlock_bus(struct i2c_adapter *adapter, unsigned int flags) #define I2C_CLIENT_SLAVE 0x20 /* we are the slave */ #define I2C_CLIENT_HOST_NOTIFY 0x40 /* We want to use I2C host notify */ #define I2C_CLIENT_WAKE 0x80 /* for board_info; true iff can wake */ +#define I2C_CLIENT_BINFO_IRQ 0x100 /* irq comes from the board_info */ #define I2C_CLIENT_SCCB 0x9000 /* Use Omnivision SCCB protocol */ /* Must match I2C_M_STOP|IGNORE_NAK */
Commit 6f108dd70d30 ("i2c: Clear client->irq in i2c_device_remove"), makes i2c_device_remove set client->irq=0, so that irqs assigned by i2c_device_probe get cleared (and re-assigned) when rebinding the driver. This breaks driver-rebinding for i2c_client-s where the irq comes from the board_info, since the irq from the board_info is now lost after the i2c_device_remove call. This commit fixes this by not clearing the irq on remove if the irq originally came from the board_info. Fixes: 6f108dd70d30 ("i2c: Clear client->irq in i2c_device_remove") Cc: Charles Keepax <ckeepax@opensource.cirrus.com> Cc: Benjamin Tissoires <benjamin.tissoires@redhat.com> Signed-off-by: Hans de Goede <hdegoede@redhat.com> --- drivers/i2c/i2c-core-base.c | 5 ++++- include/linux/i2c.h | 1 + 2 files changed, 5 insertions(+), 1 deletion(-)