[v2,2/2] OMAP3BEAGLE: Add support for mt9p031 sensor driver.

Message ID 1305899272-31839-2-git-send-email-javier.martin@vista-silicon.com (mailing list archive)
State Superseded, archived
Headers

Commit Message

Javier Martin May 20, 2011, 1:47 p.m. UTC
  isp.h file has to be included as a temporal measure
since clocks of the isp are not exposed yet.

Signed-off-by: Javier Martin <javier.martin@vista-silicon.com>
---
 arch/arm/mach-omap2/board-omap3beagle.c |  127 ++++++++++++++++++++++++++++++-
 1 files changed, 123 insertions(+), 4 deletions(-)
  

Comments

Koen Kooi May 20, 2011, 3:57 p.m. UTC | #1
Op 20 mei 2011, om 15:47 heeft Javier Martin het volgende geschreven:

> isp.h file has to be included as a temporal measure
> since clocks of the isp are not exposed yet.
> 
> Signed-off-by: Javier Martin <javier.martin@vista-silicon.com>
> ---
> arch/arm/mach-omap2/board-omap3beagle.c |  127 ++++++++++++++++++++++++++++++-

Javier,

In previous patch sets we put that in a seperate file (omap3beagle-camera.c) so we don't clutter up the board file with all the different sensor drivers. Would it make sense to do the same with the current patches? It looks like MCF cuts down a lot on the boilerplace needed already.

regards,

Koen--
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
  
Igor Grinberg May 22, 2011, 1:49 p.m. UTC | #2
Hi Javier,


linux-omap should be CC'ed - added.

In addition to Koen's comments, some comments below.


On 05/20/11 16:47, Javier Martin wrote:

> isp.h file has to be included as a temporal measure
> since clocks of the isp are not exposed yet.
>
> Signed-off-by: Javier Martin <javier.martin@vista-silicon.com>
> ---
>  arch/arm/mach-omap2/board-omap3beagle.c |  127 ++++++++++++++++++++++++++++++-
>  1 files changed, 123 insertions(+), 4 deletions(-)
>
> diff --git a/arch/arm/mach-omap2/board-omap3beagle.c b/arch/arm/mach-omap2/board-omap3beagle.c
> index 33007fd..f52e6ae 100644
> --- a/arch/arm/mach-omap2/board-omap3beagle.c
> +++ b/arch/arm/mach-omap2/board-omap3beagle.c
> @@ -24,15 +24,20 @@
>  #include <linux/input.h>
>  #include <linux/gpio_keys.h>
>  #include <linux/opp.h>
> +#include <linux/i2c.h>
> +#include <linux/mm.h>
> +#include <linux/videodev2.h>
>  
>  #include <linux/mtd/mtd.h>
>  #include <linux/mtd/partitions.h>
>  #include <linux/mtd/nand.h>
>  #include <linux/mmc/host.h>
> -
> +#include <linux/gpio.h>

Why include this for second time?

