[PATCH] winmate-fm07-keys: Winmate FM07/FM07P buttons

[PATCH] winmate-fm07-keys: Winmate FM07/FM07P buttons

[Daniel Beer]

Winmate FM07 and FM07P in-vehicle computers have a row of five buttons
below the display. This module adds an input device that delivers key
events when these buttons are pressed.

Signed-off-by: Daniel Beer <daniel.beer@tirotech.co.nz>
---
 drivers/platform/x86/Kconfig             |   8 +
 drivers/platform/x86/Makefile            |   3 +
 drivers/platform/x86/winmate-fm07-keys.c | 184 +++++++++++++++++++++++
 3 files changed, 195 insertions(+)
 create mode 100644 drivers/platform/x86/winmate-fm07-keys.c

diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
index 24deeeb29af2..62a5754d6ac9 100644
--- a/drivers/platform/x86/Kconfig
+++ b/drivers/platform/x86/Kconfig
@@ -1138,6 +1138,14 @@ config SIEMENS_SIMATIC_IPC
 	  To compile this driver as a module, choose M here: the module
 	  will be called simatic-ipc.
 
+config WINMATE_FM07_KEYS
+	tristate "Winmate FM07/FM07P front-panel keys driver"
+	depends on INPUT
+	help
+	  Winmate FM07 and FM07P in-vehicle computers have a row of five
+	  buttons below the display. This module adds an input device
+	  that delivers key events when these buttons are pressed.
+
 endif # X86_PLATFORM_DEVICES
 
 config PMC_ATOM
diff --git a/drivers/platform/x86/Makefile b/drivers/platform/x86/Makefile
index c12a9b044fd8..04e7c995b838 100644
--- a/drivers/platform/x86/Makefile
+++ b/drivers/platform/x86/Makefile
@@ -129,3 +129,6 @@ obj-$(CONFIG_PMC_ATOM)			+= pmc_atom.o
 
 # Siemens Simatic Industrial PCs
 obj-$(CONFIG_SIEMENS_SIMATIC_IPC)	+= simatic-ipc.o
