linux-watchdog.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v2 0/4] JZ4740 watchdog cleanups
@ 2019-06-07 16:24 Paul Cercueil
  2019-06-07 16:24 ` [PATCH v2 1/4] watchdog: jz4740: Use register names from <linux/mfd/ingenic-tcu.h> Paul Cercueil
                   ` (3 more replies)
  0 siblings, 4 replies; 10+ messages in thread
From: Paul Cercueil @ 2019-06-07 16:24 UTC (permalink / raw)
  To: Guenter Roeck, Wim Van Sebroeck; +Cc: od, linux-watchdog, linux-kernel

V1 patchset didn't apply anymore on v5.2-rc3 so I rebased on top of it.
The previous patch 4/4 (which added SPDX license notifier) has been
dropped since somebody else did the job in -rc3.
So I added another cleanup as patch 4/4, and I tweaked patch 2/4 to
adjust to Guenter's feedback on V1.

Cheers
-Paul



^ permalink raw reply	[flat|nested] 10+ messages in thread

* [PATCH v2 1/4] watchdog: jz4740: Use register names from <linux/mfd/ingenic-tcu.h>
  2019-06-07 16:24 [PATCH v2 0/4] JZ4740 watchdog cleanups Paul Cercueil
@ 2019-06-07 16:24 ` Paul Cercueil
  2019-06-07 16:24 ` [PATCH v2 2/4] watchdog: jz4740: Avoid starting watchdog in set_timeout Paul Cercueil
                   ` (2 subsequent siblings)
  3 siblings, 0 replies; 10+ messages in thread
From: Paul Cercueil @ 2019-06-07 16:24 UTC (permalink / raw)
  To: Guenter Roeck, Wim Van Sebroeck
  Cc: od, linux-watchdog, linux-kernel, Paul Cercueil

Use the macros from <linux/mfd/ingenic-tcu.h> instead of declaring our
own.

Signed-off-by: Paul Cercueil <paul@crapouillou.net>
Reviewed-by: Guenter Roeck <linux@roeck-us.net>
---

Notes:
    v2: Rebased on v5.2-rc3

 drivers/watchdog/jz4740_wdt.c | 39 ++++++++++++++---------------------
 1 file changed, 16 insertions(+), 23 deletions(-)

diff --git a/drivers/watchdog/jz4740_wdt.c b/drivers/watchdog/jz4740_wdt.c
index 313358b2e0b1..a2592c4fff02 100644
--- a/drivers/watchdog/jz4740_wdt.c
+++ b/drivers/watchdog/jz4740_wdt.c
@@ -4,6 +4,7 @@
  *  JZ4740 Watchdog driver
  */
 
+#include <linux/mfd/ingenic-tcu.h>
 #include <linux/module.h>
 #include <linux/moduleparam.h>
 #include <linux/types.h>
@@ -19,23 +20,16 @@
 
 #include <asm/mach-jz4740/timer.h>
 
-#define JZ_REG_WDT_TIMER_DATA     0x0
-#define JZ_REG_WDT_COUNTER_ENABLE 0x4
-#define JZ_REG_WDT_TIMER_COUNTER  0x8
-#define JZ_REG_WDT_TIMER_CONTROL  0xC
-
 #define JZ_WDT_CLOCK_PCLK 0x1
 #define JZ_WDT_CLOCK_RTC  0x2
 #define JZ_WDT_CLOCK_EXT  0x4
 
-#define JZ_WDT_CLOCK_DIV_SHIFT   3
-
-#define JZ_WDT_CLOCK_DIV_1    (0 << JZ_WDT_CLOCK_DIV_SHIFT)
-#define JZ_WDT_CLOCK_DIV_4    (1 << JZ_WDT_CLOCK_DIV_SHIFT)
-#define JZ_WDT_CLOCK_DIV_16   (2 << JZ_WDT_CLOCK_DIV_SHIFT)
-#define JZ_WDT_CLOCK_DIV_64   (3 << JZ_WDT_CLOCK_DIV_SHIFT)
-#define JZ_WDT_CLOCK_DIV_256  (4 << JZ_WDT_CLOCK_DIV_SHIFT)
-#define JZ_WDT_CLOCK_DIV_1024 (5 << JZ_WDT_CLOCK_DIV_SHIFT)
+#define JZ_WDT_CLOCK_DIV_1    (0 << TCU_TCSR_PRESCALE_LSB)
+#define JZ_WDT_CLOCK_DIV_4    (1 << TCU_TCSR_PRESCALE_LSB)
+#define JZ_WDT_CLOCK_DIV_16   (2 << TCU_TCSR_PRESCALE_LSB)
+#define JZ_WDT_CLOCK_DIV_64   (3 << TCU_TCSR_PRESCALE_LSB)
+#define JZ_WDT_CLOCK_DIV_256  (4 << TCU_TCSR_PRESCALE_LSB)
+#define JZ_WDT_CLOCK_DIV_1024 (5 << TCU_TCSR_PRESCALE_LSB)
 
 #define DEFAULT_HEARTBEAT 5
 #define MAX_HEARTBEAT     2048
@@ -63,7 +57,7 @@ static int jz4740_wdt_ping(struct watchdog_device *wdt_dev)
 {
 	struct jz4740_wdt_drvdata *drvdata = watchdog_get_drvdata(wdt_dev);
 
-	writew(0x0, drvdata->base + JZ_REG_WDT_TIMER_COUNTER);
+	writew(0x0, drvdata->base + TCU_REG_WDT_TCNT);
 	return 0;
 }
 
@@ -86,18 +80,17 @@ static int jz4740_wdt_set_timeout(struct watchdog_device *wdt_dev,
 			break;
 		}
 		timeout_value >>= 2;
-		clock_div += (1 << JZ_WDT_CLOCK_DIV_SHIFT);
+		clock_div += (1 << TCU_TCSR_PRESCALE_LSB);
 	}
 
-	writeb(0x0, drvdata->base + JZ_REG_WDT_COUNTER_ENABLE);
-	writew(clock_div, drvdata->base + JZ_REG_WDT_TIMER_CONTROL);
+	writeb(0x0, drvdata->base + TCU_REG_WDT_TCER);
+	writew(clock_div, drvdata->base + TCU_REG_WDT_TCSR);
 
-	writew((u16)timeout_value, drvdata->base + JZ_REG_WDT_TIMER_DATA);
-	writew(0x0, drvdata->base + JZ_REG_WDT_TIMER_COUNTER);
-	writew(clock_div | JZ_WDT_CLOCK_RTC,
-		drvdata->base + JZ_REG_WDT_TIMER_CONTROL);
+	writew((u16)timeout_value, drvdata->base + TCU_REG_WDT_TDR);
+	writew(0x0, drvdata->base + TCU_REG_WDT_TCNT);
+	writew(clock_div | JZ_WDT_CLOCK_RTC, drvdata->base + TCU_REG_WDT_TCSR);
 
-	writeb(0x1, drvdata->base + JZ_REG_WDT_COUNTER_ENABLE);
+	writeb(0x1, drvdata->base + TCU_REG_WDT_TCER);
 
 	wdt_dev->timeout = new_timeout;
 	return 0;
@@ -115,7 +108,7 @@ static int jz4740_wdt_stop(struct watchdog_device *wdt_dev)
 {
 	struct jz4740_wdt_drvdata *drvdata = watchdog_get_drvdata(wdt_dev);
 
-	writeb(0x0, drvdata->base + JZ_REG_WDT_COUNTER_ENABLE);
+	writeb(0x0, drvdata->base + TCU_REG_WDT_TCER);
 	jz4740_timer_disable_watchdog();
 
 	return 0;
-- 
2.21.0.593.g511ec345e18


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCH v2 2/4] watchdog: jz4740: Avoid starting watchdog in set_timeout
  2019-06-07 16:24 [PATCH v2 0/4] JZ4740 watchdog cleanups Paul Cercueil
  2019-06-07 16:24 ` [PATCH v2 1/4] watchdog: jz4740: Use register names from <linux/mfd/ingenic-tcu.h> Paul Cercueil
