From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from polaris.svanheule.net (polaris.svanheule.net [84.16.241.116]) (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 CC76D32573D for ; Thu, 13 Nov 2025 21:33:31 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=84.16.241.116 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1763069613; cv=none; b=jAK1zGQTDyyFv5BCg2DSSaN22WZNi19shdrPgZJ8lGSI2v3gQDz9/ZtQjHI+SNyZ3qwxHFlCqb3RQh/alY3F4A++GswAPlW3Pu6/jiBOyqCOZlm0ycEjefCGIAjhrhDTD2J3LThVuD7aHaTomJjmHcDcc2CrFqOUw16yAaiNNRY= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1763069613; c=relaxed/simple; bh=dOsmlXAaLYzJ3dRoD+Ceh7/hl+sEnM6GJpMWKUQ/PyE=; h=Message-ID:Subject:From:To:Cc:Date:In-Reply-To:References: Content-Type:MIME-Version; b=NjZ1jT3wbyQOmRmUReN3rsSMU34imB/J14MgnqBH7eZsqqLHQGvpFW3B3AHt3tZOIuQE+XfQeVPU2LftsxdvVgiPbVdOIwiuj8JHssj7SLsvjgDRhJHuj5VQS5MRXdbay9gSsxG8J33Nzg5pnQJXTWi2tyYXa8pVui0uJ7Ffkzk= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=svanheule.net; spf=pass smtp.mailfrom=svanheule.net; dkim=pass (2048-bit key) header.d=svanheule.net header.i=@svanheule.net header.b=vlEeOcAy; arc=none smtp.client-ip=84.16.241.116 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=svanheule.net Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=svanheule.net Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=svanheule.net header.i=@svanheule.net header.b="vlEeOcAy" Received: from [IPv6:2a02:1812:162c:8f00:1e2d:b404:3319:eba8] (2a02-1812-162c-8f00-1e2d-b404-3319-eba8.ip6.access.telenet.be [IPv6:2a02:1812:162c:8f00:1e2d:b404:3319:eba8]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) (Authenticated sender: sander@svanheule.net) by polaris.svanheule.net (Postfix) with ESMTPSA id 851FD69E555; Thu, 13 Nov 2025 22:25:49 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=svanheule.net; s=mail1707; t=1763069149; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=51L4HkBNkO3CTDdkbZuobL6jJipCP6lYYxkEhb+jVHM=; b=vlEeOcAyqO2WzKutM/FSRYPs8W3X4fMlYSSP3FWn6GO81rPCDdnZ9kFtzmAGX1BeR4/6Cz q74xxgcjcyQMgWtNTckYEyjV+hnvubxSJFjpySBdnv3mfUqwEBUwNxLXKyJU7RXqZ6wktp dSPouneWmJclkhhimjL5Y+BM68rrYd5BmU5DmztmkmjVCEYHStG1vDK9vw8AZgLEBxtl6m yoZh2y4Qr0UisFSaQHaHJC1mC00bOxLD1/JMJi0ILyhDgG3w1mzh3QwR+V/eKPwSpBSML8 nzqHo3TZx3fgfRfc8GDyojnJ4hHfIXNFVFR2nclgCCYJIBWnL3rtz6G88bk8HA== Message-ID: Subject: Re: [PATCH v6 5/8] mfd: Add RTL8231 core device From: Sander Vanheule To: Lee Jones Cc: Michael Walle , Linus Walleij , Bartosz Golaszewski , linux-gpio@vger.kernel.org, Pavel Machek , Rob Herring , Krzysztof Kozlowski , Conor Dooley , linux-leds@vger.kernel.org, devicetree@vger.kernel.org, linux-kernel@vger.kernel.org Date: Thu, 13 Nov 2025 22:25:48 +0100 In-Reply-To: <20251106163316.GV8064@google.com> References: <20251021142407.307753-1-sander@svanheule.net> <20251021142407.307753-6-sander@svanheule.net> <20251106163316.GV8064@google.com> Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable User-Agent: Evolution 3.56.2 (3.56.2-2.fc42) Precedence: bulk X-Mailing-List: devicetree@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Hi Lee, On Thu, 2025-11-06 at 16:33 +0000, Lee Jones wrote: > On Tue, 21 Oct 2025, Sander Vanheule wrote: > > @@ -0,0 +1,193 @@ > > +// SPDX-License-Identifier: GPL-2.0-only > > + > > +#include > > +#include > > +#include > > +#include > > +#include > > +#include > > +#include > > +#include > > +#include > > +#include >=20 > Alphabetical please. Ack > > +static const struct reg_field RTL8231_FIELD_LED_START =3D > > REG_FIELD(RTL8231_REG_FUNC0, 1, 1); >=20 > Why does this have to be global? >=20 > Variables should be lowercase. Moving it to the one place where it's used. > > +static const struct mfd_cell rtl8231_cells[] =3D { > > + { > > + .name =3D "rtl8231-pinctrl", > > + }, > > + { > > + .name =3D "rtl8231-leds", > > + .of_compatible =3D "realtek,rtl8231-leds", > > + }, > > +}; >=20 > This is a pretty tenuous MFD! I suppose two functions is the minimum to count as multi-functional :-) I need a place to perform chip detection and to start the output drivers. T= he latter could be part of a pinctrl driver, but making the led driver depend = on the pinctrl driver doesn't sit too well with me. Now the functionality is cleanly split across the MFD core driver and pinctrl/led drivers. > > +static int rtl8231_soft_reset(struct regmap *map) >=20 > s/map/regmap/ Ack, replaced throughout the driver. > > +{ > > + const unsigned int all_pins_mask =3D GENMASK(RTL8231_BITS_VAL - 1, > > 0); > > + unsigned int val; > > + int err; > > + > > + /* SOFT_RESET bit self-clears when done */ > > + regmap_write_bits(map, RTL8231_REG_PIN_HI_CFG, > > + RTL8231_PIN_HI_CFG_SOFT_RESET, > > RTL8231_PIN_HI_CFG_SOFT_RESET); > > + err =3D regmap_read_poll_timeout(map, RTL8231_REG_PIN_HI_CFG, val, > > + !(val & RTL8231_PIN_HI_CFG_SOFT_RESET), 50, 1000); > > + if (err) > > + return err; >=20 > Do we have to scrunch these calls together? Made it a bit less claustrophobic. > > + regcache_mark_dirty(map); This marks the device register state as invalid, which isn't what I wanted = here. After a reset, the cache should be repopulated, so this is now: regcache_drop_region(0, RTL8231_REG_COUNT - 1); > > +static int rtl8231_init(struct device *dev, struct regmap *map) > > +{ > > + struct regmap_field *led_start; > > + unsigned int started; > > + unsigned int val; >=20 > If this is used for only one thing, it makes sense to use better > nomenclature that refers to that thing. Ack, updated the naming a bit. > > + led_start =3D dev_get_drvdata(dev); > > + err =3D regmap_field_read(led_start, &started); > > + if (err) > > + return err; > > + > > + if (!started) { >=20 > Reverse the logic and exit early if 'started'. Ack >=20 > > + err =3D rtl8231_soft_reset(map); > > + if (err) > > + return err; >=20 > '\n' here. Ack > > + /* LED_START enables power to output pins, and starts the > > LED engine */ > > + err =3D regmap_field_force_write(led_start, 1); This is in a volatile register, so the driver doesn't need to force anythin= g to write to the device and will now use regmap_field_write(). > > + led_start =3D devm_regmap_field_alloc(dev, map, > > RTL8231_FIELD_LED_START); > > + if (IS_ERR(led_start)) > > + return PTR_ERR(led_start); >=20 > Why do we need to do LED specific actions in the core driver? The is naming taken from the datasheet. Setting LED_START actually enables = both the LED scanning output engine and the output drivers. So this is needed fo= r proper functioning of the GPIO functionality too. > > + return devm_mfd_add_devices(dev, PLATFORM_DEVID_AUTO, > > rtl8231_cells, > > + ARRAY_SIZE(rtl8231_cells), NULL, 0, NULL); >=20 > Odd tabbing.=C2=A0 Please line-up with the '(' like usual. Ack > > +__maybe_unused static int rtl8231_suspend(struct device *dev) >=20 > The __maybe_unused comes after the "static int" part. Ack. C11 attributes like [[maybe_unused]] must come before the "static int" part= , but that doesn't line up with how most of the kernel uses the attributes. Best, Sander