Re: [PATCH] input: keyboard: Add sprd-keypad driver
From: Andy Shevchenko
Date: Thu Aug 10 2023 - 10:01:17 EST
On Thu, Aug 10, 2023 at 08:42:36PM +0800, wenhua lin wrote:
> 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
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