Re: [PATCH] i2c: Allow recovery of the initial IRQ by an I2C client device.

[Wolfram Sang <wsa@the-dreams.de>]

[-- Attachment #1: Type: text/plain, Size: 2314 bytes --]

On Fri, Feb 22, 2019 at 11:15:59AM +0100, Benjamin Tissoires wrote:
> On Fri, Feb 22, 2019 at 12:26 AM Wolfram Sang <wsa@the-dreams.de> wrote:
> >
> > On Tue, Feb 19, 2019 at 11:30:27AM -0800, Jim Broadus wrote:
> > > A previous change allowed I2C client devices to discover new IRQs upon
> > > reprobe by clearing the IRQ in i2c_device_remove. However, if an IRQ was
> > > assigned in i2c_new_device, that information is lost.
> > >
> > > For example, the touchscreen and trackpad devices on a Dell Inspiron laptop
> > > are I2C devices whose IRQs are defined by ACPI extended IRQ types. The
> > > client device structures are initialized during an ACPI walk. After
> > > removing the i2c_hid device, modprobe fails.
> > >
> > > This change caches the initial IRQ value in i2c_new_device and then resets
> > > the client device IRQ to the initial value in i2c_device_remove.
> > >
> > > Fixes: 6f108dd70d30 ("i2c: Clear client->irq in i2c_device_remove")
> > > Signed-off-by: Jim Broadus <jbroadus@gmail.com>
> >
> > Adding Benjamin to CC
> 
> Sorry, I should have answered earlier.
> 
> I am a little bit hesitant regarding this patch. The effect is
> correct, and I indeed realized a few weeks ago that something were
> wrong as we couldn't rmmod/modprobe i2c-hid.
> 
> But I still have the feeling that the problem is not solved at the
> right place. In i2c_new_device() we are storing parts of the fields of
> struct i2c_board_info, and when resetting the irq we are losing
> information. This patch solves that, but I wonder if the IRQ should
> not be 'simply' set in i2c_device_probe(). This means we also need to
> store the .resources of info, but I have a feeling this will be less
> error prone in the future.
> 
> But this is just my guts telling me something is not right. I would
> perfectly understand if we want to get this merged ASAP.
> 
> So given that the code is correct, this is my:
> Reviewed-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
> 
> But at least I have expressed my feelings :)

Which I can relate to very much. I see the code solves the issue but my
feeling is that we are patching around something which should be handled
differently in general.

Is somebody willing to research this further?

Thanks for your input.


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]