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

Andy Shevchenko
2023-07-06T10:06:16Z

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