+
+# Winmate
+obj-$(CONFIG_WINMATE_FM07_KEYS)		+= winmate-fm07-keys.o
diff --git a/drivers/platform/x86/winmate-fm07-keys.c b/drivers/platform/x86/winmate-fm07-keys.c
new file mode 100644
index 000000000000..3e75adb15444
--- /dev/null
+++ b/drivers/platform/x86/winmate-fm07-keys.c
@@ -0,0 +1,184 @@
+// SPDX-License-Identifier: GPL-2.0
+//
+// Driver for the Winmate FM07 front-panel keys
+//
+// Author: Daniel Beer <daniel.beer@tirotech.co.nz>
+
+#include <linux/init.h>
+#include <linux/module.h>
+#include <linux/input.h>
+#include <linux/ioport.h>
+#include <linux/platform_device.h>
+#include <linux/dmi.h>
+#include <linux/io.h>
+
+#define DRV_NAME	"winmate-fm07keys"
+
+#define PORT_CMD	0x6c
+#define PORT_DATA	0x68
+
+#define EC_ADDR_KEYS	0x3b
+#define EC_CMD_READ	0x80
+
+#define BASE_KEY	KEY_F13
+#define NUM_KEYS	5
+
+/* Typically we're done in fewer than 10 iterations */
+#define LOOP_TIMEOUT	1000
+
+static void fm07keys_poll(struct input_dev *input)
+{
+	uint8_t k;
+	int i;
+
+	/* Flush output buffer */
+	i = 0;
+	while (inb(PORT_CMD) & 0x01) {
+		if (++i >= LOOP_TIMEOUT)
+			goto timeout;
+		inb(PORT_DATA);
+	}
+
+	/* Send request and wait for write completion */
+	outb(EC_CMD_READ, PORT_CMD);
+	i = 0;
+	while (inb(PORT_CMD) & 0x02)
+		if (++i >= LOOP_TIMEOUT)
+			goto timeout;
+
+	outb(EC_ADDR_KEYS, PORT_DATA);
+	i = 0;
+	while (inb(PORT_CMD) & 0x02)
+		if (++i >= LOOP_TIMEOUT)
+			goto timeout;
+
+	/* Wait for data ready */
+	i = 0;
+	while (!(inb(PORT_CMD) & 0x01))
+		if (++i >= LOOP_TIMEOUT)
+			goto timeout;
+	k = inb(PORT_DATA);
+
+	/* Notify of new key states */
+	for (i = 0; i < NUM_KEYS; i++) {
+		input_report_key(input, BASE_KEY + i, (~k) & 1);
+		k >>= 1;
+	}
+
+	input_sync(input);
+	return;
+timeout:
+	dev_err(&input->dev, "timeout polling IO memory\n");
+}
+
+static int fm07keys_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct input_dev *input;
+	int ret;
+	int i;
+
+	input = devm_input_allocate_device(dev);
+	if (!input) {
+		dev_err(dev, "no memory for input device\n");
+		return -ENOMEM;
+	}
+
+	if (!devm_request_region(dev, PORT_CMD, 1, "Winmate FM07 EC"))
+		return -EBUSY;
+	if (!devm_request_region(dev, PORT_DATA, 1, "Winmate FM07 EC"))
+		return -EBUSY;
+
+	input->name = "Winmate FM07 front-panel keys";
+	input->phys = DRV_NAME "/input0";
+
+	input->id.bustype = BUS_HOST;
+	input->id.vendor = 0x0001;
+	input->id.product = 0x0001;
+	input->id.version = 0x0100;
+
+	__set_bit(EV_KEY, input->evbit);
+
+	for (i = 0; i < NUM_KEYS; i++)
+		__set_bit(BASE_KEY + i, input->keybit);
+
+	ret = input_setup_polling(input, fm07keys_poll);
+	if (ret) {
+		dev_err(dev, "unable to set up polling, err=%d\n", ret);
+		return ret;
+	}
+
+	input_set_poll_interval(input, 20);
+
+	ret = input_register_device(input);
+	if (ret) {
+		dev_err(dev, "unable to register polled device, err=%d\n",
+			ret);
+		return ret;
+	}
+
+	input_sync(input);
+	return 0;
+}
+
+static struct platform_driver fm07keys_driver = {
+	.probe		= fm07keys_probe,
+	.driver		= {
+		.name	= DRV_NAME
+	},
+};
+
+/************************************************************************
+ * Device instance for the FM07
+ */
+
+static struct platform_device *dev;
+
+static const struct dmi_system_id fm07keys_dmi_table[] __initconst = {
+	{
+		/* FM07 and FM07P */
+		.matches = {
+			DMI_MATCH(DMI_SYS_VENDOR, "Winmate Inc."),
+			DMI_MATCH(DMI_PRODUCT_NAME, "IP30"),
+		},
+	},
+};
+
+static int __init fm07keys_init(void)
+{
+	int ret;
+
+	if (!dmi_check_system(fm07keys_dmi_table))
+		return -ENODEV;
+
+	ret = platform_driver_register(&fm07keys_driver);
+	if (ret) {
+		pr_err("fm07keys: failed to register driver, err=%d\n", ret);
+		return ret;
+	}
+
+	dev = platform_device_register_simple(DRV_NAME, -1, NULL, 0);
+	if (!dev) {
+		pr_err("fm07keys: failed to allocate device\n");
+		ret = -ENOMEM;
+		goto fail_alloc;
+	}
+
+	return 0;
+fail_alloc:
+	platform_driver_unregister(&fm07keys_driver);
+	return ret;
+}
+
+static void __exit fm07keys_exit(void)
+{
+	platform_driver_unregister(&fm07keys_driver);
+	platform_device_unregister(dev);
+}
+
+module_init(fm07keys_init);
+module_exit(fm07keys_exit);
+
+MODULE_AUTHOR("Daniel Beer <daniel.beer@tirotech.co.nz>");
+MODULE_DESCRIPTION("Winmate FM07 front-panel keys driver");
+MODULE_LICENSE("GPL v2");
-- 
2.30.2

Re: [PATCH] winmate-fm07-keys: Winmate FM07/FM07P buttons

[Hans de Goede]

Hi Daniel,

On 3/21/22 04:16, Daniel Beer wrote:
> Winmate FM07 and FM07P in-vehicle computers have a row of five buttons
> below the display. This module adds an input device that delivers key
> events when these buttons are pressed.
> 
> Signed-off-by: Daniel Beer <daniel.beer@tirotech.co.nz>

Do you have control of the firmware on this device?

