Hi Laurent, On Tue, Nov 05, 2019 at 10:36:00PM +0200, Laurent Pinchart wrote: > Hi Sakari, > > On Mon, Nov 04, 2019 at 11:30:56PM +0200, Sakari Ailus wrote: > > On Mon, Nov 04, 2019 at 08:59:03PM +0200, Laurent Pinchart wrote: > > > 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. > > You're pushing a requirement of ACPI platforms down to OF platforms, > where it can introduce a regression as driver-centric PM may lead to > larger delays when starting the stream compared to the traditional > .s_power() method. The reason why the pipeline power management was introduced was that the subdevices along the pipeline needed some kind of power management, and the s_power() op was extended to cover the pipeline. Runtime PM did not exist at the time, so having a V4L2 specific solution wasn't about re-inventing the wheel. Now that we have the wheel, there's no need for local (only somewhat functional) copies of it. Runtime PM is certainly not ACPI specific in any way, it's just means to do device power management in Linux. > > Granted, .s_power() is ill-defined, and power management rules are not > clear, especially in the application-facing APIs. Switching this driver > to use runtime PM would hardly make a noticeable difference in the big > picture. This being said, we have a broken PM model for V4L2 camera > sensors, and we'll have to address the issue one way or another. I'm > pretty sure the only reason why we managed to get away with is it that > high performance products are not shipped with a mainline kernel. The only case this really shows at the moment is the very first time streaming is started. In some use cases it could be critical, no argument about that. But I'd argue there have been more important problems to solve as there has been no concrete proposals to solve this one. Applications making use of any such solution would have to be specifically written for it, as it requires new API elements. If a solution is proposed, I think it could benefit from the existing pipeline power management codebase. -- Regards, Sakari Ailus sakari.ailus@xxxxxxxxxxxxxxx
