Re: [PATCH v2 2/3] spi / ACPI: add ACPI enumeration support

["Rafael J. Wysocki" <rjw@sisk.pl>]

On Thursday, November 15, 2012 01:03:16 PM Mika Westerberg wrote:
> ACPI 5 introduced SPISerialBus resource that allows us to enumerate and
> configure the SPI slave devices behind the SPI controller. This patch adds
> support for this to the SPI core.
> 
> In addition we bind ACPI nodes to SPI devices. This makes it possible for
> the slave drivers to get the ACPI handle for further configuration.
> 
> Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> ---
>  drivers/spi/spi.c |  201 ++++++++++++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 200 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
> index 84c2861..e39a484 100644
> --- a/drivers/spi/spi.c
> +++ b/drivers/spi/spi.c
> @@ -35,6 +35,8 @@
>  #include <linux/sched.h>
>  #include <linux/delay.h>
>  #include <linux/kthread.h>
> +#include <linux/ioport.h>
> +#include <linux/acpi.h>
>  
>  static void spidev_release(struct device *dev)
>  {
> @@ -93,6 +95,10 @@ static int spi_match_device(struct device *dev, struct device_driver *drv)
>  	if (of_driver_match_device(dev, drv))
>  		return 1;
>  
> +	/* Then try ACPI */
> +	if (acpi_driver_match_device(dev, drv))
> +		return 1;
> +
>  	if (sdrv->id_table)
>  		return !!spi_match_id(sdrv->id_table, spi);
>  
> @@ -888,6 +894,196 @@ static void of_register_spi_devices(struct spi_master *master)
>  static void of_register_spi_devices(struct spi_master *master) { }
>  #endif
>  
> +#ifdef CONFIG_ACPI
> +static int acpi_spi_add_resource(struct acpi_resource *ares, void *data)
> +{
> +	struct acpi_resource_spi_serialbus *sb;
> +	struct spi_device *spi = data;
> +
> +	if (ares->type != ACPI_RESOURCE_TYPE_SERIAL_BUS)
> +		return 0;
> +
> +	sb = &ares->data.spi_serial_bus;
> +	if (sb->type != ACPI_RESOURCE_SERIAL_TYPE_SPI)
> +		return 0;
> +
> +	spi->chip_select = sb->device_selection;
> +	spi->max_speed_hz = sb->connection_speed;
> +
> +	if (sb->clock_phase == ACPI_SPI_SECOND_PHASE)
> +		spi->mode |= SPI_CPHA;
> +	if (sb->clock_polarity == ACPI_SPI_START_HIGH)
> +		spi->mode |= SPI_CPOL;
> +	if (sb->device_polarity == ACPI_SPI_ACTIVE_HIGH)
> +		spi->mode |= SPI_CS_HIGH;
> +
> +	return 1;
> +}
> +
> +static acpi_status acpi_spi_add_device(acpi_handle handle, u32 level,
> +				       void *data, void **return_value)
> +{
> +	struct spi_master *master = data;
> +	struct resource_list_entry *rentry;
> +	struct list_head resource_list;
> +	struct acpi_device *adev;
> +	struct spi_device *spi;
> +	int ret;
> +
> +	if (acpi_bus_get_device(handle, &adev))
> +		return AE_OK;
> +	if (acpi_bus_get_status(adev) || !adev->status.present)
> +		return AE_OK;
> +
> +	spi = spi_alloc_device(master);
> +	if (!spi) {
> +		dev_err(&master->dev, "failed to allocate SPI device for %s\n",
> +			dev_name(&adev->dev));
> +		return AE_NO_MEMORY;
> +	}
> +
> +	INIT_LIST_HEAD(&resource_list);
> +	ret = acpi_dev_get_resources(adev, &resource_list,
> +				     acpi_spi_add_resource, spi);
> +	if (ret < 0)
> +		goto fail_put_dev;
> +
> +	list_for_each_entry(rentry, &resource_list, node) {
> +		struct resource *r = &rentry->res;
> +
> +		if (resource_type(r) == IORESOURCE_IRQ) {
> +			spi->irq = r->start;
> +			break;
> +		}
> +	}
> +
> +	acpi_dev_free_resource_list(&resource_list);

A potential problem is lurking here, or rather two of them.

Suppose there are multiple IRQ resources for this device.  The code
above will cause GSIs to be registered for all of them, but we'll
use only one eventually.  Moreover, we'll use the last one, but
perhaps we should use the first one instead?

Maybe a better approach would be to make acpi_spi_add_resource()
call acpi_dev_resource_interrupt(ares, 0, &r) directly and then skip all of
the remaining IRQ resources if it finds one?

For example set spi->irq to -1 initially, assign it if an IRQ
resource is found and skip all of the remaining IRQ resources if
it is non-negative?

Rafael


-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.