Hi On Thu, Jan 23, 2014 at 5:51 PM, Ingo Molnar <mingo@xxxxxxxxxx> wrote: > > Just a couple of small nits: > > * David Herrmann <dh.herrmann@xxxxxxxxx> wrote: > >> --- a/arch/x86/kernel/sysfb.c >> +++ b/arch/x86/kernel/sysfb.c >> @@ -33,11 +33,76 @@ >> #include <linux/init.h> >> #include <linux/kernel.h> >> #include <linux/mm.h> >> +#include <linux/mutex.h> >> #include <linux/platform_data/simplefb.h> >> #include <linux/platform_device.h> >> #include <linux/screen_info.h> >> #include <asm/sysfb.h> >> >> +static DEFINE_MUTEX(sysfb_lock); >> +static struct platform_device *sysfb_dev; >> + >> +int __init sysfb_register(const char *name, int id, >> + const struct resource *res, unsigned int res_num, >> + const void *data, size_t data_size) >> +{ >> + struct platform_device *pd; >> + int ret = 0; >> + >> + mutex_lock(&sysfb_lock); >> + if (!sysfb_dev) { >> + pd = platform_device_register_resndata(NULL, name, id, >> + res, res_num, >> + data, data_size); >> + if (IS_ERR(pd)) >> + ret = PTR_ERR(pd); >> + else >> + sysfb_dev = pd; >> + } >> + mutex_unlock(&sysfb_lock); >> + >> + return ret; >> +} >> + >> +static bool sysfb_match(const struct apertures_struct *apert) >> +{ >> + struct screen_info *si = &screen_info; >> + unsigned int i; >> + const struct aperture *a; >> + >> + for (i = 0; i < apert->count; ++i) { >> + a = &apert->ranges[i]; >> + if (a->base >= si->lfb_base && >> + a->base < si->lfb_base + ((u64)si->lfb_size << 16)) >> + return true; >> + if (si->lfb_base >= a->base && >> + si->lfb_base < a->base + a->size) >> + return true; >> + } >> + >> + return false; >> +} >> + >> +/* Remove sysfb and disallow new sysfbs from now on. Can be called from any >> + * context except recursively (see also remove_conflicting_framebuffers()). */ >> +void sysfb_unregister(const struct apertures_struct *apert, bool primary) > > Please use the customary (multi-line) comment style: > > /* > * Comment ..... > * ...... goes here. > */ > > specified in Documentation/CodingStyle. Whoops, will fix it up. Still used to that from HID code. >> +#ifdef CONFIG_X86_SYSFB >> +# include <asm/sysfb.h> >> +#endif > > I guess a single space is sufficient? > > Better yet, I'd include sysfb.h unconditionally: Unconditionally won't work as only x86 has this header. If there's a way to place a dummy into asm-generic which is picked if arch/xy/include/asm/ doesn't have the header, let me know. But if I include it unconditionally without any fallback, this will fail on non-x86. And adding the header to all archs seems overkill. >> @@ -1773,6 +1780,10 @@ register_framebuffer(struct fb_info *fb_info) >> { >> int ret; >> >> +#ifdef CONFIG_X86_SYSFB >> + sysfb_unregister(fb_info->apertures, fb_is_primary_device(fb_info)); >> +#endif > > So, if a dummy sysfb_unregister() inline was defined in the > !CONFIG_X86_SYSFB case then this ugly #ifdef could possibly be > removed? Especially as it's used twice. Again, this is fine for x86, but not for other archs. I would still need the #ifdef x86. Note that patch #6 introduces linux/sysfb.h and removes all these ugly #ifdefs again. They're only needed to fix the x86 code *now*. Patch #6 generalizes the x86-sysfb infrastructure and makes it arch-independent. But Patch #6 introduces new features and thus shouldn't go to stable or 3.14. As Patch #1 already fixes nearly all issues with sysfb, let me know if you want to drop this patch and just wait for the arch-independent sysfb to get merged. This patch is only needed if people enable X86_SYSFB *and* FB_SIMPLE *purposely* and want hw-handover. The case were people enable it accidentally is fixed by Patch #1. The situation is kind of screwed.. sorry for that. Thanks David _______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/dri-devel