[PATCH v1 0/6] mfd: Make use of software nodes

[PATCH v1 0/6] mfd: Make use of software nodes

[Andy Shevchenko]

Some devices would need to have a hierarchy of properties and
child nodes passed to the child or children of MFD. For such case
we may utilize software nodes, which is superior on device properties.

Add support of software nodes to MFD core and convert one driver
to show how it looks like. This allows to get rid of legacy platform
data.

The change has been tested on Intel Galileo Gen 2.

Andy Shevchenko (5):
  gpio: dwapb: Replace irq_shared flag with fwnode type check
  gpio: dwapb: Read GPIO base from snps,gpio-base property
  mfd: intel_quark_i2c_gpio: Convert to use software nodes
  gpio: dwapb: Get rid of legacy platform data
  gpio: dwapb: Define magic number for IRQ and GPIO lines

Heikki Krogerus (1):
  mfd: core: Propagate software node group to the sub devices

 drivers/gpio/gpio-dwapb.c                | 44 +++++++++-------
 drivers/mfd/intel_quark_i2c_gpio.c       | 64 +++++++++++-------------
 drivers/mfd/mfd-core.c                   | 31 ++++++++++--
 include/linux/mfd/core.h                 |  3 ++
 include/linux/platform_data/gpio-dwapb.h | 23 ---------
 5 files changed, 85 insertions(+), 80 deletions(-)
 delete mode 100644 include/linux/platform_data/gpio-dwapb.h

-- 
2.27.0.rc2

[PATCH v1 1/6] gpio: dwapb: Replace irq_shared flag with fwnode type check

[Andy Shevchenko]

Shared IRQ is only enabled for ACPI platforms or pure platform drivers,
there is no need to have a special flag for that, since we simple can test
port fwnode to be type of OF.

While at it, drop duplicate declaration of loop variable.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Cc: Lee Jones <lee.jones@linaro.org>
---
 drivers/gpio/gpio-dwapb.c                | 5 +----
 drivers/mfd/intel_quark_i2c_gpio.c       | 1 -
 include/linux/platform_data/gpio-dwapb.h | 1 -
 3 files changed, 1 insertion(+), 6 deletions(-)

