Hi Mike, Thanks for the acknowledgement. The clk will be consumed by the desginware i2c controller.
Warm Regards, Raymond Tan Software Engineer Malaysia IT Flex Services INET: 8-253-0075 Flex Website: http://flexservices.intel.com > -----Original Message----- > From: Mike Turquette [mailto:mturque...@linaro.org] > Sent: Wednesday, January 21, 2015 1:30 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 Tan, Raymond (2014-12-21 18:33:42) > > Hi Mike, > > > > Thanks for your reply. I've answered the questions as below. > > > > Warm Regards, > > > > Raymond Tan > > > > > -----Original Message----- > > > From: Mike Turquette [mailto:mturque...@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 <andriy.shevche...@linux.intel.com> > > > > Signed-off-by: Weike Chen <alvin.c...@intel.com> > > > > Signed-off-by: Raymond Tan <raymond....@intel.com> > > > > --- > > > > 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 > > > > > > <snip> > > > > > > > +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. > > Thanks for the explanation. One final question, who consumes this clock? > > The clk bits of the driver look good to me so please add my: > > Acked-by: Michael Turquette <mturque...@linaro.org> > > Thanks, > Mike > > > > > > > > > Regards, > > > Mike
