Skip to contentSkip to conversation

Revert "h8300: clocksource"

PATCH In review Daniel Lezcano 3 participants 4 messages Oct 30, 2015 – Oct 31, 2015

Subsystems (2)
lore
Share & tools
lore
#0 lore raw

PATCH 1/1 Revert "h8300: clocksource"

This reverts commit 618b902d8c098f2fff188119da7b3184c4bc5483.

The renesas architecture have been dropped and re-introduced two years after with a supposed improvement of the code.

While looking at the timer drivers, I see it did not passed a correct code review process. No maintainters were in Cc, not code review and no ack from them (Thomas and I).

Even if those drivers where already in the tree several years ago, that does not prevent them, when re-introduced, to be reviewed again and at least acked by the maintainer.

Even if the patches where on lkml@, that does not prevent the different sub-system maintainers to be in Cc.

Instead of that, these drivers passed silently through another tree (arm-soc I guess).

Revert these drivers in order to have them re-submitted and reviewed again.

There is legacy code to be removed.

Conflicts:

	drivers/clocksource/Kconfig
	drivers/clocksource/Makefile
	drivers/clocksource/h8300_timer16.c
	drivers/clocksource/h8300_timer8.c
	drivers/clocksource/h8300_tpu.c
---
 .../bindings/timer/renesas,16bit-timer.txt         |  25 --
 .../bindings/timer/renesas,8bit-timer.txt          |  25 --
 .../devicetree/bindings/timer/renesas,tpu.txt      |  21 --
 drivers/clocksource/Kconfig                        |   7 -
 drivers/clocksource/Makefile                       |   3 -
 drivers/clocksource/h8300_timer16.c                | 253 ----------------
 drivers/clocksource/h8300_timer8.c                 | 317 ---------------------
 drivers/clocksource/h8300_tpu.c                    | 206 -------------
 8 files changed, 857 deletions(-)
 delete mode 100644 Documentation/devicetree/bindings/timer/renesas,16bit-timer.txt
 delete mode 100644 Documentation/devicetree/bindings/timer/renesas,8bit-timer.txt
 delete mode 100644 Documentation/devicetree/bindings/timer/renesas,tpu.txt
 delete mode 100644 drivers/clocksource/h8300_timer16.c
 delete mode 100644 drivers/clocksource/h8300_timer8.c
 delete mode 100644 drivers/clocksource/h8300_tpu.c
Documentation/devicetree/bindings/timer/renesas,16bit-timer.txt deleted +0 -25
deleted file mode 100644
index e879244..0000000
@@ -1,25 +0,0 @@
-* Renesas H8/300 16bit timer
-
-The 16bit timer is a 16bit timer/counter with configurable clock inputs and
-programmable compare match.
-
-Required Properties:
-
-  - compatible: must contain "renesas,16bit-timer"
-  - reg: base address and length of the registers block for the timer module.
-  - interrupts: interrupt-specifier for the timer, IMIA
-  - clocks: a list of phandle, one for each entry in clock-names.
-  - clock-names: must contain "peripheral_clk" for the functional clock.
-  - renesas,channel: timer channel number.
-
-Example:
-
-	timer16: timer@ffff68 {
-		compatible = "reneas,16bit-timer";
-		reg = <0xffff68 8>, <0xffff60 8>;
-		interrupts = <24>;
-		renesas,channel = <0>;
-		clocks = <&pclk>;
-		clock-names = "peripheral_clk";
-	};
-
Documentation/devicetree/bindings/timer/renesas,8bit-timer.txt deleted +0 -25
deleted file mode 100644
index 9dca375..0000000
@@ -1,25 +0,0 @@
-* Renesas H8/300 8bit timer
-
-The 8bit timer is a 8bit timer/counter with configurable clock inputs and
-programmable compare match.
-
-This implement only supported cascade mode.
-
-Required Properties:
-
-  - compatible: must contain "renesas,8bit-timer"
-  - reg: base address and length of the registers block for the timer module.
-  - interrupts: interrupt-specifier for the timer, CMIA and TOVI
-  - clocks: a list of phandle, one for each entry in clock-names.
-  - clock-names: must contain "fck" for the functional clock.
-
-Example:
-
-	timer8_0: timer@ffff80 {
-		compatible = "renesas,8bit-timer";
-		reg = <0xffff80 10>;
-		interrupts = <36>;
-		clocks = <&fclk>;
-		clock-names = "fck";
-	};
-
Documentation/devicetree/bindings/timer/renesas,tpu.txt deleted +0 -21
deleted file mode 100644
index f8b2589..0000000
@@ -1,21 +0,0 @@
-* Renesas H8/300 Timer Pluse Unit
-
-The TPU is a 16bit timer/counter with configurable clock inputs and
-programmable compare match.
-This implementation support only cascade mode.
-
-Required Properties:
-
-  - compatible: must contain "renesas,tpu"
-  - reg: base address and length of the registers block in 2 channel.
-  - clocks: a list of phandle, one for each entry in clock-names.
-  - clock-names: must contain "peripheral_clk" for the functional clock.
-
-
-Example:
-	tpu: tpu@ffffe0 {
-		compatible = "renesas,tpu";
-		reg = <0xffffe0 16>, <0xfffff0 12>;
-		clocks = <&pclk>;
-		clock-names = "peripheral_clk";
-	};
drivers/clocksource/Kconfig +0 -7
index 50b68bc..34e1cfa 100644
@@ -290,12 +290,6 @@ config CLKSRC_PXA
 	  This enables OST0 support available on PXA and SA-11x0
 	  platforms.
 