diff --git a/drivers/gpio/gpio-dwapb.c b/drivers/gpio/gpio-dwapb.c
index 1d8d55bd63aa..a7ca72086511 100644
--- a/drivers/gpio/gpio-dwapb.c
+++ b/drivers/gpio/gpio-dwapb.c
@@ -418,9 +418,7 @@ static void dwapb_configure_irqs(struct dwapb_gpio *gpio,
 	irq_gc->chip_types[1].type = IRQ_TYPE_EDGE_BOTH;
 	irq_gc->chip_types[1].handler = handle_edge_irq;
 
-	if (!pp->irq_shared) {
-		int i;
-
+	if (to_of_node(pp->fwnode)) {
 		for (i = 0; i < pp->ngpio; i++) {
 			if (pp->irq[i])
 				irq_set_chained_handler_and_data(pp->irq[i],
@@ -601,7 +599,6 @@ static struct dwapb_platform_data *dwapb_gpio_get_pdata(struct device *dev)
 			pp->ngpio = 32;
 		}
 
-		pp->irq_shared	= false;
 		pp->gpio_base	= -1;
 
 		/*
diff --git a/drivers/mfd/intel_quark_i2c_gpio.c b/drivers/mfd/intel_quark_i2c_gpio.c
index 84ca7902e1df..06b538dd124c 100644
--- a/drivers/mfd/intel_quark_i2c_gpio.c
+++ b/drivers/mfd/intel_quark_i2c_gpio.c
@@ -216,7 +216,6 @@ static int intel_quark_gpio_setup(struct pci_dev *pdev, struct mfd_cell *cell)
 	pdata->properties->ngpio	= INTEL_QUARK_MFD_NGPIO;
 	pdata->properties->gpio_base	= INTEL_QUARK_MFD_GPIO_BASE;
 	pdata->properties->irq[0]	= pdev->irq;
-	pdata->properties->irq_shared	= true;
 
 	cell->platform_data = pdata;
 	cell->pdata_size = sizeof(*pdata);
diff --git a/include/linux/platform_data/gpio-dwapb.h b/include/linux/platform_data/gpio-dwapb.h
index ff1be737bad6..a63010b6e898 100644
--- a/include/linux/platform_data/gpio-dwapb.h
+++ b/include/linux/platform_data/gpio-dwapb.h
@@ -12,7 +12,6 @@ struct dwapb_port_property {
 	unsigned int	ngpio;
 	unsigned int	gpio_base;
 	int		irq[32];
-	bool		irq_shared;
 };
 
 struct dwapb_platform_data {
-- 
2.27.0.rc2

[PATCH v1 2/6] gpio: dwapb: Read GPIO base from snps,gpio-base property

[Andy Shevchenko]

For backward compatibility with some legacy devices, introduce
a new property snps,gpio-base to read GPIO base. Don't advertise
to discourage users from utilizing it.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/gpio/gpio-dwapb.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/gpio/gpio-dwapb.c b/drivers/gpio/gpio-dwapb.c
index a7ca72086511..e3d7589434eb 100644
--- a/drivers/gpio/gpio-dwapb.c
+++ b/drivers/gpio/gpio-dwapb.c
@@ -599,7 +599,8 @@ static struct dwapb_platform_data *dwapb_gpio_get_pdata(struct device *dev)
 			pp->ngpio = 32;
 		}
 
-		pp->gpio_base	= -1;
+		if (fwnode_property_read_u32(fwnode, "snps,gpio-base", &pp->gpio_base))
+			pp->gpio_base = -1;
 
 		/*
 		 * Only port A can provide interrupts in all configurations of
-- 
2.27.0.rc2

Re: [PATCH v1 2/6] gpio: dwapb: Read GPIO base from snps,gpio-base property

[Linus Walleij]

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

> For backward compatibility with some legacy devices, introduce
> a new property snps,gpio-base to read GPIO base. Don't advertise
> to discourage users from utilizing it.
>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

I usually have a very negative gut reaction to any attempts to hardcode
the heavily deprecated use of the gpio_base for the global GPIO
numberspace. The reason is clear from drivers/gpio/TODO I think.

I need a really good explanation why this is needed, the only reason
people have been pushing this in the past is "oh we are using the
sysfs and we don't wanna change the GPIO numbers in our scripts"
which I really want to push back on now that we have the chardev and
the libgpiod utils.

If this is needed for something driver internal, it should stay in a
driver-local
variable.

Yours,
Linus Walleij

Re: [PATCH v1 2/6] gpio: dwapb: Read GPIO base from snps,gpio-base property

[Andy Shevchenko]

On Wed, Jun 10, 2020 at 01:26:59PM +0200, Linus Walleij wrote:
> On Mon, Jun 8, 2020 at 3:43 PM Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
> 
> > For backward compatibility with some legacy devices, introduce
> > a new property snps,gpio-base to read GPIO base. Don't advertise
> > to discourage users from utilizing it.
> >
> > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> 
> I usually have a very negative gut reaction to any attempts to hardcode
> the heavily deprecated use of the gpio_base for the global GPIO
> numberspace. The reason is clear from drivers/gpio/TODO I think.
> 
> I need a really good explanation why this is needed, the only reason
> people have been pushing this in the past is "oh we are using the
> sysfs and we don't wanna change the GPIO numbers in our scripts"
> which I really want to push back on now that we have the chardev and
> the libgpiod utils.

I understand the point, that's why this property is not being advertised.
The problem here is that on existing (legacy) platform we have to deal with an absolute numbers of GPIOs and those numbers can work if we keep the base

> If this is needed for something driver internal, it should stay in a
> driver-local
> variable.

It's a cross drivers variable. The purpose of this series is to unify interface
in the GPIO driver how to deal with device properties (so, be agnostic from
resource provider at the end).

Everything so far so good, but then we need a mean how not to break the
platform. Because dropping the base in this case will immediately make a
regression.

We can re-use 'gpio-base' as gpio-mockup is using it silently. So, in such case
we won't add any deviation to the property namespace.

Or we can rename above to linux,gpio-base and re-use it here with this name to
explicitly show that this is not firmware property.

-- 
With Best Regards,
Andy Shevchenko

[PATCH v1 3/6] mfd: core: Propagate software node group to the sub devices

[Andy Shevchenko]

From: Heikki Krogerus <heikki.krogerus@linux.intel.com>

When ever device properties are supplied for a sub device, a software node
(fwnode) is actually created and then associated with that device. By allowing
the drivers to supply the complete software node group instead of just the
properties in it, the drivers can take advantage of the other features the
software nodes have on top of supplying the device properties.

Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/mfd/mfd-core.c   | 31 +++++++++++++++++++++++++++----
 include/linux/mfd/core.h |  3 +++
 2 files changed, 30 insertions(+), 4 deletions(-)

diff --git a/drivers/mfd/mfd-core.c b/drivers/mfd/mfd-core.c
index f5a73af60dd4..1a256f64dc9d 100644
--- a/drivers/mfd/mfd-core.c
+++ b/drivers/mfd/mfd-core.c
@@ -173,7 +173,24 @@ static int mfd_add_device(struct device *parent, int id,
 			goto fail_alias;
 	}
 
-	if (cell->properties) {
+	/* If software node group exists, use it, otherwise try properties */
+	if (cell->node_group) {
+		struct fwnode_handle *fwnode;
+
+		ret = software_node_register_node_group(cell->node_group);
+		if (ret)
+			goto fail_alias;
+
+		/*
+		 * The very first software node in the group is related to
+		 * the device itself. The rest can be device-less children to
+		 * fulfill the case of sub nodes (LEDs, GPIO keys, etc).
+		 */
+		fwnode = software_node_fwnode(cell->node_group[0]);
+
+		/* Assign this firmware node as a secondary one of the device */
+		set_secondary_fwnode(&pdev->dev, fwnode);
+	} else if (cell->properties) {
 		ret = platform_device_add_properties(pdev, cell->properties);
 		if (ret)
 			goto fail_alias;
@@ -213,18 +230,18 @@ static int mfd_add_device(struct device *parent, int id,
 			if (has_acpi_companion(&pdev->dev)) {
 				ret = acpi_check_resource_conflict(&res[r]);
 				if (ret)
-					goto fail_alias;
+					goto fail_resources;
 			}
 		}
 	}
 
 	ret = platform_device_add_resources(pdev, res, cell->num_resources);
 	if (ret)
-		goto fail_alias;
+		goto fail_resources;
 
 	ret = platform_device_add(pdev);
 	if (ret)
-		goto fail_alias;
+		goto fail_resources;
 
 	if (cell->pm_runtime_no_callbacks)
 		pm_runtime_no_callbacks(&pdev->dev);
@@ -233,6 +250,9 @@ static int mfd_add_device(struct device *parent, int id,
 
 	return 0;
 
+fail_resources:
+	set_secondary_fwnode(&pdev->dev, NULL);
+	software_node_unregister_node_group(cell->node_group);
 fail_alias:
 	regulator_bulk_unregister_supply_alias(&pdev->dev,
 					       cell->parent_supplies,
@@ -294,6 +314,9 @@ static int mfd_remove_devices_fn(struct device *dev, void *data)
 	pdev = to_platform_device(dev);
 	cell = mfd_get_cell(pdev);
 
+	set_secondary_fwnode(&pdev->dev, NULL);
+	software_node_unregister_node_group(cell->node_group);
+
 	regulator_bulk_unregister_supply_alias(dev, cell->parent_supplies,
 					       cell->num_parent_supplies);
 
diff --git a/include/linux/mfd/core.h b/include/linux/mfd/core.h
index d01d1299e49d..9a998568759f 100644
--- a/include/linux/mfd/core.h
+++ b/include/linux/mfd/core.h
@@ -72,6 +72,9 @@ struct mfd_cell {
 	/* device properties passed to the sub devices drivers */
 	struct property_entry *properties;
 
+	/* Software node group for the sub device */
+	const struct software_node **node_group;
+
 	/*
 	 * Device Tree compatible string
 	 * See: Documentation/devicetree/usage-model.txt Chapter 2.2 for details
-- 
2.27.0.rc2

Re: [PATCH v1 3/6] mfd: core: Propagate software node group to the sub devices

[Lee Jones]

On Mon, 08 Jun 2020, Andy Shevchenko wrote:

> From: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> 
> When ever device properties are supplied for a sub device, a software node
> (fwnode) is actually created and then associated with that device. By allowing
> the drivers to supply the complete software node group instead of just the
> properties in it, the drivers can take advantage of the other features the
> software nodes have on top of supplying the device properties.
> 
> Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>  drivers/mfd/mfd-core.c   | 31 +++++++++++++++++++++++++++----
>  include/linux/mfd/core.h |  3 +++
>  2 files changed, 30 insertions(+), 4 deletions(-)

I'm not sure a change to the API is justified presently (same does go
for 'properties' really, but as it was only a couple of lines, it
didn't seem too intrusive).

My recommendation is to handle this in-house (i.e. locally in-driver)
for now.  When (if) more users adopt the practice, then we should
consider to draw down on line numbers and repetition and make it part
of the API.

-- 
Lee Jones [李琼斯]
Senior Technical Lead - Developer Services
Linaro.org │ Open source software for Arm SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH v1 3/6] mfd: core: Propagate software node group to the sub devices

[Andy Shevchenko]

On Mon, Jun 08, 2020 at 08:25:24PM +0100, Lee Jones wrote:
> On Mon, 08 Jun 2020, Andy Shevchenko wrote:
> 
> > From: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> > 
> > When ever device properties are supplied for a sub device, a software node
> > (fwnode) is actually created and then associated with that device. By allowing
> > the drivers to supply the complete software node group instead of just the
> > properties in it, the drivers can take advantage of the other features the
> > software nodes have on top of supplying the device properties.
> > 
> > Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > ---
> >  drivers/mfd/mfd-core.c   | 31 +++++++++++++++++++++++++++----
> >  include/linux/mfd/core.h |  3 +++
> >  2 files changed, 30 insertions(+), 4 deletions(-)
> 
> I'm not sure a change to the API is justified presently (same does go
> for 'properties' really, but as it was only a couple of lines, it
> didn't seem too intrusive).

This is better and comprehensive API, but I heard you.

> My recommendation is to handle this in-house (i.e. locally in-driver)
> for now.

I think you understand that this is not gonna work (we need to attach fwnode
to the child device before it's registration.

> When (if) more users adopt the practice, then we should
> consider to draw down on line numbers and repetition and make it part
> of the API.

I briefly looked at the current state of affairs and found that properties are
used only for MFD LPSS driver. Would the conversion of that driver to swnodes
work for you?

Note, the long prospective is to get rid of platform_add_properties() API
completely.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 3/6] mfd: core: Propagate software node group to the sub devices

[Lee Jones]

On Tue, 09 Jun 2020, Andy Shevchenko wrote:

> On Mon, Jun 08, 2020 at 08:25:24PM +0100, Lee Jones wrote:
> > On Mon, 08 Jun 2020, Andy Shevchenko wrote:
> > 
> > > From: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> > > 
> > > When ever device properties are supplied for a sub device, a software node
> > > (fwnode) is actually created and then associated with that device. By allowing
> > > the drivers to supply the complete software node group instead of just the
> > > properties in it, the drivers can take advantage of the other features the
> > > software nodes have on top of supplying the device properties.
> > > 
> > > Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> > > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > > ---
> > >  drivers/mfd/mfd-core.c   | 31 +++++++++++++++++++++++++++----
> > >  include/linux/mfd/core.h |  3 +++
> > >  2 files changed, 30 insertions(+), 4 deletions(-)
> > 
> > I'm not sure a change to the API is justified presently (same does go
> > for 'properties' really, but as it was only a couple of lines, it
> > didn't seem too intrusive).
> 
> This is better and comprehensive API, but I heard you.
> 
> > My recommendation is to handle this in-house (i.e. locally in-driver)
> > for now.
> 
> I think you understand that this is not gonna work (we need to attach fwnode
> to the child device before it's registration.
> 
> > When (if) more users adopt the practice, then we should
> > consider to draw down on line numbers and repetition and make it part
> > of the API.
> 
> I briefly looked at the current state of affairs and found that properties are
> used only for MFD LPSS driver. Would the conversion of that driver to swnodes
> work for you?
> 
> Note, the long prospective is to get rid of platform_add_properties() API
> completely.

That's a shame.  Do you plan on replacing it with something else?

MFD tends to only interact with the platform_device API, and even
platform_device_add_properties() doesn't get involved in the nitty
gritty of fwnodes.  Instead it acts as a pass-through straight to
device_add_properties() which is where the magic happens.

For this to be acceptable you would need to add support into
platform_device i.e. platform_device_add_property_group() or some
such.  I really do not want the MFD API to have knowledge about
regarding the particulars i.e. software node registration, secondary
fwnodes and the like.

-- 
Lee Jones [李琼斯]
Senior Technical Lead - Developer Services
Linaro.org │ Open source software for Arm SoCs
Follow Linaro: Facebook | Twitter | Blog

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

[Andy Shevchenko]

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(-)

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;
 }
-- 
2.27.0.rc2

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

[Linus Walleij]

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

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

[Andy Shevchenko]

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 :-)

-- 
With Best Regards,
Andy Shevchenko

[PATCH v1 5/6] gpio: dwapb: Get rid of legacy platform data

[Andy Shevchenko]

Platform data is a legacy interface to supply device properties
to the driver. In this case we don't have anymore in-kernel users
for it. Just remove it for good.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/gpio/gpio-dwapb.c                | 27 +++++++++++++++---------
 include/linux/platform_data/gpio-dwapb.h | 22 -------------------
 2 files changed, 17 insertions(+), 32 deletions(-)
 delete mode 100644 include/linux/platform_data/gpio-dwapb.h

diff --git a/drivers/gpio/gpio-dwapb.c b/drivers/gpio/gpio-dwapb.c
index e3d7589434eb..de01b8943bd2 100644
--- a/drivers/gpio/gpio-dwapb.c
+++ b/drivers/gpio/gpio-dwapb.c
@@ -23,7 +23,6 @@
 #include <linux/property.h>
 #include <linux/reset.h>
 #include <linux/spinlock.h>
-#include <linux/platform_data/gpio-dwapb.h>
 #include <linux/slab.h>
 
 #include "gpiolib.h"
@@ -66,6 +65,19 @@
 
 #define DWAPB_NR_CLOCKS		2
 
+struct dwapb_port_property {
+	struct fwnode_handle *fwnode;
+	unsigned int idx;
+	unsigned int ngpio;
+	unsigned int gpio_base;
+	int irq[32];
+};
+
+struct dwapb_platform_data {
+	struct dwapb_port_property *properties;
+	unsigned int nports;
+};
+
 struct dwapb_gpio;
 
 #ifdef CONFIG_PM_SLEEP
@@ -633,17 +645,12 @@ static int dwapb_gpio_probe(struct platform_device *pdev)
 	unsigned int i;
 	struct dwapb_gpio *gpio;
 	int err;
+	struct dwapb_platform_data *pdata;
 	struct device *dev = &pdev->dev;
-	struct dwapb_platform_data *pdata = dev_get_platdata(dev);
-
-	if (!pdata) {
-		pdata = dwapb_gpio_get_pdata(dev);
-		if (IS_ERR(pdata))
-			return PTR_ERR(pdata);
-	}
 
-	if (!pdata->nports)
-		return -ENODEV;
+	pdata = dwapb_gpio_get_pdata(dev);
+	if (IS_ERR(pdata))
+		return PTR_ERR(pdata);
 
 	gpio = devm_kzalloc(&pdev->dev, sizeof(*gpio), GFP_KERNEL);
 	if (!gpio)
diff --git a/include/linux/platform_data/gpio-dwapb.h b/include/linux/platform_data/gpio-dwapb.h
deleted file mode 100644
index a63010b6e898..000000000000
--- a/include/linux/platform_data/gpio-dwapb.h
+++ /dev/null
@@ -1,22 +0,0 @@
-/* SPDX-License-Identifier: GPL-2.0-only */
-/*
- * Copyright(c) 2014 Intel Corporation.
- */
-
-#ifndef GPIO_DW_APB_H
-#define GPIO_DW_APB_H
-
-struct dwapb_port_property {
-	struct fwnode_handle *fwnode;
-	unsigned int	idx;
-	unsigned int	ngpio;
-	unsigned int	gpio_base;
-	int		irq[32];
-};
-
-struct dwapb_platform_data {
-	struct dwapb_port_property *properties;
-	unsigned int nports;
-};
-
-#endif
-- 
2.27.0.rc2

Re: [PATCH v1 5/6] gpio: dwapb: Get rid of legacy platform data

[Linus Walleij]

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

> Platform data is a legacy interface to supply device properties
> to the driver. In this case we don't have anymore in-kernel users
> for it. Just remove it for good.
>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

I really like where this series takes us so I hope we can still apply all of
this in some form!

Yours,
Linus Walleij

Re: [PATCH v1 5/6] gpio: dwapb: Get rid of legacy platform data

[Andy Shevchenko]

On Wed, Jun 10, 2020 at 01:39:50PM +0200, Linus Walleij wrote:
> On Mon, Jun 8, 2020 at 3:43 PM Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
> 
> > Platform data is a legacy interface to supply device properties
> > to the driver. In this case we don't have anymore in-kernel users
> > for it. Just remove it for good.
> >
> > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> 
> I really like where this series takes us so I hope we can still apply all of
> this in some form!

Thanks! I hope you see the benefit despite some (GPIO base) limitations.

-- 
With Best Regards,
Andy Shevchenko

[PATCH v1 6/6] gpio: dwapb: Define magic number for IRQ and GPIO lines

[Andy Shevchenko]

Define maximum number of IRQ and GPIO lines per port and
replace magic number by it.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/gpio/gpio-dwapb.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/drivers/gpio/gpio-dwapb.c b/drivers/gpio/gpio-dwapb.c
index de01b8943bd2..77a86a8eaee0 100644
--- a/drivers/gpio/gpio-dwapb.c
+++ b/drivers/gpio/gpio-dwapb.c
@@ -63,6 +63,7 @@
 #define GPIO_INTSTATUS_V2	0x3c
 #define GPIO_PORTA_EOI_V2	0x40
 
+#define DWAPB_NR_GPIOS		32
 #define DWAPB_NR_CLOCKS		2
 
 struct dwapb_port_property {
@@ -70,7 +71,7 @@ struct dwapb_port_property {
 	unsigned int idx;
 	unsigned int ngpio;
 	unsigned int gpio_base;
-	int irq[32];
+	int irq[DWAPB_NR_GPIOS];
 };
 
 struct dwapb_platform_data {
@@ -174,7 +175,7 @@ static struct dwapb_gpio_port *dwapb_offs_to_port(struct dwapb_gpio *gpio, unsig
 
 	for (i = 0; i < gpio->nr_ports; i++) {
 		port = &gpio->ports[i];
-		if (port->idx == offs / 32)
+		if (port->idx == offs / DWAPB_NR_GPIOS)
 			return port;
 	}
 
@@ -194,7 +195,7 @@ static void dwapb_toggle_trigger(struct dwapb_gpio *gpio, unsigned int offs)
 
 	pol = dwapb_read(gpio, GPIO_INT_POLARITY);
 	/* Just read the current value right out of the data register */
-	val = gc->get(gc, offs % 32);
+	val = gc->get(gc, offs % DWAPB_NR_GPIOS);
 	if (val)
 		pol &= ~BIT(offs);
 	else
@@ -209,7 +210,7 @@ static u32 dwapb_do_irq(struct dwapb_gpio *gpio)
 	irq_hw_number_t hwirq;
 
 	irq_status = dwapb_read(gpio, GPIO_INTSTATUS);
-	for_each_set_bit(hwirq, &irq_status, 32) {
+	for_each_set_bit(hwirq, &irq_status, DWAPB_NR_GPIOS) {
 		int gpio_irq = irq_find_mapping(gpio->domain, hwirq);
 		u32 irq_type = irq_get_trigger_type(gpio_irq);
 
@@ -608,7 +609,7 @@ static struct dwapb_platform_data *dwapb_gpio_get_pdata(struct device *dev)
 			dev_info(dev,
 				 "failed to get number of gpios for port%d\n",
 				 i);
-			pp->ngpio = 32;
+			pp->ngpio = DWAPB_NR_GPIOS;
 		}
 
 		if (fwnode_property_read_u32(fwnode, "snps,gpio-base", &pp->gpio_base))
-- 
2.27.0.rc2

Re: [PATCH v1 0/6] mfd: Make use of software nodes

[Serge Semin]

On Mon, Jun 08, 2020 at 04:42:54PM +0300, Andy Shevchenko wrote:
> Some devices would need to have a hierarchy of properties and
> child nodes passed to the child or children of MFD. For such case
> we may utilize software nodes, which is superior on device properties.
> 
> Add support of software nodes to MFD core and convert one driver
> to show how it looks like. This allows to get rid of legacy platform
> data.
> 
> The change has been tested on Intel Galileo Gen 2.

I am wondering whether we could move the {gpio_base, ngpio, irq_shared}
part into the gpio-dwapb.c driver and use either the ACPI-based or
platform_device_id-based matching to get the device-specific resources
info through the driver_data field. By doing so you wouldn't need to
introduce a new "snps,gpio-base"-like property and propagate
software_node-based properties, but still you could get rid of the
dwapb_platform_data structure since all the info would be locally
available.

If ACPI-based matching doesn't uniquely address the Quark GPIO node,
then you could just replace the intel_quark_mfd_cells[0].name with
something like "gpio-dwapb-quark", which then by the MFD core will be
copied to the corresponding platform_device->name due to calling
platform_device_alloc() with cell-name passed. That name will be used
to match a platform_driver with id_table having that new name added.

-Sergey

> 
> Andy Shevchenko (5):
>   gpio: dwapb: Replace irq_shared flag with fwnode type check
>   gpio: dwapb: Read GPIO base from snps,gpio-base property
>   mfd: intel_quark_i2c_gpio: Convert to use software nodes
>   gpio: dwapb: Get rid of legacy platform data
>   gpio: dwapb: Define magic number for IRQ and GPIO lines
> 
> Heikki Krogerus (1):
>   mfd: core: Propagate software node group to the sub devices
> 
>  drivers/gpio/gpio-dwapb.c                | 44 +++++++++-------
>  drivers/mfd/intel_quark_i2c_gpio.c       | 64 +++++++++++-------------
>  drivers/mfd/mfd-core.c                   | 31 ++++++++++--
>  include/linux/mfd/core.h                 |  3 ++
>  include/linux/platform_data/gpio-dwapb.h | 23 ---------
>  5 files changed, 85 insertions(+), 80 deletions(-)
>  delete mode 100644 include/linux/platform_data/gpio-dwapb.h
> 
> -- 
> 2.27.0.rc2
> 

Re: [PATCH v1 0/6] mfd: Make use of software nodes

[Andy Shevchenko]

On Tue, Jun 16, 2020 at 11:03 PM Serge Semin <fancer.lancer@gmail.com> wrote:
> On Mon, Jun 08, 2020 at 04:42:54PM +0300, Andy Shevchenko wrote:
> > Some devices would need to have a hierarchy of properties and
> > child nodes passed to the child or children of MFD. For such case
> > we may utilize software nodes, which is superior on device properties.
> >
> > Add support of software nodes to MFD core and convert one driver
> > to show how it looks like. This allows to get rid of legacy platform
> > data.
> >
> > The change has been tested on Intel Galileo Gen 2.
>
> I am wondering whether we could move the {gpio_base, ngpio, irq_shared}
> part into the gpio-dwapb.c driver and use either the ACPI-based or
> platform_device_id-based matching to get the device-specific resources
> info through the driver_data field. By doing so you wouldn't need to
> introduce a new "snps,gpio-base"-like property and propagate
> software_node-based properties, but still you could get rid of the
> dwapb_platform_data structure since all the info would be locally
> available.

The idea is to get rid of the driver being dependent on some quirks
when we may do it clearly and nicely.
We, by applying this series, make (keep) layers independent: board
code vs. driver code. Mixing them more is the opposite to what I
propose.

WRT property.
snps,gpio-base can be easily changed to *already in use* gpio-base or
being both converted to linux,gpio-base to explicitly show people that
this is internal stuff that must not be present in firmware nodes.

> If ACPI-based matching doesn't uniquely address the Quark GPIO node,
> then you could just replace the intel_quark_mfd_cells[0].name with
> something like "gpio-dwapb-quark", which then by the MFD core will be
> copied to the corresponding platform_device->name due to calling
> platform_device_alloc() with cell-name passed. That name will be used
> to match a platform_driver with id_table having that new name added.

Oh, that doesn't sound right. It makes things ugly.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 0/6] mfd: Make use of software nodes

[Serge Semin]

On Wed, Jun 17, 2020 at 12:40:35AM +0300, Andy Shevchenko wrote:
> On Tue, Jun 16, 2020 at 11:03 PM Serge Semin <fancer.lancer@gmail.com> wrote:
> > On Mon, Jun 08, 2020 at 04:42:54PM +0300, Andy Shevchenko wrote:
> > > Some devices would need to have a hierarchy of properties and
> > > child nodes passed to the child or children of MFD. For such case
> > > we may utilize software nodes, which is superior on device properties.
> > >
> > > Add support of software nodes to MFD core and convert one driver
> > > to show how it looks like. This allows to get rid of legacy platform
> > > data.
> > >
> > > The change has been tested on Intel Galileo Gen 2.
> >
> > I am wondering whether we could move the {gpio_base, ngpio, irq_shared}
> > part into the gpio-dwapb.c driver and use either the ACPI-based or
> > platform_device_id-based matching to get the device-specific resources
> > info through the driver_data field. By doing so you wouldn't need to
> > introduce a new "snps,gpio-base"-like property and propagate
> > software_node-based properties, but still you could get rid of the
> > dwapb_platform_data structure since all the info would be locally
> > available.
> 
> The idea is to get rid of the driver being dependent on some quirks
> when we may do it clearly and nicely.

Yes, I've got that and in most of the aspects I like what you suggested
in this parchset. But it seems to me that the maintainers are mostly prone
to having some of the platform-specifics being locally (in-driver) defined.
So I proposed an alternative solution, which might do to satisfy their
requirement. Note saying that you want to get rid of the quirks and
introducing something like "gpio-base" firmware property seems contradicting
a bit. 

> We, by applying this series, make (keep) layers independent: board
> code vs. driver code. Mixing them more is the opposite to what I
> propose.
> 
> WRT property.
> snps,gpio-base can be easily changed to *already in use* gpio-base or
> being both converted to linux,gpio-base to explicitly show people that
> this is internal stuff that must not be present in firmware nodes.
> 

As I see it the part with "gpio-base" and the irq_shared can be moved to the
gpio-dwapb.c driver to be defined as the quark-specific quirks. Adding a
property like "gpio-base" seems like a quirk anyway, so I'd leave it defined in
the driver.

* Note I don't really like replacing the irq_shared flag with to_of_node()
because in general to_of_node() doesn't mean the IRQ line is shared, so
selecting the shared and non-shared interrupt request paths based on that macro
seems hackish.

> > If ACPI-based matching doesn't uniquely address the Quark GPIO node,
> > then you could just replace the intel_quark_mfd_cells[0].name with
> > something like "gpio-dwapb-quark", which then by the MFD core will be
> > copied to the corresponding platform_device->name due to calling
> > platform_device_alloc() with cell-name passed. That name will be used
> > to match a platform_driver with id_table having that new name added.
> 
> Oh, that doesn't sound right. It makes things ugly.

I may have said that a bit unclearly. The only thing you'd need to do is to
add an unique name to the Quark GPIO cell, like:
drivers/mfd/intel_quark_i2c_gpio.c:
static struct mfd_cell intel_quark_mfd_cells[] = {
        {
                .name = "gpio-dwapb-quark",
        }

Then make the gpio-dwapb.c driver being compatible with that device by declaring
the id_table with that device name and passing the table to the DW APB GPIO
"struct platform_driver" descriptor. The MFD/platform cores already provide the
functionality of matching those two device and driver. If ACPI node uniquely
defines the Quark GPIO with all that quirks applicable then you wouldn't even
need to do the platform_device-driver-based matching. Just use the acpi_device_id
to get the quirks flags/descriptors. 

The rest of the patchset concerning passing software_node's down to the
individual MFD sub-device and declaring "reg" and "snps,nr-gpios" in there seems
pretty much acceptable to me.

* Though indeed it would be better to mark the "snps,nr-gpios" as deprecated in
the DT schema and have the "ngpios" supported as well.

-Sergey

> 
> -- 
> With Best Regards,
> Andy Shevchenko

Re: [PATCH v1 0/6] mfd: Make use of software nodes

[Andy Shevchenko]

On Wed, Jun 17, 2020 at 01:56:48AM +0300, Serge Semin wrote:
> On Wed, Jun 17, 2020 at 12:40:35AM +0300, Andy Shevchenko wrote:
> > On Tue, Jun 16, 2020 at 11:03 PM Serge Semin <fancer.lancer@gmail.com> wrote:
> > > On Mon, Jun 08, 2020 at 04:42:54PM +0300, Andy Shevchenko wrote:
> > > > Some devices would need to have a hierarchy of properties and
> > > > child nodes passed to the child or children of MFD. For such case
> > > > we may utilize software nodes, which is superior on device properties.
> > > >
> > > > Add support of software nodes to MFD core and convert one driver
> > > > to show how it looks like. This allows to get rid of legacy platform
> > > > data.
> > > >
> > > > The change has been tested on Intel Galileo Gen 2.
> > >
> > > I am wondering whether we could move the {gpio_base, ngpio, irq_shared}
> > > part into the gpio-dwapb.c driver and use either the ACPI-based or
> > > platform_device_id-based matching to get the device-specific resources
> > > info through the driver_data field. By doing so you wouldn't need to
> > > introduce a new "snps,gpio-base"-like property and propagate
> > > software_node-based properties, but still you could get rid of the
> > > dwapb_platform_data structure since all the info would be locally
> > > available.
> > 
> > The idea is to get rid of the driver being dependent on some quirks
> > when we may do it clearly and nicely.
> 
> Yes, I've got that and in most of the aspects I like what you suggested
> in this parchset. But it seems to me that the maintainers are mostly prone
> to having some of the platform-specifics being locally (in-driver) defined.

You are a maintainer of the dwapb-gpio. Is it what you insist?

> So I proposed an alternative solution, which might do to satisfy their
> requirement.

I'm puzzled whom about you are talking.

> Note saying that you want to get rid of the quirks and
> introducing something like "gpio-base" firmware property seems contradicting
> a bit. 

Maybe I need to elaborate that under quirks I meant quirk-clean GPIO driver,
so, it wouldn't care about what platform(s) require base and what do not.

> > We, by applying this series, make (keep) layers independent: board
> > code vs. driver code. Mixing them more is the opposite to what I
> > propose.
> > 
> > WRT property.
> > snps,gpio-base can be easily changed to *already in use* gpio-base or
> > being both converted to linux,gpio-base to explicitly show people that
> > this is internal stuff that must not be present in firmware nodes.
> 
> As I see it the part with "gpio-base" and the irq_shared can be moved to the
> gpio-dwapb.c driver to be defined as the quark-specific quirks. Adding a
> property like "gpio-base" seems like a quirk anyway, so I'd leave it defined in
> the driver.

Huh?! The whole idea is make GPIO driver agnostic from platforms and their quirks.

> * Note I don't really like replacing the irq_shared flag with to_of_node()
> because in general to_of_node() doesn't mean the IRQ line is shared, so
> selecting the shared and non-shared interrupt request paths based on that macro
> seems hackish.

This I can understand, but can you propose better alternative?

> > > If ACPI-based matching doesn't uniquely address the Quark GPIO node,
> > > then you could just replace the intel_quark_mfd_cells[0].name with
> > > something like "gpio-dwapb-quark", which then by the MFD core will be
> > > copied to the corresponding platform_device->name due to calling
> > > platform_device_alloc() with cell-name passed. That name will be used
> > > to match a platform_driver with id_table having that new name added.
> > 
> > Oh, that doesn't sound right. It makes things ugly.
> 
> I may have said that a bit unclearly. The only thing you'd need to do is to
> add an unique name to the Quark GPIO cell, like:
> drivers/mfd/intel_quark_i2c_gpio.c:
> static struct mfd_cell intel_quark_mfd_cells[] = {
>         {
>                 .name = "gpio-dwapb-quark",
>         }
> 
> Then make the gpio-dwapb.c driver being compatible with that device by declaring
> the id_table with that device name and passing the table to the DW APB GPIO
> "struct platform_driver" descriptor. The MFD/platform cores already provide the
> functionality of matching those two device and driver. If ACPI node uniquely
> defines the Quark GPIO with all that quirks applicable then you wouldn't even
> need to do the platform_device-driver-based matching. Just use the acpi_device_id
> to get the quirks flags/descriptors. 

What you are talking about? Can you provide a code we can discuss?

> * Though indeed it would be better to mark the "snps,nr-gpios" as deprecated in
> the DT schema and have the "ngpios" supported as well.

Actually I used to have that piece in my first patches, but decided to remove
due to this property being (semi-)internal. I would like to hear GPIO
maintainers about this.

Bart, Linus?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 0/6] mfd: Make use of software nodes

[Serge Semin]

On Thu, Jun 18, 2020 at 11:56:54AM +0300, Andy Shevchenko wrote:
> On Wed, Jun 17, 2020 at 01:56:48AM +0300, Serge Semin wrote:
> > On Wed, Jun 17, 2020 at 12:40:35AM +0300, Andy Shevchenko wrote:
> > > On Tue, Jun 16, 2020 at 11:03 PM Serge Semin <fancer.lancer@gmail.com> wrote:
> > > > On Mon, Jun 08, 2020 at 04:42:54PM +0300, Andy Shevchenko wrote:
> > > > > Some devices would need to have a hierarchy of properties and
> > > > > child nodes passed to the child or children of MFD. For such case
> > > > > we may utilize software nodes, which is superior on device properties.
> > > > >
> > > > > Add support of software nodes to MFD core and convert one driver
> > > > > to show how it looks like. This allows to get rid of legacy platform
> > > > > data.
> > > > >
> > > > > The change has been tested on Intel Galileo Gen 2.
> > > >
> > > > I am wondering whether we could move the {gpio_base, ngpio, irq_shared}
> > > > part into the gpio-dwapb.c driver and use either the ACPI-based or
> > > > platform_device_id-based matching to get the device-specific resources
> > > > info through the driver_data field. By doing so you wouldn't need to
> > > > introduce a new "snps,gpio-base"-like property and propagate
> > > > software_node-based properties, but still you could get rid of the
> > > > dwapb_platform_data structure since all the info would be locally
> > > > available.
> > > 
> > > The idea is to get rid of the driver being dependent on some quirks
> > > when we may do it clearly and nicely.
> > 

> > Yes, I've got that and in most of the aspects I like what you suggested
> > in this parchset. But it seems to me that the maintainers are mostly prone
> > to having some of the platform-specifics being locally (in-driver) defined.
> 
> You are a maintainer of the dwapb-gpio. Is it what you insist?

That was a discussable suggestion of how the patches could be improved to
still have the software-node functionality and to get rid of the legacy
platform data passing around.

> 
> > So I proposed an alternative solution, which might do to satisfy their
> > requirement.
> 
> I'm puzzled whom about you are talking.

I don't have a habit to say of myself like that) So obviously I was talking
about Linus and Lee in the previous sentence mostly referring to the Linus'
words regarding setting the GPIO base and having the "gpio-base" property
at all.

> 
> > Note saying that you want to get rid of the quirks and
> > introducing something like "gpio-base" firmware property seems contradicting
> > a bit. 
> 
> Maybe I need to elaborate that under quirks I meant quirk-clean GPIO driver,
> so, it wouldn't care about what platform(s) require base and what do not.

My point is that no mater how you pass the gpio-base value it will always be a
quirk. The same thing is with the irq_shared flag. Both of them are specific to
the Quark-platform. They are used to tune the DW APB GPIO driver up so one would
create a GPIO device working well with the Quark-specific DW APB GPIO block.

> 
> > > We, by applying this series, make (keep) layers independent: board
> > > code vs. driver code. Mixing them more is the opposite to what I
> > > propose.
> > > 
> > > WRT property.
> > > snps,gpio-base can be easily changed to *already in use* gpio-base or
> > > being both converted to linux,gpio-base to explicitly show people that
> > > this is internal stuff that must not be present in firmware nodes.
> > 
> > As I see it the part with "gpio-base" and the irq_shared can be moved to the
> > gpio-dwapb.c driver to be defined as the quark-specific quirks. Adding a
> > property like "gpio-base" seems like a quirk anyway, so I'd leave it defined in
> > the driver.
> 

> Huh?! The whole idea is make GPIO driver agnostic from platforms and their quirks.

As I said above having the gpio-base value set is the platform-specific thing in
any case. So no mater whether you pass it via the software_node/properties or
the private platform-data structure, the DW APB GPIO driver will have to handle
those parameters in some special way, which is quirk-prone since normal platforms
don't have such peculiar requirements.

> 
> > * Note I don't really like replacing the irq_shared flag with to_of_node()
> > because in general to_of_node() doesn't mean the IRQ line is shared, so
> > selecting the shared and non-shared interrupt request paths based on that macro
> > seems hackish.
> 
> This I can understand, but can you propose better alternative?

Alas I can't at the moment. But see my comment below.

> 
> > > > If ACPI-based matching doesn't uniquely address the Quark GPIO node,
> > > > then you could just replace the intel_quark_mfd_cells[0].name with
> > > > something like "gpio-dwapb-quark", which then by the MFD core will be
> > > > copied to the corresponding platform_device->name due to calling
> > > > platform_device_alloc() with cell-name passed. That name will be used
> > > > to match a platform_driver with id_table having that new name added.
> > > 
> > > Oh, that doesn't sound right. It makes things ugly.
> > 
> > I may have said that a bit unclearly. The only thing you'd need to do is to
> > add an unique name to the Quark GPIO cell, like:
> > drivers/mfd/intel_quark_i2c_gpio.c:
> > static struct mfd_cell intel_quark_mfd_cells[] = {
> >         {
> >                 .name = "gpio-dwapb-quark",
> >         }
> > 
> > Then make the gpio-dwapb.c driver being compatible with that device by declaring
> > the id_table with that device name and passing the table to the DW APB GPIO
> > "struct platform_driver" descriptor. The MFD/platform cores already provide the
> > functionality of matching those two device and driver. If ACPI node uniquely
> > defines the Quark GPIO with all that quirks applicable then you wouldn't even
> > need to do the platform_device-driver-based matching. Just use the acpi_device_id
> > to get the quirks flags/descriptors. 
> 
> What you are talking about? Can you provide a code we can discuss?

Here is what I suggest in step-by-step:
Patch 1) Move the Quark-specific GPIO-base and irq_shared parameters definition
         to the gpio-dwapb.c driver.
Patch 2) Retrieve the GPIO-base and IRQ-shared parameters only for the DW APB
         GPIO block living in the Quark platform (by means of either ACPI or
         the platform-device ID data).
Patch 3) Introduce the software_node propagation functionality (AFAIU Lee
         wants it to be a part of the platform-device core instead of being
         created on the MFD level).
Patch 4) Convert the intel_quark_i2c_gpio driver to using the software nodes
         instead of creating the dwapb_platform_data instance (nearly matches
         your patch 4 except it won't introduce any "gpio-base"-like property).
Patch 5) Get rid of the global dwapb_platform_data utilization (matches your
         patch 5).
Patch 6) Introduce the IRQ and GPIO lines macro (matches your patch 6).

By doing as I suggested you don't have to pass around the GPIO-base value and
the IRQ-shared flag. They will be initialized and utilized locally in the
DW APB GPIO driver if Quark-specific DW APB GPIO block is detected (patches 1
and 2). In that case a software-node created in Patch 3 and 4 will be a normal
firmware node described by the DW bindings. You'll still be able to get rid of
the legacy global platform-data in the Patch 5.

The spirit of your original patchset will still be preserved: introduce the
software-nodes propagation interface and use it to pass the generic DW APB GPIO
parameters. The only difference is that we'd move the GPIO-base and IRQ-shared
functionality fully into the DW APB GPIO driver. By doing so we'd have:
1) DW APB GPIO Quark-specifics localized in a single gpio-dwapb driver.
   In the current implementation we have them distributed between two drivers:
   intel_quark_i2c_gpio initializes the GPIO-base and IRQ-shared parameters,
   gpio-dwapb uses them for setup. Note there is no any other platform with DW
   APB GPIO which needs the GPIO-base and IRQ-shared flag being setup. Why do
   we need to have such complicated interface then if we can identify whether
   the particular DW APB GPIO is a Quark-specific block or not? Moreover having
   those specifics in both drivers in fact means having quirks in both of them.
2) Quirk-clean intel_quark_i2c_gpio driver which creates a normal DW APB GPIO
   device with software-nodes fully compatible with DT binding. DW APB GPIO
   driver will detect whether the block is Quark-specific and will activate
   the corresponding quirks if it is.
3) Still legacy global platform-data removed.

-Sergey

> 
> > * Though indeed it would be better to mark the "snps,nr-gpios" as deprecated in
> > the DT schema and have the "ngpios" supported as well.
> 
> Actually I used to have that piece in my first patches, but decided to remove
> due to this property being (semi-)internal. I would like to hear GPIO
> maintainers about this.
> 
> Bart, Linus?
> 
> -- 
> With Best Regards,
> Andy Shevchenko
> 
> 

Re: [PATCH v1 0/6] mfd: Make use of software nodes

[Andy Shevchenko]

On Sat, Jun 20, 2020 at 1:12 AM Serge Semin <fancer.lancer@gmail.com> wrote:
> On Thu, Jun 18, 2020 at 11:56:54AM +0300, Andy Shevchenko wrote:
> > On Wed, Jun 17, 2020 at 01:56:48AM +0300, Serge Semin wrote:
> > > On Wed, Jun 17, 2020 at 12:40:35AM +0300, Andy Shevchenko wrote:
> > > > On Tue, Jun 16, 2020 at 11:03 PM Serge Semin <fancer.lancer@gmail.com> wrote:
> > > > > On Mon, Jun 08, 2020 at 04:42:54PM +0300, Andy Shevchenko wrote:

...

> > > > > I am wondering whether we could move the {gpio_base, ngpio, irq_shared}
> > > > > part into the gpio-dwapb.c driver and use either the ACPI-based or
> > > > > platform_device_id-based matching to get the device-specific resources
> > > > > info through the driver_data field. By doing so you wouldn't need to
> > > > > introduce a new "snps,gpio-base"-like property and propagate
> > > > > software_node-based properties, but still you could get rid of the
> > > > > dwapb_platform_data structure since all the info would be locally
> > > > > available.
> > > >
> > > > The idea is to get rid of the driver being dependent on some quirks
> > > > when we may do it clearly and nicely.
> > >
>
> > > Yes, I've got that and in most of the aspects I like what you suggested
> > > in this parchset. But it seems to me that the maintainers are mostly prone
> > > to having some of the platform-specifics being locally (in-driver) defined.
> >
> > You are a maintainer of the dwapb-gpio. Is it what you insist?
>
> That was a discussable suggestion of how the patches could be improved to
> still have the software-node functionality and to get rid of the legacy
> platform data passing around.

> > > So I proposed an alternative solution, which might do to satisfy their
> > > requirement.
> >
> > I'm puzzled whom about you are talking.
>
> I don't have a habit to say of myself like that) So obviously I was talking
> about Linus and Lee in the previous sentence mostly referring to the Linus'
> words regarding setting the GPIO base and having the "gpio-base" property
> at all.

Linus replied to three messages out of 7, seems you read only the first one.
That's why I have been puzzled.

> > > Note saying that you want to get rid of the quirks and
> > > introducing something like "gpio-base" firmware property seems contradicting
> > > a bit.
> >
> > Maybe I need to elaborate that under quirks I meant quirk-clean GPIO driver,
> > so, it wouldn't care about what platform(s) require base and what do not.
>
> My point is that no mater how you pass the gpio-base value it will always be a
> quirk. The same thing is with the irq_shared flag. Both of them are specific to
> the Quark-platform. They are used to tune the DW APB GPIO driver up so one would
> create a GPIO device working well with the Quark-specific DW APB GPIO block.

Precisely!

> > > > We, by applying this series, make (keep) layers independent: board
> > > > code vs. driver code. Mixing them more is the opposite to what I
> > > > propose.
> > > >
> > > > WRT property.
> > > > snps,gpio-base can be easily changed to *already in use* gpio-base or
> > > > being both converted to linux,gpio-base to explicitly show people that
> > > > this is internal stuff that must not be present in firmware nodes.
> > >
> > > As I see it the part with "gpio-base" and the irq_shared can be moved to the
> > > gpio-dwapb.c driver to be defined as the quark-specific quirks. Adding a
> > > property like "gpio-base" seems like a quirk anyway, so I'd leave it defined in
> > > the driver.
> >
>
> > Huh?! The whole idea is make GPIO driver agnostic from platforms and their quirks.
>
> As I said above having the gpio-base value set is the platform-specific thing in
> any case. So no mater whether you pass it via the software_node/properties or
> the private platform-data structure, the DW APB GPIO driver will have to handle
> those parameters in some special way, which is quirk-prone since normal platforms
> don't have such peculiar requirements.

Seems you are proposing layering violation without seeing it.
Let me explain the design of the drivers in Linux kernel.

There are basically few possible concepts how drivers are handling
quirks (because we know that most of the hardware support is a set of
quirks here and there):
1) platform provides a board files and via platform data structures
supplies quirks to the certain driver
2) platform provides a firmware node (ACPI, DT, ...) and unified
driver handles it thru the fwnode interface
3) driver is split to be a library (or core part) + glue drivers full of quirks
4) driver has embedded quirks and supplies them based on IDs (PCI ID,
ACPI ID, compatible string, ID table, etc)
5) ...missed something? ...

