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

Signed-off-by: Antonio Ospite <ospite@studenti.unina.it>
Signed-off-by: Bart Visscher <bartv@thisnet.nl>
---
 arch/arm/mach-pxa/ezx.c |  178 +++++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 178 insertions(+), 0 deletions(-)

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)

or (which tends to be more accurate but not necessary)

#define GPIO_nCAM_EN	mfp_to_gpio(MFP_PIN_GPIO50)

If platform matters, I suggest something like:

#define GPIO_A780_nCAM_EN	(50)
#define GPIO_A910_nCAM_EN	(<something else>)

...

	err = gpio_request(GPIO_nCAM_EN, "nCAM_EN");

> +       if (err) {
> +               pr_err("%s: Failed to request nCAM_EN\n", __func__);
> +               goto fail;
> +       }
> +
> +       err = gpio_request(MFP_PIN_GPIO19, "CAM_RST");

ditto

> +       if (err) {
> +               pr_err("%s: Failed to request CAM_RST\n", __func__);
> +               goto fail_gpio_cam_rst;
> +       }
> +
> +       gpio_direction_output(MFP_PIN_GPIO50, 0);
> +       gpio_direction_output(MFP_PIN_GPIO19, 1);
> +
> +       return 0;
> +
> +fail_gpio_cam_rst:
> +       gpio_free(MFP_PIN_GPIO50);
> +fail:
> +       return err;
> +}
> +
> +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?

> +
> +       return 0;
> +}
> +
> +static int a780_pxacamera_reset(struct device *dev)
> +{
> +       gpio_set_value(MFP_PIN_GPIO19, 0);
> +       msleep(10);
> +       gpio_set_value(MFP_PIN_GPIO19, 1);

better to define something like above:

#define GPIO_CAM_RESET	(19)

...

	gpio_set_value(GPIO_CAM_RESET, ...);

> +
> +       return 0;
> +}
> +
> +struct pxacamera_platform_data a780_pxacamera_platform_data = {
> +       .init   = a780_pxacamera_init,
> +       .flags  = PXA_CAMERA_MASTER | PXA_CAMERA_DATAWIDTH_8 |
> +               PXA_CAMERA_PCLK_EN | PXA_CAMERA_MCLK_EN,
> +       .mclk_10khz = 5000,
> +};
> +
> +static struct i2c_board_info a780_camera_i2c_board_info = {
> +       I2C_BOARD_INFO("mt9m111", 0x5d),
> +};
> +
> +static struct soc_camera_link a780_iclink = {
> +       .bus_id         = 0,
> +       .flags          = SOCAM_SENSOR_INVERT_PCLK,
> +       .i2c_adapter_id = 0,
> +       .board_info     = &a780_camera_i2c_board_info,
> +       .module_name    = "mt9m111",
> +       .power          = a780_pxacamera_power,
> +       .reset          = a780_pxacamera_reset,
> +};
> +
> +static struct platform_device a780_camera = {
> +       .name   = "soc-camera-pdrv",
> +       .id     = 0,
> +       .dev    = {
> +               .platform_data = &a780_iclink,
> +       },
> +};
> +
>  static struct platform_device *a780_devices[] __initdata = {
>        &a780_gpio_keys,
> +       &a780_camera,
>  };
>
>  static void __init a780_init(void)
> @@ -699,6 +797,8 @@ static void __init a780_init(void)
>
>        pxa_set_keypad_info(&a780_keypad_platform_data);
>
> +       pxa_set_camera_info(&a780_pxacamera_platform_data);
> +
>        platform_add_devices(ARRAY_AND_SIZE(ezx_devices));
>        platform_add_devices(ARRAY_AND_SIZE(a780_devices));
>  }
> @@ -864,8 +964,84 @@ static struct platform_device a910_gpio_keys = {
>        },
>  };
>
> +/* camera */
> +static int a910_pxacamera_init(struct device *dev)
> +{
> +       int err;
> +
> +       /*
> +        * GPIO50_GPIO is CAM_EN: active low
> +        * GPIO28_GPIO is CAM_RST: active high
> +        */
> +       err = gpio_request(MFP_PIN_GPIO50, "nCAM_EN");
> +       if (err) {
> +               pr_err("%s: Failed to request nCAM_EN\n", __func__);
> +               goto fail;
> +       }
> +
> +       err = gpio_request(MFP_PIN_GPIO28, "CAM_RST");
> +       if (err) {
> +               pr_err("%s: Failed to request CAM_RST\n", __func__);
> +               goto fail_gpio_cam_rst;
> +       }
> +
> +       gpio_direction_output(MFP_PIN_GPIO50, 0);
> +       gpio_direction_output(MFP_PIN_GPIO28, 1);
> +
> +       return 0;
> +
> +fail_gpio_cam_rst:
> +       gpio_free(MFP_PIN_GPIO50);
> +fail:
> +       return err;
> +}
> +
> +static int a910_pxacamera_power(struct device *dev, int on)
> +{
> +       gpio_set_value(MFP_PIN_GPIO50, on ? 0 : 1);
> +       return 0;
> +}
> +
> +static int a910_pxacamera_reset(struct device *dev)
> +{
> +       gpio_set_value(MFP_PIN_GPIO28, 0);
> +       msleep(10);
> +       gpio_set_value(MFP_PIN_GPIO28, 1);
> +
> +       return 0;
> +}
> +
> +struct pxacamera_platform_data a910_pxacamera_platform_data = {
> +       .init   = a910_pxacamera_init,
> +       .flags  = PXA_CAMERA_MASTER | PXA_CAMERA_DATAWIDTH_8 |
> +               PXA_CAMERA_PCLK_EN | PXA_CAMERA_MCLK_EN,
> +       .mclk_10khz = 5000,
> +};
> +
> +static struct i2c_board_info a910_camera_i2c_board_info = {
> +       I2C_BOARD_INFO("mt9m111", 0x5d),
> +};
> +
> +static struct soc_camera_link a910_iclink = {
> +       .bus_id         = 0,
> +       .i2c_adapter_id = 0,
> +       .board_info     = &a910_camera_i2c_board_info,
> +       .module_name    = "mt9m111",
> +       .power          = a910_pxacamera_power,
> +       .reset          = a910_pxacamera_reset,
> +};
> +
> +static struct platform_device a910_camera = {
> +       .name   = "soc-camera-pdrv",
> +       .id     = 0,
> +       .dev    = {
> +               .platform_data = &a910_iclink,
> +       },
> +};
> +
>  static struct platform_device *a910_devices[] __initdata = {
>        &a910_gpio_keys,
> +       &a910_camera,
>  };
>
>  static void __init a910_init(void)
> @@ -880,6 +1056,8 @@ static void __init a910_init(void)
>
>        pxa_set_keypad_info(&a910_keypad_platform_data);
>
> +       pxa_set_camera_info(&a910_pxacamera_platform_data);
> +
>        platform_add_devices(ARRAY_AND_SIZE(ezx_devices));
>        platform_add_devices(ARRAY_AND_SIZE(a910_devices));
>  }
> --
> 1.6.5.2
>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

