Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Thu, 29 Oct 2020 14:57:15 +0100
From:   "gregkh@linuxfoundation.org" <gregkh@linuxfoundation.org>
To:     HyungJae Im <hj2.im@samsung.com>
Cc:     "manivannan.sadhasivam@linaro.org" <manivannan.sadhasivam@linaro.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        "linux-input@vger.kernel.org" <linux-input@vger.kernel.org>,
        "rydberg@bitmath.org" <rydberg@bitmath.org>,
        Jungrae Kim <jryu.kim@samsung.com>
Subject: Re: [PATCH v2] input: add 2 kind of switch
Message-ID: <20201029135715.GB3470996@kroah.com>
References: <CGME20201029132747epcms1p8fae559dff47bf0eebdcc9f94efd9a1bf@epcms1p8>
 <20201029132747epcms1p8fae559dff47bf0eebdcc9f94efd9a1bf@epcms1p8>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20201029132747epcms1p8fae559dff47bf0eebdcc9f94efd9a1bf@epcms1p8>
Precedence: bulk

On Thu, Oct 29, 2020 at 10:27:47PM +0900, HyungJae Im wrote:
> From: "hj2.im" <hj2.im@samsung.com>
> Date: Thu, 29 Oct 2020 22:11:24 +0900
> Subject: [PATCH v2] input: add 2 kind of switch

Why is this in the body of that patch?

> 
> We need support to various accessories on the device,
> some switch does not exist in switch list.
> So added switch for the following purpose.
> 
> SW_COVER_ATTACHED is for the checking the cover
> attached or not on the device. SW_EXT_PEN_ATTACHED is for the
> checking the external pen attached or not on the device

You didn't answer the previous question as to why the existing values do
not work for you instead of having to create new ones?


> 
> Signed-off-by: Hyungjae Im <hj2.im@samsung.com>
> ---
>  drivers/input/Kconfig                  |  20 ++
>  drivers/input/Makefile                 |   3 +
>  drivers/input/cover_detect.c           | 242 ++++++++++++++++++++++++
>  drivers/input/ext_pen_detect.c         | 243 +++++++++++++++++++++++++
>  include/linux/mod_devicetable.h        |   2 +-
>  include/uapi/linux/input-event-codes.h |   4 +-
>  6 files changed, 512 insertions(+), 2 deletions(-)
>  create mode 100644 drivers/input/cover_detect.c
>  create mode 100644 drivers/input/ext_pen_detect.c

If this is v2, what changed from v1?

And this is 2 different drivers, it should be 2 different patches at the
least, right?>

> 
> diff --git a/drivers/input/Kconfig b/drivers/input/Kconfig
> index 1efd3154b68d..df902f4a549e 100644
> --- a/drivers/input/Kconfig
> +++ b/drivers/input/Kconfig
> @@ -185,6 +185,26 @@ config INPUT_APMPOWER
>  	  To compile this driver as a module, choose M here: the
>  	  module will be called apm-power.
>  
> +config COVER_DETECT

INPUT_COVER_DETECT?

> +	tristate "Enable cover attach detection"
> +	default n

"default n" is always the default, no need for this here.

> +	help
> +	  Say Y here to enable cover attach detection
> +	  and send a event when cover is attached/detached.
> +	  Active gpio state is low and active event value is 0.
> +
> +	  If unsure, say N.

What is the module name?

> +
> +config EXT_PEN_DETECT

INPUT_EXT_PEN_DETECT?

> +	tristate "Enable external pen attach detection"
> +	default n

No default n.

> +	help
> +	  Say Y here to enable external pen attach detection
> +	  and send a event when external pen is attached/detached.
> +	  Active gpio state is low and active event value is 0.
> +
> +	  If unsure, say N.

What is the module name?

> +
>  comment "Input Device Drivers"
>  
>  source "drivers/input/keyboard/Kconfig"
> diff --git a/drivers/input/Makefile b/drivers/input/Makefile
> index e35650930371..31ee1f2d2e21 100644
> --- a/drivers/input/Makefile
> +++ b/drivers/input/Makefile
> @@ -29,3 +29,6 @@ obj-$(CONFIG_INPUT_MISC)	+= misc/
>  obj-$(CONFIG_INPUT_APMPOWER)	+= apm-power.o
>  
>  obj-$(CONFIG_RMI4_CORE)		+= rmi4/
> +
> +obj-$(CONFIG_COVER_DETECT)	+= cover_detect.o
> +obj-$(CONFIG_EXT_PEN_DETECT)	+= ext_pen_detect.o
> diff --git a/drivers/input/cover_detect.c b/drivers/input/cover_detect.c
> new file mode 100644
> index 000000000000..4d3d68c616ec
> --- /dev/null
> +++ b/drivers/input/cover_detect.c
> @@ -0,0 +1,242 @@
> +/*
> + * Copyright (C) 2015 Samsung Electronics Co. Ltd. All Rights Reserved.

Please use a SPDX line, and no need for this:

> + *
> + * This software is licensed under the terms of the GNU General Public
> + * License version 2, as published by the Free Software Foundation, and
> + * may be copied, distributed, and modified under those terms.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.

That can be removed.

Also your copyright line is wrong, unless you really have not touched
this file in 5 years.

Same comments on the other file.

thanks,

greg k-h