pinctrl: at91: Cleanups

PATCH In review Andy Shevchenko 3 participants 10 messages Feb 15, 2023 – Feb 27, 2023

Subsystems (4)

GPIO Arm/Microchip (AT91) SoC support Pin control Pin controller - Microchip AT91

lore

Share & tools

lore

Andy Shevchenko

author

2023-02-15 13:42 UTC #0 lore raw

v3 cover pinctrl: at91: Cleanups

The kasprintf() patch makes me look into the driver code and besides missed fix, there is a room to improve. Hence this series.

Compile tested only.

Changes in v3:
- added tags (Claudiu)
- dropped unrelated curly bracket change (Claudiu)

Changes in v2:
- fixed compilation errors (LKP)

Andy Shevchenko (5):
pinctrl: at91: use devm_kasprintf() to avoid potential leaks (part 2)
pinctrl: at91: Don't mix non-devm calls with devm ones
pinctrl: at91: Use of_device_get_match_data()
pinctrl: at91: Use dev_err_probe() instead of custom messaging
pinctrl: at91: Utilise temporary variable for struct device

 drivers/pinctrl/pinctrl-at91.c | 161 ++++++++++++++-------------------
 1 file changed, 66 insertions(+), 95 deletions(-)

Signature · 1 line

2.39.1

Andy Shevchenko

author

2023-02-15 13:42 UTC #1 lore raw

PATCH v3 1/5 pinctrl: at91: use devm_kasprintf() to avoid potential leaks (part 2)

Use devm_kasprintf() instead of kasprintf() to avoid any potential leaks. At the moment drivers have no remove functionality hence there is no need for fixes tag.

While at it, switch to use devm_kasprintf_strarray().

Signed-off-by Andy Shevchenko <[email protected]>

Reviewed-by Claudiu Beznea <[email protected]>

Tested-by Claudiu Beznea <[email protected]>

---
 drivers/pinctrl/pinctrl-at91.c | 23 +++++++++++++++++------
 1 file changed, 17 insertions(+), 6 deletions(-)

drivers/pinctrl/pinctrl-at91.c +17 -6

index 2e7debb905d5..5c01765c7a2a 100644
@@ -18,6 +18,7 @@
 #include <linux/pm.h>
 #include <linux/seq_file.h>
 #include <linux/slab.h>
+#include <linux/string_helpers.h>
 
 /* Since we request GPIOs from ourself */
 #include <linux/pinctrl/consumer.h>

