[PATCHv2 0/2] Input: mtk-pmic-keys: probe cleanups

[PATCHv2 0/2] Input: mtk-pmic-keys: probe cleanups

[Rosen Penev]

Replace an open-coded of_match_device() lookup with
of_device_get_match_data(), then rework the key-counting logic
to iterate child nodes in a single pass instead of pre-counting.

v2: split patches up and add irq checks.

Rosen Penev (2):
  Input: mtk-pmic-keys: use of_device_get_match_data()
  Input: mtk-pmic-keys: Count available keys during probe instead of
    pre-counting

 drivers/input/keyboard/mtk-pmic-keys.c | 76 ++++++++++++--------------
 1 file changed, 35 insertions(+), 41 deletions(-)

-- 
2.55.0

[PATCHv2 1/2] Input: mtk-pmic-keys: use of_device_get_match_data()

[Rosen Penev]

Instead of open coding it, use the proper function.

All entries have a corresponding data entry so a NULL check is not
applied here.

Remove of_device.h as a result.

Signed-off-by: Rosen Penev <rosenp@gmail.com>
---
 drivers/input/keyboard/mtk-pmic-keys.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/drivers/input/keyboard/mtk-pmic-keys.c b/drivers/input/keyboard/mtk-pmic-keys.c
index c78d9f6d97c4..fd684ac16938 100644
--- a/drivers/input/keyboard/mtk-pmic-keys.c
+++ b/drivers/input/keyboard/mtk-pmic-keys.c
@@ -16,7 +16,6 @@
 #include <linux/mfd/mt6397/core.h>
 #include <linux/mfd/mt6397/registers.h>
 #include <linux/module.h>
-#include <linux/of_device.h>
 #include <linux/of.h>
 #include <linux/platform_device.h>
 #include <linux/regmap.h>
@@ -333,8 +332,6 @@ static int mtk_pmic_keys_probe(struct platform_device *pdev)
 	struct mtk_pmic_keys *keys;
 	const struct mtk_pmic_regs *mtk_pmic_regs;
 	struct input_dev *input_dev;
-	const struct of_device_id *of_id =
-		of_match_device(of_mtk_pmic_keys_match_tbl, &pdev->dev);
 
 	keys = devm_kzalloc(&pdev->dev, sizeof(*keys), GFP_KERNEL);
 	if (!keys)
@@ -342,7 +339,7 @@ static int mtk_pmic_keys_probe(struct platform_device *pdev)
 
 	keys->dev = &pdev->dev;
 	keys->regmap = pmic_chip->regmap;
-	mtk_pmic_regs = of_id->data;
+	mtk_pmic_regs = of_device_get_match_data(&pdev->dev);
 
 	keys->input_dev = input_dev = devm_input_allocate_device(keys->dev);
 	if (!input_dev) {
-- 
2.55.0

Re: [PATCHv2 1/2] Input: mtk-pmic-keys: use of_device_get_match_data()

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] Mismatch between counting available child nodes and iterating over all child nodes leads to an out-of-bounds array access.
--

--- Patch [1]: [PATCHv2 1/2] Input: mtk-pmic-keys: use of_device_get_match_data() ---
commit 8b3f8694df03175d2fcd1e69a487343b4d70d629
Author: Rosen Penev <rosenp@gmail.com>

Input: mtk-pmic-keys: use of_device_get_match_data()

This commit replaces open-coded match data retrieval with
of_device_get_match_data() and removes the of_device.h include.

> diff --git a/drivers/input/keyboard/mtk-pmic-keys.c b/drivers/input/keyboard/mtk-pmic-keys.c
> index c78d9f6d97c4f..fd684ac169386 100644
> --- a/drivers/input/keyboard/mtk-pmic-keys.c
> +++ b/drivers/input/keyboard/mtk-pmic-keys.c
[ ... ]
> @@ -333,8 +332,6 @@ static int mtk_pmic_keys_probe(struct platform_device *pdev)
>  	struct mtk_pmic_keys *keys;
>  	const struct mtk_pmic_regs *mtk_pmic_regs;
>  	struct input_dev *input_dev;
> -	const struct of_device_id *of_id =
> -		of_match_device(of_mtk_pmic_keys_match_tbl, &pdev->dev);
>  
>  	keys = devm_kzalloc(&pdev->dev, sizeof(*keys), GFP_KERNEL);
>  	if (!keys)
> @@ -342,7 +339,7 @@ static int mtk_pmic_keys_probe(struct platform_device *pdev)
>  
>  	keys->dev = &pdev->dev;
>  	keys->regmap = pmic_chip->regmap;
> -	mtk_pmic_regs = of_id->data;
> +	mtk_pmic_regs = of_device_get_match_data(&pdev->dev);
>  
>  	keys->input_dev = input_dev = devm_input_allocate_device(keys->dev);

[Severity: High]
This is a pre-existing issue, but while reviewing mtk_pmic_keys_probe(),
I noticed a potential out-of-bounds array access further down in the
function.

The function correctly limits the expected number of keys by checking
the count of available child nodes:

drivers/input/keyboard/mtk-pmic-keys.c:mtk_pmic_keys_probe() {
    ...
    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;
    }
    ...
}

However, it then iterates over all child nodes, not just the available ones:

drivers/input/keyboard/mtk-pmic-keys.c:mtk_pmic_keys_probe() {
    ...
    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 a device tree provides more child nodes than MTK_PMIC_MAX_KEY_COUNT
but only a subset are marked as available (for example, with
status = "okay"), the bounds check will pass. Because the loop iterates over
all child nodes regardless of their availability status, could index exceed
the array limits?

Would it be safer to use for_each_available_child_of_node_scoped() here to
ensure the loop matches the earlier available child count check and prevents
an out-of-bounds read on irqnames[] and write to keys->keys[]?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260728212438.54078-2-rosenp@gmail.com?part=1

Re: [PATCHv2 1/2] Input: mtk-pmic-keys: use of_device_get_match_data()

[Chen-Yu Tsai]

On Wed, Jul 29, 2026 at 5:24 AM Rosen Penev <rosenp@gmail.com> wrote:
>
> Instead of open coding it, use the proper function.
>
> All entries have a corresponding data entry so a NULL check is not
> applied here.
>
> Remove of_device.h as a result.
>
> Signed-off-by: Rosen Penev <rosenp@gmail.com>

Reviewed-by: Chen-Yu Tsai <wenst@chromium.org>

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

[Rosen Penev]

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

Assisted-by: OpenCode:BigPickle
Signed-off-by: Rosen Penev <rosenp@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)) {
+			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

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

[Chen-Yu Tsai]

On Wed, Jul 29, 2026 at 5:25 AM Rosen Penev <rosenp@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@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;

Or better yet, just use for_each_available_child_of_node_scoped() { ... }
instead.

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

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@chromium.org> wrote:
>
> On Wed, Jul 29, 2026 at 5:25 AM Rosen Penev <rosenp@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@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
> >
> >