Hi Andy, On Thu, Jul 06, 2023 at 01:06:07PM +0300, Andy Shevchenko wrote: ... > > +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. The original seems fine to me. Note that this code will disappear soon. Same for the other comments regarding register access functions (apart from the return one). > > > + 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); This is redundant, too, btw., the control framework already handles this. > > > + 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. I prefer it as-is. > > > + "getting regulator\n"); > -- Kind regards, Sakari Ailus