Hi Laurent, On Mon, Nov 04, 2019 at 08:59:03PM +0200, Laurent Pinchart wrote: > Hi Sakari, > > On Mon, Nov 04, 2019 at 03:33:51PM +0200, Laurent Pinchart wrote: > > On Thu, Oct 31, 2019 at 04:25:22PM +0200, Sakari Ailus wrote: > > > On Thu, Oct 31, 2019 at 03:23:09PM +0200, Laurent Pinchart wrote: > > > > The IMX296LLR is a monochrome 1.60MP CMOS sensor from Sony. The driver > > > > supports cropping and binning (but not both at the same time due to > > > > hardware limitations) and exposure, gain, vertical blanking and test > > > > pattern controls. > > > > > > > > Preliminary support is also included for the color IMX296LQR sensor. > > > > > > > > Signed-off-by: Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx> > > > > --- > > > > This driver is a parallel implementation of IMX296 support, compatible > > > > with the DT bindings submitted by Mani in > > > > https://lore.kernel.org/linux-media/20191030094902.32582-1-manivannan.sadhasivam@xxxxxxxxxx/. > > > > > > > > drivers/media/i2c/Kconfig | 12 + > > > > drivers/media/i2c/Makefile | 1 + > > > > drivers/media/i2c/imx296.c | 1026 ++++++++++++++++++++++++++++++++++++ > > > > 3 files changed, 1039 insertions(+) > > > > create mode 100644 drivers/media/i2c/imx296.c > > [snip] > > > > > diff --git a/drivers/media/i2c/imx296.c b/drivers/media/i2c/imx296.c > > > > new file mode 100644 > > > > index 000000000000..4140637983fd > > > > --- /dev/null > > > > +++ b/drivers/media/i2c/imx296.c > > [snip] > > > > > +static int imx296_power_on(struct imx296 *imx) > > > > +{ > > > > + int ret; > > > > + > > > > + ret = regulator_enable(imx->supply); > > > > + if (ret < 0) > > > > + return ret; > > > > + > > > > + udelay(1); > > > > + > > > > + ret = gpiod_direction_output(imx->reset, 0); > > > > + if (ret < 0) > > > > + goto err_supply; > > > > + > > > > + udelay(1); > > > > + > > > > + ret = clk_prepare_enable(imx->clk); > > > > + if (ret < 0) > > > > + goto err_reset; > > > > + > > > > + /* > > > > + * The documentation doesn't explicitly say how much time is required > > > > + * after providing a clock and before starting I2C communication. It > > > > + * mentions a delay of 20µs in 4-wire mode, but tests showed that a > > > > + * delay of 100µs resulted in I2C communication failures, while 500µs > > > > + * seems to be enough. Be conservative. > > > > + */ > > > > + usleep_range(1000, 2000); > > > > + > > > > + return 0; > > > > + > > > > +err_reset: > > > > + gpiod_direction_output(imx->reset, 1); > > > > +err_supply: > > > > + regulator_disable(imx->supply); > > > > + return ret; > > > > > > Could you switch to runtime PM? It's not hard. See e.g. > > > drivers/media/i2c/ov5670.c for an example. Or, for a more complete example, > > > the smiapp driver. :-) > > > > I'll give it a try. > > I was expecting the MC and V4L2 core to deal with PM but they don't seem > to. Do I thus understand correctly that switching to runtime PM will > cause the full power on sequence to happen at stream on time ? This can > lead to a significant delay when starting the stream. > > Furthermore, if nothing else than the driver deals with runtime PM, > what's the advantage of using the runtime PM API over calling the power > on/off at stream on/off time manually ? Runtime PM abstracts power management for drivers, so drivers don't need, for instance, to know the system firmware type for its own sake. (On DT the driver still needs to implement runtime PM callbacks for device resume and suspend, for instance.) But on ACPI you effectively need runtime PM if you want some kind of dynamic power management to take place. Runtime PM also takes into account managing power for device's parents and other things there are no alternatives for. So there's really little excuse of not supporting runtime PM if the device isn't going to be always powered on. -- Regards, Sakari Ailus sakari.ailus@xxxxxxxxxxxxxxx
