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

On Thu, Jul 30, 2026 at 2:34 AM Rosen Penev <[email protected]> wrote:
>
> On Tue, Jul 28, 2026 at 8:44 PM Chen-Yu Tsai <[email protected]> wrote:
> >
> > On Wed, Jul 29, 2026 at 5:25 AM Rosen Penev <[email protected]> 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 <[email protected]>
> > > ---
> > >  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 see now that the interrupts are tied to each key in order, regardless
whether the key is enabled or not. for_each_available_child_of_node_scoped()
is not the right thing to use indeed.

Could you add this bit of context to the commit message?


Thanks
ChenYu

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