Re: [PATCH V2] i2c-algo-pca: fix coding style issues in i2c-algo-pca.c

[Farid Hammane]

Hi Jean,

On Friday 23 April 2010 11:33:26 you wrote:
> Hi Farid, Wolfram,
> 
> On Fri, 23 Apr 2010 00:01:55 +0200, Farid Hammane wrote:
> > This patch fixes coding style issues found by checkpatch.pl.
> > i2c-algo-pca.c has been built successfully after applying this patch.
> >
> > Signed-off-by: Farid Hammane <farid.hammane@xxxxxxxxx>
> > ---
> >  drivers/i2c/algos/i2c-algo-pca.c |   36
> > ++++++++++++++++++------------------ 1 files changed, 18 insertions(+),
> > 18 deletions(-)
> >
> > diff --git a/drivers/i2c/algos/i2c-algo-pca.c
> > b/drivers/i2c/algos/i2c-algo-pca.c index dcdaf8e..ca817f8 100644
> > --- a/drivers/i2c/algos/i2c-algo-pca.c
> > +++ b/drivers/i2c/algos/i2c-algo-pca.c
> > @@ -37,15 +37,15 @@
> >
> >  static int i2c_debug;
> >
> > -#define pca_outw(adap, reg, val) adap->write_byte(adap->data, reg, val)
> > -#define pca_inw(adap, reg) adap->read_byte(adap->data, reg)
> > +#define pca_outw(adap, reg, val) (adap->write_byte(adap->data, reg,
> > val)) +#define pca_inw(adap, reg) (adap->read_byte(adap->data, reg))
> 
> I'm confused by these changes. For one thing, macros which are
> shortcuts for function calls normally don't need surrounding
> parentheses. If checkpatch.pl complains about that, I would call it a
> false positive, unless someone can prove me wrong with a real-world
> case where these surrounding parentheses help.
> 
> For another, macro _parameters_ normally need parentheses for each
> non-trivial use. I would thus expect the following to be correct:
> 
> #define pca_outw(adap, reg, val) (adap)->write_byte((adap)->data, reg, val)
> #define pca_inw(adap, reg) (adap)->read_byte((adap)->data, reg)
> 
> Or is it just me?

You are right ! This will be deleted from the patch.

> 
> >  #define pca_status(adap) pca_inw(adap, I2C_PCA_STA)
> > -#define pca_clock(adap) adap->i2c_clock
> > +#define pca_clock(adap) (adap->i2c_clock)
> >  #define pca_set_con(adap, val) pca_outw(adap, I2C_PCA_CON, val)
> >  #define pca_get_con(adap) pca_inw(adap, I2C_PCA_CON)
> > -#define pca_wait(adap) adap->wait_for_completion(adap->data)
> > -#define pca_reset(adap) adap->reset_chip(adap->data)
> > +#define pca_wait(adap) (adap->wait_for_completion(adap->data))
> > +#define pca_reset(adap) (adap->reset_chip(adap->data))
> 
> Same here...
> 
> I'm fine with all other changes. But checkpatch.pl spouts 2 more errors
> to me, which we've discussed before. I'm curious why you didn't fix
> them? Just replace each block of 8 spaces with one tab.

I thought you expected just one tab and spaces. You said :
"Better use tab + spaces and align on the opening parenthesis. What
checkpatch.pl complains about here isn't the alignment, it's the use of
more than 8 consecutive spaces."
I understood here : "don't care about this warning !"

So, I'll fix it.

> 
> ERROR: code indent should use tabs where possible
> #181: FILE: i2c/algos/i2c-algo-pca.c:181:
> +                    struct i2c_msg *msgs,$
> 
> ERROR: code indent should use tabs where possible
> #182: FILE: i2c/algos/i2c-algo-pca.c:182:
> +                    int num)$
> 

Thanks for you comments and for sharing your knowledge !

A patch V3 will be sent,

Regards,
Farid,
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/