[Uclinux-dist-devel,v2] add analog devices AD714Xcaptouch input driver
diff mbox

Message ID 0F1B54C89D5F954D8535DB252AF412FA04B118A5@chinexm1.ad.analog.com
State New, archived
Headers show

Commit Message

Song, Barry Sept. 9, 2009, 3:33 a.m. UTC
>-----Original Message-----
>From: Song, Barry 
>Sent: Wednesday, September 09, 2009 11:00 AM
>To: 'David Brownell'; 'dmitry.torokhov@gmail.com'
>Cc: Barry Song; uclinux-dist-devel@blackfin.uclinux.org; 
>linux-input@vger.kernel.org
>Subject: RE: [Uclinux-dist-devel] [PATCH v2]add analog devices 
>AD714Xcaptouch input driver
>
> 
>
>>-----Original Message-----
>>From: David Brownell [mailto:david-b@pacbell.net] 
>>Sent: Wednesday, September 09, 2009 10:30 AM
>>To: Song, Barry
>>Cc: Barry Song; 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 Tuesday 08 September 2009, Song, Barry wrote:
>>> I don't know what the exact meaning of "inlined NOP stubs", 
>>do you mean things like the following?
>>> #if (defined(CONFIG_I2C) || defined(CONFIG_I2C_MODULE))
>>> ...
>>> static struct i2c_driver ad714x_i2c_driver = {
>>>         ..
>>> };
>>
>>You _should_ be able to always declare that and let GCC
>>optimize it away ...
Even though I can always declare it, the functions in ad714x_i2c_driver still access i2c symbols.
For that, I still need to use #ifdef to make the compiling pass.
>>
>>
>>> static inline int ad714x_i2c_add_driver(struct i2c_driver *i2c_drv)
>>> {
>>>         return i2c_add_driver(i2c_drv);
>>> }
>>> #else
>>> static inline int ad714x_i2c_add_driver(struct i2c_driver *i2c_drv)
>>> {
>>>         return 0;
>>> }
>>
>>That'd be OK as a quick fix; ideally i2c_add_driver() itself
>>would be what the #ifdef covers (in <linux/i2c.h> and likewise
>>in <linux/spi/spi.h>).  And you'd need to cover driver remove
>>paths too.
>>
>>
>>> #define ad714x_i2c_driver NULL
>>
>>Ditto: shouldn't need to #define that
Even though GCC can optimize unused variants/functions, there are still compiling error since field functions of ad714x_i2c_driver/ad714x_spi_driver and other functions will access other spi/i2c symbols.
So maybe the best fix is:
>>
>>
>>> #endif
>>> 
>>> static int __init ad714x_init(void)
>>> {
>>>         ...
>>>         ad714x_i2c_add_driver(&ad714x_i2c_driver);
>>
>>Yes; but do check the value!
>>
>>>         ...
>>> }
>>> 
>>> Then ad714x_init can use a unified ad714x_i2c_add_driver,
>>> ad714x_spi_register_driver to avoid #ifdeffery used? 
>>> It seems that doesn't make the codes more pretty?
>>
>>Way less #ifdeffery and *ONE* non-#ifdeffed chunk of
>>code in driver init/exit code.
>Fixed
>>
>>- Dave
>>
>>
>>
>
--
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

Comments

Mike Frysinger Sept. 9, 2009, 6:05 a.m. UTC | #1
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
Song, Barry Sept. 9, 2009, 7:15 a.m. UTC | #2
>-----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
Mike Frysinger Sept. 9, 2009, 7:47 a.m. UTC | #3
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
Song, Barry Sept. 9, 2009, 8:15 a.m. UTC | #4
>-----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

Patch
diff mbox

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);