What I'm proposing is turn 1 to 2 for Quark case, what you are
proposing is absent on the picture, i.e.:
x) platform provides a board file (intel_quark_i2c_gpio.c) and the
driver has embedded quirks based on ID.

This is not what we want, definitely.

...

> > What you are talking about? Can you provide a code we can discuss?
>
> Here is what I suggest in step-by-step:
> Patch 1) Move the Quark-specific GPIO-base and irq_shared parameters definition
>          to the gpio-dwapb.c driver.

Layering violation: spreading board code over the kernel.

> Patch 2) Retrieve the GPIO-base and IRQ-shared parameters only for the DW APB
>          GPIO block living in the Quark platform (by means of either ACPI or
>          the platform-device ID data).

Same. (The rest doesn't matter. I dropped it because it's basically
the same what I proposed)

> By doing as I suggested you don't have to pass around the GPIO-base value and
> the IRQ-shared flag. They will be initialized and utilized locally in the
> DW APB GPIO driver if Quark-specific DW APB GPIO block is detected (patches 1
> and 2). In that case a software-node created in Patch 3 and 4 will be a normal
> firmware node described by the DW bindings. You'll still be able to get rid of
> the legacy global platform-data in the Patch 5.
>
> The spirit of your original patchset will still be preserved: introduce the
> software-nodes propagation interface and use it to pass the generic DW APB GPIO
> parameters. The only difference is that we'd move the GPIO-base and IRQ-shared
> functionality fully into the DW APB GPIO driver. By doing so we'd have:
> 1) DW APB GPIO Quark-specifics localized in a single gpio-dwapb driver.
>    In the current implementation we have them distributed between two drivers:
>    intel_quark_i2c_gpio initializes the GPIO-base and IRQ-shared parameters,
>    gpio-dwapb uses them for setup. Note there is no any other platform with DW
>    APB GPIO which needs the GPIO-base and IRQ-shared flag being setup. Why do
>    we need to have such complicated interface then if we can identify whether
>    the particular DW APB GPIO is a Quark-specific block or not? Moreover having
>    those specifics in both drivers in fact means having quirks in both of them.
> 2) Quirk-clean intel_quark_i2c_gpio driver which creates a normal DW APB GPIO
>    device with software-nodes fully compatible with DT binding. DW APB GPIO
>    driver will detect whether the block is Quark-specific and will activate
>    the corresponding quirks if it is.
> 3) Still legacy global platform-data removed.

