[1/3] ezx: Add camera support for A780 and A910 EZX phones
Commit Message
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(-)
Comments
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
@@ -17,8 +17,11 @@
#include <linux/delay.h>
#include <linux/pwm_backlight.h>
#include <linux/input.h>
+#include <linux/gpio.h>
#include <linux/gpio_keys.h>
+#include <media/soc_camera.h>
+
#include <asm/setup.h>
#include <asm/mach-types.h>
#include <asm/mach/arch.h>
@@ -29,6 +32,7 @@
#include <plat/i2c.h>
#include <mach/hardware.h>
#include <mach/pxa27x_keypad.h>
+#include <mach/camera.h>
#include "devices.h"
#include "generic.h"
@@ -683,8 +687,102 @@ static struct platform_device a780_gpio_keys = {
},
};
+/* 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");
+ if (err) {
+ pr_err("%s: Failed to request nCAM_EN\n", __func__);
+ goto fail;
+ }
+
+ err = gpio_request(MFP_PIN_GPIO19, "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_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);
+
+#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
+
+ 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);
+
+ 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));
}