Re: [PATCH 3/6] misc/pvpanic: add API for pvpanic driver framework

[Greg KH <gregkh@linuxfoundation.org>]

On Tue, Jan 22, 2019 at 03:25:08AM +0800, Peng Hao wrote:
> Add pvpanic_add/remove_device API.

That says what you are doing, but not _why_ you need to do this.

Why?

> 
> Signed-off-by: Peng Hao <peng.hao2@zte.com.cn>
> ---
>  drivers/misc/pvpanic/pvpanic.c | 32 ++++++++++++++++++++++++++++++++
>  drivers/misc/pvpanic/pvpanic.h | 14 ++++++++++++++
>  2 files changed, 46 insertions(+)
>  create mode 100644 drivers/misc/pvpanic/pvpanic.h
> 
> diff --git a/drivers/misc/pvpanic/pvpanic.c b/drivers/misc/pvpanic/pvpanic.c
> index 227ab4e..f842ee4 100644
> --- a/drivers/misc/pvpanic/pvpanic.c
> +++ b/drivers/misc/pvpanic/pvpanic.c
> @@ -48,6 +48,38 @@
>  	.priority = 1, /* let this called before broken drm_fb_helper */
>  };
>  
> +int pvpanic_add_device(struct device *dev, struct resource *res)
> +{
> +	struct platform_device *pdev;
> +	int ret;
> +
> +	pdev = platform_device_alloc("pvpanic", -1);
> +	if (!pdev)
> +		return -ENOMEM;
> +
> +	pdev->dev.parent = dev;
> +
> +	ret = platform_device_add_resources(pdev, res, 1);
> +	if (ret)
> +		goto err;
> +
> +	ret = platform_device_add(pdev);
> +	if (ret)
> +		goto err;
> +	pvpanic_data.pdev = pdev;
> +
> +	return 0;
> +err:
> +	platform_device_put(pdev);
> +	return -1;
> +}
> +
> +void pvpanic_remove_device(void)
> +{
> +	platform_device_unregister(pvpanic_data.pdev);
> +	pvpanic_data.pdev = NULL;
> +}
> +
>  static int pvpanic_platform_probe(struct platform_device *pdev)
>  {
>  	struct device *dev = &pdev->dev;
> diff --git a/drivers/misc/pvpanic/pvpanic.h b/drivers/misc/pvpanic/pvpanic.h
> new file mode 100644
> index 0000000..a72ca59
> --- /dev/null
> +++ b/drivers/misc/pvpanic/pvpanic.h
> @@ -0,0 +1,14 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later

Do you really mean or later?  I have to ask, sorry.

> +/* pvpanic driver framework header file
> + *
> + * Copyright (C) 2019 ZTE Ltd.
> + * Author: Peng Hao <peng.hao2@zte.com.cn>
> + */
> +
> +#ifndef __DRIVERS_MISC_PVPANIC_H
> +#define __DRIVERS_MISC_PVPANIC_H
> +
> +extern int pvpanic_add_device(struct device *dev, struct resource *res);
> +extern void pvpanic_remove_device(void);


No need for "extern", and why have these functions here if no one is
using them?

thanks,

greg k-h