I2C and SPI dev_name change for ACPI enumerated slaves

Current I2C adapter id - client address "x-00yy" based device naming scheme is not always stable enough to be used in name based matching, for instance within ALSA SoC subsystem.

This is problematic in PC kind of platforms where I2C adapter numbers can change due variable amount of bus controllers, probe order, add-on cards or just because of BIOS settings.

This patch addresses the problem by using the ACPI device name with "i2c-" prefix for ACPI enumerated I2C slaves. For them device name "x-00yz" becomes "i2c-INTABCD:ij" after this patch.

Signed-off-by Jarkko Nikula <[email protected]>

---
I'm not sure when would acpi_bus_get_device fail and how realistic is that here. I put minimalistic error handling here which just falls back to old adapter-addr naming scheme.

---
 drivers/i2c/i2c-core.c | 24 ++++++++++++++++++++----
 1 file changed, 20 insertions(+), 4 deletions(-)

drivers/i2c/i2c-core.c +0 -0

index 111b2c6..8d6f3e5 100644

drivers/i2c/i2c-core.c +14 -0

+		struct acpi_device *adev;
+		if (!acpi_bus_get_device(ACPI_HANDLE(&client->dev), &adev)) {
+			dev_set_name(&client->dev, "i2c-%s",
+				     dev_name(&adev->dev));
+			return;
+		}
+	}
+#endif
+	/* For 10-bit clients, add an arbitrary offset to avoid collisions */
+	dev_set_name(&client->dev, "%d-%04x", i2c_adapter_id(adap),
+		     client->addr | ((client->flags & I2C_CLIENT_TEN)
+				     ? 0xa000 : 0));
+}
+
 /**
  * i2c_new_device - instantiate an i2c device
  * @adap: the adapter managing the device

@@ -671,10 +690,7 @@ i2c_new_device(struct i2c_adapter *adap, struct i2c_board_info const *info)
 	client->dev.of_node = info->of_node;

drivers/i2c/i2c-core.c +5 -0

+static void i2c_dev_set_name(struct i2c_adapter *adap,
+			     struct i2c_client *client)
+{
+#if IS_ENABLED(CONFIG_ACPI)
+	if (ACPI_HANDLE(&client->dev)) {

ACPI_HANDLE() already contains an "is CONFIG_ACPI enabled?" check, so the #if around the if (ACPI_HANDLE()) {} is redundant.

Similarly for the SPI patch.

Hi

Quoted 9 lines · Rafael J. Wysocki

drivers/i2c/i2c-core.c +5 -0

+static void i2c_dev_set_name(struct i2c_adapter *adap,
+			     struct i2c_client *client)
+{
+#if IS_ENABLED(CONFIG_ACPI)
+	if (ACPI_HANDLE(&client->dev)) {

ACPI_HANDLE() already contains an "is CONFIG_ACPI enabled?" check, so the #if around the if (ACPI_HANDLE()) {} is redundant.

Quoted 2 lines

Similarly for the SPI patch.

Well, acpi_bus_get_device() is not available for non-ACPI builds and at least the gcc I used for test build didn't optimize that block away.

Quoted 3 lines · Jarkko Nikula

Well, acpi_bus_get_device() is not available for non-ACPI builds and at least the gcc I used for test build didn't optimize that block away.

Well, it should. ACPI_HANDLE() translates to (NULL) if CONFIG_ACPI is not defined and that causes the check to become "if (NULL)" which should always be dropped by the compiler.

Does providing a stub acpi_bus_get_device() for !CONFIG_ACPI actually help?

Quoted 8 lines · Rafael J. Wysocki

Quoted 3 lines · Jarkko Nikula

Well, acpi_bus_get_device() is not available for non-ACPI builds and at least the gcc I used for test build didn't optimize that block away.

Well, it should. ACPI_HANDLE() translates to (NULL) if CONFIG_ACPI is not defined and that causes the check to become "if (NULL)" which should always be dropped by the compiler.

My very vague memory says the same. I don't know is this gcc version or flag dependent behavior. I got the build error from both i386 build using gcc 4.8.1 and arm build by using make ARCH=arm omap2plus_defconfig and gcc-4.7-arm-linux-gnueabi 4.7.2-4.

Quoted 3 lines

Does providing a stub acpi_bus_get_device() for !CONFIG_ACPI actually help?

Hmm, not only. Referencing to dev field in struct acpi_device by dev_name(&adev->dev) here will fail too.

Quoted 3 lines · Jarkko Nikula

Quoted 1 line

Hmm, not only. Referencing to dev field in struct acpi_device by dev_name(&adev->dev) here will fail too.

Well, something is not quite right here.

One of the *reasons* for having ACPI_HANDLE() defined this way is to avoid using explicit CONFIG_ACPI checks, so if that doesn't work, then all of that becomes a bit pointless.

One possible thing to do is to let structure definitions to be available for non-ACPI builds. Then compiler won't fail on structure access which will be anyway optimized away by later compiler stages.

Hi Rafael

Quoted 11 lines · Rafael J. Wysocki

Quoted 3 lines · Jarkko Nikula

Quoted 1 line

Hmm, not only. Referencing to dev field in struct acpi_device by dev_name(&adev->dev) here will fail too.

Well, something is not quite right here.

One of the *reasons* for having ACPI_HANDLE() defined this way is to avoid using explicit CONFIG_ACPI checks, so if that doesn't work, then all of that becomes a bit pointless.

One possible thing to do is to let structure definitions to be available for non-ACPI builds. Then compiler won't fail on structure access which will be anyway optimized away by later compiler stages.

With a quick test below vmlinux section sizes don't change for couple non-ACPI build tests and allow to get rid off IS_ENABLED(CONFIG_ACPI) test in this patch. It's very minimal as it only moves the CONFIG_ACPI test just after the struct acpi_device definition and defines acpi_bus_get_device for non-ACPI case.

What I don't know how consistent it is as there are still couple structure definitions under CONFIG_ACPI, doesn't touch other acpi headers and requires to include acpi_bus.h in driver (or move acpi_bus.h include in linux/acpi.h currently under CONFIG_ACPI).

include/acpi/acpi_bus.h +0 -2

index d901982..7ab7870 100644
@@ -66,8 +66,6 @@ bool acpi_ata_match(acpi_handle handle);
 
  bool acpi_bay_match(acpi_handle handle);
  bool acpi_dock_match(acpi_handle handle);
 
-#ifdef CONFIG_ACPI
-
 
  #include <linux/proc_fs.h>

  
  #define ACPI_BUS_FILE_ROOT    "acpi"

include/acpi/acpi_bus.h +2 -0

@@ -314,6 +312,8 @@ struct acpi_device {
 
      void (*remove)(struct acpi_device *);
 
  };
 
+#if IS_ENABLED(CONFIG_ACPI)
+
 

  static inline void *acpi_driver_data(struct acpi_device *d)
  {

      return d->driver_data;

Quoted 14 lines · Jarkko Nikula

Hi Rafael

Quoted 11 lines · Rafael J. Wysocki

Quoted 3 lines · Jarkko Nikula

Quoted 1 line

Hmm, not only. Referencing to dev field in struct acpi_device by dev_name(&adev->dev) here will fail too.

Well, something is not quite right here.

One of the *reasons* for having ACPI_HANDLE() defined this way is to avoid using explicit CONFIG_ACPI checks, so if that doesn't work, then all of that becomes a bit pointless.

Can you please send patches inline instead of using inline attachments, so that people don't have to paste your patches back to comment them?

Quoted 3 lines

One possible thing to do is to let structure definitions to be available for non-ACPI builds. Then compiler won't fail on structure access which will be anyway optimized away by later compiler stages.

Yes, we can do that, but as I said that means we're giving up on the "why ACPI_HANDLE() doesn't work as expected" front. For now, I'd like to understand what's up before making that change. Moreover ->

Quoted 42 lines

With a quick test below vmlinux section sizes don't change for couple non-ACPI build tests and allow to get rid off IS_ENABLED(CONFIG_ACPI) test in this patch. It's very minimal as it only moves the CONFIG_ACPI test just after the struct acpi_device definition and defines acpi_bus_get_device for non-ACPI case.

What I don't know how consistent it is as there are still couple structure definitions under CONFIG_ACPI, doesn't touch other acpi headers and requires to include acpi_bus.h in driver (or move acpi_bus.h include in linux/acpi.h currently under CONFIG_ACPI).

include/acpi/acpi_bus.h +0 -2

index d901982..7ab7870 100644
@@ -66,8 +66,6 @@ bool acpi_ata_match(acpi_handle handle);
 
  bool acpi_bay_match(acpi_handle handle);
  bool acpi_dock_match(acpi_handle handle);
 
-#ifdef CONFIG_ACPI
-
 
  #include <linux/proc_fs.h>

  
  #define ACPI_BUS_FILE_ROOT    "acpi"

include/acpi/acpi_bus.h +2 -0

@@ -314,6 +312,8 @@ struct acpi_device {
 
      void (*remove)(struct acpi_device *);
 
  };
 
+#if IS_ENABLED(CONFIG_ACPI)
+
 

  static inline void *acpi_driver_data(struct acpi_device *d)
  {

      return d->driver_data;

include/acpi/acpi_bus.h static inline bool acpi_device_can_poweroff(struct · line 537

Quoted 7 lines

include/acpi/acpi_bus.h +2 -0

@@ -531,6 +531,8 @@ static inline bool acpi_device_can_poweroff(struct
 acpi_device *adev)
 
  static inline int register_acpi_bus_type(void *bus) { return 0; }
  static inline int unregister_acpi_bus_type(void *bus) { return 0; }
 
+static inline int acpi_bus_get_device(acpi_handle handle,
+                      struct acpi_device **device) { return 0; }

This has to return an error code, because otherwise the caller can assume *device to be a valid pointer after it returns which may not be the case.

Quoted 2 lines

include/acpi/acpi_bus.h +0 -0

  #endif                /* CONFIG_ACPI */

