| 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: <pnfj4tyj3hovtu5ttnecmgozdq7hm2clxhl4xpuzrahlrzqmdm@qpdr4z2y5ylg>
Date: Fri, 16 May 2025 15:55:02 -0700
From: Dmitry Torokhov <dmitry.torokhov@...il.com>
To: Samuel Kayode <samuel.kayode@...oirfairelinux.com>
Cc: Lee Jones <lee@...nel.org>, Liam Girdwood <lgirdwood@...il.com>,
Mark Brown <broonie@...nel.org>, Sebastian Reichel <sre@...nel.org>,
Robin Gong <yibin.gong@....com>, linux-kernel@...r.kernel.org, linux-pm@...r.kernel.org,
linux-imx@....com, linux-input@...r.kernel.org, Abel Vesa <abelvesa@...ux.com>,
Abel Vesa <abel.vesa@....com>, Robin Gong <b38343@...escale.com>,
Enric Balletbo Serra <eballetbo@...il.com>
Subject: Re: [PATCH v2 7/9] input: pf1550: add onkey support
Hi Samuel,
On Fri, May 16, 2025 at 02:57:28PM -0400, Samuel Kayode wrote:
> Add support for the onkey of the pf1550 PMIC.
>
> Signed-off-by: Samuel Kayode <samuel.kayode@...oirfairelinux.com>
> ---
> drivers/input/keyboard/Kconfig | 8 ++
> drivers/input/keyboard/Makefile | 1 +
> drivers/input/keyboard/pf1550_onkey.c | 200 ++++++++++++++++++++++++++
> 3 files changed, 209 insertions(+)
> create mode 100644 drivers/input/keyboard/pf1550_onkey.c
>
> diff --git a/drivers/input/keyboard/Kconfig b/drivers/input/keyboard/Kconfig
> index 721ab69e84ac..b01decc03ab9 100644
> --- a/drivers/input/keyboard/Kconfig
> +++ b/drivers/input/keyboard/Kconfig
> @@ -444,6 +444,14 @@ config KEYBOARD_SNVS_PWRKEY
> To compile this driver as a module, choose M here; the
> module will be called snvs_pwrkey.
With the exception of snvs-pwrkey, all other onkey/pwrkey drivers live
in drivers/input/misc, please move them there. drivers/input/keyboard is
really for real keyboard devices.
>
> +config KEYBOARD_PF1550_ONKEY
> + tristate "PF1550 OnKey Driver"
> + depends on MFD_PF1550
> + depends on OF
I do not think your driver has any direct dependencies on OF, especially
if you switch to using generic device properties (device_property_read_*).
> + help
> + This is onkey driver for PF1550 pmic, onkey can trigger release
> + and 1s(push hold), 2s, 3s, 4s, 8s interrupt for long press detect
> +
> config KEYBOARD_IMX
> tristate "IMX keypad support"
> depends on ARCH_MXC || COMPILE_TEST
> diff --git a/drivers/input/keyboard/Makefile b/drivers/input/keyboard/Makefile
> index 1e0721c30709..8a6feae3ddb1 100644
> --- a/drivers/input/keyboard/Makefile
> +++ b/drivers/input/keyboard/Makefile
> @@ -59,6 +59,7 @@ obj-$(CONFIG_KEYBOARD_QT2160) += qt2160.o
> obj-$(CONFIG_KEYBOARD_SAMSUNG) += samsung-keypad.o
> obj-$(CONFIG_KEYBOARD_SH_KEYSC) += sh_keysc.o
> obj-$(CONFIG_KEYBOARD_SNVS_PWRKEY) += snvs_pwrkey.o
> +obj-$(CONFIG_KEYBOARD_PF1550_ONKEY) += pf1550_onkey.o
> obj-$(CONFIG_KEYBOARD_SPEAR) += spear-keyboard.o
> obj-$(CONFIG_KEYBOARD_STMPE) += stmpe-keypad.o
> obj-$(CONFIG_KEYBOARD_STOWAWAY) += stowaway.o
> diff --git a/drivers/input/keyboard/pf1550_onkey.c b/drivers/input/keyboard/pf1550_onkey.c
> new file mode 100644
> index 000000000000..b0601acf893d
> --- /dev/null
> +++ b/drivers/input/keyboard/pf1550_onkey.c
> @@ -0,0 +1,200 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Driver for the PF1550 ON_KEY
> + * Copyright (C) 2016 Freescale Semiconductor, Inc. All Rights Reserved.
> + */
> +
> +#include <linux/device.h>
> +#include <linux/err.h>
> +#include <linux/init.h>
> +#include <linux/input.h>
> +#include <linux/interrupt.h>
> +#include <linux/io.h>
I don't think you actually need io.h
> +#include <linux/jiffies.h>
and neither jiffies.h. Please audit your includes.
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/mfd/pf1550.h>
> +#include <linux/of.h>
> +#include <linux/of_address.h>
> +#include <linux/platform_device.h>
> +#include <linux/regmap.h>
> +
> +struct onkey_drv_data {
> + struct device *dev;
> + struct pf1550_dev *pf1550;
> + int irq;
> + int keycode;
> + int wakeup;
> + struct input_dev *input;
> +};
> +
> +static struct pf1550_irq_info pf1550_onkey_irqs[] = {
> + { PF1550_ONKEY_IRQ_PUSHI, "release" },
> + { PF1550_ONKEY_IRQ_1SI, "1S" },
> + { PF1550_ONKEY_IRQ_2SI, "2S" },
> + { PF1550_ONKEY_IRQ_3SI, "3S" },
> + { PF1550_ONKEY_IRQ_4SI, "4S" },
> + { PF1550_ONKEY_IRQ_8SI, "8S" },
> +};
> +
> +static irqreturn_t pf1550_onkey_irq_handler(int irq, void *data)
> +{
> + struct onkey_drv_data *onkey = data;
> + int i, state, irq_type = -1;
> +
> + onkey->irq = irq;
> +
> + for (i = 0; i < ARRAY_SIZE(pf1550_onkey_irqs); i++)
> + if (onkey->irq == pf1550_onkey_irqs[i].virq)
> + irq_type = pf1550_onkey_irqs[i].irq;
> + switch (irq_type) {
> + case PF1550_ONKEY_IRQ_PUSHI:
> + state = 0;
> + break;
> + case PF1550_ONKEY_IRQ_1SI:
> + case PF1550_ONKEY_IRQ_2SI:
> + case PF1550_ONKEY_IRQ_3SI:
> + case PF1550_ONKEY_IRQ_4SI:
> + case PF1550_ONKEY_IRQ_8SI:
> + state = 1;
> + break;
> + default:
> + dev_err(onkey->dev, "onkey interrupt: irq %d occurred\n",
> + irq_type);
> + return IRQ_HANDLED;
> + }
> +
> + input_event(onkey->input, EV_KEY, onkey->keycode, state);
> + input_sync(onkey->input);
> +
> + return IRQ_HANDLED;
> +}
> +
> +static int pf1550_onkey_probe(struct platform_device *pdev)
> +{
> + struct onkey_drv_data *onkey;
> + struct input_dev *input = NULL;
This initialization is not needed.
> + struct device_node *np = pdev->dev.of_node;
Please switch to generic device properties and stop referencing of_node.
> + struct pf1550_dev *pf1550 = dev_get_drvdata(pdev->dev.parent);
> + int i, error;
> +
> + if (!np)
> + return -ENODEV;
> +
> + onkey = devm_kzalloc(&pdev->dev, sizeof(*onkey), GFP_KERNEL);
> + if (!onkey)
> + return -ENOMEM;
> +
> + if (of_property_read_u32(np, "linux,keycodes", &onkey->keycode)) {
> + onkey->keycode = KEY_POWER;
> + dev_warn(&pdev->dev, "KEY_POWER without setting in dts\n");
Maybe make this property mandatory?
> + }
> +
> + onkey->wakeup = of_property_read_bool(np, "wakeup-source");
> +
> + input = devm_input_allocate_device(&pdev->dev);
> + if (!input) {
> + dev_err(&pdev->dev, "failed to allocate the input device\n");
> + return -ENOMEM;
> + }
> +
> + input->name = pdev->name;
> + input->phys = "pf1550-onkey/input0";
> + input->id.bustype = BUS_HOST;
> +
> + input_set_capability(input, EV_KEY, onkey->keycode);
> +
> + for (i = 0; i < ARRAY_SIZE(pf1550_onkey_irqs); i++) {
> + struct pf1550_irq_info *onkey_irq =
> + &pf1550_onkey_irqs[i];
> + unsigned int virq = 0;
> +
> + virq = regmap_irq_get_virq(pf1550->irq_data_onkey,
> + onkey_irq->irq);
> + if (!virq)
> + return -EINVAL;
> +
> + onkey_irq->virq = virq;
I think this kind of mapping needs to be done in the core part of your
driver.
> +
> + error = devm_request_threaded_irq(&pdev->dev, virq, NULL,
> + pf1550_onkey_irq_handler,
> + IRQF_NO_SUSPEND,
> + onkey_irq->name, onkey);
> + if (error) {
> + dev_err(&pdev->dev,
> + "failed: irq request (IRQ: %d, error :%d)\n",
> + onkey_irq->irq, error);
> + return error;
> + }
> + }
> +
> + error = input_register_device(input);
> + if (error < 0) {
Input API return 0 or negative, so simply "if (error) { ... }"
> + dev_err(&pdev->dev, "failed to register input device\n");
> + input_free_device(input);
You are using devm, so no need to free it manually.
> + return error;
> + }
> +
> + onkey->input = input;
You are too late. Interrupts are already active and you may dereference
onkey->input from your ISR.
> + onkey->pf1550 = pf1550;
> + platform_set_drvdata(pdev, onkey);
> +
> + device_init_wakeup(&pdev->dev, onkey->wakeup);
> +
> + return 0;
> +}
> +
> +static int pf1550_onkey_suspend(struct device *dev)
> +{
> + struct platform_device *pdev = to_platform_device(dev);
> + struct onkey_drv_data *onkey = platform_get_drvdata(pdev);
> +
> + if (!device_may_wakeup(&pdev->dev))
> + regmap_write(onkey->pf1550->regmap,
> + PF1550_PMIC_REG_ONKEY_INT_MASK0,
> + ONKEY_IRQ_PUSHI | ONKEY_IRQ_1SI | ONKEY_IRQ_2SI |
> + ONKEY_IRQ_3SI | ONKEY_IRQ_4SI | ONKEY_IRQ_8SI);
> + else
> + enable_irq_wake(onkey->pf1550->irq);
This is suspicious that you need to twiddle the state of the main
interrupt line. I'd expect you manipulate your own interrupts and this
all cascades up.
> +
> + return 0;
> +}
> +
> +static int pf1550_onkey_resume(struct device *dev)
> +{
> + struct platform_device *pdev = to_platform_device(dev);
> + struct onkey_drv_data *onkey = platform_get_drvdata(pdev);
> +
> + if (!device_may_wakeup(&pdev->dev))
> + regmap_write(onkey->pf1550->regmap,
> + PF1550_PMIC_REG_ONKEY_INT_MASK0,
> + ~(ONKEY_IRQ_PUSHI | ONKEY_IRQ_1SI | ONKEY_IRQ_2SI |
> + ONKEY_IRQ_3SI | ONKEY_IRQ_4SI | ONKEY_IRQ_8SI));
> + else
> + disable_irq_wake(onkey->pf1550->irq);
> +
> + return 0;
> +}
> +
> +static const struct of_device_id pf1550_onkey_ids[] = {
> + { .compatible = "fsl,pf1550-onkey" },
> + { /* sentinel */ }
> +};
> +MODULE_DEVICE_TABLE(of, pf1550_onkey_ids);
> +
> +static SIMPLE_DEV_PM_OPS(pf1550_onkey_pm_ops, pf1550_onkey_suspend,
> + pf1550_onkey_resume);
> +
> +static struct platform_driver pf1550_onkey_driver = {
> + .driver = {
> + .name = "pf1550-onkey",
> + .pm = &pf1550_onkey_pm_ops,
> + .of_match_table = pf1550_onkey_ids,
> + },
> + .probe = pf1550_onkey_probe,
> +};
> +module_platform_driver(pf1550_onkey_driver);
> +
> +MODULE_AUTHOR("Freescale Semiconductor");
> +MODULE_DESCRIPTION("PF1550 onkey Driver");
> +MODULE_LICENSE("GPL v2");
Thanks.
--
Dmitry
Powered by blists - more mailing lists