>  #include <linux/regulator/machine.h>
>  #include <linux/i2c/twl.h>
>  
> +#include <media/mt9p031.h>
> +
>  #include <mach/hardware.h>
>  #include <asm/mach-types.h>
>  #include <asm/mach/arch.h>
> @@ -47,12 +52,17 @@
>  #include <plat/nand.h>
>  #include <plat/usb.h>
>  #include <plat/omap_device.h>
> +#include <plat/i2c.h>
>  
>  #include "mux.h"
>  #include "hsmmc.h"
>  #include "timer-gp.h"
>  #include "pm.h"
> +#include "devices.h"
> +#include "../../../drivers/media/video/omap3isp/isp.h"
>  
> +#define MT9P031_RESET_GPIO	98
> +#define MT9P031_XCLK		ISP_XCLK_A
>  #define NAND_BLOCK_SIZE		SZ_128K
>  
>  /*
> @@ -273,6 +283,44 @@ static struct regulator_consumer_supply beagle_vsim_supply = {
>  
>  static struct gpio_led gpio_leds[];
>  
> +static struct regulator_consumer_supply beagle_vaux3_supply = {
> +	.supply		= "cam_1v8",
> +};
> +
> +static struct regulator_consumer_supply beagle_vaux4_supply = {
> +	.supply		= "cam_2v8",
> +};
> +
> +/* VAUX3 for CAM_1V8 */
> +static struct regulator_init_data beagle_vaux3 = {
> +	.constraints = {
> +		.min_uV			= 1800000,
> +		.max_uV			= 1800000,
> +		.apply_uV		= true,
> +		.valid_modes_mask	= REGULATOR_MODE_NORMAL
> +					| REGULATOR_MODE_STANDBY,
> +		.valid_ops_mask		= REGULATOR_CHANGE_MODE
> +					| REGULATOR_CHANGE_STATUS,
> +	},
> +	.num_consumer_supplies	= 1,
> +	.consumer_supplies	= &beagle_vaux3_supply,
> +};
> +
> +/* VAUX4 for CAM_2V8 */
> +static struct regulator_init_data beagle_vaux4 = {
> +	.constraints = {
> +		.min_uV			= 1800000,
> +		.max_uV			= 1800000,
> +		.apply_uV		= true,
> +		.valid_modes_mask	= REGULATOR_MODE_NORMAL
> +					| REGULATOR_MODE_STANDBY,
> +		.valid_ops_mask		= REGULATOR_CHANGE_MODE
> +					| REGULATOR_CHANGE_STATUS,
> +	},
> +	.num_consumer_supplies	= 1,
> +	.consumer_supplies	= &beagle_vaux4_supply,
> +};
> +
>  static int beagle_twl_gpio_setup(struct device *dev,
>  		unsigned gpio, unsigned ngpio)
>  {
> @@ -309,6 +357,15 @@ static int beagle_twl_gpio_setup(struct device *dev,
>  			pr_err("%s: unable to configure EHCI_nOC\n", __func__);
>  	}
>  
> +	if (omap3_beagle_get_rev() == OMAP3BEAGLE_BOARD_XM) {
> +		/*
> +		 * Power on camera interface - only on pre-production, not
> +		 * needed on production boards
> +		 */
> +		gpio_request(gpio + 2, "CAM_EN");
> +		gpio_direction_output(gpio + 2, 1);

Why not gpio_request_one()?

> +	}
> +
>  	/*
>  	 * TWL4030_GPIO_MAX + 0 == ledA, EHCI nEN_USB_PWR (out, XM active
>  	 * high / others active low)
> @@ -451,6 +508,8 @@ static struct twl4030_platform_data beagle_twldata = {
>  	.vsim		= &beagle_vsim,
>  	.vdac		= &beagle_vdac,
>  	.vpll2		= &beagle_vpll2,
> +	.vaux3		= &beagle_vaux3,
> +	.vaux4		= &beagle_vaux4,
>  };
>  
>  static struct i2c_board_info __initdata beagle_i2c_boardinfo[] = {
> @@ -463,15 +522,16 @@ static struct i2c_board_info __initdata beagle_i2c_boardinfo[] = {
>  };
>  
>  static struct i2c_board_info __initdata beagle_i2c_eeprom[] = {
> -       {
> -               I2C_BOARD_INFO("eeprom", 0x50),
> -       },
> +	{
> +		I2C_BOARD_INFO("eeprom", 0x50),
> +	},
>  };

This part of the hunk is not related to the patch
and should be in a separate (cleanup) patch.

>  
>  static int __init omap3_beagle_i2c_init(void)
>  {
>  	omap_register_i2c_bus(1, 2600, beagle_i2c_boardinfo,
>  			ARRAY_SIZE(beagle_i2c_boardinfo));
> +	omap_register_i2c_bus(2, 100, NULL, 0); /* Enable I2C2 for camera */
>  	/* Bus 3 is attached to the DVI port where devices like the pico DLP
>  	 * projector don't work reliably with 400kHz */
>  	omap_register_i2c_bus(3, 100, beagle_i2c_eeprom, ARRAY_SIZE(beagle_i2c_eeprom));
> @@ -654,6 +714,60 @@ static void __init beagle_opp_init(void)
>  	return;
>  }
>  
> +static int beagle_cam_set_xclk(struct v4l2_subdev *subdev, int hz)
> +{
> +	struct isp_device *isp = v4l2_dev_to_isp_device(subdev->v4l2_dev);
> +	int ret;
> +
> +	ret = isp->platform_cb.set_xclk(isp, hz, MT9P031_XCLK);
> +	return 0;
> +}

Why do you need ret variable here, if you always return zero?

