[PATCH 0/3] ACPI/ALSA/soundwire: add acpi_get_local_u64_address() helper

[PATCH 0/3] ACPI/ALSA/soundwire: add acpi_get_local_u64_address() helper

[Pierre-Louis Bossart]

The acpi_get_local_address() helper assumes a 32-bit ADR is used. If
we want to use this helper for SoundWire/SDCA ASoC codecs, we need an
extension where the native 64-bits are used. This patchset suggests a
new helper, acpi_get_local_address() may be renamed if desired in a
folow-up patch.

The path of least resistance would be to merge this patchset in the
ASoC tree, since I have additional changes for ASoC/SDCA (SoundWire
Device Class) that depend on the new helper.

Pierre-Louis Bossart (3):
  ACPI: utils: introduce acpi_get_local_u64_address()
  soundwire: slave: simplify code with acpi_get_local_u64_address()
  ALSA: hda: intel-sdw-acpi: use acpi_get_local_u64_address()

 drivers/acpi/utils.c       | 22 ++++++++++++++++------
 drivers/soundwire/slave.c  | 13 ++++---------
 include/linux/acpi.h       |  1 +
 sound/hda/intel-sdw-acpi.c |  6 +++---
 4 files changed, 24 insertions(+), 18 deletions(-)

-- 
2.43.0

[PATCH 1/3] ACPI: utils: introduce acpi_get_local_u64_address()

[Pierre-Louis Bossart]

The ACPI _ADR is a 64-bit value. We changed the definitions in commit
ca6f998cf9a2 ("ACPI: bus: change _ADR representation to 64 bits") but
some helpers still assume the value is a 32-bit value.

This patch adds a new helper to extract the full 64-bits. The existing
32-bit helper is kept for backwards-compatibility and cases where the
_ADR is known to fit in a 32-bit value.

Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Reviewed-by: Péter Ujfalusi <peter.ujfalusi@linux.intel.com>
Reviewed-by: Bard Liao <yung-chuan.liao@linux.intel.com>
---
 drivers/acpi/utils.c | 22 ++++++++++++++++------
 include/linux/acpi.h |  1 +
 2 files changed, 17 insertions(+), 6 deletions(-)

diff --git a/drivers/acpi/utils.c b/drivers/acpi/utils.c
index 202234ba54bd..ae9384282273 100644
--- a/drivers/acpi/utils.c
+++ b/drivers/acpi/utils.c
@@ -277,15 +277,25 @@ acpi_evaluate_integer(acpi_handle handle,
 
 EXPORT_SYMBOL(acpi_evaluate_integer);
 
+int acpi_get_local_u64_address(acpi_handle handle, u64 *addr)
+{
+	acpi_status status;
+
+	status = acpi_evaluate_integer(handle, METHOD_NAME__ADR, NULL, addr);
+	if (ACPI_FAILURE(status))
+		return -ENODATA;
+	return 0;
+}
+EXPORT_SYMBOL(acpi_get_local_u64_address);
+
 int acpi_get_local_address(acpi_handle handle, u32 *addr)
 {
-	unsigned long long adr;
-	acpi_status status;
-
-	status = acpi_evaluate_integer(handle, METHOD_NAME__ADR, NULL, &adr);
-	if (ACPI_FAILURE(status))
-		return -ENODATA;
+	u64 adr;
+	int ret;
 
+	ret = acpi_get_local_u64_address(handle, &adr);
+	if (ret < 0)
+		return ret;
 	*addr = (u32)adr;
 	return 0;
 }
diff --git a/include/linux/acpi.h b/include/linux/acpi.h
index 28c3fb2bef0d..65e7177bcb02 100644
--- a/include/linux/acpi.h
+++ b/include/linux/acpi.h
@@ -761,6 +761,7 @@ static inline u64 acpi_arch_get_root_pointer(void)
 }
 #endif
 
