| Message ID | 1351928793-14375-3-git-send-email-mika.westerberg@linux.intel.com (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
On Sat, Nov 3, 2012 at 1:46 AM, Mika Westerberg <mika.westerberg@linux.intel.com> 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> > Acked-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > --- > drivers/spi/spi.c | 231 ++++++++++++++++++++++++++++++++++++++++++++++++++++- > 1 file changed, 230 insertions(+), 1 deletion(-) > > diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c > index 84c2861..de22a6e 100644 > --- a/drivers/spi/spi.c > +++ b/drivers/spi/spi.c > @@ -35,6 +35,7 @@ > #include <linux/sched.h> > #include <linux/delay.h> > #include <linux/kthread.h> > +#include <linux/acpi.h> > > static void spidev_release(struct device *dev) > { > @@ -93,6 +94,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 +893,227 @@ static void of_register_spi_devices(struct spi_master *master) > static void of_register_spi_devices(struct spi_master *master) { } > #endif > > +#ifdef CONFIG_ACPI > +struct acpi_spi { > + acpi_status (*callback)(struct acpi_device *, void *); > + void *data; > +}; > + > +static acpi_status acpi_spi_enumerate_device(acpi_handle handle, u32 level, > + void *data, void **return_value) > +{ > + struct acpi_spi *acpi_spi = data; > + struct acpi_device *adev; > + > + if (acpi_bus_get_device(handle, &adev)) > + return AE_OK; > + if (acpi_bus_get_status(adev) || !adev->status.present) > + return AE_OK; > + > + return acpi_spi->callback(adev, acpi_spi->data); > +} > + > +static acpi_status acpi_spi_enumerate(acpi_handle handle, > + acpi_status (*callback)(struct acpi_device *, void *), void *data) > +{ > + struct acpi_spi acpi_spi; > + > + acpi_spi.callback = callback; > + acpi_spi.data = data; > + > + return acpi_walk_namespace(ACPI_TYPE_DEVICE, handle, 1, > + acpi_spi_enumerate_device, NULL, > + &acpi_spi, NULL); > +} > + > +struct acpi_spi_device_info { > + struct spi_device *spi; > + int triggering; > + int polarity; > + int gsi; > + bool valid; > +}; > + > +static acpi_status acpi_spi_add_resources(struct acpi_resource *res, void *data) > +{ > + struct acpi_spi_device_info *info = data; > + struct acpi_resource_spi_serialbus *sb; > + struct spi_device *spi = info->spi; > + > + switch (res->type) { > + case ACPI_RESOURCE_TYPE_SERIAL_BUS: > + sb = &res->data.spi_serial_bus; > + if (sb->type == ACPI_RESOURCE_SERIAL_TYPE_SPI) { > + spi->chip_select = sb->device_selection; > + spi->max_speed_hz = sb->connection_speed; > + > + /* Mode (clock phase/polarity/etc. */ > + 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; > + > + /* > + * The info is valid once we have found the > + * SPISerialBus resource. > + */ > + info->valid = true; > + } > + break; > + > + case ACPI_RESOURCE_TYPE_IRQ: > + info->gsi = res->data.irq.interrupts[0]; > + info->triggering = res->data.irq.triggering; > + info->polarity = res->data.irq.polarity; > + break; > + > + case ACPI_RESOURCE_TYPE_EXTENDED_IRQ: > + info->gsi = res->data.extended_irq.interrupts[0]; > + info->triggering = res->data.extended_irq.triggering; > + info->polarity = res->data.extended_irq.polarity; A driver doesn't seem like the right place for _CRS parsing code. I think the intent of _CRS is to describe resources that need to be coordinated across all devices, e.g., MMIO space, I/O port space, and IRQs. Since these resources require system-wide coordination, even when we don't have drivers for some devices, the ACPI core should be able to parse _CRS without needing any device-specific knowledge. I know the Linux ACPI core doesn't parse _CRS today, but it should. The only reason we get away with the core ignoring _CRS is because the BIOS sets up most ACPI devices and we never change them. If we change any resource assignments, we have to know where all the other devices are so we can avoid conflicts. > + break; > + } > + > + return AE_OK; > +} > + > +static acpi_status acpi_spi_add_device(struct acpi_device *adev, void *data) > +{ > + struct acpi_spi_device_info info; > + struct spi_master *master = data; > + struct spi_device *spi; > + acpi_status status; > + > + spi = spi_alloc_device(master); > + if (!spi) { > + dev_err(&master->dev, "failed to allocate SPI device\n"); > + return AE_ERROR; > + } > + > + memset(&info, 0, sizeof(info)); > + info.spi = spi; > + info.gsi = -1; > + > + status = acpi_walk_resources(adev->handle, METHOD_NAME__CRS, > + acpi_spi_add_resources, &info); > + if (ACPI_FAILURE(status) || !info.valid) > + goto fail_put_dev; > + > + strlcpy(spi->modalias, acpi_device_hid(adev), sizeof(spi->modalias)); > + if (info.gsi >= 0) > + spi->irq = acpi_register_gsi(&adev->dev, info.gsi, > + info.triggering, info.polarity); > + request_module(spi->modalias); > + if (spi_add_device(spi)) { > + dev_err(&master->dev, "failed to add SPI device from ACPI\n"); > + goto fail_unregister_gsi; > + } > + > + return AE_OK; > + > + fail_unregister_gsi: > + if (info.gsi >= 0) > + acpi_unregister_gsi(info.gsi); > + fail_put_dev: > + spi_dev_put(spi); > + > + return AE_OK; > +} > + > +static void acpi_register_spi_devices(struct spi_master *master) > +{ > + acpi_status status; > + acpi_handle handle; > + > + handle = master->dev.acpi_handle; > + if (!handle) > + return; > + > + status = acpi_spi_enumerate(handle, acpi_spi_add_device, master); How does this work with hot-plug? acpi_spi_enumerate() walks a portion of the namespace. How do we deal with changes to that part of the namespace? For example, what happens if this part of the namespace gets pruned because an enclosing device is removed? Is there a way to discover new SPI devices if they get added? > + if (ACPI_FAILURE(status)) > + dev_warn(&master->dev, "failed to enumerate SPI slaves\n"); > +} > + > +struct acpi_spi_find { > + acpi_handle handle; > + u16 chip_select; > + bool found; > +}; > + > +static acpi_status acpi_spi_find_child_address(struct acpi_resource *res, > + void *data) > +{ > + struct acpi_resource_spi_serialbus *sb; > + struct acpi_spi_find *spi_find = data; > + > + if (res->type != ACPI_RESOURCE_TYPE_SERIAL_BUS) > + return AE_OK; > + > + sb = &res->data.spi_serial_bus; > + if (sb->type != ACPI_RESOURCE_SERIAL_TYPE_SPI) > + return AE_OK; > + > + if (sb->device_selection == spi_find->chip_select) { > + spi_find->found = true; > + return AE_CTRL_TERMINATE; > + } > + > + return AE_OK; > +} > + > +static acpi_status acpi_spi_find_child(struct acpi_device *adev, void *data) > +{ > + struct acpi_spi_find *spi_find = data; > + acpi_status status; > + > + status = acpi_walk_resources(adev->handle, METHOD_NAME__CRS, > + acpi_spi_find_child_address, spi_find); > + if (ACPI_FAILURE(status) || !spi_find->found) > + return status; > + > + spi_find->handle = adev->handle; > + return AE_CTRL_TERMINATE; > +} > + > +static int acpi_spi_find_device(struct device *dev, acpi_handle *handle) > +{ > + struct spi_device *spi = to_spi_device(dev); > + struct spi_master *master = spi->master; > + struct acpi_spi_find spi_find; > + acpi_handle parent; > + acpi_status status; > + > + parent = master->dev.acpi_handle; > + if (!parent) > + return -ENODEV; > + > + memset(&spi_find, 0, sizeof(spi_find)); > + spi_find.chip_select = spi->chip_select; > + > + status = acpi_spi_enumerate(parent, acpi_spi_find_child, &spi_find); > + if (ACPI_FAILURE(status) || !spi_find.handle) > + return -ENODEV; > + > + *handle = spi_find.handle; > + return 0; > +} > + > +static struct acpi_bus_type acpi_spi_bus = { > + .bus = &spi_bus_type, > + .find_device = acpi_spi_find_device, > +}; > + > +static void acpi_spi_bus_register(void) > +{ > + register_acpi_bus_type(&acpi_spi_bus); > +} > +#else > +static inline void acpi_register_spi_devices(struct spi_master *master) {} > +static inline void acpi_spi_bus_register(void) {} > +#endif /* CONFIG_ACPI */ > + > static void spi_master_release(struct device *dev) > { > struct spi_master *master; > @@ -1023,8 +1249,9 @@ int spi_register_master(struct spi_master *master) > spi_match_master_to_boardinfo(master, &bi->board_info); > mutex_unlock(&board_lock); > > - /* Register devices from the device tree */ > + /* Register devices from the device tree and ACPI */ > of_register_spi_devices(master); > + acpi_register_spi_devices(master); > done: > return status; > } > @@ -1550,6 +1777,8 @@ static int __init spi_init(void) > status = class_register(&spi_master_class); > if (status < 0) > goto err2; > + > + acpi_spi_bus_register(); > return 0; > > err2: > -- > 1.7.10.4 > > -- > To unsubscribe from this list: send the line "unsubscribe linux-acpi" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sat, Nov 03, 2012 at 01:42:02PM -0600, Bjorn Helgaas wrote: > On Sat, Nov 3, 2012 at 1:46 AM, Mika Westerberg > <mika.westerberg@linux.intel.com> 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> > > Acked-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > --- > > drivers/spi/spi.c | 231 ++++++++++++++++++++++++++++++++++++++++++++++++++++- > > 1 file changed, 230 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c > > index 84c2861..de22a6e 100644 > > --- a/drivers/spi/spi.c > > +++ b/drivers/spi/spi.c > > @@ -35,6 +35,7 @@ > > #include <linux/sched.h> > > #include <linux/delay.h> > > #include <linux/kthread.h> > > +#include <linux/acpi.h> > > > > static void spidev_release(struct device *dev) > > { > > @@ -93,6 +94,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 +893,227 @@ static void of_register_spi_devices(struct spi_master *master) > > static void of_register_spi_devices(struct spi_master *master) { } > > #endif > > > > +#ifdef CONFIG_ACPI > > +struct acpi_spi { > > + acpi_status (*callback)(struct acpi_device *, void *); > > + void *data; > > +}; > > + > > +static acpi_status acpi_spi_enumerate_device(acpi_handle handle, u32 level, > > + void *data, void **return_value) > > +{ > > + struct acpi_spi *acpi_spi = data; > > + struct acpi_device *adev; > > + > > + if (acpi_bus_get_device(handle, &adev)) > > + return AE_OK; > > + if (acpi_bus_get_status(adev) || !adev->status.present) > > + return AE_OK; > > + > > + return acpi_spi->callback(adev, acpi_spi->data); > > +} > > + > > +static acpi_status acpi_spi_enumerate(acpi_handle handle, > > + acpi_status (*callback)(struct acpi_device *, void *), void *data) > > +{ > > + struct acpi_spi acpi_spi; > > + > > + acpi_spi.callback = callback; > > + acpi_spi.data = data; > > + > > + return acpi_walk_namespace(ACPI_TYPE_DEVICE, handle, 1, > > + acpi_spi_enumerate_device, NULL, > > + &acpi_spi, NULL); > > +} > > + > > +struct acpi_spi_device_info { > > + struct spi_device *spi; > > + int triggering; > > + int polarity; > > + int gsi; > > + bool valid; > > +}; > > + > > +static acpi_status acpi_spi_add_resources(struct acpi_resource *res, void *data) > > +{ > > + struct acpi_spi_device_info *info = data; > > + struct acpi_resource_spi_serialbus *sb; > > + struct spi_device *spi = info->spi; > > + > > + switch (res->type) { > > + case ACPI_RESOURCE_TYPE_SERIAL_BUS: > > + sb = &res->data.spi_serial_bus; > > + if (sb->type == ACPI_RESOURCE_SERIAL_TYPE_SPI) { > > + spi->chip_select = sb->device_selection; > > + spi->max_speed_hz = sb->connection_speed; > > + > > + /* Mode (clock phase/polarity/etc. */ > > + 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; > > + > > + /* > > + * The info is valid once we have found the > > + * SPISerialBus resource. > > + */ > > + info->valid = true; > > + } > > + break; > > + > > + case ACPI_RESOURCE_TYPE_IRQ: > > + info->gsi = res->data.irq.interrupts[0]; > > + info->triggering = res->data.irq.triggering; > > + info->polarity = res->data.irq.polarity; > > + break; > > + > > + case ACPI_RESOURCE_TYPE_EXTENDED_IRQ: > > + info->gsi = res->data.extended_irq.interrupts[0]; > > + info->triggering = res->data.extended_irq.triggering; > > + info->polarity = res->data.extended_irq.polarity; > > A driver doesn't seem like the right place for _CRS parsing code. I > think the intent of _CRS is to describe resources that need to be > coordinated across all devices, e.g., MMIO space, I/O port space, and > IRQs. Since these resources require system-wide coordination, even > when we don't have drivers for some devices, the ACPI core should be > able to parse _CRS without needing any device-specific knowledge. I think the driver is the only one who really knows the resources it needs in order to talk the hardware. The purpose of the above code is to extract the resources in a suitable form so that we can create a struct spi_device out of them automatically, in a similar way than the Device Tree does. There are other things which we cannot do in the generic code, such as GPIO resources and FixedDMA resources. These should be handled by the actual driver with the help of dev->acpi_handle IMO. > I know the Linux ACPI core doesn't parse _CRS today, but it should. > The only reason we get away with the core ignoring _CRS is because the > BIOS sets up most ACPI devices and we never change them. If we change > any resource assignments, we have to know where all the other devices > are so we can avoid conflicts. I agree but these devices are typically "fixed" so that they wont be hot-plugged (although we should prepare for such devices as well) so basically we don't need to do change any assignments for resources. And if the ACPI core parses the _CRS, how does it pass all the resources to the drivers? The idea here is to reuse the existing drivers as much as possible. That's why we follow what Device Tree did already. > > > + break; > > + } > > + > > + return AE_OK; > > +} > > + > > +static acpi_status acpi_spi_add_device(struct acpi_device *adev, void *data) > > +{ > > + struct acpi_spi_device_info info; > > + struct spi_master *master = data; > > + struct spi_device *spi; > > + acpi_status status; > > + > > + spi = spi_alloc_device(master); > > + if (!spi) { > > + dev_err(&master->dev, "failed to allocate SPI device\n"); > > + return AE_ERROR; > > + } > > + > > + memset(&info, 0, sizeof(info)); > > + info.spi = spi; > > + info.gsi = -1; > > + > > + status = acpi_walk_resources(adev->handle, METHOD_NAME__CRS, > > + acpi_spi_add_resources, &info); > > + if (ACPI_FAILURE(status) || !info.valid) > > + goto fail_put_dev; > > + > > + strlcpy(spi->modalias, acpi_device_hid(adev), sizeof(spi->modalias)); > > + if (info.gsi >= 0) > > + spi->irq = acpi_register_gsi(&adev->dev, info.gsi, > > + info.triggering, info.polarity); > > + request_module(spi->modalias); > > + if (spi_add_device(spi)) { > > + dev_err(&master->dev, "failed to add SPI device from ACPI\n"); > > + goto fail_unregister_gsi; > > + } > > + > > + return AE_OK; > > + > > + fail_unregister_gsi: > > + if (info.gsi >= 0) > > + acpi_unregister_gsi(info.gsi); > > + fail_put_dev: > > + spi_dev_put(spi); > > + > > + return AE_OK; > > +} > > + > > +static void acpi_register_spi_devices(struct spi_master *master) > > +{ > > + acpi_status status; > > + acpi_handle handle; > > + > > + handle = master->dev.acpi_handle; > > + if (!handle) > > + return; > > + > > + status = acpi_spi_enumerate(handle, acpi_spi_add_device, master); > > How does this work with hot-plug? acpi_spi_enumerate() walks a > portion of the namespace. How do we deal with changes to that part of > the namespace? For example, what happens if this part of the > namespace gets pruned because an enclosing device is removed? Is > there a way to discover new SPI devices if they get added? I'm not aware that we even support SPI hot-plug in the first place (well, I'm not sure that the SPI bus even supports such thing). Typically SPI devices are pretty much "static", at least that is my understanding. The platform device type ACPI support doesn't yet handle removing of the device and same goes with this code (typically the SPI master is based on a platform device). Eventually we should prepare for hot-plugging the SPI master device and delete the slave device when the master is removed. -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Saturday, November 03, 2012 01:42:02 PM Bjorn Helgaas wrote: > On Sat, Nov 3, 2012 at 1:46 AM, Mika Westerberg > <mika.westerberg@linux.intel.com> 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> > > Acked-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > --- > > drivers/spi/spi.c | 231 ++++++++++++++++++++++++++++++++++++++++++++++++++++- > > 1 file changed, 230 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c > > index 84c2861..de22a6e 100644 > > --- a/drivers/spi/spi.c > > +++ b/drivers/spi/spi.c > > @@ -35,6 +35,7 @@ > > #include <linux/sched.h> > > #include <linux/delay.h> > > #include <linux/kthread.h> > > +#include <linux/acpi.h> > > > > static void spidev_release(struct device *dev) > > { > > @@ -93,6 +94,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 +893,227 @@ static void of_register_spi_devices(struct spi_master *master) > > static void of_register_spi_devices(struct spi_master *master) { } > > #endif > > > > +#ifdef CONFIG_ACPI > > +struct acpi_spi { > > + acpi_status (*callback)(struct acpi_device *, void *); > > + void *data; > > +}; > > + > > +static acpi_status acpi_spi_enumerate_device(acpi_handle handle, u32 level, > > + void *data, void **return_value) > > +{ > > + struct acpi_spi *acpi_spi = data; > > + struct acpi_device *adev; > > + > > + if (acpi_bus_get_device(handle, &adev)) > > + return AE_OK; > > + if (acpi_bus_get_status(adev) || !adev->status.present) > > + return AE_OK; > > + > > + return acpi_spi->callback(adev, acpi_spi->data); > > +} > > + > > +static acpi_status acpi_spi_enumerate(acpi_handle handle, > > + acpi_status (*callback)(struct acpi_device *, void *), void *data) > > +{ > > + struct acpi_spi acpi_spi; > > + > > + acpi_spi.callback = callback; > > + acpi_spi.data = data; > > + > > + return acpi_walk_namespace(ACPI_TYPE_DEVICE, handle, 1, > > + acpi_spi_enumerate_device, NULL, > > + &acpi_spi, NULL); > > +} > > + > > +struct acpi_spi_device_info { > > + struct spi_device *spi; > > + int triggering; > > + int polarity; > > + int gsi; > > + bool valid; > > +}; > > + > > +static acpi_status acpi_spi_add_resources(struct acpi_resource *res, void *data) > > +{ > > + struct acpi_spi_device_info *info = data; > > + struct acpi_resource_spi_serialbus *sb; > > + struct spi_device *spi = info->spi; > > + > > + switch (res->type) { > > + case ACPI_RESOURCE_TYPE_SERIAL_BUS: > > + sb = &res->data.spi_serial_bus; > > + if (sb->type == ACPI_RESOURCE_SERIAL_TYPE_SPI) { > > + spi->chip_select = sb->device_selection; > > + spi->max_speed_hz = sb->connection_speed; > > + > > + /* Mode (clock phase/polarity/etc. */ > > + 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; > > + > > + /* > > + * The info is valid once we have found the > > + * SPISerialBus resource. > > + */ > > + info->valid = true; > > + } > > + break; > > + > > + case ACPI_RESOURCE_TYPE_IRQ: > > + info->gsi = res->data.irq.interrupts[0]; > > + info->triggering = res->data.irq.triggering; > > + info->polarity = res->data.irq.polarity; > > + break; > > + > > + case ACPI_RESOURCE_TYPE_EXTENDED_IRQ: > > + info->gsi = res->data.extended_irq.interrupts[0]; > > + info->triggering = res->data.extended_irq.triggering; > > + info->polarity = res->data.extended_irq.polarity; > > A driver doesn't seem like the right place for _CRS parsing code. This is not a driver, however. :-) > I think the intent of _CRS is to describe resources that need to be > coordinated across all devices, e.g., MMIO space, I/O port space, and > IRQs. Since these resources require system-wide coordination, even > when we don't have drivers for some devices, the ACPI core should be > able to parse _CRS without needing any device-specific knowledge. Hmm. So you would like the ACPI core to parse _CRS centrally for each device node and create an SPI device object and run spi_add_device() to register it whenever it finds ACPI_RESOURCE_TYPE_SERIAL_BUS/ACPI_RESOURCE_SERIAL_TYPE_SPI? And analogously for I2C? That might work too. > I know the Linux ACPI core doesn't parse _CRS today, but it should. > The only reason we get away with the core ignoring _CRS is because the > BIOS sets up most ACPI devices and we never change them. If we change > any resource assignments, we have to know where all the other devices > are so we can avoid conflicts. Well, point taken. > > + break; > > + } > > + > > + return AE_OK; > > +} > > + > > +static acpi_status acpi_spi_add_device(struct acpi_device *adev, void *data) > > +{ > > + struct acpi_spi_device_info info; > > + struct spi_master *master = data; > > + struct spi_device *spi; > > + acpi_status status; > > + > > + spi = spi_alloc_device(master); > > + if (!spi) { > > + dev_err(&master->dev, "failed to allocate SPI device\n"); > > + return AE_ERROR; > > + } > > + > > + memset(&info, 0, sizeof(info)); > > + info.spi = spi; > > + info.gsi = -1; > > + > > + status = acpi_walk_resources(adev->handle, METHOD_NAME__CRS, > > + acpi_spi_add_resources, &info); > > + if (ACPI_FAILURE(status) || !info.valid) > > + goto fail_put_dev; > > + > > + strlcpy(spi->modalias, acpi_device_hid(adev), sizeof(spi->modalias)); > > + if (info.gsi >= 0) > > + spi->irq = acpi_register_gsi(&adev->dev, info.gsi, > > + info.triggering, info.polarity); > > + request_module(spi->modalias); > > + if (spi_add_device(spi)) { > > + dev_err(&master->dev, "failed to add SPI device from ACPI\n"); > > + goto fail_unregister_gsi; > > + } > > + > > + return AE_OK; > > + > > + fail_unregister_gsi: > > + if (info.gsi >= 0) > > + acpi_unregister_gsi(info.gsi); > > + fail_put_dev: > > + spi_dev_put(spi); > > + > > + return AE_OK; > > +} > > + > > +static void acpi_register_spi_devices(struct spi_master *master) > > +{ > > + acpi_status status; > > + acpi_handle handle; > > + > > + handle = master->dev.acpi_handle; > > + if (!handle) > > + return; > > + > > + status = acpi_spi_enumerate(handle, acpi_spi_add_device, master); > > How does this work with hot-plug? acpi_spi_enumerate() walks a > portion of the namespace. How do we deal with changes to that part of > the namespace? For example, what happens if this part of the > namespace gets pruned because an enclosing device is removed? Is > there a way to discover new SPI devices if they get added? Yes, there should be a way to do that eventually. No, we don't have any removable SPI devices described by ACPI yet, as far as I know. So even if we added code for that now, we wouldn't be able to test it anyway with any real hardware until such devices become available. I have no idea when that's going to happen, though. > > + if (ACPI_FAILURE(status)) > > + dev_warn(&master->dev, "failed to enumerate SPI slaves\n"); > > +} > > + > > +struct acpi_spi_find { > > + acpi_handle handle; > > + u16 chip_select; > > + bool found; > > +}; > > + > > +static acpi_status acpi_spi_find_child_address(struct acpi_resource *res, > > + void *data) > > +{ > > + struct acpi_resource_spi_serialbus *sb; > > + struct acpi_spi_find *spi_find = data; > > + > > + if (res->type != ACPI_RESOURCE_TYPE_SERIAL_BUS) > > + return AE_OK; > > + > > + sb = &res->data.spi_serial_bus; > > + if (sb->type != ACPI_RESOURCE_SERIAL_TYPE_SPI) > > + return AE_OK; > > + > > + if (sb->device_selection == spi_find->chip_select) { > > + spi_find->found = true; > > + return AE_CTRL_TERMINATE; > > + } > > + > > + return AE_OK; > > +} > > + > > +static acpi_status acpi_spi_find_child(struct acpi_device *adev, void *data) > > +{ > > + struct acpi_spi_find *spi_find = data; > > + acpi_status status; > > + > > + status = acpi_walk_resources(adev->handle, METHOD_NAME__CRS, > > + acpi_spi_find_child_address, spi_find); > > + if (ACPI_FAILURE(status) || !spi_find->found) > > + return status; > > + > > + spi_find->handle = adev->handle; > > + return AE_CTRL_TERMINATE; > > +} > > + > > +static int acpi_spi_find_device(struct device *dev, acpi_handle *handle) > > +{ > > + struct spi_device *spi = to_spi_device(dev); > > + struct spi_master *master = spi->master; > > + struct acpi_spi_find spi_find; > > + acpi_handle parent; > > + acpi_status status; > > + > > + parent = master->dev.acpi_handle; > > + if (!parent) > > + return -ENODEV; > > + > > + memset(&spi_find, 0, sizeof(spi_find)); > > + spi_find.chip_select = spi->chip_select; > > + > > + status = acpi_spi_enumerate(parent, acpi_spi_find_child, &spi_find); > > + if (ACPI_FAILURE(status) || !spi_find.handle) > > + return -ENODEV; > > + > > + *handle = spi_find.handle; > > + return 0; > > +} > > + > > +static struct acpi_bus_type acpi_spi_bus = { > > + .bus = &spi_bus_type, > > + .find_device = acpi_spi_find_device, > > +}; > > + > > +static void acpi_spi_bus_register(void) > > +{ > > + register_acpi_bus_type(&acpi_spi_bus); > > +} > > +#else > > +static inline void acpi_register_spi_devices(struct spi_master *master) {} > > +static inline void acpi_spi_bus_register(void) {} > > +#endif /* CONFIG_ACPI */ > > + > > static void spi_master_release(struct device *dev) > > { > > struct spi_master *master; > > @@ -1023,8 +1249,9 @@ int spi_register_master(struct spi_master *master) > > spi_match_master_to_boardinfo(master, &bi->board_info); > > mutex_unlock(&board_lock); > > > > - /* Register devices from the device tree */ > > + /* Register devices from the device tree and ACPI */ > > of_register_spi_devices(master); > > + acpi_register_spi_devices(master); > > done: > > return status; > > } > > @@ -1550,6 +1777,8 @@ static int __init spi_init(void) > > status = class_register(&spi_master_class); > > if (status < 0) > > goto err2; > > + > > + acpi_spi_bus_register(); > > return 0; > > > > err2: > > -- Thanks, Rafael
On Saturday, November 03, 2012 10:13:10 PM Mika Westerberg wrote: > On Sat, Nov 03, 2012 at 01:42:02PM -0600, Bjorn Helgaas wrote: > > On Sat, Nov 3, 2012 at 1:46 AM, Mika Westerberg > > <mika.westerberg@linux.intel.com> 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> > > > Acked-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > > --- > > > drivers/spi/spi.c | 231 ++++++++++++++++++++++++++++++++++++++++++++++++++++- > > > 1 file changed, 230 insertions(+), 1 deletion(-) > > > > > > diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c > > > index 84c2861..de22a6e 100644 > > > --- a/drivers/spi/spi.c > > > +++ b/drivers/spi/spi.c > > > @@ -35,6 +35,7 @@ > > > #include <linux/sched.h> > > > #include <linux/delay.h> > > > #include <linux/kthread.h> > > > +#include <linux/acpi.h> > > > > > > static void spidev_release(struct device *dev) > > > { > > > @@ -93,6 +94,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 +893,227 @@ static void of_register_spi_devices(struct spi_master *master) > > > static void of_register_spi_devices(struct spi_master *master) { } > > > #endif > > > > > > +#ifdef CONFIG_ACPI > > > +struct acpi_spi { > > > + acpi_status (*callback)(struct acpi_device *, void *); > > > + void *data; > > > +}; > > > + > > > +static acpi_status acpi_spi_enumerate_device(acpi_handle handle, u32 level, > > > + void *data, void **return_value) > > > +{ > > > + struct acpi_spi *acpi_spi = data; > > > + struct acpi_device *adev; > > > + > > > + if (acpi_bus_get_device(handle, &adev)) > > > + return AE_OK; > > > + if (acpi_bus_get_status(adev) || !adev->status.present) > > > + return AE_OK; > > > + > > > + return acpi_spi->callback(adev, acpi_spi->data); > > > +} > > > + > > > +static acpi_status acpi_spi_enumerate(acpi_handle handle, > > > + acpi_status (*callback)(struct acpi_device *, void *), void *data) > > > +{ > > > + struct acpi_spi acpi_spi; > > > + > > > + acpi_spi.callback = callback; > > > + acpi_spi.data = data; > > > + > > > + return acpi_walk_namespace(ACPI_TYPE_DEVICE, handle, 1, > > > + acpi_spi_enumerate_device, NULL, > > > + &acpi_spi, NULL); > > > +} > > > + > > > +struct acpi_spi_device_info { > > > + struct spi_device *spi; > > > + int triggering; > > > + int polarity; > > > + int gsi; > > > + bool valid; > > > +}; > > > + > > > +static acpi_status acpi_spi_add_resources(struct acpi_resource *res, void *data) > > > +{ > > > + struct acpi_spi_device_info *info = data; > > > + struct acpi_resource_spi_serialbus *sb; > > > + struct spi_device *spi = info->spi; > > > + > > > + switch (res->type) { > > > + case ACPI_RESOURCE_TYPE_SERIAL_BUS: > > > + sb = &res->data.spi_serial_bus; > > > + if (sb->type == ACPI_RESOURCE_SERIAL_TYPE_SPI) { > > > + spi->chip_select = sb->device_selection; > > > + spi->max_speed_hz = sb->connection_speed; > > > + > > > + /* Mode (clock phase/polarity/etc. */ > > > + 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; > > > + > > > + /* > > > + * The info is valid once we have found the > > > + * SPISerialBus resource. > > > + */ > > > + info->valid = true; > > > + } > > > + break; > > > + > > > + case ACPI_RESOURCE_TYPE_IRQ: > > > + info->gsi = res->data.irq.interrupts[0]; > > > + info->triggering = res->data.irq.triggering; > > > + info->polarity = res->data.irq.polarity; > > > + break; > > > + > > > + case ACPI_RESOURCE_TYPE_EXTENDED_IRQ: > > > + info->gsi = res->data.extended_irq.interrupts[0]; > > > + info->triggering = res->data.extended_irq.triggering; > > > + info->polarity = res->data.extended_irq.polarity; > > > > A driver doesn't seem like the right place for _CRS parsing code. I > > think the intent of _CRS is to describe resources that need to be > > coordinated across all devices, e.g., MMIO space, I/O port space, and > > IRQs. Since these resources require system-wide coordination, even > > when we don't have drivers for some devices, the ACPI core should be > > able to parse _CRS without needing any device-specific knowledge. > > I think the driver is the only one who really knows the resources it needs > in order to talk the hardware. > > The purpose of the above code is to extract the resources in a suitable > form so that we can create a struct spi_device out of them automatically, > in a similar way than the Device Tree does. > > There are other things which we cannot do in the generic code, such as GPIO > resources and FixedDMA resources. These should be handled by the actual > driver with the help of dev->acpi_handle IMO. > > > I know the Linux ACPI core doesn't parse _CRS today, but it should. > > The only reason we get away with the core ignoring _CRS is because the > > BIOS sets up most ACPI devices and we never change them. If we change > > any resource assignments, we have to know where all the other devices > > are so we can avoid conflicts. > > I agree but these devices are typically "fixed" so that they wont be > hot-plugged (although we should prepare for such devices as well) so > basically we don't need to do change any assignments for resources. > > And if the ACPI core parses the _CRS, how does it pass all the resources to > the drivers? Pretty much the same way the $subject patch does. Instead of parsing the entire subtree below an SPI controller and trying acpi_spi_add_device() for each device node in there, it could call acpi_spi_add_device() whenever it finds a device of type ACPI_RESOURCE_TYPE_SERIAL_BUS/ACPI_RESOURCE_SERIAL_TYPE_SPI. The only problem is how to pass "master" to it. So Bjorn, do you have any idea how we could pass the "master" pointer from the ACPI core to acpi_spi_add_device() in a sensible way? An alternative might be to store the information obtained from _CRS in struct acpi_device objects created by the ACPI core while parsing the namespace. We do that already for things like _PRW, so we might as well do it for _CRS. Then, the SPI core could just walk the subtree of the device hierarchy below the SPI controller's acpi_device to extract that information. Maybe that's the way to go? Rafael
On Saturday, November 03, 2012 09:59:28 PM Rafael J. Wysocki wrote: > On Saturday, November 03, 2012 10:13:10 PM Mika Westerberg wrote: > > On Sat, Nov 03, 2012 at 01:42:02PM -0600, Bjorn Helgaas wrote: > > > On Sat, Nov 3, 2012 at 1:46 AM, Mika Westerberg > > > <mika.westerberg@linux.intel.com> 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. [...] > > And if the ACPI core parses the _CRS, how does it pass all the resources to > > the drivers? > > Pretty much the same way the $subject patch does. > > Instead of parsing the entire subtree below an SPI controller and trying > acpi_spi_add_device() for each device node in there, it could call > acpi_spi_add_device() whenever it finds a device of type > ACPI_RESOURCE_TYPE_SERIAL_BUS/ACPI_RESOURCE_SERIAL_TYPE_SPI. > The only problem is how to pass "master" to it. > > So Bjorn, do you have any idea how we could pass the "master" pointer from the > ACPI core to acpi_spi_add_device() in a sensible way? > > An alternative might be to store the information obtained from _CRS in > struct acpi_device objects created by the ACPI core while parsing the > namespace. We do that already for things like _PRW, so we might as well do it > for _CRS. Then, the SPI core could just walk the subtree of the device hierarchy > below the SPI controller's acpi_device to extract that information. > Maybe that's the way to go? The general idea is to move the _CRS parsing routine from acpi_platform.c to scan.c and make it attach resource objects to struct acpi_device. I'm thinking about adding a list head to struct acpi_device pointing to a list of entries like: struct resource_list_entry { struct list_head node; struct resource *resources; unsigned int count; }; where "resources" is an array of resources (e.g. interrupts) in the given entry and count is the size of that array. That list would contain common resources like ACPI_RESOURCE_TYPE_FIXED_MEMORY32, ACPI_RESOURCE_TYPE_IRQ, ACPI_RESOURCE_TYPE_ADDRESS32, ACPI_RESOURCE_TYPE_EXTENDED_IRQ. I think adding it would allow us to make acpi_create_platform_device(), acpi_spi_add_resources() and acpi_i2c_add_resources() more straightforward (and remove some code duplication between the last two routines). In addition to that, I'd add an entry containing serial bus information, if applicable, for the given struct acpi_device, something like: union acpi_resource_serial_bus { struct acpi_resource_common_serialbus; struct acpi_resource_spi_serialbus; struct acpi_resource_i2c_serialbus; struct acpi_resource_uart_serialbus; }; struct acpi_device { ... union acpi_resource_serial_bus *serial; ... }; Then, things like acpi_spi_add_resources() and acpi_i2c_add_resources() would be able to use struct acpi_device objects directly without running any AML. That could be done on top of the current series and I'm going to prepare a patch for that in the next few days if I find some time between the LCE sessions. Thanks, Rafael --- I speak only for myself. Rafael J. Wysocki, Intel Open Source Technology Center. -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sat, Nov 03, 2012 at 10:13:10PM +0200, Mika Westerberg wrote: > On Sat, Nov 03, 2012 at 01:42:02PM -0600, Bjorn Helgaas wrote: > > > + case ACPI_RESOURCE_TYPE_IRQ: > > > + info->gsi = res->data.irq.interrupts[0]; > > > + info->triggering = res->data.irq.triggering; > > > + info->polarity = res->data.irq.polarity; > > > + break; > > > + case ACPI_RESOURCE_TYPE_EXTENDED_IRQ: > > > + info->gsi = res->data.extended_irq.interrupts[0]; > > > + info->triggering = res->data.extended_irq.triggering; > > > + info->polarity = res->data.extended_irq.polarity; > > A driver doesn't seem like the right place for _CRS parsing code. I > > think the intent of _CRS is to describe resources that need to be > > coordinated across all devices, e.g., MMIO space, I/O port space, and > > IRQs. Since these resources require system-wide coordination, even > > when we don't have drivers for some devices, the ACPI core should be > > able to parse _CRS without needing any device-specific knowledge. > I think the driver is the only one who really knows the resources it needs > in order to talk the hardware. Generic SPI drivers expect the subsystem to supply them with an interrupt number; if there is a single interrupt it seems reasonable for the generic code to continue to do that for them when they're instantiated from ACPI. On the other hand if there are muliple interrupts it should probably give up and punt to the driver. > The purpose of the above code is to extract the resources in a suitable > form so that we can create a struct spi_device out of them automatically, > in a similar way than the Device Tree does. Definitely agreed. -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sat, Nov 03, 2012 at 09:46:32AM +0200, Mika Westerberg wrote: > + strlcpy(spi->modalias, acpi_device_hid(adev), sizeof(spi->modalias)); > + if (info.gsi >= 0) > + spi->irq = acpi_register_gsi(&adev->dev, info.gsi, > + info.triggering, info.polarity); > + request_module(spi->modalias); request_module()? Why?
On Mon, Nov 05, 2012 at 11:31:19AM +0100, Rafael J. Wysocki wrote: > The general idea is to move the _CRS parsing routine from acpi_platform.c > to scan.c and make it attach resource objects to struct acpi_device. > > I'm thinking about adding a list head to struct acpi_device pointing to a > list of entries like: > > struct resource_list_entry { > struct list_head node; > struct resource *resources; > unsigned int count; > }; > > where "resources" is an array of resources (e.g. interrupts) in the given > entry and count is the size of that array. > > That list would contain common resources like ACPI_RESOURCE_TYPE_FIXED_MEMORY32, > ACPI_RESOURCE_TYPE_IRQ, ACPI_RESOURCE_TYPE_ADDRESS32, ACPI_RESOURCE_TYPE_EXTENDED_IRQ. > I think adding it would allow us to make acpi_create_platform_device(), > acpi_spi_add_resources() and acpi_i2c_add_resources() more straightforward (and > remove some code duplication between the last two routines). This certainly sounds good to me. > In addition to that, I'd add an entry containing serial bus information, if > applicable, for the given struct acpi_device, something like: > > union acpi_resource_serial_bus { > struct acpi_resource_common_serialbus; > struct acpi_resource_spi_serialbus; > struct acpi_resource_i2c_serialbus; > struct acpi_resource_uart_serialbus; > }; > > struct acpi_device { > ... > union acpi_resource_serial_bus *serial; > ... > }; It is also possible to have several serial bus connectors on a single device (although we've seen only one connector per device but it should not be limited to that). > Then, things like acpi_spi_add_resources() and acpi_i2c_add_resources() would > be able to use struct acpi_device objects directly without running any AML. > That could be done on top of the current series and I'm going to prepare a patch > for that in the next few days if I find some time between the LCE sessions. Cool :-) Let me know if you need any help, like testing the patches etc. -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Nov 05, 2012 at 12:56:02PM +0200, Mika Westerberg wrote: > > struct acpi_device { > > ... > > union acpi_resource_serial_bus *serial; > > ... > > }; > It is also possible to have several serial bus connectors on a single > device (although we've seen only one connector per device but it should not > be limited to that). I've got practical systems where there are multiple buses physically connected, though in practice almost always only one is actually used at runtime when it's I2C and SPI there are some systems (usually with other buses) where you might want to use more than one bus. Not sure those buses will fit in here though.
On Mon, Nov 05, 2012 at 11:54:55AM +0100, Mark Brown wrote: > On Sat, Nov 03, 2012 at 09:46:32AM +0200, Mika Westerberg wrote: > > > + strlcpy(spi->modalias, acpi_device_hid(adev), sizeof(spi->modalias)); > > + if (info.gsi >= 0) > > + spi->irq = acpi_register_gsi(&adev->dev, info.gsi, > > + info.triggering, info.polarity); > > + request_module(spi->modalias); > > request_module()? Why? The DT code does the same. I have no idea why it is there, really :-) I can remove it in the next version if you think it is not needed. -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Nov 05, 2012 at 01:03:22PM +0200, Mika Westerberg wrote: > On Mon, Nov 05, 2012 at 11:54:55AM +0100, Mark Brown wrote: > > On Sat, Nov 03, 2012 at 09:46:32AM +0200, Mika Westerberg wrote: > > > + strlcpy(spi->modalias, acpi_device_hid(adev), sizeof(spi->modalias)); > > > + if (info.gsi >= 0) > > > + spi->irq = acpi_register_gsi(&adev->dev, info.gsi, > > > + info.triggering, info.polarity); > > > + request_module(spi->modalias); > > request_module()? Why? > The DT code does the same. I have no idea why it is there, really :-) > I can remove it in the next version if you think it is not needed. Well, if you can't identify what it does...
On Mon, Nov 05, 2012 at 11:56:39AM +0100, Mark Brown wrote: > On Mon, Nov 05, 2012 at 12:56:02PM +0200, Mika Westerberg wrote: > > > > struct acpi_device { > > > ... > > > union acpi_resource_serial_bus *serial; > > > ... > > > }; > > > It is also possible to have several serial bus connectors on a single > > device (although we've seen only one connector per device but it should not > > be limited to that). > > I've got practical systems where there are multiple buses physically > connected, though in practice almost always only one is actually used at > runtime when it's I2C and SPI there are some systems (usually with other > buses) where you might want to use more than one bus. Not sure those > buses will fit in here though. Yeah, I just went through DSDT table of one of our machines and found a device that actually has two I2CSerialBus connectors (and those are to the same controller). What I'm not sure is that is it used to select between two different addresses or doest the device really have two physical I2C connections. -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, 5 Nov 2012 14:02:19 +0200, Mika Westerberg wrote: > On Mon, Nov 05, 2012 at 11:56:39AM +0100, Mark Brown wrote: > > I've got practical systems where there are multiple buses physically > > connected, though in practice almost always only one is actually used at > > runtime when it's I2C and SPI there are some systems (usually with other > > buses) where you might want to use more than one bus. Not sure those > > buses will fit in here though. > > Yeah, I just went through DSDT table of one of our machines and found a > device that actually has two I2CSerialBus connectors (and those are to the > same controller). What I'm not sure is that is it used to select between > two different addresses or doest the device really have two physical I2C > connections. Neither would make sense from a hardware perspective.
On Monday, November 05, 2012 01:23:50 PM Jean Delvare wrote: > On Mon, 5 Nov 2012 14:02:19 +0200, Mika Westerberg wrote: > > On Mon, Nov 05, 2012 at 11:56:39AM +0100, Mark Brown wrote: > > > I've got practical systems where there are multiple buses physically > > > connected, though in practice almost always only one is actually used at > > > runtime when it's I2C and SPI there are some systems (usually with other > > > buses) where you might want to use more than one bus. Not sure those > > > buses will fit in here though. > > > > Yeah, I just went through DSDT table of one of our machines and found a > > device that actually has two I2CSerialBus connectors (and those are to the > > same controller). What I'm not sure is that is it used to select between > > two different addresses or doest the device really have two physical I2C > > connections. > > Neither would make sense from a hardware perspective. Well, interesting. :-) I wonder what we're supposed to do with things like that? It looks like our patch [3/3] will use the last one found, but is that correct? Should it use the first one instead, perhaps? That little conundrum aside, the observations above seem to indicate that we'd need a list of "serial" resources per struct acpi_device too. Thanks, Rafael
On Mon, Nov 05, 2012 at 01:59:23PM +0100, Rafael J. Wysocki wrote: > On Monday, November 05, 2012 01:23:50 PM Jean Delvare wrote: > > On Mon, 5 Nov 2012 14:02:19 +0200, Mika Westerberg wrote: > > > On Mon, Nov 05, 2012 at 11:56:39AM +0100, Mark Brown wrote: > > > > I've got practical systems where there are multiple buses physically > > > > connected, though in practice almost always only one is actually used at > > > > runtime when it's I2C and SPI there are some systems (usually with other > > > > buses) where you might want to use more than one bus. Not sure those > > > > buses will fit in here though. > > > > > > Yeah, I just went through DSDT table of one of our machines and found a > > > device that actually has two I2CSerialBus connectors (and those are to the > > > same controller). What I'm not sure is that is it used to select between > > > two different addresses or doest the device really have two physical I2C > > > connections. > > > > Neither would make sense from a hardware perspective. > > Well, interesting. :-) It looks like some PMICs for example have two I2C control interfaces, like TI's TWL6030 if I'm not mistaken. If both are put behind the same I2C controller with different address, you have the situation like above. -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Nov 5, 2012 at 2:15 PM, Mika Westerberg <mika.westerberg@linux.intel.com> wrote: > On Mon, Nov 05, 2012 at 01:59:23PM +0100, Rafael J. Wysocki wrote: >> On Monday, November 05, 2012 01:23:50 PM Jean Delvare wrote: >> > On Mon, 5 Nov 2012 14:02:19 +0200, Mika Westerberg wrote: >> > > On Mon, Nov 05, 2012 at 11:56:39AM +0100, Mark Brown wrote: (...) >> > > Yeah, I just went through DSDT table of one of our machines and found a >> > > device that actually has two I2CSerialBus connectors (and those are to the >> > > same controller). What I'm not sure is that is it used to select between >> > > two different addresses or doest the device really have two physical I2C >> > > connections. >> > >> > Neither would make sense from a hardware perspective. >> >> Well, interesting. :-) > > It looks like some PMICs for example have two I2C control interfaces, like > TI's TWL6030 if I'm not mistaken. If both are put behind the same I2C > controller with different address, you have the situation like above. This is quite common. So for example the AB8500 (drivers/mfd/ab8500-core.c) has this. The reason is usually that the device has more than 256 registers to the address space simply is not big enough. What we do is refer to the subaddresses as "banks" and they happen to always be at the next consecutive address so n+1. So the same device appear behind several addresses just to support a lot of registers. So you're not actually modelling the devices on the other end but the multiple addresses of a single device. If the addresses are consecutive like ours it makes sense to just instantiate one device and have the driver know that it will also be able to access address +1, +2 ... +n. So is it possible to group the consecutive related addresses after each other here at the acpi-spi level and just create a single device? If the addresses are not consecutive I guess you need to have one device driver bind to several devices and return -EPROBE_DEFER until the driver has been probed for all of them or something like that, this is what multi-block GPIO drivers sometimes do. Yours, Linus Walleij -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Nov 05, 2012 at 02:20:52PM +0100, Linus Walleij wrote: > > > > It looks like some PMICs for example have two I2C control interfaces, like > > TI's TWL6030 if I'm not mistaken. If both are put behind the same I2C > > controller with different address, you have the situation like above. > > This is quite common. So for example the AB8500 (drivers/mfd/ab8500-core.c) > has this. The reason is usually that the device has more than 256 registers > to the address space simply is not big enough. > > What we do is refer to the subaddresses as "banks" and they happen > to always be at the next consecutive address so n+1. > > So the same device appear behind several addresses just to support > a lot of registers. > > So you're not actually modelling the devices on the other end but the > multiple addresses of a single device. That makes sense. Thanks for the explanation. > If the addresses are consecutive like ours it makes sense > to just instantiate one device and have the driver know that it will > also be able to access address +1, +2 ... +n. So is it possible > to group the consecutive related addresses after each other > here at the acpi-spi level and just create a single device? Not sure if it should be done there, unless we really can be sure that we have a single device with multiple addresses (and that is usually something that is only known by the actual driver) and not some device that has two unrelated interfaces connecting the same controller. > If the addresses are not consecutive I guess you need to have > one device driver bind to several devices and return -EPROBE_DEFER > until the driver has been probed for all of them or something like > that, this is what multi-block GPIO drivers sometimes do. The addresses in the DSDT table for this particular device are not consecutive so we could do something like you propose above. -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Linus, On Mon, 5 Nov 2012 14:20:52 +0100, Linus Walleij wrote: > On Mon, Nov 5, 2012 at 2:15 PM, Mika Westerberg > <mika.westerberg@linux.intel.com> wrote: > > On Mon, Nov 05, 2012 at 01:59:23PM +0100, Rafael J. Wysocki wrote: > >> On Monday, November 05, 2012 01:23:50 PM Jean Delvare wrote: > >> > On Mon, 5 Nov 2012 14:02:19 +0200, Mika Westerberg wrote: > >> > > On Mon, Nov 05, 2012 at 11:56:39AM +0100, Mark Brown wrote: > (...) > >> > > Yeah, I just went through DSDT table of one of our machines and found a > >> > > device that actually has two I2CSerialBus connectors (and those are to the > >> > > same controller). What I'm not sure is that is it used to select between > >> > > two different addresses or doest the device really have two physical I2C > >> > > connections. > >> > > >> > Neither would make sense from a hardware perspective. > >> > >> Well, interesting. :-) > > > > It looks like some PMICs for example have two I2C control interfaces, like > > TI's TWL6030 if I'm not mistaken. If both are put behind the same I2C > > controller with different address, you have the situation like above. Ah, OK, if this is a degenerated case of a more complex initial design then yes it makes some sense. I had never met chips like that and did not know such chips existed. That being said, from a software perspective, there is no difference between one or two I2C pin pairs being soldered. All we care about is which master they are ultimately connected to, and to which slave address(es) the chip replies. > This is quite common. So for example the AB8500 (drivers/mfd/ab8500-core.c) > has this. The reason is usually that the device has more than 256 registers > to the address space simply is not big enough. This is completely different from the case being discussed above. Even the most complex addressing scheme can be implemented using a single hardware I2C interface. You can use 16-bit register addresses (if you can live without SMBus compatibility, AT24C32 and larger EEPROMs do that), or implement internal banks (using one of the registers as the bank selector, hardware monitoring chips do that), or you can use multiple slave addresses (AT24C04 to C16 EEPROMs do that.) > What we do is refer to the subaddresses as "banks" and they happen > to always be at the next consecutive address so n+1. > > So the same device appear behind several addresses just to support > a lot of registers. > > So you're not actually modelling the devices on the other end but the > multiple addresses of a single device. > > If the addresses are consecutive like ours it makes sense > to just instantiate one device and have the driver know that it will > also be able to access address +1, +2 ... +n. So is it possible > to group the consecutive related addresses after each other > here at the acpi-spi level and just create a single device? We were actually discussing I2C here, although I admit not in the right thread, and maybe some of this applies to SPI as well. There are I2C devices replying to multiple non-consecutive slave addresses. Most notably Winbond hardware monitoring chips replying to one address in 0x2c-0x2f and 2 addresses in 0x48-0x4f. And of course there are the DDR3 DIMM SPD chips which have an EEPROM at 0x50-0x57 and a thermal sensor at 0x18-0x1f. So if multiple slave addresses must be supported, please do not limit this support to consecutive addresses. There really is no reason to limit us that way anyway, as i2c-core supports attaching any additional slave address to en existing i2c_client using i2c_new_dummy(). Note for DDR3 DIMM SPD chips we currently have two different drivers handling the two slave addresses (eeprom/at24 and jc42.) I don't know if this is something that could be instantiated from ACPI. So it seems we really have two different cases when a single chip replies to multiple slave addresses: either they refer to the same function and we want a single driver for all of them, or they are for unrelated functions and we want separate drivers (and thus separate i2c_clients.) Not sure how we can always handle that properly on the ACPI side. > If the addresses are not consecutive I guess you need to have > one device driver bind to several devices and return -EPROBE_DEFER > until the driver has been probed for all of them or something like > that, this is what multi-block GPIO drivers sometimes do. Consecutive or not makes no difference really, as long as the driver can deduce the additional addresses from the main address or register contents accessible from the main address. This has always been the case so far.
On Monday, November 05, 2012 03:03:26 PM Jean Delvare wrote: > Hi Linus, > > On Mon, 5 Nov 2012 14:20:52 +0100, Linus Walleij wrote: > > On Mon, Nov 5, 2012 at 2:15 PM, Mika Westerberg > > <mika.westerberg@linux.intel.com> wrote: > > > On Mon, Nov 05, 2012 at 01:59:23PM +0100, Rafael J. Wysocki wrote: > > >> On Monday, November 05, 2012 01:23:50 PM Jean Delvare wrote: > > >> > On Mon, 5 Nov 2012 14:02:19 +0200, Mika Westerberg wrote: > > >> > > On Mon, Nov 05, 2012 at 11:56:39AM +0100, Mark Brown wrote: > > (...) > > >> > > Yeah, I just went through DSDT table of one of our machines and found a > > >> > > device that actually has two I2CSerialBus connectors (and those are to the > > >> > > same controller). What I'm not sure is that is it used to select between > > >> > > two different addresses or doest the device really have two physical I2C > > >> > > connections. > > >> > > > >> > Neither would make sense from a hardware perspective. > > >> > > >> Well, interesting. :-) > > > > > > It looks like some PMICs for example have two I2C control interfaces, like > > > TI's TWL6030 if I'm not mistaken. If both are put behind the same I2C > > > controller with different address, you have the situation like above. > > Ah, OK, if this is a degenerated case of a more complex initial design > then yes it makes some sense. I had never met chips like that and did > not know such chips existed. > > That being said, from a software perspective, there is no difference > between one or two I2C pin pairs being soldered. All we care about is > which master they are ultimately connected to, and to which slave > address(es) the chip replies. > > > This is quite common. So for example the AB8500 (drivers/mfd/ab8500-core.c) > > has this. The reason is usually that the device has more than 256 registers > > to the address space simply is not big enough. > > This is completely different from the case being discussed above. Even > the most complex addressing scheme can be implemented using a single > hardware I2C interface. You can use 16-bit register addresses (if you > can live without SMBus compatibility, AT24C32 and larger EEPROMs do > that), or implement internal banks (using one of the registers as the > bank selector, hardware monitoring chips do that), or you can use > multiple slave addresses (AT24C04 to C16 EEPROMs do that.) > > > What we do is refer to the subaddresses as "banks" and they happen > > to always be at the next consecutive address so n+1. > > > > So the same device appear behind several addresses just to support > > a lot of registers. > > > > So you're not actually modelling the devices on the other end but the > > multiple addresses of a single device. > > > > If the addresses are consecutive like ours it makes sense > > to just instantiate one device and have the driver know that it will > > also be able to access address +1, +2 ... +n. So is it possible > > to group the consecutive related addresses after each other > > here at the acpi-spi level and just create a single device? > > We were actually discussing I2C here, although I admit not in the right > thread, and maybe some of this applies to SPI as well. > > There are I2C devices replying to multiple non-consecutive slave > addresses. Most notably Winbond hardware monitoring chips replying to > one address in 0x2c-0x2f and 2 addresses in 0x48-0x4f. And of course > there are the DDR3 DIMM SPD chips which have an EEPROM at 0x50-0x57 and > a thermal sensor at 0x18-0x1f. So if multiple slave addresses must be > supported, please do not limit this support to consecutive addresses. > There really is no reason to limit us that way anyway, as i2c-core > supports attaching any additional slave address to en existing > i2c_client using i2c_new_dummy(). > > Note for DDR3 DIMM SPD chips we currently have two different drivers > handling the two slave addresses (eeprom/at24 and jc42.) I don't know > if this is something that could be instantiated from ACPI. So it seems > we really have two different cases when a single chip replies to > multiple slave addresses: either they refer to the same function and we > want a single driver for all of them, or they are for unrelated > functions and we want separate drivers (and thus separate i2c_clients.) > Not sure how we can always handle that properly on the ACPI side. > > > If the addresses are not consecutive I guess you need to have > > one device driver bind to several devices and return -EPROBE_DEFER > > until the driver has been probed for all of them or something like > > that, this is what multi-block GPIO drivers sometimes do. > > Consecutive or not makes no difference really, as long as the driver > can deduce the additional addresses from the main address or register > contents accessible from the main address. This has always been the > case so far. In the ACPI namespace we have device nodes and serial interfaces below them. In the above case we see that a single device node supports two different interfaces and in that case we probably should create two different struct i2c_adapter objects for the same ACPI device node. Mika, what do you think? Rafael
On Mon, Nov 05, 2012 at 03:19:58PM +0100, Rafael J. Wysocki wrote: > > In the ACPI namespace we have device nodes and serial interfaces below them. > In the above case we see that a single device node supports two different > interfaces and in that case we probably should create two different > struct i2c_adapter objects for the same ACPI device node. > > Mika, what do you think? I agree. Only problem I see is that then we have two I2C adapter devices with the same ACPI ID (and hence the same i2c_client->name). I wonder what the I2C core thinks about that. -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, 5 Nov 2012 16:53:15 +0200, Mika Westerberg wrote: > On Mon, Nov 05, 2012 at 03:19:58PM +0100, Rafael J. Wysocki wrote: > > > > In the ACPI namespace we have device nodes and serial interfaces below them. > > In the above case we see that a single device node supports two different > > interfaces and in that case we probably should create two different > > struct i2c_adapter objects for the same ACPI device node. > > > > Mika, what do you think? > > I agree. > > Only problem I see is that then we have two I2C adapter devices with the > same ACPI ID (and hence the same i2c_client->name). I wonder what the I2C > core thinks about that. I2C core fears that you're mixing up everything ;) I2C adapter devices are struct i2c_adapter aka i2c-0, i2c-1 etc. i2c_client is for slave devices. There's nothing wrong with i2c_clients sharing ->name, that's even how device driver matching is achieved. The uniqueness of i2c_clients is on their bus_id which is the combination of i2c adapter number and slave address (e.g. 0-0050) i2c_adapter->name should, OTOH, be unique. In i2c bus drivers we usually append the base I/O address at the end of the name to guarantee that. ACPI will have to come up with something similar.
On Sat, Nov 3, 2012 at 2:39 PM, Rafael J. Wysocki <rjw@sisk.pl> wrote: > On Saturday, November 03, 2012 01:42:02 PM Bjorn Helgaas wrote: >> On Sat, Nov 3, 2012 at 1:46 AM, Mika Westerberg >> <mika.westerberg@linux.intel.com> 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> >> > Acked-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> >> > --- >> > drivers/spi/spi.c | 231 ++++++++++++++++++++++++++++++++++++++++++++++++++++- >> > +static acpi_status acpi_spi_add_resources(struct acpi_resource *res, void *data) >> > +{ >> > + struct acpi_spi_device_info *info = data; >> > + struct acpi_resource_spi_serialbus *sb; >> > + struct spi_device *spi = info->spi; >> > + >> > + switch (res->type) { >> > + case ACPI_RESOURCE_TYPE_SERIAL_BUS: >> > + sb = &res->data.spi_serial_bus; >> > + if (sb->type == ACPI_RESOURCE_SERIAL_TYPE_SPI) { >> > + spi->chip_select = sb->device_selection; >> > + spi->max_speed_hz = sb->connection_speed; >> > + >> > + /* Mode (clock phase/polarity/etc. */ >> > + 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; >> > + >> > + /* >> > + * The info is valid once we have found the >> > + * SPISerialBus resource. >> > + */ >> > + info->valid = true; >> > + } >> > + break; >> > + >> > + case ACPI_RESOURCE_TYPE_IRQ: >> > + info->gsi = res->data.irq.interrupts[0]; >> > + info->triggering = res->data.irq.triggering; >> > + info->polarity = res->data.irq.polarity; >> > + break; >> > + >> > + case ACPI_RESOURCE_TYPE_EXTENDED_IRQ: >> > + info->gsi = res->data.extended_irq.interrupts[0]; >> > + info->triggering = res->data.extended_irq.triggering; >> > + info->polarity = res->data.extended_irq.polarity; >> >> A driver doesn't seem like the right place for _CRS parsing code. > > This is not a driver, however. :-) OK. Can you describe what this is? I assume the picture involves an SPI master device and one or more SPI slave devices, and the ACPI namespace tells us something about them, and somehow this code is related to those devices because it's looking at _CRS for them. A _CRS method is associated with a Device in the namespace. What is that device in this case? What is its PNP ID? What driver claims devices with that ID? >> > +static void acpi_register_spi_devices(struct spi_master *master) >> > +{ >> > + acpi_status status; >> > + acpi_handle handle; >> > + >> > + handle = master->dev.acpi_handle; >> > + if (!handle) >> > + return; >> > + >> > + status = acpi_spi_enumerate(handle, acpi_spi_add_device, master); >> >> How does this work with hot-plug? acpi_spi_enumerate() walks a >> portion of the namespace. How do we deal with changes to that part of >> the namespace? For example, what happens if this part of the >> namespace gets pruned because an enclosing device is removed? Is >> there a way to discover new SPI devices if they get added? > > Yes, there should be a way to do that eventually. No, we don't have any > removable SPI devices described by ACPI yet, as far as I know. So even if > we added code for that now, we wouldn't be able to test it anyway with any > real hardware until such devices become available. I have no idea when that's > going to happen, though. I'm not asking about hotplug of devices on an SPI bus. I'm asking about hotplug of SPI masters. For example, let's say you hot-add a whole node that contains an SPI master. I assume there's some ACPI Device node that describes the SPI master, and a hot-add would add a subtree to the namespace that contains a new SPI master Device. Bjorn -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Nov 5, 2012 at 3:31 AM, Rafael J. Wysocki <rjw@sisk.pl> wrote: > On Saturday, November 03, 2012 09:59:28 PM Rafael J. Wysocki wrote: >> On Saturday, November 03, 2012 10:13:10 PM Mika Westerberg wrote: >> > On Sat, Nov 03, 2012 at 01:42:02PM -0600, Bjorn Helgaas wrote: >> > > On Sat, Nov 3, 2012 at 1:46 AM, Mika Westerberg >> > > <mika.westerberg@linux.intel.com> 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. > [...] >> > And if the ACPI core parses the _CRS, how does it pass all the resources to >> > the drivers? >> >> Pretty much the same way the $subject patch does. >> >> Instead of parsing the entire subtree below an SPI controller and trying >> acpi_spi_add_device() for each device node in there, it could call >> acpi_spi_add_device() whenever it finds a device of type >> ACPI_RESOURCE_TYPE_SERIAL_BUS/ACPI_RESOURCE_SERIAL_TYPE_SPI. >> The only problem is how to pass "master" to it. >> >> So Bjorn, do you have any idea how we could pass the "master" pointer from the >> ACPI core to acpi_spi_add_device() in a sensible way? >> >> An alternative might be to store the information obtained from _CRS in >> struct acpi_device objects created by the ACPI core while parsing the >> namespace. We do that already for things like _PRW, so we might as well do it >> for _CRS. Then, the SPI core could just walk the subtree of the device hierarchy >> below the SPI controller's acpi_device to extract that information. >> Maybe that's the way to go? > > The general idea is to move the _CRS parsing routine from acpi_platform.c > to scan.c and make it attach resource objects to struct acpi_device. > > I'm thinking about adding a list head to struct acpi_device pointing to a > list of entries like: > > struct resource_list_entry { > struct list_head node; > struct resource *resources; > unsigned int count; > }; > > where "resources" is an array of resources (e.g. interrupts) in the given > entry and count is the size of that array. > > That list would contain common resources like ACPI_RESOURCE_TYPE_FIXED_MEMORY32, > ACPI_RESOURCE_TYPE_IRQ, ACPI_RESOURCE_TYPE_ADDRESS32, ACPI_RESOURCE_TYPE_EXTENDED_IRQ. This is exactly what PNPACPI already does. For every Device node in the namespace, pnpacpi/rsparser.c parses _CRS and builds a list of struct resources that correspond to it. If you put this functionality in acpi/scan.c, should we get rid of PNPACPI? Bjorn -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Nov 05, 2012 at 04:19:20PM +0100, Jean Delvare wrote: > On Mon, 5 Nov 2012 16:53:15 +0200, Mika Westerberg wrote: > > On Mon, Nov 05, 2012 at 03:19:58PM +0100, Rafael J. Wysocki wrote: > > > > > > In the ACPI namespace we have device nodes and serial interfaces below them. > > > In the above case we see that a single device node supports two different > > > interfaces and in that case we probably should create two different > > > struct i2c_adapter objects for the same ACPI device node. > > > > > > Mika, what do you think? > > > > I agree. > > > > Only problem I see is that then we have two I2C adapter devices with the > > same ACPI ID (and hence the same i2c_client->name). I wonder what the I2C > > core thinks about that. > > I2C core fears that you're mixing up everything ;) I2C adapter devices > are struct i2c_adapter aka i2c-0, i2c-1 etc. i2c_client is for slave > devices. There's nothing wrong with i2c_clients sharing ->name, that's > even how device driver matching is achieved. The uniqueness of > i2c_clients is on their bus_id which is the combination of i2c adapter > number and slave address (e.g. 0-0050) Yeah, I mixed I2C adapter and client. Thanks for correcting. So if we create one I2C adapter from the platform bus code as we do now and then for each I2CSerialBus connector we create one I2C client (well, the one that is created when i2c_new_device() is called), everything should work, right? Then I suggest that we have a list of serial bus resources in the struct acpi_device and create the I2C clients based on that. > i2c_adapter->name should, OTOH, be unique. In i2c bus drivers we > usually append the base I/O address at the end of the name to guarantee > that. ACPI will have to come up with something similar. It should already be unique in case of ACPI. We use ACPI _HID and _UID to achieve that. -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Nov 5, 2012 at 10:12 AM, Mika Westerberg <mika.westerberg@linux.intel.com> wrote: > On Mon, Nov 05, 2012 at 04:19:20PM +0100, Jean Delvare wrote: >> On Mon, 5 Nov 2012 16:53:15 +0200, Mika Westerberg wrote: >> > On Mon, Nov 05, 2012 at 03:19:58PM +0100, Rafael J. Wysocki wrote: >> > > >> > > In the ACPI namespace we have device nodes and serial interfaces below them. >> > > In the above case we see that a single device node supports two different >> > > interfaces and in that case we probably should create two different >> > > struct i2c_adapter objects for the same ACPI device node. >> > > >> > > Mika, what do you think? >> > >> > I agree. >> > >> > Only problem I see is that then we have two I2C adapter devices with the >> > same ACPI ID (and hence the same i2c_client->name). I wonder what the I2C >> > core thinks about that. >> >> I2C core fears that you're mixing up everything ;) I2C adapter devices >> are struct i2c_adapter aka i2c-0, i2c-1 etc. i2c_client is for slave >> devices. There's nothing wrong with i2c_clients sharing ->name, that's >> even how device driver matching is achieved. The uniqueness of >> i2c_clients is on their bus_id which is the combination of i2c adapter >> number and slave address (e.g. 0-0050) > > Yeah, I mixed I2C adapter and client. Thanks for correcting. > > So if we create one I2C adapter from the platform bus code as we do now and > then for each I2CSerialBus connector we create one I2C client (well, the > one that is created when i2c_new_device() is called), everything should > work, right? > > Then I suggest that we have a list of serial bus resources in the struct > acpi_device and create the I2C clients based on that. > >> i2c_adapter->name should, OTOH, be unique. In i2c bus drivers we >> usually append the base I/O address at the end of the name to guarantee >> that. ACPI will have to come up with something similar. > > It should already be unique in case of ACPI. We use ACPI _HID and _UID to > achieve that. Using only _HID and _UID to guarantee uniqueness means you're relying on a property of the BIOS, so you're vulnerable to BIOS bugs. If there's an ACPI Device for I2C adapters, why wouldn't you just use its device name as set in acpi_device_register() (basically a _HID + instance number)? -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, 5 Nov 2012 19:12:48 +0200, Mika Westerberg wrote: > On Mon, Nov 05, 2012 at 04:19:20PM +0100, Jean Delvare wrote: > > I2C core fears that you're mixing up everything ;) I2C adapter devices > > are struct i2c_adapter aka i2c-0, i2c-1 etc. i2c_client is for slave > > devices. There's nothing wrong with i2c_clients sharing ->name, that's > > even how device driver matching is achieved. The uniqueness of > > i2c_clients is on their bus_id which is the combination of i2c adapter > > number and slave address (e.g. 0-0050) > > Yeah, I mixed I2C adapter and client. Thanks for correcting. > > So if we create one I2C adapter from the platform bus code as we do now and > then for each I2CSerialBus connector we create one I2C client (well, the > one that is created when i2c_new_device() is called), everything should > work, right? Yes. > Then I suggest that we have a list of serial bus resources in the struct > acpi_device and create the I2C clients based on that. > > > i2c_adapter->name should, OTOH, be unique. In i2c bus drivers we > > usually append the base I/O address at the end of the name to guarantee > > that. ACPI will have to come up with something similar. > > It should already be unique in case of ACPI. We use ACPI _HID and _UID to > achieve that. Perfect.
On Mon, Nov 05, 2012 at 10:43:17AM -0700, Bjorn Helgaas wrote: > > It should already be unique in case of ACPI. We use ACPI _HID and _UID to > > achieve that. > > Using only _HID and _UID to guarantee uniqueness means you're relying > on a property of the BIOS, so you're vulnerable to BIOS bugs. > > If there's an ACPI Device for I2C adapters, why wouldn't you just use > its device name as set in acpi_device_register() (basically a _HID + > instance number)? That's a good point - we could change to use that instead (the platform code in linux-pm tree uses _HID + _UID but I guess it is pretty trivial to change). -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Nov 5, 2012 at 3:03 PM, Jean Delvare <khali@linux-fr.org> wrote: (...) >> If the addresses are consecutive like ours it makes sense >> to just instantiate one device and have the driver know that it will >> also be able to access address +1, +2 ... +n. So is it possible >> to group the consecutive related addresses after each other >> here at the acpi-spi level and just create a single device? > > We were actually discussing I2C here, although I admit not in the right > thread, and maybe some of this applies to SPI as well. This is my typo. The AB8500 example is indeed I2C. > So if multiple slave addresses must be > supported, please do not limit this support to consecutive addresses. I buy that ... in Nomadik we even have an instance of a single device with two discrete *physical* interfaces connected to it, on both of them it responds to the same address, but with totally different register layouts ... hardware engineers rule! :-) > There really is no reason to limit us that way anyway, as i2c-core > supports attaching any additional slave address to en existing > i2c_client using i2c_new_dummy(). Yes, this is how we do it in AB8500. We just take advantage of the fact that the addresses are consecutive. > Note for DDR3 DIMM SPD chips we currently have two different drivers > handling the two slave addresses (eeprom/at24 and jc42.) I don't know > if this is something that could be instantiated from ACPI. So it seems > we really have two different cases when a single chip replies to > multiple slave addresses: either they refer to the same function and we > want a single driver for all of them, or they are for unrelated > functions and we want separate drivers (and thus separate i2c_clients.) > Not sure how we can always handle that properly on the ACPI side. If they can be accessed concurrently and do not share any locks or code they should be separate I think? When they have strong dependencies we immediately sort of fall into the MFD class where you need a mediator I think. >> If the addresses are not consecutive I guess you need to have >> one device driver bind to several devices and return -EPROBE_DEFER >> until the driver has been probed for all of them or something like >> that, this is what multi-block GPIO drivers sometimes do. > > Consecutive or not makes no difference really, as long as the driver > can deduce the additional addresses from the main address or register > contents accessible from the main address. This has always been the > case so far. You're right. Yours, Linus Walleij -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Nov 05, 2012 at 02:02:19PM +0200, Mika Westerberg wrote: > On Mon, Nov 05, 2012 at 11:56:39AM +0100, Mark Brown wrote: > > I've got practical systems where there are multiple buses physically > > connected, though in practice almost always only one is actually used at > > runtime when it's I2C and SPI there are some systems (usually with other > > buses) where you might want to use more than one bus. Not sure those > > buses will fit in here though. > Yeah, I just went through DSDT table of one of our machines and found a > device that actually has two I2CSerialBus connectors (and those are to the > same controller). What I'm not sure is that is it used to select between > two different addresses or doest the device really have two physical I2C > connections. That one is likely to be one device that responds on two addresses as others have mentioned. You do also get devices which have multiple physical interfaces for various reasons, such as having a dedicated bus for some functions that need to respond quickly.
On Monday, November 05, 2012 09:54:37 AM Bjorn Helgaas wrote: > On Sat, Nov 3, 2012 at 2:39 PM, Rafael J. Wysocki <rjw@sisk.pl> wrote: > > On Saturday, November 03, 2012 01:42:02 PM Bjorn Helgaas wrote: > >> On Sat, Nov 3, 2012 at 1:46 AM, Mika Westerberg > >> <mika.westerberg@linux.intel.com> 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> > >> > Acked-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > >> > --- > >> > drivers/spi/spi.c | 231 ++++++++++++++++++++++++++++++++++++++++++++++++++++- > > >> > +static acpi_status acpi_spi_add_resources(struct acpi_resource *res, void *data) > >> > +{ > >> > + struct acpi_spi_device_info *info = data; > >> > + struct acpi_resource_spi_serialbus *sb; > >> > + struct spi_device *spi = info->spi; > >> > + > >> > + switch (res->type) { > >> > + case ACPI_RESOURCE_TYPE_SERIAL_BUS: > >> > + sb = &res->data.spi_serial_bus; > >> > + if (sb->type == ACPI_RESOURCE_SERIAL_TYPE_SPI) { > >> > + spi->chip_select = sb->device_selection; > >> > + spi->max_speed_hz = sb->connection_speed; > >> > + > >> > + /* Mode (clock phase/polarity/etc. */ > >> > + 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; > >> > + > >> > + /* > >> > + * The info is valid once we have found the > >> > + * SPISerialBus resource. > >> > + */ > >> > + info->valid = true; > >> > + } > >> > + break; > >> > + > >> > + case ACPI_RESOURCE_TYPE_IRQ: > >> > + info->gsi = res->data.irq.interrupts[0]; > >> > + info->triggering = res->data.irq.triggering; > >> > + info->polarity = res->data.irq.polarity; > >> > + break; > >> > + > >> > + case ACPI_RESOURCE_TYPE_EXTENDED_IRQ: > >> > + info->gsi = res->data.extended_irq.interrupts[0]; > >> > + info->triggering = res->data.extended_irq.triggering; > >> > + info->polarity = res->data.extended_irq.polarity; > >> > >> A driver doesn't seem like the right place for _CRS parsing code. > > > > This is not a driver, however. :-) > > OK. Can you describe what this is? I assume the picture involves an > SPI master device and one or more SPI slave devices, and the ACPI > namespace tells us something about them, and somehow this code is > related to those devices because it's looking at _CRS for them. This is part of the SPI core that looks for SPI devices below a controller. > A _CRS method is associated with a Device in the namespace. What is > that device in this case? An SPI device below a controller. > What is its PNP ID? I can't answer that from the top of my head, sorry. > What driver claims devices with that ID? The driver that declares support for it in its acpi_match_table[] array. > >> > +static void acpi_register_spi_devices(struct spi_master *master) > >> > +{ > >> > + acpi_status status; > >> > + acpi_handle handle; > >> > + > >> > + handle = master->dev.acpi_handle; > >> > + if (!handle) > >> > + return; > >> > + > >> > + status = acpi_spi_enumerate(handle, acpi_spi_add_device, master); > >> > >> How does this work with hot-plug? acpi_spi_enumerate() walks a > >> portion of the namespace. How do we deal with changes to that part of > >> the namespace? For example, what happens if this part of the > >> namespace gets pruned because an enclosing device is removed? Is > >> there a way to discover new SPI devices if they get added? > > > > Yes, there should be a way to do that eventually. No, we don't have any > > removable SPI devices described by ACPI yet, as far as I know. So even if > > we added code for that now, we wouldn't be able to test it anyway with any > > real hardware until such devices become available. I have no idea when that's > > going to happen, though. > > I'm not asking about hotplug of devices on an SPI bus. I'm asking > about hotplug of SPI masters. For example, let's say you hot-add a > whole node that contains an SPI master. I assume there's some ACPI > Device node that describes the SPI master, and a hot-add would add a > subtree to the namespace that contains a new SPI master Device. The master will be a platform device and again, we're going to add support for the hotplug of those, but the SPI controllers that we currently can test it with are not removable. Thanks, Rafael
On Monday, November 05, 2012 09:54:42 AM Bjorn Helgaas wrote: > On Mon, Nov 5, 2012 at 3:31 AM, Rafael J. Wysocki <rjw@sisk.pl> wrote: > > On Saturday, November 03, 2012 09:59:28 PM Rafael J. Wysocki wrote: > >> On Saturday, November 03, 2012 10:13:10 PM Mika Westerberg wrote: > >> > On Sat, Nov 03, 2012 at 01:42:02PM -0600, Bjorn Helgaas wrote: > >> > > On Sat, Nov 3, 2012 at 1:46 AM, Mika Westerberg > >> > > <mika.westerberg@linux.intel.com> 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. > > [...] > >> > And if the ACPI core parses the _CRS, how does it pass all the resources to > >> > the drivers? > >> > >> Pretty much the same way the $subject patch does. > >> > >> Instead of parsing the entire subtree below an SPI controller and trying > >> acpi_spi_add_device() for each device node in there, it could call > >> acpi_spi_add_device() whenever it finds a device of type > >> ACPI_RESOURCE_TYPE_SERIAL_BUS/ACPI_RESOURCE_SERIAL_TYPE_SPI. > >> The only problem is how to pass "master" to it. > >> > >> So Bjorn, do you have any idea how we could pass the "master" pointer from the > >> ACPI core to acpi_spi_add_device() in a sensible way? > >> > >> An alternative might be to store the information obtained from _CRS in > >> struct acpi_device objects created by the ACPI core while parsing the > >> namespace. We do that already for things like _PRW, so we might as well do it > >> for _CRS. Then, the SPI core could just walk the subtree of the device hierarchy > >> below the SPI controller's acpi_device to extract that information. > >> Maybe that's the way to go? > > > > The general idea is to move the _CRS parsing routine from acpi_platform.c > > to scan.c and make it attach resource objects to struct acpi_device. > > > > I'm thinking about adding a list head to struct acpi_device pointing to a > > list of entries like: > > > > struct resource_list_entry { > > struct list_head node; > > struct resource *resources; > > unsigned int count; > > }; > > > > where "resources" is an array of resources (e.g. interrupts) in the given > > entry and count is the size of that array. > > > > That list would contain common resources like ACPI_RESOURCE_TYPE_FIXED_MEMORY32, > > ACPI_RESOURCE_TYPE_IRQ, ACPI_RESOURCE_TYPE_ADDRESS32, ACPI_RESOURCE_TYPE_EXTENDED_IRQ. > > This is exactly what PNPACPI already does. For every Device node in > the namespace, pnpacpi/rsparser.c parses _CRS and builds a list of > struct resources that correspond to it. If you put this functionality > in acpi/scan.c, should we get rid of PNPACPI? Quite likely. At least this part of it, if you want the core to parse resources. That said, I actually tried to abstract out resource parsing in a more generic fashion on the basis of our new platform device support code, but quite frankly I wasn't able to. The problem is that struct resource is too simple to be useful for representing all of the information that can be encoded in ACPI resources. As a result, some information have to be stored directly in things like struct pnp_dev, struct platform_device etc. and if we want to represent it generically, the only way to do that seems to be using the ACPICA resource types as defined in acrestyp.h. So we can attach a list of things like struct acpi_resource_list_entry { struct list_head node; struct acpi_resource resource; }; to struct acpi_device, but that doesn't help much, because the different bus types will then need to extract the information they need from that list anyway. It would reduce the number of _CRS invocations, but beyond that I'm not sure how usefult it's going to be. Still, we probably should centralize things like pnpacpi_parse_allocated_irqresource() to avoid duplicating that code. Thanks, Rafael
On Tue, Nov 6, 2012 at 6:43 AM, Rafael J. Wysocki <rjw@sisk.pl> wrote: > On Monday, November 05, 2012 09:54:42 AM Bjorn Helgaas wrote: >> On Mon, Nov 5, 2012 at 3:31 AM, Rafael J. Wysocki <rjw@sisk.pl> wrote: >> > On Saturday, November 03, 2012 09:59:28 PM Rafael J. Wysocki wrote: >> >> On Saturday, November 03, 2012 10:13:10 PM Mika Westerberg wrote: >> >> > On Sat, Nov 03, 2012 at 01:42:02PM -0600, Bjorn Helgaas wrote: >> >> > > On Sat, Nov 3, 2012 at 1:46 AM, Mika Westerberg >> >> > > <mika.westerberg@linux.intel.com> 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. >> > [...] >> >> > And if the ACPI core parses the _CRS, how does it pass all the resources to >> >> > the drivers? >> >> >> >> Pretty much the same way the $subject patch does. >> >> >> >> Instead of parsing the entire subtree below an SPI controller and trying >> >> acpi_spi_add_device() for each device node in there, it could call >> >> acpi_spi_add_device() whenever it finds a device of type >> >> ACPI_RESOURCE_TYPE_SERIAL_BUS/ACPI_RESOURCE_SERIAL_TYPE_SPI. >> >> The only problem is how to pass "master" to it. >> >> >> >> So Bjorn, do you have any idea how we could pass the "master" pointer from the >> >> ACPI core to acpi_spi_add_device() in a sensible way? >> >> >> >> An alternative might be to store the information obtained from _CRS in >> >> struct acpi_device objects created by the ACPI core while parsing the >> >> namespace. We do that already for things like _PRW, so we might as well do it >> >> for _CRS. Then, the SPI core could just walk the subtree of the device hierarchy >> >> below the SPI controller's acpi_device to extract that information. >> >> Maybe that's the way to go? >> > >> > The general idea is to move the _CRS parsing routine from acpi_platform.c >> > to scan.c and make it attach resource objects to struct acpi_device. >> > >> > I'm thinking about adding a list head to struct acpi_device pointing to a >> > list of entries like: >> > >> > struct resource_list_entry { >> > struct list_head node; >> > struct resource *resources; >> > unsigned int count; >> > }; >> > >> > where "resources" is an array of resources (e.g. interrupts) in the given >> > entry and count is the size of that array. >> > >> > That list would contain common resources like ACPI_RESOURCE_TYPE_FIXED_MEMORY32, >> > ACPI_RESOURCE_TYPE_IRQ, ACPI_RESOURCE_TYPE_ADDRESS32, ACPI_RESOURCE_TYPE_EXTENDED_IRQ. >> >> This is exactly what PNPACPI already does. For every Device node in >> the namespace, pnpacpi/rsparser.c parses _CRS and builds a list of >> struct resources that correspond to it. If you put this functionality >> in acpi/scan.c, should we get rid of PNPACPI? > > Quite likely. At least this part of it, if you want the core to parse > resources. > > That said, I actually tried to abstract out resource parsing in a more generic > fashion on the basis of our new platform device support code, but quite frankly > I wasn't able to. > > The problem is that struct resource is too simple to be useful for representing > all of the information that can be encoded in ACPI resources. As a result, some > information have to be stored directly in things like struct pnp_dev, struct > platform_device etc. and if we want to represent it generically, the only way > to do that seems to be using the ACPICA resource types as defined in acrestyp.h. Really? I didn't have that impression. It might be that the new GPIO and SERIAL_BUS stuff makes it too complicated for a struct resource, but prior to that, I don't think it was. It's true that there are many different formats (IO, FIXED_IO, MEMORY24, MEMORY32, etc.), but only the core needs to be aware of the encoding of all those formats. As far as a *driver* is concerned, there are only IRQ, DMA, IO, and MEM resources, and those fit easily in a struct resource. I want to expand on what I said before about _CRS being the domain of the core, not drivers. The core must evaluate _CRS to know where devices are and avoid conflicts when assigning resources. The core must evaluate _PRS and _SRS when making resource assignments. It doesn't make sense for drivers to be using _PRS and _SRS; they don't have a global view of resources, and any assignment they make would likely cause conflicts with other devices. I think it makes sense to consolidate all _CRS, _PRS, and _SRS management in the core rather than splitting it up. This is exactly analogous to PCI BAR management, and we don't intend drivers to read and write BARs directly. Bjorn -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Nov 6, 2012 at 6:16 AM, Rafael J. Wysocki <rjw@sisk.pl> wrote: > On Monday, November 05, 2012 09:54:37 AM Bjorn Helgaas wrote: >> On Sat, Nov 3, 2012 at 2:39 PM, Rafael J. Wysocki <rjw@sisk.pl> wrote: >> > On Saturday, November 03, 2012 01:42:02 PM Bjorn Helgaas wrote: >> >> On Sat, Nov 3, 2012 at 1:46 AM, Mika Westerberg >> >> <mika.westerberg@linux.intel.com> 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> >> >> > Acked-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> >> >> > --- >> >> > drivers/spi/spi.c | 231 ++++++++++++++++++++++++++++++++++++++++++++++++++++- >> >> >> > +static acpi_status acpi_spi_add_resources(struct acpi_resource *res, void *data) >> >> > +{ >> >> > + struct acpi_spi_device_info *info = data; >> >> > + struct acpi_resource_spi_serialbus *sb; >> >> > + struct spi_device *spi = info->spi; >> >> > + >> >> > + switch (res->type) { >> >> > + case ACPI_RESOURCE_TYPE_SERIAL_BUS: >> >> > + sb = &res->data.spi_serial_bus; >> >> > + if (sb->type == ACPI_RESOURCE_SERIAL_TYPE_SPI) { >> >> > + spi->chip_select = sb->device_selection; >> >> > + spi->max_speed_hz = sb->connection_speed; >> >> > + >> >> > + /* Mode (clock phase/polarity/etc. */ >> >> > + 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; >> >> > + >> >> > + /* >> >> > + * The info is valid once we have found the >> >> > + * SPISerialBus resource. >> >> > + */ >> >> > + info->valid = true; >> >> > + } >> >> > + break; >> >> > + >> >> > + case ACPI_RESOURCE_TYPE_IRQ: >> >> > + info->gsi = res->data.irq.interrupts[0]; >> >> > + info->triggering = res->data.irq.triggering; >> >> > + info->polarity = res->data.irq.polarity; >> >> > + break; >> >> > + >> >> > + case ACPI_RESOURCE_TYPE_EXTENDED_IRQ: >> >> > + info->gsi = res->data.extended_irq.interrupts[0]; >> >> > + info->triggering = res->data.extended_irq.triggering; >> >> > + info->polarity = res->data.extended_irq.polarity; >> >> >> >> A driver doesn't seem like the right place for _CRS parsing code. >> > >> > This is not a driver, however. :-) >> >> OK. Can you describe what this is? I assume the picture involves an >> SPI master device and one or more SPI slave devices, and the ACPI >> namespace tells us something about them, and somehow this code is >> related to those devices because it's looking at _CRS for them. > > This is part of the SPI core that looks for SPI devices below a controller. > >> A _CRS method is associated with a Device in the namespace. What is >> that device in this case? > > An SPI device below a controller. > >> What is its PNP ID? > > I can't answer that from the top of my head, sorry. > >> What driver claims devices with that ID? > > The driver that declares support for it in its acpi_match_table[] array. OK, I guess my problem is in understanding the difference between (1) a platform device where we use the acpi_match_table[] to locate devices with matching ACPI IDs, and (2) a device where we use pnp_register_driver() or acpi_bus_register_driver() to locate devices with matching ACPI IDs. They seem similar to me. For example, take a PNP0A03 PCI host bridge. This seems like a platform device since there's no native hardware method to enumerate it. The ACPI namespace gives us a way to find it. We use acpi_bus_register_driver() to bind a driver to it. The driver has the struct acpi_device * and can access any ACPI state it needs. The driver supplies .add() and .remove() methods, so in principle the driver doesn't need to know anything about hotplug (assuming the ACPI core handles hotplug correctly, which it doesn't yet). How is the SPI controller different than this? Is there some logical difference that requires a different framework? Or are you proposing that we get rid of acpi_bus_register_driver() and migrate everything to this new framework? Bjorn -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tuesday, November 06, 2012 01:53:01 PM Bjorn Helgaas wrote: > On Tue, Nov 6, 2012 at 6:16 AM, Rafael J. Wysocki <rjw@sisk.pl> wrote: > > On Monday, November 05, 2012 09:54:37 AM Bjorn Helgaas wrote: > >> On Sat, Nov 3, 2012 at 2:39 PM, Rafael J. Wysocki <rjw@sisk.pl> wrote: > >> > On Saturday, November 03, 2012 01:42:02 PM Bjorn Helgaas wrote: > >> >> On Sat, Nov 3, 2012 at 1:46 AM, Mika Westerberg > >> >> <mika.westerberg@linux.intel.com> 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> > >> >> > Acked-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > >> >> > --- > >> >> > drivers/spi/spi.c | 231 ++++++++++++++++++++++++++++++++++++++++++++++++++++- > >> > >> >> > +static acpi_status acpi_spi_add_resources(struct acpi_resource *res, void *data) > >> >> > +{ > >> >> > + struct acpi_spi_device_info *info = data; > >> >> > + struct acpi_resource_spi_serialbus *sb; > >> >> > + struct spi_device *spi = info->spi; > >> >> > + > >> >> > + switch (res->type) { > >> >> > + case ACPI_RESOURCE_TYPE_SERIAL_BUS: > >> >> > + sb = &res->data.spi_serial_bus; > >> >> > + if (sb->type == ACPI_RESOURCE_SERIAL_TYPE_SPI) { > >> >> > + spi->chip_select = sb->device_selection; > >> >> > + spi->max_speed_hz = sb->connection_speed; > >> >> > + > >> >> > + /* Mode (clock phase/polarity/etc. */ > >> >> > + 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; > >> >> > + > >> >> > + /* > >> >> > + * The info is valid once we have found the > >> >> > + * SPISerialBus resource. > >> >> > + */ > >> >> > + info->valid = true; > >> >> > + } > >> >> > + break; > >> >> > + > >> >> > + case ACPI_RESOURCE_TYPE_IRQ: > >> >> > + info->gsi = res->data.irq.interrupts[0]; > >> >> > + info->triggering = res->data.irq.triggering; > >> >> > + info->polarity = res->data.irq.polarity; > >> >> > + break; > >> >> > + > >> >> > + case ACPI_RESOURCE_TYPE_EXTENDED_IRQ: > >> >> > + info->gsi = res->data.extended_irq.interrupts[0]; > >> >> > + info->triggering = res->data.extended_irq.triggering; > >> >> > + info->polarity = res->data.extended_irq.polarity; > >> >> > >> >> A driver doesn't seem like the right place for _CRS parsing code. > >> > > >> > This is not a driver, however. :-) > >> > >> OK. Can you describe what this is? I assume the picture involves an > >> SPI master device and one or more SPI slave devices, and the ACPI > >> namespace tells us something about them, and somehow this code is > >> related to those devices because it's looking at _CRS for them. > > > > This is part of the SPI core that looks for SPI devices below a controller. > > > >> A _CRS method is associated with a Device in the namespace. What is > >> that device in this case? > > > > An SPI device below a controller. > > > >> What is its PNP ID? > > > > I can't answer that from the top of my head, sorry. > > > >> What driver claims devices with that ID? > > > > The driver that declares support for it in its acpi_match_table[] array. > > OK, I guess my problem is in understanding the difference between (1) > a platform device where we use the acpi_match_table[] to locate > devices with matching ACPI IDs, and (2) a device where we use > pnp_register_driver() or acpi_bus_register_driver() to locate devices > with matching ACPI IDs. > > They seem similar to me. For example, take a PNP0A03 PCI host bridge. > This seems like a platform device since there's no native hardware > method to enumerate it. The ACPI namespace gives us a way to find it. > We use acpi_bus_register_driver() to bind a driver to it. The driver > has the struct acpi_device * and can access any ACPI state it needs. > The driver supplies .add() and .remove() methods, so in principle the > driver doesn't need to know anything about hotplug (assuming the ACPI > core handles hotplug correctly, which it doesn't yet). Well, I hope this is the last time I need to explain that. :-) Please see this message for detailed discussion: http://marc.info/?l=linux-kernel&m=135180467616074&w=4 > How is the SPI controller different than this? Is there some logical > difference that requires a different framework? Or are you proposing > that we get rid of acpi_bus_register_driver() and migrate everything > to this new framework? Yes, I do, but let's just do it gradually. Thanks, Rafael
On Tuesday, November 06, 2012 01:35:56 PM Bjorn Helgaas wrote: > On Tue, Nov 6, 2012 at 6:43 AM, Rafael J. Wysocki <rjw@sisk.pl> wrote: > > On Monday, November 05, 2012 09:54:42 AM Bjorn Helgaas wrote: > >> On Mon, Nov 5, 2012 at 3:31 AM, Rafael J. Wysocki <rjw@sisk.pl> wrote: > >> > On Saturday, November 03, 2012 09:59:28 PM Rafael J. Wysocki wrote: > >> >> On Saturday, November 03, 2012 10:13:10 PM Mika Westerberg wrote: > >> >> > On Sat, Nov 03, 2012 at 01:42:02PM -0600, Bjorn Helgaas wrote: > >> >> > > On Sat, Nov 3, 2012 at 1:46 AM, Mika Westerberg > >> >> > > <mika.westerberg@linux.intel.com> 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. > >> > [...] > >> >> > And if the ACPI core parses the _CRS, how does it pass all the resources to > >> >> > the drivers? > >> >> > >> >> Pretty much the same way the $subject patch does. > >> >> > >> >> Instead of parsing the entire subtree below an SPI controller and trying > >> >> acpi_spi_add_device() for each device node in there, it could call > >> >> acpi_spi_add_device() whenever it finds a device of type > >> >> ACPI_RESOURCE_TYPE_SERIAL_BUS/ACPI_RESOURCE_SERIAL_TYPE_SPI. > >> >> The only problem is how to pass "master" to it. > >> >> > >> >> So Bjorn, do you have any idea how we could pass the "master" pointer from the > >> >> ACPI core to acpi_spi_add_device() in a sensible way? > >> >> > >> >> An alternative might be to store the information obtained from _CRS in > >> >> struct acpi_device objects created by the ACPI core while parsing the > >> >> namespace. We do that already for things like _PRW, so we might as well do it > >> >> for _CRS. Then, the SPI core could just walk the subtree of the device hierarchy > >> >> below the SPI controller's acpi_device to extract that information. > >> >> Maybe that's the way to go? > >> > > >> > The general idea is to move the _CRS parsing routine from acpi_platform.c > >> > to scan.c and make it attach resource objects to struct acpi_device. > >> > > >> > I'm thinking about adding a list head to struct acpi_device pointing to a > >> > list of entries like: > >> > > >> > struct resource_list_entry { > >> > struct list_head node; > >> > struct resource *resources; > >> > unsigned int count; > >> > }; > >> > > >> > where "resources" is an array of resources (e.g. interrupts) in the given > >> > entry and count is the size of that array. > >> > > >> > That list would contain common resources like ACPI_RESOURCE_TYPE_FIXED_MEMORY32, > >> > ACPI_RESOURCE_TYPE_IRQ, ACPI_RESOURCE_TYPE_ADDRESS32, ACPI_RESOURCE_TYPE_EXTENDED_IRQ. > >> > >> This is exactly what PNPACPI already does. For every Device node in > >> the namespace, pnpacpi/rsparser.c parses _CRS and builds a list of > >> struct resources that correspond to it. If you put this functionality > >> in acpi/scan.c, should we get rid of PNPACPI? > > > > Quite likely. At least this part of it, if you want the core to parse > > resources. > > > > That said, I actually tried to abstract out resource parsing in a more generic > > fashion on the basis of our new platform device support code, but quite frankly > > I wasn't able to. > > > > The problem is that struct resource is too simple to be useful for representing > > all of the information that can be encoded in ACPI resources. As a result, some > > information have to be stored directly in things like struct pnp_dev, struct > > platform_device etc. and if we want to represent it generically, the only way > > to do that seems to be using the ACPICA resource types as defined in acrestyp.h. > > Really? I didn't have that impression. It might be that the new GPIO > and SERIAL_BUS stuff makes it too complicated for a struct resource, > but prior to that, I don't think it was. It's true that there are > many different formats (IO, FIXED_IO, MEMORY24, MEMORY32, etc.), but > only the core needs to be aware of the encoding of all those formats. > As far as a *driver* is concerned, there are only IRQ, DMA, IO, and > MEM resources, and those fit easily in a struct resource. However, for example expanding ACPI_RESOURCE_TYPE_EXTENDED_IRQ into an array of struct resource objects before anyone actually needs it seems a bit wasteful to me. Let alone registering GSI for the interrupts while we're parsing those resources. > I want to expand on what I said before about _CRS being the domain of > the core, not drivers. Well, I see a difference between _evaluating_ _CRS and _parsing_ its output. In particular, I don't see a reason to do those two things in one operation. In fact, I see reasons to do otherwise. :-) > The core must evaluate _CRS to know where > devices are and avoid conflicts when assigning resources. The core > must evaluate _PRS and _SRS when making resource assignments. It > doesn't make sense for drivers to be using _PRS and _SRS; they don't > have a global view of resources, and any assignment they make would > likely cause conflicts with other devices. I think it makes sense to > consolidate all _CRS, _PRS, and _SRS management in the core rather > than splitting it up. This is exactly analogous to PCI BAR > management, and we don't intend drivers to read and write BARs > directly. OK, but then we need to pass the information obtained from _CRS (presumably after some adjustments through _SRS) to drivers, or rather to things like the SPI core, I2C core etc. so that they can create device objects for drivers to bind to and quite frankly I don't see why not to use ACPI resources for that. Thanks, Rafael
On Tuesday, November 06, 2012 11:28:37 PM Rafael J. Wysocki wrote: > On Tuesday, November 06, 2012 01:35:56 PM Bjorn Helgaas wrote: > > On Tue, Nov 6, 2012 at 6:43 AM, Rafael J. Wysocki <rjw@sisk.pl> wrote: > > > On Monday, November 05, 2012 09:54:42 AM Bjorn Helgaas wrote: > > >> On Mon, Nov 5, 2012 at 3:31 AM, Rafael J. Wysocki <rjw@sisk.pl> wrote: > > >> > On Saturday, November 03, 2012 09:59:28 PM Rafael J. Wysocki wrote: > > >> >> On Saturday, November 03, 2012 10:13:10 PM Mika Westerberg wrote: > > >> >> > On Sat, Nov 03, 2012 at 01:42:02PM -0600, Bjorn Helgaas wrote: > > >> >> > > On Sat, Nov 3, 2012 at 1:46 AM, Mika Westerberg > > >> >> > > <mika.westerberg@linux.intel.com> 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. > > >> > [...] > > >> >> > And if the ACPI core parses the _CRS, how does it pass all the resources to > > >> >> > the drivers? > > >> >> > > >> >> Pretty much the same way the $subject patch does. > > >> >> > > >> >> Instead of parsing the entire subtree below an SPI controller and trying > > >> >> acpi_spi_add_device() for each device node in there, it could call > > >> >> acpi_spi_add_device() whenever it finds a device of type > > >> >> ACPI_RESOURCE_TYPE_SERIAL_BUS/ACPI_RESOURCE_SERIAL_TYPE_SPI. > > >> >> The only problem is how to pass "master" to it. > > >> >> > > >> >> So Bjorn, do you have any idea how we could pass the "master" pointer from the > > >> >> ACPI core to acpi_spi_add_device() in a sensible way? > > >> >> > > >> >> An alternative might be to store the information obtained from _CRS in > > >> >> struct acpi_device objects created by the ACPI core while parsing the > > >> >> namespace. We do that already for things like _PRW, so we might as well do it > > >> >> for _CRS. Then, the SPI core could just walk the subtree of the device hierarchy > > >> >> below the SPI controller's acpi_device to extract that information. > > >> >> Maybe that's the way to go? > > >> > > > >> > The general idea is to move the _CRS parsing routine from acpi_platform.c > > >> > to scan.c and make it attach resource objects to struct acpi_device. > > >> > > > >> > I'm thinking about adding a list head to struct acpi_device pointing to a > > >> > list of entries like: > > >> > > > >> > struct resource_list_entry { > > >> > struct list_head node; > > >> > struct resource *resources; > > >> > unsigned int count; > > >> > }; > > >> > > > >> > where "resources" is an array of resources (e.g. interrupts) in the given > > >> > entry and count is the size of that array. > > >> > > > >> > That list would contain common resources like ACPI_RESOURCE_TYPE_FIXED_MEMORY32, > > >> > ACPI_RESOURCE_TYPE_IRQ, ACPI_RESOURCE_TYPE_ADDRESS32, ACPI_RESOURCE_TYPE_EXTENDED_IRQ. > > >> > > >> This is exactly what PNPACPI already does. For every Device node in > > >> the namespace, pnpacpi/rsparser.c parses _CRS and builds a list of > > >> struct resources that correspond to it. If you put this functionality > > >> in acpi/scan.c, should we get rid of PNPACPI? > > > > > > Quite likely. At least this part of it, if you want the core to parse > > > resources. > > > > > > That said, I actually tried to abstract out resource parsing in a more generic > > > fashion on the basis of our new platform device support code, but quite frankly > > > I wasn't able to. > > > > > > The problem is that struct resource is too simple to be useful for representing > > > all of the information that can be encoded in ACPI resources. As a result, some > > > information have to be stored directly in things like struct pnp_dev, struct > > > platform_device etc. and if we want to represent it generically, the only way > > > to do that seems to be using the ACPICA resource types as defined in acrestyp.h. > > > > Really? I didn't have that impression. It might be that the new GPIO > > and SERIAL_BUS stuff makes it too complicated for a struct resource, > > but prior to that, I don't think it was. It's true that there are > > many different formats (IO, FIXED_IO, MEMORY24, MEMORY32, etc.), but > > only the core needs to be aware of the encoding of all those formats. > > As far as a *driver* is concerned, there are only IRQ, DMA, IO, and > > MEM resources, and those fit easily in a struct resource. > > However, for example expanding ACPI_RESOURCE_TYPE_EXTENDED_IRQ into an > array of struct resource objects before anyone actually needs it seems a > bit wasteful to me. Let alone registering GSI for the interrupts while > we're parsing those resources. > > > I want to expand on what I said before about _CRS being the domain of > > the core, not drivers. > > Well, I see a difference between _evaluating_ _CRS and _parsing_ its > output. In particular, I don't see a reason to do those two things in > one operation. In fact, I see reasons to do otherwise. :-) > > > The core must evaluate _CRS to know where > > devices are and avoid conflicts when assigning resources. The core > > must evaluate _PRS and _SRS when making resource assignments. It > > doesn't make sense for drivers to be using _PRS and _SRS; they don't > > have a global view of resources, and any assignment they make would > > likely cause conflicts with other devices. I think it makes sense to > > consolidate all _CRS, _PRS, and _SRS management in the core rather > > than splitting it up. This is exactly analogous to PCI BAR > > management, and we don't intend drivers to read and write BARs > > directly. > > OK, but then we need to pass the information obtained from _CRS > (presumably after some adjustments through _SRS) to drivers, or rather to > things like the SPI core, I2C core etc. so that they can create device > objects for drivers to bind to and quite frankly I don't see why not to use > ACPI resources for that. Nevertheless, the routines for parsing those resources should belong to the ACPI core, mostly to avoid code duplication. Thanks, Rafael
On Tue, Nov 06, 2012 at 11:18:11PM +0100, Rafael J. Wysocki wrote: > > How is the SPI controller different than this? Is there some logical > > difference that requires a different framework? Or are you proposing > > that we get rid of acpi_bus_register_driver() and migrate everything > > to this new framework? > > Yes, I do, but let's just do it gradually. Bjorn, here is a concrete example how this is supposed to be used. Lets say we have an existing SPI slave driver that we want to extend to support enumeration from ACPI. Instead of writing acpi_driver glue for that (and registering it using acpi_bus_register_driver()) what we do is simple add these to the existing SPI driver: #ifdef CONFIG_ACPI static struct acpi_device_id my_spidrv_match[] = { /* ACPI IDs here */ { } }; MODULE_DEVICE_TABLE(acpi, my_spidrv_match); #endif static struct spi_driver my_spidrv = { ... .driver = { .acpi_match_table = ACPI_PTR(my_spidrv_match), }, }; The same thing works with platform, I2c and SPI drivers and can be extented to others as well. If the driver needs to do some ACPI specific configuration it can get the ACPI handle using its dev->acpi_handle. The above example now supports both, normal SPI (where the devices are enumerated by populating spi_board_info()) and ACPI. Adding support for Device Tree is similar than ACPI so a single driver can support all three easily at the same time. -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Nov 06, 2012 at 11:36:08PM +0100, Rafael J. Wysocki wrote: > > > > OK, but then we need to pass the information obtained from _CRS > > (presumably after some adjustments through _SRS) to drivers, or rather to > > things like the SPI core, I2C core etc. so that they can create device > > objects for drivers to bind to and quite frankly I don't see why not to use > > ACPI resources for that. > > Nevertheless, the routines for parsing those resources should belong > to the ACPI core, mostly to avoid code duplication. Rafael, So is the idea now that the ACPI core parses the resources and passes them forward via struct acpi_device? I'm just wondering how to proceed with these I2C and SPI enumeration patches. Thanks. -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wednesday, November 07, 2012 11:58:42 AM Mika Westerberg wrote: > On Tue, Nov 06, 2012 at 11:36:08PM +0100, Rafael J. Wysocki wrote: > > > > > > OK, but then we need to pass the information obtained from _CRS > > > (presumably after some adjustments through _SRS) to drivers, or rather to > > > things like the SPI core, I2C core etc. so that they can create device > > > objects for drivers to bind to and quite frankly I don't see why not to use > > > ACPI resources for that. > > > > Nevertheless, the routines for parsing those resources should belong > > to the ACPI core, mostly to avoid code duplication. > > Rafael, > > So is the idea now that the ACPI core parses the resources and passes them > forward via struct acpi_device? I'm just wondering how to proceed with > these I2C and SPI enumeration patches. Well, we definitely don't want to duplicate what drivers/pnp/pnpacpi/rsparser.c does, so the idea is to move the code from there to the core in such a way that both the SPI/I2C patches and the PNP layer can use it. I'll have some prototype code ready shortly, hopefully, and I'll post it in that form for comments (and so that you know what to expect). Thanks, Rafael
On Wed, Nov 07, 2012 at 12:14:31PM +0100, Rafael J. Wysocki wrote: > > So is the idea now that the ACPI core parses the resources and passes them > > forward via struct acpi_device? I'm just wondering how to proceed with > > these I2C and SPI enumeration patches. > > Well, we definitely don't want to duplicate what drivers/pnp/pnpacpi/rsparser.c > does, so the idea is to move the code from there to the core in such a way that > both the SPI/I2C patches and the PNP layer can use it. Ok. > I'll have some prototype code ready shortly, hopefully, and I'll post it > in that form for comments (and so that you know what to expect). Sounds good. Thanks! -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Nov 7, 2012 at 9:58 AM, Mika Westerberg <mika.westerberg@linux.intel.com> wrote: > On Tue, Nov 06, 2012 at 11:36:08PM +0100, Rafael J. Wysocki wrote: >> > >> > OK, but then we need to pass the information obtained from _CRS >> > (presumably after some adjustments through _SRS) to drivers, or rather to >> > things like the SPI core, I2C core etc. so that they can create device >> > objects for drivers to bind to and quite frankly I don't see why not to use >> > ACPI resources for that. >> >> Nevertheless, the routines for parsing those resources should belong >> to the ACPI core, mostly to avoid code duplication. > > Rafael, > > So is the idea now that the ACPI core parses the resources and passes them > forward via struct acpi_device? I'm just wondering how to proceed with > these I2C and SPI enumeration patches. From my experience with device tree, that seems the wrong way around. Device Tree used to have a separate "of_device" which is analogous to an acpi_device. The problem was always that of_devices never fit into the view that Linux has of the system. That would mean having both an of_device and and spi_device in completely separate parts of the driver model tree to support an spi device. Same for platform, i2c and onewire and others. Blech. So, yes I agree that ACPI core should have the tools for parsing the resources, but it makes sense for those functions to be helpers that the spi core acpi support and the i2c core acpi support use to populate the native spi_device and i2c_client structures. Plus individual drivers can call the same functions if (and only if) the needed resources cannot fit into the bus type's native format. We really could also use more common code between bus types for storing various kinds of resources, but that's a separate issue and doesn't affect this discussion. g. -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sat, Nov 3, 2012 at 7:46 AM, Mika Westerberg <mika.westerberg@linux.intel.com> 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> > Acked-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > --- > drivers/spi/spi.c | 231 ++++++++++++++++++++++++++++++++++++++++++++++++++++- > 1 file changed, 230 insertions(+), 1 deletion(-) > > diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c > index 84c2861..de22a6e 100644 > --- a/drivers/spi/spi.c > +++ b/drivers/spi/spi.c > @@ -35,6 +35,7 @@ > #include <linux/sched.h> > #include <linux/delay.h> > #include <linux/kthread.h> > +#include <linux/acpi.h> > > static void spidev_release(struct device *dev) > { > @@ -93,6 +94,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 +893,227 @@ static void of_register_spi_devices(struct spi_master *master) > static void of_register_spi_devices(struct spi_master *master) { } > #endif > > +#ifdef CONFIG_ACPI > +struct acpi_spi { > + acpi_status (*callback)(struct acpi_device *, void *); > + void *data; > +}; > + > +static acpi_status acpi_spi_enumerate_device(acpi_handle handle, u32 level, > + void *data, void **return_value) > +{ > + struct acpi_spi *acpi_spi = data; > + struct acpi_device *adev; > + > + if (acpi_bus_get_device(handle, &adev)) > + return AE_OK; > + if (acpi_bus_get_status(adev) || !adev->status.present) > + return AE_OK; > + > + return acpi_spi->callback(adev, acpi_spi->data); > +} > + > +static acpi_status acpi_spi_enumerate(acpi_handle handle, > + acpi_status (*callback)(struct acpi_device *, void *), void *data) > +{ > + struct acpi_spi acpi_spi; > + > + acpi_spi.callback = callback; > + acpi_spi.data = data; > + > + return acpi_walk_namespace(ACPI_TYPE_DEVICE, handle, 1, > + acpi_spi_enumerate_device, NULL, > + &acpi_spi, NULL); > +} From my reading of this, the block causes 2 levels of callback indirection. First to either acpi_spi_find_child or acpi_spi_add_device and second to acpi_spi_enumerate_device. All to share about 4 lines of code in acpi_spi_enumerate_device. It took me a while to unravel it. I think acpi_spi_find_child and acpi_spi_add_device should be passed directly to acpi_walk_namespace. Is there anything that prevents that? I also agree with the discussion that the actual parsing code for the resources should be common,. Retrieving things like IRQs and address resources should be function calls into ACPI helpers instead of open coding it in the spi core code. Otherwise the patch looks sane to me. g. -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Nov 7, 2012 at 2:56 AM, Mika Westerberg <mika.westerberg@linux.intel.com> wrote: > On Tue, Nov 06, 2012 at 11:18:11PM +0100, Rafael J. Wysocki wrote: >> > How is the SPI controller different than this? Is there some logical >> > difference that requires a different framework? Or are you proposing >> > that we get rid of acpi_bus_register_driver() and migrate everything >> > to this new framework? >> >> Yes, I do, but let's just do it gradually. > > Bjorn, here is a concrete example how this is supposed to be used. > > Lets say we have an existing SPI slave driver that we want to extend to > support enumeration from ACPI. Instead of writing acpi_driver glue for that > (and registering it using acpi_bus_register_driver()) what we do is simple > add these to the existing SPI driver: > > #ifdef CONFIG_ACPI > static struct acpi_device_id my_spidrv_match[] = { > /* ACPI IDs here */ > { } > }; > MODULE_DEVICE_TABLE(acpi, my_spidrv_match); > #endif > > static struct spi_driver my_spidrv = { > ... > .driver = { > .acpi_match_table = ACPI_PTR(my_spidrv_match), > }, > }; > > The same thing works with platform, I2c and SPI drivers and can be extented > to others as well. If the driver needs to do some ACPI specific > configuration it can get the ACPI handle using its dev->acpi_handle. > > The above example now supports both, normal SPI (where the devices are > enumerated by populating spi_board_info()) and ACPI. Adding support for > Device Tree is similar than ACPI so a single driver can support all three > easily at the same time. Thanks for the concrete example; that helps me a lot. Struct device_driver is a generic structure, so it seems strange to have to include non-generic things like of_device_id and now acpi_match_table there. I'm actually interested in the details you didn't include above, too. For example, I don't know of a generic way to get resource information from a "struct device *", so I assume you need to figure out what sort of device it is and then do the appropriate PCI/ACPI/OF/DT/etc operations to learn the resources? I think it would be cool if there *were* a generic way to get "struct device" resources. Then you could imagine a mechanism where a driver supplied a list of identifiers it could claim, e.g., PCI_VEN_DEV(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_CE4100_UART), ACPI_ID("PNP0501"), etc., and it might not need to know anything more than what the identifier is. -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Nov 08, 2012 at 12:32:25PM -0700, Bjorn Helgaas wrote: > Struct device_driver is a generic structure, so it seems strange to > have to include non-generic things like of_device_id and now > acpi_match_table there. Yes, but in a sense the DT and ACPI are "generic". So that they are used to describe the configuration of a machine. > I'm actually interested in the details you didn't include above, too. > For example, I don't know of a generic way to get resource information > from a "struct device *", so I assume you need to figure out what sort > of device it is and then do the appropriate PCI/ACPI/OF/DT/etc > operations to learn the resources? Right. Typically you check, dev->of_node (or dev->acpi_handle) in a driver and if it is non-NULL you can extract the resources using DT or ACPI specific API calls. Some things like IRQs and MMIO addresses can be passed to the driver using the struct resource (and we do that already) but others we can't, like GPIOs and some DT specific properties. Also with ACPI there might be need to call some ACPI method, like _DSM where we need to have access to the ACPI handle. > I think it would be cool if there *were* a generic way to get "struct > device" resources. Then you could imagine a mechanism where a driver > supplied a list of identifiers it could claim, e.g., > PCI_VEN_DEV(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_CE4100_UART), > ACPI_ID("PNP0501"), etc., and it might not need to know anything more > than what the identifier is. Indeed that would be cool, and we should probably try to implement something like that, eventually. If you have followed the discusion there is already talks about having a single API to get GPIOs to the driver in a generic way. -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thursday, November 08, 2012 06:05:23 PM Grant Likely wrote: > On Wed, Nov 7, 2012 at 9:58 AM, Mika Westerberg > <mika.westerberg@linux.intel.com> wrote: > > On Tue, Nov 06, 2012 at 11:36:08PM +0100, Rafael J. Wysocki wrote: > >> > > >> > OK, but then we need to pass the information obtained from _CRS > >> > (presumably after some adjustments through _SRS) to drivers, or rather to > >> > things like the SPI core, I2C core etc. so that they can create device > >> > objects for drivers to bind to and quite frankly I don't see why not to use > >> > ACPI resources for that. > >> > >> Nevertheless, the routines for parsing those resources should belong > >> to the ACPI core, mostly to avoid code duplication. > > > > Rafael, > > > > So is the idea now that the ACPI core parses the resources and passes them > > forward via struct acpi_device? Not exactly. The idea is to execute _CRS in the core and attach the result as a list of resources the struct acpi_device object representing the given device node. > > I'm just wondering how to proceed with these I2C and SPI enumeration patches. > > From my experience with device tree, that seems the wrong way around. > Device Tree used to have a separate "of_device" which is analogous to > an acpi_device. No, it is not. If anything, struct acpi_device is a counterpart of struct device_node. :-) Yes, the name is misleading and it should be something like struct acpi_dev_node. Yes, these objects _are_ registered as devices with the driver model and there are drivers that bind to some of them. Yes, this is a mistake, but fixing it will take quite some time, because it involves converting the drivers in question. No, acpi_handle is not analogous to struct device_node, because it only is a pointer to a structure used internally by the AML interpreter. It only is useful for executing AML methods with the help of the interpreter, but there are reasons why _CRS, in particular, should only be executed by the ACPI core. > The problem was always that of_devices never fit into > the view that Linux has of the system. That would mean having both an > of_device and and spi_device in completely separate parts of the > driver model tree to support an spi device. Same for platform, i2c and > onewire and others. Blech. > > So, yes I agree that ACPI core should have the tools for parsing the > resources, but it makes sense for those functions to be helpers that > the spi core acpi support and the i2c core acpi support use to > populate the native spi_device and i2c_client structures. Yes, that exactly is the plan, although I2C and SPI will not receive the resources directly from _CRS. :-) > Plus individual drivers can call the same functions if (and only if) the > needed resources cannot fit into the bus type's native format. I'm kind of cautious about this particular thing. Thanks, Rafael
On Thu, Nov 8, 2012 at 9:06 PM, Rafael J. Wysocki <rjw@sisk.pl> wrote: > On Thursday, November 08, 2012 06:05:23 PM Grant Likely wrote: >> On Wed, Nov 7, 2012 at 9:58 AM, Mika Westerberg >> <mika.westerberg@linux.intel.com> wrote: >> > On Tue, Nov 06, 2012 at 11:36:08PM +0100, Rafael J. Wysocki wrote: >> >> > >> >> > OK, but then we need to pass the information obtained from _CRS >> >> > (presumably after some adjustments through _SRS) to drivers, or rather to >> >> > things like the SPI core, I2C core etc. so that they can create device >> >> > objects for drivers to bind to and quite frankly I don't see why not to use >> >> > ACPI resources for that. >> >> >> >> Nevertheless, the routines for parsing those resources should belong >> >> to the ACPI core, mostly to avoid code duplication. >> > >> > Rafael, >> > >> > So is the idea now that the ACPI core parses the resources and passes them >> > forward via struct acpi_device? > > Not exactly. The idea is to execute _CRS in the core and attach the result > as a list of resources the struct acpi_device object representing the given > device node. > >> > I'm just wondering how to proceed with these I2C and SPI enumeration patches. >> >> From my experience with device tree, that seems the wrong way around. >> Device Tree used to have a separate "of_device" which is analogous to >> an acpi_device. > > No, it is not. If anything, struct acpi_device is a counterpart of struct > device_node. :-) > > Yes, the name is misleading and it should be something like struct acpi_dev_node. > Yes, these objects _are_ registered as devices with the driver model and there > are drivers that bind to some of them. Yes, this is a mistake, but fixing it > will take quite some time, because it involves converting the drivers in > question. Okay, fair enough. I was indeed thrown off by the fact that it embeds a struct device and there are drivers that bind against it. At least that is the sort of thing that can be fixed over the long haul without undue pain. I can certainly see the advantage of having acpi nodes appear in sysfs. Interestingly enough I'm currently playing with a patch set that makes struct device_node a kobject so it can do the same. It is quite nice to get a back symlink from a struct device to a struct device_node when the pointer is populated. >> Plus individual drivers can call the same functions if (and only if) the >> needed resources cannot fit into the bus type's native format. > > I'm kind of cautious about this particular thing. I've seen enough cases where something doesn't quite fit into the model provided by a subsystem and it requires the driver to go asking for something specific. But I completely agree that caution is absolutely required and it shouldn't be done casually. g. -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Nov 08, 2012 at 06:48:05PM +0000, Grant Likely wrote: > On Sat, Nov 3, 2012 at 7:46 AM, Mika Westerberg > <mika.westerberg@linux.intel.com> 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> > > Acked-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > --- > > drivers/spi/spi.c | 231 ++++++++++++++++++++++++++++++++++++++++++++++++++++- > > 1 file changed, 230 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c > > index 84c2861..de22a6e 100644 > > --- a/drivers/spi/spi.c > > +++ b/drivers/spi/spi.c > > @@ -35,6 +35,7 @@ > > #include <linux/sched.h> > > #include <linux/delay.h> > > #include <linux/kthread.h> > > +#include <linux/acpi.h> > > > > static void spidev_release(struct device *dev) > > { > > @@ -93,6 +94,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 +893,227 @@ static void of_register_spi_devices(struct spi_master *master) > > static void of_register_spi_devices(struct spi_master *master) { } > > #endif > > > > +#ifdef CONFIG_ACPI > > +struct acpi_spi { > > + acpi_status (*callback)(struct acpi_device *, void *); > > + void *data; > > +}; > > + > > +static acpi_status acpi_spi_enumerate_device(acpi_handle handle, u32 level, > > + void *data, void **return_value) > > +{ > > + struct acpi_spi *acpi_spi = data; > > + struct acpi_device *adev; > > + > > + if (acpi_bus_get_device(handle, &adev)) > > + return AE_OK; > > + if (acpi_bus_get_status(adev) || !adev->status.present) > > + return AE_OK; > > + > > + return acpi_spi->callback(adev, acpi_spi->data); > > +} > > + > > +static acpi_status acpi_spi_enumerate(acpi_handle handle, > > + acpi_status (*callback)(struct acpi_device *, void *), void *data) > > +{ > > + struct acpi_spi acpi_spi; > > + > > + acpi_spi.callback = callback; > > + acpi_spi.data = data; > > + > > + return acpi_walk_namespace(ACPI_TYPE_DEVICE, handle, 1, > > + acpi_spi_enumerate_device, NULL, > > + &acpi_spi, NULL); > > +} > > >From my reading of this, the block causes 2 levels of callback > indirection. First to either acpi_spi_find_child or > acpi_spi_add_device and second to acpi_spi_enumerate_device. All to > share about 4 lines of code in acpi_spi_enumerate_device. It took me a > while to unravel it. I think acpi_spi_find_child and > acpi_spi_add_device should be passed directly to acpi_walk_namespace. > Is there anything that prevents that? No, I'll fix that up in the next version of the series. > I also agree with the discussion that the actual parsing code for the > resources should be common,. Retrieving things like IRQs and address > resources should be function calls into ACPI helpers instead of open > coding it in the spi core code. We are working on that and I'm hoping the second version will use the resources as provided by the ACPI core instead of calling _CRS directly here. > Otherwise the patch looks sane to me. Thanks. -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[+cc Greg, Peter, Tony since they acked the original patch [1]] On Thu, Nov 8, 2012 at 1:04 PM, Mika Westerberg <mika.westerberg@linux.intel.com> wrote: > On Thu, Nov 08, 2012 at 12:32:25PM -0700, Bjorn Helgaas wrote: >> Struct device_driver is a generic structure, so it seems strange to >> have to include non-generic things like of_device_id and now >> acpi_match_table there. > > Yes, but in a sense the DT and ACPI are "generic". So that they are used to > describe the configuration of a machine. What I meant by "generic" was "useful across all architectures." The new acpi_match_table and acpi_handle fields [1] are not generic in that sense because they're present on all architectures but used only on x86 and ia64. The existing of_match_table and of_node are similarly unused on many architectures. This doesn't seem like a scalable strategy to me. Are we going to add a pnpbios_node for x86 PNPBIOS machines without ACPI, a pdc_hpa for parisc machines with PDC, etc.? [1] https://patchwork.kernel.org/patch/1677221/ -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Nov 9, 2012 at 3:11 PM, Bjorn Helgaas <bhelgaas@google.com> wrote: > [+cc Greg, Peter, Tony since they acked the original patch [1]] > > On Thu, Nov 8, 2012 at 1:04 PM, Mika Westerberg > <mika.westerberg@linux.intel.com> wrote: >> On Thu, Nov 08, 2012 at 12:32:25PM -0700, Bjorn Helgaas wrote: >>> Struct device_driver is a generic structure, so it seems strange to >>> have to include non-generic things like of_device_id and now >>> acpi_match_table there. >> >> Yes, but in a sense the DT and ACPI are "generic". So that they are used to >> describe the configuration of a machine. > > What I meant by "generic" was "useful across all architectures." The > new acpi_match_table and acpi_handle fields [1] are not generic in > that sense because they're present on all architectures but used only > on x86 and ia64. The existing of_match_table and of_node are > similarly unused on many architectures. This doesn't seem like a > scalable strategy to me. Are we going to add a pnpbios_node for x86 > PNPBIOS machines without ACPI, a pdc_hpa for parisc machines with PDC, > etc.? > > [1] https://patchwork.kernel.org/patch/1677221/ Ultimately yes, I think that is what we want to do, but there is first the non-trivial problem to solve of figuring out how ACPI/DT/whatever data maps into what the driver expects. For example, say a device uses two GPIOs (A & B) and we have a generic get_gpio(int index) function that works for both ACPI and DT. But what if the ACPI binding has the gpios in the order A,B and DT orders them B,A? I do want to coordinate between the DT and ACPI camps to avoid those situations as much as possible, but they will happen. When they do the driver will still need firmware specific data. It doesn't make any sense to put that stuff outside the driver because only that specific driver needs the extra information. g. -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Nov 9, 2012 at 8:45 AM, Grant Likely <grant.likely@secretlab.ca> wrote: > On Fri, Nov 9, 2012 at 3:11 PM, Bjorn Helgaas <bhelgaas@google.com> wrote: >> [+cc Greg, Peter, Tony since they acked the original patch [1]] >> >> On Thu, Nov 8, 2012 at 1:04 PM, Mika Westerberg >> <mika.westerberg@linux.intel.com> wrote: >>> On Thu, Nov 08, 2012 at 12:32:25PM -0700, Bjorn Helgaas wrote: >>>> Struct device_driver is a generic structure, so it seems strange to >>>> have to include non-generic things like of_device_id and now >>>> acpi_match_table there. >>> >>> Yes, but in a sense the DT and ACPI are "generic". So that they are used to >>> describe the configuration of a machine. >> >> What I meant by "generic" was "useful across all architectures." The >> new acpi_match_table and acpi_handle fields [1] are not generic in >> that sense because they're present on all architectures but used only >> on x86 and ia64. The existing of_match_table and of_node are >> similarly unused on many architectures. This doesn't seem like a >> scalable strategy to me. Are we going to add a pnpbios_node for x86 >> PNPBIOS machines without ACPI, a pdc_hpa for parisc machines with PDC, >> etc.? >> >> [1] https://patchwork.kernel.org/patch/1677221/ > > Ultimately yes, I think that is what we want to do, Just to be clear, you think we *should* add things like pnpbios_node, pdc_hpa, etc., to struct device, one field for every scheme of telling the OS about non-enumerable devices, where only one of the N fields is used on any given machine? That seems surprising to me, but maybe I just need to be educated :) > but there is first > the non-trivial problem to solve of figuring out how ACPI/DT/whatever > data maps into what the driver expects. For example, say a device uses > two GPIOs (A & B) and we have a generic get_gpio(int index) function > that works for both ACPI and DT. But what if the ACPI binding has the > gpios in the order A,B and DT orders them B,A? I do want to coordinate > between the DT and ACPI camps to avoid those situations as much as > possible, but they will happen. When they do the driver will still > need firmware specific data. It doesn't make any sense to put that > stuff outside the driver because only that specific driver needs the > extra information. Sure. This seems like just a special case of "drivers need a way to access the underlying ACPI/DT/whatever-specific functionality," e.g., gpio = get_gpio(dev, dev_is_acpi(dev) ? 1 : 0); -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Nov 9, 2012 at 4:35 PM, Bjorn Helgaas <bhelgaas@google.com> wrote: > On Fri, Nov 9, 2012 at 8:45 AM, Grant Likely <grant.likely@secretlab.ca> wrote: >> On Fri, Nov 9, 2012 at 3:11 PM, Bjorn Helgaas <bhelgaas@google.com> wrote: >>> [+cc Greg, Peter, Tony since they acked the original patch [1]] >>> >>> On Thu, Nov 8, 2012 at 1:04 PM, Mika Westerberg >>> <mika.westerberg@linux.intel.com> wrote: >>>> On Thu, Nov 08, 2012 at 12:32:25PM -0700, Bjorn Helgaas wrote: >>>>> Struct device_driver is a generic structure, so it seems strange to >>>>> have to include non-generic things like of_device_id and now >>>>> acpi_match_table there. >>>> >>>> Yes, but in a sense the DT and ACPI are "generic". So that they are used to >>>> describe the configuration of a machine. >>> >>> What I meant by "generic" was "useful across all architectures." The >>> new acpi_match_table and acpi_handle fields [1] are not generic in >>> that sense because they're present on all architectures but used only >>> on x86 and ia64. The existing of_match_table and of_node are >>> similarly unused on many architectures. This doesn't seem like a >>> scalable strategy to me. Are we going to add a pnpbios_node for x86 >>> PNPBIOS machines without ACPI, a pdc_hpa for parisc machines with PDC, >>> etc.? >>> >>> [1] https://patchwork.kernel.org/patch/1677221/ >> >> Ultimately yes, I think that is what we want to do, > > Just to be clear, you think we *should* add things like pnpbios_node, > pdc_hpa, etc., to struct device, one field for every scheme of telling > the OS about non-enumerable devices, where only one of the N fields is > used on any given machine? That seems surprising to me, but maybe I > just need to be educated :) Ah, I see what you're asking. In the short term, yes but only because we don't have any other alternative. What I'd really rather have is a safe way to attach datum (ie. acpi_device or device_node) to a struct device and get it back later in a type safe way. It would actually be useful for all manner of things, not just ACPI/DT. I experimented a bit with trying to implement something a year back, but never spent enough time on it. g. -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Nov 09, 2012 at 04:43:27PM +0000, Grant Likely wrote: > In the short term, yes but only because we don't have any other > alternative. What I'd really rather have is a safe way to attach datum > (ie. acpi_device or device_node) to a struct device and get it back > later in a type safe way. It would actually be useful for all manner > of things, not just ACPI/DT. I experimented a bit with trying to > implement something a year back, but never spent enough time on it. devres might already do what you need here if you are OK with the performance (and frees things too for super bonus fun points!). That's how dev_get_regmap() is implemented, it's not awesome in a fast path but it seems OK otherwise.
On Fri, Nov 9, 2012 at 9:43 AM, Grant Likely <grant.likely@secretlab.ca> wrote: > On Fri, Nov 9, 2012 at 4:35 PM, Bjorn Helgaas <bhelgaas@google.com> wrote: >> On Fri, Nov 9, 2012 at 8:45 AM, Grant Likely <grant.likely@secretlab.ca> wrote: >>> On Fri, Nov 9, 2012 at 3:11 PM, Bjorn Helgaas <bhelgaas@google.com> wrote: >>>> [+cc Greg, Peter, Tony since they acked the original patch [1]] >>>> >>>> On Thu, Nov 8, 2012 at 1:04 PM, Mika Westerberg >>>> <mika.westerberg@linux.intel.com> wrote: >>>>> On Thu, Nov 08, 2012 at 12:32:25PM -0700, Bjorn Helgaas wrote: >>>>>> Struct device_driver is a generic structure, so it seems strange to >>>>>> have to include non-generic things like of_device_id and now >>>>>> acpi_match_table there. >>>>> >>>>> Yes, but in a sense the DT and ACPI are "generic". So that they are used to >>>>> describe the configuration of a machine. >>>> >>>> What I meant by "generic" was "useful across all architectures." The >>>> new acpi_match_table and acpi_handle fields [1] are not generic in >>>> that sense because they're present on all architectures but used only >>>> on x86 and ia64. The existing of_match_table and of_node are >>>> similarly unused on many architectures. This doesn't seem like a >>>> scalable strategy to me. Are we going to add a pnpbios_node for x86 >>>> PNPBIOS machines without ACPI, a pdc_hpa for parisc machines with PDC, >>>> etc.? >>>> >>>> [1] https://patchwork.kernel.org/patch/1677221/ >>> >>> Ultimately yes, I think that is what we want to do, >> >> Just to be clear, you think we *should* add things like pnpbios_node, >> pdc_hpa, etc., to struct device, one field for every scheme of telling >> the OS about non-enumerable devices, where only one of the N fields is >> used on any given machine? That seems surprising to me, but maybe I >> just need to be educated :) > > Ah, I see what you're asking. > > In the short term, yes but only because we don't have any other > alternative. What I'd really rather have is a safe way to attach datum > (ie. acpi_device or device_node) to a struct device and get it back > later in a type safe way. Yep, *that* makes perfect sense to me. Something along these lines, maybe: #define dev_is_acpi(d) ((d)->bus == &acpi_bus_type) #define dev_acpi_handle(d) (dev_is_acpi(d) ? (acpi_handle) d->datum : NULL) -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Friday, November 09, 2012 09:53:26 AM Bjorn Helgaas wrote: > On Fri, Nov 9, 2012 at 9:43 AM, Grant Likely <grant.likely@secretlab.ca> wrote: > > On Fri, Nov 9, 2012 at 4:35 PM, Bjorn Helgaas <bhelgaas@google.com> wrote: > >> On Fri, Nov 9, 2012 at 8:45 AM, Grant Likely <grant.likely@secretlab.ca> wrote: > >>> On Fri, Nov 9, 2012 at 3:11 PM, Bjorn Helgaas <bhelgaas@google.com> wrote: > >>>> [+cc Greg, Peter, Tony since they acked the original patch [1]] > >>>> > >>>> On Thu, Nov 8, 2012 at 1:04 PM, Mika Westerberg > >>>> <mika.westerberg@linux.intel.com> wrote: > >>>>> On Thu, Nov 08, 2012 at 12:32:25PM -0700, Bjorn Helgaas wrote: > >>>>>> Struct device_driver is a generic structure, so it seems strange to > >>>>>> have to include non-generic things like of_device_id and now > >>>>>> acpi_match_table there. > >>>>> > >>>>> Yes, but in a sense the DT and ACPI are "generic". So that they are used to > >>>>> describe the configuration of a machine. > >>>> > >>>> What I meant by "generic" was "useful across all architectures." The > >>>> new acpi_match_table and acpi_handle fields [1] are not generic in > >>>> that sense because they're present on all architectures but used only > >>>> on x86 and ia64. The existing of_match_table and of_node are > >>>> similarly unused on many architectures. This doesn't seem like a > >>>> scalable strategy to me. Are we going to add a pnpbios_node for x86 > >>>> PNPBIOS machines without ACPI, a pdc_hpa for parisc machines with PDC, > >>>> etc.? > >>>> > >>>> [1] https://patchwork.kernel.org/patch/1677221/ > >>> > >>> Ultimately yes, I think that is what we want to do, > >> > >> Just to be clear, you think we *should* add things like pnpbios_node, > >> pdc_hpa, etc., to struct device, one field for every scheme of telling > >> the OS about non-enumerable devices, where only one of the N fields is > >> used on any given machine? That seems surprising to me, but maybe I > >> just need to be educated :) > > > > Ah, I see what you're asking. > > > > In the short term, yes but only because we don't have any other > > alternative. What I'd really rather have is a safe way to attach datum > > (ie. acpi_device or device_node) to a struct device and get it back > > later in a type safe way. > > Yep, *that* makes perfect sense to me. Something along these lines, maybe: > > #define dev_is_acpi(d) ((d)->bus == &acpi_bus_type) No, that's not right. It won't work for things like SPI and I2C with a "backing" ACPI device node anyway (and for PCI too, by the way :-)). > #define dev_acpi_handle(d) (dev_is_acpi(d) ? (acpi_handle) > d->datum : NULL) The problem basically is how we can tell that the given struct device has a "backing" object containing device information (e.g. resources) and what that "backing" object is. For device trees that would be a struct device_node and for ACPI that would be an acpi_handle or a struct acpi_device etc. And by the way, they _can_ be used simultaneously, in principle. So we need something like of_node(dev) or acpi_node(dev), but that can't be something following two pointers or calling a function just in order to check if the pointer _is_ _there_ in either case. And since we added of_node to struct device at one point, it is only logical to treat ACPI in the same way. If we come up with a better idea _later_, then we can convert _all_ things to this new idea, whatever it is. Are you seriously expecting us to come up with such an idea on the fly just so that we can use ACPI support, which already is there in the form of archdata.acpi_handle anyway, on equal footing with Device Trees? Rafael
On Sat, Nov 10, 2012 at 11:10 AM, Rafael J. Wysocki <rjw@sisk.pl> wrote: > On Friday, November 09, 2012 09:53:26 AM Bjorn Helgaas wrote: >> On Fri, Nov 9, 2012 at 9:43 AM, Grant Likely <grant.likely@secretlab.ca> wrote: >> > On Fri, Nov 9, 2012 at 4:35 PM, Bjorn Helgaas <bhelgaas@google.com> wrote: >> >> On Fri, Nov 9, 2012 at 8:45 AM, Grant Likely <grant.likely@secretlab.ca> wrote: >> >>> On Fri, Nov 9, 2012 at 3:11 PM, Bjorn Helgaas <bhelgaas@google.com> wrote: >> >>>> [+cc Greg, Peter, Tony since they acked the original patch [1]] >> >>>> >> >>>> On Thu, Nov 8, 2012 at 1:04 PM, Mika Westerberg >> >>>> <mika.westerberg@linux.intel.com> wrote: >> >>>>> On Thu, Nov 08, 2012 at 12:32:25PM -0700, Bjorn Helgaas wrote: >> >>>>>> Struct device_driver is a generic structure, so it seems strange to >> >>>>>> have to include non-generic things like of_device_id and now >> >>>>>> acpi_match_table there. >> >>>>> >> >>>>> Yes, but in a sense the DT and ACPI are "generic". So that they are used to >> >>>>> describe the configuration of a machine. >> >>>> >> >>>> What I meant by "generic" was "useful across all architectures." The >> >>>> new acpi_match_table and acpi_handle fields [1] are not generic in >> >>>> that sense because they're present on all architectures but used only >> >>>> on x86 and ia64. The existing of_match_table and of_node are >> >>>> similarly unused on many architectures. This doesn't seem like a >> >>>> scalable strategy to me. Are we going to add a pnpbios_node for x86 >> >>>> PNPBIOS machines without ACPI, a pdc_hpa for parisc machines with PDC, >> >>>> etc.? >> >>>> >> >>>> [1] https://patchwork.kernel.org/patch/1677221/ >> >>> >> >>> Ultimately yes, I think that is what we want to do, >> >> >> >> Just to be clear, you think we *should* add things like pnpbios_node, >> >> pdc_hpa, etc., to struct device, one field for every scheme of telling >> >> the OS about non-enumerable devices, where only one of the N fields is >> >> used on any given machine? That seems surprising to me, but maybe I >> >> just need to be educated :) >> > >> > Ah, I see what you're asking. >> > >> > In the short term, yes but only because we don't have any other >> > alternative. What I'd really rather have is a safe way to attach datum >> > (ie. acpi_device or device_node) to a struct device and get it back >> > later in a type safe way. >> >> Yep, *that* makes perfect sense to me. Something along these lines, maybe: >> >> #define dev_is_acpi(d) ((d)->bus == &acpi_bus_type) > > No, that's not right. It won't work for things like SPI and I2C with a > "backing" ACPI device node anyway (and for PCI too, by the way :-)). > >> #define dev_acpi_handle(d) (dev_is_acpi(d) ? (acpi_handle) >> d->datum : NULL) > > The problem basically is how we can tell that the given struct device has > a "backing" object containing device information (e.g. resources) and what > that "backing" object is. For device trees that would be a struct device_node > and for ACPI that would be an acpi_handle or a struct acpi_device etc. And by > the way, they _can_ be used simultaneously, in principle. > > So we need something like of_node(dev) or acpi_node(dev), but that can't be > something following two pointers or calling a function just in order to check > if the pointer _is_ _there_ in either case. > > And since we added of_node to struct device at one point, it is only logical to > treat ACPI in the same way. If we come up with a better idea _later_, then we > can convert _all_ things to this new idea, whatever it is. > > Are you seriously expecting us to come up with such an idea on the fly just so > that we can use ACPI support, which already is there in the form of > archdata.acpi_handle anyway, on equal footing with Device Trees? I'm certainly not. I agree with adding it to struct device now and replace it later if someone designs something better. I also agree with using a dev_acpi_node() macro as you described above. I went the opposite way with device tree, and I absolutely regret it. g. -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sat, Nov 10, 2012 at 4:10 AM, Rafael J. Wysocki <rjw@sisk.pl> wrote: > On Friday, November 09, 2012 09:53:26 AM Bjorn Helgaas wrote: >> On Fri, Nov 9, 2012 at 9:43 AM, Grant Likely <grant.likely@secretlab.ca> wrote: >> > On Fri, Nov 9, 2012 at 4:35 PM, Bjorn Helgaas <bhelgaas@google.com> wrote: >> >> On Fri, Nov 9, 2012 at 8:45 AM, Grant Likely <grant.likely@secretlab.ca> wrote: >> >>> On Fri, Nov 9, 2012 at 3:11 PM, Bjorn Helgaas <bhelgaas@google.com> wrote: >> >>>> [+cc Greg, Peter, Tony since they acked the original patch [1]] >> >>>> >> >>>> On Thu, Nov 8, 2012 at 1:04 PM, Mika Westerberg >> >>>> <mika.westerberg@linux.intel.com> wrote: >> >>>>> On Thu, Nov 08, 2012 at 12:32:25PM -0700, Bjorn Helgaas wrote: >> >>>>>> Struct device_driver is a generic structure, so it seems strange to >> >>>>>> have to include non-generic things like of_device_id and now >> >>>>>> acpi_match_table there. >> >>>>> >> >>>>> Yes, but in a sense the DT and ACPI are "generic". So that they are used to >> >>>>> describe the configuration of a machine. >> >>>> >> >>>> What I meant by "generic" was "useful across all architectures." The >> >>>> new acpi_match_table and acpi_handle fields [1] are not generic in >> >>>> that sense because they're present on all architectures but used only >> >>>> on x86 and ia64. The existing of_match_table and of_node are >> >>>> similarly unused on many architectures. This doesn't seem like a >> >>>> scalable strategy to me. Are we going to add a pnpbios_node for x86 >> >>>> PNPBIOS machines without ACPI, a pdc_hpa for parisc machines with PDC, >> >>>> etc.? >> >>>> >> >>>> [1] https://patchwork.kernel.org/patch/1677221/ >> >>> >> >>> Ultimately yes, I think that is what we want to do, >> >> >> >> Just to be clear, you think we *should* add things like pnpbios_node, >> >> pdc_hpa, etc., to struct device, one field for every scheme of telling >> >> the OS about non-enumerable devices, where only one of the N fields is >> >> used on any given machine? That seems surprising to me, but maybe I >> >> just need to be educated :) >> > >> > Ah, I see what you're asking. >> > >> > In the short term, yes but only because we don't have any other >> > alternative. What I'd really rather have is a safe way to attach datum >> > (ie. acpi_device or device_node) to a struct device and get it back >> > later in a type safe way. >> >> Yep, *that* makes perfect sense to me. Something along these lines, maybe: >> >> #define dev_is_acpi(d) ((d)->bus == &acpi_bus_type) > > No, that's not right. It won't work for things like SPI and I2C with a > "backing" ACPI device node anyway (and for PCI too, by the way :-)). > >> #define dev_acpi_handle(d) (dev_is_acpi(d) ? (acpi_handle) >> d->datum : NULL) > > The problem basically is how we can tell that the given struct device has > a "backing" object containing device information (e.g. resources) and what > that "backing" object is. For device trees that would be a struct device_node > and for ACPI that would be an acpi_handle or a struct acpi_device etc. And by > the way, they _can_ be used simultaneously, in principle. > > So we need something like of_node(dev) or acpi_node(dev), but that can't be > something following two pointers or calling a function just in order to check > if the pointer _is_ _there_ in either case. > > And since we added of_node to struct device at one point, it is only logical to > treat ACPI in the same way. If we come up with a better idea _later_, then we > can convert _all_ things to this new idea, whatever it is. > > Are you seriously expecting us to come up with such an idea on the fly just so > that we can use ACPI support, which already is there in the form of > archdata.acpi_handle anyway, on equal footing with Device Trees? Of course not. I'm just trying to understand where we're headed. That was not obvious from the patches I've seen so far. -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Saturday, November 10, 2012 10:14:47 AM Bjorn Helgaas wrote: > On Sat, Nov 10, 2012 at 4:10 AM, Rafael J. Wysocki <rjw@sisk.pl> wrote: > > On Friday, November 09, 2012 09:53:26 AM Bjorn Helgaas wrote: > >> On Fri, Nov 9, 2012 at 9:43 AM, Grant Likely <grant.likely@secretlab.ca> wrote: > >> > On Fri, Nov 9, 2012 at 4:35 PM, Bjorn Helgaas <bhelgaas@google.com> wrote: > >> >> On Fri, Nov 9, 2012 at 8:45 AM, Grant Likely <grant.likely@secretlab.ca> wrote: > >> >>> On Fri, Nov 9, 2012 at 3:11 PM, Bjorn Helgaas <bhelgaas@google.com> wrote: > >> >>>> [+cc Greg, Peter, Tony since they acked the original patch [1]] > >> >>>> > >> >>>> On Thu, Nov 8, 2012 at 1:04 PM, Mika Westerberg > >> >>>> <mika.westerberg@linux.intel.com> wrote: > >> >>>>> On Thu, Nov 08, 2012 at 12:32:25PM -0700, Bjorn Helgaas wrote: > >> >>>>>> Struct device_driver is a generic structure, so it seems strange to > >> >>>>>> have to include non-generic things like of_device_id and now > >> >>>>>> acpi_match_table there. > >> >>>>> > >> >>>>> Yes, but in a sense the DT and ACPI are "generic". So that they are used to > >> >>>>> describe the configuration of a machine. > >> >>>> > >> >>>> What I meant by "generic" was "useful across all architectures." The > >> >>>> new acpi_match_table and acpi_handle fields [1] are not generic in > >> >>>> that sense because they're present on all architectures but used only > >> >>>> on x86 and ia64. The existing of_match_table and of_node are > >> >>>> similarly unused on many architectures. This doesn't seem like a > >> >>>> scalable strategy to me. Are we going to add a pnpbios_node for x86 > >> >>>> PNPBIOS machines without ACPI, a pdc_hpa for parisc machines with PDC, > >> >>>> etc.? > >> >>>> > >> >>>> [1] https://patchwork.kernel.org/patch/1677221/ > >> >>> > >> >>> Ultimately yes, I think that is what we want to do, > >> >> > >> >> Just to be clear, you think we *should* add things like pnpbios_node, > >> >> pdc_hpa, etc., to struct device, one field for every scheme of telling > >> >> the OS about non-enumerable devices, where only one of the N fields is > >> >> used on any given machine? That seems surprising to me, but maybe I > >> >> just need to be educated :) > >> > > >> > Ah, I see what you're asking. > >> > > >> > In the short term, yes but only because we don't have any other > >> > alternative. What I'd really rather have is a safe way to attach datum > >> > (ie. acpi_device or device_node) to a struct device and get it back > >> > later in a type safe way. > >> > >> Yep, *that* makes perfect sense to me. Something along these lines, maybe: > >> > >> #define dev_is_acpi(d) ((d)->bus == &acpi_bus_type) > > > > No, that's not right. It won't work for things like SPI and I2C with a > > "backing" ACPI device node anyway (and for PCI too, by the way :-)). > > > >> #define dev_acpi_handle(d) (dev_is_acpi(d) ? (acpi_handle) > >> d->datum : NULL) > > > > The problem basically is how we can tell that the given struct device has > > a "backing" object containing device information (e.g. resources) and what > > that "backing" object is. For device trees that would be a struct device_node > > and for ACPI that would be an acpi_handle or a struct acpi_device etc. And by > > the way, they _can_ be used simultaneously, in principle. > > > > So we need something like of_node(dev) or acpi_node(dev), but that can't be > > something following two pointers or calling a function just in order to check > > if the pointer _is_ _there_ in either case. > > > > And since we added of_node to struct device at one point, it is only logical to > > treat ACPI in the same way. If we come up with a better idea _later_, then we > > can convert _all_ things to this new idea, whatever it is. > > > > Are you seriously expecting us to come up with such an idea on the fly just so > > that we can use ACPI support, which already is there in the form of > > archdata.acpi_handle anyway, on equal footing with Device Trees? > > Of course not. I'm just trying to understand where we're headed. > That was not obvious from the patches I've seen so far. No, it wasn't, fair enough. Thanks, Rafael
diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c index 84c2861..de22a6e 100644 --- a/drivers/spi/spi.c +++ b/drivers/spi/spi.c @@ -35,6 +35,7 @@ #include <linux/sched.h> #include <linux/delay.h> #include <linux/kthread.h> +#include <linux/acpi.h> static void spidev_release(struct device *dev) { @@ -93,6 +94,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 +893,227 @@ static void of_register_spi_devices(struct spi_master *master) static void of_register_spi_devices(struct spi_master *master) { } #endif +#ifdef CONFIG_ACPI +struct acpi_spi { + acpi_status (*callback)(struct acpi_device *, void *); + void *data; +}; + +static acpi_status acpi_spi_enumerate_device(acpi_handle handle, u32 level, + void *data, void **return_value) +{ + struct acpi_spi *acpi_spi = data; + struct acpi_device *adev; + + if (acpi_bus_get_device(handle, &adev)) + return AE_OK; + if (acpi_bus_get_status(adev) || !adev->status.present) + return AE_OK; + + return acpi_spi->callback(adev, acpi_spi->data); +} + +static acpi_status acpi_spi_enumerate(acpi_handle handle, + acpi_status (*callback)(struct acpi_device *, void *), void *data) +{ + struct acpi_spi acpi_spi; + + acpi_spi.callback = callback; + acpi_spi.data = data; + + return acpi_walk_namespace(ACPI_TYPE_DEVICE, handle, 1, + acpi_spi_enumerate_device, NULL, + &acpi_spi, NULL); +} + +struct acpi_spi_device_info { + struct spi_device *spi; + int triggering; + int polarity; + int gsi; + bool valid; +}; + +static acpi_status acpi_spi_add_resources(struct acpi_resource *res, void *data) +{ + struct acpi_spi_device_info *info = data; + struct acpi_resource_spi_serialbus *sb; + struct spi_device *spi = info->spi; + + switch (res->type) { + case ACPI_RESOURCE_TYPE_SERIAL_BUS: + sb = &res->data.spi_serial_bus; + if (sb->type == ACPI_RESOURCE_SERIAL_TYPE_SPI) { + spi->chip_select = sb->device_selection; + spi->max_speed_hz = sb->connection_speed; + + /* Mode (clock phase/polarity/etc. */ + 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; + + /* + * The info is valid once we have found the + * SPISerialBus resource. + */ + info->valid = true; + } + break; + + case ACPI_RESOURCE_TYPE_IRQ: + info->gsi = res->data.irq.interrupts[0]; + info->triggering = res->data.irq.triggering; + info->polarity = res->data.irq.polarity; + break; + + case ACPI_RESOURCE_TYPE_EXTENDED_IRQ: + info->gsi = res->data.extended_irq.interrupts[0]; + info->triggering = res->data.extended_irq.triggering; + info->polarity = res->data.extended_irq.polarity; + break; + } + + return AE_OK; +} + +static acpi_status acpi_spi_add_device(struct acpi_device *adev, void *data) +{ + struct acpi_spi_device_info info; + struct spi_master *master = data; + struct spi_device *spi; + acpi_status status; + + spi = spi_alloc_device(master); + if (!spi) { + dev_err(&master->dev, "failed to allocate SPI device\n"); + return AE_ERROR; + } + + memset(&info, 0, sizeof(info)); + info.spi = spi; + info.gsi = -1; + + status = acpi_walk_resources(adev->handle, METHOD_NAME__CRS, + acpi_spi_add_resources, &info); + if (ACPI_FAILURE(status) || !info.valid) + goto fail_put_dev; + + strlcpy(spi->modalias, acpi_device_hid(adev), sizeof(spi->modalias)); + if (info.gsi >= 0) + spi->irq = acpi_register_gsi(&adev->dev, info.gsi, + info.triggering, info.polarity); + request_module(spi->modalias); + if (spi_add_device(spi)) { + dev_err(&master->dev, "failed to add SPI device from ACPI\n"); + goto fail_unregister_gsi; + } + + return AE_OK; + + fail_unregister_gsi: + if (info.gsi >= 0) + acpi_unregister_gsi(info.gsi); + fail_put_dev: + spi_dev_put(spi); + + return AE_OK; +} + +static void acpi_register_spi_devices(struct spi_master *master) +{ + acpi_status status; + acpi_handle handle; + + handle = master->dev.acpi_handle; + if (!handle) + return; + + status = acpi_spi_enumerate(handle, acpi_spi_add_device, master); + if (ACPI_FAILURE(status)) + dev_warn(&master->dev, "failed to enumerate SPI slaves\n"); +} + +struct acpi_spi_find { + acpi_handle handle; + u16 chip_select; + bool found; +}; + +static acpi_status acpi_spi_find_child_address(struct acpi_resource *res, + void *data) +{ + struct acpi_resource_spi_serialbus *sb; + struct acpi_spi_find *spi_find = data; + + if (res->type != ACPI_RESOURCE_TYPE_SERIAL_BUS) + return AE_OK; + + sb = &res->data.spi_serial_bus; + if (sb->type != ACPI_RESOURCE_SERIAL_TYPE_SPI) + return AE_OK; + + if (sb->device_selection == spi_find->chip_select) { + spi_find->found = true; + return AE_CTRL_TERMINATE; + } + + return AE_OK; +} + +static acpi_status acpi_spi_find_child(struct acpi_device *adev, void *data) +{ + struct acpi_spi_find *spi_find = data; + acpi_status status; + + status = acpi_walk_resources(adev->handle, METHOD_NAME__CRS, + acpi_spi_find_child_address, spi_find); + if (ACPI_FAILURE(status) || !spi_find->found) + return status; + + spi_find->handle = adev->handle; + return AE_CTRL_TERMINATE; +} + +static int acpi_spi_find_device(struct device *dev, acpi_handle *handle) +{ + struct spi_device *spi = to_spi_device(dev); + struct spi_master *master = spi->master; + struct acpi_spi_find spi_find; + acpi_handle parent; + acpi_status status; + + parent = master->dev.acpi_handle; + if (!parent) + return -ENODEV; + + memset(&spi_find, 0, sizeof(spi_find)); + spi_find.chip_select = spi->chip_select; + + status = acpi_spi_enumerate(parent, acpi_spi_find_child, &spi_find); + if (ACPI_FAILURE(status) || !spi_find.handle) + return -ENODEV; + + *handle = spi_find.handle; + return 0; +} + +static struct acpi_bus_type acpi_spi_bus = { + .bus = &spi_bus_type, + .find_device = acpi_spi_find_device, +}; + +static void acpi_spi_bus_register(void) +{ + register_acpi_bus_type(&acpi_spi_bus); +} +#else +static inline void acpi_register_spi_devices(struct spi_master *master) {} +static inline void acpi_spi_bus_register(void) {} +#endif /* CONFIG_ACPI */ + static void spi_master_release(struct device *dev) { struct spi_master *master; @@ -1023,8 +1249,9 @@ int spi_register_master(struct spi_master *master) spi_match_master_to_boardinfo(master, &bi->board_info); mutex_unlock(&board_lock); - /* Register devices from the device tree */ + /* Register devices from the device tree and ACPI */ of_register_spi_devices(master); + acpi_register_spi_devices(master); done: return status; } @@ -1550,6 +1777,8 @@ static int __init spi_init(void) status = class_register(&spi_master_class); if (status < 0) goto err2; + + acpi_spi_bus_register(); return 0; err2: