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

Hans de Goede
2023-07-06T14:35:28Z

Hi,

On 7/6/23 12:06, Andy Shevchenko wrote:
> 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? :-)

Nope, this is 79 chars.

>
>> + * 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/*.

Ack (gone after switching to CCI helpers in next version).

>> +#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 ?

I don't see how that helps readability.

>
> ...
>
>> +#define DELAY_MAX_PER_STEP_NS (1000000 * 1023)
>
> NSEC_PER_MSEC ?

This define is not used so I've dropped it for the next version.

>
> ...
>
>> +#define DW9719_DEFAULT_VCM_FREQ 0x60
>
> Any comment what this value means in Hz?

This comes directly from the Android driver, no idea what this actually means (no datasheet).

>
> ...
>
>> +#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.

Ack.

> 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.

Right, this is all gone after switching to the new CCI helpers.

>
> ...
>
>> + 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.

fsleep() indeed is better 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?

Ack, fixed.

>
>> +}
>
> ...
>
>> +static int __maybe_unused dw9719_suspend(struct device *dev)
>
> Can we use new PM macros instead of __maybe_unused?

Ack, fixed.

>> +{
>> + 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() ?

fsleep would expand to:

usleep_range(DW9719_CTRL_DELAY_US, 2 * DW9719_CTRL_DELAY_US);

making the loop take up to twice as long.

>
>> + }
>> +
>> + 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.

Ack, fixed.

Regards,

Hans