Re: [PATCH v3 5/7] mfd: ioc3: Add driver for SGI IOC3 chip

From: Lee Jones
Date: Thu Jul 25 2019 - 14:05:50 EST

On Thu, 13 Jun 2019, Thomas Bogendoerfer wrote:

> SGI IOC3 chip has integrated ethernet, keyboard and mouse interface.
> It also supports connecting a SuperIO chip for serial and parallel
> interfaces. IOC3 is used inside various SGI systemboards and add-on
> cards with different equipped external interfaces.
>
> Support for ethernet and serial interfaces were implemented inside
> the network driver. This patchset moves out the not network related
> parts to a new MFD driver, which takes care of card detection,
> setup of platform devices and interrupt distribution for the subdevices.
>
> Serial portion: Acked-by: Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx>
>
> Signed-off-by: Thomas Bogendoerfer <tbogendoerfer@xxxxxxx>
> ---
> arch/mips/include/asm/sn/ioc3.h | 345 +++----
> arch/mips/sgi-ip27/ip27-timer.c | 20 -

> drivers/mfd/Kconfig | 13 +
> drivers/mfd/Makefile | 1 +
> drivers/mfd/ioc3.c | 683 +++++++++++++

A vast improvement, but still a little work to do.

> drivers/net/ethernet/sgi/Kconfig | 4 +-
> drivers/net/ethernet/sgi/ioc3-eth.c | 1932 ++++++++++++++---------------------
> drivers/tty/serial/8250/8250_ioc3.c | 98 ++
> drivers/tty/serial/8250/Kconfig | 11 +
> drivers/tty/serial/8250/Makefile | 1 +
> 10 files changed, 1693 insertions(+), 1415 deletions(-)
> create mode 100644 drivers/mfd/ioc3.c
> create mode 100644 drivers/tty/serial/8250/8250_ioc3.c

[...]

> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
> index a17d275bf1d4..5c9f1bd9bd0a 100644
> --- a/drivers/mfd/Kconfig
> +++ b/drivers/mfd/Kconfig
> @@ -1989,5 +1989,18 @@ config RAVE_SP_CORE
> Select this to get support for the Supervisory Processor
> device found on several devices in RAVE line of hardware.
>
> +config SGI_MFD_IOC3
> + tristate "SGI IOC3 core driver"
> + depends on PCI && MIPS
> + select MFD_CORE
> + help
> + This option enables basic support for the SGI IOC3-based
> + controller cards. This option does not enable any specific
> + functions on such a card, but provides necessary infrastructure
> + for other drivers to utilize.
> +
> + If you have an SGI Origin, Octane, or a PCI IOC3 card,
> + then say Y. Otherwise say N.
> +
> endmenu
> endif
> diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
> index 52b1a90ff515..bba0d9eb0b18 100644
> --- a/drivers/mfd/Makefile
> +++ b/drivers/mfd/Makefile
> @@ -249,3 +249,4 @@ obj-$(CONFIG_MFD_SC27XX_PMIC) += sprd-sc27xx-spi.o
> obj-$(CONFIG_RAVE_SP_CORE) += rave-sp.o
> obj-$(CONFIG_MFD_ROHM_BD718XX) += rohm-bd718x7.o
> obj-$(CONFIG_MFD_STMFX) += stmfx.o
> +obj-$(CONFIG_SGI_MFD_IOC3) += ioc3.o
> diff --git a/drivers/mfd/ioc3.c b/drivers/mfd/ioc3.c
> new file mode 100644
> index 000000000000..0c0d1b3475d0
> --- /dev/null
> +++ b/drivers/mfd/ioc3.c
> @@ -0,0 +1,683 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * SGI IOC3 multifunction device driver
> + *
> + * Copyright (C) 2018, 2019 Thomas Bogendoerfer <tbogendoerfer@xxxxxxx>
> + *
> + * Based on work by:
> + * Stanislaw Skowronek <skylark@xxxxxxxxxxxxx>
> + * Joshua Kinard <kumba@xxxxxxxxxx>
> + * Brent Casavant <bcasavan@xxxxxxx> - IOC4 master driver
> + * Pat Gefre <pfg@xxxxxxx> - IOC3 serial port IRQ demuxer
> + */
> +
> +#include <linux/delay.h>
> +#include <linux/errno.h>
> +#include <linux/interrupt.h>
> +#include <linux/mfd/core.h>
> +#include <linux/module.h>
> +#include <linux/pci.h>
> +#include <linux/platform_device.h>
> +#include <linux/platform_data/sgi-w1.h>
> +#include <linux/rtc/ds1685.h>
> +
> +#include <asm/pci/bridge.h>
> +#include <asm/sn/ioc3.h>
> +
> +#define IOC3_IRQ_SERIAL_A 6
> +#define IOC3_IRQ_SERIAL_B 15
> +#define IOC3_IRQ_KBD 22
> +#define IOC3_IRQ_ETH_DOMAIN 23
> +
> +static int ioc3_serial_id;
> +static int ioc3_eth_id;
> +static int ioc3_kbd_id;
> +
> +struct ioc3_priv_data {
> + struct irq_domain *domain;
> + struct ioc3 __iomem *regs;
> + struct pci_dev *pdev;
> + int domain_irq;
> +};
> +
> +static void ioc3_irq_ack(struct irq_data *d)
> +{
> + struct ioc3_priv_data *ipd = irq_data_get_irq_chip_data(d);
> + unsigned int hwirq = irqd_to_hwirq(d);
> +
> + writel(BIT(hwirq), &ipd->regs->sio_ir);
> +}
> +
> +static void ioc3_irq_mask(struct irq_data *d)
> +{
> + struct ioc3_priv_data *ipd = irq_data_get_irq_chip_data(d);
> + unsigned int hwirq = irqd_to_hwirq(d);
> +
> + writel(BIT(hwirq), &ipd->regs->sio_iec);
> +}
> +
> +static void ioc3_irq_unmask(struct irq_data *d)
> +{
> + struct ioc3_priv_data *ipd = irq_data_get_irq_chip_data(d);
> + unsigned int hwirq = irqd_to_hwirq(d);
> +
> + writel(BIT(hwirq), &ipd->regs->sio_ies);
> +}
> +
> +static struct irq_chip ioc3_irq_chip = {
> + .name = "IOC3",
> + .irq_ack = ioc3_irq_ack,
> + .irq_mask = ioc3_irq_mask,
> + .irq_unmask = ioc3_irq_unmask,
> +};
> +
> +#define IOC3_LVL_MASK (BIT(IOC3_IRQ_SERIAL_A) | BIT(IOC3_IRQ_SERIAL_B))
> +
> +static int ioc3_irq_domain_map(struct irq_domain *d, unsigned int irq,
> + irq_hw_number_t hwirq)
> +{
> + /* use level irqs for every interrupt contained in IOC3_LVL_MASK */

Nit: Could you use proper grammar (less the full stops) in the
comments please? Start with an uppercase character and things like
"irq" should be "IRQ", etc.

> + if (BIT(hwirq) & IOC3_LVL_MASK)
> + irq_set_chip_and_handler(irq, &ioc3_irq_chip, handle_level_irq);
> + else
> + irq_set_chip_and_handler(irq, &ioc3_irq_chip, handle_edge_irq);
> +
> + irq_set_chip_data(irq, d->host_data);
> + return 0;
> +}
> +
> +static const struct irq_domain_ops ioc3_irq_domain_ops = {
> + .map = ioc3_irq_domain_map,
> +};
> +
> +static void ioc3_irq_handler(struct irq_desc *desc)
> +{
> + struct irq_domain *domain = irq_desc_get_handler_data(desc);
> + struct ioc3_priv_data *ipd = domain->host_data;
> + struct ioc3 __iomem *regs = ipd->regs;
> + u32 pending, mask;
> + unsigned int irq;
> +
> + pending = readl(&regs->sio_ir);
> + mask = readl(&regs->sio_ies);
> + pending &= mask; /* mask off not enabled but pending irqs */
> +
> + if (mask & BIT(IOC3_IRQ_ETH_DOMAIN))
> + /* if eth irq is enabled we need to check in eth irq regs */
> + if (readl(&regs->eth.eisr) & readl(&regs->eth.eier))
> + pending |= IOC3_IRQ_ETH_DOMAIN;
> +
> + if (pending) {
> + irq = irq_find_mapping(domain, __ffs(pending));
> + if (irq)
> + generic_handle_irq(irq);
> + } else {
> + spurious_interrupt();
> + }
> +}
> +
> +/*
> + * System boards/BaseIOs use more interrupt pins of the bridge asic

"ASIC"

> + * to which the IOC3 is connected. Since the IOC3 MFD driver
> + * knows wiring of these extra pins, we use the map_irq function
> + * to get interrupts activated
> + */
> +static int ioc3_map_irq(struct pci_dev *pdev, int pin)
> +{
> + struct pci_host_bridge *hbrg = pci_find_host_bridge(pdev->bus);
> +
> + return hbrg->map_irq(pdev, pin, 0);
> +}
> +
> +static int ioc3_irq_domain_setup(struct ioc3_priv_data *ipd, int irq)
> +{
> + struct irq_domain *domain;
> + struct fwnode_handle *fn;
> +
> + fn = irq_domain_alloc_named_fwnode("IOC3");
> + if (!fn)
> + goto err;
> +
> + domain = irq_domain_create_linear(fn, 24, &ioc3_irq_domain_ops, ipd);
> + if (!domain)
> + goto err;
> +
> + irq_domain_free_fwnode(fn);
> + ipd->domain = domain;
> +
> + irq_set_chained_handler_and_data(irq, ioc3_irq_handler, domain);
> + ipd->domain_irq = irq;
> + return 0;
> +err:
> + dev_err(&ipd->pdev->dev, "irq domain setup failed\n");
> + return -ENOMEM;
> +}
> +
> +static struct resource ioc3_uarta_resources[] = {
> + DEFINE_RES_MEM(offsetof(struct ioc3, sregs.uarta),
> + sizeof_field(struct ioc3, sregs.uarta)),
> + DEFINE_RES_IRQ(IOC3_IRQ_SERIAL_A)
> +};
> +
> +static struct resource ioc3_uartb_resources[] = {
> + DEFINE_RES_MEM(offsetof(struct ioc3, sregs.uartb),
> + sizeof_field(struct ioc3, sregs.uartb)),
> + DEFINE_RES_IRQ(IOC3_IRQ_SERIAL_B)
> +};
> +
> +static struct mfd_cell ioc3_serial_cells[] = {
> + {
> + .name = "ioc3-serial8250",
> + .resources = ioc3_uarta_resources,
> + .num_resources = ARRAY_SIZE(ioc3_uarta_resources),
> + .id = 0,
> + },
> + {
> + .name = "ioc3-serial8250",
> + .resources = ioc3_uartb_resources,
> + .num_resources = ARRAY_SIZE(ioc3_uartb_resources),
> + .id = 1,

Any reason for not using PLATFORM_DEVID_AUTO.

> + }
> +};
> +
> +static int ioc3_serial_setup(struct ioc3_priv_data *ipd)
> +{
> + int ret;
> +
> + /* set gpio pins for RS232/RS422 mode selection */
> + writel(GPCR_UARTA_MODESEL | GPCR_UARTB_MODESEL,
> + &ipd->regs->gpcr_s);
> + /* select RS232 mode for uart a */
> + writel(0, &ipd->regs->gppr[6]);
> + /* select RS232 mode for uart b */
> + writel(0, &ipd->regs->gppr[7]);
> +
> + /* switch both ports to 16650 mode */
> + writel(readl(&ipd->regs->port_a.sscr) & ~SSCR_DMA_EN,
> + &ipd->regs->port_a.sscr);
> + writel(readl(&ipd->regs->port_b.sscr) & ~SSCR_DMA_EN,
> + &ipd->regs->port_b.sscr);
> + udelay(1000); /* wait until mode switch is done */
> +
> + ret = mfd_add_devices(&ipd->pdev->dev, ioc3_serial_id,

Any reason for not using PLATFORM_DEVID_AUTO.

> + ioc3_serial_cells, ARRAY_SIZE(ioc3_serial_cells),
> + &ipd->pdev->resource[0], 0, ipd->domain);
> + if (ret) {
> + dev_err(&ipd->pdev->dev, "Failed to add 16550 subdevs\n");
> + return ret;
> + }
> + ioc3_serial_id += 2;

When is this likely to be re-read?

> + return 0;
> +}
> +
> +static struct resource ioc3_kbd_resources[] = {
> + DEFINE_RES_MEM(offsetof(struct ioc3, serio),
> + sizeof_field(struct ioc3, serio)),
> + DEFINE_RES_IRQ(IOC3_IRQ_KBD)
> +};
> +
> +static struct mfd_cell ioc3_kbd_cells[] = {
> + {
> + .name = "ioc3-kbd",
> + .resources = ioc3_kbd_resources,
> + .num_resources = ARRAY_SIZE(ioc3_kbd_resources),
> + }
> +};
> +
> +static int ioc3_kbd_setup(struct ioc3_priv_data *ipd)
> +{
> + int ret;
> +
> + ret = mfd_add_devices(&ipd->pdev->dev, ioc3_kbd_id, ioc3_kbd_cells,
> + ARRAY_SIZE(ioc3_kbd_cells),
> + &ipd->pdev->resource[0], 0, ipd->domain);
> + if (ret) {
> + dev_err(&ipd->pdev->dev, "Failed to add 16550 subdevs\n");
> + return ret;
> + }
> + ioc3_kbd_id++;

Nit: '\n' here

> + return 0;
> +}
> +
> +static struct resource ioc3_eth_resources[] = {
> + DEFINE_RES_MEM(offsetof(struct ioc3, eth),
> + sizeof_field(struct ioc3, eth)),
> + DEFINE_RES_MEM(offsetof(struct ioc3, ssram),
> + sizeof_field(struct ioc3, ssram)),
> + DEFINE_RES_IRQ(0)
> +};
> +
> +static struct resource ioc3_w1_resources[] = {
> + DEFINE_RES_MEM(offsetof(struct ioc3, mcr),
> + sizeof_field(struct ioc3, mcr)),
> +};
> +static struct sgi_w1_platform_data ioc3_w1_platform_data;
> +
> +static struct mfd_cell ioc3_eth_cells[] = {
> + {
> + .name = "ioc3-eth",
> + .resources = ioc3_eth_resources,
> + .num_resources = ARRAY_SIZE(ioc3_eth_resources),
> + },
> + {
> + .name = "sgi_w1",
> + .resources = ioc3_w1_resources,
> + .num_resources = ARRAY_SIZE(ioc3_w1_resources),
> + .platform_data = &ioc3_w1_platform_data,
> + .pdata_size = sizeof(ioc3_w1_platform_data),
> + }
> +};
> +
> +static int ioc3_eth_setup(struct ioc3_priv_data *ipd, bool use_domain)
> +{
> + int irq = ipd->pdev->irq;
> + int ret;
> +
> + /* enable One-Wire bus */
> + writel(GPCR_MLAN_EN, &ipd->regs->gpcr_s);
> +
> + /* generate unique identifier */
> + snprintf(ioc3_w1_platform_data.dev_id,
> + sizeof(ioc3_w1_platform_data.dev_id), "ioc3-%012llx",
> + ipd->pdev->resource->start);
> +
> + if (use_domain)
> + irq = irq_create_mapping(ipd->domain, IOC3_IRQ_ETH_DOMAIN);
> +
> + ret = mfd_add_devices(&ipd->pdev->dev, ioc3_eth_id, ioc3_eth_cells,
> + ARRAY_SIZE(ioc3_eth_cells),
> + &ipd->pdev->resource[0], irq, NULL);
> + if (ret) {
> + dev_err(&ipd->pdev->dev, "Failed to add ETH/W1 subdev\n");
> + return ret;
> + }
> +

Nit: Remove this line and place it below:

> + ioc3_eth_id++;

Nit: '\n' here

> + return 0;
> +}
> +
> +#define M48T35_REG_SIZE 32768 /* size of m48t35 registers */
> +
> +static struct resource ioc3_m48t35_resources[] = {
> + DEFINE_RES_MEM(IOC3_BYTEBUS_DEV0, M48T35_REG_SIZE)
> +};
> +
> +static struct mfd_cell ioc3_m48t35_cells[] = {
> + {
> + .name = "rtc-m48t35",
> + .resources = ioc3_m48t35_resources,
> + .num_resources = ARRAY_SIZE(ioc3_m48t35_resources),
> + }
> +};
> +
> +static int ioc3_m48t35_setup(struct ioc3_priv_data *ipd)
> +{
> + int ret;
> +
> + ret = mfd_add_devices(&ipd->pdev->dev, PLATFORM_DEVID_AUTO,
> + ioc3_m48t35_cells, ARRAY_SIZE(ioc3_m48t35_cells),
> + &ipd->pdev->resource[0], 0, ipd->domain);
> + if (ret)
> + dev_err(&ipd->pdev->dev, "Failed to add M48T35 subdev\n");
> +
> + return ret;
> +}
> +
> +/*
> + * On IP30 the RTC (a DS1687) is behind the IOC3 on the generic
> + * ByteBus regions. We have to write the RTC address of interest to
> + * IOC3_BYTEBUS_DEV1, then read the data from IOC3_BYTEBUS_DEV2.
> + * rtc->regs already points to IOC3_BYTEBUS_DEV1.
> + */
> +#define IP30_RTC_ADDR(rtc) (rtc->regs)
> +#define IP30_RTC_DATA(rtc) ((rtc->regs) + IOC3_BYTEBUS_DEV2 - IOC3_BYTEBUS_DEV1)
> +
> +static u8 ip30_rtc_read(struct ds1685_priv *rtc, int reg)
> +{
> + writeb((reg & 0x7f), IP30_RTC_ADDR(rtc));
> + return readb(IP30_RTC_DATA(rtc));
> +}
> +
> +static void ip30_rtc_write(struct ds1685_priv *rtc, int reg, u8 value)
> +{
> + writeb((reg & 0x7f), IP30_RTC_ADDR(rtc));
> + writeb(value, IP30_RTC_DATA(rtc));
> +}

