RE: [Uclinux-dist-devel] [PATCH v2]add analog devices AD714Xcaptouch input driver

["Song, Barry" <Barry.Song@xxxxxxxxxx>]

>-----Original Message-----
>From: Mike Frysinger [mailto:vapier.adi@xxxxxxxxx] 
>Sent: Wednesday, September 09, 2009 3:48 PM
>To: Song, Barry
>Cc: David Brownell; dmitry.torokhov@xxxxxxxxx; 
>uclinux-dist-devel@xxxxxxxxxxxxxxxxxxxx; linux-input@xxxxxxxxxxxxxxx
>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@xxxxxxxxx]
>>>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@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html