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

[Stefan Binding <sbinding@xxxxxxxxxxxxxxxxxxxxx>]

Hi,

> -----Original Message-----
> From: Hans de Goede <hdegoede@xxxxxxxxxx>
> Sent: 01 February 2022 14:28
> To: Stefan Binding <sbinding@xxxxxxxxxxxxxxxxxxxxx>; Mark Brown
> <broonie@xxxxxxxxxx>; Rafael J . Wysocki <rafael@xxxxxxxxxx>; Len Brown
> <lenb@xxxxxxxxxx>; Mark Gross <markgross@xxxxxxxxxx>; Jaroslav Kysela
> <perex@xxxxxxxx>; Takashi Iwai <tiwai@xxxxxxxx>
> Cc: alsa-devel@xxxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; linux-
> spi@xxxxxxxxxxxxxxx; linux-acpi@xxxxxxxxxxxxxxx; platform-driver-
> x86@xxxxxxxxxxxxxxx; patches@xxxxxxxxxxxxxxxxxxxxx
> 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 <sbinding@xxxxxxxxxxxxxxxxxxxxx>
> > ---
> >  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 <hdegoede@xxxxxxxxxx>
> 
> 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
> >   */
> >