diff mbox series

[v2,4/4] powerpc/52xx: Convert to use fwnode API

Message ID 20220507100147.5802-4-andriy.shevchenko@linux.intel.com
State New
Headers show
Series [v2,1/4] powerpc/52xx: Remove dead code, i.e. mpc52xx_get_xtal_freq() | expand

Commit Message

Andy Shevchenko May 7, 2022, 10:01 a.m. UTC
We may convert the GPT driver to use fwnode API for the sake
of consistency of the used APIs inside the driver.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
v2: no changes
 arch/powerpc/platforms/52xx/mpc52xx_gpt.c | 47 +++++++++++------------
 1 file changed, 22 insertions(+), 25 deletions(-)

Comments

Michael Ellerman May 16, 2022, 1:48 p.m. UTC | #1
Andy Shevchenko <andriy.shevchenko@linux.intel.com> writes:
> We may convert the GPT driver to use fwnode API for the sake
> of consistency of the used APIs inside the driver.

I'm not sure about this one.

It's more consistent to use fwnode in this driver, but it's very
inconsistent with the rest of the powerpc code. We have basically no
uses of the fwnode APIs at the moment.

It seems like a pretty straight-forward conversion, but there could
easily be a bug in there, I don't have any way to test it. Do you?

cheers



> diff --git a/arch/powerpc/platforms/52xx/mpc52xx_gpt.c b/arch/powerpc/platforms/52xx/mpc52xx_gpt.c
> index ae47fdcc8a96..58c3651034bd 100644
> --- a/arch/powerpc/platforms/52xx/mpc52xx_gpt.c
> +++ b/arch/powerpc/platforms/52xx/mpc52xx_gpt.c
> @@ -53,10 +53,9 @@
>  #include <linux/interrupt.h>
>  #include <linux/io.h>
>  #include <linux/list.h>
> +#include <linux/mod_devicetable.h>
> +#include <linux/module.h>
>  #include <linux/mutex.h>
> -#include <linux/of.h>
> -#include <linux/of_platform.h>
> -#include <linux/of_gpio.h>
>  #include <linux/kernel.h>
>  #include <linux/property.h>
>  #include <linux/slab.h>
> @@ -64,7 +63,7 @@
>  #include <linux/watchdog.h>
>  #include <linux/miscdevice.h>
>  #include <linux/uaccess.h>
> -#include <linux/module.h>
> +
>  #include <asm/div64.h>
>  #include <asm/mpc52xx.h>
>  
> @@ -235,18 +234,17 @@ static const struct irq_domain_ops mpc52xx_gpt_irq_ops = {
>  	.xlate = mpc52xx_gpt_irq_xlate,
>  };
>  
> -static void
> -mpc52xx_gpt_irq_setup(struct mpc52xx_gpt_priv *gpt, struct device_node *node)
> +static void mpc52xx_gpt_irq_setup(struct mpc52xx_gpt_priv *gpt)
>  {
>  	int cascade_virq;
>  	unsigned long flags;
>  	u32 mode;
>  
> -	cascade_virq = irq_of_parse_and_map(node, 0);
> -	if (!cascade_virq)
> +	cascade_virq = platform_get_irq(to_platform_device(gpt->dev), 0);
> +	if (cascade_virq < 0)
>  		return;
>  
> -	gpt->irqhost = irq_domain_add_linear(node, 1, &mpc52xx_gpt_irq_ops, gpt);
> +	gpt->irqhost = irq_domain_create_linear(dev_fwnode(gpt->dev), 1, &mpc52xx_gpt_irq_ops, gpt);
>  	if (!gpt->irqhost) {
>  		dev_err(gpt->dev, "irq_domain_add_linear() failed\n");
>  		return;
> @@ -670,8 +668,7 @@ static int mpc52xx_gpt_wdt_init(void)
>  	return err;
>  }
>  
> -static int mpc52xx_gpt_wdt_setup(struct mpc52xx_gpt_priv *gpt,
> -				 const u32 *period)
> +static int mpc52xx_gpt_wdt_setup(struct mpc52xx_gpt_priv *gpt, const u32 period)
>  {
>  	u64 real_timeout;
>  
> @@ -679,14 +676,14 @@ static int mpc52xx_gpt_wdt_setup(struct mpc52xx_gpt_priv *gpt,
>  	mpc52xx_gpt_wdt = gpt;
>  
>  	/* configure the wdt if the device tree contained a timeout */
> -	if (!period || *period == 0)
> +	if (period == 0)
>  		return 0;
>  
> -	real_timeout = (u64) *period * 1000000000ULL;
> +	real_timeout = (u64)period * 1000000000ULL;
>  	if (mpc52xx_gpt_do_start(gpt, real_timeout, 0, 1))
>  		dev_warn(gpt->dev, "starting as wdt failed\n");
>  	else
> -		dev_info(gpt->dev, "watchdog set to %us timeout\n", *period);
> +		dev_info(gpt->dev, "watchdog set to %us timeout\n", period);
>  	return 0;
>  }
>  
> @@ -697,8 +694,7 @@ static int mpc52xx_gpt_wdt_init(void)
>  	return 0;
>  }
>  
> -static inline int mpc52xx_gpt_wdt_setup(struct mpc52xx_gpt_priv *gpt,
> -					const u32 *period)
> +static inline int mpc52xx_gpt_wdt_setup(struct mpc52xx_gpt_priv *gpt, const u32 period)
>  {
>  	return 0;
>  }
> @@ -726,25 +722,26 @@ static int mpc52xx_gpt_probe(struct platform_device *ofdev)
>  	dev_set_drvdata(&ofdev->dev, gpt);
>  
>  	mpc52xx_gpt_gpio_setup(gpt);
> -	mpc52xx_gpt_irq_setup(gpt, ofdev->dev.of_node);
> +	mpc52xx_gpt_irq_setup(gpt);
>  
>  	mutex_lock(&mpc52xx_gpt_list_mutex);
>  	list_add(&gpt->list, &mpc52xx_gpt_list);
>  	mutex_unlock(&mpc52xx_gpt_list_mutex);
>  
>  	/* check if this device could be a watchdog */
> -	if (of_get_property(ofdev->dev.of_node, "fsl,has-wdt", NULL) ||
> -	    of_get_property(ofdev->dev.of_node, "has-wdt", NULL)) {
> -		const u32 *on_boot_wdt;
> +	if (device_property_present(gpt->dev, "fsl,has-wdt") ||
> +	    device_property_present(gpt->dev, "has-wdt")) {
> +		u32 on_boot_wdt = 0;
> +		int ret;
>  
>  		gpt->wdt_mode = MPC52xx_GPT_CAN_WDT;
> -		on_boot_wdt = of_get_property(ofdev->dev.of_node,
> -					      "fsl,wdt-on-boot", NULL);
> -		if (on_boot_wdt) {
> +		ret = device_property_read_u32(gpt->dev, "fsl,wdt-on-boot", &on_boot_wdt);
> +		if (ret) {
> +			dev_info(gpt->dev, "can function as watchdog\n");
> +		} else {
>  			dev_info(gpt->dev, "used as watchdog\n");
>  			gpt->wdt_mode |= MPC52xx_GPT_IS_WDT;
> -		} else
> -			dev_info(gpt->dev, "can function as watchdog\n");
> +		}
>  		mpc52xx_gpt_wdt_setup(gpt, on_boot_wdt);
>  	}
>  
> -- 
> 2.35.1
Andy Shevchenko May 16, 2022, 2:05 p.m. UTC | #2
On Mon, May 16, 2022 at 11:48:05PM +1000, Michael Ellerman wrote:
> Andy Shevchenko <andriy.shevchenko@linux.intel.com> writes:
> > We may convert the GPT driver to use fwnode API for the sake
> > of consistency of the used APIs inside the driver.
> 
> I'm not sure about this one.
> 
> It's more consistent to use fwnode in this driver, but it's very
> inconsistent with the rest of the powerpc code. We have basically no
> uses of the fwnode APIs at the moment.

Fair point!

> It seems like a pretty straight-forward conversion, but there could
> easily be a bug in there, I don't have any way to test it. Do you?

Nope, only compile testing. The important part of this series is to
clean up of_node from GPIO library, so since here it's a user of
it I want to do that. This patch is just ad-hoc conversion that I
noticed is possible. But there is no any requirement to do so.

Lemme drop this from v3.
Andy Shevchenko May 16, 2022, 2:10 p.m. UTC | #3
On Mon, May 16, 2022 at 05:05:12PM +0300, Andy Shevchenko wrote:
> On Mon, May 16, 2022 at 11:48:05PM +1000, Michael Ellerman wrote:
> > Andy Shevchenko <andriy.shevchenko@linux.intel.com> writes:
> > > We may convert the GPT driver to use fwnode API for the sake
> > > of consistency of the used APIs inside the driver.
> > 
> > I'm not sure about this one.
> > 
> > It's more consistent to use fwnode in this driver, but it's very
> > inconsistent with the rest of the powerpc code. We have basically no
> > uses of the fwnode APIs at the moment.
> 
> Fair point!
> 
> > It seems like a pretty straight-forward conversion, but there could
> > easily be a bug in there, I don't have any way to test it. Do you?
> 
> Nope, only compile testing. The important part of this series is to
> clean up of_node from GPIO library, so since here it's a user of
> it I want to do that. This patch is just ad-hoc conversion that I
> noticed is possible. But there is no any requirement to do so.
> 
> Lemme drop this from v3.

I just realize that there is no point to send a v3. You can just apply
first 3 patches. Or is your comment against entire series?
Michael Ellerman May 16, 2022, 11:38 p.m. UTC | #4
Andy Shevchenko <andriy.shevchenko@linux.intel.com> writes:
> On Mon, May 16, 2022 at 05:05:12PM +0300, Andy Shevchenko wrote:
>> On Mon, May 16, 2022 at 11:48:05PM +1000, Michael Ellerman wrote:
>> > Andy Shevchenko <andriy.shevchenko@linux.intel.com> writes:
>> > > We may convert the GPT driver to use fwnode API for the sake
>> > > of consistency of the used APIs inside the driver.
>> > 
>> > I'm not sure about this one.
>> > 
>> > It's more consistent to use fwnode in this driver, but it's very
>> > inconsistent with the rest of the powerpc code. We have basically no
>> > uses of the fwnode APIs at the moment.
>> 
>> Fair point!
>> 
>> > It seems like a pretty straight-forward conversion, but there could
>> > easily be a bug in there, I don't have any way to test it. Do you?
>> 
>> Nope, only compile testing. The important part of this series is to
>> clean up of_node from GPIO library, so since here it's a user of
>> it I want to do that. This patch is just ad-hoc conversion that I
>> noticed is possible. But there is no any requirement to do so.
>> 
>> Lemme drop this from v3.
>
> I just realize that there is no point to send a v3. You can just apply
> first 3 patches. Or is your comment against entire series?

No, my comment is just about this patch.

I don't mind converting to new APIs when it's blocking some other
cleanup. But given the age of this code I think it's probably better to
just leave the rest of it as-is, unless someone volunteers to test it.

So yeah I'll just take patches 1-3 of this v2 series, no need to resend.

cheers
Andy Shevchenko May 17, 2022, 4:30 p.m. UTC | #5
On Tue, May 17, 2022 at 09:38:56AM +1000, Michael Ellerman wrote:
> Andy Shevchenko <andriy.shevchenko@linux.intel.com> writes:
> > On Mon, May 16, 2022 at 05:05:12PM +0300, Andy Shevchenko wrote:
> >> On Mon, May 16, 2022 at 11:48:05PM +1000, Michael Ellerman wrote:
> >> > Andy Shevchenko <andriy.shevchenko@linux.intel.com> writes:
> >> > > We may convert the GPT driver to use fwnode API for the sake
> >> > > of consistency of the used APIs inside the driver.
> >> > 
> >> > I'm not sure about this one.
> >> > 
> >> > It's more consistent to use fwnode in this driver, but it's very
> >> > inconsistent with the rest of the powerpc code. We have basically no
> >> > uses of the fwnode APIs at the moment.
> >> 
> >> Fair point!
> >> 
> >> > It seems like a pretty straight-forward conversion, but there could
> >> > easily be a bug in there, I don't have any way to test it. Do you?
> >> 
> >> Nope, only compile testing. The important part of this series is to
> >> clean up of_node from GPIO library, so since here it's a user of
> >> it I want to do that. This patch is just ad-hoc conversion that I
> >> noticed is possible. But there is no any requirement to do so.
> >> 
> >> Lemme drop this from v3.
> >
> > I just realize that there is no point to send a v3. You can just apply
> > first 3 patches. Or is your comment against entire series?
> 
> No, my comment is just about this patch.
> 
> I don't mind converting to new APIs when it's blocking some other
> cleanup. But given the age of this code I think it's probably better to
> just leave the rest of it as-is, unless someone volunteers to test it.
> 
> So yeah I'll just take patches 1-3 of this v2 series, no need to resend.

Thanks!

One note though, the fwnode_for_each_parent_node() is not yet available in
upstream, but will be after v5.19-rc1. It means the patch 3 can't be applied
without that. That's why LKP complained on patch 4 in this series.

That said, the easiest way is to postpone it till v5.19-rc1 is out.
diff mbox series

Patch

diff --git a/arch/powerpc/platforms/52xx/mpc52xx_gpt.c b/arch/powerpc/platforms/52xx/mpc52xx_gpt.c
index ae47fdcc8a96..58c3651034bd 100644
--- a/arch/powerpc/platforms/52xx/mpc52xx_gpt.c
+++ b/arch/powerpc/platforms/52xx/mpc52xx_gpt.c
@@ -53,10 +53,9 @@ 
 #include <linux/interrupt.h>
 #include <linux/io.h>
 #include <linux/list.h>
+#include <linux/mod_devicetable.h>
+#include <linux/module.h>
 #include <linux/mutex.h>
-#include <linux/of.h>
-#include <linux/of_platform.h>
-#include <linux/of_gpio.h>
 #include <linux/kernel.h>
 #include <linux/property.h>
 #include <linux/slab.h>
@@ -64,7 +63,7 @@ 
 #include <linux/watchdog.h>
 #include <linux/miscdevice.h>
 #include <linux/uaccess.h>
-#include <linux/module.h>
+
 #include <asm/div64.h>
 #include <asm/mpc52xx.h>
 
@@ -235,18 +234,17 @@  static const struct irq_domain_ops mpc52xx_gpt_irq_ops = {
 	.xlate = mpc52xx_gpt_irq_xlate,
 };
 
-static void
-mpc52xx_gpt_irq_setup(struct mpc52xx_gpt_priv *gpt, struct device_node *node)
+static void mpc52xx_gpt_irq_setup(struct mpc52xx_gpt_priv *gpt)
 {
 	int cascade_virq;
 	unsigned long flags;
 	u32 mode;
 
-	cascade_virq = irq_of_parse_and_map(node, 0);
-	if (!cascade_virq)
+	cascade_virq = platform_get_irq(to_platform_device(gpt->dev), 0);
+	if (cascade_virq < 0)
 		return;
 
-	gpt->irqhost = irq_domain_add_linear(node, 1, &mpc52xx_gpt_irq_ops, gpt);
+	gpt->irqhost = irq_domain_create_linear(dev_fwnode(gpt->dev), 1, &mpc52xx_gpt_irq_ops, gpt);
 	if (!gpt->irqhost) {
 		dev_err(gpt->dev, "irq_domain_add_linear() failed\n");
 		return;
@@ -670,8 +668,7 @@  static int mpc52xx_gpt_wdt_init(void)
 	return err;
 }
 
-static int mpc52xx_gpt_wdt_setup(struct mpc52xx_gpt_priv *gpt,
-				 const u32 *period)
+static int mpc52xx_gpt_wdt_setup(struct mpc52xx_gpt_priv *gpt, const u32 period)
 {
 	u64 real_timeout;
 
@@ -679,14 +676,14 @@  static int mpc52xx_gpt_wdt_setup(struct mpc52xx_gpt_priv *gpt,
 	mpc52xx_gpt_wdt = gpt;
 
 	/* configure the wdt if the device tree contained a timeout */
-	if (!period || *period == 0)
+	if (period == 0)
 		return 0;
 
-	real_timeout = (u64) *period * 1000000000ULL;
+	real_timeout = (u64)period * 1000000000ULL;
 	if (mpc52xx_gpt_do_start(gpt, real_timeout, 0, 1))
 		dev_warn(gpt->dev, "starting as wdt failed\n");
 	else
-		dev_info(gpt->dev, "watchdog set to %us timeout\n", *period);
+		dev_info(gpt->dev, "watchdog set to %us timeout\n", period);
 	return 0;
 }
 
@@ -697,8 +694,7 @@  static int mpc52xx_gpt_wdt_init(void)
 	return 0;
 }
 
-static inline int mpc52xx_gpt_wdt_setup(struct mpc52xx_gpt_priv *gpt,
-					const u32 *period)
+static inline int mpc52xx_gpt_wdt_setup(struct mpc52xx_gpt_priv *gpt, const u32 period)
 {
 	return 0;
 }
@@ -726,25 +722,26 @@  static int mpc52xx_gpt_probe(struct platform_device *ofdev)
 	dev_set_drvdata(&ofdev->dev, gpt);
 
 	mpc52xx_gpt_gpio_setup(gpt);
-	mpc52xx_gpt_irq_setup(gpt, ofdev->dev.of_node);
+	mpc52xx_gpt_irq_setup(gpt);
 
 	mutex_lock(&mpc52xx_gpt_list_mutex);
 	list_add(&gpt->list, &mpc52xx_gpt_list);
 	mutex_unlock(&mpc52xx_gpt_list_mutex);
 
 	/* check if this device could be a watchdog */
-	if (of_get_property(ofdev->dev.of_node, "fsl,has-wdt", NULL) ||
-	    of_get_property(ofdev->dev.of_node, "has-wdt", NULL)) {
-		const u32 *on_boot_wdt;
+	if (device_property_present(gpt->dev, "fsl,has-wdt") ||
+	    device_property_present(gpt->dev, "has-wdt")) {
+		u32 on_boot_wdt = 0;
+		int ret;
 
 		gpt->wdt_mode = MPC52xx_GPT_CAN_WDT;
-		on_boot_wdt = of_get_property(ofdev->dev.of_node,
-					      "fsl,wdt-on-boot", NULL);
-		if (on_boot_wdt) {
+		ret = device_property_read_u32(gpt->dev, "fsl,wdt-on-boot", &on_boot_wdt);
+		if (ret) {
+			dev_info(gpt->dev, "can function as watchdog\n");
+		} else {
 			dev_info(gpt->dev, "used as watchdog\n");
 			gpt->wdt_mode |= MPC52xx_GPT_IS_WDT;
-		} else
-			dev_info(gpt->dev, "can function as watchdog\n");
+		}
 		mpc52xx_gpt_wdt_setup(gpt, on_boot_wdt);
 	}