diff mbox series

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

Message ID 20240528192936.16180-2-pierre-louis.bossart@linux.intel.com (mailing list archive)
State Handled Elsewhere, archived
Headers show
Series [1/3] ACPI: utils: introduce acpi_get_local_u64_address() | expand

Commit Message

Pierre-Louis Bossart May 28, 2024, 7:29 p.m. UTC
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(-)

Comments

Rafael J. Wysocki June 7, 2024, 6:51 p.m. UTC | #1
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);
>
> --
Pierre-Louis Bossart June 7, 2024, 8:33 p.m. UTC | #2
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);
Mark Brown June 7, 2024, 9:59 p.m. UTC | #3
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.
Rafael J. Wysocki June 8, 2024, 11:58 a.m. UTC | #4
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.
diff mbox series

Patch

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);