subject:"\[V2,1\/1\] platform\/x86\: add support for Advantech software defined button"

Re: [V2,1/1] platform/x86: add support for Advantech software defined button

2021-03-23 Thread Hans de Goede

Hi,

On 3/19/21 5:00 AM, YingChieh, Ho wrote:
> Hi Hans,
> 
> Thank you for your kindly review.
> As a beginner in driver coding, I got gains much from your suggestions.

I'm glad to hear that my reviews are helping you.

> Path v3 is ready at patchwork.^^

Thank you, I'll try to take a look at it soon.

Regards,

Hans


> 
> Hans de Goede  於 2021年3月19日 週五 上午12:21寫道：
>>
>> Hi,
>>
>> On 3/12/21 9:11 AM, YingChieh Ho wrote:
>>> From: "Andrea.Ho" 
>>>
>>> Advantech sw_button is a ACPI event trigger button.
>>>
>>> With this driver, we can report KEY_EVENT on the
>>> Advantech Tabletop Network Appliances products and it has been
>>> tested in FWA1112VC.
>>>
>>> Add the software define button support to report EV_REP key_event
>>> (KEY_PROG1) by pressing button that cloud be get on user
>>> interface and trigger the customized actions.
>>>
>>> Signed-off-by: Andrea.Ho 
>>>
>>> v2:
>>> - change evdev key-code from BTN_TRIGGER_HAPPY to KEY_PROG1
>>> - to rewrite the driver to be a regular platform_driver
>>> - use specific names instead of generic names,
>>>   "Software Button" to "Advantech Software Button",
>>>   "button" to "adv_swbutton"
>>> - remove unused defines or use-once defines, ACPI_BUTTON_FILE_INFO,
>>>   ACPI_BUTTON_FILE_STATE, ACPI_BUTTON_TYPE_UNKNOWN, ACPI_SWBTN_NAME
>>
>> Thank you this version is much better, I have various review remarks below.
>>
>> Please send a v3 with those fixed.
>>
>>
>>> ---
>>>  MAINTAINERS |   5 +
>>>  drivers/platform/x86/Kconfig|  11 ++
>>>  drivers/platform/x86/Makefile   |   3 +
>>>  drivers/platform/x86/adv_swbutton.c | 192 
>>>  4 files changed, 211 insertions(+)
>>>  create mode 100644 drivers/platform/x86/adv_swbutton.c
>>>
>>> diff --git a/MAINTAINERS b/MAINTAINERS
>>> index 68f21d46614c..e35c48e411b7 100644
>>> --- a/MAINTAINERS
>>> +++ b/MAINTAINERS
>>> @@ -562,6 +562,11 @@ S: Maintained
>>>  F: Documentation/scsi/advansys.rst
>>>  F: drivers/scsi/advansys.c
>>>
>>> +ADVANTECH SWBTN DRIVER
>>> +M: Andrea Ho 
>>> +S: Maintained
>>> +F: drivers/platform/x86/adv_swbutton.c
>>> +
>>>  ADXL34X THREE-AXIS DIGITAL ACCELEROMETER DRIVER (ADXL345/ADXL346)
>>>  M: Michael Hennerich 
>>>  S: Supported
>>> diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
>>> index 0581a54cf562..b1340135c5e9 100644
>>> --- a/drivers/platform/x86/Kconfig
>>> +++ b/drivers/platform/x86/Kconfig
>>> @@ -180,6 +180,17 @@ config ACER_WMI
>>>   If you have an ACPI-WMI compatible Acer/ Wistron laptop, say Y or 
>>> M
>>>   here.
>>
>> This is supposed to be indented by a space (from the diff format) and then
>> a tab + 2 spaces, but in your patch this is now indented by 10 spaces.
>>
>> In general your entire patch has all tabs replaced by spaces, I guess that
>> your mail-client or editor is mangling things. Please do the following:
>>
>> 1. Check your patch for checkpatch errors:
>>
>> git format-patch HEAD~
>> scripts/checkpatch.pl 0001-platform...patch
>>
>> And fix all reported issues, both whitespace issues and others.
>>
>> 2. Send the next version of the patch like this:
>>
>> git format-patch -v3 HEAD~
>> git send-email v3-0001-platform...patch
>>
>> Do NOT edit the v3-0001-platform...patch file between these 2 steps.
>>
>>
>>> +config ADV_SWBUTTON
>>> +tristate "Advantech ACPI Software Button Driver"
>>
>> You are using indent steps of 4 spaces here, that should be a tab
>>> +depends on ACPI
>>> +help
>>> +  Say Y here to enable support for Advantech software defined
>>> +  button feature. More information can be fount at
>>> +  
>>
>> And a tab and 2 spaces for the help text.
>>
>>> +
>>> +  To compile this driver as a module, choose M here. The module will
>>> +  be called adv_swbutton.
>>> +
>>>  config APPLE_GMUX
>>> tristate "Apple Gmux Driver"
>>> depends on ACPI && PCI
>>> diff --git a/drivers/platform/x86/Makefile b/drivers/platform/x86/Makefile
>>> index 2b85852a1a87..76a321fc58ba 100644
>>> --- a/drivers/platform/x86/Makefile
>>> +++ b/drivers/platform/x86/Makefile
>>> @@ -22,6 +22,9 @@ obj-$(CONFIG_ACERHDF) += acerhdf.o
>>>  obj-$(CONFIG_ACER_WIRELESS)+= acer-wireless.o
>>>  obj-$(CONFIG_ACER_WMI) += acer-wmi.o
>>>
>>> +# Advantech
>>> +obj-$(CONFIG_ADV_SWBUTTON)  += adv_swbutton.o
>>> +
>>
>> The indent before the += should use tabs not spaces.
>>
>>>  # Apple
>>>  obj-$(CONFIG_APPLE_GMUX)   += apple-gmux.o
>>>
>>> diff --git a/drivers/platform/x86/adv_swbutton.c
>>> b/drivers/platform/x86/adv_swbutton.c
>>> new file mode 100644
>>> index ..f4387220e8a0
>>> --- /dev/null
>>> +++ b/drivers/platform/x86/adv_swbutton.c
>>> @@ -0,0 +1,192 @@
>>> +// SPDX-License-Identifier: GPL-2.0
>>> +/*
>>> + *  adv_swbutton.c - Software Button Interface Driver.
>>> + *
>>> + *  (C)

Re: [V2,1/1] platform/x86: add support for Advantech software defined button

2021-03-18 Thread YingChieh, Ho

Hi Hans,

Thank you for your kindly review.
As a beginner in driver coding, I got gains much from your suggestions.
Path v3 is ready at patchwork.^^

Hans de Goede  於 2021年3月19日 週五 上午12:21寫道：
>
> Hi,
>
> On 3/12/21 9:11 AM, YingChieh Ho wrote:
> > From: "Andrea.Ho" 
> >
> > Advantech sw_button is a ACPI event trigger button.
> >
> > With this driver, we can report KEY_EVENT on the
> > Advantech Tabletop Network Appliances products and it has been
> > tested in FWA1112VC.
> >
> > Add the software define button support to report EV_REP key_event
> > (KEY_PROG1) by pressing button that cloud be get on user
> > interface and trigger the customized actions.
> >
> > Signed-off-by: Andrea.Ho 
> >
> > v2:
> > - change evdev key-code from BTN_TRIGGER_HAPPY to KEY_PROG1
> > - to rewrite the driver to be a regular platform_driver
> > - use specific names instead of generic names,
> >   "Software Button" to "Advantech Software Button",
> >   "button" to "adv_swbutton"
> > - remove unused defines or use-once defines, ACPI_BUTTON_FILE_INFO,
> >   ACPI_BUTTON_FILE_STATE, ACPI_BUTTON_TYPE_UNKNOWN, ACPI_SWBTN_NAME
>
> Thank you this version is much better, I have various review remarks below.
>
> Please send a v3 with those fixed.
>
>
> > ---
> >  MAINTAINERS |   5 +
> >  drivers/platform/x86/Kconfig|  11 ++
> >  drivers/platform/x86/Makefile   |   3 +
> >  drivers/platform/x86/adv_swbutton.c | 192 
> >  4 files changed, 211 insertions(+)
> >  create mode 100644 drivers/platform/x86/adv_swbutton.c
> >
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index 68f21d46614c..e35c48e411b7 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -562,6 +562,11 @@ S: Maintained
> >  F: Documentation/scsi/advansys.rst
> >  F: drivers/scsi/advansys.c
> >
> > +ADVANTECH SWBTN DRIVER
> > +M: Andrea Ho 
> > +S: Maintained
> > +F: drivers/platform/x86/adv_swbutton.c
> > +
> >  ADXL34X THREE-AXIS DIGITAL ACCELEROMETER DRIVER (ADXL345/ADXL346)
> >  M: Michael Hennerich 
> >  S: Supported
> > diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
> > index 0581a54cf562..b1340135c5e9 100644
> > --- a/drivers/platform/x86/Kconfig
> > +++ b/drivers/platform/x86/Kconfig
> > @@ -180,6 +180,17 @@ config ACER_WMI
> >   If you have an ACPI-WMI compatible Acer/ Wistron laptop, say Y or 
> > M
> >   here.
>
> This is supposed to be indented by a space (from the diff format) and then
> a tab + 2 spaces, but in your patch this is now indented by 10 spaces.
>
> In general your entire patch has all tabs replaced by spaces, I guess that
> your mail-client or editor is mangling things. Please do the following:
>
> 1. Check your patch for checkpatch errors:
>
> git format-patch HEAD~
> scripts/checkpatch.pl 0001-platform...patch
>
> And fix all reported issues, both whitespace issues and others.
>
> 2. Send the next version of the patch like this:
>
> git format-patch -v3 HEAD~
> git send-email v3-0001-platform...patch
>
> Do NOT edit the v3-0001-platform...patch file between these 2 steps.
>
>
> > +config ADV_SWBUTTON
> > +tristate "Advantech ACPI Software Button Driver"
>
> You are using indent steps of 4 spaces here, that should be a tab
> > +depends on ACPI
> > +help
> > +  Say Y here to enable support for Advantech software defined
> > +  button feature. More information can be fount at
> > +  
>
> And a tab and 2 spaces for the help text.
>
> > +
> > +  To compile this driver as a module, choose M here. The module will
> > +  be called adv_swbutton.
> > +
> >  config APPLE_GMUX
> > tristate "Apple Gmux Driver"
> > depends on ACPI && PCI
> > diff --git a/drivers/platform/x86/Makefile b/drivers/platform/x86/Makefile
> > index 2b85852a1a87..76a321fc58ba 100644
> > --- a/drivers/platform/x86/Makefile
> > +++ b/drivers/platform/x86/Makefile
> > @@ -22,6 +22,9 @@ obj-$(CONFIG_ACERHDF) += acerhdf.o
> >  obj-$(CONFIG_ACER_WIRELESS)+= acer-wireless.o
> >  obj-$(CONFIG_ACER_WMI) += acer-wmi.o
> >
> > +# Advantech
> > +obj-$(CONFIG_ADV_SWBUTTON)  += adv_swbutton.o
> > +
>
> The indent before the += should use tabs not spaces.
>
> >  # Apple
> >  obj-$(CONFIG_APPLE_GMUX)   += apple-gmux.o
> >
> > diff --git a/drivers/platform/x86/adv_swbutton.c
> > b/drivers/platform/x86/adv_swbutton.c
> > new file mode 100644
> > index ..f4387220e8a0
> > --- /dev/null
> > +++ b/drivers/platform/x86/adv_swbutton.c
> > @@ -0,0 +1,192 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + *  adv_swbutton.c - Software Button Interface Driver.
> > + *
> > + *  (C) Copyright 2020 Advantech Corporation, Inc
> > + *
> > + */
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> >

