[PATCH 0/5] gpio: ppc44x: assorted cleanups

[PATCH 0/5] gpio: ppc44x: assorted cleanups

[Rosen Penev]

A follow-up to the earlier ppc44x GPIO conversion to gpio-mmio. With the
driver now built on gpio_generic_chip, a few leftovers can go:

 - Patch 1 drops the label and parent assignments that
   gpio_generic_chip_init() already does.
 - Patches 2-3 deduplicate the OSR/TSR source selection shared by both
   direction callbacks and fold the single-caller __ppc44x_gpio_set()
   into ppc44x_gpio_dir_out().
 - Patch 4 expresses the MSB-first pin masks with BIT() and GENMASK().
 - Patch 5 trims includes, switches to devm_kzalloc_obj() and tidies
   up probe and the OF match table.

No functional change intended.

An AI coding assistant (Claude) was used to help prepare this series.

Rosen Penev (5):
  gpio: ppc44x: drop redundant label and parent assignments
  gpio: ppc44x: factor out the OSR/TSR source selection
  gpio: ppc44x: fold __ppc44x_gpio_set() into ppc44x_gpio_dir_out()
  gpio: ppc44x: use BIT() and GENMASK() for the pin masks
  gpio: ppc44x: tidy up includes and probe

 drivers/gpio/gpio-ppc44x.c | 89 ++++++++++++++++----------------------
 1 file changed, 37 insertions(+), 52 deletions(-)

-- 
2.56.0

[PATCH 1/5] gpio: ppc44x: drop redundant label and parent assignments

[Rosen Penev]

gpio_generic_chip_init() already sets gc->parent to the device and
gc->label to dev_name() of it. Drop the duplicate assignments from
probe.

Signed-off-by: Rosen Penev <rosenp@gmail.com>
---
 drivers/gpio/gpio-ppc44x.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/gpio/gpio-ppc44x.c b/drivers/gpio/gpio-ppc44x.c
index 189667e0e1ba..01dd5e7cc311 100644
--- a/drivers/gpio/gpio-ppc44x.c
+++ b/drivers/gpio/gpio-ppc44x.c
@@ -178,8 +178,6 @@ static int ppc44x_gpio_probe(struct platform_device *ofdev)
 		return ret;
 
 	gc = &chip->chip.gc;
-	gc->label = dev_name(dev);
-	gc->parent = dev;
 	gc->direction_input = ppc44x_gpio_dir_in;
 	gc->direction_output = ppc44x_gpio_dir_out;
 
-- 
2.56.0

[PATCH 2/5] gpio: ppc44x: factor out the OSR/TSR source selection

[Rosen Penev]

Both direction callbacks clear the pin's 2-bit field in the output
source and three-state source registers, picking the low or high
register by bank. Move that into a helper instead of duplicating the
if/else in each callback.

No functional change; the registers are written in the same order.

Signed-off-by: Rosen Penev <rosenp@gmail.com>
---
 drivers/gpio/gpio-ppc44x.c | 33 +++++++++++++++++----------------
 1 file changed, 17 insertions(+), 16 deletions(-)

diff --git a/drivers/gpio/gpio-ppc44x.c b/drivers/gpio/gpio-ppc44x.c
index 01dd5e7cc311..b3cbc832cf24 100644
--- a/drivers/gpio/gpio-ppc44x.c
+++ b/drivers/gpio/gpio-ppc44x.c
@@ -64,6 +64,21 @@ static inline void ppc44x_setbits32(void __iomem *addr, u32 mask)
 	iowrite32be(val, addr);
 }
 
+/*
+ * Route the pin's output and three-state control to the GPIO controller
+ * rather than an alternate function. Bits 0-15 use OSRL/TSRL, bits 16-31
+ * use OSRH/TSRH.
+ */
+static void ppc44x_gpio_select_gpio(struct ppc44x_gpio __iomem *regs,
+				    unsigned int gpio)
+{
+	__be32 __iomem *osr = gpio < 16 ? &regs->osrl : &regs->osrh;
+	__be32 __iomem *tsr = gpio < 16 ? &regs->tsrl : &regs->tsrh;
+
+	ppc44x_clrbits32(osr, GPIO_MASK2(gpio));
+	ppc44x_clrbits32(tsr, GPIO_MASK2(gpio));
+}
+
 /*
  * GPIO LIB API implementation for GPIOs
  *
@@ -99,14 +114,7 @@ static int ppc44x_gpio_dir_in(struct gpio_chip *gc, unsigned int gpio)
 	ppc44x_clrbits32(&regs->tcr, GPIO_MASK(gpio));
 	gen_gc->sdir &= ~GPIO_MASK(gpio);
 
-	/* Bits 0-15 use TSRL/OSRL, bits 16-31 use TSRH/OSRH */
-	if (gpio < 16) {
-		ppc44x_clrbits32(&regs->osrl, GPIO_MASK2(gpio));
-		ppc44x_clrbits32(&regs->tsrl, GPIO_MASK2(gpio));
-	} else {
-		ppc44x_clrbits32(&regs->osrh, GPIO_MASK2(gpio));
-		ppc44x_clrbits32(&regs->tsrh, GPIO_MASK2(gpio));
-	}
+	ppc44x_gpio_select_gpio(regs, gpio);
 
 	return 0;
 }
