[PATCH v1 0/2] Introduce Intel Elkhart Lake PSE I/O

[PATCH v1 0/2] Introduce Intel Elkhart Lake PSE I/O

[Raag Jadav]

This series adds Intel Elkhart Lake PSE I/O driver which enumerates the
PCI parent device and splits two child I/O devices (GPIO and Timed I/O
which are available as a single PCI function through shared MMIO) to their
respective I/O drivers.

In spirit, it is a continuation of PSE TIO series[1] which received
objection from Greg for abusing platform device and has now been reworked
to use auxiliary device instead.

Currently TIO driver[2] falls under PPS subsystem supporting generator
functionality and will be coming up in a separate follow-up series for
its independent design changes as per below roadmap.

=> Extend TIO driver[2] to support PPS client functionality.
=> Develop a PPS common driver which hooks to both generator and client
   counterparts.
=> Develop an auxiliary glue driver for PPS common driver.

[1] https://lore.kernel.org/r/20250307052231.551737-1-raag.jadav@intel.com
[2] https://lore.kernel.org/r/20250219040618.70962-1-subramanian.mohan@intel.com

Raag Jadav (2):
  platform/x86/intel: Introduce Intel Elkhart Lake PSE I/O
  gpio: elkhartlake: Convert to auxiliary driver

 MAINTAINERS                             |   7 ++
 drivers/gpio/Kconfig                    |   2 +-
 drivers/gpio/gpio-elkhartlake.c         |  34 +++----
 drivers/platform/x86/intel/Kconfig      |  13 +++
 drivers/platform/x86/intel/Makefile     |   1 +
 drivers/platform/x86/intel/ehl_pse_io.c | 128 ++++++++++++++++++++++++
 include/linux/ehl_pse_io_aux.h          |  30 ++++++
 7 files changed, 196 insertions(+), 19 deletions(-)
 create mode 100644 drivers/platform/x86/intel/ehl_pse_io.c
 create mode 100644 include/linux/ehl_pse_io_aux.h

-- 
2.34.1

[PATCH v1 1/2] platform/x86/intel: Introduce Intel Elkhart Lake PSE I/O

[Raag Jadav]

Intel Elkhart Lake Programmable Service Engine (PSE) includes two PCI
devices that expose two different capabilities of GPIO and Timed I/O
as a single PCI function through shared MMIO with below layout.

GPIO: 0x0000 - 0x1000
TIO:  0x1000 - 0x2000

This driver enumerates the PCI parent device and creates auxiliary child
devices for these capabilities. The actual functionalities are provided
by their respective auxiliary drivers.

Signed-off-by: Raag Jadav <raag.jadav@intel.com>
---
 MAINTAINERS                             |   7 ++
 drivers/platform/x86/intel/Kconfig      |  13 +++
 drivers/platform/x86/intel/Makefile     |   1 +
 drivers/platform/x86/intel/ehl_pse_io.c | 128 ++++++++++++++++++++++++
 include/linux/ehl_pse_io_aux.h          |  30 ++++++
 5 files changed, 179 insertions(+)
 create mode 100644 drivers/platform/x86/intel/ehl_pse_io.c
 create mode 100644 include/linux/ehl_pse_io_aux.h

diff --git a/MAINTAINERS b/MAINTAINERS
index 46126ce2f968..bd2a009d73c6 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -12499,6 +12499,13 @@ F:	drivers/gpu/drm/xe/
 F:	include/drm/intel/
 F:	include/uapi/drm/xe_drm.h
 
+INTEL ELKHART LAKE PSE I/O DRIVER
+M:	Raag Jadav <raag.jadav@intel.com>
+L:	platform-driver-x86@vger.kernel.org
+S:	Supported
+F:	drivers/platform/x86/intel/ehl_pse_io.c
+F:	include/linux/ehl_pse_io_aux.h
+
 INTEL ETHERNET DRIVERS
 M:	Tony Nguyen <anthony.l.nguyen@intel.com>
 M:	Przemek Kitszel <przemyslaw.kitszel@intel.com>
diff --git a/drivers/platform/x86/intel/Kconfig b/drivers/platform/x86/intel/Kconfig
index 19a2246f2770..2900407d6095 100644
--- a/drivers/platform/x86/intel/Kconfig
+++ b/drivers/platform/x86/intel/Kconfig
@@ -41,6 +41,19 @@ config INTEL_VBTN
 	  To compile this driver as a module, choose M here: the module will
 	  be called intel_vbtn.
 
+config INTEL_EHL_PSE_IO
+	tristate "Intel Elkhart Lake PSE I/O driver"
+	depends on PCI
+	select AUXILIARY_BUS
+	help
+	  Select this option to enable Intel Elkhart Lake PSE GPIO and Timed
+	  I/O support. This driver enumerates the PCI parent device and
+	  creates auxiliary child devices for these capabilities. The actual
+	  functionalities are provided by their respective auxiliary drivers.
+
+	  To compile this driver as a module, choose M here: the module will
+	  be called intel_ehl_pse_io.
+
 config INTEL_INT0002_VGPIO
 	tristate "Intel ACPI INT0002 Virtual GPIO driver"
 	depends on GPIOLIB && ACPI && PM_SLEEP
diff --git a/drivers/platform/x86/intel/Makefile b/drivers/platform/x86/intel/Makefile
index 78acb414e154..138b13756158 100644
--- a/drivers/platform/x86/intel/Makefile
+++ b/drivers/platform/x86/intel/Makefile
@@ -21,6 +21,7 @@ intel-target-$(CONFIG_INTEL_HID_EVENT)		+= hid.o
 intel-target-$(CONFIG_INTEL_VBTN)		+= vbtn.o
 
 # Intel miscellaneous drivers
+intel-target-$(CONFIG_INTEL_EHL_PSE_IO)		+= ehl_pse_io.o
 intel-target-$(CONFIG_INTEL_INT0002_VGPIO)	+= int0002_vgpio.o
 intel-target-$(CONFIG_INTEL_ISHTP_ECLITE)	+= ishtp_eclite.o
 intel-target-$(CONFIG_INTEL_OAKTRAIL)		+= oaktrail.o
