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