[PATCH 0/4] ACPI: NHLT: Access and query helpers

[PATCH 0/4] ACPI: NHLT: Access and query helpers

[Cezary Rojewski]

The goal of this patchset is to enhance existing interface of
NonHDAudioLinkTable (NHLT), that is, accessing device and format
configuration spaces as well as adding query functions for finding
specific endpoints and format configurations. Once that is done,
existing sound-drivers can move from utilizing sound/hda/intel-nhlt.c
to this very code and ultimately the former file can be removed.

It's important to highlight that currently presented implementation is
not final and intention is to first transfer sound-driver to new API and
then provide incremental updates to internal code in drivers/acpi/nhlt.c
to improve reliability and safety.

Series starts with addition of devcfg-access helpers. The main reasoning
for adding access function is inability for a user to predict whether
given space (device config) is valid or not. While inheritance in the
specification is allowed, e.g.: mic_devcfg being inherited by
mic_vendor_devcfg, until size is verified, one shall not be accessing
fields which are not guaranteed by the spec. The only field guaranteed
is "capabilities_size".
The xxx_devcfg structs added here kind of duplicate few existing ones
in actbl2.h. This is mainly motivated by usage improvements -
simplicity, shorten wording. Intention is to have them replacing
existing actbl2.h members in the future.

Follow up is the declaration of acpi_gbl_NHLT. Motivation is to make
sound-drivers life easier i.e.: release them from storing pointer to the
first NHLT in the system internally. Such drivers may utilize
acpi_gbl_NHLT when querying endpoints and formats instead.

Table manipulation functions and macros serve as a base for the
follow-up query functions. So called query functions represent standard
operations performed by user (a sound driver) when attempting to open an
audio stream. These more or less mimic what's present in
sound/hda/intel-nhlt.c.

Cezary Rojewski (4):
  ACPI: NHLT: Device configuration access interface
  ACPI: NHLT: Introduce acpi_gbl_NHLT
  ACPI: NHLT: Table manipulation helpers
  ACPI: NHLT: Add query functions

 drivers/acpi/Kconfig  |   3 +
 drivers/acpi/Makefile |   1 +
 drivers/acpi/nhlt.c   | 196 ++++++++++++++++++++++++++++++++++++++++
 include/acpi/actbl2.h |  28 ++++++
 include/acpi/nhlt.h   | 206 ++++++++++++++++++++++++++++++++++++++++++
 5 files changed, 434 insertions(+)
 create mode 100644 drivers/acpi/nhlt.c
 create mode 100644 include/acpi/nhlt.h

-- 
2.25.1

[PATCH 1/4] ACPI: NHLT: Device configuration access interface

[Cezary Rojewski]

Device configuration structures are plenty so declare a struct for each
known variant. As neither of them shall be accessed without verifying
the memory block first, introduce macros to make it easy to do so.

Link: https://github.com/acpica/acpica/pull/881
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Reviewed-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Signed-off-by: Cezary Rojewski <cezary.rojewski@intel.com>
---
 include/acpi/actbl2.h | 28 ++++++++++++++++++
 include/acpi/nhlt.h   | 66 +++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 94 insertions(+)
 create mode 100644 include/acpi/nhlt.h

diff --git a/include/acpi/actbl2.h b/include/acpi/actbl2.h
index 0029336775a9..c4c9a3a89ba6 100644
--- a/include/acpi/actbl2.h
+++ b/include/acpi/actbl2.h
@@ -2014,6 +2014,25 @@ struct acpi_nhlt_vendor_mic_count {
 	u8 microphone_count;
 };
 
+/* The only guaranteed configuration space header. Any other requires validation. */
+struct acpi_nhlt_cfg {
+	u32 capabilities_size;
+	u8 capabilities[];
+};
+
+struct acpi_nhlt_devcfg {
+	u32 capabilities_size;
+	u8 virtual_slot;
+	u8 config_type;
+};
+
+struct acpi_nhlt_mic_devcfg {
+	u32 capabilities_size;
+	u8 virtual_slot;
+	u8 config_type;
+	u8 array_type;
+};
+
 struct acpi_nhlt_vendor_mic_config {
 	u8 type;
 	u8 panel;
@@ -2030,6 +2049,15 @@ struct acpi_nhlt_vendor_mic_config {
 	u16 work_horizontal_angle_end;	/* -180 - + 180 with 2 deg step */
 };
 
+struct acpi_nhlt_vendor_mic_devcfg {
+	u32 capabilities_size;
+	u8 virtual_slot;
+	u8 config_type;
+	u8 array_type;
+	u8 num_mics;
+	struct acpi_nhlt_vendor_mic_config mics[];
+};
+
 /* Values for Type field above */
 
 #define ACPI_NHLT_MIC_OMNIDIRECTIONAL       0
diff --git a/include/acpi/nhlt.h b/include/acpi/nhlt.h
new file mode 100644
index 000000000000..af3ec45ba4f9
--- /dev/null
+++ b/include/acpi/nhlt.h
@@ -0,0 +1,66 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+/*
+ * Copyright(c) 2023 Intel Corporation. All rights reserved.
+ *
+ * Authors: Cezary Rojewski <cezary.rojewski@intel.com>
+ *          Amadeusz Slawinski <amadeuszx.slawinski@linux.intel.com>
+ */
+
+#ifndef __ACPI_NHLT_H__
+#define __ACPI_NHLT_H__
+
+#include <linux/acpi.h>
+#include <linux/overflow.h>
+#include <linux/types.h>
+
+#define __acpi_nhlt_endpoint_cfg(ep)	((void *)((ep) + 1))
+
+/*
+ * As device configuration spaces present in NHLT tables around the world are
+ * not following single pattern, first check if 'capabilities_size' is correct
+ * in respect to size of the specified space type before returning the pointer.
+ */
+#define __acpi_nhlt_endpoint_devcfg(ep, type) ({				\
+	struct acpi_nhlt_cfg *__cfg = __acpi_nhlt_endpoint_cfg(ep);		\
+	__cfg->capabilities_size >= sizeof(type) ?				\
+		((type *)__cfg) : NULL; })
+
+/*
+ * acpi_nhlt_endpoint_devcfg - Test and access device configuration.
+ * @ep:		endpoint for which to retrieve device configuration.
+ *
+ * Return: A pointer to device configuration space or NULL if the space's
+ * 'capabilities_size' is insufficient to cover the nested structure.
+ */
+#define acpi_nhlt_endpoint_devcfg(ep) \
+	__acpi_nhlt_endpoint_devcfg(ep, struct acpi_nhlt_devcfg)
+
+/*
+ * acpi_nhlt_endpoint_mic_devcfg - Test and access device configuration.
+ * @ep:		endpoint for which to retrieve device configuration.
+ *
+ * Return: A pointer to device configuration space or NULL if the space's
+ * 'capabilities_size' is insufficient to cover the nested structure.
+ */
+#define acpi_nhlt_endpoint_mic_devcfg(ep) \
+	__acpi_nhlt_endpoint_devcfg(ep, struct acpi_nhlt_mic_devcfg)
+
+/*
+ * acpi_nhlt_endpoint_vendor_mic_devcfg - Test and access device configuration.
+ * @ep:		endpoint for which to retrieve device configuration.
+ * @ptr:	pointer to a device configuration structure.
+ *
+ * This is the same as acpi_nhlt_endpoint_devcfg(), except that it verifies
+ * if size of the flexible array following the structure header is also
+ * reflected in 'capabilities_size'.
+ *
+ * Return: A pointer to device configuration space or NULL if the space's
+ * 'capabilities_size' is insufficient to cover the nested structure.
+ */
+#define acpi_nhlt_endpoint_vendor_mic_devcfg(ep) ({					\
+	struct acpi_nhlt_vendor_mic_devcfg *__cfg = __acpi_nhlt_endpoint_cfg(ep);	\
+	__cfg->capabilities_size >= sizeof(*__cfg) &&					\
+	__cfg->capabilities_size == struct_size(__cfg, mics, __cfg->num_mics) ?		\
+		__cfg : NULL; })
+
+#endif /* __ACPI_NHLT_H__ */
-- 
2.25.1

[PATCH 2/4] ACPI: NHLT: Introduce acpi_gbl_NHLT

[Cezary Rojewski]

While there is no strict limit to amount of NHLT tables present, usually
just the first one is utilized. To simplify implementation of sound
drivers, provide publicly accessible pointer. Accessing it after calling
acpi_nhlt_get_gbl_table() yields the first NHLT table met during the
scan.

Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Reviewed-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Signed-off-by: Cezary Rojewski <cezary.rojewski@intel.com>
---
 drivers/acpi/Kconfig  |  3 +++
 drivers/acpi/Makefile |  1 +
 drivers/acpi/nhlt.c   | 13 +++++++++++++
 include/acpi/nhlt.h   | 18 ++++++++++++++++++
 4 files changed, 35 insertions(+)
 create mode 100644 drivers/acpi/nhlt.c

diff --git a/drivers/acpi/Kconfig b/drivers/acpi/Kconfig
index ccbeab9500ec..01ce5d3533db 100644
--- a/drivers/acpi/Kconfig
+++ b/drivers/acpi/Kconfig
@@ -594,6 +594,9 @@ config ACPI_PRMT
 	  substantially increase computational overhead related to the
 	  initialization of some server systems.
 
+config ACPI_NHLT
+	bool
+
 endif	# ACPI
 
 config X86_PM_TIMER
diff --git a/drivers/acpi/Makefile b/drivers/acpi/Makefile
index feb36c0b9446..8de34970e7db 100644
--- a/drivers/acpi/Makefile
+++ b/drivers/acpi/Makefile
@@ -93,6 +93,7 @@ obj-$(CONFIG_ACPI)		+= container.o
 obj-$(CONFIG_ACPI_THERMAL)	+= thermal.o
 obj-$(CONFIG_ACPI_PLATFORM_PROFILE) 	+= platform_profile.o
 obj-$(CONFIG_ACPI_NFIT)		+= nfit/
+obj-$(CONFIG_ACPI_NHLT)		+= nhlt.o
 obj-$(CONFIG_ACPI_NUMA)		+= numa/
 obj-$(CONFIG_ACPI)		+= acpi_memhotplug.o
 obj-$(CONFIG_ACPI_HOTPLUG_IOAPIC) += ioapic.o
diff --git a/drivers/acpi/nhlt.c b/drivers/acpi/nhlt.c
new file mode 100644
index 000000000000..90d74d0d803e
--- /dev/null
+++ b/drivers/acpi/nhlt.c
@@ -0,0 +1,13 @@
+// SPDX-License-Identifier: GPL-2.0-only
+//
+// Copyright(c) 2023 Intel Corporation. All rights reserved.
+//
+// Authors: Cezary Rojewski <cezary.rojewski@intel.com>
+//          Amadeusz Slawinski <amadeuszx.slawinski@linux.intel.com>
+//
+
+#include <linux/export.h>
+#include <acpi/nhlt.h>
+
+struct acpi_table_nhlt *acpi_gbl_NHLT;
+EXPORT_SYMBOL_GPL(acpi_gbl_NHLT);
diff --git a/include/acpi/nhlt.h b/include/acpi/nhlt.h
index af3ec45ba4f9..a2b93b08218f 100644
--- a/include/acpi/nhlt.h
+++ b/include/acpi/nhlt.h
@@ -13,6 +13,24 @@
 #include <linux/overflow.h>
 #include <linux/types.h>
 
+/* System-wide pointer to the first NHLT table. */
+extern struct acpi_table_nhlt *acpi_gbl_NHLT;
+
+/*
+ * A sound driver may utilize the two below on its initialization and removal
+ * respectively to avoid excessive mapping and unmapping of the memory
+ * occupied by the table between streaming operations.
+ */
+static inline acpi_status acpi_nhlt_get_gbl_table(void)
+{
+	return acpi_get_table(ACPI_SIG_NHLT, 0, (struct acpi_table_header **)(&acpi_gbl_NHLT));
+}
+
+static inline void acpi_nhlt_put_gbl_table(void)
+{
+	acpi_put_table((struct acpi_table_header *)acpi_gbl_NHLT);
+}
+
 #define __acpi_nhlt_endpoint_cfg(ep)	((void *)((ep) + 1))
 
 /*
-- 
2.25.1

[PATCH 3/4] ACPI: NHLT: Table manipulation helpers

[Cezary Rojewski]

The table is composed of a range of endpoints with each describing
audio formats they support. Thus most of the operations involve
iterating over elements of the table. Simplify the process by
implementing range of getters.

Reviewed-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Signed-off-by: Cezary Rojewski <cezary.rojewski@intel.com>
---
 include/acpi/nhlt.h | 68 +++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 68 insertions(+)

diff --git a/include/acpi/nhlt.h b/include/acpi/nhlt.h
index a2b93b08218f..076aac41a74e 100644
--- a/include/acpi/nhlt.h
+++ b/include/acpi/nhlt.h
@@ -81,4 +81,72 @@ static inline void acpi_nhlt_put_gbl_table(void)
 	__cfg->capabilities_size == struct_size(__cfg, mics, __cfg->num_mics) ?		\
 		__cfg : NULL; })
 
+/**
+ * acpi_nhlt_endpoint_fmtscfg - Get the formats configuration space.
+ * @ep:		the endpoint to retrieve the space for.
+ *
+ * Return: A pointer to the formats configuration space.
+ */
+static inline struct acpi_nhlt_formats_config *
+acpi_nhlt_endpoint_fmtscfg(const struct acpi_nhlt_endpoint *ep)
+{
+	struct acpi_nhlt_cfg *cfg = __acpi_nhlt_endpoint_cfg(ep);
+
+	return (struct acpi_nhlt_formats_config *)((u8 *)(cfg + 1) + cfg->capabilities_size);
+}
+
+#define __acpi_nhlt_first_endpoint(tb) \
+	((void *)(tb + 1))
+
+#define __acpi_nhlt_next_endpoint(ep) \
+	((void *)((u8 *)(ep) + (ep)->descriptor_length))
+
+#define __acpi_nhlt_get_endpoint(tb, ep, i) \
+	((i) ? __acpi_nhlt_next_endpoint(ep) : __acpi_nhlt_first_endpoint(tb))
+
+#define __acpi_nhlt_first_fmtcfg(fmts) \
+	((void *)(fmts + 1))
+
+#define __acpi_nhlt_next_fmtcfg(fmt) \
+	((void *)((u8 *)((fmt) + 1) + (fmt)->capability_size))
+
+#define __acpi_nhlt_get_fmtcfg(fmts, fmt, i) \
+	((i) ? __acpi_nhlt_next_fmtcfg(fmt) : __acpi_nhlt_first_fmtcfg(fmts))
+
+/*
+ * The for_each_nhlt_xxx() macros rely on an iterator to deal with the
+ * variable length of each endpoint structure and the possible presence
+ * of an OED-Config used by Windows only.
+ */
+
+/**
+ * for_each_nhlt_endpoint - Iterate over endpoints in a NHLT table.
+ * @tb:		the pointer to a NHLT table.
+ * @ep:		the pointer to endpoint to use as loop cursor.
+ */
+#define for_each_nhlt_endpoint(tb, ep)					\
+	for (unsigned int __i = 0;					\
+	     __i < (tb)->endpoint_count &&				\
+		((ep) = __acpi_nhlt_get_endpoint(tb, ep, __i));		\
+	     __i++)
+
+/**
+ * for_each_nhlt_fmtcfg - Iterate over format configurations.
+ * @fmts:	the pointer to formats configuration space.
+ * @fmt:	the pointer to format to use as loop cursor.
+ */
+#define for_each_nhlt_fmtcfg(fmts, fmt)					\
+	for (unsigned int __i = 0;					\
+	     __i < (fmts)->formats_count &&				\
+		((fmt) = __acpi_nhlt_get_fmtcfg(fmts, fmt, __i));	\
+	     __i++)
+
+/**
+ * for_each_nhlt_endpoint_fmtcfg - Iterate over format configurations in an endpoint.
+ * @ep:		the pointer to an endpoint.
+ * @fmt:	the pointer to format to use as loop cursor.
+ */
+#define for_each_nhlt_endpoint_fmtcfg(ep, fmt) \
+	for_each_nhlt_fmtcfg(acpi_nhlt_endpoint_fmtscfg(ep), fmt)
+
 #endif /* __ACPI_NHLT_H__ */
