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

From: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
To: Dongchun Zhu <dongchun.zhu@mediatek.com>
Cc: linus.walleij@linaro.org, bgolaszewski@baylibre.com,
	mchehab@kernel.org, robh+dt@kernel.org, mark.rutland@arm.com,
	sakari.ailus@linux.intel.com, drinkcat@chromium.org,
	tfiga@chromium.org, matthias.bgg@gmail.com, bingbu.cao@intel.com,
	srv_heupstream@mediatek.com, linux-mediatek@lists.infradead.org,
	linux-arm-kernel@lists.infradead.org, sj.huang@mediatek.com,
	linux-media@vger.kernel.org, devicetree@vger.kernel.org,
	louis.kuo@mediatek.com, shengnan.wang@mediatek.com
Subject: Re: [V7, 2/2] media: i2c: dw9768: Add DW9768 VCM driver
Date: Fri, 5 Jun 2020 15:46:43 +0300	[thread overview]
Message-ID: <20200605124643.GG2428291@smile.fi.intel.com> (raw)
In-Reply-To: <20200605105412.18813-3-dongchun.zhu@mediatek.com>

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