I see. But you mistakenly made a conclusion 1). It will be spread to
two drivers.
And in 2) you considered (wrongly) that MFD driver is not a board code.

Again MFD driver _is_ a board code or i.o.w. quirk driver for specific
Quark (MFD) device.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 0/6] mfd: Make use of software nodes

[Serge Semin]

On Sat, Jun 20, 2020 at 01:13:56PM +0300, Andy Shevchenko wrote:
> On Sat, Jun 20, 2020 at 1:12 AM Serge Semin <fancer.lancer@gmail.com> wrote:
> > On Thu, Jun 18, 2020 at 11:56:54AM +0300, Andy Shevchenko wrote:
> > > On Wed, Jun 17, 2020 at 01:56:48AM +0300, Serge Semin wrote:
> > > > On Wed, Jun 17, 2020 at 12:40:35AM +0300, Andy Shevchenko wrote:
> > > > > On Tue, Jun 16, 2020 at 11:03 PM Serge Semin <fancer.lancer@gmail.com> wrote:
> > > > > > On Mon, Jun 08, 2020 at 04:42:54PM +0300, Andy Shevchenko wrote:
> 
> ...
> 
> > > > > > I am wondering whether we could move the {gpio_base, ngpio, irq_shared}
> > > > > > part into the gpio-dwapb.c driver and use either the ACPI-based or
> > > > > > platform_device_id-based matching to get the device-specific resources
> > > > > > info through the driver_data field. By doing so you wouldn't need to
> > > > > > introduce a new "snps,gpio-base"-like property and propagate
> > > > > > software_node-based properties, but still you could get rid of the
> > > > > > dwapb_platform_data structure since all the info would be locally
> > > > > > available.
> > > > >
> > > > > The idea is to get rid of the driver being dependent on some quirks
> > > > > when we may do it clearly and nicely.
> > > >
> >
> > > > Yes, I've got that and in most of the aspects I like what you suggested
> > > > in this parchset. But it seems to me that the maintainers are mostly prone
> > > > to having some of the platform-specifics being locally (in-driver) defined.
> > >
> > > You are a maintainer of the dwapb-gpio. Is it what you insist?
> >
> > That was a discussable suggestion of how the patches could be improved to
> > still have the software-node functionality and to get rid of the legacy
> > platform data passing around.
> 
> > > > So I proposed an alternative solution, which might do to satisfy their
> > > > requirement.
> > >
> > > I'm puzzled whom about you are talking.
> >
> > I don't have a habit to say of myself like that) So obviously I was talking
> > about Linus and Lee in the previous sentence mostly referring to the Linus'
> > words regarding setting the GPIO base and having the "gpio-base" property
> > at all.
> 
> Linus replied to three messages out of 7, seems you read only the first one.
> That's why I have been puzzled.

