From mboxrd@z Thu Jan  1 00:00:00 1970
Received: from mail-ej1-f41.google.com (mail-ej1-f41.google.com [209.85.218.41])
	(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 8B79321ADA4
	for <linux-staging@lists.linux.dev>; Tue, 26 Aug 2025 22:05:16 +0000 (UTC)
Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=209.85.218.41
ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116;
	t=1756245919; cv=none; b=Fpro/ElZjTxbiPd+6O9/drt1mqPBulcFFxR2/+/hpmXGgw9MUSKWGUSJzBkTXmYGCnXQ19n4gISRhhzl0Pu2rAzW/e0RiDBnnNHllKrklCGMV6pfkbgAx4oBoViUImlfXM9b3d2imq2n5F2dF6mdFrhuUZCuDi7CaixBSXjXqSA=
ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org;
	s=arc-20240116; t=1756245919; c=relaxed/simple;
	bh=CcoUYKhr2YqXvegnfI/s8qSchCDEhcQEWbufXc6+c80=;
	h=From:To:Cc:Subject:Date:Message-ID:MIME-Version; b=i1ZVLT5bqRvq8J5eosgGu5BX1jGu8qRbZ8kj1EQEjZWBxgHORhm4dv0cR6z5HK/ch+OzzBtK5sI+ZyAFvUenVS8YCaKsBtLI1mgJvconxAOiJVo3uzOqvRaTd7n2vauvj1PK2Iz6yHBLEzEJDz++s16xczXgdP7gcMqHj5nvKb8=
ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=gmail.com; spf=pass smtp.mailfrom=gmail.com; dkim=pass (2048-bit key) header.d=gmail.com header.i=@gmail.com header.b=A5pYkAqt; arc=none smtp.client-ip=209.85.218.41
Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=gmail.com
Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=gmail.com
Authentication-Results: smtp.subspace.kernel.org;
	dkim=pass (2048-bit key) header.d=gmail.com header.i=@gmail.com header.b="A5pYkAqt"
Received: by mail-ej1-f41.google.com with SMTP id a640c23a62f3a-afcb7347e09so1060526266b.0
        for <linux-staging@lists.linux.dev>; Tue, 26 Aug 2025 15:05:16 -0700 (PDT)
DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed;
        d=gmail.com; s=20230601; t=1756245915; x=1756850715; darn=lists.linux.dev;
        h=content-transfer-encoding:mime-version:message-id:date:subject:cc
         :to:from:from:to:cc:subject:date:message-id:reply-to;
        bh=iY5jgqXj688PFbTycou4DPxWJ19/3+efTPtWXBoI4TE=;
        b=A5pYkAqtmlbOhuI7NdkEJiATHIVZbrZ6SFseMMl/DtYVPGaxC3ZDPEkwutdYgo5bAc
         slGjqdJuqhOg+UmStqLrQMHK/v26X0PBYxdS2uCpiqJKbXU3CCKfRTLOxHWzzO5oEQzs
         whlBSU82Sr2b3uHaeCQiWHQ2nnv57nqwHfg5sYZV7MF8mQP6DnePma46/PmSFiyblYpr
         tXEkZB2tb69uPY6IL+KJvBOrXXAYzi0+tKElITTM4ueoyVK88bEdpjJMZf7ivb7Qkipy
         rvQrq3Qc4eOj9sPh2hX/nwUouBXL8Wz46FXHoR1YYnwJiFjqyr2cvFfgLoXctgvFDR/l
         z1tg==
X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed;
        d=1e100.net; s=20230601; t=1756245915; x=1756850715;
        h=content-transfer-encoding:mime-version:message-id:date:subject:cc
         :to:from:x-gm-message-state:from:to:cc:subject:date:message-id
         :reply-to;
        bh=iY5jgqXj688PFbTycou4DPxWJ19/3+efTPtWXBoI4TE=;
        b=o6vMHm6x7hgkhoLZhrgovw91WucaHtasIIiTEA5WqUVQ4k5NHFj6sB+lq2PGNRiAfW
         hkZtadsBxO6/FQWyZZ3sHVzomzpPKcyJYdKe6RXnGpevSaLjC81y/bYOCQ96/9k0NDcz
         OAEza8I22YHl6i8VWdYzASSQbCvEjNRmtTpQLNwNYM7PK3qdEZ6Ag+35hbMjfvvyxntC
         WmVycz1mE71AeNrRUrZ1wtbjnyJFBmmNq7KQm7LK3/yeab+gSGjsuNtFWvqc4A5XUQTX
         UADrNB/6HBztv12uyLtuE0jyz7r5FVTLAT0h6ymP7dA4F9dCpRlXBKRpxejj6OfCCUI4
         dyhQ==
X-Forwarded-Encrypted: i=1; AJvYcCXrgtEXgmSuOn4kA7JUfZKAg9mbOm7ouxkPd2Fx1BburSZNy7FC29xWNi3sPwHSpbnpoI3FMLoDrHUtslSp@lists.linux.dev
X-Gm-Message-State: AOJu0YyEN/pVkSw61w9kNJuSXwY+QLfs25Uoq9I9prTQ2V8GBYBOnoHd
	B4MI0a2072DMr775G2Y9HXSxR3NDFjjVf7ifJmx2kpSZDDwzqp5EZCV7
X-Gm-Gg: ASbGncvI3dPw8QTP4j7Scu0N8TaKEQS6zr2KrTsxVUmlD2WdCCEl8g8YSPBF32SGJhl
	a2h4S/rKH+cuO/6ASzV2s7LP2H9C3P5bNfnmRRb8Z/7kmD5W+RQH7nLCmWJFcAmSMjsFvC29mba
	TiPSSo2zlEsXA7UHnLXHMoCsB1HV+OJ3htNbTHBbkoN+dY8g4uIV9l7fpDfeKI6+R2ehfUxuU9E
	VXlFZly5GBUMvp7o/o/pmy9y0oHZfADXiNe8zbIgniUywWz6BRGB2O2Zqf9CzN4M66YBwaAzk++
	88jfONLZ1DCSow8j6jAxvFvyUm5/5D5tLDrLgpHMgb4CdRHtK3WQzrvsDGreAxz9go7SUREicf4
	ZpLse/uLNSmOJ+IzDbCEhVVT1Vead6XEl03b1HS5quvk8OD8AlIMQFv5A7gYwB6aoFzv+bzkvB4
	2zLtyh
X-Google-Smtp-Source: AGHT+IFzEwVcxMfQ7XalQBhmYFyxbLkLDeKDjgoqS30M2rsyVg2UheDko9Zat6hdGvCGTyCQVsOfPg==
X-Received: by 2002:a17:906:6a1f:b0:ae6:f087:953 with SMTP id a640c23a62f3a-afe28f8581cmr1691238766b.12.1756245914523;
        Tue, 26 Aug 2025 15:05:14 -0700 (PDT)
Received: from XPS.. ([2a02:908:1b0:afe0:c431:8253:5fc5:acbd])
        by smtp.gmail.com with ESMTPSA id a640c23a62f3a-afe849d7e6bsm481602066b.61.2025.08.26.15.05.13
        (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);
        Tue, 26 Aug 2025 15:05:14 -0700 (PDT)
From: Osama Abdelkader <osama.abdelkader@gmail.com>
To: gregkh@linuxfoundation.org,
	dpenkler@gmail.com,
	matchstick@neverthere.org,
	dan.carpenter@linaro.org,
	arnd@arndb.de
Cc: linux-kernel@vger.kernel.org,
	linux-staging@lists.linux.dev,
	Osama Abdelkader <osama.abdelkader@gmail.com>
Subject: [PATCH] staging: gpib: simplify and fix get_data_lines
Date: Wed, 27 Aug 2025 00:05:02 +0200
Message-ID: <20250826220502.210485-1-osama.abdelkader@gmail.com>
X-Mailer: git-send-email 2.43.0
Precedence: bulk
X-Mailing-List: linux-staging@lists.linux.dev
List-Id: <linux-staging.lists.linux.dev>
List-Subscribe: <mailto:linux-staging+subscribe@lists.linux.dev>
List-Unsubscribe: <mailto:linux-staging+unsubscribe@lists.linux.dev>
MIME-Version: 1.0
Content-Transfer-Encoding: 8bit

The function `get_data_lines()` in gpib_bitbang.c currently reads 8
GPIO descriptors individually and combines them into a byte.
This has two issues:

  * `gpiod_get_value()` returns an `int` which may be negative on
    error. Assigning it directly into a `u8` may propagate unexpected
    values. Masking ensures only the LSB is used.
  * The code is repetitive and harder to extend.

Fix this by introducing a local array of GPIO descriptors and looping
over them, while masking the return value to its least significant bit.

This reduces duplication, makes the code more maintainable, and avoids
possible data corruption from negative `gpiod_get_value()` returns.

Signed-off-by: Osama Abdelkader <osama.abdelkader@gmail.com>
---
 drivers/staging/gpib/gpio/gpib_bitbang.c | 19 +++++++++----------
 1 file changed, 9 insertions(+), 10 deletions(-)

diff --git a/drivers/staging/gpib/gpio/gpib_bitbang.c b/drivers/staging/gpib/gpio/gpib_bitbang.c
index 17884810fd69..b6d1e650687f 100644
--- a/drivers/staging/gpib/gpio/gpib_bitbang.c
+++ b/drivers/staging/gpib/gpio/gpib_bitbang.c
@@ -1403,16 +1403,15 @@ static void set_data_lines(u8 byte)
 
 static u8 get_data_lines(void)
 {
-	u8 ret;
-
-	ret = gpiod_get_value(D01);
-	ret |= gpiod_get_value(D02) << 1;
-	ret |= gpiod_get_value(D03) << 2;
-	ret |= gpiod_get_value(D04) << 3;
-	ret |= gpiod_get_value(D05) << 4;
-	ret |= gpiod_get_value(D06) << 5;
-	ret |= gpiod_get_value(D07) << 6;
-	ret |= gpiod_get_value(D08) << 7;
+	struct gpio_desc *lines[8] = {
+		D01, D02, D03, D04, D05, D06, D07, D08
+	};
+	u8 ret = 0;
+	int i;
+
+	for (i = 0; i < 8; i++)
+		ret |= (gpiod_get_value(lines[i]) & 1) << i;
+
 	return ~ret;
 }
 
-- 
2.43.0


From mboxrd@z Thu Jan  1 00:00:00 1970
Received: from mail-wr1-f54.google.com (mail-wr1-f54.google.com [209.85.221.54])
	(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 69103223DD5
	for <linux-staging@lists.linux.dev>; Wed, 27 Aug 2025 07:15:40 +0000 (UTC)
Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=209.85.221.54
ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116;
	t=1756278942; cv=none; b=pWHx6m1wTEcYlb7+uRPl/kidRml/dQjsBNp/nbAzkQtzVU2qRlkUnzfZBcClWcn1QU2w07loFgQuNF14qLg6NMBI+39IBt56h0VPogJTh6/7jCQt5qzTP01YSWY5f+qnzZLafsCt9M3X1zZyhqLWlESSPx7SeT0/Z54SwLU46Wo=
ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org;
	s=arc-20240116; t=1756278942; c=relaxed/simple;
	bh=eJUdkWuojsgRcPJQERC9HoSP+HFyVytDvOc649sWpJU=;
	h=Date:From:To:Cc:Subject:Message-ID:References:MIME-Version:
	 Content-Type:Content-Disposition:In-Reply-To; b=HUgPT+l9+eJtrcB6C3AuV5Zn4iKN4yuWXk4PWe/Snovfb5H6j5BCzaL4uzaTzBAI7TMT9YAX40jrOSs3H/19ZA9ZDjpVsCF8fKjrCVVqplE7NRpxqAbp7AQzB3djf9gC5G4+RIB+BHdtlCOwd+VY45PG9BbOUE0nyV3TbMUxFHA=
ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=linaro.org; spf=pass smtp.mailfrom=linaro.org; dkim=pass (2048-bit key) header.d=linaro.org header.i=@linaro.org header.b=jULbaoCx; arc=none smtp.client-ip=209.85.221.54
Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=linaro.org
Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=linaro.org
Authentication-Results: smtp.subspace.kernel.org;
	dkim=pass (2048-bit key) header.d=linaro.org header.i=@linaro.org header.b="jULbaoCx"
Received: by mail-wr1-f54.google.com with SMTP id ffacd0b85a97d-3c380aa1ad0so3367277f8f.3
        for <linux-staging@lists.linux.dev>; Wed, 27 Aug 2025 00:15:40 -0700 (PDT)
DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed;
        d=linaro.org; s=google; t=1756278939; x=1756883739; darn=lists.linux.dev;
        h=in-reply-to:content-disposition:mime-version:references:message-id
         :subject:cc:to:from:date:from:to:cc:subject:date:message-id:reply-to;
        bh=hlLv4S/UPFBuBEfiiKihhCuycUkTi3/tZ4pnK9d2kGk=;
        b=jULbaoCxy/x6Xxl4bgmkXXPP50YRPAKFlstR2Dw9F6CkcV6h1X4EqvQ0IQTvanjIYq
         mDdC/5Fpc2O+WDvv2gLIWdKaWJpSKrb/JIQFLl7tObmX/xRfEJBC74UuteHLH+x1QhsE
         F8y6Vo0dn6B5F1k9ju7+JZ4aQ0UidqpA5P9E5OMBc5L6Vt3+E36WPyXci4AMaLL53WFb
         oPN4Fpfq3ZWYMTYDhIPJojG36HtVlf9PXDpOyCZVsA7WYX3X4HU3t29y6dkFZ82pJSXO
         p2wGaB8yfNGzIOv2jtjMKhFQKX57fE88qBtDH3yBVd77Eae5wZA5aUBA/mhDJGFgJLFw
         Z4pw==
X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed;
        d=1e100.net; s=20230601; t=1756278939; x=1756883739;
        h=in-reply-to:content-disposition:mime-version:references:message-id
         :subject:cc:to:from:date:x-gm-message-state:from:to:cc:subject:date
         :message-id:reply-to;
        bh=hlLv4S/UPFBuBEfiiKihhCuycUkTi3/tZ4pnK9d2kGk=;
        b=wBWLKdqrLVFNms71Ou9zQcVg0Osl1kjguxRO7CjowACBBV2wrV7kBR54bd663ZZZMx
         cUivu5vL/ye7iGP4wvN/4Oa1b+WdzE22cA8zVsS/bz5IPSWbaF/DSbzVi6zkrd2WcQPt
         L5oJtcN62zgac9t7B5Ibghge5MybRI5IVo8R86zWW2GlIsic4hBfpHF5paigxmx2dBzo
         Rq1gAI8Ht0fj6kqMd1uRrb8XlFiQ//ONEaHtBOR9nNo4//qPXfVU6RcMf8vJkwLJXYK8
         wwUa14N01f7OdaFm72RKMjJl5AnDvx199YdsoXbfWFlWXZYvxmvxPjj9CujdkOmxP4Pz
         G9Rw==
X-Forwarded-Encrypted: i=1; AJvYcCWmKeAE03OwZ7Rj1SgVn3NUMtbNukIEdwbqk94q5I8RtizE2o2V+hxla0p19TDYpGBvR+Pz7TdxGVjbjgcY@lists.linux.dev
X-Gm-Message-State: AOJu0Yydkw5BIUp+vZS3NMmYTZD2KzAOzA9DEZJ/xHaiucuKrowqCzmo
	+5s3Ay2WcidGdpsoavZwKupmiofMTWS/T4O+DwiEhWTAs9TX1Pf0dUJVcmi81pYBRug=
X-Gm-Gg: ASbGncsrvKfU2d0j2DZeNleRGzVIiAZPWs7MwaZbF8uibTX7pYeXStUmh0F43zEABkF
	4XMvND2xh9FsDVUR0vJTcI4H/hqJMBPonaMyx1+ve5fAY6b4UmVDqqI+uyfohOexdnumsYBjM79
	i8xNOJSt29WYRF8onprKG1NZA/49Xqt46wuhOh5L4BTcbkf7yKNQW0PKQNxTnZe0zU+FUbGpJZw
	4cApnu8Yk+X2Q3IiAUQ3R6KQogg/fxF4GEZwdnpmMHe5XOLkTKPgDrkd9e6uetuAzw3PETrXjkL
	RINnX3lO6ezk2keE7WJu9YSLtuj0VOpAqvQTy35S1GT5Itt72opUTWRQgRFSYHTAwOsNXufi7HF
	AOnrxBya3Kl6J6etX1mGI9Kbp4zM=
X-Google-Smtp-Source: AGHT+IHGLQK+/bIqO0EGakdV6TeJY1UPjMkYcLkAtbFxpvXgRAwvr0BTLtLlmQ9ZA9cZOdmyT34Qdw==
X-Received: by 2002:a05:6000:2304:b0:3ca:ad45:6363 with SMTP id ffacd0b85a97d-3caad456a18mr5084286f8f.17.1756278938614;
        Wed, 27 Aug 2025 00:15:38 -0700 (PDT)
Received: from localhost ([196.207.164.177])
        by smtp.gmail.com with UTF8SMTPSA id 5b1f17b1804b1-45b6b1cdf05sm14123395e9.1.2025.08.27.00.15.36
        (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);
        Wed, 27 Aug 2025 00:15:37 -0700 (PDT)
Date: Wed, 27 Aug 2025 10:15:33 +0300
From: Dan Carpenter <dan.carpenter@linaro.org>
To: Osama Abdelkader <osama.abdelkader@gmail.com>
Cc: gregkh@linuxfoundation.org, dpenkler@gmail.com,
	matchstick@neverthere.org, arnd@arndb.de,
	linux-kernel@vger.kernel.org, linux-staging@lists.linux.dev
Subject: Re: [PATCH] staging: gpib: simplify and fix get_data_lines
Message-ID: <aK6wlcLBN1HclMpl@stanley.mountain>
References: <20250826220502.210485-1-osama.abdelkader@gmail.com>
Precedence: bulk
X-Mailing-List: linux-staging@lists.linux.dev
List-Id: <linux-staging.lists.linux.dev>
List-Subscribe: <mailto:linux-staging+subscribe@lists.linux.dev>
List-Unsubscribe: <mailto:linux-staging+unsubscribe@lists.linux.dev>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20250826220502.210485-1-osama.abdelkader@gmail.com>

On Wed, Aug 27, 2025 at 12:05:02AM +0200, Osama Abdelkader wrote:
> The function `get_data_lines()` in gpib_bitbang.c currently reads 8
> GPIO descriptors individually and combines them into a byte.
> This has two issues:
> 
>   * `gpiod_get_value()` returns an `int` which may be negative on
>     error. Assigning it directly into a `u8` may propagate unexpected
>     values. Masking ensures only the LSB is used.

Using the last bit in an error code is not really "error handling"...

What you could do instead would be something like:

	int ret;

	for (i = 0; i < 8; i++) {
		ret |= (gpiod_get_value(lines[i]) & 1) << i;
		if (ret < 0) {
			pr_err("something failed\n");
			return -EINVAL;
		}
	}

	return ~ret;

Which might also not be correct, but it's probably closer to being okay.

regards,
dan carpenter


From mboxrd@z Thu Jan  1 00:00:00 1970
Received: from mail-wm1-f47.google.com (mail-wm1-f47.google.com [209.85.128.47])
	(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 A3160191F6A
	for <linux-staging@lists.linux.dev>; Wed, 27 Aug 2025 08:07:13 +0000 (UTC)
Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=209.85.128.47
ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116;
	t=1756282035; cv=none; b=WV23tntaZ3EZGuhILS6SUIU/FHFmIBnfxCbx6wFoPd+8FidBJqRW8jmWvBXjGRkku5D5kbFjjdqC2mGMPPVNxcDYqBWB8rUtASYymrzwJ4c3KzhY0lD2w7GsHutEXfyWJV5QVyxhhjDyCyd6CRHCNdTR16N/4wo8eD76OJEC0Xc=
ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org;
	s=arc-20240116; t=1756282035; c=relaxed/simple;
	bh=5HApl7J1+KZSg0OQPRohvfP1g3izBueYr15Dlsj4rF8=;
	h=Date:From:To:Cc:Subject:Message-ID:References:MIME-Version:
	 Content-Type:Content-Disposition:In-Reply-To; b=e7T2lQ+SG8V1jselPW5BMDxE0oI8nKxjMQRhCJNPh/ww8BL0TdwRJFAibUmdMGTOGRCjcHa89fPPX8uoixZNIBNCDtao68OY1Z2+mHsGVBByucmKJqvu0ff2lQQWzlGIQoeQrByDEOX3x/gpUZtAMExTrTySRnVXvEfehh8rMF8=
ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=linaro.org; spf=pass smtp.mailfrom=linaro.org; dkim=pass (2048-bit key) header.d=linaro.org header.i=@linaro.org header.b=B+RaifF+; arc=none smtp.client-ip=209.85.128.47
Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=linaro.org
Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=linaro.org
Authentication-Results: smtp.subspace.kernel.org;
	dkim=pass (2048-bit key) header.d=linaro.org header.i=@linaro.org header.b="B+RaifF+"
Received: by mail-wm1-f47.google.com with SMTP id 5b1f17b1804b1-45b6278893eso8732035e9.0
        for <linux-staging@lists.linux.dev>; Wed, 27 Aug 2025 01:07:13 -0700 (PDT)
DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed;
        d=linaro.org; s=google; t=1756282032; x=1756886832; darn=lists.linux.dev;
        h=in-reply-to:content-disposition:mime-version:references:message-id
         :subject:cc:to:from:date:from:to:cc:subject:date:message-id:reply-to;
        bh=+XEEEilHfE1GLhoyVzsYIneNopkvi33gatO1klgimhI=;
        b=B+RaifF+oDXhBfZKn2CrPAxu+PCD5QmskQsK89ZrQa72T/H/7BEaMYDp4pUFLb5Hgb
         Ar8DFNfevIrbIn0EcEsRQzUuFYIYl6DIOaWo91736yxXljX/EQ+VeZfp66C1tpEC1Tu6
         GS9+xNCeLkDfdTSNffKXaIHICtubeVOK0sP9w3spJBD2AxlKpnGK++3UNlo3BMGO4epB
         OPvXuz7o2x9PaMlSv6nziQeDqUpAGizNdC18MlC9STlPyNAjmrjY2P2HnKrnX6i91LdQ
         GdcEMrJnULxQoDdauka9dqvkgN3/RCmuPqphGJUE2Ah3k4XG0x94YRkG1XBsegnIEGpk
         c2qg==
X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed;
        d=1e100.net; s=20230601; t=1756282032; x=1756886832;
        h=in-reply-to:content-disposition:mime-version:references:message-id
         :subject:cc:to:from:date:x-gm-message-state:from:to:cc:subject:date
         :message-id:reply-to;
        bh=+XEEEilHfE1GLhoyVzsYIneNopkvi33gatO1klgimhI=;
        b=a0mzNITsxR185HiIHHYJUZaXDISSdJQTMwxWUu+xam+oTnCcu/3NV84AlMnBErNCqP
         +5hErzOk4bYxEyqBmcqiQMcHGOxDymg3sLGRS+GTGlSLa/BPAO+C4CW7UDfu+9mUU8Eg
         H+nKHOOwi7XWdYWs5fzbEFHneU4qrruzXZ3MotKGB8XsyVBUOmYUuSAoM3KwkiAxZL04
         ZoNktilOhRdoBMShWgHEYO9awcaIN59hH/qXXj77qw5rjtf5zMxeM/PasSl9eMrS+Vfy
         HRLVn42Wx26Xklj6GnthTPFEdDWQBDV09OOqDDU4cxN+Iq6Nl8XHjy0ORGfYK+o5iGEW
         tf8A==
X-Forwarded-Encrypted: i=1; AJvYcCXJ2b+5VNNEay2QSNdgCABDs0S2IkU8AVuEFUzF1BIDnnKPY6wISAaaO2cQr65/IppWGSTM+lvQ4gwbpkVU@lists.linux.dev
X-Gm-Message-State: AOJu0YwGkgpcr9yViGPseSjkohnpt02/eWc5dXkekP/Rkcfo9syt6eZl
	iaMkv2CBfZwL9Z3dFT6qTAVZEPbLiK3GEYf3/wdi4UMeN6Uh5GM9tUjNCxaP1QJVHXM=
X-Gm-Gg: ASbGncuV2jlsEf8vbFcGmpQqedS9yLq/BUAam99aZRVLoWDbnL8j45gq6jxJ7kU07NN
	p2c5uVUZGW5/W5MAgcvNOtZgtCpsP7MVQYiu0/lIGgJCxSrhvg3JaDHARSLhgsa4K6tkNf2sRSv
	gNCovjtAX7W3djfeUjLRRgX7KEbjz2Dgl1Ldw+iznY+EXYU7WImohyDNONZDxw3xSSQAsySLTDu
	bIzqoDkjJ8W8zeHTtNf1WLU8WBc4J5VLbOpNWbww7zWeCCQTZ1apNLUB2Oei8FZ4krnC9L6+7AF
	YqRRH7RgBPwXIHGkDQgrSEt/c53rc/a4nS9vCd5c67i8q30OiuJFFpBzRwTEoyKt9ve3APFM3UY
	VDqauGr7I596WE/8QhxEnKeWiwOs=
X-Google-Smtp-Source: AGHT+IE8CHB+zttlbDDIwDXRSFgPzyUYpDYBRdNDJ3qUHnqYkUBhH/KN69QYUoUlHi/jGAxq2dXn+Q==
X-Received: by 2002:a05:600c:4687:b0:45b:47e1:ef71 with SMTP id 5b1f17b1804b1-45b517f8e7fmr156917515e9.36.1756282032014;
        Wed, 27 Aug 2025 01:07:12 -0700 (PDT)
Received: from localhost ([196.207.164.177])
        by smtp.gmail.com with UTF8SMTPSA id 5b1f17b1804b1-45b6f0f320esm19676075e9.16.2025.08.27.01.07.09
        (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);
        Wed, 27 Aug 2025 01:07:11 -0700 (PDT)
Date: Wed, 27 Aug 2025 11:07:05 +0300
From: Dan Carpenter <dan.carpenter@linaro.org>
To: Osama Abdelkader <osama.abdelkader@gmail.com>
Cc: gregkh@linuxfoundation.org, dpenkler@gmail.com,
	matchstick@neverthere.org, arnd@arndb.de,
	linux-kernel@vger.kernel.org, linux-staging@lists.linux.dev
Subject: Re: [PATCH] staging: gpib: simplify and fix get_data_lines
Message-ID: <aK68qXqStIwBrejF@stanley.mountain>
References: <20250826220502.210485-1-osama.abdelkader@gmail.com>
 <aK6wlcLBN1HclMpl@stanley.mountain>
Precedence: bulk
X-Mailing-List: linux-staging@lists.linux.dev
List-Id: <linux-staging.lists.linux.dev>
List-Subscribe: <mailto:linux-staging+subscribe@lists.linux.dev>
List-Unsubscribe: <mailto:linux-staging+unsubscribe@lists.linux.dev>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <aK6wlcLBN1HclMpl@stanley.mountain>

On Wed, Aug 27, 2025 at 10:15:33AM +0300, Dan Carpenter wrote:
> On Wed, Aug 27, 2025 at 12:05:02AM +0200, Osama Abdelkader wrote:
> > The function `get_data_lines()` in gpib_bitbang.c currently reads 8
> > GPIO descriptors individually and combines them into a byte.
> > This has two issues:
> > 
> >   * `gpiod_get_value()` returns an `int` which may be negative on
> >     error. Assigning it directly into a `u8` may propagate unexpected
> >     values. Masking ensures only the LSB is used.
> 
> Using the last bit in an error code is not really "error handling"...
> 
> What you could do instead would be something like:
> 
> 	int ret;
> 
> 	for (i = 0; i < 8; i++) {
> 		ret |= (gpiod_get_value(lines[i]) & 1) << i;
> 		if (ret < 0) {
> 			pr_err("something failed\n");
> 			return -EINVAL;

I meant to write "return 0;".  It's type u8.

Also that doesn't work.  The masks and shift mess it up.

	u8 val = 0;
	int ret;

	for (i = 0; i < 8; i++) {
		ret = gpiod_get_value(lines[i]);
		if (ret < 0) {
			pr_err("something failed\n");
			continue;
		}
		val |= ret << i;
	}

	return ~val;

regards,
dan carpenter


From mboxrd@z Thu Jan  1 00:00:00 1970
Received: from mail-ed1-f43.google.com (mail-ed1-f43.google.com [209.85.208.43])
	(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 1B5161D5150
	for <linux-staging@lists.linux.dev>; Wed, 27 Aug 2025 09:28:39 +0000 (UTC)
Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=209.85.208.43
ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116;
	t=1756286921; cv=none; b=JuuP91g17d4iIsjl3+KnAD6D3BKZf2F0e6mUXKtJB19Nxhqasg2gv6PLaJxIyyw2KAf9pGUJigQIMdYSjfUn+XKvEHRIunxDuiMALpySZykY2CUBTvbUng/SopFz8KKbkrdwgWqVO5ynmQemYB/QS9TAFUkPYSq67atVJajDA/k=
ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org;
	s=arc-20240116; t=1756286921; c=relaxed/simple;
	bh=u1JT9esYUPh//8vGQAYEOXy1sSFQYIQb0ssbEMuIBNo=;
	h=Message-ID:Date:MIME-Version:Subject:To:Cc:References:From:
	 In-Reply-To:Content-Type; b=fkZaDHbr+HLo+bEIEiTlmS6pbuJoAjilYIAo/Y3T6fl3R7OhGZ3uyH0l1IXNY2xI6gJuLwSgSWB4s69TTPLv29bOyQuhweOdCTwr40QGna2nlH2ZQE/k7uWKHkEl8L0GVIB4mAmxYvea/68x8AWTFZvYpFJIAg/NR7u91HmBpZI=
ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=gmail.com; spf=pass smtp.mailfrom=gmail.com; dkim=pass (2048-bit key) header.d=gmail.com header.i=@gmail.com header.b=elSVr/aH; arc=none smtp.client-ip=209.85.208.43
Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=gmail.com
Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=gmail.com
Authentication-Results: smtp.subspace.kernel.org;
	dkim=pass (2048-bit key) header.d=gmail.com header.i=@gmail.com header.b="elSVr/aH"
Received: by mail-ed1-f43.google.com with SMTP id 4fb4d7f45d1cf-6188b793d21so9791158a12.3
        for <linux-staging@lists.linux.dev>; Wed, 27 Aug 2025 02:28:39 -0700 (PDT)
DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed;
        d=gmail.com; s=20230601; t=1756286918; x=1756891718; darn=lists.linux.dev;
        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=5WNhk1ZhM3W2L38lczRb2sJSYi/8+nhFpxpIIZjaaw4=;
        b=elSVr/aHjmWbiRSeRRLHwegjsInCXyeNo5JXPecDnY7XMLXVl15gTVYOotnqy4KbKy
         8zeY3DcAGLW+zQmVJgDvN7SJjKydyJ/eHGXvaLG6323Xze94OhmQrj3qf6dpAmPXsTsw
         FquV8/+ow48fKMQ53QAIQd28Zpy5CAmvhKWFill2YYZt0WR7PQPAvNfSmfS3PaJ2tDdM
         GpKis/4v+7qtOBirx80E1QnAjrqPuXUEqG5KWYumI7vrO6Rzj3ipx4nTzo0ebfV8eZrK
         5J/lxdM938QmjyJv/F+2EpNDQDWEtK2uIjN03Q13GWCqpDEir+rCZpWCJ99huJxsVOBm
         GpPQ==
X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed;
        d=1e100.net; s=20230601; t=1756286918; x=1756891718;
        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=5WNhk1ZhM3W2L38lczRb2sJSYi/8+nhFpxpIIZjaaw4=;
        b=syYFxvWAE/4Wx8iWK9U63pW5s6Nd3uRWREJmVSKd2anvoMlz5Ekd8GBMFeSyZAFlUJ
         XCI75Lgn7zZpHalZkzUeiEvRrfWsye8wV2dqQs3z1bYU2tsKJHd770EPenS7V9ZFx7dA
         C55zDdUl771lsO2fQEavXRud1xBB8SKEFx8nZmVJcCNcnElCt0/HJJosWfR4fWO8FZu0
         O4PcC0vh7Oslg4xV/xpkbKpq29XrJEMSQw7XSfs5FnEwznECFly9zQrn+sO5UEO4UqKI
         LKs6T+IUCi4n75Z8z0Cip+Fyxl7bvYYY1bwl9Uo5fYoZdDbWB8pa3T9glhWKnYCkT/Re
         1uTg==
X-Forwarded-Encrypted: i=1; AJvYcCW+bZaRniEy9STAnR1g8rwmn5uIsdF7HN9eHl6L3OA369bjXIuQuKfs8jkgZgpEUc9ayHi2ibjVlLHGX9mM@lists.linux.dev
X-Gm-Message-State: AOJu0Yy8723JAHYfr5KatTbzhEwCwGXmhNQWbfewno/dMAWmTjaABmLL
	Y0+FOJvjnYYQyPKRfoHuE+f4Uuf1g5GAhNe2YaOj7tWNYGkcwiB4sCCj
X-Gm-Gg: ASbGncsmCG4gArYj4k+3EYbIpui9apQvqKrmkah46QgewGlNFb9IrMDqH2oX6n/7Tzw
	l1QcHRdYdRAueKwfmhE3ImEu6tVT+rDmBMu1BiPaVsQO8uW7bFghvQDqz4A9A6fHCD61OLxKN+S
	NrzVqhohOeokL69ee35XqceYIhtO/7J3ehufBMjmiJsNCP9BL6soRBGWTxuW8Wrr/9uZVHCuQZY
	hLySFgRkL5GgB3dyVXnPntisiWSaf33Vnp61FxW0dHw32mYxEHx8tbMWSAOrfxrkE90fr2lBVBj
	8Mc/YkZ+YUYPElv8DYkHMLGoIWg4x9xLE0TZXXx1Ct/j2hwpeKm2vs/5JUpnSithDRubQ9Emv/r
	0bM/G/DdiPNzQTNAUux2x++o5dB5jsImLyqO6oa5QcAYEeIrPMd4shtYNMw0Etpi0HufB6WJIbQ
	4HNP4GJv2bKE3HnNzI8AocIkBtRP4/EWPaOM+HdDxyRssHwooRmYJsaXmH
X-Google-Smtp-Source: AGHT+IEiUn0mw0Bd+eWTikezNk1J+BkpH25CzT2HWGH+6VVEhsrZO7sZhbdFaQJFJa9fYRGNuchv1Q==
X-Received: by 2002:a17:907:72c9:b0:afd:d6e0:ee39 with SMTP id a640c23a62f3a-afe295c9153mr1713662666b.50.1756286918192;
        Wed, 27 Aug 2025 02:28:38 -0700 (PDT)
Received: from ?IPV6:2a02:908:1b0:afe0:92e9:ef08:4a92:adcf? ([2a02:908:1b0:afe0:92e9:ef08:4a92:adcf])
        by smtp.gmail.com with ESMTPSA id a640c23a62f3a-afe79b08cfasm684913866b.101.2025.08.27.02.28.37
        (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128);
        Wed, 27 Aug 2025 02:28:37 -0700 (PDT)
Message-ID: <f3ab3c2d-2056-4802-aa73-2b0db4c7fc30@gmail.com>
Date: Wed, 27 Aug 2025 11:28:36 +0200
Precedence: bulk
X-Mailing-List: linux-staging@lists.linux.dev
List-Id: <linux-staging.lists.linux.dev>
List-Subscribe: <mailto:linux-staging+subscribe@lists.linux.dev>
List-Unsubscribe: <mailto:linux-staging+unsubscribe@lists.linux.dev>
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH] staging: gpib: simplify and fix get_data_lines
To: Dan Carpenter <dan.carpenter@linaro.org>
Cc: gregkh@linuxfoundation.org, dpenkler@gmail.com,
 matchstick@neverthere.org, arnd@arndb.de, linux-kernel@vger.kernel.org,
 linux-staging@lists.linux.dev
References: <20250826220502.210485-1-osama.abdelkader@gmail.com>
 <aK6wlcLBN1HclMpl@stanley.mountain> <aK68qXqStIwBrejF@stanley.mountain>
Content-Language: en-US
From: Osama Abdelkader <osama.abdelkader@gmail.com>
In-Reply-To: <aK68qXqStIwBrejF@stanley.mountain>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit


On 8/27/25 10:07 AM, Dan Carpenter wrote:
> On Wed, Aug 27, 2025 at 10:15:33AM +0300, Dan Carpenter wrote:
>> On Wed, Aug 27, 2025 at 12:05:02AM +0200, Osama Abdelkader wrote:
>>> The function `get_data_lines()` in gpib_bitbang.c currently reads 8
>>> GPIO descriptors individually and combines them into a byte.
>>> This has two issues:
>>>
>>>   * `gpiod_get_value()` returns an `int` which may be negative on
>>>     error. Assigning it directly into a `u8` may propagate unexpected
>>>     values. Masking ensures only the LSB is used.
>> Using the last bit in an error code is not really "error handling"...
>>
>> What you could do instead would be something like:
>>
>> 	int ret;
>>
>> 	for (i = 0; i < 8; i++) {
>> 		ret |= (gpiod_get_value(lines[i]) & 1) << i;
>> 		if (ret < 0) {
>> 			pr_err("something failed\n");
>> 			return -EINVAL;
> I meant to write "return 0;".  It's type u8.
>
> Also that doesn't work.  The masks and shift mess it up.
>
> 	u8 val = 0;
> 	int ret;
>
> 	for (i = 0; i < 8; i++) {
> 		ret = gpiod_get_value(lines[i]);
> 		if (ret < 0) {
> 			pr_err("something failed\n");
> 			continue;
> 		}
> 		val |= ret << i;
> 	}
>
> 	return ~val;

We can change the return type to int and propagate the error, so:

static int get_data_lines(u8 *out)

{

	int val, i;

	u8 ret = 0;

	struct gpio_desc *lines[8] = { D01, D02, D03, D04, D05, D06, D07, D08 };
	for (i = 0; i < 8; i++) { 		val = gpiod_get_value(lines[i]);

		if (val < 0)

			return val; // propagate error

		ret |= (val & 1) << i;

	}

	*out = ~ret; 	return 0;

}

Then in the caller:

u8 data;
if (!get_data_lines(&data))

	priv->rbuf[priv->count++] = data;

or we print the error here, What do you think?

>
> regards,
> dan carpenter
>

From mboxrd@z Thu Jan  1 00:00:00 1970
Received: from mail-wm1-f48.google.com (mail-wm1-f48.google.com [209.85.128.48])
	(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 0E7142820BA
	for <linux-staging@lists.linux.dev>; Wed, 27 Aug 2025 10:09:41 +0000 (UTC)
Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=209.85.128.48
ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116;
	t=1756289383; cv=none; b=YYz4o6Goq1aGFDZPuWydNb005IKyaQRDHhSlH53mauoOQn2EAJ36z9373u/xj1Fb8DkOcrHjdpfx4S/xpt7BdbRKbKncWMmdVbiKlWBSDEpp3nmrja0akaAnzHQyMj1aVOz84alPR2DFeTWqLe6g49MvvJhs+xeW1IWdrLQhnCE=
ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org;
	s=arc-20240116; t=1756289383; c=relaxed/simple;
	bh=J0lkcTHUid3XxD8U8mPToo1lJJyfeJ5m6rE6h09yz5g=;
	h=Date:From:To:Cc:Subject:Message-ID:References:MIME-Version:
	 Content-Type:Content-Disposition:In-Reply-To; b=HxeNzeT8CglH8Uzb2Fc2TCGpL/Y2Uyib630RuGeq8FLZaOF1yyel82hnhR2dIYEUcRYbqMkOly0R+EN2O8Ej1S/LAjuQAmcMRh740uXCPYPV6jJBuU73w/btIrEsYT3Te0xaChEgphsPXxlI5upLpm1qBmiR78yxaXs+Wk5/Iw8=
ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=linaro.org; spf=pass smtp.mailfrom=linaro.org; dkim=pass (2048-bit key) header.d=linaro.org header.i=@linaro.org header.b=ECU8fq5w; arc=none smtp.client-ip=209.85.128.48
Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=linaro.org
Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=linaro.org
Authentication-Results: smtp.subspace.kernel.org;
	dkim=pass (2048-bit key) header.d=linaro.org header.i=@linaro.org header.b="ECU8fq5w"
Received: by mail-wm1-f48.google.com with SMTP id 5b1f17b1804b1-45b4e5c3d0fso16295735e9.2
        for <linux-staging@lists.linux.dev>; Wed, 27 Aug 2025 03:09:41 -0700 (PDT)
DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed;
        d=linaro.org; s=google; t=1756289380; x=1756894180; darn=lists.linux.dev;
        h=in-reply-to:content-disposition:mime-version:references:message-id
         :subject:cc:to:from:date:from:to:cc:subject:date:message-id:reply-to;
        bh=VPJo3DFYnaR9E2CWsARN7ky89otQMe3GktK/razN7cg=;
        b=ECU8fq5wtJfcD9UruWQXlryZ+wNojLP7/qeyHQ/Gd8uSneFwFMdY5Ji2XXX5m3mxQU
         nUdEKrv/2qg3D5HkCyaZZkMmIEPGt/dSaCdyiYbF3xfCBFPhcu4woOPlatmINEtO6Du/
         cDxyXyl6hq7wgi3hRwrdu8U+xBWl+X5hny7+a7WmknOiXedaaMNyOan5vj22oZpCQgyY
         lQ15krSqDsjvux2GxEJoHAUp5YC74AnCPdS+mFEaQ7L6Uy+JwtlrlJaQCoE89khj0qg6
         UN5cKqxIqZfWORbcCYYRKdvPc4fphfj1LGfu48eJJPBoBuDEFZUOWU00S5AfL+xx29kS
         UbkQ==
X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed;
        d=1e100.net; s=20230601; t=1756289380; x=1756894180;
        h=in-reply-to:content-disposition:mime-version:references:message-id
         :subject:cc:to:from:date:x-gm-message-state:from:to:cc:subject:date
         :message-id:reply-to;
        bh=VPJo3DFYnaR9E2CWsARN7ky89otQMe3GktK/razN7cg=;
        b=tkDfBDLBQ3VxoDvZKYjg2RcdJhr1zjnshb/i7t7CkSJZd6j+U4W7rAyVb/i+6P9eNc
         YcQmS5NAnUAvqUfxmvVwpoIVOvEkuRMqs92vXfq9WnsamZUj86jpVIGD5T6WC6K6BfLj
         NjNE3VVwqyFq+Au6O2lsX30NO2GFTbYzL6qRhzXP2pqfR7dL8WkRjvz3bVyoNoSFdvue
         +SZNfctDUIgLDWsXJfOBNSOGs97iYxUpZu/DjdUD+1gBI/m4gjio49rUVlNy29qh4kRm
         0r2SJ3fio8xMa3PMHmUIQ+rlixCnSkxaetsdxvKSS2gHSrXHoLAiWSY2zcRK0Hdm4b7Z
         pMWg==
X-Forwarded-Encrypted: i=1; AJvYcCXwmy5NJw10HYImwBfsEz5KGI2R7EpThTFbXzYYoVvHyuS9Z0iSh+MBXXpUNnBQQ+OPvAiO5a7ASKvdAyNI@lists.linux.dev
X-Gm-Message-State: AOJu0YwLs+z44zZ4fhADw5AOslrjXEOpuIh7RaTCRserFmh6t0/iutYj
	s3+/fLG+q5JsQB2d8XkOSGSIr0YWmoWLTB2jv+tb0dsnUtS+IAIgK6xo3cg5dUHokkk=
X-Gm-Gg: ASbGnctNHMXL6/EBrDEptTut2137JklpmlXpZO712AGarOhdYgHpU3kjDHC2H3lrCSN
	8aBlbwjdhNOA6/joSouA/uyED+MKfwAK7v16Ud5el9pZbjrs6PPxJtT7CdoYwvEAzjUbxZUghLS
	yD2Xe7ZvtV+KoyMRdPcpmOt8ieuYJNRBvQ2HLTLU+Lr49DfEMy2sVJN6y95k/94zTp/UEMdWb3J
	81WW4R+5OHYisGYzx8p98IkIlDeTH0FACXOtTSyxX5fKPBmkd7eH+6HqYxJjBefmgCLHB00i6lh
	vS49CeyDxuxfFfOILroo5CK6hf43ctQZ/IprGEulISiSwZ+GYvzQakms2hLvqjS1SO2PZaCMls5
	RrzpqvjcdqK36hL1Jea8uMptqsAY=
X-Google-Smtp-Source: AGHT+IG7OpLH6k3HwvLOeBkdHsqBfZQ/AFfCGLhvEb/bBCKCAM3IYfHng/H18tROilNBSKqh4ga5LA==
X-Received: by 2002:a05:600c:4715:b0:45b:47e1:f5fb with SMTP id 5b1f17b1804b1-45b517d8ffamr156139455e9.36.1756289379317;
        Wed, 27 Aug 2025 03:09:39 -0700 (PDT)
Received: from localhost ([196.207.164.177])
        by smtp.gmail.com with UTF8SMTPSA id ffacd0b85a97d-3cbd534656asm5268714f8f.66.2025.08.27.03.09.38
        (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);
        Wed, 27 Aug 2025 03:09:38 -0700 (PDT)
Date: Wed, 27 Aug 2025 13:09:35 +0300
From: Dan Carpenter <dan.carpenter@linaro.org>
To: Osama Abdelkader <osama.abdelkader@gmail.com>
Cc: gregkh@linuxfoundation.org, dpenkler@gmail.com,
	matchstick@neverthere.org, arnd@arndb.de,
	linux-kernel@vger.kernel.org, linux-staging@lists.linux.dev
Subject: Re: [PATCH] staging: gpib: simplify and fix get_data_lines
Message-ID: <aK7ZX3rlcS6ObWvE@stanley.mountain>
References: <20250826220502.210485-1-osama.abdelkader@gmail.com>
 <aK6wlcLBN1HclMpl@stanley.mountain>
 <aK68qXqStIwBrejF@stanley.mountain>
 <f3ab3c2d-2056-4802-aa73-2b0db4c7fc30@gmail.com>
Precedence: bulk
X-Mailing-List: linux-staging@lists.linux.dev
List-Id: <linux-staging.lists.linux.dev>
List-Subscribe: <mailto:linux-staging+subscribe@lists.linux.dev>
List-Unsubscribe: <mailto:linux-staging+unsubscribe@lists.linux.dev>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <f3ab3c2d-2056-4802-aa73-2b0db4c7fc30@gmail.com>

On Wed, Aug 27, 2025 at 11:28:36AM +0200, Osama Abdelkader wrote:
> We can change the return type to int and propagate the error, so:
> 
> static int get_data_lines(u8 *out)
> 
> {
> 
> 	int val, i;
> 
> 	u8 ret = 0;
> 
> 	struct gpio_desc *lines[8] = { D01, D02, D03, D04, D05, D06, D07, D08 };
> 	for (i = 0; i < 8; i++) { 		val = gpiod_get_value(lines[i]);
> 
> 		if (val < 0)
> 
> 			return val; // propagate error
> 
> 		ret |= (val & 1) << i;
> 
> 	}
> 
> 	*out = ~ret; 	return 0;
> 
> }
> 
> Then in the caller:
> 
> u8 data;
> if (!get_data_lines(&data))
> 
> 	priv->rbuf[priv->count++] = data;
> 
> or we print the error here, What do you think?
> 

Printing the error closer to where the error occured is better.

How would we handle the error correctly?  Sometimes it's easy
because it's if we continue then we will crash and almost anything is
better than crashing.  But here if we return a 1 or 0, what's the
worst that can happen?  We can't know without testing.  Adding new
error checking often breaks stuff.  I've done it before where you
have code like:

	ret = frob();
	if (ret)
		return ret;

	frob();  // <-- here
	if (ret)
		return ret;

And obviously the original author left off the "ret = " part on the
second call.  So I don't feel bad about that I added that, but in
practice the second frob() always fails and I can't test it so now
people's driver doesn't load.

So adding error handling is a bit risky unless you have a way to test
this code.  Just printing an error and continuing as best we can is
safer.  People will let us know if the error ever happens.

Let's not over complicate it for an error which will likely never
happen.  We can just print an error and leave the bit as zero.

regards,
dan carpenter