@@ -130,14 +138,7 @@ ppc44x_gpio_dir_out(struct gpio_chip *gc, unsigned int gpio, int val)
 	ppc44x_setbits32(&regs->tcr, GPIO_MASK(gpio));
 	gen_gc->sdir |= GPIO_MASK(gpio);
 
-	/* Bits 0-15 use TSRL, bits 16-31 use TSRH */
-	if (gpio < 16) {
-		ppc44x_clrbits32(&regs->osrl, GPIO_MASK2(gpio));
-		ppc44x_clrbits32(&regs->tsrl, GPIO_MASK2(gpio));
-	} else {
-		ppc44x_clrbits32(&regs->osrh, GPIO_MASK2(gpio));
-		ppc44x_clrbits32(&regs->tsrh, GPIO_MASK2(gpio));
-	}
+	ppc44x_gpio_select_gpio(regs, gpio);
 
 	pr_debug("%s: gpio: %d val: %d\n", __func__, gpio, val);
 
-- 
2.56.0

[PATCH 3/5] gpio: ppc44x: fold __ppc44x_gpio_set() into ppc44x_gpio_dir_out()

[Rosen Penev]

__ppc44x_gpio_set() has a single caller, and it looks up the driver
data that the caller already has. Open-code it in the direction_output
callback, with a comment explaining why gpio_generic_chip_set() can't
be used there: the generic setter takes the chip lock, which is already
held at that point.

While at it, drop the pr_debug(); gpiolib's tracepoints already report
direction and value changes.

Signed-off-by: Rosen Penev <rosenp@gmail.com>
---
 drivers/gpio/gpio-ppc44x.c | 28 ++++++++++------------------
 1 file changed, 10 insertions(+), 18 deletions(-)

