* [PATCH v2 1/6] spi: Export acpi_spi_find_controller_by_adev to be used externally
2021-12-10 15:40 [PATCH v2 0/6] Support Spi in i2c-multi-instantiate driver Stefan Binding
@ 2021-12-10 15:40 ` Stefan Binding
2021-12-10 15:40 ` [PATCH v2 2/6] spi: Make spi_alloc_device and spi_add_device public again Stefan Binding
` (6 subsequent siblings)
7 siblings, 0 replies; 17+ messages in thread
From: Stefan Binding @ 2021-12-10 15:40 UTC (permalink / raw)
To: Mark Brown, Rafael J . Wysocki, Len Brown, Hans de Goede, Mark Gross
Cc: linux-kernel, linux-spi, linux-acpi, platform-driver-x86,
patches, Stefan Binding
This will be used by bus-multi-instantiate to support spi.
Signed-off-by: Stefan Binding <sbinding@opensource.cirrus.com>
---
drivers/spi/spi.c | 3 ++-
include/linux/spi/spi.h | 10 ++++++++++
2 files changed, 12 insertions(+), 1 deletion(-)
diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
index 8726309b3eaf..9495f776e53c 100644
--- a/drivers/spi/spi.c
+++ b/drivers/spi/spi.c
@@ -4200,7 +4200,7 @@ static int spi_acpi_controller_match(struct device *dev, const void *data)
return ACPI_COMPANION(dev->parent) == data;
}
-static struct spi_controller *acpi_spi_find_controller_by_adev(struct acpi_device *adev)
+struct spi_controller *acpi_spi_find_controller_by_adev(struct acpi_device *adev)
{
struct device *dev;
@@ -4214,6 +4214,7 @@ static struct spi_controller *acpi_spi_find_controller_by_adev(struct acpi_devic
return container_of(dev, struct spi_controller, dev);
}
+EXPORT_SYMBOL_GPL(acpi_spi_find_controller_by_adev);
static struct spi_device *acpi_spi_find_device_by_adev(struct acpi_device *adev)
{
diff --git a/include/linux/spi/spi.h b/include/linux/spi/spi.h
index eb7ac8a1e03c..8c2978853573 100644
--- a/include/linux/spi/spi.h
+++ b/include/linux/spi/spi.h
@@ -17,6 +17,7 @@
#include <linux/ptp_clock_kernel.h>
#include <uapi/linux/spi/spi.h>
+#include <linux/acpi.h>
struct dma_chan;
struct software_node;
@@ -759,6 +760,15 @@ extern int devm_spi_register_controller(struct device *dev,
struct spi_controller *ctlr);
extern void spi_unregister_controller(struct spi_controller *ctlr);
+#if IS_ENABLED(CONFIG_ACPI)
+extern struct spi_controller *acpi_spi_find_controller_by_adev(struct acpi_device *adev);
+#else
+static inline struct spi_controller *acpi_spi_find_controller_by_adev(struct acpi_device *adev)
+{
+ return ERR_PTR(-EOPNOTSUPP);
+}
+#endif
+
/*
* SPI resource management while processing a SPI message
*/
--
2.25.1
^ permalink raw reply related [flat|nested] 17+ messages in thread* [PATCH v2 2/6] spi: Make spi_alloc_device and spi_add_device public again
2021-12-10 15:40 [PATCH v2 0/6] Support Spi in i2c-multi-instantiate driver Stefan Binding
2021-12-10 15:40 ` [PATCH v2 1/6] spi: Export acpi_spi_find_controller_by_adev to be used externally Stefan Binding
@ 2021-12-10 15:40 ` Stefan Binding
2021-12-10 15:40 ` [PATCH v2 3/6] platform/x86: i2c-multi-instantiate: Move it to drivers/acpi folder Stefan Binding
` (5 subsequent siblings)
7 siblings, 0 replies; 17+ messages in thread
From: Stefan Binding @ 2021-12-10 15:40 UTC (permalink / raw)
To: Mark Brown, Rafael J . Wysocki, Len Brown, Hans de Goede, Mark Gross
Cc: linux-kernel, linux-spi, linux-acpi, platform-driver-x86,
patches, Stefan Binding
This functions were previously made private since they
were not used. However, these functions will be needed
again.
Partial revert of commit da21fde0fdb3
("spi: Make several public functions private to spi.c")
Signed-off-by: Stefan Binding <sbinding@opensource.cirrus.com>
---
drivers/spi/spi.c | 6 ++++--
include/linux/spi/spi.h | 12 ++++++++++++
2 files changed, 16 insertions(+), 2 deletions(-)
diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
index 9495f776e53c..33ed5693560d 100644
--- a/drivers/spi/spi.c
+++ b/drivers/spi/spi.c
@@ -531,7 +531,7 @@ static DEFINE_MUTEX(board_lock);
*
* Return: a pointer to the new device, or NULL.
*/
-static struct spi_device *spi_alloc_device(struct spi_controller *ctlr)
+struct spi_device *spi_alloc_device(struct spi_controller *ctlr)
{
struct spi_device *spi;
@@ -556,6 +556,7 @@ static struct spi_device *spi_alloc_device(struct spi_controller *ctlr)
device_initialize(&spi->dev);
return spi;
}
+EXPORT_SYMBOL_GPL(spi_alloc_device);
static void spi_dev_set_name(struct spi_device *spi)
{
@@ -651,7 +652,7 @@ static int __spi_add_device(struct spi_device *spi)
*
* Return: 0 on success; negative errno on failure
*/
-static int spi_add_device(struct spi_device *spi)
+int spi_add_device(struct spi_device *spi)
{
struct spi_controller *ctlr = spi->controller;
struct device *dev = ctlr->dev.parent;
@@ -672,6 +673,7 @@ static int spi_add_device(struct spi_device *spi)
mutex_unlock(&ctlr->add_lock);
return status;
}
+EXPORT_SYMBOL_GPL(spi_add_device);
static int spi_add_device_locked(struct spi_device *spi)
{
diff --git a/include/linux/spi/spi.h b/include/linux/spi/spi.h
index 8c2978853573..d70e66c712d0 100644
--- a/include/linux/spi/spi.h
+++ b/include/linux/spi/spi.h
@@ -1462,7 +1462,19 @@ spi_register_board_info(struct spi_board_info const *info, unsigned n)
* use spi_new_device() to describe each device. You can also call
* spi_unregister_device() to start making that device vanish, but
* normally that would be handled by spi_unregister_controller().
+ *
+ * You can also use spi_alloc_device() and spi_add_device() to use a two
+ * stage registration sequence for each spi_device. This gives the caller
+ * some more control over the spi_device structure before it is registered,
+ * but requires that caller to initialize fields that would otherwise
+ * be defined using the board info.
*/
+extern struct spi_device *
+spi_alloc_device(struct spi_controller *ctlr);
+
+extern int
+spi_add_device(struct spi_device *spi);
+
extern struct spi_device *
spi_new_device(struct spi_controller *, struct spi_board_info *);
--
2.25.1
^ permalink raw reply related [flat|nested] 17+ messages in thread* [PATCH v2 3/6] platform/x86: i2c-multi-instantiate: Move it to drivers/acpi folder
2021-12-10 15:40 [PATCH v2 0/6] Support Spi in i2c-multi-instantiate driver Stefan Binding
2021-12-10 15:40 ` [PATCH v2 1/6] spi: Export acpi_spi_find_controller_by_adev to be used externally Stefan Binding
2021-12-10 15:40 ` [PATCH v2 2/6] spi: Make spi_alloc_device and spi_add_device public again Stefan Binding
@ 2021-12-10 15:40 ` Stefan Binding
2021-12-10 17:59 ` Mark Brown
2021-12-10 15:40 ` [PATCH v2 4/6] ACPI: i2c-multi-instantiate: Rename it for a generic bus driver name Stefan Binding
` (4 subsequent siblings)
7 siblings, 1 reply; 17+ messages in thread
From: Stefan Binding @ 2021-12-10 15:40 UTC (permalink / raw)
To: Mark Brown, Rafael J . Wysocki, Len Brown, Hans de Goede, Mark Gross
Cc: linux-kernel, linux-spi, linux-acpi, platform-driver-x86,
patches, Lucas Tanure
From: Lucas Tanure <tanureal@opensource.cirrus.com>
Moving I2C multi instantiate driver to drivers/acpi folder for
upcoming conversion into a generic bus multi instantiate
driver for SPI and I2C
Signed-off-by: Lucas Tanure <tanureal@opensource.cirrus.com>
---
MAINTAINERS | 2 +-
drivers/acpi/Kconfig | 11 +++++++++++
drivers/acpi/Makefile | 1 +
.../{platform/x86 => acpi}/i2c-multi-instantiate.c | 0
drivers/acpi/scan.c | 2 +-
drivers/platform/x86/Kconfig | 11 -----------
drivers/platform/x86/Makefile | 1 -
7 files changed, 14 insertions(+), 14 deletions(-)
rename drivers/{platform/x86 => acpi}/i2c-multi-instantiate.c (100%)
diff --git a/MAINTAINERS b/MAINTAINERS
index cd55b83878e0..25e056854772 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -391,7 +391,7 @@ ACPI I2C MULTI INSTANTIATE DRIVER
M: Hans de Goede <hdegoede@redhat.com>
L: platform-driver-x86@vger.kernel.org
S: Maintained
-F: drivers/platform/x86/i2c-multi-instantiate.c
+F: drivers/acpi/i2c-multi-instantiate.c
ACPI PCC(Platform Communication Channel) MAILBOX DRIVER
M: Sudeep Holla <sudeep.holla@arm.com>
diff --git a/drivers/acpi/Kconfig b/drivers/acpi/Kconfig
index c97ee0cfe26e..6200d13fa97b 100644
--- a/drivers/acpi/Kconfig
+++ b/drivers/acpi/Kconfig
@@ -295,6 +295,17 @@ config ACPI_PROCESSOR
To compile this driver as a module, choose M here:
the module will be called processor.
+config ACPI_I2C_MULTI_INST
+ tristate "I2C multi instantiate pseudo device driver"
+ depends on I2C
+ help
+ Some ACPI-based systems list multiple i2c-devices in a single ACPI
+ firmware-node. This driver will instantiate separate i2c-clients
+ for each device in the firmware-node.
+
+ To compile this driver as a module, choose M here: the module
+ will be called i2c-multi-instantiate.
+
config ACPI_IPMI
tristate "IPMI"
depends on IPMI_HANDLER
diff --git a/drivers/acpi/Makefile b/drivers/acpi/Makefile
index 3018714e87d9..98d700d55960 100644
--- a/drivers/acpi/Makefile
+++ b/drivers/acpi/Makefile
@@ -102,6 +102,7 @@ obj-$(CONFIG_ACPI_CPPC_LIB) += cppc_acpi.o
obj-$(CONFIG_ACPI_SPCR_TABLE) += spcr.o
obj-$(CONFIG_ACPI_DEBUGGER_USER) += acpi_dbg.o
obj-$(CONFIG_ACPI_PPTT) += pptt.o
+obj-$(CONFIG_ACPI_I2C_MULTI_INST) += i2c-multi-instantiate.o
# processor has its own "processor." module_param namespace
processor-y := processor_driver.o
diff --git a/drivers/platform/x86/i2c-multi-instantiate.c b/drivers/acpi/i2c-multi-instantiate.c
similarity index 100%
rename from drivers/platform/x86/i2c-multi-instantiate.c
rename to drivers/acpi/i2c-multi-instantiate.c
diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
index 5991dddbc9ce..e5c81b3df09c 100644
--- a/drivers/acpi/scan.c
+++ b/drivers/acpi/scan.c
@@ -1700,7 +1700,7 @@ static bool acpi_device_enumeration_by_parent(struct acpi_device *device)
* must be instantiated for each, each with its own i2c_device_id.
* Normally we only instantiate an i2c-client for the first resource,
* using the ACPI HID as id. These special cases are handled by the
- * drivers/platform/x86/i2c-multi-instantiate.c driver, which knows
+ * drivers/acpi/i2c-multi-instantiate.c driver, which knows
* which i2c_device_id to use for each resource.
*/
static const struct acpi_device_id i2c_multi_instantiate_ids[] = {
diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
index 97e87628eb35..1596c8e5bc9b 100644
--- a/drivers/platform/x86/Kconfig
+++ b/drivers/platform/x86/Kconfig
@@ -958,17 +958,6 @@ config TOPSTAR_LAPTOP
If you have a Topstar laptop, say Y or M here.
-config I2C_MULTI_INSTANTIATE
- tristate "I2C multi instantiate pseudo device driver"
- depends on I2C && ACPI
- help
- Some ACPI-based systems list multiple i2c-devices in a single ACPI
- firmware-node. This driver will instantiate separate i2c-clients
- for each device in the firmware-node.
-
- To compile this driver as a module, choose M here: the module
- will be called i2c-multi-instantiate.
-
config MLX_PLATFORM
tristate "Mellanox Technologies platform support"
depends on I2C && REGMAP
diff --git a/drivers/platform/x86/Makefile b/drivers/platform/x86/Makefile
index 219478061683..dc075a036874 100644
--- a/drivers/platform/x86/Makefile
+++ b/drivers/platform/x86/Makefile
@@ -108,7 +108,6 @@ obj-$(CONFIG_TOPSTAR_LAPTOP) += topstar-laptop.o
# Platform drivers
obj-$(CONFIG_FW_ATTR_CLASS) += firmware_attributes_class.o
-obj-$(CONFIG_I2C_MULTI_INSTANTIATE) += i2c-multi-instantiate.o
obj-$(CONFIG_MLX_PLATFORM) += mlx-platform.o
obj-$(CONFIG_TOUCHSCREEN_DMI) += touchscreen_dmi.o
obj-$(CONFIG_WIRELESS_HOTKEY) += wireless-hotkey.o
--
2.25.1
^ permalink raw reply related [flat|nested] 17+ messages in thread* Re: [PATCH v2 3/6] platform/x86: i2c-multi-instantiate: Move it to drivers/acpi folder
2021-12-10 15:40 ` [PATCH v2 3/6] platform/x86: i2c-multi-instantiate: Move it to drivers/acpi folder Stefan Binding
@ 2021-12-10 17:59 ` Mark Brown
2021-12-15 10:26 ` Hans de Goede
0 siblings, 1 reply; 17+ messages in thread
From: Mark Brown @ 2021-12-10 17:59 UTC (permalink / raw)
To: Stefan Binding
Cc: Rafael J . Wysocki, Len Brown, Hans de Goede, Mark Gross,
linux-kernel, linux-spi, linux-acpi, platform-driver-x86,
patches, Lucas Tanure
[-- Attachment #1: Type: text/plain, Size: 546 bytes --]
On Fri, Dec 10, 2021 at 03:40:47PM +0000, Stefan Binding wrote:
> From: Lucas Tanure <tanureal@opensource.cirrus.com>
>
> Moving I2C multi instantiate driver to drivers/acpi folder for
> upcoming conversion into a generic bus multi instantiate
> driver for SPI and I2C
>
> Signed-off-by: Lucas Tanure <tanureal@opensource.cirrus.com>
> ---
You've not provided a Signed-off-by for this so people can't do anything
with it, please see Documentation/process/submitting-patches.rst for
details on what this is and why it's important.
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2 3/6] platform/x86: i2c-multi-instantiate: Move it to drivers/acpi folder
2021-12-10 17:59 ` Mark Brown
@ 2021-12-15 10:26 ` Hans de Goede
0 siblings, 0 replies; 17+ messages in thread
From: Hans de Goede @ 2021-12-15 10:26 UTC (permalink / raw)
To: Mark Brown, Stefan Binding
Cc: Rafael J . Wysocki, Len Brown, Mark Gross, linux-kernel,
linux-spi, linux-acpi, platform-driver-x86, patches,
Lucas Tanure
Hi,
On 12/10/21 18:59, Mark Brown wrote:
> On Fri, Dec 10, 2021 at 03:40:47PM +0000, Stefan Binding wrote:
>> From: Lucas Tanure <tanureal@opensource.cirrus.com>
>>
>> Moving I2C multi instantiate driver to drivers/acpi folder for
>> upcoming conversion into a generic bus multi instantiate
>> driver for SPI and I2C
>>
>> Signed-off-by: Lucas Tanure <tanureal@opensource.cirrus.com>
>> ---
>
> You've not provided a Signed-off-by for this so people can't do anything
> with it, please see Documentation/process/submitting-patches.rst for
> details on what this is and why it's important.
To clarify this, what I believe that Mark means here is that if you
submit a patch which was originally authored by someone else, then
you should add your own (the submitters) Signed-off-by after the
author's Signed-off-by.
The idea is that each person directly touching the patch (rather
then merging a branch which has the patch) adds its S-o-b to the
patch. You will also always see sub-system maintainers add their
own S-o-b at the end when they pick up the patch from the list
and then apply it to their own tree (and possibly resolve
conflicts or touch up things a bit).
Regards,
Hans
^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH v2 4/6] ACPI: i2c-multi-instantiate: Rename it for a generic bus driver name
2021-12-10 15:40 [PATCH v2 0/6] Support Spi in i2c-multi-instantiate driver Stefan Binding
` (2 preceding siblings ...)
2021-12-10 15:40 ` [PATCH v2 3/6] platform/x86: i2c-multi-instantiate: Move it to drivers/acpi folder Stefan Binding
@ 2021-12-10 15:40 ` Stefan Binding
2021-12-10 15:40 ` [PATCH v2 5/6] ACPI: bus-multi-instantiate: Reorganize I2C functions Stefan Binding
` (3 subsequent siblings)
7 siblings, 0 replies; 17+ messages in thread
From: Stefan Binding @ 2021-12-10 15:40 UTC (permalink / raw)
To: Mark Brown, Rafael J . Wysocki, Len Brown, Hans de Goede, Mark Gross
Cc: linux-kernel, linux-spi, linux-acpi, platform-driver-x86,
patches, Lucas Tanure
From: Lucas Tanure <tanureal@opensource.cirrus.com>
Rename I2C multi instantiate driver to bus-multi-instantiate for
upcoming addition of SPI support
Signed-off-by: Lucas Tanure <tanureal@opensource.cirrus.com>
---
MAINTAINERS | 4 +-
drivers/acpi/Kconfig | 4 +-
drivers/acpi/Makefile | 2 +-
...-instantiate.c => bus-multi-instantiate.c} | 89 +++++++++----------
drivers/acpi/scan.c | 8 +-
5 files changed, 53 insertions(+), 54 deletions(-)
rename drivers/acpi/{i2c-multi-instantiate.c => bus-multi-instantiate.c} (53%)
diff --git a/MAINTAINERS b/MAINTAINERS
index 25e056854772..256b44bb6773 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -387,11 +387,11 @@ L: linux-arm-kernel@lists.infradead.org (moderated for non-subscribers)
S: Maintained
F: drivers/acpi/arm64
-ACPI I2C MULTI INSTANTIATE DRIVER
+ACPI BUS MULTI INSTANTIATE DRIVER
M: Hans de Goede <hdegoede@redhat.com>
L: platform-driver-x86@vger.kernel.org
S: Maintained
-F: drivers/acpi/i2c-multi-instantiate.c
+F: drivers/acpi/bus-multi-instantiate.c
ACPI PCC(Platform Communication Channel) MAILBOX DRIVER
M: Sudeep Holla <sudeep.holla@arm.com>
diff --git a/drivers/acpi/Kconfig b/drivers/acpi/Kconfig
index 6200d13fa97b..6ba47dd39eb4 100644
--- a/drivers/acpi/Kconfig
+++ b/drivers/acpi/Kconfig
@@ -295,7 +295,7 @@ config ACPI_PROCESSOR
To compile this driver as a module, choose M here:
the module will be called processor.
-config ACPI_I2C_MULTI_INST
+config ACPI_BUS_MULTI_INST
tristate "I2C multi instantiate pseudo device driver"
depends on I2C
help
@@ -304,7 +304,7 @@ config ACPI_I2C_MULTI_INST
for each device in the firmware-node.
To compile this driver as a module, choose M here: the module
- will be called i2c-multi-instantiate.
+ will be called bus-multi-instantiate.
config ACPI_IPMI
tristate "IPMI"
diff --git a/drivers/acpi/Makefile b/drivers/acpi/Makefile
index 98d700d55960..38cf0ecc595a 100644
--- a/drivers/acpi/Makefile
+++ b/drivers/acpi/Makefile
@@ -102,7 +102,7 @@ obj-$(CONFIG_ACPI_CPPC_LIB) += cppc_acpi.o
obj-$(CONFIG_ACPI_SPCR_TABLE) += spcr.o
obj-$(CONFIG_ACPI_DEBUGGER_USER) += acpi_dbg.o
obj-$(CONFIG_ACPI_PPTT) += pptt.o
-obj-$(CONFIG_ACPI_I2C_MULTI_INST) += i2c-multi-instantiate.o
+obj-$(CONFIG_ACPI_BUS_MULTI_INST) += bus-multi-instantiate.o
# processor has its own "processor." module_param namespace
processor-y := processor_driver.o
diff --git a/drivers/acpi/i2c-multi-instantiate.c b/drivers/acpi/bus-multi-instantiate.c
similarity index 53%
rename from drivers/acpi/i2c-multi-instantiate.c
rename to drivers/acpi/bus-multi-instantiate.c
index 4956a1df5b90..982dfecfd27c 100644
--- a/drivers/acpi/i2c-multi-instantiate.c
+++ b/drivers/acpi/bus-multi-instantiate.c
@@ -1,6 +1,6 @@
// SPDX-License-Identifier: GPL-2.0+
/*
- * I2C multi-instantiate driver, pseudo driver to instantiate multiple
+ * Bus multi-instantiate driver, pseudo driver to instantiate multiple
* i2c-clients from a single fwnode.
*
* Copyright 2018 Hans de Goede <hdegoede@redhat.com>
@@ -21,29 +21,29 @@
#define IRQ_RESOURCE_GPIO 1
#define IRQ_RESOURCE_APIC 2
-struct i2c_inst_data {
+struct bmi_instance {
const char *type;
unsigned int flags;
int irq_idx;
};
-struct i2c_multi_inst_data {
- int num_clients;
- struct i2c_client *clients[];
+struct bmi {
+ int i2c_num;
+ struct i2c_client *i2c_devs[];
};
-static int i2c_multi_inst_probe(struct platform_device *pdev)
+static int bmi_probe(struct platform_device *pdev)
{
- struct i2c_multi_inst_data *multi;
- const struct i2c_inst_data *inst_data;
struct i2c_board_info board_info = {};
+ const struct bmi_instance *inst;
struct device *dev = &pdev->dev;
struct acpi_device *adev;
+ struct bmi *bmi;
char name[32];
int i, ret;
- inst_data = device_get_match_data(dev);
- if (!inst_data) {
+ inst = device_get_match_data(dev);
+ if (!inst) {
dev_err(dev, "Error ACPI match data is missing\n");
return -ENODEV;
}
@@ -55,33 +55,32 @@ static int i2c_multi_inst_probe(struct platform_device *pdev)
if (ret < 0)
return ret;
- multi = devm_kmalloc(dev, struct_size(multi, clients, ret), GFP_KERNEL);
- if (!multi)
+ bmi = devm_kmalloc(dev, struct_size(bmi, i2c_devs, ret), GFP_KERNEL);
+ if (!bmi)
return -ENOMEM;
- multi->num_clients = ret;
+ bmi->i2c_num = ret;
- for (i = 0; i < multi->num_clients && inst_data[i].type; i++) {
+ for (i = 0; i < bmi->i2c_num && inst[i].type; i++) {
memset(&board_info, 0, sizeof(board_info));
- strlcpy(board_info.type, inst_data[i].type, I2C_NAME_SIZE);
- snprintf(name, sizeof(name), "%s-%s.%d", dev_name(dev),
- inst_data[i].type, i);
+ strlcpy(board_info.type, inst[i].type, I2C_NAME_SIZE);
+ snprintf(name, sizeof(name), "%s-%s.%d", dev_name(dev), inst[i].type, i);
board_info.dev_name = name;
- switch (inst_data[i].flags & IRQ_RESOURCE_TYPE) {
+ switch (inst[i].flags & IRQ_RESOURCE_TYPE) {
case IRQ_RESOURCE_GPIO:
- ret = acpi_dev_gpio_irq_get(adev, inst_data[i].irq_idx);
+ ret = acpi_dev_gpio_irq_get(adev, inst[i].irq_idx);
if (ret < 0) {
dev_err(dev, "Error requesting irq at index %d: %d\n",
- inst_data[i].irq_idx, ret);
+ inst[i].irq_idx, ret);
goto error;
}
board_info.irq = ret;
break;
case IRQ_RESOURCE_APIC:
- ret = platform_get_irq(pdev, inst_data[i].irq_idx);
+ ret = platform_get_irq(pdev, inst[i].irq_idx);
if (ret < 0) {
dev_dbg(dev, "Error requesting irq at index %d: %d\n",
- inst_data[i].irq_idx, ret);
+ inst[i].irq_idx, ret);
goto error;
}
board_info.irq = ret;
@@ -90,48 +89,48 @@ static int i2c_multi_inst_probe(struct platform_device *pdev)
board_info.irq = 0;
break;
}
- multi->clients[i] = i2c_acpi_new_device(dev, i, &board_info);
- if (IS_ERR(multi->clients[i])) {
- ret = dev_err_probe(dev, PTR_ERR(multi->clients[i]),
+ bmi->i2c_devs[i] = i2c_acpi_new_device(dev, i, &board_info);
+ if (IS_ERR(bmi->i2c_devs[i])) {
+ ret = dev_err_probe(dev, PTR_ERR(bmi->i2c_devs[i]),
"Error creating i2c-client, idx %d\n", i);
goto error;
}
}
- if (i < multi->num_clients) {
+ if (i < bmi->i2c_num) {
dev_err(dev, "Error finding driver, idx %d\n", i);
ret = -ENODEV;
goto error;
}
- platform_set_drvdata(pdev, multi);
+ platform_set_drvdata(pdev, bmi);
return 0;
error:
while (--i >= 0)
- i2c_unregister_device(multi->clients[i]);
+ i2c_unregister_device(bmi->i2c_devs[i]);
return ret;
}
-static int i2c_multi_inst_remove(struct platform_device *pdev)
+static int bmi_remove(struct platform_device *pdev)
{
- struct i2c_multi_inst_data *multi = platform_get_drvdata(pdev);
+ struct bmi *bmi = platform_get_drvdata(pdev);
int i;
- for (i = 0; i < multi->num_clients; i++)
- i2c_unregister_device(multi->clients[i]);
+ for (i = 0; i < bmi->i2c_num; i++)
+ i2c_unregister_device(bmi->i2c_devs[i]);
return 0;
}
-static const struct i2c_inst_data bsg1160_data[] = {
+static const struct bmi_instance bsg1160_data[] = {
{ "bmc150_accel", IRQ_RESOURCE_GPIO, 0 },
{ "bmc150_magn" },
{ "bmg160" },
{}
};
-static const struct i2c_inst_data bsg2150_data[] = {
+static const struct bmi_instance bsg2150_data[] = {
{ "bmc150_accel", IRQ_RESOURCE_GPIO, 0 },
{ "bmc150_magn" },
/* The resources describe a 3th client, but it is not really there. */
@@ -139,7 +138,7 @@ static const struct i2c_inst_data bsg2150_data[] = {
{}
};
-static const struct i2c_inst_data int3515_data[] = {
+static const struct bmi_instance int3515_data[] = {
{ "tps6598x", IRQ_RESOURCE_APIC, 0 },
{ "tps6598x", IRQ_RESOURCE_APIC, 1 },
{ "tps6598x", IRQ_RESOURCE_APIC, 2 },
@@ -148,27 +147,27 @@ static const struct i2c_inst_data int3515_data[] = {
};
/*
- * Note new device-ids must also be added to i2c_multi_instantiate_ids in
+ * Note new device-ids must also be added to bus_multi_instantiate_ids in
* drivers/acpi/scan.c: acpi_device_enumeration_by_parent().
*/
-static const struct acpi_device_id i2c_multi_inst_acpi_ids[] = {
+static const struct acpi_device_id bmi_acpi_ids[] = {
{ "BSG1160", (unsigned long)bsg1160_data },
{ "BSG2150", (unsigned long)bsg2150_data },
{ "INT3515", (unsigned long)int3515_data },
{ }
};
-MODULE_DEVICE_TABLE(acpi, i2c_multi_inst_acpi_ids);
+MODULE_DEVICE_TABLE(acpi, bmi_acpi_ids);
-static struct platform_driver i2c_multi_inst_driver = {
+static struct platform_driver bmi_driver = {
.driver = {
- .name = "I2C multi instantiate pseudo device driver",
- .acpi_match_table = i2c_multi_inst_acpi_ids,
+ .name = "Bus multi instantiate pseudo device driver",
+ .acpi_match_table = bmi_acpi_ids,
},
- .probe = i2c_multi_inst_probe,
- .remove = i2c_multi_inst_remove,
+ .probe = bmi_probe,
+ .remove = bmi_remove,
};
-module_platform_driver(i2c_multi_inst_driver);
+module_platform_driver(bmi_driver);
-MODULE_DESCRIPTION("I2C multi instantiate pseudo device driver");
+MODULE_DESCRIPTION("Bus multi instantiate pseudo device driver");
MODULE_AUTHOR("Hans de Goede <hdegoede@redhat.com>");
MODULE_LICENSE("GPL");
diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
index e5c81b3df09c..969d8138d019 100644
--- a/drivers/acpi/scan.c
+++ b/drivers/acpi/scan.c
@@ -1700,10 +1700,10 @@ static bool acpi_device_enumeration_by_parent(struct acpi_device *device)
* must be instantiated for each, each with its own i2c_device_id.
* Normally we only instantiate an i2c-client for the first resource,
* using the ACPI HID as id. These special cases are handled by the
- * drivers/acpi/i2c-multi-instantiate.c driver, which knows
+ * drivers/acpi/bus-multi-instantiate.c driver, which knows
* which i2c_device_id to use for each resource.
*/
- static const struct acpi_device_id i2c_multi_instantiate_ids[] = {
+ static const struct acpi_device_id bus_multi_instantiate_ids[] = {
{"BSG1160", },
{"BSG2150", },
{"INT33FE", },
@@ -1721,8 +1721,8 @@ static bool acpi_device_enumeration_by_parent(struct acpi_device *device)
fwnode_property_present(&device->fwnode, "baud")))
return true;
- /* Instantiate a pdev for the i2c-multi-instantiate drv to bind to */
- if (!acpi_match_device_ids(device, i2c_multi_instantiate_ids))
+ /* Instantiate a pdev for the bus-multi-instantiate drv to bind to */
+ if (!acpi_match_device_ids(device, bus_multi_instantiate_ids))
return false;
INIT_LIST_HEAD(&resource_list);
--
2.25.1
^ permalink raw reply related [flat|nested] 17+ messages in thread* [PATCH v2 5/6] ACPI: bus-multi-instantiate: Reorganize I2C functions
2021-12-10 15:40 [PATCH v2 0/6] Support Spi in i2c-multi-instantiate driver Stefan Binding
` (3 preceding siblings ...)
2021-12-10 15:40 ` [PATCH v2 4/6] ACPI: i2c-multi-instantiate: Rename it for a generic bus driver name Stefan Binding
@ 2021-12-10 15:40 ` Stefan Binding
2021-12-10 15:40 ` [PATCH v2 6/6] ACPI: bus-multi-instantiate: Add SPI support Stefan Binding
` (2 subsequent siblings)
7 siblings, 0 replies; 17+ messages in thread
From: Stefan Binding @ 2021-12-10 15:40 UTC (permalink / raw)
To: Mark Brown, Rafael J . Wysocki, Len Brown, Hans de Goede, Mark Gross
Cc: linux-kernel, linux-spi, linux-acpi, platform-driver-x86,
patches, Lucas Tanure
From: Lucas Tanure <tanureal@opensource.cirrus.com>
Reorganize I2C functions to accommodate SPI support
Split the probe and factor out parts of the code
that will be used in the SPI support
Signed-off-by: Lucas Tanure <tanureal@opensource.cirrus.com>
---
drivers/acpi/bus-multi-instantiate.c | 150 +++++++++++++++++----------
1 file changed, 96 insertions(+), 54 deletions(-)
diff --git a/drivers/acpi/bus-multi-instantiate.c b/drivers/acpi/bus-multi-instantiate.c
index 982dfecfd27c..50f1540762e9 100644
--- a/drivers/acpi/bus-multi-instantiate.c
+++ b/drivers/acpi/bus-multi-instantiate.c
@@ -29,85 +29,129 @@ struct bmi_instance {
struct bmi {
int i2c_num;
- struct i2c_client *i2c_devs[];
+ struct i2c_client **i2c_devs;
};
-static int bmi_probe(struct platform_device *pdev)
+static int bmi_get_irq(struct platform_device *pdev, struct acpi_device *adev,
+ const struct bmi_instance *inst)
+{
+ int ret;
+
+ switch (inst->flags & IRQ_RESOURCE_TYPE) {
+ case IRQ_RESOURCE_GPIO:
+ ret = acpi_dev_gpio_irq_get(adev, inst->irq_idx);
+ break;
+ case IRQ_RESOURCE_APIC:
+ ret = platform_get_irq(pdev, inst->irq_idx);
+ break;
+ default:
+ ret = 0;
+ break;
+ }
+
+ if (ret < 0)
+ dev_err_probe(&pdev->dev, ret, "Error requesting irq at index %d: %d\n",
+ inst->irq_idx, ret);
+
+ return ret;
+}
+
+static void bmi_devs_unregister(struct bmi *bmi)
+{
+ while (bmi->i2c_num > 0)
+ i2c_unregister_device(bmi->i2c_devs[--bmi->i2c_num]);
+}
+
+/**
+ * bmi_i2c_probe - Instantiate multiple I2C devices from inst array
+ * @pdev: Platform device
+ * @adev: ACPI device
+ * @bmi: Internal struct for Bus multi instantiate driver
+ * @inst: Array of instances to probe
+ *
+ * Returns the number of I2C devices instantiate, Zero if none is found or a negative error code.
+ */
+static int bmi_i2c_probe(struct platform_device *pdev, struct acpi_device *adev, struct bmi *bmi,
+ const struct bmi_instance *inst_array)
{
struct i2c_board_info board_info = {};
- const struct bmi_instance *inst;
struct device *dev = &pdev->dev;
- struct acpi_device *adev;
- struct bmi *bmi;
char name[32];
- int i, ret;
+ int i, ret = 0, count;
- inst = device_get_match_data(dev);
- if (!inst) {
- dev_err(dev, "Error ACPI match data is missing\n");
- return -ENODEV;
- }
-
- adev = ACPI_COMPANION(dev);
-
- /* Count number of clients to instantiate */
ret = i2c_acpi_client_count(adev);
- if (ret < 0)
+ if (ret <= 0)
return ret;
+ count = ret;
- bmi = devm_kmalloc(dev, struct_size(bmi, i2c_devs, ret), GFP_KERNEL);
- if (!bmi)
+ bmi->i2c_devs = devm_kcalloc(dev, count, sizeof(*bmi->i2c_devs), GFP_KERNEL);
+ if (!bmi->i2c_devs)
return -ENOMEM;
- bmi->i2c_num = ret;
-
- for (i = 0; i < bmi->i2c_num && inst[i].type; i++) {
+ for (i = 0; i < count && inst_array[i].type; i++) {
memset(&board_info, 0, sizeof(board_info));
- strlcpy(board_info.type, inst[i].type, I2C_NAME_SIZE);
- snprintf(name, sizeof(name), "%s-%s.%d", dev_name(dev), inst[i].type, i);
+ strscpy(board_info.type, inst_array[i].type, I2C_NAME_SIZE);
+ snprintf(name, sizeof(name), "%s-%s.%d", dev_name(dev), inst_array[i].type, i);
board_info.dev_name = name;
- switch (inst[i].flags & IRQ_RESOURCE_TYPE) {
- case IRQ_RESOURCE_GPIO:
- ret = acpi_dev_gpio_irq_get(adev, inst[i].irq_idx);
- if (ret < 0) {
- dev_err(dev, "Error requesting irq at index %d: %d\n",
- inst[i].irq_idx, ret);
- goto error;
- }
- board_info.irq = ret;
- break;
- case IRQ_RESOURCE_APIC:
- ret = platform_get_irq(pdev, inst[i].irq_idx);
- if (ret < 0) {
- dev_dbg(dev, "Error requesting irq at index %d: %d\n",
- inst[i].irq_idx, ret);
- goto error;
- }
- board_info.irq = ret;
- break;
- default:
- board_info.irq = 0;
- break;
- }
+
+ ret = bmi_get_irq(pdev, adev, &inst_array[i]);
+ if (ret < 0)
+ goto error;
+ board_info.irq = ret;
+
bmi->i2c_devs[i] = i2c_acpi_new_device(dev, i, &board_info);
if (IS_ERR(bmi->i2c_devs[i])) {
ret = dev_err_probe(dev, PTR_ERR(bmi->i2c_devs[i]),
"Error creating i2c-client, idx %d\n", i);
goto error;
}
+ bmi->i2c_num++;
}
- if (i < bmi->i2c_num) {
+ if (bmi->i2c_num < count) {
dev_err(dev, "Error finding driver, idx %d\n", i);
ret = -ENODEV;
goto error;
}
- platform_set_drvdata(pdev, bmi);
- return 0;
+ dev_info(dev, "Instantiate %d I2C devices.\n", bmi->i2c_num);
+ return bmi->i2c_num;
error:
- while (--i >= 0)
- i2c_unregister_device(bmi->i2c_devs[i]);
+ dev_err_probe(dev, ret, "I2C error %d\n", ret);
+ bmi_devs_unregister(bmi);
+
+ return ret;
+}
+
+static int bmi_probe(struct platform_device *pdev)
+{
+ const struct bmi_instance *inst_array;
+ struct device *dev = &pdev->dev;
+ struct acpi_device *adev;
+ struct bmi *bmi;
+ int ret;
+
+ inst_array = device_get_match_data(dev);
+ if (!inst_array) {
+ dev_err(dev, "Error ACPI match data is missing\n");
+ return -ENODEV;
+ }
+
+ adev = ACPI_COMPANION(dev);
+ if (!adev)
+ return -ENODEV;
+
+ bmi = devm_kzalloc(dev, sizeof(*bmi), GFP_KERNEL);
+ if (!bmi)
+ return -ENOMEM;
+
+ platform_set_drvdata(pdev, bmi);
+
+ ret = bmi_i2c_probe(pdev, adev, bmi, inst_array);
+ if (ret > 0)
+ return 0;
+ if (ret == 0)
+ ret = -ENODEV;
return ret;
}
@@ -115,10 +159,8 @@ static int bmi_probe(struct platform_device *pdev)
static int bmi_remove(struct platform_device *pdev)
{
struct bmi *bmi = platform_get_drvdata(pdev);
- int i;
- for (i = 0; i < bmi->i2c_num; i++)
- i2c_unregister_device(bmi->i2c_devs[i]);
+ bmi_devs_unregister(bmi);
return 0;
}
--
2.25.1
^ permalink raw reply related [flat|nested] 17+ messages in thread* [PATCH v2 6/6] ACPI: bus-multi-instantiate: Add SPI support
2021-12-10 15:40 [PATCH v2 0/6] Support Spi in i2c-multi-instantiate driver Stefan Binding
` (4 preceding siblings ...)
2021-12-10 15:40 ` [PATCH v2 5/6] ACPI: bus-multi-instantiate: Reorganize I2C functions Stefan Binding
@ 2021-12-10 15:40 ` Stefan Binding
2021-12-21 18:32 ` Hans de Goede
2021-12-10 16:54 ` [PATCH v2 0/6] Support Spi in i2c-multi-instantiate driver Hans de Goede
2021-12-21 18:16 ` Hans de Goede
7 siblings, 1 reply; 17+ messages in thread
From: Stefan Binding @ 2021-12-10 15:40 UTC (permalink / raw)
To: Mark Brown, Rafael J . Wysocki, Len Brown, Hans de Goede, Mark Gross
Cc: linux-kernel, linux-spi, linux-acpi, platform-driver-x86,
patches, Stefan Binding
Add support for spi bus in bus-multi-instantiate driver
Some peripherals can have either a I2C or a SPI connection
to the host (but not both) but use the same HID for both
types. So it is not possible to use the HID to determine
whether it is I2C or SPI. The driver must check the node
to see if it contains I2cSerialBus or SpiSerialBus entries.
For backwards-compatibility with the existing nodes I2C is
checked first and if such entries are found ONLY I2C devices
are created. Since some existing nodes that were already
handled by this driver could also contain unrelated
SpiSerialBus nodes that were previously ignored, and this
preserves that behavior. If there is ever a need to handle
a node where both I2C and SPI devices must be instantiated
this can be added in future.
Signed-off-by: Stefan Binding <sbinding@opensource.cirrus.com>
---
drivers/acpi/Kconfig | 10 +-
drivers/acpi/bus-multi-instantiate.c | 345 ++++++++++++++++++++++++---
drivers/acpi/scan.c | 13 +-
3 files changed, 327 insertions(+), 41 deletions(-)
diff --git a/drivers/acpi/Kconfig b/drivers/acpi/Kconfig
index 6ba47dd39eb4..948f39d55595 100644
--- a/drivers/acpi/Kconfig
+++ b/drivers/acpi/Kconfig
@@ -296,12 +296,12 @@ config ACPI_PROCESSOR
the module will be called processor.
config ACPI_BUS_MULTI_INST
- tristate "I2C multi instantiate pseudo device driver"
- depends on I2C
+ tristate "I2C and SPI multi instantiate pseudo device driver"
+ depends on I2C && SPI
help
- Some ACPI-based systems list multiple i2c-devices in a single ACPI
- firmware-node. This driver will instantiate separate i2c-clients
- for each device in the firmware-node.
+ Some ACPI-based systems list multiple i2c/spi devices in a
+ single ACPI firmware-node. This driver will instantiate separate
+ i2c-clients or spi-devices for each device in the firmware-node.
To compile this driver as a module, choose M here: the module
will be called bus-multi-instantiate.
diff --git a/drivers/acpi/bus-multi-instantiate.c b/drivers/acpi/bus-multi-instantiate.c
index 50f1540762e9..c1306a0ee13c 100644
--- a/drivers/acpi/bus-multi-instantiate.c
+++ b/drivers/acpi/bus-multi-instantiate.c
@@ -1,7 +1,7 @@
// SPDX-License-Identifier: GPL-2.0+
/*
* Bus multi-instantiate driver, pseudo driver to instantiate multiple
- * i2c-clients from a single fwnode.
+ * i2c-clients or spi-devices from a single fwnode.
*
* Copyright 2018 Hans de Goede <hdegoede@redhat.com>
*/
@@ -14,6 +14,7 @@
#include <linux/module.h>
#include <linux/platform_device.h>
#include <linux/property.h>
+#include <linux/spi/spi.h>
#include <linux/types.h>
#define IRQ_RESOURCE_TYPE GENMASK(1, 0)
@@ -21,15 +22,38 @@
#define IRQ_RESOURCE_GPIO 1
#define IRQ_RESOURCE_APIC 2
+enum bmi_bus_type {
+ BMI_I2C,
+ BMI_SPI,
+ BMI_AUTO_DETEC,
+};
+
+struct bmi_spi_acpi {
+ char *resource_source;
+ struct acpi_resource_spi_serialbus sb;
+};
+
+struct bmi_spi_sb_acpi {
+ int count;
+ struct bmi_spi_acpi acpi_data[];
+};
+
struct bmi_instance {
const char *type;
unsigned int flags;
int irq_idx;
};
+struct bmi_node {
+ enum bmi_bus_type bus_type;
+ struct bmi_instance instances[];
+};
+
struct bmi {
int i2c_num;
+ int spi_num;
struct i2c_client **i2c_devs;
+ struct spi_device **spi_devs;
};
static int bmi_get_irq(struct platform_device *pdev, struct acpi_device *adev,
@@ -60,6 +84,230 @@ static void bmi_devs_unregister(struct bmi *bmi)
{
while (bmi->i2c_num > 0)
i2c_unregister_device(bmi->i2c_devs[--bmi->i2c_num]);
+
+ while (bmi->spi_num > 0)
+ spi_unregister_device(bmi->spi_devs[--bmi->spi_num]);
+}
+
+static int bmi_spi_count(struct acpi_resource *ares, void *data)
+{
+ struct acpi_resource_spi_serialbus *sb;
+ int *count = data;
+
+ if (ares->type != ACPI_RESOURCE_TYPE_SERIAL_BUS)
+ return 1;
+
+ sb = &ares->data.spi_serial_bus;
+ if (sb->type != ACPI_RESOURCE_SERIAL_TYPE_SPI)
+ return 1;
+
+ *count = *count + 1;
+
+ return 1;
+}
+
+static int bmi_spi_count_resources(struct acpi_device *adev)
+{
+ LIST_HEAD(r);
+ int count = 0;
+ int ret;
+
+ ret = acpi_dev_get_resources(adev, &r, bmi_spi_count, &count);
+ if (ret < 0)
+ return ret;
+
+ acpi_dev_free_resource_list(&r);
+
+ return count;
+}
+
+static int bmi_spi_save_res(struct acpi_resource *ares, void *data)
+{
+ struct acpi_resource_spi_serialbus *sb;
+ struct bmi_spi_sb_acpi *resources = data;
+ struct bmi_spi_acpi *acpi_data;
+
+ if (ares->type != ACPI_RESOURCE_TYPE_SERIAL_BUS)
+ return 1;
+
+ sb = &ares->data.spi_serial_bus;
+ if (sb->type != ACPI_RESOURCE_SERIAL_TYPE_SPI)
+ return 1;
+
+ acpi_data = &resources->acpi_data[resources->count];
+ memcpy(&acpi_data->sb, sb, sizeof(*sb));
+ acpi_data->resource_source = kstrndup(sb->resource_source.string_ptr,
+ sb->resource_source.string_length, GFP_KERNEL);
+ if (!acpi_data->resource_source)
+ return 1;
+ resources->count++;
+
+ return 1;
+}
+
+static void bmi_spi_res_free(struct bmi_spi_sb_acpi *resources)
+{
+ if (!resources)
+ return;
+
+ while (resources->count)
+ kfree(resources->acpi_data[--resources->count].resource_source);
+ kfree(resources);
+}
+
+static struct bmi_spi_sb_acpi *bmi_spi_get_resources(struct device *dev,
+ struct acpi_device *adev, int count)
+{
+ struct bmi_spi_sb_acpi *resources;
+ LIST_HEAD(r);
+ int ret;
+
+ resources = kmalloc(struct_size(resources, acpi_data, count), GFP_KERNEL);
+ if (!resources)
+ return NULL;
+
+ ret = acpi_dev_get_resources(adev, &r, bmi_spi_save_res, resources);
+ if (ret < 0)
+ goto error;
+
+ acpi_dev_free_resource_list(&r);
+
+ return resources;
+
+error:
+ bmi_spi_res_free(resources);
+ return NULL;
+}
+
+static struct spi_controller *bmi_find_spi_controller(char *path)
+{
+ acpi_handle parent_handle;
+ struct acpi_device *adev;
+ acpi_status status;
+
+ status = acpi_get_handle(NULL, path, &parent_handle);
+ if (ACPI_FAILURE(status))
+ return NULL;
+
+ if (acpi_bus_get_device(parent_handle, &adev))
+ return NULL;
+
+ return acpi_spi_find_controller_by_adev(adev);
+}
+
+/**
+ * bmi_spi_probe - Instantiate multiple SPI devices from inst array
+ * @pdev: Platform device
+ * @adev: ACPI device
+ * @bmi: Internal struct for Bus multi instantiate driver
+ * @inst: Array of instances to probe
+ *
+ * Returns the number of SPI devices instantiate, Zero if none is found or a negative error code.
+ */
+static int bmi_spi_probe(struct platform_device *pdev, struct acpi_device *adev, struct bmi *bmi,
+ const struct bmi_instance *inst_array)
+{
+ struct bmi_spi_sb_acpi *acpi_data;
+ struct device *dev = &pdev->dev;
+ struct spi_controller *ctlr;
+ struct spi_device *spi_dev;
+ char name[50];
+ int i, ret, count;
+
+ ret = bmi_spi_count_resources(adev);
+ if (ret <= 0)
+ return ret;
+ count = ret;
+
+ bmi->spi_devs = devm_kcalloc(dev, count, sizeof(*bmi->spi_devs), GFP_KERNEL);
+ if (!bmi->spi_devs)
+ return -ENOMEM;
+
+ acpi_data = bmi_spi_get_resources(dev, adev, count);
+ if (!acpi_data)
+ return -ENOMEM;
+
+ for (i = 0; i < count && inst_array[i].type; i++) {
+ ctlr = bmi_find_spi_controller(acpi_data->acpi_data[i].resource_source);
+ if (!ctlr) {
+ ret = -EPROBE_DEFER;
+ goto error;
+ }
+
+ spi_dev = spi_alloc_device(ctlr);
+ if (!spi_dev) {
+ dev_err(&ctlr->dev, "failed to allocate SPI device for %s\n",
+ dev_name(&adev->dev));
+ ret = -ENOMEM;
+ goto error;
+ }
+
+ strscpy(spi_dev->modalias, inst_array[i].type, sizeof(spi_dev->modalias));
+
+ if (ctlr->fw_translate_cs) {
+ ret = ctlr->fw_translate_cs(ctlr,
+ acpi_data->acpi_data[i].sb.device_selection);
+ if (ret < 0) {
+ spi_dev_put(spi_dev);
+ goto error;
+ }
+ spi_dev->chip_select = ret;
+ } else {
+ spi_dev->chip_select = acpi_data->acpi_data[i].sb.device_selection;
+ }
+
+ spi_dev->max_speed_hz = acpi_data->acpi_data[i].sb.connection_speed;
+ spi_dev->bits_per_word = acpi_data->acpi_data[i].sb.data_bit_length;
+
+ if (acpi_data->acpi_data[i].sb.clock_phase == ACPI_SPI_SECOND_PHASE)
+ spi_dev->mode |= SPI_CPHA;
+ if (acpi_data->acpi_data[i].sb.clock_polarity == ACPI_SPI_START_HIGH)
+ spi_dev->mode |= SPI_CPOL;
+ if (acpi_data->acpi_data[i].sb.device_polarity == ACPI_SPI_ACTIVE_HIGH)
+ spi_dev->mode |= SPI_CS_HIGH;
+
+ ret = bmi_get_irq(pdev, adev, &inst_array[i]);
+ if (ret < 0) {
+ spi_dev_put(spi_dev);
+ goto error;
+ }
+ spi_dev->irq = ret;
+
+ snprintf(name, sizeof(name), "%s-%s-%s.%d", dev_name(&ctlr->dev), dev_name(dev),
+ inst_array[i].type, i);
+ spi_dev->dev.init_name = name;
+
+ ret = spi_add_device(spi_dev);
+ if (ret) {
+ dev_err(&ctlr->dev, "failed to add SPI device %s from ACPI: %d\n",
+ dev_name(&adev->dev), ret);
+ spi_dev_put(spi_dev);
+ goto error;
+ }
+
+ dev_dbg(dev, "SPI device %s using chip select %u", name, spi_dev->chip_select);
+
+ bmi->spi_devs[i] = spi_dev;
+ bmi->spi_num++;
+ }
+
+ if (bmi->spi_num < count) {
+ dev_err(dev, "Error finding driver, idx %d\n", i);
+ ret = -ENODEV;
+ goto error;
+ }
+
+ dev_info(dev, "Instantiate %d SPI devices.\n", bmi->spi_num);
+ bmi_spi_res_free(acpi_data);
+
+ return bmi->spi_num;
+error:
+ bmi_spi_res_free(acpi_data);
+ bmi_devs_unregister(bmi);
+ dev_err_probe(dev, ret, "SPI error %d\n", ret);
+
+ return ret;
+
}
/**
@@ -125,14 +373,14 @@ static int bmi_i2c_probe(struct platform_device *pdev, struct acpi_device *adev,
static int bmi_probe(struct platform_device *pdev)
{
- const struct bmi_instance *inst_array;
struct device *dev = &pdev->dev;
+ const struct bmi_node *node;
struct acpi_device *adev;
struct bmi *bmi;
- int ret;
+ int i2c_ret = 0, spi_ret = 0;
- inst_array = device_get_match_data(dev);
- if (!inst_array) {
+ node = device_get_match_data(dev);
+ if (!node) {
dev_err(dev, "Error ACPI match data is missing\n");
return -ENODEV;
}
@@ -147,13 +395,44 @@ static int bmi_probe(struct platform_device *pdev)
platform_set_drvdata(pdev, bmi);
- ret = bmi_i2c_probe(pdev, adev, bmi, inst_array);
- if (ret > 0)
+ /* Each time this driver probes only one type of bus will be chosen.
+ * And I2C has preference, which means that if find a I2cSerialBus it assumes
+ * that all following devices will also be I2C.
+ * In case there are zero I2C devices, it assumes that all following devices are SPI.
+ */
+ if (node->bus_type != BMI_SPI) {
+ i2c_ret = bmi_i2c_probe(pdev, adev, bmi, node->instances);
+ if (i2c_ret > 0)
+ return 0;
+ else if (i2c_ret == -EPROBE_DEFER)
+ return i2c_ret;
+ if (node->bus_type == BMI_I2C) {
+ if (i2c_ret == 0)
+ return -ENODEV;
+ else
+ return i2c_ret;
+ }
+ }
+ /* BMI_SPI or BMI_AUTO_DETEC */
+ spi_ret = bmi_spi_probe(pdev, adev, bmi, node->instances);
+ if (spi_ret > 0)
return 0;
- if (ret == 0)
- ret = -ENODEV;
+ else if (spi_ret == -EPROBE_DEFER)
+ return -EPROBE_DEFER;
+ if (node->bus_type == BMI_SPI) {
+ if (spi_ret == 0)
+ return -ENODEV;
+ else
+ return spi_ret;
+ }
- return ret;
+ /* The only way to get here is BMI_AUTO_DETEC and i2c_ret <= 0 and spi_ret <= 0 */
+ if (i2c_ret == 0 && spi_ret == 0)
+ return -ENODEV;
+ else if (i2c_ret == 0 && spi_ret)
+ return spi_ret;
+
+ return i2c_ret;
}
static int bmi_remove(struct platform_device *pdev)
@@ -165,27 +444,33 @@ static int bmi_remove(struct platform_device *pdev)
return 0;
}
-static const struct bmi_instance bsg1160_data[] = {
- { "bmc150_accel", IRQ_RESOURCE_GPIO, 0 },
- { "bmc150_magn" },
- { "bmg160" },
- {}
+static const struct bmi_node bsg1160_data = {
+ .instances = {
+ { "bmc150_accel", IRQ_RESOURCE_GPIO, 0 },
+ { "bmc150_magn" },
+ { "bmg160" },
+ {}
+ },
};
-static const struct bmi_instance bsg2150_data[] = {
- { "bmc150_accel", IRQ_RESOURCE_GPIO, 0 },
- { "bmc150_magn" },
- /* The resources describe a 3th client, but it is not really there. */
- { "bsg2150_dummy_dev" },
- {}
+static const struct bmi_node bsg2150_data = {
+ .instances = {
+ { "bmc150_accel", IRQ_RESOURCE_GPIO, 0 },
+ { "bmc150_magn" },
+ /* The resources describe a 3th client, but it is not really there. */
+ { "bsg2150_dummy_dev" },
+ {}
+ },
};
-static const struct bmi_instance int3515_data[] = {
- { "tps6598x", IRQ_RESOURCE_APIC, 0 },
- { "tps6598x", IRQ_RESOURCE_APIC, 1 },
- { "tps6598x", IRQ_RESOURCE_APIC, 2 },
- { "tps6598x", IRQ_RESOURCE_APIC, 3 },
- {}
+static const struct bmi_node int3515_data = {
+ .instances = {
+ { "tps6598x", IRQ_RESOURCE_APIC, 0 },
+ { "tps6598x", IRQ_RESOURCE_APIC, 1 },
+ { "tps6598x", IRQ_RESOURCE_APIC, 2 },
+ { "tps6598x", IRQ_RESOURCE_APIC, 3 },
+ {}
+ },
};
/*
@@ -193,9 +478,9 @@ static const struct bmi_instance int3515_data[] = {
* drivers/acpi/scan.c: acpi_device_enumeration_by_parent().
*/
static const struct acpi_device_id bmi_acpi_ids[] = {
- { "BSG1160", (unsigned long)bsg1160_data },
- { "BSG2150", (unsigned long)bsg2150_data },
- { "INT3515", (unsigned long)int3515_data },
+ { "BSG1160", (unsigned long)&bsg1160_data },
+ { "BSG2150", (unsigned long)&bsg2150_data },
+ { "INT3515", (unsigned long)&int3515_data },
{ }
};
MODULE_DEVICE_TABLE(acpi, bmi_acpi_ids);
diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
index 969d8138d019..8b937fc20d23 100644
--- a/drivers/acpi/scan.c
+++ b/drivers/acpi/scan.c
@@ -1696,12 +1696,13 @@ static bool acpi_device_enumeration_by_parent(struct acpi_device *device)
struct list_head resource_list;
bool is_serial_bus_slave = false;
/*
- * These devices have multiple I2cSerialBus resources and an i2c-client
- * must be instantiated for each, each with its own i2c_device_id.
- * Normally we only instantiate an i2c-client for the first resource,
- * using the ACPI HID as id. These special cases are handled by the
- * drivers/acpi/bus-multi-instantiate.c driver, which knows
- * which i2c_device_id to use for each resource.
+ * These devices have multiple I2cSerialBus/SpiSerialBus resources
+ * and an (i2c/spi)-client must be instantiated for each, each with
+ * its own i2c_device_id/spi_device_id.
+ * Normally we only instantiate an (i2c/spi)-client for the first
+ * resource, using the ACPI HID as id. These special cases are handled
+ * by the drivers/acpi/bus-multi-instantiate.c driver, which
+ * knows which i2c_device_id or spi_device_id to use for each resource.
*/
static const struct acpi_device_id bus_multi_instantiate_ids[] = {
{"BSG1160", },
--
2.25.1
^ permalink raw reply related [flat|nested] 17+ messages in thread* Re: [PATCH v2 6/6] ACPI: bus-multi-instantiate: Add SPI support
2021-12-10 15:40 ` [PATCH v2 6/6] ACPI: bus-multi-instantiate: Add SPI support Stefan Binding
@ 2021-12-21 18:32 ` Hans de Goede
2021-12-21 19:22 ` Hans de Goede
2022-01-10 14:36 ` Stefan Binding
0 siblings, 2 replies; 17+ messages in thread
From: Hans de Goede @ 2021-12-21 18:32 UTC (permalink / raw)
To: Stefan Binding, Mark Brown, Rafael J . Wysocki, Len Brown, Mark Gross
Cc: linux-kernel, linux-spi, linux-acpi, platform-driver-x86, patches
Hi,
On 12/10/21 16:40, Stefan Binding wrote:
> Add support for spi bus in bus-multi-instantiate driver
>
> Some peripherals can have either a I2C or a SPI connection
> to the host (but not both) but use the same HID for both
> types. So it is not possible to use the HID to determine
> whether it is I2C or SPI. The driver must check the node
> to see if it contains I2cSerialBus or SpiSerialBus entries.
>
> For backwards-compatibility with the existing nodes I2C is
> checked first and if such entries are found ONLY I2C devices
> are created. Since some existing nodes that were already
> handled by this driver could also contain unrelated
> SpiSerialBus nodes that were previously ignored, and this
> preserves that behavior. If there is ever a need to handle
> a node where both I2C and SPI devices must be instantiated
> this can be added in future.
>
> Signed-off-by: Stefan Binding <sbinding@opensource.cirrus.com>
> ---
> drivers/acpi/Kconfig | 10 +-
> drivers/acpi/bus-multi-instantiate.c | 345 ++++++++++++++++++++++++---
> drivers/acpi/scan.c | 13 +-
> 3 files changed, 327 insertions(+), 41 deletions(-)
>
> diff --git a/drivers/acpi/Kconfig b/drivers/acpi/Kconfig
> index 6ba47dd39eb4..948f39d55595 100644
> --- a/drivers/acpi/Kconfig
> +++ b/drivers/acpi/Kconfig
> @@ -296,12 +296,12 @@ config ACPI_PROCESSOR
> the module will be called processor.
>
> config ACPI_BUS_MULTI_INST
> - tristate "I2C multi instantiate pseudo device driver"
> - depends on I2C
> + tristate "I2C and SPI multi instantiate pseudo device driver"
> + depends on I2C && SPI
> help
> - Some ACPI-based systems list multiple i2c-devices in a single ACPI
> - firmware-node. This driver will instantiate separate i2c-clients
> - for each device in the firmware-node.
> + Some ACPI-based systems list multiple i2c/spi devices in a
> + single ACPI firmware-node. This driver will instantiate separate
> + i2c-clients or spi-devices for each device in the firmware-node.
>
> To compile this driver as a module, choose M here: the module
> will be called bus-multi-instantiate.
> diff --git a/drivers/acpi/bus-multi-instantiate.c b/drivers/acpi/bus-multi-instantiate.c
> index 50f1540762e9..c1306a0ee13c 100644
> --- a/drivers/acpi/bus-multi-instantiate.c
> +++ b/drivers/acpi/bus-multi-instantiate.c
> @@ -1,7 +1,7 @@
> // SPDX-License-Identifier: GPL-2.0+
> /*
> * Bus multi-instantiate driver, pseudo driver to instantiate multiple
> - * i2c-clients from a single fwnode.
> + * i2c-clients or spi-devices from a single fwnode.
> *
> * Copyright 2018 Hans de Goede <hdegoede@redhat.com>
> */
> @@ -14,6 +14,7 @@
> #include <linux/module.h>
> #include <linux/platform_device.h>
> #include <linux/property.h>
> +#include <linux/spi/spi.h>
> #include <linux/types.h>
>
> #define IRQ_RESOURCE_TYPE GENMASK(1, 0)
> @@ -21,15 +22,38 @@
> #define IRQ_RESOURCE_GPIO 1
> #define IRQ_RESOURCE_APIC 2
>
> +enum bmi_bus_type {
> + BMI_I2C,
> + BMI_SPI,
> + BMI_AUTO_DETEC,
you are missing the T of detec_t_ here.
> +};
> +
> +struct bmi_spi_acpi {
> + char *resource_source;
> + struct acpi_resource_spi_serialbus sb;
> +};
> +
> +struct bmi_spi_sb_acpi {
> + int count;
> + struct bmi_spi_acpi acpi_data[];
> +};
> +
> struct bmi_instance {
> const char *type;
> unsigned int flags;
> int irq_idx;
> };
>
> +struct bmi_node {
> + enum bmi_bus_type bus_type;
> + struct bmi_instance instances[];
> +};
> +
> struct bmi {
> int i2c_num;
> + int spi_num;
> struct i2c_client **i2c_devs;
> + struct spi_device **spi_devs;
> };
>
> static int bmi_get_irq(struct platform_device *pdev, struct acpi_device *adev,
> @@ -60,6 +84,230 @@ static void bmi_devs_unregister(struct bmi *bmi)
> {
> while (bmi->i2c_num > 0)
> i2c_unregister_device(bmi->i2c_devs[--bmi->i2c_num]);
> +
> + while (bmi->spi_num > 0)
> + spi_unregister_device(bmi->spi_devs[--bmi->spi_num]);
> +}
> +
> +static int bmi_spi_count(struct acpi_resource *ares, void *data)
> +{
> + struct acpi_resource_spi_serialbus *sb;
> + int *count = data;
> +
> + if (ares->type != ACPI_RESOURCE_TYPE_SERIAL_BUS)
> + return 1;
> +
> + sb = &ares->data.spi_serial_bus;
> + if (sb->type != ACPI_RESOURCE_SERIAL_TYPE_SPI)
> + return 1;
> +
> + *count = *count + 1;
> +
> + return 1;
> +}
> +
> +static int bmi_spi_count_resources(struct acpi_device *adev)
> +{
> + LIST_HEAD(r);
> + int count = 0;
> + int ret;
> +
> + ret = acpi_dev_get_resources(adev, &r, bmi_spi_count, &count);
> + if (ret < 0)
> + return ret;
> +
> + acpi_dev_free_resource_list(&r);
> +
> + return count;
> +}
> +
> +static int bmi_spi_save_res(struct acpi_resource *ares, void *data)
> +{
> + struct acpi_resource_spi_serialbus *sb;
> + struct bmi_spi_sb_acpi *resources = data;
> + struct bmi_spi_acpi *acpi_data;
> +
> + if (ares->type != ACPI_RESOURCE_TYPE_SERIAL_BUS)
> + return 1;
> +
> + sb = &ares->data.spi_serial_bus;
> + if (sb->type != ACPI_RESOURCE_SERIAL_TYPE_SPI)
> + return 1;
> +
> + acpi_data = &resources->acpi_data[resources->count];
> + memcpy(&acpi_data->sb, sb, sizeof(*sb));
> + acpi_data->resource_source = kstrndup(sb->resource_source.string_ptr,
> + sb->resource_source.string_length, GFP_KERNEL);
> + if (!acpi_data->resource_source)
> + return 1;
> + resources->count++;
> +
> + return 1;
> +}
> +
> +static void bmi_spi_res_free(struct bmi_spi_sb_acpi *resources)
> +{
> + if (!resources)
> + return;
> +
> + while (resources->count)
> + kfree(resources->acpi_data[--resources->count].resource_source);
> + kfree(resources);
> +}
This save + free dance here seems over complicated, see below for more on this ...
> +
> +static struct bmi_spi_sb_acpi *bmi_spi_get_resources(struct device *dev,
> + struct acpi_device *adev, int count)
> +{
> + struct bmi_spi_sb_acpi *resources;
> + LIST_HEAD(r);
> + int ret;
> +
> + resources = kmalloc(struct_size(resources, acpi_data, count), GFP_KERNEL);
> + if (!resources)
> + return NULL;
> +
> + ret = acpi_dev_get_resources(adev, &r, bmi_spi_save_res, resources);
> + if (ret < 0)
> + goto error;
> +
> + acpi_dev_free_resource_list(&r);
> +
> + return resources;
> +
> +error:
> + bmi_spi_res_free(resources);
> + return NULL;
> +}
> +
> +static struct spi_controller *bmi_find_spi_controller(char *path)
> +{
> + acpi_handle parent_handle;
> + struct acpi_device *adev;
> + acpi_status status;
> +
> + status = acpi_get_handle(NULL, path, &parent_handle);
> + if (ACPI_FAILURE(status))
> + return NULL;
> +
> + if (acpi_bus_get_device(parent_handle, &adev))
> + return NULL;
> +
> + return acpi_spi_find_controller_by_adev(adev);
> +}
> +
> +/**
> + * bmi_spi_probe - Instantiate multiple SPI devices from inst array
> + * @pdev: Platform device
> + * @adev: ACPI device
> + * @bmi: Internal struct for Bus multi instantiate driver
> + * @inst: Array of instances to probe
> + *
> + * Returns the number of SPI devices instantiate, Zero if none is found or a negative error code.
> + */
> +static int bmi_spi_probe(struct platform_device *pdev, struct acpi_device *adev, struct bmi *bmi,
> + const struct bmi_instance *inst_array)
> +{
> + struct bmi_spi_sb_acpi *acpi_data;
> + struct device *dev = &pdev->dev;
> + struct spi_controller *ctlr;
> + struct spi_device *spi_dev;
> + char name[50];
> + int i, ret, count;
> +
> + ret = bmi_spi_count_resources(adev);
> + if (ret <= 0)
> + return ret;
> + count = ret;
Ok, so why not do the following here instead (and drop a whole bunch of
functions above):
ret = acpi_dev_get_resources(adev, &r, bmi_spi_count, &count);
if (ret < 0)
return ret;
if (count <= 0) {
acpi_dev_free_resource_list(&r);
return count;
}
/* Note we are not freeing the resource list yet here !!! */
> +
> + bmi->spi_devs = devm_kcalloc(dev, count, sizeof(*bmi->spi_devs), GFP_KERNEL);
> + if (!bmi->spi_devs)
> + return -ENOMEM;
> +
> + acpi_data = bmi_spi_get_resources(dev, adev, count);
> + if (!acpi_data)
> + return -ENOMEM;
Remove the bmi_spi_get_resources() call here.
> +
> + for (i = 0; i < count && inst_array[i].type; i++) {
Write a new:
int bmi_get_spi_resource_by_index(list_head *resource_list, struct acpi_resource_spi_serialbus *sb_ret, int index)
{}
Helper which walks the list and fills in *sb_ret with the Nth (matching index) SpiSerialBus resource found in the
list.
And then do:
ret = bmi_get_spi_resource_by_index(&r, &sb, i);
if (ret)
return ret;
ctrl = bmi_find_spi_controller(sb.resource_source.string_ptr);
> + ctlr = bmi_find_spi_controller(acpi_data->acpi_data[i].resource_source);
> + if (!ctlr) {
> + ret = -EPROBE_DEFER;
> + goto error;
> + }
> +
> + spi_dev = spi_alloc_device(ctlr);
> + if (!spi_dev) {
> + dev_err(&ctlr->dev, "failed to allocate SPI device for %s\n",
> + dev_name(&adev->dev));
> + ret = -ENOMEM;
> + goto error;
> + }
> +
> + strscpy(spi_dev->modalias, inst_array[i].type, sizeof(spi_dev->modalias));
> +
And replace all the "acpi_data->acpi_data[i].sb." reference below with simple "sb.".
> + if (ctlr->fw_translate_cs) {
> + ret = ctlr->fw_translate_cs(ctlr,
> + acpi_data->acpi_data[i].sb.device_selection);
> + if (ret < 0) {
> + spi_dev_put(spi_dev);
> + goto error;
> + }
> + spi_dev->chip_select = ret;
> + } else {
> + spi_dev->chip_select = acpi_data->acpi_data[i].sb.device_selection;
> + }
> +
> + spi_dev->max_speed_hz = acpi_data->acpi_data[i].sb.connection_speed;
> + spi_dev->bits_per_word = acpi_data->acpi_data[i].sb.data_bit_length;
> +
> + if (acpi_data->acpi_data[i].sb.clock_phase == ACPI_SPI_SECOND_PHASE)
> + spi_dev->mode |= SPI_CPHA;
> + if (acpi_data->acpi_data[i].sb.clock_polarity == ACPI_SPI_START_HIGH)
> + spi_dev->mode |= SPI_CPOL;
> + if (acpi_data->acpi_data[i].sb.device_polarity == ACPI_SPI_ACTIVE_HIGH)
> + spi_dev->mode |= SPI_CS_HIGH;
> +
> + ret = bmi_get_irq(pdev, adev, &inst_array[i]);
> + if (ret < 0) {
> + spi_dev_put(spi_dev);
> + goto error;
> + }
> + spi_dev->irq = ret;
> +
> + snprintf(name, sizeof(name), "%s-%s-%s.%d", dev_name(&ctlr->dev), dev_name(dev),
> + inst_array[i].type, i);
> + spi_dev->dev.init_name = name;
> +
> + ret = spi_add_device(spi_dev);
> + if (ret) {
> + dev_err(&ctlr->dev, "failed to add SPI device %s from ACPI: %d\n",
> + dev_name(&adev->dev), ret);
> + spi_dev_put(spi_dev);
> + goto error;
> + }
> +
> + dev_dbg(dev, "SPI device %s using chip select %u", name, spi_dev->chip_select);
> +
> + bmi->spi_devs[i] = spi_dev;
> + bmi->spi_num++;
> + }
> +
> + if (bmi->spi_num < count) {
> + dev_err(dev, "Error finding driver, idx %d\n", i);
> + ret = -ENODEV;
> + goto error;
> + }
> +
> + dev_info(dev, "Instantiate %d SPI devices.\n", bmi->spi_num);
And here replace the bmi_spi_res_free(acpi_data); call in both exit paths with:
acpi_dev_free_resource_list(&r); .
To me this way, simply using the already allocated resources from the list,
rather then making a temp copy of them and throwing that away seems like
a simpler solution ?
If you go this route, please also remove the struct bmi_spi_acpi and
struct bmi_spi_sb_acpi data types which you now no longer need.
> + bmi_spi_res_free(acpi_data);
> +
> + return bmi->spi_num;
> +error:
> + bmi_spi_res_free(acpi_data);
> + bmi_devs_unregister(bmi);
> + dev_err_probe(dev, ret, "SPI error %d\n", ret);
> +
> + return ret;
> +
> }
>
> /**
> @@ -125,14 +373,14 @@ static int bmi_i2c_probe(struct platform_device *pdev, struct acpi_device *adev,
>
> static int bmi_probe(struct platform_device *pdev)
> {
> - const struct bmi_instance *inst_array;
> struct device *dev = &pdev->dev;
> + const struct bmi_node *node;
> struct acpi_device *adev;
> struct bmi *bmi;
> - int ret;
> + int i2c_ret = 0, spi_ret = 0;
>
> - inst_array = device_get_match_data(dev);
> - if (!inst_array) {
> + node = device_get_match_data(dev);
> + if (!node) {
> dev_err(dev, "Error ACPI match data is missing\n");
> return -ENODEV;
> }
> @@ -147,13 +395,44 @@ static int bmi_probe(struct platform_device *pdev)
>
> platform_set_drvdata(pdev, bmi);
>
> - ret = bmi_i2c_probe(pdev, adev, bmi, inst_array);
> - if (ret > 0)
> + /* Each time this driver probes only one type of bus will be chosen.
> + * And I2C has preference, which means that if find a I2cSerialBus it assumes
> + * that all following devices will also be I2C.
> + * In case there are zero I2C devices, it assumes that all following devices are SPI.
> + */
> + if (node->bus_type != BMI_SPI) {
> + i2c_ret = bmi_i2c_probe(pdev, adev, bmi, node->instances);
> + if (i2c_ret > 0)
> + return 0;
> + else if (i2c_ret == -EPROBE_DEFER)
> + return i2c_ret;
> + if (node->bus_type == BMI_I2C) {
> + if (i2c_ret == 0)
> + return -ENODEV;
> + else
> + return i2c_ret;
> + }
> + }
> + /* BMI_SPI or BMI_AUTO_DETEC */
auto-detec_t_
> + spi_ret = bmi_spi_probe(pdev, adev, bmi, node->instances);
> + if (spi_ret > 0)
> return 0;
> - if (ret == 0)
> - ret = -ENODEV;
> + else if (spi_ret == -EPROBE_DEFER)
> + return -EPROBE_DEFER;
> + if (node->bus_type == BMI_SPI) {
> + if (spi_ret == 0)
> + return -ENODEV;
> + else
> + return spi_ret;
> + }
>
> - return ret;
> + /* The only way to get here is BMI_AUTO_DETEC and i2c_ret <= 0 and spi_ret <= 0 */
> + if (i2c_ret == 0 && spi_ret == 0)
> + return -ENODEV;
> + else if (i2c_ret == 0 && spi_ret)
> + return spi_ret;
> +
> + return i2c_ret;
> }
>
> static int bmi_remove(struct platform_device *pdev)
> @@ -165,27 +444,33 @@ static int bmi_remove(struct platform_device *pdev)
> return 0;
> }
>
> -static const struct bmi_instance bsg1160_data[] = {
> - { "bmc150_accel", IRQ_RESOURCE_GPIO, 0 },
> - { "bmc150_magn" },
> - { "bmg160" },
> - {}
> +static const struct bmi_node bsg1160_data = {
Please explicitly set ".type = BMI_I2C" here and for all the
other already existing bmi_node structs.
> + .instances = {
> + { "bmc150_accel", IRQ_RESOURCE_GPIO, 0 },
> + { "bmc150_magn" },
> + { "bmg160" },
> + {}
> + },
> };
>
> -static const struct bmi_instance bsg2150_data[] = {
> - { "bmc150_accel", IRQ_RESOURCE_GPIO, 0 },
> - { "bmc150_magn" },
> - /* The resources describe a 3th client, but it is not really there. */
> - { "bsg2150_dummy_dev" },
> - {}
> +static const struct bmi_node bsg2150_data = {
> + .instances = {
> + { "bmc150_accel", IRQ_RESOURCE_GPIO, 0 },
> + { "bmc150_magn" },
> + /* The resources describe a 3th client, but it is not really there. */
> + { "bsg2150_dummy_dev" },
> + {}
> + },
> };
>
> -static const struct bmi_instance int3515_data[] = {
> - { "tps6598x", IRQ_RESOURCE_APIC, 0 },
> - { "tps6598x", IRQ_RESOURCE_APIC, 1 },
> - { "tps6598x", IRQ_RESOURCE_APIC, 2 },
> - { "tps6598x", IRQ_RESOURCE_APIC, 3 },
> - {}
> +static const struct bmi_node int3515_data = {
> + .instances = {
> + { "tps6598x", IRQ_RESOURCE_APIC, 0 },
> + { "tps6598x", IRQ_RESOURCE_APIC, 1 },
> + { "tps6598x", IRQ_RESOURCE_APIC, 2 },
> + { "tps6598x", IRQ_RESOURCE_APIC, 3 },
> + {}
> + },
> };
>
> /*
> @@ -193,9 +478,9 @@ static const struct bmi_instance int3515_data[] = {
> * drivers/acpi/scan.c: acpi_device_enumeration_by_parent().
> */
> static const struct acpi_device_id bmi_acpi_ids[] = {
> - { "BSG1160", (unsigned long)bsg1160_data },
> - { "BSG2150", (unsigned long)bsg2150_data },
> - { "INT3515", (unsigned long)int3515_data },
> + { "BSG1160", (unsigned long)&bsg1160_data },
> + { "BSG2150", (unsigned long)&bsg2150_data },
> + { "INT3515", (unsigned long)&int3515_data },
> { }
> };
> MODULE_DEVICE_TABLE(acpi, bmi_acpi_ids);
> diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
> index 969d8138d019..8b937fc20d23 100644
> --- a/drivers/acpi/scan.c
> +++ b/drivers/acpi/scan.c
> @@ -1696,12 +1696,13 @@ static bool acpi_device_enumeration_by_parent(struct acpi_device *device)
> struct list_head resource_list;
> bool is_serial_bus_slave = false;
> /*
> - * These devices have multiple I2cSerialBus resources and an i2c-client
> - * must be instantiated for each, each with its own i2c_device_id.
> - * Normally we only instantiate an i2c-client for the first resource,
> - * using the ACPI HID as id. These special cases are handled by the
> - * drivers/acpi/bus-multi-instantiate.c driver, which knows
> - * which i2c_device_id to use for each resource.
> + * These devices have multiple I2cSerialBus/SpiSerialBus resources
> + * and an (i2c/spi)-client must be instantiated for each, each with
> + * its own i2c_device_id/spi_device_id.
> + * Normally we only instantiate an (i2c/spi)-client for the first
> + * resource, using the ACPI HID as id. These special cases are handled
> + * by the drivers/acpi/bus-multi-instantiate.c driver, which
> + * knows which i2c_device_id or spi_device_id to use for each resource.
> */
> static const struct acpi_device_id bus_multi_instantiate_ids[] = {
> {"BSG1160", },
>
Regards,
Hans
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [PATCH v2 6/6] ACPI: bus-multi-instantiate: Add SPI support
2021-12-21 18:32 ` Hans de Goede
@ 2021-12-21 19:22 ` Hans de Goede
2022-01-10 14:36 ` Stefan Binding
1 sibling, 0 replies; 17+ messages in thread
From: Hans de Goede @ 2021-12-21 19:22 UTC (permalink / raw)
To: Stefan Binding, Mark Brown, Rafael J . Wysocki, Len Brown, Mark Gross
Cc: linux-kernel, linux-spi, linux-acpi, platform-driver-x86, patches
Hi,
On 12/21/21 19:32, Hans de Goede wrote:
> Hi,
>
> On 12/10/21 16:40, Stefan Binding wrote:
>> Add support for spi bus in bus-multi-instantiate driver
>>
>> Some peripherals can have either a I2C or a SPI connection
>> to the host (but not both) but use the same HID for both
>> types. So it is not possible to use the HID to determine
>> whether it is I2C or SPI. The driver must check the node
>> to see if it contains I2cSerialBus or SpiSerialBus entries.
>>
>> For backwards-compatibility with the existing nodes I2C is
>> checked first and if such entries are found ONLY I2C devices
>> are created. Since some existing nodes that were already
>> handled by this driver could also contain unrelated
>> SpiSerialBus nodes that were previously ignored, and this
>> preserves that behavior. If there is ever a need to handle
>> a node where both I2C and SPI devices must be instantiated
>> this can be added in future.
>>
>> Signed-off-by: Stefan Binding <sbinding@opensource.cirrus.com>
>> ---
>> drivers/acpi/Kconfig | 10 +-
>> drivers/acpi/bus-multi-instantiate.c | 345 ++++++++++++++++++++++++---
>> drivers/acpi/scan.c | 13 +-
>> 3 files changed, 327 insertions(+), 41 deletions(-)
>>
>> diff --git a/drivers/acpi/Kconfig b/drivers/acpi/Kconfig
>> index 6ba47dd39eb4..948f39d55595 100644
>> --- a/drivers/acpi/Kconfig
>> +++ b/drivers/acpi/Kconfig
>> @@ -296,12 +296,12 @@ config ACPI_PROCESSOR
>> the module will be called processor.
>>
>> config ACPI_BUS_MULTI_INST
>> - tristate "I2C multi instantiate pseudo device driver"
>> - depends on I2C
>> + tristate "I2C and SPI multi instantiate pseudo device driver"
>> + depends on I2C && SPI
>> help
>> - Some ACPI-based systems list multiple i2c-devices in a single ACPI
>> - firmware-node. This driver will instantiate separate i2c-clients
>> - for each device in the firmware-node.
>> + Some ACPI-based systems list multiple i2c/spi devices in a
>> + single ACPI firmware-node. This driver will instantiate separate
>> + i2c-clients or spi-devices for each device in the firmware-node.
>>
>> To compile this driver as a module, choose M here: the module
>> will be called bus-multi-instantiate.
>> diff --git a/drivers/acpi/bus-multi-instantiate.c b/drivers/acpi/bus-multi-instantiate.c
>> index 50f1540762e9..c1306a0ee13c 100644
>> --- a/drivers/acpi/bus-multi-instantiate.c
>> +++ b/drivers/acpi/bus-multi-instantiate.c
>> @@ -1,7 +1,7 @@
>> // SPDX-License-Identifier: GPL-2.0+
>> /*
>> * Bus multi-instantiate driver, pseudo driver to instantiate multiple
>> - * i2c-clients from a single fwnode.
>> + * i2c-clients or spi-devices from a single fwnode.
>> *
>> * Copyright 2018 Hans de Goede <hdegoede@redhat.com>
>> */
>> @@ -14,6 +14,7 @@
>> #include <linux/module.h>
>> #include <linux/platform_device.h>
>> #include <linux/property.h>
>> +#include <linux/spi/spi.h>
>> #include <linux/types.h>
>>
>> #define IRQ_RESOURCE_TYPE GENMASK(1, 0)
>> @@ -21,15 +22,38 @@
>> #define IRQ_RESOURCE_GPIO 1
>> #define IRQ_RESOURCE_APIC 2
>>
>> +enum bmi_bus_type {
>> + BMI_I2C,
>> + BMI_SPI,
>> + BMI_AUTO_DETEC,
>
> you are missing the T of detec_t_ here.
>
>> +};
>> +
>> +struct bmi_spi_acpi {
>> + char *resource_source;
>> + struct acpi_resource_spi_serialbus sb;
>> +};
>> +
>> +struct bmi_spi_sb_acpi {
>> + int count;
>> + struct bmi_spi_acpi acpi_data[];
>> +};
>> +
>> struct bmi_instance {
>> const char *type;
>> unsigned int flags;
>> int irq_idx;
>> };
>>
>> +struct bmi_node {
>> + enum bmi_bus_type bus_type;
>> + struct bmi_instance instances[];
>> +};
>> +
>> struct bmi {
>> int i2c_num;
>> + int spi_num;
>> struct i2c_client **i2c_devs;
>> + struct spi_device **spi_devs;
>> };
>>
>> static int bmi_get_irq(struct platform_device *pdev, struct acpi_device *adev,
>> @@ -60,6 +84,230 @@ static void bmi_devs_unregister(struct bmi *bmi)
>> {
>> while (bmi->i2c_num > 0)
>> i2c_unregister_device(bmi->i2c_devs[--bmi->i2c_num]);
>> +
>> + while (bmi->spi_num > 0)
>> + spi_unregister_device(bmi->spi_devs[--bmi->spi_num]);
>> +}
>> +
>> +static int bmi_spi_count(struct acpi_resource *ares, void *data)
>> +{
>> + struct acpi_resource_spi_serialbus *sb;
>> + int *count = data;
>> +
>> + if (ares->type != ACPI_RESOURCE_TYPE_SERIAL_BUS)
>> + return 1;
>> +
>> + sb = &ares->data.spi_serial_bus;
>> + if (sb->type != ACPI_RESOURCE_SERIAL_TYPE_SPI)
>> + return 1;
>> +
>> + *count = *count + 1;
>> +
>> + return 1;
>> +}
>> +
>> +static int bmi_spi_count_resources(struct acpi_device *adev)
>> +{
>> + LIST_HEAD(r);
>> + int count = 0;
>> + int ret;
>> +
>> + ret = acpi_dev_get_resources(adev, &r, bmi_spi_count, &count);
>> + if (ret < 0)
>> + return ret;
>> +
>> + acpi_dev_free_resource_list(&r);
>> +
>> + return count;
>> +}
>> +
>> +static int bmi_spi_save_res(struct acpi_resource *ares, void *data)
>> +{
>> + struct acpi_resource_spi_serialbus *sb;
>> + struct bmi_spi_sb_acpi *resources = data;
>> + struct bmi_spi_acpi *acpi_data;
>> +
>> + if (ares->type != ACPI_RESOURCE_TYPE_SERIAL_BUS)
>> + return 1;
>> +
>> + sb = &ares->data.spi_serial_bus;
>> + if (sb->type != ACPI_RESOURCE_SERIAL_TYPE_SPI)
>> + return 1;
>> +
>> + acpi_data = &resources->acpi_data[resources->count];
>> + memcpy(&acpi_data->sb, sb, sizeof(*sb));
>> + acpi_data->resource_source = kstrndup(sb->resource_source.string_ptr,
>> + sb->resource_source.string_length, GFP_KERNEL);
>> + if (!acpi_data->resource_source)
>> + return 1;
>> + resources->count++;
>> +
>> + return 1;
>> +}
>> +
>> +static void bmi_spi_res_free(struct bmi_spi_sb_acpi *resources)
>> +{
>> + if (!resources)
>> + return;
>> +
>> + while (resources->count)
>> + kfree(resources->acpi_data[--resources->count].resource_source);
>> + kfree(resources);
>> +}
>
> This save + free dance here seems over complicated, see below for more on this ...
>
>> +
>> +static struct bmi_spi_sb_acpi *bmi_spi_get_resources(struct device *dev,
>> + struct acpi_device *adev, int count)
>> +{
>> + struct bmi_spi_sb_acpi *resources;
>> + LIST_HEAD(r);
>> + int ret;
>> +
>> + resources = kmalloc(struct_size(resources, acpi_data, count), GFP_KERNEL);
>> + if (!resources)
>> + return NULL;
>> +
>> + ret = acpi_dev_get_resources(adev, &r, bmi_spi_save_res, resources);
>> + if (ret < 0)
>> + goto error;
>> +
>> + acpi_dev_free_resource_list(&r);
>> +
>> + return resources;
>> +
>> +error:
>> + bmi_spi_res_free(resources);
>> + return NULL;
>> +}
>> +
>> +static struct spi_controller *bmi_find_spi_controller(char *path)
>> +{
>> + acpi_handle parent_handle;
>> + struct acpi_device *adev;
>> + acpi_status status;
>> +
>> + status = acpi_get_handle(NULL, path, &parent_handle);
>> + if (ACPI_FAILURE(status))
>> + return NULL;
>> +
>> + if (acpi_bus_get_device(parent_handle, &adev))
>> + return NULL;
>> +
>> + return acpi_spi_find_controller_by_adev(adev);
>> +}
>> +
>> +/**
>> + * bmi_spi_probe - Instantiate multiple SPI devices from inst array
>> + * @pdev: Platform device
>> + * @adev: ACPI device
>> + * @bmi: Internal struct for Bus multi instantiate driver
>> + * @inst: Array of instances to probe
>> + *
>> + * Returns the number of SPI devices instantiate, Zero if none is found or a negative error code.
>> + */
>> +static int bmi_spi_probe(struct platform_device *pdev, struct acpi_device *adev, struct bmi *bmi,
>> + const struct bmi_instance *inst_array)
>> +{
>> + struct bmi_spi_sb_acpi *acpi_data;
>> + struct device *dev = &pdev->dev;
>> + struct spi_controller *ctlr;
>> + struct spi_device *spi_dev;
>> + char name[50];
>> + int i, ret, count;
>> +
>
>
>
>> + ret = bmi_spi_count_resources(adev);
>> + if (ret <= 0)
>> + return ret;
>> + count = ret;
>
> Ok, so why not do the following here instead (and drop a whole bunch of
> functions above):
>
> ret = acpi_dev_get_resources(adev, &r, bmi_spi_count, &count);
> if (ret < 0)
> return ret;
>
> if (count <= 0) {
> acpi_dev_free_resource_list(&r);
> return count;
> }
>
> /* Note we are not freeing the resource list yet here !!! */
>
>> +
>> + bmi->spi_devs = devm_kcalloc(dev, count, sizeof(*bmi->spi_devs), GFP_KERNEL);
>> + if (!bmi->spi_devs)
>> + return -ENOMEM;
>> +
>> + acpi_data = bmi_spi_get_resources(dev, adev, count);
>> + if (!acpi_data)
>> + return -ENOMEM;
>
> Remove the bmi_spi_get_resources() call here.
>
>> +
>> + for (i = 0; i < count && inst_array[i].type; i++) {
>
> Write a new:
>
> int bmi_get_spi_resource_by_index(list_head *resource_list, struct acpi_resource_spi_serialbus *sb_ret, int index)
> {}
>
> Helper which walks the list and fills in *sb_ret with the Nth (matching index) SpiSerialBus resource found in the
> list.
>
> And then do:
>
> ret = bmi_get_spi_resource_by_index(&r, &sb, i);
> if (ret)
> return ret;
>
> ctrl = bmi_find_spi_controller(sb.resource_source.string_ptr);
>
>
>> + ctlr = bmi_find_spi_controller(acpi_data->acpi_data[i].resource_source);
>> + if (!ctlr) {
>> + ret = -EPROBE_DEFER;
>> + goto error;
>> + }
>> +
>> + spi_dev = spi_alloc_device(ctlr);
>> + if (!spi_dev) {
>> + dev_err(&ctlr->dev, "failed to allocate SPI device for %s\n",
>> + dev_name(&adev->dev));
>> + ret = -ENOMEM;
>> + goto error;
>> + }
>> +
>> + strscpy(spi_dev->modalias, inst_array[i].type, sizeof(spi_dev->modalias));
>> +
>
> And replace all the "acpi_data->acpi_data[i].sb." reference below with simple "sb.".
>
>
>> + if (ctlr->fw_translate_cs) {
>> + ret = ctlr->fw_translate_cs(ctlr,
>> + acpi_data->acpi_data[i].sb.device_selection);
>> + if (ret < 0) {
>> + spi_dev_put(spi_dev);
>> + goto error;
>> + }
>> + spi_dev->chip_select = ret;
>> + } else {
>> + spi_dev->chip_select = acpi_data->acpi_data[i].sb.device_selection;
>> + }
>> +
>> + spi_dev->max_speed_hz = acpi_data->acpi_data[i].sb.connection_speed;
>> + spi_dev->bits_per_word = acpi_data->acpi_data[i].sb.data_bit_length;
>> +
>> + if (acpi_data->acpi_data[i].sb.clock_phase == ACPI_SPI_SECOND_PHASE)
>> + spi_dev->mode |= SPI_CPHA;
>> + if (acpi_data->acpi_data[i].sb.clock_polarity == ACPI_SPI_START_HIGH)
>> + spi_dev->mode |= SPI_CPOL;
>> + if (acpi_data->acpi_data[i].sb.device_polarity == ACPI_SPI_ACTIVE_HIGH)
>> + spi_dev->mode |= SPI_CS_HIGH;
>> +
>> + ret = bmi_get_irq(pdev, adev, &inst_array[i]);
>> + if (ret < 0) {
>> + spi_dev_put(spi_dev);
>> + goto error;
>> + }
>> + spi_dev->irq = ret;
>> +
>> + snprintf(name, sizeof(name), "%s-%s-%s.%d", dev_name(&ctlr->dev), dev_name(dev),
>> + inst_array[i].type, i);
>> + spi_dev->dev.init_name = name;
>> +
>> + ret = spi_add_device(spi_dev);
>> + if (ret) {
>> + dev_err(&ctlr->dev, "failed to add SPI device %s from ACPI: %d\n",
>> + dev_name(&adev->dev), ret);
>> + spi_dev_put(spi_dev);
>> + goto error;
>> + }
>> +
>> + dev_dbg(dev, "SPI device %s using chip select %u", name, spi_dev->chip_select);
>> +
>> + bmi->spi_devs[i] = spi_dev;
>> + bmi->spi_num++;
>> + }
>> +
>> + if (bmi->spi_num < count) {
>> + dev_err(dev, "Error finding driver, idx %d\n", i);
>> + ret = -ENODEV;
>> + goto error;
>> + }
>> +
>> + dev_info(dev, "Instantiate %d SPI devices.\n", bmi->spi_num);
>
> And here replace the bmi_spi_res_free(acpi_data); call in both exit paths with:
> acpi_dev_free_resource_list(&r); .
>
> To me this way, simply using the already allocated resources from the list,
> rather then making a temp copy of them and throwing that away seems like
> a simpler solution ?
>
> If you go this route, please also remove the struct bmi_spi_acpi and
> struct bmi_spi_sb_acpi data types which you now no longer need.
So thinking a bit more about this, then looking up the nth SpiSerialBus
resource, and then turning that into a spi_client is something which
the SPI core ACPI code should already be doing for index==0. So I think
that you should be able to modify the SPI core ACPI code to take index
as a parameter and then have it export a helper for this which you
can use rather then duplicate the SPI core ACPI code ? Note this is
also what the I2C code is already doing.
And if you go that route you may also want to consider to add the SPI
equivalent of the i2c_acpi_client_count() helper.
Regards,
hans
>
>> + bmi_spi_res_free(acpi_data);
>> +
>> + return bmi->spi_num;
>> +error:
>> + bmi_spi_res_free(acpi_data);
>> + bmi_devs_unregister(bmi);
>> + dev_err_probe(dev, ret, "SPI error %d\n", ret);
>> +
>> + return ret;
>> +
>> }
>>
>> /**
>> @@ -125,14 +373,14 @@ static int bmi_i2c_probe(struct platform_device *pdev, struct acpi_device *adev,
>>
>> static int bmi_probe(struct platform_device *pdev)
>> {
>> - const struct bmi_instance *inst_array;
>> struct device *dev = &pdev->dev;
>> + const struct bmi_node *node;
>> struct acpi_device *adev;
>> struct bmi *bmi;
>> - int ret;
>> + int i2c_ret = 0, spi_ret = 0;
>>
>> - inst_array = device_get_match_data(dev);
>> - if (!inst_array) {
>> + node = device_get_match_data(dev);
>> + if (!node) {
>> dev_err(dev, "Error ACPI match data is missing\n");
>> return -ENODEV;
>> }
>> @@ -147,13 +395,44 @@ static int bmi_probe(struct platform_device *pdev)
>>
>> platform_set_drvdata(pdev, bmi);
>>
>> - ret = bmi_i2c_probe(pdev, adev, bmi, inst_array);
>> - if (ret > 0)
>> + /* Each time this driver probes only one type of bus will be chosen.
>> + * And I2C has preference, which means that if find a I2cSerialBus it assumes
>> + * that all following devices will also be I2C.
>> + * In case there are zero I2C devices, it assumes that all following devices are SPI.
>> + */
>> + if (node->bus_type != BMI_SPI) {
>> + i2c_ret = bmi_i2c_probe(pdev, adev, bmi, node->instances);
>> + if (i2c_ret > 0)
>> + return 0;
>> + else if (i2c_ret == -EPROBE_DEFER)
>> + return i2c_ret;
>> + if (node->bus_type == BMI_I2C) {
>> + if (i2c_ret == 0)
>> + return -ENODEV;
>> + else
>> + return i2c_ret;
>> + }
>> + }
>> + /* BMI_SPI or BMI_AUTO_DETEC */
>
> auto-detec_t_
>
>> + spi_ret = bmi_spi_probe(pdev, adev, bmi, node->instances);
>> + if (spi_ret > 0)
>> return 0;
>> - if (ret == 0)
>> - ret = -ENODEV;
>> + else if (spi_ret == -EPROBE_DEFER)
>> + return -EPROBE_DEFER;
>> + if (node->bus_type == BMI_SPI) {
>> + if (spi_ret == 0)
>> + return -ENODEV;
>> + else
>> + return spi_ret;
>> + }
>>
>> - return ret;
>> + /* The only way to get here is BMI_AUTO_DETEC and i2c_ret <= 0 and spi_ret <= 0 */
>> + if (i2c_ret == 0 && spi_ret == 0)
>> + return -ENODEV;
>> + else if (i2c_ret == 0 && spi_ret)
>> + return spi_ret;
>> +
>> + return i2c_ret;
>> }
>>
>> static int bmi_remove(struct platform_device *pdev)
>> @@ -165,27 +444,33 @@ static int bmi_remove(struct platform_device *pdev)
>> return 0;
>> }
>>
>> -static const struct bmi_instance bsg1160_data[] = {
>> - { "bmc150_accel", IRQ_RESOURCE_GPIO, 0 },
>> - { "bmc150_magn" },
>> - { "bmg160" },
>> - {}
>> +static const struct bmi_node bsg1160_data = {
>
> Please explicitly set ".type = BMI_I2C" here and for all the
> other already existing bmi_node structs.
>
>> + .instances = {
>> + { "bmc150_accel", IRQ_RESOURCE_GPIO, 0 },
>> + { "bmc150_magn" },
>> + { "bmg160" },
>> + {}
>> + },
>> };
>>
>> -static const struct bmi_instance bsg2150_data[] = {
>> - { "bmc150_accel", IRQ_RESOURCE_GPIO, 0 },
>> - { "bmc150_magn" },
>> - /* The resources describe a 3th client, but it is not really there. */
>> - { "bsg2150_dummy_dev" },
>> - {}
>> +static const struct bmi_node bsg2150_data = {
>> + .instances = {
>> + { "bmc150_accel", IRQ_RESOURCE_GPIO, 0 },
>> + { "bmc150_magn" },
>> + /* The resources describe a 3th client, but it is not really there. */
>> + { "bsg2150_dummy_dev" },
>> + {}
>> + },
>> };
>>
>> -static const struct bmi_instance int3515_data[] = {
>> - { "tps6598x", IRQ_RESOURCE_APIC, 0 },
>> - { "tps6598x", IRQ_RESOURCE_APIC, 1 },
>> - { "tps6598x", IRQ_RESOURCE_APIC, 2 },
>> - { "tps6598x", IRQ_RESOURCE_APIC, 3 },
>> - {}
>> +static const struct bmi_node int3515_data = {
>> + .instances = {
>> + { "tps6598x", IRQ_RESOURCE_APIC, 0 },
>> + { "tps6598x", IRQ_RESOURCE_APIC, 1 },
>> + { "tps6598x", IRQ_RESOURCE_APIC, 2 },
>> + { "tps6598x", IRQ_RESOURCE_APIC, 3 },
>> + {}
>> + },
>> };
>>
>> /*
>> @@ -193,9 +478,9 @@ static const struct bmi_instance int3515_data[] = {
>> * drivers/acpi/scan.c: acpi_device_enumeration_by_parent().
>> */
>> static const struct acpi_device_id bmi_acpi_ids[] = {
>> - { "BSG1160", (unsigned long)bsg1160_data },
>> - { "BSG2150", (unsigned long)bsg2150_data },
>> - { "INT3515", (unsigned long)int3515_data },
>> + { "BSG1160", (unsigned long)&bsg1160_data },
>> + { "BSG2150", (unsigned long)&bsg2150_data },
>> + { "INT3515", (unsigned long)&int3515_data },
>> { }
>> };
>> MODULE_DEVICE_TABLE(acpi, bmi_acpi_ids);
>> diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
>> index 969d8138d019..8b937fc20d23 100644
>> --- a/drivers/acpi/scan.c
>> +++ b/drivers/acpi/scan.c
>> @@ -1696,12 +1696,13 @@ static bool acpi_device_enumeration_by_parent(struct acpi_device *device)
>> struct list_head resource_list;
>> bool is_serial_bus_slave = false;
>> /*
>> - * These devices have multiple I2cSerialBus resources and an i2c-client
>> - * must be instantiated for each, each with its own i2c_device_id.
>> - * Normally we only instantiate an i2c-client for the first resource,
>> - * using the ACPI HID as id. These special cases are handled by the
>> - * drivers/acpi/bus-multi-instantiate.c driver, which knows
>> - * which i2c_device_id to use for each resource.
>> + * These devices have multiple I2cSerialBus/SpiSerialBus resources
>> + * and an (i2c/spi)-client must be instantiated for each, each with
>> + * its own i2c_device_id/spi_device_id.
>> + * Normally we only instantiate an (i2c/spi)-client for the first
>> + * resource, using the ACPI HID as id. These special cases are handled
>> + * by the drivers/acpi/bus-multi-instantiate.c driver, which
>> + * knows which i2c_device_id or spi_device_id to use for each resource.
>> */
>> static const struct acpi_device_id bus_multi_instantiate_ids[] = {
>> {"BSG1160", },
>>
>
>
> Regards,
>
> Hans
>
^ permalink raw reply [flat|nested] 17+ messages in thread* RE: [PATCH v2 6/6] ACPI: bus-multi-instantiate: Add SPI support
2021-12-21 18:32 ` Hans de Goede
2021-12-21 19:22 ` Hans de Goede
@ 2022-01-10 14:36 ` Stefan Binding
2022-01-11 9:20 ` Hans de Goede
1 sibling, 1 reply; 17+ messages in thread
From: Stefan Binding @ 2022-01-10 14:36 UTC (permalink / raw)
To: 'Hans de Goede', 'Mark Brown',
'Rafael J . Wysocki', 'Len Brown',
'Mark Gross'
Cc: linux-kernel, linux-spi, linux-acpi, platform-driver-x86, patches
Hi,
> -----Original Message-----
> From: Hans de Goede <hdegoede@redhat.com>
> Sent: 21 December 2021 18:32
> To: Stefan Binding <sbinding@opensource.cirrus.com>; Mark Brown
> <broonie@kernel.org>; Rafael J . Wysocki <rafael@kernel.org>; Len Brown
> <lenb@kernel.org>; Mark Gross <markgross@kernel.org>
> Cc: linux-kernel@vger.kernel.org; linux-spi@vger.kernel.org; linux-
> acpi@vger.kernel.org; platform-driver-x86@vger.kernel.org;
> patches@opensource.cirrus.com
> Subject: Re: [PATCH v2 6/6] ACPI: bus-multi-instantiate: Add SPI support
> > + ret = bmi_spi_count_resources(adev);
> > + if (ret <= 0)
> > + return ret;
> > + count = ret;
>
> Ok, so why not do the following here instead (and drop a whole bunch of
> functions above):
>
> ret = acpi_dev_get_resources(adev, &r, bmi_spi_count, &count);
> if (ret < 0)
> return ret;
>
> if (count <= 0) {
> acpi_dev_free_resource_list(&r);
> return count;
> }
>
> /* Note we are not freeing the resource list yet here !!! */
>
> > +
> > + bmi->spi_devs = devm_kcalloc(dev, count, sizeof(*bmi->spi_devs),
> GFP_KERNEL);
> > + if (!bmi->spi_devs)
> > + return -ENOMEM;
> > +
> > + acpi_data = bmi_spi_get_resources(dev, adev, count);
> > + if (!acpi_data)
> > + return -ENOMEM;
>
> Remove the bmi_spi_get_resources() call here.
>
> > +
> > + for (i = 0; i < count && inst_array[i].type; i++) {
>
> Write a new:
>
> int bmi_get_spi_resource_by_index(list_head *resource_list, struct
> acpi_resource_spi_serialbus *sb_ret, int index)
> {}
>
> Helper which walks the list and fills in *sb_ret with the Nth (matching index)
> SpiSerialBus resource found in the
> list.
>
> And then do:
>
> ret = bmi_get_spi_resource_by_index(&r, &sb, i);
> if (ret)
> return ret;
>
> ctrl =
> bmi_find_spi_controller(sb.resource_source.string_ptr);
>
>
> > + ctlr = bmi_find_spi_controller(acpi_data-
> >acpi_data[i].resource_source);
> > + if (!ctlr) {
> > + ret = -EPROBE_DEFER;
> > + goto error;
> > + }
> > +
> > + spi_dev = spi_alloc_device(ctlr);
> > + if (!spi_dev) {
> > + dev_err(&ctlr->dev, "failed to allocate SPI device for
> %s\n",
> > + dev_name(&adev->dev));
> > + ret = -ENOMEM;
> > + goto error;
> > + }
> > +
> > + strscpy(spi_dev->modalias, inst_array[i].type,
> sizeof(spi_dev->modalias));
> > +
>
> And replace all the "acpi_data->acpi_data[i].sb." reference below with
> simple "sb.".
>
>
> > + if (ctlr->fw_translate_cs) {
> > + ret = ctlr->fw_translate_cs(ctlr,
> > + acpi_data-
> >acpi_data[i].sb.device_selection);
> > + if (ret < 0) {
> > + spi_dev_put(spi_dev);
> > + goto error;
> > + }
> > + spi_dev->chip_select = ret;
> > + } else {
> > + spi_dev->chip_select = acpi_data-
> >acpi_data[i].sb.device_selection;
> > + }
> > +
> > + spi_dev->max_speed_hz = acpi_data-
> >acpi_data[i].sb.connection_speed;
> > + spi_dev->bits_per_word = acpi_data-
> >acpi_data[i].sb.data_bit_length;
> > +
> > + if (acpi_data->acpi_data[i].sb.clock_phase ==
> ACPI_SPI_SECOND_PHASE)
> > + spi_dev->mode |= SPI_CPHA;
> > + if (acpi_data->acpi_data[i].sb.clock_polarity ==
> ACPI_SPI_START_HIGH)
> > + spi_dev->mode |= SPI_CPOL;
> > + if (acpi_data->acpi_data[i].sb.device_polarity ==
> ACPI_SPI_ACTIVE_HIGH)
> > + spi_dev->mode |= SPI_CS_HIGH;
> > +
> > + ret = bmi_get_irq(pdev, adev, &inst_array[i]);
> > + if (ret < 0) {
> > + spi_dev_put(spi_dev);
> > + goto error;
> > + }
> > + spi_dev->irq = ret;
> > +
> > + snprintf(name, sizeof(name), "%s-%s-%s.%d",
> dev_name(&ctlr->dev), dev_name(dev),
> > + inst_array[i].type, i);
> > + spi_dev->dev.init_name = name;
> > +
> > + ret = spi_add_device(spi_dev);
> > + if (ret) {
> > + dev_err(&ctlr->dev, "failed to add SPI device %s from
> ACPI: %d\n",
> > + dev_name(&adev->dev), ret);
> > + spi_dev_put(spi_dev);
> > + goto error;
> > + }
> > +
> > + dev_dbg(dev, "SPI device %s using chip select %u", name,
> spi_dev->chip_select);
> > +
> > + bmi->spi_devs[i] = spi_dev;
> > + bmi->spi_num++;
> > + }
> > +
> > + if (bmi->spi_num < count) {
> > + dev_err(dev, "Error finding driver, idx %d\n", i);
> > + ret = -ENODEV;
> > + goto error;
> > + }
> > +
> > + dev_info(dev, "Instantiate %d SPI devices.\n", bmi->spi_num);
>
> And here replace the bmi_spi_res_free(acpi_data); call in both exit paths
> with:
> acpi_dev_free_resource_list(&r); .
>
> To me this way, simply using the already allocated resources from the list,
> rather then making a temp copy of them and throwing that away seems like
> a simpler solution ?
>
> If you go this route, please also remove the struct bmi_spi_acpi and
> struct bmi_spi_sb_acpi data types which you now no longer need.
>
I tried to implement this idea, and reuse the resource list, but I hit an issue.
The resources saved in the list are not "struct acpi_resource", but instead the
generic "struct resource".
We need the acpi_resource structure to pull the parameters from to be able to
create the spi devices.
As far as I know there is no way to convert the "struct resource" into a
"struct acpi_resource". Is there another way to do this?
Thanks,
Stefan
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [PATCH v2 6/6] ACPI: bus-multi-instantiate: Add SPI support
2022-01-10 14:36 ` Stefan Binding
@ 2022-01-11 9:20 ` Hans de Goede
0 siblings, 0 replies; 17+ messages in thread
From: Hans de Goede @ 2022-01-11 9:20 UTC (permalink / raw)
To: Stefan Binding, 'Mark Brown',
'Rafael J . Wysocki', 'Len Brown',
'Mark Gross'
Cc: linux-kernel, linux-spi, linux-acpi, platform-driver-x86, patches
Hi,
On 1/10/22 15:36, Stefan Binding wrote:
> Hi,
>
>> -----Original Message-----
>> From: Hans de Goede <hdegoede@redhat.com>
>> Sent: 21 December 2021 18:32
>> To: Stefan Binding <sbinding@opensource.cirrus.com>; Mark Brown
>> <broonie@kernel.org>; Rafael J . Wysocki <rafael@kernel.org>; Len Brown
>> <lenb@kernel.org>; Mark Gross <markgross@kernel.org>
>> Cc: linux-kernel@vger.kernel.org; linux-spi@vger.kernel.org; linux-
>> acpi@vger.kernel.org; platform-driver-x86@vger.kernel.org;
>> patches@opensource.cirrus.com
>> Subject: Re: [PATCH v2 6/6] ACPI: bus-multi-instantiate: Add SPI support
>
>>> + ret = bmi_spi_count_resources(adev);
>>> + if (ret <= 0)
>>> + return ret;
>>> + count = ret;
>>
>> Ok, so why not do the following here instead (and drop a whole bunch of
>> functions above):
>>
>> ret = acpi_dev_get_resources(adev, &r, bmi_spi_count, &count);
>> if (ret < 0)
>> return ret;
>>
>> if (count <= 0) {
>> acpi_dev_free_resource_list(&r);
>> return count;
>> }
>>
>> /* Note we are not freeing the resource list yet here !!! */
>>
>>> +
>>> + bmi->spi_devs = devm_kcalloc(dev, count, sizeof(*bmi->spi_devs),
>> GFP_KERNEL);
>>> + if (!bmi->spi_devs)
>>> + return -ENOMEM;
>>> +
>>> + acpi_data = bmi_spi_get_resources(dev, adev, count);
>>> + if (!acpi_data)
>>> + return -ENOMEM;
>>
>> Remove the bmi_spi_get_resources() call here.
>>
>>> +
>>> + for (i = 0; i < count && inst_array[i].type; i++) {
>>
>> Write a new:
>>
>> int bmi_get_spi_resource_by_index(list_head *resource_list, struct
>> acpi_resource_spi_serialbus *sb_ret, int index)
>> {}
>>
>> Helper which walks the list and fills in *sb_ret with the Nth (matching index)
>> SpiSerialBus resource found in the
>> list.
>>
>> And then do:
>>
>> ret = bmi_get_spi_resource_by_index(&r, &sb, i);
>> if (ret)
>> return ret;
>>
>> ctrl =
>> bmi_find_spi_controller(sb.resource_source.string_ptr);
>>
>>
>>> + ctlr = bmi_find_spi_controller(acpi_data-
>>> acpi_data[i].resource_source);
>>> + if (!ctlr) {
>>> + ret = -EPROBE_DEFER;
>>> + goto error;
>>> + }
>>> +
>>> + spi_dev = spi_alloc_device(ctlr);
>>> + if (!spi_dev) {
>>> + dev_err(&ctlr->dev, "failed to allocate SPI device for
>> %s\n",
>>> + dev_name(&adev->dev));
>>> + ret = -ENOMEM;
>>> + goto error;
>>> + }
>>> +
>>> + strscpy(spi_dev->modalias, inst_array[i].type,
>> sizeof(spi_dev->modalias));
>>> +
>>
>> And replace all the "acpi_data->acpi_data[i].sb." reference below with
>> simple "sb.".
>>
>>
>>> + if (ctlr->fw_translate_cs) {
>>> + ret = ctlr->fw_translate_cs(ctlr,
>>> + acpi_data-
>>> acpi_data[i].sb.device_selection);
>>> + if (ret < 0) {
>>> + spi_dev_put(spi_dev);
>>> + goto error;
>>> + }
>>> + spi_dev->chip_select = ret;
>>> + } else {
>>> + spi_dev->chip_select = acpi_data-
>>> acpi_data[i].sb.device_selection;
>>> + }
>>> +
>>> + spi_dev->max_speed_hz = acpi_data-
>>> acpi_data[i].sb.connection_speed;
>>> + spi_dev->bits_per_word = acpi_data-
>>> acpi_data[i].sb.data_bit_length;
>>> +
>>> + if (acpi_data->acpi_data[i].sb.clock_phase ==
>> ACPI_SPI_SECOND_PHASE)
>>> + spi_dev->mode |= SPI_CPHA;
>>> + if (acpi_data->acpi_data[i].sb.clock_polarity ==
>> ACPI_SPI_START_HIGH)
>>> + spi_dev->mode |= SPI_CPOL;
>>> + if (acpi_data->acpi_data[i].sb.device_polarity ==
>> ACPI_SPI_ACTIVE_HIGH)
>>> + spi_dev->mode |= SPI_CS_HIGH;
>>> +
>>> + ret = bmi_get_irq(pdev, adev, &inst_array[i]);
>>> + if (ret < 0) {
>>> + spi_dev_put(spi_dev);
>>> + goto error;
>>> + }
>>> + spi_dev->irq = ret;
>>> +
>>> + snprintf(name, sizeof(name), "%s-%s-%s.%d",
>> dev_name(&ctlr->dev), dev_name(dev),
>>> + inst_array[i].type, i);
>>> + spi_dev->dev.init_name = name;
>>> +
>>> + ret = spi_add_device(spi_dev);
>>> + if (ret) {
>>> + dev_err(&ctlr->dev, "failed to add SPI device %s from
>> ACPI: %d\n",
>>> + dev_name(&adev->dev), ret);
>>> + spi_dev_put(spi_dev);
>>> + goto error;
>>> + }
>>> +
>>> + dev_dbg(dev, "SPI device %s using chip select %u", name,
>> spi_dev->chip_select);
>>> +
>>> + bmi->spi_devs[i] = spi_dev;
>>> + bmi->spi_num++;
>>> + }
>>> +
>>> + if (bmi->spi_num < count) {
>>> + dev_err(dev, "Error finding driver, idx %d\n", i);
>>> + ret = -ENODEV;
>>> + goto error;
>>> + }
>>> +
>>> + dev_info(dev, "Instantiate %d SPI devices.\n", bmi->spi_num);
>>
>> And here replace the bmi_spi_res_free(acpi_data); call in both exit paths
>> with:
>> acpi_dev_free_resource_list(&r); .
>>
>> To me this way, simply using the already allocated resources from the list,
>> rather then making a temp copy of them and throwing that away seems like
>> a simpler solution ?
>>
>> If you go this route, please also remove the struct bmi_spi_acpi and
>> struct bmi_spi_sb_acpi data types which you now no longer need.
>>
>
> I tried to implement this idea, and reuse the resource list, but I hit an issue.
> The resources saved in the list are not "struct acpi_resource", but instead the
> generic "struct resource".
> We need the acpi_resource structure to pull the parameters from to be able to
> create the spi devices.
> As far as I know there is no way to convert the "struct resource" into a
> "struct acpi_resource". Is there another way to do this?
Ugh, you're right. Sorry about that. I still don't realy like the code
from your original v2 patch for this.
So maybe this comment from my second reply on this patch can help
clean things up:
"So thinking a bit more about this, then looking up the nth SpiSerialBus
resource, and then turning that into a spi_client is something which
the SPI core ACPI code should already be doing for index==0. So I think
that you should be able to modify the SPI core ACPI code to take index
as a parameter and then have it export a helper for this which you
can use rather then duplicate the SPI core ACPI code ? Note this is
also what the I2C code is already doing.
And if you go that route you may also want to consider to add the SPI
equivalent of the i2c_acpi_client_count() helper."
Maybe that is a possible route to go to clean this up?
Note there are also 2 other small remarks pending:
1. My comment about adding the _t_ at the end of detect
2. + Mark's remark about patch 3/6 missing your Signed-off-by.
Regards,
Hans
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2 0/6] Support Spi in i2c-multi-instantiate driver
2021-12-10 15:40 [PATCH v2 0/6] Support Spi in i2c-multi-instantiate driver Stefan Binding
` (5 preceding siblings ...)
2021-12-10 15:40 ` [PATCH v2 6/6] ACPI: bus-multi-instantiate: Add SPI support Stefan Binding
@ 2021-12-10 16:54 ` Hans de Goede
2021-12-13 11:40 ` Stefan Binding
2021-12-21 18:16 ` Hans de Goede
7 siblings, 1 reply; 17+ messages in thread
From: Hans de Goede @ 2021-12-10 16:54 UTC (permalink / raw)
To: Stefan Binding, Mark Brown, Rafael J . Wysocki, Len Brown, Mark Gross
Cc: linux-kernel, linux-spi, linux-acpi, platform-driver-x86, patches
Hi Stefan,
On 12/10/21 16:40, Stefan Binding wrote:
> Add support for SPI bus in the ic2-multi-instantiate driver as
> upcoming laptops will need to multi instantiate SPI devices from
> a single device node, which has multiple SpiSerialBus entries at
> the ACPI table.
>
> With the new SPI support, i2c-multi-instantiate becomes
> bus-multi-instantiate and is moved to the ACPI folder.
>
> The intention is to support the SPI bus by re-using the current
> I2C multi instantiate, instead of creating a new SPI multi
> instantiate, to make it possible for peripherals that can be
> controlled by I2C or SPI to have the same HID at the ACPI table.
>
> The new driver (Bus multi instantiate, bmi) checks for the
> hard-coded bus type and returns -ENODEV in case of zero devices
> found for that bus. In the case of automatic bus detection,
> the driver will give preference to I2C.
>
> The expectation is for a device node in the ACPI table to have
> multiple I2cSerialBus only or multiple SpiSerialBus only, not
> a mix of both; and for the case where there are both entries in
> one device node, only the I2C ones would be probed.
>
> This new bus multi instantiate will be used in CS35L41 HDA new
> driver, being upstreamed:
> https://lkml.org/lkml/2021/11/23/723
Unfortunately you never really answered my questions about v1
of this series:
https://lore.kernel.org/platform-driver-x86/a1f546c2-5c63-573a-c032-603c792f3f7c@redhat.com/
So looking at the linked CS35L41 HDA series there is a single
ACPI device node with a HID of CLSA0100 which describes
two CS35L41 amplifiers connected over I2C ?
I assume you are doing this work because there are also designs
where there is a similar CLSA0100 ACPI device which also describes
two CS35L41 amplifiers but then connected over SPI ?
It would really help if you can:
1. Answer my questions from v1
2. Provide a concrete example of a device where these changes will
be necessary to make things work, preferably with a link to an
actual ACPI DSDT of that device.
Until you can better clarify why this is necessary, this series
gets a nack from me. The i2c-mult-instantiate code is a hack to
deal with some rather sub-optimal choices made in DSDTs used on
devices shipped with Windows and unless absolutely necessary
I would rather not see this get expanded to SPI.
Regards,
Hans
^ permalink raw reply [flat|nested] 17+ messages in thread* RE: [PATCH v2 0/6] Support Spi in i2c-multi-instantiate driver
2021-12-10 16:54 ` [PATCH v2 0/6] Support Spi in i2c-multi-instantiate driver Hans de Goede
@ 2021-12-13 11:40 ` Stefan Binding
2021-12-15 10:22 ` Hans de Goede
0 siblings, 1 reply; 17+ messages in thread
From: Stefan Binding @ 2021-12-13 11:40 UTC (permalink / raw)
To: 'Hans de Goede', 'Mark Brown',
'Rafael J . Wysocki', 'Len Brown',
'Mark Gross'
Cc: linux-kernel, linux-spi, linux-acpi, platform-driver-x86, patches
Hi,
> -----Original Message-----
> From: Hans de Goede <hdegoede@redhat.com>
> Sent: 10 December 2021 16:55
> To: Stefan Binding <sbinding@opensource.cirrus.com>; Mark Brown
> <broonie@kernel.org>; Rafael J . Wysocki <rafael@kernel.org>; Len Brown
> <lenb@kernel.org>; Mark Gross <markgross@kernel.org>
> Cc: linux-kernel@vger.kernel.org; linux-spi@vger.kernel.org; linux-
> acpi@vger.kernel.org; platform-driver-x86@vger.kernel.org;
> patches@opensource.cirrus.com
> Subject: Re: [PATCH v2 0/6] Support Spi in i2c-multi-instantiate driver
>
> Hi Stefan,
>
> On 12/10/21 16:40, Stefan Binding wrote:
> > Add support for SPI bus in the ic2-multi-instantiate driver as
> > upcoming laptops will need to multi instantiate SPI devices from a
> > single device node, which has multiple SpiSerialBus entries at the
> > ACPI table.
> >
> > With the new SPI support, i2c-multi-instantiate becomes
> > bus-multi-instantiate and is moved to the ACPI folder.
> >
> > The intention is to support the SPI bus by re-using the current I2C
> > multi instantiate, instead of creating a new SPI multi instantiate, to
> > make it possible for peripherals that can be controlled by I2C or SPI
> > to have the same HID at the ACPI table.
> >
> > The new driver (Bus multi instantiate, bmi) checks for the hard-coded
> > bus type and returns -ENODEV in case of zero devices found for that
> > bus. In the case of automatic bus detection, the driver will give
> > preference to I2C.
> >
> > The expectation is for a device node in the ACPI table to have
> > multiple I2cSerialBus only or multiple SpiSerialBus only, not a mix of
> > both; and for the case where there are both entries in one device
> > node, only the I2C ones would be probed.
> >
> > This new bus multi instantiate will be used in CS35L41 HDA new driver,
> > being upstreamed:
> > https://lkml.org/lkml/2021/11/23/723
>
> Unfortunately you never really answered my questions about v1 of this
> series:
>
> https://lore.kernel.org/platform-driver-x86/a1f546c2-5c63-573a-c032-
> 603c792f3f7c@redhat.com/
>
> So looking at the linked CS35L41 HDA series there is a single ACPI device node
> with a HID of CLSA0100 which describes two CS35L41 amplifiers connected
> over I2C ?
Yes, the related series uses HID CLSA0100, which contains 2 I2C devices inside a
single node. This ID was mistakenly used for this laptop, and instead CSC3551
has been used for subsequent laptops.
>
> I assume you are doing this work because there are also designs where there
> is a similar CLSA0100 ACPI device which also describes two CS35L41 amplifiers
> but then connected over SPI ?
Yes, there are several laptop designs which use an equivalent ACPI which describes
2 or 4 CS35L41 amplifiers which are connected either via I2C or via SPI.
Both designs use the same ACPI design and have 2-4 devices (either I2C or SPI)
defined inside a single ACPI node for HID CSC3551.
Note that the devices inside the node can be either SPI or I2C, but never SPI
and I2C.
>
> It would really help if you can:
>
> 1. Answer my questions from v1
I hope my colleague Lucas has answered these questions now.
> 2. Provide a concrete example of a device where these changes will be
> necessary to make things work, preferably with a link to an actual ACPI DSDT
> of that device.
This is the CSC3551 node for a laptop with 4 SPI nodes, with a shared IRQ:
Scope (_SB.PC00.SPI0)
{
Device (GSPK)
{
Name (_HID, "CSC3551") // _HID: Hardware ID
Method (AUID, 0, NotSerialized)
{
Return ("103C89C3")
}
Method (_SUB, 0, NotSerialized) // _SUB: Subsystem ID
{
Return (AUID ())
}
Name (_UID, One) // _UID: Unique ID
Method (_CRS, 0, NotSerialized) // _CRS: Current Resource Settings
{
Name (RBUF, ResourceTemplate ()
{
SpiSerialBusV2 (0x0000, PolarityLow, FourWireMode, 0x08,
ControllerInitiated, 0x003D0900, ClockPolarityLow,
ClockPhaseFirst, "\\_SB.PC00.SPI0",
0x00, ResourceConsumer, , Exclusive,
)
SpiSerialBusV2 (0x0001, PolarityLow, FourWireMode, 0x08,
ControllerInitiated, 0x003D0900, ClockPolarityLow,
ClockPhaseFirst, "\\_SB.PC00.SPI0",
0x00, ResourceConsumer, , Exclusive,
)
SpiSerialBusV2 (0x0002, PolarityLow, FourWireMode, 0x08,
ControllerInitiated, 0x003D0900, ClockPolarityLow,
ClockPhaseFirst, "\\_SB.PC00.SPI0",
0x00, ResourceConsumer, , Exclusive,
)
SpiSerialBusV2 (0x0003, PolarityLow, FourWireMode, 0x08,
ControllerInitiated, 0x003D0900, ClockPolarityLow,
ClockPhaseFirst, "\\_SB.PC00.SPI0",
0x00, ResourceConsumer, , Exclusive,
)
GpioIo (Exclusive, PullUp, 0x0000, 0x0000, IoRestrictionOutputOnly,
"\\_SB.GPI0", 0x00, ResourceConsumer, ,
)
{ // Pin list
0x0000
}
GpioIo (Exclusive, PullUp, 0x0000, 0x0000, IoRestrictionOutputOnly,
"\\_SB.GPI0", 0x00, ResourceConsumer, ,
)
{ // Pin list
0x0000
}
GpioIo (Exclusive, PullUp, 0x0000, 0x0000, IoRestrictionOutputOnly,
"\\_SB.GPI0", 0x00, ResourceConsumer, ,
)
{ // Pin list
0x0000
}
GpioIo (Exclusive, PullDown, 0x0000, 0x0000, IoRestrictionOutputOnly,
"\\_SB.GPI0", 0x00, ResourceConsumer, ,
)
{ // Pin list
0x0000
}
GpioIo (Shared, PullUp, 0x0064, 0x0000, IoRestrictionInputOnly,
"\\_SB.GPI0", 0x00, ResourceConsumer, ,
)
{ // Pin list
0x0000
}
GpioInt (Edge, ActiveBoth, Shared, PullUp, 0x0064,
"\\_SB.GPI0", 0x00, ResourceConsumer, ,
)
{ // Pin list
0x0000
}
})
CreateWordField (RBUF, 0xCA, ACS1)
CreateWordField (RBUF, 0xA7, ACS2)
CreateWordField (RBUF, 0xED, ACS3)
CreateWordField (RBUF, 0x0110, ARST)
CreateWordField (RBUF, 0x0133, AINT)
CreateWordField (RBUF, 0x0156, AIN2)
ACS1 = GNUM (0x090E0016)
ACS2 = GNUM (0x090E0017)
ACS3 = GNUM (0x090C0006)
ARST = GNUM (0x09070017)
AINT = GNUM (0x09070013)
AIN2 = GNUM (0x09070013)
Return (RBUF) /* \_SB_.PC00.SPI0.GSPK._CRS.RBUF */
}
Method (_STA, 0, NotSerialized) // _STA: Status
{
Return (0x0F)
}
Method (_DIS, 0, NotSerialized) // _DIS: Disable Device
{
}
Name (_DSD, Package () // _DSD: Device-Specific Data
{
ToUUID ("daffd814-6eba-4d8c-8a91-bc9bbf4aa301") /* Device Properties for _DSD */,
Package ()
{
Package () { "cirrus,dev-index", Package () { 0, 1, 2, 3 } },
Package ()
{
"reset-gpios", Package ()
{
^GSPK, 3, 0, 0,
^GSPK, 3, 0, 0,
^GSPK, 3, 0, 0,
^GSPK, 3, 0, 0,
},
},
Package () { "cirrus,speaker-position", Package () { 1, 0, 1, 0 } },
Package () { "cirrus,gpio1-func", Package () { 3, 3, 3, 3 } },
Package () { "cirrus,gpio2-func", Package () { 2, 2, 2, 2 } },
Package () { "cirrus,boost-ind-nanohenry", Package () { 1000, 1000, 1000, 1000 } },
Package () { "cirrus,boost-peak-milliamp", Package () { 4500, 4500, 4500, 4500 } },
Package () { "cirrus,boost-cap-microfarad", Package () { 24, 24, 24, 24 } },
}
})
}
}
This is just our node from the DSDT, we are working to obtain and share the full DSDT, if still required.
>
> Until you can better clarify why this is necessary, this series gets a nack from
> me. The i2c-mult-instantiate code is a hack to deal with some rather sub-
> optimal choices made in DSDTs used on devices shipped with Windows and
> unless absolutely necessary I would rather not see this get expanded to SPI.
>
> Regards,
>
> Hans
Thanks,
Stefan
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [PATCH v2 0/6] Support Spi in i2c-multi-instantiate driver
2021-12-13 11:40 ` Stefan Binding
@ 2021-12-15 10:22 ` Hans de Goede
0 siblings, 0 replies; 17+ messages in thread
From: Hans de Goede @ 2021-12-15 10:22 UTC (permalink / raw)
To: Stefan Binding, 'Mark Brown',
'Rafael J . Wysocki', 'Len Brown',
'Mark Gross'
Cc: linux-kernel, linux-spi, linux-acpi, platform-driver-x86, patches
Hi,
On 12/13/21 12:40, Stefan Binding wrote:
> Hi,
>
>> -----Original Message-----
>> From: Hans de Goede <hdegoede@redhat.com>
>> Sent: 10 December 2021 16:55
>> To: Stefan Binding <sbinding@opensource.cirrus.com>; Mark Brown
>> <broonie@kernel.org>; Rafael J . Wysocki <rafael@kernel.org>; Len Brown
>> <lenb@kernel.org>; Mark Gross <markgross@kernel.org>
>> Cc: linux-kernel@vger.kernel.org; linux-spi@vger.kernel.org; linux-
>> acpi@vger.kernel.org; platform-driver-x86@vger.kernel.org;
>> patches@opensource.cirrus.com
>> Subject: Re: [PATCH v2 0/6] Support Spi in i2c-multi-instantiate driver
>>
>> Hi Stefan,
>>
>> On 12/10/21 16:40, Stefan Binding wrote:
>>> Add support for SPI bus in the ic2-multi-instantiate driver as
>>> upcoming laptops will need to multi instantiate SPI devices from a
>>> single device node, which has multiple SpiSerialBus entries at the
>>> ACPI table.
>>>
>>> With the new SPI support, i2c-multi-instantiate becomes
>>> bus-multi-instantiate and is moved to the ACPI folder.
>>>
>>> The intention is to support the SPI bus by re-using the current I2C
>>> multi instantiate, instead of creating a new SPI multi instantiate, to
>>> make it possible for peripherals that can be controlled by I2C or SPI
>>> to have the same HID at the ACPI table.
>>>
>>> The new driver (Bus multi instantiate, bmi) checks for the hard-coded
>>> bus type and returns -ENODEV in case of zero devices found for that
>>> bus. In the case of automatic bus detection, the driver will give
>>> preference to I2C.
>>>
>>> The expectation is for a device node in the ACPI table to have
>>> multiple I2cSerialBus only or multiple SpiSerialBus only, not a mix of
>>> both; and for the case where there are both entries in one device
>>> node, only the I2C ones would be probed.
>>>
>>> This new bus multi instantiate will be used in CS35L41 HDA new driver,
>>> being upstreamed:
>>> https://lkml.org/lkml/2021/11/23/723
>>
>> Unfortunately you never really answered my questions about v1 of this
>> series:
>>
>> https://lore.kernel.org/platform-driver-x86/a1f546c2-5c63-573a-c032-
>> 603c792f3f7c@redhat.com/
>>
>> So looking at the linked CS35L41 HDA series there is a single ACPI device node
>> with a HID of CLSA0100 which describes two CS35L41 amplifiers connected
>> over I2C ?
>
> Yes, the related series uses HID CLSA0100, which contains 2 I2C devices inside a
> single node. This ID was mistakenly used for this laptop, and instead CSC3551
> has been used for subsequent laptops.
>
>>
>> I assume you are doing this work because there are also designs where there
>> is a similar CLSA0100 ACPI device which also describes two CS35L41 amplifiers
>> but then connected over SPI ?
>
> Yes, there are several laptop designs which use an equivalent ACPI which describes
> 2 or 4 CS35L41 amplifiers which are connected either via I2C or via SPI.
> Both designs use the same ACPI design and have 2-4 devices (either I2C or SPI)
> defined inside a single ACPI node for HID CSC3551.
> Note that the devices inside the node can be either SPI or I2C, but never SPI
> and I2C.
>
>>
>> It would really help if you can:
>>
>> 1. Answer my questions from v1
>
> I hope my colleague Lucas has answered these questions now.
Yes, thank you .
>> 2. Provide a concrete example of a device where these changes will be
>> necessary to make things work, preferably with a link to an actual ACPI DSDT
>> of that device.
>
> This is the CSC3551 node for a laptop with 4 SPI nodes, with a shared IRQ:
>
> Scope (_SB.PC00.SPI0)
> {
> Device (GSPK)
> {
> Name (_HID, "CSC3551") // _HID: Hardware ID
> Method (AUID, 0, NotSerialized)
> {
> Return ("103C89C3")
> }
>
> Method (_SUB, 0, NotSerialized) // _SUB: Subsystem ID
> {
> Return (AUID ())
> }
>
> Name (_UID, One) // _UID: Unique ID
> Method (_CRS, 0, NotSerialized) // _CRS: Current Resource Settings
> {
> Name (RBUF, ResourceTemplate ()
> {
> SpiSerialBusV2 (0x0000, PolarityLow, FourWireMode, 0x08,
> ControllerInitiated, 0x003D0900, ClockPolarityLow,
> ClockPhaseFirst, "\\_SB.PC00.SPI0",
> 0x00, ResourceConsumer, , Exclusive,
> )
> SpiSerialBusV2 (0x0001, PolarityLow, FourWireMode, 0x08,
> ControllerInitiated, 0x003D0900, ClockPolarityLow,
> ClockPhaseFirst, "\\_SB.PC00.SPI0",
> 0x00, ResourceConsumer, , Exclusive,
> )
> SpiSerialBusV2 (0x0002, PolarityLow, FourWireMode, 0x08,
> ControllerInitiated, 0x003D0900, ClockPolarityLow,
> ClockPhaseFirst, "\\_SB.PC00.SPI0",
> 0x00, ResourceConsumer, , Exclusive,
> )
> SpiSerialBusV2 (0x0003, PolarityLow, FourWireMode, 0x08,
> ControllerInitiated, 0x003D0900, ClockPolarityLow,
> ClockPhaseFirst, "\\_SB.PC00.SPI0",
> 0x00, ResourceConsumer, , Exclusive,
> )
> GpioIo (Exclusive, PullUp, 0x0000, 0x0000, IoRestrictionOutputOnly,
> "\\_SB.GPI0", 0x00, ResourceConsumer, ,
> )
> { // Pin list
> 0x0000
> }
> GpioIo (Exclusive, PullUp, 0x0000, 0x0000, IoRestrictionOutputOnly,
> "\\_SB.GPI0", 0x00, ResourceConsumer, ,
> )
> { // Pin list
> 0x0000
> }
> GpioIo (Exclusive, PullUp, 0x0000, 0x0000, IoRestrictionOutputOnly,
> "\\_SB.GPI0", 0x00, ResourceConsumer, ,
> )
> { // Pin list
> 0x0000
> }
> GpioIo (Exclusive, PullDown, 0x0000, 0x0000, IoRestrictionOutputOnly,
> "\\_SB.GPI0", 0x00, ResourceConsumer, ,
> )
> { // Pin list
> 0x0000
> }
> GpioIo (Shared, PullUp, 0x0064, 0x0000, IoRestrictionInputOnly,
> "\\_SB.GPI0", 0x00, ResourceConsumer, ,
> )
> { // Pin list
> 0x0000
> }
> GpioInt (Edge, ActiveBoth, Shared, PullUp, 0x0064,
> "\\_SB.GPI0", 0x00, ResourceConsumer, ,
> )
> { // Pin list
> 0x0000
> }
> })
> CreateWordField (RBUF, 0xCA, ACS1)
> CreateWordField (RBUF, 0xA7, ACS2)
> CreateWordField (RBUF, 0xED, ACS3)
> CreateWordField (RBUF, 0x0110, ARST)
> CreateWordField (RBUF, 0x0133, AINT)
> CreateWordField (RBUF, 0x0156, AIN2)
> ACS1 = GNUM (0x090E0016)
> ACS2 = GNUM (0x090E0017)
> ACS3 = GNUM (0x090C0006)
> ARST = GNUM (0x09070017)
> AINT = GNUM (0x09070013)
> AIN2 = GNUM (0x09070013)
> Return (RBUF) /* \_SB_.PC00.SPI0.GSPK._CRS.RBUF */
> }
>
> Method (_STA, 0, NotSerialized) // _STA: Status
> {
> Return (0x0F)
> }
>
> Method (_DIS, 0, NotSerialized) // _DIS: Disable Device
> {
> }
> Name (_DSD, Package () // _DSD: Device-Specific Data
> {
> ToUUID ("daffd814-6eba-4d8c-8a91-bc9bbf4aa301") /* Device Properties for _DSD */,
> Package ()
> {
> Package () { "cirrus,dev-index", Package () { 0, 1, 2, 3 } },
> Package ()
> {
> "reset-gpios", Package ()
> {
> ^GSPK, 3, 0, 0,
> ^GSPK, 3, 0, 0,
> ^GSPK, 3, 0, 0,
> ^GSPK, 3, 0, 0,
> },
> },
> Package () { "cirrus,speaker-position", Package () { 1, 0, 1, 0 } },
> Package () { "cirrus,gpio1-func", Package () { 3, 3, 3, 3 } },
> Package () { "cirrus,gpio2-func", Package () { 2, 2, 2, 2 } },
> Package () { "cirrus,boost-ind-nanohenry", Package () { 1000, 1000, 1000, 1000 } },
> Package () { "cirrus,boost-peak-milliamp", Package () { 4500, 4500, 4500, 4500 } },
> Package () { "cirrus,boost-cap-microfarad", Package () { 24, 24, 24, 24 } },
> }
> })
> }
> }
>
> This is just our node from the DSDT, we are working to obtain and share the full DSDT, if still required.
Thanks, there is no need to share the full DSDT, this is exactly what I was looking for.
With the provided info I believe that this indeed is the right approach
and I no longer have any objections against this approach.
I will try to make some time to review this series soon-ish.
Regards,
Hans
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2 0/6] Support Spi in i2c-multi-instantiate driver
2021-12-10 15:40 [PATCH v2 0/6] Support Spi in i2c-multi-instantiate driver Stefan Binding
` (6 preceding siblings ...)
2021-12-10 16:54 ` [PATCH v2 0/6] Support Spi in i2c-multi-instantiate driver Hans de Goede
@ 2021-12-21 18:16 ` Hans de Goede
7 siblings, 0 replies; 17+ messages in thread
From: Hans de Goede @ 2021-12-21 18:16 UTC (permalink / raw)
To: Stefan Binding, Mark Brown, Rafael J . Wysocki, Len Brown, Mark Gross
Cc: linux-kernel, linux-spi, linux-acpi, platform-driver-x86, patches
Hi Stefan,
On 12/10/21 16:40, Stefan Binding wrote:
> Add support for SPI bus in the ic2-multi-instantiate driver as
> upcoming laptops will need to multi instantiate SPI devices from
> a single device node, which has multiple SpiSerialBus entries at
> the ACPI table.
>
> With the new SPI support, i2c-multi-instantiate becomes
> bus-multi-instantiate and is moved to the ACPI folder.
>
> The intention is to support the SPI bus by re-using the current
> I2C multi instantiate, instead of creating a new SPI multi
> instantiate, to make it possible for peripherals that can be
> controlled by I2C or SPI to have the same HID at the ACPI table.
>
> The new driver (Bus multi instantiate, bmi) checks for the
> hard-coded bus type and returns -ENODEV in case of zero devices
> found for that bus. In the case of automatic bus detection,
> the driver will give preference to I2C.
>
> The expectation is for a device node in the ACPI table to have
> multiple I2cSerialBus only or multiple SpiSerialBus only, not
> a mix of both; and for the case where there are both entries in
> one device node, only the I2C ones would be probed.
>
> This new bus multi instantiate will be used in CS35L41 HDA new
> driver, being upstreamed:
> https://lkml.org/lkml/2021/11/23/723
Patches 1-5 look good to me and are:
Reviewed-by: Hans de Goede <hdegoede@redhat.com>
Rafael, you may also consider this as my ack for merging this
series through the ACPI subsys branches.
But I do have some remarks on patch 6 which should be
addressed first.
Regards,
Hans
>
> Changes since V1:
> - Added Cover Letter
> - Split SPI patch into move, rename, reorganize and add
> SPI support
> - Hard coded BUS type for better decison making at bmi_probe
> - Driver moved to acpi folder
> - Change to use acpi_spi_find_controller_by_adev
> - New shared function bmi_get_irq for I2C and SPI
>
>
> Lucas Tanure (3):
> platform/x86: i2c-multi-instantiate: Move it to drivers/acpi folder
> ACPI: i2c-multi-instantiate: Rename it for a generic bus driver name
> ACPI: bus-multi-instantiate: Reorganize I2C functions
>
> Stefan Binding (3):
> spi: Export acpi_spi_find_controller_by_adev to be used externally
> spi: Make spi_alloc_device and spi_add_device public again
> ACPI: bus-multi-instantiate: Add SPI support
>
> MAINTAINERS | 4 +-
> drivers/acpi/Kconfig | 11 +
> drivers/acpi/Makefile | 1 +
> drivers/acpi/bus-multi-instantiate.c | 500 +++++++++++++++++++
> drivers/acpi/scan.c | 19 +-
> drivers/platform/x86/Kconfig | 11 -
> drivers/platform/x86/Makefile | 1 -
> drivers/platform/x86/i2c-multi-instantiate.c | 174 -------
> drivers/spi/spi.c | 9 +-
> include/linux/spi/spi.h | 22 +
> 10 files changed, 552 insertions(+), 200 deletions(-)
> create mode 100644 drivers/acpi/bus-multi-instantiate.c
> delete mode 100644 drivers/platform/x86/i2c-multi-instantiate.c
>
^ permalink raw reply [flat|nested] 17+ messages in thread