@ 2019-06-07 16:24 ` Paul Cercueil
  2019-06-07 16:24 ` [PATCH v2 3/4] watchdog: jz4740: Drop dependency on MACH_JZ47xx Paul Cercueil
  2019-06-07 16:24 ` [PATCH v2 4/4] watchdog: jz4740: Make probe function __init_or_module Paul Cercueil
  3 siblings, 0 replies; 10+ messages in thread
From: Paul Cercueil @ 2019-06-07 16:24 UTC (permalink / raw)
  To: Guenter Roeck, Wim Van Sebroeck
  Cc: od, linux-watchdog, linux-kernel, Paul Cercueil

Previously the jz4740_wdt_set_timeout() function was starting the timer
unconditionally, even if it was stopped when that function was entered.

Now, the timer will be restarted only if it was already running before
this function is called.

Signed-off-by: Paul Cercueil <paul@crapouillou.net>
Reviewed-by: Guenter Roeck <linux@roeck-us.net>
---

Notes:
    v2: - Don't disable TCU timer if it was already disabled
    	- Don't enable TCU timer if it was already enabled

 drivers/watchdog/jz4740_wdt.c | 14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)

diff --git a/drivers/watchdog/jz4740_wdt.c b/drivers/watchdog/jz4740_wdt.c
index a2592c4fff02..7519d80c5d05 100644
--- a/drivers/watchdog/jz4740_wdt.c
+++ b/drivers/watchdog/jz4740_wdt.c
@@ -68,6 +68,7 @@ static int jz4740_wdt_set_timeout(struct watchdog_device *wdt_dev,
 	unsigned int rtc_clk_rate;
 	unsigned int timeout_value;
 	unsigned short clock_div = JZ_WDT_CLOCK_DIV_1;
+	u8 tcer;
 
 	rtc_clk_rate = clk_get_rate(drvdata->rtc_clk);
 
@@ -83,6 +84,7 @@ static int jz4740_wdt_set_timeout(struct watchdog_device *wdt_dev,
 		clock_div += (1 << TCU_TCSR_PRESCALE_LSB);
 	}
 
+	tcer = readb(drvdata->base + TCU_REG_WDT_TCER);
 	writeb(0x0, drvdata->base + TCU_REG_WDT_TCER);
 	writew(clock_div, drvdata->base + TCU_REG_WDT_TCSR);
 
@@ -90,7 +92,8 @@ static int jz4740_wdt_set_timeout(struct watchdog_device *wdt_dev,
 	writew(0x0, drvdata->base + TCU_REG_WDT_TCNT);
 	writew(clock_div | JZ_WDT_CLOCK_RTC, drvdata->base + TCU_REG_WDT_TCSR);
 
-	writeb(0x1, drvdata->base + TCU_REG_WDT_TCER);
+	if (tcer & TCU_WDT_TCER_TCEN)
+		writeb(TCU_WDT_TCER_TCEN, drvdata->base + TCU_REG_WDT_TCER);
 
 	wdt_dev->timeout = new_timeout;
 	return 0;
@@ -98,9 +101,18 @@ static int jz4740_wdt_set_timeout(struct watchdog_device *wdt_dev,
 
 static int jz4740_wdt_start(struct watchdog_device *wdt_dev)
 {
+	struct jz4740_wdt_drvdata *drvdata = watchdog_get_drvdata(wdt_dev);
+	u8 tcer;
+
+	tcer = readb(drvdata->base + TCU_REG_WDT_TCER);
+
 	jz4740_timer_enable_watchdog();
 	jz4740_wdt_set_timeout(wdt_dev, wdt_dev->timeout);
 
+	/* Start watchdog if it wasn't started already */
+	if (!(tcer & TCU_WDT_TCER_TCEN))
+		writeb(TCU_WDT_TCER_TCEN, drvdata->base + TCU_REG_WDT_TCER);
+
 	return 0;
 }
 
-- 
2.21.0.593.g511ec345e18


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCH v2 3/4] watchdog: jz4740: Drop dependency on MACH_JZ47xx
  2019-06-07 16:24 [PATCH v2 0/4] JZ4740 watchdog cleanups Paul Cercueil
  2019-06-07 16:24 ` [PATCH v2 1/4] watchdog: jz4740: Use register names from <linux/mfd/ingenic-tcu.h> Paul Cercueil
  2019-06-07 16:24 ` [PATCH v2 2/4] watchdog: jz4740: Avoid starting watchdog in set_timeout Paul Cercueil
@ 2019-06-07 16:24 ` Paul Cercueil
  2019-06-07 16:24 ` [PATCH v2 4/4] watchdog: jz4740: Make probe function __init_or_module Paul Cercueil
  3 siblings, 0 replies; 10+ messages in thread
From: Paul Cercueil @ 2019-06-07 16:24 UTC (permalink / raw)
  To: Guenter Roeck, Wim Van Sebroeck
  Cc: od, linux-watchdog, linux-kernel, Paul Cercueil

Depending on MACH_JZ47xx prevent us from creating a generic kernel that
works on more than one MIPS board. Instead, we just depend on MIPS being
set.

Signed-off-by: Paul Cercueil <paul@crapouillou.net>
Reviewed-by: Guenter Roeck <linux@roeck-us.net>
---

Notes:
    v2: Rebased on v5.2-rc3

 drivers/watchdog/Kconfig | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
index ffe754539f5a..20dd9efc9416 100644
--- a/drivers/watchdog/Kconfig
+++ b/drivers/watchdog/Kconfig
@@ -1634,7 +1634,7 @@ config INDYDOG
 
 config JZ4740_WDT
 	tristate "Ingenic jz4740 SoC hardware watchdog"
-	depends on MACH_JZ4740 || MACH_JZ4780
+	depends on MIPS
 	select WATCHDOG_CORE
 	help
 	  Hardware driver for the built-in watchdog timer on Ingenic jz4740 SoCs.
-- 
2.21.0.593.g511ec345e18


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCH v2 4/4] watchdog: jz4740: Make probe function __init_or_module
  2019-06-07 16:24 [PATCH v2 0/4] JZ4740 watchdog cleanups Paul Cercueil
                   ` (2 preceding siblings ...)
  2019-06-07 16:24 ` [PATCH v2 3/4] watchdog: jz4740: Drop dependency on MACH_JZ47xx Paul Cercueil
@ 2019-06-07 16:24 ` Paul Cercueil
  2019-06-07 17:29   ` Guenter Roeck
  2019-06-08  7:51   ` Christophe Leroy
  3 siblings, 2 replies; 10+ messages in thread