-- 
2.25.1

Re: [PATCH 3/4] ACPI: NHLT: Table manipulation helpers

[Andy Shevchenko]

On Wed, Jul 12, 2023 at 11:10:47AM +0200, Cezary Rojewski wrote:
> The table is composed of a range of endpoints with each describing
> audio formats they support. Thus most of the operations involve
> iterating over elements of the table. Simplify the process by
> implementing range of getters.

A few nit-picks below.
In general, LGTM,
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

(Please, use my @linux.intel.com address for LKML and related)

> Reviewed-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
> Signed-off-by: Cezary Rojewski <cezary.rojewski@intel.com>
> ---
>  include/acpi/nhlt.h | 68 +++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 68 insertions(+)
> 
> diff --git a/include/acpi/nhlt.h b/include/acpi/nhlt.h
> index a2b93b08218f..076aac41a74e 100644
> --- a/include/acpi/nhlt.h
> +++ b/include/acpi/nhlt.h
> @@ -81,4 +81,72 @@ static inline void acpi_nhlt_put_gbl_table(void)
>  	__cfg->capabilities_size == struct_size(__cfg, mics, __cfg->num_mics) ?		\
>  		__cfg : NULL; })
>  
> +/**
> + * acpi_nhlt_endpoint_fmtscfg - Get the formats configuration space.
> + * @ep:		the endpoint to retrieve the space for.
> + *
> + * Return: A pointer to the formats configuration space.
> + */
> +static inline struct acpi_nhlt_formats_config *
> +acpi_nhlt_endpoint_fmtscfg(const struct acpi_nhlt_endpoint *ep)
> +{
> +	struct acpi_nhlt_cfg *cfg = __acpi_nhlt_endpoint_cfg(ep);
> +
> +	return (struct acpi_nhlt_formats_config *)((u8 *)(cfg + 1) + cfg->capabilities_size);
> +}
> +
> +#define __acpi_nhlt_first_endpoint(tb) \
> +	((void *)(tb + 1))
> +
> +#define __acpi_nhlt_next_endpoint(ep) \
> +	((void *)((u8 *)(ep) + (ep)->descriptor_length))
> +
> +#define __acpi_nhlt_get_endpoint(tb, ep, i) \
> +	((i) ? __acpi_nhlt_next_endpoint(ep) : __acpi_nhlt_first_endpoint(tb))
> +
> +#define __acpi_nhlt_first_fmtcfg(fmts) \
> +	((void *)(fmts + 1))
> +
> +#define __acpi_nhlt_next_fmtcfg(fmt) \
> +	((void *)((u8 *)((fmt) + 1) + (fmt)->capability_size))
> +
> +#define __acpi_nhlt_get_fmtcfg(fmts, fmt, i) \
> +	((i) ? __acpi_nhlt_next_fmtcfg(fmt) : __acpi_nhlt_first_fmtcfg(fmts))
> +
> +/*
> + * The for_each_nhlt_xxx() macros rely on an iterator to deal with the

I would do s/xxx/*/

> + * variable length of each endpoint structure and the possible presence
> + * of an OED-Config used by Windows only.
> + */
> +
> +/**
> + * for_each_nhlt_endpoint - Iterate over endpoints in a NHLT table.
> + * @tb:		the pointer to a NHLT table.
> + * @ep:		the pointer to endpoint to use as loop cursor.
> + */
> +#define for_each_nhlt_endpoint(tb, ep)					\
> +	for (unsigned int __i = 0;					\
> +	     __i < (tb)->endpoint_count &&				\
> +		((ep) = __acpi_nhlt_get_endpoint(tb, ep, __i));		\

Do you really need ep to be in parentheses?

> +	     __i++)
> +
> +/**
> + * for_each_nhlt_fmtcfg - Iterate over format configurations.
> + * @fmts:	the pointer to formats configuration space.
> + * @fmt:	the pointer to format to use as loop cursor.
> + */
> +#define for_each_nhlt_fmtcfg(fmts, fmt)					\
> +	for (unsigned int __i = 0;					\
> +	     __i < (fmts)->formats_count &&				\
> +		((fmt) = __acpi_nhlt_get_fmtcfg(fmts, fmt, __i));	\

Similar for fmt.

> +	     __i++)
> +
> +/**
> + * for_each_nhlt_endpoint_fmtcfg - Iterate over format configurations in an endpoint.
> + * @ep:		the pointer to an endpoint.
> + * @fmt:	the pointer to format to use as loop cursor.
> + */
> +#define for_each_nhlt_endpoint_fmtcfg(ep, fmt) \
> +	for_each_nhlt_fmtcfg(acpi_nhlt_endpoint_fmtscfg(ep), fmt)
> +
>  #endif /* __ACPI_NHLT_H__ */
> -- 
> 2.25.1
> 

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 3/4] ACPI: NHLT: Table manipulation helpers

[Cezary Rojewski]

On 2023-07-12 5:36 PM, Andy Shevchenko wrote:
> On Wed, Jul 12, 2023 at 11:10:47AM +0200, Cezary Rojewski wrote:
>> The table is composed of a range of endpoints with each describing
>> audio formats they support. Thus most of the operations involve
>> iterating over elements of the table. Simplify the process by
>> implementing range of getters.
> 
> A few nit-picks below.
> In general, LGTM,
> Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> 
> (Please, use my @linux.intel.com address for LKML and related)

...

>> +/*
>> + * The for_each_nhlt_xxx() macros rely on an iterator to deal with the
> 
> I would do s/xxx/*/

Ack.

>> + * variable length of each endpoint structure and the possible presence
>> + * of an OED-Config used by Windows only.
>> + */
>> +
>> +/**
>> + * for_each_nhlt_endpoint - Iterate over endpoints in a NHLT table.
>> + * @tb:		the pointer to a NHLT table.
>> + * @ep:		the pointer to endpoint to use as loop cursor.
>> + */
>> +#define for_each_nhlt_endpoint(tb, ep)					\
>> +	for (unsigned int __i = 0;					\
>> +	     __i < (tb)->endpoint_count &&				\
>> +		((ep) = __acpi_nhlt_get_endpoint(tb, ep, __i));		\
> 
> Do you really need ep to be in parentheses?

Agree. Also, checked how include/linux/list.h looks like and it aligns 
with your proposal.

>> +	     __i++)
>> +
>> +/**
>> + * for_each_nhlt_fmtcfg - Iterate over format configurations.
>> + * @fmts:	the pointer to formats configuration space.
>> + * @fmt:	the pointer to format to use as loop cursor.
>> + */
>> +#define for_each_nhlt_fmtcfg(fmts, fmt)					\
>> +	for (unsigned int __i = 0;					\
>> +	     __i < (fmts)->formats_count &&				\
>> +		((fmt) = __acpi_nhlt_get_fmtcfg(fmts, fmt, __i));	\
> 
> Similar for fmt.

Ditto.

>> +	     __i++)

[PATCH 4/4] ACPI: NHLT: Add query functions

[Cezary Rojewski]

With iteration helpers added there is a room for more complex query
tasks which are commonly performed by sound drivers. Implement them in
common API so that a unified mechanism is available for all of them.

While the acpi_nhlt_endpoint_dmic_count() stands out a bit, it is a
critical component for any AudioDSP driver to know how many digital
microphones it is dealing with. There is no one perfect method, but the
best one available is provided.

