[PATCH v1 1/4] gpio: dwapb: Unify ACPI enumeration checks in get_irq() and configure_irqs()

[PATCH v1 1/4] gpio: dwapb: Unify ACPI enumeration checks in get_irq() and configure_irqs()

[Andy Shevchenko]

Shared IRQ is only enabled for ACPI enumeration, there is no need
to have a special flag for that, since we simple can test if device
has been enumerated by ACPI. This unifies the checks in dwapb_get_irq()
and dwapb_configure_irqs().

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/gpio/gpio-dwapb.c                | 13 ++++++-------
 drivers/mfd/intel_quark_i2c_gpio.c       |  1 -
 include/linux/platform_data/gpio-dwapb.h |  1 -
 3 files changed, 6 insertions(+), 9 deletions(-)

diff --git a/drivers/gpio/gpio-dwapb.c b/drivers/gpio/gpio-dwapb.c
index 3eb13d6d31ef..f6ae69d5d644 100644
--- a/drivers/gpio/gpio-dwapb.c
+++ b/drivers/gpio/gpio-dwapb.c
@@ -436,12 +436,7 @@ static void dwapb_configure_irqs(struct dwapb_gpio *gpio,
 	pirq->irqchip.irq_set_wake = dwapb_irq_set_wake;
 #endif
 
-	if (!pp->irq_shared) {
-		girq->num_parents = pirq->nr_irqs;
-		girq->parents = pirq->irq;
-		girq->parent_handler_data = gpio;
-		girq->parent_handler = dwapb_irq_handler;
-	} else {
+	if (has_acpi_companion(gpio->dev)) {
 		/* This will let us handle the parent IRQ in the driver */
 		girq->num_parents = 0;
 		girq->parents = NULL;
@@ -458,6 +453,11 @@ static void dwapb_configure_irqs(struct dwapb_gpio *gpio,
 			dev_err(gpio->dev, "error requesting IRQ\n");
 			goto err_kfree_pirq;
 		}
+	} else {
+		girq->num_parents = pirq->nr_irqs;
+		girq->parents = pirq->irq;
+		girq->parent_handler_data = gpio;
+		girq->parent_handler = dwapb_irq_handler;
 	}
 
 	girq->chip = &pirq->irqchip;
@@ -581,7 +581,6 @@ static struct dwapb_platform_data *dwapb_gpio_get_pdata(struct device *dev)
 			pp->ngpio = DWAPB_MAX_GPIOS;
 		}
 
-		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 01935ae4e9e1..a43993e38b6e 100644
--- a/drivers/mfd/intel_quark_i2c_gpio.c
+++ b/drivers/mfd/intel_quark_i2c_gpio.c
@@ -227,7 +227,6 @@ static int intel_quark_gpio_setup(struct pci_dev *pdev)
 	pdata->properties->ngpio	= INTEL_QUARK_MFD_NGPIO;
 	pdata->properties->gpio_base	= INTEL_QUARK_MFD_GPIO_BASE;
 	pdata->properties->irq[0]	= pci_irq_vector(pdev, 0);
-	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 0aa5c6720259..535e5ed549d9 100644
--- a/include/linux/platform_data/gpio-dwapb.h
+++ b/include/linux/platform_data/gpio-dwapb.h
@@ -14,7 +14,6 @@ struct dwapb_port_property {
 	unsigned int	ngpio;
 	unsigned int	gpio_base;
 	int		irq[DWAPB_MAX_GPIOS];
-	bool		irq_shared;
 };
 
 struct dwapb_platform_data {
-- 
2.30.2

[PATCH v1 2/4] gpio: dwapb: Read GPIO base from gpio-base property

[Andy Shevchenko]

For backward compatibility with some legacy devices introduce
a new (*) property gpio-base to read GPIO base. This will allow
further cleanup of the driver.

*) Note, it's not new for GPIO library since mockup driver is
   using it already.

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 f6ae69d5d644..e3011d4e17b0 100644
--- a/drivers/gpio/gpio-dwapb.c
+++ b/drivers/gpio/gpio-dwapb.c
@@ -581,7 +581,8 @@ static struct dwapb_platform_data *dwapb_gpio_get_pdata(struct device *dev)
 			pp->ngpio = DWAPB_MAX_GPIOS;
 		}
 
-		pp->gpio_base	= -1;
+		if (fwnode_property_read_u32(fwnode, "gpio-base", &pp->gpio_base))
+			pp->gpio_base = -1;
 
 		/*
 		 * Only port A can provide interrupts in all configurations of
-- 
2.30.2

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

[Serge Semin]

+Cc Rob

On Mon, Jul 26, 2021 at 03:54:34PM +0300, Andy Shevchenko wrote:
> For backward compatibility with some legacy devices introduce
> a new (*) property gpio-base to read GPIO base. This will allow
> further cleanup of the driver.
> 
> *) Note, it's not new for GPIO library since mockup driver is
>    using it already.

You are right but I don't think it's a good idea to advertise the
pure Linux-internal property "gpio-base" to any use-case like OF
and ACPI FW nodes. Especially seeing we don't have it described in the
DT-bindings and noting that the mockup driver is dedicated for the
GPIO tests only. What about restricting the property usage for the
SW-nodes only by adding an additional check: is_software_node() here?

@Linus, @Bartosz, @Rob, what do you think about that?

-Sergey

> 
> 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 f6ae69d5d644..e3011d4e17b0 100644
> --- a/drivers/gpio/gpio-dwapb.c
> +++ b/drivers/gpio/gpio-dwapb.c
> @@ -581,7 +581,8 @@ static struct dwapb_platform_data *dwapb_gpio_get_pdata(struct device *dev)
>  			pp->ngpio = DWAPB_MAX_GPIOS;
>  		}
>  
> -		pp->gpio_base	= -1;
> +		if (fwnode_property_read_u32(fwnode, "gpio-base", &pp->gpio_base))
> +			pp->gpio_base = -1;
>  
>  		/*
>  		 * Only port A can provide interrupts in all configurations of
> -- 
> 2.30.2
> 

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

[Andy Shevchenko]

On Mon, Aug 2, 2021 at 5:14 PM Serge Semin <fancer.lancer@gmail.com> wrote:
> On Mon, Jul 26, 2021 at 03:54:34PM +0300, Andy Shevchenko wrote:
> > For backward compatibility with some legacy devices introduce
> > a new (*) property gpio-base to read GPIO base. This will allow
> > further cleanup of the driver.

Thanks for the review! My answers below.

> > *) Note, it's not new for GPIO library since mockup driver is
> >    using it already.
>
> You are right but I don't think it's a good idea to advertise the
> pure Linux-internal property "gpio-base" to any use-case like OF
> and ACPI FW nodes.

I don't want to advertise them, actually (that's why no bindings are
modified). Perhaps introducing a paragraph in the GPIO documentation
about this (and / or in GPIO generic bindings) that gpio-base property
is solely for internal use and should't be used in actual DTs?

>  Especially seeing we don't have it described in the
> DT-bindings and noting that the mockup driver is dedicated for the
> GPIO tests only. What about restricting the property usage for the
> SW-nodes only by adding an additional check: is_software_node() here?

I don't think we need this. But if you think it's better this way just
to avoid usage of this property outside of internal properties, I'm
fine to add. Perhaps we may issue a warning and continue? (see also
above)

> @Linus, @Bartosz, @Rob, what do you think about that?

-- 
With Best Regards,
Andy Shevchenko

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

[Serge Semin]

@Linus, @Bartosz, @Rob, could you join the discussion regarding the
gpio-base property usage?

On Mon, Aug 02, 2021 at 06:52:28PM +0300, Andy Shevchenko wrote:
> On Mon, Aug 2, 2021 at 5:14 PM Serge Semin <fancer.lancer@gmail.com> wrote:
> > On Mon, Jul 26, 2021 at 03:54:34PM +0300, Andy Shevchenko wrote:
> > > For backward compatibility with some legacy devices introduce
> > > a new (*) property gpio-base to read GPIO base. This will allow
> > > further cleanup of the driver.
> 
> Thanks for the review! My answers below.
> 
> > > *) Note, it's not new for GPIO library since mockup driver is
> > >    using it already.
> >
> > You are right but I don't think it's a good idea to advertise the
> > pure Linux-internal property "gpio-base" to any use-case like OF
> > and ACPI FW nodes.
> 

> I don't want to advertise them, actually (that's why no bindings are
> modified). Perhaps introducing a paragraph in the GPIO documentation
> about this (and / or in GPIO generic bindings) that gpio-base property
> is solely for internal use and should't be used in actual DTs?

It might have been not that clear but by "advertising" I meant to have
the property generically handled in the driver, thus permitting it
being specified not only via the SW-nodes but also via the ACPI
and OF firmware. (Please see my next comment for more details.)

Regarding adding the gpio-base property documentation. I am pretty
sure it shouldn't be mentioned neither in the DW APB GPIO bindings,
nor in any other GPIO device DT-bindings because as you are right
saying it is the solely Linux kernel-specific parameter and isn't
supposed to be part of the device tree specification. On the other
hand if it gets to be frequently used then indeed we need to somehow
have it described and of course make sure it isn't used
inappropriately. Thus a possible option of documenting the property
would be just adding a new paragraph/file somewhere in
Documentation/driver-api/gpio/ since the property name implies that
it's going to be generic and permitted to be specified for all
GPIO-chips. Though it's for @Linus and @Bartosz to decide after all.

> 
> >  Especially seeing we don't have it described in the
> > DT-bindings and noting that the mockup driver is dedicated for the
> > GPIO tests only. What about restricting the property usage for the
> > SW-nodes only by adding an additional check: is_software_node() here?
> 

> I don't think we need this. But if you think it's better this way just
> to avoid usage of this property outside of internal properties, I'm
> fine to add. Perhaps we may issue a warning and continue? (see also
> above)

In my opinion it's very required and here is why. Adding the generic
gpio-base property support into the driver basically means saying:
"Hey, the driver supports it, so you can add it to your firmware."
Even if the property isn't described in the bindings, the platform
developers will be able to use it in new DTS-files since it's much
easier to add a property into a DT-file and make things working than
to convert the drivers/platforms/apps to using the GPIOd API. In case
if maintainers aren't that careful at review such dts may get slip
into the kernel, which in its turn will de facto make the property
being part of the DT specification and will need to be supported. That
is we must be very careful in what properties are permitted in the
driver. Thus, yes, I think we need to make sure here that the property
is only used in framework of the kernel and isn't passed via
inappropriate paths like DT/ACPI fw so not to get into the
maintainability troubles in future.

Issuing a warning but accepting the property isn't good alternative
due to the same reason. Why do we need to add the DT/ACPI property
support, which isn't supposed to be used like that instead of just
restricting the usecases beforehand? So I vote for parsing the
"gpio-base" property only if it's passed as a part of the SW-node.

-Sergey

> 
> > @Linus, @Bartosz, @Rob, what do you think about that?
> 
> -- 
> With Best Regards,
> Andy Shevchenko

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

[Andy Shevchenko]

On Wed, Aug 04, 2021 at 03:44:33PM +0300, Serge Semin wrote:
> On Mon, Aug 02, 2021 at 06:52:28PM +0300, Andy Shevchenko wrote:
> > On Mon, Aug 2, 2021 at 5:14 PM Serge Semin <fancer.lancer@gmail.com> wrote:
> > > On Mon, Jul 26, 2021 at 03:54:34PM +0300, Andy Shevchenko wrote:
> > > > For backward compatibility with some legacy devices introduce
> > > > a new (*) property gpio-base to read GPIO base. This will allow
> > > > further cleanup of the driver.
> > 
> > Thanks for the review! My answers below.
> > 
> > > > *) Note, it's not new for GPIO library since mockup driver is
> > > >    using it already.
> > >
> > > You are right but I don't think it's a good idea to advertise the
> > > pure Linux-internal property "gpio-base" to any use-case like OF
> > > and ACPI FW nodes.
> 
> > I don't want to advertise them, actually (that's why no bindings are
> > modified). Perhaps introducing a paragraph in the GPIO documentation
> > about this (and / or in GPIO generic bindings) that gpio-base property
> > is solely for internal use and should't be used in actual DTs?
> 
> It might have been not that clear but by "advertising" I meant to have
> the property generically handled in the driver, thus permitting it
> being specified not only via the SW-nodes but also via the ACPI
> and OF firmware. (Please see my next comment for more details.)
> 
> Regarding adding the gpio-base property documentation. I am pretty
> sure it shouldn't be mentioned neither in the DW APB GPIO bindings,
> nor in any other GPIO device DT-bindings because as you are right
> saying it is the solely Linux kernel-specific parameter and isn't
> supposed to be part of the device tree specification. On the other
> hand if it gets to be frequently used then indeed we need to somehow
> have it described and of course make sure it isn't used
> inappropriately. Thus a possible option of documenting the property
> would be just adding a new paragraph/file somewhere in
> Documentation/driver-api/gpio/ since the property name implies that
> it's going to be generic and permitted to be specified for all
> GPIO-chips. Though it's for @Linus and @Bartosz to decide after all.

Thanks for elaborative point.

> > >  Especially seeing we don't have it described in the
> > > DT-bindings and noting that the mockup driver is dedicated for the
> > > GPIO tests only. What about restricting the property usage for the
> > > SW-nodes only by adding an additional check: is_software_node() here?
> 
> > I don't think we need this. But if you think it's better this way just
> > to avoid usage of this property outside of internal properties, I'm
> > fine to add. Perhaps we may issue a warning and continue? (see also
> > above)
> 
> In my opinion it's very required and here is why. Adding the generic
> gpio-base property support into the driver basically means saying:
> "Hey, the driver supports it, so you can add it to your firmware."
> Even if the property isn't described in the bindings, the platform
> developers will be able to use it in new DTS-files since it's much
> easier to add a property into a DT-file and make things working than
> to convert the drivers/platforms/apps to using the GPIOd API. In case
> if maintainers aren't that careful at review such dts may get slip
> into the kernel, which in its turn will de facto make the property
> being part of the DT specification and will need to be supported. That
> is we must be very careful in what properties are permitted in the
> driver. Thus, yes, I think we need to make sure here that the property
> is only used in framework of the kernel and isn't passed via
> inappropriate paths like DT/ACPI fw so not to get into the
> maintainability troubles in future.

Got it. I'll add the additional check in next version.

> Issuing a warning but accepting the property isn't good alternative
> due to the same reason. Why do we need to add the DT/ACPI property
> support, which isn't supposed to be used like that instead of just
> restricting the usecases beforehand? So I vote for parsing the
> "gpio-base" property only if it's passed as a part of the SW-node.

-- 
With Best Regards,
Andy Shevchenko

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

[Linus Walleij]

On Wed, Aug 4, 2021 at 4:44 PM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
> On Wed, Aug 04, 2021 at 03:44:33PM +0300, Serge Semin wrote:
> > Thus, yes, I think we need to make sure here that the property
> > is only used in framework of the kernel and isn't passed via
> > inappropriate paths like DT/ACPI fw so not to get into the
> > maintainability troubles in future.
>
> Got it. I'll add the additional check in next version.

This seems reasonable for me, if you can get this done with
some kind of elegance.

Maybe use the "linux,gpio-base" property as mentioned so it is
clear that this is a Linux-internal thing only.

Yours,
Linus Walleij

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

[Serge Semin]

Hello Linus

On Wed, Aug 11, 2021 at 02:40:49PM +0200, Linus Walleij wrote:
> On Wed, Aug 4, 2021 at 4:44 PM Andy Shevchenko
> <andy.shevchenko@gmail.com> wrote:
> > On Wed, Aug 04, 2021 at 03:44:33PM +0300, Serge Semin wrote:
> > > Thus, yes, I think we need to make sure here that the property
> > > is only used in framework of the kernel and isn't passed via
> > > inappropriate paths like DT/ACPI fw so not to get into the
> > > maintainability troubles in future.
> >
> > Got it. I'll add the additional check in next version.
> 

> This seems reasonable for me, if you can get this done with
> some kind of elegance.
> 

There is v2 of this series has already been posted:
https://lore.kernel.org/linux-gpio/20210804160019.77105-1-andriy.shevchenko@linux.intel.com/
with the denoted concern taken into account. 

-Sergey

> Maybe use the "linux,gpio-base" property as mentioned so it is
> clear that this is a Linux-internal thing only.
> 
> Yours,
> Linus Walleij

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

[Linus Walleij]

On Mon, Jul 26, 2021 at 2:54 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:

> For backward compatibility with some legacy devices introduce
> a new (*) property gpio-base to read GPIO base. This will allow
> further cleanup of the driver.
>
> *) Note, it's not new for GPIO library since mockup driver is
>    using it already.
>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
(...)
> -               pp->gpio_base   = -1;
> +               if (fwnode_property_read_u32(fwnode, "gpio-base", &pp->gpio_base))
> +                       pp->gpio_base = -1;

This is problematic because we have repeatedly NACKed this property
to be added to device trees.

I don't know about fwnode policies, but in the device tree this would have
to be "linux,gpio-base" and then it would be NACKed because of adding
an operating-system specific thing to a OS-independent hardware
description.

I don't know what to do with this really, but I understand the need of it
as a kernel-internal thing, however I am afraid that adding this will make
it possible to add linux,gpio-base = <n> to any device tree gpio_chip
as well and that encourages bad behaviour even if we don't allow a
DT binding (YAML) like that.

Is there a way to make a fwnode property only come from software
nodes and not allowed to be used in ACPI or DT? (I guess not...)

Yours,
Linus Walleij

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

[Andy Shevchenko]

On Wed, Aug 11, 2021 at 02:37:42PM +0200, Linus Walleij wrote:
> On Mon, Jul 26, 2021 at 2:54 PM Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
> 
> > For backward compatibility with some legacy devices introduce
> > a new (*) property gpio-base to read GPIO base. This will allow
> > further cleanup of the driver.
> >
> > *) Note, it's not new for GPIO library since mockup driver is
> >    using it already.
> >
> > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> (...)
> > -               pp->gpio_base   = -1;
> > +               if (fwnode_property_read_u32(fwnode, "gpio-base", &pp->gpio_base))
> > +                       pp->gpio_base = -1;
> 
> This is problematic because we have repeatedly NACKed this property
> to be added to device trees.
> 
> I don't know about fwnode policies, but in the device tree this would have
> to be "linux,gpio-base" and then it would be NACKed because of adding
> an operating-system specific thing to a OS-independent hardware
> description.
> 
> I don't know what to do with this really, but I understand the need of it
> as a kernel-internal thing, however I am afraid that adding this will make
> it possible to add linux,gpio-base = <n> to any device tree gpio_chip
> as well and that encourages bad behaviour even if we don't allow a
> DT binding (YAML) like that.
> 
> Is there a way to make a fwnode property only come from software
> nodes and not allowed to be used in ACPI or DT? (I guess not...)

This has been the very same concern by Serge and we agreed on limiting this to
software nodes only. And I have seen you are fine with the approach, thanks!

-- 
With Best Regards,
Andy Shevchenko

[PATCH v1 3/4] mfd: intel_quark_i2c_gpio: Convert GPIO 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 | 70 ++++++++++++++++--------------
 1 file changed, 37 insertions(+), 33 deletions(-)

diff --git a/drivers/mfd/intel_quark_i2c_gpio.c b/drivers/mfd/intel_quark_i2c_gpio.c
index a43993e38b6e..9b9c76bd067b 100644
--- a/drivers/mfd/intel_quark_i2c_gpio.c
+++ b/drivers/mfd/intel_quark_i2c_gpio.c
@@ -17,7 +17,6 @@
 #include <linux/clk-provider.h>
 #include <linux/dmi.h>
 #include <linux/i2c.h>
-#include <linux/platform_data/gpio-dwapb.h>
 #include <linux/property.h>
 
 /* PCI BAR for register base address */
@@ -28,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
 
@@ -111,12 +101,38 @@ 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("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[] = {
 	[MFD_I2C_BAR] = {
 		.id = MFD_I2C_BAR,
@@ -203,34 +219,19 @@ static int intel_quark_gpio_setup(struct pci_dev *pdev)
 {
 	struct mfd_cell *cell = &intel_quark_mfd_cells[MFD_GPIO_BAR];
 	struct resource *res = intel_quark_gpio_res;
-	struct dwapb_platform_data *pdata;
-	struct device *dev = &pdev->dev;
+	int ret;
 
 	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]	= pci_irq_vector(pdev, 0);
+	res[INTEL_QUARK_IORES_IRQ].start = pci_irq_vector(pdev, 0);
+	res[INTEL_QUARK_IORES_IRQ].end = pci_irq_vector(pdev, 0);
 
-	cell->platform_data = pdata;
-	cell->pdata_size = sizeof(*pdata);
+	ret = software_node_register_node_group(intel_quark_gpio_node_group);
+	if (ret)
+		return ret;
 
+	cell->swnode = &intel_quark_gpio_controller_node;
 	return 0;
 }
 
@@ -273,10 +274,12 @@ static int intel_quark_mfd_probe(struct pci_dev *pdev,
 			      ARRAY_SIZE(intel_quark_mfd_cells), NULL, 0,
 			      NULL);
 	if (ret)
-		goto err_free_irq_vectors;
+		goto err_unregister_gpio_node_group;
 
 	return 0;
 
+err_unregister_gpio_node_group:
+	software_node_unregister_node_group(intel_quark_gpio_node_group);
 err_free_irq_vectors:
 	pci_free_irq_vectors(pdev);
 err_unregister_i2c_clk:
@@ -287,6 +290,7 @@ static int intel_quark_mfd_probe(struct pci_dev *pdev,
 static void intel_quark_mfd_remove(struct pci_dev *pdev)
 {
 	mfd_remove_devices(&pdev->dev);
+	software_node_unregister_node_group(intel_quark_gpio_node_group);
 	pci_free_irq_vectors(pdev);
 	intel_quark_unregister_i2c_clk(&pdev->dev);
 }
-- 
2.30.2

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

[Linus Walleij]

On Mon, Jul 26, 2021 at 2:54 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:

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

This is really nice, I wish I knew better how to use this mechanism
myself, so I need to practice.
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij

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

[Andy Shevchenko]

On Wed, Aug 11, 2021 at 11:38 AM Linus Walleij <linus.walleij@linaro.org> wrote:
> On Mon, Jul 26, 2021 at 2:54 PM Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
>
> > 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>
>
> This is really nice, I wish I knew better how to use this mechanism
> myself, so I need to practice.

This driver is actually a complex enough example covering 90%+ cases
of software nodes in the board files.

> Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Thanks!


-- 
With Best Regards,
Andy Shevchenko

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

[Lee Jones]

On Mon, 26 Jul 2021, Andy Shevchenko wrote:

> 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 | 70 ++++++++++++++++--------------
>  1 file changed, 37 insertions(+), 33 deletions(-)

Doesn't seem to want to apply.

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

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

[Andy Shevchenko]

On Mon, Aug 16, 2021 at 4:11 PM Lee Jones <lee.jones@linaro.org> wrote:
> On Mon, 26 Jul 2021, Andy Shevchenko wrote:
>
> > 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 | 70 ++++++++++++++++--------------
> >  1 file changed, 37 insertions(+), 33 deletions(-)
>
> Doesn't seem to want to apply.

Would it be okay for you to pull the immutable tag?

-- 
With Best Regards,
Andy Shevchenko

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

[Lee Jones]

On Mon, 16 Aug 2021, Andy Shevchenko wrote:

> On Mon, Aug 16, 2021 at 4:11 PM Lee Jones <lee.jones@linaro.org> wrote:
> > On Mon, 26 Jul 2021, Andy Shevchenko wrote:
> >
> > > 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 | 70 ++++++++++++++++--------------
> > >  1 file changed, 37 insertions(+), 33 deletions(-)
> >
> > Doesn't seem to want to apply.
> 
> Would it be okay for you to pull the immutable tag?

What immutable tag?

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

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

[Andy Shevchenko]

On Mon, Aug 16, 2021 at 02:33:28PM +0100, Lee Jones wrote:
> On Mon, 16 Aug 2021, Andy Shevchenko wrote:
> 
> > On Mon, Aug 16, 2021 at 4:11 PM Lee Jones <lee.jones@linaro.org> wrote:
> > > On Mon, 26 Jul 2021, Andy Shevchenko wrote:
> > >
> > > > 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 | 70 ++++++++++++++++--------------
> > > >  1 file changed, 37 insertions(+), 33 deletions(-)
> > >
> > > Doesn't seem to want to apply.
> > 
> > Would it be okay for you to pull the immutable tag?
> 
> What immutable tag?

It's here:
https://git.kernel.org/pub/scm/linux/kernel/git/andy/linux-gpio-intel.git/tag/?h=intel-gpio-v5.15-1


-- 
With Best Regards,
Andy Shevchenko

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

[Lee Jones]

On Mon, 16 Aug 2021, Andy Shevchenko wrote:

> On Mon, Aug 16, 2021 at 02:33:28PM +0100, Lee Jones wrote:
> > On Mon, 16 Aug 2021, Andy Shevchenko wrote:
> > 
> > > On Mon, Aug 16, 2021 at 4:11 PM Lee Jones <lee.jones@linaro.org> wrote:
> > > > On Mon, 26 Jul 2021, Andy Shevchenko wrote:
> > > >
> > > > > 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 | 70 ++++++++++++++++--------------
> > > > >  1 file changed, 37 insertions(+), 33 deletions(-)
> > > >
> > > > Doesn't seem to want to apply.
> > > 
> > > Would it be okay for you to pull the immutable tag?
> > 
> > What immutable tag?
> 
> It's here:
> https://git.kernel.org/pub/scm/linux/kernel/git/andy/linux-gpio-intel.git/tag/?h=intel-gpio-v5.15-1

My Ack can't be merged like that.

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

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

[Andy Shevchenko]

On Mon, Aug 16, 2021 at 5:19 PM Lee Jones <lee.jones@linaro.org> wrote:
> On Mon, 16 Aug 2021, Andy Shevchenko wrote:
> > On Mon, Aug 16, 2021 at 02:33:28PM +0100, Lee Jones wrote:
> > > On Mon, 16 Aug 2021, Andy Shevchenko wrote:
> > > > On Mon, Aug 16, 2021 at 4:11 PM Lee Jones <lee.jones@linaro.org> wrote:
> > > > > On Mon, 26 Jul 2021, Andy Shevchenko wrote:
> > > > >
> > > > > > 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 | 70 ++++++++++++++++--------------
> > > > > >  1 file changed, 37 insertions(+), 33 deletions(-)
> > > > >
> > > > > Doesn't seem to want to apply.
> > > >
> > > > Would it be okay for you to pull the immutable tag?
> > >
> > > What immutable tag?
> >
> > It's here:
> > https://git.kernel.org/pub/scm/linux/kernel/git/andy/linux-gpio-intel.git/tag/?h=intel-gpio-v5.15-1
>
> My Ack can't be merged like that.

Which one? There are two on different patches.
Do you have any documentation on the rules you imply by MFD?

-- 
With Best Regards,
Andy Shevchenko

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

[Lee Jones]

On Mon, 16 Aug 2021, Andy Shevchenko wrote:

> On Mon, Aug 16, 2021 at 5:19 PM Lee Jones <lee.jones@linaro.org> wrote:
> > On Mon, 16 Aug 2021, Andy Shevchenko wrote:
> > > On Mon, Aug 16, 2021 at 02:33:28PM +0100, Lee Jones wrote:
> > > > On Mon, 16 Aug 2021, Andy Shevchenko wrote:
> > > > > On Mon, Aug 16, 2021 at 4:11 PM Lee Jones <lee.jones@linaro.org> wrote:
> > > > > > On Mon, 26 Jul 2021, Andy Shevchenko wrote:
> > > > > >
> > > > > > > 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 | 70 ++++++++++++++++--------------
> > > > > > >  1 file changed, 37 insertions(+), 33 deletions(-)
> > > > > >
> > > > > > Doesn't seem to want to apply.
> > > > >
> > > > > Would it be okay for you to pull the immutable tag?
> > > >
> > > > What immutable tag?
> > >
> > > It's here:
> > > https://git.kernel.org/pub/scm/linux/kernel/git/andy/linux-gpio-intel.git/tag/?h=intel-gpio-v5.15-1
> >
> > My Ack can't be merged like that.
> 
> Which one? There are two on different patches.

The one that I specifically said was "for my own reference".

> Do you have any documentation on the rules you imply by MFD?

No, the documentation is provided with the tag.

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

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

[Andy Shevchenko]

On Tue, Aug 17, 2021 at 10:26 AM Lee Jones <lee.jones@linaro.org> wrote:
> On Mon, 16 Aug 2021, Andy Shevchenko wrote:
> > On Mon, Aug 16, 2021 at 5:19 PM Lee Jones <lee.jones@linaro.org> wrote:
> > > On Mon, 16 Aug 2021, Andy Shevchenko wrote:

...

> > > > > > Would it be okay for you to pull the immutable tag?
> > > > >
> > > > > What immutable tag?
> > > >
> > > > It's here:
> > > > https://git.kernel.org/pub/scm/linux/kernel/git/andy/linux-gpio-intel.git/tag/?h=intel-gpio-v5.15-1
> > >
> > > My Ack can't be merged like that.
> >
> > Which one? There are two on different patches.
>
> The one that I specifically said was "for my own reference".
>
> > Do you have any documentation on the rules you imply by MFD?
>
> No, the documentation is provided with the tag.

I see.

So, what is the recommended solution?

-- 
With Best Regards,
Andy Shevchenko

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

[Lee Jones]

On Tue, 17 Aug 2021, Andy Shevchenko wrote:

> On Tue, Aug 17, 2021 at 10:26 AM Lee Jones <lee.jones@linaro.org> wrote:
> > On Mon, 16 Aug 2021, Andy Shevchenko wrote:
> > > On Mon, Aug 16, 2021 at 5:19 PM Lee Jones <lee.jones@linaro.org> wrote:
> > > > On Mon, 16 Aug 2021, Andy Shevchenko wrote:
> 
> ...
> 
> > > > > > > Would it be okay for you to pull the immutable tag?
> > > > > >
> > > > > > What immutable tag?
> > > > >
> > > > > It's here:
> > > > > https://git.kernel.org/pub/scm/linux/kernel/git/andy/linux-gpio-intel.git/tag/?h=intel-gpio-v5.15-1
> > > >
> > > > My Ack can't be merged like that.
> > >
> > > Which one? There are two on different patches.
> >
> > The one that I specifically said was "for my own reference".
> >
> > > Do you have any documentation on the rules you imply by MFD?
> >
> > No, the documentation is provided with the tag.
> 
> I see.
> 
> So, what is the recommended solution?

I planned to take the patch.

I'm also happy to take the set, if they are interdependent.

What is the reason the MFD patch doesn't apply to my tree?

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

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

[Andy Shevchenko]

On Wed, Aug 18, 2021 at 9:34 AM Lee Jones <lee.jones@linaro.org> wrote:
>
> On Tue, 17 Aug 2021, Andy Shevchenko wrote:
>
> > On Tue, Aug 17, 2021 at 10:26 AM Lee Jones <lee.jones@linaro.org> wrote:
> > > On Mon, 16 Aug 2021, Andy Shevchenko wrote:
> > > > On Mon, Aug 16, 2021 at 5:19 PM Lee Jones <lee.jones@linaro.org> wrote:
> > > > > On Mon, 16 Aug 2021, Andy Shevchenko wrote:
> >
> > ...
> >
> > > > > > > > Would it be okay for you to pull the immutable tag?
> > > > > > >
> > > > > > > What immutable tag?
> > > > > >
> > > > > > It's here:
> > > > > > https://git.kernel.org/pub/scm/linux/kernel/git/andy/linux-gpio-intel.git/tag/?h=intel-gpio-v5.15-1
> > > > >
> > > > > My Ack can't be merged like that.
> > > >
> > > > Which one? There are two on different patches.
> > >
> > > The one that I specifically said was "for my own reference".
> > >
> > > > Do you have any documentation on the rules you imply by MFD?
> > >
> > > No, the documentation is provided with the tag.
> >
> > I see.
> >
> > So, what is the recommended solution?
>
> I planned to take the patch.
>
> I'm also happy to take the set, if they are interdependent.
>
> What is the reason the MFD patch doesn't apply to my tree?

What I did right now:

$ git checkout -b test-mfd-merge mfd/for-mfd-next
Updating files: 100% (8674/8674), done.
Branch 'test-mfd-merge' set up to track remote branch 'for-mfd-next' from 'mfd'.
Switched to a new branch 'test-mfd-merge'

$ b4 am 20210726125436.58685-2-andriy.shevchenko@linux.intel.com
Looking up https://lore.kernel.org/r/20210726125436.58685-2-andriy.shevchenko%40linux.intel.com
Grabbing thread from lore.kernel.org/linux-gpio
Analyzing 28 messages in the thread
---
Writing ./20210726_andriy_shevchenko_gpio_dwapb_unify_acpi_enumeration_checks_in_get_irq_and_configure_irqs.mbx
  [PATCH v1 1/4] gpio: dwapb: Unify ACPI enumeration checks in
get_irq() and configure_irqs()
    + Acked-by: Lee Jones <lee.jones@linaro.org> (✓ DKIM/linaro.org)
    + Acked-by: Serge Semin <fancer.lancer@gmail.com> (✓ DKIM/gmail.com)
    + Tested-by: Serge Semin <fancer.lancer@gmail.com> (✓ DKIM/gmail.com)
  [PATCH v1 2/4] gpio: dwapb: Read GPIO base from gpio-base property
  [PATCH v1 3/4] mfd: intel_quark_i2c_gpio: Convert GPIO to use software nodes
    + Reviewed-by: Linus Walleij <linus.walleij@linaro.org> (✓ DKIM/linaro.org)
  [PATCH v1 4/4] gpio: dwapb: Get rid of legacy platform data
    + Acked-by: Serge Semin <fancer.lancer@gmail.com> (✓ DKIM/gmail.com)
    + Tested-by: Serge Semin <fancer.lancer@gmail.com> (✓ DKIM/gmail.com)
---
Total patches: 4
---
 Link: https://lore.kernel.org/r/20210726125436.58685-1-andriy.shevchenko@linux.intel.com
 Base: not found (applies clean to current tree)
       git am ./20210726_andriy_shevchenko_gpio_dwapb_unify_acpi_enumeration_checks_in_get_irq_and_configure_irqs.mbx

$ git am ./20210726_andriy_shevchenko_gpio_dwapb_unify_acpi_enumeration_checks_in_get_irq_and_configure_irqs.mbx
Applying: gpio: dwapb: Unify ACPI enumeration checks in get_irq() and
configure_irqs()
Applying: gpio: dwapb: Read GPIO base from gpio-base property
Applying: mfd: intel_quark_i2c_gpio: Convert GPIO to use software nodes
Applying: gpio: dwapb: Get rid of legacy platform data

### here we need to rebase and add your tag

No conflicts, no nothing with your current _published_ for-mfd-next.
Do you have local changes that have not been yet visible?

Alternatively you may pull the tag I have.

-- 
With Best Regards,
Andy Shevchenko

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

[Lee Jones]

On Tue, 17 Aug 2021, Andy Shevchenko wrote:

> On Tue, Aug 17, 2021 at 10:26 AM Lee Jones <lee.jones@linaro.org> wrote:
> > On Mon, 16 Aug 2021, Andy Shevchenko wrote:
> > > On Mon, Aug 16, 2021 at 5:19 PM Lee Jones <lee.jones@linaro.org> wrote:
> > > > On Mon, 16 Aug 2021, Andy Shevchenko wrote:
> 
> ...
> 
> > > > > > > Would it be okay for you to pull the immutable tag?
> > > > > >
> > > > > > What immutable tag?
> > > > >
> > > > > It's here:
> > > > > https://git.kernel.org/pub/scm/linux/kernel/git/andy/linux-gpio-intel.git/tag/?h=intel-gpio-v5.15-1
> > > >
> > > > My Ack can't be merged like that.
> > >
> > > Which one? There are two on different patches.
> >
> > The one that I specifically said was "for my own reference".
> >
> > > Do you have any documentation on the rules you imply by MFD?
> >
> > No, the documentation is provided with the tag.
> 
> I see.

And now the patch is merged with an invalid tag! *facepalm*

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

[PATCH v1 4/4] 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                | 28 +++++++++++++++---------
 include/linux/platform_data/gpio-dwapb.h | 24 --------------------
 2 files changed, 18 insertions(+), 34 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 e3011d4e17b0..b9dd0ba812dc 100644
--- a/drivers/gpio/gpio-dwapb.c
+++ b/drivers/gpio/gpio-dwapb.c
@@ -16,7 +16,6 @@
 #include <linux/mod_devicetable.h>
 #include <linux/module.h>
 #include <linux/of.h>
-#include <linux/platform_data/gpio-dwapb.h>
 #include <linux/platform_device.h>
 #include <linux/property.h>
 #include <linux/reset.h>
@@ -48,6 +47,7 @@
 
 #define DWAPB_DRIVER_NAME	"gpio-dwapb"
 #define DWAPB_MAX_PORTS		4
+#define DWAPB_MAX_GPIOS		32
 
 #define GPIO_EXT_PORT_STRIDE	0x04 /* register stride 32 bits */
 #define GPIO_SWPORT_DR_STRIDE	0x0c /* register stride 3*32 bits */
@@ -63,6 +63,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[DWAPB_MAX_GPIOS];
+};
+
+struct dwapb_platform_data {
+	struct dwapb_port_property *properties;
+	unsigned int nports;
+};
+
 struct dwapb_gpio;
 
 #ifdef CONFIG_PM_SLEEP
@@ -670,17 +683,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 535e5ed549d9..000000000000
--- a/include/linux/platform_data/gpio-dwapb.h
+++ /dev/null
@@ -1,24 +0,0 @@
-/* SPDX-License-Identifier: GPL-2.0-only */
-/*
- * Copyright(c) 2014 Intel Corporation.
- */
-
-#ifndef GPIO_DW_APB_H
-#define GPIO_DW_APB_H
-
-#define DWAPB_MAX_GPIOS		32
-
-struct dwapb_port_property {
-	struct fwnode_handle *fwnode;
-	unsigned int	idx;
-	unsigned int	ngpio;
-	unsigned int	gpio_base;
-	int		irq[DWAPB_MAX_GPIOS];
-};
-
-struct dwapb_platform_data {
-	struct dwapb_port_property *properties;
-	unsigned int nports;
-};
-
-#endif
-- 
2.30.2

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

[Serge Semin]

On Mon, Jul 26, 2021 at 03:54:36PM +0300, Andy Shevchenko 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>
> ---
>  drivers/gpio/gpio-dwapb.c                | 28 +++++++++++++++---------
>  include/linux/platform_data/gpio-dwapb.h | 24 --------------------
>  2 files changed, 18 insertions(+), 34 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 e3011d4e17b0..b9dd0ba812dc 100644
> --- a/drivers/gpio/gpio-dwapb.c
> +++ b/drivers/gpio/gpio-dwapb.c
> @@ -16,7 +16,6 @@
>  #include <linux/mod_devicetable.h>
>  #include <linux/module.h>
>  #include <linux/of.h>
> -#include <linux/platform_data/gpio-dwapb.h>
>  #include <linux/platform_device.h>
>  #include <linux/property.h>
>  #include <linux/reset.h>
> @@ -48,6 +47,7 @@
>  
>  #define DWAPB_DRIVER_NAME	"gpio-dwapb"
>  #define DWAPB_MAX_PORTS		4
> +#define DWAPB_MAX_GPIOS		32
>  
>  #define GPIO_EXT_PORT_STRIDE	0x04 /* register stride 32 bits */
>  #define GPIO_SWPORT_DR_STRIDE	0x0c /* register stride 3*32 bits */
> @@ -63,6 +63,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[DWAPB_MAX_GPIOS];
> +};
> +
> +struct dwapb_platform_data {
> +	struct dwapb_port_property *properties;
> +	unsigned int nports;
> +};
> +
>  struct dwapb_gpio;