Re: [V2,1/1] platform/x86: add support for Advantech software defined button

2021-03-18 Thread Hans de Goede

Hi,

On 3/12/21 9:11 AM, YingChieh Ho wrote:
> From: "Andrea.Ho" 
> 
> Advantech sw_button is a ACPI event trigger button.
> 
> With this driver, we can report KEY_EVENT on the
> Advantech Tabletop Network Appliances products and it has been
> tested in FWA1112VC.
> 
> Add the software define button support to report EV_REP key_event
> (KEY_PROG1) by pressing button that cloud be get on user
> interface and trigger the customized actions.
> 
> Signed-off-by: Andrea.Ho 
> 
> v2:
> - change evdev key-code from BTN_TRIGGER_HAPPY to KEY_PROG1
> - to rewrite the driver to be a regular platform_driver
> - use specific names instead of generic names,
>   "Software Button" to "Advantech Software Button",
>   "button" to "adv_swbutton"
> - remove unused defines or use-once defines, ACPI_BUTTON_FILE_INFO,
>   ACPI_BUTTON_FILE_STATE, ACPI_BUTTON_TYPE_UNKNOWN, ACPI_SWBTN_NAME

Thank you this version is much better, I have various review remarks below.

Please send a v3 with those fixed.


> ---
>  MAINTAINERS |   5 +
>  drivers/platform/x86/Kconfig|  11 ++
>  drivers/platform/x86/Makefile   |   3 +
>  drivers/platform/x86/adv_swbutton.c | 192 
>  4 files changed, 211 insertions(+)
>  create mode 100644 drivers/platform/x86/adv_swbutton.c
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 68f21d46614c..e35c48e411b7 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -562,6 +562,11 @@ S: Maintained
>  F: Documentation/scsi/advansys.rst
>  F: drivers/scsi/advansys.c
> 
> +ADVANTECH SWBTN DRIVER
> +M: Andrea Ho 
> +S: Maintained
> +F: drivers/platform/x86/adv_swbutton.c
> +
>  ADXL34X THREE-AXIS DIGITAL ACCELEROMETER DRIVER (ADXL345/ADXL346)
>  M: Michael Hennerich 
>  S: Supported
> diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
> index 0581a54cf562..b1340135c5e9 100644
> --- a/drivers/platform/x86/Kconfig
> +++ b/drivers/platform/x86/Kconfig
> @@ -180,6 +180,17 @@ config ACER_WMI
>   If you have an ACPI-WMI compatible Acer/ Wistron laptop, say Y or M
>   here.