diff --git a/drivers/platform/x86/intel/ehl_pse_io.c b/drivers/platform/x86/intel/ehl_pse_io.c
new file mode 100644
index 000000000000..f1cad102f856
--- /dev/null
+++ b/drivers/platform/x86/intel/ehl_pse_io.c
@@ -0,0 +1,128 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Intel Elkhart Lake Programmable Service Engine (PSE) I/O
+ *
+ * Copyright (c) 2025 Intel Corporation.
+ *
+ * Author: Raag Jadav <raag.jadav@intel.com>
+ */
+
+#include <linux/auxiliary_bus.h>
+#include <linux/dev_printk.h>
+#include <linux/device/devres.h>
+#include <linux/device.h>
+#include <linux/err.h>
+#include <linux/gfp_types.h>
+#include <linux/mod_devicetable.h>
+#include <linux/module.h>
+#include <linux/pci.h>
+#include <linux/sizes.h>
+#include <linux/slab.h>
+#include <linux/types.h>
+
+#include <linux/ehl_pse_io_aux.h>
+
+#define EHL_PSE_IO_DEV_OFFSET	SZ_4K
+#define EHL_PSE_IO_DEV_SIZE	SZ_4K
+
+static void ehl_pse_io_dev_release(struct device *dev)
+{
+	struct auxiliary_device *aux_dev = to_auxiliary_dev(dev);
+	struct ehl_pse_io_dev *io_dev = auxiliary_dev_to_ehl_pse_io_dev(aux_dev);
+
+	kfree(io_dev);
+}
+
+static int ehl_pse_io_dev_add(struct pci_dev *pci, const char *name, int idx)
+{
+	struct auxiliary_device *aux_dev;
+	struct device *dev = &pci->dev;
+	struct ehl_pse_io_dev *io_dev;
+	resource_size_t start;
+	int ret;
+
+	io_dev = kzalloc(sizeof(*io_dev), GFP_KERNEL);
+	if (!io_dev)
+		return -ENOMEM;
+
+	start = pci_resource_start(pci, 0);
+	io_dev->irq = pci_irq_vector(pci, idx);
+	io_dev->mem = DEFINE_RES_MEM(start + (EHL_PSE_IO_DEV_OFFSET * idx), EHL_PSE_IO_DEV_SIZE);
+
+	aux_dev = &io_dev->aux_dev;
+	aux_dev->name = name;
+	aux_dev->id = (pci_domain_nr(pci->bus) << 16) | pci_dev_id(pci);
+	aux_dev->dev.parent = dev;
+	aux_dev->dev.release = ehl_pse_io_dev_release;
+
+	ret = auxiliary_device_init(aux_dev);
+	if (ret)
+		goto free_io_dev;
+
+	ret = __auxiliary_device_add(aux_dev, dev->driver->name);
+	if (ret)
+		goto uninit_aux_dev;
+
+	return 0;
+
+uninit_aux_dev:
+	/* io_dev will be freed with the put_device() and .release sequence */
+	auxiliary_device_uninit(aux_dev);
+free_io_dev:
+	kfree(io_dev);
+	return ret;
+}
+
+static int ehl_pse_io_probe(struct pci_dev *pci, const struct pci_device_id *id)
+{
+	int ret;
+
+	ret = pcim_enable_device(pci);
+	if (ret)
+		return ret;
+
+	pci_set_master(pci);
+
+	ret = pci_alloc_irq_vectors(pci, 2, 2, PCI_IRQ_MSI);
+	if (ret < 0)
+		return ret;
+
+	ret = ehl_pse_io_dev_add(pci, EHL_PSE_GPIO_NAME, 0);
+	if (ret)
+		return ret;
+
+	return ehl_pse_io_dev_add(pci, EHL_PSE_TIO_NAME, 1);
+}
+
+static int ehl_pse_io_dev_destroy(struct device *dev, void *data)
+{
+	auxiliary_device_destroy(to_auxiliary_dev(dev));
+
+	return 0;
+}
+
+static void ehl_pse_io_remove(struct pci_dev *pci)
+{
+	struct device *dev = &pci->dev;
+
+	device_for_each_child_reverse(dev, NULL, ehl_pse_io_dev_destroy);
+}
+
+static const struct pci_device_id ehl_pse_io_ids[] = {
+	{ PCI_VDEVICE(INTEL, 0x4b88) },
+	{ PCI_VDEVICE(INTEL, 0x4b89) },
+	{ }
+};
+MODULE_DEVICE_TABLE(pci, ehl_pse_io_ids);
+
+static struct pci_driver ehl_pse_io_driver = {
+	.name		= EHL_PSE_IO_NAME,
+	.id_table	= ehl_pse_io_ids,
+	.probe		= ehl_pse_io_probe,
+	.remove		= ehl_pse_io_remove,
+};
+module_pci_driver(ehl_pse_io_driver);
+
+MODULE_AUTHOR("Raag Jadav <raag.jadav@intel.com>");
+MODULE_DESCRIPTION("Intel Elkhart Lake PSE I/O driver");
+MODULE_LICENSE("GPL");
diff --git a/include/linux/ehl_pse_io_aux.h b/include/linux/ehl_pse_io_aux.h
new file mode 100644
index 000000000000..33eb5a86ce36
--- /dev/null
+++ b/include/linux/ehl_pse_io_aux.h
@@ -0,0 +1,30 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+/*
+ * Intel Elkhart Lake PSE I/O Auxiliary Device
+ *
+ * Copyright (c) 2025 Intel Corporation.
+ *
+ * Author: Raag Jadav <raag.jadav@intel.com>
+ */
+
+#ifndef _EHL_PSE_IO_AUX_H_
+#define _EHL_PSE_IO_AUX_H_
+
+#include <linux/auxiliary_bus.h>
+#include <linux/container_of.h>
+#include <linux/ioport.h>
+
+#define EHL_PSE_IO_NAME		"ehl-pse-io"
+#define EHL_PSE_GPIO_NAME	"gpio-elkhartlake"
+#define EHL_PSE_TIO_NAME	"pps-tio"
+
+struct ehl_pse_io_dev {
+	struct auxiliary_device aux_dev;
+	struct resource mem;
+	int irq;
+};
+
+#define auxiliary_dev_to_ehl_pse_io_dev(auxiliary_dev) \
+	container_of(auxiliary_dev, struct ehl_pse_io_dev, aux_dev)
+
+#endif /* _EHL_PSE_IO_AUX_H_ */
-- 
2.34.1

