Re: [Xen-devel] [PATCH 1/2] x86/vHPET: replace literal numbers

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 17.07.18 at 11:03, <andrew.cooper3@xxxxxxxxxx> wrote:
> On 17/07/18 09:35, Jan Beulich wrote:
>> @@ -411,7 +410,13 @@ static int hpet_write(
>>      case HPET_Tn_CFG(2):
>>          tn = HPET_TN(CFG, addr);
>>  
>> -        h->hpet.timers[tn].config = hpet_fixup_reg(new_val, old_val, 
>> 0x3f4e);
>> +        h->hpet.timers[tn].config = hpet_fixup_reg(new_val, old_val,
>> +                                                   HPET_TN_LEVEL |
>> +                                                   HPET_TN_ENABLE |
>> +                                                   HPET_TN_PERIODIC |
>> +                                                   HPET_TN_SETVAL |
>> +                                                   HPET_TN_32BIT |
>> +                                                   HPET_TN_ROUTE);
> 
> This can be rather better reflowed.  e.g.
> 
> diff --git a/xen/arch/x86/hvm/hpet.c b/xen/arch/x86/hvm/hpet.c
> index f7ef4f7..5986d1d 100644
> --- a/xen/arch/x86/hvm/hpet.c
> +++ b/xen/arch/x86/hvm/hpet.c
> @@ -411,7 +411,10 @@ static int hpet_write(
>      case HPET_Tn_CFG(2):
>          tn = HPET_TN(CFG, addr);
>  
> -        h->hpet.timers[tn].config = hpet_fixup_reg(new_val, old_val, 0x3f4e);
> +        h->hpet.timers[tn].config = hpet_fixup_reg(
> +            new_val, old_val, (HPET_TN_LEVEL | HPET_TN_ENABLE |
> +                               HPET_TN_PERIODIC | HPET_TN_SETVAL |
> +                               HPET_TN_32BIT | HPET_TN_ROUTE));

Whether this is "better" is a matter of taste. To be honest, I pretty
much dislike this indentation style for function calls. I could maybe
be talked into

        h->hpet.timers[tn].config =
            hpet_fixup_reg(new_val, old_val,
                           (HPET_TN_LEVEL | HPET_TN_ENABLE |
                            HPET_TN_PERIODIC | HPET_TN_SETVAL |
                            HPET_TN_32BIT | HPET_TN_ROUTE));

(albeit it's only marginally better than what you suggest), but clearly
not your variant.

> Otherwise, Reviewed-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>

Let me know for which of the variant(s) this stands.

Jan



_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel