Hi Jacopo,
Thank you for your prompt review.
On 23.03.2018 15:40, jacopo mondi wrote:
> Hi Todor,
> thanks for the patch.
>
> When running checkpatch --strict I see a few warning you can easily
> close (braces indentation mostly, and one additional empty line at
> line 1048).
Thank you for pointing me to the --strict mode. There are a few CHECKs for
braces alignment for which the alignment is still better as it is now
I think. However there were also a few reasonable points and I have
updated the code according to them.
>
> A few more nits below.
>
> On Fri, Mar 23, 2018 at 12:14:20PM +0800, Todor Tomov wrote:
>> The ov7251 sensor is a 1/7.5-Inch B&W VGA (640x480) CMOS Digital Image
>> Sensor from Omnivision.
>>
>> The driver supports the following modes:
>> - 640x480 30fps
>> - 640x480 60fps
>> - 640x480 90fps
>>
>> Output format is MIPI RAW 10.
>>
>> The driver supports configuration via user controls for:
>> - exposure and gain;
>> - horizontal and vertical flip;
>> - test pattern.
>>
>> Signed-off-by: Todor Tomov <todor.tomov@xxxxxxxxxx>
>> ---
>> drivers/media/i2c/Kconfig | 13 +
>> drivers/media/i2c/Makefile | 1 +
>> drivers/media/i2c/ov7251.c | 1494 ++++++++++++++++++++++++++++++++++++++++++++
>> 3 files changed, 1508 insertions(+)
>> create mode 100644 drivers/media/i2c/ov7251.c
>>
>> diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig
>> index 541f0d28..89aecb3 100644
>> --- a/drivers/media/i2c/Kconfig
>> +++ b/drivers/media/i2c/Kconfig
>> @@ -688,6 +688,19 @@ config VIDEO_OV5695
>> To compile this driver as a module, choose M here: the
>> module will be called ov5695.
>>
>> +config VIDEO_OV7251
>> + tristate "OmniVision OV7251 sensor support"
>> + depends on OF
>> + depends on I2C && VIDEO_V4L2 && VIDEO_V4L2_SUBDEV_API
>> + depends on MEDIA_CAMERA_SUPPORT
>> + select V4L2_FWNODE
>> + ---help---
>> + This is a Video4Linux2 sensor-level driver for the OmniVision
>> + OV7251 camera.
>> +
>> + To compile this driver as a module, choose M here: the
>> + module will be called ov7251.
>> +
>> config VIDEO_OV772X
>> tristate "OmniVision OV772x sensor support"
>> depends on I2C && VIDEO_V4L2
>> diff --git a/drivers/media/i2c/Makefile b/drivers/media/i2c/Makefile
>> index ea34aee..c8585b1 100644
>> --- a/drivers/media/i2c/Makefile
>> +++ b/drivers/media/i2c/Makefile
>> @@ -70,6 +70,7 @@ obj-$(CONFIG_VIDEO_OV5647) += ov5647.o
>> obj-$(CONFIG_VIDEO_OV5670) += ov5670.o
>> obj-$(CONFIG_VIDEO_OV5695) += ov5695.o
>> obj-$(CONFIG_VIDEO_OV6650) += ov6650.o
>> +obj-$(CONFIG_VIDEO_OV7251) += ov7251.o
>> obj-$(CONFIG_VIDEO_OV7640) += ov7640.o
>> obj-$(CONFIG_VIDEO_OV7670) += ov7670.o
>> obj-$(CONFIG_VIDEO_OV772X) += ov772x.o
>> diff --git a/drivers/media/i2c/ov7251.c b/drivers/media/i2c/ov7251.c
>> new file mode 100644
>> index 0000000..7b401a9
>> --- /dev/null
>> +++ b/drivers/media/i2c/ov7251.c
>> @@ -0,0 +1,1494 @@
<snip>
>> +static int ov7251_s_power(struct v4l2_subdev *sd, int on)
>> +{
>> + struct ov7251 *ov7251 = to_ov7251(sd);
>> + int ret = 0;
>> +
>> + mutex_lock(&ov7251->power_lock);
>> +
>> + /*
>> + * If the power state is modified from 0 to 1 or from 1 to 0,
>> + * update the power state.
>> + */
>> + if (ov7251->power_on == !on) {
>
> if (ov7251->power_on == !!on) {
> mutex_unlock(&ov7251->power_lock);
> return 0;
> }
>
> And you can save one indentation level and remove ret initialization.
>
Good hint, I'd rather save one indentation level by:
if (ov7251->power_on == !!on)
goto exit;
>
>> + if (on) {
>> + ret = ov7251_set_power_on(ov7251);
>> + if (ret < 0)
>> + goto exit;
>> +
>> + ret = ov7251_set_register_array(ov7251,
>> + ov7251_global_init_setting,
>> + ARRAY_SIZE(ov7251_global_init_setting));
>> + if (ret < 0) {
>> + dev_err(ov7251->dev,
>> + "could not set init registers\n");
>> + ov7251_set_power_off(ov7251);
>> + goto exit;
>> + }
>> +
>> + ov7251->power_on = true;
>> + } else {
>> + ov7251_set_power_off(ov7251);
>> + ov7251->power_on = false;
>> + }
>> + }
>> +
>> +exit:
>> + mutex_unlock(&ov7251->power_lock);
>> +
>> + return ret;
>> +}
>> +
<snip>
>> +
>> +static int ov7251_enum_frame_size(struct v4l2_subdev *subdev,
>> + struct v4l2_subdev_pad_config *cfg,
>> + struct v4l2_subdev_frame_size_enum *fse)
>> +{
>
> Shouldn't you check for (pad != 0) in all subdev pad operations?
> I see other driver with a single pad doing this...
I looked up now and I can see that this is checked in v4l2-subdev.c
in subdev_do_ioctl() before the driver's callback is called.
>
>
>> + if (fse->code != MEDIA_BUS_FMT_SBGGR10_1X10)
>> + return -EINVAL;
>> +
>> + if (fse->index >= ARRAY_SIZE(ov7251_mode_info_data))
>> + return -EINVAL;
>> +
>> + fse->min_width = ov7251_mode_info_data[fse->index].width;
>> + fse->max_width = ov7251_mode_info_data[fse->index].width;
>> + fse->min_height = ov7251_mode_info_data[fse->index].height;
>> + fse->max_height = ov7251_mode_info_data[fse->index].height;
>> +
>> + return 0;
>> +}
>> +
<snip>
>> +
>> +static const struct i2c_device_id ov7251_id[] = {
>> + { "ov7251", 0 },
>> + {}
>> +};
>> +MODULE_DEVICE_TABLE(i2c, ov7251_id);
>> +
>> +static const struct of_device_id ov7251_of_match[] = {
>> + { .compatible = "ovti,ov7251" },
>> + { /* sentinel */ }
>> +};
>> +MODULE_DEVICE_TABLE(of, ov7251_of_match);
>> +
>> +static struct i2c_driver ov7251_i2c_driver = {
>> + .driver = {
>> + .of_match_table = of_match_ptr(ov7251_of_match),
>> + .name = "ov7251",
>> + },
>> + .probe = ov7251_probe,
>> + .remove = ov7251_remove,
>> + .id_table = ov7251_id,
>
> As this driver depends on CONFIG_OF, I've been suggested to use probe_new and
> get rid of i2c id_tables.
Yes, I'll do that.
>
> With the above nits clarified, and as you addressed my v1 comments:
>
> Reviewed-by: Jacopo Mondi <jacopo@xxxxxxxxxx>
Would you like to see the corrections or I can add the tag before sending them?
>
> Thanks
> j
>
>> +};
>> +
>> +module_i2c_driver(ov7251_i2c_driver);
>> +
>> +MODULE_DESCRIPTION("Omnivision OV7251 Camera Driver");
>> +MODULE_AUTHOR("Todor Tomov <todor.tomov@xxxxxxxxxx>");
>> +MODULE_LICENSE("GPL v2");
--
Best regards,
Todor Tomov