Actually I've read all of them and from what I could see, he didn't like a code
setting gpio-base regardless whether it has been created before your patchset
or your explanation why it was necessary.

> 
> > > > Note saying that you want to get rid of the quirks and
> > > > introducing something like "gpio-base" firmware property seems contradicting
> > > > a bit.
> > >
> > > Maybe I need to elaborate that under quirks I meant quirk-clean GPIO driver,
> > > so, it wouldn't care about what platform(s) require base and what do not.
> >
> > My point is that no mater how you pass the gpio-base value it will always be a
> > quirk. The same thing is with the irq_shared flag. Both of them are specific to
> > the Quark-platform. They are used to tune the DW APB GPIO driver up so one would
> > create a GPIO device working well with the Quark-specific DW APB GPIO block.
> 
> Precisely!
> 
> > > > > We, by applying this series, make (keep) layers independent: board
> > > > > code vs. driver code. Mixing them more is the opposite to what I
> > > > > propose.
> > > > >
> > > > > WRT property.
> > > > > snps,gpio-base can be easily changed to *already in use* gpio-base or
> > > > > being both converted to linux,gpio-base to explicitly show people that
> > > > > this is internal stuff that must not be present in firmware nodes.
> > > >
> > > > As I see it the part with "gpio-base" and the irq_shared can be moved to the
> > > > gpio-dwapb.c driver to be defined as the quark-specific quirks. Adding a
> > > > property like "gpio-base" seems like a quirk anyway, so I'd leave it defined in
> > > > the driver.
> > >
> >
> > > Huh?! The whole idea is make GPIO driver agnostic from platforms and their quirks.
> >
> > As I said above having the gpio-base value set is the platform-specific thing in
> > any case. So no mater whether you pass it via the software_node/properties or
> > the private platform-data structure, the DW APB GPIO driver will have to handle
> > those parameters in some special way, which is quirk-prone since normal platforms
> > don't have such peculiar requirements.
> 

