Re: [V7, 2/2] media: i2c: dw9768: Add DW9768 VCM driver

Andy Shevchenko
2020-06-05T12:47:06Z

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