Reviewed-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Signed-off-by: Cezary Rojewski <cezary.rojewski@intel.com>
---
 drivers/acpi/nhlt.c | 183 ++++++++++++++++++++++++++++++++++++++++++++
 include/acpi/nhlt.h |  54 +++++++++++++
 2 files changed, 237 insertions(+)

diff --git a/drivers/acpi/nhlt.c b/drivers/acpi/nhlt.c
index 90d74d0d803e..c61cdfd78b74 100644
--- a/drivers/acpi/nhlt.c
+++ b/drivers/acpi/nhlt.c
@@ -6,8 +6,191 @@
 //          Amadeusz Slawinski <amadeuszx.slawinski@linux.intel.com>
 //
 
+#define pr_fmt(fmt) "ACPI: NHLT: " fmt
+
 #include <linux/export.h>
 #include <acpi/nhlt.h>
 
 struct acpi_table_nhlt *acpi_gbl_NHLT;
 EXPORT_SYMBOL_GPL(acpi_gbl_NHLT);
+
+/**
+ * acpi_nhlt_endpoint_match - Verify if an endpoint matches criteria.
+ * @ep:			the endpoint to check.
+ * @link_type:		the hardware link type, e.g.: PDM or SSP.
+ * @dev_type:		the device type.
+ * @dir:		stream direction.
+ * @bus_id:		the ID of virtual bus hosting the endpoint.
+ *
+ * Either of @link_type, @dev_type, @dir or @bus_id may be set to a negative
+ * value to ignore the parameter when matching.
+ *
+ * Return: %true if endpoint matches specified criteria or %false otherwise.
+ */
+bool acpi_nhlt_endpoint_match(const struct acpi_nhlt_endpoint *ep,
+			      int link_type, int dev_type, int dir, int bus_id)
+{
+	return ep &&
+	       (link_type < 0 || ep->link_type == link_type) &&
+	       (dev_type < 0 || ep->device_type == dev_type) &&
+	       (dir < 0 || ep->direction == dir) &&
+	       (bus_id < 0 || ep->virtual_bus_id == bus_id);
+}
+EXPORT_SYMBOL_GPL(acpi_nhlt_endpoint_match);
+
+/**
+ * acpi_nhlt_find_endpoint - Search a NHLT table for an endpoint.
+ * @tb:			the table to search.
+ * @link_type:		the hardware link type, e.g.: PDM or SSP.
+ * @dev_type:		the device type.
+ * @dir:		stream direction.
+ * @bus_id:		the ID of virtual bus hosting the endpoint.
+ *
+ * Either of @link_type, @dev_type, @dir or @bus_id may be set to a negative
+ * value to ignore the parameter during the search.
+ *
+ * Return: A pointer to endpoint matching the criteria, %NULL if not found or
+ * an ERR_PTR() otherwise.
+ */
+struct acpi_nhlt_endpoint *
+acpi_nhlt_find_endpoint(const struct acpi_table_nhlt *tb,
+			int link_type, int dev_type, int dir, int bus_id)
+{
+	struct acpi_nhlt_endpoint *ep;
+
+	if (!tb)
+		return ERR_PTR(-EINVAL);
+
+	for_each_nhlt_endpoint(tb, ep)
+		if (acpi_nhlt_endpoint_match(ep, link_type, dev_type, dir, bus_id))
+			return ep;
+	return NULL;
+}
+EXPORT_SYMBOL_GPL(acpi_nhlt_find_endpoint);
+
+/**
+ * acpi_nhlt_endpoint_find_fmtcfg - Search endpoint's formats configuration space
+ *                                  for a specific format.
+ * @ep:			the endpoint to search.
+ * @ch:			number of channels.
+ * @rate:		samples per second.
+ * @vbps:		valid bits per sample.
+ * @bps:		bits per sample.
+ *
+ * Return: A pointer to format matching the criteria, %NULL if not found or
+ * an ERR_PTR() otherwise.
+ */
+struct acpi_nhlt_format_config *
+acpi_nhlt_endpoint_find_fmtcfg(const struct acpi_nhlt_endpoint *ep,
+			       u16 ch, u32 rate, u16 vbps, u16 bps)
+{
+	struct acpi_nhlt_wave_extensible *wav;
+	struct acpi_nhlt_format_config *fmt;
+
+	if (!ep)
+		return ERR_PTR(-EINVAL);
+
+	for_each_nhlt_endpoint_fmtcfg(ep, fmt) {
+		wav = &fmt->format;
+
+		if (wav->channel_count == ch &&
+		    wav->valid_bits_per_sample == vbps &&
+		    wav->bits_per_sample == bps &&
+		    wav->samples_per_sec == rate)
+			return fmt;
+	}
+
+	return NULL;
+}
+EXPORT_SYMBOL_GPL(acpi_nhlt_endpoint_find_fmtcfg);
+
+/**
+ * acpi_nhlt_find_fmtcfg - Search a NHLT table for a specific format.
+ * @tb:			the table to search.
+ * @link_type:		the hardware link type, e.g.: PDM or SSP.
+ * @dev_type:		the device type.
+ * @dir:		stream direction.
+ * @bus_id:		the ID of virtual bus hosting the endpoint.
+ *
+ * @ch:			number of channels.
+ * @rate:		samples per second.
+ * @vbps:		valid bits per sample.
+ * @bps:		bits per sample.
+ *
+ * Either of @link_type, @dev_type, @dir or @bus_id may be set to a negative
+ * value to ignore the parameter during the search.
+ *
+ * Return: A pointer to format matching the criteria, %NULL if not found or
+ * an ERR_PTR() otherwise.
+ */
+struct acpi_nhlt_format_config *
+acpi_nhlt_find_fmtcfg(const struct acpi_table_nhlt *tb,
+		      int link_type, int dev_type, int dir, int bus_id,
+		      u16 ch, u32 rate, u16 vbps, u16 bps)
+{
+	struct acpi_nhlt_format_config *fmt;
+	struct acpi_nhlt_endpoint *ep;
+
+	if (!tb)
+		return ERR_PTR(-EINVAL);
+
+	for_each_nhlt_endpoint(tb, ep) {
+		if (!acpi_nhlt_endpoint_match(ep, link_type, dev_type, dir, bus_id))
+			continue;
+
+		fmt = acpi_nhlt_endpoint_find_fmtcfg(ep, ch, rate, vbps, bps);
+		if (fmt)
+			return fmt;
+	}
+
+	return NULL;
+}
+EXPORT_SYMBOL_GPL(acpi_nhlt_find_fmtcfg);
+
+/**
+ * acpi_nhlt_endpoint_dmic_count - Retrieve number of digital microphones for a PDM endpoint.
+ * @ep:			the endpoint to return microphones count for.
+ *
+ * Return: A number of microphones or an error code if an invalid endpoint is provided.
+ */
+int acpi_nhlt_endpoint_dmic_count(const struct acpi_nhlt_endpoint *ep)
+{
+	struct acpi_nhlt_vendor_mic_devcfg *vendor_cfg;
+	struct acpi_nhlt_format_config *fmt;
+	struct acpi_nhlt_mic_devcfg *devcfg;
+	u16 max_ch = 0;
+
+	if (!ep || ep->link_type != ACPI_NHLT_PDM)
+		return -EINVAL;
+
+	/* Find max number of channels based on formats configuration. */
+	for_each_nhlt_endpoint_fmtcfg(ep, fmt)
+		max_ch = max(fmt->format.channel_count, max_ch);
+
+	/* If @ep not a mic array, fallback to channels count. */
+	devcfg = acpi_nhlt_endpoint_mic_devcfg(ep);
+	if (!devcfg || devcfg->config_type != ACPI_NHLT_CONFIG_TYPE_MIC_ARRAY)
+		return max_ch;
+
+	switch (devcfg->array_type) {
+	case ACPI_NHLT_SMALL_LINEAR_2ELEMENT:
+	case ACPI_NHLT_BIG_LINEAR_2ELEMENT:
+		return 2;
+
+	case ACPI_NHLT_FIRST_GEOMETRY_LINEAR_4ELEMENT:
+	case ACPI_NHLT_PLANAR_LSHAPED_4ELEMENT:
+	case ACPI_NHLT_SECOND_GEOMETRY_LINEAR_4ELEMENT:
+		return 4;
+
+	case ACPI_NHLT_VENDOR_DEFINED:
+		vendor_cfg = acpi_nhlt_endpoint_vendor_mic_devcfg(ep);
+		if (!vendor_cfg)
+			return -EINVAL;
+		return vendor_cfg->num_mics;
+
+	default:
+		pr_warn("undefined mic array type: %#x\n", devcfg->array_type);
+		return max_ch;
+	}
+}
+EXPORT_SYMBOL_GPL(acpi_nhlt_endpoint_dmic_count);
diff --git a/include/acpi/nhlt.h b/include/acpi/nhlt.h
index 076aac41a74e..ba093fe871d5 100644
--- a/include/acpi/nhlt.h
+++ b/include/acpi/nhlt.h
@@ -149,4 +149,58 @@ acpi_nhlt_endpoint_fmtscfg(const struct acpi_nhlt_endpoint *ep)
 #define for_each_nhlt_endpoint_fmtcfg(ep, fmt) \
 	for_each_nhlt_fmtcfg(acpi_nhlt_endpoint_fmtscfg(ep), fmt)
 