If yes then IMHO it would be better to implement this in ACPI
using the intel-hid fw API for this, see:

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/platform/x86/intel/hid.c

Basically you create an ACPI device with one of the HIDs from
the driver. Then on button presses you do an ACPI notify on
that device with a value of 0xc0 then the driver will call 
the device's _DSM (device specific ACPI) method with a GUID
of eeec56b3-4442-408f-a792-4edd4d758054 and an argument value
of INTEL_HID_DSM_HDEM_FN and then you return the event-code
for the button.

You can remap the event-codes to different KEY_FOO values using
/lib/udev/hwdb.d/60-keyboard.hwdb

Regards,

Hans




> ---
>  drivers/platform/x86/Kconfig             |   8 +
>  drivers/platform/x86/Makefile            |   3 +
>  drivers/platform/x86/winmate-fm07-keys.c | 184 +++++++++++++++++++++++
>  3 files changed, 195 insertions(+)
>  create mode 100644 drivers/platform/x86/winmate-fm07-keys.c
> 
> diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
> index 24deeeb29af2..62a5754d6ac9 100644
> --- a/drivers/platform/x86/Kconfig
> +++ b/drivers/platform/x86/Kconfig
> @@ -1138,6 +1138,14 @@ config SIEMENS_SIMATIC_IPC
>  	  To compile this driver as a module, choose M here: the module
>  	  will be called simatic-ipc.
>  
> +config WINMATE_FM07_KEYS
> +	tristate "Winmate FM07/FM07P front-panel keys driver"
> +	depends on INPUT
> +	help
> +	  Winmate FM07 and FM07P in-vehicle computers have a row of five
> +	  buttons below the display. This module adds an input device
> +	  that delivers key events when these buttons are pressed.
> +
>  endif # X86_PLATFORM_DEVICES
>  
>  config PMC_ATOM
> diff --git a/drivers/platform/x86/Makefile b/drivers/platform/x86/Makefile
> index c12a9b044fd8..04e7c995b838 100644
> --- a/drivers/platform/x86/Makefile
> +++ b/drivers/platform/x86/Makefile
> @@ -129,3 +129,6 @@ obj-$(CONFIG_PMC_ATOM)			+= pmc_atom.o
>  
>  # Siemens Simatic Industrial PCs
>  obj-$(CONFIG_SIEMENS_SIMATIC_IPC)	+= simatic-ipc.o
> +
> +# Winmate
> +obj-$(CONFIG_WINMATE_FM07_KEYS)		+= winmate-fm07-keys.o
> diff --git a/drivers/platform/x86/winmate-fm07-keys.c b/drivers/platform/x86/winmate-fm07-keys.c
> new file mode 100644
> index 000000000000..3e75adb15444
> --- /dev/null
> +++ b/drivers/platform/x86/winmate-fm07-keys.c
> @@ -0,0 +1,184 @@
> +// SPDX-License-Identifier: GPL-2.0
> +//
> +// Driver for the Winmate FM07 front-panel keys
> +//
> +// Author: Daniel Beer <daniel.beer@tirotech.co.nz>
> +
> +#include <linux/init.h>
> +#include <linux/module.h>
> +#include <linux/input.h>
> +#include <linux/ioport.h>
> +#include <linux/platform_device.h>
> +#include <linux/dmi.h>
> +#include <linux/io.h>
> +
> +#define DRV_NAME	"winmate-fm07keys"
> +
> +#define PORT_CMD	0x6c
> +#define PORT_DATA	0x68
> +
> +#define EC_ADDR_KEYS	0x3b
> +#define EC_CMD_READ	0x80
> +
> +#define BASE_KEY	KEY_F13
> +#define NUM_KEYS	5
> +
> +/* Typically we're done in fewer than 10 iterations */
> +#define LOOP_TIMEOUT	1000
> +
> +static void fm07keys_poll(struct input_dev *input)
> +{
> +	uint8_t k;
> +	int i;
> +
> +	/* Flush output buffer */
> +	i = 0;
> +	while (inb(PORT_CMD) & 0x01) {
> +		if (++i >= LOOP_TIMEOUT)
> +			goto timeout;
> +		inb(PORT_DATA);
> +	}
> +
> +	/* Send request and wait for write completion */
> +	outb(EC_CMD_READ, PORT_CMD);
> +	i = 0;
> +	while (inb(PORT_CMD) & 0x02)
> +		if (++i >= LOOP_TIMEOUT)
> +			goto timeout;
> +
> +	outb(EC_ADDR_KEYS, PORT_DATA);
> +	i = 0;
> +	while (inb(PORT_CMD) & 0x02)
> +		if (++i >= LOOP_TIMEOUT)
> +			goto timeout;
> +
> +	/* Wait for data ready */
> +	i = 0;
> +	while (!(inb(PORT_CMD) & 0x01))
> +		if (++i >= LOOP_TIMEOUT)
> +			goto timeout;
> +	k = inb(PORT_DATA);
> +
> +	/* Notify of new key states */
> +	for (i = 0; i < NUM_KEYS; i++) {
> +		input_report_key(input, BASE_KEY + i, (~k) & 1);
> +		k >>= 1;
> +	}
> +
> +	input_sync(input);
> +	return;
> +timeout:
> +	dev_err(&input->dev, "timeout polling IO memory\n");
> +}
> +
> +static int fm07keys_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct input_dev *input;
> +	int ret;
> +	int i;
> +
> +	input = devm_input_allocate_device(dev);
> +	if (!input) {
> +		dev_err(dev, "no memory for input device\n");
> +		return -ENOMEM;
> +	}
> +
> +	if (!devm_request_region(dev, PORT_CMD, 1, "Winmate FM07 EC"))
> +		return -EBUSY;
> +	if (!devm_request_region(dev, PORT_DATA, 1, "Winmate FM07 EC"))
> +		return -EBUSY;
> +
> +	input->name = "Winmate FM07 front-panel keys";
> +	input->phys = DRV_NAME "/input0";
> +
> +	input->id.bustype = BUS_HOST;
> +	input->id.vendor = 0x0001;
> +	input->id.product = 0x0001;
> +	input->id.version = 0x0100;
> +
> +	__set_bit(EV_KEY, input->evbit);
> +
> +	for (i = 0; i < NUM_KEYS; i++)
> +		__set_bit(BASE_KEY + i, input->keybit);
> +
> +	ret = input_setup_polling(input, fm07keys_poll);
> +	if (ret) {
> +		dev_err(dev, "unable to set up polling, err=%d\n", ret);
> +		return ret;
> +	}
> +
> +	input_set_poll_interval(input, 20);
> +
> +	ret = input_register_device(input);
> +	if (ret) {
> +		dev_err(dev, "unable to register polled device, err=%d\n",
> +			ret);
> +		return ret;
> +	}
> +
> +	input_sync(input);
> +	return 0;
> +}
> +
> +static struct platform_driver fm07keys_driver = {
> +	.probe		= fm07keys_probe,
> +	.driver		= {
> +		.name	= DRV_NAME
> +	},
> +};
> +
> +/************************************************************************
> + * Device instance for the FM07
> + */
> +
> +static struct platform_device *dev;
> +
> +static const struct dmi_system_id fm07keys_dmi_table[] __initconst = {
> +	{
> +		/* FM07 and FM07P */
> +		.matches = {
> +			DMI_MATCH(DMI_SYS_VENDOR, "Winmate Inc."),
> +			DMI_MATCH(DMI_PRODUCT_NAME, "IP30"),
> +		},
> +	},
> +};
> +
> +static int __init fm07keys_init(void)
> +{
> +	int ret;
> +
> +	if (!dmi_check_system(fm07keys_dmi_table))
> +		return -ENODEV;
> +
> +	ret = platform_driver_register(&fm07keys_driver);
> +	if (ret) {
> +		pr_err("fm07keys: failed to register driver, err=%d\n", ret);
> +		return ret;
> +	}
> +
> +	dev = platform_device_register_simple(DRV_NAME, -1, NULL, 0);
> +	if (!dev) {
> +		pr_err("fm07keys: failed to allocate device\n");
> +		ret = -ENOMEM;
> +		goto fail_alloc;
> +	}
> +
> +	return 0;
> +fail_alloc:
> +	platform_driver_unregister(&fm07keys_driver);
> +	return ret;
> +}
> +
> +static void __exit fm07keys_exit(void)
> +{
> +	platform_driver_unregister(&fm07keys_driver);
> +	platform_device_unregister(dev);
> +}
> +
> +module_init(fm07keys_init);
> +module_exit(fm07keys_exit);
> +
> +MODULE_AUTHOR("Daniel Beer <daniel.beer@tirotech.co.nz>");
> +MODULE_DESCRIPTION("Winmate FM07 front-panel keys driver");
> +MODULE_LICENSE("GPL v2");

