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.
>
> 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