diff --git a/drivers/gpio/gpio-ppc44x.c b/drivers/gpio/gpio-ppc44x.c
index b3cbc832cf24..20b3e9ce48e8 100644
--- a/drivers/gpio/gpio-ppc44x.c
+++ b/drivers/gpio/gpio-ppc44x.c
@@ -85,20 +85,6 @@ static void ppc44x_gpio_select_gpio(struct ppc44x_gpio __iomem *regs,
  * There are a maximum of 32 gpios in each gpio controller.
  */
 
-static inline void
-__ppc44x_gpio_set(struct gpio_chip *gc, unsigned int gpio, int val)
-{
-	struct ppc44x_gpio_chip *chip = gpiochip_get_data(gc);
-	struct gpio_generic_chip *gen_gc = &chip->chip;
-
-	if (val)
-		gen_gc->sdata |= GPIO_MASK(gpio);
-	else
-		gen_gc->sdata &= ~GPIO_MASK(gpio);
-
-	gpio_generic_write_reg(gen_gc, gen_gc->reg_set, gen_gc->sdata);
-}
-
 static int ppc44x_gpio_dir_in(struct gpio_chip *gc, unsigned int gpio)
 {
 	struct ppc44x_gpio_chip *chip = gpiochip_get_data(gc);
@@ -128,8 +114,16 @@ ppc44x_gpio_dir_out(struct gpio_chip *gc, unsigned int gpio, int val)
 
 	guard(gpio_generic_lock_irqsave)(gen_gc);
 
-	/* First set initial value */
-	__ppc44x_gpio_set(gc, gpio, val);
+	/*
+	 * First set the initial value. This can't use gpio_generic_chip_set()
+	 * as the generic setter takes the chip lock, which is already held.
+	 */
+	if (val)
+		gen_gc->sdata |= GPIO_MASK(gpio);
+	else
+		gen_gc->sdata &= ~GPIO_MASK(gpio);
+
+	gpio_generic_write_reg(gen_gc, gen_gc->reg_set, gen_gc->sdata);
 
 	/* Disable open-drain function */
 	ppc44x_clrbits32(&regs->odr, GPIO_MASK(gpio));
@@ -140,8 +134,6 @@ ppc44x_gpio_dir_out(struct gpio_chip *gc, unsigned int gpio, int val)
 
 	ppc44x_gpio_select_gpio(regs, gpio);
 
-	pr_debug("%s: gpio: %d val: %d\n", __func__, gpio, val);
-
 	return 0;
 }
 
-- 
2.56.0

[PATCH 4/5] gpio: ppc44x: use BIT() and GENMASK() for the pin masks

[Rosen Penev]

Express the per-pin masks with BIT() and GENMASK() rather than
open-coded shifts of 0x80000000 and 0xc0000000. The pins are numbered
from the MSB, matching the bit mapping gpio-mmio uses for
GPIO_GENERIC_BIG_ENDIAN.

No functional change; both forms yield the same mask for all 32 pins.

Signed-off-by: Rosen Penev <rosenp@gmail.com>
---
 drivers/gpio/gpio-ppc44x.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/gpio/gpio-ppc44x.c b/drivers/gpio/gpio-ppc44x.c
index 20b3e9ce48e8..8b9ea00f170c 100644
--- a/drivers/gpio/gpio-ppc44x.c
+++ b/drivers/gpio/gpio-ppc44x.c
@@ -9,6 +9,7 @@
  * Author: Steve Falco <sfalco@harris.com>
  */
 
+#include <linux/bits.h>
 #include <linux/kernel.h>
 #include <linux/init.h>
 #include <linux/io.h>
@@ -18,8 +19,9 @@
 #include <linux/slab.h>
 #include <linux/platform_device.h>
 
-#define GPIO_MASK(gpio)		(0x80000000 >> (gpio))
-#define GPIO_MASK2(gpio)	(0xc0000000 >> (((gpio) % 16) * 2))
+/* Pins are numbered from the MSB; the select registers use 2 bits per pin */
+#define GPIO_MASK(gpio)		BIT(31 - (gpio))
+#define GPIO_MASK2(gpio)	(GENMASK(1, 0) << (30 - ((gpio) % 16) * 2))
 
 /* Physical GPIO register layout */
 struct ppc44x_gpio {
-- 
2.56.0

[PATCH 5/5] gpio: ppc44x: tidy up includes and probe

[Rosen Penev]

Remove some unused includes that are not used directly.

While at it:
 - use devm_kzalloc_obj() for the chip allocation
 - rename the platform_device from ofdev to the conventional pdev
 - compact the OF match table
 - move module_platform_driver() next to the driver struct

No functional change.

Signed-off-by: Rosen Penev <rosenp@gmail.com>
---
 drivers/gpio/gpio-ppc44x.c | 20 ++++++--------------
 1 file changed, 6 insertions(+), 14 deletions(-)

diff --git a/drivers/gpio/gpio-ppc44x.c b/drivers/gpio/gpio-ppc44x.c
index 8b9ea00f170c..04653e753d43 100644
--- a/drivers/gpio/gpio-ppc44x.c
+++ b/drivers/gpio/gpio-ppc44x.c
@@ -10,13 +10,8 @@
  */
 
 #include <linux/bits.h>
-#include <linux/kernel.h>
-#include <linux/init.h>
 #include <linux/io.h>
-#include <linux/of.h>
 #include <linux/gpio/generic.h>
-#include <linux/types.h>
-#include <linux/slab.h>
 #include <linux/platform_device.h>
 
 /* Pins are numbered from the MSB; the select registers use 2 bits per pin */
@@ -139,20 +134,20 @@ ppc44x_gpio_dir_out(struct gpio_chip *gc, unsigned int gpio, int val)
 	return 0;
 }
 
-static int ppc44x_gpio_probe(struct platform_device *ofdev)
+static int ppc44x_gpio_probe(struct platform_device *pdev)
 {
-	struct device *dev = &ofdev->dev;
+	struct device *dev = &pdev->dev;
 	struct ppc44x_gpio __iomem *regs;
 	struct ppc44x_gpio_chip *chip;
 	struct gpio_generic_chip_config config;
 	struct gpio_chip *gc;
 	int ret;
 
-	regs = devm_platform_ioremap_resource(ofdev, 0);
+	regs = devm_platform_ioremap_resource(pdev, 0);
 	if (IS_ERR(regs))
 		return PTR_ERR(regs);
 
-	chip = devm_kzalloc(dev, sizeof(*chip), GFP_KERNEL);
+	chip = devm_kzalloc_obj(dev, *chip);
 	if (!chip)
 		return -ENOMEM;
 
@@ -180,9 +175,7 @@ static int ppc44x_gpio_probe(struct platform_device *ofdev)
 }
 
 static const struct of_device_id ppc44x_gpio_match[] = {
-	{
-		.compatible = "ibm,ppc4xx-gpio",
-	},
+	{ .compatible = "ibm,ppc4xx-gpio" },
 	{},
 };
 MODULE_DEVICE_TABLE(of, ppc44x_gpio_match);
@@ -194,8 +187,7 @@ static struct platform_driver ppc44x_gpio_driver = {
 		.of_match_table	= ppc44x_gpio_match,
 	},
 };
+module_platform_driver(ppc44x_gpio_driver);
 
 MODULE_DESCRIPTION("PPC44x gpio driver");
 MODULE_LICENSE("GPL");