> +
> +static int beagle_cam_reset(struct v4l2_subdev *subdev, int active)
> +{
> +	/* Set RESET_BAR to !active */
> +	gpio_set_value(MT9P031_RESET_GPIO, !active);
> +
> +	return 0;
> +}
> +
> +static struct mt9p031_platform_data beagle_mt9p031_platform_data = {
> +	.set_xclk		= beagle_cam_set_xclk,
> +	.reset			= beagle_cam_reset,
> +};
> +
> +static struct i2c_board_info mt9p031_camera_i2c_device = {
> +	I2C_BOARD_INFO("mt9p031", 0x48),
> +	.platform_data = &beagle_mt9p031_platform_data,
> +};
> +
> +static struct isp_subdev_i2c_board_info mt9p031_camera_subdevs[] = {
> +	{
> +		.board_info = &mt9p031_camera_i2c_device,
> +		.i2c_adapter_id = 2,
> +	},
> +	{ NULL, 0, },
> +};
> +
> +static struct isp_v4l2_subdevs_group beagle_camera_subdevs[] = {
> +	{
> +		.subdevs = mt9p031_camera_subdevs,
> +		.interface = ISP_INTERFACE_PARALLEL,
> +		.bus = {
> +			.parallel = {
> +				.data_lane_shift = 0,
> +				.clk_pol = 1,
> +				.bridge = ISPCTRL_PAR_BRIDGE_DISABLE,
> +			}
> +		},
> +	},
> +	{ },
> +};
> +
> +static struct isp_platform_data beagle_isp_platform_data = {
> +	.subdevs = beagle_camera_subdevs,
> +};
> +
>  static void __init omap3_beagle_init(void)
>  {
>  	omap3_mux_init(board_mux, OMAP_PACKAGE_CBB);
> @@ -679,6 +793,11 @@ static void __init omap3_beagle_init(void)
>  
>  	beagle_display_init();
>  	beagle_opp_init();
> +
> +	/* Enable camera */
> +	gpio_request(MT9P031_RESET_GPIO, "cam_rst");
> +	gpio_direction_output(MT9P031_RESET_GPIO, 0);

gpio_request_one()?

> +	omap3_init_camera(&beagle_isp_platform_data);
>  }
>  
>  MACHINE_START(OMAP3_BEAGLE, "OMAP3 Beagle Board")
  
Javier Martin May 23, 2011, 7:01 a.m. UTC | #3
On 20 May 2011 17:57, Koen Kooi <koen@beagleboard.org> wrote:
> In previous patch sets we put that in a seperate file (omap3beagle-camera.c) so we don't clutter up the board file with all the different sensor drivers. Would it make sense to do the same with the current patches? It looks like MCF cuts down a lot on the boilerplace needed already.

I sent my first patch using that approach but I was told to move it to
the board code.
Please, don't make undo the changes. Or at least, let's discuss this
seriously so that we all agree on what is the best way of doing it and
I don't have to change it every time.
  
Javier Martin May 23, 2011, 7:25 a.m. UTC | #4
On 22 May 2011 15:49, Igor Grinberg <grinberg@compulab.co.il> wrote:
> Hi Javier,
>
>
> linux-omap should be CC'ed - added.
>
> In addition to Koen's comments, some comments below.
>
>
> On 05/20/11 16:47, Javier Martin wrote:
>
>> isp.h file has to be included as a temporal measure
>> since clocks of the isp are not exposed yet.
>>
>> Signed-off-by: Javier Martin <javier.martin@vista-silicon.com>
>> ---
>>  arch/arm/mach-omap2/board-omap3beagle.c |  127 ++++++++++++++++++++++++++++++-
>>  1 files changed, 123 insertions(+), 4 deletions(-)
>>
>> diff --git a/arch/arm/mach-omap2/board-omap3beagle.c b/arch/arm/mach-omap2/board-omap3beagle.c
>> index 33007fd..f52e6ae 100644
>> --- a/arch/arm/mach-omap2/board-omap3beagle.c
>> +++ b/arch/arm/mach-omap2/board-omap3beagle.c
>> @@ -24,15 +24,20 @@
>>  #include <linux/input.h>
>>  #include <linux/gpio_keys.h>
>>  #include <linux/opp.h>
>> +#include <linux/i2c.h>
>> +#include <linux/mm.h>
>> +#include <linux/videodev2.h>
>>
>>  #include <linux/mtd/mtd.h>
>>  #include <linux/mtd/partitions.h>
>>  #include <linux/mtd/nand.h>
>>  #include <linux/mmc/host.h>
>> -
>> +#include <linux/gpio.h>
>
> Why include this for second time?

