Re: [PATCHv2 2/2] Input: mtk-pmic-keys: Count available keys during probe instead of pre-counting

[Rosen Penev]

On Tue, Jul 28, 2026 at 8:44 PM Chen-Yu Tsai <wenst@xxxxxxxxxxxx> wrote:
>
> On Wed, Jul 29, 2026 at 5:25 AM Rosen Penev <rosenp@xxxxxxxxx> 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@xxxxxxxxx>
> > ---
> >  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
> >
> >