Why is this not in the RTC driver?

> +static struct ds1685_rtc_platform_data ip30_rtc_platform_data = {
> + .bcd_mode = false,
> + .no_irq = false,
> + .uie_unsupported = true,
> + .alloc_io_resources = true,

> + .plat_read = ip30_rtc_read,
> + .plat_write = ip30_rtc_write,

Call-backs in a non-subsystem API is pretty ugly IMHO.

Where are these called from?

> +};
> +
> +static struct resource ioc3_rtc_ds1685_resources[] = {
> + DEFINE_RES_MEM(IOC3_BYTEBUS_DEV1,
> + IOC3_BYTEBUS_DEV2 - IOC3_BYTEBUS_DEV1 + 1),
> + DEFINE_RES_IRQ(0)
> +};
> +
> +static struct mfd_cell ioc3_ds1685_cells[] = {
> + {
> + .name = "rtc-ds1685",
> + .resources = ioc3_rtc_ds1685_resources,
> + .num_resources = ARRAY_SIZE(ioc3_rtc_ds1685_resources),
> + .platform_data = &ip30_rtc_platform_data,
> + .pdata_size = sizeof(ip30_rtc_platform_data),

> + .id = -1,

Use the #define. Same goes for below.

> + }
> +};
> +
> +static int ioc3_ds1685_setup(struct ioc3_priv_data *ipd)
> +{
> + int ret, irq;
> +
> + irq = ioc3_map_irq(ipd->pdev, 6);

Nit: '\n' here

> + ret = mfd_add_devices(&ipd->pdev->dev, 0, ioc3_ds1685_cells,
> + ARRAY_SIZE(ioc3_ds1685_cells),
> + &ipd->pdev->resource[0], irq, NULL);
> + if (ret)
> + dev_err(&ipd->pdev->dev, "Failed to add DS1685 subdev\n");
> +
> + return ret;
> +};
> +
> +
> +static struct resource ioc3_leds_resources[] = {
> + DEFINE_RES_MEM(offsetof(struct ioc3, gppr[0]),
> + sizeof_field(struct ioc3, gppr[0])),
> + DEFINE_RES_MEM(offsetof(struct ioc3, gppr[1]),
> + sizeof_field(struct ioc3, gppr[1])),
> +};
> +
> +static struct mfd_cell ioc3_led_cells[] = {
> + {
> + .name = "ip30-leds",
> + .resources = ioc3_leds_resources,
> + .num_resources = ARRAY_SIZE(ioc3_leds_resources),
> + .id = -1,
> + }
> +};
> +
> +static int ioc3_led_setup(struct ioc3_priv_data *ipd)
> +{
> + int ret;
> +
> + ret = mfd_add_devices(&ipd->pdev->dev, 0, ioc3_led_cells,
> + ARRAY_SIZE(ioc3_led_cells),
> + &ipd->pdev->resource[0], 0, ipd->domain);
> + if (ret)
> + dev_err(&ipd->pdev->dev, "Failed to add LED subdev\n");

Nit: '\n' here

> + return ret;
> +}
> +
> +static int ip27_baseio_setup(struct ioc3_priv_data *ipd)
> +{
> + int ret, io_irq;
> +
> + io_irq = ioc3_map_irq(ipd->pdev, PCI_SLOT(ipd->pdev->devfn) + 2);
> + ret = ioc3_irq_domain_setup(ipd, io_irq);
> + if (ret)
> + return ret;

Nit: '\n' here

> + ret = ioc3_eth_setup(ipd, false);
> + if (ret)
> + return ret;

Nit: '\n' here

> + ret = ioc3_serial_setup(ipd);
> + if (ret)
> + return ret;

Nit: '\n' here

Etc, etc, etc ... same for all of the instances below.

> + return ioc3_m48t35_setup(ipd);
> +}
> +
> +static int ip27_baseio6g_setup(struct ioc3_priv_data *ipd)
> +{
> + int ret, io_irq;
> +
> + io_irq = ioc3_map_irq(ipd->pdev, PCI_SLOT(ipd->pdev->devfn) + 2);
> + ret = ioc3_irq_domain_setup(ipd, io_irq);
> + if (ret)
> + return ret;
> + ret = ioc3_eth_setup(ipd, false);
> + if (ret)
> + return ret;
> + ret = ioc3_serial_setup(ipd);
> + if (ret)
> + return ret;
> + ret = ioc3_m48t35_setup(ipd);
> + if (ret)
> + return ret;
> + return ioc3_kbd_setup(ipd);
> +}
> +
> +static int ip27_mio_setup(struct ioc3_priv_data *ipd)
> +{
> + int ret;
> +
> + ret = ioc3_irq_domain_setup(ipd, ipd->pdev->irq);
> + if (ret)
> + return ret;
> + ret = ioc3_serial_setup(ipd);
> + if (ret)
> + return ret;
> + return ioc3_kbd_setup(ipd);
> +}
> +
> +static int ip29_sysboard_setup(struct ioc3_priv_data *ipd)
> +{
> + int ret, io_irq;
> +
> + io_irq = ioc3_map_irq(ipd->pdev, PCI_SLOT(ipd->pdev->devfn) + 1);
> + ret = ioc3_irq_domain_setup(ipd, io_irq);
> + if (ret)
> + return ret;
> + ret = ioc3_eth_setup(ipd, false);
> + if (ret)
> + return ret;
> + ret = ioc3_serial_setup(ipd);
> + if (ret)
> + return ret;
> + ret = ioc3_m48t35_setup(ipd);
> + if (ret)
> + return ret;
> + return ioc3_kbd_setup(ipd);
> +}
> +
> +static int ip30_sysboard_setup(struct ioc3_priv_data *ipd)
> +{
> + int ret, io_irq;
> +
> + io_irq = ioc3_map_irq(ipd->pdev, PCI_SLOT(ipd->pdev->devfn) + 2);
> + ret = ioc3_irq_domain_setup(ipd, io_irq);
> + if (ret)
> + return ret;
> + ret = ioc3_eth_setup(ipd, false);
> + if (ret)
> + return ret;
> + ret = ioc3_serial_setup(ipd);
> + if (ret)
> + return ret;
> + ret = ioc3_kbd_setup(ipd);
> + if (ret)
> + return ret;
> + ret = ioc3_ds1685_setup(ipd);
> + if (ret)
> + return ret;
> + return ioc3_led_setup(ipd);
> +}
> +
> +static int ioc3_menet_setup(struct ioc3_priv_data *ipd)
> +{
> + int ret, io_irq;
> +
> + io_irq = ioc3_map_irq(ipd->pdev, PCI_SLOT(ipd->pdev->devfn) + 4);
> + ret = ioc3_irq_domain_setup(ipd, io_irq);
> + if (ret)
> + return ret;
> + ret = ioc3_eth_setup(ipd, false);
> + if (ret)
> + return ret;
> + return ioc3_serial_setup(ipd);
> +}
> +
> +static int ioc3_menet4_setup(struct ioc3_priv_data *ipd)
> +{
> + return ioc3_eth_setup(ipd, false);
> +}
> +
> +static int ioc3_cad_duo_setup(struct ioc3_priv_data *ipd)
> +{
> + int ret;
> +
> + ret = ioc3_irq_domain_setup(ipd, ipd->pdev->irq);
> + if (ret)
> + return ret;
> + ret = ioc3_eth_setup(ipd, true);
> + if (ret)
> + return ret;
> + return ioc3_kbd_setup(ipd);
> +}
> +
> +#define IOC3_SID(_name, _sid, _setup) \
> + { \
> + .name = _name, \
> + .sid = (PCI_VENDOR_ID_SGI << 16) | IOC3_SUBSYS_ ## _sid, \
> + .setup = _setup, \
> + }
> +
> +static struct {
> + const char *name;
> + u32 sid;
> + int (*setup)(struct ioc3_priv_data *ipd);
> +} ioc3_infos[] = {

IMHO it's neater if you separate the definition and static data part.

> + IOC3_SID("IP27 BaseIO6G", IP27_BASEIO6G, &ip27_baseio6g_setup),
> + IOC3_SID("IP27 MIO", IP27_MIO, &ip27_mio_setup),
> + IOC3_SID("IP27 BaseIO", IP27_BASEIO, &ip27_baseio_setup),
> + IOC3_SID("IP29 System Board", IP29_SYSBOARD, &ip29_sysboard_setup),
> + IOC3_SID("IP30 System Board", IP30_SYSBOARD, &ip30_sysboard_setup),
> + IOC3_SID("MENET", MENET, &ioc3_menet_setup),
> + IOC3_SID("MENET4", MENET4, &ioc3_menet4_setup)
> +};
> +
> +static int ioc3_setup(struct ioc3_priv_data *ipd)
> +{
> + u32 sid;
> + int i;
> +
> + /* Clear IRQs */
> + writel(~0, &ipd->regs->sio_iec);
> + writel(~0, &ipd->regs->sio_ir);
> + writel(0, &ipd->regs->eth.eier);
> + writel(~0, &ipd->regs->eth.eisr);
> +
> + /* read subsystem vendor id and subsystem id */
> + pci_read_config_dword(ipd->pdev, PCI_SUBSYSTEM_VENDOR_ID, &sid);
> +
> + for (i = 0; i < ARRAY_SIZE(ioc3_infos); i++)
> + if (sid == ioc3_infos[i].sid) {
> + pr_info("ioc3: %s\n", ioc3_infos[i].name);
> + return ioc3_infos[i].setup(ipd);
> + }
> +
> + /* treat everything not identified by PCI subid as CAD DUO */
> + pr_info("ioc3: CAD DUO\n");
> + return ioc3_cad_duo_setup(ipd);
> +}
> +
> +static int ioc3_mfd_probe(struct pci_dev *pdev,
> + const struct pci_device_id *pci_id)
> +{
> + struct ioc3_priv_data *ipd;
> + struct ioc3 __iomem *regs;
> + int ret;
> +
> + ret = pci_enable_device(pdev);
> + if (ret)
> + return ret;
> +
> + pci_write_config_byte(pdev, PCI_LATENCY_TIMER, 64);

What does 64 mean here? Define it perhaps?

> + pci_set_master(pdev);
> +
> + ret = dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(64));
> + if (ret) {
> + dev_warn(&pdev->dev, "Warning: couldn_t set 64-bit DMA mask\n");

Remove the "Warning:" part please.

"Failed to set 64-bit DMA mask - trying 32-bit" ?

> + ret = dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(32));
> + if (ret) {
> + dev_err(&pdev->dev, "Can't set DMA mask, aborting\n");
> + return ret;
> + }
> + }
> +
> + /* Set up per-IOC3 data */
> + ipd = devm_kzalloc(&pdev->dev, sizeof(struct ioc3_priv_data),
> + GFP_KERNEL);
> + if (!ipd) {
> + ret = -ENOMEM;
> + goto out_disable_device;
> + }
> + ipd->pdev = pdev;
> +
> + /*
> + * Map all IOC3 registers. These are shared between subdevices
> + * so the main IOC3 module manages them.
> + */
> + regs = devm_ioremap(&pdev->dev, pci_resource_start(pdev, 0),
> + pci_resource_len(pdev, 0));
> + if (!regs) {
> + pr_warn("ioc3: Unable to remap PCI BAR for %s.\n",
> + pci_name(pdev));

Why are you using pr_warn() here?

> + ret = -ENOMEM;
> + goto out_disable_device;
> + }
> + ipd->regs = regs;
> +
> + /* Track PCI-device specific data */
> + pci_set_drvdata(pdev, ipd);
> +
> + ret = ioc3_setup(ipd);
> + if (ret)
> + goto out_disable_device;
> +
> + return 0;
> +
> +out_disable_device:
> + pci_disable_device(pdev);
> + return ret;
> +}
> +
> +static void ioc3_mfd_remove(struct pci_dev *pdev)
> +{
> + struct ioc3_priv_data *ipd;
> +
> + ipd = pci_get_drvdata(pdev);
> +
> + /* Clear and disable all IRQs */
> + writel(~0, &ipd->regs->sio_iec);
> + writel(~0, &ipd->regs->sio_ir);
> +
> + /* Release resources */
> + if (ipd->domain) {
> + irq_domain_remove(ipd->domain);
> + free_irq(ipd->domain_irq, (void *)ipd);
> + }
> + pci_disable_device(pdev);
> +}
> +
> +static struct pci_device_id ioc3_mfd_id_table[] = {
> + { PCI_VENDOR_ID_SGI, PCI_DEVICE_ID_SGI_IOC3, PCI_ANY_ID, PCI_ANY_ID },
> + { 0, },
> +};
> +MODULE_DEVICE_TABLE(pci, ioc3_mfd_id_table);
> +
> +static struct pci_driver ioc3_mfd_driver = {
> + .name = "IOC3",
> + .id_table = ioc3_mfd_id_table,
> + .probe = ioc3_mfd_probe,
> + .remove = ioc3_mfd_remove,
> +};
> +
> +module_pci_driver(ioc3_mfd_driver);
> +
> +MODULE_AUTHOR("Thomas Bogendoerfer <tbogendoerfer@xxxxxxx>");
> +MODULE_DESCRIPTION("SGI IOC3 MFD driver");
> +MODULE_LICENSE("GPL");

GPL v2 ?

--
Lee Jones [æçæ]
Linaro Services Technical Lead
Linaro.org â Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog