Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752050AbbGOI3F (ORCPT ); Wed, 15 Jul 2015 04:29:05 -0400 Received: from fish.king.net.pl ([79.190.246.46]:35104 "EHLO king.net.pl" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1751585AbbGOI3A (ORCPT ); Wed, 15 Jul 2015 04:29:00 -0400 Date: Wed, 15 Jul 2015 09:31:17 +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 Content-Length: 3254 Lines: 77 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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/