[PATCH v1 2/2] gpio: elkhartlake: Convert to auxiliary driver

[Raag Jadav]

Since PCI device should not be abusing platform device, MFD parent to
platform child path is no longer being pursued for this driver. Convert
it to auxiliary driver, which will be used by EHL PSE auxiliary device.

Signed-off-by: Raag Jadav <raag.jadav@intel.com>
---
 drivers/gpio/Kconfig            |  2 +-
 drivers/gpio/gpio-elkhartlake.c | 34 ++++++++++++++++-----------------
 2 files changed, 17 insertions(+), 19 deletions(-)

diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
index 7ee3afbc2b05..d4b4451b4696 100644
--- a/drivers/gpio/Kconfig
+++ b/drivers/gpio/Kconfig
@@ -1413,7 +1413,7 @@ config HTC_EGPIO
 
 config GPIO_ELKHARTLAKE
 	tristate "Intel Elkhart Lake PSE GPIO support"
-	depends on X86 || COMPILE_TEST
+	depends on INTEL_EHL_PSE_IO
 	select GPIO_TANGIER
 	help
 	  Select this option to enable GPIO support for Intel Elkhart Lake
diff --git a/drivers/gpio/gpio-elkhartlake.c b/drivers/gpio/gpio-elkhartlake.c
index 95de52d2cc63..08daf2fc59e6 100644
--- a/drivers/gpio/gpio-elkhartlake.c
+++ b/drivers/gpio/gpio-elkhartlake.c
@@ -2,43 +2,42 @@
 /*
  * Intel Elkhart Lake PSE GPIO driver
  *
- * Copyright (c) 2023 Intel Corporation.
+ * Copyright (c) 2023, 2025 Intel Corporation.
  *
  * Authors: Pandith N <pandith.n@intel.com>
  *          Raag Jadav <raag.jadav@intel.com>
  */
 
+#include <linux/auxiliary_bus.h>
 #include <linux/device.h>
 #include <linux/err.h>
 #include <linux/module.h>
-#include <linux/platform_device.h>
 #include <linux/pm.h>
 
+#include <linux/ehl_pse_io_aux.h>
+
 #include "gpio-tangier.h"
 
 /* Each Intel EHL PSE GPIO Controller has 30 GPIO pins */
 #define EHL_PSE_NGPIO		30
 
-static int ehl_gpio_probe(struct platform_device *pdev)
+static int ehl_gpio_probe(struct auxiliary_device *aux_dev, const struct auxiliary_device_id *id)
 {
-	struct device *dev = &pdev->dev;
+	struct ehl_pse_io_dev *io_dev = auxiliary_dev_to_ehl_pse_io_dev(aux_dev);
+	struct device *dev = &aux_dev->dev;
 	struct tng_gpio *priv;
-	int irq, ret;
-
-	irq = platform_get_irq(pdev, 0);
-	if (irq < 0)
-		return irq;
+	int ret;
 
 	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
 	if (!priv)
 		return -ENOMEM;
 
-	priv->reg_base = devm_platform_ioremap_resource(pdev, 0);
+	priv->reg_base = devm_ioremap_resource(dev, &io_dev->mem);
 	if (IS_ERR(priv->reg_base))
 		return PTR_ERR(priv->reg_base);
 
 	priv->dev = dev;
-	priv->irq = irq;
+	priv->irq = io_dev->irq;
 
 	priv->info.base = -1;
 	priv->info.ngpio = EHL_PSE_NGPIO;
@@ -51,25 +50,24 @@ static int ehl_gpio_probe(struct platform_device *pdev)
 	if (ret)
 		return dev_err_probe(dev, ret, "tng_gpio_probe error\n");
 
-	platform_set_drvdata(pdev, priv);
+	auxiliary_set_drvdata(aux_dev, priv);
 	return 0;
 }
 
