All of lore.kernel.org
 help / color / mirror / Atom feed
From: lee.jones@linaro.org (Lee Jones)
To: linux-arm-kernel@lists.infradead.org
Subject: [PATCH 21/33] clk: ux500: Add Device Tree support for the PRCC Kernel clock
Date: Thu, 12 Sep 2013 15:56:51 +0100	[thread overview]
Message-ID: <20130912145651.GJ11227@lee--X1> (raw)
In-Reply-To: <CACRpkdYvAfEh6UGa7qR_e9ynj3TkPSM2RKtr7Scw7L3p10AgXA@mail.gmail.com>

On Thu, 12 Sep 2013, Linus Walleij wrote:

> On Tue, Aug 27, 2013 at 10:23 AM, Lee Jones <lee.jones@linaro.org> wrote:
> > On Fri, 23 Aug 2013, Linus Walleij wrote:
> >> On Thu, Aug 22, 2013 at 11:21 AM, Lee Jones <lee.jones@linaro.org> wrote:
> 
> >> > 1. Duplicate each of the; clk_reg_prcmu_*(), clk_reg_prcc_pclk(),
> >> > clk_reg_prcc_kclk() calls into your proposed u8500_clk_init_dt(),
> >> > which, while keeping everything separate would be unrealistic.
> >>
> >> I think this is perfectly realistic.
> >>
> >> You're not going to duplicate each clk_register_clkdev(),
> >> which makes it way smaller than the original function,
> >> and since one of the function will be inside a
> >>
> >> #ifdef CONFIG_OF
> >> #endif
> >>
> >> After we switch the entire platform to DT-only it will be pretty
> >> obvious which big chunk of code that needs to go away, it's
> >> a clean cut.
> >>
> >> (Note: I know the #ifdef CONFIG_OF is not necessary anymore
> >> since we switched to multiplatform, but I intend that marker for
> >> humans, not machines.)
> >
> > This sounds gross. To duplicate; u8500_clk_init(), u8540_clk_init()
> > and u9540_clk_init() just for the sake of loading a few pointers into
> > an array for a small part of the development cycle sounds obscene.
> >
> > I genuinely think keeping the current patch in this series and then
> > removing the clk_register_clkdev() in the remove ATAG support series
> > is the best way to go.
> 
> So what I am worrying about is not only the looks of the code
> and what is beautiful or not may be something of an opinion
> anway.
> 
> What I worry about is leaving all the calls to clk_register_clkdev()
> in the DT boot path. Because that has the potential to hide a lot
> of bugs, as clk_get() from drivers that should've got named and
> probed randomly now will still find their clocks from their old
> device names, instead of using the <&ampersand> clocks from
> the device tree.
> 
> But if you still don't like this, let me cook a counter-patch so
> I can realized on my own how terribly wrong I am...

I'm going to yank all of the clk_register_clkdev() calls out
imminently anyway, so all those these hiding bugs will soon become
apparent in any case. Why don't I place my 'remove ATAGs' patch-set
on top of this one and then finally remove the clk_register_clkdev()s
and watch the carnage ensue?

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org ? Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

WARNING: multiple messages have this Message-ID (diff)
From: Lee Jones <lee.jones@linaro.org>
To: Linus Walleij <linus.walleij@linaro.org>
Cc: "linux-arm-kernel@lists.infradead.org" 
	<linux-arm-kernel@lists.infradead.org>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	Arnd Bergmann <arnd@arndb.de>,
	Linus WALLEIJ <linus.walleij@stericsson.com>,
	Mike Turquette <mturquette@linaro.org>,
	Ulf Hansson <ulf.hansson@linaro.org>
Subject: Re: [PATCH 21/33] clk: ux500: Add Device Tree support for the PRCC Kernel clock
Date: Thu, 12 Sep 2013 15:56:51 +0100	[thread overview]
Message-ID: <20130912145651.GJ11227@lee--X1> (raw)
In-Reply-To: <CACRpkdYvAfEh6UGa7qR_e9ynj3TkPSM2RKtr7Scw7L3p10AgXA@mail.gmail.com>

