Support Spi in i2c-multi-instantiate driver

From: Lucas Tanure <[email protected]>

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 <[email protected]>

---

Quoted 6 lines · Stefan Binding

From: Lucas Tanure <[email protected]>

Moving I2C multi instantiate driver to drivers/acpi folder for upcoming conversion into a generic bus multi instantiate driver for SPI and I2C

Quoted 2 lines

Signed-off-by Lucas Tanure <[email protected]>

---

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.

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 <[email protected]>

---
 drivers/acpi/Kconfig                 |  10 +-
 drivers/acpi/bus-multi-instantiate.c | 345 ++++++++++++++++++++++++---
 drivers/acpi/scan.c                  |  13 +-
 3 files changed, 327 insertions(+), 41 deletions(-)

drivers/acpi/Kconfig +5 -5

index 6ba47dd39eb4..948f39d55595 100644
@@ -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.

drivers/acpi/bus-multi-instantiate.c +2 -1

index 50f1540762e9..c1306a0ee13c 100644
@@ -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 <[email protected]>
  */

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

drivers/acpi/bus-multi-instantiate.c +79 -0

+};
+
+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;
+}

drivers/acpi/bus-multi-instantiate.c +57 -0

+
+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;

drivers/acpi/bus-multi-instantiate.c +6 -0

+		ctlr = bmi_find_spi_controller(acpi_data->acpi_data[i].resource_source);
+		if (!ctlr) {
+			ret = -EPROBE_DEFER;
+			goto error;
+		}
+

drivers/acpi/bus-multi-instantiate.c +44 -0

+		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;

drivers/acpi/bus-multi-instantiate.c +10 -0

+	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;
+

}

/**

drivers/acpi/bus-multi-instantiate.c +13 -6

@@ -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;

drivers/acpi/bus-multi-instantiate.c +17 -3

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

drivers/acpi/bus-multi-instantiate.c +25 -15

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

drivers/acpi/scan.c +0 -6

index 969d8138d019..8b937fc20d23 100644
@@ -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.

Hi,

Quoted 68 lines · 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 <[email protected]>

---
 drivers/acpi/Kconfig                 |  10 +-
 drivers/acpi/bus-multi-instantiate.c | 345 ++++++++++++++++++++++++---
 drivers/acpi/scan.c                  |  13 +-
 3 files changed, 327 insertions(+), 41 deletions(-)

drivers/acpi/Kconfig +5 -5

index 6ba47dd39eb4..948f39d55595 100644
@@ -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.

drivers/acpi/bus-multi-instantiate.c +2 -1

index 50f1540762e9..c1306a0ee13c 100644
@@ -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 <[email protected]>
  */

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

drivers/acpi/bus-multi-instantiate.c line 28

Quoted 7 lines

drivers/acpi/bus-multi-instantiate.c +4 -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.

Quoted 95 lines

drivers/acpi/bus-multi-instantiate.c +79 -0

+};
+
+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;
+}

drivers/acpi/bus-multi-instantiate.c static void bmi_devs_unregister(struct bmi *bmi) · line 156

Quoted 10 lines

drivers/acpi/bus-multi-instantiate.c +10 -0

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

Quoted 57 lines

drivers/acpi/bus-multi-instantiate.c +57 -0

+
+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;

drivers/acpi/bus-multi-instantiate.c static void bmi_devs_unregister(struct bmi *bmi) · line 220

Quoted 10 lines

drivers/acpi/bus-multi-instantiate.c +7 -0

+	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 !!! */

drivers/acpi/bus-multi-instantiate.c static void bmi_devs_unregister(struct bmi *bmi) · line 228

Quoted 8 lines

drivers/acpi/bus-multi-instantiate.c +8 -0

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

drivers/acpi/bus-multi-instantiate.c static void bmi_devs_unregister(struct bmi *bmi) · line 233

Quoted 2 lines

drivers/acpi/bus-multi-instantiate.c +2 -0

+
+	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);

Quoted 6 lines

drivers/acpi/bus-multi-instantiate.c +6 -0

