linux-media.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Dongchun Zhu <dongchun.zhu@mediatek.com>
To: <andriy.shevchenko@linux.intel.com>
Cc: <mchehab@kernel.org>, <robh+dt@kernel.org>,
	<mark.rutland@arm.com>, <matthias.bgg@gmail.com>,
	<sakari.ailus@linux.intel.com>, <bingbu.cao@intel.com>,
	<srv_heupstream@mediatek.com>,
	<linux-mediatek@lists.infradead.org>,
	<linux-arm-kernel@lists.infradead.org>, <sj.huang@mediatek.com>,
	<linux-media@vger.kernel.org>, <devicetree@vger.kernel.org>,
	<louis.kuo@mediatek.com>, <shengnan.wang@mediatek.com>
Subject: Re: [V2, 2/2] media: i2c: Add DW9768 VCM driver
Date: Mon, 20 Jan 2020 16:34:41 +0800	[thread overview]
Message-ID: <1579509281.5781.23.camel@mhfsdcap03> (raw)
In-Reply-To: <e8b59857e39744a6acfe5d862f3ac8d5@mtkmbs05n2.mediatek.inc>

Hello Andy,

Thanks for the review. Please see the replies below.

On Mon, 2019-09-05 at 12:26 +0200, Andy Shevchenko wrote:
> On Thu, Sep 05, 2019 at 03:21:42PM +0800, dongchun.zhu@mediatek.com wrote:
> > From: Dongchun Zhu <dongchun.zhu@mediatek.com>
> >
> > This patch adds a V4L2 sub-device driver for DW9768 lens voice coil,
> > and provides control to set the desired focus.
> >
> > The DW9768 is a 10 bit DAC with 100mA output current sink capability
> > from Dongwoon, designed for linear control of voice coil motor, and
> > controlled via I2C serial interface.
> 
> > Signed-off-by: Dongchun Zhu <dongchun.zhu@mediatek.com>
> > ---
> >  MAINTAINERS                |   1 +
> 
> This should go to the same patch, where you introduce a record in the MAINTAINERS database.
> 
> > +#define DW9768_SET_POSITION_ADDR                0x03
> 
> Indentation issue.
> 

Fixed in next release.

