[PATCH v2] input: add 2 kind of switch

[PATCH v2] input: add 2 kind of switch

[HyungJae Im]

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

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

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

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
+	tristate "Enable cover attach detection"
+	default n
+	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.
+
+config EXT_PEN_DETECT
+	tristate "Enable external pen attach detection"
+	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.
+
 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.
+ *
+ * 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.
+ *
+ */
+
+#include <linux/module.h>
+#include <linux/interrupt.h>
+#include <linux/pm.h>
+#include <linux/slab.h>
+#include <linux/platform_device.h>
+#include <linux/input.h>
+#include <linux/gpio.h>
+#include <linux/of_gpio.h>
+#include <linux/wakelock.h>
+
+struct cover_detect_drvdata {
+	struct input_dev *input;
+	struct delayed_work cover_detect_dwork;
+	struct wakeup_source *ws;
+	int gpio_cover_detect;
+	int irq_cover_detect;
+};
+
+static void cover_detect_work(struct work_struct *work)
+{
+	struct cover_detect_drvdata *ddata =
+		container_of(work, struct cover_detect_drvdata,
+				cover_detect_dwork.work);
+	bool cover_status;
+
+	cover_status = gpio_get_value(ddata->gpio_cover_detect);
+
+	input_report_switch(ddata->input,
+			SW_COVER_ATTACHED, cover_status);
+	input_sync(ddata->input);
+}
+
+static void __cover_detect(struct cover_detect_drvdata *ddata,
+				 bool cover_status)
+{
+	cancel_delayed_work_sync(&ddata->cover_detect_dwork);
+	if (cover_status) {
+		__pm_wakeup_event(ddata->ws, jiffies_to_msecs(HZ / 20));
+		schedule_delayed_work(&ddata->cover_detect_dwork, HZ * 1 / 100);
+	} else {
+		__pm_relax(ddata->ws);
+		schedule_delayed_work(&ddata->cover_detect_dwork, 0);
+	}
+}
+
+static irqreturn_t cover_detect_irq(int irq, void *dev_id)
+{
+	bool cover_status;
+	struct cover_detect_drvdata *ddata = dev_id;
+
+	cover_status = gpio_get_value(ddata->gpio_cover_detect);
+
+	__cover_detect(ddata, cover_status);
+
+	return IRQ_HANDLED;
+}
+
+static int cover_detect_open(struct input_dev *input)
+{
+	struct cover_detect_drvdata *ddata = input_get_drvdata(input);
+	/* update the current status */
+	schedule_delayed_work(&ddata->cover_detect_dwork, HZ / 2);
+	/* Report current state of buttons that are connected to GPIOs */
+	input_sync(input);
+
+	return 0;
+}
+
+static void cover_detect_close(struct input_dev *input)
+{
+}
+
+static void init_cover_detect_irq(struct input_dev *input)
+{
+	struct cover_detect_drvdata *ddata = input_get_drvdata(input);
+
+	int ret = 0;
+	int irq = ddata->irq_cover_detect;
+
+	ret =	request_threaded_irq(
+		irq, NULL,
+		cover_detect_irq,
+		IRQF_TRIGGER_RISING |
+		IRQF_TRIGGER_FALLING | IRQF_ONESHOT,
+		"cover_detect", ddata);
+	if (ret < 0)
+		pr_err("keys: failed to request cover detect irq %d gpio %d\n",
+			irq, ddata->gpio_cover_detect);
+}
+
+#ifdef CONFIG_OF
+static int of_cover_detect_data_parsing_dt(struct device *dev,
+					struct cover_detect_drvdata *ddata)
+{
+	struct device_node *np = dev->of_node;
+	int gpio;
+	enum of_gpio_flags flags;
+
+	gpio = of_get_named_gpio_flags(np, "gpio_cover_detect", 0, &flags);
+	ddata->gpio_cover_detect = gpio;
+
+	ddata->irq_cover_detect = gpio_to_irq(gpio);
+
+	return 0;
+}
+#endif
+
+static int cover_detect_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct cover_detect_drvdata *ddata;
+	struct input_dev *input;
+	int error;
+	int wakeup = 0;
+
+	ddata = kzalloc(sizeof(struct cover_detect_drvdata), GFP_KERNEL);
+	if (!ddata)
+		return -ENOMEM;
+
+#ifdef CONFIG_OF
+	if (dev->of_node) {
+		error = of_cover_detect_data_parsing_dt(dev, ddata);
+		if (error < 0) {
+			dev_err(dev, "fail to get the devicetree, error: %d\n",
+					error);
+			goto fail1;
+		}
+	}
+#endif
+
+	input = input_allocate_device();
+	if (!input) {
+		dev_err(dev, "failed to allocate state\n");
+		error = -ENOMEM;
+		goto fail1;
+	}
+
+	ddata->input = input;
+
+	ddata->ws = wakeup_source_register(NULL, "cover_detect");
+
+	platform_set_drvdata(pdev, ddata);
+	input_set_drvdata(input, ddata);
+
+	input->name = "cover_detect";
+	input->phys = "cover_detect";
+	input->dev.parent = &pdev->dev;
+
+	input->evbit[0] |= BIT_MASK(EV_SW);
+	input_set_capability(input, EV_SW, SW_COVER_ATTACHED);
+
+	input->open = cover_detect_open;
+	input->close = cover_detect_close;
+
+	/* Enable auto repeat feature of Linux input subsystem */
+	__set_bit(EV_REP, input->evbit);
+
+	INIT_DELAYED_WORK(&ddata->cover_detect_dwork, cover_detect_work);
+
+	init_cover_detect_irq(input);
+
+	if (ddata->gpio_cover_detect != 0) {
+		error = input_register_device(input);
+		if (error) {
+			dev_err(dev, "Unable to register input device, error: %d\n",
+				error);
+			goto fail1;
+		}
+	}
+
+	device_init_wakeup(&pdev->dev, wakeup);
+
+	return 0;
+
+ fail1:
+	kfree(ddata);
+
+	return error;
+}
+
+static int cover_detect_remove(struct platform_device *pdev)
+{
+	struct cover_detect_drvdata *ddata = platform_get_drvdata(pdev);
+	struct input_dev *input = ddata->input;
+
+	device_init_wakeup(&pdev->dev, 0);
+
+	input_unregister_device(input);
+
+	wakeup_source_unregister(ddata->ws);
+
+	kfree(ddata);
+
+	return 0;
+}
+
+#if defined(CONFIG_OF)
+static const struct of_device_id cover_detect_dt_ids[] = {
+	{ .compatible = "cover_detect" },
+	{ },
+};
+MODULE_DEVICE_TABLE(of, cover_detect_dt_ids);
+#endif /* CONFIG_OF */
+
+static struct platform_driver cover_detect_device_driver = {
+	.probe		= cover_detect_probe,
+	.remove		= cover_detect_remove,
+	.driver		= {
+		.name	= "cover_detect",
+		.owner	= THIS_MODULE,
+#if defined(CONFIG_OF)
+		.of_match_table	= cover_detect_dt_ids,
+#endif /* CONFIG_OF */
+	}
+};
+
+static int __init cover_detect_init(void)
+{
+	return platform_driver_register(&cover_detect_device_driver);
+}
+
+static void __exit cover_detect_exit(void)
+{
+	platform_driver_unregister(&cover_detect_device_driver);
+}
+
+module_init(cover_detect_init);
+module_exit(cover_detect_exit);
diff --git a/drivers/input/ext_pen_detect.c b/drivers/input/ext_pen_detect.c
new file mode 100644
index 000000000000..3595b967718b
--- /dev/null
+++ b/drivers/input/ext_pen_detect.c
@@ -0,0 +1,243 @@
+/*
+ * Copyright (C) 2015 Samsung Electronics Co. Ltd. All Rights Reserved.
+ *
+ * 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.
+ *
+ */
+
+#include <linux/module.h>
+#include <linux/interrupt.h>
+#include <linux/pm.h>
+#include <linux/slab.h>
+#include <linux/platform_device.h>
+#include <linux/input.h>
+#include <linux/gpio.h>
+#include <linux/of_gpio.h>
+#include <linux/wakelock.h>
+
+struct ext_pen_detect_drvdata {
+	struct input_dev *input;
+	struct delayed_work ext_pen_detect_dwork;
+	struct wakeup_source *ws;
+	int gpio_ext_pen_detect;
+	int irq_ext_pen_detect;
+};
+
+static void ext_pen_detect_work(struct work_struct *work)
+{
+	struct ext_pen_detect_drvdata *ddata =
+		container_of(work, struct ext_pen_detect_drvdata,
+				ext_pen_detect_dwork.work);
+	bool ext_pen_status;
+
+	ext_pen_status = gpio_get_value(ddata->gpio_ext_pen_detect);
+
+	input_report_switch(ddata->input,
+			SW_EXT_PEN_ATTACHED, ext_pen_status);
+	input_sync(ddata->input);
+}
+
+static void __ext_pen_detect(struct ext_pen_detect_drvdata *ddata,
+				bool ext_pen_status)
+{
+	cancel_delayed_work_sync(&ddata->ext_pen_detect_dwork);
+	if (ext_pen_status) {
+		__pm_wakeup_event(ddata->ws, jiffies_to_msecs(HZ / 20));
+		schedule_delayed_work(&ddata->ext_pen_detect_dwork,
+					HZ * 1 / 100);
+	} else {
+		__pm_relax(ddata->ws);
+		schedule_delayed_work(&ddata->ext_pen_detect_dwork, 0);
+	}
+}
+
+static irqreturn_t ext_pen_detect_irq(int irq, void *dev_id)
+{
+	bool ext_pen_status;
+	struct ext_pen_detect_drvdata *ddata = dev_id;
+
+	ext_pen_status = gpio_get_value(ddata->gpio_ext_pen_detect);
+
+	__ext_pen_detect(ddata, ext_pen_status);
+
+	return IRQ_HANDLED;
+}
+
+static int ext_pen_detect_open(struct input_dev *input)
+{
+	struct ext_pen_detect_drvdata *ddata = input_get_drvdata(input);
+	/* update the current status */
+	schedule_delayed_work(&ddata->ext_pen_detect_dwork, HZ / 2);
+	/* Report current state of buttons that are connected to GPIOs */
+	input_sync(input);
+
+	return 0;
+}
+
+static void ext_pen_detect_close(struct input_dev *input)
+{
+}
+
+static void init_ext_pen_detect_irq(struct input_dev *input)
+{
+	struct ext_pen_detect_drvdata *ddata = input_get_drvdata(input);
+
+	int ret = 0;
+	int irq = ddata->irq_ext_pen_detect;
+
+	ret =	request_threaded_irq(
+		irq, NULL,
+		ext_pen_detect_irq,
+		IRQF_TRIGGER_RISING |
+		IRQF_TRIGGER_FALLING | IRQF_ONESHOT,
+		"ext_pen_detect", ddata);
+	if (ret < 0)
+		pr_err("keys: failed to request ext_pen detect irq %d gpio %d\n",
+				irq, ddata->gpio_ext_pen_detect);
+}
+
+#ifdef CONFIG_OF
+static int of_ext_pen_detect_data_parsing_dt(struct device *dev,
+					struct ext_pen_detect_drvdata *ddata)
+{
+	struct device_node *np = dev->of_node;
+	int gpio;
+	enum of_gpio_flags flags;
+
+	gpio = of_get_named_gpio_flags(np, "gpio_ext_pen_detect", 0, &flags);
+	ddata->gpio_ext_pen_detect = gpio;
+
+	ddata->irq_ext_pen_detect = gpio_to_irq(gpio);
+
+	return 0;
+}
+#endif
+
+static int ext_pen_detect_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct ext_pen_detect_drvdata *ddata;
+	struct input_dev *input;
+	int error;
+	int wakeup = 0;
+
+	ddata = kzalloc(sizeof(struct ext_pen_detect_drvdata), GFP_KERNEL);
+	if (!ddata)
+		return -ENOMEM;
+
+#ifdef CONFIG_OF
+	if (dev->of_node) {
+		error = of_ext_pen_detect_data_parsing_dt(dev, ddata);
+		if (error < 0) {
+			dev_err(dev, "fail to get the devicetree, error: %d\n",
+					error);
+			goto fail1;
+		}
+	}
+#endif
+
+	input = input_allocate_device();
+	if (!input) {
+		dev_err(dev, "failed to allocate state\n");
+		error = -ENOMEM;
+		goto fail1;
+	}
+
+	ddata->input = input;
+
+	ddata->ws = wakeup_source_register(NULL, "ext_pen_detect");
+
+	platform_set_drvdata(pdev, ddata);
+	input_set_drvdata(input, ddata);
+
+	input->name = "ext_pen_detect";
+	input->phys = "ext_pen_detect";
+	input->dev.parent = &pdev->dev;
+
+	input->evbit[0] |= BIT_MASK(EV_SW);
+	input_set_capability(input, EV_SW, SW_EXT_PEN_ATTACHED);
+
+	input->open = ext_pen_detect_open;
+	input->close = ext_pen_detect_close;
+
+	/* Enable auto repeat feature of Linux input subsystem */
+	__set_bit(EV_REP, input->evbit);
+
+	INIT_DELAYED_WORK(&ddata->ext_pen_detect_dwork, ext_pen_detect_work);
+
+	init_ext_pen_detect_irq(input);
+
+	if (ddata->gpio_ext_pen_detect != 0) {
+		error = input_register_device(input);
+		if (error) {
+			dev_err(dev, "Unable to register input device, error: %d\n",
+				error);
+			goto fail1;
+		}
+	}
+
+	device_init_wakeup(&pdev->dev, wakeup);
+
+	return 0;
+
+ fail1:
+	kfree(ddata);
+
+	return error;
+}
+
+static int ext_pen_detect_remove(struct platform_device *pdev)
+{
+	struct ext_pen_detect_drvdata *ddata = platform_get_drvdata(pdev);
+	struct input_dev *input = ddata->input;
+
+	device_init_wakeup(&pdev->dev, 0);
+
+	input_unregister_device(input);
+
+	wakeup_source_unregister(ddata->ws);
+
+	kfree(ddata);
+
+	return 0;
+}
+
+#if defined(CONFIG_OF)
+static const struct of_device_id ext_pen_detect_dt_ids[] = {
+	{ .compatible = "ext_pen_detect" },
+	{ },
+};
+MODULE_DEVICE_TABLE(of, ext_pen_detect_dt_ids);
+#endif /* CONFIG_OF */
+
+static struct platform_driver ext_pen_detect_device_driver = {
+	.probe		= ext_pen_detect_probe,
+	.remove		= ext_pen_detect_remove,
+	.driver		= {
+		.name	= "ext_pen_detect",
+		.owner	= THIS_MODULE,
+#if defined(CONFIG_OF)
+		.of_match_table	= ext_pen_detect_dt_ids,
+#endif /* CONFIG_OF */
+	}
+};
+
+static int __init ext_pen_detect_init(void)
+{
+	return platform_driver_register(&ext_pen_detect_device_driver);
+}
+
+static void __exit ext_pen_detect_exit(void)
+{
+	platform_driver_unregister(&ext_pen_detect_device_driver);
+}
+
+module_init(ext_pen_detect_init);
+module_exit(ext_pen_detect_exit);
diff --git a/include/linux/mod_devicetable.h b/include/linux/mod_devicetable.h
index 5b08a473cdba..897f5a3e7721 100644
--- a/include/linux/mod_devicetable.h
+++ b/include/linux/mod_devicetable.h
@@ -320,7 +320,7 @@ struct pcmcia_device_id {
 #define INPUT_DEVICE_ID_LED_MAX		0x0f
 #define INPUT_DEVICE_ID_SND_MAX		0x07
 #define INPUT_DEVICE_ID_FF_MAX		0x7f
-#define INPUT_DEVICE_ID_SW_MAX		0x10
+#define INPUT_DEVICE_ID_SW_MAX		0x12
 #define INPUT_DEVICE_ID_PROP_MAX	0x1f
 
 #define INPUT_DEVICE_ID_MATCH_BUS	1
diff --git a/include/uapi/linux/input-event-codes.h b/include/uapi/linux/input-event-codes.h
index 0c2e27d28e0a..8ca2acee1f92 100644
--- a/include/uapi/linux/input-event-codes.h
+++ b/include/uapi/linux/input-event-codes.h
@@ -889,7 +889,9 @@
 #define SW_MUTE_DEVICE		0x0e  /* set = device disabled */
 #define SW_PEN_INSERTED		0x0f  /* set = pen inserted */
 #define SW_MACHINE_COVER	0x10  /* set = cover closed */
-#define SW_MAX			0x10
+#define SW_COVER_ATTACHED	0x11  /* set = cover attached */
+#define SW_EXT_PEN_ATTACHED	0x12  /* set = external pen attached */
+#define SW_MAX			0x12
 #define SW_CNT			(SW_MAX+1)
 
 /*
-- 
2.17.1

Re: [PATCH v2] input: add 2 kind of switch

[Barnabás Pőcze]

Hi

> [...]
> diff --git a/include/linux/mod_devicetable.h b/include/linux/mod_devicetable.h
> index 5b08a473cdba..897f5a3e7721 100644
> --- a/include/linux/mod_devicetable.h
> +++ b/include/linux/mod_devicetable.h
> @@ -320,7 +320,7 @@ struct pcmcia_device_id {
>  #define INPUT_DEVICE_ID_LED_MAX		0x0f
>  #define INPUT_DEVICE_ID_SND_MAX		0x07
>  #define INPUT_DEVICE_ID_FF_MAX		0x7f
> -#define INPUT_DEVICE_ID_SW_MAX		0x10
> +#define INPUT_DEVICE_ID_SW_MAX		0x12
>  #define INPUT_DEVICE_ID_PROP_MAX	0x1f
>
>  #define INPUT_DEVICE_ID_MATCH_BUS	1
> diff --git a/include/uapi/linux/input-event-codes.h b/include/uapi/linux/input-event-codes.h
> index 0c2e27d28e0a..8ca2acee1f92 100644
> --- a/include/uapi/linux/input-event-codes.h
> +++ b/include/uapi/linux/input-event-codes.h
> @@ -889,7 +889,9 @@
>  #define SW_MUTE_DEVICE		0x0e  /* set = device disabled */
>  #define SW_PEN_INSERTED		0x0f  /* set = pen inserted */
>  #define SW_MACHINE_COVER	0x10  /* set = cover closed */
> -#define SW_MAX			0x10
> +#define SW_COVER_ATTACHED	0x11  /* set = cover attached */
> +#define SW_EXT_PEN_ATTACHED	0x12  /* set = external pen attached */
> +#define SW_MAX			0x12
>  #define SW_CNT			(SW_MAX+1)
> [...]

This part of the patch conflicts with another one:
https://lore.kernel.org/linux-input/20201026144512.621479-1-markpearson@lenovo.com/


Regards,
Barnabás Pőcze

Re: [PATCH v2] input: add 2 kind of switch

[gregkh]

On Thu, Oct 29, 2020 at 01:41:57PM +0000, Barnabás Pőcze wrote:
> Hi
> 
> > [...]
> > diff --git a/include/linux/mod_devicetable.h b/include/linux/mod_devicetable.h
> > index 5b08a473cdba..897f5a3e7721 100644
> > --- a/include/linux/mod_devicetable.h
> > +++ b/include/linux/mod_devicetable.h
> > @@ -320,7 +320,7 @@ struct pcmcia_device_id {
> >  #define INPUT_DEVICE_ID_LED_MAX		0x0f
> >  #define INPUT_DEVICE_ID_SND_MAX		0x07
> >  #define INPUT_DEVICE_ID_FF_MAX		0x7f
> > -#define INPUT_DEVICE_ID_SW_MAX		0x10
> > +#define INPUT_DEVICE_ID_SW_MAX		0x12
> >  #define INPUT_DEVICE_ID_PROP_MAX	0x1f
> >
> >  #define INPUT_DEVICE_ID_MATCH_BUS	1
> > diff --git a/include/uapi/linux/input-event-codes.h b/include/uapi/linux/input-event-codes.h
> > index 0c2e27d28e0a..8ca2acee1f92 100644
> > --- a/include/uapi/linux/input-event-codes.h
> > +++ b/include/uapi/linux/input-event-codes.h
> > @@ -889,7 +889,9 @@
> >  #define SW_MUTE_DEVICE		0x0e  /* set = device disabled */
> >  #define SW_PEN_INSERTED		0x0f  /* set = pen inserted */
> >  #define SW_MACHINE_COVER	0x10  /* set = cover closed */
> > -#define SW_MAX			0x10
> > +#define SW_COVER_ATTACHED	0x11  /* set = cover attached */
> > +#define SW_EXT_PEN_ATTACHED	0x12  /* set = external pen attached */
> > +#define SW_MAX			0x12
> >  #define SW_CNT			(SW_MAX+1)
> > [...]
> 
> This part of the patch conflicts with another one:
> https://lore.kernel.org/linux-input/20201026144512.621479-1-markpearson@lenovo.com/

Is that merged?  If not, it's fine as-is until then, and someone has to
pick to go first :)

But, most importantly, the questions asked last time about this patch
have not been addressed at all :(

thanks,

greg k-h

Re: [PATCH v2] input: add 2 kind of switch

[gregkh]

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

Re: [PATCH v2] input: add 2 kind of switch

[Barnabás Pőcze]

Hi

> [...]
> > > diff --git a/include/linux/mod_devicetable.h b/include/linux/mod_devicetable.h
> > > index 5b08a473cdba..897f5a3e7721 100644
> > > --- a/include/linux/mod_devicetable.h
> > > +++ b/include/linux/mod_devicetable.h
> > > @@ -320,7 +320,7 @@ struct pcmcia_device_id {
> > >  #define INPUT_DEVICE_ID_LED_MAX		0x0f
> > >  #define INPUT_DEVICE_ID_SND_MAX		0x07
> > >  #define INPUT_DEVICE_ID_FF_MAX		0x7f
> > > -#define INPUT_DEVICE_ID_SW_MAX		0x10
> > > +#define INPUT_DEVICE_ID_SW_MAX		0x12
> > >  #define INPUT_DEVICE_ID_PROP_MAX	0x1f
> > >
> > >  #define INPUT_DEVICE_ID_MATCH_BUS	1
> > > diff --git a/include/uapi/linux/input-event-codes.h b/include/uapi/linux/input-event-codes.h
> > > index 0c2e27d28e0a..8ca2acee1f92 100644
> > > --- a/include/uapi/linux/input-event-codes.h
> > > +++ b/include/uapi/linux/input-event-codes.h
> > > @@ -889,7 +889,9 @@
> > >  #define SW_MUTE_DEVICE		0x0e  /* set = device disabled */
> > >  #define SW_PEN_INSERTED		0x0f  /* set = pen inserted */
> > >  #define SW_MACHINE_COVER	0x10  /* set = cover closed */
> > > -#define SW_MAX			0x10
> > > +#define SW_COVER_ATTACHED	0x11  /* set = cover attached */
> > > +#define SW_EXT_PEN_ATTACHED	0x12  /* set = external pen attached */
> > > +#define SW_MAX			0x12
> > >  #define SW_CNT			(SW_MAX+1)
> > > [...]
> >
> > This part of the patch conflicts with another one:
> > https://lore.kernel.org/linux-input/20201026144512.621479-1-markpearson@lenovo.com/
>
> Is that merged?  If not, it's fine as-is until then, and someone has to
> pick to go first :)

It is not, to my knowledge. Nonetheless I figured the information may be relevant.


Regards,
Barnabás Pőcze

Re: [PATCH v2] input: add 2 kind of switch

[Dmitry Torokhov]

On Thu, Oct 29, 2020 at 02:57:15PM +0100, gregkh@linuxfoundation.org wrote:
> 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?>

Actually the should simply use gpio-keys.c for this and dispense with
the custom drivers.

Thanks.

-- 
Dmitry

RE:(2) [PATCH v2] input: add 2 kind of switch

[HyungJae Im]

Hello, This is Hyungjae Im from Samsung Electronics.
Let me answer your questions inline.

>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?

I read "how to send your first kernel patch", but still making so many mistakes.
I will be cautious with this.
 
>> 
>> 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?

 I think I should clarify this part the most for this review.
 As you know, new added events both has similar existing events,
 but it has to operate separately.

 First, SW_COVER_ATTACHED is similar with SW_MACHINE_COVER.
 We need two events for our cover interaction.
 One is to detect if flip cover is open/closed(covers screen or not),
 and one is for detecting if cover is attached(detect if device is put into cover).
 With the second event, we send event for attachment and start authentication
 distinguishing if it was Samsung made cover.

 Second, SW_EXT_PEN_ATTACHED detects if pen is attached externally on tablet models.
 It is different with SW_PEN_INSERTED since this is detecting pens like our NOTE series.
 SW_EXT_PEN_ATTACHED has an unique role to set wacom tuning table differently
 while pen is attached/detached.

 
>> 
>> 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?>

V2 has additional submit for drivers using SW events.
I will separate these two drivers and submit it separately.
 
>> 
>> 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?

 Will change feature name to INPUT_COVER_DETECT.
 
>> +        tristate "Enable cover attach detection"
>> +        default n
 
>"default n" is always the default, no need for this here.

 Thanks for comment. I will erase it.
 
>> +        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?

 I will add additional explanation for 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
 
 I will change it. Thanks

best regard,
HJ

Re: (2) [PATCH v2] input: add 2 kind of switch

[gregkh]

On Fri, Oct 30, 2020 at 01:39:16PM +0900, HyungJae Im wrote:
> Hello, This is Hyungjae Im from Samsung Electronics.
> Let me answer your questions inline.
> 
> >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?
> 
> I read "how to send your first kernel patch", but still making so many mistakes.
> I will be cautious with this.
>  
> >> 
> >> 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?
> 
>  I think I should clarify this part the most for this review.
>  As you know, new added events both has similar existing events,
>  but it has to operate separately.
> 
>  First, SW_COVER_ATTACHED is similar with SW_MACHINE_COVER.
>  We need two events for our cover interaction.
>  One is to detect if flip cover is open/closed(covers screen or not),
>  and one is for detecting if cover is attached(detect if device is put into cover).
>  With the second event, we send event for attachment and start authentication
>  distinguishing if it was Samsung made cover.
> 
>  Second, SW_EXT_PEN_ATTACHED detects if pen is attached externally on tablet models.
>  It is different with SW_PEN_INSERTED since this is detecting pens like our NOTE series.
>  SW_EXT_PEN_ATTACHED has an unique role to set wacom tuning table differently
>  while pen is attached/detached.

All of that needs to go in the changelog text for the individual patches
when you submit them.

But as Dmitry pointed out, it doesn't look like either of these drivers
are needed at all, just use the gpio-keys driver instead.

thanks,

greg k-h

Re: (3) [PATCH v2] input: add 2 kind of switch

[Jungrae Kim]

> On Fri, Oct 30, 2020 at 01:39:16PM +0900, HyungJae Im wrote:
> > Hello, This is Hyungjae Im from Samsung Electronics.
> > Let me answer your questions inline.
> > 
> > >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?
> > 
> > I read "how to send your first kernel patch", but still making so many mistakes.
> > I will be cautious with this.
> >  
> > >> 
> > >> 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?
> > 
> >  I think I should clarify this part the most for this review.
> >  As you know, new added events both has similar existing events,
> >  but it has to operate separately.
> > 
> >  First, SW_COVER_ATTACHED is similar with SW_MACHINE_COVER.
> >  We need two events for our cover interaction.
> >  One is to detect if flip cover is open/closed(covers screen or not),
> >  and one is for detecting if cover is attached(detect if device is put into cover).
> >  With the second event, we send event for attachment and start authentication
> >  distinguishing if it was Samsung made cover.
> > 
> >  Second, SW_EXT_PEN_ATTACHED detects if pen is attached externally on tablet models.
> >  It is different with SW_PEN_INSERTED since this is detecting pens like our NOTE series.
> >  SW_EXT_PEN_ATTACHED has an unique role to set wacom tuning table differently
> >  while pen is attached/detached.
>  
> All of that needs to go in the changelog text for the individual patches
> when you submit them.
>  
> But as Dmitry pointed out, it doesn't look like either of these drivers
> are needed at all, just use the gpio-keys driver instead.
>  
> thanks,
>  
> greg k-h
 
Can you accept V1 patch? or need to add a change of device tree?
Please let me know what do I do now. 

Re: (3) [PATCH v2] input: add 2 kind of switch

[gregkh]

On Fri, Oct 30, 2020 at 08:28:12PM +0900, Jungrae Kim wrote:
> > On Fri, Oct 30, 2020 at 01:39:16PM +0900, HyungJae Im wrote:
> > > Hello, This is Hyungjae Im from Samsung Electronics.
> > > Let me answer your questions inline.
> > > 
> > > >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?
> > > 
> > > I read "how to send your first kernel patch", but still making so many mistakes.
> > > I will be cautious with this.
> > >  
> > > >> 
> > > >> 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?
> > > 
> > >  I think I should clarify this part the most for this review.
> > >  As you know, new added events both has similar existing events,
> > >  but it has to operate separately.
> > > 
> > >  First, SW_COVER_ATTACHED is similar with SW_MACHINE_COVER.
> > >  We need two events for our cover interaction.
> > >  One is to detect if flip cover is open/closed(covers screen or not),
> > >  and one is for detecting if cover is attached(detect if device is put into cover).
> > >  With the second event, we send event for attachment and start authentication
> > >  distinguishing if it was Samsung made cover.
> > > 
> > >  Second, SW_EXT_PEN_ATTACHED detects if pen is attached externally on tablet models.
> > >  It is different with SW_PEN_INSERTED since this is detecting pens like our NOTE series.
> > >  SW_EXT_PEN_ATTACHED has an unique role to set wacom tuning table differently
> > >  while pen is attached/detached.
> >  
> > All of that needs to go in the changelog text for the individual patches
> > when you submit them.
> >  
> > But as Dmitry pointed out, it doesn't look like either of these drivers
> > are needed at all, just use the gpio-keys driver instead.
> >  
> > thanks,
> >  
> > greg k-h
>  
> Can you accept V1 patch? or need to add a change of device tree?

What is "v1" patch?  Do you have a pointer to it on lore.kernel.org?

> Please let me know what do I do now. 

What is wrong with just using a device tree entry for the gpio-keys
driver instead?

thanks,

greg k-h

RE: Re: (3) [PATCH v2] input: add 2 kind of switch

[Jungrae Kim]

> On Fri, Oct 30, 2020 at 08:28:12PM +0900, Jungrae Kim wrote:
> > > On Fri, Oct 30, 2020 at 01:39:16PM +0900, HyungJae Im wrote:
> > > > Hello, This is Hyungjae Im from Samsung Electronics.
> > > > Let me answer your questions inline.
> > > > 
> > > > >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?
> > > > 
> > > > I read "how to send your first kernel patch", but still making so many mistakes.
> > > > I will be cautious with this.
> > > >  
> > > > >> 
> > > > >> 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?
> > > > 
> > > >  I think I should clarify this part the most for this review.
> > > >  As you know, new added events both has similar existing events,
> > > >  but it has to operate separately.
> > > > 
> > > >  First, SW_COVER_ATTACHED is similar with SW_MACHINE_COVER.
> > > >  We need two events for our cover interaction.
> > > >  One is to detect if flip cover is open/closed(covers screen or not),
> > > >  and one is for detecting if cover is attached(detect if device is put into cover).
> > > >  With the second event, we send event for attachment and start authentication
> > > >  distinguishing if it was Samsung made cover.
> > > > 
> > > >  Second, SW_EXT_PEN_ATTACHED detects if pen is attached externally on tablet models.
> > > >  It is different with SW_PEN_INSERTED since this is detecting pens like our NOTE series.
> > > >  SW_EXT_PEN_ATTACHED has an unique role to set wacom tuning table differently
> > > >  while pen is attached/detached.
> > >  
> > > All of that needs to go in the changelog text for the individual patches
> > > when you submit them.
> > >  
> > > But as Dmitry pointed out, it doesn't look like either of these drivers
> > > are needed at all, just use the gpio-keys driver instead.
> > >  
> > > thanks,
> > >  
> > > greg k-h
> >  
> > Can you accept V1 patch? or need to add a change of device tree?
> 
> What is "v1" patch?  Do you have a pointer to it on lore.kernel.org?
> 
> > Please let me know what do I do now. 
> 
> What is wrong with just using a device tree entry for the gpio-keys
> driver instead?
> 
> thanks,
> 
> greg k-h

V1 Patch : https://lore.kernel.org/lkml/20201021031216epcms1p556d8d7d5d763ec47f67cd8cbe3972935@epcms1p5/

I think do not need modify gpio_keys. And I`m not sure device tree need to added to patch.

Please let me know if you think need more fix than Patch v1.

Thanks
Jungrae Kim

Re: Re: (3) [PATCH v2] input: add 2 kind of switch

[gregkh]

On Fri, Oct 30, 2020 at 08:59:18PM +0900, Jungrae Kim wrote:
> > On Fri, Oct 30, 2020 at 08:28:12PM +0900, Jungrae Kim wrote:
> > > > On Fri, Oct 30, 2020 at 01:39:16PM +0900, HyungJae Im wrote:
> > > > > Hello, This is Hyungjae Im from Samsung Electronics.
> > > > > Let me answer your questions inline.
> > > > > 
> > > > > >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?
> > > > > 
> > > > > I read "how to send your first kernel patch", but still making so many mistakes.
> > > > > I will be cautious with this.
> > > > >  
> > > > > >> 
> > > > > >> 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?
> > > > > 
> > > > >  I think I should clarify this part the most for this review.
> > > > >  As you know, new added events both has similar existing events,
> > > > >  but it has to operate separately.
> > > > > 
> > > > >  First, SW_COVER_ATTACHED is similar with SW_MACHINE_COVER.
> > > > >  We need two events for our cover interaction.
> > > > >  One is to detect if flip cover is open/closed(covers screen or not),
> > > > >  and one is for detecting if cover is attached(detect if device is put into cover).
> > > > >  With the second event, we send event for attachment and start authentication
> > > > >  distinguishing if it was Samsung made cover.
> > > > > 
> > > > >  Second, SW_EXT_PEN_ATTACHED detects if pen is attached externally on tablet models.
> > > > >  It is different with SW_PEN_INSERTED since this is detecting pens like our NOTE series.
> > > > >  SW_EXT_PEN_ATTACHED has an unique role to set wacom tuning table differently
> > > > >  while pen is attached/detached.
> > > >  
> > > > All of that needs to go in the changelog text for the individual patches
> > > > when you submit them.
> > > >  
> > > > But as Dmitry pointed out, it doesn't look like either of these drivers
> > > > are needed at all, just use the gpio-keys driver instead.
> > > >  
> > > > thanks,
> > > >  
> > > > greg k-h
> > >  
> > > Can you accept V1 patch? or need to add a change of device tree?
> > 
> > What is "v1" patch?  Do you have a pointer to it on lore.kernel.org?
> > 
> > > Please let me know what do I do now. 
> > 
> > What is wrong with just using a device tree entry for the gpio-keys
> > driver instead?
> > 
> > thanks,
> > 
> > greg k-h
> 
> V1 Patch : https://lore.kernel.org/lkml/20201021031216epcms1p556d8d7d5d763ec47f67cd8cbe3972935@epcms1p5/

As I said there, that patch is not acceptable for style reasons alone,
nothing we can do with that unless it is fixed, right?

> I think do not need modify gpio_keys. And I`m not sure device tree need to added to patch.

No, you don't need to modify it, just use it.

So what exactly is the issue anymore?  Just use the gpio-keys driver and
all should be fine, right?

thanks,

greg k-h

[PATCH] Input: add SW_COVER_ATTACHED and SW_EXT_PEN_ATTACHED

[Jungrae Kim]

From 23aed4567e234b7e108c31abadb9f3a3ccccf7d2 Mon Sep 17 00:00:00 2001
From: Jungrae Kim <jryu.kim@samsung.com>
Date: Fri, 30 Oct 2020 21:23:12 +0900
Subject: [PATCH] Input: add SW_COVER_ATTACHED and SW_EXT_PEN_ATTACHED

SW_COVER_ATTACHED represents the connected state of a removable cover
of a device. Value 0 means cover was attached with device, value 1 means
removed it.
SW_EXT_PEN_ATTACHED represents the state of the pen.
Some device have internal pen slot. but other some device have external pen
slot. These two cases has different use case in userspace. So need to
separate a event. Value 0 means pen was detach on external pen slot on
device, value 1 means pen was attached external pen slot on device.

Signed-off-by: Jungrae Kim <jryu.kim@samsung.com>
---
 include/linux/mod_devicetable.h        | 2 +-
 include/uapi/linux/input-event-codes.h | 4 +++-
 2 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/include/linux/mod_devicetable.h b/include/linux/mod_devicetable.h
index 5b08a473cdba..897f5a3e7721 100644
--- a/include/linux/mod_devicetable.h
+++ b/include/linux/mod_devicetable.h
@@ -320,7 +320,7 @@ struct pcmcia_device_id {
 #define INPUT_DEVICE_ID_LED_MAX                0x0f
 #define INPUT_DEVICE_ID_SND_MAX                0x07
 #define INPUT_DEVICE_ID_FF_MAX         0x7f
-#define INPUT_DEVICE_ID_SW_MAX         0x10
+#define INPUT_DEVICE_ID_SW_MAX         0x12
 #define INPUT_DEVICE_ID_PROP_MAX       0x1f

 #define INPUT_DEVICE_ID_MATCH_BUS      1
diff --git a/include/uapi/linux/input-event-codes.h b/include/uapi/linux/input-event-codes.h
index ee93428ced9a..a0506369de6d 100644
--- a/include/uapi/linux/input-event-codes.h
+++ b/include/uapi/linux/input-event-codes.h
@@ -893,7 +893,9 @@
 #define SW_MUTE_DEVICE         0x0e  /* set = device disabled */
 #define SW_PEN_INSERTED                0x0f  /* set = pen inserted */
 #define SW_MACHINE_COVER       0x10  /* set = cover closed */
-#define SW_MAX                 0x10
+#define SW_COVER_ATTACHED      0x11  /* set = cover attached */
+#define SW_EXT_PEN_ATTACHED    0x12  /* set = external pen attached */
+#define SW_MAX                 0x12
 #define SW_CNT                 (SW_MAX+1)

 /*
-- 
2.17.1

Re: [PATCH] Input: add SW_COVER_ATTACHED and SW_EXT_PEN_ATTACHED

[Sebastian Reichel]

[-- Attachment #1: Type: text/plain, Size: 2673 bytes --]

Hi,

On Fri, Oct 30, 2020 at 10:15:52PM +0900, Jungrae Kim wrote:
> From 23aed4567e234b7e108c31abadb9f3a3ccccf7d2 Mon Sep 17 00:00:00 2001
> From: Jungrae Kim <jryu.kim@samsung.com>
> Date: Fri, 30 Oct 2020 21:23:12 +0900
> Subject: [PATCH] Input: add SW_COVER_ATTACHED and SW_EXT_PEN_ATTACHED
> 
> SW_COVER_ATTACHED represents the connected state of a removable cover
> of a device. Value 0 means cover was attached with device, value 1 means
> removed it.

Any reason against using SW_MACHINE_COVER? That was introduced for Nokia
N900, where you actually remove the cover to access battery/SD card/
SIM card (so there is state 0 = cover removed/open and state 1 = cover
attached/closed).

-- Sebastian

> SW_EXT_PEN_ATTACHED represents the state of the pen.
> Some device have internal pen slot. but other some device have external pen
> slot. These two cases has different use case in userspace. So need to
> separate a event. Value 0 means pen was detach on external pen slot on
> device, value 1 means pen was attached external pen slot on device.
> 
> Signed-off-by: Jungrae Kim <jryu.kim@samsung.com>
> ---
>  include/linux/mod_devicetable.h        | 2 +-
>  include/uapi/linux/input-event-codes.h | 4 +++-
>  2 files changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/include/linux/mod_devicetable.h b/include/linux/mod_devicetable.h
> index 5b08a473cdba..897f5a3e7721 100644
> --- a/include/linux/mod_devicetable.h
> +++ b/include/linux/mod_devicetable.h
> @@ -320,7 +320,7 @@ struct pcmcia_device_id {
>  #define INPUT_DEVICE_ID_LED_MAX                0x0f
>  #define INPUT_DEVICE_ID_SND_MAX                0x07
>  #define INPUT_DEVICE_ID_FF_MAX         0x7f
> -#define INPUT_DEVICE_ID_SW_MAX         0x10
> +#define INPUT_DEVICE_ID_SW_MAX         0x12
>  #define INPUT_DEVICE_ID_PROP_MAX       0x1f
> 
>  #define INPUT_DEVICE_ID_MATCH_BUS      1
> diff --git a/include/uapi/linux/input-event-codes.h b/include/uapi/linux/input-event-codes.h
> index ee93428ced9a..a0506369de6d 100644
> --- a/include/uapi/linux/input-event-codes.h
> +++ b/include/uapi/linux/input-event-codes.h
> @@ -893,7 +893,9 @@
>  #define SW_MUTE_DEVICE         0x0e  /* set = device disabled */
>  #define SW_PEN_INSERTED                0x0f  /* set = pen inserted */
>  #define SW_MACHINE_COVER       0x10  /* set = cover closed */
> -#define SW_MAX                 0x10
> +#define SW_COVER_ATTACHED      0x11  /* set = cover attached */
> +#define SW_EXT_PEN_ATTACHED    0x12  /* set = external pen attached */
> +#define SW_MAX                 0x12
>  #define SW_CNT                 (SW_MAX+1)
> 
>  /*
> -- 
> 2.17.1

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re:(2) [PATCH] Input: add SW_COVER_ATTACHED and SW_EXT_PEN_ATTACHED

[Jungrae Kim]

> Hi,
> 
> On Fri, Oct 30, 2020 at 10:15:52PM +0900, Jungrae Kim wrote:
> > From 23aed4567e234b7e108c31abadb9f3a3ccccf7d2 Mon Sep 17 00:00:00 2001
> > From: Jungrae Kim <jryu.kim@samsung.com>
> > Date: Fri, 30 Oct 2020 21:23:12 +0900
> > Subject: [PATCH] Input: add SW_COVER_ATTACHED and SW_EXT_PEN_ATTACHED
> > 
> > SW_COVER_ATTACHED represents the connected state of a removable cover
> > of a device. Value 0 means cover was attached with device, value 1 means
> > removed it.
> 
> Any reason against using SW_MACHINE_COVER? That was introduced for Nokia
> N900, where you actually remove the cover to access battery/SD card/
> SIM card (so there is state 0 = cover removed/open and state 1 = cover
> attached/closed).
> 
> -- Sebastian
> 
> > SW_EXT_PEN_ATTACHED represents the state of the pen.
> > Some device have internal pen slot. but other some device have external pen
> > slot. These two cases has different use case in userspace. So need to
> > separate a event. Value 0 means pen was detach on external pen slot on
> > device, value 1 means pen was attached external pen slot on device.
> > 
> > Signed-off-by: Jungrae Kim <jryu.kim@samsung.com>
> > ---
> >  include/linux/mod_devicetable.h        | 2 +-
> >  include/uapi/linux/input-event-codes.h | 4 +++-
> >  2 files changed, 4 insertions(+), 2 deletions(-)
> > 
> > diff --git a/include/linux/mod_devicetable.h b/include/linux/mod_devicetable.h
> > index 5b08a473cdba..897f5a3e7721 100644
> > --- a/include/linux/mod_devicetable.h
> > +++ b/include/linux/mod_devicetable.h
> > @@ -320,7 +320,7 @@ struct pcmcia_device_id {
> >  #define INPUT_DEVICE_ID_LED_MAX                0x0f
> >  #define INPUT_DEVICE_ID_SND_MAX                0x07
> >  #define INPUT_DEVICE_ID_FF_MAX         0x7f
> > -#define INPUT_DEVICE_ID_SW_MAX         0x10
> > +#define INPUT_DEVICE_ID_SW_MAX         0x12
> >  #define INPUT_DEVICE_ID_PROP_MAX       0x1f
> > 
> >  #define INPUT_DEVICE_ID_MATCH_BUS      1
> > diff --git a/include/uapi/linux/input-event-codes.h b/include/uapi/linux/input-event-codes.h
> > index ee93428ced9a..a0506369de6d 100644
> > --- a/include/uapi/linux/input-event-codes.h
> > +++ b/include/uapi/linux/input-event-codes.h
> > @@ -893,7 +893,9 @@
> >  #define SW_MUTE_DEVICE         0x0e  /* set = device disabled */
> >  #define SW_PEN_INSERTED                0x0f  /* set = pen inserted */
> >  #define SW_MACHINE_COVER       0x10  /* set = cover closed */
> > -#define SW_MAX                 0x10
> > +#define SW_COVER_ATTACHED      0x11  /* set = cover attached */
> > +#define SW_EXT_PEN_ATTACHED    0x12  /* set = external pen attached */
> > +#define SW_MAX                 0x12
> >  #define SW_CNT                 (SW_MAX+1)
> > 
> >  /*
> > -- 
> > 2.17.1

We need 2 kind of event, cover open/close and cover attach/detach.
The open/close of the cover must work only if the cover is attached.
So we will check cover open/close status using SW_MACHINE_COVER.

Thanks
Jungrae Kim
                                                                                                                                                                                          68,1          Bot

Re: [PATCH v2] input: add 2 kind of switch

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 1830 bytes --]

Hi HyungJae,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on input/next]
[also build test ERROR on linus/master linux/master v5.10-rc2 next-20201102]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/HyungJae-Im/input-add-2-kind-of-switch/20201029-213108
base:   https://git.kernel.org/pub/scm/linux/kernel/git/dtor/input.git next
config: riscv-randconfig-r036-20201030 (attached as .config)
compiler: riscv64-linux-gcc (GCC) 9.3.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/0day-ci/linux/commit/bf61e9f978d895850b5a30fe6a7308acb2f245d7
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review HyungJae-Im/input-add-2-kind-of-switch/20201029-213108
        git checkout bf61e9f978d895850b5a30fe6a7308acb2f245d7
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross ARCH=riscv 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

>> drivers/input/ext_pen_detect.c:23:10: fatal error: linux/wakelock.h: No such file or directory
      23 | #include <linux/wakelock.h>
         |          ^~~~~~~~~~~~~~~~~~
   compilation terminated.

vim +23 drivers/input/ext_pen_detect.c

  > 23	#include <linux/wakelock.h>
    24	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 32461 bytes --]