+		ctlr = bmi_find_spi_controller(acpi_data->acpi_data[i].resource_source);
+		if (!ctlr) {
+			ret = -EPROBE_DEFER;
+			goto error;
+		}
+

drivers/acpi/bus-multi-instantiate.c static void bmi_devs_unregister(struct bmi *bmi) · line 246

Quoted 10 lines

drivers/acpi/bus-multi-instantiate.c +10 -0

+		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.".

Quoted 44 lines

drivers/acpi/bus-multi-instantiate.c +44 -0

+		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;

drivers/acpi/bus-multi-instantiate.c static void bmi_devs_unregister(struct bmi *bmi) · line 300

Quoted 10 lines

drivers/acpi/bus-multi-instantiate.c +10 -0

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

Quoted 48 lines

drivers/acpi/bus-multi-instantiate.c +10 -0

+	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;
+

}

/**

drivers/acpi/bus-multi-instantiate.c +13 -6

@@ -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;

drivers/acpi/bus-multi-instantiate.c static int bmi_probe(struct platform_device *pdev) · line 416

Quoted 10 lines

drivers/acpi/bus-multi-instantiate.c +10 -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_

Quoted 26 lines

drivers/acpi/bus-multi-instantiate.c +17 -3

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

drivers/acpi/bus-multi-instantiate.c static int bmi_remove(struct platform_device *pdev) · line 447

Quoted 9 lines

drivers/acpi/bus-multi-instantiate.c +1 -5

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

Quoted 69 lines

drivers/acpi/bus-multi-instantiate.c +25 -15

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

drivers/acpi/scan.c +0 -6

index 969d8138d019..8b937fc20d23 100644
@@ -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.

drivers/acpi/scan.c static bool acpi_device_enumeration_by_parent(struct acpi_device *device) · line 1708

Quoted 10 lines

drivers/acpi/scan.c +7 -0

+	 * 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

-----Original Message-----
From: Hans de Goede <[email protected]>
Sent: 21 December 2021 18:32
To: Stefan Binding <[email protected]>; Mark Brown <[email protected]>; Rafael J . Wysocki <[email protected]>; Len Brown <[email protected]>; Mark Gross <[email protected]>
Cc: [email protected]; [email protected]; linux- [email protected]; [email protected]; [email protected]
Subject: Re: [PATCH v2 6/6] ACPI: bus-multi-instantiate: Add SPI support

drivers/acpi/bus-multi-instantiate.c static void bmi_devs_unregister(struct bmi *bmi) · line 220

Quoted 4 lines

drivers/acpi/bus-multi-instantiate.c +4 -0

+	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 !!! */

Review

Quoted 2 lines

(quoted diff) +2 -0

+
+	bmi->spi_devs = devm_kcalloc(dev, count, sizeof(*bmi->spi_devs),

GFP_KERNEL);

drivers/acpi/bus-multi-instantiate.c static void bmi_devs_unregister(struct bmi *bmi) · line 228

Quoted 6 lines

drivers/acpi/bus-multi-instantiate.c +6 -0

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

Review

Quoted 2 lines

(quoted diff) +2 -0

+
+	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);

drivers/acpi/bus-multi-instantiate.c static void bmi_devs_unregister(struct bmi *bmi)

Quoted 10 lines

(quoted diff) +1 -0

+		ctlr = bmi_find_spi_controller(acpi_data-

acpi_data[i].resource_source);

drivers/acpi/bus-multi-instantiate.c +8 -0

+		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",

drivers/acpi/bus-multi-instantiate.c static void bmi_devs_unregister(struct bmi *bmi)

Quoted 6 lines

drivers/acpi/bus-multi-instantiate.c +6 -0

+				dev_name(&adev->dev));
+			ret = -ENOMEM;
+			goto error;
+		}
+
+		strscpy(spi_dev->modalias, inst_array[i].type,

sizeof(spi_dev->modalias));

Quoted 1 line

+

And replace all the "acpi_data->acpi_data[i].sb." reference below with simple "sb.".

Quoted 18 lines

drivers/acpi/bus-multi-instantiate.c +3 -0

