On Thu, Sep 05, 2019 at 03:21:42PM +0800, dongchun.zhu@xxxxxxxxxxxx wrote:
> From: Dongchun Zhu <dongchun.zhu@xxxxxxxxxxxx>
>
> 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@xxxxxxxxxxxx>
> ---
> 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.
> +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.
> + 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() ?
> + 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.
> +};
> +static const struct of_device_id dw9768_of_table[] = {
> + { .compatible = "dongwoon,dw9768" },
> + { },
Ditto.
> +};
--
With Best Regards,
Andy Shevchenko
