[PATCH v1 0/5] mfd: lpc_sch: Intel Quark support

[PATCH v1 0/5] mfd: lpc_sch: Intel Quark support

[Andy Shevchenko]

This brings support of the LPC SCH device found on Intel Quark SoC. Patches
were compile tested, and driver runs on actual hardware.

Please, note that PCI ID will be used in future in the gpio-sch driver.

Andy Shevchenko (4):
  mfd: lpc_sch: reduce duplicate code
  mfd: lpc_sch: better code manageability with chipset info struct
  mfd: lpc_sch: Add support for Intel Quark X1000
  mfd: lpc_sch: remove FSF address

Josef Ahmad (1):
  pci_ids: add support for Intel Quark ILB

 drivers/mfd/lpc_sch.c   | 207 ++++++++++++++++++++++++++++--------------------
 include/linux/pci_ids.h |   1 +
 2 files changed, 122 insertions(+), 86 deletions(-)

-- 
2.1.0

[PATCH v1 1/5] mfd: lpc_sch: reduce duplicate code

[Andy Shevchenko]

This patch refactors the driver to use helper functions instead of
copy'n'pasted pieces of code.

Tested-by: Chang Rebecca Swee Fun <rebecca.swee.fun.chang@intel.com>
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/mfd/lpc_sch.c | 146 ++++++++++++++++++++++----------------------------
 1 file changed, 64 insertions(+), 82 deletions(-)

diff --git a/drivers/mfd/lpc_sch.c b/drivers/mfd/lpc_sch.c
index 4ee7550..0f01ef0 100644
--- a/drivers/mfd/lpc_sch.c
+++ b/drivers/mfd/lpc_sch.c
@@ -40,41 +40,6 @@
 #define WDTBASE		0x84
 #define WDT_IO_SIZE	64
 
-static struct resource smbus_sch_resource = {
-		.flags = IORESOURCE_IO,
-};
-
-static struct resource gpio_sch_resource = {
-		.flags = IORESOURCE_IO,
-};
-
-static struct resource wdt_sch_resource = {
-		.flags = IORESOURCE_IO,
-};
-
-static struct mfd_cell lpc_sch_cells[3];
-
-static struct mfd_cell isch_smbus_cell = {
-	.name = "isch_smbus",
-	.num_resources = 1,
-	.resources = &smbus_sch_resource,
-	.ignore_resource_conflicts = true,
-};
-
-static struct mfd_cell sch_gpio_cell = {
-	.name = "sch_gpio",
-	.num_resources = 1,
-	.resources = &gpio_sch_resource,
-	.ignore_resource_conflicts = true,
-};
-
-static struct mfd_cell wdt_sch_cell = {
-	.name = "ie6xx_wdt",
-	.num_resources = 1,
-	.resources = &wdt_sch_resource,
-	.ignore_resource_conflicts = true,
-};
-
 static const struct pci_device_id lpc_sch_ids[] = {
 	{ PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_SCH_LPC) },
 	{ PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_ITC_LPC) },
@@ -83,67 +48,87 @@ static const struct pci_device_id lpc_sch_ids[] = {
 };
 MODULE_DEVICE_TABLE(pci, lpc_sch_ids);
 
-static int lpc_sch_probe(struct pci_dev *dev,
-				const struct pci_device_id *id)
+static int lpc_sch_get_io(struct pci_dev *pdev, int where, const char *name,
+			  struct resource *res, int size)
 {
 	unsigned int base_addr_cfg;
 	unsigned short base_addr;
-	int i, cells = 0;
-	int ret;
 
-	pci_read_config_dword(dev, SMBASE, &base_addr_cfg);
+	pci_read_config_dword(pdev, where, &base_addr_cfg);
 	base_addr = 0;
 	if (!(base_addr_cfg & (1 << 31)))
-		dev_warn(&dev->dev, "Decode of the SMBus I/O range disabled\n");
+		dev_warn(&pdev->dev, "Decode of the %s I/O range disabled\n",
+			 name);
 	else
 		base_addr = (unsigned short)base_addr_cfg;
 
 	if (base_addr == 0) {
-		dev_warn(&dev->dev, "I/O space for SMBus uninitialized\n");
-	} else {
-		lpc_sch_cells[cells++] = isch_smbus_cell;
-		smbus_sch_resource.start = base_addr;
-		smbus_sch_resource.end = base_addr + SMBUS_IO_SIZE - 1;
+		dev_warn(&pdev->dev, "I/O space for %s uninitialized\n", name);
+		return -ENODEV;
 	}
 
-	pci_read_config_dword(dev, GPIOBASE, &base_addr_cfg);
-	base_addr = 0;
-	if (!(base_addr_cfg & (1 << 31)))
-		dev_warn(&dev->dev, "Decode of the GPIO I/O range disabled\n");
+	res->start = base_addr;
+	res->end = base_addr + size - 1;
+	res->flags = IORESOURCE_IO;
+
+	return 0;
+}
+
+static int lpc_sch_populate_cell(struct pci_dev *pdev, int where,
+				 const char *name, int size, int id,
+				 struct mfd_cell *cell)
+{
+	struct resource *res;
+	int ret;
+
+	res = devm_kzalloc(&pdev->dev, sizeof(*res), GFP_KERNEL);
+	if (!res)
+		return -ENOMEM;
+
+	ret = lpc_sch_get_io(pdev, where, name, res, size);
+	if (ret)
+		return ret;
+
+	memset(cell, 0, sizeof(*cell));
+
+	cell->name = name;
+	cell->resources = res;
+	cell->num_resources = 1;
+	cell->ignore_resource_conflicts = true;
+	cell->id = id;
+
+	return 0;
+}
+
+static int lpc_sch_probe(struct pci_dev *dev,
+			 const struct pci_device_id *id)
+{
+	struct mfd_cell lpc_sch_cells[3];
+	int size, cells = 0;
+	int ret;
+
+	ret = lpc_sch_populate_cell(dev, SMBASE, "isch_smbus", SMBUS_IO_SIZE,
+				    id->device, &lpc_sch_cells[cells]);
+	if (!ret)
+		cells++;
+
+	if (id->device == PCI_DEVICE_ID_INTEL_CENTERTON_ILB)
+		size = GPIO_IO_SIZE_CENTERTON;
 	else
-		base_addr = (unsigned short)base_addr_cfg;
+		size = GPIO_IO_SIZE;
 
-	if (base_addr == 0) {
-		dev_warn(&dev->dev, "I/O space for GPIO uninitialized\n");
-	} else {
-		lpc_sch_cells[cells++] = sch_gpio_cell;
-		gpio_sch_resource.start = base_addr;
-		if (id->device == PCI_DEVICE_ID_INTEL_CENTERTON_ILB)
-			gpio_sch_resource.end = base_addr + GPIO_IO_SIZE_CENTERTON - 1;
-		else
-			gpio_sch_resource.end = base_addr + GPIO_IO_SIZE - 1;
-	}
+	ret = lpc_sch_populate_cell(dev, GPIOBASE, "sch_gpio", size,
+				    id->device, &lpc_sch_cells[cells]);
+	if (!ret)
+		cells++;
 
 	if (id->device == PCI_DEVICE_ID_INTEL_ITC_LPC
 	    || id->device == PCI_DEVICE_ID_INTEL_CENTERTON_ILB) {
-		pci_read_config_dword(dev, WDTBASE, &base_addr_cfg);
-		base_addr = 0;
-		if (!(base_addr_cfg & (1 << 31)))
-			dev_warn(&dev->dev, "Decode of the WDT I/O range disabled\n");
-		else
-			base_addr = (unsigned short)base_addr_cfg;
-		if (base_addr == 0)
-			dev_warn(&dev->dev, "I/O space for WDT uninitialized\n");
-		else {
-			lpc_sch_cells[cells++] = wdt_sch_cell;
-			wdt_sch_resource.start = base_addr;
-			wdt_sch_resource.end = base_addr + WDT_IO_SIZE - 1;
-		}
-	}
-
-	if (WARN_ON(cells > ARRAY_SIZE(lpc_sch_cells))) {
-		dev_err(&dev->dev, "Cell count exceeds array size");
-		return -ENODEV;
+		ret = lpc_sch_populate_cell(dev, WDTBASE, "ie6xx_wdt",
+					    WDT_IO_SIZE,
+					    id->device, &lpc_sch_cells[cells]);
+		if (!ret)
+			cells++;
 	}
 
 	if (cells == 0) {
@@ -151,9 +136,6 @@ static int lpc_sch_probe(struct pci_dev *dev,
 		return -ENODEV;
 	}
 
-	for (i = 0; i < cells; i++)
-		lpc_sch_cells[i].id = id->device;
-
 	ret = mfd_add_devices(&dev->dev, 0, lpc_sch_cells, cells, NULL, 0, NULL);
 	if (ret)
 		mfd_remove_devices(&dev->dev);
-- 
2.1.0

[PATCH v1 2/5] mfd: lpc_sch: better code manageability with chipset info struct

[Andy Shevchenko]

Introduce additional struct to hold chipset info. This chipset
info will be used to store features that are supported by specific
processor or chipset. LPC_SCH supports SMBUS, GPIO and WDT features.
As this code base might expand further to support more processors,
this implementation will help to keep code base clean and manageable.

Signed-off-by: Chang Rebecca Swee Fun <rebecca.swee.fun.chang@intel.com>
Tested-by: Chang Rebecca Swee Fun <rebecca.swee.fun.chang@intel.com>
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/mfd/lpc_sch.c | 65 ++++++++++++++++++++++++++++++++++++---------------
 1 file changed, 46 insertions(+), 19 deletions(-)

diff --git a/drivers/mfd/lpc_sch.c b/drivers/mfd/lpc_sch.c
index 0f01ef0..c4eb359 100644
--- a/drivers/mfd/lpc_sch.c
+++ b/drivers/mfd/lpc_sch.c
@@ -40,10 +40,39 @@
 #define WDTBASE		0x84
 #define WDT_IO_SIZE	64
 
+enum sch_chipsets {
+	LPC_SCH = 0,		/* Intel Poulsbo SCH */
+	LPC_ITC,		/* Intel Tunnel Creek */
+	LPC_CENTERTON,		/* Intel Centerton */
+};
+
+struct lpc_sch_info {
+	unsigned int io_size_smbus;
+	unsigned int io_size_gpio;
+	unsigned int io_size_wdt;
+};
+
+static struct lpc_sch_info sch_chipset_info[] = {
+	[LPC_SCH] = {
+		.io_size_smbus = SMBUS_IO_SIZE,
+		.io_size_gpio = GPIO_IO_SIZE,
+	},
+	[LPC_ITC] = {
+		.io_size_smbus = SMBUS_IO_SIZE,
+		.io_size_gpio = GPIO_IO_SIZE,
+		.io_size_wdt = WDT_IO_SIZE,
+	},
+	[LPC_CENTERTON] = {
+		.io_size_smbus = SMBUS_IO_SIZE,
+		.io_size_gpio = GPIO_IO_SIZE_CENTERTON,
+		.io_size_wdt = WDT_IO_SIZE,
+	},
+};
+
 static const struct pci_device_id lpc_sch_ids[] = {
-	{ PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_SCH_LPC) },
-	{ PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_ITC_LPC) },
-	{ PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_CENTERTON_ILB) },
+	{ PCI_VDEVICE(INTEL, PCI_DEVICE_ID_INTEL_SCH_LPC), LPC_SCH },
+	{ PCI_VDEVICE(INTEL, PCI_DEVICE_ID_INTEL_ITC_LPC), LPC_ITC },
+	{ PCI_VDEVICE(INTEL, PCI_DEVICE_ID_INTEL_CENTERTON_ILB), LPC_CENTERTON },
 	{ 0, }
 };
 MODULE_DEVICE_TABLE(pci, lpc_sch_ids);
