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