Re: [PATCH] winmate-fm07-keys: Winmate FM07/FM07P buttons

[Daniel Beer]

On Mon, Mar 21, 2022 at 11:22:40AM +0100, Hans de Goede wrote:
> Do you have control of the firmware on this device?
> 
> If yes then IMHO it would be better to implement this in ACPI
> using the intel-hid fw API for this, see:

Hi Hans,

Unfortunately not. I don't work for Winmate and they haven't provided
any documentation on this.

Cheers,
Daniel

-- 
Daniel Beer
Software Development Manager
Tirotech Ltd

Re: [PATCH] winmate-fm07-keys: Winmate FM07/FM07P buttons

[Barnabás Pőcze]

Hi


2022. március 21., hétfő 4:16 keltezéssel, Daniel Beer írta:
> Winmate FM07 and FM07P in-vehicle computers have a row of five buttons
> below the display. This module adds an input device that delivers key
> events when these buttons are pressed.
>
> Signed-off-by: Daniel Beer <daniel.beer@tirotech.co.nz>
> ---
>  drivers/platform/x86/Kconfig             |   8 +
>  drivers/platform/x86/Makefile            |   3 +
>  drivers/platform/x86/winmate-fm07-keys.c | 184 +++++++++++++++++++++++
>  3 files changed, 195 insertions(+)
>  create mode 100644 drivers/platform/x86/winmate-fm07-keys.c
>
> diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
> index 24deeeb29af2..62a5754d6ac9 100644
> --- a/drivers/platform/x86/Kconfig
> +++ b/drivers/platform/x86/Kconfig
> @@ -1138,6 +1138,14 @@ config SIEMENS_SIMATIC_IPC
>  	  To compile this driver as a module, choose M here: the module
>  	  will be called simatic-ipc.
>
> +config WINMATE_FM07_KEYS
> +	tristate "Winmate FM07/FM07P front-panel keys driver"
> +	depends on INPUT
> +	help
> +	  Winmate FM07 and FM07P in-vehicle computers have a row of five
> +	  buttons below the display. This module adds an input device
> +	  that delivers key events when these buttons are pressed.
> +
>  endif # X86_PLATFORM_DEVICES
>
>  config PMC_ATOM
> diff --git a/drivers/platform/x86/Makefile b/drivers/platform/x86/Makefile
> index c12a9b044fd8..04e7c995b838 100644
> --- a/drivers/platform/x86/Makefile
> +++ b/drivers/platform/x86/Makefile
> @@ -129,3 +129,6 @@ obj-$(CONFIG_PMC_ATOM)			+= pmc_atom.o
>
>  # Siemens Simatic Industrial PCs
>  obj-$(CONFIG_SIEMENS_SIMATIC_IPC)	+= simatic-ipc.o
> +
> +# Winmate
> +obj-$(CONFIG_WINMATE_FM07_KEYS)		+= winmate-fm07-keys.o
> diff --git a/drivers/platform/x86/winmate-fm07-keys.c b/drivers/platform/x86/winmate-fm07-keys.c
> new file mode 100644
> index 000000000000..3e75adb15444
> --- /dev/null
> +++ b/drivers/platform/x86/winmate-fm07-keys.c
> @@ -0,0 +1,184 @@
> +// SPDX-License-Identifier: GPL-2.0
> +//
> +// Driver for the Winmate FM07 front-panel keys
> +//
> +// Author: Daniel Beer <daniel.beer@tirotech.co.nz>
> +
> +#include <linux/init.h>
> +#include <linux/module.h>
> +#include <linux/input.h>
> +#include <linux/ioport.h>
> +#include <linux/platform_device.h>
> +#include <linux/dmi.h>
> +#include <linux/io.h>
> +
> +#define DRV_NAME	"winmate-fm07keys"
> +
> +#define PORT_CMD	0x6c
> +#define PORT_DATA	0x68
> +
> +#define EC_ADDR_KEYS	0x3b
> +#define EC_CMD_READ	0x80

