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

[Antonio Ospite <ospite@studenti.unina.it>]

[-- Attachment #1: Type: text/plain, Size: 1764 bytes --]

On Wed, 4 Nov 2009 09:13:16 +0100 (CET)
Guennadi Liakhovetski <g.liakhovetski@gmx.de> wrote:

> On Wed, 4 Nov 2009, Eric Miao wrote:
> 
> > Hi Antonio,
> > 
> > Patch looks generally OK except for the MFP/GPIO usage, check my
> > comments below, thanks.
> > 
> > > +/* 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)
> 
> ...but without parenthesis, please:
> 
> #define GPIO_nCAM_EN	50
> 
> same everywhere below
>

OK.

BTW, Guennadi, shouldn't the pxa_camera platform_data expose also an
exit() method for symmetry with the init() one, where we can free the
requested resources?

If you want I think I can add it.

[...]
> > > +
> > > +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);
> 
> IMHO better yet
> 
> 	gpio_set_value(GPIO_nCAM_EN, !on);
> 
> Also throughout the patch below.
> 
> I'm still to look at it miself and maybe provide a couple more comments, 
> if any.
> 

Ok, thanks,
   Antonio

-- 
Antonio Ospite
http://ao2.it

PGP public key ID: 0x4553B001

A: Because it messes up the order in which people normally read text.
   See http://en.wikipedia.org/wiki/Posting_style
Q: Why is top-posting such a bad thing?
A: Top-posting.
Q: What is the most annoying thing in e-mail?

[-- Attachment #2: Type: application/pgp-signature, Size: 198 bytes --]