-static const struct platform_device_id ehl_gpio_ids[] = {
-	{ "gpio-elkhartlake" },
+static const struct auxiliary_device_id ehl_gpio_ids[] = {
+	{ EHL_PSE_IO_NAME "." EHL_PSE_GPIO_NAME },
 	{ }
 };
-MODULE_DEVICE_TABLE(platform, ehl_gpio_ids);
+MODULE_DEVICE_TABLE(auxiliary, ehl_gpio_ids);
 
-static struct platform_driver ehl_gpio_driver = {
+static struct auxiliary_driver ehl_gpio_driver = {
 	.driver	= {
-		.name	= "gpio-elkhartlake",
 		.pm	= pm_sleep_ptr(&tng_gpio_pm_ops),
 	},
 	.probe		= ehl_gpio_probe,
 	.id_table	= ehl_gpio_ids,
 };
-module_platform_driver(ehl_gpio_driver);
+module_auxiliary_driver(ehl_gpio_driver);
 
 MODULE_AUTHOR("Pandith N <pandith.n@intel.com>");
 MODULE_AUTHOR("Raag Jadav <raag.jadav@intel.com>");
-- 
2.34.1

Re: [PATCH v1 1/2] platform/x86/intel: Introduce Intel Elkhart Lake PSE I/O

[Andy Shevchenko]

On Wed, Oct 29, 2025 at 11:50:49AM +0530, Raag Jadav wrote:
> Intel Elkhart Lake Programmable Service Engine (PSE) includes two PCI
> devices that expose two different capabilities of GPIO and Timed I/O
> as a single PCI function through shared MMIO with below layout.
> 
> GPIO: 0x0000 - 0x1000
> TIO:  0x1000 - 0x2000
> 
> This driver enumerates the PCI parent device and creates auxiliary child
> devices for these capabilities. The actual functionalities are provided
> by their respective auxiliary drivers.

...

> +#include <linux/auxiliary_bus.h>
> +#include <linux/dev_printk.h>
> +#include <linux/device/devres.h>
> +#include <linux/device.h>
> +#include <linux/err.h>
> +#include <linux/gfp_types.h>
> +#include <linux/mod_devicetable.h>
> +#include <linux/module.h>
> +#include <linux/pci.h>
> +#include <linux/sizes.h>
> +#include <linux/slab.h>
> +#include <linux/types.h>

> +#define EHL_PSE_IO_DEV_OFFSET	SZ_4K
> +#define EHL_PSE_IO_DEV_SIZE	SZ_4K

Not sure if SZ_4K is a good idea for the _OFFSET, the _SIZE is fine. Also why
do we need two? If the devices are of the same size, we don't need to have a
separate offset.

...

> +static int ehl_pse_io_dev_add(struct pci_dev *pci, const char *name, int idx)
> +{
> +	struct auxiliary_device *aux_dev;
> +	struct device *dev = &pci->dev;
> +	struct ehl_pse_io_dev *io_dev;
> +	resource_size_t start;
> +	int ret;
> +
> +	io_dev = kzalloc(sizeof(*io_dev), GFP_KERNEL);
> +	if (!io_dev)
> +		return -ENOMEM;

Why devm_kzalloc() can't be used? I don't see if the device lifetime is anyhow
different to this object. Am I wrong?

> +	start = pci_resource_start(pci, 0);
> +	io_dev->irq = pci_irq_vector(pci, idx);
> +	io_dev->mem = DEFINE_RES_MEM(start + (EHL_PSE_IO_DEV_OFFSET * idx), EHL_PSE_IO_DEV_SIZE);
> +
> +	aux_dev = &io_dev->aux_dev;
> +	aux_dev->name = name;
> +	aux_dev->id = (pci_domain_nr(pci->bus) << 16) | pci_dev_id(pci);
> +	aux_dev->dev.parent = dev;
> +	aux_dev->dev.release = ehl_pse_io_dev_release;
> +
> +	ret = auxiliary_device_init(aux_dev);
> +	if (ret)
> +		goto free_io_dev;
> +
> +	ret = __auxiliary_device_add(aux_dev, dev->driver->name);

Hmm... Is it okay to use double underscored variant? Only a single driver uses
this so far... Care to elaborate?

> +	if (ret)
> +		goto uninit_aux_dev;
> +
> +	return 0;
> +
> +uninit_aux_dev:
> +	/* io_dev will be freed with the put_device() and .release sequence */

Right...

> +	auxiliary_device_uninit(aux_dev);
> +free_io_dev:
> +	kfree(io_dev);

...and this is a double free, correct?

> +	return ret;
> +}

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 2/2] gpio: elkhartlake: Convert to auxiliary driver

[Andy Shevchenko]

On Wed, Oct 29, 2025 at 11:50:50AM +0530, Raag Jadav wrote:
> Since PCI device should not be abusing platform device, MFD parent to
> platform child path is no longer being pursued for this driver. Convert
> it to auxiliary driver, which will be used by EHL PSE auxiliary device.

This looks fine,
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

I believe the idea is to push this via PDx86 tree.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 2/2] gpio: elkhartlake: Convert to auxiliary driver

