Date:   Sat, 8 Jan 2022 14:19:10 +0300
From:   Dan Carpenter <dan.carpenter@oracle.com>
To:     Paulo Miguel Almeida <paulo.miguel.almeida.rodenas@gmail.com>
Cc:     gregkh@linuxfoundation.org, realwakka@gmail.com,
        linux-staging@lists.linux.dev, linux-kernel@vger.kernel.org
Subject: Re: [PATCH v2] staging: pi433: move get version func to where all
 other functions are
Message-ID: <20220108111910.GA1978@kadam>
References: <20220106201430.GA3416@mail.google.com>
 <20220106213325.GA10767@mail.google.com>
 <20220107085343.GP7674@kadam>
 <20220107192438.GB2693@mail.google.com>
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20220107192438.GB2693@mail.google.com>
User-Agent: Mutt/1.9.4 (2018-02-28)
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?T8+6hnIBz5MhrK10TAVanOcd90zvDmJ9oKkxbGm8kRA1BhGE7itYEBrGCeNX?=
 =?us-ascii?Q?37dbFsLPu558INI8rjPEs4ihSnQpHUaz6Z9sPY+bpkJH0uQovmgxg1rElDTm?=
 =?us-ascii?Q?rKKvy/d3yikD2YGQZnA5kqWZF9rcU7p+z7gEezq50Uq+H6XQh0lB+yIWl+5V?=
 =?us-ascii?Q?8IELsepcobIsscgKYTNHjS8ZCihytWS+qdA75LGzPxOrucMY79ilYm8Hzkzn?=
 =?us-ascii?Q?DaeXZixdqRX95DawbTQW6+VOCuHHfx0oWqVa/Uw2A97+9loKDGQP0m8wFhyX?=
 =?us-ascii?Q?O3IY4bBeWkUzLzxBlQ4nNiIsLt6ChDfCBhg/06mxhnv8JBN/vIDc/uAfPy2U?=
 =?us-ascii?Q?7Fuc/J7XGl3XMSycvDfOXMQSpVzm7r/WITBiLWAAOlX4mPDvYOtGpoi81W/p?=
 =?us-ascii?Q?DkgfLPsryCh5e9mbx5hOmwATqOxRaEL81wVaPE7N9JjRAjlj4DMLRxwamS+F?=
 =?us-ascii?Q?rHZW4gYIem1qgDKvlpNFLp9IJrxRfC0mYtmD8BqGa1sWVe7W3VwMPdEpoyAC?=
 =?us-ascii?Q?NKToU1enXqVx+SIve0UzdLkiaX8JLe/BbOmR6ZCFuxFvIMc9mixH14HxTHB9?=
 =?us-ascii?Q?aIcR0RnWbKouxB7Iflrim8pPK/Ol1hMqFfxIrJdJrvWemHI8hGKXYizuJHsU?=
 =?us-ascii?Q?PPHB101bzkEFwNM9J7YpfcVEU6LIhbzT/M2EcPu+uf1KAHvWhieLfFqCjdyU?=
 =?us-ascii?Q?th1GN3slLjUbLUpzcTBZF1uglBsHIxBppVv0NqZg0XCU5LXC/LAWerNrWvWe?=
 =?us-ascii?Q?T2xxKZEVj7zg+AUejddoY6dc/QVH/6lTcPYY9E4UrPg10IJ7YeFhVMRD50UL?=
 =?us-ascii?Q?qgFdQRw2YYlNL/x7HWyoAcQGbJZKs+kydY4cHDfC23wGItTgTp4/SHdwdm31?=
 =?us-ascii?Q?ggxbS+mQYoPjMIx0d93PTCFwFdaBEKkWy9r4bV++d36UUYGqFtPVTl7Pyhty?=
 =?us-ascii?Q?RQN4EFdUXIAuGjppqG0neyWvcuT5p3nmF94yfqUypca5lldYArrrRc5xV2xI?=
 =?us-ascii?Q?fQcfMIIN1OaVLSJ0R9UhIh3+NcbmKz+psgH+tmUvl9N1X79fGNN3LDBgrzZv?=
 =?us-ascii?Q?1akKdX2kPXucbpmjENaweeFvID3uXcPlUKhgp4PPP5+qNm6N4ebiwMEb/zpH?=
 =?us-ascii?Q?Fq3rAqLgezS/zpI8GPwKDjdnAP+vjyQGDVS1q88k7jpFtkDXMmnZrVwVGRyh?=
 =?us-ascii?Q?Z8xUvG3Uyjj87KfIUJRWJjo7OSr16GK7XwR4O+fcp0rTA4LULux1WayfdaFn?=
 =?us-ascii?Q?XKeGDppAxtEwfZ2sIWSd7VNy1huO6Y6hmbO7dZ024zDZWolf4zgioFx8ZF3W?=
 =?us-ascii?Q?JOzMq74ZkhFoBxR3n0FVguo3Dyf1l/UgHXar0/VG3KOm78J2UPbmUPXp39Y1?=
 =?us-ascii?Q?PXFswBrIlEZkPRNnC2CxrY2u+SL8zVRz5rRU6FRIp35B6GxRrD+n5H2pyYOs?=
 =?us-ascii?Q?mtye+GdcJHD2sytIspoT3lljjfWmpbtIlYV/seNfnszB0D42D4Hz9UsiFbDw?=
 =?us-ascii?Q?TUrRdiqgzBBmyY4ILfrNIBdHpJ17XTkY2ZOrdBMelHYUS/wqiRavJlD/0J5n?=
 =?us-ascii?Q?9heUklu7AYKL5OYH2IntqZdvRsQ5tnQpAgWfhwg2O4wpXX02g98Su3tVts4e?=
 =?us-ascii?Q?Sau55KcMl2nvp95HcUew3Kg/g5YknHZvzkffeJ4QSRb5ha+uMi76AvE7k6Re?=
 =?us-ascii?Q?6Wu+fA=3D=3D?=
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 8eb54aee-f284-4eae-9237-08d9d298c00b
X-MS-Exchange-CrossTenant-AuthSource: MWHPR1001MB2365.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 08 Jan 2022 11:19:35.0356
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 4e2c6054-71cb-48f1-bd6c-3a9705aca71b
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: dj/tdyi0KKiu1B1mZ/xPoPs0FcdXwburv6junqe2XL05c/IOjjToTIXesGGtjpjTgTadigmM3OPANF9ndLtfgjETlOQwH86q8MeimYm4FjE=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: MWHPR1001MB2319
X-Proofpoint-Virus-Version: vendor=nai engine=6300 definitions=10220 signatures=668683
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 adultscore=0 mlxscore=0 phishscore=0
 mlxlogscore=999 spamscore=0 bulkscore=0 malwarescore=0 suspectscore=0
 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2110150000
 definitions=main-2201080090
X-Proofpoint-GUID: 98XzIZ-stUx4hwGRiHaTvakUH7qSvB0r
X-Proofpoint-ORIG-GUID: 98XzIZ-stUx4hwGRiHaTvakUH7qSvB0r
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On Sat, Jan 08, 2022 at 08:24:38AM +1300, Paulo Miguel Almeida wrote:
> On Fri, Jan 07, 2022 at 11:53:44AM +0300, Dan Carpenter wrote:
> > Just say:
> > 
> > 	retval = rf69_read_reg(spi, REG_VERSION);
> 
> atm rf69_read_reg is a static function in rf69.c.
> 

I would remove be the static.

> I do agree that this is technically possible to write the code
> exactly as you suggested but on the other hand, that would be the only
> exception to the rule when considering all other higher-level functions
> provided in the rf69.c

There may be other functions which will benefit from this later on.  So
instead of "only" a better word is "first".  Some of those high level
functions make sense because they are slightly complicated and have
multiple callers.  But in this case open coding it seems easier to read.

> 
> are you strongly set on the rf69_read_reg approach or would you
> be open to keep the original approach? (rf69_get_version)

I think my approach is best but I don't care.

> 
> > 	if (retval < 0)
> > 		return retval;
> > 
> > Deleting the error handling was a bad style choice.  Also preserve the
> > error code.
> >
> 
> I just want to double-check if this suggestion is taking into
> consideration what was mentioned here:
> https://lore.kernel.org/lkml/20220106210134.GB3416@mail.google.com/ 
> 
> If it is, I'm happy to add it back but I just wanted to confirm it
> first.

Yes.  Keep the error handling.  Your way makes the code more complicated
to read.

regards,
dan carpenter