Add matrix keypad driver, support matrix keypad function.
Signed-off-by: Wenhua Lin <[email protected]>
---
drivers/input/keyboard/Kconfig | 10 +
drivers/input/keyboard/Makefile | 1 +
drivers/input/keyboard/sprd_keypad.c | 418 +++++++++++++++++++++++++++
3 files changed, 429 insertions(+)
create mode 100644 drivers/input/keyboard/sprd_keypad.c
diff --git a/drivers/input/keyboard/Kconfig b/drivers/input/keyboard/Kconfig
index 1d0c5f4c0f99..f35d9bf05f72 100644
--- a/drivers/input/keyboard/Kconfig
+++ b/drivers/input/keyboard/Kconfig
@@ -809,4 +809,14 @@ config KEYBOARD_CYPRESS_SF
To compile this driver as a module, choose M here: the
module will be called cypress-sf.
+config KEYBOARD_SPRD
+ tristate "Spreadtrum keyboard support"
+ depends on ARCH_SPRD || COMPILE_TEST
+ select INPUT_MATRIXKMAP
+ help
+ Keypad controller is used to interface a SoC with a matrix-keypad device,
+ The keypad controller supports multiple row and column lines.
+ Say Y if you want to use the SPRD keyboard.
+ Say M if you want to use the SPRD keyboard on SoC as module.
+
endif
diff --git a/drivers/input/keyboard/Makefile b/drivers/input/keyboard/Makefile
index aecef00c5d09..b747112461b1 100644
--- a/drivers/input/keyboard/Makefile
+++ b/drivers/input/keyboard/Makefile
@@ -66,6 +66,7 @@ obj-$(CONFIG_KEYBOARD_STOWAWAY) += stowaway.o
obj-$(CONFIG_KEYBOARD_ST_KEYSCAN) += st-keyscan.o
obj-$(CONFIG_KEYBOARD_SUN4I_LRADC) += sun4i-lradc-keys.o
obj-$(CONFIG_KEYBOARD_SUNKBD) += sunkbd.o
+obj-$(CONFIG_KEYBOARD_SPRD) += sprd_keypad.o
obj-$(CONFIG_KEYBOARD_TC3589X) += tc3589x-keypad.o
obj-$(CONFIG_KEYBOARD_TEGRA) += tegra-kbc.o
obj-$(CONFIG_KEYBOARD_TM2_TOUCHKEY) += tm2-touchkey.o
diff --git a/drivers/input/keyboard/sprd_keypad.c b/drivers/input/keyboard/sprd_keypad.c
new file mode 100644
index 000000000000..5b88072831e8
--- /dev/null
+++ b/drivers/input/keyboard/sprd_keypad.c
@@ -0,0 +1,418 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (C) 2018 Spreadtrum Communications Inc.
+ */
+
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/input/matrix_keypad.h>
+#include <linux/io.h>
+#include <linux/interrupt.h>
+#include <linux/clk.h>
+#include <linux/of.h>
+#include <linux/input.h>
+
+#define SPRD_KPD_CTRL 0x0
+#define SPRD_KPD_INT_EN 0x4
+#define SPRD_KPD_INT_RAW_STATUS 0x8
+#define SPRD_KPD_INT_MASK_STATUS 0xc
+#define SPRD_KPD_INT_CLR 0x10
+#define SPRD_KPD_POLARITY 0x18
+#define SPRD_KPD_DEBOUNCE_CNT 0x1c
+#define SPRD_KPD_LONG_KEY_CNT 0x20
+#define SPRD_KPD_SLEEP_CNT 0x24
+#define SPRD_KPD_CLK_DIV_CNT 0x28
+#define SPRD_KPD_KEY_STATUS 0x2c
+#define SPRD_KPD_SLEEP_STATUS 0x30
+#define SPRD_KPD_DEBUG_STATUS1 0x34
+#define SPRD_KPD_DEBUG_STATUS2 0x38
+
+#define SPRD_KPD_EN BIT(0)
+#define SPRD_KPD_SLEEP_EN BIT(1)
+#define SPRD_KPD_LONG_KEY_EN BIT(2)
+
+#define SPRD_KPD_ROWS_MSK GENMASK(23, 16)
+#define SPRD_KPD_COLS_MSK GENMASK(15, 8)
+
+#define SPRD_KPD_INT_ALL GENMASK(11, 0)
+#define SPRD_KPD_INT_DOWNUP GENMASK(7, 0)
+#define SPRD_KPD_INT_LONG GENMASK(11, 8)
+
+#define SPRD_KPD_ROW_POLARITY GENMASK(7, 0)
+#define SPRD_KPD_COL_POLARITY GENMASK(15, 8)
+
+#define SPRD_KPD_PRESS_INTX(X, V) \
+ (((V) >> (X)) & GENMASK(0, 0))
+#define SPRD_KPD_RELEASE_INTX(X, V) \
+ (((V) >> ((X) + 4)) & GENMASK(0, 0))
+#define SPRD_KPD_INTX_COL(X, V) \
+ (((V) >> ((X) << 3)) & GENMASK(2, 0))
+#define SPRD_KPD_INTX_ROW(X, V) \
+ (((V) >> (((X) << 3) + 4)) & GENMASK(2, 0))
+#define SPRD_KPD_INTX_DOWN(X, V) \
+ (((V) >> (((X) << 3) + 7)) & GENMASK(0, 0))
+
+#define SPRD_KPD_RTC_HZ 32768
+#define SPRD_DEF_LONG_KEY_MS 1000
+#define SPRD_DEF_DIV_CNT 1
+#define SPRD_KPD_INT_CNT 4
+#define SPRD_KPD_ROWS_MAX 8
+#define SPRD_KPD_COLS_MAX 8
+#define SPRD_KPD_ROWS_SHIFT 16
+#define SPRD_KPD_COLS_SHIFT 8
+
+#define SPRD_CAP_WAKEUP BIT(0)
+#define SPRD_CAP_LONG_KEY BIT(1)
+#define SPRD_CAP_REPEAT BIT(2)
+
+struct sprd_keypad_data {
+ u32 rows_en; /* enabled rows bits */
+ u32 cols_en; /* enabled cols bits */
+ u32 num_rows;
+ u32 num_cols;
+ u32 capabilities;
+ u32 debounce_ms;
+ void __iomem *base;
+ struct input_dev *input_dev;
+ struct clk *enable;
+ struct clk *rtc;
+};
+
+static int sprd_keypad_enable(struct sprd_keypad_data *data)
+{
+ struct device *dev = data->input_dev->dev.parent;
+ int ret;
+
+ ret = clk_prepare_enable(data->rtc);
+ if (ret) {
+ dev_err(dev, "enable rtc failed.\n");
+ return ret;
+ }
+
+ ret = clk_prepare_enable(data->enable);
+ if (ret) {
+ dev_err(dev, "enable keypad failed.\n");
+ clk_disable_unprepare(data->rtc);
+ return ret;
+ }
+
+ return 0;
+}
+
+static void sprd_keypad_disable(struct sprd_keypad_data *data)
+{
+ clk_disable_unprepare(data->enable);
+ clk_disable_unprepare(data->rtc);
+}
+
+static irqreturn_t sprd_keypad_handler(int irq, void *id)
+{
+ struct platform_device *pdev = id;
+ struct device *dev = &pdev->dev;
+ struct sprd_keypad_data *data = platform_get_drvdata(pdev);
+ u32 int_status = readl_relaxed(data->base + SPRD_KPD_INT_MASK_STATUS);
+ u32 key_status = readl_relaxed(data->base + SPRD_KPD_KEY_STATUS);
+ unsigned short *keycodes = data->input_dev->keycode;
+ u32 row_shift = get_count_order(data->num_cols);
+ unsigned short key;
+ int col, row;
+ u32 i;
+
+ writel_relaxed(SPRD_KPD_INT_ALL, data->base + SPRD_KPD_INT_CLR);
+
+ for (i = 0; i < SPRD_KPD_INT_CNT; i++) {
+ if (SPRD_KPD_PRESS_INTX(i, int_status)) {
+ col = SPRD_KPD_INTX_COL(i, key_status);
+ row = SPRD_KPD_INTX_ROW(i, key_status);
+ key = keycodes[MATRIX_SCAN_CODE(row, col, row_shift)];
+ input_report_key(data->input_dev, key, 1);
+ input_sync(data->input_dev);
+ dev_dbg(dev, "%dD\n", key);
+ }
+ if (SPRD_KPD_RELEASE_INTX(i, int_status)) {
+ col = SPRD_KPD_INTX_COL(i, key_status);
+ row = SPRD_KPD_INTX_ROW(i, key_status);
+ key = keycodes[MATRIX_SCAN_CODE(row, col, row_shift)];
+ input_report_key(data->input_dev, key, 0);
+ input_sync(data->input_dev);
+ dev_dbg(dev, "%dU\n", key);
+ }
+ }
+
+ return IRQ_HANDLED;
+}
+
+static u32 sprd_keypad_time_to_counter(u32 array_size, u32 time_ms)
+{
+ u32 value;
+
+ /*
+ * y(ms) = (x + 1) * array_size
+ * / (32.768 / (clk_div_num + 1))
+ * y means time in ms
+ * x means counter
+ * array_size equal to rows * columns
+ * clk_div_num is devider to keypad source clock
+ **/
+ value = SPRD_KPD_RTC_HZ * time_ms;
+ value = value / (1000 * array_size *
+ (SPRD_DEF_DIV_CNT + 1));
+ if (value >= 1)
+ value -= 1;
+
+ return value;
+}
+
+static int sprd_keypad_hw_init(struct sprd_keypad_data *data)
+{
+ u32 value;
+
+ writel_relaxed(SPRD_KPD_INT_ALL, data->base + SPRD_KPD_INT_CLR);
+ writel_relaxed(SPRD_KPD_ROW_POLARITY | SPRD_KPD_COL_POLARITY,
+ data->base + SPRD_KPD_POLARITY);
+ writel_relaxed(SPRD_DEF_DIV_CNT, data->base + SPRD_KPD_CLK_DIV_CNT);
+
+ value = sprd_keypad_time_to_counter(data->num_rows * data->num_cols,
+ SPRD_DEF_LONG_KEY_MS);
+ writel_relaxed(value, data->base + SPRD_KPD_LONG_KEY_CNT);
+
+ value = sprd_keypad_time_to_counter(data->num_rows * data->num_cols,
+ data->debounce_ms);
+ writel_relaxed(value, data->base + SPRD_KPD_DEBOUNCE_CNT);
+
+ value = SPRD_KPD_INT_DOWNUP;
+ if (data->capabilities & SPRD_CAP_LONG_KEY)
+ value |= SPRD_KPD_INT_LONG;
+ writel_relaxed(value, data->base + SPRD_KPD_INT_EN);
+
+ value = SPRD_KPD_RTC_HZ - 1;
+ writel_relaxed(value, data->base + SPRD_KPD_SLEEP_CNT);
+
+ /* set enabled rows and columns */
+ value = (((data->rows_en << SPRD_KPD_ROWS_SHIFT)
+ | (data->cols_en << SPRD_KPD_COLS_SHIFT))
+ & (SPRD_KPD_ROWS_MSK | SPRD_KPD_COLS_MSK))
+ | SPRD_KPD_EN | SPRD_KPD_SLEEP_EN;
+ if (data->capabilities & SPRD_CAP_LONG_KEY)
+ value |= SPRD_KPD_LONG_KEY_EN;
+ writel_relaxed(value, data->base + SPRD_KPD_CTRL);
+
+ return 0;
+}
+
+static int __maybe_unused sprd_keypad_suspend(struct device *dev)
+{
+ struct sprd_keypad_data *data = dev_get_drvdata(dev);
+
+ if (!device_may_wakeup(dev))
+ sprd_keypad_disable(data);
+
+ return 0;
+}
+
+static int __maybe_unused sprd_keypad_resume(struct device *dev)
+{
+ struct sprd_keypad_data *data = dev_get_drvdata(dev);
+ int ret = 0;
+
+ if (!device_may_wakeup(dev)) {
+ ret = sprd_keypad_enable(data);
+ if (ret)
+ return ret;
+ ret = sprd_keypad_hw_init(data);
+ }
+
+ return ret;
+}
+
+static SIMPLE_DEV_PM_OPS(sprd_keypad_pm_ops,
+ sprd_keypad_suspend, sprd_keypad_resume);
+
+static int sprd_keypad_parse_dt(struct device *dev)
+{
+ struct sprd_keypad_data *data = dev_get_drvdata(dev);
+ struct device_node *np = dev->of_node;
+ int ret;
+
+ ret = matrix_keypad_parse_properties(dev, &data->num_rows, &data->num_cols);
+ if (ret)
+ return ret;
+
+ if (data->num_rows > SPRD_KPD_ROWS_MAX
+ || data->num_cols > SPRD_KPD_COLS_MAX) {
+ dev_err(dev, "invalid num_rows or num_cols\n");
+ return -EINVAL;
+ }
+
+ ret = of_property_read_u32(np, "debounce-interval", &data->debounce_ms);
+ if (ret) {
+ data->debounce_ms = 5;
+ dev_warn(dev, "parse debounce-interval failed.\n");
+ }
+
+ if (of_get_property(np, "linux,repeat", NULL))
+ data->capabilities |= SPRD_CAP_REPEAT;
+ if (of_get_property(np, "sprd,support_long_key", NULL))
+ data->capabilities |= SPRD_CAP_LONG_KEY;
+ if (of_get_property(np, "wakeup-source", NULL))
+ data->capabilities |= SPRD_CAP_WAKEUP;
+
+ data->enable = devm_clk_get(dev, "enable");
+ if (IS_ERR(data->enable)) {
+ if (PTR_ERR(data->enable) != -EPROBE_DEFER)
+ dev_err(dev, "get enable clk failed.\n");
+ return PTR_ERR(data->enable);
+ }
+
+ data->rtc = devm_clk_get(dev, "rtc");
+ if (IS_ERR(data->rtc)) {
+ if (PTR_ERR(data->enable) != -EPROBE_DEFER)
+ dev_err(dev, "get rtc clk failed.\n");
+ return PTR_ERR(data->rtc);
+ }
+
+ return 0;
+}
+
+static int sprd_keypad_probe(struct platform_device *pdev)
+{
+ struct sprd_keypad_data *data;
+ struct resource *res;
+ int ret, irq, i, j, row_shift;
+ unsigned long rows, cols;
+ unsigned short *keycodes;
+
+ data = devm_kzalloc(&pdev->dev, sizeof(*data), GFP_KERNEL);
+ if (!data)
+ return -ENOMEM;
+
+ res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+ data->base = devm_ioremap_resource(&pdev->dev, res);
+ if (IS_ERR(data->base)) {
+ dev_err(&pdev->dev, "ioremap resource failed.\n");
+ ret = PTR_ERR(data->base);
+ goto err_free;
+ }
+
+ platform_set_drvdata(pdev, data);
+ ret = sprd_keypad_parse_dt(&pdev->dev);
+ if (ret) {
+ dev_err(&pdev->dev, "keypad parse dts failed.\n");
+ goto err_free;
+ }
+
+ data->input_dev = devm_input_allocate_device(&pdev->dev);
+ if (IS_ERR(data->input_dev)) {
+ dev_err(&pdev->dev, "alloc input dev failed.\n");
+ ret = PTR_ERR(data->input_dev);
+ goto err_free;
+ }
+
+ data->input_dev->name = "sprd-keypad";
+ data->input_dev->phys = "sprd-key/input0";
+
+ ret = matrix_keypad_build_keymap(NULL, NULL, data->num_rows,
+ data->num_cols, NULL, data->input_dev);
+ if (ret) {
+ dev_err(&pdev->dev, "keypad build keymap failed.\n");
+ goto err_free;
+ }
+
+ rows = cols = 0;
+ row_shift = get_count_order(data->num_cols);
+ keycodes = data->input_dev->keycode;
+ for (i = 0; i < data->num_rows; i++) {
+ for (j = 0; j < data->num_cols; j++) {
+ if (!!keycodes[MATRIX_SCAN_CODE(i, j, row_shift)]) {
+ set_bit(i, &rows);
+ set_bit(j, &cols);
+ }
+ }
+ }
+ data->rows_en = rows;
+ data->cols_en = cols;
+
+ if (data->capabilities & SPRD_CAP_REPEAT)
+ set_bit(EV_REP, data->input_dev->evbit);
+
+ input_set_drvdata(data->input_dev, data);
+
+ ret = sprd_keypad_enable(data);
+ if (ret) {
+ dev_err(&pdev->dev, "keypad enable failed.\n");
+ goto err_free;
+ }
+
+ ret = sprd_keypad_hw_init(data);
+ if (ret) {
+ dev_err(&pdev->dev, "keypad hw init failed.\n");
+ goto clk_free;
+ }
+
+ irq = platform_get_irq(pdev, 0);
+ if (irq < 0) {
+ dev_err(&pdev->dev, "platform get irq failed.\n");
+ goto clk_free;
+ }
+
+ ret = devm_request_irq(&pdev->dev, irq, sprd_keypad_handler,
+ IRQF_NO_SUSPEND, dev_name(&pdev->dev), pdev);
+ if (ret) {
+ dev_err(&pdev->dev, "request irq failed.\n");
+ goto clk_free;
+ }
+
+ ret = input_register_device(data->input_dev);
+ if (ret) {
+ dev_err(&pdev->dev, "register input dev failed\n");
+ goto clk_free;
+ }
+
+ if (data->capabilities & SPRD_CAP_WAKEUP)
+ device_init_wakeup(&pdev->dev, true);
+
+ return 0;
+
+clk_free:
+ sprd_keypad_disable(data);
+err_free:
+ devm_kfree(&pdev->dev, data);
+ return ret;
+}
+
+static int sprd_keypad_remove(struct platform_device *pdev)
+{
+ struct sprd_keypad_data *data = platform_get_drvdata(pdev);
+ int irq = platform_get_irq(pdev, 0);
+
+ if (data->capabilities & SPRD_CAP_WAKEUP)
+ device_init_wakeup(&pdev->dev, false);
+
+ input_unregister_device(data->input_dev);
+ devm_free_irq(&pdev->dev, irq, pdev);
+ sprd_keypad_disable(data);
+
+ return 0;
+}
+
+static const struct of_device_id sprd_keypad_match[] = {
+ { .compatible = "sprd,sc9860-keypad", },
+ {},
+};
+
+static struct platform_driver sprd_keypad_driver = {
+ .driver = {
+ .name = "sprd-keypad",
+ .owner = THIS_MODULE,
+ .of_match_table = sprd_keypad_match,
+ .pm = &sprd_keypad_pm_ops,
+ },
+ .probe = sprd_keypad_probe,
+ .remove = sprd_keypad_remove,
+};
+
+module_platform_driver(sprd_keypad_driver);
+
+MODULE_DESCRIPTION("Spreadtrum KPD Driver");
+MODULE_LICENSE("GPL");
+MODULE_AUTHOR("Neo Hou <[email protected]>");
--
2.17.1
On Thu, Aug 10, 2023 at 08:42:36PM +0800, wenhua lin wrote:
> On Tue, Aug 8, 2023 at 9:51 PM Andy Shevchenko
> <[email protected]> wrote:
> > On Tue, Aug 08, 2023 at 03:25:01PM +0800, Wenhua Lin wrote:
> > > Add matrix keypad driver, support matrix keypad function.
> >
> > Bindings should be prerequisite to this, meaning this has to be a series of
> > two patches.
>
> I don't quite understand what you mean.
> Can you describe the problem in detail?
...
> > > + help
> > > + Keypad controller is used to interface a SoC with a matrix-keypad device,
> > > + The keypad controller supports multiple row and column lines.
> > > + Say Y if you want to use the SPRD keyboard.
> > > + Say M if you want to use the SPRD keyboard on SoC as module.
> >
> > What will be the module name?
>
> Keypad
With capital letter?
Are you sure?
Also I'm asking this here to make sure that you will make sure the users of it
will know without reading source code.
...
> > > obj-$(CONFIG_KEYBOARD_ST_KEYSCAN) += st-keyscan.o
> > > obj-$(CONFIG_KEYBOARD_SUN4I_LRADC) += sun4i-lradc-keys.o
> > > obj-$(CONFIG_KEYBOARD_SUNKBD) += sunkbd.o
> > > +obj-$(CONFIG_KEYBOARD_SPRD) += sprd_keypad.o
> >
> > Are you sure it's ordered correctly?
>
> This configuration is correct, we may not understand what you mean?
> Any suggestions for this modification?
Latin alphabet is an ordered set.
> > > obj-$(CONFIG_KEYBOARD_TC3589X) += tc3589x-keypad.o
> > > obj-$(CONFIG_KEYBOARD_TEGRA) += tegra-kbc.o
> > > obj-$(CONFIG_KEYBOARD_TM2_TOUCHKEY) += tm2-touchkey.o
...
> > Missing bits.h at least.
> > Revisit you header inclusion block to add exactly what you are using.
>
> The sprd_keypad.c file will include <linux/interrupt.h>, interrupt.h
> will include <linux/bitops.h>,
> and the bitops.h file will include bits.h. Can this operation method be used?
Seems you misunderstand how C language works. The idea is that you need to
include _explicitly_ all library / etc headers that your code is using.
The above is a hackish way to achieve that.
...
> > > +#include <linux/of.h>
> >
> > Why?
>
> ret = of_property_read_u32(np, "debounce-interval", &data->debounce_ms);
> if (of_get_property(np, "linux,repeat", NULL))
>
> Two interfaces of_property_read_u32 and of_get_property are used
> in the sprd_keypad_parse_dt function. If of.h is not included, the
> compilation will report an error.
Do not use of_*() in a new code for the drivers.
There are only few exceptions and this driver is not one of them.
...
> > > +#define SPRD_DEF_LONG_KEY_MS 1000
> >
> > (1 * MSEC_PER_SEC)
> >
> > ?
>
> Yes.
It makes more sense to update so easier to get the value and units from
the value.
...
> > > + u32 rows_en; /* enabled rows bits */
> > > + u32 cols_en; /* enabled cols bits */
> >
> > Why not bitmaps?
>
> Bitmap has been used, each bit represents different rows and different columns.
I meant the bitmap type (as of bitmap.h APIs).
...
> > > +static int sprd_keypad_parse_dt(struct device *dev)
> >
> > dt -> fw
>
> I don't quite understand what you mean,
> is it to change the function name to sprd_keypad_parse_fw?
Yes. And make it firmware (which may be ACPI/DT or something else).
...
> > And I'm wondering if input subsystem already does this for you.
>
> I don't quite understand what you mean.
Does input subsystem parse the (some of) device properties already?
...
> > > + data->enable = devm_clk_get(dev, "enable");
> > > + if (IS_ERR(data->enable)) {
> > > + if (PTR_ERR(data->enable) != -EPROBE_DEFER)
> > > + dev_err(dev, "get enable clk failed.\n");
> > > + return PTR_ERR(data->enable);
> > > + }
> > > +
> > > + data->rtc = devm_clk_get(dev, "rtc");
> > > + if (IS_ERR(data->rtc)) {
> > > + if (PTR_ERR(data->enable) != -EPROBE_DEFER)
> > > + dev_err(dev, "get rtc clk failed.\n");
> > > + return PTR_ERR(data->rtc);
> > > + }
> >
> > Why not bulk?
> > Why not _enabled() variant?
>
> I don't quite understand what you mean.
> Can you give me an example?
Hmm... seems there is no existing API like that, but still you have bulk
operations.
$ git grep -n clk_bulk.*\( -- include/linux/clk.h
...
> > > + data->base = devm_ioremap_resource(&pdev->dev, res);
> >
> > devm_platform_ioremap_resource()
> >
> > > + if (IS_ERR(data->base)) {
> >
> > > + dev_err(&pdev->dev, "ioremap resource failed.\n");
> >
> > Dup message.
>
> Do you mean : dev_err(&pdev->dev, "ioremap resource failed.\n")，
> I think it is necessary to prompt accurate error message.
Yes, and yours is a duplication of a such.
> > > + ret = PTR_ERR(data->base);
> > > + goto err_free;
> > > + }
...
> > > + dev_err(&pdev->dev, "alloc input dev failed.\n");
> > > + ret = PTR_ERR(data->input_dev);
>
> > Too many spaces.
>
> We will fix this issue in patch v2.
>
> > Here and elsewhere in ->probe() use return dev_err_probe() approach as Dmitry
> > nowadays is okay with that.
>
> I don't quite understand what you mean.
> Can you describe it in detail?
return dev_err_probe(...);
or
ret = dev_err_probe(... PTR_ERR(...), ...);
Btw, most of your questions can be answered by looking into the lately added
drivers in the input subsystem.
...
> > > +clk_free:
> > > + sprd_keypad_disable(data);
> >
> > See above how this can be avoided.
>
> This is hard to explain.
What do you mean?
But I guess somebody already mentioned devm_add_action_or_reset().
...
> > > +err_free:
> > > + devm_kfree(&pdev->dev, data);
> >
> > Huh?!
It's a red flag, and you have no answer to it...
> > > + return ret;
...
> > > + .owner = THIS_MODULE,
> >
> > ~15 years this is not needed.
> > Where did you get this code from? Time machine?
>
> Do you mean the keypad driver is no longer in use?
No, I meant specifically emphasized line.
--
With Best Regards,
Andy Shevchenko
On Tue, Aug 8, 2023 at 5:07 PM Jonathan Cameron
<[email protected]> wrote:
>
> On Tue, 8 Aug 2023 15:25:01 +0800
> Wenhua Lin <[email protected]> wrote:
>
> > Add matrix keypad driver, support matrix keypad function.
> >
>
> No idea why you cc'd me on this one, but I wanted some light reading whilst
> having a coffee so here's a quick review :)
>
> Hohum. Took me a bit long than planned. Ah well, I hope you find the
> feedback useful.
We may have made a mistake, your feedback is greatly appreciated,
these suggestions help us a lot.
>
> > Signed-off-by: Wenhua Lin <[email protected]>
> > ---
> > drivers/input/keyboard/Kconfig | 10 +
> > drivers/input/keyboard/Makefile | 1 +
> > drivers/input/keyboard/sprd_keypad.c | 418 +++++++++++++++++++++++++++
> > 3 files changed, 429 insertions(+)
> > create mode 100644 drivers/input/keyboard/sprd_keypad.c
> >
> > diff --git a/drivers/input/keyboard/Kconfig b/drivers/input/keyboard/Kconfig
> > index 1d0c5f4c0f99..f35d9bf05f72 100644
> > --- a/drivers/input/keyboard/Kconfig
> > +++ b/drivers/input/keyboard/Kconfig
> > @@ -809,4 +809,14 @@ config KEYBOARD_CYPRESS_SF
> > To compile this driver as a module, choose M here: the
> > module will be called cypress-sf.
> >
> > +config KEYBOARD_SPRD
> > + tristate "Spreadtrum keyboard support"
> > + depends on ARCH_SPRD || COMPILE_TEST
> > + select INPUT_MATRIXKMAP
> > + help
> > + Keypad controller is used to interface a SoC with a matrix-keypad device,
> > + The keypad controller supports multiple row and column lines.
> > + Say Y if you want to use the SPRD keyboard.
> > + Say M if you want to use the SPRD keyboard on SoC as module.
> > +
> > endif
> > diff --git a/drivers/input/keyboard/Makefile b/drivers/input/keyboard/Makefile
> > index aecef00c5d09..b747112461b1 100644
> > --- a/drivers/input/keyboard/Makefile
> > +++ b/drivers/input/keyboard/Makefile
> > @@ -66,6 +66,7 @@ obj-$(CONFIG_KEYBOARD_STOWAWAY) += stowaway.o
> > obj-$(CONFIG_KEYBOARD_ST_KEYSCAN) += st-keyscan.o
> > obj-$(CONFIG_KEYBOARD_SUN4I_LRADC) += sun4i-lradc-keys.o
> > obj-$(CONFIG_KEYBOARD_SUNKBD) += sunkbd.o
> > +obj-$(CONFIG_KEYBOARD_SPRD) += sprd_keypad.o
> > obj-$(CONFIG_KEYBOARD_TC3589X) += tc3589x-keypad.o
> > obj-$(CONFIG_KEYBOARD_TEGRA) += tegra-kbc.o
> > obj-$(CONFIG_KEYBOARD_TM2_TOUCHKEY) += tm2-touchkey.o
> > diff --git a/drivers/input/keyboard/sprd_keypad.c b/drivers/input/keyboard/sprd_keypad.c
> > new file mode 100644
> > index 000000000000..5b88072831e8
> > --- /dev/null
> > +++ b/drivers/input/keyboard/sprd_keypad.c
> > @@ -0,0 +1,418 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Copyright (C) 2018 Spreadtrum Communications Inc.
> > + */
> > +
> > +#include <linux/module.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/input/matrix_keypad.h>
> > +#include <linux/io.h>
> > +#include <linux/interrupt.h>
> > +#include <linux/clk.h>
> > +#include <linux/of.h>
> > +#include <linux/input.h>
>
> Some sort of order of headers would be good.
> Alphabetical is a good choice, though maybe Dmitry prefers something else.
We will fix this issue in patch v2.
>
> > +
> > +#define SPRD_KPD_CTRL 0x0
> > +#define SPRD_KPD_INT_EN 0x4
> > +#define SPRD_KPD_INT_RAW_STATUS 0x8
> > +#define SPRD_KPD_INT_MASK_STATUS 0xc
> > +#define SPRD_KPD_INT_CLR 0x10
> > +#define SPRD_KPD_POLARITY 0x18
> > +#define SPRD_KPD_DEBOUNCE_CNT 0x1c
> > +#define SPRD_KPD_LONG_KEY_CNT 0x20
> > +#define SPRD_KPD_SLEEP_CNT 0x24
> > +#define SPRD_KPD_CLK_DIV_CNT 0x28
> > +#define SPRD_KPD_KEY_STATUS 0x2c
> > +#define SPRD_KPD_SLEEP_STATUS 0x30
> > +#define SPRD_KPD_DEBUG_STATUS1 0x34
> > +#define SPRD_KPD_DEBUG_STATUS2 0x38
> > +
> > +#define SPRD_KPD_EN BIT(0)
> > +#define SPRD_KPD_SLEEP_EN BIT(1)
> > +#define SPRD_KPD_LONG_KEY_EN BIT(2)
> > +
> > +#define SPRD_KPD_ROWS_MSK GENMASK(23, 16)
> > +#define SPRD_KPD_COLS_MSK GENMASK(15, 8)
> > +
> > +#define SPRD_KPD_INT_ALL GENMASK(11, 0)
> > +#define SPRD_KPD_INT_DOWNUP GENMASK(7, 0)
> > +#define SPRD_KPD_INT_LONG GENMASK(11, 8)
> > +
> > +#define SPRD_KPD_ROW_POLARITY GENMASK(7, 0)
> > +#define SPRD_KPD_COL_POLARITY GENMASK(15, 8)
> > +
> > +#define SPRD_KPD_PRESS_INTX(X, V) \
> > + (((V) >> (X)) & GENMASK(0, 0))
>
> Given how this is used as a boolean check, I think
> if (SPRD_KPD_PRESS_INTX(i, int_status))
> is same as
>
> if (int_status & BIT(i)) which id easier to read.
Thanks for your suggestion, I will seriously consider it.
>
> > +#define SPRD_KPD_RELEASE_INTX(X, V) \
> > + (((V) >> ((X) + 4)) & GENMASK(0, 0))
> For this one I'd define a mask and use field get so the check
>
> if (SPRD_KPD_RELEASE_INTX(i, int_status)) {
>
> becomes
>
> #define SPRD_KPD_RELEASE_INTX_MSK GENMASK(7, 4)
>
> if (FIELD_GET(SPRD_KPD_RELEASE_INTX_MSK, int_status) & BIT(i));
>
Thanks for your suggestion, I will seriously consider it.
>
> > +#define SPRD_KPD_INTX_COL(X, V) \
> > + (((V) >> ((X) << 3)) & GENMASK(2, 0))
> > +#define SPRD_KPD_INTX_ROW(X, V) \
> > + (((V) >> (((X) << 3) + 4)) & GENMASK(2, 0))
>
> Ok, on this I'm struggling to work out what is actually happening.
>
> Looks to be picking out an 8 bit field then masking with 3 bits.
> X = 0..3
>
> So define the mask with a suitable name and provide a macro to extract
> only the 8 bit field. I would suggest using multiply as simpler.
>
> Something along the lines of...
>
> #define SPRD_KBD_INTX_COL_MSK GENMASK(2, 0)
> #define SPRD_KBD_INTX_ROW_MSK GENMASK(6, 4)
> static u8 sprd_kbd_intx_extract_entry(u32 key_input, int x)
> {
> return key_input >> (x * 8);
> }
> (key_input >> (X * 8)) & GENMASK(2, 0)
Thanks for your suggestion, but
I am concerned that the modification of the algorithm
will affect the understanding of register usage.
>
> So
> col = SPRD_KPD_INTX_COL(i, key_status);
> row = SPRD_KPD_INTX_ROW(i, key_status);
> becomes
>
> u8 entry = sprd_kbd_intx_extract_entry(key_input, i);
> col = FIELD_GET(SPRD_KBD_INTX_COL_MSK, entry)
> row = FIELD_GET(SPRD_KBD_INTX_ROW_MSK, entry);
>
> which is easier to follow than above macros.
> That of course assumes I correctly figured out what those macros
> were doing.
>
> This is a case where readability is better than short code.
>
Thanks for your suggestion, I will seriously consider it.
>
>
> > +#define SPRD_KPD_INTX_DOWN(X, V) \
> > + (((V) >> (((X) << 3) + 7)) & GENMASK(0, 0))
> > +
>
>
> > +
> > +static u32 sprd_keypad_time_to_counter(u32 array_size, u32 time_ms)
> > +{
> > + u32 value;
> > +
> > + /*
> > + * y(ms) = (x + 1) * array_size
> > + * / (32.768 / (clk_div_num + 1))
> > + * y means time in ms
> > + * x means counter
> > + * array_size equal to rows * columns
> > + * clk_div_num is devider to keypad source clock
>
> divider
> Also good to say the maths here is inverting the equation given.
We will fix this issue in patch v2.
>
> > + **/
> > + value = SPRD_KPD_RTC_HZ * time_ms;
> > + value = value / (1000 * array_size *
> > + (SPRD_DEF_DIV_CNT + 1));
> > + if (value >= 1)
> > + value -= 1;
> Good to have a comment on why this last check. Can it end up as
> less than one.
This code is implemented according to the calculation formula, and x
represents value.
y(ms) = (x + 1) * array_size / (32.768 / (clk_div_num + 1))
> > +
> > + return value;
> > +}
> > +
> > +static int sprd_keypad_hw_init(struct sprd_keypad_data *data)
> > +{
> > + u32 value;
> > +
> > + writel_relaxed(SPRD_KPD_INT_ALL, data->base + SPRD_KPD_INT_CLR);
> > + writel_relaxed(SPRD_KPD_ROW_POLARITY | SPRD_KPD_COL_POLARITY,
> > + data->base + SPRD_KPD_POLARITY);
> > + writel_relaxed(SPRD_DEF_DIV_CNT, data->base + SPRD_KPD_CLK_DIV_CNT);
> > +
> > + value = sprd_keypad_time_to_counter(data->num_rows * data->num_cols,
> > + SPRD_DEF_LONG_KEY_MS);
> > + writel_relaxed(value, data->base + SPRD_KPD_LONG_KEY_CNT);
> > +
> > + value = sprd_keypad_time_to_counter(data->num_rows * data->num_cols,
> > + data->debounce_ms);
> > + writel_relaxed(value, data->base + SPRD_KPD_DEBOUNCE_CNT);
> > +
> > + value = SPRD_KPD_INT_DOWNUP;
> > + if (data->capabilities & SPRD_CAP_LONG_KEY)
> > + value |= SPRD_KPD_INT_LONG;
> > + writel_relaxed(value, data->base + SPRD_KPD_INT_EN);
> > +
> > + value = SPRD_KPD_RTC_HZ - 1;
> > + writel_relaxed(value, data->base + SPRD_KPD_SLEEP_CNT);
> > +
> > + /* set enabled rows and columns */
> > + value = (((data->rows_en << SPRD_KPD_ROWS_SHIFT)
> > + | (data->cols_en << SPRD_KPD_COLS_SHIFT))
> > + & (SPRD_KPD_ROWS_MSK | SPRD_KPD_COLS_MSK))
> > + | SPRD_KPD_EN | SPRD_KPD_SLEEP_EN;
> > + if (data->capabilities & SPRD_CAP_LONG_KEY)
> > + value |= SPRD_KPD_LONG_KEY_EN;
> > + writel_relaxed(value, data->base + SPRD_KPD_CTRL);
> > +
> > + return 0;
> > +}
> > +
> > +static int __maybe_unused sprd_keypad_suspend(struct device *dev)
>
> What Arnd said on this.
He made no comment on this issue.
>
> > +{
> > + struct sprd_keypad_data *data = dev_get_drvdata(dev);
> > +
> > + if (!device_may_wakeup(dev))
> > + sprd_keypad_disable(data);
> > +
> > + return 0;
> > +}
> > +
> > +static int __maybe_unused sprd_keypad_resume(struct device *dev)
> > +{
> > + struct sprd_keypad_data *data = dev_get_drvdata(dev);
> > + int ret = 0;
> > +
> > + if (!device_may_wakeup(dev)) {
> > + ret = sprd_keypad_enable(data);
> > + if (ret)
> > + return ret;
> > + ret = sprd_keypad_hw_init(data);
> > + }
> > +
> > + return ret;
> > +}
> > +
> > +static SIMPLE_DEV_PM_OPS(sprd_keypad_pm_ops,
> > + sprd_keypad_suspend, sprd_keypad_resume);
> > +
> > +static int sprd_keypad_parse_dt(struct device *dev)
> > +{
> > + struct sprd_keypad_data *data = dev_get_drvdata(dev);
> > + struct device_node *np = dev->of_node;
> > + int ret;
> > +
> > + ret = matrix_keypad_parse_properties(dev, &data->num_rows, &data->num_cols);
> > + if (ret)
> > + return ret;
> > +
> > + if (data->num_rows > SPRD_KPD_ROWS_MAX
> > + || data->num_cols > SPRD_KPD_COLS_MAX) {
> > + dev_err(dev, "invalid num_rows or num_cols\n");
>
> This is only called from probe, so dev_err_probe() is appropriate throughout this
> function.
We will fix this issue in patch v2.
>
> > + return -EINVAL;
> > + }
> > +
> > + ret = of_property_read_u32(np, "debounce-interval", &data->debounce_ms);
>
> Whilst it's probably unlikely another firmware will be used with this,
> we have generic property accessors in linux/property.h that will work should anyone
> ever do so and at no cost for this driver.
We will fix this issue in patch v2.
>
> > + if (ret) {
> > + data->debounce_ms = 5;
> > + dev_warn(dev, "parse debounce-interval failed.\n");
> > + }
> > +
> > + if (of_get_property(np, "linux,repeat", NULL))
>
> device_property_read_bool() calls the check on the property being present
> so is both more general and more obvious than what you have here.
We will fix this issue in patch v2.
>
>
> > + data->capabilities |= SPRD_CAP_REPEAT;
> > + if (of_get_property(np, "sprd,support_long_key", NULL))
> > + data->capabilities |= SPRD_CAP_LONG_KEY;
> > + if (of_get_property(np, "wakeup-source", NULL))
> > + data->capabilities |= SPRD_CAP_WAKEUP;
> > +
> > + data->enable = devm_clk_get(dev, "enable");
>
> devm_clk_get_enabled() for both of these.
We will fix this issue in patch v2.
>
> > + if (IS_ERR(data->enable)) {
> > + if (PTR_ERR(data->enable) != -EPROBE_DEFER)
> > + dev_err(dev, "get enable clk failed.\n");
> > + return PTR_ERR(data->enable);
> > + }
> > +
> > + data->rtc = devm_clk_get(dev, "rtc");
> > + if (IS_ERR(data->rtc)) {
> > + if (PTR_ERR(data->enable) != -EPROBE_DEFER)
> > + dev_err(dev, "get rtc clk failed.\n");
> > + return PTR_ERR(data->rtc);
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +static int sprd_keypad_probe(struct platform_device *pdev)
> > +{
> > + struct sprd_keypad_data *data;
> > + struct resource *res;
> > + int ret, irq, i, j, row_shift;
> > + unsigned long rows, cols;
> > + unsigned short *keycodes;
> > +
> > + data = devm_kzalloc(&pdev->dev, sizeof(*data), GFP_KERNEL);
> > + if (!data)
> > + return -ENOMEM;
> > +
> > + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > + data->base = devm_ioremap_resource(&pdev->dev, res);
>
> devm_platform_get_and_ioremap_resource)
>
> > + if (IS_ERR(data->base)) {
> > + dev_err(&pdev->dev, "ioremap resource failed.\n");
> > + ret = PTR_ERR(data->base);
> > + goto err_free;
>
> Read up on what devm calls do - there is no need to manually free
> things that were allocated with them in the error paths or remove.
> So this should be a direct return. Also use
> return dev_err_probe(&pdev->dev, PTR_ERR(data->base),
> "....")
>
> It both creates neater code and for cases where deferred probing
> is possible you will get a nice message on 'why' registered for
> debug purposes.
We will fix this issue in patch v2.
>
>
> > + }
> > +
> > + platform_set_drvdata(pdev, data);
> > + ret = sprd_keypad_parse_dt(&pdev->dev);
> > + if (ret) {
> > + dev_err(&pdev->dev, "keypad parse dts failed.\n");
> > + goto err_free;
>
> Direct return and dev_err_probe()
We will fix this issue in patch v2.
>
> > + }
> > +
> > + data->input_dev = devm_input_allocate_device(&pdev->dev);
> > + if (IS_ERR(data->input_dev)) {
> > + dev_err(&pdev->dev, "alloc input dev failed.\n");
> > + ret = PTR_ERR(data->input_dev);
> > + goto err_free;
>
> Direct return, dev_err_probe() and what happened with the spacing?
We will fix this issue in patch v2.
>
> > + }
> > +
> > + data->input_dev->name = "sprd-keypad";
> > + data->input_dev->phys = "sprd-key/input0";
> > +
> > + ret = matrix_keypad_build_keymap(NULL, NULL, data->num_rows,
> > + data->num_cols, NULL, data->input_dev);
> > + if (ret) {
> > + dev_err(&pdev->dev, "keypad build keymap failed.\n");
> > + goto err_free;
>
> As above.
We will fix this issue in patch v2.
>
> > + }
> > +
> > + rows = cols = 0;
> > + row_shift = get_count_order(data->num_cols);
> > + keycodes = data->input_dev->keycode;
> > + for (i = 0; i < data->num_rows; i++) {
> > + for (j = 0; j < data->num_cols; j++) {
> > + if (!!keycodes[MATRIX_SCAN_CODE(i, j, row_shift)]) {
>
> The !! is pointless if using it as a boolean. No need to first convert it
> to 0/1 0 is false anything else is true.
>
> > + set_bit(i, &rows);
> > + set_bit(j, &cols);
> > + }
> > + }
> > + }
> > + data->rows_en = rows;
> > + data->cols_en = cols;
> > +
> > + if (data->capabilities & SPRD_CAP_REPEAT)
> > + set_bit(EV_REP, data->input_dev->evbit);
> > +
> > + input_set_drvdata(data->input_dev, data);
> > +
> > + ret = sprd_keypad_enable(data);
> > + if (ret) {
> > + dev_err(&pdev->dev, "keypad enable failed.\n");
> > + goto err_free;
> Same again.
>
> > + }
>
> I'd suggest a suitable callback and devm_add_action_or_reset()
> to unwind the enable.
>
> Actually seeing the code above, just call
> devm_clk_get_enabled() here and drop the enable / disable functions.
We will fix this issue in patch v2.
>
>
> > +
> > + ret = sprd_keypad_hw_init(data);
> > + if (ret) {
> > + dev_err(&pdev->dev, "keypad hw init failed.\n");
> > + goto clk_free;
> > + }
> > +
> > + irq = platform_get_irq(pdev, 0);
> > + if (irq < 0) {
> > + dev_err(&pdev->dev, "platform get irq failed.\n");
> > + goto clk_free;
> > + }
> > +
> > + ret = devm_request_irq(&pdev->dev, irq, sprd_keypad_handler,
> > + IRQF_NO_SUSPEND, dev_name(&pdev->dev), pdev);
> > + if (ret) {
> > + dev_err(&pdev->dev, "request irq failed.\n");
> > + goto clk_free;
> > + }
> > +
> > + ret = input_register_device(data->input_dev);
>
> Whilst there isn't a specific devm_ version of this, that is because there
> doesn't need to be one. Have a look at the implementation and how
> it handles things when input_dev->devres_managed is set.
>
> Upshot, you don't need to manually unwind this either.
I don't quite understand what you mean.
>
> > + if (ret) {
> > + dev_err(&pdev->dev, "register input dev failed\n");
> > + goto clk_free;
> > + }
> > +
> > + if (data->capabilities & SPRD_CAP_WAKEUP)
> > + device_init_wakeup(&pdev->dev, true);
> Another devm_add_action_or_reset() use case. Note: only register
> the cleanup, if you use device_init_wakeup() here.
Thanks for your suggestion, I will seriously consider it.
>
> > +
> > + return 0;
> > +
> > +clk_free:
> > + sprd_keypad_disable(data);
> > +err_free:
> > + devm_kfree(&pdev->dev, data);
> With changes above, there will be no manual cleanup to do here.
We will fix this issue in patch v2.
> > + return ret;
> > +}
> > +
> > +static int sprd_keypad_remove(struct platform_device *pdev)
> > +{
> > + struct sprd_keypad_data *data = platform_get_drvdata(pdev);
> > + int irq = platform_get_irq(pdev, 0);
> > +
> > + if (data->capabilities & SPRD_CAP_WAKEUP)
> > + device_init_wakeup(&pdev->dev, false);
> > +
> > + input_unregister_device(data->input_dev);
> > + devm_free_irq(&pdev->dev, irq, pdev);
>
> Calling a devm free is usually a bad sign and implies you shouldn't have used
> devm to get the thing in the first place.
> The two should not be mixed - so the moment you reach a call in probe() that
> you don't want to use devm_ managed releases for, stop using them for everything
> after that point. There is devm_add_action_or_reset() though which can be used
> to register your own cleanup functions and sometimes lets you take the whole
> of the release flow device managed.
>
> Suggestions above mean you will have no remove() function at all.
Thanks for your suggestion, I will seriously consider it.
>
>
> > + sprd_keypad_disable(data);
> > +
> > + return 0;
> > +}
> > +
> > +static const struct of_device_id sprd_keypad_match[] = {
> > + { .compatible = "sprd,sc9860-keypad", },
> > + {},
> > +};
> > +
> > +static struct platform_driver sprd_keypad_driver = {
> > + .driver = {
> > + .name = "sprd-keypad",
> > + .owner = THIS_MODULE,
> > + .of_match_table = sprd_keypad_match,
> > + .pm = &sprd_keypad_pm_ops,
> > + },
> > + .probe = sprd_keypad_probe,
> > + .remove = sprd_keypad_remove,
> > +};
> > +
> > +module_platform_driver(sprd_keypad_driver);
> > +
> > +MODULE_DESCRIPTION("Spreadtrum KPD Driver");
> > +MODULE_LICENSE("GPL");
> > +MODULE_AUTHOR("Neo Hou <[email protected]>");
>
On Thu, Aug 10, 2023 at 10:01 PM Andy Shevchenko
<[email protected]> wrote:
>
> On Thu, Aug 10, 2023 at 08:42:36PM +0800, wenhua lin wrote:
> > On Tue, Aug 8, 2023 at 9:51 PM Andy Shevchenko
> > <[email protected]> wrote:
> > > On Tue, Aug 08, 2023 at 03:25:01PM +0800, Wenhua Lin wrote:
> > > > Add matrix keypad driver, support matrix keypad function.
> > >
> > > Bindings should be prerequisite to this, meaning this has to be a series of
> > > two patches.
> >
> > I don't quite understand what you mean.
> > Can you describe the problem in detail?
>
> ...
>
> > > > + help
> > > > + Keypad controller is used to interface a SoC with a matrix-keypad device,
> > > > + The keypad controller supports multiple row and column lines.
> > > > + Say Y if you want to use the SPRD keyboard.
> > > > + Say M if you want to use the SPRD keyboard on SoC as module.
> > >
> > > What will be the module name?
> >
> > Keypad
>
> With capital letter?
> Are you sure?
>
> Also I'm asking this here to make sure that you will make sure the users of it
> will know without reading source code.
Thank you very much for your review.
I will fix this issue in patch v2.
>
> ...
>
> > > > obj-$(CONFIG_KEYBOARD_ST_KEYSCAN) += st-keyscan.o
> > > > obj-$(CONFIG_KEYBOARD_SUN4I_LRADC) += sun4i-lradc-keys.o
> > > > obj-$(CONFIG_KEYBOARD_SUNKBD) += sunkbd.o
> > > > +obj-$(CONFIG_KEYBOARD_SPRD) += sprd_keypad.o
> > >
> > > Are you sure it's ordered correctly?
> >
> > This configuration is correct, we may not understand what you mean?
> > Any suggestions for this modification?
>
> Latin alphabet is an ordered set.
I will fix this issue in patch v2.
>
> > > > obj-$(CONFIG_KEYBOARD_TC3589X) += tc3589x-keypad.o
> > > > obj-$(CONFIG_KEYBOARD_TEGRA) += tegra-kbc.o
> > > > obj-$(CONFIG_KEYBOARD_TM2_TOUCHKEY) += tm2-touchkey.o
>
> ...
>
> > > Missing bits.h at least.
> > > Revisit you header inclusion block to add exactly what you are using.
> >
> > The sprd_keypad.c file will include <linux/interrupt.h>, interrupt.h
> > will include <linux/bitops.h>,
> > and the bitops.h file will include bits.h. Can this operation method be used?
>
> Seems you misunderstand how C language works. The idea is that you need to
> include _explicitly_ all library / etc headers that your code is using.
> The above is a hackish way to achieve that.
I will fix this issue in patch v2.
>
> ...
>
> > > > +#include <linux/of.h>
> > >
> > > Why?
> >
> > ret = of_property_read_u32(np, "debounce-interval", &data->debounce_ms);
> > if (of_get_property(np, "linux,repeat", NULL))
> >
> > Two interfaces of_property_read_u32 and of_get_property are used
> > in the sprd_keypad_parse_dt function. If of.h is not included, the
> > compilation will report an error.
>
> Do not use of_*() in a new code for the drivers.
> There are only few exceptions and this driver is not one of them.
I will fix this issue in patch v2.
>
> ...
>
> > > > +#define SPRD_DEF_LONG_KEY_MS 1000
> > >
> > > (1 * MSEC_PER_SEC)
> > >
> > > ?
> >
> > Yes.
>
> It makes more sense to update so easier to get the value and units from
> the value.
>
> ...
>
> > > > + u32 rows_en; /* enabled rows bits */
> > > > + u32 cols_en; /* enabled cols bits */
> > >
> > > Why not bitmaps?
> >
> > Bitmap has been used, each bit represents different rows and different columns.
>
> I meant the bitmap type (as of bitmap.h APIs).
I understand what you mean, I need to study how this bitmap is used.
>
> ...
>
> > > > +static int sprd_keypad_parse_dt(struct device *dev)
> > >
> > > dt -> fw
> >
> > I don't quite understand what you mean,。
> > is it to change the function name to sprd_keypad_parse_fw?
>
> Yes. And make it firmware (which may be ACPI/DT or something else).
We need to think about how to modify it.
>
> ...
>
> > > And I'm wondering if input subsystem already does this for you.
> >
> > I don't quite understand what you mean.
>
> Does input subsystem parse the (some of) device properties already?
Yes
>
> ...
>
> > > > + data->enable = devm_clk_get(dev, "enable");
> > > > + if (IS_ERR(data->enable)) {
> > > > + if (PTR_ERR(data->enable) != -EPROBE_DEFER)
> > > > + dev_err(dev, "get enable clk failed.\n");
> > > > + return PTR_ERR(data->enable);
> > > > + }
> > > > +
> > > > + data->rtc = devm_clk_get(dev, "rtc");
> > > > + if (IS_ERR(data->rtc)) {
> > > > + if (PTR_ERR(data->enable) != -EPROBE_DEFER)
> > > > + dev_err(dev, "get rtc clk failed.\n");
> > > > + return PTR_ERR(data->rtc);
> > > > + }
> > >
> > > Why not bulk?
> > > Why not _enabled() variant?
> >
> > I don't quite understand what you mean.
> > Can you give me an example?
>
> Hmm... seems there is no existing API like that, but still you have bulk
> operations.
>
> $ git grep -n clk_bulk.*\( -- include/linux/clk.h
>
> ...
I will fix this issue in patch v2.
>
> > > > + data->base = devm_ioremap_resource(&pdev->dev, res);
> > >
> > > devm_platform_ioremap_resource()
> > >
> > > > + if (IS_ERR(data->base)) {
> > >
> > > > + dev_err(&pdev->dev, "ioremap resource failed.\n");
> > >
> > > Dup message.
> >
> > Do you mean : dev_err(&pdev->dev, "ioremap resource failed.\n")，
> > I think it is necessary to prompt accurate error message.
>
> Yes, and yours is a duplication of a such.
>
> > > > + ret = PTR_ERR(data->base);
> > > > + goto err_free;
> > > > + }
>
> ...
>
> > > > + dev_err(&pdev->dev, "alloc input dev failed.\n");
> > > > + ret = PTR_ERR(data->input_dev);
> >
> > > Too many spaces.
> >
> > We will fix this issue in patch v2.
> >
> > > Here and elsewhere in ->probe() use return dev_err_probe() approach as Dmitry
> > > nowadays is okay with that.
> >
> > I don't quite understand what you mean.
> > Can you describe it in detail?
>
> return dev_err_probe(...);
>
> or
>
> ret = dev_err_probe(... PTR_ERR(...), ...);
>
> Btw, most of your questions can be answered by looking into the lately added
> drivers in the input subsystem.
I need to refer to the new input subsystem and modify the code again,
thank you very much for your suggestion
>
> ...
>
> > > > +clk_free:
> > > > + sprd_keypad_disable(data);
> > >
> > > See above how this can be avoided.
> >
> > This is hard to explain.
>
> What do you mean?
> But I guess somebody already mentioned devm_add_action_or_reset().
I may need to understand how to use this interface
and fix this problem in patch v2.
>
> ...
>
> > > > +err_free:
> > > > + devm_kfree(&pdev->dev, data);
> > >
> > > Huh?!
>
> It's a red flag, and you have no answer to it...
I realized the problem, the interface using devm_ does not need to do the free.
I will fix this issue in patch v2.
>
> > > > + return ret;
>
> ...
>
> > > > + .owner = THIS_MODULE,
> > >
> > > ~15 years this is not needed.
> > > Where did you get this code from? Time machine?
> >
> > Do you mean the keypad driver is no longer in use?
>
> No, I meant specifically emphasized line.
The keypad driver code is used on the platform
and has not been submitted to the community.
>
> --
> With Best Regards,
> Andy Shevchenko
>
>
On Fri, Aug 11, 2023 at 03:31:01PM +0800, wenhua lin wrote:
> On Thu, Aug 10, 2023 at 10:01 PM Andy Shevchenko
> <[email protected]> wrote:
> > On Thu, Aug 10, 2023 at 08:42:36PM +0800, wenhua lin wrote:
> > > On Tue, Aug 8, 2023 at 9:51 PM Andy Shevchenko
> > > <[email protected]> wrote:
> > > > On Tue, Aug 08, 2023 at 03:25:01PM +0800, Wenhua Lin wrote:
...
> > > > > + u32 rows_en; /* enabled rows bits */
> > > > > + u32 cols_en; /* enabled cols bits */
> > > >
> > > > Why not bitmaps?
> > >
> > > Bitmap has been used, each bit represents different rows and different columns.
> >
> > I meant the bitmap type (as of bitmap.h APIs).
>
> I understand what you mean, I need to study how this bitmap is used.
Input subsystem already is using them.
...
> > > > > +static int sprd_keypad_parse_dt(struct device *dev)
> > > >
> > > > dt -> fw
> > >
> > > I don't quite understand what you mean,。
> > > is it to change the function name to sprd_keypad_parse_fw?
> >
> > Yes. And make it firmware (which may be ACPI/DT or something else).
>
> We need to think about how to modify it.
As I told already, replace mention of "DT"/"OF" by "firmware" and use device
property APIs as per property.h.
...
> > > > And I'm wondering if input subsystem already does this for you.
> > >
> > > I don't quite understand what you mean.
> >
> > Does input subsystem parse the (some of) device properties already?
>
> Yes
Does it cover what you are parsing here? At least partially...
...
> > > > > +err_free:
> > > > > + devm_kfree(&pdev->dev, data);
> > > >
> > > > Huh?!
> >
> > It's a red flag, and you have no answer to it...
>
> I realized the problem, the interface using devm_ does not need to do the free.
> I will fix this issue in patch v2.
The problem is to understand where you can and where you can't use devm_*()
in the first place. _Then_ as you said.
> > > > > + return ret;
...
> > > > > + .owner = THIS_MODULE,
> > > >
> > > > ~15 years this is not needed.
> > > > Where did you get this code from? Time machine?
> > >
> > > Do you mean the keypad driver is no longer in use?
> >
> > No, I meant specifically emphasized line.
>
> The keypad driver code is used on the platform
> and has not been submitted to the community.
I'm not sure I understand to what you reply here...
I'm talking about the "owner" member assignment in the respective
data structure.
--
With Best Regards,
Andy Shevchenko