[Bartosz Golaszewski]

On Wed, Oct 29, 2025 at 7:21 AM Raag Jadav <raag.jadav@intel.com> wrote:
>
> Since PCI device should not be abusing platform device, MFD parent to
> platform child path is no longer being pursued for this driver. Convert
> it to auxiliary driver, which will be used by EHL PSE auxiliary device.
>
> Signed-off-by: Raag Jadav <raag.jadav@intel.com>
> ---

Are there build-time dependencies between this and patch 1/2?

Bart

Re: [PATCH v1 2/2] gpio: elkhartlake: Convert to auxiliary driver

[Andy Shevchenko]

On Wed, Oct 29, 2025 at 11:40:17AM +0100, Bartosz Golaszewski wrote:
> On Wed, Oct 29, 2025 at 7:21 AM Raag Jadav <raag.jadav@intel.com> wrote:
> >
> > Since PCI device should not be abusing platform device, MFD parent to
> > platform child path is no longer being pursued for this driver. Convert
> > it to auxiliary driver, which will be used by EHL PSE auxiliary device.
> >
> Are there build-time dependencies between this and patch 1/2?

There is a new header file which is shared IIUC. So, yes.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 2/2] gpio: elkhartlake: Convert to auxiliary driver

[Bartosz Golaszewski]

On Wed, Oct 29, 2025 at 7:21 AM Raag Jadav <raag.jadav@intel.com> wrote:
>
> Since PCI device should not be abusing platform device, MFD parent to
> platform child path is no longer being pursued for this driver. Convert
> it to auxiliary driver, which will be used by EHL PSE auxiliary device.
>
> Signed-off-by: Raag Jadav <raag.jadav@intel.com>
> ---

Acked-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

Re: [PATCH v1 1/2] platform/x86/intel: Introduce Intel Elkhart Lake PSE I/O

[Raag Jadav]

On Wed, Oct 29, 2025 at 10:36:19AM +0200, Andy Shevchenko wrote:
> On Wed, Oct 29, 2025 at 11:50:49AM +0530, Raag Jadav wrote:
> > Intel Elkhart Lake Programmable Service Engine (PSE) includes two PCI
> > devices that expose two different capabilities of GPIO and Timed I/O
> > as a single PCI function through shared MMIO with below layout.
> > 
> > GPIO: 0x0000 - 0x1000
> > TIO:  0x1000 - 0x2000
> > 
> > This driver enumerates the PCI parent device and creates auxiliary child
> > devices for these capabilities. The actual functionalities are provided
> > by their respective auxiliary drivers.
> 
> ...
> 
> > +#include <linux/auxiliary_bus.h>
> > +#include <linux/dev_printk.h>
> > +#include <linux/device/devres.h>
> > +#include <linux/device.h>
> > +#include <linux/err.h>
> > +#include <linux/gfp_types.h>
> > +#include <linux/mod_devicetable.h>
> > +#include <linux/module.h>
> > +#include <linux/pci.h>
> > +#include <linux/sizes.h>
> > +#include <linux/slab.h>
> > +#include <linux/types.h>
> 
> > +#define EHL_PSE_IO_DEV_OFFSET	SZ_4K
> > +#define EHL_PSE_IO_DEV_SIZE	SZ_4K
> 
> Not sure if SZ_4K is a good idea for the _OFFSET, the _SIZE is fine. Also why
> do we need two? If the devices are of the same size, we don't need to have a
> separate offset.

Yes but they're semantically different, atleast as per DEFINE_RES_MEM().
Either way works for me.

...

