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
>
>
