Hi Andy, Alan,
On Wed, Apr 11, 2018 at 12:41 AM Andy Yeh <andy.yeh@xxxxxxxxx> wrote:
> From: Alan Chiang <alanx.chiang@xxxxxxxxx>
> 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
