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
