Re: [V6, 2/2] media: i2c: dw9768: Add DW9768 VCM driver

From: Tomasz Figa <tfiga@chromium.org>
To: Dongchun Zhu <dongchun.zhu@mediatek.com>
Cc: "Linus Walleij" <linus.walleij@linaro.org>,
	"Bartosz Golaszewski" <bgolaszewski@baylibre.com>,
	"Mauro Carvalho Chehab" <mchehab@kernel.org>,
	"Andy Shevchenko" <andriy.shevchenko@linux.intel.com>,
	"Rob Herring" <robh+dt@kernel.org>,
	"Mark Rutland" <mark.rutland@arm.com>,
	"Sakari Ailus" <sakari.ailus@linux.intel.com>,
	"Nicolas Boichat" <drinkcat@chromium.org>,
	"Matthias Brugger" <matthias.bgg@gmail.com>,
	"Cao Bing Bu" <bingbu.cao@intel.com>,
	srv_heupstream <srv_heupstream@mediatek.com>,
	"moderated list:ARM/Mediatek SoC support"
	<linux-mediatek@lists.infradead.org>,
	"list@263.net:IOMMU DRIVERS <iommu@lists.linux-foundation.org>,
	Joerg Roedel <joro@8bytes.org>,"
	<linux-arm-kernel@lists.infradead.org>,
	"Sj Huang" <sj.huang@mediatek.com>,
	"Linux Media Mailing List" <linux-media@vger.kernel.org>,
	linux-devicetree <devicetree@vger.kernel.org>,
	"Louis Kuo" <louis.kuo@mediatek.com>,
	"Shengnan Wang (王圣男)" <shengnan.wang@mediatek.com>
Subject: Re: [V6, 2/2] media: i2c: dw9768: Add DW9768 VCM driver
Date: Mon, 1 Jun 2020 20:47:54 +0200	[thread overview]
Message-ID: <CAAFQd5Dgboh8om68546ADELX3g-0y40rdBxY+H3WsX5xAD1_FQ@mail.gmail.com> (raw)
In-Reply-To: <1590570089.8804.453.camel@mhfsdcap03>

On Wed, May 27, 2020 at 11:03 AM Dongchun Zhu <dongchun.zhu@mediatek.com> wrote:
>
> Hi Tomasz,
>
> On Mon, 2020-05-25 at 13:45 +0200, Tomasz Figa wrote:
> > On Fri, May 22, 2020 at 11:27 AM Dongchun Zhu <dongchun.zhu@mediatek.com> wrote:
> > >
> > > Hi Tomasz,
> > >
> > > Thanks for the review. My replies are as below.
> > >
> > > On Thu, 2020-05-21 at 19:51 +0000, Tomasz Figa wrote:
> > > > Hi Dongchun, Sakari,
> > > >
> > > > On Mon, May 18, 2020 at 09:27:31PM +0800, Dongchun Zhu wrote:
> > [snip]
> > > > > +   pm_runtime_enable(dev);
> > > > > +   if (!pm_runtime_enabled(dev)) {
> > > > > +           ret = dw9768_runtime_resume(dev);
> > > > > +           if (ret < 0) {
> > > > > +                   dev_err(dev, "failed to power on: %d\n", ret);
> > > > > +                   goto entity_cleanup;
> > > > > +           }
> > > > > +   }
> > > > > +
> > > > > +   ret = v4l2_async_register_subdev(&dw9768->sd);
> > > > > +   if (ret < 0)
> > > > > +           goto entity_cleanup;
> > > > > +
> > > > > +   return 0;
> > > > > +
> > > > > +entity_cleanup:
> > > >
> > > > Need to power off if the code above powered on.
> > > >
> > >
> > > Thanks for the reminder.
> > > If there is something wrong with runtime PM, actuator is to be powered
> > > on via dw9768_runtime_resume() API.
> > > When actuator sub-device is powered on completely and async registered
> > > successfully, we shall power off it afterwards.
> > >
> >
> > The code above calls dw9768_runtime_resume() if
> > !pm_runtime_enabled(dev), but the clean-up code below the
> > entity_cleanup label doesn't have the corresponding
> > dw9768_runtime_suspend() call.
> >
>
> Did you mean the 'entity_cleanup' after v4l2_async_register_subdev()?

Yes.

> Actually I made some changes for OV02A V9, according to this comment.
> Could you help review that change? Thanks.

Sure, I will take a look.

Best regards,
Tomasz