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

[Andy Shevchenko <andriy.shevchenko@linux.intel.com>]

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