+#if IS_ENABLED(CONFIG_ACPI_NHLT)
+
+bool acpi_nhlt_endpoint_match(const struct acpi_nhlt_endpoint *ep,
+			      int link_type, int dev_type, int dir, int bus_id);
+struct acpi_nhlt_endpoint *
+acpi_nhlt_find_endpoint(const struct acpi_table_nhlt *tb,
+			int link_type, int dev_type, int dir, int bus_id);
+struct acpi_nhlt_format_config *
+acpi_nhlt_endpoint_find_fmtcfg(const struct acpi_nhlt_endpoint *ep,
+			       u16 ch, u32 rate, u16 vbps, u16 bps);
+struct acpi_nhlt_format_config *
+acpi_nhlt_find_fmtcfg(const struct acpi_table_nhlt *tb,
+		      int link_type, int dev_type, int dir, int bus_id,
+		      u16 ch, u32 rate, u16 vpbs, u16 bps);
+int acpi_nhlt_endpoint_dmic_count(const struct acpi_nhlt_endpoint *ep);
+
+#else /* !CONFIG_ACPI_NHLT */
+
+static bool
+acpi_nhlt_endpoint_match(const struct acpi_nhlt_endpoint *ep,
+			 int link_type, int dev_type, int dir, int bus_id)
+{
+	return false;
+}
+
+static inline struct acpi_nhlt_endpoint *
+acpi_nhlt_find_endpoint(const struct acpi_table_nhlt *tb,
+			int link_type, int dev_type, int dir, int bus_id)
+{
+	return NULL;
+}
+
+static inline struct acpi_nhlt_format_config *
+acpi_nhlt_endpoint_find_fmtcfg(const struct acpi_nhlt_endpoint *ep,
+			       u16 ch, u32 rate, u16 vbps, u16 bps)
+{
+	return NULL;
+}
+
+static inline struct acpi_nhlt_format_config *
+acpi_nhlt_find_fmtcfg(const struct acpi_table_nhlt *tb,
+		      int link_type, int dev_type, int dir, int bus_id,
+		      u16 ch, u32 rate, u16 vpbs, u16 bps);
+{
+	return NULL;
+}
+
+static inline int acpi_nhlt_endpoint_dmic_count(const struct acpi_nhlt_endpoint *ep)
+{
+	return 0;
+}
+
+#endif /* !CONFIG_ACPI_NHLT */
+
 #endif /* __ACPI_NHLT_H__ */
-- 
2.25.1

Re: [PATCH 4/4] ACPI: NHLT: Add query functions

[Andy Shevchenko]

On Wed, Jul 12, 2023 at 11:10:48AM +0200, Cezary Rojewski wrote:
> With iteration helpers added there is a room for more complex query
> tasks which are commonly performed by sound drivers. Implement them in
> common API so that a unified mechanism is available for all of them.
> 
> While the acpi_nhlt_endpoint_dmic_count() stands out a bit, it is a
> critical component for any AudioDSP driver to know how many digital
> microphones it is dealing with. There is no one perfect method, but the
> best one available is provided.

...

> +bool acpi_nhlt_endpoint_match(const struct acpi_nhlt_endpoint *ep,
> +			      int link_type, int dev_type, int dir, int bus_id)
> +{
> +	return ep &&
> +	       (link_type < 0 || ep->link_type == link_type) &&
> +	       (dev_type < 0 || ep->device_type == dev_type) &&
> +	       (dir < 0 || ep->direction == dir) &&
> +	       (bus_id < 0 || ep->virtual_bus_id == bus_id);

I think you can split these for better reading.

	if (!ep)
		return false;

	if (link_type >= 0 && ep->link_type != link_type)
		return false;

	if (dev_type >= 0 && ep->device_type != dev_type)
		return false;

	if (dir >= 0 && ep->direction != dir)
		return false;

	if (bus_id >= 0 && ep->virtual_bus_id != bus_id)
		return false;

	return true;

Yes, much more lines, but readability is better in my opinion.
I also believe that code generation on x86_64 will be the same.

> +}

...

> +struct acpi_nhlt_endpoint *
> +acpi_nhlt_find_endpoint(const struct acpi_table_nhlt *tb,
> +			int link_type, int dev_type, int dir, int bus_id)
> +{
> +	struct acpi_nhlt_endpoint *ep;

> +	if (!tb)
> +		return ERR_PTR(-EINVAL);

Just wondering how often we will have a caller that supplies NULL for tb.

> +	for_each_nhlt_endpoint(tb, ep)
> +		if (acpi_nhlt_endpoint_match(ep, link_type, dev_type, dir, bus_id))
> +			return ep;
> +	return NULL;
> +}

...

> +struct acpi_nhlt_format_config *
> +acpi_nhlt_endpoint_find_fmtcfg(const struct acpi_nhlt_endpoint *ep,
> +			       u16 ch, u32 rate, u16 vbps, u16 bps)
> +{
> +	struct acpi_nhlt_wave_extensible *wav;
> +	struct acpi_nhlt_format_config *fmt;

> +	if (!ep)
> +		return ERR_PTR(-EINVAL);

Similar Q as above.

> +	for_each_nhlt_endpoint_fmtcfg(ep, fmt) {
> +		wav = &fmt->format;
> +
> +		if (wav->channel_count == ch &&
> +		    wav->valid_bits_per_sample == vbps &&
> +		    wav->bits_per_sample == bps &&
> +		    wav->samples_per_sec == rate)

Also can be split, but this one readable in the original form.

> +			return fmt;
> +	}
> +
> +	return NULL;
> +}

...

> +struct acpi_nhlt_format_config *
> +acpi_nhlt_find_fmtcfg(const struct acpi_table_nhlt *tb,
> +		      int link_type, int dev_type, int dir, int bus_id,
> +		      u16 ch, u32 rate, u16 vbps, u16 bps)
> +{
> +	struct acpi_nhlt_format_config *fmt;
> +	struct acpi_nhlt_endpoint *ep;

> +	if (!tb)
> +		return ERR_PTR(-EINVAL);

Same as above.
Looking at them, wouldn't simply returning NULL suffice?

> +	for_each_nhlt_endpoint(tb, ep) {
> +		if (!acpi_nhlt_endpoint_match(ep, link_type, dev_type, dir, bus_id))
> +			continue;
> +
> +		fmt = acpi_nhlt_endpoint_find_fmtcfg(ep, ch, rate, vbps, bps);
> +		if (fmt)
> +			return fmt;
> +	}
> +
> +	return NULL;
> +}

...