> Seems you are proposing layering violation without seeing it.
> Let me explain the design of the drivers in Linux kernel.
> 
> There are basically few possible concepts how drivers are handling
> quirks (because we know that most of the hardware support is a set of
> quirks here and there):
> 1) platform provides a board files and via platform data structures
> supplies quirks to the certain driver
> 2) platform provides a firmware node (ACPI, DT, ...) and unified
> driver handles it thru the fwnode interface
> 3) driver is split to be a library (or core part) + glue drivers full of quirks
> 4) driver has embedded quirks and supplies them based on IDs (PCI ID,
> ACPI ID, compatible string, ID table, etc)
> 5) ...missed something? ...
> 
> What I'm proposing is turn 1 to 2 for Quark case, what you are
> proposing is absent on the picture, i.e.:
> x) platform provides a board file (intel_quark_i2c_gpio.c) and the
> driver has embedded quirks based on ID.
> 
> This is not what we want, definitely.

From the list you provided it's still not obvious why what I suggested wasn't
good, because it's perfectly fine to have both ACPI/DT-based firmware node
(entry 2) and quirks based on IDs (entry 4), which plenty of the kernel
drivers do. The difference between the most of those drivers and what I
suggested is that by bonding a device with a driver they provide
!device!-quirks, but not the !platform!-quirks. While the platform quirks
and platform properties are normally provided by means of the platform data,
firmware nodes, glue layers, etc. Moving some of the platform-quirks to a
driver you called "layering violation". Well, I've never met that definition
in the kernel before (have you just come up with it?), but at least I see
what you meant.

