Hi,

On 2/2/22 10:05, Takashi Iwai wrote:
> On Fri, 21 Jan 2022 18:24:30 +0100,
> Stefan Binding wrote:
>>
>> From: Lucas Tanure <[email protected]>
>>
>> Add support for two and four CS35L41 using the component
>> binding method
>>
>> Signed-off-by: Lucas Tanure <[email protected]>
>> Signed-off-by: Stefan Binding <[email protected]>
>
> Reviewed-by: Takashi Iwai <[email protected]>
>
> Hans, feel free to include this one into your tree.

Ok, I'm working on merging the entire series (on top of Mark's
spi-acpi-helpers tag) now. I'll send you a pull-req for
a signed tag when I'm done, in case you want to merge
this to avoid conflicts.

Regards,

Hans

2022-02-07 13:46:35

by Stefan Binding

[permalink] [raw]

Subject: RE: [PATCH v6 2/9] spi: Create helper API to lookup ACPI info for spi device

Hi,

> -----Original Message-----
> From: Hans de Goede <[email protected]>
> Sent: 01 February 2022 14:28
> To: Stefan Binding <[email protected]>; Mark Brown
> <[email protected]>; Rafael J . Wysocki <[email protected]>; Len Brown
> <[email protected]>; Mark Gross <[email protected]>; Jaroslav Kysela
> <[email protected]>; Takashi Iwai <[email protected]>
> Cc: [email protected]; [email protected]; linux-
> [email protected]; [email protected]; platform-driver-
> [email protected]; [email protected]
> Subject: Re: [PATCH v6 2/9] spi: Create helper API to lookup ACPI info for spi
> device
>
> Hi,
>
> On 1/21/22 18:24, 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 <[email protected]>
> > ---
> > drivers/spi/spi.c | 46 ++++++++++++++++++++++++++++++++---------
> > include/linux/spi/spi.h | 6 ++++++
> > 2 files changed, 42 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..d159cef12f1a 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,11 @@ 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);
> > +#endif
> > +
>
> There is no need to add a #ifdef about something which is just a
> function prototype. Having this declared when CONFIG_ACPI is not set is
> harmless, please drop the #ifdef.
>
> With that fixed, please add my R-b to the next version:
>
> Reviewed-by: Hans de Goede <[email protected]>
>
> Regards,
>
> Hans

I was just fixing this, however, I just noticed that a subset of this chain - including
this patch - just got applied. Do you want me to fix this in a separate patch?

Thanks,
Stefan

>
>
>
> > /*
> > * SPI resource management while processing a SPI message
> > */
> >