| Message ID | 20201105080014.45410-1-hdegoede@redhat.com |
|---|---|
| Headers | show |
| Series | platform/x86: i2c-multi-instantiate: Pass ACPI fwnode to instantiated i2c-clients | expand |
On Thu, Nov 5, 2020 at 10:00 AM Hans de Goede <hdegoede@redhat.com> wrote: > > The ACPI fwnode may contain additional info which is useful for the > I2C-driver. E.g. some accelerometer ACPI fwnode's contain an ACPI method > providing rotation/mount matrix info. > > Pass the ACPI-fwnode to the instantiated I2C-clients by setting > i2c_board_info.fwnode, so that the I2C-drivers can access this info. > > Now that we set i2c_board_info.irq to -ENOENT if there is no IRQ, > avoiding the I2C-core assigning the first IRQ described in the ACPI > resources to the client, this is safe to do. > > Setting the fwnode also influences acpi_device_[uevent_]modalias and > acpi_dev_pm_attach, but these both call acpi_device_is_first_physical_node > and are a no-op if this returns false. Perhaps add () to the mentioned function calls. > The first physical node for the ACPI fwnode is actually the ACPI core > instantiated platform-device to which the I2C-multi-instantiate driver > binds, so acpi_device_is_first_physical_node always returns false for > the instantiated I2C-clients and thus we can safely pass the fwnode. Ditto. > Signed-off-by: Hans de Goede <hdegoede@redhat.com> > --- > drivers/platform/x86/i2c-multi-instantiate.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/drivers/platform/x86/i2c-multi-instantiate.c b/drivers/platform/x86/i2c-multi-instantiate.c > index cb4688bdd6b6..cbccfcbed44c 100644 > --- a/drivers/platform/x86/i2c-multi-instantiate.c > +++ b/drivers/platform/x86/i2c-multi-instantiate.c > @@ -93,6 +93,7 @@ static int i2c_multi_inst_probe(struct platform_device *pdev) > snprintf(name, sizeof(name), "%s-%s.%d", dev_name(dev), > inst_data[i].type, i); > board_info.dev_name = name; > + board_info.fwnode = dev->fwnode; > switch (inst_data[i].flags & IRQ_RESOURCE_TYPE) { > case IRQ_RESOURCE_GPIO: > ret = acpi_dev_gpio_irq_get(adev, inst_data[i].irq_idx); > -- > 2.28.0 > -- With Best Regards, Andy Shevchenko
Hi, On 11/5/20 11:38 AM, Andy Shevchenko wrote: > On Thu, Nov 5, 2020 at 10:00 AM Hans de Goede <hdegoede@redhat.com> wrote: >> >> Hi All, >> >> As the subject says this series is mostly about passing the ACPI fwnode to >> i2c-clients instantiated by the i2c-multi-instantiate code. >> >> As discussed here: >> https://bugzilla.kernel.org/show_bug.cgi?id=198671 >> >> BOSC0200 ACPI devices may sometimes describe 2 accelerometers in a single >> ACPI device, while working on this I noticed that BOSC0200 ACPI nodes >> contain ACCEL_MOUNT_MATRIX info (unlike all the other ACPI ids for bmc150 >> accelerometers). Which is why I wanted to pass the fwnode so that we >> could use this info in the bmc150-accel driver. >> >> The plan was to use i2c-multi-instantiate for this, but doing so will >> change the modalias and /lib/udev/hwdb.d/60-sensor.hwdb matches on >> the modalias for various quirks setting ACCEL_MOUNT_MATRIX. So then the >> plan became to first add support for the mount-matrix provided inside >> the BOSC0200 ACPI node, making the udev info unnecessary. But for at >> least 1 model (and probably more) the BOSC0200 ACPI node and hwdb info >> does not match and since the hwdb info is added by users of the actual >> devices we can assume it is correct, so it seems that we cannot always >> trust the ACPI provided info. This is ok, the hwdb info overrides it >> (iio-sensor-proxy prefers the udev provided mount-matrix over the >> one provided by the driver) but this means that we MUST keep the >> existing hwdb matches working, which means that we cannot use >> i2c-multi-instantiate for this. >> >> Instead I will dust of an old patch for this from Jeremy Cline: >> https://patchwork.kernel.org/project/linux-iio/patch/010001602cf53153-39ad69f1-1b39-4e6d-a748-9455a16c2fbd-000000@email.amazonses.com/ >> >> Which deals with there being 2 accelerometers inside the bmc150-accel >> driver. >> >> But before coming to the conclusion that i2c-multi-instantiate >> would not work I had already written this series. Since this might >> be useful for some other case in the future I'm sending this out >> as a RFC now, mostly so that it gets added to the archives. > > I think they are in pretty good shape (only the 4th required a bit of > attention). FWIW I agree with the changes which you suggest for the 4th patch. > Please, send as non-RFC and also Cc Heikki (just in case if he has > comments wrt INT3515). But do we really want to land these changes, while ATM we do not really have any need for them ? Esp. the "platform/x86: i2c-multi-instantiate: Pass ACPI fwnode to instantiated I2C-clients" Change is not without a chance of regressions. The acpi_device_is_first_physical_node() behavior surprised me a bit while working on the BOSC0200 changes. So I'm not 100% sure I have managed to see / think of all implications of this change. Heikki do you now (or in the near future) need access to the fwnode for the TypeC controllers handled by the i2c-multi-instantiate code ? Note that if we do decide to move forward with this set, it should probably be merged in its entirety by Wolfram as it also makes i2c-core changes (or Wolfram could just merge the i2c-core change and provide an immutable branch for me to merge into pdx86/for-next. And then your (Andy's) cleanup series can be applied on top of this once merged. Regards, Hans
On Mon, Nov 9, 2020 at 1:33 PM Hans de Goede <hdegoede@redhat.com> wrote: > On 11/5/20 11:38 AM, Andy Shevchenko wrote: > > On Thu, Nov 5, 2020 at 10:00 AM Hans de Goede <hdegoede@redhat.com> wrote: ... > >> But before coming to the conclusion that i2c-multi-instantiate > >> would not work I had already written this series. Since this might > >> be useful for some other case in the future I'm sending this out > >> as a RFC now, mostly so that it gets added to the archives. > > > > I think they are in pretty good shape (only the 4th required a bit of > > attention). > > FWIW I agree with the changes which you suggest for the 4th patch. > > > Please, send as non-RFC and also Cc Heikki (just in case if he has > > comments wrt INT3515). > > But do we really want to land these changes, while ATM we do not > really have any need for them ? Esp. the > > "platform/x86: i2c-multi-instantiate: Pass ACPI fwnode to instantiated I2C-clients" > > Change is not without a chance of regressions. The acpi_device_is_first_physical_node() > behavior surprised me a bit while working on the BOSC0200 changes. So I'm not > 100% sure I have managed to see / think of all implications of this change. I think in general the direction to switch to fwnode is a good one. I was thinking about moving i2c core to use swnodes in which case they will utilize fwnode pointer. But it might have complications, you are right. > Heikki do you now (or in the near future) need access to the fwnode for > the TypeC controllers handled by the i2c-multi-instantiate code ? > > Note that if we do decide to move forward with this set, it should probably > be merged in its entirety by Wolfram as it also makes i2c-core changes > (or Wolfram could just merge the i2c-core change and provide an immutable > branch for me to merge into pdx86/for-next. > > And then your (Andy's) cleanup series can be applied on top of this once merged. Fine to me.
Hi, On 11/10/20 11:10 AM, Andy Shevchenko wrote: > On Mon, Nov 9, 2020 at 1:33 PM Hans de Goede <hdegoede@redhat.com> wrote: >> On 11/5/20 11:38 AM, Andy Shevchenko wrote: >>> On Thu, Nov 5, 2020 at 10:00 AM Hans de Goede <hdegoede@redhat.com> wrote: > > ... > >>>> But before coming to the conclusion that i2c-multi-instantiate >>>> would not work I had already written this series. Since this might >>>> be useful for some other case in the future I'm sending this out >>>> as a RFC now, mostly so that it gets added to the archives. >>> >>> I think they are in pretty good shape (only the 4th required a bit of >>> attention). >> >> FWIW I agree with the changes which you suggest for the 4th patch. >> >>> Please, send as non-RFC and also Cc Heikki (just in case if he has >>> comments wrt INT3515). >> >> But do we really want to land these changes, while ATM we do not >> really have any need for them ? Esp. the >> >> "platform/x86: i2c-multi-instantiate: Pass ACPI fwnode to instantiated I2C-clients" >> >> Change is not without a chance of regressions. The acpi_device_is_first_physical_node() >> behavior surprised me a bit while working on the BOSC0200 changes. So I'm not >> 100% sure I have managed to see / think of all implications of this change. > > I think in general the direction to switch to fwnode is a good one. I > was thinking about moving i2c core to use swnodes in which case they > will utilize fwnode pointer. But it might have complications, you are > right. So do you agree to just keep this series in the archives (in case we need it later) for now ? Or would you still like me to post a non RFC version ? Regards, Hans
On Tue, Nov 10, 2020 at 1:14 PM Hans de Goede <hdegoede@redhat.com> wrote: > On 11/10/20 11:10 AM, Andy Shevchenko wrote: > > On Mon, Nov 9, 2020 at 1:33 PM Hans de Goede <hdegoede@redhat.com> wrote: ... > > I think in general the direction to switch to fwnode is a good one. I > > was thinking about moving i2c core to use swnodes in which case they > > will utilize fwnode pointer. But it might have complications, you are > > right. > > So do you agree to just keep this series in the archives (in case we need > it later) for now ? Or would you still like me to post a non RFC version ? If nobody else has a different opinion (Heikki, Wolfram, Rafael?), I'm fine with it to be in archives for the time being.
Hi, On 11/10/20 3:47 PM, Andy Shevchenko wrote: > On Tue, Nov 10, 2020 at 1:14 PM Hans de Goede <hdegoede@redhat.com> wrote: >> On 11/10/20 11:10 AM, Andy Shevchenko wrote: >>> On Mon, Nov 9, 2020 at 1:33 PM Hans de Goede <hdegoede@redhat.com> wrote: > > ... > >>> I think in general the direction to switch to fwnode is a good one. I >>> was thinking about moving i2c core to use swnodes in which case they >>> will utilize fwnode pointer. But it might have complications, you are >>> right. >> >> So do you agree to just keep this series in the archives (in case we need >> it later) for now ? Or would you still like me to post a non RFC version ? > > If nobody else has a different opinion (Heikki, Wolfram, Rafael?), I'm > fine with it to be in archives for the time being. Since no-one else has responded, lets just keep this series for the archives. Andy, that also means that there no longer is a reason to hold of merging your i2c-multi-instantiate cleanup series (minus patch 3 as discussed), so I've merged that into my review-hans branch now. Regards, Hans
On Tue, Nov 24, 2020 at 12:35 PM Hans de Goede <hdegoede@redhat.com> wrote: > On 11/10/20 3:47 PM, Andy Shevchenko wrote: > > On Tue, Nov 10, 2020 at 1:14 PM Hans de Goede <hdegoede@redhat.com> wrote: > >> On 11/10/20 11:10 AM, Andy Shevchenko wrote: > >>> On Mon, Nov 9, 2020 at 1:33 PM Hans de Goede <hdegoede@redhat.com> wrote: > > > > ... > > > >>> I think in general the direction to switch to fwnode is a good one. I > >>> was thinking about moving i2c core to use swnodes in which case they > >>> will utilize fwnode pointer. But it might have complications, you are > >>> right. > >> > >> So do you agree to just keep this series in the archives (in case we need > >> it later) for now ? Or would you still like me to post a non RFC version ? > > > > If nobody else has a different opinion (Heikki, Wolfram, Rafael?), I'm > > fine with it to be in archives for the time being. > > Since no-one else has responded, lets just keep this series for the > archives. > > Andy, that also means that there no longer is a reason to hold of merging > your i2c-multi-instantiate cleanup series (minus patch 3 as discussed), > so I've merged that into my review-hans branch now. Cool, thanks!