+		if (ctlr->fw_translate_cs) {
+			ret = ctlr->fw_translate_cs(ctlr,
+						    acpi_data-

acpi_data[i].sb.device_selection);

drivers/acpi/bus-multi-instantiate.c +7 -0

+			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;

drivers/acpi/bus-multi-instantiate.c +3 -0

+		}
+
+		spi_dev->max_speed_hz = acpi_data-

acpi_data[i].sb.connection_speed;

(quoted diff) +1 -0

+		spi_dev->bits_per_word = acpi_data-

acpi_data[i].sb.data_bit_length;

Review

Quoted 2 lines

(quoted diff) +2 -0

+
+		if (acpi_data->acpi_data[i].sb.clock_phase ==

ACPI_SPI_SECOND_PHASE)

Review

Quoted 2 lines

(quoted diff) +2 -0

+			spi_dev->mode |= SPI_CPHA;
+		if (acpi_data->acpi_data[i].sb.clock_polarity ==

ACPI_SPI_START_HIGH)

Review

Quoted 2 lines

(quoted diff) +2 -0

+			spi_dev->mode |= SPI_CPOL;
+		if (acpi_data->acpi_data[i].sb.device_polarity ==

ACPI_SPI_ACTIVE_HIGH)

drivers/acpi/bus-multi-instantiate.c static void bmi_devs_unregister(struct bmi *bmi)

Quoted 10 lines

drivers/acpi/bus-multi-instantiate.c +10 -0

+			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),

drivers/acpi/bus-multi-instantiate.c static void bmi_devs_unregister(struct bmi *bmi)

Quoted 6 lines

drivers/acpi/bus-multi-instantiate.c +6 -0

+			 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",

drivers/acpi/bus-multi-instantiate.c static void bmi_devs_unregister(struct bmi *bmi)

Quoted 6 lines

drivers/acpi/bus-multi-instantiate.c +6 -0

+				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);

drivers/acpi/bus-multi-instantiate.c static void bmi_devs_unregister(struct bmi *bmi) · line 300

Quoted 12 lines

drivers/acpi/bus-multi-instantiate.c +12 -0

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

Hi,

Quoted 158 lines

-----Original Message-----
From: Hans de Goede <[email protected]>
Sent: 21 December 2021 18:32
To: Stefan Binding <[email protected]>; Mark Brown <[email protected]>; Rafael J . Wysocki <[email protected]>; Len Brown <[email protected]>; Mark Gross <[email protected]>
Cc: [email protected]; [email protected]; linux- [email protected]; [email protected]; [email protected]
Subject: Re: [PATCH v2 6/6] ACPI: bus-multi-instantiate: Add SPI support

drivers/acpi/bus-multi-instantiate.c static void bmi_devs_unregister(struct bmi *bmi) · line 220

Quoted 4 lines

drivers/acpi/bus-multi-instantiate.c +4 -0

+	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 !!! */

Review

Quoted 2 lines

(quoted diff) +2 -0

+
+	bmi->spi_devs = devm_kcalloc(dev, count, sizeof(*bmi->spi_devs),

GFP_KERNEL);

drivers/acpi/bus-multi-instantiate.c static void bmi_devs_unregister(struct bmi *bmi) · line 228

Quoted 6 lines

drivers/acpi/bus-multi-instantiate.c +6 -0

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

Review

Quoted 2 lines

(quoted diff) +2 -0

+
+	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);

drivers/acpi/bus-multi-instantiate.c static void bmi_devs_unregister(struct bmi *bmi)

Quoted 10 lines

(quoted diff) +1 -0

+		ctlr = bmi_find_spi_controller(acpi_data-

acpi_data[i].resource_source);

drivers/acpi/bus-multi-instantiate.c +8 -0

+		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",

drivers/acpi/bus-multi-instantiate.c static void bmi_devs_unregister(struct bmi *bmi)

Quoted 6 lines

drivers/acpi/bus-multi-instantiate.c +6 -0

+				dev_name(&adev->dev));
+			ret = -ENOMEM;
+			goto error;
+		}
+
+		strscpy(spi_dev->modalias, inst_array[i].type,

sizeof(spi_dev->modalias));