> > +static int ehl_pse_io_dev_add(struct pci_dev *pci, const char *name, int idx)
> > +{
> > +	struct auxiliary_device *aux_dev;
> > +	struct device *dev = &pci->dev;
> > +	struct ehl_pse_io_dev *io_dev;
> > +	resource_size_t start;
> > +	int ret;
> > +
> > +	io_dev = kzalloc(sizeof(*io_dev), GFP_KERNEL);
> > +	if (!io_dev)
> > +		return -ENOMEM;
> 
> Why devm_kzalloc() can't be used? I don't see if the device lifetime is anyhow
> different to this object. Am I wrong?

Looks like it but I don't know the code well enough to tell if there're
corner cases, so just following the documented rules. Your call.

> > +	start = pci_resource_start(pci, 0);
> > +	io_dev->irq = pci_irq_vector(pci, idx);
> > +	io_dev->mem = DEFINE_RES_MEM(start + (EHL_PSE_IO_DEV_OFFSET * idx), EHL_PSE_IO_DEV_SIZE);
> > +
> > +	aux_dev = &io_dev->aux_dev;
> > +	aux_dev->name = name;
> > +	aux_dev->id = (pci_domain_nr(pci->bus) << 16) | pci_dev_id(pci);
> > +	aux_dev->dev.parent = dev;
> > +	aux_dev->dev.release = ehl_pse_io_dev_release;
> > +
> > +	ret = auxiliary_device_init(aux_dev);
> > +	if (ret)
> > +		goto free_io_dev;
> > +
> > +	ret = __auxiliary_device_add(aux_dev, dev->driver->name);
> 
> Hmm... Is it okay to use double underscored variant? Only a single driver uses
> this so far... Care to elaborate?

The regular variant uses KBUILD_MODNAME which comes with 'intel' prefix
after commit df7f9acd8646, and with that we overshoot the max id string
length for leaf drivers.

> > +	if (ret)
> > +		goto uninit_aux_dev;
> > +
> > +	return 0;
> > +
> > +uninit_aux_dev:
> > +	/* io_dev will be freed with the put_device() and .release sequence */
> 
> Right...
> 
> > +	auxiliary_device_uninit(aux_dev);
> > +free_io_dev:
> > +	kfree(io_dev);
> 
> ...and this is a double free, correct?

Yeah, my sheer incompetence at stealing code :(

Raag

Re: [PATCH v1 1/2] platform/x86/intel: Introduce Intel Elkhart Lake PSE I/O

[Andy Shevchenko]

On Fri, Oct 31, 2025 at 10:34:28AM +0100, Raag Jadav wrote:
> On Wed, Oct 29, 2025 at 10:36:19AM +0200, Andy Shevchenko wrote:
> > On Wed, Oct 29, 2025 at 11:50:49AM +0530, Raag Jadav wrote:

...

> > > +#define EHL_PSE_IO_DEV_OFFSET	SZ_4K
> > > +#define EHL_PSE_IO_DEV_SIZE	SZ_4K
> > 
> > Not sure if SZ_4K is a good idea for the _OFFSET, the _SIZE is fine. Also why
> > do we need two? If the devices are of the same size, we don't need to have a
> > separate offset.
> 
> Yes but they're semantically different, atleast as per DEFINE_RES_MEM().
> Either way works for me.

They are "slices" in the HW, see also my "if the devices..." passage.

If you want to use SZ_* in _OFFSET, I would write it as (1 * SZ_4K) to point
out that size constant here is the _unit_ and not the size semantically.
Currently the definitions have the same values semantically, but you pointed
out that they should not be.

...

> > > +	io_dev = kzalloc(sizeof(*io_dev), GFP_KERNEL);
> > > +	if (!io_dev)
> > > +		return -ENOMEM;
> > 
> > Why devm_kzalloc() can't be used? I don't see if the device lifetime is anyhow
> > different to this object. Am I wrong?
> 
> Looks like it but I don't know the code well enough to tell if there're
> corner cases, so just following the documented rules. Your call.

Do you expect this to be called in non-probe() contexts? If no --> devm.
Otherwise some comments are needed.

...

> > > +	ret = __auxiliary_device_add(aux_dev, dev->driver->name);
> > 
> > Hmm... Is it okay to use double underscored variant? Only a single driver uses
> > this so far... Care to elaborate?
> 
> The regular variant uses KBUILD_MODNAME which comes with 'intel' prefix
> after commit df7f9acd8646, and with that we overshoot the max id string
> length for leaf drivers.

At bare minimum this needs a comment, but I think ideally we need to bump the
limit by factor of 2.

> > > +	if (ret)
> > > +		goto uninit_aux_dev;

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 1/2] platform/x86/intel: Introduce Intel Elkhart Lake PSE I/O

[Raag Jadav]

On Fri, Oct 31, 2025 at 12:02:14PM +0200, Andy Shevchenko wrote:
> On Fri, Oct 31, 2025 at 10:34:28AM +0100, Raag Jadav wrote:
> > On Wed, Oct 29, 2025 at 10:36:19AM +0200, Andy Shevchenko wrote:
> > > On Wed, Oct 29, 2025 at 11:50:49AM +0530, Raag Jadav wrote:
> 
> ...
> 
> > > > +#define EHL_PSE_IO_DEV_OFFSET	SZ_4K
> > > > +#define EHL_PSE_IO_DEV_SIZE	SZ_4K
> > > 
> > > Not sure if SZ_4K is a good idea for the _OFFSET, the _SIZE is fine. Also why
> > > do we need two? If the devices are of the same size, we don't need to have a
> > > separate offset.
> > 
> > Yes but they're semantically different, atleast as per DEFINE_RES_MEM().
> > Either way works for me.
> 
> They are "slices" in the HW, see also my "if the devices..." passage.
> 
> If you want to use SZ_* in _OFFSET, I would write it as (1 * SZ_4K) to point
> out that size constant here is the _unit_ and not the size semantically.
> Currently the definitions have the same values semantically, but you pointed
> out that they should not be.

Fair. Will consolidate.

> > > > +	io_dev = kzalloc(sizeof(*io_dev), GFP_KERNEL);
> > > > +	if (!io_dev)
> > > > +		return -ENOMEM;
> > > 
> > > Why devm_kzalloc() can't be used? I don't see if the device lifetime is anyhow
> > > different to this object. Am I wrong?
> > 
> > Looks like it but I don't know the code well enough to tell if there're
> > corner cases, so just following the documented rules. Your call.
> 
> Do you expect this to be called in non-probe() contexts? If no --> devm.
> Otherwise some comments are needed.

Sure.

> > > > +	ret = __auxiliary_device_add(aux_dev, dev->driver->name);
> > > 
> > > Hmm... Is it okay to use double underscored variant? Only a single driver uses
> > > this so far... Care to elaborate?
> > 
> > The regular variant uses KBUILD_MODNAME which comes with 'intel' prefix
> > after commit df7f9acd8646, and with that we overshoot the max id string
> > length for leaf drivers.
> 
> At bare minimum this needs a comment, but I think ideally we need to bump the
> limit by factor of 2.

Which will probably require a wider discussion, so perhaps let's pursue it
separately?

Raag

Re: [PATCH v1 1/2] platform/x86/intel: Introduce Intel Elkhart Lake PSE I/O

[Andy Shevchenko]

On Fri, Oct 31, 2025 at 12:59:24PM +0100, Raag Jadav wrote:
> On Fri, Oct 31, 2025 at 12:02:14PM +0200, Andy Shevchenko wrote:
> > On Fri, Oct 31, 2025 at 10:34:28AM +0100, Raag Jadav wrote:
> > > On Wed, Oct 29, 2025 at 10:36:19AM +0200, Andy Shevchenko wrote:
> > > > On Wed, Oct 29, 2025 at 11:50:49AM +0530, Raag Jadav wrote:

...

> > > > > +	ret = __auxiliary_device_add(aux_dev, dev->driver->name);
> > > > 
> > > > Hmm... Is it okay to use double underscored variant? Only a single driver uses
> > > > this so far... Care to elaborate?
> > > 
> > > The regular variant uses KBUILD_MODNAME which comes with 'intel' prefix
> > > after commit df7f9acd8646, and with that we overshoot the max id string
> > > length for leaf drivers.
> > 
> > At bare minimum this needs a comment, but I think ideally we need to bump the
> > limit by factor of 2.
> 
> Which will probably require a wider discussion, so perhaps let's pursue it
> separately?

Send a patch starting a discussion. Looking at the history of bumping other
cases it's usually well accepted when properly justified. And since it's now
32, bumping to 40 maybe enough for several more years.

-- 
With Best Regards,
Andy Shevchenko