| Message ID | 1461105548-20618-7-git-send-email-octavian.purdila@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, Apr 20, 2016 at 01:39:04AM +0300, Octavian Purdila wrote: > This patch adds supports for SPI device enumeration and removal via > ACPI reconfiguration notifications that are send as a result of an > ACPI table load or unload operation. > > Signed-off-by: Octavian Purdila <octavian.purdila@intel.com> Same here, pretty straightforward. Reviewed-by: Mika Westerberg <mika.westerberg@linux.intel.com> -- To unsubscribe from this list: send the line "unsubscribe linux-spi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Apr 20, 2016 at 01:39:04AM +0300, Octavian Purdila wrote: > + switch (value) { > + case ACPI_RECONFIG_DEVICE_ADD: > + master = acpi_spi_find_master_by_adev(adev->parent); > + if (!master) > + break; > + > + acpi_register_spi_device(master, adev); > + put_device(&master->dev); > + break; > + case ACPI_RECONFIG_DEVICE_REMOVE: > + spi = acpi_spi_find_device_by_adev(adev); > + if (!spi) > + break; There's more code here now than I remember but this all looks *really* close to the DT code except for the OF_POPULATED flag that we set when things are instantiated in DT. The duplication seems bad but the fact that we're missing the flag worries me... do we have guarantees that ACPI won't double register?
On Thu, Apr 28, 2016 at 8:42 PM, Mark Brown <broonie@kernel.org> wrote: > On Wed, Apr 20, 2016 at 01:39:04AM +0300, Octavian Purdila wrote: > >> + switch (value) { >> + case ACPI_RECONFIG_DEVICE_ADD: >> + master = acpi_spi_find_master_by_adev(adev->parent); >> + if (!master) >> + break; >> + >> + acpi_register_spi_device(master, adev); >> + put_device(&master->dev); >> + break; >> + case ACPI_RECONFIG_DEVICE_REMOVE: >> + spi = acpi_spi_find_device_by_adev(adev); >> + if (!spi) >> + break; > > There's more code here now than I remember but this all looks *really* > close to the DT code except for the OF_POPULATED flag that we set when > things are instantiated in DT. The duplication seems bad but the fact > that we're missing the flag worries me... do we have guarantees that > ACPI won't double register? We use the adev->flags.visited to check when a device has been already enumerated, and we skip registering a new SPI slave in that case. -- To unsubscribe from this list: send the line "unsubscribe linux-spi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Apr 28, 2016 at 10:37:57PM +0300, Octavian Purdila wrote: > On Thu, Apr 28, 2016 at 8:42 PM, Mark Brown <broonie@kernel.org> wrote: > > There's more code here now than I remember but this all looks *really* > > close to the DT code except for the OF_POPULATED flag that we set when > > things are instantiated in DT. The duplication seems bad but the fact > > that we're missing the flag worries me... do we have guarantees that > > ACPI won't double register? > We use the adev->flags.visited to check when a device has been already > enumerated, and we skip registering a new SPI slave in that case. OK, but the fact that I need to know that isn't exactly thrilling - that's really the issue with the not quite duplication here. It's not just if the code works but also the maintainability.
On Tue, May 3, 2016 at 3:19 PM, Mark Brown <broonie@kernel.org> wrote: > On Thu, Apr 28, 2016 at 10:37:57PM +0300, Octavian Purdila wrote: >> On Thu, Apr 28, 2016 at 8:42 PM, Mark Brown <broonie@kernel.org> wrote: > >> > There's more code here now than I remember but this all looks *really* >> > close to the DT code except for the OF_POPULATED flag that we set when >> > things are instantiated in DT. The duplication seems bad but the fact >> > that we're missing the flag worries me... do we have guarantees that >> > ACPI won't double register? > >> We use the adev->flags.visited to check when a device has been already >> enumerated, and we skip registering a new SPI slave in that case. > > OK, but the fact that I need to know that isn't exactly thrilling - > that's really the issue with the not quite duplication here. It's not > just if the code works but also the maintainability. I agree that this is unfortunate. I could not find a common path for both device tree and ACPI. Perhaps when we remove of_node from struct device and move to fwnode we could remove some duplication. I can add a couple of comments around acpi_device_enumerated() and adev->flags.visited = true; to make it clear that we won't double enumerate. Will that help? -- To unsubscribe from this list: send the line "unsubscribe linux-spi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c index de2f2f9..d6c5a9c 100644 --- a/drivers/spi/spi.c +++ b/drivers/spi/spi.c @@ -1644,18 +1644,15 @@ static int acpi_spi_add_resource(struct acpi_resource *ares, void *data) return 1; } -static acpi_status acpi_spi_add_device(acpi_handle handle, u32 level, - void *data, void **return_value) +static acpi_status acpi_register_spi_device(struct spi_master *master, + struct acpi_device *adev) { - struct spi_master *master = data; struct list_head resource_list; - struct acpi_device *adev; struct spi_device *spi; int ret; - if (acpi_bus_get_device(handle, &adev)) - return AE_OK; - if (acpi_bus_get_status(adev) || !adev->status.present) + if (acpi_bus_get_status(adev) || !adev->status.present || + acpi_device_enumerated(adev)) return AE_OK; spi = spi_alloc_device(master); @@ -1681,6 +1678,8 @@ static acpi_status acpi_spi_add_device(acpi_handle handle, u32 level, if (spi->irq < 0) spi->irq = acpi_dev_gpio_irq_get(adev, 0); + adev->flags.visited = true; + adev->power.flags.ignore_parent = true; strlcpy(spi->modalias, acpi_device_hid(adev), sizeof(spi->modalias)); if (spi_add_device(spi)) { @@ -1693,6 +1692,18 @@ static acpi_status acpi_spi_add_device(acpi_handle handle, u32 level, return AE_OK; } +static acpi_status acpi_spi_add_device(acpi_handle handle, u32 level, + void *data, void **return_value) +{ + struct spi_master *master = data; + struct acpi_device *adev; + + if (acpi_bus_get_device(handle, &adev)) + return AE_OK; + + return acpi_register_spi_device(master, adev); +} + static void acpi_register_spi_devices(struct spi_master *master) { acpi_status status; @@ -3104,6 +3115,73 @@ static struct notifier_block spi_of_notifier = { extern struct notifier_block spi_of_notifier; #endif /* IS_ENABLED(CONFIG_OF_DYNAMIC) */ +#if IS_ENABLED(CONFIG_ACPI) +static int spi_acpi_master_match(struct device *dev, const void *data) +{ + return ACPI_COMPANION(dev->parent) == data; +} + +static int spi_acpi_device_match(struct device *dev, void *data) +{ + return ACPI_COMPANION(dev) == data; +} + +static struct spi_master *acpi_spi_find_master_by_adev(struct acpi_device *adev) +{ + struct device *dev; + + dev = class_find_device(&spi_master_class, NULL, adev, + spi_acpi_master_match); + if (!dev) + return NULL; + + return container_of(dev, struct spi_master, dev); +} + +static struct spi_device *acpi_spi_find_device_by_adev(struct acpi_device *adev) +{ + struct device *dev; + + dev = bus_find_device(&spi_bus_type, NULL, adev, spi_acpi_device_match); + + return dev ? to_spi_device(dev) : NULL; +} + +static int acpi_spi_notify(struct notifier_block *nb, unsigned long value, + void *arg) +{ + struct acpi_device *adev = arg; + struct spi_master *master; + struct spi_device *spi; + + switch (value) { + case ACPI_RECONFIG_DEVICE_ADD: + master = acpi_spi_find_master_by_adev(adev->parent); + if (!master) + break; + + acpi_register_spi_device(master, adev); + put_device(&master->dev); + break; + case ACPI_RECONFIG_DEVICE_REMOVE: + spi = acpi_spi_find_device_by_adev(adev); + if (!spi) + break; + + spi_unregister_device(spi); + put_device(&spi->dev); + } + + return NOTIFY_OK; +} + +static struct notifier_block spi_acpi_notifier = { + .notifier_call = acpi_spi_notify, +}; +#else +extern struct notifier_block spi_acpi_notifier; +#endif + static int __init spi_init(void) { int status; @@ -3124,6 +3202,8 @@ static int __init spi_init(void) if (IS_ENABLED(CONFIG_OF_DYNAMIC)) WARN_ON(of_reconfig_notifier_register(&spi_of_notifier)); + if (IS_ENABLED(CONFIG_ACPI)) + WARN_ON(acpi_reconfig_notifier_register(&spi_acpi_notifier)); return 0;
This patch adds supports for SPI device enumeration and removal via ACPI reconfiguration notifications that are send as a result of an ACPI table load or unload operation. Signed-off-by: Octavian Purdila <octavian.purdila@intel.com> --- drivers/spi/spi.c | 94 ++++++++++++++++++++++++++++++++++++++++++++++++++----- 1 file changed, 87 insertions(+), 7 deletions(-)