Re: [PATCH 2/6] misc/pvpanic: Add pvpanic driver framework

[Greg KH <gregkh@linuxfoundation.org>]

On Tue, Jan 22, 2019 at 03:25:07AM +0800, Peng Hao wrote:
> Add pvpanic driver framework.
> 

You need a lot more description of what you did here than this, as I can
not understand from this text, what the patch does, or more importantly,
why it is doing this, at all.



> Signed-off-by: Peng Hao <peng.hao2@zte.com.cn>
> ---
>  drivers/misc/pvpanic/pvpanic.c | 171 ++++++++++-------------------------------
>  1 file changed, 39 insertions(+), 132 deletions(-)
> 
> diff --git a/drivers/misc/pvpanic/pvpanic.c b/drivers/misc/pvpanic/pvpanic.c
> index 595ac06..6380540 100644
> --- a/drivers/misc/pvpanic/pvpanic.c
> +++ b/drivers/misc/pvpanic/pvpanic.c
> @@ -8,15 +8,20 @@
>  
>  #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
>  
> -#include <linux/acpi.h>
> +#include <linux/io.h>
>  #include <linux/kernel.h>
>  #include <linux/module.h>
> -#include <linux/of.h>
> -#include <linux/of_address.h>
>  #include <linux/platform_device.h>
>  #include <linux/types.h>
>  
> -static void __iomem *base;
> +static struct {
> +	struct platform_device *pdev;
> +	void __iomem *base;
> +	bool is_ioport;
> +} pvpanic_data = {
> +	.pdev = NULL,
> +	.is_ioport = false,

You do not need to initialize variables to 0 specifically like this.

> +};
>  
>  #define PVPANIC_PANICKED        (1 << 0)
>  
> @@ -27,7 +32,7 @@
>  static void
>  pvpanic_send_event(unsigned int event)
>  {
> -	iowrite8(event, base);
> +	iowrite8(event, pvpanic_data.base);

Why did you convert a single global variable into a single global
structure?  Why not, if you really need to pass this value around, do
that at the same time as you will end up touching these same functions
again, right?

thanks,

greg k-h