From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from smtpout-02.galae.net (smtpout-02.galae.net [185.246.84.56]) (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 464BA32C951 for ; Fri, 17 Oct 2025 15:01:07 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=185.246.84.56 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1760713270; cv=none; b=M6benD3sRcXqOvIhD0wv52b2y6sC/VG/odnJV8szgg+QOsncu84Rc5GwRxdxVo9VeX21vJXlzJN9XEjsAVzKcaniRdUMOo5GI+royfrMY++fd9afQpoKpCgrlGc/EyKKILT9PCdBT9PZHJKdwcWR4hSeZI1UOref2aH9ImH75bY= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1760713270; c=relaxed/simple; bh=fu1rgFg6zFERmWrukTHtScTFU9VI+YSEm3Jqr3a3rqk=; h=Date:From:To:Cc:Subject:Message-ID:In-Reply-To:References: MIME-Version:Content-Type; b=aDFE3l6qX1bHGpyoWEq7OSNkbSG8cMerqwzIiW5sEmqRguFOe+/5JdivdRoycpsHvpcpCsqG0eBtS+MQQiLNjPcuVCI4YCAgR4dmu8BKPYi6BY5iUjCWQvUxBNaaje5+AEjli2C5/B7WNjBgpPvLkoWAqvXGoMRFBJU+cfNiGec= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=pass (p=reject dis=none) header.from=bootlin.com; spf=pass smtp.mailfrom=bootlin.com; dkim=pass (2048-bit key) header.d=bootlin.com header.i=@bootlin.com header.b=VWZ2lb80; arc=none smtp.client-ip=185.246.84.56 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=reject dis=none) header.from=bootlin.com Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=bootlin.com Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=bootlin.com header.i=@bootlin.com header.b="VWZ2lb80" Received: from smtpout-01.galae.net (smtpout-01.galae.net [212.83.139.233]) by smtpout-02.galae.net (Postfix) with ESMTPS id 5E4B81A1483; Fri, 17 Oct 2025 15:01:06 +0000 (UTC) Received: from mail.galae.net (mail.galae.net [212.83.136.155]) by smtpout-01.galae.net (Postfix) with ESMTPS id 28795606DB; Fri, 17 Oct 2025 15:01:06 +0000 (UTC) Received: from [127.0.0.1] (localhost [127.0.0.1]) by localhost (Mailerdaemon) with ESMTPSA id 53F01102F2363; Fri, 17 Oct 2025 17:00:56 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=bootlin.com; s=dkim; t=1760713265; h=from:subject:date:message-id:to:cc:mime-version:content-type: content-transfer-encoding:in-reply-to:references; bh=b9g9x9l+TGh+Jz6FtSUgUJKPZ2bmCq5GDO9amdTb+jU=; b=VWZ2lb805RvAcMpu2w7GsmG4qPzHEvrmTXEmSf8bLkdyqv8tELrbP1N9GAyxwhuevB6DOy ZqLha3EVHs7C3aY05GsxzDYApneZKZroDf1OnCtTXktlSpyHTuHIzyiT2dBcYisbYJ/f7T JjroPIiENIUsrrrjgkPAU1zfgNM5V990gaiQRXv265R9Ht/4SIcK6lfOUNPhmIEqJHYDG0 mVJLKaoYho9vY3N3HdSWjsf+FUAkElIPSoSpFZ7JcYofHz0UTYpB6tzfJP75sHyGiOhmcq UMkhtwb88dZwI94C0APecwjz124C0yM9cmIq7lZILPj1skAr+Jf7/L5H4YtUBA== Date: Fri, 17 Oct 2025 17:00:54 +0200 From: Herve Codina To: Wolfram Sang Cc: Jonathan Cameron , David Lechner , Nuno =?UTF-8?B?U8Oh?= , Andy Shevchenko , Rob Herring , Krzysztof Kozlowski , Conor Dooley , Geert Uytterhoeven , Magnus Damm , Liam Girdwood , Mark Brown , linux-iio@vger.kernel.org, linux-renesas-soc@vger.kernel.org, devicetree@vger.kernel.org, linux-kernel@vger.kernel.org, Pascal Eberhard , Miquel Raynal , Thomas Petazzoni Subject: Re: [PATCH 2/4] iio: adc: Add support for the Renesas RZ/N1 ADC Message-ID: <20251017170054.7a7a6d5f@bootlin.com> In-Reply-To: References: <20251015142816.1274605-1-herve.codina@bootlin.com> <20251015142816.1274605-3-herve.codina@bootlin.com> Organization: Bootlin X-Mailer: Claws Mail 4.3.1 (GTK 3.24.43; x86_64-redhat-linux-gnu) Precedence: bulk X-Mailing-List: devicetree@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit X-Last-TLS-Session-Version: TLSv1.3 Hi Wolfram, On Fri, 17 Oct 2025 11:11:49 +0200 Wolfram Sang wrote: > > +static int rzn1_adc_read_raw_ch(struct rzn1_adc *rzn1_adc, unsigned int chan, int *val) > > +{ > > + u32 *adc1_data, *adc2_data; > > + int adc1_ch, adc2_ch; > > + u32 adc_data; > > + int ret; > > + > > + if (chan < 8) { > > + /* chan 0..7 used to get ADC1 ch 0..7 */ > > + adc1_ch = chan; > > + adc1_data = &adc_data; > > + adc2_ch = -1; > > + adc2_data = NULL; > > + } else if (chan < 16) { > > + /* chan 8..15 used to get ADC2 ch 0..7 */ > > + adc1_ch = -1; > > + adc1_data = NULL; > > + adc2_ch = chan - 8; > > + adc2_data = &adc_data; > > + } else { > > + return -EINVAL; > > + } > > How about putting part of the logic into the setup function? So, here > only: > > if (chan >= 16) > return -EINVAL > > > + > > + ret = pm_runtime_resume_and_get(rzn1_adc->dev); > > + if (ret < 0) > > + return ret; > > + > > + mutex_lock(&rzn1_adc->lock); > > + > > + rzn1_adc_vc_setup_conversion(rzn1_adc, chan, adc1_ch, adc2_ch); > > rzn1_adc_vc_setup_conversion(rzn1_adc, chan); > > And in that function: > > > +static void rzn1_adc_vc_setup_conversion(struct rzn1_adc *rzn1_adc, u32 ch, > > + int adc1_ch, int adc2_ch) > > +{ > > + u32 vc = 0; > > + > > + if (adc1_ch != -1) > > + vc |= RZN1_ADC_VC_ADC1_ENABLE | RZN1_ADC_VC_ADC1_CHANNEL_SEL(adc1_ch); > > + > > + if (adc2_ch != -1) > > + vc |= RZN1_ADC_VC_ADC2_ENABLE | RZN1_ADC_VC_ADC2_CHANNEL_SEL(adc2_ch); > > + > > + writel(vc, rzn1_adc->regs + RZN1_ADC_VC_REG(ch)); > > +} > > if (ch < 8) > vc |= RZN1_ADC_VC_ADC1_ENABLE | RZN1_ADC_VC_ADC1_CHANNEL_SEL(ch); > else > vc |= RZN1_ADC_VC_ADC2_ENABLE | RZN1_ADC_VC_ADC2_CHANNEL_SEL(ch - 8); > > And a similar simplification for rzn1_adc_vc_wait_conversion(). > > Should work and the code is even more readable, I'd say. And has less > lines. > That was what I did on my first driver draft before sending this first iteration. I moved to adc1 and adc2 channel numbers in parameters to avoid adding this logic here. I think it was better to decouple the IIO chan number from the adc core channel used. I don't know if it will be relevant but we can image a future improvement where new IIO chans use both the ADC core 1 and 2. It could make sense. IMHO, I think the solution you proposed is similar in term of complexity to the RZN1_ADC_NO_CHANNEL approach. On my side, I would prefer the RZN1_ADC_NO_CHANNEL approach to keep the decoupling between IIO chan and ADC core chans. That's said, I am still open to move in your direction if you still think it is more relevant than the RZN1_ADC_NO_CHANNEL approach. Just tell me. Best regards, Hervé