On 02/22/2017 12:51 PM, Ramiro Oliveira wrote: > 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 <roliv...@synopsys.com> >>>>> --- >>>> >>>> [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. >
Just to eliminate any potential misunderstanding, if you consider to reuse the current sensor_power() function, please change variables from bool to int on a caller's side, the signature of the function shall not be changed to match .s_power type. >>>>> +{ >>>>> + 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