If you need to resend the series anyway could you please move the
structures declarations to being below the forward declaration of the
dwapb_gpio structure? Of course it's not that critical, but for the
sake of just not to have the later one left somewhere in the middle of
the unrelated structures and for at least to keep some order in the
declarations.

Then feel free to add:
Acked-by: Serge Semin <fancer.lancer@gmail.com>

The whole series has been tested on Baikal-T1 SoC:
Tested-by: Serge Semin <fancer.lancer@gmail.com>

-Sergey

>  
>  #ifdef CONFIG_PM_SLEEP
> @@ -670,17 +683,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 535e5ed549d9..000000000000
> --- a/include/linux/platform_data/gpio-dwapb.h
> +++ /dev/null
> @@ -1,24 +0,0 @@
> -/* SPDX-License-Identifier: GPL-2.0-only */
> -/*
> - * Copyright(c) 2014 Intel Corporation.
> - */
> -
> -#ifndef GPIO_DW_APB_H
> -#define GPIO_DW_APB_H
> -
> -#define DWAPB_MAX_GPIOS		32
> -
> -struct dwapb_port_property {
> -	struct fwnode_handle *fwnode;
> -	unsigned int	idx;
> -	unsigned int	ngpio;
> -	unsigned int	gpio_base;
> -	int		irq[DWAPB_MAX_GPIOS];
> -};
> -
> -struct dwapb_platform_data {
> -	struct dwapb_port_property *properties;
> -	unsigned int nports;
> -};
> -
> -#endif
> -- 
> 2.30.2
> 

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

