linux-acpi.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [RFC 0/4] platform/x86: i2c-multi-instantiate: Pass ACPI fwnode to instantiated i2c-clients
@ 2020-11-05  8:00 Hans de Goede
  2020-11-05  8:00 ` [RFC 1/4] i2c: core: Allow i2c_board_info to specify that the core should not try to find an IRQ Hans de Goede
                   ` (4 more replies)
  0 siblings, 5 replies; 14+ messages in thread
From: Hans de Goede @ 2020-11-05  8:00 UTC (permalink / raw)
  To: Mark Gross, Wolfram Sang
  Cc: Hans de Goede, Andy Shevchenko, platform-driver-x86, linux-i2c,
	linux-acpi

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.

Regards,

Hans

p.s.

The 4th patch is not related to the fwnode passing, but was also
necessary for the BOSC0200 case.


^ permalink raw reply	[flat|nested] 14+ messages in thread

* [RFC 1/4] i2c: core: Allow i2c_board_info to specify that the core should not try to find an IRQ
  2020-11-05  8:00 [RFC 0/4] platform/x86: i2c-multi-instantiate: Pass ACPI fwnode to instantiated i2c-clients Hans de Goede
@ 2020-11-05  8:00 ` Hans de Goede
  2020-11-05  8:00 ` [RFC 2/4] platform/x86: i2c-multi-instantiate: Set i2c_board_info.irq to -ENOENT when no IRQ is specified Hans de Goede
                   ` (3 subsequent siblings)
  4 siblings, 0 replies; 14+ messages in thread
From: Hans de Goede @ 2020-11-05  8:00 UTC (permalink / raw)
  To: Mark Gross, Wolfram Sang
  Cc: Hans de Goede, Andy Shevchenko, platform-driver-x86, linux-i2c,
	linux-acpi

When I2C devices are enumerated/instantiated through ACPI tables then
a single ACPI device may describe multiple separate I2C connected ICs.

This is handled by the drivers/platform/x86/i2c-multi-instantiate.c
code which contains a table which maps the ACPI-device-id to the
information necessary to instantiate the i2c-clients (type and IRQ
routing for each described IC).

In some cases the i2c-driver may need access to the ACPI-fwnode as
that may contain ACPI-methods supplying e.g. orientation-matrix info
for accelerometers.

Currently setting i2c_board_info.fwnode to point to the ACPI-fwnode
will cause the i2c-core to call i2c_acpi_get_irq() for any i2c-clients
for which i2c-multi-instantiate.c has not passed an IRQ in
i2c_board_info.irq, messing up the IRQ routing done by
i2c-multi-instantiate.c.

Make i2c_device_probe() accept a client->init_irq value < 0 to skip
the i2c-core IRQ handling, while still setting client->irq to 0
after checking for this, since most i2c-drivers expect client->irq == 0
for clients without an IRQ.

This allows i2c-multi-instantiate.c to set i2c_board_info.irq = -ENOENT
for clients without an IRQ and pass the ACPI-fwnode without issues.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/i2c/i2c-core-base.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c
index 573b5da145d1..1887e2267031 100644
--- a/drivers/i2c/i2c-core-base.c
+++ b/drivers/i2c/i2c-core-base.c
@@ -467,11 +467,10 @@ static int i2c_device_probe(struct device *dev)
 			goto put_sync_adapter;
 		}
 
-		if (irq < 0)
-			irq = 0;
-
 		client->irq = irq;
 	}
+	if (client->irq < 0)
+		client->irq = 0;
 
 	driver = to_i2c_driver(dev->driver);
 
-- 
2.28.0


^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [RFC 2/4] platform/x86: i2c-multi-instantiate: Set i2c_board_info.irq to -ENOENT when no IRQ is specified
  2020-11-05  8:00 [RFC 0/4] platform/x86: i2c-multi-instantiate: Pass ACPI fwnode to instantiated i2c-clients Hans de Goede
  2020-11-05  8:00 ` [RFC 1/4] i2c: core: Allow i2c_board_info to specify that the core should not try to find an IRQ Hans de Goede
@ 2020-11-05  8:00 ` Hans de Goede
  2020-11-05  8:00 ` [RFC 3/4] platform/x86: i2c-multi-instantiate: Pass ACPI fwnode to instantiated I2C-clients Hans de Goede
                   ` (2 subsequent siblings)
  4 siblings, 0 replies; 14+ messages in thread
From: Hans de Goede @ 2020-11-05  8:00 UTC (permalink / raw)
  To: Mark Gross, Wolfram Sang
  Cc: Hans de Goede, Andy Shevchenko, platform-driver-x86, linux-i2c,
	linux-acpi

In some cases the i2c-driver may need access to the ACPI-fwnode as
that may contain ACPI-methods supplying e.g. orientation-matrix info
for accelerometers.

Setting i2c_board_info.fwnode to point to the ACPI-fwnode, while
leaving i2c_board_info.irq set to 0 (in the IRQ_RESOURCE_NONE case)
will cause the i2c-core to assign the first IRQ described in the ACPI
resources to the client, which we do not want.

Set i2c_board_info.irq to -ENOENT instead of 0 in the IRQ_RESOURCE_NONE
case, to avoid this issue.

This is a preparation patch for passing the fwnode to i2c_acpi_new_device.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/platform/x86/i2c-multi-instantiate.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/platform/x86/i2c-multi-instantiate.c b/drivers/platform/x86/i2c-multi-instantiate.c
index 6acc8457866e..cb4688bdd6b6 100644
--- a/drivers/platform/x86/i2c-multi-instantiate.c
+++ b/drivers/platform/x86/i2c-multi-instantiate.c
@@ -113,7 +113,7 @@ static int i2c_multi_inst_probe(struct platform_device *pdev)
 			board_info.irq = ret;
 			break;
 		default:
-			board_info.irq = 0;
+			board_info.irq = -ENOENT;
 			break;
 		}
 		multi->clients[i] = i2c_acpi_new_device(dev, i, &board_info);
-- 
2.28.0


^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [RFC 3/4] platform/x86: i2c-multi-instantiate: Pass ACPI fwnode to instantiated I2C-clients
  2020-11-05  8:00 [RFC 0/4] platform/x86: i2c-multi-instantiate: Pass ACPI fwnode to instantiated i2c-clients Hans de Goede
  2020-11-05  8:00 ` [RFC 1/4] i2c: core: Allow i2c_board_info to specify that the core should not try to find an IRQ Hans de Goede
  2020-11-05  8:00 ` [RFC 2/4] platform/x86: i2c-multi-instantiate: Set i2c_board_info.irq to -ENOENT when no IRQ is specified Hans de Goede
@ 2020-11-05  8:00 ` Hans de Goede
  2020-11-05 10:31   ` Andy Shevchenko
  2020-11-05  8:00 ` [RFC 4/4] platform/x86: i2c-multi-instantiate: Add IRQ_RESOURCE_GPIO_OPTIONAL IRQ type Hans de Goede
  2020-11-05 10:38 ` [RFC 0/4] platform/x86: i2c-multi-instantiate: Pass ACPI fwnode to instantiated i2c-clients Andy Shevchenko
  4 siblings, 1 reply; 14+ messages in thread
From: Hans de Goede @ 2020-11-05  8:00 UTC (permalink / raw)
  To: Mark Gross, Wolfram Sang
  Cc: Hans de Goede, Andy Shevchenko, platform-driver-x86, linux-i2c,
	linux-acpi

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.

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.

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


^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [RFC 4/4] platform/x86: i2c-multi-instantiate: Add IRQ_RESOURCE_GPIO_OPTIONAL IRQ type
  2020-11-05  8:00 [RFC 0/4] platform/x86: i2c-multi-instantiate: Pass ACPI fwnode to instantiated i2c-clients Hans de Goede
                   ` (2 preceding siblings ...)
  2020-11-05  8:00 ` [RFC 3/4] platform/x86: i2c-multi-instantiate: Pass ACPI fwnode to instantiated I2C-clients Hans de Goede
@ 2020-11-05  8:00 ` Hans de Goede
  2020-11-05 10:36   ` Andy Shevchenko
  2020-11-05 10:38 ` [RFC 0/4] platform/x86: i2c-multi-instantiate: Pass ACPI fwnode to instantiated i2c-clients Andy Shevchenko
  4 siblings, 1 reply; 14+ messages in thread
From: Hans de Goede @ 2020-11-05  8:00 UTC (permalink / raw)
  To: Mark Gross, Wolfram Sang
  Cc: Hans de Goede, Andy Shevchenko, platform-driver-x86, linux-i2c,
	linux-acpi

