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,

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




[Index of Archives]     [Linux IBM ACPI]     [Linux Power Management]     [Linux Kernel]     [Linux Laptop]     [Kernel Newbies]     [Share Photos]     [Security]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Samba]     [Video 4 Linux]     [Device Mapper]     [Linux Resources]
  Powered by Linux