> > +static struct regval_list dw9768_init_regs[] = {
> > +{0x02, 0x02},
> > +{DW9768_CMD_DELAY, DW9768_CMD_DELAY},
> > +{0x06, 0x41},
> > +{0x07, 0x39},
> > +{DW9768_CMD_DELAY, DW9768_CMD_DELAY}, };
> > +
> > +static struct regval_list dw9768_release_regs[] = {
> > +{0x02, 0x00},
> > +{DW9768_CMD_DELAY, DW9768_CMD_DELAY},
> > +{0x01, 0x00},
> > +{DW9768_CMD_DELAY, DW9768_CMD_DELAY}, };
> > +
> > +static int dw9768_write_smbus(struct dw9768 *dw9768, unsigned char reg,
> > +      unsigned char value)
> > +{
> > +struct i2c_client *client = v4l2_get_subdevdata(&dw9768->sd);
> > +int ret;
> > +
> 
> > +if (reg == DW9768_CMD_DELAY  && value == DW9768_CMD_DELAY)
> 
> Indentation issue.
> But see other's comments.
> 
> > +usleep_range(DW9768_CTRL_DELAY_US,
> > +     DW9768_CTRL_DELAY_US + 100);
> 
> > +else
> 
> This needs an explanation.
> 

Fixed in next release.

> > +ret = i2c_smbus_write_byte_data(client, reg, value);
> > +return ret;
> > +}
> 
> I'm wondering if we can benefit from regmap I²C API in this driver.
> 
> > +static int __maybe_unused dw9768_vcm_suspend(struct device *dev) {
> 
> > +struct i2c_client *client = to_i2c_client(dev);
> > +struct v4l2_subdev *sd = i2c_get_clientdata(client);
> 
> isn't is simple dev_get_drvdata() ?
> 

As discussed in another group mail, even dev_set_drvdata() is set when
probe, the pointer that point to the private device struct changes when
using dev_get_drvdata() API.
Then resume/power on would result in kernel exception in
regulator_enable.

This is so weird.
We filed one Google issue to trace this behavior.
https://partnerissuetracker.corp.google.com/issues/147957975

I am wondering whether there is any condition of the use of
dev_get_drvdata().
I checked the other drivers and found that most devices that use
dev_get_drvdata() are platform devices or non i2c client devices.
So maybe dw9768 or other media/i2c devices cannot support
dev_get_drvdata()?

> > +struct dw9768 *dw9768 = sd_to_dw9768_vcm(sd);
> > +
> > +return dw9768_power_off(dw9768);
> > +}
> > +
> > +static int __maybe_unused dw9768_vcm_resume(struct device *dev) {
> 
> > +struct i2c_client *client = to_i2c_client(dev);
> > +struct v4l2_subdev *sd = i2c_get_clientdata(client);
> 
> Ditto.
> 
> > +struct dw9768 *dw9768 = sd_to_dw9768_vcm(sd);
> > +
> > +return dw9768_power_on(dw9768);
> > +}
> 
> > +static const struct i2c_device_id dw9768_id_table[] = {
> > +{ DW9768_NAME, 0 },
> 
> > +{ },
> 
> No comma.
> 

This struct would be removed in next release.

> > +};
> 
> > +static const struct of_device_id dw9768_of_table[] = {
> > +{ .compatible = "dongwoon,dw9768" },
> 
> > +{ },
> 
> Ditto.
> 

Fixed in next release.

> > +};
> 
> --
> With Best Regards,
> Andy Shevchenko
> 
> 
> *********************MEDIATEK Confidential/Internal Use*********************


  parent reply	other threads:[~2020-01-20  8:35 UTC|newest]

Thread overview: 20+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-09-05  7:21 [V2, 0/2] media: i2c: add support for " dongchun.zhu
2019-09-05  7:21 ` [V2, 1/2] media: i2c: dw9768: Add DT support and MAINTAINERS entry dongchun.zhu
2019-09-05 10:14   ` Andy Shevchenko
2019-09-05 10:48     ` Sakari Ailus
2019-09-05 11:35       ` Andy Shevchenko
2019-09-05 11:49         ` Javier Martinez Canillas
2019-09-05 12:00           ` Sakari Ailus
2019-09-05 12:24             ` Andy Shevchenko
2019-09-17 20:47               ` Rob Herring
2019-09-05  7:21 ` [V2, 2/2] media: i2c: Add DW9768 VCM driver dongchun.zhu
2019-09-05  8:21   ` Sakari Ailus
2019-09-05 10:19     ` Andy Shevchenko
2019-09-05 10:40       ` Sakari Ailus
2019-09-05 10:57         ` Javier Martinez Canillas
2019-09-05 11:36           ` Andy Shevchenko
2019-09-05  8:28   ` Tomasz Figa
2019-09-05 10:26   ` Andy Shevchenko
     [not found]     ` <e8b59857e39744a6acfe5d862f3ac8d5@mtkmbs05n2.mediatek.inc>
2020-01-20  8:34       ` Dongchun Zhu [this message]
2019-09-07 22:12   ` kbuild test robot
2019-10-09  4:40   ` Tomasz Figa

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=1579509281.5781.23.camel@mhfsdcap03 \
    --to=dongchun.zhu@mediatek.com \
    --cc=andriy.shevchenko@linux.intel.com \
    --cc=bingbu.cao@intel.com \
    --cc=devicetree@vger.kernel.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 \
    --subject='Re: [V2, 2/2] media: i2c: Add DW9768 VCM driver' \
    /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

This is a public inbox, see mirroring instructions
on how to clone and mirror all data and code used for this inbox