It seems you are reading from the embedded controller, have you checked if the
ACPI EC driver works? Then you could probably just use the `ec_{read,write}()` functions.


> +
> +#define BASE_KEY	KEY_F13
> +#define NUM_KEYS	5
> +
> +/* Typically we're done in fewer than 10 iterations */
> +#define LOOP_TIMEOUT	1000
> +
> +static void fm07keys_poll(struct input_dev *input)
> +{
> +	uint8_t k;
> +	int i;
> +
> +	/* Flush output buffer */
> +	i = 0;
> +	while (inb(PORT_CMD) & 0x01) {
> +		if (++i >= LOOP_TIMEOUT)
> +			goto timeout;
> +		inb(PORT_DATA);
> +	}
> +
> +	/* Send request and wait for write completion */
> +	outb(EC_CMD_READ, PORT_CMD);
> +	i = 0;
> +	while (inb(PORT_CMD) & 0x02)
> +		if (++i >= LOOP_TIMEOUT)
> +			goto timeout;
> +
> +	outb(EC_ADDR_KEYS, PORT_DATA);
> +	i = 0;
> +	while (inb(PORT_CMD) & 0x02)
> +		if (++i >= LOOP_TIMEOUT)
> +			goto timeout;
> +
> +	/* Wait for data ready */
> +	i = 0;
> +	while (!(inb(PORT_CMD) & 0x01))
> +		if (++i >= LOOP_TIMEOUT)
> +			goto timeout;
> +	k = inb(PORT_DATA);
> +
> +	/* Notify of new key states */
> +	for (i = 0; i < NUM_KEYS; i++) {
> +		input_report_key(input, BASE_KEY + i, (~k) & 1);
> +		k >>= 1;
> +	}
> +
> +	input_sync(input);
> +	return;
> +timeout:
> +	dev_err(&input->dev, "timeout polling IO memory\n");

I think it may be better to use `dev_warn_{ratelimited,once}()` here.


> +}
> +
> +static int fm07keys_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct input_dev *input;
> +	int ret;
> +	int i;
> +
> +	input = devm_input_allocate_device(dev);
> +	if (!input) {
> +		dev_err(dev, "no memory for input device\n");
> +		return -ENOMEM;
> +	}
> +
> +	if (!devm_request_region(dev, PORT_CMD, 1, "Winmate FM07 EC"))
> +		return -EBUSY;
> +	if (!devm_request_region(dev, PORT_DATA, 1, "Winmate FM07 EC"))
> +		return -EBUSY;
> +
> +	input->name = "Winmate FM07 front-panel keys";
> +	input->phys = DRV_NAME "/input0";
> +
> +	input->id.bustype = BUS_HOST;
> +	input->id.vendor = 0x0001;
> +	input->id.product = 0x0001;
> +	input->id.version = 0x0100;
> +
> +	__set_bit(EV_KEY, input->evbit);
> +
> +	for (i = 0; i < NUM_KEYS; i++)
> +		__set_bit(BASE_KEY + i, input->keybit);
> +
> +	ret = input_setup_polling(input, fm07keys_poll);
> +	if (ret) {
> +		dev_err(dev, "unable to set up polling, err=%d\n", ret);
> +		return ret;
> +	}
> +
> +	input_set_poll_interval(input, 20);