+int acpi_get_local_u64_address(acpi_handle handle, u64 *addr);
 int acpi_get_local_address(acpi_handle handle, u32 *addr);
 const char *acpi_get_subsystem_id(acpi_handle handle);
 
-- 
2.43.0

Re: [PATCH 1/3] ACPI: utils: introduce acpi_get_local_u64_address()

[Rafael J. Wysocki]

On Tue, May 28, 2024 at 9:29 PM Pierre-Louis Bossart
<pierre-louis.bossart@linux.intel.com> wrote:
>
> The ACPI _ADR is a 64-bit value. We changed the definitions in commit
> ca6f998cf9a2 ("ACPI: bus: change _ADR representation to 64 bits") but
> some helpers still assume the value is a 32-bit value.
>
> This patch adds a new helper to extract the full 64-bits. The existing
> 32-bit helper is kept for backwards-compatibility and cases where the
> _ADR is known to fit in a 32-bit value.
>
> Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
> Reviewed-by: Péter Ujfalusi <peter.ujfalusi@linux.intel.com>
> Reviewed-by: Bard Liao <yung-chuan.liao@linux.intel.com>

Do you want me to apply this or do you want me to route it along with
the rest of the series?

In the latter case feel free to add

Acked-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

to it.

Thanks!

> ---
>  drivers/acpi/utils.c | 22 ++++++++++++++++------
>  include/linux/acpi.h |  1 +
>  2 files changed, 17 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/acpi/utils.c b/drivers/acpi/utils.c
> index 202234ba54bd..ae9384282273 100644
> --- a/drivers/acpi/utils.c
> +++ b/drivers/acpi/utils.c
> @@ -277,15 +277,25 @@ acpi_evaluate_integer(acpi_handle handle,
>
>  EXPORT_SYMBOL(acpi_evaluate_integer);
>
> +int acpi_get_local_u64_address(acpi_handle handle, u64 *addr)
> +{
> +       acpi_status status;
> +
> +       status = acpi_evaluate_integer(handle, METHOD_NAME__ADR, NULL, addr);
> +       if (ACPI_FAILURE(status))
> +               return -ENODATA;
> +       return 0;
> +}
> +EXPORT_SYMBOL(acpi_get_local_u64_address);

I'd prefer EXPORT_SYMBOL_GPL() here unless you absolutely cannot live with it.

> +
>  int acpi_get_local_address(acpi_handle handle, u32 *addr)
>  {
> -       unsigned long long adr;
> -       acpi_status status;
> -
> -       status = acpi_evaluate_integer(handle, METHOD_NAME__ADR, NULL, &adr);
> -       if (ACPI_FAILURE(status))
> -               return -ENODATA;
> +       u64 adr;
> +       int ret;
>
> +       ret = acpi_get_local_u64_address(handle, &adr);
> +       if (ret < 0)
> +               return ret;
>         *addr = (u32)adr;
>         return 0;
>  }
> diff --git a/include/linux/acpi.h b/include/linux/acpi.h
> index 28c3fb2bef0d..65e7177bcb02 100644
> --- a/include/linux/acpi.h
> +++ b/include/linux/acpi.h
> @@ -761,6 +761,7 @@ static inline u64 acpi_arch_get_root_pointer(void)
>  }
>  #endif
>
> +int acpi_get_local_u64_address(acpi_handle handle, u64 *addr);
>  int acpi_get_local_address(acpi_handle handle, u32 *addr);
>  const char *acpi_get_subsystem_id(acpi_handle handle);
>
> --

Re: [PATCH 1/3] ACPI: utils: introduce acpi_get_local_u64_address()

[Pierre-Louis Bossart]

