Re: [PATCH 02/11] x86: sysfb: remove sysfb when probing real hw

From: Ingo Molnar
Date: Thu Jan 23 2014 - 11:51:27 EST



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.

> +#ifdef CONFIG_X86_SYSFB
> +# include <asm/sysfb.h>
> +#endif

I guess a single space is sufficient?

Better yet, I'd include sysfb.h unconditionally:

> @@ -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.

Thanks,

Ingo
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/