From: Paul Cercueil @ 2019-06-07 16:24 UTC (permalink / raw)
  To: Guenter Roeck, Wim Van Sebroeck
  Cc: od, linux-watchdog, linux-kernel, Paul Cercueil

This allows the probe function to be dropped after the kernel finished
its initialization, in the case where the driver was not compiled as a
module.

Signed-off-by: Paul Cercueil <paul@crapouillou.net>
---

Notes:
    v2: New patch

 drivers/watchdog/jz4740_wdt.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/watchdog/jz4740_wdt.c b/drivers/watchdog/jz4740_wdt.c
index 7519d80c5d05..2061788c1939 100644
--- a/drivers/watchdog/jz4740_wdt.c
+++ b/drivers/watchdog/jz4740_wdt.c
@@ -157,7 +157,7 @@ static const struct of_device_id jz4740_wdt_of_matches[] = {
 MODULE_DEVICE_TABLE(of, jz4740_wdt_of_matches);
 #endif
 
-static int jz4740_wdt_probe(struct platform_device *pdev)
+static int __init_or_module jz4740_wdt_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
 	struct jz4740_wdt_drvdata *drvdata;
-- 
2.21.0.593.g511ec345e18


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* Re: [PATCH v2 4/4] watchdog: jz4740: Make probe function __init_or_module
  2019-06-07 16:24 ` [PATCH v2 4/4] watchdog: jz4740: Make probe function __init_or_module Paul Cercueil
@ 2019-06-07 17:29   ` Guenter Roeck
  2019-06-08  7:51   ` Christophe Leroy
  1 sibling, 0 replies; 10+ messages in thread
From: Guenter Roeck @ 2019-06-07 17:29 UTC (permalink / raw)
  To: Paul Cercueil; +Cc: Wim Van Sebroeck, od, linux-watchdog, linux-kernel

On Fri, Jun 07, 2019 at 06:24:29PM +0200, Paul Cercueil wrote:
> This allows the probe function to be dropped after the kernel finished
> its initialization, in the case where the driver was not compiled as a
> module.
> 
> Signed-off-by: Paul Cercueil <paul@crapouillou.net>

Reviewed-by: Guenter Roeck <linux@roeck-us.net>

> ---
> 
> Notes:
>     v2: New patch
> 
>  drivers/watchdog/jz4740_wdt.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/watchdog/jz4740_wdt.c b/drivers/watchdog/jz4740_wdt.c
> index 7519d80c5d05..2061788c1939 100644
> --- a/drivers/watchdog/jz4740_wdt.c
> +++ b/drivers/watchdog/jz4740_wdt.c
> @@ -157,7 +157,7 @@ static const struct of_device_id jz4740_wdt_of_matches[] = {
>  MODULE_DEVICE_TABLE(of, jz4740_wdt_of_matches);
>  #endif
>  
> -static int jz4740_wdt_probe(struct platform_device *pdev)
> +static int __init_or_module jz4740_wdt_probe(struct platform_device *pdev)
>  {
>  	struct device *dev = &pdev->dev;
>  	struct jz4740_wdt_drvdata *drvdata;

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v2 4/4] watchdog: jz4740: Make probe function __init_or_module
  2019-06-07 16:24 ` [PATCH v2 4/4] watchdog: jz4740: Make probe function __init_or_module Paul Cercueil
  2019-06-07 17:29   ` Guenter Roeck
@ 2019-06-08  7:51   ` Christophe Leroy
  2019-06-08  9:57     ` Paul Cercueil
  1 sibling, 1 reply; 10+ messages in thread
From: Christophe Leroy @ 2019-06-08  7:51 UTC (permalink / raw)
  To: Paul Cercueil, Guenter Roeck, Wim Van Sebroeck
  Cc: od, linux-watchdog, linux-kernel

Hi Paul,

Le 07/06/2019 à 18:24, Paul Cercueil a écrit :
> This allows the probe function to be dropped after the kernel finished
> its initialization, in the case where the driver was not compiled as a
> module.

I'm not sure that's what  __init_or_module flag does.

As far as I understand, this flag makes the function being dropped only 
when the kernel is built without modules support, ie without 
CONFIG_MODULES. See 
https://elixir.bootlin.com/linux/latest/source/include/linux/module.h#L145

In addition, I'm not sure you can simply define a probe function as 
__init. What if someone tries to unbind and rebind the device through 
sysfs for instance ?

It seems there is a special function called __platform_driver_probe() 
for registering devices when the probe function is to be in __init, see 
https://elixir.bootlin.com/linux/latest/source/drivers/base/platform.c#L684

Christophe

> 
> Signed-off-by: Paul Cercueil <paul@crapouillou.net>
> ---
> 
> Notes:
>      v2: New patch
> 
>   drivers/watchdog/jz4740_wdt.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/watchdog/jz4740_wdt.c b/drivers/watchdog/jz4740_wdt.c
> index 7519d80c5d05..2061788c1939 100644
> --- a/drivers/watchdog/jz4740_wdt.c
> +++ b/drivers/watchdog/jz4740_wdt.c
> @@ -157,7 +157,7 @@ static const struct of_device_id jz4740_wdt_of_matches[] = {
>   MODULE_DEVICE_TABLE(of, jz4740_wdt_of_matches);
>   #endif
>   
> -static int jz4740_wdt_probe(struct platform_device *pdev)
> +static int __init_or_module jz4740_wdt_probe(struct platform_device *pdev)
>   {
>   	struct device *dev = &pdev->dev;
>   	struct jz4740_wdt_drvdata *drvdata;
> 

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v2 4/4] watchdog: jz4740: Make probe function __init_or_module
  2019-06-08  7:51   ` Christophe Leroy
@ 2019-06-08  9:57     ` Paul Cercueil
  2019-06-11  5:16       ` Christophe Leroy
  0 siblings, 1 reply; 10+ messages in thread
From: Paul Cercueil @ 2019-06-08  9:57 UTC (permalink / raw)
  To: Christophe Leroy
  Cc: Guenter Roeck, Wim Van Sebroeck, od, linux-watchdog, linux-kernel

Hi Christophe,

Le sam. 8 juin 2019 à 9:51, Christophe Leroy <christophe.leroy@c-s.fr> 
a écrit :
> Hi Paul,
> 
> Le 07/06/2019 à 18:24, Paul Cercueil a écrit :
>> This allows the probe function to be dropped after the kernel 
>> finished
>> its initialization, in the case where the driver was not compiled as 
>> a
>> module.
> 
> I'm not sure that's what  __init_or_module flag does.
> 
> As far as I understand, this flag makes the function being dropped 
> only when the kernel is built without modules support, ie without 
> CONFIG_MODULES. See 
> https://elixir.bootlin.com/linux/latest/source/include/linux/module.h#L145

So it doesn't depend on the driver being built-in or compiled as a 
module?

> In addition, I'm not sure you can simply define a probe function as 
> __init. What if someone tries to unbind and rebind the device through 
> sysfs for instance ?

Ouch. I feel stupid now.

> It seems there is a special function called __platform_driver_probe() 
> for registering devices when the probe function is to be in __init, 
> see 
> https://elixir.bootlin.com/linux/latest/source/drivers/base/platform.c#L684

Yes, but only usable by drivers that won't defer probe, and it removes 
the bind/unbind attributes from sysfs,
so it shouldn't be used for non-critical drivers, I think.

> Christophe
> 
>> 
>> Signed-off-by: Paul Cercueil <paul@crapouillou.net>
>> ---
>> 
>> Notes:
>>      v2: New patch
>> 
>>   drivers/watchdog/jz4740_wdt.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>> 
>> diff --git a/drivers/watchdog/jz4740_wdt.c 
>> b/drivers/watchdog/jz4740_wdt.c
>> index 7519d80c5d05..2061788c1939 100644
>> --- a/drivers/watchdog/jz4740_wdt.c
>> +++ b/drivers/watchdog/jz4740_wdt.c
>> @@ -157,7 +157,7 @@ static const struct of_device_id 
>> jz4740_wdt_of_matches[] = {
>>   MODULE_DEVICE_TABLE(of, jz4740_wdt_of_matches);
>>   #endif
>>   \x7f-static int jz4740_wdt_probe(struct platform_device *pdev)
>> +static int __init_or_module jz4740_wdt_probe(struct platform_device 
>> *pdev)
>>   {
>>   	struct device *dev = &pdev->dev;
>>   	struct jz4740_wdt_drvdata *drvdata;
>> 



^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v2 4/4] watchdog: jz4740: Make probe function __init_or_module
  2019-06-08  9:57     ` Paul Cercueil
@ 2019-06-11  5:16       ` Christophe Leroy
  2019-06-11 11:02         ` Guenter Roeck
  0 siblings, 1 reply; 10+ messages in thread
From: Christophe Leroy @ 2019-06-11  5:16 UTC (permalink / raw)
  To: Paul Cercueil
  Cc: Guenter Roeck, Wim Van Sebroeck, od, linux-watchdog, linux-kernel

Hi Paul,

Le 08/06/2019 à 11:57, Paul Cercueil a écrit :
> Hi Christophe,
> 
> Le sam. 8 juin 2019 à 9:51, Christophe Leroy <christophe.leroy@c-s.fr> a 
> écrit :
>> Hi Paul,
>>
>> Le 07/06/2019 à 18:24, Paul Cercueil a écrit :
>>> This allows the probe function to be dropped after the kernel finished
>>> its initialization, in the case where the driver was not compiled as a
>>> module.
>>
>> I'm not sure that's what  __init_or_module flag does.
>>
>> As far as I understand, this flag makes the function being dropped 
>> only when the kernel is built without modules support, ie without 
>> CONFIG_MODULES. See 
>> https://elixir.bootlin.com/linux/latest/source/include/linux/module.h#L145 
>>
> 
> So it doesn't depend on the driver being built-in or compiled as a module?

No it doesn't. This flag is for built-in functions that are needed by 
init and modules init. If the kernel doesn't support modules, it can 
drop the function after init. If the kernel supports modules, it has to 
keep the function. That's what this flag is made for.

If your need is to mark a function so that it gets discarded after init 
or module init, just mark it __init. If it is built in, it will be 
dropped after init. If it is in a module, it will be dropped after the 
module is initialised.

> 
>> In addition, I'm not sure you can simply define a probe function as 
>> __init. What if someone tries to unbind and rebind the device through 
>> sysfs for instance ?
> 
> Ouch. I feel stupid now.
> 
>> It seems there is a special function called __platform_driver_probe() 
>> for registering devices when the probe function is to be in __init, 
>> see 
>> https://elixir.bootlin.com/linux/latest/source/drivers/base/platform.c#L684 
>>
> 
> Yes, but only usable by drivers that won't defer probe, and it removes 
> the bind/unbind attributes from sysfs,
> so it shouldn't be used for non-critical drivers, I think.

I guess it would make sense for watchdog drivers, we don't expect this 
kind of driver to be unbinded, do we ?

Christophe

> 
>> Christophe
>>
>>>
>>> Signed-off-by: Paul Cercueil <paul@crapouillou.net>
>>> ---
>>>
>>> Notes:
>>>      v2: New patch
>>>
>>>   drivers/watchdog/jz4740_wdt.c | 2 +-
>>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/watchdog/jz4740_wdt.c 
>>> b/drivers/watchdog/jz4740_wdt.c
>>> index 7519d80c5d05..2061788c1939 100644
>>> --- a/drivers/watchdog/jz4740_wdt.c
>>> +++ b/drivers/watchdog/jz4740_wdt.c
>>> @@ -157,7 +157,7 @@ static const struct of_device_id 
>>> jz4740_wdt_of_matches[] = {
>>>   MODULE_DEVICE_TABLE(of, jz4740_wdt_of_matches);
>>>   #endif
>>>   \x7f-static int jz4740_wdt_probe(struct platform_device *pdev)
>>> +static int __init_or_module jz4740_wdt_probe(struct platform_device 
>>> *pdev)
>>>   {
>>>       struct device *dev = &pdev->dev;
>>>       struct jz4740_wdt_drvdata *drvdata;
>>>
> 

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v2 4/4] watchdog: jz4740: Make probe function __init_or_module
  2019-06-11  5:16       ` Christophe Leroy
@ 2019-06-11 11:02         ` Guenter Roeck
  0 siblings, 0 replies; 10+ messages in thread
From: Guenter Roeck @ 2019-06-11 11:02 UTC (permalink / raw)
  To: Christophe Leroy, Paul Cercueil
  Cc: Wim Van Sebroeck, od, linux-watchdog, linux-kernel

On 6/10/19 10:16 PM, Christophe Leroy wrote:
> Hi Paul,
> 
> Le 08/06/2019 à 11:57, Paul Cercueil a écrit :
>> Hi Christophe,
>>
>> Le sam. 8 juin 2019 à 9:51, Christophe Leroy <christophe.leroy@c-s.fr> a écrit :
>>> Hi Paul,
>>>
>>> Le 07/06/2019 à 18:24, Paul Cercueil a écrit :
>>>> This allows the probe function to be dropped after the kernel finished
>>>> its initialization, in the case where the driver was not compiled as a
>>>> module.
>>>
>>> I'm not sure that's what  __init_or_module flag does.
>>>
>>> As far as I understand, this flag makes the function being dropped only when the kernel is built without modules support, ie without CONFIG_MODULES. See https://elixir.bootlin.com/linux/latest/source/include/linux/module.h#L145
>>
>> So it doesn't depend on the driver being built-in or compiled as a module?
> 
> No it doesn't. This flag is for built-in functions that are needed by init and modules init. If the kernel doesn't support modules, it can drop the function after init. If the kernel supports modules, it has to keep the function. That's what this flag is made for.
> 
> If your need is to mark a function so that it gets discarded after init or module init, just mark it __init. If it is built in, it will be dropped after init. If it is in a module, it will be dropped after the module is initialised.
> 
>>
>>> In addition, I'm not sure you can simply define a probe function as __init. What if someone tries to unbind and rebind the device through sysfs for instance ?
>>
>> Ouch. I feel stupid now.
>>
>>> It seems there is a special function called __platform_driver_probe() for registering devices when the probe function is to be in __init, see https://elixir.bootlin.com/linux/latest/source/drivers/base/platform.c#L684
>>
>> Yes, but only usable by drivers that won't defer probe, and it removes the bind/unbind attributes from sysfs,
>> so it shouldn't be used for non-critical drivers, I think.
> 
> I guess it would make sense for watchdog drivers, we don't expect this kind of driver to be unbinded, do we ?
> 

Except for debugging, where it can be extremely useful to be able to do it.

Guenter

> Christophe
> 
>>
>>> Christophe
>>>
>>>>
>>>> Signed-off-by: Paul Cercueil <paul@crapouillou.net>
>>>> ---
>>>>
>>>> Notes:
>>>>      v2: New patch
>>>>
>>>>   drivers/watchdog/jz4740_wdt.c | 2 +-
>>>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/watchdog/jz4740_wdt.c b/drivers/watchdog/jz4740_wdt.c
>>>> index 7519d80c5d05..2061788c1939 100644
>>>> --- a/drivers/watchdog/jz4740_wdt.c
>>>> +++ b/drivers/watchdog/jz4740_wdt.c
>>>> @@ -157,7 +157,7 @@ static const struct of_device_id jz4740_wdt_of_matches[] = {
>>>>   MODULE_DEVICE_TABLE(of, jz4740_wdt_of_matches);
>>>>   #endif
>>>>   \x7f-static int jz4740_wdt_probe(struct platform_device *pdev)
>>>> +static int __init_or_module jz4740_wdt_probe(struct platform_device *pdev)
>>>>   {
>>>>       struct device *dev = &pdev->dev;
>>>>       struct jz4740_wdt_drvdata *drvdata;
>>>>
>>
> 


^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2019-06-11 11:02 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-06-07 16:24 [PATCH v2 0/4] JZ4740 watchdog cleanups Paul Cercueil
2019-06-07 16:24 ` [PATCH v2 1/4] watchdog: jz4740: Use register names from <linux/mfd/ingenic-tcu.h> Paul Cercueil
2019-06-07 16:24 ` [PATCH v2 2/4] watchdog: jz4740: Avoid starting watchdog in set_timeout Paul Cercueil
2019-06-07 16:24 ` [PATCH v2 3/4] watchdog: jz4740: Drop dependency on MACH_JZ47xx Paul Cercueil
2019-06-07 16:24 ` [PATCH v2 4/4] watchdog: jz4740: Make probe function __init_or_module Paul Cercueil
2019-06-07 17:29   ` Guenter Roeck
2019-06-08  7:51   ` Christophe Leroy
2019-06-08  9:57     ` Paul Cercueil
2019-06-11  5:16       ` Christophe Leroy
2019-06-11 11:02         ` Guenter Roeck

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).