Re: [PATCH v9 2/2] Add support for OV5647 sensor.

[Ramiro Oliveira]

Hi Zakari,

On 2/21/2017 8:36 PM, Vladimir Zapolskiy wrote:
> Hi Ramiro,
> 
> On 02/21/2017 06:42 PM, Ramiro Oliveira wrote:
>> Hi Vladimir,
>>
>> Thank you for your feedback
>>
>> On 2/21/2017 3:54 PM, Vladimir Zapolskiy wrote:
>>> Hi Ramiro,
>>>
>>> please find some review comments below.
>>>
>>> On 02/17/2017 03:14 PM, Ramiro Oliveira wrote:
>>>> The OV5647 sensor from Omnivision supports up to 2592x1944 @ 15 fps, RAW 8
>>>> and RAW 10 output formats, and MIPI CSI-2 interface.
>>>>
>>>> The driver adds support for 640x480 RAW 8.
>>>>
>>>> Signed-off-by: Ramiro Oliveira <roliveir@xxxxxxxxxxxx>
>>>> ---
>>>
>>> [snip]
>>>
>>>> +
>>>> +struct ov5647 {
>>>> +	struct v4l2_subdev		sd;
>>>> +	struct media_pad		pad;
>>>> +	struct mutex			lock;
>>>> +	struct v4l2_mbus_framefmt	format;
>>>> +	unsigned int			width;
>>>> +	unsigned int			height;
>>>> +	int				power_count;
>>>> +	struct clk			*xclk;
>>>> +	/* External clock frequency currently supported is 30MHz */
>>>> +	u32				xclk_freq;
>>>
>>> See a comment about 25MHz vs 30MHz below.
>>>
>>> Also I assume you can remove 'xclk_freq' from the struct fields,
>>> it can be replaced by a local variable.
>>>
>>
>> I'll do that.
>>
>>>> +};
>>>
>>> [snip]
>>>
>>>> +
>>>> +static int ov5647_read(struct v4l2_subdev *sd, u16 reg, u8 *val)
>>>> +{
>>>> +	int ret;
>>>> +	unsigned char data_w[2] = { reg >> 8, reg & 0xff };
>>>> +	struct i2c_client *client = v4l2_get_subdevdata(sd);
>>>> +
>>>> +	ret = i2c_master_send(client, data_w, 2);
>>>> +	if (ret < 0) {
>>>> +		dev_dbg(&client->dev, "%s: i2c read error, reg: %x\n",
>>>
>>> s/i2c read error/i2c write error/
>>>
>>
>> I'm not sure I understand what you mean.
> 
> That's a sed expression for string substitution. Here you do i2c_master_send()
> but dev_dbg() comment says "i2c read error". It's a simple copy-paste typo to fix.
> 

Ohh... now I see. I'll change it.

>>>> +			__func__, reg);
>>>> +		return ret;
>>>> +	}
>>>> +
> 
> [snip]
> 
>>>> +
>>>> +static int sensor_power(struct v4l2_subdev *sd, int on)
> 
> On the caller's side (functions ov5647_open() and ov5647_close()) the second
> argument of the function is of 'bool' type, however .s_power callback from
> struct v4l2_subdev_core_ops (see include/media/v4l2-subdev.h) defines it as
> 'int'.
> 
> It's just a nitpicking, please feel free to ignore the comment above or
> please consider to change the arguments on callers' side to integers.
> 
> Also you may consider to add 'ov5647_' prefix to the function name to
> distinguish it from a potentially added in future sensor_power() function,
> the original name sounds too generic.
> 

OK. I'll add the prefix and change the variable type from int to bool.

>>>> +{
>>>> +	int ret;
>>>> +	struct ov5647 *ov5647 = to_state(sd);
>>>> +	struct i2c_client *client = v4l2_get_subdevdata(sd);
>>>> +
>>>> +	ret = 0;
>>>> +	mutex_lock(&ov5647->lock);
>>>> +
>>>> +	if (on && !ov5647->power_count)	{
>>>> +		dev_dbg(&client->dev, "OV5647 power on\n");
>>>> +
>>>> +		clk_set_rate(ov5647->xclk, ov5647->xclk_freq);
>>>
>>> Now clk_set_rate() is redundant, please remove it.
>>>
>>> If once it is needed again, please move it to the .probe function, so
>>> it is called only once in the runtime.
>>>
>>
>> Ok. I'll remove it for now.
>>
>>>> +
>>>> +		ret = clk_prepare_enable(ov5647->xclk);
>>>
>>> I wonder would it be possible to unload the driver or to unbind the device
>>> and leave the clock unintentionally enabled? If yes, then this is a bug.
>>>
>>
>> You're saying that if the driver was unloaded and the clock was left enabled
>> when the driver was loaded again this line would cause an error?
> 
> Not exactly, here I saw a potential resource leak, namely a potentially left
> prepared/enabled clock.
> 
>>
>> Should I disable the clock when the driver is removed?
>>
> 
> The driver (and framework) shall guarantee that when it is detached from
> device(s) (e.g. by unloading "ov5647" kernel module or unbinding ov5647 device),
> all acquired resources are released.
> 
> But in this particular case most probably I've been overly alert, I believe
> that V4L2 framework correcly handles device power states, so please ignore my
> comment.
> 
> To add something valuable to the review, could you please confirm that
> ov5647_subdev_internal_ops data is in use by the driver?
> 
> E.g. shouldn't it be registered by
> 
>   sd->internal_ops = &ov5647_subdev_internal_ops;
> 
> before calling v4l2_async_register_subdev(sd) ?
> 

You're right, it's not being registered. I think I'll remove them since they
aren't being used.

> --
> With best wishes,
> Vladimir
> 

-- 
Best Regards

Ramiro Oliveira
Ramiro.Oliveira@xxxxxxxxxxxx