Linux-Media Archive on lore.kernel.org
 help / color / Atom feed
From: Tomasz Figa <tfiga@chromium.org>
To: Dongchun Zhu <dongchun.zhu@mediatek.com>
Cc: "Sakari Ailus" <sakari.ailus@linux.intel.com>,
	"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>,
	"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: [V5, 2/2] media: i2c: dw9768: Add DW9768 VCM driver
Date: Mon, 11 May 2020 20:20:59 +0200
Message-ID: <CAAFQd5DZBEUJUq65WT7i_QjAtgxjhxmfdsjQoyG2UZy8zBih+A@mail.gmail.com> (raw)
In-Reply-To: <1588991026.8804.235.camel@mhfsdcap03>

Hi Dongchun,

On Sat, May 9, 2020 at 4:25 AM Dongchun Zhu <dongchun.zhu@mediatek.com> wrote:
>
> Hi Sakari,
>
> On Sat, 2020-05-09 at 00:13 +0300, Sakari Ailus wrote:
> > Hi Dongchun,
> >
> > On Fri, May 08, 2020 at 11:08:08AM +0800, Dongchun Zhu wrote:
> > > Hi Sakari, Tomasz,
> > >
> > > Thanks for the review.
> > >
> > > On Thu, 2020-05-07 at 15:46 +0200, Tomasz Figa wrote:
> > > > Hi Sakari, Dongchun,
> > > >
> > > > On Thu, May 7, 2020 at 3:12 PM Sakari Ailus
> > > > <sakari.ailus@linux.intel.com> wrote:
> > > > >
> > > > > HI Dongchun,
> > > > >
> > > > > On Thu, May 07, 2020 at 08:45:24PM +0800, Dongchun Zhu wrote:
> > > > > > Hi Sakari,
> > > > > >
> > > > > > Thanks for the review.
> > > > > >
> > > > > > On Wed, 2020-05-06 at 18:13 +0300, Sakari Ailus wrote:
> > > > > > > Hi Dongchun,
> > > > > > >
> > > > > > > On Sun, May 03, 2020 at 12:17:27AM +0800, Dongchun Zhu wrote:
> > > > > > > > Add a V4L2 sub-device driver for DW9768 voice coil motor, providing
> > > > > > > > control to set the desired focus via IIC serial interface.
> > > > > > > >
> > > > > > > > Signed-off-by: Dongchun Zhu <dongchun.zhu@mediatek.com>
> > > > > > > > ---
> > > > > > > >  MAINTAINERS                |   1 +
> > > > > > > >  drivers/media/i2c/Kconfig  |  11 ++
> > > > > > > >  drivers/media/i2c/Makefile |   1 +
> > > > > > > >  drivers/media/i2c/dw9768.c | 440 +++++++++++++++++++++++++++++++++++++++++++++
> > > > > > > >  4 files changed, 453 insertions(+)
> > > > > > > >  create mode 100644 drivers/media/i2c/dw9768.c
> > > > > > > >
> > > > > > > > diff --git a/MAINTAINERS b/MAINTAINERS
> > > > > > > > index 8d72c41..c92dc99 100644
> > > > > > > > --- a/MAINTAINERS
> > > > > > > > +++ b/MAINTAINERS
> > > > > > > > @@ -5157,6 +5157,7 @@ L:  linux-media@vger.kernel.org
> > > > > > > >  S:       Maintained
> > > > > > > >  T:       git git://linuxtv.org/media_tree.git
> > > > > > > >  F:       Documentation/devicetree/bindings/media/i2c/dongwoon,dw9768.yaml
> > > > > > > > +F:       drivers/media/i2c/dw9768.c
> > > > > > > >
> > > > > > > >  DONGWOON DW9807 LENS VOICE COIL DRIVER
> > > > > > > >  M:       Sakari Ailus <sakari.ailus@linux.intel.com>
> > > > > > > > diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig
> > > > > > > > index 125d596..6a3f9da 100644
> > > > > > > > --- a/drivers/media/i2c/Kconfig
> > > > > > > > +++ b/drivers/media/i2c/Kconfig
> > > > > > > > @@ -1040,6 +1040,17 @@ config VIDEO_DW9714
> > > > > > > >     capability. This is designed for linear control of
> > > > > > > >     voice coil motors, controlled via I2C serial interface.
> > > > > > > >
> > > > > > > > +config VIDEO_DW9768
> > > > > > > > + tristate "DW9768 lens voice coil support"
> > > > > > > > + depends on I2C && VIDEO_V4L2 && MEDIA_CONTROLLER
> > > > > > > > + depends on VIDEO_V4L2_SUBDEV_API
> > > > > > >
> > > > > > > Please check how this works in the media tree master branch now --- it's
> > > > > > > largely select based.
> > > > > > >
> > > > > >
> > > > > > The actuator driver uses some structures that require the
> > > > > > VIDEO_V4L2_SUBDEV_API code, so here we add VIDEO_V4L2_SUBDEV_API
> > > > > > dependency to avoid possible build error when it's not enabled.
> > > > >
> > > > > Please make sure this works with current media tree master. Right now it
> > > > > does not.
> > > > >
> > > >
> > > > Dongchun, as Sakari said, please make sure to base the patches on the
> > > > master branch of the media tree.
> > > > (https://git.linuxtv.org/media_tree.git/). The approach for Kconfig
> > > > dependency selection there seems to have changed recently.
> > > >
> > >
> > > I searched the patches on the media tree master branch.
> > > It seems that we need to remove the VIDEO_V4L2_SUBDEV_API dependency in
> > > Kconfig, and add #ifdef CONFIG_VIDEO_V4L2_SUBDEV_API to include
> > > v4l2-subdev code.
> > > The change mainly is to make build pass, and don't return ENOTTY if
> > > SUBDEV_API is not set.
> > > Am I right?
> >
> > Please see Kconfig entries for other similar drivers from Dongwoon.
> >
>
> Sorry for the mistake :-)
> Just found the current media tree master branch code...
> I would update Kconfig entries in next release by referring to:
> https://git.linuxtv.org/media_tree.git/tree/drivers/media/i2c/Kconfig