Can you please elaborate how you have arrived at this number?


> +
> +	ret = input_register_device(input);
> +	if (ret) {
> +		dev_err(dev, "unable to register polled device, err=%d\n",
> +			ret);
> +		return ret;
> +	}
> +
> +	input_sync(input);
> +	return 0;
> +}
> +
> +static struct platform_driver fm07keys_driver = {
> +	.probe		= fm07keys_probe,
> +	.driver		= {
> +		.name	= DRV_NAME
> +	},
> +};
> +
> +/************************************************************************
> + * Device instance for the FM07
> + */
> +
> +static struct platform_device *dev;
> +
> +static const struct dmi_system_id fm07keys_dmi_table[] __initconst = {
> +	{
> +		/* FM07 and FM07P */
> +		.matches = {
> +			DMI_MATCH(DMI_SYS_VENDOR, "Winmate Inc."),
> +			DMI_MATCH(DMI_PRODUCT_NAME, "IP30"),
> +		},
> +	},
> +};

You should probably add:

  MODULE_DEVICE_TABLE(dmi, fm07keys_dmi_table);


> +
> +static int __init fm07keys_init(void)
> +{
> +	int ret;
> +
> +	if (!dmi_check_system(fm07keys_dmi_table))
> +		return -ENODEV;
> +
> +	ret = platform_driver_register(&fm07keys_driver);
> +	if (ret) {
> +		pr_err("fm07keys: failed to register driver, err=%d\n", ret);
> +		return ret;
> +	}
> +
> +	dev = platform_device_register_simple(DRV_NAME, -1, NULL, 0);
> +	if (!dev) {

`platform_device_register_simple()` returns an ERR_PTR(), so this should be:

  if (IS_ERR(dev)) {
    ret = PTR_ERR(dev);
    ...
  }


> +		pr_err("fm07keys: failed to allocate device\n");
> +		ret = -ENOMEM;
> +		goto fail_alloc;
> +	}
> +
> +	return 0;
> +fail_alloc:
> +	platform_driver_unregister(&fm07keys_driver);
> +	return ret;
> +}
> +
> +static void __exit fm07keys_exit(void)
> +{
> +	platform_driver_unregister(&fm07keys_driver);
> +	platform_device_unregister(dev);
> +}
> +
> +module_init(fm07keys_init);
> +module_exit(fm07keys_exit);
> +
> +MODULE_AUTHOR("Daniel Beer <daniel.beer@tirotech.co.nz>");
> +MODULE_DESCRIPTION("Winmate FM07 front-panel keys driver");
> +MODULE_LICENSE("GPL v2");
> --
> 2.30.2