[Andy Shevchenko]

On Mon, Aug 2, 2021 at 5:23 PM Serge Semin <fancer.lancer@gmail.com> wrote:
> On Mon, Jul 26, 2021 at 03:54:36PM +0300, Andy Shevchenko 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.


> > +struct dwapb_port_property {
> > +     struct fwnode_handle *fwnode;
> > +     unsigned int idx;
> > +     unsigned int ngpio;
> > +     unsigned int gpio_base;
> > +     int irq[DWAPB_MAX_GPIOS];
> > +};
> > +
> > +struct dwapb_platform_data {
> > +     struct dwapb_port_property *properties;
> > +     unsigned int nports;
> > +};
> > +
> >  struct dwapb_gpio;
>
> If you need to resend the series anyway could you please move the
> structures declarations to being below the forward declaration of the
> dwapb_gpio structure? Of course it's not that critical, but for the
> sake of just not to have the later one left somewhere in the middle of
> the unrelated structures and for at least to keep some order in the
> declarations.

Fine with me, I'll modify accordingly in the next version, thanks for
the review!

> Then feel free to add:
> Acked-by: Serge Semin <fancer.lancer@gmail.com>
>
> The whole series has been tested on Baikal-T1 SoC:
> Tested-by: Serge Semin <fancer.lancer@gmail.com>

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 1/4] gpio: dwapb: Unify ACPI enumeration checks in get_irq() and configure_irqs()