Good catch, I'll fix it.

[snip]
>> @@ -309,6 +357,15 @@ static int beagle_twl_gpio_setup(struct device *dev,
>>                       pr_err("%s: unable to configure EHCI_nOC\n", __func__);
>>       }
>>
>> +     if (omap3_beagle_get_rev() == OMAP3BEAGLE_BOARD_XM) {
>> +             /*
>> +              * Power on camera interface - only on pre-production, not
>> +              * needed on production boards
>> +              */
>> +             gpio_request(gpio + 2, "CAM_EN");
>> +             gpio_direction_output(gpio + 2, 1);
>
> Why not gpio_request_one()?

Just to follow the same approach as in the rest of the code.
Maybe a further patch could change all "gpio_request()" uses to
"gpio_request_one()".

>
>> +     }
>> +
>>       /*
>>        * TWL4030_GPIO_MAX + 0 == ledA, EHCI nEN_USB_PWR (out, XM active
>>        * high / others active low)
>> @@ -451,6 +508,8 @@ static struct twl4030_platform_data beagle_twldata = {
>>       .vsim           = &beagle_vsim,
>>       .vdac           = &beagle_vdac,
>>       .vpll2          = &beagle_vpll2,
>> +     .vaux3          = &beagle_vaux3,
>> +     .vaux4          = &beagle_vaux4,
>>  };
>>
>>  static struct i2c_board_info __initdata beagle_i2c_boardinfo[] = {
>> @@ -463,15 +522,16 @@ static struct i2c_board_info __initdata beagle_i2c_boardinfo[] = {
>>  };
>>
>>  static struct i2c_board_info __initdata beagle_i2c_eeprom[] = {
>> -       {
>> -               I2C_BOARD_INFO("eeprom", 0x50),
>> -       },
>> +     {
>> +             I2C_BOARD_INFO("eeprom", 0x50),
>> +     },
>>  };
>
> This part of the hunk is not related to the patch
> and should be in a separate (cleanup) patch.
>

Sure, I'll fix it.

>>
>>  static int __init omap3_beagle_i2c_init(void)
>>  {
>>       omap_register_i2c_bus(1, 2600, beagle_i2c_boardinfo,
>>                       ARRAY_SIZE(beagle_i2c_boardinfo));
>> +     omap_register_i2c_bus(2, 100, NULL, 0); /* Enable I2C2 for camera */
>>       /* Bus 3 is attached to the DVI port where devices like the pico DLP
>>        * projector don't work reliably with 400kHz */
>>       omap_register_i2c_bus(3, 100, beagle_i2c_eeprom, ARRAY_SIZE(beagle_i2c_eeprom));
>> @@ -654,6 +714,60 @@ static void __init beagle_opp_init(void)
>>       return;
>>  }
>>
>> +static int beagle_cam_set_xclk(struct v4l2_subdev *subdev, int hz)
>> +{
>> +     struct isp_device *isp = v4l2_dev_to_isp_device(subdev->v4l2_dev);
>> +     int ret;
>> +
>> +     ret = isp->platform_cb.set_xclk(isp, hz, MT9P031_XCLK);
>> +     return 0;
>> +}
>
> Why do you need ret variable here, if you always return zero?
>

You are right, it's not needed any longer.


