Re: [RFC 17/17] rm680: Add camera init

[Sakari Ailus <sakari.ailus@maxwell.research.nokia.com>]

Hi Laurent,

Thanks for the review!

Laurent Pinchart wrote:
> On Tuesday 20 December 2011 21:28:09 Sakari Ailus wrote:
>> This currently introduces an extra file to the arch/arm/mach-omap2
>> directory: board-rm680-camera.c. Keeping the device tree in mind, the
>> context of the file could be represented as static data with one exception:
>> the external clock to the sensor.
>>
>> This external clock is provided by the OMAP 3 SoC and required by the
>> sensor. The issue is that the clock originates from the ISP and not from
>> PRCM block as the other clocks and thus is not supported by the clock
>> framework. Otherwise the sensor driver could just clk_get() and
>> clk_enable() it, just like the regulators and gpios.
> 
> This will hopefully be fixable with the new generic clock struct. Have you had 
> a look at it lately BTW ?

Not since the ELc-E. It'd be nice to get that so we can get rid of the
final issue between OMAP 3 ISP and the device tree --- well, there may
be others but I can't say to know about them. :-)

>> Signed-off-by: Sakari Ailus <sakari.ailus@maxwell.research.nokia.com>
>> ---
>>  arch/arm/mach-omap2/Makefile             |    3 +-
>>  arch/arm/mach-omap2/board-rm680-camera.c |  408
>> ++++++++++++++++++++++++++++++ arch/arm/mach-omap2/board-rm680.c        | 
>>  42 +++
>>  3 files changed, 452 insertions(+), 1 deletions(-)
>>  create mode 100644 arch/arm/mach-omap2/board-rm680-camera.c
>>
>> diff --git a/arch/arm/mach-omap2/Makefile b/arch/arm/mach-omap2/Makefile
>> index 69ab1c0..1444bc5 100644
>> --- a/arch/arm/mach-omap2/Makefile
>> +++ b/arch/arm/mach-omap2/Makefile
>> @@ -201,7 +201,8 @@ obj-$(CONFIG_MACH_OMAP3_PANDORA)	+=
>> board-omap3pandora.o obj-$(CONFIG_MACH_OMAP_3430SDP)		+= board-3430sdp.o
>>  obj-$(CONFIG_MACH_NOKIA_N8X0)		+= board-n8x0.o
>>  obj-$(CONFIG_MACH_NOKIA_RM680)		+= board-rm680.o \
>> -					   sdram-nokia.o
>> +					   sdram-nokia.o \
>> +					   board-rm680-camera.o
>>  obj-$(CONFIG_MACH_NOKIA_RX51)		+= board-rx51.o \
>>  					   sdram-nokia.o \
>>  					   board-rx51-peripherals.o \
>> diff --git a/arch/arm/mach-omap2/board-rm680-camera.c
>> b/arch/arm/mach-omap2/board-rm680-camera.c new file mode 100644
>> index 0000000..4cc1ced
>> --- /dev/null
>> +++ b/arch/arm/mach-omap2/board-rm680-camera.c
>> @@ -0,0 +1,408 @@
>> +/**
>> + * arch/arm/mach-omap2/board-rm680-camera.c
>> + *
>> + * Copyright (C) 2010--2011 Nokia Corporation
> 
> 2012 ? :-)

Good point. I'll make the change for the rest of the files as well.

>> + * Contact: Sakari Ailus <sakari.ailus@maxwell.research.nokia.com>
>> + *
>> + * Based on board-rx71-camera.c by Vimarsh Zutshi
> 
> This one isn't upstream, I'm not sure if it's worth mentioning it.

It's still of historical importance; that is still publicly available if
not in mainline. If you really think I should remove it I will.

>> + * Based on board-rx51-camera.c by Sakari Ailus
>> + *
>> + * This program is free software; you can redistribute it and/or
>> + * modify it under the terms of the GNU General Public License
>> + * version 2 as published by the Free Software Foundation.
>> + *
>> + * This program is distributed in the hope that it will be useful, but
>> + * WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
>> + * General Public License for more details.
>> + *
>> + * You should have received a copy of the GNU General Public License
>> + * along with this program; if not, write to the Free Software
>> + * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA
>> + * 02110-1301 USA
>> + *
>> + */
>> +
>> +#include <linux/delay.h>
>> +#include <linux/mm.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/videodev2.h>
>> +
>> +#include <linux/gpio.h>
>> +#include <plat/omap-pm.h>
>> +
>> +#include "../../../drivers/media/video/omap3isp/isp.h"
> 
> What do we still miss in <media/omap3isp.h> ?

