On Fri, Jun 05, 2020 at 06:54:12PM +0800, Dongchun Zhu wrote:
> Add a V4L2 sub-device driver for DW9768 voice coil motor, providing
> control to set the desired focus via IIC serial interface.
...
> +config VIDEO_DW9768
> + tristate "DW9768 lens voice coil support"
> + depends on I2C && VIDEO_V4L2
No compile test?
> + depends on PM
This is very strange dependency for ordinary driver.
> + select MEDIA_CONTROLLER
> + select VIDEO_V4L2_SUBDEV_API
> + select V4L2_FWNODE
...
> +/*
> + * DW9768 requires waiting time (delay time) of t_OPR after power-up,
> + * or in the case of PD reset taking place.
> + */
> +#define DW9768_T_OPR_US 1000
> +#define DW9768_Tvib_MS_BASE10 (64 - 1)
> +#define DW9768_AAC_MODE_DEFAULT 2
> +#define DW9768_AAC_TIME_DEFAULT 0x20
Hex? Why not decimal?
> +#define DW9768_CLOCK_PRE_SCALE_DEFAULT 1
...
> +static int dw9768_mod_reg(struct dw9768 *dw9768, u8 reg, u8 mask, u8 val)
> +{
> + struct i2c_client *client = v4l2_get_subdevdata(&dw9768->sd);
> + int ret;
> +
> + ret = i2c_smbus_read_byte_data(client, reg);
> + if (ret < 0)
> + return ret;
> +
> + val = ((unsigned char)ret & ~mask) | (val & mask);
This cast is weird.
> +
> + return i2c_smbus_write_byte_data(client, reg, val);
> +}
...
> + dev_err(&client->dev, "%s I2C failure: %d",
> + __func__, ret);
One line?
...
> +static int dw9768_release(struct dw9768 *dw9768)
> +{
> + struct i2c_client *client = v4l2_get_subdevdata(&dw9768->sd);
> + u32 move_delay_us = dw9768_cal_move_delay(dw9768->aac_mode,
> + dw9768->clock_presc,
> + dw9768->aac_timing) / 100;
> + int ret, val;
> +
> + val = round_down(dw9768->focus->val, DW9768_MOVE_STEPS);
> + for ( ; val >= 0; val -= DW9768_MOVE_STEPS) {
> + ret = dw9768_set_dac(dw9768, val);
> + if (ret) {
> + dev_err(&client->dev, "I2C write fail: %d", ret);
> + return ret;
> + }
> + usleep_range(move_delay_us, move_delay_us + 1000);
> + }
It will look more naturally in the multiplier kind of value.
unsigned int steps = DIV_ROUND_UP(...);
while (steps--) {
...(..., steps * ..._MOVE_STEPS);
...
}
but double check arithmetics.
> + return 0;
> +}
Also it seems we need to have writex_poll_timeout() implementation (see
iopoll.h).
--
With Best Regards,
Andy Shevchenko
