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@xxxxxxxxx> >> >> 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
