[alsa-devel] [RFC 0/2] I2C and SPI dev_name change for ACPI enumerated slaves
Hi
We've run into problem of changing I2C device names while developing ALSA SoC drivers for x86 based systems. Changing device names makes more difficult to match devices by their name. Which is what we use within ASoC subsystem.
These changing names comes from changing adapter/bus numbering which could occur due variable amount of bus controllers, probe order, add-on cards or different BIOS settings.
Patches here try to solve the problem on ACPI 5 based systems by using stable ACPI device name with a "i2c-"/"spi-" prefix for I2C/SPI slave device names.
Both patches are independent from each other and can go through their own subsystems.
Jarkko Nikula (2): 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 | 21 ++++++++++++++++++--- 2 files changed, 38 insertions(+), 7 deletions(-)
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@linux.intel.com --- 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(-)
diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c index 111b2c6..8d6f3e5 100644 --- a/drivers/i2c/i2c-core.c +++ b/drivers/i2c/i2c-core.c @@ -613,6 +613,25 @@ void i2c_unlock_adapter(struct i2c_adapter *adapter) } EXPORT_SYMBOL_GPL(i2c_unlock_adapter);
+static void i2c_dev_set_name(struct i2c_adapter *adap, + struct i2c_client *client) +{ +#if IS_ENABLED(CONFIG_ACPI) + if (ACPI_HANDLE(&client->dev)) { + 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; ACPI_HANDLE_SET(&client->dev, info->acpi_node.handle);
- /* 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_dev_set_name(adap, client); status = device_register(&client->dev); if (status) goto out_err;
On Friday, October 25, 2013 03:18:59 PM Jarkko Nikula wrote:
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@linux.intel.com
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(-)
diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c index 111b2c6..8d6f3e5 100644 --- a/drivers/i2c/i2c-core.c +++ b/drivers/i2c/i2c-core.c @@ -613,6 +613,25 @@ void i2c_unlock_adapter(struct i2c_adapter *adapter) } EXPORT_SYMBOL_GPL(i2c_unlock_adapter);
+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.
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; ACPI_HANDLE_SET(&client->dev, info->acpi_node.handle);
- /* 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_dev_set_name(adap, client); status = device_register(&client->dev); if (status) goto out_err;
Thanks!
Hi
On 10/25/2013 03:52 PM, Rafael J. Wysocki wrote:
+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.
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.
On Friday, October 25, 2013 03:55:29 PM Jarkko Nikula wrote:
Hi
On 10/25/2013 03:52 PM, Rafael J. Wysocki wrote:
+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.
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?
Rafael
On 10/25/2013 04:18 PM, Rafael J. Wysocki wrote:
On Friday, October 25, 2013 03:55:29 PM Jarkko Nikula wrote:
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.
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.
On Friday, October 25, 2013 04:30:23 PM Jarkko Nikula wrote:
On 10/25/2013 04:18 PM, Rafael J. Wysocki wrote:
On Friday, October 25, 2013 03:55:29 PM Jarkko Nikula wrote:
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.
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.
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.
Thanks!
Hi Rafael
On 10/25/2013 05:08 PM, Rafael J. Wysocki wrote:
On Friday, October 25, 2013 04:30:23 PM Jarkko Nikula wrote:
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).
On Mon, Oct 28, 2013 at 03:15:25PM +0200, Jarkko Nikula wrote:
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.
You could also have inline accessor functions which can be stubbed out when not needed.
On Monday, October 28, 2013 03:15:25 PM Jarkko Nikula wrote:
Hi Rafael
On 10/25/2013 05:08 PM, Rafael J. Wysocki wrote:
On Friday, October 25, 2013 04:30:23 PM Jarkko Nikula wrote:
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?
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 ->
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).
diff --git a/include/acpi/acpi_bus.h b/include/acpi/acpi_bus.h index d901982..7ab7870 100644 --- a/include/acpi/acpi_bus.h +++ b/include/acpi/acpi_bus.h @@ -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"
@@ -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;@@ -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.
#endif /* CONFIG_ACPI */
Thanks!
On Friday, November 01, 2013 01:08:47 AM Rafael J. Wysocki wrote:
On Monday, October 28, 2013 03:15:25 PM Jarkko Nikula wrote:
Hi Rafael
On 10/25/2013 05:08 PM, Rafael J. Wysocki wrote:
On Friday, October 25, 2013 04:30:23 PM Jarkko Nikula wrote:
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?
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 ->
OK, so I *think* what happens is that the compiler checks if the particular symbols make sense before trying to optimize out the whole block.
So the patch below modulo the return value of the acpi_bus_get_device() stub would be fine by me.
Thanks!
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).
diff --git a/include/acpi/acpi_bus.h b/include/acpi/acpi_bus.h index d901982..7ab7870 100644 --- a/include/acpi/acpi_bus.h +++ b/include/acpi/acpi_bus.h @@ -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"
@@ -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;@@ -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.
#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.
This can be problem in PC kind of platforms if there are changes in SPI bus configuration, amount of busses or probe order.
This patch addresses the problem by using the ACPI device name with "spi-" prefix for ACPI enumerated SPI slave. For them device name "spix.y" becomes "spi-INTABCD:ij".
Signed-off-by: Jarkko Nikula jarkko.nikula@linux.intel.com --- "may not be" and "This can be problem" since I don't know how likely is to have changes in SPI bus configuration in PC kind of devices. I assume yes as this is already problem with I2C devices. --- drivers/spi/spi.c | 21 ++++++++++++++++++--- 1 file changed, 18 insertions(+), 3 deletions(-)
diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c index 8d05acc..2ca997f 100644 --- a/drivers/spi/spi.c +++ b/drivers/spi/spi.c @@ -345,6 +345,23 @@ struct spi_device *spi_alloc_device(struct spi_master *master) } EXPORT_SYMBOL_GPL(spi_alloc_device);
+void spi_dev_set_name(struct spi_device *spi) +{ +#if IS_ENABLED(CONFIG_ACPI) + if (ACPI_HANDLE(&spi->dev)) { + struct acpi_device *adev; + if (!acpi_bus_get_device(ACPI_HANDLE(&spi->dev), &adev)) { + dev_set_name(&spi->dev, "spi-%s", + dev_name(&adev->dev)); + return; + } + } +#endif + + dev_set_name(&spi->dev, "%s.%u", dev_name(&spi->master->dev), + spi->chip_select); +} + /** * spi_add_device - Add spi_device allocated with spi_alloc_device * @spi: spi_device to register @@ -371,9 +388,7 @@ int spi_add_device(struct spi_device *spi) }
/* Set the bus ID string */ - dev_set_name(&spi->dev, "%s.%u", dev_name(&spi->master->dev), - spi->chip_select); - + spi_dev_set_name(spi);
/* We need to make sure there's no other device with this * chipselect **BEFORE** we call setup(), else we'll trash
On Fri, Oct 25, 2013 at 03:19:00PM +0300, Jarkko Nikula wrote:
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?
On Friday, October 25, 2013 01:59:09 PM Mark Brown wrote:
On Fri, Oct 25, 2013 at 03:19:00PM +0300, Jarkko Nikula wrote:
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?
I think yes in general, but I'd like to understand why the ACPI_HANDLE() thing doesn't work as expected.
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.htm...
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(-)
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 jarkko.nikula@linux.intel.com --- include/acpi/acpi_bus.h | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-)
diff --git a/include/acpi/acpi_bus.h b/include/acpi/acpi_bus.h index 15100f6..232b37d3 100644 --- a/include/acpi/acpi_bus.h +++ b/include/acpi/acpi_bus.h @@ -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 */
On Friday, November 01, 2013 02:35:54 PM Jarkko Nikula wrote:
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 jarkko.nikula@linux.intel.com
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.
include/acpi/acpi_bus.h | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-)
diff --git a/include/acpi/acpi_bus.h b/include/acpi/acpi_bus.h index 15100f6..232b37d3 100644 --- a/include/acpi/acpi_bus.h +++ b/include/acpi/acpi_bus.h @@ -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 */
On Saturday, November 02, 2013 11:18:31 PM Rafael J. Wysocki wrote:
On Friday, November 01, 2013 02:35:54 PM Jarkko Nikula wrote:
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 jarkko.nikula@linux.intel.com
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.
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.
Thanks, Rafael
--- From: Rafael J. Wysocki rafael.j.wysocki@intel.com Subject: ACPI: Define struct acpi_device for CONFIG_ACPI unset
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.
Define a stub struct acpi_device for CONFIG_ACPI unset containing dev as the only member and a stub acpi_bus_get_device() always returning an error code.
Signed-off-by: Rafael J. Wysocki rafael.j.wysocki@intel.com --- include/linux/acpi.h | 10 ++++++++++ 1 file changed, 10 insertions(+)
Index: linux-pm/include/linux/acpi.h =================================================================== --- linux-pm.orig/include/linux/acpi.h +++ linux-pm/include/linux/acpi.h @@ -472,7 +472,17 @@ static inline bool acpi_driver_match_dev }
#define ACPI_PTR(_ptr) (NULL) +typedef void * acpi_handle;
+struct acpi_device { + struct device dev; +}; + +static inline int acpi_bus_get_device(acpi_handle handle, + struct acpi_device **device) +{ + return -ENODEV; +} #endif /* !CONFIG_ACPI */
#ifdef CONFIG_ACPI
On Mon, Nov 04, 2013 at 02:00:45AM +0100, Rafael J. Wysocki wrote:
On Saturday, November 02, 2013 11:18:31 PM Rafael J. Wysocki wrote:
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.
Generally, it is a bad idea to #include acpi_bus.h for !CONFIG_ACPI.
Ouch, indeed.
The appended patch works for me, sorry for stealing part of your changelog.
No problem. Thanks for fixing this up.
On Monday, November 04, 2013 11:15:59 PM Jarkko Nikula wrote:
On Mon, Nov 04, 2013 at 02:00:45AM +0100, Rafael J. Wysocki wrote:
On Saturday, November 02, 2013 11:18:31 PM Rafael J. Wysocki wrote:
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.
Generally, it is a bad idea to #include acpi_bus.h for !CONFIG_ACPI.
Ouch, indeed.
The appended patch works for me, sorry for stealing part of your changelog.
No problem. Thanks for fixing this up.
Well, unfortunately, this breaks builds for allnoconfig, because <acpi/acpi.h> is included directly from some header files (not under a CONFIG_ACPI ifdef) and duplicates the acpi_handle typedef.
So, we'll need to go through the headers, see which ones do that and replace <acpi/acpi.h> with <linux/acpi.h> (along with adding the stub definitions for !CONFIG_ACPI).
I'm deferring the patchset for after 3.13-rc1 for this reason.
Thanks!
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@linux.intel.com --- drivers/i2c/i2c-core.c | 24 ++++++++++++++++++++---- 1 file changed, 20 insertions(+), 4 deletions(-)
diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c index 75ba860..9126c2e 100644 --- a/drivers/i2c/i2c-core.c +++ b/drivers/i2c/i2c-core.c @@ -48,6 +48,7 @@ #include <linux/rwsem.h> #include <linux/pm_runtime.h> #include <linux/acpi.h> +#include <acpi/acpi_bus.h> #include <asm/uaccess.h>
#include "i2c-core.h" @@ -618,6 +619,24 @@ void i2c_unlock_adapter(struct i2c_adapter *adapter) } EXPORT_SYMBOL_GPL(i2c_unlock_adapter);
+static void i2c_dev_set_name(struct i2c_adapter *adap, + struct i2c_client *client) +{ + if (ACPI_HANDLE(&client->dev)) { + 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; + } + } + + /* 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 @@ -676,10 +695,7 @@ i2c_new_device(struct i2c_adapter *adap, struct i2c_board_info const *info) client->dev.of_node = info->of_node; ACPI_HANDLE_SET(&client->dev, info->acpi_node.handle);
- /* 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_dev_set_name(adap, client); status = device_register(&client->dev); if (status) goto out_err;
On Fri, Nov 01, 2013 at 02:35:55PM +0200, Jarkko Nikula wrote:
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@linux.intel.com
Acked-by: Wolfram Sang wsa@the-dreams.de
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.
This can be problem in PC kind of platforms if there are changes in SPI bus configuration, amount of busses or probe order.
This patch addresses the problem by using the ACPI device name with "spi-" prefix for ACPI enumerated SPI slave. For them device name "spix.y" becomes "spi-INTABCD:ij".
Signed-off-by: Jarkko Nikula jarkko.nikula@linux.intel.com --- drivers/spi/spi.c | 20 +++++++++++++++++--- 1 file changed, 17 insertions(+), 3 deletions(-)
diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c index 740f9dd..4c0c801 100644 --- a/drivers/spi/spi.c +++ b/drivers/spi/spi.c @@ -38,6 +38,7 @@ #include <linux/kthread.h> #include <linux/ioport.h> #include <linux/acpi.h> +#include <acpi/acpi_bus.h>
static void spidev_release(struct device *dev) { @@ -352,6 +353,21 @@ struct spi_device *spi_alloc_device(struct spi_master *master) } EXPORT_SYMBOL_GPL(spi_alloc_device);
+void spi_dev_set_name(struct spi_device *spi) +{ + if (ACPI_HANDLE(&spi->dev)) { + struct acpi_device *adev; + if (!acpi_bus_get_device(ACPI_HANDLE(&spi->dev), &adev)) { + dev_set_name(&spi->dev, "spi-%s", + dev_name(&adev->dev)); + return; + } + } + + dev_set_name(&spi->dev, "%s.%u", dev_name(&spi->master->dev), + spi->chip_select); +} + /** * spi_add_device - Add spi_device allocated with spi_alloc_device * @spi: spi_device to register @@ -378,9 +394,7 @@ int spi_add_device(struct spi_device *spi) }
/* Set the bus ID string */ - dev_set_name(&spi->dev, "%s.%u", dev_name(&spi->master->dev), - spi->chip_select); - + spi_dev_set_name(spi);
/* We need to make sure there's no other device with this * chipselect **BEFORE** we call setup(), else we'll trash
On Fri, Nov 01, 2013 at 02:35:56PM +0200, Jarkko Nikula wrote:
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.
Acked-by: Mark Brown broonie@linaro.org
On Friday, November 01, 2013 02:35:53 PM Jarkko Nikula wrote:
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.htm...
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(-)
Looks good to me. If there are no objections, I can merge these through my tree.
Thanks!
On Fri, Nov 01, 2013 at 02:18:06PM +0100, Rafael J. Wysocki wrote:
On Friday, November 01, 2013 02:35:53 PM Jarkko Nikula wrote:
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.htm...
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(-)
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.
On 11/01/2013 03:20 PM, Wolfram Sang wrote:
On Fri, Nov 01, 2013 at 02:18:06PM +0100, Rafael J. Wysocki wrote:
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
On Friday, November 01, 2013 03:44:38 PM Jarkko Nikula wrote:
On 11/01/2013 03:20 PM, Wolfram Sang wrote:
On Fri, Nov 01, 2013 at 02:18:06PM +0100, Rafael J. Wysocki wrote:
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".
Thanks!
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".
OK.
Hi
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 struct acpi_device stub for !CONFIG_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 | 21 +++++++++++++++++---- drivers/spi/spi.c | 17 ++++++++++++++--- include/linux/acpi.h | 4 ++++ 3 files changed, 35 insertions(+), 7 deletions(-)
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 jarkko.nikula@linux.intel.com --- 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(+)
diff --git a/include/linux/acpi.h b/include/linux/acpi.h index 0da49ed..df444e1 100644 --- a/include/linux/acpi.h +++ b/include/linux/acpi.h @@ -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)
At Thu, 14 Nov 2013 11:00:59 +0200, Jarkko Nikula wrote:
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 jarkko.nikula@linux.intel.com
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?
Takashi
include/linux/acpi.h | 4 ++++ 1 file changed, 4 insertions(+)
diff --git a/include/linux/acpi.h b/include/linux/acpi.h index 0da49ed..df444e1 100644 --- a/include/linux/acpi.h +++ b/include/linux/acpi.h @@ -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)
1.8.4.2
Alsa-devel mailing list Alsa-devel@alsa-project.org http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
On 11/14/2013 12:03 PM, Takashi Iwai wrote:
At Thu, 14 Nov 2013 11:00:59 +0200, Jarkko Nikula wrote:
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 jarkko.nikula@linux.intel.com
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?
It is included unconditionally in include/linux/acpi.h.
Your acpi_dev_name() proposal may be good enough for now as adev->dev access in generic code (now under CONFIG_ACPI) seems to be anyway around dev_name, using only pointer to struct acpi_device or has more things that prevents immediate #ifdef CONFIG_ACPI removal there.
But one problem at time. I'll cook a version with acpi_dev_name.
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@linux.intel.com --- drivers/i2c/i2c-core.c | 21 +++++++++++++++++---- 1 file changed, 17 insertions(+), 4 deletions(-)
diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c index f74af33..b8f2c32 100644 --- a/drivers/i2c/i2c-core.c +++ b/drivers/i2c/i2c-core.c @@ -618,6 +618,22 @@ void i2c_unlock_adapter(struct i2c_adapter *adapter) } EXPORT_SYMBOL_GPL(i2c_unlock_adapter);
+static void i2c_dev_set_name(struct i2c_adapter *adap, + struct i2c_client *client) +{ + struct acpi_device *adev = ACPI_COMPANION(&client->dev); + + if (adev) { + dev_set_name(&client->dev, "i2c-%s", dev_name(&adev->dev)); + return; + } + + /* 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 @@ -676,10 +692,7 @@ i2c_new_device(struct i2c_adapter *adap, struct i2c_board_info const *info) client->dev.of_node = info->of_node; ACPI_COMPANION_SET(&client->dev, info->acpi_node.companion);
- /* 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_dev_set_name(adap, client); status = device_register(&client->dev); if (status) goto out_err;
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.
This can be problem in PC kind of platforms if there are changes in SPI bus configuration, amount of busses or probe order.
This patch addresses the problem by using the ACPI device name with "spi-" prefix for ACPI enumerated SPI slave. For them device name "spix.y" becomes "spi-INTABCD:ij".
Signed-off-by: Jarkko Nikula jarkko.nikula@linux.intel.com --- drivers/spi/spi.c | 17 ++++++++++++++--- 1 file changed, 14 insertions(+), 3 deletions(-)
diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c index af4f971..d9140bb 100644 --- a/drivers/spi/spi.c +++ b/drivers/spi/spi.c @@ -352,6 +352,19 @@ struct spi_device *spi_alloc_device(struct spi_master *master) } EXPORT_SYMBOL_GPL(spi_alloc_device);
+static void spi_dev_set_name(struct spi_device *spi) +{ + struct acpi_device *adev = ACPI_COMPANION(&spi->dev); + + if (adev) { + dev_set_name(&spi->dev, "spi-%s", dev_name(&adev->dev)); + return; + } + + dev_set_name(&spi->dev, "%s.%u", dev_name(&spi->master->dev), + spi->chip_select); +} + /** * spi_add_device - Add spi_device allocated with spi_alloc_device * @spi: spi_device to register @@ -378,9 +391,7 @@ int spi_add_device(struct spi_device *spi) }
/* Set the bus ID string */ - dev_set_name(&spi->dev, "%s.%u", dev_name(&spi->master->dev), - spi->chip_select); - + spi_dev_set_name(spi);
/* We need to make sure there's no other device with this * chipselect **BEFORE** we call setup(), else we'll trash
On Thu, Nov 14, 2013 at 11:01:01AM +0200, Jarkko Nikula wrote:
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.
Acked-by: Mark Brown broonie@linaro.org
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
drivers/i2c/i2c-core.c | 21 +++++++++++++++++---- drivers/spi/spi.c | 17 ++++++++++++++--- include/linux/acpi.h | 10 ++++++++++ 3 files changed, 41 insertions(+), 7 deletions(-)
struct acpi_device fields are only available when CONFIG_ACPI is set. We may find use for dev_name(&adev->dev) in generic code that is build also without CONFIG_ACPI is set but currently this requires #ifdef CONFIG_ACPI churn.
Provide here an accessor that returns dev_name of embedded struct device dev in struct acpi_device or NULL depending on CONFIG_ACPI setting.
Signed-off-by: Jarkko Nikula jarkko.nikula@linux.intel.com --- include/linux/acpi.h | 10 ++++++++++ 1 file changed, 10 insertions(+)
diff --git a/include/linux/acpi.h b/include/linux/acpi.h index 0da49ed..844e7ff 100644 --- a/include/linux/acpi.h +++ b/include/linux/acpi.h @@ -53,6 +53,11 @@ static inline acpi_handle acpi_device_handle(struct acpi_device *adev) #define ACPI_COMPANION_SET(dev, adev) ACPI_COMPANION(dev) = (adev) #define ACPI_HANDLE(dev) acpi_device_handle(ACPI_COMPANION(dev))
+static inline const char *acpi_dev_name(struct acpi_device *adev) +{ + return dev_name(&adev->dev); +} + enum acpi_irq_model_id { ACPI_IRQ_MODEL_PIC = 0, ACPI_IRQ_MODEL_IOAPIC, @@ -420,6 +425,11 @@ static inline bool acpi_driver_match_device(struct device *dev, #define ACPI_COMPANION_SET(dev, adev) do { } while (0) #define ACPI_HANDLE(dev) (NULL)
+static inline const char *acpi_dev_name(struct acpi_device *adev) +{ + return NULL; +} + static inline void acpi_early_init(void) { }
static inline int early_acpi_boot_init(void)
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@linux.intel.com --- drivers/i2c/i2c-core.c | 21 +++++++++++++++++---- 1 file changed, 17 insertions(+), 4 deletions(-)
diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c index f74af33..bc9a294 100644 --- a/drivers/i2c/i2c-core.c +++ b/drivers/i2c/i2c-core.c @@ -618,6 +618,22 @@ void i2c_unlock_adapter(struct i2c_adapter *adapter) } EXPORT_SYMBOL_GPL(i2c_unlock_adapter);
+static void i2c_dev_set_name(struct i2c_adapter *adap, + struct i2c_client *client) +{ + struct acpi_device *adev = ACPI_COMPANION(&client->dev); + + if (adev) { + dev_set_name(&client->dev, "i2c-%s", acpi_dev_name(adev)); + return; + } + + /* 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 @@ -676,10 +692,7 @@ i2c_new_device(struct i2c_adapter *adap, struct i2c_board_info const *info) client->dev.of_node = info->of_node; ACPI_COMPANION_SET(&client->dev, info->acpi_node.companion);
- /* 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_dev_set_name(adap, client); status = device_register(&client->dev); if (status) goto out_err;
On Thu, Nov 14, 2013 at 02:03:52PM +0200, Jarkko Nikula wrote:
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@linux.intel.com
Acked-by: Wolfram Sang wsa@the-dreams.de
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.
This can be problem in PC kind of platforms if there are changes in SPI bus configuration, amount of busses or probe order.
This patch addresses the problem by using the ACPI device name with "spi-" prefix for ACPI enumerated SPI slave. For them device name "spix.y" becomes "spi-INTABCD:ij".
Signed-off-by: Jarkko Nikula jarkko.nikula@linux.intel.com Acked-by: Mark Brown broonie@linaro.org --- Mark's ack added as diff between v3 and v4 is just: - dev_set_name(&spi->dev, "spi-%s", dev_name(&adev->dev)); + dev_set_name(&spi->dev, "spi-%s", acpi_dev_name(adev)); --- drivers/spi/spi.c | 17 ++++++++++++++--- 1 file changed, 14 insertions(+), 3 deletions(-)
diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c index af4f971..be619e0 100644 --- a/drivers/spi/spi.c +++ b/drivers/spi/spi.c @@ -352,6 +352,19 @@ struct spi_device *spi_alloc_device(struct spi_master *master) } EXPORT_SYMBOL_GPL(spi_alloc_device);
+static void spi_dev_set_name(struct spi_device *spi) +{ + struct acpi_device *adev = ACPI_COMPANION(&spi->dev); + + if (adev) { + dev_set_name(&spi->dev, "spi-%s", acpi_dev_name(adev)); + return; + } + + dev_set_name(&spi->dev, "%s.%u", dev_name(&spi->master->dev), + spi->chip_select); +} + /** * spi_add_device - Add spi_device allocated with spi_alloc_device * @spi: spi_device to register @@ -378,9 +391,7 @@ int spi_add_device(struct spi_device *spi) }
/* Set the bus ID string */ - dev_set_name(&spi->dev, "%s.%u", dev_name(&spi->master->dev), - spi->chip_select); - + spi_dev_set_name(spi);
/* We need to make sure there's no other device with this * chipselect **BEFORE** we call setup(), else we'll trash
On Thursday, November 14, 2013 02:03:50 PM Jarkko Nikula wrote:
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
All three queued up for the next ACPI pull request, thanks!
participants (5)
-
Jarkko Nikula -
Mark Brown -
Rafael J. Wysocki -
Takashi Iwai -
Wolfram Sang