Regards,
Barnabás Pőcze

Re: [PATCH] winmate-fm07-keys: Winmate FM07/FM07P buttons

[Daniel Beer]

On Mon, Mar 21, 2022 at 08:13:15PM +0000, Barnabás Pőcze wrote:
> > +#define PORT_CMD	0x6c
> > +#define PORT_DATA	0x68
> > +
> > +#define EC_ADDR_KEYS	0x3b
> > +#define EC_CMD_READ	0x80
> 
> It seems you are reading from the embedded controller, have you checked if the
> ACPI EC driver works? Then you could probably just use the `ec_{read,write}()` functions.

Hi Barnabás,

There is already an ACPI EC driver working on this system, but it's
using ports 0x66 and 0x62. I guess this one is something unrelated,
although it does appear to use the same protocol.

I might be missing something, but it doesn't look like I can use the ec_
functions in this case as they will direct everything towards the EC at
0x66/0x62. Is there anything else you'd suggest?

If it helps to know, the hardware vendor shipped an Ubuntu distribution
with a kernel module that just exposed the ability to read/write ports
0x6c/0x68 via chardev ioctls, and then a proprietary userspace program
polled for button states.

I'll address your other comments in v2.

Cheers,
Daniel

-- 
Daniel Beer
Software Development Manager
Tirotech Ltd

Re: [PATCH] winmate-fm07-keys: Winmate FM07/FM07P buttons

[Hans de Goede]

Hi,

On 3/21/22 20:32, Daniel Beer wrote:
> On Mon, Mar 21, 2022 at 11:22:40AM +0100, Hans de Goede wrote:
>> Do you have control of the firmware on this device?
>>
>> If yes then IMHO it would be better to implement this in ACPI
>> using the intel-hid fw API for this, see:
> 
> Hi Hans,
> 
> Unfortunately not. I don't work for Winmate and they haven't provided
> any documentation on this.

Ok, in that case please send a v2 addressing Barnabás' remarks.

I'll do a full review of v2 and then depending on what I find
merge it, or ask you to do a v3.

Regards,

Hans

Re: [PATCH] winmate-fm07-keys: Winmate FM07/FM07P buttons

[Barnabás Pőcze]

2022. március 21., hétfő 4:16 keltezéssel, Daniel Beer írta
> [...]
> +static const struct dmi_system_id fm07keys_dmi_table[] __initconst = {
> +	{
> +		/* FM07 and FM07P */
> +		.matches = {
> +			DMI_MATCH(DMI_SYS_VENDOR, "Winmate Inc."),
> +			DMI_MATCH(DMI_PRODUCT_NAME, "IP30"),
> +		},
> +	},
> +};

I forgot to mention it, but the sentinel entry is missing from the end.


> [...]


Regards,
Barnabás Pőcze