On 6/7/24 20:51, Rafael J. Wysocki wrote:
> On Tue, May 28, 2024 at 9:29 PM Pierre-Louis Bossart
> <pierre-louis.bossart@linux.intel.com> wrote:
>>
>> The ACPI _ADR is a 64-bit value. We changed the definitions in commit
>> ca6f998cf9a2 ("ACPI: bus: change _ADR representation to 64 bits") but
>> some helpers still assume the value is a 32-bit value.
>>
>> This patch adds a new helper to extract the full 64-bits. The existing
>> 32-bit helper is kept for backwards-compatibility and cases where the
>> _ADR is known to fit in a 32-bit value.
>>
>> Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
>> Reviewed-by: Péter Ujfalusi <peter.ujfalusi@linux.intel.com>
>> Reviewed-by: Bard Liao <yung-chuan.liao@linux.intel.com>
> 
> Do you want me to apply this or do you want me to route it along with
> the rest of the series?
> 
> In the latter case feel free to add
> 
> Acked-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

Thanks Rafael. I think it's easier if Mark Brown takes the series in
ASoC, I have additional ASoC patches that use the u64 helper.

Mark?


>>
>> +int acpi_get_local_u64_address(acpi_handle handle, u64 *addr)
>> +{
>> +       acpi_status status;
>> +
>> +       status = acpi_evaluate_integer(handle, METHOD_NAME__ADR, NULL, addr);
>> +       if (ACPI_FAILURE(status))
>> +               return -ENODATA;
>> +       return 0;
>> +}
>> +EXPORT_SYMBOL(acpi_get_local_u64_address);
> 
> I'd prefer EXPORT_SYMBOL_GPL() here unless you absolutely cannot live with it.

I don't mind, but the existing helper was using EXPORT_SYMBOL so I just
copied. It'd be odd to have two helpers that only differ by the argument
size use a different EXPORT_ macro, no? Not to mention that the
get_local address uses EXPORT_SYMBOL but would become a wrapper for an
EXPORT_SYMBOL_GPL. That gives me a headache...


This was the original code:

int acpi_get_local_address(acpi_handle handle, u32 *addr)
{
	unsigned long long adr;
	acpi_status status;

	status = acpi_evaluate_integer(handle, METHOD_NAME__ADR, NULL, &adr);
	if (ACPI_FAILURE(status))
		return -ENODATA;

	*addr = (u32)adr;
	return 0;
}
EXPORT_SYMBOL(acpi_get_local_address);

Re: [PATCH 1/3] ACPI: utils: introduce acpi_get_local_u64_address()

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 503 bytes --]

On Fri, Jun 07, 2024 at 10:33:00PM +0200, Pierre-Louis Bossart wrote:
> On 6/7/24 20:51, Rafael J. Wysocki wrote:

> > Do you want me to apply this or do you want me to route it along with
> > the rest of the series?

> > In the latter case feel free to add

> > Acked-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

> Thanks Rafael. I think it's easier if Mark Brown takes the series in
> ASoC, I have additional ASoC patches that use the u64 helper.

> Mark?

Sure, no problem taking it via ASoC.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 1/3] ACPI: utils: introduce acpi_get_local_u64_address()

[Rafael J. Wysocki]

