On 2023-08-09 11:05 AM, Andy Shevchenko wrote:
On Wed, Aug 9, 2023 at 11:48 AM Cezary Rojewski
<cezary.rojewski@xxxxxxxxx> wrote:
...
First, you took the review seriously and provided a ton of valid
feedback. And your reviews and expertise helped me grow as a developer,
so from my perspective no need to sorry about spotting bad things late.
Now, I admit, a bit surprised given the number of revisions and age of
the initial patchset. The cover-letter, attached for each revision, made
the intentions clear.
As you may notice I'm not against code that is done as a part of the
Linux kernel and my surprise is the ACPICA change. My focus for review
was a Linux kernel and it was just by a chance I looked at the PR on
GitHub. There is neither good explanation in the commit message nor
discussion of the change. What I probably miss (and that may help me
to understand better the change) are:
- the examples of the code snippets that are using data types before and after
- explanation why not all data types were covered (there are more
"strange" names like with _a, _b suffix)
- how this is supposed to be maintained as the ACPICA has users
outside of the kernel and how the change
makes their life easier (to me it's the opposite).
In general, many types are bogus or redundant. I'd argue that having
types defined as _a, _b, _c, _d make the life harder :)
struct acpi_nhlt_device_specific_config_a
bogus, there is no '_a', it's called mic device config instead
struct acpi_nhlt_device_specific_config_d
bogus, such thing does not exist
it breaks the spec as the "CapabilitiesSize" is missing
struct acpi_nhlt_device_specific_config_b
bogus, such thing does not exist
it's the header of any dev config but just header alone is
invalid
struct acpi_nhlt_device_specific_config_c
bogus, describes an invalid type. Such thing can be encountered
only when parsing damaged NHLT
struct acpi_nhlt_render_device_specific_config
redundant
Then we have constants such as:
#define ACPI_NHLT_PDM 2
#define ACPI_NHLT_SSP 3
_PDM/_SSP what? There is no NHLT of type PDM or of type SSP. These
specify link types but it's not mentioned in constants names.
I believe that by now you see where am going. The patch focuses on
device-specific-config area as it's the area that requires most help.
Our goal is to help actual users of NHLT i.e.:
audio teams. While part of ACPICA, NHLT-code is hidden within sound/ so
no one asks questions. Leaving things at status quo does not improve the
situation.
What situation? To me it makes it worse. (Again, I'm talking solely
for ACPICA change, the rest I have reviewed and I am fine with the
direction taken.)
Situation = on top of above, NHLT-code is currently within sound/.
Let the handlers be part of drivers/acpi as is the case for all ACPI
tables. The handlers themselves do not (and shall not) belong to ACPICA
if I'm not mistaken.
Thus I believe simple "no" is not an option here. To make the
code better overall, relevant pieces should be made part of drivers/acpi.
Original problems stem from the fact that audio teams were not looped in
during initial integration of NHLT-code. Turned out that no users
utilize it in its current form. The problems are subtle, but a
discussion wouldn't hurt.
To avoid double posting, should we continue the discussion here or in
the PR on github?
Let's do it there, as it's purely about ACPICA.
The kernel part will be affected depending on the result of the discussion.
Ok.
