From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from omta-ad1-fd1-2101-ap-sydney-1.aib29aea255.vm.email.ap-sydney-1.oci.oraclecloud.com (aib29aea255.vm.email.ap-sydney-1.oci.oraclecloud.com [192.29.151.255]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 0353237106D for ; Fri, 21 Aug 2026 03:27:35 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=192.29.151.255 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1787282856; cv=none; b=KaUKznqJbZRVw9Ghbbuv9Cib/mZliR7yk09KFOXZ4MxWn34wpgYPbm/k8XB1Pe2W1WJYtwkgFsvlPyPqZUWRImuOe/If/YYUEXEQJYDaSYUS1Ek1ztkYTHiOehAZAO3OJKImZ4aUZf+DwxQyqriqDnDOxWzrUksEIFisgZ5w2K8= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1787282856; c=relaxed/simple; bh=h7Ve+n7gLalVN/hamq+9sOt/wIp9NFoxX09l0/unmsU=; h=Message-id:Date:MIME-version:Subject:To:Cc:References:From: In-reply-to:Content-type; b=No4ixwWw2QNJmE0fnyQFy/QTtYrsel/YBmqCIj0nY7yDMcDAuzEWRnMg8j5XJKXGPz8cZTZg2/PrTkB+0FBEkosz3j9sOpmppO0VSaQMwdq71je5XOGjPm3tIYbgMx9iUHA2aDMi1XoZORm68WHFgZMbqjIzY5k6p0QT21SYcWI= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=stevetech.au; spf=pass smtp.mailfrom=syd1.rp.oracleemaildelivery.com; dkim=pass (2048-bit key) header.d=stevetech.au header.i=@stevetech.au header.b=aGrYrVCf; dkim=pass (2048-bit key) header.d=syd1.rp.oracleemaildelivery.com header.i=@syd1.rp.oracleemaildelivery.com header.b=K1WqK9hU; arc=none smtp.client-ip=192.29.151.255 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=stevetech.au Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=syd1.rp.oracleemaildelivery.com Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=stevetech.au header.i=@stevetech.au header.b="aGrYrVCf"; dkim=pass (2048-bit key) header.d=syd1.rp.oracleemaildelivery.com header.i=@syd1.rp.oracleemaildelivery.com header.b="K1WqK9hU" DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; s=oracle-2026; d=stevetech.au; h=Date:To:From:Subject:Message-Id:MIME-Version:Sender:List-Unsubscribe:List-Unsubscribe-Post; bh=L66ir+PivH58IHajuTfLolp/B5mG7kVirbvlAHl33II=; b=aGrYrVCfA60htjy3Tt2GmtAMKk6fiBYskhg6gKQp5LYzGr34z2ORhb1EVF4YPVAS/I4r1eVYwpEy npCV4zwFfPmF+vRN+wR0KNCkdgDZ2hC1I2jH4vUwbujK1VeAjjZfpRhcVNw36+cCv58y1mHkvVNa OevyuFUeawmjU5lgIJObJXDJaaoQaGTHVf0/NPja8xFrNR1LeUs7I3VoKNz0YccDbmCxNqwqImgc VzvWSn3Bsk3URMEGMd+2cX78J08ufu8GDg4uFsBiLwQHoBbhsDkaZ0c6pkhL+/BUee+weF2plBu6 jR5jSIN7KZ3gpDqRWBWBcSf7uAZbcN37V5MOcw== DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; s=prod-syd-20200122; d=syd1.rp.oracleemaildelivery.com; h=Date:To:From:Subject:Message-Id:MIME-Version:Sender:List-Unsubscribe:List-Unsubscribe-Post; bh=L66ir+PivH58IHajuTfLolp/B5mG7kVirbvlAHl33II=; b=K1WqK9hULRnXDllcgCBg2bBo8YyALvW5NVg61IdaeqcfxLc2MR4UecAIqh5ca90JUgV3ZKVLsPRA ZHNxLAEP/pjCQ050dTfGZgYKI4C6ptl9ACCiJFMPoqThChZTOtnYTtD3GeS4qLfU3hykuAtyx6cD CtJxyVC8QoDeonMDSjV1skAjqGoHzh/639GswOqFePi8eIxj86CuWED1q0cD/7dagKbyAGKDpfjC pi2vxYWbs6H6BRtI14010aZmDxVuf/ai+UqPV6G8yej87Lksn46KWoAT4sErt7pyAYwPBszMNEuf 9gjOJEChQCDv0qCZW8mmPwe2D4rpsNlnS557Pg== Received: by omta-ad1-fd1-2101-ap-sydney-1.mnat.vcndpsyd.oraclevcn.com (Oracle Communications Messaging Server 8.1.0.1.20260722 64bit (built Jul 22 2026)) with ESMTPS id <0TK31OT2MO9S9F70@omta-ad1-fd1-2101-ap-sydney-1.mnat.vcndpsyd.oraclevcn.com> for linux-kernel@vger.kernel.org; Fri, 21 Aug 2026 03:27:29 +0000 (GMT) List-Unsubscribe-Post: List-Unsubscribe=One-Click Message-id: <7fac8b19-af18-4365-94d6-a6bd18076abb@stevetech.au> Date: Fri, 21 Aug 2026 13:27:28 +1000 Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH RFC] hwmon: Add Support for PMIC5000 Power Management ICs To: Guenter Roeck Cc: Andi Shyti , linux-hwmon@vger.kernel.org, linux-i2c@vger.kernel.org, linux-kernel@vger.kernel.org, Stephen Horvath References: <20260819-pmic5000-v1-1-e5e0ddcf0863@stevetech.au> <6b8bf838-011f-46b9-a2a8-379de9cceddf@roeck-us.net> From: Stephen Horvath Content-language: en-AU, en-US, en-GB In-reply-to: <6b8bf838-011f-46b9-a2a8-379de9cceddf@roeck-us.net> Content-type: text/plain; charset=UTF-8 Content-transfer-encoding: 7bit Reporting-Meta: AAE0CRPkDXUCCkglQobUipg4J5mxfCGMKIbvqKb9veEutptuF/BYXQOZL+n+BC+g 9F/l1G9fNqweZphNlPpaqUMlvd2LjJ6v1MyLT9Z94ZdJi0NTQBNPll9kPD2Yp3wc rw5N6TQpgt5ZIA7LkK6usy1o6vNW7Ti/8PebL/RSWbKg0SQ6yRwW38pONssMnqcM Fw3W2XcOr8kiPW8BRvbSm3fiNZhimCDBLPt31iEg6r5uD9KvoTcAKraU/xMv/G+a uq9jY2WjvfQErY900gDZXZDNV8KQ4IwazTzue6rShazDKrJHxpAFna+Th6HD8JTQ 979g9d7nXEPTizVvQ77XEV+QAriA6lP4blowVAbjt0f1GAOKA6byV/P8XqFXdFQY C9Il1EwZNq62tOw0RVBhE2nEOxgz70aa2hp7HznFHPj5Thmbt0R7j6brCXJm Hi Guenter, Thanks for your review! On 21/8/26 01:34, Guenter Roeck wrote: >> - Does this belong in HWMON, or should it be part of the regulator >> subsystem? > > It depends on its functionality. Is this just for monitoring, or are the voltages > controllable ? This driver is just for monitoring. They could be controllable, but I haven't implemented that as changing the voltage of active RAM modules doesn't seem safe (without retraining and so on). >> +in0_enable Whether SWA is enabled (RO) > > Does that enable the voltage or its monitoriong ? The enable attribute is > only to enable monitoring, not to enable the voltage. If this is a > controllable voltage, the driver should be a regulator driver, as Sashiko > suggested. It just queries whether that output is enabled. I'll remove it though as the disabled channels are hidden by is_visible anyway. >> +/* PMIC5000 registers. */ >> +// clang-format off > > No way. Are you serious ? clang-format would break the alignment of the #defines; and there's a few other drivers with "// clang-format" in them, so I assumed it would be okay (though none are hwmon). I'll remove it next revision. >> + >> + > > Please run checkpatch --strict on your patches and fix what it reports. Will do! >> +struct pmic5000_data { >> + struct regmap *regmap; >> + struct mutex mode_lock; >> + struct mutex adc_lock; > > Explain why those are needed on top of the hwmon subsystem lock. I didn't know the subsystem lock existed, I'll remove them. >> + if (channel != 0) >> + return -EOPNOTSUPP; > > Unnecessary check. Sure. > > Unnecessary channel checks (at least if the is_visible function does its job > and the info data is correct). > Okay. >> + case 6: { > > Why "{" ? No reason, I'll remove it. I had previously declared something there. >> + if (channel >= 0 && channel <= 3) { > > channel is always >= 0. I wasn't too sure, thanks for that! >> + /* >> + * The host shall wait minimum of 9 ms delay after the input selection >> + * for ADC readout and the actual readout >> + * >> + * msleep may sleep for up to 20ms, which is fine. > > No, it isn't fine. Fair enough, I'll change it to fsleep. > Also, why wait if the channel was not changed ? Good point. >> + *val = 1 << (regval & 0x03); > > BIT() Thanks! >> +static int pmic5000_write_interval(struct pmic5000_data *data, long val) >> +{ >> + struct regmap *regmap = data->regmap; >> + u32 regval; >> + int err; >> + >> + switch (val) { >> + case 1: >> + regval = 0; >> + break; >> + case 2: >> + regval = 1; >> + break; >> + case 4: >> + regval = 2; >> + break; >> + case 8: >> + regval = 3; >> + break; >> + default: >> + return -EINVAL; >> + } > > find_closest() would be more appropriate here. We don't usually expect users to know valid > update intervals. Thanks! >> +static int pmic5000_common_probe(struct device *dev, struct regmap *regmap) > > Why pmic5000_common_probe() ? There is only one caller. Good point, I'll move it. >> +static const struct of_device_id pmic5000_of_ids[] = { >> + { >> + .compatible = "jedec,pmic5000", > > jedec,pmic5000 is not documented. Is there some centralised database for device tree identifiers, or does it just need to be added to trivial-devices.yaml or something? Thanks again for your review! Steve