Re: [PATCH 2/2] radio-si470x: add i2c driver for si470x

[Joonyoung Shim <jy0922.shim@samsung.com>]

Hi, Alexey.

<snip>

>> diff --git a/linux/drivers/media/radio/si470x/radio-si470x-common.c
>> b/linux/drivers/media/radio/si470x/radio-si470x-common.c
>> index d2dc1ff..77f79e7 100644
>> --- a/linux/drivers/media/radio/si470x/radio-si470x-common.c
>> +++ b/linux/drivers/media/radio/si470x/radio-si470x-common.c
>> @@ -473,11 +473,13 @@ static int si470x_vidioc_g_ctrl(struct file *file,
>> void *priv,
>>  	struct si470x_device *radio = video_drvdata(file);
>>  	int retval = 0;
>>
>> +#if defined(CONFIG_USB_SI470X) || defined(CONFIG_USB_SI470X_MODULE)
>>  	/* safety checks */
>>  	if (radio->disconnected) {
>>  		retval = -EIO;
>>  		goto done;
>>  	}
>> +#endif
> 
> I'm sorry for asking but is it possible to turn this into separate macro?
> Something like this for example:
> 
> /* comment about macro */
> #if defined (CONFIG_USB_SI470X) || defined(CONFIG_USB_SI470X_MODULE)
> #define safety_check() if() {
> ... checks ...
> }
> #elseif
> #define safety_check()
> #endif
> 
> to run away from many #if defined-#endif constructions in source code.
> Is it really good to redesign this or am i wrong here?
> 

I think your example is better, i will try it.


<snip>

>> +static int __init si470x_i2c_init(void)
>> +{
>> +	return i2c_add_driver(&si470x_i2c_driver);
>> +}
>> +module_init(si470x_i2c_init);
>> +
>> +static void __exit si470x_i2c_exit(void)
>> +{
>> +	i2c_del_driver(&si470x_i2c_driver);
>> +}
>> +module_exit(si470x_i2c_exit);
>> +
>> +MODULE_DESCRIPTION("i2c radio driver for si470x fm radio receivers");
>> +MODULE_AUTHOR("Joonyoung Shim <jy0922.shim@samsung.com>");
>> +MODULE_LICENSE("GPL");
> 
> Please, move this information to the top of file to see this
> information fast when you suddenly open source file.

I'm not sure about this because many linux drivers have the module
information at the bottom of file.