| Message ID | 20180701122343.27194-1-hdegoede@redhat.com |
|---|---|
| Headers | show |
| Series | i2c: ACPI: Add multi-instantiate pseudo driver | expand |
Hi All, On 01-07-18 14:23, Hans de Goede wrote: > Hi All, > > Here is more or less a resend of v1 of my second attempt at dealing with > multiple i2c devices being described in a single ACPI fwnode. > > The only change is that it has been rebased from v4.17 to v4.18-rc2. > > Mika, Andy and Heikki, can you review this please? Ping? It would be good to get some sort of resolution for this problem / this patch-set. As mentioned in an earlier reply in this thread, the only alternative to this patch-set which I see would be to duplicate the list of ACPI HIDs found in the new drivers/i2c/i2c-multi-instantiate.c inside code under drivers/acpi and make the acpi code not skip enumerating devices with these HIDs as platform drivers. Then the i2c-multi-instantiate.c driver could become a platform driver (*) and the I2C_CLIENT_IGNORE_BUSY flag / hack can go away (at the price of duplicating the HIDs in 2 places). I'm certainly open to taking a shot at doing things that way, but getting some feedback first and an indication from the ACPI folks (Mika?) that that would be acceptable would be nice. Regards, Hans *) And probably be moved to drivers/platform/x86
On Tue, 2018-07-31 at 13:02 +0200, Hans de Goede wrote: > Hi All, > > On 01-07-18 14:23, Hans de Goede wrote: > > Hi All, > > > > Here is more or less a resend of v1 of my second attempt at dealing > > with > > multiple i2c devices being described in a single ACPI fwnode. > > > > The only change is that it has been rebased from v4.17 to v4.18-rc2. > > > > Mika, Andy and Heikki, can you review this please? > > Ping? It would be good to get some sort of resolution for > this problem / this patch-set. My personal opinion is that the imaginary devices which are not part of real hardware in this design is not good. So, to get resolution I would rather to wait for some comments from Mika, Rafael, Wolfram, and maybe others. Esp. for Wolfram's one since it's about how it would be represented in I2C bus. > As mentioned in an earlier reply in this thread, the only alternative > to this patch-set which I see would be to duplicate the list of > ACPI HIDs found in the new drivers/i2c/i2c-multi-instantiate.c > inside code under drivers/acpi and make the acpi code not > skip enumerating devices with these HIDs as platform drivers. > > Then the i2c-multi-instantiate.c driver could become a > platform driver (*) and the I2C_CLIENT_IGNORE_BUSY > flag / hack can go away (at the price of duplicating > the HIDs in 2 places). > > I'm certainly open to taking a shot at doing things that > way, but getting some feedback first and an indication > from the ACPI folks (Mika?) that that would be acceptable > would be nice. > > Regards, > > Hans > > > *) And probably be moved to drivers/platform/x86
> As mentioned in an earlier reply in this thread, the only alternative > to this patch-set which I see would be to duplicate the list of > ACPI HIDs found in the new drivers/i2c/i2c-multi-instantiate.c > inside code under drivers/acpi and make the acpi code not > skip enumerating devices with these HIDs as platform drivers. > > Then the i2c-multi-instantiate.c driver could become a > platform driver (*) and the I2C_CLIENT_IGNORE_BUSY > flag / hack can go away (at the price of duplicating > the HIDs in 2 places). What is the downside of duplicating the HID? My gut feeling is that it can't be worse than having two devices mapped to the same I2C address and work around that by I2C_CLIENT_IGNORE_BUSY. But I may be biased :) I mean this is broken firmware, and having that fixed in ACPI and platform/x86 code instead of I2C core sounds way more proper to me.
Hi, On 31-07-18 21:30, Wolfram Sang wrote: > >> As mentioned in an earlier reply in this thread, the only alternative >> to this patch-set which I see would be to duplicate the list of >> ACPI HIDs found in the new drivers/i2c/i2c-multi-instantiate.c >> inside code under drivers/acpi and make the acpi code not >> skip enumerating devices with these HIDs as platform drivers. >> >> Then the i2c-multi-instantiate.c driver could become a >> platform driver (*) and the I2C_CLIENT_IGNORE_BUSY >> flag / hack can go away (at the price of duplicating >> the HIDs in 2 places). > > What is the downside of duplicating the HID? The downside is that as new HIDs show up they need to be added in 2 places. The drivers/hid code has something similar where devices needing custom drivers needed to have their VID:PID added to a blacklist for the generic driver and in my experience I always forget about this and the first time I test a new custom hid driver it fails because of this. IOW its not ideal but it is not really a problem either. > My gut feeling is that it can't be worse than having two devices mapped > to the same I2C address and work around that by I2C_CLIENT_IGNORE_BUSY. > But I may be biased :) > > I mean this is broken firmware, and having that fixed in ACPI and > platform/x86 code instead of I2C core sounds way more proper to me. Andy, Heikki, what is your take on this approach: 1) Have a list of ACPI IDs for which to not set the acpi_device_enumeration_by_parent flag in drivers/acpi/scan.c so that they get enumerated as a platform device. 2) Turn the i2c-multi-instantiate.c driver from the second patch of this series into a platform driver which will then instantiate *all* the i2c-clients for the ACPI fwnode as before. This will get rid of the ugly duplicate i2c_client for the first listed i2c-resouce in the ACPI fwnode and instantiating a platform device for ACPI fwnode-s is normal, so I think that this is the cleaner way. If you agree then I will rework the patch-set to do this when I've some time for this. Regards, Hans
On Wed, Aug 1, 2018 at 9:56 AM, Hans de Goede <hdegoede@redhat.com> wrote: > Hi, > > On 31-07-18 21:30, Wolfram Sang wrote: >> >> >>> As mentioned in an earlier reply in this thread, the only alternative >>> to this patch-set which I see would be to duplicate the list of >>> ACPI HIDs found in the new drivers/i2c/i2c-multi-instantiate.c >>> inside code under drivers/acpi and make the acpi code not >>> skip enumerating devices with these HIDs as platform drivers. >>> >>> Then the i2c-multi-instantiate.c driver could become a >>> platform driver (*) and the I2C_CLIENT_IGNORE_BUSY >>> flag / hack can go away (at the price of duplicating >>> the HIDs in 2 places). >> >> >> What is the downside of duplicating the HID? > > > The downside is that as new HIDs show up they need to be added in > 2 places. The drivers/hid code has something similar where devices > needing custom drivers needed to have their VID:PID added to a > blacklist for the generic driver and in my experience I always > forget about this and the first time I test a new custom hid > driver it fails because of this. > > IOW its not ideal but it is not really a problem either. > >> My gut feeling is that it can't be worse than having two devices mapped >> to the same I2C address and work around that by I2C_CLIENT_IGNORE_BUSY. >> But I may be biased :) >> >> I mean this is broken firmware, and having that fixed in ACPI and >> platform/x86 code instead of I2C core sounds way more proper to me. > > > Andy, Heikki, what is your take on this approach: > > 1) Have a list of ACPI IDs for which to not set the > acpi_device_enumeration_by_parent flag in drivers/acpi/scan.c > so that they get enumerated as a platform device. > > 2) Turn the i2c-multi-instantiate.c driver from the second patch > of this series into a platform driver which will then > instantiate *all* the i2c-clients for the ACPI fwnode as > before. What would that bind to? Anyway, this sounds like a reasonable approach to me, FWIW.
Hi, On 01-08-18 10:27, Rafael J. Wysocki wrote: > On Wed, Aug 1, 2018 at 9:56 AM, Hans de Goede <hdegoede@redhat.com> wrote: >> Hi, >> >> On 31-07-18 21:30, Wolfram Sang wrote: >>> >>> >>>> As mentioned in an earlier reply in this thread, the only alternative >>>> to this patch-set which I see would be to duplicate the list of >>>> ACPI HIDs found in the new drivers/i2c/i2c-multi-instantiate.c >>>> inside code under drivers/acpi and make the acpi code not >>>> skip enumerating devices with these HIDs as platform drivers. >>>> >>>> Then the i2c-multi-instantiate.c driver could become a >>>> platform driver (*) and the I2C_CLIENT_IGNORE_BUSY >>>> flag / hack can go away (at the price of duplicating >>>> the HIDs in 2 places). >>> >>> >>> What is the downside of duplicating the HID? >> >> >> The downside is that as new HIDs show up they need to be added in >> 2 places. The drivers/hid code has something similar where devices >> needing custom drivers needed to have their VID:PID added to a >> blacklist for the generic driver and in my experience I always >> forget about this and the first time I test a new custom hid >> driver it fails because of this. >> >> IOW its not ideal but it is not really a problem either. >> >>> My gut feeling is that it can't be worse than having two devices mapped >>> to the same I2C address and work around that by I2C_CLIENT_IGNORE_BUSY. >>> But I may be biased :) >>> >>> I mean this is broken firmware, and having that fixed in ACPI and >>> platform/x86 code instead of I2C core sounds way more proper to me. >> >> >> Andy, Heikki, what is your take on this approach: >> >> 1) Have a list of ACPI IDs for which to not set the >> acpi_device_enumeration_by_parent flag in drivers/acpi/scan.c >> so that they get enumerated as a platform device. >> >> 2) Turn the i2c-multi-instantiate.c driver from the second patch >> of this series into a platform driver which will then >> instantiate *all* the i2c-clients for the ACPI fwnode as >> before. > > What would that bind to? To the platform device which will be instantiated for the ACPI fwnode because of us not setting the acpi_device_enumeration_by_parent flag (*) for fwnode's with a HID for which we know we need the i2c-multi-instantiate.c driver, which is why we will then have a list of the HIDs in 2 places. > Anyway, this sounds like a reasonable approach to me, FWIW. That is good to hear. Regards, Hans *) As mentioned as 1. above
On Wed, Aug 01, 2018 at 09:56:53AM +0200, Hans de Goede wrote: > Hi, > > On 31-07-18 21:30, Wolfram Sang wrote: > > > > > As mentioned in an earlier reply in this thread, the only alternative > > > to this patch-set which I see would be to duplicate the list of > > > ACPI HIDs found in the new drivers/i2c/i2c-multi-instantiate.c > > > inside code under drivers/acpi and make the acpi code not > > > skip enumerating devices with these HIDs as platform drivers. > > > > > > Then the i2c-multi-instantiate.c driver could become a > > > platform driver (*) and the I2C_CLIENT_IGNORE_BUSY > > > flag / hack can go away (at the price of duplicating > > > the HIDs in 2 places). > > > > What is the downside of duplicating the HID? > > The downside is that as new HIDs show up they need to be added in > 2 places. The drivers/hid code has something similar where devices > needing custom drivers needed to have their VID:PID added to a > blacklist for the generic driver and in my experience I always > forget about this and the first time I test a new custom hid > driver it fails because of this. > > IOW its not ideal but it is not really a problem either. > > > My gut feeling is that it can't be worse than having two devices mapped > > to the same I2C address and work around that by I2C_CLIENT_IGNORE_BUSY. > > But I may be biased :) > > > > I mean this is broken firmware, and having that fixed in ACPI and > > platform/x86 code instead of I2C core sounds way more proper to me. > > Andy, Heikki, what is your take on this approach: > > 1) Have a list of ACPI IDs for which to not set the > acpi_device_enumeration_by_parent flag in drivers/acpi/scan.c > so that they get enumerated as a platform device. > > 2) Turn the i2c-multi-instantiate.c driver from the second patch > of this series into a platform driver which will then > instantiate *all* the i2c-clients for the ACPI fwnode as > before. > > This will get rid of the ugly duplicate i2c_client for the first > listed i2c-resouce in the ACPI fwnode and instantiating a > platform device for ACPI fwnode-s is normal, so I think that this > is the cleaner way. > > If you agree then I will rework the patch-set to do this when > I've some time for this. Looks reasonable to me. Thanks,
On Wed, Aug 1, 2018 at 10:37 AM, Hans de Goede <hdegoede@redhat.com> wrote: > Hi, > > > On 01-08-18 10:27, Rafael J. Wysocki wrote: >> >> On Wed, Aug 1, 2018 at 9:56 AM, Hans de Goede <hdegoede@redhat.com> wrote: >>> >>> Hi, >>> >>> On 31-07-18 21:30, Wolfram Sang wrote: >>>> >>>> >>>> >>>>> As mentioned in an earlier reply in this thread, the only alternative >>>>> to this patch-set which I see would be to duplicate the list of >>>>> ACPI HIDs found in the new drivers/i2c/i2c-multi-instantiate.c >>>>> inside code under drivers/acpi and make the acpi code not >>>>> skip enumerating devices with these HIDs as platform drivers. >>>>> >>>>> Then the i2c-multi-instantiate.c driver could become a >>>>> platform driver (*) and the I2C_CLIENT_IGNORE_BUSY >>>>> flag / hack can go away (at the price of duplicating >>>>> the HIDs in 2 places). >>>> >>>> >>>> >>>> What is the downside of duplicating the HID? >>> >>> >>> >>> The downside is that as new HIDs show up they need to be added in >>> 2 places. The drivers/hid code has something similar where devices >>> needing custom drivers needed to have their VID:PID added to a >>> blacklist for the generic driver and in my experience I always >>> forget about this and the first time I test a new custom hid >>> driver it fails because of this. >>> >>> IOW its not ideal but it is not really a problem either. >>> >>>> My gut feeling is that it can't be worse than having two devices mapped >>>> to the same I2C address and work around that by I2C_CLIENT_IGNORE_BUSY. >>>> But I may be biased :) >>>> >>>> I mean this is broken firmware, and having that fixed in ACPI and >>>> platform/x86 code instead of I2C core sounds way more proper to me. >>> >>> >>> >>> Andy, Heikki, what is your take on this approach: >>> >>> 1) Have a list of ACPI IDs for which to not set the >>> acpi_device_enumeration_by_parent flag in drivers/acpi/scan.c >>> so that they get enumerated as a platform device. >>> >>> 2) Turn the i2c-multi-instantiate.c driver from the second patch >>> of this series into a platform driver which will then >>> instantiate *all* the i2c-clients for the ACPI fwnode as >>> before. >> >> >> What would that bind to? > > To the platform device which will be instantiated for the > ACPI fwnode because of us not setting the acpi_device_enumeration_by_parent > flag (*) for fwnode's with a HID for which we know we need the > i2c-multi-instantiate.c driver, which is why we will then have > a list of the HIDs in 2 places. OK
On Wed, 2018-08-01 at 09:56 +0200, Hans de Goede wrote: > Hi, > > On 31-07-18 21:30, Wolfram Sang wrote: > > > > > As mentioned in an earlier reply in this thread, the only > > > alternative > > > to this patch-set which I see would be to duplicate the list of > > > ACPI HIDs found in the new drivers/i2c/i2c-multi-instantiate.c > > > inside code under drivers/acpi and make the acpi code not > > > skip enumerating devices with these HIDs as platform drivers. > > > > > > Then the i2c-multi-instantiate.c driver could become a > > > platform driver (*) and the I2C_CLIENT_IGNORE_BUSY > > > flag / hack can go away (at the price of duplicating > > > the HIDs in 2 places). > > > > What is the downside of duplicating the HID? > > The downside is that as new HIDs show up they need to be added in > 2 places. The drivers/hid code has something similar where devices > needing custom drivers needed to have their VID:PID added to a > blacklist for the generic driver and in my experience I always > forget about this and the first time I test a new custom hid > driver it fails because of this. > > IOW its not ideal but it is not really a problem either. > > > My gut feeling is that it can't be worse than having two devices > > mapped > > to the same I2C address and work around that by > > I2C_CLIENT_IGNORE_BUSY. > > But I may be biased :) > > > > I mean this is broken firmware, and having that fixed in ACPI and > > platform/x86 code instead of I2C core sounds way more proper to me. > > Andy, Heikki, what is your take on this approach: My concern is to get rid of (more precisely do not create) the non- existing client in the hierarchy. What you described here sounds plausible approach. Looking forward for the updated version. Thanks for doing this! > > 1) Have a list of ACPI IDs for which to not set the > acpi_device_enumeration_by_parent flag in drivers/acpi/scan.c > so that they get enumerated as a platform device. > > 2) Turn the i2c-multi-instantiate.c driver from the second patch > of this series into a platform driver which will then > instantiate *all* the i2c-clients for the ACPI fwnode as > before. > > This will get rid of the ugly duplicate i2c_client for the first > listed i2c-resouce in the ACPI fwnode and instantiating a > platform device for ACPI fwnode-s is normal, so I think that this > is the cleaner way. > > If you agree then I will rework the patch-set to do this when > I've some time for this.
On Sun, 2018-07-01 at 14:23 +0200, Hans de Goede wrote: > Hi All, > > Here is more or less a resend of v1 of my second attempt at dealing > with > multiple i2c devices being described in a single ACPI fwnode. > > The only change is that it has been rebased from v4.17 to v4.18-rc2. > > Mika, Andy and Heikki, can you review this please? JFYI, I found this, I suspect Srinivas might be interested to see what we are doing: https://patchwork.kernel.org/patch/3855961/ > > Regards, > > Hans >