Anyway there are GPIO-drivers which still use the device IDs to get the
platform quirks or get the GPIO-base from an of node alias (gpio-zynq.c,
gpio-vf610.c, gpio-zx.c, gpio-mxc.c, gpio-mxs.c). There are even some,
which either use a static variable to redistribute the GPIO-base between
all available GPIO-chips (gpio-brcmstb.c, gpio-sta2x11.c, gpio-omap.c)
or set a fixed GPIO-based value (like gpio-xlp.c, gpio-iop.c, gpio-ep93xx.c,
gpio-vx855.c, gpio-cs5535.c, gpio-sch311x.c, gpio-loongson.c, gpio-loongson1.c,
gpio-ath79.c, gpio-octeon.c), or even get the GPIO-base value from some
hardware register (gpio-merrifield.c, gpio-intel-mid.c). I am pretty sure
the examples of having the locally-defined platform quirks and the concepts
1, 2, 3 and 4 at some extent utilized in a single driver can be found in another
subsystems too.

I am not saying, that the approaches utilized in those drivers are ideal.
Those are just examples, that the platform specifics can be reflected in
the corresponding drivers and the so called "layering violation" is allowed
at some circumstances. Linus, correct me if I am wrong.

Getting back to this patchset. As I see it, the main problem here is connected
with two parameters:
- GPIO-base. You suggest to update the gpio-dwapb.c driver so one would
  support a firmware property like "gpio-base". It's not good, since the
  property will be implicitly supported by OF API as well and nothing will
  prevent a user from using it. Even though you said that we won't advertise
  that property, some user may try to define it in dts anyway, which can be
  easily missed on review.
- IRQ-shared. As I said before it's not good to replace the irq_shared flag
  with the to_of_node() macro. Because having to_of_node() returned an
  of-node doesn't mean the IRQs can't be shared.

As I see it the convenience provided by your patchset in relation to the
GPIO-base and IRQ-shared properties doesn't overcome the problems denoted
above. IMO it would be better either to move the GPIO-base and the IRQ-shared
parameters definition to the gpio-dwapb.c driver despite of the so called
"layering violation" or just leave them in the MFD driver. Linus, please join
the discussion. Do you have any better idea of what to do with these
properties?

-Sergey

> 
> ...
> 
> > > What you are talking about? Can you provide a code we can discuss?
> >
> > Here is what I suggest in step-by-step:
> > Patch 1) Move the Quark-specific GPIO-base and irq_shared parameters definition
> >          to the gpio-dwapb.c driver.
> 
> Layering violation: spreading board code over the kernel.
> 
> > Patch 2) Retrieve the GPIO-base and IRQ-shared parameters only for the DW APB
> >          GPIO block living in the Quark platform (by means of either ACPI or
> >          the platform-device ID data).
> 
> Same. (The rest doesn't matter. I dropped it because it's basically
> the same what I proposed)
> 
> > By doing as I suggested you don't have to pass around the GPIO-base value and
> > the IRQ-shared flag. They will be initialized and utilized locally in the
> > DW APB GPIO driver if Quark-specific DW APB GPIO block is detected (patches 1
> > and 2). In that case a software-node created in Patch 3 and 4 will be a normal
> > firmware node described by the DW bindings. You'll still be able to get rid of
> > the legacy global platform-data in the Patch 5.
> >
> > The spirit of your original patchset will still be preserved: introduce the
> > software-nodes propagation interface and use it to pass the generic DW APB GPIO
> > parameters. The only difference is that we'd move the GPIO-base and IRQ-shared
> > functionality fully into the DW APB GPIO driver. By doing so we'd have:
> > 1) DW APB GPIO Quark-specifics localized in a single gpio-dwapb driver.
> >    In the current implementation we have them distributed between two drivers:
> >    intel_quark_i2c_gpio initializes the GPIO-base and IRQ-shared parameters,
> >    gpio-dwapb uses them for setup. Note there is no any other platform with DW
> >    APB GPIO which needs the GPIO-base and IRQ-shared flag being setup. Why do
> >    we need to have such complicated interface then if we can identify whether
> >    the particular DW APB GPIO is a Quark-specific block or not? Moreover having
> >    those specifics in both drivers in fact means having quirks in both of them.
> > 2) Quirk-clean intel_quark_i2c_gpio driver which creates a normal DW APB GPIO
> >    device with software-nodes fully compatible with DT binding. DW APB GPIO
> >    driver will detect whether the block is Quark-specific and will activate
> >    the corresponding quirks if it is.
> > 3) Still legacy global platform-data removed.
> 
> I see. But you mistakenly made a conclusion 1). It will be spread to
> two drivers.
> And in 2) you considered (wrongly) that MFD driver is not a board code.
> 
> Again MFD driver _is_ a board code or i.o.w. quirk driver for specific
> Quark (MFD) device.
> 
> -- 
> With Best Regards,
> Andy Shevchenko