> +int acpi_nhlt_endpoint_dmic_count(const struct acpi_nhlt_endpoint *ep)
> +{
> +	struct acpi_nhlt_vendor_mic_devcfg *vendor_cfg;
> +	struct acpi_nhlt_format_config *fmt;
> +	struct acpi_nhlt_mic_devcfg *devcfg;
> +	u16 max_ch = 0;
> +
> +	if (!ep || ep->link_type != ACPI_NHLT_PDM)
> +		return -EINVAL;
> +
> +	/* Find max number of channels based on formats configuration. */
> +	for_each_nhlt_endpoint_fmtcfg(ep, fmt)
> +		max_ch = max(fmt->format.channel_count, max_ch);
> +
> +	/* If @ep not a mic array, fallback to channels count. */
> +	devcfg = acpi_nhlt_endpoint_mic_devcfg(ep);
> +	if (!devcfg || devcfg->config_type != ACPI_NHLT_CONFIG_TYPE_MIC_ARRAY)
> +		return max_ch;
> +
> +	switch (devcfg->array_type) {
> +	case ACPI_NHLT_SMALL_LINEAR_2ELEMENT:
> +	case ACPI_NHLT_BIG_LINEAR_2ELEMENT:
> +		return 2;
> +
> +	case ACPI_NHLT_FIRST_GEOMETRY_LINEAR_4ELEMENT:
> +	case ACPI_NHLT_PLANAR_LSHAPED_4ELEMENT:
> +	case ACPI_NHLT_SECOND_GEOMETRY_LINEAR_4ELEMENT:
> +		return 4;
> +
> +	case ACPI_NHLT_VENDOR_DEFINED:
> +		vendor_cfg = acpi_nhlt_endpoint_vendor_mic_devcfg(ep);
> +		if (!vendor_cfg)
> +			return -EINVAL;
> +		return vendor_cfg->num_mics;
> +
> +	default:

> +		pr_warn("undefined mic array type: %#x\n", devcfg->array_type);

Is this function can ever be called without backing device? If not,
I would supply (ACPI?) device pointer and use dev_warn() instead.

But I'm not sure about this. Up to you, folks.

> +		return max_ch;
> +	}
> +}

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 4/4] ACPI: NHLT: Add query functions

[Cezary Rojewski]

On 2023-07-12 5:48 PM, Andy Shevchenko wrote:
> On Wed, Jul 12, 2023 at 11:10:48AM +0200, Cezary Rojewski wrote:

...

>> +bool acpi_nhlt_endpoint_match(const struct acpi_nhlt_endpoint *ep,
>> +			      int link_type, int dev_type, int dir, int bus_id)
>> +{
>> +	return ep &&
>> +	       (link_type < 0 || ep->link_type == link_type) &&
>> +	       (dev_type < 0 || ep->device_type == dev_type) &&
>> +	       (dir < 0 || ep->direction == dir) &&
>> +	       (bus_id < 0 || ep->virtual_bus_id == bus_id);
> 
> I think you can split these for better reading.
> 
> 	if (!ep)
> 		return false;
> 
> 	if (link_type >= 0 && ep->link_type != link_type)
> 		return false;
> 
> 	if (dev_type >= 0 && ep->device_type != dev_type)
> 		return false;
> 
> 	if (dir >= 0 && ep->direction != dir)
> 		return false;
> 
> 	if (bus_id >= 0 && ep->virtual_bus_id != bus_id)
> 		return false;
> 
> 	return true;
> 
> Yes, much more lines, but readability is better in my opinion.
> I also believe that code generation on x86_64 will be the same.

While I favor readability over less lines of code, I do not think 
splitting the conditions makes it easier in this case. Perhaps 
reverse-christmas-tree? Pivoted around '<'.

	return ep &&
	       (link_type < 0 || ep->link_type == link_type) &&
	       (dev_type < 0 || ep->device_type == dev_type) &&
	       (bus_id < 0 || ep->virtual_bus_id == bus_id) &&
	       (dir < 0 || ep->direction == dir);

In general I'd like to distinguish between conditions that one _has to_ 
read and understand and those which reader _may_ pass by. Here, we are 
checking description of an endpoint for equality. Nothing more, nothing 
less.

>> +}

...

>> +struct acpi_nhlt_endpoint *
>> +acpi_nhlt_find_endpoint(const struct acpi_table_nhlt *tb,
>> +			int link_type, int dev_type, int dir, int bus_id)
>> +{
>> +	struct acpi_nhlt_endpoint *ep;
> 
>> +	if (!tb)
>> +		return ERR_PTR(-EINVAL);
> 
> Just wondering how often we will have a caller that supplies NULL for tb.

Depends on kernel's philosophy on public API. In general, public API 
should defend themselves from harmful and illegal callers. However, in 
low level areas 'illegal' is sometimes mistaken with illogical. In such 
cases double safety can be dropped.

Or, perhaps you were discussing return value here and ERR_PTR(-EINVAL) 
could be replaced by something else or even NULL.

>> +	for_each_nhlt_endpoint(tb, ep)
>> +		if (acpi_nhlt_endpoint_match(ep, link_type, dev_type, dir, bus_id))
>> +			return ep;
>> +	return NULL;
>> +}
> 
> ...
> 
>> +struct acpi_nhlt_format_config *
>> +acpi_nhlt_endpoint_find_fmtcfg(const struct acpi_nhlt_endpoint *ep,
>> +			       u16 ch, u32 rate, u16 vbps, u16 bps)
>> +{
>> +	struct acpi_nhlt_wave_extensible *wav;
>> +	struct acpi_nhlt_format_config *fmt;
> 
>> +	if (!ep)
>> +		return ERR_PTR(-EINVAL);
> 
> Similar Q as above.
> 
>> +	for_each_nhlt_endpoint_fmtcfg(ep, fmt) {
>> +		wav = &fmt->format;
>> +
>> +		if (wav->channel_count == ch &&
>> +		    wav->valid_bits_per_sample == vbps &&
>> +		    wav->bits_per_sample == bps &&
>> +		    wav->samples_per_sec == rate)
> 
> Also can be split, but this one readable in the original form.

As order does not really matter here, perhaps reverse-christmas-tree to 
improve readability?

		if (wav->valid_bits_per_sample == vpbs &&
		    wav->samples_per_sec == rate &&
		    wav->bits_per_sample == bps &&
		    wav->channel_count == ch)

>> +			return fmt;
>> +	}
>> +
>> +	return NULL;
>> +}

...