On Thu, 12 Sep 2013, Linus Walleij wrote:

> On Tue, Aug 27, 2013 at 10:23 AM, Lee Jones <lee.jones@linaro.org> wrote:
> > On Fri, 23 Aug 2013, Linus Walleij wrote:
> >> On Thu, Aug 22, 2013 at 11:21 AM, Lee Jones <lee.jones@linaro.org> wrote:
> 
> >> > 1. Duplicate each of the; clk_reg_prcmu_*(), clk_reg_prcc_pclk(),
> >> > clk_reg_prcc_kclk() calls into your proposed u8500_clk_init_dt(),
> >> > which, while keeping everything separate would be unrealistic.
> >>
> >> I think this is perfectly realistic.
> >>
> >> You're not going to duplicate each clk_register_clkdev(),
> >> which makes it way smaller than the original function,
> >> and since one of the function will be inside a
> >>
> >> #ifdef CONFIG_OF
> >> #endif
> >>
> >> After we switch the entire platform to DT-only it will be pretty
> >> obvious which big chunk of code that needs to go away, it's
> >> a clean cut.
> >>
> >> (Note: I know the #ifdef CONFIG_OF is not necessary anymore
> >> since we switched to multiplatform, but I intend that marker for
> >> humans, not machines.)
> >
> > This sounds gross. To duplicate; u8500_clk_init(), u8540_clk_init()
> > and u9540_clk_init() just for the sake of loading a few pointers into
> > an array for a small part of the development cycle sounds obscene.
> >
> > I genuinely think keeping the current patch in this series and then
> > removing the clk_register_clkdev() in the remove ATAG support series
> > is the best way to go.
> 
> So what I am worrying about is not only the looks of the code
> and what is beautiful or not may be something of an opinion
> anway.
> 
> What I worry about is leaving all the calls to clk_register_clkdev()
> in the DT boot path. Because that has the potential to hide a lot
> of bugs, as clk_get() from drivers that should've got named and
> probed randomly now will still find their clocks from their old
> device names, instead of using the <&ampersand> clocks from
> the device tree.
> 
> But if you still don't like this, let me cook a counter-patch so
> I can realized on my own how terribly wrong I am...

I'm going to yank all of the clk_register_clkdev() calls out
imminently anyway, so all those these hiding bugs will soon become
apparent in any case. Why don't I place my 'remove ATAGs' patch-set
on top of this one and then finally remove the clk_register_clkdev()s
and watch the carnage ensue?

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

  reply	other threads:[~2013-09-12 14:56 UTC|newest]

