Re: [PATCH] V4L/DVB: Add support for M5MOLS Mega Pixel camera

[Sylwester Nawrocki <snjw23@gmail.com>]

On 12/17/2010 05:35 PM, Sylwester Nawrocki wrote:
>
> Hi HeungJun,
>
> On 12/17/2010 07:42 AM, Kim, HeungJun wrote:
>> Hi Sylwester,
>>
>> Thanks for some comments. I'll reflects this comments for the next version
>> patch. It's clear that there is a few things I missed. It's better to let's
>> talk about this rest things, as Mr. Park said.
>>
>> But, I wanna remind one thing, and know your exact thiking. about MACROs.
>>
>> I re-comments of that. look around and re-re-comments it, plz.
>>
>>>> +
>>>> +/* MACRO */
>>>> +#define e_check_w(fn, cat, byte, val, bitwidth)        do {    \
>>>> +    int ret;                        \
>>>> +    ret = (int)(fn);                    \
>>>> +    if ((ret)<   0) {                    \
>>>> +        dev_err(&client->dev, "fail i2c WRITE [%s] - "    \
>>>> +                "category:0x%02x, "        \
>>>> +                "bytes:0x%02x, "        \
>>>> +                "value:0x%02x\n",        \
>>>> +                (bitwidth),            \
>>>> +                (cat), (byte), (u32)val);    \
>>>> +        return ret;                    \
>>>> +    }                            \
>>>> +} while (0)
>>>> +
>>>> +#define e_check_r(fn, cat, byte, val, bitwidth)        do {    \
>>>> +    int ret;                        \
>>>> +    ret = (int)(fn);                    \
>>>> +    if ((ret)<   0) {                    \
>>>> +        dev_err(&client->dev, "fail i2c READ [%s] - "    \
>>>> +                "category:0x%02x, "        \
>>>> +                "bytes:0x%02x, "        \
>>>> +                "value:0x%02x\n",        \
>>>> +                (bitwidth),            \
>>>> +                (cat), (byte), (u32)(*val));    \
>>>> +        return ret;                    \
>>>> +    }                            \
>>>> +} while (0)
>>>> +
>>>> +#define REG_W_8(cat, byte, value)                    \
>>>> +    e_check_w(m5mols_write_reg(sd, M5MOLS_8BIT, cat, byte, value),    \
>>>> +            cat, byte, value, "8bit")
>>>> +#define REG_R_8(cat, byte, value)                    \
>>>> +    e_check_r(m5mols_read_reg(sd, M5MOLS_8BIT, cat, byte, value),    \
>>>> +            cat, byte, value, "8bit")
>>>> +
>>>> +#define e_check_mode(fn, mode)                do {    \
>>>> +    int ret;                        \
>>>> +    ret = (int)(fn);                    \
>>>> +    if (ret<   0) {                        \
>>>> +        dev_err(&client->dev, "Failed to %s mode\n",    \
>>>> +                (mode));            \
>>>> +        return ret;                    \
>>>> +    }                            \
>>>> +} while (0)
>>>
>>> These macros really do not look good. Moreover they all change
>>> the control flow, i.e. return a value. From Documentation/CodingStyle:
>>>
>>> "Things to avoid when using macros:
>>>
>>> 1) macros that affect control flow:
>>>
>>> #define FOO(x)                                  \
>>>          do {                                    \
>>>                  if (blah(x)<  0)                \
>>>                          return -EBUGGERED;      \
>>>          } while(0)
>>>
>>> is a _very_ bad idea.  It looks like a function call but exits the
>>> "calling" function; don't break the internal parsers of those who will
>>> read the code."
>>
>> I know about Documentation/CodingStyle and absolutely know about
>> the risks of MACRO like upper case. I even know the _very_'s meanings.
>> But, I think this case is different any other MACRO cases to be concrete
>> whether use or not. Actually, I've not even found address symbol using T32
>> debuggers cause of MACROs. I have realized danger before long time.
>>
>> I know Documentation/CodingStyle is very strong recommandation.
>> And must keep this style but, it seems to happen the specific case.
>> The specific means, not general and only used in the M5MOLS code.
>> The following is my thinking at past.
>>
>> 1. There are a lot of I2C communication is in M5MOLS driver code.
>>     The M5MOLS has 16 category, and about 50 commands in the each category.
>>     If each command need 1 line on the code, the amount to be charged I2C
>>     communication only is 800 lines. What if each command be plused 3~4
>>     error checking code? The code amount is 3200 lines at least. Moreover,
>>     No guarantiee 1 command only 1 time excuetion. So, 3200 more lines
>>     would be added at the future. (The m5mo driver you've seen and used before,
>>     is not yet inserted all controls.)
>
> Then this sensor needs careful design and function partitioning.
> Single *.c file should not exceed 1000 lines.
> I think we need a separate directory for it, like media/drivers/m5mols/.

ouch, that was supposed to be drivers/media/video/m5mols