Re: [PATCH v4 1/4] ACPI: NHLT: Device configuration access interface

[Cezary Rojewski <cezary.rojewski@intel.com>]

On 2023-08-09 11:05 AM, Andy Shevchenko wrote:
> On Wed, Aug 9, 2023 at 11:48 AM Cezary Rojewski
> <cezary.rojewski@intel.com> 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.