-config H8300_TMR16
-        bool
-
-config H8300_TPU
-        bool
-
 config CLKSRC_IMX_GPT
 	bool "Clocksource using i.MX GPT" if COMPILE_TEST
 	depends on ARM && CLKDEV_LOOKUP
@@ -308,5 +302,4 @@ config CLKSRC_ST_LPC
 	help
 	  Enable this option to use the Low Power controller timer
 	  as clocksource.
-
 endmenu
drivers/clocksource/Makefile +0 -3
index fc9348d..89ba4f2 100644
@@ -59,7 +59,4 @@ obj-$(CONFIG_CLKSRC_MIPS_GIC)		+= mips-gic-timer.o
 obj-$(CONFIG_CLKSRC_TANGO_XTAL)		+= tango_xtal.o
 obj-$(CONFIG_CLKSRC_IMX_GPT)		+= timer-imx-gpt.o
 obj-$(CONFIG_ASM9260_TIMER)		+= asm9260_timer.o
-obj-$(CONFIG_H8300)			+= h8300_timer8.o
-obj-$(CONFIG_H8300_TMR16)		+= h8300_timer16.o
-obj-$(CONFIG_H8300_TPU)			+= h8300_tpu.o
 obj-$(CONFIG_CLKSRC_ST_LPC)		+= clksrc_st_lpc.o
drivers/clocksource/h8300_timer16.c deleted +0 -253

253 lines not on this page · raw message

drivers/clocksource/h8300_timer8.c deleted +0 -317

317 lines not on this page · raw message

drivers/clocksource/h8300_tpu.c deleted +0 -206

206 lines not on this page · raw message

Signature · 1 line
1.9.1
#1 lore raw
Quoted 9 lines · On Friday 30 October 2015 11:26:08 Daniel Lezcano wrote:

This reverts commit 618b902d8c098f2fff188119da7b3184c4bc5483.

The renesas architecture have been dropped and re-introduced two years after with a supposed improvement of the code.

While looking at the timer drivers, I see it did not passed a correct code review process. No maintainters were in Cc, not code review and no ack from them (Thomas and I).

Quoted 6 lines

Even if those drivers where already in the tree several years ago, that does not prevent them, when re-introduced, to be reviewed again and at least acked by the maintainer.

Even if the patches where on lkml@, that does not prevent the different sub-system maintainers to be in Cc.

Agreed.

Quoted 2 lines

Instead of that, these drivers passed silently through another tree (arm-soc I guess).

No need to speculate when you can look it up easily (it wasn't us):

$ git log --oneline --merges --ancestry-path 618b902d8c098f2fff188119da7b3184c4bc5483.. | tail -n1
55a7d4b85ca1 Merge tag 'for-4.2' of git://git.sourceforge.jp/gitroot/uclinux-h8/linux
$ git show 55a7d4b85ca1
commit 55a7d4b85ca1f723d26b8956e8faeff730d0d240
Merge: aefbef10e3ae 07834743f42b
Author: Linus Torvalds <[email protected]>
Date:   Thu Jun 25 13:07:24 2015 -0700

Merge tag 'for-4.2' of git://git.sourceforge.jp/gitroot/uclinux-h8/linux

Pull Renesas H8/300 architecture re-introduction from Yoshinori Sato.

We dropped arch/h8300 two years ago as stale and old, this is a new and more modern rewritten arch support for the same architecture.

I reviewed the series before it was merged, but failed to notice that you were not on Cc, and I did not look at the clocksource drivers in much detail (if at all).

Quoted 3 lines

There is legacy code to be removed.

I found that Yoshinori Sato has always addressed all review comments very quickly by rewriting the code in a proper manner when mistakes were pointed out in the other drivers. Maybe you can describe now what the problems are with the three drivers to give him a chance to fix them?

Arnd

#2 lore raw
Quoted 2 lines · Arnd Bergmann
Quoted 1 line · On Friday 30 October 2015 11:26:08 Daniel Lezcano wrote:

This reverts commit 618b902d8c098f2fff188119da7b3184c4bc5483.

[ ... ]

Quoted 4 lines

I found that Yoshinori Sato has always addressed all review comments very quickly by rewriting the code in a proper manner when mistakes were pointed out in the other drivers. Maybe you can describe now what the problems are with the three drivers to give him a chance to fix them?

I agree it is a good argument to give him the opportunity to fix the code.

Yoshinori Sato, can you please do a cleanup in the init functions to comply with the other drivers, use CLOCKSOURCE_OF_DECLARE and remove the module exit code ?

Thanks.

-- Daniel

Signature · 5 lines
 <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog
#3 lore raw
Quoted 12 lines · Daniel Lezcano
Quoted 2 lines · Arnd Bergmann
Quoted 1 line · On Friday 30 October 2015 11:26:08 Daniel Lezcano wrote:

This reverts commit 618b902d8c098f2fff188119da7b3184c4bc5483.

[ ... ]

Quoted 4 lines

I found that Yoshinori Sato has always addressed all review comments very quickly by rewriting the code in a proper manner when mistakes were pointed out in the other drivers. Maybe you can describe now what the problems are with the three drivers to give him a chance to fix them?

Quoted 7 lines

I agree it is a good argument to give him the opportunity to fix the code.

Yoshinori Sato, can you please do a cleanup in the init functions to comply with the other drivers, use CLOCKSOURCE_OF_DECLARE and remove the module exit code ?

Thanks.

OK.
Please wait few days.

Quoted 9 lines

-- Daniel

Signature · 5 lines
 <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog
Signature · 2 lines
Yoshinori Sato
<[email protected]>