Quoted 1 line

+

And replace all the "acpi_data->acpi_data[i].sb." reference below with simple "sb.".

Quoted 18 lines

drivers/acpi/bus-multi-instantiate.c +3 -0

+		if (ctlr->fw_translate_cs) {
+			ret = ctlr->fw_translate_cs(ctlr,
+						    acpi_data-

acpi_data[i].sb.device_selection);

drivers/acpi/bus-multi-instantiate.c +7 -0

+			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;

drivers/acpi/bus-multi-instantiate.c +3 -0

+		}
+
+		spi_dev->max_speed_hz = acpi_data-

acpi_data[i].sb.connection_speed;

(quoted diff) +1 -0

+		spi_dev->bits_per_word = acpi_data-

acpi_data[i].sb.data_bit_length;

Review

Quoted 2 lines

(quoted diff) +2 -0

+
+		if (acpi_data->acpi_data[i].sb.clock_phase ==

ACPI_SPI_SECOND_PHASE)

Review

Quoted 2 lines

(quoted diff) +2 -0

+			spi_dev->mode |= SPI_CPHA;
+		if (acpi_data->acpi_data[i].sb.clock_polarity ==

ACPI_SPI_START_HIGH)

Review

Quoted 2 lines

(quoted diff) +2 -0

+			spi_dev->mode |= SPI_CPOL;
+		if (acpi_data->acpi_data[i].sb.device_polarity ==

ACPI_SPI_ACTIVE_HIGH)

drivers/acpi/bus-multi-instantiate.c static void bmi_devs_unregister(struct bmi *bmi)

Quoted 10 lines

drivers/acpi/bus-multi-instantiate.c +10 -0

+			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),

drivers/acpi/bus-multi-instantiate.c static void bmi_devs_unregister(struct bmi *bmi)

Quoted 6 lines

drivers/acpi/bus-multi-instantiate.c +6 -0

+			 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",

drivers/acpi/bus-multi-instantiate.c static void bmi_devs_unregister(struct bmi *bmi)

Quoted 6 lines

drivers/acpi/bus-multi-instantiate.c +6 -0

+				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);

drivers/acpi/bus-multi-instantiate.c static void bmi_devs_unregister(struct bmi *bmi) · line 300

Quoted 12 lines

drivers/acpi/bus-multi-instantiate.c +12 -0

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

Quoted 7 lines

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?

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

-----Original Message-----
From: Hans de Goede <[email protected]>
Sent: 10 December 2021 16:55
To: Stefan Binding <[email protected]>; Mark Brown <[email protected]>; Rafael J . Wysocki <[email protected]>; Len Brown <[email protected]>; Mark Gross <[email protected]>
Cc: [email protected]; [email protected]; linux- [email protected]; [email protected]; [email protected]
Subject: Re: [PATCH v2 0/6] Support Spi in i2c-multi-instantiate driver

Hi Stefan,

Quoted 18 lines · Stefan Binding

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.

Quoted 8 lines

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-
[email protected]/

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

Hi,

Quoted 44 lines

-----Original Message-----
From: Hans de Goede <[email protected]>
Sent: 10 December 2021 16:55
To: Stefan Binding <[email protected]>; Mark Brown <[email protected]>; Rafael J . Wysocki <[email protected]>; Len Brown <[email protected]>; Mark Gross <[email protected]>
Cc: [email protected]; [email protected]; linux- [email protected]; [email protected]; [email protected]
Subject: Re: [PATCH v2 0/6] Support Spi in i2c-multi-instantiate driver

Hi Stefan,

Quoted 18 lines · Stefan Binding

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.

Quoted 8 lines

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:

Quoted 6 lines

https://lore.kernel.org/platform-driver-x86/a1f546c2-5c63-573a-c032-
[email protected]/

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.

Quoted 4 lines

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.

Quoted 1 line

Quoted 3 lines

It would really help if you can:

1. Answer my questions from v1

I hope my colleague Lucas has answered these questions now.

Quoted 3 lines

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.

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

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