From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-qk1-f172.google.com (mail-qk1-f172.google.com [209.85.222.172]) (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 CFF291B0F11 for ; Thu, 2 Jan 2025 18:19:21 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=209.85.222.172 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1735841966; cv=none; b=K783PbeqlPKNDL2j6b9DvHJ4yRrGtxT80mjib9GUfn0BXc+ElTp7C+LMdJnIQ/x2NTjSawHeLHqvO5Rn+cI9BtX4ZYSK9vjX1NyB7ccde29Gy23FySEZkfjr4twR3iA2f6HyR2tcD222AFWAMNkAQhrGnMc80N8rrtbo07/bmPQ= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1735841966; c=relaxed/simple; bh=5dvgBABeyGFxnBG3Y5nT2U5CVkpQfQV1dff5/rH3IEA=; h=Message-ID:Date:MIME-Version:Subject:To:Cc:References:From: In-Reply-To:Content-Type; b=dKgNJLhQqJTXKoifcsgK47VoqvGcbgYPU7oZPLNIY1TzSgbLZ/4dvNjGT6wy+5lysDdr8Y5hJK/Q1EnSmRLmGT7TCRYo4clHXmtTveWs8GGB/S4pos48XpM1wPXlT79kXeENnHKpKDrcxzG+z1+JwL3+7ifxGRdIV3XKA1n5pWo= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=none (p=none dis=none) header.from=baylibre.com; spf=pass smtp.mailfrom=baylibre.com; dkim=pass (2048-bit key) header.d=baylibre-com.20230601.gappssmtp.com header.i=@baylibre-com.20230601.gappssmtp.com header.b=m8kmSMil; arc=none smtp.client-ip=209.85.222.172 Authentication-Results: smtp.subspace.kernel.org; dmarc=none (p=none dis=none) header.from=baylibre.com Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=baylibre.com Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=baylibre-com.20230601.gappssmtp.com header.i=@baylibre-com.20230601.gappssmtp.com header.b="m8kmSMil" Received: by mail-qk1-f172.google.com with SMTP id af79cd13be357-7b6fc5bf609so911359585a.1 for ; Thu, 02 Jan 2025 10:19:21 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=baylibre-com.20230601.gappssmtp.com; s=20230601; t=1735841961; x=1736446761; darn=vger.kernel.org; h=content-transfer-encoding:in-reply-to:from:content-language :references:cc:to:subject:user-agent:mime-version:date:message-id :from:to:cc:subject:date:message-id:reply-to; bh=bvvb86cIzS/dOwVH6EYJXcGMKz0+uVXUG4GedKpth0U=; b=m8kmSMilDE+WE7k6YQRtZ/O9w5G5W+vBpxFYZyixRdRGYV490HO59leIlTS+HqI26t YX68Cozl/swLfL4MJilyXyqpjuP9Q0QFw4jIYbPMKTI19Um01SSqURNCPJBZtDxi1TiL ySFDme2xTJgtb2hnPYTr5rEcnqddFmguMSD3XGBCDO2wGWvGw4jONYA7AYjqbm4kniNA JS6Bn4L8s3rU//RCzPmJMB26MQHEdS3W9K9ek46FI3gf6wp4m4urmGAPcp/HvKoV2+e0 TWYa+TW9WfYfOYsgLkPtJDB0LLR0zLGeQtDHtzVAKu8ILkrd0UdmSLLSwSk50wt4JT45 Y7lA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1735841961; x=1736446761; h=content-transfer-encoding:in-reply-to:from:content-language :references:cc:to:subject:user-agent:mime-version:date:message-id :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=bvvb86cIzS/dOwVH6EYJXcGMKz0+uVXUG4GedKpth0U=; b=vFPG6RLbvfa34t+o3Jm/xxjg0X/NR393xOlaFH0ocg0Z6kpCNMkRjn9K7EhK1A9Ntv jAe/p9pJ5z40kF2GVYixzl3+RkLUpi6QyDnSC8rMvNFA8qGZy/6DYn/EtHnKW+XKKtWE tMP+jxy2gXBiLgMCIrQh8UhwIROekQ+ZIumQ/X1j+QyCmx4+HgYP9yGKoTy1u3GvzTpI ft6ZFlPhq/qCv9ubKLO/iAp3r+UsRQ+9e6ESW+WUYID+Ns66SAgs9MgKBhdNBlhHxHE8 oYWiw2WsAKSETyDTURPNgFkHlDK+kS24uONoaFN6SnKjriaMeRx6JJaNHWM4yW+l3VEa Tw7A== X-Forwarded-Encrypted: i=1; AJvYcCUbfeXH3dKaiNeTJlxsr9E3tTw/CSiBb+DoS1YWhqHCQQo1X67PJPDqphMMktDjZoacKT1kJPWyEjm3Svw=@vger.kernel.org X-Gm-Message-State: AOJu0Yxx7blOk5eixbFCFudeHYhgcHJRtDHuzTUI/AbCNacvTe3VdDNn ZbYEDdwiNdE/yiIS+hVB+ou6pQGIdVc0EbgC0YHaA+cybVIY+YUgA2C8Me/yI+A= X-Gm-Gg: ASbGncuIVT082dUh6dS4ZsVHBFrOa9bVlNh4NlHqMg2auamK41soYjCowsJYSUFS2qm +s43KdSXMPvlYyZ2TgVj5U5HpX95jF3fUV2ml8Ux91qBXxzkqWYIUZpvcc9adV2vrtVFwWhUGDI T1Qs1SyhpZxh1t/lufOG7E8sJC/gf9umRR/3ZaFZM4UlTV5g49tfnaRiz4S1nbFS8G9BFh6/JXi jXaIaE4PBOH/znTuKuSIrchB9a0vhOce0esIZx4+4Y7hhghGQ73PGMhNp+KyLMRy+oE4xmnuahj aY4aC2b9BJqwvtKFWbbX9d/K X-Google-Smtp-Source: AGHT+IGeOONPgFA0dXRFKcX210Q5kkrtbZxdBQLMw2EGeHfAGwIzMg9YGL5jsDDC44doX2Wugk0ppg== X-Received: by 2002:a05:620a:2807:b0:7b6:edd0:d752 with SMTP id af79cd13be357-7b9ba7167c4mr8333933085a.5.1735841960779; Thu, 02 Jan 2025 10:19:20 -0800 (PST) Received: from [192.168.40.12] (d24-150-219-207.home.cgocable.net. [24.150.219.207]) by smtp.gmail.com with ESMTPSA id d75a77b69052e-46a3eb2b0a1sm138004651cf.74.2025.01.02.10.19.19 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Thu, 02 Jan 2025 10:19:20 -0800 (PST) Message-ID: <1c641b37-475a-4153-bcfc-e0e72d79fa76@baylibre.com> Date: Thu, 2 Jan 2025 13:19:19 -0500 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 1/2] iio: adc: ad4695: add offload-based oversampling support To: Jonathan Cameron Cc: Michael Hennerich , =?UTF-8?Q?Nuno_S=C3=A1?= , David Lechner , Lars-Peter Clausen , Jonathan Corbet , linux-iio@vger.kernel.org, linux-kernel@vger.kernel.org, linux-doc@vger.kernel.org References: <20241217-ad4695-oversampling-v1-0-0b045d835dac@baylibre.com> <20241217-ad4695-oversampling-v1-1-0b045d835dac@baylibre.com> <20241219161301.3f708302@jic23-huawei> Content-Language: en-US From: Trevor Gamblin In-Reply-To: <20241219161301.3f708302@jic23-huawei> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit On 2024-12-19 11:13, Jonathan Cameron wrote: > On Tue, 17 Dec 2024 16:47:28 -0500 > Trevor Gamblin wrote: > >> Add support for the ad4695's oversampling feature when SPI offload is >> available. This allows the ad4695 to set oversampling ratios on a >> per-channel basis, raising the effective-number-of-bits from 16 >> (OSR == 1) to 17 (4), 18 (16), or 19 (64) for a given sample (i.e. one >> full cycle through the auto-sequencer). The logic for reading and >> writing sampling frequency for a given channel is also adjusted based on >> the current oversampling ratio. >> >> The non-offload case isn't supported as there isn't a good way to >> trigger the CNV pin in this mode. Support could be added in the future >> if a use-case arises. >> >> Signed-off-by: Trevor Gamblin > Hi Trevor, > > The clamping fun of get_calibbias seems overkill. If this isn't going to ever > overflow an s64 maybe just use the high precision to do it the easy way. > I'm not sure you can't just fit it in an s32 for that matter. I've just > not done the maths to check. > > Jonathan > > >> +static unsigned int ad4695_get_calibbias(int val, int val2, int osr) >> +{ >> + unsigned int reg_val; >> + >> + switch (osr) { >> + case 4: >> + if (val2 >= 0 && val > S16_MAX / 2) >> + reg_val = S16_MAX; >> + else if ((val2 < 0 ? -val : val) < S16_MIN / 2) > It has been a while, but IIRC if val2 < 0 then val == 0 as otherwise > we carry the sign in the val part. Sometimes we generalize that to > make life easier for driver writers but I think you can use that here > to simplify things. > > (for background look at __iio_str_to_fixpoint() - it's a bit of a hack > to deal with integers have no negative 0) > > if (val > S16_MAX / 2) > ... > else if (val < S16_MIN / 2) > ... > else if (val2 < 0) etc > > You may feel it is better to keep the code considering the val2 < 0 when > val != 0 case and I don't mind that as it's not wrong, just overly complex! > > If you can easily clamp the overall range you can just do some maths > with enough precision to get one number (probably a s64) and clamp that. > Easy to sanity check for overflow based on val to ensure no overflows. Hi Jonathan, I'm reviewing this again but I'm not entirely clear what you mean. Are you suggesting that the entire switch block could be simplified (i.e. eliminating the previous simplification for the val2 < 0 case in the process), or that the calls to clamp_t can be combined? I've tested out simplifying the val2 < 0 case locally and driver functionality still seems OK. Maybe I'm missing a third option. - Trevor > > > > >> + reg_val = S16_MIN; >> + else if (val2 < 0) >> + reg_val = clamp_t(int, >> + -(val * 2 + -val2 * 2 / MICRO), >> + S16_MIN, S16_MAX); >> + else if (val < 0) >> + reg_val = clamp_t(int, >> + val * 2 - val2 * 2 / MICRO, >> + S16_MIN, S16_MAX); >> + else >> + reg_val = clamp_t(int, >> + val * 2 + val2 * 2 / MICRO, >> + S16_MIN, S16_MAX); >> + return reg_val; >> + case 16: >> + if (val2 >= 0 && val > S16_MAX) >> + reg_val = S16_MAX; >> + else if ((val2 < 0 ? -val : val) < S16_MIN) >> + reg_val = S16_MIN; >> + else if (val2 < 0) >> + reg_val = clamp_t(int, >> + -(val + -val2 / MICRO), >> + S16_MIN, S16_MAX); >> + else if (val < 0) >> + reg_val = clamp_t(int, >> + val - val2 / MICRO, >> + S16_MIN, S16_MAX); >> + else >> + reg_val = clamp_t(int, >> + val + val2 / MICRO, >> + S16_MIN, S16_MAX); >> + return reg_val; >> + case 64: >> + if (val2 >= 0 && val > S16_MAX * 2) >> + reg_val = S16_MAX; >> + else if ((val2 < 0 ? -val : val) < S16_MIN * 2) >> + reg_val = S16_MIN; >> + else if (val2 < 0) >> + reg_val = clamp_t(int, >> + -(val / 2 + -val2 / 2 / MICRO), >> + S16_MIN, S16_MAX); >> + else if (val < 0) >> + reg_val = clamp_t(int, >> + val / 2 - val2 / 2 / MICRO, >> + S16_MIN, S16_MAX); >> + else >> + reg_val = clamp_t(int, >> + val / 2 + val2 / 2 / MICRO, >> + S16_MIN, S16_MAX); >> + return reg_val; >> + default: >> + if (val2 >= 0 && val > S16_MAX / 4) >> + reg_val = S16_MAX; >> + else if ((val2 < 0 ? -val : val) < S16_MIN / 4) >> + reg_val = S16_MIN; >> + else if (val2 < 0) >> + reg_val = clamp_t(int, >> + -(val * 4 + -val2 * 4 / MICRO), >> + S16_MIN, S16_MAX); >> + else if (val < 0) >> + reg_val = clamp_t(int, >> + val * 4 - val2 * 4 / MICRO, >> + S16_MIN, S16_MAX); >> + else >> + reg_val = clamp_t(int, >> + val * 4 + val2 * 4 / MICRO, >> + S16_MIN, S16_MAX); >> + return reg_val; >> + } >> +} >> +