Re: [PATCH for-5.0 4/8] acpi: cpuhp: spec: fix 'Command data' description

[Laszlo Ersek <lersek@redhat.com>]

On 12/04/19 18:05, Igor Mammedov wrote:
> Correct returned value description in case 'Command field' == 0x0,
> it's in not PXM but CPU selector value with pending event

(1) s/in not/not/

> 
> In addition describe 0 blanket value in case of not supported
> 'Command field' value.
> 
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> ---
>  docs/specs/acpi_cpu_hotplug.txt | 11 +++++------
>  1 file changed, 5 insertions(+), 6 deletions(-)
> 
> diff --git a/docs/specs/acpi_cpu_hotplug.txt b/docs/specs/acpi_cpu_hotplug.txt
> index 4e65286..19c508f 100644
> --- a/docs/specs/acpi_cpu_hotplug.txt
> +++ b/docs/specs/acpi_cpu_hotplug.txt
> @@ -56,9 +56,8 @@ read access:
>             3-7: reserved and should be ignored by OSPM
>      [0x5-0x7] reserved
>      [0x8] Command data: (DWORD access)
> -          in case of error or unsupported command reads is 0xFFFFFFFF
> -          current 'Command field' value:
> -              0: returns PXM value corresponding to device
> +          contains 0 unless last stored in 'Command field' value is one of:
> +              0: contains 'CPU selector' value of a CPU with pending event[s]

(2) I think we can improve the word order:

  last stored in 'Command field' value
->
  value last stored in 'Command field'

>  
>  write access:
>      offset:
> @@ -81,9 +80,9 @@ write access:
>            value:
>              0: selects a CPU device with inserting/removing events and
>                 following reads from 'Command data' register return
> -               selected CPU (CPU selector value). If no CPU with events
> -               found, the current CPU selector doesn't change and
> -               corresponding insert/remove event flags are not set.
> +               selected CPU ('CPU selector' value).
> +               If no CPU with events found, the current 'CPU selector' doesn't
> +               change and corresponding insert/remove event flags are not set.

(3) AFAICT this is only a -- useful! -- re-wrapping. But, since we are
modifying this section anyway, can we replace "flags are not set" with
"flags are left unchanged" or "flags are not modified"?

"set" is ambiguous with bit fields: it can mean "rewritten", and it can
mean "set to 1".

>              1: following writes to 'Command data' register set OST event
>                 register in QEMU
>              2: following writes to 'Command data' register set OST status
> 

Anyway, these are all superficial comments. Pick up whatever you agree
with. Either way:

Reviewed-by: Laszlo Ersek <lersek@redhat.com>

Thanks!
Laszlo