From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752570AbdKHOqL (ORCPT ); Wed, 8 Nov 2017 09:46:11 -0500 Received: from mail-yw0-f194.google.com ([209.85.161.194]:43377 "EHLO mail-yw0-f194.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750818AbdKHOqJ (ORCPT ); Wed, 8 Nov 2017 09:46:09 -0500 X-Google-Smtp-Source: ABhQp+R0Zp+QRhcsfY1UxtF/hQJ0AaM+5QpcFzqqHOHdcQit3l2nCRJTABvZN8Nduse68JxpL0y0Cg== Date: Wed, 8 Nov 2017 09:46:06 -0500 From: Josh Abraham To: Dan Carpenter Cc: gregkh@linuxfoundation.org, devel@driverdev.osuosl.org, robsonde@gmail.com, dudebrobro179@gmail.com, linux-kernel@vger.kernel.org, marcin.s.ciupak@gmail.com, linux@Wolf-Entwicklungen.de, colin.king@canonical.com Subject: Re: [PATCH] staging: pi433: #define shift constants in rf69.c Message-ID: <20171108144606.GA5075@josharch> References: <20171108112506.GA1654@josharch> <20171108115230.khywgqgypxrnkryv@mwanda> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20171108115230.khywgqgypxrnkryv@mwanda> User-Agent: Mutt/1.9.1 (2017-09-22) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Wed, Nov 08, 2017 at 02:52:30PM +0300, Dan Carpenter wrote: > On Wed, Nov 08, 2017 at 06:25:06AM -0500, Joshua Abraham wrote: > > This patch completes TODO improvements in rf69.c to change shift > > constants to a define. > > > > Signed-off-by: Joshua Abraham > > --- > > drivers/staging/pi433/rf69.c | 4 ++-- > > drivers/staging/pi433/rf69_registers.h | 4 ++++ > > 2 files changed, 6 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/staging/pi433/rf69.c b/drivers/staging/pi433/rf69.c > > index e69a2153c999..cfcace195be9 100644 > > --- a/drivers/staging/pi433/rf69.c > > +++ b/drivers/staging/pi433/rf69.c > > @@ -102,7 +102,7 @@ enum modulation rf69_get_modulation(struct spi_device *spi) > > > > currentValue = READ_REG(REG_DATAMODUL); > > > > - switch (currentValue & MASK_DATAMODUL_MODULATION_TYPE >> 3) { // TODO improvement: change 3 to define > > + switch (currentValue & MASK_DATAMODUL_MODULATION_TYPE >> SHIFT_DATAMODUL_MODE) { > > You've send a few of mechanical patches without waiting for feedback and > you should probably slow down... > Understood. I am just excited about submitting patches. > The first thing to notice is that the original code is probably buggy > and needs parenthesis. > > switch ((currentValue & MASK_DATAMODUL_MODULATION_TYPE) >> 3) { > > But that still doesn't fix the problem that x18 >> 3 is never going to > equal to DATAMODUL_MODULATION_TYPE_OOK which is 0x8... So there are a > couple bugs here. > > The line is over 80 characters, so checkpatch will complain about your > patch. Please run checkpatch.pl on all your patches. Really, I hate > all the naming here... Surely we can think of a better name than > MASK_DATAMODUL_MODULATION_TYPE? Normally the "MASK" and "SHIFT" part of > the name go at the end instead of the start. > I named the define to be consistent with the extant code, but I agree that the names could be better. > > /* RegDataModul */ > > +#define SHIFT_DATAMODUL_MODE 0x03 > > + > > #define MASK_DATAMODUL_MODE 0x06 > > Why did you add a blank line? Don't use hex values for shifting, use > normal numbers. The 0x3 is indented too far. > I added the blank line to separate shifts from masks, but since the shift will only be performed on the mask I supposed it isn't needed. > Anyway, take your time and really think about patches before you send > them. Normally, I write a patch, then wait overnight, then review it > and again in the morning before I send it. > > regards, > dan carpenter > Thanks for the criticism. I will be better.