>> +
>> +static int beagle_cam_reset(struct v4l2_subdev *subdev, int active)
>> +{
>> +     /* Set RESET_BAR to !active */
>> +     gpio_set_value(MT9P031_RESET_GPIO, !active);
>> +
>> +     return 0;
>> +}
>> +
>> +static struct mt9p031_platform_data beagle_mt9p031_platform_data = {
>> +     .set_xclk               = beagle_cam_set_xclk,
>> +     .reset                  = beagle_cam_reset,
>> +};
>> +
>> +static struct i2c_board_info mt9p031_camera_i2c_device = {
>> +     I2C_BOARD_INFO("mt9p031", 0x48),
>> +     .platform_data = &beagle_mt9p031_platform_data,
>> +};
>> +
>> +static struct isp_subdev_i2c_board_info mt9p031_camera_subdevs[] = {
>> +     {
>> +             .board_info = &mt9p031_camera_i2c_device,
>> +             .i2c_adapter_id = 2,
>> +     },
>> +     { NULL, 0, },
>> +};
>> +
>> +static struct isp_v4l2_subdevs_group beagle_camera_subdevs[] = {
>> +     {
>> +             .subdevs = mt9p031_camera_subdevs,
>> +             .interface = ISP_INTERFACE_PARALLEL,
>> +             .bus = {
>> +                     .parallel = {
>> +                             .data_lane_shift = 0,
>> +                             .clk_pol = 1,
>> +                             .bridge = ISPCTRL_PAR_BRIDGE_DISABLE,
>> +                     }
>> +             },
>> +     },
>> +     { },
>> +};
>> +
>> +static struct isp_platform_data beagle_isp_platform_data = {
>> +     .subdevs = beagle_camera_subdevs,
>> +};
>> +
>>  static void __init omap3_beagle_init(void)
>>  {
>>       omap3_mux_init(board_mux, OMAP_PACKAGE_CBB);
>> @@ -679,6 +793,11 @@ static void __init omap3_beagle_init(void)
>>
>>       beagle_display_init();
>>       beagle_opp_init();
>> +
>> +     /* Enable camera */
>> +     gpio_request(MT9P031_RESET_GPIO, "cam_rst");
>> +     gpio_direction_output(MT9P031_RESET_GPIO, 0);
>
> gpio_request_one()?

ditto

>> +     omap3_init_camera(&beagle_isp_platform_data);
>>  }
>>
>>  MACHINE_START(OMAP3_BEAGLE, "OMAP3 Beagle Board")
>
>
>
> --
> Regards,
> Igor.
>
>
  
Laurent Pinchart May 23, 2011, 7:47 a.m. UTC | #5
Hi Javier,

On Monday 23 May 2011 09:25:01 javier Martin wrote:
> On 22 May 2011 15:49, Igor Grinberg <grinberg@compulab.co.il> wrote:

[snip]

> >> @@ -309,6 +357,15 @@ static int beagle_twl_gpio_setup(struct device
> >> *dev, pr_err("%s: unable to configure EHCI_nOC\n", __func__); }
> >> 
> >> +     if (omap3_beagle_get_rev() == OMAP3BEAGLE_BOARD_XM) {
> >> +             /*
> >> +              * Power on camera interface - only on pre-production, not
> >> +              * needed on production boards
> >> +              */
> >> +             gpio_request(gpio + 2, "CAM_EN");
> >> +             gpio_direction_output(gpio + 2, 1);
> > 
> > Why not gpio_request_one()?
> 
> Just to follow the same approach as in the rest of the code.
> Maybe a further patch could change all "gpio_request()" uses to
> "gpio_request_one()".

Please do it the other way around. Replace calls to gpio_request() + 
gpio_direction_output() with a call to gpio_request_one(), and then modify 
this patch to use gpio_request_one().

If you want to learn how to use coccinelle (http://coccinelle.lip6.fr/), now 
would be a good time. You could use it to replace gpio_request() + 
gpio_direction_output() through the whole kernel.
  
Laurent Pinchart May 23, 2011, 8 a.m. UTC | #6
Hi Javier,

On Monday 23 May 2011 09:01:07 javier Martin wrote:
> On 20 May 2011 17:57, Koen Kooi <koen@beagleboard.org> wrote:
> > In previous patch sets we put that in a seperate file
> > (omap3beagle-camera.c) so we don't clutter up the board file with all
> > the different sensor drivers. Would it make sense to do the same with
> > the current patches? It looks like MCF cuts down a lot on the
> > boilerplace needed already.
> 
> I sent my first patch using that approach but I was told to move it to
> the board code.
> Please, don't make undo the changes. Or at least, let's discuss this
> seriously so that we all agree on what is the best way of doing it and
> I don't have to change it every time.

What we really need here is a modular way to support sensors on pluggable 
expansion boards. Not all Beagleboard users will have an MT9P031 connected to 
the OMAP3 ISP, so that must not be hardcoded in board code. As the sensor 
boards are not runtime detectable, one solution would be to compile part of 
the code as a module. Regulator definitions and I2C2 bus registration (and 
possibly GPIO initialization) can be left in board code.
  
Koen Kooi May 23, 2011, 8:55 a.m. UTC | #7
Op 23 mei 2011, om 10:00 heeft Laurent Pinchart het volgende geschreven:

> Hi Javier,
> 
> On Monday 23 May 2011 09:01:07 javier Martin wrote:
>> On 20 May 2011 17:57, Koen Kooi <koen@beagleboard.org> wrote:
>>> In previous patch sets we put that in a seperate file
>>> (omap3beagle-camera.c) so we don't clutter up the board file with all
>>> the different sensor drivers. Would it make sense to do the same with
>>> the current patches? It looks like MCF cuts down a lot on the
>>> boilerplace needed already.
>> 
>> I sent my first patch using that approach but I was told to move it to
>> the board code.
>> Please, don't make undo the changes. Or at least, let's discuss this
>> seriously so that we all agree on what is the best way of doing it and
>> I don't have to change it every time.
> 
> What we really need here is a modular way to support sensors on pluggable 
> expansion boards. Not all Beagleboard users will have an MT9P031 connected to 
> the OMAP3 ISP, so that must not be hardcoded in board code. As the sensor 
> boards are not runtime detectable

Well, they are runtime detectable, you just need to read the ID register on the sensor and they all share the same I2C address. Once you have the sensor ID you can (re)setup the I2C. But doing that in linux seems to be impossible with the current I2C infrastructure.

What we (beagleboard.org) are doing now:

1) set a bootarg in uboot e.g. camera=llbcm5mp
2) read bootarg in linux boardfile and setup i2c

