From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S965354Ab3DHOyF (ORCPT ); Mon, 8 Apr 2013 10:54:05 -0400 Received: from mail.active-venture.com ([67.228.131.205]:58672 "EHLO mail.active-venture.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S965341Ab3DHOyD (ORCPT ); Mon, 8 Apr 2013 10:54:03 -0400 X-Originating-IP: 108.223.40.66 Date: Mon, 8 Apr 2013 07:54:07 -0700 From: Guenter Roeck To: Sebastian Hesselbarth Cc: Grant Likely , Rob Herring , Rob Landley , Mike Turquette , Stephen Warren , Thierry Reding , Dom Cobley , Linus Walleij , Arnd Bergmann , Andrew Morton , Pawel Moll , Mark Brown , Russell King - ARM Linux , Rabeeh Khoury , Daniel Mack , Jean-Francois Moine , Lars-Peter Clausen , devicetree-discuss@lists.ozlabs.org, linux-doc@vger.kernel.org, linux-kernel@vger.kernel.org, linux-arm-kernel@lists.infradead.org Subject: Re: [v5] clk: add si5351 i2c common clock driver Message-ID: <20130408145407.GA12243@roeck-us.net> References: <1365139415-17815-1-git-send-email-sebastian.hesselbarth@gmail.com> <20130407225046.GA16326@roeck-us.net> <51620604.4070204@gmail.com> <20130408001725.GA16648@roeck-us.net> <51625F9A.1050308@gmail.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <51625F9A.1050308@gmail.com> User-Agent: Mutt/1.5.21 (2010-09-15) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Mon, Apr 08, 2013 at 08:11:38AM +0200, Sebastian Hesselbarth wrote: > On 04/08/2013 02:17 AM, Guenter Roeck wrote: > >On Mon, Apr 08, 2013 at 01:49:24AM +0200, Sebastian Hesselbarth wrote: > >>On 04/08/2013 12:50 AM, Guenter Roeck wrote: > >>>On Fri, Apr 05, 2013 at 05:23:35AM -0000, Sebastian Hesselbarth wrote: > >>>>This patch adds a common clock driver for Silicon Labs Si5351a/b/c > >>>>i2c programmable clock generators. Currently, the driver supports > >>>>DT kernels only and VXCO feature of si5351b is not implemented. DT > >>>>bindings selectively allow to overwrite stored Si5351 configuration > >>>>which is very helpful for clock generators with empty eeprom > >>>>configuration. Corresponding device tree binding documentation is > >>>>also added. > >>>> > >>>>Signed-off-by: Sebastian Hesselbarth > >>>>Tested-by: Daniel Mack > >>>> > >>>[ ... ] > >>> > >>>>+static inline void _si5351_msynth_set_pll_master( > >>>>+ struct si5351_driver_data *drvdata, unsigned char num, int is_master) > >>>>+{ > >>>>+ unsigned long flags; > >>>>+ > >>>>+ if (num> 8 || > >>>>+ (drvdata->variant == SI5351_VARIANT_A3&& num> 3)) > >>>>+ return; > >>>>+ > >>>>+ flags = __clk_get_flags(drvdata->msynth[num].hw.clk); > >>>>+ if (is_master) > >>>>+ flags |= CLK_SET_RATE_PARENT; > >>>>+ else > >>>>+ flags&= ~CLK_SET_RATE_PARENT; > >>>>+ __clk_set_flags(drvdata->msynth[num].hw.clk, flags); > >>>>+} > >>>>+ > >>>Unless I am missing something, neither __clk_get_flags() nor the new > >>>__clk_set_flags is exported. > >>> > >>>Did you try to build and load the driver as module ? > >> > >>Well, good catch. I didn't try to build v5 as a module, but I guess it > >>will fail. But I consider this as something that has to be addressed in > >>clk framework itself, not in this patch. There will be other > >>clk-providers built as module in the future for sure. > >> > >Sure, but you provided the patch to make __clk_set_flags global. To avoid > >build failures, I would suggest to either submit a patch to export the > >missing functions, or to remove the ability to build the driver as module. > > Actually, I knew that __clk_set_flags patch will not be accepted > before posting it ;) > Ah, but part of that is to get you to think about it again, and to defend it if it is really needed. After all, "it can be abused" applies to pretty much every API. Key question is if you _really_ need run-time flag modifications, or if you can live with initialization-time settings. If you really need it, you'll have to explain the reasons. > >On a side note, do you happen to know anyone working on drivers for Si5319 or > >Si5368 ? > > No. Too bad ... I may have to write that code myself then. Thanks, Guenter From mboxrd@z Thu Jan 1 00:00:00 1970 From: Guenter Roeck Subject: Re: [v5] clk: add si5351 i2c common clock driver Date: Mon, 8 Apr 2013 07:54:07 -0700 Message-ID: <20130408145407.GA12243@roeck-us.net> References: <1365139415-17815-1-git-send-email-sebastian.hesselbarth@gmail.com> <20130407225046.GA16326@roeck-us.net> <51620604.4070204@gmail.com> <20130408001725.GA16648@roeck-us.net> <51625F9A.1050308@gmail.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Content-Disposition: inline In-Reply-To: <51625F9A.1050308-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: devicetree-discuss-bounces+gldd-devicetree-discuss=m.gmane.org-uLR06cmDAlY/bJ5BZ2RsiQ@public.gmane.org Sender: "devicetree-discuss" To: Sebastian Hesselbarth Cc: Jean-Francois Moine , Mark Brown , Stephen Warren , Pawel Moll , linux-doc-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ@public.gmane.org, linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, Rob Herring , Russell King - ARM Linux , Dom Cobley , Mike Turquette , Andrew Morton , Rabeeh Khoury , Lars-Peter Clausen , linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org List-Id: devicetree@vger.kernel.org On Mon, Apr 08, 2013 at 08:11:38AM +0200, Sebastian Hesselbarth wrote: > On 04/08/2013 02:17 AM, Guenter Roeck wrote: > >On Mon, Apr 08, 2013 at 01:49:24AM +0200, Sebastian Hesselbarth wrote: > >>On 04/08/2013 12:50 AM, Guenter Roeck wrote: > >>>On Fri, Apr 05, 2013 at 05:23:35AM -0000, Sebastian Hesselbarth wrote: > >>>>This patch adds a common clock driver for Silicon Labs Si5351a/b/c > >>>>i2c programmable clock generators. Currently, the driver supports > >>>>DT kernels only and VXCO feature of si5351b is not implemented. DT > >>>>bindings selectively allow to overwrite stored Si5351 configuration > >>>>which is very helpful for clock generators with empty eeprom > >>>>configuration. Corresponding device tree binding documentation is > >>>>also added. > >>>> > >>>>Signed-off-by: Sebastian Hesselbarth > >>>>Tested-by: Daniel Mack > >>>> > >>>[ ... ] > >>> > >>>>+static inline void _si5351_msynth_set_pll_master( > >>>>+ struct si5351_driver_data *drvdata, unsigned char num, int is_master) > >>>>+{ > >>>>+ unsigned long flags; > >>>>+ > >>>>+ if (num> 8 || > >>>>+ (drvdata->variant == SI5351_VARIANT_A3&& num> 3)) > >>>>+ return; > >>>>+ > >>>>+ flags = __clk_get_flags(drvdata->msynth[num].hw.clk); > >>>>+ if (is_master) > >>>>+ flags |= CLK_SET_RATE_PARENT; > >>>>+ else > >>>>+ flags&= ~CLK_SET_RATE_PARENT; > >>>>+ __clk_set_flags(drvdata->msynth[num].hw.clk, flags); > >>>>+} > >>>>+ > >>>Unless I am missing something, neither __clk_get_flags() nor the new > >>>__clk_set_flags is exported. > >>> > >>>Did you try to build and load the driver as module ? > >> > >>Well, good catch. I didn't try to build v5 as a module, but I guess it > >>will fail. But I consider this as something that has to be addressed in > >>clk framework itself, not in this patch. There will be other > >>clk-providers built as module in the future for sure. > >> > >Sure, but you provided the patch to make __clk_set_flags global. To avoid > >build failures, I would suggest to either submit a patch to export the > >missing functions, or to remove the ability to build the driver as module. > > Actually, I knew that __clk_set_flags patch will not be accepted > before posting it ;) > Ah, but part of that is to get you to think about it again, and to defend it if it is really needed. After all, "it can be abused" applies to pretty much every API. Key question is if you _really_ need run-time flag modifications, or if you can live with initialization-time settings. If you really need it, you'll have to explain the reasons. > >On a side note, do you happen to know anyone working on drivers for Si5319 or > >Si5368 ? > > No. Too bad ... I may have to write that code myself then. Thanks, Guenter From mboxrd@z Thu Jan 1 00:00:00 1970 From: linux@roeck-us.net (Guenter Roeck) Date: Mon, 8 Apr 2013 07:54:07 -0700 Subject: [v5] clk: add si5351 i2c common clock driver In-Reply-To: <51625F9A.1050308@gmail.com> References: <1365139415-17815-1-git-send-email-sebastian.hesselbarth@gmail.com> <20130407225046.GA16326@roeck-us.net> <51620604.4070204@gmail.com> <20130408001725.GA16648@roeck-us.net> <51625F9A.1050308@gmail.com> Message-ID: <20130408145407.GA12243@roeck-us.net> To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org On Mon, Apr 08, 2013 at 08:11:38AM +0200, Sebastian Hesselbarth wrote: > On 04/08/2013 02:17 AM, Guenter Roeck wrote: > >On Mon, Apr 08, 2013 at 01:49:24AM +0200, Sebastian Hesselbarth wrote: > >>On 04/08/2013 12:50 AM, Guenter Roeck wrote: > >>>On Fri, Apr 05, 2013 at 05:23:35AM -0000, Sebastian Hesselbarth wrote: > >>>>This patch adds a common clock driver for Silicon Labs Si5351a/b/c > >>>>i2c programmable clock generators. Currently, the driver supports > >>>>DT kernels only and VXCO feature of si5351b is not implemented. DT > >>>>bindings selectively allow to overwrite stored Si5351 configuration > >>>>which is very helpful for clock generators with empty eeprom > >>>>configuration. Corresponding device tree binding documentation is > >>>>also added. > >>>> > >>>>Signed-off-by: Sebastian Hesselbarth > >>>>Tested-by: Daniel Mack > >>>> > >>>[ ... ] > >>> > >>>>+static inline void _si5351_msynth_set_pll_master( > >>>>+ struct si5351_driver_data *drvdata, unsigned char num, int is_master) > >>>>+{ > >>>>+ unsigned long flags; > >>>>+ > >>>>+ if (num> 8 || > >>>>+ (drvdata->variant == SI5351_VARIANT_A3&& num> 3)) > >>>>+ return; > >>>>+ > >>>>+ flags = __clk_get_flags(drvdata->msynth[num].hw.clk); > >>>>+ if (is_master) > >>>>+ flags |= CLK_SET_RATE_PARENT; > >>>>+ else > >>>>+ flags&= ~CLK_SET_RATE_PARENT; > >>>>+ __clk_set_flags(drvdata->msynth[num].hw.clk, flags); > >>>>+} > >>>>+ > >>>Unless I am missing something, neither __clk_get_flags() nor the new > >>>__clk_set_flags is exported. > >>> > >>>Did you try to build and load the driver as module ? > >> > >>Well, good catch. I didn't try to build v5 as a module, but I guess it > >>will fail. But I consider this as something that has to be addressed in > >>clk framework itself, not in this patch. There will be other > >>clk-providers built as module in the future for sure. > >> > >Sure, but you provided the patch to make __clk_set_flags global. To avoid > >build failures, I would suggest to either submit a patch to export the > >missing functions, or to remove the ability to build the driver as module. > > Actually, I knew that __clk_set_flags patch will not be accepted > before posting it ;) > Ah, but part of that is to get you to think about it again, and to defend it if it is really needed. After all, "it can be abused" applies to pretty much every API. Key question is if you _really_ need run-time flag modifications, or if you can live with initialization-time settings. If you really need it, you'll have to explain the reasons. > >On a side note, do you happen to know anyone working on drivers for Si5319 or > >Si5368 ? > > No. Too bad ... I may have to write that code myself then. Thanks, Guenter