lkml.org 
[lkml]   [2015]   [Jan]   [26]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
SubjectRE: [PATCH v3 1/1] mfd: intel_quark_i2c_gpio: Add Intel Quark X1000 I2C-GPIO MFD Driver
Date
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:mturquette@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: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 <andriy.shevchenko@linux.intel.com>
> > > > Signed-off-by: Weike Chen <alvin.chen@intel.com>
> > > > Signed-off-by: Raymond Tan <raymond.tan@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 <mturquette@linaro.org>
>
> Thanks,
> Mike
>
> >
> > >
> > > Regards,
> > > Mike
\
 
 \ /
  Last update: 2015-01-26 15:41    [W:0.045 / U:0.600 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site