What we are going to do medium term:

1) read ID in uboot, set bootarg
2) read bootarg in linux boardfile

Long term 1) will probably do some devicetree magic. The goal is to plug in a sensor and boot, no manual modprobing, it just works.

regards,

Koen--
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
  
Laurent Pinchart May 23, 2011, 9:14 a.m. UTC | #8
Hi Koen,

On Monday 23 May 2011 10:55:53 Koen Kooi wrote:
> Op 23 mei 2011, om 10:00 heeft Laurent Pinchart het volgende geschreven:
> > On Monday 23 May 2011 09:01:07 javier Martin wrote:
> >> On 20 May 2011 17:57, Koen Kooi <koen@beagleboard.org> wrote:
> >>> In previous patch sets we put that in a seperate file
> >>> (omap3beagle-camera.c) so we don't clutter up the board file with all
> >>> the different sensor drivers. Would it make sense to do the same with
> >>> the current patches? It looks like MCF cuts down a lot on the
> >>> boilerplace needed already.
> >> 
> >> I sent my first patch using that approach but I was told to move it to
> >> the board code.
> >> Please, don't make undo the changes. Or at least, let's discuss this
> >> seriously so that we all agree on what is the best way of doing it and
> >> I don't have to change it every time.
> > 
> > What we really need here is a modular way to support sensors on pluggable
> > expansion boards. Not all Beagleboard users will have an MT9P031
> > connected to the OMAP3 ISP, so that must not be hardcoded in board code.
> > As the sensor boards are not runtime detectable
> 
> Well, they are runtime detectable, you just need to read the ID register on
> the sensor and they all share the same I2C address. Once you have the
> sensor ID you can (re)setup the I2C.

I don't think we can guarantee that all sensor boards that will ever be 
plugged into the Beagleboard will have a sensor ID register readable from a 
single I2C address at a single register offset.

> But doing that in linux seems to be impossible with the current I2C
> infrastructure.
> 
> What we (beagleboard.org) are doing now:
> 
> 1) set a bootarg in uboot e.g. camera=llbcm5mp
> 2) read bootarg in linux boardfile and setup i2c
> 
> What we are going to do medium term:
> 
> 1) read ID in uboot, set bootarg
> 2) read bootarg in linux boardfile
> 
> Long term 1) will probably do some devicetree magic. The goal is to plug in
> a sensor and boot, no manual modprobing, it just works.

Device tree is definitely the way to go. using the camera parameter in board 
code to register the correct camera sounds good to me.
  