>> +struct acpi_nhlt_format_config *
>> +acpi_nhlt_find_fmtcfg(const struct acpi_table_nhlt *tb,
>> +		      int link_type, int dev_type, int dir, int bus_id,
>> +		      u16 ch, u32 rate, u16 vbps, u16 bps)
>> +{
>> +	struct acpi_nhlt_format_config *fmt;
>> +	struct acpi_nhlt_endpoint *ep;
> 
>> +	if (!tb)
>> +		return ERR_PTR(-EINVAL);
> 
> Same as above.
> Looking at them, wouldn't simply returning NULL suffice?
> 
>> +	for_each_nhlt_endpoint(tb, ep) {
>> +		if (!acpi_nhlt_endpoint_match(ep, link_type, dev_type, dir, bus_id))
>> +			continue;
>> +
>> +		fmt = acpi_nhlt_endpoint_find_fmtcfg(ep, ch, rate, vbps, bps);
>> +		if (fmt)
>> +			return fmt;
>> +	}
>> +
>> +	return NULL;
>> +}
> 
> ...
> 
>> +int acpi_nhlt_endpoint_dmic_count(const struct acpi_nhlt_endpoint *ep)
>> +{
>> +	struct acpi_nhlt_vendor_mic_devcfg *vendor_cfg;
>> +	struct acpi_nhlt_format_config *fmt;
>> +	struct acpi_nhlt_mic_devcfg *devcfg;
>> +	u16 max_ch = 0;
>> +
>> +	if (!ep || ep->link_type != ACPI_NHLT_PDM)
>> +		return -EINVAL;
>> +
>> +	/* Find max number of channels based on formats configuration. */
>> +	for_each_nhlt_endpoint_fmtcfg(ep, fmt)
>> +		max_ch = max(fmt->format.channel_count, max_ch);
>> +
>> +	/* If @ep not a mic array, fallback to channels count. */
>> +	devcfg = acpi_nhlt_endpoint_mic_devcfg(ep);
>> +	if (!devcfg || devcfg->config_type != ACPI_NHLT_CONFIG_TYPE_MIC_ARRAY)
>> +		return max_ch;
>> +
>> +	switch (devcfg->array_type) {
>> +	case ACPI_NHLT_SMALL_LINEAR_2ELEMENT:
>> +	case ACPI_NHLT_BIG_LINEAR_2ELEMENT:
>> +		return 2;
>> +
>> +	case ACPI_NHLT_FIRST_GEOMETRY_LINEAR_4ELEMENT:
>> +	case ACPI_NHLT_PLANAR_LSHAPED_4ELEMENT:
>> +	case ACPI_NHLT_SECOND_GEOMETRY_LINEAR_4ELEMENT:
>> +		return 4;
>> +
>> +	case ACPI_NHLT_VENDOR_DEFINED:
>> +		vendor_cfg = acpi_nhlt_endpoint_vendor_mic_devcfg(ep);
>> +		if (!vendor_cfg)
>> +			return -EINVAL;
>> +		return vendor_cfg->num_mics;
>> +
>> +	default:
> 
>> +		pr_warn("undefined mic array type: %#x\n", devcfg->array_type);
> 
> Is this function can ever be called without backing device? If not,
> I would supply (ACPI?) device pointer and use dev_warn() instead.
> 
> But I'm not sure about this. Up to you, folks.

Given what's our there in the market I wouldn't say it's impossible to 
encounter such scenario. Could you elaborate on what you meant by 
"supply device pointer"?

>> +		return max_ch;
>> +	}
>> +}
> 

Re: [PATCH 4/4] ACPI: NHLT: Add query functions

[Andy Shevchenko]

On Mon, Jul 17, 2023 at 10:29:17AM +0200, Cezary Rojewski wrote:
> On 2023-07-12 5:48 PM, Andy Shevchenko wrote:
> > On Wed, Jul 12, 2023 at 11:10:48AM +0200, Cezary Rojewski wrote:

...

> > > +	return ep &&
> > > +	       (link_type < 0 || ep->link_type == link_type) &&
> > > +	       (dev_type < 0 || ep->device_type == dev_type) &&
> > > +	       (dir < 0 || ep->direction == dir) &&
> > > +	       (bus_id < 0 || ep->virtual_bus_id == bus_id);
> > 
> > I think you can split these for better reading.
> > 
> > 	if (!ep)
> > 		return false;
> > 
> > 	if (link_type >= 0 && ep->link_type != link_type)
> > 		return false;
> > 
> > 	if (dev_type >= 0 && ep->device_type != dev_type)
> > 		return false;
> > 
> > 	if (dir >= 0 && ep->direction != dir)
> > 		return false;
> > 
> > 	if (bus_id >= 0 && ep->virtual_bus_id != bus_id)
> > 		return false;
> > 
> > 	return true;
> > 
> > Yes, much more lines, but readability is better in my opinion.
> > I also believe that code generation on x86_64 will be the same.
> 
> While I favor readability over less lines of code, I do not think splitting
> the conditions makes it easier in this case. Perhaps reverse-christmas-tree?
> Pivoted around '<'.
> 
> 	return ep &&
> 	       (link_type < 0 || ep->link_type == link_type) &&
> 	       (dev_type < 0 || ep->device_type == dev_type) &&
> 	       (bus_id < 0 || ep->virtual_bus_id == bus_id) &&
> 	       (dir < 0 || ep->direction == dir);

This one definitely better.

> In general I'd like to distinguish between conditions that one _has to_ read
> and understand and those which reader _may_ pass by. Here, we are checking
> description of an endpoint for equality. Nothing more, nothing less.

...

> > > +struct acpi_nhlt_endpoint *
> > > +acpi_nhlt_find_endpoint(const struct acpi_table_nhlt *tb,
> > > +			int link_type, int dev_type, int dir, int bus_id)
> > > +{
> > > +	struct acpi_nhlt_endpoint *ep;
> > 
> > > +	if (!tb)
> > > +		return ERR_PTR(-EINVAL);
> > 
> > Just wondering how often we will have a caller that supplies NULL for tb.
> 
> Depends on kernel's philosophy on public API. In general, public API should
> defend themselves from harmful and illegal callers. However, in low level
> areas 'illegal' is sometimes mistaken with illogical. In such cases double
> safety can be dropped.

What do you put under "public"? ABI? Or just internally available API?
If the latter, we don't do defensive programming there, we usually just
add NULL(invalid data)-awareness to the free()-like functions.

> Or, perhaps you were discussing return value here and ERR_PTR(-EINVAL) could
> be replaced by something else or even NULL.

I prefer to get rid of those.

> > > +	for_each_nhlt_endpoint(tb, ep)
> > > +		if (acpi_nhlt_endpoint_match(ep, link_type, dev_type, dir, bus_id))
> > > +			return ep;
> > > +	return NULL;
> > > +}

...

> > > +		if (wav->channel_count == ch &&
> > > +		    wav->valid_bits_per_sample == vbps &&
> > > +		    wav->bits_per_sample == bps &&
> > > +		    wav->samples_per_sec == rate)
> > 
> > Also can be split, but this one readable in the original form.
> 
> As order does not really matter here, perhaps reverse-christmas-tree to
> improve readability?
> 
> 		if (wav->valid_bits_per_sample == vpbs &&
> 		    wav->samples_per_sec == rate &&
> 		    wav->bits_per_sample == bps &&
> 		    wav->channel_count == ch)

OK!

> > > +			return fmt;

...

> > > +	default:
> > 
> > > +		pr_warn("undefined mic array type: %#x\n", devcfg->array_type);
> > 
> > Is this function can ever be called without backing device? If not,
> > I would supply (ACPI?) device pointer and use dev_warn() instead.
> > 
> > But I'm not sure about this. Up to you, folks.
> 
> Given what's our there in the market I wouldn't say it's impossible to
> encounter such scenario. Could you elaborate on what you meant by "supply
> device pointer"?

In the caller (assuming it has ACPI device):

	ret = acpi_nhlt_endpoint_dmic_count(adev, ep);
	if (ret < 0)
		...

> > > +		return max_ch;
> > > +	}

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 4/4] ACPI: NHLT: Add query functions

[Cezary Rojewski]

On 2023-07-17 11:47 AM, Andy Shevchenko wrote:
> On Mon, Jul 17, 2023 at 10:29:17AM +0200, Cezary Rojewski wrote:
>> On 2023-07-12 5:48 PM, Andy Shevchenko wrote:
>>> On Wed, Jul 12, 2023 at 11:10:48AM +0200, Cezary Rojewski wrote:

...

>>>> +	return ep &&
>>>> +	       (link_type < 0 || ep->link_type == link_type) &&
>>>> +	       (dev_type < 0 || ep->device_type == dev_type) &&
>>>> +	       (dir < 0 || ep->direction == dir) &&
>>>> +	       (bus_id < 0 || ep->virtual_bus_id == bus_id);
>>>
>>> I think you can split these for better reading.
>>>
>>> 	if (!ep)
>>> 		return false;
>>>
>>> 	if (link_type >= 0 && ep->link_type != link_type)
>>> 		return false;
>>>
>>> 	if (dev_type >= 0 && ep->device_type != dev_type)
>>> 		return false;
>>>
>>> 	if (dir >= 0 && ep->direction != dir)
>>> 		return false;
>>>
>>> 	if (bus_id >= 0 && ep->virtual_bus_id != bus_id)
>>> 		return false;
>>>
>>> 	return true;
>>>
>>> Yes, much more lines, but readability is better in my opinion.
>>> I also believe that code generation on x86_64 will be the same.
>>
>> While I favor readability over less lines of code, I do not think splitting
>> the conditions makes it easier in this case. Perhaps reverse-christmas-tree?
>> Pivoted around '<'.
>>
>> 	return ep &&
>> 	       (link_type < 0 || ep->link_type == link_type) &&
>> 	       (dev_type < 0 || ep->device_type == dev_type) &&
>> 	       (bus_id < 0 || ep->virtual_bus_id == bus_id) &&
>> 	       (dir < 0 || ep->direction == dir);
> 
> This one definitely better.