-
-module_platform_driver(ppc44x_gpio_driver);
-- 
2.56.0

Re: [PATCH 5/5] gpio: ppc44x: tidy up includes and probe

[Bartosz Golaszewski]

On Wed, Nov 04, 2026 at 01:59:59PM -0800, Rosen Penev wrote:
> Remove some unused includes that are not used directly.
> 
> While at it:
>  - use devm_kzalloc_obj() for the chip allocation
>  - rename the platform_device from ofdev to the conventional pdev
>  - compact the OF match table
>  - move module_platform_driver() next to the driver struct
> 
> No functional change.
> 
> Signed-off-by: Rosen Penev <rosenp@gmail.com>
> ---
>  drivers/gpio/gpio-ppc44x.c | 20 ++++++--------------
>  1 file changed, 6 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/gpio/gpio-ppc44x.c b/drivers/gpio/gpio-ppc44x.c
> index 8b9ea00f170c..04653e753d43 100644
> --- a/drivers/gpio/gpio-ppc44x.c
> +++ b/drivers/gpio/gpio-ppc44x.c
> @@ -10,13 +10,8 @@
>   */
>  
>  #include <linux/bits.h>
> -#include <linux/kernel.h>
> -#include <linux/init.h>
>  #include <linux/io.h>
> -#include <linux/of.h>
>  #include <linux/gpio/generic.h>
> -#include <linux/types.h>
> -#include <linux/slab.h>
>  #include <linux/platform_device.h>
>  
>  /* Pins are numbered from the MSB; the select registers use 2 bits per pin */
> @@ -139,20 +134,20 @@ ppc44x_gpio_dir_out(struct gpio_chip *gc, unsigned int gpio, int val)
>  	return 0;
>  }
>  
> -static int ppc44x_gpio_probe(struct platform_device *ofdev)
> +static int ppc44x_gpio_probe(struct platform_device *pdev)
>  {
> -	struct device *dev = &ofdev->dev;
> +	struct device *dev = &pdev->dev;
>  	struct ppc44x_gpio __iomem *regs;
>  	struct ppc44x_gpio_chip *chip;
>  	struct gpio_generic_chip_config config;
>  	struct gpio_chip *gc;
>  	int ret;
>  
> -	regs = devm_platform_ioremap_resource(ofdev, 0);
> +	regs = devm_platform_ioremap_resource(pdev, 0);
>  	if (IS_ERR(regs))
>  		return PTR_ERR(regs);
>  
> -	chip = devm_kzalloc(dev, sizeof(*chip), GFP_KERNEL);
> +	chip = devm_kzalloc_obj(dev, *chip);
>  	if (!chip)
>  		return -ENOMEM;
>  
> @@ -180,9 +175,7 @@ static int ppc44x_gpio_probe(struct platform_device *ofdev)
>  }
>  
>  static const struct of_device_id ppc44x_gpio_match[] = {
> -	{
> -		.compatible = "ibm,ppc4xx-gpio",
> -	},
> +	{ .compatible = "ibm,ppc4xx-gpio" },
>  	{},

Uwe: is this in line with your recent cleanups?

Bart

Re: [PATCH 3/5] gpio: ppc44x: fold __ppc44x_gpio_set() into ppc44x_gpio_dir_out()

[Bartosz Golaszewski]

On Wed, Nov 04, 2026 at 01:59:57PM -0800, Rosen Penev wrote:
> __ppc44x_gpio_set() has a single caller, and it looks up the driver
> data that the caller already has. Open-code it in the direction_output
> callback, with a comment explaining why gpio_generic_chip_set() can't
> be used there: the generic setter takes the chip lock, which is already
> held at that point.
> 
> While at it, drop the pr_debug(); gpiolib's tracepoints already report
> direction and value changes.
> 
> Signed-off-by: Rosen Penev <rosenp@gmail.com>
> ---
 
Sigh... This is really just churn...

I'll take it with the rest of the series as sashiko doesn't seem to
complain but this time would be better spent elsewhere. If you like
fixing bugs, I can assign you a number of GPIO bugs sashiko found.

Bart

Re: [PATCH 5/5] gpio: ppc44x: tidy up includes and probe

[Uwe Kleine-König]

[-- Attachment #1.1: Type: text/plain, Size: 396 bytes --]

Hello Bart,

On 2026-11-09 10:01, Bartosz Golaszewski wrote:
>>  static const struct of_device_id ppc44x_gpio_match[] = {
>> -	{
>> -		.compatible = "ibm,ppc4xx-gpio",
>> -	},
>> +	{ .compatible = "ibm,ppc4xx-gpio" },
>>  	{},
> 
> Uwe: is this in line with your recent cleanups?
The change is fine, I'd appreciate to also do

-	{},
+	{ }

at the same time.

Best regards
Uwe

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]