This is supposed to be indented by a space (from the diff format) and then
a tab + 2 spaces, but in your patch this is now indented by 10 spaces.

In general your entire patch has all tabs replaced by spaces, I guess that
your mail-client or editor is mangling things. Please do the following:

1. Check your patch for checkpatch errors:

git format-patch HEAD~
scripts/checkpatch.pl 0001-platform...patch

And fix all reported issues, both whitespace issues and others.

2. Send the next version of the patch like this:

git format-patch -v3 HEAD~
git send-email v3-0001-platform...patch

Do NOT edit the v3-0001-platform...patch file between these 2 steps.


> +config ADV_SWBUTTON
> +tristate "Advantech ACPI Software Button Driver"

You are using indent steps of 4 spaces here, that should be a tab
> +depends on ACPI
> +help
> +  Say Y here to enable support for Advantech software defined
> +  button feature. More information can be fount at
> +  

And a tab and 2 spaces for the help text.

> +
> +  To compile this driver as a module, choose M here. The module will
> +  be called adv_swbutton.
> +
>  config APPLE_GMUX
> tristate "Apple Gmux Driver"
> depends on ACPI && PCI
> diff --git a/drivers/platform/x86/Makefile b/drivers/platform/x86/Makefile
> index 2b85852a1a87..76a321fc58ba 100644
> --- a/drivers/platform/x86/Makefile
> +++ b/drivers/platform/x86/Makefile
> @@ -22,6 +22,9 @@ obj-$(CONFIG_ACERHDF) += acerhdf.o
>  obj-$(CONFIG_ACER_WIRELESS)+= acer-wireless.o
>  obj-$(CONFIG_ACER_WMI) += acer-wmi.o
> 
> +# Advantech
> +obj-$(CONFIG_ADV_SWBUTTON)  += adv_swbutton.o
> +

