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 -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html