Re: [PATCH v1 0/6] mfd: Make use of software nodes

[Andy Shevchenko]

On Tue, Jun 23, 2020 at 4:49 AM Serge Semin <fancer.lancer@gmail.com> wrote:
> On Sat, Jun 20, 2020 at 01:13:56PM +0300, Andy Shevchenko wrote:
> > On Sat, Jun 20, 2020 at 1:12 AM Serge Semin <fancer.lancer@gmail.com> wrote:
> > > On Thu, Jun 18, 2020 at 11:56:54AM +0300, Andy Shevchenko wrote:
> > > > On Wed, Jun 17, 2020 at 01:56:48AM +0300, Serge Semin wrote:
> > > > > On Wed, Jun 17, 2020 at 12:40:35AM +0300, Andy Shevchenko wrote:
> > > > > > On Tue, Jun 16, 2020 at 11:03 PM Serge Semin <fancer.lancer@gmail.com> wrote:
> > > > > > > On Mon, Jun 08, 2020 at 04:42:54PM +0300, Andy Shevchenko wrote:

...

> > Linus replied to three messages out of 7, seems you read only the first one.

> Actually I've read all of them and from what I could see, he didn't like a code
> setting gpio-base regardless whether it has been created before your patchset
> or your explanation why it was necessary.

Yes, and then he realized what happened in the past. TBH I also do not
like this kind of thing, but we may not get rid of it because we will
get a regression.
The compromise (which is used in several cases of my knowledge, but I
believe there are more) is to use internal property. This keeps layers
separate and individual drivers nicer and neater.

...

> > > My point is that no mater how you pass the gpio-base value it will always be a
> > > quirk. The same thing is with the irq_shared flag. Both of them are specific to
> > > the Quark-platform. They are used to tune the DW APB GPIO driver up so one would
> > > create a GPIO device working well with the Quark-specific DW APB GPIO block.
> >
> > Precisely!
> >
> > > > > > We, by applying this series, make (keep) layers independent: board
> > > > > > code vs. driver code. Mixing them more is the opposite to what I
> > > > > > propose.
> > > > > >
> > > > > > WRT property.
> > > > > > snps,gpio-base can be easily changed to *already in use* gpio-base or
> > > > > > being both converted to linux,gpio-base to explicitly show people that
> > > > > > this is internal stuff that must not be present in firmware nodes.
> > > > >
> > > > > As I see it the part with "gpio-base" and the irq_shared can be moved to the
> > > > > gpio-dwapb.c driver to be defined as the quark-specific quirks. Adding a
> > > > > property like "gpio-base" seems like a quirk anyway, so I'd leave it defined in
> > > > > the driver.
> > > >
> > >
> > > > Huh?! The whole idea is make GPIO driver agnostic from platforms and their quirks.
> > >
> > > As I said above having the gpio-base value set is the platform-specific thing in
> > > any case. So no mater whether you pass it via the software_node/properties or
> > > the private platform-data structure, the DW APB GPIO driver will have to handle
> > > those parameters in some special way, which is quirk-prone since normal platforms
> > > don't have such peculiar requirements.
> >
>
> > Seems you are proposing layering violation without seeing it.
> > Let me explain the design of the drivers in Linux kernel.
> >
> > There are basically few possible concepts how drivers are handling
> > quirks (because we know that most of the hardware support is a set of
> > quirks here and there):
> > 1) platform provides a board files and via platform data structures
> > supplies quirks to the certain driver
> > 2) platform provides a firmware node (ACPI, DT, ...) and unified
> > driver handles it thru the fwnode interface
> > 3) driver is split to be a library (or core part) + glue drivers full of quirks
> > 4) driver has embedded quirks and supplies them based on IDs (PCI ID,
> > ACPI ID, compatible string, ID table, etc)
> > 5) ...missed something? ...
> >
> > What I'm proposing is turn 1 to 2 for Quark case, what you are
> > proposing is absent on the picture, i.e.:
> > x) platform provides a board file (intel_quark_i2c_gpio.c) and the
> > driver has embedded quirks based on ID.
> >
> > This is not what we want, definitely.
>
> From the list you provided it's still not obvious why what I suggested wasn't
> good, because it's perfectly fine to have both ACPI/DT-based firmware node
> (entry 2) and quirks based on IDs (entry 4), which plenty of the kernel
> drivers do.

And I'm not talking about combinations of 2+4, what I'm talking about
is 1+4 which *is* layering violation and a bad idea to start with.

> The difference between the most of those drivers and what I
> suggested is that by bonding a device with a driver they provide
> !device!-quirks, but not the !platform!-quirks. While the platform quirks
> and platform properties are normally provided by means of the platform data,
> firmware nodes, glue layers, etc. Moving some of the platform-quirks to a
> driver you called "layering violation". Well, I've never met that definition
> in the kernel before

> (have you just come up with it?),

Nope, I heard it many times during reviews.

> but at least I see
> what you meant.

> Anyway there are GPIO-drivers which still use the device IDs to get the
> platform quirks

Maybe you missed that we have more players here than simple dwapb-gpio?
How many of them are part of MFD (being used as MFD cells)?

> or get the GPIO-base from an of node alias (gpio-zynq.c,
> gpio-vf610.c, gpio-zx.c, gpio-mxc.c, gpio-mxs.c). There are even some,
> which either use a static variable to redistribute the GPIO-base between
> all available GPIO-chips (gpio-brcmstb.c, gpio-sta2x11.c, gpio-omap.c)
> or set a fixed GPIO-based value (like gpio-xlp.c, gpio-iop.c, gpio-ep93xx.c,
> gpio-vx855.c, gpio-cs5535.c, gpio-sch311x.c, gpio-loongson.c, gpio-loongson1.c,
> gpio-ath79.c, gpio-octeon.c),

So,  the question is above.

> or even get the GPIO-base value from some
> hardware register (gpio-merrifield.c, gpio-intel-mid.c).

These examples are not good, they are not part of MFD and the latter
one actually should be a glue driver to gpio-pxa.c.

> I am pretty sure
> the examples of having the locally-defined platform quirks and the concepts
> 1, 2, 3 and 4 at some extent utilized in a single driver can be found in another
> subsystems too.

I'm pretty sure layering violation can be found in many places in the
kernel. It doesn't mean we have to take bad or different examples as
suitable.

> I am not saying, that the approaches utilized in those drivers are ideal.
> Those are just examples, that the platform specifics can be reflected in
> the corresponding drivers and the so called "layering violation" is allowed
> at some circumstances. Linus, correct me if I am wrong.

It depends how a certain driver is being used. In our case we don't
need to spread board code over the kernel, we may be smarter than
that!

> Getting back to this patchset. As I see it, the main problem here is connected
> with two parameters:
> - GPIO-base. You suggest to update the gpio-dwapb.c driver so one would
>   support a firmware property like "gpio-base". It's not good, since the
>   property will be implicitly supported by OF API as well and nothing will
>   prevent a user from using it. Even though you said that we won't advertise
>   that property, some user may try to define it in dts anyway, which can be
>   easily missed on review.

No, if we don't advertise that and if we add "linux," prefix (see DWC3
for example) to be explicit what this is used for and why.

> - IRQ-shared. As I said before it's not good to replace the irq_shared flag
>   with the to_of_node() macro. Because having to_of_node() returned an
>   of-node doesn't mean the IRQs can't be shared.

This is a valid point, but I would ask you, as a more familiar guy
with the OF/DT system,  what suggestion would be better?

> As I see it the convenience provided by your patchset in relation to the
> GPIO-base and IRQ-shared properties doesn't overcome the problems denoted
> above. IMO it would be better either to move the GPIO-base and the IRQ-shared
> parameters definition to the gpio-dwapb.c driver despite of the so called
> "layering violation" or just leave them in the MFD driver. Linus, please join
> the discussion. Do you have any better idea of what to do with these
> properties?

Moving them to gpio-dwapb is a silly move. Better to do nothing if you
insist, but I consider that is non-constructive.

--
With Best Regards,
Andy Shevchenko