Re: [PATCH v3 1/1] mfd: intel_quark_i2c_gpio: Add Intel Quark X1000 I2C-GPIO MFD Driver

From: Lee Jones
Date: Tue Jan 20 2015 - 07:27:14 EST

Next message: Olivier Sobrie: "[PATCH 05/11] hso: fix small indentation error"
Previous message: Tomasz Nowicki: "Re: [Linaro-acpi] [PATCH v7 06/17] ARM64 / ACPI: Make PCI optional for ACPI on ARM64"
Next in thread: Lee Jones: "Re: [PATCH v3 1/1] mfd: intel_quark_i2c_gpio: Add Intel Quark X1000 I2C-GPIO MFD Driver"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

> 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@xxxxxxxxxx; sameo@xxxxxxxxxxxxxxx; linux-
> > kernel@xxxxxxxxxxxxxxx; Chen, Alvin; lee.jones@xxxxxxxxxx
> > 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@xxxxxxxxxxxxxxx>
> > > Signed-off-by: Weike Chen <alvin.chen@xxxxxxxxx>
> > > Signed-off-by: Raymond Tan <raymond.tan@xxxxxxxxx>
> > > ---
> > > 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@xxxxxxxxx>");
> > > +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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Next message: Olivier Sobrie: "[PATCH 05/11] hso: fix small indentation error"
Previous message: Tomasz Nowicki: "Re: [Linaro-acpi] [PATCH v7 06/17] ARM64 / ACPI: Make PCI optional for ACPI on ARM64"
Next in thread: Lee Jones: "Re: [PATCH v3 1/1] mfd: intel_quark_i2c_gpio: Add Intel Quark X1000 I2C-GPIO MFD Driver"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]