Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1702046 > unrolled thread

Re: [PATCH] staging: pi433: remove some macros, introduce some inline functions

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH] staging: pi433: remove some macros, introduce some  inline functions Dan Carpenter <dan.carpenter@oracle.com> - 2017-08-02 14:00 +0200
    Re: [PATCH] staging: pi433: remove some macros, introduce some  inline functions Marcus Wolf <marcus.wolf@wolf-entwicklungen.de> - 2017-08-02 14:10 +0200
      Re: [PATCH] staging: pi433: remove some macros, introduce some  inline functions Dan Carpenter <dan.carpenter@oracle.com> - 2017-08-02 14:20 +0200

#1702046 — Re: [PATCH] staging: pi433: remove some macros, introduce some inline functions

I'm afraid this patch is going to need to be divided into several
patches that do one thing per patch.  And I totally get that redoing
patches sucks...  Sorry.

On Wed, Aug 02, 2017 at 01:10:14PM +0200, Wolf Entwicklungen wrote:
> According to the proposal of Walter Harms, I removed some macros
> and added some inline functions.
> 
> Since I used a bit more intelligent interface, this enhances
> readability and reduces problems with checkpatch.pl at the same time.
> 
> In addition obsolete debug ifdefs were removed.
> 
> Signed-off-by: Marcus Wolf <linux@wolf-entwicklungen.de>
> ---
>  drivers/staging/pi433/rf69.c |  544 ++++++++++++++++++------------------------
>  1 file changed, 238 insertions(+), 306 deletions(-)
> 
> diff --git a/drivers/staging/pi433/rf69.c b/drivers/staging/pi433/rf69.c
> --- a/drivers/staging/pi433/rf69.c
> +++ b/drivers/staging/pi433/rf69.c
> @@ -28,33 +28,57 @@
>  #include "rf69.h"
>  #include "rf69_registers.h"
> 
> -#define F_OSC	  32000000 /* in Hz */
> -#define FIFO_SIZE 66 	   /* in byte */
> +#define F_OSC 32000000 /* in Hz */
> +#define FIFO_SIZE 66   /* in byte */


These are unrelated white space changes so they'll need to go in a
separate patch.

> 
>  /*-------------------------------------------------------------------------*/
> 
> -#define READ_REG(x)	rf69_read_reg (spi, x)
> -#define WRITE_REG(x,y)	rf69_write_reg(spi, x, y)
>  #define INVALID_PARAM \
>  	{ \
>  		dev_dbg(&spi->dev, "set: illegal input param"); \
>  		return -EINVAL; \
>  	}
> 
> +inline static int setBit(struct spi_device *spi, u8 reg, u8 mask)

Don't put "inline" on functions, let the compiler decide.  setBit is
camel case and also the name is probably too generic.  Add a prefix so
it's rf69_set_bits().


> +{
> +	u8 tempVal;

Camel case.

> +
> +	tempVal = rf69_read_reg (spi, reg);

No space before the '(' char.

> +	tempVal = tempVal | mask;
> +	return rf69_write_reg(spi, reg, tempVal);
> +}
> +
> +inline static int rstBit(struct spi_device *spi, u8 reg, u8 mask)

Maybe clear_bits is better than reset_bit.

> +{
> +	u8 tempVal;
> +
> +	tempVal = rf69_read_reg (spi, reg);
> +	tempVal = tempVal & ~mask;
> +	return rf69_write_reg(spi, reg, tempVal);
> +}
> +
> +inline static int rmw(struct spi_device *spi, u8 reg, u8 mask, u8 value)

What does rmw mean?  Maybe just full words here or add a comment.  I
guess read_modify_write is too long...


