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 0B5C24A06; Sun, 16 Mar 2025 10:01:35 +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=1742119296; cv=none; b=HnDNL7VqH9InSK8v/BmSeUd/V9D1HWnUdnBAwHyoUG6vFHNbJ5NYriP90wRAg7UmD44OgnNafhMFBKzl3lZvA0nLhDd2iQCg8OIlg22lZsCqVBsV2Z1VckpUCtYZ19xssukZcVugng7+3VfzfaLepXt76arkrafi8Miti/QsytA= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1742119296; c=relaxed/simple; bh=IuRs/vl289XZdaaDijhen/6iBsPovhGvf3hhFnmQhPY=; h=Date:From:To:Cc:Subject:Message-ID:In-Reply-To:References: MIME-Version:Content-Type; b=WLbjZY51J5QtIVdDoLTBpsTFDr+ftN+kFZ/LORjWqvd4C6+kVYesv3WlkKXvLG6UoWsY2aku0osnYN6r2biGY9YstLEaTzgpiaVioes4GIQG/u3JYoO8iqgZP+UFdc3AyioEnvmX1lQUzOtIumoOC/LTejUO4fPe9pjot2fFHR4= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org header.b=No7dn1DT; 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="No7dn1DT" Received: by smtp.kernel.org (Postfix) with ESMTPSA id 732DBC4CEDD; Sun, 16 Mar 2025 10:01:30 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1742119295; bh=IuRs/vl289XZdaaDijhen/6iBsPovhGvf3hhFnmQhPY=; h=Date:From:To:Cc:Subject:In-Reply-To:References:From; b=No7dn1DTMh4dFfzFOGf69q0MGaBWKfFRQjZ5Wo5gMgQgVQetU7TVylg8qrmnwa7JE oHavRybBvrFRzd2JlzRgtUHHW7EiBl0qgzBvhUCxhanfNERXz42sYuTkzPEjFl0FIQ MCwmf+90Otrc9kQ/E44OgTtgyW6GGPIZB3mN4JMa0NeeLUI5/mQOhcg0/hPztPKnxf O85SOz8VzFxRELIhQfkHZlvdiOW4INpQGxEiupOc8SeAAnxEOPj4QQpnRFxHGCYNhH QFTWAzPXfSL5hzdFNVPhxVWvtyH7SuB/D4S8Qk1BUniufAt45cy17coqYgB54FVRzU lLp4AF405zk6A== Date: Sun, 16 Mar 2025 10:01:23 +0000 From: Jonathan Cameron To: Andy Shevchenko Cc: Matti Vaittinen , Matti Vaittinen , Lars-Peter Clausen , Nuno Sa , David Lechner , Javier Carrasco , Olivier Moysan , Guillaume Stols , Dumitru Ceclan , Trevor Gamblin , Matteo Martelli , Alisa-Dariana Roman , =?UTF-8?B?Sm/Do28=?= Paulo =?UTF-8?B?R29uw6dhbHZlcw==?= , AngeloGioacchino Del Regno , linux-kernel@vger.kernel.org, linux-iio@vger.kernel.org Subject: Re: [PATCH v7 06/10] iio: adc: Support ROHM BD79124 ADC Message-ID: <20250316100123.4fa32464@jic23-huawei> In-Reply-To: <20250316095233.20d1a134@jic23-huawei> References: <20250316095233.20d1a134@jic23-huawei> X-Mailer: Claws Mail 4.3.0 (GTK 3.24.48; x86_64-pc-linux-gnu) Precedence: bulk X-Mailing-List: linux-iio@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit On Sun, 16 Mar 2025 09:52:33 +0000 Jonathan Cameron wrote: > On Fri, 14 Mar 2025 16:33:13 +0200 > Andy Shevchenko wrote: > > > On Fri, Mar 14, 2025 at 09:31:58AM +0200, Matti Vaittinen wrote: > > > On 13/03/2025 15:19, Andy Shevchenko wrote: > > > > On Thu, Mar 13, 2025 at 09:19:03AM +0200, Matti Vaittinen wrote: > > > > ... > Picking out a few things to comment on... > > > > > > > +#define BD79124_MASK_HYSTERESIS GENMASK(3, 0) > > > > > +#define BD79124_LOW_LIMIT_MIN 0 > > > > > +#define BD79124_HIGH_LIMIT_MAX GENMASK(11, 0) > > > > > > > > These masks are not named after registers (or I don't see it clearly), > > > > > > Naming is hard. I usually try to make a balance between: > > > > > > 1) using names which explain the purpose when seen in the code (at call > > > site) > > > 2) keeping names short enough > > > 3) following the naming convention in the data sheet > > > 4) maintain relation to the register. > > > > > > I put most emphasis to the 1, while 2 is important to me as well. 3 is > > > _very_ nice to have, but it is often somehow contradicting with 1 and 2. 4 > > > is IMO just nice to have. The register is usually clearly visible at call > > > site, and if someone adds new functionality (or checks the correctness of > > > the masks/registers), then 3 is way more important. > > > > > > I am open to any concrete suggestions though. > > > > From my point of view the starting point is 3, then one may apply 2 and 4, > > the 1 may mangle the name so much that register data field name becomes quite > > abstract. > > > > > > it's > > > > hard to understand which one relates to which register. Also, why not using > > > > bitfield.h? > > > > > > I saw no need for it? > > > > Hmm... Okay, I think Jonathan will ask that if really needed. > > > > I won't as I'm not a huge fan of bitfield.h. In many cases they bloat the code Oops. You weren't talking about the regmap bitfields. Ignore this. This driver is using FIELD_PREP / FIELD_GET. Maybe should be more extensive? > and increase the writes that go over the bus. Don't get me wrong, there > are good usecases, but it's not a universal thing (unlike PREP_FIELD() > etc which I love :) > > I do favour burning a few characters to make field / register relationship > clear though. A few things can help I think. > > Structuring defines and naming: > I like using whitespace in subtle ways for this. > > #define PREFIX_REGNAME_REG 0x00 > #define PREFIX_REGNAME_FIELDNAME_MSK GENMASK(X, Y) > #define PREFIX_REGNAME_FIELDNAME_FILEVALNAME 0x3 > etc > > Makes it easy to see if we have a mismatch going on > > However, I don't insist on this in all cases as it is one of those > "don't let perfect be the enemy of good" cases I think. > > So Matti, good to have one last look at the defines and see if they > can be wrangled into a slightly better form. > > > . > > ... > > > > > > > +static void bd79124gpo_set_multiple(struct gpio_chip *gc, unsigned long *mask, > > > > > + unsigned long *bits) > > > > > > > > Ditto, .set_multiple_rv(). > > > > > > As you know, this started at v6.14 cycle which is still ongoing. I don't > > > think .set_rv() and .set_multiple_rv() are available? > > > > You mean that you are still hope to have this series to be squeezed for > > v6.15-rc1? That's not me who decides, but if not, those APIs will be part of > > the v6.15-rc1 (at least they are pending in GPIO subsystem). > > > I'd vote for a rebase on rc1 that should be really easy to for me to pick > up. I'd accept a follow up series though. Ultimately won't affect > when this series lands as very unlikely Linus will delay the release > long enough for me to do another pull request this cycle, > > Jonathan