On 12/14/11 20:14, martin@xxxxxxxxxxxxxxxxxxxxxx wrote:
> On Wed, Dec 14, 2011 at 11:31:35AM +0200, Igor Grinberg wrote:
>> Hi Martin,
>>
>> On 12/14/11 03:25, Martin Hostettler wrote:
>>> Adds board support for an MT9M032 based camera to omap3evm.
>>>
>>> Signed-off-by: Martin Hostettler <martin@xxxxxxxxxxxxxxxxxxxxxx>
>>> ---
>>> arch/arm/mach-omap2/Makefile | 3 +-
>>> arch/arm/mach-omap2/board-omap3evm-camera.c | 155 +++++++++++++++++++++++++++
>>> arch/arm/mach-omap2/board-omap3evm.c | 4 +
>>> 3 files changed, 161 insertions(+), 1 deletions(-)
>>> create mode 100644 arch/arm/mach-omap2/board-omap3evm-camera.c
>>>
>>> Changes in V3
>>> * Added missing copyright and attribution.
>>> * switched to gpio_request_array for gpio init.
>>> * removed device_initcall and added call to omap3_evm_camera_init into omap3_evm_init
>>>
>>> Changes in V2:
>>> * ported to current mainline
>>> * Style fixes
>>> * Fix error handling
>>>
[...]
>>> +/**
>>> + * omap3evm_set_mux - Sets mux to enable signal routing to
>>> + * different peripherals present on new EVM board
>>> + * @mux_id: enum, mux id to enable
>>> + *
>>> + * Returns 0 for success or a negative error code
>>> + */
>>> +static int omap3evm_set_mux(enum omap3evmdc_mux mux_id)
>>> +{
>>> + /* Set GPIO6 = 1 */
>>> + gpio_set_value_cansleep(EVM_TWL_GPIO_BASE + 6, 1);
>>> + gpio_set_value_cansleep(EVM_TWL_GPIO_BASE + 2, 0);
>>> +
>>> + switch (mux_id) {
>>> + case MUX_TVP5146:
>>> + gpio_set_value_cansleep(EVM_TWL_GPIO_BASE + 2, 0);
>>> + gpio_set_value(nCAM_VD_SEL, 1);
>>> + break;
>>> +
>>> + case MUX_CAMERA_SENSOR:
>>> + gpio_set_value_cansleep(EVM_TWL_GPIO_BASE + 2, 0);
>>> + gpio_set_value(nCAM_VD_SEL, 0);
>>> + break;
>>> +
>>> + case MUX_EXP_CAMERA_SENSOR:
>>> + gpio_set_value_cansleep(EVM_TWL_GPIO_BASE + 2, 1);
>>> + break;
>>> +
>>> + default:
>>> + pr_err("omap3evm-camera: Invalid mux id #%d\n", mux_id);
>>> + return -EINVAL;
>>> + }
>>> +
>>> + return 0;
>>> +}
>>
>> I don't really care about that, but I don't see any mux
>> being set in the above function so the name and comments
>> are misleading.
>
> There's are video muxes on this board that's controlled by various
> gpios. It's not a mux in the omap chip if you've expected to see that.
>
> As this is an evaluation board it has a bunch of video connectors that
> the user can choose from for different input devices.
I see... Probably a comment explaining that would not hurt here.
[...]
>>> +
>>> + omap_mux_init_gpio(nCAM_VD_SEL, OMAP_PIN_OUTPUT);
>>> + ret = gpio_request_array(setup_gpios, ARRAY_SIZE(setup_gpios));
>>> + if (ret < 0) {
>>> + pr_err("omap3evm-camera: Failed to setup camera signal routing.\n");
>>> + return ret;
>>> + }
>>
>> It looks like both above calls (gpio_request and mux_init)
>> can be moved to omap3evm_set_mux() function (or a renamed version of it),
>> so all the GPIO stuff will be close to each other instead of requesting
>> in one place and playing with values in another...
>
> I'd like to keep the one time setup and the theoretically run time switchable
> parts seperate. It doesn't complicate the code and if a brave soul wants to
> connect different camera modules and switch between them it's a more reviewable
> patch from here.
Ok
>
>>
>>> + omap3evm_set_mux(MUX_CAMERA_SENSOR);
>>
>> So the plan is to add support for the 3 types,
>> but hard code to only one?
>> Can't this be runtime detected somehow?
>
> The mux code came from out of tree drivers. I did want to keep enough
> information so someone extending this board code for other setups doesn't have a
> hard time. I can't think of an reliable way to runtime detect what video source
> a specific use case would want. Ideally someone who needs one of the other
> video sources should add a more generic solution here.
I'm Ok with it, but usually, stuff that is never used stays out...
I think the way it should be is to have a platform driver that uses
a callback to switch between the "muxes" on the extension.
[...]
--
Regards,
Igor.
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
