[PATCH v1 1/1] powerpc/8xx: Drop legacy-of-mm-gpiochip.h header

[PATCH v1 1/1] powerpc/8xx: Drop legacy-of-mm-gpiochip.h header

[Andy Shevchenko]

Remove legacy-of-mm-gpiochip.h header file, replace of_* functions
and structs with appropriate alternatives.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 arch/powerpc/platforms/8xx/cpm1.c | 119 +++++++++++++++---------------
 1 file changed, 60 insertions(+), 59 deletions(-)

diff --git a/arch/powerpc/platforms/8xx/cpm1.c b/arch/powerpc/platforms/8xx/cpm1.c
index b24d4102fbf6..1262bff5ba2e 100644
--- a/arch/powerpc/platforms/8xx/cpm1.c
+++ b/arch/powerpc/platforms/8xx/cpm1.c
@@ -45,7 +45,7 @@
 #include <sysdev/fsl_soc.h>
 
 #ifdef CONFIG_8xx_GPIO
-#include <linux/gpio/legacy-of-mm-gpiochip.h>
+#include <linux/gpio/driver.h>
 #endif
 
 #define CPM_MAP_SIZE    (0x4000)
@@ -376,7 +376,8 @@ int __init cpm1_clk_setup(enum cpm_clk_target target, int clock, int mode)
 #ifdef CONFIG_8xx_GPIO
 
 struct cpm1_gpio16_chip {
-	struct of_mm_gpio_chip mm_gc;
+	struct gpio_chip gc;
+	void __iomem *regs;
 	spinlock_t lock;
 
 	/* shadowed data register to clear/set bits safely */
@@ -386,19 +387,17 @@ struct cpm1_gpio16_chip {
 	int irq[16];
 };
 
-static void cpm1_gpio16_save_regs(struct of_mm_gpio_chip *mm_gc)
+static void cpm1_gpio16_save_regs(struct cpm1_gpio16_chip *cpm1_gc)
 {
-	struct cpm1_gpio16_chip *cpm1_gc =
-		container_of(mm_gc, struct cpm1_gpio16_chip, mm_gc);
-	struct cpm_ioport16 __iomem *iop = mm_gc->regs;
+	struct cpm_ioport16 __iomem *iop = cpm1_gc->regs;
 
 	cpm1_gc->cpdata = in_be16(&iop->dat);
 }
 
 static int cpm1_gpio16_get(struct gpio_chip *gc, unsigned int gpio)
 {
-	struct of_mm_gpio_chip *mm_gc = to_of_mm_gpio_chip(gc);
-	struct cpm_ioport16 __iomem *iop = mm_gc->regs;
+	struct cpm1_gpio16_chip *cpm1_gc = gpiochip_get_data(gc);
+	struct cpm_ioport16 __iomem *iop = cpm1_gc->regs;
 	u16 pin_mask;
 
 	pin_mask = 1 << (15 - gpio);
@@ -406,11 +405,9 @@ static int cpm1_gpio16_get(struct gpio_chip *gc, unsigned int gpio)
 	return !!(in_be16(&iop->dat) & pin_mask);
 }
 
-static void __cpm1_gpio16_set(struct of_mm_gpio_chip *mm_gc, u16 pin_mask,
-	int value)
+static void __cpm1_gpio16_set(struct cpm1_gpio16_chip *cpm1_gc, u16 pin_mask, int value)
 {
-	struct cpm1_gpio16_chip *cpm1_gc = gpiochip_get_data(&mm_gc->gc);
-	struct cpm_ioport16 __iomem *iop = mm_gc->regs;
+	struct cpm_ioport16 __iomem *iop = cpm1_gc->regs;
 
 	if (value)
 		cpm1_gc->cpdata |= pin_mask;
@@ -422,38 +419,35 @@ static void __cpm1_gpio16_set(struct of_mm_gpio_chip *mm_gc, u16 pin_mask,
 
 static void cpm1_gpio16_set(struct gpio_chip *gc, unsigned int gpio, int value)
 {
-	struct of_mm_gpio_chip *mm_gc = to_of_mm_gpio_chip(gc);
-	struct cpm1_gpio16_chip *cpm1_gc = gpiochip_get_data(&mm_gc->gc);
+	struct cpm1_gpio16_chip *cpm1_gc = gpiochip_get_data(gc);
 	unsigned long flags;
 	u16 pin_mask = 1 << (15 - gpio);
 
 	spin_lock_irqsave(&cpm1_gc->lock, flags);
 
-	__cpm1_gpio16_set(mm_gc, pin_mask, value);
+	__cpm1_gpio16_set(cpm1_gc, pin_mask, value);
 
 	spin_unlock_irqrestore(&cpm1_gc->lock, flags);
 }
 
 static int cpm1_gpio16_to_irq(struct gpio_chip *gc, unsigned int gpio)
 {
-	struct of_mm_gpio_chip *mm_gc = to_of_mm_gpio_chip(gc);
-	struct cpm1_gpio16_chip *cpm1_gc = gpiochip_get_data(&mm_gc->gc);
+	struct cpm1_gpio16_chip *cpm1_gc = gpiochip_get_data(gc);
 
 	return cpm1_gc->irq[gpio] ? : -ENXIO;
 }
 
 static int cpm1_gpio16_dir_out(struct gpio_chip *gc, unsigned int gpio, int val)
 {
-	struct of_mm_gpio_chip *mm_gc = to_of_mm_gpio_chip(gc);
-	struct cpm1_gpio16_chip *cpm1_gc = gpiochip_get_data(&mm_gc->gc);
-	struct cpm_ioport16 __iomem *iop = mm_gc->regs;
+	struct cpm1_gpio16_chip *cpm1_gc = gpiochip_get_data(gc);
+	struct cpm_ioport16 __iomem *iop = cpm1_gc->regs;
 	unsigned long flags;
 	u16 pin_mask = 1 << (15 - gpio);
 
 	spin_lock_irqsave(&cpm1_gc->lock, flags);
 
 	setbits16(&iop->dir, pin_mask);
-	__cpm1_gpio16_set(mm_gc, pin_mask, val);
+	__cpm1_gpio16_set(cpm1_gc, pin_mask, val);
 
 	spin_unlock_irqrestore(&cpm1_gc->lock, flags);
 
@@ -462,9 +456,8 @@ static int cpm1_gpio16_dir_out(struct gpio_chip *gc, unsigned int gpio, int val)
 
 static int cpm1_gpio16_dir_in(struct gpio_chip *gc, unsigned int gpio)
 {
-	struct of_mm_gpio_chip *mm_gc = to_of_mm_gpio_chip(gc);
-	struct cpm1_gpio16_chip *cpm1_gc = gpiochip_get_data(&mm_gc->gc);
-	struct cpm_ioport16 __iomem *iop = mm_gc->regs;
+	struct cpm1_gpio16_chip *cpm1_gc = gpiochip_get_data(gc);
+	struct cpm_ioport16 __iomem *iop = cpm1_gc->regs;
 	unsigned long flags;
 	u16 pin_mask = 1 << (15 - gpio);
 
@@ -481,11 +474,10 @@ int cpm1_gpiochip_add16(struct device *dev)
 {
 	struct device_node *np = dev->of_node;
 	struct cpm1_gpio16_chip *cpm1_gc;
-	struct of_mm_gpio_chip *mm_gc;
 	struct gpio_chip *gc;
 	u16 mask;
 
-	cpm1_gc = kzalloc(sizeof(*cpm1_gc), GFP_KERNEL);
+	cpm1_gc = devm_kzalloc(dev, sizeof(*cpm1_gc), GFP_KERNEL);
 	if (!cpm1_gc)
 		return -ENOMEM;
 
@@ -499,10 +491,8 @@ int cpm1_gpiochip_add16(struct device *dev)
 				cpm1_gc->irq[i] = irq_of_parse_and_map(np, j++);
 	}
 
-	mm_gc = &cpm1_gc->mm_gc;
-	gc = &mm_gc->gc;
-
-	mm_gc->save_regs = cpm1_gpio16_save_regs;
+	gc = &cpm1_gc->gc;
+	gc->base = -1;
 	gc->ngpio = 16;
 	gc->direction_input = cpm1_gpio16_dir_in;
 	gc->direction_output = cpm1_gpio16_dir_out;
@@ -512,30 +502,39 @@ int cpm1_gpiochip_add16(struct device *dev)
 	gc->parent = dev;
 	gc->owner = THIS_MODULE;
 
-	return of_mm_gpiochip_add_data(np, mm_gc, cpm1_gc);
+	g->label = devm_kasprintf(dev, GFP_KERNEL, "%pOF", np);
+	if (!gc->label)
+		return -ENOMEM;
+
+	cpm1_gc->regs = devm_of_iomap(dev, np, 0);
+	if (IS_ERR(cpm1_gc->regs))
+		return PTR_ERR(cpm1_gc->regs);
+
+	cpm1_gpio16_save_regs(cpm1_gc);
+
+	return devm_gpiochip_add_data(dev, gc, cpm1_gc);
 }
 
 struct cpm1_gpio32_chip {
-	struct of_mm_gpio_chip mm_gc;
+	struct gpio_chip gc;
+	void __iomem *regs;
 	spinlock_t lock;
 
 	/* shadowed data register to clear/set bits safely */
 	u32 cpdata;
 };
 
-static void cpm1_gpio32_save_regs(struct of_mm_gpio_chip *mm_gc)
+static void cpm1_gpio32_save_regs(struct cpm1_gpio32_chip *cpm1_gc)
 {
-	struct cpm1_gpio32_chip *cpm1_gc =
-		container_of(mm_gc, struct cpm1_gpio32_chip, mm_gc);
-	struct cpm_ioport32b __iomem *iop = mm_gc->regs;
+	struct cpm_ioport32b __iomem *iop = cpm1_gc->regs;
 
 	cpm1_gc->cpdata = in_be32(&iop->dat);
 }
 
 static int cpm1_gpio32_get(struct gpio_chip *gc, unsigned int gpio)
 {
-	struct of_mm_gpio_chip *mm_gc = to_of_mm_gpio_chip(gc);
-	struct cpm_ioport32b __iomem *iop = mm_gc->regs;
+	struct cpm1_gpio32_chip *cpm1_gc = gpiochip_get_data(gc);
+	struct cpm_ioport32b __iomem *iop = cpm1_gc->regs;
 	u32 pin_mask;
 
 	pin_mask = 1 << (31 - gpio);
@@ -543,11 +542,9 @@ static int cpm1_gpio32_get(struct gpio_chip *gc, unsigned int gpio)
 	return !!(in_be32(&iop->dat) & pin_mask);
 }
 
-static void __cpm1_gpio32_set(struct of_mm_gpio_chip *mm_gc, u32 pin_mask,
-	int value)
+static void __cpm1_gpio32_set(cpm1_gpio32_chip *cpm1_gc, u32 pin_mask, int value)
 {
-	struct cpm1_gpio32_chip *cpm1_gc = gpiochip_get_data(&mm_gc->gc);
-	struct cpm_ioport32b __iomem *iop = mm_gc->regs;
+	struct cpm_ioport32b __iomem *iop = cpm1_gc->regs;
 
 	if (value)
 		cpm1_gc->cpdata |= pin_mask;
@@ -559,30 +556,28 @@ static void __cpm1_gpio32_set(struct of_mm_gpio_chip *mm_gc, u32 pin_mask,
 
 static void cpm1_gpio32_set(struct gpio_chip *gc, unsigned int gpio, int value)
 {
-	struct of_mm_gpio_chip *mm_gc = to_of_mm_gpio_chip(gc);
-	struct cpm1_gpio32_chip *cpm1_gc = gpiochip_get_data(&mm_gc->gc);
+	struct cpm1_gpio32_chip *cpm1_gc = gpiochip_get_data(gc);
 	unsigned long flags;
 	u32 pin_mask = 1 << (31 - gpio);
 
 	spin_lock_irqsave(&cpm1_gc->lock, flags);
 
-	__cpm1_gpio32_set(mm_gc, pin_mask, value);
+	__cpm1_gpio32_set(cpm1_gc, pin_mask, value);
 
 	spin_unlock_irqrestore(&cpm1_gc->lock, flags);
 }
 
 static int cpm1_gpio32_dir_out(struct gpio_chip *gc, unsigned int gpio, int val)
 {
-	struct of_mm_gpio_chip *mm_gc = to_of_mm_gpio_chip(gc);
-	struct cpm1_gpio32_chip *cpm1_gc = gpiochip_get_data(&mm_gc->gc);
-	struct cpm_ioport32b __iomem *iop = mm_gc->regs;
+	struct cpm1_gpio32_chip *cpm1_gc = gpiochip_get_data(gc);
+	struct cpm_ioport32b __iomem *iop = cpm1_gc->regs;
 	unsigned long flags;
 	u32 pin_mask = 1 << (31 - gpio);
 
 	spin_lock_irqsave(&cpm1_gc->lock, flags);
 
 	setbits32(&iop->dir, pin_mask);
-	__cpm1_gpio32_set(mm_gc, pin_mask, val);
+	__cpm1_gpio32_set(cpm1_gc, pin_mask, val);
 
 	spin_unlock_irqrestore(&cpm1_gc->lock, flags);
 
@@ -591,9 +586,8 @@ static int cpm1_gpio32_dir_out(struct gpio_chip *gc, unsigned int gpio, int val)
 
 static int cpm1_gpio32_dir_in(struct gpio_chip *gc, unsigned int gpio)
 {
-	struct of_mm_gpio_chip *mm_gc = to_of_mm_gpio_chip(gc);
-	struct cpm1_gpio32_chip *cpm1_gc = gpiochip_get_data(&mm_gc->gc);
-	struct cpm_ioport32b __iomem *iop = mm_gc->regs;
+	struct cpm1_gpio32_chip *cpm1_gc = gpiochip_get_data(gc);
+	struct cpm_ioport32b __iomem *iop = cpm1_gc->regs;
 	unsigned long flags;
 	u32 pin_mask = 1 << (31 - gpio);
 
@@ -610,19 +604,16 @@ int cpm1_gpiochip_add32(struct device *dev)
 {
 	struct device_node *np = dev->of_node;
 	struct cpm1_gpio32_chip *cpm1_gc;
-	struct of_mm_gpio_chip *mm_gc;
 	struct gpio_chip *gc;
 
-	cpm1_gc = kzalloc(sizeof(*cpm1_gc), GFP_KERNEL);
+	cpm1_gc = devm_kzalloc(dev, sizeof(*cpm1_gc), GFP_KERNEL);
 	if (!cpm1_gc)
 		return -ENOMEM;
 
 	spin_lock_init(&cpm1_gc->lock);
 
-	mm_gc = &cpm1_gc->mm_gc;
-	gc = &mm_gc->gc;
-
-	mm_gc->save_regs = cpm1_gpio32_save_regs;
+	gc = &cpm1_gc->gc;
+	gc->base = -1;
 	gc->ngpio = 32;
 	gc->direction_input = cpm1_gpio32_dir_in;
 	gc->direction_output = cpm1_gpio32_dir_out;
@@ -631,7 +622,17 @@ int cpm1_gpiochip_add32(struct device *dev)
 	gc->parent = dev;
 	gc->owner = THIS_MODULE;
 
-	return of_mm_gpiochip_add_data(np, mm_gc, cpm1_gc);
+	g->label = devm_kasprintf(dev, GFP_KERNEL, "%pOF", np);
+	if (!gc->label)
+		return -ENOMEM;
+
+	cpm1_gc->regs = devm_of_iomap(dev, np, 0);
+	if (IS_ERR(cpm1_gc->regs))
+		return PTR_ERR(cpm1_gc->regs);
+
+	cpm1_gpio32_save_regs(cpm1_gc);
+
+	return devm_gpiochip_add_data(dev, gc, cpm1_gc);
 }
 
 #endif /* CONFIG_8xx_GPIO */
-- 
2.43.0.rc1.1336.g36b5255a03ac

Re: [PATCH v1 1/1] powerpc/8xx: Drop legacy-of-mm-gpiochip.h header

[Christophe Leroy]

Le 15/11/2024 à 14:38, Andy Shevchenko a écrit :
> Remove legacy-of-mm-gpiochip.h header file, replace of_* functions
> and structs with appropriate alternatives.

Looks like you don't really have an alternative to 
of_mm_gpiochip_add_data(), you are replacing one single line by 11 new 
ones, and that is done twice (once for cpm1_gpiochip_add16(), once for 
cpm1_gpiochip_add32()).

> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>   arch/powerpc/platforms/8xx/cpm1.c | 119 +++++++++++++++---------------

Does not build:

   CC      arch/powerpc/platforms/8xx/cpm1.o
arch/powerpc/platforms/8xx/cpm1.c: In function 'cpm1_gpiochip_add16':
arch/powerpc/platforms/8xx/cpm1.c:505:2: error: 'g' undeclared (first 
use in this function); did you mean 'gc'?
   g->label = devm_kasprintf(dev, GFP_KERNEL, "%pOF", np);
   ^
   gc
arch/powerpc/platforms/8xx/cpm1.c:505:2: note: each undeclared 
identifier is reported only once for each function it appears in
arch/powerpc/platforms/8xx/cpm1.c:509:18: error: too few arguments to 
function 'devm_of_iomap'
   cpm1_gc->regs = devm_of_iomap(dev, np, 0);
                   ^~~~~~~~~~~~~
In file included from ./include/linux/dma-mapping.h:8:0,
                  from arch/powerpc/platforms/8xx/cpm1.c:27:
./include/linux/device.h:372:15: note: declared here
  void __iomem *devm_of_iomap(struct device *dev,
                ^~~~~~~~~~~~~
arch/powerpc/platforms/8xx/cpm1.c: At top level:
arch/powerpc/platforms/8xx/cpm1.c:545:31: error: unknown type name 
'cpm1_gpio32_chip'
  static void __cpm1_gpio32_set(cpm1_gpio32_chip *cpm1_gc, u32 pin_mask, 
int value)
                                ^~~~~~~~~~~~~~~~
arch/powerpc/platforms/8xx/cpm1.c: In function 'cpm1_gpio32_set':
arch/powerpc/platforms/8xx/cpm1.c:565:2: error: implicit declaration of 
function '__cpm1_gpio32_set'; did you mean 'cpm1_gpio32_set'? 
[-Werror=implicit-function-declaration]
   __cpm1_gpio32_set(cpm1_gc, pin_mask, value);
   ^~~~~~~~~~~~~~~~~
   cpm1_gpio32_set
arch/powerpc/platforms/8xx/cpm1.c: In function 'cpm1_gpiochip_add32':
arch/powerpc/platforms/8xx/cpm1.c:625:2: error: 'g' undeclared (first 
use in this function); did you mean 'gc'?
   g->label = devm_kasprintf(dev, GFP_KERNEL, "%pOF", np);
   ^
   gc
arch/powerpc/platforms/8xx/cpm1.c:629:18: error: too few arguments to 
function 'devm_of_iomap'
   cpm1_gc->regs = devm_of_iomap(dev, np, 0);
                   ^~~~~~~~~~~~~
In file included from ./include/linux/dma-mapping.h:8:0,
                  from arch/powerpc/platforms/8xx/cpm1.c:27:
./include/linux/device.h:372:15: note: declared here
  void __iomem *devm_of_iomap(struct device *dev,
                ^~~~~~~~~~~~~



>   1 file changed, 60 insertions(+), 59 deletions(-)
> 
> diff --git a/arch/powerpc/platforms/8xx/cpm1.c b/arch/powerpc/platforms/8xx/cpm1.c
> index b24d4102fbf6..1262bff5ba2e 100644
> --- a/arch/powerpc/platforms/8xx/cpm1.c
> +++ b/arch/powerpc/platforms/8xx/cpm1.c
> @@ -45,7 +45,7 @@
>   #include <sysdev/fsl_soc.h>
>   
>   #ifdef CONFIG_8xx_GPIO
> -#include <linux/gpio/legacy-of-mm-gpiochip.h>
> +#include <linux/gpio/driver.h>
>   #endif
>   
>   #define CPM_MAP_SIZE    (0x4000)
> @@ -376,7 +376,8 @@ int __init cpm1_clk_setup(enum cpm_clk_target target, int clock, int mode)
>   #ifdef CONFIG_8xx_GPIO
>   
>   struct cpm1_gpio16_chip {
> -	struct of_mm_gpio_chip mm_gc;
> +	struct gpio_chip gc;
> +	void __iomem *regs;
>   	spinlock_t lock;
>   
>   	/* shadowed data register to clear/set bits safely */
> @@ -386,19 +387,17 @@ struct cpm1_gpio16_chip {
>   	int irq[16];
>   };
>   
> -static void cpm1_gpio16_save_regs(struct of_mm_gpio_chip *mm_gc)
> +static void cpm1_gpio16_save_regs(struct cpm1_gpio16_chip *cpm1_gc)
>   {
> -	struct cpm1_gpio16_chip *cpm1_gc =
> -		container_of(mm_gc, struct cpm1_gpio16_chip, mm_gc);
> -	struct cpm_ioport16 __iomem *iop = mm_gc->regs;
> +	struct cpm_ioport16 __iomem *iop = cpm1_gc->regs;
>   
>   	cpm1_gc->cpdata = in_be16(&iop->dat);
>   }
>   
>   static int cpm1_gpio16_get(struct gpio_chip *gc, unsigned int gpio)
>   {
> -	struct of_mm_gpio_chip *mm_gc = to_of_mm_gpio_chip(gc);
> -	struct cpm_ioport16 __iomem *iop = mm_gc->regs;
> +	struct cpm1_gpio16_chip *cpm1_gc = gpiochip_get_data(gc);
> +	struct cpm_ioport16 __iomem *iop = cpm1_gc->regs;
>   	u16 pin_mask;
>   
>   	pin_mask = 1 << (15 - gpio);
> @@ -406,11 +405,9 @@ static int cpm1_gpio16_get(struct gpio_chip *gc, unsigned int gpio)
>   	return !!(in_be16(&iop->dat) & pin_mask);
>   }
>   
> -static void __cpm1_gpio16_set(struct of_mm_gpio_chip *mm_gc, u16 pin_mask,
> -	int value)
> +static void __cpm1_gpio16_set(struct cpm1_gpio16_chip *cpm1_gc, u16 pin_mask, int value)
>   {
> -	struct cpm1_gpio16_chip *cpm1_gc = gpiochip_get_data(&mm_gc->gc);
> -	struct cpm_ioport16 __iomem *iop = mm_gc->regs;
> +	struct cpm_ioport16 __iomem *iop = cpm1_gc->regs;
>   
>   	if (value)
>   		cpm1_gc->cpdata |= pin_mask;
> @@ -422,38 +419,35 @@ static void __cpm1_gpio16_set(struct of_mm_gpio_chip *mm_gc, u16 pin_mask,
>   
>   static void cpm1_gpio16_set(struct gpio_chip *gc, unsigned int gpio, int value)
>   {
> -	struct of_mm_gpio_chip *mm_gc = to_of_mm_gpio_chip(gc);
> -	struct cpm1_gpio16_chip *cpm1_gc = gpiochip_get_data(&mm_gc->gc);
> +	struct cpm1_gpio16_chip *cpm1_gc = gpiochip_get_data(gc);
>   	unsigned long flags;
>   	u16 pin_mask = 1 << (15 - gpio);
>   
>   	spin_lock_irqsave(&cpm1_gc->lock, flags);
>   
> -	__cpm1_gpio16_set(mm_gc, pin_mask, value);
> +	__cpm1_gpio16_set(cpm1_gc, pin_mask, value);
>   
>   	spin_unlock_irqrestore(&cpm1_gc->lock, flags);
>   }
>   
>   static int cpm1_gpio16_to_irq(struct gpio_chip *gc, unsigned int gpio)
>   {
> -	struct of_mm_gpio_chip *mm_gc = to_of_mm_gpio_chip(gc);
> -	struct cpm1_gpio16_chip *cpm1_gc = gpiochip_get_data(&mm_gc->gc);
> +	struct cpm1_gpio16_chip *cpm1_gc = gpiochip_get_data(gc);
>   
>   	return cpm1_gc->irq[gpio] ? : -ENXIO;
>   }
>   
>   static int cpm1_gpio16_dir_out(struct gpio_chip *gc, unsigned int gpio, int val)
>   {
> -	struct of_mm_gpio_chip *mm_gc = to_of_mm_gpio_chip(gc);
> -	struct cpm1_gpio16_chip *cpm1_gc = gpiochip_get_data(&mm_gc->gc);
> -	struct cpm_ioport16 __iomem *iop = mm_gc->regs;
> +	struct cpm1_gpio16_chip *cpm1_gc = gpiochip_get_data(gc);
> +	struct cpm_ioport16 __iomem *iop = cpm1_gc->regs;
>   	unsigned long flags;
>   	u16 pin_mask = 1 << (15 - gpio);
>   
>   	spin_lock_irqsave(&cpm1_gc->lock, flags);
>   
>   	setbits16(&iop->dir, pin_mask);
> -	__cpm1_gpio16_set(mm_gc, pin_mask, val);
> +	__cpm1_gpio16_set(cpm1_gc, pin_mask, val);
>   
>   	spin_unlock_irqrestore(&cpm1_gc->lock, flags);
>   
> @@ -462,9 +456,8 @@ static int cpm1_gpio16_dir_out(struct gpio_chip *gc, unsigned int gpio, int val)
>   
>   static int cpm1_gpio16_dir_in(struct gpio_chip *gc, unsigned int gpio)
>   {
> -	struct of_mm_gpio_chip *mm_gc = to_of_mm_gpio_chip(gc);
> -	struct cpm1_gpio16_chip *cpm1_gc = gpiochip_get_data(&mm_gc->gc);
> -	struct cpm_ioport16 __iomem *iop = mm_gc->regs;
> +	struct cpm1_gpio16_chip *cpm1_gc = gpiochip_get_data(gc);
> +	struct cpm_ioport16 __iomem *iop = cpm1_gc->regs;
>   	unsigned long flags;
>   	u16 pin_mask = 1 << (15 - gpio);
>   
> @@ -481,11 +474,10 @@ int cpm1_gpiochip_add16(struct device *dev)
>   {
>   	struct device_node *np = dev->of_node;
>   	struct cpm1_gpio16_chip *cpm1_gc;
> -	struct of_mm_gpio_chip *mm_gc;
>   	struct gpio_chip *gc;
>   	u16 mask;
>   
> -	cpm1_gc = kzalloc(sizeof(*cpm1_gc), GFP_KERNEL);
> +	cpm1_gc = devm_kzalloc(dev, sizeof(*cpm1_gc), GFP_KERNEL);
>   	if (!cpm1_gc)
>   		return -ENOMEM;
>   
> @@ -499,10 +491,8 @@ int cpm1_gpiochip_add16(struct device *dev)
>   				cpm1_gc->irq[i] = irq_of_parse_and_map(np, j++);
>   	}
>   
> -	mm_gc = &cpm1_gc->mm_gc;
> -	gc = &mm_gc->gc;
> -
> -	mm_gc->save_regs = cpm1_gpio16_save_regs;
> +	gc = &cpm1_gc->gc;
> +	gc->base = -1;
>   	gc->ngpio = 16;
>   	gc->direction_input = cpm1_gpio16_dir_in;
>   	gc->direction_output = cpm1_gpio16_dir_out;
> @@ -512,30 +502,39 @@ int cpm1_gpiochip_add16(struct device *dev)
>   	gc->parent = dev;
>   	gc->owner = THIS_MODULE;
>   
> -	return of_mm_gpiochip_add_data(np, mm_gc, cpm1_gc);
> +	g->label = devm_kasprintf(dev, GFP_KERNEL, "%pOF", np);
> +	if (!gc->label)
> +		return -ENOMEM;
> +
> +	cpm1_gc->regs = devm_of_iomap(dev, np, 0);
> +	if (IS_ERR(cpm1_gc->regs))
> +		return PTR_ERR(cpm1_gc->regs);
> +
> +	cpm1_gpio16_save_regs(cpm1_gc);
> +
> +	return devm_gpiochip_add_data(dev, gc, cpm1_gc);
>   }
>   
>   struct cpm1_gpio32_chip {
> -	struct of_mm_gpio_chip mm_gc;
> +	struct gpio_chip gc;
> +	void __iomem *regs;
>   	spinlock_t lock;
>   
>   	/* shadowed data register to clear/set bits safely */
>   	u32 cpdata;
>   };
>   
> -static void cpm1_gpio32_save_regs(struct of_mm_gpio_chip *mm_gc)
> +static void cpm1_gpio32_save_regs(struct cpm1_gpio32_chip *cpm1_gc)
>   {
> -	struct cpm1_gpio32_chip *cpm1_gc =
> -		container_of(mm_gc, struct cpm1_gpio32_chip, mm_gc);
> -	struct cpm_ioport32b __iomem *iop = mm_gc->regs;
> +	struct cpm_ioport32b __iomem *iop = cpm1_gc->regs;
>   
>   	cpm1_gc->cpdata = in_be32(&iop->dat);
>   }
>   
>   static int cpm1_gpio32_get(struct gpio_chip *gc, unsigned int gpio)
>   {
> -	struct of_mm_gpio_chip *mm_gc = to_of_mm_gpio_chip(gc);
> -	struct cpm_ioport32b __iomem *iop = mm_gc->regs;
> +	struct cpm1_gpio32_chip *cpm1_gc = gpiochip_get_data(gc);
> +	struct cpm_ioport32b __iomem *iop = cpm1_gc->regs;
>   	u32 pin_mask;
>   
>   	pin_mask = 1 << (31 - gpio);
> @@ -543,11 +542,9 @@ static int cpm1_gpio32_get(struct gpio_chip *gc, unsigned int gpio)
>   	return !!(in_be32(&iop->dat) & pin_mask);
>   }
>   
> -static void __cpm1_gpio32_set(struct of_mm_gpio_chip *mm_gc, u32 pin_mask,
> -	int value)
> +static void __cpm1_gpio32_set(cpm1_gpio32_chip *cpm1_gc, u32 pin_mask, int value)
>   {
> -	struct cpm1_gpio32_chip *cpm1_gc = gpiochip_get_data(&mm_gc->gc);
> -	struct cpm_ioport32b __iomem *iop = mm_gc->regs;
> +	struct cpm_ioport32b __iomem *iop = cpm1_gc->regs;
>   
>   	if (value)
>   		cpm1_gc->cpdata |= pin_mask;
> @@ -559,30 +556,28 @@ static void __cpm1_gpio32_set(struct of_mm_gpio_chip *mm_gc, u32 pin_mask,
>   
>   static void cpm1_gpio32_set(struct gpio_chip *gc, unsigned int gpio, int value)
>   {
> -	struct of_mm_gpio_chip *mm_gc = to_of_mm_gpio_chip(gc);
> -	struct cpm1_gpio32_chip *cpm1_gc = gpiochip_get_data(&mm_gc->gc);
> +	struct cpm1_gpio32_chip *cpm1_gc = gpiochip_get_data(gc);
>   	unsigned long flags;
>   	u32 pin_mask = 1 << (31 - gpio);
>   
>   	spin_lock_irqsave(&cpm1_gc->lock, flags);
>   
> -	__cpm1_gpio32_set(mm_gc, pin_mask, value);
> +	__cpm1_gpio32_set(cpm1_gc, pin_mask, value);
>   
>   	spin_unlock_irqrestore(&cpm1_gc->lock, flags);
>   }
>   
>   static int cpm1_gpio32_dir_out(struct gpio_chip *gc, unsigned int gpio, int val)
>   {
> -	struct of_mm_gpio_chip *mm_gc = to_of_mm_gpio_chip(gc);
> -	struct cpm1_gpio32_chip *cpm1_gc = gpiochip_get_data(&mm_gc->gc);
> -	struct cpm_ioport32b __iomem *iop = mm_gc->regs;
> +	struct cpm1_gpio32_chip *cpm1_gc = gpiochip_get_data(gc);
> +	struct cpm_ioport32b __iomem *iop = cpm1_gc->regs;
>   	unsigned long flags;
>   	u32 pin_mask = 1 << (31 - gpio);
>   
>   	spin_lock_irqsave(&cpm1_gc->lock, flags);
>   
>   	setbits32(&iop->dir, pin_mask);
> -	__cpm1_gpio32_set(mm_gc, pin_mask, val);
> +	__cpm1_gpio32_set(cpm1_gc, pin_mask, val);
>   
>   	spin_unlock_irqrestore(&cpm1_gc->lock, flags);
>   
> @@ -591,9 +586,8 @@ static int cpm1_gpio32_dir_out(struct gpio_chip *gc, unsigned int gpio, int val)
>   
>   static int cpm1_gpio32_dir_in(struct gpio_chip *gc, unsigned int gpio)
>   {
> -	struct of_mm_gpio_chip *mm_gc = to_of_mm_gpio_chip(gc);
> -	struct cpm1_gpio32_chip *cpm1_gc = gpiochip_get_data(&mm_gc->gc);
> -	struct cpm_ioport32b __iomem *iop = mm_gc->regs;
> +	struct cpm1_gpio32_chip *cpm1_gc = gpiochip_get_data(gc);
> +	struct cpm_ioport32b __iomem *iop = cpm1_gc->regs;
>   	unsigned long flags;
>   	u32 pin_mask = 1 << (31 - gpio);
>   
> @@ -610,19 +604,16 @@ int cpm1_gpiochip_add32(struct device *dev)
>   {
>   	struct device_node *np = dev->of_node;
>   	struct cpm1_gpio32_chip *cpm1_gc;
> -	struct of_mm_gpio_chip *mm_gc;
>   	struct gpio_chip *gc;
>   
> -	cpm1_gc = kzalloc(sizeof(*cpm1_gc), GFP_KERNEL);
> +	cpm1_gc = devm_kzalloc(dev, sizeof(*cpm1_gc), GFP_KERNEL);
>   	if (!cpm1_gc)
>   		return -ENOMEM;
>   
>   	spin_lock_init(&cpm1_gc->lock);
>   
> -	mm_gc = &cpm1_gc->mm_gc;
> -	gc = &mm_gc->gc;
> -
> -	mm_gc->save_regs = cpm1_gpio32_save_regs;
> +	gc = &cpm1_gc->gc;
> +	gc->base = -1;
>   	gc->ngpio = 32;
>   	gc->direction_input = cpm1_gpio32_dir_in;
>   	gc->direction_output = cpm1_gpio32_dir_out;
> @@ -631,7 +622,17 @@ int cpm1_gpiochip_add32(struct device *dev)
>   	gc->parent = dev;
>   	gc->owner = THIS_MODULE;
>   
> -	return of_mm_gpiochip_add_data(np, mm_gc, cpm1_gc);
> +	g->label = devm_kasprintf(dev, GFP_KERNEL, "%pOF", np);
> +	if (!gc->label)
> +		return -ENOMEM;
> +
> +	cpm1_gc->regs = devm_of_iomap(dev, np, 0);
> +	if (IS_ERR(cpm1_gc->regs))
> +		return PTR_ERR(cpm1_gc->regs);
> +
> +	cpm1_gpio32_save_regs(cpm1_gc);
> +
> +	return devm_gpiochip_add_data(dev, gc, cpm1_gc);
>   }
>   
>   #endif /* CONFIG_8xx_GPIO */

Re: [PATCH v1 1/1] powerpc/8xx: Drop legacy-of-mm-gpiochip.h header

[Andy Shevchenko]

On Sat, Nov 16, 2024 at 11:44:35AM +0100, Christophe Leroy wrote:
> Le 15/11/2024 à 14:38, Andy Shevchenko a écrit :
> > Remove legacy-of-mm-gpiochip.h header file, replace of_* functions
> > and structs with appropriate alternatives.
> 
> Looks like you don't really have an alternative to
> of_mm_gpiochip_add_data(), you are replacing one single line by 11 new ones,
> and that is done twice (once for cpm1_gpiochip_add16(), once for
> cpm1_gpiochip_add32()).

True, but that's the issue that we have of_specific API. If someone propose
the common API for the agnostic approach,  it would be nice, but I am not
the one. And TBH I do not see the advantage of it right now as almost every
GPIO driver is using its own labeling schema (*). Note, that this patch also
fixes a memory leak as a side effect.

*) the legacy API is mostly used by PPC code, do you want that of_mm_* thingy
to be moved to PPC specific code instead of killing it? Would be done this
way as well.

> > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > ---
> >   arch/powerpc/platforms/8xx/cpm1.c | 119 +++++++++++++++---------------
> 
> Does not build:

Crap, I most likely built something else and not these files...
I have carefully build-test this for v2.

Note to myself: Never ever send the patches on Friday evenings! :-)

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 1/1] powerpc/8xx: Drop legacy-of-mm-gpiochip.h header

[Christophe Leroy]

Le 18/11/2024 à 10:20, Andy Shevchenko a écrit :
> On Sat, Nov 16, 2024 at 11:44:35AM +0100, Christophe Leroy wrote:
>> Le 15/11/2024 à 14:38, Andy Shevchenko a écrit :
>>> Remove legacy-of-mm-gpiochip.h header file, replace of_* functions
>>> and structs with appropriate alternatives.
>>
>> Looks like you don't really have an alternative to
>> of_mm_gpiochip_add_data(), you are replacing one single line by 11 new ones,
>> and that is done twice (once for cpm1_gpiochip_add16(), once for
>> cpm1_gpiochip_add32()).
> 
> True, but that's the issue that we have of_specific API. If someone propose
> the common API for the agnostic approach,  it would be nice, but I am not
> the one. And TBH I do not see the advantage of it right now as almost every
> GPIO driver is using its own labeling schema (*). Note, that this patch also
> fixes a memory leak as a side effect.

Can you explain that in the commit message instead of saying you use 
appropriate alternatives that do not exist ?

Don't hesitate to mention the memory leak it in the commit message as well.

> 
> *) the legacy API is mostly used by PPC code, do you want that of_mm_* thingy
> to be moved to PPC specific code instead of killing it? Would be done this
> way as well.

No no, your change is ok for me, just need an accurate commit message.

> 
>>> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
>>> ---
>>>    arch/powerpc/platforms/8xx/cpm1.c | 119 +++++++++++++++---------------
>>
>> Does not build:
> 
> Crap, I most likely built something else and not these files...
> I have carefully build-test this for v2.

Just use mpc885_ads_defconfig

> 
> Note to myself: Never ever send the patches on Friday evenings! :-)
> 

Christophe

Re: [PATCH v1 1/1] powerpc/8xx: Drop legacy-of-mm-gpiochip.h header

[Andy Shevchenko]

On Mon, Nov 18, 2024 at 12:02:44PM +0100, Christophe Leroy wrote:
> Le 18/11/2024 à 10:20, Andy Shevchenko a écrit :
> > On Sat, Nov 16, 2024 at 11:44:35AM +0100, Christophe Leroy wrote:
> > > Le 15/11/2024 à 14:38, Andy Shevchenko a écrit :
> > > > Remove legacy-of-mm-gpiochip.h header file, replace of_* functions
> > > > and structs with appropriate alternatives.
> > > 
> > > Looks like you don't really have an alternative to
> > > of_mm_gpiochip_add_data(), you are replacing one single line by 11 new ones,
> > > and that is done twice (once for cpm1_gpiochip_add16(), once for
> > > cpm1_gpiochip_add32()).

Note, the overall statistics is +1 LoC only!

> > True, but that's the issue that we have of_specific API. If someone propose
> > the common API for the agnostic approach,  it would be nice, but I am not
> > the one. And TBH I do not see the advantage of it right now as almost every
> > GPIO driver is using its own labeling schema (*). Note, that this patch also
> > fixes a memory leak as a side effect.
> 
> Can you explain that in the commit message instead of saying you use
> appropriate alternatives that do not exist ?
> 
> Don't hesitate to mention the memory leak it in the commit message as well.
> 
> > *) the legacy API is mostly used by PPC code, do you want that of_mm_* thingy
> > to be moved to PPC specific code instead of killing it? Would be done this
> > way as well.
> 
> No no, your change is ok for me, just need an accurate commit message.

Fine, just sent a v2, which is now compile-tested as suggested below.

...

> > > Does not build:
> > 
> > Crap, I most likely built something else and not these files...
> > I have carefully build-test this for v2.
> 
> Just use mpc885_ads_defconfig

Thanks, that is helpful!

-- 
With Best Regards,
Andy Shevchenko