* Re: [PATCH v3 1/1] mfd: intel_quark_i2c_gpio: Add Intel Quark X1000 I2C-GPIO MFD Driver
2014-12-11 9:38 ` [PATCH v3 1/1] " Raymond Tan
@ 2014-12-11 10:27 ` Shevchenko, Andriy
2015-01-06 3:13 ` Tan, Raymond
2014-12-11 22:26 ` Mike Turquette
2015-01-20 12:47 ` Lee Jones
2 siblings, 1 reply; 14+ messages in thread
From: Shevchenko, Andriy @ 2014-12-11 10:27 UTC (permalink / raw)
To: Tan, Raymond
Cc: mturquette@linaro.org, sameo@linux.intel.com,
linux-kernel@vger.kernel.org, Chen, Alvin, lee.jones@linaro.org
[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 11999 bytes --]
On Thu, 2014-12-11 at 17:38 +0800, Raymond Tan wrote:
> 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.
See my comments below.
>
> 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 ++++++++++++++++++++++++++++++++++++
So, there is a proposal to bring the order into the source tree a bit
for Intel related MFD drivers.
What about to move this module to
drivers/mfd/intel/quark/i2c_gpio.c ?
At least what I can tell now, it will come more Intel stuff and we have
few drivers already there.
I can see the following:
drivers/mfd/intel/
-> quark/
-> mid/ (Intel MID platforms, existing drivers)
-> some new platform/ (whatever is the one of the new Intel SoCs)
Lee, what's your opinion?
I could change a layout for existing in-tree drivers.
> 3 files changed, 292 insertions(+)
> create mode 100644 drivers/mfd/intel_quark_i2c_gpio.c
>
> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
> index b7c74a7..2f2f62d 100644
> --- a/drivers/mfd/Kconfig
> +++ b/drivers/mfd/Kconfig
> @@ -219,6 +219,18 @@ config HTC_I2CPLD
> This device provides input and output GPIOs through an I2C
> interface to one or more sub-chips.
>
> +config MFD_INTEL_QUARK_I2C_GPIO
Depending on the further discussion about layout.
> + tristate "Intel Quark MFD I2C GPIO"
> + depends on PCI
> + depends on X86
> + depends on COMMON_CLK
> + select MFD_CORE
> + help
> + This MFD provides support for I2C and GPIO that exist only
> + in a single PCI device. It splits the 2 IO devices to
> + their respective IO driver.
> + The GPIO exports a total amount of 8 interrupt-capable GPIOs.
> +
> config LPC_ICH
> tristate "Intel ICH LPC"
> depends on PCI
> diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
> index 8a28dc9..d42652d 100644
> --- a/drivers/mfd/Makefile
> +++ b/drivers/mfd/Makefile
> @@ -133,6 +133,7 @@ obj-$(CONFIG_AB8500_CORE) += ab8500-core.o ab8500-sysctrl.o
> obj-$(CONFIG_MFD_TIMBERDALE) += timberdale.o
> obj-$(CONFIG_PMIC_ADP5520) += adp5520.o
> obj-$(CONFIG_MFD_KEMPLD) += kempld-core.o
> +obj-$(CONFIG_MFD_INTEL_QUARK_I2C_GPIO) += intel_quark_i2c_gpio.o
> obj-$(CONFIG_LPC_SCH) += lpc_sch.o
> obj-$(CONFIG_LPC_ICH) += lpc_ich.o
> obj-$(CONFIG_MFD_RDC321X) += rdc321x-southbridge.o
> diff --git a/drivers/mfd/intel_quark_i2c_gpio.c b/drivers/mfd/intel_quark_i2c_gpio.c
> new file mode 100644
> index 0000000..a3a7043
> --- /dev/null
> +++ b/drivers/mfd/intel_quark_i2c_gpio.c
> @@ -0,0 +1,279 @@
> +/*
> + * Intel Quark MFD PCI driver for I2C & GPIO
> + *
> + * Copyright(c) 2014 Intel Corporation.
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms and conditions of the GNU General Public License,
> + * version 2, as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope it will be useful, but WITHOUT
> + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
> + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for
> + * more details.
> + *
> + * Intel Quark PCI device for I2C and GPIO controller sharing the same
> + * PCI function. This PCI driver will split the 2 devices into their
> + * respective drivers.
> + */
> +
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/pci.h>
> +#include <linux/mfd/core.h>
> +#include <linux/clkdev.h>
> +#include <linux/clk-provider.h>
> +#include <linux/dmi.h>
> +#include <linux/platform_data/gpio-dwapb.h>
> +#include <linux/platform_data/i2c-designware.h>
> +
> +/* PCI BAR for register base address */
> +#define MFD_I2C_BAR 0
> +#define MFD_GPIO_BAR 1
> +
> +/* The base GPIO number under GPIOLIB framework */
> +#define INTEL_QUARK_MFD_GPIO_BASE 8
> +
> +/* The default number of South-Cluster GPIO on Quark. */
> +#define INTEL_QUARK_MFD_NGPIO 8
> +
> +/* The DesignWare GPIO ports on Quark. */
> +#define INTEL_QUARK_GPIO_NPORTS 1
> +
> +#define INTEL_QUARK_IORES_MEM 0
> +#define INTEL_QUARK_IORES_IRQ 1
> +
> +#define INTEL_QUARK_I2C_CONTROLLER_CLK "i2c_designware.0"
> +
> +/* The Quark I2C controller source clock */
> +#define INTEL_QUARK_I2C_CLK_HZ 33000000
> +
> +#define INTEL_QUARK_I2C_NCLK 1
> +
> +struct intel_quark_mfd {
> + struct pci_dev *pdev;
> + struct clk *i2c_clk;
> + struct clk_lookup *i2c_clk_lookup;
> +};
> +
> +struct i2c_mode_info {
> + const char *name;
> + unsigned int i2c_scl_freq;
> +};
> +
> +static const struct i2c_mode_info platform_i2c_mode_info[] = {
> + {
> + .name = "Galileo",
> + .i2c_scl_freq = 100000,
> + },
> + {
> + .name = "GalileoGen2",
> + .i2c_scl_freq = 400000,
> + },
> +};
> +
> +static struct resource intel_quark_i2c_res[] = {
> + [INTEL_QUARK_IORES_MEM] = {
> + .flags = IORESOURCE_MEM,
> + },
> + [INTEL_QUARK_IORES_IRQ] = {
> + .flags = IORESOURCE_IRQ,
> + },
> +};
> +
> +static struct resource intel_quark_gpio_res[] = {
> + [INTEL_QUARK_IORES_MEM] = {
> + .flags = IORESOURCE_MEM,
> + },
> +};
> +
> +static struct mfd_cell intel_quark_mfd_cells[] = {
> + {
> + .id = MFD_I2C_BAR,
> + .name = "i2c_designware",
> + .num_resources = ARRAY_SIZE(intel_quark_i2c_res),
> + .resources = intel_quark_i2c_res,
> + .ignore_resource_conflicts = true,
> + },
> + {
> + .id = MFD_GPIO_BAR,
> + .name = "gpio-dwapb",
> + .num_resources = ARRAY_SIZE(intel_quark_gpio_res),
> + .resources = intel_quark_gpio_res,
> + .ignore_resource_conflicts = true,
> + },
> +};
> +
> +static const struct pci_device_id intel_quark_mfd_ids[] = {
> + { PCI_VDEVICE(INTEL, 0x0934), },
> + { 0,}
> +};
> +MODULE_DEVICE_TABLE(pci, intel_quark_mfd_ids);
> +
> +static int intel_quark_register_i2c_clk(struct intel_quark_mfd *quark_mfd)
What about quark_mfd -> mfd ? This prefix is kinda useless inside the
functions in this module.
> +{
> + 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);
> + if (retval)
> + dev_err(&pdev->dev, "Fixed clk register failed: %d\n", retval);
> +
> + return retval;
> +}
> +
> +static void intel_quark_unregister_i2c_clk(struct pci_dev *pdev)
> +{
> + struct intel_quark_mfd *quark_mfd = dev_get_drvdata(&pdev->dev);
Ditto.
> +
> + if (!quark_mfd->i2c_clk || !quark_mfd->i2c_clk_lookup)
> + return;
> +
> + clkdev_drop(quark_mfd->i2c_clk_lookup);
> + clk_unregister(quark_mfd->i2c_clk);
> +}
> +
> +static int intel_quark_i2c_setup(struct pci_dev *pdev, struct mfd_cell *cell)
> +{
> + const char *board_name = dmi_get_system_info(DMI_BOARD_NAME);
> + struct dw_i2c_platform_data *pdata;
> + struct resource *res = (struct resource *)cell->resources;
Here you know what the exact resource you is going to change.
What if you update the global variable first, and assign it here to the
cell?
> + struct device *dev = &pdev->dev;
> + unsigned int i;
> +
> + res[INTEL_QUARK_IORES_MEM].start =
> + pci_resource_start(pdev, MFD_I2C_BAR);
> + res[INTEL_QUARK_IORES_MEM].end =
> + pci_resource_end(pdev, MFD_I2C_BAR);
> +
> + res[INTEL_QUARK_IORES_IRQ].start = pdev->irq;
> + res[INTEL_QUARK_IORES_IRQ].end = pdev->irq;
> +
> + pdata = devm_kzalloc(dev, sizeof(*pdata), GFP_KERNEL);
> + if (!pdata)
> + return -ENOMEM;
> +
> + /* Fast mode by default */
> + pdata->i2c_scl_freq = 400000;
> +
> + for (i = 0; i < ARRAY_SIZE(platform_i2c_mode_info); i++)
> + if (!strcmp(board_name, platform_i2c_mode_info[i].name))
> + pdata->i2c_scl_freq
> + = platform_i2c_mode_info[i].i2c_scl_freq;
> +
> + cell->platform_data = pdata;
> + cell->pdata_size = sizeof(*pdata);
> +
> + return 0;
> +}
> +
> +static int intel_quark_gpio_setup(struct pci_dev *pdev, struct mfd_cell *cell)
> +{
> + struct dwapb_platform_data *pdata;
> + struct resource *res = (struct resource *)cell->resources;
Same idea here.
> + struct device *dev = &pdev->dev;
> +
> + res[INTEL_QUARK_IORES_MEM].start =
> + pci_resource_start(pdev, MFD_GPIO_BAR);
> + res[INTEL_QUARK_IORES_MEM].end =
> + pci_resource_end(pdev, MFD_GPIO_BAR);
> +
> + pdata = devm_kzalloc(dev, sizeof(*pdata), GFP_KERNEL);
> + if (!pdata)
> + return -ENOMEM;
> +
> + /* For intel quark x1000, it has only one port: portA */
> + pdata->nports = INTEL_QUARK_GPIO_NPORTS;
> + pdata->properties = devm_kcalloc(dev, pdata->nports,
> + sizeof(*pdata->properties),
> + GFP_KERNEL);
> + if (!pdata->properties)
> + return -ENOMEM;
> +
> + /* Set the properties for portA */
> + pdata->properties->node = NULL;
> + pdata->properties->name = "intel-quark-x1000-gpio-portA";
> + pdata->properties->idx = 0;
> + pdata->properties->ngpio = INTEL_QUARK_MFD_NGPIO;
> + pdata->properties->gpio_base = INTEL_QUARK_MFD_GPIO_BASE;
> + pdata->properties->irq = pdev->irq;
> + pdata->properties->irq_shared = true;
> +
> + cell->platform_data = pdata;
> + cell->pdata_size = sizeof(*pdata);
> +
> + return 0;
> +}
> +
> +static int intel_quark_mfd_probe(struct pci_dev *pdev,
> + const struct pci_device_id *id)
> +{
> + struct intel_quark_mfd *quark_mfd;
> + int retval;
> +
> + retval = pcim_enable_device(pdev);
> + if (retval)
> + return retval;
> +
> + quark_mfd = devm_kzalloc(&pdev->dev, sizeof(*quark_mfd), GFP_KERNEL);
> + if (!quark_mfd)
> + return -ENOMEM;
> + quark_mfd->pdev = pdev;
> +
> + retval = intel_quark_register_i2c_clk(quark_mfd);
> + if (retval)
> + return retval;
> +
> + dev_set_drvdata(&pdev->dev, quark_mfd);
> +
> + retval = intel_quark_i2c_setup(pdev,
> + &intel_quark_mfd_cells[MFD_I2C_BAR]);
> + if (retval)
> + return retval;
> +
> + retval = intel_quark_gpio_setup(pdev,
> + &intel_quark_mfd_cells[MFD_GPIO_BAR]);
> + if (retval)
> + return retval;
> +
> + return mfd_add_devices(&pdev->dev, 0, intel_quark_mfd_cells,
> + ARRAY_SIZE(intel_quark_mfd_cells), NULL, 0, NULL);
> +}
> +
> +static void intel_quark_mfd_remove(struct pci_dev *pdev)
> +{
> + intel_quark_unregister_i2c_clk(pdev);
> + mfd_remove_devices(&pdev->dev);
> +}
> +
> +static struct pci_driver intel_quark_mfd_driver = {
> + .name = "intel_quark_mfd_i2c_gpio",
> + .id_table = intel_quark_mfd_ids,
> + .probe = intel_quark_mfd_probe,
> + .remove = intel_quark_mfd_remove,
> +};
> +
> +module_pci_driver(intel_quark_mfd_driver);
> +
> +MODULE_AUTHOR("Raymond Tan <raymond.tan@intel.com>");
> +MODULE_DESCRIPTION("Intel Quark MFD PCI driver for I2C & GPIO");
> +MODULE_LICENSE("GPL v2");
--
Andy Shevchenko <andriy.shevchenko@intel.com>
Intel Finland Oy
---------------------------------------------------------------------
Intel Finland Oy
Registered Address: PL 281, 00181 Helsinki
Business Identity Code: 0357606 - 4
Domiciled in Helsinki
This e-mail and any attachments may contain confidential material for
the sole use of the intended recipient(s). Any review or distribution
by others is strictly prohibited. If you are not the intended
recipient, please contact the sender and delete all copies.
ÿôèº{.nÇ+‰·Ÿ®‰†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a¢f£¢·hšïêÿ‘êçz_è®\x03(階ŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨èÚ&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥
^ permalink raw reply [flat|nested] 14+ messages in thread* RE: [PATCH v3 1/1] mfd: intel_quark_i2c_gpio: Add Intel Quark X1000 I2C-GPIO MFD Driver
2014-12-11 10:27 ` Shevchenko, Andriy
@ 2015-01-06 3:13 ` Tan, Raymond
2015-01-20 12:26 ` Lee Jones
0 siblings, 1 reply; 14+ messages in thread
From: Tan, Raymond @ 2015-01-06 3:13 UTC (permalink / raw)
To: Shevchenko, Andriy, lee.jones@linaro.org
Cc: mturquette@linaro.org, sameo@linux.intel.com,
linux-kernel@vger.kernel.org, Chen, Alvin, Tan, Raymond
[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 12967 bytes --]
Hi Lee Jones,
Happy New Year.
Regarding Andriy's suggestion to split and reorganize the MFD drivers for IA platform in the previous email, any thoughts from you? Should I proceed to work in splitting it or keep the structure as-is for now? Thanks.
Warm Regards,
 Raymond Tan