Thread overview: 178+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2013-06-06 12:16 [PATCH 00/32] ARM: ux500: Enable clocks for Device Tree Lee Jones
2013-06-06 12:16 ` Lee Jones
2013-06-06 12:16 ` [PATCH 01/33] mfd: dbx500-prcmu: Provide PRCMU numerical clock identifiers Lee Jones
2013-06-06 12:16   ` Lee Jones
2013-06-06 12:16 ` [PATCH 02/33] ARM: ux500: Add PRCMU clock node to DBx500 Device Tree Lee Jones
2013-06-06 12:16   ` Lee Jones
2013-06-06 12:16 ` [PATCH 03/33] ARM: ux500: Supply the DMA clock lookup to the DBX500 DT Lee Jones
2013-06-06 12:16   ` Lee Jones
2013-06-06 12:16 ` [PATCH 04/33] ARM: ux500: Add PRCC Peripheral clock node to DBx500 Device Tree Lee Jones
2013-06-06 12:16   ` Lee Jones
2013-06-06 12:16 ` [PATCH 05/33] ARM: ux500: Supply the GPIO clocks lookup to the DBX500 DT Lee Jones
2013-06-06 12:16   ` Lee Jones
2013-06-06 12:16 ` [PATCH 06/33] ARM: ux500: Supply the USB clock " Lee Jones
2013-06-06 12:16   ` Lee Jones
2013-06-06 12:16 ` [PATCH 07/33] ARM: ux500: Supply the Ethernet clock lookup to Snowball's DT Lee Jones
2013-06-06 12:16   ` Lee Jones
2013-06-06 12:16 ` [PATCH 08/33] ARM: ux500: Add PRCC Kernel clock node to DBx500 Device Tree Lee Jones
2013-06-06 12:16   ` Lee Jones
2013-06-06 12:16 ` [PATCH 09/33] ARM: ux500: Supply the I2C clocks lookup to the DBX500 DT Lee Jones
2013-06-06 12:16   ` Lee Jones
2013-08-20  9:11   ` Linus Walleij
2013-08-20  9:11     ` Linus Walleij
2013-08-20  9:30     ` Sascha Hauer
2013-08-20  9:30       ` Sascha Hauer
2013-08-22 13:37       ` Mark Rutland
2013-08-22 13:37         ` Mark Rutland
2013-08-22 13:49         ` Lee Jones
2013-08-22 13:49           ` Lee Jones
2013-08-22 14:19         ` Lee Jones
2013-08-22 14:19           ` Lee Jones
2013-08-22 15:17           ` Mark Rutland
2013-08-22 15:17             ` Mark Rutland
2013-08-22 15:41             ` Lee Jones
2013-08-22 15:41               ` Lee Jones
2013-08-22 16:04               ` Mark Rutland
2013-08-22 16:04                 ` Mark Rutland
2013-08-22 21:19               ` Sascha Hauer
2013-08-22 21:19                 ` Sascha Hauer
2013-08-23  7:56                 ` Lee Jones
2013-08-23  7:56                   ` Lee Jones
2013-08-23 16:55                   ` Mark Rutland
2013-08-23 16:55                     ` Mark Rutland
2013-08-27  8:06                     ` Lee Jones
2013-08-27  8:06                       ` Lee Jones
2013-08-27 13:46                       ` Mark Rutland
2013-08-27 13:46                         ` Mark Rutland
2013-08-27 13:46                         ` Mark Rutland
2013-08-27 14:08                         ` Lee Jones
2013-08-27 14:08                           ` Lee Jones
2013-08-27 15:51                           ` Rob Herring
2013-08-27 15:51                             ` Rob Herring
2013-08-27 16:15                             ` Pawel Moll
2013-08-27 16:15                               ` Pawel Moll
2013-08-21  8:28     ` Lee Jones
2013-08-21  8:28       ` Lee Jones
2013-08-21 22:44       ` Linus Walleij
2013-08-21 22:44         ` Linus Walleij
2013-08-22  9:23     ` Lee Jones
2013-08-22  9:23       ` Lee Jones
2013-06-06 12:16 ` [PATCH 10/33] ARM: ux500: Supply the UART " Lee Jones
2013-06-06 12:16   ` Lee Jones
2013-06-06 12:16 ` [PATCH 11/33] ARM: ux500: Supply the SDI (MMC) " Lee Jones
2013-06-06 12:16   ` Lee Jones
2013-06-06 12:17 ` [PATCH 12/33] ARM: ux500: Supply the MSP (Audio) " Lee Jones
2013-06-06 12:17   ` Lee Jones
2013-06-06 12:17 ` [PATCH 13/33] ARM: ux500: Add RTC (fixed-frequency) clock node to DBx500 Device Tree Lee Jones
2013-06-06 12:17   ` Lee Jones
2013-06-06 12:17 ` [PATCH 14/33] ARM: ux500: Supply the RTC clock lookup to the DBX500 DT Lee Jones
2013-06-06 12:17   ` Lee Jones
2013-06-06 12:17 ` [PATCH 15/33] ARM: ux500: Add TWD (fixed-factor) clock node to DBx500 Device Tree Lee Jones
2013-06-06 12:17   ` Lee Jones
2013-06-06 12:17 ` [PATCH 16/33] ARM: ux500: Supply the TWD Timer clock lookup to the DBX500 DT Lee Jones
2013-06-06 12:17   ` Lee Jones
2013-06-06 12:17 ` [PATCH 17/33] clk: ux500: Provide u8500_clk with skeleton Device Tree support Lee Jones
2013-06-06 12:17   ` Lee Jones
2013-06-06 12:17 ` [PATCH 18/33] clk: ux500: Add a 2-cell Device Tree parser for obtaining PRCC clocks Lee Jones
2013-06-06 12:17   ` Lee Jones
2013-06-10 20:54   ` Ulf Hansson
2013-06-10 20:54     ` Ulf Hansson
2013-06-11  9:12     ` Lee Jones
2013-06-11  9:12       ` Lee Jones
2013-06-11 11:07   ` [PATCH 18/33 v2] " Lee Jones
2013-06-11 11:07     ` Lee Jones
2013-06-06 12:17 ` [PATCH 19/33] clk: ux500: Add Device Tree support for the PRCMU clock Lee Jones
2013-06-06 12:17   ` Lee Jones
2013-06-10 21:19   ` Ulf Hansson
2013-06-10 21:19     ` Ulf Hansson
2013-06-11 11:10     ` Lee Jones
2013-06-11 11:10       ` Lee Jones
2013-06-11 11:12       ` Lee Jones
2013-06-11 11:12         ` Lee Jones
2013-08-21  8:23   ` Linus Walleij
2013-08-21  8:23     ` Linus Walleij
2013-08-21 10:10     ` Lee Jones
2013-08-21 10:10       ` Lee Jones
2013-06-06 12:17 ` [PATCH 20/33] clk: ux500: Add Device Tree support for the PRCC Peripheral clock Lee Jones
2013-06-06 12:17   ` Lee Jones
2013-06-11 11:51   ` [PATCH 20/33 v2] " Lee Jones
2013-06-11 11:51     ` Lee Jones
2013-06-06 12:17 ` [PATCH 21/33] clk: ux500: Add Device Tree support for the PRCC Kernel clock Lee Jones
2013-06-06 12:17   ` Lee Jones
2013-06-10 21:24   ` Ulf Hansson
2013-06-10 21:24     ` Ulf Hansson
2013-06-11  9:10     ` Lee Jones
2013-06-11  9:10       ` Lee Jones
2013-06-11 11:09   ` [PATCH 21/33 v2] " Lee Jones
2013-06-11 11:09     ` Lee Jones
2013-06-12 14:46     ` Arnd Bergmann
2013-06-12 14:46       ` Arnd Bergmann
2013-06-18 21:17       ` Mike Turquette
2013-06-18 21:17         ` Mike Turquette
2013-06-19  7:42         ` Lee Jones
2013-06-19  7:42           ` Lee Jones
2013-06-21 18:20           ` Mike Turquette
2013-08-21  8:17   ` [PATCH 21/33] " Linus Walleij
2013-08-21  8:17     ` Linus Walleij
2013-08-21 10:14     ` Lee Jones
2013-08-21 10:14       ` Lee Jones
2013-08-21 22:46       ` Linus Walleij
2013-08-21 22:46         ` Linus Walleij
2013-08-22  9:21         ` Lee Jones
2013-08-22  9:21           ` Lee Jones
2013-08-23 18:01           ` Linus Walleij
2013-08-23 18:01             ` Linus Walleij
2013-08-24  8:00             ` Arnd Bergmann
2013-08-24  8:00               ` Arnd Bergmann
2013-08-24 21:19               ` Linus Walleij
2013-08-24 21:19                 ` Linus Walleij
2013-08-27  8:23             ` Lee Jones
2013-08-27  8:23               ` Lee Jones
2013-09-12 12:50               ` Linus Walleij
2013-09-12 12:50                 ` Linus Walleij
2013-09-12 14:56                 ` Lee Jones [this message]
2013-09-12 14:56                   ` Lee Jones
2013-09-13  7:20                   ` Linus Walleij
2013-09-13  7:20                     ` Linus Walleij
2013-06-06 12:17 ` [PATCH 22/33] clk: ux500: Add Device Tree support for the RTC clock Lee Jones
2013-06-06 12:17   ` Lee Jones
2013-06-06 12:17 ` [PATCH 23/33] clk: ux500: Add Device Tree support for the TWD clock Lee Jones
2013-06-06 12:17   ` Lee Jones
2013-06-06 12:17 ` [PATCH 24/33] ARM: ux500: Remove AUXDATA relating to GPIO clock-name bindings Lee Jones
2013-06-06 12:17   ` Lee Jones
2013-06-06 12:17 ` [PATCH 25/33] ARM: ux500: Remove AUXDATA relating to UART " Lee Jones
2013-06-06 12:17   ` Lee Jones
2013-06-06 12:17 ` [PATCH 26/33] ARM: ux500: Remove AUXDATA relating to SDI (MMC) " Lee Jones
2013-06-06 12:17   ` Lee Jones
2013-08-23 13:31   ` Linus Walleij
2013-08-23 13:31     ` Linus Walleij
2013-08-23 14:45     ` Lee Jones
2013-08-23 14:45       ` Lee Jones
2013-08-24  7:57     ` Arnd Bergmann
2013-08-24  7:57       ` Arnd Bergmann
2013-08-27  8:11       ` Lee Jones
2013-08-27  8:11         ` Lee Jones
2013-06-06 12:17 ` [PATCH 27/33] ARM: ux500: Remove AUXDATA relating to I2C " Lee Jones
2013-06-06 12:17   ` Lee Jones
2013-06-06 12:17 ` [PATCH 28/33] ARM: ux500: Remove AUXDATA relating to MSP (Audio) " Lee Jones
2013-06-06 12:17   ` Lee Jones
2013-08-21  8:08   ` Linus Walleij
2013-08-21  8:08     ` Linus Walleij
2013-08-21  8:17     ` Lee Jones
2013-08-21  8:17       ` Lee Jones
2013-06-06 12:17 ` [PATCH 29/33] ARM: ux500: Remove AUXDATA relating to USB " Lee Jones
2013-06-06 12:17   ` Lee Jones
2013-06-06 12:17 ` [PATCH 30/33] ARM: ux500: Remove AUXDATA relating to Ethernet " Lee Jones
2013-06-06 12:17   ` Lee Jones
2013-06-06 12:17 ` [PATCH 31/33] ARM: ux500: Remove AUXDATA relating to DMA " Lee Jones
2013-06-06 12:17   ` Lee Jones
2013-06-06 12:17 ` [PATCH 32/33] ARM: ux500: Reclassify PRCMU AUXDATA entry Lee Jones
2013-06-06 12:17   ` Lee Jones
2013-06-06 12:17 ` [PATCH 33/33] ARM: ux500: Remove SSP AUXDATA pertaining to DMA bindings Lee Jones
2013-06-06 12:17   ` Lee Jones
2013-06-12 13:27 ` [PATCH 00/32] ARM: ux500: Enable clocks for Device Tree Lee Jones
2013-06-12 13:27   ` Lee Jones
2013-06-13  8:41   ` Linus Walleij
2013-06-13  8:41     ` Linus Walleij
2013-06-13  9:34     ` Lee Jones
2013-06-13  9:34       ` Lee Jones

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20130912145651.GJ11227@lee--X1 \
    --to=lee.jones@linaro.org \
    --cc=linux-arm-kernel@lists.infradead.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.