Messages in this thread | | | From | Mattijs Korpershoek <> | Subject | Re: [PATCH v20 2/3] Input: mt6779-keypad - Add MediaTek keypad driver | Date | Fri, 28 Jan 2022 11:03:08 +0100 |
| |
On Thu, Jan 27, 2022 at 17:20, Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:
> On Thu, Jan 27, 2022 at 12:15:25PM +0100, Mattijs Korpershoek wrote: >> From: "fengping.yu" <fengping.yu@mediatek.com> >> >> This patch adds matrix keypad support for Mediatek SoCs. > > Some comments which may be addressed now or in the follow-up patch(es). > Up to you. Hi Andy, Thank you for your review and your suggestions.
> > ... > >> +static const struct regmap_config mt6779_keypad_regmap_cfg = { >> + .reg_bits = 32, >> + .val_bits = 32, > >> + .reg_stride = sizeof(u32), > > I'm wondering if we need this when we have reg_bits = 32 already.
Per my understanding, .reg_stride is mainly used to check for invalid register addresses in regmap_{read,write}():
if (!IS_ALIGNED(reg, map->reg_stride)) return -EINVAL;
If .reg_stride is not set, regmap core will default it to 1. It's not computed from reg_bits.
So I think we still need it. > >> + .max_register = 36, >> +}; > > ... > >> + regmap_write(keypad->regmap, MTK_KPD_DEBOUNCE, >> + (debounce * 32) & MTK_KPD_DEBOUNCE_MASK); > > I'm wondering if << 5 is more specific to show that the value > is based on 2^5 units.
The datasheet I've seen states: "De-bounce time = KP_DEBOUNCE / 32ms" But rewriting it as 1 << 5 seems reasonable as well: regmap_write(keypad->regmap, MTK_KPD_DEBOUNCE, (debounce * (1 << 5)) & MTK_KPD_DEBOUNCE_MASK);
I don't have any preference on this one. If I have to send a v21, I will rewrite it using (1 << 5)
> > ... > >> + error = devm_add_action_or_reset(&pdev->dev, mt6779_keypad_clk_disable, keypad->clk); > > You have this long line... > >> + error = devm_request_threaded_irq(&pdev->dev, irq, >> + NULL, mt6779_keypad_irq_handler, >> + IRQF_ONESHOT, >> + MTK_KPD_NAME, keypad); > > ...at the same time you may reduce LOCs here... Ack. will join lines to reduce LOCs if I have to send v21. > >> + if (error) { >> + dev_err(&pdev->dev, "Failed to request IRQ#%d:%d\n", >> + irq, error); > > ...and here. Ack. will join lines to reduce LOCs if I have to send v21.
> >> + return error; >> + } > > -- > With Best Regards, > Andy Shevchenko
| |