With the call to of_irq_count() removed, the SiFive GPIO driver can be
built as a module. This helps to minimize the size of a multiplatform
kernel, and is required by some downstream distributions (Android GKI).
This series removes the rest of the of_* API usage in the process.
Changes in v2:
- Add 3 new patches removing of_* API usage
- Add MODULE_AUTHOR and MODULE_DESCRIPTION
Samuel Holland (4):
gpio: sifive: Directly use the device's fwnode
gpio: sifive: Look up IRQs only once during probe
gpio: sifive: Get the parent IRQ's domain from its irq_data
gpio: sifive: Allow building the driver as a module
drivers/gpio/Kconfig | 2 +-
drivers/gpio/gpio-sifive.c | 45 +++++++++++++-------------------------
2 files changed, 16 insertions(+), 31 deletions(-)
--
2.40.1
Do not parse the devicetree again when the data is already available
from the IRQ subsystem. This follows the example of the ThunderX and
X-Gene GPIO drivers. The ngpio check is needed to avoid a possible
out-of-bounds read.
Signed-off-by: Samuel Holland <[email protected]>
---
Changes in v2:
- New patch for v2
drivers/gpio/gpio-sifive.c | 22 +++++-----------------
1 file changed, 5 insertions(+), 17 deletions(-)
diff --git a/drivers/gpio/gpio-sifive.c b/drivers/gpio/gpio-sifive.c
index 6606c919d957..46a42109d6f5 100644
--- a/drivers/gpio/gpio-sifive.c
+++ b/drivers/gpio/gpio-sifive.c
@@ -6,7 +6,6 @@
#include <linux/bitops.h>
#include <linux/device.h>
#include <linux/errno.h>
-#include <linux/of_irq.h>
#include <linux/gpio/driver.h>
#include <linux/init.h>
#include <linux/platform_device.h>
@@ -180,9 +179,6 @@ static const struct regmap_config sifive_gpio_regmap_config = {
static int sifive_gpio_probe(struct platform_device *pdev)
{
struct device *dev = &pdev->dev;
- struct device_node *node = pdev->dev.of_node;
- struct device_node *irq_parent;
- struct irq_domain *parent;
struct gpio_irq_chip *girq;
struct sifive_gpio *chip;
int ret, ngpio;
@@ -202,24 +198,16 @@ static int sifive_gpio_probe(struct platform_device *pdev)
if (IS_ERR(chip->regs))
return PTR_ERR(chip->regs);
- irq_parent = of_irq_find_parent(node);
- if (!irq_parent) {
- dev_err(dev, "no IRQ parent node\n");
- return -ENODEV;
- }
- parent = irq_find_host(irq_parent);
- of_node_put(irq_parent);
- if (!parent) {
- dev_err(dev, "no IRQ parent domain\n");
- return -ENODEV;
- }
-
for (ngpio = 0; ngpio < SIFIVE_GPIO_MAX; ngpio++) {
ret = platform_get_irq_optional(pdev, ngpio);
if (ret < 0)
break;
chip->irq_number[ngpio] = ret;
}
+ if (!ngpio) {
+ dev_err(dev, "no IRQ found\n");
+ return -ENODEV;
+ }
ret = bgpio_init(&chip->gc, dev, 4,
chip->base + SIFIVE_GPIO_INPUT_VAL,
@@ -248,7 +236,7 @@ static int sifive_gpio_probe(struct platform_device *pdev)
girq = &chip->gc.irq;
gpio_irq_chip_set_chip(girq, &sifive_gpio_irqchip);
girq->fwnode = dev->fwnode;
- girq->parent_domain = parent;
+ girq->parent_domain = irq_get_irq_data(chip->irq_number[0])->domain;
girq->child_to_parent_hwirq = sifive_gpio_child_to_parent_hwirq;
girq->handler = handle_bad_irq;
girq->default_type = IRQ_TYPE_NONE;
--
2.40.1
There is no need to convert dev->of_node back to a fwnode_handle.
Signed-off-by: Samuel Holland <[email protected]>
---
Changes in v2:
- New patch for v2
drivers/gpio/gpio-sifive.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpio/gpio-sifive.c b/drivers/gpio/gpio-sifive.c
index 745e5f67254e..ab32c952c61b 100644
--- a/drivers/gpio/gpio-sifive.c
+++ b/drivers/gpio/gpio-sifive.c
@@ -254,7 +254,7 @@ static int sifive_gpio_probe(struct platform_device *pdev)
chip->gc.owner = THIS_MODULE;
girq = &chip->gc.irq;
gpio_irq_chip_set_chip(girq, &sifive_gpio_irqchip);
- girq->fwnode = of_node_to_fwnode(node);
+ girq->fwnode = dev->fwnode;
girq->parent_domain = parent;
girq->child_to_parent_hwirq = sifive_gpio_child_to_parent_hwirq;
girq->handler = handle_bad_irq;
--
2.40.1
On Wed, Jul 19, 2023 at 09:34:42AM -0700, Samuel Holland wrote:
> There is no need to convert dev->of_node back to a fwnode_handle.
...
> - girq->fwnode = of_node_to_fwnode(node);
> + girq->fwnode = dev->fwnode;
dev_fwnode(dev)
...and include property.h here
--
With Best Regards,
Andy Shevchenko
On 2023-07-19 11:54 AM, Andy Shevchenko wrote:
> On Wed, Jul 19, 2023 at 09:34:44AM -0700, Samuel Holland wrote:
>> Do not parse the devicetree again when the data is already available
>> from the IRQ subsystem. This follows the example of the ThunderX and
>> X-Gene GPIO drivers. The ngpio check is needed to avoid a possible
>> out-of-bounds read.
>
> ...
>
>> - girq->parent_domain = parent;
>> + girq->parent_domain = irq_get_irq_data(chip->irq_number[0])->domain;
>
> For the sake of readability I would like to leave parent variable
> and assign it beforehand somewhere upper in the code.
OK.
> Also, can irq_get_irq_data() return NULL? Needs a comment on top
> of that assignment or an additional check.
No, the earlier loop already verified the IRQ number was valid. I don't think it
can later become invalid. In any case, we already dereference the result of
irq_get_irq_data(irq_number[foo]) in sifive_gpio_child_to_parent_hwirq().
On Wed, Jul 19, 2023 at 09:34:44AM -0700, Samuel Holland wrote:
> Do not parse the devicetree again when the data is already available
> from the IRQ subsystem. This follows the example of the ThunderX and
> X-Gene GPIO drivers. The ngpio check is needed to avoid a possible
> out-of-bounds read.
...
> - girq->parent_domain = parent;
> + girq->parent_domain = irq_get_irq_data(chip->irq_number[0])->domain;
For the sake of readability I would like to leave parent variable
and assign it beforehand somewhere upper in the code.
Also, can irq_get_irq_data() return NULL? Needs a comment on top
of that assignment or an additional check.
--
With Best Regards,
Andy Shevchenko
This can reduce the kernel image size in multiplatform configurations.
Acked-by: Palmer Dabbelt <[email protected]>
Signed-off-by: Samuel Holland <[email protected]>
---
Changes in v2:
- Add MODULE_AUTHOR and MODULE_DESCRIPTION
drivers/gpio/Kconfig | 2 +-
drivers/gpio/gpio-sifive.c | 6 +++++-
2 files changed, 6 insertions(+), 2 deletions(-)
diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
index e382dfebad7c..1a8e8a8c85d6 100644
--- a/drivers/gpio/Kconfig
+++ b/drivers/gpio/Kconfig
@@ -564,7 +564,7 @@ config GPIO_SAMA5D2_PIOBU
maintain their value during backup/self-refresh.
config GPIO_SIFIVE
- bool "SiFive GPIO support"
+ tristate "SiFive GPIO support"
depends on OF_GPIO
select IRQ_DOMAIN_HIERARCHY
select GPIO_GENERIC
diff --git a/drivers/gpio/gpio-sifive.c b/drivers/gpio/gpio-sifive.c
index 46a42109d6f5..eacd67982de0 100644
--- a/drivers/gpio/gpio-sifive.c
+++ b/drivers/gpio/gpio-sifive.c
@@ -258,4 +258,8 @@ static struct platform_driver sifive_gpio_driver = {
.of_match_table = sifive_gpio_match,
},
};
-builtin_platform_driver(sifive_gpio_driver)
+module_platform_driver(sifive_gpio_driver)
+
+MODULE_AUTHOR("Yash Shah <[email protected]>");
+MODULE_DESCRIPTION("SiFive GPIO driver");
+MODULE_LICENSE("GPL");
--
2.40.1
of_irq_count(), or eqivalently platform_irq_count(), simply looks up
successively-numbered IRQs until that fails. Since this driver needs to
look up each IRQ anyway to get its virq number, use that existing loop
to count the IRQs at the same time.
Signed-off-by: Samuel Holland <[email protected]>
---
Changes in v2:
- New patch for v2
drivers/gpio/gpio-sifive.c | 17 +++++------------
1 file changed, 5 insertions(+), 12 deletions(-)
diff --git a/drivers/gpio/gpio-sifive.c b/drivers/gpio/gpio-sifive.c
index ab32c952c61b..6606c919d957 100644
--- a/drivers/gpio/gpio-sifive.c
+++ b/drivers/gpio/gpio-sifive.c
@@ -185,7 +185,7 @@ static int sifive_gpio_probe(struct platform_device *pdev)
struct irq_domain *parent;
struct gpio_irq_chip *girq;
struct sifive_gpio *chip;
- int ret, ngpio, i;
+ int ret, ngpio;
chip = devm_kzalloc(dev, sizeof(*chip), GFP_KERNEL);
if (!chip)
@@ -202,13 +202,6 @@ static int sifive_gpio_probe(struct platform_device *pdev)
if (IS_ERR(chip->regs))
return PTR_ERR(chip->regs);
- ngpio = of_irq_count(node);
- if (ngpio > SIFIVE_GPIO_MAX) {
- dev_err(dev, "Too many GPIO interrupts (max=%d)\n",
- SIFIVE_GPIO_MAX);
- return -ENXIO;
- }
-
irq_parent = of_irq_find_parent(node);
if (!irq_parent) {
dev_err(dev, "no IRQ parent node\n");
@@ -221,11 +214,11 @@ static int sifive_gpio_probe(struct platform_device *pdev)
return -ENODEV;
}
- for (i = 0; i < ngpio; i++) {
- ret = platform_get_irq(pdev, i);
+ for (ngpio = 0; ngpio < SIFIVE_GPIO_MAX; ngpio++) {
+ ret = platform_get_irq_optional(pdev, ngpio);
if (ret < 0)
- return ret;
- chip->irq_number[i] = ret;
+ break;
+ chip->irq_number[ngpio] = ret;
}
ret = bgpio_init(&chip->gc, dev, 4,
--
2.40.1
On Wed, Jul 19, 2023 at 12:03:46PM -0500, Samuel Holland wrote:
> On 2023-07-19 11:54 AM, Andy Shevchenko wrote:
> > On Wed, Jul 19, 2023 at 09:34:44AM -0700, Samuel Holland wrote:
...
> > Also, can irq_get_irq_data() return NULL? Needs a comment on top
> > of that assignment or an additional check.
>
> No, the earlier loop already verified the IRQ number was valid. I don't think it
> can later become invalid. In any case, we already dereference the result of
> irq_get_irq_data(irq_number[foo]) in sifive_gpio_child_to_parent_hwirq().
Thanks for explanation, just add a comment.
--
With Best Regards,
Andy Shevchenko