From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753170AbbGORig (ORCPT ); Wed, 15 Jul 2015 13:38:36 -0400 Received: from fish.king.net.pl ([79.190.246.46]:35501 "EHLO king.net.pl" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1752760AbbGORid (ORCPT ); Wed, 15 Jul 2015 13:38:33 -0400 Date: Wed, 15 Jul 2015 19:34:09 +0200 (CEST) From: Paul Osmialowski X-X-Sender: newchief@localhost.localdomain To: Linus Walleij cc: Paul Osmialowski , Greg Kroah-Hartman , Ian Campbell , Jiri Slaby , Kumar Gala , Mark Rutland , Michael Turquette , Pawel Moll , Rob Herring , Russell King , Stephen Boyd , Vinod Koul , "linux-kernel@vger.kernel.org" , "linux-arm-kernel@lists.infradead.org" , linux-clk@vger.kernel.org, "linux-gpio@vger.kernel.org" , "linux-serial@vger.kernel.org" , "devicetree@vger.kernel.org" , dmaengine@vger.kernel.org, Arnd Bergmann , Geert Uytterhoeven , Nicolas Pitre , Paul Bolle , Thomas Gleixner , Uwe Kleine-Koenig , Anson Huang , Frank Li , Jingchang Lu , Rob Herring , Yuri Tikhonov , Sergei Poselenov , Alexander Potashev Subject: Re: [PATCH v2 3/9] arm: twr-k70f120m: clock driver for Kinetis SoC In-Reply-To: Message-ID: References: <1435667250-28299-1-git-send-email-pawelo@king.net.pl> <1435667250-28299-4-git-send-email-pawelo@king.net.pl> User-Agent: Alpine 2.00 (LNX 1167 2008-08-23) MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi Linus, I had some discussion with an expert here and now I see drawbacks of using struct for regs, I'll turn it into defines as you suggested. On Wed, 15 Jul 2015, Paul Osmialowski wrote: > Hi Linus, > > Thanks for all of your comments, I'll consider them during my works on the > next iteration. However, I have doubts about this one: > > On Tue, 14 Jul 2015, Linus Walleij wrote: > > > On Tue, Jun 30, 2015 at 2:27 PM, Paul Osmialowski wrote: > > > > > Based on K70P256M150SF3RM.pdf K70 Sub-Family Reference Manual, Rev. 3. > > > > > > Signed-off-by: Paul Osmialowski > > (...) > > > +struct kinetis_sim_regs { > > > + u32 sopt1; /* System Options Register 1 */ > > > + u32 rsv0[1024]; > > > + u32 sopt2; /* System Options Register 2 */ > > > + u32 rsv1; > > > + u32 sopt4; /* System Options Register 4 */ > > > + u32 sopt5; /* System Options Register 5 */ > > > + u32 sopt6; /* System Options Register 6 */ > > > + u32 sopt7; /* System Options Register 7 */ > > > + u32 rsv2[2]; > > > + u32 sdid; /* System Device Identification Register */ > > > + u32 scgc[KINETIS_SIM_CG_NUMREGS]; /* Clock Gating Regs 1...7 */ > > > + u32 clkdiv1; /* System Clock Divider Register 1 */ > > > + u32 clkdiv2; /* System Clock Divider Register 2 */ > > > + u32 fcfg1; /* Flash Configuration Register 1 */ > > > + u32 fcfg2; /* Flash Configuration Register 2 */ > > > + u32 uidh; /* Unique Identification Register High */ > > > + u32 uidmh; /* Unique Identification Register Mid-High */ > > > + u32 uidml; /* Unique Identification Register Mid Low */ > > > + u32 uidl; /* Unique Identification Register Low */ > > > + u32 clkdiv3; /* System Clock Divider Register 3 */ > > > + u32 clkdiv4; /* System Clock Divider Register 4 */ > > > + u32 mcr; /* Misc Control Register */ > > > +}; > > > > Now there is this design pattern where you copy the datasheet > > register map to a struct again. > > > > This is not good if there is a second revision of the hardware and some > > registers are shuffled around. IMO it is better to just use #defines > > for register > > offsets, so you can do exceptions later. Else a new hardware revision > > leads to a new struct with new accessor functions etc etc. > > > > I don't see how replacing this structure with bunch of defines could make > anyones life easier. As registers are shuffled around due to updated > hardware revision they could be shuffled in this structure too. Doing > this with buch of defines would require eager and careful adaptation of > all the defines. I don't see how this could be easier. > > Note that I'm not making any instances of the structure (it is used only > for casting), so shuffling its fields around should not affect the code > that follows. > > After recent purge it is only used within this macro: > > #define KINETIS_SIM_PTR(base, reg) \ > (&(((struct kinetis_sim_regs *)(base))->reg)) > > ...and used like this: > > ioread32(KINETIS_SIM_PTR(sim, clkdiv1)); > > IMHO changing the struct internals does not require subsequent changes in > any of those. > > Thanks, > Paul >