| Message ID | 20220127111526.3716689-1-mkorpershoek@baylibre.com |
|---|---|
| Headers | show |
| Series | Add matrix keypad driver support for Mediatek SoCs | expand |
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. ... > +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. > + .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. ... > + 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... > + if (error) { > + dev_err(&pdev->dev, "Failed to request IRQ#%d:%d\n", > + irq, error); ...and here. > + return error; > + }
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
On Fri, Jan 28, 2022 at 11:03:08AM +0100, Mattijs Korpershoek wrote: > 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: ... > > Up to you. > Thank you for your review and your suggestions. Yes, as I said it you need a new version and consider them good. Thanks!