The indent before the += should use tabs not spaces.

>  # Apple
>  obj-$(CONFIG_APPLE_GMUX)   += apple-gmux.o
> 
> diff --git a/drivers/platform/x86/adv_swbutton.c
> b/drivers/platform/x86/adv_swbutton.c
> new file mode 100644
> index ..f4387220e8a0
> --- /dev/null
> +++ b/drivers/platform/x86/adv_swbutton.c
> @@ -0,0 +1,192 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + *  adv_swbutton.c - Software Button Interface Driver.
> + *
> + *  (C) Copyright 2020 Advantech Corporation, Inc
> + *
> + */
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 

Please drop the following unused defines:

#include 
#include 
#include 
#include 
#include 
#include 
#include 

And sort the remaining includes alphabetically.

> +
> +#define MODULE_NAME "adv_swbutton"

Please drop this define, instead just set the .driver.name part of the 
platform_driver struct directly to "adv_swbutton".

> +
> +#define ACPI_BUTTON_HID_SWBTN   "AHC0310"
> +#define

[V2,1/1] platform/x86: add support for Advantech software defined button

2021-03-12 Thread YingChieh Ho

From: "Andrea.Ho" 

Advantech sw_button is a ACPI event trigger button.

With this driver, we can report KEY_EVENT on the
Advantech Tabletop Network Appliances products and it has been
tested in FWA1112VC.

