diff mbox series

[v4,7/9] platform/x86: bus-multi-instantiate: Add SPI support

Message ID 20220120134326.5295-8-sbinding@opensource.cirrus.com (mailing list archive)
State Superseded
Headers show
Series Support Spi in i2c-multi-instantiate driver | expand

Commit Message

Stefan Binding Jan. 20, 2022, 1:43 p.m. UTC
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/scan.c                          |  13 +-
 drivers/platform/x86/Kconfig                 |  10 +-
 drivers/platform/x86/bus-multi-instantiate.c | 200 ++++++++++++++++---
 3 files changed, 182 insertions(+), 41 deletions(-)

Comments

Hans de Goede Jan. 20, 2022, 4:30 p.m. UTC | #1
Hi,

On 1/20/22 14:43, 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/scan.c                          |  13 +-
>  drivers/platform/x86/Kconfig                 |  10 +-
>  drivers/platform/x86/bus-multi-instantiate.c | 200 ++++++++++++++++---
>  3 files changed, 182 insertions(+), 41 deletions(-)
> 
> diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
> index e4fda14572e7..4fd1346d6e1f 100644
> --- a/drivers/acpi/scan.c
> +++ b/drivers/acpi/scan.c
> @@ -1734,12 +1734,13 @@ static bool acpi_device_enumeration_by_parent(struct acpi_device *device)
>  	bool is_serial_bus_slave = false;
>  	static const struct acpi_device_id ignore_serial_bus_ids[] = {
>  	/*
> -	 * 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/platform/x86/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/platform/x86/bus-multi-instantiate.c driver, which
> +	 * knows which i2c_device_id or spi_device_id to use for each resource.
>  	 */
>  		{"BSG1160", },
>  		{"BSG2150", },
> diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
> index 616da06aa6b9..7b49d9d1e267 100644
> --- a/drivers/platform/x86/Kconfig
> +++ b/drivers/platform/x86/Kconfig
> @@ -991,12 +991,12 @@ config TOPSTAR_LAPTOP
>  	  If you have a Topstar laptop, say Y or M here.
>  
>  config BUS_MULTI_INSTANTIATE
> -	tristate "I2C multi instantiate pseudo device driver"
> -	depends on I2C && ACPI
> +	tristate "I2C and SPI multi instantiate pseudo device driver"
> +	depends on I2C && SPI && 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.
> +	  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/platform/x86/bus-multi-instantiate.c b/drivers/platform/x86/bus-multi-instantiate.c
> index 50f1540762e9..05bc941a38fd 100644
> --- a/drivers/platform/x86/bus-multi-instantiate.c
> +++ b/drivers/platform/x86/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,28 @@
>  #define IRQ_RESOURCE_GPIO	1
>  #define IRQ_RESOURCE_APIC	2
>  
> +enum bmi_bus_type {
> +	BMI_I2C,
> +	BMI_SPI,
> +	BMI_AUTO_DETECT,
> +};
> +
>  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 +74,92 @@ 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]);
> +}
> +
> +/**
> + * 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 device *dev = &pdev->dev;
> +	struct spi_controller *ctlr;
> +	struct spi_device *spi_dev;
> +	char name[50];
> +	int i, ret, count;
> +
> +	ret = acpi_spi_count_resources(adev);
> +	if (ret <= 0)
> +		return ret;

Please change this to:

		return ret == 0 ? -ENODEV : ret;

This helps making return value handler in the caller cleaner.

> +	count = ret;
> +
> +	bmi->spi_devs = devm_kcalloc(dev, count, sizeof(*bmi->spi_devs), GFP_KERNEL);
> +	if (!bmi->spi_devs)
> +		return -ENOMEM;
> +
> +	for (i = 0; i < count && inst_array[i].type; i++) {
> +
> +		spi_dev = acpi_spi_device_alloc(NULL, adev, i, inst_array[i].irq_idx);
> +		if (IS_ERR(spi_dev)) {

Maybe put a dev_probe_error() here ?

> +			ret = PTR_ERR(spi_dev);
> +			goto error;
> +		}
> +
> +		ctlr = spi_dev->controller;
> +
> +		strscpy(spi_dev->modalias, inst_array[i].type, sizeof(spi_dev->modalias));
> +
> +		if (spi_dev->irq < 0) {

As already mentioned please drop this if. This will also allow overriding a
potentially bad IRQ from the ACPI tables (or one only valid for 1 of the SPI clients)
with IRQ_RESOURCE_NONE.


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

Nitpick, please change this to:

	dev_info(dev, "Instantiated %d SPI devices.\n", bmi->spi_num);

IOW do s/Instantiate/Instantiated/

> +
> +	return bmi->spi_num;

And change this to return 0.

> +error:
> +	bmi_devs_unregister(bmi);
> +	dev_err_probe(dev, ret, "SPI error %d\n", ret);

Please drop the dev_err_probe here, with the dev_err_probe()
which I suggested above all the error paths already print
a more detailed error.

> +
> +	return ret;
> +
>  }
>  
>  /**
> @@ -125,14 +225,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 +247,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_DETECT */
> +	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_DETECT 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;

