Re: [PATCH] input: keyboard: Add sprd-keypad driver

[wenhua lin]

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