Igor Grinberg May 23, 2011, 5:03 p.m. UTC | #9
On 05/23/11 10:47, Laurent Pinchart wrote:
> Hi Javier,
>
> On Monday 23 May 2011 09:25:01 javier Martin wrote:
>> On 22 May 2011 15:49, Igor Grinberg <grinberg@compulab.co.il> wrote:
> [snip]
>
>>>> @@ -309,6 +357,15 @@ static int beagle_twl_gpio_setup(struct device
>>>> *dev, pr_err("%s: unable to configure EHCI_nOC\n", __func__); }
>>>>
>>>> +     if (omap3_beagle_get_rev() == OMAP3BEAGLE_BOARD_XM) {
>>>> +             /*
>>>> +              * Power on camera interface - only on pre-production, not
>>>> +              * needed on production boards
>>>> +              */
>>>> +             gpio_request(gpio + 2, "CAM_EN");
>>>> +             gpio_direction_output(gpio + 2, 1);
>>> Why not gpio_request_one()?
>> Just to follow the same approach as in the rest of the code.
>> Maybe a further patch could change all "gpio_request()" uses to
>> "gpio_request_one()".
> Please do it the other way around. Replace calls to gpio_request() + 
> gpio_direction_output() with a call to gpio_request_one(), and then modify 
> this patch to use gpio_request_one().

Well, this is done already, you need to follow Tony's linux-next branch...
So, just changing this patch would do...
Also, good practice is to base patches on maintainer's appropriate branch,
so it would be easier to apply.
  

Patch

diff --git a/arch/arm/mach-omap2/board-omap3beagle.c b/arch/arm/mach-omap2/board-omap3beagle.c
index 33007fd..f52e6ae 100644
--- a/arch/arm/mach-omap2/board-omap3beagle.c
+++ b/arch/arm/mach-omap2/board-omap3beagle.c
@@ -24,15 +24,20 @@ 
 #include <linux/input.h>
 #include <linux/gpio_keys.h>
 #include <linux/opp.h>
+#include <linux/i2c.h>
+#include <linux/mm.h>
+#include <linux/videodev2.h>
 
 #include <linux/mtd/mtd.h>
 #include <linux/mtd/partitions.h>
 #include <linux/mtd/nand.h>
 #include <linux/mmc/host.h>
-
+#include <linux/gpio.h>
 #include <linux/regulator/machine.h>
 #include <linux/i2c/twl.h>
 
+#include <media/mt9p031.h>
+
 #include <mach/hardware.h>
 #include <asm/mach-types.h>
 #include <asm/mach/arch.h>
@@ -47,12 +52,17 @@ 
 #include <plat/nand.h>
 #include <plat/usb.h>
 #include <plat/omap_device.h>
+#include <plat/i2c.h>
 
 #include "mux.h"
 #include "hsmmc.h"
 #include "timer-gp.h"
 #include "pm.h"
+#include "devices.h"
+#include "../../../drivers/media/video/omap3isp/isp.h"
 
+#define MT9P031_RESET_GPIO	98
+#define MT9P031_XCLK		ISP_XCLK_A
 #define NAND_BLOCK_SIZE		SZ_128K
 
 /*
@@ -273,6 +283,44 @@  static struct regulator_consumer_supply beagle_vsim_supply = {
 
 static struct gpio_led gpio_leds[];
 
+static struct regulator_consumer_supply beagle_vaux3_supply = {
+	.supply		= "cam_1v8",
+};
+
+static struct regulator_consumer_supply beagle_vaux4_supply = {
+	.supply		= "cam_2v8",
+};
+
+/* VAUX3 for CAM_1V8 */
+static struct regulator_init_data beagle_vaux3 = {
+	.constraints = {
+		.min_uV			= 1800000,
+		.max_uV			= 1800000,
+		.apply_uV		= true,
+		.valid_modes_mask	= REGULATOR_MODE_NORMAL
+					| REGULATOR_MODE_STANDBY,
+		.valid_ops_mask		= REGULATOR_CHANGE_MODE
+					| REGULATOR_CHANGE_STATUS,
+	},
+	.num_consumer_supplies	= 1,
+	.consumer_supplies	= &beagle_vaux3_supply,
+};
+
+/* VAUX4 for CAM_2V8 */
+static struct regulator_init_data beagle_vaux4 = {
+	.constraints = {
+		.min_uV			= 1800000,
+		.max_uV			= 1800000,
+		.apply_uV		= true,
+		.valid_modes_mask	= REGULATOR_MODE_NORMAL
+					| REGULATOR_MODE_STANDBY,
+		.valid_ops_mask		= REGULATOR_CHANGE_MODE
+					| REGULATOR_CHANGE_STATUS,
+	},
+	.num_consumer_supplies	= 1,
+	.consumer_supplies	= &beagle_vaux4_supply,
+};
+
 static int beagle_twl_gpio_setup(struct device *dev,
 		unsigned gpio, unsigned ngpio)
 {
@@ -309,6 +357,15 @@  static int beagle_twl_gpio_setup(struct device *dev,
 			pr_err("%s: unable to configure EHCI_nOC\n", __func__);
 	}
 
+	if (omap3_beagle_get_rev() == OMAP3BEAGLE_BOARD_XM) {
+		/*
+		 * Power on camera interface - only on pre-production, not
+		 * needed on production boards
+		 */
+		gpio_request(gpio + 2, "CAM_EN");
+		gpio_direction_output(gpio + 2, 1);
+	}
+
 	/*
 	 * TWL4030_GPIO_MAX + 0 == ledA, EHCI nEN_USB_PWR (out, XM active
 	 * high / others active low)
@@ -451,6 +508,8 @@  static struct twl4030_platform_data beagle_twldata = {
 	.vsim		= &beagle_vsim,
 	.vdac		= &beagle_vdac,
 	.vpll2		= &beagle_vpll2,
+	.vaux3		= &beagle_vaux3,
+	.vaux4		= &beagle_vaux4,
 };
 
 static struct i2c_board_info __initdata beagle_i2c_boardinfo[] = {
@@ -463,15 +522,16 @@  static struct i2c_board_info __initdata beagle_i2c_boardinfo[] = {
 };
 
 static struct i2c_board_info __initdata beagle_i2c_eeprom[] = {
-       {
-               I2C_BOARD_INFO("eeprom", 0x50),
-       },
+	{
+		I2C_BOARD_INFO("eeprom", 0x50),
+	},
 };
 
 static int __init omap3_beagle_i2c_init(void)
 {
 	omap_register_i2c_bus(1, 2600, beagle_i2c_boardinfo,
 			ARRAY_SIZE(beagle_i2c_boardinfo));
+	omap_register_i2c_bus(2, 100, NULL, 0); /* Enable I2C2 for camera */
 	/* Bus 3 is attached to the DVI port where devices like the pico DLP
 	 * projector don't work reliably with 400kHz */
 	omap_register_i2c_bus(3, 100, beagle_i2c_eeprom, ARRAY_SIZE(beagle_i2c_eeprom));
@@ -654,6 +714,60 @@  static void __init beagle_opp_init(void)
 	return;
 }
 
+static int beagle_cam_set_xclk(struct v4l2_subdev *subdev, int hz)
+{
+	struct isp_device *isp = v4l2_dev_to_isp_device(subdev->v4l2_dev);
+	int ret;
+
+	ret = isp->platform_cb.set_xclk(isp, hz, MT9P031_XCLK);
+	return 0;
+}
+
+static int beagle_cam_reset(struct v4l2_subdev *subdev, int active)
+{
+	/* Set RESET_BAR to !active */
+	gpio_set_value(MT9P031_RESET_GPIO, !active);
+
+	return 0;
+}
+
+static struct mt9p031_platform_data beagle_mt9p031_platform_data = {
+	.set_xclk		= beagle_cam_set_xclk,
+	.reset			= beagle_cam_reset,
+};
+
+static struct i2c_board_info mt9p031_camera_i2c_device = {
+	I2C_BOARD_INFO("mt9p031", 0x48),
+	.platform_data = &beagle_mt9p031_platform_data,
+};
+
+static struct isp_subdev_i2c_board_info mt9p031_camera_subdevs[] = {
+	{
+		.board_info = &mt9p031_camera_i2c_device,
+		.i2c_adapter_id = 2,
+	},
+	{ NULL, 0, },
+};
+
+static struct isp_v4l2_subdevs_group beagle_camera_subdevs[] = {
+	{
+		.subdevs = mt9p031_camera_subdevs,
+		.interface = ISP_INTERFACE_PARALLEL,
+		.bus = {
+			.parallel = {
+				.data_lane_shift = 0,
+				.clk_pol = 1,
+				.bridge = ISPCTRL_PAR_BRIDGE_DISABLE,
+			}
+		},
+	},
+	{ },
+};
+
+static struct isp_platform_data beagle_isp_platform_data = {
+	.subdevs = beagle_camera_subdevs,
+};
+
 static void __init omap3_beagle_init(void)
 {
 	omap3_mux_init(board_mux, OMAP_PACKAGE_CBB);
@@ -679,6 +793,11 @@  static void __init omap3_beagle_init(void)
 
 	beagle_display_init();
 	beagle_opp_init();
+
+	/* Enable camera */
+	gpio_request(MT9P031_RESET_GPIO, "cam_rst");
+	gpio_direction_output(MT9P031_RESET_GPIO, 0);
+	omap3_init_camera(&beagle_isp_platform_data);
 }
 
 MACHINE_START(OMAP3_BEAGLE, "OMAP3 Beagle Board")