> +{
> +	u8 tempVal;
> +
> +	tempVal = rf69_read_reg (spi, reg);
> +	tempVal = (tempVal & ~mask) | value;
> +	return rf69_write_reg(spi, reg, tempVal);
> +}
> +
>  /*-------------------------------------------------------------------------*/
> 
>  int rf69_set_mode(struct spi_device *spi, enum mode mode)
>  {
> -	#ifdef DEBUG
> -		dev_dbg(&spi->dev, "set: mode");
> -	#endif
> 
> +	dev_dbg(&spi->dev, "set: mode");

This change is unrelated.  Also just delete the line, because we can
get the same information use ftrace instead.

Anyway, I glanced through the rest of the patch and I think probably
it should be broken up like this:

[patch 1] add rf69_rmw() and convert callers
[patch 2] add rf69_set_bit and rf69_clear_bit() and convert callers
[patch 3] convert remaining callers to use rf69_read_reg() directly
[patch 4] get rid of #ifdef debug, deleting code as appropriate
[patch 5] other white space changes

Greg is going to apply patches as they arrive on the email list, first
come, first served.  The problem is that some patches might have
problems so you kind of have to guess which are good patches that he
will apply and which are bad an then write your patch on top of that.

Normally, I just write my patch based on linux-next and hope for the
best.  If I have to redo it because of conflicts, that's just part of
the process.  But my patches are normally small so they don't often
conflict.

regards,
dan carpenter

[toc] | [next] | [standalone]

#1702059

Hi Dan,

I get your point and I understand, that there need to be rules to
simplify the life for the maintainers...

But I honestly must confess, that at the moment, I don't have the
time for doing that. I am into two customer projects, that keep me
very busy these weeks.

The only thing, I can offer, is to remove all the lines, dealing 
with the #ifdef DEBUG and leave the rest, as it is and send it again.
Then all other changes are related to the move from macro to inline...
If that helps, please let me know.

If it needs further fragmentation, it'll take something like half 
a day / a day. I most probably can spent that day end of August, 
begin of September the earliest.

Sorry,

Marcus

> Dan Carpenter <dan.carpenter@oracle.com> hat am 2. August 2017 um 13:53
> geschrieben:
>
>
> I'm afraid this patch is going to need to be divided into several
> patches that do one thing per patch. And I totally get that redoing
> patches sucks... Sorry.
>
> On Wed, Aug 02, 2017 at 01:10:14PM +0200, Wolf Entwicklungen wrote:
> > According to the proposal of Walter Harms, I removed some macros
> > and added some inline functions.
> >
> > Since I used a bit more intelligent interface, this enhances
> > readability and reduces problems with checkpatch.pl at the same time.
> >
> > In addition obsolete debug ifdefs were removed.
> >
> > Signed-off-by: Marcus Wolf <linux@wolf-entwicklungen.de>
> > ---
> > drivers/staging/pi433/rf69.c | 544
> > ++++++++++++++++++------------------------
> > 1 file changed, 238 insertions(+), 306 deletions(-)
> >
> > diff --git a/drivers/staging/pi433/rf69.c b/drivers/staging/pi433/rf69.c
> > --- a/drivers/staging/pi433/rf69.c
> > +++ b/drivers/staging/pi433/rf69.c
> > @@ -28,33 +28,57 @@
> > #include "rf69.h"
> > #include "rf69_registers.h"
> >
> > -#define F_OSC 32000000 /* in Hz */
> > -#define FIFO_SIZE 66 /* in byte */
> > +#define F_OSC 32000000 /* in Hz */
> > +#define FIFO_SIZE 66 /* in byte */
>
>
> These are unrelated white space changes so they'll need to go in a
> separate patch.
>
> >
> > /*-------------------------------------------------------------------------*/
> >
> > -#define READ_REG(x) rf69_read_reg (spi, x)
> > -#define WRITE_REG(x,y) rf69_write_reg(spi, x, y)
> > #define INVALID_PARAM \
> > { \
> > dev_dbg(&spi->dev, "set: illegal input param"); \
> > return -EINVAL; \
> > }
> >
> > +inline static int setBit(struct spi_device *spi, u8 reg, u8 mask)
>
> Don't put "inline" on functions, let the compiler decide. setBit is
> camel case and also the name is probably too generic. Add a prefix so
> it's rf69_set_bits().
>
>
> > +{
> > + u8 tempVal;
>
> Camel case.
>
> > +
> > + tempVal = rf69_read_reg (spi, reg);
>
> No space before the '(' char.
>
> > + tempVal = tempVal | mask;
> > + return rf69_write_reg(spi, reg, tempVal);
> > +}
> > +
> > +inline static int rstBit(struct spi_device *spi, u8 reg, u8 mask)
>
> Maybe clear_bits is better than reset_bit.
>
> > +{
> > + u8 tempVal;
> > +
> > + tempVal = rf69_read_reg (spi, reg);
> > + tempVal = tempVal & ~mask;
> > + return rf69_write_reg(spi, reg, tempVal);
> > +}
> > +
> > +inline static int rmw(struct spi_device *spi, u8 reg, u8 mask, u8 value)
>
> What does rmw mean? Maybe just full words here or add a comment. I
> guess read_modify_write is too long...
>
>
> > +{
> > + u8 tempVal;
> > +
> > + tempVal = rf69_read_reg (spi, reg);
> > + tempVal = (tempVal & ~mask) | value;
> > + return rf69_write_reg(spi, reg, tempVal);
> > +}
> > +
> > /*-------------------------------------------------------------------------*/
> >
> > int rf69_set_mode(struct spi_device *spi, enum mode mode)
> > {
> > - #ifdef DEBUG
> > - dev_dbg(&spi->dev, "set: mode");
> > - #endif
> >
> > + dev_dbg(&spi->dev, "set: mode");
>
> This change is unrelated. Also just delete the line, because we can
> get the same information use ftrace instead.
>
> Anyway, I glanced through the rest of the patch and I think probably
> it should be broken up like this:
>
> [patch 1] add rf69_rmw() and convert callers
> [patch 2] add rf69_set_bit and rf69_clear_bit() and convert callers
> [patch 3] convert remaining callers to use rf69_read_reg() directly
> [patch 4] get rid of #ifdef debug, deleting code as appropriate
> [patch 5] other white space changes
>
> Greg is going to apply patches as they arrive on the email list, first
> come, first served. The problem is that some patches might have
> problems so you kind of have to guess which are good patches that he
> will apply and which are bad an then write your patch on top of that.
>
> Normally, I just write my patch based on linux-next and hope for the
> best. If I have to redo it because of conflicts, that's just part of
> the process. But my patches are normally small so they don't often
> conflict.
>
> regards,
> dan carpenter
>
>

[toc] | [prev] | [next] | [standalone]

#1702067

On Wed, Aug 02, 2017 at 02:07:37PM +0200, Marcus Wolf wrote:
> Hi Dan,
> 
> I get your point and I understand, that there need to be rules to
> simplify the life for the maintainers...
> 
> But I honestly must confess, that at the moment, I don't have the
> time for doing that. I am into two customer projects, that keep me
> very busy these weeks.
> 
> The only thing, I can offer, is to remove all the lines, dealing 
> with the #ifdef DEBUG and leave the rest, as it is and send it again.
> Then all other changes are related to the move from macro to inline...
> If that helps, please let me know.
> 
> If it needs further fragmentation, it'll take something like half 
> a day / a day. I most probably can spent that day end of August, 
> begin of September the earliest.
> 

One trick that might help is you can just use `git citool` and highlight
all the rmw() lines, add selected lines to the commit, then commit that
as one patch.  Repeat for all five patches.

The kernel process does kind of have a steep learning with redoing
patches...  Sorry about that.

regards,
dan carpenter

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web