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

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

On 2023-08-09 7:00 AM, andy.shevchenko@gmail.com wrote:
> Fri, Jul 21, 2023 at 05:48:10PM +0200, Cezary Rojewski kirjoitti:
>> Device configuration structures are plenty so declare a struct for each
>> known variant. As neither of them shall be accessed without verifying
>> the memory block first, introduce macros to make it easy to do so.
>>
>> Link: https://github.com/acpica/acpica/pull/881
> 
> Thinking of this over night (as I replied in the above)...
> 
>> Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> 
> Sorry, but seems I have to retract my tag and even more, NAK to the ACPICA changes.
> 
> I have thought that this is something new to the header there, but it appears that
> it duplicates (in a wrong way in my opinion) existing data types.
> 
> Existing data types are crafted (as far as I get them) in a way to be able to be
> combined in the union. In the similar way how _CRS is parsed in DSDT (first that
> comes to my mind). Hence that "simplification" is quite wrong in a few ways:
> - it breaks ACPICA agreement on naming schema
> - it duplicates existing data
> - it made it even partially
> - it is fine and correct in ACPICA to have long dereferenced data, again see
>    for the union of acpi_object
> 
> I trully believe now that the above change in ACPICA must be reverted.
> 
> Again, sorry for this late bad news from my side. I have no clue why
> it was merged, perhaps lack of review? Or anything subtle I so miserably
> missed?

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. 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. 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?


Kind regards,
Czarek