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@xxxxxxxxxxxxxxxxxxxxx>
> ---
> 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
> */
>
