| 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 |
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
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!
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
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.
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
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
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 --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;
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(-)