Re: [PATCH v2 1/1] x86/rtc: Allocate interrupt for platform device

[Andy Shevchenko]

On Mon, Jan 16, 2017 at 11:00 PM, Thomas Gleixner <tglx@xxxxxxxxxxxxx> wrote:
> On Mon, 16 Jan 2017, Andy Shevchenko wrote:
>> On Mon, 2017-01-16 at 20:04 +0100, Luis R. Rodriguez wrote:
>> > Referring to commits on linux-next gitsums is not proper as they
>> > change
>> > every day, so you might as well leave them out.
>>
>> It's in maintainer's tree which has it the same. And most probably it
>> would go the same (since commit includes timestamps). I never heard
>> before of such issues (unless someone *rebased* tree for linux-next).
>
> Which is obviously the case. tip x86/platform has:
>
> de1c2540aa4f: x86/platform/intel-mid: Enable RTC on Intel Merrifield

Oh, didn't realize that. Thanks for pointing out.

>> ...also I have no idea when exactly it will happen during
>> initialization.
>
> That's a non argument. You really can figure that out ....
>
> Like you could have figured out that calling that function unconditially is
> not a brilliant idea....

Yes, that's correct. Just had no time to do that quickly.

>> I agree that's not nice looking piece of code, but this due to absence
>> of some stubs. IOAPIC code is really old one and misses stuff (proper
>> error codes, stubs for no IOAPIC case).
>
> That's a fixable problem and in no way a justification for horrible
> hackery.

I see your proposal below. Indeed it looks much cleaner.
Thanks!

> +static void __init intel_mid_legacy_rtc_init(void)
> +{
> +       struct irq_alloc_info info;
> +
> +       ioapic_set_alloc_attr(&info, NUMA_NO_NODE, 1, 0);
> +       if (mp_map_gsi_to_irq(RTC_IRQ, IOAPIC_MAP_ALLOC, &info)) {

I will fix this (> 0 is okay, = 0 I have to check) and give it a try.

> +               pr_info("Failed to allocate RTC interrupt. Disabling RTC\n");
> +               x86_platform.legacy.rtc = 0;
> +       }
> +}

> -static int __init intel_mid_legacy_rtc_init(void)
> -{
> -       struct irq_alloc_info info;
> -
> -       if (!x86_platform.legacy.rtc)
> -               return -ENODEV;
> -
> -       ioapic_set_alloc_attr(&info, NUMA_NO_NODE, 1, 0);
> -       return mp_map_gsi_to_irq(RTC_IRQ, IOAPIC_MAP_ALLOC, &info);
> -}
> -
>  static int __init intel_mid_platform_init(void)
>  {
> -       intel_mid_legacy_rtc_init();
> -
>         sfi_table_parse(SFI_SIG_GPIO, NULL, NULL, sfi_parse_gpio);
>         sfi_table_parse(SFI_SIG_DEVS, NULL, NULL, sfi_parse_devs);
>         return 0;

By the way, would be better to revert or dismiss that commit at all?

-- 
With Best Regards,
Andy Shevchenko