> or (which tends to be more accurate but not necessary)
> 
> #define GPIO_nCAM_EN	mfp_to_gpio(MFP_PIN_GPIO50)
> 
> If platform matters, I suggest something like:
> 
> #define GPIO_A780_nCAM_EN	(50)
> #define GPIO_A910_nCAM_EN	(<something else>)
> 
> ...
> 
> 	err = gpio_request(GPIO_nCAM_EN, "nCAM_EN");
> 
> > +       if (err) {
> > +               pr_err("%s: Failed to request nCAM_EN\n", __func__);
> > +               goto fail;
> > +       }
> > +
> > +       err = gpio_request(MFP_PIN_GPIO19, "CAM_RST");
> 
> ditto
> 
> > +       if (err) {
> > +               pr_err("%s: Failed to request CAM_RST\n", __func__);
> > +               goto fail_gpio_cam_rst;
> > +       }
> > +
> > +       gpio_direction_output(MFP_PIN_GPIO50, 0);
> > +       gpio_direction_output(MFP_PIN_GPIO19, 1);
> > +
> > +       return 0;
> > +
> > +fail_gpio_cam_rst:
> > +       gpio_free(MFP_PIN_GPIO50);
> > +fail:
> > +       return err;
> > +}
> > +
> > +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.

Thanks
Guennadi

> 
> > +
> > +#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?
> 
> > +
> > +       return 0;
> > +}
> > +
> > +static int a780_pxacamera_reset(struct device *dev)
> > +{
> > +       gpio_set_value(MFP_PIN_GPIO19, 0);
> > +       msleep(10);
> > +       gpio_set_value(MFP_PIN_GPIO19, 1);
> 
> better to define something like above:
> 
> #define GPIO_CAM_RESET	(19)
> 
> ...
> 
> 	gpio_set_value(GPIO_CAM_RESET, ...);
> 
> > +
> > +       return 0;
> > +}
> > +
> > +struct pxacamera_platform_data a780_pxacamera_platform_data = {
> > +       .init   = a780_pxacamera_init,
> > +       .flags  = PXA_CAMERA_MASTER | PXA_CAMERA_DATAWIDTH_8 |
> > +               PXA_CAMERA_PCLK_EN | PXA_CAMERA_MCLK_EN,
> > +       .mclk_10khz = 5000,
> > +};
> > +
> > +static struct i2c_board_info a780_camera_i2c_board_info = {
> > +       I2C_BOARD_INFO("mt9m111", 0x5d),
> > +};
> > +
> > +static struct soc_camera_link a780_iclink = {
> > +       .bus_id         = 0,
> > +       .flags          = SOCAM_SENSOR_INVERT_PCLK,
> > +       .i2c_adapter_id = 0,
> > +       .board_info     = &a780_camera_i2c_board_info,
> > +       .module_name    = "mt9m111",
> > +       .power          = a780_pxacamera_power,
> > +       .reset          = a780_pxacamera_reset,
> > +};
> > +
> > +static struct platform_device a780_camera = {
> > +       .name   = "soc-camera-pdrv",
> > +       .id     = 0,
> > +       .dev    = {
> > +               .platform_data = &a780_iclink,
> > +       },
> > +};
> > +
> >  static struct platform_device *a780_devices[] __initdata = {
> >        &a780_gpio_keys,
> > +       &a780_camera,
> >  };
> >
> >  static void __init a780_init(void)
> > @@ -699,6 +797,8 @@ static void __init a780_init(void)
> >
> >        pxa_set_keypad_info(&a780_keypad_platform_data);
> >
> > +       pxa_set_camera_info(&a780_pxacamera_platform_data);
> > +
> >        platform_add_devices(ARRAY_AND_SIZE(ezx_devices));
> >        platform_add_devices(ARRAY_AND_SIZE(a780_devices));
> >  }
> > @@ -864,8 +964,84 @@ static struct platform_device a910_gpio_keys = {
> >        },
> >  };
> >
> > +/* camera */
> > +static int a910_pxacamera_init(struct device *dev)
> > +{
> > +       int err;
> > +
> > +       /*
> > +        * GPIO50_GPIO is CAM_EN: active low
> > +        * GPIO28_GPIO is CAM_RST: active high
> > +        */
> > +       err = gpio_request(MFP_PIN_GPIO50, "nCAM_EN");
> > +       if (err) {
> > +               pr_err("%s: Failed to request nCAM_EN\n", __func__);
> > +               goto fail;
> > +       }
> > +
> > +       err = gpio_request(MFP_PIN_GPIO28, "CAM_RST");
> > +       if (err) {
> > +               pr_err("%s: Failed to request CAM_RST\n", __func__);
> > +               goto fail_gpio_cam_rst;
> > +       }
> > +
> > +       gpio_direction_output(MFP_PIN_GPIO50, 0);
> > +       gpio_direction_output(MFP_PIN_GPIO28, 1);
> > +
> > +       return 0;
> > +
> > +fail_gpio_cam_rst:
> > +       gpio_free(MFP_PIN_GPIO50);
> > +fail:
> > +       return err;
> > +}
> > +
> > +static int a910_pxacamera_power(struct device *dev, int on)
> > +{
> > +       gpio_set_value(MFP_PIN_GPIO50, on ? 0 : 1);
> > +       return 0;
> > +}
> > +
> > +static int a910_pxacamera_reset(struct device *dev)
> > +{
> > +       gpio_set_value(MFP_PIN_GPIO28, 0);
> > +       msleep(10);
> > +       gpio_set_value(MFP_PIN_GPIO28, 1);
> > +
> > +       return 0;
> > +}
> > +
> > +struct pxacamera_platform_data a910_pxacamera_platform_data = {
> > +       .init   = a910_pxacamera_init,
> > +       .flags  = PXA_CAMERA_MASTER | PXA_CAMERA_DATAWIDTH_8 |
> > +               PXA_CAMERA_PCLK_EN | PXA_CAMERA_MCLK_EN,
> > +       .mclk_10khz = 5000,
> > +};
> > +
> > +static struct i2c_board_info a910_camera_i2c_board_info = {
> > +       I2C_BOARD_INFO("mt9m111", 0x5d),
> > +};
> > +
> > +static struct soc_camera_link a910_iclink = {
> > +       .bus_id         = 0,
> > +       .i2c_adapter_id = 0,
> > +       .board_info     = &a910_camera_i2c_board_info,
> > +       .module_name    = "mt9m111",
> > +       .power          = a910_pxacamera_power,
> > +       .reset          = a910_pxacamera_reset,
> > +};
> > +
> > +static struct platform_device a910_camera = {
> > +       .name   = "soc-camera-pdrv",
> > +       .id     = 0,
> > +       .dev    = {
> > +               .platform_data = &a910_iclink,
> > +       },
> > +};
> > +
> >  static struct platform_device *a910_devices[] __initdata = {
> >        &a910_gpio_keys,
> > +       &a910_camera,
> >  };
> >
> >  static void __init a910_init(void)
> > @@ -880,6 +1056,8 @@ static void __init a910_init(void)
> >
> >        pxa_set_keypad_info(&a910_keypad_platform_data);
> >
> > +       pxa_set_camera_info(&a910_pxacamera_platform_data);
> > +
> >        platform_add_devices(ARRAY_AND_SIZE(ezx_devices));
> >        platform_add_devices(ARRAY_AND_SIZE(a910_devices));
> >  }
> > --
> > 1.6.5.2
> >
> >
> 

