Re: [RESEND PATCH v7 2/2] media: dw9807: Add dw9807 vcm driver

[Tomasz Figa <tfiga@chromium.org>]

Hi Andy, Alan,

On Wed, Apr 11, 2018 at 12:41 AM Andy Yeh <andy.yeh@intel.com> wrote:

> From: Alan Chiang <alanx.chiang@intel.com>

> DW9807 is a 10 bit DAC from Dongwoon, designed for linear
> control of voice coil motor.

> This driver creates a V4L2 subdevice and
> provides control to set the desired focus.

Please see my comments inline.

[snip]
> +static int dw9807_i2c_check(struct i2c_client *client)
> +{
> +       const char status_addr = DW9807_STATUS_ADDR;
> +       char status_result;
> +       int ret;
> +
> +       ret = i2c_master_send(client, (const char *)&status_addr,
> +               sizeof(status_addr));

Why is this cast needed? status_addr is const char already, so
&status_addr, should be const char *.

> +       if (ret < 0) {
> +               dev_err(&client->dev, "I2C write STATUS address fail ret
= %d\n",
> +                       ret);
> +               return ret;
> +       }
> +
> +       ret = i2c_master_recv(client, (char *)&status_result,

Shouldn't need this cast.

> +               sizeof(status_result));
> +       if (ret != sizeof(status_result)) {
> +               dev_err(&client->dev, "I2C read STATUS value fail
ret=%d\n",
> +                       ret);
> +               return ret;
> +       }
> +
> +       return status_result;
> +}
> +
> +static int dw9807_set_dac(struct i2c_client *client, u16 data)
> +{
> +       const char tx_data[3] = {
> +               DW9807_MSB_ADDR, ((data >> 8) & 0x03), (data & 0xff)
> +       };
> +       int ret, retry = 0;
> +
> +       /*
> +        * According to the datasheet, need to check the bus status
before we
> +        * write VCM position. This ensure that we really write the value
> +        * into the register
> +        */
> +       while ((ret = dw9807_i2c_check(client)) != 0) {
> +               if (ret < 0)
> +                       return ret;
> +
> +               if (MAX_RETRY == ++retry) {
> +                       dev_err(&client->dev,
> +                               "Cannot do the write operation because
VCM is busy\n");
> +                       return -EIO;
> +               }
> +               usleep_range(DW9807_CTRL_DELAY_US, DW9807_CTRL_DELAY_US +
10);
> +       }

One could use readx_poll_timeout() here:

int val;

ret = readx_poll_timeout(dw9807_i2c_check, client, val, !val,
                          DW9807_CTRL_DELAY_US,
                          MAX_RETRY * DW9807_CTRL_DELAY_US);

if (ret) {
         dev_err(&client->dev,
                 "Cannot do the write operation because VCM is busy\n");
         return -EIO;
}

> +
> +       /* Write VCM position to registers */
> +       ret = i2c_master_send(client, tx_data, sizeof(tx_data));
> +       if (ret != sizeof(tx_data)) {
> +               if (ret < 0) {
> +                       dev_err(&client->dev,
> +                               "I2C write MSB fail ret=%d\n", ret);
> +                       return ret;
> +               } else {

I believe this can't happen, both by looking at implementation of
i2c_master_send() and existing drivers checking only for (ret < 0).

> +                       dev_err(&client->dev, "I2C write MSB fail,
transmission size is not equal the size expected\n");
> +                       return -EIO;
> +               }
> +       }
> +
> +       return 0;
> +}
[snip]
> +static const struct of_device_id dw9807_of_table[] = {
> +       { .compatible = "dongwoon,dw9807" },
> +       { { 0 } }

It looks like we may need other changes, so I'd go with

{ /* sentinel */ },

here. That's (+/- the comment) what other drivers use normally.

Best regards,
Tomasz