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.
Signed-off-by: Raag Jadav <raag.jadav@intel.com>
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
MAINTAINERS | 5 ++
drivers/mfd/Kconfig | 12 ++++
drivers/mfd/Makefile | 1 +
drivers/mfd/intel_ehl_pse_gpio.c | 100 +++++++++++++++++++++++++++++++
4 files changed, 118 insertions(+)
create mode 100644 drivers/mfd/intel_ehl_pse_gpio.c
diff --git a/MAINTAINERS b/MAINTAINERS
index d4280facbe51..eb216eebb3a6 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -11604,6 +11604,11 @@ F: drivers/gpu/drm/xe/
F: include/drm/intel/
F: include/uapi/drm/xe_drm.h
+INTEL EHL PSE GPIO MFD DRIVER
+M: Raag Jadav <raag.jadav@intel.com>
+S: Supported
+F: drivers/mfd/intel_ehl_pse_gpio.c
+
INTEL ETHERNET DRIVERS
M: Tony Nguyen <anthony.l.nguyen@intel.com>
M: Przemek Kitszel <przemyslaw.kitszel@intel.com>
diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
index 6b0682af6e32..36eac5245179 100644
--- a/drivers/mfd/Kconfig
+++ b/drivers/mfd/Kconfig
@@ -597,6 +597,18 @@ config MFD_HI655X_PMIC
help
Select this option to enable Hisilicon hi655x series pmic driver.
+config MFD_INTEL_EHL_PSE_GPIO
+ tristate "Intel Elkhart Lake PSE GPIO MFD"
+ depends on PCI && (X86 || COMPILE_TEST)
+ select MFD_CORE
+ help
+ This MFD provides support for GPIO and TIO that exist on Intel
+ Elkhart Lake PSE as a single PCI device. It splits the two I/O
+ devices to their respective I/O drivers.
+
+ To compile this driver as a module, choose M here: the module will
+ be called intel_ehl_pse_gpio.
+
config MFD_INTEL_QUARK_I2C_GPIO
tristate "Intel Quark MFD I2C GPIO"
depends on PCI
diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
index 9220eaf7cf12..8f7d257856db 100644
--- a/drivers/mfd/Makefile
+++ b/drivers/mfd/Makefile
@@ -196,6 +196,7 @@ obj-$(CONFIG_MFD_TIMBERDALE) += timberdale.o
obj-$(CONFIG_PMIC_ADP5520) += adp5520.o
obj-$(CONFIG_MFD_ADP5585) += adp5585.o
obj-$(CONFIG_MFD_KEMPLD) += kempld-core.o
+obj-$(CONFIG_MFD_INTEL_EHL_PSE_GPIO) += intel_ehl_pse_gpio.o
obj-$(CONFIG_MFD_INTEL_QUARK_I2C_GPIO) += intel_quark_i2c_gpio.o
obj-$(CONFIG_LPC_SCH) += lpc_sch.o
obj-$(CONFIG_LPC_ICH) += lpc_ich.o
diff --git a/drivers/mfd/intel_ehl_pse_gpio.c b/drivers/mfd/intel_ehl_pse_gpio.c
new file mode 100644
index 000000000000..6a6ad1390a7b
--- /dev/null
+++ b/drivers/mfd/intel_ehl_pse_gpio.c
@@ -0,0 +1,100 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Intel MFD driver for Elkhart Lake - Programmable Service Engine
+ * (PSE) GPIO & TIO
+ *
+ * Copyright (c) 2025 Intel Corporation
+ *
+ * Intel Elkhart Lake PSE includes two PCI devices that expose two
+ * different capabilities of GPIO and Timed I/O as a single PCI
+ * function through shared MMIO.
+ */
+
+#include <linux/array_size.h>
+#include <linux/ioport.h>
+#include <linux/mfd/core.h>
+#include <linux/mod_devicetable.h>
+#include <linux/module.h>
+#include <linux/pci.h>
+#include <linux/platform_device.h>
+#include <linux/stddef.h>
+
+#define PSE_GPIO_OFFSET 0x0000
+#define PSE_GPIO_SIZE 0x0134
+
+#define PSE_TIO_OFFSET 0x1000
+#define PSE_TIO_SIZE 0x06B0
+
+static struct resource ehl_pse_gpio_resources[] = {
+ DEFINE_RES_MEM(PSE_GPIO_OFFSET, PSE_GPIO_SIZE),
+ DEFINE_RES_IRQ(0),
+};
+
+static struct resource ehl_pse_tio_resources[] = {
+ DEFINE_RES_MEM(PSE_TIO_OFFSET, PSE_TIO_SIZE),
+ DEFINE_RES_IRQ(1),
+};
+
+static struct mfd_cell ehl_pse_gpio_devs[] = {
+ {
+ .name = "gpio-elkhartlake",
+ .num_resources = ARRAY_SIZE(ehl_pse_gpio_resources),
+ .resources = ehl_pse_gpio_resources,
+ .ignore_resource_conflicts = true,
+ },
+ {
+ .name = "pps-gen-tio",
+ .num_resources = ARRAY_SIZE(ehl_pse_tio_resources),
+ .resources = ehl_pse_tio_resources,
+ .ignore_resource_conflicts = true,
+ },
+};
+
+static int ehl_pse_gpio_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_ALL_TYPES);
+ if (ret < 0)
+ return ret;
+
+ ret = mfd_add_devices(&pci->dev, PLATFORM_DEVID_AUTO, ehl_pse_gpio_devs,
+ ARRAY_SIZE(ehl_pse_gpio_devs), pci_resource_n(pci, 0),
+ pci_irq_vector(pci, 0), NULL);
+ if (ret)
+ pci_free_irq_vectors(pci);
+
+ return ret;
+}
+
+static void ehl_pse_gpio_remove(struct pci_dev *pdev)
+{
+ mfd_remove_devices(&pdev->dev);
+ pci_free_irq_vectors(pdev);
+}
+
+static const struct pci_device_id ehl_pse_gpio_ids[] = {
+ { PCI_VDEVICE(INTEL, 0x4b88) },
+ { PCI_VDEVICE(INTEL, 0x4b89) },
+ {}
+};
+MODULE_DEVICE_TABLE(pci, ehl_pse_gpio_ids);
+
+static struct pci_driver ehl_pse_gpio_driver = {
+ .probe = ehl_pse_gpio_probe,
+ .remove = ehl_pse_gpio_remove,
+ .id_table = ehl_pse_gpio_ids,
+ .name = "ehl_pse_gpio",
+};
+module_pci_driver(ehl_pse_gpio_driver);
+
+MODULE_AUTHOR("Raymond Tan <raymond.tan@intel.com>");
+MODULE_AUTHOR("Raag Jadav <raag.jadav@intel.com>");
+MODULE_DESCRIPTION("Intel MFD for Elkhart Lake PSE GPIO & TIO");
+MODULE_LICENSE("GPL");
--
2.34.1On Fri, 07 Mar 2025, 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.
>
> Signed-off-by: Raag Jadav <raag.jadav@intel.com>
> Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
> MAINTAINERS | 5 ++
> drivers/mfd/Kconfig | 12 ++++
> drivers/mfd/Makefile | 1 +
> drivers/mfd/intel_ehl_pse_gpio.c | 100 +++++++++++++++++++++++++++++++
> 4 files changed, 118 insertions(+)
> create mode 100644 drivers/mfd/intel_ehl_pse_gpio.c
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index d4280facbe51..eb216eebb3a6 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -11604,6 +11604,11 @@ F: drivers/gpu/drm/xe/
> F: include/drm/intel/
> F: include/uapi/drm/xe_drm.h
>
> +INTEL EHL PSE GPIO MFD DRIVER
> +M: Raag Jadav <raag.jadav@intel.com>
> +S: Supported
> +F: drivers/mfd/intel_ehl_pse_gpio.c
> +
> INTEL ETHERNET DRIVERS
> M: Tony Nguyen <anthony.l.nguyen@intel.com>
> M: Przemek Kitszel <przemyslaw.kitszel@intel.com>
> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
> index 6b0682af6e32..36eac5245179 100644
> --- a/drivers/mfd/Kconfig
> +++ b/drivers/mfd/Kconfig
> @@ -597,6 +597,18 @@ config MFD_HI655X_PMIC
> help
> Select this option to enable Hisilicon hi655x series pmic driver.
>
> +config MFD_INTEL_EHL_PSE_GPIO
> + tristate "Intel Elkhart Lake PSE GPIO MFD"
> + depends on PCI && (X86 || COMPILE_TEST)
> + select MFD_CORE
> + help
> + This MFD provides support for GPIO and TIO that exist on Intel
Remove references to MFD.
> + Elkhart Lake PSE as a single PCI device. It splits the two I/O
> + devices to their respective I/O drivers.
> +
> + To compile this driver as a module, choose M here: the module will
> + be called intel_ehl_pse_gpio.
> +
> config MFD_INTEL_QUARK_I2C_GPIO
> tristate "Intel Quark MFD I2C GPIO"
> depends on PCI
> diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
> index 9220eaf7cf12..8f7d257856db 100644
> --- a/drivers/mfd/Makefile
> +++ b/drivers/mfd/Makefile
> @@ -196,6 +196,7 @@ obj-$(CONFIG_MFD_TIMBERDALE) += timberdale.o
> obj-$(CONFIG_PMIC_ADP5520) += adp5520.o
> obj-$(CONFIG_MFD_ADP5585) += adp5585.o
> obj-$(CONFIG_MFD_KEMPLD) += kempld-core.o
> +obj-$(CONFIG_MFD_INTEL_EHL_PSE_GPIO) += intel_ehl_pse_gpio.o
> obj-$(CONFIG_MFD_INTEL_QUARK_I2C_GPIO) += intel_quark_i2c_gpio.o
> obj-$(CONFIG_LPC_SCH) += lpc_sch.o
> obj-$(CONFIG_LPC_ICH) += lpc_ich.o
> diff --git a/drivers/mfd/intel_ehl_pse_gpio.c b/drivers/mfd/intel_ehl_pse_gpio.c
> new file mode 100644
> index 000000000000..6a6ad1390a7b
> --- /dev/null
> +++ b/drivers/mfd/intel_ehl_pse_gpio.c
> @@ -0,0 +1,100 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Intel MFD driver for Elkhart Lake - Programmable Service Engine
As above.
> + * (PSE) GPIO & TIO
> + *
> + * Copyright (c) 2025 Intel Corporation
> + *
> + * Intel Elkhart Lake PSE includes two PCI devices that expose two
> + * different capabilities of GPIO and Timed I/O as a single PCI
> + * function through shared MMIO.
> + */
> +
> +#include <linux/array_size.h>
> +#include <linux/ioport.h>
> +#include <linux/mfd/core.h>
> +#include <linux/mod_devicetable.h>
> +#include <linux/module.h>
> +#include <linux/pci.h>
> +#include <linux/platform_device.h>
> +#include <linux/stddef.h>
> +
> +#define PSE_GPIO_OFFSET 0x0000
> +#define PSE_GPIO_SIZE 0x0134
> +
> +#define PSE_TIO_OFFSET 0x1000
> +#define PSE_TIO_SIZE 0x06B0
> +
> +static struct resource ehl_pse_gpio_resources[] = {
> + DEFINE_RES_MEM(PSE_GPIO_OFFSET, PSE_GPIO_SIZE),
> + DEFINE_RES_IRQ(0),
> +};
> +
> +static struct resource ehl_pse_tio_resources[] = {
> + DEFINE_RES_MEM(PSE_TIO_OFFSET, PSE_TIO_SIZE),
> + DEFINE_RES_IRQ(1),
> +};
> +
> +static struct mfd_cell ehl_pse_gpio_devs[] = {
> + {
> + .name = "gpio-elkhartlake",
> + .num_resources = ARRAY_SIZE(ehl_pse_gpio_resources),
> + .resources = ehl_pse_gpio_resources,
> + .ignore_resource_conflicts = true,
> + },
> + {
> + .name = "pps-gen-tio",
> + .num_resources = ARRAY_SIZE(ehl_pse_tio_resources),
> + .resources = ehl_pse_tio_resources,
> + .ignore_resource_conflicts = true,
> + },
> +};
> +
> +static int ehl_pse_gpio_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_ALL_TYPES);
> + if (ret < 0)
> + return ret;
> +
> + ret = mfd_add_devices(&pci->dev, PLATFORM_DEVID_AUTO, ehl_pse_gpio_devs,
dev_*?
> + ARRAY_SIZE(ehl_pse_gpio_devs), pci_resource_n(pci, 0),
> + pci_irq_vector(pci, 0), NULL);
> + if (ret)
> + pci_free_irq_vectors(pci);
> +
> + return ret;
> +}
> +
> +static void ehl_pse_gpio_remove(struct pci_dev *pdev)
> +{
> + mfd_remove_devices(&pdev->dev);
> + pci_free_irq_vectors(pdev);
> +}
> +
> +static const struct pci_device_id ehl_pse_gpio_ids[] = {
> + { PCI_VDEVICE(INTEL, 0x4b88) },
> + { PCI_VDEVICE(INTEL, 0x4b89) },
> + {}
> +};
> +MODULE_DEVICE_TABLE(pci, ehl_pse_gpio_ids);
> +
> +static struct pci_driver ehl_pse_gpio_driver = {
> + .probe = ehl_pse_gpio_probe,
> + .remove = ehl_pse_gpio_remove,
> + .id_table = ehl_pse_gpio_ids,
> + .name = "ehl_pse_gpio",
> +};
> +module_pci_driver(ehl_pse_gpio_driver);
> +
> +MODULE_AUTHOR("Raymond Tan <raymond.tan@intel.com>");
> +MODULE_AUTHOR("Raag Jadav <raag.jadav@intel.com>");
> +MODULE_DESCRIPTION("Intel MFD for Elkhart Lake PSE GPIO & TIO");
> +MODULE_LICENSE("GPL");
> --
> 2.34.1
>
--
Lee Jones [李琼斯]
On Fri, Mar 14, 2025 at 12:44:50PM +0000, Lee Jones wrote:
> On Fri, 07 Mar 2025, 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.
...
> > + ret = pci_alloc_irq_vectors(pci, 2, 2, PCI_IRQ_ALL_TYPES);
> > + if (ret < 0)
> > + return ret;
> > +
> > + ret = mfd_add_devices(&pci->dev, PLATFORM_DEVID_AUTO, ehl_pse_gpio_devs,
>
> dev_*?
devm_* ?
> > + ARRAY_SIZE(ehl_pse_gpio_devs), pci_resource_n(pci, 0),
> > + pci_irq_vector(pci, 0), NULL);
> > + if (ret)
> > + pci_free_irq_vectors(pci);
Anyway, the choice as far as I understood it is motivated by usage of
pci_*_irq_vector() APIs, which are officially not manageable (however
in practice they are).
> > + return ret;
> > +}
> > +
> > +static void ehl_pse_gpio_remove(struct pci_dev *pdev)
> > +{
> > + mfd_remove_devices(&pdev->dev);
> > + pci_free_irq_vectors(pdev);
> > +}
Same here.
--
With Best Regards,
Andy Shevchenko
On Fri, 14 Mar 2025, Andy Shevchenko wrote:
> On Fri, Mar 14, 2025 at 12:44:50PM +0000, Lee Jones wrote:
> > On Fri, 07 Mar 2025, 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.
>
> ...
>
> > > + ret = pci_alloc_irq_vectors(pci, 2, 2, PCI_IRQ_ALL_TYPES);
> > > + if (ret < 0)
> > > + return ret;
> > > +
> > > + ret = mfd_add_devices(&pci->dev, PLATFORM_DEVID_AUTO, ehl_pse_gpio_devs,
> >
> > dev_*?
>
> devm_* ?
Yes, typo.
> > > + ARRAY_SIZE(ehl_pse_gpio_devs), pci_resource_n(pci, 0),
> > > + pci_irq_vector(pci, 0), NULL);
> > > + if (ret)
> > > + pci_free_irq_vectors(pci);
>
> Anyway, the choice as far as I understood it is motivated by usage of
> pci_*_irq_vector() APIs, which are officially not manageable (however
> in practice they are).
>
> > > + return ret;
> > > +}
> > > +
> > > +static void ehl_pse_gpio_remove(struct pci_dev *pdev)
> > > +{
> > > + mfd_remove_devices(&pdev->dev);
> > > + pci_free_irq_vectors(pdev);
> > > +}
>
> Same here.
Also, Greg has been quite vocal about converting PCI devices to Platform
ones in the past. We may wish to run this past him before continuing.
--
Lee Jones [李琼斯]
On Fri, Mar 14, 2025 at 01:57:35PM +0000, Lee Jones wrote:
> On Fri, 14 Mar 2025, Andy Shevchenko wrote:
> > On Fri, Mar 14, 2025 at 12:44:50PM +0000, Lee Jones wrote:
> > > On Fri, 07 Mar 2025, Raag Jadav wrote:
...
> > > > +static void ehl_pse_gpio_remove(struct pci_dev *pdev)
> > > > +{
> > > > + mfd_remove_devices(&pdev->dev);
> > > > + pci_free_irq_vectors(pdev);
> > > > +}
> >
> > Same here.
>
> Also, Greg has been quite vocal about converting PCI devices to Platform
> ones in the past. We may wish to run this past him before continuing.
Greg, any objections on moving forward with platform device?
Raag
On Fri, Mar 21, 2025 at 07:28:22AM +0200, Raag Jadav wrote:
> On Fri, Mar 14, 2025 at 01:57:35PM +0000, Lee Jones wrote:
> > On Fri, 14 Mar 2025, Andy Shevchenko wrote:
> > > On Fri, Mar 14, 2025 at 12:44:50PM +0000, Lee Jones wrote:
> > > > On Fri, 07 Mar 2025, Raag Jadav wrote:
>
> ...
>
> > > > > +static void ehl_pse_gpio_remove(struct pci_dev *pdev)
> > > > > +{
> > > > > + mfd_remove_devices(&pdev->dev);
> > > > > + pci_free_irq_vectors(pdev);
> > > > > +}
> > >
> > > Same here.
> >
> > Also, Greg has been quite vocal about converting PCI devices to Platform
> > ones in the past. We may wish to run this past him before continuing.
>
> Greg, any objections on moving forward with platform device?
I have no context here at all, why would a PCI device EVER be a platform
device? That feels wrong on so many levels...
On Fri, Mar 21, 2025 at 06:04:38AM -0700, Greg KH wrote: > On Fri, Mar 21, 2025 at 07:28:22AM +0200, Raag Jadav wrote: > > On Fri, Mar 14, 2025 at 01:57:35PM +0000, Lee Jones wrote: > > > On Fri, 14 Mar 2025, Andy Shevchenko wrote: ... > > > Also, Greg has been quite vocal about converting PCI devices to Platform > > > ones in the past. We may wish to run this past him before continuing. > > > > Greg, any objections on moving forward with platform device? > > I have no context here at all, why would a PCI device EVER be a platform > device? That feels wrong on so many levels... It's a multi-functional device, in other words that device provides a set of (dependent or independent) subdevices. But do you have other suggestion? The auxiliary bus? -- With Best Regards, Andy Shevchenko
On Fri, Mar 21, 2025 at 03:22:36PM +0200, Andy Shevchenko wrote: > On Fri, Mar 21, 2025 at 06:04:38AM -0700, Greg KH wrote: > > On Fri, Mar 21, 2025 at 07:28:22AM +0200, Raag Jadav wrote: > > > On Fri, Mar 14, 2025 at 01:57:35PM +0000, Lee Jones wrote: > > > > On Fri, 14 Mar 2025, Andy Shevchenko wrote: > > ... > > > > > Also, Greg has been quite vocal about converting PCI devices to Platform > > > > ones in the past. We may wish to run this past him before continuing. > > > > > > Greg, any objections on moving forward with platform device? > > > > I have no context here at all, why would a PCI device EVER be a platform > > device? That feels wrong on so many levels... > > It's a multi-functional device, in other words that device provides a set of > (dependent or independent) subdevices. But do you have other suggestion? > The auxiliary bus? Yes, that is exactly what the auxiliary bus code was designed and written for.
On Tue, Mar 25, 2025 at 08:45:29PM -0400, Greg KH wrote: > On Fri, Mar 21, 2025 at 03:22:36PM +0200, Andy Shevchenko wrote: > > On Fri, Mar 21, 2025 at 06:04:38AM -0700, Greg KH wrote: > > > On Fri, Mar 21, 2025 at 07:28:22AM +0200, Raag Jadav wrote: > > > > On Fri, Mar 14, 2025 at 01:57:35PM +0000, Lee Jones wrote: > > > > > On Fri, 14 Mar 2025, Andy Shevchenko wrote: ... > > > > > Also, Greg has been quite vocal about converting PCI devices to Platform > > > > > ones in the past. We may wish to run this past him before continuing. > > > > > > > > Greg, any objections on moving forward with platform device? > > > > > > I have no context here at all, why would a PCI device EVER be a platform > > > device? That feels wrong on so many levels... > > > > It's a multi-functional device, in other words that device provides a set of > > (dependent or independent) subdevices. But do you have other suggestion? > > The auxiliary bus? > > Yes, that is exactly what the auxiliary bus code was designed and > written for. Lee, what do you think of extending mfd to cover this case, i.e. specifically for the PCI devices? Or maybe it makes sense to go to the auxiliary bus completely (I think this may break a lot of things on legacy systems, though). -- With Best Regards, Andy Shevchenko
On Wed, Mar 26, 2025 at 11:01:52AM +0200, Andy Shevchenko wrote: > On Tue, Mar 25, 2025 at 08:45:29PM -0400, Greg KH wrote: > > On Fri, Mar 21, 2025 at 03:22:36PM +0200, Andy Shevchenko wrote: > > > On Fri, Mar 21, 2025 at 06:04:38AM -0700, Greg KH wrote: > > > > On Fri, Mar 21, 2025 at 07:28:22AM +0200, Raag Jadav wrote: > > > > > On Fri, Mar 14, 2025 at 01:57:35PM +0000, Lee Jones wrote: > > > > > > On Fri, 14 Mar 2025, Andy Shevchenko wrote: > > ... > > > > > > > Also, Greg has been quite vocal about converting PCI devices to Platform > > > > > > ones in the past. We may wish to run this past him before continuing. > > > > > > > > > > Greg, any objections on moving forward with platform device? > > > > > > > > I have no context here at all, why would a PCI device EVER be a platform > > > > device? That feels wrong on so many levels... > > > > > > It's a multi-functional device, in other words that device provides a set of > > > (dependent or independent) subdevices. But do you have other suggestion? > > > The auxiliary bus? > > > > Yes, that is exactly what the auxiliary bus code was designed and > > written for. > > Lee, what do you think of extending mfd to cover this case, i.e. specifically > for the PCI devices? Or maybe it makes sense to go to the auxiliary bus > completely (I think this may break a lot of things on legacy systems, though). Don't change existing drivers, only do it for new ones please.
On Fri, Mar 14, 2025 at 01:57:35PM +0000, Lee Jones wrote:
> On Fri, 14 Mar 2025, Andy Shevchenko wrote:
> > On Fri, Mar 14, 2025 at 12:44:50PM +0000, Lee Jones wrote:
> > > On Fri, 07 Mar 2025, Raag Jadav wrote:
...
> > > > + ret = pci_alloc_irq_vectors(pci, 2, 2, PCI_IRQ_ALL_TYPES);
> > > > + if (ret < 0)
> > > > + return ret;
> > > > +
> > > > + ret = mfd_add_devices(&pci->dev, PLATFORM_DEVID_AUTO, ehl_pse_gpio_devs,
> > >
> > > dev_*?
> >
> > devm_* ?
>
> Yes, typo.
>
> > > > + ARRAY_SIZE(ehl_pse_gpio_devs), pci_resource_n(pci, 0),
> > > > + pci_irq_vector(pci, 0), NULL);
> > > > + if (ret)
> > > > + pci_free_irq_vectors(pci);
> >
> > Anyway, the choice as far as I understood it is motivated by usage of
> > pci_*_irq_vector() APIs, which are officially not manageable (however
> > in practice they are).
> >
> > > > + return ret;
> > > > +}
> > > > +
> > > > +static void ehl_pse_gpio_remove(struct pci_dev *pdev)
> > > > +{
> > > > + mfd_remove_devices(&pdev->dev);
> > > > + pci_free_irq_vectors(pdev);
> > > > +}
> >
> > Same here.
>
> Also, Greg has been quite vocal about converting PCI devices to Platform
> ones in the past. We may wish to run this past him before continuing.
What do you refer to? Any links to read?
--
With Best Regards,
Andy Shevchenko
On Fri, Mar 14, 2025 at 04:20:35PM +0200, Andy Shevchenko wrote:
> On Fri, Mar 14, 2025 at 01:57:35PM +0000, Lee Jones wrote:
> > On Fri, 14 Mar 2025, Andy Shevchenko wrote:
> > > On Fri, Mar 14, 2025 at 12:44:50PM +0000, Lee Jones wrote:
> > > > On Fri, 07 Mar 2025, Raag Jadav wrote:
>
> ...
>
> > > > > + ret = pci_alloc_irq_vectors(pci, 2, 2, PCI_IRQ_ALL_TYPES);
> > > > > + if (ret < 0)
> > > > > + return ret;
> > > > > +
> > > > > + ret = mfd_add_devices(&pci->dev, PLATFORM_DEVID_AUTO, ehl_pse_gpio_devs,
> > > >
> > > > dev_*?
> > >
> > > devm_* ?
> >
> > Yes, typo.
> >
> > > > > + ARRAY_SIZE(ehl_pse_gpio_devs), pci_resource_n(pci, 0),
> > > > > + pci_irq_vector(pci, 0), NULL);
> > > > > + if (ret)
> > > > > + pci_free_irq_vectors(pci);
> > >
> > > Anyway, the choice as far as I understood it is motivated by usage of
> > > pci_*_irq_vector() APIs, which are officially not manageable (however
> > > in practice they are).
Are you referring to pcim_setup_msi_release()? I saw it but wasn't confident
it is in the call path, so went with manual release instead.
Now that you mentioned it, will update.
> > > > > + return ret;
> > > > > +}
> > > > > +
> > > > > +static void ehl_pse_gpio_remove(struct pci_dev *pdev)
> > > > > +{
> > > > > + mfd_remove_devices(&pdev->dev);
> > > > > + pci_free_irq_vectors(pdev);
> > > > > +}
> > >
> > > Same here.
> >
> > Also, Greg has been quite vocal about converting PCI devices to Platform
> > ones in the past. We may wish to run this past him before continuing.
>
> What do you refer to? Any links to read?
Could be about faux_device, but I could be wrong since here we do use
platform resources.
Raag
© 2016 - 2025 Red Hat, Inc.