[PATCH 1/3] ezx: Add camera support for A780 and A910 EZX phones

[Eric Miao]

On Wed, Nov 4, 2009 at 5:14 PM, Antonio Ospite <ospite at studenti.unina.it> wrote:
> On Wed, 4 Nov 2009 14:38:40 +0800
> Eric Miao <eric.y.miao at gmail.com> wrote:
>
>> Hi Antonio,
>>
>> Patch looks generally OK except for the MFP/GPIO usage, check my
>> comments below, thanks.
>>
>
> Ok, will resend ASAP. Some questions inlined below after your comments.
>
>> > +/* camera */
>> > +static int a780_pxacamera_init(struct device *dev)
>> > +{
>> > +       int err;
>> > +
>> > +       /*
>> > +        * GPIO50_GPIO is CAM_EN: active low
>> > +        * GPIO19_GPIO is CAM_RST: active high
>> > +        */
>> > +       err = gpio_request(MFP_PIN_GPIO50, "nCAM_EN");
>>
>> Mmm... MFP != GPIO, so this probably should be written simply as:
>>
>> #define GPIO_nCAM_EN  (50)
>>
>
> If the use of parentheses here is recommended, should I send another
> patch to add them to current defines for GPIOs in ezx.c, for style
> consistency?

I don't actually care about that - with or without parentheses are both OK
to me, though Guennadi recommends removing them, so it would be
just OK to left them untouched.

>
>> or (which tends to be more accurate but not necessary)
>>
>> #define GPIO_nCAM_EN  mfp_to_gpio(MFP_PIN_GPIO50)
>>
>
> For me it is the same, just tell me if you really prefer this one.
>

OK, the first/simple one pls

>> > +
>> > +static int a780_pxacamera_power(struct device *dev, int on)
>> > +{
>> > +       gpio_set_value(MFP_PIN_GPIO50, on ? 0 : 1);
>>
>>       gpio_set_value(GPIO_nCAM_EN, on ? 0 : 1);
>>
>> > +
>> > +#if 0
>> > +       /*
>> > +        * This is reported to resolve the "vertical line in view finder"
>> > +        * issue (LIBff11930), in the original source code released by
>> > +        * Motorola, but we never experienced the problem, so we don't use
>> > +        * this for now.
>> > +        *
>> > +        * AP Kernel camera driver: set TC_MM_EN to low when camera is running
>> > +        * and TC_MM_EN to high when camera stops.
>> > +        *
>> > +        * BP Software: if TC_MM_EN is low, BP do not shut off 26M clock, but
>> > +        * BP can sleep itself.
>> > +        */
>> > +       gpio_set_value(MFP_PIN_GPIO99, on ? 0 : 1);
>> > +#endif
>>
>> This is a little bit confusing - can we remove this for this stage?
>>
>
> Ok, I am removing it for now. I might put this note in again in
> future, hopefully with a better description.
>

That would be good.