---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Wed, 4 Nov 2009 14:38:40 +0800
Eric Miao <eric.y.miao@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?

> 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.

> > +
> > +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.

[...]

Thanks,
   Antonio

On Wed, Nov 4, 2009 at 5:14 PM, Antonio Ospite <ospite@studenti.unina.it> wrote:
> On Wed, 4 Nov 2009 14:38:40 +0800
> Eric Miao <eric.y.miao@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.
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

(added Robert to CC)

On Wed, 4 Nov 2009, Antonio Ospite wrote:

> On Wed, 4 Nov 2009 09:13:16 +0100 (CET)
> Guennadi Liakhovetski <g.liakhovetski@gmx.de> wrote:
> 
> > > > +/* 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?

Good that you mentioned this. In fact, I think, that .init should go. So 
far it is used in pcm990-baseboard.c to initialise pins. You're doing 
essentially the same - requesting and configuring GPIOs. And it has been 
agreed, that there is so far no real case, where a static 
GPIO-configuration wouldn't work. So, I would suggest you remove .init, 
configure GPIOs statically. And then submit a patch to remove .init 
completely from struct pxacamera_platform_data. Robert, do you agree?

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Guennadi Liakhovetski <g.liakhovetski@gmx.de> writes:

> Good that you mentioned this. In fact, I think, that .init should go. So 
> far it is used in pcm990-baseboard.c to initialise pins. You're doing 
> essentially the same - requesting and configuring GPIOs. And it has been 
> agreed, that there is so far no real case, where a static 
> GPIO-configuration wouldn't work. So, I would suggest you remove .init, 
> configure GPIOs statically. And then submit a patch to remove .init 
> completely from struct pxacamera_platform_data. Robert, do you agree?

Yes, fully agree, I think too that GPIO should be static.

Cheers.

--
Robert
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Fri, 06 Nov 2009 18:05:57 +0100
Robert Jarzmik <robert.jarzmik@free.fr> wrote:

> Guennadi Liakhovetski <g.liakhovetski@gmx.de> writes:
> 
> > Good that you mentioned this. In fact, I think, that .init should go. So 
> > far it is used in pcm990-baseboard.c to initialise pins. You're doing 
> > essentially the same - requesting and configuring GPIOs. And it has been 
> > agreed, that there is so far no real case, where a static 
> > GPIO-configuration wouldn't work. So, I would suggest you remove .init, 
> > configure GPIOs statically. And then submit a patch to remove .init 
> > completely from struct pxacamera_platform_data. Robert, do you agree?
> 
> Yes, fully agree, I think too that GPIO should be static.
>

Well, the other drivers I am using (pxamci, ezx-pcap, gpio-keys,
to mention some) request and configure GPIOs during their own
init/probe, they don't require the *board* init code to configure them.

But if you really like the static way I'll bend to your will.

Regards,
   Antonio