I think it's mostly the xclk stuff. Everything else is where it should be.

>> +#include <media/omap3isp.h>
>> +#include <media/smiapp.h>
>> +#include <asm/mach-types.h>
>> +
>> +#include "../../../drivers/media/video/smiapp.h"
> 
> Time to create <media/smiapp.h> ?

Now that you ask; the file is there and it contains all the necessary
definitions. I had forgotten to remove the line. ;-)

>> +
>> +#include "devices.h"
>> +
>> +#define SEC_CAMERA_RESET_GPIO	97
>> +
>> +#define RM680_PRI_SENSOR	1
>> +#define RM680_PRI_LENS		2
>> +#define RM680_SEC_SENSOR	3
>> +#define MAIN_CAMERA_XCLK	ISP_XCLK_A
>> +#define SEC_CAMERA_XCLK		ISP_XCLK_B
>> +
>> +/*
>> + *
>> + * HW initialization
>> + *
>> + *
>> + */
>> +static int __init rm680_sec_camera_init(void)
>> +{
>> +	if (gpio_request(SEC_CAMERA_RESET_GPIO, "sec_camera reset") != 0) {
>> +		printk(KERN_INFO "%s: unable to acquire secondary "
>> +		       "camera reset gpio\n", __func__);
>> +		return -ENODEV;
>> +	}
>> +
>> +	/* XSHUTDOWN off, reset  */
>> +	gpio_direction_output(SEC_CAMERA_RESET_GPIO, 0);
>> +	gpio_set_value(SEC_CAMERA_RESET_GPIO, 0);
> 
> gpio_request_one() would be helpful here (and possibly in other locations in 
> this file).

Moved to smiapp-core.c as suggested by Sylwester.

>> +
>> +	return 0;
>> +}
>> +
>> +static int __init rm680_camera_hw_init(void)
>> +{
>> +	return rm680_sec_camera_init();
> 
> Maybe you could merge the two functions ?

Removed.

>> +}
>> +
>> +/*
>> + *
>> + * Main Camera Module EXTCLK
>> + * Used by the sensor and the actuator driver.
>> + *
>> + */
>> +static struct camera_xclk {
>> +	u32 hz;
>> +	u32 lock;
>> +	u8 xclksel;
>> +} cameras_xclk;
>> +
>> +static DEFINE_MUTEX(lock_xclk);
>> +
>> +static int rm680_update_xclk(struct v4l2_subdev *subdev, u32 hz, u32
>> which, +			     u8 xclksel)
>> +{
>> +	struct isp_device *isp = v4l2_dev_to_isp_device(subdev->v4l2_dev);
>> +	int ret;
>> +
>> +	mutex_lock(&lock_xclk);
>> +
>> +	if (which == RM680_SEC_SENSOR) {
>> +		if (cameras_xclk.xclksel == MAIN_CAMERA_XCLK) {
>> +			ret = -EBUSY;
>> +			goto done;
>> +		}
>> +	} else {
>> +		if (cameras_xclk.xclksel == SEC_CAMERA_XCLK) {
>> +			ret = -EBUSY;
>> +			goto done;
>> +		}
>> +	}
>> +
>> +	if (hz) {	/* Turn on */
>> +		cameras_xclk.lock |= which;
>> +		if (cameras_xclk.hz == 0) {
>> +			isp->platform_cb.set_xclk(isp, hz, xclksel);
>> +			cameras_xclk.hz = hz;
>> +			cameras_xclk.xclksel = xclksel;
>> +		}
>> +	} else {	/* Turn off */
>> +		cameras_xclk.lock &= ~which;
>> +		if (cameras_xclk.lock == 0) {
>> +			isp->platform_cb.set_xclk(isp, 0, xclksel);
>> +			cameras_xclk.hz = 0;
>> +			cameras_xclk.xclksel = 0;
>> +		}
>> +	}
>> +
>> +	ret = cameras_xclk.hz;
>> +
>> +done:
>> +	mutex_unlock(&lock_xclk);
>> +	return ret;
>> +}
>> +
>> +/*
>> + *
>> + * Main Camera Sensor
>> + *
>> + */
>> +
>> +static struct isp_csiphy_lanes_cfg rm696_main_camera_csi2_lanecfg = {
>> +	.clk = {
>> +		.pol = 1,
>> +		.pos = 2,
>> +	},
>> +	.data[0] = {
>> +		.pol = 1,
>> +		.pos = 1,
>> +	},
>> +	.data[1] = {
>> +		.pol = 1,
>> +		.pos = 3,
>> +	},
>> +};
>> +
>> +static struct isp_csiphy_lanes_cfg rm680_main_camera_csi2_lanecfg = {
>> +	.clk = {
>> +		.pol = 1,
>> +		.pos = 2,
>> +	},
>> +	.data[0] = {
>> +		.pol = 1,
>> +		.pos = 3,
>> +	},
>> +	.data[1] = {
>> +		.pol = 1,
>> +		.pos = 1,
>> +	},
>> +};
>> +
>> +static int rm680_main_camera_set_xclk(struct v4l2_subdev *sd, int hz)
>> +{
>> +	return rm680_update_xclk(sd, hz, RM680_PRI_SENSOR, MAIN_CAMERA_XCLK);
>> +}
>> +
>> +static struct smiapp_flash_strobe_parms rm680_main_camera_strobe_setup = {
>> +	.mode			= 0x0c,
>> +	.strobe_width_high_us	= 100000,
>> +	.strobe_delay		= 0,
>> +	.stobe_start_point	= 0,
>> +	.trigger		= 0,
>> +};
>> +
>> +static struct smiapp_platform_data rm696_main_camera_platform_data = {
>> +	.i2c_addr_dfl		= SMIAPP_DFL_I2C_ADDR,
>> +	.i2c_addr_alt		= SMIAPP_ALT_I2C_ADDR,
>> +	.nvm_size		= 16 * 64,
>> +	.ext_clk		= (9.6 * 1000 * 1000),
>> +	.lanes			= 2,
>> +	.op_sys_clock		= (s64 []){ 796800 / 2, 840000 / 2,
>> +					    1996800 / 2, 0 },
>> +	.csi_signalling_mode	= SMIAPP_CSI_SIGNALLING_MODE_CSI2,
>> +	.strobe_setup		= &rm680_main_camera_strobe_setup,
>> +	.set_xclk		= rm680_main_camera_set_xclk,
>> +};
>> +
>> +static struct smiapp_platform_data rm680_main_camera_platform_data = {
>> +	.i2c_addr_dfl		= SMIAPP_DFL_I2C_ADDR,
>> +	.i2c_addr_alt		= SMIAPP_ALT_I2C_ADDR,
>> +	.nvm_size		= 16 * 64,
>> +	.ext_clk		= (9.6 * 1000 * 1000),
>> +	.lanes			= 2,
>> +	.op_sys_clock		= (s64 []){ 840000 / 2, 1334400 / 2,
>> +					    1593600 / 2, 0 },
>> +	.csi_signalling_mode	= SMIAPP_CSI_SIGNALLING_MODE_CSI2,
>> +	.module_board_orient	= SMIAPP_MODULE_BOARD_ORIENT_180,
>> +	.strobe_setup		= &rm680_main_camera_strobe_setup,
>> +	.set_xclk		= rm680_main_camera_set_xclk,
>> +};
>> +
>> +/*
>> + *
>> + * SECONDARY CAMERA Sensor
>> + *
>> + */
>> +
>> +#define SEC_CAMERA_XCLK		ISP_XCLK_B
>> +
>> +static struct isp_csiphy_lanes_cfg rm680_sec_camera_csiphy_lanecfg = {
>> +	.clk = {
>> +		.pol = 0,
>> +		.pos = 1,
>> +	},
>> +	.data[0] = {
>> +		.pol = 0,
>> +		.pos = 2,
>> +	},
>> +};
>> +
>> +static int rm680_sec_camera_set_xclk(struct v4l2_subdev *sd, int hz)
>> +{
>> +	return rm680_update_xclk(sd, hz, RM680_SEC_SENSOR, SEC_CAMERA_XCLK);
>> +}
>> +
>> +static int rm680_sec_camera_set_xshutdown(struct v4l2_subdev *subdev, u8
>> set) +{
>> +	gpio_set_value(SEC_CAMERA_RESET_GPIO, !!set);
>> +	return 0;
>> +}
>> +
>> +static struct smiapp_platform_data rm696_sec_camera_platform_data = {
>> +	.ext_clk		= (10.8 * 1000 * 1000),
>> +	.lanes			= 1,
>> +	.op_sys_clock		= (s64 []){ 13770 * 10, 0 },
>> +	.csi_signalling_mode	= SMIAPP_CSI_SIGNALLING_MODE_CCP2_DATA_CLOCK,
>> +	.module_board_orient	= SMIAPP_MODULE_BOARD_ORIENT_180,
>> +	.set_xclk		= rm680_sec_camera_set_xclk,
>> +	.set_xshutdown		= rm680_sec_camera_set_xshutdown,
>> +};
>> +
>> +static struct smiapp_platform_data rm680_sec_camera_platform_data = {
>> +	.ext_clk		= (10.8 * 1000 * 1000),
>> +	.lanes			= 1,
>> +	.op_sys_clock		= (s64 []){ 11880 * 10, 0 },
>> +	.csi_signalling_mode	= SMIAPP_CSI_SIGNALLING_MODE_CCP2_DATA_CLOCK,
>> +	.set_xclk		= rm680_sec_camera_set_xclk,
>> +	.set_xshutdown		= rm680_sec_camera_set_xshutdown,
> 
> What about passing the GPIO number to the smiapp driver instead of using a 
> callback function ?

Done.

>> +};
>> +
>> +/*
>> + *
>> + * Init all the modules
>> + *
>> + */
>> +
>> +#define CAMERA_I2C_BUS_NUM		2
>> +#define AD5836_I2C_BUS_NUM		2
>> +#define AS3645A_I2C_BUS_NUM		2
>> +
>> +static struct i2c_board_info rm696_camera_i2c_devices[] = {
>> +	{
>> +		I2C_BOARD_INFO(SMIAPP_NAME, SMIAPP_ALT_I2C_ADDR),
>> +		.platform_data = &rm696_main_camera_platform_data,
>> +	},
>> +	{
>> +		I2C_BOARD_INFO(SMIAPP_NAME, SMIAPP_DFL_I2C_ADDR),
>> +		.platform_data = &rm696_sec_camera_platform_data,
>> +	},
>> +};
>> +
>> +static struct i2c_board_info rm680_camera_i2c_devices[] = {
>> +	{
>> +		I2C_BOARD_INFO(SMIAPP_NAME, SMIAPP_ALT_I2C_ADDR),
>> +		.platform_data = &rm680_main_camera_platform_data,
>> +	},
>> +	{
>> +		I2C_BOARD_INFO(SMIAPP_NAME, SMIAPP_DFL_I2C_ADDR),
>> +		.platform_data = &rm680_sec_camera_platform_data,
>> +	},
>> +};
>> +
>> +static struct isp_subdev_i2c_board_info rm696_camera_primary_subdevs[] = {
>> +	{
>> +		.board_info = &rm696_camera_i2c_devices[0],
>> +		.i2c_adapter_id = CAMERA_I2C_BUS_NUM,
>> +	},
>> +	{ NULL, 0, },
>> +};
>> +
>> +static struct isp_subdev_i2c_board_info rm696_camera_secondary_subdevs[] =
>> { +	{
>> +		.board_info = &rm696_camera_i2c_devices[1],
>> +		.i2c_adapter_id = CAMERA_I2C_BUS_NUM,
>> +	},
>> +	{ NULL, 0, },
>> +};
>> +
>> +static struct isp_subdev_i2c_board_info rm680_camera_primary_subdevs[] = {
>> +	{
>> +		.board_info = &rm680_camera_i2c_devices[0],
>> +		.i2c_adapter_id = CAMERA_I2C_BUS_NUM,
>> +	},
>> +	{ NULL, 0, },
>> +};
>> +
>> +static struct isp_subdev_i2c_board_info rm680_camera_secondary_subdevs[] =
>> { +	{
>> +		.board_info = &rm680_camera_i2c_devices[1],
>> +		.i2c_adapter_id = CAMERA_I2C_BUS_NUM,
>> +	},
>> +	{ NULL, 0, },
>> +};
>> +
>> +static struct isp_v4l2_subdevs_group rm696_camera_subdevs[] = {
>> +	{
>> +		.subdevs = rm696_camera_primary_subdevs,
>> +		.interface = ISP_INTERFACE_CSI2A_PHY2,
>> +		.bus = { .csi2 = {
>> +			.crc		= 1,
>> +			.vpclk_div	= 1,
>> +			.lanecfg	= &rm696_main_camera_csi2_lanecfg,
>> +		} },
>> +	},
>> +	{
>> +		.subdevs = rm696_camera_secondary_subdevs,
>> +		.interface = ISP_INTERFACE_CCP2B_PHY1,
>> +		.bus = { .ccp2 = {
>> +			.strobe_clk_pol	= 0,
>> +			.crc		= 0,
>> +			.ccp2_mode	= 0,
>> +			.phy_layer	= 0,
>> +			.vpclk_div	= 2,
>> +			.lanecfg	= &rm680_sec_camera_csiphy_lanecfg,
>> +		} },
>> +	},
>> +	{ NULL, 0, },
>> +};
>> +
>> +static struct isp_v4l2_subdevs_group rm680_camera_subdevs[] = {
>> +	{
>> +		.subdevs = rm680_camera_primary_subdevs,
>> +		.interface = ISP_INTERFACE_CSI2A_PHY2,
>> +		.bus = { .csi2 = {
>> +			.crc		= 1,
>> +			.vpclk_div	= 1,
>> +			.lanecfg	= &rm680_main_camera_csi2_lanecfg,
>> +		} },
>> +	},
>> +	{
>> +		.subdevs = rm680_camera_secondary_subdevs,
>> +		.interface = ISP_INTERFACE_CCP2B_PHY1,
>> +		.bus = { .ccp2 = {
>> +			.strobe_clk_pol	= 0,
>> +			.crc		= 0,
>> +			.ccp2_mode	= 0,
>> +			.phy_layer	= 0,
>> +			.vpclk_div	= 2,
>> +			.lanecfg	= &rm680_sec_camera_csiphy_lanecfg,
>> +		} },
>> +	},
>> +	{ NULL, 0, },
>> +};
>> +
>> +static struct isp_platform_data rm696_isp_platform_data = {
>> +	.subdevs = rm696_camera_subdevs,
>> +};
>> +
>> +static struct isp_platform_data rm680_isp_platform_data = {
>> +	.subdevs = rm680_camera_subdevs,
>> +};
>> +
>> +static inline int board_is_rm680(void)
>> +{
>> +	return (system_rev & 0x00f0) == 0x0020;
>> +}
>> +
>> +void __init rm680_camera_init(void)
>> +{
>> +	struct isp_platform_data *pdata;
>> +	int rval;
>> +
>> +	rval = rm680_camera_hw_init();
>> +	if (rval) {
>> +		printk(KERN_WARNING "%s: unable to initialise camera\n",
>> +		       __func__);
>> +		return;
>> +	}
>> +
>> +	if (board_is_rm680())
>> +		pdata = &rm680_isp_platform_data;
>> +	else
>> +		pdata = &rm696_isp_platform_data;
>> +
>> +	if (omap3_init_camera(pdata) < 0)
>> +		printk(KERN_WARNING
>> +		       "%s: unable to register camera platform device\n",
>> +		       __func__);
>> +}
>> diff --git a/arch/arm/mach-omap2/board-rm680.c
>> b/arch/arm/mach-omap2/board-rm680.c index a5bcc75..a1e33d4 100644
>> --- a/arch/arm/mach-omap2/board-rm680.c
>> +++ b/arch/arm/mach-omap2/board-rm680.c
>> @@ -66,6 +66,43 @@ static struct platform_device rm680_vemmc_device = {
>>  	},
>>  };
>>
>> +#define REGULATOR_INIT_DATA(_name, _min, _max, _apply, _ops_mask) \
>> +	static struct regulator_init_data _name##_data = { \
>> +		.constraints = { \
>> +			.name                   = #_name, \
>> +			.min_uV                 = _min, \
>> +			.max_uV                 = _max, \
>> +			.apply_uV               = _apply, \
>> +			.valid_modes_mask       = REGULATOR_MODE_NORMAL | \
>> +						REGULATOR_MODE_STANDBY, \
>> +			.valid_ops_mask         = _ops_mask, \
>> +		}, \
>> +		.num_consumer_supplies  = ARRAY_SIZE(_name##_consumers), \
>> +		.consumer_supplies      = _name##_consumers, \
>> +}
>> +#define REGULATOR_INIT_DATA_FIXED(_name, _voltage) \
>> +	REGULATOR_INIT_DATA(_name, _voltage, _voltage, true, \
>> +				REGULATOR_CHANGE_STATUS | REGULATOR_CHANGE_MODE)
>> +
>> +static struct regulator_consumer_supply rm680_vaux2_consumers[] = {
>> +	REGULATOR_SUPPLY("VDD_CSIPHY1", "omap3isp"),	/* OMAP ISP */
>> +	REGULATOR_SUPPLY("VDD_CSIPHY2", "omap3isp"),	/* OMAP ISP */
>> +	{
>> +		.supply		= "vaux2",
>> +	},
>> +};
>> +REGULATOR_INIT_DATA_FIXED(rm680_vaux2, 1800000);
>> +
>> +static struct regulator_consumer_supply rm680_vaux3_consumers[] = {
>> +	REGULATOR_SUPPLY("VANA", "2-0037"),	/* Main Camera Sensor */
>> +	REGULATOR_SUPPLY("VANA", "2-000e"),	/* Main Camera Lens */
> 
> The lens driver isn't upstream yet. We could keep the regulator supply 
> definition to avoid forgetting it later though.

Sounds good to me.

>> +	REGULATOR_SUPPLY("VANA", "2-0010"),	/* Front Camera */
>> +	{
>> +		.supply		= "vaux3",
>> +	},
>> +};
>> +REGULATOR_INIT_DATA_FIXED(rm680_vaux3, 2800000);
>> +
>>  static struct platform_device *rm680_peripherals_devices[] __initdata = {
>>  	&rm680_vemmc_device,
>>  };
>> @@ -82,6 +119,8 @@ static struct twl4030_gpio_platform_data rm680_gpio_data
>> = { static struct twl4030_platform_data rm680_twl_data = {
>>  	.gpio			= &rm680_gpio_data,
>>  	/* add rest of the children here */
>> +	.vaux2			= &rm680_vaux2_data,
>> +	.vaux3			= &rm680_vaux3_data,
>>  };
>>
>>  static void __init rm680_i2c_init(void)
>> @@ -129,6 +168,8 @@ static struct omap_board_mux board_mux[] __initdata = {
>>  };
>>  #endif
>>
>> +void rm680_camera_init(void);
>> +
>>  static void __init rm680_init(void)
>>  {
>>  	struct omap_sdrc_params *sdrc_params;
>> @@ -141,6 +182,7 @@ static void __init rm680_init(void)
>>
>>  	usb_musb_init(NULL);
>>  	rm680_peripherals_init();
>> +	rm680_camera_init();
>>  }
>>
>>  MACHINE_START(NOKIA_RM680, "Nokia RM-680 board")
> 


-- 
Sakari Ailus
sakari.ailus@maxwell.research.nokia.com