Hmm, the above is rather messy and hard to read. With the changed
return values I suggested earlier this can be changed to:

	switch (node->bus_type) {
	case BMI_I2C:
		return bmi_i2c_probe(pdev, adev, bmi, node->instances);
	case BMI_SPI:
		return bmi_spi_probe(pdev, adev, bmi, node->instances);
	case BMI_AUTO_DETECT:
		if (i2c_acpi_client_count(adev))
			return bmi_i2c_probe(pdev, adev, bmi, node->instances);
		else
			return bmi_spi_probe(pdev, adev, bmi, node->instances);
	}

	return 0; /* never reached */
}

Which IMHO is much cleaner / easier to follow.

Note this has one functional change compared to your original code,
when doing auto-detect and the i2c device instantiating fails with
an error other then -EPROBE_DEFER we now fail the entire probe
(propagate the error to our caller).

Where as before for auto-detect bmi_i2c_probe() errors other then
-EPROBE_DEFER would lead to falling back to SPI probing which
does not seem to be what we want.


>  static int bmi_remove(struct platform_device *pdev)
> @@ -165,27 +296,36 @@ 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" },
> +		{}
> +	},
> +	.bus_type = BMI_I2C,
>  };
>  
> -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" },
> +		{}
> +	},
> +	.bus_type = BMI_I2C,
>  };
>  
> -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 },
> +		{}
> +	},
> +	.bus_type = BMI_I2C,
>  };
>  
>  /*
> @@ -193,9 +333,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);
> 


Regards,

Hans
diff mbox series

Patch

diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
index e4fda14572e7..4fd1346d6e1f 100644
--- a/drivers/acpi/scan.c
+++ b/drivers/acpi/scan.c
@@ -1734,12 +1734,13 @@  static bool acpi_device_enumeration_by_parent(struct acpi_device *device)
 	bool is_serial_bus_slave = false;
 	static const struct acpi_device_id ignore_serial_bus_ids[] = {
 	/*
-	 * 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/platform/x86/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/platform/x86/bus-multi-instantiate.c driver, which
+	 * knows which i2c_device_id or spi_device_id to use for each resource.
 	 */
 		{"BSG1160", },
 		{"BSG2150", },
diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
index 616da06aa6b9..7b49d9d1e267 100644
--- a/drivers/platform/x86/Kconfig
+++ b/drivers/platform/x86/Kconfig
@@ -991,12 +991,12 @@  config TOPSTAR_LAPTOP
 	  If you have a Topstar laptop, say Y or M here.
 
 config BUS_MULTI_INSTANTIATE
-	tristate "I2C multi instantiate pseudo device driver"
-	depends on I2C && ACPI
+	tristate "I2C and SPI multi instantiate pseudo device driver"
+	depends on I2C && SPI && 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.
+	  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/platform/x86/bus-multi-instantiate.c b/drivers/platform/x86/bus-multi-instantiate.c
index 50f1540762e9..05bc941a38fd 100644
--- a/drivers/platform/x86/bus-multi-instantiate.c
+++ b/drivers/platform/x86/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,28 @@ 
 #define IRQ_RESOURCE_GPIO	1
 #define IRQ_RESOURCE_APIC	2
 
+enum bmi_bus_type {
+	BMI_I2C,
+	BMI_SPI,
+	BMI_AUTO_DETECT,
+};
+
 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 +74,92 @@  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]);
+}
+
+/**
+ * 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 device *dev = &pdev->dev;
+	struct spi_controller *ctlr;
+	struct spi_device *spi_dev;
+	char name[50];
+	int i, ret, count;
+
+	ret = acpi_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;
+
+	for (i = 0; i < count && inst_array[i].type; i++) {
+
+		spi_dev = acpi_spi_device_alloc(NULL, adev, i, inst_array[i].irq_idx);
+		if (IS_ERR(spi_dev)) {
+			ret = PTR_ERR(spi_dev);
+			goto error;
+		}
+
+		ctlr = spi_dev->controller;
+
+		strscpy(spi_dev->modalias, inst_array[i].type, sizeof(spi_dev->modalias));
+
+		if (spi_dev->irq < 0) {
+			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);
+
+	return bmi->spi_num;
+error:
+	bmi_devs_unregister(bmi);
+	dev_err_probe(dev, ret, "SPI error %d\n", ret);
+
+	return ret;
+
 }
 
 /**
@@ -125,14 +225,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 +247,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_DETECT */
+	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_DETECT 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 +296,36 @@  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" },
+		{}
+	},
+	.bus_type = BMI_I2C,
 };
 
-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" },
+		{}
+	},
+	.bus_type = BMI_I2C,
 };
 
-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 },
+		{}
+	},
+	.bus_type = BMI_I2C,
 };
 
 /*
@@ -193,9 +333,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);