Add the software define button support to report EV_REP key_event
(KEY_PROG1) by pressing button that cloud be get on user
interface and trigger the customized actions.

Signed-off-by: Andrea.Ho 

v2:
- change evdev key-code from BTN_TRIGGER_HAPPY to KEY_PROG1
- to rewrite the driver to be a regular platform_driver
- use specific names instead of generic names,
  "Software Button" to "Advantech Software Button",
  "button" to "adv_swbutton"
- remove unused defines or use-once defines, ACPI_BUTTON_FILE_INFO,
  ACPI_BUTTON_FILE_STATE, ACPI_BUTTON_TYPE_UNKNOWN, ACPI_SWBTN_NAME
---
 MAINTAINERS |   5 +
 drivers/platform/x86/Kconfig|  11 ++
 drivers/platform/x86/Makefile   |   3 +
 drivers/platform/x86/adv_swbutton.c | 192 
 4 files changed, 211 insertions(+)
 create mode 100644 drivers/platform/x86/adv_swbutton.c

diff --git a/MAINTAINERS b/MAINTAINERS
index 68f21d46614c..e35c48e411b7 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -562,6 +562,11 @@ S: Maintained
 F: Documentation/scsi/advansys.rst
 F: drivers/scsi/advansys.c

+ADVANTECH SWBTN DRIVER
+M: Andrea Ho 
+S: Maintained
+F: drivers/platform/x86/adv_swbutton.c
+
 ADXL34X THREE-AXIS DIGITAL ACCELEROMETER DRIVER (ADXL345/ADXL346)
 M: Michael Hennerich 
 S: Supported
diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
index 0581a54cf562..b1340135c5e9 100644
--- a/drivers/platform/x86/Kconfig
+++ b/drivers/platform/x86/Kconfig
@@ -180,6 +180,17 @@ config ACER_WMI
  If you have an ACPI-WMI compatible Acer/ Wistron laptop, say Y or M
  here.

+config ADV_SWBUTTON
+tristate "Advantech ACPI Software Button Driver"
+depends on ACPI
+help
+  Say Y here to enable support for Advantech software defined
+  button feature. More information can be fount at
+  
+
+  To compile this driver as a module, choose M here. The module will
+  be called adv_swbutton.
+
 config APPLE_GMUX
tristate "Apple Gmux Driver"
depends on ACPI && PCI
diff --git a/drivers/platform/x86/Makefile b/drivers/platform/x86/Makefile
index 2b85852a1a87..76a321fc58ba 100644
--- a/drivers/platform/x86/Makefile
+++ b/drivers/platform/x86/Makefile
@@ -22,6 +22,9 @@ obj-$(CONFIG_ACERHDF) += acerhdf.o
 obj-$(CONFIG_ACER_WIRELESS)+= acer-wireless.o
 obj-$(CONFIG_ACER_WMI) += acer-wmi.o

+# Advantech
+obj-$(CONFIG_ADV_SWBUTTON)  += adv_swbutton.o
+
 # Apple
 obj-$(CONFIG_APPLE_GMUX)   += apple-gmux.o

diff --git a/drivers/platform/x86/adv_swbutton.c
b/drivers/platform/x86/adv_swbutton.c
new file mode 100644
index ..f4387220e8a0
--- /dev/null
+++ b/drivers/platform/x86/adv_swbutton.c
@@ -0,0 +1,192 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ *  adv_swbutton.c - Software Button Interface Driver.
+ *
+ *  (C) Copyright 2020 Advantech Corporation, Inc
+ *
+ */
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+#define MODULE_NAME "adv_swbutton"
+
+#define ACPI_BUTTON_HID_SWBTN   "AHC0310"
+#define ACPI_BUTTON_TYPE_SOFTWARE   0x07
+
+#define ACPI_BUTTON_NOTIFY_SWBTN_RELEASE0x86
+#define ACPI_BUTTON_NOTIFY_SWBTN_PRESSED0x85
+
+#define SOFTWARE_BUTTON KEY_PROG1
+
+#define _COMPONENT  ACPI_BUTTON_COMPONENT
+
+struct acpi_button {
+   unsigned int type;
+   struct input_dev *input;
+   char phys[32];
+   bool pressed;
+};
+
+/*-
+ *   Driver Interface
+ *--
+ */
+static void acpi_button_notify(acpi_handle handle, u32 event, void *context)
+{
+   struct acpi_device *device = context;
+
+   struct acpi_button *button = dev_get_drvdata(>dev);
+   struct input_dev *input;
+
+   int keycode, BTN_KEYCODE = SOFTWARE_BUTTON;
+
+   switch (event) {
+   case ACPI_BUTTON_NOTIFY_SWBTN_RELEASE:
+   input = button->input;
+
+   if (!button->pressed)
+   return;
+
+   keycode = test_bit(BTN_KEYCODE, input->keybit) ?
+   BTN_KEYCODE : KEY_UNKNOWN;
+
+   button->pressed = false;
+
+   input_report_key(input, keycode, 0);
+   input_sync(input);
+   break;
+   case ACPI_BUTTON_NOTIFY_SWBTN_PRESSED:
+   input = button->input;
+   button->pressed = true;
+
+   keycode =

Re: [V2,1/1] platform/x86: add support for Advantech software defined button

Re: [V2,1/1] platform/x86: add support for Advantech software defined button

Re: [V2,1/1] platform/x86: add support for Advantech software defined button

[V2,1/1] platform/x86: add support for Advantech software defined button

4 matches

Site Navigation

Mail list logo

Footer information