Sorry for last minute comments again. We had a short discussion
offline with Sakari and we think there are some changes needed to this
driver, namely:

1) The hardware being driven in our case is a gt9769, which could be
compatible with dw9768, but it's still a different implementation and
could have slightly different characteristics. Thus we think the
driver name and compatible strings should be renamed from
dongwoon,dw9768 to giantec,gt9769. In the future, if there is a device
with exactly a dw9768 chip, the same driver could be reused by adding
a dongwoon,dw9768 compatible string.

2) The chip has some default configuration, which is lost because the
driver overrides it with its own values. For use cases where one would
want to keep the default values, we should make it possible to prevent
the driver from overriding them. We could achieve this by adding
optional DT properties for the custom parameters and if they are not
present, defaults would be used.

Do you think that is doable? Thanks!

Best regards,
Tomasz

  reply index

Thread overview: 17+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-05-02 16:17 [V5, 0/2] media: i2c: Add support for " Dongchun Zhu
2020-05-02 16:17 ` [V5, 1/2] media: dt-bindings: media: i2c: Document DW9768 bindings Dongchun Zhu
2020-05-05 19:15   ` Rob Herring
2020-05-02 16:17 ` [V5, 2/2] media: i2c: dw9768: Add DW9768 VCM driver Dongchun Zhu
2020-05-06 15:13   ` Sakari Ailus
2020-05-07 12:45     ` Dongchun Zhu
2020-05-07 13:12       ` Sakari Ailus
2020-05-07 13:46         ` Tomasz Figa
2020-05-07 14:00           ` Sakari Ailus
2020-05-08  3:27             ` Dongchun Zhu
2020-05-08  3:08           ` Dongchun Zhu
2020-05-08 21:13             ` Sakari Ailus
2020-05-09  2:23               ` Dongchun Zhu
2020-05-11 18:20                 ` Tomasz Figa [this message]
2020-05-12  3:33                   ` Dongchun Zhu
2020-05-12  8:58                     ` Sakari Ailus
2020-05-12 11:32                       ` Dongchun Zhu

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=CAAFQd5DZBEUJUq65WT7i_QjAtgxjhxmfdsjQoyG2UZy8zBih+A@mail.gmail.com \
    --to=tfiga@chromium.org \
    --cc=andriy.shevchenko@linux.intel.com \
    --cc=bgolaszewski@baylibre.com \
    --cc=bingbu.cao@intel.com \
    --cc=devicetree@vger.kernel.org \
    --cc=dongchun.zhu@mediatek.com \
    --cc=drinkcat@chromium.org \
    --cc=linus.walleij@linaro.org \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-media@vger.kernel.org \
    --cc=linux-mediatek@lists.infradead.org \
    --cc=louis.kuo@mediatek.com \
    --cc=mark.rutland@arm.com \
    --cc=matthias.bgg@gmail.com \
    --cc=mchehab@kernel.org \
    --cc=robh+dt@kernel.org \
    --cc=sakari.ailus@linux.intel.com \
    --cc=shengnan.wang@mediatek.com \
    --cc=sj.huang@mediatek.com \
    --cc=srv_heupstream@mediatek.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Linux-Media Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-media/0 linux-media/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-media linux-media/ https://lore.kernel.org/linux-media \
		linux-media@vger.kernel.org
	public-inbox-index linux-media

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-media


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git