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

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

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