Crap! Forgot to Cc Mike.
Fingers faster than brain.
> Mike,
>
> Something for you down below.
>
> > > > 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 | 11 ++
> > > > drivers/mfd/Makefile | 1 +
> > > > drivers/mfd/intel_quark_i2c_gpio.c | 298
> > > > ++++++++++++++++++++++++++++++++++++
> > > > 3 files changed, 310 insertions(+)
> > > > create mode 100644 drivers/mfd/intel_quark_i2c_gpio.c
>
> [...]
>
> > > > + depends on COMMON_CLK
> > >
> > > I don't think you should depend on this. Just use the API and fail if it
> > > doesn't
> > > find the clock you're after.
> >
> > This was added to make sure the clk initialization and other clk related
> > calls will work with the MFD.
>
> [...]
>
> > > > +struct clk *intel_quark_i2c_clk;
> > > > +struct clk_lookup *intel_quark_i2c_clk_lookups;
> > >
> > > Why do these need to be global?
> >
> > I was trying to keep these as runtime allocated variables, and cleaned up
> > with removal of the driver / upon error handling.
>
> They still can be. But they need to be in a struct somewhere and not
> global.
>
> [...]
>
> > > > +static int intel_quark_register_i2c_clk(struct pci_dev *pdev) {
> > > > + intel_quark_i2c_clk_lookups = devm_kcalloc(
> > > > +
> > > > + &pdev->dev, INTEL_QUARK_I2C_NCLK,
> > > > + sizeof(*intel_quark_i2c_clk_lookups), GFP_KERNEL);
> > > > +
> > > > + if (!intel_quark_i2c_clk_lookups)
> > > > + return -ENOMEM;
> > > > +
> > > > + intel_quark_i2c_clk_lookups[0].dev_id =
> > > > +INTEL_QUARK_I2C_CONTROLLER_CLK;
> > > > +
> > > > + intel_quark_i2c_clk = clk_register_fixed_rate(
> > > > + &pdev->dev, INTEL_QUARK_I2C_CONTROLLER_CLK, NULL,
> > > > + CLK_IS_ROOT, INTEL_QUARK_I2C_CLK_HZ);
> > > > +
> > > > + return clk_register_clkdevs(intel_quark_i2c_clk,
> > > > + intel_quark_i2c_clk_lookups, INTEL_QUARK_I2C_NCLK);
> > >
> > > We don't normally register clks from MFD. Normally they are registered in
> > > drivers/clk and fetched when a device requires them.
> > > Why is this different?
> >
> > This is a static clk and on this platform, there's no clk hardware
> > to have the clk driver to initialize this and add into the global
> > struct for use
> > later by i2c controller. The hardware on the platform itself is
> > same, and the current driver (i2c_designware_platform) requires the
> > clk
> > for its operation. Due to this, I decided to have the clk initialize
> > in the MFD before the device being added, which later will trigger
> > the probe on the i2c controller driver.
>
> Mike,
>
> Can I get your input on this please?
>
> [...]
>
> > > > + pdata = intel_quark_get_i2c_mode(pdev);
> > > > + if (IS_ERR(pdata))
> > > > + return PTR_ERR(pdata);
> > >
> > > Bring this functionality into here?
> >
> > Were you referring the contents of intel_quark_get_i2c_mode(), and avoid
> > the additional function call instead?
>
> Right. Please rid intel_quark_get_i2c_mode().
>
> [...]
>
--
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
