[PATCHv2 2/2] Input: mtk-pmic-keys: Count available keys during probe instead of pre-counting
Rosen Penev
rosenp at gmail.com
Wed Jul 29 11:34:17 PDT 2026
On Tue, Jul 28, 2026 at 8:44 PM Chen-Yu Tsai <wenst at chromium.org> wrote:
>
> On Wed, Jul 29, 2026 at 5:25 AM Rosen Penev <rosenp at gmail.com> wrote:
> >
> > Replace the separate of_get_available_child_count() pre-count and
> > validation step with a single pass through
> > for_each_child_of_node_scoped(). Skip unavailable child nodes and
> > bail if more than MTK_PMIC_MAX_KEY_COUNT available keys are found.
>
> This actually fixes a small bug. The pre-count only gets the number
> of available keys, but the subsequent for_each_child block doesn't
> skip over the unavailable ones, so if an unavailable node is in
> the middle of the tree, there could end up being a mismatch.
>
> > Set nkeys after the loop so suspend/resume iterate only over
> > initialized entries. Use a local key variable in the loop for
> > clarity.
> >
> > Add an irq > 0 guard to the suspend/resume wakeup paths so that
> > uninitialized key entries are safely skipped.
>
> You are trying to do too much in one patch. These other fixes should be
> in separate patches.
>
> And why not just modify the driver to keep "nkeys", so that it knows
> how many entries are valid? No use in iterating over empty entries.
>
>
> > Assisted-by: OpenCode:BigPickle
> > Signed-off-by: Rosen Penev <rosenp at gmail.com>
> > ---
> > drivers/input/keyboard/mtk-pmic-keys.c | 71 ++++++++++++--------------
> > 1 file changed, 34 insertions(+), 37 deletions(-)
> >
> > diff --git a/drivers/input/keyboard/mtk-pmic-keys.c b/drivers/input/keyboard/mtk-pmic-keys.c
> > index fd684ac16938..e2ced6e5165a 100644
> > --- a/drivers/input/keyboard/mtk-pmic-keys.c
> > +++ b/drivers/input/keyboard/mtk-pmic-keys.c
> > @@ -267,7 +267,7 @@ static int mtk_pmic_keys_suspend(struct device *dev)
> > int index;
> >
> > for (index = 0; index < MTK_PMIC_MAX_KEY_COUNT; index++) {
> > - if (keys->keys[index].wakeup) {
> > + if (keys->keys[index].irq > 0 && keys->keys[index].wakeup) {
> > enable_irq_wake(keys->keys[index].irq);
> > if (keys->keys[index].irq_r > 0)
> > enable_irq_wake(keys->keys[index].irq_r);
> > @@ -283,7 +283,7 @@ static int mtk_pmic_keys_resume(struct device *dev)
> > int index;
> >
> > for (index = 0; index < MTK_PMIC_MAX_KEY_COUNT; index++) {
> > - if (keys->keys[index].wakeup) {
> > + if (keys->keys[index].irq > 0 && keys->keys[index].wakeup) {
> > disable_irq_wake(keys->keys[index].irq);
> > if (keys->keys[index].irq_r > 0)
> > disable_irq_wake(keys->keys[index].irq_r);
> > @@ -324,13 +324,13 @@ MODULE_DEVICE_TABLE(of, of_mtk_pmic_keys_match_tbl);
> > static int mtk_pmic_keys_probe(struct platform_device *pdev)
> > {
> > int error, index = 0;
> > - unsigned int keycount;
> > struct mt6397_chip *pmic_chip = dev_get_drvdata(pdev->dev.parent);
> > struct device_node *node = pdev->dev.of_node;
> > static const char *const irqnames[] = { "powerkey", "homekey" };
> > static const char *const irqnames_r[] = { "powerkey_r", "homekey_r" };
> > struct mtk_pmic_keys *keys;
> > const struct mtk_pmic_regs *mtk_pmic_regs;
> > + struct mtk_pmic_keys_info *key;
> > struct input_dev *input_dev;
> >
> > keys = devm_kzalloc(&pdev->dev, sizeof(*keys), GFP_KERNEL);
> > @@ -353,45 +353,42 @@ static int mtk_pmic_keys_probe(struct platform_device *pdev)
> > input_dev->id.product = 0x0001;
> > input_dev->id.version = 0x0001;
> >
> > - keycount = of_get_available_child_count(node);
> > - if (keycount > MTK_PMIC_MAX_KEY_COUNT ||
> > - keycount > ARRAY_SIZE(irqnames)) {
> > - dev_err(keys->dev, "too many keys defined (%d)\n", keycount);
> > - return -EINVAL;
> > - }
> > -
> > for_each_child_of_node_scoped(node, child) {
> > - keys->keys[index].regs = &mtk_pmic_regs->keys_regs[index];
> > -
> > - keys->keys[index].irq =
> > - platform_get_irq_byname(pdev, irqnames[index]);
> > - if (keys->keys[index].irq < 0)
> > - return keys->keys[index].irq;
> > -
> > - if (mtk_pmic_regs->key_release_irq) {
> > - keys->keys[index].irq_r = platform_get_irq_byname(pdev,
> > - irqnames_r[index]);
> > -
> > - if (keys->keys[index].irq_r < 0)
> > - return keys->keys[index].irq_r;
> > + if (index >= MTK_PMIC_MAX_KEY_COUNT) {
> > + dev_err(&pdev->dev, "too many keys defined\n");
> > + return -EINVAL;
> > }
> >
> > - error = of_property_read_u32(child,
> > - "linux,keycodes", &keys->keys[index].keycode);
> > - if (error) {
> > - dev_err(keys->dev,
> > - "failed to read key:%d linux,keycode property: %d\n",
> > - index, error);
> > - return error;
> > + if (of_device_is_available(child)) {
>
> Instead of re-indenting the whole block and making the diff huge and
> unreadable, please make this skip over the remaining code when the
> condition fails, i.e.:
>
> if (!of_device_is_available(child))
> continue;
Yeah probably better.
>
> Or better yet, just use for_each_available_child_of_node_scoped() { ... }
> instead.
In v1 this is exactly what I did and was told not to.
>
> I normally suggest people read the API docs (either on docs.kernel.org
> or the kernel-doc sections in the code) to find better suited constructs
> to use.
>
>
> Thanks
> ChenYu
>
> > + key = &keys->keys[index];
> > + key->regs = &mtk_pmic_regs->keys_regs[index];
> > +
> > + key->irq = platform_get_irq_byname(pdev, irqnames[index]);
> > + if (key->irq < 0)
> > + return key->irq;
> > +
> > + if (mtk_pmic_regs->key_release_irq) {
> > + key->irq_r = platform_get_irq_byname(pdev, irqnames_r[index]);
> > + if (key->irq_r < 0)
> > + return key->irq_r;
> > + }
> > +
> > + error = of_property_read_u32(child, "linux,keycodes", &key->keycode);
> > + if (error) {
> > + dev_err(keys->dev,
> > + "failed to read key:%d linux,keycode property: %d\n",
> > + index, error);
> > + return error;
> > + }
> > +
> > + if (of_property_present(child, "wakeup-source"))
> > + key->wakeup = true;
> > +
> > + error = mtk_pmic_key_setup(keys, key);
> > + if (error)
> > + return error;
> > }
> >
> > - if (of_property_read_bool(child, "wakeup-source"))
> > - keys->keys[index].wakeup = true;
> > -
> > - error = mtk_pmic_key_setup(keys, &keys->keys[index]);
> > - if (error)
> > - return error;
> > -
> > index++;
> > }
> >
> > --
> > 2.55.0
> >
> >
More information about the Linux-mediatek
mailing list