lkml.org 
[lkml]   [2014]   [Dec]   [21]   [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 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.

    >
    > Regards,
    > Mike
    \
     
     \ /
      Last update: 2014-12-22 04:01    [W:7.240 / U:0.000 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site