linux-media.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Tomasz Figa <tfiga@chromium.org>
To: Dongchun Zhu <dongchun.zhu@mediatek.com>
Cc: Mauro Carvalho Chehab <mchehab@kernel.org>,
	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>,
	devicetree@vger.kernel.org, sam.hung@mediatek.com,
	shengnan.wang@mediatek.com
Subject: Re: [V2, 2/2] media: i2c: Add DW9768 VCM driver
Date: Wed, 9 Oct 2019 13:40:07 +0900	[thread overview]
Message-ID: <CAAFQd5A9mn+695CZdYoXzuQodQqC=p8dstbdFAkOCCHKMUtg-w@mail.gmail.com> (raw)
In-Reply-To: <20190905072142.14606-3-dongchun.zhu@mediatek.com>

Hi Dongchun,

On Thu, Sep 5, 2019 at 4:22 PM <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 +
>  drivers/media/i2c/Kconfig  |  10 ++
>  drivers/media/i2c/Makefile |   1 +
>  drivers/media/i2c/dw9768.c | 349 +++++++++++++++++++++++++++++++++++++++++++++
>  4 files changed, 361 insertions(+)
>  create mode 100644 drivers/media/i2c/dw9768.c
>

Please see my further comments inline.

[snip]
> +struct regval_list {
> +       unsigned char reg_num;
> +       unsigned char value;

nit: Since we have strictly sized values here, should we use u8 for
both fields instead?

> +};
> +
> +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)

Should we use u8 for the last two arguments here as well?

> +{
> +       struct i2c_client *client = v4l2_get_subdevdata(&dw9768->sd);
> +       int ret;
> +
> +       if (reg == DW9768_CMD_DELAY  && value == DW9768_CMD_DELAY)
> +               usleep_range(DW9768_CTRL_DELAY_US,
> +                            DW9768_CTRL_DELAY_US + 100);

ret will be uninitialized if we go this path.

> +       else
> +               ret = i2c_smbus_write_byte_data(client, reg, value);
> +       return ret;
> +}
> +
> +static int dw9768_write_array(struct dw9768 *dw9768, struct regval_list *vals,
> +                             u32 len)

Since len is an array size, should we use size_t instead?

> +{
> +       unsigned int i;

size_t?

> +       int ret;
> +
> +       for (i = 0; i < len; i++) {
> +               ret = dw9768_write_smbus(dw9768, vals->reg_num, vals->value);

This should refer to vals[i] instead.

> +               if (ret < 0)
> +                       return ret;
> +       }
> +       return 0;
> +}
> +
> +static int dw9768_set_position(struct dw9768 *dw9768, u16 val)
> +{
> +       struct i2c_client *client = v4l2_get_subdevdata(&dw9768->sd);
> +       u8 addr[2];
> +
> +       addr[0] = (val >> DW9768_DAC_SHIFT) & DW9768_REG_MASK_MSB;
> +       addr[1] = val & DW9768_REG_MASK_LSB;
> +
> +       return i2c_smbus_write_block_data(client, DW9768_SET_POSITION_ADDR,
> +                                         ARRAY_SIZE(addr), addr);

As we discovered earlier, i2c_smbus_write_block_data() uses a
different protocol from what we expected. Please change to
i2c_smbus_write_word_data(), as per our downstream changes.

> +}
> +
> +static int dw9768_release(struct dw9768 *dw9768)
> +{
> +       return dw9768_write_array(dw9768, dw9768_release_regs,
> +                                 ARRAY_SIZE(dw9768_release_regs));
> +}
> +
> +static int dw9768_init(struct dw9768 *dw9768)
> +{
> +       return dw9768_write_array(dw9768, dw9768_init_regs,
> +                                 ARRAY_SIZE(dw9768_init_regs));
> +}
> +
> +/* Power handling */
> +static int dw9768_power_off(struct dw9768 *dw9768)
> +{
> +       struct i2c_client *client = v4l2_get_subdevdata(&dw9768->sd);
> +       int ret;
> +
> +       ret = dw9768_release(dw9768);
> +       if (ret)
> +               dev_err(&client->dev, "dw9768 release failed!\n");
> +
> +       ret = regulator_disable(dw9768->vin);
> +       if (ret)
> +               return ret;
> +
> +       return regulator_disable(dw9768->vdd);
> +}
> +
> +static int dw9768_power_on(struct dw9768 *dw9768)
> +{
> +       int ret;
> +
> +       ret = regulator_enable(dw9768->vin);
> +       if (ret < 0)
> +               return ret;
> +
> +       ret = regulator_enable(dw9768->vdd);
> +       if (ret < 0)
> +               return ret;

There is at least T_opr = 200 us of delay needed here. Would you be
able to add a comment and a corresponding usleep_range() call? I guess
the range of (300, 400) would be enough on the safe side.

Best regards,
Tomasz

      parent reply	other threads:[~2019-10-09  4:40 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
2019-09-07 22:12   ` kbuild test robot
2019-10-09  4:40   ` Tomasz Figa [this message]

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='CAAFQd5A9mn+695CZdYoXzuQodQqC=p8dstbdFAkOCCHKMUtg-w@mail.gmail.com' \
    --to=tfiga@chromium.org \
    --cc=andriy.shevchenko@linux.intel.com \
    --cc=bingbu.cao@intel.com \
    --cc=devicetree@vger.kernel.org \
    --cc=dongchun.zhu@mediatek.com \
    --cc=drinkcat@chromium.org \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-media@vger.kernel.org \
    --cc=linux-mediatek@lists.infradead.org \
    --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=sam.hung@mediatek.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