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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20201030043916epcms1p3b289ca5cd902883a97e7d13ceb5c1efb@epcms1p3>
Date:   Fri, 30 Oct 2020 13:39:16 +0900
From:   HyungJae Im <hj2.im@...sung.com>
To:     "gregkh@...uxfoundation.org" <gregkh@...uxfoundation.org>,
        HyungJae Im <hj2.im@...sung.com>
CC:     "manivannan.sadhasivam@...aro.org" <manivannan.sadhasivam@...aro.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "linux-input@...r.kernel.org" <linux-input@...r.kernel.org>,
        "rydberg@...math.org" <rydberg@...math.org>,
        Jungrae Kim <jryu.kim@...sung.com>
Subject: RE:(2) [PATCH v2] input: add 2 kind of switch

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