@@ -54,6 +83,9 @@ static int lpc_sch_get_io(struct pci_dev *pdev, int where, const char *name,
 	unsigned int base_addr_cfg;
 	unsigned short base_addr;
 
+	if (size == 0)
+		return -EINVAL;
+
 	pci_read_config_dword(pdev, where, &base_addr_cfg);
 	base_addr = 0;
 	if (!(base_addr_cfg & (1 << 31)))
@@ -104,32 +136,27 @@ static int lpc_sch_probe(struct pci_dev *dev,
 			 const struct pci_device_id *id)
 {
 	struct mfd_cell lpc_sch_cells[3];
-	int size, cells = 0;
+	struct lpc_sch_info *info = &sch_chipset_info[id->driver_data];
+	int cells = 0;
 	int ret;
 
-	ret = lpc_sch_populate_cell(dev, SMBASE, "isch_smbus", SMBUS_IO_SIZE,
+	ret = lpc_sch_populate_cell(dev, SMBASE, "isch_smbus",
+				    info->io_size_smbus,
 				    id->device, &lpc_sch_cells[cells]);
 	if (!ret)
 		cells++;
 
-	if (id->device == PCI_DEVICE_ID_INTEL_CENTERTON_ILB)
-		size = GPIO_IO_SIZE_CENTERTON;
-	else
-		size = GPIO_IO_SIZE;
-
-	ret = lpc_sch_populate_cell(dev, GPIOBASE, "sch_gpio", size,
+	ret = lpc_sch_populate_cell(dev, GPIOBASE, "sch_gpio",
+				    info->io_size_gpio,
 				    id->device, &lpc_sch_cells[cells]);
 	if (!ret)
 		cells++;
 
-	if (id->device == PCI_DEVICE_ID_INTEL_ITC_LPC
-	    || id->device == PCI_DEVICE_ID_INTEL_CENTERTON_ILB) {
-		ret = lpc_sch_populate_cell(dev, WDTBASE, "ie6xx_wdt",
-					    WDT_IO_SIZE,
-					    id->device, &lpc_sch_cells[cells]);
-		if (!ret)
-			cells++;
-	}
+	ret = lpc_sch_populate_cell(dev, WDTBASE, "ie6xx_wdt",
+				    info->io_size_wdt,
+				    id->device, &lpc_sch_cells[cells]);
+	if (!ret)
+		cells++;
 
 	if (cells == 0) {
 		dev_err(&dev->dev, "All decode registers disabled.\n");
-- 
2.1.0

[PATCH v1 3/5] pci_ids: add support for Intel Quark ILB

[Andy Shevchenko]

From: Josef Ahmad <josef.ahmad@intel.com>

This patch adds the PCI id for Intel Quark ILB.
It will be used for GPIO and Multifunction device driver.

Signed-off-by: Josef Ahmad <josef.ahmad@intel.com>
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 include/linux/pci_ids.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/include/linux/pci_ids.h b/include/linux/pci_ids.h
index 6ed0bb7..4e82195 100644
--- a/include/linux/pci_ids.h
+++ b/include/linux/pci_ids.h
@@ -2557,6 +2557,7 @@
 #define PCI_DEVICE_ID_INTEL_MFD_EMMC0	0x0823
 #define PCI_DEVICE_ID_INTEL_MFD_EMMC1	0x0824
 #define PCI_DEVICE_ID_INTEL_MRST_SD2	0x084F
+#define PCI_DEVICE_ID_INTEL_QUARK_X1000_ILB	0x095E
 #define PCI_DEVICE_ID_INTEL_I960	0x0960
 #define PCI_DEVICE_ID_INTEL_I960RM	0x0962
 #define PCI_DEVICE_ID_INTEL_CENTERTON_ILB	0x0c60
-- 
2.1.0

[PATCH v1 4/5] mfd: lpc_sch: Add support for Intel Quark X1000

[Andy Shevchenko]

Intel Quark X1000 SoC supports IRQ based GPIO. This patch will
enable MFD support for Quark X1000 and provide IRQ resources
to Quark X1000 GPIO device driver.

Signed-off-by: Chang Rebecca Swee Fun <rebecca.swee.fun.chang@intel.com>
Tested-by: Chang Rebecca Swee Fun <rebecca.swee.fun.chang@intel.com>
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/mfd/lpc_sch.c | 41 +++++++++++++++++++++++++++++++++++------
 1 file changed, 35 insertions(+), 6 deletions(-)

diff --git a/drivers/mfd/lpc_sch.c b/drivers/mfd/lpc_sch.c
index c4eb359..6145a4c 100644
--- a/drivers/mfd/lpc_sch.c
+++ b/drivers/mfd/lpc_sch.c
@@ -37,6 +37,9 @@
 #define GPIO_IO_SIZE	64
 #define GPIO_IO_SIZE_CENTERTON	128
 
+/* Intel Quark X1000 GPIO IRQ Number */
+#define GPIO_IRQ_QUARK_X1000	9
+
 #define WDTBASE		0x84
 #define WDT_IO_SIZE	64
 
@@ -44,28 +47,37 @@ enum sch_chipsets {
 	LPC_SCH = 0,		/* Intel Poulsbo SCH */
 	LPC_ITC,		/* Intel Tunnel Creek */
 	LPC_CENTERTON,		/* Intel Centerton */
+	LPC_QUARK_X1000,	/* Intel Quark X1000 */
 };
 
 struct lpc_sch_info {
 	unsigned int io_size_smbus;
 	unsigned int io_size_gpio;
 	unsigned int io_size_wdt;
+	int irq_gpio;
 };
 
 static struct lpc_sch_info sch_chipset_info[] = {
 	[LPC_SCH] = {
 		.io_size_smbus = SMBUS_IO_SIZE,
 		.io_size_gpio = GPIO_IO_SIZE,
+		.irq_gpio = -1,
 	},
 	[LPC_ITC] = {
 		.io_size_smbus = SMBUS_IO_SIZE,
 		.io_size_gpio = GPIO_IO_SIZE,
 		.io_size_wdt = WDT_IO_SIZE,
+		.irq_gpio = -1,
 	},
 	[LPC_CENTERTON] = {
 		.io_size_smbus = SMBUS_IO_SIZE,
 		.io_size_gpio = GPIO_IO_SIZE_CENTERTON,
 		.io_size_wdt = WDT_IO_SIZE,
+		.irq_gpio = -1,
+	},
+	[LPC_QUARK_X1000] = {
+		.io_size_gpio = GPIO_IO_SIZE,
+		.irq_gpio = GPIO_IRQ_QUARK_X1000,
 	},
 };
 
@@ -73,6 +85,7 @@ static const struct pci_device_id lpc_sch_ids[] = {
 	{ PCI_VDEVICE(INTEL, PCI_DEVICE_ID_INTEL_SCH_LPC), LPC_SCH },
 	{ PCI_VDEVICE(INTEL, PCI_DEVICE_ID_INTEL_ITC_LPC), LPC_ITC },
 	{ PCI_VDEVICE(INTEL, PCI_DEVICE_ID_INTEL_CENTERTON_ILB), LPC_CENTERTON },
+	{ PCI_VDEVICE(INTEL, PCI_DEVICE_ID_INTEL_QUARK_X1000_ILB), LPC_QUARK_X1000 },
 	{ 0, }
 };
 MODULE_DEVICE_TABLE(pci, lpc_sch_ids);
@@ -106,14 +119,26 @@ static int lpc_sch_get_io(struct pci_dev *pdev, int where, const char *name,
 	return 0;
 }
 
+static int lpc_sch_get_irq(struct resource *res, int irq)
+{
+	if (irq < 0)
+		return -EINVAL;
+
+	res->start = irq;
+	res->end = irq;
+	res->flags = IORESOURCE_IRQ;
+
+	return 0;
+}
+
 static int lpc_sch_populate_cell(struct pci_dev *pdev, int where,
-				 const char *name, int size, int id,
-				 struct mfd_cell *cell)
+				 const char *name, int size, int irq,
+				 int id, struct mfd_cell *cell)
 {
 	struct resource *res;
 	int ret;
 
-	res = devm_kzalloc(&pdev->dev, sizeof(*res), GFP_KERNEL);
+	res = devm_kcalloc(&pdev->dev, 2, sizeof(*res), GFP_KERNEL);
 	if (!res)
 		return -ENOMEM;
 
@@ -129,6 +154,10 @@ static int lpc_sch_populate_cell(struct pci_dev *pdev, int where,
 	cell->ignore_resource_conflicts = true;
 	cell->id = id;
 
+	ret = lpc_sch_get_irq(++res, irq);
+	if (!ret)
+		cell->num_resources++;
+
 	return 0;
 }
 
@@ -141,19 +170,19 @@ static int lpc_sch_probe(struct pci_dev *dev,
 	int ret;
 
 	ret = lpc_sch_populate_cell(dev, SMBASE, "isch_smbus",
-				    info->io_size_smbus,
+				    info->io_size_smbus, -1,
 				    id->device, &lpc_sch_cells[cells]);
 	if (!ret)
 		cells++;
 
 	ret = lpc_sch_populate_cell(dev, GPIOBASE, "sch_gpio",
-				    info->io_size_gpio,
+				    info->io_size_gpio, info->irq_gpio,
 				    id->device, &lpc_sch_cells[cells]);
 	if (!ret)
 		cells++;
 
 	ret = lpc_sch_populate_cell(dev, WDTBASE, "ie6xx_wdt",
-				    info->io_size_wdt,
+				    info->io_size_wdt, -1,
 				    id->device, &lpc_sch_cells[cells]);
 	if (!ret)
 		cells++;
-- 
2.1.0

[PATCH v1 5/5] mfd: lpc_sch: remove FSF address

[Andy Shevchenko]

This patch removes FSF address becase it can be changed. While here, update the
copyright lines by adding Intel Corp. to them.

There is no functional change.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/mfd/lpc_sch.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/drivers/mfd/lpc_sch.c b/drivers/mfd/lpc_sch.c
index 6145a4c..b14ca7c 100644
--- a/drivers/mfd/lpc_sch.c
+++ b/drivers/mfd/lpc_sch.c
@@ -7,6 +7,7 @@
  *  Configuration Registers.
  *
  *  Copyright (c) 2010 CompuLab Ltd
+ *  Copyright (c) 2014 Intel Corp.
  *  Author: Denis Turischev <denis@compulab.co.il>
  *
  *  This program is free software; you can redistribute it and/or modify
@@ -17,10 +18,6 @@
  *  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.
- *
- *  You should have received a copy of the GNU General Public License
- *  along with this program; see the file COPYING.  If not, write to
- *  the Free Software Foundation, 675 Mass Ave, Cambridge, MA 02139, USA.
  */
 
 #include <linux/kernel.h>
-- 
2.1.0

Re: [PATCH v1 0/5] mfd: lpc_sch: Intel Quark support

[Andy Shevchenko]

On Fri, 2014-08-22 at 13:58 +0300, Andy Shevchenko wrote:
> This brings support of the LPC SCH device found on Intel Quark SoC. Patches
> were compile tested, and driver runs on actual hardware.
> 
> Please, note that PCI ID will be used in future in the gpio-sch driver.

Bjorn, could you Ack or comment patch 3/5?

Lee, do you have any comments on the series?

> 
> Andy Shevchenko (4):
>   mfd: lpc_sch: reduce duplicate code
>   mfd: lpc_sch: better code manageability with chipset info struct
>   mfd: lpc_sch: Add support for Intel Quark X1000
>   mfd: lpc_sch: remove FSF address
> 
> Josef Ahmad (1):
>   pci_ids: add support for Intel Quark ILB
> 
>  drivers/mfd/lpc_sch.c   | 207 ++++++++++++++++++++++++++++--------------------
>  include/linux/pci_ids.h |   1 +
>  2 files changed, 122 insertions(+), 86 deletions(-)
> 


-- 
Andy Shevchenko <andriy.shevchenko@intel.com>
Intel Finland Oy

Re: [PATCH v1 0/5] mfd: lpc_sch: Intel Quark support

[Lee Jones]

On Fri, 29 Aug 2014, Andy Shevchenko wrote:

> On Fri, 2014-08-22 at 13:58 +0300, Andy Shevchenko wrote:
> > This brings support of the LPC SCH device found on Intel Quark SoC. Patches
> > were compile tested, and driver runs on actual hardware.
> > 
> > Please, note that PCI ID will be used in future in the gpio-sch driver.
> 
> Bjorn, could you Ack or comment patch 3/5?
> 
> Lee, do you have any comments on the series?

I have marked them as 'important' and will get round to them when I
can.  This week has been hectic.  I will get round to it next week.

> > Andy Shevchenko (4):
> >   mfd: lpc_sch: reduce duplicate code
> >   mfd: lpc_sch: better code manageability with chipset info struct
> >   mfd: lpc_sch: Add support for Intel Quark X1000
> >   mfd: lpc_sch: remove FSF address
> > 
> > Josef Ahmad (1):
> >   pci_ids: add support for Intel Quark ILB
> > 
> >  drivers/mfd/lpc_sch.c   | 207 ++++++++++++++++++++++++++++--------------------
> >  include/linux/pci_ids.h |   1 +
> >  2 files changed, 122 insertions(+), 86 deletions(-)
> > 
> 
> 

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH v1 5/5] mfd: lpc_sch: remove FSF address

[Bjorn Helgaas]

On Fri, Aug 22, 2014 at 4:58 AM, Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
> This patch removes FSF address becase it can be changed. While here, update the

because

> copyright lines by adding Intel Corp. to them.
>
> There is no functional change.
>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>  drivers/mfd/lpc_sch.c | 5 +----
>  1 file changed, 1 insertion(+), 4 deletions(-)
>
> diff --git a/drivers/mfd/lpc_sch.c b/drivers/mfd/lpc_sch.c
> index 6145a4c..b14ca7c 100644
> --- a/drivers/mfd/lpc_sch.c
> +++ b/drivers/mfd/lpc_sch.c
> @@ -7,6 +7,7 @@
>   *  Configuration Registers.
>   *
>   *  Copyright (c) 2010 CompuLab Ltd
> + *  Copyright (c) 2014 Intel Corp.
>   *  Author: Denis Turischev <denis@compulab.co.il>
>   *
>   *  This program is free software; you can redistribute it and/or modify
> @@ -17,10 +18,6 @@
>   *  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.
> - *
> - *  You should have received a copy of the GNU General Public License
> - *  along with this program; see the file COPYING.  If not, write to
> - *  the Free Software Foundation, 675 Mass Ave, Cambridge, MA 02139, USA.
>   */
>
>  #include <linux/kernel.h>
> --
> 2.1.0
>

Re: [PATCH v1 3/5] pci_ids: add support for Intel Quark ILB

[Bjorn Helgaas]

On Fri, Aug 22, 2014 at 4:58 AM, Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
> From: Josef Ahmad <josef.ahmad@intel.com>
>
> This patch adds the PCI id for Intel Quark ILB.
> It will be used for GPIO and Multifunction device driver.
>
> Signed-off-by: Josef Ahmad <josef.ahmad@intel.com>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

Assuming that this will actually be used in more than one place:

Acked-by: Bjorn Helgaas <bhelgaas@google.com>

> ---
>  include/linux/pci_ids.h | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/include/linux/pci_ids.h b/include/linux/pci_ids.h
> index 6ed0bb7..4e82195 100644
> --- a/include/linux/pci_ids.h
> +++ b/include/linux/pci_ids.h
> @@ -2557,6 +2557,7 @@
>  #define PCI_DEVICE_ID_INTEL_MFD_EMMC0  0x0823
>  #define PCI_DEVICE_ID_INTEL_MFD_EMMC1  0x0824
>  #define PCI_DEVICE_ID_INTEL_MRST_SD2   0x084F
> +#define PCI_DEVICE_ID_INTEL_QUARK_X1000_ILB    0x095E
>  #define PCI_DEVICE_ID_INTEL_I960       0x0960
>  #define PCI_DEVICE_ID_INTEL_I960RM     0x0962
>  #define PCI_DEVICE_ID_INTEL_CENTERTON_ILB      0x0c60
> --
> 2.1.0
>

Re: [PATCH v1 1/5] mfd: lpc_sch: reduce duplicate code

[Lee Jones]

On Fri, 22 Aug 2014, Andy Shevchenko wrote:

> This patch refactors the driver to use helper functions instead of
> copy'n'pasted pieces of code.
> 
> Tested-by: Chang Rebecca Swee Fun <rebecca.swee.fun.chang@intel.com>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>  drivers/mfd/lpc_sch.c | 146 ++++++++++++++++++++++----------------------------
>  1 file changed, 64 insertions(+), 82 deletions(-)
> 
> diff --git a/drivers/mfd/lpc_sch.c b/drivers/mfd/lpc_sch.c
> index 4ee7550..0f01ef0 100644
> --- a/drivers/mfd/lpc_sch.c
> +++ b/drivers/mfd/lpc_sch.c
> @@ -40,41 +40,6 @@

[...]

> -static int lpc_sch_probe(struct pci_dev *dev,
> -				const struct pci_device_id *id)
> +static int lpc_sch_get_io(struct pci_dev *pdev, int where, const char *name,
> +			  struct resource *res, int size)
>  {
>  	unsigned int base_addr_cfg;
>  	unsigned short base_addr;
> -	int i, cells = 0;
> -	int ret;
>  
> -	pci_read_config_dword(dev, SMBASE, &base_addr_cfg);
> +	pci_read_config_dword(pdev, where, &base_addr_cfg);
>  	base_addr = 0;
>  	if (!(base_addr_cfg & (1 << 31)))
> -		dev_warn(&dev->dev, "Decode of the SMBus I/O range disabled\n");
> +		dev_warn(&pdev->dev, "Decode of the %s I/O range disabled\n",
> +			 name);
>  	else
>  		base_addr = (unsigned short)base_addr_cfg;
>  
>  	if (base_addr == 0) {
> -		dev_warn(&dev->dev, "I/O space for SMBus uninitialized\n");
> -	} else {
> -		lpc_sch_cells[cells++] = isch_smbus_cell;
> -		smbus_sch_resource.start = base_addr;
> -		smbus_sch_resource.end = base_addr + SMBUS_IO_SIZE - 1;
> +		dev_warn(&pdev->dev, "I/O space for %s uninitialized\n", name);
> +		return -ENODEV;

If you're going to return an error, you need to use dev_err() above.
>  	}
>  
> -	pci_read_config_dword(dev, GPIOBASE, &base_addr_cfg);
> -	base_addr = 0;
> -	if (!(base_addr_cfg & (1 << 31)))
> -		dev_warn(&dev->dev, "Decode of the GPIO I/O range disabled\n");
> +	res->start = base_addr;
> +	res->end = base_addr + size - 1;
> +	res->flags = IORESOURCE_IO;
> +
> +	return 0;
> +}
> +
> +static int lpc_sch_populate_cell(struct pci_dev *pdev, int where,
> +				 const char *name, int size, int id,
> +				 struct mfd_cell *cell)
> +{
> +	struct resource *res;
> +	int ret;
> +
> +	res = devm_kzalloc(&pdev->dev, sizeof(*res), GFP_KERNEL);
> +	if (!res)
> +		return -ENOMEM;
> +
> +	ret = lpc_sch_get_io(pdev, where, name, res, size);
> +	if (ret)
> +		return ret;
> +
> +	memset(cell, 0, sizeof(*cell));
> +
> +	cell->name = name;
> +	cell->resources = res;
> +	cell->num_resources = 1;
> +	cell->ignore_resource_conflicts = true;
> +	cell->id = id;
> +
> +	return 0;
> +}
> +
> +static int lpc_sch_probe(struct pci_dev *dev,
> +			 const struct pci_device_id *id)
> +{
> +	struct mfd_cell lpc_sch_cells[3];
> +	int size, cells = 0;
> +	int ret;
> +
> +	ret = lpc_sch_populate_cell(dev, SMBASE, "isch_smbus", SMBUS_IO_SIZE,
> +				    id->device, &lpc_sch_cells[cells]);
> +	if (!ret)
> +		cells++;

You're masking errors here.  You need to return on error.

> +	if (id->device == PCI_DEVICE_ID_INTEL_CENTERTON_ILB)
> +		size = GPIO_IO_SIZE_CENTERTON;
>  	else
> -		base_addr = (unsigned short)base_addr_cfg;
> +		size = GPIO_IO_SIZE;
>  
> -	if (base_addr == 0) {
> -		dev_warn(&dev->dev, "I/O space for GPIO uninitialized\n");
> -	} else {
> -		lpc_sch_cells[cells++] = sch_gpio_cell;
> -		gpio_sch_resource.start = base_addr;
> -		if (id->device == PCI_DEVICE_ID_INTEL_CENTERTON_ILB)
> -			gpio_sch_resource.end = base_addr + GPIO_IO_SIZE_CENTERTON - 1;
> -		else
> -			gpio_sch_resource.end = base_addr + GPIO_IO_SIZE - 1;
> -	}
> +	ret = lpc_sch_populate_cell(dev, GPIOBASE, "sch_gpio", size,
> +				    id->device, &lpc_sch_cells[cells]);
> +	if (!ret)
> +		cells++;
>  
>  	if (id->device == PCI_DEVICE_ID_INTEL_ITC_LPC
>  	    || id->device == PCI_DEVICE_ID_INTEL_CENTERTON_ILB) {
> -		pci_read_config_dword(dev, WDTBASE, &base_addr_cfg);
> -		base_addr = 0;
> -		if (!(base_addr_cfg & (1 << 31)))
> -			dev_warn(&dev->dev, "Decode of the WDT I/O range disabled\n");
> -		else
> -			base_addr = (unsigned short)base_addr_cfg;
> -		if (base_addr == 0)
> -			dev_warn(&dev->dev, "I/O space for WDT uninitialized\n");
> -		else {
> -			lpc_sch_cells[cells++] = wdt_sch_cell;
> -			wdt_sch_resource.start = base_addr;
> -			wdt_sch_resource.end = base_addr + WDT_IO_SIZE - 1;
> -		}
> -	}
> -
> -	if (WARN_ON(cells > ARRAY_SIZE(lpc_sch_cells))) {
> -		dev_err(&dev->dev, "Cell count exceeds array size");
> -		return -ENODEV;
> +		ret = lpc_sch_populate_cell(dev, WDTBASE, "ie6xx_wdt",
> +					    WDT_IO_SIZE,
> +					    id->device, &lpc_sch_cells[cells]);
> +		if (!ret)
> +			cells++;
>  	}
>  
>  	if (cells == 0) {
> @@ -151,9 +136,6 @@ static int lpc_sch_probe(struct pci_dev *dev,
>  		return -ENODEV;
>  	}
>  
> -	for (i = 0; i < cells; i++)
> -		lpc_sch_cells[i].id = id->device;
> -
>  	ret = mfd_add_devices(&dev->dev, 0, lpc_sch_cells, cells, NULL, 0, NULL);
>  	if (ret)
>  		mfd_remove_devices(&dev->dev);

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH v1 3/5] pci_ids: add support for Intel Quark ILB

[Andy Shevchenko]

On Fri, 2014-08-29 at 08:27 -0600, Bjorn Helgaas wrote:
> On Fri, Aug 22, 2014 at 4:58 AM, Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
> > From: Josef Ahmad <josef.ahmad@intel.com>
> >
> > This patch adds the PCI id for Intel Quark ILB.
> > It will be used for GPIO and Multifunction device driver.
> >
> > Signed-off-by: Josef Ahmad <josef.ahmad@intel.com>
> > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> 
> Assuming that this will actually be used in more than one place:

Yes, look for the other IDs in the same driver (lpc_sch).

The question is will we have the update for sch_gpio in time to be
included to v3.18? I think Rebecca can answer to this one.

> 
> Acked-by: Bjorn Helgaas <bhelgaas@google.com>
> 
> > ---
> >  include/linux/pci_ids.h | 1 +
> >  1 file changed, 1 insertion(+)
> >
> > diff --git a/include/linux/pci_ids.h b/include/linux/pci_ids.h
> > index 6ed0bb7..4e82195 100644
> > --- a/include/linux/pci_ids.h
> > +++ b/include/linux/pci_ids.h
> > @@ -2557,6 +2557,7 @@
> >  #define PCI_DEVICE_ID_INTEL_MFD_EMMC0  0x0823
> >  #define PCI_DEVICE_ID_INTEL_MFD_EMMC1  0x0824
> >  #define PCI_DEVICE_ID_INTEL_MRST_SD2   0x084F
> > +#define PCI_DEVICE_ID_INTEL_QUARK_X1000_ILB    0x095E
> >  #define PCI_DEVICE_ID_INTEL_I960       0x0960
> >  #define PCI_DEVICE_ID_INTEL_I960RM     0x0962
> >  #define PCI_DEVICE_ID_INTEL_CENTERTON_ILB      0x0c60
> > --
> > 2.1.0
> >


-- 
Andy Shevchenko <andriy.shevchenko@intel.com>
Intel Finland Oy

Re: [PATCH v1 2/5] mfd: lpc_sch: better code manageability with chipset info struct

[Lee Jones]

On Fri, 22 Aug 2014, Andy Shevchenko wrote:

> Introduce additional struct to hold chipset info. This chipset
> info will be used to store features that are supported by specific
> processor or chipset. LPC_SCH supports SMBUS, GPIO and WDT features.
> As this code base might expand further to support more processors,
> this implementation will help to keep code base clean and manageable.
> 
> Signed-off-by: Chang Rebecca Swee Fun <rebecca.swee.fun.chang@intel.com>
> Tested-by: Chang Rebecca Swee Fun <rebecca.swee.fun.chang@intel.com>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>  drivers/mfd/lpc_sch.c | 65 ++++++++++++++++++++++++++++++++++++---------------
>  1 file changed, 46 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/mfd/lpc_sch.c b/drivers/mfd/lpc_sch.c
> index 0f01ef0..c4eb359 100644
> --- a/drivers/mfd/lpc_sch.c
> +++ b/drivers/mfd/lpc_sch.c
> @@ -40,10 +40,39 @@
>  #define WDTBASE		0x84
>  #define WDT_IO_SIZE	64
>  
> +enum sch_chipsets {
> +	LPC_SCH = 0,		/* Intel Poulsbo SCH */
> +	LPC_ITC,		/* Intel Tunnel Creek */
> +	LPC_CENTERTON,		/* Intel Centerton */
> +};
> +
> +struct lpc_sch_info {
> +	unsigned int io_size_smbus;
> +	unsigned int io_size_gpio;
> +	unsigned int io_size_wdt;
> +};
> +
> +static struct lpc_sch_info sch_chipset_info[] = {
> +	[LPC_SCH] = {
> +		.io_size_smbus = SMBUS_IO_SIZE,
> +		.io_size_gpio = GPIO_IO_SIZE,
> +	},
> +	[LPC_ITC] = {
> +		.io_size_smbus = SMBUS_IO_SIZE,
> +		.io_size_gpio = GPIO_IO_SIZE,
> +		.io_size_wdt = WDT_IO_SIZE,
> +	},
> +	[LPC_CENTERTON] = {
> +		.io_size_smbus = SMBUS_IO_SIZE,
> +		.io_size_gpio = GPIO_IO_SIZE_CENTERTON,
> +		.io_size_wdt = WDT_IO_SIZE,
> +	},
> +};
> +
>  static const struct pci_device_id lpc_sch_ids[] = {
> -	{ PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_SCH_LPC) },
> -	{ PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_ITC_LPC) },
> -	{ PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_CENTERTON_ILB) },
> +	{ PCI_VDEVICE(INTEL, PCI_DEVICE_ID_INTEL_SCH_LPC), LPC_SCH },
> +	{ PCI_VDEVICE(INTEL, PCI_DEVICE_ID_INTEL_ITC_LPC), LPC_ITC },
> +	{ PCI_VDEVICE(INTEL, PCI_DEVICE_ID_INTEL_CENTERTON_ILB), LPC_CENTERTON },
>  	{ 0, }
>  };
>  MODULE_DEVICE_TABLE(pci, lpc_sch_ids);
> @@ -54,6 +83,9 @@ static int lpc_sch_get_io(struct pci_dev *pdev, int where, const char *name,
>  	unsigned int base_addr_cfg;
>  	unsigned short base_addr;
>  
> +	if (size == 0)
> +		return -EINVAL;
> +
>  	pci_read_config_dword(pdev, where, &base_addr_cfg);
>  	base_addr = 0;
>  	if (!(base_addr_cfg & (1 << 31)))
> @@ -104,32 +136,27 @@ static int lpc_sch_probe(struct pci_dev *dev,
>  			 const struct pci_device_id *id)
>  {
>  	struct mfd_cell lpc_sch_cells[3];
> -	int size, cells = 0;
> +	struct lpc_sch_info *info = &sch_chipset_info[id->driver_data];
> +	int cells = 0;
>  	int ret;
>  
> -	ret = lpc_sch_populate_cell(dev, SMBASE, "isch_smbus", SMBUS_IO_SIZE,
> +	ret = lpc_sch_populate_cell(dev, SMBASE, "isch_smbus",
> +				    info->io_size_smbus,
>  				    id->device, &lpc_sch_cells[cells]);
>  	if (!ret)
>  		cells++;
>  
> -	if (id->device == PCI_DEVICE_ID_INTEL_CENTERTON_ILB)
> -		size = GPIO_IO_SIZE_CENTERTON;
> -	else
> -		size = GPIO_IO_SIZE;
> -
> -	ret = lpc_sch_populate_cell(dev, GPIOBASE, "sch_gpio", size,
> +	ret = lpc_sch_populate_cell(dev, GPIOBASE, "sch_gpio",
> +				    info->io_size_gpio,
>  				    id->device, &lpc_sch_cells[cells]);
>  	if (!ret)
>  		cells++;
>  
> -	if (id->device == PCI_DEVICE_ID_INTEL_ITC_LPC
> -	    || id->device == PCI_DEVICE_ID_INTEL_CENTERTON_ILB) {
> -		ret = lpc_sch_populate_cell(dev, WDTBASE, "ie6xx_wdt",
> -					    WDT_IO_SIZE,
> -					    id->device, &lpc_sch_cells[cells]);
> -		if (!ret)
> -			cells++;
> -	}
> +	ret = lpc_sch_populate_cell(dev, WDTBASE, "ie6xx_wdt",
> +				    info->io_size_wdt,
> +				    id->device, &lpc_sch_cells[cells]);
> +	if (!ret)
> +		cells++;

The first patch would look a great deal cleaner if it had these
changes in too.  Unless you have a really good reason not to, please
consider squashing them.

>  	if (cells == 0) {
>  		dev_err(&dev->dev, "All decode registers disabled.\n");

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH v1 4/5] mfd: lpc_sch: Add support for Intel Quark X1000

[Lee Jones]

On Fri, 22 Aug 2014, Andy Shevchenko wrote:

> Intel Quark X1000 SoC supports IRQ based GPIO. This patch will
> enable MFD support for Quark X1000 and provide IRQ resources
> to Quark X1000 GPIO device driver.
> 
> Signed-off-by: Chang Rebecca Swee Fun <rebecca.swee.fun.chang@intel.com>
> Tested-by: Chang Rebecca Swee Fun <rebecca.swee.fun.chang@intel.com>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>  drivers/mfd/lpc_sch.c | 41 +++++++++++++++++++++++++++++++++++------
>  1 file changed, 35 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/mfd/lpc_sch.c b/drivers/mfd/lpc_sch.c
> index c4eb359..6145a4c 100644
> --- a/drivers/mfd/lpc_sch.c
> +++ b/drivers/mfd/lpc_sch.c
> @@ -37,6 +37,9 @@
>  #define GPIO_IO_SIZE	64
>  #define GPIO_IO_SIZE_CENTERTON	128
>  
> +/* Intel Quark X1000 GPIO IRQ Number */
> +#define GPIO_IRQ_QUARK_X1000	9
> +
>  #define WDTBASE		0x84
>  #define WDT_IO_SIZE	64
>  
> @@ -44,28 +47,37 @@ enum sch_chipsets {
>  	LPC_SCH = 0,		/* Intel Poulsbo SCH */
>  	LPC_ITC,		/* Intel Tunnel Creek */
>  	LPC_CENTERTON,		/* Intel Centerton */
> +	LPC_QUARK_X1000,	/* Intel Quark X1000 */
>  };
>  
>  struct lpc_sch_info {
>  	unsigned int io_size_smbus;
>  	unsigned int io_size_gpio;
>  	unsigned int io_size_wdt;
> +	int irq_gpio;
>  };
>  
>  static struct lpc_sch_info sch_chipset_info[] = {
>  	[LPC_SCH] = {
>  		.io_size_smbus = SMBUS_IO_SIZE,
>  		.io_size_gpio = GPIO_IO_SIZE,
> +		.irq_gpio = -1,
>  	},
>  	[LPC_ITC] = {
>  		.io_size_smbus = SMBUS_IO_SIZE,
>  		.io_size_gpio = GPIO_IO_SIZE,
>  		.io_size_wdt = WDT_IO_SIZE,
> +		.irq_gpio = -1,
>  	},
>  	[LPC_CENTERTON] = {
>  		.io_size_smbus = SMBUS_IO_SIZE,
>  		.io_size_gpio = GPIO_IO_SIZE_CENTERTON,
>  		.io_size_wdt = WDT_IO_SIZE,
> +		.irq_gpio = -1,
> +	},
> +	[LPC_QUARK_X1000] = {
> +		.io_size_gpio = GPIO_IO_SIZE,
> +		.irq_gpio = GPIO_IRQ_QUARK_X1000,
>  	},
>  };
>  
> @@ -73,6 +85,7 @@ static const struct pci_device_id lpc_sch_ids[] = {
>  	{ PCI_VDEVICE(INTEL, PCI_DEVICE_ID_INTEL_SCH_LPC), LPC_SCH },
>  	{ PCI_VDEVICE(INTEL, PCI_DEVICE_ID_INTEL_ITC_LPC), LPC_ITC },
>  	{ PCI_VDEVICE(INTEL, PCI_DEVICE_ID_INTEL_CENTERTON_ILB), LPC_CENTERTON },
> +	{ PCI_VDEVICE(INTEL, PCI_DEVICE_ID_INTEL_QUARK_X1000_ILB), LPC_QUARK_X1000 },
>  	{ 0, }
>  };
>  MODULE_DEVICE_TABLE(pci, lpc_sch_ids);
> @@ -106,14 +119,26 @@ static int lpc_sch_get_io(struct pci_dev *pdev, int where, const char *name,
>  	return 0;
>  }
>  
> +static int lpc_sch_get_irq(struct resource *res, int irq)
> +{
> +	if (irq < 0)
> +		return -EINVAL;
> +
> +	res->start = irq;
> +	res->end = irq;
> +	res->flags = IORESOURCE_IRQ;
> +
> +	return 0;
> +}

Why does this need to be a separate function?

I fear that the code will become unnecessarily fragmented, just for the
sake of it.

>  static int lpc_sch_populate_cell(struct pci_dev *pdev, int where,
> -				 const char *name, int size, int id,
> -				 struct mfd_cell *cell)
> +				 const char *name, int size, int irq,
> +				 int id, struct mfd_cell *cell)
>  {
>  	struct resource *res;
>  	int ret;
>  
> -	res = devm_kzalloc(&pdev->dev, sizeof(*res), GFP_KERNEL);
> +	res = devm_kcalloc(&pdev->dev, 2, sizeof(*res), GFP_KERNEL);
>  	if (!res)
>  		return -ENOMEM;
>  
> @@ -129,6 +154,10 @@ static int lpc_sch_populate_cell(struct pci_dev *pdev, int where,
>  	cell->ignore_resource_conflicts = true;
>  	cell->id = id;
>  
> +	ret = lpc_sch_get_irq(++res, irq);
> +	if (!ret)
> +		cell->num_resources++;

Once again, you're masking errors.  If it's not an error, don't return
one.  If it is, filter it back and fail the bind.

>  	return 0;
>  }
>  
> @@ -141,19 +170,19 @@ static int lpc_sch_probe(struct pci_dev *dev,
>  	int ret;
>  
>  	ret = lpc_sch_populate_cell(dev, SMBASE, "isch_smbus",
> -				    info->io_size_smbus,
> +				    info->io_size_smbus, -1,
>  				    id->device, &lpc_sch_cells[cells]);
>  	if (!ret)
>  		cells++;
>  
>  	ret = lpc_sch_populate_cell(dev, GPIOBASE, "sch_gpio",
> -				    info->io_size_gpio,
> +				    info->io_size_gpio, info->irq_gpio,
>  				    id->device, &lpc_sch_cells[cells]);
>  	if (!ret)
>  		cells++;
>  
>  	ret = lpc_sch_populate_cell(dev, WDTBASE, "ie6xx_wdt",
> -				    info->io_size_wdt,
> +				    info->io_size_wdt, -1,
>  				    id->device, &lpc_sch_cells[cells]);
>  	if (!ret)
>  		cells++;
-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH v1 1/5] mfd: lpc_sch: reduce duplicate code

[Andy Shevchenko]

On Mon, 2014-09-01 at 10:13 +0100, Lee Jones wrote:
> On Fri, 22 Aug 2014, Andy Shevchenko wrote:
> 
> > This patch refactors the driver to use helper functions instead of
> > copy'n'pasted pieces of code.
> > 
> > Tested-by: Chang Rebecca Swee Fun <rebecca.swee.fun.chang@intel.com>
> > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > ---
> >  drivers/mfd/lpc_sch.c | 146 ++++++++++++++++++++++----------------------------
> >  1 file changed, 64 insertions(+), 82 deletions(-)
> > 
> > diff --git a/drivers/mfd/lpc_sch.c b/drivers/mfd/lpc_sch.c
> > index 4ee7550..0f01ef0 100644
> > --- a/drivers/mfd/lpc_sch.c
> > +++ b/drivers/mfd/lpc_sch.c
> > @@ -40,41 +40,6 @@
> 
> [...]

Thanks for review, my answers below.

> 
> > -static int lpc_sch_probe(struct pci_dev *dev,
> > -				const struct pci_device_id *id)
> > +static int lpc_sch_get_io(struct pci_dev *pdev, int where, const char *name,
> > +			  struct resource *res, int size)
> >  {
> >  	unsigned int base_addr_cfg;
> >  	unsigned short base_addr;
> > -	int i, cells = 0;
> > -	int ret;
> >  
> > -	pci_read_config_dword(dev, SMBASE, &base_addr_cfg);
> > +	pci_read_config_dword(pdev, where, &base_addr_cfg);
> >  	base_addr = 0;
> >  	if (!(base_addr_cfg & (1 << 31)))
> > -		dev_warn(&dev->dev, "Decode of the SMBus I/O range disabled\n");
> > +		dev_warn(&pdev->dev, "Decode of the %s I/O range disabled\n",
> > +			 name);
> >  	else
> >  		base_addr = (unsigned short)base_addr_cfg;
> >  
> >  	if (base_addr == 0) {
> > -		dev_warn(&dev->dev, "I/O space for SMBus uninitialized\n");
> > -	} else {
> > -		lpc_sch_cells[cells++] = isch_smbus_cell;
> > -		smbus_sch_resource.start = base_addr;
> > -		smbus_sch_resource.end = base_addr + SMBUS_IO_SIZE - 1;
> > +		dev_warn(&pdev->dev, "I/O space for %s uninitialized\n", name);
> > +		return -ENODEV;
> 
> If you're going to return an error, you need to use dev_err() above.

Okay.

> >  	}
> >  
> > -	pci_read_config_dword(dev, GPIOBASE, &base_addr_cfg);
> > -	base_addr = 0;
> > -	if (!(base_addr_cfg & (1 << 31)))
> > -		dev_warn(&dev->dev, "Decode of the GPIO I/O range disabled\n");
> > +	res->start = base_addr;
> > +	res->end = base_addr + size - 1;
> > +	res->flags = IORESOURCE_IO;
> > +
> > +	return 0;
> > +}
> > +
> > +static int lpc_sch_populate_cell(struct pci_dev *pdev, int where,
> > +				 const char *name, int size, int id,
> > +				 struct mfd_cell *cell)
> > +{
> > +	struct resource *res;
> > +	int ret;
> > +
> > +	res = devm_kzalloc(&pdev->dev, sizeof(*res), GFP_KERNEL);
> > +	if (!res)
> > +		return -ENOMEM;
> > +
> > +	ret = lpc_sch_get_io(pdev, where, name, res, size);
> > +	if (ret)
> > +		return ret;
> > +
> > +	memset(cell, 0, sizeof(*cell));
> > +
> > +	cell->name = name;
> > +	cell->resources = res;
> > +	cell->num_resources = 1;
> > +	cell->ignore_resource_conflicts = true;
> > +	cell->id = id;
> > +
> > +	return 0;
> > +}
> > +
> > +static int lpc_sch_probe(struct pci_dev *dev,
> > +			 const struct pci_device_id *id)
> > +{
> > +	struct mfd_cell lpc_sch_cells[3];
> > +	int size, cells = 0;
> > +	int ret;
> > +
> > +	ret = lpc_sch_populate_cell(dev, SMBASE, "isch_smbus", SMBUS_IO_SIZE,
> > +				    id->device, &lpc_sch_cells[cells]);
> > +	if (!ret)
> > +		cells++;
> 
> You're masking errors here.  You need to return on error.

No, I don't. It's a local error, I just need to understand if the HW has
or hasn't the IP part we're looking for.

I could, let's say, return true or false, if you prefer, with the above
meaning.


-- 
Andy Shevchenko <andriy.shevchenko@intel.com>
Intel Finland Oy

Re: [PATCH v1 2/5] mfd: lpc_sch: better code manageability with chipset info struct

[Andy Shevchenko]

On Mon, 2014-09-01 at 10:16 +0100, Lee Jones wrote:
> On Fri, 22 Aug 2014, Andy Shevchenko wrote:
> 
> > Introduce additional struct to hold chipset info. This chipset
> > info will be used to store features that are supported by specific
> > processor or chipset. LPC_SCH supports SMBUS, GPIO and WDT features.
> > As this code base might expand further to support more processors,
> > this implementation will help to keep code base clean and manageable.
> > 
> > Signed-off-by: Chang Rebecca Swee Fun <rebecca.swee.fun.chang@intel.com>
> > Tested-by: Chang Rebecca Swee Fun <rebecca.swee.fun.chang@intel.com>
> > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

[]


> The first patch would look a great deal cleaner if it had these
> changes in too.  Unless you have a really good reason not to, please
> consider squashing them.

The only reason behind is that this patch (in other form) was written by
Rebecca in the first place. I recommended to clean up before, and
actually did that clean up and amended Rebecca's patch.

So, if Rebecca has now objections I could squash it.


-- 
Andy Shevchenko <andriy.shevchenko@intel.com>
Intel Finland Oy

Re: [PATCH v1 4/5] mfd: lpc_sch: Add support for Intel Quark X1000

[Andy Shevchenko]

On Mon, 2014-09-01 at 10:22 +0100, Lee Jones wrote:
> On Fri, 22 Aug 2014, Andy Shevchenko wrote:
> 
> > Intel Quark X1000 SoC supports IRQ based GPIO. This patch will
> > enable MFD support for Quark X1000 and provide IRQ resources
> > to Quark X1000 GPIO device driver.
> > 
> > Signed-off-by: Chang Rebecca Swee Fun <rebecca.swee.fun.chang@intel.com>
> > Tested-by: Chang Rebecca Swee Fun <rebecca.swee.fun.chang@intel.com>
> > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

See my answers below.

> > ---
> >  drivers/mfd/lpc_sch.c | 41 +++++++++++++++++++++++++++++++++++------
> >  1 file changed, 35 insertions(+), 6 deletions(-)
> > 
> > diff --git a/drivers/mfd/lpc_sch.c b/drivers/mfd/lpc_sch.c
> > index c4eb359..6145a4c 100644
> > --- a/drivers/mfd/lpc_sch.c
> > +++ b/drivers/mfd/lpc_sch.c
> > @@ -37,6 +37,9 @@
> >  #define GPIO_IO_SIZE	64
> >  #define GPIO_IO_SIZE_CENTERTON	128
> >  
> > +/* Intel Quark X1000 GPIO IRQ Number */
> > +#define GPIO_IRQ_QUARK_X1000	9
> > +
> >  #define WDTBASE		0x84
> >  #define WDT_IO_SIZE	64
> >  
> > @@ -44,28 +47,37 @@ enum sch_chipsets {
> >  	LPC_SCH = 0,		/* Intel Poulsbo SCH */
> >  	LPC_ITC,		/* Intel Tunnel Creek */
> >  	LPC_CENTERTON,		/* Intel Centerton */
> > +	LPC_QUARK_X1000,	/* Intel Quark X1000 */
> >  };
> >  
> >  struct lpc_sch_info {
> >  	unsigned int io_size_smbus;
> >  	unsigned int io_size_gpio;
> >  	unsigned int io_size_wdt;
> > +	int irq_gpio;
> >  };
> >  
> >  static struct lpc_sch_info sch_chipset_info[] = {
> >  	[LPC_SCH] = {
> >  		.io_size_smbus = SMBUS_IO_SIZE,
> >  		.io_size_gpio = GPIO_IO_SIZE,
> > +		.irq_gpio = -1,
> >  	},
> >  	[LPC_ITC] = {
> >  		.io_size_smbus = SMBUS_IO_SIZE,
> >  		.io_size_gpio = GPIO_IO_SIZE,
> >  		.io_size_wdt = WDT_IO_SIZE,
> > +		.irq_gpio = -1,
> >  	},
> >  	[LPC_CENTERTON] = {
> >  		.io_size_smbus = SMBUS_IO_SIZE,
> >  		.io_size_gpio = GPIO_IO_SIZE_CENTERTON,
> >  		.io_size_wdt = WDT_IO_SIZE,
> > +		.irq_gpio = -1,
> > +	},
> > +	[LPC_QUARK_X1000] = {
> > +		.io_size_gpio = GPIO_IO_SIZE,
> > +		.irq_gpio = GPIO_IRQ_QUARK_X1000,
> >  	},
> >  };
> >  
> > @@ -73,6 +85,7 @@ static const struct pci_device_id lpc_sch_ids[] = {
> >  	{ PCI_VDEVICE(INTEL, PCI_DEVICE_ID_INTEL_SCH_LPC), LPC_SCH },
> >  	{ PCI_VDEVICE(INTEL, PCI_DEVICE_ID_INTEL_ITC_LPC), LPC_ITC },
> >  	{ PCI_VDEVICE(INTEL, PCI_DEVICE_ID_INTEL_CENTERTON_ILB), LPC_CENTERTON },
> > +	{ PCI_VDEVICE(INTEL, PCI_DEVICE_ID_INTEL_QUARK_X1000_ILB), LPC_QUARK_X1000 },
> >  	{ 0, }
> >  };
> >  MODULE_DEVICE_TABLE(pci, lpc_sch_ids);
> > @@ -106,14 +119,26 @@ static int lpc_sch_get_io(struct pci_dev *pdev, int where, const char *name,
> >  	return 0;
> >  }
> >  
> > +static int lpc_sch_get_irq(struct resource *res, int irq)
> > +{
> > +	if (irq < 0)
> > +		return -EINVAL;
> > +
> > +	res->start = irq;
> > +	res->end = irq;
> > +	res->flags = IORESOURCE_IRQ;
> > +
> > +	return 0;
> > +}
> 
> Why does this need to be a separate function?
> 
> I fear that the code will become unnecessarily fragmented, just for the
> sake of it.

I could do this as a condition branch.

> 
> >  static int lpc_sch_populate_cell(struct pci_dev *pdev, int where,
> > -				 const char *name, int size, int id,
> > -				 struct mfd_cell *cell)
> > +				 const char *name, int size, int irq,
> > +				 int id, struct mfd_cell *cell)
> >  {
> >  	struct resource *res;
> >  	int ret;
> >  
> > -	res = devm_kzalloc(&pdev->dev, sizeof(*res), GFP_KERNEL);
> > +	res = devm_kcalloc(&pdev->dev, 2, sizeof(*res), GFP_KERNEL);
> >  	if (!res)
> >  		return -ENOMEM;
> >  
> > @@ -129,6 +154,10 @@ static int lpc_sch_populate_cell(struct pci_dev *pdev, int where,
> >  	cell->ignore_resource_conflicts = true;
> >  	cell->id = id;
> >  
> > +	ret = lpc_sch_get_irq(++res, irq);
> > +	if (!ret)
> > +		cell->num_resources++;
> 
> Once again, you're masking errors.  If it's not an error, don't return
> one.  If it is, filter it back and fail the bind.

I have to know if there is such resource or not. Taking into account you
prefer to see lpc_sch_get_irq embedded in here I just can do as a
condition branch and there will be no more question I hope.

> 
> >  	return 0;
> >  }
> >  
> > @@ -141,19 +170,19 @@ static int lpc_sch_probe(struct pci_dev *dev,
> >  	int ret;
> >  
> >  	ret = lpc_sch_populate_cell(dev, SMBASE, "isch_smbus",
> > -				    info->io_size_smbus,
> > +				    info->io_size_smbus, -1,
> >  				    id->device, &lpc_sch_cells[cells]);
> >  	if (!ret)
> >  		cells++;
> >  
> >  	ret = lpc_sch_populate_cell(dev, GPIOBASE, "sch_gpio",
> > -				    info->io_size_gpio,
> > +				    info->io_size_gpio, info->irq_gpio,
> >  				    id->device, &lpc_sch_cells[cells]);
> >  	if (!ret)
> >  		cells++;
> >  
> >  	ret = lpc_sch_populate_cell(dev, WDTBASE, "ie6xx_wdt",
> > -				    info->io_size_wdt,
> > +				    info->io_size_wdt, -1,
> >  				    id->device, &lpc_sch_cells[cells]);
> >  	if (!ret)
> >  		cells++;


-- 
Andy Shevchenko <andriy.shevchenko@intel.com>
Intel Finland Oy

Re: [PATCH v1 4/5] mfd: lpc_sch: Add support for Intel Quark X1000

[Lee Jones]

On Mon, 01 Sep 2014, Andy Shevchenko wrote:

> On Mon, 2014-09-01 at 10:22 +0100, Lee Jones wrote:
> > On Fri, 22 Aug 2014, Andy Shevchenko wrote:
> > 
> > > Intel Quark X1000 SoC supports IRQ based GPIO. This patch will
> > > enable MFD support for Quark X1000 and provide IRQ resources
> > > to Quark X1000 GPIO device driver.
> > > 
> > > Signed-off-by: Chang Rebecca Swee Fun <rebecca.swee.fun.chang@intel.com>
> > > Tested-by: Chang Rebecca Swee Fun <rebecca.swee.fun.chang@intel.com>
> > > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> 
> See my answers below.
> 
> > > ---
> > >  drivers/mfd/lpc_sch.c | 41 +++++++++++++++++++++++++++++++++++------
> > >  1 file changed, 35 insertions(+), 6 deletions(-)
> > > 
> > > diff --git a/drivers/mfd/lpc_sch.c b/drivers/mfd/lpc_sch.c
> > > index c4eb359..6145a4c 100644
> > > --- a/drivers/mfd/lpc_sch.c
> > > +++ b/drivers/mfd/lpc_sch.c
> > > @@ -37,6 +37,9 @@
> > >  #define GPIO_IO_SIZE	64
> > >  #define GPIO_IO_SIZE_CENTERTON	128
> > >  
> > > +/* Intel Quark X1000 GPIO IRQ Number */
> > > +#define GPIO_IRQ_QUARK_X1000	9
> > > +
> > >  #define WDTBASE		0x84
> > >  #define WDT_IO_SIZE	64
> > >  
> > > @@ -44,28 +47,37 @@ enum sch_chipsets {
> > >  	LPC_SCH = 0,		/* Intel Poulsbo SCH */
> > >  	LPC_ITC,		/* Intel Tunnel Creek */
> > >  	LPC_CENTERTON,		/* Intel Centerton */
> > > +	LPC_QUARK_X1000,	/* Intel Quark X1000 */
> > >  };
> > >  
> > >  struct lpc_sch_info {
> > >  	unsigned int io_size_smbus;
> > >  	unsigned int io_size_gpio;
> > >  	unsigned int io_size_wdt;
> > > +	int irq_gpio;
> > >  };
> > >  
> > >  static struct lpc_sch_info sch_chipset_info[] = {
> > >  	[LPC_SCH] = {
> > >  		.io_size_smbus = SMBUS_IO_SIZE,
> > >  		.io_size_gpio = GPIO_IO_SIZE,
> > > +		.irq_gpio = -1,
> > >  	},
> > >  	[LPC_ITC] = {
> > >  		.io_size_smbus = SMBUS_IO_SIZE,
> > >  		.io_size_gpio = GPIO_IO_SIZE,
> > >  		.io_size_wdt = WDT_IO_SIZE,
> > > +		.irq_gpio = -1,
> > >  	},
> > >  	[LPC_CENTERTON] = {
> > >  		.io_size_smbus = SMBUS_IO_SIZE,
> > >  		.io_size_gpio = GPIO_IO_SIZE_CENTERTON,
> > >  		.io_size_wdt = WDT_IO_SIZE,
> > > +		.irq_gpio = -1,
> > > +	},
> > > +	[LPC_QUARK_X1000] = {
> > > +		.io_size_gpio = GPIO_IO_SIZE,
> > > +		.irq_gpio = GPIO_IRQ_QUARK_X1000,
> > >  	},
> > >  };
> > >  
> > > @@ -73,6 +85,7 @@ static const struct pci_device_id lpc_sch_ids[] = {
> > >  	{ PCI_VDEVICE(INTEL, PCI_DEVICE_ID_INTEL_SCH_LPC), LPC_SCH },
> > >  	{ PCI_VDEVICE(INTEL, PCI_DEVICE_ID_INTEL_ITC_LPC), LPC_ITC },
> > >  	{ PCI_VDEVICE(INTEL, PCI_DEVICE_ID_INTEL_CENTERTON_ILB), LPC_CENTERTON },
> > > +	{ PCI_VDEVICE(INTEL, PCI_DEVICE_ID_INTEL_QUARK_X1000_ILB), LPC_QUARK_X1000 },
> > >  	{ 0, }
> > >  };
> > >  MODULE_DEVICE_TABLE(pci, lpc_sch_ids);
> > > @@ -106,14 +119,26 @@ static int lpc_sch_get_io(struct pci_dev *pdev, int where, const char *name,
> > >  	return 0;
> > >  }
> > >  
> > > +static int lpc_sch_get_irq(struct resource *res, int irq)
> > > +{
> > > +	if (irq < 0)
> > > +		return -EINVAL;
> > > +
> > > +	res->start = irq;
> > > +	res->end = irq;
> > > +	res->flags = IORESOURCE_IRQ;
> > > +
> > > +	return 0;
> > > +}
> > 
> > Why does this need to be a separate function?
> > 
> > I fear that the code will become unnecessarily fragmented, just for the
> > sake of it.
> 
> I could do this as a condition branch.

You could, but you don't.

> > >  static int lpc_sch_populate_cell(struct pci_dev *pdev, int where,
> > > -				 const char *name, int size, int id,
> > > -				 struct mfd_cell *cell)
> > > +				 const char *name, int size, int irq,
> > > +				 int id, struct mfd_cell *cell)
> > >  {
> > >  	struct resource *res;
> > >  	int ret;
> > >  
> > > -	res = devm_kzalloc(&pdev->dev, sizeof(*res), GFP_KERNEL);
> > > +	res = devm_kcalloc(&pdev->dev, 2, sizeof(*res), GFP_KERNEL);
> > >  	if (!res)
> > >  		return -ENOMEM;
> > >  
> > > @@ -129,6 +154,10 @@ static int lpc_sch_populate_cell(struct pci_dev *pdev, int where,
> > >  	cell->ignore_resource_conflicts = true;
> > >  	cell->id = id;
> > >  
> > > +	ret = lpc_sch_get_irq(++res, irq);
> > > +	if (!ret)
> > > +		cell->num_resources++;
> > 
> > Once again, you're masking errors.  If it's not an error, don't return
> > one.  If it is, filter it back and fail the bind.
> 
> I have to know if there is such resource or not. Taking into account you
> prefer to see lpc_sch_get_irq embedded in here I just can do as a
> condition branch and there will be no more question I hope.

This is true.

> > >  	return 0;
> > >  }
> > >  
> > > @@ -141,19 +170,19 @@ static int lpc_sch_probe(struct pci_dev *dev,
> > >  	int ret;
> > >  
> > >  	ret = lpc_sch_populate_cell(dev, SMBASE, "isch_smbus",
> > > -				    info->io_size_smbus,
> > > +				    info->io_size_smbus, -1,
> > >  				    id->device, &lpc_sch_cells[cells]);
> > >  	if (!ret)
> > >  		cells++;
> > >  
> > >  	ret = lpc_sch_populate_cell(dev, GPIOBASE, "sch_gpio",
> > > -				    info->io_size_gpio,
> > > +				    info->io_size_gpio, info->irq_gpio,
> > >  				    id->device, &lpc_sch_cells[cells]);
> > >  	if (!ret)
> > >  		cells++;
> > >  
> > >  	ret = lpc_sch_populate_cell(dev, WDTBASE, "ie6xx_wdt",
> > > -				    info->io_size_wdt,
> > > +				    info->io_size_wdt, -1,
> > >  				    id->device, &lpc_sch_cells[cells]);
> > >  	if (!ret)
> > >  		cells++;
> 
> 

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH v1 1/5] mfd: lpc_sch: reduce duplicate code

[Lee Jones]

On Mon, 01 Sep 2014, Andy Shevchenko wrote:

> On Mon, 2014-09-01 at 10:13 +0100, Lee Jones wrote:
> > On Fri, 22 Aug 2014, Andy Shevchenko wrote:
> > 
> > > This patch refactors the driver to use helper functions instead of
> > > copy'n'pasted pieces of code.
> > > 
> > > Tested-by: Chang Rebecca Swee Fun <rebecca.swee.fun.chang@intel.com>
> > > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > > ---
> > >  drivers/mfd/lpc_sch.c | 146 ++++++++++++++++++++++----------------------------
> > >  1 file changed, 64 insertions(+), 82 deletions(-)
> > > 
> > > diff --git a/drivers/mfd/lpc_sch.c b/drivers/mfd/lpc_sch.c
> > > index 4ee7550..0f01ef0 100644
> > > --- a/drivers/mfd/lpc_sch.c
> > > +++ b/drivers/mfd/lpc_sch.c
> > > @@ -40,41 +40,6 @@
> > 
> > [...]
> 
> Thanks for review, my answers below.
> 
> > 
> > > -static int lpc_sch_probe(struct pci_dev *dev,
> > > -				const struct pci_device_id *id)
> > > +static int lpc_sch_get_io(struct pci_dev *pdev, int where, const char *name,
> > > +			  struct resource *res, int size)
> > >  {
> > >  	unsigned int base_addr_cfg;
> > >  	unsigned short base_addr;
> > > -	int i, cells = 0;
> > > -	int ret;
> > >  
> > > -	pci_read_config_dword(dev, SMBASE, &base_addr_cfg);
> > > +	pci_read_config_dword(pdev, where, &base_addr_cfg);
> > >  	base_addr = 0;
> > >  	if (!(base_addr_cfg & (1 << 31)))
> > > -		dev_warn(&dev->dev, "Decode of the SMBus I/O range disabled\n");
> > > +		dev_warn(&pdev->dev, "Decode of the %s I/O range disabled\n",
> > > +			 name);
> > >  	else
> > >  		base_addr = (unsigned short)base_addr_cfg;
> > >  
> > >  	if (base_addr == 0) {
> > > -		dev_warn(&dev->dev, "I/O space for SMBus uninitialized\n");
> > > -	} else {
> > > -		lpc_sch_cells[cells++] = isch_smbus_cell;
> > > -		smbus_sch_resource.start = base_addr;
> > > -		smbus_sch_resource.end = base_addr + SMBUS_IO_SIZE - 1;
> > > +		dev_warn(&pdev->dev, "I/O space for %s uninitialized\n", name);
> > > +		return -ENODEV;
> > 
> > If you're going to return an error, you need to use dev_err() above.
> 
> Okay.
> 
> > >  	}
> > >  
> > > -	pci_read_config_dword(dev, GPIOBASE, &base_addr_cfg);
> > > -	base_addr = 0;
> > > -	if (!(base_addr_cfg & (1 << 31)))
> > > -		dev_warn(&dev->dev, "Decode of the GPIO I/O range disabled\n");
> > > +	res->start = base_addr;
> > > +	res->end = base_addr + size - 1;
> > > +	res->flags = IORESOURCE_IO;
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +static int lpc_sch_populate_cell(struct pci_dev *pdev, int where,
> > > +				 const char *name, int size, int id,
> > > +				 struct mfd_cell *cell)
> > > +{
> > > +	struct resource *res;
> > > +	int ret;
> > > +
> > > +	res = devm_kzalloc(&pdev->dev, sizeof(*res), GFP_KERNEL);
> > > +	if (!res)
> > > +		return -ENOMEM;
> > > +
> > > +	ret = lpc_sch_get_io(pdev, where, name, res, size);
> > > +	if (ret)
> > > +		return ret;
> > > +
> > > +	memset(cell, 0, sizeof(*cell));
> > > +
> > > +	cell->name = name;
> > > +	cell->resources = res;
> > > +	cell->num_resources = 1;
> > > +	cell->ignore_resource_conflicts = true;
> > > +	cell->id = id;
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +static int lpc_sch_probe(struct pci_dev *dev,
> > > +			 const struct pci_device_id *id)
> > > +{
> > > +	struct mfd_cell lpc_sch_cells[3];
> > > +	int size, cells = 0;
> > > +	int ret;
> > > +
> > > +	ret = lpc_sch_populate_cell(dev, SMBASE, "isch_smbus", SMBUS_IO_SIZE,
> > > +				    id->device, &lpc_sch_cells[cells]);
> > > +	if (!ret)
> > > +		cells++;
> > 
> > You're masking errors here.  You need to return on error.
> 
> No, I don't. It's a local error, I just need to understand if the HW has
> or hasn't the IP part we're looking for.

Yes, you do.  -ENOMEM is not a local error, it needs to be returned.

> I could, let's say, return true or false, if you prefer, with the above
> meaning.

I don't see this very often, which makes me wonder if it's the right
thing to do at all; however, if you're going to this then please
return a local state, instead of a Linux Error Code.

#define LCP_SKIP_RESOURCE /* Seems resonable? */

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH v1 0/5] mfd: lpc_sch: Intel Quark support

[Andy Shevchenko]

On Fri, 2014-08-29 at 12:00 +0100, Lee Jones wrote:
> On Fri, 29 Aug 2014, Andy Shevchenko wrote:
> 
> > On Fri, 2014-08-22 at 13:58 +0300, Andy Shevchenko wrote:
> > > This brings support of the LPC SCH device found on Intel Quark SoC. Patches
> > > were compile tested, and driver runs on actual hardware.
> > > 
> > > Please, note that PCI ID will be used in future in the gpio-sch driver.
> > 
> > Bjorn, could you Ack or comment patch 3/5?
> > 
> > Lee, do you have any comments on the series?
> 
> I have marked them as 'important' and will get round to them when I
> can.  This week has been hectic.  I will get round to it next week.

Thanks for your comments. I will fix everything we agreed about and send
v2. For now I'm waiting for Rebecca's comment regarding to squashing
patch 1/5 and 2/5.

> 
> > > Andy Shevchenko (4):
> > >   mfd: lpc_sch: reduce duplicate code
> > >   mfd: lpc_sch: better code manageability with chipset info struct
> > >   mfd: lpc_sch: Add support for Intel Quark X1000
> > >   mfd: lpc_sch: remove FSF address
> > > 
> > > Josef Ahmad (1):
> > >   pci_ids: add support for Intel Quark ILB
> > > 
> > >  drivers/mfd/lpc_sch.c   | 207 ++++++++++++++++++++++++++++--------------------
> > >  include/linux/pci_ids.h |   1 +
> > >  2 files changed, 122 insertions(+), 86 deletions(-)
> > > 
> > 
> > 
> 


-- 
Andy Shevchenko <andriy.shevchenko@intel.com>
Intel Finland Oy

Re: [PATCH v1 1/5] mfd: lpc_sch: reduce duplicate code

[Andy Shevchenko]

On Mon, 2014-09-01 at 12:38 +0100, Lee Jones wrote:
> On Mon, 01 Sep 2014, Andy Shevchenko wrote:
> 
> > On Mon, 2014-09-01 at 10:13 +0100, Lee Jones wrote:
> > > On Fri, 22 Aug 2014, Andy Shevchenko wrote:
> > > 
> > > > This patch refactors the driver to use helper functions instead of
> > > > copy'n'pasted pieces of code.
> > > > 
> > > > Tested-by: Chang Rebecca Swee Fun <rebecca.swee.fun.chang@intel.com>
> > > > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > > > ---
> > > >  drivers/mfd/lpc_sch.c | 146 ++++++++++++++++++++++----------------------------
> > > >  1 file changed, 64 insertions(+), 82 deletions(-)
> > > > 
> > > > diff --git a/drivers/mfd/lpc_sch.c b/drivers/mfd/lpc_sch.c
> > > > index 4ee7550..0f01ef0 100644
> > > > --- a/drivers/mfd/lpc_sch.c
> > > > +++ b/drivers/mfd/lpc_sch.c
> > > > @@ -40,41 +40,6 @@
> > > 
> > > [...]
> > 
> > Thanks for review, my answers below.
> > 
> > > 
> > > > -static int lpc_sch_probe(struct pci_dev *dev,
> > > > -				const struct pci_device_id *id)
> > > > +static int lpc_sch_get_io(struct pci_dev *pdev, int where, const char *name,
> > > > +			  struct resource *res, int size)
> > > >  {
> > > >  	unsigned int base_addr_cfg;
> > > >  	unsigned short base_addr;
> > > > -	int i, cells = 0;
> > > > -	int ret;
> > > >  
> > > > -	pci_read_config_dword(dev, SMBASE, &base_addr_cfg);
> > > > +	pci_read_config_dword(pdev, where, &base_addr_cfg);
> > > >  	base_addr = 0;
> > > >  	if (!(base_addr_cfg & (1 << 31)))
> > > > -		dev_warn(&dev->dev, "Decode of the SMBus I/O range disabled\n");
> > > > +		dev_warn(&pdev->dev, "Decode of the %s I/O range disabled\n",
> > > > +			 name);
> > > >  	else
> > > >  		base_addr = (unsigned short)base_addr_cfg;
> > > >  
> > > >  	if (base_addr == 0) {
> > > > -		dev_warn(&dev->dev, "I/O space for SMBus uninitialized\n");
> > > > -	} else {
> > > > -		lpc_sch_cells[cells++] = isch_smbus_cell;
> > > > -		smbus_sch_resource.start = base_addr;
> > > > -		smbus_sch_resource.end = base_addr + SMBUS_IO_SIZE - 1;
> > > > +		dev_warn(&pdev->dev, "I/O space for %s uninitialized\n", name);
> > > > +		return -ENODEV;
> > > 
> > > If you're going to return an error, you need to use dev_err() above.
> > 
> > Okay.
> > 
> > > >  	}
> > > >  
> > > > -	pci_read_config_dword(dev, GPIOBASE, &base_addr_cfg);
> > > > -	base_addr = 0;
> > > > -	if (!(base_addr_cfg & (1 << 31)))
> > > > -		dev_warn(&dev->dev, "Decode of the GPIO I/O range disabled\n");
> > > > +	res->start = base_addr;
> > > > +	res->end = base_addr + size - 1;
> > > > +	res->flags = IORESOURCE_IO;
> > > > +
> > > > +	return 0;
> > > > +}
> > > > +
> > > > +static int lpc_sch_populate_cell(struct pci_dev *pdev, int where,
> > > > +				 const char *name, int size, int id,
> > > > +				 struct mfd_cell *cell)
> > > > +{
> > > > +	struct resource *res;
> > > > +	int ret;
> > > > +
> > > > +	res = devm_kzalloc(&pdev->dev, sizeof(*res), GFP_KERNEL);
> > > > +	if (!res)
> > > > +		return -ENOMEM;
> > > > +
> > > > +	ret = lpc_sch_get_io(pdev, where, name, res, size);
> > > > +	if (ret)
> > > > +		return ret;
> > > > +
> > > > +	memset(cell, 0, sizeof(*cell));
> > > > +
> > > > +	cell->name = name;
> > > > +	cell->resources = res;
> > > > +	cell->num_resources = 1;
> > > > +	cell->ignore_resource_conflicts = true;
> > > > +	cell->id = id;
> > > > +
> > > > +	return 0;
> > > > +}
> > > > +
> > > > +static int lpc_sch_probe(struct pci_dev *dev,
> > > > +			 const struct pci_device_id *id)
> > > > +{
> > > > +	struct mfd_cell lpc_sch_cells[3];
> > > > +	int size, cells = 0;
> > > > +	int ret;
> > > > +
> > > > +	ret = lpc_sch_populate_cell(dev, SMBASE, "isch_smbus", SMBUS_IO_SIZE,
> > > > +				    id->device, &lpc_sch_cells[cells]);
> > > > +	if (!ret)
> > > > +		cells++;
> > > 
> > > You're masking errors here.  You need to return on error.
> > 
> > No, I don't. It's a local error, I just need to understand if the HW has
> > or hasn't the IP part we're looking for.
> 
> Yes, you do.  -ENOMEM is not a local error, it needs to be returned.
> 
> > I could, let's say, return true or false, if you prefer, with the above
> > meaning.
> 
> I don't see this very often, which makes me wonder if it's the right
> thing to do at all; however, if you're going to this then please
> return a local state, instead of a Linux Error Code.

Okay, what about 1 — IP is present and we have no error, 0 — no ip, no
error, -err — error that should be returned, would it work for you?

> #define LCP_SKIP_RESOURCE /* Seems resonable? */



-- 
Andy Shevchenko <andriy.shevchenko@intel.com>
Intel Finland Oy

Re: [PATCH v1 1/5] mfd: lpc_sch: reduce duplicate code

[Lee Jones]

On Mon, 01 Sep 2014, Andy Shevchenko wrote:

> On Mon, 2014-09-01 at 12:38 +0100, Lee Jones wrote:
> > On Mon, 01 Sep 2014, Andy Shevchenko wrote:
> > 
> > > On Mon, 2014-09-01 at 10:13 +0100, Lee Jones wrote:
> > > > On Fri, 22 Aug 2014, Andy Shevchenko wrote:
> > > > 
> > > > > This patch refactors the driver to use helper functions instead of
> > > > > copy'n'pasted pieces of code.
> > > > > 
> > > > > Tested-by: Chang Rebecca Swee Fun <rebecca.swee.fun.chang@intel.com>
> > > > > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > > > > ---
> > > > >  drivers/mfd/lpc_sch.c | 146 ++++++++++++++++++++++----------------------------
> > > > >  1 file changed, 64 insertions(+), 82 deletions(-)
> > > > > 
> > > > > diff --git a/drivers/mfd/lpc_sch.c b/drivers/mfd/lpc_sch.c
> > > > > index 4ee7550..0f01ef0 100644
> > > > > --- a/drivers/mfd/lpc_sch.c
> > > > > +++ b/drivers/mfd/lpc_sch.c
> > > > > @@ -40,41 +40,6 @@
> > > > 
> > > > [...]
> > > 
> > > Thanks for review, my answers below.
> > > 
> > > > 
> > > > > -static int lpc_sch_probe(struct pci_dev *dev,
> > > > > -				const struct pci_device_id *id)
> > > > > +static int lpc_sch_get_io(struct pci_dev *pdev, int where, const char *name,
> > > > > +			  struct resource *res, int size)
> > > > >  {
> > > > >  	unsigned int base_addr_cfg;
> > > > >  	unsigned short base_addr;
> > > > > -	int i, cells = 0;
> > > > > -	int ret;
> > > > >  
> > > > > -	pci_read_config_dword(dev, SMBASE, &base_addr_cfg);
> > > > > +	pci_read_config_dword(pdev, where, &base_addr_cfg);
> > > > >  	base_addr = 0;
> > > > >  	if (!(base_addr_cfg & (1 << 31)))
> > > > > -		dev_warn(&dev->dev, "Decode of the SMBus I/O range disabled\n");
> > > > > +		dev_warn(&pdev->dev, "Decode of the %s I/O range disabled\n",
> > > > > +			 name);
> > > > >  	else
> > > > >  		base_addr = (unsigned short)base_addr_cfg;
> > > > >  
> > > > >  	if (base_addr == 0) {
> > > > > -		dev_warn(&dev->dev, "I/O space for SMBus uninitialized\n");
> > > > > -	} else {
> > > > > -		lpc_sch_cells[cells++] = isch_smbus_cell;
> > > > > -		smbus_sch_resource.start = base_addr;
> > > > > -		smbus_sch_resource.end = base_addr + SMBUS_IO_SIZE - 1;
> > > > > +		dev_warn(&pdev->dev, "I/O space for %s uninitialized\n", name);
> > > > > +		return -ENODEV;
> > > > 
> > > > If you're going to return an error, you need to use dev_err() above.
> > > 
> > > Okay.
> > > 
> > > > >  	}
> > > > >  
> > > > > -	pci_read_config_dword(dev, GPIOBASE, &base_addr_cfg);
> > > > > -	base_addr = 0;
> > > > > -	if (!(base_addr_cfg & (1 << 31)))
> > > > > -		dev_warn(&dev->dev, "Decode of the GPIO I/O range disabled\n");
> > > > > +	res->start = base_addr;
> > > > > +	res->end = base_addr + size - 1;
> > > > > +	res->flags = IORESOURCE_IO;
> > > > > +
> > > > > +	return 0;
> > > > > +}
> > > > > +
> > > > > +static int lpc_sch_populate_cell(struct pci_dev *pdev, int where,
> > > > > +				 const char *name, int size, int id,
> > > > > +				 struct mfd_cell *cell)
> > > > > +{
> > > > > +	struct resource *res;
> > > > > +	int ret;
> > > > > +
> > > > > +	res = devm_kzalloc(&pdev->dev, sizeof(*res), GFP_KERNEL);
> > > > > +	if (!res)
> > > > > +		return -ENOMEM;
> > > > > +
> > > > > +	ret = lpc_sch_get_io(pdev, where, name, res, size);
> > > > > +	if (ret)
> > > > > +		return ret;
> > > > > +
> > > > > +	memset(cell, 0, sizeof(*cell));
> > > > > +
> > > > > +	cell->name = name;
> > > > > +	cell->resources = res;
> > > > > +	cell->num_resources = 1;
> > > > > +	cell->ignore_resource_conflicts = true;
> > > > > +	cell->id = id;
> > > > > +
> > > > > +	return 0;
> > > > > +}
> > > > > +
> > > > > +static int lpc_sch_probe(struct pci_dev *dev,
> > > > > +			 const struct pci_device_id *id)
> > > > > +{
> > > > > +	struct mfd_cell lpc_sch_cells[3];
> > > > > +	int size, cells = 0;
> > > > > +	int ret;
> > > > > +
> > > > > +	ret = lpc_sch_populate_cell(dev, SMBASE, "isch_smbus", SMBUS_IO_SIZE,
> > > > > +				    id->device, &lpc_sch_cells[cells]);
> > > > > +	if (!ret)
> > > > > +		cells++;
> > > > 
> > > > You're masking errors here.  You need to return on error.
> > > 
> > > No, I don't. It's a local error, I just need to understand if the HW has
> > > or hasn't the IP part we're looking for.
> > 
> > Yes, you do.  -ENOMEM is not a local error, it needs to be returned.
> > 
> > > I could, let's say, return true or false, if you prefer, with the above
> > > meaning.
> > 
> > I don't see this very often, which makes me wonder if it's the right
> > thing to do at all; however, if you're going to this then please
> > return a local state, instead of a Linux Error Code.
> 
> Okay, what about 1 — IP is present and we have no error, 0 — no ip, no
> error, -err — error that should be returned, would it work for you?

I would suggest -E<error_message> for messages to be passed back, 0
for 'everything found and correct' and >0 (#defined of course) for any
local messages you wish to pass 'IP not found', etc.

> > #define LCP_SKIP_RESOURCE /* Seems resonable? */
> 
> 
> 

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH v1 1/5] mfd: lpc_sch: reduce duplicate code

[Andy Shevchenko]

On Mon, 2014-09-01 at 14:46 +0100, Lee Jones wrote:

[]

> > Okay, what about 1 — IP is present and we have no error, 0 — no ip, no
> > error, -err — error that should be returned, would it work for you?
> 
> I would suggest -E<error_message> for messages to be passed back, 0
> for 'everything found and correct' and >0 (#defined of course) for any
> local messages you wish to pass 'IP not found', etc.
> 
> > > #define LCP_SKIP_RESOURCE /* Seems resonable? */

Okay, makes sense. Will do this in v2.


-- 
Andy Shevchenko <andriy.shevchenko@intel.com>
Intel Finland Oy

RE: [PATCH v1 3/5] pci_ids: add support for Intel Quark ILB

[Chang, Rebecca Swee Fun]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 2313 bytes --]



> -----Original Message-----
> From: Andy Shevchenko [mailto:andriy.shevchenko@linux.intel.com]
> Sent: 01 September, 2014 5:14 PM
> To: Bjorn Helgaas
> Cc: Lee Jones; linux-kernel@vger.kernel.org; Samuel Ortiz; Chang, Rebecca
> Swee Fun; Ahmad, Josef
> Subject: Re: [PATCH v1 3/5] pci_ids: add support for Intel Quark ILB
> 
> On Fri, 2014-08-29 at 08:27 -0600, Bjorn Helgaas wrote:
> > On Fri, Aug 22, 2014 at 4:58 AM, Andy Shevchenko
> > <andriy.shevchenko@linux.intel.com> wrote:
> > > From: Josef Ahmad <josef.ahmad@intel.com>
> > >
> > > This patch adds the PCI id for Intel Quark ILB.
> > > It will be used for GPIO and Multifunction device driver.
> > >
> > > Signed-off-by: Josef Ahmad <josef.ahmad@intel.com>
> > > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> >
> > Assuming that this will actually be used in more than one place:
> 
> Yes, look for the other IDs in the same driver (lpc_sch).
> 
> The question is will we have the update for sch_gpio in time to be included to
> v3.18? I think Rebecca can answer to this one.

Yes, we are working on updating sch_gpio but I can't give you a confirm answer whether it will be included in v3.18. Sch_gpio has been scheduled as my 2nd priority work currently. However, I will try to make it in time if possible.

Rebecca
> 
> >
> > Acked-by: Bjorn Helgaas <bhelgaas@google.com>
> >
> > > ---
> > >  include/linux/pci_ids.h | 1 +
> > >  1 file changed, 1 insertion(+)
> > >
> > > diff --git a/include/linux/pci_ids.h b/include/linux/pci_ids.h index
> > > 6ed0bb7..4e82195 100644
> > > --- a/include/linux/pci_ids.h
> > > +++ b/include/linux/pci_ids.h
> > > @@ -2557,6 +2557,7 @@
> > >  #define PCI_DEVICE_ID_INTEL_MFD_EMMC0  0x0823  #define
> > > PCI_DEVICE_ID_INTEL_MFD_EMMC1  0x0824
> > >  #define PCI_DEVICE_ID_INTEL_MRST_SD2   0x084F
> > > +#define PCI_DEVICE_ID_INTEL_QUARK_X1000_ILB    0x095E
> > >  #define PCI_DEVICE_ID_INTEL_I960       0x0960
> > >  #define PCI_DEVICE_ID_INTEL_I960RM     0x0962
> > >  #define PCI_DEVICE_ID_INTEL_CENTERTON_ILB      0x0c60
> > > --
> > > 2.1.0
> > >
> 
> 
> --
> 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¥

RE: [PATCH v1 2/5] mfd: lpc_sch: better code manageability with chipset info struct

[Chang, Rebecca Swee Fun]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 1766 bytes --]



> -----Original Message-----
> From: Andy Shevchenko [mailto:andriy.shevchenko@linux.intel.com]
> Sent: 01 September, 2014 6:25 PM
> To: Lee Jones
> Cc: Bjorn Helgaas; linux-kernel@vger.kernel.org; Samuel Ortiz; Chang, Rebecca
> Swee Fun
> Subject: Re: [PATCH v1 2/5] mfd: lpc_sch: better code manageability with
> chipset info struct
> 
> On Mon, 2014-09-01 at 10:16 +0100, Lee Jones wrote:
> > On Fri, 22 Aug 2014, Andy Shevchenko wrote:
> >
> > > Introduce additional struct to hold chipset info. This chipset info
> > > will be used to store features that are supported by specific
> > > processor or chipset. LPC_SCH supports SMBUS, GPIO and WDT features.
> > > As this code base might expand further to support more processors,
> > > this implementation will help to keep code base clean and manageable.
> > >
> > > Signed-off-by: Chang Rebecca Swee Fun
> > > <rebecca.swee.fun.chang@intel.com>
> > > Tested-by: Chang Rebecca Swee Fun <rebecca.swee.fun.chang@intel.com>
> > > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> 
> []
> 
> 
> > The first patch would look a great deal cleaner if it had these
> > changes in too.  Unless you have a really good reason not to, please
> > consider squashing them.
> 
> The only reason behind is that this patch (in other form) was written by
> Rebecca in the first place. I recommended to clean up before, and actually did
> that clean up and amended Rebecca's patch.
> 
> So, if Rebecca has now objections I could squash it.

I have no objections. Thanks.

> 
> 
> --
> 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¥

Re: [PATCH v1 2/5] mfd: lpc_sch: better code manageability with chipset info struct

[Lee Jones]

On Tue, 02 Sep 2014, Chang, Rebecca Swee Fun wrote:

> 
> 
> > -----Original Message-----
> > From: Andy Shevchenko [mailto:andriy.shevchenko@linux.intel.com]
> > Sent: 01 September, 2014 6:25 PM
> > To: Lee Jones
> > Cc: Bjorn Helgaas; linux-kernel@vger.kernel.org; Samuel Ortiz; Chang, Rebecca
> > Swee Fun
> > Subject: Re: [PATCH v1 2/5] mfd: lpc_sch: better code manageability with
> > chipset info struct
> > 
> > On Mon, 2014-09-01 at 10:16 +0100, Lee Jones wrote:
> > > On Fri, 22 Aug 2014, Andy Shevchenko wrote:
> > >
> > > > Introduce additional struct to hold chipset info. This chipset info
> > > > will be used to store features that are supported by specific
> > > > processor or chipset. LPC_SCH supports SMBUS, GPIO and WDT features.
> > > > As this code base might expand further to support more processors,
> > > > this implementation will help to keep code base clean and manageable.
> > > >
> > > > Signed-off-by: Chang Rebecca Swee Fun
> > > > <rebecca.swee.fun.chang@intel.com>
> > > > Tested-by: Chang Rebecca Swee Fun <rebecca.swee.fun.chang@intel.com>
> > > > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > 
> > []
> > 
> > 
> > > The first patch would look a great deal cleaner if it had these
> > > changes in too.  Unless you have a really good reason not to, please
> > > consider squashing them.
> > 
> > The only reason behind is that this patch (in other form) was written by
> > Rebecca in the first place. I recommended to clean up before, and actually did
> > that clean up and amended Rebecca's patch.
> > 
> > So, if Rebecca has now objections I could squash it.
> 
> I have no objections. Thanks.

Thanks Rebecca.

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog