Hi Sakari,
On Thu, May 3, 2012 at 7:03 AM, Aguirre, Sergio <saaguirre@xxxxxx> wrote:
> Hi Sakari,
>
> Thanks for reviewing.
>
> On Wed, May 2, 2012 at 2:47 PM, Sakari Ailus <sakari.ailus@xxxxxx> wrote:
>>
>> Hi Sergio,
>>
>> Thanks for the patches!!
>>
>> On Wed, May 02, 2012 at 10:15:46AM -0500, Sergio Aguirre wrote:
>> ...
>>> +static int sdp4430_ov_cam1_power(struct v4l2_subdev *subdev, int on)
>>> +{
>>> + struct device *dev = subdev->v4l2_dev->dev;
>>> + int ret;
>>> +
>>> + if (on) {
>>> + if (!regulator_is_enabled(sdp4430_cam2pwr_reg)) {
>>> + ret = regulator_enable(sdp4430_cam2pwr_reg);
>>> + if (ret) {
>>> + dev_err(dev,
>>> + "Error in enabling sensor power regulator 'cam2pwr'\n");
>>> + return ret;
>>> + }
>>> +
>>> + msleep(50);
>>> + }
>>> +
>>> + gpio_set_value(OMAP4430SDP_GPIO_CAM_PDN_B, 1);
>>> + msleep(10);
>>> + ret = clk_enable(sdp4430_cam1_aux_clk); /* Enable XCLK */
>>> + if (ret) {
>>> + dev_err(dev,
>>> + "Error in clk_enable() in %s(%d)\n",
>>> + __func__, on);
>>> + gpio_set_value(OMAP4430SDP_GPIO_CAM_PDN_B, 0);
>>> + return ret;
>>> + }
>>> + msleep(10);
>>> + } else {
>>> + clk_disable(sdp4430_cam1_aux_clk);
>>> + msleep(1);
>>> + gpio_set_value(OMAP4430SDP_GPIO_CAM_PDN_B, 0);
>>> + if (regulator_is_enabled(sdp4430_cam2pwr_reg)) {
>>> + ret = regulator_disable(sdp4430_cam2pwr_reg);
>>> + if (ret) {
>>> + dev_err(dev,
>>> + "Error in disabling sensor power regulator 'cam2pwr'\n");
>>> + return ret;
>>> + }
>>> + }
>>> + }
>>> +
>>> + return 0;
>>> +}
>>
>> Isn't this something that should be part of the sensor driver? There's
>> nothing in the above code that would be board specific, except the names of
>> the clocks, regulators and GPIOs. The sensor driver could hold the names
>> instead; this would be also compatible with the device tree.
>
> Agreed. I see what you mean...
>
> I'll take care of that.
Can you please check out these patches?
1. http://gitorious.org/omap4-v4l2-camera/omap4-v4l2-camera/commit/cb6c10d58053180364461e6bc8d30d1ec87e6e22
2. http://gitorious.org/omap4-v4l2-camera/omap4-v4l2-camera/commit/6732e0db25c6647b34ef8f01c244a49a1fd6b45d
3. http://gitorious.org/omap4-v4l2-camera/omap4-v4l2-camera/commit/d61c4e3142dc9cae972f9128fe73d986838c0ca1
4. http://gitorious.org/omap4-v4l2-camera/omap4-v4l2-camera/commit/e83f36001c7f7cbe184ad094d9b0c95c39e5028f
I want to see if I got your point properly...
Regards,
Sergio
>
>>
>> It should be possible to have s_power() callback NULL, too.
>>
>>> +static int sdp4430_ov_cam2_power(struct v4l2_subdev *subdev, int on)
>>> +{
>>> + struct device *dev = subdev->v4l2_dev->dev;
>>> + int ret;
>>> +
>>> + if (on) {
>>> + u8 gpoctl = 0;
>>> +
>>> + ret = regulator_enable(sdp4430_cam2pwr_reg);
>>> + if (ret) {
>>> + dev_err(dev,
>>> + "Error in enabling sensor power regulator 'cam2pwr'\n");
>>> + return ret;
>>> + }
>>> +
>>> + msleep(50);
>>> +
>>> + if (twl_i2c_read_u8(TWL_MODULE_AUDIO_VOICE, &gpoctl,
>>> + TWL6040_REG_GPOCTL))
>>> + return -ENODEV;
>>> + if (twl_i2c_write_u8(TWL_MODULE_AUDIO_VOICE,
>>> + gpoctl | TWL6040_GPO3,
>>> + TWL6040_REG_GPOCTL))
>>> + return -ENODEV;
>>
>> The above piece of code looks quite interesting. What does it do?
>
> Well, this is because the camera adapter board in 4430SDP has 3
> sensors actually:
>
> - 1 Sony IMX060 12.1 MP sensor
> - 2 OmniVision OV5650 sensors
>
> And there's 3 wideband analog switches, like this:
>
> http://www.analog.com/static/imported-files/data_sheets/ADG936_936R.pdf
>
> That basically select either IMX060 or OV5650 for CSI2A input.
>
> So, this commands are because the TWL6040 chip has a GPO pin to toggle
> this, instead
> of an OMAP GPIO (Don't ask me why :) )
>
> Anyways... I see your point, maybe this should be explained better
> through a comment.
>
> Regards,
> Sergio
>>
>> Kind regards,
>>
>> --
>> Sakari Ailus
>> e-mail: sakari.ailus@xxxxxx jabber/XMPP/Gmail: sailus@xxxxxxxxxxxxxx
--
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
