Hi Dongchun,
On Thu, Sep 5, 2019 at 4:22 PM <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 +
> drivers/media/i2c/Kconfig | 10 ++
> drivers/media/i2c/Makefile | 1 +
> drivers/media/i2c/dw9768.c | 349 +++++++++++++++++++++++++++++++++++++++++++++
> 4 files changed, 361 insertions(+)
> create mode 100644 drivers/media/i2c/dw9768.c
>
Please see my further comments inline.
[snip]
> +struct regval_list {
> + unsigned char reg_num;
> + unsigned char value;
nit: Since we have strictly sized values here, should we use u8 for
both fields instead?
> +};
> +
> +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)
Should we use u8 for the last two arguments here as well?
> +{
> + struct i2c_client *client = v4l2_get_subdevdata(&dw9768->sd);
> + int ret;
> +
> + if (reg == DW9768_CMD_DELAY && value == DW9768_CMD_DELAY)
> + usleep_range(DW9768_CTRL_DELAY_US,
> + DW9768_CTRL_DELAY_US + 100);
ret will be uninitialized if we go this path.
> + else
> + ret = i2c_smbus_write_byte_data(client, reg, value);
> + return ret;
> +}
> +
> +static int dw9768_write_array(struct dw9768 *dw9768, struct regval_list *vals,
> + u32 len)
Since len is an array size, should we use size_t instead?
> +{
> + unsigned int i;
size_t?
> + int ret;
> +
> + for (i = 0; i < len; i++) {
> + ret = dw9768_write_smbus(dw9768, vals->reg_num, vals->value);
This should refer to vals[i] instead.
> + if (ret < 0)
> + return ret;
> + }
> + return 0;
> +}
> +
> +static int dw9768_set_position(struct dw9768 *dw9768, u16 val)
> +{
> + struct i2c_client *client = v4l2_get_subdevdata(&dw9768->sd);
> + u8 addr[2];
> +
> + addr[0] = (val >> DW9768_DAC_SHIFT) & DW9768_REG_MASK_MSB;
> + addr[1] = val & DW9768_REG_MASK_LSB;
> +
> + return i2c_smbus_write_block_data(client, DW9768_SET_POSITION_ADDR,
> + ARRAY_SIZE(addr), addr);
As we discovered earlier, i2c_smbus_write_block_data() uses a
different protocol from what we expected. Please change to
i2c_smbus_write_word_data(), as per our downstream changes.
> +}
> +
> +static int dw9768_release(struct dw9768 *dw9768)
> +{
> + return dw9768_write_array(dw9768, dw9768_release_regs,
> + ARRAY_SIZE(dw9768_release_regs));
> +}
> +
> +static int dw9768_init(struct dw9768 *dw9768)
> +{
> + return dw9768_write_array(dw9768, dw9768_init_regs,
> + ARRAY_SIZE(dw9768_init_regs));
> +}
> +
> +/* Power handling */
> +static int dw9768_power_off(struct dw9768 *dw9768)
> +{
> + struct i2c_client *client = v4l2_get_subdevdata(&dw9768->sd);
> + int ret;
> +
> + ret = dw9768_release(dw9768);
> + if (ret)
> + dev_err(&client->dev, "dw9768 release failed!\n");
> +
> + ret = regulator_disable(dw9768->vin);
> + if (ret)
> + return ret;
> +
> + return regulator_disable(dw9768->vdd);
> +}
> +
> +static int dw9768_power_on(struct dw9768 *dw9768)
> +{
> + int ret;
> +
> + ret = regulator_enable(dw9768->vin);
> + if (ret < 0)
> + return ret;
> +
> + ret = regulator_enable(dw9768->vdd);
> + if (ret < 0)
> + return ret;
There is at least T_opr = 200 us of delay needed here. Would you be
able to add a comment and a corresponding usleep_range() call? I guess
the range of (300, 400) would be enough on the safe side.
Best regards,
Tomasz
