Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753867AbaLVCet (ORCPT ); Sun, 21 Dec 2014 21:34:49 -0500 Received: from mga09.intel.com ([134.134.136.24]:21376 "EHLO mga09.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753633AbaLVCes (ORCPT ); Sun, 21 Dec 2014 21:34:48 -0500 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.07,620,1413270000"; d="scan'208";a="658210033" From: "Tan, Raymond" To: Mike Turquette , Lee Jones , Samuel Ortiz CC: "linux-kernel@vger.kernel.org" , "Chen, Alvin" , "Shevchenko, Andriy" , "Tan, Raymond" Subject: RE: [PATCH v3 1/1] mfd: intel_quark_i2c_gpio: Add Intel Quark X1000 I2C-GPIO MFD Driver Thread-Topic: [PATCH v3 1/1] mfd: intel_quark_i2c_gpio: Add Intel Quark X1000 I2C-GPIO MFD Driver Thread-Index: AQHQFScZb+ZefmlWzUame3dicNkxRpyKcpeAgBB8lsA= Date: Mon, 22 Dec 2014 02:33:42 +0000 Message-ID: References: <1418290710-5871-1-git-send-email-raymond.tan@intel.com> <1418290710-5871-2-git-send-email-raymond.tan@intel.com> <20141211222627.20398.48669@quantum> In-Reply-To: <20141211222627.20398.48669@quantum> Accept-Language: en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-originating-ip: [172.30.20.205] Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Transfer-Encoding: 8bit X-MIME-Autoconverted: from base64 to 8bit by nfs id sBM2Yxhp028754 Hi Mike, Thanks for your reply. I've answered the questions as below. Warm Regards, Raymond Tan > -----Original Message----- > From: Mike Turquette [mailto:mturquette@linaro.org] > Sent: Friday, December 12, 2014 6:26 AM > To: Tan, Raymond; Lee Jones; Samuel Ortiz > Cc: linux-kernel@vger.kernel.org; Chen, Alvin; Shevchenko, Andriy; Tan, > Raymond > Subject: Re: [PATCH v3 1/1] mfd: intel_quark_i2c_gpio: Add Intel Quark > X1000 I2C-GPIO MFD Driver > > Quoting Raymond Tan (2014-12-11 01:38:30) > > In Quark X1000, there's a single PCI device that provides both an I2C > > controller and a GPIO controller. This MFD driver will split the 2 > > devices for their respective drivers. > > > > This patch is based on Josef Ahmad's initial work for Quark enabling. > > > > Reviewed-by: Andy Shevchenko > > Signed-off-by: Weike Chen > > Signed-off-by: Raymond Tan > > --- > > drivers/mfd/Kconfig | 12 ++ > > drivers/mfd/Makefile | 1 + > > drivers/mfd/intel_quark_i2c_gpio.c | 279 > > ++++++++++++++++++++++++++++++++++++ > > 3 files changed, 292 insertions(+) > > create mode 100644 drivers/mfd/intel_quark_i2c_gpio.c > > > > > +static int intel_quark_register_i2c_clk(struct intel_quark_mfd > > +*quark_mfd) { > > + struct pci_dev *pdev = quark_mfd->pdev; > > + struct clk_lookup *i2c_clk_lookup; > > + struct clk *i2c_clk; > > + int retval; > > + > > + i2c_clk_lookup = devm_kcalloc( > > + &pdev->dev, INTEL_QUARK_I2C_NCLK, > > + sizeof(*i2c_clk_lookup), GFP_KERNEL); > > + > > + if (!i2c_clk_lookup) > > + return -ENOMEM; > > + > > + i2c_clk_lookup[0].dev_id = INTEL_QUARK_I2C_CONTROLLER_CLK; > > + > > + i2c_clk = clk_register_fixed_rate( > > + &pdev->dev, INTEL_QUARK_I2C_CONTROLLER_CLK, NULL, > > + CLK_IS_ROOT, INTEL_QUARK_I2C_CLK_HZ); > > + > > + quark_mfd->i2c_clk_lookup = i2c_clk_lookup; > > + quark_mfd->i2c_clk = i2c_clk; > > + > > + retval = clk_register_clkdevs(i2c_clk, i2c_clk_lookup, > > + INTEL_QUARK_I2C_NCLK); > > Lee asked about this in V2, so I'll follow up here in V3. It is OK for a driver to > use the clock provider api to register clocks with the clk framework if that > device truly is the provider of that clock signal. A good example can be found > here: > > drivers/media/platform/omap3isp/isp.c > > The OMAP3 ISP receives a clock signal as a input. Within the image signal > processor IP block it also has some basic clock controls of it's own which it > feeds to downstream IP blocks. As such it is both a clock consumer and a > provider and this is a common pattern amongst SoC designs. Thanks for the reference, however the mfd driver is purely a clk provider in this case. > > So my question for this driver is if i2c_clk is provided by whatever the hell this > mfd device is supposed to be, or if it's just a convenient place to call the code? As you've noticed, this is a fixed clock which only consumed by the I2C controller. Following the structure of the designware i2c controller device driver, a clk is needed for it, and on this platform, it is a fixed clk. I'm putting the clk functions in this mfd driver is due to the fact that, this mfd driver is splitting the function of the PCI device to 2 controllers downstream. > > Another concern is that fact that this is a fixed clock. For architectures that > use device tree to desribe board topology (ARM, MIPS, > PPC) it is common to simply put the fixed-rate clocks there and not directly > into the drive code. This prevents having to hack a lot of conditionals into > your driver when rev 2.0 of your hardware comes out with a faster fixed rate > clock, but you still need to support 1.0 hardware users at the slower rate. I > don't know if x86 has a similar way of describing board topology but it might > something to look into. I checked the kernel source for x86 arch, sadly there's no similar implementation of fixed clk being developed/written on the architectures code. That being said, for this platform, we do have a separate platform board file for those onboard peripherals, do you think that it's better I put the clk function under the board file instead? My reasoning behind is if I were to introduce clk in general to x86 in this way, it's effect will be on x86 unless I introduce further checking during compilation / runtime. > > Regards, > Mike ????{.n?+???????+%?????ݶ??w??{.n?+????{??G?????{ay?ʇڙ?,j??f???h?????????z_??(?階?ݢj"???m??????G????????????&???~???iO???z??v?^?m????????????I?