[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH 1/2] x86/hpet: Factor hpet_enable_legacy_replacement_mode() out of hpet_setup()



On 25.03.2021 17:52, Andrew Cooper wrote:
> ... in preparation to introduce a second caller.
> 
> No functional change.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>

Generally
Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>
but I think there's one small code change needed plus I have two
nits (and I expect that this change wouldn't be committed without
patch 2, as making the function non-static isn't warranted
otherwise):

> --- a/xen/arch/x86/hpet.c
> +++ b/xen/arch/x86/hpet.c
> @@ -754,11 +754,70 @@ int hpet_legacy_irq_tick(void)
>  }
>  
>  static u32 *hpet_boot_cfg;
> +static u64 __initdata hpet_rate;

Use uint64_t as you move this here?

> +bool __init hpet_enable_legacy_replacement_mode(void)
> +{
> +    unsigned int id, cfg, c0_cfg, ticks, count;
> +
> +    if ( !hpet_rate ||

I think you need to also honor opt_hpet here.

> +         !((id = hpet_read32(HPET_ID)) & HPET_ID_LEGSUP) ||

I don't think I see a need for the assignment and hence the local
variable. Dropping it would make the code easier to read imo.

Jan



 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.