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

[wenhua lin]

On Thu, Aug 10, 2023 at 10:01 PM Andy Shevchenko
<andriy.shevchenko@xxxxxxxxxxxxxxx> 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
> > <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.

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