@@ -1371,6 +1372,7 @@ static int at91_pinctrl_probe_dt(struct platform_device *pdev,
 
 static int at91_pinctrl_probe(struct platform_device *pdev)
 {
+	struct device *dev = &pdev->dev;
 	struct at91_pinctrl *info;
 	struct pinctrl_pin_desc *pdesc;
 	int ret, i, j, k;

@@ -1394,9 +1396,19 @@ static int at91_pinctrl_probe(struct platform_device *pdev)
 		return -ENOMEM;
 
 	for (i = 0, k = 0; i < gpio_banks; i++) {
+		char **names;
+
+		names = devm_kasprintf_strarray(dev, "pio", MAX_NB_GPIO_PER_BANK);
+		if (!names)
+			return -ENOMEM;
+
 		for (j = 0; j < MAX_NB_GPIO_PER_BANK; j++, k++) {
+			char *name = names[j];
+
+			strreplace(name, '-', i + 'A');
+
 			pdesc->number = k;
-			pdesc->name = kasprintf(GFP_KERNEL, "pio%c%d", i + 'A', j);
+			pdesc->name = name;
 			pdesc++;
 		}
 	}

@@ -1797,7 +1809,8 @@ static const struct of_device_id at91_gpio_of_match[] = {
 
 static int at91_gpio_probe(struct platform_device *pdev)
 {
-	struct device_node *np = pdev->dev.of_node;
+	struct device *dev = &pdev->dev;
+	struct device_node *np = dev->of_node;
 	struct at91_gpio_chip *at91_chip = NULL;
 	struct gpio_chip *chip;
 	struct pinctrl_gpio_range *range;

@@ -1866,16 +1879,14 @@ static int at91_gpio_probe(struct platform_device *pdev)
 			chip->ngpio = ngpio;
 	}
 
-	names = devm_kcalloc(&pdev->dev, chip->ngpio, sizeof(char *),
-			     GFP_KERNEL);
-
+	names = devm_kasprintf_strarray(dev, "pio", chip->ngpio);
 	if (!names) {
 		ret = -ENOMEM;
 		goto clk_enable_err;
 	}
 
 	for (i = 0; i < chip->ngpio; i++)
-		names[i] = devm_kasprintf(&pdev->dev, GFP_KERNEL, "pio%c%d", alias_idx + 'A', i);
+		strreplace(names[i], '-', alias_idx + 'A');
 
 	chip->names = (const char *const *)names;

Signature · 1 line

2.39.1

Andy Shevchenko

author

2023-02-15 13:42 UTC #2 lore raw

PATCH v3 2/5 pinctrl: at91: Don't mix non-devm calls with devm ones

In reply to Andy Shevchenko

Replace devm_clk_get() by devm_clk_get_enabled() and drop unneeded code pieces. This will make sure we keep the ordering of the resource allocation correct.

Signed-off-by Andy Shevchenko <[email protected]>

Reviewed-by Claudiu Beznea <[email protected]>

Tested-by Claudiu Beznea <[email protected]>

---
 drivers/pinctrl/pinctrl-at91.c | 12 ++----------
 1 file changed, 2 insertions(+), 10 deletions(-)

drivers/pinctrl/pinctrl-at91.c +2 ~~-10~~

index 5c01765c7a2a..6d9015ed8a3b 100644
@@ -1849,19 +1849,13 @@ static int at91_gpio_probe(struct platform_device *pdev)
 	at91_chip->pioc_virq = irq;
 	at91_chip->pioc_idx = alias_idx;
 
-	at91_chip->clock = devm_clk_get(&pdev->dev, NULL);
+	at91_chip->clock = devm_clk_get_enabled(&pdev->dev, NULL);
 	if (IS_ERR(at91_chip->clock)) {
 		dev_err(&pdev->dev, "failed to get clock, ignoring.\n");
 		ret = PTR_ERR(at91_chip->clock);
 		goto err;
 	}
 
-	ret = clk_prepare_enable(at91_chip->clock);
-	if (ret) {
-		dev_err(&pdev->dev, "failed to prepare and enable clock, ignoring.\n");
-		goto clk_enable_err;
-	}
-
 	at91_chip->chip = at91_gpio_template;
 	at91_chip->id = alias_idx;

@@ -1882,7 +1876,7 @@ static int at91_gpio_probe(struct platform_device *pdev)
 	names = devm_kasprintf_strarray(dev, "pio", chip->ngpio);
 	if (!names) {
 		ret = -ENOMEM;
-		goto clk_enable_err;
+		goto err;
 	}
 
 	for (i = 0; i < chip->ngpio; i++)

@@ -1915,8 +1909,6 @@ static int at91_gpio_probe(struct platform_device *pdev)
 	return 0;
 
 gpiochip_add_err:
-clk_enable_err:
-	clk_disable_unprepare(at91_chip->clock);
 err:
 	dev_err(&pdev->dev, "Failure %i for GPIO %i\n", ret, alias_idx);

Signature · 1 line

2.39.1

Andy Shevchenko

author

2023-02-15 13:42 UTC #3 lore raw

PATCH v3 3/5 pinctrl: at91: Use of_device_get_match_data()

In reply to Andy Shevchenko

Use of_device_get_match_data() to simplify the code.

Signed-off-by Andy Shevchenko <[email protected]>

Reviewed-by Claudiu Beznea <[email protected]>

Tested-by Claudiu Beznea <[email protected]>

---
 drivers/pinctrl/pinctrl-at91.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

drivers/pinctrl/pinctrl-at91.c +2 -4

index 6d9015ed8a3b..055a88b2dacc 100644
@@ -1304,8 +1304,7 @@ static int at91_pinctrl_probe_dt(struct platform_device *pdev,
 		return -ENODEV;
 
 	info->dev = &pdev->dev;
-	info->ops = (const struct at91_pinctrl_mux_ops *)
-		of_match_device(at91_pinctrl_of_match, &pdev->dev)->data;
+	info->ops = of_device_get_match_data(dev);
 	at91_pinctrl_child_count(info, np);
 
 	/*

@@ -1844,8 +1843,7 @@ static int at91_gpio_probe(struct platform_device *pdev)
 		goto err;
 	}
 
-	at91_chip->ops = (const struct at91_pinctrl_mux_ops *)
-		of_match_device(at91_gpio_of_match, &pdev->dev)->data;
+	at91_chip->ops = of_device_get_match_data(dev);
 	at91_chip->pioc_virq = irq;
 	at91_chip->pioc_idx = alias_idx;

Signature · 1 line

2.39.1

Andy Shevchenko

author

2023-02-15 13:42 UTC #4 lore raw

PATCH v3 4/5 pinctrl: at91: Use dev_err_probe() instead of custom messaging

In reply to Andy Shevchenko

The custom message has no value except printing the error code, the same does dev_err_probe(). Let's use the latter for the sake of unification.

Note that some APIs already have messaging in them and some simply do not require the current noise.

Signed-off-by Andy Shevchenko <[email protected]>

Reviewed-by Claudiu Beznea <[email protected]>

Tested-by Claudiu Beznea <[email protected]>

---
 drivers/pinctrl/pinctrl-at91.c | 64 +++++++++++-----------------------
 1 file changed, 21 insertions(+), 43 deletions(-)

drivers/pinctrl/pinctrl-at91.c +21 ~~-43~~

index 055a88b2dacc..08f88403affb 100644
@@ -1294,10 +1294,11 @@ static const struct of_device_id at91_pinctrl_of_match[] = {
 static int at91_pinctrl_probe_dt(struct platform_device *pdev,
 				 struct at91_pinctrl *info)
 {
+	struct device *dev = &pdev->dev;
 	int ret = 0;
 	int i, j, ngpio_chips_enabled = 0;
 	uint32_t *tmp;
-	struct device_node *np = pdev->dev.of_node;
+	struct device_node *np = dev->of_node;
 	struct device_node *child;
 
 	if (!np)

@@ -1360,9 +1361,8 @@ static int at91_pinctrl_probe_dt(struct platform_device *pdev,
 			continue;
 		ret = at91_pinctrl_parse_functions(child, info, i++);
 		if (ret) {
-			dev_err(&pdev->dev, "failed to parse function\n");
 			of_node_put(child);
-			return ret;
+			return dev_err_probe(dev, ret, "failed to parse function\n");
 		}
 	}

@@ -1415,11 +1415,8 @@ static int at91_pinctrl_probe(struct platform_device *pdev)
 	platform_set_drvdata(pdev, info);
 	info->pctl = devm_pinctrl_register(&pdev->dev, &at91_pinctrl_desc,
 					   info);
-
-	if (IS_ERR(info->pctl)) {
-		dev_err(&pdev->dev, "could not register AT91 pinctrl driver\n");
-		return PTR_ERR(info->pctl);
-	}
+	if (IS_ERR(info->pctl))
+		return dev_err_probe(dev, PTR_ERR(info->pctl), "could not register AT91 pinctrl driver\n");
 
 	/* We will handle a range of GPIO pins */
 	for (i = 0; i < gpio_banks; i++)

@@ -1820,39 +1817,28 @@ static int at91_gpio_probe(struct platform_device *pdev)
 	char **names;
 
 	BUG_ON(alias_idx >= ARRAY_SIZE(gpio_chips));
-	if (gpio_chips[alias_idx]) {
-		ret = -EBUSY;
-		goto err;
-	}
+	if (gpio_chips[alias_idx])
+		return dev_err_probe(dev, -EBUSY, "%d slot is occupied.\n", alias_idx);
 
 	irq = platform_get_irq(pdev, 0);
-	if (irq < 0) {
-		ret = irq;
-		goto err;
-	}
+	if (irq < 0)
+		return irq;
 
 	at91_chip = devm_kzalloc(&pdev->dev, sizeof(*at91_chip), GFP_KERNEL);
-	if (!at91_chip) {
-		ret = -ENOMEM;
-		goto err;
-	}
+	if (!at91_chip)
+		return -ENOMEM;
 
 	at91_chip->regbase = devm_platform_ioremap_resource(pdev, 0);
-	if (IS_ERR(at91_chip->regbase)) {
-		ret = PTR_ERR(at91_chip->regbase);
-		goto err;
-	}
+	if (IS_ERR(at91_chip->regbase))
+		return PTR_ERR(at91_chip->regbase);
 
 	at91_chip->ops = of_device_get_match_data(dev);
 	at91_chip->pioc_virq = irq;
 	at91_chip->pioc_idx = alias_idx;
 
 	at91_chip->clock = devm_clk_get_enabled(&pdev->dev, NULL);
-	if (IS_ERR(at91_chip->clock)) {
-		dev_err(&pdev->dev, "failed to get clock, ignoring.\n");
-		ret = PTR_ERR(at91_chip->clock);
-		goto err;
-	}
+	if (IS_ERR(at91_chip->clock))
+		return dev_err_probe(dev, PTR_ERR(at91_chip->clock), "failed to get clock, ignoring.\n");
 
 	at91_chip->chip = at91_gpio_template;
 	at91_chip->id = alias_idx;

@@ -1865,17 +1851,15 @@ static int at91_gpio_probe(struct platform_device *pdev)
 
 	if (!of_property_read_u32(np, "#gpio-lines", &ngpio)) {
 		if (ngpio >= MAX_NB_GPIO_PER_BANK)
-			pr_err("at91_gpio.%d, gpio-nb >= %d failback to %d\n",
-			       alias_idx, MAX_NB_GPIO_PER_BANK, MAX_NB_GPIO_PER_BANK);
+			dev_err(dev, "at91_gpio.%d, gpio-nb >= %d failback to %d\n",
+				alias_idx, MAX_NB_GPIO_PER_BANK, MAX_NB_GPIO_PER_BANK);
 		else
 			chip->ngpio = ngpio;
 	}
 
 	names = devm_kasprintf_strarray(dev, "pio", chip->ngpio);
-	if (!names) {
-		ret = -ENOMEM;
-		goto err;
-	}
+	if (!names)
+		return -ENOMEM;
 
 	for (i = 0; i < chip->ngpio; i++)
 		strreplace(names[i], '-', alias_idx + 'A');

@@ -1892,11 +1876,11 @@ static int at91_gpio_probe(struct platform_device *pdev)
 
 	ret = at91_gpio_of_irq_setup(pdev, at91_chip);
 	if (ret)
-		goto gpiochip_add_err;
+		return ret;
 
 	ret = gpiochip_add_data(chip, at91_chip);
 	if (ret)
-		goto gpiochip_add_err;
+		return ret;
 
 	gpio_chips[alias_idx] = at91_chip;
 	platform_set_drvdata(pdev, at91_chip);

@@ -1905,12 +1889,6 @@ static int at91_gpio_probe(struct platform_device *pdev)
 	dev_info(&pdev->dev, "at address %p\n", at91_chip->regbase);
 
 	return 0;
-
-gpiochip_add_err:
-err:
-	dev_err(&pdev->dev, "Failure %i for GPIO %i\n", ret, alias_idx);
-
-	return ret;
 }
 
 static const struct dev_pm_ops at91_gpio_pm_ops = {

Signature · 1 line

2.39.1

Andy Shevchenko

author

2023-02-15 13:42 UTC #5 lore raw

PATCH v3 5/5 pinctrl: at91: Utilise temporary variable for struct device

In reply to Andy Shevchenko

We have a temporary variable to keep pointer to struct device. Utilise it inside the ->probe() implementation.

Signed-off-by Andy Shevchenko <[email protected]>

---
 drivers/pinctrl/pinctrl-at91.c | 60 +++++++++++++++-------------------
 1 file changed, 26 insertions(+), 34 deletions(-)

drivers/pinctrl/pinctrl-at91.c +26 ~~-34~~

index 08f88403affb..de48a8e0f85f 100644
@@ -1304,7 +1304,7 @@ static int at91_pinctrl_probe_dt(struct platform_device *pdev,
 	if (!np)
 		return -ENODEV;
 
-	info->dev = &pdev->dev;
+	info->dev = dev;
 	info->ops = of_device_get_match_data(dev);
 	at91_pinctrl_child_count(info, np);

@@ -1324,35 +1324,31 @@ static int at91_pinctrl_probe_dt(struct platform_device *pdev,
 	if (ret)
 		return ret;
 
-	dev_dbg(&pdev->dev, "nmux = %d\n", info->nmux);
+	dev_dbg(dev, "nmux = %d\n", info->nmux);
 
-	dev_dbg(&pdev->dev, "mux-mask\n");
+	dev_dbg(dev, "mux-mask\n");
 	tmp = info->mux_mask;
 	for (i = 0; i < gpio_banks; i++) {
 		for (j = 0; j < info->nmux; j++, tmp++) {
-			dev_dbg(&pdev->dev, "%d:%d\t0x%x\n", i, j, tmp[0]);
+			dev_dbg(dev, "%d:%d\t0x%x\n", i, j, tmp[0]);
 		}
 	}
 
-	dev_dbg(&pdev->dev, "nfunctions = %d\n", info->nfunctions);
-	dev_dbg(&pdev->dev, "ngroups = %d\n", info->ngroups);
-	info->functions = devm_kcalloc(&pdev->dev,
-					info->nfunctions,
-					sizeof(struct at91_pmx_func),
-					GFP_KERNEL);
+	dev_dbg(dev, "nfunctions = %d\n", info->nfunctions);
+	dev_dbg(dev, "ngroups = %d\n", info->ngroups);
+	info->functions = devm_kcalloc(dev, info->nfunctions, sizeof(*info->functions),
+				       GFP_KERNEL);
 	if (!info->functions)
 		return -ENOMEM;
 
-	info->groups = devm_kcalloc(&pdev->dev,
-					info->ngroups,
-					sizeof(struct at91_pin_group),
-					GFP_KERNEL);
+	info->groups = devm_kcalloc(dev, info->ngroups, sizeof(*info->groups),
+				    GFP_KERNEL);
 	if (!info->groups)
 		return -ENOMEM;
 
-	dev_dbg(&pdev->dev, "nbanks = %d\n", gpio_banks);
-	dev_dbg(&pdev->dev, "nfunctions = %d\n", info->nfunctions);
-	dev_dbg(&pdev->dev, "ngroups = %d\n", info->ngroups);
+	dev_dbg(dev, "nbanks = %d\n", gpio_banks);
+	dev_dbg(dev, "nfunctions = %d\n", info->nfunctions);
+	dev_dbg(dev, "ngroups = %d\n", info->ngroups);
 
 	i = 0;

@@ -1376,7 +1372,7 @@ static int at91_pinctrl_probe(struct platform_device *pdev)
 	struct pinctrl_pin_desc *pdesc;
 	int ret, i, j, k;
 
-	info = devm_kzalloc(&pdev->dev, sizeof(*info), GFP_KERNEL);
+	info = devm_kzalloc(dev, sizeof(*info), GFP_KERNEL);
 	if (!info)
 		return -ENOMEM;

@@ -1384,13 +1380,10 @@ static int at91_pinctrl_probe(struct platform_device *pdev)
 	if (ret)
 		return ret;
 
-	at91_pinctrl_desc.name = dev_name(&pdev->dev);
+	at91_pinctrl_desc.name = dev_name(dev);
 	at91_pinctrl_desc.npins = gpio_banks * MAX_NB_GPIO_PER_BANK;
 	at91_pinctrl_desc.pins = pdesc =
-		devm_kcalloc(&pdev->dev,
-			     at91_pinctrl_desc.npins, sizeof(*pdesc),
-			     GFP_KERNEL);
-
+		devm_kcalloc(dev, at91_pinctrl_desc.npins, sizeof(*pdesc), GFP_KERNEL);
 	if (!at91_pinctrl_desc.pins)
 		return -ENOMEM;

@@ -1413,8 +1406,7 @@ static int at91_pinctrl_probe(struct platform_device *pdev)
 	}
 
 	platform_set_drvdata(pdev, info);
-	info->pctl = devm_pinctrl_register(&pdev->dev, &at91_pinctrl_desc,
-					   info);
+	info->pctl = devm_pinctrl_register(dev, &at91_pinctrl_desc, info);
 	if (IS_ERR(info->pctl))
 		return dev_err_probe(dev, PTR_ERR(info->pctl), "could not register AT91 pinctrl driver\n");

@@ -1423,7 +1415,7 @@ static int at91_pinctrl_probe(struct platform_device *pdev)
 		if (gpio_chips[i])
 			pinctrl_add_gpio_range(info->pctl, &gpio_chips[i]->range);
 
-	dev_info(&pdev->dev, "initialized AT91 pinctrl driver\n");
+	dev_info(dev, "initialized AT91 pinctrl driver\n");
 
 	return 0;
 }

@@ -1714,6 +1706,7 @@ static void gpio_irq_handler(struct irq_desc *desc)
 static int at91_gpio_of_irq_setup(struct platform_device *pdev,
 				  struct at91_gpio_chip *at91_gpio)
 {
+	struct device		*dev = &pdev->dev;
 	struct gpio_chip	*gpiochip_prev = NULL;
 	struct at91_gpio_chip   *prev = NULL;
 	struct irq_data		*d = irq_get_irq_data(at91_gpio->pioc_virq);

@@ -1721,8 +1714,7 @@ static int at91_gpio_of_irq_setup(struct platform_device *pdev,
 	struct gpio_irq_chip	*girq;
 	int i;
 
-	gpio_irqchip = devm_kzalloc(&pdev->dev, sizeof(*gpio_irqchip),
-				    GFP_KERNEL);
+	gpio_irqchip = devm_kzalloc(dev, sizeof(*gpio_irqchip), GFP_KERNEL);
 	if (!gpio_irqchip)
 		return -ENOMEM;

@@ -1758,7 +1750,7 @@ static int at91_gpio_of_irq_setup(struct platform_device *pdev,
 	if (!gpiochip_prev) {
 		girq->parent_handler = gpio_irq_handler;
 		girq->num_parents = 1;
-		girq->parents = devm_kcalloc(&pdev->dev, 1,
+		girq->parents = devm_kcalloc(dev, girq->num_parents,
 					     sizeof(*girq->parents),
 					     GFP_KERNEL);
 		if (!girq->parents)

@@ -1824,7 +1816,7 @@ static int at91_gpio_probe(struct platform_device *pdev)
 	if (irq < 0)
 		return irq;
 
-	at91_chip = devm_kzalloc(&pdev->dev, sizeof(*at91_chip), GFP_KERNEL);
+	at91_chip = devm_kzalloc(dev, sizeof(*at91_chip), GFP_KERNEL);
 	if (!at91_chip)
 		return -ENOMEM;

@@ -1836,7 +1828,7 @@ static int at91_gpio_probe(struct platform_device *pdev)
 	at91_chip->pioc_virq = irq;
 	at91_chip->pioc_idx = alias_idx;
 
-	at91_chip->clock = devm_clk_get_enabled(&pdev->dev, NULL);
+	at91_chip->clock = devm_clk_get_enabled(dev, NULL);
 	if (IS_ERR(at91_chip->clock))
 		return dev_err_probe(dev, PTR_ERR(at91_chip->clock), "failed to get clock, ignoring.\n");

@@ -1844,8 +1836,8 @@ static int at91_gpio_probe(struct platform_device *pdev)
 	at91_chip->id = alias_idx;
 
 	chip = &at91_chip->chip;
-	chip->label = dev_name(&pdev->dev);
-	chip->parent = &pdev->dev;
+	chip->label = dev_name(dev);
+	chip->parent = dev;
 	chip->owner = THIS_MODULE;
 	chip->base = alias_idx * MAX_NB_GPIO_PER_BANK;

@@ -1886,7 +1878,7 @@ static int at91_gpio_probe(struct platform_device *pdev)
 	platform_set_drvdata(pdev, at91_chip);
 	gpio_banks = max(gpio_banks, alias_idx + 1);
 
-	dev_info(&pdev->dev, "at address %p\n", at91_chip->regbase);
+	dev_info(dev, "at address %p\n", at91_chip->regbase);
 
 	return 0;
 }

Signature · 1 line

2.39.1

[email protected]

Reviewed-by on v3 5/5

2023-02-17 11:54 UTC #6 lore raw

Re: [PATCH v3 5/5] pinctrl: at91: Utilise temporary variable for struct device

drivers/pinctrl/pinctrl-at91.c static int at91_gpio_of_irq_setup(struct platform_device *pdev, · line 1753

Quoted 6 lines · Andy Shevchenko

drivers/pinctrl/pinctrl-at91.c +1 -1

@@ -1758,7 +1750,7 @@ static int at91_gpio_of_irq_setup(struct platform_device *pdev,
        if (!gpiochip_prev) {
                girq->parent_handler = gpio_irq_handler;
                girq->num_parents = 1;
-               girq->parents = devm_kcalloc(&pdev->dev, 1,
+               girq->parents = devm_kcalloc(dev, girq->num_parents,

There is still the change of the 2nd argument of devm_kcalloc() from 1 -> girq->num_parents (no functional change, though) which doesn't match the purpose of the patch and is not specified anywhere. Other than this:

Reviewed-by Claudiu Beznea <[email protected]>

Quoted 2 lines

drivers/pinctrl/pinctrl-at91.c +0 -0

                                             sizeof(*girq->parents),
                                             GFP_KERNEL);

Andy Shevchenko

author

2023-02-17 11:59 UTC #7 lore raw

Re: [PATCH v3 5/5] pinctrl: at91: Utilise temporary variable for struct device

Quoted 12 lines · On Fri, Feb 17, 2023 at 11:54:02AM +0000, [email protected] wrote:

drivers/pinctrl/pinctrl-at91.c static int at91_gpio_of_irq_setup(struct platform_device *pdev, · line 1753
Quoted 6 lines · Andy Shevchenko
drivers/pinctrl/pinctrl-at91.c +1 -1
@@ -1758,7 +1750,7 @@ static int at91_gpio_of_irq_setup(struct platform_device *pdev,
        if (!gpiochip_prev) {
                girq->parent_handler = gpio_irq_handler;
                girq->num_parents = 1;
-               girq->parents = devm_kcalloc(&pdev->dev, 1,
+               girq->parents = devm_kcalloc(dev, girq->num_parents,
There is still the change of the 2nd argument of devm_kcalloc() from 1 -> girq->num_parents (no functional change, though) which doesn't match the purpose of the patch and is not specified anywhere. Other than this:

Quoted 4 lines

Reviewed-by Claudiu Beznea <[email protected]>

Quoted 2 lines

drivers/pinctrl/pinctrl-at91.c +0 -0

                                             sizeof(*girq->parents),
                                             GFP_KERNEL);

Thanks for review. I have no time to fix this. So if Linus is okay to take the first 4 patches, I'm fine. You or somebody else can submit an updated 5th patch later on.

Signature · 2 lines

With Best Regards,
Andy Shevchenko

Linus Walleij

2023-02-27 21:44 UTC #8 lore raw

Re: [PATCH v3 5/5] pinctrl: at91: Utilise temporary variable for struct device

Quoted 13 lines · Andy Shevchenko

Quoted 12 lines · On Fri, Feb 17, 2023 at 11:54:02AM +0000, [email protected] wrote:
drivers/pinctrl/pinctrl-at91.c static int at91_gpio_of_irq_setup(struct platform_device *pdev, · line 1753
Quoted 6 lines · Andy Shevchenko
drivers/pinctrl/pinctrl-at91.c +1 -1
@@ -1758,7 +1750,7 @@ static int at91_gpio_of_irq_setup(struct platform_device *pdev,
        if (!gpiochip_prev) {
                girq->parent_handler = gpio_irq_handler;
                girq->num_parents = 1;
-               girq->parents = devm_kcalloc(&pdev->dev, 1,
+               girq->parents = devm_kcalloc(dev, girq->num_parents,
There is still the change of the 2nd argument of devm_kcalloc() from 1 -> girq->num_parents (no functional change, though) which doesn't match the purpose of the patch and is not specified anywhere. Other than this:

Quoted 8 lines

Quoted 4 lines
Reviewed-by Claudiu Beznea <[email protected]>
Quoted 2 lines
drivers/pinctrl/pinctrl-at91.c +0 -0
                                             sizeof(*girq->parents),
                                             GFP_KERNEL);
Thanks for review. I have no time to fix this. So if Linus is okay to take the first 4 patches, I'm fine. You or somebody else can submit an updated 5th patch later on.

I applied all 5 patches. The num_parents is set to 1 on the line right above and it's the right thing to do, and has a reviewed tag so I don't see the problem with this patch, let's not overinvest in process.

Thanks for the very nice cleanups!

I applied it locally so it won't be in linux-next until after the merge window closes.

Yours,
Linus Walleij

Andy Shevchenko

author

2023-02-27 22:20 UTC #9 lore raw

Re: [PATCH v3 5/5] pinctrl: at91: Utilise temporary variable for struct device

Quoted 4 lines · Linus Walleij

Quoted 2 lines · Andy Shevchenko
Quoted 1 line · On Fri, Feb 17, 2023 at 11:54:02AM +0000, [email protected] wrote:
On 15.02.2023 15:42, Andy Shevchenko wrote:

...

Quoted 8 lines

Quoted 8 lines
Quoted 8 lines
drivers/pinctrl/pinctrl-at91.c static int at91_gpio_of_irq_setup(struct platform_device *pdev, · line 1753
Quoted 3 lines
                girq->num_parents = 1;
drivers/pinctrl/pinctrl-at91.c +1 -1
-               girq->parents = devm_kcalloc(&pdev->dev, 1,
+               girq->parents = devm_kcalloc(dev, girq->num_parents,
There is still the change of the 2nd argument of devm_kcalloc() from 1 -> girq->num_parents (no functional change, though) which doesn't match the purpose of the patch and is not specified anywhere. Other than this:

Quoted 5 lines

Quoted 4 lines

Reviewed-by Claudiu Beznea <[email protected]>

Quoted 2 lines

                                             sizeof(*girq->parents),
                                             GFP_KERNEL);

Quoted 9 lines

Quoted 3 lines
Thanks for review. I have no time to fix this. So if Linus is okay to take the first 4 patches, I'm fine. You or somebody else can submit an updated 5th patch later on.
I applied all 5 patches. The num_parents is set to 1 on the line right above and it's the right thing to do, and has a reviewed tag so I don't see the problem with this patch, let's not overinvest in process.
Thanks for the very nice cleanups!

Thank you!

Quoted 2 lines

I applied it locally so it won't be in linux-next until after the merge window closes.

Signature · 2 lines

With Best Regards,
Andy Shevchenko