[PATCH] Clean up magic numbers in i2c_parport.h

[PATCH] Clean up magic numbers in i2c_parport.h

[Christopher Hoover]

This small patch gets rid of some magic numbers in the i2c parport
drivers, specifically wrt the control and status handling, using the
symbols already defined in parport.h

The patch produces the same binary objects for i2c-parport.c and
i2c-parport-simple.c as before.

Please apply.

Cheers,
Christopher.

ch@murgatroid.com
ch@hpl.hp.com



Only in linux-2.6.16: .config
diff --exclude=scripts --exclude='*~' -aurp linux-2.6.16/drivers/i2c/busses/i2c-parport.h linux-2.6.16.i2c/drivers/i2c/busses/i2c-parport.h
--- linux-2.6.16/drivers/i2c/busses/i2c-parport.h	2006-03-19 21:53:29.000000000 -0800
+++ linux-2.6.16.i2c/drivers/i2c/busses/i2c-parport.h	2006-03-22 23:51:08.000000000 -0800
@@ -18,6 +18,8 @@
    Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA.
  * ------------------------------------------------------------------------ */
 
+#include <linux/parport.h>
+
 #ifdef DATA
 #undef DATA
 #endif
@@ -40,53 +42,62 @@ struct adapter_parm {
 	struct lineop init;
 };
 
+#define LINEOP_DATA(val_, inverted_) \
+   { .val=(val_), .port = DATA, .inverted=(inverted_) }
+
+#define LINEOP_STATUS(val_, inverted_) \
+   { .val=(val_), .port = STAT, .inverted=(inverted_) }
+
+#define LINEOP_CONTROL(val_, inverted_) \
+   { .val=(val_), .port = CTRL, .inverted=(inverted_) }
+
 static struct adapter_parm adapter_parm[] = {
 	/* type 0: Philips adapter */
 	{
-		.setsda	= { 0x80, DATA, 1 },
-		.setscl	= { 0x08, CTRL, 0 },
-		.getsda	= { 0x80, STAT, 0 },
-		.getscl	= { 0x08, STAT, 0 },
+		.setsda	= LINEOP_DATA(0x80, 1),
+		.setscl	= LINEOP_CONTROL(PARPORT_CONTROL_SELECT, 0),
+		.getsda	= LINEOP_STATUS(PARPORT_STATUS_BUSY, 0),
+		.getscl	= LINEOP_STATUS(PARPORT_STATUS_ERROR, 0)
 	},
 	/* type 1: home brew teletext adapter */
 	{
-		.setsda	= { 0x02, DATA, 0 },
-		.setscl	= { 0x01, DATA, 0 },
-		.getsda	= { 0x80, STAT, 1 },
+		.setsda	= LINEOP_DATA(0x02, 0),
+		.setscl	= LINEOP_DATA(0x01, 0),
+		.getsda	= LINEOP_STATUS(PARPORT_STATUS_BUSY, 1)
 	},
 	/* type 2: Velleman K8000 adapter */
 	{
-		.setsda	= { 0x02, CTRL, 1 },
-		.setscl	= { 0x08, CTRL, 1 },
-		.getsda	= { 0x10, STAT, 0 },
+		.setsda	= LINEOP_CONTROL(PARPORT_CONTROL_AUTOFD, 1),
+		.setscl	= LINEOP_CONTROL(PARPORT_CONTROL_SELECT, 1),
+		.getsda	= LINEOP_STATUS(PARPORT_STATUS_SELECT, 0)
 	},
 	/* type 3: ELV adapter */
 	{
-		.setsda	= { 0x02, DATA, 1 },
-		.setscl	= { 0x01, DATA, 1 },
-		.getsda	= { 0x40, STAT, 1 },
-		.getscl	= { 0x08, STAT, 1 },
+		.setsda	= LINEOP_DATA(0x02, 1),
+		.setscl	= LINEOP_DATA(0x01, 1),
+		.getsda	= LINEOP_STATUS(PARPORT_STATUS_ACK, 1),
+		.getscl	= LINEOP_STATUS(PARPORT_STATUS_ERROR, 1)
 	},
 	/* type 4: ADM1032 evaluation board */
 	{
-		.setsda	= { 0x02, DATA, 1 },
-		.setscl	= { 0x01, DATA, 1 },
-		.getsda	= { 0x10, STAT, 1 },
-		.init	= { 0xf0, DATA, 0 },
+		.setsda	= LINEOP_DATA(0x02, 1),
+		.setscl	= LINEOP_DATA(0x01, 1),
+		.getsda	= LINEOP_STATUS(PARPORT_STATUS_SELECT, 1),
+		.init	= LINEOP_DATA(0xf0,0)
 	},
 	/* type 5: ADM1025, ADM1030 and ADM1031 evaluation boards */
 	{
-		.setsda	= { 0x02, DATA, 1 },
-		.setscl	= { 0x01, DATA, 1 },
-		.getsda	= { 0x10, STAT, 1 },
+		.setsda	= LINEOP_DATA(0x02, 1),
+		.setscl	= LINEOP_DATA(0x01, 1),
+		.getsda	= LINEOP_STATUS(PARPORT_STATUS_SELECT, 1)
 	},
 	/* type 6: Barco LPT->DVI (K5800236) adapter */
 	{
-		.setsda	= { 0x02, DATA, 1 },
-		.setscl	= { 0x01, DATA, 1 },
-		.getsda	= { 0x20, STAT, 0 },
-		.getscl	= { 0x40, STAT, 0 },
-		.init	= { 0xfc, DATA, 0 },
+		.setsda	= LINEOP_DATA(0x02, 1),
+		.setscl	= LINEOP_DATA(0x01, 1),
+		.getsda	= LINEOP_STATUS(PARPORT_STATUS_PAPEROUT, 0),
+		.getscl	= LINEOP_STATUS(PARPORT_STATUS_ACK, 0),
+		.init	= LINEOP_DATA(0xfc, 0)
 	},
 };
 

Re: [PATCH] Clean up magic numbers in i2c_parport.h

[Jean Delvare]

Hi Christopher,

> This small patch gets rid of some magic numbers in the i2c parport
> drivers, specifically wrt the control and status handling, using the
> symbols already defined in parport.h
> 
> The patch produces the same binary objects for i2c-parport.c and
> i2c-parport-simple.c as before.

> --- linux-2.6.16/drivers/i2c/busses/i2c-parport.h	2006-03-19 21:53:29.000000000 -0800
> +++ linux-2.6.16.i2c/drivers/i2c/busses/i2c-parport.h	2006-03-22 23:51:08.000000000 -0800
> (...)
> +#define LINEOP_DATA(val_, inverted_) \
> +   { .val=(val_), .port = DATA, .inverted=(inverted_) }
> +
> +#define LINEOP_STATUS(val_, inverted_) \
> +   { .val=(val_), .port = STAT, .inverted=(inverted_) }
> +
> +#define LINEOP_CONTROL(val_, inverted_) \
> +   { .val=(val_), .port = CTRL, .inverted=(inverted_) }
> +

Beeuh. These macros don't really help. They actually make the lines
longer! I'm not taking this change, sorry.

Other than that, I am fine with your patch. Not that I really see the
benefit, but it others do, why not. Can you please respin the patch
without the additional macros?

Thanks,
-- 
Jean Delvare

RE: [PATCH] Clean up magic numbers in i2c_parport.h

[Christopher Hoover]

> Beeuh. These macros don't really help. They actually make the lines
longer! I'm not taking this change, sorry.

If I kill off the macros but continue to use C99 structure initializers,
which is I believe is the proper kernel coding style today, the lines are
going to get even longer.  Is that OK?

Or are you asking for the patch w/o macros and w/o C99 structure
initializers?

I can/will do either.  Just let me know what is acceptable a priori.

-ch

Re: [PATCH] Clean up magic numbers in i2c_parport.h

[Jean Delvare]

Hi Christopher,

> > Beeuh. These macros don't really help. They actually make the lines
> > longer! I'm not taking this change, sorry.
> 
> If I kill off the macros but continue to use C99 structure initializers,
> which is I believe is the proper kernel coding style today, the lines are
> going to get even longer.  Is that OK?
> 
> Or are you asking for the patch w/o macros and w/o C99 structure
> initializers?
> 
> I can/will do either.  Just let me know what is acceptable a priori.

I don't think C99 initializers are needed here, the structure is pretty
simple and is also defined in the same file, a few lines above all its
instance declarations. So I am indeed asking for a patch w/o macros and
w/o C99 structure initializers, unless someone objects.

Thanks,
-- 
Jean Delvare

Re: [KJ] Re: [PATCH] Clean up magic numbers in i2c_parport.h

[Greg KH]

On Fri, Mar 24, 2006 at 08:26:00AM +0100, Jean Delvare wrote:
> Hi Christopher,
> 
> > > Beeuh. These macros don't really help. They actually make the lines
> > > longer! I'm not taking this change, sorry.
> > 
> > If I kill off the macros but continue to use C99 structure initializers,
> > which is I believe is the proper kernel coding style today, the lines are
> > going to get even longer.  Is that OK?
> > 
> > Or are you asking for the patch w/o macros and w/o C99 structure
> > initializers?
> > 
> > I can/will do either.  Just let me know what is acceptable a priori.
> 
> I don't think C99 initializers are needed here, the structure is pretty
> simple and is also defined in the same file, a few lines above all its
> instance declarations. So I am indeed asking for a patch w/o macros and
> w/o C99 structure initializers, unless someone objects.

You should use structure initializers whereever possible, as it makes
future changes much easier and safer (reorder the fields and things
don't break in odd ways.)  So I would encourage this kind of change.

thanks,

greg k-h

Re: [PATCH] Clean up magic numbers in i2c_parport.h

[Jean Delvare]

Hi Greg, Christpher,

> > I don't think C99 initializers are needed here, the structure is pretty
> > simple and is also defined in the same file, a few lines above all its
> > instance declarations. So I am indeed asking for a patch w/o macros and
> > w/o C99 structure initializers, unless someone objects.
> 
> You should use structure initializers whereever possible, as it makes
> future changes much easier and safer (reorder the fields and things
> don't break in odd ways.)  So I would encourage this kind of change.

Oh well, if Greg says so...

Christopher, can you please respin a patch with C99 initializers, which
would look a bit better than your original one? I'd suggest a single,
straightforward macro (no needless underscores please):

#define LINEOP(val, port, inverted) \
    { .val = (val), .port = (port), .inverted = (inverted) }

Hopefully this will keep all lines within a reasonable length and won't
hurt the readability too much.

Also, I just noticed in your original patch: please preserve the comma
at the end of the last line of struct declarations. It's not needed,
sure, but it makes later changes easier.

Thanks,
-- 
Jean Delvare