Re: [PATCH v3 07/10] arm: omap4430sdp: Add support for omap4iss camera

["Aguirre, Sergio" <saaguirre@xxxxxx>]

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