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 AD49739ACC; Sun, 16 Mar 2025 09:52:44 +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=1742118764; cv=none; b=f6+I8ZoaTyzS9SgnVf8kYGdd4vkrcrX2l9IlOxT6Br+r2fbE1Hvtp9dJEGYJfUstTG1w8Q+mkK7odrVPtvhWq18GbhcHWH6Olrq6KTS63M3ZFu1GcbAo7ZXfr+FRcMiaX+wyOi/FHIOSA6I+3jkqkKOJE4kdBtXt2Yx0REOkSgw= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1742118764; c=relaxed/simple; bh=9Aj1RfBAi8t/f93C+aSgagstTgV2xhkQ6oHtR1PzL/E=; h=Date:From:To:Cc:Subject:Message-ID:In-Reply-To:References: MIME-Version:Content-Type; b=Nfn6PJ3kidSyCe3kUZR1L0gIX2P70zYHO7DG2nt9xjD1mKbuqhv0VUrdQIo6rmyCRebatEnXSbXyzv+cmRux+AQeH2tBruIy1T7+/FHdhzqiaeVXp2Sgn3603wh0faE1+MH7lpUPlNTdav958I8pg2CGQB10aqqpk0EUJ3t+QI8= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org header.b=IjfobbrH; 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="IjfobbrH" Received: by smtp.kernel.org (Postfix) with ESMTPSA id 3C271C4CEDD; Sun, 16 Mar 2025 09:52:37 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1742118764; bh=9Aj1RfBAi8t/f93C+aSgagstTgV2xhkQ6oHtR1PzL/E=; h=Date:From:To:Cc:Subject:In-Reply-To:References:From; b=IjfobbrHzETYxUSD5FABCQbDjNu45n2vi4KD2fceWoE5L7kzOpIn0db0ZPimbcpJ4 Og8jBuzYNEk/XBaMtLRrCwske7zsa18wAhjdbEeFGojJOK7+gXrf8y/tPkWDr5Elpu fVNbA499qzfzM2LDbTPq89n+MvRdg8pGutWB2/hdhZPwi1gkbYJqizlrjHOccuObAZ 5is4Ku9RohcjgL6+iWm0HR0WaXA+VV89fkVTHpG4Ynu2GpMqnRaCE20BHhxkn1X5/Z gwgxR+a1Z1EnpSv5GIXduBAb0cfbkx93p144abZtIemCaSq/WSPgtuYzpkku0AqUR6 nB1vCJYJJRy3g== Date: Sun, 16 Mar 2025 09:52:33 +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: <20250316095233.20d1a134@jic23-huawei> In-Reply-To: References: 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 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 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