Most I2C-drivers treat IRQs as optional and in some cases ACPI
devices managed by i2c-multi-instantiate.c may have a GpioInt resource
specified on some systems, while it is not there on others.

Add a new IRQ_RESOURCE_GPIO_OPTIONAL IRQ type, which still tries to get
a GpioInt IRQ, but does not consider it a fatal error when this fails.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/platform/x86/i2c-multi-instantiate.c | 16 +++++++++++-----
 1 file changed, 11 insertions(+), 5 deletions(-)

diff --git a/drivers/platform/x86/i2c-multi-instantiate.c b/drivers/platform/x86/i2c-multi-instantiate.c
index cbccfcbed44c..55c6d6e8d576 100644
--- a/drivers/platform/x86/i2c-multi-instantiate.c
+++ b/drivers/platform/x86/i2c-multi-instantiate.c
@@ -15,10 +15,11 @@
 #include <linux/platform_device.h>
 #include <linux/types.h>
 
-#define IRQ_RESOURCE_TYPE	GENMASK(1, 0)
-#define IRQ_RESOURCE_NONE	0
-#define IRQ_RESOURCE_GPIO	1
-#define IRQ_RESOURCE_APIC	2
+#define IRQ_RESOURCE_TYPE		GENMASK(1, 0)
+#define IRQ_RESOURCE_NONE		0
+#define IRQ_RESOURCE_GPIO		1
+#define IRQ_RESOURCE_GPIO_OPTIONAL	2
+#define IRQ_RESOURCE_APIC		3
 
 struct i2c_inst_data {
 	const char *type;
@@ -64,6 +65,7 @@ static int i2c_multi_inst_probe(struct platform_device *pdev)
 	struct i2c_board_info board_info = {};
 	struct device *dev = &pdev->dev;
 	struct acpi_device *adev;
+	bool irq_optional;
 	char name[32];
 	int i, ret;
 
@@ -94,10 +96,14 @@ static int i2c_multi_inst_probe(struct platform_device *pdev)
 			 inst_data[i].type, i);
 		board_info.dev_name = name;
 		board_info.fwnode = dev->fwnode;