Current spi bus_num.chip_select "spix.y" based device naming scheme may not be stable enough to be used in name based matching, for instance within ALSA SoC subsystem.

Quoted 3 lines · Jarkko Nikula

Current spi bus_num.chip_select "spix.y" based device naming scheme may not be stable enough to be used in name based matching, for instance within ALSA SoC subsystem.

I'm happy with this from the SPI side modulo Raphael's comments - are folks happy from the ACPI side?

We may find use for struct acpi_device and acpi_bus_get_device() in generic code without wanting to add #if IS_ENABLED(CONFIG_ACPI) churn there.

Code can use ACPI_HANDLE() test to let compiler optimize out code blocks that are not used in !CONFIG_ACPI builds but structure definitions and function stubs must be available.

This patch moves CONFIG_ACPI test so that struct acpi_device definition is available for all builds that include acpi_bus.h and provides a stub for acpi_bus_get_device().

Signed-off-by Jarkko Nikula <[email protected]>

---
 include/acpi/acpi_bus.h | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

include/acpi/acpi_bus.h +7 -2

index 15100f6..232b37d3 100644
@@ -66,8 +66,6 @@ bool acpi_ata_match(acpi_handle handle);
 bool acpi_bay_match(acpi_handle handle);
 bool acpi_dock_match(acpi_handle handle);
 
