On Tue, Aug 8, 2023 at 9:51 PM Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx> 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 > > ... > > > 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? > > > obj-$(CONFIG_KEYBOARD_TC3589X) += tc3589x-keypad.o > > obj-$(CONFIG_KEYBOARD_TEGRA) += tegra-kbc.o > > obj-$(CONFIG_KEYBOARD_TM2_TOUCHKEY) += tm2-touchkey.o > > ... > > > +/* > > + * Copyright (C) 2018 Spreadtrum Communications Inc. > > + */ > > A single line > > ... > > 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? > > +#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> > > 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. > > > +#include <linux/input.h> > > Order please? 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 > > Use fixed width for register offset, like 0x00. We will fix this issue in patch v2. > > ... > > > +#define SPRD_DEF_LONG_KEY_MS 1000 > > (1 * MSEC_PER_SEC) > > ? Yes. > > ... > > > +struct sprd_keypad_data { > > + 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. > > > + 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; > > +}; > > ... > > > + /* > > > + * y(ms) = (x + 1) * array_size > > + * / (32.768 / (clk_div_num + 1)) > > One line. > > + * Where: > > > + * y means time in ms > > + * x means counter > > + * array_size equal to rows * columns > > + * clk_div_num is devider to keypad source clock > > + **/ > > Single asterisk is enough. We will fix this issue in patch v2. > > ... > > > + value = value / (1000 * array_size * > > value /= ... ? We will fix this issue in patch v2. > > MSEC_PER_SEC ? > > > + (SPRD_DEF_DIV_CNT + 1)); > > One line. We will fix this issue in patch v2. > > ... > > > + if (value >= 1) > > + value -= 1; > > if (value) > value--; > > ... > > > + 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; > > Move | & etc on previous lines respectively. We will fix this issue in patch v2. > > ... > > > +static int __maybe_unused sprd_keypad_suspend(struct device *dev) > > No __maybe_unused. We will fix this issue in patch v2. > > ... > > > +static int __maybe_unused sprd_keypad_resume(struct device *dev) > > Ditto. We will fix this issue in patch v2. > > ... > > > +static SIMPLE_DEV_PM_OPS(sprd_keypad_pm_ops, > > + sprd_keypad_suspend, sprd_keypad_resume); > > Use new DEFINE_*() PM macro. We will fix this issue in patch v2. > > ... > > > +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? > > ... > > > + if (data->num_rows > SPRD_KPD_ROWS_MAX > > + || data->num_cols > SPRD_KPD_COLS_MAX) { > > Move the || to the previous line. We will fix this issue in patch v2. > > > + dev_err(dev, "invalid num_rows or num_cols\n"); > > + return -EINVAL; > > + } > > ... > > > + ret = of_property_read_u32(np, "debounce-interval", &data->debounce_ms); > > device_property_...() We will fix this issue in patch v2. > > > + if (ret) { > > + data->debounce_ms = 5; > > > + dev_warn(dev, "parse debounce-interval failed.\n"); > > Why do we need this message? Prompt that debounce time is not defined. > > > + } > > ... > > > + 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; > > device_property_*() We will fix this issue in patch v2. > > And I'm wondering if input subsystem already does this for you. I don't quite understand what you mean. > > ... > > > + 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? > > > + return 0; > > +} > > ... > > > + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > > + 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. > > > + ret = PTR_ERR(data->base); > > + 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); > > Too many spaces. We will fix this issue in patch v2. > > > + goto err_free; > > 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? > > > + } > > ... > > > + ret = matrix_keypad_build_keymap(NULL, NULL, data->num_rows, > > + data->num_cols, NULL, data->input_dev); > > Reindent this to be split logically. We will fix this issue in patch v2. > > > + if (ret) { > > + dev_err(&pdev->dev, "keypad build keymap failed.\n"); > > + goto err_free; > > + } > > ... > > > + irq = platform_get_irq(pdev, 0); > > + if (irq < 0) { > > > + dev_err(&pdev->dev, "platform get irq failed.\n"); > > Dup message. > > > + goto clk_free; > > + } > > ... > > > +clk_free: > > + sprd_keypad_disable(data); > > See above how this can be avoided. This is hard to explain. > > ... > > > +err_free: > > + devm_kfree(&pdev->dev, data); > > Huh?! > > > + return ret; > > ... > > > +static const struct of_device_id sprd_keypad_match[] = { > > + { .compatible = "sprd,sc9860-keypad", }, > > + {}, > > No comma for the terminator. We will fix this issue in patch v2. > > > +}; > > ... > > > +static struct platform_driver sprd_keypad_driver = { > > + .driver = { > > + .name = "sprd-keypad", > > > + .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? > > > + .of_match_table = sprd_keypad_match, > > + .pm = &sprd_keypad_pm_ops, > > + }, > > + .probe = sprd_keypad_probe, > > + .remove = sprd_keypad_remove, > > +}; > > > + > > No need to have this blank line. We will fix this issue in patch v2. > > > +module_platform_driver(sprd_keypad_driver); > > -- > With Best Regards, > Andy Shevchenko > >