| Message ID | 0F1B54C89D5F954D8535DB252AF412FA04B118A5@chinexm1.ad.analog.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Sep 8, 2009 at 23:33, Song, Barry wrote: > +static inline int ad714x_spi_register_driver(struct spi_driver *spi_drv) > +{ > + Â Â Â return spi_register_driver(spi_drv); > +} > + > +static inline void ad714x_spi_unregister_driver(struct spi_driver *spi_drv) > +{ > + Â Â Â spi_unregister_driver(spi_drv); > +} > + > +#else > +#define ad714x_spi_register_driver(p) 0 > +#define ad714x_spi_unregister_driver(p) > Â #endif i dont think you need the ifdef protection that far up. what about using the ifdef in this one place: static inline int ad714x_spi_register_driver(struct spi_driver *spi_drv) { #if defined(CONFIG_SPI) || defined(CONFIG_SPI_MODULE) return spi_register_driver(spi_drv); #else return 0; #endif } then i imagine you can have all the other pieces always enabled. gcc will see that everything is marked static and not used, so it'll cull things. -mike -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
>-----Original Message----- >From: Mike Frysinger [mailto:vapier.adi@gmail.com] >Sent: Wednesday, September 09, 2009 2:06 PM >To: Song, Barry >Cc: David Brownell; dmitry.torokhov@gmail.com; >uclinux-dist-devel@blackfin.uclinux.org; linux-input@vger.kernel.org >Subject: Re: [Uclinux-dist-devel] [PATCH v2]add analog devices >AD714Xcaptouch input driver > >On Tue, Sep 8, 2009 at 23:33, Song, Barry wrote: >> +static inline int ad714x_spi_register_driver(struct >spi_driver *spi_drv) >> +{ >> + Â Â Â return spi_register_driver(spi_drv); >> +} >> + >> +static inline void ad714x_spi_unregister_driver(struct >spi_driver *spi_drv) >> +{ >> + Â Â Â spi_unregister_driver(spi_drv); >> +} >> + >> +#else >> +#define ad714x_spi_register_driver(p) 0 >> +#define ad714x_spi_unregister_driver(p) >> Â #endif > >i dont think you need the ifdef protection that far up. what about >using the ifdef in this one place: >static inline int ad714x_spi_register_driver(struct spi_driver >*spi_drv) >{ >#if defined(CONFIG_SPI) || defined(CONFIG_SPI_MODULE) > return spi_register_driver(spi_drv); >#else > return 0; >#endif >} > >then i imagine you can have all the other pieces always enabled. gcc >will see that everything is marked static and not used, so it'll cull >things. In fact, I have tried to do like this before changing like I sent in the last mail. In fact, it will get compiling error if I always enable all spi and i2c functions without a #ifdef protection. So since there are far #ifdef, I think it shoud be better to place related codes to existed #ifdef. >-mike > -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Sep 9, 2009 at 03:15, Song, Barry wrote: >From: Mike Frysinger [mailto:vapier.adi@gmail.com] >>On Tue, Sep 8, 2009 at 23:33, Song, Barry wrote: >>> +static inline int ad714x_spi_register_driver(struct >>> spi_driver *spi_drv) >>> +{ >>> + Â Â Â return spi_register_driver(spi_drv); >>> +} >>> + >>> +static inline void ad714x_spi_unregister_driver(struct >>spi_driver *spi_drv) >>> +{ >>> + Â Â Â spi_unregister_driver(spi_drv); >>> +} >>> + >>> +#else >>> +#define ad714x_spi_register_driver(p) 0 >>> +#define ad714x_spi_unregister_driver(p) >>> Â #endif >> >>i dont think you need the ifdef protection that far up. Â what about >>using the ifdef in this one place: >>static inline int ad714x_spi_register_driver(struct spi_driver >>*spi_drv) >>{ >>#if defined(CONFIG_SPI) || defined(CONFIG_SPI_MODULE) >> Â Â return spi_register_driver(spi_drv); >>#else >> Â Â return 0; >>#endif >>} >> >>then i imagine you can have all the other pieces always enabled. Â gcc >>will see that everything is marked static and not used, so it'll cull >>things. > > In fact, I have tried to do like this before changing like I sent in the last mail. In fact, it will get compiling error if I always enable all spi and i2c functions without a #ifdef protection. > So since there are far #ifdef, I think it shoud be better to place related codes to existed #ifdef. i really have no idea what you're trying to say or why it didnt work for you -mike -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
>-----Original Message----- >From: Mike Frysinger [mailto:vapier.adi@gmail.com] >Sent: Wednesday, September 09, 2009 3:48 PM >To: Song, Barry >Cc: David Brownell; dmitry.torokhov@gmail.com; >uclinux-dist-devel@blackfin.uclinux.org; linux-input@vger.kernel.org >Subject: Re: [Uclinux-dist-devel] [PATCH v2]add analog devices >AD714Xcaptouch input driver > >On Wed, Sep 9, 2009 at 03:15, Song, Barry wrote: >>From: Mike Frysinger [mailto:vapier.adi@gmail.com] >>>On Tue, Sep 8, 2009 at 23:33, Song, Barry wrote: >>>> +static inline int ad714x_spi_register_driver(struct >>>> spi_driver *spi_drv) >>>> +{ >>>> + Â Â Â return spi_register_driver(spi_drv); >>>> +} >>>> + >>>> +static inline void ad714x_spi_unregister_driver(struct >>>spi_driver *spi_drv) >>>> +{ >>>> + Â Â Â spi_unregister_driver(spi_drv); >>>> +} >>>> + >>>> +#else >>>> +#define ad714x_spi_register_driver(p) 0 >>>> +#define ad714x_spi_unregister_driver(p) >>>> Â #endif >>> >>>i dont think you need the ifdef protection that far up. Â what about >>>using the ifdef in this one place: >>>static inline int ad714x_spi_register_driver(struct spi_driver >>>*spi_drv) >>>{ >>>#if defined(CONFIG_SPI) || defined(CONFIG_SPI_MODULE) >>> Â Â return spi_register_driver(spi_drv); >>>#else >>> Â Â return 0; >>>#endif >>>} >>> >>>then i imagine you can have all the other pieces always enabled. Â gcc >>>will see that everything is marked static and not used, so it'll cull >>>things. >> >> In fact, I have tried to do like this before changing like I >sent in the last mail. In fact, it will get compiling error if >I always enable all spi and i2c functions without a #ifdef protection. >> So since there are far #ifdef, I think it shoud be better to >place related codes to existed #ifdef. > >i really have no idea what you're trying to say or why it >didnt work for you I guess you want to change like this: - #if (defined(CONFIG_SPI) || defined(CONFIG_SPI_MODULE)) static int ad714x_spi_read(struct device *dev, unsigned short reg, unsigned short *data) { ... static struct spi_driver ad714x_spi_driver = { ... static inline int ad714x_spi_register_driver(struct spi_driver *spi_drv) { #if defined(CONFIG_SPI) || defined(CONFIG_SPI_MODULE) return spi_register_driver(spi_drv); #else return 0; #endif } static inline void ad714x_spi_unregister_driver(struct spi_driver *spi_drv) { #if defined(CONFIG_SPI) || defined(CONFIG_SPI_MODULE) spi_unregister_driver(spi_drv); #endif } - #endif - #if (defined(CONFIG_I2C) || defined(CONFIG_I2C_MODULE)) static int ad714x_i2c_write(struct device *dev, unsigned short reg, unsigned short data) { ... static struct i2c_driver ad714x_i2c_driver = { ... static inline int ad714x_i2c_add_driver(struct i2c_driver *i2c_drv) { #if defined(CONFIG_I2C) || defined(CONFIG_I2C_MODULE) return i2c_add_driver(i2c_drv); #else return 0; #endif } static inline void ad714x_i2c_del_driver(struct i2c_driver *i2c_drv) { #if defined(CONFIG_I2C) || defined(CONFIG_I2C_MODULE) i2c_del_driver(i2c_drv); #endif } - #endif static int __init ad714x_init(void) { int ret = 0; ret = ad714x_spi_register_driver(&ad714x_spi_driver); if (ret) goto err; ret = ad714x_i2c_add_driver(&ad714x_i2c_driver); if (ret) ad714x_spi_unregister_driver(&ad714x_spi_driver); err: return ret; } static void __exit ad714x_exit(void) { ad714x_spi_unregister_driver(&ad714x_spi_driver); ad714x_i2c_del_driver(&ad714x_i2c_driver); } In fact, it can still cause link errors due to lost i2c/spi symbols if only spi/i2c enabled. It makes simple things too complex. So I think keeping the #if (defined(CONFIG_I2C) || defined(CONFIG_I2C_MODULE)) #if (defined(CONFIG_SPI) || defined(CONFIG_SPI_MODULE)) is better. Since I don't delete the two far #ifdefs, I'd like to split spi/i2c register/unregister definition to those #ifdef to cover them. If you consider ad714x_spi_driver and ad714x_i2c_driver, field functions in them include internal i2c/spi symbols, which can cause link errors too if the two variants are not protected by far #ifdef. >-mike > -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Index: ad714x.c =================================================================== --- ad714x.c (revision 7274) +++ ad714x.c (working copy) @@ -1478,9 +1478,22 @@ .suspend = ad714x_spi_suspend, .resume = ad714x_spi_resume, }; + +static inline int ad714x_spi_register_driver(struct spi_driver *spi_drv) +{ + return spi_register_driver(spi_drv); +} + +static inline void ad714x_spi_unregister_driver(struct spi_driver *spi_drv) +{ + spi_unregister_driver(spi_drv); +} + +#else +#define ad714x_spi_register_driver(p) 0 +#define ad714x_spi_unregister_driver(p) #endif - #if defined(CONFIG_I2C) || defined(CONFIG_I2C_MODULE) static int ad714x_i2c_write(struct device *dev, unsigned short reg, unsigned short data) @@ -1595,43 +1608,40 @@ .resume = ad714x_i2c_resume, .id_table = ad714x_id, }; + +static inline int ad714x_i2c_add_driver(struct i2c_driver *i2c_drv) +{ + return i2c_add_driver(i2c_drv); +} + +static inline void ad714x_i2c_del_driver(struct i2c_driver *i2c_drv) +{ + i2c_del_driver(i2c_drv); +} + +#else +#define ad714x_i2c_add_driver(p) 0 +#define ad714x_i2c_del_driver(p) #endif static int __init ad714x_init(void) { -#if (defined(CONFIG_SPI) || defined(CONFIG_SPI_MODULE)) && \ - !(defined(CONFIG_I2C) || defined(CONFIG_I2C_MODULE)) - return spi_register_driver(&ad714x_spi_driver); -#endif -#if (defined(CONFIG_I2C) || defined(CONFIG_I2C_MODULE)) && \ - !(defined(CONFIG_SPI) || defined(CONFIG_SPI_MODULE)) - return i2c_add_driver(&ad714x_i2c_driver); -#endif - -#if (defined(CONFIG_SPI) || defined(CONFIG_SPI_MODULE)) && \ - (defined(CONFIG_I2C) || defined(CONFIG_I2C_MODULE)) int ret = 0; - ret = spi_register_driver(&ad714x_spi_driver); + ret = ad714x_spi_register_driver(&ad714x_spi_driver); if (ret) goto err; - ret = i2c_add_driver(&ad714x_i2c_driver); + ret = ad714x_i2c_add_driver(&ad714x_i2c_driver); if (ret) - spi_unregister_driver(&ad714x_spi_driver); + ad714x_spi_unregister_driver(&ad714x_spi_driver); err: return ret; -#endif } static void __exit ad714x_exit(void) { -#if defined(CONFIG_SPI) || defined(CONFIG_SPI_MODULE) - spi_unregister_driver(&ad714x_spi_driver); -#endif - -#if defined(CONFIG_I2C) || defined(CONFIG_I2C_MODULE) - i2c_del_driver(&ad714x_i2c_driver); -#endif + ad714x_spi_unregister_driver(&ad714x_spi_driver); + ad714x_i2c_del_driver(&ad714x_i2c_driver); } module_init(ad714x_init);