[PATCHv2 2/2] Input: mtk-pmic-keys: Count available keys during probe instead of pre-counting
Chen-Yu Tsai
wenst at chromium.org
Wed Jul 29 21:53:28 PDT 2026
On Thu, Jul 30, 2026 at 2:34 AM Rosen Penev <rosenp at gmail.com> wrote:
>
> 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 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
> > >
> > >
More information about the Linux-mediatek
mailing list