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;
+ }