[Lee Jones]

On Mon, 26 Jul 2021, Andy Shevchenko wrote:

> Shared IRQ is only enabled for ACPI enumeration, there is no need
> to have a special flag for that, since we simple can test if device
> has been enumerated by ACPI. This unifies the checks in dwapb_get_irq()
> and dwapb_configure_irqs().
> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>  drivers/gpio/gpio-dwapb.c                | 13 ++++++-------

>  drivers/mfd/intel_quark_i2c_gpio.c       |  1 -

Acked-by: Lee Jones <lee.jones@linaro.org>

>  include/linux/platform_data/gpio-dwapb.h |  1 -
>  3 files changed, 6 insertions(+), 9 deletions(-)

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

Re: [PATCH v1 1/4] gpio: dwapb: Unify ACPI enumeration checks in get_irq() and configure_irqs()

[Serge Semin]

Hello Andy
Thanks for the cleanup series. A tiny note is below.

On Mon, Jul 26, 2021 at 03:54:33PM +0300, Andy Shevchenko wrote:
> Shared IRQ is only enabled for ACPI enumeration, there is no need
> to have a special flag for that, since we simple can test if device
> has been enumerated by ACPI. This unifies the checks in dwapb_get_irq()
> and dwapb_configure_irqs().
> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>  drivers/gpio/gpio-dwapb.c                | 13 ++++++-------
>  drivers/mfd/intel_quark_i2c_gpio.c       |  1 -
>  include/linux/platform_data/gpio-dwapb.h |  1 -
>  3 files changed, 6 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/gpio/gpio-dwapb.c b/drivers/gpio/gpio-dwapb.c
> index 3eb13d6d31ef..f6ae69d5d644 100644
> --- a/drivers/gpio/gpio-dwapb.c
> +++ b/drivers/gpio/gpio-dwapb.c
> @@ -436,12 +436,7 @@ static void dwapb_configure_irqs(struct dwapb_gpio *gpio,
>  	pirq->irqchip.irq_set_wake = dwapb_irq_set_wake;
>  #endif
>  

> -	if (!pp->irq_shared) {
> -		girq->num_parents = pirq->nr_irqs;
> -		girq->parents = pirq->irq;
> -		girq->parent_handler_data = gpio;
> -		girq->parent_handler = dwapb_irq_handler;
> -	} else {
> +	if (has_acpi_companion(gpio->dev)) {

Before this patch the platform flag irq_shared has been as kind of a
hint regarding the shared IRQ case being covered here. But now it
doesn't seem obvious why we've got the ACPI and ACPI-less cases
differently handled. What about adding a small comment about that?
E.g. like this: "Intel ACPI-based platforms mostly have the DW APB
GPIO IRQ lane shared between several devices. In that case the
parental IRQ has to be handled in the shared way so to be properly
delivered to all the connected devices." or something more detailed
for your preference. After that the rest of the comments in the
if-clause could be discarded.

Other than that, feel free to add:
>  drivers/gpio/gpio-dwapb.c                | 13 ++++++-------
Acked-by: Serge Semin <fancer.lancer@gmail.com>
Tested-by: Serge Semin <fancer.lancer@gmail.com>

-Sergey

>  		/* This will let us handle the parent IRQ in the driver */
>  		girq->num_parents = 0;
>  		girq->parents = NULL;
> @@ -458,6 +453,11 @@ static void dwapb_configure_irqs(struct dwapb_gpio *gpio,
>  			dev_err(gpio->dev, "error requesting IRQ\n");
>  			goto err_kfree_pirq;
>  		}
> +	} else {
> +		girq->num_parents = pirq->nr_irqs;
> +		girq->parents = pirq->irq;
> +		girq->parent_handler_data = gpio;
> +		girq->parent_handler = dwapb_irq_handler;
>  	}
>  
>  	girq->chip = &pirq->irqchip;
> @@ -581,7 +581,6 @@ static struct dwapb_platform_data *dwapb_gpio_get_pdata(struct device *dev)
>  			pp->ngpio = DWAPB_MAX_GPIOS;
>  		}
>  
> -		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 01935ae4e9e1..a43993e38b6e 100644
> --- a/drivers/mfd/intel_quark_i2c_gpio.c
> +++ b/drivers/mfd/intel_quark_i2c_gpio.c
> @@ -227,7 +227,6 @@ static int intel_quark_gpio_setup(struct pci_dev *pdev)
>  	pdata->properties->ngpio	= INTEL_QUARK_MFD_NGPIO;
>  	pdata->properties->gpio_base	= INTEL_QUARK_MFD_GPIO_BASE;
>  	pdata->properties->irq[0]	= pci_irq_vector(pdev, 0);
> -	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 0aa5c6720259..535e5ed549d9 100644
> --- a/include/linux/platform_data/gpio-dwapb.h
> +++ b/include/linux/platform_data/gpio-dwapb.h
> @@ -14,7 +14,6 @@ struct dwapb_port_property {
>  	unsigned int	ngpio;
>  	unsigned int	gpio_base;
>  	int		irq[DWAPB_MAX_GPIOS];
> -	bool		irq_shared;
>  };
>  
>  struct dwapb_platform_data {
> -- 
> 2.30.2
> 

Re: [PATCH v1 1/4] gpio: dwapb: Unify ACPI enumeration checks in get_irq() and configure_irqs()

[Andy Shevchenko]

On Mon, Aug 02, 2021 at 04:40:21PM +0300, Serge Semin wrote:
> Hello Andy
> Thanks for the cleanup series. A tiny note is below.

Thanks for review!

> On Mon, Jul 26, 2021 at 03:54:33PM +0300, Andy Shevchenko wrote:
> > Shared IRQ is only enabled for ACPI enumeration, there is no need
> > to have a special flag for that, since we simple can test if device
> > has been enumerated by ACPI. This unifies the checks in dwapb_get_irq()
> > and dwapb_configure_irqs().
> > 
> > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > ---
> >  drivers/gpio/gpio-dwapb.c                | 13 ++++++-------
> >  drivers/mfd/intel_quark_i2c_gpio.c       |  1 -
> >  include/linux/platform_data/gpio-dwapb.h |  1 -
> >  3 files changed, 6 insertions(+), 9 deletions(-)
> > 
> > diff --git a/drivers/gpio/gpio-dwapb.c b/drivers/gpio/gpio-dwapb.c
> > index 3eb13d6d31ef..f6ae69d5d644 100644
> > --- a/drivers/gpio/gpio-dwapb.c
> > +++ b/drivers/gpio/gpio-dwapb.c
> > @@ -436,12 +436,7 @@ static void dwapb_configure_irqs(struct dwapb_gpio *gpio,
> >  	pirq->irqchip.irq_set_wake = dwapb_irq_set_wake;
> >  #endif
> >  
> 
> > -	if (!pp->irq_shared) {
> > -		girq->num_parents = pirq->nr_irqs;
> > -		girq->parents = pirq->irq;
> > -		girq->parent_handler_data = gpio;
> > -		girq->parent_handler = dwapb_irq_handler;
> > -	} else {
> > +	if (has_acpi_companion(gpio->dev)) {
> 
> Before this patch the platform flag irq_shared has been as kind of a
> hint regarding the shared IRQ case being covered here. But now it
> doesn't seem obvious why we've got the ACPI and ACPI-less cases
> differently handled. What about adding a small comment about that?
> E.g. like this: "Intel ACPI-based platforms mostly have the DW APB
> GPIO IRQ lane shared between several devices. In that case the
> parental IRQ has to be handled in the shared way so to be properly
> delivered to all the connected devices." or something more detailed
> for your preference. After that the rest of the comments in the
> if-clause could be discarded.

Sure!


-- 
With Best Regards,
Andy Shevchenko