Re: [PATCH v3 14/18] media: i2c: Add driver for DW9719 VCM

[Andy Shevchenko <andy@kernel.org>]

On Wed, Jul 05, 2023 at 11:30:06PM +0200, Hans de Goede wrote:
> From: Daniel Scally <djrscally@gmail.com>
> 
> Add a driver for the DW9719 VCM. The driver creates a v4l2 subdevice
> and registers a control to set the desired focus.

...

> +/*
> + * Based on linux/modules/camera/drivers/media/i2c/imx/dw9719.c in this repo:

Sakari, also long line? :-)

> + * https://github.com/ZenfoneArea/android_kernel_asus_zenfone5
> + */

...

> +#include <asm/unaligned.h>

Usually we include headers from generic to particular / private,
hence asm/* usually goes after linux/*.

> +#include <linux/delay.h>
> +#include <linux/i2c.h>
> +#include <linux/pm_runtime.h>
> +#include <linux/regulator/consumer.h>
> +#include <linux/types.h>

...

> +#define DW9719_CTRL_DELAY_US	1000

USEC_PER_MSEC ?

...

> +#define DELAY_MAX_PER_STEP_NS	(1000000 * 1023)

NSEC_PER_MSEC ?

...

> +#define DW9719_DEFAULT_VCM_FREQ		0x60

Any comment what this value means in Hz?

...

> +#define to_dw9719_device(x) container_of(x, struct dw9719_device, sd)

You can make this no-op at compile time by...

...

> +struct dw9719_device {
> +	struct device *dev;
> +	struct i2c_client *client;
> +	struct regulator *regulator;

> +	struct v4l2_subdev sd;

...having this to be the first member in the structure.

However bloat-o-meter can show grow of the code in case the dev is used more
often. The rule of thumb is to combine two aspects:
- frequency of usage (hence pointer arithmetics);
- hot path vs. slow path (hence importance of the lesser code).

> +	u32 sac_mode;
> +	u32 vcm_freq;
> +
> +	struct dw9719_v4l2_ctrls {
> +		struct v4l2_ctrl_handler handler;
> +		struct v4l2_ctrl *focus;
> +	} ctrls;
> +};

...

> +static int dw9719_i2c_rd8(struct i2c_client *client, u8 reg, u8 *val)
> +{
> +	struct i2c_msg msg[2];
> +	u8 buf[2] = { reg };
> +	int ret;
> +
> +	msg[0].addr = client->addr;
> +	msg[0].flags = 0;

> +	msg[0].len = 1;
> +	msg[0].buf = buf;

	sizeof(buf[0])
	&buf[0]

looks more explicit.

> +	msg[1].addr = client->addr;
> +	msg[1].flags = I2C_M_RD;
> +	msg[1].len = 1;
> +	msg[1].buf = &buf[1];

Ditto.

> +	*val = 0;
> +
> +	ret = i2c_transfer(client->adapter, msg, 2);

ARRAY_SIZE()

> +	if (ret < 0)
> +		return ret;
> +
> +	*val = buf[1];
> +
> +	return 0;
> +}

But as Sakari said this perhaps could go into CCI library.

...

> +	ret = dw9719_i2c_rd8(dw9719->client, DW9719_INFO, &val);
> +	if (ret < 0)
> +		return ret;
> +
> +	if (val != DW9719_ID) {
> +		dev_err(dw9719->dev, "Failed to detect correct id\n");
> +		ret = -ENXIO;

		return -ENXIO;

> +	}
> +
> +	return 0;

...

> +	/* Need 100us to transit from SHUTDOWN to STANDBY*/

Missing space.

> +	usleep_range(100, 1000);

Perhaps fsleep() would be better, but I'm fine with either here.

...

> +static int dw9719_t_focus_abs(struct dw9719_device *dw9719, s32 value)
> +{
> +	int ret;

Redundant?

> +	value = clamp(value, 0, DW9719_MAX_FOCUS_POS);

> +	ret = dw9719_i2c_wr16(dw9719->client, DW9719_VCM_CURRENT, value);
> +	if (ret < 0)
> +		return ret;
> +
> +	return 0;

	return _wr16(...);

or can it return positive values?

> +}

...

> +static int __maybe_unused dw9719_suspend(struct device *dev)

Can we use new PM macros instead of __maybe_unused?

> +{
> +	struct v4l2_subdev *sd = dev_get_drvdata(dev);
> +	struct dw9719_device *dw9719 = to_dw9719_device(sd);
> +	int ret;
> +	int val;
> +
> +	for (val = dw9719->ctrls.focus->val; val >= 0;
> +	     val -= DW9719_CTRL_STEPS) {
> +		ret = dw9719_t_focus_abs(dw9719, val);
> +		if (ret)
> +			return ret;

> +		usleep_range(DW9719_CTRL_DELAY_US, DW9719_CTRL_DELAY_US + 10);

fsleep() ?

> +	}
> +
> +	return dw9719_power_down(dw9719);
> +}

> +static int __maybe_unused dw9719_resume(struct device *dev)
> +{

As per above function.

...

> +err_power_down:

In one functions you use err_ in another fail_, be consistent.

> +	dw9719_power_down(dw9719);
> +	return ret;
> +}

...

> +	dw9719->regulator = devm_regulator_get(&client->dev, "vdd");
> +	if (IS_ERR(dw9719->regulator))
> +		return dev_err_probe(&client->dev, PTR_ERR(dw9719->regulator),

With

	struct device *dev = &client->dev;

code may look neater.

> +				     "getting regulator\n");

-- 
With Best Regards,
Andy Shevchenko