Re: [PATCH v4 2/9] spi: Create helper API to lookup ACPI info for spi device

Hi Stefan,

First of all thank you for doing the requested refactoring to reuse the existing ACPI SPI enumeration code from drivers/spi/spi.c .

On 1/20/22 14:43, Stefan Binding wrote:

This can then be used to find a spi resource inside an ACPI node, and allocate a spi device.

Signed-off-by: Stefan Binding sbinding@opensource.cirrus.com

drivers/spi/spi.c | 46 ++++++++++++++++++++++++++++++++--------- include/linux/spi/spi.h | 12 +++++++++++ 2 files changed, 48 insertions(+), 10 deletions(-)

diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c index 1eb84101c4ad..13f4701f0694 100644 --- a/drivers/spi/spi.c +++ b/drivers/spi/spi.c @@ -2410,8 +2410,18 @@ static int acpi_spi_add_resource(struct acpi_resource *ares, void *data) return 1; }

-static acpi_status acpi_register_spi_device(struct spi_controller *ctlr,

• 			    struct acpi_device *adev)
  

+/**

• • acpi_spi_device_alloc - Allocate a spi device, and fill it in with ACPI information
• • @ctlr: controller to which the spi device belongs
• • @adev: ACPI Device for the spi device
• • This should be used to allocate a new spi device from and ACPI Node.
• • The caller is responsible for calling spi_add_device to register the spi device.
• • Return: a pointer to the new device, or ERR_PTR on error.
• */

+struct spi_device *acpi_spi_device_alloc(struct spi_controller *ctlr,

• 			 struct acpi_device *adev)
  

{ acpi_handle parent_handle = NULL; struct list_head resource_list; @@ -2419,10 +2429,6 @@ static acpi_status acpi_register_spi_device(struct spi_controller *ctlr, struct spi_device *spi; int ret;

• if (acpi_bus_get_status(adev) || !adev->status.present ||

•    acpi_device_enumerated(adev))
  

• return AE_OK;
  

• lookup.ctlr = ctlr; lookup.irq = -1;

@@ -2433,7 +2439,7 @@ static acpi_status acpi_register_spi_device(struct spi_controller *ctlr,

if (ret < 0) /* found SPI in _CRS but it points to another controller */

• return AE_OK;
  

• return ERR_PTR(-ENODEV);
  
  

  if (!lookup.max_speed_hz && ACPI_SUCCESS(acpi_get_parent(adev->handle, &parent_handle)) &&

@@ -2443,16 +2449,15 @@ static acpi_status acpi_register_spi_device(struct spi_controller *ctlr, }

if (!lookup.max_speed_hz)

• return AE_OK;
  

• return ERR_PTR(-ENODEV);
  
  

  spi = spi_alloc_device(ctlr); if (!spi) { dev_err(&ctlr->dev, "failed to allocate SPI device for %s\n", dev_name(&adev->dev));

• return AE_NO_MEMORY;
  

• return ERR_PTR(-ENOMEM);
  

  }

• ACPI_COMPANION_SET(&spi->dev, adev); spi->max_speed_hz = lookup.max_speed_hz; spi->mode |= lookup.mode;

@@ -2460,6 +2465,27 @@ static acpi_status acpi_register_spi_device(struct spi_controller *ctlr, spi->bits_per_word = lookup.bits_per_word; spi->chip_select = lookup.chip_select;

• return spi;

+} +EXPORT_SYMBOL_GPL(acpi_spi_device_alloc);

+static acpi_status acpi_register_spi_device(struct spi_controller *ctlr,

• 			    struct acpi_device *adev)
  

+{

• struct spi_device *spi;
• if (acpi_bus_get_status(adev) || !adev->status.present ||

•    acpi_device_enumerated(adev))
  

• return AE_OK;
  

• spi = acpi_spi_device_alloc(ctlr, adev);
• if (IS_ERR(spi)) {

• if (PTR_ERR(spi) == -ENOMEM)
  

• 	return AE_NO_MEMORY;
  

• else
  

• 	return AE_OK;
  

• }
• acpi_set_modalias(adev, acpi_device_hid(adev), spi->modalias, sizeof(spi->modalias));

diff --git a/include/linux/spi/spi.h b/include/linux/spi/spi.h index 0346a3ff27fd..200725692b93 100644 --- a/include/linux/spi/spi.h +++ b/include/linux/spi/spi.h @@ -16,6 +16,7 @@ #include <linux/gpio/consumer.h>

#include <uapi/linux/spi/spi.h> +#include <linux/acpi.h>

struct dma_chan; struct software_node; @@ -759,6 +760,17 @@ extern int devm_spi_register_controller(struct device *dev, struct spi_controller *ctlr); extern void spi_unregister_controller(struct spi_controller *ctlr);

+#if IS_ENABLED(CONFIG_ACPI) +extern struct spi_device *acpi_spi_device_alloc(struct spi_controller *ctlr,

• 				struct acpi_device *adev);
  

+#else +static inline struct spi_device *acpi_spi_device_alloc(struct spi_controller *ctlr,

• 				       struct acpi_device *adev);
  

+{

• return ERR_PTR(-EOPNOTSUPP);

+} +#endif

I don't think having a stub here is necessary ? I would expect any callers of this to make sure that both CONFIG_SPI and CONFIG_ACPI are set. Note that not having CONFIG_SPI set is not caught by your stubbing here and there are no stubs for any of the other spi functions.

Regards,

Hans

/*

• SPI resource management while processing a SPI message

*/