| 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
| ||
|
Message-ID: <xveh2z7at7ion3fljdcvbhuvfij7m54bb66rlhclmxot3l5ea5@nzgzed6jwhlt>
Date: Wed, 23 Oct 2024 10:45:03 +0200
From: Krzysztof Kozlowski <krzk@...nel.org>
To: mjchen <mjchen0829@...il.com>
Cc: linux-kernel@...r.kernel.org, devicetree@...r.kernel.org,
linux-input@...r.kernel.org, linux-arm-kernel@...ts.infradead.org, mjchen@...oton.com,
peng.fan@....com, sudeep.holla@....com, arnd@...db.de, conor+dt@...nel.org,
krzk+dt@...nel.org, robh@...nel.org, dmitry.torokhov@...il.com
Subject: Re: [PATCH 2/2] input: keypad: add new keypad driver for MA35D1
On Tue, Oct 22, 2024 at 06:31:58AM +0000, mjchen wrote:
> From: mjchen <mjchen@...oton.com>
>
> Adds a new keypad driver for the MA35D1 platform.
> The driver supports key scanning and interrupt handling.
>
> Signed-off-by: mjchen <mjchen@...oton.com>
> ---
> drivers/input/keyboard/Kconfig | 10 +
> drivers/input/keyboard/Makefile | 1 +
> drivers/input/keyboard/ma35d1_keypad.c | 312 +++++++++++++++++++++++++
> 3 files changed, 323 insertions(+)
> create mode 100644 drivers/input/keyboard/ma35d1_keypad.c
>
> diff --git a/drivers/input/keyboard/Kconfig b/drivers/input/keyboard/Kconfig
> index 721ab69e84ac..ce9bd5cc13a1 100644
> --- a/drivers/input/keyboard/Kconfig
> +++ b/drivers/input/keyboard/Kconfig
> @@ -797,4 +797,14 @@ config KEYBOARD_CYPRESS_SF
> To compile this driver as a module, choose M here: the
> module will be called cypress-sf.
>
> +config KEYBOARD_MA35D1
> + tristate "Nuvoton MA35D1 keypad driver"
> + depends on ARCH_MA35
|| COMPILE_TEST
> + select INPUT_MATRIXKMAP
> + help
> + Say Y here if you want to use Nuvoton MA35D1 keypad.
> +
> + To compile this driver as a module, choose M here: the
> + module will be called ma35d1-keypad.
> +
> endif
> diff --git a/drivers/input/keyboard/Makefile b/drivers/input/keyboard/Makefile
> index 1e0721c30709..9b858cdd1b6b 100644
> --- a/drivers/input/keyboard/Makefile
> +++ b/drivers/input/keyboard/Makefile
> @@ -70,3 +70,4 @@ obj-$(CONFIG_KEYBOARD_TEGRA) += tegra-kbc.o
> obj-$(CONFIG_KEYBOARD_TM2_TOUCHKEY) += tm2-touchkey.o
> obj-$(CONFIG_KEYBOARD_TWL4030) += twl4030_keypad.o
> obj-$(CONFIG_KEYBOARD_XTKBD) += xtkbd.o
> +obj-$(CONFIG_KEYBOARD_MA35D1) += ma35d1_keypad.o
> diff --git a/drivers/input/keyboard/ma35d1_keypad.c b/drivers/input/keyboard/ma35d1_keypad.c
> new file mode 100644
> index 000000000000..20b5b1b91127
> --- /dev/null
> +++ b/drivers/input/keyboard/ma35d1_keypad.c
> @@ -0,0 +1,312 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * MA35D1 keypad driver
> + * Copyright (C) 2024 Nuvoton Technology Corp.
> + */
> +
> +#include <linux/interrupt.h>
> +#include <linux/input.h>
> +#include <linux/platform_device.h>
> +#include <linux/input/matrix_keypad.h>
> +#include <linux/clk.h>
> +#include <linux/of.h>
> +
> +/* Keypad Interface Registers */
> +#define KPI_CONF 0x00
> +#define KPI_3KCONF 0x04
> +#define KPI_STATUS 0x08
> +#define KPI_RSTC 0x0C
> +#define KPI_KEST 0x10
> +#define KPI_KPE0 0x18
> +#define KPI_KPE1 0x1C
> +#define KPI_KRE0 0x20
> +#define KPI_KRE1 0x24
> +#define KPI_PRESCALDIV 0x28
> +
> +/* KPI_CONF - Keypad Configuration Register */
> +#define KROW GENMASK(30, 28) /* Keypad Matrix ROW number */
> +#define KCOL GENMASK(26, 24) /* Keypad Matrix COL Number */
> +#define DB_CLKSEL GENMASK(19, 16) /* De-bounce sampling cycle selection */
> +#define PRESCALE GENMASK(15, 8) /* Row Scan Cycle Pre-scale Value */
> +#define WAKEUP BIT(5) /* Lower Power Wakeup Enable */
> +#define INTEN BIT(3) /* Key Interrupt Enable Control */
> +#define RKINTEN BIT(2) /* Release Key Interrupt Enable */
> +#define PKINTEN BIT(1) /* Press Key Interrupt Enable Control */
> +#define ENKP BIT(0) /* Keypad Scan Enable */
> +
> +/* KPI_STATUS - Keypad Status Register */
> +#define PKEY_INT BIT(4) /* Press key interrupt */
> +#define RKEY_INT BIT(3) /* Release key interrupt */
> +#define KEY_INT BIT(2) /* Key Interrupt */
> +#define RST_3KEY BIT(1) /* 3-Keys Reset Flag */
> +#define PDWAKE BIT(0) /* Power Down Wakeup Flag */
> +
> +#define DEFAULT_DEBOUNCE 1
> +#define DEFAULT_PRE_SCALE 1
> +#define DEFAULT_PRE_SCALEDIV 32
> +
> +struct ma35d1_keypad {
> + struct clk *clk;
> + struct input_dev *input_dev;
> + void __iomem *mmio_base;
> + int irq;
> + unsigned int kpi_row;
> + unsigned int kpi_col;
> + unsigned int debounce_val;
> + unsigned int pre_scale;
> + unsigned int pre_scale_divider;
> +};
> +
> +static void ma35d1_keypad_scan_matrix(struct ma35d1_keypad *keypad, unsigned int status)
> +{
> + struct input_dev *input_dev = keypad->input_dev;
> + unsigned int i, j;
> + unsigned int row_add = 0;
> + unsigned int code;
> + unsigned int key;
> + unsigned int press_key;
> + unsigned long KeyEvent[4];
No Windows or C++ code, please.
> + unsigned int row_shift = get_count_order(keypad->kpi_col);
> + unsigned short *keymap = input_dev->keycode;
> +
...
> +static int ma35d1_keypad_probe(struct platform_device *pdev)
> +{
> + struct ma35d1_keypad *keypad;
> + struct input_dev *input_dev;
> + struct resource *res;
> + int error = 0;
> +
> + keypad = devm_kzalloc(&pdev->dev, sizeof(*keypad), GFP_KERNEL);
> + if (!keypad)
> + return -ENOMEM;
> +
> +
> + input_dev = input_allocate_device();
> + if (!input_dev) {
> + dev_err(&pdev->dev, "failed to allocate input device\n");
> + return -ENOMEM;
> + }
> +
> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> + if (!res) {
> + dev_err(&pdev->dev, "failed to get I/O memory\n");
> + error = -ENXIO;
> + goto failed_free_input;
> + }
> +
> + keypad->mmio_base = devm_ioremap_resource(&pdev->dev, res);
> + if (IS_ERR(keypad->mmio_base)) {
> + dev_err(&pdev->dev, "failed to remap I/O memory\n");
> + return PTR_ERR(keypad->mmio_base);
> + }
> +
> + keypad->irq = platform_get_irq(pdev, 0);
> + if (keypad->irq < 0) {
> + dev_err(&pdev->dev, "failed to get IRQ\n");
> + return keypad->irq;
> + }
> +
> + keypad->clk = devm_clk_get(&pdev->dev, NULL);
> + if (IS_ERR(keypad->clk)) {
> + dev_err(&pdev->dev, "failed to get core clk: %ld\n", PTR_ERR(keypad->clk));
Syntax is: return dev_err_probe, except that your error handling code is a mess.
Earlier you have gotos, now return. Organize it nicely, so this will
follow some logical concept.
> + return PTR_ERR(keypad->clk);
> + }
> +
> + error = matrix_keypad_parse_properties(&pdev->dev,
> + &(keypad->kpi_row),
> + &(keypad->kpi_col));
How did you aligned it?
> + if (error) {
> + dev_err(&pdev->dev, "failed to parse kp params\n");
> + return error;
> + }
> +
> + error = matrix_keypad_build_keymap(NULL, NULL,
> + keypad->kpi_row,
> + keypad->kpi_col,
> + NULL, input_dev);
> + if (error) {
> + dev_err(&pdev->dev, "failed to build keymap\n");
> + return error;
> + }
> +
> + keypad->input_dev = input_dev;
> + input_dev->name = pdev->name;
> + input_dev->id.bustype = BUS_HOST;
> + input_dev->open = ma35d1_keypad_open;
> + input_dev->close = ma35d1_keypad_close;
> + input_dev->dev.parent = &pdev->dev;
> +
> + if (of_property_read_u32(pdev->dev.of_node, "debounce-period", &(keypad->debounce_val)))
> + keypad->debounce_val = DEFAULT_DEBOUNCE;
This is not used anywhere. Drop dead code.
> +
> + if (of_property_read_u32(pdev->dev.of_node, "per-scale", &(keypad->pre_scale)))
> + keypad->pre_scale = DEFAULT_PRE_SCALE;
Not better...
> +
> + if (of_property_read_u32(pdev->dev.of_node, "per-scalediv", &(keypad->pre_scale_divider)))
> + keypad->pre_scale_divider = DEFAULT_PRE_SCALEDIV;
Still not better...
So there are defaults? Why these are required by bindings? Why bindings do not say
defaults?
> +
> + __set_bit(EV_REP, input_dev->evbit);
> + input_set_drvdata(input_dev, keypad);
> + input_set_capability(input_dev, EV_MSC, MSC_SCAN);
> +
> + error = input_register_device(input_dev);
> + if (error) {
> + dev_err(&pdev->dev, "failed to register input device\n");
> + goto failed_free_input;
> + }
> +
> + error = devm_request_irq(&pdev->dev, keypad->irq,
> + ma35d1_keypad_interrupt,
> + IRQF_NO_SUSPEND, pdev->name, keypad);
Totally mesed alignment.
> + if (error) {
> + dev_err(&pdev->dev, "failed to request IRQ\n");
> + goto failed_unregister_input;
> + }
> +
> + platform_set_drvdata(pdev, keypad);
> + device_init_wakeup(&pdev->dev, 1);
> + clk_prepare_enable(keypad->clk);
> +
> + return 0;
> +
> +failed_unregister_input:
> + input_unregister_device(input_dev);
> +failed_free_input:
> + input_free_device(input_dev);
> + return error;
> +}
> +
> +static void ma35d1_keypad_remove(struct platform_device *pdev)
> +{
> + struct ma35d1_keypad *keypad = platform_get_drvdata(pdev);
> +
> + input_unregister_device(keypad->input_dev);
> + clk_disable_unprepare(keypad->clk);
Why aren't you using devm_clk_get_enabled()?
> +}
> +
> +static int ma35d1_keypad_suspend(struct platform_device *pdev,
> + pm_message_t state)
> +{
> + struct ma35d1_keypad *keypad = platform_get_drvdata(pdev);
> +
> + if (device_may_wakeup(&pdev->dev)) {
> + writel(readl(keypad->mmio_base + KPI_CONF) | WAKEUP, keypad->mmio_base + KPI_CONF);
> + enable_irq_wake(keypad->irq);
> + }
> +
> + return 0;
> +}
> +
> +static int ma35d1_keypad_resume(struct platform_device *pdev)
> +{
> + struct ma35d1_keypad *keypad = platform_get_drvdata(pdev);
> +
> + if (device_may_wakeup(&pdev->dev)) {
> + writel(readl(keypad->mmio_base + KPI_CONF) & ~(WAKEUP),
> + keypad->mmio_base + KPI_CONF);
> + disable_irq_wake(keypad->irq);
> + }
> +
> + return 0;
> +}
> +
> +static const struct of_device_id ma35d1_kpi_of_match[] = {
> + { .compatible = "nuvoton,ma35d1-kpi"},
> + {},
> +};
> +MODULE_DEVICE_TABLE(of, ma35d1_kpi_of_match);
> +
> +static struct platform_driver ma35d1_keypad_driver = {
> + .probe = ma35d1_keypad_probe,
> + .remove = ma35d1_keypad_remove,
> + .suspend = ma35d1_keypad_suspend,
> + .resume = ma35d1_keypad_resume,
> + .driver = {
> + .name = "ma35d1-kpi",
> + .of_match_table = of_match_ptr(ma35d1_kpi_of_match),
Drop of_match_ptr(), you have warnings here.
Best regards,
Krzysztof
Powered by blists - more mailing lists