diff mbox series

[v1,4/6] mfd: intel_quark_i2c_gpio: Convert to use software nodes

Message ID 20200608134300.76091-5-andriy.shevchenko@linux.intel.com
State New
Headers show
Series mfd: Make use of software nodes | expand

Commit Message

Andy Shevchenko June 8, 2020, 1:42 p.m. UTC 
The driver can provide a software node group instead of
passing legacy platform data. This will allow to drop
the legacy platform data structures along with unifying
a child device driver to use same interface for all
property providers, i.e. Device Tree, ACPI, and board files.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/mfd/intel_quark_i2c_gpio.c | 63 ++++++++++++++----------------
 1 file changed, 30 insertions(+), 33 deletions(-)

Comments

Linus Walleij June 10, 2020, 11:38 a.m. UTC | #1 
Hi Andy,

On Mon, Jun 8, 2020 at 3:43 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:

> -/* The base GPIO number under GPIOLIB framework */
> -#define INTEL_QUARK_MFD_GPIO_BASE      8

OK I see this was around before, sigh.
So it's not your fault. It was introduced in commit
60ae5b9f5cdd8 which I was not involved in reviewing,
for the record I would have said "no".

It is exploiting commit 3d2613c4289ff where I allowed
pdata to set the base so it is anyway my fault for not
noticing :(

But me complaining about this doesn't make things better.

Can we simply DELETE this assignment and just set base to
-1 in a separate patch before this patch and see what happens? It's
really unsafe to hardcode base like this.

+       PROPERTY_ENTRY_U32("snps,nr-gpios", 8),

This is however fine in principle but just use the existing generic
property "ngpios" and save this custom property.

Yours,
Linus Walleij
Andy Shevchenko June 10, 2020, 1:43 p.m. UTC | #2 
On Wed, Jun 10, 2020 at 01:38:54PM +0200, Linus Walleij wrote:
> On Mon, Jun 8, 2020 at 3:43 PM Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
> 
> > -/* The base GPIO number under GPIOLIB framework */
> > -#define INTEL_QUARK_MFD_GPIO_BASE      8
> 
> OK I see this was around before, sigh.
> So it's not your fault. It was introduced in commit
> 60ae5b9f5cdd8 which I was not involved in reviewing,
> for the record I would have said "no".
> 
> It is exploiting commit 3d2613c4289ff where I allowed
> pdata to set the base so it is anyway my fault for not
> noticing :(
> 
> But me complaining about this doesn't make things better.
> 
> Can we simply DELETE this assignment and just set base to
> -1 in a separate patch before this patch and see what happens? It's
> really unsafe to hardcode base like this.

I don't see easy way to do so, because as I explained in previous mail it will
affect relation (from firmware) to the fact that the numbering of this
controller is static for that platform.

> +       PROPERTY_ENTRY_U32("snps,nr-gpios", 8),
> 
> This is however fine in principle but just use the existing generic
> property "ngpios" and save this custom property.

This is established (not by ACPI!) property. Completely not my fault :-)
diff mbox series

Patch

diff --git a/drivers/mfd/intel_quark_i2c_gpio.c b/drivers/mfd/intel_quark_i2c_gpio.c
index 06b538dd124c..fb8c0b042ecc 100644
--- a/drivers/mfd/intel_quark_i2c_gpio.c
+++ b/drivers/mfd/intel_quark_i2c_gpio.c
@@ -16,8 +16,8 @@ 
 #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>
+#include <linux/property.h>
 
 /* PCI BAR for register base address */
 #define MFD_I2C_BAR		0
@@ -27,15 +27,6 @@ 
 #define MFD_ACPI_MATCH_GPIO	0ULL
 #define MFD_ACPI_MATCH_I2C	1ULL
 
-/* 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
 
@@ -89,17 +80,44 @@  static struct resource intel_quark_gpio_res[] = {
 	[INTEL_QUARK_IORES_MEM] = {
 		.flags = IORESOURCE_MEM,
 	},
+	[INTEL_QUARK_IORES_IRQ] = {
+		.flags = IORESOURCE_IRQ,
+	},
 };
 
 static struct mfd_cell_acpi_match intel_quark_acpi_match_gpio = {
 	.adr = MFD_ACPI_MATCH_GPIO,
 };
 
+static const struct software_node intel_quark_gpio_controller_node = {
+	.name = "intel-quark-gpio-controller",
+};
+
+static const struct property_entry intel_quark_gpio_portA_properties[] = {
+	PROPERTY_ENTRY_U32("reg", 0),
+	PROPERTY_ENTRY_U32("snps,nr-gpios", 8),
+	PROPERTY_ENTRY_U32("snps,gpio-base", 8),
+	{ }
+};
+
+static const struct software_node intel_quark_gpio_portA_node = {
+	.name = "portA",
+	.parent = &intel_quark_gpio_controller_node,
+	.properties = intel_quark_gpio_portA_properties,
+};
+
+static const struct software_node *intel_quark_gpio_node_group[] = {
+	&intel_quark_gpio_controller_node,
+	&intel_quark_gpio_portA_node,
+	NULL
+};
+
 static struct mfd_cell intel_quark_mfd_cells[] = {
 	{
 		.id = MFD_GPIO_BAR,
 		.name = "gpio-dwapb",
 		.acpi_match = &intel_quark_acpi_match_gpio,
+		.node_group = intel_quark_gpio_node_group,
 		.num_resources = ARRAY_SIZE(intel_quark_gpio_res),
 		.resources = intel_quark_gpio_res,
 		.ignore_resource_conflicts = true,
@@ -189,36 +207,15 @@  static int intel_quark_i2c_setup(struct pci_dev *pdev, struct mfd_cell *cell)
 
 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->fwnode	= NULL;
-	pdata->properties->idx		= 0;
-	pdata->properties->ngpio	= INTEL_QUARK_MFD_NGPIO;
-	pdata->properties->gpio_base	= INTEL_QUARK_MFD_GPIO_BASE;
-	pdata->properties->irq[0]	= pdev->irq;
-
-	cell->platform_data = pdata;
-	cell->pdata_size = sizeof(*pdata);
+	res[INTEL_QUARK_IORES_IRQ].start = pdev->irq;
+	res[INTEL_QUARK_IORES_IRQ].end = pdev->irq;
 
 	return 0;
 }