diff mbox series

[v4,05/13] pinctrl: samsung: Switch to use for_each_gpiochip_node() helper

Message ID 20220401103604.8705-6-andriy.shevchenko@linux.intel.com (mailing list archive)
State Under Review
Headers show
Series gpiolib: Two new helpers and way toward fwnode | expand

Commit Message

Andy Shevchenko April 1, 2022, 10:35 a.m. UTC
Switch the code to use for_each_gpiochip_node() helper.

While at it, in order to avoid additional churn in the future,
switch to fwnode APIs where it makes sense.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/pinctrl/samsung/pinctrl-exynos.c  |  8 +++---
 drivers/pinctrl/samsung/pinctrl-s3c24xx.c |  2 +-
 drivers/pinctrl/samsung/pinctrl-s3c64xx.c |  4 +--
 drivers/pinctrl/samsung/pinctrl-samsung.c | 30 +++++++++++------------
 drivers/pinctrl/samsung/pinctrl-samsung.h |  2 +-
 5 files changed, 22 insertions(+), 24 deletions(-)

Comments

Krzysztof Kozlowski April 8, 2022, 3:22 p.m. UTC | #1
On 01/04/2022 12:35, Andy Shevchenko wrote:
> Switch the code to use for_each_gpiochip_node() helper.
> 

(...)

>  /*
>   * Iterate over all driver pin banks to find one matching the name of node,
>   * skipping optional "-gpio" node suffix. When found, assign node to the bank.
>   */
> -static void samsung_banks_of_node_get(struct device *dev,
> -				      struct samsung_pinctrl_drv_data *d,
> -				      struct device_node *node)
> +static void samsung_banks_node_get(struct device *dev, struct samsung_pinctrl_drv_data *d)

This is worth simplification anyway, so please split it to separate patch.

>  {
>  	const char *suffix = "-gpio-bank";
>  	struct samsung_pin_bank *bank;
> -	struct device_node *child;
> +	struct fwnode_handle *child;
>  	/* Pin bank names are up to 4 characters */
>  	char node_name[20];
>  	unsigned int i;
> @@ -1038,17 +1037,17 @@ static void samsung_banks_of_node_get(struct device *dev,
>  			continue;
>  		}
>  
> -		for_each_child_of_node(node, child) {
> -			if (!of_find_property(child, "gpio-controller", NULL))
> -				continue;

This does not look equivalent. There are nodes without this property.

> -			if (of_node_name_eq(child, node_name))
> +		for_each_gpiochip_node(dev, child) {
> +			struct device_node *np = to_of_node(child);
> +
> +			if (of_node_name_eq(np, node_name))
>  				break;
> -			else if (of_node_name_eq(child, bank->name))
> +			if (of_node_name_eq(np, bank->name))
>  				break;
>  		}

This patch has to wait till someone provides you a tested-by. I might do
it around next week.

Best regards,
Krzysztof
Andy Shevchenko April 8, 2022, 3:39 p.m. UTC | #2
On Fri, Apr 08, 2022 at 05:22:21PM +0200, Krzysztof Kozlowski wrote:
> On 01/04/2022 12:35, Andy Shevchenko wrote:
> > Switch the code to use for_each_gpiochip_node() helper.

(...)

> >  /*
> >   * Iterate over all driver pin banks to find one matching the name of node,
> >   * skipping optional "-gpio" node suffix. When found, assign node to the bank.
> >   */
> > -static void samsung_banks_of_node_get(struct device *dev,
> > -				      struct samsung_pinctrl_drv_data *d,
> > -				      struct device_node *node)
> > +static void samsung_banks_node_get(struct device *dev, struct samsung_pinctrl_drv_data *d)
> 
> This is worth simplification anyway, so please split it to separate patch.

Not sure what to do and why it worth an additional churn.

> >  {
> >  	const char *suffix = "-gpio-bank";
> >  	struct samsung_pin_bank *bank;
> > -	struct device_node *child;
> > +	struct fwnode_handle *child;
> >  	/* Pin bank names are up to 4 characters */
> >  	char node_name[20];
> >  	unsigned int i;
> > @@ -1038,17 +1037,17 @@ static void samsung_banks_of_node_get(struct device *dev,
> >  			continue;
> >  		}
> >  
> > -		for_each_child_of_node(node, child) {
> > -			if (!of_find_property(child, "gpio-controller", NULL))
> > -				continue;
> 
> This does not look equivalent. There are nodes without this property.

Not sure I understand why not. The macro checks for the property and
iterates over nodes that have this property.

Can you elaborate, please?

> > -			if (of_node_name_eq(child, node_name))
> > +		for_each_gpiochip_node(dev, child) {
> > +			struct device_node *np = to_of_node(child);
> > +
> > +			if (of_node_name_eq(np, node_name))
> >  				break;
> > -			else if (of_node_name_eq(child, bank->name))
> > +			if (of_node_name_eq(np, bank->name))
> >  				break;
> >  		}
> 
> This patch has to wait till someone provides you a tested-by. I might do
> it around next week.

Fine with me, I will drop it from my repo for now.

Thanks for review!
Krzysztof Kozlowski April 9, 2022, 1:33 p.m. UTC | #3
On 08/04/2022 17:39, Andy Shevchenko wrote:
> On Fri, Apr 08, 2022 at 05:22:21PM +0200, Krzysztof Kozlowski wrote:
>> On 01/04/2022 12:35, Andy Shevchenko wrote:
>>> Switch the code to use for_each_gpiochip_node() helper.
> 
> (...)
> 
>>>  /*
>>>   * Iterate over all driver pin banks to find one matching the name of node,
>>>   * skipping optional "-gpio" node suffix. When found, assign node to the bank.
>>>   */
>>> -static void samsung_banks_of_node_get(struct device *dev,
>>> -				      struct samsung_pinctrl_drv_data *d,
>>> -				      struct device_node *node)
>>> +static void samsung_banks_node_get(struct device *dev, struct samsung_pinctrl_drv_data *d)
>>
>> This is worth simplification anyway, so please split it to separate patch.
> 
> Not sure what to do and why it worth an additional churn.

Makes this change smaller so it's easier to review.

> 
>>>  {
>>>  	const char *suffix = "-gpio-bank";
>>>  	struct samsung_pin_bank *bank;
>>> -	struct device_node *child;
>>> +	struct fwnode_handle *child;
>>>  	/* Pin bank names are up to 4 characters */
>>>  	char node_name[20];
>>>  	unsigned int i;
>>> @@ -1038,17 +1037,17 @@ static void samsung_banks_of_node_get(struct device *dev,
>>>  			continue;
>>>  		}
>>>  
>>> -		for_each_child_of_node(node, child) {
>>> -			if (!of_find_property(child, "gpio-controller", NULL))
>>> -				continue;
>>
>> This does not look equivalent. There are nodes without this property.
> 
> Not sure I understand why not. The macro checks for the property and
> iterates over nodes that have this property.
> 
> Can you elaborate, please?

Eh, my bad, it is equivalent.

> 
>>> -			if (of_node_name_eq(child, node_name))
>>> +		for_each_gpiochip_node(dev, child) {
>>> +			struct device_node *np = to_of_node(child);
>>> +
>>> +			if (of_node_name_eq(np, node_name))
>>>  				break;
>>> -			else if (of_node_name_eq(child, bank->name))
>>> +			if (of_node_name_eq(np, bank->name))
>>>  				break;
>>>  		}
>>
>> This patch has to wait till someone provides you a tested-by. I might do
>> it around next week.
> 
> Fine with me, I will drop it from my repo for now.


Best regards,
Krzysztof
Andy Shevchenko April 11, 2022, 11:56 a.m. UTC | #4
On Sat, Apr 09, 2022 at 03:33:49PM +0200, Krzysztof Kozlowski wrote:
> On 08/04/2022 17:39, Andy Shevchenko wrote:
> > On Fri, Apr 08, 2022 at 05:22:21PM +0200, Krzysztof Kozlowski wrote:
> >> On 01/04/2022 12:35, Andy Shevchenko wrote:
> >>> Switch the code to use for_each_gpiochip_node() helper.
> > 
> > (...)
> > 
> >>>  /*
> >>>   * Iterate over all driver pin banks to find one matching the name of node,
> >>>   * skipping optional "-gpio" node suffix. When found, assign node to the bank.
> >>>   */
> >>> -static void samsung_banks_of_node_get(struct device *dev,
> >>> -				      struct samsung_pinctrl_drv_data *d,
> >>> -				      struct device_node *node)
> >>> +static void samsung_banks_node_get(struct device *dev, struct samsung_pinctrl_drv_data *d)
> >>
> >> This is worth simplification anyway, so please split it to separate patch.
> > 
> > Not sure what to do and why it worth an additional churn.
> 
> Makes this change smaller so it's easier to review.

https://git.kernel.org/pub/scm/linux/kernel/git/andy/linux-gpio-intel.git/log/?h=review-andy

That's how it looks like. Tell me if it is what you have had in mind.
Krzysztof Kozlowski April 11, 2022, 12:03 p.m. UTC | #5
On 11/04/2022 13:56, Andy Shevchenko wrote:
>>
>> Makes this change smaller so it's easier to review.
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/andy/linux-gpio-intel.git/log/?h=review-andy
> 
> That's how it looks like. Tell me if it is what you have had in mind.

Yes, thanks.


Best regards,
Krzysztof
Krzysztof Kozlowski April 11, 2022, 12:21 p.m. UTC | #6
On 01/04/2022 12:35, Andy Shevchenko wrote:
> Switch the code to use for_each_gpiochip_node() helper.
> 
> While at it, in order to avoid additional churn in the future,
> switch to fwnode APIs where it makes sense.
> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>  drivers/pinctrl/samsung/pinctrl-exynos.c  |  8 +++---
>  drivers/pinctrl/samsung/pinctrl-s3c24xx.c |  2 +-
>  drivers/pinctrl/samsung/pinctrl-s3c64xx.c |  4 +--
>  drivers/pinctrl/samsung/pinctrl-samsung.c | 30 +++++++++++------------
>  drivers/pinctrl/samsung/pinctrl-samsung.h |  2 +-
>  5 files changed, 22 insertions(+), 24 deletions(-)
> 


Tested-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>


Best regards,
Krzysztof
Andy Shevchenko April 11, 2022, 1:13 p.m. UTC | #7
On Mon, Apr 11, 2022 at 02:21:00PM +0200, Krzysztof Kozlowski wrote:
> On 01/04/2022 12:35, Andy Shevchenko wrote:
> > Switch the code to use for_each_gpiochip_node() helper.
> > 
> > While at it, in order to avoid additional churn in the future,
> > switch to fwnode APIs where it makes sense.
> > 
> > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > ---
> >  drivers/pinctrl/samsung/pinctrl-exynos.c  |  8 +++---
> >  drivers/pinctrl/samsung/pinctrl-s3c24xx.c |  2 +-
> >  drivers/pinctrl/samsung/pinctrl-s3c64xx.c |  4 +--
> >  drivers/pinctrl/samsung/pinctrl-samsung.c | 30 +++++++++++------------
> >  drivers/pinctrl/samsung/pinctrl-samsung.h |  2 +-
> >  5 files changed, 22 insertions(+), 24 deletions(-)
> 
> Tested-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>

Thanks!
I'm going to spread this to two patches to which I pointed out before.
diff mbox series

Patch

diff --git a/drivers/pinctrl/samsung/pinctrl-exynos.c b/drivers/pinctrl/samsung/pinctrl-exynos.c
index c1c4ffbae6e2..6d7ca1758292 100644
--- a/drivers/pinctrl/samsung/pinctrl-exynos.c
+++ b/drivers/pinctrl/samsung/pinctrl-exynos.c
@@ -307,7 +307,7 @@  __init int exynos_eint_gpio_init(struct samsung_pinctrl_drv_data *d)
 		}
 		bank->irq_chip->chip.name = bank->name;
 
-		bank->irq_domain = irq_domain_add_linear(bank->of_node,
+		bank->irq_domain = irq_domain_create_linear(bank->fwnode,
 				bank->nr_pins, &exynos_eint_irqd_ops, bank);
 		if (!bank->irq_domain) {
 			dev_err(dev, "gpio irq domain add failed\n");
@@ -565,7 +565,7 @@  __init int exynos_eint_wkup_init(struct samsung_pinctrl_drv_data *d)
 		}
 		bank->irq_chip->chip.name = bank->name;
 
-		bank->irq_domain = irq_domain_add_linear(bank->of_node,
+		bank->irq_domain = irq_domain_create_linear(bank->fwnode,
 				bank->nr_pins, &exynos_eint_irqd_ops, bank);
 		if (!bank->irq_domain) {
 			dev_err(dev, "wkup irq domain add failed\n");
@@ -573,7 +573,7 @@  __init int exynos_eint_wkup_init(struct samsung_pinctrl_drv_data *d)
 			return -ENXIO;
 		}
 
-		if (!of_find_property(bank->of_node, "interrupts", NULL)) {
+		if (!fwnode_property_present(bank->fwnode, "interrupts")) {
 			bank->eint_type = EINT_TYPE_WKUP_MUX;
 			++muxed_banks;
 			continue;
@@ -588,7 +588,7 @@  __init int exynos_eint_wkup_init(struct samsung_pinctrl_drv_data *d)
 		}
 
 		for (idx = 0; idx < bank->nr_pins; ++idx) {
-			irq = irq_of_parse_and_map(bank->of_node, idx);
+			irq = irq_of_parse_and_map(to_of_node(bank->fwnode), idx);
 			if (!irq) {
 				dev_err(dev, "irq number for eint-%s-%d not found\n",
 							bank->name, idx);
diff --git a/drivers/pinctrl/samsung/pinctrl-s3c24xx.c b/drivers/pinctrl/samsung/pinctrl-s3c24xx.c
index ac1eba30cf40..625cb1065eaf 100644
--- a/drivers/pinctrl/samsung/pinctrl-s3c24xx.c
+++ b/drivers/pinctrl/samsung/pinctrl-s3c24xx.c
@@ -525,7 +525,7 @@  static int s3c24xx_eint_init(struct samsung_pinctrl_drv_data *d)
 		ops = (bank->eint_offset == 0) ? &s3c24xx_gpf_irq_ops
 					       : &s3c24xx_gpg_irq_ops;
 
-		bank->irq_domain = irq_domain_add_linear(bank->of_node,
+		bank->irq_domain = irq_domain_create_linear(bank->fwnode,
 				bank->nr_pins, ops, ddata);
 		if (!bank->irq_domain) {
 			dev_err(dev, "wkup irq domain add failed\n");
diff --git a/drivers/pinctrl/samsung/pinctrl-s3c64xx.c b/drivers/pinctrl/samsung/pinctrl-s3c64xx.c
index c5f95a1071ae..c5d92db4fdb1 100644
--- a/drivers/pinctrl/samsung/pinctrl-s3c64xx.c
+++ b/drivers/pinctrl/samsung/pinctrl-s3c64xx.c
@@ -471,7 +471,7 @@  static int s3c64xx_eint_gpio_init(struct samsung_pinctrl_drv_data *d)
 		mask = bank->eint_mask;
 		nr_eints = fls(mask);
 
-		bank->irq_domain = irq_domain_add_linear(bank->of_node,
+		bank->irq_domain = irq_domain_create_linear(bank->fwnode,
 					nr_eints, &s3c64xx_gpio_irqd_ops, bank);
 		if (!bank->irq_domain) {
 			dev_err(dev, "gpio irq domain add failed\n");
@@ -743,7 +743,7 @@  static int s3c64xx_eint_eint0_init(struct samsung_pinctrl_drv_data *d)
 			return -ENOMEM;
 		ddata->bank = bank;
 
-		bank->irq_domain = irq_domain_add_linear(bank->of_node,
+		bank->irq_domain = irq_domain_create_linear(bank->fwnode,
 				nr_eints, &s3c64xx_eint0_irqd_ops, ddata);
 		if (!bank->irq_domain) {
 			dev_err(dev, "wkup irq domain add failed\n");
diff --git a/drivers/pinctrl/samsung/pinctrl-samsung.c b/drivers/pinctrl/samsung/pinctrl-samsung.c
index f610beab23a0..26d309d2516d 100644
--- a/drivers/pinctrl/samsung/pinctrl-samsung.c
+++ b/drivers/pinctrl/samsung/pinctrl-samsung.c
@@ -18,6 +18,7 @@ 
 #include <linux/init.h>
 #include <linux/platform_device.h>
 #include <linux/io.h>
+#include <linux/property.h>
 #include <linux/slab.h>
 #include <linux/err.h>
 #include <linux/gpio/driver.h>
@@ -966,7 +967,7 @@  static int samsung_gpiolib_register(struct platform_device *pdev,
 		gc->base = bank->grange.base;
 		gc->ngpio = bank->nr_pins;
 		gc->parent = &pdev->dev;
-		gc->of_node = bank->of_node;
+		gc->fwnode = bank->fwnode;
 		gc->label = bank->name;
 
 		ret = devm_gpiochip_add_data(&pdev->dev, gc, bank);
@@ -1002,27 +1003,25 @@  samsung_pinctrl_get_soc_data_for_of_alias(struct platform_device *pdev)
 	return &(of_data->ctrl[id]);
 }
 
-static void samsung_banks_of_node_put(struct samsung_pinctrl_drv_data *d)
+static void samsung_banks_node_put(struct samsung_pinctrl_drv_data *d)
 {
 	struct samsung_pin_bank *bank;
 	unsigned int i;
 
 	bank = d->pin_banks;
 	for (i = 0; i < d->nr_banks; ++i, ++bank)
-		of_node_put(bank->of_node);
+		fwnode_handle_put(bank->fwnode);
 }
 
 /*
  * Iterate over all driver pin banks to find one matching the name of node,
  * skipping optional "-gpio" node suffix. When found, assign node to the bank.
  */
-static void samsung_banks_of_node_get(struct device *dev,
-				      struct samsung_pinctrl_drv_data *d,
-				      struct device_node *node)
+static void samsung_banks_node_get(struct device *dev, struct samsung_pinctrl_drv_data *d)
 {
 	const char *suffix = "-gpio-bank";
 	struct samsung_pin_bank *bank;
-	struct device_node *child;
+	struct fwnode_handle *child;
 	/* Pin bank names are up to 4 characters */
 	char node_name[20];
 	unsigned int i;
@@ -1038,17 +1037,17 @@  static void samsung_banks_of_node_get(struct device *dev,
 			continue;
 		}
 
-		for_each_child_of_node(node, child) {
-			if (!of_find_property(child, "gpio-controller", NULL))
-				continue;
-			if (of_node_name_eq(child, node_name))
+		for_each_gpiochip_node(dev, child) {
+			struct device_node *np = to_of_node(child);
+
+			if (of_node_name_eq(np, node_name))
 				break;
-			else if (of_node_name_eq(child, bank->name))
+			if (of_node_name_eq(np, bank->name))
 				break;
 		}
 
 		if (child)
-			bank->of_node = child;
+			bank->fwnode = child;
 		else
 			dev_warn(dev, "Missing node for bank %s - invalid DTB\n",
 				 bank->name);
@@ -1061,7 +1060,6 @@  static const struct samsung_pin_ctrl *
 samsung_pinctrl_get_soc_data(struct samsung_pinctrl_drv_data *d,
 			     struct platform_device *pdev)
 {
-	struct device_node *node = pdev->dev.of_node;
 	const struct samsung_pin_bank_data *bdata;
 	const struct samsung_pin_ctrl *ctrl;
 	struct samsung_pin_bank *bank;
@@ -1125,7 +1123,7 @@  samsung_pinctrl_get_soc_data(struct samsung_pinctrl_drv_data *d,
 	 */
 	d->virt_base = virt_base[0];
 
-	samsung_banks_of_node_get(&pdev->dev, d, node);
+	samsung_banks_node_get(&pdev->dev, d);
 
 	d->pin_base = pin_base;
 	pin_base += d->nr_pins;
@@ -1186,7 +1184,7 @@  static int samsung_pinctrl_probe(struct platform_device *pdev)
 err_unregister:
 	samsung_pinctrl_unregister(pdev, drvdata);
 err_put_banks:
-	samsung_banks_of_node_put(drvdata);
+	samsung_banks_node_put(drvdata);
 	return ret;
 }
 
diff --git a/drivers/pinctrl/samsung/pinctrl-samsung.h b/drivers/pinctrl/samsung/pinctrl-samsung.h
index 5b32d3f30fcd..fc6f5199c548 100644
--- a/drivers/pinctrl/samsung/pinctrl-samsung.h
+++ b/drivers/pinctrl/samsung/pinctrl-samsung.h
@@ -165,7 +165,7 @@  struct samsung_pin_bank {
 
 	u32		pin_base;
 	void		*soc_priv;
-	struct device_node *of_node;
+	struct fwnode_handle *fwnode;
 	struct samsung_pinctrl_drv_data *drvdata;
 	struct irq_domain *irq_domain;
 	struct gpio_chip gpio_chip;