+		irq_optional = false;
 		switch (inst_data[i].flags & IRQ_RESOURCE_TYPE) {
+		case IRQ_RESOURCE_GPIO_OPTIONAL:
+			irq_optional = true;
+			fallthrough;
 		case IRQ_RESOURCE_GPIO:
 			ret = acpi_dev_gpio_irq_get(adev, inst_data[i].irq_idx);
-			if (ret < 0) {
+			if (ret < 0 && (!irq_optional || ret != -ENOENT)) {
 				dev_err(dev, "Error requesting irq at index %d: %d\n",
 					inst_data[i].irq_idx, ret);
 				goto error;
-- 
2.28.0


^ permalink raw reply related	[flat|nested] 14+ messages in thread

* Re: [RFC 3/4] platform/x86: i2c-multi-instantiate: Pass ACPI fwnode to instantiated I2C-clients
  2020-11-05  8:00 ` [RFC 3/4] platform/x86: i2c-multi-instantiate: Pass ACPI fwnode to instantiated I2C-clients Hans de Goede
@ 2020-11-05 10:31   ` Andy Shevchenko
  0 siblings, 0 replies; 14+ messages in thread
From: Andy Shevchenko @ 2020-11-05 10:31 UTC (permalink / raw)
  To: Hans de Goede
  Cc: Mark Gross, Wolfram Sang, Andy Shevchenko, Platform Driver,
	linux-i2c, ACPI Devel Maling List

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

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [RFC 4/4] platform/x86: i2c-multi-instantiate: Add IRQ_RESOURCE_GPIO_OPTIONAL IRQ type
  2020-11-05  8:00 ` [RFC 4/4] platform/x86: i2c-multi-instantiate: Add IRQ_RESOURCE_GPIO_OPTIONAL IRQ type Hans de Goede
@ 2020-11-05 10:36   ` Andy Shevchenko
  0 siblings, 0 replies; 14+ messages in thread
From: Andy Shevchenko @ 2020-11-05 10:36 UTC (permalink / raw)
  To: Hans de Goede
  Cc: Mark Gross, Wolfram Sang, Andy Shevchenko, Platform Driver,
	linux-i2c, ACPI Devel Maling List

On Thu, Nov 5, 2020 at 10:00 AM Hans de Goede <hdegoede@redhat.com> wrote:
>
> Most I2C-drivers treat IRQs as optional and in some cases ACPI
> devices managed by i2c-multi-instantiate.c may have a GpioInt resource
> specified on some systems, while it is not there on others.

GpioInt()

> Add a new IRQ_RESOURCE_GPIO_OPTIONAL IRQ type, which still tries to get
> a GpioInt IRQ, but does not consider it a fatal error when this fails.

GpioInt()

...

> -#define IRQ_RESOURCE_TYPE      GENMASK(1, 0)
> -#define IRQ_RESOURCE_NONE      0
> -#define IRQ_RESOURCE_GPIO      1
> -#define IRQ_RESOURCE_APIC      2
> +#define IRQ_RESOURCE_TYPE              GENMASK(1, 0)
> +#define IRQ_RESOURCE_NONE              0
> +#define IRQ_RESOURCE_GPIO              1
> +#define IRQ_RESOURCE_GPIO_OPTIONAL     2
> +#define IRQ_RESOURCE_APIC              3

I think flag is cleaner:

#define IRQ_RESOURCE_OPTIONAL     BIT(2)

...

>                 case IRQ_RESOURCE_GPIO:
>                         ret = acpi_dev_gpio_irq_get(adev, inst_data[i].irq_idx);
> -                       if (ret < 0) {
> +                       if (ret < 0 && (!irq_optional || ret != -ENOENT)) {

ret < 0 && !((inst_data[i].flags & IRQ_RESOURCE_OPTIONAL) && ret == -ENOENT)

>                                 dev_err(dev, "Error requesting irq at index %d: %d\n",
>                                         inst_data[i].irq_idx, ret);
>                                 goto error;

-- 
With Best Regards,
Andy Shevchenko

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [RFC 0/4] platform/x86: i2c-multi-instantiate: Pass ACPI fwnode to instantiated i2c-clients
  2020-11-05  8:00 [RFC 0/4] platform/x86: i2c-multi-instantiate: Pass ACPI fwnode to instantiated i2c-clients Hans de Goede
                   ` (3 preceding siblings ...)
  2020-11-05  8:00 ` [RFC 4/4] platform/x86: i2c-multi-instantiate: Add IRQ_RESOURCE_GPIO_OPTIONAL IRQ type Hans de Goede
@ 2020-11-05 10:38 ` Andy Shevchenko
  2020-11-09 11:33   ` Hans de Goede
  4 siblings, 1 reply; 14+ messages in thread
From: Andy Shevchenko @ 2020-11-05 10:38 UTC (permalink / raw)
  To: Hans de Goede, Krogerus, Heikki
  Cc: Mark Gross, Wolfram Sang, Andy Shevchenko, Platform Driver,
	linux-i2c, ACPI Devel Maling List

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).

Please, send as non-RFC and also Cc Heikki (just in case if he has
comments wrt INT3515).

-- 
With Best Regards,
Andy Shevchenko

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [RFC 0/4] platform/x86: i2c-multi-instantiate: Pass ACPI fwnode to instantiated i2c-clients
  2020-11-05 10:38 ` [RFC 0/4] platform/x86: i2c-multi-instantiate: Pass ACPI fwnode to instantiated i2c-clients Andy Shevchenko
@ 2020-11-09 11:33   ` Hans de Goede
  2020-11-10 10:10     ` Andy Shevchenko
  0 siblings, 1 reply; 14+ messages in thread
From: Hans de Goede @ 2020-11-09 11:33 UTC (permalink / raw)
  To: Andy Shevchenko, Krogerus, Heikki
  Cc: Mark Gross, Wolfram Sang, Andy Shevchenko, Platform Driver,
	linux-i2c, ACPI Devel Maling List

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



^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [RFC 0/4] platform/x86: i2c-multi-instantiate: Pass ACPI fwnode to instantiated i2c-clients
  2020-11-09 11:33   ` Hans de Goede
@ 2020-11-10 10:10     ` Andy Shevchenko
  2020-11-10 11:14       ` Hans de Goede
  0 siblings, 1 reply; 14+ messages in thread
From: Andy Shevchenko @ 2020-11-10 10:10 UTC (permalink / raw)
  To: Hans de Goede
  Cc: Krogerus, Heikki, Mark Gross, Wolfram Sang, Andy Shevchenko,
	Platform Driver, linux-i2c, ACPI Devel Maling List

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.

-- 
With Best Regards,
Andy Shevchenko

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [RFC 0/4] platform/x86: i2c-multi-instantiate: Pass ACPI fwnode to instantiated i2c-clients
  2020-11-10 10:10     ` Andy Shevchenko
@ 2020-11-10 11:14       ` Hans de Goede
  2020-11-10 14:47         ` Andy Shevchenko
  0 siblings, 1 reply; 14+ messages in thread
From: Hans de Goede @ 2020-11-10 11:14 UTC (permalink / raw)
  To: Andy Shevchenko
  Cc: Krogerus, Heikki, Mark Gross, Wolfram Sang, Andy Shevchenko,
	Platform Driver, linux-i2c, ACPI Devel Maling List

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


^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [RFC 0/4] platform/x86: i2c-multi-instantiate: Pass ACPI fwnode to instantiated i2c-clients
  2020-11-10 11:14       ` Hans de Goede
@ 2020-11-10 14:47         ` Andy Shevchenko
  2020-11-24 10:35           ` Hans de Goede
  0 siblings, 1 reply; 14+ messages in thread
From: Andy Shevchenko @ 2020-11-10 14:47 UTC (permalink / raw)
  To: Hans de Goede
  Cc: Krogerus, Heikki, Mark Gross, Wolfram Sang, Andy Shevchenko,
	Platform Driver, linux-i2c, ACPI Devel Maling List

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.

-- 
With Best Regards,
Andy Shevchenko

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [RFC 0/4] platform/x86: i2c-multi-instantiate: Pass ACPI fwnode to instantiated i2c-clients
  2020-11-10 14:47         ` Andy Shevchenko
@ 2020-11-24 10:35           ` Hans de Goede
  2020-11-24 11:35             ` Andy Shevchenko
  0 siblings, 1 reply; 14+ messages in thread
From: Hans de Goede @ 2020-11-24 10:35 UTC (permalink / raw)
  To: Andy Shevchenko
  Cc: Krogerus, Heikki, Mark Gross, Wolfram Sang, Andy Shevchenko,
	Platform Driver, linux-i2c, ACPI Devel Maling List

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


^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [RFC 0/4] platform/x86: i2c-multi-instantiate: Pass ACPI fwnode to instantiated i2c-clients
  2020-11-24 10:35           ` Hans de Goede
@ 2020-11-24 11:35             ` Andy Shevchenko
  0 siblings, 0 replies; 14+ messages in thread
From: Andy Shevchenko @ 2020-11-24 11:35 UTC (permalink / raw)
  To: Hans de Goede
  Cc: Krogerus, Heikki, Mark Gross, Wolfram Sang, Andy Shevchenko,
	Platform Driver, linux-i2c, ACPI Devel Maling List

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!

-- 
With Best Regards,
Andy Shevchenko

^ permalink raw reply	[flat|nested] 14+ messages in thread

end of thread, other threads:[~2020-11-24 11:35 UTC | newest]

Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-11-05  8:00 [RFC 0/4] platform/x86: i2c-multi-instantiate: Pass ACPI fwnode to instantiated i2c-clients Hans de Goede
2020-11-05  8:00 ` [RFC 1/4] i2c: core: Allow i2c_board_info to specify that the core should not try to find an IRQ Hans de Goede
2020-11-05  8:00 ` [RFC 2/4] platform/x86: i2c-multi-instantiate: Set i2c_board_info.irq to -ENOENT when no IRQ is specified Hans de Goede
2020-11-05  8:00 ` [RFC 3/4] platform/x86: i2c-multi-instantiate: Pass ACPI fwnode to instantiated I2C-clients Hans de Goede
2020-11-05 10:31   ` Andy Shevchenko
2020-11-05  8:00 ` [RFC 4/4] platform/x86: i2c-multi-instantiate: Add IRQ_RESOURCE_GPIO_OPTIONAL IRQ type Hans de Goede
2020-11-05 10:36   ` Andy Shevchenko
2020-11-05 10:38 ` [RFC 0/4] platform/x86: i2c-multi-instantiate: Pass ACPI fwnode to instantiated i2c-clients Andy Shevchenko
2020-11-09 11:33   ` Hans de Goede
2020-11-10 10:10     ` Andy Shevchenko
2020-11-10 11:14       ` Hans de Goede
2020-11-10 14:47         ` Andy Shevchenko
2020-11-24 10:35           ` Hans de Goede
2020-11-24 11:35             ` Andy Shevchenko

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).