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? :-) > + * 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