-#ifdef CONFIG_ACPI
-
 #include <linux/proc_fs.h>
 
 #define ACPI_BUS_FILE_ROOT	"acpi"

@@ -315,6 +313,8 @@ struct acpi_device {
 	void (*remove)(struct acpi_device *);
 };
 
+#if IS_ENABLED(CONFIG_ACPI)
+
 static inline void *acpi_driver_data(struct acpi_device *d)
 {
 	return d->driver_data;

@@ -532,6 +532,11 @@ static inline bool acpi_device_can_poweroff(struct acpi_device *adev)
 
 static inline int register_acpi_bus_type(void *bus) { return 0; }
 static inline int unregister_acpi_bus_type(void *bus) { return 0; }
+static inline int acpi_bus_get_device(acpi_handle handle,
+				      struct acpi_device **device)
+{
+	return -ENODEV;
+}
 
 #endif				/* CONFIG_ACPI */
 

Quoted 7 lines · Jarkko Nikula

We may find use for struct acpi_device and acpi_bus_get_device() in generic code without wanting to add #if IS_ENABLED(CONFIG_ACPI) churn there.

Code can use ACPI_HANDLE() test to let compiler optimize out code blocks that are not used in !CONFIG_ACPI builds but structure definitions and function stubs must be available.

Quoted 5 lines

This patch moves CONFIG_ACPI test so that struct acpi_device definition is available for all builds that include acpi_bus.h and provides a stub for acpi_bus_get_device().

Signed-off-by Jarkko Nikula <jarkko.nikula-VuQAYsv1563Yd54FQh9/[email protected]>

This turns out the cause build problems to happen on some architectures.

I guess it's sufficient to simply define a stub struct acpi_device as

struct acpi_device {
	struct device dev;
};

for !CONFIG_ACPI instead.

Quoted 4 lines · Rafael J. Wysocki

This turns out the cause build problems to happen on some architectures.

I guess it's sufficient to simply define a stub struct acpi_device as

Quoted 5 lines

struct acpi_device {
	struct device dev;
};

for !CONFIG_ACPI instead.

Generally, it is a bad idea to #include acpi_bus.h for !CONFIG_ACPI.

The appended patch works for me, sorry for stealing part of your changelog.

Quoted 11 lines · Rafael J. Wysocki

Quoted 9 lines · Rafael J. Wysocki

This turns out the cause build problems to happen on some architectures.

I guess it's sufficient to simply define a stub struct acpi_device as

struct acpi_device {
	struct device dev;
};

for !CONFIG_ACPI instead.

Quoted 2 lines

Generally, it is a bad idea to #include acpi_bus.h for !CONFIG_ACPI.

Ouch, indeed.

Quoted 2 lines

The appended patch works for me, sorry for stealing part of your changelog.

No problem. Thanks for fixing this up.

Current spi bus_num.chip_select "spix.y" based device naming scheme may not be stable enough to be used in name based matching, for instance within ALSA SoC subsystem.

Hi

Second version of the set that changes I2C and SPI slave device names to be generated from stable ACPI device names on ACPI 5 based systems instead of using bus numbers which could change.

Slave device name change goes as

	"x-00yz" -> "i2c-INTABCD:ij"
	"spix.y" -> "spi-INTABCD:ij"

This version adds patch to include/acpi/acpi_bus.h that allow us to remove #if IS_ENABLED(CONFIG_ACPI) checks that were added in the first version.

Set goes on top linux-pm/linux-next commit e56b4d2.

First version here:
http://mailman.alsa-project.org/pipermail/alsa-devel/2013-October/067737.html

Jarkko Nikula (3):
ACPI: Expose struct acpi_device and acpi_bus_get_device() to non-ACPI
builds
i2c: Use stable dev_name for ACPI enumerated I2C slaves
spi: Use stable dev_name for ACPI enumerated SPI slaves

 drivers/i2c/i2c-core.c  | 24 ++++++++++++++++++++----
 drivers/spi/spi.c       | 20 +++++++++++++++++---
 include/acpi/acpi_bus.h |  9 +++++++--
 3 files changed, 44 insertions(+), 9 deletions(-)

Quoted 26 lines · Jarkko Nikula

Hi

Second version of the set that changes I2C and SPI slave device names to be generated from stable ACPI device names on ACPI 5 based systems instead of using bus numbers which could change.

Slave device name change goes as

	"x-00yz" -> "i2c-INTABCD:ij"
	"spix.y" -> "spi-INTABCD:ij"

This version adds patch to include/acpi/acpi_bus.h that allow us to remove #if IS_ENABLED(CONFIG_ACPI) checks that were added in the first version.

Set goes on top linux-pm/linux-next commit e56b4d2.

First version here:
http://mailman.alsa-project.org/pipermail/alsa-devel/2013-October/067737.html

Jarkko Nikula (3):
ACPI: Expose struct acpi_device and acpi_bus_get_device() to non-ACPI
builds
i2c: Use stable dev_name for ACPI enumerated I2C slaves
spi: Use stable dev_name for ACPI enumerated SPI slaves

Quoted 4 lines

 drivers/i2c/i2c-core.c  | 24 ++++++++++++++++++++----
 drivers/spi/spi.c       | 20 +++++++++++++++++---
 include/acpi/acpi_bus.h |  9 +++++++--
 3 files changed, 44 insertions(+), 9 deletions(-)

Looks good to me. If there are no objections, I can merge these through my tree.

Quoted 2 lines · Rafael J. Wysocki

Looks good to me. If there are no objections, I can merge these through my tree.

Which is basically fine with me. Do you want to have it in 3.13 already? I mean renaming the devices could lead to regressions, so I'd rather be conservative and aim for 3.14.

Quoted 7 lines · Wolfram Sang

Quoted 2 lines · Rafael J. Wysocki

Looks good to me. If there are no objections, I can merge these through my tree.

Which is basically fine with me. Do you want to have it in 3.13 already? I mean renaming the devices could lead to regressions, so I'd rather be conservative and aim for 3.14.

Valid concern. Quick grep below doesn't reveal any obvious device name matching outside of sound/soc/ but of course it doesn't prove it to be impossible.

git grep '[0-9]\-00' |grep name
git grep 'spi[0-9].[0-9]' |grep name

Well, if any breakage is caught in 3.13-rc, it should be easy enough to revert these changes and try again during the next cycle. I honestly don't see any benefit from waiting for the next cycle "just in case".

We have a few cases where we want to access struct device dev field in struct acpi_device from generic code that is build also without CONFIG_ACPI.

Provide here a minimal struct acpi_device stub that allows to build such a code without adding new #ifdef CONFIG_ACPI churn. This should not increase section sizes if code is protected by ACPI_COMPANION() runtime checks as those will be optimized out by later compiler stages in case CONFIG_ACPI is not set.

Signed-off-by Jarkko Nikula <[email protected]>

---
Rafael, instead of this we could also have an accessor but adev->dev looked better in actual code and size vmlinux didn't change for x86_64_defconfig with CONFIG_ACPI is not set nor for omap2plus_defconfig.

---
 include/linux/acpi.h | 4 ++++
 1 file changed, 4 insertions(+)

include/linux/acpi.h +4 -0

index 0da49ed..df444e1 100644
@@ -414,6 +414,10 @@ static inline bool acpi_driver_match_device(struct device *dev,
 
 #else	/* !CONFIG_ACPI */
 
+struct acpi_device {
+	struct device dev;
+};
+
 #define acpi_disabled 1
 
 #define ACPI_COMPANION(dev)		(NULL)

Signature · 6 lines

1.8.4.2

_______________________________________________
Alsa-devel mailing list
[email protected]
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

At Thu, 14 Nov 2013 11:00:59 +0200,

Quoted 9 lines · Jarkko Nikula

We have a few cases where we want to access struct device dev field in struct acpi_device from generic code that is build also without CONFIG_ACPI.

Provide here a minimal struct acpi_device stub that allows to build such a code without adding new #ifdef CONFIG_ACPI churn. This should not increase section sizes if code is protected by ACPI_COMPANION() runtime checks as those will be optimized out by later compiler stages in case CONFIG_ACPI is not set.

Quoted 5 lines

Signed-off-by Jarkko Nikula <[email protected]>

---
Rafael, instead of this we could also have an accessor but adev->dev looked better in actual code and size vmlinux didn't change for x86_64_defconfig with CONFIG_ACPI is not set nor for omap2plus_defconfig.

This looks too hackish, IMO. Defining a different dummy struct often gives more headaches. Thinking of the potential risk by misuses, it'd be simpler and safer to define a macro like acpi_dev_name() instead.

BTW, is linux/device.h already included in !CONFIG_ACPI case?

Current spi bus_num.chip_select "spix.y" based device naming scheme may not be stable enough to be used in name based matching, for instance within ALSA SoC subsystem.

Current I2C adapter id - client address "x-00yy" based device naming scheme is not always stable enough to be used in name based matching, for instance within ALSA SoC subsystem.

This is problematic in PC kind of platforms where I2C adapter numbers can change due variable amount of bus controllers, probe order, add-on cards or just because of BIOS settings.

This patch addresses the problem by using the ACPI device name with "i2c-" prefix for ACPI enumerated I2C slaves. For them device name "x-00yz" becomes "i2c-INTABCD:ij" after this patch.

Signed-off-by Jarkko Nikula <jarkko.nikula-VuQAYsv1563Yd54FQh9/[email protected]>

v4:
This version adds and uses acpi_dev_name() accessor instead defining struct acpi_device stub for !CONFIG_ACPI

v3:
Third version of the set that changes I2C and SPI slave device names to be generated from stable ACPI device names on ACPI 5 based systems instead of using bus numbers which could change.

Slave device name change goes as

	"x-00yz" -> "i2c-INTABCD:ij"
	"spix.y" -> "spi-INTABCD:ij"

This version is actually a bit cleaner thanks to Rafael's "ACPI / driver core: Store an ACPI device pointer in struct acpi_dev_node" in git://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm

Set goes on top of linux-pm/linux-next due dependency to 1/3. As we are now in merge-window I'm fine if 1/3 goes in first and I queue 2-3/3 later to i2c and spi trees.

Jarkko Nikula (3):
ACPI: Provide acpi_dev_name accessor for struct acpi_device device
name
i2c: Use stable dev_name for ACPI enumerated I2C slaves
spi: Use stable dev_name for ACPI enumerated SPI slaves

Current I2C adapter id - client address "x-00yy" based device naming scheme is not always stable enough to be used in name based matching, for instance within ALSA SoC subsystem.

This is problematic in PC kind of platforms where I2C adapter numbers can change due variable amount of bus controllers, probe order, add-on cards or just because of BIOS settings.

This patch addresses the problem by using the ACPI device name with "i2c-" prefix for ACPI enumerated I2C slaves. For them device name "x-00yz" becomes "i2c-INTABCD:ij" after this patch.

Signed-off-by Jarkko Nikula <jarkko.nikula-VuQAYsv1563Yd54FQh9/[email protected]>