This new driver loads resources related to southbridges available in DM&P
Vortex devices, currently only the GPIO pins.
Signed-off-by: Marcos Del Sol Vives <marcos@orca.pet>
---
MAINTAINERS | 1 +
drivers/mfd/Kconfig | 9 +++
drivers/mfd/Makefile | 1 +
drivers/mfd/vortex-sb.c | 135 ++++++++++++++++++++++++++++++++++++++++
include/linux/pci_ids.h | 3 +
5 files changed, 149 insertions(+)
create mode 100644 drivers/mfd/vortex-sb.c
diff --git a/MAINTAINERS b/MAINTAINERS
index afa88f446630..63e410e23e95 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -26957,6 +26957,7 @@ VORTEX HARDWARE SUPPORT
R: Marcos Del Sol Vives <marcos@orca.pet>
S: Maintained
F: drivers/gpio/gpio-vortex.c
+F: drivers/mfd/vortex-sb.c
VRF
M: David Ahern <dsahern@kernel.org>
diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
index 425c5fba6cb1..fe54bb22687d 100644
--- a/drivers/mfd/Kconfig
+++ b/drivers/mfd/Kconfig
@@ -2008,6 +2008,15 @@ config MFD_VX855
VIA VX855/VX875 south bridge. You will need to enable the vx855_spi
and/or vx855_gpio drivers for this to do anything useful.
+config MFD_VORTEX_SB
+ tristate "Vortex southbridge"
+ select MFD_CORE
+ depends on PCI
+ help
+ Say yes here if you want to have support for the southbridge
+ present on Vortex SoCs. You will need to enable the vortex-gpio
+ driver for this to do anything useful.
+
config MFD_ARIZONA
select REGMAP
select REGMAP_IRQ
diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
index f7bdedd5a66d..2504ba311f1a 100644
--- a/drivers/mfd/Makefile
+++ b/drivers/mfd/Makefile
@@ -202,6 +202,7 @@ obj-$(CONFIG_MFD_JANZ_CMODIO) += janz-cmodio.o
obj-$(CONFIG_MFD_TPS6586X) += tps6586x.o
obj-$(CONFIG_MFD_VX855) += vx855.o
obj-$(CONFIG_MFD_WL1273_CORE) += wl1273-core.o
+obj-$(CONFIG_MFD_VORTEX_SB) += vortex-sb.o
si476x-core-y := si476x-cmd.o si476x-prop.o si476x-i2c.o
obj-$(CONFIG_MFD_SI476X_CORE) += si476x-core.o
diff --git a/drivers/mfd/vortex-sb.c b/drivers/mfd/vortex-sb.c
new file mode 100644
index 000000000000..141fa19773e2
--- /dev/null
+++ b/drivers/mfd/vortex-sb.c
@@ -0,0 +1,135 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * MFD southbridge driver for Vortex SoCs
+ *
+ * Author: Marcos Del Sol Vives <marcos@orca.pet>
+ *
+ * Based on the RDC321x MFD driver by Florian Fainelli and Bernhard Loos
+ */
+
+#include <linux/module.h>
+#include <linux/kernel.h>
+#include <linux/platform_device.h>
+#include <linux/pci.h>
+#include <linux/mfd/core.h>
+
+struct vortex_southbridge {
+ const struct mfd_cell *cells;
+ int n_cells;
+};
+
+/* Layout for Vortex86DX/MX */
+static const struct resource vortex_dx_gpio_resources[] = {
+ {
+ .name = "dat1",
+ .start = 0x78,
+ .end = 0x7C,
+ .flags = IORESOURCE_IO,
+ }, {
+ .name = "dir1",
+ .start = 0x98,
+ .end = 0x9C,
+ .flags = IORESOURCE_IO,
+ }
+};
+
+static const struct mfd_cell vortex_dx_sb_cells[] = {
+ {
+ .name = "vortex-gpio",
+ .resources = vortex_dx_gpio_resources,
+ .num_resources = ARRAY_SIZE(vortex_dx_gpio_resources),
+ },
+};
+
+static const struct vortex_southbridge vortex_dx_sb = {
+ .cells = vortex_dx_sb_cells,
+ .n_cells = ARRAY_SIZE(vortex_dx_sb_cells),
+};
+
+/* Layout for Vortex86DX2/DX3 */
+static const struct resource vortex_dx2_gpio_resources[] = {
+ {
+ .name = "dat1",
+ .start = 0x78,
+ .end = 0x7C,
+ .flags = IORESOURCE_IO,
+ }, {
+ .name = "dat2",
+ .start = 0x100,
+ .end = 0x105,
+ .flags = IORESOURCE_IO,
+ }, {
+ .name = "dir1",
+ .start = 0x98,
+ .end = 0x9D,
+ .flags = IORESOURCE_IO,
+ }, {
+ .name = "dir2",
+ .start = 0x93,
+ .end = 0x97,
+ .flags = IORESOURCE_IO,
+ }
+};
+
+static const struct mfd_cell vortex_dx2_sb_cells[] = {
+ {
+ .name = "vortex-gpio",
+ .resources = vortex_dx2_gpio_resources,
+ .num_resources = ARRAY_SIZE(vortex_dx2_gpio_resources),
+ },
+};
+
+static const struct vortex_southbridge vortex_dx2_sb = {
+ .cells = vortex_dx2_sb_cells,
+ .n_cells = ARRAY_SIZE(vortex_dx2_sb_cells),
+};
+
+static int vortex_sb_probe(struct pci_dev *pdev,
+ const struct pci_device_id *ent)
+{
+ struct vortex_southbridge *priv = (struct vortex_southbridge *) ent->driver_data;
+ int err;
+
+ /*
+ * In the Vortex86DX3, the southbridge appears twice (on both 00:07.0
+ * and 00:07.1). Register only once for .0.
+ *
+ * Other Vortex boards (eg Vortex86MX+) have the southbridge exposed
+ * only once, also at 00:07.0.
+ */
+ if (PCI_FUNC(pdev->devfn) != 0)
+ return -ENODEV;
+
+ err = pci_enable_device(pdev);
+ if (err) {
+ dev_err(&pdev->dev, "failed to enable device\n");
+ return err;
+ }
+
+ return devm_mfd_add_devices(&pdev->dev, PLATFORM_DEVID_NONE,
+ priv->cells, priv->n_cells,
+ NULL, 0, NULL);
+}
+
+static const struct pci_device_id vortex_sb_table[] = {
+ /* Vortex86DX */
+ { PCI_DEVICE_DATA(RDC, R6031, &vortex_dx_sb) },
+ /* Vortex86DX2/DX3 */
+ { PCI_DEVICE_DATA(RDC, R6035, &vortex_dx2_sb) },
+ /* Vortex86MX */
+ { PCI_DEVICE_DATA(RDC, R6036, &vortex_dx_sb) },
+ {}
+};
+MODULE_DEVICE_TABLE(pci, vortex_sb_table);
+
+static struct pci_driver vortex_sb_driver = {
+ .name = "vortex-sb",
+ .id_table = vortex_sb_table,
+ .probe = vortex_sb_probe,
+};
+
+module_pci_driver(vortex_sb_driver);
+
+MODULE_AUTHOR("Marcos Del Sol Vives <marcos@orca.pet>");
+MODULE_LICENSE("GPL");
+MODULE_DESCRIPTION("Vortex MFD southbridge driver");
diff --git a/include/linux/pci_ids.h b/include/linux/pci_ids.h
index 92ffc4373f6d..2c7afebd94ea 100644
--- a/include/linux/pci_ids.h
+++ b/include/linux/pci_ids.h
@@ -2412,6 +2412,9 @@
#define PCI_VENDOR_ID_RDC 0x17f3
#define PCI_DEVICE_ID_RDC_R6020 0x6020
#define PCI_DEVICE_ID_RDC_R6030 0x6030
+#define PCI_DEVICE_ID_RDC_R6031 0x6031
+#define PCI_DEVICE_ID_RDC_R6035 0x6035
+#define PCI_DEVICE_ID_RDC_R6036 0x6036
#define PCI_DEVICE_ID_RDC_R6040 0x6040
#define PCI_DEVICE_ID_RDC_R6060 0x6060
#define PCI_DEVICE_ID_RDC_R6061 0x6061
--
2.34.1On Fri, 22 Aug 2025, Marcos Del Sol Vives wrote:
> This new driver loads resources related to southbridges available in DM&P
> Vortex devices, currently only the GPIO pins.
>
> Signed-off-by: Marcos Del Sol Vives <marcos@orca.pet>
> ---
> MAINTAINERS | 1 +
> drivers/mfd/Kconfig | 9 +++
> drivers/mfd/Makefile | 1 +
> drivers/mfd/vortex-sb.c | 135 ++++++++++++++++++++++++++++++++++++++++
> include/linux/pci_ids.h | 3 +
> 5 files changed, 149 insertions(+)
> create mode 100644 drivers/mfd/vortex-sb.c
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index afa88f446630..63e410e23e95 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -26957,6 +26957,7 @@ VORTEX HARDWARE SUPPORT
> R: Marcos Del Sol Vives <marcos@orca.pet>
> S: Maintained
> F: drivers/gpio/gpio-vortex.c
> +F: drivers/mfd/vortex-sb.c
>
> VRF
> M: David Ahern <dsahern@kernel.org>
> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
> index 425c5fba6cb1..fe54bb22687d 100644
> --- a/drivers/mfd/Kconfig
> +++ b/drivers/mfd/Kconfig
> @@ -2008,6 +2008,15 @@ config MFD_VX855
> VIA VX855/VX875 south bridge. You will need to enable the vx855_spi
> and/or vx855_gpio drivers for this to do anything useful.
>
> +config MFD_VORTEX_SB
> + tristate "Vortex southbridge"
> + select MFD_CORE
> + depends on PCI
> + help
> + Say yes here if you want to have support for the southbridge
> + present on Vortex SoCs. You will need to enable the vortex-gpio
> + driver for this to do anything useful.
> +
> config MFD_ARIZONA
> select REGMAP
> select REGMAP_IRQ
> diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
> index f7bdedd5a66d..2504ba311f1a 100644
> --- a/drivers/mfd/Makefile
> +++ b/drivers/mfd/Makefile
> @@ -202,6 +202,7 @@ obj-$(CONFIG_MFD_JANZ_CMODIO) += janz-cmodio.o
> obj-$(CONFIG_MFD_TPS6586X) += tps6586x.o
> obj-$(CONFIG_MFD_VX855) += vx855.o
> obj-$(CONFIG_MFD_WL1273_CORE) += wl1273-core.o
> +obj-$(CONFIG_MFD_VORTEX_SB) += vortex-sb.o
>
> si476x-core-y := si476x-cmd.o si476x-prop.o si476x-i2c.o
> obj-$(CONFIG_MFD_SI476X_CORE) += si476x-core.o
> diff --git a/drivers/mfd/vortex-sb.c b/drivers/mfd/vortex-sb.c
> new file mode 100644
> index 000000000000..141fa19773e2
> --- /dev/null
> +++ b/drivers/mfd/vortex-sb.c
> @@ -0,0 +1,135 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * MFD southbridge driver for Vortex SoCs
Drop references to MFD.
Call it "Core southbridge ..."
> + *
> + * Author: Marcos Del Sol Vives <marcos@orca.pet>
> + *
> + * Based on the RDC321x MFD driver by Florian Fainelli and Bernhard Loos
Drop this.
> + */
> +
> +#include <linux/module.h>
> +#include <linux/kernel.h>
> +#include <linux/platform_device.h>
> +#include <linux/pci.h>
> +#include <linux/mfd/core.h>
Alphabetical.
> +
> +struct vortex_southbridge {
> + const struct mfd_cell *cells;
> + int n_cells;
> +};
Why is this needed?
> +/* Layout for Vortex86DX/MX */
> +static const struct resource vortex_dx_gpio_resources[] = {
> + {
> + .name = "dat1",
> + .start = 0x78,
Define _all_ magic numbers.
> + .end = 0x7C,
> + .flags = IORESOURCE_IO,
> + }, {
> + .name = "dir1",
> + .start = 0x98,
> + .end = 0x9C,
> + .flags = IORESOURCE_IO,
> + }
Use DEFINE_RES_*() macros.
> +};
> +
> +static const struct mfd_cell vortex_dx_sb_cells[] = {
> + {
> + .name = "vortex-gpio",
> + .resources = vortex_dx_gpio_resources,
> + .num_resources = ARRAY_SIZE(vortex_dx_gpio_resources),
> + },
> +};
It's not an MFD until you have more than one device.
> +static const struct vortex_southbridge vortex_dx_sb = {
> + .cells = vortex_dx_sb_cells,
> + .n_cells = ARRAY_SIZE(vortex_dx_sb_cells),
> +};
> +
> +/* Layout for Vortex86DX2/DX3 */
> +static const struct resource vortex_dx2_gpio_resources[] = {
> + {
> + .name = "dat1",
> + .start = 0x78,
> + .end = 0x7C,
> + .flags = IORESOURCE_IO,
> + }, {
> + .name = "dat2",
> + .start = 0x100,
> + .end = 0x105,
> + .flags = IORESOURCE_IO,
> + }, {
> + .name = "dir1",
> + .start = 0x98,
> + .end = 0x9D,
> + .flags = IORESOURCE_IO,
> + }, {
> + .name = "dir2",
> + .start = 0x93,
> + .end = 0x97,
> + .flags = IORESOURCE_IO,
> + }
> +};
As above.
> +static const struct mfd_cell vortex_dx2_sb_cells[] = {
> + {
> + .name = "vortex-gpio",
> + .resources = vortex_dx2_gpio_resources,
> + .num_resources = ARRAY_SIZE(vortex_dx2_gpio_resources),
> + },
> +};
> +
> +static const struct vortex_southbridge vortex_dx2_sb = {
> + .cells = vortex_dx2_sb_cells,
> + .n_cells = ARRAY_SIZE(vortex_dx2_sb_cells),
> +};
> +
> +static int vortex_sb_probe(struct pci_dev *pdev,
> + const struct pci_device_id *ent)
Why line-feed here and not 2 lines down?
> +{
> + struct vortex_southbridge *priv = (struct vortex_southbridge *) ent->driver_data;
s/priv/ddata/
> + int err;
> +
> + /*
> + * In the Vortex86DX3, the southbridge appears twice (on both 00:07.0
> + * and 00:07.1). Register only once for .0.
> + *
> + * Other Vortex boards (eg Vortex86MX+) have the southbridge exposed
> + * only once, also at 00:07.0.
> + */
> + if (PCI_FUNC(pdev->devfn) != 0)
> + return -ENODEV;
> +
> + err = pci_enable_device(pdev);
> + if (err) {
> + dev_err(&pdev->dev, "failed to enable device\n");
> + return err;
> + }
> +
> + return devm_mfd_add_devices(&pdev->dev, PLATFORM_DEVID_NONE,
> + priv->cells, priv->n_cells,
> + NULL, 0, NULL);
> +}
> +
> +static const struct pci_device_id vortex_sb_table[] = {
> + /* Vortex86DX */
> + { PCI_DEVICE_DATA(RDC, R6031, &vortex_dx_sb) },
We're not passing one initialisation API's data (MFD) through another (PCI).
Pass a device ID (if you don't already have one) and match on that instead.
> + /* Vortex86DX2/DX3 */
> + { PCI_DEVICE_DATA(RDC, R6035, &vortex_dx2_sb) },
> + /* Vortex86MX */
> + { PCI_DEVICE_DATA(RDC, R6036, &vortex_dx_sb) },
> + {}
> +};
> +MODULE_DEVICE_TABLE(pci, vortex_sb_table);
> +
> +static struct pci_driver vortex_sb_driver = {
> + .name = "vortex-sb",
> + .id_table = vortex_sb_table,
> + .probe = vortex_sb_probe,
> +};
> +
Remove this line.
> +module_pci_driver(vortex_sb_driver);
> +
> +MODULE_AUTHOR("Marcos Del Sol Vives <marcos@orca.pet>");
> +MODULE_LICENSE("GPL");
> +MODULE_DESCRIPTION("Vortex MFD southbridge driver");
> diff --git a/include/linux/pci_ids.h b/include/linux/pci_ids.h
> index 92ffc4373f6d..2c7afebd94ea 100644
> --- a/include/linux/pci_ids.h
> +++ b/include/linux/pci_ids.h
> @@ -2412,6 +2412,9 @@
> #define PCI_VENDOR_ID_RDC 0x17f3
> #define PCI_DEVICE_ID_RDC_R6020 0x6020
> #define PCI_DEVICE_ID_RDC_R6030 0x6030
> +#define PCI_DEVICE_ID_RDC_R6031 0x6031
> +#define PCI_DEVICE_ID_RDC_R6035 0x6035
> +#define PCI_DEVICE_ID_RDC_R6036 0x6036
> #define PCI_DEVICE_ID_RDC_R6040 0x6040
> #define PCI_DEVICE_ID_RDC_R6060 0x6060
> #define PCI_DEVICE_ID_RDC_R6061 0x6061
> --
> 2.34.1
>
--
Lee Jones [李琼斯]
El 02/09/2025 a las 17:18, Lee Jones escribió:
>> +
>> +struct vortex_southbridge {
>> + const struct mfd_cell *cells;
>> + int n_cells;
>> +};
>
> Why is this needed?
>
To have a variable amount of cells. Currently I am only implementing the
GPIO device because it's the most critical (required for device shutdown),
but I plan on implementing once this gets merged at least also the watchdog,
which is provided by the same southbridge.
Adding support for this is should make adding that simpler.
>> +static const struct mfd_cell vortex_dx_sb_cells[] = {
>> + {
>> + .name = "vortex-gpio",
>> + .resources = vortex_dx_gpio_resources,
>> + .num_resources = ARRAY_SIZE(vortex_dx_gpio_resources),
>> + },
>> +};
>
> It's not an MFD until you have more than one device.
Same as above.
>> +static const struct pci_device_id vortex_sb_table[] = {
>> + /* Vortex86DX */
>> + { PCI_DEVICE_DATA(RDC, R6031, &vortex_dx_sb) },
>
> We're not passing one initialisation API's data (MFD) through another (PCI).
Unless I understood you incorrectly, you mean I should not pass MFD cells/
data as private data?
vortex_dx_sb are "struct vortex_southbridge" type, not raw MFD API data.
On Tue, 02 Sep 2025, Marcos Del Sol Vives wrote:
> El 02/09/2025 a las 17:18, Lee Jones escribió:
> >> +
> >> +struct vortex_southbridge {
> >> + const struct mfd_cell *cells;
> >> + int n_cells;
> >> +};
> >
> > Why is this needed?
> >
>
> To have a variable amount of cells. Currently I am only implementing the
> GPIO device because it's the most critical (required for device shutdown),
> but I plan on implementing once this gets merged at least also the watchdog,
> which is provided by the same southbridge.
>
> Adding support for this is should make adding that simpler.
You don't need it. Please find another way to achieve your goal.
> >> +static const struct mfd_cell vortex_dx_sb_cells[] = {
> >> + {
> >> + .name = "vortex-gpio",
> >> + .resources = vortex_dx_gpio_resources,
> >> + .num_resources = ARRAY_SIZE(vortex_dx_gpio_resources),
> >> + },
> >> +};
> >
> > It's not an MFD until you have more than one device.
>
> Same as above.
It will not be accepted with only a single device (SFD?).
> >> +static const struct pci_device_id vortex_sb_table[] = {
> >> + /* Vortex86DX */
> >> + { PCI_DEVICE_DATA(RDC, R6031, &vortex_dx_sb) },
> >
> > We're not passing one initialisation API's data (MFD) through another (PCI).
>
> Unless I understood you incorrectly, you mean I should not pass MFD cells/
> data as private data?
Right.
> vortex_dx_sb are "struct vortex_southbridge" type, not raw MFD API data.
I like your style, but nope!
vortex_southbridge contains MFD data and shouldn't exist anyway.
--
Lee Jones [李琼斯]
El 03/09/2025 a las 9:21, Lee Jones escribió:
>>>> +static const struct mfd_cell vortex_dx_sb_cells[] = {
>>>> + {
>>>> + .name = "vortex-gpio",
>>>> + .resources = vortex_dx_gpio_resources,
>>>> + .num_resources = ARRAY_SIZE(vortex_dx_gpio_resources),
>>>> + },
>>>> +};
>>>
>>> It's not an MFD until you have more than one device.
>>
>> Same as above.
>
> It will not be accepted with only a single device (SFD?).
>
I've been working on making all the changes, except this one.
If you prefer, I can either implement the watchdog now, add it on this
patch series and thus make it a proper MFD (at the cost of delaying
even further the GPIO inclusion), or keep the struct mfd_cell array
as a single-element array and implement the watchdog later on another
merge request, using that very same array.
I am however not okay with wasting my time rewriting that to bypass
the MFD API for this, so I can waste even more time later
implementing again the MFD API, just because linguistically
one (right now) is technically not "multi".
That seems very unreasonable, specially when it wouldn't be a first
since at least these other devices are also using MFD with a single
device:
- 88pm800
- 88pm805
- at91-usart
- stw481x
- vx855
- wm8400
- zynqmp (last changed in 2024, so certainly not legacy!)
And probably others since I did not look too deep into it.
Greetings,
Marcos
On Wed, 03 Sep 2025, Marcos Del Sol Vives wrote:
> El 03/09/2025 a las 9:21, Lee Jones escribió:
> >>>> +static const struct mfd_cell vortex_dx_sb_cells[] = {
> >>>> + {
> >>>> + .name = "vortex-gpio",
> >>>> + .resources = vortex_dx_gpio_resources,
> >>>> + .num_resources = ARRAY_SIZE(vortex_dx_gpio_resources),
> >>>> + },
> >>>> +};
> >>>
> >>> It's not an MFD until you have more than one device.
> >>
> >> Same as above.
> >
> > It will not be accepted with only a single device (SFD?).
> >
>
> I've been working on making all the changes, except this one.
>
> If you prefer, I can either implement the watchdog now, add it on this
Yes, please implement the WDT now.
> patch series and thus make it a proper MFD (at the cost of delaying
> even further the GPIO inclusion), or keep the struct mfd_cell array
> as a single-element array and implement the watchdog later on another
> merge request, using that very same array.
>
> I am however not okay with wasting my time rewriting that to bypass
> the MFD API for this, so I can waste even more time later
> implementing again the MFD API, just because linguistically
> one (right now) is technically not "multi".
I don't get this. If you implement the WDT now, you will be "multi", so
what are you protesting against?
> That seems very unreasonable, specially when it wouldn't be a first
> since at least these other devices are also using MFD with a single
> device:
>
> - 88pm80
% grep name drivers/mfd/88pm800.c
.name = "88pm80x-rtc",
.name = "88pm80x-onkey",
.name = "88pm80x-regulator",
.name = "88pm800",
> - 88pm805
% grep name drivers/mfd/88pm805.c
.name = "88pm80x-codec",
.name = "88pm805",
> - at91-usart
% grep NAME drivers/mfd/at91-usart.c
MFD_CELL_NAME("at91_usart_spi");
MFD_CELL_NAME("atmel_usart_serial");
> - stw481x
* Copyright (C) 2013 ST-Ericsson SA
> - vx855
* Copyright (C) 2009 VIA Technologies, Inc.
> - wm8400
* Copyright 2008 Wolfson Microelectronics PLC.
> - zynqmp (last changed in 2024, so certainly not legacy!)
This should _not_ be using the MFD API at all!
> And probably others since I did not look too deep into it.
>
> Greetings,
> Marcos
>
--
Lee Jones [李琼斯]
El 03/09/2025 a las 16:01, Lee Jones escribió:
>> patch series and thus make it a proper MFD (at the cost of delaying
>> even further the GPIO inclusion), or keep the struct mfd_cell array
>> as a single-element array and implement the watchdog later on another
>> merge request, using that very same array.
>>
>> I am however not okay with wasting my time rewriting that to bypass
>> the MFD API for this, so I can waste even more time later
>> implementing again the MFD API, just because linguistically
>> one (right now) is technically not "multi".
>
> I don't get this. If you implement the WDT now, you will be "multi", so
> what are you protesting against?
That GPIO is something required to perform the poweroff sequence, a must
for any machine, while WDT is just a "nice to have".
Implementing now the WDT just because of a linguistic preference means
delaying something more important in favour of a "nice to have".
>> That seems very unreasonable, specially when it wouldn't be a first
>> since at least these other devices are also using MFD with a single
>> device:
>>
>> - 88pm80
>
> % grep name drivers/mfd/88pm800.c
> .name = "88pm80x-rtc",
> .name = "88pm80x-onkey",
> .name = "88pm80x-regulator",
> .name = "88pm800",
If you open the file, you'll see it uses five single-element arrays.
>> - 88pm805
>
> % grep name drivers/mfd/88pm805.c
> .name = "88pm80x-codec",
> .name = "88pm805",
>
Same as above.
>> - at91-usart
>
> % grep NAME drivers/mfd/at91-usart.c
> MFD_CELL_NAME("at91_usart_spi");
> MFD_CELL_NAME("atmel_usart_serial");
Has two single-element arrays. It registers one or the other, never both
(just like my patch does!)
>> - stw481x
>
> * Copyright (C) 2013 ST-Ericsson SA
>
>> - vx855
>
> * Copyright (C) 2009 VIA Technologies, Inc.
>
>> - wm8400
>
> * Copyright 2008 Wolfson Microelectronics PLC.
>
To my knowledge the definition of "multi" has not been changed
since any of those years.
On Wed, 03 Sep 2025, Marcos Del Sol Vives wrote:
> El 03/09/2025 a las 16:01, Lee Jones escribió:
> >> patch series and thus make it a proper MFD (at the cost of delaying
> >> even further the GPIO inclusion), or keep the struct mfd_cell array
> >> as a single-element array and implement the watchdog later on another
> >> merge request, using that very same array.
> >>
> >> I am however not okay with wasting my time rewriting that to bypass
> >> the MFD API for this, so I can waste even more time later
> >> implementing again the MFD API, just because linguistically
> >> one (right now) is technically not "multi".
> >
> > I don't get this. If you implement the WDT now, you will be "multi", so
> > what are you protesting against?
>
> That GPIO is something required to perform the poweroff sequence, a must
> for any machine, while WDT is just a "nice to have".
>
> Implementing now the WDT just because of a linguistic preference means
> delaying something more important in favour of a "nice to have".
You use the word "delaying" here. What's the rush?
If you only need a GPIO driver, then you don't need the MFD part.
> >> That seems very unreasonable, specially when it wouldn't be a first
> >> since at least these other devices are also using MFD with a single
> >> device:
> >>
> >> - 88pm80
> >
> > % grep name drivers/mfd/88pm800.c
> > .name = "88pm80x-rtc",
> > .name = "88pm80x-onkey",
> > .name = "88pm80x-regulator",
> > .name = "88pm800",
>
> If you open the file, you'll see it uses five single-element arrays.
Which is fine.
> >> - 88pm805
> >
> > % grep name drivers/mfd/88pm805.c
> > .name = "88pm80x-codec",
> > .name = "88pm805",
> >
>
> Same as above.
>
> >> - at91-usart
> >
> > % grep NAME drivers/mfd/at91-usart.c
> > MFD_CELL_NAME("at91_usart_spi");
> > MFD_CELL_NAME("atmel_usart_serial");
>
> Has two single-element arrays. It registers one or the other, never both
> (just like my patch does!)
Fair point.
I guess this is more of a selector than a true concurrent MFD.
> >> - stw481x
> >
> > * Copyright (C) 2013 ST-Ericsson SA
> >
> >> - vx855
> >
> > * Copyright (C) 2009 VIA Technologies, Inc.
> >
> >> - wm8400
> >
> > * Copyright 2008 Wolfson Microelectronics PLC.
> >
>
> To my knowledge the definition of "multi" has not been changed
> since any of those years.
If they were submitted during my tenure, it is likely that they would
not have been accepted. Besides, past mistakes is no excuse for making
them in the present.
--
Lee Jones [李琼斯]
El 04/09/2025 a las 12:17, Lee Jones escribió: >> That GPIO is something required to perform the poweroff sequence, a must >> for any machine, while WDT is just a "nice to have". >> >> Implementing now the WDT just because of a linguistic preference means >> delaying something more important in favour of a "nice to have". > > You use the word "delaying" here. What's the rush? > > If you only need a GPIO driver, then you don't need the MFD part. > I would honestly like that my machines can turn off properly and pretty sure others using these platforms would agree on that, as having to yank out the power cable is far from ideal. Adding WDT would lengthen even further the review process. That ignoring I am doing this as a hobby on my spare time and I'd rather spend my scarce free time implementing the power off driver than the WDT (something I'd do out of completion, I have absolutely no use for a WDT in this machine). The reason I am using an MFD is that I was asked to back in v2 (https://lore.kernel.org/all/aHElavFTptu0q4Kj@smile.fi.intel.com/). I'll be CC'ing him. I was told to create a southbridge driver that would match on PCI and registered other devices exposed by it as platform drivers. GPIO was the only functionality implemented at the time, and is the only functionality implemented right now. So I simply delivered was I was asked for.
On Thu, 04 Sep 2025, Marcos Del Sol Vives wrote: > El 04/09/2025 a las 12:17, Lee Jones escribió: > >> That GPIO is something required to perform the poweroff sequence, a must > >> for any machine, while WDT is just a "nice to have". > >> > >> Implementing now the WDT just because of a linguistic preference means > >> delaying something more important in favour of a "nice to have". > > > > You use the word "delaying" here. What's the rush? > > > > If you only need a GPIO driver, then you don't need the MFD part. > > > > I would honestly like that my machines can turn off properly and pretty > sure others using these platforms would agree on that, as having to yank > out the power cable is far from ideal. > > Adding WDT would lengthen even further the review process. That ignoring > I am doing this as a hobby on my spare time and I'd rather spend my > scarce free time implementing the power off driver than the WDT > (something I'd do out of completion, I have absolutely no use for a WDT > in this machine). Then don't implement it. Just have the GPIO driver probe on PCI match. > The reason I am using an MFD is that I was asked to back in v2 > (https://lore.kernel.org/all/aHElavFTptu0q4Kj@smile.fi.intel.com/). > I'll be CC'ing him. Andy knows the rules. > I was told to create a southbridge driver that would match on PCI > and registered other devices exposed by it as platform drivers. PCI => Platform is generally decried by Greg and others. a) With only one device to register, are you sure you need this? b) If you have more devices, either add them here or use Aux Bus. > GPIO was the only functionality implemented at the time, and is > the only functionality implemented right now. So I simply delivered was > I was asked for. -- Lee Jones [李琼斯]
El 03/09/2025 a las 9:21, Lee Jones escribió: >> vortex_dx_sb are "struct vortex_southbridge" type, not raw MFD API data. > > I like your style, but nope! > > vortex_southbridge contains MFD data and shouldn't exist anyway. I'm not sure if I follow. You're suggesting not using driver_data at all and using a big "if" instead, matching manually myself on the correct cells to register against the PCI device ID, instead of relying on PCI matching giving me already the cells structure inside driver_data? That seems to increase code size and be more error prone for no reason.
On Wed, 03 Sep 2025, Marcos Del Sol Vives wrote: > El 03/09/2025 a las 9:21, Lee Jones escribió: > >> vortex_dx_sb are "struct vortex_southbridge" type, not raw MFD API data. > > > > I like your style, but nope! > > > > vortex_southbridge contains MFD data and shouldn't exist anyway. > > I'm not sure if I follow. > > You're suggesting not using driver_data at all and using a big "if" instead, > matching manually myself on the correct cells to register against the PCI > device ID, instead of relying on PCI matching giving me already the cells > structure inside driver_data? Yes. > That seems to increase code size and be more error prone for no reason. It may make sense for your use-case, but believe me, I've seen some crazy implementations of this. I found it's easier just to have a no cross contamination of early init APSs rule and call it a day. -- Lee Jones [李琼斯]
© 2016 - 2025 Red Hat, Inc.