Ack.

>> In general I'd like to distinguish between conditions that one _has to_ read
>> and understand and those which reader _may_ pass by. Here, we are checking
>> description of an endpoint for equality. Nothing more, nothing less.
> 
> ...
> 
>>>> +struct acpi_nhlt_endpoint *
>>>> +acpi_nhlt_find_endpoint(const struct acpi_table_nhlt *tb,
>>>> +			int link_type, int dev_type, int dir, int bus_id)
>>>> +{
>>>> +	struct acpi_nhlt_endpoint *ep;
>>>
>>>> +	if (!tb)
>>>> +		return ERR_PTR(-EINVAL);
>>>
>>> Just wondering how often we will have a caller that supplies NULL for tb.
>>
>> Depends on kernel's philosophy on public API. In general, public API should
>> defend themselves from harmful and illegal callers. However, in low level
>> areas 'illegal' is sometimes mistaken with illogical. In such cases double
>> safety can be dropped.
> 
> What do you put under "public"? ABI? Or just internally available API?
> If the latter, we don't do defensive programming there, we usually just
> add NULL(invalid data)-awareness to the free()-like functions.

Thanks for explaining!

>> Or, perhaps you were discussing return value here and ERR_PTR(-EINVAL) could
>> be replaced by something else or even NULL.
> 
> I prefer to get rid of those.

Agreed.

>>>> +	for_each_nhlt_endpoint(tb, ep)
>>>> +		if (acpi_nhlt_endpoint_match(ep, link_type, dev_type, dir, bus_id))
>>>> +			return ep;
>>>> +	return NULL;
>>>> +}
> 
> ...
> 
>>>> +		if (wav->channel_count == ch &&
>>>> +		    wav->valid_bits_per_sample == vbps &&
>>>> +		    wav->bits_per_sample == bps &&
>>>> +		    wav->samples_per_sec == rate)
>>>
>>> Also can be split, but this one readable in the original form.
>>
>> As order does not really matter here, perhaps reverse-christmas-tree to
>> improve readability?
>>
>> 		if (wav->valid_bits_per_sample == vpbs &&
>> 		    wav->samples_per_sec == rate &&
>> 		    wav->bits_per_sample == bps &&
>> 		    wav->channel_count == ch)
> 
> OK!

Ack.

>>>> +			return fmt;
> 
> ...
> 
>>>> +	default:
>>>
>>>> +		pr_warn("undefined mic array type: %#x\n", devcfg->array_type);
>>>
>>> Is this function can ever be called without backing device? If not,
>>> I would supply (ACPI?) device pointer and use dev_warn() instead.
>>>
>>> But I'm not sure about this. Up to you, folks.
>>
>> Given what's our there in the market I wouldn't say it's impossible to
>> encounter such scenario. Could you elaborate on what you meant by "supply
>> device pointer"?
> 
> In the caller (assuming it has ACPI device):
> 
> 	ret = acpi_nhlt_endpoint_dmic_count(adev, ep);
> 	if (ret < 0)
> 		...
> 

Thanks for explaining. I'm going to kindly disagree here - dev pointer 
would be here only to print the warning. NHLT is also independent of any 
device - even if its present in the system, one may not have a single 
device that utilizes it. Worth mentioning is fact that all other 
functions do not accept such argument. Doing so here alone would break 
API consistency.

>>>> +		return max_ch;
>>>> +	}
> 

Re: [PATCH 4/4] ACPI: NHLT: Add query functions

[Cezary Rojewski]

On 2023-07-17 11:47 AM, Andy Shevchenko wrote:
> On Mon, Jul 17, 2023 at 10:29:17AM +0200, Cezary Rojewski wrote:
>> On 2023-07-12 5:48 PM, Andy Shevchenko wrote:
>>> On Wed, Jul 12, 2023 at 11:10:48AM +0200, Cezary Rojewski wrote:

...

>>>> +struct acpi_nhlt_endpoint *
>>>> +acpi_nhlt_find_endpoint(const struct acpi_table_nhlt *tb,
>>>> +			int link_type, int dev_type, int dir, int bus_id)
>>>> +{
>>>> +	struct acpi_nhlt_endpoint *ep;
>>>
>>>> +	if (!tb)
>>>> +		return ERR_PTR(-EINVAL);
>>>
>>> Just wondering how often we will have a caller that supplies NULL for tb.
>>
>> Depends on kernel's philosophy on public API. In general, public API should
>> defend themselves from harmful and illegal callers. However, in low level
>> areas 'illegal' is sometimes mistaken with illogical. In such cases double
>> safety can be dropped.
> 
> What do you put under "public"? ABI? Or just internally available API?
> If the latter, we don't do defensive programming there, we usually just
> add NULL(invalid data)-awareness to the free()-like functions.
> 
>> Or, perhaps you were discussing return value here and ERR_PTR(-EINVAL) could
>> be replaced by something else or even NULL.
> 
> I prefer to get rid of those.

Decided to do some manual tests on more exotic setups that are not part 
of our daily CI/CD routine and, completely getting rid of those ifs 
causes problems. Those setups are part of the market, expose DSP 
capabilities but have invalid BIOS configurations.

Rather than just bringing back the if-statement, different solution came 
to my mind:

static struct acpi_table_nhlt empty_nhlt = {
	.header = {
		.signature = ACPI_SIG_NHLT,
	},
};

struct acpi_table_nhlt *acpi_gbl_NHLT;
EXPORT_SYMBOL_GPL(acpi_gbl_NHLT);

acpi_status acpi_nhlt_get_gbl_table(void)
{
	acpi_status status;

	status = acpi_get_table(ACPI_SIG_NHLT, 0, (struct acpi_table_header 
**)(&acpi_gbl_NHLT));
	if (!acpi_gbl_NHLT)
		acpi_gbl_NHLT = &empty_nhlt;
	return status;
}
EXPORT_SYMBOL_GPL(acpi_nhlt_get_gbl_table);


What do you think?

Re: [PATCH 4/4] ACPI: NHLT: Add query functions

[Andy Shevchenko]

On Tue, Jul 18, 2023 at 01:11:03PM +0200, Cezary Rojewski wrote:
> On 2023-07-17 11:47 AM, Andy Shevchenko wrote:
> > On Mon, Jul 17, 2023 at 10:29:17AM +0200, Cezary Rojewski wrote:

...

> > I prefer to get rid of those.
> 
> Decided to do some manual tests on more exotic setups that are not part of
> our daily CI/CD routine and, completely getting rid of those ifs causes
> problems. Those setups are part of the market, expose DSP capabilities but
> have invalid BIOS configurations.
> 
> Rather than just bringing back the if-statement, different solution came to
> my mind:
> 
> static struct acpi_table_nhlt empty_nhlt = {
> 	.header = {
> 		.signature = ACPI_SIG_NHLT,
> 	},
> };
> 
> struct acpi_table_nhlt *acpi_gbl_NHLT;
> EXPORT_SYMBOL_GPL(acpi_gbl_NHLT);
> 
> acpi_status acpi_nhlt_get_gbl_table(void)
> {
> 	acpi_status status;
> 
> 	status = acpi_get_table(ACPI_SIG_NHLT, 0, (struct acpi_table_header
> **)(&acpi_gbl_NHLT));
> 	if (!acpi_gbl_NHLT)
> 		acpi_gbl_NHLT = &empty_nhlt;
> 	return status;
> }
> EXPORT_SYMBOL_GPL(acpi_nhlt_get_gbl_table);
> 
> What do you think?

I think it's wonderful what you found and I dunno how I missed this.
Go for this, definitely!

-- 
With Best Regards,
Andy Shevchenko