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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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



[Index of Archives]     [Linux Input]     [Video for Linux]     [Gstreamer Embedded]     [Mplayer Users]     [Linux USB Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [Yosemite Backpacking]

  Powered by Linux