From: Hans de Goede <hdegoede@redhat.com>
To: Stefan Binding <sbinding@opensource.cirrus.com>,
Mark Brown <broonie@kernel.org>,
"Rafael J . Wysocki" <rafael@kernel.org>,
Len Brown <lenb@kernel.org>, Mark Gross <markgross@kernel.org>,
Jaroslav Kysela <perex@perex.cz>, Takashi Iwai <tiwai@suse.com>
Cc: alsa-devel@alsa-project.org, linux-kernel@vger.kernel.org,
linux-spi@vger.kernel.org, linux-acpi@vger.kernel.org,
platform-driver-x86@vger.kernel.org,
patches@opensource.cirrus.com
Subject: Re: [PATCH v4 2/9] spi: Create helper API to lookup ACPI info for spi device
Date: Thu, 20 Jan 2022 15:37:46 +0100 [thread overview]
Message-ID: <f849c693-86d3-8454-eda9-96c69daaf862@redhat.com> (raw)
In-Reply-To: <20220120134326.5295-3-sbinding@opensource.cirrus.com>
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
> */
>
next prev parent reply other threads:[~2022-01-20 14:37 UTC|newest]
Thread overview: 27+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-01-20 13:43 [PATCH v4 0/9] Support Spi in i2c-multi-instantiate driver Stefan Binding
2022-01-20 13:43 ` [PATCH v4 1/9] spi: Make spi_alloc_device and spi_add_device public again Stefan Binding
2022-01-20 14:39 ` Hans de Goede
2022-01-20 13:43 ` [PATCH v4 2/9] spi: Create helper API to lookup ACPI info for spi device Stefan Binding
2022-01-20 14:37 ` Hans de Goede [this message]
2022-01-20 13:43 ` [PATCH v4 3/9] spi: Support selection of the index of the ACPI Spi Resource before alloc Stefan Binding
2022-01-20 15:03 ` Hans de Goede
2022-01-20 13:43 ` [PATCH v4 4/9] spi: Add API to count spi acpi resources Stefan Binding
2022-01-20 14:39 ` Hans de Goede
2022-01-20 15:04 ` Hans de Goede
2022-01-20 13:43 ` [PATCH v4 5/9] platform/x86: i2c-multi-instantiate: Rename it for a generic bus driver name Stefan Binding
2022-01-20 15:05 ` Hans de Goede
2022-01-20 18:34 ` Rafael J. Wysocki
2022-01-20 18:39 ` Hans de Goede
2022-01-20 13:43 ` [PATCH v4 6/9] platform/x86: bus-multi-instantiate: Reorganize I2C functions Stefan Binding
2022-01-20 15:13 ` Hans de Goede
2022-01-20 16:03 ` Hans de Goede
2022-01-20 16:17 ` Hans de Goede
2022-01-20 13:43 ` [PATCH v4 7/9] platform/x86: bus-multi-instantiate: Add SPI support Stefan Binding
2022-01-20 16:30 ` Hans de Goede
2022-01-20 13:43 ` [PATCH v4 8/9] ALSA: hda/realtek: Add support for HP Laptops Stefan Binding
2022-01-20 15:27 ` Takashi Iwai
2022-01-21 14:32 ` Stefan Binding
2022-01-21 14:39 ` Takashi Iwai
2022-01-20 13:43 ` [PATCH v4 9/9] ACPI / scan: Create platform device for CS35L41 Stefan Binding
2022-01-20 16:31 ` Hans de Goede
2022-01-20 16:34 ` [PATCH v4 0/9] Support Spi in i2c-multi-instantiate driver Hans de Goede
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=f849c693-86d3-8454-eda9-96c69daaf862@redhat.com \
--to=hdegoede@redhat.com \
--cc=alsa-devel@alsa-project.org \
--cc=broonie@kernel.org \
--cc=lenb@kernel.org \
--cc=linux-acpi@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-spi@vger.kernel.org \
--cc=markgross@kernel.org \
--cc=patches@opensource.cirrus.com \
--cc=perex@perex.cz \
--cc=platform-driver-x86@vger.kernel.org \
--cc=rafael@kernel.org \
--cc=sbinding@opensource.cirrus.com \
--cc=tiwai@suse.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).