> -----Original Message-----
> From: Shevchenko, Andriy
> Sent: Thursday, December 11, 2014 6:27 PM
> To: Tan, Raymond
> Cc: mturquette@linaro.org; sameo@linux.intel.com; linux-
> kernel@vger.kernel.org; Chen, Alvin; lee.jones@linaro.org
> Subject: Re: [PATCH v3 1/1] mfd: intel_quark_i2c_gpio: Add Intel Quark
> X1000 I2C-GPIO MFD Driver
>
> On Thu, 2014-12-11 at 17:38 +0800, Raymond Tan wrote:
> > 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.
>
> See my comments below.
>
> >
> > 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
> > ++++++++++++++++++++++++++++++++++++
>
> So, there is a proposal to bring the order into the source tree a bit for Intel
> related MFD drivers.
>
> What about to move this module to
> drivers/mfd/intel/quark/i2c_gpio.c ?
>
> At least what I can tell now, it will come more Intel stuff and we have few
> drivers already there.
>
> I can see the following:
> drivers/mfd/intel/
> -> quark/
> -> mid/ (Intel MID platforms, existing drivers) -> some new platform/
> (whatever is the one of the new Intel SoCs)
>
> Lee, what's your opinion?
>
> I could change a layout for existing in-tree drivers.
>
> > 3 files changed, 292 insertions(+)
> > create mode 100644 drivers/mfd/intel_quark_i2c_gpio.c
> >
> > diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig index
> > b7c74a7..2f2f62d 100644
> > --- a/drivers/mfd/Kconfig
> > +++ b/drivers/mfd/Kconfig
> > @@ -219,6 +219,18 @@ config HTC_I2CPLD
> > This device provides input and output GPIOs through an I2C
> > interface to one or more sub-chips.
> >
> > +config MFD_INTEL_QUARK_I2C_GPIO
>
> Depending on the further discussion about layout.
>
> > + tristate "Intel Quark MFD I2C GPIO"
> > + depends on PCI
> > + depends on X86
> > + depends on COMMON_CLK
> > + select MFD_CORE
> > + help
> > + This MFD provides support for I2C and GPIO that exist only
> > + in a single PCI device. It splits the 2 IO devices to
> > + their respective IO driver.
> > + The GPIO exports a total amount of 8 interrupt-capable GPIOs.
> > +
> > config LPC_ICH
> > tristate "Intel ICH LPC"
> > depends on PCI
> > diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile index
> > 8a28dc9..d42652d 100644
> > --- a/drivers/mfd/Makefile
> > +++ b/drivers/mfd/Makefile
> > @@ -133,6 +133,7 @@ obj-$(CONFIG_AB8500_CORE) += ab8500-
> core.o ab8500-sysctrl.o
> > obj-$(CONFIG_MFD_TIMBERDALE) += timberdale.o
> > obj-$(CONFIG_PMIC_ADP5520) += adp5520.o
> > obj-$(CONFIG_MFD_KEMPLD) += kempld-core.o
> > +obj-$(CONFIG_MFD_INTEL_QUARK_I2C_GPIO) +=
> intel_quark_i2c_gpio.o
> > obj-$(CONFIG_LPC_SCH) += lpc_sch.o
> > obj-$(CONFIG_LPC_ICH) += lpc_ich.o
> > obj-$(CONFIG_MFD_RDC321X) += rdc321x-southbridge.o
> > diff --git a/drivers/mfd/intel_quark_i2c_gpio.c
> > b/drivers/mfd/intel_quark_i2c_gpio.c
> > new file mode 100644
> > index 0000000..a3a7043
> > --- /dev/null
> > +++ b/drivers/mfd/intel_quark_i2c_gpio.c
> > @@ -0,0 +1,279 @@
> > +/*
> > + * Intel Quark MFD PCI driver for I2C & GPIO
> > + *
> > + * Copyright(c) 2014 Intel Corporation.
> > + *
> > + * This program is free software; you can redistribute it and/or
> > +modify it
> > + * under the terms and conditions of the GNU General Public License,
> > + * version 2, as published by the Free Software Foundation.
> > + *
> > + * This program is distributed in the hope it will be useful, but
> > +WITHOUT
> > + * ANY WARRANTY; without even the implied warranty of
> MERCHANTABILITY
> > +or
> > + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public
> > +License for
> > + * more details.
> > + *
> > + * Intel Quark PCI device for I2C and GPIO controller sharing the
> > +same
> > + * PCI function. This PCI driver will split the 2 devices into their
> > + * respective drivers.
> > + */
> > +
> > +#include <linux/kernel.h>
> > +#include <linux/module.h>
> > +#include <linux/pci.h>
> > +#include <linux/mfd/core.h>
> > +#include <linux/clkdev.h>
> > +#include <linux/clk-provider.h>
> > +#include <linux/dmi.h>
> > +#include <linux/platform_data/gpio-dwapb.h>
> > +#include <linux/platform_data/i2c-designware.h>
> > +
> > +/* PCI BAR for register base address */
> > +#define MFD_I2C_BAR 0
> > +#define MFD_GPIO_BAR 1
> > +
> > +/* The base GPIO number under GPIOLIB framework */
> > +#define INTEL_QUARK_MFD_GPIO_BASE 8
> > +
> > +/* The default number of South-Cluster GPIO on Quark. */
> > +#define INTEL_QUARK_MFD_NGPIO 8
> > +
> > +/* The DesignWare GPIO ports on Quark. */
> > +#define INTEL_QUARK_GPIO_NPORTS 1
> > +
> > +#define INTEL_QUARK_IORES_MEM 0
> > +#define INTEL_QUARK_IORES_IRQ 1
> > +
> > +#define INTEL_QUARK_I2C_CONTROLLER_CLK "i2c_designware.0"
> > +
> > +/* The Quark I2C controller source clock */
> > +#define INTEL_QUARK_I2C_CLK_HZ 33000000
> > +
> > +#define INTEL_QUARK_I2C_NCLK 1
> > +
> > +struct intel_quark_mfd {
> > + struct pci_dev *pdev;
> > + struct clk *i2c_clk;
> > + struct clk_lookup *i2c_clk_lookup;
> > +};
> > +
> > +struct i2c_mode_info {
> > + const char *name;
> > + unsigned int i2c_scl_freq;
> > +};
> > +
> > +static const struct i2c_mode_info platform_i2c_mode_info[] = {
> > + {
> > + .name = "Galileo",
> > + .i2c_scl_freq = 100000,
> > + },
> > + {
> > + .name = "GalileoGen2",
> > + .i2c_scl_freq = 400000,
> > + },
> > +};
> > +
> > +static struct resource intel_quark_i2c_res[] = {
> > + [INTEL_QUARK_IORES_MEM] = {
> > + .flags = IORESOURCE_MEM,
> > + },
> > + [INTEL_QUARK_IORES_IRQ] = {
> > + .flags = IORESOURCE_IRQ,
> > + },
> > +};
> > +
> > +static struct resource intel_quark_gpio_res[] = {
> > + [INTEL_QUARK_IORES_MEM] = {
> > + .flags = IORESOURCE_MEM,
> > + },
> > +};
> > +
> > +static struct mfd_cell intel_quark_mfd_cells[] = {
> > + {
> > + .id = MFD_I2C_BAR,
> > + .name = "i2c_designware",
> > + .num_resources = ARRAY_SIZE(intel_quark_i2c_res),
> > + .resources = intel_quark_i2c_res,
> > + .ignore_resource_conflicts = true,
> > + },
> > + {
> > + .id = MFD_GPIO_BAR,
> > + .name = "gpio-dwapb",
> > + .num_resources = ARRAY_SIZE(intel_quark_gpio_res),
> > + .resources = intel_quark_gpio_res,
> > + .ignore_resource_conflicts = true,
> > + },
> > +};
> > +
> > +static const struct pci_device_id intel_quark_mfd_ids[] = {
> > + { PCI_VDEVICE(INTEL, 0x0934), },
> > + { 0,}
> > +};
> > +MODULE_DEVICE_TABLE(pci, intel_quark_mfd_ids);
> > +
> > +static int intel_quark_register_i2c_clk(struct intel_quark_mfd
> > +*quark_mfd)
>
> What about quark_mfd -> mfd ? This prefix is kinda useless inside the
> functions in this module.
>
> > +{
> > + 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);
> > + if (retval)
> > + dev_err(&pdev->dev, "Fixed clk register failed: %d\n",
> retval);
> > +
> > + return retval;
> > +}
> > +
> > +static void intel_quark_unregister_i2c_clk(struct pci_dev *pdev) {
> > + struct intel_quark_mfd *quark_mfd = dev_get_drvdata(&pdev-
> >dev);
>
> Ditto.
>
> > +
> > + if (!quark_mfd->i2c_clk || !quark_mfd->i2c_clk_lookup)
> > + return;
> > +
> > + clkdev_drop(quark_mfd->i2c_clk_lookup);
> > + clk_unregister(quark_mfd->i2c_clk);
> > +}
> > +
> > +static int intel_quark_i2c_setup(struct pci_dev *pdev, struct
> > +mfd_cell *cell) {
> > + const char *board_name =
> dmi_get_system_info(DMI_BOARD_NAME);
> > + struct dw_i2c_platform_data *pdata;
> > + struct resource *res = (struct resource *)cell->resources;
>
> Here you know what the exact resource you is going to change.
>
> What if you update the global variable first, and assign it here to the cell?
>
>
> > + struct device *dev = &pdev->dev;
> > + unsigned int i;
> > +
> > + res[INTEL_QUARK_IORES_MEM].start =
> > + pci_resource_start(pdev, MFD_I2C_BAR);
> > + res[INTEL_QUARK_IORES_MEM].end =
> > + pci_resource_end(pdev, MFD_I2C_BAR);
> > +
> > + res[INTEL_QUARK_IORES_IRQ].start = pdev->irq;
> > + res[INTEL_QUARK_IORES_IRQ].end = pdev->irq;
> > +
> > + pdata = devm_kzalloc(dev, sizeof(*pdata), GFP_KERNEL);
> > + if (!pdata)
> > + return -ENOMEM;
> > +
> > + /* Fast mode by default */
> > + pdata->i2c_scl_freq = 400000;
> > +
> > + for (i = 0; i < ARRAY_SIZE(platform_i2c_mode_info); i++)
> > + if (!strcmp(board_name, platform_i2c_mode_info[i].name))
> > + pdata->i2c_scl_freq
> > + = platform_i2c_mode_info[i].i2c_scl_freq;
> > +
> > + cell->platform_data = pdata;
> > + cell->pdata_size = sizeof(*pdata);
> > +
> > + return 0;
> > +}
> > +
> > +static int intel_quark_gpio_setup(struct pci_dev *pdev, struct
> > +mfd_cell *cell) {
> > + struct dwapb_platform_data *pdata;
> > + struct resource *res = (struct resource *)cell->resources;
>
> Same idea here.
>
> > + struct device *dev = &pdev->dev;
> > +
> > + res[INTEL_QUARK_IORES_MEM].start =
> > + pci_resource_start(pdev, MFD_GPIO_BAR);
> > + res[INTEL_QUARK_IORES_MEM].end =
> > + pci_resource_end(pdev, MFD_GPIO_BAR);
> > +
> > + pdata = devm_kzalloc(dev, sizeof(*pdata), GFP_KERNEL);
> > + if (!pdata)
> > + return -ENOMEM;
> > +
> > + /* For intel quark x1000, it has only one port: portA */
> > + pdata->nports = INTEL_QUARK_GPIO_NPORTS;
> > + pdata->properties = devm_kcalloc(dev, pdata->nports,
> > + sizeof(*pdata->properties),
> > + GFP_KERNEL);
> > + if (!pdata->properties)
> > + return -ENOMEM;
> > +
> > + /* Set the properties for portA */
> > + pdata->properties->node = NULL;
> > + pdata->properties->name = "intel-quark-x1000-gpio-portA";
> > + pdata->properties->idx = 0;
> > + pdata->properties->ngpio = INTEL_QUARK_MFD_NGPIO;
> > + pdata->properties->gpio_base = INTEL_QUARK_MFD_GPIO_BASE;
> > + pdata->properties->irq = pdev->irq;
> > + pdata->properties->irq_shared = true;
> > +
> > + cell->platform_data = pdata;
> > + cell->pdata_size = sizeof(*pdata);
> > +
> > + return 0;
> > +}
> > +
> > +static int intel_quark_mfd_probe(struct pci_dev *pdev,
> > + const struct pci_device_id *id)
> > +{
> > + struct intel_quark_mfd *quark_mfd;
> > + int retval;
> > +
> > + retval = pcim_enable_device(pdev);
> > + if (retval)
> > + return retval;
> > +
> > + quark_mfd = devm_kzalloc(&pdev->dev, sizeof(*quark_mfd),
> GFP_KERNEL);
> > + if (!quark_mfd)
> > + return -ENOMEM;
> > + quark_mfd->pdev = pdev;
> > +
> > + retval = intel_quark_register_i2c_clk(quark_mfd);
> > + if (retval)
> > + return retval;
> > +
> > + dev_set_drvdata(&pdev->dev, quark_mfd);
> > +
> > + retval = intel_quark_i2c_setup(pdev,
> > + &intel_quark_mfd_cells[MFD_I2C_BAR]);
> > + if (retval)
> > + return retval;
> > +
> > + retval = intel_quark_gpio_setup(pdev,
> > +
> &intel_quark_mfd_cells[MFD_GPIO_BAR]);
> > + if (retval)
> > + return retval;
> > +
> > + return mfd_add_devices(&pdev->dev, 0, intel_quark_mfd_cells,
> > + ARRAY_SIZE(intel_quark_mfd_cells), NULL, 0, NULL); }
> > +
> > +static void intel_quark_mfd_remove(struct pci_dev *pdev) {
> > + intel_quark_unregister_i2c_clk(pdev);
> > + mfd_remove_devices(&pdev->dev);
> > +}
> > +
> > +static struct pci_driver intel_quark_mfd_driver = {
> > + .name = "intel_quark_mfd_i2c_gpio",
> > + .id_table = intel_quark_mfd_ids,
> > + .probe = intel_quark_mfd_probe,
> > + .remove = intel_quark_mfd_remove,
> > +};
> > +
> > +module_pci_driver(intel_quark_mfd_driver);
> > +
> > +MODULE_AUTHOR("Raymond Tan <raymond.tan@intel.com>");
> > +MODULE_DESCRIPTION("Intel Quark MFD PCI driver for I2C & GPIO");
> > +MODULE_LICENSE("GPL v2");
>
>
> --
> Andy Shevchenko <andriy.shevchenko@intel.com> Intel Finland Oy
ÿôèº{.nÇ+‰·Ÿ®‰†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a¢f£¢·hšïêÿ‘êçz_è®\x03(階ŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨èÚ&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [PATCH v3 1/1] mfd: intel_quark_i2c_gpio: Add Intel Quark X1000 I2C-GPIO MFD Driver
2015-01-06 3:13 ` Tan, Raymond
@ 2015-01-20 12:26 ` Lee Jones
0 siblings, 0 replies; 14+ messages in thread
From: Lee Jones @ 2015-01-20 12:26 UTC (permalink / raw)
To: Tan, Raymond
Cc: Shevchenko, Andriy, mturquette@linaro.org, sameo@linux.intel.com,
linux-kernel@vger.kernel.org, Chen, Alvin
> Happy New Year.
And to you.
> Regarding Andriy's suggestion to split and reorganize the MFD drivers for IA platform in the previous email, any thoughts from you? Should I proceed to work in splitting it or keep the structure as-is for now? Thanks.
I will review it as it is for now. I need to take a closer look at
the proposal when I get down to it. I am currently only half way
through my post-vacation backlog.
> > -----Original Message-----
> > From: Shevchenko, Andriy
> > Sent: Thursday, December 11, 2014 6:27 PM
> > To: Tan, Raymond
> > Cc: mturquette@linaro.org; sameo@linux.intel.com; linux-
> > kernel@vger.kernel.org; Chen, Alvin; lee.jones@linaro.org
> > Subject: Re: [PATCH v3 1/1] mfd: intel_quark_i2c_gpio: Add Intel Quark
> > X1000 I2C-GPIO MFD Driver
> >
> > On Thu, 2014-12-11 at 17:38 +0800, Raymond Tan wrote:
> > > 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.
> >
> > See my comments below.
> >
> > >
> > > 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
> > > ++++++++++++++++++++++++++++++++++++
> >
> > So, there is a proposal to bring the order into the source tree a bit for Intel
> > related MFD drivers.
> >
> > What about to move this module to
> > drivers/mfd/intel/quark/i2c_gpio.c ?
> >
> > At least what I can tell now, it will come more Intel stuff and we have few
> > drivers already there.
> >
> > I can see the following:
> > drivers/mfd/intel/
> > -> quark/
> > -> mid/ (Intel MID platforms, existing drivers) -> some new platform/
> > (whatever is the one of the new Intel SoCs)
> >
> > Lee, what's your opinion?
> >
> > I could change a layout for existing in-tree drivers.
> >
> > > 3 files changed, 292 insertions(+)
> > > create mode 100644 drivers/mfd/intel_quark_i2c_gpio.c
> > >
> > > diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig index
> > > b7c74a7..2f2f62d 100644
> > > --- a/drivers/mfd/Kconfig
> > > +++ b/drivers/mfd/Kconfig
> > > @@ -219,6 +219,18 @@ config HTC_I2CPLD
> > > This device provides input and output GPIOs through an I2C
> > > interface to one or more sub-chips.
> > >
> > > +config MFD_INTEL_QUARK_I2C_GPIO
> >
> > Depending on the further discussion about layout.
> >
> > > + tristate "Intel Quark MFD I2C GPIO"
> > > + depends on PCI
> > > + depends on X86
> > > + depends on COMMON_CLK
> > > + select MFD_CORE
> > > + help
> > > + This MFD provides support for I2C and GPIO that exist only
> > > + in a single PCI device. It splits the 2 IO devices to
> > > + their respective IO driver.
> > > + The GPIO exports a total amount of 8 interrupt-capable GPIOs.
> > > +
> > > config LPC_ICH
> > > tristate "Intel ICH LPC"
> > > depends on PCI
> > > diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile index
> > > 8a28dc9..d42652d 100644
> > > --- a/drivers/mfd/Makefile
> > > +++ b/drivers/mfd/Makefile
> > > @@ -133,6 +133,7 @@ obj-$(CONFIG_AB8500_CORE) += ab8500-
> > core.o ab8500-sysctrl.o
> > > obj-$(CONFIG_MFD_TIMBERDALE) += timberdale.o
> > > obj-$(CONFIG_PMIC_ADP5520) += adp5520.o
> > > obj-$(CONFIG_MFD_KEMPLD) += kempld-core.o
> > > +obj-$(CONFIG_MFD_INTEL_QUARK_I2C_GPIO) +=
> > intel_quark_i2c_gpio.o
> > > obj-$(CONFIG_LPC_SCH) += lpc_sch.o
> > > obj-$(CONFIG_LPC_ICH) += lpc_ich.o
> > > obj-$(CONFIG_MFD_RDC321X) += rdc321x-southbridge.o
> > > diff --git a/drivers/mfd/intel_quark_i2c_gpio.c
> > > b/drivers/mfd/intel_quark_i2c_gpio.c
> > > new file mode 100644
> > > index 0000000..a3a7043
> > > --- /dev/null
> > > +++ b/drivers/mfd/intel_quark_i2c_gpio.c
> > > @@ -0,0 +1,279 @@
> > > +/*
> > > + * Intel Quark MFD PCI driver for I2C & GPIO
> > > + *
> > > + * Copyright(c) 2014 Intel Corporation.
> > > + *
> > > + * This program is free software; you can redistribute it and/or
> > > +modify it
> > > + * under the terms and conditions of the GNU General Public License,
> > > + * version 2, as published by the Free Software Foundation.
> > > + *
> > > + * This program is distributed in the hope it will be useful, but
> > > +WITHOUT
> > > + * ANY WARRANTY; without even the implied warranty of
> > MERCHANTABILITY
> > > +or
> > > + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public
> > > +License for
> > > + * more details.
> > > + *
> > > + * Intel Quark PCI device for I2C and GPIO controller sharing the
> > > +same
> > > + * PCI function. This PCI driver will split the 2 devices into their
> > > + * respective drivers.
> > > + */
> > > +
> > > +#include <linux/kernel.h>
> > > +#include <linux/module.h>
> > > +#include <linux/pci.h>
> > > +#include <linux/mfd/core.h>
> > > +#include <linux/clkdev.h>
> > > +#include <linux/clk-provider.h>
> > > +#include <linux/dmi.h>
> > > +#include <linux/platform_data/gpio-dwapb.h>
> > > +#include <linux/platform_data/i2c-designware.h>
> > > +
> > > +/* PCI BAR for register base address */
> > > +#define MFD_I2C_BAR 0
> > > +#define MFD_GPIO_BAR 1
> > > +
> > > +/* The base GPIO number under GPIOLIB framework */
> > > +#define INTEL_QUARK_MFD_GPIO_BASE 8
> > > +
> > > +/* The default number of South-Cluster GPIO on Quark. */
> > > +#define INTEL_QUARK_MFD_NGPIO 8
> > > +
> > > +/* The DesignWare GPIO ports on Quark. */
> > > +#define INTEL_QUARK_GPIO_NPORTS 1
> > > +
> > > +#define INTEL_QUARK_IORES_MEM 0
> > > +#define INTEL_QUARK_IORES_IRQ 1
> > > +
> > > +#define INTEL_QUARK_I2C_CONTROLLER_CLK "i2c_designware.0"
> > > +
> > > +/* The Quark I2C controller source clock */
> > > +#define INTEL_QUARK_I2C_CLK_HZ 33000000
> > > +
> > > +#define INTEL_QUARK_I2C_NCLK 1
> > > +
> > > +struct intel_quark_mfd {
> > > + struct pci_dev *pdev;
> > > + struct clk *i2c_clk;
> > > + struct clk_lookup *i2c_clk_lookup;
> > > +};
> > > +
> > > +struct i2c_mode_info {
> > > + const char *name;
> > > + unsigned int i2c_scl_freq;
> > > +};
> > > +
> > > +static const struct i2c_mode_info platform_i2c_mode_info[] = {
> > > + {
> > > + .name = "Galileo",
> > > + .i2c_scl_freq = 100000,
> > > + },
> > > + {
> > > + .name = "GalileoGen2",
> > > + .i2c_scl_freq = 400000,
> > > + },
> > > +};
> > > +
> > > +static struct resource intel_quark_i2c_res[] = {
> > > + [INTEL_QUARK_IORES_MEM] = {
> > > + .flags = IORESOURCE_MEM,
> > > + },
> > > + [INTEL_QUARK_IORES_IRQ] = {
> > > + .flags = IORESOURCE_IRQ,
> > > + },
> > > +};
> > > +
> > > +static struct resource intel_quark_gpio_res[] = {
> > > + [INTEL_QUARK_IORES_MEM] = {
> > > + .flags = IORESOURCE_MEM,
> > > + },
> > > +};
> > > +
> > > +static struct mfd_cell intel_quark_mfd_cells[] = {
> > > + {
> > > + .id = MFD_I2C_BAR,
> > > + .name = "i2c_designware",
> > > + .num_resources = ARRAY_SIZE(intel_quark_i2c_res),
> > > + .resources = intel_quark_i2c_res,
> > > + .ignore_resource_conflicts = true,
> > > + },
> > > + {
> > > + .id = MFD_GPIO_BAR,
> > > + .name = "gpio-dwapb",
> > > + .num_resources = ARRAY_SIZE(intel_quark_gpio_res),
> > > + .resources = intel_quark_gpio_res,
> > > + .ignore_resource_conflicts = true,
> > > + },
> > > +};
> > > +
> > > +static const struct pci_device_id intel_quark_mfd_ids[] = {
> > > + { PCI_VDEVICE(INTEL, 0x0934), },
> > > + { 0,}
> > > +};
> > > +MODULE_DEVICE_TABLE(pci, intel_quark_mfd_ids);
> > > +
> > > +static int intel_quark_register_i2c_clk(struct intel_quark_mfd
> > > +*quark_mfd)
> >
> > What about quark_mfd -> mfd ? This prefix is kinda useless inside the
> > functions in this module.
> >
> > > +{
> > > + 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);
> > > + if (retval)
> > > + dev_err(&pdev->dev, "Fixed clk register failed: %d\n",
> > retval);
> > > +
> > > + return retval;
> > > +}
> > > +
> > > +static void intel_quark_unregister_i2c_clk(struct pci_dev *pdev) {
> > > + struct intel_quark_mfd *quark_mfd = dev_get_drvdata(&pdev-
> > >dev);
> >
> > Ditto.
> >
> > > +
> > > + if (!quark_mfd->i2c_clk || !quark_mfd->i2c_clk_lookup)
> > > + return;
> > > +
> > > + clkdev_drop(quark_mfd->i2c_clk_lookup);
> > > + clk_unregister(quark_mfd->i2c_clk);
> > > +}
> > > +
> > > +static int intel_quark_i2c_setup(struct pci_dev *pdev, struct
> > > +mfd_cell *cell) {
> > > + const char *board_name =
> > dmi_get_system_info(DMI_BOARD_NAME);
> > > + struct dw_i2c_platform_data *pdata;
> > > + struct resource *res = (struct resource *)cell->resources;
> >
> > Here you know what the exact resource you is going to change.
> >
> > What if you update the global variable first, and assign it here to the cell?
> >
> >
> > > + struct device *dev = &pdev->dev;
> > > + unsigned int i;
> > > +
> > > + res[INTEL_QUARK_IORES_MEM].start =
> > > + pci_resource_start(pdev, MFD_I2C_BAR);
> > > + res[INTEL_QUARK_IORES_MEM].end =
> > > + pci_resource_end(pdev, MFD_I2C_BAR);
> > > +
> > > + res[INTEL_QUARK_IORES_IRQ].start = pdev->irq;
> > > + res[INTEL_QUARK_IORES_IRQ].end = pdev->irq;
> > > +
> > > + pdata = devm_kzalloc(dev, sizeof(*pdata), GFP_KERNEL);
> > > + if (!pdata)
> > > + return -ENOMEM;
> > > +
> > > + /* Fast mode by default */
> > > + pdata->i2c_scl_freq = 400000;
> > > +
> > > + for (i = 0; i < ARRAY_SIZE(platform_i2c_mode_info); i++)
> > > + if (!strcmp(board_name, platform_i2c_mode_info[i].name))
> > > + pdata->i2c_scl_freq
> > > + = platform_i2c_mode_info[i].i2c_scl_freq;
> > > +
> > > + cell->platform_data = pdata;
> > > + cell->pdata_size = sizeof(*pdata);
> > > +
> > > + return 0;
> > > +}
> > > +
> > > +static int intel_quark_gpio_setup(struct pci_dev *pdev, struct
> > > +mfd_cell *cell) {
> > > + struct dwapb_platform_data *pdata;
> > > + struct resource *res = (struct resource *)cell->resources;
> >
> > Same idea here.
> >
> > > + struct device *dev = &pdev->dev;
> > > +
> > > + res[INTEL_QUARK_IORES_MEM].start =
> > > + pci_resource_start(pdev, MFD_GPIO_BAR);
> > > + res[INTEL_QUARK_IORES_MEM].end =
> > > + pci_resource_end(pdev, MFD_GPIO_BAR);
> > > +
> > > + pdata = devm_kzalloc(dev, sizeof(*pdata), GFP_KERNEL);
> > > + if (!pdata)
> > > + return -ENOMEM;
> > > +
> > > + /* For intel quark x1000, it has only one port: portA */
> > > + pdata->nports = INTEL_QUARK_GPIO_NPORTS;
> > > + pdata->properties = devm_kcalloc(dev, pdata->nports,
> > > + sizeof(*pdata->properties),
> > > + GFP_KERNEL);
> > > + if (!pdata->properties)
> > > + return -ENOMEM;
> > > +
> > > + /* Set the properties for portA */
> > > + pdata->properties->node = NULL;
> > > + pdata->properties->name = "intel-quark-x1000-gpio-portA";
> > > + pdata->properties->idx = 0;
> > > + pdata->properties->ngpio = INTEL_QUARK_MFD_NGPIO;
> > > + pdata->properties->gpio_base = INTEL_QUARK_MFD_GPIO_BASE;
> > > + pdata->properties->irq = pdev->irq;
> > > + pdata->properties->irq_shared = true;
> > > +
> > > + cell->platform_data = pdata;
> > > + cell->pdata_size = sizeof(*pdata);
> > > +
> > > + return 0;
> > > +}
> > > +
> > > +static int intel_quark_mfd_probe(struct pci_dev *pdev,
> > > + const struct pci_device_id *id)
> > > +{
> > > + struct intel_quark_mfd *quark_mfd;
> > > + int retval;
> > > +
> > > + retval = pcim_enable_device(pdev);
> > > + if (retval)
> > > + return retval;
> > > +
> > > + quark_mfd = devm_kzalloc(&pdev->dev, sizeof(*quark_mfd),
> > GFP_KERNEL);
> > > + if (!quark_mfd)
> > > + return -ENOMEM;
> > > + quark_mfd->pdev = pdev;
> > > +
> > > + retval = intel_quark_register_i2c_clk(quark_mfd);
> > > + if (retval)
> > > + return retval;
> > > +
> > > + dev_set_drvdata(&pdev->dev, quark_mfd);
> > > +
> > > + retval = intel_quark_i2c_setup(pdev,
> > > + &intel_quark_mfd_cells[MFD_I2C_BAR]);
> > > + if (retval)
> > > + return retval;
> > > +
> > > + retval = intel_quark_gpio_setup(pdev,
> > > +
> > &intel_quark_mfd_cells[MFD_GPIO_BAR]);
> > > + if (retval)
> > > + return retval;
> > > +
> > > + return mfd_add_devices(&pdev->dev, 0, intel_quark_mfd_cells,
> > > + ARRAY_SIZE(intel_quark_mfd_cells), NULL, 0, NULL); }
> > > +
> > > +static void intel_quark_mfd_remove(struct pci_dev *pdev) {
> > > + intel_quark_unregister_i2c_clk(pdev);
> > > + mfd_remove_devices(&pdev->dev);
> > > +}
> > > +
> > > +static struct pci_driver intel_quark_mfd_driver = {
> > > + .name = "intel_quark_mfd_i2c_gpio",
> > > + .id_table = intel_quark_mfd_ids,
> > > + .probe = intel_quark_mfd_probe,
> > > + .remove = intel_quark_mfd_remove,
> > > +};
> > > +
> > > +module_pci_driver(intel_quark_mfd_driver);
> > > +
> > > +MODULE_AUTHOR("Raymond Tan <raymond.tan@intel.com>");
> > > +MODULE_DESCRIPTION("Intel Quark MFD PCI driver for I2C & GPIO");
> > > +MODULE_LICENSE("GPL v2");
> >
> >
--
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v3 1/1] mfd: intel_quark_i2c_gpio: Add Intel Quark X1000 I2C-GPIO MFD Driver
2014-12-11 9:38 ` [PATCH v3 1/1] " Raymond Tan
2014-12-11 10:27 ` Shevchenko, Andriy
@ 2014-12-11 22:26 ` Mike Turquette
2014-12-22 2:33 ` Tan, Raymond
2015-01-20 12:47 ` Lee Jones
2 siblings, 1 reply; 14+ messages in thread
From: Mike Turquette @ 2014-12-11 22:26 UTC (permalink / raw)
To: Raymond Tan, Lee Jones, Samuel Ortiz
Cc: linux-kernel, Alvin Chen, Andriy Shevchenko, Raymond Tan
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.
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?
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.
Regards,
Mike
^ permalink raw reply [flat|nested] 14+ messages in thread* RE: [PATCH v3 1/1] mfd: intel_quark_i2c_gpio: Add Intel Quark X1000 I2C-GPIO MFD Driver
2014-12-11 22:26 ` Mike Turquette
@ 2014-12-22 2:33 ` Tan, Raymond
2015-01-20 17:30 ` Mike Turquette
0 siblings, 1 reply; 14+ messages in thread
From: Tan, Raymond @ 2014-12-22 2:33 UTC (permalink / raw)
To: Mike Turquette, Lee Jones, Samuel Ortiz
Cc: linux-kernel@vger.kernel.org, Chen, Alvin, Shevchenko, Andriy,
Tan, Raymond
[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 4745 bytes --]
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
ÿôèº{.nÇ+‰·Ÿ®‰†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a¢f£¢·hšïêÿ‘êçz_è®\x03(階ŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨èÚ&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥
^ permalink raw reply [flat|nested] 14+ messages in thread* RE: [PATCH v3 1/1] mfd: intel_quark_i2c_gpio: Add Intel Quark X1000 I2C-GPIO MFD Driver
2014-12-22 2:33 ` Tan, Raymond
@ 2015-01-20 17:30 ` Mike Turquette
2015-01-26 14:28 ` Tan, Raymond
0 siblings, 1 reply; 14+ messages in thread
From: Mike Turquette @ 2015-01-20 17:30 UTC (permalink / raw)
To: Tan, Raymond, Lee Jones, Samuel Ortiz
Cc: linux-kernel@vger.kernel.org, Chen, Alvin, Shevchenko, Andriy,
Tan, Raymond
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
^ permalink raw reply [flat|nested] 14+ messages in thread* RE: [PATCH v3 1/1] mfd: intel_quark_i2c_gpio: Add Intel Quark X1000 I2C-GPIO MFD Driver
2015-01-20 17:30 ` Mike Turquette
@ 2015-01-26 14:28 ` Tan, Raymond
0 siblings, 0 replies; 14+ messages in thread
From: Tan, Raymond @ 2015-01-26 14:28 UTC (permalink / raw)
To: Mike Turquette, Lee Jones, Samuel Ortiz
Cc: linux-kernel@vger.kernel.org, Chen, Alvin, Shevchenko, Andriy,
Tan, Raymond
[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 6052 bytes --]
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
ÿôèº{.nÇ+‰·Ÿ®‰†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a¢f£¢·hšïêÿ‘êçz_è®\x03(階ŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨èÚ&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v3 1/1] mfd: intel_quark_i2c_gpio: Add Intel Quark X1000 I2C-GPIO MFD Driver
2014-12-11 9:38 ` [PATCH v3 1/1] " Raymond Tan
2014-12-11 10:27 ` Shevchenko, Andriy
2014-12-11 22:26 ` Mike Turquette
@ 2015-01-20 12:47 ` Lee Jones
2015-01-20 13:41 ` Shevchenko, Andriy
2015-01-20 17:31 ` Mike Turquette
2 siblings, 2 replies; 14+ messages in thread
From: Lee Jones @ 2015-01-20 12:47 UTC (permalink / raw)
To: Raymond Tan
Cc: Mike Turquette, Samuel Ortiz, linux-kernel, Alvin Chen,
Andriy Shevchenko
On Thu, 11 Dec 2014, Raymond Tan wrote:
> 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
>
> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
> index b7c74a7..2f2f62d 100644
> --- a/drivers/mfd/Kconfig
> +++ b/drivers/mfd/Kconfig
> @@ -219,6 +219,18 @@ config HTC_I2CPLD
> This device provides input and output GPIOs through an I2C
> interface to one or more sub-chips.
>
> +config MFD_INTEL_QUARK_I2C_GPIO
> + tristate "Intel Quark MFD I2C GPIO"
> + depends on PCI
> + depends on X86
> + depends on COMMON_CLK
> + select MFD_CORE
> + help
> + This MFD provides support for I2C and GPIO that exist only
> + in a single PCI device. It splits the 2 IO devices to
> + their respective IO driver.
> + The GPIO exports a total amount of 8 interrupt-capable GPIOs.
> +
> config LPC_ICH
> tristate "Intel ICH LPC"
> depends on PCI
> diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
> index 8a28dc9..d42652d 100644
> --- a/drivers/mfd/Makefile
> +++ b/drivers/mfd/Makefile
> @@ -133,6 +133,7 @@ obj-$(CONFIG_AB8500_CORE) += ab8500-core.o ab8500-sysctrl.o
> obj-$(CONFIG_MFD_TIMBERDALE) += timberdale.o
> obj-$(CONFIG_PMIC_ADP5520) += adp5520.o
> obj-$(CONFIG_MFD_KEMPLD) += kempld-core.o
> +obj-$(CONFIG_MFD_INTEL_QUARK_I2C_GPIO) += intel_quark_i2c_gpio.o
> obj-$(CONFIG_LPC_SCH) += lpc_sch.o
> obj-$(CONFIG_LPC_ICH) += lpc_ich.o
> obj-$(CONFIG_MFD_RDC321X) += rdc321x-southbridge.o
> diff --git a/drivers/mfd/intel_quark_i2c_gpio.c b/drivers/mfd/intel_quark_i2c_gpio.c
> new file mode 100644
> index 0000000..a3a7043
> --- /dev/null
> +++ b/drivers/mfd/intel_quark_i2c_gpio.c
> @@ -0,0 +1,279 @@
> +/*
> + * Intel Quark MFD PCI driver for I2C & GPIO
> + *
> + * Copyright(c) 2014 Intel Corporation.
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms and conditions of the GNU General Public License,
> + * version 2, as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope it will be useful, but WITHOUT
> + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
> + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for
> + * more details.
> + *
> + * Intel Quark PCI device for I2C and GPIO controller sharing the same
> + * PCI function. This PCI driver will split the 2 devices into their
> + * respective drivers.
> + */
> +
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/pci.h>
> +#include <linux/mfd/core.h>
> +#include <linux/clkdev.h>
> +#include <linux/clk-provider.h>
> +#include <linux/dmi.h>
> +#include <linux/platform_data/gpio-dwapb.h>
> +#include <linux/platform_data/i2c-designware.h>
> +
> +/* PCI BAR for register base address */
> +#define MFD_I2C_BAR 0
> +#define MFD_GPIO_BAR 1
> +
> +/* The base GPIO number under GPIOLIB framework */
> +#define INTEL_QUARK_MFD_GPIO_BASE 8
> +
> +/* The default number of South-Cluster GPIO on Quark. */
> +#define INTEL_QUARK_MFD_NGPIO 8
> +
> +/* The DesignWare GPIO ports on Quark. */
> +#define INTEL_QUARK_GPIO_NPORTS 1
> +
> +#define INTEL_QUARK_IORES_MEM 0
> +#define INTEL_QUARK_IORES_IRQ 1
> +
> +#define INTEL_QUARK_I2C_CONTROLLER_CLK "i2c_designware.0"
> +
> +/* The Quark I2C controller source clock */
> +#define INTEL_QUARK_I2C_CLK_HZ 33000000
> +
> +#define INTEL_QUARK_I2C_NCLK 1
> +
> +struct intel_quark_mfd {
> + struct pci_dev *pdev;
> + struct clk *i2c_clk;
> + struct clk_lookup *i2c_clk_lookup;
> +};
> +
> +struct i2c_mode_info {
> + const char *name;
> + unsigned int i2c_scl_freq;
> +};
> +
> +static const struct i2c_mode_info platform_i2c_mode_info[] = {
> + {
> + .name = "Galileo",
> + .i2c_scl_freq = 100000,
> + },
> + {
> + .name = "GalileoGen2",
> + .i2c_scl_freq = 400000,
> + },
> +};
We normally do not capitalise device names.
> +static struct resource intel_quark_i2c_res[] = {
> + [INTEL_QUARK_IORES_MEM] = {
> + .flags = IORESOURCE_MEM,
> + },
> + [INTEL_QUARK_IORES_IRQ] = {
> + .flags = IORESOURCE_IRQ,
> + },
> +};
> +
> +static struct resource intel_quark_gpio_res[] = {
> + [INTEL_QUARK_IORES_MEM] = {
> + .flags = IORESOURCE_MEM,
> + },
> +};
> +
> +static struct mfd_cell intel_quark_mfd_cells[] = {
> + {
> + .id = MFD_I2C_BAR,
> + .name = "i2c_designware",
> + .num_resources = ARRAY_SIZE(intel_quark_i2c_res),
> + .resources = intel_quark_i2c_res,
> + .ignore_resource_conflicts = true,
> + },
> + {
> + .id = MFD_GPIO_BAR,
> + .name = "gpio-dwapb",
> + .num_resources = ARRAY_SIZE(intel_quark_gpio_res),
> + .resources = intel_quark_gpio_res,
> + .ignore_resource_conflicts = true,
> + },
> +};
> +
> +static const struct pci_device_id intel_quark_mfd_ids[] = {
> + { PCI_VDEVICE(INTEL, 0x0934), },
> + { 0,}
No need to populate this; just { }, will do fine.
> +};
> +MODULE_DEVICE_TABLE(pci, intel_quark_mfd_ids);
> +
> +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,
Place this line back on the one above and line subsequent lines up
against the '('.
> + sizeof(*i2c_clk_lookup), GFP_KERNEL);
> +
Rid this empty line.
> + 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,
Same here, put this back up on the line above.
> + 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);
> + if (retval)
> + dev_err(&pdev->dev, "Fixed clk register failed: %d\n", retval);
> +
> + return retval;
> +}
I wouldn't mind a clk Ack on this. Did you Cc Mike already?
> +static void intel_quark_unregister_i2c_clk(struct pci_dev *pdev)
> +{
> + struct intel_quark_mfd *quark_mfd = dev_get_drvdata(&pdev->dev);
> +
> + if (!quark_mfd->i2c_clk || !quark_mfd->i2c_clk_lookup)
> + return;
> +
> + clkdev_drop(quark_mfd->i2c_clk_lookup);
> + clk_unregister(quark_mfd->i2c_clk);
> +}
> +
> +static int intel_quark_i2c_setup(struct pci_dev *pdev, struct mfd_cell *cell)
> +{
> + const char *board_name = dmi_get_system_info(DMI_BOARD_NAME);
> + struct dw_i2c_platform_data *pdata;
> + struct resource *res = (struct resource *)cell->resources;
> + struct device *dev = &pdev->dev;
> + unsigned int i;
> +
> + res[INTEL_QUARK_IORES_MEM].start =
> + pci_resource_start(pdev, MFD_I2C_BAR);
> + res[INTEL_QUARK_IORES_MEM].end =
> + pci_resource_end(pdev, MFD_I2C_BAR);
> +
> + res[INTEL_QUARK_IORES_IRQ].start = pdev->irq;
> + res[INTEL_QUARK_IORES_IRQ].end = pdev->irq;
> +
> + pdata = devm_kzalloc(dev, sizeof(*pdata), GFP_KERNEL);
> + if (!pdata)
> + return -ENOMEM;
> +
> + /* Fast mode by default */
> + pdata->i2c_scl_freq = 400000;
> +
> + for (i = 0; i < ARRAY_SIZE(platform_i2c_mode_info); i++)
> + if (!strcmp(board_name, platform_i2c_mode_info[i].name))
> + pdata->i2c_scl_freq
> + = platform_i2c_mode_info[i].i2c_scl_freq;
> +
> + cell->platform_data = pdata;
> + cell->pdata_size = sizeof(*pdata);
> +
> + return 0;
> +}
> +
> +static int intel_quark_gpio_setup(struct pci_dev *pdev, struct mfd_cell *cell)
> +{
> + struct dwapb_platform_data *pdata;
> + struct resource *res = (struct resource *)cell->resources;
> + struct device *dev = &pdev->dev;
> +
> + res[INTEL_QUARK_IORES_MEM].start =
> + pci_resource_start(pdev, MFD_GPIO_BAR);
> + res[INTEL_QUARK_IORES_MEM].end =
> + pci_resource_end(pdev, MFD_GPIO_BAR);
> +
> + pdata = devm_kzalloc(dev, sizeof(*pdata), GFP_KERNEL);
> + if (!pdata)
> + return -ENOMEM;
> +
> + /* For intel quark x1000, it has only one port: portA */
> + pdata->nports = INTEL_QUARK_GPIO_NPORTS;
> + pdata->properties = devm_kcalloc(dev, pdata->nports,
> + sizeof(*pdata->properties),
> + GFP_KERNEL);
> + if (!pdata->properties)
> + return -ENOMEM;
> +
> + /* Set the properties for portA */
> + pdata->properties->node = NULL;
> + pdata->properties->name = "intel-quark-x1000-gpio-portA";
> + pdata->properties->idx = 0;
> + pdata->properties->ngpio = INTEL_QUARK_MFD_NGPIO;
> + pdata->properties->gpio_base = INTEL_QUARK_MFD_GPIO_BASE;
> + pdata->properties->irq = pdev->irq;
> + pdata->properties->irq_shared = true;
Nit: This looks untidy. Either line up all of the '=' or don't do any
aligning at all.
> + cell->platform_data = pdata;
> + cell->pdata_size = sizeof(*pdata);
> +
> + return 0;
> +}
> +
> +static int intel_quark_mfd_probe(struct pci_dev *pdev,
> + const struct pci_device_id *id)
> +{
> + struct intel_quark_mfd *quark_mfd;
> + int retval;
I prefer the use of 'ret' or 'err', as it's more consistent with the
rest of the kernel.
> + retval = pcim_enable_device(pdev);
> + if (retval)
> + return retval;
> +
> + quark_mfd = devm_kzalloc(&pdev->dev, sizeof(*quark_mfd), GFP_KERNEL);
> + if (!quark_mfd)
> + return -ENOMEM;
> + quark_mfd->pdev = pdev;
> +
> + retval = intel_quark_register_i2c_clk(quark_mfd);
> + if (retval)
> + return retval;
> +
> + dev_set_drvdata(&pdev->dev, quark_mfd);
> +
> + retval = intel_quark_i2c_setup(pdev,
> + &intel_quark_mfd_cells[MFD_I2C_BAR]);
> + if (retval)
> + return retval;
> +
> + retval = intel_quark_gpio_setup(pdev,
> + &intel_quark_mfd_cells[MFD_GPIO_BAR]);
> + if (retval)
> + return retval;
> +
> + return mfd_add_devices(&pdev->dev, 0, intel_quark_mfd_cells,
> + ARRAY_SIZE(intel_quark_mfd_cells), NULL, 0, NULL);
Odd alignment. Please use the '('.
> +}
> +
> +static void intel_quark_mfd_remove(struct pci_dev *pdev)
> +{
> + intel_quark_unregister_i2c_clk(pdev);
> + mfd_remove_devices(&pdev->dev);
> +}
> +
> +static struct pci_driver intel_quark_mfd_driver = {
> + .name = "intel_quark_mfd_i2c_gpio",
> + .id_table = intel_quark_mfd_ids,
> + .probe = intel_quark_mfd_probe,
> + .remove = intel_quark_mfd_remove,
> +};
> +
> +module_pci_driver(intel_quark_mfd_driver);
> +
> +MODULE_AUTHOR("Raymond Tan <raymond.tan@intel.com>");
> +MODULE_DESCRIPTION("Intel Quark MFD PCI driver for I2C & GPIO");
> +MODULE_LICENSE("GPL v2");
--
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [PATCH v3 1/1] mfd: intel_quark_i2c_gpio: Add Intel Quark X1000 I2C-GPIO MFD Driver
2015-01-20 12:47 ` Lee Jones
@ 2015-01-20 13:41 ` Shevchenko, Andriy
2015-01-20 15:54 ` Lee Jones
2015-01-20 17:31 ` Mike Turquette
1 sibling, 1 reply; 14+ messages in thread
From: Shevchenko, Andriy @ 2015-01-20 13:41 UTC (permalink / raw)
To: lee.jones@linaro.org
Cc: mturquette@linaro.org, sameo@linux.intel.com,
linux-kernel@vger.kernel.org, Chen, Alvin, Tan, Raymond
[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 1112 bytes --]
On Tue, 2015-01-20 at 12:47 +0000, Lee Jones wrote:
> On Thu, 11 Dec 2014, Raymond Tan wrote:
[]
> > +static const struct i2c_mode_info platform_i2c_mode_info[] = {
> > + {
> > + .name = "Galileo",
> > + .i2c_scl_freq = 100000,
> > + },
> > + {
> > + .name = "GalileoGen2",
> > + .i2c_scl_freq = 400000,
> > + },
> > +};
>
> We normally do not capitalise device names.
This comes from DMI, so, can't be altered I suppose.
--
Andy Shevchenko <andriy.shevchenko@intel.com>
Intel Finland Oy
---------------------------------------------------------------------
Intel Finland Oy
Registered Address: PL 281, 00181 Helsinki
Business Identity Code: 0357606 - 4
Domiciled in Helsinki
This e-mail and any attachments may contain confidential material for
the sole use of the intended recipient(s). Any review or distribution
by others is strictly prohibited. If you are not the intended
recipient, please contact the sender and delete all copies.
ÿôèº{.nÇ+‰·Ÿ®‰†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a¢f£¢·hšïêÿ‘êçz_è®\x03(階ŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨èÚ&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [PATCH v3 1/1] mfd: intel_quark_i2c_gpio: Add Intel Quark X1000 I2C-GPIO MFD Driver
2015-01-20 13:41 ` Shevchenko, Andriy
@ 2015-01-20 15:54 ` Lee Jones
2015-01-20 16:10 ` Shevchenko, Andriy
0 siblings, 1 reply; 14+ messages in thread
From: Lee Jones @ 2015-01-20 15:54 UTC (permalink / raw)
To: Shevchenko, Andriy
Cc: mturquette@linaro.org, sameo@linux.intel.com,
linux-kernel@vger.kernel.org, Chen, Alvin, Tan, Raymond
On Tue, 20 Jan 2015, Shevchenko, Andriy wrote:
> On Tue, 2015-01-20 at 12:47 +0000, Lee Jones wrote:
> > On Thu, 11 Dec 2014, Raymond Tan wrote:
>
> []
>
> > > +static const struct i2c_mode_info platform_i2c_mode_info[] = {
> > > + {
> > > + .name = "Galileo",
> > > + .i2c_scl_freq = 100000,
> > > + },
> > > + {
> > > + .name = "GalileoGen2",
> > > + .i2c_scl_freq = 400000,
> > > + },
> > > +};
> >
> > We normally do not capitalise device names.
>
> This comes from DMI, so, can't be altered I suppose.
I'm not sure what that means, but sounds reasonable.
--
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [PATCH v3 1/1] mfd: intel_quark_i2c_gpio: Add Intel Quark X1000 I2C-GPIO MFD Driver
2015-01-20 15:54 ` Lee Jones
@ 2015-01-20 16:10 ` Shevchenko, Andriy
0 siblings, 0 replies; 14+ messages in thread
From: Shevchenko, Andriy @ 2015-01-20 16:10 UTC (permalink / raw)
To: lee.jones@linaro.org
Cc: mturquette@linaro.org, sameo@linux.intel.com,
linux-kernel@vger.kernel.org, Chen, Alvin, Tan, Raymond
[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 1488 bytes --]
On Tue, 2015-01-20 at 15:54 +0000, Lee Jones wrote:
> On Tue, 20 Jan 2015, Shevchenko, Andriy wrote:
>
> > On Tue, 2015-01-20 at 12:47 +0000, Lee Jones wrote:
> > > On Thu, 11 Dec 2014, Raymond Tan wrote:
> >
> > []
> >
> > > > +static const struct i2c_mode_info platform_i2c_mode_info[] = {
> > > > + {
> > > > + .name = "Galileo",
> > > > + .i2c_scl_freq = 100000,
> > > > + },
> > > > + {
> > > > + .name = "GalileoGen2",
> > > > + .i2c_scl_freq = 400000,
> > > > + },
> > > > +};
> > >
> > > We normally do not capitalise device names.
> >
> > This comes from DMI, so, can't be altered I suppose.
>
> I'm not sure what that means, but sounds reasonable.
http://en.wikipedia.org/wiki/Desktop_Management_Interface
In short, it comes from firmware that's why we have to use exact names
here.
--
Andy Shevchenko <andriy.shevchenko@intel.com>
Intel Finland Oy
---------------------------------------------------------------------
Intel Finland Oy
Registered Address: PL 281, 00181 Helsinki
Business Identity Code: 0357606 - 4
Domiciled in Helsinki
This e-mail and any attachments may contain confidential material for
the sole use of the intended recipient(s). Any review or distribution
by others is strictly prohibited. If you are not the intended
recipient, please contact the sender and delete all copies.
ÿôèº{.nÇ+‰·Ÿ®‰†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a¢f£¢·hšïêÿ‘êçz_è®\x03(階ŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨èÚ&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v3 1/1] mfd: intel_quark_i2c_gpio: Add Intel Quark X1000 I2C-GPIO MFD Driver
2015-01-20 12:47 ` Lee Jones
2015-01-20 13:41 ` Shevchenko, Andriy
@ 2015-01-20 17:31 ` Mike Turquette
1 sibling, 0 replies; 14+ messages in thread
From: Mike Turquette @ 2015-01-20 17:31 UTC (permalink / raw)
To: Lee Jones, Raymond Tan
Cc: Samuel Ortiz, linux-kernel, Alvin Chen, Andriy Shevchenko
Quoting Lee Jones (2015-01-20 04:47:46)
> On Thu, 11 Dec 2014, Raymond Tan wrote:
> > + 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);
> > + if (retval)
> > + dev_err(&pdev->dev, "Fixed clk register failed: %d\n", retval);
> > +
> > + return retval;
> > +}
>
> I wouldn't mind a clk Ack on this. Did you Cc Mike already?
Yes he did and I just supplied it in a parallel thread.
Regards,
Mike
^ permalink raw reply [flat|nested] 14+ messages in thread