From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from smtp.kernel.org (aws-us-west-2-korg-mail-alma10-1.taild15c8.ts.net [100.103.45.18]) (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 6EBFA25FA29 for ; Thu, 11 Jun 2026 16:36:16 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=100.103.45.18 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1781195778; cv=none; b=GKibIm2BoVG/zjKmdT2+Dr7DR1gEKQl27+aJqDX9ljqaQEsrc9pr2O1eKHDDROTvFZIoS+4eKHI9W7bxxVxyaGNB4GxqnX14qVkt5vTgRaHhOuvzaGuJWL3C8tCosvLT3sC5ljy1c79nohkzqvp/miLUbjxwL94FHYOmeMK4abw= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1781195778; c=relaxed/simple; bh=kZp+5X1ngNxvfcIFoQSG6CGo8rLZ5EOtIQPgPYS0+ro=; h=Date:From:To:Cc:Subject:Message-ID:References:MIME-Version: Content-Type:Content-Disposition:In-Reply-To; b=O9jbvkdQ+dDkFVIionS32rKKnbr8zeHMoChnzJXayVIG1IxG7W9mVZEfvg4OF9VgX314Tg8zHR/DdcqpNnf2Y9Orr3EmPX5r8BaethrboMZa4I9CW55JUamIs0kvxE/eZOZsLt8VgWXQG39ZxJLC98HbmD7vdpjfvkuV61A2O7s= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org header.b=JS9KowvZ; arc=none smtp.client-ip=100.103.45.18 Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org header.b="JS9KowvZ" Received: by smtp.kernel.org (Postfix) with ESMTPSA id 3AB081F00898; Thu, 11 Jun 2026 16:36:15 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=kernel.org; s=k20260515; t=1781195776; bh=HzAZ5SieHMV04MpSp0iK5thmfrpdd+B05W4FmA9zEIs=; h=Date:From:To:Cc:Subject:References:In-Reply-To; b=JS9KowvZoppf6yokgdER3wr8u0LnoyeNCU3+l86DLIWvyN7shoGc+xiFnMuy9z39k 6DEKZUk4F5qA6oU4rY+GDXxEm2or1JTJDbSis6u+Y1IAOks5fMhtVnLPnEOclgRGmf YaSRrUcKg6QeiUw+CnLs+hi3VZuGK7McXcTiqZ8m+dkSD695K6k+6jbrFT+vz4jbGA Hza9H1I1jDlG74bV67WcKy99BIxsIfR0EuQaftS5jEOWbR71RGJurmEw8wEOtB7sFr C7wg84fpOsGhq1MSxNfxPu0dqMFzy2jYSUgbV60PDKACXVpd5lM5P3TRh524ASASFB gkJFNo9vJw2DQ== Date: Thu, 11 Jun 2026 17:36:12 +0100 From: Lee Jones To: Herman van Hazendonk Cc: linux-kernel@vger.kernel.org Subject: Re: [PATCH v2 1/1] mfd: lm8502: add core MFD driver for TI LM8502 Message-ID: <20260611163612.GA1212816@google.com> References: <20260603035411.396383-1-github.com@herrie.org> <20260603040026.398009-1-github.com@herrie.org> <20260603040026.398009-2-github.com@herrie.org> Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <20260603040026.398009-2-github.com@herrie.org> Could you answer or fix this Sashiko review comments please? /* Sashiko Automation: Issues Found (3 Findings) */ On Wed, 03 Jun 2026, Herman van Hazendonk wrote: > The TI LM8502 is an I2C-attached combo device with ten constant- > current LED outputs (D1..D10) and an internal H-bridge that drives a > vibrator motor. Pin D10 is shared between the tenth LED channel and > the haptic output; the haptic child driver mux's D10 to the H-bridge > path at each FF_RUMBLE start. > > This commit adds the MFD core: i2c probe, regmap, optional vcc > regulator (with High Power Mode handshake for RPM-managed LDOs that > carry "regulator-allow-set-load"), optional chip-enable GPIO, software > reset, and the post-reset configuration sequence. Chip readiness is > verified inside probe using an LP55xx-style write-CHIP_EN-then- > readback loop bounded at 250 ms, so children only probe once the chip > is known to be ACKing the bus. > > Two children are spawned via devm_mfd_add_devices(): > > ti,lm8502-leds - LED-class outputs D1..D10 > ti,lm8502-haptic - EV_FF / FF_RUMBLE input device > > The corresponding child drivers land in subsequent patches. > > Also include MAINTAINERS entry covering all four driver files plus > the DT binding from the preceding patch. > > Signed-off-by: Herman van Hazendonk > --- > MAINTAINERS | 9 + > drivers/mfd/Kconfig | 18 ++ > drivers/mfd/Makefile | 1 + > drivers/mfd/lm8502.c | 511 +++++++++++++++++++++++++++++++++++++ > include/linux/mfd/lm8502.h | 130 ++++++++++ > 5 files changed, 669 insertions(+) > create mode 100644 drivers/mfd/lm8502.c > create mode 100644 include/linux/mfd/lm8502.h > > diff --git a/MAINTAINERS b/MAINTAINERS > index 2fb1c75afd16..3d83c03ff7ed 100644 > --- a/MAINTAINERS > +++ b/MAINTAINERS > @@ -26770,6 +26770,15 @@ S: Maintained > F: sound/soc/codecs/isabelle* > F: sound/soc/codecs/lm49453* > > +TI LM8502 MFD DRIVER (LED + HAPTIC) > +M: Herman van Hazendonk > +S: Maintained > +F: Documentation/devicetree/bindings/mfd/ti,lm8502.yaml > +F: drivers/input/misc/lm8502-haptic.c > +F: drivers/leds/leds-lm8502.c > +F: drivers/mfd/lm8502.c > +F: include/linux/mfd/lm8502.h > + > TI LMP92064 ADC DRIVER > M: Leonard Göhrs > R: kernel@pengutronix.de > diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig > index 7192c9d1d268..93018e5d2bd0 100644 > --- a/drivers/mfd/Kconfig > +++ b/drivers/mfd/Kconfig > @@ -2027,6 +2027,24 @@ config MFD_LM3533 > additional drivers must be enabled in order to use the LED, > backlight or ambient-light-sensor functionality of the device. > > +config MFD_LM8502 > + tristate "TI LM8502 combo LED + haptic controller" > + depends on I2C && OF > + select MFD_CORE > + select REGMAP_I2C > + help > + Say yes here to enable support for the TI LM8502 combo LED and > + haptic-feedback controller. The chip is used in the HP TouchPad > + tablet (Topaz and Opal). > + > + This driver provides the chip-wide MFD core (i2c probe, regmap, > + regulator High Power Mode handshake, enable GPIO, reset and > + configuration sequence, and PM suspend/resume). Per-subsystem > + drivers must be enabled separately: > + > + LEDS_LM8502 - LED-class outputs D1..D10 > + INPUT_LM8502_HAPTIC - FF_RUMBLE vibrator on the internal H-bridge > + > config MFD_TIMBERDALE > tristate "Timberdale FPGA" > select MFD_CORE > diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile > index e75e8045c28a..6d23c4dd6870 100644 > --- a/drivers/mfd/Makefile > +++ b/drivers/mfd/Makefile > @@ -241,6 +241,7 @@ obj-$(CONFIG_MFD_SEC_ACPM) += sec-acpm.o > obj-$(CONFIG_MFD_SEC_I2C) += sec-i2c.o > obj-$(CONFIG_MFD_SYSCON) += syscon.o > obj-$(CONFIG_MFD_LM3533) += lm3533-core.o lm3533-ctrlbank.o > +obj-$(CONFIG_MFD_LM8502) += lm8502.o > obj-$(CONFIG_MFD_VEXPRESS_SYSREG) += vexpress-sysreg.o > obj-$(CONFIG_MFD_RETU) += retu-mfd.o > obj-$(CONFIG_MFD_AS3711) += as3711.o > diff --git a/drivers/mfd/lm8502.c b/drivers/mfd/lm8502.c > new file mode 100644 > index 000000000000..4562f05b12bc > --- /dev/null > +++ b/drivers/mfd/lm8502.c > @@ -0,0 +1,511 @@ > +/* SPDX-License-Identifier: GPL-2.0-only */ > +/* > + * TI LM8502 Multi-Function Device core > + * > + * Copyright (C) 2008 Palm Inc. > + * Copyright (C) 2024 Christophe Chapuis > + * Copyright (C) 2024-2026 Herman van Hazendonk > + * > + * The LM8502 is a combo LED + haptic controller on a single I2C address > + * (0x33 on the HP TouchPad). This MFD core handles everything chip-wide: > + * > + * - the i2c_client and regmap > + * - the chip-enable GPIO (a TLMM line on tenderloin) > + * - the vcc regulator and the High Power Mode (HPM) handshake with > + * RPM-managed PMIC LDOs (PM8058_L16 on tenderloin) > + * - the post-reset chip configuration sequence (software reset, > + * ENGINE_CNTRL1/2, MISC) with explicit write-then-readback so > + * I2C controllers that do not surface NAKs (notably MSM8x60 QUP > + * in the immediate post-power-up window) fail fast in probe > + * rather than silently leaving the chip unconfigured > + * - PM suspend/resume of the whole chip via the CHIP_EN bit > + * > + * Per-subsystem code lives in child drivers: > + * > + * drivers/leds/leds-lm8502.c - LED class for outputs D1..D10 > + * drivers/input/misc/lm8502-haptic.c - EV_FF / FF_RUMBLE for the > + * internal H-bridge vibrator > + * > + * Both children are spawned via devm_mfd_add_devices() with explicit > + * .of_compatible matches so their DT subnodes drive per-subsystem > + * configuration (the LED children, ti,invert-direction, etc). > + */ > + > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > + > +/* Maximum time to wait for the chip to ACK reads/writes after power-up. */ > +#define LM8502_DETECT_TIMEOUT_MS 250 > +#define LM8502_DETECT_POLL_MS 5 > + > +/* Time the chip needs after RESET=0xFF before responding again. */ > +#define LM8502_RESET_SETTLE_MS 50 > + > +/* > + * After raising enable_gpio the chip's internal reset deasserts and > + * the I2C front-end becomes responsive; datasheet calls out ~1 ms. > + */ > +#define LM8502_ENABLE_SETTLE_US 1000 > + > +/* > + * regulator_set_load() on RPM-managed PMIC LDOs (PM8058_L16) only > + * sends an IPC; the rail change is asynchronous. 5 ms is the worst- > + * case RPM commit window measured on MSM8x60 silicon -- no polling > + * API exposes commit completion. > + */ > +#define LM8502_RPM_HPM_SETTLE_US 5000 > + > +/* Driver-private wrapper around the shared struct lm8502. */ > +struct lm8502_core { > + struct lm8502 chip; /* shared with children via parent drvdata */ > + > + /* Resources only the core touches: */ > + struct regulator *vcc; > + struct gpio_desc *enable_gpio; > +}; > + > +static bool lm8502_volatile_reg(struct device *dev, unsigned int reg) > +{ > + return true; /* every read goes to the bus */ > +} > + > +static const struct regmap_config lm8502_regmap_config = { > + .reg_bits = 8, > + .val_bits = 8, > + .max_register = 0xF0, > + .volatile_reg = lm8502_volatile_reg, > + .cache_type = REGCACHE_NONE, > +}; > + > +/* MFD children spawned by this core. */ > +static const struct mfd_cell lm8502_devs[] = { > + { > + .name = "lm8502-leds", > + .of_compatible = "ti,lm8502-leds", > + }, > + { > + .name = "lm8502-haptic", > + .of_compatible = "ti,lm8502-haptic", > + }, > +}; > + > +/* > + * Bring the chip out of reset and verify it is ACKing the bus. > + * > + * The post-power-up window is tricky on MSM8x60 QUP: writes complete > + * without -ENXIO even when the chip is not actually responding, so a > + * blind probe-time init silently writes into the void. We use the > + * LP55xx-family pattern: write CHIP_EN, read it back, retry until the > + * read matches or we time out. > + */ > +static int lm8502_wait_for_chip(struct lm8502 *chip) > +{ > + unsigned long deadline = jiffies + > + msecs_to_jiffies(LM8502_DETECT_TIMEOUT_MS); > + unsigned int val; > + int ret; > + > + /* > + * Use a wall-clock deadline rather than counting only sleep time: > + * regmap_{write,read}() round trips and any NAK/timeout cost on a > + * slow I2C bus can otherwise extend the wall-clock probe time > + * beyond LM8502_DETECT_TIMEOUT_MS without the loop noticing. > + */ > + do { > + ret = regmap_write(chip->regmap, LM8502_ENGINE_CNTRL1, > + LM8502_CHIP_EN); > + if (!ret) { > + ret = regmap_read(chip->regmap, LM8502_ENGINE_CNTRL1, > + &val); > + if (ret == 0 && (val & LM8502_CHIP_EN)) > + return 0; > + } > + > + fsleep(LM8502_DETECT_POLL_MS * USEC_PER_MSEC); > + } while (time_before(jiffies, deadline)); > + > + return -ENODEV; [Severity: Medium] Could this loop return -ENODEV spuriously if there is a long scheduling delay? If the thread is preempted or experiences a delay during fsleep() that exceeds the timeout, time_before() will be false and the loop will exit immediately. Should there be one final check of the hardware state after the loop terminates to prevent false negatives? > +} > + > +/* > + * Bring the chip up: software reset, prove the bus, then program the > + * baseline ENGINE_CNTRL2 / MISC / per-channel D_CONTROL registers. > + * > + * Errors are reported with plain dev_err() rather than dev_err_probe() > + * because this is also called from the resume path -- using > + * dev_err_probe() there would pollute the deferred-probe last-failure > + * tracker for a device that is already bound. Probe callers wrap the > + * return with dev_err_probe() themselves. > + */ > +static int lm8502_chip_init(struct lm8502_core *core) > +{ > + struct lm8502 *chip = &core->chip; > + struct device *dev = chip->dev; > + int ret, i; > + > + /* > + * Software reset. Writes through regmap; the chip may briefly > + * NAK reads in this window so we do not bother checking the > + * return value -- lm8502_wait_for_chip() proves bidirectional > + * communication below. > + */ > + regmap_write(chip->regmap, LM8502_RESET, LM8502_RESET_MAGIC); > + fsleep(LM8502_RESET_SETTLE_MS * USEC_PER_MSEC); > + > + ret = lm8502_wait_for_chip(chip); > + if (ret) { > + dev_err(dev, "chip did not respond: %d\n", ret); > + return ret; > + } > + > + ret = regmap_write(chip->regmap, LM8502_ENGINE_CNTRL2, > + LM8502_ENGINE_CNTRL2_INIT); > + if (ret) { > + dev_err(dev, "ENGINE_CNTRL2 write failed: %d\n", ret); > + return ret; > + } > + > + ret = regmap_write(chip->regmap, LM8502_MISC, LM8502_MISC_INIT); > + if (ret) { > + dev_err(dev, "MISC write failed: %d\n", ret); > + return ret; > + } > + > + /* > + * Program every D_CONTROL register so the LED child can issue > + * brightness writes to D_CURRENT_CTRL immediately. Without > + * this the channels stay at the post-reset default and ignore > + * current writes until the user manually configures them. > + */ > + for (i = 0; i < LM8502_MAX_LEDS; i++) { > + ret = regmap_write(chip->regmap, LM8502_D1_CONTROL + i, > + LM8502_LED_CONTROL_INIT); > + if (ret) { > + dev_err(dev, "D%d_CONTROL write failed: %d\n", > + i + 1, ret); > + return ret; > + } > + } > + > + return 0; > +} > + > +/* > + * devm-registered cleanup callback. Runs at unbind / probe-failure > + * time AFTER the MFD child platform devices spawned by > + * devm_mfd_add_devices() have been torn down (LIFO devres ordering -- > + * see lm8502_i2c_probe for the order in which actions and child > + * devices are registered). This is what makes the children's remove > + * callbacks issue their final regmap writes against a still-powered > + * chip rather than a clamped/disabled one. > + */ > +static void lm8502_power_off(void *data) > +{ > + struct lm8502_core *core = data; > + > + if (core->enable_gpio) > + gpiod_set_value_cansleep(core->enable_gpio, 0); > + > + if (core->vcc) { > + regulator_set_load(core->vcc, 0); > + regulator_disable(core->vcc); > + } > +} [Severity: Medium] Will the chip remain actively powered on after the driver is unbound if the GPIO isn't present and the regulator is shared? Unlike lm8502_suspend(), this function doesn't explicitly clear the LM8502_CHIP_EN bit over I2C. Does it make sense to clear LM8502_CHIP_EN here before disabling the regulator to prevent power leaks? > + > +static int lm8502_i2c_probe(struct i2c_client *client) > +{ > + struct device *dev = &client->dev; > + struct lm8502_core *core; > + struct lm8502 *chip; > + int ret; > + > + core = devm_kzalloc(dev, sizeof(*core), GFP_KERNEL); > + if (!core) > + return -ENOMEM; > + > + chip = &core->chip; > + chip->dev = dev; > + mutex_init(&chip->lock); > + > + chip->regmap = devm_regmap_init_i2c(client, &lm8502_regmap_config); > + if (IS_ERR(chip->regmap)) > + return dev_err_probe(dev, PTR_ERR(chip->regmap), > + "failed to allocate regmap\n"); > + > + /* > + * Optional vcc regulator. On the HP TouchPad this is PM8058_L16, > + * a 1.8 V always-on RPM-managed LDO with "regulator-allow-set-load" > + * so we can ask for High Power Mode below. > + */ > + core->vcc = devm_regulator_get_optional(dev, "vcc"); > + if (IS_ERR(core->vcc)) { > + ret = PTR_ERR(core->vcc); > + if (ret == -ENODEV) { > + core->vcc = NULL; > + } else { > + return dev_err_probe(dev, ret, > + "failed to get vcc supply\n"); > + } > + } > + > + if (core->vcc) { > + ret = regulator_enable(core->vcc); > + if (ret) > + return dev_err_probe(dev, ret, > + "failed to enable vcc supply\n"); > + > + /* > + * Ask for High Power Mode. The LM8502 internal boost converter > + * plus all ten LED outputs at up to 9 mA each draw ~100 mA > + * worst case. On RPM-managed LDOs (PM8058_L16) the default Low > + * Power Mode caps the regulator at ~1 mA, leaving the chip > + * current-starved. > + * > + * regulator_set_load() is a no-op on consumers whose supply > + * does not carry "regulator-allow-set-load", so this is safe > + * on platforms with simpler regulator topology. > + */ > + ret = regulator_set_load(core->vcc, 100000); > + if (ret) { > + dev_err_probe(dev, ret, > + "regulator_set_load(100mA) failed\n"); > + goto err_disable_vcc; > + } > + > + fsleep(LM8502_RPM_HPM_SETTLE_US); > + } > + > + core->enable_gpio = devm_gpiod_get_optional(dev, "enable", > + GPIOD_OUT_LOW); > + if (IS_ERR(core->enable_gpio)) { > + ret = PTR_ERR(core->enable_gpio); > + goto err_disable_vcc; > + } > + > + if (core->enable_gpio) { > + gpiod_set_value_cansleep(core->enable_gpio, 1); > + fsleep(LM8502_ENABLE_SETTLE_US); > + } > + > + dev_set_drvdata(dev, chip); > + > + ret = lm8502_chip_init(core); > + if (ret) { > + dev_err_probe(dev, ret, "chip init failed\n"); > + goto err_disable_chip; > + } > + > + /* > + * Register the power-off callback BEFORE devm_mfd_add_devices() so > + * devres unwinds LIFO: child platform devices teardown FIRST (their > + * .remove callbacks issue final regmap writes against a still- > + * powered chip), and lm8502_power_off() runs SECOND. Without this > + * ordering, lm8502_i2c_remove() / err_disable_chip would cut > + * CHIP_EN and the vcc rail before the children's cleanup writes > + * landed -- on MSM8x60 QUP the controller does not surface NAKs in > + * that window (see lm8502_wait_for_chip), so the writes are either > + * silently dropped or fail with -EREMOTEIO and the haptic motor > + * can be left energised. > + */ > + ret = devm_add_action_or_reset(dev, lm8502_power_off, core); > + if (ret) > + return dev_err_probe(dev, ret, > + "failed to register power-off action\n"); > + > + /* > + * Spawn LED and haptic child platform devices. Children retrieve > + * the shared struct lm8502 via dev_get_drvdata(pdev->dev.parent); > + * since chip_init above already completed, children can issue > + * register writes from their own probe paths immediately. > + */ > + ret = devm_mfd_add_devices(dev, PLATFORM_DEVID_NONE, lm8502_devs, > + ARRAY_SIZE(lm8502_devs), NULL, 0, NULL); > + if (ret) > + return dev_err_probe(dev, ret, > + "failed to add child devices\n"); > + > + return 0; > + > + /* > + * Failure paths below run BEFORE lm8502_power_off() is registered > + * via devm, so they must still tear the rail down manually. > + */ > +err_disable_chip: > + if (core->enable_gpio) > + gpiod_set_value_cansleep(core->enable_gpio, 0); > +err_disable_vcc: > + if (core->vcc) { > + regulator_set_load(core->vcc, 0); > + regulator_disable(core->vcc); > + } > + return ret; > +} > + > +static int lm8502_suspend(struct device *dev) > +{ > + struct lm8502 *chip = dev_get_drvdata(dev); > + struct lm8502_core *core = container_of(chip, struct lm8502_core, chip); > + int ret; > + > + mutex_lock(&chip->lock); > + if (chip->suspended) { > + mutex_unlock(&chip->lock); > + return 0; > + } > + > + /* > + * Mark the chip as suspended UP FRONT, before any register write > + * that can fail. The semantic is "system PM committed to suspend > + * us"; even if a midway regmap write returns -ENXIO and the rest > + * of system suspend continues without us, the resume path must > + * still run the full re-init sequence because the rails / GPIO > + * may have been power-cycled across S3. Without this, a single > + * I2C error during suspend would leave chip->suspended == false, > + * lm8502_resume() would early-out, and the children's first > + * register access after resume would hit an unconfigured chip. > + */ > + chip->suspended = true; > + > + /* Stop the haptic output in case a child left it running. */ > + ret = regmap_write(chip->regmap, LM8502_HAPTIC_FEEDBACK_CTRL, 0); > + if (ret) { > + dev_err(dev, "suspend: HAPTIC_FEEDBACK_CTRL write failed: %d\n", > + ret); > + goto unlock; > + } > + > + /* Drop CHIP_EN to power-gate the analog side. */ > + ret = regmap_update_bits(chip->regmap, LM8502_ENGINE_CNTRL1, > + LM8502_CHIP_EN, 0); > + if (ret) { > + dev_err(dev, "suspend: CHIP_EN clear failed: %d\n", ret); > + goto unlock; > + } > + > + if (core->enable_gpio) > + gpiod_set_value_cansleep(core->enable_gpio, 0); > + > + /* > + * Drop the regulator to Low Power Mode now that the chip is > + * off; HPM is only needed while the analog stage draws ~100 mA, > + * and leaving it set wastes power across system suspend on > + * RPM-managed PMIC LDOs. Restored in resume. > + */ > + if (core->vcc) > + regulator_set_load(core->vcc, 0); > + > + mutex_unlock(&chip->lock); > + > + return 0; > + > +unlock: > + mutex_unlock(&chip->lock); > + return ret; [Severity: High] If regmap_write() fails, we jump to unlock and return an error without resetting chip->suspended to false. Since returning an error from a suspend callback causes the PM core to abort the global suspend transition without calling resume() on this device, won't chip->suspended stay true indefinitely? Child drivers might then be permanently locked out of the hardware until module reload. Should chip->suspended be reverted on the error path? > +} > + > +static int lm8502_resume(struct device *dev) > +{ > + struct lm8502 *chip = dev_get_drvdata(dev); > + struct lm8502_core *core = container_of(chip, struct lm8502_core, chip); > + int ret; > + > + mutex_lock(&chip->lock); > + if (!chip->suspended) { > + mutex_unlock(&chip->lock); > + return 0; > + } > + > + /* > + * Restore High Power Mode so the regulator can supply the > + * ~100 mA the analog stage draws under load. Symmetric with the > + * drop in suspend; safe no-op on consumers without > + * regulator-allow-set-load. > + */ > + if (core->vcc) { > + ret = regulator_set_load(core->vcc, 100000); > + if (ret) { > + dev_err(dev, "resume: regulator_set_load(HPM) failed: %d\n", > + ret); > + goto unlock; > + } > + fsleep(LM8502_RPM_HPM_SETTLE_US); > + } > + > + if (core->enable_gpio) { > + gpiod_set_value_cansleep(core->enable_gpio, 1); > + fsleep(LM8502_ENABLE_SETTLE_US); > + } > + > + /* > + * Suspend de-asserts the enable_gpio, which on most boards power- > + * gates the chip and clears its register state. Re-run the full > + * init sequence so ENGINE_CNTRL1/2, MISC and the per-channel > + * D_CONTROL registers are restored before any child driver > + * touches the chip. lm8502_chip_init() also sets CHIP_EN as a > + * side-effect of programming ENGINE_CNTRL1. Errors are already > + * logged with dev_err() inside lm8502_chip_init(). > + */ > + ret = lm8502_chip_init(core); > + if (ret) > + goto err_disable; > + > + chip->suspended = false; > + mutex_unlock(&chip->lock); > + > + return 0; > + > +err_disable: > + /* > + * chip_init failed; we already raised enable_gpio and committed > + * HPM. Undo what resume did: de-assert enable_gpio and drop > + * the regulator back to Low Power Mode. The vcc regulator is > + * intentionally NOT disabled -- it was never disabled during > + * the preceding suspend either, so the device returns to the > + * same suspended state it was in before resume started rather > + * than a fully-off state. > + */ > + if (core->enable_gpio) > + gpiod_set_value_cansleep(core->enable_gpio, 0); > + if (core->vcc) > + regulator_set_load(core->vcc, 0); > +unlock: > + mutex_unlock(&chip->lock); > + return ret; [Severity: High] If regmap_write() fails, we jump to unlock and return an error without resetting chip->suspended to false. Since returning an error from a suspend callback causes the PM core to abort the global suspend transition without calling resume() on this device, won't chip->suspended stay true indefinitely? Child drivers might then be permanently locked out of the hardware until module reload. Should chip->suspended be reverted on the error path? > +} > + > +static DEFINE_SIMPLE_DEV_PM_OPS(lm8502_pm_ops, lm8502_suspend, lm8502_resume); > + > +static const struct of_device_id lm8502_of_match[] = { > + { .compatible = "ti,lm8502" }, > + {} > +}; > +MODULE_DEVICE_TABLE(of, lm8502_of_match); > + > +static const struct i2c_device_id lm8502_i2c_id[] = { > + { "lm8502" }, > + {} > +}; > +MODULE_DEVICE_TABLE(i2c, lm8502_i2c_id); > + > +static struct i2c_driver lm8502_i2c_driver = { > + .driver = { > + .name = "lm8502", > + .of_match_table = lm8502_of_match, > + .pm = pm_sleep_ptr(&lm8502_pm_ops), > + }, > + .probe = lm8502_i2c_probe, > + .id_table = lm8502_i2c_id, > +}; > +module_i2c_driver(lm8502_i2c_driver); > + > +MODULE_DESCRIPTION("TI LM8502 combo LED + haptic controller (MFD core)"); > +MODULE_AUTHOR("Herman van Hazendonk "); > +MODULE_LICENSE("GPL"); > diff --git a/include/linux/mfd/lm8502.h b/include/linux/mfd/lm8502.h > new file mode 100644 > index 000000000000..9c7c90cdd273 > --- /dev/null > +++ b/include/linux/mfd/lm8502.h > @@ -0,0 +1,130 @@ > +/* SPDX-License-Identifier: GPL-2.0-only */ > +/* > + * TI LM8502 Multi-Function Device > + * > + * The LM8502 is a combo LED + haptic controller exposed over I2C. The > + * MFD core handles the i2c_client, regmap, regulator and chip-wide > + * initialization; per-subsystem child platform drivers in drivers/leds/ > + * and drivers/input/misc/ bind to the spawned children and use the > + * parent's regmap. > + * > + * Children retrieve the parent's lm8502 state via > + * struct lm8502 *chip = dev_get_drvdata(pdev->dev.parent); > + * and serialise register access on chip->lock. > + */ > +#ifndef __LINUX_MFD_LM8502_H > +#define __LINUX_MFD_LM8502_H > + > +#include > +#include > +#include > + > +/* --- Register map --------------------------------------------------- */ > + > +#define LM8502_ENGINE_CNTRL1 0x00 > +#define LM8502_ENGINE_CNTRL2 0x01 > +#define LM8502_GROUP_FADING1 0x02 > +#define LM8502_GROUP_FADING2 0x03 > +#define LM8502_GROUP_FADING3 0x04 > + > +#define LM8502_D1_CONTROL 0x06 > +#define LM8502_D2_CONTROL 0x07 > +#define LM8502_D3_CONTROL 0x08 > +#define LM8502_D4_CONTROL 0x09 > +#define LM8502_D5_CONTROL 0x0A > +#define LM8502_D6_CONTROL 0x0B > +#define LM8502_D7_CONTROL 0x0C > +#define LM8502_D8_CONTROL 0x0D > +#define LM8502_D9_CONTROL 0x0E > +#define LM8502_D10_CONTROL 0x0F > + > +#define LM8502_HAPTIC_CONTROL 0x10 > +#define LM8502_HAPTIC_FEEDBACK_CTRL 0x21 > +#define LM8502_HAPTIC_PWM_DUTY_CYCLE 0x22 > + > +#define LM8502_D1_CURRENT_CTRL 0x26 > +#define LM8502_D2_CURRENT_CTRL 0x27 > +#define LM8502_D3_CURRENT_CTRL 0x28 > +#define LM8502_D4_CURRENT_CTRL 0x29 > +#define LM8502_D5_CURRENT_CTRL 0x2A > +#define LM8502_D6_CURRENT_CTRL 0x2B > +#define LM8502_D7_CURRENT_CTRL 0x2C > +#define LM8502_D8_CURRENT_CTRL 0x2D > +#define LM8502_D9_CURRENT_CTRL 0x2E > +#define LM8502_D10_CURRENT_CTRL 0x2F > + > +#define LM8502_MISC 0x36 > +#define LM8502_ENGINE1_PC 0x37 > +#define LM8502_ENGINE2_PC 0x38 > +#define LM8502_STATUS 0x3A > +#define LM8502_INT 0x3B > +#define LM8502_RESET 0x3D > + > +/* ENGINE_CNTRL1 bits */ > +#define LM8502_CHIP_EN BIT(6) > + > +/* ENGINE_CNTRL2 — value applied at chip_init: charge-pump CP_MODE = 1x. > + * Bit 5 selects the boost-converter compare mode; the value the legacy > + * legacy vendor driver writes matches the LP55xx-family ENGINE_CNTRL2 "all > + * engines in HOLD" default plus CP_MODE_1X. > + */ > +#define LM8502_ENGINE_CNTRL2_INIT 0x20 > + > +/* MISC (0x36) bits applied at chip_init */ > +#define LM8502_MISC_PWM_INPUT BIT(1) /* PWM input mux on D10 */ > +/* BOOST_EN: internal boost converter -- REQUIRED for LED current. */ > +#define LM8502_MISC_BOOST_EN BIT(3) > +#define LM8502_MISC_POWER_SAVE BIT(5) /* clock gate idle blocks */ > +#define LM8502_MISC_INIT (LM8502_MISC_POWER_SAVE | \ > + LM8502_MISC_BOOST_EN | \ > + LM8502_MISC_PWM_INPUT) > + > +/* D_CONTROL bits */ > +#define LM8502_LED_MAPPING_MASK 0x03 > +#define LM8502_LED_MAPPING_DIRECT 0x00 /* drive from current reg */ > +#define LM8502_LED_MAX_CURRENT_MASK 0x18 > +#define LM8502_LED_MAX_CURRENT_SHIFT 3 > +#define LM8502_LED_MAX_CURRENT_9MA 0x02 /* field = 2 -> 9 mA cap */ > +/* > + * Initial D_CONTROL value: MAX_CURRENT field = 2 (9 mA), DIRECT > + * mapping. Programmed for every channel during chip_init so a child > + * brightness write to D_CURRENT_CTRL is honoured immediately. > + */ > +#define LM8502_LED_CONTROL_INIT (LM8502_LED_MAX_CURRENT_9MA << \ > + LM8502_LED_MAX_CURRENT_SHIFT) > + > +/* HAPTIC_FEEDBACK_CTRL bits */ > +#define LM8502_HAPTIC_FB_INVERT_DIR BIT(0) /* H-bridge polarity flip */ > +#define LM8502_HAPTIC_FB_ENABLE BIT(1) /* drive the motor */ > + > +#define LM8502_MAX_LEDS 10 > + > +/* RESET (0x3D) magic write value (LP55xx-family software reset). */ > +#define LM8502_RESET_MAGIC 0xFF > + > +/* --- Shared parent state -------------------------------------------- */ > + > +/** > + * struct lm8502 - parent (MFD core) state shared with child drivers > + * @dev: the I2C client's struct device (parent of every child pdev) > + * @regmap: the chip's 8-bit register map (no cache; every read goes > + * to the bus) > + * @lock: serialises register sequences across children and against > + * the parent's PM callbacks > + * @suspended: set true while the chip is power-gated by parent suspend; > + * children must check it under @lock before touching > + * registers > + * > + * Chip readiness is implicit: child platform devices only probe after > + * the MFD parent's probe has completed lm8502_chip_init() successfully, > + * so by the time a child runs the chip is fully configured and ACKing > + * I2C. There is no separate "initialized" flag. > + */ > +struct lm8502 { > + struct device *dev; > + struct regmap *regmap; > + struct mutex lock; > + bool suspended; > +}; > + > +#endif /* __LINUX_MFD_LM8502_H */ > -- > 2.43.0 > -- Lee Jones