Re: [PATCH v2] cirrus-logic-framebuffer-i2c-support.patch

Antonino A. Daplas
2006-07-10T10:07:44Z

Krzysztof Halasa wrote:
> "Antonino A. Daplas" <adaplas@pol.net> writes:
>
>> Why don't you create a separate function for this, ie cirrusfb_create_i2c()
>> or something. This way, we eliminate the #ifdef/#endif inside the function.
>
> #ifdef inside a function isn't a problem, while unnecessary complication
> (= worse readability) is.

There are better reasons (ie, smaller patch), but worse readability is not
one of them.

I could more easily grasp the code flow of cirrusfb_register() if you
just inserted "cirrusfb_create_i2c_buses()" instead of:

+ cinfo->bit_cirrus_data.setsda = alpine_setsda;
+ cinfo->bit_cirrus_data.setscl = alpine_setscl;
+ cinfo->bit_cirrus_data.getsda = alpine_getsda;
+ cinfo->bit_cirrus_data.getscl = alpine_getscl;
+ cinfo->bit_cirrus_data.udelay = 5;
+ cinfo->bit_cirrus_data.mdelay = 1;
+ cinfo->bit_cirrus_data.timeout = HZ;
+ cinfo->bit_cirrus_data.data = cinfo;
+ cinfo->cirrus_ops.owner = THIS_MODULE;
+ cinfo->cirrus_ops.id = I2C_HW_B_CIRRUS;
+ cinfo->cirrus_ops.class = I2C_CLASS_DDC;
+ cinfo->cirrus_ops.algo_data = &cinfo->bit_cirrus_data;
+ cinfo->cirrus_ops.dev.parent = info->device;
+ strlcpy(cinfo->cirrus_ops.name, "Cirrus Logic DDC I2C adapter",
+ I2C_NAME_SIZE);
+ if (!(err = i2c_bit_add_bus(&cinfo->cirrus_ops))) {
+ printk(KERN_DEBUG "Initialized Cirrus Logic I2C"
+ " adapter\n");
cinfo->i2c_used = 1;
} else
- printk(KERN_WARNING "Unable to initialize Alpine I2C"
- "adapter (result = %i)\n", err);
+ printk(KERN_WARNING "Unable to initialize Cirrus"
+ " Logic I2C adapter (result = %i)\n", err);

Tony