From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from smtp.kernel.org (aws-us-west-2-korg-mail-1.web.codeaurora.org [10.30.226.201]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 7BF58239E7E; Thu, 23 Apr 2026 15:12:53 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=10.30.226.201 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1776957173; cv=none; b=u4P+lxfpZasLgbyOJioq8kU1Rx4sZ/hoZaukBMgcV/sPKn4UjIkjqdsDgDpwgmQRi86EwI7Reurn7Ph6zvN2vPx9es1kSneJRxHwv5JgUB9lXjcQqEeUwyJGENM0O5Th6cYpB38/VX2Q35oqy9nR1m8zW4opD7mT+1cPCI0TkNg= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1776957173; c=relaxed/simple; bh=v23S4mhg7PPhHzQyfCD39hHWUeLssiPPN2LPJOx+dJI=; h=Date:From:To:Cc:Subject:Message-ID:References:MIME-Version: Content-Type:Content-Disposition:In-Reply-To; b=rjcWHaU3dkH0sSIiQ+S+6kpd0nau+D8atLWqTXwSpG0OmpGnFpC22hqm6JU3eT8Yg0HJpwfHKpwuiQGYaYhVchoGyALmK89ELstW5JbpgDTy4uWf1T9jewSWw/qoaVleyyaZ8X8iUXCVbPAu4/yvRMosAYrqkSzCYuvhn5tam0Q= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org header.b=S76GQuyG; arc=none smtp.client-ip=10.30.226.201 Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org header.b="S76GQuyG" Received: by smtp.kernel.org (Postfix) with ESMTPSA id B8D05C2BCAF; Thu, 23 Apr 2026 15:12:49 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1776957173; bh=v23S4mhg7PPhHzQyfCD39hHWUeLssiPPN2LPJOx+dJI=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=S76GQuyGj28biDvP0F6FqXd04j+01mxkf7T6uWGPeupkGlMiyiiXffOS2C58URZs3 CdPTzTk2D3uIdMzxIs576ZpBkapSQRUUTZPbefgfem+G93gz9EgsmISvMwN0i6B7qZ b+QaafNVvBYyeFxc4YQAEdRJkpHn4rRbM1zzIa1ITe3NDY7Kb8MfO77TnQF9kkfrlr TD8vwUEGCXie9AoNToWrKWLIW4XPRuMkaFpxQ2+laxKUxWD+3DiLC8MCMLFSCJrMUe lUUONshV08NBdRoTXjTiceQh7rsSW1fB3dBCFoBBehFiDek5n+S55pM+f3w3PBE2fg dF7G87qRuwuGA== Date: Thu, 23 Apr 2026 16:12:46 +0100 From: Lee Jones To: Oleksij Rempel Cc: Guenter Roeck , Rob Herring , Krzysztof Kozlowski , Conor Dooley , Peter Rosin , Linus Walleij , kernel@pengutronix.de, linux-kernel@vger.kernel.org, devicetree@vger.kernel.org, linux-hwmon@vger.kernel.org, linux-gpio@vger.kernel.org, David Jander Subject: Re: [PATCH v11 2/6] mfd: add NXP MC33978/MC34978 core driver Message-ID: <20260423151246.GE170138@google.com> References: <20260402174349.3220518-1-o.rempel@pengutronix.de> <20260402174349.3220518-3-o.rempel@pengutronix.de> Precedence: bulk X-Mailing-List: linux-gpio@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20260402174349.3220518-3-o.rempel@pengutronix.de> On Thu, 02 Apr 2026, Oleksij Rempel wrote: > Add core Multi-Function Device (MFD) driver for the NXP MC33978 and > MC34978 Multiple Switch Detection Interfaces (MSDI). > > The MC33978/MC34978 devices provide 22 switch detection inputs, analog > multiplexing (AMUX), and comprehensive hardware fault detection. > > This core driver handles: > - SPI communications via a custom regmap bus to support the device's > pipelined two-frame MISO response requirement. > - Power sequencing for the VDDQ (logic) and VBATP (battery) regulators. > - Interrupt demultiplexing, utilizing an irq_domain to provide 22 virtual > IRQs for switch state changes and 1 virtual IRQ for hardware faults. > - Inline status harvesting from the SPI MSB to detect and trigger events > without requiring dedicated status register polling. > > Child devices (pinctrl, hwmon, mux) are instantiated by the core driver > from match data. > > Signed-off-by: Oleksij Rempel > --- > changes v11: > - Introduce synthetic wakeup bit to prevent hardware bit overloading and > avoid phantom transient faults. > - Add smp_mb__before_atomic() to ensure memory visibility before > updating harvested_flags. > - Update event processing to mask synthetic bits before evaluating > hardware fault conditions. > - Introduce fault_lock spinlock to protect fault state members. > - Replace manual memory barriers and READ_ONCE/WRITE_ONCE with > spinlock-protected access. > - Introduce irq_state_lock raw spinlock to protect IRQ state fields. > - Replace irq_lock mutex with raw spinlock in IRQ callbacks and event > handling paths. > - Include MC33978_REG_ENTER_LPM in volatile and precious ranges. > - Mark MC33978_REG_ENTER_LPM as readable to avoid regcache sync writes. > changes v10: > - Refactor IRQ setup into a common helper to unify the .map and .alloc > code paths. > - Enable hierarchical IRQ support to allow integration with the > pinctrl/GPIO child domain. > - Simplify event handling by removing redundant boolean return values > from internal handlers. > - Fix IRQ cleanup by disposing of all active mappings before removing > the IRQ domain. > - Standardize IRQ constants using MC33978_NUM_IRQS for consistent domain > sizing and bounds checks. > changes v9: > - Fix null irq_domain dereference from debugfs race by initializing IRQ domain > early before regmap initialization. > - Refactor mc33978_handle_fault_condition() to improve readability by keeping > variable declarations at the top and adding inline comments. > - Fix spurious transient fault events caused by redundant STAT_FAULT flags > during event loop. > - Fix spurious interrupt loops by explicitly returning -ENODATA in > mc33978_rx_decode() for registers without status bits. > - Validate hwirq bounds in mc33978_irq_domain_alloc() to prevent corruption > of irq_rise/irq_fall bitmasks by malformed device tree inputs. > - set DOMAIN_BUS_NEXUS > - Protect work on teardown > - remove IRQF_SHARED > changes v8: > - Fix TOCTOU race condition in SPI event harvesting loop by grabbing > harvested_flags before hardware reads. > - Fix broken hierarchical IRQ allocation by replacing > irq_domain_set_hwirq_and_chip() with irq_domain_set_info() and passing > the handle_simple_irq flow handler. > - Fix out-of-bounds stack read and endianness bug in for_each_set_bit() by > typing fired_pins as unsigned long instead of casting u32. > - Prevent DMA cacheline corruption by explicitly aligning rx_frame with > ____cacheline_aligned to separate it from tx_frame. > - Prevent spurious IRQs by verifying irq_find_mapping() returns non-zero > before calling handle_nested_irq(). > - Prevent missed transient hardware faults by explicitly evaluating > hw_flags in mc33978_handle_fault_condition(). > - Fix missing memory barrier in mc33978_harvest_status() with > smp_mb__after_atomic() to ensure harvested_flags visibility. > - Fix devres use-after-free teardown race by using INIT_WORK and a custom > cancel action after the IRQ domain is destroyed, instead of > devm_work_autocancel. > - Prevent spurious pin interrupts on boot by priming cached_pin_state via > a regmap_read() during probe before enabling IRQs. > - Implement .irq_set_wake callback to support system wake from > hardware faults and switch state changes. > changes v7: > - Fix event handling race condition with smp_mb() > - Replace INIT_WORK() with devm_work_autocancel() > changes v6: > - Remove the hardcoded bypass in irq_set_type to allow child drivers to > configure the FAULT line for edge-triggering. > - Implement software edge-detection for FAULT interrupt. > - Add MC33978_FAULT_ALARM_MASK to the shared header for child devices > - Use READ_ONCE() and WRITE_ONCE() for lockless shared state variables > (cached_pin_mask, irq_rise, irq_fall, bus_fault_active, > cached_fault_active) accessed across the SPI harvesting context and > the event worker. > - Add an if (hwirq < MC33978_NUM_PINS) guard in irq_mask() and > irq_unmask() to prevent the FAULT hwirq (22) from altering the > physical pin mask registers. > - Lowercase the error strings in dev_err_probe() > - Add inline comments explaining the irq_map fallback behavior > changes v5: > - no changes > changes v4: > - Removed .of_compatible strings from the mfd_cell arrays > changes v3: > - Select IRQ_DOMAIN_HIERARCHY in Kconfig > - Add .alloc and .free callbacks to irq_domain_ops to support hierarchical > IRQ domains > - Set IRQ_DOMAIN_FLAG_HIERARCHY flag on the core MFD irq_domain > - replace manual lock/unlock with guard() > changes v2: > - Rewrite the driver header comment > - Explicitly reject IRQ_TYPE_LEVEL_HIGH and IRQ_TYPE_LEVEL_LOW in > mc33978_irq_set_type() to correctly reflect the hardware's edge-only > interrupt capabilities. > - Pass the hardware fault IRQ to the hwmon child driver via mfd_cell > resources, rather than requiring the child to parse the parent's irq_domain. > - Ensure the Kconfig strictly depends on OF and SPI > --- > drivers/mfd/Kconfig | 15 + > drivers/mfd/Makefile | 2 + > drivers/mfd/mc33978.c | 1088 +++++++++++++++++++++++++++++++++++ > include/linux/mfd/mc33978.h | 95 +++ > 4 files changed, 1200 insertions(+) > create mode 100644 drivers/mfd/mc33978.c > create mode 100644 include/linux/mfd/mc33978.h > > diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig > index 7192c9d1d268..6dc9554822c9 100644 > --- a/drivers/mfd/Kconfig > +++ b/drivers/mfd/Kconfig > @@ -2566,6 +2566,21 @@ config MFD_UPBOARD_FPGA > To compile this driver as a module, choose M here: the module will be > called upboard-fpga. > > +config MFD_MC33978 > + tristate "NXP MC33978/MC34978 industrial input controller core" > + depends on OF > + depends on SPI > + select IRQ_DOMAIN_HIERARCHY > + select MFD_CORE > + select REGMAP > + help > + Support for the NXP MC33978/MC34978 industrial input controllers > + using the SPI interface. > + > + This driver provides common support for accessing the device. > + Additional drivers must be enabled in order to use the functionality > + of the device. > + > config MFD_MAX7360 > tristate "Maxim MAX7360 I2C IO Expander" > depends on I2C > diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile > index e75e8045c28a..dcd99315f683 100644 > --- a/drivers/mfd/Makefile > +++ b/drivers/mfd/Makefile > @@ -122,6 +122,8 @@ obj-$(CONFIG_MFD_MC13XXX) += mc13xxx-core.o > obj-$(CONFIG_MFD_MC13XXX_SPI) += mc13xxx-spi.o > obj-$(CONFIG_MFD_MC13XXX_I2C) += mc13xxx-i2c.o > > +obj-$(CONFIG_MFD_MC33978) += mc33978.o > + > obj-$(CONFIG_MFD_PF1550) += pf1550.o > > obj-$(CONFIG_MFD_NCT6694) += nct6694.o > diff --git a/drivers/mfd/mc33978.c b/drivers/mfd/mc33978.c > new file mode 100644 > index 000000000000..a64ae9178953 > --- /dev/null > +++ b/drivers/mfd/mc33978.c > @@ -0,0 +1,1088 @@ > +// SPDX-License-Identifier: GPL-2.0-only > +/* > + * Copyright (C) 2024 David Jander , Protonic Holland > + * Copyright (C) 2026 Oleksij Rempel , Pengutronix > + * > + * MC33978/MC34978 Multiple Switch Detection Interface - MFD Core Driver > + * > + * Driver Architecture: > + * This is the core MFD driver handling the physical SPI interface, power > + * management, and central interrupt routing. It instantiates the following > + * child devices: > + * - pinctrl: For GPIO read/write and wetting current configuration. > + * - hwmon: For hardware fault monitoring (tLIM, over/under-voltage). > + * - mux: For the 24-to-1 analog multiplexer (AMUX). > + * > + * Custom SPI Regmap & Event Harvesting: > + * The device uses a non-standard pipelined SPI protocol where the MISO > + * response logically lags the MOSI command by one frame. Furthermore, the > + * hardware embeds volatile global status bits (INT_flg, FAULT_STAT) into the > + * high byte of almost every SPI response (with specific exceptions handled by > + * the decoder). This core implements a custom regmap_bus to handle the > + * 2-frame dummy fetches and transparently "harvests" these status bits in > + * the background to schedule event processing. > + * > + * Interrupt Quirks & Limitations: > + * - Clear-on-Read: The physical INT_B line is directly tied to the INT_flg > + * bit. The hardware deasserts INT_B immediately upon *any* SPI transfer > + * that returns INT_flg. Harvesting this bit from all SPI traffic is the > + * ONLY way to know this device triggered an interrupt (crucial for shared > + * IRQ lines). > + * - Stateless Pin Edge Detection: The hardware lacks per-pin interrupt status > + * registers. To determine which pin triggered an event, the driver must > + * read the current pin states and XOR them against a previously cached state. > + * - Missed Short Pulses: Because pin interrupts are state-derived rather than > + * hardware-latched, very short physical pulses (shorter than the SPI read > + * latency) will be missed entirely if the pin reverts to its original state > + * before the READ_IN register is sampled by the IRQ thread. > + * - Edge-Only Pin Interrupts: The hardware only asserts INT_B on a state > + * change. It cannot continuously assert an interrupt while a pin is held at a > + * specific logic level. Consequently, the driver strictly emulates edge > + * interrupts (RISING/FALLING) and explicitly rejects LEVEL interrupt > + * configurations to prevent consumer misalignment. > + */ > + > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > + > +#include > + > +#define MC33978_DRV_NAME "mc33978" > + As this is only used in a few places, could we just use the string literal directly rather than defining a macro for it? > +/* Device identification signature returned by CHECK register */ > +#define MC33978_CHECK_SIGNATURE 0x123456 > + > +/* > + * Pipelined two-frame SPI transfer: > + * [REQ] - Transmits command/write-data, receives dummy/previous response > + * [PIPE] - Transmits dummy CHECK, receives actual response to current command > + */ > +enum mc33978_frame_index { > + MC33978_FRAME_REQ = 0, > + MC33978_FRAME_PIPE, > + MC33978_FRAME_COUNT > +}; > + > +/* SPI frame byte offsets (transmitted MSB first) */ > +enum mc33978_frame_offset { > + MC33978_FRAME_CMD = 0, > + MC33978_FRAME_DATA_HI, > + MC33978_FRAME_DATA_MID, > + MC33978_FRAME_DATA_LO > +}; > + > +#define MC33978_FRAME_LEN 4 > + > +/* Regmap internal value buffer offsets */ > +enum mc33978_payload_offset { > + MC33978_PAYLOAD_HI = 0, > + MC33978_PAYLOAD_MID, > + MC33978_PAYLOAD_LO > +}; > + > +#define MC33978_PAYLOAD_LEN 3 > + > +/* > + * SPI Command Byte (FRAME_CMD). > + * Maps to frame bit [24] in the datasheet. > + */ > +#define MC33978_CMD_BYTE_WRITE BIT(0) > + > +/* High Payload Byte Masks (FRAME_DATA_HI / PAYLOAD_HI). */ > +#define MC33978_HI_BYTE_STAT_FAULT BIT(7) /* Maps to frame bit [23] */ > +#define MC33978_HI_BYTE_STAT_INT BIT(6) /* Maps to frame bit [22] */ > + > +#define MC33978_HI_BYTE_STATUS_MASK (MC33978_HI_BYTE_STAT_FAULT | \ > + MC33978_HI_BYTE_STAT_INT) > + > +/* Synthetic wakeup bit for harvested flags */ > +#define MC33978_HARVEST_WAKE_BIT BIT(8) > + > +/* Maps to frame bits [21:16] */ > +#define MC33978_HI_BYTE_DATA_MASK GENMASK(5, 0) > + > +#define MC33978_CACHE_SG_PIN_MASK GENMASK(13, 0) > +#define MC33978_CACHE_SP_PIN_MASK GENMASK(21, 14) > + > +#define MC33978_SG_PIN_MASK GENMASK(13, 0) > +#define MC33978_SP_PIN_MASK GENMASK(7, 0) > + > +struct mc33978_data { > + const struct mfd_cell *cells; > + int num_cells; > +}; > + > +struct mc33978_mfd_priv { mc33978_ddata > + /* Immutable after initialization (no lock needed) */ > + struct spi_device *spi; > + struct regmap *map; > + struct regulator *vddq; > + struct regulator *vbatp; > + struct irq_domain *domain; > + > + /* Pre-built SPI messages (immutable after init) */ > + struct spi_message msg_read; > + struct spi_message msg_write; > + struct spi_transfer xfer_read[MC33978_FRAME_COUNT]; > + struct spi_transfer xfer_write; > + > + /* Protected by event_lock */ > + struct mutex event_lock; > + u32 cached_pin_state; /* Previous pin state for edge detection */ > + > + struct mutex irq_lock; > + The changelog for v11 mentions "Replace irq_lock mutex with raw spinlock". It seems 'irq_state_lock' has taken over the role for the state fields, but this 'irq_lock' is still used in the '.irq_bus_lock' and '.irq_bus_sync_unlock' callbacks. Was this mutex intended to be removed? > + /* Protected by irq_state_lock */ > + raw_spinlock_t irq_state_lock; > + u32 cached_pin_mask; /* IRQ mask for 22 pins */ > + u32 irq_rise; /* Rising edge IRQ enable mask */ > + u32 irq_fall; /* Falling edge IRQ enable mask */ > + > + /* Protected by teardown_lock */ > + spinlock_t teardown_lock; > + bool tearing_down; /* Prevents work scheduling during teardown */ > + > + /* Protected by fault_lock */ > + spinlock_t fault_lock; > + bool bus_fault_active; /* Latest physical fault state on bus */ > + bool cached_fault_active; /* Cached fault state from previous event */ > + > + /* Atomic operations (no lock needed) */ > + atomic_t harvested_flags; /* Status bits from SPI responses */ > + > + /* > + * Work scheduling protected by teardown_lock. > + * Work execution serialized by workqueue subsystem. > + */ > + struct work_struct event_work; > + > + /* > + * DMA buffers protected by SPI subsystem + regmap serialization. > + * Modified before spi_sync(), read after it returns. > + * Must be at end for ____cacheline_aligned. > + */ > + This comment seems a little misleading. The '____cacheline_aligned' attribute can be applied to any member of a struct to align its start address, not just the last one. > + u8 tx_frame[MC33978_FRAME_COUNT][MC33978_FRAME_LEN] ____cacheline_aligned; > + u8 rx_frame[MC33978_FRAME_COUNT][MC33978_FRAME_LEN] ____cacheline_aligned; > +}; > + > +static void mc33978_irq_mask(struct irq_data *data) > +{ > + struct mc33978_mfd_priv *mc = irq_data_get_irq_chip_data(data); mc => ddata, throughout. > + irq_hw_number_t hwirq = irqd_to_hwirq(data); > + > + if (hwirq < MC33978_NUM_PINS) { > + scoped_guard(raw_spinlock_irqsave, &mc->irq_state_lock) > + mc->cached_pin_mask &= ~BIT(hwirq); > + } > +} > + > +static void mc33978_irq_unmask(struct irq_data *data) > +{ > + struct mc33978_mfd_priv *mc = irq_data_get_irq_chip_data(data); > + irq_hw_number_t hwirq = irqd_to_hwirq(data); > + > + if (hwirq < MC33978_NUM_PINS) { > + scoped_guard(raw_spinlock_irqsave, &mc->irq_state_lock) > + mc->cached_pin_mask |= BIT(hwirq); > + } > +} > + > +static void mc33978_irq_bus_lock(struct irq_data *data) > +{ > + struct mc33978_mfd_priv *mc = irq_data_get_irq_chip_data(data); > + > + mutex_lock(&mc->irq_lock); > +} > + > +/** Kerneldoc is usually for exported functions. > + * mc33978_irq_bus_sync_unlock() - Sync cached IRQ mask to hardware and unlock > + * @data: IRQ data > + * > + * Writes the cached interrupt mask to the hardware IE_SG and IE_SP registers, > + * then releases the IRQ lock. This is where the actual hardware update occurs > + * after mask/unmask operations. > + */ > +static void mc33978_irq_bus_sync_unlock(struct irq_data *data) > +{ > + struct mc33978_mfd_priv *mc = irq_data_get_irq_chip_data(data); > + u32 sg_mask, sp_mask, cached_mask; > + int ret; > + > + scoped_guard(raw_spinlock_irqsave, &mc->irq_state_lock) > + cached_mask = mc->cached_pin_mask; > + > + /* > + * Split the cached 22-bit pin mask into hardware register format: > + * - SG pins: bits [13:0] (14 pins, mask 0x3FFF) > + * - SP pins: bits [21:14] (8 pins, mask 0xFF) > + */ > + sg_mask = FIELD_GET(MC33978_CACHE_SG_PIN_MASK, cached_mask); > + sp_mask = FIELD_GET(MC33978_CACHE_SP_PIN_MASK, cached_mask); > + > + ret = regmap_update_bits(mc->map, MC33978_REG_IE_SG, > + MC33978_SG_PIN_MASK, sg_mask); I'd encourage you to use 100-chars throughout on new drivers. > + if (ret) > + goto unlock; > + > + ret = regmap_update_bits(mc->map, MC33978_REG_IE_SP, > + MC33978_SP_PIN_MASK, sp_mask); > +unlock: > + > + if (ret) > + dev_err(&mc->spi->dev, "failed to sync IRQ mask to hardware: %d\n", > + ret); > + > + mutex_unlock(&mc->irq_lock); > +} > + > +static int mc33978_irq_set_type(struct irq_data *data, unsigned int type) > +{ > + struct mc33978_mfd_priv *mc = irq_data_get_irq_chip_data(data); > + irq_hw_number_t hwirq = irqd_to_hwirq(data); > + u32 mask = BIT(hwirq); > + > + if (type & (IRQ_TYPE_LEVEL_HIGH | IRQ_TYPE_LEVEL_LOW)) > + return -EINVAL; > + > + scoped_guard(raw_spinlock_irqsave, &mc->irq_state_lock) { > + if (type & IRQ_TYPE_EDGE_RISING) > + mc->irq_rise |= mask; > + else > + mc->irq_rise &= ~mask; > + > + if (type & IRQ_TYPE_EDGE_FALLING) > + mc->irq_fall |= mask; > + else > + mc->irq_fall &= ~mask; > + } > + > + return 0; > +} > + > +static int mc33978_irq_set_wake(struct irq_data *data, unsigned int on) > +{ > + struct mc33978_mfd_priv *mc = irq_data_get_irq_chip_data(data); > + > + return irq_set_irq_wake(mc->spi->irq, on); > +} > + > +static struct irq_chip mc33978_irq_chip = { > + .name = MC33978_DRV_NAME, > + .irq_mask = mc33978_irq_mask, > + .irq_unmask = mc33978_irq_unmask, > + .irq_bus_lock = mc33978_irq_bus_lock, > + .irq_bus_sync_unlock = mc33978_irq_bus_sync_unlock, > + .irq_set_type = mc33978_irq_set_type, > + .irq_set_wake = mc33978_irq_set_wake, > +}; > + > +static void mc33978_irq_setup(struct irq_domain *domain, unsigned int virq, > + irq_hw_number_t hwirq) > +{ > + struct mc33978_mfd_priv *mc = domain->host_data; > + > + irq_domain_set_info(domain, virq, hwirq, &mc33978_irq_chip, mc, > + handle_simple_irq, NULL, NULL); > + irq_set_nested_thread(virq, 1); > + irq_clear_status_flags(virq, IRQ_NOREQUEST | IRQ_NOPROBE); > +} > + > +static int mc33978_irq_map(struct irq_domain *d, unsigned int virq, > + irq_hw_number_t hw) > +{ > + mc33978_irq_setup(d, virq, hw); > + return 0; > +} > + > +static int mc33978_irq_domain_alloc(struct irq_domain *domain, > + unsigned int virq, > + unsigned int nr_irqs, void *arg) > +{ > + struct irq_fwspec *fwspec = arg; > + irq_hw_number_t hwirq; > + int i; > + > + if (fwspec->param_count < 1) > + return -EINVAL; > + > + hwirq = fwspec->param[0]; > + > + if (hwirq >= MC33978_NUM_IRQS || > + nr_irqs > MC33978_NUM_IRQS - hwirq) > + return -EINVAL; > + > + for (i = 0; i < nr_irqs; i++) > + mc33978_irq_setup(domain, virq + i, hwirq + i); > + > + return 0; > +} > + > +static void mc33978_irq_domain_free(struct irq_domain *domain, > + unsigned int virq, > + unsigned int nr_irqs) > +{ > + int i; > + > + for (i = 0; i < nr_irqs; i++) > + irq_domain_reset_irq_data(irq_domain_get_irq_data(domain, > + virq + i)); > +} > + > +/* > + * IRQ domain operations for dual-mode interrupt allocation. > + * > + * This domain serves two types of consumers: > + * > + * 1. Direct MFD child (hwmon): > + * - Uses platform_get_irq() with DEFINE_RES_IRQ(MC33978_HWIRQ_FAULT) > + * - Calls irq_create_mapping(domain, hwirq) > + * - Invokes .map callback -> mc33978_irq_map() > + * > + * 2. Hierarchical child domain (pinctrl's GPIO IRQ chip): > + * - Pinctrl finds this domain via irq_find_matching_fwnode(DOMAIN_BUS_NEXUS) > + * - Creates GPIO IRQ domain with parent_domain = this domain > + * - External devices reference pinctrl as interrupt-parent in devicetree > + * - When GPIO-to-IRQ translation occurs, calls irq_domain_alloc_irqs() > + * - Chains up to parent (this domain), invokes .alloc callback > + * - See drivers/pinctrl/pinctrl-mc33978.c for hierarchical setup > + * > + * The .xlate callback translates devicetree interrupt specifiers (2-cell > + * format: > + * hwirq number 0-22, IRQ type flags) into kernel hwirq and type values. > + * > + * Both .map and .alloc perform similar initialization (set chip, handler, > + * flags) but are invoked by different IRQ subsystem code paths. > + * IRQ_DOMAIN_FLAG_HIERARCHY enables the .alloc path for the pinctrl > + * hierarchical chain. > + */ > +static const struct irq_domain_ops mc33978_irq_domain_ops = { > + .map = mc33978_irq_map, > + .alloc = mc33978_irq_domain_alloc, > + .free = mc33978_irq_domain_free, > + .xlate = irq_domain_xlate_twocell, > +}; > + > +static void mc33978_irq_domain_remove(void *data) > +{ > + struct mc33978_mfd_priv *mc = data; > + struct irq_domain *domain = mc->domain; > + int hwirq; > + > + for (hwirq = 0; hwirq < MC33978_NUM_IRQS; hwirq++) { > + unsigned int virq; > + > + virq = irq_find_mapping(domain, hwirq); > + if (virq) > + irq_dispose_mapping(virq); > + } > + > + irq_domain_remove(domain); > +} > + > +static void mc33978_handle_pin_changes(struct mc33978_mfd_priv *mc, > + unsigned int pin_state) > +{ > + unsigned long fired_pins = 0; > + u32 changed_pins; > + u32 rise, fall, pin_mask; > + int i; > + > + changed_pins = pin_state ^ mc->cached_pin_state; > + if (!changed_pins) > + return; > + > + mc->cached_pin_state = pin_state; > + > + scoped_guard(raw_spinlock_irqsave, &mc->irq_state_lock) { > + pin_mask = mc->cached_pin_mask; > + rise = mc->irq_rise; > + fall = mc->irq_fall; > + } > + > + changed_pins &= pin_mask; > + > + if (!changed_pins) > + return; > + > + fired_pins |= (changed_pins & pin_state) & rise; > + fired_pins |= (changed_pins & ~pin_state) & fall; > + > + for_each_set_bit(i, &fired_pins, MC33978_NUM_PINS) { > + int virq = irq_find_mapping(mc->domain, i); > + > + if (virq) > + handle_nested_irq(virq); > + } > +} > + > +static void mc33978_handle_fault_condition(struct mc33978_mfd_priv *mc, > + unsigned int hw_flags) > +{ > + bool fault_active, cached_fault, transient, changed; > + u32 rise, fall; > + int virq; > + > + scoped_guard(spinlock_irqsave, &mc->fault_lock) { > + fault_active = mc->bus_fault_active; > + cached_fault = mc->cached_fault_active; > + > + changed = fault_active ^ cached_fault; > + if (changed) > + mc->cached_fault_active = fault_active; > + } > + > + /* > + * A transient fault is a pulse that was caught by the clear-on-read > + * status flags, but is no longer physically active on the bus. > + */ > + transient = !changed && !fault_active && > + (hw_flags & MC33978_HARVEST_WAKE_BIT); > + > + if (!changed && !transient) > + return; > + > + scoped_guard(raw_spinlock_irqsave, &mc->irq_state_lock) { > + rise = mc->irq_rise; > + fall = mc->irq_fall; > + } > + > + virq = irq_find_mapping(mc->domain, MC33978_HWIRQ_FAULT); > + if (!virq) > + return; > + > + if (transient) { > + /* Transient pulse: trigger both edges if enabled */ > + if (rise & BIT(MC33978_HWIRQ_FAULT)) > + handle_nested_irq(virq); > + if (fall & BIT(MC33978_HWIRQ_FAULT)) > + handle_nested_irq(virq); > + } else if ((fault_active && (rise & BIT(MC33978_HWIRQ_FAULT))) || > + (!fault_active && (fall & BIT(MC33978_HWIRQ_FAULT)))) { > + /* Normal edge */ > + handle_nested_irq(virq); > + } > +} > + > +static void mc33978_process_single_event(struct mc33978_mfd_priv *mc) > +{ > + unsigned int harvested; > + unsigned int pin_state; > + int ret; > + > + /* > + * Grab harvested_flags BEFORE reading the hardware. If the read itself > + * or a concurrent SPI transfer harvests new flags, they will remain set > + * in harvested_flags and correctly trigger another pass of the event > + * loop. > + * > + * Note on Performance: This architecture intentionally forces a second > + * (redundant) SPI read of READ_IN during almost every interrupt event. > + * While SPI framework overhead (CS toggling, DMA setup, context > + * switches) makes this 4-byte transfer relatively costly, it is > + * mathematically necessary to guarantee no edge events are permanently > + * lost when a concurrent regmap access races with the IRQ thread, due > + * to the hardware's clear-on-read global INT_flg design. > + */ > + harvested = atomic_xchg(&mc->harvested_flags, 0); > + > + ret = regmap_read(mc->map, MC33978_REG_READ_IN, &pin_state); > + if (ret) > + dev_err_ratelimited(&mc->spi->dev, "failed to read pin state: %d\n", > + ret); > + else > + mc33978_handle_pin_changes(mc, pin_state); > + > + mc33978_handle_fault_condition(mc, harvested); > +} > + > +static void mc33978_handle_events(struct mc33978_mfd_priv *mc) > +{ > + guard(mutex)(&mc->event_lock); > + > + do { > + mc33978_process_single_event(mc); > + } while (atomic_read(&mc->harvested_flags) != 0); > +} What is all of this event handling? My first thoughts are that this doesn't belong in the MFD subsystem. > +static irqreturn_t mc33978_irq_thread(int irq, void *data) > +{ > + mc33978_handle_events(data); > + > + return IRQ_HANDLED; > +} > + > +static void mc33978_teardown(void *data) > +{ > + struct mc33978_mfd_priv *mc = data; > + > + /* > + * During the devres LIFO teardown window, the workqueue is canceled > + * before the regmap is destroyed. A concurrent debugfs regmap read > + * can trigger mc33978_harvest_status() and wrongly reschedule the > + * workqueue after it was already canceled. > + * > + * Flag the teardown state under a lock so the harvester atomically > + * checks and ignores status bits before scheduling new work. > + */ > + scoped_guard(spinlock_irqsave, &mc->teardown_lock) { > + mc->tearing_down = true; > + } > + > + cancel_work_sync(&mc->event_work); > +} > + > +static int mc33978_irq_init(struct mc33978_mfd_priv *mc, > + struct fwnode_handle *fwnode) > +{ > + struct device *dev = &mc->spi->dev; > + int ret; > + > + mutex_init(&mc->irq_lock); > + > + /* > + * Create IRQ domain with 23 interrupts: > + * - hwirq 0-21: Pin change interrupts (22 pins) > + * - hwirq 22: Fault interrupt (for hwmon driver) > + */ > + mc->domain = irq_domain_create_linear(fwnode, MC33978_NUM_IRQS, > + &mc33978_irq_domain_ops, mc); > + if (!mc->domain) > + return dev_err_probe(dev, -ENOMEM, "failed to create IRQ domain\n"); > + > + /* > + * Use DOMAIN_BUS_NEXUS to distinguish this intermediate demux domain > + * from child domains sharing the same fwnode. Matches the pattern used > + * by other MFD drivers (e.g., crystalcove). > + */ > + irq_domain_update_bus_token(mc->domain, DOMAIN_BUS_NEXUS); > + > + /* > + * Enable hierarchical IRQ domain support for pinctrl's GPIO IRQ chip. > + * See mc33978_irq_domain_ops for detailed architecture explanation. > + */ > + mc->domain->flags |= IRQ_DOMAIN_FLAG_HIERARCHY; > + > + ret = devm_add_action_or_reset(dev, mc33978_irq_domain_remove, mc); > + if (ret) > + return ret; > + > + return 0; > +} > + > +static void mc33978_event_work(struct work_struct *work) > +{ > + struct mc33978_mfd_priv *mc = > + container_of(work, struct mc33978_mfd_priv, event_work); > + > + mc33978_handle_events(mc); > +} > + > +/** > + * mc33978_harvest_status() - Collect status flags from SPI responses > + * @mc: Device private data > + * @status: Status bits (FAULT_STAT and INT_flg) from MISO frame > + * > + * Accumulates status flags harvested from SPI responses and schedules > + * event processing if not already in progress. Called by the SPI > + * read/write functions when status bits are detected in responses. > + */ > +static void mc33978_harvest_status(struct mc33978_mfd_priv *mc, int status) > +{ > + bool fault_active; > + > + fault_active = !!(status & MC33978_HI_BYTE_STAT_FAULT); > + > + scoped_guard(spinlock_irqsave, &mc->fault_lock) { > + mc->bus_fault_active = fault_active; > + > + /* > + * If the bus state changed from what the IRQ thread last > + * evaluated, wake it up using a synthetic software bit to avoid > + * overloading the hardware STAT_FAULT bit and causing phantom > + * transient faults. > + */ > + if (fault_active != mc->cached_fault_active) > + atomic_or(MC33978_HARVEST_WAKE_BIT, > + &mc->harvested_flags); > + } > + > + if (status & MC33978_HI_BYTE_STAT_INT) > + atomic_or(MC33978_HI_BYTE_STAT_INT, &mc->harvested_flags); > + > + /* Ensure harvested_flags is visible before checking teardown state */ > + smp_mb__after_atomic(); > + > + scoped_guard(spinlock_irqsave, &mc->teardown_lock) { > + if (!mc->tearing_down && atomic_read(&mc->harvested_flags)) > + schedule_work(&mc->event_work); > + } > +} > + > +/** > + * mc33978_prepare_messages() - Initialize the persistent SPI messages > + * @mc: Device private data > + * > + * Hardware pipelining constraints: > + * - Write (1 Frame): The device executes write commands immediately upon > + * CS de-assertion. No fetch frame is required. > + * - Read (2 Frames): The MISO response logically lags by one frame. > + * Frame 1 transmits the read request and toggles CS to latch it. > + * Frame 2 transmits a dummy CHECK command to fetch the actual payload. > + */ > +static void mc33978_prepare_messages(struct mc33978_mfd_priv *mc) > +{ > + /* --- Prepare Write Message (1 Frame) --- */ > + spi_message_init(&mc->msg_write); > + > + mc->xfer_write.tx_buf = mc->tx_frame[MC33978_FRAME_REQ]; > + mc->xfer_write.rx_buf = mc->rx_frame[MC33978_FRAME_REQ]; > + mc->xfer_write.len = MC33978_FRAME_LEN; > + > + spi_message_add_tail(&mc->xfer_write, &mc->msg_write); > + > + /* --- Prepare Read Message (2 Frames) --- */ > + spi_message_init(&mc->msg_read); > + > + /* Frame 1: Request */ > + mc->xfer_read[MC33978_FRAME_REQ].tx_buf = > + mc->tx_frame[MC33978_FRAME_REQ]; > + mc->xfer_read[MC33978_FRAME_REQ].rx_buf = > + mc->rx_frame[MC33978_FRAME_REQ]; > + mc->xfer_read[MC33978_FRAME_REQ].len = MC33978_FRAME_LEN; > + mc->xfer_read[MC33978_FRAME_REQ].cs_change = 1; /* Latch command */ > + > + /* Frame 2: Fetch (Dummy CHECK) */ > + mc->xfer_read[MC33978_FRAME_PIPE].tx_buf = > + mc->tx_frame[MC33978_FRAME_PIPE]; > + mc->xfer_read[MC33978_FRAME_PIPE].rx_buf = > + mc->rx_frame[MC33978_FRAME_PIPE]; > + mc->xfer_read[MC33978_FRAME_PIPE].len = MC33978_FRAME_LEN; > + > + /* Preload the dummy CHECK command statically */ > + mc->tx_frame[MC33978_FRAME_PIPE][MC33978_FRAME_CMD] = MC33978_REG_CHECK; > + > + spi_message_add_tail(&mc->xfer_read[MC33978_FRAME_REQ], &mc->msg_read); > + spi_message_add_tail(&mc->xfer_read[MC33978_FRAME_PIPE], &mc->msg_read); > +} > + > +/** > + * mc33978_rx_decode() - Decode MISO response frame and extract status > + * @rx_frame: Received SPI frame buffer (4 bytes) > + * @val_buf: Output buffer for regmap (exactly 3 bytes, optional) > + * > + * Translates the 4-byte SPI response into a 3-byte regmap payload. > + * Harvests the volatile INTflg and FAULT_STAT bits from the MSB. > + * > + * Note: MC33978_REG_CHECK, MC33978_REG_WET_SP, and MC33978_REG_WET_SG0 do not > + * contain fault status or interrupt flags. > + * > + * Return: Status bits if present, negative error code otherwise. > + */ > +static int mc33978_rx_decode(const u8 *rx_frame, u8 *val_buf) > +{ > + u8 cmd = rx_frame[MC33978_FRAME_CMD] & ~MC33978_CMD_BYTE_WRITE; > + bool has_status; > + u8 status = 0; > + > + switch (cmd) { > + case MC33978_REG_CHECK: > + case MC33978_REG_WET_SP: > + case MC33978_REG_WET_SG0: > + has_status = false; > + break; > + default: > + has_status = true; > + break; > + } > + > + if (has_status) > + status = rx_frame[MC33978_FRAME_DATA_HI] & > + MC33978_HI_BYTE_STATUS_MASK; > + > + if (val_buf) { > + memcpy(val_buf, &rx_frame[MC33978_FRAME_DATA_HI], > + MC33978_PAYLOAD_LEN); > + > + if (has_status) > + val_buf[MC33978_PAYLOAD_HI] &= MC33978_HI_BYTE_DATA_MASK; > + } > + > + return has_status ? status : -ENODATA; > +} > + > +static int mc33978_spi_write(void *ctx, const void *data, size_t count) > +{ > + struct mc33978_mfd_priv *mc = ctx; > + int status; > + int ret; > + > + if (count != MC33978_FRAME_LEN) > + return -EINVAL; > + > + memcpy(mc->tx_frame[MC33978_FRAME_REQ], data, MC33978_FRAME_LEN); > + > + ret = spi_sync(mc->spi, &mc->msg_write); > + if (ret) > + return ret; > + > + status = mc33978_rx_decode(mc->rx_frame[MC33978_FRAME_REQ], NULL); > + if (status >= 0) > + mc33978_harvest_status(mc, status); > + > + return 0; > +} > + > +static int mc33978_spi_read(void *ctx, const void *reg_buf, size_t reg_size, > + void *val_buf, size_t val_size) > +{ > + struct mc33978_mfd_priv *mc = ctx; > + int status_req, status_pipe; > + int ret; > + > + if (reg_size != 1 || val_size != MC33978_PAYLOAD_LEN) > + return -EINVAL; > + > + memset(&mc->tx_frame[MC33978_FRAME_REQ][MC33978_FRAME_DATA_HI], 0, > + MC33978_PAYLOAD_LEN); > + mc->tx_frame[MC33978_FRAME_REQ][MC33978_FRAME_CMD] = > + ((const u8 *)reg_buf)[0]; > + > + ret = spi_sync(mc->spi, &mc->msg_read); > + if (ret) > + return ret; > + > + status_req = mc33978_rx_decode(mc->rx_frame[MC33978_FRAME_REQ], NULL); > + status_pipe = mc33978_rx_decode(mc->rx_frame[MC33978_FRAME_PIPE], > + val_buf); > + > + if (status_req >= 0) > + mc33978_harvest_status(mc, status_req); > + if (status_pipe >= 0) > + mc33978_harvest_status(mc, status_pipe); > + > + return 0; > +} I get the impression that we're _doing_ too much in here. Anything beyond pulling a transport message together is too heavyweight for MFD. Again, I suggest moving the vast majority of this to somewhere like drivers/platform or similar. > +static const struct regmap_bus mc33978_regmap_bus = { > + .read = mc33978_spi_read, > + .write = mc33978_spi_write, > +}; > + > +static const struct regmap_range mc33978_volatile_range[] = { > + regmap_reg_range(MC33978_REG_ENTER_LPM, MC33978_REG_ENTER_LPM), > + regmap_reg_range(MC33978_REG_READ_IN, MC33978_REG_RESET), > +}; > + > +static const struct regmap_access_table mc33978_volatile_table = { > + .yes_ranges = mc33978_volatile_range, > + .n_yes_ranges = ARRAY_SIZE(mc33978_volatile_range), > +}; > + > +static const struct regmap_range mc33978_precious_range[] = { > + regmap_reg_range(MC33978_REG_ENTER_LPM, MC33978_REG_ENTER_LPM), > + regmap_reg_range(MC33978_REG_READ_IN, MC33978_REG_RESET), > +}; > + > +static const struct regmap_access_table mc33978_precious_table = { > + .yes_ranges = mc33978_precious_range, > + .n_yes_ranges = ARRAY_SIZE(mc33978_precious_range), > +}; > + > +/* > + * NOTE: Need to fake REG_ENTER_LPM, REG_IRQ and REG_RESET as readable, so > + * regcache will NOT write them on a cache sync. Sounds counterintuitive, but > + * marking a reg as "precious" or "volatile" is the only way to avoid this, > + * and that works only with readable regs. > + */ > +static const struct regmap_range mc33978_readable_range[] = { > + regmap_reg_range(MC33978_REG_CHECK, MC33978_REG_WET_SG1), > + regmap_reg_range(MC33978_REG_CWET_SP, MC33978_REG_ENTER_LPM), > + regmap_reg_range(MC33978_REG_AMUX_CTRL, MC33978_REG_RESET), > +}; > + > +static const struct regmap_access_table mc33978_readable_table = { > + .yes_ranges = mc33978_readable_range, > + .n_yes_ranges = ARRAY_SIZE(mc33978_readable_range), > +}; > + > +static const struct regmap_range mc33978_writable_range[] = { > + regmap_reg_range(MC33978_REG_CONFIG, MC33978_REG_WET_SG1), > + regmap_reg_range(MC33978_REG_CWET_SP, MC33978_REG_AMUX_CTRL), > + regmap_reg_range(MC33978_REG_IRQ, MC33978_REG_RESET), > +}; > + > +static const struct regmap_access_table mc33978_writable_table = { > + .yes_ranges = mc33978_writable_range, > + .n_yes_ranges = ARRAY_SIZE(mc33978_writable_range), > +}; > + > +static const struct regmap_config mc33978_regmap_config = { > + .name = MC33978_DRV_NAME, > + .reg_bits = 8, > + .val_bits = 24, > + .reg_stride = 2, > + .write_flag_mask = MC33978_CMD_BYTE_WRITE, > + .reg_format_endian = REGMAP_ENDIAN_BIG, > + .val_format_endian = REGMAP_ENDIAN_BIG, > + .use_single_read = true, > + .use_single_write = true, > + .volatile_table = &mc33978_volatile_table, > + .precious_table = &mc33978_precious_table, > + .rd_table = &mc33978_readable_table, > + .wr_table = &mc33978_writable_table, > + .cache_type = REGCACHE_MAPLE, > + .max_register = MC33978_REG_RESET, > +}; > + > +static int mc33978_power_on(struct mc33978_mfd_priv *mc) > +{ > + struct device *dev = &mc->spi->dev; > + int ret; > + > + ret = regulator_enable(mc->vddq); > + if (ret) > + return dev_err_probe(dev, ret, "failed to enable VDDQ supply\n"); > + > + ret = regulator_enable(mc->vbatp); > + if (ret) { > + regulator_disable(mc->vddq); > + return dev_err_probe(dev, ret, "failed to enable VBATP supply\n"); > + } > + > + return 0; > +} > + > +static void mc33978_power_off(void *data) > +{ > + struct mc33978_mfd_priv *mc = data; > + > + regulator_disable(mc->vbatp); > + regulator_disable(mc->vddq); > +} > + > +/** > + * mc33978_check_device() - Verify SPI communication with device > + * @mc: Device context > + * > + * Reads the CHECK register which should return a fixed signature (0x123456). > + * This verifies that SPI communication is working correctly. > + * > + * Note: MC33978_REG_CHECK does not contain fault status or interrupt flags. > + * See mc33978_rx_decode() for details. > + * > + * Return: 0 on success, -ENODEV if signature doesn't match > + */ > +static int mc33978_check_device(struct mc33978_mfd_priv *mc) > +{ > + struct device *dev = &mc->spi->dev; > + unsigned int check; > + int ret; > + > + ret = regmap_read(mc->map, MC33978_REG_CHECK, &check); > + if (ret) > + return ret; > + > + if (check != MC33978_CHECK_SIGNATURE) > + return dev_err_probe(dev, -ENODEV, > + "SPI check failed. Expected: 0x%06x, got: 0x%06x\n", > + MC33978_CHECK_SIGNATURE, check); > + > + return 0; > +} > + > +static const struct resource mc33978_hwmon_resources[] = { > + DEFINE_RES_IRQ(MC33978_HWIRQ_FAULT), > +}; > + > +static const struct mfd_cell mc33978_cells[] = { > + { .name = "mc33978-pinctrl" }, MFD_CELL_BASIC() > + { > + .name = "mc33978-hwmon", > + .resources = mc33978_hwmon_resources, > + .num_resources = ARRAY_SIZE(mc33978_hwmon_resources), > + }, MFD_CELL_RES() > + { .name = "mc33978-mux" }, > +}; > + > +static const struct mfd_cell mc34978_cells[] = { > + { .name = "mc34978-pinctrl" }, > + { > + .name = "mc34978-hwmon", > + .resources = mc33978_hwmon_resources, > + .num_resources = ARRAY_SIZE(mc33978_hwmon_resources), > + }, > + { .name = "mc34978-mux" }, > +}; As above. > +static const struct mc33978_data mc33978_match_data = { > + .cells = mc33978_cells, > + .num_cells = ARRAY_SIZE(mc33978_cells), > +}; > + > +static const struct mc33978_data mc34978_match_data = { > + .cells = mc34978_cells, > + .num_cells = ARRAY_SIZE(mc34978_cells), > +}; Remove these (see below). > +static int mc33978_probe(struct spi_device *spi) > +{ > + const struct mc33978_data *match_data; > + struct device *dev = &spi->dev; > + struct fwnode_handle *fwnode; > + struct mc33978_mfd_priv *mc; > + int ret; > + > + fwnode = dev_fwnode(dev); > + if (!fwnode) > + return dev_err_probe(dev, -ENODEV, "missing firmware node\n"); This isn't used here. Put it where it's used. > + match_data = spi_get_device_match_data(spi); > + if (!match_data) > + return dev_err_probe(dev, -ENODEV, "no device match data found\n"); > + > + mc = devm_kzalloc(dev, sizeof(*mc), GFP_KERNEL); > + if (!mc) > + return -ENOMEM; > + > + mc->spi = spi; > + spi_set_drvdata(spi, mc); > + > + mc->vddq = devm_regulator_get(dev, "vddq"); > + if (IS_ERR(mc->vddq)) > + return dev_err_probe(dev, PTR_ERR(mc->vddq), > + "failed to get VDDQ regulator\n"); > + > + mc->vbatp = devm_regulator_get(dev, "vbatp"); > + if (IS_ERR(mc->vbatp)) > + return dev_err_probe(dev, PTR_ERR(mc->vbatp), > + "failed to get VBATP regulator\n"); > + > + ret = mc33978_power_on(mc); > + if (ret) > + return ret; > + > + ret = devm_add_action_or_reset(dev, mc33978_power_off, mc); > + if (ret) > + return ret; > + > + mutex_init(&mc->event_lock); > + spin_lock_init(&mc->teardown_lock); > + spin_lock_init(&mc->fault_lock); > + raw_spin_lock_init(&mc->irq_state_lock); Do you really need 4 locks? Can't you reuse them? > + INIT_WORK(&mc->event_work, mc33978_event_work); > + > + atomic_set(&mc->harvested_flags, 0); > + > + mc33978_prepare_messages(mc); > + > + ret = mc33978_irq_init(mc, fwnode); > + if (ret) > + return ret; > + > + mc->map = devm_regmap_init(dev, &mc33978_regmap_bus, mc, > + &mc33978_regmap_config); > + if (IS_ERR(mc->map)) > + return dev_err_probe(dev, PTR_ERR(mc->map), "can't init regmap\n"); Don't shorten error messages. "failed to ..." usually reads better. > + /* > + * Ensure event_work is canceled before regmap and irq_domain teardown, > + * since the worker dereferences both mc->map and mc->domain. > + */ > + ret = devm_add_action_or_reset(dev, mc33978_teardown, mc); > + if (ret) > + return ret; > + > + ret = mc33978_check_device(mc); > + if (ret) > + return dev_err_probe(dev, ret, "can't use SPI bus\n"); > + mc33978_check_device() already prints a specific error with the received and expected values. This one seems superfluous. > + /* Disable interrupts to prevent storms during priming */ > + ret = regmap_write(mc->map, MC33978_REG_IE_SP, 0); > + if (ret) > + return ret; > + > + ret = regmap_write(mc->map, MC33978_REG_IE_SG, 0); > + if (ret) > + return ret; > + > + /* Prime the cached pin state under lock to prevent spurious events */ > + scoped_guard(mutex, &mc->event_lock) { > + ret = regmap_read(mc->map, MC33978_REG_READ_IN, > + &mc->cached_pin_state); > + } > + if (ret) > + return dev_err_probe(dev, ret, "failed to read initial pin state\n"); > + > + if (mc->spi->irq <= 0) > + return dev_err_probe(dev, -EINVAL, "no valid IRQ provided for INT_B pin\n"); > + > + /* > + * Deliberately not using IRQF_SHARED. > + * > + * MC33978 clear-on-read interrupt status can make shared wiring with > + * another MC33978/MC34978 functionally possible, but this handler runs > + * threaded with IRQF_ONESHOT and may hold the line masked for a long > + * time on slow SPI. The added latency/jitter makes shared operation > + * impractical. > + */ > + ret = devm_request_threaded_irq(dev, mc->spi->irq, > + NULL, > + mc33978_irq_thread, > + IRQF_ONESHOT, > + dev_name(dev), mc); > + if (ret) > + return dev_err_probe(dev, ret, "failed to request IRQ\n"); > + > + ret = devm_mfd_add_devices(dev, PLATFORM_DEVID_NONE, > + match_data->cells, match_data->num_cells, > + NULL, 0, mc->domain); > + if (ret) > + return dev_err_probe(dev, ret, "failed to add MFD child devices\n"); > + > + return 0; > +} > + > +static const struct of_device_id mc33978_of_match[] = { > + { .compatible = "nxp,mc33978", .data = &mc33978_match_data }, > + { .compatible = "nxp,mc34978", .data = &mc34978_match_data }, We don't allow MFD data to be passed through OF/ACPI/SPI/etc APIs. Pass a value to match on (usually in a switch()) instead. > + { } > +}; > +MODULE_DEVICE_TABLE(of, mc33978_of_match); > + > +static const struct spi_device_id mc33978_spi_id[] = { > + { "mc33978", (kernel_ulong_t)&mc33978_match_data }, > + { "mc34978", (kernel_ulong_t)&mc34978_match_data }, > + { } > +}; > +MODULE_DEVICE_TABLE(spi, mc33978_spi_id); > + > +static struct spi_driver mc33978_driver = { > + .driver = { > + .name = MC33978_DRV_NAME, > + .of_match_table = mc33978_of_match, > + }, > + .probe = mc33978_probe, > + .id_table = mc33978_spi_id, > +}; > +module_spi_driver(mc33978_driver); > + > +MODULE_AUTHOR("David Jander "); > +MODULE_DESCRIPTION("NXP MC33978/MC34978 MFD core driver"); > +MODULE_LICENSE("GPL"); > diff --git a/include/linux/mfd/mc33978.h b/include/linux/mfd/mc33978.h > new file mode 100644 > index 000000000000..06ff3c245093 > --- /dev/null > +++ b/include/linux/mfd/mc33978.h > @@ -0,0 +1,95 @@ > +/* SPDX-License-Identifier: GPL-2.0-only */ > +/* > + * Copyright (C) 2024 David Jander , Protonic Holland > + * Copyright (C) 2026 Oleksij Rempel , Pengutronix > + * > + * MC34978/MC33978 Multiple Switch Detection Interface - Shared Definitions > + */ > + > +#ifndef _LINUX_MFD_MC33978_H > +#define _LINUX_MFD_MC33978_H > + > +#include > + > +/* Register Map - All addresses are base command bytes (R/W bit = 0) */ > +#define MC33978_REG_CHECK 0x00 /* SPI communication check */ > +#define MC33978_REG_CONFIG 0x02 /* Device configuration */ > +#define MC33978_REG_TRI_SP 0x04 /* Tri-state enable SP */ > +#define MC33978_REG_TRI_SG 0x06 /* Tri-state enable SG */ > +#define MC33978_REG_WET_SP 0x08 /* Wetting current level SP */ > +#define MC33978_REG_WET_SG0 0x0a /* Wetting current level SG0 (SG7-SG0) */ > +#define MC33978_REG_WET_SG1 0x0c /* Wetting current level SG1 (SG13-SG8) */ > +#define MC33978_REG_CWET_SP 0x16 /* Continuous wetting current SP */ > +#define MC33978_REG_CWET_SG 0x18 /* Continuous wetting current SG */ > +#define MC33978_REG_IE_SP 0x1a /* Interrupt enable SP */ > +#define MC33978_REG_IE_SG 0x1c /* Interrupt enable SG */ > +#define MC33978_REG_LPM_CONFIG 0x1e /* Low-power mode configuration */ > +#define MC33978_REG_WAKE_SP 0x20 /* Wake-up enable SP */ > +#define MC33978_REG_WAKE_SG 0x22 /* Wake-up enable SG */ > +#define MC33978_REG_COMP_SP 0x24 /* Comparator only mode SP */ > +#define MC33978_REG_COMP_SG 0x26 /* Comparator only mode SG */ > +#define MC33978_REG_LPM_VT_SP 0x28 /* LPM voltage threshold SP */ > +#define MC33978_REG_LPM_VT_SG 0x2a /* LPM voltage threshold SG */ > +#define MC33978_REG_IP_SP 0x2c /* Polling current SP */ > +#define MC33978_REG_IP_SG 0x2e /* Polling current SG */ > +#define MC33978_REG_SPOLL_SP 0x30 /* Slow polling SP */ > +#define MC33978_REG_SPOLL_SG 0x32 /* Slow polling SG */ > +#define MC33978_REG_WDEB_SP 0x34 /* Wake-up debounce SP */ > +#define MC33978_REG_WDEB_SG 0x36 /* Wake-up debounce SG */ > +#define MC33978_REG_ENTER_LPM 0x38 /* Enter low-power mode (write-only) */ > +#define MC33978_REG_AMUX_CTRL 0x3a /* AMUX control */ > +#define MC33978_REG_READ_IN 0x3e /* Read switch status (READ_SW in datasheet) */ > +#define MC33978_REG_FAULT 0x42 /* Fault status register */ > +#define MC33978_REG_IRQ 0x46 /* Interrupt request (write-only) */ > +#define MC33978_REG_RESET 0x48 /* Reset (write-only) */ Tab all of these out so they align. > + > +/* > + * FAULT Register (0x42) bit definitions > + * Reading this register clears most fault flags except persistent conditions > + */ > +#define MC33978_FAULT_SPI_ERROR BIT(10) /* SPI communication error */ > +#define MC33978_FAULT_HASH BIT(9) /* SPI register hash mismatch */ > +#define MC33978_FAULT_UV BIT(7) /* VBATP undervoltage */ > +#define MC33978_FAULT_OV BIT(6) /* VBATP overvoltage */ > +#define MC33978_FAULT_TEMP_WARN BIT(5) /* Temperature warning threshold */ > +#define MC33978_FAULT_OT BIT(4) /* Over-temperature */ > +#define MC33978_FAULT_INTB_WAKE BIT(3) /* Woken by INT_B pin */ > +#define MC33978_FAULT_WAKEB_WAKE BIT(2) /* Woken by WAKE_B pin */ > +#define MC33978_FAULT_SPI_WAKE BIT(1) /* Woken by SPI message */ > +#define MC33978_FAULT_POR BIT(0) /* Power-on reset occurred */ > + > +/* Critical faults that need immediate attention */ > +#define MC33978_FAULT_CRITICAL (MC33978_FAULT_UV | \ > + MC33978_FAULT_OV | \ > + MC33978_FAULT_OT) > + > +/* Bits relevant as hwmon alarms; excludes wake/reset/SPI status bits */ > +#define MC33978_FAULT_ALARM_MASK (MC33978_FAULT_UV | \ > + MC33978_FAULT_OV | \ > + MC33978_FAULT_TEMP_WARN | \ > + MC33978_FAULT_OT) > + > +#define MC33978_NUM_PINS 22 > + > +/* > + * Virtual IRQ number for fault handling. > + * Using hwirq 22 (beyond the 22 pin IRQs 0-21). > + */ > +#define MC33978_HWIRQ_FAULT 22 > + > +/* Total number of hwirqs exposed by the MFD IRQ domain */ > +#define MC33978_NUM_IRQS (MC33978_HWIRQ_FAULT + 1) > + > +/* > + * AMUX channel definitions > + * The AMUX can route one of 24 signals to the external AMUX pin > + */ > +#define MC33978_AMUX_CH_SG0 0 /* Switch-to-Ground inputs 0-13 */ > +#define MC33978_AMUX_CH_SG13 13 > +#define MC33978_AMUX_CH_SP0 14 /* Programmable switch inputs 0-7 */ > +#define MC33978_AMUX_CH_SP7 21 > +#define MC33978_AMUX_CH_TEMP 22 /* Internal temperature diode */ > +#define MC33978_AMUX_CH_VBATP 23 /* Battery voltage sense */ > +#define MC33978_NUM_AMUX_CH 24 /* Total number of AMUX channels */ And everywhere else too. Without straight lines, I wouldn't sleep at night! > +#endif /* _LINUX_MFD_MC33978_H */ > -- > 2.47.3 >