Date:   Fri, 7 Jan 2022 11:53:44 +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: <20220107085343.GP7674@kadam>
References: <20220106201430.GA3416@mail.google.com>
 <20220106213325.GA10767@mail.google.com>
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20220106213325.GA10767@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?vcbyyxjtZSqY17KFnrfEdc5rJkCLb3UuiDpHp+Tc82A75Li7BAsScSlVxm3Q?=
 =?us-ascii?Q?rs7/BzwVatlofQlWS1pZxUQZwWNNPwWROf4HZrLlyzfacjNiGc+XsxNxfxKO?=
 =?us-ascii?Q?Pm0bVWMdyjvjS4XcbQGyXnRA9EBCyTz3dyNOwdCBCeRq+6QdBMX65e8VlP57?=
 =?us-ascii?Q?F8fTqdou6vRe5GHo1TdArAxEjW/nA0PbXLdEy/upMTx6ZbUOZumfzP8T0Bfa?=
 =?us-ascii?Q?0T9GCJLGQe+py/fezZ8CrUrFTNaigOG/jOwfMRrSmKt+03KIPbUAcKYmq0Dc?=
 =?us-ascii?Q?o4xioncx1TNQucDQy+x6+S7t93HRRlAV+cxPyFHB2ObJtZFY49h5Fu/kveQZ?=
 =?us-ascii?Q?drHQK3Fmhu4huYM5SFM2A2TiCSqk+6SfU4JOJbTZbBySJ7q15J4Zx/AM1Ow8?=
 =?us-ascii?Q?bvpNEjW8h1odNcRRfxhUi7rH/duOSqVF0dO5zMjsD8WSfAcZ7gk9Ck43zdoB?=
 =?us-ascii?Q?dmlhy0UkgmQ4WzXfWx/jOJ9OjpWL/CQh9ecso8TC+rTCzzy9233aUs2R3iXX?=
 =?us-ascii?Q?bjmsJD8ZkwFhV2tAFlwV+MqPoVzc4E+OSVD4ClptSpezf6IMcPyNyNNh//Q+?=
 =?us-ascii?Q?543XK+Dx1MZ+YCIp4a9EQNFNfmgOjBwk8Pmfs5HqCSOTPq3wr4OUcbdhbHkr?=
 =?us-ascii?Q?EVpb0lL//NlK1XHKFtLBDJLBAr570lo1NmU/CkLRerr9JuBM5PFZ4Xi7hv7o?=
 =?us-ascii?Q?f6nEILK9t2KROKbe8NbTCLcfynBUdb3Z4qy6RW69wIa+4L7ahunFOBTIENgZ?=
 =?us-ascii?Q?ZeNQ9pwYUS93taYgmwTXESx1KhYC85Q0N4/fW8kDL+S7wIiYYrRyatMgZTGY?=
 =?us-ascii?Q?j5b+BrMbtpfgxLC+BEeulY1alhl4obflIglw7V3I65XvnRZEDDFCYGCfQY+O?=
 =?us-ascii?Q?fanikSxqyu8VyT1hyX41bUs+vfyciD+ya8gjn8TwEx52RURPvqlfJoUAOl6J?=
 =?us-ascii?Q?tZ1Rsp4hX/0I5cyg9JVRrbLRO3clVj0aa+7OaoRllXjiQkQqxywiOhdNa/m8?=
 =?us-ascii?Q?qydMKAMrH3T6V907eMqBQz70lOQxHcRQAN/2sLqW3qKlJaL/f92vQeDmRq1y?=
 =?us-ascii?Q?962pwGSN1EsTPknztYpTmRqCiWHNK+UFJwEfLZC+siqJ1Pk01AcNh5z8QcgN?=
 =?us-ascii?Q?SgXVmSHzzMdofktuXEZUm+X0MdmjFDb7B8vCnxxx1m1IXI7zf+UIFTSgI2o9?=
 =?us-ascii?Q?Whbb8ldSQvSGSsLTR+VToibZzPM+aW2X7jc+Bhqxm9rB+1EBP6T+exnYEExx?=
 =?us-ascii?Q?gdP5It+P8CL8/VOgHGwv43m6jHlc+CUKR0UgIui8lLUp1zW5GzJC82gl4WiY?=
 =?us-ascii?Q?aLw51cshTEbUDxfamYtv7JER1J3TfNZdE3o/KDmYtMQEngKPeCB8x5GT6eQ9?=
 =?us-ascii?Q?pUejvl2cAzigQ4FnrnQpj3VrMODl9aN37Vxx4MFVkrsCmrI8C18n1zrQhccG?=
 =?us-ascii?Q?PPjJ0GcfjoqJWpaXy6b8A6jthpGrx6Qv/wIx6hVIuPMt1804hlnDB12/U18L?=
 =?us-ascii?Q?M+PZl+z00e8zxnfupy98b6q6UeSwqp3GTWb8O1wzrQAG9CMPKbaX/QNr4FIZ?=
 =?us-ascii?Q?skotp3gg8/fEVNzS7wKUKcinigVuJPtEJfmceJsTsmJfA7mF20A7Uho5IiJz?=
 =?us-ascii?Q?xHTVqrLIAoth2vjRLmtESj+yFbYOfbIGB9UiESHRfkPxzZq2LxA6C2ZJQ7dq?=
 =?us-ascii?Q?lzyq843V3syUBSpVzqIceIA2mzw=3D?=
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 3b58c80f-740f-4b4c-ab2f-08d9d1bb417c
X-MS-Exchange-CrossTenant-AuthSource: MWHPR1001MB2365.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 07 Jan 2022 08:54:03.9289
 (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: gwn288VmWTtkMt8ThseSrhP7mVir1RJi3fNzrBDMWswV+553e4ic81Oj01u67gICVg2vpUSwYuwhYvcL6lhiMD8qS5YVG5B5VtkGO/hCtZ8=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: MWHPR1001MB2254
X-Proofpoint-Virus-Version: vendor=nai engine=6300 definitions=10219 signatures=668683
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 phishscore=0 bulkscore=0 suspectscore=0
 mlxlogscore=999 spamscore=0 mlxscore=0 malwarescore=0 adultscore=0
 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2110150000
 definitions=main-2201070063
X-Proofpoint-ORIG-GUID: 1QfxEGi4VCkpT0YVaggBa7Bx-H0VsdxY
X-Proofpoint-GUID: 1QfxEGi4VCkpT0YVaggBa7Bx-H0VsdxY
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On Fri, Jan 07, 2022 at 10:33:25AM +1300, Paulo Miguel Almeida wrote:
> As a convention for the pi433 driver, all routines that deals with the
> rf69 chip are defined in the rf69.c file.

That's some EnterpriseQuality[tm] style guidelines.  It's an over fussy
rule that just makes the code harder to read for no reason.

> While at it, the Version Register hardcoded value was replaced with a
> pre-existing constant in the driver.

This is good, though.

> diff --git a/drivers/staging/pi433/pi433_if.c b/drivers/staging/pi433/pi433_if.c
> index 68c09fa016ed..1372361d56e1 100644
> --- a/drivers/staging/pi433/pi433_if.c
> +++ b/drivers/staging/pi433/pi433_if.c
> @@ -1116,9 +1116,7 @@ static int pi433_probe(struct spi_device *spi)
>  		spi->mode, spi->bits_per_word, spi->max_speed_hz);
>  
>  	/* Ping the chip by reading the version register */

This comment doesn't make sense now.

> -	retval = spi_w8r8(spi, 0x10);
> -	if (retval < 0)
> -		return retval;
> +	retval = rf69_get_version(spi);

Just say:

	retval = rf69_read_reg(spi, REG_VERSION);
	if (retval < 0)
		return retval;

Deleting the error handling was a bad style choice.  Also preserve the
error code.

regards,
dan carpenter