On Fri, Jun 7, 2024 at 10:33 PM Pierre-Louis Bossart
<pierre-louis.bossart@linux.intel.com> wrote:
>
>
>
> On 6/7/24 20:51, Rafael J. Wysocki wrote:
> > On Tue, May 28, 2024 at 9:29 PM Pierre-Louis Bossart
> > <pierre-louis.bossart@linux.intel.com> wrote:
> >>
> >> The ACPI _ADR is a 64-bit value. We changed the definitions in commit
> >> ca6f998cf9a2 ("ACPI: bus: change _ADR representation to 64 bits") but
> >> some helpers still assume the value is a 32-bit value.
> >>
> >> This patch adds a new helper to extract the full 64-bits. The existing
> >> 32-bit helper is kept for backwards-compatibility and cases where the
> >> _ADR is known to fit in a 32-bit value.
> >>
> >> Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
> >> Reviewed-by: Péter Ujfalusi <peter.ujfalusi@linux.intel.com>
> >> Reviewed-by: Bard Liao <yung-chuan.liao@linux.intel.com>
> >
> > Do you want me to apply this or do you want me to route it along with
> > the rest of the series?
> >
> > In the latter case feel free to add
> >
> > Acked-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>
> Thanks Rafael. I think it's easier if Mark Brown takes the series in
> ASoC, I have additional ASoC patches that use the u64 helper.
>
> Mark?
>
>
> >>
> >> +int acpi_get_local_u64_address(acpi_handle handle, u64 *addr)
> >> +{
> >> +       acpi_status status;
> >> +
> >> +       status = acpi_evaluate_integer(handle, METHOD_NAME__ADR, NULL, addr);
> >> +       if (ACPI_FAILURE(status))
> >> +               return -ENODATA;
> >> +       return 0;
> >> +}
> >> +EXPORT_SYMBOL(acpi_get_local_u64_address);
> >
> > I'd prefer EXPORT_SYMBOL_GPL() here unless you absolutely cannot live with it.
>
> I don't mind, but the existing helper was using EXPORT_SYMBOL so I just
> copied. It'd be odd to have two helpers that only differ by the argument
> size use a different EXPORT_ macro, no? Not to mention that the
> get_local address uses EXPORT_SYMBOL but would become a wrapper for an
> EXPORT_SYMBOL_GPL. That gives me a headache...

OK, fair enough.

[PATCH 2/3] soundwire: slave: simplify code with acpi_get_local_u64_address()

[Pierre-Louis Bossart]

Now we have a helper so there's no need to open-code.

Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Reviewed-by: Péter Ujfalusi <peter.ujfalusi@linux.intel.com>
Reviewed-by: Bard Liao <yung-chuan.liao@linux.intel.com>
---
 drivers/soundwire/slave.c | 13 ++++---------
 1 file changed, 4 insertions(+), 9 deletions(-)

diff --git a/drivers/soundwire/slave.c b/drivers/soundwire/slave.c
index 9963b92eb505..f1a4df6cfebd 100644
--- a/drivers/soundwire/slave.c
+++ b/drivers/soundwire/slave.c
@@ -97,18 +97,13 @@ static bool find_slave(struct sdw_bus *bus,
 		       struct acpi_device *adev,
 		       struct sdw_slave_id *id)
 {
-	u64 addr;
 	unsigned int link_id;
-	acpi_status status;
+	u64 addr;
+	int ret;
 
-	status = acpi_evaluate_integer(adev->handle,
-				       METHOD_NAME__ADR, NULL, &addr);
-
-	if (ACPI_FAILURE(status)) {
-		dev_err(bus->dev, "_ADR resolution failed: %x\n",
-			status);
+	ret = acpi_get_local_u64_address(adev->handle, &addr);
+	if (ret < 0)
 		return false;
-	}
 
 	if (bus->ops->override_adr)
 		addr = bus->ops->override_adr(bus, addr);
-- 
2.43.0

Re: [PATCH 2/3] soundwire: slave: simplify code with acpi_get_local_u64_address()

[Vinod Koul]

On 28-05-24, 14:29, Pierre-Louis Bossart wrote:
> Now we have a helper so there's no need to open-code.

Acked-by: Vinod Koul <vkoul@kernel.org>

-- 
~Vinod

[PATCH 3/3] ALSA: hda: intel-sdw-acpi: use acpi_get_local_u64_address()

[Pierre-Louis Bossart]

Now we have a helper so there's no need to open-code.

Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Reviewed-by: Péter Ujfalusi <peter.ujfalusi@linux.intel.com>
Reviewed-by: Bard Liao <yung-chuan.liao@linux.intel.com>
---
 sound/hda/intel-sdw-acpi.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/sound/hda/intel-sdw-acpi.c b/sound/hda/intel-sdw-acpi.c
index d7417a40392b..f3b2a610df23 100644
--- a/sound/hda/intel-sdw-acpi.c
+++ b/sound/hda/intel-sdw-acpi.c
@@ -125,11 +125,11 @@ static acpi_status sdw_intel_acpi_cb(acpi_handle handle, u32 level,
 				     void *cdata, void **return_value)
 {
 	struct sdw_intel_acpi_info *info = cdata;
-	acpi_status status;
 	u64 adr;
+	int ret;
 
-	status = acpi_evaluate_integer(handle, METHOD_NAME__ADR, NULL, &adr);
-	if (ACPI_FAILURE(status))
+	ret = acpi_get_local_u64_address(handle, &adr);
+	if (ret < 0)
 		return AE_OK; /* keep going */
 
 	if (!acpi_fetch_acpi_dev(handle)) {
-- 
2.43.0

Re: [PATCH 0/3] ACPI/ALSA/soundwire: add acpi_get_local_u64_address() helper

[Takashi Iwai]

On Tue, 28 May 2024 21:29:32 +0200,
Pierre-Louis Bossart wrote:
> 
> The acpi_get_local_address() helper assumes a 32-bit ADR is used. If
> we want to use this helper for SoundWire/SDCA ASoC codecs, we need an
> extension where the native 64-bits are used. This patchset suggests a
> new helper, acpi_get_local_address() may be renamed if desired in a
> folow-up patch.
> 
> The path of least resistance would be to merge this patchset in the
> ASoC tree, since I have additional changes for ASoC/SDCA (SoundWire
> Device Class) that depend on the new helper.
> 
> Pierre-Louis Bossart (3):
>   ACPI: utils: introduce acpi_get_local_u64_address()
>   soundwire: slave: simplify code with acpi_get_local_u64_address()
>   ALSA: hda: intel-sdw-acpi: use acpi_get_local_u64_address()

Reviewed-by: Takashi Iwai <tiwai@suse.de>


thanks,

Takashi

Re: [PATCH 0/3] ACPI/ALSA/soundwire: add acpi_get_local_u64_address() helper

[Andy Shevchenko]

On Tue, May 28, 2024 at 02:29:32PM -0500, Pierre-Louis Bossart wrote:
> The acpi_get_local_address() helper assumes a 32-bit ADR is used. If
> we want to use this helper for SoundWire/SDCA ASoC codecs, we need an
> extension where the native 64-bits are used. This patchset suggests a
> new helper, acpi_get_local_address() may be renamed if desired in a
> folow-up patch.
> 
> The path of least resistance would be to merge this patchset in the
> ASoC tree, since I have additional changes for ASoC/SDCA (SoundWire
> Device Class) that depend on the new helper.

Perhaps we may rename existing one to _u32 or so.
Anyway, for this series:
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 0/3] ACPI/ALSA/soundwire: add acpi_get_local_u64_address() helper

[Mark Brown]

On Tue, 28 May 2024 14:29:32 -0500, Pierre-Louis Bossart wrote:
> The acpi_get_local_address() helper assumes a 32-bit ADR is used. If
> we want to use this helper for SoundWire/SDCA ASoC codecs, we need an
> extension where the native 64-bits are used. This patchset suggests a
> new helper, acpi_get_local_address() may be renamed if desired in a
> folow-up patch.
> 
> The path of least resistance would be to merge this patchset in the
> ASoC tree, since I have additional changes for ASoC/SDCA (SoundWire
> Device Class) that depend on the new helper.
> 
> [...]

Applied to

   https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git for-next

Thanks!

[1/3] ACPI: utils: introduce acpi_get_local_u64_address()
      commit: 0b7e448119428e1dcb854abb5855f66966fb82dc
[2/3] soundwire: slave: simplify code with acpi_get_local_u64_address()
      commit: b6212f9bf489daf9716aed0e8c4dc6a807ce839f
[3/3] ALSA: hda: intel-sdw-acpi: use acpi_get_local_u64_address()
      commit: 9b7dc68eeba04d20f4a1733e791bc71355423612

All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.

You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.

If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.

Please add any relevant lists and maintainers to the CCs when replying
to this mail.

Thanks,
Mark