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